From 35d08a25a37eb801227403448a010627f9307094 Mon Sep 17 00:00:00 2001 From: Deluan Date: Fri, 17 Jul 2026 21:34:15 -0400 Subject: [PATCH] fix(album): don't wipe covers on album merges; bust disc/song art caches Adversarial review findings: - CopyAttributes copied uploaded_image/cover_art_updated_at unconditionally, so merging an album without a cover into one with a cover (retag onto an existing album ID, or phase-2 moved-track matching) blanked the destination's uploaded cover. Skip empty/NULL source values, mirroring the created_at guard, and make the mock faithful to the same semantics. - The disc and mediafile artwork readers did not fold cover_art_updated_at into their cache keys, so song/disc art reached through the fromAlbum fallback kept serving stale bytes after a cover upload or delete. - The UI cover cache-buster compared timestamps lexicographically; mixed offset/fractional formats could pick the older one. Compare as dates. --- core/artwork/reader_disc.go | 3 +++ core/artwork/reader_mediafile.go | 3 +++ persistence/album_repository.go | 4 ++++ persistence/album_repository_test.go | 8 ++++++++ tests/mock_album_repo.go | 9 +++++++-- ui/src/subsonic/index.js | 4 ++-- 6 files changed, 27 insertions(+), 4 deletions(-) diff --git a/core/artwork/reader_disc.go b/core/artwork/reader_disc.go index 0f648c987..44d199023 100644 --- a/core/artwork/reader_disc.go +++ b/core/artwork/reader_disc.go @@ -110,6 +110,9 @@ func newDiscArtworkReader(ctx context.Context, a *artwork, artID model.ArtworkID if imagesUpdatedAt != nil { r.cacheKey.lastUpdate = utils.TimeNewest(r.cacheKey.lastUpdate, *imagesUpdatedAt) } + if al.CoverArtUpdatedAt != nil { + r.cacheKey.lastUpdate = utils.TimeNewest(r.cacheKey.lastUpdate, *al.CoverArtUpdatedAt) + } return r, nil } diff --git a/core/artwork/reader_mediafile.go b/core/artwork/reader_mediafile.go index eac3c5e70..96c3635fd 100644 --- a/core/artwork/reader_mediafile.go +++ b/core/artwork/reader_mediafile.go @@ -49,6 +49,9 @@ func newMediafileArtworkReader(ctx context.Context, artwork *artwork, artID mode if imagesUpdatedAt != nil && imagesUpdatedAt.After(a.cacheKey.lastUpdate) { a.cacheKey.lastUpdate = *imagesUpdatedAt } + if al.CoverArtUpdatedAt != nil && al.CoverArtUpdatedAt.After(a.cacheKey.lastUpdate) { + a.cacheKey.lastUpdate = *al.CoverArtUpdatedAt + } return a, nil } diff --git a/persistence/album_repository.go b/persistence/album_repository.go index 6192b3d18..d59fef9a7 100644 --- a/persistence/album_repository.go +++ b/persistence/album_repository.go @@ -287,6 +287,10 @@ func (r *albumRepository) CopyAttributes(fromID, toID string, columns ...string) if col == "created_at" && (!v.Valid || v.String == "" || strings.HasPrefix(v.String, "0001-")) { continue } + // A source without an uploaded cover must not wipe one the destination has. + if (col == "uploaded_image" || col == "cover_art_updated_at") && (!v.Valid || v.String == "") { + continue + } to[col] = v } if len(to) == 0 { diff --git a/persistence/album_repository_test.go b/persistence/album_repository_test.go index 7978557a8..64eb7d4a4 100644 --- a/persistence/album_repository_test.go +++ b/persistence/album_repository_test.go @@ -116,6 +116,14 @@ var _ = Describe("AlbumRepository", func() { Expect(err).ToNot(HaveOccurred()) Expect(got.UploadedImage).To(Equal("copy-src_cover.jpg")) }) + It("does not wipe the destination's cover when the source has none", func() { + Expect(albumRepo.UpdateImage("copy-dst", "copy-dst_cover.jpg")).To(Succeed()) + Expect(albumRepo.CopyAttributes("copy-src", "copy-dst", "uploaded_image", "cover_art_updated_at")).To(Succeed()) + got, err := albumRepo.Get("copy-dst") + Expect(err).ToNot(HaveOccurred()) + Expect(got.UploadedImage).To(Equal("copy-dst_cover.jpg")) + Expect(got.CoverArtUpdatedAt).ToNot(BeNil()) + }) }) Describe("GetCursor", func() { diff --git a/tests/mock_album_repo.go b/tests/mock_album_repo.go index 1ac03394f..843552fc6 100644 --- a/tests/mock_album_repo.go +++ b/tests/mock_album_repo.go @@ -190,9 +190,14 @@ func (m *MockAlbumRepo) CopyAttributes(fromID, toID string, columns ...string) e case "created_at": to.CreatedAt = from.CreatedAt case "uploaded_image": - to.UploadedImage = from.UploadedImage + // Mirrors the real repo: an empty source never wipes the destination's cover + if from.UploadedImage != "" { + to.UploadedImage = from.UploadedImage + } case "cover_art_updated_at": - to.CoverArtUpdatedAt = from.CoverArtUpdatedAt + if from.CoverArtUpdatedAt != nil { + to.CoverArtUpdatedAt = from.CoverArtUpdatedAt + } } } if m.CopyAttributesCalls == nil { diff --git a/ui/src/subsonic/index.js b/ui/src/subsonic/index.js index d0271cac0..140774550 100644 --- a/ui/src/subsonic/index.js +++ b/ui/src/subsonic/index.js @@ -82,10 +82,10 @@ const getAvatarUrl = (username, size) => const getCoverArtUrl = (record, size, square) => { // Bust the cache on the newest of updatedAt / coverArtUpdatedAt (album cover - // uploads bump the latter) — ISO timestamps sort chronologically. + // uploads bump the latter). const cacheKey = [record.updatedAt, record.coverArtUpdatedAt] .filter(Boolean) - .sort() + .sort((a, b) => new Date(a) - new Date(b)) .pop() const options = { ...(cacheKey && { _: cacheKey }),