nik/navidrome
1
0
Fork 0
mirror of https://github.com/navidrome/navidrome.git synced 2026-08-01 07:21:17 +00:00
Code Issues Packages Projects Releases Wiki Activity

refactor(artwork): remove dead plumbing from the serving path

Browse Source 
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.
This commit is contained in:
Deluan 2026-07-26 20:25:48 -04:00
parent 28f3c720da
commit 1bb1c7464e
8 changed files with 58 additions and 93 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
core/artwork/image_cache.go
View File

@ -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)
}),
}
})

2
core/artwork/prune.go
View File

@ -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()

20
core/artwork/prune_test.go
View File

@ -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())

75
core/artwork/serving.go
View File

@ -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
}

3
core/artwork/serving_test.go
View File

@ -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)

24
core/artwork/testhelpers_test.go
View File

@ -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
}

15
core/artwork/worker.go
View File

@ -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 {

5
core/artwork/worker_test.go
View File

@ -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)