fix(flatkv): report snapshots a rollback could not remove

[blindchaser]

Rollback promises that no snapshot beyond its target survives, but the removal step dropped both the traversal error and each per-directory error, so it logged "Rollback complete" and returned nil with a future snapshot still on disk. An operator running seid rollback, or startup reconciliation, was told the rewind was clean when it was not.

pruneSnapshots then compounded it: treating every version != currentVersion as "old" let such a remnant take a keep slot, evicting a genuinely older snapshot that rollback still needs as a base.

Describe your changes and provide context

Return the removal error, and run the removal before the WAL is pruned. Propagating the error on its own is not enough, which is what review caught first: at the end of Rollback the WAL and both databases have already reached the target, so an error there aborts CompositeCommitStore.Rollback before it resets the commit-info latches, and rootmulti.RollbackToVersion before it refreshes lastCommitInfo. seid rollback rewinds the app before Tendermint, so failing between those two leaves the app at the target and consensus above it.

Running the removal ahead of the WAL prune gives the invariant that makes the error actionable:

an error from Rollback means the rollback did not take effect.

Every caller that skips its post-rollback bookkeeping on error is then right to, so no caller changes, and a restart replays the un-pruned WAL back to the old tail so the rollback can be retried. The cost is a cached checkpoint the next WriteSnapshot rebuilds, never history.

Attempt every candidate rather than stopping at the first failure. Halting names one directory when several may survive; the errors are joined instead, the shape removeTmpDirs already uses in this file. The remediation text is dropped from the message: atomicRemoveDir renames before unlinking, so a partial removal leaves snapshot-N-removing rather than the snapshot-N the text named, and removeTmpDirs sweeps that on the next open regardless.

Restrict prune candidates to versions strictly below the current one, matching the guard memiavl's pruneSnapshots already applies (memiavl/db.go:572), and log the traversal error it also dropped.

Operator-facing note

Rollback can now fail where it previously logged and continued. The failure is clean and retryable: it happens before the WAL is pruned, so the store is left untouched and a restart plus a re-run converges. A node that cannot unlink a stale snapshot directory during startup reconciliation will fail to start rather than continue with a remnant on disk.

Testing performed to validate your change

• TestRollbackReportsUnremovableSnapshotWithoutRewinding blocks the removal with an undeletable trash directory, then asserts Rollback reports it and leaves the WAL still holding the blocks above the target. Moving the removal back to the end of Rollback changes the failure to open for rollback: cleanup tmp dirs: ..., so this test pins the ordering rather than only the error.
• TestRemoveSnapshotsAboveReportsEveryFailure pins that a failure on the lowest candidate does not hide the others.
• TestPruneSnapshotsIgnoresSnapshotsAboveCurrent was verified to fail under the old != guard, which prunes snapshot-10.
• go test -race ./sei-db/state_db/sc/flatkv/... (292s), plus the rollback and reconcile tests in composite and storev2/rootmulti. golangci-lint reports 0 issues; gofmt -s and goimports clean.

[github-actions]

The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).

Build	Format	Lint	Breaking	Updated (UTC)
✅ passed	✅ passed	✅ passed	✅ passed	Aug 10, 2026, 4:25 PM

[cursor]

PR Summary

Medium Risk
Changes rollback and snapshot retention semantics on the state store—operators and seid rollback now see real errors instead of false success, which is correctness-critical but localized to offline recovery paths.

Overview
Rollback no longer swallows failures when deleting snapshots above the target. Those removals are handled by new removeSnapshotsAbove, which runs before WAL pruning and returns an error (including every failed path via errors.Join) instead of logging and still reporting success. That ordering lets callers such as seid rollback treat a failed rewind as a no-op before app-side or Tendermint bookkeeping runs.

pruneSnapshots only treats snapshots strictly below the current version as prunable, so a leftover snapshot above current (failed rollback or in-flight write) cannot consume a keep slot and delete an older checkpoint rollback still needs. Listing errors during prune are logged instead of ignored.

Tests cover the prune guard, aggregated removal errors, and rollback failing without touching the WAL when a post-target snapshot cannot be removed.

^{Reviewed by Cursor Bugbot for commit a81f20f. Bugbot is set up for automated code reviews on this repo. Configure here.}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 245d310b10

ℹ️ 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".

[codecov]

Codecov Report

❌ Patch coverage is 72.22222% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 58.68%. Comparing base (ab08efb) to head (a81f20f).

Files with missing lines	Patch %	Lines
sei-db/state_db/sc/flatkv/snapshot.go	72.22%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3887      +/-   ##
==========================================
- Coverage   59.45%   58.68%   -0.78%     
==========================================
  Files        2319     2241      -78     
  Lines      198379   190384    -7995     
==========================================
- Hits       117946   111720    -6226     
+ Misses      69235    68052    -1183     
+ Partials    11198    10612     -586     

Flag	Coverage Δ
sei-db	70.41% <ø> (-0.22%)	⬇️
sei-db-state-db	?
sei-db-state-db-pr	75.24% <72.22%> (?)

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

Files with missing lines	Coverage Δ
sei-db/state_db/sc/flatkv/snapshot.go	67.51% <72.22%> (+0.15%)	⬆️

... and 79 files with indirect coverage changes

🚀 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.

[seidroid]

Both changes are correct and well-motivated: restricting prune candidates to versions strictly below currentVersion matches memiavl's guard and the new test genuinely fails under the old v != currentVersion logic, and surfacing the removal error makes Rollback's contract honest. Remaining notes are about the accuracy of the new error message (partial removal leaves a -removing path, and traversal aborts at the first failure so other remnants go unnamed) plus the operational effect of the now-propagating error.

Findings: 0 blocking | 5 non-blocking | 2 posted inline

Blockers

• None at the file/PR level.

Non-blocking

• Behavioral change worth calling out: Rollback's error now propagates through CompositeCommitStore.Rollback and the version-reconciliation path (sei-db/state_db/sc/composite/store.go:884), so a node that reaches the target version but cannot unlink a stale snapshot directory will now fail startup reconciliation instead of continuing. This looks intentional and is retryable across a restart (the WAL is already pruned and current points at the base, so a re-run re-attempts the removal), but it converts a previously survivable filesystem hiccup into a startup failure — worth a line in the PR description or release notes for operators.
• Test coverage stops at the helper: TestRemoveSnapshotsAboveKeepsTargetAndBelow and TestRemoveSnapshotsAboveReportsFailure cover removeSnapshotsAbove directly, but nothing asserts that Rollback itself now returns (rather than swallows) that error — which is the actual regression the PR describes. The existing TestRollbackRemovesPostTargetSnapshots could be paired with a variant that makes the removal fail and asserts Rollback returns an error naming the leftover directory.
• The Cursor second-opinion file (./cursor-review.md) is empty — that pass produced no output, so its perspective is not represented in this consolidated review.
• 2 suggestion(s)/nit(s) flagged inline on specific lines.

[seidroid]

A well-targeted correctness fix: Rollback no longer reports success while a snapshot above the target survives, and pruneSnapshots no longer lets such a remnant take a keep slot and evict a genuinely older snapshot (matching memiavl's version >= currentVersion guard, which I verified at sei-db/state_db/sc/memiavl/db.go:572). No blockers; the notes below are about a doc-comment invariant that is overstated, error-message consistency, and a pre-existing composite-rollback gap that Codex surfaced.

Findings: 0 blocking | 7 non-blocking | 3 posted inline

Blockers

• None at the file/PR level.

Non-blocking

• Codex's point, kept with a downgrade: CompositeCommitStore.Rollback (sei-db/state_db/sc/composite/store.go) rolls back memIAVL first and flatKV second, with no compensation if the second fails after the first succeeded — memIAVL stays rewound while flatKV self-heals to its old tail on restart. That hazard already exists for every other error return in flatKV.Rollback (closeDBsOnly, updateCurrentSymlink, SNAPSHOT_BASE removal, open, WAL close/prune/reopen, catchup, version mismatch), so this PR adds one more instance rather than a new class — and the alternative (keep swallowing the error) is exactly the bug being fixed. Worth a follow-up on the composite's two-phase ordering; not a reason to hold this change.
• pruneSnapshots's new traversal-error branch (returns 0 and logs) has no test. The two new prune/remove tests cover the happy paths and the per-directory failure path, but not the traverseSnapshots failure that the diff newly stops ignoring.
• With pruneSnapshots now restricted to v < currentVersion, a remnant above the current version has no dedicated reclaim path — it is only cleared by a later successful Rollback, or overwritten once the chain re-reaches that version (WriteSnapshot's atomicRemoveDir(finalPath) at snapshot.go:497). That is the right trade-off and the PR documents it, but it is worth stating in the doc comment that the remnant lingers rather than being collected, so a reader does not assume something else sweeps it.
• The Cursor second-opinion file (./cursor-review.md) is empty — that pass produced no output, so this review reflects only the Claude and Codex passes.
• 3 suggestion(s)/nit(s) flagged inline on specific lines.

[seidroid]

[suggestion] This invariant is overstated: by the time this step runs, things have moved.

• CompositeCommitStore.Rollback calls memIAVL.Rollback(targetVersion) before flatKV.Rollback(targetVersion), so the cosmos stores are already rewound when this returns an error.
• Within flatkv itself, closeDBsOnly, updateCurrentSymlink(dir, snapshotName(baseVersion)) and the SNAPSHOT_BASE removal have all already run (lines 662-676).

The claim that actually holds — and it is the load-bearing one — is narrower: the WAL is untouched, so flatkv replays back to its old tail on restart, and Tendermint has not been rolled back yet because the CLI does the app first (sei-cosmos/server/rollback.go, rollbackAppState before rollbackTendermintState). Per AGENTS.md the doc comment is where the why lives, so it is worth narrowing "before either moves" to that, and saying explicitly that memIAVL is left rewound.

[seidroid]

[suggestion] Bare return err here is the only mid-rollback error return that does not name the store's state. The WAL-window errors just below (lines ~698-712) all end with "store is mid-rollback, restart to recover then retry", and the earlier steps wrap with "... for rollback". At this point the DBs are closed, current already points at snapshot %d and SNAPSHOT_BASE is gone — the operator needs the same restart-then-retry hint, and the store's identity in the message. Suggest wrapping consistently, e.g.:

if err := removeSnapshotsAbove(dir, targetVersion); err != nil {
	return fmt.Errorf("rollback to version %d (from snapshot %d): %w; "+
		"store is mid-rollback, restart to recover then retry", targetVersion, baseVersion, err)
}

[seidroid]

[nit] Good test — I traced through atomicRemoveDir (snapshot.go:314) and confirmed the mechanism works: the ignored os.RemoveAll(trashPath) cannot unlink occupied inside a 0555 dir, so the subsequent os.Rename fails on a non-empty target. That dependency on RemoveAll's error being discarded is load-bearing and a bit indirect; consider naming it in the comment so a future change to atomicRemoveDir's error handling does not silently turn this into a no-op test that passes for the wrong reason.

[cursor]

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.

^{Reviewed by Cursor Bugbot for commit a81f20f. Configure here.}

[cursor]

Early removal wedges dual-backend rollback

High Severity

Moving removeSnapshotsAbove before WAL prune means a removal failure aborts after memiavl.Rollback has already persisted its rewind. The next LoadLatest sees mismatched backend heights, reconcileVersions re-enters flatkv.Rollback, hits the same failure, and can leave the node unable to start. With removal at the end, both stores were already at the target so restart skipped that path.

Additional Locations (1)

• sei-db/state_db/sc/flatkv/snapshot.go#L730-L735

 

^{Reviewed by Cursor Bugbot for commit a81f20f. Configure here.}