Fail snapshot remove when a destination-only snapshot's metadata cannot be deleted (closes #340)

When a snapshot was only on the destination store and deleting its
metadata there failed, snapshot remove warned and exited 0 having
removed nothing. That failure is now an error. A snapshot in the local
index keeps its behaviour: it is removed from the local index and the
failed delete on the destination store is a warning.

The README and the help text said the `vaultik prune` follow-up is
printed on every success; they now say it is printed only when metadata
was removed from the destination store, and never under --json. The
comments on RemoveSnapshot and removeSnapshotRemote describe both
cases; removeSnapshotRemote's also says it returns true only when the
delete finished without an error and removed at least one object.

Model: opus-5-5

internal/vaultik/remove_snapshot_test.go

@@ -17,6 +17,7 @@ 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"
 )
@@ -431,6 +432,40 @@ 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

@@ -1228,9 +1228,11 @@ 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 — the command prints the prune invocation to
-// run as a follow-up. When the remote is unreachable the command still
-// completes the local-DB removal and warns.
+// `vaultik prune` is for — when metadata was removed from the
+// destination store, the text output names the prune invocation to run
+// as a follow-up. When removing the metadata fails, a snapshot in the
+// 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) {
@@ -1265,8 +1267,10 @@ func (v *Vaultik) RemoveSnapshot(
 		}
 	}
 
-	if !opts.LocalOnly {
-		result.RemoteRemoved = v.removeSnapshotRemote(snapshotID, opts)
+	result.RemoteRemoved, err = v.removeSnapshotRemote(
+		snapshotID, inLocalIndex, opts)
+	if err != nil {
+		return result, err
 	}
 
 	if v.SnapshotManager != nil {
@@ -1391,19 +1395,33 @@ func (v *Vaultik) confirmRemoveSnapshot(snapshotID string, opts *RemoveOptions)
 }
 
 // removeSnapshotRemote strips the snapshot's metadata from the
-// destination store, warning and proceeding on failure: the local-DB
-// removal has already happened, so the user is told the remote half
-// 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 and deleted something.
-func (v *Vaultik) removeSnapshotRemote(snapshotID string, opts *RemoveOptions) bool {
+// destination store. It returns true only when the delete finished
+// without an error and removed at least one object; under LocalOnly it
+// does nothing. When the snapshot is only on the destination store, a
+// failure is returned as an error, because that removal is all the
+// command does. When the snapshot was in the local index, its removal
+// there has already happened, so a failure is only a warning telling the
+// user to run `vaultik snapshot remove` for the snapshot again once the
+// destination store is reachable (`vaultik 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 "+
@@ -1418,10 +1436,10 @@ func (v *Vaultik) removeSnapshotRemote(snapshotID string, opts *RemoveOptions) b
 				err, snapshotRemoveCommandHint, snapshotID)
 		}
 
-		return false
+		return false, nil
 	}
 
-	return deleted > 0
+	return deleted > 0, nil
 }
 
 // RemoveAllSnapshots removes every snapshot known to the local