From 13a7a3629ad5729ba0ac6f67e7d59f5877255721 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Tue, 6 Oct 2026 21:22:05 -0400 Subject: [PATCH] fix(library): MBID backfills skip tracks whose files are missing (#5139) The track backfill listed every track with a NULL mbid, missing or not, so each scan tried to open every missing file and logged an "open failed" warning per track. Nothing ever healed. The album backfill could pick a missing track as the one to read, and since that pass is capped per scan, albums stuck that way were retried ahead of the rest every time. Both now read only tracks still on disk; an album with none left is skipped until a scan finds its files again. Co-Authored-By: Claude Opus 5.5 --- internal/db/dbq/albums.sql.go | 5 +- internal/db/dbq/tracks.sql.go | 3 ++ internal/db/queries/albums.sql | 5 +- internal/db/queries/tracks.sql | 3 ++ internal/library/mbidbackfill_test.go | 71 +++++++++++++++++++++++++++ 5 files changed, 85 insertions(+), 2 deletions(-) create mode 100644 internal/library/mbidbackfill_test.go diff --git a/internal/db/dbq/albums.sql.go b/internal/db/dbq/albums.sql.go index 98b07b4a..04ce65e6 100644 --- a/internal/db/dbq/albums.sql.go +++ b/internal/db/dbq/albums.sql.go @@ -552,6 +552,7 @@ SELECT a.id AS album_id, SELECT file_path FROM tracks WHERE album_id = a.id + AND missing_since IS NULL ORDER BY disc_number NULLS LAST, track_number NULLS LAST, id LIMIT 1 ) t ON true @@ -569,7 +570,9 @@ type ListAlbumsMissingMbidWithTrackRow struct { // 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 -// supplied by caller for batching/progress purposes. +// supplied by caller for batching/progress purposes. The track read is +// one still on disk; an album with none left is skipped until a scan +// finds its files again (#5139). func (q *Queries) ListAlbumsMissingMbidWithTrack(ctx context.Context, limit int32) ([]ListAlbumsMissingMbidWithTrackRow, error) { rows, err := q.db.Query(ctx, listAlbumsMissingMbidWithTrack, limit) if err != nil { diff --git a/internal/db/dbq/tracks.sql.go b/internal/db/dbq/tracks.sql.go index 4cfe2964..638a5687 100644 --- a/internal/db/dbq/tracks.sql.go +++ b/internal/db/dbq/tracks.sql.go @@ -668,6 +668,7 @@ const listTracksMissingMbidWithPath = `-- name: ListTracksMissingMbidWithPath :m SELECT id, file_path FROM tracks WHERE mbid IS NULL + AND missing_since IS NULL ORDER BY id LIMIT $1 ` @@ -679,6 +680,8 @@ type ListTracksMissingMbidWithPathRow struct { // Track recording-MBID backfill: tracks with NULL mbid that still have // a file to re-read. $1 caps the batch (mirrors the album backfill). +// Missing tracks are skipped: there is no file, and opening it anyway +// logged a warning per track on every scan (#5139). func (q *Queries) ListTracksMissingMbidWithPath(ctx context.Context, limit int32) ([]ListTracksMissingMbidWithPathRow, error) { rows, err := q.db.Query(ctx, listTracksMissingMbidWithPath, limit) if err != nil { diff --git a/internal/db/queries/albums.sql b/internal/db/queries/albums.sql index 6f87dfc8..c9b88fa9 100644 --- a/internal/db/queries/albums.sql +++ b/internal/db/queries/albums.sql @@ -145,7 +145,9 @@ UPDATE albums -- 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 --- supplied by caller for batching/progress purposes. +-- supplied by caller for batching/progress purposes. The track read is +-- one still on disk; an album with none left is skipped until a scan +-- finds its files again (#5139). SELECT a.id AS album_id, a.artist_id AS artist_id, a.title AS title, @@ -155,6 +157,7 @@ SELECT a.id AS album_id, SELECT file_path FROM tracks WHERE album_id = a.id + AND missing_since IS NULL ORDER BY disc_number NULLS LAST, track_number NULLS LAST, id LIMIT 1 ) t ON true diff --git a/internal/db/queries/tracks.sql b/internal/db/queries/tracks.sql index e8075668..6d0b179e 100644 --- a/internal/db/queries/tracks.sql +++ b/internal/db/queries/tracks.sql @@ -26,9 +26,12 @@ RETURNING *; -- name: ListTracksMissingMbidWithPath :many -- Track recording-MBID backfill: tracks with NULL mbid that still have -- a file to re-read. $1 caps the batch (mirrors the album backfill). +-- Missing tracks are skipped: there is no file, and opening it anyway +-- logged a warning per track on every scan (#5139). SELECT id, file_path FROM tracks WHERE mbid IS NULL + AND missing_since IS NULL ORDER BY id LIMIT $1; diff --git a/internal/library/mbidbackfill_test.go b/internal/library/mbidbackfill_test.go new file mode 100644 index 00000000..a645abe4 --- /dev/null +++ b/internal/library/mbidbackfill_test.go @@ -0,0 +1,71 @@ +package library + +import ( + "bytes" + "context" + "log/slog" + "os" + "path/filepath" + "strings" + "testing" + + "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" +) + +// The MBID backfills read tags from files, so a track whose file is gone has +// nothing to give them. Before #5139 they opened it anyway, and every scan +// logged an "open failed" warning per missing track. +func TestMBIDBackfill_SkipsMissingTracks_Integration(t *testing.T) { + pool := newPool(t) + ctx := context.Background() + q := dbq.New(pool) + dir := t.TempDir() + + // The album's first track is missing; its second is on disk, untagged. + gone, album, artist := seedTrack(t, pool, filepath.Join(dir, "gone.mp3")) + present := filepath.Join(dir, "present.mp3") + if err := os.WriteFile(present, []byte("not really audio"), 0o600); err != nil { + t.Fatal(err) + } + two := int32(2) + if _, err := q.UpsertTrack(ctx, dbq.UpsertTrackParams{ + Title: "Present", AlbumID: album.ID, ArtistID: artist.ID, TrackNumber: &two, + DurationMs: 1000, FilePath: present, FileSize: 100, FileFormat: "mp3", + }); err != nil { + t.Fatalf("track: %v", err) + } + if _, err := pool.Exec(ctx, "UPDATE tracks SET missing_since = now() WHERE id = $1", gone.ID); err != nil { + t.Fatalf("mark missing: %v", err) + } + + tracks, err := q.ListTracksMissingMbidWithPath(ctx, 100) + if err != nil { + t.Fatal(err) + } + if len(tracks) != 1 || tracks[0].FilePath != present { + t.Errorf("track backfill lists %+v, want only %s", tracks, present) + } + albums, err := q.ListAlbumsMissingMbidWithTrack(ctx, 100) + if err != nil { + t.Fatal(err) + } + if len(albums) != 1 || albums[0].TrackFilePath != present { + t.Errorf("album backfill reads %+v, want the album via %s", albums, present) + } + + // With every track gone, the album has no file to read and is skipped. + if _, err := pool.Exec(ctx, "UPDATE tracks SET missing_since = now() WHERE album_id = $1", album.ID); err != nil { + t.Fatalf("mark missing: %v", err) + } + var logs bytes.Buffer + logger := slog.New(slog.NewTextHandler(&logs, nil)) + if res, err := BackfillTrackMBIDs(ctx, pool, logger, -1, nil); err != nil || res.Processed != 0 { + t.Errorf("track backfill processed %d (err %v), want 0", res.Processed, err) + } + if res, err := BackfillMBIDs(ctx, pool, logger, -1, nil); err != nil || res.Processed != 0 { + t.Errorf("album backfill processed %d (err %v), want 0", res.Processed, err) + } + if strings.Contains(logs.String(), "open failed") { + t.Errorf("a backfill opened a missing file:\n%s", logs.String()) + } +}