fix: FLAC, Ogg and Opus tracks carry their MusicBrainz recording id from the tag: one reader in library gives the scanner, the backfills and the importer the same ids, and the next scan re-reads the library (#5637)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

internal/importer/inspect.go

@@ -10,7 +10,6 @@ import (
 	"strings"
 
 	"github.com/dhowden/tag"
-	"github.com/dhowden/tag/mbz"
 
 	"git.fabledsword.com/bvandeusen/minstrel/internal/acoustid"
 	"git.fabledsword.com/bvandeusen/minstrel/internal/library"
@@ -136,15 +135,8 @@ func readFile(ctx context.Context, root, path string) File {
 	f.Title, f.Artist, f.AlbumArtist, f.Album = m.Title(), m.Artist(), m.AlbumArtist(), m.Album()
 	f.Track, _ = m.Track()
 	f.Disc, _ = m.Disc()
-	info := mbz.Extract(m)
-	f.GroupMBID = clean(info.Get(mbz.ReleaseGroup))
-	f.ReleaseMBID = clean(info.Get(mbz.Album))
-	f.RecordingMBID = clean(info.Get(mbz.Recording))
-	if f.RecordingMBID == "" && m.Format() == tag.VORBIS {
-		// Picard's Vorbis key for the recording is MUSICBRAINZ_TRACKID, which
-		// mbz files under its release-track name.
-		f.RecordingMBID = clean(info.Get(mbz.Track))
-	}
+	ids := library.ReadTagMBIDs(m)
+	f.GroupMBID, f.ReleaseMBID, f.RecordingMBID = ids.ReleaseGroup, ids.Release, ids.Recording
 	return f
 }
 
@@ -155,16 +147,6 @@ func rel(root, path string) string {
 	return filepath.Base(path)
 }
 
-// clean takes the first of NUL-separated values: ID3v2.4 multi-values.
-func clean(s string) string {
-	for _, part := range strings.Split(s, "\x00") {
-		if t := strings.TrimSpace(part); t != "" {
-			return t
-		}
-	}
-	return ""
-}
-
 // inspect reads every audio file under root and asks the printer about each.
 // fingerprinted reports whether the printer answered at all.
 func inspect(ctx context.Context, root string, p Printer) (files []File, fingerprinted bool, err error) {

internal/library/mbidbackfill.go

@@ -165,7 +165,8 @@ func readMBIDsForFile(path string, logger *slog.Logger) (albumMBID, artistMBID s
 		logger.Warn("mbid backfill: tag read failed", "path", path, "err", err)
 		return "", ""
 	}
-	return extractMBIDs(meta)
+	ids := ReadTagMBIDs(meta)
+	return ids.Release, ids.Artist
 }
 
 // BackfillTrackMBIDsResult tallies the track recording-MBID backfill.
@@ -258,5 +259,5 @@ func readRecordingMBIDForFile(path string, logger *slog.Logger) string {
 		logger.Warn("track mbid backfill: tag read failed", "path", path, "err", err)
 		return ""
 	}
-	return extractRecordingMBID(meta)
+	return ReadTagMBIDs(meta).Recording
 }

internal/library/mbids.go

@@ -7,54 +7,57 @@ import (
 	"github.com/dhowden/tag/mbz"
 )
 
-// extractMBIDs reads MusicBrainz album and artist IDs from a tag.Metadata.
+// TagMBIDs is what a file's MusicBrainz Picard tags name. Empty means the
+// tag is absent.
+type TagMBIDs struct {
+	// Release is musicbrainz_albumid: one edition, what albums.mbid stores.
+	Release string
+	Artist  string
+	// ReleaseGroup is musicbrainz_releasegroupid. Wants name albums by
+	// release group, so re-acquisition and request completion key on it
+	// (#5241).
+	ReleaseGroup string
+	// Recording is what tracks.mbid stores and the ListenBrainz Labs
+	// similar-recordings API keys on.
+	Recording string
+}
+
+// ReadTagMBIDs reads a file's MusicBrainz IDs, for the scanner, the MBID
+// backfills and the importer alike.
 //
 // Delegates to dhowden/tag/mbz, which knows the per-format Raw() conventions:
-//   - Vorbis (FLAC/OGG) keys are lowercased by the parser.
+//   - Vorbis (FLAC/OGG/Opus) keys are lowercased by the parser.
 //   - ID3v2 (MP3) TXXX frames are stored under "TXXX"/"TXXX_N" with
 //     a *tag.Comm value whose Description holds the Picard tag name.
 //   - MP4 (M4A) freeform iTunes atoms are stored under their bare sub-name.
 //
-// Then sanitizes: dhowden's readTextWithDescrFrame (used for ID3v2 TXXX)
-// does NOT strip the trailing/embedded NUL bytes that ID3v2.4 uses as
-// frame terminator and multi-value separator — so the Text often comes
-// out as "uuid\x00" or "uuid1\x00uuid2". Postgres rejects NULs in text
-// columns (SQLSTATE 22021). We split on NUL and take the first non-empty
-// segment — Minstrel uses the primary release artist for MBCAA lookup,
-// so first-wins matches the original semantics.
-func extractMBIDs(m tag.Metadata) (albumMBID, artistMBID string) {
+// mbz maps keys by name alone, and Picard names the recording differently
+// in Vorbis: MUSICBRAINZ_TRACKID there is the recording (the release track
+// is MUSICBRAINZ_RELEASETRACKID), so mbz files it under mbz.Track, its name
+// for the release track. Read mbz.Recording alone and every FLAC has no
+// recording id (#5637).
+func ReadTagMBIDs(m tag.Metadata) TagMBIDs {
 	info := mbz.Extract(m)
-	return cleanMBID(info.Get(mbz.Album)), cleanMBID(info.Get(mbz.Artist))
-}
-
-// extractRecordingMBID reads the MusicBrainz *recording* ID — Picard's
-// musicbrainz_recordingid, surfaced by dhowden/tag as mbz.Recording
-// (tag display name "MusicBrainz Track Id"). This is the id the
-// ListenBrainz Labs similar-recordings API keys on and what tracks.mbid
-// stores.
-//
-// Deliberately NOT mbz.Track ("MusicBrainz Release Track Id"): that is
-// per-release-track, whereas similarity is per-recording. Kept separate
-// from extractMBIDs so its existing (album, artist) signature and unit
-// tests stay untouched.
-func extractRecordingMBID(m tag.Metadata) string {
-	return cleanMBID(mbz.Extract(m).Get(mbz.Recording))
-}
-
-// extractReleaseGroupMBID reads the MusicBrainz *release-group* ID —
-// Picard's musicbrainz_releasegroupid, surfaced by dhowden/tag as
-// mbz.ReleaseGroup. mbz.Album (what albums.mbid stores) is the release: one
-// edition. Wants name albums by release group, so re-acquisition and
-// request completion key on this one (#5241). Separate from extractMBIDs for
-// the same reason extractRecordingMBID is.
-func extractReleaseGroupMBID(m tag.Metadata) string {
-	return cleanMBID(mbz.Extract(m).Get(mbz.ReleaseGroup))
-}
-
-func cleanMBID(s string) string {
-	if s == "" {
-		return ""
+	ids := TagMBIDs{
+		Release:      cleanMBID(info.Get(mbz.Album)),
+		Artist:       cleanMBID(info.Get(mbz.Artist)),
+		ReleaseGroup: cleanMBID(info.Get(mbz.ReleaseGroup)),
+		Recording:    cleanMBID(info.Get(mbz.Recording)),
 	}
+	if ids.Recording == "" && m.Format() == tag.VORBIS {
+		ids.Recording = cleanMBID(info.Get(mbz.Track))
+	}
+	return ids
+}
+
+// cleanMBID takes the first non-empty NUL-separated value. dhowden's
+// readTextWithDescrFrame (used for ID3v2 TXXX) does NOT strip the
+// trailing/embedded NUL bytes that ID3v2.4 uses as frame terminator and
+// multi-value separator — so the Text often comes out as "uuid\x00" or
+// "uuid1\x00uuid2". Postgres rejects NULs in text columns (SQLSTATE 22021).
+// Minstrel uses the primary release artist for MBCAA lookup, so first-wins
+// matches the original semantics.
+func cleanMBID(s string) string {
 	for _, part := range strings.Split(s, "\x00") {
 		if t := strings.TrimSpace(part); t != "" {
 			return t

internal/library/mbids_test.go

@@ -40,7 +40,8 @@ func TestExtractMBIDs_Vorbis(t *testing.T) {
 			"musicbrainz_artistid": "def",
 		},
 	}
-	a, ar := extractMBIDs(m)
+	ids := ReadTagMBIDs(m)
+	a, ar := ids.Release, ids.Artist
 	if a != "abc" || ar != "def" {
 		t.Errorf("got (%q, %q), want (\"abc\", \"def\")", a, ar)
 	}
@@ -62,7 +63,8 @@ func TestExtractMBIDs_ID3v2(t *testing.T) {
 			},
 		},
 	}
-	a, ar := extractMBIDs(m)
+	ids := ReadTagMBIDs(m)
+	a, ar := ids.Release, ids.Artist
 	if a != "abc-123" || ar != "def-456" {
 		t.Errorf("got (%q, %q), want (\"abc-123\", \"def-456\")", a, ar)
 	}
@@ -78,7 +80,8 @@ func TestExtractMBIDs_MP4(t *testing.T) {
 			"MusicBrainz Artist Id": "def",
 		},
 	}
-	a, ar := extractMBIDs(m)
+	ids := ReadTagMBIDs(m)
+	a, ar := ids.Release, ids.Artist
 	if a != "abc" || ar != "def" {
 		t.Errorf("got (%q, %q), want (\"abc\", \"def\")", a, ar)
 	}
@@ -86,7 +89,8 @@ func TestExtractMBIDs_MP4(t *testing.T) {
 
 func TestExtractMBIDs_MissingTagsReturnEmpty(t *testing.T) {
 	m := stubMeta{format: tag.VORBIS, raw: map[string]interface{}{}}
-	a, ar := extractMBIDs(m)
+	ids := ReadTagMBIDs(m)
+	a, ar := ids.Release, ids.Artist
 	if a != "" || ar != "" {
 		t.Errorf("got (%q, %q), want empty", a, ar)
 	}
@@ -104,7 +108,8 @@ func TestExtractMBIDs_VorbisUppercaseKeysMiss(t *testing.T) {
 			"MUSICBRAINZ_ARTISTID": "wrong-case",
 		},
 	}
-	a, ar := extractMBIDs(m)
+	ids := ReadTagMBIDs(m)
+	a, ar := ids.Release, ids.Artist
 	if a != "" || ar != "" {
 		t.Errorf("uppercase Vorbis keys must not match dhowden output; got (%q, %q)", a, ar)
 	}
@@ -127,7 +132,8 @@ func TestExtractMBIDs_ID3v2StripsTrailingNUL(t *testing.T) {
 			},
 		},
 	}
-	a, ar := extractMBIDs(m)
+	ids := ReadTagMBIDs(m)
+	a, ar := ids.Release, ids.Artist
 	if a != "abc-123" || ar != "def-456" {
 		t.Errorf("got (%q, %q), want trailing NUL stripped", a, ar)
 	}
@@ -146,7 +152,7 @@ func TestExtractMBIDs_ID3v2MultiValueTakesFirst(t *testing.T) {
 			},
 		},
 	}
-	_, ar := extractMBIDs(m)
+	ar := ReadTagMBIDs(m).Artist
 	if ar != "primary-mbid" {
 		t.Errorf("got %q, want %q", ar, "primary-mbid")
 	}
@@ -186,17 +192,53 @@ func TestExtractReleaseGroupMBID(t *testing.T) {
 	}
 	for _, c := range cases {
 		t.Run(c.name, func(t *testing.T) {
-			if got := extractReleaseGroupMBID(c.meta); got != "group-1" {
+			if got := ReadTagMBIDs(c.meta).ReleaseGroup; got != "group-1" {
 				t.Errorf("release group = %q, want group-1", got)
 			}
-			if album, _ := extractMBIDs(c.meta); album != "release-1" {
+			if album := ReadTagMBIDs(c.meta).Release; album != "release-1" {
 				t.Errorf("album (release) = %q, want release-1", album)
 			}
 		})
 	}
 
 	none := stubMeta{format: tag.VORBIS, raw: map[string]interface{}{"musicbrainz_albumid": "release-1"}}
-	if got := extractReleaseGroupMBID(none); got != "" {
+	if got := ReadTagMBIDs(none).ReleaseGroup; got != "" {
 		t.Errorf("untagged release group = %q, want empty", got)
 	}
 }
+
+// #5637: each format's recording id lands in Recording, and the release-track
+// id never does. Picard calls the recording MUSICBRAINZ_TRACKID in Vorbis
+// comments, the name mbz gives the release track.
+func TestReadTagMBIDs_Recording(t *testing.T) {
+	cases := []struct {
+		name string
+		meta stubMeta
+	}{
+		{"vorbis", stubMeta{format: tag.VORBIS, raw: map[string]interface{}{
+			"musicbrainz_trackid":        "rec-1",
+			"musicbrainz_releasetrackid": "reltrack-1",
+		}}},
+		{"id3v2 ufid", stubMeta{format: tag.ID3v2_4, raw: map[string]interface{}{
+			"UFID":   &tag.UFID{Provider: "http://musicbrainz.org", Identifier: []byte("rec-1")},
+			"TXXX_0": &tag.Comm{Description: "MusicBrainz Release Track Id", Text: "reltrack-1\x00"},
+		}}},
+		{"mp4", stubMeta{format: tag.MP4, raw: map[string]interface{}{
+			"MusicBrainz Track Id":         "rec-1",
+			"MusicBrainz Release Track Id": "reltrack-1",
+		}}},
+	}
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			if got := ReadTagMBIDs(c.meta).Recording; got != "rec-1" {
+				t.Errorf("recording = %q, want rec-1", got)
+			}
+		})
+	}
+
+	// A Vorbis file carrying only the release-track id has no recording id.
+	only := stubMeta{format: tag.VORBIS, raw: map[string]interface{}{"musicbrainz_releasetrackid": "reltrack-1"}}
+	if got := ReadTagMBIDs(only).Recording; got != "" {
+		t.Errorf("release track only: recording = %q, want empty", got)
+	}
+}

internal/library/scanner.go

@@ -64,7 +64,12 @@ var audioExtensions = map[string]bool{
 //	   and MP4 (#2500). dhowden/tag kept only the last, and 3,658 of the
 //	   operator's 4,092 FLACs declare more than one, so most of their genres
 //	   were never stored.
-const tagReadVersion int16 = 4
+//	5: the recording id is read from FLAC, Ogg Vorbis and Opus (#5637). Picard
+//	   calls it MUSICBRAINZ_TRACKID there and dhowden/tag files that under the
+//	   release-track name, so every Vorbis-tagged track came in without its
+//	   tag id and waited on an AcoustID lookup. A re-read stores the tag id,
+//	   over a looked-up one (D4).
+const tagReadVersion int16 = 5
 
 type Stats struct {
 	Scanned int `json:"scanned"`
@@ -272,9 +277,9 @@ func (s *Scanner) scanFile(
 	if err != nil {
 		return pgtype.UUID{}, false, fmt.Errorf("tag read: %w", err)
 	}
-	albumMBID, artistMBID := extractMBIDs(meta)
-	recordingMBID := extractRecordingMBID(meta)
-	releaseGroupMBID := extractReleaseGroupMBID(meta)
+	ids := ReadTagMBIDs(meta)
+	albumMBID, artistMBID := ids.Release, ids.Artist
+	recordingMBID, releaseGroupMBID := ids.Recording, ids.ReleaseGroup
 
 	artistName := meta.Artist()
 	if artistName == "" {