Fail closed on unreadable manifests instead of losing blobs (closes #157)

Prune learned which blobs are in use by reading every snapshot's
manifest, but merely logged and skipped one it could not download or
decode. Blobs referenced only by that snapshot then looked unreferenced
and were deleted, with a zero exit — and `snapshot create --prune` runs
this unattended. collectReferencedBlobs now errors, naming the remote
key, so prune deletes nothing and exits non-zero.

Manifest generation likewise skipped a blob whose lookup failed or was
missing, yielding a manifest short of what the snapshot needs; it now
fails. Deep verify only warned when the manifest omitted a database
blob; it now fails on any divergence. Docs corrected.

Model: opus-4-8

docs/REPOSTRUCTURE.md

@@ -145,10 +145,10 @@ An observer cannot determine:
 ## Pruning Safety
 
 The prune operation is safe because:
-1. It only deletes blobs not referenced in any manifest
+1. It keeps every blob listed in any snapshot's manifest and deletes only blobs that no manifest references
 2. Manifests are unencrypted and can be read without keys
-3. The operation compares the latest local DB snapshot with the latest S3 snapshot to ensure consistency
-4. Pruning will fail if these don't match, preventing accidental deletion of needed blobs
+3. If any manifest cannot be downloaded or decoded, prune deletes nothing and exits with an error, rather than treating that snapshot's blobs as unreferenced
+4. Prune requires exclusive access to the destination: running it during a concurrent backup can race a snapshot whose manifest is not yet written, so do not prune while a backup is in progress
 
 ## Restoration Requirements
 

internal/snapshot/manifest_gen_test.go

@@ -0,0 +1,64 @@
+//nolint:testpackage // exercises the unexported generateBlobManifest
+package snapshot
+
+import (
+	"context"
+	"path/filepath"
+	"testing"
+	"time"
+
+	"github.com/spf13/afero"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/vaultik/internal/config"
+	"sneak.berlin/go/vaultik/internal/database"
+	"sneak.berlin/go/vaultik/internal/log"
+	"sneak.berlin/go/vaultik/internal/types"
+)
+
+// TestGenerateBlobManifest_MissingBlobFails is the regression guard for
+// issue #157: a blob the snapshot references but that is absent from the
+// blobs table used to be logged and skipped, yielding a manifest with
+// fewer blobs than the snapshot needs. Since prune trusts the manifest
+// alone, that omitted blob would be deleted at the next prune. Manifest
+// generation must fail instead.
+func TestGenerateBlobManifest_MissingBlobFails(t *testing.T) {
+	log.Initialize(log.Config{})
+	t.Parallel()
+
+	ctx := context.Background()
+	dbPath := filepath.Join(t.TempDir(), "snapshot.db")
+
+	db, err := database.New(ctx, dbPath)
+	require.NoError(t, err)
+
+	repos := database.NewRepositories(db)
+
+	// A real blob row satisfies the snapshot_blobs foreign key on
+	// blob_id; the snapshot then references a different, absent hash.
+	presentBlob := &database.Blob{
+		ID:        types.NewBlobID(),
+		Hash:      types.BlobHash("present-blob-hash"),
+		CreatedTS: time.Now().Truncate(time.Second),
+	}
+	require.NoError(t, repos.Blobs.Create(ctx, nil, presentBlob))
+
+	snap := &database.Snapshot{
+		ID:       "testhost_home_2026-05-01T00:00:00Z",
+		Hostname: "testhost",
+	}
+	require.NoError(t, repos.Snapshots.Create(ctx, nil, snap))
+	require.NoError(t, repos.Snapshots.AddBlob(ctx, nil,
+		snap.ID.String(), presentBlob.ID, types.BlobHash("absent-blob-hash")))
+
+	require.NoError(t, db.Close())
+
+	sm := &SnapshotManager{
+		config: &config.Config{CompressionLevel: 3},
+		fs:     afero.NewOsFs(),
+	}
+
+	_, err = sm.generateBlobManifest(ctx, dbPath, snap.ID.String())
+	require.Error(t, err, "manifest generation must fail on a missing blob")
+	assert.Contains(t, err.Error(), "absent-blob-hash")
+}

internal/snapshot/snapshot.go

@@ -809,6 +809,11 @@ func (sm *SnapshotManager) copyFile(src, dst string) error {
 	return nil
 }
 
+// errBlobMissingFromDatabase means a snapshot references a blob that is
+// absent from the blobs table, so a complete manifest cannot be built.
+var errBlobMissingFromDatabase = errors.New(
+	"blob referenced by snapshot is not in the database")
+
 // generateBlobManifest creates a compressed JSON list of all blobs in the snapshot
 func (sm *SnapshotManager) generateBlobManifest(
 	ctx context.Context, dbPath string, snapshotID string,
@@ -839,20 +844,26 @@ func (sm *SnapshotManager) generateBlobManifest(
 	totalCompressedSize := int64(0)
 
 	for _, hash := range blobHashes {
+		// Every blob the snapshot references must appear in the manifest.
+		// Prune consults only the manifest to decide what is still in use,
+		// so silently dropping a blob here would let a later prune delete
+		// it while this snapshot still needs it. A lookup failure or a
+		// missing blob row therefore fails manifest generation.
 		blob, err := repos.Blobs.GetByHash(ctx, hash)
 		if err != nil {
-			log.Warn("Failed to get blob details", "hash", hash, "error", err)
-
-			continue
+			return nil, fmt.Errorf("getting blob details for %s: %w", hash, err)
 		}
 
-		if blob != nil {
-			blobs = append(blobs, BlobInfo{
-				Hash:           hash,
-				CompressedSize: blob.CompressedSize,
-			})
-			totalCompressedSize += blob.CompressedSize
+		if blob == nil {
+			return nil, fmt.Errorf("%w: blob %s, snapshot %s",
+				errBlobMissingFromDatabase, hash, snapshotID)
 		}
+
+		blobs = append(blobs, BlobInfo{
+			Hash:           hash,
+			CompressedSize: blob.CompressedSize,
+		})
+		totalCompressedSize += blob.CompressedSize
 	}
 
 	// Create manifest. SnapshotID in the unencrypted manifest is the

internal/vaultik/prune.go

@@ -167,6 +167,12 @@ func (v *Vaultik) PruneBlobs(opts *PruneOptions) error {
 
 // collectReferencedBlobs downloads all manifests and returns the set of
 // referenced blob hashes.
+//
+// Every manifest must be read successfully. A manifest that cannot be
+// downloaded or decoded means its snapshot's blobs are unknown, so
+// treating them as unreferenced would let prune delete data a snapshot
+// still needs. Rather than risk that silent loss, any failure returns an
+// error naming the remote key and prune deletes nothing.
 func (v *Vaultik) collectReferencedBlobs() (map[string]bool, error) {
 	log.Info("Listing remote snapshots")
 	// IDs returned by listUniqueSnapshotIDs are remote keys (hashed
@@ -179,27 +185,22 @@ func (v *Vaultik) collectReferencedBlobs() (map[string]bool, error) {
 	log.Info("Found manifests in remote storage", "count", len(remoteKeys))
 
 	allBlobsReferenced := make(map[string]bool)
-	manifestCount := 0
 
 	for _, remoteKey := range remoteKeys {
 		log.Debug("Processing manifest", "remote_key", remoteKey)
 
 		manifest, err := v.downloadManifestByKey(remoteKey)
 		if err != nil {
-			log.Error("Failed to download manifest", "remote_key", remoteKey, "error", err)
-
-			continue
+			return nil, fmt.Errorf("reading manifest %s: %w", remoteKey, err)
 		}
 
 		for _, blob := range manifest.Blobs {
 			allBlobsReferenced[blob.Hash] = true
 		}
-
-		manifestCount++
 	}
 
 	log.Info("Processed manifests",
-		"count", manifestCount, "unique_blobs_referenced", len(allBlobsReferenced))
+		"count", len(remoteKeys), "unique_blobs_referenced", len(allBlobsReferenced))
 
 	return allBlobsReferenced, nil
 }

internal/vaultik/prune_manifest_safety_test.go

@@ -0,0 +1,47 @@
+package vaultik_test
+
+import (
+	"bytes"
+	"context"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/vaultik/internal/log"
+	"sneak.berlin/go/vaultik/internal/vaultik"
+)
+
+// TestPruneBlobs_UnreadableManifestDeletesNothing is the regression guard
+// for issue #157: prune identifies referenced blobs by reading every
+// snapshot's manifest, and a manifest it cannot decode used to be logged
+// and skipped. Blobs referenced only by that snapshot then looked
+// unreferenced and were deleted, with a zero exit — silent backup loss,
+// made worse by `snapshot create --prune` running unattended with force.
+//
+// The single blob here is referenced only by the snapshot whose manifest
+// is corrupt, so the old behaviour would delete it and succeed. Prune
+// must instead delete nothing and return an error.
+func TestPruneBlobs_UnreadableManifestDeletesNothing(t *testing.T) {
+	log.Initialize(log.Config{})
+	t.Parallel()
+
+	env := newListEnv(t)
+	ctx := context.Background()
+
+	blobKey := "blobs/" + testBlobHashA[:2] + "/" + testBlobHashA[2:4] +
+		"/" + testBlobHashA
+	require.NoError(t, env.store.Put(ctx, blobKey,
+		bytes.NewReader([]byte("blob-bytes"))))
+
+	// A manifest at the path prune reads, but with contents it cannot
+	// decode.
+	require.NoError(t, env.store.Put(ctx,
+		"metadata/corruptkey/manifest.json.zst",
+		bytes.NewReader([]byte("not a valid manifest"))))
+
+	err := env.v.PruneBlobs(&vaultik.PruneOptions{Force: true})
+
+	require.Error(t, err, "prune must fail when a manifest cannot be read")
+	assert.True(t, env.store.hasKey(blobKey),
+		"no blob may be deleted when a manifest is unreadable")
+}

internal/vaultik/verify.go

@@ -28,8 +28,10 @@ var (
 	errChunkHashMismatch = errors.New("chunk hash mismatch")
 	errTrailingBlobData  = errors.New(
 		"blob has unexpected trailing bytes not covered by chunk list")
-	errManifestExtraBlob = errors.New("manifest contains blob not in database")
-	errBlobSizeMismatch  = errors.New("blob size mismatch")
+	errManifestExtraBlob   = errors.New("manifest contains blob not in database")
+	errManifestMissingBlob = errors.New(
+		"manifest omits blob present in database")
+	errBlobSizeMismatch = errors.New("blob size mismatch")
 )
 
 // verifyStatusFailed is the JSON status value for a failed verification.
@@ -575,16 +577,11 @@ func (v *Vaultik) verifyManifestAgainstDatabase(
 		manifestBlobMap[blob.Hash] = blob.CompressedSize
 	}
 
-	// Check counts match
-	if len(dbBlobMap) != len(manifestBlobMap) {
-		log.Warn("Manifest blob count mismatch",
-			"database_blobs", len(dbBlobMap),
-			"manifest_blobs", len(manifestBlobMap),
-		)
-		// This is a warning, not an error - database is authoritative
-	}
-
-	// Check each manifest blob exists in database with correct size
+	// The manifest is the only blob list prune consults, so it must match
+	// the database exactly. A blob in the manifest but not the database
+	// points at a corrupt manifest; a blob in the database but omitted
+	// from the manifest would be pruned away while this snapshot still
+	// needs it. Either divergence fails verification.
 	for hash, manifestSize := range manifestBlobMap {
 		dbSize, exists := dbBlobMap[hash]
 		if !exists {
@@ -598,6 +595,12 @@ func (v *Vaultik) verifyManifestAgainstDatabase(
 		}
 	}
 
+	for hash := range dbBlobMap {
+		if _, exists := manifestBlobMap[hash]; !exists {
+			return fmt.Errorf("%w: %s", errManifestMissingBlob, hash)
+		}
+	}
+
 	log.Info("✓ Manifest verified against database",
 		"manifest_blobs", len(manifestBlobMap),
 		"database_blobs", len(dbBlobMap),

internal/vaultik/verify_manifest_test.go

@@ -0,0 +1,61 @@
+package vaultik //nolint:testpackage // calls unexported verifyManifestAgainstDatabase
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/vaultik/internal/log"
+	"sneak.berlin/go/vaultik/internal/snapshot"
+)
+
+// Blob hashes shared by the manifest-verification tests below.
+const (
+	manifestTestBlobA = "blob-a"
+	manifestTestBlobB = "blob-b"
+)
+
+// TestVerifyManifestAgainstDatabase_MissingBlobFails is the regression
+// guard for issue #157: deep verify must fail when the manifest omits a
+// blob the database records. The divergence used to be logged as a
+// warning while verification still returned ok, so an incomplete
+// manifest — the exact defect that lets prune later delete a needed blob
+// — passed unnoticed.
+func TestVerifyManifestAgainstDatabase_MissingBlobFails(t *testing.T) {
+	log.Initialize(log.Config{})
+	t.Parallel()
+
+	v := &Vaultik{}
+
+	dbBlobs := []snapshot.BlobInfo{
+		{Hash: manifestTestBlobA, CompressedSize: 10},
+		{Hash: manifestTestBlobB, CompressedSize: 20},
+	}
+	manifest := &snapshot.Manifest{
+		Blobs: []snapshot.BlobInfo{
+			{Hash: manifestTestBlobA, CompressedSize: 10},
+		},
+	}
+
+	err := v.verifyManifestAgainstDatabase(manifest, dbBlobs)
+	require.Error(t, err, "verify must fail when the manifest omits a database blob")
+	assert.Contains(t, err.Error(), manifestTestBlobB)
+}
+
+// TestVerifyManifestAgainstDatabase_MatchingSetsPass keeps the other half
+// honest: identical blob sets still verify, so the check above cannot be
+// satisfied by failing everything.
+func TestVerifyManifestAgainstDatabase_MatchingSetsPass(t *testing.T) {
+	log.Initialize(log.Config{})
+	t.Parallel()
+
+	v := &Vaultik{}
+
+	blobs := []snapshot.BlobInfo{
+		{Hash: manifestTestBlobA, CompressedSize: 10},
+		{Hash: manifestTestBlobB, CompressedSize: 20},
+	}
+	manifest := &snapshot.Manifest{Blobs: blobs}
+
+	require.NoError(t, v.verifyManifestAgainstDatabase(manifest, blobs))
+}