Author SHA1 Message Date
sneak 635d2c27a8 Make prune and remote nuke fail when a delete fails (closes #290)
check / check (push) Waiting to run
prune, and the prune step of snapshot create --prune, counted a blob
it could not delete and still exited 0. It now prints its summary or
--json document and then returns an error, which replaces the
"Failed to delete N blob(s)" line.

remote nuke only warned when a snapshot's metadata could not be
deleted, advised vaultik prune, which never removes metadata, and then
reported the store empty. It now stops with an error, does not report
the store empty, and says to run vaultik remote nuke --force again. A
successful nuke no longer advises prune for blobs it deletes itself.

The faultstore test double gains an OnDelete hook.

Judgement call: a snapshot nuke cannot remove from the local index
also fails the run, since its metadata is then left on the store.

Model: opus-5-5
2026-10-13 02:04:43 +00:00
clawbot 07c957f84b Bind the local index to the s3.* destination too (closes #298)
check / check (push) Waiting to run
The local index was bound to its destination only through storage_url.
With the s3.* form nothing was checked, so after s3.bucket or s3.prefix
changed the next backup treated every chunk as already stored, uploaded
nothing, and completed a snapshot that could not be restored.

Without storage_url, the binding now records the destination the s3.*
fields name, written as an s3:// URL from s3.endpoint, s3.bucket and
s3.prefix, and a change is refused with the same error and advice. The
advice line now names the s3.* fields as well as storage_url.

Judgement call: the fields are compared as written, so an equivalent
spelling (a trailing slash on the prefix, a scheme on the endpoint) is
also refused, as it already is for storage_url.

Model: opus-5-5
2026-10-13 03:48:36 +02:00
10 changed files with 320 additions and 52 deletions
+8 -4
View File
@@ -382,10 +382,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.
@@ -405,14 +407,16 @@ snapshot's blob count and blob size unknown, `null` in `--json`.
**`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
entirely. Remote storage is unaffected; the next backup will do a full
scan and re-deduplicate against existing remote blobs, and the local
index will re-bind to the currently configured storage destination.
Use this after changing `storage_url` to a different destination.
Use this after changing `storage_url`, or `s3.endpoint`, `s3.bucket` or
`s3.prefix`, to a different destination.
* `--force`: Skip confirmation prompt
---
+20
View File
@@ -22,6 +22,26 @@ the tag exists and is exercised; what is left is merging `next` to
# Completed Steps
- 2026-10-13: Made `prune` and `remote nuke` exit non-zero when a delete
fails ([issue #290](https://git.eeqj.de/sneak/vaultik/issues/290)).
`prune`, and the prune step of `snapshot create --prune`, counted a
blob it could not delete and still exited 0; it now prints its summary
or `--json` document and then fails. `remote nuke` warned about a
snapshot's metadata it could not delete, said to run `vaultik prune`,
which never removes metadata, and then reported the store empty. It
now fails without that report and says to run `vaultik remote nuke
--force` again, and a successful nuke no longer advises `prune` for
blobs it deletes itself.
- 2026-10-12: Bound the local index to the destination an `s3.*` config
names ([issue #298](https://git.eeqj.de/sneak/vaultik/issues/298)).
Only `storage_url` was checked, so after `s3.bucket` or `s3.prefix`
changed, the next backup treated every chunk as already stored,
uploaded nothing, and completed a snapshot that could not be restored.
Without `storage_url`, the local index is now bound to `s3.endpoint`,
`s3.bucket` and `s3.prefix`, and a change is refused as a changed
`storage_url` is.
- 2026-10-12: Rejected a retention duration longer than about 292 years
([issue #289](https://git.eeqj.de/sneak/vaultik/issues/289)). The
number given to `--older-than` or `--keep-newer-than` was multiplied
+23 -9
View File
@@ -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)
}
+61 -1
View File
@@ -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,62 @@ 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")
})
}
}
// 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")
}
+32 -7
View File
@@ -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"
@@ -35,6 +43,22 @@ func (v *Vaultik) NukeRemote(force bool) error {
return errNukeRequiresForce
}
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})
@@ -58,8 +82,6 @@ func (v *Vaultik) NukeRemote(force bool) error {
}
}
v.UI.Completef("Backup destination store is now empty.")
return nil
}
@@ -162,14 +184,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
@@ -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.Error(t, err, "the prune step must fail the run")
assert.FileExists(t, orphan)
}
+2 -1
View File
@@ -361,7 +361,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) {
+18 -20
View File
@@ -23,6 +23,7 @@ var (
errNoSnapshotsInConfig = errors.New("no snapshots configured")
errSnapshotVerifyFailed = errors.New("verification failed")
errRemoveAllNeedsForce = errors.New("--all requires --force")
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 " +
@@ -1464,8 +1465,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) {
@@ -1479,18 +1480,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 {
@@ -1500,6 +1494,10 @@ func (v *Vaultik) executeRemoveAll(
}
}
if failures > 0 {
return result, fmt.Errorf("%d %w", failures, errSnapshotRemoveFailed)
}
if opts.JSON {
return result, v.outputRemoveJSON(result)
}
@@ -1508,9 +1506,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
@@ -1553,12 +1548,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)
@@ -1568,6 +1564,8 @@ func (v *Vaultik) removeAllLocalSnapshots(
log.Error("Failed to remove from local database",
"snapshot_id", snapshotID, "error", err)
failures++
continue
}
@@ -1578,14 +1576,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
+10 -10
View File
@@ -9,7 +9,7 @@ import (
)
// errStorageBindingMismatch is returned when the local index database is
// bound to a different destination than the configured storage_url.
// bound to a different destination than the configured one.
var errStorageBindingMismatch = errors.New(
"local index is bound to a different backup destination")
@@ -24,8 +24,9 @@ var errStorageBindingMismatch = errors.New(
//
// Behaviour:
// - On first use (empty stored value), record the configured
// storage_url and log the binding.
// - When the stored value matches the configured storage_url, do
// destination (storage_url, or the s3.* endpoint, bucket and
// prefix) and log the binding.
// - When the stored value matches the configured destination, do
// nothing and return nil.
// - When the two differ, refuse with an error that tells the user
// how to recover (revert the config, or run `vaultik database
@@ -46,12 +47,10 @@ func (v *Vaultik) EnsureStorageBinding() error {
configured := v.Config.StorageURL
if configured == "" {
// Some legacy configs still use the split s3.* keys instead of
// storage_url. Falling back to a synthetic URL for those would
// hide the fact that the binding is loose. Instead, treat
// unset as "nothing to bind against" — the check is
// necessarily best-effort for those configs.
return nil
// The s3.* form: bind on its endpoint, bucket and prefix,
// written the way storage_url would name the same destination.
s3 := v.Config.S3
configured = "s3://" + s3.Bucket + "/" + s3.Prefix + "?endpoint=" + s3.Endpoint
}
stored, err := v.Repositories.LocalMeta.Get(v.ctx, database.LocalMetaKeyStorageURL)
@@ -96,7 +95,8 @@ func buildBindingMismatchMessage(stored, configured string) string {
"future snapshots referencing blobs that don't exist at the new destination.\n" +
"\n" +
"To proceed, either:\n" +
" - revert storage_url in your config to the bound destination, or\n" +
" - revert storage_url (or the s3.* endpoint, bucket and prefix) in your\n" +
" config to the bound destination, or\n" +
" - run 'vaultik database delete' to discard the local index and rebuild it\n" +
" from a fresh full backup against the new destination"
}
+48
View File
@@ -76,3 +76,51 @@ func TestEnsureStorageBinding_MismatchRefuses(t *testing.T) {
assert.Contains(t, err.Error(), "file:///mnt/backups/new")
assert.Contains(t, err.Error(), "vaultik database delete")
}
// A config with no storage_url names its destination with s3.endpoint,
// s3.bucket and s3.prefix; a change to any of them must be refused just
// as a changed storage_url is.
func TestEnsureStorageBinding_S3FieldsChangeRefuses(t *testing.T) {
log.Initialize(log.Config{})
t.Parallel()
bound := config.S3Config{
Endpoint: "https://s3.example.com",
Bucket: "bucket-a",
Prefix: "backups",
}
tests := []struct {
name string
changed config.S3Config
}{
{"bucket", config.S3Config{
Endpoint: bound.Endpoint, Bucket: "bucket-b", Prefix: bound.Prefix,
}},
{"prefix", config.S3Config{
Endpoint: bound.Endpoint, Bucket: bound.Bucket, Prefix: "other",
}},
{"endpoint", config.S3Config{
Endpoint: "https://s3.other.com", Bucket: bound.Bucket, Prefix: bound.Prefix,
}},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
tv, _ := buildBindTestVaultik(t, "")
tv.Config.S3 = bound
require.NoError(t, tv.EnsureStorageBinding(), "first use binds")
require.NoError(t, tv.EnsureStorageBinding(), "same destination passes")
tv.Config.S3 = tt.changed
err := tv.EnsureStorageBinding()
require.Error(t, err)
assert.Contains(t, err.Error(),
"local index is bound to a different backup destination")
assert.Contains(t, err.Error(), "vaultik database delete")
})
}
}