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 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) and MarkSnapshotComplete (after it). An interrupted export now leaves the snapshot incomplete, so the next run PruneDatabase drops it and re-backs-up the data; the reverse tiny window leaves a restorable snapshot the index reports honestly as remote-only. Update REPOSTRUCTURE.md guarantee 4 and the ARCHITECTURE.md flow. Add a fault-injection test driving the full create path.

Model: opus-4-8

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()
+    ├─► 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
     │
-            ├─► Copy database to temp file
+    └─► SnapshotManager.MarkSnapshotComplete()   // only after the export succeeds
-            ├─► 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
 ```
 
 ## Deduplication Strategy

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
 

internal/snapshot/snapshot.go

@@ -201,11 +201,14 @@ func (sm *SnapshotManager) UpdateSnapshotStatsExtended(
 	})
 }
 
-// CompleteSnapshot marks a snapshot as completed and ensures snapshot_blobs
+// PopulateSnapshotBlobs ensures snapshot_blobs holds an entry for every
-// is populated with every blob holding any chunk referenced by the
+// blob that stores a chunk referenced by the snapshot's files, including
-// snapshot's files (including deduplicated blobs uploaded by prior
+// blobs deduplicated from earlier snapshots. Without it, a fully
-// snapshots). Without this, fully-deduplicated snapshots are unrestorable.
+// deduplicated snapshot would record no blobs and be unrestorable.
-func (sm *SnapshotManager) CompleteSnapshot(
+//
+// 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:

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
+// Automatic detection and repair of this partial state on the next run is
-// is tracked in https://git.eeqj.de/sneak/vaultik/issues/177 and is not
+// covered by TestBackupCompletesOnlyAfterMetadataExport
-// asserted here.
+// (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

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
 }