From bc16cafb91a7795f545866fd4854937e5f05bb8d Mon Sep 17 00:00:00 2001 From: Deluan Date: Mon, 14 Sep 2026 19:36:50 -0400 Subject: [PATCH 1/4] fix(artwork): convert animated GIF covers in the background Resized animated GIF covers were converted to animated WebP by ffmpeg inside the image request, bound to its 10s timeout. On slow CPUs large GIFs never finished: ffmpeg was killed, nothing was cached, and every request repeated the work. With ffmpeg 5.1 the conversion also failed mid-stream when reading the GIF from a pipe, after the stream was already handed to the client, so there was no fallback. Requests now get a static stand-in right away, served with no-store and no validators, while the conversion runs in the background. A single process-wide slot bounds these conversions, since they outlive the request throttle; when it is busy nothing is queued and a later request retries. The result, or a static fallback if ffmpeg fails, is cached under the same key. ffmpeg output is now buffered so mid-stream failures fall back to a static resize. The file cache gains cache.Transient for loader results that must be served but not stored. Without an available cache, conversion stays inline. --- core/artwork/artwork.go | 64 ++++++++++++++++++++++++-- core/artwork/artwork_test.go | 80 +++++++++++++++++++++++++++++++++ core/artwork/image_cache.go | 22 +++++++++ core/artwork/resize.go | 23 +++++++++- core/artwork/resize_test.go | 72 +++++++++++++++++++++++++++++ server/imghttp/headers.go | 4 +- server/imghttp/headers_test.go | 9 ++++ utils/cache/file_caches.go | 36 ++++++++++++++- utils/cache/file_caches_test.go | 61 +++++++++++++++++++++++++ 9 files changed, 362 insertions(+), 9 deletions(-) diff --git a/core/artwork/artwork.go b/core/artwork/artwork.go index 663d06d25..1da419dff 100644 --- a/core/artwork/artwork.go +++ b/core/artwork/artwork.go @@ -31,6 +31,7 @@ type Image struct { ETag string // representation validator; "" means Hash applies (full-size original) LastUpdated time.Time Placeholder bool + Transient bool // stand-in while the real representation is produced in the background } // representationTag varies with dimensions and encode settings, so a config change invalidates @@ -152,15 +153,72 @@ func (s *service) serveSource(ctx context.Context, key, hash string, lastUpdate } return img, nil } - stream, err := s.cache.Get(ctx, &resizedItem{ - hash: key, size: size, square: square, ffmpeg: s.ffmpeg, open: open, - }) + item := &resizedItem{hash: key, size: size, square: square, ffmpeg: s.ffmpeg, open: open} + var animated []byte + // Without a cache a background conversion could never be served, so convert inline instead. + if s.cache.Available(ctx) { + item.deferAnimated = func(data []byte) { animated = data } + } + stream, err := s.cache.Get(ctx, item) if err != nil { return nil, err } + if stream.Transient { + s.convertInBackground(item, animated) + return &Image{ReadCloser: stream, Hash: hash, LastUpdated: lastUpdate, Transient: true}, nil + } return &Image{ReadCloser: stream, Hash: hash, ETag: representationTag(key, size, square), LastUpdated: lastUpdate}, nil } +// animConvertSlot bounds animated conversions process-wide: they outlive their request, so no +// request throttle limits them. +var animConvertSlot = make(chan struct{}, 1) + +const animConvertTimeout = time.Minute + +// convertInBackground caches item's conversion of data. When the slot is busy it does nothing, +// and a later request for the key tries again. +func (s *service) convertInBackground(item *resizedItem, data []byte) { + select { + case animConvertSlot <- struct{}{}: + default: + return + } + conv := *item + conv.deferAnimated = nil + conv.open = func() (io.ReadCloser, error) { return io.NopCloser(bytes.NewReader(data)), nil } + key := conv.Key() + go func() { + defer func() { <-animConvertSlot }() + ctx, cancel := context.WithTimeout(context.Background(), animConvertTimeout) + defer cancel() + start := time.Now() + rc, err := conv.Reader(ctx) + if err != nil { + log.Warn(ctx, "Artwork: Could not convert animated image", "key", key, err) + return + } + out, err := io.ReadAll(rc) + _ = rc.Close() + if err != nil { + log.Warn(ctx, "Artwork: Could not convert animated image", "key", key, err) + return + } + // Converting before touching the cache keeps the entry from blocking readers for the whole conversion. + stream, err := s.cache.Get(ctx, &storedItem{key: key, data: out}) + if err != nil { + log.Warn(ctx, "Artwork: Could not cache animated image", "key", key, err) + return + } + defer stream.Close() + if _, err := io.Copy(io.Discard, stream); err != nil { + log.Debug(ctx, "Artwork: Could not cache animated image", "key", key, err) + return + } + log.Debug(ctx, "Artwork: Converted animated image", "key", key, "bytes", len(out), "elapsed", time.Since(start)) + }() +} + // serveHash serves the bytes of a found state row. A mismatch/open error is dangling, but a // cancelled request is not: it must not enqueue a re-resolution. func (s *service) serveHash(ctx context.Context, artID model.ArtworkID, ia *model.ItemArtwork, size int, square bool) (*Image, error) { diff --git a/core/artwork/artwork_test.go b/core/artwork/artwork_test.go index 907b300de..707eb3e89 100644 --- a/core/artwork/artwork_test.go +++ b/core/artwork/artwork_test.go @@ -3,10 +3,12 @@ package artwork import ( "bytes" "context" + "errors" "image" "io" "os" "path/filepath" + "sync" "time" "github.com/navidrome/navidrome/conf" @@ -422,6 +424,84 @@ var _ = Describe("Artwork", func() { }) }) + Describe("animated GIF", func() { + var ( + gifBytes []byte + fake *animFFmpeg + ) + + get := func(id string) *Image { + GinkgoHelper() + img, err := svc.Get(ctx, model.MustParseArtworkID(id), 100, false) + Expect(err).ToNot(HaveOccurred()) + return img + } + waitForConversions := func() { + GinkgoHelper() + // Taking the slot, not len(), gives the race detector a happens-before with the conversion. + Eventually(func() bool { + select { + case animConvertSlot <- struct{}{}: + <-animConvertSlot + return true + default: + return false + } + }).Should(BeTrue()) + } + + BeforeEach(func() { + gifBytes = createAnimatedGIF(3) + fake = &animFFmpeg{MockFFmpeg: tests.NewMockFFmpeg(""), out: []byte("animated-webp")} + svc = NewArtwork(ds, imgCache, store, fake) + seedFoundStore("al", "al1", gifBytes) + DeferCleanup(waitForConversions) + }) + + It("serves a transient stand-in, then the conversion from the cache", func() { + img := get("al-al1") + Expect(img.Transient).To(BeTrue()) + Expect(readAll(img)).To(Equal(gifBytes)) + + waitForConversions() + img = get("al-al1") + Expect(img.Transient).To(BeFalse()) + Expect(readAll(img)).To(Equal([]byte("animated-webp"))) + Expect(fake.calls.Load()).To(Equal(int32(1))) + }) + + It("runs one conversion at a time and does not queue the others", func() { + fake.release = make(chan struct{}) + release := sync.OnceFunc(func() { close(fake.release) }) + DeferCleanup(release) + seedFoundStore("al", "al2", createAnimatedGIF(4)) + + readAll(get("al-al1")) + Eventually(fake.calls.Load).Should(Equal(int32(1))) + img := get("al-al2") + Expect(img.Transient).To(BeTrue()) + readAll(img) + Consistently(fake.calls.Load, "100ms").Should(Equal(int32(1))) + + release() + waitForConversions() + img = get("al-al2") + Expect(img.Transient).To(BeTrue()) + readAll(img) + }) + + It("caches the static fallback when the conversion fails, so it is not retried", func() { + fake.err = errors.New("pipe:0: Input/output error") + readAll(get("al-al1")) + waitForConversions() + + img := get("al-al1") + Expect(img.Transient).To(BeFalse()) + Expect(readAll(img)).To(Equal(gifBytes)) + Expect(fake.calls.Load()).To(Equal(int32(1))) + }) + }) + Describe("GetOrPlaceholder", func() { It("accepts a raw entity id and serves its cover art", func() { albumRepo.SetData(model.Albums{{ID: "rawal", Name: "Album"}}) diff --git a/core/artwork/image_cache.go b/core/artwork/image_cache.go index b1970d21d..53eeb26f5 100644 --- a/core/artwork/image_cache.go +++ b/core/artwork/image_cache.go @@ -42,6 +42,8 @@ type resizedItem struct { square bool ffmpeg ffmpeg.FFmpeg open func() (io.ReadCloser, error) + // deferAnimated, when set, receives an animated GIF's bytes, and a transient static stand-in is served. + deferAnimated func(data []byte) } // Key is the ETag namespaced for the cache, so the validator a client holds and the entry it @@ -64,6 +66,14 @@ func (r *resizedItem) Reader(ctx context.Context) (io.ReadCloser, error) { if err != nil { return nil, err } + if r.deferAnimated != nil && isAnimatedGIF(data) && r.ffmpeg.IsAvailable() { + r.deferAnimated(data) + static, _, err := resizeStaticImage(data, r.size, r.square) + if err != nil || static == nil { + static = bytes.NewReader(data) + } + return cache.Transient(io.NopCloser(static)), nil + } 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. @@ -74,3 +84,15 @@ func (r *resizedItem) Reader(ctx context.Context) (io.ReadCloser, error) { } return io.NopCloser(resized), nil } + +// storedItem caches bytes produced outside the cache. +type storedItem struct { + key string + data []byte +} + +func (i *storedItem) Key() string { return i.key } + +func (i *storedItem) Reader(context.Context) (io.ReadCloser, error) { + return io.NopCloser(bytes.NewReader(i.data)), nil +} diff --git a/core/artwork/resize.go b/core/artwork/resize.go index 9b3ff46e5..f233f0586 100644 --- a/core/artwork/resize.go +++ b/core/artwork/resize.go @@ -3,6 +3,7 @@ package artwork import ( "bytes" "context" + "errors" "fmt" "image" "image/draw" @@ -48,9 +49,9 @@ func resizeImageData(ctx context.Context, ffm ffmpeg.FFmpeg, data []byte, size i if isAnimatedGIF(data) { if ffm.IsAvailable() { // Animated GIF: convert to animated WebP via ffmpeg (with optional resize) - r, err := ffm.ConvertAnimatedImage(ctx, bytes.NewReader(data), size, conf.Server.CoverArtQuality) + out, err := convertAnimatedGIF(ctx, ffm, data, size) if err == nil { - return r, 0, nil + return bytes.NewReader(out), 0, nil } log.Warn(ctx, "Artwork: Could not convert animated GIF, falling back to static", err) } @@ -62,6 +63,24 @@ func resizeImageData(ctx context.Context, ffm ffmpeg.FFmpeg, data []byte, size i return resizeStaticImage(data, size, square) } +// convertAnimatedGIF buffers the output because ffmpeg can fail after it starts streaming, +// too late for a caller holding the stream to fall back. +func convertAnimatedGIF(ctx context.Context, ffm ffmpeg.FFmpeg, data []byte, size int) ([]byte, error) { + r, err := ffm.ConvertAnimatedImage(ctx, bytes.NewReader(data), size, conf.Server.CoverArtQuality) + if err != nil { + return nil, err + } + defer r.Close() + out, err := io.ReadAll(r) + if err != nil { + return nil, err + } + if len(out) == 0 { + return nil, errors.New("ffmpeg produced no output") + } + return out, nil +} + // toFastScaleType converts types x/image/draw has no optimized scaler for (e.g. *image.NYCbCrA, // *image.Paletted) to *image.RGBA, avoiding CatmullRom.Scale's generic per-pixel fallback. func toFastScaleType(img image.Image) image.Image { diff --git a/core/artwork/resize_test.go b/core/artwork/resize_test.go index 76b95974d..d89ee0f3b 100644 --- a/core/artwork/resize_test.go +++ b/core/artwork/resize_test.go @@ -1,6 +1,17 @@ package artwork import ( + "bytes" + "context" + "errors" + "image" + "io" + "sync/atomic" + "testing/iotest" + + "github.com/navidrome/navidrome/conf" + "github.com/navidrome/navidrome/conf/configtest" + "github.com/navidrome/navidrome/tests" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" ) @@ -11,3 +22,64 @@ var _ = Describe("resizeStaticImage", func() { Expect(err).To(MatchError(ContainSubstring("exceed pixel cap"))) }) }) + +var _ = Describe("resizeImageData", func() { + var gifBytes []byte + + BeforeEach(func() { + DeferCleanup(configtest.SetupConfig()) + conf.Server.EnableWebPEncoding = false + gifBytes = createAnimatedGIF(3) + }) + + It("converts an animated GIF with ffmpeg", func() { + fake := &animFFmpeg{MockFFmpeg: tests.NewMockFFmpeg(""), out: []byte("animated-webp")} + r, _, err := resizeImageData(context.Background(), fake, gifBytes, 1, false) + Expect(err).ToNot(HaveOccurred()) + Expect(io.ReadAll(r)).To(Equal([]byte("animated-webp"))) + }) + + DescribeTable("falls back to a static resize when ffmpeg fails", + func(fake *animFFmpeg) { + fake.MockFFmpeg = tests.NewMockFFmpeg("") + r, _, err := resizeImageData(context.Background(), fake, gifBytes, 1, false) + Expect(err).ToNot(HaveOccurred()) + data, err := io.ReadAll(r) + Expect(err).ToNot(HaveOccurred()) + _, format, err := image.DecodeConfig(bytes.NewReader(data)) + Expect(err).ToNot(HaveOccurred()) + Expect(format).To(Equal("jpeg")) + }, + Entry("before producing output", &animFFmpeg{err: errors.New("no libwebp_anim")}), + Entry("mid-stream, like ffmpeg 5.1 reading a GIF from a pipe", &animFFmpeg{streamErr: errors.New("pipe:0: Input/output error")}), + Entry("with empty output", &animFFmpeg{}), + ) +}) + +// animFFmpeg fakes animated conversion; a non-nil release blocks it until closed. +type animFFmpeg struct { + *tests.MockFFmpeg + calls atomic.Int32 + release chan struct{} + out []byte + err error + streamErr error +} + +func (f *animFFmpeg) ConvertAnimatedImage(ctx context.Context, _ io.Reader, _ int, _ int) (io.ReadCloser, error) { + f.calls.Add(1) + if f.release != nil { + select { + case <-f.release: + case <-ctx.Done(): + return nil, ctx.Err() + } + } + if f.err != nil { + return nil, f.err + } + if f.streamErr != nil { + return io.NopCloser(iotest.ErrReader(f.streamErr)), nil //nolint:nilerr // the stream fails, not the call + } + return io.NopCloser(bytes.NewReader(f.out)), nil +} diff --git a/server/imghttp/headers.go b/server/imghttp/headers.go index 9308bcdab..561da34b2 100644 --- a/server/imghttp/headers.go +++ b/server/imghttp/headers.go @@ -13,8 +13,8 @@ import ( // (the caller must then not write a body). requestedHash is the hash the client asserted, or "". func WriteImageHeaders(w http.ResponseWriter, r *http.Request, img *artwork.Image, requestedHash string) (wrote304 bool) { h := w.Header() - // Placeholders are transient stand-ins for unresolved art: never cached, no validators. - if img.Placeholder { + // Placeholders and transient stand-ins must not outlive what they stand in for: never cached, no validators. + if img.Placeholder || img.Transient { h.Set("Cache-Control", "no-store") return false } diff --git a/server/imghttp/headers_test.go b/server/imghttp/headers_test.go index 2e8313faf..ec06f84d6 100644 --- a/server/imghttp/headers_test.go +++ b/server/imghttp/headers_test.go @@ -41,6 +41,12 @@ func resized() *artwork.Image { } } +func transient() *artwork.Image { + img := resized() + img.Transient = true + return img +} + func unvalidated() *artwork.Image { return &artwork.Image{ReadCloser: io.NopCloser(strings.NewReader("IMG")), LastUpdated: lastMod} } @@ -105,6 +111,9 @@ var _ = Describe("WriteImageHeaders", func() { testCase{img: found(), ifNoneMatch: `"deadbeefdeadbeef"`, want304: false, wantCache: "public, no-cache", wantETag: `"` + testHash + `"`, wantLastMod: true}), Entry("placeholder ignores If-None-Match and never 304s", testCase{img: placeholder(), ifNoneMatch: "*", want304: false, wantCache: "no-store"}), + // A stand-in shares the final representation's ETag inputs, so a validator would pin it. + Entry("transient stand-in is never cached and carries no validators", + testCase{img: transient(), ifNoneMatch: `"` + testRepTag + `"`, want304: false, wantCache: "no-store"}), // With no validator, an emitted ETag would be the same empty tag on every such response, // and matching it would 304 changed bytes. diff --git a/utils/cache/file_caches.go b/utils/cache/file_caches.go index 48cd135cb..90764f326 100644 --- a/utils/cache/file_caches.go +++ b/utils/cache/file_caches.go @@ -25,6 +25,18 @@ type Item interface { Key() string } +// Transient wraps a ReadFunc result that Get must serve but never store. +func Transient(rc io.ReadCloser) io.ReadCloser { return transientReader{rc} } + +type transientReader struct{ io.ReadCloser } + +func unwrapTransient(r io.Reader) (io.Reader, bool) { + if t, ok := r.(transientReader); ok { + return t.ReadCloser, true + } + return r, false +} + // ReadFunc is a function that retrieves the data to be cached. It receives the Item to be cached and returns // an io.Reader with the data and an error. type ReadFunc func(ctx context.Context, item Item) (io.Reader, error) @@ -155,7 +167,8 @@ func (fc *fileCache) Get(ctx context.Context, arg Item) (*CachedStream, error) { if err != nil { return nil, err } - return &CachedStream{Reader: reader}, nil + reader, transient := unwrapTransient(reader) + return &CachedStream{Reader: reader, Transient: transient}, nil } key := arg.Key() @@ -182,6 +195,10 @@ func (fc *fileCache) Get(ctx context.Context, arg Item) (*CachedStream, error) { _ = fc.invalidate(ctx, key) return nil, err } + if reader, transient := unwrapTransient(reader); transient { + fc.discard(ctx, key, r, w) + return &CachedStream{Reader: reader, Transient: true}, nil + } go func() { if err := fc.copyAndClose(ctx, key, w, reader); err != nil { log.Debug(ctx, "Error storing file in cache", "cache", fc.name, "key", key, err) @@ -213,12 +230,27 @@ func (fc *fileCache) Get(ctx context.Context, arg Item) (*CachedStream, error) { return &CachedStream{Reader: r, Cached: cached}, nil } +// discard drops a miss entry that will never be written. Cancelling first makes the removal +// non-blocking, so the key is free when Get returns; readers that joined meanwhile fail. +func (fc *fileCache) discard(ctx context.Context, key string, r io.Closer, w io.WriteCloser) { + _ = r.Close() + if cw, ok := w.(interface{ CloseWithError(error) error }); ok { + _ = cw.CloseWithError(errTransientEntry) + } else { + _ = w.Close() + } + _ = fc.invalidate(ctx, key) +} + +var errTransientEntry = errors.New("cache entry was served without being stored") + // CachedStream is a wrapper around an io.ReadCloser that allows reading from a cache. type CachedStream struct { io.Reader io.Seeker io.Closer - Cached bool + Cached bool + Transient bool } func (s *CachedStream) Close() error { diff --git a/utils/cache/file_caches_test.go b/utils/cache/file_caches_test.go index 3189de6b2..70cd2a0dc 100644 --- a/utils/cache/file_caches_test.go +++ b/utils/cache/file_caches_test.go @@ -106,6 +106,67 @@ var _ = Describe("File Caches", func() { Expect(called).To(BeTrue()) }) + It("serves a transient result without storing it", func() { + var calls atomic.Int32 + fc := callNewFileCache("test", "1KB", "test", 0, func(ctx context.Context, arg Item) (io.Reader, error) { + calls.Add(1) + return Transient(io.NopCloser(strings.NewReader("stand-in"))), nil + }) + + for range 2 { + s, err := fc.Get(context.Background(), &testArg{"transient"}) + Expect(err).ToNot(HaveOccurred()) + Expect(s.Transient).To(BeTrue()) + Expect(s.Cached).To(BeFalse()) + Expect(io.ReadAll(s)).To(Equal([]byte("stand-in"))) + Expect(s.Close()).To(Succeed()) + } + Expect(calls.Load()).To(Equal(int32(2))) + + dataPath := fcSpreadFS(fc).KeyMapper((&testArg{"transient"}).Key()) + _, statErr := os.Stat(dataPath + ".complete") + Expect(os.IsNotExist(statErr)).To(BeTrue()) + }) + + It("stores the next result once a transient one was served", func() { + var calls atomic.Int32 + fc := callNewFileCache("test", "1KB", "test", 0, func(ctx context.Context, arg Item) (io.Reader, error) { + if calls.Add(1) == 1 { + return Transient(io.NopCloser(strings.NewReader("stand-in"))), nil + } + return strings.NewReader("real"), nil + }) + + s, err := fc.Get(context.Background(), &testArg{"k"}) + Expect(err).ToNot(HaveOccurred()) + Expect(io.ReadAll(s)).To(Equal([]byte("stand-in"))) + _ = s.Close() + + // The transient entry is gone when Get returns, so this is a MISS that stores. + s, err = fc.Get(context.Background(), &testArg{"k"}) + Expect(err).ToNot(HaveOccurred()) + Expect(s.Transient).To(BeFalse()) + Expect(io.ReadAll(s)).To(Equal([]byte("real"))) + _ = s.Close() + + Eventually(func() bool { + s, _ := fc.Get(context.Background(), &testArg{"k"}) + defer s.Close() + return s.Cached + }).Should(BeTrue()) + Expect(calls.Load()).To(Equal(int32(2))) + }) + + It("reports a transient result when the cache is disabled", func() { + fc := callNewFileCache("test", "0", "test", 0, func(ctx context.Context, arg Item) (io.Reader, error) { + return Transient(io.NopCloser(strings.NewReader("stand-in"))), nil + }) + s, err := fc.Get(context.Background(), &testArg{"k"}) + Expect(err).ToNot(HaveOccurred()) + Expect(s.Transient).To(BeTrue()) + Expect(io.ReadAll(s)).To(Equal([]byte("stand-in"))) + }) + It("writes a completion marker after a successful cache write", func() { fc := callNewFileCache("test", "1KB", "test", 0, func(ctx context.Context, arg Item) (io.Reader, error) { return strings.NewReader("complete-data"), nil From c0da9fcd93ac8fd4381adeac01450d7cb6a2a55b Mon Sep 17 00:00:00 2001 From: Deluan Date: Mon, 14 Sep 2026 20:14:13 -0400 Subject: [PATCH 2/4] refactor(artwork): defer animated GIF conversion via a loader error Replace cache.Transient with an artwork-local *deferredAnimation error returned by resizedItem.Reader. The file cache already stores nothing when its loader fails, so the generic cache no longer needs a transient mode. The error carries the GIF bytes and the static stand-in, replacing the deferAnimated callback. The cache's loader-error path now cancels the entry writer, so readers that joined the miss fail with the loader's error instead of reading an empty entry, and the removal no longer blocks on them. The background job resizes the bytes it already holds instead of re-reading them, orOriginal centralizes the serve-the-original fallback, and warmCache shares the store-and-drain step with the worker's precache. --- core/artwork/artwork.go | 41 +++++-------------- core/artwork/artwork_test.go | 2 +- core/artwork/image_cache.go | 43 ++++++++++++++------ core/artwork/resize_test.go | 12 +++--- core/artwork/worker.go | 5 +-- utils/cache/file_caches.go | 43 ++++---------------- utils/cache/file_caches_test.go | 71 ++++++++------------------------- 7 files changed, 72 insertions(+), 145 deletions(-) diff --git a/core/artwork/artwork.go b/core/artwork/artwork.go index 1da419dff..fb67917b5 100644 --- a/core/artwork/artwork.go +++ b/core/artwork/artwork.go @@ -153,20 +153,17 @@ func (s *service) serveSource(ctx context.Context, key, hash string, lastUpdate } return img, nil } - item := &resizedItem{hash: key, size: size, square: square, ffmpeg: s.ffmpeg, open: open} - var animated []byte // Without a cache a background conversion could never be served, so convert inline instead. - if s.cache.Available(ctx) { - item.deferAnimated = func(data []byte) { animated = data } - } + item := &resizedItem{hash: key, size: size, square: square, ffmpeg: s.ffmpeg, open: open, + deferAnimated: s.cache.Available(ctx)} stream, err := s.cache.Get(ctx, item) + if deferred, ok := errors.AsType[*deferredAnimation](err); ok { + s.convertInBackground(item, deferred.data) + return &Image{ReadCloser: io.NopCloser(deferred.standIn), Hash: hash, LastUpdated: lastUpdate, Transient: true}, nil + } if err != nil { return nil, err } - if stream.Transient { - s.convertInBackground(item, animated) - return &Image{ReadCloser: stream, Hash: hash, LastUpdated: lastUpdate, Transient: true}, nil - } return &Image{ReadCloser: stream, Hash: hash, ETag: representationTag(key, size, square), LastUpdated: lastUpdate}, nil } @@ -184,37 +181,19 @@ func (s *service) convertInBackground(item *resizedItem, data []byte) { default: return } - conv := *item - conv.deferAnimated = nil - conv.open = func() (io.ReadCloser, error) { return io.NopCloser(bytes.NewReader(data)), nil } - key := conv.Key() + key, size, square := item.Key(), item.size, item.square go func() { defer func() { <-animConvertSlot }() ctx, cancel := context.WithTimeout(context.Background(), animConvertTimeout) defer cancel() start := time.Now() - rc, err := conv.Reader(ctx) - if err != nil { - log.Warn(ctx, "Artwork: Could not convert animated image", "key", key, err) - return - } - out, err := io.ReadAll(rc) - _ = rc.Close() - if err != nil { - log.Warn(ctx, "Artwork: Could not convert animated image", "key", key, err) - return - } + resized, _, err := resizeImageData(ctx, s.ffmpeg, data, size, square) + out, _ := io.ReadAll(orOriginal(data, resized, err)) // Converting before touching the cache keeps the entry from blocking readers for the whole conversion. - stream, err := s.cache.Get(ctx, &storedItem{key: key, data: out}) - if err != nil { + if err := warmCache(ctx, s.cache, &storedItem{key: key, data: out}); err != nil { log.Warn(ctx, "Artwork: Could not cache animated image", "key", key, err) return } - defer stream.Close() - if _, err := io.Copy(io.Discard, stream); err != nil { - log.Debug(ctx, "Artwork: Could not cache animated image", "key", key, err) - return - } log.Debug(ctx, "Artwork: Converted animated image", "key", key, "bytes", len(out), "elapsed", time.Since(start)) }() } diff --git a/core/artwork/artwork_test.go b/core/artwork/artwork_test.go index 707eb3e89..bbbcdf43d 100644 --- a/core/artwork/artwork_test.go +++ b/core/artwork/artwork_test.go @@ -491,7 +491,7 @@ var _ = Describe("Artwork", func() { }) It("caches the static fallback when the conversion fails, so it is not retried", func() { - fake.err = errors.New("pipe:0: Input/output error") + fake.Error = errors.New("pipe:0: Input/output error") readAll(get("al-al1")) waitForConversions() diff --git a/core/artwork/image_cache.go b/core/artwork/image_cache.go index 53eeb26f5..7d58b56f1 100644 --- a/core/artwork/image_cache.go +++ b/core/artwork/image_cache.go @@ -42,8 +42,8 @@ type resizedItem struct { square bool ffmpeg ffmpeg.FFmpeg open func() (io.ReadCloser, error) - // deferAnimated, when set, receives an animated GIF's bytes, and a transient static stand-in is served. - deferAnimated func(data []byte) + // deferAnimated makes Reader return a *deferredAnimation for animated GIFs instead of converting inline. + deferAnimated bool } // Key is the ETag namespaced for the cache, so the validator a client holds and the entry it @@ -66,23 +66,40 @@ func (r *resizedItem) Reader(ctx context.Context) (io.ReadCloser, error) { if err != nil { return nil, err } - if r.deferAnimated != nil && isAnimatedGIF(data) && r.ffmpeg.IsAvailable() { - r.deferAnimated(data) + if r.deferAnimated && isAnimatedGIF(data) && r.ffmpeg.IsAvailable() { static, _, err := resizeStaticImage(data, r.size, r.square) - if err != nil || static == nil { - static = bytes.NewReader(data) - } - return cache.Transient(io.NopCloser(static)), nil + return nil, &deferredAnimation{data: data, standIn: orOriginal(data, static, err)} } resized, _, err := resizeImageData(ctx, r.ffmpeg, data, r.size, r.square) + return io.NopCloser(orOriginal(data, resized, err)), nil +} + +// orOriginal serves the original bytes when the resize failed or the image was already within bounds. +func orOriginal(data []byte, resized io.Reader, err error) io.Reader { if err != nil || resized == nil { - // Resize failed or image already within bounds: serve the original bytes. - return io.NopCloser(bytes.NewReader(data)), nil + return bytes.NewReader(data) } - if rc, ok := resized.(io.ReadCloser); ok { - return rc, nil + return resized +} + +// deferredAnimation is returned as an error so the cache stores nothing: it carries the GIF bytes +// to convert in the background and a static stand-in to serve meanwhile. +type deferredAnimation struct { + data []byte + standIn io.Reader +} + +func (*deferredAnimation) Error() string { return "animated image conversion deferred" } + +// warmCache stores item in c, returning once the entry is written. +func warmCache(ctx context.Context, c cache.FileCache, item cache.Item) error { + stream, err := c.Get(ctx, item) + if err != nil { + return err } - return io.NopCloser(resized), nil + defer stream.Close() + _, err = io.Copy(io.Discard, stream) + return err } // storedItem caches bytes produced outside the cache. diff --git a/core/artwork/resize_test.go b/core/artwork/resize_test.go index d89ee0f3b..73463f901 100644 --- a/core/artwork/resize_test.go +++ b/core/artwork/resize_test.go @@ -41,7 +41,6 @@ var _ = Describe("resizeImageData", func() { DescribeTable("falls back to a static resize when ffmpeg fails", func(fake *animFFmpeg) { - fake.MockFFmpeg = tests.NewMockFFmpeg("") r, _, err := resizeImageData(context.Background(), fake, gifBytes, 1, false) Expect(err).ToNot(HaveOccurred()) data, err := io.ReadAll(r) @@ -50,9 +49,9 @@ var _ = Describe("resizeImageData", func() { Expect(err).ToNot(HaveOccurred()) Expect(format).To(Equal("jpeg")) }, - Entry("before producing output", &animFFmpeg{err: errors.New("no libwebp_anim")}), - Entry("mid-stream, like ffmpeg 5.1 reading a GIF from a pipe", &animFFmpeg{streamErr: errors.New("pipe:0: Input/output error")}), - Entry("with empty output", &animFFmpeg{}), + Entry("before producing output", &animFFmpeg{MockFFmpeg: &tests.MockFFmpeg{Error: errors.New("no libwebp_anim")}}), + Entry("mid-stream, like ffmpeg 5.1 reading a GIF from a pipe", &animFFmpeg{MockFFmpeg: tests.NewMockFFmpeg(""), streamErr: errors.New("pipe:0: Input/output error")}), + Entry("with empty output", &animFFmpeg{MockFFmpeg: tests.NewMockFFmpeg("")}), ) }) @@ -62,7 +61,6 @@ type animFFmpeg struct { calls atomic.Int32 release chan struct{} out []byte - err error streamErr error } @@ -75,8 +73,8 @@ func (f *animFFmpeg) ConvertAnimatedImage(ctx context.Context, _ io.Reader, _ in return nil, ctx.Err() } } - if f.err != nil { - return nil, f.err + if f.Error != nil { + return nil, f.Error } if f.streamErr != nil { return io.NopCloser(iotest.ErrReader(f.streamErr)), nil //nolint:nilerr // the stream fails, not the call diff --git a/core/artwork/worker.go b/core/artwork/worker.go index bb09be55e..3c76e3c72 100644 --- a/core/artwork/worker.go +++ b/core/artwork/worker.go @@ -310,13 +310,10 @@ func (w *Worker) precache(ctx context.Context, got *acquired) { ffmpeg: w.ffmpeg, open: func() (io.ReadCloser, error) { return io.NopCloser(bytes.NewReader(got.data)), nil }, } - stream, err := w.cache.Get(ctx, item) - if err != nil { + if err := warmCache(ctx, w.cache, item); err != nil { log.Debug(ctx, "Artwork: Precache failed", "kind", got.ia.ItemKind, "id", got.ia.ItemID, err) return } - _, _ = io.Copy(io.Discard, stream) - _ = stream.Close() log.Trace(ctx, "Artwork: Precached UI size", "kind", got.ia.ItemKind, "id", got.ia.ItemID, "size", conf.Server.UICoverArtSize, "elapsed", time.Since(precacheStart)) } diff --git a/utils/cache/file_caches.go b/utils/cache/file_caches.go index 90764f326..91cf87f10 100644 --- a/utils/cache/file_caches.go +++ b/utils/cache/file_caches.go @@ -25,18 +25,6 @@ type Item interface { Key() string } -// Transient wraps a ReadFunc result that Get must serve but never store. -func Transient(rc io.ReadCloser) io.ReadCloser { return transientReader{rc} } - -type transientReader struct{ io.ReadCloser } - -func unwrapTransient(r io.Reader) (io.Reader, bool) { - if t, ok := r.(transientReader); ok { - return t.ReadCloser, true - } - return r, false -} - // ReadFunc is a function that retrieves the data to be cached. It receives the Item to be cached and returns // an io.Reader with the data and an error. type ReadFunc func(ctx context.Context, item Item) (io.Reader, error) @@ -167,8 +155,7 @@ func (fc *fileCache) Get(ctx context.Context, arg Item) (*CachedStream, error) { if err != nil { return nil, err } - reader, transient := unwrapTransient(reader) - return &CachedStream{Reader: reader, Transient: transient}, nil + return &CachedStream{Reader: reader}, nil } key := arg.Key() @@ -191,14 +178,15 @@ func (fc *fileCache) Get(ctx context.Context, arg Item) (*CachedStream, error) { reader, err := fc.getReader(ctx, arg) if err != nil { _ = r.Close() - _ = w.Close() + // Cancelling fails readers that joined meanwhile with err, and keeps the removal from blocking on them. + if cw, ok := w.(interface{ CloseWithError(error) error }); ok { + _ = cw.CloseWithError(err) + } else { + _ = w.Close() + } _ = fc.invalidate(ctx, key) return nil, err } - if reader, transient := unwrapTransient(reader); transient { - fc.discard(ctx, key, r, w) - return &CachedStream{Reader: reader, Transient: true}, nil - } go func() { if err := fc.copyAndClose(ctx, key, w, reader); err != nil { log.Debug(ctx, "Error storing file in cache", "cache", fc.name, "key", key, err) @@ -230,27 +218,12 @@ func (fc *fileCache) Get(ctx context.Context, arg Item) (*CachedStream, error) { return &CachedStream{Reader: r, Cached: cached}, nil } -// discard drops a miss entry that will never be written. Cancelling first makes the removal -// non-blocking, so the key is free when Get returns; readers that joined meanwhile fail. -func (fc *fileCache) discard(ctx context.Context, key string, r io.Closer, w io.WriteCloser) { - _ = r.Close() - if cw, ok := w.(interface{ CloseWithError(error) error }); ok { - _ = cw.CloseWithError(errTransientEntry) - } else { - _ = w.Close() - } - _ = fc.invalidate(ctx, key) -} - -var errTransientEntry = errors.New("cache entry was served without being stored") - // CachedStream is a wrapper around an io.ReadCloser that allows reading from a cache. type CachedStream struct { io.Reader io.Seeker io.Closer - Cached bool - Transient bool + Cached bool } func (s *CachedStream) Close() error { diff --git a/utils/cache/file_caches_test.go b/utils/cache/file_caches_test.go index 70cd2a0dc..5547db736 100644 --- a/utils/cache/file_caches_test.go +++ b/utils/cache/file_caches_test.go @@ -106,65 +106,28 @@ var _ = Describe("File Caches", func() { Expect(called).To(BeTrue()) }) - It("serves a transient result without storing it", func() { - var calls atomic.Int32 + It("fails a reader that joined a miss whose loader failed, without blocking the removal", func() { + entered, release := make(chan struct{}), make(chan struct{}) fc := callNewFileCache("test", "1KB", "test", 0, func(ctx context.Context, arg Item) (io.Reader, error) { - calls.Add(1) - return Transient(io.NopCloser(strings.NewReader("stand-in"))), nil + close(entered) + <-release + return nil, errors.New("loader failed") }) + firstErr := make(chan error, 1) + go func() { + _, err := fc.Get(context.Background(), &testArg{"k"}) + firstErr <- err + }() + <-entered - for range 2 { - s, err := fc.Get(context.Background(), &testArg{"transient"}) - Expect(err).ToNot(HaveOccurred()) - Expect(s.Transient).To(BeTrue()) - Expect(s.Cached).To(BeFalse()) - Expect(io.ReadAll(s)).To(Equal([]byte("stand-in"))) - Expect(s.Close()).To(Succeed()) - } - Expect(calls.Load()).To(Equal(int32(2))) - - dataPath := fcSpreadFS(fc).KeyMapper((&testArg{"transient"}).Key()) - _, statErr := os.Stat(dataPath + ".complete") - Expect(os.IsNotExist(statErr)).To(BeTrue()) - }) - - It("stores the next result once a transient one was served", func() { - var calls atomic.Int32 - fc := callNewFileCache("test", "1KB", "test", 0, func(ctx context.Context, arg Item) (io.Reader, error) { - if calls.Add(1) == 1 { - return Transient(io.NopCloser(strings.NewReader("stand-in"))), nil - } - return strings.NewReader("real"), nil - }) - - s, err := fc.Get(context.Background(), &testArg{"k"}) + joined, err := fc.Get(context.Background(), &testArg{"k"}) Expect(err).ToNot(HaveOccurred()) - Expect(io.ReadAll(s)).To(Equal([]byte("stand-in"))) - _ = s.Close() + close(release) + Eventually(firstErr).Should(Receive(MatchError("loader failed"))) - // The transient entry is gone when Get returns, so this is a MISS that stores. - s, err = fc.Get(context.Background(), &testArg{"k"}) - Expect(err).ToNot(HaveOccurred()) - Expect(s.Transient).To(BeFalse()) - Expect(io.ReadAll(s)).To(Equal([]byte("real"))) - _ = s.Close() - - Eventually(func() bool { - s, _ := fc.Get(context.Background(), &testArg{"k"}) - defer s.Close() - return s.Cached - }).Should(BeTrue()) - Expect(calls.Load()).To(Equal(int32(2))) - }) - - It("reports a transient result when the cache is disabled", func() { - fc := callNewFileCache("test", "0", "test", 0, func(ctx context.Context, arg Item) (io.Reader, error) { - return Transient(io.NopCloser(strings.NewReader("stand-in"))), nil - }) - s, err := fc.Get(context.Background(), &testArg{"k"}) - Expect(err).ToNot(HaveOccurred()) - Expect(s.Transient).To(BeTrue()) - Expect(io.ReadAll(s)).To(Equal([]byte("stand-in"))) + _, err = io.ReadAll(joined) + Expect(err).To(MatchError("loader failed")) + _ = joined.Close() }) It("writes a completion marker after a successful cache write", func() { From 2cc2ab116067e26208f0e766607fef6977b617fb Mon Sep 17 00:00:00 2001 From: Deluan Date: Mon, 14 Sep 2026 21:31:14 -0400 Subject: [PATCH 3/4] fix(artwork): reload animated covers in the UI once converted A resized animated GIF is first served as a static stand-in while the conversion runs in the background. The converted image is cached under the same URL, since the cover hash identifies the original upload, so the UI kept painting the stand-in from its blob cache until a full page reload. When a background conversion is stored, the artwork service now broadcasts a refreshResource event for the item (the service receives the event broker via wire). In the UI, useArtworkRefresh returns when the last event named a record, ignoring wildcards so scans do not refetch every cover, and useImageUrl takes that as a version: a changed version re-fetches the cached URL, keeps showing the old blob until the new one arrives, and keeps it if the re-fetch fails. --- cmd/wire_gen.go | 11 +-- core/artwork/artwork.go | 28 ++++--- core/artwork/artwork_test.go | 16 +++- core/artwork/e2e/acquire_serve_test.go | 2 +- core/artwork/e2e/resolution_harness_test.go | 2 +- server/subsonic/e2e/subsonic_artwork_test.go | 2 +- ui/src/common/Artwork.jsx | 4 +- ui/src/common/Artwork.test.jsx | 10 +++ ui/src/common/useArtworkRefresh.js | 23 ++++++ ui/src/common/useArtworkRefresh.test.js | 44 ++++++++++ ui/src/common/useImageUrl.js | 30 ++++--- ui/src/common/useImageUrl.test.js | 86 ++++++++++++++++++++ 12 files changed, 226 insertions(+), 32 deletions(-) create mode 100644 ui/src/common/useArtworkRefresh.js create mode 100644 ui/src/common/useArtworkRefresh.test.js diff --git a/cmd/wire_gen.go b/cmd/wire_gen.go index fd04c44c5..6449e46fd 100644 --- a/cmd/wire_gen.go +++ b/cmd/wire_gen.go @@ -89,13 +89,13 @@ func CreateSubsonicAPIRouter(ctx context.Context) *subsonic.Router { fileCache := artwork.GetImageCache() imageStore := artwork.GetImageStore() fFmpeg := ffmpeg.New() - artworkArtwork := artwork.NewArtwork(dataStore, fileCache, imageStore, fFmpeg) + broker := events.GetBroker() + artworkArtwork := artwork.NewArtwork(dataStore, fileCache, imageStore, fFmpeg, broker) transcodingCache := stream.GetTranscodingCache() mediaStreamer := stream.NewMediaStreamer(dataStore, fFmpeg, transcodingCache) share := core.NewShare(dataStore) archiver := core.NewArchiver(mediaStreamer, dataStore, share) players := core.NewPlayers(dataStore) - broker := events.GetBroker() metricsMetrics := metrics.GetPrometheusInstance(dataStore) manager := plugins.GetManager(dataStore, broker, metricsMetrics) agentsAgents := agents.GetAgents(dataStore, manager) @@ -119,12 +119,12 @@ func CreateJellyfinAPIRouter(ctx context.Context) *jellyfin.Router { fileCache := artwork.GetImageCache() imageStore := artwork.GetImageStore() fFmpeg := ffmpeg.New() - artworkArtwork := artwork.NewArtwork(dataStore, fileCache, imageStore, fFmpeg) + broker := events.GetBroker() + artworkArtwork := artwork.NewArtwork(dataStore, fileCache, imageStore, fFmpeg, broker) transcodingCache := stream.GetTranscodingCache() mediaStreamer := stream.NewMediaStreamer(dataStore, fFmpeg, transcodingCache) transcodeDecider := stream.NewTranscodeDecider(dataStore, fFmpeg) players := core.NewPlayers(dataStore) - broker := events.GetBroker() metricsMetrics := metrics.GetPrometheusInstance(dataStore) manager := plugins.GetManager(dataStore, broker, metricsMetrics) playTracker := scrobbler.GetPlayTracker(dataStore, broker, manager) @@ -145,7 +145,8 @@ func CreatePublicRouter() *public.Router { fileCache := artwork.GetImageCache() imageStore := artwork.GetImageStore() fFmpeg := ffmpeg.New() - artworkArtwork := artwork.NewArtwork(dataStore, fileCache, imageStore, fFmpeg) + broker := events.GetBroker() + artworkArtwork := artwork.NewArtwork(dataStore, fileCache, imageStore, fFmpeg, broker) transcodingCache := stream.GetTranscodingCache() mediaStreamer := stream.NewMediaStreamer(dataStore, fFmpeg, transcodingCache) share := core.NewShare(dataStore) diff --git a/core/artwork/artwork.go b/core/artwork/artwork.go index fb67917b5..38fdef783 100644 --- a/core/artwork/artwork.go +++ b/core/artwork/artwork.go @@ -16,6 +16,7 @@ import ( "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/resources" + "github.com/navidrome/navidrome/server/events" "github.com/navidrome/navidrome/utils/cache" ) @@ -49,8 +50,8 @@ type Artwork interface { GetOrPlaceholder(ctx context.Context, id string, size int, square bool) (*Image, error) } -func NewArtwork(ds model.DataStore, cache cache.FileCache, store *ImageStore, ffm ffmpeg.FFmpeg) Artwork { - return &service{ds: ds, cache: cache, store: store, ffmpeg: ffm} +func NewArtwork(ds model.DataStore, cache cache.FileCache, store *ImageStore, ffm ffmpeg.FFmpeg, broker events.Broker) Artwork { + return &service{ds: ds, cache: cache, store: store, ffmpeg: ffm, broker: broker} } // entityExists reports whether the entity an artwork id points at is still there: state rows @@ -86,6 +87,7 @@ type service struct { cache cache.FileCache store *ImageStore ffmpeg ffmpeg.FFmpeg + broker events.Broker } func (s *service) GetOrPlaceholder(ctx context.Context, id string, size int, square bool) (*Image, error) { @@ -136,7 +138,7 @@ func (s *service) serveEntity(ctx context.Context, artID model.ArtworkID, size i // serveSource is the one place bytes become an Image. hash is the pixel identity ("" for disc art) // and doubles as the full-size validator, so an ETag is only needed when resized or hash is "". -func (s *service) serveSource(ctx context.Context, key, hash string, lastUpdate time.Time, +func (s *service) serveSource(ctx context.Context, artID model.ArtworkID, key, hash string, lastUpdate time.Time, size int, square bool, open func() (io.ReadCloser, error), ) (*Image, error) { if size == 0 && !square { @@ -158,7 +160,7 @@ func (s *service) serveSource(ctx context.Context, key, hash string, lastUpdate deferAnimated: s.cache.Available(ctx)} stream, err := s.cache.Get(ctx, item) if deferred, ok := errors.AsType[*deferredAnimation](err); ok { - s.convertInBackground(item, deferred.data) + s.convertInBackground(artID, item, deferred.data) return &Image{ReadCloser: io.NopCloser(deferred.standIn), Hash: hash, LastUpdated: lastUpdate, Transient: true}, nil } if err != nil { @@ -175,7 +177,7 @@ const animConvertTimeout = time.Minute // convertInBackground caches item's conversion of data. When the slot is busy it does nothing, // and a later request for the key tries again. -func (s *service) convertInBackground(item *resizedItem, data []byte) { +func (s *service) convertInBackground(artID model.ArtworkID, item *resizedItem, data []byte) { select { case animConvertSlot <- struct{}{}: default: @@ -195,6 +197,10 @@ func (s *service) convertInBackground(item *resizedItem, data []byte) { return } log.Debug(ctx, "Artwork: Converted animated image", "key", key, "bytes", len(out), "elapsed", time.Since(start)) + // The image URL is unchanged, so UIs holding the stand-in only reload it when told. + if res, ok := artworkKindToResource[artID.Kind]; ok { + s.broker.SendBroadcastMessage(ctx, (&events.RefreshResource{}).With(res, artID.ID)) + } }() } @@ -212,7 +218,7 @@ func (s *service) serveHash(ctx context.Context, artID model.ArtworkID, ia *mode } return nil, err } - img, err := s.serveSource(ctx, ia.Hash, ia.Hash, ia.UpdatedAt, size, square, + img, err := s.serveSource(ctx, artID, ia.Hash, ia.Hash, ia.UpdatedAt, size, square, func() (io.ReadCloser, error) { return openOriginal(ia, art.Mime, s.store) }) if err != nil { if errors.Is(err, context.Canceled) { @@ -270,11 +276,11 @@ func (s *service) provisional(ctx context.Context, artID model.ArtworkID, size i s.enqueue(ctx, artID, model.ArtworkPriorityBump) log.Debug(ctx, "Artwork: Provisional read-through, no state row yet", "artID", artID, "source", res.source, "hit", res.reader != nil) - return s.serveResolution(ctx, res, size, square) + return s.serveResolution(ctx, artID, res, size, square) } // serveResolution turns a local resolution's bytes into a servable Image (byte-hash only, no decode). -func (s *service) serveResolution(ctx context.Context, res resolution, size int, square bool) (*Image, error) { +func (s *service) serveResolution(ctx context.Context, artID model.ArtworkID, res resolution, size int, square bool) (*Image, error) { if res.reader == nil { return nil, ErrUnavailable } @@ -288,7 +294,7 @@ func (s *service) serveResolution(ctx context.Context, res resolution, size int, return nil, ErrUnavailable } // Keyed by the byte-hash, so the entry lines up with the worker's eventual store entry. - return s.serveSource(ctx, hash, hash, unixMtime(res.refMtime), size, square, + return s.serveSource(ctx, artID, hash, hash, unixMtime(res.refMtime), size, square, func() (io.ReadCloser, error) { return io.NopCloser(bytes.NewReader(data)), nil }) } @@ -338,7 +344,7 @@ func (s *service) provisionalEmbedded(ctx context.Context, artID model.ArtworkID // Eligible but unextractable: fall back the way CoverArtID does, not to a placeholder. return s.Get(ctx, mf.DiscCoverArtID(), size, square) } - return s.serveResolution(ctx, res, size, square) + return s.serveResolution(ctx, artID, res, size, square) } // serveDisc reads disc art through with no state row and no enqueue, falling back to the album cover. @@ -356,7 +362,7 @@ func (s *service) serveDisc(ctx context.Context, artID model.ArtworkID, size int // Disc art has no state row, hence no content hash: keying on id, album mtime and // DiscArtPriority lets a warm cache answer without running the chain or touching the disk. key := fmt.Sprintf("%s|%d|%s", artID.ID, dr.cacheTime().UnixNano(), conf.Server.DiscArtPriority) - img, err := s.serveSource(ctx, key, "", dr.cacheTime(), size, square, selectImage) + img, err := s.serveSource(ctx, artID, key, "", dr.cacheTime(), size, square, selectImage) if err != nil { if errors.Is(err, context.Canceled) { return nil, err diff --git a/core/artwork/artwork_test.go b/core/artwork/artwork_test.go index bbbcdf43d..af6e75214 100644 --- a/core/artwork/artwork_test.go +++ b/core/artwork/artwork_test.go @@ -16,6 +16,7 @@ import ( "github.com/navidrome/navidrome/consts" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/resources" + "github.com/navidrome/navidrome/server/events" "github.com/navidrome/navidrome/tests" "github.com/navidrome/navidrome/utils/cache" . "github.com/onsi/ginkgo/v2" @@ -109,7 +110,7 @@ var _ = Describe("Artwork", func() { return arg.(artworkReader).Reader(ctx) }) Eventually(func() bool { return imgCache.Available(ctx) }, 10*time.Second).Should(BeTrue()) - svc = NewArtwork(ds, imgCache, store, ffm) + svc = NewArtwork(ds, imgCache, store, ffm, events.NoopBroker()) }) Describe("found state", func() { @@ -428,6 +429,7 @@ var _ = Describe("Artwork", func() { var ( gifBytes []byte fake *animFFmpeg + broker *fakeEventBroker ) get := func(id string) *Image { @@ -453,7 +455,8 @@ var _ = Describe("Artwork", func() { BeforeEach(func() { gifBytes = createAnimatedGIF(3) fake = &animFFmpeg{MockFFmpeg: tests.NewMockFFmpeg(""), out: []byte("animated-webp")} - svc = NewArtwork(ds, imgCache, store, fake) + broker = &fakeEventBroker{} + svc = NewArtwork(ds, imgCache, store, fake, broker) seedFoundStore("al", "al1", gifBytes) DeferCleanup(waitForConversions) }) @@ -470,6 +473,15 @@ var _ = Describe("Artwork", func() { Expect(fake.calls.Load()).To(Equal(int32(1))) }) + It("tells the UI to reload the item once its conversion is cached", func() { + readAll(get("al-al1")) + waitForConversions() + + sent := broker.getEvents() + Expect(sent).To(HaveLen(1)) + Expect(sent[0].Data(sent[0])).To(MatchJSON(`{"album":["al1"]}`)) + }) + It("runs one conversion at a time and does not queue the others", func() { fake.release = make(chan struct{}) release := sync.OnceFunc(func() { close(fake.release) }) diff --git a/core/artwork/e2e/acquire_serve_test.go b/core/artwork/e2e/acquire_serve_test.go index 34dfb2cac..ea23fa08c 100644 --- a/core/artwork/e2e/acquire_serve_test.go +++ b/core/artwork/e2e/acquire_serve_test.go @@ -108,7 +108,7 @@ var _ = Describe("Acquisition → serve loop", func() { }) Eventually(func() bool { return imgCache.Available(ctx) }, 10*time.Second).Should(BeTrue()) - svc = artwork.NewArtwork(ds, imgCache, store, ffm) + svc = artwork.NewArtwork(ds, imgCache, store, ffm, events.NoopBroker()) worker = artwork.NewWorker(ds, store, agents.GetAgents(ds, nil), ffm, events.NoopBroker(), imgCache) }) diff --git a/core/artwork/e2e/resolution_harness_test.go b/core/artwork/e2e/resolution_harness_test.go index fff62ce69..6427d7fce 100644 --- a/core/artwork/e2e/resolution_harness_test.go +++ b/core/artwork/e2e/resolution_harness_test.go @@ -119,7 +119,7 @@ func setupResolutionHarness() { }) Eventually(func() bool { return imgCache.Available(rctx) }, 10*time.Second).Should(BeTrue()) - rsvc = artwork.NewArtwork(rds, imgCache, rstore, ffm) + rsvc = artwork.NewArtwork(rds, imgCache, rstore, ffm, events.NoopBroker()) rworker = artwork.NewWorker(rds, rstore, agents.GetAgents(rds, nil), ffm, events.NoopBroker(), imgCache) } diff --git a/server/subsonic/e2e/subsonic_artwork_test.go b/server/subsonic/e2e/subsonic_artwork_test.go index 9324ea9e3..57d916f64 100644 --- a/server/subsonic/e2e/subsonic_artwork_test.go +++ b/server/subsonic/e2e/subsonic_artwork_test.go @@ -132,7 +132,7 @@ var _ = Describe("Artwork Serving", Ordered, func() { store := artwork.NewImageStore(GinkgoT().TempDir()) imgCache := newDummyImageCache(ctx) ffm := harness.NoopFFmpeg{} - artSvc = artwork.NewArtwork(ds, imgCache, store, ffm) + artSvc = artwork.NewArtwork(ds, imgCache, store, ffm, events.NoopBroker()) worker = artwork.NewWorker(ds, store, agents.GetAgents(ds, nil), ffm, events.NoopBroker(), imgCache) artRouter = buildArtworkRouter(artSvc) diff --git a/ui/src/common/Artwork.jsx b/ui/src/common/Artwork.jsx index 21ac5e07b..3f39bc94a 100644 --- a/ui/src/common/Artwork.jsx +++ b/ui/src/common/Artwork.jsx @@ -5,6 +5,7 @@ import { makeStyles } from '@material-ui/core/styles' import config from '../config' import subsonic from '../subsonic' import { useImageUrl } from './useImageUrl' +import { useArtworkRefresh } from './useArtworkRefresh' import { ThumbHashCanvas } from './ThumbHashCanvas' // Drives both the CSS transition and the timer that retires the placeholder, so they cannot drift. @@ -47,7 +48,8 @@ export const Artwork = ({ }) => { const classes = useStyles() const url = record ? subsonic.getCoverArtUrl(record, size, square) : '' - const { imgUrl, fromCache } = useImageUrl(url) + const version = useArtworkRefresh(record?.id) + const { imgUrl, fromCache } = useImageUrl(url, version) const [decoded, setDecoded] = useState(false) const [faded, setFaded] = useState(false) diff --git a/ui/src/common/Artwork.test.jsx b/ui/src/common/Artwork.test.jsx index 18b3c916f..bbf744057 100644 --- a/ui/src/common/Artwork.test.jsx +++ b/ui/src/common/Artwork.test.jsx @@ -2,6 +2,7 @@ import { render, fireEvent, act } from '@testing-library/react' import { describe, it, expect, vi, beforeEach } from 'vitest' vi.mock('./useImageUrl', () => ({ useImageUrl: vi.fn() })) +vi.mock('./useArtworkRefresh', () => ({ useArtworkRefresh: vi.fn() })) vi.mock('../subsonic', () => ({ default: { // Mirrors the real hash suffix, so a refreshed record yields a different URL. @@ -13,6 +14,7 @@ vi.mock('../subsonic', () => ({ vi.mock('../config', () => ({ default: { uiCoverArtSize: 300 } })) import { useImageUrl } from './useImageUrl' +import { useArtworkRefresh } from './useArtworkRefresh' import { Artwork } from './Artwork' const withArt = { @@ -28,6 +30,14 @@ describe('Artwork', () => { HTMLCanvasElement.prototype.getContext = vi.fn(() => null) }) + it('loads the image under the version of the last refresh naming its record', () => { + useImageUrl.mockReturnValue({ imgUrl: null, loading: true }) + useArtworkRefresh.mockReturnValue(1234) + render() + expect(useArtworkRefresh).toHaveBeenCalledWith('al-1') + expect(useImageUrl).toHaveBeenCalledWith('/rest/getCoverArt?id=al-1', 1234) + }) + it('renders nothing without a record', () => { useImageUrl.mockReturnValue({ imgUrl: null, loading: false }) const { container } = render() diff --git a/ui/src/common/useArtworkRefresh.js b/ui/src/common/useArtworkRefresh.js new file mode 100644 index 000000000..2e87d139c --- /dev/null +++ b/ui/src/common/useArtworkRefresh.js @@ -0,0 +1,23 @@ +import { useState } from 'react' +import { useSelector } from 'react-redux' + +const namesId = (resources, id) => + Object.values(resources || {}).some( + (ids) => Array.isArray(ids) && ids.includes(id), + ) + +// Artwork can change behind an unchanged URL (a background conversion replacing its stand-in), so +// this returns when the server last announced new artwork for id, for useImageUrl to version by. +export const useArtworkRefresh = (id) => { + const refresh = useSelector((state) => state.activity?.refresh) + const [refreshedAt, setRefreshedAt] = useState() + if ( + id && + refresh && + namesId(refresh.resources, id) && + refresh.lastReceived !== refreshedAt + ) { + setRefreshedAt(refresh.lastReceived) + } + return refreshedAt +} diff --git a/ui/src/common/useArtworkRefresh.test.js b/ui/src/common/useArtworkRefresh.test.js new file mode 100644 index 000000000..89e6f9a1d --- /dev/null +++ b/ui/src/common/useArtworkRefresh.test.js @@ -0,0 +1,44 @@ +import { renderHook } from '@testing-library/react-hooks' +import { vi, describe, it, expect, beforeEach } from 'vitest' +import { useSelector } from 'react-redux' +import { useArtworkRefresh } from './useArtworkRefresh' + +vi.mock('react-redux', () => ({ useSelector: vi.fn() })) + +const withRefresh = (refresh) => + useSelector.mockImplementation((select) => select({ activity: { refresh } })) + +describe('useArtworkRefresh', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it('has no version before any refresh event', () => { + withRefresh(undefined) + const { result } = renderHook(() => useArtworkRefresh('ar-1')) + expect(result.current).toBeUndefined() + }) + + it('versions the artwork with the time of an event naming its id', () => { + withRefresh({ lastReceived: 100, resources: { artist: ['ar-1'] } }) + const { result } = renderHook(() => useArtworkRefresh('ar-1')) + expect(result.current).toBe(100) + }) + + it('keeps its version when a later event names something else', () => { + withRefresh({ lastReceived: 100, resources: { artist: ['ar-1'] } }) + const { result, rerender } = renderHook(() => useArtworkRefresh('ar-1')) + + withRefresh({ lastReceived: 200, resources: { album: ['al-9'] } }) + rerender() + + expect(result.current).toBe(100) + }) + + // Honoring a wildcard would refetch every cover on screen after each scan. + it('ignores wildcard events', () => { + withRefresh({ lastReceived: 100, resources: { '*': '*', artist: ['*'] } }) + const { result } = renderHook(() => useArtworkRefresh('ar-1')) + expect(result.current).toBeUndefined() + }) +}) diff --git a/ui/src/common/useImageUrl.js b/ui/src/common/useImageUrl.js index 8a7f2bcc7..f52d13352 100644 --- a/ui/src/common/useImageUrl.js +++ b/ui/src/common/useImageUrl.js @@ -34,9 +34,10 @@ const evictIfNeeded = () => { /** * Loads an image via fetch() with AbortController so that in-flight requests * are canceled on unmount (e.g., during pagination). Uses a module-level cache - * so remounting returns the cached blob URL instantly. + * so remounting returns the cached blob URL instantly. A new version re-fetches a cached url whose + * image changed behind it, painting the old blob until the new one arrives. */ -export const useImageUrl = (url) => { +export const useImageUrl = (url, version) => { const cached = url ? cache.get(url) : null const [imgUrl, setImgUrl] = useState(cached?.blobUrl || null) const [loading, setLoading] = useState(!!url && !cached) @@ -68,7 +69,7 @@ export const useImageUrl = (url) => { // Re-check: another component's effect may have populated the cache // between this component's render and effect execution. const entry = cache.get(url) - if (entry) { + if (entry && entry.version === version) { entry.refCount++ setImgUrl(entry.blobUrl) setLoading(false) @@ -77,12 +78,15 @@ export const useImageUrl = (url) => { entry.refCount-- } } + const stale = entry?.blobUrl ? entry : null const controller = new AbortController() let queued = true - setImgUrl(null) - setLoading(true) - setError(false) + if (!stale) { + setImgUrl(null) + setLoading(true) + setError(false) + } const doFetch = () => { queued = false @@ -105,12 +109,15 @@ export const useImageUrl = (url) => { // Handle concurrent fetches: if another component already cached // this URL, use its entry and discard our blob. const existing = cache.get(url) - if (existing && existing.blobUrl) { + if (existing && existing.blobUrl && existing.version === version) { existing.refCount++ URL.revokeObjectURL(objectUrl) setImgUrl(existing.blobUrl) } else { - cache.set(url, { blobUrl: objectUrl, refCount: 1 }) + if (existing?.blobUrl && existing.refCount <= 0) { + URL.revokeObjectURL(existing.blobUrl) + } + cache.set(url, { blobUrl: objectUrl, refCount: 1, version }) evictIfNeeded() setImgUrl(objectUrl) } @@ -122,8 +129,11 @@ export const useImageUrl = (url) => { if (err.name === 'AbortError') { return // Expected on unmount or URL change } + if (stale) { + return // The old image is still valid; keep showing it + } // Cache the error so repeated mounts don't re-fetch broken URLs - cache.set(url, { blobUrl: null, error: true, refCount: 0 }) + cache.set(url, { blobUrl: null, error: true, refCount: 0, version }) setError(true) setLoading(false) }) @@ -150,7 +160,7 @@ export const useImageUrl = (url) => { entry.refCount-- } } - }, [url]) + }, [url, version]) return { imgUrl, loading, error, fromCache } } diff --git a/ui/src/common/useImageUrl.test.js b/ui/src/common/useImageUrl.test.js index 818732d06..bc6bc70fd 100644 --- a/ui/src/common/useImageUrl.test.js +++ b/ui/src/common/useImageUrl.test.js @@ -234,4 +234,90 @@ describe('useImageUrl', () => { // A remembered failure has no blob, so callers must not treat it as instantly painted. expect(result2.current.fromCache).toBe(false) }) + + describe('version', () => { + const ok = (data) => + Promise.resolve({ + ok: true, + blob: () => Promise.resolve(new Blob([data])), + }) + const renderVersioned = (version) => + renderHook( + ({ version }) => useImageUrl('http://example.com/img.jpg', version), + { initialProps: { version } }, + ) + + it('re-fetches a cached url when its version changes, showing the old image meanwhile', async () => { + let resolveNew + global.URL.createObjectURL = vi + .fn() + .mockReturnValueOnce('blob:old') + .mockReturnValueOnce('blob:new') + global.fetch = vi + .fn() + .mockImplementationOnce(() => ok('old')) + .mockImplementationOnce( + () => new Promise((resolve) => (resolveNew = resolve)), + ) + + const { result, rerender } = renderVersioned(undefined) + await act(async () => { + await flushPromises() + }) + expect(result.current.imgUrl).toBe('blob:old') + + rerender({ version: 1 }) + await act(async () => { + await flushPromises() + }) + expect(global.fetch).toHaveBeenCalledTimes(2) + expect(result.current.imgUrl).toBe('blob:old') + expect(result.current.loading).toBe(false) + + await act(async () => { + resolveNew({ ok: true, blob: () => Promise.resolve(new Blob(['new'])) }) + await flushPromises() + }) + expect(result.current.imgUrl).toBe('blob:new') + }) + + it('keeps the old image when the re-fetch fails', async () => { + global.fetch = vi + .fn() + .mockImplementationOnce(() => ok('old')) + .mockImplementationOnce(() => + Promise.resolve({ ok: false, status: 500 }), + ) + + const { result, rerender } = renderVersioned(undefined) + await act(async () => { + await flushPromises() + }) + rerender({ version: 1 }) + await act(async () => { + await flushPromises() + }) + + expect(global.fetch).toHaveBeenCalledTimes(2) + expect(result.current.imgUrl).toBe('blob:mock-url') + expect(result.current.error).toBe(false) + }) + + it('does not re-fetch on remount with the version it already has', async () => { + global.fetch = vi.fn(() => ok('data')) + + const { unmount } = renderVersioned(1) + await act(async () => { + await flushPromises() + }) + unmount() + const { result } = renderVersioned(1) + await act(async () => { + await flushPromises() + }) + + expect(global.fetch).toHaveBeenCalledTimes(1) + expect(result.current.fromCache).toBe(true) + }) + }) }) From 1bfe5accc8dec9d834949f3861b4e0472b212fb9 Mon Sep 17 00:00:00 2001 From: Deluan Date: Mon, 14 Sep 2026 21:45:46 -0400 Subject: [PATCH 4/4] refactor(ui): only re-fetch no-store artwork stand-ins Refresh events also name albums and songs for plays, stars and ratings, so versioning every cached cover re-downloaded final, immutable images on each of them. useImageUrl now marks a no-store response as a stand-in and re-fetches only those entries, on remount as well as on a version change; final images are never re-fetched, and cache entries no longer carry a version. useArtworkRefresh now selects a primitive, so an event naming other items does not re-render every mounted cover. --- ui/src/common/useArtworkRefresh.js | 16 +++--- ui/src/common/useImageUrl.js | 27 +++++----- ui/src/common/useImageUrl.test.js | 84 ++++++++++++++++-------------- 3 files changed, 69 insertions(+), 58 deletions(-) diff --git a/ui/src/common/useArtworkRefresh.js b/ui/src/common/useArtworkRefresh.js index 2e87d139c..7fb92586b 100644 --- a/ui/src/common/useArtworkRefresh.js +++ b/ui/src/common/useArtworkRefresh.js @@ -9,15 +9,15 @@ const namesId = (resources, id) => // Artwork can change behind an unchanged URL (a background conversion replacing its stand-in), so // this returns when the server last announced new artwork for id, for useImageUrl to version by. export const useArtworkRefresh = (id) => { - const refresh = useSelector((state) => state.activity?.refresh) + // A primitive, so an event naming other items does not re-render this cover. + const named = useSelector(({ activity }) => + id && namesId(activity?.refresh?.resources, id) + ? activity.refresh.lastReceived + : undefined, + ) const [refreshedAt, setRefreshedAt] = useState() - if ( - id && - refresh && - namesId(refresh.resources, id) && - refresh.lastReceived !== refreshedAt - ) { - setRefreshedAt(refresh.lastReceived) + if (named !== undefined && named !== refreshedAt) { + setRefreshedAt(named) } return refreshedAt } diff --git a/ui/src/common/useImageUrl.js b/ui/src/common/useImageUrl.js index f52d13352..71811dbed 100644 --- a/ui/src/common/useImageUrl.js +++ b/ui/src/common/useImageUrl.js @@ -34,8 +34,8 @@ const evictIfNeeded = () => { /** * Loads an image via fetch() with AbortController so that in-flight requests * are canceled on unmount (e.g., during pagination). Uses a module-level cache - * so remounting returns the cached blob URL instantly. A new version re-fetches a cached url whose - * image changed behind it, painting the old blob until the new one arrives. + * so remounting returns the cached blob URL instantly. A no-store response is a stand-in: it is + * re-fetched on remount and on a new version, and painted until its replacement arrives. */ export const useImageUrl = (url, version) => { const cached = url ? cache.get(url) : null @@ -69,7 +69,7 @@ export const useImageUrl = (url, version) => { // Re-check: another component's effect may have populated the cache // between this component's render and effect execution. const entry = cache.get(url) - if (entry && entry.version === version) { + if (entry && !entry.transient) { entry.refCount++ setImgUrl(entry.blobUrl) setLoading(false) @@ -78,11 +78,11 @@ export const useImageUrl = (url, version) => { entry.refCount-- } } - const stale = entry?.blobUrl ? entry : null + const standIn = !!entry const controller = new AbortController() let queued = true - if (!stale) { + if (!standIn) { setImgUrl(null) setLoading(true) setError(false) @@ -96,9 +96,12 @@ export const useImageUrl = (url, version) => { if (!res.ok) { throw new Error(`HTTP ${res.status}`) } - return res.blob() + const transient = !!res.headers + ?.get('Cache-Control') + ?.includes('no-store') + return res.blob().then((blob) => ({ blob, transient })) }) - .then((blob) => { + .then(({ blob, transient }) => { activeFetches-- processQueue() // Guard against late resolution after abort @@ -109,7 +112,7 @@ export const useImageUrl = (url, version) => { // Handle concurrent fetches: if another component already cached // this URL, use its entry and discard our blob. const existing = cache.get(url) - if (existing && existing.blobUrl && existing.version === version) { + if (existing?.blobUrl && !existing.transient) { existing.refCount++ URL.revokeObjectURL(objectUrl) setImgUrl(existing.blobUrl) @@ -117,7 +120,7 @@ export const useImageUrl = (url, version) => { if (existing?.blobUrl && existing.refCount <= 0) { URL.revokeObjectURL(existing.blobUrl) } - cache.set(url, { blobUrl: objectUrl, refCount: 1, version }) + cache.set(url, { blobUrl: objectUrl, refCount: 1, transient }) evictIfNeeded() setImgUrl(objectUrl) } @@ -129,11 +132,11 @@ export const useImageUrl = (url, version) => { if (err.name === 'AbortError') { return // Expected on unmount or URL change } - if (stale) { - return // The old image is still valid; keep showing it + if (standIn) { + return // Keep painting the stand-in } // Cache the error so repeated mounts don't re-fetch broken URLs - cache.set(url, { blobUrl: null, error: true, refCount: 0, version }) + cache.set(url, { blobUrl: null, error: true, refCount: 0 }) setError(true) setLoading(false) }) diff --git a/ui/src/common/useImageUrl.test.js b/ui/src/common/useImageUrl.test.js index bc6bc70fd..fc7b91ab8 100644 --- a/ui/src/common/useImageUrl.test.js +++ b/ui/src/common/useImageUrl.test.js @@ -235,89 +235,97 @@ describe('useImageUrl', () => { expect(result2.current.fromCache).toBe(false) }) - describe('version', () => { - const ok = (data) => + describe('no-store stand-ins', () => { + const image = (data, cacheControl = 'public, no-cache') => Promise.resolve({ ok: true, + headers: { + get: (name) => (name === 'Cache-Control' ? cacheControl : null), + }, blob: () => Promise.resolve(new Blob([data])), }) + const standIn = (data) => image(data, 'no-store') const renderVersioned = (version) => renderHook( ({ version }) => useImageUrl('http://example.com/img.jpg', version), { initialProps: { version } }, ) + const settle = () => + act(async () => { + await flushPromises() + }) - it('re-fetches a cached url when its version changes, showing the old image meanwhile', async () => { - let resolveNew + it('re-fetches a stand-in when its version changes, painting it meanwhile', async () => { + let resolveFinal global.URL.createObjectURL = vi .fn() - .mockReturnValueOnce('blob:old') - .mockReturnValueOnce('blob:new') + .mockReturnValueOnce('blob:stand-in') + .mockReturnValueOnce('blob:final') global.fetch = vi .fn() - .mockImplementationOnce(() => ok('old')) + .mockImplementationOnce(() => standIn('still')) .mockImplementationOnce( - () => new Promise((resolve) => (resolveNew = resolve)), + () => new Promise((resolve) => (resolveFinal = resolve)), ) const { result, rerender } = renderVersioned(undefined) - await act(async () => { - await flushPromises() - }) - expect(result.current.imgUrl).toBe('blob:old') + await settle() + expect(result.current.imgUrl).toBe('blob:stand-in') rerender({ version: 1 }) - await act(async () => { - await flushPromises() - }) + await settle() expect(global.fetch).toHaveBeenCalledTimes(2) - expect(result.current.imgUrl).toBe('blob:old') + expect(result.current.imgUrl).toBe('blob:stand-in') expect(result.current.loading).toBe(false) await act(async () => { - resolveNew({ ok: true, blob: () => Promise.resolve(new Blob(['new'])) }) + resolveFinal(image('animated')) await flushPromises() }) - expect(result.current.imgUrl).toBe('blob:new') + expect(result.current.imgUrl).toBe('blob:final') }) - it('keeps the old image when the re-fetch fails', async () => { + it('keeps the stand-in when the re-fetch fails', async () => { global.fetch = vi .fn() - .mockImplementationOnce(() => ok('old')) + .mockImplementationOnce(() => standIn('still')) .mockImplementationOnce(() => Promise.resolve({ ok: false, status: 500 }), ) const { result, rerender } = renderVersioned(undefined) - await act(async () => { - await flushPromises() - }) + await settle() rerender({ version: 1 }) - await act(async () => { - await flushPromises() - }) + await settle() expect(global.fetch).toHaveBeenCalledTimes(2) expect(result.current.imgUrl).toBe('blob:mock-url') expect(result.current.error).toBe(false) }) - it('does not re-fetch on remount with the version it already has', async () => { - global.fetch = vi.fn(() => ok('data')) + // Plays, stars and ratings also name items, and must not re-download final images. + it('does not re-fetch a final image when its version changes', async () => { + global.fetch = vi.fn(() => image('final')) - const { unmount } = renderVersioned(1) - await act(async () => { - await flushPromises() - }) - unmount() - const { result } = renderVersioned(1) - await act(async () => { - await flushPromises() - }) + const { rerender } = renderVersioned(undefined) + await settle() + rerender({ version: 1 }) + await settle() expect(global.fetch).toHaveBeenCalledTimes(1) - expect(result.current.fromCache).toBe(true) + }) + + it('re-fetches a stand-in on remount', async () => { + global.fetch = vi.fn(() => standIn('still')) + + const { unmount } = renderVersioned(undefined) + await settle() + unmount() + const { result } = renderVersioned(undefined) + await settle() + + expect(global.fetch).toHaveBeenCalledTimes(2) + expect(result.current.imgUrl).toBe('blob:mock-url') }) }) })