From 5482784bfc19a5a8a0aae74623d25d9ea0eaa602 Mon Sep 17 00:00:00 2001 From: Deluan Date: Wed, 22 Jul 2026 12:39:16 -0400 Subject: [PATCH] fix(artwork): treat playlist cover URL 404 as definitive miss The playlist ExternalImageURL step used sources.go's fromURL, which maps any non-200 to a generic error, so a stale URL returning 404/410 was classified transient: infinite backoff plus it counted toward the circuit breaker, blocking valid external work. Add a local fetch in resolve.go that maps 404/410 to model.ErrNotFound (definitive) while keeping other non-200s transient. sources.go is left untouched. --- core/artwork/resolve.go | 26 ++++++++++++++++++++++- core/artwork/resolve_test.go | 41 ++++++++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 1 deletion(-) diff --git a/core/artwork/resolve.go b/core/artwork/resolve.go index bb2a3f01b..8597ec836 100644 --- a/core/artwork/resolve.go +++ b/core/artwork/resolve.go @@ -10,12 +10,15 @@ import ( "image/png" "io" "io/fs" + "net/http" "net/url" "os" "strings" + "time" "github.com/Masterminds/squirrel" "github.com/navidrome/navidrome/conf" + "github.com/navidrome/navidrome/consts" "github.com/navidrome/navidrome/core/external" "github.com/navidrome/navidrome/core/ffmpeg" "github.com/navidrome/navidrome/model" @@ -283,7 +286,7 @@ func fromPlaylistExternalSource(ctx context.Context, pl model.Playlist) sourceFu if !conf.Server.EnableM3UExternalAlbumArt { return nil, "", nil } - return fromURL(ctx, parsed) + return fetchPlaylistImageURL(ctx, parsed) } // A missing/unreadable local file is a definitive miss, not a transient // failure to retry: swallow the open error and fall through to the grid. @@ -292,6 +295,27 @@ func fromPlaylistExternalSource(ctx context.Context, pl model.Playlist) sourceFu } } +// Like sources.go's fromURL but maps 404/410 to ErrNotFound (definitive), so a stale M3U +// cover URL falls through to the grid instead of retrying forever and tripping the breaker. +func fetchPlaylistImageURL(ctx context.Context, imageURL *url.URL) (io.ReadCloser, string, error) { + hc := http.Client{Timeout: 5 * time.Second} + req, _ := http.NewRequestWithContext(ctx, http.MethodGet, imageURL.String(), nil) + req.Header.Set("User-Agent", consts.HTTPUserAgent) + resp, err := hc.Do(req) //nolint:gosec + if err != nil { + return nil, "", err + } + if resp.StatusCode == http.StatusNotFound || resp.StatusCode == http.StatusGone { + resp.Body.Close() + return nil, "", model.ErrNotFound + } + if resp.StatusCode != http.StatusOK { + resp.Body.Close() + return nil, "", fmt.Errorf("error retrieving artwork from %s: %s", imageURL, resp.Status) + } + return resp.Body, imageURL.String(), nil +} + 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 21b742345..96702ef33 100644 --- a/core/artwork/resolve_test.go +++ b/core/artwork/resolve_test.go @@ -5,6 +5,8 @@ import ( "errors" "image" "io" + "net/http" + "net/http/httptest" "net/url" "os" "path/filepath" @@ -452,6 +454,45 @@ var _ = Describe("resolveItem", func() { Expect(res.extError).To(BeFalse()) }) + It("treats an ExternalImageURL 404 as a definitive miss and falls through to the grid", func() { + conf.Server.EnableM3UExternalAlbumArt = true + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNotFound) + })) + defer srv.Close() + + plRepo := tests.CreateMockPlaylistRepo() + plRepo.SetData(model.Playlists{{ID: "pl404", Name: "Playlist", ExternalImageURL: srv.URL}}) + plRepo.TracksRepo = &tests.MockPlaylistTrackRepo{AlbumIDs: []string{"t1"}} + ds.MockedPlaylist = plRepo + + res, err := resolveItem(ctx, ds, prov, ffm, model.ArtworkQueueItem{ItemKind: "pl", ItemID: "pl404"}, nil) + Expect(err).ToNot(HaveOccurred()) + Expect(res.reader).ToNot(BeNil()) + defer res.reader.Close() + Expect(res.source).To(Equal("generated")) + Expect(res.extError).To(BeFalse()) + }) + + It("treats an ExternalImageURL 500 as a transient failure and sets extError", func() { + conf.Server.EnableM3UExternalAlbumArt = true + folderRepo.result = nil // no grid tiles, so the external failure is what surfaces + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + })) + defer srv.Close() + + plRepo := tests.CreateMockPlaylistRepo() + plRepo.SetData(model.Playlists{{ID: "pl500", Name: "Playlist", ExternalImageURL: srv.URL}}) + plRepo.TracksRepo = &tests.MockPlaylistTrackRepo{AlbumIDs: []string{"t1"}} + ds.MockedPlaylist = plRepo + + res, err := resolveItem(ctx, ds, prov, ffm, model.ArtworkQueueItem{ItemKind: "pl", ItemID: "pl500"}, nil) + Expect(err).ToNot(HaveOccurred()) + Expect(res.reader).To(BeNil()) + Expect(res.extError).To(BeTrue()) + }) + It("yields an empty resolution when no album has art", func() { ds.MockedAlbum.(*tests.MockAlbumRepo).SetData(model.Albums{ {ID: "empty1", Name: "Empty"},