diff --git a/README.md b/README.md index f4c5f61..471501e 100644 --- a/README.md +++ b/README.md @@ -386,10 +386,12 @@ manifest on the destination store to compute the still-referenced blob set and delete any blob not in that set. Step (3) reads all remote manifests — network cost scales with the number of snapshots. `snapshot create --prune` runs the same cleanup automatically; this is the -manual entry point for the same work. +manual entry point for the same work. If a blob cannot be deleted, the +command (and `snapshot create --prune`) exits non-zero. * `--force`: Skip confirmation prompt * `--json`: Output stats as JSON. Also skips the confirmation prompt, as - `--force` does. + `--force` does. A blob that cannot be deleted is counted in + `blobs_failed`, and the document is printed before the non-zero exit. **`info`**: Display system configuration, storage settings, encryption recipients, and local database statistics. @@ -411,7 +413,8 @@ since `prune` never deletes it. **`remote nuke`**: Delete every snapshot's metadata and every blob from the backup destination store, leaving the bucket prefix empty. Destructive and irreversible. This is the single supported way to wipe the entire -destination store. +destination store. If anything cannot be deleted, the command exits +non-zero; running it again deletes what is left. * `--force`: Required to confirm destruction. **`database delete`**: Delete the local SQLite state database file diff --git a/internal/storage/faultstore/faultstore.go b/internal/storage/faultstore/faultstore.go index ae5d889..bdc4dc3 100644 --- a/internal/storage/faultstore/faultstore.go +++ b/internal/storage/faultstore/faultstore.go @@ -1,15 +1,16 @@ // Package faultstore provides a storage.Storer wrapper that injects // faults on demand, so tests can reproduce the failure modes a real // backend exhibits: an upload that fails partway, a backend that reports -// success while storing nothing, and reads that return corrupt or -// truncated bytes. It is the seam called for by the fault-injection -// tests (sneak/vaultik issue 72) and is meant to be reused by future -// tests rather than re-implemented per case. +// success while storing nothing, reads that return corrupt or truncated +// bytes, and a delete the backend refuses. It is the seam called for by +// the fault-injection tests (sneak/vaultik issue 72) and is meant to be +// reused by future tests rather than re-implemented per case. // -// The wrapper delegates every method to the inner Storer. Two hooks -// change that: OnPut decides the fate of each write, and OnGet decides -// how each read's bytes are returned. Both are keyed by the object key, -// so a test can fault only blobs, only metadata, or a single object. +// The wrapper delegates every method to the inner Storer. Three hooks +// change that: OnPut decides the fate of each write, OnGet decides how +// each read's bytes are returned, and OnDelete decides whether each +// delete fails. All are keyed by the object key, so a test can fault +// only blobs, only metadata, or a single object. package faultstore import ( @@ -25,6 +26,10 @@ import ( // ErrInjectedUpload is returned by a Put the OnPut hook chose to fail. var ErrInjectedUpload = errors.New("faultstore: injected upload failure") +// ErrInjectedDelete is returned by a Delete the OnDelete hook chose to +// fail. +var ErrInjectedDelete = errors.New("faultstore: injected delete failure") + // PutAction is the disposition OnPut assigns to a write. type PutAction int @@ -66,6 +71,11 @@ type Storer struct { // OnGet, when set, is consulted for every Get with the object key // and damages the returned bytes accordingly. OnGet func(key string) GetFault + + // OnDelete, when set, is consulted before every Delete with the + // object key. Returning true fails the delete and leaves the object + // in place. + OnDelete func(key string) bool } // New wraps inner. inner must be non-nil. @@ -154,8 +164,12 @@ func (f *Storer) Stat(ctx context.Context, key string) (*storage.ObjectInfo, err return f.inner.Stat(ctx, key) } -// Delete delegates unchanged. +// Delete removes the object unless OnDelete fails the delete. func (f *Storer) Delete(ctx context.Context, key string) error { + if f.OnDelete != nil && f.OnDelete(key) { + return fmt.Errorf("%w for %q", ErrInjectedDelete, key) + } + return f.inner.Delete(ctx, key) } diff --git a/internal/vaultik/nuke_remote_test.go b/internal/vaultik/nuke_remote_test.go index 6e22699..701b0a5 100644 --- a/internal/vaultik/nuke_remote_test.go +++ b/internal/vaultik/nuke_remote_test.go @@ -5,12 +5,14 @@ import ( "io/fs" "os" "path/filepath" + "strings" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "sneak.berlin/go/vaultik/internal/log" "sneak.berlin/go/vaultik/internal/snapshot" + "sneak.berlin/go/vaultik/internal/storage/faultstore" ) // TestNukeRemoteLeavesNoFiles checks that remote nuke leaves no file @@ -23,7 +25,7 @@ func TestNukeRemoteLeavesNoFiles(t *testing.T) { ctx := context.Background() storeDir := filepath.Join(t.TempDir(), "store") - v, repos, _ := backUpToFileDestination(ctx, t, storeDir) + v, repos, out := backUpToFileDestination(ctx, t, storeDir) snapshots, err := repos.Snapshots.ListRecent(ctx, listRecentTestLimit) require.NoError(t, err) @@ -61,4 +63,82 @@ func TestNukeRemoteLeavesNoFiles(t *testing.T) { }) require.NoError(t, err) assert.Empty(t, files) + + // Nuke deletes the blobs itself, so advice to run prune is wrong. + assert.Contains(t, out.String(), "Backup destination store is now empty.") + assert.NotContains(t, out.String(), "vaultik prune") +} + +// TestNukeRemoteFailsWhenADeleteFails checks that remote nuke returns an +// error when a snapshot's metadata or a blob cannot be deleted, does not +// report the store empty, and advises running nuke again: prune deletes +// no metadata. +func TestNukeRemoteFailsWhenADeleteFails(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + for _, refusedPrefix := range []string{"metadata/", "blobs/"} { + t.Run(refusedPrefix, func(t *testing.T) { + t.Parallel() + + ctx := context.Background() + storeDir := filepath.Join(t.TempDir(), "store") + v, _, out := backUpToFileDestination(ctx, t, storeDir) + + store := faultstore.New(v.Storage) + store.OnDelete = func(key string) bool { + return strings.HasPrefix(key, refusedPrefix) + } + v.Storage = store + + require.Error(t, v.NukeRemote(true)) + + assert.NotContains(t, out.String(), "now empty") + assert.Contains(t, out.String(), "vaultik remote nuke --force") + assert.NotContains(t, out.String(), "vaultik prune") + }) + } +} + +// TestNukeRemoteRefusesAnotherDestinationWithoutRetryAdvice checks that +// remote nuke, when the local index belongs to a different destination +// than the configured one, fails without advising to run nuke again: +// that run would fail the same way. +func TestNukeRemoteRefusesAnotherDestinationWithoutRetryAdvice(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + ctx := context.Background() + storeDir := filepath.Join(t.TempDir(), "store") + v, _, out := backUpToFileDestination(ctx, t, storeDir) + + v.Config.StorageURL = "file://" + filepath.Join(t.TempDir(), "elsewhere") + + err := v.NukeRemote(true) + + require.ErrorContains(t, err, "bound to a different backup destination") + assert.NotContains(t, out.String(), "vaultik remote nuke --force") +} + +// TestNukeRemoteFailsWhenALocalSnapshotCannotBeRemoved checks that a +// snapshot remote nuke cannot remove from the local index fails the +// run: nuke then leaves that snapshot's metadata on the destination +// store. +func TestNukeRemoteFailsWhenALocalSnapshotCannotBeRemoved(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + ctx := context.Background() + storeDir := filepath.Join(t.TempDir(), "store") + v, _, out := backUpToFileDestination(ctx, t, storeDir) + + _, err := v.DB.Conn().ExecContext(ctx, ` + CREATE TRIGGER refuse_snapshot_delete BEFORE DELETE ON snapshots + BEGIN SELECT RAISE(ABORT, 'simulated index error'); END`) + require.NoError(t, err) + + require.Error(t, v.NukeRemote(true)) + + assert.NotContains(t, out.String(), "now empty") + assert.Contains(t, out.String(), "vaultik remote nuke --force") } diff --git a/internal/vaultik/prune.go b/internal/vaultik/prune.go index 31949c9..7df20e1 100644 --- a/internal/vaultik/prune.go +++ b/internal/vaultik/prune.go @@ -19,6 +19,14 @@ type PruneOptions struct { var errNukeRequiresForce = errors.New( "nuke requires --force (this deletes ALL remote snapshots and blobs)") +// errBlobDeleteFailed reports unreferenced blobs prune could not delete. +var errBlobDeleteFailed = errors.New("blob(s) could not be deleted") + +// nukeCommandHint is the command suggested when a remote nuke could not +// delete everything: running it again deletes what is left, which +// `vaultik prune` does not do for snapshot metadata. +const nukeCommandHint = "vaultik remote nuke --force" + // metadataDirName is the top-level remote directory holding snapshot // metadata. const metadataDirName = "metadata" @@ -39,6 +47,29 @@ func (v *Vaultik) NukeRemote(force bool) error { return errNukeRequiresForce } + // Checked before anything is deleted: running nuke again would fail + // the same way, so the advice below must not follow this error. + err := v.EnsureStorageBinding() + if err != nil { + return err + } + + err = v.emptyDestinationStore() + if err != nil { + v.UI.Warningf("Run '%s' again to delete what is left on the "+ + "backup destination store.", nukeCommandHint) + + return err + } + + v.UI.Completef("Backup destination store is now empty.") + + return nil +} + +// emptyDestinationStore does the deleting for NukeRemote and stops at +// the first step that fails. +func (v *Vaultik) emptyDestinationStore() error { v.UI.Beginf("Removing all snapshot metadata from backup destination store.") _, err := v.RemoveAllSnapshots(&RemoveOptions{Force: true}) @@ -62,8 +93,6 @@ func (v *Vaultik) NukeRemote(force bool) error { } } - v.UI.Completef("Backup destination store is now empty.") - return nil } @@ -166,14 +195,17 @@ func (v *Vaultik) PruneBlobs(opts *PruneOptions) error { v.deleteUnreferencedBlobs(unreferencedBlobs, allBlobs, result) if opts.JSON { - return v.outputPruneBlobsJSON(result) + err = v.outputPruneBlobsJSON(result) + if err != nil { + return err + } + } else { + v.stdoutf("\nDeleted %d blob(s) totaling %s\n", + result.BlobsDeleted, ubytes(result.BytesFreed)) } - v.stdoutf("\nDeleted %d blob(s) totaling %s\n", - result.BlobsDeleted, ubytes(result.BytesFreed)) - if result.BlobsFailed > 0 { - v.stdoutf("Failed to delete %d blob(s)\n", result.BlobsFailed) + return fmt.Errorf("%d %w", result.BlobsFailed, errBlobDeleteFailed) } return nil diff --git a/internal/vaultik/prune_delete_failure_test.go b/internal/vaultik/prune_delete_failure_test.go new file mode 100644 index 0000000..54fe788 --- /dev/null +++ b/internal/vaultik/prune_delete_failure_test.go @@ -0,0 +1,98 @@ +package vaultik_test + +import ( + "context" + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "sneak.berlin/go/vaultik/internal/log" + "sneak.berlin/go/vaultik/internal/storage/faultstore" + "sneak.berlin/go/vaultik/internal/vaultik" +) + +// refuseBlobDeletes is an OnDelete hook that fails every blob delete. +func refuseBlobDeletes(key string) bool { + return strings.HasPrefix(key, "blobs/") +} + +// TestPruneBlobs_FailedDeleteReturnsError checks that prune returns an +// error, so the command exits non-zero, when an unreferenced blob cannot +// be deleted. +func TestPruneBlobs_FailedDeleteReturnsError(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + env := newListEnv(t) + + // The store holds no manifest, so nothing references this blob. + addBlob(t, env.store.testStorer, testBlobHashA) + + store := faultstore.New(env.store) + store.OnDelete = refuseBlobDeletes + env.v.Storage = store + + err := env.v.PruneBlobs(&vaultik.PruneOptions{Force: true}) + + require.ErrorContains(t, err, "1 blob(s) could not be deleted") +} + +// TestPruneBlobs_JSONFailedDeleteReturnsError checks that prune --json +// prints its document and then returns an error when an unreferenced +// blob cannot be deleted. +func TestPruneBlobs_JSONFailedDeleteReturnsError(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + env := newListEnv(t) + addBlob(t, env.store.testStorer, testBlobHashA) + + store := faultstore.New(env.store) + store.OnDelete = refuseBlobDeletes + env.v.Storage = store + + err := env.v.PruneBlobs(&vaultik.PruneOptions{JSON: true}) + + require.Error(t, err, "prune must fail when a blob cannot be deleted") + + var result vaultik.PruneBlobsResult + + require.NoError(t, json.Unmarshal(env.stdout.Bytes(), &result), + "stdout must hold the JSON document, got:\n%s", env.stdout.String()) + assert.Equal(t, 1, result.BlobsFailed) + assert.Equal(t, 0, result.BlobsDeleted) +} + +// TestCreateSnapshotPrune_FailedBlobDeleteReturnsError checks that the +// prune step of snapshot create --prune fails the run when an +// unreferenced blob cannot be deleted. +func TestCreateSnapshotPrune_FailedBlobDeleteReturnsError(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + ctx := context.Background() + storeDir := filepath.Join(t.TempDir(), "store") + v, _, _ := backUpToFileDestination(ctx, t, storeDir) + + // No snapshot references this blob, so the prune step deletes it. + orphan := filepath.Join(storeDir, "blobs", testBlobHashA[:2], + testBlobHashA[2:4], testBlobHashA) + require.NoError(t, os.MkdirAll(filepath.Dir(orphan), 0o750)) + require.NoError(t, os.WriteFile(orphan, []byte("orphan"), 0o600)) + + store := faultstore.New(v.Storage) + store.OnDelete = refuseBlobDeletes + v.Storage = store + + err := v.CreateSnapshot(&vaultik.SnapshotCreateOptions{ + Cron: true, + Prune: true, + }) + + require.ErrorContains(t, err, "1 blob(s) could not be deleted") + assert.FileExists(t, orphan) +} diff --git a/internal/vaultik/remove_snapshot_test.go b/internal/vaultik/remove_snapshot_test.go index 076ccdd..7d85bea 100644 --- a/internal/vaultik/remove_snapshot_test.go +++ b/internal/vaultik/remove_snapshot_test.go @@ -476,7 +476,8 @@ func TestRemoveAllSnapshots_WithForce(t *testing.T) { 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, "vaultik prune") + // Remote nuke, the only caller, deletes the blobs next itself. + assert.NotContains(t, out, "vaultik prune") } func TestRemoveAllSnapshots_DryRun(t *testing.T) { diff --git a/internal/vaultik/snapshot.go b/internal/vaultik/snapshot.go index d5bdb19..630e2ba 100644 --- a/internal/vaultik/snapshot.go +++ b/internal/vaultik/snapshot.go @@ -25,6 +25,7 @@ var ( errSnapshotVerifyFailed = errors.New("verification failed") errRemoveAllNeedsForce = errors.New("--all requires --force") errSnapshotNotFound = errors.New("snapshot not found") + errSnapshotRemoveFailed = errors.New("snapshot(s) could not be removed") errInvalidTableName = errors.New("invalid table name") errNoAgeRecipients = errors.New( "creating a snapshot needs at least one public key in " + @@ -1591,8 +1592,8 @@ func (v *Vaultik) handleRemoveAllDryRun( // executeRemoveAll deletes every local snapshot and, unless LocalOnly // is set, every corresponding remote metadata directory plus any -// orphan remote keys. Blobs are not touched; the printed prune-command -// hint is the next step. +// orphan remote keys. Blobs are not touched. It returns an error when +// any snapshot could not be removed. func (v *Vaultik) executeRemoveAll( localSnaps, orphanRemoteKeys []string, opts *RemoveOptions, ) (*RemoveResult, error) { @@ -1606,18 +1607,11 @@ func (v *Vaultik) executeRemoveAll( result := &RemoveResult{} - remoteErrors := v.removeAllLocalSnapshots(localSnaps, opts, result) + failures := v.removeAllLocalSnapshots(localSnaps, opts, result) if !opts.LocalOnly { - remoteErrors += v.removeAllOrphanRemotes(orphanRemoteKeys, result) - - if remoteErrors == 0 { - result.RemoteRemoved = true - } else if v.UI != nil { - v.UI.Warningf("Some remote metadata deletions failed. Run '%s' "+ - "once the remote is healthy to clean up unreferenced blobs.", - pruneCommandHint) - } + failures += v.removeAllOrphanRemotes(orphanRemoteKeys, result) + result.RemoteRemoved = failures == 0 } if v.SnapshotManager != nil { @@ -1627,6 +1621,10 @@ func (v *Vaultik) executeRemoveAll( } } + if failures > 0 { + return result, fmt.Errorf("%d %w", failures, errSnapshotRemoveFailed) + } + if opts.JSON { return result, v.outputRemoveJSON(result) } @@ -1635,9 +1633,6 @@ func (v *Vaultik) executeRemoveAll( if !opts.LocalOnly && result.RemoteRemoved { v.printlnStdout("Removed snapshot metadata from remote storage") - v.stdoutf("\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 @@ -1680,12 +1675,13 @@ func (v *Vaultik) deleteIncompleteSnapshot(snapshotIDStr string, id any) bool { } // removeAllLocalSnapshots removes each known local snapshot (and, unless -// --local-only, its remote metadata), returning the number of remote -// deletion failures. +// --local-only, its remote metadata), returning the number of snapshots +// it could not remove. A snapshot left in the local index keeps its +// remote metadata too, since it is not an orphan remote key. func (v *Vaultik) removeAllLocalSnapshots( localSnaps []string, opts *RemoveOptions, result *RemoveResult, ) int { - remoteErrors := 0 + failures := 0 for _, snapshotID := range localSnaps { log.Info("Removing snapshot", "snapshot_id", snapshotID) @@ -1695,6 +1691,8 @@ func (v *Vaultik) removeAllLocalSnapshots( log.Error("Failed to remove from local database", "snapshot_id", snapshotID, "error", err) + failures++ + continue } @@ -1705,14 +1703,14 @@ func (v *Vaultik) removeAllLocalSnapshots( log.Warn("Failed to remove snapshot metadata from remote", "snapshot_id", snapshotID, "error", err) - remoteErrors++ + failures++ } } result.SnapshotsRemoved = append(result.SnapshotsRemoved, snapshotID) } - return remoteErrors + return failures } // removeAllOrphanRemotes deletes remote snapshot metadata that has no