From e45561e282d97f0ded5064a20723c287e8c841bf Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Tue, 20 Oct 2026 14:13:58 -0400 Subject: [PATCH] fix: same-titled albums of two release groups are two albums, and a missing track with no disc is on disc 1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The scanner found an album by artist and title alone, so Weezer's six self-titled albums (1994–2026) were one 118-track row and their merges, wants and upgrades counted them as one. A same-named row whose release group is known and differs from the file's is now another album; tagReadVersion 7 re-reads the library so existing tracks move to their own rows. WantHoldings compared discs with IS NOT DISTINCT FROM, so a missing track 6 with no disc was never covered by a present disc 1 track 6. Air's Love 2 stayed wanted and was downloaded again for nothing. Co-Authored-By: Claude Opus 5.5 --- internal/db/dbq/albums.sql.go | 20 +++++-- internal/db/dbq/wanted.sql.go | 6 ++- internal/db/queries/albums.sql | 13 ++++- internal/db/queries/wanted.sql | 6 ++- internal/library/scanner.go | 13 ++++- internal/library/scanner_test.go | 91 ++++++++++++++++++++++++++++++++ internal/wanted/upgrade_test.go | 37 +++++++++++++ 7 files changed, 173 insertions(+), 13 deletions(-) diff --git a/internal/db/dbq/albums.sql.go b/internal/db/dbq/albums.sql.go index baaf1f56..5f21694d 100644 --- a/internal/db/dbq/albums.sql.go +++ b/internal/db/dbq/albums.sql.go @@ -72,17 +72,27 @@ func (q *Queries) DeleteAlbumIfEmpty(ctx context.Context, id pgtype.UUID) (Delet } const getAlbumByArtistAndTitle = `-- name: GetAlbumByArtistAndTitle :one -SELECT id, title, sort_title, artist_id, release_date, mbid, cover_art_path, created_at, updated_at, cover_art_source, cover_art_sources_version, release_group_mbid FROM albums WHERE artist_id = $1 AND title = $2 LIMIT 1 +SELECT id, title, sort_title, artist_id, release_date, mbid, cover_art_path, created_at, updated_at, cover_art_source, cover_art_sources_version, release_group_mbid FROM albums + WHERE artist_id = $1 AND title = $2 + AND ($3::text IS NULL OR release_group_mbid IS NULL + OR release_group_mbid = $3) + ORDER BY (release_group_mbid IS NOT DISTINCT FROM $3) DESC, id + LIMIT 1 ` type GetAlbumByArtistAndTitleParams struct { - ArtistID pgtype.UUID - Title string + ArtistID pgtype.UUID + Title string + ReleaseGroupMbid *string } -// Scanner uses this for the no-mbid dedupe path: resolve-or-create. +// Scanner uses this for the no-mbid dedupe path: resolve-or-create. An album +// of the same name in another release group is a different album (Weezer's +// six self-titled albums shared one row until 2026-10-20), so a row whose +// group is known and differs is not returned. A row of the file's own group +// comes first, then one with no group yet. func (q *Queries) GetAlbumByArtistAndTitle(ctx context.Context, arg GetAlbumByArtistAndTitleParams) (Album, error) { - row := q.db.QueryRow(ctx, getAlbumByArtistAndTitle, arg.ArtistID, arg.Title) + row := q.db.QueryRow(ctx, getAlbumByArtistAndTitle, arg.ArtistID, arg.Title, arg.ReleaseGroupMbid) var i Album err := row.Scan( &i.ID, diff --git a/internal/db/dbq/wanted.sql.go b/internal/db/dbq/wanted.sql.go index c1390d4c..2a7ca770 100644 --- a/internal/db/dbq/wanted.sql.go +++ b/internal/db/dbq/wanted.sql.go @@ -1572,11 +1572,13 @@ SELECT a.id AS album_id, COALESCE(bool_or(t.missing_since IS NOT NULL AND t.missing_since < $1::timestamptz), false)::bool AS missing_before, -- Missing tracks no track on disk stands in for, by position or -- title. A missing row whose song is back at a new path (#5245) is - -- covered; a lost bonus track is not. + -- covered; a lost bonus track is not. A track with no disc number is + -- on disc 1: Air's Love 2 stayed wanted, and was downloaded again for + -- nothing, over a missing track 6 with no disc beside a present 1/6. COUNT(*) FILTER (WHERE t.missing_since IS NOT NULL AND NOT EXISTS ( SELECT 1 FROM tracks p WHERE p.album_id = a.id AND p.missing_since IS NULL - AND ((p.track_number = t.track_number AND p.disc_number IS NOT DISTINCT FROM t.disc_number) + AND ((p.track_number = t.track_number AND COALESCE(p.disc_number, 1) = COALESCE(t.disc_number, 1)) OR lower(p.title) = lower(t.title))))::int AS uncovered_missing FROM albums a JOIN tracks t ON t.album_id = a.id diff --git a/internal/db/queries/albums.sql b/internal/db/queries/albums.sql index 4ce5452f..1a298bc0 100644 --- a/internal/db/queries/albums.sql +++ b/internal/db/queries/albums.sql @@ -36,8 +36,17 @@ WHERE albums.artist_id = $1 ORDER BY release_date NULLS LAST, sort_title; -- name: GetAlbumByArtistAndTitle :one --- Scanner uses this for the no-mbid dedupe path: resolve-or-create. -SELECT * FROM albums WHERE artist_id = $1 AND title = $2 LIMIT 1; +-- Scanner uses this for the no-mbid dedupe path: resolve-or-create. An album +-- of the same name in another release group is a different album (Weezer's +-- six self-titled albums shared one row until 2026-10-20), so a row whose +-- group is known and differs is not returned. A row of the file's own group +-- comes first, then one with no group yet. +SELECT * FROM albums + WHERE artist_id = sqlc.arg(artist_id) AND title = sqlc.arg(title) + AND (sqlc.narg(release_group_mbid)::text IS NULL OR release_group_mbid IS NULL + OR release_group_mbid = sqlc.narg(release_group_mbid)) + ORDER BY (release_group_mbid IS NOT DISTINCT FROM sqlc.narg(release_group_mbid)) DESC, id + LIMIT 1; -- name: ListAlbumsByArtist :many SELECT * FROM albums WHERE artist_id = $1 ORDER BY release_date NULLS LAST, sort_title; diff --git a/internal/db/queries/wanted.sql b/internal/db/queries/wanted.sql index ba2b1edc..16d47116 100644 --- a/internal/db/queries/wanted.sql +++ b/internal/db/queries/wanted.sql @@ -146,11 +146,13 @@ SELECT a.id AS album_id, COALESCE(bool_or(t.missing_since IS NOT NULL AND t.missing_since < sqlc.arg(since)::timestamptz), false)::bool AS missing_before, -- Missing tracks no track on disk stands in for, by position or -- title. A missing row whose song is back at a new path (#5245) is - -- covered; a lost bonus track is not. + -- covered; a lost bonus track is not. A track with no disc number is + -- on disc 1: Air's Love 2 stayed wanted, and was downloaded again for + -- nothing, over a missing track 6 with no disc beside a present 1/6. COUNT(*) FILTER (WHERE t.missing_since IS NOT NULL AND NOT EXISTS ( SELECT 1 FROM tracks p WHERE p.album_id = a.id AND p.missing_since IS NULL - AND ((p.track_number = t.track_number AND p.disc_number IS NOT DISTINCT FROM t.disc_number) + AND ((p.track_number = t.track_number AND COALESCE(p.disc_number, 1) = COALESCE(t.disc_number, 1)) OR lower(p.title) = lower(t.title))))::int AS uncovered_missing FROM albums a JOIN tracks t ON t.album_id = a.id diff --git a/internal/library/scanner.go b/internal/library/scanner.go index e61f2abb..40cbb51c 100644 --- a/internal/library/scanner.go +++ b/internal/library/scanner.go @@ -74,7 +74,11 @@ var audioExtensions = map[string]bool{ // dhowden/tag reads it as 0, and Minstrel's own importer wrote it that // way, so all 245 FLAC tracks it imported came in with no track or disc // number and their albums lost their order. -const tagReadVersion int16 = 6 +// 7: an album of the same name in another release group gets its own row +// (resolveAlbum). Weezer's six self-titled albums were one 118-track row, +// so their merges, wants and upgrades all counted as one album; a re-read +// files each existing track under its own album. +const tagReadVersion int16 = 7 type Stats struct { Scanned int `json:"scanned"` @@ -562,7 +566,12 @@ func (s *Scanner) resolveArtist(ctx context.Context, q *dbq.Queries, name, mbid } func (s *Scanner) resolveAlbum(ctx context.Context, q *dbq.Queries, artistID pgtype.UUID, title string, year int, mbid, releaseGroupMBID string) (dbq.Album, error) { - existing, err := q.GetAlbumByArtistAndTitle(ctx, dbq.GetAlbumByArtistAndTitleParams{ArtistID: artistID, Title: title}) + lookup := dbq.GetAlbumByArtistAndTitleParams{ArtistID: artistID, Title: title} + if releaseGroupMBID != "" { + rg := releaseGroupMBID + lookup.ReleaseGroupMbid = &rg + } + existing, err := q.GetAlbumByArtistAndTitle(ctx, lookup) if err == nil { // Heal the release group the same way. Not unique, so no conflict to // handle: every release of a group may carry it. diff --git a/internal/library/scanner_test.go b/internal/library/scanner_test.go index b5404c63..66554044 100644 --- a/internal/library/scanner_test.go +++ b/internal/library/scanner_test.go @@ -385,3 +385,94 @@ func TestScanner_ReleaseGroupMBID_Integration(t *testing.T) { t.Errorf("album A release group changed to %v", rg) } } + +// Two albums of one name by one artist, in different release groups, are two +// albums: Weezer's six self-titled albums shared one row until 2026-10-20. A +// track of the same group joins its album, and a track filed under the other +// album's row by the old lookup moves to its own on the tagReadVersion re-read. +func TestScanner_SameTitledAlbumsOfTwoGroupsAreTwoAlbums_Integration(t *testing.T) { + if testing.Short() { + t.Skip("skipping scanner integration in -short mode") + } + dsn := os.Getenv("MINSTREL_TEST_DATABASE_URL") + if dsn == "" { + t.Skip("MINSTREL_TEST_DATABASE_URL not set") + } + ctx := context.Background() + logger := slog.New(slog.NewTextHandler(io.Discard, nil)) + if err := db.Migrate(dsn, logger); err != nil { + t.Fatalf("migrate: %v", err) + } + pool, err := pgxpool.New(ctx, dsn) + if err != nil { + t.Fatalf("pool: %v", err) + } + t.Cleanup(pool.Close) + if _, err := pool.Exec(ctx, "TRUNCATE tracks, albums, artists RESTART IDENTITY CASCADE"); err != nil { + t.Fatalf("truncate: %v", err) + } + + const ( + blue = "aaaaaaaa-1994-2222-3333-444444444444" + green = "bbbbbbbb-2001-2222-3333-444444444444" + ) + root := t.TempDir() + tagged := func(rel, title, group string) string { + p := filepath.Join(root, rel) + writeTestMP3(t, p, map[string]string{ + "TIT2": title, "TPE1": "Weezer", "TALB": "Weezer", + "TXXX": "MusicBrainz Release Group Id\x00" + group, + }) + return p + } + tagged("Weezer (1994)/01.mp3", "My Name Is Jonas", blue) + tagged("Weezer (1994)/02.mp3", "No One Else", blue) + greenPath := tagged("Weezer (2001)/01.mp3", "Don't Let Go", green) + + scanner := New(pool, logger, []string{root}, nil) + if _, err := scanner.Scan(ctx, nil); err != nil { + t.Fatalf("scan: %v", err) + } + albumsByGroup := func() map[string]int { + t.Helper() + rows, err := pool.Query(ctx, `SELECT a.release_group_mbid, count(t.id) FROM albums a + JOIN tracks t ON t.album_id = a.id WHERE a.title = 'Weezer' GROUP BY a.id, a.release_group_mbid`) + if err != nil { + t.Fatal(err) + } + defer rows.Close() + out := map[string]int{} + for rows.Next() { + var g *string + var n int + if err := rows.Scan(&g, &n); err != nil { + t.Fatal(err) + } + if g == nil { + t.Fatal("an album with no release group") + } + out[*g] = n + } + return out + } + if got := albumsByGroup(); len(got) != 2 || got[blue] != 2 || got[green] != 1 { + t.Fatalf("albums by group = %v, want blue with 2 tracks and green with 1", got) + } + + // The green track as the old lookup left it: on the blue album's row, + // under an older tag-read version, its own album row gone. + if _, err := pool.Exec(ctx, ` + WITH b AS (SELECT id FROM albums WHERE release_group_mbid = $1) + UPDATE tracks SET album_id = (SELECT id FROM b), tag_read_version = 6 WHERE file_path = $2`, blue, greenPath); err != nil { + t.Fatal(err) + } + if _, err := pool.Exec(ctx, `DELETE FROM albums WHERE release_group_mbid = $1`, green); err != nil { + t.Fatal(err) + } + if _, err := scanner.Scan(ctx, nil); err != nil { + t.Fatalf("rescan: %v", err) + } + if got := albumsByGroup(); len(got) != 2 || got[blue] != 2 || got[green] != 1 { + t.Errorf("after the re-read, albums by group = %v, want the green track on its own album", got) + } +} diff --git a/internal/wanted/upgrade_test.go b/internal/wanted/upgrade_test.go index 9bc39008..51c43edb 100644 --- a/internal/wanted/upgrade_test.go +++ b/internal/wanted/upgrade_test.go @@ -6,6 +6,7 @@ import ( "testing" "time" + "github.com/jackc/pgx/v5/pgtype" "github.com/jackc/pgx/v5/pgxpool" "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" @@ -135,3 +136,39 @@ func TestCheckUpgrades_ARipIsReplacedWhateverTheSetting_Integration(t *testing.T t.Errorf("a rip: upgrade %q", w.Upgrade) } } + +// A missing track with no disc number is on disc 1, so a present 1/2 stands in +// for a missing "2". Air's Love 2 stayed wanted over one such row and was +// downloaded again for nothing (2026-10-20). A missing track no present track +// covers still counts. +func TestWantHoldings_AMissingTrackWithNoDiscIsOnDiscOne_Integration(t *testing.T) { + pool := newPool(t) + ctx := context.Background() + album := seedMP3s(t, pool, 320, false) + if _, err := pool.Exec(ctx, `UPDATE tracks SET disc_number = 1 WHERE album_id = $1`, album.ID); err != nil { + t.Fatal(err) + } + q := dbq.New(pool) + for _, n := range []int32{2, 3} { + num := n + tr, err := q.UpsertTrack(ctx, dbq.UpsertTrackParams{ + Title: fmt.Sprintf("Stale %d", n), AlbumID: album.ID, ArtistID: album.ArtistID, TrackNumber: &num, + DurationMs: 200000, FilePath: fmt.Sprintf("/music/ballad/stale-%d.mp3", n), FileSize: 1, FileFormat: "mp3", + }) + if err != nil { + t.Fatal(err) + } + if _, err := pool.Exec(ctx, `UPDATE tracks SET missing_since = now() - interval '2 days' WHERE id = $1`, tr.ID); err != nil { + t.Fatal(err) + } + } + rows, err := q.WantHoldings(ctx, dbq.WantHoldingsParams{ + Since: pgtype.Timestamptz{Time: time.Now(), Valid: true}, GroupMbid: theBallad, AlbumID: album.ID, + }) + if err != nil { + t.Fatal(err) + } + if len(rows) != 1 || rows[0].UncoveredMissing != 1 { + t.Fatalf("holdings = %+v, want one album with 1 uncovered missing track (track 3 only)", rows) + } +}