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. //