586 commits

Author	SHA1	Message	Date
Deluan Quintão	4652b46602	fix(plugins): populate username for buffered plugin scrobbles (#5736) ... Plugin scrobblers read the username from the request context via getUsernameFromContext, but buffered scrobbles are persisted to the DB scrobble buffer (which stores only the userId) and later drained by a background worker running on context.Background(). That context carries no authenticated user, so ScrobbleRequest.Username was always empty for WASM scrobbler plugins. NowPlaying is unaffected because it is dispatched synchronously on context.WithoutCancel(requestCtx), which retains the user. Restore the user in the drain path: processUserQueue now looks up the user by the buffered userId and injects it into the context via request.WithUser before dispatching, mirroring the pattern already used in play_tracker. Builtin scrobblers are unaffected as they resolve the account from the userId argument rather than the context.	2026-07-08 12:37:42 -04:00
Deluan Quintão	f48943c058	fix(plugins): discard buffered scrobbles when a plugin is removed (#5737) ... * fix(plugins): discard buffered scrobbles when a plugin is removed Scrobbles are buffered in the DB per service, keyed by the plugin name. When a plugin was removed (deleted from the plugins folder and detected by the sync), its pending buffer entries were left behind forever: the drain goroutine is stopped on the next scrobbler refresh, so the rows were never retried nor discarded. Worse, if a plugin with the same name was installed later, the stale entries would be drained into it - potentially a completely unrelated plugin that just reuses the name. Add a Discard(service) method to ScrobbleBufferRepository and call it from removePluginFromDB, right after the plugin record is deleted. Disabling a plugin intentionally keeps its buffered scrobbles, consistent with the buffer's purpose of surviving temporary outages, and transient unload/reload cycles during config updates are unaffected since they never delete the plugin record. * fix(plugins): don't wipe builtin scrobbler queues on plugin removal Buffer entries are keyed by service name only, and removePluginFromDB runs for any removed plugin file, so removing a plugin named e.g. lastfm.ndp - regardless of its capability - would discard the builtin Last.fm retry queue. Skip the discard when the plugin name is owned by a registered builtin scrobbler, exposed via a new scrobbler.IsBuiltinScrobbler helper. Reported by Codex review on the PR. Also drop the testBroker usage from the new removePluginFromDB spec: it is defined in manager_test.go which is excluded on Windows, breaking the Windows test build. sendPluginRefreshEvent is nil-safe, so no broker is needed.	2026-07-08 12:37:17 -04:00
Deluan Quintão	01b7c86f90	fix(scanner): stop logging expected lyrics sniff misses as warnings (#5702) ... * fix(scanner): stop logging expected lyrics sniff misses as warnings During a scan, embedded lyrics are parsed with an empty suffix, which puts ParseLyrics into content-sniffing mode: it tries the TTML, SRT and Lyricsfile YAML parsers in turn before falling back to plain text. Every plain-text or LRC lyric therefore fails the structured probes on its way to the fallback, and each failure was logged at warning level with no indication of which file triggered it, flooding the scan log with benign "Error parsing lyrics, falling back to plain text" messages. A probe rejecting content it does not own during sniffing is expected control flow, so it is now logged at trace instead. A parse failure under an explicitly requested suffix (e.g. a malformed .yaml/.srt/.ttml sidecar) still warns, since the user declared that format. ParseLyrics gains ctx and path parameters so any warning names the offending file and carries request context where available; all call sites are updated accordingly. Also fixes a test-isolation bug in the new logging spec: the BeforeEach swapped the process-global default logger via SetDefaultLogger but only restored the log level on cleanup, leaking the null logger and its hook into later specs in the shared model suite. * test: use spec-scoped contexts instead of context.Background in lyrics tests Replace context.Background() with GinkgoT().Context() (and b.Context() in the parse benchmarks) across the lyrics-related tests, so contexts are cancelled when each spec ends. The embeddedLyrics fixture in core/lyrics is now a hand-written literal like its sibling fixtures, removing the construction-time ParseLyrics call that could not use a spec-scoped context. * refactor(model): attach lyrics parse log attribution via context Narrow ParseLyrics back to (ctx, suffix, lang, contents), dropping the path parameter added by the previous commit. Attribution now uses the codebase's existing idiom: callers that know the source attach it with log.NewContext (e.g. "file" for the media file or sidecar), and the plugin adapter tags both the plugin name and the track, fixing probe-miss logs that misattributed plugin-returned content to the file's own tags. This removes three adjacent string parameters that were easy to swap silently, and the "" placeholder most call sites had to pass. Also hardens the logging spec from the previous commit: the null test logger is now swapped in before raising the level (SetLevel forces the current default logger to trace, so the old order left the null logger at info and trace entries never reached the hook), the sniff test now asserts probe misses are observable at trace with file attribution instead of only asserting the absence of warnings, and cleanup restores the actual previous logger — via a new return value on log.SetDefaultLogger — instead of a bare logrus.New() that would discard hooks configured on the process-wide logger. * refactor(lyrics): hoist attributed log contexts out of loops Address review feedback on #5702: build the log-attributed context once per operation instead of per iteration, and reuse it on the surrounding log calls so the error/trace lines around ParseLyrics carry the same attribution fields. In fromExternalFile the sidecar path now rides the context for all log lines in the function, replacing the repeated explicit "path" field. * style(model): pass lyrics parse errors as final log arguments Per the project logging convention, errors go as the last argument (the log package normalizes them via its error case) instead of a keyed "error" pair, which stores the raw error value and bypasses that handling. Flagged by review on #5702; the keyed form was inherited from the original warning line.	2026-07-02 09:46:57 -04:00
Deluan Quintão	bd9fa1c602	feat(listenbrainz): match collaboration top-songs via all credited artist MBIDs (#5670) ... * refactor(agents): drop single Artist fields from Song, keep only Artists Song now represents credited artists solely via the Artists slice; the single Artist/ArtistMBID fields and the ArtistList passthrough are removed. Equals continues to hash the whole value. * refactor(lastfm,listenbrainz): build Song.Artists in built-in agents Last.fm and ListenBrainz now populate the Artists slice directly. For ListenBrainz top songs, all credited artist MBIDs are mapped (the combined display name on the first credit plus MBID-only collaborators) instead of keeping only the first MBID, feeding the matcher's per-MBID specificity. * refactor(external): set Song.Artists in top-songs enrichment getMatchingTopSongs now seeds an Artists entry from the known artist when a song carries none, replacing the single Artist/ArtistMBID writes. * refactor(plugins): fold single-artist SongRef into Song.Artists SongRef keeps its single Artist/ArtistMBID fields as part of the plugin wire contract; songRefToAgentSong now folds them into a one-element Artists list when a plugin sends no artists array. * refactor(matcher): read Song.Artists directly groupQueries consumes s.Artists now that ArtistList is gone; test inputs build the Artists slice. * fix(matcher): keep MBID-only artists as identity signals Review follow-up: an artist credited only by MBID (empty ID and name) was dropped before resolution in four places, defeating the multi-MBID matching path this PR adds. - matcher.groupQueries: treat a non-empty MBID as a usable artist signal - external.getMatchingTopSongs: backfill the primary credit's name/MBID when the agent left them empty - plugins.songRefToAgentSong: fold a single-artist SongRef when only ArtistMBID is set (not just when Artist name is set) - listenbrainz.topSongArtists: return nil instead of an empty-name placeholder when neither name nor MBIDs are present * fix(external): only backfill top-song artist onto an unnamed credit Review follow-up (codex P2): the previous backfill stamped the queried artist's MBID onto Artists[0] whenever it was empty, even when that credit already named a different (e.g. featured) artist — producing a mismatched name+MBID pair that could mis-rank matches. Now only an unnamed first credit is filled (it is, by construction, the queried artist); an already-named credit is left untouched. Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org>	2026-06-26 17:06:14 -04:00
Deluan Quintão	7b7721f002	feat(matcher): match similar/top songs by multiple artists (#5668) ... * feat(matcher): add Song.Artists (agents.Artist) and field-wise song dedup * refactor(matcher): make song equality an agents.Song.Equals method via hashstructure Move the sameSong free function from core/matcher into an Equals method on agents.Song, following the model.MediaFile/Album.Equals convention. Uses strict hashstructure hashing (nil opts, no IgnoreZeroValue) to preserve the original whole-value equality contract. Tests moved to core/agents. * feat(matcher): match by multiple artists with overlap ranking and artist-ID fast-path * refactor(matcher): rank artist overlap and specificity above the preferred-track flag Identity signals (specificityLevel, artistOverlap) now outrank the taste signal (preferredMatch) in betterThan. A starred/4-star track that is a worse identity match no longer beats a more specific or higher-overlap track. PreferStarred still breaks ties when specificity and overlap are equal. * fix(matcher): score artist-MBID specificity against all credited artists, not just the last sanitizedTrack.artistMBID (string) replaced with artistMBIDs (map[string]struct{}) so bucketTracks collects all credited owned MBIDs per query instead of last-write-wins. computeSpecificityLevel tests set membership, letting each of a collaboration's MBID-bearing artists reach the proper specificity level (4/5) independently. * feat(plugins): carry multiple artists (with IDs) through SongRef conversions * feat(plugins): regenerate schemas and PDK wrappers for multi-artist SongRef * refactor(matcher): tidy bucketTracks accumulator and artist resolution Replace bucketTracks' two parallel per-track maps (overlapByQuery/mbidsByQuery) with a named queryAccum struct (F2). Collapse resolveArtists' four hand-mutated parallel maps into a pendingArtist slice with derived nameToQueries/mbidToQueries maps (F1). Replace the own() method on resolvedArtists with a package-level addToSet helper that drops the method/receiver indirection (F3). * docs(matcher): trim comments that restate the code * fix(plugins): use Vec::is_empty for slice fields in generated Rust PDK * fix(matcher): treat a resolved artist ID as an identity match for specificity * docs(matcher): reflect artist-ID identity in the specificity ladder	2026-06-26 14:46:09 -04:00
Deluan Quintão	06993a8e04	test(storage): re-enable local storage tests on Windows (#5654) ... * refactor(storage): extract LocalPathToURL from storage.For * test(storage): re-enable local storage tests on Windows (#5381) * test(storage): fix Windows drive-letter path expectation The 'should handle Windows drive letters correctly' test was gated behind the now-removed SkipOnWindows BeforeEach, so its expectation had never run. On Windows, newLocalStorage re-joins u.Host+u.Path via filepath.Join, which yields a backslash path (C:\music), not C:/music. Assert against filepath.Join so the expectation matches the OS-native result. * test(storage): probe Windows drive-letter path in LocalPathToURL Three review bots flagged that LocalPathToURL escapes the drive-letter colon (C: -> C%3A), which url.Parse rejects. There are no Windows bug reports, so add a Windows-gated test that exercises the real conversion on a drive-letter path and let CI decide whether the bug is real before changing production code.	2026-06-22 21:33:14 -04:00
Deluan Quintão	b38054b29c	perf(artwork): faster image resize + update gen2brain/webp to v0.6.0 (#5652) ... * fix(artwork): convert decoded images to a fast-path type before resizing x/image/draw's CatmullRom scaler only has optimized paths for *image.RGBA, *image.NRGBA, *image.Gray and *image.YCbCr. Other concrete types — notably *image.NYCbCrA (from WebP) and *image.Paletted (indexed PNGs) — fall back to a generic per-pixel At()/RGBA() loop that is several times slower. Convert such images to *image.RGBA once before scaling; fast-path types are returned unchanged. This makes resize performance independent of which decoder wins the image.Decode("webp") registration, and also speeds up indexed PNGs. Signed-off-by: Deluan <deluan@navidrome.org> * chore(deps): update gen2brain/webp to v0.6.0 v0.6.0 replaces the wazero WASM runtime with a self-contained wasm2go-transpiled WebP decoder/encoder. This drops the webp -> wazero dependency edge (wazero is still used by the plugin system) and makes the WASM-only build path (32-bit / nodynamic) faster and far lighter on allocations. Signed-off-by: Deluan <deluan@navidrome.org> * perf(artwork): defer fast-path conversion until a resize is needed Move toFastScaleType to just before the CatmullRom.Scale call, after the no-upscale early return. Previously the conversion ran right after decode, so a request for a size >= the source dimensions would allocate and walk a full RGBA copy only to discard it when resizeStaticImage returns nil. The resize path is unchanged; the no-op path drops ~30-40% time and up to ~79% memory for large indexed/WebP artwork. Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org>	2026-06-22 16:33:01 -04:00
Deluan Quintão	803b385920	fix(matcher): match by artist credit so artist-MBID specificity works and collaborators match (#5637) ... * refactor(matcher): carry resolved artist MBID on sanitizedTrack * feat(matcher): two-phase artist resolution for title matching * fix(matcher): wire phase-1 artist mock in consumer tests; guard back-map dupes - External tests for title-fallback paths now register a second `.Maybe()` artistRepo.GetAll expectation for the matcher's phase-1 artist-resolution call, and supply RoleArtist Participants on phase-2 track mocks so the back-map routes tracks to their query buckets correctly. - Back-map loop extracted into buildTracksByQuery helper; guard ensures each track is appended at most once per query bucket even when it credits multiple resolved artists in that bucket, preventing duplicate scoring. - matchTitlePhase2 helper: removed redundant squirrel.Sqlizer type assertion (ranging over squirrel.And already yields Sqlizer). * perf(matcher): use non-correlated IN subquery for phase-2 lookup * refactor(matcher): decompose two-phase title matching into named steps matchByTitle had grown to ~98 lines holding four jobs and eleven locals. Split it into a thin orchestrator over named phases: - groupQueriesByArtist: build the per-artist query buckets. - resolveArtists / resolvedArtists: phase 1, with the three parallel maps (byQuery / mbid / allIDs) folded into one type that owns artist-ID routing and track bucketing. Artist ownership is now two direct map lookups (by order name, by MBID) instead of the prior O(artists x queries) nested scan. - fetchTracksCreditedTo: phase 2, using squirrel.Placeholders instead of a hand-rolled placeholder string. - bucketTracks / scoring loop: unchanged behavior. Pure restructuring; the suite, race, consumers, and the real-DB benchmark are all unchanged. Also dedupes the phase-1 MBID filter as a side effect. * refactor(matcher): apply simplify cleanups to two-phase resolution Behavior-preserving cleanups from a /simplify pass: - struct{} sets instead of bool-valued sets (codebase convention) - move newSanitizedTrack after the dedup guard in bucketTracks, so a track credited to multiple resolved artists is sanitized only when actually bucketed - pre-size the phase-1/phase-2 maps; drop the dead nil-guard in own() - slice.Map for the []string->[]any arg conversion - document why the raw media_file_artists SQL stays in this layer, and that bucketTracks relies on the bulk participants JSON carrying artist IDs - extract an artistParticipants() test helper, collapsing 57 four-level Participants literals (test file -211 lines net) * docs(matcher): rename inner title-matching steps to avoid 'phase' clash The matcher's top-level strategies (ID/MBID/ISRC/Title) are already called phases. Reusing 'phase 1/2' for the two steps inside title matching (resolve artists, fetch their tracks) was confusing. Drop the ordinals and let the function names (resolveArtists, fetchTracksCreditedTo) and prose describe the steps. Renamed the matchTitlePhase2 test helper to matchTracksByArtistQuery. Comment-only; no behavior change. * fix(matcher): own MBID-resolved artists for every aliased query Address bot review on PR #5637. When several agent queries share one ArtistMBID under different sanitized names (agent aliases), the resolver kept only the last query name per MBID, so the others never owned the resolved artist and their songs fell through. Track all query names per MBID instead; add a RED-proven test for the two-alias case. Also invert byQuery into a reverse artist-ID -> query-name index in bucketTracks, dropping the per-participant scan over all queries, and guard fetchTracksCreditedTo against an empty artist-ID slice. Fix the artist mock to forward variadic options with Called(options...) so QueryOptions-shaped matchers receive the same argument shape as real calls. * docs(matcher): trim comments that restated the code Cut three doc comments down to their why: groupQueriesByArtist, own, and bucketTracks no longer restate what the signature and body already show. Condense fetchTracksCreditedTo's rationale from two paragraphs to one, keeping the role='artist', non-correlated-IN, and layer-boundary notes.	2026-06-21 11:05:58 -04:00
Deluan Quintão	6486a27634	refactor(matcher): index-space resolution + batched title lookups (#5635) ... * refactor(matcher): resolve matches in song-index space * test(matcher): pin per-index duration matching for duplicate title+artist songs * refactor(matcher): drop unreachable specificity sentinel * refactor(matcher): hoist PreferStarred read out of scoring loop * docs(matcher): correct config field references in MatchSongs doc * fix(matcher): log swallowed per-artist DB error in title matching * fix(matcher): fail title matching when all artist lookups error * refactor(matcher): simplify loaders and test helpers * docs(matcher): move algorithm docs to package-level doc.go * docs(matcher): focus examples on fuzzy matching behavior * refactor(matcher): store index in dedup map and harden test helper * fix(matcher): keep exact-phase matches when all title lookups fail * perf(matcher): batch title-phase artist lookups into one query matchByTitle issued one GetAll per distinct artist, run serially. On a large library a batch of similar-songs spans dozens of artists, and profiling against a 95k-track library showed the matcher was ~90% bound in that serial query loop (a 100-song batch fired ~89 separate multi-join queries, taking ~6s). Replace the loop with a single 'order_artist_name IN (...)' query, then group the returned tracks by artist in memory and score each song against its bucket. This cuts a 100-song batch from ~6s to ~0.4s (roughly 14x) with less memory. Grouping keys on order_artist_name (the field the query filters on, matching how the per-artist queries are keyed), falling back to the sanitized Artist when it is unset. Because there is now a single query, matchByTitle is all-or-nothing like the ID/MBID/ISRC loaders: the per-artist best-effort skip is gone, while resolveMatches still preserves exact-phase matches when the title query fails. * refactor(matcher): group batched title matches by order_artist_name After batching the title-phase lookups into one query, the returned tracks must be grouped back to their artist. Key on MediaFile.OrderArtistName — the exact field the query filters on — so collaboration/"feat." tracks (whose display Artist differs from the sort artist) bucket correctly, with a sanitized-Artist fallback when it is unset. OrderArtistName is deprecated in favor of Participants, but the bulk GetAll path does not hydrate participant detail (the rich artist fields come only from the per-record GetWithParticipants JOIN), so the participant order name is empty here and the column is the only populated source. Also adds a TODO in computeSpecificityLevel: its artist-MBID levels read the deprecated, unpopulated MediaFile.MbzArtistID column, so they never fire today.	2026-06-21 03:45:56 -04:00
Deluan Quintão	aa5aa731dc	refactor(lyrics): single ParseLyrics entry point + all-format plugin lyrics (#5632) ... * refactor(lyrics): read sidecar files via library storage FS Routes fromExternalFile reads through storage.For(mf.LibraryPath) instead of os.Open on AbsolutePath, fixing sidecar reads for non-local backends. UTF-16 LE/BE and BOM handling preserved via ioutils.UTF8Reader. * refactor(lyrics): address review feedback on sidecar FS read - Move blank local-storage import from sources.go into lyrics_suite_test.go (the test suite already imports the local package for RegisterExtractor, so local's init() runs; production binaries get the scheme via normal wiring) - Fix misleading comment: model.ParseLyrics → model.ParseLyricsFile - Replace what-comment with why-comment in BeforeSuite explaining the log.Fatal guard that requires the no-op extractor registration * test(lyrics): add subsonic e2e baseline for getLyrics endpoints Establishes a behavioral baseline for getLyricsBySongId (v2 structured) and getLyrics (legacy) before the lyrics parser refactor. Covers embedded formats (LRC synced, plain text, TTML) and sidecar formats (LRC, SRT, YAML), all isolated under a Lyrics/ fixture folder so the new fixtures do not perturb existing test behavior beyond fixture counts. Sidecar files are injected as raw &fstest.MapFile{Data: []byte(...)} entries; the scanner skips non-audio extensions (.lrc, .srt, .yaml) so they are invisible to scanning but reachable via the fake FS at request time through fromExternalFile/storage.For. Update album/artist/song counts in the album-list, multi-library, and search3 empty-query tests to reflect the six new tracks (1 new artist, 1 new album, 6 new songs). * test(lyrics): strengthen e2e lyrics baseline (lang assertions, rename helper) Rename the local helper `main` to `firstLyric` to avoid collision with the reserved-feeling built-in name. Add `Lang` assertions to both embedded and sidecar DescribeTable entries, locking the current observed values: "xxx" (ISO 639-2 "no language specified") for all embedded and LRC/SRT sidecars, and "eng" for the YAML sidecar (which explicitly sets `language: eng`). * feat(lyrics): detect Lyricsfile YAML in content-sniffing * feat(plugins): content-sniff plugin lyrics for all formats Replace model.ToLyrics (LRC/plain only) with model.ParseEmbedded so plugin responses are content-sniffed for TTML, SRT, YAML, LRC, and plain text. ParseEmbedded returns a LyricList, so the loop now flattens multiple tracks per response entry. The test-lyrics WASM plugin gains a "ttml" format mode (configured via pdk.GetConfig) that returns a minimal TTML document; rebuilt with the standard Go wasip1 toolchain (GOOS=wasip1 GOARCH=wasm). A new Ginkgo test asserts Synced==true and the exact cue value, which the old plain-text path could not produce. GetLyrics doc comment updated to reflect content-sniffing; a later task will retarget it to ParseLyrics once that function is introduced. * test(plugins): validate plugin lyrics auto-detect across all formats The test-lyrics WASM plugin now supports per-format modes via the "format" config key: ttml, srt, yaml, lrc, and plain, in addition to the existing default plain-text response. The plugin is rebuilt with the standard Go wasip1 compiler. lyrics_adapter_test.go gains a DescribeTable covering all five formats, asserting both Synced (the discriminator that proves correct format detection) and the exact line value. This validates the full auto-detect chain (TTML → SRT → YAML/Lyricsfile → LRC → plain) end-to-end through the real plugin → adapter → parser flow. * refactor(lyrics): consolidate parsers into model.ParseLyrics * refactor(lyrics): retarget legacy callers to model.ParseLyrics Pin suffix to ".lrc" to preserve byte-identical output for stored plain/LRC text that was previously handled by the now-removed ToLyrics. * test(lyrics): fix lyrics tests after parser consolidation - Rewrite the YAML-fallback test to assert the correct design: a non-Lyricsfile .yaml sidecar returns as plain text and shadows lower-priority sources (rather than falling through to .lrc). - Add LibraryPath + relative Path split to the three subsonic tests that read sidecar files via storage.For(), so they resolve against the correct fixtures directory. - Register a no-op extractor in api_suite_test.go BeforeSuite so newLocalStorage does not fatal when storage.For is called during sidecar-lyrics tests. * test(lyrics): add per-format ParseLyrics benchmarks Baseline measurements (count=2 runs) on M2: BenchmarkParseLyrics_LRC-8 5725 178796 ns/op 49.78 MB/s 427877 B/op 523 allocs/op BenchmarkParseLyrics_Plain-8 5425 230854 ns/op 32.44 MB/s 102508 B/op 16 allocs/op BenchmarkParseLyrics_EnhancedLRC-8 1942 605893 ns/op 17.66 MB/s 860678 B/op 4256 allocs/op BenchmarkParseLyrics_SRT-8 3249 373991 ns/op 25.91 MB/s 1113575 B/op 4407 allocs/op BenchmarkParseLyrics_TTML-8 1483 813027 ns/op 13.86 MB/s 2198052 B/op 8665 allocs/op BenchmarkParseLyrics_YAML-8 1700 678250 ns/op 13.01 MB/s 1235096 B/op 8288 allocs/op BenchmarkParseLyrics_SniffTTML-8 1525 776482 ns/op 14.51 MB/s 2225448 B/op 8681 allocs/op BenchmarkParseLyrics_SniffSRT-8 2528 451210 ns/op 21.48 MB/s 1157000 B/op 4422 allocs/op BenchmarkParseLyrics_SniffYAML-8 1333 827152 ns/op 10.67 MB/s 1337195 B/op 8718 allocs/op BenchmarkParseLyrics_SniffLRC-8 2820 413038 ns/op 21.55 MB/s 588934 B/op 1812 allocs/op BenchmarkParseLyrics_SniffPlain-8 2968 409091 ns/op 18.31 MB/s 254470 B/op 1491 allocs/op Content-sniff path overhead: 1.5–15% depending on format. * test(lyrics): use real public-domain fixtures for parser benchmarks Replace synthetic benchmark payloads with 'Auld Lang Syne' (Robert Burns, 1788, public domain) rendered into every supported format (LRC, plain, enhanced LRC, SRT, TTML, Lyricsfile YAML) so the numbers reflect realistic content. Same song across formats makes per-format cost comparable. Baseline (Apple M-series, -benchmem, real fixtures): LRC ~28 us/op 42 KB 147 allocs Plain ~23 us/op 18 KB 22 allocs EnhancedLRC ~37 us/op 51 KB 374 allocs SRT ~52 us/op 139 KB 581 allocs TTML ~119 us/op 276 KB 1227 allocs YAML ~142 us/op 193 KB 1732 allocs Sniff(LRC) ~47 us/op 57 KB 237 allocs Sniff(TTML) ~122 us/op 282 KB 1250 allocs Sniff(YAML) ~186 us/op 218 KB 1847 allocs Fixtures in tests/fixtures/lyrics/. * fix(lyrics): preserve [] (not null) for empty lyrics in backfill migration ParseLyrics returns nil for zero-line input (whitespace-only stored lyrics). json.Marshal(nil LyricList) produces null, violating the DB invariant that media_file.lyrics uses [] for empty lyrics, never null. Initialize to model.LyricList{} when ParseLyrics returns nil so the marshalled result is always []. * refactor(lyrics): unify parser dispatch and centralize empty-list invariant Apply thermo-nuclear review findings (behavior-preserving): - Replace the suffix switch + three single-use closure adapters (parseTTMLKnown/parseSRTKnown + inline YAML closure) with a bySuffix map of a single lyricParser(lang, contents) signature. Normalize parseTTMLWithDefaultLang/parseSRTWithLanguage to that (lang, contents) order so no adapter glue is needed. - Collapse the parallel sniffLyrics engine into one parseFirstMatch primitive shared by both the suffix and content-sniff paths (sniffOrder candidate list). TTML stays gated via parseTTMLIfDocument in sniff mode to avoid running the XML decoder on plain/LRC text. - Add LyricList.MarshalJSON so empty/nil always serializes to [] (the lyrics column invariant), in one canonical place. Delete the migration's nil-guard, which the marshaler now subsumes. Behavior verified unchanged: full suite + race + e2e green. * refactor(lyrics): single registry drives both suffix dispatch and sniff order Collapse the bySuffix map and sniffOrder slice into one ordered registry: slice order is the content-sniff probe order, each row's suffixes drive sidecar dispatch, and per-row bySuffix/byContent parsers preserve the gated-TTML-when-sniffing distinction. One source of truth, no duplicated parser references. * refactor(lyrics): self-skipping parsers collapse the format table to one column Move the TTML <tt>-document gate into parseTTMLWithDefaultLang itself (after the encoding fixup, so UTF-16-declared docs are still recognized): non-TTML content returns (nil, nil) to skip; a malformed <tt> document still errors. SRT and Lyricsfile YAML already self-skip. With every structured parser self-skipping, the format table drops to one {suffixes, parse} column named lyricFormats — no bySuffix/byContent split, no separate sniff-only TTML gate. Both the suffix and content-sniff paths share the same parser per format. * refactor(lyrics): strip BOM once at ParseLyrics entry for all paths Previously only the content-sniff path stripped the BOM; the suffix path relied on its callers (fromExternalFile via UTF8Reader) having already stripped it. That implicit contract was fragile — a caller passing raw BOM-prefixed bytes with a suffix would reach the parsers with the BOM intact (SanitizeText does not strip it). Strip once at entry so every path and parser sees clean bytes regardless of caller. No-op for already-stripped input. * refactor(lyrics): trim verbose comments to essential why * refactor(lyrics): move LRC parser to its own lyrics_lrc.go Extract parseLRC, the enhanced-LRC helpers (parseEnhancedLine, adjustGroup, stripEnhancedMarkers, shiftELRCCues), parseTime, and the LRC regexes from lyrics.go into lyrics_lrc.go, with the parseLRC tests in lyrics_lrc_test.go. This makes the layout symmetric — one file per format (lrc/srt/ttml/yaml) — and leaves lyrics.go holding only shared types and cue normalization. All moved symbols were already LRC-private; no behavior change. * refactor(lyrics): collapse ParseLyrics suffix/sniff branches into one loop Both modes differ only in which formats to try, so select candidates in a single loop (all formats when sniffing, the suffix's own otherwise) and run them through parseFirstMatch once. Drops the projected-slice make+index and the ContainsFunc closure; unmatched suffixes yield no candidates and fall to the plain-text floor, as before. * refactor(lyrics): apply simplify-review cleanups - stripBOM: bytes.TrimPrefix instead of []byte<->string round-trip (no alloc) - ParseLyrics: pre-size the candidates slice - move isTTMLDocument to lyrics_ttml.go beside its only caller (the dispatch layer should hold no per-format knowledge) * refactor(lyrics): simplify test descriptions for structured lyrics Signed-off-by: Deluan <deluan@navidrome.org> * refactor(lyrics): fold parseLyricsfile into lyricParser signature and rename file - parseLyricsfile now matches the lyricParser signature directly (reads via bytes.NewReader), removing the parseLyricsfileBytes adapter and the string(contents) copy; the lyricFormats table references it directly. - StructuredLyrics drops the vestigial LyricList{} init (json.Unmarshal overwrites; MarshalJSON owns the empty->[] invariant). - Rename lyricsfile.go -> lyrics_lyricsfile.go (and its test) to match the lyrics_<format>.go convention used by lrc/srt/ttml. * refactor(lyrics): move test-only parseTTML/parseSRT wrappers to test files These zero-arg wrappers (defaulting lang to "xxx") had no production callers after the consolidation — only the format tests used them. Move each beside its tests so the production files carry no test-only code. * build: exclude generated *_gen.go files from linting The plugin host *_gen.go files (ndpgen output) were tripping the whitespace linter despite carrying a generated marker. Exclude them by path so make lint and the pre-push hook pass on untouched generated code. * perf(lyrics): drop []byte/string round-trips in parsers Apply code-review feedback to remove avoidable allocations in the lyrics parsers. isTTMLDocument now takes []byte directly, so parseTTMLWithDefaultLang no longer copies its buffer into a string before the TTML probe. parseSRTBlock splits its block with strings.Split instead of converting to []byte and back per line. ParseLyrics hoists strings.ToLower(suffix) out of the format loop. No behavior change; the dropped len(scanner)==0 SRT guard was dead (strings.Split never returns an empty slice, and the existing len(lines)==0 check still covers empty input). Signed-off-by: Deluan <deluan@navidrome.org> * refactor(lyrics): colocate and unexport cue-normalization helpers Move the cue-normalization machinery out of lyrics.go into a dedicated lyrics_normalize.go (with lyrics_normalize_test.go), leaving lyrics.go to hold just the shared lyric types and their methods. lyrics.go was mixing the domain type/contract definitions with format-agnostic post-processing. Unexport normalizeLyrics, normalizeCueLines, and normalizeLineTiming: they have no callers outside the model package, so they should not be part of its public API. NormalizeCueEnds stays exported because the Subsonic enhanced-lyrics serializer (server/subsonic/lyrics.go) resolves cue ends per agent group while building the response; that is the only legitimate cross-package caller. Also includes a small no-op robustness tweak in parseLRC: len(times) == 0 instead of times == nil (equivalent here, more idiomatic). No behavior change. * test(lyrics): add direct coverage for NormalizeCueEnds NormalizeCueEnds is exported and carries the most intricate logic in the normalization cluster (fill-from-next, fill-from-fallback, both clamps, and the all-or-none clear), but was only exercised transitively. Add a focused spec covering each branch plus the empty-input and no-mutation guarantees, bringing the function to 100% coverage. * test(lyrics): cover legacy getLyrics across formats and sources Expand the legacy getLyrics e2e coverage from a single embedded-plain case to a table over all six fixtures: embedded LRC/plain/TTML and sidecar LRC/SRT/YAML. Each case asserts the v1 plain-text fallback contract — the structured lyric is flattened to LRC-style plain text with no timing markup leaking through (no LRC brackets, SRT arrows, or XML tags), regardless of the source format or whether it is embedded or a sidecar file. This pins the behavior that synced TTML/SRT/ YAML formats degrade gracefully to plain text on the legacy endpoint. * test(lyrics): cover songLyrics v1 vs v2 with word-level fixtures Correct and expand the e2e lyrics coverage to match the OpenSubsonic songLyrics extension contract: - v1 (getLyricsBySongId, no enhanced): line-level lyrics with no cueLine, kind, or agents — even for word-level formats (ELRC, Lyricsfile YAML). - v2 (getLyricsBySongId?enhanced=true): word-level cueLine surfaces for ELRC and YAML sources; kind="main" is set; a line-level source (SRT) still yields no cueLine even when enhanced. - legacy getLyrics (artist/title): the original Subsonic endpoint, flattening any format to plain text. A prior commit mislabeled this as the "v1 contract"; getLyrics predates OpenSubsonic and is unrelated to the extension versions. Drive these with the public-domain tests/fixtures/lyrics files (the same set the parser benchmarks use) so the e2e content stays in sync and actually carries the word-level timing needed to distinguish v1 from v2. The embedded "synced LRC" fixture is upgraded to ELRC (word-level); track counts are unchanged, so the rest of the suite is unaffected. * test(lyrics): parameterize v2 enhanced coverage across all formats Convert the v2 (enhanced) e2e block from three ad-hoc cases into a DescribeTable covering all six formats, matching the v1 and legacy tables. Each entry declares whether the source carries word-level timing: ELRC, TTML, and Lyricsfile YAML surface a cueLine; LRC, SRT, and plain text do not. All six get kind="main". Add word-level <span> timing to the first line of the auld-lang-syne.ttml fixture so TTML exercises the word-level cueLine path (the parser already supports <span begin/end>, but the fixture was line-level only). The first line now yields the same five word cues as the ELRC and YAML fixtures, keeping the table assertions uniform across formats. * fix(lyrics): honor caller language when Lyricsfile YAML omits it parseLyricsfile discarded the caller's language argument, so a Lyricsfile YAML parsed from an embedded tag or plugin response with no metadata.language was labeled "xxx" even when ParseLyrics was given a language. The SRT and TTML parsers already use the caller language as their default; fall back to it here too, preferring the document's own metadata.language when present. Also reword a misleading TTML comment: isTTMLDocument still runs an XML decode (it stops at the first element), so the skip avoids the full TTML parse, not the XML decoder entirely. * refactor(lyrics): consolidate lyrics parsing functions names Signed-off-by: Deluan <deluan@navidrome.org> * test(lyrics): drop test-only parse wrappers after parser rename Commit 48c0173e8 renamed the production parsers to parseTTML/parseSRT, which collided with the same-named test-only wrappers and broke the model test build (parseTTML/parseSRT redeclared). Remove the wrappers and call the production parsers directly with the placeholder language at each test site. * test(lyrics): complete the truncated enhanced-LRC fixture The auld-lang-syne.elrc fixture stopped after the first two stanzas (8 lyric lines) while every other format fixture carries the full 24-line song. Extend it to all 24 lines with per-word timing so it is a faithful enhanced-LRC sample and the EnhancedLRC parser benchmark runs on a workload comparable to the others. The first line's word timings are unchanged, so the e2e cueLine assertions still hold. --------- Signed-off-by: Deluan <deluan@navidrome.org>	2026-06-19 18:25:35 -04:00
Yuuta	3a14faa033	feat(subsonic): add structured sidecar lyrics support with OpenSubsonic v2 karaoke cues and agent layers (#5076) ... Expand backend lyrics support with richer sidecar formats and upgrade the OpenSubsonic songLyrics implementation to the version 2 structured karaoke contract, while preserving version 1 behavior by default. Sidecar formats and parsing: - Add a TTML parser (core/lyrics/ttml.go): clock time, offset time, bare decimal seconds, nested timing contexts, and token-level <span> timing for word/syllable karaoke. Parses Apple Music-style metadata tracks (translation and pronunciation/transliteration) and agent metadata into per-track agents[] plus per-cue-line agentId. Hydrates missing line timing from cue timing. - Add an SRT parser (core/lyrics/srt.go). - Add a LRCLIB Lyricsfile (.yaml/.yml) parser (model/lyricsfile.go): maps per-word lines[].words[] to cues with inclusive UTF-8 byte offsets and attributes overlapping lines to synthetic voice agents so parallel vocals split correctly in the enhanced response. - Extend LRC parsing for Enhanced LRC inline <mm:ss.xx> word-timing markers. - Add UTF-8 BOM and UTF-16 LE support for TTML/LRC sidecars. - Parse the above formats from embedded tags as well as sidecar files. Source resolution: - Default lyricspriority is now ".ttml,.yaml,.yml,.elrc,.lrc,.srt,.txt,embedded" so the new formats are discoverable without manual configuration. - Preserve configured source priority across duplicate media-file candidates instead of only checking the first DB match, so higher-priority sidecar lyrics on older duplicates can still win. - Raise the embedded-lyrics tag maxLength to 1 MB to fit word-timed TTML/Enhanced-LRC karaoke for a full song. OpenSubsonic songLyrics v2: - Advertise songLyrics versions [1, 2]. - With enhanced=true, getLyricsBySongId may return structuredLyrics.kind (main/translation/pronunciation), cueLine[] line-level karaoke groupings, cueLine.cue[] timed words/syllables with required UTF-8 byteStart/byteEnd, reusable structuredLyrics.agents[], and cueLine.agentId references. - Without enhanced=true, the response stays v1-compatible: no kind, no cueLine, no agents, no non-main tracks; the existing line[] payload is always populated so legacy clients keep working. Contract details: - cueLine is emitted only for synced lyrics with cue data. - Within a cueLine, cue.end is normalized all-or-none and overlaps are removed; overlaps across separate cueLines remain valid for parallel vocal layers. - Missing cue end-times are filled from the next cue or the parent line. - When cueLines share an index, the one whose agent has role "main" is first. - LyricCue.Value is serialized as XML chardata; cues with nil start are skipped rather than serialized as 0. Refactoring: - Move pure format parsers into model/ (lyrics.go, lyrics_ttml.go, lyrics_srt.go, lyrics_embedded.go, lyricsfile.go) and extract Subsonic response building into server/subsonic/lyrics.go. - Centralize lyric-kind constants and add Lyrics.EffectiveKind/IsMainKind. - Add gg.Clone helper. Spec references: https://github.com/opensubsonic/open-subsonic-api/discussions/213 https://github.com/opensubsonic/open-subsonic-api/pull/218 (songLyrics v2) https://github.com/opensubsonic/open-subsonic-api/pull/228 (cue byte offsets)	2026-06-19 12:00:58 -04:00
Deluan Quintão	6abc2ed517	fix(transcoding): preserve source metadata when transcoding downloads (#5628) ... * fix(transcoding): preserve source metadata when transcoding downloads Default transcoding commands used `-map 0🅰️0` with no metadata mapping, so transcoded files lost all source tags (title, artist, album, etc.). Downloads in the original format were unaffected because the file is copied byte-for-byte. Add `-map_metadata 0 -map_metadata 0:s:0` to the default commands. Both flags are required: `-map_metadata 0` copies format-level tags (MP3/FLAC sources) and `-map_metadata 0:s:0` copies stream-level tags (OPUS/OGG sources), which store tags at different levels. The flags are added in three coordinated places, since for users on the default command the args are built programmatically (buildDynamicArgs) rather than from the stored command string: - consts.go default commands, for new installations - buildDynamicArgs, the active path for default-command users - a migration updating only rows that still hold the exact old default, so customized commands are left untouched AAC is included for consistency but remains a no-op: its `-f adts` container cannot hold metadata, and the MP4 alternative breaks pipe streaming. Fixes #5623 * fix(transcoding): target audio stream for metadata and propagate ctx Address review feedback on the metadata-preservation change: - Use `-map_metadata 0:s:a:0` instead of `0:s:0` to copy tags from the first audio stream specifically. When a source has embedded cover art exposed as a video stream at index 0 (common in music files), `0:s:0` pulls the image stream's metadata and the audio tags are lost. Verified empirically with ffmpeg 7.1.3: a source with video at stream 0 and a tagged audio stream loses its title under `0:s:0` but keeps it under `0:s:a:0`; audio-only OPUS/MP3/FLAC sources are unaffected by the change. - Propagate the migration context via `tx.ExecContext(ctx, ...)` instead of discarding it, so the migration honors cancellation/timeouts. Claude-Session: https://claude.ai/code/session_015iFHDzX53wCKt11qFHMeZk	2026-06-18 09:58:43 -04:00
Deluan Quintão	838ceee26d	perf(subsonic): speed up artist search3 deep-offset pagination (#5620) ... * perf(subsonic): speed up artist search3 deep-offset pagination Empty-query and FTS artist search (search3/search2) paginated via a CROSS JOIN library_artist + DISTINCT in Phase 1 purely for library access control. The DISTINCT forced a temp b-tree over the whole junction table on every page, making deep offsets O(offset): ~200ms at offset 299k on 300k artists. Replace it with a join-free EXISTS predicate keyed on artist.id, backed by a new covering index on library_artist(artist_id, library_id). EXISTS keeps artist as the ordered driver and never fans out rowids, so Phase 1 stays a plain ordered scan that LIMIT/OFFSET can short-circuit. Admin, headless, and all-libraries users skip the filter entirely (the dominant case) for a flat ordered walk over the primary key. Measured on a 300k-artist / 1M-song library: admin/all-libs pagination is ~4.5-5.4x faster at depth (~180ms to ~33ms at offset 400k); restricted subset users keep correct, gap-free pages while also getting faster. The narrowing artist filter is applied at the subsonic layer only when the request targets a strict subset of the user's libraries, so the common case (and the admin fast-path) is never burdened with a redundant predicate. * fix(subsonic): narrow artist search by library set, not count narrowsArtistLibraries decided whether to add the subsonic-layer artist narrowing filter by comparing len(requested) < len(accessible). musicFolderId is not deduplicated, so duplicate IDs inflated the requested count: a user requesting ?musicFolderId=1&musicFolderId=1&musicFolderId=2 against three accessible libraries produced len([1,1,2])==3, which is not < 3, so the filter was skipped and the user saw artists from the third library too. Compare as set membership instead: the request narrows iff some accessible library is absent from it (requested is always a subset of accessible, validated upstream by selectedMusicFolderIds). This is immune to duplicate IDs. Add a regression test that fails against the old length-based check. Also consolidate the repeated EXISTS/no-DISTINCT/O(page) rationale that the prior commit spread across five sites down to a single authoritative comment on ArtistLibraryFilter, with the call sites referencing it. * perf(subsonic): drop redundant library_artist covering index The migration added an index on library_artist(artist_id, library_id) on the theory that the restricted-subset artist-search EXISTS needed it to seek by artist_id. Benchmarking on a 405k-artist / 5-library dataset showed no benefit: the EXISTS subquery constrains both columns (artist_id = and library_id IN), so SQLite already resolves it as a covering-index seek on the existing (library_id, artist_id) UNIQUE autoindex. With the new index present the planner still picks the autoindex and ignores it. Drop the migration and correct the comment. Removing ~11MB of dead index plus its write-amplification on every library_artist insert/delete, for zero query gain. * fix(scanner): mark artists missing when they lose their last library Artist search Phase 1 filters on artist.missing and Phase 2 inner-joins library_artist, so a non-missing artist with no library_artist row (an orphan) takes a pagination slot in Phase 1 and then vanishes in Phase 2, shortening the page and shifting deep offsets. The admin/headless search fast-path walks artist unfiltered, so it is fully exposed to this. Two paths created such orphans without updating artist.missing: - RefreshStats deletes library_artist rows whose stats are '{}' (artist lost all content in a library) after every scan. This is the common source. - Library deletion cascades away the library's library_artist rows. Mark newly-orphaned artists missing at both sources, so the shared 'missing = false' search filter excludes them immediately instead of waiting for a later scan. In RefreshStats the update only runs when the cleanup actually removed rows (the only way a new orphan can appear), so steady-state scans pay nothing; measured ~160ms on 300k artists only when orphans can exist. * refactor(subsonic): address review feedback on artist search filter Code-review follow-ups to the artist search pagination change: - ArtistLibraryFilter: short-circuit to a constant-false predicate when no library IDs are given, avoiding a degenerate empty IN () subquery. - ArtistLibraryFilter: add an inner LIMIT 1 to the correlated EXISTS so SQLite cannot flatten it into a fan-out join (an artist in multiple of the user's libraries would otherwise yield duplicate rowids and corrupt pagination). - narrowsArtistLibraries: compare accessible-vs-requested as a set lookup instead of slices.Contains in a loop. - searchConfig.LibraryFilter: document that a join-free filter is now a correctness requirement (DISTINCT was removed), not just a performance one. * docs: trim verbose comments in artist search/orphan code Condense the over-explained comments added in this PR to the essential 'why', removing repeated cross-references and restatements of the adjacent code. * fix(scanner): heal pre-existing orphan artists on full refresh The orphan-marking added to RefreshStats only ran when its empty-stats cleanup deleted rows, so it reconciled newly-created orphans but not ones already left in the database by older versions (whose library_artist row was deleted before this fix existed). Such legacy orphans would surface in the admin/headless search fast-path as short/gappy pages. Also run the orphan-marking on a full refresh (allArtists), so a full scan — which upgrades commonly trigger and users can run manually — reconciles the backlog. No migration needed; the runtime fixes prevent recurrence. * perf(subsonic): extend artist search fast-path to all-library users applyLibraryFilterToSearchQuery only skipped the library filter for admin and headless processes. A regular (non-admin) user who can access every library has the same result set as an admin, but was still given the EXISTS filter — an O(offset) cost for a predicate that matches every non-missing artist anyway. Skip the filter for them too, using a cheap library CountAll() (a count over the tiny library table) compared against the user's library count. On any error it falls back to the filtered path, which is correct, just slower. * fix(scanner): log error as trailing arg, not explicit error key Signed-off-by: Deluan <deluan@navidrome.org> * test(scanner): e2e guard for orphan artists under PurgeMissing Adds an end-to-end scanner test for the orphan-artist invariant fixed in RefreshStats: with Scanner.PurgeMissing enabled, removing all of an artist's files hard-deletes them, cascades away their media_file_artists rows, and RefreshStats then drops the artist's emptied library_artist row. The test asserts no non-missing artist is left without a library_artist row. Verified it fails without the RefreshStats orphan-marking and passes with it. * test(scanner): assert the orphaned artist is marked missing The orphan e2e test only checked the aggregate no-orphan invariant (orphanCount == 0), which a fully-deleted artist or an un-cleaned row would also satisfy — so it could pass without exercising the fix. Assert Pink Floyd's row specifically: missing=false before, missing=true after, and absent from the non-missing results. Verified it fails without the RefreshStats orphan-marking. * test(scanner): drop misleading non-missing-list assertion for orphan GetAll has no default missing filter, but selectArtist inner-joins library_artist, so an orphaned artist (no junction row) is excluded from the results whether or not it is marked missing. The Not(ContainElement) check therefore passed for the wrong reason. The direct floydMissing() == 1 query is the assertion that actually validates the missing flag; keep that plus the orphan-count invariant and an over-marking guard on The Beatles. * test(scanner): document why orphan check reads the artist row directly Clarify that GetAll cannot observe the orphan: selectArtist inner-joins library_artist, so an artist with no junction row is excluded from results whether or not it is marked missing. Asserting on GetAll would pass even without the fix, so the test reads the artist row directly to check the missing flag. * test(scanner): return descriptive artist state for clearer failures floydState returns PRESENT/MISSING/NOT_FOUND instead of 0/1/-1, so a failure reads '<string>: PRESENT to equal MISSING' rather than '0 to equal 1'. * refactor(subsonic): keep artist library scoping in the repository The search endpoint built a persistence-layer EXISTS predicate (persistence.ArtistLibraryFilter) and injected it into artistOpts.Filters — the only place the subsonic package reached into persistence, leaking a storage detail up two layers. Pass the same Eq{"library_id": ids} filter used for albums and songs, and let the artist repository translate it to the join-free library_artist predicate (scopeSearchToLibraries), where the junction knowledge belongs. The subset-vs- fast-path decision moves there too, so narrowsArtistLibraries and the persistence import are gone from the subsonic layer. Behavior is unchanged; coverage for the translation moves to artist_repository_test. * refactor(persistence): extract canonical markOrphansMissing helper The 'mark non-missing artists with no library_artist row as missing' invariant was hand-written as SQL in two places (RefreshStats and libraryRepository.Delete), in two slightly different dialects (not exists vs id not in). Extract a single artistRepository.markOrphansMissing method next to markMissing and call it from both sites, so the invariant has one definition. * fix(persistence): apply scoped library filter in both search phases Two bugs from moving the artist library-scoping into the repository: - Search() scoped opts.Filters for Phase 1 but still passed the original (unscoped) options to selectArtist, so Phase 2 re-applied the raw Eq{library_id} against the wrong columns and a restricted user's search returned nothing. Pass the scoped opts to both phases. - scopeSearchToLibraries dropped the filter unconditionally for admins, so an admin explicitly narrowing via musicFolderId (e.g. search3?musicFolderId=2) leaked content from other libraries. Compare the request against the user's visible library set (all libraries for admin/headless), narrowing whenever it is a strict subset. Both regressions were caught by the server/e2e multi-library suite. * fix(core): delete library and reconcile orphans in one transaction libraryRepository.Delete runs the FK-cascade delete and the orphaned-artist reconciliation (markOrphansMissing) as two writes on r.db. Called directly they autocommit separately, so an interruption between them could leave non-missing artists with no library_artist row — the orphan state the artist search fast-path forbids. Wrap the deletion in ds.WithTx at the core wrapper so both writes commit atomically; the watcher/scanner/broker side-effects stay post-commit. * refactor(persistence): unify artist search library scoping into one filter Phase 1 previously applied two overlapping library predicates: cfg.LibraryFilter (scoped to the user's libraries) AND options.Filters (the requested subset), producing two correlated EXISTS subqueries per rowid even though the request is always a subset of the user's libraries. And the 'does this user see everything' decision was implemented twice (userHasAllLibraries via CountAll vs scopeSearchToLibraries via set-membership), with applyLibraryFilterToSearchQuery as a third scoping path. Resolve the effective library scope once in Search() via searchScope (intersect the requested set with the user's visible libraries; nil = fast-path), clear opts.Filters, and realize that single scope as the only Phase-1 LibraryFilter. The visibility logic is now one pipeline: requestedLibraryIDs + visibleLibraryIDs + userSeesAllLibraries. Behavior unchanged; one EXISTS instead of two on the hot path, one source of truth for library visibility. * fix(persistence): harden artist search against malformed library_id filter Search consumed only an Eq{"library_id": []int} filter; an Eq whose library_id value wasn't []int slipped through unconsumed and would reach Phase 1's bare artist table (no library_id column) → SQL error. Recognize any Eq carrying a library_id key (isLibraryIDFilter) and always consume it, falling back to the user's visible scope for a malformed value. Non-library filters are still left in place for doSearch. * refactor(persistence): trim redundant comments and unexport artist library filter The artist-search-pagination work left dense explanatory comments, with the join-free / LIMIT-1 anti-flatten rationale and the orphan-artist mechanics each restated in several places. Consolidate each rationale into one canonical home (artistLibraryFilter for the EXISTS/LIMIT-1 trick, markOrphansMissing for the orphan lifecycle) and have the other sites reference it instead of repeating it. Also unexport ArtistLibraryFilter to artistLibraryFilter: its only caller is searchCfg in the same package and no test references it, so it never needed to be part of the package's exported surface. Comments only plus the rename; no behavior change. * refactor: add slice.ToSet and use it for the artist search subset check searchScope's subset test compared the requested libraries against the visible set with a nested slices.Contains, which is O(visible * requested). On an instance with many libraries (e.g. 100 libraries, a user granted 99) and an explicit musicFolderId request, that is ~9.8k comparisons; with a set it is ~200. Add a small reusable slice.ToSet helper (a slice -> map[T]struct{} set, collapsing duplicates) and use it to make the membership lookups O(1), restoring O(n+m) without the throwaway struct{}{} literal that an inline ToMap would need. No behavior change. * refactor(artist): move artistLibraryFilter to artist_repository Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org>	2026-06-16 21:47:15 -04:00
Deluan Quintão	08a027dbcc	fix(transcoding): honor player forced format on the WebUI transcode flow (#5613) ... * feat(stream): add ClientInfo.ForceFormat for browser-aware forced format Restricts the client to a forced transcoding format and suppresses direct play, but only when the client declares it supports that format. Part of #5583. * fix(transcoding): honor player forced format on getTranscodeDecision When the WebUI player has a forced transcoding format configured and the browser declares it can play that format, transcode to it (suppressing direct play). Fall back to normal negotiation with a warning when the format is unsupported. The MaxBitRate cap still applies on top. Fixes #5583. * test(e2e): cover player forced format on getTranscodeDecision Forced format honored when the client supports it, falls back to negotiation otherwise, and the MaxBitRate cap still applies on top. Part of #5583. * feat(ui): remove obsolete 'format ignored' helper text on player form The web player now honors the forced transcoding format, so the caveat added in #5611 no longer applies. Reverts the Transcoding field to a plain selector. Part of #5583.	2026-06-14 20:52:19 -04:00
Deluan Quintão	c4c70519b5	fix(transcoding): enforce server-side player MaxBitRate on /rest/stream (#5611) ... * fix(transcoding): enforce player MaxBitRate on getTranscodeDecision The Web UI streams via getTranscodeDecision, which (since #5473) ignored the server-side player config. Apply the player's MaxBitRate as a bitrate ceiling on the client's declared limits before MakeDecision, restoring per-player bitrate enforcement without reintroducing the forced-format override. Fixes #5583. * test(e2e): assert player MaxBitRate is enforced on getTranscodeDecision Invert the assertions added in #5473 that expected the player cap to be ignored; getTranscodeDecision now enforces it (issue #5583). * feat(ui): clarify web player ignores forced transcoding format Add helper text to the Transcoding field on the player edit form when the player is the NavidromeUI web client, since it enforces only the Max. Bit Rate, not the forced format. Part of issue #5583. * refactor(stream): extract ClientInfo.CapBitrate, share across transcode paths Move the player MaxBitRate ceiling logic into a canonical ClientInfo.CapBitrate method in core/stream, used by both getTranscodeDecision and the legacy ResolveRequest path. Removes handler-layer duplication and corrects a misleading comment that wrongly implied the legacy single-field cap was buggy. * fix(transcoding): downsample on legacy /stream when only player MaxBitRate is set A bare /stream or /download request from a player configured with a server-side MaxBitRate (but no forced format) was served raw, ignoring the cap. buildLegacyClientInfo now triggers DefaultDownsamplingFormat when the player MaxBitRate alone is below the source bitrate, matching the already-correct forced-format and request-bitrate paths. Part of #5583. * fix(ui): add Brazilian Portuguese translation for player transcoding helper text Translates the new resources.player.helperTexts.transcodingId key added for the web player transcoding-format clarification. Part of #5583. * fix(ui): restore Transcoding field styling and render helper text The TranscodingInput wrapper swallowed the variant SimpleForm injects into its direct children (field lost its outlined box) and put helperText on the ReferenceInput, which does not forward it to the input. Spread the form props onto ReferenceInput and move helperText to the SelectInput child so both the outlined styling and the helper text render. Part of #5583. * fix(i18n): update Brazilian Portuguese translation for album artist field Signed-off-by: Deluan <deluan@navidrome.org> * fix(ui): clean up comments in PlayerEdit component Signed-off-by: Deluan <deluan@navidrome.org> * test(ui): mock useTranslate in PlayerEdit test for determinism Avoid depending on ra-core's out-of-provider translation behavior, which can vary by version. Part of #5583. --------- Signed-off-by: Deluan <deluan@navidrome.org>	2026-06-14 16:52:01 -04:00
Deluan Quintão	c466f6b612	fix(artwork): prevent WebP segfault on 32-bit and disable WebP-by-default in Docker (#5606) ... * fix(artwork): avoid WebP segfault on 32-bit ARM On 32-bit ARM, the gen2brain/webp native libwebp path uses ebitengine/purego reverse callbacks, which purego does not support on that architecture. Selecting it crashes the process with a SIGSEGV when encoding or decoding WebP cover art, taking down the whole server on the first web UI artwork request (issue #5597). Force the safe WASM path on armv7/v6 in two layers: build the Docker arm binary with the gen2brain/webp "nodynamic" tag so purego is never linked, and add a runtime GOARCH guard in the init hook so source builds on 32-bit ARM are also protected. arm64 keeps the native libwebp path. * fix(artwork): also disable native WebP on 32-bit x86 purego's callback implementation is built with the constraint !386 && !arm, so 32-bit x86 (386) crashes with the same SIGSEGV as 32-bit ARM when the native libwebp path is used. Navidrome ships linux/386 and windows/386 builds, so guard 386 alongside arm: extend the runtime GOARCH check and the Docker nodynamic build tag to cover both. 64-bit arches keep the native libwebp path. * fix(artwork): rely on nodynamic build tag, drop ineffective runtime guard The previous runtime GOARCH guard did not actually prevent the crash: gen2brain/webp selects the native (purego) vs WASM backend in its own package init() and registers the purego write callback at import time, before any Navidrome hook runs. webp.Dynamic() is only a status getter, and Decode/Encode branch on the library's unexported flag, so the guard merely skipped a log line while the native path stayed active. The effective fix is the nodynamic build tag (applied for 32-bit ARM and x86 in the Dockerfile), which compiles gen2brain/webp WASM-only so purego is never linked. Drop the misleading guard and document that source builds on 32-bit architectures must be built with -tags nodynamic. * fix(artwork): don't enable WebP encoding by default in Docker The Docker image set ND_ENABLEWEBPENCODING=true, which (a) forced cover-art thumbnails through WebP for every install and (b) overrode any EnableWebPEncoding=false set in the user's navidrome.toml, since env vars take precedence over the config file in Viper. On 32-bit platforms the only available WebP backend is the WASM encoder, which is slow on the underpowered hardware those builds typically run on, so enabling it by default is the wrong tradeoff there. Remove the env default and leave EnableWebPEncoding off unless the user opts in. Combined with the nodynamic build tag, 32-bit images neither crash nor pay the WASM cost out of the box. A smarter automatic policy (use WebP only when native libwebp is available) can be revisited separately.	2026-06-13 13:58:26 -04:00
Deluan Quintão	af78bdeb3a	fix(artwork): never serve artist folder images as album art (#5596) ... * test(artwork): add failing e2e tests for artist image leaking as album art Reproduces a v0.62.0 regression (#5451/#5457): the album cover-art parent-folder fallback can include the artist folder, serving the artist thumbnail (e.g. Artist/folder.jpg) as album art for any album without image files in its own folder(s). Covers three scenarios: a plain Artist/Album layout with no album images, a single-disc album spread across sibling folders under the artist folder, and a spread album whose own front.jpg is shadowed by the artist's cover.jpg via CoverArtPriority order. Also adds an albumByName test helper for multi-album layouts. The tests are expected to fail until the parent-folder inclusion is gated by a structural check (skip the common parent when audio from other albums lives under it). * fix(artwork): never serve artist folder images as album art The album cover-art parent-folder fallback (introduced in #5451/#5457) could include the artist folder as a source of album images, serving the artist thumbnail (e.g. Artist/folder.jpg) as cover art for any album without image files in its own folder(s). This affected both plain Artist/Album layouts and single-disc albums spread across sibling folders under the artist folder. Gate the common-parent inclusion with a structural check: the parent only qualifies as an album root when no audio belonging to other albums lives in it or anywhere beneath it. An artist folder contains other albums' tracks, while an album root above disc subfolders contains only this album's, so the check works for any disc folder naming scheme and never affects the multi-disc fixes from #5376/#5456. A single-album artist with no images anywhere remains structurally indistinguishable from an album root and is a known residual case. * refactor(artwork): move album-root audio check into folder repository Replace the raw subtree SQL (LIKE/ESCAPE expression and wildcard escaping) that lived in core/artwork with an explicit FolderRepository.HasAudioOutsideFolders method, implemented in the persistence layer next to the existing folder-subtree query pattern. This also removes the test mock's brittle dispatch that sniffed the generated SQL to recognize the query; the fake now overrides the new method directly. Extract the whole parent-folder resolution from loadAlbumFoldersPaths into an albumRootParent helper, flattening four levels of nesting back into a linear flow. Behavior is unchanged; the unit test for a parent containing audio moved to the persistence suite, with added coverage for subtree boundaries, missing folders, and LIKE-wildcard escaping in folder paths. * refactor(persistence): use exists helper in HasAudioOutsideFolders Replace the hand-rolled count(*) query with the repository's canonical exists helper, as suggested in PR review.	2026-06-13 13:29:29 -04:00
Deluan Quintão	da56df3160	feat(smartplaylist): extend isMissing/isPresent to bpm, bitDepth and many text fields (#5603) ... * feat(smartplaylist): support isMissing/isPresent on mbz_* and lyrics fields Mark the six mbz_* MusicBrainz ID columns and the lyrics column as Nullable in the criteria field map, then extend missingExpr to handle string columns where absence is encoded as NULL or empty string (plus '[]' for lyrics). The Numeric/Boolean path (ReplayGain) is preserved via an explicit type check. * refactor(model): make MediaFile BPM and BitDepth nullable pointers Convert BPM and BitDepth fields in model.MediaFile from int to *int so that 'tag absent' is distinguishable from zero. The metadata mapper now uses NullableFloat for BPM (nil when absent or zero/unparseable) and only sets BitDepth when the audio property is non-zero (lossy codecs report 0). All read sites use gg.V() for zero-fallback deref so Subsonic API output and transcoding behaviour are byte-identical to before. The persistence layer bridges the existing NOT NULL DB columns by coercing nil to 0 on write and 0 back to nil on read in PostMapArgs/PostScan; a later migration task will drop those constraints. Hash upgrade safety is verified by a new MediaFile.Hash describe block: nil *int hashes identically to the old int(0) default via ZeroNil+IgnoreZeroValue, so no files will be spuriously re-imported after this change. Extra files touched beyond the plan's list: core/stream/legacy_client_test.go (BitDepth in model.MediaFile literals), persistence/mediafile_repository.go (NOT NULL bridge). * test(model): pin pre-conversion golden hashes for BPM/BitDepth * feat(smartplaylist): support isMissing/isPresent on bpm and bitDepth * feat(db): make bpm and bit_depth columns nullable, backfill 0 to NULL Drop the NOT NULL constraint on media_file.bpm and bit_depth via a lossless migration that converts legacy 0-means-absent values to real NULL. Remove the temporary shim in PostScan/PostMapArgs that was bridging the old NOT NULL columns to the *int model fields. Add round-trip persistence tests asserting NULL storage for nil pointers and correct value round-trip for non-nil pointers. * test(e2e): verify isMissing/isPresent partition for nullable fields Add DescribeTable covering bpm, bitdepth, lyrics, and mbz_recording_id: for each field, isMissing + isPresent song counts must equal the total library count, proving the nullable-column SQL is exhaustive and correct. * test(e2e): seed bpm tag so isMissing/isPresent partition is non-trivial * fix(model): omit bitDepth from JSON when absent instead of emitting null * feat(smartplaylist): support isMissing/isPresent on more string fields Enable isMissing/isPresent operators for album, comment, catalognumber, discsubtitle, albumcomment, sorttitle, sortalbum, sortartist, sortalbumartist, and explicitstatus by marking them Nullable in fieldMap. * refactor(smartplaylist): unify missingExpr column logic into one flow Collapse the numeric/string fork in missingExpr into a single empties-driven loop (numeric/boolean fields simply have no empties), and replace the duplicated IsTag/IsRole guard with a three-way switch that expresses the dispatch model once. No SQL semantics change for string fields; numeric/boolean fields now emit a single-element Or/And which squirrel parenthesizes (e.g. `(col IS NULL)` instead of bare `col IS NULL`) — update the affected test expectations accordingly.	2026-06-13 13:15:20 -04:00
Deluan Quintão	3b958dd6a7	refactor(stream): remove dead type branches from getIntClaim (#5594) ... The jwx library always deserializes numeric claims from a parsed token as float64, so the int and int64 branches in getIntClaim could never succeed and were dead code. Keep only the float64 path, which is the one actually exercised by the token round-trip, and update the comment to document the library behavior.	2026-06-10 23:15:58 -04:00
Deluan Quintão	bc107d1cee	fix(scrobbler): proxy NowPlaying even when ignoreScrobble is set (#5559) ... * fix(scrobbler): proxy NowPlaying even when ignoreScrobble is set When a client reports playback with ignoreScrobble=true, the reportPlayback handler suppressed both the scrobble submission and the NowPlaying update sent to external agents (Last.fm, ListenBrainz, plugins). These are independent concerns: ignoring the scrobble submission should not stop Navidrome from telling external services what is currently playing. The !params.IgnoreScrobble guard now applies only to the scrobble submission and play-count path; the NowPlaying dispatch is gated solely by the player's ScrobbleEnabled flag. This mirrors the legacy scrobble endpoint, where submission=false has always still set NowPlaying. * test(scrobbler): assert no scrobble dispatch when ignoreScrobble=true Address PR review feedback: explicitly verify that ignoreScrobble=true suppresses the scrobble submission (not just the play count) while NowPlaying is still dispatched, so the flag cannot regress into ignoring nothing. Also expand the NowPlaying gating comment to spell out the IgnoreScrobble vs ScrobbleEnabled rules and identify the external agents involved.	2026-06-03 20:03:08 -04:00
Deluan	2a43c4683e	chore: go fix ... Signed-off-by: Deluan <deluan@navidrome.org>	2026-05-28 22:13:05 -03:00
Deluan	833c50adc7	test(stream): fix data race in MediaStreamer transcoding cap tests ... The three It blocks that build a tight-cap streamer each spawned a fresh transcoding cache without waiting for its background initialization. The init goroutine reads conf.Server.CacheFolder, which races against SnapshotConfig's pointer-swap restore (Server = &restored) fired by DeferCleanup at the end of the spec. CI tripped the race under -shuffle=on -race; locally it reproduced about 10% of the time. Wait for tightCache.Available() before constructing the streamer, mirroring the outer BeforeEach. For the slot-saturation spec, swap in a blocking io.Pipe-backed mock ffmpeg so the cache's background copyAndClose can't drain the source and release the slot — the previous behavior happened to work only because the cache wasn't yet available and the no-cache path was exercised.	2026-05-28 00:07:49 -03:00
Deluan Quintão	74a5c0c6d1	fix(playlists): preserve unchanged fields on partial REST updates (#5542) ... * fix(playlists): preserve unchanged fields on partial REST updates (#5541) The REST adapter for playlists was discarding the `cols` argument that rest.Put provides (the list of fields actually present in the JSON body). updatePlaylistEntity then compared the deserialized entity's zero-valued Name/Comment against the DB row, decided "content changed", and called updateMetadata with &entity.Name — overwriting the name with the empty string. This surfaced via the Playlists list view's bulk "Make Public" action, which sends N parallel `PUT /api/playlist/{id}` requests with body `{"public": true}`. Affected playlists ended up with their names wiped (UI showed "Loading..." indefinitely). The per-row Public toggle was unaffected because it spreads the full record into the payload. Honor the cols list: gate every field-change check and every pointer passed to updateMetadata by whether the field was actually in the request body. Empty cols falls back to the existing "treat as a full record" behavior so non-REST callers are unaffected. * test(playlists): cover rules-only PUT + case-variant owner-change guard Follow-ups from manual testing and code review of the prior commit: - Manual testing confirmed Feishin-style rules-only PUT works correctly on the fix; add ginkgo regression tests for rules-only update, name+ rules combined, idempotent rules PUT (no-op), and bulk Make-Public preserving rules on smart playlists. - Keep the non-admin owner-change permission check gated on the deserialized entity content (not on `sent("ownerId")`) so a case-variant JSON key like {"OwnerId":"x"} can't downgrade the 403 to a silent 200. Go's json decoder is case-insensitive on struct field matching but rest.Put's field-name extraction is case- sensitive; the entity-based guard catches both spellings. The apply-side gating on ownerChanged still prevents the actual mutation, so this was a behavioral (not security) regression, but worth fixing. Adds a regression test asserting the case-variant key still returns rest.ErrPermissionDenied. - Correct misleading doc on applyContentUpdate: the path does not rewrite the backing M3U file; it goes through updateMetadata which bumps updatedAt and invalidates cached cover-art URLs. * fix(playlists): match REST cols case-insensitively (PR #5542 review) Go's encoding/json populates struct fields from case-variant keys like {"Name":"x"} or {"OwnerId":"y"}, but rest.Put's getFieldNames extracts raw JSON keys verbatim. With case-sensitive matching, sentFields would ignore the field on the update side — a request with {"Name":"Renamed"} would parse into entity.Name but then sent("name") returns false and the rename silently no-ops. Normalize both sides to lowercase. The entity-based owner-permission guard added in the previous commit remains as belt-and-suspenders but is now redundant with this change. Also clarify the applyContentUpdate doc comment: namePtr/commentPtr are nil when the field is absent OR present-but-unchanged, while publicPtr only tracks presence (an idempotent public is still forwarded). * refactor(playlists): drop redundant entity-based owner-permission guard The case-insensitive sentFields predicate already prevents case-variant JSON keys like {"OwnerId":"x"} from bypassing the ownerChanged check, so the duplicated entity-content guard is no longer load-bearing. Strengthen the regression test into a DescribeTable covering canonical, PascalCase, all-upper, and all-lower spellings to lock in the case-insensitive contract.	2026-05-27 23:29:17 -03:00
Deluan Quintão	823d851b75	refactor(transcoding): rename EnableTranscodingCancellation to Transcoding.EnableCancellation (#5523) ... Move the option into the nested Transcoding config group alongside the limit knobs it interacts with, so all transcoding-related settings live together. The old top-level name is still honored via the existing mapDeprecatedOption / logDeprecatedOptions plumbing, which forwards the value to the new key and logs a deprecation warning at startup. The old struct field is removed (the new field is the single source of truth); the deprecated default is removed so viper.IsSet correctly distinguishes "user set the legacy option" from "no one set it."	2026-05-24 00:51:58 -03:00
Deluan Quintão	945d0ba1e2	fix(transcoding): cap concurrent transcodes to prevent ffmpeg DoS (#5522) ... * feat(transcoding): add MaxConcurrent and MaxConcurrentPerUser config Introduce Transcoding.MaxConcurrent (default NumCPU()*2) and Transcoding.MaxConcurrentPerUser (default 3) to support upcoming concurrency limits on the streaming pipeline. No behavior change yet. Refs #5246 * feat(transcoding): add TranscodeLimiter with global and per-user caps Introduce a non-blocking limiter that gates concurrent transcodes. Returns ErrTooManyTranscodes immediately when the cap is reached so callers can translate it into a 429 response, rather than queuing requests. The per-user reservation is taken first to avoid burning a global slot that would only be rolled back when the per-user cap rejects the caller. Release is idempotent so wrapping the transcoder reader's Close is safe. Refs #5246 * feat(transcoding): cap concurrent transcodes in media streamer Acquire a TranscodeLimiter slot before spawning ffmpeg in the transcoding cache's read function, and release it when the resulting reader is closed. Raw streams and cache hits bypass the limiter so a single saturating client cannot block ordinary playback. When the cap is reached, ErrTooManyTranscodes bubbles up through cache.Get, ready for the HTTP layer to translate into a 429 response. Refs #5246 * feat(transcoding): return HTTP 429 with Retry-After when transcode cap is hit Map stream.ErrTooManyTranscodes to HTTP 429 in both the Subsonic API (/stream, /download) and the public share endpoint, including a 5s Retry-After hint. The Subsonic response still carries a failed-status envelope so clients that ignore HTTP codes also see the failure. Refs #5246 * feat(transcoding): default MaxConcurrent to 0 (disabled) Ship the limiter opt-in so existing installations are not affected by a behavior change on upgrade. Users hitting the DoS reported in #5246 can enable it by setting Transcoding.MaxConcurrent to a positive value (NumCPU()*2 is a reasonable starting point). Refs #5246 * fix(transcoding): make global and per-user caps independent Previously the limiter short-circuited to a no-op whenever MaxConcurrent was zero, silently ignoring a configured MaxConcurrentPerUser. Treat each cap independently so an operator can throttle per-user without enforcing a global ceiling (or vice versa), and only fall back to the no-op limiter when both caps are disabled. * fix(archiver): abort archive download when the transcode limiter rejects The album/artist/playlist zip writers were silently producing zip entries with headers but no data when ms.NewStream returned ErrTooManyTranscodes, because the per-file error was discarded by `_ = a.addFileToZip(...)`. The client received HTTP 200 with a corrupt zip and no indication that the server was rate-limited. Now the zip loop bails out as soon as it sees ErrTooManyTranscodes, and the Download handler swallows the error (the response status and Content-Disposition are already flushed by the time the limit is hit, so no 429 can be sent). The truncated zip surfaces the problem to the client; operators see a clear "transcode cap reached" warning in the server logs. Refs #5246 * fix(transcoding): release limiter slot on client close, not ffmpeg EOF Previously the slot was wrapped around the ffmpeg source reader, so it was only released by the cache's background copyAndClose goroutine when ffmpeg finished producing the file — meaning a client that disconnected after a single byte still held the slot for the full transcode duration. Under MaxConcurrent=N this serialized fresh requests behind abandoned encodes for minutes. Hand the release function back from the cache producer via the streamJob struct and wire it into the consumer-side Stream.Close. The HTTP handler already runs `defer stream.Close()`, so disconnect now frees the slot immediately. Cache hits never enter the producer and still pay no slot, and singleflight waiters on the same key correctly inherit no release (only the original producer's job holds the slot). Refs #5246 * fix(transcoding): skip per-user cap for anonymous requests Public share viewers have no user in context, so userName(ctx) returned the literal string "UNKNOWN" and the limiter mapped every anonymous viewer to the same bucket. With MaxConcurrentPerUser=N, only N unrelated anonymous clients could stream a viral share at any time — the opposite of the fairness the per-user cap is meant to provide. Introduce a limiterKey(ctx) helper that returns "" for anonymous callers (userName(ctx) is unchanged for logs), and teach Acquire to skip the per-user reservation when the key is empty. The global cap is still enforced for anonymous traffic and remains the protection against runaway anonymous load. Refs #5246 * refactor(transcoding): tidy limiter struct and centralize Retry-After Per review feedback: - Drop the redundant maxConcurrent field on transcodeLimiter; the channel capacity already enforces the global cap and the field was only used inside the constructor. - Only allocate the perUser map when MaxConcurrentPerUser > 0. - Move the Retry-After value into core/stream as RetryAfterSeconds so the Subsonic API and public-share handlers cannot drift if the window is later tuned. * fix(transcoding): do not log limiter rejections as cache failures NewStream was emitting an error-level "Error accessing transcoding cache" log whenever cache.Get returned anything non-nil, including the limiter's ErrTooManyTranscodes — even though the producer had already logged the rejection at warn level. The result was double logging and a misleading "cache failure" classification that buries real cache problems. Skip the error log when the cause is ErrTooManyTranscodes; the warn line from the producer is the canonical signal. * fix(archiver): open stream before writing zip entry header Per review: addFileToZip previously called z.CreateHeader before NewStream, so when the limiter rejected a transcode the zip already contained a 0-byte entry for that track. Open the source first and only write the header once the read side is ready; rejections now skip the entry entirely. The truncation comment in handleArchiveErr was also misleading — z.Close finalises the central directory, so the client receives a well-formed zip containing only the tracks written before the rejection, not a "truncated" archive. Reword to match reality. * fix(transcoding): hold slot for ffmpeg lifetime, force cancellable ctx The previous release-on-consumer-close design let a client open many unique transcodes, disconnect immediately, and still spawn the configured cap's worth of ffmpeg processes — the cache writer goroutine continued draining ffmpeg to disk after the client disappeared, defeating the DoS protection the limiter is meant to provide. Move the release back onto the source reader so the slot is freed only when ffmpeg actually exits (either EOF or context cancellation). To keep disconnects from leaking slots for the full transcode duration, force the request context into ffmpeg whenever the limiter is enabled — so client disconnect cancels the process and frees the slot promptly. When the limiter is disabled, the legacy EnableTranscodingCancellation behavior is preserved unchanged. Reported by codex and Copilot reviewers on #5522.	2026-05-24 00:24:30 -03:00
Deluan	03ac02d964	refactor: more warnings clean up ... Signed-off-by: Deluan <deluan@navidrome.org>	2026-05-20 17:43:12 -03:00
Deluan	efe9291db0	refactor: multiple syntax updates for Go 1.26 ... Signed-off-by: Deluan <deluan@navidrome.org>	2026-05-19 18:02:36 -03:00
Deluan Quintão	8f0b4930ff	refactor(conf): replace eager dir creation with lazy Dir type (#5495) ... * feat(conf): add Dir type with lazy directory creation Introduces the Dir type that wraps a directory path string and defers os.MkdirAll until the first call to Path() or MustPath(), using sync.Once to ensure the creation happens exactly once. Implements fmt.Stringer, encoding.TextMarshaler, and encoding.TextUnmarshaler for config integration. Includes Ginkgo/Gomega tests covering all methods and error paths. * refactor(conf): replace eager dir creation with lazy Dir type Change DataFolder, CacheFolder, Plugins.Folder, and Backup.Path from string to Dir. Remove all os.MkdirAll calls from Load() so directories are created lazily on first Path()/MustPath() call. Artwork folder creation was already handled at point-of-use in image_upload.go. Add SnapshotConfig() to conf package for safe test config save/restore that avoids copying sync.Once inside Dir fields. Fix copy-lock vet warning in nativeapi/config.go by marshalling pointer instead of value. * refactor(conf): migrate tests and db init to lazy Dir type Update all test files to use conf.NewDir() for Dir field assignments. Ensure DataFolder is created lazily when the database is first opened in db.Db(). Remove eager directory creation from conf.Load() tests. * fix(conf): address review findings for Dir type - Use os.ModePerm for DataFolder/CacheFolder (was 0700, should match original behavior). Add NewDirWithPerm for PluginsFolder (0700). - Use Path() instead of MustPath() in db.Prune() to avoid logFatal from background cron job. - Panic on marshal/unmarshal errors in SnapshotConfig (test helper). - Clean up redundant String()/MustPath() calls in plugin manager. - Remove dead code in dir_test.go. Signed-off-by: Deluan <deluan@navidrome.org> * fix(conf): add GoString to Dir for clean config dump output Implement fmt.GoStringer on Dir so pretty.Sprintf shows the path string instead of internal struct fields (sync.Once, perm, err). Also add TODO comment to configtest about removing the indirection. * fix(dir): improve error logging in MustPath method Signed-off-by: Deluan <deluan@navidrome.org> * refactor(tests): remove redundant tests for unwritable DataFolder and CacheFolder Signed-off-by: Deluan <deluan@navidrome.org> * fix(conf): address PR review feedback - Ensure Plugins.Folder always uses 0700, even when user-configured (previously only the derived default got restrictive permissions). - Create LogFile parent directory before opening, so LogFile paths inside a not-yet-created DataFolder work correctly. --------- Signed-off-by: Deluan <deluan@navidrome.org>	2026-05-13 17:44:22 -03:00
Deluan Quintão	24e526e09a	fix(transcoding): place -ss before -i for fast input seeking (#5492) ... Move the ffmpeg -ss (seek/offset) parameter before -i in all transcoding commands so ffmpeg uses input seeking instead of output seeking. Per the ffmpeg docs, placing -ss before -i seeks at the demuxer level by keyframe (very fast), and since FFmpeg 2.1 it is also frame-accurate when transcoding. The previous placement after -i caused ffmpeg to decode and discard all audio up to the seek point, which was unnecessarily slow — especially problematic for lengthy files (4+ hours). Both code paths are updated: buildDynamicArgs (for default formats) and createFFmpegCommand (for custom templates without %t). A database migration updates existing default commands in the transcoding table.	2026-05-13 17:17:20 -03:00
Deluan Quintão	b18dfb474a	fix(transcoding): don't apply server-side override on getTranscodeDecision (#5473) ... * fix(transcoding): don't apply server-side transcoding override on getTranscodeDecision The getTranscodeDecision endpoint was incorrectly applying server-side player transcoding overrides (forced format and MaxBitRate cap), which replaced the client's declared capabilities with synthetic profiles. This caused the endpoint to ignore what the client can actually play and return decisions for formats the client never requested (e.g. AAC when the client only supports FLAC/opus/mp3). The override is now gated behind an ApplyServerOverride flag in TranscodeOptions, which is only set by the legacy stream endpoint where this behavior is expected. Signed-off-by: Deluan <deluan@navidrome.org> * refactor: move server-side transcoding override to ResolveRequest Moved the server-side player transcoding override logic (forced format and MaxBitRate cap) from MakeDecision into ResolveRequest, where the legacy stream context is handled. This makes MakeDecision a pure function that only operates on the ClientInfo it receives, removing the ApplyServerOverride flag and all context-sniffing from the decision engine. Tests moved accordingly to legacy_client_test.go. * test(e2e): update transcode decision tests for server override removal Updated e2e tests to reflect that getTranscodeDecision no longer applies server-side player overrides (MaxBitRate cap and forced transcoding profile). The player MaxBitRate tests now verify the endpoint ignores the player cap and relies solely on client-declared capabilities. * test(e2e): assert opus default bitrate when player cap is ignored Added bitrate assertion to verify the player MaxBitRate cap is truly ignored: the target bitrate should be the opus format default (128kbps), not the player cap (320kbps). --------- Signed-off-by: Deluan <deluan@navidrome.org>	2026-05-06 10:03:24 -04:00
Deluan Quintão	f48416685f	fix(artwork): fix stale cache and top-level album artwork for multi-disc albums (#5457) ... * fix(artwork): include top-level album folders in parent cover art lookup The Path != "." guard added in #5451 was too aggressive — it excluded any folder with Path=".", which includes top-level album folders (not just the library root). Changed to ParentID != "" which correctly excludes only the actual library root folder. Fixes #5456 * fix: correct comment in test — album is under library root, not artist root * test: add ascii tree diagram to top-level album e2e test * test: replace internal bug references with issue link in e2e comments Signed-off-by: Deluan <deluan@navidrome.org> * test: add e2e test matching reporter's exact library layout (#5456) Adds a deeply nested test (Genre/Artist/Album/Disc) with 12 discs using the reporter's actual folder names to verify artwork resolution works for non-top-level album folders too. * fix(scanner): use a syntectic admin user when no admin user is found Signed-off-by: Deluan <deluan@navidrome.org> * fix(scanner): bump album UpdatedAt on Phase 3 refresh to invalidate artwork cache When Phase 3 corrects an album's FolderIDs (or any other field), bump UpdatedAt to the current time. This ensures the artwork cache key changes, invalidating any stale artwork that was resolved and cached during Phase 1 when the album had incomplete folder data. * fix(artwork): include ImportedAt in artwork cache key to invalidate stale cache Reverts the Phase 3 UpdatedAt bump (which would change album.UpdatedAt semantics) and instead includes album.ImportedAt in the artwork cache key computation. Since ImportedAt is bumped to time.Now() on every album Put, any Phase 3 correction naturally invalidates cached artwork that was resolved mid-scan with incomplete folder data. * fix(artwork): simplify lastUpdate logic using TimeNewest utility Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org>	2026-05-04 17:26:39 -04:00
Deluan Quintão	a00152397e	fix(artwork): prefer album-root images over disc-subfolder images for multi-disc albums (#5451) ... Fixed two bugs in album cover art resolution for multi-disc layouts: 1. compareImageFiles now sorts by path depth (shallower first) when basenames tie, so album-root images like Artist/Album/cover.jpg are preferred over disc-subfolder images like Artist/Album/CD1/cover.jpg. 2. commonParentFolder now includes the parent folder for single-disc-subfolder albums, with a Path != "." guard to avoid pulling artist-folder images. Closes #5376	2026-05-02 19:48:44 -04:00
Deluan Quintão	ae0e0c89d9	feat(plugins): add PlaybackReport to scrobbler capability (#5452) ... * feat(plugins): add PlaybackReport to Scrobbler interface and all implementations * feat(plugins): add PlaybackReport worker and dispatch in PlayTracker * feat(plugins): add PlaybackReportRequest to plugin scrobbler capability * chore(plugins): regenerate PDK files with PlaybackReport * feat(plugins): add PlaybackReport to test scrobbler plugin * feat(plugins): add PlaybackReport to plugin scrobbler adapter * refactor(plugins): fix double DB fetch in StateStopped and batch getActiveScrobblers - Hoist mf from scrobble branch so PlaybackReport reuses it instead of fetching again from DB - Call getActiveScrobblers once per drain batch instead of per-entry * chore(plugins): include generated scrobbler schema with PlaybackReport * fix(plugins): skip PlaybackReport for plugins that don't export it Plugins detected as scrobblers only need to export one scrobbler function. Older plugins that don't export nd_scrobbler_playback_report would cause noisy error logs on every reportPlayback call. Now errFunctionNotFound and errNotImplemented are treated as no-ops. * refactor: rename NowPlayingInfo to PlaybackReport Signed-off-by: Deluan <deluan@navidrome.org> * refactor: rename stopNowPlayingWorker to stopBackgroundWorkers Signed-off-by: Deluan <deluan@navidrome.org> * refactor: move NowPlaying and PlaybackReport logic to separate worker files Signed-off-by: Deluan <deluan@navidrome.org> * refactor(scrobbler): rename NowPlayingInfo to PlaybackSession and add expired state Rename NowPlayingInfo struct to PlaybackSession to better reflect its role as a complete playback session representation. Add UserId field to make sessions self-contained, removing redundant userId parameters from PlaybackReport interface method and internal dispatch functions. Introduce StateExpired internal state that fires when a session cache entry expires without an explicit stop, ensuring plugins always receive a terminal event regardless of client behavior. * fix(scrobbler): update playback state description to include 'expired' Signed-off-by: Deluan <deluan@navidrome.org> * fix(scrobbler): resolve data race in OnExpiration callback Capture conf.Server.EnableNowPlaying at construction time instead of reading it from the background ttlcache eviction goroutine. The previous code raced with test config cleanup that writes to the same field concurrently. * fix(scrobbler): return error when media file lookup fails in StateStopped Simplify the MediaFile population logic in the stopped case to return an error if the track cannot be found. A stop report with an empty MediaFile is useless to plugins, and returning the error allows clients to retry or alert the user when auto-scrobble is enabled. * refactor(scrobbler): use session data directly in PlaybackReport adapter Use info.Username from PlaybackSession instead of extracting it from context in the plugin adapter, since the session is now self-contained. Add debug/trace logging for session expiration and enqueue the expired report with a user-enriched context so downstream handlers can identify the user. --------- Signed-off-by: Deluan <deluan@navidrome.org>	2026-05-02 16:14:53 -04:00
Deluan Quintão	7e16b6acb5	feat(ui): replace UI scrobble with reportPlayback and redesign NowPlaying panel (#5448) ... * feat(config): add UIPlaybackReportInterval setting * feat(server): expose playbackReportIntervalMs to UI config * feat(ui): add playbackReportIntervalMs config default * feat(ui): replace scrobble/nowPlaying with reportPlayback in subsonic API layer * feat(ui): replace scrobble logic with reportPlayback state machine in Player * refactor(ui): simplify Player heartbeat using useInterval hook - Replace manual setInterval/clearInterval with existing useInterval hook - Extract shared reportPlaybackUrl helper to deduplicate URL construction - Use ref for currentTrackId in beforeunload to stabilize effect deps - Have heartbeat read lastPositionMsRef instead of audioInstance.currentTime * feat(ui): redesign NowPlaying panel with Discord-style layout Show album art with play/pause overlay icon, track title, artist, album name, progress bar with position/duration, and username. * fix(ui): adjust NowPlaying panel height to fit 3 entries * fix(ui): send stopped on player close and tab close while paused - onBeforeDestroy now sends reportPlayback stopped before clearing queue - beforeunload sends stopped beacon regardless of pause state * feat(ui): animate NowPlaying progress bar with 1s client-side tick * fix(ui): account for playbackRate in NowPlaying progress interpolation * fix(ui): use timestamp-based interpolation for smooth NowPlaying progress Replace tick counter with fetchedAt timestamp so the progress bar advances smoothly without resetting on each server poll. * fix(ui): fix NowPlaying progress bar not animating Pass `now` (Date.now()) as a prop that changes every tick, so React.memo'd components actually re-render each second. * fix(ui): prevent progress bar reset on NowPlaying poll Set fetchedAt and now atomically on fetch so the elapsed offset starts at zero and the server's already-estimated positionMs is used as the base without a visible jump. * fix(ui): stamp entries with fetch time to prevent progress bar reset Embed _fetchedAt timestamp directly into each entry object so the position and its reference timestamp are always in the same state update, eliminating the React 17 multi-setState batching race. * fix(server): estimate position for starting state in GetNowPlaying GetNowPlaying was only estimating elapsed position for the "playing" state, returning raw positionMs=0 for "starting". Since the UI player sends "starting" once and then doesn't update until the 60s heartbeat, NowPlaying polls returned 0 for up to a minute, causing the progress bar to reset on every poll. * fix(ui): send playing immediately after starting to enable position estimation The server only estimates elapsed position for "playing" state in GetNowPlaying. The Player was sending "starting" once and then not updating until the 60s heartbeat, leaving the server state as "starting" with positionMs=0 for up to a minute. Now the Player follows up "starting" with an immediate "playing" call, transitioning the server state so position estimation works from the first poll. * fix(subsonic): fix getNowPlaying returning same playerId for all entries PlayerId was never incremented in the map callback, so every entry got playerId=1. This caused the UI to use duplicate React keys, mixing up rendered entries between players. Also use a stable composite key in the UI instead of the sequential playerId. * fix(ui): only send stopped beacon when tab is actually closing Move the reportPlaybackBeacon call from beforeunload to pagehide. beforeunload fires before the confirmation dialog, so cancelling the close would still send stopped. pagehide only fires when the page is actually being unloaded. * fix(ui): revert to beforeunload for stopped beacon pagehide does not fire reliably in Chrome when closing tabs. Use beforeunload instead — if the user cancels the close dialog, the heartbeat will re-register the NowPlaying entry on its next tick. * fix(ui): use synchronous XHR for stopped report on tab close Replace sendBeacon with synchronous XMLHttpRequest in beforeunload. This blocks the page from closing until the server acknowledges the stopped state, ensuring the NowPlaying entry is always removed. * fix(ui): fix confirmation dialog and use fetch keepalive for tab close - Move e.preventDefault() before the stopped report so the dialog always shows regardless of XHR errors - Use fetch with keepalive:true instead of sync XHR (more reliable, non-blocking, survives page teardown) - Fall back to sendBeacon if fetch throws * fix(ui): prevent heartbeat from re-adding entry after stopped on tab close Set a stoppedRef flag in beforeunload so the heartbeat interval skips sending playing reports after stopped has been sent. Without this, the heartbeat could re-register the NowPlaying entry after the stopped event removed it. * fix(ui): include client unique ID header in stopped report on tab close Root cause: reportPlaybackSync (fetch keepalive) did not include the X-ND-Client-Unique-Id header. Regular reportPlayback calls via httpClient include this header, and the server uses it as the playMap key. Without the header, the stopped call fell back to player.ID as the key, which didn't match the entry added with the UUID key. The playMap.Remove targeted the wrong key, so the entry persisted. Fix: export clientUniqueId from httpClient and include it as a header in the fetch keepalive request. * fix(ui): use pagehide for stopped report to avoid premature send beforeunload fires before the confirmation dialog, so the stopped event was sent even when the user cancelled closing. Move the stopped report to pagehide, which only fires when the page is actually being unloaded (after confirmation). * feat(server): broadcast NowPlaying SSE on every state change Previously, the SSE broadcast only fired when the NowPlaying count changed. Now it fires on every reportPlayback call (starting, playing, paused, stopped), so the NowPlaying panel gets instant updates for state transitions and position changes. The UI reducer stores a nowPlayingLastUpdate timestamp alongside the count, ensuring every SSE event triggers a re-fetch even when the count is unchanged (e.g., pause/resume). * fix(ui): clamp NowPlaying position to prevent negative time display * fix(ui): debounce NowPlaying fetches to prevent progress bar trembling During track changes, rapid SSE events (stopped, starting, playing) triggered multiple refetches within milliseconds, each resetting the interpolation base and causing the progress bar to oscillate. Skip fetches within 1 second of the previous fetch. * feat(ui): report playback position on seek Send a reportPlayback(playing) call when the user seeks/scrubs in the player, so the NowPlaying panel and server position stay in sync immediately instead of waiting for the next 60s heartbeat. * refactor: code review cleanup - Export clientUniqueIdHeader from httpClient, use in subsonic layer - Fix variable shadowing (now → fetchNow) in NowPlayingPanel fetchList - Fix onBeforeDestroy nested dep (read isRadio from ref instead) - Only broadcast SSE on state transitions, not heartbeat position updates - Only enqueue NowPlaying to external scrobblers on state transitions Signed-off-by: Deluan <deluan@navidrome.org> * fix(ui): use trailing-edge debounce for NowPlaying fetch Replace the leading-edge throttle (which fetched on the first event and blocked subsequent ones) with a trailing-edge debounce (300ms). During track transitions, the burst of events (stopped → starting → playing) now collapses into a single fetch after the burst settles, showing the new track immediately instead of briefly showing empty. * fix(ui): only show overlay on NowPlaying artwork when paused Signed-off-by: Deluan <deluan@navidrome.org> * refactor(ui): remove unnecessary sendBeacon fallback from reportPlaybackSync * refactor(ui): rename reportPlaybackSync to reportPlaybackKeepalive The function was never synchronous — it uses fetch with keepalive:true, which is fire-and-forget. The name now reflects the actual behavior. * style: format code with prettier * test: add tests for reportPlayback SSE broadcast and UI changes - play_tracker: verify SSE broadcast on every state transition and that broadcasts are skipped when EnableNowPlaying is false - activityReducer: verify nowPlayingLastUpdate timestamp is set - subsonic/index: verify reportPlayback URL construction Signed-off-by: Deluan <deluan@navidrome.org> * fix(ui): prevent NowPlaying from fetching every second when panel is open fetchList had unstable identity because it depended on doFetch (which depended on notify/dispatch). Each 1s setNow re-render recreated the callback chain, re-triggering the useEffect that calls fetchList. Use a ref for the fetch logic so fetchList has a stable identity with empty deps. * fix(ui): break fetch→dispatch→effect→fetch loop in NowPlaying panel The fetch dispatched nowPlayingCountUpdate on every result, which updated nowPlayingLastUpdate in Redux, which triggered the SSE effect to call fetchList again — creating a fetch loop. Fix: remove dispatch from fetch results. The badge count uses entries.length (from local state) when entries are loaded, falling back to Redux count (from SSE) when they aren't. SSE events remain the only trigger for nowPlayingLastUpdate, breaking the loop. * fix(ui): clear NowPlaying entries on panel close so badge uses SSE count * style: format code with prettier * fix: address code review feedback - Fix currentTime truthiness check to handle position 0 correctly - Report actual player state (playing/paused) on seek instead of always sending 'playing' - Remove idx from React key to avoid reorder issues - Add debounce timer cleanup on unmount - Keep entries on panel close so badge stays accurate from polling - Fix test description to match actual assertion * fix(ui): keep NowPlaying badge count accurate from polling Add a separate nowPlayingCountSync action that updates the Redux count without setting nowPlayingLastUpdate (which would trigger the SSE effect and cause a fetch loop). Polling results now sync the badge count via this action, so the badge stays accurate even when SSE is unavailable. * style: format code with prettier --------- Signed-off-by: Deluan <deluan@navidrome.org>	2026-05-01 15:27:32 -04:00
Deluan Quintão	94eb6c522b	feat(subsonic): implement playbackReport OpenSubsonic extension (#5442) ... * feat(req): add Float64Or helper for parsing float query params * feat(scrobbler): extend NowPlayingInfo with state/position/rate fields * feat(scrobbler): implement ReportPlayback with state machine and auto-scrobble * feat(responses): add state/positionMs/playbackRate to NowPlayingEntry * feat(subsonic): add reportPlayback endpoint handler * feat(subsonic): include state/positionMs/playbackRate in getNowPlaying response * feat(subsonic): register playbackReport OpenSubsonic extension * test(e2e): add reportPlayback endpoint e2e tests * refactor(scrobbler): simplify ReportPlayback — extract helpers, remove duplication - Add state constants and exported ValidStates map - Extract remainingTTL() helper (was duplicated 3x) - Merge playing/paused switch cases into single branch - Use Get instead of GetWithParticipants for non-stopped states - Guard NowPlayingCount broadcast with count-change detection - Use cache entry for NowPlaying dispatch instead of extra DB query - Remove redundant Position field from NowPlayingInfo * refactor(scrobbler): skip DB query in playing/paused when playMap has entry * fix(play_tracker): handle errors when adding/updating NowPlayingInfo in cache Signed-off-by: Deluan <deluan@navidrome.org> * refactor(play_tracker): replace sort with slices.SortFunc for NowPlayingInfo Signed-off-by: Deluan <deluan@navidrome.org> * fix(play_tracker): check all ReportPlayback errors in tests Replace _ = with explicit error assertions to avoid masking failures in intermediate calls. Signed-off-by: Deluan <deluan@navidrome.org> * test(e2e): use real PlayTracker and assert getNowPlaying after reportPlayback Replace noopPlayTracker with a real PlayTracker backed by the E2E database. E2E tests now verify the full round-trip: reportPlayback creates/updates/removes entries visible via getNowPlaying, including state, positionMs, and playbackRate fields. Export NewPlayTracker constructor for use outside the scrobbler package. * fix(play_tracker): account for playback rate in TTL and detect track switches The remainingTTL function now divides remaining time by the playback rate, so cache entries expire correctly at non-1x speeds (e.g., 2x playback halves the TTL). Zero/negative rates default to 1.0. The playing/paused case now checks if the cached MediaFile ID matches the reported mediaId, falling back to a DB fetch when the client switches tracks without sending stopped/starting. Adds parameterized tests for remainingTTL covering rate variations and edge cases. * fix(subsonic): validate positionMs and playbackRate in reportPlayback Reject negative positionMs values and invalid playbackRate values (NaN, Inf, zero, negative) at the API boundary before they reach TTL and position estimation math. Returns clear error messages for each case. * feat(play_tracker): add ClientId and ClientName to ReportPlayback parameters Signed-off-by: Deluan <deluan@navidrome.org> * refactor(play_tracker): replace NowPlaying method with ReportPlayback calls Signed-off-by: Deluan <deluan@navidrome.org> * refactor(play_tracker_test): remove redundant TTL behavior tests and clean up mockPluginLoader Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org>	2026-04-30 23:04:05 -04:00
Deluan Quintão	5c4f0298a6	fix(sharing): validate JWT expiration and share existence on stream endpoint (#5426) ... * fix(sharing): validate JWT expiration and share existence on stream endpoint The public stream endpoint (/public/s/{token}) was using TokenAuth.Decode() which only verifies the JWT signature but skips exp claim validation. This allowed expired share stream URLs to remain functional indefinitely. Additionally, deleting a share did not revoke previously issued stream tokens since the handler never performed a server-side share lookup. Fixed by switching decodeStreamInfo() to use auth.Validate() which properly checks the exp claim, and by embedding the share ID ("sid") in stream tokens so the handler can verify the share still exists. Old tokens without the sid claim remain backward compatible but still benefit from expiration validation. * fix(sharing): check share expiration on stream requests Replace the lightweight Exists() check with Get() + expiration validation, so that shares whose ExpiresAt was updated to an earlier time after token issuance are also rejected (410 Gone). Reuses the existing checkShareError handler for consistent error responses.	2026-04-27 19:36:57 -04:00
Deluan Quintão	259c1a9484	feat(subsonic): add sonicSimilarity extension as plugin capability (#5419) ... * feat(plugins): add sonicSimilarity capability types Defines the SonicSimilarity plugin capability interface with GetSonicSimilarTracks and FindSonicPath methods, along with their request/response types. * feat(sonic): add core sonic similarity service Implements the Sonic service with HasProvider, GetSonicSimilarTracks, and FindSonicPath, delegating to the PluginLoader and using the Matcher for index-preserving library resolution. * test(sonic): add sonic service unit tests Covers HasProvider, GetSonicSimilarTracks, and FindSonicPath with mock plugin loader and provider, verifying error propagation and successful match resolution via the library matcher. * feat(matcher): add MatchSongsToLibraryMap for index-preserving matching Adds a new method alongside MatchSongsToLibrary that returns a map[int]MediaFile keyed by input song index rather than a flat slice, enabling callers to correlate similarity scores back to the original position in the results. * fix(sonic): check provider availability before MediaFile lookup Avoids unnecessary DB call when no plugin is available, and ensures the correct error path is tested. * feat(plugins): add sonic similarity adapter Adds SonicSimilarityAdapter implementing sonic.Provider, bridging the plugin system to the core sonic service via Extism plugin functions. Reuses existing songRefsToAgentSongs helper for SongRef conversion. * feat(plugins): add LoadSonicSimilarity to plugin manager Adds Manager.LoadSonicSimilarity method following the pattern of LoadLyricsProvider, enabling the core sonic service to load a SonicSimilarityAdapter from a named plugin. * feat(subsonic): add sonicMatch response type Add SonicMatch struct with Entry and Similarity fields, and a SonicMatches slice to the Subsonic response struct. These types support the OpenSubsonic sonicSimilarity extension for returning similarity-scored track results. * feat(subsonic): add getSonicSimilarTracks and findSonicPath handlers Add two new Subsonic API handlers for the sonicSimilarity OpenSubsonic extension: GetSonicSimilarTracks returns similarity-scored tracks similar to a given song, and FindSonicPath returns a path of tracks connecting two songs. Both handlers delegate to the sonic core service and map results to SonicMatch response types. * feat(subsonic): advertise sonicSimilarity extension when plugin available Update GetOpenSubsonicExtensions to conditionally include the sonicSimilarity extension only when a sonic similarity plugin provider is available. The nil guard ensures backward compatibility with tests that pass nil for the sonic field. Also update the existing test to pass the new nil parameter. * feat(subsonic): wire sonic similarity service into router Add the sonic.Sonic service to the Router struct and New() constructor, register the getSonicSimilarTracks and findSonicPath routes, and wire sonic.New and its PluginLoader binding into the Wire dependency injection graph. Update all existing test call sites to pass the new nil parameter. Regenerate wire_gen.go. * fix(e2e): add sonic parameter to subsonic.New call in e2e tests * test(subsonic): add sonicSimilarity extension advertisement tests Restructures the GetOpenSubsonicExtensions test into two contexts: one verifying the baseline 5 extensions are returned when no sonic similarity plugin is configured, and one verifying that the sonicSimilarity extension is advertised (making 6 total) when a plugin loader reports an available provider. Adds a mockSonicPluginLoader to satisfy the sonic.PluginLoader interface without requiring a real plugin. * feat(subsonic): add nil guard and e2e tests for sonic similarity endpoints Handlers return ErrorDataNotFound when no sonic service is configured, preventing nil panics. E2e tests verify both endpoints return proper error responses when no plugin is available. * fix(subsonic): return HTTP 404 when no sonic similarity plugin available Endpoints are always registered but return 404 when no provider is available, rather than a subsonic error code 70. * refactor: clean up sonic similarity code after review Extract shared helpers to reduce duplication across the sonic similarity implementation: loadAllMatches in matcher consolidates the 4-phase matching pipeline, songRefToAgentSong eliminates per-iteration slice allocation in the adapter, sonicMatchResponse deduplicates response building in handlers, and a package-level constant replaces raw capability name strings in core/sonic. * fix empty response shapes Signed-off-by: Deluan <deluan@navidrome.org> * test(plugins): add testdata plugin and e2e tests for sonic similarity Add a test-sonic-similarity WASM plugin that implements both GetSonicSimilarTracks and FindSonicPath via the generated sonicsimilarity PDK. The plugin returns deterministic test data with decreasing similarity scores and supports error injection via config. Adapter tests verify the full round-trip through the WASM plugin including error handling. Also includes regenerated PDK code from make gen. * docs: update README to include new capabilities and usage examples for plugins Signed-off-by: Deluan <deluan@navidrome.org> * test(e2e): enhance sonic similarity tests with additional scenarios and mock provider Signed-off-by: Deluan <deluan@navidrome.org> * fix: address PR review feedback for sonic similarity Fix incorrect field names in README documentation ({from, to} → {startSong, endSong}) and remove unnecessary XML serialization test from e2e suite since OpenSubsonic endpoints only use JSON. * refactor: rename Matcher methods for conciseness Rename MatchSongsToLibrary to MatchSongs and MatchSongsToLibraryMap to MatchSongsIndexed. The Matcher receiver already establishes the "to library" context, making that suffix redundant, and "Indexed" better describes the intent (preserving input ordering) than "Map" which describes the data structure. * refactor: standardize variable naming for media files in sonic path methods Signed-off-by: Deluan <deluan@navidrome.org> * refactor: simplify plugin loading by introducing adapter constructors Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org>	2026-04-27 17:50:09 -04:00
Deluan Quintão	e6680c904b	fix(playlists): allow toggling auto-import and avoid unnecessary artwork reloads (#5421) ... * fix(playlists): allow toggling auto-import (sync) via REST API The updatePlaylistEntity handler was not applying the sync field from incoming requests, causing the auto-import toggle in the UI to have no effect. Apply the sync value for file-backed playlists only. * fix(playlists): enhance update logic for playlist metadata and sync toggle Signed-off-by: Deluan <deluan@navidrome.org> * fix(playlists): address code review feedback - Add pointer equality short-circuit in rulesEqual before reflect.DeepEqual - Guard against empty ID in Put's partial-update path - Only apply Sync when it actually differs from current value, preventing zero-value overwrites from partial payloads * fix(playlists): remove unused parameters from Update method Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org>	2026-04-27 12:20:27 -04:00
Deluan Quintão	5d1c1157b5	refactor(artwork): migrate readers to storage.MusicFS and add e2e suite (#5379) ... * test(artwork): add e2e suite documenting album/disc resolution Adds core/artwork/e2e/ with a real-tempdir + scanner harness that exercises artwork resolution end-to-end. Covers album and disc kinds; pending (PIt) cases document two known bugs in reader_album.go for regression-guard flipping once they are fixed. * refactor(artwork): add libraryFS helper to resolve MusicFS for a library * test(artwork): tighten libraryFS test isolation and add scheme-error case * test(artwork): update libraryFS test description to match implementation * refactor(artwork): convert fromExternalFile to use fs.FS Add a temporary fromExternalFileAbs shim so existing absolute-path callers still compile; the shim is removed once all readers are migrated. * refactor(artwork): make fromExternalFileAbs a thin delegator Introduce a minimal osDirectFS adapter so the shim no longer duplicates the matching loop. Both will be removed in Task 9. * refactor(artwork): convert fromTag to taglib.OpenStream over fs.FS Add a temporary fromTagAbs shim so existing absolute-path callers still compile; removed in Task 9. Reuses the osDirectFS adapter from Task 2. * refactor(artwork): defer fs.File close until after taglib reads finish Mirror the lifetime pattern used by adapters/gotaglib/gotaglib.go: keep the underlying fs.File open until taglib.File is closed, and pass WithFilename so format detection doesn't rely on content sniffing. * docs(artwork): note ffmpeg's path-based API limitation * refactor(artwork): migrate album reader to MusicFS - Add libFS (storage.MusicFS) field to albumArtworkReader; resolved once at construction time via libraryFS() - Switch fromCoverArtPriority from abs-path shims to FS-based fromTag/fromExternalFile; only fromFFmpegTag retains absolute path - Build imgFiles as library-relative forward-slash paths in loadAlbumFoldersPaths using path.Join(f.Path, f.Name, img) - Guard embedAbs so that an empty EmbedArtPath never produces a non-empty absolute path (prevents accidental ffmpeg invocation) - Register testfile:// storage scheme in artwork test suite to provide an os.DirFS-backed MusicFS without requiring the taglib extractor - Update test assertions from filepath.FromSlash(abs) to bare forward-slash relative strings * fix(artwork): use path package in compareImageFiles for forward-slash relative paths * refactor(artwork): migrate disc reader to MusicFS Replace os.Open absolute-path access with libFS.Open on library-relative forward-slash paths. Rename discFolders→discFoldersRel, split firstTrackPath into firstTrackRelPath (for fromTag) and firstTrackAbsPath (for fromFFmpegTag), and switch path.Dir/Base/Ext for forward-slash safety. * refactor(artwork): build discFoldersRel directly and guard empty first track * refactor(artwork): migrate mediafile reader to MusicFS * refactor(artwork): migrate artist album-art lookup to MusicFS * refactor(artwork): remove temporary path-based shims All readers now use the FS-based fromTag and fromExternalFile directly, so the absolute-path adapters and the osDirectFS helper that backed them can go away. * test(artwork): rewrite e2e suite to use storagetest.FakeFS Switches from real-tempdir + local storage to FakeFS via the storage registry. Adds a proper multi-disc scenario using the disc tag, which previously required curated MP3 fixtures we did not have. * test(artwork): use maps.Copy in trackFile tag merge Lint cleanup: replace the manual map-copy loop flagged by mapsloop. * test(artwork): reuse tests.MockFFmpeg in e2e harness Replace the hand-rolled noopFFmpeg stub with tests.NewMockFFmpeg, which already satisfies the full ffmpeg.FFmpeg interface and won't drift when new methods are added. Also tie imageBytes to imageFile so they cannot silently disagree on the on-disk encoding. * test(artwork): add e2e scenarios from artwork documentation Covers the behaviors documented at https://www.navidrome.org/docs/usage/library/artwork/: - Album: folder.*/front.* fallbacks and priority order with cover.*. - Disc: cd*.* match, cover.* inside disc folder, DiscArtPriority="" skip path, the documented multi-disc layout, and the discsubtitle keyword. - MediaFile: disc-level fallback for multi-disc tracks and album-level fallback for single-disc tracks (doc section "MediaFiles" items 2-3). - Artist: album/artist.* lookup via libFS (passes). The artist-folder branch is XIt-marked because fromArtistFolder still calls os.DirFS directly on an absolute path and can't read from a FakeFS-backed library — migrating that to storage.MusicFS is a follow-up. Signed-off-by: Deluan <deluan@navidrome.org> * refactor(artwork): scope artist folder traversal to library root Route fromArtistFolder reads through storage.MusicFS and bound the parent-directory walk at the library root. This keeps artwork resolution scoped to the configured library and unblocks FakeFS-backed e2e scenarios that depend on the artist folder. Also consolidate the libraryFS + core.AbsolutePath pairing (used by three readers) into a single libraryFSAndRoot helper. * test(artwork): add ASCII file-tree diagrams to e2e scenarios Each It/PIt block now shows the on-disk layout it exercises, with arrows indicating which file wins (or should win, for the known-bug PIt cases). Makes scenarios readable at a glance without having to parse the MapFS map. * test(artwork): add e2e tests for playlist and radio artwork resolution Signed-off-by: Deluan <deluan@navidrome.org> * test(artwork): enhance e2e tests with real MP3 fixtures for embedded artwork Signed-off-by: Deluan <deluan@navidrome.org> * test(ffmpeg): add support for animated WebP encoder detection and fallback handling Signed-off-by: Deluan <deluan@navidrome.org> * test(artwork): cover additional edge cases in e2e suite Add high-value scenarios uncovered by the existing specs: - Album: three-way basename tie (unsuffixed wins), unknown pattern in CoverArtPriority is skipped, embedded-first with no embedded art falls through. - Disc: discsubtitle with no matching image falls through. - Artist: ArtistArtPriority can reach images via album/<pattern>. - Playlist: generates a 2x2 tiled cover from album art when the playlist has no uploaded/sidecar/external image. New helper realPNG() produces real taglib/image-decodable bytes so the tiled-cover test can exercise the generator's decode + compose path. * test(artwork): refactor image upload logic in e2e tests for consistency Signed-off-by: Deluan <deluan@navidrome.org> * test(ffmpeg): simplify animated WebP encoder check by removing context parameter Signed-off-by: Deluan <deluan@navidrome.org> * fix(artwork): normalize rel path for fs.Glob on Windows filepath.Rel returns backslash-separated paths on Windows, but fs.Glob and path.Join require forward slashes. Convert with filepath.ToSlash after computing the relative path and use path.Dir for the parent walk so the artist-folder lookup works cross-platform. * fix(ffmpeg): retry animated WebP probe on transient failure The probe previously used the caller's request context inside sync.Once, so a single cancelled first request would permanently disable animated WebP for the rest of the process. Switch to a mutex + probed flag, use a fresh background context with its own timeout, and only cache the result when the probe actually succeeds. * test(ffmpeg): reset ffOnce so ConvertAnimatedImage test is order-independent The ConvertAnimatedImage stand-in test sets ffmpegPath directly but does not reset ffOnce. If ffmpegCmd() has not been called earlier in the test process, the next call inside hasAnimatedWebPEncoder runs ffOnce.Do and re-resolves the real ffmpeg binary, overwriting the stand-in and breaking the test. Reset ffOnce and conf.Server.FFmpegPath alongside the other globals to pin resolution to the stand-in. * test(artwork): unblock Windows CI — forward-slash fs paths and suite-level DB lifetime The internal artwork test planted a Windows absolute path (backslashes) into Folder.Path and then fed it through libFS.Open, which fs.ValidPath rejects. Rooting the testfile library at the temp dir directly and using filepath.ToSlash keeps the path model library-relative and forward-slash, matching production. The e2e suite opened a per-spec DB in a per-spec TempDir, but the go-sqlite3 singleton kept the file open across specs. Ginkgo's per-spec TempDir cleanup then tried to unlink a file still held by that handle — fine on POSIX, fails on Windows. Moving the DB to a suite-level tempdir and closing it in AfterSuite avoids the race. * test(artwork): keep Windows drive letters intact in testfile library URLs url.Parse on `testfile://C:/path` reads `C` as the host and the path loses the drive letter, so Windows libFS lookups go to `/path` and fail. testFileLibPath now prepends a `/` when the OS path has no leading slash, and the testfile constructor strips that extra slash back off before handing the path to os.Stat / os.DirFS. * refactor(artwork): consolidate libFS + root into libraryView helper Collapses the per-reader libFS/libPath/rootFolder/firstTrackAbsPath fields into a single libraryView{FS, absRoot} with an Abs(rel) method. Also folds the two library lookups (ds.Library.Get + core.AbsolutePath) into one, and uses mf.Path directly instead of stripping libRoot off an absolute path. * refactor(ffmpeg): replace hasAnimatedWebPEncoder with encoderProbe for state management Signed-off-by: Deluan <deluan@navidrome.org> * fix: escape artist folder names in artwork glob Escape glob metacharacters in the library-relative artist folder path before composing the fs.Glob pattern for artist image lookup. This preserves literal folder names such as Artist [Live] while keeping the configured filename pattern behavior unchanged, and adds a regression test for bracketed artist folders. Signed-off-by: Deluan <deluan@navidrome.org> * fix(artwork): correct test path assertions after MusicFS migration Source functions (fromTag, fromExternalFile) now return forward-slash fs.FS-relative paths, so test assertions should compare against plain forward-slash strings, not filepath.FromSlash(). The artistArtPriority test needs filepath.FromSlash() on the suffix because findImageInFolder returns OS-native absolute paths via filepath.Join. * fix(artwork): normalize path separators in artistArtPriority assertion The two table entries exercise different code paths: entry 1 goes through fromArtistFolder (returns OS-native paths via filepath.Join), while entry 2 goes through fromExternalFile (returns forward-slash fs.FS paths). Using filepath.FromSlash on the expected value only works for entry 1. Normalize the actual path to forward slashes with filepath.ToSlash so a single HaveSuffix assertion works for both code paths on all platforms. --------- Signed-off-by: Deluan <deluan@navidrome.org>	2026-04-26 18:16:14 -04:00
Deluan Quintão	a756cad1dc	test: enable artwork tests on Windows (#5416) ... * fix(test): enable artwork tests on Windows by using OS-aware path assertions Replace hardcoded forward-slash path expectations with filepath.FromSlash() so assertions match OS-native separators on Windows. Removes all 8 SkipOnWindows("#TBD-path-sep-artwork") guards from artwork unit tests. * test: add comment explaining forward-slash paths in test fixtures	2026-04-26 17:34:39 -04:00
Deluan Quintão	5d1c9530ab	feat(cli): add pls export/import subcommands for bulk playlist management (#5412) ... * refactor: rename ImportFile to ImportFromFolder in playlists service * feat: add ImportFile method with library/folder resolution * feat: allow sync flag upgrade on re-import of non-synced playlists * feat: add pls export subcommand with bulk and single export Add `navidrome pls export` command that supports: - Single playlist export to stdout (-p flag only) - Single playlist export to directory (-p and -o flags) - Bulk export of all playlists to a directory (-o flag only) - Filtering by user (-u flag) - Automatic filename sanitization and collision detection Also extracts findPlaylist helper from runExporter for reuse. * feat: add pls import subcommand with sync flag support * fix: improve error message for export without output directory * test: add tests for ImportFile sync flag and sync upgrade behavior * refactor: streamline export and import logic by removing redundant comments and improving library path matching Signed-off-by: Deluan <deluan@navidrome.org> * feat: update ImportFile method to include sync flag for playlist imports Signed-off-by: Deluan <deluan@navidrome.org> * feat: implement fetchPlaylists function to streamline playlist retrieval Signed-off-by: Deluan <deluan@navidrome.org> * feat: replace inline filename sanitization with centralized utility function Signed-off-by: Deluan <deluan@navidrome.org> * feat: refactor playlist import logic to consolidate sync handling and improve method signatures Signed-off-by: Deluan <deluan@navidrome.org> * fix: address code review feedback on playlist import/export - Fix duplicate playlist creation on non-sync re-import: only reconcile sync flag when the playlist was actually persisted (has an ID) - Distinguish "not in any library" from real errors in resolveFolder using a sentinel error, so DB/folder errors propagate instead of falling back to ImportM3U - Use bufio.Scanner in countM3UTrackLines instead of reading entire file * feat: replace bufio.Scanner with UTF8Reader and LinesFrom utility for improved file reading Signed-off-by: Deluan <deluan@navidrome.org> * fix: record path for outside-library imports to prevent duplicates Files outside all libraries now go through updatePlaylist with the absolute path recorded, so re-importing the same file updates the existing playlist instead of creating a duplicate. * refactor: name guard condition in updatePlaylist for readability Extracted the compound boolean expression into a named local variable `alreadyImportedAndNotSynced` to make the intent of the early-return guard clearer at a glance. * add godocs Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org>	2026-04-25 20:54:02 -04:00
Deluan Quintão	ca09070a6c	feat(smartplaylists): relax playlist visibility in inPlaylist/notInPlaylist rules (#5411) ... * test(e2e): add end-to-end tests for smart playlists functionality Signed-off-by: Deluan <deluan@navidrome.org> * fix: enforce playlist visibility in smart playlist InPlaylist/NotInPlaylist rules Previously, the InPlaylist/NotInPlaylist smart playlist criteria only allowed referencing public playlists, regardless of who owned the smart playlist. This was too restrictive for owners referencing their own private playlists and for admins who should have unrestricted access. The fix passes the smart playlist owner's identity and admin status into the criteria SQL builder, so that: admins can reference any playlist, regular users can reference public playlists plus their own private ones, and inaccessible referenced playlists produce a warning instead of a hard error. Also prevents recursive refresh of child playlists the owner cannot access. * test(e2e): clarify user roles and fix playlist visibility tests Renamed testUser/otherUser to adminUser/regularUser to make the admin vs regular user distinction explicit in test code. Fixed three playlist visibility tests that were evaluating as admin (bypassing all access checks) instead of as a regular user, so the public playlist path is now actually exercised. All playlist operator tests now use explicit evaluateRuleAs calls with the appropriate user role. * fix: sync rulesSQL criteria after limitPercent resolution The rulesSQL struct captures a copy of rules at creation time. When limitPercent is resolved later, rules.Limit is updated but rulesSQL still holds the stale value. This caused percentage-based smart playlist limits to be silently ignored. Fix by updating rulesSQL.criteria after the resolution. * refactor: convert inList to a method on smartPlaylistCriteria The inList function already receives ownerID and ownerIsAdmin from the smartPlaylistCriteria caller. Making it a method lets it access those fields directly from the receiver, simplifying the signature and staying consistent with exprSQL which was already converted to a method. * refactor: simplify function signatures by removing type parameters in criteria_sql.go Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org>	2026-04-25 14:59:06 -04:00
Deluan Quintão	3b3b9a62ca	test(smartplaylists): add smart playlist e2e test suite (#5409) ... * test: add smart playlist e2e suite infrastructure * test: add string field smart playlist e2e tests * test: add numeric, boolean, tag, participant, annotation, logic, sorting smart playlist e2e tests * test: add playlist operator smart playlist e2e tests * test: add isNot and endsWith string field e2e tests * test: add date/time field smart playlist e2e tests * fix: add gosec nolint directives for safe SQL concatenation in e2e restore * refactor: address code review feedback for smart playlist e2e tests - Deduplicate evaluateRule by delegating to evaluateRuleOrdered - Cache table list in BeforeSuite instead of querying sqlite_master per test - Wrap restoreDB in a transaction with defer cleanup for DETACH/foreign_keys - Use JSON numbers for numeric criteria values to match canonical JSON shape * refactor: simplify e2e test infrastructure - Remove unused return value from buildTestFS - Add deferred ROLLBACK as safety net in restoreDB transaction - Cache Come Together ID to avoid repeated lookups in BeforeSuite - Use range-over-int for play count loop * test: add missing operator coverage to smart playlist e2e tests Add 4 tests for operators/paths that had no e2e coverage: - notContains on string fields (LIKE negation path) - before on date fields (Lt for dates, only after was tested) - startsWith on tag fields (json_tree + LIKE subquery) - endsWith on participant/role fields (json_tree + LIKE subquery)	2026-04-24 23:03:10 -04:00
Deluan Quintão	7e083e0795	fix: split html sanitization from plaintext handling (#5403) ... * fix: split html sanitization from plaintext handling Add a dedicated SanitizeHTML helper for HTML-rendered values so entity-encoded markup is decoded before bluemonday sanitization. Use the new helper for the login welcome message and artist biographies while preserving SanitizeText semantics for lyrics and other plaintext callers. Add regression coverage for both helpers and the serveIndex welcomeMessage path. * docs: add SanitizeText and SanitizeHTML godoc Signed-off-by: Deluan <deluan@navidrome.org> * fix: preserve plain text in artist biographies Revert artist biography storage to SanitizeText so entity-encoded plain text remains decoded for Subsonic consumers. This avoids double-escaping values like R&B in XML responses while keeping the new welcomeMessage HTML sanitization in place, and adds a regression test covering the biography storage behavior. --------- Signed-off-by: Deluan <deluan@navidrome.org>	2026-04-23 17:53:28 -04:00
Deluan Quintão	64c8d3f4c5	ci: run Go tests on Windows (#5380) ... * ci(windows): add skeleton go-windows job (compile-only smoke test) * ci(windows): fix comment to reference Task 7 not Task 6 * ci(windows): harden PATH visibility and set explicit bash shell * ci(windows): enable full go test suite and ndpgen check * test(gotaglib): skip Unix-only permission tests on Windows * test(lyrics): skip Windows-incompatible tests * test(utils): skip Windows-incompatible tests * test(mpv): skip Windows-incompatible playback tests Skip 3 subprocess-execution tests that rely on Unix-style mpv invocation; .bat output includes \r-terminated lines that break argument parsing (#TBD-mpv-windows). * test(storage): skip Windows-incompatible tests Skip relative-path test where filepath.Join uses backslash but the storage implementation returns a forward-slash URL path (#TBD-path-sep-storage). * test(storage/local): skip Windows-incompatible tests Skip 13 tests that fail because url.Parse("file://" + windowsPath) treats the drive letter colon as an invalid port; also skip the Windows drive-letter path test that exposes a backslash vs forward-slash normalisation bug (#TBD-path-sep-storage-local). * test(playlists): skip Windows-incompatible tests * test(model): skip Windows-incompatible tests * test(model/metadata): skip Windows-incompatible tests * test(core): skip Windows-incompatible tests AbsolutePath uses filepath.Join which produces OS-native path separators; skip the assertion test on Windows until the production code is fixed (#TBD-path-sep-core). * test(artwork): skip Windows-incompatible tests Artwork readers produce OS-native path separators on Windows while tests assert forward-slash paths; skip 11 affected tests pending a fix in production code (#TBD-path-sep-artwork). * test(persistence): skip Windows-incompatible tests Skip flaky timestamp comparison (#TBD-flake-persistence) and path-separator real-bugs (#TBD-path-sep-persistence) in FolderRepository.GetFolderUpdateInfo which uses filepath.Clean/os.PathSeparator converting stored forward-slash paths to backslashes on Windows. * test(scanner): skip Windows-incompatible tests Skip symlink tests (Unix-assumption), ndignore path-separator bugs (#TBD-path-sep-scanner) in processLibraryEvents/resolveFolderPath where filepath.Rel/filepath.Split return backslash paths incompatible with fs.FS forward-slash expectations, error message mismatch on Windows, and file format upgrade detection (#TBD-path-sep-scanner). * test(plugins): skip Windows-incompatible tests Add //go:build !windows tags to test files that reference the suite bootstrap (testManager, testdataDir, createTestManager) which is only compiled on non-Windows. Add a Windows-only suite stub that skips all specs via BeforeEach to prevent [build failed] on Windows CI. * test(server): skip Windows-incompatible tests Skip createUnixSocketFile tests that rely on Unix file permission bits (chmod/fchmod) which are not supported on Windows. * test(nativeapi): skip Windows-incompatible tests Skip the i18n JSON validation test that uses filepath.Join to build embedded-FS paths; filepath.Join produces backslashes on Windows which breaks fs.Open (embedded FS always uses forward slashes). * test(e2e): skip Windows-incompatible tests On Windows, SQLite holds file locks that prevent the Ginkgo TempDir DeferCleanup from deleting the DB file. Register an explicit db.Close DeferCleanup (LIFO before TempDir cleanup) on Windows so the file lock is released before the temp directory is removed. * test(windows): fix e2e AfterSuite and skip remaining scanner path test * test(scanner): skip another Windows path-sep test (#TBD-path-sep-scanner) * test(subsonic): skip timing-flaky test on Windows (#TBD-flake-time-resolution-subsonic) * test(scanner): skip 'detects file moved to different folder' on Windows * test(scanner): consolidate 'Library changes' Windows skips into BeforeEach * test(scanner): close DB before TempDir cleanup to fix Windows file lock * test(scanner): skip ScanFolders suite on Windows instead of closing shared DB * ci: retrigger for Windows soak run 2/3 * ci: retrigger for Windows soak run 3/3 * ci: retrigger for Windows soak run 3/3 (take 2) * test(scanner): skip Multi-Library suite on Windows (SQLite file lock) * ci(windows): promote go-windows to blocking status check * test(plugins): run platform-neutral specs on Windows, drop blanket Skip * test(windows): make tests cross-platform instead of skipping - subsonic: back-date submissionTime baseline by 1s so BeTemporally(">") passes under millisecond clock resolution - persistence: sleep briefly between Put calls so UpdatedAt is strictly after CreatedAt on low-resolution clocks - utils/files: close tempFile before os.Remove so the test works on Windows (where an open handle holds a file lock) - tests.TempFile: close the handle before returning; metadata tests no longer leak the open file into Ginkgo's TempDir cleanup Resolves Copilot review comments on #5380. * test(tests): add SkipOnWindows helper to reduce boilerplate Introduces tests.SkipOnWindows(reason) that wraps the 3-line runtime.GOOS guard pattern used in every Windows-skipped spec. * test(adapters): use tests.SkipOnWindows helper * test(core): use tests.SkipOnWindows helper * test(model): use tests.SkipOnWindows helper * test(persistence): use tests.SkipOnWindows helper * test(scanner): use tests.SkipOnWindows helper * test(server): use tests.SkipOnWindows helper * test(plugins): run pure-Go unit tests on Windows config_validation_test, manager_loader_test, and migrate_test have no WASM/exec dependencies and don't rely on the make-built test plugins from plugins_suite_test.go. Let them run on Windows too.	2026-04-19 13:16:47 -04:00
Deluan Quintão	3b7d3f4383	feat(matcher): add Matcher.PreferStarred option to bias fuzzy matcher toward starred/high-rated tracks (#5387) ... * matcher: update godoc for matcher config scoring order * conf: log deprecated SimilarSongsMatchThreshold option * conf: enable matcher prefer-starred by default	2026-04-19 12:54:41 -04:00
bobo-xxx	28eba567a7	fix(artwork): return correct timestamp when disc or album coverart changes (#5378) ... * fix(artwork): return imagesUpdatedAt in LastUpdated when cover art changes When cover art (cover.jpg) is updated in an album folder, the HTTP Last-Modified header was incorrectly returning album.UpdatedAt (which only tracks media file changes) instead of imagesUpdatedAt (which tracks cover art changes). This caused browsers to use their cached cover art because the Last-Modified header didn't change, even though the actual cover art image data was new (due to cache key changing based on imagesUpdatedAt). The fix ensures LastUpdated() returns a.lastUpdate (which is the max of album.UpdatedAt and imagesUpdatedAt) instead of always returning album.UpdatedAt. Fixes navidrome/navidrome#5377 * refactor tests Signed-off-by: Deluan <deluan@navidrome.org> * fix(artwork): return imagesUpdatedAt in disc LastUpdated The discArtworkReader had the same bug as albumArtworkReader (fixed in 9a741859f): LastUpdated() returned album.UpdatedAt while Key() used the max of album.UpdatedAt and ImagesUpdatedAt. This mismatch caused browsers to keep stale disc cover art in cache when only the image file changed. Also strengthen the album LastUpdated tests and add matching tests for the disc reader. The tests use DescribeTable and were verified to fail when the fix is reverted. --------- Signed-off-by: Deluan <deluan@navidrome.org> Co-authored-by: Deluan <deluan@navidrome.org>	2026-04-17 21:35:33 -04:00
Deluan Quintão	0a6b5519cc	refactor(scanner): remove C++ taglib adapter (#5349) ... * refactor(build): remove CPP taglib adapter Remove the CGO-based TagLib adapter (adapters/taglib/) and all cross-taglib build infrastructure. The WASM-based go-taglib adapter (adapters/gotaglib/) is now the sole metadata extractor. - Delete adapters/taglib/ (CPP/CGO wrapper) - Delete .github/actions/download-taglib/ - Remove CROSS_TAGLIB_VERSION, CGO_CFLAGS_ALLOW, and all taglib-related references from Dockerfile, Makefile, CI pipeline, and devcontainer * fix(scanner): gracefully fallback to default extractor instead of crashing Replace log.Fatal with a graceful fallback when the configured scanner extractor is not found. Instead of terminating the process, the code now warns and falls back to the default taglib extractor using the existing consts.DefaultScannerExtractor constant. A fatal log is retained only for the case where the default extractor itself is not registered, which indicates a broken build. * test(scanner): cover default extractor fallback and suppress redundant warn Address review feedback on the extractor fallback in newLocalStorage: - Only log the "using default" warning when the configured extractor differs from the default, so a broken build (default extractor itself missing) logs only the fatal — not a misleading "falling back" warn followed immediately by the fatal. - Add a unit test that registers a mock under consts.DefaultScannerExtractor, sets the configured extractor to an unknown name, and asserts the local storage is constructed using the default extractor's constructor.	2026-04-12 21:52:29 -04:00
Deluan Quintão	52e47b896a	refactor: extract song-to-library matcher to core/matcher package (#5348) ... * refactor: extract matchSongsToLibrary to core/matcher package Move the song-to-library matching algorithm from core/external into its own core/matcher package. The Matcher struct exposes a single public method MatchSongsToLibrary that implements a multi-phase matching algorithm (ID > MBID > ISRC > fuzzy title+artist). Includes pre-sanitization optimization for the fuzzy matching loop. No behavioral changes — the algorithm is identical to the version in core/external/provider_matching.go. * refactor: inject matcher.Matcher via Wire instead of creating it inline Add *matcher.Matcher as a dependency of external.NewProvider, wired via Google Wire. Update all provider test files to pass matcher.New(ds). This eliminates tight coupling so future consumers can reuse the matcher without depending on the external package. * refactor: remove old provider_matching files Delete core/external/provider_matching.go and its tests. All matching logic now lives in core/matcher/. * test(matcher): restore test coverage lost in extraction Port back 23 specs that existed in the old provider_matching_test.go but were dropped during the extraction. Covers specificity levels, fuzzy matching thresholds, fuzzy album matching, duration matching, and deduplication edge cases. * test(matcher): extract matchFieldInAnd/matchFieldInEq helpers The four inline mock.MatchedBy closures in setupAllPhaseExpectations all followed the same squirrel.And -> squirrel.Eq -> field-name-check pattern. Extract into two small helpers to reduce duplication and make the setup functions read as a concise list of phase expectations. * refactor(matcher): address PR #5348 review feedback - sanitizedTrack now holds *model.MediaFile instead of a value copy. Since MediaFile is a large struct (~74 fields), this avoids the per-track copy into sanitized[] and a second copy when findBestMatch assigns the winner. loadTracksByTitleAndArtist updated to iterate by index and pass &tracks[i]. - loadTracksByISRC now sorts results (starred desc, rating desc, year asc, compilation asc) so that when multiple library tracks share an ISRC the most relevant one is picked deterministically, matching the sort order already used by loadTracksByTitleAndArtist. - Restored the four worked examples (MBID Priority, ISRC Priority, Specificity Ranking, Fuzzy Title Matching) in the MatchSongsToLibrary godoc that were dropped during the extraction. - matcher_test.go: tests now enforce expectations via AssertExpectations in a DeferCleanup. The old setupAllPhaseExpectations helper was replaced with per-phase helpers (expectIDPhase/expectMBIDPhase/expectISRCPhase + allowOtherPhases) so each test deterministically verifies which matching phases fire. This surfaced (and fixes) a latent issue copilot flagged: the old .Once() expectations were not actually asserted, so tests would silently pass even when phases short-circuited unexpectedly.	2026-04-12 16:47:22 -04:00
Deluan	501c6eaf8f	refactor(ffmpeg): consolidate dynamic audio flag injection into a single function ... Signed-off-by: Deluan <deluan@navidrome.org>	2026-04-11 23:23:04 -04:00