Bind the local index only after a backup writes the marker (closes #420)
check / check (push) Successful in 9m27s

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
This commit is contained in:
clawbot committed 2026-11-11 01:30:14 +00:00
1 parent aae7fa17ae
commit 62719f4bd1
5 files changed
+178 -44

No files matched your search

+14 -6
View File
@@ -51,7 +51,7 @@ func (v *Vaultik) checkDestinationMarker() error {
} }
// Config is nil only in tests, which then record no binding (see // Config is nil only in tests, which then record no binding (see
// EnsureStorageBinding). // checkStorageBinding).
if v.Config != nil { if v.Config != nil {
stored, configured, err := v.storageBinding() stored, configured, err := v.storageBinding()
if err != nil { if err != nil {
@@ -83,11 +83,19 @@ func (v *Vaultik) checkDestinationMarker() error {
return nil return nil
} }
// markDestination checks the backup destination store with // markDestination refuses a local index bound to a different
// checkDestinationMarker and then writes the marker, so a first backup // destination, checks the backup destination store for its marker,
// marks its destination and a later one writes the same text again. // 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 { func (v *Vaultik) markDestination() error {
err := v.checkDestinationMarker() err := v.checkStorageBinding()
if err != nil {
return err
}
err = v.checkDestinationMarker()
if err != nil { if err != nil {
return err return err
} }
@@ -99,5 +107,5 @@ func (v *Vaultik) markDestination() error {
destinationMarkerKey, err) destinationMarkerKey, err)
} }
return nil return v.EnsureStorageBinding()
} }
+2 -2
View File
@@ -68,7 +68,7 @@ func (v *Vaultik) NukeRemote(force bool) error {
// the same way, so the advice below must not follow these errors. // the same way, so the advice below must not follow these errors.
// The listing also fails on a destination that answers that it has // The listing also fails on a destination that answers that it has
// no marker while the local index holds snapshots. // no marker while the local index holds snapshots.
err := v.EnsureStorageBinding() err := v.checkStorageBinding()
if err != nil { if err != nil {
return err return err
} }
@@ -192,7 +192,7 @@ type PruneBlobsResult struct {
// PruneDatabase or PruneBlobs individually unless it specifically wants // PruneDatabase or PruneBlobs individually unless it specifically wants
// one half. // one half.
func (v *Vaultik) Prune(opts *PruneOptions) error { func (v *Vaultik) Prune(opts *PruneOptions) error {
err := v.EnsureStorageBinding() err := v.checkStorageBinding()
if err != nil { if err != nil {
return err return err
} }
+5 -10
View File
@@ -115,14 +115,9 @@ func (v *Vaultik) CreateSnapshot(opts *SnapshotCreateOptions) error {
"index_path", v.Config.IndexPath, "index_path", v.Config.IndexPath,
) )
err := v.EnsureStorageBinding()
if err != nil {
return err
}
// Before PruneDatabase: it drops incomplete snapshots, which could // Before PruneDatabase: it drops incomplete snapshots, which could
// leave the local index without one and so skip the marker check. // leave the local index without one and so skip the marker check.
err = v.markDestination() err := v.markDestination()
if err != nil { if err != nil {
return err return err
} }
@@ -749,7 +744,7 @@ type SnapshotPurgeOptions struct {
// snapshot name, not the latest globally. This prevents `home` and `system` // snapshot name, not the latest globally. This prevents `home` and `system`
// snapshots from cannibalizing each other. // snapshots from cannibalizing each other.
func (v *Vaultik) PurgeSnapshotsWithOptions(opts *SnapshotPurgeOptions) error { func (v *Vaultik) PurgeSnapshotsWithOptions(opts *SnapshotPurgeOptions) error {
err := v.EnsureStorageBinding() err := v.checkStorageBinding()
if err != nil { if err != nil {
return err return err
} }
@@ -1285,7 +1280,7 @@ func (v *Vaultik) outputVerifyJSON(result *VerifyResult) error {
// jq` fail (issue #108). The narration is duplicated as log records, // jq` fail (issue #108). The narration is duplicated as log records,
// which go to stderr and so cannot corrupt the document. // which go to stderr and so cannot corrupt the document.
func (v *Vaultik) CleanupLocalSnapshots(opts *PruneOptions) error { func (v *Vaultik) CleanupLocalSnapshots(opts *PruneOptions) error {
err := v.EnsureStorageBinding() err := v.checkStorageBinding()
if err != nil { if err != nil {
return err return err
} }
@@ -1475,7 +1470,7 @@ func (v *Vaultik) RemoveSnapshot(
SnapshotID: snapshotID, SnapshotID: snapshotID,
} }
err := v.EnsureStorageBinding() err := v.checkStorageBinding()
if err != nil { if err != nil {
return result, err return result, err
} }
@@ -1712,7 +1707,7 @@ func (v *Vaultik) removeSnapshotRemote(
// before it removes anything when the destination store cannot be // before it removes anything when the destination store cannot be
// listed or is not present by checkDestinationMarker. // listed or is not present by checkDestinationMarker.
func (v *Vaultik) RemoveAllSnapshots(opts *RemoveOptions) (*RemoveResult, error) { func (v *Vaultik) RemoveAllSnapshots(opts *RemoveOptions) (*RemoveResult, error) {
err := v.EnsureStorageBinding() err := v.checkStorageBinding()
if err != nil { if err != nil {
return nil, err return nil, err
} }
+39 -26
View File
@@ -13,35 +13,30 @@ import (
var errStorageBindingMismatch = errors.New( var errStorageBindingMismatch = errors.New(
"local index is bound to a different backup destination") "local index is bound to a different backup destination")
// EnsureStorageBinding guarantees that the local index database is // checkStorageBinding refuses when the local index database is bound to
// bound to the currently-configured storage destination. Every mutating // a different destination than the configured one, with an error that
// command must call this before touching either the local index or the // tells the user how to recover (revert the config, or run `vaultik
// destination store, because the two live in lockstep: the local index // database delete`). Every mutating command calls it before touching
// records which chunks/blobs already exist at the destination, and // either the local index or the destination store, because the two live
// mismatched destination + local index produces silent corruption (the // in lockstep: the local index records which chunks/blobs already exist
// scanner sees "known" chunks and skips uploads, then writes snapshots // at the destination, and a mismatched pair produces silent corruption
// whose manifests reference blobs that aren't on the new destination). // (the scanner sees "known" chunks and skips uploads, then writes
// snapshots whose manifests reference blobs that aren't on the new
// destination).
// //
// Behaviour: // It records nothing. Only a backup binds the local index, through
// - On first use (empty stored value), record the configured // EnsureStorageBinding, and it does so before adding anything to it. An
// destination (storage_url, or the s3.* endpoint, bucket and // unbound local index therefore holds nothing from any destination.
// 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).
// //
// Read-only inspection commands (remote info, snapshot list, etc.) // Read-only inspection commands (remote info, snapshot list, etc.)
// deliberately don't call this: they can be run against a bare // deliberately don't call this: they can be run against a bare
// destination store without any binding state. // destination store without any binding state.
func (v *Vaultik) EnsureStorageBinding() error { func (v *Vaultik) checkStorageBinding() error {
if v.Repositories == nil || v.Config == nil { if v.Repositories == nil || v.Config == nil {
// NewForTesting builds a Vaultik with no DB or config; // NewForTesting builds a Vaultik with no DB or config;
// there's nothing to bind and no binding to check. Callers // there's no binding to check. Callers exercising the bind
// exercising the bind path use a real DB and populate Config // path use a real DB and populate Config explicitly (see
// explicitly (see storage_bind_test.go). // storage_bind_test.go).
return nil return nil
} }
@@ -50,6 +45,24 @@ func (v *Vaultik) EnsureStorageBinding() error {
return err 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 == "" { if stored == "" {
err = v.Repositories.LocalMeta.Set( err = v.Repositories.LocalMeta.Set(
v.ctx, database.LocalMetaKeyStorageURL, configured) v.ctx, database.LocalMetaKeyStorageURL, configured)
@@ -62,12 +75,12 @@ func (v *Vaultik) EnsureStorageBinding() error {
return nil return nil
} }
if stored == configured { if stored != configured {
return nil
}
return fmt.Errorf("%w\n%s", return fmt.Errorf("%w\n%s",
errStorageBindingMismatch, buildBindingMismatchMessage(stored, configured)) errStorageBindingMismatch, buildBindingMismatchMessage(stored, configured))
}
return nil
} }
// storageBinding returns the destination the local index is bound to // storageBinding returns the destination the local index is bound to
+118
View File
@@ -2,6 +2,7 @@ package vaultik_test
import ( import (
"context" "context"
"path/filepath"
"testing" "testing"
"github.com/stretchr/testify/assert" "github.com/stretchr/testify/assert"
@@ -9,9 +10,126 @@ import (
"sneak.berlin/go/vaultik/internal/config" "sneak.berlin/go/vaultik/internal/config"
"sneak.berlin/go/vaultik/internal/database" "sneak.berlin/go/vaultik/internal/database"
"sneak.berlin/go/vaultik/internal/log" "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" "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 // buildBindTestVaultik returns a minimal Vaultik wired with a real
// in-memory DB and a config carrying the given StorageURL. Enough to // in-memory DB and a config carrying the given StorageURL. Enough to
// exercise EnsureStorageBinding without spinning up storage or fx. // exercise EnsureStorageBinding without spinning up storage or fx.