mirror of
https://github.com/navidrome/navidrome.git
synced 2026-08-01 07:21:17 +00:00
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.
This commit is contained in:
parent
67f6d8aee8
commit
6afcb93a9b
@ -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
|
||||
}
|
||||
|
||||
|
||||
@ -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",
|
||||
|
||||
@ -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.
|
||||
|
||||
@ -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{
|
||||
|
||||
@ -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)
|
||||
|
||||
@ -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",
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user