From 56389b7ccc31bb225ea6f99cdfc7f544c0a4ffb6 Mon Sep 17 00:00:00 2001 From: clawbot Date: Wed, 11 Nov 2026 00:43:09 +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 | 18 ++++-- internal/vaultik/prune.go | 4 +- internal/vaultik/snapshot.go | 15 ++--- internal/vaultik/storage_bind.go | 82 +++++++++++++++----------- internal/vaultik/storage_bind_test.go | 56 ++++++++++++++++++ 5 files changed, 123 insertions(+), 52 deletions(-) diff --git a/internal/vaultik/destination_marker.go b/internal/vaultik/destination_marker.go index af28fcf..472a388 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 { @@ -84,10 +84,18 @@ func (v *Vaultik) checkDestinationMarker() error { } // 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. +// checkStorageBinding and checkDestinationMarker, writes the marker, and +// then binds the local index to the destination with +// EnsureStorageBinding. A first backup marks its destination and a later +// one writes the same text again. Binding after the write means a 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..0b92b4b 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 only once it has written to the destination. +// 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,19 +45,7 @@ func (v *Vaultik) EnsureStorageBinding() error { return err } - if stored == "" { - err = v.Repositories.LocalMeta.Set( - v.ctx, database.LocalMetaKeyStorageURL, configured) - if err != nil { - return fmt.Errorf("recording local storage binding: %w", err) - } - - log.Info("Bound local index to storage destination", "storage_url", configured) - - return nil - } - - if stored == configured { + if stored == "" || stored == configured { return nil } @@ -70,6 +53,35 @@ func (v *Vaultik) EnsureStorageBinding() error { errStorageBindingMismatch, buildBindingMismatchMessage(stored, configured)) } +// EnsureStorageBinding refuses as checkStorageBinding does and, when the +// local index is not bound yet, records the configured destination +// (storage_url, or the s3.* endpoint, bucket and prefix) as its binding. +// A backup calls it right after writing the destination marker, its +// first write to the destination, so a backup that fails before then +// (a mistyped bucket, a refused credential) leaves the local index +// unbound and a corrected config is enough to go on. +func (v *Vaultik) EnsureStorageBinding() error { + err := v.checkStorageBinding() + if err != nil || v.Repositories == nil || v.Config == nil { + return err + } + + stored, configured, err := v.storageBinding() + if err != nil || stored != "" { + return err + } + + err = v.Repositories.LocalMeta.Set( + v.ctx, database.LocalMetaKeyStorageURL, configured) + if err != nil { + return fmt.Errorf("recording local storage binding: %w", err) + } + + log.Info("Bound local index to storage destination", "storage_url", configured) + + return nil +} + // storageBinding returns the destination the local index is bound to // ("" when no binding is recorded yet), then the configured destination // written the same way. It records nothing, so a read-only command can diff --git a/internal/vaultik/storage_bind_test.go b/internal/vaultik/storage_bind_test.go index 69d97b6..32cbb3d 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,64 @@ 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/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)) +} + +// TestPruneLeavesIndexUnbound checks that a command other than a backup +// does not bind an unbound local index. +func TestPruneLeavesIndexUnbound(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + ctx := context.Background() + v, repos := newFileDestinationVaultik(ctx, t, t.TempDir()) + + require.NoError(t, v.Prune(&vaultik.PruneOptions{Force: true})) + 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.