Stop snapshot purge when a snapshot cannot leave the local index (closes #337)

snapshot purge, and the purge step of snapshot create --prune, logged a
failed local index delete, then deleted that snapshot's metadata from
the destination store anyway and exited 0, leaving a local record whose
metadata was gone. It now stops at that snapshot with an error, before
touching its metadata, so the command exits non-zero; snapshots already
purged stay purged. This matches how the same loop already handles a
failed metadata delete.

Model: opus-5-5

README.md

@@ -339,7 +339,9 @@ local index alone, and still exits zero.
 
 **`snapshot purge`**: Remove old snapshots based on criteria. Retention is
 per-snapshot-name (`--keep-latest` keeps the latest of each name, not the
-latest globally).
+latest globally). If a snapshot cannot be removed from the local index, the
+command stops there, leaves that snapshot's metadata on the destination store
+and exits non-zero; `snapshot create --prune` fails the same way.
 * `--keep-latest`: Keep only the most recent snapshot of each name
 * `--older-than <duration>`: Remove snapshots older than duration (e.g. `30d`,
   `4w`, `6mo`, `1y`; `m` is minutes, `mo` is months)

internal/cli/entry_snapshot_purge_test.go

@@ -0,0 +1,90 @@
+package cli //nolint:testpackage // shares runEntry and the hermetic config helpers
+
+import (
+	"context"
+	"database/sql"
+	"os"
+	"path/filepath"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/vaultik/internal/database"
+	"sneak.berlin/go/vaultik/internal/snapshot"
+	"sneak.berlin/go/vaultik/internal/types"
+)
+
+// The `snapshot purge` command word and flags used below.
+const (
+	cmdPurge       = "purge"
+	flagKeepLatest = "--keep-latest"
+	flagForce      = "--force"
+)
+
+// TestEntrySnapshotPurgeKeepsMetadataWhenLocalIndexDeleteFails makes
+// removing a snapshot from the local index fail and checks that
+// `snapshot purge` exits non-zero and leaves that snapshot's metadata on
+// the destination store. It used to log the error, delete the metadata
+// anyway and exit 0.
+//
+//nolint:paralleltest // replaces os.Args, os.Stdout, os.Stderr and the xdg globals
+func TestEntrySnapshotPurgeKeepsMetadataWhenLocalIndexDeleteFails(t *testing.T) {
+	ctx := context.Background()
+	configPath := writeHermeticPruneConfig(t, false)
+
+	// writeHermeticPruneConfig puts the index and the store directory
+	// beside the config file.
+	dir := filepath.Dir(configPath)
+
+	// Two snapshots of the same name: --keep-latest purges the older.
+	olderStart := time.Date(2026, 5, 1, 9, 0, 0, 0, time.UTC)
+	newerStart := olderStart.Add(time.Hour)
+	olderID := "test-host_test_" + olderStart.Format(time.RFC3339)
+	newerID := "test-host_test_" + newerStart.Format(time.RFC3339)
+
+	db, err := database.New(ctx, filepath.Join(dir, "index.sqlite"))
+	require.NoError(t, err)
+
+	repos := database.NewRepositories(db)
+
+	manifestPaths := make(map[string]string)
+
+	for id, startedAt := range map[string]time.Time{
+		olderID: olderStart,
+		newerID: newerStart,
+	} {
+		completedAt := startedAt.Add(time.Minute)
+		snap := &database.Snapshot{
+			ID:             types.SnapshotID(id),
+			Hostname:       "test-host",
+			VaultikVersion: "test",
+			StartedAt:      startedAt,
+			CompletedAt:    &completedAt,
+		}
+
+		err = repos.WithTx(ctx, func(ctx context.Context, tx *sql.Tx) error {
+			return repos.Snapshots.Create(ctx, tx, snap)
+		})
+		require.NoError(t, err)
+
+		manifestPaths[id] = filepath.Join(dir, "store", "metadata",
+			snapshot.RemoteSnapshotKey(id), "manifest.json.zst")
+		require.NoError(t, os.MkdirAll(filepath.Dir(manifestPaths[id]), 0o750))
+		require.NoError(t, os.WriteFile(manifestPaths[id], []byte("stub"), 0o600))
+	}
+
+	_, err = db.Conn().ExecContext(ctx, `
+		CREATE TRIGGER refuse_snapshot_delete BEFORE DELETE ON snapshots
+		BEGIN SELECT RAISE(ABORT, 'simulated index error'); END`)
+	require.NoError(t, err)
+	require.NoError(t, db.Close())
+
+	code, _, stderr := runEntry(t,
+		flagConfig, configPath, cmdSnapshot, cmdPurge, flagKeepLatest, flagForce)
+
+	assert.Equal(t, 1, code)
+	assert.Contains(t, stderr, "simulated index error")
+	assert.FileExists(t, manifestPaths[olderID],
+		"a snapshot still in the local index keeps its metadata")
+}

internal/storage/file.go

@@ -120,10 +120,9 @@ func (f *FileStorer) Delete(_ context.Context, key string) error {
 	return nil
 }
 
-// List returns all keys with the given prefix, reading through symlinked
-// directories. It fails when the destination directory is missing, or
-// when the prefix or a directory above it is a symlink whose target is
-// missing; a prefix that is truly missing is an empty listing.
+// List returns all keys with the given prefix. It fails when the
+// destination directory is missing; a missing prefix under it is an
+// empty listing.
 func (f *FileStorer) List(ctx context.Context, prefix string) ([]string, error) {
 	var keys []string
 
@@ -134,7 +133,8 @@ func (f *FileStorer) List(ctx context.Context, prefix string) ([]string, error)
 
 	basePath := f.fullPath(prefix)
 
-	exists, err := f.prefixExists(prefix)
+	// Check if the prefix exists
+	exists, err := afero.Exists(f.fs, basePath)
 	if err != nil {
 		return nil, fmt.Errorf("checking path: %w", err)
 	}
@@ -143,7 +143,7 @@ func (f *FileStorer) List(ctx context.Context, prefix string) ([]string, error)
 		return keys, nil // Empty list for non-existent prefix
 	}
 
-	err = f.walk(basePath, func(path string, info os.FileInfo, err error) error {
+	err = afero.Walk(f.fs, basePath, func(path string, info os.FileInfo, err error) error {
 		if err != nil {
 			return err
 		}
@@ -176,9 +176,7 @@ func (f *FileStorer) List(ctx context.Context, prefix string) ([]string, error)
 }
 
 // ListStream returns a channel of ObjectInfo for large result sets. Like
-// List, it reads through symlinked directories and sends an error when
-// the destination directory is missing, or when the prefix or a directory
-// above it is a symlink whose target is missing.
+// List, it sends an error when the destination directory is missing.
 func (f *FileStorer) ListStream(ctx context.Context, prefix string) <-chan ObjectInfo {
 	ch := make(chan ObjectInfo)
 
@@ -194,7 +192,8 @@ func (f *FileStorer) ListStream(ctx context.Context, prefix string) <-chan Objec
 
 		basePath := f.fullPath(prefix)
 
-		exists, err := f.prefixExists(prefix)
+		// Check if the prefix exists
+		exists, err := afero.Exists(f.fs, basePath)
 		if err != nil {
 			ch <- ObjectInfo{Err: fmt.Errorf("checking path: %w", err)}
 
@@ -205,7 +204,7 @@ func (f *FileStorer) ListStream(ctx context.Context, prefix string) <-chan Objec
 			return // Empty channel for non-existent prefix
 		}
 
-		_ = f.walk(basePath, func(path string, info os.FileInfo, err error) error {
+		_ = afero.Walk(f.fs, basePath, func(path string, info os.FileInfo, err error) error {
 			// Check context cancellation
 			select {
 			case <-ctx.Done():
@@ -244,12 +243,11 @@ func (f *FileStorer) ListStream(ctx context.Context, prefix string) <-chan Objec
 }
 
 // DeletePartialUploads removes every file under prefix whose name ends in
-// tempSuffix. Like List, it reads through symlinked directories and fails
-// on one whose target is missing. A missing prefix has none to remove.
+// tempSuffix. A missing prefix has none to remove.
 func (f *FileStorer) DeletePartialUploads(ctx context.Context, prefix string) error {
 	basePath := f.fullPath(prefix)
 
-	exists, err := f.prefixExists(prefix)
+	exists, err := afero.Exists(f.fs, basePath)
 	if err != nil {
 		return fmt.Errorf("checking path: %w", err)
 	}
@@ -258,7 +256,7 @@ func (f *FileStorer) DeletePartialUploads(ctx context.Context, prefix string) er
 		return nil
 	}
 
-	err = f.walk(basePath, func(path string, info os.FileInfo, err error) error {
+	err = afero.Walk(f.fs, basePath, func(path string, info os.FileInfo, err error) error {
 		if err != nil {
 			return err
 		}
@@ -354,54 +352,6 @@ func (f *FileStorer) writeAtomic(
 	return nil
 }
 
-// walk is afero.Walk, except that it reads through a symlinked directory
-// the way Get and Stat do, where afero.Walk reports the link as one file.
-// afero.Walk uses Lstat, which does not follow a link, whenever the
-// filesystem has one; the struct below carries only the afero.Fs methods,
-// so the walk uses Stat instead. A link back up the tree is followed until
-// the operating system refuses the path for too many levels of links, and
-// the walk then fails.
-func (f *FileStorer) walk(root string, fn filepath.WalkFunc) error {
-	return afero.Walk(struct{ afero.Fs }{f.fs}, root, fn)
-}
-
-// prefixExists reports whether prefix exists under the destination
-// directory. It checks each directory on the way down to prefix in turn,
-// and the first one Stat cannot find decides: if it is not there at all,
-// prefix is missing; if it is a symlink whose target is missing, such as
-// a link to a volume that is not mounted, prefixExists fails, since
-// listing it as empty would let prune delete every blob.
-func (f *FileStorer) prefixExists(prefix string) (bool, error) {
-	lstater, canLstat := f.fs.(afero.Lstater)
-	path := f.basePath
-
-	for name := range strings.SplitSeq(prefix, "/") {
-		path = filepath.Join(path, name)
-
-		_, err := f.fs.Stat(path)
-		if err == nil {
-			continue
-		}
-
-		if !os.IsNotExist(err) {
-			return false, err
-		}
-
-		// Lstat does not follow a symlink at path, so it finds the link
-		// that Stat could not follow.
-		if canLstat {
-			_, _, lstatErr := lstater.LstatIfPossible(path)
-			if lstatErr == nil {
-				return false, err
-			}
-		}
-
-		return false, nil
-	}
-
-	return true, nil
-}
-
 // fullPath returns the full filesystem path for a key.
 func (f *FileStorer) fullPath(key string) string {
 	return filepath.Join(f.basePath, key)

internal/storage/file_backend_test.go

@@ -4,10 +4,7 @@ import (
 	"context"
 	"errors"
 	"io/fs"
-	"os"
 	"path/filepath"
-	"reflect"
-	"strings"
 	"testing"
 
 	"sneak.berlin/go/vaultik/internal/storage"
@@ -63,232 +60,3 @@ func TestFileStorerListMissingDestination(t *testing.T) {
 		t.Errorf("ListStream error = %v, want a not-exist error", streamErr)
 	}
 }
-
-// The two directories at the top of a destination, either of which the
-// tests below replace with a symlink.
-const (
-	metadataDir = "metadata"
-	blobsDir    = "blobs"
-)
-
-// moveBehindSymlink moves base/dir to a directory elsewhere and puts a
-// symlink to it in its place.
-func moveBehindSymlink(t *testing.T, base, dir string) {
-	t.Helper()
-
-	linkPath := filepath.Join(base, dir)
-	movedPath := filepath.Join(t.TempDir(), dir)
-
-	err := os.Rename(linkPath, movedPath)
-	if err != nil {
-		t.Fatalf("moving %s away: %v", dir, err)
-	}
-
-	err = os.Symlink(movedPath, linkPath)
-	if err != nil {
-		t.Fatalf("linking %s: %v", dir, err)
-	}
-}
-
-// TestFileStorerListsThroughSymlinkedDirectory checks that List and
-// ListStream read through a symlinked metadata/ or blobs/ directory, as Get
-// and Stat do, instead of listing the link as one key. Listing the whole
-// destination meets the link part-way through the walk; listing its prefix
-// starts the walk at the link. A prefix missing behind the link lists as
-// empty.
-func TestFileStorerListsThroughSymlinkedDirectory(t *testing.T) {
-	t.Parallel()
-
-	for _, dir := range []string{metadataDir, blobsDir} {
-		t.Run(dir, func(t *testing.T) {
-			t.Parallel()
-
-			base := t.TempDir()
-			key := dir + "/aa/bb/object"
-
-			s, err := storage.NewFileStorer(base)
-			if err != nil {
-				t.Fatalf("NewFileStorer: %v", err)
-			}
-
-			err = s.Put(context.Background(), key, strings.NewReader("data"))
-			if err != nil {
-				t.Fatalf("Put: %v", err)
-			}
-
-			moveBehindSymlink(t, base, dir)
-
-			want := []string{key}
-
-			for _, prefix := range []string{"", dir + "/"} {
-				got := listSorted(t, s, prefix)
-				if !reflect.DeepEqual(got, want) {
-					t.Errorf("List(%q) = %v, want %v", prefix, got, want)
-				}
-
-				got = listStreamKeys(t, s, prefix)
-				if !reflect.DeepEqual(got, want) {
-					t.Errorf("ListStream(%q) = %v, want %v", prefix, got, want)
-				}
-			}
-
-			absent := dir + "/absent/"
-
-			if got := listSorted(t, s, absent); len(got) != 0 {
-				t.Errorf("List(%q) = %v, want empty", absent, got)
-			}
-
-			if got := listStreamKeys(t, s, absent); len(got) != 0 {
-				t.Errorf("ListStream(%q) = %v, want empty", absent, got)
-			}
-		})
-	}
-}
-
-// TestFileStorerListFailsOnSymlinkWithMissingTarget checks that List and
-// ListStream fail on a symlinked metadata/ or blobs/ directory whose target
-// is missing, such as a link to a volume that is not mounted, as they do on
-// a missing destination directory, instead of listing it as empty. That
-// holds for a prefix below the link too.
-func TestFileStorerListFailsOnSymlinkWithMissingTarget(t *testing.T) {
-	t.Parallel()
-
-	ctx := context.Background()
-
-	for _, dir := range []string{metadataDir, blobsDir} {
-		t.Run(dir, func(t *testing.T) {
-			t.Parallel()
-
-			base := t.TempDir()
-
-			s, err := storage.NewFileStorer(base)
-			if err != nil {
-				t.Fatalf("NewFileStorer: %v", err)
-			}
-
-			unmounted := filepath.Join(t.TempDir(), "unmounted")
-
-			err = os.Symlink(unmounted, filepath.Join(base, dir))
-			if err != nil {
-				t.Fatalf("Symlink: %v", err)
-			}
-
-			for _, prefix := range []string{"", dir + "/", dir + "/aa/"} {
-				keys, err := s.List(ctx, prefix)
-				if !errors.Is(err, fs.ErrNotExist) {
-					t.Errorf("List(%q) = %v, %v; want a not-exist error",
-						prefix, keys, err)
-				}
-
-				var streamErr error
-
-				for object := range s.ListStream(ctx, prefix) {
-					if object.Err != nil {
-						streamErr = object.Err
-					}
-				}
-
-				if !errors.Is(streamErr, fs.ErrNotExist) {
-					t.Errorf("ListStream(%q) error = %v, want a not-exist error",
-						prefix, streamErr)
-				}
-			}
-		})
-	}
-}
-
-// TestFileStorerListFailsOnSymlinkLoop checks that a symlink pointing back
-// at the destination makes List and ListStream fail instead of walking it
-// forever.
-func TestFileStorerListFailsOnSymlinkLoop(t *testing.T) {
-	t.Parallel()
-
-	ctx := context.Background()
-	base := t.TempDir()
-
-	s, err := storage.NewFileStorer(base)
-	if err != nil {
-		t.Fatalf("NewFileStorer: %v", err)
-	}
-
-	err = s.Put(ctx, testBlobKey, strings.NewReader("data"))
-	if err != nil {
-		t.Fatalf("Put: %v", err)
-	}
-
-	err = os.Symlink(base, filepath.Join(base, "blobs", "loop"))
-	if err != nil {
-		t.Fatalf("Symlink: %v", err)
-	}
-
-	keys, err := s.List(ctx, "blobs/")
-	if err == nil {
-		t.Errorf("List = %d keys and no error, want an error", len(keys))
-	}
-
-	var streamErr error
-
-	for object := range s.ListStream(ctx, "blobs/") {
-		if object.Err != nil {
-			streamErr = object.Err
-		}
-	}
-
-	if streamErr == nil {
-		t.Error("ListStream sent no error, want one")
-	}
-}
-
-// TestFileStorerDeletePartialUploadsThroughSymlinkedDirectory checks that
-// DeletePartialUploads, which remote nuke uses, removes a leftover temp
-// file behind a symlinked metadata/ or blobs/ directory and keeps the
-// object at the real key.
-func TestFileStorerDeletePartialUploadsThroughSymlinkedDirectory(t *testing.T) {
-	t.Parallel()
-
-	ctx := context.Background()
-
-	for _, dir := range []string{metadataDir, blobsDir} {
-		t.Run(dir, func(t *testing.T) {
-			t.Parallel()
-
-			base := t.TempDir()
-			key := dir + "/aa/bb/object"
-
-			s, err := storage.NewFileStorer(base)
-			if err != nil {
-				t.Fatalf("NewFileStorer: %v", err)
-			}
-
-			err = s.Put(ctx, key, strings.NewReader("data"))
-			if err != nil {
-				t.Fatalf("Put: %v", err)
-			}
-
-			leftover := filepath.Join(base, key+"-123456.partial")
-
-			err = os.WriteFile(leftover, []byte("half"), 0o600)
-			if err != nil {
-				t.Fatalf("writing leftover temp file: %v", err)
-			}
-
-			moveBehindSymlink(t, base, dir)
-
-			err = s.DeletePartialUploads(ctx, dir+"/")
-			if err != nil {
-				t.Fatalf("DeletePartialUploads: %v", err)
-			}
-
-			// leftover runs through the link.
-			_, err = os.Stat(leftover)
-			if !os.IsNotExist(err) {
-				t.Errorf("leftover temp file was not removed: %v", err)
-			}
-
-			_, err = s.Stat(ctx, key)
-			if err != nil {
-				t.Errorf("Stat of the real key: %v", err)
-			}
-		})
-	}
-}

internal/vaultik/blobcache.go

@@ -39,9 +39,9 @@ type blobDiskCacheEntry struct {
 // internally and will fall back to LRU eviction if curBytes exceeds
 // maxBytes. Restore passes math.MaxInt64 as maxBytes and drives
 // eviction itself via Delete() through restoreSweeper, which deletes
-// each blob once no file still to be restored in this run needs it.
-// LRU never fires under that configuration; it is kept as a safety net
-// for callers that don't manage eviction themselves.
+// each blob the moment every file that references its chunks has been
+// written. LRU never fires under that configuration; it is kept as a
+// safety net for callers that don't manage eviction themselves.
 //
 // Get/ReadAt/peak-Len counters are debugging instrumentation used by
 // tests to assert that the restore code path uses ReadAt rather than

internal/vaultik/restore.go

@@ -299,6 +299,12 @@ func (v *Vaultik) restoreAllFiles(
 		_ = blobCache.Close()
 	}()
 
+	// Per-restore sweep state: every blob_size_limit/100 bytes written,
+	// scan the cache and delete any blob whose remaining file references
+	// are all already restored.
+	sweeper := newRestoreSweeper(v.ctx, repos, blobCache,
+		v.Config.BlobSizeLimit.Int64()/sweepIntervalDivisor)
+
 	blobByHash, blobIDToHash, err := v.buildBlobIndexes(repos)
 	if err != nil {
 		return nil, err
@@ -310,12 +316,6 @@ func (v *Vaultik) restoreAllFiles(
 		return nil, fmt.Errorf("building restore plan: %w", err)
 	}
 
-	// Per-restore sweep state: every blob_size_limit/100 bytes written,
-	// scan the cache and delete any blob that no pending file in the
-	// plan references.
-	sweeper := newRestoreSweeper(plan, blobCache,
-		v.Config.BlobSizeLimit.Int64()/sweepIntervalDivisor)
-
 	// Ranging over files rather than the map reports them in a stable order.
 	for _, file := range files {
 		if planErr, ok := unplanned[file.ID]; ok {
@@ -403,8 +403,10 @@ func (v *Vaultik) runRestoreLoop(
 			continue
 		}
 
-		// Drop the file from the plan's indexes so future picks ignore
-		// it and the sweeper can free blobs no pending file needs.
+		// Record the file as restored so the sweeper can free blobs
+		// once all referencing files are done, and drop it from the
+		// plan's indexes so future picks ignore it.
+		session.sweeper.fileRestored(fileID.String())
 		plan.finishFile(fileID)
 
 		processed++

internal/vaultik/restore_interrupt_test.go

@@ -74,8 +74,7 @@ func TestRestoreCleansTempDirOnInterrupt(t *testing.T) {
 	buildLocalityFixture(t, fs, dataDir)
 
 	cfg, storer, snapshotID := setupLocalityBackup(
-		context.Background(), t, fs, dataDir, storeDir, dbPath,
-		localitySourceBytes)
+		context.Background(), t, fs, dataDir, storeDir, dbPath)
 
 	// Point the "" temp paths (the blob cache directory and the
 	// snapshot-database directory) at a private directory so the test can

internal/vaultik/restore_locality_test.go

@@ -63,10 +63,6 @@ type localityCopy struct {
 	data []byte
 }
 
-// localitySourceBytes is the size of each source file of the locality
-// fixture.
-const localitySourceBytes = 1024 * 1024
-
 // buildLocalityFixture writes the adversarial source layout described
 // in TestRestoreLocalityAndReadAt: 15 sources of 1 MiB each (which the
 // backup packs into 3 blobs of 5 chunks) plus 9 interleaved-name copies
@@ -77,6 +73,7 @@ func buildLocalityFixture(
 	t.Helper()
 
 	const (
+		srcBytes   = 1024 * 1024
 		srcCount   = 15
 		blobsCount = 3
 		perBlob    = srcCount / blobsCount
@@ -86,7 +83,7 @@ func buildLocalityFixture(
 	for i := range srcCount {
 		s := &localitySource{
 			path: fmt.Sprintf("src-%03d.bin", i+1),
-			data: randomBytes(t, localitySourceBytes),
+			data: randomBytes(t, srcBytes),
 		}
 		sources[i] = s
 		require.NoError(t, afero.WriteFile(
@@ -118,20 +115,19 @@ func buildLocalityFixture(
 
 // setupLocalityBackup runs the backup half of the locality test: a
 // snapshot of dataDir into a file storer with sizes tuned so 15 one-chunk
-// files of srcBytes pack into 3 blobs. The index database is closed
-// before returning so the restore half runs from remote bytes only.
+// files pack into 3 blobs. The index database is closed before returning
+// so the restore half runs from remote bytes only.
 func setupLocalityBackup(
 	ctx context.Context, t *testing.T, fs afero.Fs,
-	dataDir, storeDir, dbPath string, srcBytes int64,
+	dataDir, storeDir, dbPath string,
 ) (*config.Config, *storage.FileStorer, string) {
 	t.Helper()
 
-	// An average chunk size of four times srcBytes makes the minimum
-	// chunk srcBytes, so each source file becomes one chunk. A blob limit
-	// of five times srcBytes packs exactly 5 chunks per blob, yielding 3
-	// blobs from 15 source files.
-	chunkSize := 4 * srcBytes
-	maxBlobSize := 5 * srcBytes
+	// chunkSize avg = 4 MiB makes minSize = 1 MiB, so a 1 MiB file
+	// becomes one chunk. maxBlobSize = 5 MiB packs exactly 5 chunks
+	// per blob, yielding 3 blobs from 15 source files.
+	chunkSize := int64(4 * 1024 * 1024)
+	maxBlobSize := int64(5 * 1024 * 1024)
 
 	storer, err := storage.NewFileStorer(storeDir)
 	require.NoError(t, err)
@@ -212,7 +208,7 @@ func TestRestoreLocalityAndReadAt(t *testing.T) {
 	ctx := context.Background()
 
 	cfg, storer, snapshotID := setupLocalityBackup(
-		ctx, t, fs, dataDir, storeDir, dbPath, localitySourceBytes)
+		ctx, t, fs, dataDir, storeDir, dbPath)
 
 	// Wrap the storer so we can count downloads per blob key.
 	counter := newCountingStorer(storer)

internal/vaultik/restore_plan.go

@@ -31,8 +31,7 @@ var (
 //     with no further downloads.
 //   - blobFiles: for each blob, the set of pending files referencing
 //     it. Used to short-circuit "when this blob lands, which files
-//     become ready" without a global scan, and by the sweeper to tell
-//     whether a cached blob is still needed.
+//     become ready" without a global scan.
 type restorePlan struct {
 	fileBlobs map[types.FileID]map[string]struct{}
 	blobFiles map[string]map[types.FileID]struct{}
@@ -238,11 +237,6 @@ func (p *restorePlan) filesReferencingBlob(blobHash string) []types.FileID {
 	return out
 }
 
-// blobNeeded reports whether any pending file references the named blob.
-func (p *restorePlan) blobNeeded(blobHash string) bool {
-	return len(p.blobFiles[blobHash]) > 0
-}
-
 // hasPending reports whether any unfinished files remain.
 func (p *restorePlan) hasPending() bool {
 	return len(p.fileBlobs) > 0

internal/vaultik/restore_plan_test.go

@@ -63,6 +63,15 @@ func TestRunRestoreLoopFailsOnAbandonedFiles(t *testing.T) {
 
 	t.Cleanup(func() { _ = cache.Close() })
 
+	v := &Vaultik{ctx: ctx}
+	session := &restoreSession{
+		v:       v,
+		ctx:     ctx,
+		repos:   repos,
+		sweeper: newRestoreSweeper(ctx, repos, cache, 1),
+		result:  &RestoreResult{},
+	}
+
 	// A file that is still pending but whose uncached-blob set is empty
 	// and which was never queued as ready: the loop can neither restore
 	// nor download it. This is the abandonment the guard must catch.
@@ -74,15 +83,6 @@ func TestRunRestoreLoopFailsOnAbandonedFiles(t *testing.T) {
 		cached:    map[string]struct{}{},
 	}
 
-	v := &Vaultik{ctx: ctx}
-	session := &restoreSession{
-		v:       v,
-		ctx:     ctx,
-		repos:   repos,
-		sweeper: newRestoreSweeper(plan, cache, 1),
-		result:  &RestoreResult{},
-	}
-
 	err = v.runRestoreLoop(session, plan, map[types.FileID]*database.File{}, 0)
 	require.ErrorIs(t, err, errRestoreIncomplete)
 }

internal/vaultik/restore_sweeper.go

@@ -1,30 +1,46 @@
 package vaultik
 
-// restoreSweeper frees cached blobs once no file still to be restored in
-// this run needs them. It works as follows:
+import (
+	"context"
+	"fmt"
+
+	"sneak.berlin/go/vaultik/internal/database"
+	"sneak.berlin/go/vaultik/internal/log"
+)
+
+// restoreSweeper frees cached blobs once all files that reference any of
+// their chunks have been restored. It works as follows:
 //
-//  1. After each chunk is restored, chunkRestored accumulates a running
+//  1. Callers add a file's ID to an in-memory restored set via
+//     fileRestored once the file is fully written to disk.
+//  2. After each chunk is restored, chunkRestored accumulates a running
 //     byte count.
-//  2. When the accumulator crosses a threshold (one hundredth of the
+//  3. When the accumulator crosses a threshold (one hundredth of the
 //     configured blob size — so a sweep runs about a hundred times per
 //     blob's worth of restored bytes), the sweeper iterates every key in
-//     the cache and deletes each blob that the restore plan no longer
-//     needs.
+//     the cache. For each cached blob it asks the DB which files
+//     reference any chunk in that blob, then compares that list against
+//     the in-memory restored set. If any referencing file is missing
+//     from the set the blob is kept; otherwise the cache entry is
+//     deleted.
 //
-// The plan drops a file once it is restored or has failed, and holds
-// only the files this run restores, so files outside the requested paths
-// never keep a blob.
+// All DB reads happen against the snapshot's temporary metadata DB,
+// which is local, indexed, and not under contention — the queries are
+// cheap and run at most once per blob per sweep interval.
 type restoreSweeper struct {
-	plan       *restorePlan
+	ctx        context.Context //nolint:containedctx // ctx bound at construction by design
+	repos      *database.Repositories
 	cache      *blobDiskCache
 	threshold  int64
 	bytesAccum int64
+	restored   map[string]struct{}
 }
 
 // newRestoreSweeper returns a sweeper that triggers eviction every
 // `threshold` bytes restored. Callers should pass blob_size_limit/100.
 func newRestoreSweeper(
-	plan *restorePlan,
+	ctx context.Context,
+	repos *database.Repositories,
 	cache *blobDiskCache,
 	threshold int64,
 ) *restoreSweeper {
@@ -33,12 +49,21 @@ func newRestoreSweeper(
 	}
 
 	return &restoreSweeper{
-		plan:      plan,
+		ctx:       ctx,
+		repos:     repos,
 		cache:     cache,
 		threshold: threshold,
+		restored:  make(map[string]struct{}),
 	}
 }
 
+// fileRestored records a file as fully restored. After this call, any
+// blob whose only remaining references come from files in the restored
+// set may be evicted on the next sweep.
+func (s *restoreSweeper) fileRestored(fileID string) {
+	s.restored[fileID] = struct{}{}
+}
+
 // chunkRestored accounts n bytes against the sweep threshold and runs a
 // sweep if the threshold has been crossed since the last sweep.
 func (s *restoreSweeper) chunkRestored(n int64) {
@@ -51,11 +76,60 @@ func (s *restoreSweeper) chunkRestored(n int64) {
 	s.sweep()
 }
 
-// sweep deletes every cached blob that no pending file references.
+// sweep deletes any cached blob whose chunks are no longer referenced
+// by an unrestored file. Per-blob DB failures are logged and the blob
+// is kept — we'd rather hold a blob longer than risk a re-download.
 func (s *restoreSweeper) sweep() {
 	for _, blobHash := range s.cache.Keys() {
-		if !s.plan.blobNeeded(blobHash) {
+		needed, err := s.blobStillNeeded(blobHash)
+		if err != nil {
+			log.Debug("sweeper referencing-files query failed",
+				"blob_hash", blobHash[:16], "error", err)
+
+			continue
+		}
+
+		if !needed {
 			s.cache.Delete(blobHash)
 		}
 	}
 }
+
+// blobStillNeeded returns true if any file that references a chunk in
+// this blob has not yet been restored. On any error the function
+// returns true — keeping the blob is always the safe answer because we
+// can't prove we're done with it.
+func (s *restoreSweeper) blobStillNeeded(blobHash string) (bool, error) {
+	rows, err := s.repos.DB().Conn().QueryContext(s.ctx, `
+		SELECT DISTINCT fc.file_id
+		FROM file_chunks fc
+		JOIN blob_chunks bc ON bc.chunk_hash = fc.chunk_hash
+		JOIN blobs b ON b.id = bc.blob_id
+		WHERE b.blob_hash = ?
+	`, blobHash)
+	if err != nil {
+		return true, fmt.Errorf("querying referencing files: %w", err)
+	}
+
+	defer func() { _ = rows.Close() }()
+
+	for rows.Next() {
+		var fileID string
+
+		err := rows.Scan(&fileID)
+		if err != nil {
+			return true, fmt.Errorf("scanning file_id: %w", err)
+		}
+
+		if _, ok := s.restored[fileID]; !ok {
+			return true, nil
+		}
+	}
+
+	err = rows.Err()
+	if err != nil {
+		return true, err
+	}
+
+	return false, nil
+}

internal/vaultik/restore_sweeper_test.go

@@ -1,163 +0,0 @@
-package vaultik //nolint:testpackage // reads the restore blob cache's peak size
-
-import (
-	"context"
-	"fmt"
-	"io"
-	"path/filepath"
-	"testing"
-
-	"github.com/spf13/afero"
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-	"sneak.berlin/go/vaultik/internal/config"
-	"sneak.berlin/go/vaultik/internal/log"
-	"sneak.berlin/go/vaultik/internal/storage"
-	"sneak.berlin/go/vaultik/internal/ui"
-)
-
-// TestRestoreFreesBlobsNoPendingFileNeeds asserts that the restore cache
-// frees a blob once no file still to be restored in this run needs it.
-// Every original file has a copy with the same content, so every blob
-// holds chunks of copies: a restore that leaves the copies out of its
-// paths, or fails on them, must not keep the blobs for them.
-func TestRestoreFreesBlobsNoPendingFileNeeds(t *testing.T) {
-	log.Initialize(log.Config{})
-	t.Parallel()
-
-	// Small files keep the backup and both restores quick.
-	const fileBytes = 16 * 1024
-
-	fs := afero.NewOsFs()
-	tempDir := t.TempDir()
-
-	dataDir := filepath.Join(tempDir, "source")
-	originalsDir := filepath.Join(dataDir, "originals")
-	copiesDir := filepath.Join(dataDir, "copies")
-	storeDir := filepath.Join(tempDir, "remote")
-	dbPath := filepath.Join(tempDir, "index.sqlite")
-
-	originals := writeFilesWithCopies(t, fs, originalsDir, copiesDir, fileBytes)
-
-	ctx := context.Background()
-
-	cfg, storer, snapshotID := setupLocalityBackup(
-		ctx, t, fs, dataDir, storeDir, dbPath, fileBytes)
-
-	t.Run("restore by path", func(t *testing.T) {
-		t.Parallel()
-
-		restoreDir := filepath.Join(tempDir, "by-path")
-
-		peak, err := restorePeakCachedBlobs(ctx, t, fs, cfg, storer,
-			&RestoreOptions{
-				SnapshotID: snapshotID,
-				TargetDir:  restoreDir,
-				Paths:      []string{originalsDir},
-			})
-		require.NoError(t, err)
-
-		requireRestoredFiles(t, fs, restoreDir, originals)
-		assert.LessOrEqualf(t, peak, 1,
-			"peak cached blobs was %d; blobs were kept for unselected files",
-			peak)
-	})
-
-	t.Run("restore with failed files", func(t *testing.T) {
-		t.Parallel()
-
-		restoreDir := filepath.Join(tempDir, "with-failures")
-
-		// A regular file where the copies directory belongs makes every
-		// copy fail to restore.
-		blocker := filepath.Join(restoreDir, copiesDir)
-		require.NoError(t, fs.MkdirAll(filepath.Dir(blocker), 0o755))
-		require.NoError(t, afero.WriteFile(fs, blocker, nil, 0o644))
-
-		peak, err := restorePeakCachedBlobs(ctx, t, fs, cfg, storer,
-			&RestoreOptions{
-				SnapshotID: snapshotID,
-				TargetDir:  restoreDir,
-				SkipErrors: true,
-			})
-		require.ErrorIs(t, err, errFilesFailedRestore)
-
-		requireRestoredFiles(t, fs, restoreDir, originals)
-		assert.LessOrEqualf(t, peak, 1,
-			"peak cached blobs was %d; blobs were kept for failed files", peak)
-	})
-}
-
-// writeFilesWithCopies writes 15 random files of fileBytes into
-// originalsDir and a copy of each into copiesDir. With
-// setupLocalityBackup given the same size, each file is one chunk and the
-// 15 chunks fill 3 blobs. Returns the originals' content by path.
-func writeFilesWithCopies(
-	t *testing.T, fs afero.Fs, originalsDir, copiesDir string, fileBytes int,
-) map[string][]byte {
-	t.Helper()
-
-	const fileCount = 15
-
-	require.NoError(t, fs.MkdirAll(originalsDir, 0o755))
-	require.NoError(t, fs.MkdirAll(copiesDir, 0o755))
-
-	originals := make(map[string][]byte, fileCount)
-
-	for i := range fileCount {
-		name := fmt.Sprintf("file-%02d.bin", i)
-		data := randomBytes(t, fileBytes)
-		original := filepath.Join(originalsDir, name)
-		originals[original] = data
-
-		require.NoError(t, afero.WriteFile(fs, original, data, 0o644))
-		require.NoError(t, afero.WriteFile(
-			fs, filepath.Join(copiesDir, name), data, 0o644))
-	}
-
-	return originals
-}
-
-// requireRestoredFiles requires every file in want to have been restored
-// under restoreDir with its content intact.
-func requireRestoredFiles(
-	t *testing.T, fs afero.Fs, restoreDir string, want map[string][]byte,
-) {
-	t.Helper()
-
-	for path, data := range want {
-		got, err := afero.ReadFile(fs, filepath.Join(restoreDir, path))
-		require.NoErrorf(t, err, "file missing after restore: %s", path)
-		require.Equalf(t, data, got, "byte mismatch for %s", path)
-	}
-}
-
-// restorePeakCachedBlobs runs a restore and returns the most blobs its
-// cache held at once, together with the restore's error.
-func restorePeakCachedBlobs(
-	ctx context.Context, t *testing.T, fs afero.Fs,
-	cfg *config.Config, storer storage.Storer, opts *RestoreOptions,
-) (int, error) {
-	t.Helper()
-
-	var cache *blobDiskCache
-
-	v := &Vaultik{
-		Config:  cfg,
-		Storage: storer,
-		Fs:      fs,
-		Stdout:  io.Discard,
-		Stderr:  io.Discard,
-		UI:      ui.NewWithColor(io.Discard, false),
-		restoreCacheObserver: func(c *blobDiskCache) {
-			cache = c
-		},
-	}
-	v.SetContext(ctx)
-
-	err := v.Restore(opts)
-
-	require.NotNil(t, cache, "restoreCacheObserver must fire during restore")
-
-	return cache.PeakLen(), err
-}

internal/vaultik/snapshot.go

@@ -627,7 +627,8 @@ func selectSnapshotsToPurge(
 }
 
 // confirmAndExecutePurge shows deletion candidates, confirms with the
-// user, and deletes snapshots.
+// user, and deletes snapshots. It stops with an error at the first
+// snapshot it cannot delete.
 func (v *Vaultik) confirmAndExecutePurge(
 	toDelete []SnapshotInfo, force, quiet bool,
 ) error {
@@ -669,10 +670,11 @@ func (v *Vaultik) confirmAndExecutePurge(
 		snapshotID := snap.ID.String()
 		log.Info("Deleting snapshot", "id", snapshotID)
 
+		// Stop before the remote delete: a snapshot still in the local
+		// index must keep its metadata on the destination store.
 		err := v.deleteSnapshotFromLocalDB(snapshotID)
 		if err != nil {
-			log.Error("Failed to delete from local database",
-				"snapshot_id", snapshotID, "error", err)
+			return fmt.Errorf("deleting snapshot %s from local index: %w", snapshotID, err)
 		}
 
 		_, err = v.deleteRemoteSnapshotByKey(snapshot.RemoteSnapshotKey(snapshotID))

internal/vaultik/symlinked_destination_test.go

@@ -1,147 +0,0 @@
-package vaultik_test
-
-import (
-	"context"
-	"io/fs"
-	"os"
-	"path/filepath"
-	"testing"
-
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-	"sneak.berlin/go/vaultik/internal/log"
-	"sneak.berlin/go/vaultik/internal/vaultik"
-)
-
-// TestPruneDeletesNothingThroughSymlinkedDirectory checks that prune keeps
-// every blob and the local snapshot record when the metadata/ or blobs/
-// directory of a file:// destination is a symlink to a directory elsewhere
-// (https://git.eeqj.de/sneak/vaultik/issues/296). A backup writes through
-// such a link, so prune has to list through it too.
-func TestPruneDeletesNothingThroughSymlinkedDirectory(t *testing.T) {
-	log.Initialize(log.Config{})
-	t.Parallel()
-
-	for _, dir := range []string{"metadata", "blobs"} {
-		t.Run(dir, func(t *testing.T) {
-			t.Parallel()
-
-			ctx := context.Background()
-			storeDir := filepath.Join(t.TempDir(), "store")
-			v, repos, _ := backUpToFileDestination(ctx, t, storeDir)
-			blobFiles := blobFilesIn(t, storeDir)
-			moveBehindSymlink(t, storeDir, dir)
-
-			require.NoError(t, v.Prune(&vaultik.PruneOptions{Force: true}))
-
-			// When blobs/ is the link, these paths run through it.
-			for _, blobFile := range blobFiles {
-				assert.FileExists(t, blobFile, "prune must delete no blob")
-			}
-
-			snapshots, err := repos.Snapshots.ListRecent(ctx, listRecentTestLimit)
-			require.NoError(t, err)
-			assert.Len(t, snapshots, 1, "prune must delete no local snapshot record")
-		})
-	}
-}
-
-// TestPruneFailsOnSymlinkedDirectoryWithMissingTarget checks that prune
-// fails, deleting no blob and keeping the local snapshot record, when the
-// metadata/ or blobs/ directory of a file:// destination is a symlink
-// whose target is missing, as a link to a volume that is not mounted is.
-func TestPruneFailsOnSymlinkedDirectoryWithMissingTarget(t *testing.T) {
-	log.Initialize(log.Config{})
-	t.Parallel()
-
-	for _, dir := range []string{"metadata", "blobs"} {
-		t.Run(dir, func(t *testing.T) {
-			t.Parallel()
-
-			ctx := context.Background()
-			storeDir := filepath.Join(t.TempDir(), "store")
-			v, repos, _ := backUpToFileDestination(ctx, t, storeDir)
-			blobFiles := blobFilesIn(t, storeDir)
-			targetPath := moveBehindSymlink(t, storeDir, dir)
-
-			// Unmount the volume the link points to, and mount it again
-			// once prune has run.
-			unmountedPath := targetPath + "-unmounted"
-			require.NoError(t, os.Rename(targetPath, unmountedPath))
-
-			err := v.Prune(&vaultik.PruneOptions{Force: true})
-			require.ErrorIs(t, err, fs.ErrNotExist)
-
-			require.NoError(t, os.Rename(unmountedPath, targetPath))
-
-			// When blobs/ is the link, these paths run through it.
-			for _, blobFile := range blobFiles {
-				assert.FileExists(t, blobFile, "prune must delete no blob")
-			}
-
-			snapshots, err := repos.Snapshots.ListRecent(ctx, listRecentTestLimit)
-			require.NoError(t, err)
-			assert.Len(t, snapshots, 1, "prune must delete no local snapshot record")
-		})
-	}
-}
-
-// TestRemoveSnapshotWarnsOnSymlinkedMetadataWithMissingTarget checks that
-// snapshot remove warns that the metadata could not be removed from the
-// destination when metadata/ is a symlink whose target is missing, as it
-// does when the destination directory is missing, instead of finding no
-// metadata to remove.
-func TestRemoveSnapshotWarnsOnSymlinkedMetadataWithMissingTarget(t *testing.T) {
-	log.Initialize(log.Config{})
-	t.Parallel()
-
-	ctx := context.Background()
-	storeDir := filepath.Join(t.TempDir(), "store")
-	v, repos, out := backUpToFileDestination(ctx, t, storeDir)
-	targetPath := moveBehindSymlink(t, storeDir, "metadata")
-	require.NoError(t, os.Rename(targetPath, targetPath+"-unmounted"))
-
-	result, err := v.RemoveSnapshot(localSnapshotID(ctx, t, repos, "first"),
-		&vaultik.RemoveOptions{Force: true})
-	require.NoError(t, err)
-
-	assert.False(t, result.RemoteRemoved)
-	assert.Contains(t, out.String(),
-		"Could not remove snapshot metadata from remote")
-	assert.NotContains(t, out.String(),
-		"Removed snapshot metadata from remote storage")
-}
-
-// blobFilesIn returns the paths of the blob files under storeDir/blobs, and
-// fails the test when there are none.
-func blobFilesIn(t *testing.T, storeDir string) []string {
-	t.Helper()
-
-	var blobFiles []string
-
-	err := filepath.WalkDir(filepath.Join(storeDir, "blobs"),
-		func(path string, entry fs.DirEntry, err error) error {
-			if err == nil && !entry.IsDir() {
-				blobFiles = append(blobFiles, path)
-			}
-
-			return err
-		})
-	require.NoError(t, err)
-	require.NotEmpty(t, blobFiles)
-
-	return blobFiles
-}
-
-// moveBehindSymlink moves storeDir/dir to a directory elsewhere, puts a
-// symlink to it in its place, and returns where it went.
-func moveBehindSymlink(t *testing.T, storeDir, dir string) string {
-	t.Helper()
-
-	linkPath := filepath.Join(storeDir, dir)
-	targetPath := filepath.Join(t.TempDir(), dir)
-	require.NoError(t, os.Rename(linkPath, targetPath))
-	require.NoError(t, os.Symlink(targetPath, linkPath))
-
-	return targetPath
-}