Make FSStore.Delete* return ErrNotFound for missing files

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, &ethereum.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")
+}

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) {

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