fix(bootstrap): recover state from WAL when WAL is ahead after crash - #1048
Conversation
A crash between `commit_wal` and `commit_state` leaves the WAL tip ahead of the state cursor. On restart `catch_up_stores` only brought archive/index up to the *state* cursor, and the upstream puller builds intersection candidates from the WAL tip — so the peer resumed one block past the gap and the un-applied block's effects (produced/consumed UTxOs) were silently lost. Retarget catch-up to the WAL tip and add `catch_up_state`, which forward-replays WAL entries into the state store (mirror of the rollback loop: apply entity deltas + `apply_utxoset` + set cursor). Only Roll/Genesis units write the WAL, so a WAL that is ahead of state can only ever be ahead by Roll blocks, making forward replay lossless. Mid-boundary recovery (RUPD/EWRAP/ESTART, which don't write the WAL) remains the separate #1018 concern. Also set the state cursor to `Origin` at the end of genesis `execute`, so restart can tell "genesis done" from "genesis never ran" and doesn't re-run genesis (whose `commit_wal` resets the WAL) over blocks awaiting recovery. Skip synthetic empty-block WAL entries in all catch-up paths. Tests: new `test_catchup_recovers_state_from_wal` and `test_catchup_converges_all_stores_to_wal_tip` in `tests/bootstrap.rs`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughGenesis now persists an Origin WAL cursor after bootstrap. Bootstrap catch-up replays state, archive, and index stores to a WAL-derived target. Shared entity replay helpers were added, rollback uses them, and tests cover crash recovery plus rollback restoration. ChangesBootstrap recovery and rollback
Estimated code review effort: 4 (Complex) | ~60 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 |
| enum CrashAfter { | ||
| /// Run commit_wal only — models a crash between `commit_wal` and | ||
| /// `commit_state`. | ||
| Wal, | ||
| /// Run commit_wal + commit_state — models a crash between the state | ||
| /// commit and the archive/index commits. | ||
| State, | ||
| } |
There was a problem hiding this comment.
what about the other potential crash locations? do we have test coverage for those?
There was a problem hiding this comment.
Good catch — the matrix had a hole. There are three inter-store commit boundaries (wal→state→archive→indexes) and only two were modeled. Addressed in af02a33:
CrashAfternow hasWal/State/Archivevariants, one per boundary, andtest_catchup_recovers_indexes_when_archive_aheadcovers the previously untested archive→indexes window.- Two crash classes are intentionally not covered here and now documented on the enum: crashes inside a phase (mid-shard) and crashes during epoch-boundary work units (RUPD/EWRAP/ESTART) — those don't write the WAL at all, so they can't be recovered by WAL replay. That's the Epoch boundary resume is not idempotent — root cause of persisted pool-snapshot lag #1018 boundary-resume territory.
| } | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
this code should be very similar to what the roll-forward does during normal operation. Is there an opportunity to abstract this in a function somewhere and make it more DRY?
There was a problem hiding this comment.
Extracted in a3ee096 — with one correction to the framing: the true twin of this code isn't roll-forward but rollback (sync.rs), which runs the same load→mutate loop in the undo direction. Normal roll-forward (Roll::commit_state) persists entities already mutated in memory during compute — it never re-applies deltas — so only the save step overlaps there.
New shared kernels in core/state.rs: apply_delta_chunk / undo_delta_chunk (load missing entities into a caller-owned map, replay deltas forward/reverse) and save_entities (persist the map). catch_up_state uses the apply direction; rollback the undo direction. Commit cadence stays with each caller (catch-up commits per entry, rollback once) since they need different atomicity.
The refactor also surfaced and fixed a latent bug (was tracked in a local note): rollback undid entities in memory but never saved them back — post-rollback entity state still reflected the undone blocks. Rollback now threads one entity map across all undone entries (so consecutive blocks touching the same entity unwind from the in-memory value, not the stale uncommitted store) and persists it before commit. The rollback lifecycle test now asserts the accounts namespace is byte-identical to its snapshot at the rollback target.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/bootstrap.rs (1)
129-240: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting a shared convergence-assertion helper.
Both new tests (and the existing
test_catchup_recovers_archive_and_indexes) repeat the same three-store convergence assertions (state cursor, archive tip, index cursor vs. WAL tip). A small helper likeassert_stores_at(&domain, &wal_tip)would reduce duplication across the file.♻️ Example helper
fn assert_stores_converged(domain: &ToyDomain, target: &ChainPoint) { assert_eq!(domain.state().read_cursor().unwrap().as_ref(), Some(target)); assert_eq!(domain.archive().get_tip().unwrap().map(|(s, _)| s), Some(target.slot())); assert_eq!(domain.indexes().cursor().unwrap().as_ref(), Some(target)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/bootstrap.rs` around lines 129 - 240, The new catch-up tests duplicate the same state/archive/index convergence checks against the WAL tip. Extract a shared helper in tests/bootstrap.rs (for example around ToyDomain and ChainPoint, like assert_stores_converged or assert_stores_at) and use it from test_catchup_recovers_state_from_wal, test_catchup_converges_all_stores_to_wal_tip, and the existing catch-up test to keep the assertions consistent and reduce repetition.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/core/src/bootstrap.rs`:
- Around line 200-209: After each catch-up helper in bootstrap.rs replays WAL
entries, verify the store cursor/tip actually reaches the requested target
instead of returning Ok(()) based only on skipped entries. Update the logic
around the replay loops in the affected helpers so that synthetic reset_to
entries and state/index entries do not leave the cursor behind silently; re-read
the cursor/tip after the loop and return an error if it still does not match
target, or only advance the cursor in the places where that is known to be safe.
---
Nitpick comments:
In `@tests/bootstrap.rs`:
- Around line 129-240: The new catch-up tests duplicate the same
state/archive/index convergence checks against the WAL tip. Extract a shared
helper in tests/bootstrap.rs (for example around ToyDomain and ChainPoint, like
assert_stores_converged or assert_stores_at) and use it from
test_catchup_recovers_state_from_wal,
test_catchup_converges_all_stores_to_wal_tip, and the existing catch-up test to
keep the assertions consistent and reduce repetition.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a1e91683-3229-46e4-83c2-b392c49161e1
📒 Files selected for processing (3)
crates/cardano/src/genesis/mod.rscrates/core/src/bootstrap.rstests/bootstrap.rs
…eplay Extract the load→mutate→save entity replay shared by crash-recovery catch-up and rollback into `state.rs` helpers: `apply_delta_chunk`, `undo_delta_chunk` and `save_entities`. `catch_up_state` now uses the apply direction; `rollback` uses the undo direction. This surfaces and fixes a latent rollback bug: undone entities were mutated in memory but never written back through the state writer, so after a rollback entity state (accounts, pools, epoch state) still reflected the undone blocks. Rollback now accumulates entities across all undone entries — so consecutive blocks touching the same entity unwind from the in-memory value rather than re-reading the not-yet-committed store — and persists them before commit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… persistence Complete the crash matrix over the inter-store commit boundaries: `CrashAfter::Archive` models a crash between `commit_archive` and `commit_indexes` (only indexes lag), with a test asserting bootstrap catches indexes up to the WAL tip. Mid-shard and epoch-boundary (RUPD/EWRAP/ESTART) crash windows are documented as out of scope on the enum — see #1018. Extend the rollback lifecycle test to snapshot the accounts namespace at the rollback target and assert byte-exact restoration after rollback, covering the entity persistence fix (previously undone entities were never written back). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The catch-up helpers could skip entries (synthetic markers, slot-based dedup) and return Ok while a store silently remained behind the target — e.g. when a `reset_to` wiped the WAL entries needed for replay. State is consensus-critical: fail with InconsistentState when the post-replay cursor doesn't reach the target slot. Archive and indexes keep the lenient log-and-continue handling that `check_archive_in_sync_with_state` already uses, but now flag the residual lag explicitly. Comparisons are by slot, not point: a Slot-only cursor (post-ESTART) at the target's slot is already at the target even though the points differ. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…mment Extract the three copy-pasted "did this store reach the WAL target?" post-conditions into a single `verify_caught_up` helper parameterized by a `CatchUpSeverity` (Fatal for state, Lenient for archive/indexes). The comparison (by slot, so a Slot-only post-ESTART cursor counts as reached) and the fail-vs-warn policy are now expressed once and explicitly, rather than as three subtly different inline checks. Also correct the `(Some(Origin), None)` arm comment in `check_wal_in_sync_with_state`: now that genesis sets the cursor to Origin, a completed genesis takes the `(Some, Some)` arm, so this arm only fires on an interrupted (crash mid-) genesis. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
Problem
A crash between
commit_walandcommit_state(which write to separate redb databases, so they are not atomic) leaves the WAL tip ahead of the state cursor — WAL at block N, state at N-1.On restart, this was silently unrecoverable and corrupted the ledger:
catch_up_storesonly brought archive/index up to the state cursor (N-1), never to the WAL tip.intersect_candidates,src/sync/pull.rs), so the peer resumed streaming at N+1.receive_blockdoes no continuity check against the state cursor, so N+1 applied cleanly on top of N-1 — block N's effects (produced/consumed UTxOs) were never applied to state and were silently lost, and the corruption compounds forward.Root cause
Recovery treated state as the leader and only reconciled followers down to it, while the sync intersection trusts the WAL. Nothing brought state up to the WAL after a crash in that window.
Fix
catch_up_storesso every store (state, archive, index) reconciles forward to the WAL tip under one uniform rule.catch_up_state: forward-replays WAL entries into the state store — the exact mirror of the existingrollbackloop (load entity →delta.apply→save_entity_typed), plusapply_utxoset(compute_catchup(..).utxo_delta)and cursor advance, committed per entry.Roll/Genesiswork units write the WAL (the defaultcommit_walis a no-op; RUPD/EWRAP/ESTART mutate state directly). So a WAL that is ahead of state can only ever be ahead byRollblocks, and eachRollWAL entry fully captures its own state mutation. Recovering a crash mid-epoch-boundary is out of scope here — that's the separate Epoch boundary resume is not idempotent — root cause of persisted pool-snapshot lag #1018 gap (noted in code).Originat the end of genesisexecute(set last, so a mid-genesis crash re-runs genesis cleanly). This lets restart distinguish "genesis done, no blocks yet" from "genesis never ran", so it doesn't re-run genesis — whosecommit_walresets the WAL — over blocks awaiting recovery.reset_to) in all catch-up paths.Behavior note
Because genesis now leaves the state cursor at
Origininstead ofNone,dolos bootstrap'shas_existing_datareports true after a genesis-only run, so re-bootstrapping requires--force/--skip-if-data. This is intentional and safer than silently importing over genesis-seeded state.Testing
test_catchup_recovers_state_from_wal(WAL-only feed →bootstrap()→ state/archive/index all at WAL tip; the replayed block's UTxO is queryable from state) andtest_catchup_converges_all_stores_to_wal_tip(each store at a different point behind the tip).dolos-core(122),dolos-cardano(16),tests/bootstrap.rs(5, incl. existing catch-up/rollback/origin regressions), plussync/housekeeping/snapshotintegration tests; workspace check + clippy clean.tests/boundary_resume.rs(test(cardano): epoch-boundary resume reproductions + import-path evaluation (#1018) #1019) is not onmain, so that regression check should be re-run once the branches meet.🤖 Generated with Claude Code
Summary by CodeRabbit