From 28eba567a74c348263f3c59e1882c54b4fec9f33 Mon Sep 17 00:00:00 2001 From: bobo-xxx <111567133+bobo-xxx@users.noreply.github.com> Date: Sat, 18 Apr 2026 09:35:33 +0800 Subject: [PATCH] fix(artwork): return correct timestamp when disc or album coverart changes (#5378) * fix(artwork): return imagesUpdatedAt in LastUpdated when cover art changes When cover art (cover.jpg) is updated in an album folder, the HTTP Last-Modified header was incorrectly returning album.UpdatedAt (which only tracks media file changes) instead of imagesUpdatedAt (which tracks cover art changes). This caused browsers to use their cached cover art because the Last-Modified header didn't change, even though the actual cover art image data was new (due to cache key changing based on imagesUpdatedAt). The fix ensures LastUpdated() returns a.lastUpdate (which is the max of album.UpdatedAt and imagesUpdatedAt) instead of always returning album.UpdatedAt. Fixes navidrome/navidrome#5377 * refactor tests Signed-off-by: Deluan * fix(artwork): return imagesUpdatedAt in disc LastUpdated The discArtworkReader had the same bug as albumArtworkReader (fixed in 9a741859f): LastUpdated() returned album.UpdatedAt while Key() used the max of album.UpdatedAt and ImagesUpdatedAt. This mismatch caused browsers to keep stale disc cover art in cache when only the image file changed. Also strengthen the album LastUpdated tests and add matching tests for the disc reader. The tests use DescribeTable and were verified to fail when the fix is reverted. --------- Signed-off-by: Deluan Co-authored-by: Deluan --- core/artwork/artwork_internal_test.go | 48 +++++++++++++++++++++++++-- core/artwork/reader_album.go | 2 +- core/artwork/reader_disc.go | 2 +- 3 files changed, 48 insertions(+), 4 deletions(-) diff --git a/core/artwork/artwork_internal_test.go b/core/artwork/artwork_internal_test.go index 380352d3f..0c03ef0ca 100644 --- a/core/artwork/artwork_internal_test.go +++ b/core/artwork/artwork_internal_test.go @@ -7,12 +7,11 @@ import ( "image/jpeg" "image/png" "io" - "os" "path/filepath" + "time" _ "github.com/gen2brain/webp" - "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/conf/configtest" "github.com/navidrome/navidrome/log" @@ -146,6 +145,51 @@ var _ = Describe("Artwork", func() { Entry(nil, " embedded , front.* , cover.*,folder.*", "tests/fixtures/artist/an-album/test.mp3"), ) }) + Context("LastUpdated", func() { + // Regression test for #5377: LastUpdated feeds the HTTP Last-Modified header. + // It must return max(album.UpdatedAt, ImagesUpdatedAt) so browsers revalidate + // cached cover art when only the image file changes. + now := time.Now().Truncate(time.Second) + DescribeTable("returns the max of album.UpdatedAt and ImagesUpdatedAt", + func(albumUpdatedAt, imagesUpdatedAt, expected time.Time) { + album := model.Album{ID: "al1", UpdatedAt: albumUpdatedAt} + folderRepo.result = []model.Folder{{ImagesUpdatedAt: imagesUpdatedAt}} + ds.Album(ctx).(*tests.MockAlbumRepo).SetData(model.Albums{album}) + + ar, err := newAlbumArtworkReader(ctx, aw, album.CoverArtID(), nil) + Expect(err).ToNot(HaveOccurred()) + Expect(ar.LastUpdated()).To(Equal(expected)) + }, + Entry("album newer than images", now, now.Add(-1*time.Hour), now), + Entry("images newer than album", now.Add(-24*time.Hour), now.Add(-1*time.Hour), now.Add(-1*time.Hour)), + Entry("equal timestamps", now, now, now), + ) + }) + }) + Describe("discArtworkReader", func() { + Context("LastUpdated", func() { + // Regression test for #5377: same bug as albumArtworkReader — disc covers + // must also revalidate when the image file changes, not only when media files do. + now := time.Now().Truncate(time.Second) + DescribeTable("returns the max of album.UpdatedAt and ImagesUpdatedAt", + func(albumUpdatedAt, imagesUpdatedAt, expected time.Time) { + album := model.Album{ID: "al1", UpdatedAt: albumUpdatedAt} + folderRepo.result = []model.Folder{{ImagesUpdatedAt: imagesUpdatedAt}} + ds.Album(ctx).(*tests.MockAlbumRepo).SetData(model.Albums{album}) + ds.MediaFile(ctx).(*tests.MockMediaFileRepo).SetData(model.MediaFiles{ + {ID: "mf1", AlbumID: "al1", DiscNumber: 1, Path: "tests/fixtures/test.mp3"}, + }) + + artID := model.NewArtworkID(model.KindDiscArtwork, model.DiscArtworkID("al1", 1), nil) + dr, err := newDiscArtworkReader(ctx, aw, artID) + Expect(err).ToNot(HaveOccurred()) + Expect(dr.LastUpdated()).To(Equal(expected)) + }, + Entry("album newer than images", now, now.Add(-1*time.Hour), now), + Entry("images newer than album", now.Add(-24*time.Hour), now.Add(-1*time.Hour), now.Add(-1*time.Hour)), + Entry("equal timestamps", now, now, now), + ) + }) }) Describe("artistArtworkReader", func() { Context("Multiple covers", func() { diff --git a/core/artwork/reader_album.go b/core/artwork/reader_album.go index 641b12b33..35d489b6c 100644 --- a/core/artwork/reader_album.go +++ b/core/artwork/reader_album.go @@ -72,7 +72,7 @@ func (a *albumArtworkReader) Key() string { ) } func (a *albumArtworkReader) LastUpdated() time.Time { - return a.album.UpdatedAt + return a.lastUpdate } func (a *albumArtworkReader) Reader(ctx context.Context) (io.ReadCloser, string, error) { diff --git a/core/artwork/reader_disc.go b/core/artwork/reader_disc.go index 5a7a8a65e..30d4968e1 100644 --- a/core/artwork/reader_disc.go +++ b/core/artwork/reader_disc.go @@ -116,7 +116,7 @@ func (d *discArtworkReader) Key() string { } func (d *discArtworkReader) LastUpdated() time.Time { - return d.album.UpdatedAt + return d.lastUpdate } func (d *discArtworkReader) Reader(ctx context.Context) (io.ReadCloser, string, error) {