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]) + } } }