From 693700dbc9c14519bf0bc61f0adc2fdee0f90df1 Mon Sep 17 00:00:00 2001 From: sneak Date: Tue, 13 Oct 2026 19:43:13 +0000 Subject: [PATCH] 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 --- README.md | 22 +++++++----- internal/cli/snapshot.go | 15 ++++---- internal/vaultik/remove_snapshot_test.go | 35 ++++++++++++++++++ internal/vaultik/snapshot.go | 46 ++++++++++++++++-------- 4 files changed, 89 insertions(+), 29 deletions(-) diff --git a/README.md b/README.md index 3ba0768..17ff3b9 100644 --- a/README.md +++ b/README.md @@ -360,16 +360,20 @@ 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. On success the command prints the exact `vaultik -prune` invocation to run as a follow-up. An ID that names no snapshot in -the local index or on the destination store is an error; with -`--local-only`, the snapshot must be in the local index. Local row +`vaultik prune` does. The command prints the `vaultik prune` invocation +to run as a follow-up only when it removed the 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; +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 the destination store is unreachable, the local-DB -removal still completes and a warning is emitted; run `vaultik snapshot -remove ` again once the store is reachable to remove the -snapshot's metadata from it (`vaultik prune` does not). To wipe everything -on the destination in one go, use `vaultik remote nuke --force`. +runs automatically. If removing the snapshot's metadata from the +destination store fails (for example because the store is unreachable), +the local-DB removal still completes and a warning is emitted; run +`vaultik snapshot remove ` again once the store is reachable +to remove 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 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 diff --git a/internal/cli/snapshot.go b/internal/cli/snapshot.go index 0997140..7fd5b73 100644 --- a/internal/cli/snapshot.go +++ b/internal/cli/snapshot.go @@ -270,8 +270,9 @@ 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. On success the command -prints the exact 'vaultik prune' invocation to run as a follow-up. +know about), which is what 'vaultik prune' does. The command prints the +'vaultik prune' invocation to run as a follow-up only when it removed the +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. @@ -280,10 +281,12 @@ 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 the remote is unreachable, the local-database removal still completes -and a warning is emitted; run 'vaultik snapshot remove ' again -once the destination store is reachable to remove the snapshot's metadata -from it ('vaultik prune' does not). +If removing the snapshot's metadata from the destination store fails (for +example because the remote is unreachable), the local-database removal +still completes and a warning is emitted; run 'vaultik snapshot remove +' again once the destination store is reachable to remove +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.`, diff --git a/internal/vaultik/remove_snapshot_test.go b/internal/vaultik/remove_snapshot_test.go index 77dfc2f..7e13f7a 100644 --- a/internal/vaultik/remove_snapshot_test.go +++ b/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() diff --git a/internal/vaultik/snapshot.go b/internal/vaultik/snapshot.go index 52a638a..d7e5c2d 100644 --- a/internal/vaultik/snapshot.go +++ b/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