From 1bb1c7464e9a9e72812f31478a0338929ce1bd19 Mon Sep 17 00:00:00 2001 From: Deluan Date: Sun, 26 Jul 2026 20:25:48 -0400 Subject: [PATCH] refactor(artwork): remove dead plumbing from the serving path artworkReader.LastUpdated had no callers: invalidation rides entirely on the cache key, so the interface member, the resizedItem field and its four assignments were vestigial. Reader's second return value was likewise discarded at all three call sites. resizedItem.Key duplicated representationTag's format string over identical inputs, where drift would serve a wrong-keyed entry under a right-looking validator; it now derives from it. newResizedItem had one caller and a doc comment claiming a sharing with worker.precache that never existed -- precache builds its own literal. Also unexport Prune, which no caller outside the package used while RunPrune documented itself as the only sanctioned path, drop a single-call placeholder wrapper, and delete five fakeFolderRepo fields no spec ever set. --- core/artwork/image_cache.go | 7 +-- core/artwork/prune.go | 2 +- core/artwork/prune_test.go | 20 ++++----- core/artwork/serving.go | 75 ++++++++++++-------------------- core/artwork/serving_test.go | 3 +- core/artwork/testhelpers_test.go | 24 +++------- core/artwork/worker.go | 15 +++---- core/artwork/worker_test.go | 5 +-- 8 files changed, 58 insertions(+), 93 deletions(-) diff --git a/core/artwork/image_cache.go b/core/artwork/image_cache.go index 7d73b1bae..3d58e691b 100644 --- a/core/artwork/image_cache.go +++ b/core/artwork/image_cache.go @@ -3,7 +3,6 @@ package artwork import ( "context" "io" - "time" "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/consts" @@ -15,8 +14,7 @@ import ( // produces the (possibly resized) bytes to store under Key. type artworkReader interface { cache.Item - LastUpdated() time.Time - Reader(ctx context.Context) (io.ReadCloser, string, error) + Reader(ctx context.Context) (io.ReadCloser, error) } type imageCache struct { @@ -28,8 +26,7 @@ func GetImageCache() cache.FileCache { return &imageCache{ FileCache: cache.NewFileCache("Image", conf.Server.ImageCacheSize, consts.ImageCacheDir, consts.DefaultImageCacheMaxItems, func(ctx context.Context, arg cache.Item) (io.Reader, error) { - r, _, err := arg.(artworkReader).Reader(ctx) - return r, err + return arg.(artworkReader).Reader(ctx) }), } }) diff --git a/core/artwork/prune.go b/core/artwork/prune.go index f6bc53186..7c3e59676 100644 --- a/core/artwork/prune.go +++ b/core/artwork/prune.go @@ -11,7 +11,7 @@ import ( // pruneMinAge guards the window between artwork insert and item_artwork upsert. const pruneMinAge = time.Hour -func Prune(ctx context.Context, ds model.DataStore, store *ImageStore) error { +func prune(ctx context.Context, ds model.DataStore, store *ImageStore) error { repo := ds.Artwork(ctx) purged, err := repo.PurgeDanglingItemArtwork() diff --git a/core/artwork/prune_test.go b/core/artwork/prune_test.go index d102f155c..832f5456a 100644 --- a/core/artwork/prune_test.go +++ b/core/artwork/prune_test.go @@ -49,7 +49,7 @@ var _ = Describe("Prune", func() { "ar": {"live-artist": true}, } - Expect(Prune(context.Background(), ds, store)).To(Succeed()) + Expect(prune(context.Background(), ds, store)).To(Succeed()) _, err := awRepo.GetItemArtwork(model.KindAlbumArtwork, "gone-album", model.ImageTypePrimary) Expect(err).To(MatchError(model.ErrNotFound)) @@ -68,7 +68,7 @@ var _ = Describe("Prune", func() { queueRepo.ExistingIDs = map[string]map[string]bool{"al": {"live-album": true}} ds.MockedArtworkQueue = queueRepo - Expect(Prune(context.Background(), ds, store)).To(Succeed()) + Expect(prune(context.Background(), ds, store)).To(Succeed()) Expect(findQueued(queueRepo, "al", "gone-album")).To(BeNil()) Expect(findQueued(queueRepo, "al", "live-album")).ToNot(BeNil()) @@ -89,7 +89,7 @@ var _ = Describe("Prune", func() { Expect(store.Write(hk, "image/jpeg", bytes.NewReader(kept))).To(Succeed()) Expect(awRepo.PutImage(&model.Artwork{Hash: hk, Mime: "image/jpeg"})).To(Succeed()) - Expect(Prune(context.Background(), ds, store)).To(Succeed()) + Expect(prune(context.Background(), ds, store)).To(Succeed()) _, err := awRepo.GetImage(h) Expect(err).To(MatchError(model.ErrNotFound)) @@ -111,7 +111,7 @@ var _ = Describe("Prune", func() { Expect(awRepo.PutItemArtwork(&model.ItemArtwork{ItemKind: "al", ItemID: "a1", ImageType: model.ImageTypePrimary, Hash: h, Source: "folder"})).To(Succeed()) - Expect(Prune(context.Background(), ds, store)).To(Succeed()) + Expect(prune(context.Background(), ds, store)).To(Succeed()) _, err := awRepo.GetImage(h) Expect(err).ToNot(HaveOccurred()) @@ -128,7 +128,7 @@ var _ = Describe("Prune", func() { Expect(awRepo.PutImage(&model.Artwork{Hash: h, Mime: "image/jpeg"})).To(Succeed()) awRepo.OrphanHashes = []string{h} - Expect(Prune(context.Background(), ds, store)).To(Succeed()) + Expect(prune(context.Background(), ds, store)).To(Succeed()) _, err := awRepo.GetImage(h) Expect(err).ToNot(HaveOccurred()) @@ -147,7 +147,7 @@ var _ = Describe("Prune", func() { // The row is legitimately orphaned, but a concurrent acquisition just touched the // file's mtime (duplicate Write) and is about to commit a row referencing it. - Expect(Prune(context.Background(), ds, store)).To(Succeed()) + Expect(prune(context.Background(), ds, store)).To(Succeed()) rc, err := store.Open(h, "image/jpeg") Expect(err).ToNot(HaveOccurred()) @@ -161,7 +161,7 @@ var _ = Describe("Prune", func() { old := time.Now().Add(-2 * time.Hour) Expect(os.Chtimes(store.path(h, "image/jpeg"), old, old)).To(Succeed()) - Expect(Prune(context.Background(), ds, store)).To(Succeed()) + Expect(prune(context.Background(), ds, store)).To(Succeed()) _, err := store.Open(h, "image/jpeg") Expect(os.IsNotExist(err)).To(BeTrue()) @@ -178,7 +178,7 @@ var _ = Describe("Prune", func() { // The row records the current mime; the .png file is a superseded variant. Expect(awRepo.PutImage(&model.Artwork{Hash: h, Mime: "image/jpeg"})).To(Succeed()) - Expect(Prune(context.Background(), ds, store)).To(Succeed()) + Expect(prune(context.Background(), ds, store)).To(Succeed()) _, err := store.Open(h, "image/png") Expect(os.IsNotExist(err)).To(BeTrue()) @@ -219,7 +219,7 @@ var _ = Describe("Prune", func() { // Prune still errors: Sweep independently revisits hb's leftover file and, // unlike the loop below, has no warn-and-continue fallback of its own. - err := Prune(context.Background(), ds, store) + err := prune(context.Background(), ds, store) Expect(err).To(HaveOccurred()) // hg: reached and fully pruned despite being queued after the failing hb - @@ -245,7 +245,7 @@ var _ = Describe("Prune", func() { h, _ := HashImage(bytes.NewReader(data)) Expect(store.Write(h, "image/jpeg", bytes.NewReader(data))).To(Succeed()) - Expect(Prune(context.Background(), ds, store)).ToNot(Succeed()) + Expect(prune(context.Background(), ds, store)).ToNot(Succeed()) rc, err := store.Open(h, "image/jpeg") Expect(err).ToNot(HaveOccurred()) diff --git a/core/artwork/serving.go b/core/artwork/serving.go index 073159f44..38b86ce9c 100644 --- a/core/artwork/serving.go +++ b/core/artwork/serving.go @@ -69,7 +69,7 @@ func (s *service) GetOrPlaceholder(ctx context.Context, id string, size int, squ // Only a resolvable entity with no art gets the placeholder. An id that matches no entity // stays ErrNotFound, so getCoverArt keeps answering error 70 and Jellyfin keeps 404ing. if errors.Is(err, ErrUnavailable) { - return s.placeholder(artID.Kind), nil + return placeholderImage(artID.Kind), nil } return img, err } @@ -136,7 +136,10 @@ func (s *service) serveHash(ctx context.Context, artID model.ArtworkID, ia *mode return &Image{ReadCloser: rc, Hash: ia.Hash, LastUpdated: ia.UpdatedAt}, nil } - item := newResizedItem(ia, art.Mime, size, square, s.store, s.ffmpeg) + item := &resizedItem{ + hash: ia.Hash, size: size, square: square, ffmpeg: s.ffmpeg, + open: func() (io.ReadCloser, error) { return openOriginal(ia, art.Mime, s.store) }, + } stream, err := s.cache.Get(ctx, item) if err != nil { if errors.Is(err, context.Canceled) { @@ -181,19 +184,6 @@ func openOriginal(ia *model.ItemArtwork, mime string, store *ImageStore) (io.Rea return store.Open(ia.Hash, mime) } -// newResizedItem builds the resize-cache reader for a found state row's bytes; shared by -// the serving path and the worker's precache so both key the cache identically. -func newResizedItem(ia *model.ItemArtwork, mime string, size int, square bool, store *ImageStore, ffm ffmpeg.FFmpeg) *resizedItem { - return &resizedItem{ - hash: ia.Hash, - size: size, - square: square, - lastUpdate: ia.UpdatedAt, - ffmpeg: ffm, - open: func() (io.ReadCloser, error) { return openOriginal(ia, mime, store) }, - } -} - // provisional does a local-only read-through for an entity with no state row: it enqueues // the worker (Bump) and serves any local bytes immediately, never writing a state row. func (s *service) provisional(ctx context.Context, artID model.ArtworkID, size int, square bool) (*Image, error) { @@ -231,12 +221,11 @@ func (s *service) serveBytes(ctx context.Context, hash string, data []byte, last return &Image{ReadCloser: io.NopCloser(bytes.NewReader(data)), Hash: hash, LastUpdated: lastUpdate}, nil } item := &resizedItem{ - hash: hash, - size: size, - square: square, - lastUpdate: lastUpdate, - ffmpeg: s.ffmpeg, - open: func() (io.ReadCloser, error) { return io.NopCloser(bytes.NewReader(data)), nil }, + hash: hash, + size: size, + square: square, + ffmpeg: s.ffmpeg, + open: func() (io.ReadCloser, error) { return io.NopCloser(bytes.NewReader(data)), nil }, } stream, err := s.cache.Get(ctx, item) if err != nil { @@ -328,12 +317,11 @@ func (s *service) serveDisc(ctx context.Context, artID model.ArtworkID, size int } item := &resizedItem{ - hash: key, - size: size, - square: square, - lastUpdate: dr.cacheTime(), - ffmpeg: s.ffmpeg, - open: func() (io.ReadCloser, error) { rc, _, err := selectImage(); return rc, err }, + hash: key, + size: size, + square: square, + ffmpeg: s.ffmpeg, + open: func() (io.ReadCloser, error) { rc, _, err := selectImage(); return rc, err }, } stream, err := s.cache.Get(ctx, item) if err != nil { @@ -363,10 +351,6 @@ func (s *service) enqueue(ctx context.Context, artID model.ArtworkID, priority i } } -func (s *service) placeholder(kind model.Kind) *Image { - return placeholderImage(kind) -} - func placeholderImage(kind model.Kind) *Image { path := consts.PlaceholderAlbumArt if kind == model.KindArtistArtwork { @@ -416,37 +400,36 @@ func unixMtime(mtime int64) time.Time { // resizedItem is an artworkReader that resizes bytes opened by open() and caches the // result under a hash-derived key. type resizedItem struct { - hash string - size int - square bool - lastUpdate time.Time - ffmpeg ffmpeg.FFmpeg - open func() (io.ReadCloser, error) + hash string + size int + square bool + ffmpeg ffmpeg.FFmpeg + open func() (io.ReadCloser, error) } +// Key is the ETag namespaced for the cache, so the validator a client holds and the entry it +// validates can never drift apart. func (r *resizedItem) Key() string { - return fmt.Sprintf("h-%s.%d.%v.%s", r.hash, r.size, r.square, formatQualityTag()) + return "h-" + representationTag(r.hash, r.size, r.square) } -func (r *resizedItem) LastUpdated() time.Time { return r.lastUpdate } - -func (r *resizedItem) Reader(ctx context.Context) (io.ReadCloser, string, error) { +func (r *resizedItem) Reader(ctx context.Context) (io.ReadCloser, error) { orig, err := r.open() if err != nil { - return nil, "", err + return nil, err } defer orig.Close() data, err := readCapped(orig) if err != nil { - return nil, "", err + return nil, err } resized, _, err := resizeImageData(ctx, r.ffmpeg, data, r.size, r.square) if err != nil || resized == nil { // Resize failed or image already within bounds: serve the original bytes. - return io.NopCloser(bytes.NewReader(data)), r.Key(), nil + return io.NopCloser(bytes.NewReader(data)), nil } if rc, ok := resized.(io.ReadCloser); ok { - return rc, r.Key(), nil + return rc, nil } - return io.NopCloser(resized), r.Key(), nil + return io.NopCloser(resized), nil } diff --git a/core/artwork/serving_test.go b/core/artwork/serving_test.go index 0baea606c..2774b6ac8 100644 --- a/core/artwork/serving_test.go +++ b/core/artwork/serving_test.go @@ -93,8 +93,7 @@ var _ = Describe("Service", func() { store = NewImageStore(GinkgoT().TempDir()) imgCache = cache.NewFileCache("ServingTest", "100MB", "images", 0, func(ctx context.Context, arg cache.Item) (io.Reader, error) { - r, _, err := arg.(artworkReader).Reader(ctx) - return r, err + return arg.(artworkReader).Reader(ctx) }) Eventually(func() bool { return imgCache.Available(ctx) }).Should(BeTrue()) svc = NewService(ds, imgCache, store, ffm) diff --git a/core/artwork/testhelpers_test.go b/core/artwork/testhelpers_test.go index 97f69136a..2d1a3f3d3 100644 --- a/core/artwork/testhelpers_test.go +++ b/core/artwork/testhelpers_test.go @@ -4,17 +4,12 @@ import ( "github.com/navidrome/navidrome/model" ) +// fakeFolderRepo covers the three FolderRepository methods the resolvers reach for; only the +// folder listing varies per spec, so the other two answer as an unremarkable library does. type fakeFolderRepo struct { model.FolderRepository - result []model.Folder - parentResult *model.Folder - getErr error - getCallCount int - err error - // hasOtherAudio is returned by HasAudioOutsideFolders (the album-root - // check). False means the parent qualifies as an album root. - hasOtherAudio bool - otherAudioErr error + result []model.Folder + err error } func (f *fakeFolderRepo) GetAll(...model.QueryOptions) ([]model.Folder, error) { @@ -22,16 +17,9 @@ func (f *fakeFolderRepo) GetAll(...model.QueryOptions) ([]model.Folder, error) { } func (f *fakeFolderRepo) HasAudioOutsideFolders(model.Folder, []string) (bool, error) { - return f.hasOtherAudio, f.otherAudioErr + return false, nil } -func (f *fakeFolderRepo) Get(id string) (*model.Folder, error) { - f.getCallCount++ - if f.getErr != nil { - return nil, f.getErr - } - if f.parentResult != nil { - return f.parentResult, nil - } +func (f *fakeFolderRepo) Get(string) (*model.Folder, error) { return nil, model.ErrNotFound } diff --git a/core/artwork/worker.go b/core/artwork/worker.go index 68af3080b..dfd5f2de6 100644 --- a/core/artwork/worker.go +++ b/core/artwork/worker.go @@ -160,12 +160,12 @@ func (w *Worker) Bump(kind, id string) { } } -// RunPrune runs Prune under the worker's write lock, so no acquisition can place +// RunPrune runs prune under the worker's write lock, so no acquisition can place // a file while orphans are being reclaimed. This is the only sanctioned prune path. func (w *Worker) RunPrune(ctx context.Context) error { w.pruneMu.Lock() defer w.pruneMu.Unlock() - return Prune(ctx, w.deps.ds, w.deps.store) + return prune(ctx, w.deps.ds, w.deps.store) } func (w *Worker) drain(ctx context.Context, concurrency int, kinds ...string) (int, error) { @@ -307,12 +307,11 @@ func (w *Worker) precache(ctx context.Context, got *acquired) { // Same key as the serving path (hash/size/square); only the source of the bytes differs. // square matches what the list surfaces request, otherwise this warms a key nothing reads. item := &resizedItem{ - hash: got.ia.Hash, - size: conf.Server.UICoverArtSize, - square: true, - lastUpdate: got.ia.UpdatedAt, - ffmpeg: w.deps.ffmpeg, - open: func() (io.ReadCloser, error) { return io.NopCloser(bytes.NewReader(got.data)), nil }, + hash: got.ia.Hash, + size: conf.Server.UICoverArtSize, + square: true, + ffmpeg: w.deps.ffmpeg, + open: func() (io.ReadCloser, error) { return io.NopCloser(bytes.NewReader(got.data)), nil }, } stream, err := w.deps.cache.Get(ctx, item) if err != nil { diff --git a/core/artwork/worker_test.go b/core/artwork/worker_test.go index 64e345616..903a1e86f 100644 --- a/core/artwork/worker_test.go +++ b/core/artwork/worker_test.go @@ -149,8 +149,7 @@ var _ = Describe("Worker", func() { broker = &fakeEventBroker{} imgCache = &recordingCache{FileCache: cache.NewFileCache("WorkerTest", "100MB", "images", 0, func(ctx context.Context, arg cache.Item) (io.Reader, error) { - r, _, err := arg.(artworkReader).Reader(ctx) - return r, err + return arg.(artworkReader).Reader(ctx) })} Eventually(func() bool { return imgCache.Available(ctx) }).Should(BeTrue()) w = NewWorker(ds, store, ag, ffm, broker, imgCache) @@ -620,7 +619,7 @@ var _ = Describe("Worker", func() { // from the bytes handed in. Probing with a source that refuses to open proves it // is really cached rather than re-read on demand. probe := &resizedItem{ - hash: ia.Hash, size: 300, square: true, lastUpdate: ia.UpdatedAt, ffmpeg: ffm, + hash: ia.Hash, size: 300, square: true, ffmpeg: ffm, open: func() (io.ReadCloser, error) { return nil, errors.New("precache must not re-read the source") }, } stream, err := imgCache.Get(ctx, probe)