From c196c1a25ec9513cf1366cde1c9f5100d68e5b94 Mon Sep 17 00:00:00 2001 From: clawbot Date: Tue, 13 Oct 2026 00:29:29 +0000 Subject: [PATCH] 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 --- README.md | 8 +- TODO.md | 10 ++ internal/cli/snapshot.go | 6 +- internal/vaultik/remove_snapshot_test.go | 115 +++++++++++++++++++++ internal/vaultik/snapshot.go | 123 +++++++++++++++++------ 5 files changed, 226 insertions(+), 36 deletions(-) diff --git a/README.md b/README.md index 1523495..c1d87ad 100644 --- a/README.md +++ b/README.md @@ -349,9 +349,11 @@ 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. Local row cleanup (files, -chunks, blobs the snapshot was the last referrer for) runs -automatically. If the destination store is unreachable, the local-DB +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 +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 diff --git a/TODO.md b/TODO.md index 61fdbc6..e124826 100644 --- a/TODO.md +++ b/TODO.md @@ -22,6 +22,16 @@ the tag exists and is exercised; what is left is merging `next` to # Completed Steps +- 2026-10-13: Made `snapshot remove` fail for a snapshot that does not + exist ([issue #293](https://git.eeqj.de/sneak/vaultik/issues/293)). + The command never checked the ID: it reported the snapshot removed + from the local database and from the destination store and exited 0, + under `--json`, `--dry-run` and `--local-only` too. An ID that names + no snapshot in the local index or on the destination store is now an + error; with `--local-only`, the snapshot must be in the local index. + The command now reports a removal from either side only when one + happened, and `remote_removed` is true only when metadata was deleted. + - 2026-10-12: Bound the local index to the destination an `s3.*` config names ([issue #298](https://git.eeqj.de/sneak/vaultik/issues/298)). Only `storage_url` was checked, so after `s3.bucket` or `s3.prefix` diff --git a/internal/cli/snapshot.go b/internal/cli/snapshot.go index 58b32db..1dc6811 100644 --- a/internal/cli/snapshot.go +++ b/internal/cli/snapshot.go @@ -255,8 +255,12 @@ 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. + Use --local-only to skip the remote half (e.g. when you want to forget a -snapshot locally without touching the destination store). +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 diff --git a/internal/vaultik/remove_snapshot_test.go b/internal/vaultik/remove_snapshot_test.go index b337c1b..076ccdd 100644 --- a/internal/vaultik/remove_snapshot_test.go +++ b/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() diff --git a/internal/vaultik/snapshot.go b/internal/vaultik/snapshot.go index 97ecfdb..2d65a0c 100644 --- a/internal/vaultik/snapshot.go +++ b/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// 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// 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// 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 -- 2.54.0