Concurrent snapshot cleanup can turn an already-removed file into a fatal purge error

Bug report criteria

• This report concerns a correctness/availability defect, not a security vulnerability report.
• This is not a support request.
• I have read the bug reporting guidelines.
• I checked existing issues and the FAQ. The related reports below concern different failure conditions.

What happened?

The periodic snapshot DB purger and Snapshotter.ReleaseSnapDBs can remove the same obsolete .snap.db file. If snapshot release removes a file after the purger lists it, the purger's subsequent os.Remove returns ENOENT; the worker reports the error and exits. EtcdServer.purgeFile receives it and calls Fatal("failed to purge snap db file").

This is reproducible against main at d8a03ef08fb958516404b6063365ecc6dbfc0905. The relevant paths are:

• purgeFile lists candidates before removing them and exits on any removal error.
• storage.Release calls ReleaseSnapDBs. Its storage mutex does not serialize with the independent periodic purger. ReleaseSnapDBs already tolerates a file that another cleanup removed.
• The server's purge-error consumer treats the resulting error as fatal.

The proposed hardening would prevent this benign concurrent deletion from terminating cleanup and taking a member out of service. I have not measured production incidence or cluster-wide downtime.

What did you expect to happen?

When an obsolete candidate has already been removed and the snapshot directory remains readable, cleanup should continue through the current pass and subsequent cycles. Missing or inaccessible directories and other filesystem failures should still follow the existing error path.

How can we reproduce it (as minimally and precisely as possible)?

The test below forces this execution sequence using the existing logger hook:

1. Create snapshot DB files with indices 1 through 8; configure periodic cleanup to retain five.
2. Pause the periodic worker after it has listed the files and removed file 1.
3. Call the real ReleaseSnapDBs with snapshot index 4; it removes obsolete files 2 and 3.
4. Create file 9 and resume the periodic worker. Unmodified code reports ENOENT when it attempts to remove file 2 and exits.
5. With the proposed change, the same test verifies cleanup continues into another cycle and retains files 5 through 9.

Save the following as server/etcdserver/api/snap/purge_snapshot_race_repro_test.go in a checkout of the commit above. From the server directory, run:

go test -v -race -count=1 -timeout=120s ./etcdserver/api/snap -run '^TestPurgeSnapDBsDuringRelease$'

The unmodified source fails with the removal error shown below. The test uses a 1 ms purge interval to exercise the next cycle quickly; the production interval is 30 seconds. It supplies a controlled directory state and does not create a live cluster.

// Copyright 2015 The etcd Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
//     http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package snap

import (
	"fmt"
	"os"
	"path/filepath"
	"sync"
	"testing"
	"time"

	"github.com/stretchr/testify/require"
	"go.uber.org/zap"
	"go.uber.org/zap/zapcore"
	"go.uber.org/zap/zaptest"

	"go.etcd.io/etcd/client/pkg/v3/fileutil"
	"go.etcd.io/raft/v3/raftpb"
)

// Snapshot release can remove a file already selected by the periodic purger.
func TestPurgeSnapDBsDuringRelease(t *testing.T) {
	dir := t.TempDir()
	writeSnapshot := func(index uint64) {
		t.Helper()
		name := filepath.Join(dir, fmt.Sprintf("%016x.snap.db", index))
		require.NoError(t, os.WriteFile(name, []byte("snap file\n"), 0o600))
	}
	for index := uint64(1); index <= 8; index++ {
		writeSnapshot(index)
	}

	paused, resume, progressed := make(chan struct{}), make(chan struct{}), make(chan struct{})
	var resumeOnce sync.Once
	resumePurge := func() { resumeOnce.Do(func() { close(resume) }) }
	purged := 0
	lg := zaptest.NewLogger(t).WithOptions(zap.Hooks(func(entry zapcore.Entry) error {
		if entry.Message == "purged" {
			purged++
			if purged == 1 {
				close(paused)
				<-resume
			}
			if purged == 4 {
				close(progressed)
			}
		}
		return nil
	}))
	stop := make(chan struct{})
	done, errc := fileutil.PurgeFileWithoutFlock(lg, dir, "snap.db", 5, time.Millisecond, stop)
	t.Cleanup(func() {
		close(stop)
		resumePurge()
		select {
		case <-done:
		case <-time.After(5 * time.Second):
			t.Error("purger did not stop")
		}
	})
	select {
	case <-paused:
	case err := <-errc:
		t.Fatalf("purger failed before snapshot release: %v", err)
	case <-time.After(5 * time.Second):
		t.Fatal("purger did not select the old snapshots")
	}

	ss := New(zaptest.NewLogger(t), dir)
	require.NoError(t, ss.ReleaseSnapDBs(&raftpb.Snapshot{Metadata: &raftpb.SnapshotMetadata{Index: new(uint64(4))}}))
	writeSnapshot(9)
	resumePurge()
	select {
	case <-progressed:
	case err := <-errc:
		t.Fatalf("concurrent snapshot release stopped the purger: %v", err)
	case <-time.After(5 * time.Second):
		t.Fatal("purger did not resume cleanup")
	}
	names, err := fileutil.ReadDir(dir)
	require.NoError(t, err)
	var want []string
	for index := uint64(5); index <= 9; index++ {
		want = append(want, fmt.Sprintf("%016x.snap.db", index))
	}
	require.Equal(t, want, names)
}

Anything else we need to know?

Proposed hardening plan

1. In purgeFile, recognize os.IsNotExist from removing a selected candidate. Check ReadDir(dirname) before treating that candidate as already cleaned, so a missing or inaccessible parent is not silently accepted.
2. Preserve the current handling of other errors, WAL lock barriers, retention rules, cancellation, and handle cleanup. Merely lowering the server's log severity would leave the purge worker stopped.
3. Add the concurrent snapshot-release regression and helper controls for a missing parent, permission denial, an undeletable path, and continued cleanup. Retain the existing lock/retention tests.
4. Validate the scoped change across the client/pkg and server modules and review whether a multi-member snapshot/restart test is needed before merging. General recovery or retries for persistent storage failures remain separate work.

The local production change is seven added lines and one removed line:

diff --git a/client/pkg/fileutil/purge.go b/client/pkg/fileutil/purge.go
index e40bfd5..7d3ab9c 100644
--- a/client/pkg/fileutil/purge.go
+++ b/client/pkg/fileutil/purge.go
@@ -73,7 +73,13 @@ func purgeFile(lg *zap.Logger, dirname string, suffix string, max uint, interval
 						break
 					}
 				}
-				if err = os.Remove(f); err != nil {
+				if err = os.Remove(f); os.IsNotExist(err) {
+					// Another cleanup may have removed the file, but not the directory.
+					if _, dirErr := ReadDir(dirname); dirErr == nil {
+						err = nil
+					}
+				}
+				if err != nil {
 					if flock {
 						if closeErr := l.Close(); closeErr != nil {
 							lg.Error("failed to unlock/close", zap.String("path", l.Name()), zap.Error(closeErr))

Verification

On the main commit above, using Go 1.27.1 on macOS arm64:

• Unmodified source: the standalone regression fails with the expected ENOENT.
• With the diff above: the same command with -count=100 passes under the race detector; git diff --check passes.

Earlier validation on 21307b36894e497aca972ca01b641ae41c02e3e0 also covered the helper's filesystem-error controls, an actual server purge-consumer probe, full client/pkg and server race suites, builds, and vet. The first full server run failed TestWatchRestore/NoResync/zero_revision; ten targeted repetitions against the unchanged baseline and the full rerun passed, but that first failure's cause was not established. Linux amd64 package and repeated regression tests also passed in an emulated container as a non-root user, without the Linux race detector. Those broader runs have not been repeated on the newer commit.

This report establishes the filesystem interleaving and resulting fatal error path. It does not claim a live-cluster restart/recovery test or a measured frequency of deployed failures.

Related work

• Follow-up to the hardening discussion in #22447 and #22452, which fixed handle cleanup on a removal error.
• #18055 and #22046 concern deleting snapshots before they have been applied. This proposal concerns an already-obsolete file removed by two cleanup paths and does not claim to fix that separate case.
• #18123 reported a directory permission error. The proposed change preserves errors in that class.

Etcd version (please run commands below)

Source-based reproduction at main commit d8a03ef08fb958516404b6063365ecc6dbfc0905 using go version go1.27.1 darwin/arm64. No running etcd or etcdctl binary was used for the standalone reproduction, so there is no runtime version output to attach.

Etcd configuration (command line flags or environment variables)

No running cluster. The test invokes the real snapshot purger and snapshot release implementations, retains five files, and uses a 1 ms interval for the controlled schedule.

Etcd debug information

Not applicable to this package-level reproduction; no member list or endpoint status was collected.

Relevant log output

concurrent snapshot release stopped the purger: remove .../0000000000000002.snap.db: no such file or directory

AI disclosure: Codex assisted with orchestrating a proprietary static analysis tool and preparing structured regression tests.