From 3a1a0d79278951670fdbb9a4833e4450c9f290c7 Mon Sep 17 00:00:00 2001 From: sneak Date: Tue, 22 Sep 2026 17:44:03 +0000 Subject: [PATCH] Mark a snapshot complete only after its metadata export succeeds (closes #177) finalizeSnapshotMetadata marked the snapshot complete and then exported its metadata. A crash after completion but before/during the export left the local index showing the snapshot as complete while the destination had no manifest or database, and PruneDatabase (which drops only NULL completed_at rows) kept it: a silently unrestorable snapshot. Reorder so completion is recorded last. CompleteSnapshot is split into PopulateSnapshotBlobs (before the export, which reads snapshot_blobs) and MarkSnapshotComplete (after it). An interrupted export now leaves the snapshot incomplete, so the next run's PruneDatabase drops it and re-backs-up the data. The reverse tiny window leaves a fully restorable snapshot at the destination that the index reports honestly as remote-only. Update REPOSTRUCTURE.md guarantee 4 and the ARCHITECTURE.md flow to the new order. Add a fault-injection test driving the full create path. Model: opus-4-8 --- ARCHITECTURE.md | 24 +-- docs/REPOSTRUCTURE.md | 2 +- internal/snapshot/snapshot.go | 48 +++++- internal/vaultik/fault_injection_test.go | 188 ++++++++++++++++++++++- internal/vaultik/snapshot.go | 23 ++- 5 files changed, 263 insertions(+), 22 deletions(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 75dc841..6428d8f 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -284,8 +284,10 @@ Manages snapshot lifecycle and metadata export. Key methods: - `CreateSnapshot(ctx, hostname, version, commit)` → Create snapshot record -- `CompleteSnapshot(ctx, snapshotID)` → Mark snapshot complete +- `PopulateSnapshotBlobs(ctx, snapshotID)` → Record every blob the snapshot references - `ExportSnapshotMetadata(ctx, dbPath, snapshotID)` → Export to S3 +- `MarkSnapshotComplete(ctx, snapshotID)` → Record completion, only after a successful export +- `CompleteSnapshot(ctx, snapshotID)` → Convenience: populate blobs, then mark complete (no export between) ### `internal/database` SQLite database for local index. Single-writer mode for thread safety. @@ -335,16 +337,18 @@ CreateSnapshot(opts) │ ├─► SnapshotManager.UpdateSnapshotStatsExtended() │ - ├─► SnapshotManager.CompleteSnapshot() + ├─► SnapshotManager.PopulateSnapshotBlobs() // record referenced blobs │ - └─► SnapshotManager.ExportSnapshotMetadata() - │ - ├─► Copy database to temp file - ├─► Clean to only current snapshot data (VACUUM) - ├─► Compress binary SQLite with zstd - ├─► Encrypt with age - ├─► Upload db.zst.age to storage - └─► Upload manifest.json.zst to storage + ├─► SnapshotManager.ExportSnapshotMetadata() + │ │ + │ ├─► Copy database to temp file + │ ├─► Clean to only current snapshot data (VACUUM) + │ ├─► Compress binary SQLite with zstd + │ ├─► Encrypt with age + │ ├─► Upload db.zst.age to storage + │ └─► Upload manifest.json.zst to storage + │ + └─► SnapshotManager.MarkSnapshotComplete() // only after the export succeeds ``` ## Deduplication Strategy diff --git a/docs/REPOSTRUCTURE.md b/docs/REPOSTRUCTURE.md index 7ea8eab..0ad5884 100644 --- a/docs/REPOSTRUCTURE.md +++ b/docs/REPOSTRUCTURE.md @@ -153,7 +153,7 @@ These are known, deliberate properties of the format and the tooling, recorded s 1. **Blobs are immutable** - Once written, a blob is never modified 2. **Blobs are written before metadata** - A snapshot's metadata is only written after all its blobs are successfully uploaded 3. **Metadata is written atomically** - Both db.zst.age and manifest.json.zst are written as complete files -4. **A snapshot is marked complete in the local DB before its metadata is uploaded, not after** - `CompleteSnapshot` runs first, then `ExportSnapshotMetadata` (see the backup data flow in [ARCHITECTURE.md](../ARCHITECTURE.md)). A crash between the two leaves a completed-looking row in the local index with no matching metadata on the destination store. `vaultik prune` reconciles this away: it drops any local snapshot whose remote metadata is missing. +4. **A snapshot is marked complete in the local DB only after its metadata is uploaded** - `finalizeSnapshotMetadata` runs `ExportSnapshotMetadata` first and records completion (`MarkSnapshotComplete`) only once the export succeeds (see the backup data flow in [ARCHITECTURE.md](../ARCHITECTURE.md)). A crash during the export therefore leaves the snapshot incomplete, so the next backup's `PruneDatabase` drops it and re-backs-up its data, rather than leaving a completed-looking row in the local index with no matching metadata on the destination store. (A crash in the brief moment after the export succeeds but before completion is recorded leaves a fully-restorable snapshot on the destination that the local index drops as incomplete on the next run; `snapshot list` then reports it honestly as remote-only, which is the safe direction: the destination copy stays restorable.) ## Pruning Safety diff --git a/internal/snapshot/snapshot.go b/internal/snapshot/snapshot.go index acea700..2fac581 100644 --- a/internal/snapshot/snapshot.go +++ b/internal/snapshot/snapshot.go @@ -201,11 +201,14 @@ func (sm *SnapshotManager) UpdateSnapshotStatsExtended( }) } -// CompleteSnapshot marks a snapshot as completed and ensures snapshot_blobs -// is populated with every blob holding any chunk referenced by the -// snapshot's files (including deduplicated blobs uploaded by prior -// snapshots). Without this, fully-deduplicated snapshots are unrestorable. -func (sm *SnapshotManager) CompleteSnapshot( +// PopulateSnapshotBlobs ensures snapshot_blobs holds an entry for every +// blob that stores a chunk referenced by the snapshot's files, including +// blobs deduplicated from earlier snapshots. Without it, a fully +// deduplicated snapshot would record no blobs and be unrestorable. +// +// This must run before ExportSnapshotMetadata: the blob manifest and the +// trimmed metadata database are both built from snapshot_blobs. +func (sm *SnapshotManager) PopulateSnapshotBlobs( ctx context.Context, snapshotID string, ) error { err := sm.repos.WithTx(ctx, func(ctx context.Context, tx *sql.Tx) error { @@ -219,6 +222,25 @@ func (sm *SnapshotManager) CompleteSnapshot( "snapshot_id", snapshotID, "added", added) } + return nil + }) + if err != nil { + return fmt.Errorf("populating snapshot blobs: %w", err) + } + + return nil +} + +// MarkSnapshotComplete records the snapshot's completion timestamp. On the +// backup path this runs only after ExportSnapshotMetadata has succeeded, so +// the local index never marks a snapshot complete while the destination +// holds no manifest or database for it. A crash before this point leaves the +// snapshot incomplete, and the next run's PruneDatabase drops it. See +// https://git.eeqj.de/sneak/vaultik/issues/177. +func (sm *SnapshotManager) MarkSnapshotComplete( + ctx context.Context, snapshotID string, +) error { + err := sm.repos.WithTx(ctx, func(ctx context.Context, tx *sql.Tx) error { return sm.repos.Snapshots.MarkComplete(ctx, tx, snapshotID) }) if err != nil { @@ -230,6 +252,22 @@ func (sm *SnapshotManager) CompleteSnapshot( return nil } +// CompleteSnapshot populates snapshot_blobs and then marks the snapshot +// complete. The backup path (finalizeSnapshotMetadata) instead calls the two +// halves separately, with the metadata export between them, so completion is +// recorded only after a successful export. This convenience is for callers +// that do not interleave an export. +func (sm *SnapshotManager) CompleteSnapshot( + ctx context.Context, snapshotID string, +) error { + err := sm.PopulateSnapshotBlobs(ctx, snapshotID) + if err != nil { + return err + } + + return sm.MarkSnapshotComplete(ctx, snapshotID) +} + // ExportSnapshotMetadata exports snapshot metadata to S3 // // This method executes the complete snapshot metadata export process: diff --git a/internal/vaultik/fault_injection_test.go b/internal/vaultik/fault_injection_test.go index 9e39096..dd02574 100644 --- a/internal/vaultik/fault_injection_test.go +++ b/internal/vaultik/fault_injection_test.go @@ -14,6 +14,7 @@ import ( "github.com/stretchr/testify/require" "sneak.berlin/go/vaultik/internal/config" "sneak.berlin/go/vaultik/internal/database" + "sneak.berlin/go/vaultik/internal/globals" "sneak.berlin/go/vaultik/internal/log" "sneak.berlin/go/vaultik/internal/snapshot" "sneak.berlin/go/vaultik/internal/storage" @@ -417,9 +418,10 @@ func TestBackupRetryAfterInterruptedUploadIsRestorable(t *testing.T) { // database is uploaded but before the manifest. The destination is left // with blobs and a database but no manifest. verify and snapshot list // must report the damage honestly rather than crashing or passing. -// Automatic detection and repair of this partial state on the next run -// is tracked in https://git.eeqj.de/sneak/vaultik/issues/177 and is not -// asserted here. +// Automatic detection and repair of this partial state on the next run is +// covered by TestBackupCompletesOnlyAfterMetadataExport +// (https://git.eeqj.de/sneak/vaultik/issues/177); this test exercises the +// lower-level export path in isolation. // //nolint:paralleltest // installs the global logger via log.Initialize func TestBackupSurvivesMetadataExportInterruption(t *testing.T) { @@ -496,6 +498,186 @@ func TestBackupSurvivesMetadataExportInterruption(t *testing.T) { "snapshot list must tolerate a partially-exported snapshot") } +// Scenario 2, repair: the process dies during the metadata export of a +// full backup run. Because completion is recorded only after the export +// succeeds (finalizeSnapshotMetadata), the interrupted snapshot is left +// incomplete rather than silently marked complete without metadata at the +// destination. Rerunning the backup must then prune the incomplete +// snapshot, produce a snapshot whose destination metadata and local index +// agree, and restore. See https://git.eeqj.de/sneak/vaultik/issues/177. +// +//nolint:paralleltest // installs the global logger via log.Initialize +func TestBackupCompletesOnlyAfterMetadataExport(t *testing.T) { + log.Initialize(log.Config{}) + + fs := afero.NewOsFs() + tempDir := t.TempDir() + dataDir := filepath.Join(tempDir, "src") + storeDir := filepath.Join(tempDir, "remote") + restoreDir := filepath.Join(tempDir, "restored") + dbPath := filepath.Join(tempDir, "index.sqlite") + + ctx := context.Background() + testFiles := writeFaultSourceTree(t, fs, dataDir) + + // A full-backup config: the fault-test defaults plus the fields the + // production create path reads (index location, chunk size, and the + // named snapshot to back up). + cfg := faultTestConfig() + cfg.IndexPath = dbPath + cfg.ChunkSize = config.Size(faultChunkSize) + cfg.Snapshots = map[string]config.SnapshotConfig{ + "data": {Paths: []string{dataDir}}, + } + + inner, err := storage.NewFileStorer(storeDir) + require.NoError(t, err) + + db, err := database.New(ctx, dbPath) + require.NoError(t, err) + + repos := database.NewRepositories(db) + + // failManifest is on for the first backup and off for the retry, so the + // manifest upload fails once — interrupting the export mid-way — then + // succeeds. + failManifest := true + store := faultstore.New(inner) + store.OnPut = func(key string) faultstore.PutAction { + if failManifest && strings.HasSuffix(key, "manifest.json.zst") { + return faultstore.PutFail + } + + return faultstore.PutNormal + } + + v := newBackupVaultik(ctx, cfg, store, repos, db, fs) + opts := &vaultik.SnapshotCreateOptions{Cron: true} + + // First run: the export fails at the manifest upload, so the whole + // create fails and the snapshot is left incomplete. + require.Error(t, v.CreateSnapshot(opts), + "backup must fail when the metadata export is interrupted") + + incompletes, err := repos.Snapshots.GetIncompleteSnapshots(ctx) + require.NoError(t, err) + require.Len(t, incompletes, 1, + "an interrupted export must leave exactly one incomplete snapshot") + + afterFirst, err := repos.Snapshots.ListRecent(ctx, listRecentTestLimit) + require.NoError(t, err) + + for _, s := range afterFirst { + require.Nil(t, s.CompletedAt, + "no snapshot may be marked complete before its metadata is exported") + } + + // Second run on the same index and destination: the retry succeeds. + failManifest = false + + require.NoError(t, v.CreateSnapshot(opts), + "a retry after an interrupted export must succeed") + + assertRetryConsistentAndRestorable( + ctx, t, cfg, inner, repos, db, fs, restoreDir, testFiles) +} + +// assertRetryConsistentAndRestorable checks the end state after the retry +// backup in TestBackupCompletesOnlyAfterMetadataExport: the interrupted +// snapshot is pruned, exactly one completed snapshot remains, its metadata +// is at the destination, and it restores to the original tree. +func assertRetryConsistentAndRestorable( + ctx context.Context, t *testing.T, cfg *config.Config, + inner storage.Storer, repos *database.Repositories, db *database.DB, + fs afero.Fs, restoreDir string, testFiles map[string][]byte, +) { + t.Helper() + + incompletes, err := repos.Snapshots.GetIncompleteSnapshots(ctx) + require.NoError(t, err) + assert.Empty(t, incompletes, + "the next run's prune must drop the interrupted snapshot") + + local, err := repos.Snapshots.ListRecent(ctx, listRecentTestLimit) + require.NoError(t, err) + require.Len(t, local, 1, "exactly one snapshot must remain after the retry") + + final := local[0] + require.NotNil(t, final.CompletedAt, "the retry's snapshot must be complete") + + // The destination and the local index agree: the completed snapshot has + // both its metadata objects at the destination. + key := snapshot.RemoteSnapshotKey(final.ID.String()) + _, err = inner.Stat(ctx, "metadata/"+key+"/manifest.json.zst") + require.NoError(t, err, "the completed snapshot's manifest must be at the destination") + _, err = inner.Stat(ctx, "metadata/"+key+"/db.zst.age") + require.NoError(t, err, "the completed snapshot's database must be at the destination") + + require.NoError(t, db.Close()) + + // The snapshot restores from the destination alone. + reader := newReaderVaultik(ctx, cfg, inner, nil, fs) + require.NoError(t, reader.Restore(&vaultik.RestoreOptions{ + SnapshotID: final.ID.String(), + TargetDir: restoreDir, + Verify: true, + }), "the retry's snapshot must be restorable") + + assertRestoredTree(t, fs, restoreDir, testFiles) +} + +// listRecentTestLimit is a generous cap for the handful of snapshots these +// tests create when reading the local index directly. +const listRecentTestLimit = 100 + +// newBackupVaultik builds a Vaultik that runs the full create path +// (CreateSnapshot) writing through storer, wiring the same scanner factory +// and snapshot manager the production dependency graph provides. +func newBackupVaultik( + ctx context.Context, cfg *config.Config, storer storage.Storer, + repos *database.Repositories, db *database.DB, fs afero.Fs, +) *vaultik.Vaultik { + v := &vaultik.Vaultik{ + Globals: &globals.Globals{Version: "v", Commit: "g"}, + Config: cfg, + DB: db, + Repositories: repos, + Storage: storer, + SnapshotManager: newFaultSnapshotManager(fs, storer, cfg, repos), + ScannerFactory: faultScannerFactory(cfg, repos, storer), + Fs: fs, + Stdout: io.Discard, + Stderr: io.Discard, + UI: ui.NewWithColor(io.Discard, false), + } + v.SetContext(ctx) + + return v +} + +// faultScannerFactory mirrors the production provideScannerFactory, binding +// the scanner to the given store, repositories, and config so a full +// create-path backup writes through the fault-injecting store. +func faultScannerFactory( + cfg *config.Config, repos *database.Repositories, storer storage.Storer, +) snapshot.ScannerFactory { + return func(params snapshot.ScannerParams) *snapshot.Scanner { + return snapshot.NewScanner(snapshot.ScannerConfig{ + FS: params.Fs, + Storage: storer, + ChunkSize: faultChunkSize, + MaxBlobSize: faultMaxBlobSize, + CompressionLevel: cfg.CompressionLevel, + AgeRecipients: cfg.AgeRecipients, + Repositories: repos, + EnableProgress: params.EnableProgress, + UI: params.UI, + Exclude: params.Exclude, + SkipErrors: params.SkipErrors, + }) + } +} + // Scenario 5: the restore target runs out of space mid-file. Restore // must fail with an out-of-space error, and must not leave a truncated // file at the target path presenting as a complete restore. Restore diff --git a/internal/vaultik/snapshot.go b/internal/vaultik/snapshot.go index 7ab3d63..9f76fdf 100644 --- a/internal/vaultik/snapshot.go +++ b/internal/vaultik/snapshot.go @@ -69,6 +69,9 @@ func (v *Vaultik) CreateSnapshot(opts *SnapshotCreateOptions) error { // Prune the database before starting: delete incomplete snapshots and orphaned data. // This ensures the database is consistent before we start a new snapshot. // Since we use locking, only one vaultik instance accesses the DB at a time. + // A snapshot whose metadata export was interrupted is left incomplete by + // finalizeSnapshotMetadata, so it is among the incomplete snapshots dropped + // here (https://git.eeqj.de/sneak/vaultik/issues/177). _, err = v.PruneDatabase() if err != nil { return fmt.Errorf("prune database: %w", err) @@ -324,7 +327,12 @@ func (v *Vaultik) collectUploadStats(scanner *snapshot.Scanner, stats *snapshotS } } -// finalizeSnapshotMetadata updates stats, marks complete, and exports metadata +// finalizeSnapshotMetadata updates stats, exports metadata, and only then +// marks the snapshot complete. Recording completion last is deliberate: an +// export interrupted by a crash leaves the snapshot incomplete rather than +// looking complete with no manifest or database at the destination. The next +// run's PruneDatabase drops the incomplete snapshot and re-backs-up its data. +// See https://git.eeqj.de/sneak/vaultik/issues/177. func (v *Vaultik) finalizeSnapshotMetadata( snapshotID string, stats *snapshotStats, ) error { @@ -346,9 +354,11 @@ func (v *Vaultik) finalizeSnapshotMetadata( return fmt.Errorf("updating snapshot stats: %w", err) } - err = v.SnapshotManager.CompleteSnapshot(v.ctx, snapshotID) + // snapshot_blobs must be populated before the export, which builds the + // manifest and the trimmed metadata database from it. + err = v.SnapshotManager.PopulateSnapshotBlobs(v.ctx, snapshotID) if err != nil { - return fmt.Errorf("completing snapshot: %w", err) + return fmt.Errorf("populating snapshot blobs: %w", err) } err = v.SnapshotManager.ExportSnapshotMetadata( @@ -357,6 +367,13 @@ func (v *Vaultik) finalizeSnapshotMetadata( return fmt.Errorf("exporting snapshot metadata: %w", err) } + // Record completion last, so an interrupted export never leaves a + // snapshot marked complete without its metadata at the destination. + err = v.SnapshotManager.MarkSnapshotComplete(v.ctx, snapshotID) + if err != nil { + return fmt.Errorf("marking snapshot complete: %w", err) + } + return nil }