fix(responses): reclaim abandoned state temps on a timer, not only at load

devlog/_plan/260819_response_state_temp_reclaim/000_plan.md

@@ -0,0 +1,105 @@
+# 260819 — response-state temp reclaim
+
+## Objective
+
+Abandoned `responses-state.json.ocx.<pid>.<seq>.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
+
+**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.
+
+**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
+
+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 + 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
+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.
+- 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.

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.

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.<pid>.<seq>.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.