From 6bb3e98e4c2be4ce460e565397e00a5aa486a234 Mon Sep 17 00:00:00 2001 From: Deluan Date: Sat, 25 Jul 2026 15:06:51 -0400 Subject: [PATCH] fix(artwork): give full-size disc art a real ETag MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Regression from 04d5a556. Keying the resize cache on identity meant the disc response no longer carried a content hash, and the full-size branch set no ETag either — so WriteImageHeaders fell back to the empty hash and emitted `ETag: ""` for every full-size disc image. Since ifNoneMatch compares the unquoted value, a client echoing that back matched, and got a 304 even after the image was replaced. The full-size branch now carries the same identity validator the sized branch uses, so it moves when the folder's images change. WriteImageHeaders is hardened against the class as well: an empty validator is no validator, so it is neither emitted nor matched. Reported by Codex on #5847. --- core/artwork/serving.go | 15 +++++++-------- core/artwork/serving_test.go | 22 ++++++++++++++++++++++ server/imghttp/headers.go | 8 ++++++-- server/imghttp/headers_test.go | 13 +++++++++++++ 4 files changed, 48 insertions(+), 10 deletions(-) diff --git a/core/artwork/serving.go b/core/artwork/serving.go index 9199c4ed6..ecefd133b 100644 --- a/core/artwork/serving.go +++ b/core/artwork/serving.go @@ -313,20 +313,19 @@ func (s *service) serveDisc(ctx context.Context, artID model.ArtworkID, size int return selectImageReader(ctx, artID, funcs...) } albumArtID := model.ArtworkID{Kind: model.KindAlbumArtwork, ID: dr.album.ID} + // Disc art has no state row, so there is no stored content hash — to key the resize cache + // on, or to fall back to as a validator. Keying on the id and the album's mtime, as the + // legacy reader did, lets a warm cache answer without touching the filesystem; the chain + // runs only on a miss. The key carries DiscArtPriority so changing it invalidates. + key := fmt.Sprintf("%s|%d|%s", artID.ID, dr.cacheTime().UnixNano(), conf.Server.DiscArtPriority) if size == 0 && !square { - r, path, err := selectImage() + r, _, err := selectImage() if err != nil || r == nil { return s.Get(ctx, albumArtID, size, square) } - return &Image{ReadCloser: r, LastUpdated: unixMtime(mtimeViaFS(dr.lib.FS, path))}, nil + return &Image{ReadCloser: r, ETag: representationTag(key, size, square), LastUpdated: dr.cacheTime()}, nil } - // Disc art has no state row, so there is no stored content hash to key the resize cache on. - // Keying on the id and the album's mtime — as the legacy reader did — lets a warm cache - // answer without touching the filesystem at all; the chain runs only on a miss. The key - // carries DiscArtPriority so changing it invalidates. resizedItem.hash is key material - // here, not a content hash, so the response carries no Image.Hash. - key := fmt.Sprintf("%s|%d|%s", artID.ID, dr.cacheTime().UnixNano(), conf.Server.DiscArtPriority) item := &resizedItem{ hash: key, size: size, diff --git a/core/artwork/serving_test.go b/core/artwork/serving_test.go index 03499d8bf..0baea606c 100644 --- a/core/artwork/serving_test.go +++ b/core/artwork/serving_test.go @@ -393,6 +393,28 @@ var _ = Describe("Service", func() { Expect(second.ETag).ToNot(Equal(firstKey), "a replaced image must not keep the old cache entry") }) + // Disc art has no content hash to fall back to, so without an explicit validator the + // full-size response would carry an empty ETag — identical for every disc image. + It("gives a full-size disc image a validator that tracks the source", func() { + folderRepo.result = []model.Folder{{ + Path: "tests/fixtures/artist/an-album", ImageFiles: []string{"cover.jpg"}, + ImagesUpdatedAt: time.Now().Add(-time.Hour), + }} + albumRepo.SetData(model.Albums{{ID: "aldc5", Name: "Album", FolderIDs: []string{"f1"}}}) + discID := model.NewArtworkID(model.KindDiscArtwork, model.DiscArtworkID("aldc5", 1), nil) + + first, err := svc.Get(ctx, discID, 0, false) + Expect(err).ToNot(HaveOccurred()) + readAll(first) + Expect(first.ETag).ToNot(BeEmpty()) + + folderRepo.result[0].ImagesUpdatedAt = time.Now() + second, err := svc.Get(ctx, discID, 0, false) + Expect(err).ToNot(HaveOccurred()) + readAll(second) + Expect(second.ETag).ToNot(Equal(first.ETag), "a replaced image must not revalidate as unchanged") + }) + It("falls back to album art when no disc image matches", func() { folderRepo.result = nil albumRepo.SetData(model.Albums{{ID: "aldc2", Name: "Album"}}) diff --git a/server/imghttp/headers.go b/server/imghttp/headers.go index e7bb6d562..8f017d60a 100644 --- a/server/imghttp/headers.go +++ b/server/imghttp/headers.go @@ -28,7 +28,11 @@ func WriteImageHeaders(w http.ResponseWriter, r *http.Request, img *artwork.Imag if etag == "" { etag = img.Hash } - h.Set("ETag", `"`+etag+`"`) + // An empty validator is not one: emitting it would hand every such response the same ETag, + // and matching it would 304 a client that echoed it back even after the bytes changed. + if etag != "" { + h.Set("ETag", `"`+etag+`"`) + } if !img.LastUpdated.IsZero() { h.Set("Last-Modified", img.LastUpdated.UTC().Format(http.TimeFormat)) } @@ -40,7 +44,7 @@ func WriteImageHeaders(w http.ResponseWriter, r *http.Request, img *artwork.Imag h.Set("Cache-Control", "public, no-cache") } - if ifNoneMatch(r.Header.Get("If-None-Match"), etag) { + if etag != "" && ifNoneMatch(r.Header.Get("If-None-Match"), etag) { w.WriteHeader(http.StatusNotModified) return true } diff --git a/server/imghttp/headers_test.go b/server/imghttp/headers_test.go index 52a549556..80baae47d 100644 --- a/server/imghttp/headers_test.go +++ b/server/imghttp/headers_test.go @@ -41,6 +41,11 @@ func resized() *artwork.Image { } } +// unvalidated stands for a response with no content hash and no representation tag. +func unvalidated() *artwork.Image { + return &artwork.Image{ReadCloser: io.NopCloser(strings.NewReader("IMG")), LastUpdated: lastMod} +} + var _ = Describe("WriteImageHeaders", func() { type testCase struct { img *artwork.Image @@ -101,5 +106,13 @@ 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"}), + + // An image with neither ETag nor Hash has no validator. Emitting one would give every + // such response the same empty tag, and matching it would 304 changed bytes. + Entry("omits the ETag entirely when there is no validator", + testCase{img: unvalidated(), wantCache: "public, no-cache", wantLastMod: true}), + Entry("never 304s an empty validator echoed back by the client", + testCase{img: unvalidated(), ifNoneMatch: `""`, want304: false, + wantCache: "public, no-cache", wantLastMod: true}), ) })