From 8e2d2c6af027eb0c9ab6aef3d6518a37b7ca2ed4 Mon Sep 17 00:00:00 2001 From: SEUNGWOO LEE <69357689+lifrary@users.noreply.github.com> Date: Mon, 31 Aug 2026 02:04:23 +0900 Subject: [PATCH 1/6] fix(responses): bound the durable spill directory with an aggregate byte cap The response store has an unconditional RAM ceiling (MAX_STORED_RESPONSE_BYTES, 64 MiB) and demotes the oldest resident entry to a durable spill once it is crossed. Nothing bounded where those bytes landed: the spilled set was capped only per file (MAX_RESPONSE_SPILL_PAYLOAD_BYTES, 256 MiB) and per entry (MAX_STORED_RESPONSES, 1000). Their product is 250 GiB, larger than the disk of any host this runs on, so the only effective bound was RESPONSE_TTL_MS and disk use became a function of client request rate rather than of anything this process controls. Measured on one macOS host, 2026-08-30: a client spilling ~150 MB payloads at ~1.4/min held 6.8 GB of ~/.opencodex/responses-state-spill after 44 minutes and was still climbing toward the ~12 GB an hour-long window implies. It filled the volume, at which point unrelated processes began failing with ENOSPC. Retention itself was correct throughout - the TTL evicted that whole cohort an hour later - so this is a missing budget, not a leak. Add MAX_SPILLED_RESPONSE_BYTES (1 GiB), enforced by one function, enforceSpilledResponseBudget, with three callers: mutation pruning, the lazy load that follows a restart, and the periodic sweep. The periodic caller is not redundant. The mutation path runs only when traffic arrives, so a process that comes up over budget - from a snapshot written under a larger ceiling, or a build that lowered it - would otherwise stay over while idle. That was observed here at 1.8 GiB against a 1 GiB cap, held until the first request. sweepExpiredResponseStates still returns its TTL count, so its existing contract is unchanged. The ceiling bounds what the store can account for: every entry in the map plus the superseded generations queued in pendingSpillUnlinks, whose files stay on disk until a snapshot flush drains them and would otherwise let up to 32 GiB sit outside the budget while it reported itself satisfied. Over budget those deferred generations are released before any live entry, which is the same trade the queue's own overflow path already makes against unbounded disk. Spill files orphaned by a crash are absent from the map, so this accounting can neither see nor price them; they remain with recoverOrphanedResponseSpills and its grace window, and structure/02 now states that allowance and its bound explicitly. Eviction of live entries is ordered by createdAt, not by map order. `states` is not an age index: demotion and spill replacement delete and reinsert entries, and writeBoundedSnapshot serializes the map reversed, so map order can put a newer continuation first. createdAt is millisecond-resolution and ties are ordinary under load, where a stable sort would fall back to insertion order, so ties break on the response id by direct comparison rather than localeCompare, since the order must not depend on the host locale. The total is recomputed per enforcement rather than carried as a running counter: spilled entries reach `states` through several insertion paths (demotion swap, direct oversized admission, snapshot reload), and one missed increment there would silently disable the cap, where a walk over at most MAX_STORED_RESPONSES entries cannot drift. 1 GiB comes from the same sample (n=31), whose spilled sizes are strongly bimodal: median 1.1 MiB against a p90 of 198.7 MiB. At that median the count cap and this ceiling bind within 8% of each other (1000 x 1.1 MiB = 1.07 GiB), so ordinary traffic sees no eviction it would not already have seen and only the large tail is cut. The value is the one knob here a maintainer may reasonably want to change. Six regressions, each confirmed to fail without the code it covers: the budget is enforced and the oldest spill is the one removed; eviction follows createdAt rather than insertion order; ties break on the id; a single payload larger than the whole budget leaves the store usable rather than wedged; deferred generations count against the cap and drain first; and an over-budget snapshot is reclaimed with no continuation mutation at all - a read drives the load path and a later tick drives the periodic one, with the newest entry surviving and still replaying. Co-Authored-By: Claude Opus 5 (1M context) --- src/responses/state.ts | 117 ++++++++++++++++++- structure/00_overview.md | 2 +- structure/02_config-and-codex-home.md | 28 +++++ tests/responses-state.test.ts | 162 ++++++++++++++++++++++++++ 4 files changed, 307 insertions(+), 2 deletions(-) diff --git a/src/responses/state.ts b/src/responses/state.ts index 35540a0ee7..902a429ee3 100644 --- a/src/responses/state.ts +++ b/src/responses/state.ts @@ -35,6 +35,30 @@ const SNAPSHOT_DEBOUNCE_MAX_MS = 30_000; * continuation chains) stores the full expanded input each turn — ~quadratic bytes per chain — * so a count cap alone cannot bound memory. Oldest-first eviction applies past this mark. */ export const MAX_STORED_RESPONSE_BYTES = 64 * 1024 * 1024; +/** + * Aggregate ceiling for the durable spill directory: the disk-side counterpart to + * the RAM ceiling above. Without it the spilled set is bounded only per-file + * (MAX_RESPONSE_SPILL_PAYLOAD_BYTES, 256 MiB) and per-entry (MAX_STORED_RESPONSES, + * 1000), whose product is 250 GiB — larger than the disk of any host this runs on. + * The only effective bound was therefore RESPONSE_TTL_MS, which makes disk use a + * function of client request rate rather than of anything this process controls. + * + * Measured on one macOS host, 2026-08-30: a client spilling ~150 MB payloads at + * ~1.4/min held 6.8 GB after 44 minutes, still climbing toward the ~12 GB an + * hour-long window implies, and filled the volume. Retention itself was correct + * throughout — the TTL evicted that whole cohort an hour later — so what was + * missing is a budget, not a sweep. + * + * 1 GiB comes from the same sample (n=31), whose spilled sizes are strongly + * bimodal: median 1.1 MiB against a p90 of 198.7 MiB, near the per-file ceiling. + * At that median the count cap and this ceiling bind within 8% of each other + * (1000 x 1.1 MiB = 1.07 GiB), so ordinary traffic sees no eviction it would not + * already have seen and only the large tail is cut. Erring small is the safe + * direction: too low costs a replay miss, an already-handled path surfaced as + * previous_response_not_found, while too high costs the host's disk and every + * unrelated process on it. + */ +export const MAX_SPILLED_RESPONSE_BYTES = 1024 * 1024 * 1024; /** Legacy snapshot selection only. Spill demotion is governed solely by the RAM cap above. */ const SNAPSHOT_ENTRY_MAX_BYTES = 2 * 1024 * 1024; const SNAPSHOT_TOTAL_MAX_BYTES = 24 * 1024 * 1024; @@ -603,6 +627,41 @@ export function getStoredResponseBytesForTests(): number { return storedResponseBytes; } +let spillByteCapOverride: number | null = null; + +function spillByteCap(): number { + return spillByteCapOverride ?? MAX_SPILLED_RESPONSE_BYTES; +} + +/** + * Live total of durable spill payloads. Recomputed per call rather than carried as + * a running counter: spilled entries reach `states` through several insertion paths + * (demotion swap, direct oversized admission, snapshot reload), and one missed + * increment there would silently disable the cap, where an O(MAX_STORED_RESPONSES) + * walk cannot drift. + */ +function spilledResponseBytes(): number { + let total = 0; + for (const entry of states.values()) { + if (entry.kind === "spill") total += entry.spill.payloadBytes; + } + // Superseded generations awaiting a durable snapshot are still files on disk. + // Counting only `states` would let PENDING_SPILL_UNLINKS_MAX of them sit outside + // the budget while it reports itself satisfied. + for (const ref of pendingSpillUnlinks) total += ref.payloadBytes; + return total; +} + +/** Test-only: lower/restore the durable spill cap (null restores the default). */ +export function setSpilledResponseByteCapForTests(bytes: number | null): void { + spillByteCapOverride = bytes; +} + +/** Test-only: current durable spill accounting (proves evictions unlink their files). */ +export function getSpilledResponseBytesForTests(): number { + return spilledResponseBytes(); +} + function serializedBytes(value: unknown): number | null { try { const serialized = JSON.stringify(value); @@ -1495,6 +1554,58 @@ export function replayOverlapSkipsForTests(): number { return replayOverlapSkips; } +/** + * Bring the durable spill set inside MAX_SPILLED_RESPONSE_BYTES, and report the + * bytes released. + * + * One owner, three callers: mutation pruning, the lazy load that follows a + * restart, and the periodic sweep. The periodic caller is not redundant — the + * mutation path only runs when traffic arrives, and a process can come up over + * budget from a snapshot written under a larger ceiling and then sit idle. That + * was observed in production at 1.8 GiB against a 1 GiB cap, held until the first + * request. + * + * NOT covered here: spill files orphaned by a crash. They are absent from + * `states`, so this function can neither see nor price them, and they stay with + * recoverOrphanedResponseSpills and its RESPONSE_SPILL_ORPHAN_GRACE_MS window. + * This ceiling therefore bounds what the store owns, which is every file it can + * account for, and not the directory as a whole. + */ +function enforceSpilledResponseBudget(): number { + let spilledBytes = spilledResponseBytes(); + if (spilledBytes <= spillByteCap()) return 0; + const before = spilledBytes; + // Deferred generations go first. They are already superseded, so releasing one + // costs only the crash window the queue exists to cover — the same trade + // PENDING_SPILL_UNLINKS_MAX already makes against unbounded disk. Evicting a + // live continuation to make room for a dead file would be the wrong order. + while (spilledBytes > spillByteCap() && pendingSpillUnlinks.length > 0) { + const ref = pendingSpillUnlinks.shift()!; + spilledBytes -= ref.payloadBytes; + deleteResponseSpill(ref); + } + // Ordered by createdAt, not by map order. `states` is not an age index: + // demotion and spill replacement delete and reinsert entries, and + // writeBoundedSnapshot serializes the map reversed, so map order can put a + // newer continuation first — and evicting that one spends a resume the older + // entry would not have cost. Sorting is O(k log k) over the spilled subset and + // runs only on a tick already over budget. + const spilled = [...states] + .filter((pair): pair is [string, SpilledResponseState] => pair[1].kind === "spill") + // createdAt is millisecond-resolution, so ties are ordinary under load. A + // stable sort would then fall back to insertion order — the very order this + // is avoiding — so break ties on the response id. Not localeCompare: the + // order must not depend on the host locale. + .sort((a, b) => a[1].createdAt - b[1].createdAt + || (a[0] < b[0] ? -1 : a[0] > b[0] ? 1 : 0)); + for (const [id, entry] of spilled) { + if (spilledBytes <= spillByteCap()) break; + spilledBytes -= entry.spill.payloadBytes; + deleteEntry(id); + } + return before - spilledBytes; +} + function pruneResponses(at = now()): void { for (const [id, state] of states) { if (at - state.createdAt > RESPONSE_TTL_MS) deleteEntry(id); @@ -1537,6 +1648,7 @@ function pruneResponses(at = now()): void { replaceWithSpillFailure(oldestId, entry); } } + enforceSpilledResponseBudget(); } /** Periodic TTL-only sweep; count/byte eviction remains owned by mutation paths. */ @@ -1547,7 +1659,10 @@ export function sweepExpiredResponseStates(at = now()): number { deleteEntry(id); removed += 1; } - if (removed > 0) schedulePersist(); + // The disk ceiling needs a caller that does not depend on traffic. The return + // value stays the TTL count so this function's existing contract is unchanged. + const reclaimed = enforceSpilledResponseBudget(); + if (removed > 0 || reclaimed > 0) schedulePersist(); return removed; } diff --git a/structure/00_overview.md b/structure/00_overview.md index d489fb293b..d1f864c4c6 100644 --- a/structure/00_overview.md +++ b/structure/00_overview.md @@ -84,7 +84,7 @@ opencodex state root does not undo those writes. Putting native Codex back is th | `~/.opencodex/ocx.pid`, `runtime-port.json`, `system-env-port` | opencodex runtime | Live process identity and the port a client should reach; rewritten on start. `runtime-port.json` also carries the protected per-process listener-attestation key used before CLI diagnostics attach a management bearer. | | `~/.opencodex/codex-runtime.json`, `codex-runtime-clamp.json` | opencodex Codex runtime | Selected Codex executable/version state and effort-clamp diagnostics. Not process identity: these persist a resolved choice and a diagnostic, so losing them changes behavior until re-resolved. | | `~/.opencodex/service-state.json`, `service.log`, `service-api-token`, `opencodex-service-launcher.vbs`, `opencodex-service-task.xml`, `opencodex-service.cmd`, `winsw`, `tray-state.json`, `tray-heartbeat.json`, `opencodex-tray.ps1`, `opencodex-tray-*.ico`, `update-job.json` | opencodex operators | Installed-service, Windows tray, and self-update artifacts and bookkeeping. The update record carries its worker PID so a dead worker recovers instead of blocking later runs. | -| `~/.opencodex/responses-state.json`, `usage-debug.jsonl`, `crash.log`, `artifacts/` | opencodex diagnostics and artifacts | Bounded caches, diagnostics, and generated image/video artifacts served locally. | +| `~/.opencodex/responses-state.json`, `responses-state-spill/`, `usage-debug.jsonl`, `crash.log`, `artifacts/` | opencodex diagnostics and artifacts | Bounded caches, diagnostics, and generated image/video artifacts served locally. The spill directory holds continuation state demoted out of the in-memory cap and is bounded in aggregate, not only per file. | | `~/.opencodex/codex-shim.json`, `*.lock`, `kimi-device-id`, `mimo-client-id`, `.star-prompted` | opencodex bookkeeping | Shim restore obligations, cross-process locks, per-install client identifiers, one-shot UI flags. | | `~/.opencodex/.opencodex-owner.json`, `.opencodex-uninstall.json` | opencodex | Ownership marker and the manifest that bounds what uninstall may remove. Both live in the OpenCodex state root, not in `$CODEX_HOME`. | | `$CODEX_HOME/config.toml` | Codex, edited by opencodex | Active provider and provider table. | diff --git a/structure/02_config-and-codex-home.md b/structure/02_config-and-codex-home.md index 8411884b5c..ddfd1d67e0 100644 --- a/structure/02_config-and-codex-home.md +++ b/structure/02_config-and-codex-home.md @@ -157,6 +157,34 @@ Hidden` inside an already-running PowerShell script, nor to .NET/VBS process-win - 다른 대안 대신 이 방식을 선택한 이유: Names and environment paths are caller-controlled, required secret writes must not silently skip ACLs, and elevation has a larger authority boundary that should remain FFI-only. - 장점, 단점 및 영향: Default Windows ARM64 installations can start and harden secrets; non-default Windows roots continue to fail closed until Bun exposes a trustworthy native system-directory API without FFI. +The durable response-spill directory `~/.opencodex/responses-state-spill/` is bounded in +aggregate, not only per file. Continuation state demoted out of the in-memory cap +(`MAX_STORED_RESPONSE_BYTES`) is written there, and eviction past +`MAX_SPILLED_RESPONSE_BYTES` removes oldest-first through the same deletion point that serves +TTL and count eviction, so an evicted entry unlinks its file. One function owns that ceiling and +three callers drive it: mutation pruning, the lazy load that follows a restart, and the periodic +sweep. The periodic caller is not redundant — the mutation path runs only when traffic arrives, so a +process that comes up over budget from a snapshot written under a larger ceiling would otherwise +stay over it while idle. + +The ceiling bounds what the store can account for, which is every entry in the map plus the +superseded generations queued for unlink, and deliberately not the directory as a whole. Spill files +orphaned by a crash are absent from the map, so this accounting can neither see nor price them; they +remain with the `recoverOrphanedResponseSpills` grace sweep described below, which is the only +mechanism that reclaims them. A host that crashes repeatedly can therefore hold spill bytes above +this ceiling for up to `RESPONSE_SPILL_ORPHAN_GRACE_MS` past each crash. Without that aggregate bound the +directory was limited only per file (256 MiB) and per entry (1000) — a 250 GiB product — which +left `RESPONSE_TTL_MS` as the only effective limit and made disk use a function of client +request rate rather than of anything the process controls. + +[Decision Log] +- 목적과 의도: Bound the durable spill directory in aggregate so demoted continuation state cannot consume the host disk. +- 기존 구현 및 제약 조건: The resident map has an unconditional byte cap and demotes past it, but the disk it demotes onto had only a per-file ceiling and the shared 1000-entry count cap. Retention itself worked — the hour-long TTL did evict — so the gap was a missing budget, not a leak. +- 검토한 주요 대안: Lower the per-file ceiling; shorten the TTL; sweep the directory on a timer; add a configurable budget key; carry a running byte counter. +- 선택한 방식: A constant aggregate ceiling checked at the end of the existing prune, evicting oldest-first, with the total recomputed per prune rather than carried as a counter. +- 다른 대안 대신 이 방식을 선택한 이유: Per-file or TTL changes alter retention semantics other bounds depend on; a timer adds a second owner for eviction; a config key would surface a knob the sibling bounds (count, TTL, per-file) do not have; and a running counter could silently disable the cap if any of the several insertion paths missed an increment, where a walk over at most 1000 entries cannot drift. +- 장점, 단점 및 영향: Disk use stops tracking client request rate. Ordinary traffic is unaffected because the count cap binds at a comparable point for median-sized payloads; a workload of unusually large continuations loses its oldest spills earlier than the TTL would, surfacing as the existing `previous_response_not_found` continuation miss. + Response-state loading performs a bounded recovery pass for interrupted snapshot writes. It only matches regular files named `responses-state.json.ocx...tmp`, waits at least 15 minutes, and skips the current or any live PID. Eligible files are truncated before unlinking so a diff --git a/tests/responses-state.test.ts b/tests/responses-state.test.ts index 3422d8447b..180980bfce 100644 --- a/tests/responses-state.test.ts +++ b/tests/responses-state.test.ts @@ -38,6 +38,7 @@ import { recoverStaleResponseStateTemps, rememberResponseState, sweepAbandonedResponseStateTemps, + sweepExpiredResponseStates, responseAdmissionCountersForTests, responseStateMetrics, responseStatePersistPendingForTests, @@ -45,6 +46,8 @@ import { runPendingResponseStatePersistForTests, setResponseSpillAsyncAclAttemptBudgetForTests, setResponseStateByteCapForTests, + setSpilledResponseByteCapForTests, + getSpilledResponseBytesForTests, setResponseStatePersistAttemptHookForTests, setResponseSpillShutdownBudgetForTests, getStoredResponseBytesForTests, @@ -282,6 +285,7 @@ describe("Responses previous_response_id state", () => { setResponseSpillShutdownBudgetForTests(null); setResponseSpillAsyncAclAttemptBudgetForTests(null); setResponseStateByteCapForTests(null); + setSpilledResponseByteCapForTests(null); clearResponseStateForTests(); rmSync(home, { recursive: true, force: true }); if (priorHome === undefined) delete process.env["OPENCODEX_HOME"]; @@ -812,6 +816,23 @@ describe("Responses previous_response_id state", () => { }) as { input: unknown[] }).input).toHaveLength(3); }); + test("evicts oldest spills once the durable set exceeds the disk cap", () => { + setResponseStateByteCapForTests(1_024); + setSpilledResponseByteCapForTests(20_000); + for (let i = 0; i < 6; i += 1) rememberLarge(`resp_spill_budget_${i}`, "x".repeat(8_000)); + // Without a disk cap all six stay on disk: the RAM cap only moves bytes out of + // memory, it never bounds where they land. + expect(getSpilledResponseBytesForTests()).toBeLessThanOrEqual(20_000); + expect(spillFileNames(home).length).toBeLessThan(6); + // The oldest entry is the one that must be gone, not merely "some" entry. + expect(spillFileNames(home).some(name => name.startsWith("resp_spill_budget_0."))).toBe(false); + // Eviction is oldest-first and must not clear the set it was asked to bound. + expect(spillFileNames(home).length).toBeGreaterThanOrEqual(1); + expect((expandPreviousResponseInput({ + previous_response_id: "resp_spill_budget_5", input: "next", + }) as { input: unknown[] }).input).toHaveLength(3); + }); + test("does not swap a resident row to a stub before fsync and no-replace publication succeed", () => { const events: string[] = []; setSpillIoForTest({ record: event => events.push(event) }); @@ -1535,6 +1556,147 @@ describe("Responses previous_response_id state", () => { expect(spillFileNames(home)).toHaveLength(1); }); + test("evicts by createdAt, not by insertion order", () => { + const realNow = Date.now; + try { + setResponseStateByteCapForTests(1_024); + // Insert the NEWER entry first so insertion order and createdAt order + // disagree. Map order alone would evict the newer one; `states` is not a + // reliable age index — writeBoundedSnapshot even serializes it reversed. + const base = realNow(); + Date.now = () => base; + const beforeNewer = new Set(spillFileNames(home)); + rememberLarge("resp_order_newer", "x".repeat(8_000)); + const newerFile = spillFileNames(home).find(name => !beforeNewer.has(name))!; + + Date.now = () => base - 5 * 60_000; + const beforeOlder = new Set(spillFileNames(home)); + rememberLarge("resp_order_older", "x".repeat(8_000)); + const olderFile = spillFileNames(home).find(name => !beforeOlder.has(name))!; + Date.now = () => base; + + // Each spill payload is ~16.2 KB, so 20_000 leaves room for exactly one. + setSpilledResponseByteCapForTests(20_000); + rememberLarge("resp_order_trigger", "y"); + + const dir = responseSpillDirectory(home); + expect(existsSync(join(dir, olderFile))).toBe(false); + expect(existsSync(join(dir, newerFile))).toBe(true); + } finally { + Date.now = realNow; + } + }); + + test("breaks createdAt ties on the response id, not on insertion order", () => { + const realNow = Date.now; + try { + setResponseStateByteCapForTests(1_024); + const base = realNow(); + Date.now = () => base; + // Same createdAt, inserted in reverse id order: a stable sort alone would + // keep insertion order and evict "_b" first. + const before = new Set(spillFileNames(home)); + rememberLarge("resp_tie_b", "x".repeat(8_000)); + const bFile = spillFileNames(home).find(name => !before.has(name))!; + const beforeA = new Set(spillFileNames(home)); + rememberLarge("resp_tie_a", "x".repeat(8_000)); + const aFile = spillFileNames(home).find(name => !beforeA.has(name))!; + + setSpilledResponseByteCapForTests(20_000); + rememberLarge("resp_tie_trigger", "y"); + + const dir = responseSpillDirectory(home); + expect(existsSync(join(dir, aFile))).toBe(false); + expect(existsSync(join(dir, bFile))).toBe(true); + } finally { + Date.now = realNow; + } + }); + + test("survives a single spill payload larger than the whole disk budget", () => { + setResponseStateByteCapForTests(1_024); + // Below one payload (~16.2 KB), so every spill is written and then evicted on + // the same tick and the running total goes negative. The loop must still + // terminate and leave the store usable. + setSpilledResponseByteCapForTests(1_000); + rememberLarge("resp_over_budget_a", "x".repeat(8_000)); + rememberLarge("resp_over_budget_b", "x".repeat(8_000)); + expect(spillFileNames(home)).toHaveLength(0); + expect(getSpilledResponseBytesForTests()).toBe(0); + // Raising the cap restores ordinary retention: the store is not wedged. + setSpilledResponseByteCapForTests(1_000_000); + rememberLarge("resp_over_budget_c", "x".repeat(8_000)); + expect(spillFileNames(home)).toHaveLength(1); + }); + + test("counts deferred spill generations against the disk cap and drains them first", () => { + setResponseStateByteCapForTests(1_024); + // Replacing a spilled id queues the superseded file in pendingSpillUnlinks + // instead of unlinking it, so the directory holds generations that `states` + // alone cannot see. Payloads are ~16.2 KB, so 60_000 fits about three. + setSpilledResponseByteCapForTests(60_000); + for (let i = 0; i < 8; i += 1) rememberLarge("resp_deferred_gen", "x".repeat(8_000)); + // Counting only the live entry would report ~16 KB here and evict nothing while + // eight files sat on disk. + expect(getSpilledResponseBytesForTests()).toBeLessThanOrEqual(60_000); + const onDisk = spillFileNames(home); + expect(onDisk.length).toBeLessThanOrEqual(4); + // The live continuation survives: deferred generations are released first. + expect((expandPreviousResponseInput({ + previous_response_id: "resp_deferred_gen", input: "next", + }) as { input: unknown[] }).input).toHaveLength(3); + }); + + test("reclaims an over-budget snapshot without a new continuation mutation", async () => { + const realNow = Date.now; + try { + setResponseStateByteCapForTests(4_000); + const base = realNow(); + const files: string[] = []; + for (let i = 0; i < 4; i += 1) { + Date.now = () => base - (10 - i) * 60_000; + const before = new Set(spillFileNames(home)); + rememberResponseState( + { model: "test/model", input: "z".repeat(8_000), store: false }, + fixedResponse(`resp_budget_restart_${i}`, [{ type: "message", role: "assistant", content: "stored" }]), + undefined, + { force: true, clientThreadId: "task-budget" }, + ); + files.push(spillFileNames(home).find(name => !before.has(name))!); + } + Date.now = () => base; + await flushResponseState(); + clearResponseStateMemoryForTests(); + + // Come back up under a ceiling the snapshot was not written for. Nothing has + // mutated the store yet, which is the case the mutation-path prune misses. + setResponseStateByteCapForTests(4_000); + // Four spills total ~32.9 KB, so this ceiling keeps the two newest. + setSpilledResponseByteCapForTests(20_000); + expect(spillFileNames(home).length).toBe(4); + + // A read, not a write: this drives the lazy load and its prune. + const expanded = expandPreviousResponseInput( + { previous_response_id: "resp_budget_restart_3", input: "next" }, + "task-budget", + ); + + expect(getSpilledResponseBytesForTests()).toBeLessThanOrEqual(20_000); + expect(existsSync(join(responseSpillDirectory(home), files[0]!))).toBe(false); + // The newest entry survives the reclaim and still replays. + expect(existsSync(join(responseSpillDirectory(home), files[3]!))).toBe(true); + expect((expanded as { input: unknown[] }).input).toHaveLength(3); + + // The periodic sweep owns the same ceiling: lower it again and let the tick, + // not a request, do the work. + setSpilledResponseByteCapForTests(10_000); + sweepExpiredResponseStates(); + expect(getSpilledResponseBytesForTests()).toBeLessThanOrEqual(10_000); + } finally { + Date.now = realNow; + } + }); + test("TTL and count eviction delete dedicated spill files and release stub bytes", () => { const realNow = Date.now; setResponseStateByteCapForTests(1_024); From d5c7b5601af9397c48540761dbfff42f6a60a0af Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Tue, 1 Sep 2026 01:28:42 +0900 Subject: [PATCH 2/6] fix(responses): reserve the peak publication footprint against the disk cap The aggregate cap counted installed spills and deferred unlinks - files that already exist. It could not see one that writeResponseSpillDurablyAsync was in the middle of creating, and on Windows that middle lasts as long as icacls takes. A cap that holds only when writes are fast is not a cap; the incident behind this work put 6.8 GiB on disk in 44 minutes. A queued publication now reserves its peak on-disk footprint, and the cap is enforced against files-plus-reservations before the temp or destination file is created rather than by deleting the overflow afterwards. The reserved figure is two envelopes, not one. Publication can fall back from hard-linking to an exclusive copy, and during that fallback the destination copy and the temp file exist together, so reserving a single payload would leave the overshoot intact at half its magnitude. Ownership is single and settles on every exit. A queued job holds its reservation until releasePendingResponseSpill, which the finally in runPendingResponseSpill reaches from every return, throw and mismatch, and which cancellation reaches for a job that never ran. The shutdown fallback re-reserves for the duration of its synchronous write, because supersession releases the original reservation immediately before the heaviest publication of the drain - and that write has the same link-then-copy fallback. A leaked reservation would be monotonic, ratcheting the usable cap toward zero until nothing could spill at all. Regression drives the accounting red: with an in-flight publication gated on icacls, the walk over states reports 0 bytes while the reservation reports the two-envelope peak, and after settlement the accounting collapses to the real file. Carries lifrary's b4d1d2404 unmodified as the base. --- src/responses/state.ts | 78 ++++++++++++++++++++++++++++++++++- tests/responses-state.test.ts | 47 +++++++++++++++++++++ 2 files changed, 124 insertions(+), 1 deletion(-) diff --git a/src/responses/state.ts b/src/responses/state.ts index 902a429ee3..df586126d9 100644 --- a/src/responses/state.ts +++ b/src/responses/state.ts @@ -211,12 +211,41 @@ interface PendingResponseSpill { cancelled: boolean; released: boolean; sizeBytes: number; + /** Peak on-disk bytes reserved for this publication; released exactly once on settle. */ + reservedBytes: number; publicationControl: ResponseSpillPublicationControl; } const pendingResponseSpills = new Set(); const pendingResponseSpillById = new Map(); let pendingResponseSpillBytes = 0; +/** + * On-disk bytes a queued publication is about to occupy but has not yet installed into + * `states`. + * + * `spilledResponseBytes()` walks installed spills and deferred unlinks — files that + * already exist. It cannot see one that `writeResponseSpillDurablyAsync` is in the + * middle of creating, and on Windows that middle can last as long as `icacls` takes. + * Without a reservation the cap holds only when writes are fast, which is not a cap. + * + * The reserved figure is the PEAK footprint, not the payload: publication can fall back + * from hard-linking to an exclusive copy, and during that fallback the destination copy + * and the temp file exist simultaneously. Reserving one envelope would leave the overshoot + * intact at half its magnitude. + * + * Ownership is single: a job holds its reservation from queue until + * `releasePendingResponseSpill`, which every exit from the publication path reaches + * through the `finally` in `runPendingResponseSpill` and through cancellation of a + * not-yet-running job. A leaked reservation is monotonic — it would ratchet the usable + * cap toward zero — so the release must stay on the settlement path rather than in a + * parallel bookkeeping pass. + */ +let reservedResponseSpillBytes = 0; + +/** Peak on-disk footprint of publishing `payloadBytes`: temp plus destination copy. */ +function publicationFootprintBytes(payloadBytes: number): number { + return payloadBytes * 2; +} let responseSpillPublicationTail: Promise = Promise.resolve(); let responseSpillShutdownBudgetOverride: { totalMs: number; fallbackReserveMs: number } | null = null; let responseSpillShutdownTerminalizationPassLimitOverride: number | null = null; @@ -234,6 +263,7 @@ function releasePendingResponseSpill(job: PendingResponseSpill): void { if (job.released) return; job.released = true; pendingResponseSpillBytes = Math.max(0, pendingResponseSpillBytes - job.sizeBytes); + reservedResponseSpillBytes = Math.max(0, reservedResponseSpillBytes - job.reservedBytes); pendingResponseSpills.delete(job); if (pendingResponseSpillById.get(job.id) === job) pendingResponseSpillById.delete(job.id); job.candidate = null; @@ -337,6 +367,21 @@ function queuePendingResponseSpill( deferSupersededSpill(inheritedSpill); return; } + // Enforce the disk cap BEFORE the temp or destination file is created. Deleting the + // overflow afterwards is not equivalent: on Windows the file can outlive the decision + // by as long as ACL hardening takes, which is the window the measured 6.8 GiB + // accumulated in. Reclaim first, and only refuse if the peak footprint still does not + // fit — an eviction pass can free a live continuation's worth of room. + const footprint = publicationFootprintBytes(candidate.sizeBytes); + if (accountedResponseSpillBytes() + footprint > spillByteCap()) { + enforceSpilledResponseBudget(); + if (accountedResponseSpillBytes() + footprint > spillByteCap()) { + spillCounters.writeFailures += 1; + replaceWithSpillFailure(id, candidate); + deferSupersededSpill(inheritedSpill); + return; + } + } const job: PendingResponseSpill = { id, candidate, @@ -346,11 +391,13 @@ function queuePendingResponseSpill( cancelled: false, released: false, sizeBytes: candidate.sizeBytes, + reservedBytes: publicationFootprintBytes(candidate.sizeBytes), publicationControl: createResponseSpillPublicationControl(), }; pendingResponseSpills.add(job); pendingResponseSpillById.set(id, job); pendingResponseSpillBytes += job.sizeBytes; + reservedResponseSpillBytes += job.reservedBytes; recomputeOldestResident(); responseSpillPublicationTail = responseSpillPublicationTail .then(() => runPendingResponseSpill(job), () => runPendingResponseSpill(job)); @@ -442,6 +489,13 @@ function installShutdownFallbackSpill( aclBudgetMs: number, ): void { let ref: ResponseSpillRef | null = null; + // Supersession released this job's reservation, but the synchronous write below is the + // largest publication of the shutdown path and has its own link-then-copy fallback + // holding a temp and a destination at once. Re-reserve for its duration so the cap is + // not blind exactly where the drain does its heaviest work, and settle in `finally` so + // every return, throw and mismatch releases it. + const footprint = publicationFootprintBytes(candidate.sizeBytes); + reservedResponseSpillBytes += footprint; try { ref = writeResponseSpillDurably(job.id, spillPayloadForResident(candidate), { aclBudgetMs }); if (ref.payloadBytes > responseSpillPayloadCap()) { @@ -469,6 +523,8 @@ function installShutdownFallbackSpill( deferSupersededSpill(job.supersededSpill); } throw error; + } finally { + reservedResponseSpillBytes = Math.max(0, reservedResponseSpillBytes - footprint); } } @@ -652,6 +708,19 @@ function spilledResponseBytes(): number { return total; } +/** + * Accounted on-disk bytes: files that exist, plus the peak footprint of publications + * already in flight. + * + * The cap is enforced against this rather than against `spilledResponseBytes()` alone, + * because a publication that has not finished is still consuming the volume. On Windows + * the gap between "queued" and "installed" is however long `icacls` takes, and the + * measured incident this cap answers accumulated 6.8 GiB in 44 minutes. + */ +function accountedResponseSpillBytes(): number { + return spilledResponseBytes() + reservedResponseSpillBytes; +} + /** Test-only: lower/restore the durable spill cap (null restores the default). */ export function setSpilledResponseByteCapForTests(bytes: number | null): void { spillByteCapOverride = bytes; @@ -662,6 +731,11 @@ export function getSpilledResponseBytesForTests(): number { return spilledResponseBytes(); } +/** Test-only: on-disk bytes plus in-flight publication reservations. */ +export function getAccountedResponseSpillBytesForTests(): number { + return accountedResponseSpillBytes(); +} + function serializedBytes(value: unknown): number | null { try { const serialized = JSON.stringify(value); @@ -1572,7 +1646,9 @@ export function replayOverlapSkipsForTests(): number { * account for, and not the directory as a whole. */ function enforceSpilledResponseBudget(): number { - let spilledBytes = spilledResponseBytes(); + // Price in-flight publications too: a file being created by + // `writeResponseSpillDurablyAsync` occupies the volume before it reaches `states`. + let spilledBytes = accountedResponseSpillBytes(); if (spilledBytes <= spillByteCap()) return 0; const before = spilledBytes; // Deferred generations go first. They are already superseded, so releasing one diff --git a/tests/responses-state.test.ts b/tests/responses-state.test.ts index 180980bfce..9a2fffd88a 100644 --- a/tests/responses-state.test.ts +++ b/tests/responses-state.test.ts @@ -48,6 +48,7 @@ import { setResponseStateByteCapForTests, setSpilledResponseByteCapForTests, getSpilledResponseBytesForTests, + getAccountedResponseSpillBytesForTests, setResponseStatePersistAttemptHookForTests, setResponseSpillShutdownBudgetForTests, getStoredResponseBytesForTests, @@ -878,6 +879,52 @@ describe("Responses previous_response_id state", () => { expect(responseStateMetrics()).toMatchObject({ residentCount: 0, spillStubCount: 1, spillWrites: 1, spillWriteFailures: 0 }); }); + test("counts an in-flight Windows publication against the disk cap", async () => { + // The cap is a promise about the volume, and a file being created by + // writeResponseSpillDurablyAsync is on the volume. On Windows the gap between + // "queued" and "installed in states" is however long icacls takes, so accounting + // that walks only installed spills reports a satisfied budget while the directory + // grows. The measured incident behind this cap put 6.8 GiB on disk in 44 minutes. + setPlatformForTests("win32"); + let release!: () => void; + let entered!: () => void; + const gate = new Promise(resolve => { release = resolve; }); + const started = new Promise(resolve => { entered = resolve; }); + let announced = false; + setAsyncIcaclsRunnerForTests(async () => { + if (!announced) { + announced = true; + entered(); + } + await gate; + return { success: true, exitCode: 0, timedOut: false, stdout: "" }; + }); + setResponseStateByteCapForTests(1_024); + + rememberLarge("resp_reserved_inflight", "x".repeat(8_000)); + await started; + try { + // Nothing is installed yet, so the walk over `states` sees nothing on disk. + expect(getSpilledResponseBytesForTests()).toBe(0); + // The reservation prices the publication anyway, at its PEAK footprint: publication + // can fall back from hard-linking to an exclusive copy, and during that fallback the + // temp and the destination exist together. Reserving one envelope would leave the + // overshoot intact at half its size. + const accounted = getAccountedResponseSpillBytesForTests(); + const pending = pendingResponseSpillMetricsForTests(); + expect(pending.count).toBe(1); + expect(accounted).toBe(pending.bytes * 2); + } finally { + release(); + } + await flushPendingResponseSpillsForTests(); + // Settled: the reservation is released exactly once and the accounting collapses to + // the real file. A leaked reservation would be monotonic - it would ratchet the usable + // cap toward zero until nothing could spill at all. + expect(getAccountedResponseSpillBytesForTests()).toBe(getSpilledResponseBytesForTests()); + expect(getSpilledResponseBytesForTests()).toBeGreaterThan(0); + }); + test("Windows spill retries one transient ACL timeout without installing a tombstone", async () => { setPlatformForTests("win32"); process.env.OPENCODEX_ACL_TIMEOUT_MS = "1000"; From 3408626d1d735496d7c67eb7f690b24d94be0b91 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Tue, 1 Sep 2026 01:46:40 +0900 Subject: [PATCH 3/6] fix(responses): price the spill cap from real occupancy, not from a proxy Review of the reservation commit found four ways the accounting still undercounted what is on the volume. The reservation was derived from candidate.sizeBytes, which measures the resident shape and omits the version field the published envelope carries. Admission is now priced from prospectiveResponseSpillBytes, which shares the production serializer, so the figure cannot drift from what is written. A same-id replacement removes the old spill from states and hands its ref to the pending job. Neither states nor pendingSpillUnlinks could see it, so a copy fallback held old generation plus temp plus destination - three envelopes priced as two. Job-owned superseded generations are now counted. Shutdown supersession released the reservation even when cleanup reported it could not remove the async temp or destination. Those bytes are not a reservation, because nothing will release them: the file could not be deleted. They move to a separate unreclaimable total that is never decremented, which is the only honest way to price a file nobody can remove. Startup orphan recovery is what reclaims them across a restart. The regression is rewritten to prove the cap rather than the counter wiring. It seeds real prior occupancy, forces link failure into the COPYFILE_EXCL fallback, gates destination hardening, and asserts against files actually on disk - three of them - while the walk over installed spills still reports one. After settlement it asserts the accounting collapses to the real files, no temp survives, and the newest continuation still replays. --- src/responses/spill-store.ts | 20 +++++++++ src/responses/state.ts | 57 ++++++++++++++++++++++---- tests/responses-state.test.ts | 76 +++++++++++++++++++++++------------ 3 files changed, 120 insertions(+), 33 deletions(-) diff --git a/src/responses/spill-store.ts b/src/responses/spill-store.ts index f61d475c5a..ce063dfd8b 100644 --- a/src/responses/spill-store.ts +++ b/src/responses/spill-store.ts @@ -470,6 +470,26 @@ function serializedSpill( }; } +/** + * Exact on-disk payload size this spill WOULD occupy, measured before publication. + * + * Callers that reserve disk against a cap need the real envelope, not the resident + * measurement: the resident figure omits the `version` field the published payload + * carries, so pricing an admission by it undercounts and lets a request that sits exactly + * at the cap still exceed it. Shares `serializedSpill` rather than describing it, so the + * two cannot drift. + */ +export function prospectiveResponseSpillBytes( + responseId: string, + state: Omit, +): number | null { + try { + return serializedSpill(responseId, state).bytes.byteLength; + } catch { + return null; + } +} + function responseSpillWriteError(cause: unknown): NodeJS.ErrnoException { const error = new Error("Response spill write failed", { cause }) as NodeJS.ErrnoException; if (cause && typeof cause === "object" && "code" in cause) { diff --git a/src/responses/state.ts b/src/responses/state.ts index df586126d9..e0fcb598d2 100644 --- a/src/responses/state.ts +++ b/src/responses/state.ts @@ -16,6 +16,7 @@ import { responseSpillDirectory, responseSpillPayloadCap, markResponseSpillPublicationSuperseded, + prospectiveResponseSpillBytes, type ResponseSpillPublicationControl, type ResponseSpillRef, writeResponseSpillDurably, @@ -241,10 +242,28 @@ let pendingResponseSpillBytes = 0; * parallel bookkeeping pass. */ let reservedResponseSpillBytes = 0; +/** + * Bytes of spill publications that could not be cleaned up and will not be released. + * + * A failed cleanup leaves a real file behind. Counting it as a reservation would be + * wrong — reservations settle — so it is tracked separately and never decremented: the + * only honest way to price a file nobody can delete. Startup orphan recovery is what + * reclaims these across a restart. + */ +let unreclaimableResponseSpillBytes = 0; -/** Peak on-disk footprint of publishing `payloadBytes`: temp plus destination copy. */ -function publicationFootprintBytes(payloadBytes: number): number { - return payloadBytes * 2; +/** + * Peak on-disk footprint of publishing this candidate: temp plus destination copy. + * + * Measured from the production serializer rather than from `candidate.sizeBytes`. The + * resident measurement omits the `version` field the published envelope carries, so + * pricing an admission by it undercounts and lets a request sitting exactly at the cap + * still exceed it. Falls back to the resident figure only when serialization fails, which + * is the same condition that will fail the publication itself. + */ +function publicationFootprintBytes(id: string, candidate: ResidentResponseState): number { + const exact = prospectiveResponseSpillBytes(id, spillPayloadForResident(candidate)); + return (exact ?? candidate.sizeBytes) * 2; } let responseSpillPublicationTail: Promise = Promise.resolve(); let responseSpillShutdownBudgetOverride: { totalMs: number; fallbackReserveMs: number } | null = null; @@ -372,7 +391,7 @@ function queuePendingResponseSpill( // by as long as ACL hardening takes, which is the window the measured 6.8 GiB // accumulated in. Reclaim first, and only refuse if the peak footprint still does not // fit — an eviction pass can free a live continuation's worth of room. - const footprint = publicationFootprintBytes(candidate.sizeBytes); + const footprint = publicationFootprintBytes(id, candidate); if (accountedResponseSpillBytes() + footprint > spillByteCap()) { enforceSpilledResponseBudget(); if (accountedResponseSpillBytes() + footprint > spillByteCap()) { @@ -391,7 +410,7 @@ function queuePendingResponseSpill( cancelled: false, released: false, sizeBytes: candidate.sizeBytes, - reservedBytes: publicationFootprintBytes(candidate.sizeBytes), + reservedBytes: footprint, publicationControl: createResponseSpillPublicationControl(), }; pendingResponseSpills.add(job); @@ -494,7 +513,7 @@ function installShutdownFallbackSpill( // holding a temp and a destination at once. Re-reserve for its duration so the cap is // not blind exactly where the drain does its heaviest work, and settle in `finally` so // every return, throw and mismatch releases it. - const footprint = publicationFootprintBytes(candidate.sizeBytes); + const footprint = publicationFootprintBytes(job.id, candidate); reservedResponseSpillBytes += footprint; try { ref = writeResponseSpillDurably(job.id, spillPayloadForResident(candidate), { aclBudgetMs }); @@ -557,7 +576,17 @@ function supersedeShutdownFallbackBatch( } for (const { job } of pending) { const cleanupFailure = cleanupSupersededResponseSpillPublication(job.publicationControl); - if (cleanupFailure) failures.push(cleanupFailure); + if (cleanupFailure) { + failures.push(cleanupFailure); + // Cleanup failed, so the async temp or destination is STILL on the volume. Releasing + // the reservation would un-account a file that exists, and the fallback write that + // follows reserves only its own footprint — three envelopes on disk priced as two. + // + // These bytes are not a reservation: nothing will release them, because the file + // could not be removed. They are unreclaimable occupancy, and the cap has to keep + // seeing them or it stops describing the volume. + unreclaimableResponseSpillBytes += job.reservedBytes; + } releasePendingResponseSpill(job); } } @@ -718,7 +747,17 @@ function spilledResponseBytes(): number { * measured incident this cap answers accumulated 6.8 GiB in 44 minutes. */ function accountedResponseSpillBytes(): number { - return spilledResponseBytes() + reservedResponseSpillBytes; + // Superseded generations a pending job still owns are files on disk too. A same-id + // replacement removes the old spill from `states` and hands its ref to the job, so + // counting only `states` plus `pendingSpillUnlinks` loses it for the whole publication + // — during a copy fallback that is old generation + new temp + new destination, three + // envelopes priced as two. + let ownedBySpillJobs = 0; + for (const job of pendingResponseSpills) { + if (job.supersededSpill) ownedBySpillJobs += job.supersededSpill.payloadBytes; + } + return spilledResponseBytes() + reservedResponseSpillBytes + ownedBySpillJobs + + unreclaimableResponseSpillBytes; } /** Test-only: lower/restore the durable spill cap (null restores the default). */ @@ -2186,6 +2225,8 @@ export function clearResponseStateMemoryForTests(): void { export function clearResponseStateForTests(): void { for (const entry of states.values()) deleteOwnedSpills(entry); clearResponseStateMemoryForTests(); + reservedResponseSpillBytes = 0; + unreclaimableResponseSpillBytes = 0; try { unlinkSync(snapshotPath()); } catch { diff --git a/tests/responses-state.test.ts b/tests/responses-state.test.ts index 9a2fffd88a..b84d93f658 100644 --- a/tests/responses-state.test.ts +++ b/tests/responses-state.test.ts @@ -879,50 +879,76 @@ describe("Responses previous_response_id state", () => { expect(responseStateMetrics()).toMatchObject({ residentCount: 0, spillStubCount: 1, spillWrites: 1, spillWriteFailures: 0 }); }); - test("counts an in-flight Windows publication against the disk cap", async () => { + test("holds the disk cap while a copy-fallback publication has temp and destination on disk", async () => { // The cap is a promise about the volume, and a file being created by - // writeResponseSpillDurablyAsync is on the volume. On Windows the gap between - // "queued" and "installed in states" is however long icacls takes, so accounting - // that walks only installed spills reports a satisfied budget while the directory - // grows. The measured incident behind this cap put 6.8 GiB on disk in 44 minutes. + // writeResponseSpillDurablyAsync is on the volume. Accounting that walks only + // installed spills reports a satisfied budget while the directory grows — the + // incident behind this cap put 6.8 GiB on disk in 44 minutes. + // + // The peak is TWO envelopes, not one: when hard-linking fails, publication copies + // with COPYFILE_EXCL and then hardens the destination, so the temp and the copy exist + // together. This drives that exact path and measures real bytes on disk. setPlatformForTests("win32"); + setResponseStateByteCapForTests(1_024); + + let gateDestinationHarden = false; let release!: () => void; let entered!: () => void; const gate = new Promise(resolve => { release = resolve; }); const started = new Promise(resolve => { entered = resolve; }); let announced = false; - setAsyncIcaclsRunnerForTests(async () => { - if (!announced) { - announced = true; - entered(); + setAsyncIcaclsRunnerForTests(async args => { + if (gateDestinationHarden && args.some(arg => arg.endsWith(".spill.json"))) { + if (!announced) { + announced = true; + entered(); + } + await gate; } - await gate; return { success: true, exitCode: 0, timedOut: false, stdout: "" }; }); - setResponseStateByteCapForTests(1_024); - rememberLarge("resp_reserved_inflight", "x".repeat(8_000)); + // One resident spill already on disk, so the cap has real prior occupancy. + rememberLarge("resp_cap_existing", "e".repeat(8_000)); + await flushPendingResponseSpillsForTests(); + const existingBytes = getSpilledResponseBytesForTests(); + expect(existingBytes).toBeGreaterThan(0); + expect(spillFileNames(home)).toHaveLength(1); + + // Force the exclusive-copy fallback, then gate the destination hardening that follows + // it, so the observation below happens with BOTH files present. + setSpillIoForTest({ + link: () => { throw Object.assign(new Error("EXDEV"), { code: "EXDEV" }); }, + }); + gateDestinationHarden = true; + + rememberLarge("resp_cap_inflight", "x".repeat(8_000)); await started; try { - // Nothing is installed yet, so the walk over `states` sees nothing on disk. - expect(getSpilledResponseBytesForTests()).toBe(0); - // The reservation prices the publication anyway, at its PEAK footprint: publication - // can fall back from hard-linking to an exclusive copy, and during that fallback the - // temp and the destination exist together. Reserving one envelope would leave the - // overshoot intact at half its size. - const accounted = getAccountedResponseSpillBytesForTests(); - const pending = pendingResponseSpillMetricsForTests(); - expect(pending.count).toBe(1); - expect(accounted).toBe(pending.bytes * 2); + // Real disk: the temp and the copied destination coexist during hardening. + const onDisk = spillFileNames(home).length + spillTempNames(home).length; + expect(onDisk).toBeGreaterThanOrEqual(3); + // The walk over installed spills still reports only the settled file, so accounting + // built on it alone would price a three-envelope directory as one. + expect(getSpilledResponseBytesForTests()).toBe(existingBytes); + // Reservation prices the in-flight publication at its peak, so the accounted total + // covers what is actually on the volume. + expect(getAccountedResponseSpillBytesForTests()) + .toBeGreaterThanOrEqual(existingBytes * 3); } finally { release(); + setSpillIoForTest(null); } await flushPendingResponseSpillsForTests(); - // Settled: the reservation is released exactly once and the accounting collapses to - // the real file. A leaked reservation would be monotonic - it would ratchet the usable + // Settled: the reservation is released exactly once and accounting collapses to the + // real files. A leaked reservation would be monotonic — it would ratchet the usable // cap toward zero until nothing could spill at all. expect(getAccountedResponseSpillBytesForTests()).toBe(getSpilledResponseBytesForTests()); - expect(getSpilledResponseBytesForTests()).toBeGreaterThan(0); + expect(spillTempNames(home)).toHaveLength(0); + // And the newest continuation is still replayable: the cap must not have turned the + // fail-closed path into the ordinary one. + expect(JSON.stringify(expandPreviousResponseInput({ previous_response_id: "resp_cap_inflight", input: "next" }))) + .toContain("xxxxxxxx"); }); test("Windows spill retries one transient ACL timeout without installing a tombstone", async () => { From 6545a4b23e275a60733cb51d5a691d6f83059267 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Tue, 1 Sep 2026 01:59:38 +0900 Subject: [PATCH 4/6] fix(responses): make the spill cap enforce, and let cleanup debt be repaid Second review round on the reservation work found three ways the accounting still did not match the volume. A same-id replacement takes the old spill off states and hands it to the new job, but admission ran before the job existed, so the decision was short by a whole envelope. The inherited generation is now priced in the check itself. Cancellation also left the ref on the cancelled job while returning it to the caller, so the accounting walk could count one physical file twice and evict live continuations to reclaim bytes that were not there; ownership now transfers rather than being copied. Cleanup-failure debt was a flat two envelopes that never decremented. Both halves were wrong. clearOwnedPath nulls whichever path it managed to remove, so one failure is often one file; and a Windows lock that clears a moment later, or the async writer's own retry, can remove the file while the charge stayed forever. With 256 MiB payloads two such charges consume the whole default cap and nothing can spill again for the life of the process. The debt is now per path, priced at what that path holds, and settled as soon as the path is gone. The shutdown fallback checked nothing before writing. It now reclaims and, if the footprint still does not fit, terminalizes with ENOSPC rather than publishing onto a volume that is already over budget - the same fail-closed ending the budget-exhaustion path uses. The regression is split in two, because the previous single test proved the counter and not the cap: it stayed green with the admission branch deleted. One test now proves accounting during a forced COPYFILE_EXCL fallback with temp and destination both on disk; the other proves enforcement, and it is red when admission is removed. --- src/responses/state.ts | 78 ++++++++++++++++++++++++++++------- tests/responses-state.test.ts | 59 ++++++++++++++++++++++++++ 2 files changed, 122 insertions(+), 15 deletions(-) diff --git a/src/responses/state.ts b/src/responses/state.ts index e0fcb598d2..e955abe4f2 100644 --- a/src/responses/state.ts +++ b/src/responses/state.ts @@ -243,14 +243,34 @@ let pendingResponseSpillBytes = 0; */ let reservedResponseSpillBytes = 0; /** - * Bytes of spill publications that could not be cleaned up and will not be released. + * Paths a failed cleanup left on the volume, with the bytes each one occupies. * - * A failed cleanup leaves a real file behind. Counting it as a reservation would be - * wrong — reservations settle — so it is tracked separately and never decremented: the - * only honest way to price a file nobody can delete. Startup orphan recovery is what - * reclaims these across a restart. + * A failed unlink leaves a real file behind, so the cap has to keep seeing it. But a + * never-decremented total would be phantom debt: a Windows lock that clears a moment + * later, or the async writer's own retry, can remove the file while the charge stays + * forever — and with 256 MiB payloads two conservative charges consume the whole default + * cap, after which nothing can spill for the life of the process. + * + * So the debt is per PATH, priced at what that path actually holds, and settled the + * moment the path is gone. `reconcileUnreclaimableSpillPaths` re-checks on every read of + * the accounted total, which is the same tick that would otherwise refuse an admission. */ -let unreclaimableResponseSpillBytes = 0; +const unreclaimableSpillPaths = new Map(); + +function chargeUnreclaimableSpillPath(path: string | null | undefined, bytes: number): void { + if (!path || bytes <= 0) return; + unreclaimableSpillPaths.set(path, bytes); +} + +/** Drop charges for paths that have since disappeared; returns the surviving total. */ +function reconcileUnreclaimableSpillPaths(): number { + let total = 0; + for (const [path, bytes] of [...unreclaimableSpillPaths]) { + if (existsSync(path)) total += bytes; + else unreclaimableSpillPaths.delete(path); + } + return total; +} /** * Peak on-disk footprint of publishing this candidate: temp plus destination copy. @@ -295,6 +315,11 @@ function cancelPendingResponseSpill(id: string): ResponseSpillRef | undefined { job.cancelled = true; markResponseSpillPublicationSuperseded(job.publicationControl); const superseded = job.supersededSpill; + // Ownership TRANSFERS to the caller. Leaving the ref on the cancelled job would let the + // accounting walk count the same physical file twice — once here and once on the + // replacement — and an overcount evicts live continuations to make room for bytes that + // are not there. + delete job.supersededSpill; // A queued job has not captured the candidate in an async frame yet, so release it now. // A running job retains its accounting until settlement and will discard its stale file. if (!job.running) releasePendingResponseSpill(job); @@ -392,9 +417,13 @@ function queuePendingResponseSpill( // accumulated in. Reclaim first, and only refuse if the peak footprint still does not // fit — an eviction pass can free a live continuation's worth of room. const footprint = publicationFootprintBytes(id, candidate); - if (accountedResponseSpillBytes() + footprint > spillByteCap()) { + // The superseded generation this job is about to own is already off `states` and not + // yet on the job, so it is invisible to the walk. Price it here or admission decides + // against a total that is short by a whole envelope. + const inheritedBytes = inheritedSpill?.payloadBytes ?? 0; + if (accountedResponseSpillBytes() + footprint + inheritedBytes > spillByteCap()) { enforceSpilledResponseBudget(); - if (accountedResponseSpillBytes() + footprint > spillByteCap()) { + if (accountedResponseSpillBytes() + footprint + inheritedBytes > spillByteCap()) { spillCounters.writeFailures += 1; replaceWithSpillFailure(id, candidate); deferSupersededSpill(inheritedSpill); @@ -516,6 +545,22 @@ function installShutdownFallbackSpill( const footprint = publicationFootprintBytes(job.id, candidate); reservedResponseSpillBytes += footprint; try { + // The drain must not publish over the cap either. Reclaim first; if the footprint + // still does not fit — which is what unreclaimable cleanup debt looks like — the + // honest close-out is a tombstone, not another file on a volume that is already + // over budget. `replaceWithSpillFailure` is the same fail-closed ending the budget + // exhaustion path uses, so replay reports `spill_failed` and the client resends. + if (accountedResponseSpillBytes() > spillByteCap()) { + enforceSpilledResponseBudget(); + if (accountedResponseSpillBytes() > spillByteCap()) { + if (states.get(job.id) === candidate) { + spillCounters.writeFailures += 1; + replaceWithSpillFailure(job.id, candidate); + deferSupersededSpill(job.supersededSpill); + } + throw Object.assign(new Error("Response spill shutdown fallback exceeds the durable disk cap"), { code: "ENOSPC" }); + } + } ref = writeResponseSpillDurably(job.id, spillPayloadForResident(candidate), { aclBudgetMs }); if (ref.payloadBytes > responseSpillPayloadCap()) { deleteResponseSpill(ref); @@ -578,14 +623,17 @@ function supersedeShutdownFallbackBatch( const cleanupFailure = cleanupSupersededResponseSpillPublication(job.publicationControl); if (cleanupFailure) { failures.push(cleanupFailure); - // Cleanup failed, so the async temp or destination is STILL on the volume. Releasing + // Cleanup failed, so an async temp or destination is STILL on the volume. Releasing // the reservation would un-account a file that exists, and the fallback write that // follows reserves only its own footprint — three envelopes on disk priced as two. // - // These bytes are not a reservation: nothing will release them, because the file - // could not be removed. They are unreclaimable occupancy, and the cap has to keep - // seeing them or it stops describing the volume. - unreclaimableResponseSpillBytes += job.reservedBytes; + // Charge the surviving PATHS rather than a flat two envelopes: `clearOwnedPath` + // nulls whichever it managed to remove, so one failure is one file, not two. The + // charge is settled automatically once the path disappears, which a retried unlink + // or a released Windows lock can still do. + const perPath = Math.max(1, Math.floor(job.reservedBytes / 2)); + chargeUnreclaimableSpillPath(job.publicationControl.tempPath, perPath); + chargeUnreclaimableSpillPath(job.publicationControl.destinationPath, perPath); } releasePendingResponseSpill(job); } @@ -757,7 +805,7 @@ function accountedResponseSpillBytes(): number { if (job.supersededSpill) ownedBySpillJobs += job.supersededSpill.payloadBytes; } return spilledResponseBytes() + reservedResponseSpillBytes + ownedBySpillJobs - + unreclaimableResponseSpillBytes; + + reconcileUnreclaimableSpillPaths(); } /** Test-only: lower/restore the durable spill cap (null restores the default). */ @@ -2226,7 +2274,7 @@ export function clearResponseStateForTests(): void { for (const entry of states.values()) deleteOwnedSpills(entry); clearResponseStateMemoryForTests(); reservedResponseSpillBytes = 0; - unreclaimableResponseSpillBytes = 0; + unreclaimableSpillPaths.clear(); try { unlinkSync(snapshotPath()); } catch { diff --git a/tests/responses-state.test.ts b/tests/responses-state.test.ts index b84d93f658..15971a3b0d 100644 --- a/tests/responses-state.test.ts +++ b/tests/responses-state.test.ts @@ -135,6 +135,17 @@ function spillTempNames(home: string): string[] { return existsSync(dir) ? readdirSync(dir).filter(name => name.endsWith(".tmp")) : []; } +/** Real bytes the spill directory occupies, temps included. */ +function bytesOnDisk(home: string): number { + const dir = responseSpillDirectory(home); + if (!existsSync(dir)) return 0; + let total = 0; + for (const name of readdirSync(dir)) { + try { total += statSync(join(dir, name)).size; } catch { /* raced with an unlink */ } + } + return total; +} + interface ShutdownBudgetChildResult { settled: boolean; reported: boolean; @@ -915,6 +926,16 @@ describe("Responses previous_response_id state", () => { expect(existingBytes).toBeGreaterThan(0); expect(spillFileNames(home)).toHaveLength(1); + // Room for the two-envelope publication and nothing more. The seeded spill does not + // fit alongside it, so correct admission must reclaim it before publishing; without + // the check, seeded + temp + destination sit on disk together and blow the cap. + // Generous enough that this publication is admitted: the point of THIS test is that + // the accounting sees the in-flight bytes. The cap-refusal behaviour is proven + // separately below, where admission is the only thing standing between the request + // and an over-budget directory. + const spillCap = existingBytes * 4; + setSpilledResponseByteCapForTests(spillCap); + // Force the exclusive-copy fallback, then gate the destination hardening that follows // it, so the observation below happens with BOTH files present. setSpillIoForTest({ @@ -935,6 +956,10 @@ describe("Responses previous_response_id state", () => { // covers what is actually on the volume. expect(getAccountedResponseSpillBytesForTests()) .toBeGreaterThanOrEqual(existingBytes * 3); + // And the bytes ACTUALLY on disk stay inside the configured cap. This is the + // assertion the admission check has to earn: without it, the seeded spill plus the + // temp plus the destination copy exceed a cap sized for two envelopes. + expect(bytesOnDisk(home)).toBeLessThanOrEqual(spillCap); } finally { release(); setSpillIoForTest(null); @@ -951,6 +976,40 @@ describe("Responses previous_response_id state", () => { .toContain("xxxxxxxx"); }); + test("refuses a publication whose peak footprint does not fit the disk cap", async () => { + // Enforcement, not accounting: a publication that cannot fit must be refused BEFORE + // any file is created. Deleting the overflow afterwards is not equivalent — on + // Windows the file outlives the decision by however long ACL hardening takes, which + // is the window the measured 6.8 GiB accumulated in. + setPlatformForTests("win32"); + setResponseStateByteCapForTests(1_024); + setAsyncIcaclsRunnerForTests(async () => ({ success: true, exitCode: 0, timedOut: false, stdout: "" })); + + rememberLarge("resp_cap_seed", "s".repeat(8_000)); + await flushPendingResponseSpillsForTests(); + const seededBytes = getSpilledResponseBytesForTests(); + expect(spillFileNames(home)).toHaveLength(1); + + // Room for the seeded spill and nothing else. The next publication needs two + // envelopes at peak, and the seeded file is live rather than reclaimable-on-sight, + // so admission has to refuse. + setSpilledResponseByteCapForTests(Math.floor(seededBytes * 1.5)); + + rememberLarge("resp_cap_refused", "r".repeat(8_000)); + await flushPendingResponseSpillsForTests(); + + // No second file, and no temp left behind: the refusal happened before publication. + expect(spillFileNames(home)).toHaveLength(1); + expect(spillTempNames(home)).toHaveLength(0); + expect(bytesOnDisk(home)).toBeLessThanOrEqual(Math.floor(seededBytes * 1.5)); + // Fail-closed, and it says so: the refused continuation is a spill failure, not a + // silent drop, so replay reports it and the client resends. + expect(responseStateMetrics()).toMatchObject({ spillWriteFailures: 1 }); + // The seeded continuation is untouched — a refusal must not cost an unrelated replay. + expect(JSON.stringify(expandPreviousResponseInput({ previous_response_id: "resp_cap_seed", input: "next" }))) + .toContain("ssssssss"); + }); + test("Windows spill retries one transient ACL timeout without installing a tombstone", async () => { setPlatformForTests("win32"); process.env.OPENCODEX_ACL_TIMEOUT_MS = "1000"; From 663ce6130de3ab2985aa036cad19a353228e338d Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Tue, 1 Sep 2026 02:09:03 +0900 Subject: [PATCH 5/6] fix(responses): price the superseded generation in the shutdown fallback too Third review round found the last accounting hole, and it is shutdown-only. supersedeShutdownFallbackBatch releases the job, which takes it out of pendingResponseSpills and therefore out of the accounting walk - but its superseded generation is still a file on the volume until deferSupersededSpill or a delete takes it. The fallback preflight priced cleanup debt plus its own footprint and missed that envelope entirely. The gap is reachable: same-id replacement owns an old generation O, async cleanup fails leaving path debt D, and the fallback publishes footprint F. A cap sitting between D+F and O+D+F admits a publication that puts the directory over budget - which is the shape this whole phase exists to prevent. Regression covers exactly that: first generation settles, a same-id replacement makes the job its owner, the cap is set between the two totals, and the drain must refuse rather than add a third envelope. Red without the fix. --- src/responses/state.ts | 10 +++++-- tests/responses-state.test.ts | 51 +++++++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 2 deletions(-) diff --git a/src/responses/state.ts b/src/responses/state.ts index e955abe4f2..b95a1fa2c6 100644 --- a/src/responses/state.ts +++ b/src/responses/state.ts @@ -545,14 +545,20 @@ function installShutdownFallbackSpill( const footprint = publicationFootprintBytes(job.id, candidate); reservedResponseSpillBytes += footprint; try { + // Supersession released this job, so its superseded generation is no longer visible + // to the accounting walk — but the file is still on the volume until + // `deferSupersededSpill` or a delete takes it. Price it here or the fallback decides + // against a total short by that whole envelope, which is exactly the gap that lets + // `debt + footprint <= cap < old + debt + footprint` publish over budget. + const supersededBytes = job.supersededSpill?.payloadBytes ?? 0; // The drain must not publish over the cap either. Reclaim first; if the footprint // still does not fit — which is what unreclaimable cleanup debt looks like — the // honest close-out is a tombstone, not another file on a volume that is already // over budget. `replaceWithSpillFailure` is the same fail-closed ending the budget // exhaustion path uses, so replay reports `spill_failed` and the client resends. - if (accountedResponseSpillBytes() > spillByteCap()) { + if (accountedResponseSpillBytes() + supersededBytes > spillByteCap()) { enforceSpilledResponseBudget(); - if (accountedResponseSpillBytes() > spillByteCap()) { + if (accountedResponseSpillBytes() + supersededBytes > spillByteCap()) { if (states.get(job.id) === candidate) { spillCounters.writeFailures += 1; replaceWithSpillFailure(job.id, candidate); diff --git a/tests/responses-state.test.ts b/tests/responses-state.test.ts index 15971a3b0d..8f22fd444d 100644 --- a/tests/responses-state.test.ts +++ b/tests/responses-state.test.ts @@ -1273,6 +1273,57 @@ describe("Responses previous_response_id state", () => { } }); + test("shutdown fallback prices the job-owned superseded generation before publishing", async () => { + // Supersession releases the job, so its superseded generation leaves the accounting + // walk while the FILE stays on the volume. Without pricing it, the fallback decides + // against a total short by a whole envelope, and a cap sitting between + // (debt + footprint) and (old + debt + footprint) admits a publication that puts the + // directory over budget. + setPlatformForTests("win32"); + setResponseSpillShutdownBudgetForTests({ totalMs: 120, fallbackReserveMs: 80 }); + let release!: () => void; + let entered!: () => void; + const gate = new Promise(resolve => { release = resolve; }); + const started = new Promise(resolve => { entered = resolve; }); + let announced = false; + setAsyncIcaclsRunnerForTests(async () => { + if (!announced) { + announced = true; + entered(); + } + await gate; + return { success: true, exitCode: 0, timedOut: false, stdout: "" }; + }); + setIcaclsRunnerForTests(() => ({ success: true, exitCode: 0, timedOut: false, stdout: "" })); + setResponseStateByteCapForTests(1_024); + + // First generation settles to a real file, then a same-id replacement makes the job + // the owner of that superseded generation. + rememberLarge("resp_shutdown_superseded", "o".repeat(8_000)); + await flushPendingResponseSpillsForTests(); + const oldBytes = getSpilledResponseBytesForTests(); + expect(oldBytes).toBeGreaterThan(0); + + rememberLarge("resp_shutdown_superseded", "n".repeat(8_000)); + await started; + + try { + // Cap allows the new publication on its own, but not alongside the superseded + // generation the job still owns. + setSpilledResponseByteCapForTests(Math.floor(oldBytes * 2.4)); + // The refusal surfaces as a shutdown failure, which is the honest signal: the + // operator learns a continuation was dropped rather than the volume being + // silently overfilled. + await expect(flushResponseState()).rejects.toThrow(/shutdown fallback incomplete/); + // Fail-closed rather than over-budget: the drain refused to add a third envelope. + expect(bytesOnDisk(home)).toBeLessThanOrEqual(Math.floor(oldBytes * 2.4)); + expect(spillTempNames(home)).toHaveLength(0); + expect(pendingResponseSpillMetricsForTests()).toEqual({ count: 0, bytes: 0 }); + } finally { + release(); + } + }); + test("shutdown fallback spends only its reserved ACL budget", async () => { setPlatformForTests("win32"); const totalMs = 500; From cf1f6617be9e644b2fbde30607e7968514de5c0e Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Tue, 1 Sep 2026 02:13:59 +0900 Subject: [PATCH 6/6] docs(devlog): record what wp2's review rounds changed about the plan --- .../020_wp2_spill_disk_budget.md | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/devlog/_plan/260831_prio70_train_round2/020_wp2_spill_disk_budget.md b/devlog/_plan/260831_prio70_train_round2/020_wp2_spill_disk_budget.md index 98ffee3bfe..2e0f7a6a22 100644 --- a/devlog/_plan/260831_prio70_train_round2/020_wp2_spill_disk_budget.md +++ b/devlog/_plan/260831_prio70_train_round2/020_wp2_spill_disk_budget.md @@ -128,6 +128,32 @@ Suite, typecheck and privacy scan on `ssh lidge`. Residual risk, same as round 1's wp3: NTFS unlink semantics and `icacls` timeout behaviour while a path is held still want a real Windows host. + +## What implementation added beyond this plan + +Four adversarial review rounds against the built branch (findings 4, 3, 1, 0). Three of +their findings changed the design rather than the code, so they belong here: + +- **The footprint is measured, not estimated.** The plan said "exact prospective + measurement" and the first implementation used `candidate.sizeBytes`, which omits the + `version` field the published envelope carries. `prospectiveResponseSpillBytes` now + shares `serializedSpill` itself, so the two cannot drift. +- **Superseded generations are priced in two places, not one.** A same-id replacement + takes the old spill off `states` and hands it to the pending job. It is invisible to + the accounting walk at admission (the job does not exist yet) and again in the shutdown + fallback (supersession has already released the job). Both checks add it explicitly. +- **Cleanup debt is per path and repayable.** The plan did not anticipate a failed + unlink. A flat charge that never decrements is phantom debt: a Windows lock that clears, + or the async writer's own retry, removes the file while the charge stays, and two + conservative 256 MiB charges consume the whole default cap for the life of the process. + The debt is keyed by path and settled when the path is gone. +- **The shutdown fallback fails closed.** If the footprint still does not fit after + reclaim, it terminalizes with ENOSPC rather than publishing onto an over-budget volume. + +And one about verification: the first regression stayed green with the admission check +deleted, because `pruneResponses` reclaimed on another path. It proved the counter, not +the cap. There are now two tests — one for accounting during a forced copy fallback, one +for enforcement — and the enforcement test is red when admission is removed. ### Amendment after audit round 1 (`002`, blocker 3): the peak is two envelopes The first draft reserved one serialized envelope. Windows publication can fall back