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

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

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)
+				}
+			}
+		})
+	}
+}

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 {

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())
 		})
 	})
 })