diff --git a/pkg/server/service/indexer/retention_zombie_row_test.go b/pkg/server/service/indexer/retention_zombie_row_test.go new file mode 100644 index 0000000..58523c8 --- /dev/null +++ b/pkg/server/service/indexer/retention_zombie_row_test.go @@ -0,0 +1,124 @@ +package indexer + +import ( + "context" + "fmt" + "os" + "testing" + "time" + + "github.com/ethpandaops/beacon/pkg/human" + "github.com/ethpandaops/tracoor/pkg/server/ethereum" + "github.com/ethpandaops/tracoor/pkg/server/persistence" + "github.com/ethpandaops/tracoor/pkg/store" + "github.com/sirupsen/logrus" + "github.com/stretchr/testify/require" +) + +func newRetentionTestIndexer(t *testing.T) *Indexer { + t.Helper() + + ctx := context.Background() + + dbFile, err := os.CreateTemp("", "retention_zombie_row_*.db") + require.NoError(t, err) + dbPath := dbFile.Name() + dbFile.Close() + os.Remove(dbPath) + + t.Cleanup(func() { + os.Remove(dbPath) + os.Remove(dbPath + "-wal") + os.Remove(dbPath + "-shm") + }) + + db, err := persistence.NewIndexer("retention-zombie-row-test", logrus.New(), persistence.Config{ + DSN: fmt.Sprintf("file:%s?parseTime=True", dbPath), + DriverName: "sqlite", + }, persistence.DefaultOptions().SetMetricsEnabled(false)) + require.NoError(t, err) + require.NoError(t, db.Start(ctx)) + + basePath, err := os.MkdirTemp("", "retention_zombie_row_fs") + require.NoError(t, err) + t.Cleanup(func() { os.RemoveAll(basePath) }) + + st, err := store.NewFSStore("retention-zombie-row-test", logrus.New(), &store.FSStoreConfig{BasePath: basePath}, &store.Options{}) + require.NoError(t, err) + + conf := &Config{ + Retention: RetentionConfig{ + BeaconStates: human.Duration{Duration: time.Minute}, + }, + } + + idx, err := NewIndexer(ctx, logrus.New(), conf, db, st, ðereum.Config{}) + require.NoError(t, err) + + return idx +} + +// TestPurgeOldBeaconStates_DeletesDBRowWhenFileAlreadyMissing is a +// regression test for NM-W1-003. Before the fix, a beacon state row whose +// underlying file was already gone from disk (manual cleanup, a crash +// mid-delete, or one of a pair of duplicate rows sharing a location where a +// sibling delete already removed it) could never be purged: FSStore's +// DeleteBeaconState returned a raw *PathError that didn't satisfy +// errors.Is(err, store.ErrNotFound), so the retention loop treated it as a +// real failure, skipped the database delete, and retried forever. +func TestPurgeOldBeaconStates_DeletesDBRowWhenFileAlreadyMissing(t *testing.T) { + idx := newRetentionTestIndexer(t) + ctx := context.Background() + + // FetchedAt always arrives from a protobuf timestamp's AsTime(), which is + // always UTC-located; matching that here rather than using the server's + // local time.Now() keeps this test representative of production data. + oldFetchedAt := time.Now().UTC().Add(-time.Hour) + + require.NoError(t, idx.db.InsertBeaconState(ctx, &persistence.BeaconState{ + ID: "zombie-state", + Node: "some-node", + Network: "mainnet", + Slot: 100, + Epoch: 3, + StateRoot: "0xzombie", + FetchedAt: oldFetchedAt, + BeaconImplementation: "teku", + NodeVersion: "1.0.0", + // This location was never written to the FS store, standing in for + // a file that's already gone by the time retention gets to it. + Location: "beacon_state/already_gone.ssz", + })) + + require.NoError(t, idx.purgeOldBeaconStates(ctx)) + + count, err := idx.db.CountBeaconState(ctx, &persistence.BeaconStateFilter{}) + require.NoError(t, err) + require.Equal(t, int64(0), count, "expected the database row to be purged even though its file was already missing") +} + +// TestPurgeOldBeaconStates_LeavesRecentStatesAlone is a baseline check that +// the fix didn't change which rows are eligible for purging. +func TestPurgeOldBeaconStates_LeavesRecentStatesAlone(t *testing.T) { + idx := newRetentionTestIndexer(t) + ctx := context.Background() + + require.NoError(t, idx.db.InsertBeaconState(ctx, &persistence.BeaconState{ + ID: "recent-state", + Node: "some-node", + Network: "mainnet", + Slot: 200, + Epoch: 6, + StateRoot: "0xrecent", + FetchedAt: time.Now().UTC(), + BeaconImplementation: "teku", + NodeVersion: "1.0.0", + Location: "beacon_state/recent.ssz", + })) + + require.NoError(t, idx.purgeOldBeaconStates(ctx)) + + count, err := idx.db.CountBeaconState(ctx, &persistence.BeaconStateFilter{}) + require.NoError(t, err) + require.Equal(t, int64(1), count, "a state fetched within the retention window must not be purged") +} diff --git a/pkg/store/fs.go b/pkg/store/fs.go index b235915..b891834 100644 --- a/pkg/store/fs.go +++ b/pkg/store/fs.go @@ -97,7 +97,15 @@ func (s *FSStore) getFile(path string) (*[]byte, error) { } func (s *FSStore) removeFile(path string) error { - return os.Remove(path) + if err := os.Remove(path); err != nil { + if os.IsNotExist(err) { + return ErrNotFound + } + + return err + } + + return nil } func (s *FSStore) SaveBeaconState(ctx context.Context, params *SaveParams) (string, error) { diff --git a/pkg/store/fs_delete_not_found_test.go b/pkg/store/fs_delete_not_found_test.go new file mode 100644 index 0000000..be2aad6 --- /dev/null +++ b/pkg/store/fs_delete_not_found_test.go @@ -0,0 +1,75 @@ +package store_test + +import ( + "context" + "errors" + "os" + "testing" + + "github.com/ethpandaops/tracoor/pkg/store" + "github.com/sirupsen/logrus" + "github.com/stretchr/testify/require" +) + +// TestFSStoreDeleteMissingFileReturnsErrNotFound is a regression test for +// NM-W1-003: FSStore's Delete* methods used to return the raw *PathError +// from os.Remove when the target file didn't exist, rather than the +// package's store.ErrNotFound sentinel. Callers such as the retention +// watcher use errors.Is(err, store.ErrNotFound) to decide whether a missing +// file is fine to treat as "already gone" (and proceed to remove the +// database row) versus a real failure worth retrying. Since the FS backend +// never produced that sentinel, a missing file permanently blocked the +// corresponding database row from ever being cleaned up. +func TestFSStoreDeleteMissingFileReturnsErrNotFound(t *testing.T) { + basePath, err := os.MkdirTemp("", "fsstore_delete_not_found_test") + require.NoError(t, err) + + defer os.RemoveAll(basePath) + + fsStore, err := store.NewFSStore("test", logrus.New(), &store.FSStoreConfig{BasePath: basePath}, nil) + require.NoError(t, err) + + ctx := context.Background() + + deleters := map[string]func(context.Context, string) error{ + "DeleteBeaconState": fsStore.DeleteBeaconState, + "DeleteBeaconBlock": fsStore.DeleteBeaconBlock, + "DeleteBeaconBadBlock": fsStore.DeleteBeaconBadBlock, + "DeleteBeaconBadBlob": fsStore.DeleteBeaconBadBlob, + "DeleteExecutionBlockTrace": fsStore.DeleteExecutionBlockTrace, + "DeleteExecutionBadBlock": fsStore.DeleteExecutionBadBlock, + } + + for name, deleteFn := range deleters { + t.Run(name, func(t *testing.T) { + err := deleteFn(ctx, "does/not/exist.json") + require.Error(t, err) + require.True(t, errors.Is(err, store.ErrNotFound), "expected errors.Is(err, store.ErrNotFound) to be true, got: %v", err) + }) + } +} + +// TestFSStoreDeleteExistingFileStillSucceeds guards against the fix +// accidentally turning every delete into an error. +func TestFSStoreDeleteExistingFileStillSucceeds(t *testing.T) { + basePath, err := os.MkdirTemp("", "fsstore_delete_existing_test") + require.NoError(t, err) + + defer os.RemoveAll(basePath) + + fsStore, err := store.NewFSStore("test", logrus.New(), &store.FSStoreConfig{BasePath: basePath}, nil) + require.NoError(t, err) + + ctx := context.Background() + location := "beacon_state/present.json" + data := []byte(`{"a":"b"}`) + + _, err = fsStore.SaveBeaconState(ctx, &store.SaveParams{Data: &data, Location: location}) + require.NoError(t, err) + + require.NoError(t, fsStore.DeleteBeaconState(ctx, location)) + + exists, err := fsStore.Exists(ctx, location) + require.NoError(t, err) + require.False(t, exists) +}