From 696399dab774d3b936be5b683a2544c446a96e9d Mon Sep 17 00:00:00 2001 From: Deluan Date: Fri, 17 Jul 2026 11:20:41 -0400 Subject: [PATCH] fix(artwork): don't memoize transient blurhash failures; track image freshness with cache disabled Only ErrUnavailable (definitively no artwork) is negative-cached; timeouts and storage/agent hiccups retry on a later serve. Enqueue now carries the reader's LastUpdated so file swaps are detected even when the image cache is disabled (where every serve reads the source and miss-forcing is off). --- core/artwork/artwork.go | 3 +- core/artwork/blurhash/blurhash_bench_test.go | 41 +++++++++++++ core/artwork/blurhash_updater.go | 59 +++++++++++++------ .../artwork/blurhash_updater_internal_test.go | 47 +++++++++++---- 4 files changed, 118 insertions(+), 32 deletions(-) create mode 100644 core/artwork/blurhash/blurhash_bench_test.go diff --git a/core/artwork/artwork.go b/core/artwork/artwork.go index ee1a6807b..3b403ae6c 100644 --- a/core/artwork/artwork.go +++ b/core/artwork/artwork.go @@ -76,8 +76,9 @@ func (a *artwork) Get(ctx context.Context, artID model.ArtworkID, size int, squa if a.blurHashes != nil { // A cache miss means the image is new or changed, even when no entity row moved (e.g. an // in-place cover.jpg swap) — force a recompute so the stored blurhash follows the image. + // The reader's LastUpdated covers the same case when the image cache is disabled. force := !r.Cached && !a.cache.Disabled(ctx) - a.blurHashes.Enqueue(artID, force) + a.blurHashes.Enqueue(artID, artReader.LastUpdated(), force) } return r, artReader.LastUpdated(), nil } diff --git a/core/artwork/blurhash/blurhash_bench_test.go b/core/artwork/blurhash/blurhash_bench_test.go new file mode 100644 index 000000000..43b593eb5 --- /dev/null +++ b/core/artwork/blurhash/blurhash_bench_test.go @@ -0,0 +1,41 @@ +package blurhash_test + +import ( + "fmt" + "image" + "image/color" + "testing" + + "github.com/navidrome/navidrome/core/artwork/blurhash" +) + +// benchImage builds a deterministic gradient so runs are comparable across revisions. +func benchImage(size int) image.Image { + img := image.NewNRGBA(image.Rect(0, 0, size, size)) + for y := 0; y < size; y++ { + for x := 0; x < size; x++ { + img.SetNRGBA(x, y, color.NRGBA{ + R: uint8(255 * x / size), + G: uint8(255 * y / size), + B: uint8((x + y) * 255 / (2 * size)), + A: 255, + }) + } + } + return img +} + +func BenchmarkEncode(b *testing.B) { + for _, size := range []int{100, 300, 600, 900, 1200, 1500} { + img := benchImage(size) + x, y := blurhash.Components(size, size) + b.Run(fmt.Sprintf("%dx%d", size, size), func(b *testing.B) { + b.ReportAllocs() + for range b.N { + if _, err := blurhash.Encode(img, x, y); err != nil { + b.Fatal(err) + } + } + }) + } +} diff --git a/core/artwork/blurhash_updater.go b/core/artwork/blurhash_updater.go index 9f93b628d..735256baa 100644 --- a/core/artwork/blurhash_updater.go +++ b/core/artwork/blurhash_updater.go @@ -2,6 +2,7 @@ package artwork import ( "context" + "errors" "fmt" "image" "sync" @@ -17,10 +18,17 @@ import ( // blurHashUpdater keeps stored blurhashes in sync with the artwork actually served. Enqueue is // cheap (dedup map insert); the worker re-checks freshness against the DB and only decodes when // the hash is missing or was computed from an older artwork version. +// enqueueRequest carries the staleness signals seen at serve time: force (image-cache miss) and +// the reader's LastUpdated, which tracks file mtimes that no entity row timestamp reflects. +type enqueueRequest struct { + force bool + imageUpdatedAt time.Time +} + type blurHashUpdater struct { a *artwork mutex sync.Mutex - buffer map[model.ArtworkID]bool // value: force recompute (image-cache miss) + buffer map[model.ArtworkID]enqueueRequest noResult map[model.ArtworkID]time.Time wake chan struct{} start sync.Once @@ -29,13 +37,13 @@ type blurHashUpdater struct { func newBlurHashUpdater(a *artwork) *blurHashUpdater { return &blurHashUpdater{ a: a, - buffer: make(map[model.ArtworkID]bool), + buffer: make(map[model.ArtworkID]enqueueRequest), noResult: make(map[model.ArtworkID]time.Time), wake: make(chan struct{}, 1), } } -func (u *blurHashUpdater) Enqueue(artID model.ArtworkID, force bool) { +func (u *blurHashUpdater) Enqueue(artID model.ArtworkID, imageUpdatedAt time.Time, force bool) { switch artID.Kind { case model.KindAlbumArtwork, model.KindArtistArtwork, model.KindPlaylistArtwork: default: @@ -47,7 +55,12 @@ func (u *blurHashUpdater) Enqueue(artID model.ArtworkID, force bool) { go u.run(request.WithUser(context.Background(), model.User{IsAdmin: true})) }) u.mutex.Lock() - u.buffer[artID] = u.buffer[artID] || force + req := u.buffer[artID] + req.force = req.force || force + if imageUpdatedAt.After(req.imageUpdatedAt) { + req.imageUpdatedAt = imageUpdatedAt + } + u.buffer[artID] = req u.mutex.Unlock() select { case u.wake <- struct{}{}: @@ -58,30 +71,30 @@ func (u *blurHashUpdater) Enqueue(artID model.ArtworkID, force bool) { func (u *blurHashUpdater) run(ctx context.Context) { for range u.wake { for { - artID, force, ok := u.next() + artID, req, ok := u.next() if !ok { break } - u.process(ctx, artID, force) + u.process(ctx, artID, req) } } } -func (u *blurHashUpdater) next() (model.ArtworkID, bool, bool) { +func (u *blurHashUpdater) next() (model.ArtworkID, enqueueRequest, bool) { u.mutex.Lock() defer u.mutex.Unlock() - for artID, force := range u.buffer { + for artID, req := range u.buffer { delete(u.buffer, artID) - return artID, force, true + return artID, req, true } - return model.ArtworkID{}, false, false + return model.ArtworkID{}, enqueueRequest{}, false } // processTimeout bounds one computation: readers can call external agents, and a hung call must // not stall the worker forever. const processTimeout = 30 * time.Second -func (u *blurHashUpdater) process(ctx context.Context, artID model.ArtworkID, force bool) { +func (u *blurHashUpdater) process(ctx context.Context, artID model.ArtworkID, req enqueueRequest) { // Artwork readers can touch storage, agents and plugins; a panic here must not kill the server. defer func() { if r := recover(); r != nil { @@ -95,21 +108,31 @@ func (u *blurHashUpdater) process(ctx context.Context, artID model.ArtworkID, fo log.Trace(ctx, "BlurHash: could not load entity", "artID", artID, err) return } - if !force { - if stored != "" && storedAt != nil && storedAt.Equal(version) { + // sig is the newest staleness signal: the entity's artwork version or the served image's own + // timestamp, whichever is later (file swaps move the latter without touching any row). + sig := version + if req.imageUpdatedAt.After(sig) { + sig = req.imageUpdatedAt + } + if !req.force { + if stored != "" && storedAt != nil && storedAt.Equal(version) && !sig.After(*storedAt) { return } - if last, ok := u.lastNoResult(artID); ok && last.Equal(version) { + if last, ok := u.lastNoResult(artID); ok && !sig.After(last) { return } } hash, err := u.computeFromArtwork(ctx, artID) + if err != nil && !errors.Is(err, ErrUnavailable) { + // Transient failure (timeout, storage/agent hiccup) — leave un-memoized so a later serve retries. + log.Trace(ctx, "BlurHash: compute failed", "artID", artID, err) + return + } if err != nil || hash == "" { + // Definitively no artwork (or a placeholder) for this state — remember it, so browsing + // artwork-less entities doesn't re-resolve them on every serve. log.Trace(ctx, "BlurHash: nothing to persist", "artID", artID, err) - // A timed-out/cancelled attempt is transient — don't suppress future retries for it. - if ctx.Err() == nil { - u.setNoResult(artID, version) - } + u.setNoResult(artID, sig) return } if err := u.persist(ctx, artID, hash, version); err != nil { diff --git a/core/artwork/blurhash_updater_internal_test.go b/core/artwork/blurhash_updater_internal_test.go index 70022bd51..ec099eb91 100644 --- a/core/artwork/blurhash_updater_internal_test.go +++ b/core/artwork/blurhash_updater_internal_test.go @@ -18,25 +18,28 @@ var _ = Describe("blurHashUpdater", func() { // No run() goroutine: tests drive next()/process() directly. u = &blurHashUpdater{ a: &artwork{ds: ds}, - buffer: make(map[model.ArtworkID]bool), + buffer: make(map[model.ArtworkID]enqueueRequest), noResult: make(map[model.ArtworkID]time.Time), wake: make(chan struct{}, 1), } }) Describe("Enqueue", func() { - It("accepts album, artist and playlist artwork and dedups, keeping the force flag sticky", func() { + It("accepts album, artist and playlist artwork and dedups, merging force and newest image time", func() { id := model.Album{ID: "al-1"}.CoverArtID() - u.Enqueue(id, true) - u.Enqueue(id, false) - u.Enqueue(model.Artist{ID: "ar-1"}.CoverArtID(), false) + t1 := time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC) + t2 := t1.Add(time.Hour) + u.Enqueue(id, t2, true) + u.Enqueue(id, t1, false) + u.Enqueue(model.Artist{ID: "ar-1"}.CoverArtID(), t1, false) Expect(u.buffer).To(HaveLen(2)) - Expect(u.buffer[id]).To(BeTrue()) + Expect(u.buffer[id].force).To(BeTrue()) + Expect(u.buffer[id].imageUpdatedAt).To(Equal(t2)) }) It("ignores other artwork kinds", func() { - u.Enqueue(model.ArtworkID{Kind: model.KindMediaFileArtwork, ID: "mf-1"}, false) - u.Enqueue(model.ArtworkID{Kind: model.KindRadioArtwork, ID: "ra-1"}, true) + u.Enqueue(model.ArtworkID{Kind: model.KindMediaFileArtwork, ID: "mf-1"}, time.Time{}, false) + u.Enqueue(model.ArtworkID{Kind: model.KindRadioArtwork, ID: "ra-1"}, time.Time{}, true) Expect(u.buffer).To(BeEmpty()) }) }) @@ -54,27 +57,45 @@ var _ = Describe("blurHashUpdater", func() { repo.SetData(model.Albums{al}) ds.MockedAlbum = repo - u.process(GinkgoT().Context(), al.CoverArtID(), false) + u.process(GinkgoT().Context(), al.CoverArtID(), enqueueRequest{imageUpdatedAt: version}) stored, err := ds.Album(GinkgoT().Context()).Get("al-1") Expect(err).ToNot(HaveOccurred()) Expect(stored.BlurHash).To(Equal("LEHV6nWB2yk8")) }) - It("skips entities that previously yielded no result for the same artwork version", func() { + It("skips entities that previously yielded no result for the same signals", func() { al := model.Album{ID: "al-1", UpdatedAt: version} repo := tests.CreateMockAlbumRepo() repo.SetData(model.Albums{al}) ds.MockedAlbum = repo - u.setNoResult(al.CoverArtID(), al.ArtworkUpdatedAt()) + u.setNoResult(al.CoverArtID(), version) - u.process(GinkgoT().Context(), al.CoverArtID(), false) + u.process(GinkgoT().Context(), al.CoverArtID(), enqueueRequest{imageUpdatedAt: version}) stored, _ := ds.Album(GinkgoT().Context()).Get("al-1") Expect(stored.BlurHash).To(BeEmpty()) }) + It("does not refresh the no-result entry when a retry fails transiently", func() { + al := model.Album{ID: "al-1", UpdatedAt: version} + repo := tests.CreateMockAlbumRepo() + repo.SetData(model.Albums{al}) + ds.MockedAlbum = repo + u.setNoResult(al.CoverArtID(), version) + + // A newer image mtime must bypass the no-result skip and attempt a compute; the compute + // fails transiently here (no readers wired), so the no-result entry must NOT be refreshed. + newer := version.Add(time.Hour) + u.process(GinkgoT().Context(), al.CoverArtID(), enqueueRequest{imageUpdatedAt: newer}) + last, ok := u.lastNoResult(al.CoverArtID()) + Expect(ok).To(BeTrue()) + Expect(last).To(Equal(version)) + }) + It("does nothing when the entity is gone", func() { ds.MockedAlbum = tests.CreateMockAlbumRepo() - Expect(func() { u.process(GinkgoT().Context(), model.Album{ID: "missing"}.CoverArtID(), false) }).ToNot(Panic()) + Expect(func() { + u.process(GinkgoT().Context(), model.Album{ID: "missing"}.CoverArtID(), enqueueRequest{}) + }).ToNot(Panic()) }) }) })