fix(explore): stop asking ListenBrainz after it refuses this client
Every popularity request answered 401 on 2026-10-05: 399 in one minute of a real library's discography backfill, one rate-limited request per artist to be told the same thing, each with its own log line. A token is a property of the installation, not of the artist, so the first 401 is the answer for the rest of that client's life. The refusal is latched, logged once at warning level, and returned as ErrListenBrainzUnauthorized — separate from the generic HTTP error because it is the one failure no retry can fix. A 500 still does not latch, so a transient failure leaves the artist unmarked and the next run asks again. The backfill also stops feeding artists once the client is refused: the rest of the pass would be the same 401, and the artists stay unmarked for a run that has a token. Closes #284
This commit is contained in:
1 parent
c9b3048a20
commit
bce8d27370
3 files changed
+157
No files matched your search
@@ -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),
|
||||
|
||||
@@ -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")
|
||||
}
|
||||
}
|
||||
@@ -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)
|
||||
|
||||
Reference in new issue
Block a user