Merge remote-tracking branch 'origin/master' into fix/artwork-no-absent-retry

adapters/deezer/client.go

@@ -13,15 +13,26 @@ import (
 	"strings"
 
 	"github.com/microcosm-cc/bluemonday"
+	"github.com/navidrome/navidrome/core/agents"
 	"github.com/navidrome/navidrome/log"
 )
 
 const apiBaseURL = "https://api.deezer.com"
 const authBaseURL = "https://auth.deezer.com"
 
-var (
-	ErrNotFound = errors.New("deezer: not found")
-)
+// errCodeQuota is Deezer's "Quota limit exceeded"; it arrives in the body, with HTTP 200
+// and no rate-limit headers, so the body code is the only signal.
+const errCodeQuota = 4
+
+type deezerError struct {
+	Type    string `json:"type"`
+	Message string `json:"message"`
+	Code    int    `json:"code"`
+}
+
+func (e *deezerError) Error() string {
+	return fmt.Sprintf("deezer error(%d): %s", e.Code, e.Message)
+}
 
 type httpDoer interface {
 	Do(req *http.Request) (*http.Response, error)
@@ -56,7 +67,7 @@ func (c *client) searchArtists(ctx context.Context, name string, limit int) ([]A
 	}
 
 	if len(results.Data) == 0 {
-		return nil, ErrNotFound
+		return nil, agents.ErrNotFound
 	}
 	return results.Data, nil
 }
@@ -74,20 +85,31 @@ func (c *client) makeRequest(req *http.Request, response any) error {
 		return err
 	}
 
+	// Checked before the status: a throttled request still answers 200, and decoding its body
+	// into a result type yields an empty one, which reads as "nothing found".
+	if err := parseBodyError(data); err != nil {
+		return err
+	}
 	if resp.StatusCode != 200 {
-		return c.parseError(data)
+		return fmt.Errorf("deezer http status: (%d)", resp.StatusCode)
 	}
 
 	return json.Unmarshal(data, response)
 }
 
-func (c *client) parseError(data []byte) error {
-	var deezerError Error
-	err := json.Unmarshal(data, &deezerError)
-	if err != nil {
-		return err
+// parseBodyError returns the error Deezer reported in the body, or nil when it reported none.
+func parseBodyError(data []byte) error {
+	var body errorResponse
+	// Discarded: a payload that is not an error object leaves Error nil, which is the "none" answer.
+	_ = json.Unmarshal(data, &body)
+	switch {
+	case body.Error == nil:
+		return nil
+	case body.Error.Code == errCodeQuota:
+		return errors.Join(body.Error, agents.ErrRetryLater)
+	default:
+		return body.Error
 	}
-	return fmt.Errorf("deezer error(%d): %s", deezerError.Error.Code, deezerError.Error.Message)
 }
 
 func (c *client) getRelatedArtists(ctx context.Context, artistID int) ([]Artist, error) {

adapters/deezer/client_test.go

@@ -2,12 +2,14 @@ package deezer
 
 import (
 	"bytes"
+	"errors"
 	"fmt"
 	"io"
 	"net/http"
 	"os"
 	"time"
 
+	"github.com/navidrome/navidrome/core/agents"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 )
@@ -41,7 +43,37 @@ var _ = Describe("client", func() {
 			})
 
 			_, err := client.searchArtists(GinkgoT().Context(), "Michael Jackson", 20)
-			Expect(err).To(MatchError(ErrNotFound))
+			Expect(err).To(MatchError(agents.ErrNotFound))
+		})
+
+		// Deezer answers 200 with no rate-limit headers when throttling, so this body is the only signal.
+		It("reports an exhausted quota as a retryable error, not as a missing artist", func() {
+			httpClient.mock("https://api.deezer.com/search/artist", http.Response{
+				StatusCode: 200,
+				Body: io.NopCloser(bytes.NewBufferString(
+					`{"error":{"type":"Exception","message":"Quota limit exceeded","code":4}}`)),
+			})
+
+			_, err := client.searchArtists(GinkgoT().Context(), "Michael Jackson", 20)
+			Expect(err).To(HaveOccurred())
+			Expect(err).ToNot(MatchError(agents.ErrNotFound),
+				"a throttled lookup would otherwise settle the artist as having no image")
+			Expect(errors.Is(err, agents.ErrRetryLater)).To(BeTrue())
+			Expect(err.Error()).To(ContainSubstring("Quota limit exceeded"))
+		})
+
+		It("reports a non-quota body error as a plain error", func() {
+			httpClient.mock("https://api.deezer.com/search/artist", http.Response{
+				StatusCode: 200,
+				Body: io.NopCloser(bytes.NewBufferString(
+					`{"error":{"type":"Exception","message":"Invalid query","code":100}}`)),
+			})
+
+			_, err := client.searchArtists(GinkgoT().Context(), "Michael Jackson", 20)
+			Expect(err).To(HaveOccurred())
+			Expect(err).ToNot(MatchError(agents.ErrNotFound))
+			Expect(errors.Is(err, agents.ErrRetryLater)).To(BeFalse(),
+				"only a throttle asks the caller to come back later")
 		})
 	})
 

adapters/deezer/deezer.go

@@ -91,9 +91,6 @@ func isPlaceholderPicture(url string) bool {
 
 func (s *deezerAgent) searchArtist(ctx context.Context, name string) (*Artist, error) {
 	artists, err := s.client.searchArtists(ctx, name, deezerArtistSearchLimit)
-	if errors.Is(err, ErrNotFound) || len(artists) == 0 {
-		return nil, agents.ErrNotFound
-	}
 	if err != nil {
 		return nil, err
 	}

adapters/deezer/deezer_test.go

@@ -3,6 +3,7 @@ package deezer
 import (
 	"bytes"
 	"context"
+	"errors"
 	"fmt"
 	"io"
 	"net/http"
@@ -80,6 +81,22 @@ var _ = Describe("deezerAgent", func() {
 			Expect(artist.ID).To(Equal(2))
 		})
 
+		// The artwork worker settles an artist as "no image" on agents.ErrNotFound, so a throttled
+		// lookup reaching that here would record a permanent absence.
+		It("surfaces an exhausted quota instead of reporting the artist as not found", func() {
+			httpClient.mock("https://api.deezer.com/search/artist", http.Response{
+				StatusCode: 200,
+				Body: io.NopCloser(bytes.NewBufferString(
+					`{"error":{"type":"Exception","message":"Quota limit exceeded","code":4}}`)),
+			})
+
+			_, err := agent.searchArtist(ctx, "Queen")
+
+			Expect(err).To(HaveOccurred())
+			Expect(err).ToNot(MatchError(agents.ErrNotFound))
+			Expect(errors.Is(err, agents.ErrRetryLater)).To(BeTrue())
+		})
+
 		It("returns ErrNotFound when no result matches the name exactly", func() {
 			httpClient.mock("https://api.deezer.com/search/artist", http.Response{
 				StatusCode: 200,

adapters/deezer/responses.go

@@ -22,12 +22,8 @@ type Artist struct {
 	Type          string `json:"type"`
 }
 
-type Error struct {
-	Error struct {
-		Type    string `json:"type"`
-		Message string `json:"message"`
-		Code    int    `json:"code"`
-	} `json:"error"`
+type errorResponse struct {
+	Error *deezerError `json:"error"`
 }
 
 type RelatedArtists struct {

adapters/deezer/responses_test.go

@@ -26,7 +26,7 @@ var _ = Describe("Responses", func() {
 
 	Describe("Error", func() {
 		It("parses the error response correctly", func() {
-			var errorResp Error
+			var errorResp errorResponse
 			body := []byte(`{"error":{"type":"MissingParameterException","message":"Missing parameters: q","code":501}}`)
 			err := json.Unmarshal(body, &errorResp)
 			Expect(err).To(BeNil())

core/scrobbler/buffered_scrobbler_test.go

@@ -251,12 +251,13 @@ func TestBufferedScrobblerTakesTheLongestServerDelayAcrossUsers(t *testing.T) {
 			"user1": 10 * time.Second,
 			"user2": 45 * time.Second,
 		}}
+		// Both are buffered before the drain goroutine exists: it drains once on startup, and
+		// seeing only one user there would park it on that user's delay, ignoring the other.
+		_ = buffer.Enqueue("test", "user1", "1", time.Now())
+		_ = buffer.Enqueue("test", "user2", "2", time.Now())
 		bs := newBufferedScrobbler(ds, scr, "test")
 		defer bs.Stop()
 
-		// user2 is enqueued directly so both are buffered before the first drain wakes.
-		_ = buffer.Enqueue("test", "user2", "2", time.Now())
-		_ = bs.Scrobble(context.Background(), "user1", Scrobble{MediaFile: model.MediaFile{ID: "1"}, TimeStamp: time.Now()})
 		synctest.Wait()
 		if got := scr.count.Load(); got != 2 {
 			t.Fatalf("expected both users drained, got %d attempts", got)