diff --git a/cmd/minstrel/main.go b/cmd/minstrel/main.go index 1169204a..5b67a423 100644 --- a/cmd/minstrel/main.go +++ b/cmd/minstrel/main.go @@ -16,6 +16,7 @@ import ( "git.fabledsword.com/bvandeusen/minstrel/internal/db" "git.fabledsword.com/bvandeusen/minstrel/internal/eventbus" "git.fabledsword.com/bvandeusen/minstrel/internal/library" + "git.fabledsword.com/bvandeusen/minstrel/internal/lidarr" "git.fabledsword.com/bvandeusen/minstrel/internal/lidarrconfig" "git.fabledsword.com/bvandeusen/minstrel/internal/lidarrrequests" "git.fabledsword.com/bvandeusen/minstrel/internal/logging" @@ -173,7 +174,18 @@ func run() error { // threading the bus through RunScan + TryStartScan + the Scheduler // + every test caller. Per-process singleton, set once at startup. library.SetEventBus(bus) - lidarrReconciler := lidarrrequests.NewReconciler(pool, lidarrCfg, logger.With("component", "lidarr"), bus) + // Per-tick Lidarr client factory: re-reads config so an admin save + // takes effect without a restart, returning nil while Lidarr is + // disabled/unconfigured. Mirrors server.go's lidarrClientFn; the + // reconciler uses it to (re)send unconfirmed adds. + lidarrClientFn := func() *lidarr.Client { + c, cerr := lidarrCfg.Get(ctx) + if cerr != nil || !c.Enabled || c.BaseURL == "" || c.APIKey == "" { + return nil + } + return lidarr.NewClient(c.BaseURL, c.APIKey) + } + lidarrReconciler := lidarrrequests.NewReconciler(pool, lidarrCfg, lidarrClientFn, logger.With("component", "lidarr"), bus) go lidarrReconciler.Run(ctx) // library_changes compactor (#357 follow-up). Daily tick; deletes diff --git a/flutter_client/lib/player/audio_handler.dart b/flutter_client/lib/player/audio_handler.dart index b8156f08..6e39a924 100644 --- a/flutter_client/lib/player/audio_handler.dart +++ b/flutter_client/lib/player/audio_handler.dart @@ -74,6 +74,20 @@ class MinstrelAudioHandler extends BaseAudioHandler with QueueHandler, SeekHandl /// new queue, leaving the player "locked" to a corrupted state. int _queueGeneration = 0; + /// Logical-queue index that just_audio player-index 0 currently maps + /// to. setQueueFromTracks fast-starts with a SINGLE source at player + /// index 0 while queue.value already holds the full list, so the + /// playing track's logical index is clampedInitial, not 0. The + /// transient currentIndexStream→0 emission that arrives just after + /// setAudioSources resolves (and after _suppressIndexUpdates is back + /// to false) would otherwise make _onCurrentIndexChanged broadcast + /// queue.value[0] — the FIRST track — over the correct item, pinning + /// the mini bar / playlist marker to the wrong track until a later + /// index event. Decremented in lockstep with the backward fill's + /// front inserts so player-index → logical-index stays correct and + /// lands at 0 once the player list fully matches queue.value. + int _logicalIndexBase = 0; + /// Tracks the most recent setQueueFromTracks() input so /// skipToQueueItem can reconstruct the source list. just_audio /// requires every source to be built before it can be a skip @@ -178,6 +192,11 @@ class MinstrelAudioHandler extends BaseAudioHandler with QueueHandler, SeekHandl _suppressIndexUpdates = true; try { await _player.setAudioSources([initial], initialIndex: 0); + // Player-index 0 holds the single fast-start source, which is + // logical-queue index clampedInitial. Record the offset before + // broadcasting so the post-resolve currentIndexStream→0 emission + // maps back to the correct item instead of queue.value[0]. + _logicalIndexBase = clampedInitial; // Broadcast in this order: queue first (so any consumer that // reacts to mediaItem and reads queue.value sees the consistent // pair), then mediaItem. @@ -242,6 +261,11 @@ class MinstrelAudioHandler extends BaseAudioHandler with QueueHandler, SeekHandl final src = await _buildAudioSource(tracks[i]); if (gen != _queueGeneration) return; await _player.insertAudioSource(i, src); + // Each front insert shifts the playing source's player index + // up by one; drop the base in lockstep so player-index → + // logical-index stays correct (and reaches 0 once the player + // list fully matches queue.value). + _logicalIndexBase -= 1; } } catch (e) { debugPrint('audio_handler: backward fill failed: $e'); @@ -447,8 +471,15 @@ class MinstrelAudioHandler extends BaseAudioHandler with QueueHandler, SeekHandl // track was passed via setQueueFromTracks(initialIndex:) regardless // of skip/auto-advance. final items = queue.value; - if (idx >= 0 && idx < items.length) { - mediaItem.add(items[idx]); + // Map the just_audio player index back to the logical queue index. + // During the fast-start/fill window the player list is a moving + // window offset from queue.value by _logicalIndexBase; mapping the + // raw player index straight in here is what previously broadcast + // queue.value[0] (the first track) over the correct item on every + // fast-start with initialIndex > 0. + final logical = idx + _logicalIndexBase; + if (logical >= 0 && logical < items.length) { + mediaItem.add(items[logical]); } unawaited(_loadArtForCurrentItem()); } diff --git a/internal/api/admin_requests_test.go b/internal/api/admin_requests_test.go index 741a7d6a..2106b5ed 100644 --- a/internal/api/admin_requests_test.go +++ b/internal/api/admin_requests_test.go @@ -287,9 +287,12 @@ func TestHandleApproveRequest_OverrideUsed(t *testing.T) { } } -// TestHandleApproveRequest_LidarrUnreachable_503 points config at an unreachable -// port and verifies POST /approve → 503 lidarr_unreachable, row stays pending. -func TestHandleApproveRequest_LidarrUnreachable_503(t *testing.T) { +// TestHandleApproveRequest_LidarrUnreachable_ApprovesDeferred points config +// at an unreachable port and verifies the durable-approve contract: POST +// /approve still succeeds (200) and the request becomes approved with the +// Lidarr add deferred (lidarr_add_confirmed_at NULL) for the reconciler to +// retry — the admin's decision is never lost to a transient Lidarr outage. +func TestHandleApproveRequest_LidarrUnreachable_ApprovesDeferred(t *testing.T) { h, _ := testHandlersWithClientFn(t) resetLidarrState(t, h) @@ -301,31 +304,39 @@ func TestHandleApproveRequest_LidarrUnreachable_503(t *testing.T) { rv := seedPendingArtistRequest(t, h, alice, "artist-mbid-unreachable", "Unreachable") w := doAdminRequestReq(t, h, http.MethodPost, "/api/admin/requests/"+uuidToString(rv.ID)+"/approve", nil, admin) - if w.Code != http.StatusServiceUnavailable { - t.Fatalf("status = %d, want 503; body = %s", w.Code, w.Body.String()) - } - var resp map[string]string - if err := json.Unmarshal(w.Body.Bytes(), &resp); err != nil { - t.Fatalf("decode: %v; body = %s", err, w.Body.String()) - } - if resp["error"] != "lidarr_unreachable" { - t.Errorf("error = %q, want lidarr_unreachable", resp["error"]) + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want 200 (durable approve despite Lidarr down); body = %s", w.Code, w.Body.String()) } - // Row must still be pending. - rows, err := h.lidarrRequests.ListByStatus(context.Background(), "pending", 10) + // Must no longer be pending. + pending, err := h.lidarrRequests.ListByStatus(context.Background(), "pending", 10) if err != nil { t.Fatalf("list pending: %v", err) } - found := false - for _, r := range rows { + for _, r := range pending { if r.ID == rv.ID { - found = true + t.Error("row still pending after durable approve") + } + } + + // Must be approved with the add deferred (confirmed_at NULL) so the + // reconciler retries it — not failed, not lost. + approved, err := h.lidarrRequests.ListByStatus(context.Background(), "approved", 10) + if err != nil { + t.Fatalf("list approved: %v", err) + } + var got *dbq.LidarrRequest + for i := range approved { + if approved[i].ID == rv.ID { + got = &approved[i] break } } - if !found { - t.Error("row is no longer pending after failed approve") + if got == nil { + t.Fatalf("row %s not in approved after approve with Lidarr down", uuidToString(rv.ID)) + } + if got.LidarrAddConfirmedAt.Valid { + t.Error("lidarr_add_confirmed_at set despite Lidarr unreachable; want NULL (deferred to reconciler)") } } diff --git a/internal/db/dbq/lidarr_requests.sql.go b/internal/db/dbq/lidarr_requests.sql.go index ac9d796e..73f2c716 100644 --- a/internal/db/dbq/lidarr_requests.sql.go +++ b/internal/db/dbq/lidarr_requests.sql.go @@ -20,7 +20,7 @@ UPDATE lidarr_requests decided_by = $4, updated_at = now() WHERE id = $1 AND status = 'pending' - RETURNING id, user_id, status, kind, lidarr_artist_mbid, lidarr_album_mbid, lidarr_track_mbid, artist_name, album_title, track_title, quality_profile_id, root_folder_path, decided_at, decided_by, notes, completed_at, matched_track_id, matched_album_id, matched_artist_id, requested_at, updated_at + RETURNING id, user_id, status, kind, lidarr_artist_mbid, lidarr_album_mbid, lidarr_track_mbid, artist_name, album_title, track_title, quality_profile_id, root_folder_path, decided_at, decided_by, notes, completed_at, matched_track_id, matched_album_id, matched_artist_id, requested_at, updated_at, lidarr_add_confirmed_at ` type ApproveLidarrRequestParams struct { @@ -60,6 +60,7 @@ func (q *Queries) ApproveLidarrRequest(ctx context.Context, arg ApproveLidarrReq &i.MatchedArtistID, &i.RequestedAt, &i.UpdatedAt, + &i.LidarrAddConfirmedAt, ) return i, err } @@ -72,7 +73,7 @@ UPDATE lidarr_requests decided_by = $2, updated_at = now() WHERE id = $1 AND user_id = $2 AND status = 'pending' - RETURNING id, user_id, status, kind, lidarr_artist_mbid, lidarr_album_mbid, lidarr_track_mbid, artist_name, album_title, track_title, quality_profile_id, root_folder_path, decided_at, decided_by, notes, completed_at, matched_track_id, matched_album_id, matched_artist_id, requested_at, updated_at + RETURNING id, user_id, status, kind, lidarr_artist_mbid, lidarr_album_mbid, lidarr_track_mbid, artist_name, album_title, track_title, quality_profile_id, root_folder_path, decided_at, decided_by, notes, completed_at, matched_track_id, matched_album_id, matched_artist_id, requested_at, updated_at, lidarr_add_confirmed_at ` type CancelLidarrRequestParams struct { @@ -110,6 +111,7 @@ func (q *Queries) CancelLidarrRequest(ctx context.Context, arg CancelLidarrReque &i.MatchedArtistID, &i.RequestedAt, &i.UpdatedAt, + &i.LidarrAddConfirmedAt, ) return i, err } @@ -123,7 +125,7 @@ UPDATE lidarr_requests completed_at = now(), updated_at = now() WHERE id = $1 AND status = 'approved' - RETURNING id, user_id, status, kind, lidarr_artist_mbid, lidarr_album_mbid, lidarr_track_mbid, artist_name, album_title, track_title, quality_profile_id, root_folder_path, decided_at, decided_by, notes, completed_at, matched_track_id, matched_album_id, matched_artist_id, requested_at, updated_at + RETURNING id, user_id, status, kind, lidarr_artist_mbid, lidarr_album_mbid, lidarr_track_mbid, artist_name, album_title, track_title, quality_profile_id, root_folder_path, decided_at, decided_by, notes, completed_at, matched_track_id, matched_album_id, matched_artist_id, requested_at, updated_at, lidarr_add_confirmed_at ` type CompleteLidarrRequestParams struct { @@ -169,6 +171,7 @@ func (q *Queries) CompleteLidarrRequest(ctx context.Context, arg CompleteLidarrR &i.MatchedArtistID, &i.RequestedAt, &i.UpdatedAt, + &i.LidarrAddConfirmedAt, ) return i, err } @@ -179,7 +182,7 @@ INSERT INTO lidarr_requests ( lidarr_artist_mbid, lidarr_album_mbid, lidarr_track_mbid, artist_name, album_title, track_title ) VALUES ($1, $2, $3, $4, $5, $6, $7, $8) -RETURNING id, user_id, status, kind, lidarr_artist_mbid, lidarr_album_mbid, lidarr_track_mbid, artist_name, album_title, track_title, quality_profile_id, root_folder_path, decided_at, decided_by, notes, completed_at, matched_track_id, matched_album_id, matched_artist_id, requested_at, updated_at +RETURNING id, user_id, status, kind, lidarr_artist_mbid, lidarr_album_mbid, lidarr_track_mbid, artist_name, album_title, track_title, quality_profile_id, root_folder_path, decided_at, decided_by, notes, completed_at, matched_track_id, matched_album_id, matched_artist_id, requested_at, updated_at, lidarr_add_confirmed_at ` type CreateLidarrRequestParams struct { @@ -227,12 +230,13 @@ func (q *Queries) CreateLidarrRequest(ctx context.Context, arg CreateLidarrReque &i.MatchedArtistID, &i.RequestedAt, &i.UpdatedAt, + &i.LidarrAddConfirmedAt, ) return i, err } const getLidarrRequestByID = `-- name: GetLidarrRequestByID :one -SELECT id, user_id, status, kind, lidarr_artist_mbid, lidarr_album_mbid, lidarr_track_mbid, artist_name, album_title, track_title, quality_profile_id, root_folder_path, decided_at, decided_by, notes, completed_at, matched_track_id, matched_album_id, matched_artist_id, requested_at, updated_at FROM lidarr_requests WHERE id = $1 +SELECT id, user_id, status, kind, lidarr_artist_mbid, lidarr_album_mbid, lidarr_track_mbid, artist_name, album_title, track_title, quality_profile_id, root_folder_path, decided_at, decided_by, notes, completed_at, matched_track_id, matched_album_id, matched_artist_id, requested_at, updated_at, lidarr_add_confirmed_at FROM lidarr_requests WHERE id = $1 ` func (q *Queries) GetLidarrRequestByID(ctx context.Context, id pgtype.UUID) (LidarrRequest, error) { @@ -260,6 +264,53 @@ func (q *Queries) GetLidarrRequestByID(ctx context.Context, id pgtype.UUID) (Lid &i.MatchedArtistID, &i.RequestedAt, &i.UpdatedAt, + &i.LidarrAddConfirmedAt, + ) + return i, err +} + +const getNonTerminalRequestForMBID = `-- name: GetNonTerminalRequestForMBID :one +SELECT id, user_id, status, kind, lidarr_artist_mbid, lidarr_album_mbid, lidarr_track_mbid, artist_name, album_title, track_title, quality_profile_id, root_folder_path, decided_at, decided_by, notes, completed_at, matched_track_id, matched_album_id, matched_artist_id, requested_at, updated_at, lidarr_add_confirmed_at FROM lidarr_requests + WHERE status IN ('pending', 'approved', 'completed') + AND ((kind = 'artist' AND lidarr_artist_mbid = $1) + OR (kind = 'album' AND lidarr_album_mbid = $1) + OR (kind = 'track' AND lidarr_track_mbid = $1)) + ORDER BY requested_at ASC + LIMIT 1 +` + +// Returns the oldest non-terminal (pending/approved/completed) request +// whose own-kind MBID matches @mbid, or pgx.ErrNoRows when none. Create() +// uses this to dedupe: a re-request for something already in flight +// returns the existing row instead of inserting a duplicate. MBIDs are +// globally unique per entity type, so the cross-column OR cannot +// false-match (an artist MBID never collides with an album/track MBID). +func (q *Queries) GetNonTerminalRequestForMBID(ctx context.Context, mbid string) (LidarrRequest, error) { + row := q.db.QueryRow(ctx, getNonTerminalRequestForMBID, mbid) + var i LidarrRequest + err := row.Scan( + &i.ID, + &i.UserID, + &i.Status, + &i.Kind, + &i.LidarrArtistMbid, + &i.LidarrAlbumMbid, + &i.LidarrTrackMbid, + &i.ArtistName, + &i.AlbumTitle, + &i.TrackTitle, + &i.QualityProfileID, + &i.RootFolderPath, + &i.DecidedAt, + &i.DecidedBy, + &i.Notes, + &i.CompletedAt, + &i.MatchedTrackID, + &i.MatchedAlbumID, + &i.MatchedArtistID, + &i.RequestedAt, + &i.UpdatedAt, + &i.LidarrAddConfirmedAt, ) return i, err } @@ -287,7 +338,7 @@ func (q *Queries) HasNonTerminalRequestForMBID(ctx context.Context, mbid string) } const listApprovedLidarrRequestsForReconcile = `-- name: ListApprovedLidarrRequestsForReconcile :many -SELECT id, user_id, status, kind, lidarr_artist_mbid, lidarr_album_mbid, lidarr_track_mbid, artist_name, album_title, track_title, quality_profile_id, root_folder_path, decided_at, decided_by, notes, completed_at, matched_track_id, matched_album_id, matched_artist_id, requested_at, updated_at FROM lidarr_requests +SELECT id, user_id, status, kind, lidarr_artist_mbid, lidarr_album_mbid, lidarr_track_mbid, artist_name, album_title, track_title, quality_profile_id, root_folder_path, decided_at, decided_by, notes, completed_at, matched_track_id, matched_album_id, matched_artist_id, requested_at, updated_at, lidarr_add_confirmed_at FROM lidarr_requests WHERE status = 'approved' ORDER BY decided_at ASC LIMIT $1 @@ -324,6 +375,7 @@ func (q *Queries) ListApprovedLidarrRequestsForReconcile(ctx context.Context, li &i.MatchedArtistID, &i.RequestedAt, &i.UpdatedAt, + &i.LidarrAddConfirmedAt, ); err != nil { return nil, err } @@ -336,7 +388,7 @@ func (q *Queries) ListApprovedLidarrRequestsForReconcile(ctx context.Context, li } const listLidarrRequestsByStatus = `-- name: ListLidarrRequestsByStatus :many -SELECT id, user_id, status, kind, lidarr_artist_mbid, lidarr_album_mbid, lidarr_track_mbid, artist_name, album_title, track_title, quality_profile_id, root_folder_path, decided_at, decided_by, notes, completed_at, matched_track_id, matched_album_id, matched_artist_id, requested_at, updated_at FROM lidarr_requests +SELECT id, user_id, status, kind, lidarr_artist_mbid, lidarr_album_mbid, lidarr_track_mbid, artist_name, album_title, track_title, quality_profile_id, root_folder_path, decided_at, decided_by, notes, completed_at, matched_track_id, matched_album_id, matched_artist_id, requested_at, updated_at, lidarr_add_confirmed_at FROM lidarr_requests WHERE status = $1 ORDER BY requested_at DESC LIMIT $2 @@ -378,6 +430,7 @@ func (q *Queries) ListLidarrRequestsByStatus(ctx context.Context, arg ListLidarr &i.MatchedArtistID, &i.RequestedAt, &i.UpdatedAt, + &i.LidarrAddConfirmedAt, ); err != nil { return nil, err } @@ -390,7 +443,7 @@ func (q *Queries) ListLidarrRequestsByStatus(ctx context.Context, arg ListLidarr } const listLidarrRequestsForUser = `-- name: ListLidarrRequestsForUser :many -SELECT id, user_id, status, kind, lidarr_artist_mbid, lidarr_album_mbid, lidarr_track_mbid, artist_name, album_title, track_title, quality_profile_id, root_folder_path, decided_at, decided_by, notes, completed_at, matched_track_id, matched_album_id, matched_artist_id, requested_at, updated_at FROM lidarr_requests +SELECT id, user_id, status, kind, lidarr_artist_mbid, lidarr_album_mbid, lidarr_track_mbid, artist_name, album_title, track_title, quality_profile_id, root_folder_path, decided_at, decided_by, notes, completed_at, matched_track_id, matched_album_id, matched_artist_id, requested_at, updated_at, lidarr_add_confirmed_at FROM lidarr_requests WHERE user_id = $1 ORDER BY requested_at DESC LIMIT $2 @@ -432,6 +485,7 @@ func (q *Queries) ListLidarrRequestsForUser(ctx context.Context, arg ListLidarrR &i.MatchedArtistID, &i.RequestedAt, &i.UpdatedAt, + &i.LidarrAddConfirmedAt, ); err != nil { return nil, err } @@ -443,6 +497,23 @@ func (q *Queries) ListLidarrRequestsForUser(ctx context.Context, arg ListLidarrR return items, nil } +const markLidarrRequestAddConfirmed = `-- name: MarkLidarrRequestAddConfirmed :exec +UPDATE lidarr_requests + SET lidarr_add_confirmed_at = now(), + updated_at = now() + WHERE id = $1 + AND status = 'approved' + AND lidarr_add_confirmed_at IS NULL +` + +// Idempotently records that Lidarr accepted the add for an approved +// request. The lidarr_add_confirmed_at IS NULL guard makes a redundant +// reconciler retry a no-op rather than bumping updated_at every tick. +func (q *Queries) MarkLidarrRequestAddConfirmed(ctx context.Context, id pgtype.UUID) error { + _, err := q.db.Exec(ctx, markLidarrRequestAddConfirmed, id) + return err +} + const rejectLidarrRequest = `-- name: RejectLidarrRequest :one UPDATE lidarr_requests SET status = 'rejected', @@ -451,7 +522,7 @@ UPDATE lidarr_requests decided_by = $3, updated_at = now() WHERE id = $1 AND status = 'pending' - RETURNING id, user_id, status, kind, lidarr_artist_mbid, lidarr_album_mbid, lidarr_track_mbid, artist_name, album_title, track_title, quality_profile_id, root_folder_path, decided_at, decided_by, notes, completed_at, matched_track_id, matched_album_id, matched_artist_id, requested_at, updated_at + RETURNING id, user_id, status, kind, lidarr_artist_mbid, lidarr_album_mbid, lidarr_track_mbid, artist_name, album_title, track_title, quality_profile_id, root_folder_path, decided_at, decided_by, notes, completed_at, matched_track_id, matched_album_id, matched_artist_id, requested_at, updated_at, lidarr_add_confirmed_at ` type RejectLidarrRequestParams struct { @@ -485,6 +556,7 @@ func (q *Queries) RejectLidarrRequest(ctx context.Context, arg RejectLidarrReque &i.MatchedArtistID, &i.RequestedAt, &i.UpdatedAt, + &i.LidarrAddConfirmedAt, ) return i, err } diff --git a/internal/db/dbq/models.go b/internal/db/dbq/models.go index 3b772075..230daadc 100644 --- a/internal/db/dbq/models.go +++ b/internal/db/dbq/models.go @@ -325,27 +325,28 @@ type LidarrQuarantineAction struct { } type LidarrRequest struct { - ID pgtype.UUID - UserID pgtype.UUID - Status LidarrRequestStatus - Kind LidarrRequestKind - LidarrArtistMbid string - LidarrAlbumMbid *string - LidarrTrackMbid *string - ArtistName string - AlbumTitle *string - TrackTitle *string - QualityProfileID *int32 - RootFolderPath *string - DecidedAt pgtype.Timestamptz - DecidedBy pgtype.UUID - Notes *string - CompletedAt pgtype.Timestamptz - MatchedTrackID pgtype.UUID - MatchedAlbumID pgtype.UUID - MatchedArtistID pgtype.UUID - RequestedAt pgtype.Timestamptz - UpdatedAt pgtype.Timestamptz + ID pgtype.UUID + UserID pgtype.UUID + Status LidarrRequestStatus + Kind LidarrRequestKind + LidarrArtistMbid string + LidarrAlbumMbid *string + LidarrTrackMbid *string + ArtistName string + AlbumTitle *string + TrackTitle *string + QualityProfileID *int32 + RootFolderPath *string + DecidedAt pgtype.Timestamptz + DecidedBy pgtype.UUID + Notes *string + CompletedAt pgtype.Timestamptz + MatchedTrackID pgtype.UUID + MatchedAlbumID pgtype.UUID + MatchedArtistID pgtype.UUID + RequestedAt pgtype.Timestamptz + UpdatedAt pgtype.Timestamptz + LidarrAddConfirmedAt pgtype.Timestamptz } type PasswordReset struct { diff --git a/internal/db/migrations/0031_lidarr_add_confirmed_at.down.sql b/internal/db/migrations/0031_lidarr_add_confirmed_at.down.sql new file mode 100644 index 00000000..40964d14 --- /dev/null +++ b/internal/db/migrations/0031_lidarr_add_confirmed_at.down.sql @@ -0,0 +1,2 @@ +ALTER TABLE lidarr_requests + DROP COLUMN lidarr_add_confirmed_at; diff --git a/internal/db/migrations/0031_lidarr_add_confirmed_at.up.sql b/internal/db/migrations/0031_lidarr_add_confirmed_at.up.sql new file mode 100644 index 00000000..37b05a72 --- /dev/null +++ b/internal/db/migrations/0031_lidarr_add_confirmed_at.up.sql @@ -0,0 +1,8 @@ +-- Additive: records when Lidarr accepted the add (POST artist/album) +-- for an approved request. NULL = not yet sent/confirmed; the +-- reconciler idempotently (re)sends the add until this is set, then +-- watches for the import. Decouples the durable admin approval from the +-- best-effort Lidarr call so a transient Lidarr outage no longer forces +-- a manual re-approve. +ALTER TABLE lidarr_requests + ADD COLUMN lidarr_add_confirmed_at timestamptz; diff --git a/internal/db/queries/lidarr_requests.sql b/internal/db/queries/lidarr_requests.sql index d075a07a..7561acb0 100644 --- a/internal/db/queries/lidarr_requests.sql +++ b/internal/db/queries/lidarr_requests.sql @@ -93,3 +93,29 @@ SELECT EXISTS ( OR (kind = 'album' AND lidarr_album_mbid = @mbid) OR (kind = 'track' AND lidarr_track_mbid = @mbid)) ); + +-- name: GetNonTerminalRequestForMBID :one +-- Returns the oldest non-terminal (pending/approved/completed) request +-- whose own-kind MBID matches @mbid, or pgx.ErrNoRows when none. Create() +-- uses this to dedupe: a re-request for something already in flight +-- returns the existing row instead of inserting a duplicate. MBIDs are +-- globally unique per entity type, so the cross-column OR cannot +-- false-match (an artist MBID never collides with an album/track MBID). +SELECT * FROM lidarr_requests + WHERE status IN ('pending', 'approved', 'completed') + AND ((kind = 'artist' AND lidarr_artist_mbid = @mbid) + OR (kind = 'album' AND lidarr_album_mbid = @mbid) + OR (kind = 'track' AND lidarr_track_mbid = @mbid)) + ORDER BY requested_at ASC + LIMIT 1; + +-- name: MarkLidarrRequestAddConfirmed :exec +-- Idempotently records that Lidarr accepted the add for an approved +-- request. The lidarr_add_confirmed_at IS NULL guard makes a redundant +-- reconciler retry a no-op rather than bumping updated_at every tick. +UPDATE lidarr_requests + SET lidarr_add_confirmed_at = now(), + updated_at = now() + WHERE id = $1 + AND status = 'approved' + AND lidarr_add_confirmed_at IS NULL; diff --git a/internal/lidarr/client.go b/internal/lidarr/client.go index 83f2f10c..dcbbe0c0 100644 --- a/internal/lidarr/client.go +++ b/internal/lidarr/client.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "encoding/json" + "errors" "fmt" "io" "net/http" @@ -300,12 +301,29 @@ func (c *Client) AddArtist(ctx context.Context, p AddArtistParams) error { } resp, err := c.post(ctx, "/api/v1/artist", body) if err != nil { + if isAlreadyExists(err) { + return ErrAlreadyExists + } return err } _ = resp.Body.Close() return nil } +// isAlreadyExists detects Lidarr's 400 "already in your library" rejection +// from an add endpoint. The post() helper buckets it as ErrLookupFailed +// but embeds Lidarr's errorMessage body snippet; matching the phrase lets +// the idempotent retry path treat a re-add as success instead of error. +func isAlreadyExists(err error) bool { + if !errors.Is(err, ErrLookupFailed) { + return false + } + m := strings.ToLower(err.Error()) + return strings.Contains(m, "already exists") || + strings.Contains(m, "already been added") || + strings.Contains(m, "already in your library") +} + // AddAlbum posts to POST /api/v1/album. Returns nil on 2xx; typed error // otherwise. func (c *Client) AddAlbum(ctx context.Context, p AddAlbumParams) error { @@ -324,6 +342,9 @@ func (c *Client) AddAlbum(ctx context.Context, p AddAlbumParams) error { } resp, err := c.post(ctx, "/api/v1/album", body) if err != nil { + if isAlreadyExists(err) { + return ErrAlreadyExists + } return err } _ = resp.Body.Close() diff --git a/internal/lidarr/errors.go b/internal/lidarr/errors.go index 0351f12c..fc27d359 100644 --- a/internal/lidarr/errors.go +++ b/internal/lidarr/errors.go @@ -14,6 +14,12 @@ var ( ErrLookupFailed = errors.New("lidarr: lookup failed") // 4xx other than 401/403 ErrServerError = errors.New("lidarr: server error") // 5xx ErrInvalidPayload = errors.New("lidarr: invalid payload") + // ErrAlreadyExists is returned by AddArtist/AddAlbum when Lidarr + // rejects the add with a 400 because the artist/album is already in + // its library. This is the desired end state, not a failure — the + // idempotent retry path (reconciler re-sending an add after a partial + // success) treats it as confirmed rather than spinning forever. + ErrAlreadyExists = errors.New("lidarr: already exists") // ErrNotFound is returned by LookupArtistByMBID and LookupAlbumByMBID // when Lidarr returns 200 with an empty array — i.e., the MBID isn't in // Lidarr's monitored set. Distinguished from network/auth errors so admin diff --git a/internal/lidarrrequests/reconciler.go b/internal/lidarrrequests/reconciler.go index 3546dc1b..22b3c5cb 100644 --- a/internal/lidarrrequests/reconciler.go +++ b/internal/lidarrrequests/reconciler.go @@ -2,6 +2,7 @@ package lidarrrequests import ( "context" + "fmt" "log/slog" "time" @@ -11,16 +12,20 @@ import ( "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" "git.fabledsword.com/bvandeusen/minstrel/internal/eventbus" + "git.fabledsword.com/bvandeusen/minstrel/internal/lidarr" "git.fabledsword.com/bvandeusen/minstrel/internal/lidarrconfig" ) // Reconciler is a background worker that periodically scans approved -// lidarr_requests and transitions them to completed when their target -// track/album/artist has appeared in the local library (matched by MBID). -// Errors per-row are logged at WARN and do not abort the tick. +// lidarr_requests. For rows whose Lidarr add hasn't been confirmed it +// (re)sends the add idempotently until it sticks; for confirmed rows it +// transitions them to completed when their target track/album/artist has +// appeared in the local library (matched by MBID). Errors per-row are +// logged at WARN and do not abort the tick. type Reconciler struct { pool *pgxpool.Pool lidarrCfg *lidarrconfig.Service + clientFn func() *lidarr.Client logger *slog.Logger bus *eventbus.Bus tick time.Duration @@ -28,12 +33,19 @@ type Reconciler struct { } // NewReconciler constructs a Reconciler with production defaults: -// 5-minute tick, batch size 50. Pass nil for bus when no SSE publishing -// is desired (tests, or environments without the event stream enabled). -func NewReconciler(pool *pgxpool.Pool, cfg *lidarrconfig.Service, logger *slog.Logger, bus *eventbus.Bus) *Reconciler { +// 5-minute tick, batch size 50. clientFn is the per-tick Lidarr client +// factory (same pattern as Service) so config changes apply without a +// restart; pass nil to disable the add-retry path. Pass nil for bus when +// no SSE publishing is desired (tests, or environments without the event +// stream enabled). +func NewReconciler(pool *pgxpool.Pool, cfg *lidarrconfig.Service, clientFn func() *lidarr.Client, logger *slog.Logger, bus *eventbus.Bus) *Reconciler { + if clientFn == nil { + clientFn = func() *lidarr.Client { return nil } + } return &Reconciler{ pool: pool, lidarrCfg: cfg, + clientFn: clientFn, logger: logger, bus: bus, tick: 5 * time.Minute, @@ -118,7 +130,7 @@ func (r *Reconciler) tickOnce(ctx context.Context) error { } for _, row := range rows { - if err := r.reconcileRow(ctx, q, row); err != nil { + if err := r.reconcileRow(ctx, q, cfg, row); err != nil { r.logger.Warn("lidarrrequests: reconciler row failed", "request_id", row.ID, "kind", row.Kind, @@ -129,10 +141,23 @@ func (r *Reconciler) tickOnce(ctx context.Context) error { return nil } -// reconcileRow checks the library for a match and, if found, calls -// CompleteLidarrRequest. It returns an error only for unexpected DB -// failures; a no-match is not an error. -func (r *Reconciler) reconcileRow(ctx context.Context, q *dbq.Queries, row dbq.LidarrRequest) error { +// reconcileRow drives a request forward by one step. If Lidarr hasn't +// confirmed the add yet it (re)sends it and returns — the import can't +// land before Lidarr has the add, so there's nothing to match this tick. +// Once confirmed, it checks the library for a match and, if found, calls +// CompleteLidarrRequest. Returns an error only for unexpected failures +// (the add retry's transient error included, so tickOnce logs WARN and +// it's retried next tick — "Lidarr just keeps trying"); a no-match is +// not an error. +func (r *Reconciler) reconcileRow(ctx context.Context, q *dbq.Queries, cfg lidarrconfig.Config, row dbq.LidarrRequest) error { + if !row.LidarrAddConfirmedAt.Valid { + if err := r.ensureLidarrAdd(ctx, q, cfg, row); err != nil { + return err + } + // ensureLidarrAdd either confirmed the add or no-op'd (Lidarr + // client unavailable). Fall through to import-matching so an + // item already present in the library still completes this tick. + } switch row.Kind { case dbq.LidarrRequestKindArtist: return r.reconcileArtist(ctx, q, row) @@ -146,6 +171,26 @@ func (r *Reconciler) reconcileRow(ctx context.Context, q *dbq.Queries, row dbq.L } } +// ensureLidarrAdd (re)sends the Lidarr add for an approved request whose +// add hasn't been confirmed. On success (including ErrAlreadyExists, +// handled inside sendLidarrAdd) it stamps lidarr_add_confirmed_at so this +// stops re-sending and the next tick moves to import-matching. A failure +// is returned so tickOnce logs WARN and the row is retried next tick; +// there is intentionally no failed-state or attempt cap — per product +// decision Lidarr just keeps trying and the operator monitors Lidarr. +func (r *Reconciler) ensureLidarrAdd(ctx context.Context, q *dbq.Queries, cfg lidarrconfig.Config, row dbq.LidarrRequest) error { + client := r.clientFn() + if client == nil { + // Lidarr disabled mid-flight; tickOnce already gates on + // cfg.Enabled, so this is just defensive — nothing to do. + return nil + } + if err := sendLidarrAdd(ctx, client, cfg, row); err != nil { + return fmt.Errorf("lidarr add retry: %w", err) + } + return q.MarkLidarrRequestAddConfirmed(ctx, row.ID) +} + func (r *Reconciler) reconcileArtist(ctx context.Context, q *dbq.Queries, row dbq.LidarrRequest) error { var artistID pgtype.UUID err := r.pool.QueryRow(ctx, diff --git a/internal/lidarrrequests/reconciler_integration_test.go b/internal/lidarrrequests/reconciler_integration_test.go index 28bbd50a..959b6067 100644 --- a/internal/lidarrrequests/reconciler_integration_test.go +++ b/internal/lidarrrequests/reconciler_integration_test.go @@ -121,7 +121,7 @@ func TestReconciler_MatchesArtistByMBID(t *testing.T) { Kind: "artist", LidarrArtistMBID: artistMBID, ArtistName: "Boards of Canada", }) - rec := NewReconciler(pool, lidarrconfig.New(pool), newTestLogger(), nil) + rec := NewReconciler(pool, lidarrconfig.New(pool), nil, newTestLogger(), nil) if err := rec.tickOnce(ctx); err != nil { t.Fatalf("tickOnce: %v", err) } @@ -158,7 +158,7 @@ func TestReconciler_MatchesAlbumByMBID(t *testing.T) { LidarrAlbumMBID: albumMBID, AlbumTitle: "Test Album", }) - rec := NewReconciler(pool, lidarrconfig.New(pool), newTestLogger(), nil) + rec := NewReconciler(pool, lidarrconfig.New(pool), nil, newTestLogger(), nil) if err := rec.tickOnce(ctx); err != nil { t.Fatalf("tickOnce: %v", err) } @@ -200,7 +200,7 @@ func TestReconciler_MatchesTrackViaAlbumMBID(t *testing.T) { LidarrTrackMBID: trackMBID, TrackTitle: "Track One", }) - rec := NewReconciler(pool, lidarrconfig.New(pool), newTestLogger(), nil) + rec := NewReconciler(pool, lidarrconfig.New(pool), nil, newTestLogger(), nil) if err := rec.tickOnce(ctx); err != nil { t.Fatalf("tickOnce: %v", err) } @@ -234,7 +234,7 @@ func TestReconciler_NoMatchLeavesPending(t *testing.T) { Kind: "artist", LidarrArtistMBID: "nonexistent-mbid-xyz", ArtistName: "Ghost Artist", }) - rec := NewReconciler(pool, lidarrconfig.New(pool), newTestLogger(), nil) + rec := NewReconciler(pool, lidarrconfig.New(pool), nil, newTestLogger(), nil) if err := rec.tickOnce(ctx); err != nil { t.Fatalf("tickOnce: %v", err) } @@ -278,7 +278,7 @@ func TestReconciler_AlreadyCompletedRowNotReprocessed(t *testing.T) { t.Fatalf("CompleteLidarrRequest setup: %v", err) } - rec := NewReconciler(pool, lidarrconfig.New(pool), newTestLogger(), nil) + rec := NewReconciler(pool, lidarrconfig.New(pool), nil, newTestLogger(), nil) if err := rec.tickOnce(ctx); err != nil { t.Fatalf("tickOnce: %v", err) } @@ -313,7 +313,7 @@ func TestReconciler_DisabledIsNoOp(t *testing.T) { Kind: "artist", LidarrArtistMBID: artistMBID, ArtistName: "No-Op Artist", }) - rec := NewReconciler(pool, lidarrconfig.New(pool), newTestLogger(), nil) + rec := NewReconciler(pool, lidarrconfig.New(pool), nil, newTestLogger(), nil) if err := rec.tickOnce(ctx); err != nil { t.Fatalf("tickOnce: %v", err) } @@ -345,7 +345,7 @@ func TestReconciler_RunRunsAtLeastOneTick(t *testing.T) { Kind: "artist", LidarrArtistMBID: mbid, ArtistName: "Run Artist", }) - rec := NewReconciler(pool, lidarrconfig.New(pool), newTestLogger(), nil) + rec := NewReconciler(pool, lidarrconfig.New(pool), nil, newTestLogger(), nil) rec.tick = 25 * time.Millisecond done := make(chan struct{}) @@ -399,7 +399,7 @@ func TestReconciler_TrackKindNoTracksInAlbumYet(t *testing.T) { LidarrTrackMBID: "t-mbid-not-yet", TrackTitle: "Some Track", }) - rec := NewReconciler(pool, lidarrconfig.New(pool), newTestLogger(), nil) + rec := NewReconciler(pool, lidarrconfig.New(pool), nil, newTestLogger(), nil) if err := rec.tickOnce(ctx); err != nil { t.Fatalf("tickOnce: %v", err) } @@ -428,7 +428,7 @@ func TestReconciler_AlbumKindNoMatchInAlbumsTable(t *testing.T) { LidarrAlbumMBID: "al-not-in-library", AlbumTitle: "Nope Album", }) - rec := NewReconciler(pool, lidarrconfig.New(pool), newTestLogger(), nil) + rec := NewReconciler(pool, lidarrconfig.New(pool), nil, newTestLogger(), nil) if err := rec.tickOnce(ctx); err != nil { t.Fatalf("tickOnce: %v", err) } diff --git a/internal/lidarrrequests/service.go b/internal/lidarrrequests/service.go index 7552b3b9..c5721732 100644 --- a/internal/lidarrrequests/service.go +++ b/internal/lidarrrequests/service.go @@ -84,6 +84,23 @@ func (s *Service) Create(ctx context.Context, userID pgtype.UUID, p CreateParams return dbq.LidarrRequest{}, err } q := dbq.New(s.pool) + // Dedup: if a non-terminal request for this MBID already exists, + // return it instead of inserting a duplicate. The Discover/search UI + // already hides requested candidates; this backstops races and direct + // API callers so we don't accrue duplicate request rows. Match on the + // new request's own-kind MBID (mirrors HasNonTerminalRequestForMBID). + dedupMBID := p.LidarrArtistMBID + switch p.Kind { + case "album": + dedupMBID = p.LidarrAlbumMBID + case "track": + dedupMBID = p.LidarrTrackMBID + } + if existing, derr := q.GetNonTerminalRequestForMBID(ctx, dedupMBID); derr == nil { + return existing, nil + } else if !errors.Is(derr, pgx.ErrNoRows) { + return dbq.LidarrRequest{}, fmt.Errorf("create: dedup check: %w", derr) + } row, err := q.CreateLidarrRequest(ctx, dbq.CreateLidarrRequestParams{ UserID: userID, Kind: dbq.LidarrRequestKind(p.Kind), @@ -145,11 +162,14 @@ func (s *Service) ListForUser(ctx context.Context, userID pgtype.UUID, limit int }) } -// Approve transitions a pending request to approved, snapshotting the -// chosen quality profile + root folder, then calls Lidarr to actually -// add the artist/album, then triggers a library scan. If Lidarr returns -// an error, the request stays pending — the admin sees the error and -// can retry without losing the request. +// Approve records the admin's approval DURABLY first (snapshotting the +// chosen quality profile + root folder), then makes a best-effort Lidarr +// add and triggers a library scan. The add is no longer a gate: if Lidarr +// is unreachable the request still becomes approved and the reconciler +// idempotently (re)sends the add on each tick until it sticks (see +// sendLidarrAdd + Reconciler.ensureLidarrAdd) — "Lidarr just keeps +// trying" without the admin re-clicking. Pre-conditions (lidarr disabled, +// not pending, defaults incomplete) still fail fast before any approval. func (s *Service) Approve(ctx context.Context, requestID pgtype.UUID, adminID pgtype.UUID, ov ApproveOverrides) (dbq.LidarrRequest, error) { cfg, err := s.lidarrCfg.Get(ctx) if err != nil { @@ -182,23 +202,62 @@ func (s *Service) Approve(ctx context.Context, requestID pgtype.UUID, adminID pg return dbq.LidarrRequest{}, ErrDefaultsIncomplete } - // Lidarr requires a metadata profile id on POST /api/v1/artist (and - // on /api/v1/album when it has to create the parent artist). Prefer - // the operator's pinned choice from cfg; fall back to fetching the - // list and using the first when nothing's been saved yet (operators - // upgrading from before metadata-profile support landed). + // Record the approval durably FIRST so a Lidarr outage can't lose the + // admin's decision. The snapshotted qp/rf also feed the reconciler's + // retry, so a deferred add uses exactly what the admin approved with. + approved, err := dbq.New(s.pool).ApproveLidarrRequest(ctx, dbq.ApproveLidarrRequestParams{ + ID: requestID, + QualityProfileID: int32Ptr(qp), + RootFolderPath: strPtr(rf), + DecidedBy: adminID, + }) + if err != nil { + return dbq.LidarrRequest{}, fmt.Errorf("approve: persist: %w", err) + } + + // Best-effort immediate add for the common (Lidarr-up) path. Any + // failure is deliberately swallowed: the request is already durably + // approved and the reconciler retries the add on its next tick, + // leaving lidarr_add_confirmed_at NULL until it succeeds. + if addErr := sendLidarrAdd(ctx, client, cfg, approved); addErr == nil { + _ = dbq.New(s.pool).MarkLidarrRequestAddConfirmed(ctx, requestID) + } + s.scanFn() + return approved, nil +} + +// sendLidarrAdd performs the idempotent Lidarr add for an approved +// request. Used by Service.Approve (best-effort, common path) and by the +// Reconciler (retry path). qp/rf come from the request's snapshot (set at +// Approve time) with a config-default fallback; the metadata profile is +// resolved from config, falling back to Lidarr's first profile for +// operators who upgraded from before metadata-profile support. Treats +// lidarr.ErrAlreadyExists as success — the artist/album is already in +// Lidarr, which is exactly the desired end state. +func sendLidarrAdd(ctx context.Context, client *lidarr.Client, cfg lidarrconfig.Config, row dbq.LidarrRequest) error { + qp := cfg.DefaultQualityProfileID + if row.QualityProfileID != nil && *row.QualityProfileID != 0 { + qp = int(*row.QualityProfileID) + } + rf := cfg.DefaultRootFolderPath + if row.RootFolderPath != nil && *row.RootFolderPath != "" { + rf = *row.RootFolderPath + } + if qp == 0 || rf == "" { + return ErrDefaultsIncomplete + } mdProfileID := cfg.DefaultMetadataProfileID if mdProfileID == 0 { mdProfiles, mdErr := client.ListMetadataProfiles(ctx) if mdErr != nil { - return dbq.LidarrRequest{}, fmt.Errorf("approve: list metadata profiles: %w", mdErr) + return fmt.Errorf("list metadata profiles: %w", mdErr) } if len(mdProfiles) == 0 { - return dbq.LidarrRequest{}, fmt.Errorf("approve: no metadata profiles configured in lidarr") + return fmt.Errorf("no metadata profiles configured in lidarr") } mdProfileID = mdProfiles[0].ID } - + var err error switch row.Kind { case dbq.LidarrRequestKindArtist: err = client.AddArtist(ctx, lidarr.AddArtistParams{ @@ -224,22 +283,10 @@ func (s *Service) Approve(ctx context.Context, requestID pgtype.UUID, adminID pg RootFolderPath: rf, }) } - if err != nil { - return dbq.LidarrRequest{}, fmt.Errorf("approve: lidarr add: %w", err) + if err != nil && !errors.Is(err, lidarr.ErrAlreadyExists) { + return err } - - approved, err := dbq.New(s.pool).ApproveLidarrRequest(ctx, dbq.ApproveLidarrRequestParams{ - ID: requestID, - QualityProfileID: int32Ptr(qp), - RootFolderPath: strPtr(rf), - DecidedBy: adminID, - }) - if err != nil { - // Lidarr accepted but our DB update failed; admin should retry. - return dbq.LidarrRequest{}, fmt.Errorf("approve: persist: %w", err) - } - s.scanFn() - return approved, nil + return nil } // Reject transitions a pending request to rejected, recording notes and