perf(artwork): key blurhash dedup by identity, plus review cleanups

The seen map was keyed by the full ArtworkID, which embeds the client token's
LastUpdate: every scan bump rotated the key, so the same-bytes dedup (and its
cheap version re-persist path) never fired in production and stale entries
accumulated forever. The key now zeroes LastUpdate, making the map bounded by
entity count and restoring the tested dedup behavior.

Cleanups from the same review: the base83 encoder is now exported from the
blurhash package and the DTO's duplicate copy is deleted; the always-set
blurHashes field lost its test-shaped nil guards; clearIfStored dropped its
inert version parameter (cleared rows never have their timestamp read); the
teeReader done flag is replaced by nilling the callback; worker-era comments
(async, cache-miss recompute) were updated to the inline design; and the e2e
specs assert directly instead of polling, since the hash is persisted before
the read helpers return.

core/artwork/artwork.go

@@ -38,9 +38,7 @@ type Artwork interface {
 }
 
 func NewArtwork(ds model.DataStore, cache cache.FileCache, ffmpeg ffmpeg.FFmpeg, provider external.Provider) Artwork {
-	a := &artwork{ds: ds, cache: cache, ffmpeg: ffmpeg, provider: provider}
-	a.blurHashes = newBlurHashUpdater(ds)
-	return a
+	return &artwork{ds: ds, cache: cache, ffmpeg: ffmpeg, provider: provider, blurHashes: newBlurHashUpdater(ds)}
 }
 
 type artwork struct {
@@ -63,11 +61,9 @@ func (a *artwork) GetOrPlaceholder(ctx context.Context, id string, size int, squ
 		reader, lastUpdate, err = a.Get(ctx, artID, size, square)
 	}
 	if errors.Is(err, ErrUnavailable) {
-		if a.blurHashes != nil {
-			// The client is receiving the placeholder, so a stored hash describing the old cover must
-			// clear — hash-what-you-serve applies to the fallback too.
-			a.blurHashes.clearIfStored(ctx, artID, time.Now())
-		}
+		// The client is receiving the placeholder, so a stored hash describing the old cover must
+		// clear — hash-what-you-serve applies to the fallback too.
+		a.blurHashes.clearIfStored(ctx, artID)
 		if artID.Kind == model.KindArtistArtwork {
 			reader, _ = resources.FS().Open(consts.PlaceholderArtistArt)
 		} else {
@@ -92,7 +88,7 @@ func (a *artwork) Get(ctx context.Context, artID model.ArtworkID, size int, squa
 		return nil, time.Time{}, err
 	}
 	reader = r
-	if a.blurHashes != nil && size == 0 && !square && eligibleKind(artID) {
+	if size == 0 && !square && eligibleKind(artID) {
 		// Tee the served bytes: the blurhash is computed from exactly what the client downloads, so it
 		// changes precisely when the served cover changes. Placeholder bytes (playlist fallback) clear.
 		// The tee wraps r directly, so Close reaches the underlying stream (no fd leak).

core/artwork/blurhash/blurhash.go

@@ -85,7 +85,7 @@ func Encode(img image.Image, xComp, yComp int) (string, error) {
 	}
 
 	var sb strings.Builder
-	sb.WriteString(encode83((xComp-1)+(yComp-1)*9, 1))
+	sb.WriteString(Encode83((xComp-1)+(yComp-1)*9, 1))
 
 	ac := factors[1:]
 	maxVal := 1.0
@@ -96,15 +96,15 @@ func Encode(img image.Image, xComp, yComp int) (string, error) {
 		}
 		quantMax := int(math.Max(0, math.Min(82, math.Floor(actualMax*166-0.5))))
 		maxVal = float64(quantMax+1) / 166
-		sb.WriteString(encode83(quantMax, 1))
+		sb.WriteString(Encode83(quantMax, 1))
 	} else {
-		sb.WriteString(encode83(0, 1))
+		sb.WriteString(Encode83(0, 1))
 	}
 
 	dc := factors[0]
-	sb.WriteString(encode83(linearToSRGB(dc[0])<<16|linearToSRGB(dc[1])<<8|linearToSRGB(dc[2]), 4))
+	sb.WriteString(Encode83(linearToSRGB(dc[0])<<16|linearToSRGB(dc[1])<<8|linearToSRGB(dc[2]), 4))
 	for _, f := range ac {
-		sb.WriteString(encode83(quantAC(f[0], maxVal)*19*19+quantAC(f[1], maxVal)*19+quantAC(f[2], maxVal), 2))
+		sb.WriteString(Encode83(quantAC(f[0], maxVal)*19*19+quantAC(f[1], maxVal)*19+quantAC(f[2], maxVal), 2))
 	}
 	return sb.String(), nil
 }
@@ -165,7 +165,9 @@ func linearToSRGB(v float64) int {
 	return int((1.055*math.Pow(v, 1/2.4)-0.055)*255 + 0.5)
 }
 
-func encode83(value, length int) string {
+// Encode83 encodes value as a fixed-width, big-endian base83 string of the given length, using the
+// blurhash spec's alphabet.
+func Encode83(value, length int) string {
 	b := make([]byte, length)
 	for i := length - 1; i >= 0; i-- {
 		b[i] = alphabet[value%83]

core/artwork/blurhash_updater.go

@@ -55,10 +55,13 @@ func (u *blurHashUpdater) update(ctx context.Context, artID model.ArtworkID, dat
 			log.Error(ctx, "BlurHash: recovered from panic", "artID", artID, "panic", r)
 		}
 	}()
+	// ArtworkID embeds the client token's LastUpdate; without zeroing it the seen key would rotate on
+	// every scan bump, defeating the same-bytes dedup and stranding stale entries forever.
+	artID.LastUpdate = time.Time{}
 	// The response is already written when the tee fires; a client abort must not lose the write.
 	ctx = context.WithoutCancel(ctx)
 	if isPlaceholder(data) {
-		u.clearIfStored(ctx, artID, version)
+		u.clearIfStored(ctx, artID)
 		return
 	}
 	sum := checksum(data)
@@ -91,10 +94,11 @@ func (u *blurHashUpdater) update(ctx context.Context, artID model.ArtworkID, dat
 
 // clearIfStored clears the persisted hash after a placeholder was served (a cold map costs one row
 // read to skip never-hashed entities); a failed read clears nothing — unknown state is not deletion.
-func (u *blurHashUpdater) clearIfStored(ctx context.Context, artID model.ArtworkID, version time.Time) {
+func (u *blurHashUpdater) clearIfStored(ctx context.Context, artID model.ArtworkID) {
 	if !eligibleKind(artID) {
 		return
 	}
+	artID.LastUpdate = time.Time{}
 	ctx = context.WithoutCancel(ctx)
 	u.mutex.Lock()
 	prev, ok := u.seen[artID]
@@ -108,15 +112,15 @@ func (u *blurHashUpdater) clearIfStored(ctx context.Context, artID model.Artwork
 			return
 		}
 		if stored == "" {
-			u.remember(artID, blurHashState{version: version})
+			u.remember(artID, blurHashState{})
 			return
 		}
 	}
-	if err := u.persist(ctx, artID, "", version); err != nil {
+	if err := u.persist(ctx, artID, "", time.Now()); err != nil {
 		log.Warn(ctx, "BlurHash: error clearing hash", "artID", artID, err)
 		return
 	}
-	u.remember(artID, blurHashState{version: version})
+	u.remember(artID, blurHashState{})
 }
 
 func (u *blurHashUpdater) persistAndRemember(ctx context.Context, artID model.ArtworkID, hash string, sum uint64, version time.Time) {

core/artwork/blurhash_updater_internal_test.go

@@ -111,8 +111,22 @@ var _ = Describe("blurHashUpdater", func() {
 
 	It("ignores non-eligible artwork kinds", func() {
 		Expect(func() {
-			u.clearIfStored(GinkgoT().Context(), model.ArtworkID{Kind: model.KindMediaFileArtwork, ID: "mf-1"}, version)
+			u.clearIfStored(GinkgoT().Context(), model.ArtworkID{Kind: model.KindMediaFileArtwork, ID: "mf-1"})
 		}).ToNot(Panic())
 		Expect(u.seen).To(BeEmpty())
 	})
+
+	It("dedups across artwork ids that differ only in their embedded timestamp", func() {
+		// Client coverArt tokens embed a LastUpdate; the seen key must ignore it, or every scan bump
+		// would defeat the dedup and re-decode identical bytes.
+		id := album(model.Album{ID: "al-1", UpdatedAt: version})
+		data := realPNGBytes("dedup")
+		u.update(GinkgoT().Context(), id, data, version)
+		Expect(repo.UpdateBlurHash("al-1", "TAMPERED", version)).To(Succeed())
+		bumped := id
+		bumped.LastUpdate = version.Add(time.Hour)
+		u.update(GinkgoT().Context(), bumped, data, version)
+		Expect(stored("al-1").BlurHash).To(Equal("TAMPERED"))
+		Expect(u.seen).To(HaveLen(1))
+	})
 })

core/artwork/e2e/blurhash_test.go

@@ -16,6 +16,15 @@ var _ = Describe("BlurHash", func() {
 		setupHarness()
 	})
 
+	// The blurhash is computed inline when the served reader is closed, so by the time the read
+	// helpers return, the hash is already persisted — no polling needed.
+	storedAlbum := func(id string) model.Album {
+		GinkgoHelper()
+		updated, err := ds.Album(ctx).Get(id)
+		Expect(err).ToNot(HaveOccurred())
+		return *updated
+	}
+
 	It("persists a real blurhash after album artwork is served", func() {
 		setLayout(fstest.MapFS{
 			"Artist/Album/01 - Song.mp3": trackFile(1, "Song"),
@@ -25,18 +34,14 @@ var _ = Describe("BlurHash", func() {
 		al := firstAlbum()
 		Expect(al.BlurHash).To(BeEmpty())
 
-		// Serving the artwork enqueues the async blurhash computation.
 		readArtwork(al.CoverArtID())
 
-		Eventually(func(g Gomega) {
-			updated, err := ds.Album(ctx).Get(al.ID)
-			g.Expect(err).ToNot(HaveOccurred())
-			g.Expect(len(updated.BlurHash)).To(BeNumerically(">", 6))
-			g.Expect(updated.BlurHashUpdatedAt).ToNot(BeNil())
-			// The snapshot must not be before the artwork version, or the DTO would treat it as
-			// stale (it may exceed it: image file mtimes are folded in).
-			g.Expect(updated.BlurHashUpdatedAt.Before(updated.ArtworkUpdatedAt())).To(BeFalse())
-		}, "10s", "100ms").Should(Succeed())
+		updated := storedAlbum(al.ID)
+		Expect(len(updated.BlurHash)).To(BeNumerically(">", 6))
+		Expect(updated.BlurHashUpdatedAt).ToNot(BeNil())
+		// The snapshot must not be before the artwork version, or the DTO would treat it as
+		// stale (it may exceed it: image file mtimes are folded in).
+		Expect(updated.BlurHashUpdatedAt.Before(updated.ArtworkUpdatedAt())).To(BeFalse())
 	})
 
 	It("does not persist a future-dated blurhash timestamp", func() {
@@ -50,15 +55,12 @@ var _ = Describe("BlurHash", func() {
 		al := firstAlbum()
 		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.BlurHashUpdatedAt).ToNot(BeNil())
-			// A future file mtime must be capped at now, or the !Before checks would pin the hash
-			// (and the client's cover cache) until wall time caught up.
-			g.Expect(updated.BlurHashUpdatedAt.After(time.Now())).To(BeFalse())
-		}, "10s", "100ms").Should(Succeed())
+		updated := storedAlbum(al.ID)
+		Expect(updated.BlurHash).ToNot(BeEmpty())
+		Expect(updated.BlurHashUpdatedAt).ToNot(BeNil())
+		// A future file mtime must be capped at now, or the !Before checks would pin the hash
+		// (and the client's cover cache) until wall time caught up.
+		Expect(updated.BlurHashUpdatedAt.After(time.Now())).To(BeFalse())
 	})
 
 	It("recomputes when the cover is swapped in place", func() {
@@ -69,13 +71,8 @@ var _ = Describe("BlurHash", func() {
 		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())
+		firstHash := storedAlbum(al.ID).BlurHash
+		Expect(firstHash).ToNot(BeEmpty())
 
 		// Swap the cover bytes and rescan, then serve: the tee hashes the newly-served bytes, so the
 		// stored hash moves to describe the new cover.
@@ -86,12 +83,9 @@ var _ = Describe("BlurHash", func() {
 		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())
+		updated := storedAlbum(al.ID)
+		Expect(updated.BlurHash).ToNot(BeEmpty())
+		Expect(updated.BlurHash).ToNot(Equal(firstHash))
 	})
 
 	It("clears the stored blurhash when the cover disappears", func() {
@@ -102,25 +96,17 @@ var _ = Describe("BlurHash", func() {
 		scan()
 		al := firstAlbum()
 		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())
-		}, "10s", "100ms").Should(Succeed())
+		Expect(storedAlbum(al.ID).BlurHash).ToNot(BeEmpty())
 
 		// No rescan: the folder row still lists the cover, but the file is gone. The serve falls back
-		// to the placeholder (GetOrPlaceholder, the real Jellyfin/Subsonic path), and the worker's
-		// gone-recheck confirms the source is really gone and clears the stored hash.
+		// to the placeholder (GetOrPlaceholder, the real Jellyfin/Subsonic path), which clears the
+		// stored hash inline.
 		setLayout(fstest.MapFS{
 			"Artist/Album/01 - Song.mp3": trackFile(1, "Song"),
 		})
 		Expect(readOrPlaceholder(al.CoverArtID())).To(Equal(placeholderBytes()))
 
-		Eventually(func(g Gomega) {
-			updated, err := ds.Album(ctx).Get(al.ID)
-			g.Expect(err).ToNot(HaveOccurred())
-			g.Expect(updated.BlurHash).To(BeEmpty())
-		}, "10s", "100ms").Should(Succeed())
+		Expect(storedAlbum(al.ID).BlurHash).To(BeEmpty())
 	})
 
 	It("recomputes when cover bytes change under a preserved mtime (cache disabled)", func() {
@@ -134,13 +120,8 @@ var _ = Describe("BlurHash", func() {
 		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())
+		firstHash := storedAlbum(al.ID).BlurHash
+		Expect(firstHash).ToNot(BeEmpty())
 
 		// Replace the bytes but keep the SAME mtime and do NOT rescan: only the served bytes change.
 		swapped := realPNG("swapped-bytes")
@@ -151,11 +132,7 @@ var _ = Describe("BlurHash", func() {
 		})
 		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(Equal(firstHash))
-		}, "10s", "100ms").Should(Succeed())
+		Expect(storedAlbum(al.ID).BlurHash).ToNot(Equal(firstHash))
 	})
 
 	It("clears a stored playlist hash when it falls back to the placeholder", func() {
@@ -169,21 +146,17 @@ var _ = Describe("BlurHash", func() {
 
 		pl := putPlaylist(model.Playlist{ID: "pl-blur", Name: "MyList", Path: m3uPath})
 		readArtwork(pl.CoverArtID())
-		Eventually(func(g Gomega) {
-			updated, err := ds.Playlist(ctx).Get(pl.ID)
-			g.Expect(err).ToNot(HaveOccurred())
-			g.Expect(updated.BlurHash).ToNot(BeEmpty())
-		}, "10s", "100ms").Should(Succeed())
+		stored, err := ds.Playlist(ctx).Get(pl.ID)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(stored.BlurHash).ToNot(BeEmpty())
 
 		// Remove the sidecar: the serve now falls through to the placeholder, captured by the tee.
 		Expect(os.Remove(sidecar)).To(Succeed())
 		Expect(readArtwork(pl.CoverArtID())).To(Equal(placeholderBytes()))
 
-		Eventually(func(g Gomega) {
-			updated, err := ds.Playlist(ctx).Get(pl.ID)
-			g.Expect(err).ToNot(HaveOccurred())
-			g.Expect(updated.BlurHash).To(BeEmpty())
-		}, "10s", "100ms").Should(Succeed())
+		stored, err = ds.Playlist(ctx).Get(pl.ID)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(stored.BlurHash).To(BeEmpty())
 	})
 
 	It("does not persist a blurhash when the served image cannot be decoded", func() {
@@ -196,10 +169,6 @@ var _ = Describe("BlurHash", func() {
 
 		readArtwork(al.CoverArtID())
 
-		Consistently(func(g Gomega) {
-			updated, err := ds.Album(ctx).Get(al.ID)
-			g.Expect(err).ToNot(HaveOccurred())
-			g.Expect(updated.BlurHash).To(BeEmpty())
-		}, "600ms", "100ms").Should(Succeed())
+		Expect(storedAlbum(al.ID).BlurHash).To(BeEmpty())
 	})
 })

core/artwork/tee_reader.go

@@ -15,7 +15,6 @@ type teeReader struct {
 	onComplete func(data []byte)
 	eof        bool
 	over       bool
-	done       bool
 }
 
 func newTeeReader(src io.ReadCloser, maxBytes int, onComplete func(data []byte)) *teeReader {
@@ -40,9 +39,10 @@ func (t *teeReader) Read(p []byte) (int, error) {
 
 func (t *teeReader) Close() error {
 	err := t.src.Close()
-	if !t.done && t.eof && !t.over && t.onComplete != nil {
-		t.done = true
-		t.onComplete(t.buf.Bytes())
+	if t.eof && !t.over && t.onComplete != nil {
+		cb := t.onComplete
+		t.onComplete = nil // fire at most once, even on double Close
+		cb(t.buf.Bytes())
 	}
 	return err
 }

model/album.go

@@ -68,7 +68,7 @@ type Album struct {
 	CreatedAt  time.Time `structs:"created_at" json:"createdAt"`                 // Oldest CreatedAt for all songs in this album
 	UpdatedAt  time.Time `structs:"updated_at" json:"updatedAt"`                 // Newest UpdatedAt for all songs in this album
 
-	// BlurHash of the album cover, computed asynchronously from the served artwork. Excluded from
+	// BlurHash of the album cover, computed from the served artwork bytes. Excluded from
 	// full-row writes (structs:"-"): only UpdateBlurHash writes it, so scans can't erase it.
 	BlurHash          string     `structs:"-" json:"blurHash,omitempty" hash:"ignore"`
 	BlurHashUpdatedAt *time.Time `structs:"-" json:"-" hash:"ignore"`
@@ -80,7 +80,7 @@ func (a Album) CoverArtID() ArtworkID {
 
 // ArtworkUpdatedAt is the album's artwork version. ExternalInfoUpdatedAt is deliberately excluded:
 // it bumps on every agent TTL refresh even when the image is unchanged, and actual image changes
-// are caught by the image-cache-miss recompute instead.
+// are caught by hashing the served bytes instead.
 func (a Album) ArtworkUpdatedAt() time.Time {
 	t := a.UpdatedAt
 	if a.ImportedAt.After(t) {

model/artist.go

@@ -68,7 +68,7 @@ func (a Artist) CoverArtID() ArtworkID {
 
 // ArtworkUpdatedAt is the artist's artwork version. ExternalInfoUpdatedAt is deliberately
 // excluded: it bumps on every agent TTL refresh even when the image is unchanged, and actual
-// image changes are caught by the image-cache-miss recompute instead.
+// image changes are caught by hashing the served bytes instead.
 func (a Artist) ArtworkUpdatedAt() time.Time {
 	if a.UpdatedAt == nil {
 		return time.Time{}

server/jellyfin/dto/blurhash.go

@@ -4,29 +4,10 @@ import (
 	"fmt"
 	"hash/fnv"
 	"time"
+
+	"github.com/navidrome/navidrome/core/artwork/blurhash"
 )
 
-// base83Alphabet is the blurhash spec's base83 encoding alphabet; order is part of the spec.
-const base83Alphabet = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz#$%*+,-.:;=?@[]^_{|}~"
-
-// base83 encodes value as a fixed-width, big-endian base83 string of the given length.
-func base83(value, length int) string {
-	b := make([]byte, length)
-	for i := 1; i <= length; i++ {
-		digit := (value / pow83(length-i)) % 83
-		b[i-1] = base83Alphabet[digit]
-	}
-	return string(b)
-}
-
-func pow83(n int) int {
-	result := 1
-	for range n {
-		result *= 83
-	}
-	return result
-}
-
 // blurHash returns a valid 6-char blurhash for a solid color derived from seed. Finamp only needs a
 // well-formed, per-tag-stable value (it uses this as a download de-dup key and blur placeholder), so
 // a solid color unique to the tag satisfies both without decoding cover art.
@@ -36,7 +17,7 @@ func blurHash(seed string) string {
 	sum := h.Sum(nil)
 	r, g, b := int(sum[0]), int(sum[1]), int(sum[2])
 	dc := (r << 16) | (g << 8) | b
-	return "00" + base83(dc, 4)
+	return "00" + blurhash.Encode83(dc, 4)
 }
 
 // primaryBlurHash returns the stored blurhash when it was computed from the entity's current

server/jellyfin/dto/blurhash_test.go

@@ -8,6 +8,10 @@ import (
 	. "github.com/onsi/gomega"
 )
 
+// base83Alphabet duplicates the spec alphabet on purpose: the test must catch the production copy
+// drifting, not drift along with it.
+const base83Alphabet = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz#$%*+,-.:;=?@[]^_{|}~"
+
 var _ = Describe("blurHash", func() {
 	It("returns a 6-char valid blurhash starting with the 1x1 component prefix", func() {
 		h := blurHash("x")