Port upstream PR #357: rune-based track matching + shared LibraryIndex

[joestump-agent]

Ports upstream joestump#357 (commits b04c2df + review-polish e4d4ed7) into the fork, implementing joestump#330.

What was ported

• Rune-based similarity() — both the Levenshtein distance and the max-length denominator are now measured in runes. The fork's version computed a rune-based distance but divided by a byte-based len(string), inflating similarity 2-3x for non-ASCII (CJK/Cyrillic) titles and letting wrong tracks clear the 0.7 fuzzy threshold. Governing: SPEC track-matching REQ-TM-031, ADR-0014.
• Exported LibraryIndex / NormalizedCandidate — the user's library is loaded and normalized once per sync tick via LoadLibraryIndex and shared across playlists via MatchTracksWithIndex (no I/O). MatchTracks keeps its old signature for single-playlist callers.
• SyncAllEnabledPlaylists builds the index once per tick. A tick-level LoadLibraryIndex failure does not abort the tick: it falls back to per-playlist loading so each playlist still reaches the fork's handleSyncError path (sync_status=error, SyncEvent audit log, UI notification per REQ-PLSYNC-060). syncPlaylistToNavidrome also reloads defensively if the supplied index was built for a different user.
• Tests — track_matcher_internal_test.go (rune-unit similarity/levenshtein cases, CJK/Cyrillic low-similarity pins, suffix-variant normalization) and track_matcher_index_test.go (query-counting proof that per-tick library queries don't scale with playlist count, CJK/Cyrillic no-false-match regressions, shared-index vs legacy benchmarks).

Intentional behavior change: per-tick library snapshot

Within a single sync tick, all playlists now match against a library snapshot taken at tick start. Previously each playlist reloaded the library fresh, so a track that gained a navidrome_id mid-tick (e.g. a concurrent Navidrome sync finishing) could be picked up by later playlists in the same tick. With the shared index, such tracks are picked up on the next tick instead. This staleness is bounded by one tick interval (default 1h), is the deliberate trade-off behind the per-tick performance win in issue joestump#330, and matches upstream's semantics. The index is read-only after construction and the playlist loop is sequential, so there is no concurrent-mutation concern.

Gap analysis (what the fork already had)

• playlist_sync.min_match_confidence already defaulted to 0.7 in the fork (upstream was aligning 0.8 → 0.7), so that part reduced to a comment no-op; the fork's governing comment was kept. One adjacent comment fix was kept: the fork's vibes.min_match_confidence comment claimed 0.7 was "lower than playlist sync", which was stale — both are 0.7.
• The fork's own features were preserved through the port: regex-based suffix normalization (featParenRe/featBareRe/yearRemasterRe), per-strategy metric.track_match structured metrics (ADR-0019, REQ MATCH-001/002/003), and library scoping through the artist→user edge.

Review findings on the upstream change

1. MatchTracksWithIndex nil-index panic — evaluated, kept, now documented. The method performs no I/O and cannot fail; a nil index is a programmer contract violation that would nil-deref panic at idx.Size() anyway. The explicit panic just upgrades the message. All in-repo callers guarantee non-nil (syncPlaylistToNavidrome loads on demand when the shared index is nil or for the wrong user). Per adversarial review feedback, the non-nil INVARIANT is now stated in the method's doc comment — including the warning that an unguarded caller in a background goroutine would crash the server — without changing the signature.
2. levenshtein operates on runes — confirmed. After the port it takes []rune parameters and all indexing/comparison is in rune units; similarity divides rune distance by rune max length. (Pre-port, the fork's levenshtein already converted to runes internally, but similarity used byte lengths for the denominator — the exact mismatch Fix track-matching correctness (rune/byte) and per-tick performance joestump/spotter#330 describes.)
3. No correctness defects found requiring fixes. The shared index is read-only after construction and the sync loop is sequential, so no race. Fuzzy candidates all come from a NavidromeIDNotNil query, so the removed per-candidate nil check was genuinely redundant.
4. Known pre-existing issue flagged, not fixed (out of scope): the same rune-distance/byte-maxLen mismatch exists in internal/vibes/generator.go similarity(). Upstream deliberately deferred consolidating the vibes matcher onto NormalizedCandidate to story Deduplicate mixtape generation; unify vibes fuzzy matching with track matcher joestump/spotter#340; recommend a follow-up issue in this fork.

Post-review additions (non-blocking items from the adversarial review)

• Doc-commented the MatchTracksWithIndex non-nil index invariant (see finding 1).
• Added TestPlaylistSyncService_SyncAllEnabledPlaylists_LibraryIndexLoadFailure: injects an Ent driver that fails only tracks-table queries so the tick-level LoadLibraryIndex fails, then asserts the tick does not abort — the fallback issues exactly one per-playlist load per playlist (query-count proof: 1 tick-level + 2 per-playlist failed loads) and every due playlist ends with sync_status=error, its sync_error recording the library-index failure, and a playlist_sync_failed SyncEvent (REQ-PLSYNC-060).

Test results

make generate            OK (templ)
go build ./...           OK
go vet ./...             OK
gofmt -l internal/ cmd/  clean
go test ./internal/services/...   ok
go test ./...            31 packages ok, 0 failures

Benchmarks (5 iterations, 500-track library, 25 fuzzy source tracks):

BenchmarkMatchTracksSharedIndex-4   56.6ms/op
BenchmarkMatchTracksLegacy-4        75.1ms/op

🤖 Generated with Claude Code

[joestump-agent]

VERDICT: APPROVE (submitted as COMMENT — GitHub rejects APPROVE because the review identity is the PR author; zero blocking findings.)

Adversarial review of the rune-similarity fix + shared LibraryIndex port (upstream joestump#357 / issue joestump#330). I tried to break the rune math, the per-tick index sharing, user scoping, the failure fallback, and the new tests. No blocking findings; three non-blocking notes.

Verification on the branch: make generate clean, go build ./... clean, go vet ./... clean, gofmt -l . clean, go test ./... all green (services package re-run with -count=1).

Findings

1. NON-BLOCKING — internal/services/track_matcher.go:160-163: MatchTracksWithIndex panics on a nil index. The scheduler's per-user goroutines in cmd/server/main.go:261-278 have no recover(), so a future caller that passes nil (e.g. someone forgetting the nil-guard that syncPlaylistToNavidrome has at playlist_sync.go:200) crashes the entire server process from a background tick, not just that user's sync. All current callers guard correctly, so this is a latent-API hazard, not a live bug. An error return (or documented invariant plus the panic) would be safer for the "other fuzzy-matching consumers" the exported type is advertised to.

2. NON-BLOCKING — internal/services/playlist_sync.go:457-463: the tick-level LoadLibraryIndex failure fallback (nil index → per-playlist load → handleSyncError → sync_status=error + SyncEvent + notification) is correct by code reading, but has no test. A test that injects a failing driver into the matcher and asserts each playlist ends in sync_status=error with a playlist_sync_failed SyncEvent would pin the REQ-PLSYNC-060 behavior the comment promises.

3. NON-BLOCKING — internal/services/playlist_sync.go:449-463: behavior change worth stating in the PR description: within one tick, all playlists now match against a library snapshot taken at tick start. Tracks gaining navidrome_id mid-tick (concurrent Navidrome library sync) won't match until the next tick, whereas the old code reloaded fresh per playlist. Bounded by the sync interval and inherent to issue joestump#330's design, so acceptable — just no longer "always fresh".

Probed and could not break

• Rune math, hand-computed: library "夜に駆ける"/YOASOBI vs source "群青"/YOASOBI — old code: title 1−5/15 = 0.667 (rune distance / byte maxLen), weighted 0.6·0.667 + 0.4·1.0 = 0.8 ≥ 0.7 → false match. New code: title 0.0, weighted 0.4 → unmatched. Similarity is now provably in [0,1] (rune distance ≤ rune maxLen); ASCII scores are bit-identical to before (TestSimilarity_ASCII_UnchangedByRuneFix). Both-empty → 1.0 via slices.Equal, one-empty → 0.0, no division by zero.
• Test quality, mechanically verified: I locally re-introduced the byte-based maxLen (len(string(a))) and re-ran the new tests — TestSimilarity_CJK_DissimilarTitlesScoreZero, TestSimilarity_Cyrillic_DissimilarTitlesScoreLow, TestTrackMatcher_CJK_DissimilarTitlesDoNotMatch, and TestTrackMatcher_Cyrillic_DissimilarTitlesDoNotMatch all FAIL under the reverted math and pass on the branch. The tests genuinely pin the fix.
• User scoping: SyncAllEnabledPlaylists only queries playlists with HasUserWith(user.ID(userID)) (playlist_sync.go:424-431), so the shared index user always equals the playlist owner; the belt-and-suspenders guard libraryIndex == nil || libraryIndex.UserID != u.ID at playlist_sync.go:200 reloads on any mismatch. LoadLibraryIndex keeps the Track→Artist→User edge filter (track_matcher.go:87-93). I could not construct a cross-user leak.
• Concurrency: runPerUserTick runs users in parallel but each SyncAllEnabledPlaylists call builds its own index and syncs its playlists sequentially; the index is never mutated after LoadLibraryIndex returns.
• Fork regressions: regex suffix normalization (featParenRe/featBareRe/yearRemasterRe) untouched; metric.track_match emission preserved for isrc/exact/fuzzy/none paths in MatchTracksWithIndex; the removed NavidromeID == nil skip in findBestFuzzyMatch is safe because LoadLibraryIndex filters NavidromeIDNotNil at the query.
• Query-count test: loadCost is measured empirically then asserted as playlists*loadCost for the legacy path — robust to eager-load query-shape changes.
• Config: vibes.min_match_confidence change is comment-only (value stays 0.7, now accurately described as equal to the playlist-sync default).

---

Generated by Claude Code