From 76d4074ca1e2f4540c0601f98e01c4ca580e8d25 Mon Sep 17 00:00:00 2001 From: deluan Date: Fri, 20 Mar 2026 20:32:41 -0400 Subject: [PATCH] fix: distinguish transient failures from definitive misses in CAA cache MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit headCoverArt now returns (url, definitive) — transient failures (network errors, 5xx) are not cached, allowing immediate retries. Only definitive 404s are cached for caaCacheTTLMiss (4h). --- coverart.go | 32 ++++++++++++++++++++------------ coverart_test.go | 36 ++++++++++++++++++++++++++++-------- 2 files changed, 48 insertions(+), 20 deletions(-) diff --git a/coverart.go b/coverart.go index 8aa17c1..2f1bfe2 100644 --- a/coverart.go +++ b/coverart.go @@ -20,8 +20,10 @@ const ( ) // headCoverArt sends a HEAD request to the given CAA URL without following redirects. -// Returns the Location header value on 307 (image exists), or "" otherwise. -func headCoverArt(url string) string { +// Returns (location, true) on 307 with a Location header (image exists), +// ("", true) on 404 (definitive miss — safe to cache), +// ("", false) on network errors or unexpected responses (transient — do not cache). +func headCoverArt(url string) (string, bool) { resp, err := host.HTTPSend(host.HTTPRequest{ Method: "HEAD", URL: url, @@ -30,16 +32,20 @@ func headCoverArt(url string) string { }) if err != nil { pdk.Log(pdk.LogDebug, fmt.Sprintf("CAA HEAD request failed for %s: %v", url, err)) - return "" + return "", false + } + if resp.StatusCode == 404 { + return "", true } if resp.StatusCode != 307 { - return "" + pdk.Log(pdk.LogDebug, fmt.Sprintf("CAA HEAD unexpected status %d for %s", resp.StatusCode, url)) + return "", false } location := resp.Headers["Location"] if location == "" { pdk.Log(pdk.LogWarn, fmt.Sprintf("CAA returned 307 but no Location header for %s", url)) } - return location + return location, true } // getImageViaCoverArt checks the Cover Art Archive for album artwork. @@ -65,21 +71,23 @@ func getImageViaCoverArt(mbzAlbumID, mbzReleaseGroupID string) string { // Try release first var imageURL string + definitive := false if mbzAlbumID != "" { - imageURL = headCoverArt(fmt.Sprintf("https://coverartarchive.org/release/%s/front-500", mbzAlbumID)) + imageURL, definitive = headCoverArt(fmt.Sprintf("https://coverartarchive.org/release/%s/front-500", mbzAlbumID)) } // Fall back to release group if imageURL == "" && mbzReleaseGroupID != "" { - imageURL = headCoverArt(fmt.Sprintf("https://coverartarchive.org/release-group/%s/front-500", mbzReleaseGroupID)) + imageURL, definitive = headCoverArt(fmt.Sprintf("https://coverartarchive.org/release-group/%s/front-500", mbzReleaseGroupID)) } - // Cache the result (hit or miss) - ttl := caaCacheTTLHit - if imageURL == "" { - ttl = caaCacheTTLMiss + // Cache hits always; only cache misses if the response was definitive (404), + // not transient failures (network errors, 5xx) which should be retried sooner. + if imageURL != "" { + _ = host.CacheSetString(cacheKey, imageURL, caaCacheTTLHit) + } else if definitive { + _ = host.CacheSetString(cacheKey, "", caaCacheTTLMiss) } - _ = host.CacheSetString(cacheKey, imageURL, ttl) if imageURL != "" { pdk.Log(pdk.LogDebug, fmt.Sprintf("CAA resolved artwork for %s: %s", cacheKey, imageURL)) diff --git a/coverart_test.go b/coverart_test.go index d095c91..c6be6bf 100644 --- a/coverart_test.go +++ b/coverart_test.go @@ -20,7 +20,7 @@ var _ = Describe("headCoverArt", func() { pdk.PDKMock.On("Log", mock.Anything, mock.Anything).Maybe() }) - It("returns Location header on 307 response", func() { + It("returns Location header and definitive=true on 307 response", func() { host.HTTPMock.On("Send", mock.MatchedBy(func(req host.HTTPRequest) bool { return req.Method == "HEAD" && req.URL == "https://coverartarchive.org/release/test-mbid/front-500" && @@ -30,29 +30,32 @@ var _ = Describe("headCoverArt", func() { Headers: map[string]string{"Location": "https://archive.org/download/mbid-test/thumb500.jpg"}, }, nil) - result := headCoverArt("https://coverartarchive.org/release/test-mbid/front-500") + result, definitive := headCoverArt("https://coverartarchive.org/release/test-mbid/front-500") Expect(result).To(Equal("https://archive.org/download/mbid-test/thumb500.jpg")) + Expect(definitive).To(BeTrue()) }) - It("returns empty string on 404 response", func() { + It("returns empty and definitive=true on 404 response", func() { host.HTTPMock.On("Send", mock.MatchedBy(func(req host.HTTPRequest) bool { return req.Method == "HEAD" && req.NoFollowRedirects == true })).Return(&host.HTTPResponse{StatusCode: 404}, nil) - result := headCoverArt("https://coverartarchive.org/release/no-art/front-500") + result, definitive := headCoverArt("https://coverartarchive.org/release/no-art/front-500") Expect(result).To(BeEmpty()) + Expect(definitive).To(BeTrue()) }) - It("returns empty string on HTTP error", func() { + It("returns empty and definitive=false on HTTP error", func() { host.HTTPMock.On("Send", mock.MatchedBy(func(req host.HTTPRequest) bool { return req.Method == "HEAD" })).Return((*host.HTTPResponse)(nil), errors.New("connection refused")) - result := headCoverArt("https://coverartarchive.org/release/err/front-500") + result, definitive := headCoverArt("https://coverartarchive.org/release/err/front-500") Expect(result).To(BeEmpty()) + Expect(definitive).To(BeFalse()) }) - It("returns empty string when Location header is missing on 307", func() { + It("returns empty and definitive=true when Location header is missing on 307", func() { host.HTTPMock.On("Send", mock.MatchedBy(func(req host.HTTPRequest) bool { return req.Method == "HEAD" })).Return(&host.HTTPResponse{ @@ -60,8 +63,9 @@ var _ = Describe("headCoverArt", func() { Headers: map[string]string{}, }, nil) - result := headCoverArt("https://coverartarchive.org/release/no-location/front-500") + result, definitive := headCoverArt("https://coverartarchive.org/release/no-location/front-500") Expect(result).To(BeEmpty()) + Expect(definitive).To(BeTrue()) }) }) @@ -304,6 +308,22 @@ var _ = Describe("getImageViaCoverArt", func() { host.CacheMock.AssertCalled(GinkgoT(), "SetString", "caa.artwork.album-123", "", int64(14400)) }) + It("does not cache miss on transient failure", func() { + host.CacheMock.On("GetString", "caa.artwork.album-123").Return("", false, nil) + // Both requests fail with network errors (transient) + host.HTTPMock.On("Send", mock.MatchedBy(func(req host.HTTPRequest) bool { + return req.URL == "https://coverartarchive.org/release/album-123/front-500" + })).Return((*host.HTTPResponse)(nil), errors.New("connection refused")) + host.HTTPMock.On("Send", mock.MatchedBy(func(req host.HTTPRequest) bool { + return req.URL == "https://coverartarchive.org/release-group/rg-456/front-500" + })).Return((*host.HTTPResponse)(nil), errors.New("timeout")) + + result := getImageViaCoverArt("album-123", "rg-456") + Expect(result).To(BeEmpty()) + // Should NOT cache the miss since failures were transient + host.CacheMock.AssertNotCalled(GinkgoT(), "SetString", mock.Anything, mock.Anything, mock.Anything) + }) + It("tries only release-group when MBZAlbumID is empty", func() { host.CacheMock.On("GetString", "caa.artwork.rg.rg-456").Return("", false, nil) host.HTTPMock.On("Send", mock.MatchedBy(func(req host.HTTPRequest) bool {