diff --git a/backend/explore/listenbrainz.go b/backend/explore/listenbrainz.go index 725d47b..bc05729 100644 --- a/backend/explore/listenbrainz.go +++ b/backend/explore/listenbrainz.go @@ -13,6 +13,7 @@ import ( "net/http" "slices" "strings" + "sync/atomic" "time" ) @@ -25,6 +26,19 @@ const ( // responds with a non-2xx status code. var ErrListenBrainzHTTP = errors.New("listenbrainz HTTP error") +// ErrListenBrainzUnauthorized is a 401: the endpoint wants a token, and +// no retry will change that. +// +// It is separate from ErrListenBrainzHTTP because it is the one +// failure that is about *this client* rather than about the thing being +// asked for — which is what makes it the one worth latching. The +// popularity endpoints answered 401 to every request on 2026-10-05, so +// a discography backfill spent one rate-limited request per artist to +// be told the same thing: 399 of them in a minute of a real library's +// backfill, each one a log line and a wasted slot in the shared +// limiter. +var ErrListenBrainzUnauthorized = errors.New("listenbrainz requires a token") + // ListenBrainzClient is a thin HTTP client for the ListenBrainz // popularity and labs APIs. All requests are rate-limited via the // shared RateLimiter and cached via the shared Cache. @@ -39,6 +53,13 @@ type ListenBrainzClient struct { // SetBaseURL shape — so a test that points one client at an // httptest server does not stop being parallel-safe. baseURL string + + // refused latches the first 401. A token is a property of the + // installation, not of the artist being asked about, so the answer + // is the same for every later request and asking again is pure + // cost. Per client rather than global so a test can have one that + // is refused and one that is not. + refused atomic.Bool } // NewListenBrainzClient creates a ListenBrainz API client. @@ -56,6 +77,14 @@ func NewListenBrainzClient( } } +// Unauthorized reports whether this client has been refused with a 401 +// during its life. A caller that is about to do a long pass of +// requests should ask before starting it: the answer will not change +// mid-pass. +func (c *ListenBrainzClient) Unauthorized() bool { + return c.refused.Load() +} + // SetBaseURL redirects this client at another host. Tests only. func (c *ListenBrainzClient) SetBaseURL(url string) { c.baseURL = strings.TrimSuffix(url, "/") @@ -489,6 +518,11 @@ func (c *ListenBrainzClient) doPost( func (c *ListenBrainzClient) doRequest( ctx context.Context, method string, url string, body []byte, ) ([]byte, error) { + // Asked and answered, for the rest of this client's life. + if c.refused.Load() { + return nil, fmt.Errorf("%w: %s", ErrListenBrainzUnauthorized, url) + } + c.logger.Debug("listenbrainz rate limiter wait", "url", url) if err := c.limiter.Wait(ctx); err != nil { @@ -533,6 +567,21 @@ func (c *ListenBrainzClient) doRequest( "status", resp.StatusCode, ) + if resp.StatusCode == http.StatusUnauthorized { + // Recorded once, at warning level, because the next thing this + // client does is stop asking: a log line per artist is the + // symptom this latch exists to remove. + if c.refused.CompareAndSwap(false, true) { + c.logger.Warn("listenbrainz refused this client: "+ + "popularity data needs a token, so the rest of this "+ + "run will not ask for it", + "url", url, + ) + } + + return nil, fmt.Errorf("%w: %s", ErrListenBrainzUnauthorized, url) + } + if resp.StatusCode < 200 || resp.StatusCode >= 300 { return nil, fmt.Errorf( "%w: %d %s", ErrListenBrainzHTTP, resp.StatusCode, truncateBody(respBody), diff --git a/backend/explore/listenbrainz_refusal_test.go b/backend/explore/listenbrainz_refusal_test.go new file mode 100644 index 0000000..033fcf7 --- /dev/null +++ b/backend/explore/listenbrainz_refusal_test.go @@ -0,0 +1,90 @@ +package explore + +import ( + "context" + "errors" + "log/slog" + "net/http" + "net/http/httptest" + "sync/atomic" + "testing" + + "yellowjacket/backend/database" +) + +// #284: the popularity endpoints answered 401 to every request, so a +// backfill spent one rate-limited request per artist to be told the same +// thing — 399 of them in a minute against a real library. A token is a +// property of the installation, not of the artist, so the first refusal +// is the answer for the whole client. +func TestListenBrainzLatchesARefusal(t *testing.T) { + t.Parallel() + + var requests atomic.Int64 + + srv := httptest.NewServer(http.HandlerFunc( + func(w http.ResponseWriter, _ *http.Request) { + requests.Add(1) + w.WriteHeader(http.StatusUnauthorized) + }, + )) + t.Cleanup(srv.Close) + + c := NewListenBrainzClient( + NewRateLimiter(), NewCache(database.NewTestDB(t), slog.Default()), slog.Default(), + ) + c.SetBaseURL(srv.URL) + + for i := range 5 { + _, err := c.TopRecordingsForArtist(context.Background(), "an-mbid") + + if !errors.Is(err, ErrListenBrainzUnauthorized) { + t.Fatalf("call %d: err = %v, want ErrListenBrainzUnauthorized", i, err) + } + } + + if got := requests.Load(); got != 1 { + t.Errorf("requests = %d, want 1: the rest of the calls are the same answer", got) + } + + if !c.Unauthorized() { + t.Error("Unauthorized() = false after a 401") + } +} + +// A failure that a retry could fix must not latch: the artist stays +// unmarked and the next run asks again. +func TestListenBrainzDoesNotLatchATransientFailure(t *testing.T) { + t.Parallel() + + var requests atomic.Int64 + + srv := httptest.NewServer(http.HandlerFunc( + func(w http.ResponseWriter, _ *http.Request) { + requests.Add(1) + w.WriteHeader(http.StatusInternalServerError) + }, + )) + t.Cleanup(srv.Close) + + c := NewListenBrainzClient( + NewRateLimiter(), NewCache(database.NewTestDB(t), slog.Default()), slog.Default(), + ) + c.SetBaseURL(srv.URL) + + for range 3 { + _, err := c.TopRecordingsForArtist(context.Background(), "an-mbid") + + if !errors.Is(err, ErrListenBrainzHTTP) { + t.Fatalf("err = %v, want ErrListenBrainzHTTP", err) + } + } + + if got := requests.Load(); got != 3 { + t.Errorf("requests = %d, want 3", got) + } + + if c.Unauthorized() { + t.Error("Unauthorized() = true after a 500") + } +} diff --git a/backend/explore/searchindex.go b/backend/explore/searchindex.go index 4e4c7c5..4832a4c 100644 --- a/backend/explore/searchindex.go +++ b/backend/explore/searchindex.go @@ -518,6 +518,13 @@ func (si *SearchIndex) BackfillLibraryDiscographies(ctx context.Context) { break } + // A refused client is refused for every artist: the rest of + // this pass would be the same 401, once per artist (#284). The + // artists stay unmarked, so a run with a token picks them up. + if indexLB.Unauthorized() { + break + } + work <- mbid } @@ -534,6 +541,17 @@ func (si *SearchIndex) BackfillLibraryDiscographies(ctx context.Context) { return } + if indexLB.Unauthorized() { + si.logger.Warn("discography backfill stopped early: "+ + "listenbrainz refused this client, and a token is what it wants", + "artists", total, "of", len(mbids), + ) + + job.logf(jobs.LevelWarn, "Stopped early: ListenBrainz needs a token") + + return + } + job.logf(jobs.LevelInfo, "Filled in "+strconv.Itoa(total)+" artists") si.logger.Info("discography backfill complete", "artists", total)