backend/database/sql/queries/albums.sql

@@ -40,7 +40,8 @@ WHERE id = ? AND (mbid IS NULL OR mbid = '');
 -- name: GetAlbumsWithPendingReleaseMBID :many
 SELECT id, pending_release_mbid FROM albums
 WHERE pending_release_mbid IS NOT NULL AND pending_release_mbid != ''
-  AND (mbid IS NULL OR mbid = '');
+  AND (mbid IS NULL OR mbid = '')
+LIMIT ?;
 
 -- name: DeleteAlbum :exec
 DELETE FROM albums WHERE id = ?;

backend/database/sql/sqlcgen/albums.sql.go

@@ -331,6 +331,7 @@ const getAlbumsWithPendingReleaseMBID = `-- name: GetAlbumsWithPendingReleaseMBI
 SELECT id, pending_release_mbid FROM albums
 WHERE pending_release_mbid IS NOT NULL AND pending_release_mbid != ''
   AND (mbid IS NULL OR mbid = '')
+LIMIT ?
 `
 
 type GetAlbumsWithPendingReleaseMBIDRow struct {
@@ -338,8 +339,8 @@ type GetAlbumsWithPendingReleaseMBIDRow struct {
 	PendingReleaseMbid sql.NullString
 }
 
-func (q *Queries) GetAlbumsWithPendingReleaseMBID(ctx context.Context) ([]GetAlbumsWithPendingReleaseMBIDRow, error) {
-	rows, err := q.db.QueryContext(ctx, getAlbumsWithPendingReleaseMBID)
+func (q *Queries) GetAlbumsWithPendingReleaseMBID(ctx context.Context, limit int64) ([]GetAlbumsWithPendingReleaseMBIDRow, error) {
+	rows, err := q.db.QueryContext(ctx, getAlbumsWithPendingReleaseMBID, limit)
 	if err != nil {
 		return nil, err
 	}

backend/explore/explore.go

@@ -2,6 +2,7 @@ package explore
 
 import (
 	"context"
+	"database/sql"
 	"log/slog"
 	"math"
 	"sort"
@@ -12,6 +13,7 @@ import (
 	"golang.org/x/sync/singleflight"
 
 	"yellowjacket/backend/database"
+	"yellowjacket/backend/database/sql/sqlcgen"
 	"yellowjacket/backend/events"
 	"yellowjacket/backend/jobs"
 )
@@ -279,37 +281,32 @@ func (e *Service) BackfillReleaseGroupMBIDs() {
 	go e.backfillReleaseGroupMBIDs(e.ctx)
 }
 
-func (e *Service) backfillReleaseGroupMBIDs(ctx context.Context) {
-	rows, err := e.db.QueryContext(
-		"SELECT id, pending_release_mbid FROM release_groups "+
-			"WHERE (mbid IS NULL OR mbid = '') "+
-			"AND pending_release_mbid IS NOT NULL AND pending_release_mbid != '' "+
-			"LIMIT ?",
-		releaseGroupMBIDBackfillMaxPerRun,
+// pendingReleaseMBIDs is the albums this pass has work to do on.
+//
+// It is separate from the pass, and returns its error rather than
+// logging it, so that a test can assert the statement runs against the
+// real schema. That is not a general preference -- it is this
+// statement's history: it named `release_groups`, a table plan 013
+// renamed to `albums`, so it failed on every launch since e7748f1 and
+// the pass returned quietly having done nothing. A test of the pass
+// as a whole cannot see that, because a query error and an empty
+// library are the same early return.
+func (e *Service) pendingReleaseMBIDs(
+	ctx context.Context,
+) ([]sqlcgen.GetAlbumsWithPendingReleaseMBIDRow, error) {
+	return e.db.ReadQueries.GetAlbumsWithPendingReleaseMBID(
+		ctx, releaseGroupMBIDBackfillMaxPerRun,
 	)
+}
+
+func (e *Service) backfillReleaseGroupMBIDs(ctx context.Context) {
+	pending, err := e.pendingReleaseMBIDs(ctx)
 	if err != nil {
 		e.logger.Warn("release-group mbid backfill: query failed", "error", err)
 
 		return
 	}
 
-	type pendingRow struct {
-		id          int64
-		releaseMBID string
-	}
-
-	var pending []pendingRow
-
-	for rows.Next() {
-		var p pendingRow
-
-		if err := rows.Scan(&p.id, &p.releaseMBID); err == nil {
-			pending = append(pending, p)
-		}
-	}
-
-	_ = rows.Close()
-
 	if len(pending) == 0 {
 		return
 	}
@@ -335,7 +332,7 @@ func (e *Service) backfillReleaseGroupMBIDs(ctx context.Context) {
 
 		job.progress(i, len(pending))
 
-		release, err := e.mb.LookupRelease(ctx, p.releaseMBID)
+		release, err := e.mb.LookupRelease(ctx, p.PendingReleaseMbid.String)
 		if err != nil || release.ReleaseGroupMBID == "" {
 			// Left alone rather than cleared: LookupRelease caches its
 			// answer (success or a release with no group) for 7 days,
@@ -344,12 +341,19 @@ func (e *Service) backfillReleaseGroupMBIDs(ctx context.Context) {
 			continue
 		}
 
-		_, err = e.db.ExecContext(
-			"UPDATE release_groups SET mbid = ?, pending_release_mbid = NULL "+
-				"WHERE id = ? AND (mbid IS NULL OR mbid = '')",
-			release.ReleaseGroupMBID, p.id,
-		)
-		if err != nil {
+		// The writer, not ReadQueries: an UPDATE issued on the
+		// query-only pool fails at runtime with "attempt to write a
+		// readonly database".
+		if err := e.db.Queries.ResolveAlbumPendingReleaseMBID(
+			ctx,
+			sqlcgen.ResolveAlbumPendingReleaseMBIDParams{
+				Mbid: sql.NullString{
+					String: release.ReleaseGroupMBID,
+					Valid:  true,
+				},
+				ID: p.ID,
+			},
+		); err != nil {
 			e.logger.Warn("release-group mbid backfill: update failed", "error", err)
 		}
 	}

backend/explore/pendingreleasembid_test.go

@@ -0,0 +1,250 @@
+package explore
+
+import (
+	"database/sql"
+	"log/slog"
+	"strconv"
+	"testing"
+
+	"yellowjacket/backend/database"
+	"yellowjacket/backend/database/sql/sqlcgen"
+)
+
+// The release-group MBID backfill queried `release_groups`, a table
+// plan 013 renamed to `albums`, so it failed on its first statement on
+// every launch from e7748f1 until #189 -- and the pass swallowed that,
+// because a query error and an empty library are the same early
+// return. Nothing noticed for two reasons worth keeping in mind:
+//
+//   - the statement was **raw SQL**, so sqlc never read it. Every other
+//     statement in the repo was renamed by the same change because sqlc
+//     reads sql/schemas/ and cannot generate against a table that is not
+//     declared. The two sqlc queries this now calls were written by 013
+//     and left uncalled.
+//   - it needs no network and no fixture library to reproduce. The
+//     failure is at prepare time.
+
+// seedPendingAlbum inserts an album whose files carried a release MBID
+// but no release-group MBID, which is what `library.updateMBIDs`
+// leaves behind for this pass to resolve.
+func seedPendingAlbum(
+	t *testing.T,
+	db *database.DB,
+	name, pendingMBID string,
+) int64 {
+	t.Helper()
+
+	res, err := db.ExecContext(
+		"INSERT INTO albums (name, artist_credit, pending_release_mbid) "+
+			"VALUES (?, ?, ?)",
+		name, "Test Artist", pendingMBID,
+	)
+	if err != nil {
+		t.Fatalf("insert albums row: %v", err)
+	}
+
+	id, err := res.LastInsertId()
+	if err != nil {
+		t.Fatalf("last insert id: %v", err)
+	}
+
+	return id
+}
+
+func newPendingTestService(db *database.DB) *Service {
+	return &Service{db: db, logger: slog.Default()}
+}
+
+// TestPendingReleaseMBIDsRunsAgainstTheRealSchema is the regression.
+//
+// It asserts the statement *runs*, which is the whole of what was
+// broken: against the old raw SQL this returns
+// "no such table: release_groups" rather than a row.
+func TestPendingReleaseMBIDsRunsAgainstTheRealSchema(t *testing.T) {
+	t.Parallel()
+
+	db := database.NewTestDB(t)
+	e := newPendingTestService(db)
+
+	want := seedPendingAlbum(t, db, "Pending Album", "release-mbid-1")
+
+	pending, err := e.pendingReleaseMBIDs(db.Ctx)
+	if err != nil {
+		t.Fatalf("the backfill's query failed: %v", err)
+	}
+
+	if len(pending) != 1 {
+		t.Fatalf("got %d pending albums, want 1", len(pending))
+	}
+
+	if pending[0].ID != want {
+		t.Errorf("got album id %d, want %d", pending[0].ID, want)
+	}
+
+	if got := pending[0].PendingReleaseMbid.String; got != "release-mbid-1" {
+		t.Errorf("got pending mbid %q, want %q", got, "release-mbid-1")
+	}
+}
+
+// TestOnlyUnresolvedAlbumsAreReturned pins the two conditions that make
+// the pass idempotent, since between them they are what stops it doing
+// the same MusicBrainz lookups on every launch forever.
+func TestOnlyUnresolvedAlbumsAreReturned(t *testing.T) {
+	t.Parallel()
+
+	db := database.NewTestDB(t)
+	e := newPendingTestService(db)
+
+	pendingID := seedPendingAlbum(t, db, "Still Pending", "release-mbid-1")
+
+	// Already resolved: it has a real MBID, so there is nothing to
+	// look up even though a marker is still sitting on it.
+	resolved := seedPendingAlbum(t, db, "Already Resolved", "release-mbid-2")
+	if err := db.Queries.SetAlbumMBID(db.Ctx, sqlcgen.SetAlbumMBIDParams{
+		Mbid: sql.NullString{String: "rg-mbid", Valid: true},
+		ID:   resolved,
+	}); err != nil {
+		t.Fatalf("set album mbid: %v", err)
+	}
+
+	// Never had a release MBID to resolve in the first place, which is
+	// most of a library.
+	seedPendingAlbum(t, db, "Nothing Pending", "")
+
+	pending, err := e.pendingReleaseMBIDs(db.Ctx)
+	if err != nil {
+		t.Fatalf("the backfill's query failed: %v", err)
+	}
+
+	if len(pending) != 1 || pending[0].ID != pendingID {
+		t.Fatalf(
+			"got %d albums %v, want only the unresolved one (%d)",
+			len(pending), pending, pendingID,
+		)
+	}
+}
+
+// TestResolvingClearsTheMarker is the other half: once the lookup has
+// answered, the album must stop being a candidate, or the pass repeats
+// the same live MusicBrainz call on every launch.
+func TestResolvingClearsTheMarker(t *testing.T) {
+	t.Parallel()
+
+	db := database.NewTestDB(t)
+	e := newPendingTestService(db)
+
+	id := seedPendingAlbum(t, db, "Pending Album", "release-mbid-1")
+
+	// The writer, deliberately: this is an UPDATE, and the read pool
+	// would refuse it at runtime.
+	if err := db.Queries.ResolveAlbumPendingReleaseMBID(
+		db.Ctx,
+		sqlcgen.ResolveAlbumPendingReleaseMBIDParams{
+			Mbid: sql.NullString{String: "resolved-rg-mbid", Valid: true},
+			ID:   id,
+		},
+	); err != nil {
+		t.Fatalf("resolve pending release mbid: %v", err)
+	}
+
+	pending, err := e.pendingReleaseMBIDs(db.Ctx)
+	if err != nil {
+		t.Fatalf("the backfill's query failed: %v", err)
+	}
+
+	if len(pending) != 0 {
+		t.Fatalf("a resolved album is still a candidate: %v", pending)
+	}
+
+	album, err := db.ReadQueries.GetAlbum(db.Ctx, id)
+	if err != nil {
+		t.Fatalf("get album: %v", err)
+	}
+
+	if album.Mbid.String != "resolved-rg-mbid" {
+		t.Errorf("album mbid = %q, want the resolved one", album.Mbid.String)
+	}
+
+	if album.PendingReleaseMbid.Valid &&
+		album.PendingReleaseMbid.String != "" {
+		t.Errorf(
+			"the pending marker survived as %q",
+			album.PendingReleaseMbid.String,
+		)
+	}
+}
+
+// TestAResolvedMBIDIsNeverOverwritten covers the guard in the UPDATE.
+//
+// The pass runs against rows it read earlier, and a rescan can resolve
+// an album from its tags in between -- a real MBID from the file must
+// win over one this pass inferred from a release.
+func TestAResolvedMBIDIsNeverOverwritten(t *testing.T) {
+	t.Parallel()
+
+	db := database.NewTestDB(t)
+
+	id := seedPendingAlbum(t, db, "Pending Album", "release-mbid-1")
+
+	if err := db.Queries.SetAlbumMBID(db.Ctx, sqlcgen.SetAlbumMBIDParams{
+		Mbid: sql.NullString{String: "from-the-tags", Valid: true},
+		ID:   id,
+	}); err != nil {
+		t.Fatalf("set album mbid: %v", err)
+	}
+
+	if err := db.Queries.ResolveAlbumPendingReleaseMBID(
+		db.Ctx,
+		sqlcgen.ResolveAlbumPendingReleaseMBIDParams{
+			Mbid: sql.NullString{String: "from-the-backfill", Valid: true},
+			ID:   id,
+		},
+	); err != nil {
+		t.Fatalf("resolve pending release mbid: %v", err)
+	}
+
+	album, err := db.ReadQueries.GetAlbum(db.Ctx, id)
+	if err != nil {
+		t.Fatalf("get album: %v", err)
+	}
+
+	if album.Mbid.String != "from-the-tags" {
+		t.Errorf(
+			"album mbid = %q, want the tagged one to have won",
+			album.Mbid.String,
+		)
+	}
+}
+
+// TestThePassIsBounded checks the LIMIT.
+//
+// Each row costs a live MusicBrainz lookup on a 1 req/s limiter shared
+// with every page the user can open, so an unbounded read is a run that
+// lasts as long as the library is untagged. The sqlc query 013 wrote
+// had no LIMIT; the raw statement it was replacing did.
+func TestThePassIsBounded(t *testing.T) {
+	t.Parallel()
+
+	db := database.NewTestDB(t)
+	e := newPendingTestService(db)
+
+	for i := range releaseGroupMBIDBackfillMaxPerRun + 10 {
+		seedPendingAlbum(
+			t, db,
+			"Album "+string(rune('A'+i%26))+strconv.Itoa(i),
+			"release-mbid-"+strconv.Itoa(i),
+		)
+	}
+
+	pending, err := e.pendingReleaseMBIDs(db.Ctx)
+	if err != nil {
+		t.Fatalf("the backfill's query failed: %v", err)
+	}
+
+	if len(pending) != releaseGroupMBIDBackfillMaxPerRun {
+		t.Errorf(
+			"got %d albums, want the run bounded at %d",
+			len(pending), releaseGroupMBIDBackfillMaxPerRun,
+		)
+	}
+}