fix(artwork): address blurhash review findings

- force recompute on image-cache miss, so in-place cover/sidecar swaps and
  agent image updates refresh the stored hash even when no entity row moved
- exclude external_info_updated_at from the artwork version: agent TTL
  refreshes bump it with an unchanged image, churning Finamp's cover cache
- lazy-start the worker goroutine and bound each computation with a 30s
  timeout; remember no-result entities per version to avoid re-decoding
  placeholders on every serve
- error (instead of silently skipping) on unknown artwork kinds in persist

core/artwork/artwork.go

@@ -74,7 +74,10 @@ func (a *artwork) Get(ctx context.Context, artID model.ArtworkID, size int, squa
 		return nil, time.Time{}, err
 	}
 	if a.blurHashes != nil {
-		a.blurHashes.Enqueue(artID)
+		// 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.
+		force := !r.Cached && !a.cache.Disabled(ctx)
+		a.blurHashes.Enqueue(artID, force)
 	}
 	return r, artReader.LastUpdated(), nil
 }

core/artwork/blurhash_updater.go

@@ -2,6 +2,7 @@ package artwork
 
 import (
 	"context"
+	"fmt"
 	"image"
 	"sync"
 	"time"
@@ -17,32 +18,36 @@ import (
 // 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.
 type blurHashUpdater struct {
-	a      *artwork
-	mutex  sync.Mutex
-	buffer map[model.ArtworkID]struct{}
-	wake   chan struct{}
+	a        *artwork
+	mutex    sync.Mutex
+	buffer   map[model.ArtworkID]bool // value: force recompute (image-cache miss)
+	noResult map[model.ArtworkID]time.Time
+	wake     chan struct{}
+	start    sync.Once
 }
 
 func newBlurHashUpdater(a *artwork) *blurHashUpdater {
-	u := &blurHashUpdater{
-		a:      a,
-		buffer: make(map[model.ArtworkID]struct{}),
-		wake:   make(chan struct{}, 1),
+	return &blurHashUpdater{
+		a:        a,
+		buffer:   make(map[model.ArtworkID]bool),
+		noResult: make(map[model.ArtworkID]time.Time),
+		wake:     make(chan struct{}, 1),
 	}
-	// Playlist artwork readers require a user in the context.
-	ctx := request.WithUser(context.TODO(), model.User{IsAdmin: true})
-	go u.run(ctx)
-	return u
 }
 
-func (u *blurHashUpdater) Enqueue(artID model.ArtworkID) {
+func (u *blurHashUpdater) Enqueue(artID model.ArtworkID, force bool) {
 	switch artID.Kind {
 	case model.KindAlbumArtwork, model.KindArtistArtwork, model.KindPlaylistArtwork:
 	default:
 		return
 	}
+	u.start.Do(func() {
+		// Playlist artwork readers require a user in the context. Like the cacheWarmer, the worker
+		// lives for the rest of the process; lazy-starting keeps idle Artwork instances goroutine-free.
+		go u.run(request.WithUser(context.TODO(), model.User{IsAdmin: true}))
+	})
 	u.mutex.Lock()
-	u.buffer[artID] = struct{}{}
+	u.buffer[artID] = u.buffer[artID] || force
 	u.mutex.Unlock()
 	select {
 	case u.wake <- struct{}{}:
@@ -53,48 +58,81 @@ func (u *blurHashUpdater) Enqueue(artID model.ArtworkID) {
 func (u *blurHashUpdater) run(ctx context.Context) {
 	for range u.wake {
 		for {
-			artID, ok := u.next()
+			artID, force, ok := u.next()
 			if !ok {
 				break
 			}
-			u.process(ctx, artID)
+			u.process(ctx, artID, force)
 		}
 	}
 }
 
-func (u *blurHashUpdater) next() (model.ArtworkID, bool) {
+func (u *blurHashUpdater) next() (model.ArtworkID, bool, bool) {
 	u.mutex.Lock()
 	defer u.mutex.Unlock()
-	for artID := range u.buffer {
+	for artID, force := range u.buffer {
 		delete(u.buffer, artID)
-		return artID, true
+		return artID, force, true
 	}
-	return model.ArtworkID{}, false
+	return model.ArtworkID{}, false, false
 }
 
-func (u *blurHashUpdater) process(ctx context.Context, artID model.ArtworkID) {
+// 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) {
 	// Artwork readers can touch storage, agents and plugins; a panic here must not kill the server.
 	defer func() {
 		if r := recover(); r != nil {
 			log.Error(ctx, "BlurHash: recovered from panic", "artID", artID, "panic", r)
 		}
 	}()
+	ctx, cancel := context.WithTimeout(ctx, processTimeout)
+	defer cancel()
 	stored, storedAt, version, err := u.loadState(ctx, artID)
 	if err != nil {
 		log.Trace(ctx, "BlurHash: could not load entity", "artID", artID, err)
 		return
 	}
-	if stored != "" && storedAt != nil && storedAt.Equal(version) {
-		return
+	if !force {
+		if stored != "" && storedAt != nil && storedAt.Equal(version) {
+			return
+		}
+		if last, ok := u.lastNoResult(artID); ok && last.Equal(version) {
+			return
+		}
 	}
 	hash, err := u.computeFromArtwork(ctx, artID)
 	if err != nil || hash == "" {
-		log.Trace(ctx, "BlurHash: skipping", "artID", artID, err)
+		log.Trace(ctx, "BlurHash: nothing to persist", "artID", artID, err)
+		u.setNoResult(artID, version)
 		return
 	}
 	if err := u.persist(ctx, artID, hash, version); err != nil {
 		log.Warn(ctx, "BlurHash: error persisting", "artID", artID, err)
+		return
 	}
+	u.clearNoResult(artID)
+}
+
+func (u *blurHashUpdater) lastNoResult(artID model.ArtworkID) (time.Time, bool) {
+	u.mutex.Lock()
+	defer u.mutex.Unlock()
+	t, ok := u.noResult[artID]
+	return t, ok
+}
+
+func (u *blurHashUpdater) setNoResult(artID model.ArtworkID, version time.Time) {
+	u.mutex.Lock()
+	defer u.mutex.Unlock()
+	u.noResult[artID] = version
+}
+
+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) {
@@ -154,5 +192,5 @@ func (u *blurHashUpdater) persist(ctx context.Context, artID model.ArtworkID, ha
 	case model.KindPlaylistArtwork:
 		return u.a.ds.Playlist(ctx).UpdateBlurHash(artID.ID, hash, version)
 	}
-	return nil
+	return fmt.Errorf("blurhash: no persister for artwork kind %q", artID.Kind)
 }

core/artwork/blurhash_updater_internal_test.go

@@ -17,46 +17,64 @@ var _ = Describe("blurHashUpdater", func() {
 		ds = &tests.MockDataStore{}
 		// No run() goroutine: tests drive next()/process() directly.
 		u = &blurHashUpdater{
-			a:      &artwork{ds: ds},
-			buffer: make(map[model.ArtworkID]struct{}),
-			wake:   make(chan struct{}, 1),
+			a:        &artwork{ds: ds},
+			buffer:   make(map[model.ArtworkID]bool),
+			noResult: make(map[model.ArtworkID]time.Time),
+			wake:     make(chan struct{}, 1),
 		}
 	})
 
 	Describe("Enqueue", func() {
-		It("accepts album, artist and playlist artwork and dedups", func() {
+		It("accepts album, artist and playlist artwork and dedups, keeping the force flag sticky", func() {
 			id := model.Album{ID: "al-1"}.CoverArtID()
-			u.Enqueue(id)
-			u.Enqueue(id)
-			u.Enqueue(model.Artist{ID: "ar-1"}.CoverArtID())
+			u.Enqueue(id, true)
+			u.Enqueue(id, false)
+			u.Enqueue(model.Artist{ID: "ar-1"}.CoverArtID(), false)
 			Expect(u.buffer).To(HaveLen(2))
+			Expect(u.buffer[id]).To(BeTrue())
 		})
 
 		It("ignores other artwork kinds", func() {
-			u.Enqueue(model.ArtworkID{Kind: model.KindMediaFileArtwork, ID: "mf-1"})
-			u.Enqueue(model.ArtworkID{Kind: model.KindRadioArtwork, ID: "ra-1"})
+			u.Enqueue(model.ArtworkID{Kind: model.KindMediaFileArtwork, ID: "mf-1"}, false)
+			u.Enqueue(model.ArtworkID{Kind: model.KindRadioArtwork, ID: "ra-1"}, true)
 			Expect(u.buffer).To(BeEmpty())
 		})
 	})
 
 	Describe("process", func() {
+		var version time.Time
+
+		BeforeEach(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() {
-			version := time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC)
 			al := model.Album{ID: "al-1", UpdatedAt: version, BlurHash: "LEHV6nWB2yk8", BlurHashUpdatedAt: &version}
 			repo := tests.CreateMockAlbumRepo()
 			repo.SetData(model.Albums{al})
 			ds.MockedAlbum = repo
 
-			// u.a has no cache: if process tried to compute, it would panic. Not panicking proves the skip.
-			Expect(func() { u.process(GinkgoT().Context(), al.CoverArtID()) }).ToNot(Panic())
+			u.process(GinkgoT().Context(), al.CoverArtID(), false)
 			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() {
+			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.process(GinkgoT().Context(), al.CoverArtID(), false)
+			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() { u.process(GinkgoT().Context(), model.Album{ID: "missing"}.CoverArtID()) }).ToNot(Panic())
+			Expect(func() { u.process(GinkgoT().Context(), model.Album{ID: "missing"}.CoverArtID(), false) }).ToNot(Panic())
 		})
 	})
 })

model/album.go

@@ -77,16 +77,14 @@ func (a Album) CoverArtID() ArtworkID {
 	return artworkIDFromAlbum(a)
 }
 
-// ArtworkUpdatedAt is the album's artwork version: the newest row timestamp that can affect
-// which cover image is served (scan updates, imports, agent-fetched external images).
+// 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.
 func (a Album) ArtworkUpdatedAt() time.Time {
 	t := a.UpdatedAt
 	if a.ImportedAt.After(t) {
 		t = a.ImportedAt
 	}
-	if a.ExternalInfoUpdatedAt != nil && a.ExternalInfoUpdatedAt.After(t) {
-		t = *a.ExternalInfoUpdatedAt
-	}
 	return t
 }
 

model/album_test.go

@@ -65,8 +65,8 @@ var _ = Describe("Album.ArtworkUpdatedAt", func() {
 		al := Album{UpdatedAt: base, ImportedAt: later}
 		Expect(al.ArtworkUpdatedAt()).To(Equal(later))
 	})
-	It("returns ExternalInfoUpdatedAt when it is the newest", func() {
+	It("ignores ExternalInfoUpdatedAt (agent TTL refreshes bump it without an image change)", func() {
 		al := Album{UpdatedAt: base, ImportedAt: later, ExternalInfoUpdatedAt: &latest}
-		Expect(al.ArtworkUpdatedAt()).To(Equal(latest))
+		Expect(al.ArtworkUpdatedAt()).To(Equal(later))
 	})
 })

model/artist.go

@@ -66,17 +66,14 @@ func (a Artist) CoverArtID() ArtworkID {
 	return artworkIDFromArtist(a)
 }
 
-// ArtworkUpdatedAt is the artist's artwork version; images often arrive via external agents,
-// which bump ExternalInfoUpdatedAt rather than UpdatedAt.
+// 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.
 func (a Artist) ArtworkUpdatedAt() time.Time {
-	var t time.Time
-	if a.UpdatedAt != nil {
-		t = *a.UpdatedAt
+	if a.UpdatedAt == nil {
+		return time.Time{}
 	}
-	if a.ExternalInfoUpdatedAt != nil && a.ExternalInfoUpdatedAt.After(t) {
-		t = *a.ExternalInfoUpdatedAt
-	}
-	return t
+	return *a.UpdatedAt
 }
 
 func (a Artist) UploadedImagePath() string {

model/artist_test.go

@@ -37,10 +37,10 @@ var _ = Describe("Artist.ArtworkUpdatedAt", func() {
 	It("handles nil timestamps", func() {
 		Expect(model.Artist{}.ArtworkUpdatedAt()).To(Equal(time.Time{}))
 	})
-	It("returns UpdatedAt when newest", func() {
+	It("returns UpdatedAt", func() {
 		Expect(model.Artist{UpdatedAt: &later, ExternalInfoUpdatedAt: &base}.ArtworkUpdatedAt()).To(Equal(later))
 	})
-	It("returns ExternalInfoUpdatedAt when newest", func() {
-		Expect(model.Artist{UpdatedAt: &base, ExternalInfoUpdatedAt: &later}.ArtworkUpdatedAt()).To(Equal(later))
+	It("ignores ExternalInfoUpdatedAt (agent TTL refreshes bump it without an image change)", func() {
+		Expect(model.Artist{UpdatedAt: &base, ExternalInfoUpdatedAt: &later}.ArtworkUpdatedAt()).To(Equal(base))
 	})
 })