diff --git a/internal/db/dbq/candidate_artist_tags.sql.go b/internal/db/dbq/candidate_artist_tags.sql.go new file mode 100644 index 00000000..29ec71a9 --- /dev/null +++ b/internal/db/dbq/candidate_artist_tags.sql.go @@ -0,0 +1,233 @@ +// Code generated by sqlc. DO NOT EDIT. +// versions: +// sqlc v1.31.1 +// source: candidate_artist_tags.sql + +package dbq + +import ( + "context" +) + +const countCandidateArtistTagCoverage = `-- name: CountCandidateArtistTagCoverage :one +SELECT count(*)::bigint AS processed, + count(*) FILTER (WHERE tag_source <> 'none')::bigint AS with_tags + FROM candidate_artist_tag_state +` + +type CountCandidateArtistTagCoverageRow struct { + Processed int64 + WithTags int64 +} + +// Operator-facing coverage: how many distinct candidates have been processed, +// and how many of those actually yielded tags. The gap is the honest ceiling +// from the task — obscure artists with no MBID presence or no upstream tags +// stay thin no matter how long the worker runs, and that is worth being able +// to see rather than inferring from a silent surface. +func (q *Queries) CountCandidateArtistTagCoverage(ctx context.Context) (CountCandidateArtistTagCoverageRow, error) { + row := q.db.QueryRow(ctx, countCandidateArtistTagCoverage) + var i CountCandidateArtistTagCoverageRow + err := row.Scan(&i.Processed, &i.WithTags) + return i, err +} + +const deleteCandidateArtistTags = `-- name: DeleteCandidateArtistTags :exec +DELETE FROM candidate_artist_tags WHERE candidate_mbid = $1 +` + +// Clear a candidate's cached tags before rewriting (atomic replace by the +// caller, same shape as DeleteTrackTags). +func (q *Queries) DeleteCandidateArtistTags(ctx context.Context, candidateMbid string) error { + _, err := q.db.Exec(ctx, deleteCandidateArtistTags, candidateMbid) + return err +} + +const gcDeleteOrphanedCandidateArtistTagState = `-- name: GcDeleteOrphanedCandidateArtistTagState :execrows +DELETE FROM candidate_artist_tag_state s + WHERE NOT EXISTS ( + SELECT 1 FROM artist_similarity_unmatched u + WHERE u.candidate_mbid = s.candidate_mbid + ) + OR EXISTS (SELECT 1 FROM artists a WHERE a.mbid = s.candidate_mbid) +` + +// Same sweep for the bookkeeping rows. Kept as a separate statement rather +// than a cascade: the two tables are independent by design (a 'none' outcome +// has state but no tags), so neither can be the parent of the other. +func (q *Queries) GcDeleteOrphanedCandidateArtistTagState(ctx context.Context) (int64, error) { + result, err := q.db.Exec(ctx, gcDeleteOrphanedCandidateArtistTagState) + if err != nil { + return 0, err + } + return result.RowsAffected(), nil +} + +const gcDeleteOrphanedCandidateArtistTags = `-- name: GcDeleteOrphanedCandidateArtistTags :execrows +DELETE FROM candidate_artist_tags t + WHERE NOT EXISTS ( + SELECT 1 FROM artist_similarity_unmatched u + WHERE u.candidate_mbid = t.candidate_mbid + ) + OR EXISTS (SELECT 1 FROM artists a WHERE a.mbid = t.candidate_mbid) +` + +// Drops cached tags for candidates that no longer appear in the similarity +// feed, or that have since been added to the library (their tags now live in +// track_tags). The feed is refetched periodically and churns, so without this +// the cache only ever grows. +func (q *Queries) GcDeleteOrphanedCandidateArtistTags(ctx context.Context) (int64, error) { + result, err := q.db.Exec(ctx, gcDeleteOrphanedCandidateArtistTags) + if err != nil { + return 0, err + } + return result.RowsAffected(), nil +} + +const insertCandidateArtistTag = `-- name: InsertCandidateArtistTag :exec +INSERT INTO candidate_artist_tags (candidate_mbid, tag, weight) +VALUES ($1, $2, $3) +ON CONFLICT (candidate_mbid, tag) + DO UPDATE SET weight = GREATEST(candidate_artist_tags.weight, EXCLUDED.weight) +` + +type InsertCandidateArtistTagParams struct { + CandidateMbid string + Tag string + Weight float64 +} + +// Upsert one (candidate, tag); keep the stronger weight when two providers +// agree on a tag with different folksonomy strengths. +func (q *Queries) InsertCandidateArtistTag(ctx context.Context, arg InsertCandidateArtistTagParams) error { + _, err := q.db.Exec(ctx, insertCandidateArtistTag, arg.CandidateMbid, arg.Tag, arg.Weight) + return err +} + +const listCandidateArtistTagsForMbids = `-- name: ListCandidateArtistTagsForMbids :many +SELECT candidate_mbid, tag, weight + FROM candidate_artist_tags + WHERE candidate_mbid = ANY($1::text[]) +` + +type ListCandidateArtistTagsForMbidsRow struct { + CandidateMbid string + Tag string + Weight float64 +} + +// Cached tags for a set of candidates, for slice 6's taste-overlap ranking. +// One row per (candidate, tag). +func (q *Queries) ListCandidateArtistTagsForMbids(ctx context.Context, dollar_1 []string) ([]ListCandidateArtistTagsForMbidsRow, error) { + rows, err := q.db.Query(ctx, listCandidateArtistTagsForMbids, dollar_1) + if err != nil { + return nil, err + } + defer rows.Close() + var items []ListCandidateArtistTagsForMbidsRow + for rows.Next() { + var i ListCandidateArtistTagsForMbidsRow + if err := rows.Scan(&i.CandidateMbid, &i.Tag, &i.Weight); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + +const listCandidateArtistsMissingTags = `-- name: ListCandidateArtistsMissingTags :many + +SELECT u.candidate_mbid, + coalesce(max(u.candidate_name), '')::text AS candidate_name, + sum(u.score)::float8 AS total_score + FROM artist_similarity_unmatched u + LEFT JOIN candidate_artist_tag_state s ON s.candidate_mbid = u.candidate_mbid + WHERE NOT EXISTS (SELECT 1 FROM artists a WHERE a.mbid = u.candidate_mbid) + AND ( + s.candidate_mbid IS NULL + OR (s.tag_source = 'none' AND s.tag_sources_version < $1) + ) + GROUP BY u.candidate_mbid + ORDER BY total_score DESC, u.candidate_mbid + LIMIT $2 +` + +type ListCandidateArtistsMissingTagsParams struct { + TagSourcesVersion int32 + Limit int32 +} + +type ListCandidateArtistsMissingTagsRow struct { + CandidateMbid string + CandidateName string + TotalScore float64 +} + +// Folksonomy tags for out-of-library Discover candidates (#2376). Parallel to +// track_tags.sql, but keyed by MBID because the artist has no local row. See +// 0050_candidate_artist_tags.up.sql for why the bookkeeping is its own table. +// Candidates eligible for tag enrichment: never processed (no state row) or +// settled 'none' under an older provider version. +// +// artist_similarity_unmatched holds one row per (seed, candidate, source), so +// this GROUPs to one row per candidate — enriching the same MBID once per seed +// that pointed at it would multiply the API calls for no gain. +// +// ORDER BY summed similarity DESC is the load-bearing part. The candidate pool +// is O(library artists x neighbours per artist) — thousands — and MusicBrainz +// allows ~1 req/s, so it can NEVER be fully enriched in one pass. Draining in +// strength order means the candidates most likely to actually reach a user's +// deck get tags first, and the long tail fills in over subsequent ticks +// instead of starving behind it. +// +// Already-in-library candidates are skipped: they have an artists row, so +// their tags belong in track_tags, and the suggestion query filters them out +// anyway. $1 = current tag_sources_version, $2 = limit. +// candidate_name is coalesced to '' so it lands non-nullable in Go: the name is +// only a Last.fm lookup key, and empty simply means "MBID-keyed providers only", +// which the provider chain already handles. max() is an arbitrary-but- +// deterministic pick when several seeds spell the same MBID differently. +func (q *Queries) ListCandidateArtistsMissingTags(ctx context.Context, arg ListCandidateArtistsMissingTagsParams) ([]ListCandidateArtistsMissingTagsRow, error) { + rows, err := q.db.Query(ctx, listCandidateArtistsMissingTags, arg.TagSourcesVersion, arg.Limit) + if err != nil { + return nil, err + } + defer rows.Close() + var items []ListCandidateArtistsMissingTagsRow + for rows.Next() { + var i ListCandidateArtistsMissingTagsRow + if err := rows.Scan(&i.CandidateMbid, &i.CandidateName, &i.TotalScore); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + +const setCandidateArtistTagState = `-- name: SetCandidateArtistTagState :exec +INSERT INTO candidate_artist_tag_state (candidate_mbid, tag_source, tag_sources_version) +VALUES ($1, $2, $3) +ON CONFLICT (candidate_mbid) DO UPDATE + SET tag_source = EXCLUDED.tag_source, + tag_sources_version = EXCLUDED.tag_sources_version, + updated_at = now() +` + +type SetCandidateArtistTagStateParams struct { + CandidateMbid string + TagSource string + TagSourcesVersion int32 +} + +// Stamp the enrichment outcome so the drainer skips settled candidates. +// $2 = 'musicbrainz' | 'lastfm' | 'mixed' | 'none', $3 = current version. +func (q *Queries) SetCandidateArtistTagState(ctx context.Context, arg SetCandidateArtistTagStateParams) error { + _, err := q.db.Exec(ctx, setCandidateArtistTagState, arg.CandidateMbid, arg.TagSource, arg.TagSourcesVersion) + return err +} diff --git a/internal/db/dbq/models.go b/internal/db/dbq/models.go index cc61f970..d4ec39f7 100644 --- a/internal/db/dbq/models.go +++ b/internal/db/dbq/models.go @@ -240,6 +240,19 @@ type AuditLog struct { CreatedAt pgtype.Timestamptz } +type CandidateArtistTagState struct { + CandidateMbid string + TagSource string + TagSourcesVersion int32 + UpdatedAt pgtype.Timestamptz +} + +type CandidateArtistTag struct { + CandidateMbid string + Tag string + Weight float64 +} + type ContextualLike struct { ID pgtype.UUID UserID pgtype.UUID diff --git a/internal/db/migrations/0050_candidate_artist_tags.down.sql b/internal/db/migrations/0050_candidate_artist_tags.down.sql new file mode 100644 index 00000000..cd7cd862 --- /dev/null +++ b/internal/db/migrations/0050_candidate_artist_tags.down.sql @@ -0,0 +1,3 @@ +DROP INDEX IF EXISTS candidate_artist_tag_state_source_idx; +DROP TABLE IF EXISTS candidate_artist_tag_state; +DROP TABLE IF EXISTS candidate_artist_tags; diff --git a/internal/db/migrations/0050_candidate_artist_tags.up.sql b/internal/db/migrations/0050_candidate_artist_tags.up.sql new file mode 100644 index 00000000..660a3da7 --- /dev/null +++ b/internal/db/migrations/0050_candidate_artist_tags.up.sql @@ -0,0 +1,54 @@ +-- 0050_candidate_artist_tags.up.sql — folksonomy tags for OUT-OF-LIBRARY +-- artists (#2376, milestone #268 slice 5). +-- +-- track_tags (0042) cannot hold these: it is FK'd to tracks(id), and a +-- Discover candidate has no local row by definition. So this is a parallel +-- cache keyed by the candidate's MusicBrainz MBID — the only stable identity +-- available for an artist we don't have. +-- +-- Purpose is slice 6: rank suggestions by overlap between these tags and the +-- user's taste_profile_tags, turning "neighbour of an artist you play" into +-- "matches the sound you like". +-- +-- GLOBAL, not per-user: a candidate's tags are a property of the artist, not +-- of anyone's taste. Nothing here is user-scoped, so rule #47 has nothing to +-- scope — the per-user part lives entirely in slice 6's ranking. +-- +-- weight is a normalized folksonomy strength in [0,1], same scale as +-- track_tags, so the two can be compared without a conversion step. +CREATE TABLE candidate_artist_tags ( + candidate_mbid text NOT NULL, + tag text NOT NULL, + weight double precision NOT NULL DEFAULT 1, + PRIMARY KEY (candidate_mbid, tag) +); + +-- Enrichment bookkeeping. This is a SEPARATE table rather than columns on the +-- tags table, because the "providers had nothing" outcome must be recordable +-- for a candidate with zero tag rows — otherwise every empty candidate stays +-- eligible forever and the worker re-fetches it on every tick. +-- +-- tracks solved the same problem with columns on `tracks` (0042), but there is +-- no per-candidate row anywhere to hang them off: artist_similarity_unmatched +-- is keyed (seed_artist_id, candidate_mbid, source) and holds MANY rows per +-- candidate. +-- +-- Absence of a row here means "never processed", so unlike tracks.tag_source +-- this column can be NOT NULL — there is no null-means-pending state to model. +-- 'musicbrainz' | 'lastfm' | 'mixed' → found, cached +-- 'none' → providers confirmed nothing +-- tag_sources_version → bump to re-process settled 'none' +-- +-- A transient failure writes NO row at all (rather than a row it would then +-- have to distinguish), which leaves the candidate eligible for the next pass. +CREATE TABLE candidate_artist_tag_state ( + candidate_mbid text PRIMARY KEY, + tag_source text NOT NULL, + tag_sources_version integer NOT NULL DEFAULT 0, + updated_at timestamptz NOT NULL DEFAULT now() +); + +-- Serves the eligibility scan's "settled 'none' under an older version" +-- branch. The PK already covers the per-candidate lookups. +CREATE INDEX candidate_artist_tag_state_source_idx + ON candidate_artist_tag_state (tag_source, tag_sources_version); diff --git a/internal/db/queries/candidate_artist_tags.sql b/internal/db/queries/candidate_artist_tags.sql new file mode 100644 index 00000000..150b0637 --- /dev/null +++ b/internal/db/queries/candidate_artist_tags.sql @@ -0,0 +1,102 @@ +-- Folksonomy tags for out-of-library Discover candidates (#2376). Parallel to +-- track_tags.sql, but keyed by MBID because the artist has no local row. See +-- 0050_candidate_artist_tags.up.sql for why the bookkeeping is its own table. + +-- name: ListCandidateArtistsMissingTags :many +-- Candidates eligible for tag enrichment: never processed (no state row) or +-- settled 'none' under an older provider version. +-- +-- artist_similarity_unmatched holds one row per (seed, candidate, source), so +-- this GROUPs to one row per candidate — enriching the same MBID once per seed +-- that pointed at it would multiply the API calls for no gain. +-- +-- ORDER BY summed similarity DESC is the load-bearing part. The candidate pool +-- is O(library artists x neighbours per artist) — thousands — and MusicBrainz +-- allows ~1 req/s, so it can NEVER be fully enriched in one pass. Draining in +-- strength order means the candidates most likely to actually reach a user's +-- deck get tags first, and the long tail fills in over subsequent ticks +-- instead of starving behind it. +-- +-- Already-in-library candidates are skipped: they have an artists row, so +-- their tags belong in track_tags, and the suggestion query filters them out +-- anyway. $1 = current tag_sources_version, $2 = limit. +-- candidate_name is coalesced to '' so it lands non-nullable in Go: the name is +-- only a Last.fm lookup key, and empty simply means "MBID-keyed providers only", +-- which the provider chain already handles. max() is an arbitrary-but- +-- deterministic pick when several seeds spell the same MBID differently. +SELECT u.candidate_mbid, + coalesce(max(u.candidate_name), '')::text AS candidate_name, + sum(u.score)::float8 AS total_score + FROM artist_similarity_unmatched u + LEFT JOIN candidate_artist_tag_state s ON s.candidate_mbid = u.candidate_mbid + WHERE NOT EXISTS (SELECT 1 FROM artists a WHERE a.mbid = u.candidate_mbid) + AND ( + s.candidate_mbid IS NULL + OR (s.tag_source = 'none' AND s.tag_sources_version < $1) + ) + GROUP BY u.candidate_mbid + ORDER BY total_score DESC, u.candidate_mbid + LIMIT $2; + +-- name: DeleteCandidateArtistTags :exec +-- Clear a candidate's cached tags before rewriting (atomic replace by the +-- caller, same shape as DeleteTrackTags). +DELETE FROM candidate_artist_tags WHERE candidate_mbid = $1; + +-- name: InsertCandidateArtistTag :exec +-- Upsert one (candidate, tag); keep the stronger weight when two providers +-- agree on a tag with different folksonomy strengths. +INSERT INTO candidate_artist_tags (candidate_mbid, tag, weight) +VALUES ($1, $2, $3) +ON CONFLICT (candidate_mbid, tag) + DO UPDATE SET weight = GREATEST(candidate_artist_tags.weight, EXCLUDED.weight); + +-- name: SetCandidateArtistTagState :exec +-- Stamp the enrichment outcome so the drainer skips settled candidates. +-- $2 = 'musicbrainz' | 'lastfm' | 'mixed' | 'none', $3 = current version. +INSERT INTO candidate_artist_tag_state (candidate_mbid, tag_source, tag_sources_version) +VALUES ($1, $2, $3) +ON CONFLICT (candidate_mbid) DO UPDATE + SET tag_source = EXCLUDED.tag_source, + tag_sources_version = EXCLUDED.tag_sources_version, + updated_at = now(); + +-- name: ListCandidateArtistTagsForMbids :many +-- Cached tags for a set of candidates, for slice 6's taste-overlap ranking. +-- One row per (candidate, tag). +SELECT candidate_mbid, tag, weight + FROM candidate_artist_tags + WHERE candidate_mbid = ANY($1::text[]); + +-- name: CountCandidateArtistTagCoverage :one +-- Operator-facing coverage: how many distinct candidates have been processed, +-- and how many of those actually yielded tags. The gap is the honest ceiling +-- from the task — obscure artists with no MBID presence or no upstream tags +-- stay thin no matter how long the worker runs, and that is worth being able +-- to see rather than inferring from a silent surface. +SELECT count(*)::bigint AS processed, + count(*) FILTER (WHERE tag_source <> 'none')::bigint AS with_tags + FROM candidate_artist_tag_state; + +-- name: GcDeleteOrphanedCandidateArtistTags :execrows +-- Drops cached tags for candidates that no longer appear in the similarity +-- feed, or that have since been added to the library (their tags now live in +-- track_tags). The feed is refetched periodically and churns, so without this +-- the cache only ever grows. +DELETE FROM candidate_artist_tags t + WHERE NOT EXISTS ( + SELECT 1 FROM artist_similarity_unmatched u + WHERE u.candidate_mbid = t.candidate_mbid + ) + OR EXISTS (SELECT 1 FROM artists a WHERE a.mbid = t.candidate_mbid); + +-- name: GcDeleteOrphanedCandidateArtistTagState :execrows +-- Same sweep for the bookkeeping rows. Kept as a separate statement rather +-- than a cascade: the two tables are independent by design (a 'none' outcome +-- has state but no tags), so neither can be the parent of the other. +DELETE FROM candidate_artist_tag_state s + WHERE NOT EXISTS ( + SELECT 1 FROM artist_similarity_unmatched u + WHERE u.candidate_mbid = s.candidate_mbid + ) + OR EXISTS (SELECT 1 FROM artists a WHERE a.mbid = s.candidate_mbid); diff --git a/internal/dbtest/reset.go b/internal/dbtest/reset.go index 180690ce..b39669cc 100644 --- a/internal/dbtest/reset.go +++ b/internal/dbtest/reset.go @@ -58,6 +58,12 @@ var dataTables = []string{ // explicitly or a stale snooze silently hides a candidate from the // next test's suggestion assertions. "suggestion_snoozes", + // #2376. Same reasoning: keyed by candidate MBID with no FK anywhere, + // so nothing cascades to them. A leftover tag row would make a + // candidate look enriched to the next test, and a leftover state row + // would make it look already-settled and thus ineligible. + "candidate_artist_tags", + "candidate_artist_tag_state", "playlist_tracks", "playlists", "library_changes", // M7 #357 — must reset to keep cursor isolated per test diff --git a/internal/gc/worker.go b/internal/gc/worker.go index b38dc8f6..39ca9a3c 100644 --- a/internal/gc/worker.go +++ b/internal/gc/worker.go @@ -18,6 +18,8 @@ // - GcDeleteExpiredPasswordResets (#575) // - GcPruneDiagnostics (M9 — diagnostics 30d retention) // - GcDeleteExpiredSuggestionSnoozes (#2374 — snoozes expire, then go) +// - GcDeleteOrphanedCandidateArtistTags(+State) (#2376 — the similarity +// feed churns, so cached candidate tags outlive their candidates) package gc import ( @@ -86,6 +88,13 @@ func (w *Worker) tickOnce(ctx context.Context) { w.runSweep(ctx, "delete_expired_password_resets", q.GcDeleteExpiredPasswordResets) w.runSweep(ctx, "prune_diagnostics", q.GcPruneDiagnostics) w.runSweep(ctx, "delete_expired_suggestion_snoozes", q.GcDeleteExpiredSuggestionSnoozes) + // Tags before state: if the process dies between the two, a candidate left + // with a state row and no tags simply reads as "settled, nothing found", + // which is already a valid state. The reverse order could leave tags with + // no state row, which the drainer would treat as never-processed and + // re-fetch on top of rows that are already there. + w.runSweep(ctx, "orphaned_candidate_artist_tags", q.GcDeleteOrphanedCandidateArtistTags) + w.runSweep(ctx, "orphaned_candidate_artist_tag_state", q.GcDeleteOrphanedCandidateArtistTagState) } // runSweep is a small adapter so each sweep call site is a one-liner diff --git a/internal/recommendation/candidate_tags_integration_test.go b/internal/recommendation/candidate_tags_integration_test.go new file mode 100644 index 00000000..eda08a71 --- /dev/null +++ b/internal/recommendation/candidate_tags_integration_test.go @@ -0,0 +1,282 @@ +package recommendation + +import ( + "context" + "testing" + + "github.com/jackc/pgx/v5/pgxpool" + + "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" +) + +// Slice 5 (#2376): the candidate-artist tag cache for out-of-library Discover +// candidates. These cover the SQL rather than the provider chain — the +// eligibility query is the piece with real risk in it (a GROUP BY over a +// many-rows-per-candidate table, a LEFT JOIN to bookkeeping, and two exclusion +// branches), and it's consumed by this package's slice-6 ranking. +// +// Fixtures live here because the harness and seedUnmatched do. + +const ( + tagVersionCurrent = 2 + tagVersionOld = 1 +) + +// listEligible is the query under test, at the current provider version. +func listEligible(t *testing.T, pool *pgxpool.Pool, limit int32) []dbq.ListCandidateArtistsMissingTagsRow { + t.Helper() + rows, err := dbq.New(pool).ListCandidateArtistsMissingTags(context.Background(), + dbq.ListCandidateArtistsMissingTagsParams{ + TagSourcesVersion: tagVersionCurrent, + Limit: limit, + }) + if err != nil { + t.Fatalf("ListCandidateArtistsMissingTags: %v", err) + } + return rows +} + +func setState(t *testing.T, pool *pgxpool.Pool, mbid, source string, version int32) { + t.Helper() + if err := dbq.New(pool).SetCandidateArtistTagState(context.Background(), + dbq.SetCandidateArtistTagStateParams{ + CandidateMbid: mbid, TagSource: source, TagSourcesVersion: version, + }); err != nil { + t.Fatalf("SetCandidateArtistTagState: %v", err) + } +} + +func mbidsOf(rows []dbq.ListCandidateArtistsMissingTagsRow) []string { + out := make([]string, 0, len(rows)) + for _, r := range rows { + out = append(out, r.CandidateMbid) + } + return out +} + +func TestCandidateTags_NeverProcessedIsEligible(t *testing.T) { + pool := newPool(t) + user := seedUser(t, pool, "alice") + seed := seedArtist(t, pool, "Seed", "") + likeArtist(t, pool, user.ID, seed.ID) + seedUnmatched(t, pool, seed.ID, "cand-1", "Candidate One", 0.9) + + rows := listEligible(t, pool, 10) + if len(rows) != 1 { + t.Fatalf("len = %d, want 1: %v", len(rows), mbidsOf(rows)) + } + if rows[0].CandidateName != "Candidate One" { + t.Errorf("name = %q, want Candidate One", rows[0].CandidateName) + } + if rows[0].TotalScore != 0.9 { + t.Errorf("score = %v, want 0.9", rows[0].TotalScore) + } +} + +// An in-library candidate's tags belong in track_tags, and the suggestion query +// filters it out anyway — enriching it would be wasted API budget. +func TestCandidateTags_InLibraryCandidateIsExcluded(t *testing.T) { + pool := newPool(t) + seed := seedArtist(t, pool, "Seed", "") + seedArtist(t, pool, "Already Here", "cand-in-lib") + seedUnmatched(t, pool, seed.ID, "cand-in-lib", "Already Here", 0.9) + + if rows := listEligible(t, pool, 10); len(rows) != 0 { + t.Errorf("len = %d, want 0: %v", len(rows), mbidsOf(rows)) + } +} + +func TestCandidateTags_SettledWithTagsIsExcluded(t *testing.T) { + pool := newPool(t) + seed := seedArtist(t, pool, "Seed", "") + seedUnmatched(t, pool, seed.ID, "cand-1", "Candidate One", 0.9) + setState(t, pool, "cand-1", "musicbrainz", tagVersionCurrent) + + if rows := listEligible(t, pool, 10); len(rows) != 0 { + t.Errorf("len = %d, want 0 (already enriched): %v", len(rows), mbidsOf(rows)) + } +} + +// A candidate that settled 'none' becomes eligible again when the provider set +// widens (version bump) — that's the whole point of the version column. It must +// NOT be eligible at the current version, or the worker re-fetches it forever. +func TestCandidateTags_SettledNoneReopensOnlyOnVersionBump(t *testing.T) { + pool := newPool(t) + seed := seedArtist(t, pool, "Seed", "") + seedUnmatched(t, pool, seed.ID, "cand-1", "Candidate One", 0.9) + + setState(t, pool, "cand-1", "none", tagVersionCurrent) + if rows := listEligible(t, pool, 10); len(rows) != 0 { + t.Errorf("current version: len = %d, want 0 (settled)", len(rows)) + } + + setState(t, pool, "cand-1", "none", tagVersionOld) + if rows := listEligible(t, pool, 10); len(rows) != 1 { + t.Errorf("older version: len = %d, want 1 (eligible again)", len(rows)) + } +} + +// artist_similarity_unmatched holds one row per (seed, candidate, source). +// Without the GROUP BY, a candidate that five seeds point at would be fetched +// five times — five times the MusicBrainz budget for identical data. +func TestCandidateTags_ManySeedsCollapseToOneRowAndSumScores(t *testing.T) { + pool := newPool(t) + seedA := seedArtist(t, pool, "Seed A", "") + seedB := seedArtist(t, pool, "Seed B", "") + seedUnmatched(t, pool, seedA.ID, "cand-1", "Candidate One", 0.4) + seedUnmatched(t, pool, seedB.ID, "cand-1", "Candidate One", 0.3) + + rows := listEligible(t, pool, 10) + if len(rows) != 1 { + t.Fatalf("len = %d, want 1 (grouped): %v", len(rows), mbidsOf(rows)) + } + if got := rows[0].TotalScore; got < 0.69 || got > 0.71 { + t.Errorf("total_score = %v, want ~0.7 (summed across seeds)", got) + } +} + +// The pool is far larger than one pass can drain at ~1 req/s, so the ordering +// IS the feature: the strongest candidates must be enriched first, or the ones +// that actually reach a user's deck starve behind the long tail. +func TestCandidateTags_StrongestCandidatesComeFirst(t *testing.T) { + pool := newPool(t) + seed := seedArtist(t, pool, "Seed", "") + seedUnmatched(t, pool, seed.ID, "weak", "Weak", 0.1) + seedUnmatched(t, pool, seed.ID, "strong", "Strong", 0.95) + seedUnmatched(t, pool, seed.ID, "middle", "Middle", 0.5) + + rows := listEligible(t, pool, 10) + want := []string{"strong", "middle", "weak"} + got := mbidsOf(rows) + if len(got) != 3 { + t.Fatalf("len = %d, want 3: %v", len(got), got) + } + for i := range want { + if got[i] != want[i] { + t.Fatalf("order = %v, want %v", got, want) + } + } + + // And the limit takes the strongest, not an arbitrary slice. + if top := mbidsOf(listEligible(t, pool, 1)); len(top) != 1 || top[0] != "strong" { + t.Errorf("limit 1 returned %v, want [strong]", top) + } +} + +func TestCandidateTags_InsertKeepsTheStrongerWeight(t *testing.T) { + pool := newPool(t) + q := dbq.New(pool) + ctx := context.Background() + ins := func(w float64) { + if err := q.InsertCandidateArtistTag(ctx, dbq.InsertCandidateArtistTagParams{ + CandidateMbid: "cand-1", Tag: "shoegaze", Weight: w, + }); err != nil { + t.Fatalf("InsertCandidateArtistTag: %v", err) + } + } + ins(0.8) + ins(0.3) // weaker second write must not clobber + + rows, err := q.ListCandidateArtistTagsForMbids(ctx, []string{"cand-1"}) + if err != nil { + t.Fatalf("ListCandidateArtistTagsForMbids: %v", err) + } + if len(rows) != 1 { + t.Fatalf("len = %d, want 1", len(rows)) + } + if rows[0].Weight != 0.8 { + t.Errorf("weight = %v, want 0.8 (GREATEST)", rows[0].Weight) + } +} + +func TestCandidateTags_ListForMbidsIgnoresUnaskedCandidates(t *testing.T) { + pool := newPool(t) + q := dbq.New(pool) + ctx := context.Background() + for _, mbid := range []string{"want-1", "want-2", "other"} { + if err := q.InsertCandidateArtistTag(ctx, dbq.InsertCandidateArtistTagParams{ + CandidateMbid: mbid, Tag: "rock", Weight: 1, + }); err != nil { + t.Fatalf("insert %s: %v", mbid, err) + } + } + rows, err := q.ListCandidateArtistTagsForMbids(ctx, []string{"want-1", "want-2"}) + if err != nil { + t.Fatalf("ListCandidateArtistTagsForMbids: %v", err) + } + if len(rows) != 2 { + t.Errorf("len = %d, want 2", len(rows)) + } + for _, r := range rows { + if r.CandidateMbid == "other" { + t.Error("returned a candidate that wasn't asked for") + } + } +} + +// The similarity feed is refetched and churns, so without the sweep the cache +// only grows. Both halves must survive/die together for the right candidates. +func TestCandidateTags_GcDropsOrphansAndKeepsLiveOnes(t *testing.T) { + pool := newPool(t) + q := dbq.New(pool) + ctx := context.Background() + + seed := seedArtist(t, pool, "Seed", "") + seedUnmatched(t, pool, seed.ID, "live", "Live", 0.9) + // "gone" is cached but no longer in the feed; "adopted" got added to the + // library since, so its tags belong in track_tags now. + seedArtist(t, pool, "Adopted", "adopted") + seedUnmatched(t, pool, seed.ID, "adopted", "Adopted", 0.8) + + for _, mbid := range []string{"live", "gone", "adopted"} { + if err := q.InsertCandidateArtistTag(ctx, dbq.InsertCandidateArtistTagParams{ + CandidateMbid: mbid, Tag: "rock", Weight: 1, + }); err != nil { + t.Fatalf("insert %s: %v", mbid, err) + } + setState(t, pool, mbid, "musicbrainz", tagVersionCurrent) + } + + deleted, err := q.GcDeleteOrphanedCandidateArtistTags(ctx) + if err != nil { + t.Fatalf("GcDeleteOrphanedCandidateArtistTags: %v", err) + } + if deleted != 2 { + t.Errorf("deleted %d tag rows, want 2 (gone + adopted)", deleted) + } + deletedState, err := q.GcDeleteOrphanedCandidateArtistTagState(ctx) + if err != nil { + t.Fatalf("GcDeleteOrphanedCandidateArtistTagState: %v", err) + } + if deletedState != 2 { + t.Errorf("deleted %d state rows, want 2", deletedState) + } + + rows, err := q.ListCandidateArtistTagsForMbids(ctx, []string{"live", "gone", "adopted"}) + if err != nil { + t.Fatalf("ListCandidateArtistTagsForMbids: %v", err) + } + if len(rows) != 1 || rows[0].CandidateMbid != "live" { + t.Errorf("survivors = %v, want [live] only", rows) + } +} + +// Coverage is the operator's window onto the honest ceiling: processed vs +// actually-tagged. A big gap means thin upstream data, not a broken worker. +func TestCandidateTags_CoverageCountsProcessedAndTagged(t *testing.T) { + pool := newPool(t) + setState(t, pool, "has-tags", "musicbrainz", tagVersionCurrent) + setState(t, pool, "mixed-tags", "mixed", tagVersionCurrent) + setState(t, pool, "no-tags", "none", tagVersionCurrent) + + got, err := dbq.New(pool).CountCandidateArtistTagCoverage(context.Background()) + if err != nil { + t.Fatalf("CountCandidateArtistTagCoverage: %v", err) + } + if got.Processed != 3 { + t.Errorf("processed = %d, want 3", got.Processed) + } + if got.WithTags != 2 { + t.Errorf("with_tags = %d, want 2 ('none' excluded)", got.WithTags) + } +} diff --git a/internal/tags/enricher.go b/internal/tags/enricher.go index 60b9909f..b41ac4cb 100644 --- a/internal/tags/enricher.go +++ b/internal/tags/enricher.go @@ -61,40 +61,25 @@ func NewEnricher(pool *pgxpool.Pool, logger *slog.Logger, settings *SettingsServ // enabled) the row settles to 'none'. If a provider fails transiently and // no tags surfaced, the row is left NULL for a next-pass retry. func (e *Enricher) EnrichTrack(ctx context.Context, trackID pgtype.UUID, ref TrackRef) (outcome, error) { - merged := map[string]float64{} - contributors := map[string]bool{} - anyTransient := false - - for _, provider := range e.settings.EnabledTrackTagProviders() { - tags, perr := provider.FetchTrackTags(ctx, ref) - switch { - case perr == nil: - for _, t := range tags { - if t.Weight > merged[t.Name] { - merged[t.Name] = t.Weight - } - } - if len(tags) > 0 { - contributors[provider.ID()] = true - } - case errors.Is(perr, ErrNotFound): - // Clean "no data from this source" — try the next provider. - default: - anyTransient = true - e.logger.Warn("tags: provider fetch failed; continuing", - "track_id", uuidString(trackID), "provider", provider.ID(), "err", perr) - } + providers := e.settings.EnabledTrackTagProviders() + calls := make([]tagFetch, 0, len(providers)) + for _, provider := range providers { + calls = append(calls, tagFetch{ + providerID: provider.ID(), + fetch: func(c context.Context) ([]Tag, error) { return provider.FetchTrackTags(c, ref) }, + }) } + res := e.runChain(ctx, calls, "track_id", uuidString(trackID)) - if len(merged) > 0 { - top := topKByWeight(merged, e.topK) - source := sourceLabel(contributorIDs(contributors)) + if len(res.merged) > 0 { + top := topKByWeight(res.merged, e.topK) + source := sourceLabel(res.contributors) if err := e.writeTags(ctx, trackID, top, source, e.settings.CurrentVersion()); err != nil { return outcomeLeftNull, err } return outcomeEnriched, nil } - if anyTransient { + if res.anyTransient { // Nothing landed but a source may recover — leave NULL for retry. return outcomeLeftNull, nil } @@ -106,6 +91,65 @@ func (e *Enricher) EnrichTrack(ctx context.Context, trackID pgtype.UUID, ref Tra return outcomeNone, nil } +// tagFetch pairs a provider ID with a bound fetch call, so the merge-and- +// classify loop below is shared between the track chain and the candidate- +// artist chain (#2376) instead of being written twice with one word changed. +type tagFetch struct { + providerID string + fetch func(context.Context) ([]Tag, error) +} + +// chainResult is what running a provider chain produced. contributors is the +// sorted set of provider IDs that actually returned tags — the input to +// sourceLabel. +type chainResult struct { + merged map[string]float64 + contributors []string + anyTransient bool +} + +// runChain queries every provider in order and UNIONS the results (max weight +// wins on overlap), unlike coverart's first-success-wins. +// +// A clean ErrNotFound means "this source has nothing" and moves to the next. +// Anything else is transient and recorded, so the caller can leave the row +// eligible for a retry rather than wrongly settling it as "nothing exists" — +// the distinction between those two is the whole point of the return value. +// +// logKey/logVal identify the subject in warnings (a track id or a candidate +// MBID), since this is shared across entity types. +func (e *Enricher) runChain(ctx context.Context, calls []tagFetch, logKey, logVal string) chainResult { + merged := map[string]float64{} + contributors := map[string]bool{} + anyTransient := false + + for _, c := range calls { + tags, perr := c.fetch(ctx) + switch { + case perr == nil: + for _, t := range tags { + if t.Weight > merged[t.Name] { + merged[t.Name] = t.Weight + } + } + if len(tags) > 0 { + contributors[c.providerID] = true + } + case errors.Is(perr, ErrNotFound): + // Clean "no data from this source" — try the next provider. + default: + anyTransient = true + e.logger.Warn("tags: provider fetch failed; continuing", + logKey, logVal, "provider", c.providerID, "err", perr) + } + } + return chainResult{ + merged: merged, + contributors: contributorIDs(contributors), + anyTransient: anyTransient, + } +} + // writeTags atomically replaces a track's cached tags and stamps the source // + version. tags may be empty (the 'none' settle path), which just clears // any prior tags and records the outcome. @@ -196,6 +240,146 @@ func (e *Enricher) EnrichTrackBatch(ctx context.Context, limit int, return processed, enriched, settledNone + leftNull + errored, nil } +// EnrichCandidateArtist runs the artist-tag chain for one out-of-library +// Discover candidate and caches the merged result (#2376). +// +// Mirrors EnrichTrack, with one deliberate difference in the transient case: +// there is no row to "leave NULL", because eligibility is the ABSENCE of a +// candidate_artist_tag_state row. So a transient failure writes nothing at all, +// which leaves the candidate eligible for the next tick. Writing a state row +// here would settle a candidate whose tags we simply failed to fetch. +func (e *Enricher) EnrichCandidateArtist(ctx context.Context, mbid, name string) (outcome, error) { + providers := e.settings.EnabledArtistTagProviders() + ref := ArtistRef{MBID: mbid, Name: name} + calls := make([]tagFetch, 0, len(providers)) + for _, provider := range providers { + calls = append(calls, tagFetch{ + providerID: provider.ID(), + fetch: func(c context.Context) ([]Tag, error) { return provider.FetchArtistTags(c, ref) }, + }) + } + res := e.runChain(ctx, calls, "candidate_mbid", mbid) + version := e.settings.CurrentVersion() + + if len(res.merged) > 0 { + top := topKByWeight(res.merged, e.topK) + if err := e.writeCandidateTags(ctx, mbid, top, sourceLabel(res.contributors), version); err != nil { + return outcomeLeftNull, err + } + return outcomeEnriched, nil + } + if res.anyTransient { + return outcomeLeftNull, nil + } + if err := e.writeCandidateTags(ctx, mbid, nil, sourceNone, version); err != nil { + return outcomeNone, err + } + return outcomeNone, nil +} + +// writeCandidateTags atomically replaces a candidate's cached tags and stamps +// its state. tags may be empty (the 'none' settle path), which clears any prior +// tags and records that the providers had nothing. +func (e *Enricher) writeCandidateTags( + ctx context.Context, mbid string, tags []Tag, source string, version int32, +) error { + tx, err := e.pool.Begin(ctx) + if err != nil { + return fmt.Errorf("begin tx: %w", err) + } + defer func() { _ = tx.Rollback(ctx) }() + q := dbq.New(tx) + if err := q.DeleteCandidateArtistTags(ctx, mbid); err != nil { + return fmt.Errorf("delete candidate artist tags: %w", err) + } + for _, t := range tags { + if err := q.InsertCandidateArtistTag(ctx, dbq.InsertCandidateArtistTagParams{ + CandidateMbid: mbid, Tag: t.Name, Weight: t.Weight, + }); err != nil { + return fmt.Errorf("insert candidate artist tag: %w", err) + } + } + if err := q.SetCandidateArtistTagState(ctx, dbq.SetCandidateArtistTagStateParams{ + CandidateMbid: mbid, + TagSource: source, + TagSourcesVersion: version, + }); err != nil { + return fmt.Errorf("set candidate artist tag state: %w", err) + } + if err := tx.Commit(ctx); err != nil { + return fmt.Errorf("commit: %w", err) + } + return nil +} + +// EnrichCandidateArtistBatch drains up to limit out-of-library candidates and +// enriches each serially. Same limit semantics as EnrichTrackBatch: 0 = +// disabled, >0 = bounded, <0 = unbounded. +// +// The candidate pool is far larger than the track pool (every library artist's +// neighbours) and can never be drained in one pass at MusicBrainz's ~1 req/s, +// so the query hands them back in descending similarity order — see +// ListCandidateArtistsMissingTags. A bounded batch here is therefore normal +// operation, not a degraded mode. +func (e *Enricher) EnrichCandidateArtistBatch(ctx context.Context, limit int) ( + processed, succeeded, failed int, err error, +) { + if limit == 0 { + return 0, 0, 0, nil + } + queryLimit := int32(limit) + if limit < 0 { + queryLimit = 1<<31 - 1 + } + q := dbq.New(e.pool) + rows, qerr := q.ListCandidateArtistsMissingTags(ctx, dbq.ListCandidateArtistsMissingTagsParams{ + TagSourcesVersion: e.settings.CurrentVersion(), + Limit: queryLimit, + }) + if qerr != nil { + return 0, 0, 0, fmt.Errorf("list candidate artists missing tags: %w", qerr) + } + + var enriched, settledNone, leftNull, errored int + for _, r := range rows { + if ctx.Err() != nil { + e.logCandidateBatchSummary(len(rows), processed, enriched, settledNone, leftNull, errored) + return processed, enriched, settledNone + leftNull + errored, ctx.Err() + } + processed++ + oc, eerr := e.EnrichCandidateArtist(ctx, r.CandidateMbid, r.CandidateName) + if eerr != nil { + e.logger.Warn("tags: candidate batch entry failed", + "candidate_mbid", r.CandidateMbid, "err", eerr) + errored++ + continue + } + switch oc { + case outcomeEnriched: + enriched++ + case outcomeNone: + settledNone++ + case outcomeLeftNull: + leftNull++ + } + } + e.logCandidateBatchSummary(len(rows), processed, enriched, settledNone, leftNull, errored) + return processed, enriched, settledNone + leftNull + errored, nil +} + +// logCandidateBatchSummary mirrors logBatchSummary. `settled_none` is the +// honest-ceiling counter for this surface: candidates whose MBID has no +// upstream tags at all, which no amount of retrying will fix. +func (e *Enricher) logCandidateBatchSummary(eligible, processed, enriched, settledNone, leftNull, errored int) { + e.logger.Info("tags: candidate-artist enrichment batch complete", + "eligible", eligible, + "processed", processed, + "enriched", enriched, + "settled_none", settledNone, + "left_null", leftNull, + "errored", errored) +} + // logBatchSummary emits one Info line with the category breakdown — the // enriched/settled/left-null split is the operator's diagnostic for a // "0 enriched" symptom the collapsed tally can't explain. diff --git a/internal/tags/enricher_chain_test.go b/internal/tags/enricher_chain_test.go new file mode 100644 index 00000000..23b73fb4 --- /dev/null +++ b/internal/tags/enricher_chain_test.go @@ -0,0 +1,142 @@ +package tags + +import ( + "context" + "errors" + "io" + "log/slog" + "testing" +) + +// runChain is the merge-and-classify loop shared by the track and +// candidate-artist drains (#2376). It touches no DB, so it is testable with a +// bare Enricher. +// +// The classification is the part that matters: "every source cleanly had +// nothing" and "a source failed" lead to opposite persistence decisions +// (settle vs. leave eligible for retry), and conflating them either writes off +// an artist over a transient blip or re-fetches a genuinely untagged one +// forever. + +func chainEnricher() *Enricher { + return &Enricher{logger: slog.New(slog.NewTextHandler(io.Discard, nil))} +} + +func fixedFetch(tags []Tag, err error) func(context.Context) ([]Tag, error) { + return func(context.Context) ([]Tag, error) { return tags, err } +} + +func TestRunChain_UnionsAcrossProvidersMaxWeightWins(t *testing.T) { + res := chainEnricher().runChain(context.Background(), []tagFetch{ + {providerID: "musicbrainz", fetch: fixedFetch([]Tag{ + {Name: "shoegaze", Weight: 0.4}, + {Name: "noise", Weight: 0.9}, + }, nil)}, + {providerID: "lastfm", fetch: fixedFetch([]Tag{ + {Name: "shoegaze", Weight: 0.8}, // higher — should win + {Name: "dream pop", Weight: 0.3}, + }, nil)}, + }, "subject", "x") + + if got := res.merged["shoegaze"]; got != 0.8 { + t.Errorf("shoegaze = %v, want 0.8 (max across providers)", got) + } + if got := res.merged["noise"]; got != 0.9 { + t.Errorf("noise = %v, want 0.9", got) + } + if got := res.merged["dream pop"]; got != 0.3 { + t.Errorf("dream pop = %v, want 0.3", got) + } + if len(res.merged) != 3 { + t.Errorf("merged has %d tags, want 3: %v", len(res.merged), res.merged) + } + if res.anyTransient { + t.Error("anyTransient set with no failures") + } + if len(res.contributors) != 2 { + t.Errorf("contributors = %v, want both providers", res.contributors) + } +} + +// A lower weight arriving second must not overwrite a higher one — the +// ordering of the chain must not change the result. +func TestRunChain_LowerWeightSecondDoesNotClobber(t *testing.T) { + res := chainEnricher().runChain(context.Background(), []tagFetch{ + {providerID: "a", fetch: fixedFetch([]Tag{{Name: "rock", Weight: 0.9}}, nil)}, + {providerID: "b", fetch: fixedFetch([]Tag{{Name: "rock", Weight: 0.2}}, nil)}, + }, "subject", "x") + if got := res.merged["rock"]; got != 0.9 { + t.Errorf("rock = %v, want 0.9", got) + } +} + +func TestRunChain_NotFoundIsSkippedNotTransient(t *testing.T) { + res := chainEnricher().runChain(context.Background(), []tagFetch{ + {providerID: "a", fetch: fixedFetch(nil, ErrNotFound)}, + {providerID: "b", fetch: fixedFetch([]Tag{{Name: "folk", Weight: 1}}, nil)}, + }, "subject", "x") + + if res.anyTransient { + t.Error("ErrNotFound must not be treated as transient — it would keep a settled subject eligible forever") + } + if len(res.contributors) != 1 || res.contributors[0] != "b" { + t.Errorf("contributors = %v, want [b] only (a returned nothing)", res.contributors) + } +} + +func TestRunChain_TransientIsRecordedEvenWhenAnotherProviderSucceeds(t *testing.T) { + res := chainEnricher().runChain(context.Background(), []tagFetch{ + {providerID: "a", fetch: fixedFetch(nil, ErrTransient)}, + {providerID: "b", fetch: fixedFetch([]Tag{{Name: "folk", Weight: 1}}, nil)}, + }, "subject", "x") + + if !res.anyTransient { + t.Error("anyTransient should be set — 'a' may have had tags we never saw") + } + // Tags DID land, so the caller writes them; anyTransient only decides the + // no-tags case. Asserting both here pins that they're independent. + if len(res.merged) != 1 { + t.Errorf("merged = %v, want folk", res.merged) + } +} + +// An unexpected error type is transient, not terminal. Defaulting the other way +// would settle a subject on any bug in a provider. +func TestRunChain_UnknownErrorIsTransient(t *testing.T) { + res := chainEnricher().runChain(context.Background(), []tagFetch{ + {providerID: "a", fetch: fixedFetch(nil, errors.New("boom"))}, + }, "subject", "x") + if !res.anyTransient { + t.Error("unknown error should count as transient") + } + if len(res.merged) != 0 { + t.Errorf("merged = %v, want empty", res.merged) + } +} + +// A provider returning (empty, nil) is not a contributor: sourceLabel would +// otherwise stamp its ID onto a subject it gave nothing to. +func TestRunChain_EmptySuccessIsNotAContributor(t *testing.T) { + res := chainEnricher().runChain(context.Background(), []tagFetch{ + {providerID: "a", fetch: fixedFetch(nil, nil)}, + {providerID: "b", fetch: fixedFetch([]Tag{{Name: "folk", Weight: 1}}, nil)}, + }, "subject", "x") + if len(res.contributors) != 1 || res.contributors[0] != "b" { + t.Errorf("contributors = %v, want [b]", res.contributors) + } +} + +// No enabled providers is a clean "nothing found", NOT a failure — the caller +// settles the subject rather than retrying an empty chain on every tick. +func TestRunChain_EmptyChainSettlesRatherThanRetries(t *testing.T) { + res := chainEnricher().runChain(context.Background(), nil, "subject", "x") + if res.anyTransient { + t.Error("an empty chain must not look transient") + } + if len(res.merged) != 0 || len(res.contributors) != 0 { + t.Errorf("empty chain produced %v / %v", res.merged, res.contributors) + } + if sourceLabel(res.contributors) != sourceNone { + t.Errorf("sourceLabel = %q, want %q", sourceLabel(res.contributors), sourceNone) + } +} diff --git a/internal/tags/provider.go b/internal/tags/provider.go index d1d8c8ce..900825f3 100644 --- a/internal/tags/provider.go +++ b/internal/tags/provider.go @@ -87,6 +87,32 @@ type TrackTagProvider interface { FetchTrackTags(ctx context.Context, ref TrackRef) ([]Tag, error) } +// ArtistRef is the lookup key for artist-level tags. MBID is the artist's +// MusicBrainz ID (required by MBID-keyed providers); Name is the fallback for +// name-based providers (Last.fm). At least one must be set or every provider +// returns ErrNotFound. +type ArtistRef struct { + MBID string + Name string +} + +// ArtistTagProvider is the artist-level tag capability, added for tag-space +// Discover (#2376). Separate from TrackTagProvider — exactly the split that +// interface's doc comment anticipated — so a source can implement either +// without the other, and so the enricher can ask for the capability it needs +// rather than checking at the call site. +// +// The subject here is an artist Minstrel does NOT have locally, so there is no +// track to fall back to and no recording-level tag to prefer. +type ArtistTagProvider interface { + Provider + // FetchArtistTags returns the artist's folksonomy tags, or ErrNotFound + // (terminal — nothing upstream) / ErrTransient (retry). A disabled + // provider, or one missing a required key, returns ErrNotFound so the + // chain simply skips it. + FetchArtistTags(ctx context.Context, ref ArtistRef) ([]Tag, error) +} + // TestableProvider is an opt-in capability for the admin Test-Connection // button: answer "is my config working?" without a full enrichment cycle. type TestableProvider interface { diff --git a/internal/tags/provider_artist_tags_test.go b/internal/tags/provider_artist_tags_test.go new file mode 100644 index 00000000..489be798 --- /dev/null +++ b/internal/tags/provider_artist_tags_test.go @@ -0,0 +1,173 @@ +package tags + +import ( + "context" + "errors" + "net/http" + "net/http/httptest" + "testing" +) + +// Artist-level tag fetching for out-of-library Discover candidates (#2376). +// Reuses the mbEntityServer / newMBProvider / newLastfmProvider helpers from +// the per-provider test files. + +func TestMusicBrainzFetchArtistTags_UsesFullWeightNotTheFallbackDiscount(t *testing.T) { + // THE distinction worth a test. artistTagWeightFactor (0.6) exists because + // FetchTrackTags uses artist tags as a *proxy* for a track's tags. Here the + // artist IS the subject, so weights must land unscaled — otherwise these + // are not comparable with track_tags, which is the exact comparison slice 6 + // is built on. + srv := mbEntityServer(``, `{"tags":[{"count":4,"name":"shoegaze"},{"count":2,"name":"dream pop"}]}`) + defer srv.Close() + old := mbBaseURL + mbBaseURL = srv.URL + defer func() { mbBaseURL = old }() + + tags, err := newMBProvider(true).FetchArtistTags(context.Background(), + ArtistRef{MBID: "art-1", Name: "Some Band"}) + if err != nil { + t.Fatalf("fetch: %v", err) + } + m := tagsByName(tags) + if got := m["shoegaze"]; got != 1.0 { + t.Errorf("shoegaze = %v, want 1.0 — not discounted by artistTagWeightFactor", got) + } + if got := m["dream pop"]; got != 0.5 { + t.Errorf("dream pop = %v, want 0.5", got) + } +} + +func TestMusicBrainzFetchArtistTags_GatedOff(t *testing.T) { + if _, err := newMBProvider(false).FetchArtistTags(context.Background(), + ArtistRef{MBID: "art-1"}); !errors.Is(err, ErrNotFound) { + t.Errorf("disabled: err = %v, want ErrNotFound", err) + } + // No MBID → nothing MusicBrainz can look up. A name-based guess could + // silently attach the wrong artist's tags, so it deliberately doesn't try. + if _, err := newMBProvider(true).FetchArtistTags(context.Background(), + ArtistRef{Name: "Some Band"}); !errors.Is(err, ErrNotFound) { + t.Errorf("no MBID: err = %v, want ErrNotFound", err) + } +} + +// fetchEntityTags reports an existing-but-untagged entity as (empty, nil) so +// FetchTrackTags can fall through to the artist level. FetchArtistTags has no +// next level, so it must convert that to the terminal ErrNotFound — otherwise +// the enricher would read "no error" as success and settle the candidate as +// enriched with zero tags. +func TestMusicBrainzFetchArtistTags_UntaggedArtistIsNotFound(t *testing.T) { + srv := mbEntityServer(``, `{"tags":[]}`) + defer srv.Close() + old := mbBaseURL + mbBaseURL = srv.URL + defer func() { mbBaseURL = old }() + + _, err := newMBProvider(true).FetchArtistTags(context.Background(), ArtistRef{MBID: "art-1"}) + if !errors.Is(err, ErrNotFound) { + t.Errorf("err = %v, want ErrNotFound", err) + } +} + +func TestLastfmFetchArtistTags_CallsArtistGetTopTags(t *testing.T) { + var gotMethod, gotArtist, gotMBID string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotMethod = r.URL.Query().Get("method") + gotArtist = r.URL.Query().Get("artist") + gotMBID = r.URL.Query().Get("mbid") + _, _ = w.Write([]byte(`{"toptags":{"tag":[{"name":"post-punk","count":100},{"name":"moody","count":40}]}}`)) + })) + defer srv.Close() + old := lastfmBaseURL + lastfmBaseURL = srv.URL + "/" + defer func() { lastfmBaseURL = old }() + + p := &lastfmProvider{client: newHTTPClient(httpClientOptions{Name: "lastfm", MaxRetries: 1})} + _ = p.Configure(ProviderSettings{Enabled: true, APIKey: "k"}) + + tags, err := p.FetchArtistTags(context.Background(), + ArtistRef{MBID: "art-1", Name: "Some Band"}) + if err != nil { + t.Fatalf("fetch: %v", err) + } + if gotMethod != "artist.gettoptags" { + t.Errorf("method = %q, want artist.gettoptags", gotMethod) + } + if gotArtist != "Some Band" { + t.Errorf("artist = %q, want Some Band", gotArtist) + } + // MBID is sent as a disambiguating hint when we have one. + if gotMBID != "art-1" { + t.Errorf("mbid = %q, want art-1", gotMBID) + } + m := tagsByName(tags) + if m["post-punk"] != 1.0 { + t.Errorf("post-punk = %v, want 1.0", m["post-punk"]) + } + if m["moody"] != 0.4 { + t.Errorf("moody = %v, want 0.4", m["moody"]) + } +} + +func TestLastfmFetchArtistTags_GatedOff(t *testing.T) { + unkeyed := &lastfmProvider{client: newHTTPClient(httpClientOptions{Name: "lastfm", MaxRetries: 1})} + _ = unkeyed.Configure(ProviderSettings{Enabled: true}) + if _, err := unkeyed.FetchArtistTags(context.Background(), + ArtistRef{Name: "A"}); !errors.Is(err, ErrNotFound) { + t.Errorf("unkeyed: err = %v, want ErrNotFound", err) + } + + keyed := &lastfmProvider{client: newHTTPClient(httpClientOptions{Name: "lastfm", MaxRetries: 1})} + _ = keyed.Configure(ProviderSettings{Enabled: false, APIKey: "k"}) + if _, err := keyed.FetchArtistTags(context.Background(), + ArtistRef{Name: "A"}); !errors.Is(err, ErrNotFound) { + t.Errorf("disabled: err = %v, want ErrNotFound", err) + } + + // Name-based provider with no name → nothing to query. This is the case + // that matters in practice: a candidate whose name coalesced to '' still + // has an MBID, so MusicBrainz can serve it while Last.fm cannot. + enabled := &lastfmProvider{client: newHTTPClient(httpClientOptions{Name: "lastfm", MaxRetries: 1})} + _ = enabled.Configure(ProviderSettings{Enabled: true, APIKey: "k"}) + if _, err := enabled.FetchArtistTags(context.Background(), + ArtistRef{MBID: "art-1"}); !errors.Is(err, ErrNotFound) { + t.Errorf("no name: err = %v, want ErrNotFound", err) + } +} + +func TestLastfmFetchArtistTags_TransientErrorCodeRetries(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{"error":29}`)) // rate limited + })) + defer srv.Close() + old := lastfmBaseURL + lastfmBaseURL = srv.URL + "/" + defer func() { lastfmBaseURL = old }() + + p := &lastfmProvider{client: newHTTPClient(httpClientOptions{Name: "lastfm", MaxRetries: 1})} + _ = p.Configure(ProviderSettings{Enabled: true, APIKey: "k"}) + _, err := p.FetchArtistTags(context.Background(), ArtistRef{Name: "A"}) + // Must be ErrTransient, not ErrNotFound: the enricher settles a candidate + // to 'none' on ErrNotFound, which would permanently write off an artist we + // were merely throttled on. + if !errors.Is(err, ErrTransient) { + t.Errorf("err = %v, want ErrTransient", err) + } +} + +func TestLastfmFetchArtistTags_UnknownArtistIsNotFound(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{"error":6}`)) // "not found" — terminal + })) + defer srv.Close() + old := lastfmBaseURL + lastfmBaseURL = srv.URL + "/" + defer func() { lastfmBaseURL = old }() + + p := &lastfmProvider{client: newHTTPClient(httpClientOptions{Name: "lastfm", MaxRetries: 1})} + _ = p.Configure(ProviderSettings{Enabled: true, APIKey: "k"}) + if _, err := p.FetchArtistTags(context.Background(), + ArtistRef{Name: "Nobody"}); !errors.Is(err, ErrNotFound) { + t.Errorf("err = %v, want ErrNotFound", err) + } +} diff --git a/internal/tags/provider_lastfm.go b/internal/tags/provider_lastfm.go index e37a3eb8..12917333 100644 --- a/internal/tags/provider_lastfm.go +++ b/internal/tags/provider_lastfm.go @@ -121,6 +121,50 @@ func (p *lastfmProvider) FetchTrackTags(ctx context.Context, ref TrackRef) ([]Ta return out, nil } +// FetchArtistTags looks up an artist's top tags by name, with the MBID as an +// extra hint when present (#2376). Name-first is deliberate and the opposite +// emphasis from MusicBrainz: Last.fm's tag data is keyed on its own artist +// pages, and autocorrect resolves most spelling drift from the similarity feed. +// +// `artist.getTopTags` returns the same `toptags` envelope as +// `track.getTopTags`, so the response type and normalizer are reused as-is — +// the 0-100 popularity scale is identical. +func (p *lastfmProvider) FetchArtistTags(ctx context.Context, ref ArtistRef) ([]Tag, error) { + if !p.enabled.Load() || p.currentKey() == "" { + return nil, ErrNotFound + } + if ref.Name == "" { + return nil, ErrNotFound + } + + q := url.Values{ + "method": {"artist.gettoptags"}, + "api_key": {p.currentKey()}, + "format": {"json"}, + "artist": {ref.Name}, + "autocorrect": {"1"}, + } + if ref.MBID != "" { + q.Set("mbid", ref.MBID) + } + + var resp lastfmTopTags + if err := p.client.getJSON(ctx, lastfmBaseURL+"?"+q.Encode(), &resp); err != nil { + return nil, err + } + if resp.Error != 0 { + if lastfmTransientErrors[resp.Error] { + return nil, ErrTransient + } + return nil, ErrNotFound + } + out := normalizeLastfmTags(resp.TopTags.Tag) + if len(out) == 0 { + return nil, ErrNotFound + } + return out, nil +} + // TestConnection verifies the key against a well-known track. func (p *lastfmProvider) TestConnection(ctx context.Context) error { if p.currentKey() == "" { @@ -163,6 +207,7 @@ func normalizeLastfmTags(raw []lastfmTag) []Tag { // Compile-time capability checks. var ( - _ TrackTagProvider = (*lastfmProvider)(nil) - _ TestableProvider = (*lastfmProvider)(nil) + _ TrackTagProvider = (*lastfmProvider)(nil) + _ ArtistTagProvider = (*lastfmProvider)(nil) + _ TestableProvider = (*lastfmProvider)(nil) ) diff --git a/internal/tags/provider_musicbrainz.go b/internal/tags/provider_musicbrainz.go index 5c89e7d9..5a0362c5 100644 --- a/internal/tags/provider_musicbrainz.go +++ b/internal/tags/provider_musicbrainz.go @@ -96,6 +96,33 @@ func (p *musicbrainzProvider) FetchTrackTags(ctx context.Context, ref TrackRef) return nil, ErrNotFound } +// FetchArtistTags looks up an artist's own tags by MBID (#2376). MBID-only: +// MusicBrainz has no name-based tag lookup worth trusting for this, and a +// wrong-artist match would poison the tag cache silently. +// +// Note the scale is 1.0, NOT artistTagWeightFactor. That discount exists +// because FetchTrackTags uses artist tags as a *proxy* for a track's tags, and +// the artist's overall character is the coarser signal of the two. Here the +// artist IS the subject, so there is nothing to discount relative to — and +// applying it would make these weights incomparable with track_tags, which is +// exactly the comparison slice 6 depends on. +func (p *musicbrainzProvider) FetchArtistTags(ctx context.Context, ref ArtistRef) ([]Tag, error) { + if !p.enabled.Load() || ref.MBID == "" { + return nil, ErrNotFound + } + tags, err := p.fetchEntityTags(ctx, "artist", ref.MBID, 1.0) + if err != nil { + return nil, err + } + // fetchEntityTags reports an existing-but-untagged entity as (empty, nil) + // so FetchTrackTags can fall through to the next level. There is no next + // level here, so empty is the terminal "nothing upstream". + if len(tags) == 0 { + return nil, ErrNotFound + } + return tags, nil +} + // fetchEntityTags loads folksonomy tags for a MusicBrainz entity ("recording" // or "artist") by MBID and scales the normalized weights by `scale`. Returns // an empty slice (not ErrNotFound) when the entity exists but is untagged, so @@ -153,6 +180,7 @@ func normalizeMBTags(raw []mbTag) []Tag { // Compile-time capability checks. var ( - _ TrackTagProvider = (*musicbrainzProvider)(nil) - _ TestableProvider = (*musicbrainzProvider)(nil) + _ TrackTagProvider = (*musicbrainzProvider)(nil) + _ ArtistTagProvider = (*musicbrainzProvider)(nil) + _ TestableProvider = (*musicbrainzProvider)(nil) ) diff --git a/internal/tags/settings.go b/internal/tags/settings.go index b16a3117..3914bb3d 100644 --- a/internal/tags/settings.go +++ b/internal/tags/settings.go @@ -135,6 +135,27 @@ func (s *SettingsService) EnabledTrackTagProviders() []TrackTagProvider { return out } +// EnabledArtistTagProviders returns the enabled providers implementing +// ArtistTagProvider, in registration order. Snapshot — do not mutate. +// +// Separate from EnabledTrackTagProviders rather than one call with a capability +// argument: the two chains are consumed by different drains, and a provider may +// implement one capability without the other. +func (s *SettingsService) EnabledArtistTagProviders() []ArtistTagProvider { + s.mu.RLock() + defer s.mu.RUnlock() + var out []ArtistTagProvider + for _, p := range AllProviders() { + if !s.enabledIDs[p.ID()] { + continue + } + if ap, ok := p.(ArtistTagProvider); ok { + out = append(out, ap) + } + } + return out +} + // CurrentVersion returns the version the enricher stamps onto rows. func (s *SettingsService) CurrentVersion() int32 { s.mu.RLock() diff --git a/internal/tags/worker.go b/internal/tags/worker.go index 701a08f3..35ab883c 100644 --- a/internal/tags/worker.go +++ b/internal/tags/worker.go @@ -14,22 +14,32 @@ import ( // providers' httpClients, so a tick just drains a bounded batch and the // external APIs pace themselves. type Worker struct { - enricher *Enricher - logger *slog.Logger - tick time.Duration - batch int + enricher *Enricher + logger *slog.Logger + tick time.Duration + batch int + candidateBatch int } // NewWorker constructs a worker with production defaults: an initial drain // shortly after boot, then every 30 minutes, up to 200 tracks per tick. // MusicBrainz's 1 req/s ceiling is the real throttle, so the batch size // mainly bounds how long one tick runs, not the request rate. +// +// candidateBatch is smaller than the track batch on purpose. Library tracks are +// a finite set that drains to completion and then costs nothing; out-of-library +// candidates (#2376) are effectively unbounded — every library artist's +// neighbours — so this arm would otherwise monopolise every tick forever and +// starve the track arm. 50/tick at ~1 req/s is roughly a minute of work, and +// the query hands back the highest-similarity candidates first so the ones that +// can actually reach a user's deck are enriched first. func NewWorker(enricher *Enricher, logger *slog.Logger) *Worker { return &Worker{ - enricher: enricher, - logger: logger, - tick: 30 * time.Minute, - batch: 200, + enricher: enricher, + logger: logger, + tick: 30 * time.Minute, + batch: 200, + candidateBatch: 50, } } @@ -48,12 +58,27 @@ func (w *Worker) Run(ctx context.Context) { } } -// tickOnce drains one bounded batch. EnrichTrackBatch already logs a -// category breakdown, so this only surfaces a fatal batch error. +// tickOnce drains one bounded batch of each kind. Both Enrich*Batch methods +// already log a category breakdown, so this only surfaces a fatal batch error. +// +// Tracks first: they back the taste profile the whole app reads from, whereas +// candidate tags only affect the Discover request surface. On a fresh install +// both are cold, and getting the profile warm matters more. func (w *Worker) tickOnce(ctx context.Context) { if _, _, _, err := w.enricher.EnrichTrackBatch(ctx, w.batch, nil); err != nil { if ctx.Err() == nil { w.logger.Error("tags: enrichment tick failed", "err", err) } } + // Not gated on the track arm's success: the two drains share nothing but a + // provider chain, and a track-side failure says nothing about whether + // candidate lookups will work. + if ctx.Err() != nil { + return + } + if _, _, _, err := w.enricher.EnrichCandidateArtistBatch(ctx, w.candidateBatch); err != nil { + if ctx.Err() == nil { + w.logger.Error("tags: candidate-artist enrichment tick failed", "err", err) + } + } }