mirror of
https://github.com/navidrome/navidrome.git
synced 2026-08-01 07:21:17 +00:00
600 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Kendall Garner
|
4f29465d27
|
fix(artwork): do not promote artist folder to album folder (#5856)
* fix(artwork): do not promote artist folder to album folder * use filesystem join for path instead * fix(artwork): resolve artist folder for albums with disc subfolders Dropping the promoted album root from the artist reader's paths fixed the flat-album case but broke albums whose tracks live in disc subfolders: the promoted root was what kept the byte-wise longest common prefix on a directory boundary. Without it, CD1/CD2 siblings share the fragment "Album/CD", so the artist folder resolved one level too deep and album/artist.* won over artist.*. loadAlbumFoldersPaths now collapses each album to its own root before the paths are compared across albums, so a disc-split album contributes its album folder rather than each disc folder. Folders no album claims (the promoted parent) are still returned unchanged, keeping the album, disc and mediafile readers unaffected. The prefix math moves into commonDir, which appends a trailing separator so the comparison lands on segment boundaries - this also fixes sibling folders sharing a name prefix (Album/Album2). Adds e2e coverage for the case the branch fixes (artist.* with no album/ fallback, which failed before this branch), the disc-subfolder regression, and a multi-album guard that pins the scope boundary. * test(artwork): cover artist whose albums are all disc-split The existing specs cover a single disc-split album and a disc-split album alongside a flat one, but not an artist where every album is split into disc subfolders. That layout resolves correctly because the albums diverge one level above the disc folders, which re-anchors the common prefix - a property worth pinning so a future change to the path math can't silently break it. * fix(artwork): resolve artist folder when albums share a folder loadArtistFolder decided whether to climb above the common directory by comparing len(paths) against 2. That count stands in for "how many distinct album roots are there", and the two diverge when an artist has two albums in the same folder: the slice holds two identical entries, the climb is skipped, and the artist folder resolves to the album folder. Deduplicate the roots so the branch tests the property it means to test. Replace the includeParent flag with two functions split by audience. The flag was a caller-identity switch - constant true at the album, disc and mediafile readers, all of which discard the returned paths, and constant false at the only caller that reads them. The artist path now has loadArtistAlbumRoots, which collapses each album to its own root and never consults albumRootParent; loadAlbumFoldersPaths goes back to taking a single album and always promoting the parent. Both share the folder load and image aggregation. This also makes albumRootParent's contract structural. Its guard only rejects an artist folder when it can find audio outside the album being resolved, so passing every album of an artist at once made the check meaningless; taking a single album means it can no longer be called that way. Drops the per-album grouping and unclaimed-folder reconciliation from the album path, where the result was discarded, and removes 15 mechanical true arguments from the tests. --------- Co-authored-by: Deluan <deluan@navidrome.org> |
||
|
Deluan Quintão
|
23548f40a0
|
fix(share): give visual feedback when downloading from a share (#5865)
* fix(share): give visual feedback when downloading a share The share page handed the download URL to navidrome-music-player, which fell through to downloadjs and buffered the whole ZIP into memory via XHR before saving it. Nothing was handed to the browser until the last byte arrived, so a large share produced a long silent window with no player feedback and no browser download UI, inviting repeat clicks that each spawn another server-side zip+transcode. Use the player's customDownloader prop to trigger a synthetic anchor instead, so the browser performs the download and reports its own progress. An anchor rather than assigning window.location.href: the share page's service worker registers a NavigationRoute over all navigations, which intercepts the streamed archive and fails it into the offline fallback (observed as HTTP 503 in Chrome). handleDownloads now loads the share before streaming so it can set Content-Disposition and Content-Type. This also fixes error reporting: ZipShare previously wrote to the ResponseWriter before checkShareError ran, locking the status at 200, so expired, missing and non-downloadable shares all returned 200. They now correctly return 410, 404 and 403. * feat(share): acknowledge the download click in the player The browser's download UI is the real progress indicator, but nothing in the page itself reacted to the click, so the moment before the browser catches up still read as unresponsive. Dim the download button and make it unclickable for two seconds after a download starts, reusing the JSS function-value pattern the existing single-track styling already uses. A repeat download restarts the window instead of extending the original, and the timer is cleared on unmount. This also blunts repeat clicking, where every extra click costs another server-side zip and transcode. Add SharePlayer tests covering the download mechanism and this state machine. The dimming itself is verified in a browser rather than jsdom: JSS function values are not evaluated there, so the rule is never emitted and a CSS assertion would pass or fail for the wrong reason. Signed-off-by: Deluan <deluan@navidrome.org> * test(share): assert render counts in SharePlayer feedback tests The two acknowledgement tests compared the props object across renders, which React may reuse, so they passed without proving anything and then failed once the surrounding assertions changed. Count renders instead, and let the pending timer run out rather than advancing exactly to its deadline, which does not cross it. The repeat-download test now also asserts that no render happens at the original deadline, proving the timer was replaced rather than merely that one eventually fired. * fix(share): count one visit per share download The preflight share load added in this branch made every download record two visits: handleDownloads called Share.Load, and ZipShare then loaded the share again internally. Share.Load increments and persists VisitCount, so the counter advanced twice per download and the repository work was duplicated. Pass the already-loaded share into ZipShare instead of its id. handleDownloads is its only production caller, and it now has the share in hand for the Content-Disposition header anyway. The archiver test asserts Load is not called, so the double-load cannot come back unnoticed. Verified against a running server: the counter now advances by one per download. * test(share): derive feedback-window timings from the constant The acknowledgement tests hardcoded clock advances tuned to a 2000ms window. Raising DOWNLOAD_FEEDBACK_MS to 5000 left them advancing 1500ms and 1001ms, which no longer reach the deadline they are meant to cross, so the repeat- download test passed without proving the timer had been replaced. Export the constant and derive the advances from it, and let the pending timer run out in the unmount test rather than advancing a fixed amount. Changing the duration can no longer silently strand a test short of its deadline. --------- Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
Deluan Quintão
|
fed9665060
|
fix(streaming): surface why a transcode decision failed (#5820)
* fix(subsonic): surface the reason a transcode decision failed
getTranscodeDecision returned a bare "failed to make transcode decision"
with no clue why, and the probe command ran ffprobe with -v quiet, so even
the server log bottomed out at "exit status 1". A user whose files had been
moved by an external tool only saw the opaque error.
ProbeAudioStream now returns a typed ProbeError that separates the file path
from the reason: ffprobe runs with -v error so its stderr diagnostic is
captured, and a missing or unreadable file is reported as "file not found"
rather than ffprobe's misleading "Invalid data found". The handler logs the
full detail (including the path) and returns the reason to the client with
the server path stripped out.
Reported-by: Tolriq (Symfonium)
* refactor(ffmpeg): use errors.AsType for ExitError match
probeErrorReason used the older var+errors.As form while the rest of the
codebase (and its sibling transcodeFailureReason) uses the generic
errors.AsType. Switch to it for consistency; behavior is unchanged.
* fix(subsonic): return error 70 when the source file is missing
A getTranscodeDecision probe failure was always reported as generic error 0.
When the source file is gone (moved or deleted out from under the DB), that is
a not-found condition, so return the standard Subsonic error 70 ("data not
found") instead — matching what the endpoint already returns for an unknown
mediaId. Files that exist but are corrupt or unreadable stay error 0.
ProbeError now wraps the underlying cause and implements Unwrap, so the handler
detects the case with errors.Is(err, fs.ErrNotExist).
* fix(ffmpeg): keep probe error paths out of client-facing reasons
Addresses review feedback on the ProbeError type: the Reason field doubled as
both the log detail and the client message, so an ffprobe launch failure (a
*os.PathError from fork/exec) could leak the ffprobe binary path to clients,
and an unexpected stat error was reduced to "file not accessible" in the log.
Split the two concerns: Reason now holds only a path-free, client-safe string
(built at construction), while Error() logs the full underlying cause. Launch
failures return a generic "could not read file" instead of the raw exec error.
SafeReason no longer does substring path-stripping (removing the empty-Path
edge case); the stripping happens once, against ffprobe's stderr.
* fix(subsonic): don't report a broken ffprobe as a missing media file
Two issues from review of the previous commit:
Code 70 was selected with errors.Is(err, fs.ErrNotExist), but a launch failure
of a deleted ffprobe binary is an *os.PathError that also wraps fs.ErrNotExist.
A server-side ffprobe problem was therefore reported to clients as a missing
media file. ProbeError now carries an explicit NotFound flag, set only on the
file-access branch, and the handler keys the code off that instead of the chain.
ffprobe can also exit 0 while yielding no audio stream (an audio-suffixed
container holding only video). That parse failure was returned unwrapped, so
clients got "internal error"; it is now wrapped in a ProbeError too.
|
||
|
Deluan Quintão
|
e6597398c2
|
feat(scrobbler): exponential backoff for scrobble retries during outages (#5818)
* feat(scrobbler): add exponential backoff delay helper * feat(scrobbler): back off retries up to 4m during outages * test(scrobbler): verify backoff schedule with synctest; clarify backoffDelay doc Adds a testing/synctest-based test that drives the real run loop against a failing service and asserts the exact 5s/10s/20s/40s retry schedule and the drain-on-recovery reset, addressing the review note that the run loop's behavior was untested. Also clarifies the backoffDelay doc comment: the argument is a zero-based retry index. |
||
|
Deluan Quintão
|
85132240e0
|
feat(smartplaylist): per-playlist refreshDelay for stable daily/weekly playlists (#5790)
* feat(utils): add ParseDuration/FormatDuration with day and week units * feat(criteria): add per-playlist refreshDelay to smart playlist rules * feat(smartplaylist): honor per-playlist refreshDelay in refresh gate * feat(subsonic): compute smart playlist validUntil from effective refresh delay * fix(playlists): reset smart playlist evaluation window when rules change via API * refactor(utils): flatten FormatDuration recursion, single-pass duration regex * fix(playlists): address PR review feedback - Reset EvaluatedAt to nil instead of zero-time on rules change and NSP re-import, so getPlaylist(s) never reports year-1 Changed/validUntil in the window between an edit and the next owner read - Parse negative d/w durations so they are rejected with the consistent "negative duration" error instead of "unknown unit" - Quote input in the negative-duration error, matching the parse error - Gate per-playlist RefreshDelay behind IsSmartPlaylist, matching its doc |
||
|
Deluan Quintão
|
27f0210392
|
fix(scrobbler): tolerate out-of-order playback reports (#5793)
* fix(scrobbler): tolerate out-of-order playback reports Clients may fire reportPlayback requests concurrently (Feishin sends 'starting' and 'playing' in parallel, and the previous track's 'stopped' races the next track's start), so reports can be processed out of order. Two cases corrupted the now-playing session: a late 'starting' for the track already playing downgraded the session state, freezing position estimation at 0:00 until the next report; and a late 'stopped' for the previous track removed the new track's session and dispatched a playback report mislabeled with the new track's metadata, causing presence-style plugins (e.g. Discord Rich Presence) to clear or show stale state. ReportPlayback now ignores a 'starting' report when the session already has the same track in playing state, and ignores a 'stopped' report for a track other than the current session's - skipping both the session removal and the plugin dispatch, while still counting the play and dispatching external scrobbles for the stopped track. Reported in https://github.com/jeffvli/feishin/issues/2131 * fix(scrobbler): serialize session writes to close starting/playing race The out-of-order 'starting' guard checked the session cache before the media-file load, leaving a window where a concurrent 'playing' report on a fresh session could write between the check and the write, and still be overwritten back to 'starting'. Session check-then-write sections are now serialized by a mutex, with the guard re-checked after the load. Also tightens the guard comment to say 'playing session', matching the condition. Found by Codex review on #5793. * fix(scrobbler): fully exit ReportPlayback when ignoring out-of-order reports The out-of-order guards used 'break', which only exits the switch, so the post-switch NowPlaying block still ran for an ignored 'starting' report and enqueued a NowPlaying dispatch with the stale report's position - potentially overwriting a pending correct-position entry, since the queue is keyed by client. Return nil instead, so ignored reports have no side effects. Found by Gemini review on #5793. |
||
|
Deluan Quintão
|
09022b4bd2
|
feat(jellyfin): AudioMuse-AI compatible sonic endpoints (#5782)
* refactor(jellyfin): inject core/sonic into the Jellyfin Router
* feat(jellyfin): add AudioMuse /info endpoint
* feat(jellyfin): add AudioMuse /similar_tracks endpoint
* feat(jellyfin): gate AudioMuse endpoints on sonic provider
* feat(jellyfin): add AudioMuse /find_path endpoint
* fix(jellyfin): fix case-insensitive route collision across positions
canonicalRouteSegments keyed canonical case by lower-cased segment name
alone, globally. Two unrelated routes sharing a segment name with
different casing at different tree depths (e.g. "Info" in
/System/Info/Public vs "info" in /AudioMuseAI/info) silently overwrote
each other, 404-ing the loser even for exact-case requests. Replace the
flat map with a position-aware trie mirroring the routing tree.
* test(jellyfin): e2e tests for AudioMuse endpoints
* docs(jellyfin): document AudioMuse compatibility endpoints
* test(jellyfin): harden AudioMuse tests and doc note (final-review follow-ups)
- Comment-lock the []string{} (not nil) contract for /AudioMuseAI/info's
AvailableEndpoints so it keeps serializing as [] rather than null, and
add a raw-body assertion to the existing empty-list test to catch a
regression a struct-only unmarshal can't detect.
- Cover the previously-untested engine-error branch in similar_tracks and
find_path, both of which degrade to an empty result.
- Document that find_path's path/total_distance only reflect hops through
libraries the caller can access in multi-library setups.
* refactor(jellyfin): dedup AudioMuse test request helper, presize dedup map
* refactor(sonic): expose sonic.Engine interface; drop typed-nil guard in jellyfin.New
The Jellyfin Router's sonic field was an interface but New() took the concrete
*sonic.Sonic, so a nil arg became a non-nil typed-nil and needed a guard — the
only injected dependency that did. Move the interface (sonic.Engine) beside its
implementation, take it in New() like every other service, and bind it in wire.
* refactor(jellyfin): case-insensitive routing via lowercased paths
Replace the position-aware route trie with a trivial middleware that lowercases
the request path, and register every route in lowercase. Simpler, and no segment
name can collide across positions. caseInsensitivePaths moves into middlewares.go
alongside normalizeQueryKeys. Relies on the invariant that no Jellyfin path segment
carries case-sensitive data (all ids are lowercase hex via dto.EncodeID).
* feat(jellyfin): add AudioMuse /health endpoint
A liveness probe matching the reference plugin: 200 with an empty body when a
SonicSimilarity provider is loaded, 404 otherwise. /AudioMuseAI/info now
advertises it (list alphabetized like the plugin's OrderBy).
Also trims the AudioMuse and case-insensitive-routing comments to their essential
rationale.
* fix(jellyfin): hex-encode user IDs so lowercased paths stay valid
Address PR review: user IDs were the one id the Jellyfin API emitted raw (base62,
uppercase-capable), so lowercasing request paths could alter a userId segment. Encode
them via dto.EncodeID like every other id, making the 'all boundary ids are lowercase
hex' invariant true — no routing special-casing needed. Also caps user-controlled n /
max_steps, fixes the songAgent test comment, and adds leading slashes to the README
endpoint list.
|
||
|
Deluan Quintão
|
3d438b08ef
|
fix(jellyfin): close the unbounded playlist and search paths left by #5783 (#5784)
* fix(jellyfin): stream playlist tracks instead of loading every one PR #5783 left the playlist paths materializing: all three loaded every track of a playlist, whatever the client asked for. A playlist can be the whole library — a smart playlist matching everything — so this is the same OOM class that PR fixed for the other collections. Measured on a 96k-track smart playlist, /Items?ParentId=<playlist>&Limit=10 peaked at 1.3GB to return ten tracks. Playlist tracks now stream from a cursor, like every other collection: - PlaylistTrackRepository gains GetCursor and CountAll, sharing the select builder with loadTracks so cursor rows hydrate identically, plus GetMediaFileIDs for callers that need every id but no track data. - playlists.Tracks(ctx, id) exposes that repo to the HTTP layer with visibility enforced, returning ErrNotFound rather than the repo's nil-and-log-a-warning (which /Items would hit on every album browse, since ParentId is usually not a playlist). - /Playlists/{id}/Items now honors StartIndex/Limit, which it silently ignored before — it always returned the whole playlist. Real Jellyfin pages it. - getPlaylist runs an id-only query: PlaylistInfo carries every track id so it can't be paged, but it no longer hydrates rows it discards. - The route joins the throttled group, as it's now cursor-backed. Measured against a copy of a 96k-track production DB, peak RSS over idle, with byte-for-byte identical responses on every endpoint: /Items?ParentId=<pl>&Limit=10 1275MB -> 1MB 8.8s -> 3.6s (0.27s warm) /Items?ParentId=<pl> unbounded 1353MB -> 8MB 8.7s -> 3.4s /Playlists/<pl>/Items 1217MB -> 7MB 8.8s -> 3.4s /Playlists/<pl> 1178MB -> 33MB 9.1s -> 3.8s TotalRecordCount now costs a count query where the old path got it from len(tracks): 6ms on the largest real playlist in that library (2638 tracks), 288ms on the synthetic all-96k one. The old path paid 1.3GB and 4s+ instead. * fix(jellyfin): bound unbounded /Items searches The other collections stream, so an unbounded one costs about one item of memory. Search can't: the repositories' Search returns a slice, so it materializes every match. PR #5783 left two ways to reach that. A whitespace-only SearchTerm was the first. " " != "", so it took the search path, where doSearch trims it back to empty and hits its "empty query, return everything in natural order" branch — with no LIMIT, since executeTwoPhase only applies one when Max > 0. The whole library, materialized. Trimming at the two parse sites makes the `search != ""` checks mean what they look like they mean: a blank term is not a search, so it takes the unfiltered streaming path, which still returns everything, exactly as real Jellyfin does for an empty term. A real search with no Limit was the second, and needs an actual bound. Search gets a default of 100 when the client sends no Limit — matching the DefaultSearchLimit in Jellyfin's unreleased SqlSearchProvider — plus a ceiling, without which Limit=999999 would still materialize the library. The default alone wouldn't have closed the hole. An explicit Limit under the ceiling is honored unclamped, as upstream does; truncating a search is safe in a way truncating /Items is not, since nothing syncs a library through searchTerm. Note this is upstream's own bug: v10.11's /Search/Hints returns the entire library for a whitespace-only term, which master fixed by switching to ThrowIfNullOrWhiteSpace. * fix(jellyfin): cap the search Limit the client asked for, not the merge window searchPage sees two different things in opts.Max: the client's Limit for a single-type query, and mergeTypes' internal offset+limit window for a multi-type one. Clamping there hit both, so a multi-type search paging past the ceiling fetched only `ceiling` rows of the first type and the merged page skipped into the next one — IncludeItemTypes=Audio,MusicAlbum&SearchTerm=song&StartIndex=2000 &Limit=1 returned an album instead of the 2001st song. The ceiling now applies where the client's Limit is read: queryItems (after the playlist branch, so a playlist parent's page isn't capped by a stray SearchTerm) and getArtists, which reads its own. A limit of 0 stays 0, keeping searchPage's default. What a deep page materializes is then bounded by the client's StartIndex, as it already was for any multi-type query, search or not. Also from review: the playlist-track mock reused the previously stored Options when called without any, so a later no-args call inherited stale paging. * fix(jellyfin): apply the search default to the client's Limit, not per type The default lived in searchPage, which runs per type and after mergeTypes has already picked its branch. So an unbounded multi-type search left q.limit at 0, mergeTypes took its chained branch, and StartIndex was applied to a list each type had already truncated to the default — dropping matches rather than paging them. With 200 matching songs, IncludeItemTypes=Audio,MusicAlbum&SearchTerm=song &StartIndex=150 returned nothing at all, and without StartIndex it returned the default per type instead of in total. clampSearchLimit now applies the default and the ceiling together, to the client's Limit, at the two places it's read (queryItems and getArtists). A search therefore always gives mergeTypes a real window, so it pages the merged result and cuts it once, at the end. * fix(jellyfin): bound the multi-type search window against StartIndex mergeTypes asks each type for offset+limit rows before paginating the merged list, so a search still materialized whatever StartIndex asked for: IncludeItemTypes=Audio,MusicAlbum&SearchTerm=x&StartIndex=500000&Limit=1 pulled ~500001 matches per type. Bounding the window alone isn't enough — below it the merged rows are the client's page, but at it a truncated type is followed by the next one's rows, which is what made a clamped window serve an album where the 2001st song belonged. So the window is capped at maxSearchLimit and the page is clipped to it: pages below the ceiling are served in full and unchanged, a page straddling it is cut at it, and past it the result is empty rather than another type's rows. The total reports what can actually be paged to, so a client stops instead of asking for pages that no longer exist. This is the merge's own limit, not the client's: a single-type search still pages as deep as it likes, since its offset goes to SQL. Non-search multi-type queries keep the unbounded offset+limit window, which predates this and wants the same treatment via CountAll (exact per-type totals let whole types be skipped) rather than a cap. * fix(jellyfin): advertise the pageable search total, not the page window Clipping the multi-type search total to `window` clipped it to StartIndex+Limit on an ordinary page, so a first page of Limit=10 reported TotalRecordCount 10 however many matches there were, and a client paging on the total stopped after one page. The cap belongs at the ceiling — what can be paged to overall — not at the current page. The tests missed it because the only one asserting a total used StartIndex=maxSearchLimit, where the window happens to equal the ceiling. * refactor(jellyfin): fold the search clamp into clampLimit and simplify mergeTypes Cleanup pass over the branch, no behaviour change: - clampSearchLimit was clampLimit (similar.go) with different constants, so the latter takes the default and ceiling as arguments and both call it. Similar's default moves out of three IntOr calls into defaultSimilarLimit. - mergeTypes derived a second `limit` and needed an early return for the empty page, only because paginate reads 0 as "unbounded". Clipping the merged slice to the window instead lets q.limit be passed straight through: window is min(offset+limit, ceiling), so clipping there is the same cut. - playlistTracks returned (result, handled, error) where handled=false always meant error=nil. Splitting the lookup out gives playlistTracksRepo returning (repo, ok), and the nilerr suppression goes with it. - The comment on playlists.Tracks blamed the log warning for its extra Get; the reason is that PlaylistRepository.Tracks discards the error behind a nil. Also drops a stale reference to a renamed variable. |
||
|
Deluan Quintão
|
fe6ac2e577
|
feat(jellyfin): experimental Jellyfin Music API support (#5730)
* feat(sharing): enable sharing by default
Flip the EnableSharing default from false to true so new installations have the sharing feature available out of the box. Users can still disable it via the EnableSharing config option.
The native API only registers the /share route when sharing is enabled, so the nativeapi tests that build the router without wiring a share service now explicitly disable sharing in their setup to avoid registering a route backed by a nil service.
* feat(jellyfin): add config flag and URL path constant
Adds the disabled-by-default Server.Jellyfin config option (Enabled,
ServerName) and consts.URLPathJellyfinAPI, following the existing
LastFM/ListenBrainz patterns. Later tasks will use these to mount the
Jellyfin-compatible API router.
* fix(jellyfin): default Jellyfin ServerName to "Navidrome"
* feat(jellyfin): package skeleton with System handshake endpoints
Adds server/jellyfin: the Router (mirrors server/subsonic and
server/public), its Wire-friendly New(...) constructor, a chi routes()
table, and the ok() JSON response helper. Implements the unauthenticated
handshake surface Jellyfin clients probe first: GET /System/Info/Public,
GET+POST /System/Ping, and GET /QuickConnect/Enabled (quick connect is
unsupported, so it always reports disabled).
The public info payload's Id must be stable across restarts (Jellyfin
clients cache ServerId), so it reuses the get-or-create Property pattern
already used for InsightsID in core/metrics/insights.go: fetch
consts.JellyfinServerIDKey from the Property repository, generating and
persisting a new UUID on first read. A dedicated key (rather than the
existing InsightsID) keeps the anonymous telemetry identifier from being
exposed on this unauthenticated endpoint.
* test(jellyfin): cover ping and quickConnectEnabled handlers
Both were added alongside the System/Info/Public handshake endpoint but
had no direct test coverage.
* fix(jellyfin): memoize stable server Id and cover get-or-create path
* feat(jellyfin): wire and mount the router behind Jellyfin.Enabled
Add jellyfin.New to the shared Wire provider set and a
CreateJellyfinAPIRouter injector, then mount the router at /jellyfin
in startServer(), gated by conf.Server.Jellyfin.Enabled, mirroring the
existing LastFM/ListenBrainz router mounts.
* feat(jellyfin): add Jellyfin DTOs and model mappers
Adds BaseItemDto, QueryResult, UserItemDataDto, UserDto,
AuthenticationResult, MediaSourceInfo, PlaybackInfoResponse,
NameGuidPair and SessionInfo DTOs to server/jellyfin/dto, plus
model-to-DTO mappers for songs, albums, artists and genres that
later browse/stream/write endpoints will consume.
* test(jellyfin): cover GenreToBaseItem mapper
* feat(jellyfin): advertise Jellyfin-compatible version, brand ServerName with Navidrome version
* fix(jellyfin): map LastPlayedDate and omit zero track/disc numbers
* feat(jellyfin): authentication middleware and AuthenticateByName login
* fix(jellyfin): reject empty login password, wire ServerName, add negative auth tests
* refactor(jellyfin): use new(x) builtin instead of intPtr helper
* feat(jellyfin): user views and current-user endpoints
* feat(jellyfin): Items query engine, item detail, and latest
Adds the /Items universal query endpoint, dispatching by IncludeItemTypes
over albums/artists/songs/genres with ParentId, SearchTerm, Filters=IsFavorite,
SortBy/SortOrder and StartIndex/Limit support, plus GET /Items/{itemId} and
/Users/{userId}/Items/Latest. Reuses server/subsonic/filter builders (by
artist/album/starred) instead of hand-rolled squirrel filters, and resolves
sort keys per item type since each repository maps sort names to different
real/aliased columns.
Also adds the missing CountAll to tests.MockArtistRepo, exposed by this task
(the mock embedded a nil ArtistRepository for it and would panic on use).
* refactor(server): extract shared query filter builders to server/filter
Moves server/subsonic/filter to server/filter so server/jellyfin can use
the shared query-option builders without importing server/subsonic,
enforcing the rule that no API package imports another API's package.
* feat(jellyfin): full multi-library support with per-user access scoping
Replace the single hardcoded "music" UserView with one CollectionFolder
view per library the user can access, and scope every /Items browse
query (albums, songs, artists, /Latest) to those libraries via
server/filter's ApplyLibraryFilter/ApplyArtistLibraryFilter.
ParentId is now disambiguated: a numeric value the user has access to is
treated as a library scope (browsing a UserView), otherwise it falls
through as an entity id (artist/album), which safely matches nothing
rather than leaking another library's content. getItem now 404s when
fetching an album or song outside the user's accessible libraries;
artists are skipped (they can span multiple libraries) with a TODO.
Genres remain unscoped since they're global tags, not per-library
entities.
* test(jellyfin): cover admin library-access path and drop nil test contexts
* feat(jellyfin): Artists and Genres endpoints
Add /Artists, /Artists/AlbumArtists, /Genres and /MusicGenres. Artists
listing is library-scoped (defaulting to the user's accessible
libraries, narrowed by an accessible ParentId), delegating access
control to listArtists/ApplyArtistLibraryFilter. Genres are global and
unscoped, matching the ML decision already made for listGenres.
* refactor(jellyfin): share resolveLibraryScope between items and artists
* feat(jellyfin): item image endpoint via artwork service
* fix(jellyfin): let net/http sniff image Content-Type instead of forcing jpeg
* feat(jellyfin): audio streaming and PlaybackInfo with library access control
* feat(jellyfin): favorites and rating write-back with library access control
Adds POST/DELETE handlers for /Users/{userId}/FavoriteItems/{itemId} and
/Users/{userId}/Items/{itemId}/Rating. A shared resolveAnnotated helper
probes album/artist/media file (mirroring getItem's order) and 404s before
writing if the user lacks access to the album/song's library; artists are
exempt since they span multiple libraries. Ratings are halved coming in
(Jellyfin 0-10 -> Navidrome 0-5) to match the doubling in dto.UserData.
Also teaches MockMediaFileRepo and MockArtistRepo's SetStar/SetRating (and
MockAlbumRepo's, previously a no-op) to actually mutate the backing data so
write-back can be asserted in tests.
* feat(jellyfin): playback reporting and scrobbling
Add /Sessions/Playing[/Progress|/Stopped] and /Sessions/Capabilities[/Full]
handlers, backed by core/scrobbler.PlayTracker. A new withPlayer middleware
resolves/registers a model.Player from the Emby DeviceId header (used
directly as the stable player id, unlike Subsonic's cookie fallback) and
injects it into the request context for scrobbling.
The Stopped report calls ReportPlayback with IgnoreScrobble to end the
now-playing session without double-counting, then calls Submit as the
single source of the play-count increment and external scrobble.
* feat(jellyfin): playlist read and write-back
Adds POST /Playlists, GET/POST/DELETE /Playlists/{id}/Items. Tags each
playlist item with PlaylistItemId (the entry's position within the
playlist) so DELETE .../Items?EntryIds=... can remove a specific
occurrence by the id core/playlists.RemoveTracks actually expects,
rather than the song id used everywhere else.
* test(jellyfin): cover empty Ids/EntryIds in playlist add/remove
* feat(jellyfin): unknown-route logging, generic 500s, rating clamp, docs
Hardening pass ahead of real client testing: unmatched routes and
unsupported methods now return a logged, JSON 404 instead of chi's
default plain-text response, so a missing endpoint a client needs is
easy to spot in the logs. Internal errors (ffmpeg output, file paths,
etc.) no longer leak into 500 response bodies -- a shared
internalError helper logs the real error server-side and always
returns a generic message. Inbound Jellyfin ratings are clamped to
0-10 before being halved into Navidrome's 0-5 scale, and /System/Ping
now replies with a bare plain-text body as real Jellyfin servers do.
Adds a README with an enable/curl walkthrough and known limitations.
* docs(jellyfin): clarify public image endpoint and accessibleLibraryIDs comments
* fix(jellyfin): case-insensitive path routing for Jellyfin client compatibility
* refactor(server): extract case-insensitive path routing to a shared helper
* fix(jellyfin): return User Policy and Configuration so Finamp completes login
* fix(jellyfin): support Playlist type, multi-type Items queries, and PlayCount/DatePlayed sort
Real Finamp requests break against three /Items query engine bugs: Playlist
requests fell through to albums, multi-type IncludeItemTypes (e.g. Finamp's
favorites screen) only returned the first requested type, and comma-separated
SortBy lists (e.g. "DateCreated,SortName") were matched as one opaque string
so they always fell through to the repo default.
Also extends tests/mock_playlist_repo.go with SetData/GetAll so playlist
listing is testable like the other mock repos.
* feat(jellyfin): implement /socket WebSocket for real-time client sessions
* fix(jellyfin): resolve library-view ids in getItem so clients can load the library
* fix(jellyfin): serve direct file at /Items/{id}/File and accept ApiKey query param
* fix(jellyfin): resolve playlist ids in getItem
* fix(jellyfin): read lowercase ids/entryIds playlist params and add playlist-users endpoints
* fix(jellyfin): include MediaSources with Size/Bitrate on tracks so clients show download size
* fix(jellyfin): populate all required MediaSourceInfo bool/array fields to match Jellyfin
* fix(jellyfin): hex-encode item ids at the API boundary for Jellyfin client compatibility
Finamp (and presumably other clients) parses ids as radix-16, but Navidrome's
base62 nanoids aren't valid hex and crash its queue packing. Hex-encode every
id emitted by the Jellyfin API and decode every id received, keeping the
transform reversible and stateless at the boundary rather than touching any
model id.
* fix(jellyfin): populate MediaStreams with the audio stream so clients can size/transcode
* fix(jellyfin): support Ids batch-fetch in /Items so clients can fetch items by id
* fix(jellyfin): do not disable Jellyfin server when disabling external services
* fix(jellyfin): emit per-image ImageBlurHashes so clients de-dupe images and stop warning
* fix(jellyfin): support playlist cover upload/delete and display via item image endpoints
* feat(jellyfin): implement GET /Playlists/{id} for playlist visibility
Finamp's playlist edit screen calls GET /Playlists/{id} to read the
OpenAccess (public visibility) flag; we only had the sub-routes, so it
404'd and the edit screen failed to load. Return the Jellyfin PlaylistDto
shape (OpenAccess from Public, empty Shares, media item ids).
* fix(jellyfin): display playlist cover art
Uploaded playlist covers never showed in clients for two reasons: the
playlist BaseItemDto advertised no Primary ImageTag (so clients didn't
know to fetch a cover), and the public image endpoint resolved artwork
under the request's anonymous context, so a private playlist failed its
visibility filter and fell back to the placeholder. Advertise the Primary
image tag/blurhash on playlists, and resolve artwork under an elevated
context (as core/artwork's cache warmer does).
* fix(jellyfin): expand album/artist/playlist ids when building playlists
Jellyfin clients (Finamp) send container ids — an album, artist or
playlist — in a playlist's Ids list and expect the server to expand each
into its child tracks. core/playlists only understands media file ids, so
creating or adding with an album id silently produced an empty playlist.
Expand container ids to their tracks (in order) before create/add; bare
song ids still pass through. Adds filter.SongsByArtistID.
* fix(jellyfin): support playlist deletion via DELETE /Items/{id}
Finamp deletes a playlist with DELETE /Items/{id}, which we didn't route,
so deletion silently failed. Implement it via core/playlists.Delete (which
enforces ownership and removes the cover file). Only playlists are
deletable through this API; non-playlist ids return 404, non-owners 403.
* test(jellyfin): add e2e suite harness + smoke tests
* test(jellyfin): e2e for system, auth, routing, browsing, annotations
* test(jellyfin): e2e for playlists (CRUD, expansion, cover) and item images
* test(jellyfin): e2e for streaming, sessions, and multi-user access control
* fix(jellyfin): artist search 500 (library filter leaked into FTS query)
getArtists/listArtists reused the browse-path filters (notMissing +
ApplyArtistLibraryFilter) for the search path, but artist Search expects a
sole Eq{library_id} filter it can consume as a scope — artists have no
library_id column, so the compound/join filter leaked into the FTS query
and 500'd. Every Finamp artist search failed (the filter is applied even
unscoped, since admins resolve to all library ids). Build search filters
separately. Adds e2e search coverage.
* feat(jellyfin): implement POST /Playlists/{id} to update name, visibility, tracks
Finamp edits a playlist (make public, rename, reorder) via POST
/Playlists/{id}, which we didn't route, so every edit 404'd. Implement it:
Ids present -> replace track list (Create with existing id, preserving
name); otherwise update Name/IsPublic via core/playlists.Update. Adds a
shared playlistError helper (403/404/500) reused by deleteItem, plus e2e
coverage.
* docs(jellyfin): update README for playlists, images, id encoding, and e2e
* fix(jellyfin): default album track listing to track order
Browsing an album's tracks (Items?ParentId=<albumId>&IncludeItemTypes=Audio)
took only SongsByAlbum's filters and dropped its Sort, so tracks came back
in arbitrary order. Default opts.Sort to the album (disc+track) order when
browsing an album without an explicit SortBy — matching Subsonic's GetAlbum
and real Jellyfin. An explicit SortBy still wins.
* fix(jellyfin): filter items by AlbumArtistIds/ArtistIds
An artist's page in Finamp sends ParentId=<libraryId> (scoping) plus
AlbumArtistIds/ArtistIds/contributingArtistIds for the artist itself, but
queryItems only honored ParentId, so an artist's albums and tracks came
back unfiltered (every artist's content). Parse the artist-id params and
apply AlbumsByArtistID (albums) / SongsByArtistID (tracks). Adds e2e
coverage.
* fix(jellyfin): only count a play past the scrobble threshold
reportPlaybackStopped set IgnoreScrobble and force-submitted a play on
every Stopped report, so a briefly-played track (e.g. an immediate skip)
was marked played. Finamp sends Stopped on every track switch, so the
threshold must be applied server-side. Let ReportPlayback's StateStopped
logic decide (play + scrobble only past 50% of the track / 4-minute cap)
instead. Subsonic differs because there the client gates submission.
* fix(jellyfin): sort album tracks by track number when client sends IndexNumber SortBy
Finamp's album view requests SortBy=ParentIndexNumber,IndexNumber,SortName
(disc, track, name), but applySort didn't recognize ParentIndexNumber or
IndexNumber and fell through to SortName, sorting tracks alphabetically by
title. Map both keys to the album (disc+track) sort. The reversed-title
fixture lets the e2e tell track order from title order.
* fix(jellyfin): honor the isFavorite query param for favorites filtering
Finamp's artist 'Favourite tracks' widget requests favorites via the
standalone isFavorite=true query param, not Filters=IsFavorite, so the
filter was ignored and non-favorited tracks were returned. Detect both
forms. (The widget's reshuffling is Finamp's explicit SortBy=Random.)
* feat(jellyfin): emit DateCreated (Date Added) on items
BaseItemDto had no DateCreated, so clients showed 'No Date Added' and had
nothing to sort 'Recently Added' by. Emit it as ISO 8601 from each entity's
CreatedAt (the same field the recently_added sort uses) for songs, albums
and artists.
* fix(jellyfin): set ArtistItems/AlbumArtists on songs (Now Playing artist)
SongToBaseItem only set Artists (names) and AlbumArtist, not the structured
ArtistItems/AlbumArtists. Finamp's Now Playing screen reads ArtistItems and
shows 'Unknown Artist' when it's absent. Populate both (track artist and
album artist) as name+id pairs, mirroring AlbumToBaseItem.
* docs(jellyfin): note synthetic blurhash as a follow-up
Document that ImageBlurHashes are derived from the item id (a solid-color
placeholder), not computed from the cover art like real Jellyfin, and
outline what a proper implementation would take.
* docs(jellyfin): note WebSocket events and favourited playlists as follow-ups
* fix(jellyfin): filter the Artists page by role (album artist vs performer)
/Artists and /Artists/AlbumArtists both called the same role-agnostic
handler, so Navidrome's per-role artist entries (composers, arrangers,
performers) all showed as Album Artists, and the two tabs were identical.
Filter /Artists/AlbumArtists to RoleAlbumArtist and /Artists to RoleArtist
(and the MusicArtist browse to album artists), via filter.ArtistsByRole.
Verified live: Beatles Singles' album-artists dropped 138->5, composers
excluded, performers (Billy Preston) show only under /Artists.
* fix(jellyfin): sort playlists by name (missing Playlist sort mapping)
applySort had no sortColumnsByType entry for the Playlist type, so
SortBy=SortName was ignored and the Playlists screen showed them in the
repo's default order. Map SortName/Name -> name (as Subsonic does) and
DateCreated -> created_at. Verified live: case-insensitive alphabetical.
* fix(jellyfin): read query params case-insensitively (Jellify support)
Jellify (and the official Jellyfin TypeScript SDK) send query params in
camelCase (parentId, albumArtistIds, artistIds, includeItemTypes), where
Finamp sends PascalCase. Our handlers read fixed-case keys, so every
Jellify filter/sort/paging param was silently dropped:
- an artist's page listed albums and tracks from all artists
- opening an album listed every album instead of its tracks
Real Jellyfin binds query params case-insensitively (ASP.NET model
binding), so add a normalizeQueryKeys middleware that folds every query
key to lowercase once, and read params by their lowercase name. This
also removes the scattered dual-case hacks (Ids/ids, IsFavorite,
ContributingArtistIds, api_key/ApiKey, queryParam) that let this bug
class through.
Also infer the child type from an album parent: Jellify browses an album
with only parentId (no IncludeItemTypes), and Jellyfin infers Audio from
the parent; without it we fell back to listing all albums.
Verified live against Finamp+Jellify and with 4 new e2e specs replaying
Jellify's exact request shapes.
* fix(jellyfin): advertise LocalAddress in the public system info handshake
Jellify (and other @jellyfin/sdk clients) that connect by raw address fall
back to HTTP when TLS isn't available, then adopt the handshake's
LocalAddress as their server base URL. We never populated it, so Jellify's
SDK `api` object was undefined and sign-in crashed with "Cannot read
property 'configuration' of undefined" (getUserApi(api!) with an undefined
api). Over an HTTPS connection Jellify uses connectionType=hostname and
never reads LocalAddress, which is why it worked while an HTTPS proxy was
in front.
Populate LocalAddress from the request — scheme + host (honoring
X-Forwarded-*), plus the /jellyfin mount path — matching real Jellyfin,
which always sends it. Export server.ServerAddress for the resolution.
* fix(jellyfin): separate "Featured On" from an artist's own discography
Jellify's artist page fetches the discography via albumArtistIds and the
"Featured On" section via contributingArtistIds, relying on the server to
return disjoint sets. We collapsed albumArtistIds/artistIds/
contributingArtistIds into one AlbumsByArtistID filter, so an artist's own
albums appeared in both sections.
Add AlbumsByContributingArtistID — albums where the artist is a track
artist but NOT the album artist — matching Jellyfin's ContributingArtistIds
(in Artists, not in AlbumArtists), and route contributingArtistIds to it.
* fix(jellyfin): accept a bare Authorization token for audio streaming
Jellify's native player (react-native-nitro-player) authenticates the audio
stream by setting a bare Authorization header carrying the raw access token
({ AUTHORIZATION: api.accessToken }), not the "MediaBrowser ... Token=" scheme
parseEmbyAuth understands. tokenFromRequest didn't recognize it, so every
/Audio/{id}/stream request from the native player 401'd and no audio played
(the JS client still optimistically posted playback progress, masking it).
Accept a bare (or "Bearer <token>") Authorization header, as real Jellyfin
does. Verified live: the native player's exact request now streams 200.
* fix(jellyfin): embed a self-authenticating stream URL in PlaybackInfo
Jellify's native audio player (react-native-nitro-player / ExoPlayer) fetches
the stream without forwarding any auth — captured request headers were only
Connection/Icy-Metadata/Accept-Encoding/User-Agent, no Authorization and no
api_key — so every /Audio/{id}/stream request 401'd and nothing played (the
JS client still optimistically posted progress, masking it).
Real Jellyfin returns stream URLs with the token embedded; we returned none,
so Jellify fell back to a token-less DirectPlay URL. Populate
MediaSources[0].TranscodingUrl with /Audio/{id}/universal?api_key=<caller
token>, which Jellify's player uses verbatim. Direct-play clients (Finamp
builds its own /Items/{id}/File?ApiKey URL) ignore the field, so they're
unaffected.
* feat(jellyfin): serve per-item UserData (GET /UserItems/{id}/UserData)
Jellify fetches this per item to render played/favourite indicators; we 404'd
it. Resolve the item via the existing getItem resolver (which loads the
caller's annotations and enforces the same library-access gate) and return its
UserData, falling back to an empty-but-valid object for items without
annotations (e.g. playlists). Also registers the legacy
/Users/{userId}/Items/{itemId}/UserData spelling.
* refactor(jellyfin): drop unused bare-Authorization token support
Added mid-troubleshooting on the theory that Jellify's native player sends a
bare Authorization header, but the debug capture showed it sends no auth header
at all — the real fix was embedding api_key in PlaybackInfo's TranscodingUrl.
No client reaches this path (Jellify JS uses the MediaBrowser scheme, Finamp
uses X-Emby-Token, the native player uses the api_key URL), so remove it and
its tests per YAGNI. Effectively reverts 4f8ac1e0.
* feat(jellyfin): implement Similar endpoints via the external provider
Add GET /Artists/{id}/Similar (related artists) and GET /Items/{id}/Similar
(similar songs for a track, similar albums for an album, related artists for an
artist), sourced from the same external.Provider (Last.fm etc.) that powers
Subsonic's getArtistInfo2/getSimilarSongs. Only library-present artists are
returned so each is navigable; provider errors and unknown ids degrade to an
empty 200 result, so clients (Jellify) stop hammering these with 404 retries.
Injects external.Provider into the Router (regenerated via make wire).
* perf(jellyfin): make Similar endpoints non-blocking (bounded quick wait)
Each /Similar request fetched from Last.fm synchronously (500ms-1.4s), and
Jellify requests Similar for many items at once on the home/artist screens, so
the whole screen stalled — a home pull-to-refresh dragged for seconds.
Run the external lookup on a background context and return within a 500ms quick
wait: a cached artist resolves instantly, a cold one returns empty now while the
lookup finishes caching in the background, so a later load is fast and
populated. Verified live: cold calls bounded at ~500ms (was up to 1.4s), warm
calls 3-8ms.
* fix(jellyfin): answer ManualPlaylistsFolder so the home stops stalling
Jellify resolves its "playlists library" via IncludeItemTypes=
ManualPlaylistsFolder, then lists playlists with ParentId set to that folder's
id. We didn't recognize the type, so parseTypes fell back to MusicAlbum and
returned the album list. Jellify's query then found no item with
CollectionType=playlists and resolved undefined — which React Query rejects,
retrying it in a backoff loop that stalled the home pull-to-refresh for ~5s
(every response was fast server-side; the delay was the client's retries).
Return a synthetic "playlists" folder (CollectionType=playlists) for the
ManualPlaylistsFolder query, resolve ParentId=<that folder> to the user's
playlists, and give playlists a Path under "data" (Jellify drops playlists
whose Path lacks it). Adds a Path field to BaseItemDto.
* docs(jellyfin): note lyrics and InstantMix/sonic-similarity as follow-ups
* fix(jellyfin): genre paging params and same-key casing collisions
getGenres read StartIndex/Limit in PascalCase, which normalizeQueryKeys had
already folded to lowercase, so genre paging was silently ignored; totals now
come from the full (small) genre list instead of the page length.
normalizeQueryKeys now merges values when two casings of a key collide,
instead of nondeterministically keeping one.
* fix(jellyfin): round ratings to the nearest star instead of truncating
Rating is a nullable double 0-10 in Jellyfin's contract. Truncating integer
division stored 9 as 4 stars, and both Rating=1 and fractional values (which
failed integer parsing) became 0 — silently deleting the rating. Parse as
float, round, and floor nonzero input at one star.
* fix(jellyfin): apply Name/IsPublic sent together with a track replacement
Jellyfin's UpdatePlaylist applies every provided field, but the Ids branch
returned early, silently discarding a rename or visibility change sent in the
same body (core/playlists.Create with an existing id ignores the name).
* fix(jellyfin): don't rotate the stored server id on transient DB errors
serverID treated any Property.Get error as "no id yet" and persisted a fresh
UUID over JellyfinServerID, with sync.Once pinning it for the process lifetime
— a busy DB or canceled request context on the first request would break every
client's cached ServerId. Only ErrNotFound mints a new id now, and failures
yield an uncached temporary value so the next request retries.
* fix(jellyfin): report real search totals instead of page length or unfiltered counts
Artist search returned TotalRecordCount = len(page), so clients stopped after
the first page; album/song search counted via CountAll, which can't see the
search term, so clients paged through phantom results. The repos' Search API
has no match count, so fetch one row beyond the page: offset+len is exact on
the last page and a strictly growing lower bound before it — paging clients
terminate exactly at the last match.
* fix(jellyfin): browse playlists via the generic /Items path and resolve the playlists folder by id
A typeless /Items?ParentId=<playlistId> (legal in real Jellyfin, used by
generic clients) fell through to the MusicAlbum default and returned an empty
list; it now returns the playlist's tracks, paginated, with visibility
enforced by GetWithTracks. resolveItemByID also answers the synthetic
playlists-folder id the server itself advertises instead of 404ing it.
* perf(jellyfin): cap per-type queries in multi-type /Items requests
The multi-type merge path queried each type with a zero-value QueryOptions —
no LIMIT in SQL — materializing every matching row (each with embedded
MediaSources) just to slice out one page in memory. Each type now fetches at
most StartIndex+Limit rows, the worst case one type can contribute to the
merged window; totals still come from CountAll.
* fix(jellyfin): dedupe and bound the background Similar fetches
awaitSimilar spawned a detached, deadline-free goroutine per request; clients
re-polling after the empty quick-wait response piled up duplicate provider
chains (no singleflight anywhere below) racing writes on the same artist row.
Identical in-flight requests now share one fetch — keyed per user, since the
mapped items embed the user's annotations — and the background context gets a
one-minute deadline so a hung provider can't hold goroutines forever.
* fix(jellyfin): gate private playlist covers on the public image route
The unauthenticated image endpoint elevated every request to an admin context,
so anyone who knew or guessed a playlist id could fetch another user's private
uploaded cover. Library artwork still resolves elevated (Jellyfin clients fetch
images without auth headers), but playlist covers are now served only when the
playlist is public or the request's optional token identifies its owner or an
admin; everyone else gets the placeholder.
* fix(log): redact full api_key values, including JWTs
The api_key pattern matched only word characters, stopping at a JWT's first
'.' — the Jellyfin API embeds the session JWT as api_key in TranscodingUrl, so
request logs kept its payload and signature, enough to reconstruct a replayable
token by prepending the constant header. Match to the next query separator
instead, like the sibling s=/p=/jwt= patterns.
* fix(jellyfin): record LastLoginAt on Jellyfin logins
authenticateByName re-implements credential validation and skipped the
UpdateLastLoginAt call the web UI's validateLogin makes, so users who only log
in via Jellyfin clients showed a never/stale Last Login in the admin UI.
* perf(jellyfin): batch song resolution in /Items?ids= and playlist expansion
Both paths probed up to four repositories per client-supplied id. Songs — the
common case — now resolve via chunked media_file.id IN queries (same pattern
as playqueue's loadTracks); only the residue pays the container probes.
* fix(jellyfin): wait for the real Similar result instead of answering a cacheable empty list
The 500ms quick wait returned an empty 200 for any cold lookup —
indistinguishable from "no similar items exist", so clients cached the wrong
answer until an app restart. With fetches deduplicated, wait up to the agents'
HTTP timeout for the actual result; only a hung provider now yields the empty
fallback, and its fetch still warms the cache in the background. Also makes
the dedup test deterministic (the old one raced its release channel).
* refactor(tests): extract the shared e2e harness into tests/harness
The Subsonic and Jellyfin e2e suites each carried their own copy of the
golden-DB lifecycle (boot, seed users/library, scan, WAL snapshot), the
ATTACH-DATABASE restore, fixture-FS registration, and the SpyStreamer /
NoopFFmpeg doubles. Those now live in one importable package (same pattern as
core/storage/storagetest); fixture libraries and request helpers stay
per-suite since they encode each API's test expectations.
* docs(jellyfin): make comments concise
Compress the narrative comments accumulated during live client testing into
short why-only notes; keep the client quirks, security rationales and gotchas,
drop the exposition. Comments-only change (net -141 lines).
* fix(tests): silence gosec taint false-positive in harness snapshot write
The write moved from a _test.go file (which gosec skips) into the importable
harness package; the path derives from GinkgoT().TempDir().
* fix(jellyfin): route the current /UserFavoriteItems favorite endpoint
@jellyfin/sdk 0.13.0 (used by Jellify) posts favorites to
POST/DELETE /UserFavoriteItems/{itemId}, while we only routed the legacy
/Users/{userId}/FavoriteItems/{itemId} (Finamp). Jellify favorites 404'd
("Failed to add favourite"). Route both spellings to the same handlers.
* feat(jellyfin): honor Fields on /Items and add missing conformance fields
Match real Jellyfin's response shape: gate MediaSources (and MediaStreams)
behind Fields=MediaSources instead of always embedding them — clients that
need Size request it, as Finamp does — which cuts a 46-track artist response
from ~87KB to a fraction. Also emit the always-present fields Jellyfin sets:
ServerId (stamped centrally in ok), LocationType, HasLyrics, and SortName
(when Fields=SortName). PlaybackInfo still carries MediaSources (its purpose).
* fix(jellyfin): address code review security and correctness findings
Applies the reviewed findings from PR #5730:
- Gate similar songs/albums on the caller's library access, so the
external provider can't surface metadata from libraries the user
cannot see. Also clamp the client-supplied limit before it sizes any
allocation or provider fetch (CodeQL user-controlled allocation).
- Prepend the /jellyfin mount prefix to the PlaybackInfo TranscodingUrl,
so a client resolving it as an absolute host path still reaches the
mounted router.
- Recognize WebP and GIF magic numbers on raw cover uploads, matching
the formats Navidrome already supports.
- Bound the playlist cover upload: honor EnableArtworkUpload for
non-admins and cap the body with MaxBytesReader/MaxImageUploadSize,
mirroring the native image endpoint.
- Propagate non-not-found repository errors from resolveAnnotated as 500
instead of silently returning 404.
- Normalize the literal prefix of mixed literal.param path segments
(e.g. STREAM.mp3) so case-insensitive routing reaches stream.{container}.
- Rate-limit POST /Users/AuthenticateByName with the same per-IP limiter
as /auth/login when AuthRequestLimit is set.
- Guard against a nil user in authenticateByName.
* fix(jellyfin): correct playlist track browsing and multi-id edits
Fixes three playlist issues, two found testing against Jellify and one
from the PR review:
- Resolve a playlist ParentId to its tracks even when the client sends
IncludeItemTypes=Audio. Jellify opens a playlist with
ParentId=<playlist>&IncludeItemTypes=Audio; the id was routed through
listSongs as an album id, returning an empty list.
- Read repeated id query params (ids=X&ids=Y), not just the first value.
Jellify's @jellyfin/sdk serializes id arrays as repeated params, so
adding an album (which it expands client-side into many ids) only
added the first track. Applies to both add and remove; the
comma-separated form other clients use still works.
- Let an explicit empty Ids array clear a playlist. Ids is now a pointer
so an omitted field still means 'leave unchanged', while an empty list
clears the tracks (via RemoveTracks, since the repository skips track
writes for an empty list).
* fix(jellyfin): register clients as players on any authenticated request
Jellyfin clients did not appear in the players list. Unlike Subsonic,
whose getPlayer middleware runs on every authenticated endpoint, the
Jellyfin router only registered a player on the /Sessions/Playing
reports, so browsing or streaming never created one.
Apply withPlayer to the whole authenticated group, mirroring Subsonic,
so the calling device registers (and scrobbling has a player) as soon as
it makes any authenticated request. Two follow-ups found while testing:
- Skip registration when the request carries no client/device info (no
X-Emby-Authorization, e.g. the /socket handshake that auths via
?api_key= only), which otherwise created a junk player named ' []'.
- URL-decode the X-Emby-Authorization field values. Jellify's
@jellyfin/sdk percent-encodes them (Device='Pixel%208%20Pro') while
Finamp sends them raw, so the player name showed as
'Jellify [Pixel%208%20Pro]'.
* refactor(jellyfin): move case-insensitive routing into the jellyfin package
The case-insensitive path normalization lived in the server package but
was only ever used by the Jellyfin router (its whole purpose is that
Jellyfin clients route case-insensitively while chi does not). Move it
into server/jellyfin and unexport it, so it sits with its only caller
and no longer needs to be exported across a package boundary.
* docs(jellyfin): document player registration, playlist and image behavior
Updates the package README for the behavior added/fixed this round:
- New 'Players and sessions' section: any authenticated request now
registers the device as a player (like Subsonic), with the
Client [Device] naming, URL-decoding of the Emby auth fields, and the
/socket skip that avoids a nameless player.
- Authentication: note AuthenticateByName is rate-limited per IP.
- Playlists: repeated vs comma-separated id params, and that an explicit
empty Ids clears the playlist while an omitted Ids leaves it untouched.
- Cover art: WebP/GIF magic-number detection plus the MaxImageUploadSize
and EnableArtworkUpload gates.
- Endpoints table: add the /Similar and /UserFavoriteItems /
/UserItems/.../UserData routes that were already served but unlisted.
* feat(jellyfin): expose configured users on the login user-picker
Adds Jellyfin.ExposedPublicUsers, a comma-separated allowlist of
usernames that GET /Users/Public advertises so Jellyfin clients (Finamp,
Jellify) can show a login user-picker instead of a blank username field.
The endpoint is unauthenticated, so it defaults to exposing no users and
never lists the full user table: only the admin-configured names are
returned, resolved live per request (a name that doesn't exist is skipped
and logged). Each entry is a minimal DTO (Name, Id) with no
Policy/Configuration, so admin status isn't leaked pre-login, and no
avatar since Navidrome has no per-user profile images.
* style(jellyfin): trim redundant comments
Remove comments that restated the code or duplicated an explanation
already given nearby, keeping the ones that capture non-obvious rationale
(client-specific quirks, gotchas). Comment-only change; no behavior
difference.
* perf(db): keep query planner statistics trustworthy with full ANALYZE
PRAGMA optimize's internal ANALYZE runs with a limited analysis budget
(~2000 rows) that writes wrong sqlite_stat1 entries for low-cardinality
indexes: on a 96K-track library it claimed (missing, library_id) narrows
to ~2000 rows when it matches the whole table. The planner then prefers
that index over the sort index and falls back to a full-table temp
B-tree sort per request, turning paginated song listings into
multi-second queries (reproduced at 5.5s on real hardware; ~90x slower
than with correct stats). Every index-creating migration re-triggered
the poisoning via the post-migration optimize, and the daily optimizer
could re-trigger it on large library changes. Setting analysis_limit on
the connection does not help: optimize ignores it.
Run a plain full ANALYZE instead: after migrations with schema changes,
and in db.Optimize (daily schedule and scan-end). Stats are stored in
the database file, so one connection suffices and the per-connection
pool loop is gone. The Optimize call at shutdown is removed: stats are
maintained at migration/scan/daily points, and an ANALYZE during
shutdown only delays it and races container stop timeouts.
* perf(db): add covering index for title-sorted song listings
Deep pagination over songs sorted by title (WHERE missing/library_id,
ORDER BY order_title LIMIT/OFFSET - the shape Jellyfin clients use to
enumerate the library, and non-admin native/Subsonic song lists share)
walked media_file_order_title and fetched the table row for every
skipped entry just to evaluate the filter and the annotation/bookmark
join keys: offset+limit random reads, seconds per page on cold spinning
disks.
The new (missing, library_id, order_title, id) index makes the offset
skip fully index-resident: filter columns and the join key come from the
index, and only the emitted page touches table rows. Measured on a
96K-track library: offset 50000 drops from ~6.5s (poisoned stats) /
~100ms (good stats, warm) to 24ms, and cold deep pages on NAS hardware
from ~7s to ~0.5s.
* feat(jellyfin): support transcoding via HLS playlist and server-forced player format
- withPlayer now propagates the player's configured transcoding into the
request context (like Subsonic's getPlayer), so a format forced in
Settings > Players applies to the Jellyfin stream endpoints
- new GET /Audio/{itemId}/main.m3u8, the endpoint Finamp plays through
when its transcoding setting is enabled: a single-segment HLS VOD
playlist pointing at the existing progressive transcode endpoint
- streamAudio: treat audioBitRate as bits/sec (Jellyfin convention) and
fall back to audioCodec as target format when no container is given
* fix(jellyfin): honor GenreIds when browsing genre albums and tracks
Finamp's genre screen sends ParentId=<libraryId> plus GenreIds=<genreId>,
but /Items ignored the param, so every genre returned the whole library.
- filter.ByGenreID delegates to persistence.TagIDFilter (exported, was
tagIDFilter), the same mechanism behind the native API's genre_id filter
- id lists are read via queryIDs, covering both spellings clients use
(comma-separated and repeated params); /Items?ids= gains the repeated
form too
* feat(jellyfin): filter album artists by GenreIds
Finamp's artist tab sends GenreIds to /Artists/AlbumArtists when a genre
filter is active; the param was ignored, returning all artists.
filter.ArtistsByGenreID matches artists credited as album artist on an
album with the genre, via a non-correlated semi-join over album
participants (86ms on a 29K-artist library; the correlated EXISTS form
takes 11 minutes). Applied on the browse path of /Artists* and
/Items?IncludeItemTypes=MusicArtist.
The performers variant (/Artists?GenreIds=) still ignores the filter: it
needs the same semi-join against media_file.participants, unmeasured on
large libraries.
* fix(jellyfin): version playlist image tag so clients refresh uploaded covers
Uploads were stored and served correctly, but Finamp kept showing the old
cover: it caches covers keyed by blurHash (its imageId is the item id,
which never changes), and both our image tag and synthetic blurhash were
derived from the playlist id alone.
The tag is now <id>-<UpdatedAt millis hex> (SetImage/RemoveImage go
through a full Put, which bumps UpdatedAt), and the blurhash derives from
the tag, so every cover change rotates both. UpdatedAt over-invalidates
(any playlist edit busts the cover cache), which only costs a refetch;
hashing the actual image remains the proper long-term tag.
An e2e test guards the whole chain, since a partial Put(pls, cols...)
would silently stop bumping UpdatedAt.
* fix(jellyfin): align cover upload limit and validation with the native endpoint
Cover uploads of large photos failed with a silent 400: the
MaxImageUploadSize cap was applied to the wire body, which Jellyfin
clients base64-encode (4/3 inflation), so the effective raw-image limit
was only ~7.5MB — and Finamp uploads picked photos uncompressed.
Align with the native endpoint:
- the limit caps the decoded image; the read cap allows for base64
inflation
- validate by decoding (image.DecodeConfig) and take the storage
extension from the real format instead of the Content-Type header,
which clients get wrong (Finamp falls back to image/jpeg); a HEIC or
corrupt file is now rejected instead of stored as a broken cover
- rejected uploads log the reason (size/limit/decode error); diagnosing
this from production logs previously required guesswork
* fix(jellyfin): optimize SongsByArtistID filter to improve performance at library scale
Signed-off-by: Deluan <deluan@navidrome.org>
* fix(jellyfin): sort tracks by release year for SortBy=PremiereDate
Finamp's "Latest Releases" artist section sends
SortBy=PremiereDate,Album,... descending; PremiereDate wasn't in the
Audio sort map, so applySort fell through to Album and the view came
back in reverse album-name order (a 2002 remix album first, the 2013
release last).
Map premieredate/productionyear to the year sort key, matching the
ProductionYear the DTO exposes for songs and the existing MusicAlbum
mapping (premieredate -> max_year).
* feat(jellyfin): expose PremiereDate on tracks and albums
Finamp's "Latest Releases" artist/genre sections re-sort the merged
server responses client-side by PremiereDate and keep the top 5. Without
the field every comparison returns equal and Dart's unstable sort leaves
the picks in arbitrary order — a 2007 remix could lead the list even
with the server sorting by year correctly.
Serialize PremiereDate as ISO 8601 from the date tag (padding partial
"2007"/"2007-02" values so DateTime.tryParse accepts them), falling
back to the year; omitted when neither exists.
* feat(jellyfin): resolve Finamp-truncated item ids (saved queue restore)
Finamp persists its play queue by packing every item id into exactly 16
bytes (packIds assumes Jellyfin's 32-hex GUID ids), so our longer ids
come back truncated after an app restart and "Failed to restore queue"
loops forever: the /Items?ids= batch resolves nothing.
Navidrome ids can't be made GUID-shaped (nanoid ids can exceed 128 bits),
so compensate server-side: a 16-char id — a length no Navidrome id family
uses — is resolved by unique-prefix range scan, with ambiguity failing
safe. The ids= batch echoes the id as requested (Finamp matches restored
items by its stored ids), and stream, image, item, user-data, favorite,
rating and playback-report endpoints accept truncated ids transparently.
The proper fix belongs upstream in Finamp's packIds; documented in the
README so this layer can be removed once that ships.
* feat(jellyfin): implement Items/{id}/InstantMix
Finamp requests an instant mix on every track tap when its "start
instant mix for individual tracks" setting is on (plus the long-press
menus); the 404 made those taps fail with an error and play nothing.
A track seed returns itself first — Finamp plays exactly what comes
back — followed by the external provider's similar songs, capped at the
requested limit and filtered to the caller's libraries. Container seeds
(artist/album) return the provider's similar-songs blend. Provider
errors and unknown seeds degrade to seed-only/empty results instead of
404s, reusing the Similar endpoints' bounded-wait singleflight (with a
distinct cache key, since mixes and similar lists answer different
shapes). Sonic-similarity backing stays a follow-up (see README).
* style(jellyfin): trim wordy comments
* refactor(jellyfin): apply cleanup review findings
- batch truncated-id resolution in /Items?ids=: one chunked range query
for all media-file prefixes instead of a query per id (a restored
queue sends hundreds of truncated ids)
- resolve truncated ids on the /Similar endpoints too, matching the
neighboring InstantMix; document which entry points don't resolve
- hoist maxImageUploadSize to core, deleting the byte-identical copies
in nativeapi and jellyfin (tests moved to core)
- drop the unused type parameter on premiereDate
* fix(jellyfin): never drop the instant mix seed on a slow provider
The seed track was built inside the awaited provider fetch, so when the
external agent was slow or unreachable the request hit the 10s wait and
answered a fully empty mix — Finamp then played nothing on tap, even
though the seed needs no provider at all (seen live: Last.fm unreachable
from the server, responseSize=49).
Build the seed outside the await: only the similar-songs tail is fetched
and bounded, and a timeout now degrades to a seed-only mix. Also folds
instantMixForSong into getInstantMix, since the tail is exactly
similarSongs.
* fix(jellyfin): prefer the recommended Authorization scheme when picking a token
Jellyfin's authorization guidance deprecates X-Emby-Token,
X-MediaBrowser-Token, X-Emby-Authorization and api_key; the Authorization
MediaBrowser scheme is the recommended form. All spellings stay accepted,
but when a client sends several, the recommended one now wins. Adds
coverage for the canonical Authorization header, which no test exercised
directly.
* refactor(jellyfin): rename parseEmbyAuth to parseMediaBrowserAuth
The scheme is named MediaBrowser; the old name evoked the deprecated
X-Emby-* spellings even though the function also parses the recommended
Authorization header.
* fix(jellyfin): prefer the Authorization header over X-Emby-Authorization
The recommended header now wins when both carry MediaBrowser data — but
only when it actually parses as MediaBrowser: a reverse proxy may inject
Basic/Digest credentials into Authorization while the client sends the
deprecated header, and those must not swallow the client's auth.
* fix(jellyfin): require the MediaBrowser scheme when parsing auth headers
The parser extracted key="value" pairs from any Authorization value; a
foreign scheme whose parameters happened to use our field names would
have been misread as client auth. Validate the scheme word instead
(case-insensitively, per HTTP), accepting the legacy "Emby" spelling
like real Jellyfin. Replaces the any-recognized-field heuristic for
detecting proxy-injected Basic/Digest credentials.
* Revert "perf(db): keep query planner statistics trustworthy with full ANALYZE"
This reverts commit 118563e1053196f0e2e42dd4bee54e39a04ab145.
The planner-statistics work now lives in its own PR (#5740); this branch
keeps only the covering-index migration.
* fix(db): renumber jellyfin covering-index migration after master's latest
* fix(db): renumber Jellyfin covering-index migration
* docs(jellyfin): document playlist annotations
* refactor(log): clarify comments on external services query params
* refactor(e2e): move Subsonic e2e suite to server/subsonic/e2e
All files in server/e2e were Subsonic tests, so relocate the package
under server/subsonic/e2e to sit alongside the API it exercises. The
package name stays 'e2e'; only doc/comment references are updated.
* refactor(filter): consolidate genre filters and unexport tagIDFilter
Route filterByGenre, ByGenreID, and ArtistsByGenreID through a single
genreTagFilter helper so the EXISTS json_tree(tags,"$.genre") predicate
lives in one place. With server/filter no longer using it, persistence's
TagIDFilter is only referenced within the package, so unexport it.
---------
Signed-off-by: Deluan <deluan@navidrome.org>
|
||
|
Deluan Quintão
|
cc315dcc8c
|
perf(db): keep query planner statistics trustworthy with full ANALYZE (#5740)
* perf(db): keep query planner statistics trustworthy with full ANALYZE PRAGMA optimize's internal ANALYZE runs with a limited analysis budget (~2000 rows) that writes wrong sqlite_stat1 entries for low-cardinality indexes: on a 96K-track library it claimed (missing, library_id) narrows to ~2000 rows when it matches the whole table. The planner then prefers that index over the sort index and falls back to a full-table temp B-tree sort per request, turning paginated song listings into multi-second queries (reproduced at 5.5s on real hardware; ~90x slower than with correct stats). Every index-creating migration re-triggered the poisoning via the post-migration optimize, and the daily optimizer could re-trigger it on large library changes. Setting analysis_limit on the connection does not help: optimize ignores it. Run a plain full ANALYZE instead: after migrations with schema changes, and in db.Optimize (daily schedule and scan-end). Stats are stored in the database file, so one connection suffices and the per-connection pool loop is gone. The Optimize call at shutdown is removed: stats are maintained at migration/scan/daily points, and an ANALYZE during shutdown only delays it and races container stop timeouts. * perf(db): drop startup PRAGMA optimize that re-poisons planner stats The startup PRAGMA optimize=0x10002 runs SQLite's budget-limited internal ANALYZE (bit 0x02), which writes truncated sqlite_stat1 rows for low-cardinality indexes -- the exact statistics-poisoning this PR set out to eliminate. Because DevOptimizeDB defaults to true, a restart with no pending migrations would re-poison the planner until the next scan or daily Optimize. Remove it: statistics are already refreshed with a full ANALYZE after schema-changing migrations (Init) and via Optimize at scan-end and on the daily schedule, so nothing on the startup path needs to touch them. Also clarify that Optimize is a no-op unless DevOptimizeDB is enabled. * chore(db): remove the DevOptimizeDB flag and skip Optimize on quick scans The flag only gated the optimize/ANALYZE maintenance calls and there is no reason to leave planner statistics unmaintained; the guards are gone along with the flag. The scan-end Optimize now runs only after full scans — quick scans barely move the statistics, and the daily schedule covers drift. * style(scanner): drop redundant comment in runOptimize * chore(persistence): drop the no-op PRAGMA optimize from ScanEnd Mask 0x10000 only selects candidate tables by size change; without the 0x02 action bit optimize does nothing (verified: sqlite_stat1 stays stale after a 100x table growth). The scan-end statistics refresh is db.Optimize's full ANALYZE, and the expression-collation-index concern the old comment guarded against no longer applies. * fix(scanner): run the post-scan ANALYZE in the server process With the external scanner (the default), the scan pipeline runs in a subprocess, so its ANALYZE was invisible to the server: SQLite loads sqlite_stat1 into the process's shared schema cache, and an ANALYZE from another process does not refresh it — verified with the production DSN that even brand-new pool connections keep planning with the old statistics until the server restarts. An in-process ANALYZE, by contrast, is immediately visible to every pooled connection through the same shared cache. Move the full-scan Optimize from the scanner pipeline to the scan controller, which always runs in the server process. * fix(scanner): honor promoted full scans in the optimize gate A quick scan resuming an interrupted full scan is promoted inside the scanner (possibly in a subprocess); mirror the promotion in the controller so the post-scan ANALYZE isn't skipped. * refactor: apply cleanup review findings - drop forceFullRescan's inline ANALYZE: Init already runs a full ANALYZE after any migration batch with schema changes, so upgrades including a full-rescan migration analyzed the whole DB twice - resumingFullScan uses a filtered CountAll instead of fetching and scanning all libraries - document why CallScan (CLI) deliberately skips the post-scan Optimize * perf(db): make planner analysis maintenance resilient Check analysis freshness every 30 minutes and refresh statistics when the last successful run is over 24 hours old or a scan marked them pending. Persist successful analysis state, retry skipped or failed maintenance, coordinate checks with scans, and cover standalone CLI full scans. * perf(db): avoid analyzing routine quick-scan changes Reserve pending analysis for full scans, unscanned libraries, and retry state. Incremental quick scans now rely on the 24-hour freshness window instead of triggering a full ANALYZE at the next maintenance check. * fix(scan): analyze resumed full scans in CLI * fix(db): back off failed analysis retries * feat(db): allow disabling scheduled analysis * test(db): remove redundant analysis coverage * refactor(db): split ANALYZE maintenance into optimize.go and dedupe call sites - move query-planner statistics code from db.go to its own optimize.go (and matching optimize_test.go) - log ANALYZE elapsed time inside Optimize/OptimizeIfNeeded instead of repeating the timing block at every call site - drop the LastDBAnalyzeAttemptAt write on success: it is only read while failures >= 1, and every failure rewrites it first - extract runPostScanAnalysis (cmd) and anyIncludedLibrary (scanner) helpers |
||
|
Kendall Garner
|
4998ac2c59
|
feat(server): add scrobble history Native API (#5761)
* initial scrobble api * feat: add scrobble retrieval api * address feedback (1) * fix spelling * be explicit about get * add primary key field, update index, remove rowid references * use unix timestamp for input and output --------- Co-authored-by: Deluan Quintão <deluan@navidrome.org> |
||
|
Deluan Quintão
|
6b9f85efcc
|
fix(subsonic): omit bit depth for lossy targets in transcode decision (#5768)
getTranscodeDecision was copying the source file's bit depth into the transcodeStream details, so a 24-bit FLAC negotiated to Opus reported audioBitdepth=24. Lossy codecs (Opus, MP3, AAC) have no PCM bit depth, and ffmpeg only honors a bit depth constraint (-sample_fmt) for lossless outputs, so the value was both meaningless and misleading to clients that use it as a quality indicator. Only set the transcoded stream's bit depth when the target format is lossless; a zero value omits audioBitdepth from the response. This also makes audioBitdepth codec limitations a no-op for lossy targets instead of rejecting the profile. Lossless targets (e.g. FLAC->FLAC downconvert) keep reporting and clamping bit depth as before. |
||
|
Deluan Quintão
|
e91687e760
|
fix(smartplaylist): reject NSP mixing top-level 'any' and 'all' (#5759)
* test(scanner): fix flaky Windows search_normalized rescan test The 'repopulates a stale search_normalized on a full rescan' spec runs two full scans back-to-back. Whether the second scan refreshes the unchanged artist depends on folderEntry.isOutdated(), which compares folder.updated_at (written during the first scan) against the second scan's library.last_scan_started_at using a strict time.Before(). Both are time.Now() values captured milliseconds apart. On Linux's fine-grained clock they are always distinct, so the test passes. On Windows the coarse wall-clock granularity frequently makes the two timestamps land in the same tick and compare equal, so Before() returns false, the folder is treated as up-to-date and skipped, the artist is never re-persisted, and search_normalized stays empty -- failing the assertion intermittently across unrelated PRs. Backdate the folder's updated_at an hour before the second scan so the comparison is unambiguous on every platform. This is a test-only timing artifact (real rescans never run milliseconds apart on an unchanged library), so no production code changes are needed. * fix(smartplaylist): reject NSP mixing top-level 'any' and 'all' A smart playlist (.nsp) that specified both a top-level "any" and a top-level "all" group was imported by silently keeping only "any" and discarding "all", regardless of key order. The Criteria model holds a single top-level Expression, so it cannot represent both groups, and the parser picked "any" without reporting the dropped rules. Make Criteria.UnmarshalJSON return an error when both keys are present at the top level, so the scanner fails loudly (logging the playlist as invalid) instead of silently losing rules. Users should nest one group inside the other, as shown in the documented examples. Fixes #5757 * fix(smartplaylist): reject top-level any+all by key presence Address code review feedback: the previous guard checked decoded slice lengths, so it only rejected the mixed top-level any/all form when both groups were non-empty. An input like {"any":[],"all":[...]} (or a null group) slipped past and silently used just one group — the same class of silent drop this change set out to prevent. Decode the two keys as json.RawMessage and detect presence by key rather than length, so any file that provides both top-level keys is rejected regardless of whether one group is empty or null. * refactor(smartplaylist): detect top-level any+all via presence type Replace the json.RawMessage + manual double-unmarshal in Criteria.UnmarshalJSON with a small optionalConjunction wrapper whose UnmarshalJSON records that its key was present. Because encoding/json invokes UnmarshalJSON even for a JSON null, this keeps the exact behavior (a present-but-empty or null group still counts, so mixing both top-level keys is rejected) while decoding in a single pass — no raw-message capture, no re-decode, no shadow variables. No behavior change; existing tests pass unchanged. |
||
|
Deluan Quintão
|
7fa13761d7
|
fix(scanner): resolve file symlinks with the production local storage FS (#5755)
* fix(scanner): resolve file symlinks with the production local storage FS The symlink classification added for GHSA-r5qr-m328-qcf4 relied on fs.ReadLink, but the local storage FS wraps os.DirFS behind the fs.FS interface, hiding its ReadLinkFS implementation. Every resolution failed at the first hop, so the scanner silently skipped ALL file symlinks, regardless of target or the FollowSymlinks setting. Libraries made of symlinks (e.g. shared-pool setups) lost all their tracks after upgrading to 0.63. The local storage now exposes full OS-level resolution (EvalSymlinks) through a new optional storage.SymlinkResolverFS interface, which the scanner prefers over the fs.ReadLink hop loop. This also classifies a chain by its FINAL target even when it passes through an audio-named intermediate outside the library, closing a bypass the hop loop had. Regular (non-symlink) entries keep the same early-return path, so scan performance is unaffected for normal libraries. Fixes #5752 * fix(test): keep watcher specs off the real local storage The watcher specs spawn watchLibrary goroutines that are not joined on spec teardown. Now that the scanner test binary registers the file:// storage, those leaked goroutines reached newLocalStorage, which reads conf.Server on construction, racing with the configtest cleanup that restores the config snapshot (caught by CI's race detector). Point the mock libraries at a fake storage scheme, which never touches the config and does not support watching, so the goroutine exits immediately. * fix(storage): reject invalid fs paths in ResolveSymlink Defense-in-depth for the SymlinkResolverFS contract: names must be valid fs.FS paths. A lexical ".." in the name would otherwise escape the library root via filepath.Join cleaning. No current caller can produce such a name (they come from ReadDir walks), but the guard enforces the documented contract at the boundary. |
||
|
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. |
||
|
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. |
||
|
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. |
||
|
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> |
||
|
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
|
||
|
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. |
||
|
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>
|
||
|
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.
|
||
|
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. |
||
|
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>
|
||
|
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) |
||
|
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 |
||
|
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>
|
||
|
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. |
||
|
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> |
||
|
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. |
||
|
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. |
||
|
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. |
||
|
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. |
||
|
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. |
||
|
Deluan
|
2a43c4683e |
chore: go fix
Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
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. |
||
|
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. |
||
|
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." |
||
|
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. |
||
|
Deluan
|
03ac02d964 |
refactor: more warnings clean up
Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
Deluan
|
efe9291db0 |
refactor: multiple syntax updates for Go 1.26
Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
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> |
||
|
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. |
||
|
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> |
||
|
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> |
||
|
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 |
||
|
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> |
||
|
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> |
||
|
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> |
||
|
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.
|