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

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);

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

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