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
 }