diff --git a/core/artwork/artwork.go b/core/artwork/artwork.go index 0b9b8e3a4..7b99d89ee 100644 --- a/core/artwork/artwork.go +++ b/core/artwork/artwork.go @@ -83,19 +83,15 @@ func (a *artwork) Get(ctx context.Context, artID model.ArtworkID, size int, squa // A vanished source must still reach the worker, or a stored hash would keep describing // artwork that no longer exists. if a.blurHashes != nil && errors.Is(err, ErrUnavailable) { - a.blurHashes.Enqueue(artID, artReader.LastUpdated(), false, true) + a.blurHashes.EnqueueGone(artID) } return nil, time.Time{}, err } - if a.blurHashes != nil { - // An original-size miss on an operational cache means a new/changed image even when no - // entity row moved. Resized misses don't qualify (their keys vary per size, and their - // readers re-fetch the original through Get, carrying the real signal). With the cache - // permanently disabled every serve reads live bytes, so original serves always force - // (the worker's unchanged-hash guard keeps that write-free); warmup forces nothing. - force := size == 0 && !square && - ((!r.Cached && a.cache.Available(ctx)) || a.cache.Disabled(ctx)) - a.blurHashes.Enqueue(artID, artReader.LastUpdated(), force, false) + if a.blurHashes != nil && size == 0 && !square && !r.Cached { + // An original-size cache fill is exactly when the served bytes change: the single recompute + // trigger. Resized fills recurse through Get(size=0); a disabled cache reports every serve as + // a fill (the worker's unchanged-hash guard keeps that write-free). + a.blurHashes.Enqueue(artID, artReader.LastUpdated()) } return r, artReader.LastUpdated(), nil } diff --git a/core/artwork/blurhash_updater.go b/core/artwork/blurhash_updater.go index 2ca56e56b..23a8799ee 100644 --- a/core/artwork/blurhash_updater.go +++ b/core/artwork/blurhash_updater.go @@ -17,22 +17,22 @@ import ( "github.com/navidrome/navidrome/resources" ) -// enqueueRequest carries the staleness signals seen at serve time: force (image-cache miss), -// sourceGone (the serve failed with ErrUnavailable) and the reader's LastUpdated, which tracks -// file mtimes that no entity row timestamp reflects. +// enqueueRequest carries the fill-time snapshot (the reader's LastUpdated, which folds row +// timestamps and live file mtimes into one clock). gone marks a serve that failed with +// ErrUnavailable: the only change no passive signal witnesses, so it clears a stale hash. type enqueueRequest struct { - force bool - sourceGone bool - imageUpdatedAt time.Time + snapshot time.Time + gone bool } -// 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. +// blurHashUpdater keeps stored blurhashes in sync with the artwork entering the image cache. +// Computation is triggered by cache fills (every change), not serves, so the worker only re-derives +// the hash and skips idempotent writes: Enqueue is a cheap dedup insert, a single worker decodes +// and persists. type blurHashUpdater struct { a *artwork mutex sync.Mutex buffer map[model.ArtworkID]enqueueRequest - noResult map[model.ArtworkID]noResultEntry wake chan struct{} done chan struct{} runDone chan struct{} @@ -41,27 +41,29 @@ type blurHashUpdater struct { 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]noResultEntry), - wake: make(chan struct{}, 1), - done: make(chan struct{}), - runDone: make(chan struct{}), + a: a, + buffer: make(map[model.ArtworkID]enqueueRequest), + wake: make(chan struct{}, 1), + done: make(chan struct{}), + runDone: make(chan struct{}), } } -func (u *blurHashUpdater) Enqueue(artID model.ArtworkID, imageUpdatedAt time.Time, force, sourceGone bool) { +// Enqueue schedules a recompute for an original-size cache fill, using the reader's snapshot as the +// artwork version. Called on the miss that fills the cache — i.e. exactly when the served bytes change. +func (u *blurHashUpdater) Enqueue(artID model.ArtworkID, snapshot time.Time) { + u.enqueue(artID, enqueueRequest{snapshot: snapshot}) +} + +// EnqueueGone schedules a clear for a serve that failed with ErrUnavailable, so a stored hash stops +// describing artwork that no longer exists (deletion is invisible to every passive signal). +func (u *blurHashUpdater) EnqueueGone(artID model.ArtworkID) { + u.enqueue(artID, enqueueRequest{gone: true}) +} + +func (u *blurHashUpdater) enqueue(artID model.ArtworkID, req enqueueRequest) { switch artID.Kind { case model.KindAlbumArtwork, model.KindArtistArtwork, model.KindPlaylistArtwork: default: @@ -80,13 +82,12 @@ func (u *blurHashUpdater) Enqueue(artID model.ArtworkID, imageUpdatedAt time.Tim u.runCancel = cancel go u.run(ctx) } - req := u.buffer[artID] - req.force = req.force || force - req.sourceGone = req.sourceGone || sourceGone - if imageUpdatedAt.After(req.imageUpdatedAt) { - req.imageUpdatedAt = imageUpdatedAt + prev := u.buffer[artID] + if req.snapshot.After(prev.snapshot) { + prev.snapshot = req.snapshot } - u.buffer[artID] = req + prev.gone = prev.gone || req.gone + u.buffer[artID] = prev u.mutex.Unlock() select { case u.wake <- struct{}{}: @@ -94,8 +95,8 @@ func (u *blurHashUpdater) Enqueue(artID model.ArtworkID, imageUpdatedAt time.Tim } } -// stop ends the worker and waits for any in-flight computation, so callers can safely tear down -// the resources (DataStore, filesystems) the worker touches. +// stop ends the worker and waits for any in-flight computation, so callers can safely tear down the +// resources (DataStore, filesystems) the worker touches. func (u *blurHashUpdater) stop() { u.mutex.Lock() if u.stopped { @@ -146,8 +147,8 @@ func (u *blurHashUpdater) next() (model.ArtworkID, enqueueRequest, bool) { return model.ArtworkID{}, enqueueRequest{}, false } -// processTimeout bounds one computation: readers can call external agents, and a hung call must -// not stall the worker forever. +// 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, req enqueueRequest) { @@ -164,76 +165,41 @@ func (u *blurHashUpdater) process(ctx context.Context, artID model.ArtworkID, re log.Trace(ctx, "BlurHash: could not load entity", "artID", artID, err) return } - // 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 - } - // A gone source only forces while a hash remains to clear; afterwards the negative cache - // applies, so artwork-less entities don't re-resolve on every placeholder serve. - force := req.force || (req.sourceGone && stored != "") - if !force { - // Current when computed from this row version or later; the snapshot may exceed the row - // version because file mtimes (which don't move rows) are folded into it on persist. - if stored != "" && storedAt != nil && !storedAt.Before(version) && !sig.After(*storedAt) { - return - } - 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 || hash == "" { - // 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) - // Reaching compute with a stored hash means there was change evidence — clear it, so the - // DTO falls back to the rotating fake instead of describing artwork no longer served. + if req.gone { + // Only the failed serve witnesses a deletion; clear a stored hash so the DTO falls back to + // the rotating fake. An empty hash means there is nothing to clear. if stored != "" { - if err := u.persist(ctx, artID, "", sig); err != nil { + if err := u.persist(ctx, artID, "", version); err != nil { log.Warn(ctx, "BlurHash: error clearing stale hash", "artID", artID, err) } } return } - // Unchanged hash with an unmoved signal needs no write — keeps forced recomputes (e.g. every - // original serve on cache-disabled installs) from hammering the DB. - if hash == stored && storedAt != nil && !sig.After(*storedAt) { + // snapshot folds row timestamps and file mtimes into one clock; a stored hash at or after it is + // already current. This is the only freshness comparison the fill trigger needs. + if stored != "" && storedAt != nil && !storedAt.Before(req.snapshot) { return } - if err := u.persist(ctx, artID, hash, sig); err != nil { + hash, err := u.computeFromArtwork(ctx, artID) + if err != nil || hash == "" { + log.Trace(ctx, "BlurHash: nothing to persist", "artID", artID, err) + // Reaching compute with a stored hash means the cover became a placeholder or vanished; + // clear it so the DTO stops describing artwork no longer served. + if stored != "" { + if err := u.persist(ctx, artID, "", req.snapshot); err != nil { + log.Warn(ctx, "BlurHash: error clearing stale hash", "artID", artID, err) + } + } + return + } + // Unchanged hash with an unmoved snapshot needs no write — keeps cache-disabled installs (which + // fill on every original serve) from hammering the DB. + if hash == stored && storedAt != nil && !req.snapshot.After(*storedAt) { + return + } + if err := u.persist(ctx, artID, hash, req.snapshot); err != nil { log.Warn(ctx, "BlurHash: error persisting", "artID", artID, err) - return } - u.clearNoResult(artID) -} - -func (u *blurHashUpdater) lastNoResult(artID model.ArtworkID) (noResultEntry, bool) { - u.mutex.Lock() - defer u.mutex.Unlock() - 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, sig time.Time) { - u.mutex.Lock() - defer u.mutex.Unlock() - if len(u.noResult) >= maxNoResultEntries { - clear(u.noResult) - } - u.noResult[artID] = noResultEntry{sig: sig, at: time.Now()} -} - -func (u *blurHashUpdater) clearNoResult(artID model.ArtworkID) { - u.mutex.Lock() - defer u.mutex.Unlock() - delete(u.noResult, artID) } func (u *blurHashUpdater) loadState(ctx context.Context, artID model.ArtworkID) (string, *time.Time, time.Time, error) { diff --git a/core/artwork/blurhash_updater_internal_test.go b/core/artwork/blurhash_updater_internal_test.go index 69c75c0c3..8d020e37b 100644 --- a/core/artwork/blurhash_updater_internal_test.go +++ b/core/artwork/blurhash_updater_internal_test.go @@ -25,31 +25,38 @@ var _ = Describe("blurHashUpdater", func() { ds = &tests.MockDataStore{} // started is pre-set so Enqueue never spawns run(): tests drive next()/process() directly. u = &blurHashUpdater{ - a: &artwork{ds: ds}, - buffer: make(map[model.ArtworkID]enqueueRequest), - noResult: make(map[model.ArtworkID]noResultEntry), - wake: make(chan struct{}, 1), - started: true, + a: &artwork{ds: ds}, + buffer: make(map[model.ArtworkID]enqueueRequest), + wake: make(chan struct{}, 1), + started: true, } }) Describe("Enqueue", func() { - It("accepts album, artist and playlist artwork and dedups, merging force and newest image time", func() { + It("accepts album, artist and playlist artwork and dedups, keeping the newest snapshot", func() { id := model.Album{ID: "al-1"}.CoverArtID() t1 := time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC) t2 := t1.Add(time.Hour) - u.Enqueue(id, t2, true, false) - u.Enqueue(id, t1, false, true) - u.Enqueue(model.Artist{ID: "ar-1"}.CoverArtID(), t1, false, false) + u.Enqueue(id, t1) + u.Enqueue(id, t2) + u.Enqueue(model.Artist{ID: "ar-1"}.CoverArtID(), t1) Expect(u.buffer).To(HaveLen(2)) - Expect(u.buffer[id].force).To(BeTrue()) - Expect(u.buffer[id].sourceGone).To(BeTrue()) - Expect(u.buffer[id].imageUpdatedAt).To(Equal(t2)) + Expect(u.buffer[id].snapshot).To(Equal(t2)) + Expect(u.buffer[id].gone).To(BeFalse()) + }) + + It("merges a gone flag onto a pending snapshot for the same artwork", func() { + id := model.Album{ID: "al-1"}.CoverArtID() + t1 := time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC) + u.Enqueue(id, t1) + u.EnqueueGone(id) + Expect(u.buffer[id].snapshot).To(Equal(t1)) + Expect(u.buffer[id].gone).To(BeTrue()) }) It("ignores other artwork kinds", func() { - u.Enqueue(model.ArtworkID{Kind: model.KindMediaFileArtwork, ID: "mf-1"}, time.Time{}, false, false) - u.Enqueue(model.ArtworkID{Kind: model.KindRadioArtwork, ID: "ra-1"}, time.Time{}, true, false) + u.Enqueue(model.ArtworkID{Kind: model.KindMediaFileArtwork, ID: "mf-1"}, time.Time{}) + u.EnqueueGone(model.ArtworkID{Kind: model.KindRadioArtwork, ID: "ra-1"}) Expect(u.buffer).To(BeEmpty()) }) }) @@ -61,77 +68,59 @@ var _ = Describe("blurHashUpdater", func() { version = time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC) }) - It("skips entities whose stored hash matches the current artwork version", func() { + It("skips entities whose stored hash is at or after the snapshot", func() { al := model.Album{ID: "al-1", UpdatedAt: version, BlurHash: "LEHV6nWB2yk8", BlurHashUpdatedAt: &version} repo := tests.CreateMockAlbumRepo() repo.SetData(model.Albums{al}) ds.MockedAlbum = repo - u.process(GinkgoT().Context(), al.CoverArtID(), enqueueRequest{imageUpdatedAt: version}) + u.process(GinkgoT().Context(), al.CoverArtID(), enqueueRequest{snapshot: 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 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(), version) - - u.process(GinkgoT().Context(), al.CoverArtID(), enqueueRequest{imageUpdatedAt: version}) - stored, _ := ds.Album(GinkgoT().Context()).Get("al-1") - Expect(stored.BlurHash).To(BeEmpty()) - }) - - It("memoizes a failed retry under the newer signal", func() { - al := model.Album{ID: "al-1", UpdatedAt: version} + It("clears a stored hash when a recompute yields no result", func() { + al := model.Album{ID: "al-1", UpdatedAt: version, BlurHash: "LEHV6nWB2yk8", BlurHashUpdatedAt: nil} repo := tests.CreateMockAlbumRepo() repo.SetData(model.Albums{al}) ds.MockedAlbum = repo ds.MockedFolder = failingFolderRepo{} - u.setNoResult(al.CoverArtID(), version) - // 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. + // A stored hash with a newer snapshot is change evidence; the compute fails, so the + // stale hash must go. 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.sig).To(Equal(newer)) - }) - - It("clears a stored hash when a recompute with change evidence yields no result", func() { - storedAt := version.Add(-time.Hour) - al := model.Album{ID: "al-1", UpdatedAt: version, BlurHash: "LEHV6nWB2yk8", BlurHashUpdatedAt: &storedAt} - repo := tests.CreateMockAlbumRepo() - repo.SetData(model.Albums{al}) - ds.MockedAlbum = repo - ds.MockedFolder = failingFolderRepo{} - - // storedAt < version = change evidence; the compute fails, so the stale hash must go. - u.process(GinkgoT().Context(), al.CoverArtID(), enqueueRequest{imageUpdatedAt: version}) + u.process(GinkgoT().Context(), al.CoverArtID(), enqueueRequest{snapshot: newer}) stored, err := ds.Album(GinkgoT().Context()).Get("al-1") Expect(err).ToNot(HaveOccurred()) Expect(stored.BlurHash).To(BeEmpty()) }) - It("clears a fresh-looking stored hash when the source is gone", func() { + It("clears a stored hash when the source is gone", func() { al := model.Album{ID: "al-1", UpdatedAt: version, BlurHash: "LEHV6nWB2yk8", BlurHashUpdatedAt: &version} repo := tests.CreateMockAlbumRepo() repo.SetData(model.Albums{al}) ds.MockedAlbum = repo - ds.MockedFolder = failingFolderRepo{} - // No row/mtime signal moved (eviction/restart window), but the serve 404ed: sourceGone - // must bypass the freshness skip so the stale hash is cleared. - u.process(GinkgoT().Context(), al.CoverArtID(), enqueueRequest{imageUpdatedAt: version, sourceGone: true}) + u.process(GinkgoT().Context(), al.CoverArtID(), enqueueRequest{gone: true}) stored, err := ds.Album(GinkgoT().Context()).Get("al-1") Expect(err).ToNot(HaveOccurred()) Expect(stored.BlurHash).To(BeEmpty()) }) + It("does nothing for a gone serve with no stored hash", func() { + al := model.Album{ID: "al-1", UpdatedAt: version} + repo := tests.CreateMockAlbumRepo() + repo.SetData(model.Albums{al}) + ds.MockedAlbum = repo + + Expect(func() { + u.process(GinkgoT().Context(), al.CoverArtID(), enqueueRequest{gone: true}) + }).ToNot(Panic()) + stored, _ := ds.Album(GinkgoT().Context()).Get("al-1") + Expect(stored.BlurHash).To(BeEmpty()) + }) + It("does nothing when the entity is gone", func() { ds.MockedAlbum = tests.CreateMockAlbumRepo() Expect(func() { diff --git a/core/artwork/e2e/blurhash_test.go b/core/artwork/e2e/blurhash_test.go index 2bd9aaf18..e92efee86 100644 --- a/core/artwork/e2e/blurhash_test.go +++ b/core/artwork/e2e/blurhash_test.go @@ -35,6 +35,39 @@ var _ = Describe("BlurHash", func() { }, "10s", "100ms").Should(Succeed()) }) + It("recomputes when the cover is swapped in place", func() { + setLayout(fstest.MapFS{ + "Artist/Album/01 - Song.mp3": trackFile(1, "Song"), + "Artist/Album/cover.png": realPNG("original-cover"), + }) + scan() + al := firstAlbum() + readArtwork(al.CoverArtID()) + var firstHash string + Eventually(func(g Gomega) { + updated, err := ds.Album(ctx).Get(al.ID) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(updated.BlurHash).ToNot(BeEmpty()) + firstHash = updated.BlurHash + }, "10s", "100ms").Should(Succeed()) + + // Swap the cover bytes and rescan: the folder's image version moves, so the reader key moves, + // the cache misses and the fill re-triggers the compute — no serve-time force hint needed. + setLayout(fstest.MapFS{ + "Artist/Album/01 - Song.mp3": trackFile(1, "Song"), + "Artist/Album/cover.png": realPNG("swapped-cover"), + }) + scan() + readArtwork(al.CoverArtID()) + + Eventually(func(g Gomega) { + updated, err := ds.Album(ctx).Get(al.ID) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(updated.BlurHash).ToNot(BeEmpty()) + g.Expect(updated.BlurHash).ToNot(Equal(firstHash)) + }, "10s", "100ms").Should(Succeed()) + }) + It("clears the stored blurhash when the cover disappears", func() { setLayout(fstest.MapFS{ "Artist/Album/01 - Song.mp3": trackFile(1, "Song"),