From e2432caa6569fe5dfc3db6e221b1c3a87284deb0 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Sat, 16 May 2026 13:49:38 -0400 Subject: [PATCH] =?UTF-8?q?fix(library):=20extract=20recording=20MBID=20?= =?UTF-8?q?=E2=86=92=20tracks.mbid;=20unblock=20LB=20similarity?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit tracks.mbid was 100% NULL: the scanner only extracted album + artist MBIDs, never the recording MBID. The ListenBrainz similarity worker is gated on tracks.mbid IS NOT NULL, so track_similarity could never populate — starving For-You/radio's strongest candidate source (lb_similar) on every library. - mbids.go: add extractRecordingMBID (mbz.Recording / Picard musicbrainz_recordingid). Separate fn so extractMBIDs' signature + unit tests stay untouched. - scanner.go: persist recording MBID via UpsertTrack (heals on the file_path conflict, so re-scans backfill for free). - BackfillTrackMBIDs: one-shot pass mirroring BackfillMBIDs, wired as scan Stage 2b (idempotent via SetTrackMbidIfNull, gated by BackfillCap, log-only progress). - migration 0029: tracks_mbid_unique (0002, written when the column was always NULL) wrongly assumes one MBID == one track. A recording appears on multiple releases, so rows legitimately share a recording MBID. Replace with a non-unique partial index. Zero-risk: column is 100% NULL at migration time. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/db/dbq/tracks.sql.go | 53 +++++++++++ .../0029_tracks_mbid_nonunique.down.sql | 6 ++ .../0029_tracks_mbid_nonunique.up.sql | 15 +++ internal/db/queries/tracks.sql | 16 ++++ internal/library/mbidbackfill.go | 93 +++++++++++++++++++ internal/library/mbids.go | 13 +++ internal/library/scanner.go | 8 ++ internal/library/scanrun.go | 10 ++ 8 files changed, 214 insertions(+) create mode 100644 internal/db/migrations/0029_tracks_mbid_nonunique.down.sql create mode 100644 internal/db/migrations/0029_tracks_mbid_nonunique.up.sql diff --git a/internal/db/dbq/tracks.sql.go b/internal/db/dbq/tracks.sql.go index 7e21a3f3..ffe498e3 100644 --- a/internal/db/dbq/tracks.sql.go +++ b/internal/db/dbq/tracks.sql.go @@ -376,6 +376,41 @@ func (q *Queries) ListTracksByAlbum(ctx context.Context, arg ListTracksByAlbumPa return items, nil } +const listTracksMissingMbidWithPath = `-- name: ListTracksMissingMbidWithPath :many +SELECT id, file_path + FROM tracks + WHERE mbid IS NULL + ORDER BY id + LIMIT $1 +` + +type ListTracksMissingMbidWithPathRow struct { + ID pgtype.UUID + FilePath string +} + +// Track recording-MBID backfill: tracks with NULL mbid that still have +// a file to re-read. $1 caps the batch (mirrors the album backfill). +func (q *Queries) ListTracksMissingMbidWithPath(ctx context.Context, limit int32) ([]ListTracksMissingMbidWithPathRow, error) { + rows, err := q.db.Query(ctx, listTracksMissingMbidWithPath, limit) + if err != nil { + return nil, err + } + defer rows.Close() + var items []ListTracksMissingMbidWithPathRow + for rows.Next() { + var i ListTracksMissingMbidWithPathRow + if err := rows.Scan(&i.ID, &i.FilePath); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + const searchTracks = `-- name: SearchTracks :many SELECT id, title, album_id, artist_id, track_number, disc_number, duration_ms, file_path, file_size, file_format, bitrate, mbid, genre, added_at, updated_at FROM tracks WHERE title ILIKE '%' || $1::text || '%' @@ -437,6 +472,24 @@ func (q *Queries) SearchTracks(ctx context.Context, arg SearchTracksParams) ([]T return items, nil } +const setTrackMbidIfNull = `-- name: SetTrackMbidIfNull :exec +UPDATE tracks + SET mbid = $2, updated_at = now() + WHERE id = $1 AND mbid IS NULL +` + +type SetTrackMbidIfNullParams struct { + ID pgtype.UUID + Mbid *string +} + +// Heal a track's recording MBID only while still NULL — idempotent, so +// re-running the backfill is a no-op for already-healed rows. +func (q *Queries) SetTrackMbidIfNull(ctx context.Context, arg SetTrackMbidIfNullParams) error { + _, err := q.db.Exec(ctx, setTrackMbidIfNull, arg.ID, arg.Mbid) + return err +} + const upsertTrack = `-- name: UpsertTrack :one INSERT INTO tracks ( title, album_id, artist_id, track_number, disc_number, diff --git a/internal/db/migrations/0029_tracks_mbid_nonunique.down.sql b/internal/db/migrations/0029_tracks_mbid_nonunique.down.sql new file mode 100644 index 00000000..b40e0408 --- /dev/null +++ b/internal/db/migrations/0029_tracks_mbid_nonunique.down.sql @@ -0,0 +1,6 @@ +-- Reverts to the unique partial index. This FAILS if any recording +-- MBID is shared across releases in the library (the exact case 0029 +-- exists to allow) — dedupe tracks.mbid before rolling back. + +DROP INDEX IF EXISTS tracks_mbid_idx; +CREATE UNIQUE INDEX tracks_mbid_unique ON tracks (mbid) WHERE mbid IS NOT NULL; diff --git a/internal/db/migrations/0029_tracks_mbid_nonunique.up.sql b/internal/db/migrations/0029_tracks_mbid_nonunique.up.sql new file mode 100644 index 00000000..714789cb --- /dev/null +++ b/internal/db/migrations/0029_tracks_mbid_nonunique.up.sql @@ -0,0 +1,15 @@ +-- tracks.mbid holds the MusicBrainz *recording* MBID — the id the +-- ListenBrainz similarity pipeline (ListPlayedTracksNeedingSimilarity, +-- GetTracksByMBIDs) matches on. A single recording legitimately appears +-- on multiple releases (album + compilation + single), so multiple +-- tracks rows share one recording MBID. +-- +-- tracks_mbid_unique (added in 0002, when this column was always NULL +-- and never populated) wrongly assumes one MBID == one track. Now that +-- the scanner extracts the recording MBID, that uniqueness throws +-- 23505 for any recording present on >1 release. Replace it with a +-- non-unique partial index serving the same lookups. Zero-risk: the +-- column is 100% NULL at migration time. + +DROP INDEX IF EXISTS tracks_mbid_unique; +CREATE INDEX IF NOT EXISTS tracks_mbid_idx ON tracks (mbid) WHERE mbid IS NOT NULL; diff --git a/internal/db/queries/tracks.sql b/internal/db/queries/tracks.sql index 4019b684..1790ddf4 100644 --- a/internal/db/queries/tracks.sql +++ b/internal/db/queries/tracks.sql @@ -19,6 +19,22 @@ ON CONFLICT (file_path) DO UPDATE SET updated_at = now() RETURNING *; +-- name: ListTracksMissingMbidWithPath :many +-- Track recording-MBID backfill: tracks with NULL mbid that still have +-- a file to re-read. $1 caps the batch (mirrors the album backfill). +SELECT id, file_path + FROM tracks + WHERE mbid IS NULL + ORDER BY id + LIMIT $1; + +-- name: SetTrackMbidIfNull :exec +-- Heal a track's recording MBID only while still NULL — idempotent, so +-- re-running the backfill is a no-op for already-healed rows. +UPDATE tracks + SET mbid = $2, updated_at = now() + WHERE id = $1 AND mbid IS NULL; + -- name: GetTrackByID :one SELECT * FROM tracks WHERE id = $1; diff --git a/internal/library/mbidbackfill.go b/internal/library/mbidbackfill.go index b312c1f5..5d79ff96 100644 --- a/internal/library/mbidbackfill.go +++ b/internal/library/mbidbackfill.go @@ -167,3 +167,96 @@ func readMBIDsForFile(path string, logger *slog.Logger) (albumMBID, artistMBID s } return extractMBIDs(meta) } + +// BackfillTrackMBIDsResult tallies the track recording-MBID backfill. +type BackfillTrackMBIDsResult struct { + Processed int // Tracks considered (mbid IS NULL). + Healed int // Tracks whose recording MBID we set. + Skipped int // Tracks whose file was missing / untagged / write failed. +} + +// BackfillTrackMBIDs walks tracks with NULL mbid, re-reads each file's +// tags, and persists the MusicBrainz recording MBID. This is the +// counterpart to BackfillMBIDs (which heals album/artist MBIDs); it +// unblocks the ListenBrainz similarity pipeline, which is gated on +// tracks.mbid IS NOT NULL. +// +// Idempotent via SetTrackMbidIfNull: re-running costs the same N file +// reads but is a no-op for already-healed rows. limit caps one call; +// pass -1 for unbounded (caller normally batches per scan). +func BackfillTrackMBIDs(ctx context.Context, pool *pgxpool.Pool, logger *slog.Logger, + limit int, progressCb func(BackfillTrackMBIDsResult)) (BackfillTrackMBIDsResult, error) { + q := dbq.New(pool) + queryLimit := int32(limit) + if limit < 0 { + queryLimit = 1<<31 - 1 // effectively unbounded + } + + rows, err := q.ListTracksMissingMbidWithPath(ctx, queryLimit) + if err != nil { + return BackfillTrackMBIDsResult{}, fmt.Errorf("list tracks missing mbid: %w", err) + } + + var res BackfillTrackMBIDsResult + for i, r := range rows { + if ctx.Err() != nil { + return res, ctx.Err() + } + res.Processed++ + + rec := readRecordingMBIDForFile(r.FilePath, logger) + if rec == "" { + res.Skipped++ + if progressCb != nil { + progressCb(res) + } + continue + } + + m := rec + if uerr := q.SetTrackMbidIfNull(ctx, dbq.SetTrackMbidIfNullParams{ + ID: r.ID, Mbid: &m, + }); uerr != nil { + logger.Warn("track mbid backfill: set failed", + "track_id", r.ID, "err", uerr) + res.Skipped++ + if progressCb != nil { + progressCb(res) + } + continue + } + + res.Healed++ + if (i+1)%500 == 0 { + logger.Info("track mbid backfill progress", + "processed", res.Processed, "healed", res.Healed, "skipped", res.Skipped) + } + if progressCb != nil { + progressCb(res) + } + } + + logger.Info("track mbid backfill complete", + "processed", res.Processed, "healed", res.Healed, "skipped", res.Skipped) + return res, nil +} + +// readRecordingMBIDForFile opens one audio file and returns its +// MusicBrainz recording MBID, or "" when the file can't be opened, the +// tag read fails, or the tag is absent. Errors are logged at Warn, not +// propagated — one broken file must not halt the backfill. +func readRecordingMBIDForFile(path string, logger *slog.Logger) string { + f, err := os.Open(path) + if err != nil { + logger.Warn("track mbid backfill: open failed", "path", path, "err", err) + return "" + } + defer func() { _ = f.Close() }() + + meta, err := tag.ReadFrom(f) + if err != nil { + logger.Warn("track mbid backfill: tag read failed", "path", path, "err", err) + return "" + } + return extractRecordingMBID(meta) +} diff --git a/internal/library/mbids.go b/internal/library/mbids.go index 608ed0a0..55592be4 100644 --- a/internal/library/mbids.go +++ b/internal/library/mbids.go @@ -27,6 +27,19 @@ func extractMBIDs(m tag.Metadata) (albumMBID, artistMBID string) { return cleanMBID(info.Get(mbz.Album)), cleanMBID(info.Get(mbz.Artist)) } +// extractRecordingMBID reads the MusicBrainz *recording* ID — Picard's +// musicbrainz_recordingid, surfaced by dhowden/tag as mbz.Recording +// (tag display name "MusicBrainz Track Id"). This is the id ListenBrainz +// /explore/similar-recordings keys on and what tracks.mbid stores. +// +// Deliberately NOT mbz.Track ("MusicBrainz Release Track Id"): that is +// per-release-track, whereas similarity is per-recording. Kept separate +// from extractMBIDs so its existing (album, artist) signature and unit +// tests stay untouched. +func extractRecordingMBID(m tag.Metadata) string { + return cleanMBID(mbz.Extract(m).Get(mbz.Recording)) +} + func cleanMBID(s string) string { if s == "" { return "" diff --git a/internal/library/scanner.go b/internal/library/scanner.go index be3476ad..5f47912a 100644 --- a/internal/library/scanner.go +++ b/internal/library/scanner.go @@ -142,6 +142,7 @@ func (s *Scanner) scanFile(ctx context.Context, q *dbq.Queries, path string, sta return fmt.Errorf("tag read: %w", err) } albumMBID, artistMBID := extractMBIDs(meta) + recordingMBID := extractRecordingMBID(meta) artistName := meta.Artist() if artistName == "" { @@ -195,6 +196,13 @@ func (s *Scanner) scanFile(ctx context.Context, q *dbq.Queries, path string, sta if g := meta.Genre(); g != "" { params.Genre = &g } + // Recording MBID feeds the ListenBrainz similarity pipeline. + // UpsertTrack heals mbid on the file_path conflict, so a re-scan + // of a previously-untagged-into-DB track backfills it for free. + if recordingMBID != "" { + m := recordingMBID + params.Mbid = &m + } track, err := q.UpsertTrack(ctx, params) if err != nil { diff --git a/internal/library/scanrun.go b/internal/library/scanrun.go index 4b8a5db3..e53612ba 100644 --- a/internal/library/scanrun.go +++ b/internal/library/scanrun.go @@ -154,6 +154,16 @@ func RunScan( } } + // Stage 2b: track recording-MBID backfill. Unblocks the ListenBrainz + // similarity pipeline (gated on tracks.mbid IS NOT NULL). Log-only + // progress — no scan_runs jsonb column to avoid a schema addition; + // it's idempotent and reruns each scan until every file is healed. + if cfg.BackfillCap != 0 { + if _, tberr := BackfillTrackMBIDs(ctx, pool, logger, cfg.BackfillCap, nil); tberr != nil { + captureErr("track_mbid_backfill", tberr) + } + } + // Stage 3: cover enrichment. if enricher != nil && cfg.EnrichCap != 0 { var lastP, lastS, lastF int