diff --git a/internal/db/dbq/albums.sql.go b/internal/db/dbq/albums.sql.go index ff5aa098..baaf1f56 100644 --- a/internal/db/dbq/albums.sql.go +++ b/internal/db/dbq/albums.sql.go @@ -248,6 +248,22 @@ func (q *Queries) GetAlbumsByIDs(ctx context.Context, dollar_1 []pgtype.UUID) ([ return items, nil } +const getReleaseGroupForReleaseMbid = `-- name: GetReleaseGroupForReleaseMbid :one +SELECT release_group_mbid::text + FROM albums + WHERE mbid = $1 AND release_group_mbid IS NOT NULL + LIMIT 1 +` + +// #5241: the release group the library already knows for a release id, so a +// request carrying a release id can be repaired without asking MusicBrainz. +func (q *Queries) GetReleaseGroupForReleaseMbid(ctx context.Context, mbid *string) (string, error) { + row := q.db.QueryRow(ctx, getReleaseGroupForReleaseMbid, mbid) + var release_group_mbid string + err := row.Scan(&release_group_mbid) + return release_group_mbid, err +} + const listAlbumsAlphaByArtist = `-- name: ListAlbumsAlphaByArtist :many SELECT albums.id, albums.title, albums.sort_title, albums.artist_id, albums.release_date, albums.mbid, albums.cover_art_path, albums.created_at, albums.updated_at, albums.cover_art_source, albums.cover_art_sources_version, albums.release_group_mbid, artists.sort_name AS artist_sort_name FROM albums @@ -852,6 +868,25 @@ func (q *Queries) SetAlbumReleaseGroupMbidIfNull(ctx context.Context, arg SetAlb return err } +const setReleaseGroupForReleaseMbidIfNull = `-- name: SetReleaseGroupForReleaseMbidIfNull :exec +UPDATE albums + SET release_group_mbid = $1::text, + updated_at = now() + WHERE mbid = $2::text AND release_group_mbid IS NULL +` + +type SetReleaseGroupForReleaseMbidIfNullParams struct { + ReleaseGroupMbid string + ReleaseMbid string +} + +// #5241: cache a MusicBrainz-resolved release group onto the album row that +// holds the release, filling only a NULL. +func (q *Queries) SetReleaseGroupForReleaseMbidIfNull(ctx context.Context, arg SetReleaseGroupForReleaseMbidIfNullParams) error { + _, err := q.db.Exec(ctx, setReleaseGroupForReleaseMbidIfNull, arg.ReleaseGroupMbid, arg.ReleaseMbid) + return err +} + const upsertAlbum = `-- name: UpsertAlbum :one INSERT INTO albums (title, sort_title, artist_id, release_date, mbid, cover_art_path, release_group_mbid) VALUES ($1, $2, $3, $4, $5, $6, $7) diff --git a/internal/db/dbq/lidarr_requests.sql.go b/internal/db/dbq/lidarr_requests.sql.go index 73f2c716..ecaac112 100644 --- a/internal/db/dbq/lidarr_requests.sql.go +++ b/internal/db/dbq/lidarr_requests.sql.go @@ -560,3 +560,23 @@ func (q *Queries) RejectLidarrRequest(ctx context.Context, arg RejectLidarrReque ) return i, err } + +const setLidarrRequestAlbumMbid = `-- name: SetLidarrRequestAlbumMbid :exec +UPDATE lidarr_requests + SET lidarr_album_mbid = $2, + updated_at = now() + WHERE id = $1 +` + +type SetLidarrRequestAlbumMbidParams struct { + ID pgtype.UUID + LidarrAlbumMbid *string +} + +// #5241: repoint a request at the album's release group. Requests made before +// the release-group read named albums by MusicBrainz release id, which Lidarr +// does not index; the reconciler rewrites them once it has resolved the group. +func (q *Queries) SetLidarrRequestAlbumMbid(ctx context.Context, arg SetLidarrRequestAlbumMbidParams) error { + _, err := q.db.Exec(ctx, setLidarrRequestAlbumMbid, arg.ID, arg.LidarrAlbumMbid) + return err +} diff --git a/internal/db/dbq/reacquisition.sql.go b/internal/db/dbq/reacquisition.sql.go index 335e2771..0a4e9da0 100644 --- a/internal/db/dbq/reacquisition.sql.go +++ b/internal/db/dbq/reacquisition.sql.go @@ -114,6 +114,7 @@ const listAlbumsDueReacquisition = `-- name: ListAlbumsDueReacquisition :many SELECT albums.id AS album_id, albums.title AS album_title, albums.mbid AS album_mbid, + albums.release_group_mbid AS album_release_group_mbid, artists.id AS artist_id, artists.name AS artist_name, artists.mbid AS artist_mbid, @@ -135,7 +136,7 @@ SELECT albums.id AS album_id, * POWER(2, GREATEST(COALESCE(r.attempts, 0) - 1, 0)))::int, $3::int)) ) - GROUP BY albums.id, albums.title, albums.mbid, + GROUP BY albums.id, albums.title, albums.mbid, albums.release_group_mbid, artists.id, artists.name, artists.mbid, r.attempts, r.last_attempt_at ORDER BY r.last_attempt_at NULLS FIRST, albums.sort_title LIMIT $4 @@ -149,14 +150,15 @@ type ListAlbumsDueReacquisitionParams struct { } type ListAlbumsDueReacquisitionRow struct { - AlbumID pgtype.UUID - AlbumTitle string - AlbumMbid *string - ArtistID pgtype.UUID - ArtistName string - ArtistMbid *string - MissingTrackCount int64 - Attempts int32 + AlbumID pgtype.UUID + AlbumTitle string + AlbumMbid *string + AlbumReleaseGroupMbid *string + ArtistID pgtype.UUID + ArtistName string + ArtistMbid *string + MissingTrackCount int64 + Attempts int32 } // The sweeper's selection. An album qualifies when: @@ -194,6 +196,7 @@ func (q *Queries) ListAlbumsDueReacquisition(ctx context.Context, arg ListAlbums &i.AlbumID, &i.AlbumTitle, &i.AlbumMbid, + &i.AlbumReleaseGroupMbid, &i.ArtistID, &i.ArtistName, &i.ArtistMbid, diff --git a/internal/db/queries/albums.sql b/internal/db/queries/albums.sql index 1a7ec259..4ce5452f 100644 --- a/internal/db/queries/albums.sql +++ b/internal/db/queries/albums.sql @@ -152,6 +152,22 @@ UPDATE albums updated_at = now() WHERE id = $1 AND release_group_mbid IS NULL; +-- name: GetReleaseGroupForReleaseMbid :one +-- #5241: the release group the library already knows for a release id, so a +-- request carrying a release id can be repaired without asking MusicBrainz. +SELECT release_group_mbid::text + FROM albums + WHERE mbid = $1 AND release_group_mbid IS NOT NULL + LIMIT 1; + +-- name: SetReleaseGroupForReleaseMbidIfNull :exec +-- #5241: cache a MusicBrainz-resolved release group onto the album row that +-- holds the release, filling only a NULL. +UPDATE albums + SET release_group_mbid = sqlc.arg(release_group_mbid)::text, + updated_at = now() + WHERE mbid = sqlc.arg(release_mbid)::text AND release_group_mbid IS NULL; + -- name: ListAlbumsMissingMbidWithTrack :many -- One-shot MBID backfill: returns each album where mbid IS NULL alongside -- one of its tracks' file_path so the worker can re-read tags. LIMIT diff --git a/internal/db/queries/lidarr_requests.sql b/internal/db/queries/lidarr_requests.sql index 7561acb0..ec96568c 100644 --- a/internal/db/queries/lidarr_requests.sql +++ b/internal/db/queries/lidarr_requests.sql @@ -119,3 +119,12 @@ UPDATE lidarr_requests WHERE id = $1 AND status = 'approved' AND lidarr_add_confirmed_at IS NULL; + +-- name: SetLidarrRequestAlbumMbid :exec +-- #5241: repoint a request at the album's release group. Requests made before +-- the release-group read named albums by MusicBrainz release id, which Lidarr +-- does not index; the reconciler rewrites them once it has resolved the group. +UPDATE lidarr_requests + SET lidarr_album_mbid = $2, + updated_at = now() + WHERE id = $1; diff --git a/internal/db/queries/reacquisition.sql b/internal/db/queries/reacquisition.sql index 5f638e3e..85f3de86 100644 --- a/internal/db/queries/reacquisition.sql +++ b/internal/db/queries/reacquisition.sql @@ -40,6 +40,7 @@ RETURNING *; SELECT albums.id AS album_id, albums.title AS album_title, albums.mbid AS album_mbid, + albums.release_group_mbid AS album_release_group_mbid, artists.id AS artist_id, artists.name AS artist_name, artists.mbid AS artist_mbid, @@ -61,7 +62,7 @@ SELECT albums.id AS album_id, * POWER(2, GREATEST(COALESCE(r.attempts, 0) - 1, 0)))::int, sqlc.arg(backoff_max_hours)::int)) ) - GROUP BY albums.id, albums.title, albums.mbid, + GROUP BY albums.id, albums.title, albums.mbid, albums.release_group_mbid, artists.id, artists.name, artists.mbid, r.attempts, r.last_attempt_at ORDER BY r.last_attempt_at NULLS FIRST, albums.sort_title LIMIT sqlc.arg(page_limit); diff --git a/internal/lidarrrequests/reconciler.go b/internal/lidarrrequests/reconciler.go index 22b3c5cb..589f5264 100644 --- a/internal/lidarrrequests/reconciler.go +++ b/internal/lidarrrequests/reconciler.go @@ -2,6 +2,7 @@ package lidarrrequests import ( "context" + "errors" "fmt" "log/slog" "time" @@ -14,6 +15,7 @@ import ( "git.fabledsword.com/bvandeusen/minstrel/internal/eventbus" "git.fabledsword.com/bvandeusen/minstrel/internal/lidarr" "git.fabledsword.com/bvandeusen/minstrel/internal/lidarrconfig" + "git.fabledsword.com/bvandeusen/minstrel/internal/tags" ) // Reconciler is a background worker that periodically scans approved @@ -30,6 +32,10 @@ type Reconciler struct { bus *eventbus.Bus tick time.Duration batch int32 + // releaseGroup names the MusicBrainz release group of a release id, to + // repair a request stored under a release id Lidarr does not index + // (#5241). A field so tests can stand in for MusicBrainz. + releaseGroup func(ctx context.Context, releaseMBID string) (string, error) } // NewReconciler constructs a Reconciler with production defaults: @@ -50,6 +56,8 @@ func NewReconciler(pool *pgxpool.Pool, cfg *lidarrconfig.Service, clientFn func( bus: bus, tick: 5 * time.Minute, batch: 50, + + releaseGroup: tags.ReleaseGroupForRelease, } } @@ -185,12 +193,84 @@ func (r *Reconciler) ensureLidarrAdd(ctx context.Context, q *dbq.Queries, cfg li // cfg.Enabled, so this is just defensive — nothing to do. return nil } - if err := sendLidarrAdd(ctx, client, cfg, row); err != nil { + err := sendLidarrAdd(ctx, client, cfg, row) + if errors.Is(err, lidarr.ErrNotFound) { + // Lidarr's metadata has no album under this id. Requests made before + // albums carried a release group (#5241) name the album by its + // MusicBrainz release id, which Lidarr does not index: repoint the + // request at the release group and try once more. + if repaired, ok := r.repointAtReleaseGroup(ctx, q, row); ok { + row = repaired + err = sendLidarrAdd(ctx, client, cfg, row) + } + } + if err != nil { return fmt.Errorf("lidarr add retry: %w", err) } return q.MarkLidarrRequestAddConfirmed(ctx, row.ID) } +// repointAtReleaseGroup reads an album or track request's album id as a +// MusicBrainz release and, when its release group can be named, rewrites the +// request to it. The library's own albums answer first; MusicBrainz is asked +// only for a release no album row knows the group of, and its answer is cached +// onto that album. Reports false when there is nothing to repoint: not an +// album request, the group is unknown, or the id already is the group. +func (r *Reconciler) repointAtReleaseGroup(ctx context.Context, q *dbq.Queries, row dbq.LidarrRequest) (dbq.LidarrRequest, bool) { + if row.Kind == dbq.LidarrRequestKindArtist || row.LidarrAlbumMbid == nil || *row.LidarrAlbumMbid == "" { + return row, false + } + release := *row.LidarrAlbumMbid + group, err := q.GetReleaseGroupForReleaseMbid(ctx, &release) + if err != nil { + if !isNoRows(err) { + r.logger.Warn("lidarrrequests: release group lookup failed", "request_id", row.ID, "err", err) + return row, false + } + group, err = r.releaseGroup(ctx, release) + if err != nil { + if !errors.Is(err, tags.ErrNotFound) { + r.logger.Warn("lidarrrequests: MusicBrainz release group lookup failed", + "request_id", row.ID, "release_mbid", release, "err", err) + } + return row, false + } + if cerr := q.SetReleaseGroupForReleaseMbidIfNull(ctx, dbq.SetReleaseGroupForReleaseMbidIfNullParams{ + ReleaseGroupMbid: group, + ReleaseMbid: release, + }); cerr != nil { + r.logger.Warn("lidarrrequests: cache release group failed", "release_mbid", release, "err", cerr) + } + } + if group == "" || group == release { + return row, false + } + if err := q.SetLidarrRequestAlbumMbid(ctx, dbq.SetLidarrRequestAlbumMbidParams{ + ID: row.ID, + LidarrAlbumMbid: &group, + }); err != nil { + r.logger.Warn("lidarrrequests: repoint request failed", "request_id", row.ID, "err", err) + return row, false + } + r.logger.Info("lidarrrequests: request repointed from release to release group", + "request_id", row.ID, "release_mbid", release, "release_group_mbid", group) + row.LidarrAlbumMbid = &group + 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. +// 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) + ORDER BY (a.mbid = $1) DESC, a.id + LIMIT 1` + func (r *Reconciler) reconcileArtist(ctx context.Context, q *dbq.Queries, row dbq.LidarrRequest) error { var artistID pgtype.UUID err := r.pool.QueryRow(ctx, @@ -221,10 +301,7 @@ func (r *Reconciler) reconcileAlbum(ctx context.Context, q *dbq.Queries, row dbq return nil } var albumID pgtype.UUID - err := r.pool.QueryRow(ctx, - "SELECT id FROM albums WHERE mbid = $1", - *row.LidarrAlbumMbid, - ).Scan(&albumID) + err := r.pool.QueryRow(ctx, albumForRequest, *row.LidarrAlbumMbid).Scan(&albumID) if err != nil { if isNoRows(err) { return nil @@ -250,10 +327,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, - "SELECT id FROM albums WHERE mbid = $1", - *row.LidarrAlbumMbid, - ).Scan(&albumID) + err := r.pool.QueryRow(ctx, albumForRequest, *row.LidarrAlbumMbid).Scan(&albumID) if err != nil { if isNoRows(err) { return nil @@ -264,7 +338,7 @@ func (r *Reconciler) reconcileTrack(ctx context.Context, q *dbq.Queries, row dbq // Load any track from that album to set matched_track_id. var trackID pgtype.UUID err = r.pool.QueryRow(ctx, - "SELECT id FROM tracks WHERE album_id = $1 ORDER BY id LIMIT 1", + "SELECT id FROM tracks WHERE album_id = $1 AND missing_since IS NULL ORDER BY id LIMIT 1", albumID, ).Scan(&trackID) if err != nil { diff --git a/internal/lidarrrequests/reconciler_integration_test.go b/internal/lidarrrequests/reconciler_integration_test.go index 959b6067..6458f83f 100644 --- a/internal/lidarrrequests/reconciler_integration_test.go +++ b/internal/lidarrrequests/reconciler_integration_test.go @@ -152,6 +152,7 @@ func TestReconciler_MatchesAlbumByMBID(t *testing.T) { artist := seedArtist(t, q, "Test Artist", artistMBID) album := seedAlbum(t, q, artist.ID, "Test Album", albumMBID) + _ = seedTrack(t, q, album.ID, artist.ID, "Album Track", "/music/album-test/01.flac") req := seedApprovedRequestDirect(t, q, user, CreateParams{ Kind: "album", LidarrArtistMBID: artistMBID, ArtistName: "Test Artist", diff --git a/internal/lidarrrequests/reconciler_release_group_test.go b/internal/lidarrrequests/reconciler_release_group_test.go new file mode 100644 index 00000000..f8cd3a22 --- /dev/null +++ b/internal/lidarrrequests/reconciler_release_group_test.go @@ -0,0 +1,203 @@ +package lidarrrequests + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "sync" + "testing" + + "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" + "git.fabledsword.com/bvandeusen/minstrel/internal/lidarr" + "git.fabledsword.com/bvandeusen/minstrel/internal/lidarrconfig" + "git.fabledsword.com/bvandeusen/minstrel/internal/tags" +) + +// #5241: Lidarr names albums by MusicBrainz release group. A request carrying +// a group id completes against the album row whose tags named that group, +// once a track of it is on disk. +func TestReconciler_AlbumMatchesByReleaseGroup(t *testing.T) { + pool := newPool(t) + q := dbq.New(pool) + ctx := context.Background() + enableLidarrForPool(t, pool) + user := seedUser(t, pool) + + const artistMBID, release, group = "rg-artist-match", "rg-release-match", "rg-group-match" + artist := seedArtist(t, q, "RG Artist", artistMBID) + album := seedAlbum(t, q, artist.ID, "RG Album", release) + if err := q.SetAlbumReleaseGroupMbidIfNull(ctx, dbq.SetAlbumReleaseGroupMbidIfNullParams{ + ID: album.ID, ReleaseGroupMbid: nilableStr(group), + }); err != nil { + t.Fatal(err) + } + _ = seedTrack(t, q, album.ID, artist.ID, "RG One", "/music/rg-match/01.flac") + + req := seedApprovedRequestDirect(t, q, user, CreateParams{ + Kind: "album", LidarrArtistMBID: artistMBID, ArtistName: "RG Artist", + LidarrAlbumMBID: group, AlbumTitle: "RG Album", + }) + rec := NewReconciler(pool, lidarrconfig.New(pool), nil, newTestLogger(), nil) + if err := rec.tickOnce(ctx); err != nil { + t.Fatalf("tickOnce: %v", err) + } + got, err := q.GetLidarrRequestByID(ctx, req.ID) + if err != nil { + t.Fatal(err) + } + if got.Status != dbq.LidarrRequestStatusCompleted || got.MatchedAlbumID != album.ID { + t.Errorf("status %v matched %v, want completed on %v", got.Status, got.MatchedAlbumID, album.ID) + } +} + +// A re-acquisition request names an album whose row never went away. The row +// alone must not complete it: nothing has come back until a track is on disk. +func TestReconciler_AlbumWithEveryTrackMissingStaysApproved(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-missing", "rg-release-missing" + artist := seedArtist(t, q, "Missing Artist", artistMBID) + album := seedAlbum(t, q, artist.ID, "Missing Album", release) + tr := seedTrack(t, q, album.ID, artist.ID, "Gone", "/music/rg-missing/01.flac") + if _, err := pool.Exec(ctx, "UPDATE tracks SET missing_since = now() WHERE id = $1", tr.ID); err != nil { + t.Fatal(err) + } + + req := seedApprovedRequestDirect(t, q, user, CreateParams{ + Kind: "album", LidarrArtistMBID: artistMBID, ArtistName: "Missing Artist", + LidarrAlbumMBID: release, AlbumTitle: "Missing Album", + }) + rec := NewReconciler(pool, lidarrconfig.New(pool), nil, newTestLogger(), nil) + if err := rec.tickOnce(ctx); err != nil { + t.Fatalf("tickOnce: %v", err) + } + got, err := q.GetLidarrRequestByID(ctx, req.ID) + if err != nil { + t.Fatal(err) + } + if got.Status != dbq.LidarrRequestStatusApproved { + t.Errorf("status = %v, want approved while every track is missing", got.Status) + } +} + +// fakeAlbumLidarr answers album lookups only for knownGroup (Lidarr's metadata +// is keyed by release group) and records every add. +type fakeAlbumLidarr struct { + knownGroup string + mu sync.Mutex + added []string +} + +func (f *fakeAlbumLidarr) handler(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch { + case strings.HasSuffix(r.URL.Path, "/metadataprofile"): + _, _ = w.Write([]byte(`[{"id":1,"name":"Standard"}]`)) + case strings.HasSuffix(r.URL.Path, "/album/lookup"): + if r.URL.Query().Get("term") == "lidarr:"+f.knownGroup { + _, _ = w.Write([]byte(`[{"foreignAlbumId":"` + f.knownGroup + `","artist":{"id":42}}]`)) + return + } + _, _ = w.Write([]byte(`[]`)) + case r.Method == http.MethodPost && strings.HasSuffix(r.URL.Path, "/album"): + var body map[string]any + _ = json.NewDecoder(r.Body).Decode(&body) + f.mu.Lock() + f.added = append(f.added, body["foreignAlbumId"].(string)) + f.mu.Unlock() + w.WriteHeader(http.StatusCreated) + _, _ = w.Write([]byte(`{"id":9}`)) + default: + w.WriteHeader(http.StatusNotFound) + } +} + +// The ~49 requests on the deploy were stored under release ids. The reconciler +// resolves the group (MusicBrainz here, since no album row knows it), rewrites +// the request, adds it, and caches the group onto the album holding the +// release. +func TestReconciler_RepointsAReleaseIdRequestAtItsReleaseGroup(t *testing.T) { + pool := newPool(t) + q := dbq.New(pool) + ctx := context.Background() + user := seedUser(t, pool) + + const artistMBID, release, group = "rg-artist-repoint", "rg-release-repoint", "rg-group-repoint" + fake := &fakeAlbumLidarr{knownGroup: group} + srv := httptest.NewServer(http.HandlerFunc(fake.handler)) + t.Cleanup(srv.Close) + if err := lidarrconfig.New(pool).Save(ctx, lidarrconfig.Config{ + Enabled: true, BaseURL: srv.URL, APIKey: "k", + DefaultQualityProfileID: 7, DefaultRootFolderPath: "/music", + }); err != nil { + t.Fatal(err) + } + + artist := seedArtist(t, q, "Repoint Artist", artistMBID) + album := seedAlbum(t, q, artist.ID, "Repoint Album", release) + tr := seedTrack(t, q, album.ID, artist.ID, "Lost", "/music/rg-repoint/01.flac") + if _, err := pool.Exec(ctx, "UPDATE tracks SET missing_since = now() WHERE id = $1", tr.ID); err != nil { + t.Fatal(err) + } + req := seedApprovedRequestDirect(t, q, user, CreateParams{ + Kind: "album", LidarrArtistMBID: artistMBID, ArtistName: "Repoint Artist", + LidarrAlbumMBID: release, AlbumTitle: "Repoint Album", + }) + + rec := NewReconciler(pool, lidarrconfig.New(pool), + func() *lidarr.Client { return lidarr.NewClient(srv.URL, "k") }, newTestLogger(), nil) + asked := 0 + rec.releaseGroup = func(_ context.Context, id string) (string, error) { + asked++ + if id == release { + return group, nil + } + return "", tags.ErrNotFound + } + if err := rec.tickOnce(ctx); err != nil { + t.Fatalf("tickOnce: %v", err) + } + + got, err := q.GetLidarrRequestByID(ctx, req.ID) + if err != nil { + t.Fatal(err) + } + if got.LidarrAlbumMbid == nil || *got.LidarrAlbumMbid != group { + t.Errorf("request album mbid = %v, want the release group %s", got.LidarrAlbumMbid, group) + } + if !got.LidarrAddConfirmedAt.Valid { + t.Error("add not confirmed after the repoint") + } + fake.mu.Lock() + added := append([]string(nil), fake.added...) + fake.mu.Unlock() + if len(added) != 1 || added[0] != group { + t.Errorf("Lidarr adds = %v, want [%s]", added, group) + } + var cached *string + if err := pool.QueryRow(ctx, "SELECT release_group_mbid FROM albums WHERE id = $1", album.ID).Scan(&cached); err != nil { + t.Fatal(err) + } + if cached == nil || *cached != group { + t.Errorf("album release group = %v, want %s cached", cached, group) + } + if got.Status != dbq.LidarrRequestStatusApproved { + t.Errorf("status = %v, want approved until a track is back", got.Status) + } + + // The next tick finds the group on the request and the album: no second + // MusicBrainz question. + before := asked + if err := rec.tickOnce(ctx); err != nil { + t.Fatalf("second tick: %v", err) + } + if asked != before { + t.Errorf("MusicBrainz asked again on the next tick (%d -> %d)", before, asked) + } +} diff --git a/internal/reacquisition/sweeper.go b/internal/reacquisition/sweeper.go index e14d1820..16875b40 100644 --- a/internal/reacquisition/sweeper.go +++ b/internal/reacquisition/sweeper.go @@ -13,6 +13,7 @@ import ( "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" "git.fabledsword.com/bvandeusen/minstrel/internal/lidarrrequests" + "git.fabledsword.com/bvandeusen/minstrel/internal/tags" ) // requestCreator is the slice of lidarrrequests.Service the sweeper needs, @@ -37,6 +38,10 @@ type Sweeper struct { requests requestCreator logger *slog.Logger tick time.Duration + // releaseGroup names the MusicBrainz release group of a release id, for an + // album whose tags never carried one (#5241). Lidarr knows albums only by + // release group. A field so tests can stand in for MusicBrainz. + releaseGroup func(ctx context.Context, releaseMBID string) (string, error) } // NewSweeper constructs a Sweeper. The tick is deliberately coarse: the @@ -49,11 +54,12 @@ func NewSweeper( logger *slog.Logger, ) *Sweeper { return &Sweeper{ - pool: pool, - settings: settings, - requests: requests, - logger: logger, - tick: 1 * time.Hour, + pool: pool, + settings: settings, + requests: requests, + logger: logger, + tick: 1 * time.Hour, + releaseGroup: tags.ReleaseGroupForRelease, } } @@ -80,6 +86,10 @@ type PassResult struct { Approved int // of those, sent on to Lidarr GaveUp int // albums that spent their attempt budget Unnameable int64 // albums with missing files but no MBID to ask for + // Unresolved: albums skipped this pass because MusicBrainz could not name + // their release group (switched off, or no such release). Lidarr cannot be + // asked for them, and no attempt is spent; the next pass tries again. + Unresolved int } // SweepOnce runs one pass. Exported so the admin surface can offer a "run @@ -160,11 +170,23 @@ func (s *Sweeper) attempt( return nil } + // Lidarr names albums by release group; albums.mbid is the release. + group, err := s.albumReleaseGroup(ctx, q, album) + if err != nil { + if errors.Is(err, tags.ErrNotFound) { + res.Unresolved++ + s.logger.Info("reacquisition: no MusicBrainz release group for album; skipped", + "album", album.AlbumTitle, "release_mbid", *album.AlbumMbid) + return nil + } + return fmt.Errorf("release group: %w", err) + } + req, err := s.requests.Create(ctx, adminID, lidarrrequests.CreateParams{ Kind: "album", LidarrArtistMBID: *album.ArtistMbid, ArtistName: album.ArtistName, - LidarrAlbumMBID: *album.AlbumMbid, + LidarrAlbumMBID: group, AlbumTitle: album.AlbumTitle, }) if err != nil { @@ -212,10 +234,31 @@ func (s *Sweeper) attempt( return nil } +// albumReleaseGroup is the album's release group: the one its tags carried, +// else MusicBrainz's answer for its release id, cached onto the album so the +// next pass (and request completion) need not ask again. Returns +// tags.ErrNotFound when MusicBrainz cannot name it. +func (s *Sweeper) albumReleaseGroup(ctx context.Context, q *dbq.Queries, album dbq.ListAlbumsDueReacquisitionRow) (string, error) { + if album.AlbumReleaseGroupMbid != nil && *album.AlbumReleaseGroupMbid != "" { + return *album.AlbumReleaseGroupMbid, nil + } + group, err := s.releaseGroup(ctx, *album.AlbumMbid) + if err != nil { + return "", err + } + if cerr := q.SetAlbumReleaseGroupMbidIfNull(ctx, dbq.SetAlbumReleaseGroupMbidIfNullParams{ + ID: album.AlbumID, + ReleaseGroupMbid: &group, + }); cerr != nil { + s.logger.Warn("reacquisition: cache release group failed", "album", album.AlbumTitle, "err", cerr) + } + return group, nil +} + func (s *Sweeper) logSummary(res PassResult) { // Silence when a pass did nothing at all — this runs hourly forever, and // an unconditional line would bury the passes that mattered. - if res.Requested == 0 && res.Cleared == 0 && res.GaveUp == 0 { + if res.Requested == 0 && res.Cleared == 0 && res.GaveUp == 0 && res.Unresolved == 0 { return } s.logger.Info("reacquisition: sweep", @@ -224,5 +267,6 @@ func (s *Sweeper) logSummary(res PassResult) { "gave_up", res.GaveUp, "cleared", res.Cleared, "unnameable", res.Unnameable, + "unresolved", res.Unresolved, ) } diff --git a/internal/reacquisition/sweeper_test.go b/internal/reacquisition/sweeper_test.go new file mode 100644 index 00000000..d8833a69 --- /dev/null +++ b/internal/reacquisition/sweeper_test.go @@ -0,0 +1,163 @@ +package reacquisition + +import ( + "context" + "io" + "log/slog" + "os" + "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/dbtest" + "git.fabledsword.com/bvandeusen/minstrel/internal/lidarrconfig" + "git.fabledsword.com/bvandeusen/minstrel/internal/lidarrrequests" + "git.fabledsword.com/bvandeusen/minstrel/internal/tags" +) + +func newSweepPool(t *testing.T) *pgxpool.Pool { + t.Helper() + if testing.Short() { + t.Skip("skipping integration test in -short mode") + } + dsn := os.Getenv("MINSTREL_TEST_DATABASE_URL") + if dsn == "" { + t.Skip("MINSTREL_TEST_DATABASE_URL not set") + } + if err := db.Migrate(dsn, slog.New(slog.NewTextHandler(io.Discard, nil))); err != nil { + t.Fatalf("migrate: %v", err) + } + pool, err := pgxpool.New(context.Background(), dsn) + if err != nil { + t.Fatalf("pool: %v", err) + } + t.Cleanup(pool.Close) + dbtest.ResetDB(t, pool) + if _, err := pool.Exec(context.Background(), "DELETE FROM lidarr_requests"); err != nil { + t.Fatalf("reset requests: %v", err) + } + return pool +} + +// lostAlbum seeds an album whose only track has been missing for two days, +// named by MusicBrainz release id and (when group is set) release group. +func lostAlbum(t *testing.T, pool *pgxpool.Pool, title, release, group string) pgtype.UUID { + t.Helper() + ctx := context.Background() + q := dbq.New(pool) + artistMBID := "artist-" + release + ar, err := q.UpsertArtist(ctx, dbq.UpsertArtistParams{Name: title + " Artist", SortName: title, Mbid: &artistMBID}) + if err != nil { + t.Fatal(err) + } + params := dbq.UpsertAlbumParams{Title: title, SortTitle: title, ArtistID: ar.ID, Mbid: &release} + if group != "" { + params.ReleaseGroupMbid = &group + } + al, err := q.UpsertAlbum(ctx, params) + if err != nil { + t.Fatal(err) + } + tr, err := q.UpsertTrack(ctx, dbq.UpsertTrackParams{ + Title: "Lost", AlbumID: al.ID, ArtistID: ar.ID, DurationMs: 180000, + FilePath: "/music/" + release + "/01.flac", FileSize: 1, FileFormat: "flac", + }) + if err != nil { + t.Fatal(err) + } + if _, err := pool.Exec(ctx, "UPDATE tracks SET missing_since = now() - interval '2 days' WHERE id = $1", tr.ID); err != nil { + t.Fatal(err) + } + return al.ID +} + +// #5241: re-acquisition asks Lidarr for the album's release group, never its +// release id. A tag-supplied group is used as is; otherwise MusicBrainz names +// it and the answer is cached onto the album; an album MusicBrainz cannot +// name is skipped without spending an attempt. +func TestSweep_RequestsAlbumsByReleaseGroup_Integration(t *testing.T) { + pool := newSweepPool(t) + ctx := context.Background() + logger := slog.New(slog.NewTextHandler(io.Discard, nil)) + q := dbq.New(pool) + + if _, err := q.CreateUser(ctx, dbq.CreateUserParams{ + Username: dbtest.TestUserPrefix + "sweepadmin", PasswordHash: "x", ApiTokenHash: "x", IsAdmin: true, + }); err != nil { + t.Fatal(err) + } + settings, err := NewSettingsService(ctx, pool, logger) + if err != nil { + t.Fatal(err) + } + cfg := Defaults + cfg.AutoApprove = false + if _, err := settings.Set(ctx, cfg); err != nil { + t.Fatal(err) + } + + tagged := lostAlbum(t, pool, "Tagged", "release-tagged", "group-tagged") + resolved := lostAlbum(t, pool, "Resolved", "release-resolved", "") + unknown := lostAlbum(t, pool, "Unknown", "release-unknown", "") + + requests := lidarrrequests.NewService(pool, lidarrconfig.New(pool), nil, nil) + s := NewSweeper(pool, settings, requests, logger) + asked := map[string]int{} + s.releaseGroup = func(_ context.Context, release string) (string, error) { + asked[release]++ + if release == "release-resolved" { + return "group-resolved", nil + } + return "", tags.ErrNotFound + } + if err := s.SweepOnce(ctx); err != nil { + t.Fatalf("sweep: %v", err) + } + + requested := func(album pgtype.UUID) (string, bool) { + t.Helper() + var mbid *string + err := pool.QueryRow(ctx, ` +SELECT lr.lidarr_album_mbid + FROM missing_reacquisitions r + JOIN lidarr_requests lr ON lr.id = r.last_request_id + WHERE r.album_id = $1`, album).Scan(&mbid) + if err != nil || mbid == nil { + return "", false + } + return *mbid, true + } + if got, ok := requested(tagged); !ok || got != "group-tagged" { + t.Errorf("tagged album requested as %q (ok=%v), want group-tagged", got, ok) + } + if asked["release-tagged"] != 0 { + t.Error("MusicBrainz asked about an album whose tags named its group") + } + if got, ok := requested(resolved); !ok || got != "group-resolved" { + t.Errorf("resolved album requested as %q (ok=%v), want group-resolved", got, ok) + } + var cached *string + if err := pool.QueryRow(ctx, "SELECT release_group_mbid FROM albums WHERE id = $1", resolved).Scan(&cached); err != nil { + t.Fatal(err) + } + if cached == nil || *cached != "group-resolved" { + t.Errorf("resolved group not cached on the album: %v", cached) + } + var attempts int + if err := pool.QueryRow(ctx, "SELECT count(*) FROM missing_reacquisitions WHERE album_id = $1", unknown).Scan(&attempts); err != nil { + t.Fatal(err) + } + if attempts != 0 { + t.Errorf("unresolvable album spent an attempt (%d state rows)", attempts) + } + var stray int + if err := pool.QueryRow(ctx, "SELECT count(*) FROM lidarr_requests WHERE lidarr_album_mbid LIKE 'release-%'").Scan(&stray); err != nil { + t.Fatal(err) + } + if stray != 0 { + t.Errorf("%d request(s) named an album by its release id", stray) + } +}