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

@@ -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()
 }

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
 	}

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
 	}

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

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.