fix(scanner): read multi-value genre frames correctly — #2499

dhowden/tag's readTFrame splits ID3v2 null-separated multi-value text
frames and rejoins them with the EMPTY string, so a file tagged
"Alternative Rock" + "Rock" was stored as "Alternative RockRock". It also
leaves bare numeric ID3v1 references unresolved, which is why the
library showed genres like "4017" and "526617".

This corrupted more than the browse axis added in #367: taste_profile.sql
reads tracks.genre directly, so the welded tokens were entering the taste
profile's tag vocabulary, and recommendation.sql/discover.sql were
comparing them as single opaque tags. Genre counts were wrong everywhere.

ffprobe is not a fix — ffmpeg's read_ttag calls decode_str once with no
loop, keeping only the first value. Truncating multi-genre tags would
blunt the similarity signal genre mainly feeds. So the TCON frame is now
parsed directly (ID3v2.2/2.3/2.4, all four text encodings, per-frame and
tag-level unsynchronisation, numeric and parenthesised ID3v1 references);
everything else still comes from dhowden/tag. Values are stored
";"-delimited, which the read side already splits on, so no query changes.

Existing rows are repaired without an operator-run rebuild: migration
0054 adds tracks.tag_read_version DEFAULT 0, below the scanner's current
tagReadVersion, so the next scan re-reads tags it would otherwise skip on
mtime. Such a re-read reuses the stored duration instead of re-running
ffprobe, keeping a repair pass tag-read-bound rather than one fork+exec
per file. Bumping the constant is how a future extraction fix reaches an
existing library.

Only ID3v2 is in scope — dhowden welds nowhere else. The Vorbis/MP4
repeated-field question is #2500, unproven and deliberately not built.

internal/library/genre_test.go

@@ -0,0 +1,421 @@
+package library
+
+import (
+	"bytes"
+	"encoding/binary"
+	"strings"
+	"testing"
+
+	"github.com/dhowden/tag"
+)
+
+// rawFrame is a frame with a byte-exact payload, so tests can express encoding
+// bytes and embedded nulls that a string-keyed helper can't.
+type rawFrame struct {
+	id      string
+	payload []byte
+}
+
+// buildID3v2 assembles a tag for the given major version. Frame size encoding
+// differs per version (2.4 is synchsafe, 2.2/2.3 are plain), which is exactly
+// the kind of detail a parser gets subtly wrong, so tests build all three.
+func buildID3v2(t *testing.T, major byte, frames ...rawFrame) []byte {
+	t.Helper()
+	var body bytes.Buffer
+	for _, f := range frames {
+		switch major {
+		case 2:
+			if len(f.id) != 3 {
+				t.Fatalf("v2.2 frame id %q must be 3 bytes", f.id)
+			}
+			body.WriteString(f.id)
+			n := len(f.payload)
+			body.Write([]byte{byte(n >> 16), byte(n >> 8), byte(n)})
+		case 3:
+			body.WriteString(f.id)
+			_ = binary.Write(&body, binary.BigEndian, uint32(len(f.payload)))
+			body.Write([]byte{0x00, 0x00})
+		case 4:
+			body.WriteString(f.id)
+			body.Write(synchsafeBytes(len(f.payload)))
+			body.Write([]byte{0x00, 0x00})
+		}
+		body.Write(f.payload)
+	}
+	var out bytes.Buffer
+	out.WriteString("ID3")
+	out.Write([]byte{major, 0x00, 0x00})
+	out.Write(synchsafeBytes(body.Len()))
+	out.Write(body.Bytes())
+	// A few bytes of MPEG sync so dhowden/tag accepts the file shape.
+	out.Write([]byte{0xFF, 0xFB, 0x90, 0x00})
+	return out.Bytes()
+}
+
+func synchsafeBytes(n int) []byte {
+	return []byte{
+		byte((n >> 21) & 0x7F),
+		byte((n >> 14) & 0x7F),
+		byte((n >> 7) & 0x7F),
+		byte(n & 0x7F),
+	}
+}
+
+// utf8Frame builds a text-frame payload: encoding byte 3 (UTF-8) followed by
+// values joined with the null separator ID3v2 uses for multiple values.
+func utf8Frame(values ...string) []byte {
+	return append([]byte{0x03}, []byte(strings.Join(values, "\x00"))...)
+}
+
+// TestReadID3v2GenreValues_MultiValue is the #2499 regression. dhowden/tag
+// rejoins these values with the empty string, producing "Alternative RockRock";
+// the whole point of our own reader is that they stay separate.
+func TestReadID3v2GenreValues_MultiValue(t *testing.T) {
+	for _, major := range []byte{2, 3, 4} {
+		id := "TCON"
+		if major == 2 {
+			id = "TCO"
+		}
+		data := buildID3v2(t, major, rawFrame{id, utf8Frame("Alternative Rock", "Rock")})
+		got, err := readID3v2GenreValues(bytes.NewReader(data))
+		if err != nil {
+			t.Fatalf("v2.%d: %v", major, err)
+		}
+		want := []string{"Alternative Rock", "Rock"}
+		if !equalStrings(got, want) {
+			t.Errorf("v2.%d genres = %q, want %q", major, got, want)
+		}
+	}
+}
+
+// The operator's worst case: eight values welded into one 70-character token.
+func TestReadID3v2GenreValues_ManyValues(t *testing.T) {
+	values := []string{
+		"Boom Bap", "Downtempo", "Hip Hop", "Instrumental",
+		"Lo-Fi", "Lo-Fi Hip Hop", "Chillwave", "Instrumental Hip Hop",
+	}
+	data := buildID3v2(t, 4, rawFrame{"TCON", utf8Frame(values...)})
+	got, err := readID3v2GenreValues(bytes.NewReader(data))
+	if err != nil {
+		t.Fatal(err)
+	}
+	if !equalStrings(got, values) {
+		t.Errorf("genres = %q, want %q", got, values)
+	}
+}
+
+// A trailing null terminator is legal and must not produce an empty value.
+func TestReadID3v2GenreValues_TrailingTerminator(t *testing.T) {
+	data := buildID3v2(t, 4, rawFrame{"TCON", append(utf8Frame("Jazz"), 0x00)})
+	got, err := readID3v2GenreValues(bytes.NewReader(data))
+	if err != nil {
+		t.Fatal(err)
+	}
+	if !equalStrings(got, []string{"Jazz"}) {
+		t.Errorf("genres = %q, want [Jazz]", got)
+	}
+}
+
+// UTF-16 uses a TWO-byte separator. Splitting it on single nulls would cut
+// every ASCII character in half, so this guards the width handling.
+func TestReadID3v2GenreValues_UTF16(t *testing.T) {
+	tests := []struct {
+		name    string
+		payload []byte
+	}{
+		{
+			// Spec-correct: encoding 1 with a BOM on every value.
+			name: "utf16le, BOM on each value",
+			payload: concat([]byte{0x01},
+				[]byte{0xFF, 0xFE}, utf16LE("Rock"),
+				[]byte{0x00, 0x00},
+				[]byte{0xFF, 0xFE}, utf16LE("Pop")),
+		},
+		{
+			// Sloppy but common: BOM only on the first value. Without carrying
+			// the byte order forward, "Pop" decodes byte-swapped to CJK.
+			name: "utf16le, BOM only on the first value",
+			payload: concat([]byte{0x01},
+				[]byte{0xFF, 0xFE}, utf16LE("Rock"),
+				[]byte{0x00, 0x00}, utf16LE("Pop")),
+		},
+		{
+			// Encoding 2: big-endian, no BOM anywhere.
+			name: "utf16be no BOM",
+			payload: concat([]byte{0x02},
+				utf16BE("Rock"), []byte{0x00, 0x00}, utf16BE("Pop")),
+		},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			data := buildID3v2(t, 4, rawFrame{"TCON", tc.payload})
+			got, err := readID3v2GenreValues(bytes.NewReader(data))
+			if err != nil {
+				t.Fatal(err)
+			}
+			if !equalStrings(got, []string{"Rock", "Pop"}) {
+				t.Errorf("genres = %q, want [Rock Pop]", got)
+			}
+		})
+	}
+}
+
+// ISO-8859-1 must be widened, not reinterpreted as UTF-8 — "Bj\xf6rk" would
+// otherwise come back as invalid bytes.
+func TestReadID3v2GenreValues_Latin1(t *testing.T) {
+	payload := append([]byte{0x00}, []byte("Chanson Fran\xe7aise")...)
+	data := buildID3v2(t, 3, rawFrame{"TCON", payload})
+	got, err := readID3v2GenreValues(bytes.NewReader(data))
+	if err != nil {
+		t.Fatal(err)
+	}
+	if !equalStrings(got, []string{"Chanson Française"}) {
+		t.Errorf("genres = %q, want [Chanson Française]", got)
+	}
+}
+
+// Frames before TCON must be walked over correctly. If the size field were
+// decoded with the wrong scheme the walk lands mid-frame and TCON is missed.
+func TestReadID3v2GenreValues_SkipsPrecedingFrames(t *testing.T) {
+	for _, major := range []byte{3, 4} {
+		data := buildID3v2(t, major,
+			rawFrame{"TIT2", utf8Frame("Some Title")},
+			rawFrame{"TPE1", utf8Frame("Some Artist")},
+			rawFrame{"TCON", utf8Frame("Shoegaze", "Dream Pop")},
+		)
+		got, err := readID3v2GenreValues(bytes.NewReader(data))
+		if err != nil {
+			t.Fatalf("v2.%d: %v", major, err)
+		}
+		if !equalStrings(got, []string{"Shoegaze", "Dream Pop"}) {
+			t.Errorf("v2.%d genres = %q, want [Shoegaze Dream Pop]", major, got)
+		}
+	}
+}
+
+func TestReadID3v2GenreValues_NoGenreFrame(t *testing.T) {
+	data := buildID3v2(t, 4, rawFrame{"TIT2", utf8Frame("Only A Title")})
+	if _, err := readID3v2GenreValues(bytes.NewReader(data)); err == nil {
+		t.Fatal("expected an error when no genre frame is present")
+	}
+}
+
+func TestReadID3v2GenreValues_NotAnID3File(t *testing.T) {
+	if _, err := readID3v2GenreValues(bytes.NewReader([]byte("not a tag at all"))); err == nil {
+		t.Fatal("expected an error for a file with no ID3v2 tag")
+	}
+}
+
+// Padding after the last frame is zero bytes; the walk must stop rather than
+// read a frame id of "\x00\x00\x00\x00".
+func TestReadID3v2GenreValues_StopsAtPadding(t *testing.T) {
+	tagged := buildID3v2(t, 4, rawFrame{"TCON", utf8Frame("Rock")})
+	// Splice 32 padding bytes in before the MPEG sync trailer, growing the
+	// declared tag size to match.
+	body := tagged[10 : len(tagged)-4]
+	padded := append(append([]byte{}, body...), make([]byte, 32)...)
+	var out bytes.Buffer
+	out.WriteString("ID3")
+	out.Write([]byte{4, 0x00, 0x00})
+	out.Write(synchsafeBytes(len(padded)))
+	out.Write(padded)
+	got, err := readID3v2GenreValues(bytes.NewReader(out.Bytes()))
+	if err != nil {
+		t.Fatal(err)
+	}
+	if !equalStrings(got, []string{"Rock"}) {
+		t.Errorf("genres = %q, want [Rock]", got)
+	}
+}
+
+// Unsynchronisation inserts 0xFF 0x00 pairs that must be collapsed before the
+// frame list is walked, or every offset past the first pair is wrong.
+func TestReadID3v2GenreValues_TagUnsynchronisation(t *testing.T) {
+	// Latin-1 so a genre can legitimately contain the byte 0xFF ("ÿ"). Once
+	// unsynchronised that becomes 0xFF 0x00 — which is indistinguishable from a
+	// value separator until the collapse runs, so this fails loudly if
+	// undoUnsynchronisation is skipped.
+	payload := concat([]byte{0x00}, []byte("Ro\xffck"), []byte{0x00}, []byte("Pop"))
+	inner := buildID3v2(t, 3, rawFrame{"TCON", payload})
+	body := inner[10 : len(inner)-4]
+	encoded := bytes.ReplaceAll(body, []byte{0xFF}, []byte{0xFF, 0x00})
+	if bytes.Equal(encoded, body) {
+		t.Fatal("test is vacuous: nothing was unsynchronised")
+	}
+	var out bytes.Buffer
+	out.WriteString("ID3")
+	out.Write([]byte{3, 0x00, 0x80}) // 0x80 = unsynchronisation
+	out.Write(synchsafeBytes(len(encoded)))
+	out.Write(encoded)
+
+	got, err := readID3v2GenreValues(bytes.NewReader(out.Bytes()))
+	if err != nil {
+		t.Fatal(err)
+	}
+	if !equalStrings(got, []string{"Roÿck", "Pop"}) {
+		t.Errorf("genres = %q, want [Roÿck Pop]", got)
+	}
+}
+
+func TestNormaliseGenreValue(t *testing.T) {
+	tests := []struct {
+		name string
+		in   string
+		want []string
+	}{
+		{"plain text", "Alternative Rock", []string{"Alternative Rock"}},
+		{"trims whitespace", "  Jazz  ", []string{"Jazz"}},
+		{"empty", "", nil},
+		{"whitespace only", "   ", nil},
+
+		// The operator's digit soup, one value at a time.
+		{"bare numeric", "17", []string{"Rock"}},
+		{"bare numeric pop", "13", []string{"Pop"}},
+		{"bare numeric electronic", "52", []string{"Electronic"}},
+		{"winamp extension range", "187", []string{"Indie Rock"}},
+		{"numeric out of range", "9999", nil},
+		{"negative", "-1", nil},
+
+		// ID3v2.3 refinement syntax.
+		{"parenthesised", "(17)", []string{"Rock"}},
+		{"parenthesised repeated", "(51)(39)", []string{"Techno-Industrial", "Noise"}},
+		{"parenthesised with refinement", "(17)Hard Rock", []string{"Rock", "Hard Rock"}},
+		{"remix", "(RX)", []string{"Remix"}},
+		{"cover", "(CR)", []string{"Cover"}},
+		{"escaped open paren", "((Weird", []string{"(Weird"}},
+		{"parenthesised non-numeric", "(Live)", []string{"(Live)"}},
+
+		// A label that merely starts with digits is text, not a reference.
+		{"digits in a name", "1980s", []string{"1980s"}},
+		{"hyphenated", "Lo-Fi Hip Hop", []string{"Lo-Fi Hip Hop"}},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			got := normaliseGenreValue(tc.in)
+			if !equalStrings(got, tc.want) {
+				t.Errorf("normaliseGenreValue(%q) = %q, want %q", tc.in, got, tc.want)
+			}
+		})
+	}
+}
+
+func TestNormaliseGenres_DedupesCaseInsensitively(t *testing.T) {
+	got := normaliseGenres([]string{"Rock", "rock", "ROCK", "Pop"})
+	// First spelling wins — we are not imposing a canonical case here, only
+	// removing values that repeat within a single file.
+	if !equalStrings(got, []string{"Rock", "Pop"}) {
+		t.Errorf("genres = %q, want [Rock Pop]", got)
+	}
+}
+
+// "(40)AlternRock" declares the same genre twice — numerically and in text.
+func TestNormaliseGenres_DedupesResolvedNumeric(t *testing.T) {
+	got := normaliseGenres([]string{"(40)AlternRock"})
+	if !equalStrings(got, []string{"AlternRock"}) {
+		t.Errorf("genres = %q, want [AlternRock]", got)
+	}
+}
+
+func TestNormaliseGenres_AllJunkYieldsNil(t *testing.T) {
+	if got := normaliseGenres([]string{"", "  ", "9999"}); got != nil {
+		t.Errorf("genres = %q, want nil", got)
+	}
+}
+
+// End-to-end through dhowden/tag, which is what the scanner actually calls.
+// Proves the welded value never reaches the caller.
+func TestExtractGenres_EndToEnd(t *testing.T) {
+	data := buildID3v2(t, 4,
+		rawFrame{"TIT2", utf8Frame("A Song")},
+		rawFrame{"TCON", utf8Frame("Alternative Rock", "Rock")},
+	)
+	rs := bytes.NewReader(data)
+	meta, err := tag.ReadFrom(rs)
+	if err != nil {
+		t.Fatalf("tag.ReadFrom: %v", err)
+	}
+	// Confirm the upstream behaviour this fix exists for is still present —
+	// if dhowden ever fixes it, this test tells us the workaround can go.
+	if welded := meta.Genre(); welded != "Alternative RockRock" {
+		t.Logf("note: dhowden/tag no longer welds multi-values (got %q)", welded)
+	}
+
+	genres, fellBack := extractGenres(meta, rs)
+	if fellBack {
+		t.Error("fellBack = true, want false — the TCON frame is parseable")
+	}
+	if !equalStrings(genres, []string{"Alternative Rock", "Rock"}) {
+		t.Errorf("genres = %q, want [Alternative Rock Rock]", genres)
+	}
+	if joined := strings.Join(genres, genreDelimiter); joined != "Alternative Rock;Rock" {
+		t.Errorf("stored value = %q, want %q", joined, "Alternative Rock;Rock")
+	}
+}
+
+// The digit-soup case, end to end: numeric references resolve to names.
+func TestExtractGenres_ResolvesNumericReferences(t *testing.T) {
+	data := buildID3v2(t, 4, rawFrame{"TCON", utf8Frame("40", "17")})
+	rs := bytes.NewReader(data)
+	meta, err := tag.ReadFrom(rs)
+	if err != nil {
+		t.Fatalf("tag.ReadFrom: %v", err)
+	}
+	genres, _ := extractGenres(meta, rs)
+	if !equalStrings(genres, []string{"AlternRock", "Rock"}) {
+		t.Errorf("genres = %q, want [AlternRock Rock]", genres)
+	}
+}
+
+// A file with no genre at all must yield nothing and must NOT be reported as a
+// fallback — that would log a warning for every untagged file in the library.
+func TestExtractGenres_NoGenreIsNotAFallback(t *testing.T) {
+	data := buildID3v2(t, 4, rawFrame{"TIT2", utf8Frame("A Song")})
+	rs := bytes.NewReader(data)
+	meta, err := tag.ReadFrom(rs)
+	if err != nil {
+		t.Fatalf("tag.ReadFrom: %v", err)
+	}
+	genres, fellBack := extractGenres(meta, rs)
+	if len(genres) != 0 {
+		t.Errorf("genres = %q, want none", genres)
+	}
+	if fellBack {
+		t.Error("fellBack = true for an untagged file; would log on every such file")
+	}
+}
+
+func concat(parts ...[]byte) []byte {
+	var out []byte
+	for _, p := range parts {
+		out = append(out, p...)
+	}
+	return out
+}
+
+func utf16LE(s string) []byte {
+	out := make([]byte, 0, len(s)*2)
+	for _, r := range s {
+		out = append(out, byte(r), byte(r>>8))
+	}
+	return out
+}
+
+func utf16BE(s string) []byte {
+	out := make([]byte, 0, len(s)*2)
+	for _, r := range s {
+		out = append(out, byte(r>>8), byte(r))
+	}
+	return out
+}
+
+func equalStrings(a, b []string) bool {
+	if len(a) != len(b) {
+		return false
+	}
+	for i := range a {
+		if a[i] != b[i] {
+			return false
+		}
+	}
+	return true
+}