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

cmd/server/main.go

@@ -147,14 +147,21 @@ func main() {
 	playlistSyncSvc.Register(navidrome.New(logger, cfg))
 
 	// Initialize Metadata Service (for catalog enrichment)
+	// Governing: ADR-0015, SPEC metadata-enrichment-pipeline REQ-ENRICH-050 (duplicate registrations error)
 	metadataSvc := services.NewMetadataService(client, rawDB, cfg, logger, bus)
-	metadataSvc.Register(enrichers.TypeLidarr, enricherLidarr.New(logger, cfg, client))
-	metadataSvc.Register(enrichers.TypeMusicBrainz, enricherMusicbrainz.New(logger, cfg))
-	metadataSvc.Register(enrichers.TypeNavidrome, enricherNavidrome.New(logger, cfg))
-	metadataSvc.Register(enrichers.TypeSpotify, enricherSpotify.New(logger, cfg))
-	metadataSvc.Register(enrichers.TypeLastFM, enricherLastfm.New(logger, cfg))
-	metadataSvc.Register(enrichers.TypeFanart, enricherFanart.New(logger, cfg))
-	metadataSvc.Register(enrichers.TypeOpenAI, enricherOpenai.New(logger, cfg))
+	mustRegisterEnricher := func(t enrichers.Type, factory enrichers.Factory) {
+		if err := metadataSvc.Register(t, factory); err != nil {
+			logger.Error("failed to register enricher", "type", string(t), "error", err)
+			os.Exit(1)
+		}
+	}
+	mustRegisterEnricher(enrichers.TypeLidarr, enricherLidarr.New(logger, cfg, client))
+	mustRegisterEnricher(enrichers.TypeMusicBrainz, enricherMusicbrainz.New(logger, cfg))
+	mustRegisterEnricher(enrichers.TypeNavidrome, enricherNavidrome.New(logger, cfg))
+	mustRegisterEnricher(enrichers.TypeSpotify, enricherSpotify.New(logger, cfg))
+	mustRegisterEnricher(enrichers.TypeLastFM, enricherLastfm.New(logger, cfg))
+	mustRegisterEnricher(enrichers.TypeFanart, enricherFanart.New(logger, cfg))
+	mustRegisterEnricher(enrichers.TypeOpenAI, enricherOpenai.New(logger, cfg))
 
 	// Initialize Mixtape Generator Service (for AI-powered mixtape generation)
 	mixtapeGenerator := vibes.NewMixtapeGenerator(client, cfg, logger, bus)

internal/enrichers/enrichers.go

@@ -6,6 +6,7 @@ package enrichers
 
 import (
 	"context"
+	"fmt"
 
 	"spotter/ent"
 	"spotter/internal/tags"
@@ -217,8 +218,16 @@ func NewRegistry() *Registry {
 }
 
 // Register adds a factory for the given enricher type.
-func (r *Registry) Register(t Type, factory Factory) {
+// Registering the same type twice returns an error instead of silently
+// overwriting the earlier factory.
+// Governing: ADR-0015, SPEC metadata-enrichment-pipeline REQ-ENRICH-050
+// (duplicate type registrations MUST return an error)
+func (r *Registry) Register(t Type, factory Factory) error {
+	if _, exists := r.factories[t]; exists {
+		return fmt.Errorf("enricher type %q is already registered", t)
+	}
 	r.factories[t] = factory
+	return nil
 }
 
 // Get returns the factory for the given enricher type.

internal/enrichers/registry_test.go

@@ -0,0 +1,66 @@
+package enrichers
+
+// Governing: ADR-0015 (type-keyed enricher registry),
+// SPEC metadata-enrichment-pipeline REQ-ENRICH-050 (duplicate type registrations MUST return an error)
+
+import (
+	"context"
+	"testing"
+
+	"spotter/ent"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestRegistry_Register_Succeeds(t *testing.T) {
+	r := NewRegistry()
+
+	err := r.Register(TypeSpotify, func(ctx context.Context, user *ent.User) (Enricher, error) {
+		return nil, nil
+	})
+	require.NoError(t, err)
+
+	_, ok := r.Get(TypeSpotify)
+	assert.True(t, ok, "registered factory should be retrievable")
+}
+
+// TestRegistry_Register_DuplicateTypeErrors verifies REQ-ENRICH-050: registering
+// the same enricher type twice must return an error instead of silently
+// overwriting the earlier factory.
+func TestRegistry_Register_DuplicateTypeErrors(t *testing.T) {
+	r := NewRegistry()
+
+	firstCalled := false
+	first := func(ctx context.Context, user *ent.User) (Enricher, error) {
+		firstCalled = true
+		return nil, nil
+	}
+	second := func(ctx context.Context, user *ent.User) (Enricher, error) {
+		return nil, nil
+	}
+
+	require.NoError(t, r.Register(TypeFanart, first))
+
+	err := r.Register(TypeFanart, second)
+	require.Error(t, err, "duplicate registration must return an error")
+	assert.Contains(t, err.Error(), string(TypeFanart))
+
+	// The original factory must remain registered (no silent overwrite).
+	got, ok := r.Get(TypeFanart)
+	require.True(t, ok)
+	_, _ = got(context.Background(), nil)
+	assert.True(t, firstCalled, "original factory must survive a duplicate registration attempt")
+}
+
+// TestRegistry_Register_DistinctTypesCoexist verifies duplicate detection does
+// not interfere with registering different enricher types.
+func TestRegistry_Register_DistinctTypesCoexist(t *testing.T) {
+	r := NewRegistry()
+	factory := func(ctx context.Context, user *ent.User) (Enricher, error) { return nil, nil }
+
+	for _, typ := range DefaultOrder() {
+		require.NoError(t, r.Register(typ, factory), "registering type %q", typ)
+	}
+	assert.Len(t, r.Types(), len(DefaultOrder()))
+}

internal/handlers/images.go

@@ -8,6 +8,7 @@ import (
 	"strconv"
 	"strings"
 
+	"spotter/ent"
 	"spotter/ent/album"
 	"spotter/ent/artist"
 	"spotter/ent/playlist"
@@ -55,33 +56,97 @@ func (h *Handler) ArtistImage(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	// Governing: issue #127 — skip records with empty local_path to avoid serving 404 when primary image wasn't downloaded
-	// Prefer primary thumbnail, then any primary, then first image with a valid path
-	var localPath string
-	for _, img := range a.Edges.Images {
-		if img.IsPrimary && string(img.ImageType) == "thumbnail" && img.LocalPath != "" {
-			localPath = img.LocalPath
-			break
-		}
+	h.serveImage(w, r, bestArtistImagePath(a.Edges.Images))
+}
+
+// imageCandidate normalizes artist and album image records for best-image ranking.
+type imageCandidate struct {
+	localPath string
+	isPrimary bool
+	likes     int
+	area      int64
+	id        int
+}
+
+// betterImage reports whether a should be served over b.
+// Governing: SPEC metadata-enrichment-pipeline REQ-ENRICH-022 — rank by
+// IsPrimary, then highest Likes, then largest dimensions (Width × Height).
+// ID is a deterministic final tie-breaker.
+func betterImage(a, b imageCandidate) bool {
+	if a.isPrimary != b.isPrimary {
+		return a.isPrimary
+	}
+	if a.likes != b.likes {
+		return a.likes > b.likes
+	}
+	if a.area != b.area {
+		return a.area > b.area
 	}
-	if localPath == "" {
-		for _, img := range a.Edges.Images {
-			if img.IsPrimary && img.LocalPath != "" {
-				localPath = img.LocalPath
-				break
-			}
+	return a.id < b.id
+}
+
+// bestCandidatePath returns the local path of the best-ranked candidate, or ""
+// if there are no candidates.
+func bestCandidatePath(candidates []imageCandidate) string {
+	var best *imageCandidate
+	for i := range candidates {
+		if best == nil || betterImage(candidates[i], *best) {
+			best = &candidates[i]
 		}
 	}
-	if localPath == "" {
-		for _, img := range a.Edges.Images {
-			if img.LocalPath != "" {
-				localPath = img.LocalPath
-				break
-			}
+	if best == nil {
+		return ""
+	}
+	return best.localPath
+}
+
+// bestArtistImagePath selects the best downloaded artist image per
+// REQ-ENRICH-022 (IsPrimary → Likes → dimensions).
+// Governing: SPEC metadata-enrichment-pipeline REQ-ENRICH-022;
+// issue #127 — skip records with empty local_path to avoid serving 404 when the image wasn't downloaded
+func bestArtistImagePath(images []*ent.ArtistImage) string {
+	candidates := make([]imageCandidate, 0, len(images))
+	for _, img := range images {
+		if img.LocalPath == "" {
+			continue
+		}
+		c := imageCandidate{
+			localPath: img.LocalPath,
+			isPrimary: img.IsPrimary,
+			id:        img.ID,
 		}
+		if img.Likes != nil {
+			c.likes = *img.Likes
+		}
+		if img.Width != nil && img.Height != nil {
+			c.area = int64(*img.Width) * int64(*img.Height)
+		}
+		candidates = append(candidates, c)
 	}
+	return bestCandidatePath(candidates)
+}
 
-	h.serveImage(w, r, localPath)
+// bestAlbumImagePath selects the best downloaded album image per
+// REQ-ENRICH-022 (IsPrimary → dimensions; album images carry no Likes field).
+// Governing: SPEC metadata-enrichment-pipeline REQ-ENRICH-022;
+// issue #127 — skip records with empty local_path to avoid serving 404 when the image wasn't downloaded
+func bestAlbumImagePath(images []*ent.AlbumImage) string {
+	candidates := make([]imageCandidate, 0, len(images))
+	for _, img := range images {
+		if img.LocalPath == "" {
+			continue
+		}
+		c := imageCandidate{
+			localPath: img.LocalPath,
+			isPrimary: img.IsPrimary,
+			id:        img.ID,
+		}
+		if img.Width != nil && img.Height != nil {
+			c.area = int64(*img.Width) * int64(*img.Height)
+		}
+		candidates = append(candidates, c)
+	}
+	return bestCandidatePath(candidates)
 }
 
 // AlbumImage serves the primary image for an album
@@ -123,25 +188,7 @@ func (h *Handler) AlbumImage(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	// Governing: issue #127 — skip records with empty local_path to avoid serving 404 when primary image wasn't downloaded
-	// Prefer primary image, then first image with a valid path
-	var localPath string
-	for _, img := range a.Edges.Images {
-		if img.IsPrimary && img.LocalPath != "" {
-			localPath = img.LocalPath
-			break
-		}
-	}
-	if localPath == "" {
-		for _, img := range a.Edges.Images {
-			if img.LocalPath != "" {
-				localPath = img.LocalPath
-				break
-			}
-		}
-	}
-
-	h.serveImage(w, r, localPath)
+	h.serveImage(w, r, bestAlbumImagePath(a.Edges.Images))
 }
 
 // PlaylistImage serves the image for a playlist

internal/handlers/images_selection_test.go

@@ -0,0 +1,103 @@
+package handlers
+
+// Best-image selection tests for REQ-ENRICH-022 (issue #343).
+//
+// Governing: SPEC metadata-enrichment-pipeline REQ-ENRICH-022 — the best image
+// is ranked by IsPrimary, then highest Likes, then largest dimensions
+// (Width × Height). Records without a downloaded local_path are skipped
+// (issue #127).
+
+import (
+	"testing"
+
+	"spotter/ent"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func intPtr(v int) *int { return &v }
+
+func TestBestArtistImagePath_PrimaryBeatsLikesAndSize(t *testing.T) {
+	images := []*ent.ArtistImage{
+		{ID: 1, LocalPath: "data/images/artists/1-fanart-aaaa.png", Likes: intPtr(500), Width: intPtr(4000), Height: intPtr(4000)},
+		{ID: 2, LocalPath: "data/images/artists/1-thumbnail-bbbb.png", IsPrimary: true, Likes: intPtr(1), Width: intPtr(100), Height: intPtr(100)},
+	}
+	assert.Equal(t, "data/images/artists/1-thumbnail-bbbb.png", bestArtistImagePath(images),
+		"IsPrimary must outrank Likes and dimensions")
+}
+
+func TestBestArtistImagePath_LikesBreakPrimaryTie(t *testing.T) {
+	images := []*ent.ArtistImage{
+		{ID: 1, LocalPath: "low-likes.png", IsPrimary: true, Likes: intPtr(3), Width: intPtr(2000), Height: intPtr(2000)},
+		{ID: 2, LocalPath: "high-likes.png", IsPrimary: true, Likes: intPtr(42), Width: intPtr(500), Height: intPtr(500)},
+	}
+	assert.Equal(t, "high-likes.png", bestArtistImagePath(images),
+		"among primaries, higher Likes must win before dimensions are considered")
+}
+
+func TestBestArtistImagePath_DimensionsBreakLikesTie(t *testing.T) {
+	images := []*ent.ArtistImage{
+		{ID: 1, LocalPath: "small.png", Likes: intPtr(10), Width: intPtr(500), Height: intPtr(500)},
+		{ID: 2, LocalPath: "large.png", Likes: intPtr(10), Width: intPtr(1920), Height: intPtr(1080)},
+	}
+	assert.Equal(t, "large.png", bestArtistImagePath(images),
+		"with equal Likes, larger Width × Height must win")
+}
+
+func TestBestArtistImagePath_SkipsRecordsWithoutLocalPath(t *testing.T) {
+	// Governing: issue #127 — an undownloaded primary must not shadow a
+	// downloaded fallback.
+	images := []*ent.ArtistImage{
+		{ID: 1, LocalPath: "", IsPrimary: true, Likes: intPtr(100)},
+		{ID: 2, LocalPath: "downloaded.png"},
+	}
+	assert.Equal(t, "downloaded.png", bestArtistImagePath(images))
+}
+
+func TestBestArtistImagePath_NilLikesAndDimensionsRankLowest(t *testing.T) {
+	images := []*ent.ArtistImage{
+		{ID: 1, LocalPath: "no-metadata.png"},
+		{ID: 2, LocalPath: "with-likes.png", Likes: intPtr(1)},
+	}
+	assert.Equal(t, "with-likes.png", bestArtistImagePath(images),
+		"nil Likes must be treated as zero, not preferred")
+}
+
+func TestBestArtistImagePath_NoServableImages(t *testing.T) {
+	assert.Empty(t, bestArtistImagePath(nil))
+	assert.Empty(t, bestArtistImagePath([]*ent.ArtistImage{{ID: 1, LocalPath: "", IsPrimary: true}}))
+}
+
+func TestBestArtistImagePath_Deterministic(t *testing.T) {
+	// Full ties fall back to lowest ID so serving is stable across requests.
+	images := []*ent.ArtistImage{
+		{ID: 7, LocalPath: "seven.png"},
+		{ID: 3, LocalPath: "three.png"},
+	}
+	assert.Equal(t, "three.png", bestArtistImagePath(images))
+}
+
+func TestBestAlbumImagePath_PrimaryBeatsSize(t *testing.T) {
+	images := []*ent.AlbumImage{
+		{ID: 1, LocalPath: "big.png", Width: intPtr(3000), Height: intPtr(3000)},
+		{ID: 2, LocalPath: "primary.png", IsPrimary: true, Width: intPtr(200), Height: intPtr(200)},
+	}
+	assert.Equal(t, "primary.png", bestAlbumImagePath(images),
+		"IsPrimary must outrank dimensions")
+}
+
+func TestBestAlbumImagePath_DimensionsBreakTie(t *testing.T) {
+	images := []*ent.AlbumImage{
+		{ID: 1, LocalPath: "small.png", Width: intPtr(300), Height: intPtr(300)},
+		{ID: 2, LocalPath: "large.png", Width: intPtr(1400), Height: intPtr(1400)},
+	}
+	assert.Equal(t, "large.png", bestAlbumImagePath(images))
+}
+
+func TestBestAlbumImagePath_SkipsRecordsWithoutLocalPath(t *testing.T) {
+	images := []*ent.AlbumImage{
+		{ID: 1, LocalPath: "", IsPrimary: true},
+		{ID: 2, LocalPath: "fallback.png"},
+	}
+	assert.Equal(t, "fallback.png", bestAlbumImagePath(images))
+}