From aa6c0b1f17fdfb28e88d43e8f93266fe947c2175 Mon Sep 17 00:00:00 2001 From: Deluan Date: Thu, 23 Jul 2026 09:37:07 -0400 Subject: [PATCH] fix(artwork): enqueue new empty playlists by id, and refresh on absent outcomes Two worker/enqueue fixes from review: - playlistRepository.Put assigned the generated id to the caller's Playlist but passed the stale copy (empty id) to refreshCounters, enqueueing a pl|"" row the worker failed until the daily dangling purge while the real playlist went unresolved. Set the id on the copy before enqueueing. - The drain refresh batch only included found/foundStale, so a cover removed by a scan (found -> absent) never notified clients, leaving the old immutable image displayed. Broadcast absent outcomes too; precache still only warms found/foundStale. --- core/artwork/worker.go | 25 +++++++++++++++---------- core/artwork/worker_test.go | 21 ++++++++++++++++++++- persistence/playlist_repository.go | 1 + persistence/playlist_repository_test.go | 13 +++++++++++++ 4 files changed, 49 insertions(+), 11 deletions(-) diff --git a/core/artwork/worker.go b/core/artwork/worker.go index 01ff548de..5191e2285 100644 --- a/core/artwork/worker.go +++ b/core/artwork/worker.go @@ -135,8 +135,8 @@ func (w *Worker) drain(ctx context.Context, concurrency int) (int, error) { } sem := make(chan struct{}, concurrency) var wg sync.WaitGroup - var foundMu sync.Mutex - var found []model.ArtworkQueueItem + var refreshMu sync.Mutex + var refresh []model.ArtworkQueueItem for _, item := range items { sem <- struct{}{} wg.Add(1) @@ -144,19 +144,24 @@ func (w *Worker) drain(ctx context.Context, concurrency int) (int, error) { defer wg.Done() defer func() { <-sem }() defer w.release(it) - // foundStale also wrote a served state row, so it must refresh the UI too. - if out := w.process(ctx, it); out == outcomeFound || out == outcomeFoundStale { - foundMu.Lock() - found = append(found, it) - foundMu.Unlock() - // Post-outcome only: the queue row was already settled by process, so warming - // the resize cache here can never block or alter queue-op handling. + out := w.process(ctx, it) + // Refresh clients on any visible state change: found/foundStale (new art) and absent + // (removed art — clients must drop a previously-served immutable cover). foundStale + // also wrote a served state row. + if out == outcomeFound || out == outcomeFoundStale || out == outcomeAbsent { + refreshMu.Lock() + refresh = append(refresh, it) + refreshMu.Unlock() + } + // Precache only actual images. Post-outcome only: the queue row was already settled + // by process, so warming the resize cache here can never block or alter queue ops. + if out == outcomeFound || out == outcomeFoundStale { w.precache(ctx, it) } }(item) } wg.Wait() - w.broadcastRefresh(ctx, found) + w.broadcastRefresh(ctx, refresh) return len(items), nil } diff --git a/core/artwork/worker_test.go b/core/artwork/worker_test.go index 3917036c7..d773d6bec 100644 --- a/core/artwork/worker_test.go +++ b/core/artwork/worker_test.go @@ -363,10 +363,29 @@ var _ = Describe("Worker", func() { Expect(data).To(ContainSubstring(`"album"`)) Expect(data).To(ContainSubstring("al1")) Expect(data).To(ContainSubstring("al2")) - Expect(data).ToNot(ContainSubstring("artist"), "the absent artist must not be refreshed") + Expect(data).ToNot(ContainSubstring("artist"), "a failed (unresolved) artist must not be refreshed") Expect(data).ToNot(ContainSubstring("ar1")) }) + It("broadcasts a refresh when an item resolves to absent (removed cover)", func() { + conf.Server.CoverArtPriority = "cover.*" // local-only; no folder image → absent + ds.MockedAlbum.(*tests.MockAlbumRepo).SetData(model.Albums{{ID: "al3", Name: "Artless"}}) + folderRepo.result = nil + Expect(queueRepo.Enqueue(model.ArtworkQueueItem{ItemKind: "al", ItemID: "al3", Priority: model.ArtworkPriorityScan})).To(Succeed()) + + n, err := w.drain(ctx, 2) + Expect(err).ToNot(HaveOccurred()) + Expect(n).To(Equal(1)) + + evts := broker.getEvents() + Expect(evts).To(HaveLen(1), "a removed cover must live-refresh clients so they drop it") + Expect(evts[0].(*events.RefreshResource).Data(evts[0])).To(ContainSubstring("al3")) + + ia, err := artRepo.GetItemArtwork("al", "al3", model.ImageTypePrimary) + Expect(err).ToNot(HaveOccurred()) + Expect(ia.Hash).To(BeEmpty(), "the outcome was absent, not found") + }) + It("does not broadcast when no item is found", func() { conf.Server.CoverArtPriority = "external" ds.MockedAlbum.(*tests.MockAlbumRepo).SetData(model.Albums{{ID: "alx", Name: "Album"}}) diff --git a/persistence/playlist_repository.go b/persistence/playlist_repository.go index 397838f07..9942432fa 100644 --- a/persistence/playlist_repository.go +++ b/persistence/playlist_repository.go @@ -132,6 +132,7 @@ func (r *playlistRepository) Put(p *model.Playlist, cols ...string) error { if len(pls.Tracks) > 0 { return r.updateTracks(id, p.MediaFiles()) } + pls.ID = id // r.put assigns the generated id to p, not to this copy; refreshCounters enqueues by it return r.refreshCounters(&pls.Playlist) } diff --git a/persistence/playlist_repository_test.go b/persistence/playlist_repository_test.go index ec5259400..9905d3da6 100644 --- a/persistence/playlist_repository_test.go +++ b/persistence/playlist_repository_test.go @@ -260,6 +260,19 @@ var _ = Describe("PlaylistRepository", func() { Expect(repo.Exists(newPls.ID)).To(BeFalse()) }) + It("enqueues a new empty playlist's artwork under its generated id, not an empty id", func() { + ctx := request.WithUser(log.NewContext(GinkgoT().Context()), model.User{ID: "userid", UserName: "userid", IsAdmin: true}) + newPls := model.Playlist{Name: "Empty PL", OwnerID: "userid"} // no tracks → refreshCounters path + Expect(repo.Put(&newPls)).To(Succeed()) + Expect(newPls.ID).ToNot(BeEmpty()) + DeferCleanup(func() { _ = repo.Delete(newPls.ID) }) + + queued, err := NewArtworkQueueRepository(ctx, GetDBXBuilder()).DequeueBatch(1000) + Expect(err).ToNot(HaveOccurred()) + Expect(queued).To(ContainElement(SatisfyAll(HaveField("ItemKind", "pl"), HaveField("ItemID", newPls.ID)))) + Expect(queued).ToNot(ContainElement(HaveField("ItemID", "")), "must not enqueue an empty playlist id") + }) + It("enqueues the playlist's artwork when its track set changes", func() { ctx := request.WithUser(log.NewContext(GinkgoT().Context()), model.User{ID: "userid", UserName: "userid", IsAdmin: true}) newPls := model.Playlist{Name: "Grid PL", OwnerID: "userid"}