From bf409d5f3f2bac924f12f451c21491c121e93489 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Sat, 10 Oct 2026 17:44:06 -0400 Subject: [PATCH] fix: a second copy of a track counts once, joining words don't part two titles, and a newer matcher retries an older one's refusals (M502 #5569) The second run's refusals were mostly albums holding a track twice (a two-track single with three files), which no edition can pair, and release names that differ only in "+" against "/". Tracks are now read once per disc, number and title, and "and", "&" and "+" count for nothing when titles are compared; an edition's own title also counts as the album's name. library_album_matches records which matcher version decided each album. Refusals by an older version are matched again, even after the listing is done, so an improvement reaches them without a migration. Co-Authored-By: Claude Opus 5.5 --- internal/db/dbq/models.go | 1 + internal/db/dbq/wanted.sql.go | 45 +++++++++++++------ .../0088_library_matcher_version.down.sql | 1 + .../0088_library_matcher_version.up.sql | 4 ++ internal/db/queries/wanted.sql | 30 ++++++++----- internal/wanted/library.go | 28 +++++++++--- internal/wanted/library_test.go | 41 +++++++++++++++++ internal/wanted/match.go | 30 ++++++++++--- 8 files changed, 146 insertions(+), 34 deletions(-) create mode 100644 internal/db/migrations/0088_library_matcher_version.down.sql create mode 100644 internal/db/migrations/0088_library_matcher_version.up.sql diff --git a/internal/db/dbq/models.go b/internal/db/dbq/models.go index 883db4df..84b05f94 100644 --- a/internal/db/dbq/models.go +++ b/internal/db/dbq/models.go @@ -494,6 +494,7 @@ type LibraryAlbumMatch struct { ReleaseGroupMbid string Detail string DecidedAt pgtype.Timestamptz + Matcher int32 } type LibraryChange struct { diff --git a/internal/db/dbq/wanted.sql.go b/internal/db/dbq/wanted.sql.go index fbe40c14..4699d712 100644 --- a/internal/db/dbq/wanted.sql.go +++ b/internal/db/dbq/wanted.sql.go @@ -20,6 +20,19 @@ func (q *Queries) ClearDefaultReleaseTypeProfile(ctx context.Context) error { return err } +const countAlbumsToRematch = `-- name: CountAlbumsToRematch :one +SELECT COUNT(*) FROM library_album_matches + WHERE outcome = 'unmatched' AND matcher < $1::int +` + +// Refusals by an older matcher, which a finished listing goes back to. +func (q *Queries) CountAlbumsToRematch(ctx context.Context, matcher int32) (int64, error) { + row := q.db.QueryRow(ctx, countAlbumsToRematch, matcher) + var count int64 + err := row.Scan(&count) + return count, err +} + const countWants = `-- name: CountWants :one SELECT COUNT(*) FILTER (WHERE fulfilled_at IS NULL AND reason <> 'missing')::bigint AS wanted, COUNT(*) FILTER (WHERE fulfilled_at IS NULL AND reason = 'missing')::bigint AS missing, @@ -126,9 +139,9 @@ func (q *Queries) DeleteWant(ctx context.Context, id pgtype.UUID) (int64, error) const finishLibraryListing = `-- name: FinishLibraryListing :one UPDATE acquisition_settings - SET library_listed_at = now(), + SET library_listed_at = COALESCE(library_listed_at, now()), library_unlisted = (SELECT COUNT(*) FROM library_album_matches WHERE outcome = 'unmatched') - WHERE id = true AND library_listed_at IS NULL + WHERE id = true RETURNING library_listed_at, library_unlisted ` @@ -137,7 +150,8 @@ type FinishLibraryListingRow struct { LibraryUnlisted int32 } -// The listing is done: count the albums no match was found for. +// The listing is done, or done again after a newer matcher's retries: +// count the albums no match was found for. The first finish is kept. func (q *Queries) FinishLibraryListing(ctx context.Context) (FinishLibraryListingRow, error) { row := q.db.QueryRow(ctx, finishLibraryListing) var i FinishLibraryListingRow @@ -405,13 +419,15 @@ SELECT a.id, a.title, a.mbid, a.release_group_mbid, ar.name AS artist_name, ar.m JOIN artists ar ON ar.id = a.artist_id WHERE a.created_at <= $1::timestamptz AND EXISTS (SELECT 1 FROM tracks t WHERE t.album_id = a.id) - AND NOT EXISTS (SELECT 1 FROM library_album_matches m WHERE m.album_id = a.id) + AND NOT EXISTS (SELECT 1 FROM library_album_matches m + WHERE m.album_id = a.id AND (m.outcome = 'listed' OR m.matcher >= $2::int)) ORDER BY a.created_at, a.id - LIMIT $2 + LIMIT $3 ` type ListAlbumsToMatchParams struct { Cutoff pgtype.Timestamptz + Matcher int32 MaxRows int32 } @@ -424,11 +440,11 @@ type ListAlbumsToMatchRow struct { ArtistMbid *string } -// Library albums from before the cutoff that the listing has not decided: -// each is matched to MusicBrainz (#5569). An album row with no tracks is -// not an album in the library. +// Library albums from before the cutoff that the listing has not decided, +// or that an older matcher refused: each is matched to MusicBrainz (#5569). +// An album row with no tracks is not an album in the library. func (q *Queries) ListAlbumsToMatch(ctx context.Context, arg ListAlbumsToMatchParams) ([]ListAlbumsToMatchRow, error) { - rows, err := q.db.Query(ctx, listAlbumsToMatch, arg.Cutoff, arg.MaxRows) + rows, err := q.db.Query(ctx, listAlbumsToMatch, arg.Cutoff, arg.Matcher, arg.MaxRows) if err != nil { return nil, err } @@ -937,11 +953,12 @@ func (q *Queries) MarkGroupSeen(ctx context.Context, arg MarkGroupSeenParams) er } const recordAlbumMatch = `-- name: RecordAlbumMatch :exec -INSERT INTO library_album_matches (album_id, outcome, method, release_group_mbid, detail) -VALUES ($1, $2, $3, $4, $5) +INSERT INTO library_album_matches (album_id, outcome, method, release_group_mbid, detail, matcher) +VALUES ($1, $2, $3, $4, $5, + $6) ON CONFLICT (album_id) DO UPDATE - SET outcome = EXCLUDED.outcome, method = EXCLUDED.method, - release_group_mbid = EXCLUDED.release_group_mbid, detail = EXCLUDED.detail, decided_at = now() + SET outcome = EXCLUDED.outcome, method = EXCLUDED.method, release_group_mbid = EXCLUDED.release_group_mbid, + detail = EXCLUDED.detail, matcher = EXCLUDED.matcher, decided_at = now() ` type RecordAlbumMatchParams struct { @@ -950,6 +967,7 @@ type RecordAlbumMatchParams struct { Method string ReleaseGroupMbid string Detail string + Matcher int32 } func (q *Queries) RecordAlbumMatch(ctx context.Context, arg RecordAlbumMatchParams) error { @@ -959,6 +977,7 @@ func (q *Queries) RecordAlbumMatch(ctx context.Context, arg RecordAlbumMatchPara arg.Method, arg.ReleaseGroupMbid, arg.Detail, + arg.Matcher, ) return err } diff --git a/internal/db/migrations/0088_library_matcher_version.down.sql b/internal/db/migrations/0088_library_matcher_version.down.sql new file mode 100644 index 00000000..1e22094a --- /dev/null +++ b/internal/db/migrations/0088_library_matcher_version.down.sql @@ -0,0 +1 @@ +ALTER TABLE library_album_matches DROP COLUMN matcher; diff --git a/internal/db/migrations/0088_library_matcher_version.up.sql b/internal/db/migrations/0088_library_matcher_version.up.sql new file mode 100644 index 00000000..112d7d6b --- /dev/null +++ b/internal/db/migrations/0088_library_matcher_version.up.sql @@ -0,0 +1,4 @@ +-- Which version of the matching (#5569) decided an album. A refusal by an +-- older matcher is tried again by a newer one, so improving the matching +-- reaches the albums it refused without a migration each time. +ALTER TABLE library_album_matches ADD COLUMN matcher integer NOT NULL DEFAULT 0; diff --git a/internal/db/queries/wanted.sql b/internal/db/queries/wanted.sql index 2cb967b1..49077412 100644 --- a/internal/db/queries/wanted.sql +++ b/internal/db/queries/wanted.sql @@ -272,18 +272,24 @@ SELECT a.id, 'listed', 'tags', a.release_group_mbid ON CONFLICT (album_id) DO NOTHING; -- name: ListAlbumsToMatch :many --- Library albums from before the cutoff that the listing has not decided: --- each is matched to MusicBrainz (#5569). An album row with no tracks is --- not an album in the library. +-- Library albums from before the cutoff that the listing has not decided, +-- or that an older matcher refused: each is matched to MusicBrainz (#5569). +-- An album row with no tracks is not an album in the library. SELECT a.id, a.title, a.mbid, a.release_group_mbid, ar.name AS artist_name, ar.mbid AS artist_mbid FROM albums a JOIN artists ar ON ar.id = a.artist_id WHERE a.created_at <= sqlc.arg(cutoff)::timestamptz AND EXISTS (SELECT 1 FROM tracks t WHERE t.album_id = a.id) - AND NOT EXISTS (SELECT 1 FROM library_album_matches m WHERE m.album_id = a.id) + AND NOT EXISTS (SELECT 1 FROM library_album_matches m + WHERE m.album_id = a.id AND (m.outcome = 'listed' OR m.matcher >= sqlc.arg(matcher)::int)) ORDER BY a.created_at, a.id LIMIT sqlc.arg(max_rows); +-- name: CountAlbumsToRematch :one +-- Refusals by an older matcher, which a finished listing goes back to. +SELECT COUNT(*) FROM library_album_matches + WHERE outcome = 'unmatched' AND matcher < sqlc.arg(matcher)::int; + -- name: ListAlbumTracksToMatch :many -- What an album's tracks say about themselves, to check a match against. SELECT title, disc_number, track_number, duration_ms, mbid @@ -308,18 +314,20 @@ VALUES (sqlc.arg(release_group_mbid), sqlc.arg(artist_mbid), sqlc.arg(artist_nam ON CONFLICT (release_group_mbid) DO NOTHING; -- name: RecordAlbumMatch :exec -INSERT INTO library_album_matches (album_id, outcome, method, release_group_mbid, detail) -VALUES (sqlc.arg(album_id), sqlc.arg(outcome), sqlc.arg(method), sqlc.arg(release_group_mbid), sqlc.arg(detail)) +INSERT INTO library_album_matches (album_id, outcome, method, release_group_mbid, detail, matcher) +VALUES (sqlc.arg(album_id), sqlc.arg(outcome), sqlc.arg(method), sqlc.arg(release_group_mbid), sqlc.arg(detail), + sqlc.arg(matcher)) ON CONFLICT (album_id) DO UPDATE - SET outcome = EXCLUDED.outcome, method = EXCLUDED.method, - release_group_mbid = EXCLUDED.release_group_mbid, detail = EXCLUDED.detail, decided_at = now(); + SET outcome = EXCLUDED.outcome, method = EXCLUDED.method, release_group_mbid = EXCLUDED.release_group_mbid, + detail = EXCLUDED.detail, matcher = EXCLUDED.matcher, decided_at = now(); -- name: FinishLibraryListing :one --- The listing is done: count the albums no match was found for. +-- The listing is done, or done again after a newer matcher's retries: +-- count the albums no match was found for. The first finish is kept. UPDATE acquisition_settings - SET library_listed_at = now(), + SET library_listed_at = COALESCE(library_listed_at, now()), library_unlisted = (SELECT COUNT(*) FROM library_album_matches WHERE outcome = 'unmatched') - WHERE id = true AND library_listed_at IS NULL + WHERE id = true RETURNING library_listed_at, library_unlisted; -- name: ListUnmatchedLibraryAlbums :many diff --git a/internal/wanted/library.go b/internal/wanted/library.go index 8642d846..ea56759c 100644 --- a/internal/wanted/library.go +++ b/internal/wanted/library.go @@ -23,6 +23,15 @@ const libraryMatchBudget = 2 * time.Minute // libraryMatchBatch is how many albums one query hands the matching. const libraryMatchBatch = 25 +// libraryMatcher is the matching's version. Raise it when the matching +// changes in a way that could match an album it refused: albums an older +// version refused are matched again, even after the listing is done. +// +// 1 tags, release, discography and search, checked by title and length +// 2 titles alone under the album's own name; a refusal names the track +// 3 a second copy of a track counts once; joining words count for nothing +const libraryMatcher = 3 + // listLibrary puts the library's albums on the wanted list as held, once // (operator, 2026-10-10: "make a record saying that the import has // happened so later changes like unwanting hold and dont auto import from @@ -39,7 +48,8 @@ const libraryMatchBatch = 25 // of the albums no match was found for. // // Each album decided is recorded in library_album_matches, so an album taken -// off the list stays off, and once done the listing never runs again. +// off the list stays off. Once done the listing runs again only for albums +// an older matcher refused (libraryMatcher). func (s *Service) listLibrary(ctx context.Context, res *PassResult) error { q := dbq.New(s.pool) state, err := q.GetLibraryListing(ctx) @@ -47,7 +57,13 @@ func (s *Service) listLibrary(ctx context.Context, res *PassResult) error { return fmt.Errorf("wanted: read the library listing: %w", err) } if state.LibraryListedAt.Valid { - return nil + stale, err := q.CountAlbumsToRematch(ctx, libraryMatcher) + if err != nil { + return fmt.Errorf("wanted: count refusals to retry: %w", err) + } + if stale == 0 { + return nil + } } cutoff := state.LibraryListingStartedAt if !cutoff.Valid { @@ -67,7 +83,8 @@ func (s *Service) listLibrary(ctx context.Context, res *PassResult) error { deadline := s.now().Add(libraryMatchBudget) for { - albums, err := q.ListAlbumsToMatch(ctx, dbq.ListAlbumsToMatchParams{Cutoff: cutoff, MaxRows: libraryMatchBatch}) + albums, err := q.ListAlbumsToMatch(ctx, dbq.ListAlbumsToMatchParams{ + Cutoff: cutoff, Matcher: libraryMatcher, MaxRows: libraryMatchBatch}) if err != nil { return fmt.Errorf("wanted: albums to match: %w", err) } @@ -107,7 +124,8 @@ func (s *Service) listAlbum(ctx context.Context, q *dbq.Queries, a dbq.ListAlbum } if !m.OK { s.logger.Info("wanted: no MusicBrainz match for a library album", "album", a.Title, "artist", a.ArtistName, "why", m.Detail) - return 0, q.RecordAlbumMatch(ctx, dbq.RecordAlbumMatchParams{AlbumID: a.ID, Outcome: "unmatched", Detail: m.Detail}) + return 0, q.RecordAlbumMatch(ctx, dbq.RecordAlbumMatchParams{ + AlbumID: a.ID, Outcome: "unmatched", Detail: m.Detail, Matcher: libraryMatcher}) } var listed int64 err = pgx.BeginFunc(ctx, s.pool, func(tx pgx.Tx) error { @@ -124,7 +142,7 @@ func (s *Service) listAlbum(ctx context.Context, q *dbq.Queries, a dbq.ListAlbum } listed = n return tq.RecordAlbumMatch(ctx, dbq.RecordAlbumMatchParams{ - AlbumID: a.ID, Outcome: "listed", Method: m.Method, ReleaseGroupMbid: m.Group, + AlbumID: a.ID, Outcome: "listed", Method: m.Method, ReleaseGroupMbid: m.Group, Matcher: libraryMatcher, }) }) if err != nil { diff --git a/internal/wanted/library_test.go b/internal/wanted/library_test.go index 76410b7e..029252ab 100644 --- a/internal/wanted/library_test.go +++ b/internal/wanted/library_test.go @@ -268,7 +268,48 @@ func TestTrackAgreement(t *testing.T) { if _, miss := trackAgreement([]libraryTrack{{Title: "Other", LengthMs: 61000}}, edition, true); miss != `no track of its edition is like "Other" (1:01)` { t.Errorf("unplaced: %q", miss) } + if !sameTitle("Brain Stew + Jaded", "Brain Stew / Jaded") || sameTitle("+++", "Grind & Hustle") || sameTitle("Blur", "The Best of Blur") { + t.Error("sameTitle: joining words count for nothing, and a title of none is no title") + } + n := int32(1) + copies := tracksToMatch([]dbq.ListAlbumTracksToMatchRow{ + {Title: "Song", TrackNumber: &n, DurationMs: 1}, {Title: "song", TrackNumber: &n, DurationMs: 2}, {Title: "Other", TrackNumber: &n}}) + if len(copies) != 2 { + t.Errorf("a second copy of a track is read once: %+v", copies) + } if agrees(3, 4) || !agrees(4, 5) || agrees(0, 0) { t.Error("agrees: 80% of the album's tracks, and an album has tracks") } } + +// A refusal by an older matcher is matched again once the matching improves, +// even after the listing is done; a refusal by this one is not. +func TestListLibrary_RetriesAnOlderMatchersRefusals_Integration(t *testing.T) { + pool := newPool(t) + ctx := context.Background() + q := dbq.New(pool) + s := newService(t, pool, newFakeMB(t)) + ballad := seedAlbum(t, q, blur, "The Ballad of Darren", "", []string{"The Ballad", "St. Charles Square"}) + bootleg := seedAlbum(t, q, blur, "Bootleg", "", []string{"Intro"}) + for _, a := range []pgtype.UUID{ballad.ID, bootleg.ID} { + if err := q.RecordAlbumMatch(ctx, dbq.RecordAlbumMatchParams{AlbumID: a, Outcome: "unmatched", Detail: "old", Matcher: 1}); err != nil { + t.Fatal(err) + } + } + if _, err := pool.Exec(ctx, `UPDATE library_album_matches SET matcher = $1 WHERE album_id = $2`, libraryMatcher, bootleg.ID); err != nil { + t.Fatal(err) + } + // The reset leaves the listing done. + if err := s.listLibrary(ctx, &PassResult{}); err != nil { + t.Fatal(err) + } + if outcome, method, _, _ := matchOf(t, pool, ballad.ID); outcome != "listed" || method != matchDiscography { + t.Errorf("the older refusal was decided %s by %s", outcome, method) + } + if _, _, _, detail := matchOf(t, pool, bootleg.ID); detail != "old" { + t.Errorf("this matcher's own refusal was retried: %q", detail) + } + if state, _ := q.GetLibraryListing(ctx); state.LibraryUnlisted != 1 { + t.Errorf("unlisted = %d, want the bootleg alone", state.LibraryUnlisted) + } +} diff --git a/internal/wanted/match.go b/internal/wanted/match.go index 04c7527e..9d9d8c99 100644 --- a/internal/wanted/match.go +++ b/internal/wanted/match.go @@ -85,8 +85,13 @@ type albumMatch struct { Detail string } +// tracksToMatch reads an album's tracks, each once: a second copy of a +// track (the same disc, number and title, as a duplicate file in another +// format is) would otherwise count against the match as a track no edition +// has. func tracksToMatch(rows []dbq.ListAlbumTracksToMatchRow) []libraryTrack { out := make([]libraryTrack, 0, len(rows)) + seen := make(map[string]bool, len(rows)) for _, r := range rows { t := libraryTrack{Title: r.Title, LengthMs: int(r.DurationMs), Recording: deref(r.Mbid)} if r.DiscNumber != nil { @@ -95,6 +100,11 @@ func tracksToMatch(rows []dbq.ListAlbumTracksToMatchRow) []libraryTrack { if r.TrackNumber != nil { t.Number = int(*r.TrackNumber) } + key := fmt.Sprintf("%d/%d/%s", max(t.Disc, 1), t.Number, strings.Join(release.Tokens(t.Title), " ")) + if seen[key] { + continue + } + seen[key] = true out = append(out, t) } return out @@ -165,9 +175,15 @@ func agrees(n, tracks int) bool { return tracks > 0 && n*10 >= tracks*matchAgreement } -// sameTitle: the same words, not merely one holding the other. +// sameTitle: the same words, not merely one holding the other. "And", +// "&" and "+" join two titles however a release spells it ("Brain Stew + +// Jaded", "Brain Stew / Jaded"), so they count for nothing. func sameTitle(a, b string) bool { - return slices.Equal(release.Tokens(a), release.Tokens(b)) + words := func(s string) []string { + return slices.DeleteFunc(release.Tokens(s), func(w string) bool { return w == "and" }) + } + wa, wb := words(a), words(b) + return len(wa) > 0 && slices.Equal(wa, wb) } // matchAlbum finds the release group a library album is. A MusicBrainz @@ -204,7 +220,9 @@ func (s *Service) matchAlbum(ctx context.Context, a dbq.ListAlbumsToMatchRow, tr continue } tried[c.Group] = true - n, size, miss, err := s.agreementWith(ctx, c.Group, tracks, sameTitle(c.Title, a.Title)) + n, size, miss, err := s.agreementWith(ctx, c.Group, tracks, func(edition string) bool { + return sameTitle(c.Title, a.Title) || sameTitle(edition, a.Title) + }) if err != nil { return err } @@ -328,7 +346,9 @@ func (s *Service) searchCandidates(ctx context.Context, a dbq.ListAlbumsToMatchR // agreementWith is how many of the album's tracks the group's best edition // agrees with, that edition's length, and how it differs. A group // MusicBrainz no longer has, or lists no edition of, agrees with none (-1). -func (s *Service) agreementWith(ctx context.Context, group string, tracks []libraryTrack, titlesEnough bool) (n, size int, miss string, err error) { +// ownName says whether an edition, by its title, is called what the album +// is, so that its track titles are enough. +func (s *Service) agreementWith(ctx context.Context, group string, tracks []libraryTrack, ownName func(edition string) bool) (n, size int, miss string, err error) { eds, err := s.cat.Releases(ctx, group) switch { case errors.Is(err, catalogue.ErrNotFound): @@ -343,7 +363,7 @@ func (s *Service) agreementWith(ctx context.Context, group string, tracks []libr edition = append(edition, editionTrack{Medium: int(t.MediumPosition), Position: int(t.Position), Title: t.Title, LengthMs: int(t.LengthMs), Recording: t.RecordingMbid}) } - if got, m := trackAgreement(tracks, edition, titlesEnough); got > n || (got == n && abs(len(edition)-len(tracks)) < abs(size-len(tracks))) { + if got, m := trackAgreement(tracks, edition, ownName(e.Release.Title)); got > n || (got == n && abs(len(edition)-len(tracks)) < abs(size-len(tracks))) { n, size, miss = got, len(edition), m } }