fix(persist): retry dirty content stores on graceful flush - #1534

🤖 Powered by ework · qwen3.8-27b

[bot] 🏷 Reviewing this PR now against #1533. Branch 2026-09-28_flush-content-store @ cccc5bb is based on current master (86e0b68) — good. Will report findings shortly.

🤖 Powered by ework · qwen3.8-27b

[bot] 🏷 Review complete — verdict: mergeable. ✅

① Issue direction (#1533) — reproduced against code, bug is real and the fix hits the root cause. Every save path piggybacks the content-store write (scheduleSave src/persist.ts:624-627, writeNow :632-635, flushSync :642-645 all call saveContentStore first) — only flushAll didn't, despite receiving the resident session list. A transient write failure restores contentStoreDirty = true by design (src/persist.ts:333), and with no further mutation before SIGTERM/SIGINT the dirty store was never retried → latest CCR entries lost on exit. Fixing the gap at the flush decision point is the source-level fix, not a mask.

② Regression review — existing behaviors on touched paths:

1. Kernel drain (await this.store.flushAll()): preserved verbatim, still runs after the new loop.
2. Payload-first invariant (Feature: content-addressed message store — ID-referenced tool results + acp_retrieve (built-in CCR) #1097) — content stores must land before session-state writes so a mid-save crash leaves at worst a retrieve miss: preserved and extended to shutdown. The loop runs before the kernel drain, which is the correct order (reversed order could leave persisted state referencing refs missing from disk).
3. Call-shape compat: old _sessions was a required param; all existing callers pass an iterable (flushAllSessions src/session.ts:771 passes sessions.values() — used by server.ts:595/645/652 and restart.ts:266; tests pass [] / [a, b]). The = [] default only adds a new zero-arg form; existing call sites behave identically.
4. Clean-shutdown no-op: saveContentStore early-returns unless contentStoreDirty (src/persist.ts:320), so for any session saved normally the new loop is a zero-cost no-op. Edge case dirty+empty store (rebase reset) retries the rmSync delete — consistent with the same function's semantics on every other path.
5. Iteration safety: Map.values() iterators tolerate concurrent add/delete; saveContentStore doesn't mutate the session map anyway.

Behavior change (intended, flagged per policy): old — a content store left dirty by a transient failure stayed unwritten through graceful shutdown (data loss); new — one synchronous retry write per dirty session during flushAll, then normal exit even if it fails again (warn logged, same degradation as any failed shutdown write). This is the fix itself, no incidental drift.

③ Code — minimal and correct; saveContentStore is private, called in-class; doc comment updated accurately.

④ Test quality — real teeth. Beyond your verification claims, I independently ran the mutation check on PR head: restoring the old flushAll makes exactly the new test fail (22/23 pass, 1 fail); with the fix all 23 pass. Local runs: tsc --noEmit tsconfig.build.json clean; store + side-request-isolation + proxy-persist + persist-stats-roundtrip + persist-rollback-guard = 67/67 pass. The test is deterministic (EISDIR trap via file→directory swap forces writeFileSync failure — no timing/port luck), asserts the full chain (dirty restored → warn logged → flush recovers → envelope exists → m00005 ref round-trips, proving the retry wrote the latest store not a stale one), and cleans up in finally.

⑤ Diff cleanliness — 2 files, every line purpose-related, no whitespace/lockfile churn. Base is current master (86e0b68), mergeable_state: clean, no open PR rewriting src/persist.ts.

§7.4 auto-merge gate: all 8 criteria met (single-module fix, regression test reproduces+passes, green on rebased head, no config/persistence-format/wire/cross-repo changes, pure fix:, clean diff, Fixes #1533, no load-bearing infra). Merge is yours to click: #1534

Deep review complete — mergeable. Verified independently:

• Semantics: saveContentStore short-circuits on clean/empty stores (!enabled || !contentStoreDirty → no-op), and its failure path catches internally (restores the dirty flag, logs) — so the new retry loop can never abort the kernel store.flushAll() that follows. Ordering (content stores before kernel drain) is right.
• Caller: flushAllSessions() (session.ts:770) passes the resident sessions.values() map; all three shutdown sites (server.ts:595/645/652, restart.ts:266) go through it — the previously-ignored parameter is now fed everywhere it exists.
• Tests: 23/23 store suite; mutation check (revert the retry loop) → the new test goes red (1/23). The 2 ccr-plugin failures seen locally reproduce identically on current master with the same shared node_modules (stale acp-kernel 0.0.93 vs the pinned 0.0.97) — environment drift, not this PR; CI with the correct pin is authoritative.
• The repro test follows the issue's steps exactly (EISDIR as the transient fault, dirty-flag persistence asserted, envelope restored once the fault clears, reload reads back the pre-failure ref).

Nice catch, and thanks for the second contribution in a row.

🤖 Powered by ework · qwen3.8-27b

[bot] 🏷 Thanks for the deep-review pass. One independent datapoint on your environment-drift call: I confirmed master and PR head pin acp-kernel at exactly 0.0.97, and with a clean npm ci from PR head (installed kernel verified = 0.0.97) tests/ccr-plugin.test.ts runs 10/10 pass here — so those 2 local failures track a stale shared node_modules (0.0.93), not this PR's diff. That also means my earlier 67/67 targeted run (store / side-request-isolation / proxy-persist / persist-stats-roundtrip / persist-rollback-guard) was executed against the correct kernel pin.

Nothing further from my side — review record stands: mergeable, §7.4 gate satisfied, merge is yours.