nik/navidrome
1
0
Fork 0
mirror of https://github.com/navidrome/navidrome.git synced 2026-08-01 07:21:17 +00:00
Code Issues Packages Projects Releases Wiki Activity

refactor(artwork): trigger blurhash recompute from the cache fill

Browse Source 
The blurhash worker previously recomputed on every artwork serve and
reconciled a row-level freshness oracle against serve-time hints (force,
sourceGone, imageUpdatedAt) to decide whether the served bytes had actually
changed. Every staleness bug found in review was one case where that weak
oracle disagreed with the true one, and each fix imported one more serve-time
fragment into it.

Move the trigger to the image-cache fill instead: an original-size cache miss
is exactly when the served bytes change, so the reader's LastUpdated snapshot
is the truth and no reconciliation is needed. Resized fills recurse through
Get(size=0) and hash the original once; a disabled cache reports every original
serve as a fill, keeping real hashes available (the worker's unchanged-hash
guard keeps that write-free).

This deletes the force/sourceGone/imageUpdatedAt flags, the double-freshness
comparison, and the entire negative cache. Only the two irreducible pieces
survive: the placeholder byte-compare and the ErrUnavailable clear-hook
(EnqueueGone), since deletion-without-rescan is invisible to every passive
signal. Adds an e2e test for in-place cover swaps, the scenario that drove the
deleted machinery, now covered structurally by the fill trigger.

Schema, DTO, repositories and the blurhash encoder package are unchanged.
This commit is contained in:
Deluan 2026-07-17 14:47:01 -04:00
parent ae36e4dfc7
commit 3759488db5
4 changed files with 143 additions and 159 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

16
core/artwork/artwork.go
View File

@ -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
}

156
core/artwork/blurhash_updater.go
View File

@ -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) {

97
core/artwork/blurhash_updater_internal_test.go
View File

@ -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() {

33
core/artwork/e2e/blurhash_test.go
View File

@ -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"),