From ecf36cbefa5054aa30fffd11ddb054d0e0c8d365 Mon Sep 17 00:00:00 2001 From: sneak Date: Sun, 8 Nov 2026 14:01:40 +0000 Subject: [PATCH] snapshot remove accepts a remote-only snapshot's remote key (closes #400) A host with no local index could remove a remote-only snapshot only by its plain snapshot ID, which it does not have. snapshot remove passed its argument through RemoteSnapshotKey, rehashing a remote key so it never matched. It now resolves the identifier with the same resolveSnapshotRemoteKey that verify and restore use, so a remote key or an unambiguous leading part of it works. Model: deepseek-v4-pro --- README.md | 3 ++ internal/cli/snapshot.go | 4 ++ internal/vaultik/remove_snapshot_test.go | 42 +++++++++++++++++ internal/vaultik/snapshot.go | 58 ++++++++++++++---------- 4 files changed, 83 insertions(+), 24 deletions(-) diff --git a/README.md b/README.md index f83d065..d283310 100644 --- a/README.md +++ b/README.md @@ -446,6 +446,9 @@ 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`. +* Accepts a snapshot ID, or — for a snapshot only on the destination + store — its remote key (or an unambiguous leading part of it) as shown + by `snapshot list`. * `--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 8996d35..7a3b402 100644 --- a/internal/cli/snapshot.go +++ b/internal/cli/snapshot.go @@ -288,6 +288,10 @@ 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. +The snapshot may be named by its ID or, on a host with no local index, by +the remote key that 'snapshot list' prints for a remote-only snapshot (an +unambiguous leading part is enough). + An ID that names no snapshot in the local index or on the destination store is an error. diff --git a/internal/vaultik/remove_snapshot_test.go b/internal/vaultik/remove_snapshot_test.go index 9d43aec..0bbc110 100644 --- a/internal/vaultik/remove_snapshot_test.go +++ b/internal/vaultik/remove_snapshot_test.go @@ -451,6 +451,48 @@ func TestRemoveSnapshot_NotInLocalIndex(t *testing.T) { assert.Contains(t, out, "Removed snapshot metadata from remote storage") } +// TestRemoveSnapshot_RemoteOnlyByRemoteKey checks that a snapshot only +// on the destination store, on a host with no local index, is removed by +// the remote key that 'snapshot list' prints for such a snapshot, and by +// an unambiguous leading part of it — the same identifiers verify and +// restore accept. Snapshot remove used to hash the argument again, so +// the hashed key never matched and only the plain snapshot ID worked. +func TestRemoveSnapshot_RemoteOnlyByRemoteKey(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + remoteKey := snapshot.RemoteSnapshotKey("snapshot-001") + + identifiers := map[string]string{ + "full key": remoteKey, + "leading part": remoteKey[:8], + } + + for name, identifier := range identifiers { + t.Run(name, func(t *testing.T) { + t.Parallel() + + store := newTestStorer() + addManifest(t, store, "snapshot-001", nil) + + // No local index: a host that sees the snapshot only remotely. + tv := vaultik.NewForTesting(store) + tv.Repositories = newLocalIndex(t) + + result, err := tv.RemoveSnapshot(identifier, + &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") + }) + } +} + // 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 diff --git a/internal/vaultik/snapshot.go b/internal/vaultik/snapshot.go index 68b7d00..3e9fb19 100644 --- a/internal/vaultik/snapshot.go +++ b/internal/vaultik/snapshot.go @@ -1480,7 +1480,7 @@ func (v *Vaultik) RemoveSnapshot( return result, err } - inLocalIndex, err := v.findSnapshotToRemove(snapshotID, opts) + inLocalIndex, remoteKey, err := v.findSnapshotToRemove(snapshotID, opts) if err != nil { return result, err } @@ -1511,7 +1511,7 @@ func (v *Vaultik) RemoveSnapshot( } result.RemoteRemoved, err = v.removeSnapshotRemote( - snapshotID, inLocalIndex, markerErr, opts) + snapshotID, remoteKey, inLocalIndex, markerErr, opts) if err != nil { return result, err } @@ -1544,49 +1544,58 @@ func (v *Vaultik) RemoveSnapshot( } // 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, -// and only after checkDestinationMarker passes, so an unplugged volume's -// empty mount point is not taken for a store without the snapshot. +// snapshotID, and returns the remote key to strip from the destination +// store, or 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, and only after +// checkDestinationMarker passes, so an unplugged volume's empty mount +// point is not taken for a store without the snapshot. The identifier is +// resolved the way verify and restore do: a human ID is hashed; a remote +// key, or an unambiguous leading part of one, names a remote-only +// snapshot whose metadata directory is used as-is. func (v *Vaultik) findSnapshotToRemove( snapshotID string, opts *RemoveOptions, -) (bool, error) { +) (bool, string, 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) + return false, "", fmt.Errorf("looking up snapshot in local index: %w", err) } if snap != nil { - return true, nil + return true, snapshot.RemoteSnapshotKey(snapshotID), nil } } if opts.LocalOnly { - return false, fmt.Errorf("%w in the local index: %s", + return false, "", fmt.Errorf("%w in the local index: %s", errSnapshotNotFound, snapshotID) } err := v.checkDestinationMarker() if err != nil { - return false, err + return false, "", err } - objects, err := v.remoteSnapshotObjects(snapshot.RemoteSnapshotKey(snapshotID)) + remoteKey, err := v.resolveSnapshotRemoteKey(snapshotID) if err != nil { - return false, fmt.Errorf("looking up snapshot on destination store: %w", err) + return false, "", err + } + + objects, err := v.remoteSnapshotObjects(remoteKey) + if err != nil { + return false, "", fmt.Errorf("looking up snapshot on destination store: %w", err) } if len(objects) == 0 { - return false, fmt.Errorf( + return false, "", fmt.Errorf( "%w in the local index or on the destination store: %s", errSnapshotNotFound, snapshotID) } - return false, nil + return false, remoteKey, nil } // removeSnapshotDryRun reports what RemoveSnapshot would do without @@ -1649,9 +1658,11 @@ func (v *Vaultik) confirmRemoveSnapshot(snapshotID string, opts *RemoveOptions) } // removeSnapshotRemote strips the snapshot's metadata from the -// destination store. It returns true only when the delete finished -// without an error and removed at least one object; under LocalOnly it -// does nothing. markerErr is the result of checkDestinationMarker; when +// destination store. remoteKey is the resolved metadata directory name +// to delete, as returned by findSnapshotToRemove. It returns true only +// when the delete finished without an error and removed at least one +// object; under LocalOnly it does nothing. markerErr is the result of +// checkDestinationMarker; when // it is set, nothing is deleted and it is the failure. 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 @@ -1661,7 +1672,7 @@ func (v *Vaultik) confirmRemoveSnapshot(snapshotID string, opts *RemoveOptions) // (`vaultik prune` never removes snapshot metadata that has its // manifest). func (v *Vaultik) removeSnapshotRemote( - snapshotID string, inLocalIndex bool, markerErr error, + snapshotID, remoteKey string, inLocalIndex bool, markerErr error, opts *RemoveOptions, ) (bool, error) { if opts.LocalOnly { @@ -1675,8 +1686,7 @@ func (v *Vaultik) removeSnapshotRemote( err := markerErr if err == nil { - deleted, err = v.deleteRemoteSnapshotByKey( - snapshot.RemoteSnapshotKey(snapshotID)) + deleted, err = v.deleteRemoteSnapshotByKey(remoteKey) } if err != nil && !inLocalIndex {