diff --git a/sei-db/state_db/sc/flatkv/snapshot.go b/sei-db/state_db/sc/flatkv/snapshot.go index 8477aed430..ce8052ce17 100644 --- a/sei-db/state_db/sc/flatkv/snapshot.go +++ b/sei-db/state_db/sc/flatkv/snapshot.go @@ -527,6 +527,11 @@ func (s *CommitStore) WriteSnapshot(_ string) (err error) { // 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. +// // Does nothing when config.ExternalPruning is set, which hands retention to the // StorageGarbageCollector and its by-block-height PruneSnapshots. func (s *CommitStore) pruneSnapshotsByCount(dir string, currentVersion int64) int { @@ -543,12 +548,15 @@ func (s *CommitStore) pruneSnapshotsByCount(dir string, currentVersion int64) in 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 @@ -635,9 +643,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) @@ -673,6 +682,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 + } + if err := s.open(); err != nil { return fmt.Errorf("open for rollback: %w", err) } @@ -714,21 +727,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 +// 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. Skipped when there is no snapshot to truncate against. // diff --git a/sei-db/state_db/sc/flatkv/snapshot_test.go b/sei-db/state_db/sc/flatkv/snapshot_test.go index e727d35a80..12d08582a7 100644 --- a/sei-db/state_db/sc/flatkv/snapshot_test.go +++ b/sei-db/state_db/sc/flatkv/snapshot_test.go @@ -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.pruneSnapshotsByCount(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 // ============================================================================= @@ -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) + 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 // =============================================================================