diff --git a/README.md b/README.md index 784afca..13de4f2 100644 --- a/README.md +++ b/README.md @@ -351,16 +351,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 7d85bea..f50212e 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 630e2ba..a170611 100644 --- a/internal/vaultik/snapshot.go +++ b/internal/vaultik/snapshot.go @@ -1218,9 +1218,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) { @@ -1255,8 +1257,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 { @@ -1378,19 +1382,32 @@ 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 and returns true when it deleted something; 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 "+ @@ -1405,10 +1422,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