From f893690cb60e2176a79876b8cbde12083db615c2 Mon Sep 17 00:00:00 2001 From: Deluan Date: Sun, 26 Jul 2026 21:15:23 -0400 Subject: [PATCH] refactor(artwork): give workerDeps only what the processor uses The bag carried cache, which processItem never reads and only the worker's precache uses, and carried agents/ffmpeg/gate solely to reconstruct a resolver on every queue item. cache and ffmpeg move to Worker, where precache actually uses them, and the resolver is built once in NewWorker. persist's hash parameter was redundant: decodeArtwork sets Hash and GetImage selects it, so art.Hash already holds it on both paths. The type itself now lives beside Worker, which owns it, rather than in the file of the function it is passed to. --- core/artwork/processor.go | 25 ++++--------------------- core/artwork/processor_test.go | 2 +- core/artwork/worker.go | 23 ++++++++++++++++++----- core/artwork/worker_soak_test.go | 2 +- 4 files changed, 24 insertions(+), 28 deletions(-) diff --git a/core/artwork/processor.go b/core/artwork/processor.go index fcdcaa44a..a62cf131c 100644 --- a/core/artwork/processor.go +++ b/core/artwork/processor.go @@ -9,15 +9,11 @@ import ( "image/draw" _ "image/gif" // the only artwork format with no other importer in this package "io" - "sync" "time" - "github.com/navidrome/navidrome/core/agents" "github.com/navidrome/navidrome/core/artwork/blurhash" - "github.com/navidrome/navidrome/core/ffmpeg" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" - "github.com/navidrome/navidrome/utils/cache" xdraw "golang.org/x/image/draw" ) @@ -45,19 +41,6 @@ const maxImageBytes = 20 << 20 // huge canvas that image.Decode would expand into gigabytes (decompression bomb). const maxImagePixels = 64 << 20 -// workerDeps are the collaborators processItem needs; gate and pruneLock are set by NewWorker -// in production and nil only in tests, where resolveItem falls back to a plain passthrough and -// nothing prunes. -type workerDeps struct { - ds model.DataStore - store *ImageStore - agents *agents.Agents - ffmpeg ffmpeg.FFmpeg - cache cache.FileCache - gate gateFunc - pruneLock sync.Locker -} - // acquired is what processItem persisted, handed back so the caller can warm the resize // cache without re-reading the rows and the file it just wrote. type acquired struct { @@ -71,7 +54,7 @@ type acquired struct { func processItem(ctx context.Context, deps *workerDeps, item model.ArtworkQueueItem) (outcome, *acquired) { repo := deps.ds.Artwork(ctx) - res, err := newResolver(deps.ds, deps.agents, deps.ffmpeg, deps.gate).resolve(ctx, item) + res, err := deps.resolver.resolve(ctx, item) if err != nil { log.Warn(ctx, "Artwork: Could not resolve item", "kind", item.ItemKind, "id", item.ItemID, err) return outcomeFailed, nil @@ -115,7 +98,7 @@ func processItem(ctx context.Context, deps *workerDeps, item model.ArtworkQueueI } art.SizeBytes = int64(len(data)) - ia, err := persist(deps, repo, item, hash, art, res, data) + ia, err := persist(deps, repo, item, art, res, data) if err != nil { log.Warn(ctx, "Artwork: Failed to persist resolved image", "kind", item.ItemKind, "id", item.ItemID, err) return outcomeFailed, nil @@ -130,7 +113,7 @@ func processItem(ctx context.Context, deps *workerDeps, item model.ArtworkQueueI // persist places the bytes and commits the rows referencing them. Only this window excludes // Prune, which reclaims store files no row points at; resolution stays outside so a slow fetch // cannot hold prune off. -func persist(deps *workerDeps, repo model.ArtworkRepository, item model.ArtworkQueueItem, hash string, +func persist(deps *workerDeps, repo model.ArtworkRepository, item model.ArtworkQueueItem, art *model.Artwork, res resolution, data []byte, ) (*model.ItemArtwork, error) { if deps.pruneLock != nil { @@ -148,7 +131,7 @@ func persist(deps *workerDeps, repo model.ArtworkRepository, item model.ArtworkQ ItemKind: item.ItemKind, ItemID: item.ItemID, ImageType: item.ImageType, - Hash: hash, + Hash: art.Hash, Source: res.source, SourcePath: sourcePath, RefMtime: refMtime, diff --git a/core/artwork/processor_test.go b/core/artwork/processor_test.go index c6fb3321d..d16179624 100644 --- a/core/artwork/processor_test.go +++ b/core/artwork/processor_test.go @@ -71,7 +71,7 @@ var _ = Describe("processItem", func() { } ds.MockedAlbum = tests.CreateMockAlbumRepo() store = NewImageStore(GinkgoT().TempDir()) - deps = &workerDeps{ds: ds, store: store, agents: ag, ffmpeg: ffm} + deps = &workerDeps{ds: ds, store: store, resolver: newResolver(ds, ag, ffm, nil)} conf.Server.CoverArtPriority = "cover.jpg, embedded" }) diff --git a/core/artwork/worker.go b/core/artwork/worker.go index 0fece4ed3..6881ac1a1 100644 --- a/core/artwork/worker.go +++ b/core/artwork/worker.go @@ -49,11 +49,22 @@ type drainPool struct { wake chan struct{} } +// workerDeps are the collaborators processItem needs. The resolver is built once by NewWorker +// rather than per item; pruneLock is nil only in tests, where nothing prunes. +type workerDeps struct { + ds model.DataStore + store *ImageStore + resolver *resolver + pruneLock sync.Locker +} + // Worker drains the artwork queue through processItem: each external agent is rate-limited // and circuit-broken independently, and prune is serialized against the store-write window // via pruneMu. type Worker struct { deps workerDeps + cache cache.FileCache + ffmpeg ffmpeg.FFmpeg broker events.Broker pruneMu sync.RWMutex pools []*drainPool @@ -65,13 +76,15 @@ type Worker struct { func NewWorker(ds model.DataStore, store *ImageStore, ag *agents.Agents, ffmpeg ffmpeg.FFmpeg, broker events.Broker, imgCache cache.FileCache) *Worker { w := &Worker{ - deps: workerDeps{ds: ds, store: store, agents: ag, ffmpeg: ffmpeg, cache: imgCache}, + deps: workerDeps{ds: ds, store: store}, + cache: imgCache, + ffmpeg: ffmpeg, broker: broker, pools: newDrainPools(), runCtx: context.Background(), gates: map[string]*extGate{}, } - w.deps.gate = w.gate + w.deps.resolver = newResolver(ds, ag, ffmpeg, w.gate) w.deps.pruneLock = w.pruneMu.RLocker() return w } @@ -301,7 +314,7 @@ func (w *Worker) hasResolvedArtwork(ctx context.Context, item model.ArtworkQueue // first UI request is a cache hit without re-reading the rows or the file. Skipped when // disabled; failures are debug-only. func (w *Worker) precache(ctx context.Context, got *acquired) { - if !conf.Server.EnableArtworkPrecache || w.deps.cache == nil || w.deps.cache.Disabled(ctx) { + if !conf.Server.EnableArtworkPrecache || w.cache == nil || w.cache.Disabled(ctx) { return } // Same key as the serving path (hash/size/square); only the source of the bytes differs. @@ -310,10 +323,10 @@ func (w *Worker) precache(ctx context.Context, got *acquired) { hash: got.ia.Hash, size: conf.Server.UICoverArtSize, square: true, - ffmpeg: w.deps.ffmpeg, + ffmpeg: w.ffmpeg, open: func() (io.ReadCloser, error) { return io.NopCloser(bytes.NewReader(got.data)), nil }, } - stream, err := w.deps.cache.Get(ctx, item) + stream, err := w.cache.Get(ctx, item) if err != nil { log.Debug(ctx, "Artwork: Precache failed", "kind", got.ia.ItemKind, "id", got.ia.ItemID, err) return diff --git a/core/artwork/worker_soak_test.go b/core/artwork/worker_soak_test.go index f728e6899..5d06a19a0 100644 --- a/core/artwork/worker_soak_test.go +++ b/core/artwork/worker_soak_test.go @@ -54,7 +54,7 @@ var _ = Describe("Worker soak", func() { MockedAlbum: albumRepo, } store := NewImageStore(GinkgoT().TempDir()) - deps := &workerDeps{ds: ds, store: store, agents: ag, ffmpeg: ffm} + deps := &workerDeps{ds: ds, store: store, resolver: newResolver(ds, ag, ffm, nil)} conf.Server.CoverArtPriority = "cover.jpg, embedded" // Dangling refs (al/ra ids the repos don't know about) mirror an entity