fix: distinguish transient failures from definitive misses in CAA cache

headCoverArt now returns (url, definitive) — transient failures
(network errors, 5xx) are not cached, allowing immediate retries.
Only definitive 404s are cached for caaCacheTTLMiss (4h).
This commit is contained in:
deluan
2026-03-20 20:32:41 -04:00
parent 357f922070
commit 76d4074ca1
2 changed files with 48 additions and 20 deletions
+20 -12
View File
@@ -20,8 +20,10 @@ const (
) )
// headCoverArt sends a HEAD request to the given CAA URL without following redirects. // headCoverArt sends a HEAD request to the given CAA URL without following redirects.
// Returns the Location header value on 307 (image exists), or "" otherwise. // Returns (location, true) on 307 with a Location header (image exists),
func headCoverArt(url string) string { // ("", 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{ resp, err := host.HTTPSend(host.HTTPRequest{
Method: "HEAD", Method: "HEAD",
URL: url, URL: url,
@@ -30,16 +32,20 @@ func headCoverArt(url string) string {
}) })
if err != nil { if err != nil {
pdk.Log(pdk.LogDebug, fmt.Sprintf("CAA HEAD request failed for %s: %v", url, err)) 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 { 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"] location := resp.Headers["Location"]
if location == "" { if location == "" {
pdk.Log(pdk.LogWarn, fmt.Sprintf("CAA returned 307 but no Location header for %s", url)) 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. // getImageViaCoverArt checks the Cover Art Archive for album artwork.
@@ -65,21 +71,23 @@ func getImageViaCoverArt(mbzAlbumID, mbzReleaseGroupID string) string {
// Try release first // Try release first
var imageURL string var imageURL string
definitive := false
if mbzAlbumID != "" { 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 // Fall back to release group
if imageURL == "" && mbzReleaseGroupID != "" { 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) // Cache hits always; only cache misses if the response was definitive (404),
ttl := caaCacheTTLHit // not transient failures (network errors, 5xx) which should be retried sooner.
if imageURL == "" { if imageURL != "" {
ttl = caaCacheTTLMiss _ = host.CacheSetString(cacheKey, imageURL, caaCacheTTLHit)
} else if definitive {
_ = host.CacheSetString(cacheKey, "", caaCacheTTLMiss)
} }
_ = host.CacheSetString(cacheKey, imageURL, ttl)
if imageURL != "" { if imageURL != "" {
pdk.Log(pdk.LogDebug, fmt.Sprintf("CAA resolved artwork for %s: %s", cacheKey, imageURL)) pdk.Log(pdk.LogDebug, fmt.Sprintf("CAA resolved artwork for %s: %s", cacheKey, imageURL))
+28 -8
View File
@@ -20,7 +20,7 @@ var _ = Describe("headCoverArt", func() {
pdk.PDKMock.On("Log", mock.Anything, mock.Anything).Maybe() 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 { host.HTTPMock.On("Send", mock.MatchedBy(func(req host.HTTPRequest) bool {
return req.Method == "HEAD" && return req.Method == "HEAD" &&
req.URL == "https://coverartarchive.org/release/test-mbid/front-500" && 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"}, Headers: map[string]string{"Location": "https://archive.org/download/mbid-test/thumb500.jpg"},
}, nil) }, 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(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 { host.HTTPMock.On("Send", mock.MatchedBy(func(req host.HTTPRequest) bool {
return req.Method == "HEAD" && req.NoFollowRedirects == true return req.Method == "HEAD" && req.NoFollowRedirects == true
})).Return(&host.HTTPResponse{StatusCode: 404}, nil) })).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(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 { host.HTTPMock.On("Send", mock.MatchedBy(func(req host.HTTPRequest) bool {
return req.Method == "HEAD" return req.Method == "HEAD"
})).Return((*host.HTTPResponse)(nil), errors.New("connection refused")) })).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(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 { host.HTTPMock.On("Send", mock.MatchedBy(func(req host.HTTPRequest) bool {
return req.Method == "HEAD" return req.Method == "HEAD"
})).Return(&host.HTTPResponse{ })).Return(&host.HTTPResponse{
@@ -60,8 +63,9 @@ var _ = Describe("headCoverArt", func() {
Headers: map[string]string{}, Headers: map[string]string{},
}, nil) }, 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(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)) 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() { It("tries only release-group when MBZAlbumID is empty", func() {
host.CacheMock.On("GetString", "caa.artwork.rg.rg-456").Return("", false, nil) host.CacheMock.On("GetString", "caa.artwork.rg.rg-456").Return("", false, nil)
host.HTTPMock.On("Send", mock.MatchedBy(func(req host.HTTPRequest) bool { host.HTTPMock.On("Send", mock.MatchedBy(func(req host.HTTPRequest) bool {