From 6afcb93a9b84889ea08f4c27f3948e395c765e70 Mon Sep 17 00:00:00 2001 From: Deluan Date: Wed, 22 Jul 2026 12:39:15 -0400 Subject: [PATCH] fix(artwork): retry higher-priority external art after fallback hit With CoverArtPriority="external,cover.jpg", a transient external failure followed by a folder hit dropped the external error: the worker recorded found and deleted the queue row, so the configured higher-priority external art was never retried. Carry extError onto the fallback resolution and add an outcomeFoundStale that persists+serves the art but reschedules via MarkFailed, giving the external source another chance. When external later answers definitively-not-found, the hit is not stale and the row is deleted. --- core/artwork/processor.go | 6 +++++ core/artwork/processor_test.go | 22 +++++++++++++++++++ core/artwork/resolve.go | 11 ++++++++-- core/artwork/resolve_test.go | 40 ++++++++++++++++++++++++++++++++++ core/artwork/worker.go | 2 +- core/artwork/worker_test.go | 28 ++++++++++++++++++++++++ 6 files changed, 106 insertions(+), 3 deletions(-) diff --git a/core/artwork/processor.go b/core/artwork/processor.go index 262d06518..73a522daf 100644 --- a/core/artwork/processor.go +++ b/core/artwork/processor.go @@ -24,6 +24,9 @@ type outcome int const ( outcomeFound outcome = iota + // outcomeFoundStale: state was written and is served, but a higher-priority external + // step failed, so the row must retry (via MarkFailed) to give that source another chance. + outcomeFoundStale outcomeAbsent outcomeFailed ) @@ -116,6 +119,9 @@ func processItem(ctx context.Context, deps *workerDeps, item model.ArtworkQueueI log.Warn(ctx, "artwork: failed to persist item artwork state", "kind", item.ItemKind, "id", item.ItemID, err) return outcomeFailed } + if res.extError { + return outcomeFoundStale + } return outcomeFound } diff --git a/core/artwork/processor_test.go b/core/artwork/processor_test.go index 64b0181bd..876124bc3 100644 --- a/core/artwork/processor_test.go +++ b/core/artwork/processor_test.go @@ -136,6 +136,28 @@ var _ = Describe("processItem", func() { Expect(err).To(MatchError(model.ErrNotFound)) }) + It("found-stale: a fallback hit after a transient external failure persists state and returns outcomeFoundStale", func() { + conf.Server.CoverArtPriority = "external, cover.jpg" + folderRepo.result = []model.Folder{{ + Path: "tests/fixtures/artist/an-album", + ImageFiles: []string{"cover.jpg"}, + }} + ds.MockedAlbum.(*tests.MockAlbumRepo).SetData(model.Albums{ + {ID: "alstale", Name: "Album", FolderIDs: []string{"f1"}}, + }) + prov.albumImage = func(context.Context, string) (*url.URL, error) { + return nil, errors.New("agent timed out") + } + + out := processItem(ctx, deps, model.ArtworkQueueItem{ItemKind: "al", ItemID: "alstale"}) + Expect(out).To(Equal(outcomeFoundStale)) + + ia, err := artRepo.GetItemArtwork("al", "alstale", model.ImageTypePrimary) + Expect(err).ToNot(HaveOccurred()) + Expect(ia.Hash).ToNot(BeEmpty()) + Expect(ia.Source).To(Equal("folder")) + }) + It("dedup: a second item with identical bytes skips decode and reuses the artwork row", func() { folderRepo.result = []model.Folder{{ Path: "tests/fixtures/artist/an-album", diff --git a/core/artwork/resolve.go b/core/artwork/resolve.go index 7bc5115d0..bb2a3f01b 100644 --- a/core/artwork/resolve.go +++ b/core/artwork/resolve.go @@ -27,7 +27,9 @@ type resolution struct { source string // model.ItemArtwork.Source value: "folder", "embedded", "external", "upload", "generated" sourcePath string // backing library/upload file (folder/upload: the image; embedded: the audio file); "" otherwise refMtime int64 // mtime of sourcePath at resolution time; 0 when no sourcePath - extError bool // an external source errored/timed out (forces failed, never absent) + // external source errored/timed out. With no reader: forces failed (never absent). + // On a hit: a higher-priority external step failed—serve this, but retry later. + extError bool } // extGateFunc is an alias for the external-step wrapper the worker injects (rate @@ -79,6 +81,7 @@ func resolveAlbum(ctx context.Context, ds model.DataStore, prov external.Provide switch { case pattern == "embedded": if res, ok := resolveEmbedded(ctx, lib, ffm, al.EmbedArtPath); ok { + res.extError = extErr return res, nil } case pattern == "external": @@ -89,6 +92,7 @@ func resolveAlbum(ctx context.Context, ds model.DataStore, prov external.Provide } case len(imgFiles) > 0: if res, ok := resolveFolderFile(ctx, lib, imgFiles, pattern); ok { + res.extError = extErr return res, nil } } @@ -145,6 +149,7 @@ func resolveArtist(ctx context.Context, ds model.DataStore, prov external.Provid } case pattern == "image-folder": if res, ok := resolveArtistImageFolder(ar); ok { + res.extError = extErr return res, nil } case strings.HasPrefix(pattern, "album/"): @@ -152,6 +157,7 @@ func resolveArtist(ctx context.Context, ds model.DataStore, prov external.Provid continue } if res, ok := resolveFolderFile(ctx, lib, imgFiles, strings.TrimPrefix(pattern, "album/")); ok { + res.extError = extErr return res, nil } default: @@ -159,6 +165,7 @@ func resolveArtist(ctx context.Context, ds model.DataStore, prov external.Provid continue } if res, ok := resolveArtistFolderPattern(ctx, lib, artistFolder, pattern); ok { + res.extError = extErr return res, nil } } @@ -236,7 +243,7 @@ func resolvePlaylist(ctx context.Context, ds model.DataStore, prov external.Prov if err != nil { return resolution{extError: extErr}, nil //nolint:nilerr // encode failure is a soft "no image", not a resolveItem error } - return resolution{reader: r, source: "generated"}, nil + return resolution{reader: r, source: "generated", extError: extErr}, nil } // resolveRadio ports reader_radio.go: only an uploaded image, no fallback. diff --git a/core/artwork/resolve_test.go b/core/artwork/resolve_test.go index a80d02ca9..21b742345 100644 --- a/core/artwork/resolve_test.go +++ b/core/artwork/resolve_test.go @@ -148,6 +148,46 @@ var _ = Describe("resolveItem", func() { Expect(res.extError).To(BeFalse()) }) + It("carries extError onto a fallback folder hit after a transient external failure", func() { + conf.Server.CoverArtPriority = "external, cover.jpg" + folderRepo.result = []model.Folder{{ + Path: "tests/fixtures/artist/an-album", + ImageFiles: []string{"cover.jpg"}, + }} + ds.MockedAlbum.(*tests.MockAlbumRepo).SetData(model.Albums{ + {ID: "al6", Name: "Album", FolderIDs: []string{"f1"}}, + }) + prov.albumImage = func(context.Context, string) (*url.URL, error) { + return nil, errors.New("agent timed out") + } + + res, err := resolveItem(ctx, ds, prov, ffm, model.ArtworkQueueItem{ItemKind: "al", ItemID: "al6"}, nil) + Expect(err).ToNot(HaveOccurred()) + Expect(res.reader).ToNot(BeNil()) + defer res.reader.Close() + Expect(res.source).To(Equal("folder")) + Expect(res.extError).To(BeTrue()) + }) + + It("does not carry extError onto a fallback folder hit after a definitive external not-found", func() { + conf.Server.CoverArtPriority = "external, cover.jpg" + folderRepo.result = []model.Folder{{ + Path: "tests/fixtures/artist/an-album", + ImageFiles: []string{"cover.jpg"}, + }} + ds.MockedAlbum.(*tests.MockAlbumRepo).SetData(model.Albums{ + {ID: "al7", Name: "Album", FolderIDs: []string{"f1"}}, + }) + // prov.albumImage left nil -> fakeExternalProvider returns model.ErrNotFound + + res, err := resolveItem(ctx, ds, prov, ffm, model.ArtworkQueueItem{ItemKind: "al", ItemID: "al7"}, nil) + Expect(err).ToNot(HaveOccurred()) + Expect(res.reader).ToNot(BeNil()) + defer res.reader.Close() + Expect(res.source).To(Equal("folder")) + Expect(res.extError).To(BeFalse()) + }) + It("routes the external step through a custom extGate", func() { conf.Server.CoverArtPriority = "external" ds.MockedAlbum.(*tests.MockAlbumRepo).SetData(model.Albums{ diff --git a/core/artwork/worker.go b/core/artwork/worker.go index fb20a2bbc..bc3e47af0 100644 --- a/core/artwork/worker.go +++ b/core/artwork/worker.go @@ -154,7 +154,7 @@ func (w *Worker) process(ctx context.Context, item model.ArtworkQueueItem) { if err := queue.DeleteIfUnchanged(item.ItemKind, item.ItemID, item.ImageType, item.RetryAt); err != nil { log.Warn(ctx, "artwork: could not delete processed queue item", "kind", item.ItemKind, "id", item.ItemID, err) } - case outcomeFailed: + case outcomeFoundStale, outcomeFailed: retryAt := time.Now().Add(backoff(item.Attempts)) if err := queue.MarkFailed(item.ItemKind, item.ItemID, item.ImageType, retryAt); err != nil { log.Warn(ctx, "artwork: could not reschedule failed queue item", "kind", item.ItemKind, "id", item.ItemID, err) diff --git a/core/artwork/worker_test.go b/core/artwork/worker_test.go index 058d707b8..7865b6cfc 100644 --- a/core/artwork/worker_test.go +++ b/core/artwork/worker_test.go @@ -135,6 +135,34 @@ var _ = Describe("Worker", func() { Expect(err).To(MatchError(model.ErrNotFound), "a timeout must never settle on absent") }) + It("reschedules a found-stale item via MarkFailed while keeping its served state", func() { + conf.Server.CoverArtPriority = "external, cover.jpg" + folderRepo.result = []model.Folder{{ + Path: "tests/fixtures/artist/an-album", + ImageFiles: []string{"cover.jpg"}, + }} + ds.MockedAlbum.(*tests.MockAlbumRepo).SetData(model.Albums{ + {ID: "alstale", Name: "Album", FolderIDs: []string{"f1"}}, + }) + prov.albumImage = func(context.Context, string) (*url.URL, error) { + return nil, errors.New("agent timed out") + } + Expect(queueRepo.Enqueue(model.ArtworkQueueItem{ItemKind: "al", ItemID: "alstale"})).To(Succeed()) + + n, err := w.drain(ctx, 2) + Expect(err).ToNot(HaveOccurred()) + Expect(n).To(Equal(1)) + + it := findQueued(queueRepo, "al", "alstale") + Expect(it).ToNot(BeNil(), "a found-stale row must survive for a higher-priority retry") + Expect(it.Attempts).To(Equal(1)) + Expect(it.RetryAt).To(BeTemporally(">", time.Now())) + + ia, err := artRepo.GetItemArtwork("al", "alstale", model.ImageTypePrimary) + Expect(err).ToNot(HaveOccurred()) + Expect(ia.Source).To(Equal("folder"), "the fallback art is served meanwhile") + }) + It("keeps a row re-enqueued between dequeue and delete", func() { folderRepo.result = []model.Folder{{ Path: "tests/fixtures/artist/an-album",