From ecb18b6b8e3772b0d3e9ab71130f440f58864f44 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 10 Jul 2026 08:10:17 +0000 Subject: [PATCH 1/2] Port upstream PR #357: rune-based track matching and shared LibraryIndex Ports joestump/spotter#357 (commits b04c2df + e4d4ed7, review polish included), implementing joestump/spotter#330. - similarity() now measures both Levenshtein distance and max length in RUNES; the fork's version computed a rune-based distance but divided by a byte-based max length, inflating similarity 2-3x for non-ASCII (CJK/Cyrillic) titles and 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 and normalized once per sync tick via LoadLibraryIndex and shared across playlists via MatchTracksWithIndex; MatchTracks keeps its old signature for single-playlist callers - SyncAllEnabledPlaylists loads the index once per tick; a tick-level load failure falls back to per-playlist loading so each playlist still reaches handleSyncError (sync_status=error, SyncEvent, UI notification per REQ-PLSYNC-060) - Preserves the fork's regex-based suffix normalization, per-strategy metric.track_match metrics (ADR-0019), and artist-edge user scoping - Config default playlist_sync.min_match_confidence was already 0.7 in the fork; upstream's config change reduced to a comment no-op - 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, shared-index vs legacy benchmarks Known follow-up (out of scope, upstream deferred to story #340): the same rune/byte mismatch exists in internal/vibes/generator.go similarity(). Co-Authored-By: Claude Fable 5 --- internal/config/config.go | 2 +- internal/services/playlist_sync.go | 38 ++- internal/services/track_matcher.go | 215 +++++++++----- internal/services/track_matcher_index_test.go | 264 ++++++++++++++++++ .../services/track_matcher_internal_test.go | 116 ++++++++ 5 files changed, 554 insertions(+), 81 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 18c9d18..48d4cc2 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -380,7 +380,7 @@ func Load() (*Config, error) { 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 de5b654..a688700 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(libraryIndex, sourceTracks) + // Filter to only matched tracks (we can only add tracks that exist in Navidrome) var matchedTracks []providers.Track matchedCount := 0 @@ -432,10 +446,26 @@ 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. + // + // 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 shared library index for sync tick, falling back to per-playlist loading", + "user_id", userID, + "error", err) + libraryIndex = nil + } + 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 39a6e0c..24d7e52 100644 --- a/internal/services/track_matcher.go +++ b/internal/services/track_matcher.go @@ -5,6 +5,7 @@ import ( "context" "log/slog" "regexp" + "slices" "strings" "time" "unicode" @@ -50,17 +51,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(). @@ -77,49 +98,94 @@ 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. + // 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 lookup maps", - "isrc_map_size", len(isrcMap), - "exact_map_size", len(exactMap)) + 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(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(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)) + + 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") + + // 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{ @@ -142,7 +208,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 @@ -167,8 +233,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 @@ -191,7 +259,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 @@ -262,7 +330,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, @@ -272,31 +340,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) @@ -311,7 +368,7 @@ func (m *TrackMatcher) findBestFuzzyMatch(source providers.Track, candidates []* if score > bestScore { bestScore = score - bestMatch = candidate + bestMatch = candidate.Track } } @@ -403,17 +460,25 @@ 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 { +// 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 { + // 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 { return 0.0 } - // Calculate Levenshtein distance + // Calculate Levenshtein distance (in runes) distance := levenshtein(a, b) maxLen := len(a) if len(b) > maxLen { @@ -423,18 +488,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..bb4337d --- /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(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(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 4535764613d92fafa0811867006909a1f8e4c5aa Mon Sep 17 00:00:00 2001 From: "Joe (Agent) Stump" Date: Fri, 10 Jul 2026 10:38:05 +0100 Subject: [PATCH 2/2] Address PR #32 review: nil-index invariant doc + tick-level fallback test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Non-blocking review items from the adversarial review of the #357 port: - MatchTracksWithIndex: document the non-nil index INVARIANT in the doc comment (callers must guard, as syncPlaylistToNavidrome does); an unguarded nil from a future background-goroutine caller would panic and crash the server. Signature unchanged. - Add TestPlaylistSyncService_SyncAllEnabledPlaylists_LibraryIndexLoadFailure: injects a driver that fails only "tracks"-table queries so the tick-level LoadLibraryIndex in SyncAllEnabledPlaylists fails, then asserts the tick does not abort — each due playlist falls back to a per-playlist load (query-count proof: 1 tick + 1 per playlist) and still reaches handleSyncError with sync_status=error and a playlist_sync_failed SyncEvent per playlist (REQ-PLSYNC-060). Co-Authored-By: Claude Fable 5 --- internal/services/playlist_sync_test.go | 92 +++++++++++++++++++++++++ internal/services/track_matcher.go | 9 +++ 2 files changed, 101 insertions(+) diff --git a/internal/services/playlist_sync_test.go b/internal/services/playlist_sync_test.go index 9751c4d..5d6ab1b 100644 --- a/internal/services/playlist_sync_test.go +++ b/internal/services/playlist_sync_test.go @@ -2,16 +2,23 @@ package services_test import ( "context" + "errors" "io" "log/slog" + "strings" + "sync/atomic" "testing" "time" "fmt" + "entgo.io/ent/dialect" + entsql "entgo.io/ent/dialect/sql" + "spotter/ent" "spotter/ent/enttest" "spotter/ent/playlist" + "spotter/ent/syncevent" user_ent "spotter/ent/user" "spotter/internal/config" "spotter/internal/events" @@ -715,3 +722,88 @@ func TestPlaylistSyncService_RebuildPlaylistSync_NoExistingNavidromeID(t *testin require.NoError(t, err) assert.Equal(t, "nav-playlist-123", updatedPl.NavidromePlaylistID) } + +// libraryQueryFailingDriver wraps an Ent driver and, when enabled, fails any +// read query against the "tracks" table (the library query issued by +// TrackMatcher.LoadLibraryIndex) while letting every other statement through, +// so playlist updates and SyncEvent writes still succeed. +type libraryQueryFailingDriver struct { + dialect.Driver + fail atomic.Bool + failedQueries atomic.Int64 +} + +func (d *libraryQueryFailingDriver) Query(ctx context.Context, query string, args, v any) error { + if d.fail.Load() && (strings.Contains(query, "`tracks`") || strings.Contains(query, `"tracks"`)) { + d.failedQueries.Add(1) + return errors.New("injected library query failure") + } + return d.Driver.Query(ctx, query, args, v) +} + +// Governing: SPEC playlist-sync-navidrome REQ-PLSYNC-060 (SyncEvent audit logging on failure) +// Issue #330 / PR review follow-up: when the tick-level LoadLibraryIndex in +// SyncAllEnabledPlaylists fails, the tick must NOT abort before any playlist +// gets error handling. It falls back to per-playlist loading so each due +// playlist still reaches handleSyncError: sync_status=error, a +// playlist_sync_failed SyncEvent, and a UI notification. +func TestPlaylistSyncService_SyncAllEnabledPlaylists_LibraryIndexLoadFailure(t *testing.T) { + ctx := context.Background() + + drv, err := entsql.Open("sqlite3", "file:libfailtick?mode=memory&cache=shared&_fk=1") + require.NoError(t, err) + failing := &libraryQueryFailingDriver{Driver: drv} + client := ent.NewClient(ent.Driver(failing)) + t.Cleanup(func() { client.Close() }) + require.NoError(t, client.Schema.Create(ctx)) + + logger := slog.New(slog.NewTextHandler(io.Discard, nil)) + cfg := &config.Config{} + cfg.PlaylistSync.MinMatchConfidence = 0.7 + bus := events.NewBus() + svc := services.NewPlaylistSyncService(client, cfg, logger, bus) + svc.Register(mockPlaylistSyncerFactory(&mockPlaylistSyncer{ + providerType: providers.TypeNavidrome, + syncedID: "nav-playlist-tickfail", + })) + + user := createTestUserWithNavidromeAuth(t, client) + pl1 := createTestPlaylistForSync(t, client, user, "spotify", true) + createTestPlaylistTracksForSync(t, client, pl1) + pl2 := createTestPlaylistForSync(t, client, user, "spotify", true) + createTestPlaylistTracksForSync(t, client, pl2) + + failing.fail.Store(true) + err = svc.SyncAllEnabledPlaylists(ctx, user.ID) + failing.fail.Store(false) + + // The tick itself reports the per-playlist failures, not the index error. + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to sync 2 playlists") + + // Proof the fallback ran: one failed tick-level load, then one failed + // per-playlist load for each of the two playlists (1 + 2 = 3). + assert.Equal(t, int64(3), failing.failedQueries.Load(), + "expected tick-level load plus one per-playlist fallback load per playlist") + + // REQ-PLSYNC-060: every due playlist ends in sync_status=error with the + // library-index error recorded. + for _, plID := range []int{pl1.ID, pl2.ID} { + updated, getErr := client.Playlist.Get(ctx, plID) + require.NoError(t, getErr) + assert.Equal(t, playlist.SyncStatusError, updated.SyncStatus, + "playlist %d must end with sync_status=error", plID) + assert.Contains(t, updated.SyncError, "failed to load library index", + "playlist %d must record the library index error", plID) + } + + // ...and every due playlist gets a playlist_sync_failed SyncEvent. + failedEvents, err := client.SyncEvent.Query(). + Where(syncevent.EventTypeEQ(syncevent.EventTypePlaylistSyncFailed)). + All(ctx) + require.NoError(t, err) + require.Len(t, failedEvents, 2) + combinedMetadata := failedEvents[0].Metadata + failedEvents[1].Metadata + assert.Contains(t, combinedMetadata, fmt.Sprintf(`"playlist_id":%d`, pl1.ID)) + assert.Contains(t, combinedMetadata, fmt.Sprintf(`"playlist_id":%d`, pl2.ID)) +} diff --git a/internal/services/track_matcher.go b/internal/services/track_matcher.go index 24d7e52..d5eb403 100644 --- a/internal/services/track_matcher.go +++ b/internal/services/track_matcher.go @@ -156,6 +156,15 @@ func (m *TrackMatcher) MatchTracks(ctx context.Context, userID int, tracks []pro // MatchTracksWithIndex matches source tracks against a precomputed LibraryIndex. // It performs no I/O: the index must have been built via LoadLibraryIndex. +// +// INVARIANT: idx MUST be non-nil. A nil index is a programming error and +// panics immediately with a descriptive message (rather than nil-dereferencing +// a few lines later). Callers are responsible for guarding before the call — +// as syncPlaylistToNavidrome does by loading an index on demand when its +// shared one is nil or built for a different user. This matters especially +// for future callers running in background goroutines, where an unrecovered +// panic crashes the whole server; if the index may be absent, load one via +// LoadLibraryIndex (or fall back to MatchTracks) instead of passing nil. // Governing: ADR-0014 (three-tier ISRC -> exact -> fuzzy matching) func (m *TrackMatcher) MatchTracksWithIndex(idx *LibraryIndex, tracks []providers.Track) []MatchResult { if idx == nil {