fix(storage): let crash-recovery finalize tolerate legitimate reclassification - #3370
Conversation
…ification ## Problem Discovered live 2026-07-28, after deploying PRs #3368/#3369 (which fixed two real bugs and measurably improved the writer-hold on this recurring failure from 543s to 210s): the daemon still crashed on every restart with `RuntimeError: raw authority postflight changed a retryable/carried-forward plan`, citing the exact same 4 plan_ids every time. Root cause (a third, distinct layer): `recover_interrupted_raw_authority_frontier` runs on every daemon startup and calls `finalize_raw_authority_census(..., interrupted=True)` on EVERY still-`planned` census -- not just ones with unrecorded outcomes. A "planned" census can sit unfinalized across an arbitrary gap (this one spanned multiple days, verified via `created_at_ms` timestamps in `raw_authority_census_plans`). During that gap, a retryable/ carried-forward plan's own evidence can legitimately shift for reasons unrelated to what the census originally selected -- confirmed via a stopped-daemon read-only diagnostic: all 4 affected sessions now classify as `unresolved_provenance`/`refine_quarantined_raw` (the underlying raw got quarantined by an unrelated safety mechanism), not `duplicate_alias` anymore. `finalize_raw_authority_census`'s strict "every retryable/carried-forward plan must survive byte-identical in the fresh post-census" check applied identically whether `interrupted=True` (crash recovery) or not, even though the `interrupted` parameter was already threaded through to every caller -- it just wasn't consulted by this specific check. This left the stuck census permanently unable to finalize: every recovery attempt re-selected the same stale plan_ids, recomputed a genuinely different `post_ids` set (since the true classification had moved on), and raised -- on every single daemon restart, indefinitely. ## Solution Skip the byte-identical-persistence check when `interrupted=True`. This invariant is correct for a normal, uninterrupted apply (nothing else should touch the archive between planning and finalizing that same call), but wrong for crash recovery, whose entire purpose is reconciling against CURRENT ground truth after an arbitrary gap, not demanding continuity with a stale snapshot. New regression test proves `interrupted=True` tolerates the exact plan shape the existing `test_postflight_rejects_carried_plan_shared_with_retryable_selection` asserts DOES raise for a normal apply (kept unchanged, still passing) -- confirming the fix is scoped precisely to crash recovery. Anti-vacuity: reverting the fix reproduces the exact failure shape. ## Verification - `devtools test tests/unit/storage/test_raw_authority_ledger.py tests/unit/storage/test_duplicate_raw_identity_repair.py` -> 41 passed - `mypy polylogue/storage/raw_authority.py` -> clean - `devtools verify --quick` -> exit 0 Ref polylogue-ewfp, polylogue-ihc8 Co-Authored-By: Claude <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughInterrupted raw-authority census finalization now tolerates carried-forward plan reclassification during crash recovery. A regression test verifies the census reaches the interrupted lifecycle state, while normal finalization retains the existing invariant check. ChangesRaw-authority recovery
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec599579ce
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # finalize, repeatedly re-selecting and re-failing the same already | ||
| # -reclassified plan_ids on every subsequent recovery attempt | ||
| # (observed live: a 200+s writer-lock hold on every daemon restart). | ||
| if not interrupted and not persistent.issubset(post_ids): |
There was a problem hiding this comment.
interrupted=True does not exclusively identify recovery after an arbitrary gap: apply_raw_authority_frontier passes it whenever any strategy returns a retryable outcome (raw_reconciler.py:1393-1399), while still finalizing in the same invocation. If a strategy fails after partially committing durable work—for example, browser-origin copy-forward commits its source stage before the separate index stage—this condition now suppresses the check that would detect the immutable plan disappearing or changing, and publishes the census as interrupted without creating the stale-plan blocker that previously kept the archive fail-closed. Use a distinct actual-recovery flag, or keep this invariant enabled for the ordinary apply caller even when its outcome is retryable.
Useful? React with 👍 / 👎.
… follow-up polylogue-ewfp's crash is fixed and confirmed live: PR #3370 (crash-recovery finalize tolerance) resolves the true recurring-crash mechanism after #3368/#3369 fixed two adjacent layers of the same stack. Verified: the stuck census finally finalized (outcome=success), every previously-starved daemon actor now runs cleanly, no more crash across multiple subsequent passes. Filed polylogue-zaiz for the distinct, unattempted residual: 3 fan-out sessions now correctly classify as refine_quarantined_raw (a separate actuator/workflow) rather than crashing -- non-urgent, daemon operates normally around it. Co-Authored-By: Claude <noreply@anthropic.com>
1 participant
Summary
Fix the final layer of the live incident chain (PRs #3368, #3369 fixed two real bugs and measurably improved the writer-hold from 543s to 210s): crash-recovery finalization was applying the same strict "no plan may change" invariant as a normal apply, which a stuck multi-day-old census can never satisfy once its classification legitimately moves on.
Problem
recover_interrupted_raw_authority_frontierruns on every daemon startup and finalizes every still-plannedcensus. One census had sat unfinalized across multiple days (verified viacreated_at_mstimestamps). A stopped-daemon read-only diagnostic showed the affected sessions now classify asunresolved_provenance/refine_quarantined_raw(the underlying raw got quarantined by an unrelated safety mechanism) — notduplicate_aliasanymore, as they were when the census was originally planned.finalize_raw_authority_census's postflight check required every retryable/carried-forward plan to survive byte-identical in the fresh post-census, regardless of whether this was a normal apply or crash recovery — even thoughinterruptedwas already threaded through as a parameter, just not consulted by this specific check. This left the census permanently unable to finalize: every recovery attempt raised on the same stale plan_ids, on every single daemon restart.Solution
Skip the byte-identical-persistence check when
interrupted=True. Correct for a normal apply (nothing else should touch the archive mid-call); wrong for crash recovery, whose entire purpose is reconciling against current ground truth after an arbitrary gap.Verification
devtools test tests/unit/storage/test_raw_authority_ledger.py tests/unit/storage/test_duplicate_raw_identity_repair.py→ 41 passed (new regression test provesinterrupted=Truetolerates exactly the shape the existing non-interrupted test asserts DOES raise; anti-vacuity confirmed by reverting)mypy polylogue/storage/raw_authority.py→ cleandevtools verify --quick→ exit 0Ref polylogue-ewfp, polylogue-ihc8
Co-Authored-By: Claude noreply@anthropic.com
Summary by CodeRabbit
Bug Fixes
Tests