fix(restore): force joiners of a restored cluster through checkpoint sync

[Azorlogh]

Bug

A restored node's FSM genesis is the entire backup, but its raft log is brand new and claims completeness from index 1: bootstrap plants the WAL snapshot at the RESTORED marker's lastAppliedIndex, which restore preparation reset to 0.

Raft only sends MsgSnap when a follower needs entries that have been compacted away. With a snapshot at 0 and an intact log from index 1, a learner joining before the leader's first post-restore raft snapshot is "caught up" by plain MsgApp replay from index 1 — applied onto an empty store. It silently misses every restored row and keeps only what the replayed entries' cache preloads re-materialize, permanently desynced from the rest of the cluster. Whether a given joiner races the leader's first raft snapshot decides corruption vs. correctness.

Found by the model test on Antithesis (#1593 runs 87dbab…-57-8 / 5e600bbb…-57-8): right after each completed restore cycle, the freshly joined follower's post-commit sentinel fired aggregated volume imbalance at raft index 8–32 and the node crash-looped (WAL replay reproduces it deterministically). Its volume table held only world + post-restore accounts; pod-0, applying the same entries on the restored store, never fired. A control run whose restore cycles never completed shows zero hits.

Fix

PrepareForBackup now pins the restored store's lastAppliedIndex to 1 — the index the restored FSM genesis occupies in the new log. The RESTORED bootstrap plants its WAL snapshot there (CreateSnapshot's restore branch — empty storage, index > 0, term 1 — existed for exactly this), so the log starts at 2 and raft must route every fresh peer through MsgSnap → InstallSnapshot → checkpoint sync, which transfers the full store. The join-safety becomes structural instead of racing the first raft snapshot.

Both restore flows (gRPC restore finalize and offline ledgerctl store bootstrap) share this preparation step.

Repro / regression test

New spec at the end of the restore e2e: joins a fresh learner to the freshly restored node, proves log catch-up via a fence transaction, then asserts a pre-backup balance that no post-restore entry (and no cache preload) can re-materialize — the learner serves it iff the restored store itself was transferred. Before the fix, the learner's store lacked even the ledger row (ledger restore-ledger not found), matching the Antithesis signature.

-tags "e2e,s3" -ginkgo.focus Restore: 37/37 specs green with the fix (36 passed + this one failed without it).

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

🗂️ Base branches to auto review (1)

• main

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c84e25fd-7c23-4647-b822-9aaba1aded94

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/restore-learner-join-snapshot

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[NumaryBot]

🛑 Changes requested — automated review

The core restore-index fix (pinning applied index to the checkpoint boundary) and the cache generation realignment are sound and well-tested. The main outstanding structural concern is the commit ordering in the finalize path: if the process crashes or encounters an error after HardLink creates checkpoints/0 but before WriteRestoredMarker completes, the staging store is already invalidated. A subsequent retry then fails the fresh-target guard because the checkpoint exists without a RESTORED marker, making the restore non-retryable without manual cleanup. This applies to both the gRPC server restore path and the offline bootstrap path. This issue was raised in prior discussion (NumaryBot thread, thread 2) and remains unresolved per the available evidence — the prior reply from Azorlogh describes switching to a temp+rename approach for the marker write itself but does not address the case where the checkpoint (checkpoints/0) has already been placed and the marker rename fails. The fix should either clean up the checkpoint on marker-write failure, or detect and tolerate the partial state on retry.

Findings outside the diff

🟠 [major] Checkpoint left in place when marker write fails, making restore non-retryable — internal/adapter/grpc/server_restore.go:368 _{(codex, NumaryBot)}

If the process is killed or encounters an error after HardLink creates checkpoints/0 but before WriteRestoredMarker completes (e.g. ENOSPC, permissions, cross-filesystem issue, or power loss), the error-path rollback never removes the checkpoint. A subsequent retry then fails the fresh-target guard because checkpoints/0 already exists without a RESTORED marker, leaving the data directory in a permanently broken half-restored state that requires manual cleanup. The same commit-ordering window exists in the offline ledgerctl store bootstrap path.

Suggestion: On failure from WriteRestoredMarker, explicitly remove the newly created checkpoint directory before returning the error. Alternatively, detect the partial-restore state on retry (checkpoint present, no marker) and re-attempt only the marker creation step rather than rejecting as non-fresh.

[gfyrag]

Thanks, the production direction looks right: pinning the restored genesis at Raft index 1 makes the empty-WAL restore snapshot push the first real entry to 2, and the local WAL/state behavior lines up with that.

I do not think this should merge yet because the regression coverage can currently pass without proving that the learner received the restored store.

Blocking items:

• The new learner test uses default linearizable GetAccount calls against the joiner. RoutedController.readCtrl falls back to the leader while the local node is syncing, so the join-fence, alice, and treasury reads can be served by node 1. Force local reads with outgoing metadata x-consistency: stale, or explicitly wait for the joiner to leave sync before checking restored rows.
• The new test runs at the end of the ordered restore suite. DefaultTestInstruments enables background maintenance every 5s, and the restored node has already executed several post-restore writes before the learner joins, so a maintenance WAL snapshot/checkpoint may already exist. That undercuts the advertised before any raft snapshot regression. Move the learner join right after restored bootstrap, or override maintenance so no background snapshot can happen first.
• Keep the docs aligned with the new invariant: docs/ops/cli.md and docs/technical/architecture/subsystems/fsm/deterministic-fsm.md still say prepare resets the applied index to 0; the RESTORED marker JSON example in docs/ops/backup-restore.md still shows 12345; the test comment in internal/infra/attributes/prepare_test.go still says 0.
• gofmt -l tests/e2e/cluster/restore_test.go still reports the e2e file.
• Since restored bootstrap now relies on RESTORED.lastAppliedIndex >= 1, please fail loudly if the marker says 0 before wal.CreateSnapshot. Otherwise a stale/manual marker silently re-enters the old unsafe path: snapshot at 0, log starts at 1, and a fresh learner can replay onto an empty store.

[codecov]

Codecov Report

❌ Patch coverage is 46.15385% with 42 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.72%. Comparing base (680efb2) to head (8311f1f).
⚠️ Report is 3 commits behind head on release/v3.0.

Files with missing lines	Patch %	Lines
internal/infra/node/restored_marker.go	0.00%	11 Missing ⚠️
internal/adapter/grpc/server_restore.go	0.00%	8 Missing ⚠️
internal/infra/attributes/prepare.go	70.00%	4 Missing and 2 partials ⚠️
internal/storage/dal/store.go	62.50%	3 Missing and 3 partials ⚠️
internal/bootstrap/module_restore.go	0.00%	5 Missing ⚠️
internal/infra/node/node.go	0.00%	4 Missing ⚠️
internal/infra/state/cache_snapshotter.go	75.00%	1 Missing and 1 partial ⚠️

❌ Your patch check has failed because the patch coverage (46.15%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@              Coverage Diff              @@
##           release/v3.0    #1596   +/-   ##
=============================================
  Coverage         74.72%   74.72%           
=============================================
  Files               430      430           
  Lines             45722    45782   +60     
=============================================
+ Hits              34164    34209   +45     
- Misses             8510     8518    +8     
- Partials           3048     3055    +7     

Flag	Coverage Δ
e2e	74.72% <46.15%> (+<0.01%)	⬆️
scenario	74.72% <46.15%> (+<0.01%)	⬆️
unit	74.72% <46.15%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Azorlogh]

@gfyrag all five items addressed in 3456662 + f61e165:

• Leader fallback: confirmed in RoutedController.readCtrl — the joiner's reads now carry x-consistency: stale, pinning every assertion to the learner's own store. Re-verified the red-proof with the fix reverted: the spec now fails via the learner's local reads (ledger restore-ledger not found for the full window).
• Background snapshot: the restored node runs with maintenance at 1h and a 1M compaction margin, so the spec deterministically exercises the log-as-only-catch-up-source window.
• Docs: cli.md, deterministic-fsm.md, the RESTORED marker JSON example, and the prepare_test comment aligned — plus a sweep caught a few more (FinalizeRestore step list was missing the now-load-bearing prepare step; RestoredMarker.LastAppliedIndex now documents the >= 1 contract). The manifest's 12345 example is the source's real backup-time index, so it stays.
• gofmt: clean.
• Marker = 0: restored bootstrap now fails loudly before wal.CreateSnapshot with a pointer to re-run finalize/bootstrap.

Suite state with the hardened test: 37/37 green with the fix, red without it.

[gfyrag]

Follow-up after the latest commits: the five blocking points from my previous review look addressed now. The learner regression now uses local stale reads, prevents background maintenance/compaction from creating a later snapshot first, the docs are aligned with the current model, formatting is clean, and RESTORED.lastAppliedIndex == 0 now fails loudly before wal.CreateSnapshot.

On the applied-index model itself, I rechecked the alternative of preserving the checkpoint's existing lastAppliedIndex instead of pinning it to 1. I think that approach is valid and arguably cleaner than a magic 1, but only with a precise contract:

• Treat the preserved value as the restored cluster's genesis snapshot boundary, not as the final source-cluster applied index. ApplyExports / RebuildDelta do not currently advance or export a final Raft applied index for incremental backups, so after full + incremental restore this value is the full checkpoint boundary, while the restored state may include later exported audit/log data.
• Keep the same N in both places: Pebble SubGlobLastAppliedIndex and the RESTORED marker used by wal.CreateSnapshot(N). With snapshot N, the new log starts at N + 1; the FSM gap detector also expects the next entry after Pebble's lastAppliedIndex, so those two values must stay identical.
• Require N > 0 before boot. Snapshot 0 is the unsafe empty-snapshot shape this PR is fixing, so preserving 0 would re-open the learner replay bug. If we want to support a full checkpoint taken at index 0 plus later incrementals, we would need either a fallback genesis index (1) or an exported final restore boundary.
• Guard pathological overflow (N == math.MaxUint64) because multiple Raft paths do N + 1.
• Add coverage with N > 1: prepare should preserve the seeded index, finalize/bootstrap should write the marker with that same index, restored boot should create the WAL snapshot at N, the first post-restore proposal should apply at N + 1, and the current learner e2e should keep the stale-read/no-maintenance protections so it proves checkpoint sync rather than leader fallback or a later maintenance snapshot.

The rest of the cleanup still needs to remain exactly as this PR has it: persisted config, peers, cluster-transient state, persisted bloom blocks, and cache rows are source/checkpoint-local and should not survive restore preparation.

So I would be fine with switching from lastAppliedIndex = 1 to preserving the restored checkpoint boundary N, provided the implementation documents that boundary semantics and covers the non-1 case explicitly.

[Azorlogh]

@gfyrag switched to the preserved-boundary design in c3e4766, following your contract point by point:

• PrepareForBackup preserves the checkpoint's applied index as the restored genesis boundary; the code and docs state explicitly that it labels the new log's start and is not state provenance (incremental exports are sequence-keyed and never advance it, so the restored state is newer than the boundary).
• Pebble and the RESTORED marker can't diverge: finalize/bootstrap read the marker value back from the prepared store, same as before.
• N = 0 (genesis checkpoint + incrementals) takes the fallback boundary 1 — the cheaper of the two options you listed, since an exported final restore boundary is a backup-format change; bootstrap's loud refusal of a 0-marker stays as the second line of defense.
• N = MaxUint64 is refused in prepare (raft paths compute boundary+1).
• Non-1 coverage: unit tests for preserve (seeded 200, and the tar-pipeline at N=5), the genesis fallback, and the MaxUint64 guard; a new e2e spec asserts marker.lastAppliedIndex == manifest.checkpoint.lastAppliedIndex with N > 1; restored boot + first apply at N+1 are exercised by the whole Phase 3 suite (the FSM gap check would fail loudly on any divergence); and the learner-join spec now runs at N > 1 with the stale-read / no-maintenance protections unchanged.
• The other five preparation resets (config, peers, cluster-transient, bloom, cache) are untouched.

Suite state: 38/38 restore specs green; red-proof with the old reset-to-0 restored shows the new marker spec failing and the 0-guard refusing every restore boot.

[NumaryBot]

NumaryBot posted 1 new inline finding.

Summary: #1596 (comment)

[gfyrag]

I re-reviewed c3e4766 in detail. The preserved-boundary model itself looks sound: using the checkpoint applied index as the restored genesis snapshot boundary keeps Pebble lastAppliedIndex and the initial WAL snapshot aligned, and the normal Raft/WAL path starts the new log above that boundary.

I also checked the cache-generation finding from NumaryBot. I do not think that is the main issue as stated: after restored boot, admission waits on WaitLeaderReady; the leadership no-op at N+1 goes through PrepareDecodedEntries, and CheckRotationNeeded(N+1) realigns currentGeneration / BaseIndex before writes are admitted. So a high preserved N should not by itself leave admission permanently two generations ahead.

That said, I still do not think this should merge yet:

1. The restore/bootstrap freshness guard only rejects existing checkpoints/, not an existing live/ database. RestoreModule and ledgerctl store bootstrap both call ScanLatestCheckpointID, but normal startup prefers an existing live/ over checkpoints/0. If someone runs restore against a data dir containing live/ but no checkpoints, finalize/bootstrap can write a restored checkpoint plus RESTORED, then the next boot opens the stale live/ store and creates the restore WAL snapshot from the marker against the wrong Pebble state. The preserved-boundary design makes this marker/store alignment load-bearing, so the guard needs to reject existing live/ as well (and probably any pre-existing RESTORED/WAL state).

2. The gRPC FinalizeRestore path writes RESTORED before hard-linking staging into checkpoints/0. If HardLink fails, or the process dies after the marker write but before checkpoint placement, the next normal boot can observe a valid nonzero marker without the restored checkpoint in place. That can make NewNode create a WAL snapshot at the marker index over an empty or stale live/ store. Please write the marker only after checkpoint placement succeeds, preferably via temp-file + rename, and clean up/refuse stale markers on failure.

3. PrepareForBackup reads the boundary with dal.ReadUint64, which returns the default for both missing and too-short values. A corrupt short SubGlobLastAppliedIndex is therefore silently treated as 0 and rewritten as fallback boundary 1. Since this path already treats MaxUint64 as corrupt, it should also fail closed on malformed length while still allowing the genuinely-missing genesis case if that case is intentional.

4. The consumer-side marker guard rejects 0 but not math.MaxUint64. A manual/corrupt RESTORED marker with lastAppliedIndex = MaxUint64 can still reach wal.CreateSnapshot; WAL.FirstIndex() computes snapshot.Index + 1, which wraps. The marker consumer should duplicate the overflow guard.

5. The Dirty check is currently failing. The CI log shows golangci-lint --fix rewrites two constant fmt.Errorf(...) calls to errors.New(...) in internal/infra/attributes/prepare.go and internal/infra/node/node.go; those generated fixes need to be committed.

6. Some API comments are still stale. misc/proto/restore.proto still says FinalizeRestore compacts attributes, and PreviewRestoreResponse.last_applied_index still says Last Raft index applied. With this PR, the finalized value is a restore genesis boundary, and preview can still show 0 before PrepareForBackup converts a genesis checkpoint to fallback 1. Please update the proto comments and regenerate the restore protobufs.

Focused checks I ran locally:

GOROOT= go test ./internal/infra/attributes ./internal/infra/cache ./internal/infra/plan ./internal/storage/wal ./internal/infra/node -run 'TestPrepareForBackup|TestAttributeCache_CheckCache|TestBuildPreloads_RejectsCacheHorizonExceeded|TestCreateSnapshot_RestoreOnEmptyStorage|TestRaftApplied_ControlsCommittedEntriesDelivery' -count=1

gofmt -l ... and git diff --check origin/release/v3.0...HEAD were clean locally.

[NumaryBot]

NumaryBot review complete: no remaining inline findings.

Resolved 1 stale NumaryBot review thread (1 fixed, 0 outdated).

Summary: #1596 (comment)

[Azorlogh]

@gfyrag all six items addressed (688189a; the Dirty fix was already in 763f000, and c108eae hardened the NumaryBot cache-generation window you also analyzed — RestoreFromStore now realigns the generation when meta is absent, closing the transient between boot and the election no-op):

1. Freshness guard: dal.ValidateFreshRestoreTarget rejects existing live/, live.staging/, live.discard/ and checkpoints; both restore mode and store bootstrap additionally reject a pre-existing RESTORED marker. Unit-tested.
2. Finalize ordering: checkpoint placement now precedes the marker, and the marker write is atomic (temp + rename) via a shared node.WriteRestoredMarker used by both flows — the marker is the restore's commit point.
3. Malformed boundary: PrepareForBackup reads the raw key; present-but-wrong-length fails closed ("corrupt checkpoint"), only ErrNotFound takes the genesis fallback. Unit-tested.
4. Marker MaxUint64: consumer-side guard added next to the zero guard.
5. Dirty: green as of 763f000.
6. Proto comments: FinalizeRestore no longer claims attribute compaction, last_applied_index documents the pre-preparation staged value and the boundary conversion; restorepb regenerated.

Restore e2e 38/38 green, full pre-commit clean.

[NumaryBot]

NumaryBot posted 1 new inline finding.

Summary: #1596 (comment)

[NumaryBot]

🛑 Changes requested — automated review

The core restore-index fix (pinning applied index to the checkpoint boundary) and the cache generation realignment are sound and well-tested. The main outstanding structural concern is the commit ordering in the finalize path: if the process crashes or encounters an error after HardLink creates checkpoints/0 but before WriteRestoredMarker completes, the staging store is already invalidated. A subsequent retry then fails the fresh-target guard because the checkpoint exists without a RESTORED marker, making the restore non-retryable without manual cleanup. This applies to both the gRPC server restore path and the offline bootstrap path. This issue was raised in prior discussion (NumaryBot thread, thread 2) and remains unresolved per the available evidence — the prior reply from Azorlogh describes switching to a temp+rename approach for the marker write itself but does not address the case where the checkpoint (checkpoints/0) has already been placed and the marker rename fails. The fix should either clean up the checkpoint on marker-write failure, or detect and tolerate the partial state on retry.

Findings outside the diff

🟠 [major] Checkpoint left in place when marker write fails, making restore non-retryable — internal/adapter/grpc/server_restore.go:368 _{(codex, NumaryBot)}

If the process is killed or encounters an error after HardLink creates checkpoints/0 but before WriteRestoredMarker completes (e.g. ENOSPC, permissions, cross-filesystem issue, or power loss), the error-path rollback never removes the checkpoint. A subsequent retry then fails the fresh-target guard because checkpoints/0 already exists without a RESTORED marker, leaving the data directory in a permanently broken half-restored state that requires manual cleanup. The same commit-ordering window exists in the offline ledgerctl store bootstrap path.

Suggestion: On failure from WriteRestoredMarker, explicitly remove the newly created checkpoint directory before returning the error. Alternatively, detect the partial-restore state on retry (checkpoint present, no marker) and re-attempt only the marker creation step rather than rejecting as non-fresh.

Summary: #1596 (comment)

[gfyrag]

Approved current head 8311f1ff. I rechecked the restore finalize path and the previous checkpoint/RESTORED marker concern is addressed: both finalize flows roll back the placed checkpoint if marker creation fails, and the fresh-target guard can reclaim the lone half-finalized checkpoints/0 state after a crash. The cache-generation realignment path also looks correct for restored stores with no persisted cache meta.

Caveats: codecov/patch is still failing and the NumaryBot status is still in an error state, but I did not find a remaining code blocker in this pass.