fix(session): reconcile restart policy authority - #2777
zaxbysauce wants to merge 2 commits into
Conversation
Drift check reportFound 2 drift finding(s): 0 error, 0 warning, 2 notice. required-check-contract (2)
|
There was a problem hiding this comment.
🔵 Needs a closer look
It makes deep, interdependent changes to plan-durability (invariant 5) and session-state (invariant 8) recovery paths across many files whose correctness depends on subtle ordering and authority-fencing invariants that warrant final human verification.
Pull request overview
This PR (Closes #2668) hardens restart/hydration reconciliation so that a delayed or superseded operation can never publish stale durable state over newer accepted state. It introduces a process-monotonic "authority epoch" paired with the existing per-project hydration generation to close an ABA window (numeric generations are reusable after FIFO eviction/reset), and threads a synchronous authority predicate (preCommitCheck) into every recovery-path publication boundary, plus a typed PlanRecoverySupersededError that broad recovery catches must rethrow. It also makes the post-resolution coordinator rebuild missing/corrupt projections authoritatively from the ledger via loadPlan.
Changes:
- Adds
hydrationAuthorityEpochto hydration ownership + session state, fencing caches, aggregate ownership, subscriptions, and rehydration at their async commit boundaries. - Makes plan recovery (
loadPlan/savePlan/rebuildPlan/ledger init/replace/quarantine/marker/spec-staleness) fail-closed on supersession viapreCommitCheck+commitAsyncPreparedFile, and switches the projection writer to a synchronous rename with an adjacent authority check. - Adds a coordination
supersededreadiness state, extensive#2668unit/journey tests, a J08 registered-host journey, and documentation/release fragments.
File summaries
| File | Description |
|---|---|
| src/session/hydration-ownership.ts | Replaces generation counters with bounded authority records (generation + monotonic epoch); adds capture/current predicates and epoch-scoped caches/aggregate keys. |
| src/state.ts | Stamps hydrationAuthorityEpoch, returns a commit result from buildRehydrationCache, fences PR-subscription rehydration, adds _internals/re-export seams. |
| src/session/snapshot-reader.ts | Authority-epoch-fenced eviction/protection and cache/rehydrate commit gating in rehydrateState/loadSnapshot. |
| src/session/snapshot-writer.ts | Sync rename + shouldCommit predicate; restructured atomic write try/finally; adds hydrationAuthorityEpoch transient-field doc. |
| src/session/snapshot-coordination-init.ts | Adds superseded outcome, authoritative loadPlan recovery before cache/projection, and per-boundary isCurrent() fences. |
| src/plan/manager.ts | Adds PlanRecoverySupersededError, commitAsyncPreparedFile, preCommitCheck threading, and rebuild marker-cleanup supersession handling. |
| src/plan/ledger.ts | Threads preCommitCheck through init/append/snapshot/replace/replay and moves quarantine write outside its broad catch. |
| src/observability/catalog.ts | Updates producer line citations to match moved emit sites. |
| scripts/retention-registry.data.ts | Updates loadPlan/writeSnapshot reader/writer line citations. |
| scripts/registry-citation-baseline.json | Removes the now-resolved loadPlan out-of-range debt entry. |
| docs/* | Documents restart authority boundaries, recovery runbook, and J08 journey; adds/updates release fragments. |
| tests/unit/** (session/plan/execute-journey/commands) | Adds ABA, supersession, recovery-replay/quarantine, marker, subscription-fence, and J08 journey coverage; updates existing coordination/parity tests. |
Review details
- Files reviewed: 31/31 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Swarm PR Review — #2777 (issue #2668, restart policy authority)Reviewed at head
|
🤖 Multi-Stage PR ReviewPipeline: MiniMax-M2.7-highspeed (orientation) (context pack) → MiniMax-M2.7-highspeed (explorer) (explore → candidates) → MiniMax-M2.7-highspeed (fallback arbiter) (challenge + blind-spot) PR Reviewer — opencode-swarm🔍 PR IntentReconstructed from PR description, linked issue #2668, and diff:
📦 Implementation SummaryThe PR adds an exact authority token ( ✅ /
|
| Obligation | Status | Evidence |
|---|---|---|
| O-001 (authority fence in recovery paths) | SUPPORTED |
src/plan/manager.ts:2292–2319 — commitAsyncPreparedFile calls preCommitCheck before renameSync; every ledger/snapshot call site propagated |
| O-002 (authority epoch) | SUPPORTED |
src/session/hydration-ownership.ts:66–69 — hydrationAuthorityEpoch is process-monotonic and never reset |
| O-003 (PlanRecoverySupersededError propagation) | SUPPORTED |
src/plan/manager.ts:27–32 — class defined; throw error / if (e instanceof PlanRecoverySupersededError) throw e appears at 19 call sites |
| O-004 (durable vs ephemeral authority) | SUPPORTED |
docs/plan-durability.md:466–496 — explicit table distinguishing durable from ephemeral; j08 fixture asserts it |
| O-005 (new tests) | SUPPORTED |
7 new test files; tests/unit/execute-journey/j08-restart-policy-reconciliation.test.ts and 6 unit test files |
🚨 Confirmed Findings
[HIGH] renameWithTransientRetry silently drops false when _internals.rename is void
- Location:
src/session/snapshot-writer.ts:91–98 - Why it matters: The
renameWithTransientRetryfunction is typedPromise<boolean>and returnstrueon success /falseon fence-reject. However,_internals.renameis typed as(from, to, shouldCommit?) => void | Promise<void>. When the DI adapter is the productionrenameSync(returnsundefined/void), the async loop's inner callawait _internals.rename(tempPath, targetPath, shouldCommit)resolves toawait undefined— a no-op — and control falls through to the next iteration attempt. The retry loop will callshouldCommit()again, but if the fence has flippedfalseon a retry after the loop already entered withtrue, the false is never surfaced andrenameWithTransientRetryincorrectly returnstrue. More critically, when_internals.renameis a void-returning mock (as insnapshot-writer-rename-retry.test.tsline 224),await undefinedmakes the for-loop iterate allSNAPSHOT_RENAME_MAX_ATTEMPTStimes without ever returningtrue. - Evidence:
snapshot-writer.ts:84— thetrybody assignsawait _internals.rename(tempPath, targetPath, shouldCommit)where the return type isvoid | Promise<void>;src/session/snapshot-writer.ts:96—return trueis reached only if_internals.renameresolves; if_internals.renameis void-sync, the try body resolves immediately toundefinedand the loop continues. - Fix direction: Wrap the inner call to detect void-sync adapters: if
_internals.rename(tempPath, targetPath, shouldCommit)returns a non-undefined value, await it; if it returnsundefined(void-sync), checkshouldCommit?.()synchronously before the synchronous rename and returnfalseif rejected. Or guard the return path: after the try body, explicitly checkif (!renamed) return falseby storing the call's return value (or checkingshouldCommit?.()inline for void-sync paths).
🔬 Unverified but Plausible Risks
None — all plausible concerns are structurally covered by the preCommitCheck fence propagation or the typed PlanRecoverySupersededError catch chain.
🧪 Test / Coverage Gaps
- Gap:
snapshot-writer-rename-retry.test.tsadds a test forwriteSnapshotProjectionwith a void-sync_internals.renameadapter, but the testrenameStartedis captured viamock()andawait renameStartedPromise— the mock resolves immediately on call, not afterbunWriteinsidewriteSnapshotProjection. The test path does not exercise the real ordering constraint (shouldCommit check before renameSync inside the mock adapter). This is acceptable as a unit-test limitation since the end-to-end j08 fixture drives the real production path. - Gap: The
commitAsyncPreparedFilefunction (src/plan/manager.ts:2292–2319) usesrenameSync(synchronous) and is tested indirectly throughrebuild-plan-marker-supersession-2668.test.ts. No dedicated unit test isolates it in isolation fromrebuildPlan.
📋 Shipped-vs-Claimed Gaps
- Gap:
tests/unit/plan/write-marker-in-progress-manager.test.tsadds two new tests at line 201. The PR description does not mention these tests in the acceptance evidence. However, they are present in the diff, so this is not a stealth addition — the acceptance report listed 7 new test files, not every new test case.
📝 Merge Recommendation
[APPROVE_WITH_FIXES]
One HIGH finding: _internals.rename is typed void | Promise<void> but renameWithTransientRetry silently falls through when the adapter is void-sync, never surfacing the fence-reject (false). The fix is small (one conditional guard at snapshot-writer.ts:91–98) and mechanically correct.
| Check | Result |
|---|---|
| No CRITICAL findings | ✅ |
| No unresolved STEALTH_CHANGE | ✅ |
| No UNSUPPORTED obligations | ✅ |
| Test coverage adequate | ✅ |
| No hardcoded secrets | ✅ |
| All async errors handled | ✅ (PlanRecoverySupersededError chain) |
| Input validation present | ✅ |
| No broken agent role boundaries | ✅ |
| Prompt format contracts intact | ✅ |
| Lockfile consistent | ✅ (no lockfile in diff) |
🔁 Validation provenance
Reviewer-REFUTED findings (dropped):
-
writeSnapshotProjectionmissingshouldCommitpropagation —writeSnapshotProjectionis called withshouldCommitatsnapshot-writer.ts:610(renameWithTransientRetry(tempPath, resolvedPath, shouldCommit)). The LOW finding was refuted:shouldCommitis propagated torenameWithTransientRetry, and the call towriteSnapshotProjectionfromsnapshot-coordination-init.tsuses the_internalsseam which is typed correctly. The claim thatwriteSnapshotdoes not propagateshouldCommittowriteSnapshotProjectionis irrelevant becausewriteSnapshotis the SQLite authority path andwriteSnapshotProjectionis the derived-compatibility shadow path — they are separate write surfaces. -
Mock resource leak in
write-marker-in-progress-manager.test.ts— the existingafterEachcoverstempDirandclosePlanTerminalState. The new tests patchmock.moduleinside the test body; Bun'smock.modulepatches are function-scoped and self-cleaning on test exit. No global state is mutated by these patches. -
verifyWrittenPlanJsonmonkey-patch never restored — the patch atwrite-marker-in-progress-manager.test.ts:202sets_internals.verifyWrittenPlanJsonto a no-op. This is set on the module-level_internalsexport and is never restored. However, Bun'smock.modulecreates fresh module instances per test file; the_internalspatch from this file cannot bleed into other test files. Within this file, both new tests set the same no-op, so no inter-test interference occurs.
Blind-spot findings added:
None — the type mismatch was the only structural defect identified.
Confirmed findings kept:
- [HIGH]
renameWithTransientRetrysilently dropsfalsewhen_internals.renameis void-sync — exact file:line cited above.
🔒 Reviewed by a multi-stage local-first funnel (architect context pack → explorer candidates → critic challenge/author) for high recall with low false-positive noise. Findings are advisory — verify before acting.
|
Feedback resolution — commit |
🤖 Multi-Stage PR ReviewPipeline: MiniMax-M2.7-highspeed (orientation) (context pack) → MiniMax-M2.7-highspeed (explorer) (explore → candidates) → MiniMax-M2.7-highspeed (fallback arbiter) (challenge + blind-spot) 🔍 PR IntentReconstructed from PR description, linked issue #2668, commit messages, and diff:
📦 Implementation SummaryThe PR adds an ✅ /
|
| Obligation | Status | Evidence (file:line) |
|---|---|---|
| O-001 | SUPPORTED |
src/session/hydration-ownership.ts:174-179 — nextHydrationAuthorityEpoch increments; epoch returned in beginHydrationScope at :120-130 |
| O-002 | SUPPORTED |
src/plan/ledger.ts:1100 (initLedger), :1439/:1473 (appendLedgerEvent), :1687 (takeSnapshotEvent); src/plan/manager.ts:2339 (rebuildPlan), :2163 (regeneratePlanMarkdown), etc. |
| O-003 | SUPPORTED |
src/session/snapshot-reader.ts:477-482 — re-checks isHydrationAuthorityCurrent after Promise.allSettled(pendingRehydrations) |
| O-004 | SUPPORTED |
tests/unit/execute-journey/j08-restart-policy-reconciliation.test.ts — asserts session overrides absent post-restart |
| O-005 | SUPPORTED |
src/state.ts:3770-3782 — setRehydrationCache requires exact epoch match |
| O-006 | SUPPORTED |
docs/releases/pending/2668-restart-policy-reconciliation.md, docs/troubleshooting/recovery-runbook.md, docs/testing/execute-journey.md |
🚨 Confirmed Findings
[HIGH] rebuildPlan silently discards the markdown write error when the final marker write fails with a non-supersession error
-
Location:
src/plan/manager.ts:2443-2455 -
Why it matters: If
commitAsyncPreparedFilefor the final.plan-write-markerthrows a non-PlanRecoverySupersededError(e.g. disk full, permissions), thefinallyblock at :2443 unconditionally throws that error. ThemarkdownWriteErrorstored by the earlier catch block at :2437 is discarded, so callers cannot distinguish a markdown failure from a marker failure and the root cause of a degraded save is obscured. -
Evidence: Diff lines:
} catch (error) { markdownWriteFailed = true; markdownWriteError = error; } finally { try { ... await commitAsyncPreparedFile(markerPath, marker, ...); } catch (error) { if (error instanceof PlanRecoverySupersededError) { markerSupersededError = error; } /* Advisory only */ } } +if (markerSupersededError) throw markerSupersededError; +if (markdownWriteFailed) throw markdownWriteError; // ← markdownWriteError discarded if markerSupersededError is setThe control flow is linear:
finallyruns after every path through thetry/catchabove it. If the markercatchcaptures a non-supersession error,markerSupersededErrorstaysundefinedand theif (markerSupersededError)guard passes, butfinallyalready threw. Theif (markdownWriteFailed)line is unreachable. -
Fix direction: Capture both errors (e.g.
let finalError = markerSupersededError ?? (markdownWriteFailed ? markdownWriteError : undefined)) and throw the aggregated context, or throwmarkdownWriteErrorfirst and let the marker error surface in the error chain.
🔬 Unverified but Plausible Risks
-
Epoch exhaustion:
nextHydrationAuthorityEpoch(src/session/hydration-ownership.ts:174-179) throwsError('hydration authority epoch exhausted')whenhydrationAuthorityEpoch >= Number.MAX_SAFE_INTEGER. The reviewer confirmed this; however, the epoch increments only on project eviction or reset (not per session/task), and the maximum tracked project count is 32, making practical exhaustion impossible. The PR explicitly chooses process-monotonic semantics where "an old token can never become current again," which would be violated by graceful degradation. Confidence: low — this is intentional design for a practically unreachable boundary; no runtime guard needed. -
quarantineLedgerSuffixpath-null silent return: IfassertProjectRootor the hash computation throws (beforequarantinePathis assigned), the catch block returns{ path: null, salvagedCount }silently. The caller inreplayFromLedgerWithStatusdoes not use the returned path on the non-error path, so this is not a correctness issue — but callers testing the return value for thepath !== nullcase would get a false negative on these rare I/O failures. Confidence: low — no caller checks the returned path.
🧪 Test / Coverage Gaps
-
Gap:
rebuildPlanhas no test covering a non-PlanRecoverySupersededErrorthrow fromcommitAsyncPreparedFilein the final marker path (thefinally/ error-shadowing scenario above).- Evidence:
tests/unit/plan/rebuild-plan-marker-supersession-2668.test.tstests supersession only;tests/unit/plan/manager-recovery-replay-supersession-2668.test.tstests quarantine/marker supersession but not disk-write failure.
- Evidence:
-
Gap: No test exercises
nextHydrationAuthorityEpochthrowing in production. The ABA test suite (hydration-authority-aba-2668.test.ts) triggers eviction via FIFO flooding but never hits the numeric ceiling.- Evidence: No test file contains a probe that forces
hydrationAuthorityEpoch >= Number.MAX_SAFE_INTEGER.
- Evidence: No test file contains a probe that forces
📋 Shipped-vs-Claimed Gaps
None found. The PR description's acceptance evidence, C1/C2/C3 checks, 17-file isolation suite, mutation probes, and invariant audit are all structurally verifiable from the diff and test files.
📝 Merge Recommendation
[APPROVE_WITH_FIXES]
The HIGH finding — markdownWriteError discarded when the final marker write throws a non-supersession error — is a real, fixable defect introduced by this PR. The finally/if ordering means callers receive the wrong error type on a specific degraded-save path. Fixing it (aggregate both errors or throw in the correct order) is a small, targeted change. No other PR-introduced defect meets the bar.
The epoch-exhaustion "finding" is not a bug: it is intentional design (process-monotonic semantics require an unrecoverable ceiling to prevent stale-token revival), it is practically unreachable (9 quadrillion eviction cycles on a 32-slot FIFO), and the PR explicitly documents it. The reviewer's confirmation is technically accurate but conflates "could theoretically throw" with "is a fixable defect" — it is neither.
| Check | Result |
|---|---|
| No CRITICAL findings | ✅ |
| No unresolved STEALTH_CHANGE | ✅ |
| No UNSUPPORTED obligations | ✅ |
| Test coverage adequate | |
| No hardcoded secrets | ✅ |
| All async errors handled | ✅ |
| Input validation present | ✅ |
| No broken agent role boundaries | ✅ |
| Prompt format contracts intact | ✅ |
| Lockfile consistent | ✅ |
🔁 Validation provenance
Survived challenge:
- HIGH —
rebuildPlanerror shadowing (kept): concrete code path insrc/plan/manager.ts:2437-2455wherefinallythrow makesmarkdownWriteErrorunreachable when the final marker write fails with a non-supersession error. A test covering this specific path is absent.
Dropped:
- Epoch exhaustion (refuted as a fixable defect): intentional design choice with a practically unreachable ceiling (2^53-2 increments on a 32-entry FIFO); violates process-monotonic semantics if degraded gracefully; documented in PR design notes. Kept in Unverified Risks at low confidence.
- 31 reviewer-refuted items (pre-existing list; no diff evidence contradicts those refutations).
- Several speculative concerns were scoped away after tracing the actual
try/finally/control-flow ordering in the diff.
🔒 Reviewed by a multi-stage local-first funnel (architect context pack → explorer candidates → critic challenge/author) for high recall with low false-positive noise. Findings are advisory — verify before acting.
Closes #2668
Summary
Root cause
Restart and hydration work was keyed too coarsely. A delayed task could survive an ordinary generation change or an evict-and-reintroduce cycle, then publish a result for authority it no longer owned. Several recovery paths also validated ownership before an asynchronous operation instead of immediately before publication. Together those gaps allowed stale projections, caches, coordination outcomes, markers, subscriptions, or replay results to overwrite the newer accepted state.
Fix
loadPlanRecurrence prevention
The Phase 4.2 sweep classified 43 related sites across 12 predicates: 40 are fixed and 3 are explicitly out of class. Focused mutation probes removed or moved the reducer, coordinator, recovery, replay, marker, cache, and aggregate fences; each probe turned its targeted regression test red before the exact implementation was restored.
Acceptance evidence
0 pass / 4 fail / 9 expect()) was performed in a disposable external workspace;ISSUE2668_EXPECTED_FAILURE_SETwas a harness label, not a tracked repository file. Reproduce the current head suite withbun --smol test tests/unit/session/restart-reconciliation-2668.test.ts --timeout 60000(11 pass / 0 fail / 46 expect()on feedback commit97a7de56a).5 pass / 0 fail / 36 expect()1 pass / 0 fail / 17 expect()94 pass / 0 fail / 398 expect()across 17 files; feedback regressions:55 pass / 0 fail / 229 expect()across seven modified files, plus the C1 head suite above132 pass / 0 fail / 431 expect()git diff --checkpassed97a7de56a9100b81f923881a97d8a110114a7b2397a7de56a9100b81f923881a97d8a110114a7b23Invariant audit
test_runnerbun:testfiles stay below the 500-line cap, use dependency seams rather than newmock.moduletargets, and pass per-file isolationTest plan
bun run typecheckbun run lint:cibun run buildand Node ESM importRisk and rollback
The principal risk is a legitimate delayed operation being rejected after ownership changes. That is intentional fail-closed behavior and is observable through typed supersession rather than silent partial publication. Rollback is the single commit on this branch; no persistent schema migration is required.
Waivers
None.
Merge status
Ready for CI and maintainer review. Keep as draft until required checks are green.