From 28f3c720daea502303ef477ba103f0f9cdf1cf20 Mon Sep 17 00:00:00 2001 From: Deluan Date: Sun, 26 Jul 2026 20:16:18 -0400 Subject: [PATCH] fix(artwork): never settle absent for a kind no recheck job revisits The 12h retry budget hands a bare failure to the periodic stale-absent sweep, which is what makes the resulting absent row recoverable. Media files are deliberately excluded from that sweep -- they resolve embedded only, at scan or on view -- so exhausting the budget on a transient read error recorded a "this track has no cover" verdict that nothing would ever revisit. Settle absent only for kinds a recheck job covers. Without a row the track stays unresolved, so the next view re-enqueues it. --- core/artwork/housekeeping.go | 8 ++++++++ core/artwork/worker.go | 10 +++++----- core/artwork/worker_test.go | 26 ++++++++++++++++++++++++++ 3 files changed, 39 insertions(+), 5 deletions(-) diff --git a/core/artwork/housekeeping.go b/core/artwork/housekeeping.go index 3f0e98bad..365438549 100644 --- a/core/artwork/housekeeping.go +++ b/core/artwork/housekeeping.go @@ -5,6 +5,7 @@ import ( "crypto/md5" "encoding/hex" "fmt" + "slices" "time" "github.com/navidrome/navidrome/conf" @@ -23,6 +24,13 @@ var recheckKinds = []model.Kind{ model.KindArtistArtwork, model.KindAlbumArtwork, model.KindPlaylistArtwork, model.KindRadioArtwork, } +// hasRecheckPath reports whether a periodic job will revisit this kind, which is what makes +// settling absent on an exhausted retry budget recoverable rather than permanent. +func hasRecheckPath(prefix string) bool { + kind, ok := model.ParseKind(prefix) + return ok && slices.Contains(recheckKinds, kind) +} + // artworkEpoch invalidates all resolution state when bumped; bump it in the same change that // alters resolution semantics. Deliberately not the server version, which changes every build. const artworkEpoch = 1 diff --git a/core/artwork/worker.go b/core/artwork/worker.go index eb935d781..68af3080b 100644 --- a/core/artwork/worker.go +++ b/core/artwork/worker.go @@ -273,11 +273,11 @@ func (w *Worker) process(ctx context.Context, item model.ArtworkQueueItem) (outc } break } - // Retry budget exhausted: stop retrying. A bare failure settles absent so the stale-absent - // sweep (and a page view) can still recover it; a stale-found keeps its already-served art. - // Art we are already serving is kept too: exhaustion means the source stayed unreachable, - // not that the entity lost its cover. - if out == outcomeFailed && !w.hasResolvedArtwork(ctx, item) { + // Retry budget exhausted: stop retrying. Absent is only recoverable where a periodic + // recheck will revisit it, so kinds without one keep no row at all; and art already + // being served is kept, since exhaustion means the source stayed unreachable rather + // than that the entity lost its cover. + if out == outcomeFailed && hasRecheckPath(item.ItemKind) && !w.hasResolvedArtwork(ctx, item) { writeAbsent(ctx, w.deps.ds.Artwork(ctx), item) } if err := queue.DeleteIfUnchanged(item.ItemKind, item.ItemID, item.ImageType, item.RetryAt); err != nil { diff --git a/core/artwork/worker_test.go b/core/artwork/worker_test.go index 5271592ba..64e345616 100644 --- a/core/artwork/worker_test.go +++ b/core/artwork/worker_test.go @@ -364,6 +364,32 @@ var _ = Describe("Worker", func() { Expect(ia.Hash).To(Equal("cafebabe"), "a persistent outage must not discard served art") }) + // Media files are excluded from recheckKinds, so an absent row written here would never + // be revisited: a transient read error would look like "this track has no cover" forever. + It("does not settle absent on exhaustion for a kind with no recheck path", func() { + conf.Server.EnableMediaFileCoverArt = true + ds.MockedMediaFile = tests.CreateMockMediaFileRepo() + ds.MockedMediaFile.(*tests.MockMediaFileRepo).SetData(model.MediaFiles{ + {ID: "mfX", LibraryID: 0, Path: "tests/fixtures/artist/an-album/gone.mp3", HasCoverArt: true}, + }) + Expect(queueRepo.Enqueue(model.ArtworkQueueItem{ItemKind: "mf", ItemID: "mfX"})).To(Succeed()) + for k, v := range queueRepo.Data { + if v.ItemID == "mfX" { + v.EnqueuedAt = time.Now().Add(-(giveUpAfter + time.Hour)) + queueRepo.Data[k] = v + } + } + + n, err := w.drain(ctx, 1) + Expect(err).ToNot(HaveOccurred()) + Expect(n).To(Equal(1)) + + Expect(findQueued(queueRepo, "mf", "mfX")).To(BeNil(), "the row must stop retrying") + _, err = artRepo.GetItemArtwork(model.KindMediaFileArtwork, "mfX", model.ImageTypePrimary) + Expect(err).To(MatchError(model.ErrNotFound), + "no row leaves the track unresolved, so a later view can still recover it") + }) + It("resolves a private playlist under an admin context instead of failing forever", func() { ds.MockedUser = adminUserRepo() vds := &visibilityPlaylistDS{