Leave remote info orphan figures unknown when a manifest is unreadable (closes #228)

When a manifest could not be read, remote info skipped it, counted that snapshot's blobs as orphaned and advised running prune. The orphan figures are now unknown in that case, with no prune advice; --json gives them as null and lists the unreadable remote keys in unreadable_manifests. Names under metadata/ were used unchecked and printed raw. A name that is not a remote key is now skipped with a warning. Its manifest is then not read either, so a skipped name also leaves the orphan figures unknown; --json counts such names in skipped_metadata_names without printing them. The closing log line carries the unreadable manifest count in place of the orphan count. Model: opus-5-5
2026-10-07 05:46:08 +00:00
parent b57ce2277d
commit 167adf8eb1
4 changed files with 214 additions and 22 deletions
@@ -206,9 +206,19 @@ type RemoteInfoResult struct {
 	ReferencedBlobCount int   `json:"referenced_blob_count"`
 	ReferencedBlobSize  int64 `json:"referenced_blob_size"`

-	// Orphaned blobs
-	OrphanedBlobCount int   `json:"orphaned_blob_count"`
-	OrphanedBlobSize  int64 `json:"orphaned_blob_size"`
+	// Orphaned blobs. Both stay nil (null in the JSON) when a manifest
+	// could not be read or a name under metadata/ was skipped, since
+	// that snapshot's blobs would be counted as orphaned.
+	OrphanedBlobCount *int   `json:"orphaned_blob_count"`
+	OrphanedBlobSize  *int64 `json:"orphaned_blob_size"`
+
+	// Remote key of each snapshot whose manifest could not be read
+	UnreadableManifests []string `json:"unreadable_manifests,omitempty"`
+
+	// Number of names under metadata/ skipped because they are not
+	// remote keys. The names themselves are not reported: they come
+	// from the destination store and may hold control characters.
+	SkippedMetadataNames int `json:"skipped_metadata_names,omitempty"`
 }

 // RemoteInfo displays information about remote storage
@@ -234,16 +244,20 @@ func (v *Vaultik) RemoteInfo(jsonOutput bool) error {
 		v.stdoutf("Scanning snapshot metadata...\n")
 	}

-	snapshotMetadata, snapshotIDs, err := v.collectSnapshotMetadata()
+	snapshotMetadata, snapshotIDs, skippedNames, err := v.collectSnapshotMetadata()
 	if err != nil {
 		return err
 	}

+	result.SkippedMetadataNames = skippedNames
+
 	if showText {
 		v.stdoutf("Downloading %d manifest(s)...\n", len(snapshotIDs))
 	}

-	referencedBlobs := v.collectReferencedBlobsFromManifests(snapshotIDs, snapshotMetadata)
+	referencedBlobs, unreadableManifests := v.collectReferencedBlobsFromManifests(
+		snapshotIDs, snapshotMetadata)
+	result.UnreadableManifests = unreadableManifests

 	v.populateRemoteInfoResult(result, snapshotMetadata, snapshotIDs, referencedBlobs)

@@ -256,7 +270,7 @@ func (v *Vaultik) RemoteInfo(jsonOutput bool) error {
 		"snapshots", result.TotalMetadataCount,
 		"total_blobs", result.TotalBlobCount,
 		"referenced_blobs", result.ReferencedBlobCount,
-		"orphaned_blobs", result.OrphanedBlobCount)
+		"unreadable_manifests", len(result.UnreadableManifests))

 	if jsonOutput {
 		enc := json.NewEncoder(v.Stdout)
@@ -273,16 +287,18 @@ func (v *Vaultik) RemoteInfo(jsonOutput bool) error {
 }

 // collectSnapshotMetadata scans remote metadata and returns
-// per-snapshot info and sorted IDs.
+// per-snapshot info, sorted IDs and the number of names it skipped
+// because they are not remote keys.
 func (v *Vaultik) collectSnapshotMetadata() (
-	map[string]*SnapshotMetadataInfo, []string, error,
+	map[string]*SnapshotMetadataInfo, []string, int, error,
 ) {
 	snapshotMetadata := make(map[string]*SnapshotMetadataInfo)
+	skippedNames := make(map[string]bool)

 	metadataCh := v.Storage.ListStream(v.ctx, "metadata/")
 	for obj := range metadataCh {
 		if obj.Err != nil {
-			return nil, nil, fmt.Errorf("listing metadata: %w", obj.Err)
+			return nil, nil, 0, fmt.Errorf("listing metadata: %w", obj.Err)
 		}

 		parts := strings.Split(obj.Key, "/")
@@ -292,6 +308,18 @@ func (v *Vaultik) collectSnapshotMetadata() (

 		snapshotID := parts[1]

+		// The name comes from the destination store, which is not
+		// trusted, and is printed in the report. Accept it only in the
+		// form of a remote key.
+		if !isBlobHash(snapshotID) {
+			log.Warn("Skipping non-conforming key under metadata/",
+				"key", obj.Key)
+
+			skippedNames[snapshotID] = true
+
+			continue
+		}
+
 		if _, exists := snapshotMetadata[snapshotID]; !exists {
 			snapshotMetadata[snapshotID] = &SnapshotMetadataInfo{SnapshotID: snapshotID}
 		}
@@ -315,16 +343,19 @@ func (v *Vaultik) collectSnapshotMetadata() (

 	sort.Strings(snapshotIDs)

-	return snapshotMetadata, snapshotIDs, nil
+	return snapshotMetadata, snapshotIDs, len(skippedNames), nil
 }

 // collectReferencedBlobsFromManifests downloads manifests and returns
-// referenced blob hashes with sizes.
+// referenced blob hashes with sizes, and the remote keys of the
+// manifests it could not read.
 func (v *Vaultik) collectReferencedBlobsFromManifests(
 	snapshotIDs []string, snapshotMetadata map[string]*SnapshotMetadataInfo,
-) map[string]int64 {
+) (map[string]int64, []string) {
 	referencedBlobs := make(map[string]int64)

+	var unreadable []string
+
 	for _, snapshotID := range snapshotIDs {
 		// snapshotIDs here are remote keys, taken straight from the
 		// metadata/ listing. downloadManifestByKey is the single reader
@@ -333,6 +364,8 @@ func (v *Vaultik) collectReferencedBlobsFromManifests(
 		if err != nil {
 			log.Warn("Failed to read manifest", "snapshot", snapshotID, "error", err)

+			unreadable = append(unreadable, snapshotID)
+
 			continue
 		}

@@ -349,7 +382,7 @@ func (v *Vaultik) collectReferencedBlobsFromManifests(
 		info.BlobsSize = blobsSize
 	}

-	return referencedBlobs
+	return referencedBlobs, unreadable
 }

 // populateRemoteInfoResult fills in the result's snapshot and
@@ -378,8 +411,9 @@ func (v *Vaultik) populateRemoteInfoResult(
 }

 // scanRemoteBlobStorage lists all blobs on remote and computes orphan
-// stats. showText is true only when the human report is being printed
-// (not --json, not --quiet), gating the progress line.
+// stats when every manifest was read and no name under metadata/ was
+// skipped. showText is true only when the human report is being
+// printed (not --json, not --quiet), gating the progress line.
 func (v *Vaultik) scanRemoteBlobStorage(
 	result *RemoteInfoResult, referencedBlobs map[string]int64, showText bool,
 ) error {
@@ -406,13 +440,28 @@ func (v *Vaultik) scanRemoteBlobStorage(
 		result.TotalBlobSize += obj.Size
 	}

+	// A blob named only by a manifest that could not be read, or by one
+	// under a skipped name, would be counted as orphaned, so the orphan
+	// figures stay unknown.
+	if len(result.UnreadableManifests) > 0 || result.SkippedMetadataNames > 0 {
+		return nil
+	}
+
+	var (
+		orphanedCount int
+		orphanedSize  int64
+	)
+
 	for hash, size := range allBlobs {
 		if _, referenced := referencedBlobs[hash]; !referenced {
-			result.OrphanedBlobCount++
-			result.OrphanedBlobSize += size
+			orphanedCount++
+			orphanedSize += size
 		}
 	}

+	result.OrphanedBlobCount = &orphanedCount
+	result.OrphanedBlobSize = &orphanedSize
+
 	return nil
 }

@@ -465,11 +514,21 @@ func (v *Vaultik) printRemoteInfoTable(result *RemoteInfoResult) {
 	v.stdoutf("Referenced by snapshots:    %s (%s)\n",
 		humanize.Comma(int64(result.ReferencedBlobCount)),
 		ubytes(result.ReferencedBlobSize))
-	v.stdoutf("Orphaned (unreferenced):    %s (%s)\n",
-		humanize.Comma(int64(result.OrphanedBlobCount)),
-		ubytes(result.OrphanedBlobSize))

-	if result.OrphanedBlobCount > 0 {
+	if result.OrphanedBlobCount == nil {
+		v.stdoutf("Orphaned (unreferenced):    unknown "+
+			"(%d manifest(s) could not be read, "+
+			"%d name(s) under metadata/ skipped)\n",
+			len(result.UnreadableManifests), result.SkippedMetadataNames)
+
+		return
+	}
+
+	v.stdoutf("Orphaned (unreferenced):    %s (%s)\n",
+		humanize.Comma(int64(*result.OrphanedBlobCount)),
+		ubytes(*result.OrphanedBlobSize))
+
+	if *result.OrphanedBlobCount > 0 {
 		v.stdoutf("\nRun 'vaultik prune' to remove orphaned blobs.\n")
 	}
 }
@@ -0,0 +1,114 @@
+package vaultik_test
+
+import (
+	"bytes"
+	"context"
+	"encoding/json"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/vaultik/internal/log"
+	"sneak.berlin/go/vaultik/internal/snapshot"
+)
+
+// testBlobHashB is a blob that the manifest written by addRemote does
+// not reference.
+const testBlobHashB = "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" +
+	"bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"
+
+// TestRemoteInfo_UnreadableManifestLeavesOrphansUnknown checks that a
+// manifest remote info cannot read makes the orphan figures unknown. A
+// blob referenced only by that snapshot would otherwise be counted as
+// orphaned, and the report would advise running prune.
+func TestRemoteInfo_UnreadableManifestLeavesOrphansUnknown(t *testing.T) {
+	log.Initialize(log.Config{})
+	t.Parallel()
+
+	env := newListEnv(t)
+
+	// The readable manifest references blob A only.
+	env.addRemote(t, listRemoteID, time.Date(2026, 3, 2, 0, 0, 0, 0, time.UTC))
+	addBlob(t, env.store.testStorer, testBlobHashA)
+	addBlob(t, env.store.testStorer, testBlobHashB)
+
+	// With every manifest readable, blob B is orphaned.
+	require.NoError(t, env.v.RemoteInfo(true))
+
+	var doc map[string]any
+
+	require.NoError(t, json.Unmarshal(env.stdout.Bytes(), &doc))
+	assert.InDelta(t, 1, doc["orphaned_blob_count"], 0)
+
+	// A second snapshot whose manifest cannot be decoded. Blob B may be
+	// one of its blobs.
+	unreadableKey := snapshot.RemoteSnapshotKey(listLocalID)
+	require.NoError(t, env.store.Put(context.Background(),
+		"metadata/"+unreadableKey+"/manifest.json.zst",
+		bytes.NewReader([]byte("not a valid manifest"))))
+
+	env.stdout.Reset()
+	require.NoError(t, env.v.RemoteInfo(false))
+
+	text := env.stdout.String()
+	assert.Contains(t, text, "Orphaned (unreferenced):    unknown "+
+		"(1 manifest(s) could not be read, 0 name(s) under metadata/ skipped)")
+	assert.NotContains(t, text, "vaultik prune")
+
+	env.stdout.Reset()
+	require.NoError(t, env.v.RemoteInfo(true))
+
+	doc = nil
+	require.NoError(t, json.Unmarshal(env.stdout.Bytes(), &doc))
+	assert.Contains(t, doc, "orphaned_blob_count")
+	assert.Nil(t, doc["orphaned_blob_count"])
+	assert.Contains(t, doc, "orphaned_blob_size")
+	assert.Nil(t, doc["orphaned_blob_size"])
+	assert.Equal(t, []any{unreadableKey}, doc["unreadable_manifests"])
+}
+
+// TestRemoteInfo_SkipsNonConformingMetadataName checks that a directory
+// under metadata/ whose name is not a remote key is left out of the
+// report, and that the orphan figures are then unknown. The name comes
+// from the destination store; printed raw, its control characters would
+// reach the terminal. Its manifest is not read, so a blob only it
+// references would otherwise be counted as orphaned.
+func TestRemoteInfo_SkipsNonConformingMetadataName(t *testing.T) {
+	log.Initialize(log.Config{})
+	t.Parallel()
+
+	env := newListEnv(t)
+	env.addRemote(t, listRemoteID, time.Date(2026, 3, 2, 0, 0, 0, 0, time.UTC))
+	addBlob(t, env.store.testStorer, testBlobHashA)
+	addBlob(t, env.store.testStorer, testBlobHashB)
+	require.NoError(t, env.store.Put(context.Background(),
+		"metadata/\x1b[31mred/manifest.json.zst",
+		bytes.NewReader([]byte("not a valid manifest"))))
+
+	require.NoError(t, env.v.RemoteInfo(false))
+
+	text := env.stdout.String()
+	assert.NotContains(t, text, "\x1b")
+	assert.NotContains(t, text, "31mred")
+	assert.Contains(t, text, "Total (1 snapshots)")
+	assert.Contains(t, text, "Orphaned (unreferenced):    unknown "+
+		"(0 manifest(s) could not be read, 1 name(s) under metadata/ skipped)")
+	assert.NotContains(t, text, "vaultik prune")
+
+	env.stdout.Reset()
+	require.NoError(t, env.v.RemoteInfo(true))
+
+	out := env.stdout.String()
+	assert.NotContains(t, out, "31mred")
+
+	var doc map[string]any
+
+	require.NoError(t, json.Unmarshal([]byte(out), &doc))
+	assert.Contains(t, doc, "orphaned_blob_count")
+	assert.Nil(t, doc["orphaned_blob_count"])
+	assert.Contains(t, doc, "orphaned_blob_size")
+	assert.Nil(t, doc["orphaned_blob_size"])
+	assert.InDelta(t, 1, doc["skipped_metadata_names"], 0)
+	assert.NotContains(t, doc, "unreadable_manifests")
+}