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

@@ -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
+snapshot from the local index and strips the snapshot's metadata from
-backup destination store, and prunes any blobs that are no longer
+the backup destination store. Blobs are NOT touched — deleting blobs
-referenced by any remaining remote manifest. Local row cleanup (files,
+requires reading every remaining remote manifest (the destination store
-chunks, blobs the snapshot was the last referrer for) runs automatically;
+may hold snapshots this host doesn't know about), which is what
-no separate prune step is needed. If the destination store is unreachable,
+`vaultik prune` does. On success the command prints the exact `vaultik
-the local-DB removal still completes and a warning is emitted; rerun
+prune` invocation to run as a follow-up. Local row cleanup (files,
-`vaultik prune` once the store is reachable to finish remote cleanup.
+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

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
+By default, this removes the snapshot from the local index database and
-the snapshot's metadata from the backup destination store, and prunes any
+strips the snapshot's metadata from the backup destination store. Blobs
-blobs that are no longer referenced by any remaining remote snapshot.
+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).

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
+// TestRemoveSnapshot_DefaultRemovesMetadataNotBlobs is the canonical
-// flags. The local-DB entry is removed, the snapshot's metadata is
+// case: no flags. The local-DB entry and the snapshot's remote metadata
-// removed from the destination store, and any blob that was unique to
+// are removed; blobs stay on the destination store. The user must then
-// this snapshot (i.e. not referenced by any remaining manifest) is
+// run `vaultik prune` to delete blobs no longer referenced by any
-// pruned from the destination store too.
+// remaining remote manifest, and the output prints that exact command.
-func TestRemoveSnapshot_DefaultFullCleanup(t *testing.T) {
+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.
+	// Blobs are intentionally NOT touched — that's what `vaultik prune`
-	assert.False(t, store.hasKey("blobs/aa/aa/"+blobUnique))
+	// is for.
-	// Shared blob preserved (still referenced by snapshot-002).
+	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) {

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
+// destination store. Blobs are NOT touched: removing a snapshot's
-// by any remaining remote snapshot. When the remote is unreachable the
+// blobs requires reading every remaining remote manifest (the remote
-// command still completes the local-DB removal and warns; callers can
+// may hold snapshots this host doesn't know about), which is what
-// retry remote cleanup later with `vaultik prune`.
+// `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
+			// Warn-and-proceed: the local-DB removal has already
-			// already happened, so the user can retry remote cleanup
+			// happened, so let the user know the remote half didn't
-			// with `vaultik prune` later.
+			// 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("\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)
-			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.")
-		}
 	}
 
 	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
+// orphan remote keys. Blobs are not touched; the printed prune-command
-// blobs from the destination store.
+// 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("\nNote: Removed snapshots' blobs remain on the remote. Run '%s' to delete any blobs no longer referenced by any remaining remote snapshot.\n", pruneCommandHint)
-			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.")
-		}
 	}
 
 	return result, nil