diff --git a/core/artwork/resolve.go b/core/artwork/resolve.go index bd314e37d..5aafdf5c5 100644 --- a/core/artwork/resolve.go +++ b/core/artwork/resolve.go @@ -81,11 +81,9 @@ func resolveAlbum(ctx context.Context, ds model.DataStore, prov external.Provide return res, nil } case pattern == "external": - r, path, err := extGate(fromAlbumExternalSource(ctx, *al, prov)) - if r != nil { - return resolution{reader: r, source: "external", sourcePath: path}, nil - } - if err != nil && !errors.Is(err, model.ErrNotFound) { + if res, ok, isErr := resolveExternalStep(extGate, fromAlbumExternalSource(ctx, *al, prov)); ok { + return res, nil + } else if isErr { extErr = true } case len(imgFiles) > 0: @@ -139,11 +137,9 @@ func resolveArtist(ctx context.Context, ds model.DataStore, prov external.Provid pattern = strings.TrimSpace(pattern) switch { case pattern == "external": - r, path, err := extGate(fromArtistExternalSource(ctx, *ar, prov)) - if r != nil { - return resolution{reader: r, source: "external", sourcePath: path}, nil - } - if err != nil && !errors.Is(err, model.ErrNotFound) { + if res, ok, isErr := resolveExternalStep(extGate, fromArtistExternalSource(ctx, *ar, prov)); ok { + return res, nil + } else if isErr { extErr = true } case pattern == "image-folder": @@ -183,9 +179,13 @@ func resolvePlaylist(ctx context.Context, ds model.DataStore, prov external.Prov var tiles []image.Image var extErr bool + var tileErr error // first internal (non-external) tile failure, e.g. album deleted mid-flight for _, albumID := range albumIDs { res, err := resolveAlbum(ctx, ds, prov, ffm, albumID, extGate) if err != nil { + if tileErr == nil { + tileErr = err + } continue } if res.extError { @@ -203,10 +203,16 @@ func resolvePlaylist(ctx context.Context, ds model.DataStore, prov external.Prov break } } + if len(tiles) == 0 { + // A tile-level failure must never resolve as a clean absent: propagate + // internal errors, and force extError for external ones. + if tileErr != nil { + return resolution{}, fmt.Errorf("resolvePlaylist: sampled album art failed: %w", tileErr) + } + return resolution{extError: extErr}, nil + } // Grow to 4 tiles by repeating what we have, mirroring reader_playlist.go's loadTiles. switch len(tiles) { - case 0: - return resolution{extError: extErr}, nil case 2: tiles = append(tiles, tiles[1], tiles[0]) case 3: @@ -229,6 +235,17 @@ func resolveRadio(ctx context.Context, ds model.DataStore, radioID string) (reso return res, nil } +// resolveExternalStep runs an external sourceFunc through extGate, shared by +// resolveAlbum and resolveArtist. ok reports a hit; extErr reports a +// non-not-found error (a not-found is a definitive "no", not a failure). +func resolveExternalStep(extGate extGateFunc, sf func() (io.ReadCloser, string, error)) (res resolution, ok bool, extErr bool) { + r, path, err := extGate(sf) + if r != nil { + return resolution{reader: r, source: "external", sourcePath: path}, true, false + } + return resolution{}, false, err != nil && !errors.Is(err, model.ErrNotFound) +} + func resolveEmbedded(ctx context.Context, lib libraryView, ffm ffmpeg.FFmpeg, embedRel string) (resolution, bool) { if embedRel == "" { return resolution{}, false diff --git a/core/artwork/resolve_test.go b/core/artwork/resolve_test.go index c3fda1f1d..c4ccc3733 100644 --- a/core/artwork/resolve_test.go +++ b/core/artwork/resolve_test.go @@ -207,6 +207,54 @@ var _ = Describe("resolveItem", func() { Expect(res.source).To(Equal("folder")) Expect(filepath.ToSlash(res.sourcePath)).To(HaveSuffix("tests/fixtures/artist/an-album/artist.png")) }) + + It("sets extError when the external source errors without being not-found", func() { + conf.Server.ArtistArtPriority = "external" + artistRepo := tests.CreateMockArtistRepo() + artistRepo.SetData(model.Artists{{ID: "ar3", Name: "Artist"}}) + ds.MockedArtist = artistRepo + prov.artistImage = func(context.Context, string) (*url.URL, error) { + return nil, errors.New("agent timed out") + } + + res, err := resolveItem(ctx, ds, prov, ffm, model.ArtworkQueueItem{ItemKind: "ar", ItemID: "ar3"}, nil) + Expect(err).ToNot(HaveOccurred()) + Expect(res.reader).To(BeNil()) + Expect(res.extError).To(BeTrue()) + }) + + It("does not set extError when the external source reports not-found", func() { + conf.Server.ArtistArtPriority = "external" + artistRepo := tests.CreateMockArtistRepo() + artistRepo.SetData(model.Artists{{ID: "ar4", Name: "Artist"}}) + ds.MockedArtist = artistRepo + // prov.artistImage left nil -> fakeExternalProvider returns model.ErrNotFound + + res, err := resolveItem(ctx, ds, prov, ffm, model.ArtworkQueueItem{ItemKind: "ar", ItemID: "ar4"}, nil) + Expect(err).ToNot(HaveOccurred()) + Expect(res.reader).To(BeNil()) + Expect(res.extError).To(BeFalse()) + }) + + It("routes the external step through a custom extGate", func() { + conf.Server.ArtistArtPriority = "external" + artistRepo := tests.CreateMockArtistRepo() + artistRepo.SetData(model.Artists{{ID: "ar5", Name: "Artist"}}) + ds.MockedArtist = artistRepo + prov.artistImage = func(context.Context, string) (*url.URL, error) { + return nil, errors.New("boom") + } + var extGateCalls int + extGate := func(f func() (io.ReadCloser, string, error)) (io.ReadCloser, string, error) { + extGateCalls++ + return f() + } + + res, err := resolveItem(ctx, ds, prov, ffm, model.ArtworkQueueItem{ItemKind: "ar", ItemID: "ar5"}, extGate) + Expect(err).ToNot(HaveOccurred()) + Expect(res.extError).To(BeTrue()) + Expect(extGateCalls).To(Equal(1)) + }) }) Describe("radio", func() { @@ -301,5 +349,18 @@ var _ = Describe("resolveItem", func() { Expect(res.reader).To(BeNil()) Expect(res.source).To(BeEmpty()) }) + + It("does not resolve as absent when every sampled album fails to resolve", func() { + // "missing1"/"missing2" are not in MockAlbumRepo's data, so resolveAlbum + // returns a genuine (non-external) error for every sampled tile. + plRepo := tests.CreateMockPlaylistRepo() + plRepo.SetData(model.Playlists{{ID: "pl3", Name: "Playlist"}}) + plRepo.TracksRepo = &tests.MockPlaylistTrackRepo{AlbumIDs: []string{"missing1", "missing2"}} + ds.MockedPlaylist = plRepo + + res, err := resolveItem(ctx, ds, prov, ffm, model.ArtworkQueueItem{ItemKind: "pl", ItemID: "pl3"}, nil) + Expect(err).To(HaveOccurred()) + Expect(res).To(Equal(resolution{})) + }) }) })