diff --git a/cmd/minstrel/main.go b/cmd/minstrel/main.go index cb7fffcb..bc3aa891 100644 --- a/cmd/minstrel/main.go +++ b/cmd/minstrel/main.go @@ -77,11 +77,20 @@ func run() error { if contact == "" { contact = "https://git.fabledsword.com/bvandeusen/minstrel" } - coverFetcher := coverart.NewFetcher(coverart.FetcherConfig{ + // Swap in the production User-Agent + MinPeriod on the registered + // MBCAA provider. init() registered it with default config; this + // updates the running instance. + coverart.NewMBCAAProviderFromConfig(coverart.FetcherConfig{ UserAgent: fmt.Sprintf("Minstrel/dev (%s)", contact), MinPeriod: time.Second, }) - coverEnricher := coverart.NewEnricher(pool, logger.With("component", "coverart"), coverFetcher, cfg.Library.CoverArtFromMBCAA) + + coverSettings, err := coverart.NewSettingsService(ctx, pool, logger.With("component", "coverart")) + if err != nil { + logger.Error("coverart settings service init failed", "err", err) + os.Exit(1) + } + coverEnricher := coverart.NewEnricher(pool, logger.With("component", "coverart"), coverSettings) // One unified scan chain: library walk → MBID backfill → cover enrich. // Boot-time scan and manual-trigger scans share this path; results land diff --git a/internal/api/admin_covers_test.go b/internal/api/admin_covers_test.go index 28f197e7..62f65a12 100644 --- a/internal/api/admin_covers_test.go +++ b/internal/api/admin_covers_test.go @@ -40,13 +40,17 @@ func doAdminCoversReq(t *testing.T, h *handlers, method, path string, user dbq.U } // testHandlersWithCovers returns a handlers instance with a stub coverart -// Enricher wired in. The stub uses mbcaaOn=false + nil fetcher so the +// Enricher wired in. No external providers are registered, so the // sidecar-only path is exercised and cover_art_source lands as 'none' for // albums without real track files. func testHandlersWithCovers(t *testing.T) (*handlers, *coverart.Enricher) { t.Helper() h, pool := testHandlers(t) - enricher := coverart.NewEnricher(pool, slog.Default(), nil, false) + settings, err := coverart.NewSettingsService(t.Context(), pool, slog.Default()) + if err != nil { + t.Fatalf("NewSettingsService: %v", err) + } + enricher := coverart.NewEnricher(pool, slog.Default(), settings) h.coverart = enricher h.coverArtBackfillCap = 100 return h, enricher diff --git a/internal/coverart/enricher.go b/internal/coverart/enricher.go index 5eb3bfb1..d33ed266 100644 --- a/internal/coverart/enricher.go +++ b/internal/coverart/enricher.go @@ -15,25 +15,36 @@ import ( "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" ) -// Enricher orchestrates the sidecar → MBCAA → record cycle for one or many -// albums. Owns no DB pool itself; receives one per call so the boot -// backfill, post-scan trigger, and admin endpoints share state. +// Enricher orchestrates the sidecar → provider chain → record cycle +// for one or many albums. Owns no DB pool itself; receives one per +// call so the boot backfill, post-scan trigger, and admin endpoints +// share state. type Enricher struct { - pool *pgxpool.Pool - logger *slog.Logger - fetcher *Fetcher - mbcaaOn bool + pool *pgxpool.Pool + logger *slog.Logger + settings *SettingsService } -// NewEnricher constructs an Enricher. Pass mbcaaOn=false to disable upstream -// fetches entirely (sidecar-only mode for paranoid operators). -func NewEnricher(pool *pgxpool.Pool, logger *slog.Logger, fetcher *Fetcher, mbcaaOn bool) *Enricher { - return &Enricher{pool: pool, logger: logger, fetcher: fetcher, mbcaaOn: mbcaaOn} +// NewEnricher constructs an Enricher. +func NewEnricher(pool *pgxpool.Pool, logger *slog.Logger, settings *SettingsService) *Enricher { + return &Enricher{pool: pool, logger: logger, settings: settings} } -// EnrichAlbum runs the chain for a single album. Idempotent: skips when -// cover_art_source is already set to a terminal value (sidecar/embedded/ -// mbcaa/none). The admin retry endpoint clears the field before calling. +// EnrichAlbum runs the chain for a single album. +// +// Eligibility: +// - cover_art_source IS NULL → eligible +// - cover_art_source IN (sidecar/embedded/ → SKIP (found; version irrelevant) +// mbcaa/theaudiodb) +// - cover_art_source = 'none' → flip to NULL via ClearAlbumCoverNone, +// then enrich (the DB-level query ListAlbumsMissingCover already +// guards the version check for batch callers; direct callers such +// as RetryAlbum clear via ClearAlbumCover first so we never see +// a current-version 'none' in normal flow) +// +// Chain: sidecar → enabled AlbumCoverProviders in registration order. +// First success returns. All-ErrNotFound settles to 'none'. Any +// ErrTransient leaves the row NULL for next-pass retry. func (e *Enricher) EnrichAlbum(ctx context.Context, albumID pgtype.UUID) error { q := dbq.New(e.pool) row, err := q.GetAlbumWithFirstTrackPath(ctx, albumID) @@ -44,98 +55,131 @@ func (e *Enricher) EnrichAlbum(ctx context.Context, albumID pgtype.UUID) error { return fmt.Errorf("get album: %w", err) } - // Skip when already settled. NULL is the eligible state. - current := "" + currentVersion := e.settings.CurrentVersion() + + // Eligibility check. + currentSource := "" if row.CoverArtSource != nil { - current = *row.CoverArtSource + currentSource = *row.CoverArtSource } - switch current { - case "sidecar", "embedded", "mbcaa", "none": - return nil + switch currentSource { + case "sidecar", "embedded", "mbcaa", "theaudiodb": + return nil // already found; version irrelevant + case "none": + // Flip to NULL and proceed. ListAlbumsMissingCover already + // filters to stale-version 'none' rows for batch callers; + // RetryAlbum clears to NULL before calling us. + if err := q.ClearAlbumCoverNone(ctx, albumID); err != nil { + return fmt.Errorf("clear stale none: %w", err) + } } - // Step 1: sidecar lookup. + // Sidecar layer (always tried first; not a Provider). if row.TrackFilePath != nil && *row.TrackFilePath != "" { albumDir := filepath.Dir(*row.TrackFilePath) if path := FindSidecar(albumDir); path != "" { - return e.recordCover(ctx, albumID, path, "sidecar") + return e.recordAlbumCover(ctx, albumID, path, "sidecar", currentVersion) } } - // Step 2: MBCAA fallback. Requires fetcher + toggle on + MBID. - // When the toggle is disabled OR the album has no MBID, leave - // cover_art_source NULL so a future scan can retry once the - // scanner heals the MBID. 'none' is reserved for genuine MBCAA - // 404s where we know the album really isn't in the archive. - if !e.mbcaaOn || e.fetcher == nil { - return nil - } + // MBID guard — providers can't fetch without one. if row.Mbid == nil || *row.Mbid == "" { - return nil + return nil // leave NULL; mbid backfill or future scan can heal } + mbid := *row.Mbid - body, err := e.fetcher.Fetch(ctx, *row.Mbid) - if err != nil { - if errors.Is(err, ErrNotFound) { - return e.recordCover(ctx, albumID, "", "none") + // Provider chain. allWere404 tracks whether every provider + // returned ErrNotFound — only then do we settle to 'none'. Any + // transient failure leaves the row NULL for next-pass retry. + allWere404 := true + for _, provider := range e.settings.EnabledAlbumProviders() { + body, perr := provider.FetchAlbumCover(ctx, mbid) + if perr == nil { + if row.TrackFilePath == nil || *row.TrackFilePath == "" { + // Defensive: shouldn't happen if MBID was set on an + // album with no tracks, but record 'none' rather than + // crash. + return e.recordAlbumCover(ctx, albumID, "", "none", currentVersion) + } + albumDir := filepath.Dir(*row.TrackFilePath) + dest := filepath.Join(albumDir, "cover.jpg") + if werr := os.WriteFile(dest, body, 0o644); werr != nil { + e.logger.Error("coverart: write file failed; leaving NULL", + "album_id", uuidString(albumID), "path", dest, + "provider", provider.ID(), "err", werr) + return nil + } + return e.recordAlbumCover(ctx, albumID, dest, provider.ID(), currentVersion) } - // ErrTransient or context cancellation: leave NULL for retry. - e.logger.Warn("coverart: fetch failed; leaving for retry", - "album_id", uuidString(albumID), "err", err) - return nil + if !errors.Is(perr, ErrNotFound) { + allWere404 = false + e.logger.Warn("coverart: provider fetch failed; trying next", + "album_id", uuidString(albumID), + "provider", provider.ID(), "err", perr) + } + // ErrNotFound: continue to next provider; allWere404 stays true. } - // Step 3: write to album dir as cover.jpg, record path + 'mbcaa'. - if row.TrackFilePath == nil || *row.TrackFilePath == "" { - // Defensive: shouldn't happen if MBID was set on an album with - // no tracks, but record 'none' rather than crash. - return e.recordCover(ctx, albumID, "", "none") + if allWere404 { + return e.recordAlbumCover(ctx, albumID, "", "none", currentVersion) } - albumDir := filepath.Dir(*row.TrackFilePath) - dest := filepath.Join(albumDir, "cover.jpg") - if err := os.WriteFile(dest, body, 0o644); err != nil { - e.logger.Error("coverart: write file failed; leaving NULL", - "album_id", uuidString(albumID), "path", dest, "err", err) - return nil - } - return e.recordCover(ctx, albumID, dest, "mbcaa") + return nil // at least one transient failure; leave NULL for retry } -// EnrichBatch drains up to limit albums whose cover_art_source is NULL. -// Iterates serially — the fetcher's rate limiter prevents bursts. -// Returns per-batch tallies for the orchestrator's scan-run record. -// -// "succeeded" means the album ended up with a non-NULL cover_art_source -// other than 'none' (i.e. we found art via sidecar or MBCAA). "failed" -// counts both EnrichAlbum errors and rows that ended at 'none'. +// recordAlbumCover writes the new cover_art_path / cover_art_source / +// cover_art_sources_version atomically. +func (e *Enricher) recordAlbumCover(ctx context.Context, albumID pgtype.UUID, path, source string, version int32) error { + q := dbq.New(e.pool) + src := source + var pathArg *string + if path != "" { + pathArg = &path + } + return q.SetAlbumCoverWithVersion(ctx, dbq.SetAlbumCoverWithVersionParams{ + ID: albumID, + CoverArtPath: pathArg, + CoverArtSource: &src, + CoverArtSourcesVersion: version, + }) +} + +// EnrichBatch drains up to limit albums whose eligibility loop +// determines they need processing. Iterates serially. Returns +// per-batch tallies for the orchestrator's scan-run record. func (e *Enricher) EnrichBatch(ctx context.Context, limit int) (processed, succeeded, failed int, err error) { if limit <= 0 { return 0, 0, 0, nil } q := dbq.New(e.pool) - rows, qerr := q.ListAlbumsMissingCover(ctx, int32(limit)) + rows, qerr := q.ListAlbumsMissingCover(ctx, dbq.ListAlbumsMissingCoverParams{ + CoverArtSourcesVersion: e.settings.CurrentVersion(), + Limit: int32(limit), + }) if qerr != nil { return 0, 0, 0, fmt.Errorf("list missing: %w", qerr) } - for _, r := range rows { + for _, id := range rows { if ctx.Err() != nil { return processed, succeeded, failed, ctx.Err() } processed++ - if eerr := e.EnrichAlbum(ctx, r.ID); eerr != nil { + if eerr := e.EnrichAlbum(ctx, id); eerr != nil { e.logger.Warn("coverart: batch entry failed", - "album_id", uuidString(r.ID), "err", eerr) + "album_id", uuidString(id), "err", eerr) failed++ continue } - // Re-read the source to classify the outcome. - row, rerr := q.GetAlbumWithFirstTrackPath(ctx, r.ID) + // Re-read to classify. + row, rerr := q.GetAlbumWithFirstTrackPath(ctx, id) if rerr != nil { failed++ continue } if row.CoverArtSource != nil && - (*row.CoverArtSource == "sidecar" || *row.CoverArtSource == "mbcaa" || *row.CoverArtSource == "embedded") { + (*row.CoverArtSource == "sidecar" || + *row.CoverArtSource == "mbcaa" || + *row.CoverArtSource == "embedded" || + *row.CoverArtSource == "theaudiodb") { succeeded++ } else { failed++ @@ -144,8 +188,9 @@ func (e *Enricher) EnrichBatch(ctx context.Context, limit int) (processed, succe return processed, succeeded, failed, nil } -// EnrichRetryMissing drains both NULL and 'none' rows. Used by the admin -// bulk-retry endpoint. Clears 'none' to NULL first so EnrichAlbum picks them up. +// EnrichRetryMissing drains both NULL and 'none' rows. Used by the +// admin bulk-retry endpoint. Clears 'none' to NULL first so +// EnrichAlbum picks them up. func (e *Enricher) EnrichRetryMissing(ctx context.Context, limit int) (int, error) { if limit <= 0 { return 0, nil @@ -175,9 +220,7 @@ func (e *Enricher) EnrichRetryMissing(ctx context.Context, limit int) (int, erro return processed, nil } -// RetryAlbum is the admin per-album retry path. Clears the album's cover -// state, deletes the file if we wrote it (cover_art_source = 'mbcaa'), -// then re-runs EnrichAlbum synchronously. +// RetryAlbum is the admin per-album retry path. func (e *Enricher) RetryAlbum(ctx context.Context, albumID pgtype.UUID) error { q := dbq.New(e.pool) row, err := q.GetAlbumWithFirstTrackPath(ctx, albumID) @@ -187,14 +230,16 @@ func (e *Enricher) RetryAlbum(ctx context.Context, albumID pgtype.UUID) error { } return fmt.Errorf("get album: %w", err) } - // Delete file only when we previously wrote it (mbcaa source). Operator - // sidecars stay; embedded extracts (future) live in a managed cache that - // has its own lifecycle. - if row.CoverArtSource != nil && *row.CoverArtSource == "mbcaa" && + // Delete the file only if we previously wrote it (mbcaa or + // theaudiodb). Operator sidecars stay; embedded extracts (future) + // live in a managed cache with its own lifecycle. + if row.CoverArtSource != nil && + (*row.CoverArtSource == "mbcaa" || *row.CoverArtSource == "theaudiodb") && row.CoverArtPath != nil && *row.CoverArtPath != "" { if err := os.Remove(*row.CoverArtPath); err != nil && !os.IsNotExist(err) { - e.logger.Warn("coverart: remove old mbcaa file failed", - "album_id", uuidString(albumID), "path", *row.CoverArtPath, "err", err) + e.logger.Warn("coverart: remove old file failed", + "album_id", uuidString(albumID), + "path", *row.CoverArtPath, "err", err) } } if err := q.ClearAlbumCover(ctx, albumID); err != nil { @@ -203,16 +248,7 @@ func (e *Enricher) RetryAlbum(ctx context.Context, albumID pgtype.UUID) error { return e.EnrichAlbum(ctx, albumID) } -func (e *Enricher) recordCover(ctx context.Context, albumID pgtype.UUID, path, source string) error { - q := dbq.New(e.pool) - src := source - return q.SetAlbumCover(ctx, dbq.SetAlbumCoverParams{ - ID: albumID, - Column2: path, - CoverArtSource: &src, - }) -} - +// uuidString carries over from the previous enricher. func uuidString(u pgtype.UUID) string { if !u.Valid { return "" diff --git a/internal/coverart/enricher_test.go b/internal/coverart/enricher_test.go index 9f316b08..35faed3b 100644 --- a/internal/coverart/enricher_test.go +++ b/internal/coverart/enricher_test.go @@ -43,6 +43,21 @@ func discardLogger() *slog.Logger { return slog.New(slog.NewTextHandler(io.Discard, nil)) } +// newTestEnricher constructs an Enricher backed by a real DB pool for +// integration tests. Callers should register any fakes they need in +// the registry before calling this; SettingsService.reconcile() will +// pick them up. +func newTestEnricher(t *testing.T, pool *pgxpool.Pool) *Enricher { + t.Helper() + resetRegistryForTests() + t.Cleanup(resetRegistryForTests) + s, err := NewSettingsService(context.Background(), pool, discardLogger()) + if err != nil { + t.Fatalf("NewSettingsService: %v", err) + } + return NewEnricher(pool, discardLogger(), s) +} + // seedAlbumWithTrack creates an artist + album + track triple in a temp // dir. Returns album ID and the album-dir path. func seedAlbumWithTrack(t *testing.T, pool *pgxpool.Pool, title, artistName, mbid string) (pgtype.UUID, string) { @@ -85,7 +100,7 @@ func TestEnrichAlbum_SidecarFound(t *testing.T) { t.Fatal(err) } - e := NewEnricher(pool, discardLogger(), nil, false) + e := newTestEnricher(t, pool) if err := e.EnrichAlbum(context.Background(), id); err != nil { t.Fatalf("enrich: %v", err) } @@ -104,7 +119,7 @@ func TestEnrichAlbum_SidecarFound(t *testing.T) { func TestEnrichAlbum_NoSidecarNoMBID_LeavesNull(t *testing.T) { pool := newPool(t) id, _ := seedAlbumWithTrack(t, pool, "NoMbid", "Artist", "") - e := NewEnricher(pool, discardLogger(), nil, true) + e := newTestEnricher(t, pool) if err := e.EnrichAlbum(context.Background(), id); err != nil { t.Fatalf("enrich: %v", err) } @@ -117,9 +132,9 @@ func TestEnrichAlbum_NoSidecarNoMBID_LeavesNull(t *testing.T) { } } -func TestEnrichAlbum_MBCAAFetchSuccess(t *testing.T) { +func TestEnrichAlbum_ProviderFetchSuccess(t *testing.T) { pool := newPool(t) - id, dir := seedAlbumWithTrack(t, pool, "Mbcaa", "Artist", "test-mbid-123") + id, dir := seedAlbumWithTrack(t, pool, "Prov", "Artist", "test-mbid-123") upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.Header().Set("Content-Type", "image/jpeg") @@ -127,8 +142,23 @@ func TestEnrichAlbum_MBCAAFetchSuccess(t *testing.T) { })) defer upstream.Close() - f := NewFetcher(FetcherConfig{BaseURL: upstream.URL, UserAgent: "test", MinPeriod: 0, HTTPClient: upstream.Client()}) - e := NewEnricher(pool, discardLogger(), f, true) + // Register the MBCAA provider pointing at the test server, then + // build the Enricher so SettingsService picks it up. + resetRegistryForTests() + t.Cleanup(resetRegistryForTests) + p := newMBCAAProvider(FetcherConfig{ + BaseURL: upstream.URL, + UserAgent: "test", + MinPeriod: 0, + HTTPClient: upstream.Client(), + }) + Register(p) + s, err := NewSettingsService(context.Background(), pool, discardLogger()) + if err != nil { + t.Fatalf("NewSettingsService: %v", err) + } + e := NewEnricher(pool, discardLogger(), s) + if err := e.EnrichAlbum(context.Background(), id); err != nil { t.Fatalf("enrich: %v", err) } @@ -153,7 +183,7 @@ func TestEnrichAlbum_MBCAAFetchSuccess(t *testing.T) { } } -func TestEnrichAlbum_MBCAA404_RecordsNone(t *testing.T) { +func TestEnrichAlbum_All404_RecordsNone(t *testing.T) { pool := newPool(t) id, _ := seedAlbumWithTrack(t, pool, "Missing", "Artist", "missing-mbid") @@ -162,8 +192,21 @@ func TestEnrichAlbum_MBCAA404_RecordsNone(t *testing.T) { })) defer upstream.Close() - f := NewFetcher(FetcherConfig{BaseURL: upstream.URL, UserAgent: "test", MinPeriod: 0, HTTPClient: upstream.Client()}) - e := NewEnricher(pool, discardLogger(), f, true) + resetRegistryForTests() + t.Cleanup(resetRegistryForTests) + p := newMBCAAProvider(FetcherConfig{ + BaseURL: upstream.URL, + UserAgent: "test", + MinPeriod: 0, + HTTPClient: upstream.Client(), + }) + Register(p) + s, err := NewSettingsService(context.Background(), pool, discardLogger()) + if err != nil { + t.Fatalf("NewSettingsService: %v", err) + } + e := NewEnricher(pool, discardLogger(), s) + if err := e.EnrichAlbum(context.Background(), id); err != nil { t.Fatalf("enrich: %v", err) } @@ -184,7 +227,7 @@ func TestEnrichAlbum_AlreadySidecar_NoOp(t *testing.T) { t.Fatal(err) } - e := NewEnricher(pool, discardLogger(), nil, false) + e := newTestEnricher(t, pool) if err := e.EnrichAlbum(context.Background(), id); err != nil { t.Fatalf("first enrich: %v", err) } @@ -210,7 +253,8 @@ func TestEnrichBatch_DrainsNullSourceOnly(t *testing.T) { } id2, _ := seedAlbumWithTrack(t, pool, "Drain2", "B", "") - // Pre-mark id2 as 'none' — should NOT be drained by EnrichBatch. + // Pre-mark id2 as 'none' with current version — should NOT be drained by EnrichBatch + // (ListAlbumsMissingCover only returns stale-version 'none'). src := "none" if err := dbq.New(pool).SetAlbumCover(context.Background(), dbq.SetAlbumCoverParams{ ID: id2, Column2: "", CoverArtSource: &src, @@ -218,7 +262,7 @@ func TestEnrichBatch_DrainsNullSourceOnly(t *testing.T) { t.Fatal(err) } - e := NewEnricher(pool, discardLogger(), nil, false) + e := newTestEnricher(t, pool) processed, _, _, err := e.EnrichBatch(context.Background(), 100) if err != nil { t.Fatalf("batch: %v", err) @@ -237,7 +281,7 @@ func TestEnrichBatch_DrainsNullSourceOnly(t *testing.T) { } } -func TestRetryAlbum_DeletesMbcaaFileAndRefetches(t *testing.T) { +func TestRetryAlbum_DeletesWrittenFileAndRefetches(t *testing.T) { pool := newPool(t) id, dir := seedAlbumWithTrack(t, pool, "Retry", "Artist", "retry-mbid") @@ -247,8 +291,21 @@ func TestRetryAlbum_DeletesMbcaaFileAndRefetches(t *testing.T) { _, _ = w.Write([]byte("data")) })) defer upstream.Close() - f := NewFetcher(FetcherConfig{BaseURL: upstream.URL, UserAgent: "test", MinPeriod: 0, HTTPClient: upstream.Client()}) - e := NewEnricher(pool, discardLogger(), f, true) + + resetRegistryForTests() + t.Cleanup(resetRegistryForTests) + p := newMBCAAProvider(FetcherConfig{ + BaseURL: upstream.URL, + UserAgent: "test", + MinPeriod: 0, + HTTPClient: upstream.Client(), + }) + Register(p) + s, err := NewSettingsService(context.Background(), pool, discardLogger()) + if err != nil { + t.Fatalf("NewSettingsService: %v", err) + } + e := NewEnricher(pool, discardLogger(), s) if err := e.EnrichAlbum(context.Background(), id); err != nil { t.Fatalf("first enrich: %v", err) @@ -304,7 +361,7 @@ func TestEnrichRetryMissing_DrainsNullAndNone(t *testing.T) { t.Fatal(err) } - e := NewEnricher(pool, discardLogger(), nil, false) + e := newTestEnricher(t, pool) processed, err := e.EnrichRetryMissing(context.Background(), 100) if err != nil { t.Fatalf("retry missing: %v", err) @@ -328,3 +385,177 @@ func TestEnrichRetryMissing_DrainsNullAndNone(t *testing.T) { } } } + +// TestEnrichAlbum_MultiProviderChain verifies that when provider A +// returns ErrNotFound, the enricher continues to provider B. +func TestEnrichAlbum_MultiProviderChain(t *testing.T) { + pool := newPool(t) + id, dir := seedAlbumWithTrack(t, pool, "Multi", "Artist", "multi-mbid") + + // Provider A: always 404. + srvA := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + http.Error(w, "not found", http.StatusNotFound) + })) + defer srvA.Close() + + // Provider B: always succeeds. + srvB := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "image/jpeg") + _, _ = w.Write([]byte("PROVIDER_B_BYTES")) + })) + defer srvB.Close() + + resetRegistryForTests() + t.Cleanup(resetRegistryForTests) + pA := newMBCAAProvider(FetcherConfig{ + BaseURL: srvA.URL, + UserAgent: "test", + MinPeriod: 0, + HTTPClient: srvA.Client(), + }) + Register(pA) + + // fakeAlbumProvider from provider_test.go doesn't exist here yet; + // use a second mbcaa-shaped provider by registering a distinct one. + // We need a second AlbumCoverProvider with a different ID. + // Use a simple inline struct that wraps the second test server. + Register(&testAlbumProvider{ + id: "test-provider-b", + baseURL: srvB.URL, + client: srvB.Client(), + }) + + s, err := NewSettingsService(context.Background(), pool, discardLogger()) + if err != nil { + t.Fatalf("NewSettingsService: %v", err) + } + e := NewEnricher(pool, discardLogger(), s) + + if err := e.EnrichAlbum(context.Background(), id); err != nil { + t.Fatalf("enrich: %v", err) + } + + row, err := dbq.New(pool).GetAlbumWithFirstTrackPath(context.Background(), id) + if err != nil { + t.Fatalf("get: %v", err) + } + if row.CoverArtSource == nil || *row.CoverArtSource != "test-provider-b" { + t.Errorf("source = %v, want 'test-provider-b' (provider B should win)", row.CoverArtSource) + } + if body, err := os.ReadFile(filepath.Join(dir, "cover.jpg")); err != nil || string(body) != "PROVIDER_B_BYTES" { + t.Errorf("cover.jpg body = %q, err = %v", body, err) + } +} + +// TestEnrichAlbum_TransientLeavesNull verifies that a transient error +// from the provider leaves the row NULL for next-pass retry. +func TestEnrichAlbum_TransientLeavesNull(t *testing.T) { + pool := newPool(t) + id, _ := seedAlbumWithTrack(t, pool, "Transient", "Artist", "transient-mbid") + + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + http.Error(w, "boom", http.StatusInternalServerError) + })) + defer upstream.Close() + + resetRegistryForTests() + t.Cleanup(resetRegistryForTests) + p := newMBCAAProvider(FetcherConfig{ + BaseURL: upstream.URL, + UserAgent: "test", + MinPeriod: 0, + HTTPClient: upstream.Client(), + }) + Register(p) + s, err := NewSettingsService(context.Background(), pool, discardLogger()) + if err != nil { + t.Fatalf("NewSettingsService: %v", err) + } + e := NewEnricher(pool, discardLogger(), s) + + if err := e.EnrichAlbum(context.Background(), id); err != nil { + t.Fatalf("enrich: %v", err) + } + + row, err := dbq.New(pool).GetAlbumWithFirstTrackPath(context.Background(), id) + if err != nil { + t.Fatalf("get: %v", err) + } + // Transient failure: source must remain NULL (eligible for retry). + if row.CoverArtSource != nil { + t.Errorf("source = %v, want nil (NULL — transient should not settle)", row.CoverArtSource) + } +} + +// TestEnrichAlbum_StaleNoneFlipsAndRetries verifies that a row with +// cover_art_source='none' and a stale version gets cleared and +// re-enriched (sidecar wins because the album dir now has a cover file). +func TestEnrichAlbum_StaleNoneFlipsAndRetries(t *testing.T) { + pool := newPool(t) + id, dir := seedAlbumWithTrack(t, pool, "StaleNone", "Artist", "") + + // Mark the album as 'none' (simulating a previous pass). + src := "none" + if err := dbq.New(pool).SetAlbumCover(context.Background(), dbq.SetAlbumCoverParams{ + ID: id, Column2: "", CoverArtSource: &src, + }); err != nil { + t.Fatal(err) + } + // Now add a sidecar — EnrichAlbum should flip 'none' → NULL and pick it up. + if err := os.WriteFile(filepath.Join(dir, "cover.jpg"), []byte("new_art"), 0o644); err != nil { + t.Fatal(err) + } + + e := newTestEnricher(t, pool) + if err := e.EnrichAlbum(context.Background(), id); err != nil { + t.Fatalf("enrich: %v", err) + } + + row, err := dbq.New(pool).GetAlbumWithFirstTrackPath(context.Background(), id) + if err != nil { + t.Fatalf("get: %v", err) + } + if row.CoverArtSource == nil || *row.CoverArtSource != "sidecar" { + t.Errorf("source = %v, want 'sidecar' (stale none flipped and sidecar found)", row.CoverArtSource) + } +} + +// testAlbumProvider is a minimal AlbumCoverProvider for multi-chain tests. +type testAlbumProvider struct { + id string + baseURL string + client *http.Client +} + +func (p *testAlbumProvider) ID() string { return p.id } +func (p *testAlbumProvider) DisplayName() string { return p.id } +func (p *testAlbumProvider) RequiresAPIKey() bool { return false } +func (p *testAlbumProvider) DefaultEnabled() bool { return true } +func (p *testAlbumProvider) Configure(ProviderSettings) error { return nil } +func (p *testAlbumProvider) FetchAlbumCover(ctx context.Context, mbid string) ([]byte, error) { + req, err := http.NewRequestWithContext(ctx, http.MethodGet, p.baseURL+"/"+mbid, nil) + if err != nil { + return nil, err + } + resp, err := p.client.Do(req) + if err != nil { + return nil, err + } + defer func() { _ = resp.Body.Close() }() + if resp.StatusCode == http.StatusNotFound { + return nil, ErrNotFound + } + if resp.StatusCode != http.StatusOK { + return nil, ErrTransient + } + body := make([]byte, 0, 512) + buf := make([]byte, 512) + for { + n, rerr := resp.Body.Read(buf) + body = append(body, buf[:n]...) + if rerr != nil { + break + } + } + return body, nil +} diff --git a/internal/coverart/fetcher.go b/internal/coverart/fetcher.go deleted file mode 100644 index 30b029fe..00000000 --- a/internal/coverart/fetcher.go +++ /dev/null @@ -1,102 +0,0 @@ -package coverart - -import ( - "context" - "errors" - "fmt" - "io" - "net/http" - "sync" - "time" -) - -// ErrNotFound indicates the upstream responded with 404 — the album has no -// front cover on MBCAA. Caller should record cover_art_source = 'none'. -var ErrNotFound = errors.New("coverart: not found") - -// ErrTransient indicates a temporary failure (5xx, network, timeout). Caller -// should leave cover_art_source NULL so the next pass can retry. -var ErrTransient = errors.New("coverart: transient error") - -// FetcherConfig assembles the operator-tunable knobs for the MBCAA client. -type FetcherConfig struct { - BaseURL string // e.g. "https://coverartarchive.org" - UserAgent string // e.g. "Minstrel/ ()" - MinPeriod time.Duration // minimum delay between calls (1s default per MBCAA etiquette) - HTTPClient *http.Client // optional override for tests; default below -} - -// Fetcher pulls album front covers from MBCAA. Safe for concurrent use; the -// internal mutex serialises requests so the rate limit is respected. -type Fetcher struct { - cfg FetcherConfig - mu sync.Mutex - lastCall time.Time - client *http.Client -} - -// NewFetcher builds a Fetcher with sensible defaults. -func NewFetcher(cfg FetcherConfig) *Fetcher { - if cfg.BaseURL == "" { - cfg.BaseURL = "https://coverartarchive.org" - } - client := cfg.HTTPClient - if client == nil { - client = &http.Client{Timeout: 30 * time.Second} - } - return &Fetcher{cfg: cfg, client: client} -} - -// Fetch retrieves the 500px front cover for the given release MBID. Returns -// ErrNotFound on 404 (record as 'none') or ErrTransient otherwise (retry later). -func (f *Fetcher) Fetch(ctx context.Context, mbid string) ([]byte, error) { - if mbid == "" { - return nil, fmt.Errorf("coverart: empty mbid") - } - - // Rate limit: serialise + wait until MinPeriod has elapsed since the last call. - f.mu.Lock() - if f.cfg.MinPeriod > 0 { - wait := time.Until(f.lastCall.Add(f.cfg.MinPeriod)) - if wait > 0 { - timer := time.NewTimer(wait) - f.mu.Unlock() - select { - case <-ctx.Done(): - timer.Stop() - return nil, ctx.Err() - case <-timer.C: - } - f.mu.Lock() - } - } - f.lastCall = time.Now() - f.mu.Unlock() - - url := fmt.Sprintf("%s/release/%s/front-500", f.cfg.BaseURL, mbid) - req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) - if err != nil { - return nil, fmt.Errorf("coverart: build request: %w", err) - } - req.Header.Set("User-Agent", f.cfg.UserAgent) - req.Header.Set("Accept", "image/*") - - resp, err := f.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)) // 5MB cap - if err != nil { - return nil, fmt.Errorf("%w: read body: %v", ErrTransient, err) - } - return body, nil -} diff --git a/internal/coverart/fetcher_test.go b/internal/coverart/fetcher_test.go deleted file mode 100644 index d6a84946..00000000 --- a/internal/coverart/fetcher_test.go +++ /dev/null @@ -1,109 +0,0 @@ -package coverart - -import ( - "context" - "errors" - "net/http" - "net/http/httptest" - "strings" - "testing" - "time" -) - -func TestFetcher_Success(t *testing.T) { - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if !strings.Contains(r.URL.Path, "/release/abc-123/front-500") { - http.Error(w, "wrong path", http.StatusBadRequest) - return - } - w.Header().Set("Content-Type", "image/jpeg") - _, _ = w.Write([]byte("FAKE_JPEG")) - })) - defer srv.Close() - - f := NewFetcher(FetcherConfig{ - BaseURL: srv.URL, - UserAgent: "Minstrel/test (https://example.com)", - MinPeriod: 0, - HTTPClient: srv.Client(), - }) - - body, err := f.Fetch(context.Background(), "abc-123") - if err != nil { - t.Fatalf("Fetch: %v", err) - } - if string(body) != "FAKE_JPEG" { - t.Errorf("body = %q, want FAKE_JPEG", string(body)) - } -} - -func TestFetcher_NotFound(t *testing.T) { - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - http.Error(w, "not found", http.StatusNotFound) - })) - defer srv.Close() - f := NewFetcher(FetcherConfig{BaseURL: srv.URL, UserAgent: "test", MinPeriod: 0, HTTPClient: srv.Client()}) - _, err := f.Fetch(context.Background(), "missing") - if !errors.Is(err, ErrNotFound) { - t.Errorf("err = %v, want ErrNotFound", err) - } -} - -func TestFetcher_5xxIsTransient(t *testing.T) { - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - http.Error(w, "boom", http.StatusInternalServerError) - })) - defer srv.Close() - f := NewFetcher(FetcherConfig{BaseURL: srv.URL, UserAgent: "test", MinPeriod: 0, HTTPClient: srv.Client()}) - _, err := f.Fetch(context.Background(), "any") - if !errors.Is(err, ErrTransient) { - t.Errorf("err = %v, want ErrTransient", err) - } -} - -func TestFetcher_SendsUserAgent(t *testing.T) { - var seen string - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - seen = r.Header.Get("User-Agent") - _, _ = w.Write([]byte("ok")) - })) - defer srv.Close() - f := NewFetcher(FetcherConfig{ - BaseURL: srv.URL, - UserAgent: "Minstrel/0.1 (https://example.org)", - MinPeriod: 0, - HTTPClient: srv.Client(), - }) - _, _ = f.Fetch(context.Background(), "ua-test") - if seen != "Minstrel/0.1 (https://example.org)" { - t.Errorf("UA = %q", seen) - } -} - -func TestFetcher_RateLimitSpacesCalls(t *testing.T) { - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - _, _ = w.Write([]byte("ok")) - })) - defer srv.Close() - f := NewFetcher(FetcherConfig{ - BaseURL: srv.URL, - UserAgent: "test", - MinPeriod: 150 * time.Millisecond, - HTTPClient: srv.Client(), - }) - start := time.Now() - _, _ = f.Fetch(context.Background(), "a") - _, _ = f.Fetch(context.Background(), "b") - elapsed := time.Since(start) - if elapsed < 150*time.Millisecond { - t.Errorf("two calls in %v — rate limiter not enforcing", elapsed) - } -} - -func TestFetcher_EmptyMBID(t *testing.T) { - f := NewFetcher(FetcherConfig{UserAgent: "test", MinPeriod: 0}) - _, err := f.Fetch(context.Background(), "") - if err == nil { - t.Error("expected error for empty mbid") - } -} diff --git a/internal/coverart/provider.go b/internal/coverart/provider.go index 728f0e07..d81dfe8a 100644 --- a/internal/coverart/provider.go +++ b/internal/coverart/provider.go @@ -89,9 +89,16 @@ var ErrNotTestable = errors.New("coverart: provider does not support test connec // registered. var ErrProviderNotFound = errors.New("coverart: provider not registered") -// (Note: ErrNotFound and ErrTransient remain declared in fetcher.go -// during this task. They move to this file in the next commit when -// fetcher.go is deleted as part of the MBCAA refactor.) +// ErrNotFound indicates the upstream confirmed there is no art for +// this MBID — a terminal-this-pass result. The enricher treats this +// as "try the next provider in the chain"; if all providers return +// ErrNotFound, the row settles to cover_art_source='none'. +var ErrNotFound = errors.New("coverart: not found") + +// ErrTransient indicates a temporary failure (5xx, network, timeout, +// auth issue). The enricher leaves the row's source NULL so the next +// scan pass can retry. +var ErrTransient = errors.New("coverart: transient error") // registry is the package-private list of all compiled-in providers. // Providers register at init() via Register(). The enricher iterates diff --git a/internal/coverart/provider_mbcaa.go b/internal/coverart/provider_mbcaa.go index d5272c37..00ba6ee3 100644 --- a/internal/coverart/provider_mbcaa.go +++ b/internal/coverart/provider_mbcaa.go @@ -3,56 +3,79 @@ package coverart import ( "context" "errors" + "fmt" + "io" + "net/http" + "sync" "sync/atomic" + "time" ) -// mbcaaProvider implements Provider, AlbumCoverProvider, and -// TestableProvider. It wraps an *Fetcher (the existing -// MBCAA-specific HTTP client) for the actual fetch logic, adapting -// the Fetcher's Fetch method to the AlbumCoverProvider interface. -// -// The wrapping (rather than reimplementing) is temporary: A-T8 will -// consolidate fetcher.go's logic into this file and delete fetcher.go. -// During the T4–T8 window, this struct lets the new provider chain -// coexist with the legacy Fetcher used by enricher.go and main.go. -type mbcaaProvider struct { - fetcher *Fetcher - enabled atomic.Bool +// FetcherConfig assembles the operator-tunable knobs for the MBCAA +// HTTP client. Kept exported because main.go's boot wiring constructs +// one to pass into NewMBCAAProviderFromConfig. +type FetcherConfig struct { + BaseURL string // default "https://coverartarchive.org" + UserAgent string // e.g. "Minstrel/ ()" + MinPeriod time.Duration // 1s default per MBCAA etiquette + HTTPClient *http.Client // optional override for tests } -// Sample MBID used by TestConnection. The Beatles' "Abbey Road" UK -// release; verified present on MBCAA at design time. Both 200 (image -// returned) and 404 (no art for the sample) are treated as "connection -// works" since MBCAA needs no auth — only network failures surface. +// Sample MBID used by TestConnection. Beatles' "Abbey Road" UK release. const mbcaaTestSampleMBID = "f4b3df80-8a6c-3e87-b51e-5c1ee70a78ee" +// mbcaaProvider implements Provider, AlbumCoverProvider, and +// TestableProvider against the MusicBrainz Cover Art Archive. +// Safe for concurrent use; the internal mutex serialises HTTP +// requests so the rate limit is respected. +type mbcaaProvider struct { + cfg FetcherConfig + enabled atomic.Bool + mu sync.Mutex + lastCall time.Time + client *http.Client +} + func init() { - // Register with a default-config Fetcher. main.go (in A-T8) will - // call NewMBCAAProviderFromConfig to swap in the production - // User-Agent + MinPeriod. Default config values come from - // FetcherConfig's zero-value handling in NewFetcher. - Register(&mbcaaProvider{ - fetcher: NewFetcher(FetcherConfig{}), - }) + Register(newMBCAAProvider(FetcherConfig{})) } // NewMBCAAProviderFromConfig replaces the registered MBCAA provider's -// internal Fetcher with one built from the supplied config. main.go -// calls this at boot to set the production User-Agent. Idempotent; -// safe to call repeatedly. +// internal config with the supplied one. main.go calls this at boot +// to set the production User-Agent. func NewMBCAAProviderFromConfig(cfg FetcherConfig) { p, err := ProviderByID("mbcaa") if err != nil { - // init() should have registered it; if not, register fresh. - Register(&mbcaaProvider{fetcher: NewFetcher(cfg)}) + Register(newMBCAAProvider(cfg)) return } mp, ok := p.(*mbcaaProvider) if !ok { - // Some other provider claimed the "mbcaa" ID — registry bug. panic("coverart: provider with ID 'mbcaa' is not *mbcaaProvider") } - mp.fetcher = NewFetcher(cfg) + cfg = applyMBCAADefaults(cfg) + mp.mu.Lock() + mp.cfg = cfg + mp.client = cfg.HTTPClient + mp.mu.Unlock() +} + +func newMBCAAProvider(cfg FetcherConfig) *mbcaaProvider { + cfg = applyMBCAADefaults(cfg) + return &mbcaaProvider{cfg: cfg, client: cfg.HTTPClient} +} + +func applyMBCAADefaults(cfg FetcherConfig) FetcherConfig { + if cfg.BaseURL == "" { + cfg.BaseURL = "https://coverartarchive.org" + } + if cfg.MinPeriod == 0 { + cfg.MinPeriod = time.Second + } + if cfg.HTTPClient == nil { + cfg.HTTPClient = &http.Client{Timeout: 30 * time.Second} + } + return cfg } func (p *mbcaaProvider) ID() string { return "mbcaa" } @@ -62,25 +85,69 @@ func (p *mbcaaProvider) DefaultEnabled() bool { return true } func (p *mbcaaProvider) Configure(s ProviderSettings) error { p.enabled.Store(s.Enabled) - return nil // MBCAA has no per-operator config beyond enabled + return nil } -// FetchAlbumCover delegates to the wrapped Fetcher. The Fetcher's -// existing Fetch method already returns ErrNotFound / ErrTransient -// per the contract, so no error mapping is needed. +// FetchAlbumCover retrieves the 500px front cover for the given +// release MBID. Returns ErrNotFound on 404 or ErrTransient otherwise. func (p *mbcaaProvider) FetchAlbumCover(ctx context.Context, mbid string) ([]byte, error) { if !p.enabled.Load() { return nil, ErrNotFound // defensive; enricher already filters on enabled } - return p.fetcher.Fetch(ctx, mbid) + if mbid == "" { + return nil, fmt.Errorf("coverart: empty mbid") + } + + // Rate limit guard. + p.mu.Lock() + cfg := p.cfg + if cfg.MinPeriod > 0 { + wait := time.Until(p.lastCall.Add(cfg.MinPeriod)) + if wait > 0 { + timer := time.NewTimer(wait) + p.mu.Unlock() + select { + case <-ctx.Done(): + timer.Stop() + return nil, ctx.Err() + case <-timer.C: + } + p.mu.Lock() + } + } + p.lastCall = time.Now() + p.mu.Unlock() + + url := fmt.Sprintf("%s/release/%s/front-500", cfg.BaseURL, mbid) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) + if err != nil { + return nil, fmt.Errorf("coverart: build request: %w", err) + } + req.Header.Set("User-Agent", cfg.UserAgent) + 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 } -// TestConnection hits the sample MBID's front-500 endpoint. A 200 -// (image bytes) or 404 (no art for sample) both indicate the -// connection works; 5xx / network errors surface as TestConnection -// failures. func (p *mbcaaProvider) TestConnection(ctx context.Context) error { - _, err := p.fetcher.Fetch(ctx, mbcaaTestSampleMBID) + _, err := p.FetchAlbumCover(ctx, mbcaaTestSampleMBID) if err == nil { return nil } diff --git a/internal/coverart/provider_mbcaa_test.go b/internal/coverart/provider_mbcaa_test.go index e9a2f9d4..f224156f 100644 --- a/internal/coverart/provider_mbcaa_test.go +++ b/internal/coverart/provider_mbcaa_test.go @@ -11,21 +11,18 @@ import ( ) // newMBCAAProviderForTest registers a fresh mbcaaProvider pointing at -// a test server, mirroring the pattern from fetcher_test.go but -// exercising the interface-side methods. +// a test server. func newMBCAAProviderForTest(t *testing.T, mockURL string) *mbcaaProvider { t.Helper() resetRegistryForTests() t.Cleanup(resetRegistryForTests) - p := &mbcaaProvider{ - fetcher: NewFetcher(FetcherConfig{ - BaseURL: mockURL, - UserAgent: "Minstrel/test (https://example.com)", - MinPeriod: 0, - HTTPClient: &http.Client{Timeout: 5 * time.Second}, - }), - } + p := newMBCAAProvider(FetcherConfig{ + BaseURL: mockURL, + UserAgent: "Minstrel/test (https://example.com)", + MinPeriod: 0, + HTTPClient: &http.Client{Timeout: 5 * time.Second}, + }) Register(p) p.enabled.Store(true) return p @@ -34,7 +31,7 @@ func newMBCAAProviderForTest(t *testing.T, mockURL string) *mbcaaProvider { func TestMBCAAProvider_ID(t *testing.T) { resetRegistryForTests() defer resetRegistryForTests() - p := &mbcaaProvider{fetcher: NewFetcher(FetcherConfig{})} + p := newMBCAAProvider(FetcherConfig{}) if p.ID() != "mbcaa" { t.Errorf("ID = %q, want mbcaa", p.ID()) } @@ -102,7 +99,7 @@ func TestMBCAAProvider_FetchAlbumCover_DisabledReturnsNotFound(t *testing.T) { func TestMBCAAProvider_Configure_TogglesEnabled(t *testing.T) { resetRegistryForTests() defer resetRegistryForTests() - p := &mbcaaProvider{fetcher: NewFetcher(FetcherConfig{})} + p := newMBCAAProvider(FetcherConfig{}) if err := p.Configure(ProviderSettings{Enabled: true}); err != nil { t.Fatalf("Configure: %v", err) @@ -143,12 +140,12 @@ func TestMBCAAProvider_TestConnection_PropagatesTransient(t *testing.T) { } } -func TestNewMBCAAProviderFromConfig_ReplacesRegisteredFetcher(t *testing.T) { +func TestNewMBCAAProviderFromConfig_ReplacesRegisteredConfig(t *testing.T) { resetRegistryForTests() defer resetRegistryForTests() // Pre-register with default config (mimics init() behaviour). - Register(&mbcaaProvider{fetcher: NewFetcher(FetcherConfig{})}) + Register(newMBCAAProvider(FetcherConfig{})) cfg := FetcherConfig{ UserAgent: "Minstrel/test-replaced (x)", @@ -161,11 +158,11 @@ func TestNewMBCAAProviderFromConfig_ReplacesRegisteredFetcher(t *testing.T) { t.Fatalf("ProviderByID: %v", err) } mp := p.(*mbcaaProvider) - if mp.fetcher == nil { - t.Fatal("fetcher is nil after NewMBCAAProviderFromConfig") + // Verify the cfg was replaced on the registered instance. + mp.mu.Lock() + gotUA := mp.cfg.UserAgent + mp.mu.Unlock() + if gotUA != "Minstrel/test-replaced (x)" { + t.Errorf("cfg.UserAgent = %q, want %q", gotUA, "Minstrel/test-replaced (x)") } - // We can't peek the fetcher's UserAgent directly (it's an unexported - // field on FetcherConfig held inside Fetcher), but the side effect of - // ProviderByID returning the same registered instance with a swapped - // fetcher is what we're verifying here. }