Fix track-matching correctness (rune/byte) and per-tick performance (#330)

[joestump]

Part of epic #317. Governing: ADR-0014 (three-tier track matching), SPEC track-matching REQ-TM-031/REQ-TM-041, SPEC playlist-sync-navidrome REQ-PLSYNC-003.

What / Why

1. Rune/byte similarity mismatch (correctness bug)

similarity() divided a rune-based Levenshtein distance by a byte-based len(). For non-ASCII titles (CJK = 3 bytes/rune, Cyrillic = 2 bytes/rune) this inflated similarity 2-3x, so entirely dissimilar titles cleared the fuzzy threshold and wrong tracks were written into synced Navidrome playlists. Example: library "夜に駆ける" vs source "群青" by the same artist scored 0.6*(1-5/15) + 0.4*1.0 = 0.8 (match!) instead of 0.6*0.0 + 0.4*1.0 = 0.4 (no match). similarity() and levenshtein() now operate on []rune throughout — distance and max length are both in rune units.

2. Per-candidate re-normalization (O(sources x library))

findBestFuzzyMatch re-ran normalizeForMatch (~30 TrimSuffix passes + rune scan) for every candidate for every unmatched source track. Normalization now happens once per library track when building the index.

3. Library reloaded per playlist (O(playlists x library) DB cost)

SyncAllEnabledPlaylists -> SyncPlaylistToNavidrome -> MatchTracks re-ran the full unbounded library query per playlist. New exported TrackMatcher.LoadLibraryIndex(ctx, userID) builds a LibraryIndex (ISRC map, exact-match map, normalized fuzzy candidates) once; MatchTracksWithIndex matches against it with zero DB queries. SyncAllEnabledPlaylists builds the index once per tick and shares it across all playlists. MatchTracks keeps its old signature (load + match) for single-playlist callers (manual sync, pair, rebuild).

Design note for #340: LibraryIndex / NormalizedCandidate are exported precisely so the vibes fuzzy matcher can unify onto this stack instead of keeping its duplicate implementation — the cache is a reusable type, not buried in one function.

4. Confidence default aligned to 0.7

ADR-0014 and REQ-PLSYNC-003 both say default 0.7; internal/config/config.go set playlist_sync.min_match_confidence to 0.8. Git history shows 0.8 came from the original "Got syncing to Navidrome working" bootstrap commit (db9844d) with no recorded rationale, while the ADR's 0.7 is a deliberate, documented decision. Decision: config now defaults to 0.7 (the ADR value), with a governing comment. The stale vibes comment ("Lower than playlist sync") was updated since the two defaults are now equal.

Test evidence

• TestSimilarity_CJK_DissimilarTitlesScoreZero / TestSimilarity_Cyrillic_DissimilarTitlesScoreLow — dissimilar CJK titles now score 0.0 (previously ~0.67 via byte inflation)
• TestTrackMatcher_CJK_DissimilarTitlesDoNotMatch / TestTrackMatcher_Cyrillic_DissimilarTitlesDoNotMatch — end-to-end regression: same-artist dissimilar non-ASCII titles report MatchMethod=none at the 0.7 threshold (old code matched them)
• TestFindBestFuzzyMatch_SuffixVariantsStillMatch + existing TestTrackMatcher_FuzzyMatch_Remastered / _RadioEdit — remaster/radio-edit/live suffix variants still match
• TestSimilarity_ASCII_UnchangedByRuneFix — ASCII scoring is bit-identical to the old behavior
• Query-count proof (TestLibraryIndex_LoadedOncePerTick, via a counting dialect.Driver): matching 5 playlists with the shared index issues exactly 1 library load (2 SQL queries: tracks + eager artist edge) and zero per-playlist queries; the legacy path issues 5x that. Per-tick DB cost no longer scales with playlist count x library size.
• Benchmarks (500-track library, 25 fuzzy-path sources, Apple M4): BenchmarkMatchTracksSharedIndex 21.5ms/op vs BenchmarkMatchTracksLegacy 25.3ms/op — and the shared-index path amortizes its one-time index build across all playlists in the tick, while legacy pays the query + re-normalization on every playlist.

make test green; gofmt -l . clean; go vet clean. (make lint-go fails identically on origin/main — local golangci-lint v2 binary rejects the repo's v1 config; pre-existing, unrelated.)

Deferred Design Doc Updates

Spec paths are protected; the following governing-doc edits are deferred to a docs-owner pass:

• docs/openspec/specs/track-matching/spec.md REQ-TM-031: pin rune units for both the Levenshtein distance and the max string length (currently says 1.0 - (levenshtein_distance / max_string_length) without units — too imprecise to catch this bug class)
• docs/openspec/specs/track-matching/spec.md config reference: document playlist_sync.min_match_confidence default 0.7 (per ADR-0014 / REQ-PLSYNC-003), now aligned in internal/config/config.go
• docs/adrs/ADR-0014-three-tier-track-matching-algorithm.md: "More Information" section references findBestFuzzyMatch() iterating raw *ent.Track candidates; could be refreshed to mention LibraryIndex/NormalizedCandidate and per-tick index reuse

Closes #330

🤖 Posted on behalf of @joestump by Claude.

[joestump]

Adversarial review — PR #357 (issue #330)

Verdict: APPROVE — no blocking defects found. The rune/byte fix, the normalization index, the per-tick library load, and the 0.7 alignment are all correct and well-tested. Findings below are non-blocking.

What was verified adversarially (independent checkout, make test re-run green; gofmt/go vet clean)

• ASCII parity: fuzzed 5,000 random ASCII pairs through the old implementation (rune distance / byte max-length, a == b fast path) vs the new one — bit-identical. The rune fix alone causes zero rematch drift for pure-ASCII libraries.
• Edge semantics preserved: both-empty → 1.0 (old a == b path), one-empty → 0.0, one-rune titles → 0.0/1.0 as expected. Cached normalized forms are byte-identical to what the old per-call path produced (same normalizeForMatch, same suffix ordering).
• Multi-user safety (Add per-user scoping to unscoped query paths (cross-user leakage) #338 leakage class): SyncAllEnabledPlaylists queries playlists with playlist.HasUserWith(user.ID(userID)) (playlist_sync.go:408-414), so every playlist handed the shared index belongs to userID; the libraryIndex.UserID != u.ID guard (playlist_sync.go:200) reloads defensively on any mismatch. No path lets one user's index match another user's playlists. Verified LoadLibraryIndex filters track.HasArtistWith(artist.HasUserWith(user.ID(userID))).
• Concurrency: the scheduler (cmd/server/main.go:387-401) runs one goroutine per user, each building its own index; within a tick the playlist loop is sequential and LibraryIndex is never mutated after build. Safe.
• Dropped error return: MatchTracksWithIndex performs no fallible I/O — nothing is swallowed. Single-playlist callers (manual sync, pair, rebuild via SyncPlaylistToNavidrome) route through the nil-index on-demand load and keep prior behavior.
• Query-count test and CJK/Cyrillic regression tests reproduced locally; the counting-driver proof is a nice touch.

Findings

Low — tick-level index-load failure skips per-playlist error handling (playlist_sync.go:435-441). Previously a library-query failure inside MatchTracks hit handleSyncError per playlist: sync_status=error, SyncEvent (REQ-PLSYNC-060), UI notification. Now a failed LoadLibraryIndex at tick start returns before any playlist is touched — statuses stay stale and no SyncEvent is recorded for that tick. Practical impact is small (a DB read failing usually means the error-status writes would fail too), but consider a fallback to the per-playlist path or an explicit SyncEvent on tick-level failure.

Low — quantified false-positive risk of the 0.7 default (ADR-governed, deliberate). Probed hostile same-artist pairs against the new math:

• калинка vs катюша → 0.657 (rejected; 0.043 margin)
• 夜に駆ける vs 夜に溺れる → title 0.60 → 0.76: matches at 0.7, would have been rejected at 0.8
• With an exact artist, any title with rune-similarity ≥ 0.5 now clears the gate exactly (0.6×0.5 + 0.4 = 0.70, and REQ-TM-033 mandates >=)

Short CJK/Cyrillic titles sharing prefix/suffix runes are the exposed class. ADR-0014 explicitly picks 0.7 and acknowledges short-title Levenshtein sensitivity, so this is a recorded trade-off, not a defect — but a follow-up guard (e.g. stricter effective threshold for titles under ~5 runes) would close the residual gap. Note part 1 vs part 2 scores 1.0 via the dual-confidence bonus — that false positive is pre-existing at both thresholds, not introduced here.

Note — stale-index window widened: the index is built at tick start, so tracks added mid-tick (e.g. a concurrent Navidrome library sync) aren't matchable until the next tick, where the old per-playlist reload picked them up sooner. Acceptable trade-off; documenting so it's a known behavior.

Nits (take or leave):

• similarity() dropped the old a == b fast path — identical titles now run the full O(n·m) DP. Tier 2 catches most identical pairs first, so impact is marginal; a string(a) == string(b) (or equal-slice) early-out would restore it (track_matcher.go:428).
• MatchTracksWithIndex takes ctx but never uses it, and a nil idx panics — fine for an internal-facing API, maybe worth a doc line (track_matcher.go:157).
• if t.NavidromeID != nil at track_matcher.go:125 is redundant with track.NavidromeIDNotNil() in the query — harmless defense.

Out of scope but urgent — vibes duplicate matcher (#340)

internal/vibes/generator.go:800-840 retains both the byte/rune bug and an ASCII-only normalizeForMatch that reduces any pure-CJK/Cyrillic title to "" — verified live: two arbitrary CJK titles normalize equal-empty and score 1.0 via the a == b fast path, i.e. any CJK AI suggestion "matches" an arbitrary CJK library track at full confidence. That's a strictly worse instance of the #330 bug class in the AI-mixtape path. #340 already covers unification onto the post-#330 stack — recommend prioritizing it.

Pre-existing, not this PR: plSyncErrors++ in cmd/server/main.go races across per-user goroutines (and the metric line logs it before they finish); match_rate logs NaN when len(tracks)==0.

Process note: issue #330's spec-update checkbox (REQ-TM-031 rune units) is deferred to a docs-owner pass due to protected spec paths — the PR body records exactly what to change; make sure that pass lands before closing epic #317's correctness story.

🤖 Posted on behalf of @joestump by Claude.