mirror of
https://github.com/navidrome/navidrome.git
synced 2026-08-01 07:21:17 +00:00
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.
This commit is contained in:
parent
e9c15d7fcb
commit
aa6c0b1f17
@ -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
|
||||
}
|
||||
|
||||
|
||||
@ -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"}})
|
||||
|
||||
@ -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)
|
||||
}
|
||||
|
||||
|
||||
@ -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"}
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user