Merge branch 'fix/snapshot-rm-no-auto-prune'

2026-06-28 06:20:13 +02:00
parent c06d53ad45 c17426b556
commit d492f34e8d
4 changed files with 54 additions and 107 deletions
--- 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
--- 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).
--- 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) {
--- 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