Fix enrichment batch starvation, image collisions, best-image selection (#343)

[joestump]

What / Why

Implements story #343 (part of epic #322). Governing artifacts: ADR-0015, ADR-0027, SPEC metadata-enrichment-pipeline REQ-ENRICH-022 / REQ-ENRICH-050.

Batch starvation (internal/services/metadata.go)

The artist/album/track enrichment queries used unordered Limit(100) (tracks: 200) with Or-predicates (LidarrIDIsNil, LastAiEnrichedAt*) that still match just-enriched rows, so the same first-limit rows re-filled every batch and rows beyond the limit were never enriched. The queries now Order by last_enriched_at ascending with NULLs first and ID as tie-break (byLastEnrichedAtNullsFirst). Since every processed row gets its last_enriched_at bumped, successive ticks rotate through the entire library. The NULL grouping is expressed as a portable boolean sort key (col IS NOT NULL ASC) because Postgres sorts NULLs last on ASC and MySQL lacks NULLS FIRST.

Image filename collisions (internal/services/metadata.go)

downloadArtistImage/downloadAlbumImage named files {id}-{imageType}{ext} while rows dedupe by URL, so N same-type images collapsed onto one file via the os.Stat exists-branch (all DB rows pointed at image #1's bytes). Filenames now include a per-image discriminator — an 8-hex-char SHA-256 hash of the URL: {id}-{imageType}-{urlhash}{ext}. Same-URL re-downloads still hit the exists-branch (REQ-ENRICH-033 skip behavior preserved). Note on existing data: rows already collapsed under the old naming are NOT automatically re-fetched on persistent-volume deployments — DownloadImages only selects rows with an empty local_path, and repairStaleImagePaths only fires when the file is missing from disk. Previously-collapsed rows are re-downloaded (with distinct filenames) only if the data directory is lost or the file otherwise goes missing. A one-shot backfill that clears local_path for rows sharing a file is possible follow-up work.

REQ-ENRICH-022 best-image selection (internal/handlers/images.go)

ArtistImage/AlbumImage serving previously ignored the stored Likes and dimensions. Selection now ranks per spec: IsPrimary → highest Likes → largest Width × Height, with ID as a deterministic final tie-break, skipping records with no downloaded local_path (issue #127 behavior preserved). Album images carry no Likes column, so their ranking is IsPrimary → dimensions.

REQ-ENRICH-050 duplicate registration (internal/enrichers/enrichers.go)

Registry.Register silently overwrote an existing factory. It now returns an error on duplicate type registration and keeps the original factory; MetadataService.Register propagates it. cmd/server/main.go registration was minimally adapted (mustRegisterEnricher closure that logs and exits) — note a sibling story (#324) also owns that file; the edit is confined to the metadata enricher registration block.

Test evidence

• make test — all packages pass; gofmt -l clean; golangci-lint (v1.64.8, repo config) clean on the four changed packages.
• New regression tests were verified to fail against the pre-fix code (all 6 service-level tests fail on origin/main's metadata.go):
  • TestEnrichArtists_NoBatchStarvation — 250 never-matchable artists (fixture per acceptance criteria) all enriched across 3 ticks at Limit(100); plus TestEnrichArtists_BatchesRotate, TestEnrichAlbums_NoBatchStarvation, TestEnrichTracks_NoBatchStarvation (Limit 200).
  • TestDownloadArtistImage_SameTypeImagesGetDistinctFiles — an artist with 5 fanart images stores 5 distinct files with distinct bytes; album counterpart included; TestDownloadArtistImage_ExistingFileSkipsRedownload guards the REQ-ENRICH-033 skip path.
  • internal/handlers/images_selection_test.go — 10 cases covering IsPrimary > Likes > dimensions ordering, nil Likes/dimensions, empty-local_path skipping, determinism.
  • internal/enrichers/registry_test.go — duplicate registration errors and preserves the original factory; distinct types coexist.

Deferred Design Doc Updates

• docs/adrs/ADR-0027-image-storage-filesystem.md — example local_path format (data/images/artists/1-thumbnail.png) should be updated to the new {id}-{type}-{urlhash}.{ext} pattern.
• docs/adrs/ADR-0015-pluggable-enricher-registry-pattern.md — the "Bad, because … only one factory per type is allowed" consequence and the Confirmation section should note that duplicate registration now errors per REQ-ENRICH-050.
• docs/openspec/specs/metadata-enrichment-pipeline/spec.md — REQ-ENRICH-050 describes Register(enricher Enricher), while the implemented signature is Register(t Type, factory Factory) (pre-existing drift, now with error return).

Closes #343

🤖 Posted on behalf of @joestump by Claude.

[joestump]

Independent review — verdict: LGTM

Adversarial review against issue #343, ADR-0015, ADR-0027, and SPEC metadata-enrichment-pipeline (REQ-ENRICH-022/040/050). I re-ran make test on a detached checkout of 8c1cabd (all green) and independently confirmed both regression suites fail on origin/main (TestEnrichArtists_BatchesRotate: second tick re-selects the first 100; TestDownloadArtistImage_SameTypeImagesGetDistinctFiles: 5 rows collapse onto 1 file). The fixes are real, not test-shaped.

Verified

• Starvation semantics hold for unmatchable rows. SetLastEnrichedAt(time.Now()) is unconditional in enrichArtist (internal/services/metadata.go:765), enrichAlbum (:1185), and enrichTrack (:1483) — bumped even when every enricher errors or returns nil, nil. So permanently unmatchable rows (e.g. LidarrIDIsNil) rotate to the back of the byLastEnrichedAtNullsFirst order instead of re-pinning the batch. The only skip-bump path is update.Save failing.
• Portable NULLS-FIRST. The boolean sort key (col IS NOT NULL ASC, metadata.go:82-91) is correct on all three configured drivers (sqlite3/postgres/mysql per internal/config/config.go:350): false/0 sorts first everywhere, so NULLs lead. Columns come from s.C() on Ent-generated field constants — properly quoted, no injection surface.
• REQ-ENRICH-022 nil handling. Likes/Width/Height are Optional().Nillable() in ent/schema/artistimage.go; the nil-guards in bestArtistImagePath are correct, and AlbumImage genuinely has no likes column, so the IsPrimary → dimensions album ranking matches the schema.
• No path traversal. getImageExtension whitelists .jpg/.jpeg/.png/.gif/.webp (fallback .jpg) and imageURLHash is hex — the URL cannot inject path segments into the filename.
• Existing deployments keep serving. Rows with old-style {id}-{type}.png paths still point at existing files; repairStaleImagePaths won't clear them and DownloadImages only selects empty local_path, so nothing mass-re-downloads on upgrade.
• REQ-ENRICH-050: both Register call sites handle the new error; original factory preserved on duplicate.

Findings (all non-blocking)

1. Medium (doc accuracy): pre-existing collapsed rows are not actually self-healed on persistent-volume deployments. The PR body claims old-naming files "are self-healed by the existing repairStaleImagePaths + re-download cycle" — but that only fires when the file is missing. On a deployment with a persistent /app/data mount, the N rows that collapsed onto one old {id}-{type}.png all keep a non-empty local_path pointing at image Introduce an initial vibe system implementation throughout  #1's bytes indefinitely (DownloadImages internal/services/metadata.go:1530-1537 only selects empty local_path). It heals on the current prod topology (ie01 has no volume mount per ADR-0027, images are wiped on every container update), so this is not blocking — but either correct the claim or add a one-shot backfill that clears local_path for rows sharing a duplicate path.
2. Low: rows whose update.Save persistently fails re-pin the batch front. A row that repeatedly fails the final save (metadata.go:767) never gets its last_enriched_at bumped and sorts first every tick; ≥100 such rows would reintroduce starvation. DB write failures are rare and such rows need retrying anyway — acceptable.
3. Info: artist-avatar behavior change is spec-sanctioned but worth watching. The schema comments is_primary as "primary image of its type" (ent/schema/artistimage.go:44), so an artist can carry multiple primaries (thumbnail + fanart + background); the old handler hard-preferred primary thumbnail, the new ranking can serve a high-likes primary fanart/background as the avatar at /library/artist/{id}.png. REQ-ENRICH-022's text ("best image from all ImageData results") and issue Fix enrichment batch starvation, image collisions, best-image selection #343's explicit targeting of internal/handlers/images.go support the new behavior, so this is correct as implemented — if avatars visually regress, the fix is a spec amendment (per-type preference), not a code revert.
4. Info: cmd/server/main.go conflict likelihood. Sibling work (Consolidate configuration into Viper Config struct; make Lidarr optional #324/Story #324: Consolidate configuration into Viper Config struct; make Lidarr optional #352) also touches this file; the mustRegisterEnricher block is small and confined, so resolution should be trivial — just merge in a deliberate order.
5. Info (pre-existing, out of scope): steady-state churn. Permanently unmatchable rows still re-enrich (external API calls included) every rotation cycle because the Or-predicates keep matching. That's the predicate design, not this PR — a failure-cooldown would be a separate story.

Nice work on the honest test evidence (verifying the regression tests fail pre-fix) and on the deferred-doc-updates list — items 1 above overlaps with the ADR-0027 update already flagged there.

🤖 Posted on behalf of @joestump by Claude.