diff --git a/core/artwork/e2e/helpers_test.go b/core/artwork/e2e/helpers_test.go index e3abca097..4e99f7a5c 100644 --- a/core/artwork/e2e/helpers_test.go +++ b/core/artwork/e2e/helpers_test.go @@ -177,6 +177,9 @@ func (n *noopProvider) TopSongs(context.Context, string, int) (model.MediaFiles, func (n *noopProvider) ArtistImage(context.Context, string) (*url.URL, error) { return nil, model.ErrNotFound } +func (n *noopProvider) ArtistImageResult(context.Context, string) (*url.URL, error) { + return nil, model.ErrNotFound +} func (n *noopProvider) AlbumImage(context.Context, string) (*url.URL, error) { return nil, model.ErrNotFound } diff --git a/core/artwork/sources.go b/core/artwork/sources.go index 04a9257fb..d4ad0a2a7 100644 --- a/core/artwork/sources.go +++ b/core/artwork/sources.go @@ -198,6 +198,18 @@ func fromArtistExternalSource(ctx context.Context, ar model.Artist, provider ext } } +// fromArtistExternalResult is the worker's artist external step: via ArtistImageResult a +// transient agent failure surfaces as an error (extError) rather than settling as absent. +func fromArtistExternalResult(ctx context.Context, ar model.Artist, provider external.Provider) sourceFunc { + return func() (io.ReadCloser, string, error) { + imageUrl, err := provider.ArtistImageResult(ctx, ar.ID) + if err != nil { + return nil, "", err + } + return fromURL(ctx, imageUrl) + } +} + func fromAlbumExternalSource(ctx context.Context, al model.Album, provider external.Provider) sourceFunc { return func() (io.ReadCloser, string, error) { imageUrl, err := provider.AlbumImage(ctx, al.ID) diff --git a/core/external/provider.go b/core/external/provider.go index 459e8a205..42da7eb45 100644 --- a/core/external/provider.go +++ b/core/external/provider.go @@ -36,6 +36,8 @@ type Provider interface { SimilarSongs(ctx context.Context, id string, count int) (model.MediaFiles, error) TopSongs(ctx context.Context, artist string, count int) (model.MediaFiles, error) ArtistImage(ctx context.Context, id string) (*url.URL, error) + // ArtistImageResult is like ArtistImage but reports a transient agent failure as a real error, not ErrNotFound. + ArtistImageResult(ctx context.Context, id string) (*url.URL, error) AlbumImage(ctx context.Context, id string) (*url.URL, error) } @@ -258,7 +260,7 @@ func (e *provider) populateArtistInfo(ctx context.Context, artist auxArtist) (au // Call all registered agents and collect information g := errgroup.Group{} g.SetLimit(2) - g.Go(func() error { e.callGetImage(ctx, e.ag, &artist); return nil }) + g.Go(func() error { _ = e.callGetImage(ctx, e.ag, &artist); return nil }) g.Go(func() error { e.callGetBiography(ctx, e.ag, &artist); return nil }) g.Go(func() error { e.callGetURL(ctx, e.ag, &artist); return nil }) g.Go(func() error { e.callGetSimilarArtists(ctx, e.ag, &artist, maxSimilarArtists, true); return nil }) @@ -371,18 +373,35 @@ func (e *provider) similarSongsFallback(ctx context.Context, id string, count in } func (e *provider) ArtistImage(ctx context.Context, id string) (*url.URL, error) { + u, _, err := e.artistImage(ctx, id) + return u, err +} + +// ArtistImageResult is like ArtistImage but surfaces a transient agent failure as the +// real error, so an agent outage is not mistaken for a definitive no-image (ErrNotFound). +func (e *provider) ArtistImageResult(ctx context.Context, id string) (*url.URL, error) { + u, agentErr, err := e.artistImage(ctx, id) + if agentErr != nil && errors.Is(err, model.ErrNotFound) { + return nil, agentErr + } + return u, err +} + +// artistImage returns the agent error (agentErr) separately from the caller-facing err, +// so ArtistImageResult can tell "agent errored" apart from "definitively no image". +func (e *provider) artistImage(ctx context.Context, id string) (u *url.URL, agentErr error, err error) { artist, err := e.getArtist(ctx, id) if err != nil { - return nil, err + return nil, nil, err } imageUrl := artist.ArtistImageUrl() if imageUrl == "" { // No cached URL — must fetch from external source synchronously - e.callGetImage(ctx, e.ag, &artist) + agentErr = e.callGetImage(ctx, e.ag, &artist) if utils.IsCtxDone(ctx) { log.Warn(ctx, "ArtistImage call canceled", ctx.Err()) - return nil, ctx.Err() + return nil, agentErr, ctx.Err() } imageUrl = artist.ArtistImageUrl() } else { @@ -396,9 +415,10 @@ func (e *provider) ArtistImage(ctx context.Context, id string) (*url.URL, error) } if imageUrl == "" { - return nil, model.ErrNotFound + return nil, agentErr, model.ErrNotFound } - return url.Parse(imageUrl) + u, err = url.Parse(imageUrl) + return u, agentErr, err } func (e *provider) AlbumImage(ctx context.Context, id string) (*url.URL, error) { @@ -519,10 +539,15 @@ func (e *provider) callGetBiography(ctx context.Context, agent agents.ArtistBiog artist.Biography = strings.ReplaceAll(bio, " images[j].Size }) @@ -535,6 +560,7 @@ func (e *provider) callGetImage(ctx context.Context, agent agents.ArtistImageRet if len(images) >= 3 { artist.SmallImageUrl = images[2].URL } + return nil } func (e *provider) callGetSimilarArtists(ctx context.Context, agent agents.ArtistSimilarRetriever, artist *auxArtist, diff --git a/core/external/provider_artistimage_test.go b/core/external/provider_artistimage_test.go index 79612d651..621aac574 100644 --- a/core/external/provider_artistimage_test.go +++ b/core/external/provider_artistimage_test.go @@ -330,6 +330,49 @@ var _ = Describe("Provider - ArtistImage", func() { Expect(logBuf.String()).To(ContainSubstring("Artist image info expired, enqueuing background refresh")) }) + Describe("ArtistImageResult", func() { + It("returns the real agent error on a transient failure, not ErrNotFound", func() { + agentErr := errors.New("agent timed out") + mockImageAgent.Mock = mock.Mock{} + mockImageAgent.On("GetArtistImages", ctx, "artist-1", "Artist One", "").Return(nil, agentErr).Once() + + imgURL, err := provider.ArtistImageResult(ctx, "artist-1") + + Expect(err).To(MatchError(agentErr)) + Expect(err).ToNot(MatchError(model.ErrNotFound)) + Expect(imgURL).To(BeNil()) + }) + + It("returns ErrNotFound when the agent definitively has no image", func() { + mockImageAgent.Mock = mock.Mock{} + mockImageAgent.On("GetArtistImages", ctx, "artist-1", "Artist One", "").Return(nil, agents.ErrNotFound).Once() + + imgURL, err := provider.ArtistImageResult(ctx, "artist-1") + + Expect(err).To(MatchError(model.ErrNotFound)) + Expect(imgURL).To(BeNil()) + }) + + It("returns ErrNotFound when the agent returns no images without error", func() { + mockImageAgent.Mock = mock.Mock{} + mockImageAgent.On("GetArtistImages", ctx, "artist-1", "Artist One", "").Return([]agents.ExternalImage{}, nil).Once() + + imgURL, err := provider.ArtistImageResult(ctx, "artist-1") + + Expect(err).To(MatchError(model.ErrNotFound)) + Expect(imgURL).To(BeNil()) + }) + + It("returns the largest image URL on success", func() { + expectedURL, _ := url.Parse("http://example.com/large.jpg") + + imgURL, err := provider.ArtistImageResult(ctx, "artist-1") + + Expect(err).ToNot(HaveOccurred()) + Expect(imgURL).To(Equal(expectedURL)) + }) + }) + Context("Unicode handling in artist names", func() { var artistWithEnDash *model.Artist var expectedURL *url.URL diff --git a/server/subsonic/e2e/e2e_suite_test.go b/server/subsonic/e2e/e2e_suite_test.go index 6875b6370..98ad6d1ee 100644 --- a/server/subsonic/e2e/e2e_suite_test.go +++ b/server/subsonic/e2e/e2e_suite_test.go @@ -361,6 +361,10 @@ func (n noopProvider) ArtistImage(context.Context, string) (*url.URL, error) { return nil, model.ErrNotFound } +func (n noopProvider) ArtistImageResult(context.Context, string) (*url.URL, error) { + return nil, model.ErrNotFound +} + func (n noopProvider) AlbumImage(context.Context, string) (*url.URL, error) { return nil, model.ErrNotFound }