From c17426b55691ec537f94ebb1a9955eb47aea1336 Mon Sep 17 00:00:00 2001 From: sneak Date: Sun, 28 Jun 2026 06:20:09 +0200 Subject: [PATCH] snapshot rm: remove metadata only, print prune command for blobs The previous change had snapshot rm auto-prune unreferenced blobs. The correct division of labor is: rm removes a snapshot (local DB + remote metadata), prune cleans up blobs. Reverting the auto-prune means rm stays a cheap, deterministic operation: it touches one snapshot's worth of state and emits the exact 'vaultik prune' command the user should run next to delete blobs no longer referenced by any remote manifest. This is correct because prune must consult every remote manifest (including snapshots this host doesn't know about) to determine which blobs are still referenced, and folding that work into rm would silently turn rm into an expensive O(remote snapshots) operation that also assumes the remote is fully reachable. --- README.md | 17 ++-- internal/cli/snapshot.go | 11 ++- internal/vaultik/remove_snapshot_test.go | 30 +++---- internal/vaultik/snapshot.go | 103 ++++++----------------- 4 files changed, 54 insertions(+), 107 deletions(-) diff --git a/README.md b/README.md index 76ead69..2c5b1a9 100644 --- a/README.md +++ b/README.md @@ -199,13 +199,16 @@ latest globally). * `--force`: Skip confirmation prompt **`snapshot remove`**: Remove a snapshot. By default this removes the -snapshot from the local index, strips the snapshot's metadata from the -backup destination store, and prunes any blobs that are no longer -referenced by any remaining remote manifest. Local row cleanup (files, -chunks, blobs the snapshot was the last referrer for) runs automatically; -no separate prune step is needed. If the destination store is unreachable, -the local-DB removal still completes and a warning is emitted; rerun -`vaultik prune` once the store is reachable to finish remote cleanup. +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. 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; rerun `vaultik prune` +once the store is reachable to finish remote cleanup. * `--local-only`: Skip remote cleanup; only touch the local index * `--all`: Remove all snapshots (requires `--force`) * `--dry-run`: Show what would be deleted without deleting diff --git a/internal/cli/snapshot.go b/internal/cli/snapshot.go index d83c02f..7c733ad 100644 --- a/internal/cli/snapshot.go +++ b/internal/cli/snapshot.go @@ -324,12 +324,15 @@ func newSnapshotRemoveCommand() *cobra.Command { cmd := &cobra.Command{ Use: "remove [snapshot-id]", Aliases: []string{"rm"}, - Short: "Remove a snapshot from local index and remote storage", + Short: "Remove a snapshot from local index and remote metadata", Long: `Removes a snapshot. -By default, this removes the snapshot from the local index database, strips -the snapshot's metadata from the backup destination store, and prunes any -blobs that are no longer referenced by any remaining remote snapshot. +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. Use --local-only to skip the remote half (e.g. when you want to forget a snapshot locally without touching the destination store). diff --git a/internal/vaultik/remove_snapshot_test.go b/internal/vaultik/remove_snapshot_test.go index 7c719a7..b61b722 100644 --- a/internal/vaultik/remove_snapshot_test.go +++ b/internal/vaultik/remove_snapshot_test.go @@ -209,7 +209,6 @@ func TestRemoveSnapshot_LocalOnly_PreservesRemote(t *testing.T) { require.NoError(t, err) assert.Equal(t, "snapshot-001", result.SnapshotID) assert.False(t, result.RemoteRemoved) - assert.Equal(t, 0, result.BlobsDeleted) assert.True(t, store.hasKey("blobs/aa/aa/"+blobA)) assert.True(t, store.hasKey(remoteKeyPath("snapshot-001", "manifest.json.zst"))) @@ -217,12 +216,12 @@ func TestRemoveSnapshot_LocalOnly_PreservesRemote(t *testing.T) { assert.Contains(t, tv.Stdout.String(), "Removed snapshot 'snapshot-001' from local database") } -// TestRemoveSnapshot_DefaultFullCleanup is the canonical case: no -// flags. The local-DB entry is removed, the snapshot's metadata is -// removed from the destination store, and any blob that was unique to -// this snapshot (i.e. not referenced by any remaining manifest) is -// pruned from the destination store too. -func TestRemoveSnapshot_DefaultFullCleanup(t *testing.T) { +// TestRemoveSnapshot_DefaultRemovesMetadataNotBlobs is the canonical +// case: no flags. The local-DB entry and the snapshot's remote metadata +// are removed; blobs stay on the destination store. The user must then +// run `vaultik prune` to delete blobs no longer referenced by any +// remaining remote manifest, and the output prints that exact command. +func TestRemoveSnapshot_DefaultRemovesMetadataNotBlobs(t *testing.T) { log.Initialize(log.Config{}) store := newTestStorer() @@ -243,21 +242,18 @@ func TestRemoveSnapshot_DefaultFullCleanup(t *testing.T) { require.NoError(t, err) assert.Equal(t, "snapshot-001", result.SnapshotID) assert.True(t, result.RemoteRemoved) - assert.Equal(t, 1, result.BlobsDeleted, "exactly the unique blob should be deleted") - // Snapshot-001's metadata gone. assert.False(t, store.hasKey(remoteKeyPath("snapshot-001", "manifest.json.zst"))) - // Snapshot-002 untouched. assert.True(t, store.hasKey(remoteKeyPath("snapshot-002", "manifest.json.zst"))) - // Unique blob deleted. - assert.False(t, store.hasKey("blobs/aa/aa/"+blobUnique)) - // Shared blob preserved (still referenced by snapshot-002). + // Blobs are intentionally NOT touched — that's what `vaultik prune` + // is for. + assert.True(t, store.hasKey("blobs/aa/aa/"+blobUnique)) assert.True(t, store.hasKey("blobs/bb/bb/"+blobShared)) out := tv.Stdout.String() assert.Contains(t, out, "Removed snapshot 'snapshot-001' from local database") assert.Contains(t, out, "Removed snapshot metadata from remote storage") - assert.Contains(t, out, "Removed 1 unreferenced blob") + assert.Contains(t, out, "vaultik prune") } func TestRemoveSnapshot_DryRun(t *testing.T) { @@ -320,16 +316,16 @@ func TestRemoveAllSnapshots_WithForce(t *testing.T) { require.NoError(t, err) assert.Len(t, result.SnapshotsRemoved, 2) assert.True(t, result.RemoteRemoved) - assert.Equal(t, 1, result.BlobsDeleted) - assert.False(t, store.hasKey("blobs/aa/aa/"+blobA)) + // Blobs intentionally preserved — that's prune's job. + assert.True(t, store.hasKey("blobs/aa/aa/"+blobA)) assert.False(t, store.hasKey(remoteKeyPath("snapshot-001", "manifest.json.zst"))) assert.False(t, store.hasKey(remoteKeyPath("snapshot-002", "manifest.json.zst"))) out := tv.Stdout.String() assert.Contains(t, out, "Removed 2 snapshot(s)") assert.Contains(t, out, "Removed snapshot metadata from remote storage") - assert.Contains(t, out, "Removed 1 unreferenced blob") + assert.Contains(t, out, "vaultik prune") } func TestRemoveAllSnapshots_DryRun(t *testing.T) { diff --git a/internal/vaultik/snapshot.go b/internal/vaultik/snapshot.go index d678e42..02bbd7a 100644 --- a/internal/vaultik/snapshot.go +++ b/internal/vaultik/snapshot.go @@ -985,17 +985,22 @@ type RemoveResult struct { SnapshotID string `json:"snapshot_id,omitempty"` SnapshotsRemoved []string `json:"snapshots_removed,omitempty"` RemoteRemoved bool `json:"remote_removed,omitempty"` - BlobsDeleted int `json:"blobs_deleted,omitempty"` - BytesFreed int64 `json:"bytes_freed,omitempty"` DryRun bool `json:"dry_run,omitempty"` } +// pruneCommandHint is the exact command suggested after a remove +// completes so the user knows how to clean up the blobs that the +// just-removed snapshot left behind on the destination store. +const pruneCommandHint = "vaultik prune" + // RemoveSnapshot removes a snapshot from the local index database and, // unless LocalOnly is set, also strips the snapshot's metadata from the -// destination store and prunes any blobs that are no longer referenced -// by any remaining remote snapshot. When the remote is unreachable the -// command still completes the local-DB removal and warns; callers can -// retry remote cleanup later with `vaultik prune`. +// 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. func (v *Vaultik) RemoveSnapshot(snapshotID string, opts *RemoveOptions) (*RemoveResult, error) { result := &RemoveResult{ SnapshotID: snapshotID, @@ -1006,7 +1011,7 @@ func (v *Vaultik) RemoveSnapshot(snapshotID string, opts *RemoveOptions) (*Remov if !opts.JSON { v.printfStdout("Would remove snapshot: %s\n", snapshotID) if !opts.LocalOnly { - v.printlnStdout("Would also remove metadata and any unique blobs from remote storage") + v.printlnStdout("Would also remove snapshot metadata from remote storage (blobs untouched)") } v.printlnStdout("[Dry run - no changes made]") } @@ -1020,7 +1025,7 @@ func (v *Vaultik) RemoveSnapshot(snapshotID string, opts *RemoveOptions) (*Remov if opts.LocalOnly { v.printfStdout("Remove snapshot '%s' from local database (remote untouched)? [y/N] ", snapshotID) } else { - v.printfStdout("Remove snapshot '%s' from local database AND remote storage (including any blobs unique to this snapshot)? [y/N] ", snapshotID) + v.printfStdout("Remove snapshot '%s' from local database AND its metadata from remote storage? [y/N] ", snapshotID) } var confirm string if _, err := v.scanStdin(&confirm); err != nil { @@ -1043,25 +1048,16 @@ func (v *Vaultik) RemoveSnapshot(snapshotID string, opts *RemoveOptions) (*Remov log.Info("Removing snapshot metadata from remote storage", "snapshot_id", snapshotID) remoteKey := snapshot.RemoteSnapshotKey(snapshotID) if err := v.deleteRemoteSnapshotByKey(remoteKey); err != nil { - // Per design: warn-and-proceed; the local-DB removal has - // already happened, so the user can retry remote cleanup - // with `vaultik prune` later. + // Warn-and-proceed: the local-DB removal has already + // happened, so let the user know the remote half didn't + // finish and they can retry with `vaultik prune` once the + // destination store is reachable. log.Warn("Could not remove snapshot metadata from remote storage", "error", err) if v.UI != nil { - v.UI.Warning("Could not remove snapshot metadata from remote: %v. Run 'vaultik prune' once the remote is reachable to finish cleanup.", err) + v.UI.Warning("Could not remove snapshot metadata from remote: %v. Run '%s' once the remote is reachable to finish cleanup.", err, pruneCommandHint) } } else { result.RemoteRemoved = true - blobsDeleted, bytesFreed, pruneErr := v.pruneUnreferencedBlobsAfterRemoval() - if pruneErr != nil { - log.Warn("Failed to prune unreferenced blobs after snapshot removal", "error", pruneErr) - if v.UI != nil { - v.UI.Warning("Snapshot metadata removed, but blob cleanup failed: %v. Run 'vaultik prune' to retry.", pruneErr) - } - } else { - result.BlobsDeleted = blobsDeleted - result.BytesFreed = bytesFreed - } } } @@ -1078,47 +1074,12 @@ func (v *Vaultik) RemoveSnapshot(snapshotID string, opts *RemoveOptions) (*Remov v.printfStdout("Removed snapshot '%s' from local database\n", snapshotID) if !opts.LocalOnly && result.RemoteRemoved { v.printlnStdout("Removed snapshot metadata from remote storage") - if result.BlobsDeleted > 0 { - v.printfStdout("Removed %d unreferenced blob(s) (%s freed)\n", - result.BlobsDeleted, humanize.Bytes(uint64(result.BytesFreed))) - } else { - v.printlnStdout("No blobs unique to this snapshot were found.") - } + v.printfStdout("\nNote: The removed snapshot's blobs remain on the remote. Run '%s' to delete any blobs no longer referenced by any remaining remote snapshot.\n", pruneCommandHint) } return result, nil } -// pruneUnreferencedBlobsAfterRemoval deletes blobs no longer referenced -// by any remaining remote manifest. Used by RemoveSnapshot / -// RemoveAllSnapshots after metadata has been stripped from the -// destination store; with the just-removed snapshot's manifest gone, -// any blobs that were only referenced by it become unreferenced and -// are swept here. -func (v *Vaultik) pruneUnreferencedBlobsAfterRemoval() (int, int64, error) { - referenced, err := v.collectReferencedBlobs() - if err != nil { - return 0, 0, fmt.Errorf("collecting referenced blobs: %w", err) - } - - allBlobs, err := v.listAllRemoteBlobs() - if err != nil { - return 0, 0, fmt.Errorf("listing remote blobs: %w", err) - } - - unreferenced, totalSize := v.findUnreferencedBlobs(allBlobs, referenced) - if len(unreferenced) == 0 { - return 0, 0, nil - } - - result := &PruneBlobsResult{BlobsFound: len(unreferenced)} - log.Info("Pruning unreferenced blobs after snapshot removal", - "count", len(unreferenced), - "size", humanize.Bytes(uint64(totalSize))) - v.deleteUnreferencedBlobs(unreferenced, allBlobs, result) - return result.BlobsDeleted, result.BytesFreed, nil -} - // RemoveAllSnapshots removes every snapshot known to the local // database from the local index, and (with --remote) every snapshot // metadata directory in remote storage. Both sides are processed so a @@ -1241,9 +1202,8 @@ func (v *Vaultik) handleRemoveAllDryRun(localSnaps, orphanRemoteKeys []string, o v.printfStdout(" %s\n", key) } } else { - v.printlnStdout("Would also remove from remote storage") + v.printlnStdout("Would also remove snapshot metadata from remote storage (blobs untouched)") } - v.printlnStdout("Would then prune all unreferenced blobs from remote storage") } v.printlnStdout("[Dry run - no changes made]") } @@ -1255,8 +1215,8 @@ func (v *Vaultik) handleRemoveAllDryRun(localSnaps, orphanRemoteKeys []string, o // executeRemoveAll deletes every local snapshot and, unless LocalOnly // is set, every corresponding remote metadata directory plus any -// orphan remote keys, then prunes the resulting set of unreferenced -// blobs from the destination store. +// orphan remote keys. Blobs are not touched; the printed prune-command +// hint is the next step. func (v *Vaultik) executeRemoveAll(localSnaps, orphanRemoteKeys []string, opts *RemoveOptions) (*RemoveResult, error) { if !opts.Force { return nil, fmt.Errorf("--all requires --force") @@ -1297,18 +1257,8 @@ func (v *Vaultik) executeRemoveAll(localSnaps, orphanRemoteKeys []string, opts * if remoteErrors == 0 { result.RemoteRemoved = true - blobsDeleted, bytesFreed, pruneErr := v.pruneUnreferencedBlobsAfterRemoval() - if pruneErr != nil { - log.Warn("Failed to prune unreferenced blobs after bulk removal", "error", pruneErr) - if v.UI != nil { - v.UI.Warning("Bulk metadata removal succeeded, but blob cleanup failed: %v. Run 'vaultik prune' to retry.", pruneErr) - } - } else { - result.BlobsDeleted = blobsDeleted - result.BytesFreed = bytesFreed - } } else if v.UI != nil { - v.UI.Warning("Some remote metadata deletions failed; skipping automatic blob prune. Run 'vaultik prune' once the remote is healthy.") + v.UI.Warning("Some remote metadata deletions failed. Run '%s' once the remote is healthy to clean up unreferenced blobs.", pruneCommandHint) } } @@ -1325,12 +1275,7 @@ func (v *Vaultik) executeRemoveAll(localSnaps, orphanRemoteKeys []string, opts * v.printfStdout("Removed %d snapshot(s)\n", len(result.SnapshotsRemoved)) if !opts.LocalOnly && result.RemoteRemoved { v.printlnStdout("Removed snapshot metadata from remote storage") - if result.BlobsDeleted > 0 { - v.printfStdout("Removed %d unreferenced blob(s) (%s freed)\n", - result.BlobsDeleted, humanize.Bytes(uint64(result.BytesFreed))) - } else { - v.printlnStdout("No unreferenced blobs were found.") - } + v.printfStdout("\nNote: Removed snapshots' blobs remain on the remote. Run '%s' to delete any blobs no longer referenced by any remaining remote snapshot.\n", pruneCommandHint) } return result, nil