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)
@@ -351,20 +353,16 @@ snapshot from the local index and strips the snapshot's metadata from
 the backup destination store. Blobs are NOT touched — deleting blobs
 requires reading every remaining remote manifest (the destination store
 may hold snapshots this host doesn't know about), which is what
-`vaultik prune` does. The command prints the `vaultik prune` invocation
+`vaultik prune` does. On success the command prints the exact `vaultik
-to run as a follow-up only when it removed the snapshot's metadata from
+prune` invocation to run as a follow-up. An ID that names no snapshot in
-the destination store, and never under `--json`. An ID that names no
+the local index or on the destination store is an error; with
-snapshot in the local index or on the destination store is an error;
+`--local-only`, the snapshot must be in the local index. Local row
-with `--local-only`, the snapshot must be in the local index. Local row
 cleanup (files, chunks, blobs the snapshot was the last referrer for)
-runs automatically. If removing the snapshot's metadata from the
+runs automatically. If the destination store is unreachable, the local-DB
-destination store fails (for example because the store is unreachable),
+removal still completes and a warning is emitted; run `vaultik snapshot
-the local-DB removal still completes and a warning is emitted; run
+remove <snapshot-id>` again once the store is reachable to remove the
-`vaultik snapshot remove <snapshot-id>` again once the store is reachable
+snapshot's metadata from it (`vaultik prune` does not). To wipe everything
-to remove the snapshot's metadata from it (`vaultik prune` does not). For
+on the destination in one go, use `vaultik remote nuke --force`.
-a snapshot that is only on the destination store, that failure is an
-error. To wipe everything on the destination in one go, use `vaultik
-remote nuke --force`.
 * `--local-only`: Skip remote cleanup; only touch the local index
 * `--dry-run`: Show what would be deleted without deleting
 * `--force`: Skip confirmation prompt

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/cli/snapshot.go

@@ -270,9 +270,8 @@ By default, this removes the snapshot from the local index database and
 strips the snapshot's metadata from the backup destination store. Blobs
 are NOT touched: deleting them requires reading every remaining remote
 manifest (the destination store may hold snapshots this host doesn't
-know about), which is what 'vaultik prune' does. The command prints the
+know about), which is what 'vaultik prune' does. On success the command
-'vaultik prune' invocation to run as a follow-up only when it removed the
+prints the exact 'vaultik prune' invocation to run as a follow-up.
-snapshot's metadata from the destination store, and never under --json.
 
 An ID that names no snapshot in the local index or on the destination
 store is an error.
@@ -281,12 +280,10 @@ Use --local-only to skip the remote half (e.g. when you want to forget a
 snapshot locally without touching the destination store); the snapshot
 must then be in the local index.
 
-If removing the snapshot's metadata from the destination store fails (for
+If the remote is unreachable, the local-database removal still completes
-example because the remote is unreachable), the local-database removal
+and a warning is emitted; run 'vaultik snapshot remove <snapshot-id>' again
-still completes and a warning is emitted; run 'vaultik snapshot remove
+once the destination store is reachable to remove the snapshot's metadata
-<snapshot-id>' again once the destination store is reachable to remove
+from it ('vaultik prune' does not).
-the snapshot's metadata from it ('vaultik prune' does not). For a
-snapshot that is only on the destination store, that failure is an error.
 
 To wipe the entire destination store and start over, use 'vaultik remote
 nuke --force' — it is the single supported entry point for that.`,

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
+// List returns all keys with the given prefix. It fails when the
-// directories. It fails when the destination directory is missing, or
+// destination directory is missing; a missing prefix under it is an
-// when the prefix or a directory above it is a symlink whose target is
+// empty listing.
-// missing; a prefix that is truly missing 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
+// List, it sends an error when the destination directory is missing.
-// the destination directory is missing, or when the prefix or a directory
-// above it is a symlink whose target 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
+// tempSuffix. A missing prefix has none to remove.
-// on one whose target is missing. 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/remove_snapshot_test.go

@@ -17,7 +17,6 @@ import (
 	"sneak.berlin/go/vaultik/internal/log"
 	"sneak.berlin/go/vaultik/internal/snapshot"
 	"sneak.berlin/go/vaultik/internal/storage"
-	"sneak.berlin/go/vaultik/internal/storage/faultstore"
 	"sneak.berlin/go/vaultik/internal/types"
 	"sneak.berlin/go/vaultik/internal/vaultik"
 )
@@ -432,40 +431,6 @@ func TestRemoveSnapshot_NotInLocalIndex(t *testing.T) {
 	assert.Contains(t, out, "Removed snapshot metadata from remote storage")
 }
 
-// TestRemoveSnapshot_NotInLocalIndexDeleteFails checks that removing a
-// snapshot only the destination store holds fails when its metadata
-// cannot be deleted there, under --force and under --json: nothing was
-// removed, so the command must not succeed.
-func TestRemoveSnapshot_NotInLocalIndexDeleteFails(t *testing.T) {
-	log.Initialize(log.Config{})
-	t.Parallel()
-
-	for name, opts := range map[string]vaultik.RemoveOptions{
-		"force": {Force: true},
-		"json":  {JSON: true},
-	} {
-		t.Run(name, func(t *testing.T) {
-			t.Parallel()
-
-			store := newTestStorer()
-			addManifest(t, store, "snapshot-001", nil)
-
-			faulty := faultstore.New(store)
-			faulty.OnDelete = func(string) bool { return true }
-
-			tv := vaultik.NewForTesting(faulty)
-			tv.Repositories = newLocalIndex(t)
-
-			result, err := tv.RemoveSnapshot("snapshot-001", &opts)
-
-			require.ErrorIs(t, err, faultstore.ErrInjectedDelete)
-			assert.False(t, result.RemoteRemoved)
-			assert.True(t, store.hasKey(remoteKeyPath("snapshot-001")))
-			assert.Empty(t, tv.Stdout.String())
-		})
-	}
-}
-
 func TestRemoveAllSnapshots_RequiresForce(t *testing.T) {
 	log.Initialize(log.Config{})
 	t.Parallel()

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",
+			return fmt.Errorf("deleting snapshot %s from local index: %w", snapshotID, err)
-				"snapshot_id", snapshotID, "error", err)
 		}
 
 		_, err = v.deleteRemoteSnapshotByKey(snapshot.RemoteSnapshotKey(snapshotID))
@@ -1218,11 +1220,9 @@ const snapshotRemoveCommandHint = "vaultik snapshot remove"
 // destination store. Blobs are NOT touched: removing a snapshot's
 // blobs requires reading every remaining remote manifest (the remote
 // may hold snapshots this host doesn't know about), which is what
-// `vaultik prune` is for — when metadata was removed from the
+// `vaultik prune` is for — the command prints the prune invocation to
-// destination store, the text output names the prune invocation to run
+// run as a follow-up. When the remote is unreachable the command still
-// as a follow-up. When removing the metadata fails, a snapshot in the
+// completes the local-DB removal and warns.
-// local index is still removed from it, with a warning; a snapshot
-// only on the destination store is an error.
 func (v *Vaultik) RemoveSnapshot(
 	snapshotID string, opts *RemoveOptions,
 ) (*RemoveResult, error) {
@@ -1257,10 +1257,8 @@ func (v *Vaultik) RemoveSnapshot(
 		}
 	}
 
-	result.RemoteRemoved, err = v.removeSnapshotRemote(
+	if !opts.LocalOnly {
-		snapshotID, inLocalIndex, opts)
+		result.RemoteRemoved = v.removeSnapshotRemote(snapshotID, opts)
-	if err != nil {
-		return result, err
 	}
 
 	if v.SnapshotManager != nil {
@@ -1382,32 +1380,19 @@ func (v *Vaultik) confirmRemoveSnapshot(snapshotID string, opts *RemoveOptions)
 }
 
 // removeSnapshotRemote strips the snapshot's metadata from the
-// destination store and returns true when it deleted something; under
+// destination store, warning and proceeding on failure: the local-DB
-// LocalOnly it does nothing. When the snapshot is only on the
+// removal has already happened, so the user is told the remote half
-// destination store, a failure is returned as an error, because that
+// didn't finish and to run `vaultik snapshot remove` for the snapshot
-// removal is all the command does. When the snapshot was in the local
+// again once the destination store is reachable (`vaultik prune` never
-// index, its removal there has already happened, so a failure is only a
+// removes snapshot metadata). Returns true when the remote removal
-// warning telling the user to run `vaultik snapshot remove` for the
+// succeeded and deleted something.
-// snapshot again once the destination store is reachable (`vaultik
+func (v *Vaultik) removeSnapshotRemote(snapshotID string, opts *RemoveOptions) bool {
-// prune` never removes snapshot metadata).
-func (v *Vaultik) removeSnapshotRemote(
-	snapshotID string, inLocalIndex bool, opts *RemoveOptions,
-) (bool, error) {
-	if opts.LocalOnly {
-		return false, nil
-	}
-
 	log.Info("Removing snapshot metadata from remote storage",
 		"snapshot_id", snapshotID)
 
 	remoteKey := snapshot.RemoteSnapshotKey(snapshotID)
 
 	deleted, err := v.deleteRemoteSnapshotByKey(remoteKey)
-	if err != nil && !inLocalIndex {
-		return false, fmt.Errorf(
-			"removing snapshot metadata from destination store: %w", err)
-	}
-
 	if err != nil {
 		log.Warn("Could not remove snapshot metadata from remote storage; "+
 			"run '"+snapshotRemoveCommandHint+"' with the snapshot's ID "+
@@ -1422,10 +1407,10 @@ func (v *Vaultik) removeSnapshotRemote(
 				err, snapshotRemoveCommandHint, snapshotID)
 		}
 
-		return false, nil
+		return false
 	}
 
-	return deleted > 0, nil
+	return deleted > 0
 }
 
 // RemoveAllSnapshots removes every snapshot known to the local

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