Make snapshot remove fail for an unknown snapshot ID (closes #293)

snapshot remove never checked that the ID existed: it deleted nothing,
reported the snapshot removed from the local database and from the
destination store, and exited 0, under --json, --dry-run and
--local-only too. It now looks the ID up in the local index and, only
when that has no row and --local-only is not set, on the destination
store; finding neither is an error. The command reports a removal from
the local database only when the local index had the row, and
remote_removed is true only when metadata was actually deleted.

Judgement call: under --local-only the destination store is not
consulted, so the snapshot must be in the local index.

Model: opus-5-5

internal/vaultik/remove_snapshot_test.go

@@ -3,17 +3,21 @@ package vaultik_test
 import (
 	"bytes"
 	"context"
+	"database/sql"
 	"io"
 	"strings"
 	"sync"
 	"testing"
+	"time"
 
 	"github.com/klauspost/compress/zstd"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/vaultik/internal/database"
 	"sneak.berlin/go/vaultik/internal/log"
 	"sneak.berlin/go/vaultik/internal/snapshot"
 	"sneak.berlin/go/vaultik/internal/storage"
+	"sneak.berlin/go/vaultik/internal/types"
 	"sneak.berlin/go/vaultik/internal/vaultik"
 )
 
@@ -214,6 +218,37 @@ func addBlob(t *testing.T, store *testStorer, hash string) {
 	require.NoError(t, err)
 }
 
+// newLocalIndex returns an in-memory local index holding a completed
+// snapshot row for each of ids.
+func newLocalIndex(t *testing.T, ids ...string) *database.Repositories {
+	t.Helper()
+
+	ctx := context.Background()
+
+	db, err := database.New(ctx, ":memory:")
+	require.NoError(t, err)
+	t.Cleanup(func() { _ = db.Close() })
+
+	repos := database.NewRepositories(db)
+
+	for _, id := range ids {
+		completedAt := time.Now().UTC()
+		snap := &database.Snapshot{
+			ID:          types.SnapshotID(id),
+			Hostname:    snapHostname,
+			StartedAt:   completedAt,
+			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)
+	}
+
+	return repos
+}
+
 // ============================================================================
 // Unit Tests for RemoveSnapshot
 // ============================================================================
@@ -233,6 +268,7 @@ func TestRemoveSnapshot_LocalOnly_PreservesRemote(t *testing.T) {
 	addBlob(t, store, blobA)
 
 	tv := vaultik.NewForTesting(store)
+	tv.Repositories = newLocalIndex(t, "snapshot-001")
 
 	opts := &vaultik.RemoveOptions{Force: true, LocalOnly: true}
 	result, err := tv.RemoveSnapshot("snapshot-001", opts)
@@ -268,6 +304,7 @@ func TestRemoveSnapshot_DefaultRemovesMetadataNotBlobs(t *testing.T) {
 	addBlob(t, store, blobShared)
 
 	tv := vaultik.NewForTesting(store)
+	tv.Repositories = newLocalIndex(t, "snapshot-001", "snapshot-002")
 
 	opts := &vaultik.RemoveOptions{Force: true}
 	result, err := tv.RemoveSnapshot("snapshot-001", opts)
@@ -316,6 +353,84 @@ func TestRemoveSnapshot_DryRun(t *testing.T) {
 	assert.Contains(t, tv.Stdout.String(), "[Dry run - no changes made]")
 }
 
+// TestRemoveSnapshot_UnknownIDIsError checks that an ID naming no
+// snapshot in the local index or on the destination store is an error
+// under every option, and that nothing is reported as removed.
+func TestRemoveSnapshot_UnknownIDIsError(t *testing.T) {
+	log.Initialize(log.Config{})
+	t.Parallel()
+
+	for name, opts := range map[string]vaultik.RemoveOptions{
+		"force":      {Force: true},
+		"json":       {JSON: true},
+		"dry-run":    {Force: true, DryRun: true},
+		"local-only": {Force: true, LocalOnly: true},
+	} {
+		t.Run(name, func(t *testing.T) {
+			t.Parallel()
+
+			store := newTestStorer()
+			addManifest(t, store, "snapshot-001", nil)
+
+			tv := vaultik.NewForTesting(store)
+			tv.Repositories = newLocalIndex(t, "snapshot-001")
+
+			result, err := tv.RemoveSnapshot("snapshot-999", &opts)
+
+			require.ErrorContains(t, err, "snapshot not found")
+			assert.False(t, result.RemoteRemoved)
+			assert.Empty(t, tv.Stdout.String())
+			assert.True(t, store.hasKey(remoteKeyPath("snapshot-001")))
+		})
+	}
+}
+
+// TestRemoveSnapshot_NoMetadataOnDestination checks that removing a
+// snapshot whose metadata is not on the destination store does not
+// report metadata removed from it.
+func TestRemoveSnapshot_NoMetadataOnDestination(t *testing.T) {
+	log.Initialize(log.Config{})
+	t.Parallel()
+
+	tv := vaultik.NewForTesting(newTestStorer())
+	tv.Repositories = newLocalIndex(t, "snapshot-001")
+
+	result, err := tv.RemoveSnapshot("snapshot-001",
+		&vaultik.RemoveOptions{Force: true})
+
+	require.NoError(t, err)
+	assert.False(t, result.RemoteRemoved)
+
+	out := tv.Stdout.String()
+	assert.Contains(t, out, "Removed snapshot 'snapshot-001' from local database")
+	assert.NotContains(t, out, "Removed snapshot metadata from remote storage")
+}
+
+// TestRemoveSnapshot_NotInLocalIndex checks that removing a snapshot
+// only the destination store holds removes its metadata there without
+// reporting a removal from the local database.
+func TestRemoveSnapshot_NotInLocalIndex(t *testing.T) {
+	log.Initialize(log.Config{})
+	t.Parallel()
+
+	store := newTestStorer()
+	addManifest(t, store, "snapshot-001", nil)
+
+	tv := vaultik.NewForTesting(store)
+	tv.Repositories = newLocalIndex(t)
+
+	result, err := tv.RemoveSnapshot("snapshot-001",
+		&vaultik.RemoveOptions{Force: true})
+
+	require.NoError(t, err)
+	assert.True(t, result.RemoteRemoved)
+	assert.False(t, store.hasKey(remoteKeyPath("snapshot-001")))
+
+	out := tv.Stdout.String()
+	assert.NotContains(t, out, "from local database")
+	assert.Contains(t, out, "Removed snapshot metadata from remote storage")
+}
+
 func TestRemoveAllSnapshots_RequiresForce(t *testing.T) {
 	log.Initialize(log.Config{})
 	t.Parallel()

internal/vaultik/snapshot.go

@@ -23,6 +23,7 @@ var (
 	errNoSnapshotsInConfig  = errors.New("no snapshots configured")
 	errSnapshotVerifyFailed = errors.New("verification failed")
 	errRemoveAllNeedsForce  = errors.New("--all requires --force")
+	errSnapshotNotFound     = errors.New("snapshot not found")
 	errInvalidTableName     = errors.New("invalid table name")
 	errNoAgeRecipients      = errors.New(
 		"creating a snapshot needs at least one public key in " +
@@ -643,7 +644,7 @@ func (v *Vaultik) confirmAndExecutePurge(
 				"snapshot_id", snapshotID, "error", err)
 		}
 
-		err = v.deleteRemoteSnapshotByKey(snapshot.RemoteSnapshotKey(snapshotID))
+		_, err = v.deleteRemoteSnapshotByKey(snapshot.RemoteSnapshotKey(snapshotID))
 		if err != nil {
 			return fmt.Errorf("deleting snapshot %s from remote: %w", snapshotID, err)
 		}
@@ -1153,6 +1154,11 @@ func (v *Vaultik) RemoveSnapshot(
 		return result, err
 	}
 
+	inLocalIndex, err := v.findSnapshotToRemove(snapshotID, opts)
+	if err != nil {
+		return result, err
+	}
+
 	if opts.DryRun {
 		return v.removeSnapshotDryRun(result, snapshotID, opts)
 	}
@@ -1161,11 +1167,13 @@ func (v *Vaultik) RemoveSnapshot(
 		return result, nil
 	}
 
-	log.Info("Removing snapshot from local database", "snapshot_id", snapshotID)
+	if inLocalIndex {
+		log.Info("Removing snapshot from local database", "snapshot_id", snapshotID)
 
-	err = v.deleteSnapshotFromLocalDB(snapshotID)
-	if err != nil {
-		return result, fmt.Errorf("removing from local database: %w", err)
+		err = v.deleteSnapshotFromLocalDB(snapshotID)
+		if err != nil {
+			return result, fmt.Errorf("removing from local database: %w", err)
+		}
 	}
 
 	if !opts.LocalOnly {
@@ -1184,9 +1192,11 @@ func (v *Vaultik) RemoveSnapshot(
 		return result, v.outputRemoveJSON(result)
 	}
 
-	v.stdoutf("Removed snapshot '%s' from local database\n", snapshotID)
+	if inLocalIndex {
+		v.stdoutf("Removed snapshot '%s' from local database\n", snapshotID)
+	}
 
-	if !opts.LocalOnly && result.RemoteRemoved {
+	if result.RemoteRemoved {
 		v.printlnStdout("Removed snapshot metadata from remote storage")
 		v.stdoutf("\nNote: The removed snapshot's blobs remain on the remote. "+
 			"Run '%s' to delete any blobs no longer referenced by any "+
@@ -1196,6 +1206,45 @@ func (v *Vaultik) RemoveSnapshot(
 	return result, nil
 }
 
+// findSnapshotToRemove reports whether the local index has a row for
+// snapshotID, and returns errSnapshotNotFound when there is nothing to
+// remove: no row and, unless LocalOnly is set, no metadata on the
+// destination store either. The destination store is consulted only
+// when the local index has no row, so a snapshot in the local index can
+// still be removed from it while the destination store is unreachable.
+func (v *Vaultik) findSnapshotToRemove(
+	snapshotID string, opts *RemoveOptions,
+) (bool, error) {
+	if v.Repositories != nil {
+		snap, err := v.Repositories.Snapshots.GetByID(v.ctx, snapshotID)
+		if err != nil {
+			return false, fmt.Errorf("looking up snapshot in local index: %w", err)
+		}
+
+		if snap != nil {
+			return true, nil
+		}
+	}
+
+	if opts.LocalOnly {
+		return false, fmt.Errorf("%w in the local index: %s",
+			errSnapshotNotFound, snapshotID)
+	}
+
+	objects, err := v.remoteSnapshotObjects(snapshot.RemoteSnapshotKey(snapshotID))
+	if err != nil {
+		return false, fmt.Errorf("looking up snapshot on destination store: %w", err)
+	}
+
+	if len(objects) == 0 {
+		return false, fmt.Errorf(
+			"%w in the local index or on the destination store: %s",
+			errSnapshotNotFound, snapshotID)
+	}
+
+	return false, nil
+}
+
 // removeSnapshotDryRun reports what RemoveSnapshot would do without
 // making changes.
 func (v *Vaultik) removeSnapshotDryRun(
@@ -1255,14 +1304,14 @@ func (v *Vaultik) confirmRemoveSnapshot(snapshotID string, opts *RemoveOptions)
 // didn't finish and to run `vaultik snapshot remove` for the snapshot
 // again once the destination store is reachable (`vaultik prune` never
 // removes snapshot metadata). Returns true when the remote removal
-// succeeded.
+// succeeded and deleted something.
 func (v *Vaultik) removeSnapshotRemote(snapshotID string, opts *RemoveOptions) bool {
 	log.Info("Removing snapshot metadata from remote storage",
 		"snapshot_id", snapshotID)
 
 	remoteKey := snapshot.RemoteSnapshotKey(snapshotID)
 
-	err := v.deleteRemoteSnapshotByKey(remoteKey)
+	deleted, err := v.deleteRemoteSnapshotByKey(remoteKey)
 	if err != nil {
 		log.Warn("Could not remove snapshot metadata from remote storage; "+
 			"run '"+snapshotRemoveCommandHint+"' with the snapshot's ID "+
@@ -1280,7 +1329,7 @@ func (v *Vaultik) removeSnapshotRemote(snapshotID string, opts *RemoveOptions) b
 		return false
 	}
 
-	return true
+	return deleted > 0
 }
 
 // RemoveAllSnapshots removes every snapshot known to the local
@@ -1572,7 +1621,7 @@ func (v *Vaultik) removeAllLocalSnapshots(
 		}
 
 		if !opts.LocalOnly {
-			err = v.deleteRemoteSnapshotByKey(
+			_, err = v.deleteRemoteSnapshotByKey(
 				snapshot.RemoteSnapshotKey(snapshotID))
 			if err != nil {
 				log.Warn("Failed to remove snapshot metadata from remote",
@@ -1598,7 +1647,7 @@ func (v *Vaultik) removeAllOrphanRemotes(
 	for _, key := range orphanRemoteKeys {
 		log.Info("Removing orphan remote snapshot", "remote_key", key)
 
-		err := v.deleteRemoteSnapshotByKey(key)
+		_, err := v.deleteRemoteSnapshotByKey(key)
 		if err != nil {
 			log.Warn("Failed to remove orphan from remote",
 				"remote_key", key, "error", err)
@@ -1645,34 +1694,44 @@ func (v *Vaultik) deleteSnapshotFromLocalDB(snapshotID string) error {
 }
 
 // deleteRemoteSnapshotByKey removes everything under
-// metadata/<remoteKey>/ on the destination store. The argument is a
-// remote key (double-SHA256 derivation), not a human snapshot ID;
-// callers that have a human ID must hash via snapshot.RemoteSnapshotKey
-// first.
-func (v *Vaultik) deleteRemoteSnapshotByKey(remoteKey string) error {
-	prefix := fmt.Sprintf("metadata/%s/", remoteKey)
-	objectCh := v.Storage.ListStream(v.ctx, prefix)
-
-	var objectsToDelete []string
-
-	for object := range objectCh {
-		if object.Err != nil {
-			return fmt.Errorf("listing objects: %w", object.Err)
-		}
-
-		objectsToDelete = append(objectsToDelete, object.Key)
+// metadata/<remoteKey>/ on the destination store and returns how many
+// objects it deleted. The argument is a remote key (double-SHA256
+// derivation), not a human snapshot ID; callers that have a human ID
+// must hash via snapshot.RemoteSnapshotKey first.
+func (v *Vaultik) deleteRemoteSnapshotByKey(remoteKey string) (int, error) {
+	objectsToDelete, err := v.remoteSnapshotObjects(remoteKey)
+	if err != nil {
+		return 0, err
 	}
 
-	for _, key := range objectsToDelete {
-		err := v.Storage.Delete(v.ctx, key)
+	for i, key := range objectsToDelete {
+		err = v.Storage.Delete(v.ctx, key)
 		if err != nil {
-			return fmt.Errorf("removing %s: %w", key, err)
+			return i, fmt.Errorf("removing %s: %w", key, err)
 		}
 
 		log.Debug("Deleted remote object", "key", key)
 	}
 
-	return nil
+	return len(objectsToDelete), nil
+}
+
+// remoteSnapshotObjects lists the keys of every object under
+// metadata/<remoteKey>/ on the destination store.
+func (v *Vaultik) remoteSnapshotObjects(remoteKey string) ([]string, error) {
+	prefix := fmt.Sprintf("metadata/%s/", remoteKey)
+
+	var keys []string
+
+	for object := range v.Storage.ListStream(v.ctx, prefix) {
+		if object.Err != nil {
+			return nil, fmt.Errorf("listing objects: %w", object.Err)
+		}
+
+		keys = append(keys, object.Key)
+	}
+
+	return keys, nil
 }
 
 // outputRemoveJSON outputs the removal result as JSON