From e100c48102ee97ce46644b0e87a41b28e1b5a67e Mon Sep 17 00:00:00 2001 From: Deluan Date: Thu, 16 Jul 2026 08:21:57 -0400 Subject: [PATCH] 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 | 5 +- core/artwork/blurhash_updater.go | 88 +++++++++++++------ .../artwork/blurhash_updater_internal_test.go | 44 +++++++--- model/album.go | 8 +- model/album_test.go | 4 +- model/artist.go | 15 ++-- model/artist_test.go | 6 +- 7 files changed, 112 insertions(+), 58 deletions(-) diff --git a/core/artwork/artwork.go b/core/artwork/artwork.go index 51fefcc18..ee1a6807b 100644 --- a/core/artwork/artwork.go +++ b/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 } diff --git a/core/artwork/blurhash_updater.go b/core/artwork/blurhash_updater.go index 90947c2b1..fee2d388d 100644 --- a/core/artwork/blurhash_updater.go +++ b/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) } diff --git a/core/artwork/blurhash_updater_internal_test.go b/core/artwork/blurhash_updater_internal_test.go index 1dd22f308..70022bd51 100644 --- a/core/artwork/blurhash_updater_internal_test.go +++ b/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()) }) }) }) diff --git a/model/album.go b/model/album.go index 9122276ff..4e8dfae51 100644 --- a/model/album.go +++ b/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 } diff --git a/model/album_test.go b/model/album_test.go index 3caa359f9..9dcf2353e 100644 --- a/model/album_test.go +++ b/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)) }) }) diff --git a/model/artist.go b/model/artist.go index b917023f8..bbb27c3a8 100644 --- a/model/artist.go +++ b/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 { diff --git a/model/artist_test.go b/model/artist_test.go index 135576e81..2d78d8ab3 100644 --- a/model/artist_test.go +++ b/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)) }) })