From f9f6d36ebbeb387cc76a4d7808054616dc5eee0d Mon Sep 17 00:00:00 2001 From: Deluan Date: Fri, 17 Jul 2026 12:11:30 -0400 Subject: [PATCH] fix(artwork): TTL for no-result memoization, prompt Close, no warmup force spike - memoize every no-result outcome (ErrUnavailable can wrap transient agent and storage failures, so it is not a reliable definitive/transient discriminator) but bound it with a 1h TTL, which also un-poisons entries recorded under future file mtimes - cancel the worker context and check done in the drain loop, so Close returns promptly instead of draining the backlog at up to 30s per item - only force recompute when the image cache is operational: during warmup every serve is a miss, which caused a recompute spike at startup --- core/artwork/artwork.go | 7 +- core/artwork/blurhash_updater.go | 70 +++++++++++-------- .../artwork/blurhash_updater_internal_test.go | 19 +++-- 3 files changed, 58 insertions(+), 38 deletions(-) diff --git a/core/artwork/artwork.go b/core/artwork/artwork.go index a021e7e46..9e909068f 100644 --- a/core/artwork/artwork.go +++ b/core/artwork/artwork.go @@ -83,10 +83,9 @@ func (a *artwork) Get(ctx context.Context, artID model.ArtworkID, size int, squa return nil, time.Time{}, err } 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 miss on an operational cache means a new/changed image even when no entity row moved; + // while warming up or disabled every serve misses, so only the LastUpdated signal applies. + force := !r.Cached && a.cache.Available(ctx) a.blurHashes.Enqueue(artID, artReader.LastUpdated(), force) } return r, artReader.LastUpdated(), nil diff --git a/core/artwork/blurhash_updater.go b/core/artwork/blurhash_updater.go index 76077e0d1..e2d7c2f4b 100644 --- a/core/artwork/blurhash_updater.go +++ b/core/artwork/blurhash_updater.go @@ -2,7 +2,6 @@ package artwork import ( "context" - "errors" "fmt" "image" "sync" @@ -15,9 +14,6 @@ import ( "github.com/navidrome/navidrome/model/request" ) -// 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 { @@ -25,23 +21,35 @@ type enqueueRequest struct { imageUpdatedAt time.Time } +// blurHashUpdater keeps stored blurhashes in sync with the artwork actually served: Enqueue is a +// cheap dedup insert, and a single worker re-checks freshness before decoding. type blurHashUpdater struct { - a *artwork - mutex sync.Mutex - buffer map[model.ArtworkID]enqueueRequest - noResult map[model.ArtworkID]time.Time - wake chan struct{} - done chan struct{} - runDone chan struct{} - started bool - stopped bool + a *artwork + mutex sync.Mutex + buffer map[model.ArtworkID]enqueueRequest + noResult map[model.ArtworkID]noResultEntry + wake chan struct{} + done chan struct{} + runDone chan struct{} + runCancel context.CancelFunc + started bool + stopped bool +} + +// noResultTTL bounds how long a failed or empty computation suppresses retries, so transient +// outages (agents, storage) self-heal despite being indistinguishable from "no artwork". +const noResultTTL = time.Hour + +type noResultEntry struct { + sig time.Time + at time.Time } func newBlurHashUpdater(a *artwork) *blurHashUpdater { return &blurHashUpdater{ a: a, buffer: make(map[model.ArtworkID]enqueueRequest), - noResult: make(map[model.ArtworkID]time.Time), + noResult: make(map[model.ArtworkID]noResultEntry), wake: make(chan struct{}, 1), done: make(chan struct{}), runDone: make(chan struct{}), @@ -61,9 +69,11 @@ func (u *blurHashUpdater) Enqueue(artID model.ArtworkID, imageUpdatedAt time.Tim } if !u.started { u.started = true - // Playlist artwork readers require a user in the context. Lazy-starting keeps idle Artwork + // Admin context: playlist artwork readers require a user. Lazy-starting keeps idle Artwork // instances goroutine-free; stop() ends the worker (tests must call it, the server never does). - go u.run(request.WithUser(context.Background(), model.User{IsAdmin: true})) + ctx, cancel := context.WithCancel(request.WithUser(context.Background(), model.User{IsAdmin: true})) + u.runCancel = cancel + go u.run(ctx) } req := u.buffer[artID] req.force = req.force || force @@ -88,9 +98,11 @@ func (u *blurHashUpdater) stop() { } u.stopped = true started := u.started + cancel := u.runCancel u.mutex.Unlock() close(u.done) if started { + cancel() <-u.runDone } } @@ -104,6 +116,11 @@ func (u *blurHashUpdater) run(ctx context.Context) { case <-u.wake: } for { + select { + case <-u.done: + return + default: + } artID, req, ok := u.next() if !ok { break @@ -153,19 +170,14 @@ func (u *blurHashUpdater) process(ctx context.Context, artID model.ArtworkID, re if stored != "" && storedAt != nil && !storedAt.Before(version) && !sig.After(*storedAt) { return } - if last, ok := u.lastNoResult(artID); ok && !sig.After(last) { + if last, ok := u.lastNoResult(artID); ok && !sig.After(last.sig) && time.Since(last.at) < noResultTTL { 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. + // Any no-result (no artwork, placeholder, decode failure, transient outage) is memoized + // with a TTL: browsing stays cheap, and failures still retry once it expires. log.Trace(ctx, "BlurHash: nothing to persist", "artID", artID, err) u.setNoResult(artID, sig) return @@ -177,24 +189,24 @@ func (u *blurHashUpdater) process(ctx context.Context, artID model.ArtworkID, re u.clearNoResult(artID) } -func (u *blurHashUpdater) lastNoResult(artID model.ArtworkID) (time.Time, bool) { +func (u *blurHashUpdater) lastNoResult(artID model.ArtworkID) (noResultEntry, bool) { u.mutex.Lock() defer u.mutex.Unlock() - t, ok := u.noResult[artID] - return t, ok + e, ok := u.noResult[artID] + return e, ok } // maxNoResultEntries bounds the negative cache; entries only accumulate for artwork-less entities, // so a wholesale reset just costs those entities one extra verification pass each. const maxNoResultEntries = 25_000 -func (u *blurHashUpdater) setNoResult(artID model.ArtworkID, version time.Time) { +func (u *blurHashUpdater) setNoResult(artID model.ArtworkID, sig time.Time) { u.mutex.Lock() defer u.mutex.Unlock() if len(u.noResult) >= maxNoResultEntries { clear(u.noResult) } - u.noResult[artID] = version + u.noResult[artID] = noResultEntry{sig: sig, at: time.Now()} } func (u *blurHashUpdater) clearNoResult(artID model.ArtworkID) { diff --git a/core/artwork/blurhash_updater_internal_test.go b/core/artwork/blurhash_updater_internal_test.go index e996e2abe..8120649d5 100644 --- a/core/artwork/blurhash_updater_internal_test.go +++ b/core/artwork/blurhash_updater_internal_test.go @@ -1,6 +1,7 @@ package artwork import ( + "errors" "time" "github.com/navidrome/navidrome/model" @@ -9,6 +10,13 @@ import ( . "github.com/onsi/gomega" ) +// failingFolderRepo makes the artwork reader chain fail with a clean (transient-style) error. +type failingFolderRepo struct{ model.FolderRepository } + +func (failingFolderRepo) GetAll(...model.QueryOptions) ([]model.Folder, error) { + return nil, errors.New("boom") +} + var _ = Describe("blurHashUpdater", func() { var u *blurHashUpdater var ds *tests.MockDataStore @@ -19,7 +27,7 @@ var _ = Describe("blurHashUpdater", func() { u = &blurHashUpdater{ a: &artwork{ds: ds}, buffer: make(map[model.ArtworkID]enqueueRequest), - noResult: make(map[model.ArtworkID]time.Time), + noResult: make(map[model.ArtworkID]noResultEntry), wake: make(chan struct{}, 1), started: true, } @@ -76,20 +84,21 @@ var _ = Describe("blurHashUpdater", func() { Expect(stored.BlurHash).To(BeEmpty()) }) - It("does not refresh the no-result entry when a retry fails transiently", func() { + It("memoizes a failed retry under the newer signal", func() { al := model.Album{ID: "al-1", UpdatedAt: version} repo := tests.CreateMockAlbumRepo() repo.SetData(model.Albums{al}) ds.MockedAlbum = repo + ds.MockedFolder = failingFolderRepo{} 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. + // A newer image mtime bypasses the no-result skip; the compute fails cleanly here, so + // the entry is refreshed under the newer signal, with the TTL as the retry bound. 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)) + Expect(last.sig).To(Equal(newer)) }) It("does nothing when the entity is gone", func() {