Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
blindchaser wants to merge 2 commits into
base: main
Choose a base branch
from
+155 −15
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
245d310
fix(flatkv): report snapshots a rollback could not remove
blindchaser Aug 10, 2026
a81f20f
fix(flatkv): remove post-target snapshots before the WAL is pruned
blindchaser Aug 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 45 additions & 15 deletions sei-db/state_db/sc/flatkv/snapshot.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -526,6 +526,11 @@ func (s *CommitStore) WriteSnapshot(_ string) (err error) {
// pruneSnapshots removes old snapshots beyond SnapshotKeepRecent, keeping
// the latest snapshot (currentVersion) plus the N most recent older ones.
// Best-effort: errors are logged but do not fail the snapshot operation.
//
// Only snapshots strictly below currentVersion are candidates. A snapshot above it is either a rewrite in
// progress or a remnant of a rollback that could not finish, and neither is this function's to reclaim:
// counting one as "old" would spend a keep slot on it and evict a genuinely older snapshot that rollback
// still needs as a base. memiavl's pruneSnapshots applies the same guard.
func (s *CommitStore) pruneSnapshots(dir string, currentVersion int64) int {
start := time.Now()
defer func() {
Expand All @@ -536,12 +541,15 @@ func (s *CommitStore) pruneSnapshots(dir string, currentVersion int64) int {
pruned := 0

var older []int64
_ = traverseSnapshots(dir, false, func(v int64) (bool, error) {
if v != currentVersion {
if err := traverseSnapshots(dir, false, func(v int64) (bool, error) {
if v < currentVersion {
older = append(older, v)
}
return false, nil
})
}); err != nil {
logger.Error("prune snapshots: failed to list snapshot dirs", "err", err)
return 0
}

if len(older) <= keep {
return 0
Expand Down Expand Up @@ -628,9 +636,10 @@ func (s *CommitStore) rollbackBaseVersion(dir string, targetVersion int64) (int6
// A failure while resetting the WAL leaves the store mid-rollback: "current" and the working directory are
// already at the rollback snapshot while the WAL still holds the blocks past targetVersion, and s.wal is
// closed. Retrying in-process does not work, because establishing reachability reads the WAL's stored range
// and that now fails as closed. Nothing is lost — snapshots above targetVersion are removed only at the very
// end, so a restart replays the un-pruned WAL back to its old tail and the rollback can be retried. The
// errors from that window say so.
// and that now fails as closed. No block is lost: the un-pruned WAL still holds them, so a restart replays
// back to the old tail and the rollback can be retried. The errors from that window say so. Snapshots above
// the target are already gone by then, which costs a cached checkpoint the next WriteSnapshot rebuilds, not
// history.
func (s *CommitStore) Rollback(targetVersion int64) (err error) {
obs := s.observeOp("Rollback", otelMetrics.RollbackLatency,
"targetVersion", targetVersion)
Expand Down Expand Up @@ -666,6 +675,10 @@ func (s *CommitStore) Rollback(targetVersion int64) (err error) {
return fmt.Errorf("remove SNAPSHOT_BASE for rollback: %w", err)
}

if err := removeSnapshotsAbove(dir, targetVersion); err != nil {
return err

@seidroid seidroid Bot Aug 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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)
}

}

@cursor cursor Bot Aug 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit a81f20f. Configure here.


if err := s.open(); err != nil {
return fmt.Errorf("open for rollback: %w", err)
}
Expand Down Expand Up @@ -707,21 +720,38 @@ func (s *CommitStore) Rollback(targetVersion int64) (err error) {
targetVersion, s.committedVersion)
}

_ = traverseSnapshots(dir, true, func(v int64) (bool, error) {
if v > targetVersion {
if err := atomicRemoveDir(filepath.Join(dir, snapshotName(v))); err != nil {
logger.Error("failed to remove snapshot", "version", v, "err", err)
}
}
return false, nil
})

logger.Info("FlatKV Rollback complete",
"version", s.committedVersion,
"elapsed", obs.elapsed())
return nil
}

// removeSnapshotsAbove deletes every snapshot directory above targetVersion.
//
// A failure here is returned rather than logged, which is why the step runs before the WAL is pruned: an
// error then means the rollback did not take effect, so a caller that skips its own post-rollback bookkeeping
// on error is right to. `seid rollback` in particular rewinds the app before Tendermint, and aborting between
// those two would leave the two heights apart; running here it aborts before either moves. Reporting success

@seidroid seidroid Bot Aug 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// with a snapshot above the target still on disk would break the same contract from the other side.
//
// Every candidate is attempted even after one fails, because their removals are independent and the caller
// needs the whole list to reconcile from, not just the first name. This mirrors removeTmpDirs.
func removeSnapshotsAbove(dir string, targetVersion int64) error {
var errs []error
if err := traverseSnapshots(dir, true, func(v int64) (bool, error) {
if v <= targetVersion {
return false, nil
}
if err := atomicRemoveDir(filepath.Join(dir, snapshotName(v))); err != nil {
errs = append(errs, fmt.Errorf("remove snapshot %d above rollback target %d: %w", v, targetVersion, err))
}
return false, nil
}); err != nil {
return fmt.Errorf("list snapshots above rollback target %d: %w", targetVersion, err)
}
return errors.Join(errs...)
}

// tryTruncateWAL truncates WAL entries older than the earliest snapshot, keeping enough entries for rollback
// to any retained snapshot. Scheduling the truncation is best-effort in that it is skipped when there is
// nothing to prune against, but a prune that fails is not a benign outcome: it only fails when the WAL is
Expand Down
110 changes: 110 additions & 0 deletions sei-db/state_db/sc/flatkv/snapshot_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1059,6 +1059,35 @@ func TestPruneSnapshotsKeepAll(t *testing.T) {
require.Equal(t, 4, count, "all snapshots should be kept when KeepRecent is large")
}

func TestPruneSnapshotsIgnoresSnapshotsAboveCurrent(t *testing.T) {
cfg := config.DefaultTestConfig(t)
cfg.DataDir = filepath.Join(t.TempDir(), flatkvRootDir)
cfg.SnapshotKeepRecent = 2
s, err := newCommitStoreWithWAL(t.Context(), cfg)
require.NoError(t, err)
require.NoError(t, s.LoadLatest())
defer s.Close()

// snapshot-40 is what a rollback that could not finish leaves behind. Pruning runs against a directory of
// its own so that remnant is the only thing separating this layout from a healthy one.
dir := t.TempDir()
planted := []int64{10, 20, 30, 40}
for _, v := range planted {
require.NoError(t, os.MkdirAll(filepath.Join(dir, snapshotName(v)), 0750))
}

require.Equal(t, 0, s.pruneSnapshots(dir, 30),
"only 10 and 20 sit below the current version, and KeepRecent=2 covers both")

var remaining []int64
require.NoError(t, traverseSnapshots(dir, true, func(v int64) (bool, error) {
remaining = append(remaining, v)
return false, nil
}))
require.Equal(t, planted, remaining,
"snapshot-40 must not take a keep slot and evict snapshot-10, which rollback still needs as a base")
}

// =============================================================================
// Orphan snapshot recovery
// =============================================================================
Expand Down Expand Up @@ -1185,6 +1214,87 @@ func TestRollbackRemovesPostTargetSnapshots(t *testing.T) {
require.NoError(t, s.Close())
}

func TestRemoveSnapshotsAboveKeepsTargetAndBelow(t *testing.T) {
dir := t.TempDir()
for _, v := range []int64{3, 5, 7, 9} {
require.NoError(t, os.MkdirAll(filepath.Join(dir, snapshotName(v)), 0750))
}

require.NoError(t, removeSnapshotsAbove(dir, 5))

var remaining []int64
require.NoError(t, traverseSnapshots(dir, true, func(v int64) (bool, error) {
remaining = append(remaining, v)
return false, nil
}))
require.Equal(t, []int64{3, 5}, remaining, "the snapshot sitting on the target is kept")
}

func TestRemoveSnapshotsAboveReportsEveryFailure(t *testing.T) {
if os.Geteuid() == 0 {
t.Skip("running as root, which is not stopped by the directory permissions this test relies on")
}

dir := t.TempDir()
for _, v := range []int64{3, 7, 9} {
require.NoError(t, os.MkdirAll(filepath.Join(dir, snapshotName(v)), 0750))
}

// atomicRemoveDir renames the snapshot within this directory, so withholding write permission on it is
// what makes the removal fail. Restore it before t.TempDir's own cleanup, which runs after this one.
require.NoError(t, os.Chmod(dir, 0555))
t.Cleanup(func() { _ = os.Chmod(dir, 0750) })

err := removeSnapshotsAbove(dir, 5)
require.Error(t, err, "a snapshot above the target that survives must not be reported as a clean rollback")
require.Contains(t, err.Error(), "remove snapshot 7", "the first failure must be named")
require.Contains(t, err.Error(), "remove snapshot 9",
"failing on 7 must not hide 9: the caller reconciles from the whole list, not the first name")
}

// TestRollbackReportsUnremovableSnapshotWithoutRewinding pins the ordering that makes the error safe to act
// on. Removing snapshots above the target runs before the WAL is pruned, so a failure there means the
// rollback did not take effect and every caller that skips its post-rollback bookkeeping on error is right
// to — `seid rollback` most of all, since it rewinds the app before Tendermint.
func TestRollbackReportsUnremovableSnapshotWithoutRewinding(t *testing.T) {
if os.Geteuid() == 0 {
t.Skip("running as root, which is not stopped by the directory permissions this test relies on")
}

dir := t.TempDir()
cfg := config.DefaultTestConfig(t)
cfg.DataDir = filepath.Join(dir, flatkvRootDir)
s, err := newCommitStoreWithWAL(t.Context(), cfg)
require.NoError(t, err)
require.NoError(t, s.LoadLatest())
defer func() { _ = s.Close() }()

for i := 0; i < 3; i++ {
commitStorageEntry(t, s, ktype.Address{byte(i + 1)}, ktype.Slot{byte(i + 1)}, []byte{byte(i + 1)})
}
require.NoError(t, s.WriteSnapshot(""))
for i := 3; i < 6; i++ {
commitStorageEntry(t, s, ktype.Address{byte(i + 1)}, ktype.Slot{byte(i + 1)}, []byte{byte(i + 1)})
}
require.NoError(t, s.WriteSnapshot("")) // snapshot-6, above the rollback target below

// atomicRemoveDir renames snapshot-6 onto this trash name before unlinking it, so an undeletable
// directory already sitting there fails that rename. Restore permissions before t.TempDir's own cleanup,
// which runs after this one.
blocker := filepath.Join(cfg.DataDir, snapshotName(6)+removingSuffix)

@seidroid seidroid Bot Aug 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

require.NoError(t, os.MkdirAll(blocker, 0750))
require.NoError(t, os.WriteFile(filepath.Join(blocker, "occupied"), []byte("x"), 0600))
require.NoError(t, os.Chmod(blocker, 0555))
t.Cleanup(func() { _ = os.Chmod(blocker, 0750) })

err = s.Rollback(5)
require.Error(t, err, "Rollback must not report success while a snapshot above the target is still on disk")
require.Contains(t, err.Error(), "remove snapshot 6")

require.Contains(t, walBlockNumbers(t, s), uint64(6),
"the WAL must be untouched, so a restart can replay back to the old tail and the rollback be retried")
}

// =============================================================================
// updateCurrentSymlink
// =============================================================================
Expand Down
Loading