diff --git a/cmd/server/main.go b/cmd/server/main.go index de441d1..9ea9749 100644 --- a/cmd/server/main.go +++ b/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) diff --git a/internal/enrichers/enrichers.go b/internal/enrichers/enrichers.go index 0357f35..8cbc0ba 100644 --- a/internal/enrichers/enrichers.go +++ b/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. diff --git a/internal/enrichers/registry_test.go b/internal/enrichers/registry_test.go new file mode 100644 index 0000000..c8203e4 --- /dev/null +++ b/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())) +} diff --git a/internal/handlers/images.go b/internal/handlers/images.go index 6a8ade3..b65984c 100644 --- a/internal/handlers/images.go +++ b/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 diff --git a/internal/handlers/images_selection_test.go b/internal/handlers/images_selection_test.go new file mode 100644 index 0000000..b65938e --- /dev/null +++ b/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)) +} diff --git a/internal/services/image_filename_collision_test.go b/internal/services/image_filename_collision_test.go new file mode 100644 index 0000000..6619269 --- /dev/null +++ b/internal/services/image_filename_collision_test.go @@ -0,0 +1,188 @@ +package services + +// Filename-collision regression tests for issue #343. +// +// Image rows are deduped by URL, but the local filename used to be +// {entityID}-{imageType}{ext}. N same-type images for one entity therefore +// collapsed onto a single file: the first download created the file and every +// later row hit the os.Stat exists-branch and pointed its local_path at image +// #1's bytes. Filenames now include a per-image URL hash discriminator. +// +// Governing: ADR-0027 (filesystem image storage), +// SPEC metadata-enrichment-pipeline REQ-ENRICH-030 (images downloaded locally) + +import ( + "context" + "fmt" + "io" + "log/slog" + "net/http" + "net/http/httptest" + "os" + "testing" + + "spotter/ent/albumimage" + "spotter/ent/artistimage" + "spotter/ent/enttest" + "spotter/internal/config" + + _ "github.com/mattn/go-sqlite3" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func newImageDownloadTestService(t *testing.T) (*MetadataService, *httptest.Server) { + t.Helper() + client := enttest.Open(t, "sqlite3", fmt.Sprintf("file:imgdl_%s?mode=memory&cache=shared&_fk=1", t.Name())) + t.Cleanup(func() { client.Close() }) + + // Each URL path returns distinct bytes so collapsed files are detectable. + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, _ = fmt.Fprintf(w, "image-bytes-for:%s", r.URL.Path) + })) + t.Cleanup(server.Close) + + svc := &MetadataService{ + client: client, + logger: slog.New(slog.NewTextHandler(io.Discard, nil)), + config: &config.Config{}, + httpClient: server.Client(), + } + return svc, server +} + +// TestDownloadArtistImage_SameTypeImagesGetDistinctFiles verifies that an +// artist with 5 fanart images stores 5 distinct files with distinct contents, +// and each DB row's local_path points at its own bytes. +func TestDownloadArtistImage_SameTypeImagesGetDistinctFiles(t *testing.T) { + svc, server := newImageDownloadTestService(t) + ctx := context.Background() + baseDir := t.TempDir() + + u, err := svc.client.User.Create().SetUsername("imguser").Save(ctx) + require.NoError(t, err) + art, err := svc.client.Artist.Create().SetName("Fanart Artist").SetUser(u).Save(ctx) + require.NoError(t, err) + + const count = 5 + for i := 0; i < count; i++ { + _, err := svc.client.ArtistImage.Create(). + SetArtist(art). + SetSource("fanart"). + SetURL(fmt.Sprintf("%s/fanart/%d.png", server.URL, i)). + SetImageType(artistimage.ImageTypeFanart). + Save(ctx) + require.NoError(t, err) + } + + images, err := svc.client.ArtistImage.Query().WithArtist().All(ctx) + require.NoError(t, err) + require.Len(t, images, count) + + for _, img := range images { + require.NoError(t, svc.downloadArtistImage(ctx, u, img, baseDir)) + } + + updated, err := svc.client.ArtistImage.Query().All(ctx) + require.NoError(t, err) + + paths := make(map[string]bool, count) + contents := make(map[string]bool, count) + for _, img := range updated { + require.NotEmpty(t, img.LocalPath, "image %d must have a local path", img.ID) + paths[img.LocalPath] = true + + data, err := os.ReadFile(img.LocalPath) + require.NoError(t, err, "file for image %d must exist on disk", img.ID) + contents[string(data)] = true + } + + assert.Len(t, paths, count, "%d same-type images must produce %d distinct files, not collapse onto one", count, count) + assert.Len(t, contents, count, "each image row must point at its own bytes") +} + +// TestDownloadAlbumImage_SameTypeImagesGetDistinctFiles mirrors the artist test +// for album images (metadata.go downloadAlbumImage). +func TestDownloadAlbumImage_SameTypeImagesGetDistinctFiles(t *testing.T) { + svc, server := newImageDownloadTestService(t) + ctx := context.Background() + baseDir := t.TempDir() + + u, err := svc.client.User.Create().SetUsername("albimguser").Save(ctx) + require.NoError(t, err) + art, err := svc.client.Artist.Create().SetName("Cover Artist").SetUser(u).Save(ctx) + require.NoError(t, err) + alb, err := svc.client.Album.Create().SetName("Cover Album").SetUser(u).SetArtist(art).Save(ctx) + require.NoError(t, err) + + const count = 3 + for i := 0; i < count; i++ { + _, err := svc.client.AlbumImage.Create(). + SetAlbum(alb). + SetSource("fanart"). + SetURL(fmt.Sprintf("%s/covers/%d.png", server.URL, i)). + SetImageType(albumimage.ImageTypeCoverFront). + Save(ctx) + require.NoError(t, err) + } + + images, err := svc.client.AlbumImage.Query().WithAlbum().All(ctx) + require.NoError(t, err) + require.Len(t, images, count) + + for _, img := range images { + require.NoError(t, svc.downloadAlbumImage(ctx, u, img, baseDir)) + } + + updated, err := svc.client.AlbumImage.Query().All(ctx) + require.NoError(t, err) + + paths := make(map[string]bool, count) + for _, img := range updated { + require.NotEmpty(t, img.LocalPath, "image %d must have a local path", img.ID) + paths[img.LocalPath] = true + } + assert.Len(t, paths, count, "%d same-type album images must produce %d distinct files", count, count) +} + +// TestDownloadArtistImage_ExistingFileSkipsRedownload verifies REQ-ENRICH-033 +// still holds with hashed filenames: re-downloading the SAME image (same URL) +// hits the exists-branch and does not re-fetch. +func TestDownloadArtistImage_ExistingFileSkipsRedownload(t *testing.T) { + svc, server := newImageDownloadTestService(t) + ctx := context.Background() + baseDir := t.TempDir() + + u, err := svc.client.User.Create().SetUsername("skipuser").Save(ctx) + require.NoError(t, err) + art, err := svc.client.Artist.Create().SetName("Skip Artist").SetUser(u).Save(ctx) + require.NoError(t, err) + + _, err = svc.client.ArtistImage.Create(). + SetArtist(art). + SetSource("fanart"). + SetURL(server.URL + "/fanart/same.png"). + SetImageType(artistimage.ImageTypeFanart). + Save(ctx) + require.NoError(t, err) + + img, err := svc.client.ArtistImage.Query().WithArtist().Only(ctx) + require.NoError(t, err) + + require.NoError(t, svc.downloadArtistImage(ctx, u, img, baseDir)) + first, err := svc.client.ArtistImage.Query().Only(ctx) + require.NoError(t, err) + require.NotEmpty(t, first.LocalPath) + + // Overwrite the on-disk bytes; a second download for the same URL must + // keep the existing file (exists-branch) rather than re-fetching. + require.NoError(t, os.WriteFile(first.LocalPath, []byte("sentinel"), 0644)) + + img, err = svc.client.ArtistImage.Query().WithArtist().Only(ctx) + require.NoError(t, err) + require.NoError(t, svc.downloadArtistImage(ctx, u, img, baseDir)) + + data, err := os.ReadFile(first.LocalPath) + require.NoError(t, err) + assert.Equal(t, "sentinel", string(data), "same-URL re-download must hit the exists-branch and keep the file") +} diff --git a/internal/services/metadata.go b/internal/services/metadata.go index a50def9..de84def 100644 --- a/internal/services/metadata.go +++ b/internal/services/metadata.go @@ -4,7 +4,9 @@ package services import ( "context" + "crypto/sha256" "database/sql" + "encoding/hex" "encoding/json" "fmt" "io" @@ -15,6 +17,8 @@ import ( "strings" "time" + entsql "entgo.io/ent/dialect/sql" + "spotter/ent" "spotter/ent/album" "spotter/ent/albumimage" @@ -61,9 +65,36 @@ func NewMetadataService(client *ent.Client, db *sql.DB, cfg *config.Config, logg } } +// byLastEnrichedAtNullsFirst orders enrichment batches so never-enriched rows +// (NULL last_enriched_at) come first, then the least-recently-enriched, with ID +// as a deterministic tie-breaker. Because each processed row gets its +// last_enriched_at bumped, successive Limit(N) batches rotate through the whole +// library instead of re-selecting the same first N rows — even for rows whose +// Or-predicates (e.g. LidarrIDIsNil) still match after enrichment. +// +// The NULL grouping is expressed as a portable boolean sort key ("IS NOT NULL" +// ascending puts NULLs first) because NULL ordering defaults differ across +// dialects (Postgres sorts NULLs last on ASC) and MySQL does not support the +// NULLS FIRST clause. +// +// Governing: SPEC metadata-enrichment-pipeline REQ-ENRICH-040 (enrich ALL +// un-enriched or stale entities), issue #343 (batch starvation) +func byLastEnrichedAtNullsFirst(lastEnrichedAtField, idField string) func(*entsql.Selector) { + return func(s *entsql.Selector) { + col := s.C(lastEnrichedAtField) + s.OrderExpr( + entsql.Expr(col+" IS NOT NULL"), + entsql.Expr(col), + entsql.Expr(s.C(idField)), + ) + } +} + // Register adds a new enricher factory to the service. -func (s *MetadataService) Register(t enrichers.Type, factory enrichers.Factory) { - s.registry.Register(t, factory) +// Governing: ADR-0015, SPEC metadata-enrichment-pipeline REQ-ENRICH-050 +// (duplicate type registrations MUST return an error) +func (s *MetadataService) Register(t enrichers.Type, factory enrichers.Factory) error { + return s.registry.Register(t, factory) } // GetEnricherFactory returns the factory for the given enricher type, if registered. @@ -572,6 +603,9 @@ func (s *MetadataService) EnrichArtists(ctx context.Context, u *ent.User) (int, q.WithAlbum() }). WithImages(). + // Rotate batches through the library so rows beyond the limit are not starved. + // Governing: issue #343 (batch starvation) + Order(byLastEnrichedAtNullsFirst(artist.FieldLastEnrichedAt, artist.FieldID)). Limit(100). // Process in batches All(ctx) if err != nil { @@ -859,6 +893,9 @@ func (s *MetadataService) EnrichAlbums(ctx context.Context, u *ent.User) (int, e WithArtist(). WithTracks(). WithImages(). + // Rotate batches through the library so rows beyond the limit are not starved. + // Governing: issue #343 (batch starvation) + Order(byLastEnrichedAtNullsFirst(album.FieldLastEnrichedAt, album.FieldID)). Limit(100). All(ctx) if err != nil { @@ -1282,6 +1319,9 @@ func (s *MetadataService) EnrichTracks(ctx context.Context, u *ent.User) (int, e q.Where(artist.HasUserWith(user.ID(u.ID))) }). WithAlbum(). + // Rotate batches through the library so rows beyond the limit are not starved. + // Governing: issue #343 (batch starvation) + Order(byLastEnrichedAtNullsFirst(track.FieldLastEnrichedAt, track.FieldID)). Limit(200). All(ctx) if err != nil { @@ -1589,9 +1629,12 @@ func (s *MetadataService) downloadArtistImage(ctx context.Context, u *ent.User, return err } - // Determine filename using artist ID and image type (e.g., 123-hero.png) + // Determine filename using artist ID, image type, and a per-image URL hash + // (e.g., 123-fanart-a1b2c3d4.png). Rows are deduped by URL, so N same-type + // images must not collapse onto one file via the os.Stat exists-branch below. + // Governing: ADR-0027 (filesystem image storage), issue #343 (filename collisions) ext := getImageExtension(img.URL) - filename := fmt.Sprintf("%d-%s%s", img.Edges.Artist.ID, img.ImageType.String(), ext) + filename := fmt.Sprintf("%d-%s-%s%s", img.Edges.Artist.ID, img.ImageType.String(), imageURLHash(img.URL), ext) localPath := filepath.Join(artistDir, filename) // Check if file already exists on disk @@ -1640,9 +1683,12 @@ func (s *MetadataService) downloadAlbumImage(ctx context.Context, u *ent.User, i return err } - // Determine filename using album ID and image type (e.g., 456-cover.png) + // Determine filename using album ID, image type, and a per-image URL hash + // (e.g., 456-cover_front-a1b2c3d4.png). Rows are deduped by URL, so N same-type + // images must not collapse onto one file via the os.Stat exists-branch below. + // Governing: ADR-0027 (filesystem image storage), issue #343 (filename collisions) ext := getImageExtension(img.URL) - filename := fmt.Sprintf("%d-%s%s", img.Edges.Album.ID, img.ImageType.String(), ext) + filename := fmt.Sprintf("%d-%s-%s%s", img.Edges.Album.ID, img.ImageType.String(), imageURLHash(img.URL), ext) localPath := filepath.Join(albumDir, filename) // Check if file already exists on disk @@ -1901,6 +1947,15 @@ func uniqueStrings(s []string) []string { return result } +// imageURLHash returns a short, stable hash of an image URL. It is used as a +// per-image filename discriminator so multiple same-type images for one entity +// are written to distinct files instead of colliding on {id}-{type}{ext}. +// Governing: ADR-0027 (filesystem image storage), issue #343 (filename collisions) +func imageURLHash(url string) string { + sum := sha256.Sum256([]byte(url)) + return hex.EncodeToString(sum[:4]) +} + // getImageExtension extracts the file extension from a URL. func getImageExtension(url string) string { // Try to get extension from URL diff --git a/internal/services/metadata_service_test.go b/internal/services/metadata_service_test.go index 43bb82a..df940cc 100644 --- a/internal/services/metadata_service_test.go +++ b/internal/services/metadata_service_test.go @@ -28,7 +28,7 @@ func TestMetadataService_GetEnricherFactory_Registered(t *testing.T) { called = true return nil, nil } - svc.Register(enrichers.TypeSpotify, factory) + require.NoError(t, svc.Register(enrichers.TypeSpotify, factory)) // Get the factory back got, ok := svc.GetEnricherFactory(enrichers.TypeSpotify) diff --git a/internal/services/metadata_starvation_test.go b/internal/services/metadata_starvation_test.go new file mode 100644 index 0000000..d04da6f --- /dev/null +++ b/internal/services/metadata_starvation_test.go @@ -0,0 +1,181 @@ +package services + +// Batch-starvation regression tests for issue #343. +// +// The enrichment queries use Limit(100) (tracks: 200) with Or-predicates such as +// LidarrIDIsNil that still match rows immediately after they are enriched. Without +// an explicit order, 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 +// (NULLs first) so successive ticks rotate through the entire library. +// +// Governing: SPEC metadata-enrichment-pipeline REQ-ENRICH-040 (enrich ALL +// un-enriched or stale entities), issue #343 (batch starvation) + +import ( + "context" + "fmt" + "io" + "log/slog" + "testing" + + "spotter/ent/album" + "spotter/ent/artist" + "spotter/ent/enttest" + "spotter/ent/track" + "spotter/internal/config" + "spotter/internal/enrichers" + + _ "github.com/mattn/go-sqlite3" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func newStarvationTestService(t *testing.T) *MetadataService { + t.Helper() + client := enttest.Open(t, "sqlite3", fmt.Sprintf("file:starvation_%s?mode=memory&cache=shared&_fk=1", t.Name())) + t.Cleanup(func() { client.Close() }) + + return &MetadataService{ + client: client, + logger: slog.New(slog.NewTextHandler(io.Discard, nil)), + config: &config.Config{}, + registry: enrichers.NewRegistry(), + } +} + +// TestEnrichArtists_NoBatchStarvation creates 250 never-matchable artists +// (no Lidarr ID, so they keep matching the LidarrIDIsNil Or-predicate even +// after being enriched) and verifies that all of them receive enrichment +// across successive ticks with a batch limit of 100. +func TestEnrichArtists_NoBatchStarvation(t *testing.T) { + svc := newStarvationTestService(t) + ctx := context.Background() + + u, err := svc.client.User.Create().SetUsername("starvation-artists").Save(ctx) + require.NoError(t, err) + + const total = 250 + for i := 0; i < total; i++ { + _, err := svc.client.Artist.Create(). + SetName(fmt.Sprintf("Artist %03d", i)). + SetUser(u). + Save(ctx) + require.NoError(t, err) + } + + // 250 artists at Limit(100) per tick: 3 ticks must cover everyone. + for tick := 1; tick <= 3; tick++ { + _, err := svc.EnrichArtists(ctx, u) + require.NoError(t, err, "tick %d", tick) + } + + unenriched, err := svc.client.Artist.Query(). + Where(artist.LastEnrichedAtIsNil()). + Count(ctx) + require.NoError(t, err) + assert.Zero(t, unenriched, "all %d artists must be enriched after 3 ticks (no starvation)", total) +} + +// TestEnrichArtists_BatchesRotate verifies the rotation mechanics directly: +// the second tick must select the rows the first tick skipped, not re-select +// the just-enriched rows that still match the Or-predicates. +func TestEnrichArtists_BatchesRotate(t *testing.T) { + svc := newStarvationTestService(t) + ctx := context.Background() + + u, err := svc.client.User.Create().SetUsername("rotation-artists").Save(ctx) + require.NoError(t, err) + + const total = 150 + for i := 0; i < total; i++ { + _, err := svc.client.Artist.Create(). + SetName(fmt.Sprintf("Artist %03d", i)). + SetUser(u). + Save(ctx) + require.NoError(t, err) + } + + _, err = svc.EnrichArtists(ctx, u) + require.NoError(t, err) + + afterFirst, err := svc.client.Artist.Query(). + Where(artist.LastEnrichedAtNotNil()). + Count(ctx) + require.NoError(t, err) + assert.Equal(t, 100, afterFirst, "first tick enriches one full batch") + + _, err = svc.EnrichArtists(ctx, u) + require.NoError(t, err) + + afterSecond, err := svc.client.Artist.Query(). + Where(artist.LastEnrichedAtNotNil()). + Count(ctx) + require.NoError(t, err) + assert.Equal(t, total, afterSecond, + "second tick must pick up the remaining %d never-enriched artists instead of re-selecting the first batch", total-100) +} + +// TestEnrichAlbums_NoBatchStarvation mirrors the artist test for the album +// query (Limit(100)). +func TestEnrichAlbums_NoBatchStarvation(t *testing.T) { + svc := newStarvationTestService(t) + ctx := context.Background() + + u, err := svc.client.User.Create().SetUsername("starvation-albums").Save(ctx) + require.NoError(t, err) + art, err := svc.client.Artist.Create().SetName("Album Artist").SetUser(u).Save(ctx) + require.NoError(t, err) + + const total = 120 + for i := 0; i < total; i++ { + _, err := svc.client.Album.Create(). + SetName(fmt.Sprintf("Album %03d", i)). + SetUser(u). + SetArtist(art). + Save(ctx) + require.NoError(t, err) + } + + for tick := 1; tick <= 2; tick++ { + _, err := svc.EnrichAlbums(ctx, u) + require.NoError(t, err, "tick %d", tick) + } + + unenriched, err := svc.client.Album.Query(). + Where(album.LastEnrichedAtIsNil()). + Count(ctx) + require.NoError(t, err) + assert.Zero(t, unenriched, "all %d albums must be enriched after 2 ticks (no starvation)", total) +} + +// TestEnrichTracks_NoBatchStarvation mirrors the artist test for the track +// query (Limit(200)). +func TestEnrichTracks_NoBatchStarvation(t *testing.T) { + svc := newStarvationTestService(t) + ctx := context.Background() + + u, err := svc.client.User.Create().SetUsername("starvation-tracks").Save(ctx) + require.NoError(t, err) + art, err := svc.client.Artist.Create().SetName("Track Artist").SetUser(u).Save(ctx) + require.NoError(t, err) + + const total = 250 + for i := 0; i < total; i++ { + _, err := svc.client.Track.Create(). + SetName(fmt.Sprintf("Track %03d", i)). + SetArtist(art). + Save(ctx) + require.NoError(t, err) + } + + for tick := 1; tick <= 2; tick++ { + _, err := svc.EnrichTracks(ctx, u) + require.NoError(t, err, "tick %d", tick) + } + + unenriched, err := svc.client.Track.Query(). + Where(track.LastEnrichedAtIsNil()). + Count(ctx) + require.NoError(t, err) + assert.Zero(t, unenriched, "all %d tracks must be enriched after 2 ticks (no starvation)", total) +}