mirror of
https://github.com/navidrome/navidrome.git
synced 2026-10-09 02:47:29 +02:00
632 commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Deluan
|
9e3fc91cec |
test(artwork): separate the cover swap in time for Windows clock granularity
The quick-scan e2e swapped the cover milliseconds after the first serve; Windows' ~15ms timer granularity could collapse the stored version and the new folder timestamp into equal values, failing the staleness assertion. A 50ms gap makes the ordering deterministic on all platforms. |
||
|
Deluan
|
a98bebc314 |
fix(artwork): clamp the persisted blurhash version to the entity's
The read-side artwork version may over-approximate the served sources (folder parents for disc layouts), which with omission semantics suppresses a perfectly valid stored hash. At persist time the hash is fresh for the served bytes by construction, so the write now loads the entity and clamps blur_hash_updated_at up to its current ArtworkUpdatedAt: after any serve the DTO accepts the stored hash, and every omission window closes regardless of read-side precision. The in-memory state shrinks to a pure decode cache (checksum -> hash); staleness decisions moved to the row read, which also lets a drifted stored value be restored from the served bytes. |
||
|
Deluan
|
f0fe070ba1 |
fix(scanner): cap folder images_updated_at at scan time
A future-stamped image file (clock skew on NAS mounts) previously flowed verbatim into folder.images_updated_at and from there into the album artwork version, while the stored blur_hash_updated_at is capped at now on write. The DTO staleness gate then kept emitting the fake blurhash until wall time caught up with the file mtime. Capping at the source keeps future values out of the DB entirely; rotation is preserved because any later change is capped to a later scan time. Capping in the DTO instead would be worse: the version would become a moving target and the fake seed would rotate on every request. |
||
|
Deluan
|
9a36047096 |
fix(artwork): version the resized cache key to backfill blurhashes on upgrade
Resized entries cached by a pre-blurhash version serve straight from the cache: the resized reader never runs, the original is never read, and the tee never gets a chance to compute a hash — clients that only request sized images (Jellyfin maxwidth) would keep receiving the fake blurhash indefinitely for those items. Versioning the resized cache key makes every post-upgrade sized request miss and refill. The refill pulls the original through Get, which usually hits the cached original (original keys are unchanged), so the backfill costs one decode+re-encode per resized variant with no source or external-provider I/O. Orphaned entries are evicted by the cache's LRU as usual. |
||
|
Deluan
|
07af29fbb4 |
fix(jellyfin): fold folder image mtimes into the album blurhash version
An in-place cover-file swap followed by a quick scan updates only the folder's images_updated_at: no tracks are imported, so the album row never moves and ArtworkUpdatedAt() stayed at the old value. The Jellyfin DTO then kept emitting the previous stored blurhash, and clients that key their cover caches on it never refetched the image, so the served-bytes recompute could never run. The album select now surfaces the newest folder images_updated_at (bare-column correlated subquery over json_each(folder_ids), so the datetime decltype survives and scans as time.Time) and ArtworkUpdatedAt() folds it in. Benchmarked on a 96K track production copy: ~90us/page added, no query-plan change; the subquery is a PK point lookup per folder with at most two rows to order. Artist images and playlist sidecars have the same theoretical gap but their timestamps cannot be derived in SQL; they remain a documented follow-up. |
||
|
Deluan
|
2499de2bac |
perf(artwork): key blurhash dedup by identity, plus review cleanups
The seen map was keyed by the full ArtworkID, which embeds the client token's LastUpdate: every scan bump rotated the key, so the same-bytes dedup (and its cheap version re-persist path) never fired in production and stale entries accumulated forever. The key now zeroes LastUpdate, making the map bounded by entity count and restoring the tested dedup behavior. Cleanups from the same review: the base83 encoder is now exported from the blurhash package and the DTO's duplicate copy is deleted; the always-set blurHashes field lost its test-shaped nil guards; clearIfStored dropped its inert version parameter (cleared rows never have their timestamp read); the teeReader done flag is replaced by nilling the callback; worker-era comments (async, cache-miss recompute) were updated to the inline design; and the e2e specs assert directly instead of polling, since the hash is persisted before the read helpers return. |
||
|
Deluan
|
83e9bb8180 |
refactor(artwork): compute the blurhash inline, drop the background worker
Decode+encode is a few ms (the encoder downscales before its pixel loops), and the tee fires on Close after the response is fully written, so the hash can be computed in the serving goroutine: the queue, wake/stop lifecycle, lazy-start admin context, and the e2e worker-teardown ordering all become unnecessary and are removed. This also closes two correctness holes the queue had: a deletion signal could be dropped behind a pending serve job, and buffered image bytes had no global cap. The in-memory dedup now remembers a checksum of the served bytes plus the persisted version: identical serves skip the decode and the write, but the same bytes under a newer entity version re-persist, so blur_hash_updated_at keeps pace with row updates and the Jellyfin DTO's staleness gate keeps emitting the stored hash after scans. Placeholder-triggered clears check the seen map, then the stored row, so coverless entities cost one row read once per process instead of a probe and write per serve. UpdateBlurHash is a plain UPDATE with no user filtering, so the request context is used directly (a client abort is survived via context.WithoutCancel). |
||
|
Deluan
|
7307fd716b |
fix(artwork): close the underlying stream from the tee; rename to tee_reader
The tee was built around io.NopCloser(r) and teeCachedStream.Close only closed the tee, so the underlying CachedStream (an open cache-file fd, or the raw source stream when the image cache is disabled) was never closed — one fd leaked per teed serve, enough to exhaust the process limit during a client's initial cover sync. The tee now wraps the stream directly and its Close propagates; teeCachedStream is gone (all artwork handlers io.Copy, none Seek). The file is renamed to tee_reader.go since the wrapper is generic, not blurhash-specific. |
||
|
Deluan
|
b63c9b095b |
test(artwork): cover playlist placeholder-clear via the tee
A playlist that loses its sidecar cover serves the bundled placeholder through Get; those bytes flow through the tee, the runner recognizes the placeholder, and clears the stored hash. This exercises the free deletion path (no GetOrPlaceholder needed, since the playlist reader chain ends in fromAlbumPlaceholder). |
||
|
Deluan
|
e8fac4c335 |
feat(artwork): trigger blurhash from the served-bytes tee
Get wraps an eligible original-size serve (album/artist/playlist) in a teeCachedStream so the bytes streamed to the client are also captured; on a fully-consumed Close the runner hashes exactly what was served. GetOrPlaceholder routes a vanished album/artist cover through EnqueueClearIfGone, since no bytes flow through the tee on ErrUnavailable. capAtNow keeps a future mtime out of the stored version. The mtime-preserved cover-swap characterization test (previously pending) now passes, and the disappearing-cover e2e serves through GetOrPlaceholder to match the real Jellyfin/Subsonic path. |
||
|
Deluan
|
f6dd722119 |
refactor(artwork): compute blurhash from served bytes, drop proxy signals
The worker no longer infers whether the served bytes changed through a stack of proxy signals (snapshot timestamp vs stored blur_hash_updated_at, freshness guard, gone bit, idempotent-write skip, computeFromArtwork re-read). It now takes the exact bytes captured from a serve and is a pure function of them: placeholder clears, undecodable is left alone, otherwise encode and write with an in-memory last-hash dedup. Deletion is a checkGone job that re-reads once and clears only if the source is still gone, so a transient fetch failure can't clobber a valid hash. |
||
|
Deluan
|
2169938b30 |
feat(artwork): add teeReader to capture served artwork bytes
A wrapping io.ReadCloser that mirrors read bytes into a bounded buffer and, on a fully-consumed Close, hands the captured bytes to a callback. Partial reads and oversized streams are skipped so the callback only ever receives a complete, bounded image — the exact bytes the client received. This is the capture side of the served-bytes blurhash tee. |
||
|
Deluan
|
eb5ecabc5a |
test(artwork): characterize mtime-preserved cover swap staleness (pending)
A cover replaced in place without a mtime change is not re-hashed today: the freshness guard skips recompute when no timestamp moved, so the stored blurhash (and the client's cover cache keyed by it) stays stale. Marked pending until the served-bytes tee lands, which recomputes from the exact bytes served. |
||
|
Deluan
|
bf7ae5e82e |
fix(artwork): cap the blurhash snapshot at now to survive future mtimes
A future-dated artwork file mtime (clock skew, or a file stamped ahead of the server clock) flowed into the reader's LastUpdated snapshot and was persisted verbatim as blur_hash_updated_at. Both the worker's freshness guard and the Jellyfin DTO use a !Before comparison against that timestamp, so a later legitimate cover change whose row/mtime clock was still behind the future value would be skipped, pinning the stored hash (and the client's cached cover) until wall time caught up. Cap the snapshot at time.Now() in process() before every freshness comparison and persist, so a real later change always advances past it. Normal past mtimes (the legitimate 'snapshot exceeds row version' case) are unaffected. |
||
|
Deluan
|
5cd75503cb |
fix(artwork): backfill warm caches and supersede pending gone on refill
Two issues in the fill-triggered blurhash recompute: 1. Enqueuing was gated on a cache miss (!r.Cached). On an upgraded instance the image cache is persisted and adopted across restarts, so already-cached artwork serves as a cache hit and never enqueued, leaving its migrated-empty blur_hash as a synthetic value indefinitely. Enqueue on every original-size serve instead: the worker's freshness guard turns already-hashed rows into a cheap read and only recomputes when the hash is stale or missing. 2. Enqueue merged the gone flag with a sticky OR, so a cover restored right after a missing-art serve kept gone=true and took the clear-and-return path, never recomputing. A successful serve proves the artwork exists, so it now clears any pending gone for that artwork; a gone that follows a fill still sticks. |
||
|
Deluan
|
2cd967a456 |
fix(artwork): keep the stored blurhash on transient recompute errors
process() cleared the stored hash whenever computeFromArtwork returned an error OR an empty hash. A transient failure (the 30s context timeout, a flaky cache/DB/reader read) is not evidence the artwork changed, so clearing on it made the Jellyfin DTO fall back to a fake blurhash and churn clients' cover caches until a later successful fill restored it. Split the two outcomes: a compute error now leaves the stored hash intact and lets a later fill retry, while an empty hash (a placeholder, i.e. the cover is confirmed gone) still clears it. Deletion witnessed by a failed serve is already handled separately by the gone path. |
||
|
Deluan
|
66d6fdc1a6 |
test(artwork): stop the blurhash worker before spec TempDir cleanup
The artwork e2e suite stopped the worker via a DeferCleanup registered in
setupHarness's BeforeEach, which Ginkgo runs LAST — after a spec body's own
GinkgoT().TempDir() cleanups. With the cache disabled in these tests, every
artwork serve now enqueues a blurhash recompute, so the worker can still be
reading a spec-local sidecar file when its TempDir is removed. On Windows that
unlink fails ("the process cannot access the file because it is being used by
another process"), which flaked the playlist case-insensitive sidecar spec.
Move the worker shutdown to a suite-level AfterEach, which runs before any
spec-body DeferCleanup, so no artwork file is held open when TempDir removal
runs. Close() is idempotent, so the change is safe across specs.
|
||
|
Deluan
|
3759488db5 |
refactor(artwork): trigger blurhash recompute from the cache fill
The blurhash worker previously recomputed on every artwork serve and reconciled a row-level freshness oracle against serve-time hints (force, sourceGone, imageUpdatedAt) to decide whether the served bytes had actually changed. Every staleness bug found in review was one case where that weak oracle disagreed with the true one, and each fix imported one more serve-time fragment into it. Move the trigger to the image-cache fill instead: an original-size cache miss is exactly when the served bytes change, so the reader's LastUpdated snapshot is the truth and no reconciliation is needed. Resized fills recurse through Get(size=0) and hash the original once; a disabled cache reports every original serve as a fill, keeping real hashes available (the worker's unchanged-hash guard keeps that write-free). This deletes the force/sourceGone/imageUpdatedAt flags, the double-freshness comparison, and the entire negative cache. Only the two irreducible pieces survive: the placeholder byte-compare and the ErrUnavailable clear-hook (EnqueueGone), since deletion-without-rescan is invisible to every passive signal. Adds an e2e test for in-place cover swaps, the scenario that drove the deleted machinery, now covered structurally by the fill trigger. Schema, DTO, repositories and the blurhash encoder package are unchanged. |
||
|
Deluan
|
ae36e4dfc7 |
fix(artwork): recompute on original serves when the image cache is disabled
With ImageCacheSize=0 every serve reads live bytes, so an in-place cover swap without rescan changed the served image with no signal the worker could see. Original-size serves now force on disabled caches, and a new unchanged-hash guard skips the DB write, so the forced path costs only the background decode those installs already pay per request. |
||
|
Deluan
|
7a61776c1c |
fix(artwork): clear a fresh-looking hash when the serve finds no source
In the window before a rescan records a deleted cover (or after cache eviction/restart), no row or mtime signal moves, so the freshness skip kept the stale hash. ErrUnavailable serves now carry a sourceGone signal that bypasses the skip only while a stored hash remains to clear — afterwards the negative cache applies, so artwork-less entities still don't re-resolve per serve. The e2e spec now covers the no-rescan window. |
||
|
Deluan
|
a7eec3afc3 |
fix(artwork): clear the stored hash when the artwork source disappears
A removed cover made Get error with ErrUnavailable before the enqueue, so the worker never saw the entity and the stale hash kept being emitted. The unavailable path now enqueues too; the worker's reader signal carries the folder change and the existing no-result clearing applies. Covered by an e2e spec exercising the full disappear-and-clear flow. |
||
|
Deluan
|
0e74cf0ab1 |
fix(artwork): only force blurhash recompute on original-size cache misses
Resized cache keys vary per requested size, so a first request for a new thumbnail size (or an evicted resized entry) forced a recompute with an unchanged source — wasted work, and a transient failure during it could clear a valid stored hash. Resized readers re-fetch the original through Get, so the original-size call still carries the real change signal. |
||
|
Deluan
|
c4ca3dca5e |
fix(artwork): clear the stored hash when a recompute yields no result
A cover deleted or corrupted in place (mtime-only signal, row unchanged) left the old blur_hash in the DB, and the DTO kept emitting a hash for artwork no longer served. Since the worker only reaches compute when there is change evidence, a no-result with a stored hash now clears it, making the DTO fall back to the rotating fake. |
||
|
Deluan
|
3a8505584b |
fix(artwork): hash the cached artwork bytes, not a fresh source read
Generated playlist mosaics pick albums with random(), so a direct source read produced a different image than the cached one clients download, and the persisted blurhash described a mosaic nobody sees. The worker now reads through the image cache and detects placeholders by byte equality with the embedded assets (cached reads carry no source path). |
||
|
Deluan
|
f9f6d36ebb |
fix(artwork): TTL for no-result memoization, prompt Close, no warmup force spike
- memoize every no-result outcome (ErrUnavailable can wrap transient agent and storage failures, so it is not a reliable definitive/transient discriminator) but bound it with a 1h TTL, which also un-poisons entries recorded under future file mtimes - cancel the worker context and check done in the drain loop, so Close returns promptly instead of draining the backlog at up to 30s per item - only force recompute when the image cache is operational: during warmup every serve is a miss, which caused a recompute spike at startup |
||
|
Deluan
|
43500c0ffe |
fix(artwork): persist the image freshness signal to stop per-serve recomputes
When a cover file's mtime is newer than the entity row (common), persisting the row version made the freshness check fail on every serve, re-encoding and re-writing the same hash each time. The snapshot now stores the newest signal (row version or image mtime), and both freshness checks accept a snapshot at-or-after the row version. |
||
|
Deluan
|
b040345fb0 |
fix(artwork): stop the blurhash worker on Close to fix test data races
CI's race detector caught the process-lifetime worker goroutine outliving Ginkgo specs and touching mocks/fake filesystems being torn down. The worker now has a stop() that waits for in-flight work, exposed as Close() on the artwork service; unit suites close it up front (they don't exercise blurhash), the artwork e2e suite closes it on cleanup. |
||
|
Deluan
|
5bdeaad95e |
perf(artwork): eliminate per-pixel allocations in blurhash encoding
Normalize the downscaled image to *image.RGBA once and read Pix directly (the image.At interface boxed a color per pixel — ~16k allocs/encode), and replace per-pixel math.Pow with a 256-entry sRGB-to-linear table. ~72% faster (3.0ms -> 0.86ms for covers >=300px), 19 allocs/op. Benchmark included. |
||
|
Deluan
|
696399dab7 |
fix(artwork): don't memoize transient blurhash failures; track image freshness with cache disabled
Only ErrUnavailable (definitively no artwork) is negative-cached; timeouts and storage/agent hiccups retry on a later serve. Enqueue now carries the reader's LastUpdated so file swaps are detected even when the image cache is disabled (where every serve reads the source and miss-forcing is off). |
||
|
Deluan
|
9ea4e22ea0 |
fix(artwork): address PR review bot feedback
- don't record noResult for timed-out/cancelled computations (transient failures must not suppress retries for the same artwork version) - bound the noResult negative cache at 25k entries - use context.Background for the worker's root context - add hash:"ignore" to Artist/Playlist blurhash fields for consistency with Album (inert today; neither struct is hashed) |
||
|
Deluan
|
4d8b486dfd |
Merge remote-tracking branch 'origin/master' into feat/artwork-blurhash
# Conflicts: # model/playlist.go # model/playlist_test.go |
||
|
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
|
e100c48102 |
fix(artwork): address blurhash review findings
- force recompute on image-cache miss, so in-place cover/sidecar swaps and agent image updates refresh the stored hash even when no entity row moved - exclude external_info_updated_at from the artwork version: agent TTL refreshes bump it with an unchanged image, churning Finamp's cover cache - lazy-start the worker goroutine and bound each computation with a 30s timeout; remember no-result entities per version to avoid re-decoding placeholders on every serve - error (instead of silently skipping) on unknown artwork kinds in persist |
||
|
Deluan
|
0b365a0090 |
refactor(artwork): simplify blurhash cleanup review findings
- detect placeholder artwork via the reader's source path instead of comparing against precomputed placeholder hashes (fragile fingerprint, dead artist-placeholder branch) - collapse the three identical UpdateBlurHash repo methods into a shared sqlRepository.updateBlurHash helper |
||
|
Deluan
|
76e1fba067 | test(artwork): e2e coverage for async blurhash persistence | ||
|
Deluan
|
9f9019df07 | feat(artwork): compute and persist blurhashes asynchronously on artwork serve | ||
|
Deluan
|
470b4bdfa0 | feat(artwork): add blurhash encoder package | ||
|
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
|
||
|
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> |