From 19896120ffcc2aed5b7b78d90ee90a0790a7b9fb Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Wed, 11 Nov 2026 00:43:17 +0000 Subject: [PATCH] Bind the local index only after a backup writes the marker (closes #420) A first backup that failed before writing anything (a mistyped bucket, a refused credential) bound the local index to that destination, and the corrected config was then refused. A backup now checks for a different binding first, writes the destination marker, and only then records the configured destination. Purge, prune, snapshot remove and remote nuke still refuse a different binding but no longer record one: only a backup puts data in the local index, so an unbound local index holds nothing from any destination. Model: opus-5-5 --- internal/vaultik/destination_marker.go | 20 +++-- internal/vaultik/prune.go | 4 +- internal/vaultik/snapshot.go | 15 ++-- internal/vaultik/storage_bind.go | 65 ++++++++------ internal/vaultik/storage_bind_test.go | 118 +++++++++++++++++++++++++ 5 files changed, 178 insertions(+), 44 deletions(-) diff --git a/internal/vaultik/destination_marker.go b/internal/vaultik/destination_marker.go index af28fcf..66d3420 100644 --- a/internal/vaultik/destination_marker.go +++ b/internal/vaultik/destination_marker.go @@ -51,7 +51,7 @@ func (v *Vaultik) checkDestinationMarker() error { } // Config is nil only in tests, which then record no binding (see - // EnsureStorageBinding). + // checkStorageBinding). if v.Config != nil { stored, configured, err := v.storageBinding() if err != nil { @@ -83,11 +83,19 @@ func (v *Vaultik) checkDestinationMarker() error { return nil } -// markDestination checks the backup destination store with -// checkDestinationMarker and then writes the marker, so a first backup -// marks its destination and a later one writes the same text again. +// markDestination refuses a local index bound to a different +// destination, checks the backup destination store for its marker, +// writes the marker, and then records the binding. A first backup marks +// its destination and a later one writes the same text again. The +// binding is recorded last, so a first backup that cannot write to the +// destination leaves the local index unbound. func (v *Vaultik) markDestination() error { - err := v.checkDestinationMarker() + err := v.checkStorageBinding() + if err != nil { + return err + } + + err = v.checkDestinationMarker() if err != nil { return err } @@ -99,5 +107,5 @@ func (v *Vaultik) markDestination() error { destinationMarkerKey, err) } - return nil + return v.EnsureStorageBinding() } diff --git a/internal/vaultik/prune.go b/internal/vaultik/prune.go index dcffb0e..c952fd2 100644 --- a/internal/vaultik/prune.go +++ b/internal/vaultik/prune.go @@ -68,7 +68,7 @@ func (v *Vaultik) NukeRemote(force bool) error { // the same way, so the advice below must not follow these errors. // The listing also fails on a destination that answers that it has // no marker while the local index holds snapshots. - err := v.EnsureStorageBinding() + err := v.checkStorageBinding() if err != nil { return err } @@ -192,7 +192,7 @@ type PruneBlobsResult struct { // PruneDatabase or PruneBlobs individually unless it specifically wants // one half. func (v *Vaultik) Prune(opts *PruneOptions) error { - err := v.EnsureStorageBinding() + err := v.checkStorageBinding() if err != nil { return err } diff --git a/internal/vaultik/snapshot.go b/internal/vaultik/snapshot.go index 663656e..29fa971 100644 --- a/internal/vaultik/snapshot.go +++ b/internal/vaultik/snapshot.go @@ -115,14 +115,9 @@ func (v *Vaultik) CreateSnapshot(opts *SnapshotCreateOptions) error { "index_path", v.Config.IndexPath, ) - err := v.EnsureStorageBinding() - if err != nil { - return err - } - // Before PruneDatabase: it drops incomplete snapshots, which could // leave the local index without one and so skip the marker check. - err = v.markDestination() + err := v.markDestination() if err != nil { return err } @@ -749,7 +744,7 @@ type SnapshotPurgeOptions struct { // snapshot name, not the latest globally. This prevents `home` and `system` // snapshots from cannibalizing each other. func (v *Vaultik) PurgeSnapshotsWithOptions(opts *SnapshotPurgeOptions) error { - err := v.EnsureStorageBinding() + err := v.checkStorageBinding() if err != nil { return err } @@ -1285,7 +1280,7 @@ func (v *Vaultik) outputVerifyJSON(result *VerifyResult) error { // jq` fail (issue #108). The narration is duplicated as log records, // which go to stderr and so cannot corrupt the document. func (v *Vaultik) CleanupLocalSnapshots(opts *PruneOptions) error { - err := v.EnsureStorageBinding() + err := v.checkStorageBinding() if err != nil { return err } @@ -1475,7 +1470,7 @@ func (v *Vaultik) RemoveSnapshot( SnapshotID: snapshotID, } - err := v.EnsureStorageBinding() + err := v.checkStorageBinding() if err != nil { return result, err } @@ -1712,7 +1707,7 @@ func (v *Vaultik) removeSnapshotRemote( // 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() + err := v.checkStorageBinding() if err != nil { return nil, err } diff --git a/internal/vaultik/storage_bind.go b/internal/vaultik/storage_bind.go index c3dcc83..878ed69 100644 --- a/internal/vaultik/storage_bind.go +++ b/internal/vaultik/storage_bind.go @@ -13,35 +13,30 @@ import ( var errStorageBindingMismatch = errors.New( "local index is bound to a different backup destination") -// EnsureStorageBinding guarantees that the local index database is -// bound to the currently-configured storage destination. Every mutating -// command must call this before touching either the local index or the -// destination store, because the two live in lockstep: the local index -// records which chunks/blobs already exist at the destination, and -// mismatched destination + local index produces silent corruption (the -// scanner sees "known" chunks and skips uploads, then writes snapshots -// whose manifests reference blobs that aren't on the new destination). +// checkStorageBinding refuses when the local index database is bound to +// a different destination than the configured one, with an error that +// tells the user how to recover (revert the config, or run `vaultik +// database delete`). Every mutating command calls it before touching +// either the local index or the destination store, because the two live +// in lockstep: the local index records which chunks/blobs already exist +// at the destination, and a mismatched pair produces silent corruption +// (the scanner sees "known" chunks and skips uploads, then writes +// snapshots whose manifests reference blobs that aren't on the new +// destination). // -// Behaviour: -// - On first use (empty stored value), record the configured -// 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 -// delete` to discard the local index and rebuild against the new -// destination on the next backup). +// It records nothing. Only a backup binds the local index, through +// EnsureStorageBinding, and it does so before adding anything to it. An +// unbound local index therefore holds nothing from any destination. // // Read-only inspection commands (remote info, snapshot list, etc.) // deliberately don't call this: they can be run against a bare // destination store without any binding state. -func (v *Vaultik) EnsureStorageBinding() error { +func (v *Vaultik) checkStorageBinding() error { if v.Repositories == nil || v.Config == nil { // NewForTesting builds a Vaultik with no DB or config; - // there's nothing to bind and no binding to check. Callers - // exercising the bind path use a real DB and populate Config - // explicitly (see storage_bind_test.go). + // there's no binding to check. Callers exercising the bind + // path use a real DB and populate Config explicitly (see + // storage_bind_test.go). return nil } @@ -50,6 +45,24 @@ func (v *Vaultik) EnsureStorageBinding() error { return err } + if stored == "" || stored == configured { + return nil + } + + return fmt.Errorf("%w\n%s", + errStorageBindingMismatch, buildBindingMismatchMessage(stored, configured)) +} + +// EnsureStorageBinding records the configured destination (storage_url, +// or the s3.* endpoint, bucket and prefix) as the binding of an unbound +// local index. It refuses as checkStorageBinding does when the local +// index is bound to a different destination. +func (v *Vaultik) EnsureStorageBinding() error { + stored, configured, err := v.storageBinding() + if err != nil { + return err + } + if stored == "" { err = v.Repositories.LocalMeta.Set( v.ctx, database.LocalMetaKeyStorageURL, configured) @@ -62,12 +75,12 @@ func (v *Vaultik) EnsureStorageBinding() error { return nil } - if stored == configured { - return nil + if stored != configured { + return fmt.Errorf("%w\n%s", + errStorageBindingMismatch, buildBindingMismatchMessage(stored, configured)) } - return fmt.Errorf("%w\n%s", - errStorageBindingMismatch, buildBindingMismatchMessage(stored, configured)) + return nil } // storageBinding returns the destination the local index is bound to diff --git a/internal/vaultik/storage_bind_test.go b/internal/vaultik/storage_bind_test.go index 69d97b6..e517a9e 100644 --- a/internal/vaultik/storage_bind_test.go +++ b/internal/vaultik/storage_bind_test.go @@ -2,6 +2,7 @@ package vaultik_test import ( "context" + "path/filepath" "testing" "github.com/stretchr/testify/assert" @@ -9,9 +10,126 @@ import ( "sneak.berlin/go/vaultik/internal/config" "sneak.berlin/go/vaultik/internal/database" "sneak.berlin/go/vaultik/internal/log" + "sneak.berlin/go/vaultik/internal/storage" + "sneak.berlin/go/vaultik/internal/storage/faultstore" "sneak.berlin/go/vaultik/internal/vaultik" ) +// storageBindingOf returns the destination the local index is bound to, +// or "" when it is not bound. +func storageBindingOf( + ctx context.Context, t *testing.T, repos *database.Repositories, +) string { + t.Helper() + + got, err := repos.LocalMeta.Get(ctx, database.LocalMetaKeyStorageURL) + require.NoError(t, err) + + return got +} + +// TestFailedFirstBackupLeavesIndexUnbound checks that a first backup +// the destination refuses, as a mistyped bucket would, leaves the local +// index unbound, so a backup with the corrected destination succeeds. +func TestFailedFirstBackupLeavesIndexUnbound(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + ctx := context.Background() + storeDir := t.TempDir() + v, repos := newFileDestinationVaultik(ctx, t, storeDir) + working := v.Storage + + refusing := faultstore.New(working) + refusing.OnPut = func(string) faultstore.PutAction { return faultstore.PutFail } + refusing.OnStat = func(string) bool { return true } + v.Storage = refusing + v.Config.StorageURL = "file://" + filepath.Join(t.TempDir(), "typo") + + require.ErrorIs(t, backUp(v, "first"), faultstore.ErrInjectedUpload) + assert.Empty(t, storageBindingOf(ctx, t, repos)) + + v.Storage = working + v.Config.StorageURL = "file://" + storeDir + + require.NoError(t, backUp(v, "first")) + assert.Equal(t, "file://"+storeDir, storageBindingOf(ctx, t, repos)) +} + +// TestBackupToOtherDestinationWritesNoMarker checks that a backup +// refuses a local index bound to a different destination before it +// writes the marker to the configured one. +func TestBackupToOtherDestinationWritesNoMarker(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + ctx := context.Background() + firstDir := t.TempDir() + v, _ := newFileDestinationVaultik(ctx, t, firstDir) + v.Config.StorageURL = "file://" + firstDir + + require.NoError(t, backUp(v, "first")) + + secondDir := t.TempDir() + secondStore, err := storage.NewFileStorer(secondDir) + require.NoError(t, err) + + v.Storage = secondStore + v.Config.StorageURL = "file://" + secondDir + + require.ErrorContains(t, backUp(v, "first"), + "local index is bound to a different backup destination") + assert.NoFileExists(t, filepath.Join(secondDir, markerFile)) +} + +// TestCommandsOtherThanBackupLeaveIndexUnbound checks that commands +// other than a backup do not bind an unbound local index, whether or not +// they fail. Each runs on a fresh local index. +func TestCommandsOtherThanBackupLeaveIndexUnbound(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + tests := []struct { + name string + run func(v *vaultik.Vaultik) error + }{ + {"prune", func(v *vaultik.Vaultik) error { + return v.Prune(&vaultik.PruneOptions{Force: true}) + }}, + {"snapshot purge", func(v *vaultik.Vaultik) error { + return v.PurgeSnapshotsWithOptions( + &vaultik.SnapshotPurgeOptions{KeepLatest: true, Force: true}) + }}, + {"snapshot remove", func(v *vaultik.Vaultik) error { + _, err := v.RemoveSnapshot("host_first_2026-01-02T03:04:05Z", + &vaultik.RemoveOptions{Force: true}) + + return err + }}, + {"snapshot remove --all", func(v *vaultik.Vaultik) error { + _, err := v.RemoveAllSnapshots(&vaultik.RemoveOptions{Force: true}) + + return err + }}, + {"remote nuke", func(v *vaultik.Vaultik) error { + return v.NukeRemote(true) + }}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + ctx := context.Background() + v, repos := newFileDestinationVaultik(ctx, t, t.TempDir()) + + _ = tt.run(v) + + assert.Empty(t, storageBindingOf(ctx, t, repos)) + }) + } +} + // buildBindTestVaultik returns a minimal Vaultik wired with a real // in-memory DB and a config carrying the given StorageURL. Enough to // exercise EnsureStorageBinding without spinning up storage or fx. -- 2.54.0