fix(persist): spill to side file on rename failure + per-entry compress invalid reasons

[ranxianglei]

Addresses billion-context#362 (Windows persist EPERM storm + compress no-valid-ranges).

1. Persist: stop silently dropping session data on rename failure (main)

Root cause. On Windows, file locks (AV real-time scan / indexers) are held long enough that the ~120ms rename retry window always exhausted. Each failed persist then unlinked the temp file — permanently losing that round of session state. In the field: 4,879 EPERM lines, 117 sessions, and the top-3 failing sessions (2062/271/214 failures) had no file on disk at all — never persisted. All silent except the error log lines.

Fix (src/persist/store.ts):

• Exponential-backoff retry — default 6 attempts, 50ms base, 1600ms cap (was 3 attempts, 20/40/60ms). Configurable via retryAttempts/retryBaseMs/retryMaxMs.
• Spill on final failure — instead of unlinking the temp file, write the envelope to <name>.fb.json (one slot per id, overwritten) so the data is on disk. loadAll discovers spills and reconciles canonical vs spill by savedAt (freshest wins); a successful canonical write removes the now-stale spill of the same id.
• Rate-limited alerting — 1x error, then warn at powers of two (2x, 4x, 8x…), instead of one error line per failed write.

The store still never deletes a record's data; it only removes a temp file it itself created or a stale spill superseded by a newer canonical write.

2. Compress: report per-entry invalid reasons (secondary)

Root cause. When a compress call is rejected with no-valid-ranges, hosts only knew invalidItems=N, not why each entry was invalid — so the model blind-retried the same payload (5x in the field).

Fix (src/parse-compress-input.ts): add diagnostics.invalidReasons — one index-prefixed, human-readable reason per dropped entry (missing range bounds / missing summary / not an object). Hosts can surface the specific reason to the model so it self-corrects.

Pre-flight

• npm run typecheck — clean
• npm test — 548 pass, 0 fail (+8 new: 6 persist spill/retry/alert, 2 compress reasons)
• npm run build — success

Note for billion-context

This ships as a kernel release; billion-context must bump acp-kernel to the new version (after it is live on npm) to pick up the persist fix, and can then use invalidReasons in its compress error message. The spill files end in .json so existing loadAll discovery and the billion-context relPathFor/flatFileNameFor reconciliation both pick them up with no proxy-side change.

Requires review by ≥2 agents per acp-kernel AGENTS.md.

[github-actions]

📦 Built Package Artifact

Branch: 2026-08-31_persist-spill-compress-reasons (f2a4b78)

Option A — Install from npm PR tag (recommended)

In your adapter project:

npm install acp-kernel@pr-172

Each push to this PR publishes a new version under the pr-172 npm tag.

Option B — Download artifact

1. Download the artifact from the Actions run
2. Extract the tarball and install:

tar xzf acp-kernel-pr172.tgz
npm install ./package

---

This comment is automatically updated on each push.

[ranxianglei]

🤖 ework agent · vllm-qwen/qwen3.8-27b

[bot] Acknowledged — reviewing PR #172 now. I'll check out 2026-08-31_persist-spill-compress-reasons, run the pre-flight checks (typecheck / test / build), and do a full code review of both changes (persist spill/retry/alert + compress invalidReasons). Will report findings here.

[ranxianglei]

🤖 ework agent · vllm-qwen/qwen3.8-27b

[bot] Review #1 (ework-daemon) — full review of 2026-08-31_persist-spill-compress-reasons (f2a4b78). A second independent agent review is in flight and will follow as a separate comment.

Pre-flight (re-ran everything myself)

• npm run typecheck — clean
• npm test — 548 pass / 0 fail on branch; 540 on master → the "+8 new" claim is verified
• npm run build — success
• scripts/ci/check-pr.sh gates: branch name OK, package.json version untouched ✓
• Note: npm run format:check fails on 89 files, but identically on master (repo-wide prettier drift, pre-existing). CI only gates typecheck/test/build, so this doesn't block.

Findings

1. [MAJOR — design inconsistency, no data loss] Sync vs async spill semantics diverge

• flushSync returns spillPath !== null (src/persist/store.ts:259) → true when the canonical rename failed but the spill succeeded (data safe on disk).
• writeInner still throw es unconditionally after the spill attempt (src/persist/store.ts:403) → writeNow rejects even when the data was safely spilled.

Consequences:

• flushAll logs [persist] shutdown flush failed for <id> (src/persist/store.ts:349) even when the data is safe in the spill — misleading at exactly the moment (shutdown) operators look at logs.
• Downstream hosts that treat a writeNow rejection as "data may be lost" would be wrong post-PR.

Suggestion: reject only when both canonical and spill failed (or at minimum, don't error-log in flushAll when the spill succeeded). This is the only substantive point; everything below is minor.

2. [MINOR] flushSync's doc comment (src/persist/store.ts:191-194) still says "Returns true on success, false on failure" — ambiguous now that canonical-failure + spill-success returns true.

3. [MINOR] The top-level single-range path (src/parse-compress-input.ts:120-130) still reports missing-content for an invalid single object and does not populate invalidReasons. The exact reason this PR exists would help the model self-correct in that shape too.

4. [MINOR — test gap] No dedicated test for "canonical fresher than spill → loadAll picks the canonical" (the existing test only checks stale-spill cleanup after a successful write, by which point the spill is already deleted). The >= reconciliation is symmetric so it's implicitly covered, but a direct test would pin it.

Verified correct (spot-checked, no issues)

• Backoff math: 50+100+200+400+800 = 1550ms over attempts 0–4, no sleep after the last attempt — matches the documented "~1.5s window".
• loadAll owner resolution checks the canonical name before the .fb.json strip, so a legitimate canonical file named foo.fb.json is handled correctly; owner is always taken from the envelope's own id (no cross-id confusion); discovered is set only for the reconciliation winner.
• The field case (session never persisted, only a spill on disk) is discovered by loadAll ✓; canonical-success while the spill is locked → removeSpill fails silently, next loadAll reconciles (fresher canonical wins) ✓.
• recordFailure alerts at 1 (error), then 2/4/8… (warn); counter resets on success via clearFailure.
• spillPathFor preserves namespaced dirnames (a.json → a.fb.json, multi-dot names safe); constructor clamps all three retry options (retryAttempts≥1, retryBaseMs≥1, retryMaxMs≥retryBaseMs).
• parse-compress-input: EntryOutcome is a proper discriminated union (no as any); invalidReasons is optional and set only when non-empty (backward-compatible); the diagnostics type was already barrel-exported (src/index.ts:107).

Observations (pre-existing, out of scope)

• Promise.withResolvers requires Node 22+, but engines says >=20 — already on master (src/persist/store.ts:466 pre-PR); CI matrix is Node 22/24 so CI is safe.
• flushSync can block the main thread up to ~1.55s via Atomics.wait under sustained EPERM — deliberate and documented in the option docstring.

Verdict

approve-with-nits — no data-loss path found in either the persist or parse changes; finding #1 is worth a small follow-up (or an explicit doc note) before merge.

[ranxianglei]

🤖 ework agent · vllm-qwen/qwen3.8-27b

[bot] Review #2 (independent agent) — second review complete; this satisfies the ≥2-agent requirement from AGENTS.md. Reviewer also independently re-verified: typecheck clean, 548/548 tests pass.

Agrees with Review #1 on the flushSync/writeNow spill-semantics asymmetry. New findings:

1. [MINOR] loadSync in a fresh process cannot see a spilled record — src/persist/store.ts:269-282: candidates are discovered / hint / flat canonical name only; the flat spill name (<hash>.fb.json) is never probed. After a restart (exactly the persistent-AV-lock scenario this PR targets), a consumer that calls loadSync(id) without loadAll gets the stale canonical (the locked file still exists) and believes it has current state. Data is not lost (it's on disk, loadAll recovers it), but the "flat default name is always checked as a fallback" contract is now incomplete. Suggested one-line fix: add path.join(this.dir, flatFileNameFor(id).replace(/\.json$/, ".fb.json")) to the candidates. (I verified this against the code — accurate.)

2. [MINOR] Spill write is not atomic — src/persist/store.ts:252 (sync) / :396 (async): the spill goes straight through writeFileSync, no temp+rename. A crash mid-spill leaves a truncated .fb.json that loadAll skips as corrupt → freshest state lost (older canonical remains), and the truncated spill emits a "skipping corrupt file" warn on every subsequent boot. Given this PR's crash-safety goal, the spill path deserves the same temp+rename treatment as the canonical path.

3. [MINOR] Test gaps (core paths are well covered — spill-on-failure, both reconciliation directions, alert counts, counter reset, sync spill — but):

• The only true data-loss path is untested: canonical AND spill both fail → flushSync returns false + SPILLOVER ALSO FAILED — data at risk log (src/persist/store.ts:255-257, :475).
• Non-retryable code (e.g. ENOSPC) skips retries (exactly 1 attempt) yet still spills — untested.
• Namespaced (subdirectory) spill — spillPathFor + the .fb owner branch in loadAll only exercised with flat names.
• Backoff math / attempt counting.
• invalidReasons in salvage mode (src/parse-compress-input.ts:193-199).

4. [NIT] savedAt tie-break is readdir-order-dependent — src/persist/store.ts:316 uses >=, so same-millisecond ties resolve by nondeterministic readdir order. A deterministic tie-break (prefer canonical over spill) would be cleaner.

5. [NIT] Sync/async retry asymmetry — the sync loop retries writeFileSync too (src/persist/store.ts:219-224); the async path retries only the rename (src/persist/store.ts:381-382). Conservative (data preserved), just inconsistent.

6. [NIT] removeSpill is unconditional (src/persist/store.ts:242, :385) — a clock rewind (NTP step) or cross-process sharing could make a just-written canonical older than an existing spill, and removing it loses the newer state. Impossible in-process (per-id serialization); a savedAt comparison before unlink is free insurance.

7. [NIT] Formatting regression — tests/persist.test.ts:472-475: the "loadSync hint" test body is de-indented 4 spaces relative to its try block. Compiles fine; note format:check fails repo-wide (89 files incl. untouched tsconfig.json) so that's pre-existing, but the new indentation is still sloppy.

8. [NIT] Promise.withResolvers (src/persist/store.ts:497) needs Node ≥22 (backported to 20.13) while engines says >=20 — pre-existing on master, not introduced here.

Verified OK (both reviewers concur)

Backoff math (1550ms window, no sleep after last attempt, constructor clamps); error-code filtering on both paths (EPERM/EBUSY/EACCES only, non-Error throws treated as non-retryable); loadAll owner resolution (owner is always the envelope's own id → no cross-id confusion, works for namespaced subdirs); reconciliation + discovered consistency; rate-limited alerting (exactly 1,2,4,8…; reset on success); temp-file handling and canonical atomicity preserved; parse-compress-input type safety (proper discriminated union, no as any/@ts-ignore anywhere in src/); invalidReasons backward compatible (optional, set only when non-empty); zero runtime deps; no version bump; and the original silent data-loss path (unlink-on-failure) no longer exists.

Combined verdict: approve-with-nits (2/2)

Suggested pre-merge items (all small, none blocking):

1. Unify or document the flushSync-true vs writeNow-reject spill semantics (R1#1 + R2#2) — incl. the stale flushSync doc comment (R1#2).
2. Add the flat spill name to loadSync candidates (R2#1) — one line.
3. Optionally: atomic (temp+rename) spill write + a test for the both-fail data-at-risk path (R2#2, R2#3).
4. invalidReasons for the single top-level range shape (R1#3).

I will not merge — per AGENTS.md, PR merge is human-only: #172