feat(discover): artist-tag cache for out-of-library candidates — #2376
Migration 0050 adds candidate_artist_tags + candidate_artist_tag_state: folksonomy tags for artists NOT in the library, which track_tags cannot hold because it's FK'd to tracks(id) and a Discover candidate has no local row. Slice 6 ranks against these; this slice only fills the cache. The reuse the task claimed is real and verified: MusicBrainz's fetchEntityTags(ctx, "artist", mbid, scale) already existed for the #1519 recording→artist fallback, so FetchArtistTags is a thin wrapper. Two subtleties it does NOT inherit: - Weight scale is 1.0, not artistTagWeightFactor (0.6). That discount exists because FetchTrackTags uses artist tags as a *proxy* for a track's; here the artist IS the subject. Applying it would make these weights incomparable with track_tags — exactly the comparison slice 6 depends on. Pinned by a test. - fetchEntityTags reports existing-but-untagged as (empty, nil) so the track path can fall through. There's no next level here, so empty becomes the terminal ErrNotFound; otherwise the enricher would settle a candidate as "enriched" with zero tags. ArtistTagProvider is the split TrackTagProvider's own doc comment anticipated ("e.g. artist-level tags"). Last.fm gains artist.getTopTags, which returns the same toptags envelope, so the response type and normalizer are reused unchanged. Rather than write the merge-and-classify loop twice, extracted it from EnrichTrack into runChain(). The ErrNotFound-vs-transient split is the load-bearing part — those lead to opposite persistence decisions — so it now has direct unit tests it never had while inlined. Bookkeeping is a separate table, not columns, because the "providers had nothing" outcome must be recordable for a candidate with zero tag rows, and there is no per-candidate row to hang columns off ( artist_similarity_unmatched holds many rows per candidate). Absence of a state row means "never processed", so a transient failure writes nothing and stays eligible. Two capacity realities are designed for, not papered over: - The pool is O(library artists x neighbours) and MusicBrainz allows ~1 req/s, so it can never drain in one pass. The eligibility query returns candidates in descending summed-similarity order, so the ones that can actually reach a deck are enriched first. - candidateBatch (50) is smaller than the track batch (200): tracks are finite and drain to completion, candidates are effectively unbounded and would otherwise starve the track arm forever. GC sweeps both tables — the similarity feed churns, and a candidate that joins the library has its tags in track_tags now. Tags swept before state so a mid-sweep crash leaves a valid state, not a re-fetch loop. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -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
|
||||
}
|
||||
@@ -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
|
||||
|
||||
@@ -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;
|
||||
@@ -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);
|
||||
@@ -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);
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
+212
-28
@@ -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
|
||||
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))
|
||||
|
||||
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)
|
||||
}
|
||||
}
|
||||
|
||||
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.
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
@@ -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 {
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
@@ -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() == "" {
|
||||
@@ -164,5 +208,6 @@ func normalizeLastfmTags(raw []lastfmTag) []Tag {
|
||||
// Compile-time capability checks.
|
||||
var (
|
||||
_ TrackTagProvider = (*lastfmProvider)(nil)
|
||||
_ ArtistTagProvider = (*lastfmProvider)(nil)
|
||||
_ TestableProvider = (*lastfmProvider)(nil)
|
||||
)
|
||||
|
||||
@@ -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
|
||||
@@ -154,5 +181,6 @@ func normalizeMBTags(raw []mbTag) []Tag {
|
||||
// Compile-time capability checks.
|
||||
var (
|
||||
_ TrackTagProvider = (*musicbrainzProvider)(nil)
|
||||
_ ArtistTagProvider = (*musicbrainzProvider)(nil)
|
||||
_ TestableProvider = (*musicbrainzProvider)(nil)
|
||||
)
|
||||
|
||||
@@ -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()
|
||||
|
||||
+27
-2
@@ -18,18 +18,28 @@ type Worker struct {
|
||||
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,
|
||||
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)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user