From 4481874482a9d5502cff133a674bb2cd8f2c54c4 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Mon, 4 May 2026 21:45:15 -0400 Subject: [PATCH] fix(server,web/m7-382): handle duplicate-MBID conflicts during backfill MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Forward-fix from running the prior two commits in production. Backfill on a real library exposed two further issues: (1) gofmt -s flagged the column-aligned stubMeta methods in mbids_test.go. Switched to the gofmt-canonical single-space form. (2) Many albums failed to heal with SQLSTATE 23505 (unique constraint albums_mbid_unique). Cause: the operator's library has duplicate album rows in the DB — same MusicBrainz release, split into multiple rows by the pre-MBID scanner because of subtle title or artist disagreements between files. When backfill now correctly extracts the MBID, two rows want to claim the same one and the partial unique index rejects the second. The conflict is correct DB behavior — we shouldn't have two rows with the same MBID — but it's not a write failure to alarm the operator about. Detect 23505 specifically: - downgrade the log line from Warn to Info - track these in a new BackfillMBIDsResult.Duplicates counter (separate from Skipped, which retains its "no MBID in tag" meaning) - leave the duplicate row's mbid NULL; merging duplicates is a separate (future) operator workflow Same handling threaded through the scanner heal path so a regular rescan doesn't generate the noise either. JSON tally + admin UI gain a "Duplicates" line so the operator can see how many duplicate rows their library carries. Follow-up scope (separate task): a duplicate-album merge UX that reparents the duplicate's tracks to the canonical row and deletes the orphaned album row. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/library/mbidbackfill.go | 47 ++++++++++++++++++++++++++----- internal/library/mbids_test.go | 30 ++++++++++---------- internal/library/scanner.go | 11 ++++++-- internal/library/scanrun.go | 10 ++++--- web/src/lib/api/admin.ts | 1 + web/src/routes/admin/+page.svelte | 1 + 6 files changed, 72 insertions(+), 28 deletions(-) diff --git a/internal/library/mbidbackfill.go b/internal/library/mbidbackfill.go index 84040595..804ad39c 100644 --- a/internal/library/mbidbackfill.go +++ b/internal/library/mbidbackfill.go @@ -2,21 +2,36 @@ package library import ( "context" + "errors" "fmt" "log/slog" "os" "github.com/dhowden/tag" + "github.com/jackc/pgx/v5/pgconn" "github.com/jackc/pgx/v5/pgxpool" "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" ) +// pgUniqueViolation is the SQLSTATE for a unique-constraint conflict. +const pgUniqueViolation = "23505" + +// isUniqueViolation reports whether err is a Postgres unique-constraint +// conflict. Used to distinguish "another row already owns this MBID" +// (a duplicate album/artist row in our DB, the operator should merge +// it) from genuine write failures. +func isUniqueViolation(err error) bool { + var pgErr *pgconn.PgError + return errors.As(err, &pgErr) && pgErr.Code == pgUniqueViolation +} + // BackfillMBIDsResult tallies what the one-shot worker did. type BackfillMBIDsResult struct { - Processed int // Albums considered (mbid IS NULL). - Healed int // Albums whose mbid we set. - Skipped int // Albums where the file was missing or had no MBID tag. + Processed int // Albums considered (mbid IS NULL). + Healed int // Albums whose mbid we set. + Skipped int // Albums where the file was missing or had no MBID tag. + Duplicates int // Albums whose mbid is already held by another row (operator-resolvable). } // BackfillMBIDs walks albums with NULL mbid, opens one track per album, @@ -61,6 +76,17 @@ func BackfillMBIDs(ctx context.Context, pool *pgxpool.Pool, logger *slog.Logger, if err := q.SetAlbumMbidIfNull(ctx, dbq.SetAlbumMbidIfNullParams{ ID: r.AlbumID, Mbid: &m, }); err != nil { + if isUniqueViolation(err) { + // Another album row already owns this MBID — i.e. our DB + // has two rows for the same MusicBrainz release (split by + // title/artist disagreements during the pre-MBID scan). + // Operator must merge them; for now, leave this row's + // mbid NULL and skip the artist + cover-clear steps. + logger.Info("mbid backfill: duplicate album mbid (canonical row already owns it)", + "album_id", r.AlbumID, "mbid", albumMBID) + res.Duplicates++ + continue + } logger.Warn("mbid backfill: set album mbid failed", "album_id", r.AlbumID, "err", err) res.Skipped++ @@ -73,8 +99,13 @@ func BackfillMBIDs(ctx context.Context, pool *pgxpool.Pool, logger *slog.Logger, if err := q.SetArtistMbidIfNull(ctx, dbq.SetArtistMbidIfNullParams{ ID: r.ArtistID, Mbid: &am, }); err != nil { - logger.Warn("mbid backfill: set artist mbid failed", - "artist_id", r.ArtistID, "err", err) + if isUniqueViolation(err) { + logger.Info("mbid backfill: duplicate artist mbid (canonical row already owns it)", + "artist_id", r.ArtistID, "mbid", artistMBID) + } else { + logger.Warn("mbid backfill: set artist mbid failed", + "artist_id", r.ArtistID, "err", err) + } } } @@ -90,14 +121,16 @@ func BackfillMBIDs(ctx context.Context, pool *pgxpool.Pool, logger *slog.Logger, // Progress log every 100 healed. if (i+1)%100 == 0 { logger.Info("mbid backfill progress", - "processed", res.Processed, "healed", res.Healed, "skipped", res.Skipped) + "processed", res.Processed, "healed", res.Healed, + "skipped", res.Skipped, "duplicates", res.Duplicates) } } logger.Info("mbid backfill complete", "processed", res.Processed, "healed", res.Healed, - "skipped", res.Skipped) + "skipped", res.Skipped, + "duplicates", res.Duplicates) return res, nil } diff --git a/internal/library/mbids_test.go b/internal/library/mbids_test.go index 33494bf7..ccafeb32 100644 --- a/internal/library/mbids_test.go +++ b/internal/library/mbids_test.go @@ -15,21 +15,21 @@ type stubMeta struct { raw map[string]interface{} } -func (s stubMeta) Format() tag.Format { return s.format } -func (s stubMeta) FileType() tag.FileType { return tag.UnknownFileType } -func (s stubMeta) Title() string { return "" } -func (s stubMeta) Album() string { return "" } -func (s stubMeta) Artist() string { return "" } -func (s stubMeta) AlbumArtist() string { return "" } -func (s stubMeta) Composer() string { return "" } -func (s stubMeta) Year() int { return 0 } -func (s stubMeta) Genre() string { return "" } -func (s stubMeta) Track() (int, int) { return 0, 0 } -func (s stubMeta) Disc() (int, int) { return 0, 0 } -func (s stubMeta) Picture() *tag.Picture { return nil } -func (s stubMeta) Lyrics() string { return "" } -func (s stubMeta) Comment() string { return "" } -func (s stubMeta) Raw() map[string]interface{} { return s.raw } +func (s stubMeta) Format() tag.Format { return s.format } +func (s stubMeta) FileType() tag.FileType { return tag.UnknownFileType } +func (s stubMeta) Title() string { return "" } +func (s stubMeta) Album() string { return "" } +func (s stubMeta) Artist() string { return "" } +func (s stubMeta) AlbumArtist() string { return "" } +func (s stubMeta) Composer() string { return "" } +func (s stubMeta) Year() int { return 0 } +func (s stubMeta) Genre() string { return "" } +func (s stubMeta) Track() (int, int) { return 0, 0 } +func (s stubMeta) Disc() (int, int) { return 0, 0 } +func (s stubMeta) Picture() *tag.Picture { return nil } +func (s stubMeta) Lyrics() string { return "" } +func (s stubMeta) Comment() string { return "" } +func (s stubMeta) Raw() map[string]interface{} { return s.raw } func TestExtractMBIDs_Vorbis(t *testing.T) { // dhowden lowercases all Vorbis comment keys at parse time. diff --git a/internal/library/scanner.go b/internal/library/scanner.go index d3753269..7381f817 100644 --- a/internal/library/scanner.go +++ b/internal/library/scanner.go @@ -240,8 +240,15 @@ func (s *Scanner) resolveAlbum(ctx context.Context, q *dbq.Queries, artistID pgt ID: existing.ID, Mbid: &m, }); uerr != nil { - s.logger.Warn("library scan: heal album mbid failed", - "album_id", existing.ID, "err", uerr) + if isUniqueViolation(uerr) { + // Another album row already owns this MBID — duplicate + // release in the DB. Leave NULL; operator merges later. + s.logger.Info("library scan: duplicate album mbid (canonical row already owns it)", + "album_id", existing.ID, "mbid", mbid) + } else { + s.logger.Warn("library scan: heal album mbid failed", + "album_id", existing.ID, "err", uerr) + } } else { existing.Mbid = &m } diff --git a/internal/library/scanrun.go b/internal/library/scanrun.go index 1fa3fa9f..a27b7f56 100644 --- a/internal/library/scanrun.go +++ b/internal/library/scanrun.go @@ -27,9 +27,10 @@ type LibraryStageTallies struct { // MBIDBackfillStageTallies wires BackfillMBIDsResult into the scan_runs jsonb column. type MBIDBackfillStageTallies struct { - Processed int `json:"processed"` - Healed int `json:"healed"` - Skipped int `json:"skipped"` + Processed int `json:"processed"` + Healed int `json:"healed"` + Skipped int `json:"skipped"` + Duplicates int `json:"duplicates"` } // CoverEnrichStageTallies wires EnrichBatch tallies into the scan_runs jsonb column. @@ -104,7 +105,8 @@ func RunScan( captureErr("mbid_backfill", bferr) } blob, _ := json.Marshal(MBIDBackfillStageTallies{ - Processed: bfRes.Processed, Healed: bfRes.Healed, Skipped: bfRes.Skipped, + Processed: bfRes.Processed, Healed: bfRes.Healed, + Skipped: bfRes.Skipped, Duplicates: bfRes.Duplicates, }) if uerr := q.UpdateScanRunMbidBackfill(ctx, dbq.UpdateScanRunMbidBackfillParams{ ID: row.ID, MbidBackfill: blob, diff --git a/web/src/lib/api/admin.ts b/web/src/lib/api/admin.ts index 29d0b111..815badbd 100644 --- a/web/src/lib/api/admin.ts +++ b/web/src/lib/api/admin.ts @@ -196,6 +196,7 @@ export type ScanStageMbidBackfill = { processed: number; healed: number; skipped: number; + duplicates?: number; }; export type ScanStageCoverEnrich = { diff --git a/web/src/routes/admin/+page.svelte b/web/src/routes/admin/+page.svelte index aed8e7d5..09460146 100644 --- a/web/src/routes/admin/+page.svelte +++ b/web/src/routes/admin/+page.svelte @@ -360,6 +360,7 @@
Processed
{scan.mbid_backfill.processed}
Healed
{scan.mbid_backfill.healed}
Skipped
{scan.mbid_backfill.skipped}
+
Duplicates
{scan.mbid_backfill.duplicates ?? 0}
{:else}

{scanInFlight ? 'Pending…' : 'Skipped.'}