diff --git a/core/artwork/artwork.go b/core/artwork/artwork.go index f3dcb7593..077ca31d4 100644 --- a/core/artwork/artwork.go +++ b/core/artwork/artwork.go @@ -19,6 +19,19 @@ import ( var ErrUnavailable = errors.New("artwork unavailable") +// maxTeeBytes bounds the per-serve capture buffer; artwork is a few MB, and anything larger is not +// hashed (skipped), so a pathological source can't accumulate unbounded memory across serves. +const maxTeeBytes = 20 * 1024 * 1024 + +// capAtNow keeps a future artwork mtime (clock skew, a future-stamped file) from being stored as the +// blurhash version, which would let the DTO's !Before check pin the hash until wall time caught up. +func capAtNow(t time.Time) time.Time { + if now := time.Now(); t.After(now) { + return now + } + return t +} + type Artwork interface { Get(ctx context.Context, artID model.ArtworkID, size int, square bool) (io.ReadCloser, time.Time, error) GetOrPlaceholder(ctx context.Context, id string, size int, square bool) (io.ReadCloser, time.Time, error) @@ -59,6 +72,11 @@ func (a *artwork) GetOrPlaceholder(ctx context.Context, id string, size int, squ reader, lastUpdate, err = a.Get(ctx, artID, size, square) } if errors.Is(err, ErrUnavailable) { + if a.blurHashes != nil && eligibleKind(artID) { + // No bytes flowed through the tee; a real deletion must still clear the stored hash. The + // worker re-checks so a transient fetch failure doesn't clobber a valid hash. + a.blurHashes.EnqueueClearIfGone(artID, capAtNow(consts.ServerStart)) + } if artID.Kind == model.KindArtistArtwork { reader, _ = resources.FS().Open(consts.PlaceholderArtistArt) } else { @@ -80,21 +98,17 @@ func (a *artwork) Get(ctx context.Context, artID model.ArtworkID, size int, squa if !errors.Is(err, context.Canceled) && !errors.Is(err, ErrUnavailable) { log.Error(ctx, "Error accessing image cache", "id", artID, "size", size, err) } - // A vanished source must still reach the worker, or a stored hash would keep describing - // artwork that no longer exists. - if a.blurHashes != nil && errors.Is(err, ErrUnavailable) { - a.blurHashes.EnqueueGone(artID) - } return nil, time.Time{}, err } - if a.blurHashes != nil && size == 0 && !square { - // Every original-size serve carries the reader's true version; the worker's freshness guard - // turns already-hashed rows into a cheap read and only recomputes when the hash is stale or - // missing. Enqueuing on cache hits too (not just fills) is what backfills warm caches adopted - // from a pre-blurhash version, whose rows migrated in with an empty hash. - a.blurHashes.Enqueue(artID, artReader.LastUpdated()) + reader = r + if a.blurHashes != nil && size == 0 && !square && eligibleKind(artID) { + // Tee the served bytes: the blurhash is computed from exactly what the client downloads, so it + // changes precisely when the served cover changes. Placeholder bytes (playlist fallback) clear. + version := capAtNow(artReader.LastUpdated()) + reader = &teeCachedStream{CachedStream: r, tee: newTeeReader(io.NopCloser(r), maxTeeBytes, + func(data []byte) { a.blurHashes.EnqueueBytes(artID, data, version) })} } - return r, artReader.LastUpdated(), nil + return reader, artReader.LastUpdated(), nil } type coverArtGetter interface { diff --git a/core/artwork/blurhash_tee.go b/core/artwork/blurhash_tee.go index f603f1f65..60c0d3661 100644 --- a/core/artwork/blurhash_tee.go +++ b/core/artwork/blurhash_tee.go @@ -3,6 +3,8 @@ package artwork import ( "bytes" "io" + + "github.com/navidrome/navidrome/utils/cache" ) // teeReader mirrors bytes read from src into buf, and on Close invokes onComplete with the captured @@ -46,3 +48,14 @@ func (t *teeReader) Close() error { } return err } + +// teeCachedStream wraps a *cache.CachedStream so reads are teed for blurhash capture while callers +// still see a ReadCloser. Seek is intentionally dropped: blurhash-eligible serves are full reads +// (every artwork handler does io.Copy), so no caller Seeks a teed stream. +type teeCachedStream struct { + *cache.CachedStream + tee *teeReader +} + +func (t *teeCachedStream) Read(p []byte) (int, error) { return t.tee.Read(p) } +func (t *teeCachedStream) Close() error { return t.tee.Close() } diff --git a/core/artwork/e2e/blurhash_test.go b/core/artwork/e2e/blurhash_test.go index 148a3ff56..9c834a15c 100644 --- a/core/artwork/e2e/blurhash_test.go +++ b/core/artwork/e2e/blurhash_test.go @@ -74,8 +74,8 @@ var _ = Describe("BlurHash", func() { firstHash = updated.BlurHash }, "10s", "100ms").Should(Succeed()) - // Swap the cover bytes and rescan: the folder's image version moves, so the reader key moves, - // the cache misses and the fill re-triggers the compute — no serve-time force hint needed. + // Swap the cover bytes and rescan, then serve: the tee hashes the newly-served bytes, so the + // stored hash moves to describe the new cover. setLayout(fstest.MapFS{ "Artist/Album/01 - Song.mp3": trackFile(1, "Song"), "Artist/Album/cover.png": realPNG("swapped-cover"), @@ -105,13 +105,13 @@ var _ = Describe("BlurHash", func() { g.Expect(updated.BlurHash).ToNot(BeEmpty()) }, "10s", "100ms").Should(Succeed()) - // No rescan: the folder row still lists the cover, but the file is gone — the serve's - // ErrUnavailable alone must trigger the clear. + // No rescan: the folder row still lists the cover, but the file is gone. The serve falls back + // to the placeholder (GetOrPlaceholder, the real Jellyfin/Subsonic path), and the worker's + // gone-recheck confirms the source is really gone and clears the stored hash. setLayout(fstest.MapFS{ "Artist/Album/01 - Song.mp3": trackFile(1, "Song"), }) - _, err := readArtworkOrErr(al.CoverArtID()) - Expect(err).To(HaveOccurred()) + Expect(readOrPlaceholder(al.CoverArtID())).To(Equal(placeholderBytes())) Eventually(func(g Gomega) { updated, err := ds.Album(ctx).Get(al.ID) @@ -120,7 +120,7 @@ var _ = Describe("BlurHash", func() { }, "10s", "100ms").Should(Succeed()) }) - PIt("recomputes when cover bytes change under a preserved mtime (cache disabled)", func() { + It("recomputes when cover bytes change under a preserved mtime (cache disabled)", func() { cover := realPNG("orig-bytes") fixed := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC) cover.ModTime = fixed diff --git a/core/artwork/e2e/helpers_test.go b/core/artwork/e2e/helpers_test.go index e3abca097..807c368e1 100644 --- a/core/artwork/e2e/helpers_test.go +++ b/core/artwork/e2e/helpers_test.go @@ -158,6 +158,18 @@ func readArtworkOrErr(artID model.ArtworkID) ([]byte, error) { return io.ReadAll(r) } +// readOrPlaceholder serves through GetOrPlaceholder — the path real Jellyfin/Subsonic handlers use — +// so a vanished album/artist cover falls back to the placeholder, whose bytes drive the blurhash clear. +func readOrPlaceholder(artID model.ArtworkID) []byte { + GinkgoHelper() + r, _, err := aw.GetOrPlaceholder(ctx, artID.String(), 0, false) + Expect(err).ToNot(HaveOccurred()) + defer r.Close() + b, err := io.ReadAll(r) + Expect(err).ToNot(HaveOccurred()) + return b +} + // noopProvider implements external.Provider with not-found returns so the // "external" priority entry never produces a result. type noopProvider struct{}