From 36a59905b157fe5890a81140a0f5d0f054bac828 Mon Sep 17 00:00:00 2001 From: "Masih H. Derkani" Date: Thu, 27 Aug 2026 18:04:58 +0100 Subject: [PATCH 1/5] Validate snapshots before publication Reject malformed node hashes before writing snapshot records and validate completed snapshots before updating the current symlink. Add regression coverage for partial leaf records and corrupt snapshot publication. --- sei-db/state_db/sc/memiavl/db.go | 79 +++++++++++-------- sei-db/state_db/sc/memiavl/db_rewrite_test.go | 41 ++++++++++ sei-db/state_db/sc/memiavl/snapshot.go | 8 ++ .../sc/memiavl/snapshot_pipeline_test.go | 22 ++++++ 4 files changed, 118 insertions(+), 32 deletions(-) diff --git a/sei-db/state_db/sc/memiavl/db.go b/sei-db/state_db/sc/memiavl/db.go index ddd18f675c..71217440c2 100644 --- a/sei-db/state_db/sc/memiavl/db.go +++ b/sei-db/state_db/sc/memiavl/db.go @@ -771,6 +771,9 @@ func (db *DB) RewriteSnapshot(ctx context.Context) error { // Check if snapshot already exists if info, err := os.Stat(targetPath); err == nil { if info.IsDir() { + if err := db.validateSnapshot(ctx, targetPath); err != nil { + return fmt.Errorf("existing snapshot %q is invalid: %w", snapshotDir, err) + } logger.Info("snapshot already exists, skipping", "snapshot_dir", snapshotDir, "version", db.lastCommitInfo.Version) @@ -791,27 +794,23 @@ func (db *DB) RewriteSnapshot(ctx context.Context) error { writeElapsed := time.Since(writeStart).Seconds() if err != nil { - logger.Error("snapshot write failed, cleaning up temporary directory", - "tmpDir", tmpDir, - "error", err, - ) - cleanupErr := os.RemoveAll(path) - if cleanupErr != nil { - logger.Error("failed to clean up temporary snapshot directory", - "tmpDir", tmpDir, - "cleanup_error", cleanupErr, - ) - } else { - logger.Debug("temporary snapshot directory cleaned up successfully", - "tmpDir", tmpDir, - ) - } - return errorutils.Join(err, cleanupErr) + return cleanupFailedSnapshotRewrite(path, tmpDir, "snapshot write failed", err) + } + + if err := db.publishSnapshot(ctx, path, targetPath, snapshotDir); err != nil { + return cleanupFailedSnapshotRewrite(path, tmpDir, "snapshot publication failed", err) } logger.Info("snapshot rewrite completed", "duration_sec", writeElapsed) + return nil +} + +// publishSnapshot validates and publishes a completed snapshot. +func (db *DB) publishSnapshot(ctx context.Context, path, targetPath, snapshotDir string) error { + if err := db.validateSnapshot(ctx, path); err != nil { + return fmt.Errorf("validate temporary snapshot: %w", err) + } - // Rename temporary directory to final location if err := os.Rename(path, targetPath); err != nil { // An existing snapshot- directory (from a prior atomic rename) can be // used; drop our redundant temp rather than failing this rewrite. Only a @@ -821,6 +820,9 @@ func (db *DB) RewriteSnapshot(ctx context.Context) error { if info, statErr := os.Stat(targetPath); statErr != nil || !info.IsDir() { return fmt.Errorf("snapshot path %q exists but is not a usable directory: %w", targetPath, err) } + if validationErr := db.validateSnapshot(ctx, targetPath); validationErr != nil { + return fmt.Errorf("existing snapshot %q is invalid: %w", targetPath, validationErr) + } logger.Info("reusing existing snapshot directory, dropping redundant temp", "snapshotDir", snapshotDir, ) @@ -829,26 +831,39 @@ func (db *DB) RewriteSnapshot(ctx context.Context) error { } return updateCurrentSymlink(db.dir, snapshotDir) } - logger.Error("failed to rename snapshot directory, cleaning up", + return fmt.Errorf("rename snapshot directory to %q: %w", targetPath, err) + } + + return updateCurrentSymlink(db.dir, snapshotDir) +} + +func (db *DB) validateSnapshot(ctx context.Context, path string) error { + opts := db.opts + opts.SnapshotPrefetchThreshold = 0 + mtree, err := LoadMultiTree(ctx, path, opts) + if err != nil { + return err + } + return mtree.Close() +} + +func cleanupFailedSnapshotRewrite(path, tmpDir, operation string, err error) error { + logger.Error(operation+", cleaning up temporary directory", + "tmpDir", tmpDir, + "error", err, + ) + cleanupErr := os.RemoveAll(path) + if cleanupErr != nil { + logger.Error("failed to clean up temporary snapshot directory", "tmpDir", tmpDir, - "targetDir", snapshotDir, - "error", err, + "cleanup_error", cleanupErr, ) - // Clean up temporary directory on rename failure - if cleanupErr := os.RemoveAll(path); cleanupErr != nil { - logger.Error("failed to clean up temporary snapshot directory after rename failure", - "tmpDir", tmpDir, - "cleanup_error", cleanupErr, - ) - return errorutils.Join(err, cleanupErr) - } - logger.Info("temporary snapshot directory cleaned up after rename failure", + } else { + logger.Debug("temporary snapshot directory cleaned up successfully", "tmpDir", tmpDir, ) - return err } - - return updateCurrentSymlink(db.dir, snapshotDir) + return errorutils.Join(err, cleanupErr) } func (db *DB) Reload() error { diff --git a/sei-db/state_db/sc/memiavl/db_rewrite_test.go b/sei-db/state_db/sc/memiavl/db_rewrite_test.go index 7cc75446d9..568fd724ba 100644 --- a/sei-db/state_db/sc/memiavl/db_rewrite_test.go +++ b/sei-db/state_db/sc/memiavl/db_rewrite_test.go @@ -86,3 +86,44 @@ func TestLoadMultiTreeWithPrefetchDisabled(t *testing.T) { tree := db2.TreeByName("test") require.NotNil(t, tree) } + +func TestPublishSnapshotRejectsCorruptCandidate(t *testing.T) { + dir := t.TempDir() + db, err := OpenDB(0, Options{ + Dir: dir, + CreateIfMissing: true, + InitialStores: []string{"test"}, + }) + require.NoError(t, err) + t.Cleanup(func() { require.NoError(t, db.Close()) }) + + require.NoError(t, db.ApplyChangeSets([]*proto.NamedChangeSet{{ + Name: "test", + Changeset: proto.ChangeSet{Pairs: []*proto.KVPair{{ + Key: []byte("key"), + Value: []byte("value"), + }}}, + }})) + _, err = db.Commit() + require.NoError(t, err) + + snapshotDir := snapshotName(db.Version()) + tmpPath := filepath.Join(dir, snapshotDir+"-tmp") + targetPath := filepath.Join(dir, snapshotDir) + require.NoError(t, db.MultiTree.WriteSnapshot(context.Background(), tmpPath, db.snapshotWriterPool)) + + leavesPath := filepath.Join(tmpPath, "test", FileNameLeaves) + leaves, err := os.OpenFile(leavesPath, os.O_WRONLY|os.O_APPEND, 0) + require.NoError(t, err) + _, err = leaves.Write(make([]byte, SizeLeafWithoutHash)) + require.NoError(t, err) + require.NoError(t, leaves.Close()) + + err = db.publishSnapshot(context.Background(), tmpPath, targetPath, snapshotDir) + require.ErrorContains(t, err, "corrupted snapshot, leaves file size") + require.NoDirExists(t, targetPath) + + current, err := os.Readlink(currentPath(dir)) + require.NoError(t, err) + require.Equal(t, snapshotName(0), current) +} diff --git a/sei-db/state_db/sc/memiavl/snapshot.go b/sei-db/state_db/sc/memiavl/snapshot.go index d5434a5f07..f0d43ecb2b 100644 --- a/sei-db/state_db/sc/memiavl/snapshot.go +++ b/sei-db/state_db/sc/memiavl/snapshot.go @@ -932,6 +932,10 @@ func (w *snapshotWriter) writeLeaf(version uint32, key, value, hash []byte) erro // writeLeafDirect performs the actual leaf write (called by writer goroutine) func (w *snapshotWriter) writeLeafDirect(version uint32, keyLen uint32, keyOffset uint64, hash []byte) error { + if len(hash) != SizeHash { + return fmt.Errorf("invalid leaf hash size %d, expected %d", len(hash), SizeHash) + } + var buf [SizeLeafWithoutHash]byte binary.LittleEndian.PutUint32(buf[OffsetLeafVersion:], version) binary.LittleEndian.PutUint32(buf[OffsetLeafKeyLen:], keyLen) @@ -974,6 +978,10 @@ func (w *snapshotWriter) writeBranch(version, size uint32, height, preTrees uint // writeBranchDirect performs the actual branch write (called by writer goroutine) func (w *snapshotWriter) writeBranchDirect(version, size uint32, height, preTrees uint8, keyLeaf uint32, hash []byte) error { + if len(hash) != SizeHash { + return fmt.Errorf("invalid branch hash size %d, expected %d", len(hash), SizeHash) + } + var buf [SizeNodeWithoutHash]byte buf[OffsetHeight] = height buf[OffsetPreTrees] = preTrees diff --git a/sei-db/state_db/sc/memiavl/snapshot_pipeline_test.go b/sei-db/state_db/sc/memiavl/snapshot_pipeline_test.go index 891d585ada..a63a50da01 100644 --- a/sei-db/state_db/sc/memiavl/snapshot_pipeline_test.go +++ b/sei-db/state_db/sc/memiavl/snapshot_pipeline_test.go @@ -210,6 +210,28 @@ func TestSnapshotWriterErrorHandling(t *testing.T) { require.Error(t, err) } +func TestSnapshotWriterRejectsInvalidHashLengths(t *testing.T) { + t.Run("leaf", func(t *testing.T) { + var leaves bytes.Buffer + writer := &snapshotWriter{leavesWriter: &leaves} + + err := writer.writeLeafDirect(1, 1, 0, nil) + + require.ErrorContains(t, err, "invalid leaf hash size 0, expected 32") + require.Empty(t, leaves.Bytes()) + }) + + t.Run("branch", func(t *testing.T) { + var nodes bytes.Buffer + writer := &snapshotWriter{nodesWriter: &nodes} + + err := writer.writeBranchDirect(1, 2, 1, 0, 1, make([]byte, SizeHash-1)) + + require.ErrorContains(t, err, "invalid branch hash size 31, expected 32") + require.Empty(t, nodes.Bytes()) + }) +} + // TestEmptySnapshotWrite tests writing an empty snapshot func TestEmptySnapshotWrite(t *testing.T) { tree := New(0) From dd274e81a29e1741653e87848b208c47bbc59e30 Mon Sep 17 00:00:00 2001 From: "Masih H. Derkani" Date: Thu, 27 Aug 2026 18:47:37 +0100 Subject: [PATCH 2/5] Harden background snapshot failure --- sei-db/state_db/sc/memiavl/db.go | 13 ++++------ sei-db/state_db/sc/memiavl/db_test.go | 37 +++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 8 deletions(-) diff --git a/sei-db/state_db/sc/memiavl/db.go b/sei-db/state_db/sc/memiavl/db.go index 71217440c2..059f84d01f 100644 --- a/sei-db/state_db/sc/memiavl/db.go +++ b/sei-db/state_db/sc/memiavl/db.go @@ -488,18 +488,15 @@ func (db *DB) checkBackgroundSnapshotRewrite() error { db.snapshotRewriteCancelFunc = nil if !ok { - // channel was closed without sending a result - // Still prune old snapshots to prevent accumulation - go db.pruneSnapshots() - return errors.New("snapshot rewrite channel closed unexpectedly") + otelMetrics.NumSnapshotRewriteAttempts.Add(context.Background(), 1, metric.WithAttributes(attribute.String("success", "false"))) + logger.Error("snapshot rewrite channel closed unexpectedly; keeping current snapshot") + return nil } if result.mtree == nil { - // background snapshot rewrite failed otelMetrics.NumSnapshotRewriteAttempts.Add(context.Background(), 1, metric.WithAttributes(attribute.String("success", "false"))) - // Still prune old snapshots to prevent accumulation - go db.pruneSnapshots() - return fmt.Errorf("background snapshot rewriting failed: %w", result.err) + logger.Error("background snapshot rewriting failed; keeping current snapshot", "error", result.err) + return nil } else { otelMetrics.NumSnapshotRewriteAttempts.Add(context.Background(), 1, metric.WithAttributes(attribute.String("success", "true"))) } diff --git a/sei-db/state_db/sc/memiavl/db_test.go b/sei-db/state_db/sc/memiavl/db_test.go index 3cddd00f77..8c2843f8a9 100644 --- a/sei-db/state_db/sc/memiavl/db_test.go +++ b/sei-db/state_db/sc/memiavl/db_test.go @@ -830,6 +830,43 @@ func TestFastCommit(t *testing.T) { require.NoError(t, db.Close()) } +func TestCommitContinuesAfterBackgroundSnapshotFailure(t *testing.T) { + dir := t.TempDir() + db, err := OpenDB(0, Options{ + Config: Config{ + SnapshotInterval: 1000, + }, + Dir: dir, + CreateIfMissing: true, + InitialStores: []string{"test"}, + }) + require.NoError(t, err) + t.Cleanup(func() { require.NoError(t, db.Close()) }) + + rewriteResult := make(chan snapshotResult, 1) + rewriteResult <- snapshotResult{err: context.Canceled} + close(rewriteResult) + db.snapshotRewriteChan = rewriteResult + db.snapshotRewriteCancelFunc = func() {} + + require.NoError(t, db.ApplyChangeSets([]*proto.NamedChangeSet{{ + Name: "test", + Changeset: proto.ChangeSet{Pairs: []*proto.KVPair{{ + Key: []byte("key"), + Value: []byte("value"), + }}}, + }})) + + version, err := db.Commit() + + require.NoError(t, err) + require.EqualValues(t, 1, version) + require.Nil(t, db.snapshotRewriteChan) + current, err := os.Readlink(currentPath(dir)) + require.NoError(t, err) + require.Equal(t, snapshotName(0), current) +} + func TestRepeatedApplyChangeSet(t *testing.T) { db, err := OpenDB(0, Options{ Config: Config{ From d41bff17d9db73bbf137671fb44e8693f2d0544d Mon Sep 17 00:00:00 2001 From: "Masih H. Derkani" Date: Fri, 28 Aug 2026 11:33:32 +0100 Subject: [PATCH 3/5] Address comments and fix CI issue --- sei-db/state_db/sc/memiavl/db.go | 94 ++++++++++--- sei-db/state_db/sc/memiavl/db_rewrite_test.go | 124 ++++++++++++++++++ sei-db/state_db/sc/memiavl/db_test.go | 37 ------ sei-db/state_db/sc/memiavl/multitree.go | 11 ++ sei-db/state_db/sc/memiavl/snapshot.go | 17 ++- 5 files changed, 223 insertions(+), 60 deletions(-) diff --git a/sei-db/state_db/sc/memiavl/db.go b/sei-db/state_db/sc/memiavl/db.go index 059f84d01f..aa230b74b1 100644 --- a/sei-db/state_db/sc/memiavl/db.go +++ b/sei-db/state_db/sc/memiavl/db.go @@ -1,6 +1,7 @@ package memiavl import ( + "bytes" "context" "errors" "fmt" @@ -488,15 +489,18 @@ func (db *DB) checkBackgroundSnapshotRewrite() error { db.snapshotRewriteCancelFunc = nil if !ok { - otelMetrics.NumSnapshotRewriteAttempts.Add(context.Background(), 1, metric.WithAttributes(attribute.String("success", "false"))) - logger.Error("snapshot rewrite channel closed unexpectedly; keeping current snapshot") - return nil + // channel was closed without sending a result + // Still prune old snapshots to prevent accumulation + go db.pruneSnapshots() + return errors.New("snapshot rewrite channel closed unexpectedly") } if result.mtree == nil { + // background snapshot rewrite failed otelMetrics.NumSnapshotRewriteAttempts.Add(context.Background(), 1, metric.WithAttributes(attribute.String("success", "false"))) - logger.Error("background snapshot rewriting failed; keeping current snapshot", "error", result.err) - return nil + // Still prune old snapshots to prevent accumulation + go db.pruneSnapshots() + return fmt.Errorf("background snapshot rewriting failed: %w", result.err) } else { otelMetrics.NumSnapshotRewriteAttempts.Add(context.Background(), 1, metric.WithAttributes(attribute.String("success", "true"))) } @@ -765,22 +769,31 @@ func (db *DB) RewriteSnapshot(ctx context.Context) error { snapshotDir := snapshotName(db.lastCommitInfo.Version) targetPath := filepath.Clean(filepath.Join(db.dir, snapshotDir)) - // Check if snapshot already exists - if info, err := os.Stat(targetPath); err == nil { - if info.IsDir() { - if err := db.validateSnapshot(ctx, targetPath); err != nil { - return fmt.Errorf("existing snapshot %q is invalid: %w", snapshotDir, err) - } - logger.Info("snapshot already exists, skipping", - "snapshot_dir", snapshotDir, - "version", db.lastCommitInfo.Version) - return nil - } else { + // A directory left by a prior attempt at this height is adopted when it + // holds the state this DB would publish; an invalid one is removed so this + // rewrite regenerates it instead of failing every future attempt. + if info, statErr := os.Stat(targetPath); statErr == nil { + if !info.IsDir() { // targetPath exists but is not a directory - this is unexpected logger.Error("snapshot path exists but is not a directory", "path", targetPath) return fmt.Errorf("snapshot path exists but is not a directory: %s", targetPath) } + validationErr := db.validateSnapshot(ctx, targetPath) + if validationErr == nil { + logger.Info("snapshot already exists, skipping", + "snapshot_dir", snapshotDir, + "version", db.lastCommitInfo.Version) + return nil + } + logger.Error("existing snapshot is invalid, removing and rewriting", + "path", targetPath, + "error", validationErr, + ) + if rmErr := os.RemoveAll(targetPath); rmErr != nil { + return fmt.Errorf("existing snapshot %q is invalid and could not be removed: %w", + targetPath, errorutils.Join(validationErr, rmErr)) + } } tmpDir := snapshotDir + "-tmp" @@ -818,7 +831,20 @@ func (db *DB) publishSnapshot(ctx context.Context, path, targetPath, snapshotDir return fmt.Errorf("snapshot path %q exists but is not a usable directory: %w", targetPath, err) } if validationErr := db.validateSnapshot(ctx, targetPath); validationErr != nil { - return fmt.Errorf("existing snapshot %q is invalid: %w", targetPath, validationErr) + // The freshly written temp already passed validation; it + // replaces the invalid directory. + logger.Error("existing snapshot is invalid, replacing with freshly written snapshot", + "path", targetPath, + "error", validationErr, + ) + if rmErr := os.RemoveAll(targetPath); rmErr != nil { + return fmt.Errorf("existing snapshot %q is invalid and could not be removed: %w", + targetPath, errorutils.Join(validationErr, rmErr)) + } + if renameErr := os.Rename(path, targetPath); renameErr != nil { + return fmt.Errorf("rename snapshot directory to %q: %w", targetPath, renameErr) + } + return updateCurrentSymlink(db.dir, snapshotDir) } logger.Info("reusing existing snapshot directory, dropping redundant temp", "snapshotDir", snapshotDir, @@ -834,14 +860,44 @@ func (db *DB) publishSnapshot(ctx context.Context, path, targetPath, snapshotDir return updateCurrentSymlink(db.dir, snapshotDir) } -func (db *DB) validateSnapshot(ctx context.Context, path string) error { +// validateSnapshot loads the snapshot at path and verifies it holds the state +// this DB would publish: the recorded version and every store's root hash must +// match lastCommitInfo. File sizes and record alignment are checked by the +// load; interior nodes and key data are not otherwise verified. +func (db *DB) validateSnapshot(ctx context.Context, path string) (returnErr error) { opts := db.opts opts.SnapshotPrefetchThreshold = 0 mtree, err := LoadMultiTree(ctx, path, opts) if err != nil { return err } - return mtree.Close() + defer func() { + returnErr = errorutils.Join(returnErr, mtree.Close()) + }() + + if mtree.Version() != db.lastCommitInfo.Version { + return fmt.Errorf("snapshot version %d does not match expected version %d", + mtree.Version(), db.lastCommitInfo.Version) + } + loaded := mtree.Trees() + if len(loaded) != len(db.lastCommitInfo.StoreInfos) { + return fmt.Errorf("snapshot has %d stores, expected %d", + len(loaded), len(db.lastCommitInfo.StoreInfos)) + } + rootHashes := make(map[string][]byte, len(loaded)) + for _, entry := range loaded { + rootHashes[entry.Name] = entry.RootHash() + } + for _, info := range db.lastCommitInfo.StoreInfos { + hash, ok := rootHashes[info.Name] + if !ok { + return fmt.Errorf("snapshot is missing store %q", info.Name) + } + if !bytes.Equal(hash, info.CommitId.Hash) { + return fmt.Errorf("snapshot store %q root hash does not match the expected commit hash", info.Name) + } + } + return nil } func cleanupFailedSnapshotRewrite(path, tmpDir, operation string, err error) error { diff --git a/sei-db/state_db/sc/memiavl/db_rewrite_test.go b/sei-db/state_db/sc/memiavl/db_rewrite_test.go index 568fd724ba..d375f4da5c 100644 --- a/sei-db/state_db/sc/memiavl/db_rewrite_test.go +++ b/sei-db/state_db/sc/memiavl/db_rewrite_test.go @@ -127,3 +127,127 @@ func TestPublishSnapshotRejectsCorruptCandidate(t *testing.T) { require.NoError(t, err) require.Equal(t, snapshotName(0), current) } + +// openCommittedDB opens a fresh DB with one store and one committed change, +// which is the smallest state that exercises the snapshot rewrite paths. +func openCommittedDB(t *testing.T) (*DB, string) { + t.Helper() + dir := t.TempDir() + db, err := OpenDB(0, Options{ + Dir: dir, + CreateIfMissing: true, + InitialStores: []string{"test"}, + }) + require.NoError(t, err) + t.Cleanup(func() { require.NoError(t, db.Close()) }) + + require.NoError(t, db.ApplyChangeSets([]*proto.NamedChangeSet{{ + Name: "test", + Changeset: proto.ChangeSet{Pairs: []*proto.KVPair{{ + Key: []byte("key"), + Value: []byte("value"), + }}}, + }})) + _, err = db.Commit() + require.NoError(t, err) + return db, dir +} + +// writeCorruptSnapshotDir plants a directory at path that fails validation. +func writeCorruptSnapshotDir(t *testing.T, path string) { + t.Helper() + require.NoError(t, os.MkdirAll(path, 0o750)) + require.NoError(t, os.WriteFile(filepath.Join(path, MetadataFileName), []byte("garbage"), 0o600)) +} + +func TestRewriteSnapshotSkipsValidExistingSnapshot(t *testing.T) { + db, dir := openCommittedDB(t) + require.NoError(t, db.RewriteSnapshot(context.Background())) + + targetPath := filepath.Join(dir, snapshotName(db.Version())) + before, err := os.Stat(filepath.Join(targetPath, MetadataFileName)) + require.NoError(t, err) + + require.NoError(t, db.RewriteSnapshot(context.Background())) + + after, err := os.Stat(filepath.Join(targetPath, MetadataFileName)) + require.NoError(t, err) + require.Equal(t, before.ModTime(), after.ModTime(), "a valid existing snapshot must be adopted, not rewritten") + require.NoDirExists(t, targetPath+"-tmp") +} + +func TestRewriteSnapshotReplacesCorruptExistingSnapshot(t *testing.T) { + db, dir := openCommittedDB(t) + snapshotDir := snapshotName(db.Version()) + targetPath := filepath.Join(dir, snapshotDir) + writeCorruptSnapshotDir(t, targetPath) + + require.NoError(t, db.RewriteSnapshot(context.Background())) + + require.NoError(t, db.validateSnapshot(context.Background(), targetPath)) + current, err := os.Readlink(currentPath(dir)) + require.NoError(t, err) + require.Equal(t, snapshotDir, current) +} + +func TestPublishSnapshotReplacesCorruptExistingTarget(t *testing.T) { + db, dir := openCommittedDB(t) + snapshotDir := snapshotName(db.Version()) + tmpPath := filepath.Join(dir, snapshotDir+"-tmp") + targetPath := filepath.Join(dir, snapshotDir) + require.NoError(t, db.MultiTree.WriteSnapshot(context.Background(), tmpPath, db.snapshotWriterPool)) + writeCorruptSnapshotDir(t, targetPath) + + require.NoError(t, db.publishSnapshot(context.Background(), tmpPath, targetPath, snapshotDir)) + + require.NoError(t, db.validateSnapshot(context.Background(), targetPath)) + require.NoDirExists(t, tmpPath) + current, err := os.Readlink(currentPath(dir)) + require.NoError(t, err) + require.Equal(t, snapshotDir, current) +} + +func TestPublishSnapshotAdoptsValidExistingTarget(t *testing.T) { + db, dir := openCommittedDB(t) + snapshotDir := snapshotName(db.Version()) + tmpPath := filepath.Join(dir, snapshotDir+"-tmp") + targetPath := filepath.Join(dir, snapshotDir) + require.NoError(t, db.MultiTree.WriteSnapshot(context.Background(), tmpPath, db.snapshotWriterPool)) + require.NoError(t, db.MultiTree.WriteSnapshot(context.Background(), targetPath, db.snapshotWriterPool)) + + require.NoError(t, db.publishSnapshot(context.Background(), tmpPath, targetPath, snapshotDir)) + + require.NoDirExists(t, tmpPath, "the redundant temp must be dropped when the target is adopted") + current, err := os.Readlink(currentPath(dir)) + require.NoError(t, err) + require.Equal(t, snapshotDir, current) +} + +func TestValidateSnapshotComparesCommitInfo(t *testing.T) { + db, dir := openCommittedDB(t) + require.NoError(t, db.RewriteSnapshot(context.Background())) + staleTarget := filepath.Join(dir, snapshotName(db.Version())) + + // Another commit moves lastCommitInfo past the published snapshot. + require.NoError(t, db.ApplyChangeSets([]*proto.NamedChangeSet{{ + Name: "test", + Changeset: proto.ChangeSet{Pairs: []*proto.KVPair{{ + Key: []byte("key"), + Value: []byte("value2"), + }}}, + }})) + _, err := db.Commit() + require.NoError(t, err) + + err = db.validateSnapshot(context.Background(), staleTarget) + require.ErrorContains(t, err, "does not match expected version") + + // A snapshot at the right version but holding different content must be + // rejected on its root hash. + require.NoError(t, db.RewriteSnapshot(context.Background())) + freshTarget := filepath.Join(dir, snapshotName(db.Version())) + require.NoError(t, db.validateSnapshot(context.Background(), freshTarget)) + db.lastCommitInfo.StoreInfos[0].CommitId.Hash = []byte("not the real root hash") + err = db.validateSnapshot(context.Background(), freshTarget) + require.ErrorContains(t, err, "root hash does not match") +} diff --git a/sei-db/state_db/sc/memiavl/db_test.go b/sei-db/state_db/sc/memiavl/db_test.go index 8c2843f8a9..3cddd00f77 100644 --- a/sei-db/state_db/sc/memiavl/db_test.go +++ b/sei-db/state_db/sc/memiavl/db_test.go @@ -830,43 +830,6 @@ func TestFastCommit(t *testing.T) { require.NoError(t, db.Close()) } -func TestCommitContinuesAfterBackgroundSnapshotFailure(t *testing.T) { - dir := t.TempDir() - db, err := OpenDB(0, Options{ - Config: Config{ - SnapshotInterval: 1000, - }, - Dir: dir, - CreateIfMissing: true, - InitialStores: []string{"test"}, - }) - require.NoError(t, err) - t.Cleanup(func() { require.NoError(t, db.Close()) }) - - rewriteResult := make(chan snapshotResult, 1) - rewriteResult <- snapshotResult{err: context.Canceled} - close(rewriteResult) - db.snapshotRewriteChan = rewriteResult - db.snapshotRewriteCancelFunc = func() {} - - require.NoError(t, db.ApplyChangeSets([]*proto.NamedChangeSet{{ - Name: "test", - Changeset: proto.ChangeSet{Pairs: []*proto.KVPair{{ - Key: []byte("key"), - Value: []byte("value"), - }}}, - }})) - - version, err := db.Commit() - - require.NoError(t, err) - require.EqualValues(t, 1, version) - require.Nil(t, db.snapshotRewriteChan) - current, err := os.Readlink(currentPath(dir)) - require.NoError(t, err) - require.Equal(t, snapshotName(0), current) -} - func TestRepeatedApplyChangeSet(t *testing.T) { db, err := OpenDB(0, Options{ Config: Config{ diff --git a/sei-db/state_db/sc/memiavl/multitree.go b/sei-db/state_db/sc/memiavl/multitree.go index 09380543e0..2ff8f642f8 100644 --- a/sei-db/state_db/sc/memiavl/multitree.go +++ b/sei-db/state_db/sc/memiavl/multitree.go @@ -88,10 +88,20 @@ func LoadMultiTree(ctx context.Context, dir string, opts Options) (*MultiTree, e treeMap := make(map[string]*Tree, len(entries)) treeNames := make([]string, 0, len(entries)) + // closeOpened releases the snapshots already opened when the load cannot + // complete, so a failed load does not leak their mmaps and file handles. + closeOpened := func() { + for _, tree := range treeMap { + if closeErr := tree.Close(); closeErr != nil { + logger.Error("failed to close partially loaded tree", "error", closeErr) + } + } + } for _, e := range entries { // Check for cancellation select { case <-ctx.Done(): + closeOpened() return nil, ctx.Err() default: } @@ -102,6 +112,7 @@ func LoadMultiTree(ctx context.Context, dir string, opts Options) (*MultiTree, e treeNames = append(treeNames, name) snapshot, err := OpenSnapshot(filepath.Join(dir, name), opts) if err != nil { + closeOpened() return nil, err } treeMap[name] = NewFromSnapshot(snapshot, opts) diff --git a/sei-db/state_db/sc/memiavl/snapshot.go b/sei-db/state_db/sc/memiavl/snapshot.go index f0d43ecb2b..4e7cd91f7c 100644 --- a/sei-db/state_db/sc/memiavl/snapshot.go +++ b/sei-db/state_db/sc/memiavl/snapshot.go @@ -930,10 +930,19 @@ func (w *snapshotWriter) writeLeaf(version uint32, key, value, hash []byte) erro } } +// checkNodeHash rejects a hash whose length is not the fixed 32-byte record +// size, so a malformed hash cannot produce a misaligned snapshot file. +func checkNodeHash(kind string, hash []byte) error { + if len(hash) != SizeHash { + return fmt.Errorf("invalid %s hash size %d, expected %d", kind, len(hash), SizeHash) + } + return nil +} + // writeLeafDirect performs the actual leaf write (called by writer goroutine) func (w *snapshotWriter) writeLeafDirect(version uint32, keyLen uint32, keyOffset uint64, hash []byte) error { - if len(hash) != SizeHash { - return fmt.Errorf("invalid leaf hash size %d, expected %d", len(hash), SizeHash) + if err := checkNodeHash("leaf", hash); err != nil { + return err } var buf [SizeLeafWithoutHash]byte @@ -978,8 +987,8 @@ func (w *snapshotWriter) writeBranch(version, size uint32, height, preTrees uint // writeBranchDirect performs the actual branch write (called by writer goroutine) func (w *snapshotWriter) writeBranchDirect(version, size uint32, height, preTrees uint8, keyLeaf uint32, hash []byte) error { - if len(hash) != SizeHash { - return fmt.Errorf("invalid branch hash size %d, expected %d", len(hash), SizeHash) + if err := checkNodeHash("branch", hash); err != nil { + return err } var buf [SizeNodeWithoutHash]byte From 641d2473862fd132089f7c190c06aca0bdb1b184 Mon Sep 17 00:00:00 2001 From: "Masih H. Derkani" Date: Fri, 28 Aug 2026 13:09:28 +0100 Subject: [PATCH 4/5] Address review comments --- sei-db/state_db/sc/memiavl/db.go | 66 ++++++++++++----- sei-db/state_db/sc/memiavl/db_rewrite_test.go | 73 +++++++++++++++++++ sei-db/state_db/sc/memiavl/multitree.go | 8 +- sei-db/state_db/sc/memiavl/snapshot.go | 12 +-- 4 files changed, 134 insertions(+), 25 deletions(-) diff --git a/sei-db/state_db/sc/memiavl/db.go b/sei-db/state_db/sc/memiavl/db.go index aa230b74b1..78f609943e 100644 --- a/sei-db/state_db/sc/memiavl/db.go +++ b/sei-db/state_db/sc/memiavl/db.go @@ -30,6 +30,12 @@ const LockFileName = "LOCK" var errReadOnly = errors.New("db is read-only") +// errCorruptedSnapshot classifies snapshot data that is structurally invalid, +// as opposed to environmental failures (cancellation, file-handle or memory +// exhaustion) that say nothing about the data. Deleting a snapshot directory +// is justified only for errors carrying this sentinel. +var errCorruptedSnapshot = errors.New("corrupted snapshot") + // DB implements DB-like functionalities on top of MultiTree: // - async snapshot rewriting // - Write-ahead-log @@ -786,12 +792,18 @@ func (db *DB) RewriteSnapshot(ctx context.Context) error { "version", db.lastCommitInfo.Version) return nil } - logger.Error("existing snapshot is invalid, removing and rewriting", + if !errors.Is(validationErr, errCorruptedSnapshot) { + // Cancellation and resource exhaustion say nothing about the + // directory's contents; deleting it here could strand the current + // symlink on a directory that was perfectly good. + return fmt.Errorf("validate existing snapshot %q: %w", targetPath, validationErr) + } + logger.Error("existing snapshot is corrupted, removing and rewriting", "path", targetPath, "error", validationErr, ) if rmErr := os.RemoveAll(targetPath); rmErr != nil { - return fmt.Errorf("existing snapshot %q is invalid and could not be removed: %w", + return fmt.Errorf("existing snapshot %q is corrupted and could not be removed: %w", targetPath, errorutils.Join(validationErr, rmErr)) } } @@ -831,14 +843,20 @@ func (db *DB) publishSnapshot(ctx context.Context, path, targetPath, snapshotDir return fmt.Errorf("snapshot path %q exists but is not a usable directory: %w", targetPath, err) } if validationErr := db.validateSnapshot(ctx, targetPath); validationErr != nil { + if !errors.Is(validationErr, errCorruptedSnapshot) { + // Cancellation and resource exhaustion say nothing about + // the directory's contents; deleting it here could strand + // the current symlink on a directory that was perfectly good. + return fmt.Errorf("validate existing snapshot %q: %w", targetPath, validationErr) + } // The freshly written temp already passed validation; it - // replaces the invalid directory. - logger.Error("existing snapshot is invalid, replacing with freshly written snapshot", + // replaces the corrupted directory. + logger.Error("existing snapshot is corrupted, replacing with freshly written snapshot", "path", targetPath, "error", validationErr, ) if rmErr := os.RemoveAll(targetPath); rmErr != nil { - return fmt.Errorf("existing snapshot %q is invalid and could not be removed: %w", + return fmt.Errorf("existing snapshot %q is corrupted and could not be removed: %w", targetPath, errorutils.Join(validationErr, rmErr)) } if renameErr := os.Rename(path, targetPath); renameErr != nil { @@ -861,14 +879,21 @@ func (db *DB) publishSnapshot(ctx context.Context, path, targetPath, snapshotDir } // validateSnapshot loads the snapshot at path and verifies it holds the state -// this DB would publish: the recorded version and every store's root hash must -// match lastCommitInfo. File sizes and record alignment are checked by the -// load; interior nodes and key data are not otherwise verified. +// this DB would publish: the recorded multi-tree version and every store's +// version and root hash must match lastCommitInfo. File sizes and record +// alignment are checked by the load; interior nodes and key data are not +// otherwise verified. Failures that prove the data is bad carry +// errCorruptedSnapshot; environmental failures do not. func (db *DB) validateSnapshot(ctx context.Context, path string) (returnErr error) { opts := db.opts opts.SnapshotPrefetchThreshold = 0 mtree, err := LoadMultiTree(ctx, path, opts) if err != nil { + if errors.Is(err, fs.ErrNotExist) { + // A published snapshot contains every file it references; a missing + // one is an incomplete or mangled directory, not a transient failure. + return fmt.Errorf("%w: %w", errCorruptedSnapshot, err) + } return err } defer func() { @@ -876,25 +901,30 @@ func (db *DB) validateSnapshot(ctx context.Context, path string) (returnErr erro }() if mtree.Version() != db.lastCommitInfo.Version { - return fmt.Errorf("snapshot version %d does not match expected version %d", - mtree.Version(), db.lastCommitInfo.Version) + return fmt.Errorf("%w: snapshot version %d does not match expected version %d", + errCorruptedSnapshot, mtree.Version(), db.lastCommitInfo.Version) } loaded := mtree.Trees() if len(loaded) != len(db.lastCommitInfo.StoreInfos) { - return fmt.Errorf("snapshot has %d stores, expected %d", - len(loaded), len(db.lastCommitInfo.StoreInfos)) + return fmt.Errorf("%w: snapshot has %d stores, expected %d", + errCorruptedSnapshot, len(loaded), len(db.lastCommitInfo.StoreInfos)) } - rootHashes := make(map[string][]byte, len(loaded)) + loadedTrees := make(map[string]*Tree, len(loaded)) for _, entry := range loaded { - rootHashes[entry.Name] = entry.RootHash() + loadedTrees[entry.Name] = entry.Tree } for _, info := range db.lastCommitInfo.StoreInfos { - hash, ok := rootHashes[info.Name] + tree, ok := loadedTrees[info.Name] if !ok { - return fmt.Errorf("snapshot is missing store %q", info.Name) + return fmt.Errorf("%w: snapshot is missing store %q", errCorruptedSnapshot, info.Name) + } + if tree.Version() != info.CommitId.Version { + return fmt.Errorf("%w: snapshot store %q version %d does not match expected version %d", + errCorruptedSnapshot, info.Name, tree.Version(), info.CommitId.Version) } - if !bytes.Equal(hash, info.CommitId.Hash) { - return fmt.Errorf("snapshot store %q root hash does not match the expected commit hash", info.Name) + if !bytes.Equal(tree.RootHash(), info.CommitId.Hash) { + return fmt.Errorf("%w: snapshot store %q root hash does not match the expected commit hash", + errCorruptedSnapshot, info.Name) } } return nil diff --git a/sei-db/state_db/sc/memiavl/db_rewrite_test.go b/sei-db/state_db/sc/memiavl/db_rewrite_test.go index d375f4da5c..7e8b97b100 100644 --- a/sei-db/state_db/sc/memiavl/db_rewrite_test.go +++ b/sei-db/state_db/sc/memiavl/db_rewrite_test.go @@ -223,6 +223,79 @@ func TestPublishSnapshotAdoptsValidExistingTarget(t *testing.T) { require.Equal(t, snapshotDir, current) } +// TestRewriteSnapshotKeepsSnapshotOnCancelledValidation pins the boundary of +// the self-heal: a validation failure that says nothing about the directory's +// contents, such as a cancelled context at shutdown, must not delete a +// published snapshot out from under the current symlink. +func TestRewriteSnapshotKeepsSnapshotOnCancelledValidation(t *testing.T) { + db, dir := openCommittedDB(t) + require.NoError(t, db.RewriteSnapshot(context.Background())) + snapshotDir := snapshotName(db.Version()) + + cancelled, cancel := context.WithCancel(context.Background()) + cancel() + err := db.RewriteSnapshot(cancelled) + + require.ErrorIs(t, err, context.Canceled) + require.DirExists(t, filepath.Join(dir, snapshotDir)) + current, err := os.Readlink(currentPath(dir)) + require.NoError(t, err) + require.Equal(t, snapshotDir, current) +} + +// TestLoadMultiTreeRejectsMetadataWithoutCommitInfo covers the metadata shape +// an unclean shutdown leaves behind: a file that unmarshals successfully but +// carries no commit info. Loading it must fail as corruption, not panic. +func TestLoadMultiTreeRejectsMetadataWithoutCommitInfo(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, MetadataFileName), []byte{}, 0o600)) + + _, err := LoadMultiTree(context.Background(), dir, Options{}) + + require.ErrorIs(t, err, errCorruptedSnapshot) + require.ErrorContains(t, err, "no commit info") +} + +func TestRewriteSnapshotReplacesSnapshotWithEmptyMetadata(t *testing.T) { + db, dir := openCommittedDB(t) + snapshotDir := snapshotName(db.Version()) + targetPath := filepath.Join(dir, snapshotDir) + require.NoError(t, os.MkdirAll(targetPath, 0o750)) + require.NoError(t, os.WriteFile(filepath.Join(targetPath, MetadataFileName), []byte{}, 0o600)) + + require.NoError(t, db.RewriteSnapshot(context.Background())) + + require.NoError(t, db.validateSnapshot(context.Background(), targetPath)) + current, err := os.Readlink(currentPath(dir)) + require.NoError(t, err) + require.Equal(t, snapshotDir, current) +} + +// TestValidateSnapshotRejectsStoreVersionSkew pins the per-store version +// comparison. An empty commit advances every version while leaving root hashes +// unchanged, so a directory mixing an old store with a new multi-tree metadata +// is caught only by comparing each store's own version. +func TestValidateSnapshotRejectsStoreVersionSkew(t *testing.T) { + db, dir := openCommittedDB(t) + require.NoError(t, db.RewriteSnapshot(context.Background())) + oldSnapshot := filepath.Join(dir, snapshotName(db.Version())) + + _, err := db.Commit() + require.NoError(t, err) + require.NoError(t, db.RewriteSnapshot(context.Background())) + newSnapshot := filepath.Join(dir, snapshotName(db.Version())) + + skewed := filepath.Join(dir, "skewed-snapshot") + require.NoError(t, os.CopyFS(skewed, os.DirFS(oldSnapshot))) + metadata, err := os.ReadFile(filepath.Join(newSnapshot, MetadataFileName)) + require.NoError(t, err) + require.NoError(t, os.WriteFile(filepath.Join(skewed, MetadataFileName), metadata, 0o600)) + + err = db.validateSnapshot(context.Background(), skewed) + require.ErrorIs(t, err, errCorruptedSnapshot) + require.ErrorContains(t, err, "does not match expected version") +} + func TestValidateSnapshotComparesCommitInfo(t *testing.T) { db, dir := openCommittedDB(t) require.NoError(t, db.RewriteSnapshot(context.Background())) diff --git a/sei-db/state_db/sc/memiavl/multitree.go b/sei-db/state_db/sc/memiavl/multitree.go index 2ff8f642f8..21fcb42644 100644 --- a/sei-db/state_db/sc/memiavl/multitree.go +++ b/sei-db/state_db/sc/memiavl/multitree.go @@ -628,7 +628,13 @@ func readMetadata(dir string) (*proto.MultiTreeMetadata, error) { } var metadata proto.MultiTreeMetadata if err := metadata.Unmarshal(bz); err != nil { - return nil, err + return nil, fmt.Errorf("%w: unmarshal metadata: %w", errCorruptedSnapshot, err) + } + if metadata.CommitInfo == nil { + // An empty or truncated metadata file unmarshals successfully but + // carries no commit info; reject it here so every load path returns a + // validation failure instead of dereferencing nil. + return nil, fmt.Errorf("%w: metadata has no commit info", errCorruptedSnapshot) } if metadata.CommitInfo.Version > math.MaxUint32 { return nil, fmt.Errorf("commit info version overflows uint32: %d", metadata.CommitInfo.Version) diff --git a/sei-db/state_db/sc/memiavl/snapshot.go b/sei-db/state_db/sc/memiavl/snapshot.go index 69e87d96a7..d339382b86 100644 --- a/sei-db/state_db/sc/memiavl/snapshot.go +++ b/sei-db/state_db/sc/memiavl/snapshot.go @@ -169,16 +169,16 @@ func OpenSnapshot(snapshotDir string, opts Options) (*Snapshot, error) { return nil, err } if len(bz) != SizeMetadata { - return nil, fmt.Errorf("wrong metadata file size, expcted: %d, found: %d", SizeMetadata, len(bz)) + return nil, fmt.Errorf("%w: wrong metadata file size, expcted: %d, found: %d", errCorruptedSnapshot, SizeMetadata, len(bz)) } magic := binary.LittleEndian.Uint32(bz) if magic != SnapshotFileMagic { - return nil, fmt.Errorf("invalid metadata file magic: %d", magic) + return nil, fmt.Errorf("%w: invalid metadata file magic: %d", errCorruptedSnapshot, magic) } format := binary.LittleEndian.Uint32(bz[4:]) if format != SnapshotFormat { - return nil, fmt.Errorf("unknown snapshot format: %d", format) + return nil, fmt.Errorf("%w: unknown snapshot format: %d", errCorruptedSnapshot, format) } version := binary.LittleEndian.Uint32(bz[8:]) @@ -216,12 +216,12 @@ func OpenSnapshot(snapshotDir string, opts Options) (*Snapshot, error) { // validate nodes length if len(nodes)%SizeNode != 0 { return nil, cleanupHandles( - fmt.Errorf("corrupted snapshot, nodes file size %d is not a multiple of %d", len(nodes), SizeNode), + fmt.Errorf("%w, nodes file size %d is not a multiple of %d", errCorruptedSnapshot, len(nodes), SizeNode), ) } if len(leaves)%SizeLeaf != 0 { return nil, cleanupHandles( - fmt.Errorf("corrupted snapshot, leaves file size %d is not a multiple of %d", len(leaves), SizeLeaf), + fmt.Errorf("%w, leaves file size %d is not a multiple of %d", errCorruptedSnapshot, len(leaves), SizeLeaf), ) } @@ -229,7 +229,7 @@ func OpenSnapshot(snapshotDir string, opts Options) (*Snapshot, error) { leavesLen := len(leaves) / SizeLeaf if (leavesLen > 0 && nodesLen+1 != leavesLen) || (leavesLen == 0 && nodesLen != 0) { return nil, cleanupHandles( - fmt.Errorf("corrupted snapshot, branch nodes size %d don't match leaves size %d", nodesLen, leavesLen), + fmt.Errorf("%w, branch nodes size %d don't match leaves size %d", errCorruptedSnapshot, nodesLen, leavesLen), ) } From 1c45d685d68c91dc958b1bdaacc36ef933804160 Mon Sep 17 00:00:00 2001 From: "Masih H. Derkani" Date: Fri, 28 Aug 2026 15:18:12 +0100 Subject: [PATCH 5/5] Fix blocker --- sei-db/state_db/sc/memiavl/db.go | 33 ++++--- sei-db/state_db/sc/memiavl/db_rewrite_test.go | 92 +++++++++++++++++++ sei-db/state_db/sc/memiavl/layout_native.go | 10 +- sei-db/state_db/sc/memiavl/multitree.go | 13 ++- 4 files changed, 126 insertions(+), 22 deletions(-) diff --git a/sei-db/state_db/sc/memiavl/db.go b/sei-db/state_db/sc/memiavl/db.go index 78f609943e..b342cbfd76 100644 --- a/sei-db/state_db/sc/memiavl/db.go +++ b/sei-db/state_db/sc/memiavl/db.go @@ -32,8 +32,9 @@ var errReadOnly = errors.New("db is read-only") // errCorruptedSnapshot classifies snapshot data that is structurally invalid, // as opposed to environmental failures (cancellation, file-handle or memory -// exhaustion) that say nothing about the data. Deleting a snapshot directory -// is justified only for errors carrying this sentinel. +// exhaustion) that say nothing about the data. Replacing a snapshot directory +// is justified only for errors carrying this sentinel, so every structural +// check on the snapshot load path must wrap it. var errCorruptedSnapshot = errors.New("corrupted snapshot") // DB implements DB-like functionalities on top of MultiTree: @@ -776,8 +777,7 @@ func (db *DB) RewriteSnapshot(ctx context.Context) error { targetPath := filepath.Clean(filepath.Join(db.dir, snapshotDir)) // A directory left by a prior attempt at this height is adopted when it - // holds the state this DB would publish; an invalid one is removed so this - // rewrite regenerates it instead of failing every future attempt. + // holds the state this DB would publish; a corrupted one is rewritten. if info, statErr := os.Stat(targetPath); statErr == nil { if !info.IsDir() { // targetPath exists but is not a directory - this is unexpected @@ -798,14 +798,14 @@ func (db *DB) RewriteSnapshot(ctx context.Context) error { // symlink on a directory that was perfectly good. return fmt.Errorf("validate existing snapshot %q: %w", targetPath, validationErr) } - logger.Error("existing snapshot is corrupted, removing and rewriting", + // Fall through and rewrite. The corrupted directory — possibly what the + // current symlink points at — is not deleted here: publishSnapshot + // replaces it only after a freshly written, validated temp exists, so a + // failed write or a crash in the window cannot leave current dangling. + logger.Error("existing snapshot is corrupted, rewriting", "path", targetPath, "error", validationErr, ) - if rmErr := os.RemoveAll(targetPath); rmErr != nil { - return fmt.Errorf("existing snapshot %q is corrupted and could not be removed: %w", - targetPath, errorutils.Join(validationErr, rmErr)) - } } tmpDir := snapshotDir + "-tmp" @@ -1411,10 +1411,19 @@ func atomicRemoveDir(path string) error { // createDBIfNotExist detects if db does not exist and try to initialize an empty one. func createDBIfNotExist(dir string, initialVersion uint32) error { _, err := os.Stat(filepath.Join(dir, "current", MetadataFileName)) - if err != nil && os.IsNotExist(err) { - return initEmptyDB(dir, initialVersion) + if err == nil || !os.IsNotExist(err) { + return nil } - return nil + // Stat through a dangling symlink reports not-exist, which is + // indistinguishable from a directory that was never initialized. A current + // link pointing at a missing snapshot is evidence of data loss; initializing + // here would silently reset the store to an empty version 0. + if _, lstatErr := os.Lstat(currentPath(dir)); lstatErr == nil { + return fmt.Errorf("current link at %q points to a missing snapshot; refusing to initialize an empty db over it", currentPath(dir)) + } else if !os.IsNotExist(lstatErr) { + return lstatErr + } + return initEmptyDB(dir, initialVersion) } func isSnapshotName(name string) bool { diff --git a/sei-db/state_db/sc/memiavl/db_rewrite_test.go b/sei-db/state_db/sc/memiavl/db_rewrite_test.go index 7e8b97b100..461592feda 100644 --- a/sei-db/state_db/sc/memiavl/db_rewrite_test.go +++ b/sei-db/state_db/sc/memiavl/db_rewrite_test.go @@ -243,6 +243,98 @@ func TestRewriteSnapshotKeepsSnapshotOnCancelledValidation(t *testing.T) { require.Equal(t, snapshotDir, current) } +// TestRewriteSnapshotKeepsCorruptTargetUntilReplacementExists pins the +// replacement ordering: a corrupted directory — possibly what current points +// at — must survive until a freshly written, validated temp exists to take its +// place. A write that fails after corruption was detected must leave it alone. +func TestRewriteSnapshotKeepsCorruptTargetUntilReplacementExists(t *testing.T) { + db, dir := openCommittedDB(t) + snapshotDir := snapshotName(db.Version()) + targetPath := filepath.Join(dir, snapshotDir) + writeCorruptSnapshotDir(t, targetPath) + + // Corruption detection reads the planted metadata before any context + // check, so the cancelled context fails the snapshot write that follows, + // not the validation. + cancelled, cancel := context.WithCancel(context.Background()) + cancel() + err := db.RewriteSnapshot(cancelled) + + require.Error(t, err) + require.DirExists(t, targetPath, "the corrupt directory must survive until a validated replacement exists") +} + +// TestPublishSnapshotKeepsTargetOnEnvironmentalValidationFailure mirrors +// TestRewriteSnapshotKeepsSnapshotOnCancelledValidation for the rename-conflict +// branch: an existing target whose validation fails for a reason that says +// nothing about its contents must be kept, not replaced. +func TestPublishSnapshotKeepsTargetOnEnvironmentalValidationFailure(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root bypasses file permissions, so an unreadable target cannot be simulated") + } + db, dir := openCommittedDB(t) + snapshotDir := snapshotName(db.Version()) + tmpPath := filepath.Join(dir, snapshotDir+"-tmp") + targetPath := filepath.Join(dir, snapshotDir) + require.NoError(t, db.MultiTree.WriteSnapshot(context.Background(), tmpPath, db.snapshotWriterPool)) + require.NoError(t, db.MultiTree.WriteSnapshot(context.Background(), targetPath, db.snapshotWriterPool)) + // An unreadable target fails validation with permission denied, which is + // environmental, not proof of corruption. + require.NoError(t, os.Chmod(targetPath, 0)) + t.Cleanup(func() { _ = os.Chmod(targetPath, 0o750) }) + + err := db.publishSnapshot(context.Background(), tmpPath, targetPath, snapshotDir) + + require.Error(t, err) + require.NotErrorIs(t, err, errCorruptedSnapshot) + require.DirExists(t, targetPath) +} + +// TestOpenDBRefusesDanglingCurrentLink pins startup behavior when the current +// link's snapshot has gone missing: that is evidence of data loss, and opening +// with CreateIfMissing must fail loudly rather than silently reinitialize an +// empty store at version 0. +func TestOpenDBRefusesDanglingCurrentLink(t *testing.T) { + dir := t.TempDir() + db, err := OpenDB(0, Options{ + Dir: dir, + CreateIfMissing: true, + InitialStores: []string{"test"}, + }) + require.NoError(t, err) + require.NoError(t, db.Close()) + + target, err := os.Readlink(currentPath(dir)) + require.NoError(t, err) + require.NoError(t, os.RemoveAll(filepath.Join(dir, target))) + + _, err = OpenDB(0, Options{ + Dir: dir, + CreateIfMissing: true, + InitialStores: []string{"test"}, + }) + require.ErrorContains(t, err, "refusing to initialize") +} + +// TestLoadMultiTreeRejectsNegativeInitialVersion covers corrupted metadata +// carrying a negative initial version, which panics in setInitialVersion if it +// survives the load. It must fail as corruption instead. +func TestLoadMultiTreeRejectsNegativeInitialVersion(t *testing.T) { + dir := t.TempDir() + metadata := proto.MultiTreeMetadata{ + CommitInfo: &proto.CommitInfo{Version: 1}, + InitialVersion: -1, + } + bz, err := metadata.Marshal() + require.NoError(t, err) + require.NoError(t, os.WriteFile(filepath.Join(dir, MetadataFileName), bz, 0o600)) + + _, err = LoadMultiTree(context.Background(), dir, Options{}) + + require.ErrorIs(t, err, errCorruptedSnapshot) + require.ErrorContains(t, err, "out of uint32 range") +} + // TestLoadMultiTreeRejectsMetadataWithoutCommitInfo covers the metadata shape // an unclean shutdown leaves behind: a file that unmarshals successfully but // carries no commit info. Loading it must fail as corruption, not panic. diff --git a/sei-db/state_db/sc/memiavl/layout_native.go b/sei-db/state_db/sc/memiavl/layout_native.go index 46a5bc88a5..2ca2c38a0f 100644 --- a/sei-db/state_db/sc/memiavl/layout_native.go +++ b/sei-db/state_db/sc/memiavl/layout_native.go @@ -3,7 +3,7 @@ package memiavl import ( - "errors" + "fmt" "unsafe" ) @@ -27,11 +27,11 @@ func NewNodes(buf []byte) (Nodes, error) { // check alignment and size of the buffer p := unsafe.Pointer(unsafe.SliceData(buf)) if uintptr(p)%unsafe.Alignof(nodeLayout{}) != 0 { - return Nodes{}, errors.New("input buffer is not aligned") + return Nodes{}, fmt.Errorf("%w: nodes buffer is not aligned", errCorruptedSnapshot) } size := int(unsafe.Sizeof(nodeLayout{})) if len(buf)%size != 0 { - return Nodes{}, errors.New("input buffer length is not correct") + return Nodes{}, fmt.Errorf("%w: nodes buffer length is not correct", errCorruptedSnapshot) } nodes := unsafe.Slice((*nodeLayout)(p), len(buf)/size) return Nodes{nodes}, nil @@ -82,11 +82,11 @@ func NewLeaves(buf []byte) (Leaves, error) { // check alignment and size of the buffer p := unsafe.Pointer(unsafe.SliceData(buf)) if uintptr(p)%unsafe.Alignof(leafLayout{}) != 0 { - return Leaves{}, errors.New("input buffer is not aligned") + return Leaves{}, fmt.Errorf("%w: leaves buffer is not aligned", errCorruptedSnapshot) } size := int(unsafe.Sizeof(leafLayout{})) if len(buf)%size != 0 { - return Leaves{}, errors.New("input buffer length is not correct") + return Leaves{}, fmt.Errorf("%w: leaves buffer length is not correct", errCorruptedSnapshot) } leaves := unsafe.Slice((*leafLayout)(p), len(buf)/size) return Leaves{leaves}, nil diff --git a/sei-db/state_db/sc/memiavl/multitree.go b/sei-db/state_db/sc/memiavl/multitree.go index 21fcb42644..5bb1e4abf8 100644 --- a/sei-db/state_db/sc/memiavl/multitree.go +++ b/sei-db/state_db/sc/memiavl/multitree.go @@ -636,11 +636,14 @@ func readMetadata(dir string) (*proto.MultiTreeMetadata, error) { // validation failure instead of dereferencing nil. return nil, fmt.Errorf("%w: metadata has no commit info", errCorruptedSnapshot) } - if metadata.CommitInfo.Version > math.MaxUint32 { - return nil, fmt.Errorf("commit info version overflows uint32: %d", metadata.CommitInfo.Version) - } - if metadata.InitialVersion > math.MaxUint32 { - return nil, fmt.Errorf("initial version overflows uint32: %d", metadata.InitialVersion) + // Both fields are int64 varints on disk, so corrupted metadata can carry + // negative values as easily as oversized ones; setInitialVersion panics on + // a negative initial version if it gets that far. + if metadata.CommitInfo.Version < 0 || metadata.CommitInfo.Version > math.MaxUint32 { + return nil, fmt.Errorf("%w: commit info version %d out of uint32 range", errCorruptedSnapshot, metadata.CommitInfo.Version) + } + if metadata.InitialVersion < 0 || metadata.InitialVersion > math.MaxUint32 { + return nil, fmt.Errorf("%w: initial version %d out of uint32 range", errCorruptedSnapshot, metadata.InitialVersion) } return &metadata, nil