From b04c2df46c53df3e7a855c689fc3ea6f6d2c0475 Mon Sep 17 00:00:00 2001 From: Joe Stump Date: Fri, 10 Jul 2026 07:45:04 +0100 Subject: [PATCH 1/2] Fix track-matching rune/byte similarity and per-tick performance Implements #330. - similarity() now measures both Levenshtein distance and max length in runes; the byte-based max length inflated similarity 2-3x for non-ASCII (CJK/Cyrillic) titles, letting wrong tracks clear the fuzzy threshold (SPEC track-matching REQ-TM-031, ADR-0014) - New exported LibraryIndex/NormalizedCandidate: the user's library is loaded once and each candidate's title/artist is normalized once, instead of re-running normalizeForMatch per candidate per source track; reusable by other fuzzy-match consumers (story #340) - SyncAllEnabledPlaylists loads the library index once per sync tick and shares it across playlists; MatchTracks keeps its old signature for single-playlist callers - playlist_sync.min_match_confidence default aligned to 0.7 per ADR-0014 and REQ-PLSYNC-003 (was 0.8 with no recorded rationale) - Tests: rune-unit similarity/levenshtein cases, CJK/Cyrillic no-false-match regressions, suffix-variant matches, query-counting proof that per-tick library queries no longer scale with playlist count, plus shared-index vs legacy benchmarks Co-Authored-By: Claude Fable 5 --- internal/config/config.go | 5 +- internal/services/playlist_sync.go | 33 ++- internal/services/track_matcher.go | 212 ++++++++------ internal/services/track_matcher_index_test.go | 264 ++++++++++++++++++ .../services/track_matcher_internal_test.go | 116 ++++++++ 5 files changed, 546 insertions(+), 84 deletions(-) create mode 100644 internal/services/track_matcher_index_test.go create mode 100644 internal/services/track_matcher_internal_test.go diff --git a/internal/config/config.go b/internal/config/config.go index dbdc354..79b8037 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -402,7 +402,8 @@ func newViper() *viper.Viper { // Playlist sync defaults v.SetDefault("playlist_sync.sync_interval", "1h") v.SetDefault("playlist_sync.delete_on_unsync", false) - v.SetDefault("playlist_sync.min_match_confidence", 0.8) + // Governing: ADR-0014, SPEC playlist-sync-navidrome REQ-PLSYNC-003 (default 0.7; issue #330 aligned config with the ADR) + v.SetDefault("playlist_sync.min_match_confidence", 0.7) v.SetDefault("playlist_sync.include_unmatched_tracks", false) // Vibes (mixtape generation) defaults @@ -416,7 +417,7 @@ func newViper() *viper.Viper { v.SetDefault("vibes.max_tokens", 4000) // Enough for track list + explanations v.SetDefault("vibes.timeout_seconds", 120) // 2 minutes v.SetDefault("vibes.prompts_directory", "") // Falls back to metadata.ai.prompts_directory - v.SetDefault("vibes.min_match_confidence", 0.7) // Lower than playlist sync for more matches + v.SetDefault("vibes.min_match_confidence", 0.7) // Same as playlist sync default (ADR-0014) // Metadata enrichment defaults v.SetDefault("metadata.enabled", true) diff --git a/internal/services/playlist_sync.go b/internal/services/playlist_sync.go index 5390e4c..c4f53f7 100644 --- a/internal/services/playlist_sync.go +++ b/internal/services/playlist_sync.go @@ -59,6 +59,14 @@ func (s *PlaylistSyncService) Register(factory providers.Factory) { // SPEC playlist-sync-navidrome REQ-PLSYNC-032 (remotePlaylistID stored on playlist entity), // SPEC playlist-sync-navidrome REQ-PLSYNC-060 (SyncEvent audit logging) func (s *PlaylistSyncService) SyncPlaylistToNavidrome(ctx context.Context, playlistID int) error { + return s.syncPlaylistToNavidrome(ctx, playlistID, nil) +} + +// syncPlaylistToNavidrome performs the sync. libraryIndex may be nil (it is +// loaded on demand); callers syncing many playlists in one tick (issue #330) +// pass a shared index so the user's library is loaded once per tick instead +// of once per playlist. +func (s *PlaylistSyncService) syncPlaylistToNavidrome(ctx context.Context, playlistID int, libraryIndex *LibraryIndex) error { startTime := time.Now() s.logger.Info("starting playlist sync to Navidrome", @@ -187,11 +195,17 @@ func (s *PlaylistSyncService) SyncPlaylistToNavidrome(ctx context.Context, playl "playlist_id", playlistID, "source_track_count", len(sourceTracks)) - matchResults, err := s.trackMatcher.MatchTracks(ctx, u.ID, sourceTracks) - if err != nil { - return s.handleSyncError(ctx, pl, u, fmt.Errorf("failed to match tracks: %w", err)) + // Load the library index on demand if the caller didn't supply a shared + // one (or supplied one built for a different user). + if libraryIndex == nil || libraryIndex.UserID != u.ID { + libraryIndex, err = s.trackMatcher.LoadLibraryIndex(ctx, u.ID) + if err != nil { + return s.handleSyncError(ctx, pl, u, fmt.Errorf("failed to load library index: %w", err)) + } } + matchResults := s.trackMatcher.MatchTracksWithIndex(ctx, libraryIndex, sourceTracks) + // Filter to only matched tracks (we can only add tracks that exist in Navidrome) var matchedTracks []providers.Track matchedCount := 0 @@ -415,10 +429,21 @@ func (s *PlaylistSyncService) SyncAllEnabledPlaylists(ctx context.Context, userI return nil } + // Governing: ADR-0014; issue #330 — load the user's library once per sync + // tick and share the index across all playlists, instead of re-running the + // full library query per playlist. + libraryIndex, err := s.trackMatcher.LoadLibraryIndex(ctx, userID) + if err != nil { + s.logger.Error("failed to load library index for sync tick", + "user_id", userID, + "error", err) + return fmt.Errorf("failed to load library index: %w", err) + } + var syncErrors []error successCount := 0 for _, pl := range playlists { - if err := s.SyncPlaylistToNavidrome(ctx, pl.ID); err != nil { + if err := s.syncPlaylistToNavidrome(ctx, pl.ID, libraryIndex); err != nil { s.logger.Error("failed to sync playlist", "user_id", userID, "playlist_id", pl.ID, diff --git a/internal/services/track_matcher.go b/internal/services/track_matcher.go index 9a233b8..5d93245 100644 --- a/internal/services/track_matcher.go +++ b/internal/services/track_matcher.go @@ -49,17 +49,37 @@ func NewTrackMatcher(client *ent.Client, logger *slog.Logger, minConfidence floa } } -// MatchTracks attempts to find Navidrome track IDs for source tracks. -// Uses multiple strategies: ISRC matching, exact name match, fuzzy matching. -func (m *TrackMatcher) MatchTracks(ctx context.Context, userID int, tracks []providers.Track) ([]MatchResult, error) { - startTime := time.Now() - results := make([]MatchResult, len(tracks)) +// NormalizedCandidate is a library track whose match keys have been normalized +// once, up front. Fuzzy matching compares against the precomputed rune slices +// so per-source-track work never re-runs normalizeForMatch over the library. +// Exported so other fuzzy-matching consumers (e.g. the vibes matcher, story +// #340) can reuse the same candidate representation. +type NormalizedCandidate struct { + Track *ent.Track + TitleRunes []rune // normalizeForMatch(track name), in runes + ArtistRunes []rune // normalizeForMatch(artist name), in runes +} - m.Logger.Info("starting track matching", - "user_id", userID, - "source_track_count", len(tracks), - "min_match_confidence", m.MinMatchConfidence) +// LibraryIndex is a precomputed matching index over a user's Navidrome library. +// Build it once per sync tick with LoadLibraryIndex and reuse it across +// playlists via MatchTracksWithIndex so matching cost no longer scales with +// playlist count x library size. +type LibraryIndex struct { + UserID int + isrcMap map[string]*ent.Track // key: lowercased ISRC + exactMap map[string]*ent.Track // key: normalized "artist|title" + candidates []NormalizedCandidate +} +// Size returns the number of library tracks in the index. +func (idx *LibraryIndex) Size() int { + return len(idx.candidates) +} + +// LoadLibraryIndex loads the user's Navidrome-linked library once and builds +// the lookup maps and normalized fuzzy-match candidates. +// Governing: ADR-0014 (lookup maps for tiers 1-2, normalized candidates for tier 3) +func (m *TrackMatcher) LoadLibraryIndex(ctx context.Context, userID int) (*LibraryIndex, error) { // Get all tracks for this user's library (tracks that have a navidrome_id) // FIX: Filter by user through the artist edge: Track -> Artist -> User libraryTracks, err := m.Client.Track.Query(). @@ -76,49 +96,89 @@ func (m *TrackMatcher) MatchTracks(ctx context.Context, userID int, tracks []pro return nil, err } - m.Logger.Debug("loaded library tracks for matching", - "user_id", userID, - "library_track_count", len(libraryTracks)) - - if len(libraryTracks) == 0 { - m.Logger.Warn("no tracks with navidrome_id found for user", - "user_id", userID, - "hint", "ensure Navidrome sync has run to populate navidrome_id on tracks") - - // Return all results as unmatched - for i, sourceTrack := range tracks { - results[i] = MatchResult{ - SourceTrack: sourceTrack, - MatchConfidence: 0.0, - MatchMethod: MatchMethodNone, - } - } - return results, nil + idx := &LibraryIndex{ + UserID: userID, + isrcMap: make(map[string]*ent.Track), + exactMap: make(map[string]*ent.Track), + candidates: make([]NormalizedCandidate, 0, len(libraryTracks)), } - // Build lookup maps for efficient matching - isrcMap := make(map[string]*ent.Track) - exactMap := make(map[string]*ent.Track) // key: normalized "artist|title" - for _, t := range libraryTracks { // ISRC map (if available) if t.Isrc != nil && *t.Isrc != "" { isrcKey := strings.ToLower(*t.Isrc) - isrcMap[isrcKey] = t + idx.isrcMap[isrcKey] = t } - // Exact match map artistName := "" if t.Edges.Artist != nil { artistName = t.Edges.Artist.Name } - key := normalizeForMatch(artistName) + "|" + normalizeForMatch(t.Name) - exactMap[key] = t + normalizedTitle := normalizeForMatch(t.Name) + normalizedArtist := normalizeForMatch(artistName) + + // Exact match map + key := normalizedArtist + "|" + normalizedTitle + idx.exactMap[key] = t + + // Fuzzy match candidate with precomputed normalized runes + if t.NavidromeID != nil { + idx.candidates = append(idx.candidates, NormalizedCandidate{ + Track: t, + TitleRunes: []rune(normalizedTitle), + ArtistRunes: []rune(normalizedArtist), + }) + } + } + + m.Logger.Debug("built library index", + "user_id", userID, + "library_track_count", len(libraryTracks), + "isrc_map_size", len(idx.isrcMap), + "exact_map_size", len(idx.exactMap)) + + return idx, nil +} + +// MatchTracks attempts to find Navidrome track IDs for source tracks. +// Uses multiple strategies: ISRC matching, exact name match, fuzzy matching. +// It loads the library index on every call; callers matching multiple +// playlists in one tick should use LoadLibraryIndex + MatchTracksWithIndex. +func (m *TrackMatcher) MatchTracks(ctx context.Context, userID int, tracks []providers.Track) ([]MatchResult, error) { + idx, err := m.LoadLibraryIndex(ctx, userID) + if err != nil { + return nil, err } + return m.MatchTracksWithIndex(ctx, idx, tracks), nil +} + +// MatchTracksWithIndex matches source tracks against a precomputed LibraryIndex. +// Governing: ADR-0014 (three-tier ISRC -> exact -> fuzzy matching) +func (m *TrackMatcher) MatchTracksWithIndex(ctx context.Context, idx *LibraryIndex, tracks []providers.Track) []MatchResult { + startTime := time.Now() + results := make([]MatchResult, len(tracks)) + + m.Logger.Info("starting track matching", + "user_id", idx.UserID, + "source_track_count", len(tracks), + "library_track_count", idx.Size(), + "min_match_confidence", m.MinMatchConfidence) + + if idx.Size() == 0 { + m.Logger.Warn("no tracks with navidrome_id found for user", + "user_id", idx.UserID, + "hint", "ensure Navidrome sync has run to populate navidrome_id on tracks") - m.Logger.Debug("built lookup maps", - "isrc_map_size", len(isrcMap), - "exact_map_size", len(exactMap)) + // Return all results as unmatched + for i, sourceTrack := range tracks { + results[i] = MatchResult{ + SourceTrack: sourceTrack, + MatchConfidence: 0.0, + MatchMethod: MatchMethodNone, + } + } + return results + } // Track match statistics by method matchStats := map[MatchMethod]int{ @@ -141,7 +201,7 @@ func (m *TrackMatcher) MatchTracks(ctx context.Context, userID int, tracks []pro // Strategy 1: ISRC matching (highest confidence) if sourceTrack.ISRC != "" { isrcKey := strings.ToLower(sourceTrack.ISRC) - if matchedTrack, ok := isrcMap[isrcKey]; ok { + if matchedTrack, ok := idx.isrcMap[isrcKey]; ok { if matchedTrack.NavidromeID != nil { result.NavidromeTrackID = *matchedTrack.NavidromeID result.MatchConfidence = 1.0 @@ -166,8 +226,10 @@ func (m *TrackMatcher) MatchTracks(ctx context.Context, userID int, tracks []pro } // Strategy 2: Exact match (artist + title) - exactKey := normalizeForMatch(sourceTrack.Artist) + "|" + normalizeForMatch(sourceTrack.Name) - if matchedTrack, ok := exactMap[exactKey]; ok { + normalizedTitle := normalizeForMatch(sourceTrack.Name) + normalizedArtist := normalizeForMatch(sourceTrack.Artist) + exactKey := normalizedArtist + "|" + normalizedTitle + if matchedTrack, ok := idx.exactMap[exactKey]; ok { if matchedTrack.NavidromeID != nil { result.NavidromeTrackID = *matchedTrack.NavidromeID result.MatchConfidence = 1.0 @@ -190,7 +252,7 @@ func (m *TrackMatcher) MatchTracks(ctx context.Context, userID int, tracks []pro } // Strategy 3: Fuzzy matching - bestMatch, confidence := m.findBestFuzzyMatch(sourceTrack, libraryTracks) + bestMatch, confidence := findBestFuzzyMatch([]rune(normalizedTitle), []rune(normalizedArtist), idx.candidates) if bestMatch != nil && confidence >= m.MinMatchConfidence { if bestMatch.NavidromeID != nil { result.NavidromeTrackID = *bestMatch.NavidromeID @@ -246,7 +308,7 @@ func (m *TrackMatcher) MatchTracks(ctx context.Context, userID int, tracks []pro duration := time.Since(startTime) m.Logger.Info("track matching complete", - "user_id", userID, + "user_id", idx.UserID, "total_tracks", len(tracks), "matched_tracks", matched, "unmatched_tracks", len(tracks)-matched, @@ -256,31 +318,20 @@ func (m *TrackMatcher) MatchTracks(ctx context.Context, userID int, tracks []pro "matches_by_fuzzy", matchStats[MatchMethodFuzzy], "duration_ms", duration.Milliseconds()) - return results, nil + return results } -// findBestFuzzyMatch finds the best fuzzy match for a source track. -func (m *TrackMatcher) findBestFuzzyMatch(source providers.Track, candidates []*ent.Track) (*ent.Track, float64) { +// findBestFuzzyMatch finds the best fuzzy match for a normalized source +// title/artist among precomputed candidates. +// Governing: SPEC track-matching REQ-TM-031, REQ-TM-032 (weighted score + dual-confidence bonus) +func findBestFuzzyMatch(sourceTitle, sourceArtist []rune, candidates []NormalizedCandidate) (*ent.Track, float64) { var bestMatch *ent.Track bestScore := 0.0 - sourceTitle := normalizeForMatch(source.Name) - sourceArtist := normalizeForMatch(source.Artist) - for _, candidate := range candidates { - if candidate.NavidromeID == nil { - continue - } - - candidateTitle := normalizeForMatch(candidate.Name) - candidateArtist := "" - if candidate.Edges.Artist != nil { - candidateArtist = normalizeForMatch(candidate.Edges.Artist.Name) - } - // Calculate similarity scores - titleScore := similarity(sourceTitle, candidateTitle) - artistScore := similarity(sourceArtist, candidateArtist) + titleScore := similarity(sourceTitle, candidate.TitleRunes) + artistScore := similarity(sourceArtist, candidate.ArtistRunes) // Weighted average: title is more important score := (titleScore * 0.6) + (artistScore * 0.4) @@ -295,7 +346,7 @@ func (m *TrackMatcher) findBestFuzzyMatch(source providers.Track, candidates []* if score > bestScore { bestScore = score - bestMatch = candidate + bestMatch = candidate.Track } } @@ -365,17 +416,24 @@ func normalizeForMatch(s string) string { return strings.TrimSpace(result.String()) } -// similarity calculates the similarity between two strings using Levenshtein distance. -// Returns a value between 0.0 (completely different) and 1.0 (identical). -func similarity(a, b string) float64 { - if a == b { - return 1.0 - } +// similarity calculates the similarity between two rune slices using +// Levenshtein distance. Returns a value between 0.0 (completely different) +// and 1.0 (identical). +// +// Both the edit distance and the maximum length are measured in RUNES. +// Mixing a rune-based distance with a byte-based max length inflated +// similarity 2-3x for non-ASCII (CJK/Cyrillic) titles, letting wrong tracks +// clear the fuzzy threshold. +// Governing: SPEC track-matching REQ-TM-031 (issue #330: distance and max length in rune units) +func similarity(a, b []rune) float64 { if len(a) == 0 || len(b) == 0 { + if len(a) == 0 && len(b) == 0 { + return 1.0 + } return 0.0 } - // Calculate Levenshtein distance + // Calculate Levenshtein distance (in runes) distance := levenshtein(a, b) maxLen := len(a) if len(b) > maxLen { @@ -385,18 +443,16 @@ func similarity(a, b string) float64 { return 1.0 - float64(distance)/float64(maxLen) } -// levenshtein calculates the Levenshtein distance between two strings. -func levenshtein(a, b string) int { - if len(a) == 0 { - return len(b) +// levenshtein calculates the Levenshtein distance between two rune slices, +// measured in rune edits. +func levenshtein(aRunes, bRunes []rune) int { + if len(aRunes) == 0 { + return len(bRunes) } - if len(b) == 0 { - return len(a) + if len(bRunes) == 0 { + return len(aRunes) } - aRunes := []rune(a) - bRunes := []rune(b) - // Create distance matrix d := make([][]int, len(aRunes)+1) for i := range d { diff --git a/internal/services/track_matcher_index_test.go b/internal/services/track_matcher_index_test.go new file mode 100644 index 0000000..0190909 --- /dev/null +++ b/internal/services/track_matcher_index_test.go @@ -0,0 +1,264 @@ +package services_test + +import ( + "context" + "fmt" + "io" + "log/slog" + "sync/atomic" + "testing" + + "entgo.io/ent/dialect" + entsql "entgo.io/ent/dialect/sql" + + "spotter/ent" + "spotter/internal/providers" + "spotter/internal/services" + + _ "github.com/mattn/go-sqlite3" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// countingDriver wraps an Ent dialect.Driver and counts read queries so tests +// can prove how many times the library is loaded from the database. +type countingDriver struct { + dialect.Driver + queries atomic.Int64 +} + +func (d *countingDriver) Query(ctx context.Context, query string, args, v any) error { + d.queries.Add(1) + return d.Driver.Query(ctx, query, args, v) +} + +func setupCountingMatcher(t *testing.T, minConfidence float64) (*ent.Client, *countingDriver, *services.TrackMatcher, *ent.User) { + t.Helper() + + drv, err := entsql.Open("sqlite3", + fmt.Sprintf("file:%s?mode=memory&cache=shared&_fk=1", t.Name())) + require.NoError(t, err) + + counting := &countingDriver{Driver: drv} + client := ent.NewClient(ent.Driver(counting)) + t.Cleanup(func() { client.Close() }) + + require.NoError(t, client.Schema.Create(context.Background())) + + logger := slog.New(slog.NewTextHandler(io.Discard, nil)) + matcher := services.NewTrackMatcher(client, logger, minConfidence) + + user, err := client.User.Create(). + SetUsername("indexuser"). + Save(context.Background()) + require.NoError(t, err) + + return client, counting, matcher, user +} + +// Issue #330: per-tick matching cost must not scale with playlist count x +// library size. A shared LibraryIndex is loaded with exactly ONE library +// query and reused across an arbitrary number of MatchTracksWithIndex calls, +// whereas legacy MatchTracks re-queries the library on every call. +func TestLibraryIndex_LoadedOncePerTick(t *testing.T) { + client, counting, matcher, user := setupCountingMatcher(t, 0.7) + ctx := context.Background() + + // Seed a small library. + for i := 0; i < 10; i++ { + createTestTrackWithNavidromeID(t, client, user, + fmt.Sprintf("Library Song %d", i), + fmt.Sprintf("Library Artist %d", i), + fmt.Sprintf("nav-%d", i)) + } + + sourceTracks := []providers.Track{ + {ID: "s-1", Name: "Library Song 1", Artist: "Library Artist 1"}, + {ID: "s-2", Name: "Library Song 2 (Remastered)", Artist: "Library Artist 2"}, + {ID: "s-3", Name: "Nonexistent Song", Artist: "Nobody"}, + } + + const playlists = 5 + + // Measure the cost of a single library load (the track query plus its + // eager-loaded artist edge — 2 SQL queries with the current schema). + counting.queries.Store(0) + idx, err := matcher.LoadLibraryIndex(ctx, user.ID) + require.NoError(t, err) + assert.Equal(t, 10, idx.Size()) + loadCost := counting.queries.Load() + require.Positive(t, loadCost) + + // Shared-index path: matching N playlists issues ZERO additional queries. + for i := 0; i < playlists; i++ { + results := matcher.MatchTracksWithIndex(ctx, idx, sourceTracks) + require.Len(t, results, 3) + assert.Equal(t, "nav-1", results[0].NavidromeTrackID) + assert.Equal(t, "nav-2", results[1].NavidromeTrackID) + assert.Empty(t, results[2].NavidromeTrackID) + } + sharedQueries := counting.queries.Load() + assert.Equal(t, loadCost, sharedQueries, + "shared index should load the library exactly once per tick, with no per-playlist queries") + + // Legacy path: the library is reloaded for every playlist. + counting.queries.Store(0) + for i := 0; i < playlists; i++ { + _, err := matcher.MatchTracks(ctx, user.ID, sourceTracks) + require.NoError(t, err) + } + legacyQueries := counting.queries.Load() + assert.Equal(t, playlists*loadCost, legacyQueries, + "MatchTracks reloads the library on every call") + + assert.Less(t, sharedQueries, legacyQueries, + "per-tick query count must not scale with playlist count") +} + +// Issue #330 regression: with the old byte-based max length, a dissimilar CJK +// title by the same artist scored ~0.8 (0.6*0.67 + 0.4*1.0) and wrongly +// cleared the 0.7 threshold. With rune-based similarity it scores 0.4 and is +// reported unmatched. +func TestTrackMatcher_CJK_DissimilarTitlesDoNotMatch(t *testing.T) { + client, _, matcher, user := setupCountingMatcher(t, 0.7) + ctx := context.Background() + + createTestTrackWithNavidromeID(t, client, user, "夜に駆ける", "YOASOBI", "nav-yoasobi-1") + + sourceTracks := []providers.Track{ + {ID: "s-1", Name: "群青", Artist: "YOASOBI"}, + } + + results, err := matcher.MatchTracks(ctx, user.ID, sourceTracks) + require.NoError(t, err) + require.Len(t, results, 1) + + assert.Empty(t, results[0].NavidromeTrackID, + "dissimilar CJK title must not fuzzy-match, got confidence %f", results[0].MatchConfidence) + assert.Equal(t, services.MatchMethodNone, results[0].MatchMethod) +} + +func TestTrackMatcher_Cyrillic_DissimilarTitlesDoNotMatch(t *testing.T) { + client, _, matcher, user := setupCountingMatcher(t, 0.7) + ctx := context.Background() + + createTestTrackWithNavidromeID(t, client, user, "Калинка", "Русский Хор", "nav-ru-1") + + sourceTracks := []providers.Track{ + {ID: "s-1", Name: "Катюша", Artist: "Русский Хор"}, + } + + results, err := matcher.MatchTracks(ctx, user.ID, sourceTracks) + require.NoError(t, err) + require.Len(t, results, 1) + + assert.Empty(t, results[0].NavidromeTrackID, + "dissimilar Cyrillic title must not fuzzy-match, got confidence %f", results[0].MatchConfidence) + assert.Equal(t, services.MatchMethodNone, results[0].MatchMethod) +} + +// Identical non-ASCII titles must still match exactly. +func TestTrackMatcher_CJK_IdenticalTitlesMatch(t *testing.T) { + client, _, matcher, user := setupCountingMatcher(t, 0.7) + ctx := context.Background() + + createTestTrackWithNavidromeID(t, client, user, "夜に駆ける", "YOASOBI", "nav-yoasobi-1") + + sourceTracks := []providers.Track{ + {ID: "s-1", Name: "夜に駆ける", Artist: "YOASOBI"}, + } + + results, err := matcher.MatchTracks(ctx, user.ID, sourceTracks) + require.NoError(t, err) + require.Len(t, results, 1) + + assert.Equal(t, "nav-yoasobi-1", results[0].NavidromeTrackID) + assert.Equal(t, 1.0, results[0].MatchConfidence) +} + +// BenchmarkMatchTracksSharedIndex measures matching with a precomputed +// LibraryIndex (normalization done once per candidate, no per-call DB query). +func BenchmarkMatchTracksSharedIndex(b *testing.B) { + client, matcher, user, sourceTracks := setupBenchmarkLibrary(b) + defer client.Close() + ctx := context.Background() + + idx, err := matcher.LoadLibraryIndex(ctx, user.ID) + if err != nil { + b.Fatal(err) + } + + b.ResetTimer() + for i := 0; i < b.N; i++ { + matcher.MatchTracksWithIndex(ctx, idx, sourceTracks) + } +} + +// BenchmarkMatchTracksLegacy measures the legacy per-playlist path that +// reloads the library and re-normalizes every candidate on every call. +func BenchmarkMatchTracksLegacy(b *testing.B) { + client, matcher, user, sourceTracks := setupBenchmarkLibrary(b) + defer client.Close() + ctx := context.Background() + + b.ResetTimer() + for i := 0; i < b.N; i++ { + if _, err := matcher.MatchTracks(ctx, user.ID, sourceTracks); err != nil { + b.Fatal(err) + } + } +} + +func setupBenchmarkLibrary(b *testing.B) (*ent.Client, *services.TrackMatcher, *ent.User, []providers.Track) { + b.Helper() + + drv, err := entsql.Open("sqlite3", + fmt.Sprintf("file:%s?mode=memory&cache=shared&_fk=1", b.Name())) + if err != nil { + b.Fatal(err) + } + client := ent.NewClient(ent.Driver(drv)) + ctx := context.Background() + if err := client.Schema.Create(ctx); err != nil { + b.Fatal(err) + } + + logger := slog.New(slog.NewTextHandler(io.Discard, nil)) + matcher := services.NewTrackMatcher(client, logger, 0.7) + + user, err := client.User.Create().SetUsername("benchuser").Save(ctx) + if err != nil { + b.Fatal(err) + } + + const librarySize = 500 + for i := 0; i < librarySize; i++ { + artistEnt, err := client.Artist.Create(). + SetName(fmt.Sprintf("Bench Artist %d (Deluxe Edition)", i)). + SetUser(user). + Save(ctx) + if err != nil { + b.Fatal(err) + } + navID := fmt.Sprintf("nav-bench-%d", i) + if _, err := client.Track.Create(). + SetName(fmt.Sprintf("Bench Song %d (Remastered)", i)). + SetArtist(artistEnt). + SetNillableNavidromeID(&navID). + Save(ctx); err != nil { + b.Fatal(err) + } + } + + // Source tracks that miss the exact map and exercise the fuzzy path. + sourceTracks := make([]providers.Track, 25) + for i := range sourceTracks { + sourceTracks[i] = providers.Track{ + ID: fmt.Sprintf("bench-src-%d", i), + Name: fmt.Sprintf("Bench Songg %d", i), + Artist: fmt.Sprintf("Bench Artistt %d", i), + } + } + + return client, matcher, user, sourceTracks +} diff --git a/internal/services/track_matcher_internal_test.go b/internal/services/track_matcher_internal_test.go new file mode 100644 index 0000000..02f9baa --- /dev/null +++ b/internal/services/track_matcher_internal_test.go @@ -0,0 +1,116 @@ +package services + +import ( + "testing" +) + +// Governing: SPEC track-matching REQ-TM-031 (similarity measured in rune units) +// Issue #330: similarity() previously divided a rune-based Levenshtein +// distance by a byte-based max length, inflating scores 2-3x for non-ASCII +// titles. These tests pin the rune-based behavior. + +func runeSim(a, b string) float64 { + return similarity([]rune(a), []rune(b)) +} + +func TestSimilarity_CJK_DissimilarTitlesScoreZero(t *testing.T) { + // "夜に駆ける" (5 runes) vs "群青" (2 runes): every rune differs. + // Rune-based: distance 5 / maxLen 5 = similarity 0.0. + // Byte-based maxLen (15 bytes) would have inflated this to ~0.67. + score := runeSim("夜に駆ける", "群青") + if score > 0.05 { + t.Fatalf("expected ~0.0 similarity for dissimilar CJK titles, got %f", score) + } + + // "君の名は" vs "千本桜": no runes in common. + score = runeSim("君の名は", "千本桜") + if score > 0.05 { + t.Fatalf("expected ~0.0 similarity for dissimilar CJK titles, got %f", score) + } +} + +func TestSimilarity_Cyrillic_DissimilarTitlesScoreLow(t *testing.T) { + // "калинка" vs "катюша": shares only leading "ка" and trailing "а". + // Rune-based maxLen is 7; byte-based would have been 14, halving the + // apparent distance ratio. + score := runeSim("калинка", "катюша") + if score > 0.45 { + t.Fatalf("expected low similarity for dissimilar Cyrillic titles, got %f", score) + } + + // Fully dissimilar Cyrillic strings should score ~0.0. + score = runeSim("жизнь", "туман") + if score > 0.05 { + t.Fatalf("expected ~0.0 similarity for dissimilar Cyrillic titles, got %f", score) + } +} + +func TestSimilarity_Identical(t *testing.T) { + for _, s := range []string{"hello", "夜に駆ける", "калинка", ""} { + if got := runeSim(s, s); got != 1.0 { + t.Fatalf("expected 1.0 for identical strings %q, got %f", s, got) + } + } +} + +func TestSimilarity_Empty(t *testing.T) { + if got := runeSim("", "abc"); got != 0.0 { + t.Fatalf("expected 0.0 for empty vs non-empty, got %f", got) + } + if got := runeSim("abc", ""); got != 0.0 { + t.Fatalf("expected 0.0 for non-empty vs empty, got %f", got) + } +} + +func TestSimilarity_ASCII_UnchangedByRuneFix(t *testing.T) { + // ASCII behavior must be identical to the previous implementation + // (bytes == runes for ASCII): "kitten" vs "sitting" has distance 3, + // maxLen 7 -> 1 - 3/7. + got := runeSim("kitten", "sitting") + want := 1.0 - 3.0/7.0 + if diff := got - want; diff > 1e-9 || diff < -1e-9 { + t.Fatalf("expected %f for kitten/sitting, got %f", want, got) + } +} + +func TestLevenshtein_RuneUnits(t *testing.T) { + cases := []struct { + a, b string + want int + }{ + {"", "", 0}, + {"abc", "", 3}, + {"", "abc", 3}, + {"夜に駆ける", "群青", 5}, // all 5 runes replaced/deleted + {"君の名は", "君の名は", 0}, // identical CJK + {"кошка", "мошка", 1}, // single rune substitution + } + for _, c := range cases { + if got := levenshtein([]rune(c.a), []rune(c.b)); got != c.want { + t.Fatalf("levenshtein(%q, %q) = %d, want %d", c.a, c.b, got, c.want) + } + } +} + +func TestFindBestFuzzyMatch_SuffixVariantsStillMatch(t *testing.T) { + // Known remaster/suffix variants must still clear the 0.7 default + // threshold after normalization (SPEC track-matching REQ-TM-041). + variants := []struct { + source string + library string + }{ + {"Song Title (Remastered)", "Song Title"}, + {"Amazing Song (Radio Edit)", "Amazing Song"}, + {"Track Name - Remastered", "Track Name"}, + {"Ballad [Live]", "Ballad"}, + } + for _, v := range variants { + src := normalizeForMatch(v.source) + lib := normalizeForMatch(v.library) + score := runeSim(src, lib) + if score < 0.99 { + t.Fatalf("expected suffix variant %q to normalize to match %q, got similarity %f", + v.source, v.library, score) + } + } +} From e4d4ed70c3ad8fda2dba5da0d9863f0f97e2df2c Mon Sep 17 00:00:00 2001 From: Joe Stump Date: Fri, 10 Jul 2026 07:57:43 +0100 Subject: [PATCH 2/2] Polish pass from PR #357 review (non-blocking items) - SyncAllEnabledPlaylists: a tick-level LoadLibraryIndex failure no longer aborts the tick before any playlist gets error handling; it falls back to per-playlist loading so each playlist still reaches handleSyncError (sync_status=error, SyncEvent, UI notification per REQ-PLSYNC-060) - similarity(): restore identical-input fast path via slices.Equal - MatchTracksWithIndex: drop unused ctx param (no I/O) and add a defensive nil-index panic with a clear message - LoadLibraryIndex: remove redundant NavidromeID nil check (query already filters NavidromeIDNotNil) Co-Authored-By: Claude Fable 5 --- internal/services/playlist_sync.go | 11 +++++-- internal/services/track_matcher.go | 33 +++++++++++-------- internal/services/track_matcher_index_test.go | 4 +-- 3 files changed, 30 insertions(+), 18 deletions(-) diff --git a/internal/services/playlist_sync.go b/internal/services/playlist_sync.go index c4f53f7..fb91571 100644 --- a/internal/services/playlist_sync.go +++ b/internal/services/playlist_sync.go @@ -204,7 +204,7 @@ func (s *PlaylistSyncService) syncPlaylistToNavidrome(ctx context.Context, playl } } - matchResults := s.trackMatcher.MatchTracksWithIndex(ctx, libraryIndex, sourceTracks) + matchResults := s.trackMatcher.MatchTracksWithIndex(libraryIndex, sourceTracks) // Filter to only matched tracks (we can only add tracks that exist in Navidrome) var matchedTracks []providers.Track @@ -432,12 +432,17 @@ func (s *PlaylistSyncService) SyncAllEnabledPlaylists(ctx context.Context, userI // Governing: ADR-0014; issue #330 — load the user's library once per sync // tick and share the index across all playlists, instead of re-running the // full library query per playlist. + // + // If the tick-level load fails, do NOT abort the tick: fall back to + // per-playlist loading (libraryIndex == nil) so each playlist still goes + // through handleSyncError — sync_status=error, SyncEvent audit logging, + // and UI notification per REQ-PLSYNC-060 — instead of failing invisibly. libraryIndex, err := s.trackMatcher.LoadLibraryIndex(ctx, userID) if err != nil { - s.logger.Error("failed to load library index for sync tick", + s.logger.Error("failed to load shared library index for sync tick, falling back to per-playlist loading", "user_id", userID, "error", err) - return fmt.Errorf("failed to load library index: %w", err) + libraryIndex = nil } var syncErrors []error diff --git a/internal/services/track_matcher.go b/internal/services/track_matcher.go index 5d93245..6110619 100644 --- a/internal/services/track_matcher.go +++ b/internal/services/track_matcher.go @@ -4,6 +4,7 @@ package services import ( "context" "log/slog" + "slices" "strings" "time" "unicode" @@ -121,14 +122,14 @@ func (m *TrackMatcher) LoadLibraryIndex(ctx context.Context, userID int) (*Libra key := normalizedArtist + "|" + normalizedTitle idx.exactMap[key] = t - // Fuzzy match candidate with precomputed normalized runes - if t.NavidromeID != nil { - idx.candidates = append(idx.candidates, NormalizedCandidate{ - Track: t, - TitleRunes: []rune(normalizedTitle), - ArtistRunes: []rune(normalizedArtist), - }) - } + // Fuzzy match candidate with precomputed normalized runes. + // The query above already filters NavidromeIDNotNil, so every track + // here is a valid candidate. + idx.candidates = append(idx.candidates, NormalizedCandidate{ + Track: t, + TitleRunes: []rune(normalizedTitle), + ArtistRunes: []rune(normalizedArtist), + }) } m.Logger.Debug("built library index", @@ -149,12 +150,17 @@ func (m *TrackMatcher) MatchTracks(ctx context.Context, userID int, tracks []pro if err != nil { return nil, err } - return m.MatchTracksWithIndex(ctx, idx, tracks), nil + return m.MatchTracksWithIndex(idx, tracks), nil } // MatchTracksWithIndex matches source tracks against a precomputed LibraryIndex. +// It performs no I/O: the index must have been built via LoadLibraryIndex. // Governing: ADR-0014 (three-tier ISRC -> exact -> fuzzy matching) -func (m *TrackMatcher) MatchTracksWithIndex(ctx context.Context, idx *LibraryIndex, tracks []providers.Track) []MatchResult { +func (m *TrackMatcher) MatchTracksWithIndex(idx *LibraryIndex, tracks []providers.Track) []MatchResult { + if idx == nil { + panic("services.TrackMatcher.MatchTracksWithIndex: nil LibraryIndex; call LoadLibraryIndex first") + } + startTime := time.Now() results := make([]MatchResult, len(tracks)) @@ -426,10 +432,11 @@ func normalizeForMatch(s string) string { // clear the fuzzy threshold. // Governing: SPEC track-matching REQ-TM-031 (issue #330: distance and max length in rune units) func similarity(a, b []rune) float64 { + // Fast path: identical strings (including both-empty) skip the DP matrix. + if slices.Equal(a, b) { + return 1.0 + } if len(a) == 0 || len(b) == 0 { - if len(a) == 0 && len(b) == 0 { - return 1.0 - } return 0.0 } diff --git a/internal/services/track_matcher_index_test.go b/internal/services/track_matcher_index_test.go index 0190909..bb4337d 100644 --- a/internal/services/track_matcher_index_test.go +++ b/internal/services/track_matcher_index_test.go @@ -91,7 +91,7 @@ func TestLibraryIndex_LoadedOncePerTick(t *testing.T) { // Shared-index path: matching N playlists issues ZERO additional queries. for i := 0; i < playlists; i++ { - results := matcher.MatchTracksWithIndex(ctx, idx, sourceTracks) + results := matcher.MatchTracksWithIndex(idx, sourceTracks) require.Len(t, results, 3) assert.Equal(t, "nav-1", results[0].NavidromeTrackID) assert.Equal(t, "nav-2", results[1].NavidromeTrackID) @@ -190,7 +190,7 @@ func BenchmarkMatchTracksSharedIndex(b *testing.B) { b.ResetTimer() for i := 0; i < b.N; i++ { - matcher.MatchTracksWithIndex(ctx, idx, sourceTracks) + matcher.MatchTracksWithIndex(idx, sourceTracks) } }