Known-absent states carry an empty hash and malformed persisted hashes could panic path sharding or inject separators; Write/Open/Remove now return an error for anything but 16 lowercase hex chars.
The previous leading retry_at range column forced a temp B-tree sort of the whole eligible set on every DequeueBatch; ordering the index by (priority DESC, enqueued_at) lets scans stop after the batch size.
Write falls through to a real write when the liveness touch fails, and sweep retention now matches the recorded mime's extension so obsolete variants are reclaimed.
Duplicate ImageStore writes refresh the file mtime and Remove skips files newer than the cutoff, so overlapping acquisitions cannot lose their store files to a concurrent prune.
Sweep now honors an mtime grace window (in-flight acquisitions and temp files), reacquired orphans reset the prune grace window, and the queue mock implements real stale-absent semantics.
When AppendSubtitle or AppendAlbumVersion is enabled, the subtitle/version
tag was always wrapped in parentheses and appended to the title/album name.
If the tag value already came wrapped in brackets (e.g. "(non-explicit
version)"), the result was doubled: "Title ((non-explicit version))".
Append the tag as-is when it is already wrapped in a matching bracket pair
- (), [], {} or <> - and trim surrounding whitespace first. The shared
appendSuffix helper is used by MediaFile.FullTitle, MediaFile.FullAlbumName
and Album.FullName so all consumers behave consistently.
* feat(ui): add perPageStore helper for items-per-page persistence
* feat(ui): persist items-per-page selection in localStorage
* feat(ui): enable per-page persistence on album grid, missing files and playlist tracks
* feat(ui): restore saved album grid page size on fresh load
* feat(ui): restore saved items-per-page on list load
* fix(ui): move defaultRowsPerPageOptions to perPageStore to satisfy react-refresh lint
* fix(ui): correct defaultRowsPerPageOptions import in List and add render test
* refactor(ui): default getStoredPerPage fallback to the first option
The fallback equalled options[0] at three of four call sites; default it so
those sites stop restating it. SongList and useAlbumsPerPage still pass an
explicit fallback where it legitimately differs from the first option.
* fix(ui): persist only user-selected page sizes, validate album size against width
Persisting on every pagination context value let a URL-injected perPage (e.g.
the perPage=15 album link in NowPlayingPanel) or a forced single-option mobile
grid overwrite the saved preference. Persist only a value that is an actual
option in a multi-option selector. Also validate the album grid's redux session
value against the current width so a size chosen at a wider breakpoint can't
leave an out-of-range selector.
* fix(ui): restore saved items-per-page on the radio list
RadioList passed a hard-coded perPage that overrode List's stored seed via the
props spread, so a radio-list page size was persisted but never restored. Seed
it from storage like the other list views.
* fix(ui): persist page size only on an actual selector change
Watching the pagination context value meant any valid value persisted itself:
loading a list at a breakpoint where the saved size is invalid stored the
responsive fallback, and opening a URL with a valid ?perPage= stored that too,
either way discarding the user's real preference. Inject a wrapped setPerPage
instead, so only the rows-per-page selector writes. This also drops the
option-validation heuristics, which the new trigger makes unnecessary.
* 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.
* 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.
* feat(persistence): add AlbumRepository.GetYears for distinct album years
* test(persistence): verify GetYears de-duplicates repeated years
Regression test that adds two albums with the same non-zero max_year
(2005) and verifies that GetYears() returns that year exactly once,
ensuring the SQL DISTINCT clause is applied correctly. Catches any
future removal of DISTINCT from the GetYears query.
* feat(jellyfin): add legacy /Items/Filters endpoint (genres + years)
* feat(jellyfin): add /Studios endpoint from record label tags
* refactor(persistence): drop duplicate columns in tagRepository.GetAll
* fix(jellyfin): exclude missing albums from filter years
GetYears only filtered max_year > 0, so albums whose files were all removed
(missing=true, kept when Scanner.PurgeMissing=never) contributed stale years
to /Items/Filters. Filter them out like the normal album listings do. Also
return an empty slice from the MockAlbumRepo to match the real repository.
* feat(jellyfin): filter /Items by Years=
* feat(jellyfin): filter /Items by StudioIds= (record labels)
* feat(jellyfin): scope filter and studio lists to ParentId library
* refactor(jellyfin): extract parentIDScope and libraryScopeFilter helpers
Collapse the three inline resolveLibraryScope(dto.DecodeID(parentid)) call
sites and the duplicated empty-scope guard into two small helpers, so the
empty-scope=unrestricted contract lives in one place. Reuse the existing
names() helper in the Years= e2e test.
* feat(jellyfin): expose record labels as album Studios
Add a Studios field to the album BaseItemDto, populated from the record-label
tags and gated behind Fields=Studios (matching Jellyfin's ItemFields
convention). Studio ids reuse the record-label tag identity, so they round-trip
with the /Studios list and the StudioIds= filter. Real Jellyfin leaves Studios
empty for music; Feishin reads it as the album's record label.
The windowed-CTE backfill was compiled as a correlated scalar subquery
re-evaluated once per album (a full media_file group-by + window sort each
time), which never finished on a large library. Stage the ReplayGain-bearing
rows into an indexed temp table once and update only the affected albums:
~14s on a 727MB / 6945-album library, vs. effectively never.
* feat(model): aggregate album ReplayGain in ToAlbum
* feat(db): add nullable album ReplayGain columns with backfill
* feat(persistence): persist album ReplayGain fields
* feat(jellyfin): expose album NormalizationGain from ReplayGain
* refactor(model): lazy-init mostFrequentPtr map; clean up RG test rows
* fix(db): backfill album ReplayGain with most-frequent value, not max
A plain max() picked a minority outlier that diverged from MediaFiles.ToAlbum
(which uses the most-frequent value), and GetTouchedAlbums never re-derives an
unchanged album, so the wrong value would persist. Reproduce the modal
aggregation via grouped CTEs, which also groups media_file once instead of a
per-album correlated scan.
* refactor(model): return an owned pointer from mostFrequentPtr
Avoid aliasing a MediaFile field so the resulting Album is independent of the
source slice.
* feat(jellyfin): expose NormalizationGain from ReplayGain tags
Adds NormalizationGain and AlbumNormalizationGain to Audio BaseItemDtos,
sourced from the scanner's ReplayGain values (REPLAYGAIN_* tags, with R128_*
already converted to the same -18 LUFS reference). Same wire contract as real
Jellyfin: PascalCase keys, omitted when absent, no Fields gating. Album items
intentionally omit the field: model.Album has no gain column and Feishin reads
gain from song DTOs.
* test(jellyfin): e2e coverage for NormalizationGain fields
* test(jellyfin): drop redundant NormalizationGain passthrough spec
The JSON-casing spec already proves the mapped values (the substring
"NormalizationGain":-3.5 can only appear if the passthrough worked), so the
direct-struct spec added no coverage.
* style(jellyfin): use ASCII punctuation in gain comment
The image endpoint only served a private playlist's cover when the request
carried a token identifying its owner or an admin. But clients fetch cover
URLs without credentials — real Jellyfin's image routes are anonymous — so
every private playlist rendered the generic placeholder in Jellyfin clients
(observed in production), while the same covers displayed fine through the
always-authenticated Subsonic/native APIs.
Drop the gate and serve playlist covers like album/artist/track artwork:
playlist ids are unguessable without credentials, so anonymous access does
not meaningfully expose private playlist contents.
* refactor(req): drop redundant error return from Strings
The error from Strings carried no information beyond emptiness — it fired
exactly when the param was absent — and nearly every caller discarded it with
a blank identifier. Strings now just returns the values (empty when absent),
making the common optional-list reads one clean expression.
The few required-param callers (scrobble, createShare) check for emptiness and
return the same Subsonic error code 10 as before; their e2e tests now pin that
code. Ints and Times keep their contracts by synthesizing ErrMissingParam
themselves, so selectedMusicFolderIds is untouched. The jellyfin parseFields
helper is inlined away, since ParseFields(p.Strings("fields")...) now
compiles directly.
* docs(req): clarify Strings returns nil when param is absent
The Fields param was read with StringOr, which keeps only one value, so a
request sending it as repeated params (Fields=Genres&Fields=MediaSources) — as
Finamp and Feishin do — lost all but the first. Field-gated data like
MediaSources was then omitted for Audio items even though the client asked for
it; the comma-separated form happened to work because ParseFields splits on
commas. Real Jellyfin accepts both forms.
ParseFields is now variadic and a parseFields helper reads every repeated value
via req.Values.Strings, applied to the item list, single-item, and playlist
endpoints.
* fix(jellyfin): split Artists/ArtistItems per track artist
The Jellyfin API returned a single ArtistItems entry built from the flattened
display artist, so a multi-artist track (e.g. "De La Soul feat. Redman") lost
its individual artists and dropped every artist id but the first — while the
same track's OpenSubsonic response correctly split them. Real Jellyfin splits
Artists/ArtistItems one entry per track artist.
Build both from Participants[RoleArtist], which is already loaded on the search
and browse paths, falling back to the flattened display fields when absent.
AlbumArtists stays a single credit, matching real Jellyfin.
* fix(jellyfin): omit empty Artists in the fallback path
When a track has no participants and no display artist, the fallback set
Artists to a single-element slice holding an empty string. Only populate it
when the display artist is non-empty, so untagged tracks omit the field
instead of sending [""].
Like Subsonic's setStar/setRating, the Jellyfin favorite and rating
endpoints now broadcast a refreshResource event, so the web UI updates
immediately when a Jellyfin client changes an annotation.
Also fixes model.GetEntityByID to propagate unexpected repository errors
instead of reporting them as not-found, preserving the 500-vs-404
distinction for all its callers.
* fix(deezer): pick most-popular artist among same-name matches
The Deezer agent searched with order=RANKING and always took the
top-ranked result (artists[0]) as long as its name matched. Deezer's
RANKING order isn't reliable for homonyms, so for names shared by
several artists (e.g. "Queen") it locked onto a low-popularity artist
whose Top Tracks are empty, leaving getTopSongs empty.
Among the exact-name matches, select the one with the highest fan
count instead. This resolves "Queen" to the real band (Deezer ID 412)
and preserves the existing ErrNotFound behavior when nothing matches
the name exactly.
Fixes#5802
* fix(deezer): improve artist disambiguation by ranking exact-case names
Signed-off-by: Deluan <deluan@navidrome.org>
---------
Signed-off-by: Deluan <deluan@navidrome.org>
* feat(playlists): register starred REST filter on playlist repository
* feat(ui): add persisted sidebarPlaylistsOnlyFavourites setting
* feat(ui): playlist favourites heart column and list filter
* feat(ui): favourite heart on playlist details header
* feat(ui): sidebar favourites-only playlist toggle with live refresh
* fix(playlists): qualify id in REST filter to avoid ambiguous column
The annotation join in selectPlaylist made a bare id filter ambiguous, so
GET /api/playlist?id=X (react-admin getMany, used by the sidebar refetch and
useResourceRefresh) failed with 'ambiguous column name: id'. Register
idFilter("playlist") like the album/artist/mediafile repos.
* fix(ui): refresh favourites sidebar on local star toggle
The SSE broker skips the client that originated a star, so the acting client
never got the refreshResource echo and its favourites-only sidebar went stale.
Key the sidebar query on a fingerprint of locally-known starred playlists so a
star/unstar on this client refetches; SSE still covers other clients.
* style(ui): prettier-format PlaylistsSubMenu test
* feat(ui): refine playlist favourites layout
- Move the favourite heart column to just before the edit button
- Space the Playlists sidebar header text from its action icons
- Use a list icon instead of a cog for the playlist-management action
* feat(ui): make the playlist Favourite column toggleable
Move the heart into the toggleable fields map so users can show/hide it
from the column selector like the other optional columns, keeping it last
so it stays just before the edit button.
* refactor(ui): memoize sidebar star fingerprint and gate it on favourites-only
- Derive starFingerprint via useMemo on the playlist data reference instead
of recomputing sort/join inside useSelector on every app-wide dispatch.
- Only include the fingerprint in the query payload when favourites-only is
on, so a star toggle no longer refetches the sidebar when it shows all
playlists.
* fix(ui): don't refetch favourites sidebar on SSE events when showing all
When favourites-only is off the sidebar shows every playlist, so a star
event from another client changes nothing visible. Gate the SSE-driven
refresh counter (and its payload key) on onlyFavourites so the sidebar no
longer redraws on unrelated playlist star events.
* fix(ui): address automated review feedback on playlist favourites
- Ignore a persisted favourites-only preference when EnableFavourites is off,
so disabling the feature later can't strand a filtered sidebar (Codex P2).
- Make PlaylistLove's datagrid header props explicit (source/sortable via
defaultProps, className forwarded) instead of relying on prop pass-through.
- Add aria-label to the SubMenu secondary action button.
- Cover the PlaylistLove list column with tests (Codex P1).
* 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
Update the Jellyfin API README to cover changes that landed after it was
written: add the Lyrics and AudioMuse-AI endpoints to the implemented-endpoints
table, document the AlbumIds filter (Feishin), Recursive=false handling, and
the Jellyfin.MaxConcurrentStreams config option. Mention Symfonium as a client
of the AudioMuse-AI endpoints. Update the lyrics limitation for the
singleflighted cache loader shipped in #5792. Drop two stale known-limitation
entries: sonic similarity (plugin metadata agents already feed
InstantMix/Similar) and MD5-hash ids (the codec round-trip is symmetric, and
with the API unreleased no client can hold a raw un-encoded id).
* 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.
* fix: dedupe and cap concurrent lyrics plugin fetches
Clients like Finamp prefetch lyrics for several queue tracks at once. The
resulting burst of concurrent plugin calls can rate-limit the primary
lyrics provider into a timeout, making the plugin fall back to a lower
quality source and cache the bad result.
SimpleCache.GetWithLoader now deduplicates concurrent loads of the same
key via singleflight, with every waiter receiving the winner's result or
error. The Jellyfin lyrics loader is detached from the request context so
one cancelled request cannot fail the load for all waiters, and the
lyrics adapter caps in-flight plugin calls at 2 per plugin, queueing the
rest. As a side effect, the cached HTTP client used by the Last.fm,
Deezer and ListenBrainz agents also collapses identical concurrent
requests into a single upstream call.
* fix: harden lyrics concurrency fixes per review
Replace the stringified singleflight keys with a per-cache flight map
keyed by the cache key type itself, eliminating potential key collisions
for non-string keys, the nil-interface assertion panic, and the
stringification overhead. Release the lyrics semaphore slot via defer so
a panicking plugin call cannot leak it, and bound the detached lyrics
load with a one-minute timeout so a hung plugin cannot pin its
singleflight and semaphore slot indefinitely.
* feat(jellyfin): add LyricDto and lyrics mapper
* feat(jellyfin): advertise Lyric media stream for embedded lyrics
* feat(jellyfin): implement GET /Audio/{itemId}/Lyrics
* feat(jellyfin): advertise pipeline-resolved lyrics in PlaybackInfo
* feat(jellyfin): advertise server version 10.9.11 for client lyrics gates
* test(jellyfin): e2e coverage for lyrics endpoint and advertising
Seeds "Stairway To Heaven" with an embedded LRC lyric tag (lyrics:eng)
and covers PlaybackInfo's Lyric MediaStream, GET /Audio/{id}/Lyrics,
and the HasLyrics badge end to end.
Fixes a bug the new seed exposed: HasLyrics and the Lyric MediaStream
gate compared mf.Lyrics against "", but the persistence layer never
stores an empty string post-scan (it normalizes to the JSON sentinel
"[]"), so every track was reporting HasLyrics=true. Both call sites
now parse the column via StructuredLyrics()/LyricList.Main() instead.
* fix(jellyfin): cheap sentinel check for embedded lyrics advertising
* chore(jellyfin): trim over-budget comments in lyrics code
* test(jellyfin): cover lyrics pipeline error and nil-start cue skip
* docs(jellyfin): document lyrics support and follow-ups in README
* refactor(jellyfin): promote embedded-lyrics sentinel check to MediaFile
The "[]" no-lyrics sentinel is persistence-layer knowledge; expose it as
model.MediaFile.HasEmbeddedLyrics() instead of a dto-local helper. Also
dedupe the test lyrics-cache construction and pre-size the media stream
slice.
* refactor(jellyfin): consolidate tick conversions around one constant
ticksPerMillis is now the single source of the 100ns-tick unit; the
scrobble handlers' three inline /10_000 divisions become
dto.MillisFromTicks.
* fix(jellyfin): align lyric advertising with the serving predicate
PlaybackInfo advertised on any non-empty LyricList while the endpoint
404s when the main lyric has no lines; both now share servableLyric.
Handler tests also send hex-encoded ids to match real traffic.
* chore(jellyfin): drop unneeded lyrics package alias in e2e suite
Wire up GET /System/Info returning the previously unused dto.SystemInfo,
available to any authenticated user, matching real Jellyfin's authorization
(FirstTimeSetupOrIgnoreParentalControl, not admin-only). Feishin calls this
endpoint on connect and reads Version to feature-gate; it previously got the
unhandled-route 404.
The advertised version stays 10.8.13: Feishin unlocks structured lyrics and
public-playlist share permissions at >=10.9.0, and this API serves neither
(no lyrics endpoint; playlist user permissions are stubs), so a higher
version would falsely advertise capabilities.
Feishin fetches an album's tracks with AlbumIds=<id>&IncludeItemTypes=Audio&Recursive=true, but the /Items handler never read the AlbumIds parameter, so the request degenerated into the entire library sorted by album: every track (with MediaSources, when requested) was counted and streamed for what should be one album's worth of songs — very slow on large libraries, and wrong results. Parse AlbumIds like GenreIds (comma-separated and repeated spellings, hex-decoded) and filter songs through a new filter.ByAlbumID helper, keeping the album_id column knowledge in the filter package.
Finamp's Radio Mix requests /Items/{id}/InstantMix?limit=250, but getInstantMix
clamped the limit to maxSimilarLimit (100), so the queue was cut to 100 tracks.
A mix is a playback queue, not a "related items" list, so it gets its own
ceiling instead of sharing the Similar one. The Similar handlers keep 100.
Verified against a live library: the sonic provider supplies all 250 tracks for
a seed that previously returned 100.
* 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.
* fix(jellyfin): honor Recursive=false for library parents
/Items ignored the Recursive parameter entirely — it appeared only in tests,
never in production code. Finamp's download sync sends exactly one
Recursive=false request per library (ParentId=<library>&IncludeItemTypes=Audio)
to pick up tracks outside any album, supplementing its recursive per-album
fetch. We answered with every song in the library: 208MB and 21s per sync
measured on a real library, against 20KB for the album queries. Finamp then
required every track twice, once via its album and once under the library node.
In Jellyfin 10.10 Recursive never reaches SQL. It selects between an in-memory
walk of a folder's direct Children (false) and a DB ancestor query (true), with
IncludeItemTypes applied as a post-filter over that direct-child list
(ItemsController.cs:308, Folder.cs:949-994). The default is false, and neither
IncludeItemTypes nor SearchTerm forces recursion, so Recursive=false with
IncludeItemTypes=Audio on a music library returns an empty list — which is what
Finamp expects and codes for.
Filter the requested types to those nested directly under a library when the
parent is a library and Recursive is not true; the hierarchy this API exposes is
library -> album -> track, so no track is ever a library's direct child. Album
and playlist parents are untouched, as their tracks are real direct children in
Jellyfin (MusicAlbum.cs:86-89, Playlist.cs:140-167) and Jellify opens playlists
with Recursive=false. A library parent is the only case handled: /Items with no
ParentId still returns every song where Jellyfin returns the root's children,
but no observed client sends that, and honoring it would surprise any client
that simply omits Recursive.
* refactor(jellyfin): narrow the Recursive=false filter to Audio
Replace the libraryChildTypes allowlist with a direct Audio check. The two are
behaviorally identical — parseTypes only ever yields Audio, MusicArtist,
MusicAlbum, MusicGenre or Playlist, and the allowlist held the latter four, so
it excluded exactly Audio and nothing else.
The allowlist claimed those four are a library's direct children. That isn't
true of MusicGenre or Playlist: listGenres is deliberately unscoped because
genres are global tags, and listPlaylists ignores scopeIDs entirely, so neither
is nested under a library at all. They were on the list only to leave their
behavior untouched. The one verified invariant is that no track is a library's
direct child, so state just that — it is also more conservative, as a type added
later keeps its current behavior instead of being filtered by a stale list.
Add a test pinning the omitted-Recursive default: ItemsController binds
`bool? recursive` and reads it as `recursive ?? false`, so omitting the param is
a non-recursive request and filters Audio for a library parent.
* 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.
* fix(jellyfin): stream /Items responses to prevent OOM on large libraries
Finamp's library sync issues an unbounded GET /Items?IncludeItemTypes=Audio
with Fields=MediaSources and no Limit. On a large library this built the whole
result set — every MediaFile and every BaseItemDto, each fat with MediaSources —
in memory, and json.Encoder then buffered the entire ~200MB response before
writing a byte. Measured on a 96k-track library, one such request peaked near
1.6GB RSS; in a memory-limited container the resulting slowness made clients
retry, stacking concurrent full-library builds until the process was OOM-killed.
Stream the song listing straight from a DB cursor (MediaFileRepository.GetCursor),
mapping and encoding one row at a time, so peak memory is bounded to about one
item regardless of library size. The cursor is opened lazily, when streaming
begins, so it doesn't hold a DB connection across the CountAll and ServerId
lookups that run first (ServerId can write on first use and would deadlock
against an open reader). Pagination still works — the cursor query carries
LIMIT/OFFSET/ORDER BY from StartIndex/Limit/SortBy — and TotalRecordCount stays
the full count. Other materialized responses stream their JSON too, avoiding the
encoder's buffer-the-whole-output cost.
Also bound artwork image concurrency with the same server.ThrottleBacklog that
Subsonic's getCoverArt uses, so a burst of image requests during sync can't
exhaust memory through concurrent decode/resize.
Verified against a copy of a 96k-track production database: byte-for-byte
identical response, peak RSS 1593MB -> 53MB, time-to-first-byte 8.1s -> 0.2s.
* fix(jellyfin): fail loudly on /Items streaming errors instead of a truncated 200
Addresses code review: a streamed /Items response commits HTTP 200 before an
error can occur, so failures were surfacing as misleadingly successful bodies.
- A cursor-open failure (e.g. a busy DB under connection pressure) is now
surfaced before the first byte is written: the cursor open is deferred into
the item source and run in writeItems after the ServerId lookup but before the
envelope, returning a clean 500 the client can retry — not a 200 with an empty
item list.
- A mid-stream (row-scan) error now aborts without closing the JSON envelope,
leaving the body malformed. A truncated-but-valid response would let a sync
client (Finamp) treat the short list as the whole library and prune local
tracks; malformed JSON forces the client's parser to fail and retry.
* refactor(jellyfin): stream every collection endpoint from a DB cursor
Streaming was applied only to the song listing, which left two write paths, two
ServerId stamping mechanisms and a listXxx return-type split ("songs is
special") that made the code hard to follow.
Add GetCursor to the album, artist, genre and playlist repositories, mirroring
MediaFileRepository.GetCursor: same select builder as GetAll, so each cursor
yields identical, fully-hydrated rows (hydration happens per-row in PostScan,
and toModels is only a deref loop). The playlist cursor keeps GetAll's
owner/public visibility filter. Each repository test asserts the cursor yields
exactly what GetAll returns, including Max/Offset.
With cursors everywhere, every listXxx returns itemsResult and every collection
streams through one writer:
- listAlbums, listArtists and listPlaylists now stream from their cursors;
search paths stay materialized (Search returns a slice).
- listGenres stays materialized: its total is the length of the full list and it
paginates in memory, so there is nothing for a cursor to page over.
- api.ok's QueryResult case delegates to writeItems, so the ~9 inline callers
funnel into the same writer; streamQueryResult and wrapResult are gone.
- /Items/Latest streams as a bare JSON array (its Jellyfin wire shape) via
writeItemsArray, which shares the item loop and stamping. That also closes the
same unbounded hole /Items had: limit=0 disabled its LIMIT.
- ServerId is now stamped in exactly one place, so api.ok only handles single,
non-collection payloads.
Verified against a copy of a 96k-track production database: every endpoint
byte-for-byte identical to master. Unbounded /Items peak RSS 1691MB -> 54MB;
time-to-first-byte 6.3s -> 0.19s, /Artists 1.14s -> 0.13s.
* refactor(jellyfin): route every response through api.ok
Handlers were split between api.ok and api.writeItems with no clear rule for
which to call. api.ok now accepts itemsResult too, so it is the single entry
point: callers hand it whatever they have and it routes collections (cursor
-backed or materialized) to the streaming writer. writeItems is reached only
through api.ok now. The one exception is /Items/Latest, which returns a bare
JSON array rather than a QueryResult envelope and so writes directly — noted in
both doc comments.
Also drop GenreRepository.GetCursor: listGenres derives its total from the
length of the full list and paginates in memory, so there is nothing for a
cursor to page over, leaving the method unused.
* fix(jellyfin): stream the unbounded multi-type /Items merge
The multi-type merge capped each per-type query at offset+limit only when a
Limit was given. Without one (Finamp's favorites screen sends multi-type), every
type ran an unbounded query and collect() drained each cursor into a slice — so
/Items?IncludeItemTypes=MusicAlbum,Audio with no Limit materialized every album
and every song, the same OOM class this branch set out to fix. The comment
claiming the set was "capped at offset+limit per type" was wrong for limit=0.
Without a limit the merged page is just each type's rows in order minus the
first offset, which is exactly what chaining the per-type cursors yields, so
stream that instead of merging in memory. The bounded path is unchanged: with a
Limit each type holds at most offset+limit rows, so merging and paginating
across the combined list is safe. Cursors are opened one at a time (each pins a
DB connection until drained), with the first opened eagerly so the usual failure
is still a clean error before any byte is written.
Measured on a 96k-track library, /Items?IncludeItemTypes=MusicAlbum,Audio with
no Limit (103,393 items): peak RSS 612MB -> 53MB, time-to-first-byte 4.9s ->
0.6s, response byte-for-byte identical.
* refactor(jellyfin): parse /Items params into a struct
The /Items dispatcher was a single 90-line block that parsed a dozen params and
then threaded them positionally through three layers — queryItemsOfType took 11
arguments, listSongs and listAlbums 9 each — so reading any one of them meant
decoding a long argument list.
Parse once into an itemsQuery and pass that instead. queryItems now reads as the
four things it actually does: parse, the id/playlists-folder/playlist-parent
special cases, single-type dispatch, multi-type merge — with the playlist-parent
and merge bodies moved to playlistTracks and mergeTypes. Every listXxx takes
(ctx, opts, q).
No behavior change: parsing, ordering and the entityParent rule are unchanged,
and /Artists still passes favOnly=false explicitly by building the subset of the
query it uses.
* docs(jellyfin): trim comments on the streaming path
Cut the comments back to the non-obvious why: the deferred cursor open (the
ServerId write would deadlock against an open reader), the deliberate malformed
JSON on a mid-stream error, why chained opens one cursor at a time, why
listGenres stays materialized, and why the cursor helpers take the underlying
func type. Dropped the rest — restatements of the code, doc comments that
repeated a test's own name, and the GetCursor interface comments that the
existing MediaFileRepository.GetCursor does without.
* refactor(persistence): fold the cursor wrappers into a generic wrapCursor
wrapAlbumCursor, wrapArtistCursor, wrapMediaFileCursor and wrapFolderCursor were
the same twelve lines four times over, differing only in the type name, and the
playlist cursor had its own inline copy of the loop.
Add one generic wrapCursor. It takes an extractor func rather than a method on
an interface: a type parameter can't reach an embedded field, and methods that
only satisfy a generic constraint are reported by the unused linter. The
extractor also lets both type parameters be inferred, so call sites need no
explicit instantiation. Each entity keeps its named wrapper as a one-liner,
since the model cursor types are defined types and need the conversion — and the
existing wrapper tests call them directly.
The nil-row error now names the model type via %T ("unexpected nil model.Album")
instead of a hand-written per-entity string; the three tests asserting that
message are updated.
* fix(jellyfin): bound concurrent collection streams
A streamed collection holds a DB cursor — and its pooled connection — for the whole client-paced
response, where the old materialize-then-write path released it as soon as the query finished. The
pool is shared with the scanner, Subsonic, the native API and the UI, so enough slow clients take
every connection and everything else blocks waiting for one. Measured against a copy of a 96k-track
production DB, 20 concurrent unbounded streams against a pool of 16 stalled a write for 16.2s (the
other 14 writes in the run took ~2ms — the signature of connection starvation, not lock contention).
Cap concurrent streams at half the pool. Excess requests queue rather than fail, so no client is
rejected: the same 20 streams now all complete and the worst write is 2.5ms.
- conf.MaxOpenConns() now owns the pool sizing (db calls it). It belongs in conf: the pool is a
tunable, db already imports conf, and putting it in db would force server/jellyfin to import db
just to size the cap against it. Expressing the cap as MaxOpenConns()/2 also keeps the two from
drifting apart.
- Uses chi's ThrottleBacklog, not server.ThrottleBacklog: the latter buffers the whole response to
release its token early, which is right for artwork but would undo the streaming. chi's panics on a
non-positive limit, so throttleStreams guards it — setting MaxConcurrentStreams=0 disables the cap
instead of crashing the server at startup.
* refactor(jellyfin): move throttleStreams to middlewares.go
It's a middleware, so it belongs beside normalizeQueryKeys, authenticate and
withPlayer rather than in api.go. Its tests move to middlewares_test.go with it,
keeping one test file per production file.
* fix(jellyfin): abandon the scan when the client stops reading
encodeItems discarded its write errors and relied on the final Flush to report
them, so once bufio's buffer filled and the flush failed, the loop still pulled
every remaining row through the cursor and serialized it for a client that was
gone. A test with a failing writer confirms it: all 20k items were drained.
That wastes CPU, and holds the cursor's pooled DB connection and a stream slot
(now a capped resource) for the length of a full scan nobody is reading. Check
the per-item writes so the first failure ends the scan; the fixed envelope
writes stay unchecked, since bufio latches for them anyway.
