raftengine/etcd: address second review round

- Fix data-loss risk: purge only runs on successful snapshot
  persistence (err == nil), not on ErrUnavailable/ErrCompacted
- Make purge best-effort: log warning instead of returning error,
  so purge failure does not block snapshot progression
- Fix unparam: remove unused maxSnap parameter from purgeOldSnapFiles,
  use defaultMaxSnapFiles constant directly
- Fix unparam: remove unused return value from createSnapFile test helper

Author: bootjp

internal/raftengine/etcd/engine.go

@@ -1320,8 +1320,8 @@ func (e *Engine) persistCreatedSnapshot(snap raftpb.Snapshot) error {
 	}
 
 	snapDir := filepath.Join(e.dataDir, snapDirName)
-	if purgeErr := purgeOldSnapFiles(snapDir, defaultMaxSnapFiles); purgeErr != nil {
-		return errors.Wrap(purgeErr, "purge old snap files")
+	if purgeErr := purgeOldSnapFiles(snapDir); purgeErr != nil {
+		slog.Warn("failed to purge old snap files", "error", purgeErr)
 	}
 	return nil
 }
@@ -2268,18 +2268,20 @@ func (e *Engine) persistLocalSnapshotPayload(index uint64, payload []byte) error
 	_, err = persistLocalSnapshotPayload(e.storage, e.persist, index, payload)
 	switch {
 	case err == nil:
+		snapDir := filepath.Join(e.dataDir, snapDirName)
+		if purgeErr := purgeOldSnapFiles(snapDir); purgeErr != nil {
+			slog.Warn("failed to purge old snap files", "error", purgeErr)
+		}
+		return nil
 	case errors.Is(err, etcdraft.ErrCompacted):
+		return nil
 	case errors.Is(err, etcdraft.ErrUnavailable):
+		return nil
 	case errors.Is(err, etcdraft.ErrSnapOutOfDate):
+		return nil
 	default:
 		return err
 	}
-
-	snapDir := filepath.Join(e.dataDir, snapDirName)
-	if purgeErr := purgeOldSnapFiles(snapDir, defaultMaxSnapFiles); purgeErr != nil {
-		return errors.Wrap(purgeErr, "purge old snap files")
-	}
-	return nil
 }
 
 func encodeReadContext(id uint64) []byte {

internal/raftengine/etcd/snapshot_spool_test.go

@@ -49,12 +49,11 @@ func TestCleanupStaleSnapshotSpoolsNonExistentDir(t *testing.T) {
 }
 
 // createSnapFile creates a fake .snap file with the etcd naming convention.
-func createSnapFile(t *testing.T, dir string, term, index uint64) string {
+func createSnapFile(t *testing.T, dir string, term, index uint64) {
 	t.Helper()
 	name := fmt.Sprintf("%016x-%016x.snap", term, index)
 	path := filepath.Join(dir, name)
 	require.NoError(t, os.WriteFile(path, []byte("fake"), 0o600))
-	return path
 }
 
 func TestPurgeOldSnapFiles(t *testing.T) {
@@ -69,7 +68,7 @@ func TestPurgeOldSnapFiles(t *testing.T) {
 	other := filepath.Join(dir, "db.tmp.12345")
 	require.NoError(t, os.WriteFile(other, []byte("x"), 0o600))
 
-	require.NoError(t, purgeOldSnapFiles(dir, defaultMaxSnapFiles))
+	require.NoError(t, purgeOldSnapFiles(dir))
 
 	entries, err := os.ReadDir(dir)
 	require.NoError(t, err)
@@ -99,7 +98,7 @@ func TestPurgeOldSnapFilesUnderLimit(t *testing.T) {
 	createSnapFile(t, dir, 1, 1000)
 	createSnapFile(t, dir, 1, 2000)
 
-	require.NoError(t, purgeOldSnapFiles(dir, defaultMaxSnapFiles))
+	require.NoError(t, purgeOldSnapFiles(dir))
 
 	entries, err := os.ReadDir(dir)
 	require.NoError(t, err)
@@ -108,5 +107,5 @@ func TestPurgeOldSnapFilesUnderLimit(t *testing.T) {
 
 func TestPurgeOldSnapFilesEmptyDir(t *testing.T) {
 	dir := t.TempDir()
-	require.NoError(t, purgeOldSnapFiles(dir, defaultMaxSnapFiles))
+	require.NoError(t, purgeOldSnapFiles(dir))
 }

internal/raftengine/etcd/wal_store.go

@@ -347,10 +347,10 @@ func persistLocalSnapshotPayload(storage *etcdraft.MemoryStorage, persist etcdst
 const defaultMaxSnapFiles = 3
 
 // purgeOldSnapFiles removes old .snap files from snapDir, keeping the most
-// recent maxSnap files. Snap file names encode term and index in hex and sort
-// lexicographically from oldest to newest, matching etcd's Snapshotter
-// convention.
-func purgeOldSnapFiles(snapDir string, maxSnap int) error {
+// recent defaultMaxSnapFiles files. Snap file names encode term and index in
+// hex and sort lexicographically from oldest to newest, matching etcd's
+// Snapshotter convention.
+func purgeOldSnapFiles(snapDir string) error {
 	entries, err := os.ReadDir(snapDir)
 	if err != nil {
 		return errors.WithStack(err)
@@ -363,7 +363,7 @@ func purgeOldSnapFiles(snapDir string, maxSnap int) error {
 		}
 	}
 
-	if len(snaps) <= maxSnap {
+	if len(snaps) <= defaultMaxSnapFiles {
 		return nil
 	}
 
@@ -372,7 +372,7 @@ func purgeOldSnapFiles(snapDir string, maxSnap int) error {
 	sort.Strings(snaps)
 
 	var combined error
-	for _, name := range snaps[:len(snaps)-maxSnap] {
+	for _, name := range snaps[:len(snaps)-defaultMaxSnapFiles] {
 		if removeErr := os.Remove(filepath.Join(snapDir, name)); removeErr != nil && !os.IsNotExist(removeErr) {
 			combined = errors.CombineErrors(combined, errors.WithStack(removeErr))
 		}