From 1e3a490c12dc06d6e4874077d7c42c5c67b4c7c2 Mon Sep 17 00:00:00 2001
From: Caleb Allen <caleballen1276@gmail.com>
Date: Fri, 25 Sep 2026 11:03:33 -0400
Subject: [PATCH] fix(explore): refuse a catalog merge that does not land every
 row
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The walk's predicates partition the artifact's key space, so a merge that
ends with fewer rows than the artifact declares does not mean the artifact
was smaller than it said — it means a predicate filtered rows out, and
the catalog is quietly partial while reporting complete.

Equality rather than a lower bound: RowsAffected counts an upsert that
changes nothing, and a row already merged locally is counted again here.

One reachable case, so this is not merely a tripwire. A row whose mbid is
empty is excluded by `mbid > ?` in both encodings, so an artifact
carrying one imports as a success with a row missing — which is the shape
#258 had, one cause over. The test covers exactly that artifact.

Refs #258
---
 backend/explore/artifactimport.go      | 19 ++++++++++++++
 backend/explore/artifactimport_test.go | 35 ++++++++++++++++++++++++++
 2 files changed, 54 insertions(+)

diff --git a/backend/explore/artifactimport.go b/backend/explore/artifactimport.go
index c174eb0..c6cfaaa 100644
--- a/backend/explore/artifactimport.go
+++ b/backend/explore/artifactimport.go
@@ -285,6 +285,25 @@ func (si *SearchIndex) importCoreArtifact(ctx context.Context, path string) erro
 	}
 
 	merged, mergeErr := si.mergeArtifactRows(ctx, info.rows)
+
+	// Every row the artifact declares has to land.  The walk partitions
+	// the artifact's key space, so a total short of info.rows does not
+	// mean the artifact was smaller than it said -- it means a predicate
+	// filtered rows out, and the catalog is quietly partial.  Equality
+	// rather than a lower bound because RowsAffected counts an upsert
+	// that changes nothing, and a row already merged locally is counted
+	// again here.
+	//
+	// One reachable case, so this is not merely a tripwire: a row whose
+	// mbid is empty is excluded by `mbid > ?` in both encodings, and an
+	// artifact carrying one would otherwise import as complete.
+	if mergeErr == nil && merged != info.rows {
+		mergeErr = fmt.Errorf(
+			"%w: merged %d of %d rows — a row the artifact holds was not selected",
+			ErrArtifactUnusable, merged, info.rows,
+		)
+	}
+
 	if mergeErr == nil {
 		si.mergeArtifactCredits(ctx)
 	}
diff --git a/backend/explore/artifactimport_test.go b/backend/explore/artifactimport_test.go
index 0379775..2b4a552 100644
--- a/backend/explore/artifactimport_test.go
+++ b/backend/explore/artifactimport_test.go
@@ -852,6 +852,41 @@ func TestImportCoreArtifactWithoutCredits(t *testing.T) {
 	}
 }
 
+// TestImportCoreArtifactRefusesAMergeThatLosesRows is the count guard's
+// positive case.
+//
+// The walk's predicates partition the artifact's key space, so a merge
+// that lands fewer rows than the artifact declares means a predicate
+// dropped some — and the failure is a catalog that looks populated and
+// is missing things nobody can name.  An empty mbid is the reachable
+// way to get there: `mbid > ?` is false of it in both encodings, so it
+// is never selected, and nothing else in the import would notice.
+func TestImportCoreArtifactRefusesAMergeThatLosesRows(t *testing.T) {
+	db := database.NewTestDB(t)
+	si := NewSearchIndex(db, nil, nil, testLogger())
+
+	path := writeCompactTestArtifact(t, validMeta(), []artifactRow{
+		{EntityArtist, artA, "Artist A", "Artist A", artA, 5000},
+		{EntityArtist, "", "Nameless", "Artist A", artA, 4000},
+	})
+
+	err := si.importCoreArtifact(context.Background(), path)
+	if err == nil {
+		t.Fatal("a merge that lost a row was reported as a complete import")
+	}
+
+	if !strings.Contains(err.Error(), "merged 1 of 2 rows") {
+		t.Errorf("error = %v, want it to name the shortfall", err)
+	}
+
+	// And the same rule as every other rejection: a failed merge must not
+	// leave the index claiming it has a catalog, or the real build would
+	// never run again.
+	if si.hasMeta(dumpImportDoneKey) {
+		t.Error("a failed import still stamped dump_import_done")
+	}
+}
+
 // TestArtifactKeyBindsInTheArtifactsOwnEncoding pins the one place the
 // batch walk's comparison type is decided.
 //