fix(lidarr): complete a request only when the album actually came back (#5263)

Re-acquisition targets albums with ANY track missing, and completion only
asked for a track on disk, so the tracks that never left completed every
re-acquisition request the moment Lidarr accepted the add (52 on the deploy,
each ~150ms after its add).

An album or track request now completes when an album named by its release
or group has a track on disk AND either a track arrived after the request
(a new album, or Lidarr fetching another release into its own row) or no
track that was missing at the request is still missing. added_at is the
arrival clock; updated_at moves on every tag re-read.

Migration 0071 reopens completed album/track requests whose matched album
fails that test, as approved with the match cleared; the Lidarr add stays
confirmed, so nothing is re-sent.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

internal/db/migrations/0071_reopen_false_reacquisition_completions.down.sql

@@ -0,0 +1,3 @@
+-- The reopened requests were never complete; the reconciler completes them
+-- again once their albums come back. Nothing to undo.
+SELECT 1;

internal/db/migrations/0071_reopen_false_reacquisition_completions.up.sql

@@ -0,0 +1,30 @@
+-- #5263: the reconciler completed album and track requests as soon as the album
+-- had any track on disk. Re-acquisition targets albums with only SOME tracks
+-- missing, so those requests completed the moment Lidarr accepted the add,
+-- with nothing downloaded.
+--
+-- Reopen every completed album/track request whose matched album does not meet
+-- the corrected test (see albumForRequest in internal/lidarrrequests): a track
+-- that arrived after the request, or no track that was already missing at the
+-- request still missing. The reconciler then judges them again; the Lidarr add
+-- stays confirmed, so nothing is re-sent. Requests that genuinely completed
+-- meet the test and are left alone.
+UPDATE lidarr_requests lr
+   SET status           = 'approved',
+       completed_at     = NULL,
+       matched_album_id = NULL,
+       matched_track_id = NULL,
+       updated_at       = now()
+ WHERE lr.status = 'completed'
+   AND lr.kind IN ('album', 'track')
+   AND lr.matched_album_id IS NOT NULL
+   AND NOT EXISTS (
+         SELECT 1 FROM tracks t
+          WHERE t.album_id = lr.matched_album_id
+            AND t.missing_since IS NULL
+            AND t.added_at > lr.requested_at)
+   AND EXISTS (
+         SELECT 1 FROM tracks t
+          WHERE t.album_id = lr.matched_album_id
+            AND t.missing_since IS NOT NULL
+            AND t.missing_since < lr.requested_at);

internal/lidarrrequests/reconciler.go

@@ -258,16 +258,32 @@ func (r *Reconciler) repointAtReleaseGroup(ctx context.Context, q *dbq.Queries,
 	return row, true
 }
 
-// albumForRequest finds the library album a request's album id names, by
+// albumForRequest finds the library album a request's album id ($1) names, by
-// release id or release group, and only once it has a track back on disk. A
+// release id or release group, once the request ($2 = requested_at) has been
-// re-acquisition request names an album whose row never went away; matching
+// answered: the album has a track on disk AND either
-// the row alone would complete the request before Lidarr delivered anything.
+//
+//   - a track arrived after the request (a new album, or Lidarr fetching
+//     another release of the group into its own row), or
+//   - no track that was already missing when the request was made is still
+//     missing (the lost files came back in place or were adopted at a new path).
+//
+// "Has a track on disk" alone is not enough (#5263): re-acquisition targets
+// albums with ANY track missing, so the tracks that never left satisfied it
+// the moment Lidarr accepted the add. added_at is the arrival clock because
+// nothing rewrites it; updated_at moves on every tag re-read.
 // An exact release match is preferred over another release of the group.
 const albumForRequest = `
 SELECT a.id
   FROM albums a
  WHERE (a.mbid = $1 OR a.release_group_mbid = $1)
    AND EXISTS (SELECT 1 FROM tracks t WHERE t.album_id = a.id AND t.missing_since IS NULL)
+   AND (
+        EXISTS (SELECT 1 FROM tracks t
+                 WHERE t.album_id = a.id AND t.missing_since IS NULL AND t.added_at > $2)
+        OR NOT EXISTS (SELECT 1 FROM tracks t
+                        WHERE t.album_id = a.id AND t.missing_since IS NOT NULL
+                          AND t.missing_since < $2)
+       )
  ORDER BY (a.mbid = $1) DESC, a.id
  LIMIT 1`
 
@@ -301,7 +317,7 @@ func (r *Reconciler) reconcileAlbum(ctx context.Context, q *dbq.Queries, row dbq
 		return nil
 	}
 	var albumID pgtype.UUID
-	err := r.pool.QueryRow(ctx, albumForRequest, *row.LidarrAlbumMbid).Scan(&albumID)
+	err := r.pool.QueryRow(ctx, albumForRequest, *row.LidarrAlbumMbid, row.RequestedAt).Scan(&albumID)
 	if err != nil {
 		if isNoRows(err) {
 			return nil
@@ -327,7 +343,7 @@ func (r *Reconciler) reconcileTrack(ctx context.Context, q *dbq.Queries, row dbq
 	}
 	// Track-kind requests match via their parent album's MBID, not track.mbid.
 	var albumID pgtype.UUID
-	err := r.pool.QueryRow(ctx, albumForRequest, *row.LidarrAlbumMbid).Scan(&albumID)
+	err := r.pool.QueryRow(ctx, albumForRequest, *row.LidarrAlbumMbid, row.RequestedAt).Scan(&albumID)
 	if err != nil {
 		if isNoRows(err) {
 			return nil
@@ -335,10 +351,10 @@ func (r *Reconciler) reconcileTrack(ctx context.Context, q *dbq.Queries, row dbq
 		return err
 	}
 
-	// Load any track from that album to set matched_track_id.
+	// Load a track from that album to set matched_track_id, newest arrival first.
 	var trackID pgtype.UUID
 	err = r.pool.QueryRow(ctx,
-		"SELECT id FROM tracks WHERE album_id = $1 AND missing_since IS NULL ORDER BY id LIMIT 1",
+		"SELECT id FROM tracks WHERE album_id = $1 AND missing_since IS NULL ORDER BY added_at DESC, id LIMIT 1",
 		albumID,
 	).Scan(&trackID)
 	if err != nil {

internal/lidarrrequests/reconciler_release_group_test.go

@@ -3,12 +3,17 @@ package lidarrrequests
 import (
 	"context"
 	"encoding/json"
+	"io/fs"
 	"net/http"
 	"net/http/httptest"
 	"strings"
 	"sync"
 	"testing"
 
+	"github.com/jackc/pgx/v5/pgtype"
+	"github.com/jackc/pgx/v5/pgxpool"
+
+	"git.fabledsword.com/bvandeusen/minstrel/internal/db"
 	"git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq"
 	"git.fabledsword.com/bvandeusen/minstrel/internal/lidarr"
 	"git.fabledsword.com/bvandeusen/minstrel/internal/lidarrconfig"
@@ -201,3 +206,168 @@ func TestReconciler_RepointsAReleaseIdRequestAtItsReleaseGroup(t *testing.T) {
 		t.Errorf("MusicBrainz asked again on the next tick (%d -> %d)", before, asked)
 	}
 }
+
+// markMissing stamps a track missing since well before any request a test makes.
+func markMissing(t *testing.T, pool *pgxpool.Pool, trackID pgtype.UUID) {
+	t.Helper()
+	if _, err := pool.Exec(context.Background(),
+		"UPDATE tracks SET missing_since = now() - interval '2 days' WHERE id = $1", trackID); err != nil {
+		t.Fatal(err)
+	}
+}
+
+func requestStatus(t *testing.T, q *dbq.Queries, id pgtype.UUID) dbq.LidarrRequest {
+	t.Helper()
+	got, err := q.GetLidarrRequestByID(context.Background(), id)
+	if err != nil {
+		t.Fatal(err)
+	}
+	return got
+}
+
+// #5263: re-acquisition targets albums with SOME tracks missing. The tracks
+// that never left must not complete the request; it completes once the lost
+// track is back.
+func TestReconciler_PartlyMissingAlbumCompletesWhenTheLostTrackReturns(t *testing.T) {
+	pool := newPool(t)
+	q := dbq.New(pool)
+	ctx := context.Background()
+	enableLidarrForPool(t, pool)
+	user := seedUser(t, pool)
+
+	const artistMBID, release = "rg-artist-partial", "rg-release-partial"
+	artist := seedArtist(t, q, "Partial Artist", artistMBID)
+	album := seedAlbum(t, q, artist.ID, "Partial Album", release)
+	_ = seedTrack(t, q, album.ID, artist.ID, "Kept", "/music/rg-partial/01.flac")
+	lost := seedTrack(t, q, album.ID, artist.ID, "Lost", "/music/rg-partial/02.flac")
+	markMissing(t, pool, lost.ID)
+
+	req := seedApprovedRequestDirect(t, q, user, CreateParams{
+		Kind: "album", LidarrArtistMBID: artistMBID, ArtistName: "Partial Artist",
+		LidarrAlbumMBID: release, AlbumTitle: "Partial Album",
+	})
+	rec := NewReconciler(pool, lidarrconfig.New(pool), nil, newTestLogger(), nil)
+	if err := rec.tickOnce(ctx); err != nil {
+		t.Fatalf("tickOnce: %v", err)
+	}
+	if got := requestStatus(t, q, req.ID); got.Status != dbq.LidarrRequestStatusApproved {
+		t.Fatalf("status = %v while a track is still missing, want approved", got.Status)
+	}
+
+	if _, err := pool.Exec(ctx, "UPDATE tracks SET missing_since = NULL WHERE id = $1", lost.ID); err != nil {
+		t.Fatal(err)
+	}
+	if err := rec.tickOnce(ctx); err != nil {
+		t.Fatalf("second tick: %v", err)
+	}
+	got := requestStatus(t, q, req.ID)
+	if got.Status != dbq.LidarrRequestStatusCompleted || got.MatchedAlbumID != album.ID {
+		t.Errorf("status %v matched %v after the track returned, want completed on %v",
+			got.Status, got.MatchedAlbumID, album.ID)
+	}
+}
+
+// Lidarr may fetch a different release of the group, which the scanner files
+// as its own album row. A track arriving there after the request completes it,
+// even though the old row's lost tracks stay missing.
+func TestReconciler_PartlyMissingAlbumCompletesOnAnotherReleaseArriving(t *testing.T) {
+	pool := newPool(t)
+	q := dbq.New(pool)
+	ctx := context.Background()
+	enableLidarrForPool(t, pool)
+	user := seedUser(t, pool)
+
+	const artistMBID, oldRelease, newRelease, group = "rg-artist-edition", "rg-release-old", "rg-release-new", "rg-group-edition"
+	artist := seedArtist(t, q, "Edition Artist", artistMBID)
+	old := seedAlbum(t, q, artist.ID, "Edition Album", oldRelease)
+	if err := q.SetAlbumReleaseGroupMbidIfNull(ctx, dbq.SetAlbumReleaseGroupMbidIfNullParams{
+		ID: old.ID, ReleaseGroupMbid: nilableStr(group),
+	}); err != nil {
+		t.Fatal(err)
+	}
+	_ = seedTrack(t, q, old.ID, artist.ID, "Kept", "/music/rg-edition-old/01.flac")
+	lost := seedTrack(t, q, old.ID, artist.ID, "Lost", "/music/rg-edition-old/02.flac")
+	markMissing(t, pool, lost.ID)
+
+	req := seedApprovedRequestDirect(t, q, user, CreateParams{
+		Kind: "album", LidarrArtistMBID: artistMBID, ArtistName: "Edition Artist",
+		LidarrAlbumMBID: group, AlbumTitle: "Edition Album",
+	})
+	rec := NewReconciler(pool, lidarrconfig.New(pool), nil, newTestLogger(), nil)
+	if err := rec.tickOnce(ctx); err != nil {
+		t.Fatalf("tickOnce: %v", err)
+	}
+	if got := requestStatus(t, q, req.ID); got.Status != dbq.LidarrRequestStatusApproved {
+		t.Fatalf("status = %v before anything arrived, want approved", got.Status)
+	}
+
+	arrived := seedAlbum(t, q, artist.ID, "Edition Album (Deluxe)", newRelease)
+	if err := q.SetAlbumReleaseGroupMbidIfNull(ctx, dbq.SetAlbumReleaseGroupMbidIfNullParams{
+		ID: arrived.ID, ReleaseGroupMbid: nilableStr(group),
+	}); err != nil {
+		t.Fatal(err)
+	}
+	_ = seedTrack(t, q, arrived.ID, artist.ID, "Lost", "/music/rg-edition-new/02.flac")
+	if err := rec.tickOnce(ctx); err != nil {
+		t.Fatalf("second tick: %v", err)
+	}
+	got := requestStatus(t, q, req.ID)
+	if got.Status != dbq.LidarrRequestStatusCompleted || got.MatchedAlbumID != arrived.ID {
+		t.Errorf("status %v matched %v, want completed on the arrived release %v",
+			got.Status, got.MatchedAlbumID, arrived.ID)
+	}
+}
+
+// Migration 0071 reopens requests the old check completed against a partly
+// missing album, and leaves a genuine completion alone.
+func TestMigration0071_ReopensFalseCompletions(t *testing.T) {
+	pool := newPool(t)
+	q := dbq.New(pool)
+	ctx := context.Background()
+	user := seedUser(t, pool)
+
+	artist := seedArtist(t, q, "Reopen Artist", "rg-artist-reopen")
+	partial := seedAlbum(t, q, artist.ID, "Partial", "rg-release-reopen-partial")
+	_ = seedTrack(t, q, partial.ID, artist.ID, "Kept", "/music/rg-reopen-partial/01.flac")
+	lost := seedTrack(t, q, partial.ID, artist.ID, "Lost", "/music/rg-reopen-partial/02.flac")
+	markMissing(t, pool, lost.ID)
+	falseReq := seedApprovedRequestDirect(t, q, user, CreateParams{
+		Kind: "album", LidarrArtistMBID: "rg-artist-reopen", ArtistName: "Reopen Artist",
+		LidarrAlbumMBID: "rg-release-reopen-partial", AlbumTitle: "Partial",
+	})
+
+	genuineReq := seedApprovedRequestDirect(t, q, user, CreateParams{
+		Kind: "album", LidarrArtistMBID: "rg-artist-reopen", ArtistName: "Reopen Artist",
+		LidarrAlbumMBID: "rg-release-reopen-new", AlbumTitle: "New",
+	})
+	arrived := seedAlbum(t, q, artist.ID, "New", "rg-release-reopen-new")
+	_ = seedTrack(t, q, arrived.ID, artist.ID, "Fresh", "/music/rg-reopen-new/01.flac")
+
+	for _, c := range []struct {
+		id    pgtype.UUID
+		album pgtype.UUID
+	}{{falseReq.ID, partial.ID}, {genuineReq.ID, arrived.ID}} {
+		if _, err := q.CompleteLidarrRequest(ctx, dbq.CompleteLidarrRequestParams{
+			ID: c.id, MatchedAlbumID: c.album,
+		}); err != nil {
+			t.Fatal(err)
+		}
+	}
+
+	up, err := fs.ReadFile(db.MigrationsFS(), "0071_reopen_false_reacquisition_completions.up.sql")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if _, err := pool.Exec(ctx, string(up)); err != nil {
+		t.Fatalf("migration: %v", err)
+	}
+
+	got := requestStatus(t, q, falseReq.ID)
+	if got.Status != dbq.LidarrRequestStatusApproved || got.CompletedAt.Valid || got.MatchedAlbumID.Valid {
+		t.Errorf("false completion: status %v completed_at %v matched %v, want approved and cleared",
+			got.Status, got.CompletedAt, got.MatchedAlbumID)
+	}
+	if got := requestStatus(t, q, genuineReq.ID); got.Status != dbq.LidarrRequestStatusCompleted {
+		t.Errorf("genuine completion reopened: status %v", got.Status)
+	}
+}