From 4e3ce4065cec0b6a9d540bad0a0b8b3a53dbd3ef Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Wed, 7 Oct 2026 14:55:32 -0400 Subject: [PATCH] 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 --- ...n_false_reacquisition_completions.down.sql | 3 + ...pen_false_reacquisition_completions.up.sql | 30 ++++ internal/lidarrrequests/reconciler.go | 32 +++- .../reconciler_release_group_test.go | 170 ++++++++++++++++++ 4 files changed, 227 insertions(+), 8 deletions(-) create mode 100644 internal/db/migrations/0071_reopen_false_reacquisition_completions.down.sql create mode 100644 internal/db/migrations/0071_reopen_false_reacquisition_completions.up.sql diff --git a/internal/db/migrations/0071_reopen_false_reacquisition_completions.down.sql b/internal/db/migrations/0071_reopen_false_reacquisition_completions.down.sql new file mode 100644 index 00000000..03bb6852 --- /dev/null +++ b/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; diff --git a/internal/db/migrations/0071_reopen_false_reacquisition_completions.up.sql b/internal/db/migrations/0071_reopen_false_reacquisition_completions.up.sql new file mode 100644 index 00000000..dd240a0b --- /dev/null +++ b/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); diff --git a/internal/lidarrrequests/reconciler.go b/internal/lidarrrequests/reconciler.go index 589f5264..122bdb4d 100644 --- a/internal/lidarrrequests/reconciler.go +++ b/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 -// release id or release group, and only once it has a track back on disk. A -// re-acquisition request names an album whose row never went away; matching -// the row alone would complete the request before Lidarr delivered anything. +// albumForRequest finds the library album a request's album id ($1) names, by +// release id or release group, once the request ($2 = requested_at) has been +// answered: the album has a track on disk AND either +// +// - 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 { diff --git a/internal/lidarrrequests/reconciler_release_group_test.go b/internal/lidarrrequests/reconciler_release_group_test.go index f8cd3a22..696e2a4d 100644 --- a/internal/lidarrrequests/reconciler_release_group_test.go +++ b/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) + } +}