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)