From d75a2402f7752724ecc24ecf8d439e7f702d388b Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Wed, 19 Aug 2026 09:48:29 +0900 Subject: [PATCH 1/7] docs(devlog): plan response-state temp reclaim as a two-layer stack --- .../000_plan.md | 71 +++++++++++ .../010_phase1_periodic_sweeper.md | 119 ++++++++++++++++++ .../020_phase2_doctor_reclaim.md | 106 ++++++++++++++++ 3 files changed, 296 insertions(+) create mode 100644 devlog/_plan/260819_response_state_temp_reclaim/000_plan.md create mode 100644 devlog/_plan/260819_response_state_temp_reclaim/010_phase1_periodic_sweeper.md create mode 100644 devlog/_plan/260819_response_state_temp_reclaim/020_phase2_doctor_reclaim.md diff --git a/devlog/_plan/260819_response_state_temp_reclaim/000_plan.md b/devlog/_plan/260819_response_state_temp_reclaim/000_plan.md new file mode 100644 index 0000000000..a10cab2830 --- /dev/null +++ b/devlog/_plan/260819_response_state_temp_reclaim/000_plan.md @@ -0,0 +1,71 @@ +# 260819 — response-state temp reclaim + +## Objective + +Abandoned `responses-state.json.ocx...tmp` files can accumulate without +bound. A field report described ~19.6 GB of these files on one machine. Make the +existing reclaim run on a schedule that does not depend on serving traffic, and give +an operator a way to reclaim them when the proxy will not start at all. + +## Evidence (verified against this tree at 59964ad77) + +- `src/config.ts:293` — `atomicWriteFileAsync` names its temp + `${target}.ocx.${process.pid}.${++_atomicSeq}.tmp`. This is the exact reported shape. +- `src/responses/state.ts:26` — `SNAPSHOT_TOTAL_MAX_BYTES` is 24 MiB, and the snapshot + is rewritten whole on every persist. One abandoned temp is therefore up to 24 MiB, + which matches the reported 20–27 MB per file. +- `src/responses/state.ts:548` — `recoverStaleResponseStateTemps` already implements + the reclaim, with a 15-minute age gate, a PID-liveness check, a regular-file check, + and bounded scan/cleanup counts. **The reclaim logic is correct and is not the defect.** +- `src/responses/state.ts:621` — its ONLY caller is `ensureLoaded()`, which is lazy and + runs on first continuation access (`state.ts:991`, `:1073`, `:1185`). + +## Root cause + +The reclaim is attached to the request path. A proxy that crashes before serving a +continuation request leaves its temp behind and never reaches the code that would +reclaim it. The condition that produces the garbage is the same condition that +disables the collector, so the file count only ever grows. + +This is a scheduling defect, not a missing-feature defect. Both layers below move or +add a CALLER; neither changes reclaim semantics. + +## Scope + +IN: caller placement for the existing reclaim; an operator-facing reclaim path. + +OUT: the 24 MiB whole-file rewrite. Incremental snapshotting would reduce the blast +radius per failure, but it changes the durability contract of the continuation cache +and is a much larger risk surface. It is recorded here as a known residual, not +silently dropped. + +OUT: `src/storage/cleanup.ts` temps (`:1073`, `:2420`). Different owner, different +lifecycle; if they share the defect it is a separate unit. + +## Work-phase map (dependency-ordered — PHASE-SPLIT-01) + +| # | Phase | Doc | Depends on | +|---|-------|-----|------------| +| 1 | Periodic reclaim via the state-store sweeper | `010_phase1_periodic_sweeper.md` | — | +| 2 | Operator reclaim via `ocx doctor` | `020_phase2_doctor_reclaim.md` | phase 1 | + +Phase 1 makes a RUNNING proxy self-healing. Phase 2 covers the case phase 1 cannot +reach — a proxy that will not start — and reuses the reporting shape phase 1 +establishes. The dependency runs upward, so the stack lands bottom-up. + +## Stack plan (DEV-STACK-01) + +Two layers. Phase 1 is mergeable alone and fixes the reported accumulation for every +user whose proxy runs at all; phase 2 builds on it. + +``` +codex/tmp-reclaim-2-doctor → PR #2 (base: codex/tmp-reclaim-1-sweeper) +codex/tmp-reclaim-1-sweeper → PR #1 (base: dev) +``` + +## Terminal criteria + +- A proxy that never serves a continuation request still reclaims abandoned temps. +- An operator whose proxy will not start can reclaim them with a documented command. +- No live temp is ever removed: the age gate and PID-liveness check stay intact. +- `bun run typecheck` and `bun run test` green before either PR is review-ready. diff --git a/devlog/_plan/260819_response_state_temp_reclaim/010_phase1_periodic_sweeper.md b/devlog/_plan/260819_response_state_temp_reclaim/010_phase1_periodic_sweeper.md new file mode 100644 index 0000000000..980c23eefb --- /dev/null +++ b/devlog/_plan/260819_response_state_temp_reclaim/010_phase1_periodic_sweeper.md @@ -0,0 +1,119 @@ +# Phase 1 — periodic reclaim via the state-store sweeper + +## Thesis + +Abandoned response-state temps are reclaimed on a timer, so a proxy that never serves +a continuation request still cleans up after a previous crash. + +## Why the sweeper is the right owner + +`src/lib/state-store-sweeper.ts` already runs every 60 s (`STATE_SWEEP_INTERVAL_MS`), +is started once per process from `startProcessLoops` in +`src/server/background-lifecycle.ts:59`, is `unref`'d so it cannot hold the process +open, and wraps every callback in try/catch with `logCallbackFailure`. The +`responses-continuation` store is ALREADY registered there +(`src/lib/state-store-registrations.ts:87`) for TTL eviction. Disk reclaim for the +same subsystem belongs on the same tick. + +`sweepExpired` is the wrong slot: it is called by `sweepExpiredOnWrite` +(`state-store-sweeper.ts:91`) on write paths, and filesystem scans do not belong on a +write. `sweepLiveness` is the correct slot — it runs only on the interval tick +(`:162`), and "is the process that owns this temp still alive" is precisely a +liveness question. + +## Change map + +### MODIFY `src/responses/state.ts` + +Add an exported wrapper next to `sweepExpiredResponseStates` (after line 899). It +resolves the same two directories `ensureLoaded` sweeps (literal + symlink-resolved), +and returns a removed count so the sweeper's `rowsRemoved` accounting stays truthful. + +```ts +/** + * Periodic disk reclaim for abandoned atomic-write temps. `ensureLoaded` sweeps once on + * first continuation access, which never happens in the case that produces the garbage: + * a proxy that crashes before serving a continuation request leaves its temp behind and + * never reaches that path. Registered on the sweeper's liveness tick so reclaim does not + * depend on serving traffic. + */ +export function sweepAbandonedResponseStateTemps(): number { + const path = snapshotPath(); + let resolvedDir = dirname(path); + try { + resolvedDir = dirname(resolveWriteTarget(path)); + } catch { + /* unresolvable link: sweep the literal dir only */ + } + let removed = 0; + for (const dir of new Set([dirname(path), resolvedDir])) { + try { + removed += recoverStaleResponseStateTemps(dir).removed; + } catch { + /* best-effort: disk reclaim must never destabilize the sweeper tick */ + } + } + return removed; +} +``` + +No new imports: `dirname`, `resolveWriteTarget`, and `recoverStaleResponseStateTemps` +are all already in scope in this module. + +### MODIFY `src/lib/state-store-registrations.ts` + +Line 37 — extend the existing import: + +```diff +-import { sweepExpiredResponseStates } from "../responses/state"; ++import { sweepAbandonedResponseStateTemps, sweepExpiredResponseStates } from "../responses/state"; +``` + +Line 87 — extend the existing registration rather than adding a second store, so one +subsystem keeps one row: + +```diff +- { name: "responses-continuation", sweepExpired: sweepExpiredResponseStates }, ++ { ++ name: "responses-continuation", ++ sweepExpired: sweepExpiredResponseStates, ++ sweepLiveness: sweepAbandonedResponseStateTemps, ++ }, +``` + +### MODIFY `tests/responses-state.test.ts` + +Add a regression test asserting the reclaim runs without any continuation access — +the exact property that was missing. It must prove the negative: a stale temp is +removed while a live-PID temp and a young temp survive, with `ensureLoaded` never +driven. + +## Scope boundary + +IN: the wrapper, the registration, the test. + +OUT: any change to `recoverStaleResponseStateTemps` itself — its age gate, PID check, +file-type check, and bounds are already correct and independently tested +(`tests/responses-state.test.ts:1522`, `:1575`). Touching them would widen the blast +radius of a scheduling fix into a safety-critical one. + +OUT: startup one-shot reclaim. The first tick lands 60 s after start, which is +adequate for a defect measured in months of accumulation, and adding a startup call +would put a filesystem scan on the boot path. + +## Accept criteria + +| # | Scenario | Observable proof | +|---|----------|------------------| +| 1 | Sweeper tick with no continuation traffic | stale temp gone; `ensureLoaded` never invoked | +| 2 | Temp owned by a live PID | survives the tick | +| 3 | Temp younger than the 15-minute grace | survives the tick | +| 4 | Reclaim throws (unreadable dir) | tick completes; other stores still swept | + +Criterion 4 is the activation scenario for the new catch block +(C-ACTIVATION-GROUNDING-01): force `list` to throw and assert the tick still returns. + +## Verification + +`bun test tests/responses-state.test.ts`, then `bun run typecheck` and +`bun run test` before the PR is review-ready (shared runtime + registration table). diff --git a/devlog/_plan/260819_response_state_temp_reclaim/020_phase2_doctor_reclaim.md b/devlog/_plan/260819_response_state_temp_reclaim/020_phase2_doctor_reclaim.md new file mode 100644 index 0000000000..4bb61a0d1c --- /dev/null +++ b/devlog/_plan/260819_response_state_temp_reclaim/020_phase2_doctor_reclaim.md @@ -0,0 +1,106 @@ +# Phase 2 — operator reclaim via `ocx doctor` + +## Thesis + +An operator whose proxy will not start can reclaim abandoned response-state temps with +a documented command, instead of being told to hand-craft a `find -delete`. + +## Why this layer exists on top of phase 1 + +Phase 1 covers every proxy that runs. It cannot cover the reported case at its worst: +a proxy stuck in a crash loop never reaches a sweeper tick either, because the tick +lives in the same process. The field report described exactly that state — scheduled +task installed, proxy not running, disk filling. For that operator the only in-product +recovery is a command that runs WITHOUT the server. + +Depends on phase 1 for `sweepAbandonedResponseStateTemps`: the doctor path reuses the +same two-directory resolution, so the two surfaces cannot disagree about which files +are reclaimable. + +## Change map + +### MODIFY `src/cli/doctor.ts` + +`runDoctor` (`:768`) currently branches on `--fix-codex-runtime`. Add a reporting +section to the default path, and reclaim only when explicitly asked. + +- Default `ocx doctor`: REPORT matched temps and their total bytes. Read-only. +- `ocx doctor --reclaim-response-temps`: perform the reclaim and print what was freed. + +Report-by-default is deliberate. `doctor` is a diagnostic an operator runs to +understand a machine; deleting files as a side effect of asking a question is the +wrong default, even for cache files. + +```ts +const reclaim = args.includes("--reclaim-response-temps"); +const result = reclaim + ? reclaimAbandonedResponseStateTemps() + : inspectAbandonedResponseStateTemps(); +if (result.matched === 0) { + console.log("Response-state temps: none abandoned."); +} else if (reclaim) { + console.log(`Response-state temps: reclaimed ${result.removed} file(s), ${formatBytes(result.bytesRemoved)} freed.`); + if (result.failed > 0) console.log(` ${result.failed} file(s) could not be removed (in use or locked).`); +} else { + console.log(`Response-state temps: ${result.matched} abandoned file(s), ${formatBytes(result.bytes)} reclaimable.`); + console.log(" Run: ocx doctor --reclaim-response-temps"); +} +``` + +### MODIFY `src/responses/state.ts` + +Export a dry-run counterpart so doctor can report without deleting. It reuses +`recoverStaleResponseStateTemps` with an injected no-op `unlink`, so the SAME +selection predicate decides both report and reclaim — a separate matcher would drift. + +```ts +/** Report-only counterpart: same selection predicate, no removal. */ +export function inspectAbandonedResponseStateTemps(): { matched: number; bytes: number } { + // ... resolve both dirs as in sweepAbandonedResponseStateTemps, + // call recoverStaleResponseStateTemps(dir, { unlink: () => {} }) and sum. +} +``` + +Note the existing accounting detail: `bytesRemoved` only accrues on a successful +`unlink` (`state.ts:590`), so with a no-op unlink the byte total must come from +`inspect`. Confirm against the implementation during B and adjust the wrapper — this +is the one place the reuse is not free. + +### MODIFY `docs-site/` + +Document the flag on the troubleshooting/disk-usage page, including what the files are +and why they are safe to remove (continuation cache, not durable state). + +### MODIFY `tests/` + +Doctor-level test: report mode leaves files intact; reclaim mode removes only stale +ones. Live-PID and young-file protection is already covered by phase 1's tests and is +not re-asserted here. + +## Scope boundary + +IN: the doctor surface, the dry-run export, docs, tests. + +OUT: an auto-reclaim-on-start behavior. That would run before the crash that is being +diagnosed, and silently deleting evidence during a crash loop is hostile to whoever is +debugging it. + +OUT: reclaiming any other subsystem's temps under the same flag. The flag names +response temps and reclaims only those. + +## Accept criteria + +| # | Scenario | Observable proof | +|---|----------|------------------| +| 1 | `ocx doctor` with abandoned temps present | count + bytes reported; files still on disk | +| 2 | `ocx doctor --reclaim-response-temps` | stale files removed; freed bytes printed | +| 3 | No abandoned temps | clean single-line report, no flag suggestion | +| 4 | Proxy not running | both paths work — no server dependency | + +Criterion 4 is the whole point of the layer: assert the code path imports nothing that +requires a live server. + +## Verification + +Focused doctor + state tests, then `bun run typecheck` and `bun run test` before the +PR is review-ready. From 265a21abe375fcc7fce549879c62ed8e8328328d Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Wed, 19 Aug 2026 10:07:17 +0900 Subject: [PATCH 2/7] docs(devlog): fold audit round 1 into the reclaim roadmap --- .../001_audit_round1.md | 93 +++++++++++++++++++ .../010_phase1_periodic_sweeper.md | 87 ++++++++++++++--- .../020_phase2_doctor_reclaim.md | 34 ++++--- 3 files changed, 192 insertions(+), 22 deletions(-) create mode 100644 devlog/_plan/260819_response_state_temp_reclaim/001_audit_round1.md diff --git a/devlog/_plan/260819_response_state_temp_reclaim/001_audit_round1.md b/devlog/_plan/260819_response_state_temp_reclaim/001_audit_round1.md new file mode 100644 index 0000000000..45f4795ade --- /dev/null +++ b/devlog/_plan/260819_response_state_temp_reclaim/001_audit_round1.md @@ -0,0 +1,93 @@ +# Audit round 1 — independent adversarial review of the roadmap + +Reviewer: independent `explorer` subagent, read-only, dispatched against this worktree +at `d75a2402f`. Verdict: **GO-WITH-FIXES (blockers=3)**. Main-agent judgment: +**near-pass** — every blocker is folded below as a concrete amendment; no blocker was +rebutted. + +A first reviewer produced nothing across four wait cycles (~11 min) and was retired as +a failed dispatch; this is the replacement's round, with a tighter falsify-this packet. + +## Confirmed by the reviewer + +- **Root cause holds (Q1).** `recoverStaleResponseStateTemps` has exactly one call + site — `state.ts:621` inside `ensureLoaded` — and `ensureLoaded` is reached only from + `:991`, `:1073`, `:1185`, all request-path. The 60 s tick's existing + `sweepExpiredResponseStates` (`:890`) touches only the in-memory map and never disk. + No off-request-path caller exists, so the plan is not misdirected. +- **`sweepLiveness` is the right slot (Q2).** `sweepExpiredOnWrite` + (`state-store-sweeper.ts:91`) is called from write paths — `key-failover.ts:171`, + `subagent-model-fallback.ts:321`, `gcp-adc.ts:324` — and `runCallbacks` fans out to + every registration, so a directory scan on `sweepExpired` would run `opendir` plus up + to 4096 `lstat`s on hot write paths. `sweepLiveness` has exactly one caller, the + interval body at `:161-162`, with `sweepDeadOcxStartProcessCache` as precedent for + syscall work in that slot. +- **The sweeper is ungated (Q3).** `startStateStoreSweeper()` runs unconditionally via + `background-lifecycle.ts:59` ← `:129` ← `index.ts:718`. Independently re-verified. + Phase 1 therefore reaches every affected user. +- **Windows liveness is correct (Q5).** `process.kill(pid, 0)` maps to `OpenProcess`; + `ESRCH` means gone, `EPERM` means alive-but-unsignallable, and `state.ts:520` treats + only `ESRCH` as dead. No change needed. + +## Blocker 1 (accepted, HIGH) — pid reuse makes the skip permanent + +`state.ts:582` skips a temp whose pid is alive. The 15-minute gate at `:581` is a +LOWER bound, so it never expires the skip: once a dead writer's pid is reused by any +live process, that temp is skipped on every future pass forever. + +This matters more than the original scheduling defect for the reported case. Reboots +recycle low pids deterministically, and the field report was specifically about +**per-reboot accumulation**. The scheduling defect explains why nothing cleaned up; +pid reuse explains why the files survived even the passes that did run. + +**Amendment (phase 1, additive):** add a boot-time floor. A temp whose `mtimeMs` +predates system boot cannot belong to any currently-live pid, so the liveness check is +provably vacuous for it. Reclaim when `file.mtimeMs < bootMs - skew` in ADDITION to the +existing gates; every original guard stays intact. `bootMs` derives from +`os.uptime()` and becomes an injectable IO member for testability. + +This moves phase 1 from "runs on a timer" to "actually reclaims the reported files", +so it belongs in the bottom layer, not deferred. + +## Blocker 2 (accepted) — the callback must be synchronous and self-bounding + +`runCallbacks` (`state-store-sweeper.ts:66-84`) discards a returned promise, so an +`async` reclaim would swallow every error and defeat its `try/catch`. The signature is +`() => number`, so the wrapper must be sync and return a real removed count. + +The reviewer also notes the inverted risk: a synchronous scan BLOCKS the event loop, so +the startup-scale `maxEntries = 4096` budget is wrong for a 60 s repeating tick on a +slow or network-mounted config dir. + +**Amendment (phase 1):** the wrapper stays synchronous, and the periodic path passes a +smaller `maxEntries`/`maxCleanups` budget than the startup path. Reclaim is idempotent +and repeats every 60 s, so a smaller per-tick budget loses nothing. + +## Blocker 3 (accepted) — symlink resolution must be shared, not duplicated + +The two-directory resolution lives inside `ensureLoaded` (`:604-625`). A callback that +swept only `getConfigDir()` would miss temps stranded in a symlinked snapshot's real +directory — the exact case the comment at `:606-610` documents. + +**Amendment (phase 1):** extract `new Set([dirname(path), resolvedDir])` into one shared +helper used by BOTH `ensureLoaded` and the new callback, so the two surfaces cannot +drift. The 010 doc already sweeps both directories; this makes it a single source. + +## Self-found defects (main agent, during WP0 verification) + +- **(a) Phase 2's dry run is wrong as written.** `020` proposed reusing + `recoverStaleResponseStateTemps` with a no-op `unlink`. But `state.ts:586-590` + increments `removed` and accrues `bytesRemoved` only INSIDE the successful-unlink + branch, so a no-op unlink reports `removed` as if files were deleted while + `bytesRemoved` stays truthful — inverted from what the doc claims. `maxCleanups` also + bounds a report-only pass. Phase 2 needs an explicit `dryRun` mode with its own + accounting, not injected-IO trickery. +- **(b) The options type is not exported.** `ResponseStateTempRecoveryOptions` + (`state.ts:507`) is module-private, so out-of-module IO injection does not typecheck. + Phase 2 must export it or expose a purpose-built wrapper. + +## Residual (not blocking, recorded) + +The 24 MiB whole-file rewrite stays out of scope. It bounds the SIZE of each leaked +file, not the leak; changing it alters the continuation cache's durability contract and +deserves its own unit. diff --git a/devlog/_plan/260819_response_state_temp_reclaim/010_phase1_periodic_sweeper.md b/devlog/_plan/260819_response_state_temp_reclaim/010_phase1_periodic_sweeper.md index 980c23eefb..d0dc0aeb0b 100644 --- a/devlog/_plan/260819_response_state_temp_reclaim/010_phase1_periodic_sweeper.md +++ b/devlog/_plan/260819_response_state_temp_reclaim/010_phase1_periodic_sweeper.md @@ -5,6 +5,10 @@ Abandoned response-state temps are reclaimed on a timer, so a proxy that never serves a continuation request still cleans up after a previous crash. +Amended after audit round 1 (`001_audit_round1.md`): the timer alone does not reclaim +the reported files, because a reused pid makes the liveness skip permanent. This layer +therefore ships the boot-time floor with it. + ## Why the sweeper is the right owner `src/lib/state-store-sweeper.ts` already runs every 60 s (`STATE_SWEEP_INTERVAL_MS`), @@ -23,13 +27,70 @@ liveness question. ## Change map +### MODIFY `src/responses/state.ts` — boot-time floor (audit blocker 1) + +`recoverStaleResponseStateTemps` skips a temp whose pid is alive (`:582`). The 15-minute +gate at `:581` is a LOWER bound and never expires that skip, so a pid reused after a +reboot strands the file forever. A temp whose `mtimeMs` predates system boot cannot +belong to any live pid, so the liveness probe is provably vacuous for it. + +Add `bootTime: () => number` to `ResponseStateTempRecoveryIO` (default +`() => Date.now() - os.uptime() * 1000`) and reclaim when the file predates boot, in +ADDITION to the existing gates: + +```diff +- if (pid === process.pid || io.isProcessAlive(pid)) continue; ++ // A temp written before the current boot cannot belong to any live pid: after a ++ // reboot the original writer's pid is routinely reused, which would otherwise make ++ // the liveness skip permanent (the 15-minute gate is a lower bound, so it never ++ // expires it). Every other guard still applies. ++ const predatesBoot = file.mtimeMs < io.bootTime() - BOOT_FLOOR_SKEW_MS; ++ if (!predatesBoot && (pid === process.pid || io.isProcessAlive(pid))) continue; ++ if (predatesBoot && pid === process.pid) continue; +``` + +`BOOT_FLOOR_SKEW_MS = 60_000` absorbs clock skew and `os.uptime()` granularity. The +`pid === process.pid` guard is kept unconditionally: this process is by definition +younger than boot, and must never unlink its own in-flight temp. + +### MODIFY `src/responses/state.ts` — shared directory resolution (audit blocker 3) + +The literal + symlink-resolved pair is computed inside `ensureLoaded` (`:604-625`). A +callback sweeping only `getConfigDir()` would miss temps stranded in a symlinked +snapshot's real directory. Extract it once and use it from BOTH callers: + +```ts +/** Literal config dir plus the snapshot's resolved dir; identical when nothing is symlinked. */ +function responseStateSweepDirectories(): Set { + const path = snapshotPath(); + let resolvedDir = dirname(path); + try { + resolvedDir = dirname(resolveWriteTarget(path)); + } catch { + /* unresolvable link: sweep the literal dir only */ + } + return new Set([dirname(path), resolvedDir]); +} +``` + ### MODIFY `src/responses/state.ts` Add an exported wrapper next to `sweepExpiredResponseStates` (after line 899). It resolves the same two directories `ensureLoaded` sweeps (literal + symlink-resolved), and returns a removed count so the sweeper's `rowsRemoved` accounting stays truthful. +It MUST be synchronous (audit blocker 2): `runCallbacks` discards a returned promise, +so an `async` reclaim would swallow every error and defeat its `try/catch`. It also +passes a smaller per-tick budget than the startup path — 4096 entries is a startup-scale +budget, and a synchronous scan blocks the event loop. Reclaim is idempotent and repeats +every 60 s, so a smaller budget costs nothing. + ```ts +/** Per-tick budget. Smaller than the startup budget: this runs every 60 s, synchronously, + * on the event loop, and any remainder is reclaimed by the next tick. */ +const PERIODIC_TEMP_MAX_ENTRIES = 512; +const PERIODIC_TEMP_MAX_CLEANUPS = 64; + /** * Periodic disk reclaim for abandoned atomic-write temps. `ensureLoaded` sweeps once on * first continuation access, which never happens in the case that produces the garbage: @@ -38,17 +99,13 @@ and returns a removed count so the sweeper's `rowsRemoved` accounting stays trut * depend on serving traffic. */ export function sweepAbandonedResponseStateTemps(): number { - const path = snapshotPath(); - let resolvedDir = dirname(path); - try { - resolvedDir = dirname(resolveWriteTarget(path)); - } catch { - /* unresolvable link: sweep the literal dir only */ - } let removed = 0; - for (const dir of new Set([dirname(path), resolvedDir])) { + for (const dir of responseStateSweepDirectories()) { try { - removed += recoverStaleResponseStateTemps(dir).removed; + removed += recoverStaleResponseStateTemps(dir, { + maxEntries: PERIODIC_TEMP_MAX_ENTRIES, + maxCleanups: PERIODIC_TEMP_MAX_CLEANUPS, + }).removed; } catch { /* best-effort: disk reclaim must never destabilize the sweeper tick */ } @@ -57,8 +114,8 @@ export function sweepAbandonedResponseStateTemps(): number { } ``` -No new imports: `dirname`, `resolveWriteTarget`, and `recoverStaleResponseStateTemps` -are all already in scope in this module. +New import: `uptime` from `node:os` for the boot floor. `dirname`, `resolveWriteTarget`, +and `recoverStaleResponseStateTemps` are already in scope. ### MODIFY `src/lib/state-store-registrations.ts` @@ -109,11 +166,19 @@ would put a filesystem scan on the boot path. | 2 | Temp owned by a live PID | survives the tick | | 3 | Temp younger than the 15-minute grace | survives the tick | | 4 | Reclaim throws (unreadable dir) | tick completes; other stores still swept | +| 5 | Temp predating boot whose pid is now LIVE (reuse) | reclaimed — the permanent-skip case | +| 6 | Temp predating boot owned by THIS process | survives; never unlink our own in-flight temp | +| 7 | Symlinked snapshot dir | temp in the resolved real dir is reclaimed | Criterion 4 is the activation scenario for the new catch block (C-ACTIVATION-GROUNDING-01): force `list` to throw and assert the tick still returns. +Criterion 5 is the activation scenario for the boot floor: without it the file is +skipped forever, so the test must fail if the floor is removed. ## Verification `bun test tests/responses-state.test.ts`, then `bun run typecheck` and `bun run test` before the PR is review-ready (shared runtime + registration table). +Also `bun test tests/state-store-sweeper.test.ts`: its "global fake-clock sweep" +assertion derives from `STATE_STORE_REGISTRATIONS`, so adding a `sweepLiveness` member +changes what that test expects. diff --git a/devlog/_plan/260819_response_state_temp_reclaim/020_phase2_doctor_reclaim.md b/devlog/_plan/260819_response_state_temp_reclaim/020_phase2_doctor_reclaim.md index 4bb61a0d1c..a81e632c12 100644 --- a/devlog/_plan/260819_response_state_temp_reclaim/020_phase2_doctor_reclaim.md +++ b/devlog/_plan/260819_response_state_temp_reclaim/020_phase2_doctor_reclaim.md @@ -49,22 +49,32 @@ if (result.matched === 0) { ### MODIFY `src/responses/state.ts` -Export a dry-run counterpart so doctor can report without deleting. It reuses -`recoverStaleResponseStateTemps` with an injected no-op `unlink`, so the SAME -selection predicate decides both report and reclaim — a separate matcher would drift. +Export a dry-run counterpart so doctor can report without deleting. It must share the +SAME selection predicate as the reclaim — a separate matcher would drift and the two +surfaces would disagree about which files are reclaimable. + +**Corrected after WP0 self-verification.** The original proposal (inject a no-op +`unlink`) is wrong: `state.ts:586-590` increments `removed` and accrues +`bytesRemoved` only inside the successful-unlink branch, so a no-op `unlink` still +reports `removed` as though files were deleted. `maxCleanups` also bounds a +report-only pass, truncating the count an operator is shown. And +`ResponseStateTempRecoveryOptions` (`state.ts:507`) is module-private, so +out-of-module IO injection does not typecheck at all. + +Add an explicit `dryRun` option to the shared function instead, with its own +accounting branch: ```ts -/** Report-only counterpart: same selection predicate, no removal. */ -export function inspectAbandonedResponseStateTemps(): { matched: number; bytes: number } { - // ... resolve both dirs as in sweepAbandonedResponseStateTemps, - // call recoverStaleResponseStateTemps(dir, { unlink: () => {} }) and sum. +// inside the loop, replacing the unconditional unlink: +if (dryRun) { + result.wouldRemove += 1; + result.bytesReclaimable += file.size; + continue; } ``` -Note the existing accounting detail: `bytesRemoved` only accrues on a successful -`unlink` (`state.ts:590`), so with a no-op unlink the byte total must come from -`inspect`. Confirm against the implementation during B and adjust the wrapper — this -is the one place the reuse is not free. +A dry run keeps every selection gate (basename, regular-file, age, boot floor, pid +liveness) and changes only the action. Report and reclaim then cannot disagree. ### MODIFY `docs-site/` @@ -96,6 +106,8 @@ response temps and reclaims only those. | 2 | `ocx doctor --reclaim-response-temps` | stale files removed; freed bytes printed | | 3 | No abandoned temps | clean single-line report, no flag suggestion | | 4 | Proxy not running | both paths work — no server dependency | +| 5 | Report then reclaim on the same fixture | reported count/bytes equal what reclaim removes | +| 6 | More stale temps than the cleanup budget | report is not truncated by `maxCleanups` | Criterion 4 is the whole point of the layer: assert the code path imports nothing that requires a live server. From 6d89332b61c13ccb80508422f14be331215e232f Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Wed, 19 Aug 2026 10:08:05 +0900 Subject: [PATCH 3/7] docs(devlog): lock the reclaim roadmap and record the pid-reuse second cause --- .../000_plan.md | 23 +++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/devlog/_plan/260819_response_state_temp_reclaim/000_plan.md b/devlog/_plan/260819_response_state_temp_reclaim/000_plan.md index a10cab2830..4eb73c9c19 100644 --- a/devlog/_plan/260819_response_state_temp_reclaim/000_plan.md +++ b/devlog/_plan/260819_response_state_temp_reclaim/000_plan.md @@ -28,7 +28,16 @@ reclaim it. The condition that produces the garbage is the same condition that disables the collector, so the file count only ever grows. This is a scheduling defect, not a missing-feature defect. Both layers below move or -add a CALLER; neither changes reclaim semantics. +add a CALLER; neither loosens a reclaim safety gate. + +**Second cause, found in audit round 1 (`001_audit_round1.md`).** Scheduling alone does +not explain the reported files surviving the passes that DID run. `state.ts:582` skips a +temp whose pid is alive, and the 15-minute gate at `:581` is a lower bound that never +expires that skip. After a reboot the original writer's pid is routinely reused, so the +file is skipped forever. That matches the reported symptom — accumulation measured per +reboot — more precisely than scheduling does. Phase 1 therefore ships a boot-time floor +alongside the timer; a reclaim that runs on schedule but still skips every file would be +a phantom fix. ## Scope @@ -46,13 +55,22 @@ lifecycle; if they share the defect it is a separate unit. | # | Phase | Doc | Depends on | |---|-------|-----|------------| -| 1 | Periodic reclaim via the state-store sweeper | `010_phase1_periodic_sweeper.md` | — | +| 1 | Periodic reclaim + boot-time floor | `010_phase1_periodic_sweeper.md` | — | | 2 | Operator reclaim via `ocx doctor` | `020_phase2_doctor_reclaim.md` | phase 1 | Phase 1 makes a RUNNING proxy self-healing. Phase 2 covers the case phase 1 cannot reach — a proxy that will not start — and reuses the reporting shape phase 1 establishes. The dependency runs upward, so the stack lands bottom-up. +Audit round 1 is recorded in `001_audit_round1.md` (research range, per LEXICO-SPLIT-01). + +## Roadmap lock + +This docs-only cycle closes with the map above final and 1:1 with the goalplan's +`wp1`/`wp2`. Both decade docs are written to diff-level precision, so each later cycle's +P begins by re-verifying its pre-written doc against the tree rather than designing then. +Appending a later work-phase stays allowed as a P-phase amendment if one is discovered. + ## Stack plan (DEV-STACK-01) Two layers. Phase 1 is mergeable alone and fixes the reported accumulation for every @@ -66,6 +84,7 @@ codex/tmp-reclaim-1-sweeper → PR #1 (base: dev) ## Terminal criteria - A proxy that never serves a continuation request still reclaims abandoned temps. +- A temp stranded by a reused pid across a reboot is reclaimed rather than skipped forever. - An operator whose proxy will not start can reclaim them with a documented command. - No live temp is ever removed: the age gate and PID-liveness check stay intact. - `bun run typecheck` and `bun run test` green before either PR is review-ready. From f138dac159a29696543d2fbf4d23dc0752ea9c90 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Wed, 19 Aug 2026 10:17:11 +0900 Subject: [PATCH 4/7] docs(devlog): fold audit round 2 into the phase 1 plan --- .../010_phase1_periodic_sweeper.md | 124 ++++++++++++++---- .../011_audit_round2.md | 64 +++++++++ 2 files changed, 162 insertions(+), 26 deletions(-) create mode 100644 devlog/_plan/260819_response_state_temp_reclaim/011_audit_round2.md diff --git a/devlog/_plan/260819_response_state_temp_reclaim/010_phase1_periodic_sweeper.md b/devlog/_plan/260819_response_state_temp_reclaim/010_phase1_periodic_sweeper.md index d0dc0aeb0b..ad7e01b50d 100644 --- a/devlog/_plan/260819_response_state_temp_reclaim/010_phase1_periodic_sweeper.md +++ b/devlog/_plan/260819_response_state_temp_reclaim/010_phase1_periodic_sweeper.md @@ -34,24 +34,66 @@ gate at `:581` is a LOWER bound and never expires that skip, so a pid reused aft reboot strands the file forever. A temp whose `mtimeMs` predates system boot cannot belong to any live pid, so the liveness probe is provably vacuous for it. -Add `bootTime: () => number` to `ResponseStateTempRecoveryIO` (default -`() => Date.now() - os.uptime() * 1000`) and reclaim when the file predates boot, in -ADDITION to the existing gates: +Amended by audit round 2 (`011_audit_round2.md`): the original justification — "a temp +predating boot cannot belong to any live pid" — is FALSE under a container sharing the +config dir, suspend-excluding `os.uptime()`, and network mtime skew. The real safety +argument is that the unconditional 15-minute grace at `:581` stays AHEAD of this gate. +The boot floor only retires a liveness probe that has become vacuous. + +Add `bootTime: () => number` to `ResponseStateTempRecoveryIO` (`:499`) AND to the +default literal `responseStateTempRecoveryIO` (`:524`, else typecheck fails): + +```diff + const responseStateTempRecoveryIO: ResponseStateTempRecoveryIO = { + now: Date.now, ++ bootTime: () => Date.now() - uptime() * 1_000, +``` + +Hoist the probe ABOVE the loop (one syscall per scan, not per entry) and guard it: + +```ts +const rawBoot = io.bootTime(); +// Not finite or in the future: treat the floor as absent rather than trusting it. +const bootMs = Number.isFinite(rawBoot) ? Math.min(rawBoot, io.now()) : Number.NEGATIVE_INFINITY; +``` + +Then, per entry: ```diff - if (pid === process.pid || io.isProcessAlive(pid)) continue; -+ // A temp written before the current boot cannot belong to any live pid: after a -+ // reboot the original writer's pid is routinely reused, which would otherwise make -+ // the liveness skip permanent (the 15-minute gate is a lower bound, so it never -+ // expires it). Every other guard still applies. -+ const predatesBoot = file.mtimeMs < io.bootTime() - BOOT_FLOOR_SKEW_MS; ++ // After a reboot the original writer's pid is routinely reused, which makes the ++ // liveness skip PERMANENT: the 15-minute gate at :581 is a lower bound and never ++ // expires it. A temp older than this boot cannot be owned by the pid we would be ++ // probing, so the probe is vacuous and we retire it — we do NOT claim the file is ++ // provably dead. The unconditional 15-minute grace above remains the safety floor, ++ // which is what keeps this sound under a shared-volume container, suspend-excluding ++ // uptime, or a network config dir, where the computed boot can land after real boot. ++ const predatesBoot = file.mtimeMs < bootMs - BOOT_FLOOR_SKEW_MS; + if (!predatesBoot && (pid === process.pid || io.isProcessAlive(pid))) continue; + if (predatesBoot && pid === process.pid) continue; ``` -`BOOT_FLOOR_SKEW_MS = 60_000` absorbs clock skew and `os.uptime()` granularity. The -`pid === process.pid` guard is kept unconditionally: this process is by definition -younger than boot, and must never unlink its own in-flight temp. +`BOOT_FLOOR_SKEW_MS = 60_000` absorbs granularity only; it is explicitly NOT what makes +the change safe (the named failure modes are hours, not seconds). The +`pid === process.pid` guard is kept unconditionally: this process must never unlink its +own in-flight temp. + +### MODIFY `src/responses/state.ts` — scan deadline (audit blocker 6) + +An entry cap bounds syscalls, not time: 512 synchronous `lstat`s is 2-5 ms on APFS but +5-10 s on an SMB/NFS config dir, which would block the event loop and stall in-flight +SSE streams. Add a wall-clock deadline inside the scan loop, with the entry cap kept as +a backstop: + +```ts +const SCAN_DEADLINE_MS = 25; +// inside the loop, alongside the existing bounds: +if (deadlineMs !== null && io.now() - startedAt > deadlineMs) break; +``` + +`deadlineMs` is an option, null for the startup path (unchanged behavior) and +`SCAN_DEADLINE_MS` for the periodic path. Reclaim is idempotent, so a truncated tick +simply resumes on the next one. ### MODIFY `src/responses/state.ts` — shared directory resolution (audit blocker 3) @@ -100,22 +142,41 @@ const PERIODIC_TEMP_MAX_CLEANUPS = 64; */ export function sweepAbandonedResponseStateTemps(): number { let removed = 0; - for (const dir of responseStateSweepDirectories()) { - try { + // The try encloses responseStateSweepDirectories() deliberately (audit blocker 4): + // recoverStaleResponseStateTemps already swallows its own list/iterator failures, so a + // catch around only that call would be unreachable. snapshotPath()/getConfigDir() can + // genuinely throw, and that is the failure this guard exists for. + try { + for (const dir of responseStateSweepDirectories()) { removed += recoverStaleResponseStateTemps(dir, { maxEntries: PERIODIC_TEMP_MAX_ENTRIES, maxCleanups: PERIODIC_TEMP_MAX_CLEANUPS, + deadlineMs: SCAN_DEADLINE_MS, }).removed; - } catch { - /* best-effort: disk reclaim must never destabilize the sweeper tick */ } + } catch { + /* best-effort: disk reclaim must never destabilize the sweeper tick */ } return removed; } ``` -New import: `uptime` from `node:os` for the boot floor. `dirname`, `resolveWriteTarget`, -and `recoverStaleResponseStateTemps` are already in scope. +New import: `uptime` from `node:os`. `dirname`, `resolveWriteTarget`, and +`recoverStaleResponseStateTemps` are already in scope. + +### MODIFY `tests/responses-state.test.ts` — existing fixtures (audit blocker 1) + +The existing test at `:1522` ages fixtures exactly 60 minutes and keeps `live` via +`isProcessAlive`. On a host booted <60 min ago (a normal CI runner) the boot floor would +bypass that skip and delete `live`, turning `removed: 1` into `removed: 2`. Inject +`bootTime: () => 0` there and at `:1575` to pin the floor out of those cases. + +### MODIFY `tests/state-store-sweeper.test.ts` — home isolation (audit blocker 5) + +Once `sweepLiveness` is registered, the fake-clock test at `:132` invokes the REAL +reclaim, and that describe block sets no `OPENCODEX_HOME` — so the suite would +`opendir` the developer's real `~/.opencodex` and could unlink real temps. Point +`OPENCODEX_HOME` at a temp dir for that block. ### MODIFY `src/lib/state-store-registrations.ts` @@ -149,10 +210,14 @@ driven. IN: the wrapper, the registration, the test. -OUT: any change to `recoverStaleResponseStateTemps` itself — its age gate, PID check, -file-type check, and bounds are already correct and independently tested -(`tests/responses-state.test.ts:1522`, `:1575`). Touching them would widen the blast -radius of a scheduling fix into a safety-critical one. +OUT: loosening any existing gate. The age gate, file-type check, unlink-only removal, +and `pid === process.pid` guard are unchanged; the boot floor is ADDITIVE and sits +behind the 15-minute grace. + +IN (widened by audit round 2): `recoverStaleResponseStateTemps` gains the boot floor and +the scan deadline, and its existing tests at `tests/responses-state.test.ts:1522`/`:1575` +gain `bootTime` injection. The earlier "no changes to this function" boundary was +unachievable once the pid-reuse leak was accepted as in scope. OUT: startup one-shot reclaim. The first tick lands 60 s after start, which is adequate for a defect measured in months of accumulation, and adding a startup call @@ -169,16 +234,23 @@ would put a filesystem scan on the boot path. | 5 | Temp predating boot whose pid is now LIVE (reuse) | reclaimed — the permanent-skip case | | 6 | Temp predating boot owned by THIS process | survives; never unlink our own in-flight temp | | 7 | Symlinked snapshot dir | temp in the resolved real dir is reclaimed | +| 8 | Temp predating boot but YOUNGER than the 15-min grace | survives — the grace outranks the floor | +| 9 | `bootTime` in the future / not finite | floor ignored; live-pid temps still skipped | -Criterion 4 is the activation scenario for the new catch block -(C-ACTIVATION-GROUNDING-01): force `list` to throw and assert the tick still returns. +Criterion 4 is the activation scenario for the wrapper's catch +(C-ACTIVATION-GROUNDING-01): make `responseStateSweepDirectories()` throw — NOT `list`, +which the reclaim already swallows internally, and which would leave the catch +unreachable. Criterion 5 is the activation scenario for the boot floor: without it the file is skipped forever, so the test must fail if the floor is removed. +Criterion 8 proves the ordering that carries the whole safety argument. ## Verification `bun test tests/responses-state.test.ts`, then `bun run typecheck` and `bun run test` before the PR is review-ready (shared runtime + registration table). -Also `bun test tests/state-store-sweeper.test.ts`: its "global fake-clock sweep" -assertion derives from `STATE_STORE_REGISTRATIONS`, so adding a `sweepLiveness` member -changes what that test expects. +Also `bun test tests/state-store-sweeper.test.ts`. Corrected by audit round 2: its +assertions do NOT change, because both the registration-name list (`:100`) and the +fake-clock test (`:132`) derive from `STATE_STORE_REGISTRATIONS` and we EXTEND the +existing `responses-continuation` entry rather than adding a store. That test does begin +invoking the real reclaim, which is why it needs `OPENCODEX_HOME` isolation. diff --git a/devlog/_plan/260819_response_state_temp_reclaim/011_audit_round2.md b/devlog/_plan/260819_response_state_temp_reclaim/011_audit_round2.md new file mode 100644 index 0000000000..8de885e300 --- /dev/null +++ b/devlog/_plan/260819_response_state_temp_reclaim/011_audit_round2.md @@ -0,0 +1,64 @@ +# Audit round 2 — phase 1 implementation plan + +Reviewer: independent `explorer`, read-only, against `6d89332b6`. Verdict: +**GO-WITH-FIXES (blockers=6)**. Main-agent judgment: **near-pass** — all six folded, none +rebutted. The two that would have shipped real damage are 1 and 5. + +## The safety argument was wrong, and the fix is the wording + +Round 1 justified the boot floor as "a temp predating boot cannot belong to any live +pid." That is false under three environments the reviewer named: a container sharing the +config dir by volume mount (uptime is sandbox uptime), suspend-excluding `os.uptime()` +(a 3-hour lid-close shifts computed boot forward by 3 hours), and a network config dir +where `mtimeMs` comes from the server clock. + +What actually keeps this safe is the UNCONDITIONAL 15-minute grace at `state.ts:581`, +which stays ahead of the new gate. The boot floor never bypasses it. So the honest claim +is narrower: **the boot floor retires a liveness probe that has become vacuous, and the +15-minute grace remains the safety floor.** The residual exposure is a writer stalled +>15 minutes mid-write, whose worst case is a lost cache write (and on Windows an +`EACCES` that merely increments `failed`). + +60 s of skew is also the wrong order of magnitude for those three cases — they are +hours — so the constant is not what buys the safety, and the doc must stop implying it. + +## Blockers (all accepted) + +1. **CI flake, `tests/responses-state.test.ts:1522`.** The existing fixture ages files + exactly 60 minutes and keeps `live` via `isProcessAlive: pid => pid === 5252`. On a + host booted <60 min ago — the normal state of a CI runner — the boot floor bypasses + that skip and deletes `live`, so `removed` becomes 2. Fix: inject `bootTime: () => 0` + in that test (and `:1575` for symmetry). The 010 scope boundary widens to include + these tests. +2. **`state.ts:524`.** A required `bootTime` on the IO interface fails typecheck until + the default literal `responseStateTempRecoveryIO` gains it. Name it in the change map. +3. **`state.ts:582`.** Hoist `io.bootTime()` above the loop — as drafted it was one + `os.uptime()` syscall PER directory entry — and guard it: + `const bootMs = Math.min(io.bootTime(), io.now())`, skipping the floor when the value + is not finite. Replace the false comment with the accurate one above. +4. **Accept criterion 4 was vacuous.** `recoverStaleResponseStateTemps` already swallows + `list` failures at `:561` and iterator failures at `:567`, so a throwing `list` can + never reach the wrapper's new `catch` — the test would pass with the `catch` deleted. + Fix: the `try` must enclose `responseStateSweepDirectories()`, whose + `snapshotPath()`/`getConfigDir()` can genuinely throw, and criterion 4 grounds there. +5. **A unit test would touch the developer's real home.** Once `sweepLiveness` is + registered, the fake-clock test at `tests/state-store-sweeper.test.ts:132` invokes the + REAL reclaim, and that describe block sets no `OPENCODEX_HOME` — so + `bun test tests/state-store-sweeper.test.ts` would `opendir` `~/.opencodex` and could + unlink real temps as a side effect. Fix: isolate `OPENCODEX_HOME` in that block. +6. **An entry cap does not bound time.** 512 synchronous `lstat`s is 2-5 ms on APFS but + 5-10 s on an SMB/NFS config dir, blocking the event loop and stalling in-flight SSE + streams. Round 1's answer (a smaller constant) addressed the symptom, not the + mechanism. Fix: add a wall-clock deadline inside the scan loop + (`io.now() - startedAt > SCAN_DEADLINE_MS`), keeping the entry cap as a backstop. + +## Confirmed non-issues + +- Adding a required member to `ResponseStateTempRecoveryIO` does NOT break existing call + sites: `Options` is `Partial & {...}` (`:507`). No export needed for phase 1; + round 1's defect (b) is genuinely phase-2-only. +- The registration-name test (`:100`) and the fake-clock test (`:132`) need no assertion + changes, because we EXTEND the existing `responses-continuation` entry instead of + adding a store. That design choice is load-bearing. The 010 Verification section + claimed otherwise and is corrected. +- Reclaim throughput is fine: ~800 files at 64 cleanups/tick is ~13 minutes. From 48b0c2a702bd1aa7171170582af65036360231a2 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Wed, 19 Aug 2026 10:21:23 +0900 Subject: [PATCH 5/7] fix(responses): reclaim abandoned state temps on a timer, not only at load --- .../000_plan.md | 23 +++- .../002_audit_round1_late.md | 72 ++++++++++ src/lib/state-store-registrations.ts | 10 +- src/responses/state.ts | 113 +++++++++++++++- tests/responses-state.test.ts | 127 ++++++++++++++++++ tests/state-store-sweeper.test.ts | 28 +++- 6 files changed, 360 insertions(+), 13 deletions(-) create mode 100644 devlog/_plan/260819_response_state_temp_reclaim/002_audit_round1_late.md diff --git a/devlog/_plan/260819_response_state_temp_reclaim/000_plan.md b/devlog/_plan/260819_response_state_temp_reclaim/000_plan.md index 4eb73c9c19..dece4c6cc2 100644 --- a/devlog/_plan/260819_response_state_temp_reclaim/000_plan.md +++ b/devlog/_plan/260819_response_state_temp_reclaim/000_plan.md @@ -22,10 +22,25 @@ an operator a way to reclaim them when the proxy will not start at all. ## Root cause -The reclaim is attached to the request path. A proxy that crashes before serving a -continuation request leaves its temp behind and never reaches the code that would -reclaim it. The condition that produces the garbage is the same condition that -disables the collector, so the file count only ever grows. +**Corrected after the late round-1 audit (`002_audit_round1_late.md`); the original +narrative here was falsified.** It claimed a crashing proxy never reaches the reclaim. +That is wrong: a temp only exists if a snapshot write ran, and every `schedulePersist` +site (`:897`, `:929`, `:956`, `:971`, `:1214`) is downstream of `ensureLoaded`, so a +process that produced a temp had ALREADY run the reclaim. + +The reclaim runs **once per process, at load, before that process writes anything**. +Three properties then combine: + +1. **One-shot per process.** `ensureLoaded` sets `loaded = true` and never sweeps again, + so any temp a process abandons after startup is invisible to it forever. +2. **The 15-minute grace excludes the predecessor.** `:581` skips files younger than 15 + minutes, so a proxy restarting promptly after a crash cannot reclaim the temp that + crash just produced — and by (1) it never looks again. +3. **`maxCleanups = 512` caps a single pass** below the ~816 files implied by + 19.6 GB ÷ 24 MiB, so even a well-timed sweep cannot drain the backlog in one pass. + +A restart loop therefore accumulates monotonically: each process sweeps once, too early +to see its predecessor's fresh temp, then adds one of its own. This is a scheduling defect, not a missing-feature defect. Both layers below move or add a CALLER; neither loosens a reclaim safety gate. diff --git a/devlog/_plan/260819_response_state_temp_reclaim/002_audit_round1_late.md b/devlog/_plan/260819_response_state_temp_reclaim/002_audit_round1_late.md new file mode 100644 index 0000000000..5c418dc545 --- /dev/null +++ b/devlog/_plan/260819_response_state_temp_reclaim/002_audit_round1_late.md @@ -0,0 +1,72 @@ +# Audit round 1 (late) — roadmap review, VERDICT: FAIL + +The first reviewer, retired as a failed dispatch after ~11 minutes of silence, returned +afterwards against the ORIGINAL roadmap at `d75a2402f`. Its verdict is **FAIL**. Several +findings were independently fixed by round 2 in the meantime; the rest are folded here. + +**The headline finding is correct and I verified it myself.** + +## Falsified: the original root-cause narrative + +`000_plan.md` claimed a crashing proxy "never reaches the code that would reclaim." +That is wrong. A temp only exists if `atomicWriteFileAsync` ran, which requires +`writeBoundedSnapshot` ← `persistNow` ← `schedulePersist`. Every `schedulePersist` site +is downstream of a populated store: `:1214` follows `ensureLoaded()` at `:1185`; +`:956`/`:971` sit under `expandPreviousResponseInput` → `ensureLoaded`; `:897` and +`:929` are no-ops on an empty store (`:897` fires only when `removed > 0`). + +Verified directly: `grep -n 'schedulePersist()' src/responses/state.ts` returns exactly +`:897, :929, :956, :971, :1214`, and `:890-898` confirms the `removed > 0` guard. So +**a process that produced a temp had already run the reclaim.** + +## The corrected cause (three parts, all still fixed by this unit) + +The reclaim runs **once per process, at load, before that process writes anything**: + +1. **One-shot per process.** `ensureLoaded` sets `loaded = true` and never sweeps again, + so every temp a process abandons after startup is invisible to that process forever. +2. **The 15-minute grace excludes the predecessor.** `:581` skips anything younger than + 15 minutes, so a successor starting promptly after a crash cannot reclaim the temp + that crash just produced — and it never looks again (part 1). +3. **`maxCleanups = 512` caps one pass** below the ~816 files implied by 19.6 GB ÷ 24 MiB, + so even a well-timed startup sweep cannot finish the backlog in one go. + +A periodic sweep fixes all three: it repeats, so the grace expires into a later tick and +the per-pass cap becomes a per-tick rate. **The fix is unchanged; the justification is +corrected.** That distinction matters — the original story would have made the periodic +tick look optional. + +## Blockers folded + +- **B1 root cause** — restated in `000_plan.md` as the three-part cause above. +- **B5 the stack's dependency edge did not typecheck.** Phase 1 defined only + `sweepAbandonedResponseStateTemps(): number`, but phase 2 consumed + `removed`/`failed`/`bytesRemoved` from a `reclaimAbandonedResponseStateTemps()` that + phase 1 never defined. Fix: phase 1 exports a result-returning core and the sweeper + adapter narrows it to `number`. +- **B8 concurrent proxies produce false failures.** Two processes ticking over one config + dir race; the loser's `unlink` raises ENOENT and lands in `failed`, which phase 2 would + surface as "in use or locked". Fix: treat a missing path as removed, mirroring + `isMissingPathError` (`config.ts:132`). +- **B2 `matched` overstates.** It increments at `:574` BEFORE the age and PID gates, so + doctor would report live-PID temps, young temps, and directories as "abandoned". Fix + (phase 2): count eligibility after the gates. +- **B10 `formatBytes` does not exist in `src/`** — only `gui/src/format-bytes.ts`, which + needs a `Locale`. Phase 2 must name a CLI-side helper. +- **B9 sibling producers, recorded as residuals.** The same `.ocx...tmp` + template is minted by `config.ts:214`, `config.ts:453`, `catalog-writer.ts`, and + `prompt-journal.ts`. None are matched by `RESPONSE_STATE_TEMP_NAME` (`:34`) and none + are reclaimed anywhere. This unit deliberately does not widen the regex — reclaiming + another subsystem's files under a response-state name would be worse — but they are now + named as a follow-up unit rather than silently ignored. + +## Rejected + +- **B6 (unbudgeted tick)** and **B4 (vacuous criterion 4)** were already fixed by round 2 + (scan deadline; catch moved to enclose `responseStateSweepDirectories()`). +- **B3** misreads the dry-run hazard in the opposite direction from my own note; round 2 + supersedes both by replacing injected-IO trickery with an explicit `dryRun` mode. +- **"The stack is too small to justify splitting."** Rejected with reason: phase 1 is a + correctness fix that every user needs and is mergeable alone; phase 2 adds a CLI surface + plus docs and carries its own review risk. Landing the correctness fix without waiting + on CLI review is the point. diff --git a/src/lib/state-store-registrations.ts b/src/lib/state-store-registrations.ts index 55bb4f292a..61aad5f7b9 100644 --- a/src/lib/state-store-registrations.ts +++ b/src/lib/state-store-registrations.ts @@ -35,7 +35,7 @@ import { listLiveOAuthAccountKeys, reconcileOAuthReauthState } from "../oauth/st import { reconcileGuardianBackoff } from "../oauth/token-guardian"; import { sweepExpiredApiKeyCooldowns } from "../providers/key-failover"; import { reconcileProviderRequestPacing } from "../providers/request-pacing"; -import { sweepExpiredResponseStates } from "../responses/state"; +import { sweepAbandonedResponseStateTemps, sweepExpiredResponseStates } from "../responses/state"; import { sweepExpiredAntigravityReplay } from "../adapters/google-antigravity-replay"; import { reconcileProviderAccountQuotaRows } from "../providers/quota"; import { reconcileRouterWarningMemos } from "../router"; @@ -84,7 +84,13 @@ export const STATE_STORE_REGISTRATIONS = [ }, { name: "anthropic-routing-health", sweepExpired: sweepExpiredAnthropicRoutingHealth }, { name: "xai-refresh-verdicts", sweepExpired: sweepExpiredXaiPermanentFailureVerdicts }, - { name: "responses-continuation", sweepExpired: sweepExpiredResponseStates }, + { + name: "responses-continuation", + sweepExpired: sweepExpiredResponseStates, + // Disk reclaim rides the liveness tick, not the TTL tick: sweepExpiredOnWrite puts + // sweepExpired on hot write paths, where a directory scan does not belong. + sweepLiveness: sweepAbandonedResponseStateTemps, + }, { name: "antigravity-replay", sweepExpired: sweepExpiredAntigravityReplay }, { name: "config-warning-memos", reconcileGeneration: (context: GenerationContext) => reconcileConfigWarningMemos(context.generation) }, { name: "catalog-warning-memos", reconcileGeneration: (context: GenerationContext) => reconcileCatalogWarningMemos(context.generation) }, diff --git a/src/responses/state.ts b/src/responses/state.ts index a08ed1012e..a31da4c67d 100644 --- a/src/responses/state.ts +++ b/src/responses/state.ts @@ -1,4 +1,5 @@ import { chmodSync, existsSync, lstatSync, mkdirSync, opendirSync, readFileSync, rmSync, statSync, unlinkSync } from "node:fs"; +import { uptime } from "node:os"; import { dirname, join } from "node:path"; import { atomicWriteFileAsync, getConfigDir, resolveWriteTarget } from "../config"; import { enforceAppOwnedMemoryBudget, type RetainedStoreSnapshot } from "../lib/app-owned-memory"; @@ -31,6 +32,17 @@ const SNAPSHOT_FILE_MAX_BYTES = 32 * 1024 * 1024; const STALE_TEMP_GRACE_MS = 15 * 60 * 1_000; const STALE_TEMP_MAX_ENTRIES = 4_096; const STALE_TEMP_MAX_CLEANUPS = 512; +/** Absorbs `os.uptime()` granularity only. It is deliberately NOT the safety margin: + * the unconditional 15-minute grace above is (see the boot floor in the scan loop). */ +const BOOT_FLOOR_SKEW_MS = 60 * 1_000; +/** Per-tick budget for the periodic reclaim. Smaller than the startup budget because the + * periodic pass runs synchronously on the serving process's event loop every 60 s. */ +const PERIODIC_TEMP_MAX_ENTRIES = 512; +const PERIODIC_TEMP_MAX_CLEANUPS = 64; +/** Wall-clock ceiling for one periodic scan. An entry cap bounds syscalls, not time: on a + * network-mounted config dir each `lstat` can cost 10-20 ms, which would stall in-flight + * streams. Reclaim is idempotent, so a truncated tick simply resumes on the next one. */ +const PERIODIC_TEMP_SCAN_DEADLINE_MS = 25; const RESPONSE_STATE_TEMP_NAME = /^responses-state\.json\.ocx\.(\d+)\.(\d+)\.tmp$/; const MAX_SNAPSHOT_REWRITE_ATTEMPTS = 4; @@ -498,6 +510,8 @@ export interface ResponseStateTempRecoveryResult { interface ResponseStateTempRecoveryIO { now: () => number; + /** Approximate epoch ms of the current boot; see the boot floor in the scan loop. */ + bootTime: () => number; list: (dir: string) => Iterable; inspect: (path: string) => { isFile: boolean; mtimeMs: number; size: number }; isProcessAlive: (pid: number) => boolean; @@ -507,6 +521,8 @@ interface ResponseStateTempRecoveryIO { type ResponseStateTempRecoveryOptions = Partial & { maxEntries?: number; maxCleanups?: number; + /** Wall-clock ceiling for the scan, or null/undefined for no deadline (startup path). */ + deadlineMs?: number | null; }; function processIsAlive(pid: number): boolean { @@ -523,6 +539,7 @@ function processIsAlive(pid: number): boolean { const responseStateTempRecoveryIO: ResponseStateTempRecoveryIO = { now: Date.now, + bootTime: () => Date.now() - uptime() * 1_000, list: function* list(dir) { const handle = opendirSync(dir); try { @@ -549,7 +566,12 @@ export function recoverStaleResponseStateTemps( dir = getConfigDir(), options: ResponseStateTempRecoveryOptions = {}, ): ResponseStateTempRecoveryResult { - const { maxEntries = STALE_TEMP_MAX_ENTRIES, maxCleanups = STALE_TEMP_MAX_CLEANUPS, ...overrides } = options; + const { + maxEntries = STALE_TEMP_MAX_ENTRIES, + maxCleanups = STALE_TEMP_MAX_CLEANUPS, + deadlineMs = null, + ...overrides + } = options; const io = { ...responseStateTempRecoveryIO, ...overrides }; const result: ResponseStateTempRecoveryResult = { matched: 0, @@ -557,6 +579,13 @@ export function recoverStaleResponseStateTemps( failed: 0, bytesRemoved: 0, }; + const startedAt = io.now(); + // One probe per scan, not one per entry. A non-finite or future-dated boot is anomalous, and + // clamping it to "now" would be the WORST response: the floor would then retire the liveness + // probe for every file older than the skew, which is every file past the grace. Disable it + // instead -- an absent floor only costs a missed reclaim, never a wrong one. + const rawBoot = io.bootTime(); + const bootMs = Number.isFinite(rawBoot) && rawBoot <= startedAt ? rawBoot : Number.NEGATIVE_INFINITY; let names: Iterable; try { names = io.list(dir); } catch { return result; } let iterator: Iterator; @@ -569,6 +598,7 @@ export function recoverStaleResponseStateTemps( const name = next.value; scanned += 1; if (scanned > maxEntries || result.removed + result.failed >= maxCleanups) break; + if (deadlineMs !== null && io.now() - startedAt > deadlineMs) break; const match = RESPONSE_STATE_TEMP_NAME.exec(name); if (!match) continue; result.matched += 1; @@ -579,13 +609,30 @@ export function recoverStaleResponseStateTemps( let file: ReturnType; try { file = io.inspect(path); } catch { continue; } if (!file.isFile || io.now() - file.mtimeMs < STALE_TEMP_GRACE_MS) continue; - if (pid === process.pid || io.isProcessAlive(pid)) continue; + // Boot floor. After a reboot the original writer's pid is routinely reused, which makes + // the liveness skip PERMANENT: the 15-minute grace above is a lower bound and never + // expires it, so the file is skipped on every future pass forever. A temp older than + // this boot cannot be owned by the pid we would probe, so the probe is vacuous and we + // retire it. This does NOT claim the file is provably dead: under a shared-volume + // container, suspend-excluding uptime, or a network config dir the computed boot can + // land after the real one. The unconditional 15-minute grace above remains the safety + // floor, and this process's own temps are never touched. + const predatesBoot = file.mtimeMs < bootMs - BOOT_FLOOR_SKEW_MS; + if (pid === process.pid) continue; + if (!predatesBoot && io.isProcessAlive(pid)) continue; try { io.unlink(path); result.removed += 1; result.bytesRemoved += file.size; - } catch { + } catch (error) { + // Another proxy sharing this config dir may have won the race. A file that is already + // gone is reclaimed, not a failure -- reporting it as one would surface "in use or + // locked" to an operator for a file nobody holds. + if ((error as NodeJS.ErrnoException)?.code === "ENOENT") { + result.removed += 1; + continue; + } // Locked files remain for a later startup. Do not truncate by path: a same-user // replacement could turn that fallback into an arbitrary symlink-target write. result.failed += 1; @@ -594,6 +641,23 @@ export function recoverStaleResponseStateTemps( return result; } +/** + * Literal config dir plus the snapshot's resolved dir. Atomic writes place their temp beside + * the RESOLVED target, so a symlinked snapshot (dotfiles-managed config dir) strands temps in + * the link's real directory where a scan of the literal dir would never see them. The two + * collapse to one when nothing is symlinked. + */ +function responseStateSweepDirectories(): Set { + const path = snapshotPath(); + let resolvedDir = dirname(path); + try { + resolvedDir = dirname(resolveWriteTarget(path)); + } catch { + /* unresolvable link: sweep the literal dir only */ + } + return new Set([dirname(path), resolvedDir]); +} + /** * Best-effort disk snapshot so previous_response_id chains survive a proxy restart (the * dominant expansion-miss cause: an in-memory-only store dies with the process, and the next @@ -898,6 +962,49 @@ export function sweepExpiredResponseStates(at = now()): number { return removed; } +/** + * Periodic disk reclaim for abandoned atomic-write temps. + * + * `ensureLoaded` sweeps once per process, at load, BEFORE that process writes anything: + * every `schedulePersist` site is downstream of it. So a process that abandons a temp has + * already had its only look, the 15-minute grace hides the temp its predecessor's crash + * just produced, and `maxCleanups` caps a single pass below a large backlog. A restart + * loop therefore accumulates monotonically. Repeating the reclaim on a timer fixes all + * three: the grace expires into a later tick and the per-pass cap becomes a per-tick rate. + * + * Registered on the sweeper's LIVENESS tick, not the TTL tick: `sweepExpiredOnWrite` puts + * `sweepExpired` on hot write paths, and a directory scan does not belong there. + */ +export function reclaimAbandonedResponseStateTemps( + options: ResponseStateTempRecoveryOptions = {}, +): ResponseStateTempRecoveryResult { + const total: ResponseStateTempRecoveryResult = { matched: 0, removed: 0, failed: 0, bytesRemoved: 0 }; + // The try encloses responseStateSweepDirectories() deliberately: recoverStaleResponseStateTemps + // already swallows its own enumeration failures, so a catch around only that call would be + // unreachable. snapshotPath()/getConfigDir() are the paths that can genuinely throw. + try { + for (const dir of responseStateSweepDirectories()) { + const result = recoverStaleResponseStateTemps(dir, options); + total.matched += result.matched; + total.removed += result.removed; + total.failed += result.failed; + total.bytesRemoved += result.bytesRemoved; + } + } catch { + /* best-effort: disk reclaim must never destabilize the caller */ + } + return total; +} + +/** Sweeper adapter: narrows the reclaim to the `() => number` the liveness tick expects. */ +export function sweepAbandonedResponseStateTemps(): number { + return reclaimAbandonedResponseStateTemps({ + maxEntries: PERIODIC_TEMP_MAX_ENTRIES, + maxCleanups: PERIODIC_TEMP_MAX_CLEANUPS, + deadlineMs: PERIODIC_TEMP_SCAN_DEADLINE_MS, + }).removed; +} + export function responseContinuationRetainedStoreSnapshot(): RetainedStoreSnapshot { return { count: states.size, diff --git a/tests/responses-state.test.ts b/tests/responses-state.test.ts index 3084c44858..95a88fb935 100644 --- a/tests/responses-state.test.ts +++ b/tests/responses-state.test.ts @@ -36,6 +36,7 @@ import { previousResponseScopeMismatch, recoverStaleResponseStateTemps, rememberResponseState, + sweepAbandonedResponseStateTemps, responseAdmissionCountersForTests, responseStateMetrics, responseStatePersistPendingForTests, @@ -1521,6 +1522,10 @@ describe("Responses previous_response_id state", () => { const result = recoverStaleResponseStateTemps(home, { isProcessAlive: pid => pid === 5252, + // Pin the boot floor out of this case: it ages fixtures by exactly 60 minutes, so on a + // host booted more recently (a normal CI runner) the floor would retire the liveness + // probe and reclaim `live` too. The floor has its own tests below. + bootTime: () => 0, }); expect(result).toMatchObject({ matched: 5, removed: 1, failed: 0 }); @@ -1575,6 +1580,7 @@ describe("Responses previous_response_id state", () => { const result = recoverStaleResponseStateTemps(home, { isProcessAlive: () => false, unlink: () => { throw new Error("locked"); }, + bootTime: () => 0, }); expect(result).toMatchObject({ matched: 1, removed: 0, failed: 1, bytesRemoved: 0 }); @@ -1620,6 +1626,127 @@ describe("Responses previous_response_id state", () => { expect(result).toEqual({ matched: 0, removed: 0, failed: 0, bytesRemoved: 0 }); }); + test("periodic reclaim frees abandoned temps without any continuation access", () => { + // The defect this fixes: the reclaim ran only from ensureLoaded, which every + // schedulePersist site sits downstream of, so a process had its only look BEFORE it + // wrote anything. Here nothing touches the continuation store at all. + const old = new Date(Date.now() - 60 * 60 * 1_000); + const deadPid = process.pid === 4242 ? 4243 : 4242; + const stale = join(home, `responses-state.json.ocx.${deadPid}.1.tmp`); + const young = join(home, "responses-state.json.ocx.6262.4.tmp"); + for (const path of [stale, young]) writeFileSync(path, "private state"); + utimesSync(stale, old, old); + + const removed = sweepAbandonedResponseStateTemps(); + + expect(removed).toBe(1); + expect(existsSync(stale)).toBe(false); + expect(existsSync(young)).toBe(true); + }); + + test("boot floor reclaims a pre-boot temp whose pid has been reused", () => { + // Without the floor this file is immortal: the liveness probe matches a recycled pid + // and the 15-minute grace is a lower bound that never expires the skip. + const old = new Date(Date.now() - 60 * 60 * 1_000); + const path = join(home, "responses-state.json.ocx.9101.1.tmp"); + writeFileSync(path, "private state"); + utimesSync(path, old, old); + + const result = recoverStaleResponseStateTemps(home, { + isProcessAlive: () => true, + bootTime: () => Date.now() - 30 * 60 * 1_000, + }); + + expect(result).toMatchObject({ matched: 1, removed: 1, failed: 0 }); + expect(existsSync(path)).toBe(false); + }); + + test("the 15-minute grace outranks the boot floor", () => { + // A temp written after boot but younger than the grace must survive even though the + // floor would otherwise retire its liveness probe. This ordering is the safety argument. + const path = join(home, "responses-state.json.ocx.9102.1.tmp"); + writeFileSync(path, "private state"); + + const result = recoverStaleResponseStateTemps(home, { + isProcessAlive: () => true, + bootTime: () => Date.now() - 24 * 60 * 60 * 1_000, + }); + + expect(result).toMatchObject({ matched: 1, removed: 0, failed: 0 }); + expect(existsSync(path)).toBe(true); + }); + + test("this process's own temps are never reclaimed, even before boot", () => { + const old = new Date(Date.now() - 60 * 60 * 1_000); + const path = join(home, `responses-state.json.ocx.${process.pid}.1.tmp`); + writeFileSync(path, "private state"); + utimesSync(path, old, old); + + const result = recoverStaleResponseStateTemps(home, { + isProcessAlive: () => false, + bootTime: () => Date.now(), + }); + + expect(result).toMatchObject({ matched: 1, removed: 0, failed: 0 }); + expect(existsSync(path)).toBe(true); + }); + + test("a future or non-finite boot time disables the floor instead of trusting it", () => { + const old = new Date(Date.now() - 60 * 60 * 1_000); + const path = join(home, "responses-state.json.ocx.9103.1.tmp"); + writeFileSync(path, "private state"); + utimesSync(path, old, old); + + for (const bootTime of [() => Date.now() + 60 * 60 * 1_000, () => Number.NaN]) { + const result = recoverStaleResponseStateTemps(home, { isProcessAlive: () => true, bootTime }); + expect(result).toMatchObject({ matched: 1, removed: 0, failed: 0 }); + expect(existsSync(path)).toBe(true); + } + }); + + test("a temp another process already removed counts as reclaimed, not failed", () => { + // Two proxies sharing one config dir race every tick. Reporting the loser's ENOENT as a + // failure would tell an operator a file is "in use or locked" when nobody holds it. + const old = new Date(Date.now() - 60 * 60 * 1_000); + const path = join(home, "responses-state.json.ocx.9104.1.tmp"); + writeFileSync(path, "private state"); + utimesSync(path, old, old); + + const result = recoverStaleResponseStateTemps(home, { + isProcessAlive: () => false, + bootTime: () => 0, + unlink: () => { + const error = new Error("gone") as NodeJS.ErrnoException; + error.code = "ENOENT"; + throw error; + }, + }); + + expect(result).toMatchObject({ matched: 1, removed: 1, failed: 0 }); + }); + + test("the periodic scan stops at its wall-clock deadline", () => { + const old = new Date(Date.now() - 60 * 60 * 1_000); + const names = ["responses-state.json.ocx.9201.1.tmp", "responses-state.json.ocx.9202.2.tmp"]; + for (const name of names) { + const path = join(home, name); + writeFileSync(path, "private state"); + utimesSync(path, old, old); + } + // Clock jumps past the deadline on the first in-loop read. + let ticks = 0; + const result = recoverStaleResponseStateTemps(home, { + list: () => names, + isProcessAlive: () => false, + bootTime: () => 0, + now: () => (ticks++ === 0 ? 0 : 10_000), + deadlineMs: 25, + }); + + expect(result.removed).toBe(0); + for (const name of names) expect(existsSync(join(home, name))).toBe(true); + }); + test("v1 Cursor snapshot migrates to versioned provider state", () => { mkdirSync(home, { recursive: true }); writeFileSync(join(home, "responses-state.json"), JSON.stringify({ diff --git a/tests/state-store-sweeper.test.ts b/tests/state-store-sweeper.test.ts index 8b9ac2f10d..ea2d473b27 100644 --- a/tests/state-store-sweeper.test.ts +++ b/tests/state-store-sweeper.test.ts @@ -64,7 +64,17 @@ function context( }; } +// The responses-continuation store now reclaims abandoned atomic-write temps on the liveness +// tick, so any test that drives a real tick performs filesystem work under OPENCODEX_HOME. +// Without this isolation the suite would scan (and could unlink inside) a developer's real +// ~/.opencodex as a side effect of a unit test. +let sweeperHome: string; +let previousSweeperHome: string | undefined; + beforeEach(() => { + previousSweeperHome = process.env.OPENCODEX_HOME; + sweeperHome = mkdtempSync(join(tmpdir(), "ocx-sweeper-home-")); + process.env.OPENCODEX_HOME = sweeperHome; resetStateStoreSweeperForTests(); resetAppOwnedMemoryForTests(); clearResponseStateMemoryForTests(); @@ -77,6 +87,9 @@ afterEach(() => { __resetAntigravityReplayCache(); setOcxStartProcessCacheForTests([]); setOcxStartProcessProbeForTests(null); + if (previousSweeperHome === undefined) delete process.env.OPENCODEX_HOME; + else process.env.OPENCODEX_HOME = previousSweeperHome; + rmSync(sweeperHome, { recursive: true, force: true }); }); describe("state-store sweeper", () => { @@ -151,10 +164,17 @@ describe("state-store sweeper", () => { sweepExpired(123); sweepLiveness(); - expect(visits).toEqual(STATE_STORE_REGISTRATIONS.flatMap(registration => [ - ...(registration.sweepExpired ? [`${registration.name}:ttl:123`] : []), - ...(registration.sweepLiveness ? [`${registration.name}:liveness`] : []), - ])); + // Two separate passes over the table, not one interleaved pass: sweepExpired visits every + // TTL owner, then sweepLiveness visits every liveness owner. The previous per-registration + // flatMap only matched because the single liveness owner happened to sit last in the table. + expect(visits).toEqual([ + ...STATE_STORE_REGISTRATIONS.flatMap(registration => ( + registration.sweepExpired ? [`${registration.name}:ttl:123`] : [] + )), + ...STATE_STORE_REGISTRATIONS.flatMap(registration => ( + registration.sweepLiveness ? [`${registration.name}:liveness`] : [] + )), + ]); }); test("expiry boundary removes expired rows and preserves live rows", () => { From 816024c954fae882fec4133bbcc70812c6e54382 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Wed, 19 Aug 2026 11:06:39 +0900 Subject: [PATCH 6/7] docs(devlog): record phase 1 verification evidence --- .../012_phase1_verification.md | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) create mode 100644 devlog/_plan/260819_response_state_temp_reclaim/012_phase1_verification.md diff --git a/devlog/_plan/260819_response_state_temp_reclaim/012_phase1_verification.md b/devlog/_plan/260819_response_state_temp_reclaim/012_phase1_verification.md new file mode 100644 index 0000000000..c7ec1d8383 --- /dev/null +++ b/devlog/_plan/260819_response_state_temp_reclaim/012_phase1_verification.md @@ -0,0 +1,47 @@ +# Phase 1 verification + +Full suite executed on `macmini-cf` (CPU-heavy work does not run on the workstation), in a +dedicated worktree at `/tmp/ocx-reclaim` checked out to `48b0c2a70`. + +## Focused suites — green + +`bun test tests/responses-state.test.ts tests/state-store-sweeper.test.ts` +→ **125 pass, 0 fail**, 349 assertions. + +## Full suite — 13296 pass, 8 fail, all pre-existing + +`bun run test` → `Ran 13316 tests across 850 files [478.53s]`, 13296 pass / 8 fail. + +The 8 failures are two environmental classes, neither touched by this change: + +1. **`update-npm-cache-preflight` (1).** `runNpmCachePreflight` returns + `npm_config_failed` instead of `cache_accessible`. Proven pre-existing by checking the + worktree out to the UNMODIFIED base `59964ad77` and re-running that file: **10 pass, + 1 fail** — identical. It depends on a working `npm config` on the host. +2. **GUI module loads (7).** `Cannot find package 'react'` / + `Cannot find module 'react/jsx-dev-runtime'` from `gui/src/...`. That box has no + `gui/node_modules`; only the root workspace was installed. + +## Local checks + +- `bun run typecheck` (`bun x tsc --noEmit`) — clean. +- `bun test tests/repo-hygiene.test.ts` — 11 pass. +- `bun run privacy:scan` — passed. +- `bun test tests/core-lab-boundary.test.ts` — 13 pass (registration touches a + Lab-protected import path, so this was re-verified rather than assumed). + +## Defect found by the new tests + +The first draft clamped an anomalous boot time with `Math.min(rawBoot, now)`. The +future/non-finite test failed immediately: clamping to "now" makes the floor MAXIMALLY +aggressive — every file past the 15-minute grace would have its liveness probe retired. +Corrected to disable the floor outright when the value is not finite or is in the future. +An absent floor costs a missed reclaim; a wrong floor costs a live file. + +## Correction to audit round 2 + +Round 2 predicted the "global fake-clock sweep" assertion in +`tests/state-store-sweeper.test.ts` would NOT change. It did. Its per-registration +`flatMap` interleaved `:ttl` and `:liveness` per store, which only matched observed order +while the single liveness owner sat last in the table. `sweepExpired()` and +`sweepLiveness()` are two separate passes, so the expectation is now built as two passes. From 1fbac66f813cb83accf87fa03d86b96a18bdecc7 Mon Sep 17 00:00:00 2001 From: jun Date: Wed, 19 Aug 2026 18:57:39 +0900 Subject: [PATCH 7/7] fix(responses): close the directory iterator when a reclaim scan truncates An early break abandons the enumeration generator instead of resuming it, so the finally that closes the directory handle never runs. The periodic reclaim truncates by design -- entry cap, cleanup cap, wall-clock deadline -- which turned that into one leaked handle per truncated tick. Route every early exit through a stopScan() helper that calls iterator.return() before returning, and add a regression that fails when the fix is reverted. Also repairs the deadline test's oracle. Its fake clock started at 0 while the fixtures carried real epoch mtimes, making every computed age negative, so the files survived the 15-minute grace whether or not a deadline check existed -- the test passed against its own ablation. Anchor the clock to real time and add an explicit unbounded-run assertion so the deadline is the only reason nothing is removed. --- src/responses/state.ts | 14 ++++++++-- tests/responses-state.test.ts | 52 +++++++++++++++++++++++++++++++++-- 2 files changed, 62 insertions(+), 4 deletions(-) diff --git a/src/responses/state.ts b/src/responses/state.ts index a31da4c67d..d269c03f4b 100644 --- a/src/responses/state.ts +++ b/src/responses/state.ts @@ -591,14 +591,24 @@ export function recoverStaleResponseStateTemps( let iterator: Iterator; try { iterator = names[Symbol.iterator](); } catch { return result; } let scanned = 0; + // Every early exit runs through this. The production `list` is a generator that closes its + // directory handle in a `finally`, and a `finally` does NOT run when the consumer simply + // stops calling `next()` -- only `return()` resumes the generator to completion. Breaking + // out of the loop directly therefore leaked one directory handle per truncated scan, and the + // periodic reclaim truncates on purpose (entry cap, cleanup cap, deadline), so on a slow + // filesystem that is a leak per tick, forever. + const stopScan = (): ResponseStateTempRecoveryResult => { + try { iterator.return?.(); } catch { /* closing is best-effort; never fail a reclaim on it */ } + return result; + }; for (;;) { let next: IteratorResult; try { next = iterator.next(); } catch { return result; } if (next.done) break; const name = next.value; scanned += 1; - if (scanned > maxEntries || result.removed + result.failed >= maxCleanups) break; - if (deadlineMs !== null && io.now() - startedAt > deadlineMs) break; + if (scanned > maxEntries || result.removed + result.failed >= maxCleanups) return stopScan(); + if (deadlineMs !== null && io.now() - startedAt > deadlineMs) return stopScan(); const match = RESPONSE_STATE_TEMP_NAME.exec(name); if (!match) continue; result.matched += 1; diff --git a/tests/responses-state.test.ts b/tests/responses-state.test.ts index 95a88fb935..3b530ab088 100644 --- a/tests/responses-state.test.ts +++ b/tests/responses-state.test.ts @@ -1733,18 +1733,66 @@ describe("Responses previous_response_id state", () => { writeFileSync(path, "private state"); utimesSync(path, old, old); } - // Clock jumps past the deadline on the first in-loop read. + // The fake clock must stay ANCHORED to real time, or this test proves nothing: an + // `io.now()` of 10_000 against real epoch mtimes makes every age negative, so the files + // survive the 15-minute grace whether or not a deadline check exists. Anchoring instead + // means the only reason a file survives is the deadline itself. + const base = Date.now(); let ticks = 0; const result = recoverStaleResponseStateTemps(home, { list: () => names, isProcessAlive: () => false, bootTime: () => 0, - now: () => (ticks++ === 0 ? 0 : 10_000), + // First read is startedAt; every later read is past the 25 ms budget. + now: () => (ticks++ === 0 ? base : base + 10_000), deadlineMs: 25, }); expect(result.removed).toBe(0); for (const name of names) expect(existsSync(join(home, name))).toBe(true); + + // Ablation guard: the SAME inputs without a deadline must remove both files. If this + // half ever fails, the assertions above stopped depending on the deadline. + ticks = 0; + const unbounded = recoverStaleResponseStateTemps(home, { + list: () => names, + isProcessAlive: () => false, + bootTime: () => 0, + now: () => (ticks++ === 0 ? base : base + 10_000), + }); + expect(unbounded.removed).toBe(2); + }); + + test("a truncated scan closes the directory iterator", () => { + const old = new Date(Date.now() - 60 * 60 * 1_000); + const names = ["responses-state.json.ocx.9301.1.tmp", "responses-state.json.ocx.9302.2.tmp"]; + for (const name of names) { + const path = join(home, name); + writeFileSync(path, "private state"); + utimesSync(path, old, old); + } + + // Production enumerates with a generator that closes its directory handle in a finally. + // A finally only runs if the consumer calls return() -- abandoning the iterator leaks the + // handle, once per truncated scan, and the periodic reclaim truncates by design. + let closed = false; + const list = function* list(): Generator { + try { + for (const name of names) yield name; + } finally { + closed = true; + } + }; + + const result = recoverStaleResponseStateTemps(home, { + list, + isProcessAlive: () => false, + bootTime: () => 0, + maxEntries: 1, + }); + + expect(result.removed).toBe(1); + expect(closed).toBe(true); }); test("v1 Cursor snapshot migrates to versioned provider state", () => {