From 8bd48b4302e3c2c1608981589ab4ec1398ffab3a Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 8 Oct 2026 22:53:35 -0400 Subject: [PATCH] fix: a merge keeps the song link when the removed copy names the song (M498) MergeInheritSongLink took "linked" to mean song_key <> id, but the copy whose id became the song's key has no song_id of its own. Removing that copy dropped the survivor out of the song. It now asks whether another copy shares the key. The test covers both copies as the one removed, so it no longer passes or fails on which random id sorts first. The release-change test counted audit rows other tests left behind (ResetDB keeps audit_log); it now counts what its own pass adds. Co-Authored-By: Claude Opus 5.5 --- internal/db/dbq/songs.sql.go | 6 ++- internal/db/queries/songs.sql | 6 ++- internal/library/duplicate_resolve_test.go | 20 +++++--- internal/library/song_link_test.go | 56 +++++++++++++--------- 4 files changed, 55 insertions(+), 33 deletions(-) diff --git a/internal/db/dbq/songs.sql.go b/internal/db/dbq/songs.sql.go index 0820427d..1eaee010 100644 --- a/internal/db/dbq/songs.sql.go +++ b/internal/db/dbq/songs.sql.go @@ -85,7 +85,8 @@ UPDATE tracks s WHERE s.id = $1::uuid AND l.id = $2::uuid AND s.song_id IS NULL - AND l.song_key <> l.id + AND EXISTS (SELECT 1 FROM tracks o + WHERE o.song_key = l.song_key AND o.id <> l.id AND o.id <> s.id) ` type MergeInheritSongLinkParams struct { @@ -95,7 +96,8 @@ type MergeInheritSongLinkParams struct { // A merge keeps the removed copy's song link: when the copy being removed was // linked to copies on other releases and the survivor was not, the survivor -// joins that song. +// joins that song. The copy whose id names the song has no song_id of its +// own, so "linked" is another copy sharing its key, not a song_id. func (q *Queries) MergeInheritSongLink(ctx context.Context, arg MergeInheritSongLinkParams) error { _, err := q.db.Exec(ctx, mergeInheritSongLink, arg.SurvivorID, arg.LoserID) return err diff --git a/internal/db/queries/songs.sql b/internal/db/queries/songs.sql index bc3e200a..b57185ef 100644 --- a/internal/db/queries/songs.sql +++ b/internal/db/queries/songs.sql @@ -48,11 +48,13 @@ SELECT id, song_key, (source_verdict IS NOT DISTINCT FROM 'suspect')::boolean AS -- name: MergeInheritSongLink :exec -- A merge keeps the removed copy's song link: when the copy being removed was -- linked to copies on other releases and the survivor was not, the survivor --- joins that song. +-- joins that song. The copy whose id names the song has no song_id of its +-- own, so "linked" is another copy sharing its key, not a song_id. UPDATE tracks s SET song_id = l.song_key FROM tracks l WHERE s.id = sqlc.arg(survivor_id)::uuid AND l.id = sqlc.arg(loser_id)::uuid AND s.song_id IS NULL - AND l.song_key <> l.id; + AND EXISTS (SELECT 1 FROM tracks o + WHERE o.song_key = l.song_key AND o.id <> l.id AND o.id <> s.id); diff --git a/internal/library/duplicate_resolve_test.go b/internal/library/duplicate_resolve_test.go index 12df7e89..135e55ca 100644 --- a/internal/library/duplicate_resolve_test.go +++ b/internal/library/duplicate_resolve_test.go @@ -592,6 +592,17 @@ func TestResolveDuplicates_RecordsAReleaseChangeLidarrDidNotAnswer_Integration(t if !landed { lid.setErr = fmt.Errorf("%w: connection refused", lidarr.ErrUnreachable) } + // ResetDB keeps audit_log, so other tests' release changes are + // still in it: count what this pass adds. + audited := func() (n int) { + t.Helper() + if err := f.pool.QueryRow(context.Background(), + `SELECT count(*) FROM audit_log WHERE action = 'lidarr_release_change'`).Scan(&n); err != nil { + t.Fatal(err) + } + return n + } + before := audited() res, err := ResolveDuplicates(context.Background(), f.pool, nil, "", &undoingLidarr{fakeLidarr: lid, undo: !landed}, true) if landed { if err != nil || len(res.ReleaseChanges) != 1 { @@ -600,13 +611,8 @@ func TestResolveDuplicates_RecordsAReleaseChangeLidarrDidNotAnswer_Integration(t } else if len(res.ReleaseChanges) != 0 { t.Fatalf("recorded %d changes that did not land", len(res.ReleaseChanges)) } - var audited int - if err := f.pool.QueryRow(context.Background(), - `SELECT count(*) FROM audit_log WHERE action = 'lidarr_release_change'`).Scan(&audited); err != nil { - t.Fatal(err) - } - if want := map[bool]int{true: 1, false: 0}[landed]; audited != want { - t.Errorf("audited %d release changes, want %d", audited, want) + if got, want := audited()-before, map[bool]int{true: 1, false: 0}[landed]; got != want { + t.Errorf("audited %d release changes, want %d", got, want) } }) } diff --git a/internal/library/song_link_test.go b/internal/library/song_link_test.go index 97bf3aab..8d9179b7 100644 --- a/internal/library/song_link_test.go +++ b/internal/library/song_link_test.go @@ -164,29 +164,41 @@ func TestCrossReleaseLinkNeedsAutoResolve_Integration(t *testing.T) { } } -// A merge keeps the removed copy's link to the song on another release. +// A merge keeps the removed copy's link to the song on another release, +// whichever copy's id names the song. That copy has no song_id of its own, +// and before the fix its link was lost whenever the random ids made it the +// key (the flaky failure in run 8891). func TestMergeInheritsSongLink_Integration(t *testing.T) { - pool := newPool(t) - ctx := context.Background() - q := dbq.New(pool) - f := newSongLinkFixture(t, pool) - if _, err := q.LinkTracksAsSong(ctx, []pgtype.UUID{f.single.ID, f.albumCut.ID}); err != nil { - t.Fatal(err) - } - // A third, unlinked copy of the album cut stands in for a survivor. - other, err := q.UpsertTrack(ctx, dbq.UpsertTrackParams{ - Title: "Feel Good Inc.", AlbumID: f.albumCut.AlbumID, ArtistID: f.albumCut.ArtistID, - DurationMs: 1000, FilePath: "/music/Link Artist/Demon Days/06 Feel Good Inc (1).mp3", FileSize: 90, FileFormat: "mp3", - }) - if err != nil { - t.Fatal(err) - } - if err := q.MergeInheritSongLink(ctx, dbq.MergeInheritSongLinkParams{SurvivorID: other.ID, LoserID: f.albumCut.ID}); err != nil { - t.Fatal(err) - } - keys := songKeys(t, q, other.ID, f.single.ID) - if keys[0] != keys[1] { - t.Error("survivor did not join the song") + for _, removeAlbumCut := range []bool{true, false} { + pool := newPool(t) + ctx := context.Background() + q := dbq.New(pool) + f := newSongLinkFixture(t, pool) + if _, err := q.LinkTracksAsSong(ctx, []pgtype.UUID{f.single.ID, f.albumCut.ID}); err != nil { + t.Fatal(err) + } + loser, kept := f.albumCut, f.single + if !removeAlbumCut { + loser, kept = f.single, f.albumCut + } + // A third, unlinked copy of the removed track stands in for a survivor. + other, err := q.UpsertTrack(ctx, dbq.UpsertTrackParams{ + Title: loser.Title, AlbumID: loser.AlbumID, ArtistID: loser.ArtistID, + DurationMs: 1000, FilePath: loser.FilePath + ".copy.mp3", FileSize: 90, FileFormat: "mp3", + }) + if err != nil { + t.Fatal(err) + } + if err := q.MergeInheritSongLink(ctx, dbq.MergeInheritSongLinkParams{SurvivorID: other.ID, LoserID: loser.ID}); err != nil { + t.Fatal(err) + } + if _, err := q.DeleteTrack(ctx, loser.ID); err != nil { + t.Fatal(err) + } + keys := songKeys(t, q, other.ID, kept.ID) + if keys[0] != keys[1] { + t.Errorf("removing the %s: survivor did not join the song", map[bool]string{true: "album cut", false: "single"}[removeAlbumCut]) + } } }