From 13680221de7127bbb6f02d6cbb0e72fddbb0ccc2 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 8 May 2026 07:52:28 -0400 Subject: [PATCH] refactor(coverart/lastfm): delegate HTTP plumbing to httpClient (C1 1/3) --- internal/coverart/provider_lastfm.go | 161 +++------------------------ 1 file changed, 14 insertions(+), 147 deletions(-) diff --git a/internal/coverart/provider_lastfm.go b/internal/coverart/provider_lastfm.go index 9a253409..17564a70 100644 --- a/internal/coverart/provider_lastfm.go +++ b/internal/coverart/provider_lastfm.go @@ -2,15 +2,10 @@ package coverart import ( "context" - "encoding/json" - "fmt" - "io" - "math/rand" "net/http" "net/url" "path" "strings" - "sync" "sync/atomic" "time" ) @@ -44,16 +39,20 @@ var lastfmPlaceholderHashes = map[string]bool{ // operator-supplied API key — provider stays effectively disabled // (returns ErrNotFound on every call) when key is empty. type lastfmProvider struct { - enabled atomic.Bool - apiKey atomic.Pointer[string] - mu sync.Mutex - lastCall time.Time - client *http.Client + enabled atomic.Bool + apiKey atomic.Pointer[string] + client *httpClient } func init() { Register(&lastfmProvider{ - client: &http.Client{Timeout: 30 * time.Second}, + client: newHTTPClient(httpClientOptions{ + Name: "lastfm", + HTTPClient: &http.Client{Timeout: 30 * time.Second}, + MinInterval: lastfmMinPeriod, + MaxRetries: lastfmMaxRetries, + TreatAuthAsTransient: true, + }), }) } @@ -112,7 +111,7 @@ func (p *lastfmProvider) FetchArtistArt(ctx context.Context, ref ArtistRef) (thu } `json:"artist"` Error int `json:"error"` // present on error responses } - if err := p.getJSON(ctx, q, &resp); err != nil { + if err := p.client.getJSON(ctx, lastfmBaseURL+"?"+q.Encode(), &resp); err != nil { return nil, nil, err } if resp.Error != 0 || resp.Artist.Name == "" { @@ -122,7 +121,7 @@ func (p *lastfmProvider) FetchArtistArt(ctx context.Context, ref ArtistRef) (thu if imgURL == "" || isLastfmPlaceholder(imgURL) { return nil, nil, ErrNotFound } - img, ferr := p.getImage(ctx, imgURL) + img, ferr := p.client.getImage(ctx, imgURL) if ferr != nil { return nil, nil, ferr } @@ -158,7 +157,7 @@ func (p *lastfmProvider) FetchAlbumCover(ctx context.Context, ref AlbumRef) ([]b } `json:"album"` Error int `json:"error"` } - if err := p.getJSON(ctx, q, &resp); err != nil { + if err := p.client.getJSON(ctx, lastfmBaseURL+"?"+q.Encode(), &resp); err != nil { return nil, err } if resp.Error != 0 || resp.Album.Name == "" { @@ -168,7 +167,7 @@ func (p *lastfmProvider) FetchAlbumCover(ctx context.Context, ref AlbumRef) ([]b if imgURL == "" || isLastfmPlaceholder(imgURL) { return nil, ErrNotFound } - return p.getImage(ctx, imgURL) + return p.client.getImage(ctx, imgURL) } // lastfmImage models one entry of Last.fm's image[] array. @@ -210,138 +209,6 @@ func isLastfmPlaceholder(rawURL string) bool { return lastfmPlaceholderHashes[strings.ToLower(base)] } -// getJSON performs a rate-limited GET against Last.fm's JSON API. -// Retries on 429 / 5xx with exponential backoff up to lastfmMaxRetries. -func (p *lastfmProvider) getJSON(ctx context.Context, q url.Values, out any) error { - urlStr := lastfmBaseURL + "?" + q.Encode() - for attempt := 0; attempt <= lastfmMaxRetries; attempt++ { - if err := p.rateLimitWait(ctx); err != nil { - return err - } - req, err := http.NewRequestWithContext(ctx, http.MethodGet, urlStr, nil) - if err != nil { - return fmt.Errorf("coverart lastfm: build request: %w", err) - } - req.Header.Set("Accept", "application/json") - - resp, err := p.client.Do(req) - if err != nil { - if attempt < lastfmMaxRetries { - if waitErr := p.backoff(ctx, attempt); waitErr != nil { - return waitErr - } - continue - } - return fmt.Errorf("%w: %v", ErrTransient, err) - } - - switch { - case resp.StatusCode == http.StatusNotFound: - _ = resp.Body.Close() - return ErrNotFound - case resp.StatusCode == http.StatusUnauthorized, - resp.StatusCode == http.StatusForbidden: - _ = resp.Body.Close() - return fmt.Errorf("%w: status %d (auth)", ErrTransient, resp.StatusCode) - case resp.StatusCode == http.StatusTooManyRequests || resp.StatusCode >= 500: - _ = resp.Body.Close() - if attempt < lastfmMaxRetries { - if waitErr := p.backoff(ctx, attempt); waitErr != nil { - return waitErr - } - continue - } - return fmt.Errorf("%w: status %d", ErrTransient, resp.StatusCode) - case resp.StatusCode != http.StatusOK: - _ = resp.Body.Close() - return fmt.Errorf("%w: status %d", ErrTransient, resp.StatusCode) - } - - body, err := io.ReadAll(io.LimitReader(resp.Body, 1*1024*1024)) - _ = resp.Body.Close() - if err != nil { - return fmt.Errorf("%w: read body: %v", ErrTransient, err) - } - if err := json.Unmarshal(body, out); err != nil { - return fmt.Errorf("%w: decode body: %v", ErrTransient, err) - } - return nil - } - return fmt.Errorf("%w: exhausted retries", ErrTransient) -} - -// getImage performs a rate-limited GET of an image URL. 404 → ErrNotFound; -// 5xx / network → ErrTransient. -func (p *lastfmProvider) getImage(ctx context.Context, urlStr string) ([]byte, error) { - if err := p.rateLimitWait(ctx); err != nil { - return nil, err - } - req, err := http.NewRequestWithContext(ctx, http.MethodGet, urlStr, nil) - if err != nil { - return nil, fmt.Errorf("coverart lastfm: build request: %w", err) - } - req.Header.Set("Accept", "image/*") - resp, err := p.client.Do(req) - if err != nil { - return nil, fmt.Errorf("%w: %v", ErrTransient, err) - } - defer func() { _ = resp.Body.Close() }() - switch { - case resp.StatusCode == http.StatusNotFound: - return nil, ErrNotFound - case resp.StatusCode != http.StatusOK: - return nil, fmt.Errorf("%w: status %d", ErrTransient, resp.StatusCode) - } - body, err := io.ReadAll(io.LimitReader(resp.Body, 5*1024*1024)) - if err != nil { - return nil, fmt.Errorf("%w: read body: %v", ErrTransient, err) - } - return body, nil -} - -// rateLimitWait blocks until at least lastfmMinPeriod has elapsed since -// the last call, then claims the slot. -func (p *lastfmProvider) rateLimitWait(ctx context.Context) error { - p.mu.Lock() - wait := time.Until(p.lastCall.Add(lastfmMinPeriod)) - if wait > 0 { - timer := time.NewTimer(wait) - p.mu.Unlock() - select { - case <-ctx.Done(): - timer.Stop() - return ctx.Err() - case <-timer.C: - } - p.mu.Lock() - } - p.lastCall = time.Now() - p.mu.Unlock() - return nil -} - -// backoff sleeps for 2^attempt × 250ms ± 20% jitter. -func (p *lastfmProvider) backoff(ctx context.Context, attempt int) error { - base := time.Duration(250*(1<