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.
This commit is contained in:
Deluan 2026-07-22 12:39:16 -04:00
parent 6afcb93a9b
commit 5482784bfc
2 changed files with 66 additions and 1 deletions

View File

@ -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

View File

@ -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"},