diff --git a/internal/db/dbq/reacquisition.sql.go b/internal/db/dbq/reacquisition.sql.go index d5a05760..a8078c59 100644 --- a/internal/db/dbq/reacquisition.sql.go +++ b/internal/db/dbq/reacquisition.sql.go @@ -17,7 +17,7 @@ SELECT COUNT(DISTINCT albums.id)::bigint JOIN artists ON artists.id = albums.artist_id JOIN tracks ON tracks.album_id = albums.id WHERE tracks.missing_since IS NOT NULL - AND (albums.mbid IS NULL OR artists.mbid IS NULL) + AND ((albums.mbid IS NULL AND albums.release_group_mbid IS NULL) OR artists.mbid IS NULL) ` // Albums with missing files that can never be wanted automatically because @@ -105,7 +105,7 @@ SELECT albums.id AS album_id, JOIN tracks ON tracks.album_id = albums.id WHERE tracks.missing_since IS NOT NULL AND tracks.missing_since <= now() - make_interval(hours => $1::int) - AND albums.mbid IS NOT NULL + AND (albums.mbid IS NOT NULL OR albums.release_group_mbid IS NOT NULL) AND artists.mbid IS NOT NULL GROUP BY albums.id, albums.title, albums.mbid, albums.release_group_mbid, artists.name, artists.mbid @@ -141,8 +141,11 @@ type ListAlbumsDueReacquisitionRow struct { // - it still has at least one track whose file has been missing longer than // the grace window. Measured on missing_since, which reconcile never // re-stamps (#2523), so it is a genuine "gone since" clock; -// - MusicBrainz can name it: the album and its artist have MBIDs. Albums -// failing this are counted separately (CountAlbumsMissingWithoutMbid); +// - MusicBrainz can name it: the artist has an MBID, and the album a +// release MBID or a release group. The release MBID is only ever used to +// find the group, and 238 albums matched to a group carried no release +// id, so their losses were never wanted back (#6167). Albums failing this +// are counted separately (CountAlbumsMissingWithoutMbid); // - it is on the wanted list (#5567): an album taken off the list, or // dropped into the library by hand, is not fetched back; // - no want already covers it: none open, and none held since its files diff --git a/internal/db/queries/reacquisition.sql b/internal/db/queries/reacquisition.sql index fbae4a29..5754b908 100644 --- a/internal/db/queries/reacquisition.sql +++ b/internal/db/queries/reacquisition.sql @@ -21,8 +21,11 @@ RETURNING *; -- * it still has at least one track whose file has been missing longer than -- the grace window. Measured on missing_since, which reconcile never -- re-stamps (#2523), so it is a genuine "gone since" clock; --- * MusicBrainz can name it: the album and its artist have MBIDs. Albums --- failing this are counted separately (CountAlbumsMissingWithoutMbid); +-- * MusicBrainz can name it: the artist has an MBID, and the album a +-- release MBID or a release group. The release MBID is only ever used to +-- find the group, and 238 albums matched to a group carried no release +-- id, so their losses were never wanted back (#6167). Albums failing this +-- are counted separately (CountAlbumsMissingWithoutMbid); -- * it is on the wanted list (#5567): an album taken off the list, or -- dropped into the library by hand, is not fetched back; -- * no want already covers it: none open, and none held since its files @@ -41,7 +44,7 @@ SELECT albums.id AS album_id, JOIN tracks ON tracks.album_id = albums.id WHERE tracks.missing_since IS NOT NULL AND tracks.missing_since <= now() - make_interval(hours => sqlc.arg(grace_hours)::int) - AND albums.mbid IS NOT NULL + AND (albums.mbid IS NOT NULL OR albums.release_group_mbid IS NOT NULL) AND artists.mbid IS NOT NULL GROUP BY albums.id, albums.title, albums.mbid, albums.release_group_mbid, artists.name, artists.mbid @@ -64,7 +67,7 @@ SELECT COUNT(DISTINCT albums.id)::bigint JOIN artists ON artists.id = albums.artist_id JOIN tracks ON tracks.album_id = albums.id WHERE tracks.missing_since IS NOT NULL - AND (albums.mbid IS NULL OR artists.mbid IS NULL); + AND ((albums.mbid IS NULL AND albums.release_group_mbid IS NULL) OR artists.mbid IS NULL); -- name: WantMissingAlbum :one -- Records an album with missing files as wanted. A held want for the same diff --git a/internal/reacquisition/sweeper.go b/internal/reacquisition/sweeper.go index 5d67d5e2..5ad73cc8 100644 --- a/internal/reacquisition/sweeper.go +++ b/internal/reacquisition/sweeper.go @@ -103,7 +103,8 @@ func (s *Sweeper) SweepOnce(ctx context.Context) error { // want records one album as wanted under its release group. func (s *Sweeper) want(ctx context.Context, q *dbq.Queries, album dbq.ListAlbumsDueReacquisitionRow, res *PassResult) error { // The query already filters these out; belt and braces. - if album.AlbumMbid == nil || album.ArtistMbid == nil { + hasGroup := album.AlbumReleaseGroupMbid != nil && *album.AlbumReleaseGroupMbid != "" + if (album.AlbumMbid == nil && !hasGroup) || album.ArtistMbid == nil { return nil } group, err := s.albumReleaseGroup(ctx, q, album) diff --git a/internal/reacquisition/sweeper_test.go b/internal/reacquisition/sweeper_test.go index 51a974f6..fbd4b3ff 100644 --- a/internal/reacquisition/sweeper_test.go +++ b/internal/reacquisition/sweeper_test.go @@ -38,17 +38,21 @@ func newSweepPool(t *testing.T) *pgxpool.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. +// named by MusicBrainz release id (when release is set) and release group +// (when group is set). 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 + artistMBID := "artist-" + title 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} + params := dbq.UpsertAlbumParams{Title: title, SortTitle: title, ArtistID: ar.ID} + if release != "" { + params.Mbid = &release + } if group != "" { params.ReleaseGroupMbid = &group } @@ -58,7 +62,7 @@ func lostAlbum(t *testing.T, pool *pgxpool.Pool, title, release, group string) p } 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", + FilePath: "/music/" + title + "/01.flac", FileSize: 1, FileFormat: "flac", }) if err != nil { t.Fatal(err) @@ -99,7 +103,11 @@ func TestSweep_WantsBackOnlyAlbumsOnTheList_Integration(t *testing.T) { resolved := lostAlbum(t, pool, "Resolved", "release-resolved", "") unknown := lostAlbum(t, pool, "Unknown", "release-unknown", "") _ = lostAlbum(t, pool, "Off The List", "release-off", "group-off") + // Matched to a group with no release id: 238 such albums on the operator's + // library were never wanted back (#6167). + groupOnly := lostAlbum(t, pool, "Group Only", "", "group-only") heldWant(t, pool, "group-tagged", tagged, "3 days") + heldWant(t, pool, "group-only", groupOnly, "3 days") heldWant(t, pool, "group-resolved", resolved, "3 days") heldWant(t, pool, "group-unknown", unknown, "3 days") @@ -128,6 +136,7 @@ func TestSweep_WantsBackOnlyAlbumsOnTheList_Integration(t *testing.T) { } wantFor("group-tagged", tagged) wantFor("group-resolved", resolved) + wantFor("group-only", groupOnly) if w, _ := q.GetWantByReleaseGroup(ctx, "group-unknown"); !w.FulfilledAt.Valid { t.Error("an album MusicBrainz cannot name was reopened under the wrong group") }