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