diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 8079402..d0383a0 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -385,11 +385,12 @@ bucket/ A destination without `vaultik-destination.txt` is treated as not present while the local index holds snapshots stored there (`checkDestinationMarker` in `internal/vaultik/destination_marker.go`): `snapshot create` refuses to -write to it, `prune` and `snapshot purge` fail, and `snapshot list` and -`snapshot remove` behave as for a destination that cannot be listed. This -keeps those commands from taking an unplugged volume's empty mount point for -a destination whose snapshots are all gone. The `file://` backend never -creates its destination directory, so a write to a missing one fails. +write to it, `prune`, `snapshot purge` and `remote nuke` fail, and +`snapshot list` and `snapshot remove` behave as for a destination that cannot +be listed. This keeps those commands from taking an unplugged volume's empty +mount point for a destination whose snapshots are all gone. The `file://` +backend never creates its destination directory, so a write to a missing one +fails. The `{remote-key}` directory name is a one-way double SHA-256 hash of the human snapshot ID, so the human ID (hostname, snapshot name, timestamp) is never diff --git a/README.md b/README.md index a84d9ff..f1facea 100644 --- a/README.md +++ b/README.md @@ -454,9 +454,14 @@ since `prune` never deletes it. the backup destination store, whatever its name, and then `vaultik-destination.txt`, leaving the bucket prefix empty. Destructive and irreversible. This is the single supported way to wipe the entire -destination store. If a listing afterwards still finds anything under -`metadata/` or `blobs/`, the command names each object left and exits -non-zero; running it again deletes what is left. +destination store. A snapshot is removed from the local index only once +its metadata is deleted from the destination store. If the destination +store cannot be listed, or the local index holds snapshots and the +destination has no `vaultik-destination.txt` (an unplugged volume's +empty mount point), the command fails before it removes anything. If a +listing afterwards still finds anything under `metadata/` or `blobs/`, +the command names each object left and exits non-zero; running it again +deletes what is left. * `--force`: Required to confirm destruction. **`database delete`**: Delete the local SQLite state database file @@ -529,10 +534,11 @@ For an implementation-level view of the internals — the data model, the * `vaultik-destination.txt` holds one fixed line of text, with no host name, path or time. A destination without it is treated as not present while the local index holds snapshots stored there: - `snapshot create` refuses to write to it, `prune` and `snapshot purge` - fail, and `snapshot list` and `snapshot remove` behave as for a - destination that cannot be listed. This keeps an unplugged volume's empty mount point - from being taken for a destination whose snapshots are all gone. Mount + `snapshot create` refuses to write to it, `prune`, `snapshot purge` + and `remote nuke` fail, and `snapshot list` and `snapshot remove` + behave as for a destination that cannot be listed. This keeps an + unplugged volume's empty mount point from being taken for a + destination whose snapshots are all gone. Mount the volume if it is removable; if the destination was emptied on purpose, run `vaultik database delete` and back up again. * Blobs are two-level directory sharded using the first 4 hex chars of the blob hash diff --git a/docs/REPOSTRUCTURE.md b/docs/REPOSTRUCTURE.md index 36711c7..6a0661a 100644 --- a/docs/REPOSTRUCTURE.md +++ b/docs/REPOSTRUCTURE.md @@ -27,7 +27,7 @@ The metadata subdirectory is named with the **remote key**, a one-way hash of th ## Marker File (`vaultik-destination.txt`) -Every backup writes this file at the root before it uploads anything. It holds one fixed line of plain text and names no host, path or time. A destination without it is treated as not present while the local index holds snapshots stored there: `snapshot create` refuses to write to it, and `prune`, `snapshot purge`, `snapshot remove` and `snapshot list` behave as for a destination that cannot be listed. An unplugged volume leaves an empty directory at its mount point, and without the marker that directory would read as a destination whose snapshots are all gone. `remote nuke` deletes the marker last. +Every backup writes this file at the root before it uploads anything. It holds one fixed line of plain text and names no host, path or time. A destination without it is treated as not present while the local index holds snapshots stored there: `snapshot create` refuses to write to it, and `prune`, `snapshot purge`, `snapshot remove`, `snapshot list` and `remote nuke` behave as for a destination that cannot be listed. An unplugged volume leaves an empty directory at its mount point, and without the marker that directory would read as a destination whose snapshots are all gone. `remote nuke` deletes the marker last. ## Blobs Directory (`blobs/`) diff --git a/internal/vaultik/destination_marker_test.go b/internal/vaultik/destination_marker_test.go index 03775d6..08cb35b 100644 --- a/internal/vaultik/destination_marker_test.go +++ b/internal/vaultik/destination_marker_test.go @@ -152,6 +152,25 @@ func TestPurgeRefusesDestinationWithoutMarker(t *testing.T) { assertUntouched(ctx, t, repos, storeDir) } +// TestNukeRemoteRefusesDestinationWithoutMarker checks that remote nuke +// fails, deletes no local snapshot record, and neither reports the +// destination empty nor advises running nuke again when the destination +// has no marker. +func TestNukeRemoteRefusesDestinationWithoutMarker(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + ctx := context.Background() + v, repos, out, storeDir := backUpThenUnplugMountPoint(ctx, t) + + err := v.NukeRemote(true) + require.ErrorContains(t, err, markerFile) + assert.NotContains(t, out.String(), "now empty") + assert.NotContains(t, out.String(), "vaultik remote nuke --force") + + assertUntouched(ctx, t, repos, storeDir) +} + // TestRemoveSnapshotWarnsWhenDestinationHasNoMarker checks that snapshot // remove warns that the metadata could not be removed from a destination // without a marker, instead of reporting that it was. diff --git a/internal/vaultik/missing_destination_test.go b/internal/vaultik/missing_destination_test.go index cf8cc54..1ab5a5c 100644 --- a/internal/vaultik/missing_destination_test.go +++ b/internal/vaultik/missing_destination_test.go @@ -188,3 +188,41 @@ func TestPurgeSaysListingFailedOnceWhenDestinationMissing(t *testing.T) { assert.Equal(t, 1, strings.Count(err.Error(), "listing remote snapshots"), err.Error()) } + +// TestNukeRemoteKeepsLocalIndexWhenDestinationMissing checks that remote +// nuke fails on a destination it cannot list, leaves the local index +// unchanged, and neither reports the destination empty nor advises +// running nuke again. +func TestNukeRemoteKeepsLocalIndexWhenDestinationMissing(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + ctx := context.Background() + v, repos, out := backUpThenUnplug(ctx, t) + + filesBefore, err := repos.Files.ListAll(ctx) + require.NoError(t, err) + require.NotEmpty(t, filesBefore) + + blobsBefore, err := repos.Blobs.GetAll(ctx) + require.NoError(t, err) + require.NotEmpty(t, blobsBefore) + + err = v.NukeRemote(true) + require.ErrorIs(t, err, fs.ErrNotExist) + require.ErrorContains(t, err, "listing remote snapshots") + assert.NotContains(t, out.String(), "now empty") + assert.NotContains(t, out.String(), "vaultik remote nuke --force") + + snapshots, err := repos.Snapshots.ListRecent(ctx, listRecentTestLimit) + require.NoError(t, err) + assert.Len(t, snapshots, 1, "nuke must delete no local snapshot record") + + files, err := repos.Files.ListAll(ctx) + require.NoError(t, err) + assert.Len(t, files, len(filesBefore), "nuke must delete no local file record") + + blobs, err := repos.Blobs.GetAll(ctx) + require.NoError(t, err) + assert.Len(t, blobs, len(blobsBefore), "nuke must delete no local blob record") +} diff --git a/internal/vaultik/nuke_remote_test.go b/internal/vaultik/nuke_remote_test.go index 04addda..63ebcbe 100644 --- a/internal/vaultik/nuke_remote_test.go +++ b/internal/vaultik/nuke_remote_test.go @@ -104,6 +104,32 @@ func TestNukeRemoteFailsWhenADeleteFails(t *testing.T) { } } +// TestNukeRemoteKeepsLocalSnapshotWhoseMetadataIsNotDeleted checks that +// remote nuke keeps a snapshot in the local index while its metadata +// cannot be deleted from the destination store: dropped from the local +// index, it would be left there as a remote-only snapshot without its +// name. +func TestNukeRemoteKeepsLocalSnapshotWhoseMetadataIsNotDeleted(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + ctx := context.Background() + storeDir := filepath.Join(t.TempDir(), "store") + v, repos, _ := backUpToFileDestination(ctx, t, storeDir) + + store := faultstore.New(v.Storage) + store.OnDelete = func(key string) bool { + return strings.HasPrefix(key, "metadata/") + } + v.Storage = store + + require.Error(t, v.NukeRemote(true)) + + snapshots, err := repos.Snapshots.ListRecent(ctx, listRecentTestLimit) + require.NoError(t, err) + assert.Len(t, snapshots, 1) +} + // TestNukeRemoteNamesWhatIsLeft checks that remote nuke, when an object // it does not recognise cannot be deleted, does not report the store // empty but names that object and fails. @@ -154,8 +180,7 @@ func TestNukeRemoteRefusesAnotherDestinationWithoutRetryAdvice(t *testing.T) { // 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. +// run, and that nuke then does not report the destination store empty. func TestNukeRemoteFailsWhenALocalSnapshotCannotBeRemoved(t *testing.T) { log.Initialize(log.Config{}) t.Parallel() diff --git a/internal/vaultik/prune.go b/internal/vaultik/prune.go index f8e68cc..b7148e9 100644 --- a/internal/vaultik/prune.go +++ b/internal/vaultik/prune.go @@ -55,12 +55,19 @@ func (v *Vaultik) NukeRemote(force bool) error { } // Checked before anything is deleted: running nuke again would fail - // the same way, so the advice below must not follow this error. + // the same way, so the advice below must not follow these errors. + // The listing also fails on a destination without the marker while + // the local index holds snapshots. err := v.EnsureStorageBinding() if err != nil { return err } + _, err = v.listAllRemoteSnapshotKeys() + if err != nil { + return err + } + err = v.emptyDestinationStore() if err != nil { v.UI.Warningf("Run '%s' again to delete what is left on the "+ diff --git a/internal/vaultik/snapshot.go b/internal/vaultik/snapshot.go index 7c9b5fe..6d74865 100644 --- a/internal/vaultik/snapshot.go +++ b/internal/vaultik/snapshot.go @@ -1516,7 +1516,9 @@ func (v *Vaultik) removeSnapshotRemote( // database from the local index, and (with --remote) every snapshot // metadata directory in remote storage. Both sides are processed so a // "remove --all" leaves nothing behind, even when the local DB and -// remote storage have diverged. +// remote storage have diverged. Unless LocalOnly is set, it fails +// before it removes anything when the destination store cannot be +// listed or is not present by checkDestinationMarker. func (v *Vaultik) RemoveAllSnapshots(opts *RemoveOptions) (*RemoveResult, error) { err := v.EnsureStorageBinding() if err != nil { @@ -1529,11 +1531,15 @@ func (v *Vaultik) RemoveAllSnapshots(opts *RemoveOptions) (*RemoveResult, error) } // remoteKeys is the set of metadata// subdirectories on the - // destination store; failures are downgraded to a warning so a - // permission-denied or unreachable remote can't block a local-only - // remove. + // destination store. Only for a local-only remove is a failure + // downgraded to a warning, so a permission-denied or unreachable + // remote can't block it. remoteKeys, remoteErr := v.listAllRemoteSnapshotKeys() if remoteErr != nil { + if !opts.LocalOnly { + return nil, remoteErr + } + log.Warn("Could not list remote snapshots", "error", remoteErr) v.UI.Warningf("Could not list remote snapshots: %v.", remoteErr) } @@ -1595,8 +1601,7 @@ func (v *Vaultik) localSnapshotIDs() ([]string, error) { // listAllRemoteSnapshotKeys collects the hashed remote keys // (subdirectories under metadata/) currently present in the // destination store. Returns (nil, err) when the store cannot be -// listed, or is not present by checkDestinationMarker; callers must -// treat that as "no remote info available," not fatal. +// listed, or is not present by checkDestinationMarker. func (v *Vaultik) listAllRemoteSnapshotKeys() ([]string, error) { log.Info("Listing all remote snapshots") @@ -1780,10 +1785,12 @@ func (v *Vaultik) deleteIncompleteSnapshot(snapshotIDStr string, id any) bool { return true } -// removeAllLocalSnapshots removes each known local snapshot (and, unless -// --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. +// removeAllLocalSnapshots removes each known local snapshot's remote +// metadata, unless --local-only, and then its local record, returning +// the number of snapshots it could not remove. The local record goes +// only once the metadata is deleted: dropped first, it would leave that +// metadata on the destination store as a remote-only snapshot whose name +// is lost. func (v *Vaultik) removeAllLocalSnapshots( localSnaps []string, opts *RemoveOptions, result *RemoveResult, ) int { @@ -1792,6 +1799,19 @@ func (v *Vaultik) removeAllLocalSnapshots( for _, snapshotID := range localSnaps { log.Info("Removing snapshot", "snapshot_id", snapshotID) + if !opts.LocalOnly { + _, err := v.deleteRemoteSnapshotByKey( + snapshot.RemoteSnapshotKey(snapshotID)) + if err != nil { + log.Warn("Failed to remove snapshot metadata from remote", + "snapshot_id", snapshotID, "error", err) + + failures++ + + continue + } + } + err := v.deleteSnapshotFromLocalDB(snapshotID) if err != nil { log.Error("Failed to remove from local database", @@ -1802,17 +1822,6 @@ func (v *Vaultik) removeAllLocalSnapshots( continue } - if !opts.LocalOnly { - _, err = v.deleteRemoteSnapshotByKey( - snapshot.RemoteSnapshotKey(snapshotID)) - if err != nil { - log.Warn("Failed to remove snapshot metadata from remote", - "snapshot_id", snapshotID, "error", err) - - failures++ - } - } - result.SnapshotsRemoved = append(result.SnapshotsRemoved, snapshotID) }