test+fix(turns): crash-consistency chaos suite + the two crash-recovery defects it found (#6263, #6284) - #6295
Merged
Conversation
…re (#6263 Phase 0) WIP checkpoint: seed-deterministic crash/fault chaos harness over the row store; oracle = lockstep InMemoryTurnStateStore engine. Green on the current write-through store (12 tests) and pins two real defects it found as ignored reproducers (fixed in the following commits): - no-op empty-delta desyncs the journal reservation seq -> a later active-lock DELETE is not durable -> thread stranded after crash; - recover_expired_leases strands a checkpointless crashed run as terminal Failed(lease_expired), violating #6284 (a crash must stay re-drivable). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ion seq (#6263) Found by the crash-consistency chaos suite. A mutation whose durable delta is empty (a claim matching nothing, an idempotent-replay submit, a no-op recover/cancel) still advanced the hot-cache journal_seq while enqueue_delta skipped the (empty) backend append — drifting the reservation seq +1 ahead of the real append log. A later complete_run then pre-reserved its active-lock row at the desynced higher seq while the DELETE tombstone materialized at the real lower seq, so write_materialized_row's current_seq >= journal_seq guard skipped the tombstone: the completed run kept its active lock durably (live cache 0, recovered state 1) and the thread was permanently ThreadBusy after a crash. Both commit paths now early-out on an empty persist_delta: update the hot cache to the new state at the CURRENT seq (no advance) and skip reservation + enqueue, mirroring the existing new_snapshot == baseline no-op early-out but keyed on durable emptiness. The crash suite's generator no longer steers around no-op mutations; the reproducer is un-ignored and green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ead of terminal Failed (#6263, #6284) recover_expired_leases terminated every expired Running lease as terminal Failed(lease_expired) — a dead end for a run that crashed before its first checkpoint, i.e. before any side effect. #6284 forbids this: a crash is not a genuine invariant (allowed terminal set = cancellation / budget / DriverBug), and a pre-first-checkpoint run is always safe to re-drive. New resolve_expired_lease classifier: - CancelRequested -> Cancelled (genuine invariant) — unchanged. - Running with ANY loop checkpoint -> Failed(lease_expired) + latest resumable checkpoint — unchanged. Gating on 'no loop checkpoint at all' rather than 'no *resumable* checkpoint' avoids re-driving a Final-only, post-side-effect run. - Running with NO loop checkpoint -> re-queue to Queued (re-drivable), bounded by claim_count: at/over max_crash_recovery_reclaims (new limits field, default 5) it terminal-fails with a distinct model-visible crash_retry_exhausted reason, never a silent lease_expired. Shared-engine change (applies to both the direct authority and the row store). Existing tests pinning the old checkpointless-terminal behavior updated to the re-drivable contract, or to max_crash_recovery_reclaims=0/1 helpers where they exercise the terminal-recovery publish/lock path; checkpointed, Final-only, and CancelRequested expiry paths unchanged (verified incl. the lease_wedge integration test). Both crash-suite reproducers un-ignored and green. Follow-up: a run whose only checkpoint is a non-resumable Final one still terminates without a resume path (#6284 checkpointed-terminal dead-end) — out of scope here. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
🔎 IronLoop Review StatusHead: Current reviewers:
Reviewer summaries
Recent activity
Available commands
Run metadataAdmission: webhook accepted the request and IronLoop persisted reviewer state before this projection. |
github-actions
Bot
added
scope: dependencies
github-actions
Bot
added
risk: low
|
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 ignored due to path filters (1)
📒 Files selected for processing (9)
📝 WalkthroughSummary by CodeRabbit
WalkthroughLease recovery now re-drives checkpointless runs within a configurable reclaim bound, terminally failing exhausted retries. Row-store durable-empty deltas avoid journal reservations, and extensive crash-consistency tests validate persistence, recovery, locking, and event contracts. ChangesLease recovery and durability
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Scheduler
participant InMemoryTurnStateStore
participant EventPublisher
Scheduler->>InMemoryTurnStateStore: recover_expired_leases
InMemoryTurnStateStore->>InMemoryTurnStateStore: resolve checkpointless lease
InMemoryTurnStateStore->>Scheduler: re-queue or terminal failure
InMemoryTurnStateStore->>EventPublisher: publish recovery event
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
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 |
![ironloopai[bot]](https://avatars.githubusercontent.com/in/4116490?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
⏭️ IronLoop Review Declined: reviewer
Review at a glance
| Disposition | Head |
|---|---|
| ⏭️ Review declined | 9aa733a5dcc0 |
Head: 9aa733a5dcc0ee6a779bb217b813c17465b11d8f
Reason: The supplied base commit is not an ancestor of the head (merge-base is dca1623), so reviewing base..head would conflate this three-commit turns PR with 17,606 lines of unrelated divergence. A reliable complete review is not possible within this comparison.
Next: Refresh the reviewer job with the PR's actual common base (or rebase the head onto the stated base) and rerun the review against that ancestor comparison.
Run details
Status: Current
Trustworthy review produced: no
Summary
Review skipped: the supplied base and head do not form the stated PR comparison, and their direct diff is a mega-change spanning 203 files across unrelated subsystems.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a bounded re-drive mechanism for checkpoint-less runs whose leases expire, allowing them to be safely re-queued up to a configurable limit before failing with crash_retry_exhausted (addressing #6284). It also resolves a hot-cache desynchronization issue in the row store by ensuring that empty durable deltas do not advance the journal reservation sequence (addressing #6263). To safeguard these changes, a comprehensive crash-consistency chaos-testing suite has been added. There are no review comments to address, and I have no additional feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Contributor
Coverage ratchetReborn integration-tier coverageLine coverage (Reborn crates): 85.68% — 313921 / 366395 lines Per-crate breakdown (65 crates, lowest-covered first)
This table itself is informational and never gates the PR on its own — not the percentage, not the per-crate holes, not the 0-coverage callout. A separate coverage ratchet (dry-run until enforce=true; see tests/integration/coverage-floor.toml) can fail the build on specific configured floors. Exemptions (3 entry/entries excluded from the accounting above)
|
Member
Author
|
✅ Ready for merge — crash-consistency chaos suite + the two crash-recovery defects it surfaced (#6263 Phase 0 / #6284). IronLoop: reviewed, 0 blocking. All 59 CI checks green (the one pending is the Railway preview-env deploy, not a merge gate). Reviewed both production fixes — sound:
Both defects ship as passing chaos-suite reproducers (15 tests, 0 ignored); the shared-engine defect-2 fix corrects both the direct authority and the row store. clippy |
ilblackdragon
added a commit
that referenced
this pull request
Jul 20, 2026
Keep PR #6299 current with main's tip so GitHub can compute mergeability and run the heavy CI lanes. #6295 (turns crash-consistency chaos suite + two crash-recovery fixes) auto-merged cleanly — only Cargo.lock and turn_coordinator_contract.rs overlapped, both resolved by git. Verified: `cargo test -p ironclaw_turns --all-features` — 144 passed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
18 of 29 tasks
This branch is being deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Phase 0 of the turn-state consolidation in #6263: a crash-consistency chaos suite that is the acceptance oracle for the Step 3 async-write-behind change — plus the two real crash-recovery defects it surfaced, fixed here so it lands fully green (no ignored tests).
The suite (
crates/ironclaw_turns/tests/row_store_crash_consistency.rs)Seed-deterministic crash/fault chaos harness over
FilesystemTurnStateRowStore:InMemoryBackend: injects write failures (Nth write / next journal append / path prefix) and forks the durable bytes at any moment.StdRngover 4 scopes drawing submit / claim / heartbeat / block(approval|auth) / resume / complete / fail / cancel / recover_expired_leases, incl. idempotent-replay submits and no-op claims; ~1,000 ops across 10+ seeds; seed + op log printed on failure.InMemoryTurnStateStore(the row store's own engine) receives every acked op; recovered snapshot diffed against it, plus internal-consistency invariants and the four [EPIC] error-recoverability endgame — the model recovers from 100% of the errors it sees #6284 crash-recovery invariants (crash → re-drivable; write-failure recoverable + atomic; gate-park + terminal always durable via a namedis_recoverability_criticalpredicate; failure detail survives).Defect 1 — durability: empty delta desyncs the journal reservation seq
An
Okmutation with an empty durable delta (a claim matching nothing, an idempotent-replay submit, a no-op recover/cancel) advanced the hot-cache journal seq whileenqueue_deltaskipped the empty append — drifting the reservation seq ahead of the real append log, so a latercomplete_run's active-lock DELETE materialized below the guard seq and was skipped. After a crash the completed run kept its active lock and the thread was permanentlyThreadBusy. Fix: both commit paths early-out on an emptypersist_delta(advance nothing), mirroring the existing identity no-op early-out. Generator no longer steers around no-ops.Defect 2 — #6284: checkpointless crashed run stranded as terminal
Failedrecover_expired_leasesterminated every expired Running lease asFailed(lease_expired). For a run that crashed before its first checkpoint (before any side effect) #6284 requires it stay re-drivable —lease_expiredisn't a genuine invariant. Fix: aRunningrun with no loop checkpoint at all is re-queued toQueued(re-drivable), bounded byclaim_count— at/overmax_crash_recovery_reclaims(new limit, default 5) it terminal-fails with a distinct model-visiblecrash_retry_exhausted, never a silentlease_expired. Checkpointed / Final-only / CancelRequested expiry paths unchanged. This is a shared-engine change, so it corrects crash recovery for both the direct authority and the row store.Gating on "no loop checkpoint at all" (not "no resumable checkpoint") deliberately avoids re-driving a
Final-only, post-side-effect run.Tests
Full
ironclaw_turnsgreen;ironclaw_runner --features filesystem-goal-storegreen;reborn_integration_lease_wedgegreen (confirms the checkpointed path is untouched); crash suite 15 passed, 0 ignored (both reproducers are now passing regression tests). Existing tests that pinned the old checkpointless-terminal behavior updated to the re-drivable contract, each noted in the commit. clippy-D warningsclean; pre-commit-safety exit 0.Follow-up (out of scope): a run whose only checkpoint is a non-resumable
Finalone still terminates without a resume path — the #6284 checkpointed-terminal dead-end.🤖 Generated with Claude Code