612 Commits

Author	SHA1	Message	Date
Deluan Quintão	95615bcb18	fix(artwork): serve images whose format has no registered decoder (#5952) ... * fix(artwork): serve images whose format has no registered decoder The new pipeline derives dimensions, mime and the placeholder hashes at resolution time, so a decode became a precondition for recording artwork at all. An image in a format Go has no decoder for therefore failed acquisition, retried until the 12h budget ran out, and then settled as absent, serving a placeholder from that point on. The old pipeline decoded only to resize and fell back to the original bytes when that failed, so these covers used to work. A local file is picked by matching an image extension, so bytes it cannot decode are most likely a codec we lack: image.ErrFormat on a folder, upload or embedded source now yields an Artwork row carrying just the hash and mime, and the bytes stay servable. An external response carries no such guarantee, so it still fails and retries rather than pinning a non-image body as a cover. A corrupt image of a known format and an over-cap declared size still fail, so the decompression bomb guard is unchanged. Absent rows recorded by earlier builds are re-resolved by the existing stale-absent recheck within a day, so no epoch bump is needed to repair them. Reusing a stored image now re-decodes when it carries no dimensions, so a row recorded while a decoder was missing can still be upgraded later. Registers jxl, heic and heif in mime_types.yaml: image detection resolves the extension through the host MIME table, and the Alpine release image ships no /etc/mime.types, so those covers were never recorded in folder.ImageFiles there and never reached the pipeline at all. * fix(artwork): never record empty bytes as artwork image.DecodeConfig returns image.ErrFormat for an empty payload just as it does for a codec with no registered decoder, so a zero-byte cover file was recorded as found artwork and served as an empty response instead of falling back to the placeholder. A truncated image of a known format already fails with unexpected EOF rather than ErrFormat, so only the empty case needed the guard.	2026-08-14 09:43:13 -04:00
Deluan Quintão	59a4ed8e79	fix(plugins): stop reporting plugin call failures as not-found (#5953) ... * fix(plugins): stop reporting plugin call failures as not-found MetadataAgent joined agents.ErrNotFound onto every failed plugin call, so a transport fault was indistinguishable from a definitive miss. The artwork circuit breaker treats a not-found as a successful, definitive answer and resets its failure counter, so it never opened for a failing plugin and kept calling it on every request. Observed with the apple-music plugin against prod: ~900 iTunes 429s in 27 minutes with the breaker never tripping. Return the underlying error instead. The genuine empty-result branches still return agents.ErrNotFound, and agent fallback is unaffected because callAgentMethod/callAgentSliceMethod continue on any error, not only on ErrNotFound. * test(plugins): fold duplicate metadata agent error specs into one table The error-handling container drove all 11 MetadataAgent methods twice: once to assert the message, once to assert the failure is not an ErrNotFound. The argument lists were identical, so each method cost two WASM instantiations for one method's worth of coverage, and a new capability had to be registered in two places to stay guarded. Fold both assertions into a single DescribeTable, document the ErrNotFound contract at the sentinel where agent implementers will read it, and collapse breaker.record's hand-inlined predicate onto isTransientExternal, which it already duplicated by hand with a keep-in-sync comment. * fix(plugins): keep an unimplemented plugin method a definitive miss Returning the raw plugin error made errNotImplemented and errFunctionNotFound look like provider faults. Every MetadataAgent satisfies ArtistImageRetriever and AlbumImageRetriever regardless of what the plugin actually exports, so artwork resolution calls those stubs on a partially-implemented plugin: each call counted toward the artwork circuit breaker and kept the item in the retry queue instead of settling it absent. Map both sentinels back onto agents.ErrNotFound, joined so the underlying reason survives for diagnostics, and leave real call failures untouched. This matches what ScrobblerPlugin already does for the same two sentinels. The partial-implementation specs asserted only MatchError(errNotImplemented), which the previous errors.Join satisfied incidentally, so nothing caught the lost not-found semantics. They now assert both and are folded into one table. * test(plugins): cover the missing-export arm of agentErr The partial-metadata-agent fixture registers through the Go PDK, which exports every method and answers with the not-implemented code, so no fixture reaches the errFunctionNotFound branch. Building one would mean hand-writing Extism exports to deliberately omit a function, which tests the manager's function lookup rather than the mapping this PR added. Cover agentErr directly instead: both sentinels classify as a definitive miss, a call failure and a non-zero exit stay faults, and the underlying reason survives in every case. * fix(artwork): stop counting a cancelled run against the circuit breaker callPluginFunction returns ctx.Err() when a plugin call is cancelled, and that reached breaker.record as an ordinary error, so cancellations counted toward the five consecutive failures that open a gate. A cancellation says nothing about the provider, so it now neither counts nor clears the failure run. Deliberately scoped to breaker.record rather than isTransientExternal: the latter also drives whether the queue item is rescheduled, and a cancelled item must still be retried rather than settling absent. context.DeadlineExceeded is left counting as a fault, since a provider that blows the budget is one worth backing off from. Reachable today only at shutdown, where the in-memory breaker state is discarded anyway. It becomes live the moment Worker.gate is used on a request-scoped context, which is why it is worth closing now.	2026-08-13 22:56:09 -04:00
Deluan Quintão	757ca783d3	fix(instant-mix): top short mixes up instead of returning what the first source found (#5951) ... * fix(instant-mix): top short mixes up instead of returning what the first source found SimilarSongs returned the first non-empty source's tracks, however few. For a thinly-represented artist that meant a 3-track mix no matter the requested count: the artist agent found nothing, the similar-artists fallback matched 3 library tracks, and `len(res) > 0` kept seed-track sampling from ever running. Clients treat that as a failed mix and retry with a bigger limit forever. Finamp cycles limit 34 through 472 and starts over, ~1 request every 2s indefinitely, each one re-hitting Last.fm, Deezer and AudioMuse. Sources are now chained rather than raced: each one tops the mix up until it holds count tracks, so the agent's picks, the similar-artists fallback and seed-track sampling all contribute instead of the first one winning outright. * refactor(external): move similar-songs code to its own file provider.go held two distinct concerns: artist/album external metadata and the similar-songs mix pipeline. The mix code was already one contiguous block, and maxSeeds, maxSimilarSongs and dedupByID were used by nothing else. Moved SimilarSongs and its helpers to provider_similarsongs.go, matching the existing provider_similarsongs_test.go. Pure code motion: the moved block is byte-for-byte unchanged and provider.go has no additions, only deletions. * fix(instant-mix): dedup before deciding a mix is full topUp measured res before deduplicating it. Matcher.MatchSongs deliberately re-emits a library track when the same input song repeats, and the similar-artists fallback can reach one track through several artists, so len(res) could equal count while holding fewer unique tracks. That returned a mix with duplicates in it and stopped the top-up early; the caller then deduplicated and handed back a short mix, which is the client retry loop this branch set out to fix. Deduplicate first, so the length check counts what the client will actually receive. * perf(instant-mix): skip a fallback once the mix is already full The artist path nested one topUp inside another, so the inner one measured only similarSongsFallback's own result against the full count. With 49 agent matches and one fallback match for count=50 the mix was already full, yet seed-track sampling still ran and fired up to five GetSimilarSongsByTrack calls whose results the outer topUp then truncated away. topUp now takes the sources as a variadic list and re-checks the accumulated mix before each one, so a later, costlier source only runs while the mix is still short. That also flattens the artist case: the agent, the similar-artists fallback and seed-track sampling are now three peers in one chain instead of two nested calls. * fix(instant-mix): count distinct tracks when picking the fallback mix similarSongsFallback stopped after count picks from the weighted chooser, but a track can sit in that chooser once per artist listing it in their top songs, and Pick removes the entry it returns. Repeats therefore consumed pick slots and left unique candidates stranded, so the batch could come back short of count. On the track path this is the only source, so that short mix reached the client and kept the retry loop alive. Track the ids already picked and keep drawing until count distinct tracks are held or the chooser is empty. * fix(instant-mix): match the whole agent response before trimming MatchSongs was capped at count, and it re-emits a track when the same song repeats, so [A, A, B] with count=2 returned [A, A] and never reached B. topUp then shrank that to [A] and, with an empty or overlapping fallback, the mix stayed short even though B had been available all along. seedMix already matched its full merged set for this reason; mixFromAgent now does the same and leaves the trim to topUp. Also drop the capacity hint on the picked-ids map. It was sized from the caller's count, which CodeQL flags as an allocation sized by user input (go/uncontrolled- allocation-size). SimilarSongs clamps count to maxSimilarSongs long before this point, so the hint bought nothing worth the alert. * refactor(instant-mix): tidy the mix chain and its specs Quality pass over the new code, no behaviour change: - topUp: drop the first-vs-last error bookkeeping (the value is only read when the mix is empty, so the distinction is unobservable) and the redundant nil guard (dedupByID returns nil for an empty result, so both branches already agreed). - mixFromAgent: assign through the if-scoped err instead of a second error name. - Hoist the similar-artists fallback closure written verbatim in two switch arms. - Use map[string]struct{} in the pick loop, matching dedupByID in the same file. - Trim three comments back within budget; two restated the line below them and one carried commit-message rationale. - Tests: add an ids() helper for the ID assertion repeated seven times, and fold the track-entity stub block copied into three specs into stubTrackEntity. The block hard-coded .Twice() on GetEntityByID, which pinned an implementation detail no spec asserts. * revert(instant-mix): inline the similar-artists fallback closure again Hoisting it to a shared artistFallback var moved the call away from the arm that uses it and saved nothing: each arm reads better spelling out its own sources.	2026-08-13 18:50:34 -04:00
Deluan Quintão	6c3e7e268b	feat(instant-mix): support album, playlist and genre sources (#5948) ... * feat(agents): local agent genre-hint similar songs fallback * feat(external): playlist instant mix via seed-track sampling * test(external): cover playlist mix never-empty fallback and maxSeeds cap Adds coverage for the empty-match seed fallback and the maxSeeds call cap on GetSimilarSongsByTrack, per code review finding. * feat(external): genre instant mix via seed-track sampling * feat(external): album instant mix falls back to AudioMuse track similarity * feat(external): artist instant mix falls back to seed-track sampling * fix(jellyfin): route genre seeds through instant mix instead of empty * feat(jellyfin): add /Albums/{id}/Similar route for albumMix radio * perf(external): bound playlist seed sampling to a random N samplePlaylistTracks loaded an entire playlist's joined rows just to keep 5 random seeds; push the bound and randomization into the query instead, matching the other samplers (GetRandom/GetAllByTags with Max). Fixing this surfaced a real bug: resetSeededRandom's SEEDEDRAND rewrite assumed every table's id is TEXT, but playlist_tracks.id is an INTEGER position, so the random sort silently dropped every row. Cast the id to TEXT before hashing (no-op for the other, TEXT-id tables). Also trims a changelog-flavored comment and a duplicated rationale in server/jellyfin/similar_test.go. * refactor(external): parallelize seed mix and dedup mix helpers Run the up-to-5 per-seed GetSimilarSongsByTrack calls concurrently (errgroup), route the four container cases through a shared seedMix helper, flatten the genre lookup, and sample playlist seeds without forcing a smart-playlist rebuild. Share the media-file->Song mapping in the local agent. * perf(agents): use the indexed genre filter for local similarity Replace GetAllByTags (a json_tree scan of every media_file row) with the media_file_tags semi-join from #5940, deriving the seed's genre tag ids locally since they hash from (name, value). Also carry the library id and the recording MBID on the returned songs: the matcher resolves by id first and looks up mbz_recording_id, so the release-track id it got before matched nothing and the local fallback silently returned no songs. * refactor: drop redundant MBID and fold mixFromSeeds into seedMix The local agent returns library tracks, so the id alone resolves them in the matcher's first phase; the MBID was never consulted. mixFromSeeds had no caller other than seedMix. * docs: trim redundant comments * fix(jellyfin): adopt the GUID id codec in the merged similar routes getSimilarAlbums still used resolveItemID/DecodeID, which #5942 replaced with itemIDParam; its tests passed raw ids that the strict codec now rejects. * fix(external): guard non-positive counts and blend every seed A negative Subsonic count reached matched[:count] and panicked. The matcher also keeps input order and stops at count, so seed-grouped results let the first seed fill the whole mix; interleaving gives every seed a share. Drops the duplicate playlist-track mock in favour of tests.MockPlaylistTrackRepo, which pages like the real repository and records the query options. * fix(external): refresh smart playlists before sampling seeds A smart playlist materializes no playlist_tracks until it is evaluated, so sampling without the refresh mixed an empty seed set. The refresh is a no-op for regular playlists, inside the refresh delay, and for non-owners. * fix(external): skip missing tracks and a nil playlist-track repo when sampling Tracks() logs and returns a nil repository when its own lookup fails, so the chained GetAll panicked. Seeds can also reach the mix verbatim when the agents find nothing, so a missing file would surface as an unplayable entry. * fix(jellyfin): never report the seed album as its own similar album The sampled-seed fallback returns the album's own tracks, which similarAlbums mapped straight back to the requested album, often as the only result. * test(agents): assert the genre predicate instead of relying on the mock MockMediaFileRepo ignores QueryOptions.Filters, so the spec passed even with no genre filter at all. It now checks the generated predicate carries the seed's own tag id, the indexed join and the missing exclusion. * fix(external): clamp the requested count before it becomes a query limit Subsonic passes the client's count through unbounded. At MaxInt64 the local agent's count+1 overflows negative, and GetRandom omits the SQL limit unless Max is positive, so one request would hydrate every matching track. 500 is what the widest caller (similarAlbums, limit*5) legitimately asks for. * fix(external): deduplicate playlist seeds by media file A playlist can hold the same file at several positions, so sampling its rows could seed the mix twice: a wasted agent call, and a duplicate track whenever the seed fallback kicks in. * fix(external): drop tracks two seeds both recommend The matcher re-emits a track when two inputs are identical, so overlapping recommendations took several slots in the mix. Match the whole merged set and dedup before trimming. Playlist sampling now over-fetches before its own dedup, so repeated positions cannot collapse the seed count. * test(external): make the seed-blend assertion independent of the shuffle It matched four tracks and kept two at random, so both could come from the first seed once in six runs. Keeping three of the four makes a seed-two track unavoidable. * fix(external): seed artist mixes from every credited role media_file.artist_id is the deprecated primary artist, so an artist credited only on the album, as on compilations, sampled no seeds at all. Use the same participant filter the artist listings use. * refactor(external): drop the now-vestigial seed interleaving Matching the whole merged set removed the early truncation the interleave guarded against, and the shuffle before the trim makes input order irrelevant. Its comment described the old behaviour. * test(agents): give the id-mapping fixture a matching genre The related track carried no genre, so the real query would never return it; the spec only passed because the mock ignores QueryOptions.Filters. * test(agents): drop the MBID from the id-mapping fixture Local agent candidates are non-missing library rows, so the matcher always resolves them in its id phase and never reads the MBID. The field guarded a regression that could not change behaviour. * test(agents): remove unnecessary comment about MBID in GetArtistTopSongs test * fix(jellyfin): only let a not-found entity fall through in getInstantMix Discarding the error conflated a genre id, which never resolves, with a real lookup failure, which then made a provider call that fails the same way. * test: pin the invariants the specs only appeared to cover The missing filter was asserted by substring, so flipping it to true passed everywhere, including the spec named for it. Matching the whole merged set, the local agent's over-fetch, and its no-genres early return had no coverage at all; each is now pinned by a spec that fails when the code is broken. * test: make the remaining specs say what they actually guard The playlist-track spec named a sort whitelist it does not exercise; it guards the integer-id CAST, so it now asserts no rows are dropped. The maxSeeds cap passed with either bound removed, and the over-fetch was pinned by its literal value rather than the duplicate positions it exists for. Also drops setup the count guard returns before reaching. * fix(external): fall back when the agent's picks are not in this library A non-empty answer whose songs are all absent locally matched nothing and was returned as-is, so the mix came back empty with sampleable source tracks sitting right there. * refactor(external): name the agent-then-fallback flow once Each entity case repeated the same error and emptiness plumbing around the matcher. mixFromAgent states it once and each case supplies only what differs: how to ask, and what to do when the answer is unusable.	2026-08-12 23:02:13 -04:00
Deluan Quintão	752b38609c	feat(artist): add Share and Download actions to the Artist detail page (#5944) ... * feat(ui): add Share button to artist detail page * feat(ui): add Download button to artist detail page * fix(ui): scope artist share/download to album-artist content Gate the artist Share/Download actions on album-artist stats and show the album-artist size, since ZipArtist and the share query only cover album_artist_id songs. Previously the total (role-inclusive) size was shown and guest-only artists could produce an empty archive. Applies to the artist toolbar, the shared context menu, and the download dialog title. * fix: match artist download/share to album-artist participation ZipArtist and the artist share query filtered the deprecated album_artist_id column, which only stores the first album artist of a track. Secondary album-artists (co-credited but not first) got an empty download/share even though the UI offered it. Filter by the album-artist role participation instead, matching the artist's album-artist stats used to gate the actions. Also cover the artist-specific size branch of the download dialog. * fix(share): scope artist shares to the owner's libraries The artist share query broadened to album-artist participation, which could pull a secondary album artist's tracks from libraries the (non-admin) share owner cannot access into the public share. Load the artist share as the owner so their library access is applied, mirroring how playlist shares already work. Adds a repository test covering co-album-artist inclusion and library scoping. * test(share): assert album participation branch of artist shares Link the co-album-artist fixtures to albums and assert share.Albums (used by Subsonic getShares) includes the accessible album and excludes the one in a library the owner cannot access, so the album participation + scoping branch is covered too. * fix: exclude missing files from artist download/share actions An artist's stats still count files that went missing, so the toolbar/context menu could offer Download/Share for an artist whose files are all gone, while the share query (missing=false) returns nothing and downloads open dead paths. Hide the actions when the artist is missing and exclude missing files from ZipArtist, matching the share semantics. * refactor: dedupe artist download-size and share-owner lookups Extract the 'album-artist download size (or none when missing)' rule into a single artistDownloadSize() helper shared by the toolbar, context menu, and download dialog, and factor the duplicated share-owner context lookup into a shareRepository.ownerContext() method used by both the artist and playlist share cases. * refactor(ui): move artistDownloadSize helper to common utils is for domain-agnostic, potentially portable code; this helper is Navidrome-specific (artist stats shape), so it belongs in common. Consumers import it directly from common/artist to avoid pulling in the common barrel.	2026-08-12 11:59:56 -04:00
Deluan	8978c7b9fa	Revert "feat(jellyfin): send a synthetic placeholder blurhash for unresolved artwork (#5941)" ... This reverts commit 036c9cab9671505c83fce6523f55d97b152b3b32.	2026-08-12 11:46:23 -04:00
Deluan Quintão	036c9cab96	feat(jellyfin): send a synthetic placeholder blurhash for unresolved artwork (#5941) ... * refactor(artwork): let callers pin the blurhash component counts * feat(artwork): synthesize a unique placeholder blurhash from a seed * fix(artwork): base the synthetic-hash no-collision guarantee on the prefix, not length The prior comment and test claimed a synthetic value could never collide with a real one because it's always shorter. That's false: components() targets ~16 tiles by scaling one axis down as the other hits the 9 cap, so an extreme aspect ratio (e.g. 10x200) collapses to 1x9 = 9 components, which encodes to the same 22 characters as a 3x3 synthetic hash. The old test only exercised a square 64x64 gradient, so it never caught this. The real guarantee is structural, not length-based: the first character encodes shape as (xComp-1)+(yComp-1)*9, and components() derives xf*yf = 16 exactly before flooring/capping (xf = sqrt(16w/h), yf = xf*h/w = sqrt(16h/w), so xf*yf = sqrt(256) = 16). Two factors both in [2,3) can't multiply to 16, so Encode can never derive 3x3 - the synthetic prefix 'K' is structurally exclusive to Synthetic. Replaced the length-based test with one that sweeps extreme aspect ratios and asserts no real encode ever produces prefix 'K', alongside the assertion that Synthetic always does. * feat(artwork): tint a synthetic blurhash from a base colour Parses baseColor into HSL and clamps saturation/lightness so the tint stays muted; per-cell hashing (unchanged) is what keeps a shared tint across an album's tracks from colliding, as the new 1M-seed spec proves under a single fixed colour. * fix(artwork): trim over-long comment in synthetic blurhash test Review flagged the collision spec's comment for exceeding the 2-line budget; the Finamp rationale it restated already lives in the design doc and commit history. * feat(jellyfin): send a synthetic blurhash for unresolved artwork Clients render nothing where a placeholder belongs when ImageBlurHashes is omitted for pending artwork. primaryImage now synthesizes a value (seeded on the tag, so a cover swap re-keys it) whenever a tag is present but no blurhash has been computed yet. Known-absent artwork (ImageAbsent) is unaffected: it still emits neither tag nor blurhash, since GetOrPlaceholder would otherwise pin a shared placeholder under a distinct cache key for a year. * fix(jellyfin): avoid computing a synthetic blurhash when a real one exists cmp.Or evaluates both arguments before choosing between them, so blurhash.Synthetic ran (and was discarded) on every call even when img.BlurHash was already set. That's the common case in production: artwork_repository.go populates BlurHash from persisted values once a scan resolves it, so a healthy library paid the synthesis cost (xxh3 hashing, HSL conversion, image alloc, DCT encode) on every mapped item for a value it never used. Branch on emptiness first instead. * feat(jellyfin): tint a pending track's placeholder with its album's colour primaryImage never reaches the embeddedArtPending branch of SongToBaseItem, since there is no resolved image yet to feed it. Seed the synthetic blurhash on mf.ID (so the value stays unique and the client still issues the read-through request) but tint it with the album's DominantColor, which is already hydrated on MediaFile at no extra cost. * style(artwork): trim synthetic blurhash comments to the budget * docs(jellyfin): fix stale blurhash README bullet + two review nits The "Blurhashes are synthetic" bullet under Known limitations described dto/blurhash.go, which was deleted when the real core/artwork-computed blurhash + synthetic-fallback pipeline landed; every claim in it was false. Replaced it with an accurate paragraph in the Images section, since the described behaviour is now the finished design, not a gap. Also: fix a doc/body comment mismatch in blurhash.go (component counts are 2..9, not 1..9), and deduplicate the inline DC-extraction logic in synthetic_test.go by reusing the existing dcOf helper. * refactor(artwork): slice the synthetic cell jitter on byte boundaries The three perturbations came off one hash with mismatched masks and shifts (0xFF at 0, 0x3F at 8, 0x3F at 14), so a reader had to do the arithmetic to confirm the fields did not overlap. Only 20 of the 64 bits were in use either way, so the narrower fields bought nothing. Uniform byte slices at 0/8/16 are non-overlapping by inspection and give each field the full 8 bits. Both 1,000,000-seed collision specs still measure 1,000,000 distinct values. Also drops the local `n` alias, which was a second name for synthComponents inside a 15-line function. * docs(jellyfin): fix the blurhash paragraph's opening sentence It opened with "follow the same principle", pointing back at the preceding paragraph on admin-context artwork resolution — an unrelated subject, so the reader looks for a connection that is not there. * fix(artwork): render the synthetic grid larger than its component count The 3x3 cell grid was handed straight to the encoder as a 3x3 image, so the source had exactly as many samples as basis functions. Blurhash normalises its coefficients by 1/(w*h) and 2/(w*h), which assumes many samples per component, so the AC terms came out far too large. At the bottom-right corner the x and y bases are both [1, -0.5, -0.5], everything lines up negative, and the result clamped to black — a dark blob on every synthetic placeholder. Rendering the same nine colours bilinearly at 8x8 first removes it: measured over three seeds, the darkest corner goes from 13 to 74 and the darkest pixel from 1 to 63. 8px is the smallest size that clears the artefact; 12 and 16 are visually indistinguishable and cost 1.7x and 2.7x more. The interpolation is hand-rolled rather than x/image's scaler, which allocated 528 times per call against 14 for this. Both 1,000,000-seed collision specs still measure 1,000,000 distinct values. * refactor(artwork): tidy the synthetic upscale helpers cellWeight nudged its upper bound with a 1e-9 epsilon so int() could never land on the last cell. Clamping the coordinate and then the index says the same thing without a magic constant, and makes the clamp-don't-extrapolate intent explicit — edge pixels map outside the cell centres, so the fraction would otherwise run past 1. The grid type is spelled once as colorGrid rather than repeated in the local and the upscale signature, and encodeAt's doc now states the source-size contract that Synthetic depends on, so the next caller sees it at the function rather than only in synthetic.go. Output is unchanged: all seven sample hashes match byte for byte. * perf(artwork): make the synthetic upscale separable Both axes are square and constant-sized, so the per-pixel cell index and weight were the same 8 values recomputed 64 times per call. They are now built once by sync.OnceValue, matching the srgbToLinearTable pattern. The interpolation is also separable: stretching each of the 3 grid rows horizontally once and then blending rows vertically does 264 lerps where the per-pixel form did 576. upscale drops from 347ns to 260ns, Synthetic from 1780ns to 1669ns. Output is byte-identical across all seven sample hashes — same operations in the same order, only hoisted. * refactor(artwork): let a caller supply the cosine basis encodeAt built its cosine tables inline, so Synthetic rebuilt bit-identical ones on every call — its shape is always 3 components over 8 pixels. Splitting the table construction into cosBasis and the encoder proper into encodePixels lets Synthetic build the basis once via sync.OnceValue, and leaves encodeAt's signature and behaviour untouched. Synthetic drops from 14 allocations to 6 (1669ns to 1508ns). The time saving is small and this path only runs while artwork is still unresolved; the allocation cut is the point, alongside a shorter encodeAt. Every hash is unchanged: nine real Encode outputs spanning square, 10x200, 200x10, 1x50 and a real JPEG, plus all seven synthetic samples, all byte for byte identical before and after.	2026-08-11 18:34:27 -04:00
Deluan Quintão	9e95b19a4f	feat(artwork): make the artwork image size cap configurable (#5931) ... * feat(artwork): make the artwork image size cap configurable Replace the hardcoded 20MB cap on resolved image reads with a new MaxImageSize config option. Load floors it at MaxImageUploadSize so an accepted upload can never be too large for the resolver to read back. * fix(conf): reject zero-valued byte-size options at startup ParseBytes accepts "0", but parseSize silently substitutes the default for it, so the accepted config would differ from the effective limit. * fix(conf): reject byte-size options that overflow int64 A raw value above math.MaxInt64 parses as a valid uint64 but wraps to a negative int64 in parseSize, giving readCapped a non-positive LimitReader bound so every artwork read comes back empty.	2026-08-11 10:42:39 -04:00
Deluan Quintão	7736bbb545	fix(cache): write the completion marker before closing the cache writer (#5927) ... * fix(cache): write the completion marker before closing the cache writer Readers of an in-progress cache write see EOF the moment the writer closes, but the .complete marker was created after the close, on the background goroutine — so a fully-read stream did not mean the cache was done touching disk. The new artwork precache spec ends right at EOF, and its GinkgoT().TempDir() cleanup raced the marker creation, failing the Windows CI job with 'unlinkat ...: The directory is not empty' (the race also reproduces on macOS, 2 of 3 runs, with the tightened test). Writing the marker after a clean copy but before Close makes reader-EOF imply every on-disk write for the entry is finished. A failed writer Close still invalidates the entry, which removes both the marker and the data file. The existing marker test now asserts the marker exists immediately at EOF instead of Eventually. * fix(artwork): never dispatch queue items after the drain context is cancelled The 10x Windows stress run for the previous commit surfaced a second flake in the same package: 'leaves undispatched items queued when cancelled mid-batch' lost row alc7 in 4 of 10 runs. In drain, when a semaphore slot is free and the context is already cancelled, both cases of the blocking select are ready and Go picks one at random — so a cancelled drain could still dispatch items. A non-blocking Done check before the select gives cancellation priority. The race was invisible on Linux/macOS only by accident: the spec seeded the album repo with a single album (each SetData overwrote the last), so only the final row (alc7) resolved to absent and got deleted when dispatched; the others fell on the retry path and survived. Nanosecond enqueue timestamps made alc0 always first out of the mock dequeue, masking the race, while Windows' coarse clock ties the timestamps and randomizes the order. The spec now seeds all eight albums, which made the race reproduce locally on the first try (row alc0) and now guards the fix on every platform. * test: give cache-init waits a 10s timeout for loaded CI runners A 10x parallel Windows stress run timed out one artwork spec in BeforeEach: the FileCache init goroutine (mkdir + reload walk) took over Gomega's default 1s Eventually timeout under shared-runner disk contention. Bump the three identical init waits (two artwork suites and the utils/cache helper) to 10s. * test(scanner): widen watcher debounce margins for loaded CI runners The watcher debouncing spec asserts 'no scan yet' inside 20ms Consistently windows while the debounce wait was only 50ms — a 2.5x margin that a loaded Windows runner blows through by delaying the timer-reset notification, firing the scan early (failed all three FlakeAttempts in a 10x stress run). Raise the test debounce wait to 200ms (10x the observation windows) and the scan-fired Eventually timeouts to 2s to match. * refactor(artwork): collapse drain cancellation into a single exit path Replace the non-blocking ctx pre-check plus duplicated select exit with one select and a ctx.Err() check after it. Besides removing the duplication, this closes the residual race: a cancellation landing between the two selects could still let the blocking select randomly pick the free semaphore slot and dispatch the item. Now a dispatch is only possible when the context was live after slot acquisition. * style: trim flaky-test fix comments to single lines Compress each two-line comment added by this PR to the one line that carries the invariant; drop the narration around it.	2026-08-10 14:10:05 -04:00
Deluan Quintão	944ca3100f	feat(artwork): new artwork pipeline with background resolution and Low Quality Image Placeholders (#5847) ... * feat(artwork): add artwork, item_artwork and artwork_queue tables * feat(artwork): add artwork models, repository interfaces and mocks * feat(artwork): implement artwork repository * feat(artwork): implement item_artwork repository with batched hydration * feat(artwork): implement artwork_queue repository * feat(artwork): add content-addressed originals store * feat(artwork): add artwork prune (orphan cleanup) * fix(artwork): never sweep files on transient DB errors during prune * refactor(artwork): fold originals package into core/artwork as ImageStore * refactor(artwork): merge item artwork state into ArtworkRepository * fix(artwork): chunk unbounded IN clauses and restore interface docs * refactor(artwork): apply simplify-pass cleanups Internal item_artwork sqlRepository helper, toSQLArgs upserts, batched queue enqueue, EnqueueStaleAbsent moved to queue repo, snapshot-based prune sweep, mock/real semantics aligned. * fix(artwork): address review findings on prune/sweep races and mock fidelity 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. * fix(artwork): atomic orphan deletion and timestamp semantics from review DeleteOrphans re-checks age+references at delete time, PutItemArtwork defaults attempted_at, queue mock timestamps mirror SQL. * fix(artwork): guard orphan file removal with the prune grace window 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. * fix(artwork): rewrite vanished duplicates and sweep stale mime variants 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. * fix(artwork): index artwork_queue in dequeue order 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. * fix(artwork): honor the orphan cutoff in the repository mock The mock's DeleteOrphans now applies createdBefore like the SQL implementation, and a new spec covers a freshly reacquired row surviving prune. * fix(artwork): reject malformed hashes in ImageStore operations 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. * fix(artwork): mock PutImage refreshes created_at like the SQL repository Prune specs now age fixtures directly instead of seeding stale timestamps through the upsert. * fix(artwork): store backing-file provenance per item, not per hash * feat(artwork): import blurhash encoder from #5797 * feat(artwork): add worker-side artwork resolvers * fix(artwork): propagate playlist tile failures and dedupe external step * feat(artwork): add acquisition processor Resolves one queue item end to end: hash/dedup, decode + 128px thumbnail blurhash, place bytes (store vs source file), and persist found/absent/ failed state for the worker (Task 4) to act on. * style(artwork): tighten processor comments to budget * feat(artwork): add acquisition worker service * feat(artwork): enqueue artwork resolution from scan and CRUD paths * feat(artwork): artwork backfill, fingerprint re-resolution and scheduled jobs * test(artwork): leak/soak coverage and deferred assertions * fix(artwork): propagate transient artist image errors to the worker callGetImage swallowed all agent errors, so an agent outage surfaced as ErrNotFound and the worker settled artist artwork as a definitive absent (and reset the breaker). Add an additive ArtistImageResult path that returns the underlying agent error on transient failure while keeping ArtistImage byte-identical for existing callers; the worker's artist external step uses it via fromArtistExternalResult. * fix(artwork): resolve full playlist source chain resolvePlaylist only built the generated grid, dropping the uploaded-image, sidecar and ExternalImageURL sources the old reader_playlist.go chain serves. Port the full chain before the grid fallback: uploaded (upload), sidecar (folder), and ExternalImageURL routed through extGate with the same extError semantics as the other external steps. Also rewires the artist external step onto ArtistImageResult. * fix(artwork): purge dangling queue rows and guard concurrent re-enqueues Queue rows for deleted entities failed forever (Get -> ErrNotFound -> failed -> capped retries, unbounded). Add ArtworkQueueRepository.PurgeDangling, called from Prune next to the item_artwork purge. Separately, the found/absent path unconditionally deleted the dequeued row, erasing a concurrent scan re-enqueue; switch to DeleteIfUnchanged, which deletes only while retry_at still matches the dequeued value (verified retry_at is the column an Enqueue upsert resets). * style(artwork): fix comment accuracy and budget; fingerprint ArtistImageFolder Correct the inverted workerDeps.extGate comment, trim over-budget doc comments, and add conf.Server.ArtistImageFolder to the resolution fingerprint so an image-folder change re-resolves artist artwork. * fix(artwork): treat missing local playlist cover as definitive, not transient A playlist ExternalImageURL pointing at a local file that fails to open was routed through extError, causing failed/48h-retry loops that burn a rate limiter token forever instead of falling through to the generated grid. * refactor(artwork): deduplicate purge loop, backfill table, and extGate alias * fix(artwork): cap resolved image reads A user-editable ExternalImageURL can point at an arbitrarily large endpoint; a fast server could make the worker buffer hundreds of MB inside the 5s HTTP timeout. Bound the read to a fixed 20MB cap (no config knob) via io.LimitReader and fail the item if it is exceeded. * fix(artwork): retry higher-priority external art after fallback hit With CoverArtPriority="external,cover.jpg", a transient external failure followed by a folder hit dropped the external error: the worker recorded found and deleted the queue row, so the configured higher-priority external art was never retried. Carry extError onto the fallback resolution and add an outcomeFoundStale that persists+serves the art but reschedules via MarkFailed, giving the external source another chance. When external later answers definitively-not-found, the hit is not stale and the row is deleted. * fix(artwork): treat playlist cover URL 404 as definitive miss The playlist ExternalImageURL step used sources.go's fromURL, which maps any non-200 to a generic error, so a stale URL returning 404/410 was classified transient: infinite backoff plus it counted toward the circuit breaker, blocking valid external work. Add a local fetch in resolve.go that maps 404/410 to model.ErrNotFound (definitive) while keeping other non-200s transient. sources.go is left untouched. * test(artwork): move soak test into the Ginkgo suite * test(artwork): make leak and permission tests pass on linux goleak now ignores notify's nonrecursive-tree goroutines (linux uses inotify, which spawns dispatch+internal instead of darwin's recursive dispatch), and the read-only-dir prune spec skips under root, where permission bits cannot make Remove fail. * fix(artwork): reject decompression-bomb dimensions before decoding * fix(artwork): keep fresh re-enqueues ahead of stale failure backoff * fix(artwork): include M3U external art flag in the config fingerprint * fix(artwork): resolve private playlists with an admin context * test(artwork): convert non-synctest timing tests to Ginkgo specs TestArtworkBackoffSchedule and TestArtworkWorkerRunNoLeak needed no real *testing.T (no synctest), so move them into worker_test.go as Ginkgo specs. TestArtworkBreakerHalfOpen stays plain since testing/synctest requires a real *testing.T, matching core/scrobbler's precedent. * fix(artwork): store backing-file provenance per item, not per hash * fix(artwork): apply image limits to playlist tile decoding decodeTile ran image.Decode on every sampled album's resolved bytes before processItem's maxImageBytes/maxImagePixels guards applied, letting an oversized or decompression-bomb tile fully decode unbounded. Enforce both caps inside decodeTile itself. * refactor(artwork): reuse auth.WithAdminUser and dedupe image cap guards * perf(artwork): fetch only IDs for backfill enumeration Backfill enumerated every album, artist, playlist and radio via GetAll and mapped out just the ID. GetAll materializes full entities (library joins, participant/stats/tags JSON, annotation, artwork hydration), so on a large library it loaded tens of thousands of heavy structs only to read one field each — spiking transient RSS to ~1GB during the one-time upgrade backfill, a memory risk on small NAS/Pi hardware. Add GetAllIDs to the album, artist, playlist and radio repositories: it reuses each repo's base row-set filter (library visibility, artist content join, playlist userFilter) but projects only id, skipping the heavy columns and post-processing. A per-repo parity test asserts GetAllIDs returns exactly the same id set as GetAll. Verified on a 727MB / 29k-artist production DB copy: peak RSS during backfill dropped from ~1012MB to ~89MB, file descriptors flat, same 36,138 items enqueued. * feat(artwork): promote worker concurrency and external rate to real configs The artwork worker's drain speed was governed by two hidden Dev flags, DevArtworkWorkerConcurrency and DevArtworkExternalRPS, both defaulting to 2. On a large library's one-time backfill the external rate limiter is the real ceiling: every art-less item waits on it before the (rate-limited) external lookup, so the drain crawls at ~RPS items/sec while local-art items are unaffected. Promote both to documented, supported options: ArtworkWorkerConcurrency (default 4) sets local-resolution parallelism, ArtworkExternalMaxRPS (default 2, 0 = unlimited) caps external-agent lookups to stay polite to Last.fm/Deezer/etc. Operators can now trade first-backfill speed against external-API rate limits. The old Dev names still map for backward compat. * fix(deezer): never return empty-image-id placeholder pictures * feat(agents): enumerate enabled image-retriever agents per capability * feat(artwork): worker fetches agent images directly with per-agent rate limits and breakers * fix(artwork): treat agent not-found as breaker success * feat(model): content-hash artwork id suffix and hydratable per-entity image state * feat(persistence): hydrate artwork hash and absence onto entity pages * feat(artwork): resolve media_file embedded art in the worker, invalidate on rescan * feat(artwork): broadcast refresh events when artwork lands * fix(artwork): broadcast refresh for stale-found artwork too * feat(artwork): state-backed serving path with provisional read-through * feat(server): serve artwork from persisted state with content-hash caching * feat(subsonic): content-hash coverArt ids, omit artwork on known-absent * refactor(artwork): delete the legacy reader chain, cache warmer, and provider image methods * feat(artwork): precache on acquisition, bump on upload/radio changes, manual re-resolve API * test(artwork): end-to-end coverage for the serving cutover * chore(artwork): generic 500 bodies on refresh endpoint, trim stale test comments * fix(artwork): request read-through must not reset the failure backoff The provisional read-through and dangling re-enqueue used Enqueue, whose upsert resets retry_at, so any browse of an unresolved entity that was backing off after an external failure made it immediately eligible again — defeating the exponential backoff during a provider outage. Add EnqueueBump, which raises priority but leaves an existing row's retry_at intact, and route the serving path through it. Scan and manual re-resolve keep Enqueue's reset (a detected change wants immediate retry). * fix(artwork): keep an eligible track's cover requestable when its album is absent An embedded-eligible track with no resolved item_artwork row inherited the album's ImageAbsent, so when the album resolved absent (e.g. CoverArtPriority without 'embedded') the track's coverArt was omitted permanently — the client never requested it, so the lazy mediafile path never resolved it — even though the serving path would extract and serve the track's own embedded art. Hydration now never copies the album's absence onto an eligible-but-unresolved track. * fix(artwork): validate each agent image URL before picking the largest bestImageURL selected the largest by size and only then parsed it, so a malformed largest URL (e.g. a bad percent-escape) returned nil and shadowed a valid smaller candidate, contradicting the documented skip-unparseable behavior. Parse per candidate and compare sizes only among URLs that parse. * fix(artwork): fall back to disc art, not the album, for multi-disc tracks serveMediaFile delegated an absent/ineligible track straight to AlbumCoverArtID, skipping the disc-specific lookup that MediaFile.CoverArtID (and the deleted legacy reader) use. On multi-disc albums with per-disc images that served the album cover instead of the configured disc artwork. Delegate through DiscCoverArtID. * fix(artwork): enqueue uploaded artwork only after the filename is persisted SetImage cleared state and enqueued the bump before the caller stored the new filename, so a worker drain in that window could resolve against the old (already deleted) file and settle absent, leaving the upload unused until a later scan. Move the invalidate+enqueue into EnqueueArtwork, which each caller now invokes after the entity Put. * fix(artwork): keep multi-disc tracks requestable when the album is absent Round-1's hydration fix still copied the album's known-absent onto a non-eligible (or own-absent) track, but MediaFile.CoverArtID routes a multi-disc track to disc art, which resolves provisionally and is never known-absent. Marking it absent made Subsonic omit coverArt so clients never requested a valid disc image. Only mark a single-disc track absent, and only when its own art won't resolve. * fix(artwork): serve a local playlist ExternalImageURL as a file-backed reference A local ExternalImageURL was resolved through the external step and labelled external, so placeBytes copied it into the content-addressed store and dropped its path/mtime — replacing the file never tripped the staleness check. Classify local references as file-backed (resolved in place, even on the request path) and keep store-backed behaviour only for http(s) URLs. * fix(artwork): requeue playlist cover when its track set changes A generated-grid cover went stale after track mutations: nothing re-resolved the playlist's artwork, and the request path deliberately never rebuilds the grid, so serveEntity kept returning the old grid hash indefinitely. Enqueue pl artwork from refreshCounters (the choke point for every track-set change); no clear, so the old cover keeps serving until the worker rebuilds. * fix(artwork): open library-backed artwork through its on-disk root A library configured with a file:// path stored absRoot as the raw URI, so Abs produced strings like file:/music/cover.jpg that os.Open/os.Stat reject — folder, upload and embedded art were treated as dangling on every request, looping forever. Normalize a file:// path to its parsed OS path (the same root os.DirFS uses); non-local schemes are left unchanged (out of scope, per the artwork-musicfs TODO). * fix(artwork): only use disc resolution for multi-disc albums DiscCoverArtID returns a dc- id for any track with DiscNumber>0, so serveDisc ran the full DiscArtPriority chain even for single-disc albums, where a stray disc*/ embedded image could shadow higher-priority album art. Gate disc resolution on the album having more than one disc, matching the legacy reader; single-disc tracks serve album art directly. * fix(artwork): invalidate artwork when an uploaded image is deleted Deleting an artist/radio/playlist upload cleared the filename but left the found item_artwork row and its hash, so lists kept advertising the deleted cover's hash-suffixed immutable URL and clients could display it indefinitely. Call EnqueueArtwork after the delete-side Put, symmetric with upload, so the state is cleared and re-resolved to the next source (or absent). * fix(artwork): restore synthetic-artist guard and unicode normalization in agent lookups Moving agent calls into the worker bypassed two behaviors of the aggregate provider: Agents.GetArtistImages' guard for Unknown/Various Artists (a direct retriever call could assign an unrelated image to a synthetic artist), and auxAlbum/auxArtist.Name's DevPreserveUnicodeInExternalCalls normalization (records with typographic quotes/dashes missed exact-name searches). Re-apply both before enumerating retrievers. * fix(artwork): enforce entity visibility on the Subsonic getCoverArt path serveEntity reads persisted item_artwork by id, bypassing the library and private- playlist filters that the legacy entity-load applied. On the authenticated Subsonic path a user could fetch artwork for an inaccessible album or someone else's private playlist by guessing an id. getCoverArt now resolves the underlying entity through the request-scoped (filtered) repositories and serves the placeholder when it is not visible, so existence isn't leaked and the always-an-image invariant holds. The public share (JWT-authorized) and Jellyfin (admin) paths are intentionally untouched. * fix(artwork): version the artwork ETag with the served representation The ETag was the pixel hash of the original image, so a CoverArtQuality or EnableWebPEncoding change altered the resized bytes without changing the ETag — revalidating clients got a spurious 304 and kept the old encoding. Resized responses now carry a representation ETag (hash + size + square + encode settings) used for the ETag header and If-None-Match, while the immutable decision stays on the pixel hash (URLs remain pixel-identity per the spec, so hash-suffixed clients keep zero-request caching). Full-size originals fall back to the pixel hash as before. * fix(artwork): don't stamp the album hash onto multi-disc tracks The hydration fallback assigned a found album hash to every fallback track, but a multi-disc track's CoverArtID emits a dc- id served from disc-specific art whose hash is unknown at hydration time. Advertising dc-..._<albumHash> gave clients a content- version that never changes when the disc image does, breaking id-based refresh. Only stamp the album hash for single-disc tracks (DiscNumber == 0); multi-disc tracks stay unhashed and rely on the correct ETag returned by the served response. * fix(artwork): enqueue new empty playlists by id, and refresh on absent outcomes Two worker/enqueue fixes from review: - playlistRepository.Put assigned the generated id to the caller's Playlist but passed the stale copy (empty id) to refreshCounters, enqueueing a pl|"" row the worker failed until the daily dangling purge while the real playlist went unresolved. Set the id on the copy before enqueueing. - The drain refresh batch only included found/foundStale, so a cover removed by a scan (found -> absent) never notified clients, leaving the old immutable image displayed. Broadcast absent outcomes too; precache still only warms found/foundStale. * fix(artwork): honor disabled per-track art at serve time; use nanosecond mtime provenance Two serving-correctness fixes from review: - serveMediaFile served a persisted mf embedded image even after EnableMediaFileCoverArt was turned off (the setting isn't in the config fingerprint, so found rows aren't reprocessed). Direct mf- URLs now honor the setting at serve time and fall back to disc/album art. - The file-backed staleness check compared whole-second mtimes, so a same-second content replacement (two writes in one second, or timestamp-preserving tools) could serve different bytes under the old hash + immutable policy. RefMtime is now unix-nanoseconds (no schema change; int64 column), detecting sub-second changes where the filesystem records them. * fix(artwork): preserve the drive when normalizing Windows file:// library paths url.Parse puts the volume of file://C:/Music in Host, not Path, so localOSRoot dropped it and returned /Music — os.Open/os.Stat then failed and folder/embedded art on Windows looped as dangling. Rejoin the host volume, matching core/storage/local's newLocalStorage. * fix(artwork): clamp negative sizes to full-size; convert imghttp test to Ginkgo - A negative size (Subsonic size / Jellyfin maxwidth accept signed ints) reached resizeStaticImage, where the square path builds image.NewNRGBA(Rect(0,0,size,size)) — a giant rectangle that panics/OOMs. Clamp size<0 to 0 (full-size) at the Service entry. Positive sizes were already clamped to the original. - imghttp used a plain func Test with a table; convert to a Ginkgo DescribeTable with the suite entry point in imghttp_suite_test.go (AGENTS.md test-framework requirement). * test(artwork): use renamed ArtworkWorkerConcurrency in e2e tests * feat(artwork): carry blurhash through item image hydration * feat(nativeapi): expose artwork hash, absence and blurhash * feat(artwork): hydrate the parent album's artwork state onto tracks * test(artwork): add hydrateArtwork regression guard for AlbumImage wiring Drives hydrateArtwork itself (not applyItemImage directly) over tracks that take each of the loop's continue branches, so a future edit moving the AlbumImage fill below a continue would fail loudly instead of passing silently. * feat(jellyfin): version album and artist image tags by content hash * fix(jellyfin): trim primaryImageTag comment to why-only, within budget * feat(jellyfin): emit real blurhashes and drop the synthesized fallback * feat(ui): version cover art urls by content hash and skip absent art * feat(ui): add BlurHashCanvas placeholder component * fix(ui): clear stale blurhash pixels and assert the draw path in tests Clear the canvas before each decode attempt so a hash change that fails to decode doesn't leave the previous frame's pixels on screen once this wires into a list that recycles items. Also strengthen the specs to assert createImageData/putImageData were actually invoked (and with what), instead of only checking that a <canvas> element exists. * feat(ui): show the blurhash while an album cover loads * fix(artwork): hydrate cursor streams via an id pre-pass The album, artist and playlist GetCursor built their own select and never called hydrateArtwork, so every Jellyfin list endpoint (all six stream via GetCursor) emitted entity-id image tags and no blurhash. Only GetAll hydrated, which is why Subsonic and the native API were unaffected. Each cursor now resolves its ordered/filtered/paginated id set with the cheap id-only GetAllIDs query, then streams those ids in chunks through the repo's existing GetAll, which already hydrates and applies the full select. Max/Offset are consumed by the pre-pass alone; the chunk query carries only the caller's filters, Sort and Order. This also removes a pre-existing deep-pagination cost: keeping OFFSET out of the joined query makes the pre-pass a covering index scan instead of paying the library and annotation joins for every skipped row. Benchmarked on a synthetic 100k-album DB with the real schema, page=500 at offset 90,000: 3.9ms via the id pre-pass, 52.5ms for the current shape, 192.7ms for a naive join. An unpaginated full stream costs ~24% more, which is the trade. GetAllIDs gains the annotation join whenever the caller's filters or sort reference an annotation column (same gate CountAll uses), otherwise Filters=IsFavorite and SortBy=PlayCount would fail in the pre-pass. The playlist pre-pass repeats GetAll's columns so ORDER BY keeps resolving to playlist.name rather than the joined user.name. * fix(jellyfin): hydrate artwork on the song cursor Jellyfin's listSongs streamed media files via GetCursor, which never hydrates artwork, so songs emitted entity-id image tags and no blurhash. media_file now uses the same id pre-pass as the other three cursors (album/artist/playlist), for consistency, but on a separate method, GetCursorWithArtwork: GetCursor itself must stay untouched, since it's also the scanner's hot path and the scanner never reads artwork. Measured on 1,000,000 tracks, the pre-pass over all ids costs +41.8 MB heap and +298 ms versus GetCursor's bounded +0.0 MB. The Jellyfin path is paginated, though, so in practice it only ever pre-passes a page's worth of ids, not the full library, and doesn't pay that cost. * feat(jellyfin): emit a song's own cover art when it differs from the album's Real Jellyfin fills ImageTags from each item's own images before falling back to the parent album, and Finamp checks imageTags.Primary before AlbumId. Our mapper read only the album's image, so a track with distinct embedded art silently showed the album cover. Emit exactly one entry under ImageBlurHashes.Primary: Go marshals map[string]string in sorted key order rather than insertion order, so a second entry could pair the wrong blurhash with the image imageId resolves to, and Finamp pins that pairing in its cache for 365 days. * test(persistence): scope the GetCursorWithArtwork full-stream spec to tie-free ids The fixture has title ties (e.g. three "Antenna" tracks), so the unscoped positional comparison against GetAll only passed because SQLite's tie order happened to coincide between the full scan and the pre-pass's id IN (...) fetch. Scope it to onlySongs like the sibling ordering specs already do. * refactor(artwork): route song own-art through primaryImageTag; align chunk size Cleanups surfaced by /simplify: the song mapper's own-art branch reimplemented primaryImageTag's tag+blurhash-map construction (and its one-entry invariant) — route it through the helper so that invariant lives in one place. Tie artworkChunkSize to a whole multiple of artworkBatchSize so a cursor page re-chunks into even hydration batches. Hoist a duplicated imageLoading && blurHash boolean in the album grid. * fix(ui): serve the placeholder for known-absent art instead of a broken icon getCoverArtUrl returned '' for an imageAbsent record, so <img src={undefined}> rendered as the browser's broken-image icon on every absent cover. The server already serves a proper placeholder for absent art, so build the url and let it render. * feat(ui): show the blurhash as the loading placeholder across cover surfaces Add a shared CoverImage component (useImageUrl blob cache + blurhash + fade) and render the blurhash while a cover loads on the list thumbnails (CoverArtAvatar, radio) and the artist/album/playlist detail pages. The detail pages now go through CoverImage instead of a plain CardMedia, so their images come from the in-memory blob cache and survive React remounts without re-fetching. BlurHashCanvas gains an optional style prop. * refactor(ui): unify list cover surfaces onto the shared CoverImage component Route the album grid, CoverArtAvatar (artist/playlist lists) and the radio list's cover field through CoverImage instead of each carrying its own useImageUrl + blurhash-overlay wiring. CoverImage gains a default object-fit: cover. Radio keeps its uploaded-image gate and the generic radio placeholder for stations with no art. * fix(ui): address CoverImage review findings Restructure CoverImage so the size/shape lives on the root and the blurhash + image are absolute fills: the <img> mounts only once its blob is ready, so an unresolved cover never flashes a broken <img>. Add a fit prop (default cover) so album/playlist detail keep their letterbox instead of being cropped by a hardcoded object-fit. Remove the orphaned coverLoading styles and an unused subsonic import; add a CoverImage unit test. * perf(ui): only refetch already-loaded records on SSE refresh The artwork worker broadcasts a RefreshResource event per resolved chunk, carrying every id in the chunk. useResourceRefresh was doing a getMany for all of them, so any open list/detail page fetched hundreds of artists it was not displaying. Filter the event ids to records already in the store; the rest load fresh (with their new artwork) when navigated to. * refactor(artwork): scale worker concurrency with CPU count ArtworkWorkerConcurrency now defaults to max(2, NumCPU()/2) instead of a fixed 4, mirroring MaxOpenConns: local resolution scales with the host but stays at half the SQLite pool so it never starves the scanner/UI. External RPS stays a fixed 2 — it gates third-party API calls and is bounded by their tolerance, not the host, so it must not scale with CPUs. Also drop the DevArtworkWorkerConcurrency/DevArtworkExternalRPS deprecated aliases: those names were never released, so there is nothing to migrate. * feat(artwork): re-queue an absent cover when its page is viewed serveEntity now schedules a Bump recheck for an entity whose art was recorded absent, so viewing a missing cover re-triggers resolution (e.g. after an external source that was down during the scan comes back), matching the request-time bump that already covers never-resolved entities. Throttled by attempted_at against requestRecheckAge (1h) so repeatedly opening a genuinely-absent page can't hammer external services. EnqueueBump preserves an existing failed-state backoff via MAX(priority,...) and inserts a fresh, immediately-eligible recheck for a settled-absent row (whose queue row was already deleted). * refactor(artwork): move ImageUploadService to artwork.Uploader Relocate the image-upload service from core to core/artwork as artwork.Uploader, co-locating it with the resolver/worker/serving that own the artwork state it invalidates. MaxImageUploadSize moves too — its only callers are the two image-upload handlers — which lets core/image_upload.go be deleted entirely. Extract the shared "clear resolved state + re-queue at Bump" invalidation into artwork.Refresh and fold nativeapi's refreshArtwork handler onto it, removing the duplicated DeleteForItem+Enqueue block that had drifted into three places. The wire provider moves from core's set to artwork's; the playlists.ImageUploadService binding moves to the top-level injector so core/artwork stays unaware of playlists. Behavior is unchanged. * refactor(artwork): thread model.Kind through the artwork API Entity-level artwork queries now take a typed model.Kind instead of a bare prefix string. GetItemArtwork, DeleteForItem(s), GetInfoForItems, EnqueueStaleAbsent, hydrateItemImages, enqueueBackfillKind and artwork.Refresh convert to the prefix string only at the two real boundaries: the SQL item_kind column (kind.Prefix() inside each repo) and external string inputs (a new model.ParseKind for the nativeapi URL param, which also validates it). The Backfill/stale-absent kind slices, the resolve.go dispatch switch, and the kind→resource / kind→table lookup maps now use the Kind vars directly. The queue lifecycle methods (MarkFailed/Delete*) keep string kinds — they operate on a dequeued item's raw ItemKind column, which stays a string field, always populated via kind.Prefix(). Removes every bare "al"/"ar"/… prefix literal from non-test code (27 -> 0); behavior is unchanged. * tune(artwork): drop backoff base from 5m to 15s The exponential retry (base × 4^attempts, cap 48h) started at 5 minutes, so a single transient failure — a timeout under load, an external blip — parked a cover for 5 minutes even though a retry seconds later would have resolved it. Start at 15s instead: transient failures recover almost immediately (15s → 1m → 4m → 16m …), while persistent failures still escalate to the 48h cap (now at the 8th attempt instead of the 5th). * tune(artwork): 5s backoff base + 12h give-up, drop the cap Retry backoff now starts at 5s (was 15s) so a transient failure recovers on essentially the next drain, and jitter widens to ±40% so a wave of correlated failures doesn't re-clump into one poll. Add a 12h give-up budget measured from enqueued_at: once the next backoff would land past it, the worker stops retrying instead of grinding at a cap forever. A bare failure settles absent (handed to the 24h stale-absent sweep, and still recoverable on a page view); a found-stale keeps its already-served art. The budget bounds the tail, so the separate 48h backoffCap is removed. * fix(lastfm): match album.getInfo on name+artist only, not MBID Last.fm's album.getInfo by MBID is unreliable: a correct MBID can return a different album, or none. Observed with black midi's "7-eleven" (whose correct MBID returned a FLEETWOOD release) and both missing The Chats albums (one MBID 404s, the other resolves to a different self-titled release). The worker then recorded covers absent — or would fetch the wrong art — even though the correct cover is on Last.fm by name+artist. Stop passing the MBID to album.getInfo; query by name+artist only, which also drops the now-dead error-6 MBID-retry fallback. The low-level client keeps its MBID support for other callers; only the album lookup changes. * fix(lastfm): return agents.ErrNotFound on error 6 (not found) Last.fm returns error 6 for a missing artist/album — a definitive negative — but the agent returned the raw *lastFMError, so the artwork worker treated every not-found as a real fault: it counted toward the per-source circuit breaker (5 in a row opens it, fast-failing all Last.fm calls including valid ones) and was retried as a transient error instead of settling absent. On a first scan of a library with many artists Last.fm lacks, this stalled valid cover lookups and left entities churning in backoff. Translate error 6 to the shared agents.ErrNotFound at the agent boundary (callAlbumGetInfo / callArtistGetInfo), matching how the Deezer agent maps its client's not-found, and log it at Debug instead of Error — which also removes the not-found log spam. * feat(artwork): log external image-lookup failures at debug The worker's res.reader==nil && extError branch returned outcomeFailed with no log, so a failing external cover lookup (agent error, dead image URL, download timeout) was undiagnosable. Log the agent, entity, and underlying error at the fetch site where it's in hand — this surfaced a Last.fm album.getInfo returning an image URL that itself 404s. * fix(artwork): treat a 404/410 image URL as not-found, not a transient fault An agent (notably Last.fm's album.getInfo) can advertise a cover URL that is itself dead — a 404. sources.go's fromURL returned a generic error for any non-200, so a dead URL was treated as a transient failure: it churned in backoff and counted toward the circuit breaker, stalling valid lookups. Map 404/410 to model.ErrNotFound in fromURL so a dead URL settles absent, and collapse the near-identical fetchPlaylistImageURL (which already did this for M3U covers) into it. * feat(artwork): make artwork re-resolution targeted, not blunt Two gaps in when the pipeline re-resolves artwork: The recheck job only requeued absent-state rows (hash=''), so an entity that was never processed — added between scans, or on a server with the scanner disabled — had no periodic safety net and stayed without artwork indefinitely. Add EnqueueMissing(kind): a SQL set-difference enqueueing entities with no item_artwork row at Recheck priority (ON CONFLICT DO NOTHING, so it never disturbs a queued row). Run it once at startup and hourly alongside the stale-absent recheck. Rename staleAbsentKinds -> recheckKinds accordingly. Conversely, the config fingerprint included consts.Version, which embeds the git SHA and so changed on every build, re-enqueueing every entity in the library (~34k here) and re-querying external agents at the configured RPS for anything without local art. Replace it with an explicit artworkEpoch constant, bumped deliberately when resolution semantics change. The cases that motivated the version input — absent art becoming available — are already covered by the stale-absent and missing-row rechecks; only a corrected wrong-pick needs the epoch. A test guards against reintroducing the version. * test(artwork): restore resolution edge-case e2e coverage The serving cutover removed the album/disc/artist/mediafile/playlist/radio e2e specs that documented the folder-selection rules and guarded the #5376/#5456/ #5451/#5457 regressions; nothing replaced them, so compareImageFiles and the parent-fallback logic were left untested. Restore them driving the real pipeline: a real scanner populates the folder graph from an in-memory library, the real Worker drains the queue, and the real Service serves. Folder-backed art is file-backed (served via os.Open, which the in-memory FS can't satisfy) so its selection is asserted on the persisted state row; store-backed and real-disk sources are asserted byte-for-byte. Single-disc disc resolution now serves album art directly, so only multi-disc disc scenarios are ported. * fix(artwork): run disc resolution for single-disc albums too ed4178a6 gated serveDisc on len(album.Discs) > 1, claiming parity with the legacy reader. The legacy reader has no such gate: artwork.go dispatches every dc- id to newDiscArtworkReader, whose Reader() walks DiscArtPriority unconditionally. The gate also lost art. For a single-disc album whose only image is disc1.jpg, the disc request skipped the chain and fell through to album art, which does not match CoverArtPriority — so tracks tagged disc 1 (whose CoverArtID is a dc- id) served nothing at all, where before they served disc1.jpg. A single disc can legitimately have its own cover, distinct from the album's, and DiscArtPriority is what expresses that preference. Drop the gate and restore the single-disc e2e scenarios that covered it. * fix(artwork): register the GIF decoder in core/artwork The deleted artwork.go carried blank imports for image/gif and x/image/webp. WebP came back via resize.go's gen2brain/webp, which self-registers, but GIF did not: core/artwork claims GIF support in mimeForFormat and extForMime while relying on an unrelated server package to have imported the decoder. The guard lives in the e2e suite because that test binary has no other image/gif importer; a spec in core/artwork would pass regardless, since animation_test.go imports the package non-blank. * fix(artwork): never record absent after a local I/O failure Local sources swallowed their open errors, so a stale NFS/SMB mount was indistinguishable from "this entity has no artwork": the chain returned no reader, processItem took the absent branch, and the upsert replaced a good content hash with the empty string. Clients then saw a placeholder until the 1h request recheck or the 24h stale-absent sweep, and the orphaned bytes became eligible for the next prune. A candidate the resolver knows about — a file in the folder listing, a track's own audio file — failing to open is not evidence of absence, so it now forces a retry the same way an external agent error does. * fix(artwork): keep served art when the retry budget runs out Exhausting the 12h budget called writeAbsent unconditionally, so an entity whose art was already resolved and serving lost it to a long upstream outage: the hash went empty, clients fell back to the placeholder, and the now-unreferenced bytes were freed by the next prune even though nothing about the image had changed. Exhaustion means the source stayed unreachable, not that the cover disappeared, so absent is now recorded only when there is nothing to keep. * fix(artwork): restart the retry budget on re-enqueue The conflict clause updated only priority and retry_at, so a row that already existed kept its original attempts and enqueued_at. The worker measures the 12h give-up budget from enqueued_at, so any row that had been pending across a longer gap — a server left off, an upgrade, a laptop asleep — gave up on its very first attempt and settled absent. The manual re-resolve endpoint is the sharpest case: it clears artwork state and re-queues, but inherited the old row's spent window, so a deliberate retry got one shot. A fresh request now gets a fresh budget, with the mock updated to match. * fix(artwork): keep not-found distinct from artwork-absent GetOrPlaceholder folded model.ErrNotFound in with ErrUnavailable, so an id matching no entity returned 200 and the placeholder PNG. That made the ErrorDataNotFound branch in getCoverArt unreachable: Subsonic went from error 70 to a successful placeholder, and Jellyfin's Primary image endpoint from 404 to 200. Neither is ours to change. An entity with no art and an id with no entity are different answers; only the first is a placeholder. * fix(artwork): make the prune sweep cancellable Sweep walked the whole store with no context, and RunPrune holds the prune write lock for its full duration. In-flight acquisitions park on the read lock, drain's WaitGroup never returns, and Run never reaches its ctx.Err() check — so a SIGTERM during a daily prune over a large store on slow storage waits out the container's grace period and dies mid-remove. * fix(artwork): hydrate the tracks reached through a playlist loadTracks and the playlist-track cursor were the only entity-page paths that never hydrated artwork state, so a song reached through a playlist behaved differently from the same song in the songs list: Subsonic emitted a hashless coverArt id, which imghttp downgrades to no-cache, and advertised art even for known-absent albums; Jellyfin emitted AlbumPrimaryImageTag as the bare album id — a tag that never changes when the cover does — and no blurhash at all. The media-file hydration moves next to the other hydration helpers so both paths share one implementation rather than growing a third. * feat(ui): cross-fade the cover over its blurhash The blurhash unmounted the moment the blob arrived, so the placeholder vanished a frame before the image painted. The image now mounts transparent and fades in over the blurhash, which stays behind it until the fade completes. A blob already cached when the instance mounts skips the fade, so a remount does not re-animate. * fix(ui): retire the blurhash on a timer, not transitionend Under prefers-reduced-motion the img rule sets transition:none, so toggling opacity fires no transitionend and the handler that unmounts the blurhash never ran. The placeholder stayed mounted for the life of the component — visible in the letterbox bars wherever the cover is rendered with fit="contain", and a live canvas per tile everywhere else. One duration constant now drives both the CSS transition and the timer, so they cannot drift. * fix(artwork): stop advertising a hash for bytes that won't be served Hydration stamped the album's hash and blurhash onto an embedded-eligible track that had no state row yet. Serving takes provisionalEmbedded for exactly that case and returns the track's own embedded image, so the id carried a content-version belonging to a different picture: every such request fell back to no-cache instead of immutable, and a client keying its cover cache on the blurhash paired the album's with the track's art. AlbumCoverArtID had the mirror-image problem, building the album id from the track's own ItemImage. It happened to work only because hydration overwrote ImageHash in precisely the fallback cases; a track with its own resolved art would have stamped that hash onto the album's id. * perf(artwork): precache from the bytes just acquired Warming the resize cache re-read the two rows and the file the acquisition had just written, so every acquired image cost two extra queries and a second full read of a file whose bytes were still in memory. processItem now hands back what it persisted and precache warms from that, under the same cache key the serving path computes. Resolving the admin user also moves behind the empty-queue check: it is needed only to resolve private playlists, so an idle server no longer runs a user lookup on every poll. * perf(artwork): keep the worker pool fed across a drain The pool was fed from a batch sized to the pool itself (2x concurrency) with a WaitGroup barrier before the next dequeue, so one item burning its external timeout idled every other slot until it finished. The legacy cache warmer had no such barrier: it streamed through a pipeline of 4. Dequeuing well past the pool keeps the slots fed for the whole pass at no extra cost, since DequeueBatch does not mark rows taken and was already one query per pass. Acquiring a slot now also observes cancellation, so a larger batch cannot delay shutdown. * perf(artwork): drain local and external artwork in separate pools A first backfill enqueues artists before albums at a single priority, so the drain took them in that order. Artists resolve through a rate-limited agent, and gate() waits for its permit while holding a worker slot, so the whole pool sat asleep in the limiter with every album queued behind it. Measured on a 96k-track library (29,115 artists to 6,949 albums, 4:1): zero albums resolved in seven minutes, and roughly 3.3 hours before the first album cover would have appeared. Splitting the drain gives each class its own slots: albums now finish in under eight minutes while artists trickle at the same 2/s they were always limited to. The two budgets are carved out of MaxOpenConns so a second pool cannot take connections the scanner and the UI need. Dequeue filters by kind, and the drain index leads with item_kind so each pool seeks to its own work instead of scanning past the other's backlog. * fix(artwork): treat an unreadable upload as a failure, not a miss resolveLocalFile swallowed every os.Open error, so uploads, playlist sidecars, a local M3U image and the artist image folder still had the bug that was fixed for folder and embedded sources: a permission or transient I/O error on a file that exists read as "no image here". The worker then settled the item absent and dropped its queue row. Uploads outrank every other source, so an unreadable one now stops the chain rather than letting a lower-priority image be persisted in its place. A genuinely missing file stays a clean miss. Reported by Codex on #5847. * fix(artwork): make the pool-split worker test race-clean CI runs the suite under -race, which the local `make test` does not, so this only showed up there: 230 specs passed and the detector still failed the run. The drain-pools spec started Run and never waited for it, so pool goroutines outlived the spec and raced the config snapshot Ginkgo restores on cleanup. It now cancels, unparks the blocked lookups and waits for Run to return. Two test doubles also had to become concurrency-safe, since the spec is the first to resolve several artists at once: fakeImageAgent's call counters, and MockAlbumRepo.GetAll, which records the last query options on a read path. MockDataStore's lazy accessors get the same treatment — only MediaFile was guarded before, and two pools now reach them concurrently. ArtworkQueue takes an unlocked helper for its internal Artwork call, since repoMu is not reentrant. * perf(artwork): stop reading and hashing disc art on every request The resize-cache key was the content hash, which cannot be computed without reading the file, so a warm cache never prevented the I/O: every sized disc request read up to 20MB and hashed it before the lookup. The legacy reader keyed on the id and the album's mtime and touched the file only on a miss. Disc art has no state row and therefore no stored hash, so the key is that same identity — id, album mtime, DiscArtPriority — and the selection chain now runs only when the cache misses. Full-size requests stream the source instead of buffering and hashing it. This matters more now that single-disc albums keep running the disc chain, which puts every disc-tagged track without embedded art on this path. * fix(artwork): key disc art on folder image changes, not just the album The identity cache key used album.UpdatedAt alone, which a replaced disc image does not necessarily move — the sized response would then serve the old image indefinitely. The legacy reader folded ImportedAt and the folder's ImagesUpdatedAt into its key for exactly this reason, and loadAlbumFoldersPaths already returns that timestamp; the disc reader was discarding it. * fix(artwork): return undispatched items when a drain is cancelled claim() reserves the whole batch before dispatch, but the cancellation path returned without releasing what it had not yet started, leaving those items in the in-flight set permanently — no later drain could claim them again. Harmless until the batch grew past the pool size; now a cancel strands up to a full batch. The e2e harness cancels mid-drain after every acquire, so it surfaced there first: one spec timed out waiting for an item that had been claimed and abandoned, and the suite went from 59s to 87s on CI. * fix(jellyfin): make a track's own cover reachable for Jellyfin clients Media files are never enqueued — the scanner drops their state without queueing them and the recheck kinds exclude them — so an unresolved track keeps an empty ImageHash and SongToBaseItem always fell through to AlbumPrimaryImageTag. Finamp then asks for the album image, nothing ever requests mf-, and the read-through that resolves the track never fires: the own-cover branch was unreachable for Jellyfin-only users. An eligible, unresolved, not-known-absent track now advertises its id as the Primary tag, which is what makes the client ask. The request serves the embedded art and queues the track so the worker persists a real hash. No blurhash is sent, since none exists yet and a fake would be cached against that tag forever. Serving gains the album fallback that made this safe to advertise: an eligible track whose frame will not extract now falls back the way CoverArtID does instead of answering with a placeholder. Reported by Codex on #5847. * fix(artwork): treat an unreadable artist-folder image as a failure Third and last site in this class: findImageInFolder logged and skipped an image the glob had already matched, so a permissions or mount failure during the artist-folder traversal read as "no image here" and let processItem settle the artist absent, discarding any artwork already resolved. A matched-but-unreadable file now propagates through fromArtistFolder and lands as localError, the same as album folder art, embedded art and uploads. A folder with no match stays a definitive miss. Also normalizes the e2e path assertions with filepath.ToSlash: the stored SourcePath is OS-native, so the forward-slash suffixes failed all 23 folder specs on Windows. Reported by Codex on #5847. * fix(artwork): give full-size disc art a real ETag Regression from 04d5a556. Keying the resize cache on identity meant the disc response no longer carried a content hash, and the full-size branch set no ETag either — so WriteImageHeaders fell back to the empty hash and emitted `ETag: ""` for every full-size disc image. Since ifNoneMatch compares the unquoted value, a client echoing that back matched, and got a 304 even after the image was replaced. The full-size branch now carries the same identity validator the sized branch uses, so it moves when the folder's images change. WriteImageHeaders is hardened against the class as well: an empty validator is no validator, so it is neither emitted nor matched. Reported by Codex on #5847. * test(artwork): inject the unreadable source instead of chmod os.Chmod cannot revoke read access on Windows — it only toggles the read-only attribute — so the findImageInFolder spec opened the file happily and failed there, and the upload spec passed for the wrong reason: outcomeFailed came from the 1-byte payload failing to decode, not from the source being unreadable. findImageInFolder takes an fs.FS, so the failure is now injected and the spec is filesystem-independent. The upload path goes through os.Open directly and has nothing to inject, so it skips on Windows rather than pretend to cover it. * fix(artwork): compare the pixel cap without multiplying Defence in depth rather than a live hole: the reported crafted PNG (0xffffffff square) never reaches the multiplication, because image/png rejects it at DecodeConfig, and the largest dimensions any supported format can declare — 2^30-1 for PNG, 16-bit for JPEG and GIF, 14-bit for WebP — cannot overflow the int64 product. decodeCapped is format-agnostic though, so the guard should not depend on a decoder's own limits staying where they are. Comparing by division holds for any dimensions a decoder might report, and non-positive ones are now rejected outright. * refactor(ui): rename cover artwork components * fix(ui): remove Artwork rendering gate Signed-off-by: Deluan <deluan@navidrome.org> * fix(cache): re-fetch when a cache entry outlives its data file fscache's Remove drops the in-memory entry, releases the lock, and only then unlinks - blocking until every outstanding reader closes. A Get landing in that window re-creates the file at the same path under a fresh entry, and the deferred unlink deletes those new bytes. The entry survives pointing at nothing, and since a present entry is treated as a hit, every later Get for that key returns ENOENT for the rest of the process's life. Only a restart, which rebuilds the map from disk, cleared it. Get now drops such an entry and retries once, so a vanished data file costs one re-fetch instead of poisoning the key permanently. This also covers a file disappearing for reasons unrelated to that race, such as external deletion or a restored backup. Specs cover an in-process entry, one adopted at startup, and the deferred-removal race itself. * fix(artwork): log why a sized cover fell back to the placeholder serveHash routed every non-cancel cache error into dangling(), which returns ErrUnavailable and is then rendered as a placeholder at 200 OK. A cache-layer fault was therefore indistinguishable from an album genuinely having no artwork, and left no trace: a broken resize cache silently served placeholders for a quarter of the library while the logs stayed clean. Log the error before falling back, so the cause is recoverable from the logs. * fix(artwork): precache the cover variant the UI actually requests precache built its resizedItem without setting square, so it warmed h-<hash>.<size>.false.<quality>. The list surfaces - album grid, artwork avatars, playlist and radio details - all request square covers, so the warmed entry was never read and every grid cover stayed a cold miss on first view. Set square on the precache item so the key matches the request path. The existing specs asserted the '.300.false.' key and were updated accordingly. * fix(cache): give a re-created cache entry its own file Create re-opened the path with O_TRUNC, which shrinks the file out from under an older stream that may still be serving readers. stream.Reader then hits EOF from the OS at the new, shorter length while the broadcaster still reports the original size, so Wait() reports 'more data exists' and the reader retries forever - a tight pread loop that burns a core and never releases its handle, which in turn blocks Stream.Remove() indefinitely. Unlink first and create with O_EXCL so the new entry gets a fresh inode. Existing readers keep their descriptor on the old inode, see its full contents, and reach a clean EOF. Note this does not address the deferred unlink deleting the re-created file, which is handled separately by the re-fetch in fileCache.Get. * fix(cache): keep the in-place truncate on windows Unlinking before re-creating fixes the premature-EOF spin on unix, but Windows refuses to remove a file another handle still has open and returns a sharing violation. Because Create surfaces that error, every cache miss on a path with a live reader would have failed outright - worse than the spin it was meant to fix. Split the create behind a build tag: unix unlinks for a fresh inode, Windows keeps truncating in place and stays exposed to the spin, which is the behaviour it already had. The unix-only spec is skipped there. * fix(artwork): shape the blurhash placeholder to the artwork's aspect ratio The UI decoded every blurhash into a 32x32 bitmap and stretched it to fill its container, so a non-square cover showed a full-box blur that collapsed into a letterboxed image the moment it loaded. On the album detail page the placeholder overhung the image by a third of the box height. A blurhash string carries no aspect ratio of its own, so the dimensions have to come from the server. artwork.width/height were already stored and read by nobody; they now surface on ItemImage as imageWidth/imageHeight, hydrated through the join that was already in place. Existing rows already carry them, so no migration or rescan is needed. A square request is padded rather than cropped, which aspect-fits the content inside the square the server returns. That made the grid a second instance of the same bug, so `square` now implies contain for the image as well as the placeholder, instead of the two renderers reading it differently. * feat(jellyfin): expose PrimaryImageAspectRatio Real Jellyfin carries width/height of the Primary image on BaseItemDto (MediaBrowser.Model/Dto/BaseItemDto.cs), attaching it only when the request's Fields asks for it (DtoService.cs ContainsField). The dimensions are now on model.ItemImage for the web UI's blurhash placeholder, so the adapter can report the same thing for free. Gated behind Fields to match, and omitted rather than defaulted when the item has no image or unknown dimensions: real Jellyfin falls back to a per-type default of 1 for music, but a wrong ratio mis-shapes a client's placeholder, and we only lack dimensions when the artwork is genuinely unresolved. primaryImageTag becomes primaryImage, returning the tag, blurhashes and ratio together, so the choice of which image is Primary is made once per mapper rather than the same ItemImage being threaded through two calls. ArtistToBaseItem and PlaylistToBaseItem take Fields now, like the album and song mappers already did. * refactor(model): give ItemImage an AspectRatio method The zero/absent guards around width/height are ItemImage's own invariant, not the Jellyfin adapter's. Moving them onto the model keeps one definition for every consumer, so a second one cannot quietly disagree about what an unknown ratio means. * fix(ui): anchor detail pages to the top when opened from a list React Router keeps the previous page's scroll offset, so opening an album from a scrolled list started the detail page mid-song-list. Artist pages had the same bug; it just shows less because the artist list is rarely long enough to scroll far. Keyed on the record id rather than mount, so detail-to-detail navigation (an album's artist link) resets too, and so the scroll waits for the record instead of firing against an empty page. * fix(ui): stop the Random album grid collapsing on every keystroke The grid was replaced by a spinner whenever the random list was loading, so each search keystroke collapsed it to spinner height and back. That blanket blanking is the flicker commit 9e559311a removed for every other list; random kept the exception so a re-roll would not flash the roll it is replacing. Blank on a seed change instead of on any load: a re-roll gets a new seed and still blanks, while a search keeps the seed and leaves the grid in place. The seed is tracked from empty rather than from the current value because a re-roll redirects and remounts the grid, which would otherwise look already-settled with the previous roll still on screen. Same rule for the pagination, which was hidden on the same condition. * refactor(artwork): move fingerprint property key const to `consts` package Signed-off-by: Deluan <deluan@navidrome.org> * refactor(tests): enhance database handling with resettable tables and truncation Signed-off-by: Deluan <deluan@navidrome.org> * refactor(artwork): narrow the prune lock and drop redundant in-flight tracking The prune read-lock wrapped all of processItem, including external fetches under their own timeout. Since a pending RWMutex writer blocks new readers, one prune arriving behind a slow provider stalled every subsequent item in both drain pools. Extract persist() so the lock covers only the window it protects: store placement plus the two row writes. The in-flight set guarded against a queue row appearing twice in one batch, but artwork_queue's primary key makes that impossible, drains are serial per pool, and the pools' kind lists are disjoint. Removing it also retires the cancellation unwind loop that existed only to release those claims. * fix(artwork): never settle absent for a kind no recheck job revisits The 12h retry budget hands a bare failure to the periodic stale-absent sweep, which is what makes the resulting absent row recoverable. Media files are deliberately excluded from that sweep -- they resolve embedded only, at scan or on view -- so exhausting the budget on a transient read error recorded a "this track has no cover" verdict that nothing would ever revisit. Settle absent only for kinds a recheck job covers. Without a row the track stays unresolved, so the next view re-enqueues it. * refactor(artwork): remove dead plumbing from the serving path artworkReader.LastUpdated had no callers: invalidation rides entirely on the cache key, so the interface member, the resizedItem field and its four assignments were vestigial. Reader's second return value was likewise discarded at all three call sites. resizedItem.Key duplicated representationTag's format string over identical inputs, where drift would serve a wrong-keyed entry under a right-looking validator; it now derives from it. newResizedItem had one caller and a doc comment claiming a sharing with worker.precache that never existed -- precache builds its own literal. Also unexport Prune, which no caller outside the package used while RunPrune documented itself as the only sanctioned path, drop a single-call placeholder wrapper, and delete five fakeFolderRepo fields no spec ever set. * refactor(artwork): drop queue repository methods only tests called MarkFailed and Delete had no production caller: the worker only ever uses MarkFailedIfUnchanged and DeleteIfUnchanged, which refuse to act on a row a concurrent scan re-enqueued. Keeping the unconditional pair meant the interface offered the racy variant under the more obvious name. MarkFailedIfUnchanged does not build on MarkFailed, so nothing in the implementation needed them either. The repository tests used them to put a row into a backed-off state; they now do that directly. * test(artwork): pin that a request never fetches or samples album art resolveItemLocal's guard against the remote ExternalImageURL fetch and the 2x2 grid had no coverage: deleting it left the whole suite green while putting synchronous network calls on the request path. The worker resolving the same playlist is asserted alongside, so the spec cannot pass by simply resolving nothing. * refactor(artwork): give the resolver a receiver and one capability field The resolve* chain walkers each took seven parameters -- ds, agents, ffmpeg, gate, localOnly -- while the package already had workerDeps bundling the same collaborators for processItem. They are now methods on a resolver. The external capability is one nilable field instead of three values that had to agree. Previously a local-only resolution passed agents=nil, gate=denyGate and localOnly=true, and only the localOnly check actually protected anything: the external branch dereferences agents in the loop header, before the gate closure runs, so denyGate could never fire. It is deleted. A nil ext now both marks the resolution local-only and removes the agents there were to dereference, and newLocalResolver takes no parameter that could supply one. The playlist tile loop hardcoded localOnly=false, safe only because an early return 22 lines above it made that unreachable; it now inherits the resolver's capability. * refactor(artwork): funnel every served representation through one path serveHash, serveBytes and serveDisc each hand-copied the same five steps -- test for full size, stream or build a resizedItem, call the cache, wrap with a validator -- with a different error policy bolted on. The ETag rule was restated at four sites and applied inconsistently. serveSource now states it once: full size streams open() directly, and an ETag is attached only when the bytes are resized or there is no hash to validate against. Each caller keeps just its own error policy, and serveBytes folds into its single caller. Two behavior changes fall out, both narrowing an aborted request's blast radius: serveDisc propagates context.Canceled instead of falling back to a full album resolution, and serveHash's full-size path propagates it instead of going dangling, which would have enqueued a re-resolution for a request nobody is waiting on. * style(artwork): use one log prefix, spelled the way the codebase does The package logged under three spellings of its own name -- "artwork: " lowercase, "Prune: " and one "Artwork: " -- and the lowercase ones carried lowercase message text, against 568 capitalized to 48 lowercase elsewhere. Prefixing itself is the convention here (Scanner:, API:, Watcher:) and it earns its place: DevLogSourceLine is off by default, so without it a line does not say which subsystem emitted it. So this normalizes the spelling rather than dropping the prefix. Error strings stay lowercase and unprefixed per Go convention. * refactor(artwork): give workerDeps only what the processor uses The bag carried cache, which processItem never reads and only the worker's precache uses, and carried agents/ffmpeg/gate solely to reconstruct a resolver on every queue item. cache and ffmpeg move to Worker, where precache actually uses them, and the resolver is built once in NewWorker. persist's hash parameter was redundant: decodeArtwork sets Hash and GetImage selects it, so art.Hash already holds it on both paths. The type itself now lives beside Worker, which owns it, rather than in the file of the function it is passed to. * refactor(artwork): make acquisition a processor with its own receiver workerDeps was a parameter bag threaded into two free functions that nothing outside the worker calls. It becomes the processor type, with processItem and persist as acquire and persist methods on it, and Worker holds one collaborator instead of reaching through a bag. Kept as a separate type rather than folding onto Worker: acquisition takes a queue item and returns bytes, while Worker.process settles the queue row around it. That boundary is what keeps retry policy out of the image pipeline, and what lets the acquisition specs build a three-field value instead of a Worker with drain pools, gates, a broker and a real on-disk cache. * refactor(artwork): collect the external gate contract in one file gateFunc, passthroughGate and isTransientExternal sat in agent_images.go while every implementation lived in worker.go: extGate, breaker, Worker.gate, gateFor. isTransientExternal even carries a comment saying it must stay consistent with breaker.record, which was in the other file -- a rule spanning two files with only a comment holding it together. Pure move into gate.go: no symbol added or removed. * refactor(artwork): put the whole playlist grid in playlist_cover.go decodeTile and assembleTiles were in resolve.go while the geometry they depend on -- rect, fillCenter, tileSize -- was in playlist_cover.go and used nowhere else, so one file held the grid's helpers and another its assembly. Pure move. * refactor(artwork): move resizedItem next to the interface it implements resizedItem is the only implementation of artworkReader, which is declared in image_cache.go, and it is used by the worker's precache as well as the serving path -- so worker.go was reaching into serving.go for a cache type. representationTag stays in serving.go, where the HTTP validator belongs. Pure move. * refactor(artwork): fold Refresh into housekeeping refresh.go was a 21-line file for one function that clears artwork state and enqueues -- the same thing Backfill, EnqueueStaleAbsentAll and EnqueueMissingAll already do next door. Pure move. * refactor(artwork): accumulate priority-chain state in one place Every source in the album and artist chains repeated the same five lines: stamp the accumulated external failure onto a hit, or OR the local fault into the running total on a miss. Each new source was a chance to forget the OR, which is how the local-I/O-settles-absent bug happened. chainState.try does both, so each case drops to three lines and the omission is no longer expressible. Semantics are unchanged: a hit still carries extErr only. Also drops localErr from resolvePlaylist, which declared it but never assigned it. * refactor(artwork): expose housekeeping through the Worker scheduleArtworkHousekeeping received a *artwork.Worker and then called CreateDataStore() for a second handle onto the state that Worker already owns, because Backfill, EnqueueStaleAbsentAll and EnqueueMissingAll were free functions taking a DataStore. They are now Worker methods over unexported implementations, the same shape prune/RunPrune already uses: one public path, and the specs keep calling the plain function with a mock store instead of standing up a Worker. Fingerprint is unexported too -- nothing outside the package used it. * refactor(artwork): name the service for its domain, not its role Every other core interface is named for what it is -- Playlists, Library, Scrobbler, MediaStreamer -- while this one was artwork.Service, the only X.Service in the tree. It becomes artwork.Artwork/NewArtwork, and serving.go follows the type to artwork.go. ProvideImageStore was likewise the only "func Provide" in the repo; it is GetImageStore now, matching GetImageCache beside it. cmd/wire_gen.go regenerated via make wire. * docs(artwork): give each Worker housekeeping method its own godoc The three methods shared one comment attached to Backfill, so godoc rendered the other two undocumented and the one it did show described the group rather than the call. Each now opens with its own name and says what that call does, including Backfill's bool return. * refactor(artwork): move fakeFolderRepo to artwork_suite_test.go Signed-off-by: Deluan <deluan@navidrome.org> * refactor(artwork): unexport artwork.HashImage function * fix(log): stop ShortDur eating significant trailing zeros TrimSuffix(s, "0s") was meant to turn "4h0m0s" into "4h", but it strips any trailing "0s"/"0m" -- so 10s logged as "1", 20s as "2", 1m30s as "1m3", 2h30m as "2h3", and a zero duration as the empty string. Every elapsed/duration field in the app was affected. The suffix now has to include the preceding unit, so only a whole zero-valued component is dropped. The existing table only covered values that dodge the bug (4m, 4h, 4m3s); added the ones that don't. * feat(artwork): prefix every log message and time the slow steps Prefix: 22 messages still logged unprefixed, so a line from this package was indistinguishable from any other subsystem's. All 40 now carry "Artwork: ", matching Scanner:/API:/Watcher: -- which earns its place because DevLogSourceLine is off by default. Timing on what can actually be slow: total per acquisition (on every exit, failures included), the read that also covers the provider download, hashing, decode+blurhash, resize, drain batch, precache, prune, backfill, and the external agent call -- with the rate-limiter wait counted separately, since a throttled agent and a slow one look identical from the drain. Debug coverage for states that were previously silent: dedup hit vs decode, settling absent, serving a lower-priority source after an external failure, retry scheduling with attempts and budget left, giving up when the budget runs out, breaker open/close per agent, provisional read-through, dangling state rows, and the mtime mismatch that makes art appear to vanish. outcome gained a String() so it reads as a name. * refactor(artwork): log decoded dimensions as fields, not a formatted string fmt.Sprintf ran on every newly-decoded image even with Debug off, since Go evaluates log arguments regardless of level. Separate width/height fields also query better than a "300x300" string. Correction to e0f1acd1a's message: it said "all 40" messages carry the prefix. The package has 68 log call sites and 62 distinct messages; 40 was only what the test suite happened to exercise. The sweep itself was complete -- zero unprefixed messages remain. * fix(subsonic): stop serving artwork for a deleted radio artworkAccessible checked every kind except radio, which fell through to the default "no per-user access control" branch and returned true without a lookup. Radio deletion removes only the entity row -- item_artwork and the uploaded file survive until the next prune -- so the old ra- id kept serving the removed radio's image for up to a day. This is a regression against master, where newRadioArtworkReader loaded the radio first and returned its error, so deletion took effect at once. Radios stay globally visible; only existence is checked. CreateMockedRadioRepo left Data nil, so its own Put panicked on first use; initialized it. Reported by Codex on #5847. * fix(artwork): stop serving artwork for entities that no longer exist Artwork state and its bytes outlive a deleted entity until the next prune (@daily), and the serving path consulted only item_artwork, so a Subsonic id or a signed public token kept serving a removed entity's image in the meantime. Master's readers loaded the entity first, so this was a regression. The check goes in serveHash, the one path that can hand back a found row's bytes: absent rows are already unavailable, and the provisional and disc paths load their entity to resolve at all. Doing it there instead of per-handler also settles who owns the invariant. Subsonic had worked around it with artworkAccessible, whose comment described the service "bypassing the library and private-playlist filters"; that workaround is now deleted, since the service resolves through the request-scoped repositories and enforces the filters itself. Because those repositories are ctx-scoped, each caller says what it wants by what it passes: Subsonic hands over the request context and so gets visibility as well as existence, while the public image route elevates like the Jellyfin one already did -- a token is the authorization there, and a visibility check would hide a shared private playlist, the very case shares exist for. Two supporting fixes. albumRepository.Exists and mediaFileRepository .Exists used the plain exists() helper, which applies no library filter, so they reported rows in libraries the caller cannot see; they now count through applyLibraryFilter as CountAll and artistRepository.Exists already do. Neither had a production caller. RadioRepository gained the Exists it lacked. Tests follow the layers: the service refuses a found row whose entity is gone, the repositories hide rows the caller may not see, and the handlers only assert the context they hand over. Jellyfin needed no change -- resolveArtworkID probes the entity tables, so a deleted item yields an empty artwork id -- but that protection was incidental and untested, so it is pinned now. * fix(playlist): rebuild the generated cover only when the tracks change The enqueue sat in refreshCounters, which Put also reaches for an ordinary metadata update, so renaming a playlist or editing its comment re-resolved the cover. The 2x2 grid samples albums with random(), so that silently handed the playlist a different cover for an edit that touched no tracks -- and refetched remote artwork to do it. It now happens where the track set actually changes: addTracks (which Put-with-tracks and updatePlaylist both funnel through) and renumber (reached from removeOrphans). Creation still enqueues even with no tracks, since an imported m3u can carry an ExternalImageURL. Reported by Codex on #5847. * style(artwork): trim verbose comments to the 1-2 line budget Comments only; no executable code changed. Verified by comparing the Go token stream of every touched file before and after: identical. Removes 375 of the 1104 comment lines this branch added, targeting content that belongs in a commit message or PR body rather than in the code: rejected alternatives ("DeleteIfUnchanged, not Delete", "Waking all beats routing by kind"), refactor history ("as the legacy reader did"), issue references (#5798, #5597, #5376), benchmark numbers (~400ms, ~16k allocs), and four persistence doc comments that duplicated the interface godoc in model/artwork.go verbatim. Comments predating this branch are left untouched. The ASCII fixture trees in the e2e suites are deliberately kept above the line budget: they diagram the fixture layout with its expected outcomes, and every pre-existing block in those files carries one. * test(artwork): make the artists-first backfill assertion non-vacuous The follow-up loop could never fail: once the first "ar" index is asserted to be 0, every non-"ar" element is necessarily at an index greater than 0. A sequence like ["ar", "al", "ar"] passed both assertions, which is exactly the interleaving the check exists to forbid. Assert the partition directly instead: nothing after the first non-artist call may be an artist. Verified by mutation — enqueueing artists a second time after albums now fails the spec. * refactor: use stdlib slices/maps and utils helpers in artwork code Mechanical cleanups, no behavior change: - 15 copies of the same id-extraction loop collapse to slice.Map (5 repo mocks, the scanner's track sweep, 4 wantIDs assertions) and slice.ToMap (6 index-by-id loops in the hydration specs). - disc.go built a map[string]bool purely to dedup folder ids and then walked it back into a slice; slice.Unique says that directly. - folders_artist.go's image filter is slice.Filter over model.IsImageFile. - mock_artwork_repo deleted from a map while ranging it; maps.DeleteFunc states the intent. - sort.Slice -> slices.SortFunc + cmp.Or; math.Min/Max -> builtin min/max; make+copy -> bytes.Clone; strings.Split -> SplitSeq on a per-request path; three-clause pixel loops -> for range. - Reuse utils.BaseName where a stem was recomputed by hand. Not at playlist_cover.go:27: that path is a full OS path and utils.BaseName uses path.Base, which does not split backslashes. - Drop a dead nil-guard in agents.go: getAgent returns a bare nil interface, and a type assertion on nil already yields ok == false. cmp.Or was rejected for the gate fallback (func types are not comparable, does not compile) and for ItemArtwork.AttemptedAt (cmp.Or compares time.Time with ==, which includes loc; IsZero does not). * refactor(persistence): collapse the four hydrateArtwork copies into one generic album/artist/playlist/radio each carried the same eight lines, differing only in element type and model.Kind. hydrateItems takes a ref callback yielding an item's id and the ItemImage to fill, which is all that varied. The len()==0 guards drop out: hydrateItemImages already short-circuits an empty id list, and both loops are no-ops on an empty slice. applyItemImage stays as-is; hydrateMediaFileArtwork and its own specs still use it. * perf(scanner): stop reprocessing album and artist artwork on every full scan A full scan re-imports every track, so the per-entity artwork enqueue in persistChanges fired for every album and artist in the library — measured as 100% of both on a live instance, and ~8.5k artists / ~16.7k Deezer fetches on a 96k-file library. Re-importing a track is no evidence the art changed: artist art has no track-content source at all, and the albums re-derived to identical hashes across three consecutive scans. Enqueue through EnqueueIfMissing on a full scan, which anti-joins item_artwork so only entities that never resolved are queued. Incremental scans keep the unconditional Enqueue, where a re-import does mean the file changed. Doing this at the enqueue site rather than skipping it wholesale keeps a first scan filling in covers as it walks, instead of stalling every cover until the scan ends. EnqueueMissing grows a priority argument and runs once at end of scan, so an entity phase 1 never saw still resolves, at Scan priority rather than dropping to Recheck behind the backfill. * refactor(artwork): derive blurhash components inside Encode Components had a single caller, which only ever fed it the bounds of the image it then passed to Encode. Exporting it gave callers two ways to get it wrong — components mismatched with the image, or out of the 1..9 range — in exchange for a knob nobody turned. Derive them from img.Bounds() at the top of Encode and unexport the helper. The counts must come from the pre-downscale bounds: downscale's integer rounding can shift the ratio across a component boundary, and the hash is a client-side cache key. Verified byte-identical over 18 hashes spanning 9 aspect ratios. The out-of-range validation goes with it, being unreachable once the counts are always derived. The aspect-ratio table now asserts through Encode's size flag, which encodes (x-1)+(y-1)*9. * refactor(ui): replace the blurhash package with a local decoder The UI pulled in the `blurhash` dependency for one function, `decode`, called from a single component. The decoder is ~80 lines of well-specified arithmetic, so carrying a dependency for it costs more in supply chain and bundle than it saves. Equivalence was proven against the package before removing it: 84 hashes — three real ones plus every component count from 1x1 to 9x9 — decoded at six sizes, compared byte for byte, plus parity on which malformed inputs throw. Those pixel values are now pinned in the spec, so drift from the reference algorithm fails. The punch parameter is dropped rather than reproduced: no caller passes one, and the package applies `punch | 1`, which silently turns a punch of 2 into 3. * refactor: simplify the artwork enqueue and blurhash paths Cleanup pass over the three preceding commits. Tabulate the cosine terms in the UI blurhash decoder instead of calling Math.cos per pixel per component: 248us -> 75us for a 32x32 decode, and an album grid mounts one decoder per tile. Output is unchanged, which the pinned pixel specs enforce. The Go encoder already tabulated the same terms. Drop the dead paths that deriving components inside Encode left behind: the zero-size guard in components, the post-downscale empty check, and the no-AC-factor branch, which cannot be reached now that the counts are always at least 1x9. The empty-image check moves ahead of the derivation, where it belongs. In the queue mock, look up item_artwork by its existing iaKey rather than scanning the map, and hold the lock across EnqueueIfMissing through a shared unlocked helper instead of releasing it mid-operation. Extract the duplicated drain-and-resolve block in the scanner specs into one helper. * fix(artwork): refresh songs when their album's artwork changes A track with no art of its own is served its album's, so hydration copies the album's hash onto the track record. When an album resolution changed, the worker broadcast only an `album` refresh, leaving song and now-playing surfaces holding the previous hash-suffixed URL until something else refetched them. Pair the album refresh with a song one. The dependent id list is unbounded — an album has arbitrarily many tracks and a drained batch arbitrarily many albums — so this refreshes the resource as a whole via the protocol's existing wildcard rather than enumerating ids. Reported by Codex on #5847. * feat(persistence): log SQLite result codes on failed statements SQLite reuses one message for errors that need different responses: "database is locked" is both SQLITE_BUSY, which busy_timeout retries, and SQLITE_BUSY_SNAPSHOT, which it can never retry because the transaction's read snapshot is already stale. Reading only the message, the two are indistinguishable, and a lock error seen in the wild could not be diagnosed without guessing which one it was. Add db.ErrorCodes to unwrap a sqlite3.Error and report its result and extended result codes, and include them in the SQL error log. The helper lives in db because that package already owns the driver, so persistence does not need to import it. Constraint, readonly and disk-full errors share messages the same way, so this applies to every failed statement, not just locks. * fix(ui): keep the Random grid blank while a refresh re-rolls Refresh bumps the list version, which changes the random seed and remounts the grid. useRollChanged tracked the seed on screen in a ref inside the grid, so the remount started it empty and adopted the new seed on the first render, while the refetch had not begun and the store still held the previous roll. The grid painted the old albums for the length of the request and swapped when the new roll arrived. Move the ref up to AlbumList, which a refresh does not remount, and pass it to the grid and the pagination. The seed on screen then survives the remount, so a refresh reads as a re-roll and blanks until the new roll lands. A search keystroke keeps the seed and still leaves the grid in place. * fix(ui): keep the album grid working outside the Random list Hoisting the shown-seed ref into AlbumList made the prop mandatory in practice: ArtistShow renders the same grid through ReferenceManyField and passes no seed tracking, so useRollChanged dereferenced undefined and the artist page died with "Cannot read properties of undefined (reading 'current')". Own a ref in the grid when none is passed. A caller with no roll to track then behaves as it did before, while the Random list keeps the ref that has to outlive the refresh remount. * refactor(config): make the artwork tuning options dev flags ArtworkWorkerConcurrency and ArtworkExternalMaxRPS become DevArtworkWorkerConcurrency and DevArtworkExternalMaxRPS, joining the other DevArtwork* flags. Their defaults should not need tuning, so they do not belong in the documented, user-facing option set. * refactor(artwork): name the hashed image store folder for how it is addressed The content-addressed store sat in artwork/store/, which did not distinguish it from the artist/, playlist/ and radio/ upload folders beside it — those hold artwork too. It is now artwork/hashed/, naming the one property that sets it apart, and the path comes from a consts entry rather than a bare literal, matching how the sibling folders are built. Deliberately not under cache/: that folder holds resizes that rebuild offline from a local source, while this one holds the only local copy of externally fetched images, whose re-fetch depends on a third party still serving them and whose bytes back the stored blurhash and dimensions. No migration: anything left in the old artwork/store/ is orphaned and re-resolved into the new location. * refactor(artwork): call the album e2e helpers by their real names album_test.go aliased expectAlbumFolderCover and expectAlbumAbsent to shorter local names, which cost a lookup to resolve and hid the prefix that distinguishes them from the artist and playlist helpers. * test(artwork): cover the album-root and artist-folder path arithmetic The unit specs for these helpers lived in the readers #5856 patched, both deleted here, leaving the album-root promotion and the artist-folder climb covered only end-to-end. A layout can show which image won but not why, so these pin the parts behind it: that the parent is fetched only when it could qualify as an album root, that a failed fetch propagates or degrades, and that commonDir keeps a shared name fragment from reading as a shared folder. Each spec was checked against a mutant: removing the library-root guard, the other-album audio check, the single-folder short circuit, the single-root climb, or commonDir's separator all turn one red. Master's remaining specs were dropped as redundant — they cover sources this branch already exercises under different names. * fix(ui): stop artwork refreshes reloading the whole page Resolving an album broadcast song:["*"], because a track with no art of its own is served its album's and the dependent ids are unbounded server-side. useResourceRefresh checked for that wildcard across every resource in the payload, not just the ones a component shows, so the album grid called refresh() — a full page reload — for every drained batch. Upgrading a large library reloaded the grid thousands of times. The client knows what the server cannot: which tracks are loaded, and their albumId. So the fan-out moves there, the wildcard check is scoped to watched resources, and the backend now names only what it resolved. The playlist views watch playlistTrack too: their rows carry albumId but are keyed by playlist entry, so a song refresh never reached them — previously masked by the wildcard's page reload. Signed-off-by: Deluan <deluan@navidrome.org> * fix(ui): stop the album grid blanking its covers on refresh Cover takes its height from react-measure, which reports nothing until it re-measures. That was harmless while the cover class sat on the img itself, which has intrinsic size; it now sits on the Artwork root, whose img fills it absolutely and so lends no height. A refresh remounts the tiles, and for the frame before measurement lands the box collapsed to zero and every cover vanished. Give the box its own aspect ratio as a floor. The measured height still wins once it arrives. * refactor(artwork): drop the unused Bump/wake path from the worker Worker.Bump had no production callers. Every real bump writes the queue row through the repository instead: artwork.Refresh (the refresh endpoint and the uploader), service.enqueue (read-through and stale-absent views), and radio_repository. None of them wake a pool. Bump was also the only sender to drainPool.wake, so the select case it fed was unreachable outside tests, and runPool read as if a bump were picked up promptly when it always waited for the 5s poll. The e2e and unit suites used Bump only as a driver, never as the subject, so they now enqueue with EnqueueBump and exercise the path production takes. EnqueueBump rather than Refresh because Refresh also clears the state row, which would change semantics for the re-bump-after-state and dedup specs. Both harnesses start Run after enqueueing, so a fresh pool drains on its first iteration and the wake never affected them: core/artwork/e2e averaged 3.26s before and 3.30s after over 5 runs, within noise. Signed-off-by: Deluan <deluan@navidrome.org> * refactor(artwork): unexport what nothing outside the package calls PlaceholderFor had no callers at all. Its comment offered it to callers that must not consult persisted state, but no such caller was ever written, so it is removed rather than unexported. entityExists and encode83 are only reached from inside their own packages; their tests are package-internal (artwork) or never touch them (blurhash_test). ImageStore stays exported: server/subsonic/e2e constructs one to wire up the worker, and that suite cannot move into package artwork. Signed-off-by: Deluan <deluan@navidrome.org> * refactor(persistence): name the backoff-preserving enqueue for what it does EnqueueBump was named for a priority, but its behaviour is preserving an existing row's retry_at; the priority comes from the item. Its one caller passes ArtworkPriorityScan, which read like a bug at the call site and is not. The three DO NOTHING inserts each repeated the INSERT prefix, the column list and the conflict clause. They now share insertIfNotQueued, with the CTE that EnqueueIfMissing needs passed as a prefix, and the column list lives in one slice used by both squirrel and the raw SQL. EnqueueIfMissing had no test against real SQL, only a mock mirroring the anti-join by hand, so the rewritten statement had nothing verifying it. Two specs now cover it: items with a state row are skipped, and an already-queued row keeps its priority. Signed-off-by: Deluan <deluan@navidrome.org> * test(thumbhash): vendor the reference and generate golden vectors * test(thumbhash): add a faithful reference port as the differential oracle * fix(thumbhash): re-condition alpha fixture and assert solid.png header alpha.png varied only alpha over a constant color, so compositing atop the average canceled L/P/Q exactly like solid.png, leaving it just as float-noise unstable and silently dropping alpha-path coverage. Vary R/G/B with position too so alpha.png carries real signal and reproduces byte-exactly, then replace the blanket solid.png skip with a precise assertion on its well-conditioned header bytes and quantized-zero scales. Also apply a range-over-int modernizer hint in reference_test.go. * test(thumbhash): dither the fixtures so no DCT coefficient is degenerate The gradient fixtures are smooth analytic ramps, and alpha.png's alpha channel varied along x only. Both make whole families of DCT coefficients mathematically zero: 13 of alpha.png's 38 AC nibbles, and at least one in every other ramp fixture. A zero coefficient normalizes to exactly the 0.5 midpoint, i.e. 15*f = 7.5, so which of nibble 7 or 8 it quantizes to is decided by ~1e-16 of float rounding noise. Those nibbles are therefore unstable across any two summation orders -- the vendored JS and the Go port already disagreed on solid.png for this reason -- and they carry no signal, so a bug that transposed two of them would be invisible. A deterministic +/-4 dither, plus an alpha ramp that varies in x and y, leaves every coefficient at least 4.8e-5 from the tie boundary: ~1e9 times the observed inter- implementation noise. solid.png and tiny.png regenerate byte-identical and keep their existing carve-outs. * feat(thumbhash): add a two-pass separable ThumbHash encoder Encode replaces the reference's four w*h scratch arrays (~320 KB at 100x100) and its 40 re-reads of every pixel with a single pixel pass that folds each row into per-frequency sums, then a second fold over rows. The transform drops from O(terms * pixels) to O(nx * pixels + terms * h), nx <= 7. Verified against the vendored JS goldens and, differentially, against the literal Go port on 500 randomized images. On the fixtures the two implementations' AC coefficients agree to 8e-15; the separable inner loop reassociates the additions, so exact agreement holds only where a coefficient is not sitting on a quantization tie, which is why the fixtures are now dithered. * test(thumbhash): fuzz the opaque path and drop an ill-conditioned golden The 500-image randomized differential filled every byte randomly, so hasAlpha (avgA < w*h) was true for all 500 images: the 7x7 no-alpha layout, terms(7,7), nx=7 and the `if hasAlpha` false branch were never fuzzed. Force full opacity on alternating iterations, which splits the run 250/250 with the seed and iteration count unchanged. All 500 still match the reference port. tiny.png is 1x1, so it has no non-zero AC content: 12 of its 37 AC coefficients sit exactly on the round(15*f) = .5 tie and 24 are within 1e-12. It passed only because a single pixel admits no summation reassociation. Give it the same header-only carve-out solid.png already had, in both test files. The four well-conditioned fixtures keep strict full byte equality. Also document the divergence class on Encode itself rather than only in test comments, add a sub-image regression spec, and use the max builtin over math.Max. The toNRGBA Rect.Min gate turns out to protect nothing — SubImage re-slices Pix so Pix[0] is the Rect.Min pixel and the loops read it correctly either way — so its comment, which claimed the opposite, is corrected. The gate is kept for now; removing it is a separate call. * test(thumbhash): benchmark against blurhash at the pipeline input size * feat(artwork): persist a thumbhash alongside the blurhash The artwork table is content-addressed, so this costs one row per unique image rather than per entity. Measured on a 25k-image library: +0.6MB on a 727MB DB. The column is added to the existing add_artwork_tables migration rather than a new one, since #5847 has not been released and the table it extends ships in that same PR. * feat(artwork): hydrate and expose thumbHash on every entity ItemArtworkInfo.Image() is the single projection every hydration branch goes through, so adding the field there covers albums, artists, playlists and both the own-art and inherited media-file branches. * feat(artwork): encode blurhash and thumbhash from one 100px thumbnail thumbnailSize drops 128 -> 100 so a single CatmullRom scale feeds both encoders; thumbhash hard-rejects anything larger and a second downscale would cost more than shrinking the shared one. decodeArtwork on a 1000x1000 JPEG, before -> after: 15.53ms -> 15.45ms/op, 5878122 -> 5014796 B/op, 37 -> 75 allocs/op Time is a wash because the JPEG decode dominates; the 863KB drop is the smaller thumbnail more than paying for thumbhash's added work. This rewrites every blurhash value, so it must land before #5847 reaches a release: Finamp keys its cover cache and download dedup on that value. * feat(ui): use thumbhash for the cover loading placeholder Adds a local decoder rather than the thumbhash npm package, matching the local blurhash decoder it replaces. Pixels are pinned against evanw/thumbhash's reference decoder via the vendored copy already in the encoder's testdata. Unlike a blurhash, a thumbhash carries its own approximate aspect, so a record with no dimensions now falls back to that instead of to a square. The blurHash API field stays: Jellyfin clients and third-party native clients still consume it. * perf(artwork): hand both hash encoders one NRGBA thumbnail makeThumbnail emitted a premultiplied *image.RGBA, so thumbhash allocated and un-premultiplied a full copy on every image while blurhash paid nothing. It now emits *image.NRGBA, which thumbhash wants as-is, and blurhash reads that type directly, premultiplying per pixel to keep its output identical. thumbhash.Encode at the pipeline's 100x100 input: 134200 -> 95300 ns/op, 56188 -> 15163 B/op, 37 -> 35 allocs/op decodeArtwork on a 1000x1000 JPEG: 5014796 -> 4973800 B/op, exactly the conversion that is gone The scaler costs the same into either destination (7.33ms vs 7.34ms measured), so no time is traded for this. * test(artwork): drop the decodeArtwork benchmark It existed to capture the before/after baseline for the 128->100 thumbnail change, which it has served. As an ongoing guard it is misleading: a 1000x1000 JPEG decode is ~97% of the 15ms it measures, so the two encoders it would be reached for are ~2.6% of the signal. It reported a phantom 2.8% regression for the NRGBA thumbnail change that isolating the scaler disproved. The per-encoder benchmarks in blurhash/ and thumbhash/ cover the part that actually changes. * test(artwork): benchmark both hash encoders from one file benchImage and BenchmarkEncodeAtInputSize existed in both encoder packages, and blurhash's copy had drifted into a third inline duplicate once both benches moved to NRGBA input. The head-to-head now lives in core/artwork, which already imports both encoders and can pin the input to the real thumbnailSize constant. The gradient builder moves to tests, which both packages already import via their suite bootstrap. thumbhash keeps a shipped-vs-reference benchmark, since referenceEncode is test-only and cannot be reached from core/artwork. * test(thumbhash): drop the shipped-vs-reference benchmark The two-pass rewrite's advantage over the naive port is already recorded in its commit message; carrying the benchmark to re-derive it has no ongoing use. reference_test.go stays: besides the benchmark baseline it holds the differential oracle the golden-vector and randomised specs assert against. * test(thumbhash): assert against reference goldens, drop the Go port The reference port existed to be a differential oracle, but its packing half was a verbatim copy of production pack(), so it was not as independent as it looked. Only the 500-image randomised spec needed it; the six PNG fixtures cannot reach random sizes, aspects, or both coefficient layouts. gen_generated.mjs now emits 300 vectors from evanw's actual JS. Their pixels are a pure function of their index, so Go rebuilds them byte-for-byte and only the hashes are committed (16KB). The oracle is now the reference itself rather than a hand transcription of it. Also from the cleanup pass: - maxCX/maxCY scanned every term to recover a bound that is just the widest coefficient region - tests.GradientImage duplicated generateGradientImage three files away - BenchmarkHashEncodersAtInputSize was also BenchmarkHashEncoders/*/100x100 - processor had two internal test files with no rule for which gets a new spec - three blurhash specs differing only by alpha became a DescribeTable - the shared mock's GetInfoForItems projection had drifted from the real query * refactor(model): keep the blurhash off native JSON and fold its specs Nothing on the native API consumes blurHash: the web UI reads thumbHash, and Jellyfin's mappers take the Go field directly rather than through this serialization. It was shipping ~40 unused bytes on every row of every list response, on a surface that accretes clients once published. The JSON specs were five marshal-and-check-keys blocks over the same struct; they collapse to one populated case, one bare case, and a guard that the blurhash stays off the wire. * refactor(persistence): simplify the artwork dangling-row purge helper purgeDangling took a bound executeSQL method value and a table name only because its two callers live on unrelated types. Neither parameter was load-bearing: executeSQL is a value receiver on sqlRepository that never reads tableName, and both callers already hold an sqlRepository whose tableName is the table being purged. Taking that struct collapses both parameters into the receiver. itemArtworkSQL wrapped sqlRepository without adding a single method, so the items field becomes a plain sqlRepository. danglingItemArtworkKinds is also read by EnqueueAllMissing, which enqueues entities that have no artwork row yet - neither a purge nor anything specific to item_artwork. Renamed to artworkOwnerTables to describe both of its uses. * fix(artwork): keep sweeping past a store file that cannot be removed Sweep returned the os.Remove error straight out of WalkDir, so a single unremovable file abandoned the rest of the walk. Every later orphan, stale mime variant and abandoned temp file then survived until the next prune, which would abort at the same place. Warn and carry on instead. The orphan-file loop in prune already had this resilience; Sweep is where it belongs, since it is the only step that reaches stray files and superseded variants. * refactor(artwork): let the sweep reclaim orphan files instead of prune Deleting unreferenced artwork rows took five round trips: snapshot the orphan hashes, fetch their mimes, delete, re-fetch to see which rows actually went, then remove each file. The last four exist only to work out which files to delete - but the sweep that runs moments later already derives exactly that from the database, since GetAllMimes is read after the delete and Sweep removes any file whose hash has no row, under the same mtime guard. The sweep is also the more thorough of the two: it reclaims stale mime variants of an orphan hash, which the per-hash removal never looked at. Delete the rows in one predicate-only statement and let the sweep follow. The orphan predicate now runs once per prune instead of once for the snapshot plus once per 200-hash delete chunk, and with no hash list to bind there is nothing left to chunk. GetOrphanHashes and GetImages had no other caller and are gone. Dropping them also removes the mock's OrphanHashes lever, which let a spec declare a hash orphaned while item_artwork still referenced it - a state the SQL repository could never produce, since both predicates were always identical. * refactor(model): name the artwork mime lookup for what it is keyed by GetAllMimes reads as a collection of mime types; it returns a hash -> mime index over every stored artwork. GetMimeByHash names the key, matching the convention the other map-returning repository methods already follow (CountBySuffix, CountByClient). * refactor(artwork): split the repository's deletes from its purges by name Six removal methods across the two artwork repositories used Delete and Purge interchangeably, so neither prefix told you anything. There is a real distinction underneath: Delete methods are caller-directed and take the rows to remove, while Purge methods are garbage collection - the repository finds rows whose referent is gone by joining against the owning table, and returns a count because the caller cannot know one. The signatures already followed that split; only the names did not. DeleteOrphans was the odd one out, taking a grace cutoff rather than a target and returning a count, so it becomes PurgeOrphans. PurgeDanglingItemArtwork loses the Artwork it repeats from its own interface and keeps the Items that says which of that interface's two tables it means. DeleteForItem was DeleteForItems with one id - squirrel emits = for a scalar and IN for a slice against the same predicate - so its one caller passes a slice instead. Dangling and Orphan stay distinct: dangling is a row whose entity is gone, orphan is an image no state row references. * refactor(artwork): drop the orphaned store remover, aggregate sweep failures Review follow-ups to the prune rework. ImageStore.Remove lost its only production caller when the sweep took over orphan-file reclamation. It carried its own copy of the mtime guard that now lives solely in Sweep, so the two could drift, and it stood as an invitation to grow a second reclaim path. Its specs either duplicated Sweep's own coverage or tested nothing else, so they go with it; the one unrelated test that used it to delete a file calls os.Remove directly. Sweep warned once per file it could not remove, which is fine for a stray permission bit and useless for a store mounted read-only - a large library would emit one line per aged file on every prune, and prune would still report success. Count the failures instead and warn once, with the count and the last error, so a systemic failure is legible without drowning the log. Also: the orphan log line counts rows, not files, and now says so; and the blocked-removal spec asserts its two fixtures really do land in different shards, since the assertion is vacuous if they collide. * fix(ui): show the placeholder when a refresh swaps in an uncached cover A cover whose hash changed under a mounted grid went blank for the whole fetch, instead of falling back to its thumbhash. Artwork decided 'this blob was already cached, skip the placeholder' once at mount and never revisited it, so every later url for that record inherited the answer. The fix has to live in useImageUrl. imgUrl is state cleared in an effect, so on the render where url changes it still holds the previous url's blob - any caller inspecting it reads the new url as already cached. Only the hook can see cache.get(url) for the url actually being rendered, so it now reports fromCache and resyncs its state during render rather than one render later. Two shapes were tried and rejected against a live server before this one: a ref recomputed per url, which a render-phase resync discards without rolling back the ref, and the same tracking in Artwork state, which still samples imgUrl before React re-runs the component. Verified on a running instance with cover-art responses held open: the thumbhash now covers the whole window and stays under the image until the fade ends. * fix(ui): keep the placeholder for a cover whose fetch failed useImageUrl caches a failed fetch as an entry with no blob, so treating every cached entry as instantly painted suppressed the placeholder on remount: the cover became an empty box instead of falling back to its thumbhash. Only a cached blob counts as instant. Caught by Codex on the previous commit, which introduced fromCache with the looser predicate. * feat(artwork): capture each image's dominant colour A flat colour is the one placeholder a client can paint with no decoding at all, so it can cover the frames before a thumbhash canvas even renders. Extract it from the same 100px thumbnail the two hash encoders already share, and expose it as dominantColor on the native API. Dominant means presence, not salience: the largest cluster wins, so a white sleeve reports white. That is what a placeholder needs. An accent colour is the opposite question and is deliberately not answered here - it depends on the client's theme, and by the time a client needs one it has the cover pixels and can compute whatever suits it. Bins at 4 bits per channel, then merges perceptually-near bins in Oklab before picking the winner: a gradient splits across adjacent bins and would otherwise lose to a smaller flat region. Measured at 246us per image on the weakest target hardware we care about (Celeron N5105), against ~15.8 images/s for the pipeline around it. The column goes into the feature's original migration rather than a new one: computing it later would mean re-decoding every image in the library. * fix(artwork): ignore image candidates that cannot be fetched bestImageURL only rejected URLs that url.Parse itself refuses, which is almost nothing: relative paths and any scheme parse cleanly. A candidate like images/big.jpg or ftp://host/big.jpg therefore won on size, and since the function returns a single URL whose failure ends that agent's turn, the agent's valid smaller images were never tried. Two details made it easy to hit rather than theoretical. Size is frequently 0 for every candidate, and only a strictly larger one replaces the first, so an unfetchable entry in first position sticks. And these strings come straight from plugins, where a relative path is an ordinary mistake. Accept only absolute http/https URLs with a host. * revert(ui): move detail-page scroll handling to its own branch Reverts 40153bd43. Scroll position carrying over between pages is stock React Router behaviour rather than something this branch introduced, so the fix does not belong in an artwork PR that is otherwise ready for review. The work continues on fix/scroll-restoration, off master, where it grew into per-route restoration: a new page starts at the top and going back returns to where you were, which the scroll-to-top could not do. * fix(ui): fade cover art consistently, and shorten the fade to 150ms The <img> mounts with its blob URL already set, so onLoad often fires in the same frame the element is inserted. The opacity:0 start state never gets painted, the transition has no value to animate from, and the cover pops in instead of fading. Only tiles whose decode happened to straddle a paint boundary actually faded, which made a grid load look ragged: 4 to 6 of 18 covers faded, varying between runs. Deferring the decoded flag by two animation frames guarantees the hidden state is painted first. Measured on a live grid afterwards, 17 of 18 covers fade, the 18th having a cached blob and taking the intentional instant path, and the tail after the last byte arrives drops from ~440ms to ~163ms. The duration also drops from 500ms to 150ms. That 500ms mirrored master's fade, but master paints no placeholder behind the image and needs a slow blend to cover a blank tile. Here a thumbhash already fills the gap, so a long crossfade only delays the grid settling, measured at 145-190ms slower to full opacity than master. --------- Signed-off-by: Deluan <deluan@navidrome.org>	2026-08-09 15:03:27 -04:00
Deluan Quintão	f853ca604a	refactor(db): migrate all ids to a uniform canonical 128-bit base62 encoding (#5824) ... * refactor(model): extract canonical 128-bit base62 id codec * feat(model): generate random ids as canonical 128-bit base62 values * feat(scanner): emit legacy PIDs in canonical base62 encoding * feat(db): add id canonicalization transform for the uniform-ids migration * feat(db): migrate all ids to canonical 128-bit base62 encoding * fix(db): canonicalize ids in junction tables and JSON columns * chore(jellyfin): update id-family notes for uniform canonical ids * test(ids): harden codec input contract and migration edge coverage * refactor(model): use log.Fatal for Encode128 contract guard per project convention * fix(db): force full rescan after id migration for legacy PID configs * test(db): guard id-column inventory against schema drift * refactor(ids): compile-time Encode128 contract and unified column rewrite helper * refactor(db): apply review feedback to id migration Filter empty strings in collectColumn's SQL, reuse a prepared statement for rewriteColumn updates, and clarify the legacy ID functions' comment now that they emit the canonical encoding. * feat(auth): split session and public-link JWT secrets, rotating sessions on id migration * test(subsonic): initialize public token secret in helpers suite The suite sets auth.TokenAuth directly instead of calling auth.Init, so the new PublicTokenAuth was nil whenever Ginkgo's spec order ran a helpers spec before any spec that calls auth.Init, panicking in publicurl.ImageURL. * refactor(db): inline canonicalID into its only consumer, the uniform-ids migration * refactor(model): rename Encode128/Decode128 to Encode/Decode With every id now exactly 128 bits, the width suffix is redundant; the package-qualified id.Encode/id.Decode carries the same information. * test(db): make the id-columns guard classify JSON columns too The guard only inspected columns named id/pid/*_id, so it could not see ids embedded in JSON. Widen it to *_ids and to every JSON column, and drive the "covered" set from a new embeddedIDColumns list instead of the inline calls in the migration. Every JSON column the schema has now carries a verdict. The four denormalized caches -- media_file/album.participants, media_file/album.tags, album.folder_ids and artist.similar_artists -- hold only artist, tag and folder ids. Those all come from id.NewHash, whose 22-char base62 encoding of a 128-bit MD5 is already in canonical range, so canonicalID is the identity on them and the migration correctly leaves them alone. A new codec test pins that invariant, since the exemptions depend on it. Verified on a copy of a 727MB/96k-track production database: canonicalizing those four columns changed zero rows, and artist, tag and folder ids were themselves unchanged by the migration (only media_file ids moved, 95108 of 96666).	2026-08-02 12:58:53 -04:00
Kendall Garner	b40b41584a	feat(subsonic): Implement OpenSubsonic topSongsByArtistId extension (#5853) ... * implement topSongsByArtistId extension * do not look up by artist name if empty --------- Co-authored-by: Deluan Quintão <deluan@navidrome.org>	2026-08-02 12:39:51 -04:00
Kendall Garner	4f29465d27	fix(artwork): do not promote artist folder to album folder (#5856) ... * fix(artwork): do not promote artist folder to album folder * use filesystem join for path instead * fix(artwork): resolve artist folder for albums with disc subfolders Dropping the promoted album root from the artist reader's paths fixed the flat-album case but broke albums whose tracks live in disc subfolders: the promoted root was what kept the byte-wise longest common prefix on a directory boundary. Without it, CD1/CD2 siblings share the fragment "Album/CD", so the artist folder resolved one level too deep and album/artist.* won over artist.*. loadAlbumFoldersPaths now collapses each album to its own root before the paths are compared across albums, so a disc-split album contributes its album folder rather than each disc folder. Folders no album claims (the promoted parent) are still returned unchanged, keeping the album, disc and mediafile readers unaffected. The prefix math moves into commonDir, which appends a trailing separator so the comparison lands on segment boundaries - this also fixes sibling folders sharing a name prefix (Album/Album2). Adds e2e coverage for the case the branch fixes (artist.* with no album/ fallback, which failed before this branch), the disc-subfolder regression, and a multi-album guard that pins the scope boundary. * test(artwork): cover artist whose albums are all disc-split The existing specs cover a single disc-split album and a disc-split album alongside a flat one, but not an artist where every album is split into disc subfolders. That layout resolves correctly because the albums diverge one level above the disc folders, which re-anchors the common prefix - a property worth pinning so a future change to the path math can't silently break it. * fix(artwork): resolve artist folder when albums share a folder loadArtistFolder decided whether to climb above the common directory by comparing len(paths) against 2. That count stands in for "how many distinct album roots are there", and the two diverge when an artist has two albums in the same folder: the slice holds two identical entries, the climb is skipped, and the artist folder resolves to the album folder. Deduplicate the roots so the branch tests the property it means to test. Replace the includeParent flag with two functions split by audience. The flag was a caller-identity switch - constant true at the album, disc and mediafile readers, all of which discard the returned paths, and constant false at the only caller that reads them. The artist path now has loadArtistAlbumRoots, which collapses each album to its own root and never consults albumRootParent; loadAlbumFoldersPaths goes back to taking a single album and always promoting the parent. Both share the folder load and image aggregation. This also makes albumRootParent's contract structural. Its guard only rejects an artist folder when it can find audio outside the album being resolved, so passing every album of an artist at once made the check meaningless; taking a single album means it can no longer be called that way. Drops the per-album grouping and unclaimed-folder reconciliation from the album path, where the result was discarded, and removes 15 mechanical true arguments from the tests. --------- Co-authored-by: Deluan <deluan@navidrome.org>	2026-07-29 21:48:42 -04:00
Deluan Quintão	23548f40a0	fix(share): give visual feedback when downloading from a share (#5865) ... * fix(share): give visual feedback when downloading a share The share page handed the download URL to navidrome-music-player, which fell through to downloadjs and buffered the whole ZIP into memory via XHR before saving it. Nothing was handed to the browser until the last byte arrived, so a large share produced a long silent window with no player feedback and no browser download UI, inviting repeat clicks that each spawn another server-side zip+transcode. Use the player's customDownloader prop to trigger a synthetic anchor instead, so the browser performs the download and reports its own progress. An anchor rather than assigning window.location.href: the share page's service worker registers a NavigationRoute over all navigations, which intercepts the streamed archive and fails it into the offline fallback (observed as HTTP 503 in Chrome). handleDownloads now loads the share before streaming so it can set Content-Disposition and Content-Type. This also fixes error reporting: ZipShare previously wrote to the ResponseWriter before checkShareError ran, locking the status at 200, so expired, missing and non-downloadable shares all returned 200. They now correctly return 410, 404 and 403. * feat(share): acknowledge the download click in the player The browser's download UI is the real progress indicator, but nothing in the page itself reacted to the click, so the moment before the browser catches up still read as unresponsive. Dim the download button and make it unclickable for two seconds after a download starts, reusing the JSS function-value pattern the existing single-track styling already uses. A repeat download restarts the window instead of extending the original, and the timer is cleared on unmount. This also blunts repeat clicking, where every extra click costs another server-side zip and transcode. Add SharePlayer tests covering the download mechanism and this state machine. The dimming itself is verified in a browser rather than jsdom: JSS function values are not evaluated there, so the rule is never emitted and a CSS assertion would pass or fail for the wrong reason. Signed-off-by: Deluan <deluan@navidrome.org> * test(share): assert render counts in SharePlayer feedback tests The two acknowledgement tests compared the props object across renders, which React may reuse, so they passed without proving anything and then failed once the surrounding assertions changed. Count renders instead, and let the pending timer run out rather than advancing exactly to its deadline, which does not cross it. The repeat-download test now also asserts that no render happens at the original deadline, proving the timer was replaced rather than merely that one eventually fired. * fix(share): count one visit per share download The preflight share load added in this branch made every download record two visits: handleDownloads called Share.Load, and ZipShare then loaded the share again internally. Share.Load increments and persists VisitCount, so the counter advanced twice per download and the repository work was duplicated. Pass the already-loaded share into ZipShare instead of its id. handleDownloads is its only production caller, and it now has the share in hand for the Content-Disposition header anyway. The archiver test asserts Load is not called, so the double-load cannot come back unnoticed. Verified against a running server: the counter now advances by one per download. * test(share): derive feedback-window timings from the constant The acknowledgement tests hardcoded clock advances tuned to a 2000ms window. Raising DOWNLOAD_FEEDBACK_MS to 5000 left them advancing 1500ms and 1001ms, which no longer reach the deadline they are meant to cross, so the repeat- download test passed without proving the timer had been replaced. Export the constant and derive the advances from it, and let the pending timer run out in the unmount test rather than advancing a fixed amount. Changing the duration can no longer silently strand a test short of its deadline. --------- Signed-off-by: Deluan <deluan@navidrome.org>	2026-07-28 16:13:44 -04:00
Deluan Quintão	fed9665060	fix(streaming): surface why a transcode decision failed (#5820) ... * fix(subsonic): surface the reason a transcode decision failed getTranscodeDecision returned a bare "failed to make transcode decision" with no clue why, and the probe command ran ffprobe with -v quiet, so even the server log bottomed out at "exit status 1". A user whose files had been moved by an external tool only saw the opaque error. ProbeAudioStream now returns a typed ProbeError that separates the file path from the reason: ffprobe runs with -v error so its stderr diagnostic is captured, and a missing or unreadable file is reported as "file not found" rather than ffprobe's misleading "Invalid data found". The handler logs the full detail (including the path) and returns the reason to the client with the server path stripped out. Reported-by: Tolriq (Symfonium) * refactor(ffmpeg): use errors.AsType for ExitError match probeErrorReason used the older var+errors.As form while the rest of the codebase (and its sibling transcodeFailureReason) uses the generic errors.AsType. Switch to it for consistency; behavior is unchanged. * fix(subsonic): return error 70 when the source file is missing A getTranscodeDecision probe failure was always reported as generic error 0. When the source file is gone (moved or deleted out from under the DB), that is a not-found condition, so return the standard Subsonic error 70 ("data not found") instead — matching what the endpoint already returns for an unknown mediaId. Files that exist but are corrupt or unreadable stay error 0. ProbeError now wraps the underlying cause and implements Unwrap, so the handler detects the case with errors.Is(err, fs.ErrNotExist). * fix(ffmpeg): keep probe error paths out of client-facing reasons Addresses review feedback on the ProbeError type: the Reason field doubled as both the log detail and the client message, so an ffprobe launch failure (a *os.PathError from fork/exec) could leak the ffprobe binary path to clients, and an unexpected stat error was reduced to "file not accessible" in the log. Split the two concerns: Reason now holds only a path-free, client-safe string (built at construction), while Error() logs the full underlying cause. Launch failures return a generic "could not read file" instead of the raw exec error. SafeReason no longer does substring path-stripping (removing the empty-Path edge case); the stripping happens once, against ffprobe's stderr. * fix(subsonic): don't report a broken ffprobe as a missing media file Two issues from review of the previous commit: Code 70 was selected with errors.Is(err, fs.ErrNotExist), but a launch failure of a deleted ffprobe binary is an *os.PathError that also wraps fs.ErrNotExist. A server-side ffprobe problem was therefore reported to clients as a missing media file. ProbeError now carries an explicit NotFound flag, set only on the file-access branch, and the handler keys the code off that instead of the chain. ffprobe can also exit 0 while yielding no audio stream (an audio-suffixed container holding only video). That parse failure was returned unwrapped, so clients got "internal error"; it is now wrapped in a ProbeError too.	2026-07-19 18:51:32 -04:00
Deluan Quintão	e6597398c2	feat(scrobbler): exponential backoff for scrobble retries during outages (#5818) ... * feat(scrobbler): add exponential backoff delay helper * feat(scrobbler): back off retries up to 4m during outages * test(scrobbler): verify backoff schedule with synctest; clarify backoffDelay doc Adds a testing/synctest-based test that drives the real run loop against a failing service and asserts the exact 5s/10s/20s/40s retry schedule and the drain-on-recovery reset, addressing the review note that the run loop's behavior was untested. Also clarifies the backoffDelay doc comment: the argument is a zero-based retry index.	2026-07-19 13:36:05 -04:00
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	2026-07-16 22:20:35 -04:00
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.	2026-07-16 20:47:47 -04:00
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.	2026-07-15 20:44:56 -04:00
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.	2026-07-15 18:09:59 -04:00
Deluan Quintão	fe6ac2e577	feat(jellyfin): experimental Jellyfin Music API support (#5730) ... * feat(sharing): enable sharing by default Flip the EnableSharing default from false to true so new installations have the sharing feature available out of the box. Users can still disable it via the EnableSharing config option. The native API only registers the /share route when sharing is enabled, so the nativeapi tests that build the router without wiring a share service now explicitly disable sharing in their setup to avoid registering a route backed by a nil service. * feat(jellyfin): add config flag and URL path constant Adds the disabled-by-default Server.Jellyfin config option (Enabled, ServerName) and consts.URLPathJellyfinAPI, following the existing LastFM/ListenBrainz patterns. Later tasks will use these to mount the Jellyfin-compatible API router. * fix(jellyfin): default Jellyfin ServerName to "Navidrome" * feat(jellyfin): package skeleton with System handshake endpoints Adds server/jellyfin: the Router (mirrors server/subsonic and server/public), its Wire-friendly New(...) constructor, a chi routes() table, and the ok() JSON response helper. Implements the unauthenticated handshake surface Jellyfin clients probe first: GET /System/Info/Public, GET+POST /System/Ping, and GET /QuickConnect/Enabled (quick connect is unsupported, so it always reports disabled). The public info payload's Id must be stable across restarts (Jellyfin clients cache ServerId), so it reuses the get-or-create Property pattern already used for InsightsID in core/metrics/insights.go: fetch consts.JellyfinServerIDKey from the Property repository, generating and persisting a new UUID on first read. A dedicated key (rather than the existing InsightsID) keeps the anonymous telemetry identifier from being exposed on this unauthenticated endpoint. * test(jellyfin): cover ping and quickConnectEnabled handlers Both were added alongside the System/Info/Public handshake endpoint but had no direct test coverage. * fix(jellyfin): memoize stable server Id and cover get-or-create path * feat(jellyfin): wire and mount the router behind Jellyfin.Enabled Add jellyfin.New to the shared Wire provider set and a CreateJellyfinAPIRouter injector, then mount the router at /jellyfin in startServer(), gated by conf.Server.Jellyfin.Enabled, mirroring the existing LastFM/ListenBrainz router mounts. * feat(jellyfin): add Jellyfin DTOs and model mappers Adds BaseItemDto, QueryResult, UserItemDataDto, UserDto, AuthenticationResult, MediaSourceInfo, PlaybackInfoResponse, NameGuidPair and SessionInfo DTOs to server/jellyfin/dto, plus model-to-DTO mappers for songs, albums, artists and genres that later browse/stream/write endpoints will consume. * test(jellyfin): cover GenreToBaseItem mapper * feat(jellyfin): advertise Jellyfin-compatible version, brand ServerName with Navidrome version * fix(jellyfin): map LastPlayedDate and omit zero track/disc numbers * feat(jellyfin): authentication middleware and AuthenticateByName login * fix(jellyfin): reject empty login password, wire ServerName, add negative auth tests * refactor(jellyfin): use new(x) builtin instead of intPtr helper * feat(jellyfin): user views and current-user endpoints * feat(jellyfin): Items query engine, item detail, and latest Adds the /Items universal query endpoint, dispatching by IncludeItemTypes over albums/artists/songs/genres with ParentId, SearchTerm, Filters=IsFavorite, SortBy/SortOrder and StartIndex/Limit support, plus GET /Items/{itemId} and /Users/{userId}/Items/Latest. Reuses server/subsonic/filter builders (by artist/album/starred) instead of hand-rolled squirrel filters, and resolves sort keys per item type since each repository maps sort names to different real/aliased columns. Also adds the missing CountAll to tests.MockArtistRepo, exposed by this task (the mock embedded a nil ArtistRepository for it and would panic on use). * refactor(server): extract shared query filter builders to server/filter Moves server/subsonic/filter to server/filter so server/jellyfin can use the shared query-option builders without importing server/subsonic, enforcing the rule that no API package imports another API's package. * feat(jellyfin): full multi-library support with per-user access scoping Replace the single hardcoded "music" UserView with one CollectionFolder view per library the user can access, and scope every /Items browse query (albums, songs, artists, /Latest) to those libraries via server/filter's ApplyLibraryFilter/ApplyArtistLibraryFilter. ParentId is now disambiguated: a numeric value the user has access to is treated as a library scope (browsing a UserView), otherwise it falls through as an entity id (artist/album), which safely matches nothing rather than leaking another library's content. getItem now 404s when fetching an album or song outside the user's accessible libraries; artists are skipped (they can span multiple libraries) with a TODO. Genres remain unscoped since they're global tags, not per-library entities. * test(jellyfin): cover admin library-access path and drop nil test contexts * feat(jellyfin): Artists and Genres endpoints Add /Artists, /Artists/AlbumArtists, /Genres and /MusicGenres. Artists listing is library-scoped (defaulting to the user's accessible libraries, narrowed by an accessible ParentId), delegating access control to listArtists/ApplyArtistLibraryFilter. Genres are global and unscoped, matching the ML decision already made for listGenres. * refactor(jellyfin): share resolveLibraryScope between items and artists * feat(jellyfin): item image endpoint via artwork service * fix(jellyfin): let net/http sniff image Content-Type instead of forcing jpeg * feat(jellyfin): audio streaming and PlaybackInfo with library access control * feat(jellyfin): favorites and rating write-back with library access control Adds POST/DELETE handlers for /Users/{userId}/FavoriteItems/{itemId} and /Users/{userId}/Items/{itemId}/Rating. A shared resolveAnnotated helper probes album/artist/media file (mirroring getItem's order) and 404s before writing if the user lacks access to the album/song's library; artists are exempt since they span multiple libraries. Ratings are halved coming in (Jellyfin 0-10 -> Navidrome 0-5) to match the doubling in dto.UserData. Also teaches MockMediaFileRepo and MockArtistRepo's SetStar/SetRating (and MockAlbumRepo's, previously a no-op) to actually mutate the backing data so write-back can be asserted in tests. * feat(jellyfin): playback reporting and scrobbling Add /Sessions/Playing[/Progress|/Stopped] and /Sessions/Capabilities[/Full] handlers, backed by core/scrobbler.PlayTracker. A new withPlayer middleware resolves/registers a model.Player from the Emby DeviceId header (used directly as the stable player id, unlike Subsonic's cookie fallback) and injects it into the request context for scrobbling. The Stopped report calls ReportPlayback with IgnoreScrobble to end the now-playing session without double-counting, then calls Submit as the single source of the play-count increment and external scrobble. * feat(jellyfin): playlist read and write-back Adds POST /Playlists, GET/POST/DELETE /Playlists/{id}/Items. Tags each playlist item with PlaylistItemId (the entry's position within the playlist) so DELETE .../Items?EntryIds=... can remove a specific occurrence by the id core/playlists.RemoveTracks actually expects, rather than the song id used everywhere else. * test(jellyfin): cover empty Ids/EntryIds in playlist add/remove * feat(jellyfin): unknown-route logging, generic 500s, rating clamp, docs Hardening pass ahead of real client testing: unmatched routes and unsupported methods now return a logged, JSON 404 instead of chi's default plain-text response, so a missing endpoint a client needs is easy to spot in the logs. Internal errors (ffmpeg output, file paths, etc.) no longer leak into 500 response bodies -- a shared internalError helper logs the real error server-side and always returns a generic message. Inbound Jellyfin ratings are clamped to 0-10 before being halved into Navidrome's 0-5 scale, and /System/Ping now replies with a bare plain-text body as real Jellyfin servers do. Adds a README with an enable/curl walkthrough and known limitations. * docs(jellyfin): clarify public image endpoint and accessibleLibraryIDs comments * fix(jellyfin): case-insensitive path routing for Jellyfin client compatibility * refactor(server): extract case-insensitive path routing to a shared helper * fix(jellyfin): return User Policy and Configuration so Finamp completes login * fix(jellyfin): support Playlist type, multi-type Items queries, and PlayCount/DatePlayed sort Real Finamp requests break against three /Items query engine bugs: Playlist requests fell through to albums, multi-type IncludeItemTypes (e.g. Finamp's favorites screen) only returned the first requested type, and comma-separated SortBy lists (e.g. "DateCreated,SortName") were matched as one opaque string so they always fell through to the repo default. Also extends tests/mock_playlist_repo.go with SetData/GetAll so playlist listing is testable like the other mock repos. * feat(jellyfin): implement /socket WebSocket for real-time client sessions * fix(jellyfin): resolve library-view ids in getItem so clients can load the library * fix(jellyfin): serve direct file at /Items/{id}/File and accept ApiKey query param * fix(jellyfin): resolve playlist ids in getItem * fix(jellyfin): read lowercase ids/entryIds playlist params and add playlist-users endpoints * fix(jellyfin): include MediaSources with Size/Bitrate on tracks so clients show download size * fix(jellyfin): populate all required MediaSourceInfo bool/array fields to match Jellyfin * fix(jellyfin): hex-encode item ids at the API boundary for Jellyfin client compatibility Finamp (and presumably other clients) parses ids as radix-16, but Navidrome's base62 nanoids aren't valid hex and crash its queue packing. Hex-encode every id emitted by the Jellyfin API and decode every id received, keeping the transform reversible and stateless at the boundary rather than touching any model id. * fix(jellyfin): populate MediaStreams with the audio stream so clients can size/transcode * fix(jellyfin): support Ids batch-fetch in /Items so clients can fetch items by id * fix(jellyfin): do not disable Jellyfin server when disabling external services * fix(jellyfin): emit per-image ImageBlurHashes so clients de-dupe images and stop warning * fix(jellyfin): support playlist cover upload/delete and display via item image endpoints * feat(jellyfin): implement GET /Playlists/{id} for playlist visibility Finamp's playlist edit screen calls GET /Playlists/{id} to read the OpenAccess (public visibility) flag; we only had the sub-routes, so it 404'd and the edit screen failed to load. Return the Jellyfin PlaylistDto shape (OpenAccess from Public, empty Shares, media item ids). * fix(jellyfin): display playlist cover art Uploaded playlist covers never showed in clients for two reasons: the playlist BaseItemDto advertised no Primary ImageTag (so clients didn't know to fetch a cover), and the public image endpoint resolved artwork under the request's anonymous context, so a private playlist failed its visibility filter and fell back to the placeholder. Advertise the Primary image tag/blurhash on playlists, and resolve artwork under an elevated context (as core/artwork's cache warmer does). * fix(jellyfin): expand album/artist/playlist ids when building playlists Jellyfin clients (Finamp) send container ids — an album, artist or playlist — in a playlist's Ids list and expect the server to expand each into its child tracks. core/playlists only understands media file ids, so creating or adding with an album id silently produced an empty playlist. Expand container ids to their tracks (in order) before create/add; bare song ids still pass through. Adds filter.SongsByArtistID. * fix(jellyfin): support playlist deletion via DELETE /Items/{id} Finamp deletes a playlist with DELETE /Items/{id}, which we didn't route, so deletion silently failed. Implement it via core/playlists.Delete (which enforces ownership and removes the cover file). Only playlists are deletable through this API; non-playlist ids return 404, non-owners 403. * test(jellyfin): add e2e suite harness + smoke tests * test(jellyfin): e2e for system, auth, routing, browsing, annotations * test(jellyfin): e2e for playlists (CRUD, expansion, cover) and item images * test(jellyfin): e2e for streaming, sessions, and multi-user access control * fix(jellyfin): artist search 500 (library filter leaked into FTS query) getArtists/listArtists reused the browse-path filters (notMissing + ApplyArtistLibraryFilter) for the search path, but artist Search expects a sole Eq{library_id} filter it can consume as a scope — artists have no library_id column, so the compound/join filter leaked into the FTS query and 500'd. Every Finamp artist search failed (the filter is applied even unscoped, since admins resolve to all library ids). Build search filters separately. Adds e2e search coverage. * feat(jellyfin): implement POST /Playlists/{id} to update name, visibility, tracks Finamp edits a playlist (make public, rename, reorder) via POST /Playlists/{id}, which we didn't route, so every edit 404'd. Implement it: Ids present -> replace track list (Create with existing id, preserving name); otherwise update Name/IsPublic via core/playlists.Update. Adds a shared playlistError helper (403/404/500) reused by deleteItem, plus e2e coverage. * docs(jellyfin): update README for playlists, images, id encoding, and e2e * fix(jellyfin): default album track listing to track order Browsing an album's tracks (Items?ParentId=<albumId>&IncludeItemTypes=Audio) took only SongsByAlbum's filters and dropped its Sort, so tracks came back in arbitrary order. Default opts.Sort to the album (disc+track) order when browsing an album without an explicit SortBy — matching Subsonic's GetAlbum and real Jellyfin. An explicit SortBy still wins. * fix(jellyfin): filter items by AlbumArtistIds/ArtistIds An artist's page in Finamp sends ParentId=<libraryId> (scoping) plus AlbumArtistIds/ArtistIds/contributingArtistIds for the artist itself, but queryItems only honored ParentId, so an artist's albums and tracks came back unfiltered (every artist's content). Parse the artist-id params and apply AlbumsByArtistID (albums) / SongsByArtistID (tracks). Adds e2e coverage. * fix(jellyfin): only count a play past the scrobble threshold reportPlaybackStopped set IgnoreScrobble and force-submitted a play on every Stopped report, so a briefly-played track (e.g. an immediate skip) was marked played. Finamp sends Stopped on every track switch, so the threshold must be applied server-side. Let ReportPlayback's StateStopped logic decide (play + scrobble only past 50% of the track / 4-minute cap) instead. Subsonic differs because there the client gates submission. * fix(jellyfin): sort album tracks by track number when client sends IndexNumber SortBy Finamp's album view requests SortBy=ParentIndexNumber,IndexNumber,SortName (disc, track, name), but applySort didn't recognize ParentIndexNumber or IndexNumber and fell through to SortName, sorting tracks alphabetically by title. Map both keys to the album (disc+track) sort. The reversed-title fixture lets the e2e tell track order from title order. * fix(jellyfin): honor the isFavorite query param for favorites filtering Finamp's artist 'Favourite tracks' widget requests favorites via the standalone isFavorite=true query param, not Filters=IsFavorite, so the filter was ignored and non-favorited tracks were returned. Detect both forms. (The widget's reshuffling is Finamp's explicit SortBy=Random.) * feat(jellyfin): emit DateCreated (Date Added) on items BaseItemDto had no DateCreated, so clients showed 'No Date Added' and had nothing to sort 'Recently Added' by. Emit it as ISO 8601 from each entity's CreatedAt (the same field the recently_added sort uses) for songs, albums and artists. * fix(jellyfin): set ArtistItems/AlbumArtists on songs (Now Playing artist) SongToBaseItem only set Artists (names) and AlbumArtist, not the structured ArtistItems/AlbumArtists. Finamp's Now Playing screen reads ArtistItems and shows 'Unknown Artist' when it's absent. Populate both (track artist and album artist) as name+id pairs, mirroring AlbumToBaseItem. * docs(jellyfin): note synthetic blurhash as a follow-up Document that ImageBlurHashes are derived from the item id (a solid-color placeholder), not computed from the cover art like real Jellyfin, and outline what a proper implementation would take. * docs(jellyfin): note WebSocket events and favourited playlists as follow-ups * fix(jellyfin): filter the Artists page by role (album artist vs performer) /Artists and /Artists/AlbumArtists both called the same role-agnostic handler, so Navidrome's per-role artist entries (composers, arrangers, performers) all showed as Album Artists, and the two tabs were identical. Filter /Artists/AlbumArtists to RoleAlbumArtist and /Artists to RoleArtist (and the MusicArtist browse to album artists), via filter.ArtistsByRole. Verified live: Beatles Singles' album-artists dropped 138->5, composers excluded, performers (Billy Preston) show only under /Artists. * fix(jellyfin): sort playlists by name (missing Playlist sort mapping) applySort had no sortColumnsByType entry for the Playlist type, so SortBy=SortName was ignored and the Playlists screen showed them in the repo's default order. Map SortName/Name -> name (as Subsonic does) and DateCreated -> created_at. Verified live: case-insensitive alphabetical. * fix(jellyfin): read query params case-insensitively (Jellify support) Jellify (and the official Jellyfin TypeScript SDK) send query params in camelCase (parentId, albumArtistIds, artistIds, includeItemTypes), where Finamp sends PascalCase. Our handlers read fixed-case keys, so every Jellify filter/sort/paging param was silently dropped: - an artist's page listed albums and tracks from all artists - opening an album listed every album instead of its tracks Real Jellyfin binds query params case-insensitively (ASP.NET model binding), so add a normalizeQueryKeys middleware that folds every query key to lowercase once, and read params by their lowercase name. This also removes the scattered dual-case hacks (Ids/ids, IsFavorite, ContributingArtistIds, api_key/ApiKey, queryParam) that let this bug class through. Also infer the child type from an album parent: Jellify browses an album with only parentId (no IncludeItemTypes), and Jellyfin infers Audio from the parent; without it we fell back to listing all albums. Verified live against Finamp+Jellify and with 4 new e2e specs replaying Jellify's exact request shapes. * fix(jellyfin): advertise LocalAddress in the public system info handshake Jellify (and other @jellyfin/sdk clients) that connect by raw address fall back to HTTP when TLS isn't available, then adopt the handshake's LocalAddress as their server base URL. We never populated it, so Jellify's SDK `api` object was undefined and sign-in crashed with "Cannot read property 'configuration' of undefined" (getUserApi(api!) with an undefined api). Over an HTTPS connection Jellify uses connectionType=hostname and never reads LocalAddress, which is why it worked while an HTTPS proxy was in front. Populate LocalAddress from the request — scheme + host (honoring X-Forwarded-*), plus the /jellyfin mount path — matching real Jellyfin, which always sends it. Export server.ServerAddress for the resolution. * fix(jellyfin): separate "Featured On" from an artist's own discography Jellify's artist page fetches the discography via albumArtistIds and the "Featured On" section via contributingArtistIds, relying on the server to return disjoint sets. We collapsed albumArtistIds/artistIds/ contributingArtistIds into one AlbumsByArtistID filter, so an artist's own albums appeared in both sections. Add AlbumsByContributingArtistID — albums where the artist is a track artist but NOT the album artist — matching Jellyfin's ContributingArtistIds (in Artists, not in AlbumArtists), and route contributingArtistIds to it. * fix(jellyfin): accept a bare Authorization token for audio streaming Jellify's native player (react-native-nitro-player) authenticates the audio stream by setting a bare Authorization header carrying the raw access token ({ AUTHORIZATION: api.accessToken }), not the "MediaBrowser ... Token=" scheme parseEmbyAuth understands. tokenFromRequest didn't recognize it, so every /Audio/{id}/stream request from the native player 401'd and no audio played (the JS client still optimistically posted playback progress, masking it). Accept a bare (or "Bearer <token>") Authorization header, as real Jellyfin does. Verified live: the native player's exact request now streams 200. * fix(jellyfin): embed a self-authenticating stream URL in PlaybackInfo Jellify's native audio player (react-native-nitro-player / ExoPlayer) fetches the stream without forwarding any auth — captured request headers were only Connection/Icy-Metadata/Accept-Encoding/User-Agent, no Authorization and no api_key — so every /Audio/{id}/stream request 401'd and nothing played (the JS client still optimistically posted progress, masking it). Real Jellyfin returns stream URLs with the token embedded; we returned none, so Jellify fell back to a token-less DirectPlay URL. Populate MediaSources[0].TranscodingUrl with /Audio/{id}/universal?api_key=<caller token>, which Jellify's player uses verbatim. Direct-play clients (Finamp builds its own /Items/{id}/File?ApiKey URL) ignore the field, so they're unaffected. * feat(jellyfin): serve per-item UserData (GET /UserItems/{id}/UserData) Jellify fetches this per item to render played/favourite indicators; we 404'd it. Resolve the item via the existing getItem resolver (which loads the caller's annotations and enforces the same library-access gate) and return its UserData, falling back to an empty-but-valid object for items without annotations (e.g. playlists). Also registers the legacy /Users/{userId}/Items/{itemId}/UserData spelling. * refactor(jellyfin): drop unused bare-Authorization token support Added mid-troubleshooting on the theory that Jellify's native player sends a bare Authorization header, but the debug capture showed it sends no auth header at all — the real fix was embedding api_key in PlaybackInfo's TranscodingUrl. No client reaches this path (Jellify JS uses the MediaBrowser scheme, Finamp uses X-Emby-Token, the native player uses the api_key URL), so remove it and its tests per YAGNI. Effectively reverts 4f8ac1e0. * feat(jellyfin): implement Similar endpoints via the external provider Add GET /Artists/{id}/Similar (related artists) and GET /Items/{id}/Similar (similar songs for a track, similar albums for an album, related artists for an artist), sourced from the same external.Provider (Last.fm etc.) that powers Subsonic's getArtistInfo2/getSimilarSongs. Only library-present artists are returned so each is navigable; provider errors and unknown ids degrade to an empty 200 result, so clients (Jellify) stop hammering these with 404 retries. Injects external.Provider into the Router (regenerated via make wire). * perf(jellyfin): make Similar endpoints non-blocking (bounded quick wait) Each /Similar request fetched from Last.fm synchronously (500ms-1.4s), and Jellify requests Similar for many items at once on the home/artist screens, so the whole screen stalled — a home pull-to-refresh dragged for seconds. Run the external lookup on a background context and return within a 500ms quick wait: a cached artist resolves instantly, a cold one returns empty now while the lookup finishes caching in the background, so a later load is fast and populated. Verified live: cold calls bounded at ~500ms (was up to 1.4s), warm calls 3-8ms. * fix(jellyfin): answer ManualPlaylistsFolder so the home stops stalling Jellify resolves its "playlists library" via IncludeItemTypes= ManualPlaylistsFolder, then lists playlists with ParentId set to that folder's id. We didn't recognize the type, so parseTypes fell back to MusicAlbum and returned the album list. Jellify's query then found no item with CollectionType=playlists and resolved undefined — which React Query rejects, retrying it in a backoff loop that stalled the home pull-to-refresh for ~5s (every response was fast server-side; the delay was the client's retries). Return a synthetic "playlists" folder (CollectionType=playlists) for the ManualPlaylistsFolder query, resolve ParentId=<that folder> to the user's playlists, and give playlists a Path under "data" (Jellify drops playlists whose Path lacks it). Adds a Path field to BaseItemDto. * docs(jellyfin): note lyrics and InstantMix/sonic-similarity as follow-ups * fix(jellyfin): genre paging params and same-key casing collisions getGenres read StartIndex/Limit in PascalCase, which normalizeQueryKeys had already folded to lowercase, so genre paging was silently ignored; totals now come from the full (small) genre list instead of the page length. normalizeQueryKeys now merges values when two casings of a key collide, instead of nondeterministically keeping one. * fix(jellyfin): round ratings to the nearest star instead of truncating Rating is a nullable double 0-10 in Jellyfin's contract. Truncating integer division stored 9 as 4 stars, and both Rating=1 and fractional values (which failed integer parsing) became 0 — silently deleting the rating. Parse as float, round, and floor nonzero input at one star. * fix(jellyfin): apply Name/IsPublic sent together with a track replacement Jellyfin's UpdatePlaylist applies every provided field, but the Ids branch returned early, silently discarding a rename or visibility change sent in the same body (core/playlists.Create with an existing id ignores the name). * fix(jellyfin): don't rotate the stored server id on transient DB errors serverID treated any Property.Get error as "no id yet" and persisted a fresh UUID over JellyfinServerID, with sync.Once pinning it for the process lifetime — a busy DB or canceled request context on the first request would break every client's cached ServerId. Only ErrNotFound mints a new id now, and failures yield an uncached temporary value so the next request retries. * fix(jellyfin): report real search totals instead of page length or unfiltered counts Artist search returned TotalRecordCount = len(page), so clients stopped after the first page; album/song search counted via CountAll, which can't see the search term, so clients paged through phantom results. The repos' Search API has no match count, so fetch one row beyond the page: offset+len is exact on the last page and a strictly growing lower bound before it — paging clients terminate exactly at the last match. * fix(jellyfin): browse playlists via the generic /Items path and resolve the playlists folder by id A typeless /Items?ParentId=<playlistId> (legal in real Jellyfin, used by generic clients) fell through to the MusicAlbum default and returned an empty list; it now returns the playlist's tracks, paginated, with visibility enforced by GetWithTracks. resolveItemByID also answers the synthetic playlists-folder id the server itself advertises instead of 404ing it. * perf(jellyfin): cap per-type queries in multi-type /Items requests The multi-type merge path queried each type with a zero-value QueryOptions — no LIMIT in SQL — materializing every matching row (each with embedded MediaSources) just to slice out one page in memory. Each type now fetches at most StartIndex+Limit rows, the worst case one type can contribute to the merged window; totals still come from CountAll. * fix(jellyfin): dedupe and bound the background Similar fetches awaitSimilar spawned a detached, deadline-free goroutine per request; clients re-polling after the empty quick-wait response piled up duplicate provider chains (no singleflight anywhere below) racing writes on the same artist row. Identical in-flight requests now share one fetch — keyed per user, since the mapped items embed the user's annotations — and the background context gets a one-minute deadline so a hung provider can't hold goroutines forever. * fix(jellyfin): gate private playlist covers on the public image route The unauthenticated image endpoint elevated every request to an admin context, so anyone who knew or guessed a playlist id could fetch another user's private uploaded cover. Library artwork still resolves elevated (Jellyfin clients fetch images without auth headers), but playlist covers are now served only when the playlist is public or the request's optional token identifies its owner or an admin; everyone else gets the placeholder. * fix(log): redact full api_key values, including JWTs The api_key pattern matched only word characters, stopping at a JWT's first '.' — the Jellyfin API embeds the session JWT as api_key in TranscodingUrl, so request logs kept its payload and signature, enough to reconstruct a replayable token by prepending the constant header. Match to the next query separator instead, like the sibling s=/p=/jwt= patterns. * fix(jellyfin): record LastLoginAt on Jellyfin logins authenticateByName re-implements credential validation and skipped the UpdateLastLoginAt call the web UI's validateLogin makes, so users who only log in via Jellyfin clients showed a never/stale Last Login in the admin UI. * perf(jellyfin): batch song resolution in /Items?ids= and playlist expansion Both paths probed up to four repositories per client-supplied id. Songs — the common case — now resolve via chunked media_file.id IN queries (same pattern as playqueue's loadTracks); only the residue pays the container probes. * fix(jellyfin): wait for the real Similar result instead of answering a cacheable empty list The 500ms quick wait returned an empty 200 for any cold lookup — indistinguishable from "no similar items exist", so clients cached the wrong answer until an app restart. With fetches deduplicated, wait up to the agents' HTTP timeout for the actual result; only a hung provider now yields the empty fallback, and its fetch still warms the cache in the background. Also makes the dedup test deterministic (the old one raced its release channel). * refactor(tests): extract the shared e2e harness into tests/harness The Subsonic and Jellyfin e2e suites each carried their own copy of the golden-DB lifecycle (boot, seed users/library, scan, WAL snapshot), the ATTACH-DATABASE restore, fixture-FS registration, and the SpyStreamer / NoopFFmpeg doubles. Those now live in one importable package (same pattern as core/storage/storagetest); fixture libraries and request helpers stay per-suite since they encode each API's test expectations. * docs(jellyfin): make comments concise Compress the narrative comments accumulated during live client testing into short why-only notes; keep the client quirks, security rationales and gotchas, drop the exposition. Comments-only change (net -141 lines). * fix(tests): silence gosec taint false-positive in harness snapshot write The write moved from a _test.go file (which gosec skips) into the importable harness package; the path derives from GinkgoT().TempDir(). * fix(jellyfin): route the current /UserFavoriteItems favorite endpoint @jellyfin/sdk 0.13.0 (used by Jellify) posts favorites to POST/DELETE /UserFavoriteItems/{itemId}, while we only routed the legacy /Users/{userId}/FavoriteItems/{itemId} (Finamp). Jellify favorites 404'd ("Failed to add favourite"). Route both spellings to the same handlers. * feat(jellyfin): honor Fields on /Items and add missing conformance fields Match real Jellyfin's response shape: gate MediaSources (and MediaStreams) behind Fields=MediaSources instead of always embedding them — clients that need Size request it, as Finamp does — which cuts a 46-track artist response from ~87KB to a fraction. Also emit the always-present fields Jellyfin sets: ServerId (stamped centrally in ok), LocationType, HasLyrics, and SortName (when Fields=SortName). PlaybackInfo still carries MediaSources (its purpose). * fix(jellyfin): address code review security and correctness findings Applies the reviewed findings from PR #5730: - Gate similar songs/albums on the caller's library access, so the external provider can't surface metadata from libraries the user cannot see. Also clamp the client-supplied limit before it sizes any allocation or provider fetch (CodeQL user-controlled allocation). - Prepend the /jellyfin mount prefix to the PlaybackInfo TranscodingUrl, so a client resolving it as an absolute host path still reaches the mounted router. - Recognize WebP and GIF magic numbers on raw cover uploads, matching the formats Navidrome already supports. - Bound the playlist cover upload: honor EnableArtworkUpload for non-admins and cap the body with MaxBytesReader/MaxImageUploadSize, mirroring the native image endpoint. - Propagate non-not-found repository errors from resolveAnnotated as 500 instead of silently returning 404. - Normalize the literal prefix of mixed literal.param path segments (e.g. STREAM.mp3) so case-insensitive routing reaches stream.{container}. - Rate-limit POST /Users/AuthenticateByName with the same per-IP limiter as /auth/login when AuthRequestLimit is set. - Guard against a nil user in authenticateByName. * fix(jellyfin): correct playlist track browsing and multi-id edits Fixes three playlist issues, two found testing against Jellify and one from the PR review: - Resolve a playlist ParentId to its tracks even when the client sends IncludeItemTypes=Audio. Jellify opens a playlist with ParentId=<playlist>&IncludeItemTypes=Audio; the id was routed through listSongs as an album id, returning an empty list. - Read repeated id query params (ids=X&ids=Y), not just the first value. Jellify's @jellyfin/sdk serializes id arrays as repeated params, so adding an album (which it expands client-side into many ids) only added the first track. Applies to both add and remove; the comma-separated form other clients use still works. - Let an explicit empty Ids array clear a playlist. Ids is now a pointer so an omitted field still means 'leave unchanged', while an empty list clears the tracks (via RemoveTracks, since the repository skips track writes for an empty list). * fix(jellyfin): register clients as players on any authenticated request Jellyfin clients did not appear in the players list. Unlike Subsonic, whose getPlayer middleware runs on every authenticated endpoint, the Jellyfin router only registered a player on the /Sessions/Playing reports, so browsing or streaming never created one. Apply withPlayer to the whole authenticated group, mirroring Subsonic, so the calling device registers (and scrobbling has a player) as soon as it makes any authenticated request. Two follow-ups found while testing: - Skip registration when the request carries no client/device info (no X-Emby-Authorization, e.g. the /socket handshake that auths via ?api_key= only), which otherwise created a junk player named ' []'. - URL-decode the X-Emby-Authorization field values. Jellify's @jellyfin/sdk percent-encodes them (Device='Pixel%208%20Pro') while Finamp sends them raw, so the player name showed as 'Jellify [Pixel%208%20Pro]'. * refactor(jellyfin): move case-insensitive routing into the jellyfin package The case-insensitive path normalization lived in the server package but was only ever used by the Jellyfin router (its whole purpose is that Jellyfin clients route case-insensitively while chi does not). Move it into server/jellyfin and unexport it, so it sits with its only caller and no longer needs to be exported across a package boundary. * docs(jellyfin): document player registration, playlist and image behavior Updates the package README for the behavior added/fixed this round: - New 'Players and sessions' section: any authenticated request now registers the device as a player (like Subsonic), with the Client [Device] naming, URL-decoding of the Emby auth fields, and the /socket skip that avoids a nameless player. - Authentication: note AuthenticateByName is rate-limited per IP. - Playlists: repeated vs comma-separated id params, and that an explicit empty Ids clears the playlist while an omitted Ids leaves it untouched. - Cover art: WebP/GIF magic-number detection plus the MaxImageUploadSize and EnableArtworkUpload gates. - Endpoints table: add the /Similar and /UserFavoriteItems / /UserItems/.../UserData routes that were already served but unlisted. * feat(jellyfin): expose configured users on the login user-picker Adds Jellyfin.ExposedPublicUsers, a comma-separated allowlist of usernames that GET /Users/Public advertises so Jellyfin clients (Finamp, Jellify) can show a login user-picker instead of a blank username field. The endpoint is unauthenticated, so it defaults to exposing no users and never lists the full user table: only the admin-configured names are returned, resolved live per request (a name that doesn't exist is skipped and logged). Each entry is a minimal DTO (Name, Id) with no Policy/Configuration, so admin status isn't leaked pre-login, and no avatar since Navidrome has no per-user profile images. * style(jellyfin): trim redundant comments Remove comments that restated the code or duplicated an explanation already given nearby, keeping the ones that capture non-obvious rationale (client-specific quirks, gotchas). Comment-only change; no behavior difference. * perf(db): keep query planner statistics trustworthy with full ANALYZE PRAGMA optimize's internal ANALYZE runs with a limited analysis budget (~2000 rows) that writes wrong sqlite_stat1 entries for low-cardinality indexes: on a 96K-track library it claimed (missing, library_id) narrows to ~2000 rows when it matches the whole table. The planner then prefers that index over the sort index and falls back to a full-table temp B-tree sort per request, turning paginated song listings into multi-second queries (reproduced at 5.5s on real hardware; ~90x slower than with correct stats). Every index-creating migration re-triggered the poisoning via the post-migration optimize, and the daily optimizer could re-trigger it on large library changes. Setting analysis_limit on the connection does not help: optimize ignores it. Run a plain full ANALYZE instead: after migrations with schema changes, and in db.Optimize (daily schedule and scan-end). Stats are stored in the database file, so one connection suffices and the per-connection pool loop is gone. The Optimize call at shutdown is removed: stats are maintained at migration/scan/daily points, and an ANALYZE during shutdown only delays it and races container stop timeouts. * perf(db): add covering index for title-sorted song listings Deep pagination over songs sorted by title (WHERE missing/library_id, ORDER BY order_title LIMIT/OFFSET - the shape Jellyfin clients use to enumerate the library, and non-admin native/Subsonic song lists share) walked media_file_order_title and fetched the table row for every skipped entry just to evaluate the filter and the annotation/bookmark join keys: offset+limit random reads, seconds per page on cold spinning disks. The new (missing, library_id, order_title, id) index makes the offset skip fully index-resident: filter columns and the join key come from the index, and only the emitted page touches table rows. Measured on a 96K-track library: offset 50000 drops from ~6.5s (poisoned stats) / ~100ms (good stats, warm) to 24ms, and cold deep pages on NAS hardware from ~7s to ~0.5s. * feat(jellyfin): support transcoding via HLS playlist and server-forced player format - withPlayer now propagates the player's configured transcoding into the request context (like Subsonic's getPlayer), so a format forced in Settings > Players applies to the Jellyfin stream endpoints - new GET /Audio/{itemId}/main.m3u8, the endpoint Finamp plays through when its transcoding setting is enabled: a single-segment HLS VOD playlist pointing at the existing progressive transcode endpoint - streamAudio: treat audioBitRate as bits/sec (Jellyfin convention) and fall back to audioCodec as target format when no container is given * fix(jellyfin): honor GenreIds when browsing genre albums and tracks Finamp's genre screen sends ParentId=<libraryId> plus GenreIds=<genreId>, but /Items ignored the param, so every genre returned the whole library. - filter.ByGenreID delegates to persistence.TagIDFilter (exported, was tagIDFilter), the same mechanism behind the native API's genre_id filter - id lists are read via queryIDs, covering both spellings clients use (comma-separated and repeated params); /Items?ids= gains the repeated form too * feat(jellyfin): filter album artists by GenreIds Finamp's artist tab sends GenreIds to /Artists/AlbumArtists when a genre filter is active; the param was ignored, returning all artists. filter.ArtistsByGenreID matches artists credited as album artist on an album with the genre, via a non-correlated semi-join over album participants (86ms on a 29K-artist library; the correlated EXISTS form takes 11 minutes). Applied on the browse path of /Artists* and /Items?IncludeItemTypes=MusicArtist. The performers variant (/Artists?GenreIds=) still ignores the filter: it needs the same semi-join against media_file.participants, unmeasured on large libraries. * fix(jellyfin): version playlist image tag so clients refresh uploaded covers Uploads were stored and served correctly, but Finamp kept showing the old cover: it caches covers keyed by blurHash (its imageId is the item id, which never changes), and both our image tag and synthetic blurhash were derived from the playlist id alone. The tag is now <id>-<UpdatedAt millis hex> (SetImage/RemoveImage go through a full Put, which bumps UpdatedAt), and the blurhash derives from the tag, so every cover change rotates both. UpdatedAt over-invalidates (any playlist edit busts the cover cache), which only costs a refetch; hashing the actual image remains the proper long-term tag. An e2e test guards the whole chain, since a partial Put(pls, cols...) would silently stop bumping UpdatedAt. * fix(jellyfin): align cover upload limit and validation with the native endpoint Cover uploads of large photos failed with a silent 400: the MaxImageUploadSize cap was applied to the wire body, which Jellyfin clients base64-encode (4/3 inflation), so the effective raw-image limit was only ~7.5MB — and Finamp uploads picked photos uncompressed. Align with the native endpoint: - the limit caps the decoded image; the read cap allows for base64 inflation - validate by decoding (image.DecodeConfig) and take the storage extension from the real format instead of the Content-Type header, which clients get wrong (Finamp falls back to image/jpeg); a HEIC or corrupt file is now rejected instead of stored as a broken cover - rejected uploads log the reason (size/limit/decode error); diagnosing this from production logs previously required guesswork * fix(jellyfin): optimize SongsByArtistID filter to improve performance at library scale Signed-off-by: Deluan <deluan@navidrome.org> * fix(jellyfin): sort tracks by release year for SortBy=PremiereDate Finamp's "Latest Releases" artist section sends SortBy=PremiereDate,Album,... descending; PremiereDate wasn't in the Audio sort map, so applySort fell through to Album and the view came back in reverse album-name order (a 2002 remix album first, the 2013 release last). Map premieredate/productionyear to the year sort key, matching the ProductionYear the DTO exposes for songs and the existing MusicAlbum mapping (premieredate -> max_year). * feat(jellyfin): expose PremiereDate on tracks and albums Finamp's "Latest Releases" artist/genre sections re-sort the merged server responses client-side by PremiereDate and keep the top 5. Without the field every comparison returns equal and Dart's unstable sort leaves the picks in arbitrary order — a 2007 remix could lead the list even with the server sorting by year correctly. Serialize PremiereDate as ISO 8601 from the date tag (padding partial "2007"/"2007-02" values so DateTime.tryParse accepts them), falling back to the year; omitted when neither exists. * feat(jellyfin): resolve Finamp-truncated item ids (saved queue restore) Finamp persists its play queue by packing every item id into exactly 16 bytes (packIds assumes Jellyfin's 32-hex GUID ids), so our longer ids come back truncated after an app restart and "Failed to restore queue" loops forever: the /Items?ids= batch resolves nothing. Navidrome ids can't be made GUID-shaped (nanoid ids can exceed 128 bits), so compensate server-side: a 16-char id — a length no Navidrome id family uses — is resolved by unique-prefix range scan, with ambiguity failing safe. The ids= batch echoes the id as requested (Finamp matches restored items by its stored ids), and stream, image, item, user-data, favorite, rating and playback-report endpoints accept truncated ids transparently. The proper fix belongs upstream in Finamp's packIds; documented in the README so this layer can be removed once that ships. * feat(jellyfin): implement Items/{id}/InstantMix Finamp requests an instant mix on every track tap when its "start instant mix for individual tracks" setting is on (plus the long-press menus); the 404 made those taps fail with an error and play nothing. A track seed returns itself first — Finamp plays exactly what comes back — followed by the external provider's similar songs, capped at the requested limit and filtered to the caller's libraries. Container seeds (artist/album) return the provider's similar-songs blend. Provider errors and unknown seeds degrade to seed-only/empty results instead of 404s, reusing the Similar endpoints' bounded-wait singleflight (with a distinct cache key, since mixes and similar lists answer different shapes). Sonic-similarity backing stays a follow-up (see README). * style(jellyfin): trim wordy comments * refactor(jellyfin): apply cleanup review findings - batch truncated-id resolution in /Items?ids=: one chunked range query for all media-file prefixes instead of a query per id (a restored queue sends hundreds of truncated ids) - resolve truncated ids on the /Similar endpoints too, matching the neighboring InstantMix; document which entry points don't resolve - hoist maxImageUploadSize to core, deleting the byte-identical copies in nativeapi and jellyfin (tests moved to core) - drop the unused type parameter on premiereDate * fix(jellyfin): never drop the instant mix seed on a slow provider The seed track was built inside the awaited provider fetch, so when the external agent was slow or unreachable the request hit the 10s wait and answered a fully empty mix — Finamp then played nothing on tap, even though the seed needs no provider at all (seen live: Last.fm unreachable from the server, responseSize=49). Build the seed outside the await: only the similar-songs tail is fetched and bounded, and a timeout now degrades to a seed-only mix. Also folds instantMixForSong into getInstantMix, since the tail is exactly similarSongs. * fix(jellyfin): prefer the recommended Authorization scheme when picking a token Jellyfin's authorization guidance deprecates X-Emby-Token, X-MediaBrowser-Token, X-Emby-Authorization and api_key; the Authorization MediaBrowser scheme is the recommended form. All spellings stay accepted, but when a client sends several, the recommended one now wins. Adds coverage for the canonical Authorization header, which no test exercised directly. * refactor(jellyfin): rename parseEmbyAuth to parseMediaBrowserAuth The scheme is named MediaBrowser; the old name evoked the deprecated X-Emby-* spellings even though the function also parses the recommended Authorization header. * fix(jellyfin): prefer the Authorization header over X-Emby-Authorization The recommended header now wins when both carry MediaBrowser data — but only when it actually parses as MediaBrowser: a reverse proxy may inject Basic/Digest credentials into Authorization while the client sends the deprecated header, and those must not swallow the client's auth. * fix(jellyfin): require the MediaBrowser scheme when parsing auth headers The parser extracted key="value" pairs from any Authorization value; a foreign scheme whose parameters happened to use our field names would have been misread as client auth. Validate the scheme word instead (case-insensitively, per HTTP), accepting the legacy "Emby" spelling like real Jellyfin. Replaces the any-recognized-field heuristic for detecting proxy-injected Basic/Digest credentials. * Revert "perf(db): keep query planner statistics trustworthy with full ANALYZE" This reverts commit 118563e1053196f0e2e42dd4bee54e39a04ab145. The planner-statistics work now lives in its own PR (#5740); this branch keeps only the covering-index migration. * fix(db): renumber jellyfin covering-index migration after master's latest * fix(db): renumber Jellyfin covering-index migration * docs(jellyfin): document playlist annotations * refactor(log): clarify comments on external services query params * refactor(e2e): move Subsonic e2e suite to server/subsonic/e2e All files in server/e2e were Subsonic tests, so relocate the package under server/subsonic/e2e to sit alongside the API it exercises. The package name stays 'e2e'; only doc/comment references are updated. * refactor(filter): consolidate genre filters and unexport tagIDFilter Route filterByGenre, ByGenreID, and ArtistsByGenreID through a single genreTagFilter helper so the EXISTS json_tree(tags,"$.genre") predicate lives in one place. With server/filter no longer using it, persistence's TagIDFilter is only referenced within the package, so unexport it. --------- Signed-off-by: Deluan <deluan@navidrome.org>	2026-07-14 11:46:37 -04:00
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	2026-07-13 12:04:29 -04:00
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>	2026-07-13 11:32:03 -04:00
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.	2026-07-12 13:27:01 -04:00
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.	2026-07-10 20:27:29 -04:00
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.	2026-07-10 10:52:05 -04:00
Deluan Quintão	4652b46602	fix(plugins): populate username for buffered plugin scrobbles (#5736) ... Plugin scrobblers read the username from the request context via getUsernameFromContext, but buffered scrobbles are persisted to the DB scrobble buffer (which stores only the userId) and later drained by a background worker running on context.Background(). That context carries no authenticated user, so ScrobbleRequest.Username was always empty for WASM scrobbler plugins. NowPlaying is unaffected because it is dispatched synchronously on context.WithoutCancel(requestCtx), which retains the user. Restore the user in the drain path: processUserQueue now looks up the user by the buffered userId and injects it into the context via request.WithUser before dispatching, mirroring the pattern already used in play_tracker. Builtin scrobblers are unaffected as they resolve the account from the userId argument rather than the context.	2026-07-08 12:37:42 -04:00
Deluan Quintão	f48943c058	fix(plugins): discard buffered scrobbles when a plugin is removed (#5737) ... * fix(plugins): discard buffered scrobbles when a plugin is removed Scrobbles are buffered in the DB per service, keyed by the plugin name. When a plugin was removed (deleted from the plugins folder and detected by the sync), its pending buffer entries were left behind forever: the drain goroutine is stopped on the next scrobbler refresh, so the rows were never retried nor discarded. Worse, if a plugin with the same name was installed later, the stale entries would be drained into it - potentially a completely unrelated plugin that just reuses the name. Add a Discard(service) method to ScrobbleBufferRepository and call it from removePluginFromDB, right after the plugin record is deleted. Disabling a plugin intentionally keeps its buffered scrobbles, consistent with the buffer's purpose of surviving temporary outages, and transient unload/reload cycles during config updates are unaffected since they never delete the plugin record. * fix(plugins): don't wipe builtin scrobbler queues on plugin removal Buffer entries are keyed by service name only, and removePluginFromDB runs for any removed plugin file, so removing a plugin named e.g. lastfm.ndp - regardless of its capability - would discard the builtin Last.fm retry queue. Skip the discard when the plugin name is owned by a registered builtin scrobbler, exposed via a new scrobbler.IsBuiltinScrobbler helper. Reported by Codex review on the PR. Also drop the testBroker usage from the new removePluginFromDB spec: it is defined in manager_test.go which is excluded on Windows, breaking the Windows test build. sendPluginRefreshEvent is nil-safe, so no broker is needed.	2026-07-08 12:37:17 -04:00
Deluan Quintão	01b7c86f90	fix(scanner): stop logging expected lyrics sniff misses as warnings (#5702) ... * fix(scanner): stop logging expected lyrics sniff misses as warnings During a scan, embedded lyrics are parsed with an empty suffix, which puts ParseLyrics into content-sniffing mode: it tries the TTML, SRT and Lyricsfile YAML parsers in turn before falling back to plain text. Every plain-text or LRC lyric therefore fails the structured probes on its way to the fallback, and each failure was logged at warning level with no indication of which file triggered it, flooding the scan log with benign "Error parsing lyrics, falling back to plain text" messages. A probe rejecting content it does not own during sniffing is expected control flow, so it is now logged at trace instead. A parse failure under an explicitly requested suffix (e.g. a malformed .yaml/.srt/.ttml sidecar) still warns, since the user declared that format. ParseLyrics gains ctx and path parameters so any warning names the offending file and carries request context where available; all call sites are updated accordingly. Also fixes a test-isolation bug in the new logging spec: the BeforeEach swapped the process-global default logger via SetDefaultLogger but only restored the log level on cleanup, leaking the null logger and its hook into later specs in the shared model suite. * test: use spec-scoped contexts instead of context.Background in lyrics tests Replace context.Background() with GinkgoT().Context() (and b.Context() in the parse benchmarks) across the lyrics-related tests, so contexts are cancelled when each spec ends. The embeddedLyrics fixture in core/lyrics is now a hand-written literal like its sibling fixtures, removing the construction-time ParseLyrics call that could not use a spec-scoped context. * refactor(model): attach lyrics parse log attribution via context Narrow ParseLyrics back to (ctx, suffix, lang, contents), dropping the path parameter added by the previous commit. Attribution now uses the codebase's existing idiom: callers that know the source attach it with log.NewContext (e.g. "file" for the media file or sidecar), and the plugin adapter tags both the plugin name and the track, fixing probe-miss logs that misattributed plugin-returned content to the file's own tags. This removes three adjacent string parameters that were easy to swap silently, and the "" placeholder most call sites had to pass. Also hardens the logging spec from the previous commit: the null test logger is now swapped in before raising the level (SetLevel forces the current default logger to trace, so the old order left the null logger at info and trace entries never reached the hook), the sniff test now asserts probe misses are observable at trace with file attribution instead of only asserting the absence of warnings, and cleanup restores the actual previous logger — via a new return value on log.SetDefaultLogger — instead of a bare logrus.New() that would discard hooks configured on the process-wide logger. * refactor(lyrics): hoist attributed log contexts out of loops Address review feedback on #5702: build the log-attributed context once per operation instead of per iteration, and reuse it on the surrounding log calls so the error/trace lines around ParseLyrics carry the same attribution fields. In fromExternalFile the sidecar path now rides the context for all log lines in the function, replacing the repeated explicit "path" field. * style(model): pass lyrics parse errors as final log arguments Per the project logging convention, errors go as the last argument (the log package normalizes them via its error case) instead of a keyed "error" pair, which stores the raw error value and bypasses that handling. Flagged by review on #5702; the keyed form was inherited from the original warning line.	2026-07-02 09:46:57 -04:00
Deluan Quintão	bd9fa1c602	feat(listenbrainz): match collaboration top-songs via all credited artist MBIDs (#5670) ... * refactor(agents): drop single Artist fields from Song, keep only Artists Song now represents credited artists solely via the Artists slice; the single Artist/ArtistMBID fields and the ArtistList passthrough are removed. Equals continues to hash the whole value. * refactor(lastfm,listenbrainz): build Song.Artists in built-in agents Last.fm and ListenBrainz now populate the Artists slice directly. For ListenBrainz top songs, all credited artist MBIDs are mapped (the combined display name on the first credit plus MBID-only collaborators) instead of keeping only the first MBID, feeding the matcher's per-MBID specificity. * refactor(external): set Song.Artists in top-songs enrichment getMatchingTopSongs now seeds an Artists entry from the known artist when a song carries none, replacing the single Artist/ArtistMBID writes. * refactor(plugins): fold single-artist SongRef into Song.Artists SongRef keeps its single Artist/ArtistMBID fields as part of the plugin wire contract; songRefToAgentSong now folds them into a one-element Artists list when a plugin sends no artists array. * refactor(matcher): read Song.Artists directly groupQueries consumes s.Artists now that ArtistList is gone; test inputs build the Artists slice. * fix(matcher): keep MBID-only artists as identity signals Review follow-up: an artist credited only by MBID (empty ID and name) was dropped before resolution in four places, defeating the multi-MBID matching path this PR adds. - matcher.groupQueries: treat a non-empty MBID as a usable artist signal - external.getMatchingTopSongs: backfill the primary credit's name/MBID when the agent left them empty - plugins.songRefToAgentSong: fold a single-artist SongRef when only ArtistMBID is set (not just when Artist name is set) - listenbrainz.topSongArtists: return nil instead of an empty-name placeholder when neither name nor MBIDs are present * fix(external): only backfill top-song artist onto an unnamed credit Review follow-up (codex P2): the previous backfill stamped the queried artist's MBID onto Artists[0] whenever it was empty, even when that credit already named a different (e.g. featured) artist — producing a mismatched name+MBID pair that could mis-rank matches. Now only an unnamed first credit is filled (it is, by construction, the queried artist); an already-named credit is left untouched. Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org>	2026-06-26 17:06:14 -04:00
Deluan Quintão	7b7721f002	feat(matcher): match similar/top songs by multiple artists (#5668) ... * feat(matcher): add Song.Artists (agents.Artist) and field-wise song dedup * refactor(matcher): make song equality an agents.Song.Equals method via hashstructure Move the sameSong free function from core/matcher into an Equals method on agents.Song, following the model.MediaFile/Album.Equals convention. Uses strict hashstructure hashing (nil opts, no IgnoreZeroValue) to preserve the original whole-value equality contract. Tests moved to core/agents. * feat(matcher): match by multiple artists with overlap ranking and artist-ID fast-path * refactor(matcher): rank artist overlap and specificity above the preferred-track flag Identity signals (specificityLevel, artistOverlap) now outrank the taste signal (preferredMatch) in betterThan. A starred/4-star track that is a worse identity match no longer beats a more specific or higher-overlap track. PreferStarred still breaks ties when specificity and overlap are equal. * fix(matcher): score artist-MBID specificity against all credited artists, not just the last sanitizedTrack.artistMBID (string) replaced with artistMBIDs (map[string]struct{}) so bucketTracks collects all credited owned MBIDs per query instead of last-write-wins. computeSpecificityLevel tests set membership, letting each of a collaboration's MBID-bearing artists reach the proper specificity level (4/5) independently. * feat(plugins): carry multiple artists (with IDs) through SongRef conversions * feat(plugins): regenerate schemas and PDK wrappers for multi-artist SongRef * refactor(matcher): tidy bucketTracks accumulator and artist resolution Replace bucketTracks' two parallel per-track maps (overlapByQuery/mbidsByQuery) with a named queryAccum struct (F2). Collapse resolveArtists' four hand-mutated parallel maps into a pendingArtist slice with derived nameToQueries/mbidToQueries maps (F1). Replace the own() method on resolvedArtists with a package-level addToSet helper that drops the method/receiver indirection (F3). * docs(matcher): trim comments that restate the code * fix(plugins): use Vec::is_empty for slice fields in generated Rust PDK * fix(matcher): treat a resolved artist ID as an identity match for specificity * docs(matcher): reflect artist-ID identity in the specificity ladder	2026-06-26 14:46:09 -04:00
Deluan Quintão	06993a8e04	test(storage): re-enable local storage tests on Windows (#5654) ... * refactor(storage): extract LocalPathToURL from storage.For * test(storage): re-enable local storage tests on Windows (#5381) * test(storage): fix Windows drive-letter path expectation The 'should handle Windows drive letters correctly' test was gated behind the now-removed SkipOnWindows BeforeEach, so its expectation had never run. On Windows, newLocalStorage re-joins u.Host+u.Path via filepath.Join, which yields a backslash path (C:\music), not C:/music. Assert against filepath.Join so the expectation matches the OS-native result. * test(storage): probe Windows drive-letter path in LocalPathToURL Three review bots flagged that LocalPathToURL escapes the drive-letter colon (C: -> C%3A), which url.Parse rejects. There are no Windows bug reports, so add a Windows-gated test that exercises the real conversion on a drive-letter path and let CI decide whether the bug is real before changing production code.	2026-06-22 21:33:14 -04:00
Deluan Quintão	b38054b29c	perf(artwork): faster image resize + update gen2brain/webp to v0.6.0 (#5652) ... * fix(artwork): convert decoded images to a fast-path type before resizing x/image/draw's CatmullRom scaler only has optimized paths for *image.RGBA, *image.NRGBA, *image.Gray and *image.YCbCr. Other concrete types — notably *image.NYCbCrA (from WebP) and *image.Paletted (indexed PNGs) — fall back to a generic per-pixel At()/RGBA() loop that is several times slower. Convert such images to *image.RGBA once before scaling; fast-path types are returned unchanged. This makes resize performance independent of which decoder wins the image.Decode("webp") registration, and also speeds up indexed PNGs. Signed-off-by: Deluan <deluan@navidrome.org> * chore(deps): update gen2brain/webp to v0.6.0 v0.6.0 replaces the wazero WASM runtime with a self-contained wasm2go-transpiled WebP decoder/encoder. This drops the webp -> wazero dependency edge (wazero is still used by the plugin system) and makes the WASM-only build path (32-bit / nodynamic) faster and far lighter on allocations. Signed-off-by: Deluan <deluan@navidrome.org> * perf(artwork): defer fast-path conversion until a resize is needed Move toFastScaleType to just before the CatmullRom.Scale call, after the no-upscale early return. Previously the conversion ran right after decode, so a request for a size >= the source dimensions would allocate and walk a full RGBA copy only to discard it when resizeStaticImage returns nil. The resize path is unchanged; the no-op path drops ~30-40% time and up to ~79% memory for large indexed/WebP artwork. Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org>	2026-06-22 16:33:01 -04:00
Deluan Quintão	803b385920	fix(matcher): match by artist credit so artist-MBID specificity works and collaborators match (#5637) ... * refactor(matcher): carry resolved artist MBID on sanitizedTrack * feat(matcher): two-phase artist resolution for title matching * fix(matcher): wire phase-1 artist mock in consumer tests; guard back-map dupes - External tests for title-fallback paths now register a second `.Maybe()` artistRepo.GetAll expectation for the matcher's phase-1 artist-resolution call, and supply RoleArtist Participants on phase-2 track mocks so the back-map routes tracks to their query buckets correctly. - Back-map loop extracted into buildTracksByQuery helper; guard ensures each track is appended at most once per query bucket even when it credits multiple resolved artists in that bucket, preventing duplicate scoring. - matchTitlePhase2 helper: removed redundant squirrel.Sqlizer type assertion (ranging over squirrel.And already yields Sqlizer). * perf(matcher): use non-correlated IN subquery for phase-2 lookup * refactor(matcher): decompose two-phase title matching into named steps matchByTitle had grown to ~98 lines holding four jobs and eleven locals. Split it into a thin orchestrator over named phases: - groupQueriesByArtist: build the per-artist query buckets. - resolveArtists / resolvedArtists: phase 1, with the three parallel maps (byQuery / mbid / allIDs) folded into one type that owns artist-ID routing and track bucketing. Artist ownership is now two direct map lookups (by order name, by MBID) instead of the prior O(artists x queries) nested scan. - fetchTracksCreditedTo: phase 2, using squirrel.Placeholders instead of a hand-rolled placeholder string. - bucketTracks / scoring loop: unchanged behavior. Pure restructuring; the suite, race, consumers, and the real-DB benchmark are all unchanged. Also dedupes the phase-1 MBID filter as a side effect. * refactor(matcher): apply simplify cleanups to two-phase resolution Behavior-preserving cleanups from a /simplify pass: - struct{} sets instead of bool-valued sets (codebase convention) - move newSanitizedTrack after the dedup guard in bucketTracks, so a track credited to multiple resolved artists is sanitized only when actually bucketed - pre-size the phase-1/phase-2 maps; drop the dead nil-guard in own() - slice.Map for the []string->[]any arg conversion - document why the raw media_file_artists SQL stays in this layer, and that bucketTracks relies on the bulk participants JSON carrying artist IDs - extract an artistParticipants() test helper, collapsing 57 four-level Participants literals (test file -211 lines net) * docs(matcher): rename inner title-matching steps to avoid 'phase' clash The matcher's top-level strategies (ID/MBID/ISRC/Title) are already called phases. Reusing 'phase 1/2' for the two steps inside title matching (resolve artists, fetch their tracks) was confusing. Drop the ordinals and let the function names (resolveArtists, fetchTracksCreditedTo) and prose describe the steps. Renamed the matchTitlePhase2 test helper to matchTracksByArtistQuery. Comment-only; no behavior change. * fix(matcher): own MBID-resolved artists for every aliased query Address bot review on PR #5637. When several agent queries share one ArtistMBID under different sanitized names (agent aliases), the resolver kept only the last query name per MBID, so the others never owned the resolved artist and their songs fell through. Track all query names per MBID instead; add a RED-proven test for the two-alias case. Also invert byQuery into a reverse artist-ID -> query-name index in bucketTracks, dropping the per-participant scan over all queries, and guard fetchTracksCreditedTo against an empty artist-ID slice. Fix the artist mock to forward variadic options with Called(options...) so QueryOptions-shaped matchers receive the same argument shape as real calls. * docs(matcher): trim comments that restated the code Cut three doc comments down to their why: groupQueriesByArtist, own, and bucketTracks no longer restate what the signature and body already show. Condense fetchTracksCreditedTo's rationale from two paragraphs to one, keeping the role='artist', non-correlated-IN, and layer-boundary notes.	2026-06-21 11:05:58 -04:00
Deluan Quintão	6486a27634	refactor(matcher): index-space resolution + batched title lookups (#5635) ... * refactor(matcher): resolve matches in song-index space * test(matcher): pin per-index duration matching for duplicate title+artist songs * refactor(matcher): drop unreachable specificity sentinel * refactor(matcher): hoist PreferStarred read out of scoring loop * docs(matcher): correct config field references in MatchSongs doc * fix(matcher): log swallowed per-artist DB error in title matching * fix(matcher): fail title matching when all artist lookups error * refactor(matcher): simplify loaders and test helpers * docs(matcher): move algorithm docs to package-level doc.go * docs(matcher): focus examples on fuzzy matching behavior * refactor(matcher): store index in dedup map and harden test helper * fix(matcher): keep exact-phase matches when all title lookups fail * perf(matcher): batch title-phase artist lookups into one query matchByTitle issued one GetAll per distinct artist, run serially. On a large library a batch of similar-songs spans dozens of artists, and profiling against a 95k-track library showed the matcher was ~90% bound in that serial query loop (a 100-song batch fired ~89 separate multi-join queries, taking ~6s). Replace the loop with a single 'order_artist_name IN (...)' query, then group the returned tracks by artist in memory and score each song against its bucket. This cuts a 100-song batch from ~6s to ~0.4s (roughly 14x) with less memory. Grouping keys on order_artist_name (the field the query filters on, matching how the per-artist queries are keyed), falling back to the sanitized Artist when it is unset. Because there is now a single query, matchByTitle is all-or-nothing like the ID/MBID/ISRC loaders: the per-artist best-effort skip is gone, while resolveMatches still preserves exact-phase matches when the title query fails. * refactor(matcher): group batched title matches by order_artist_name After batching the title-phase lookups into one query, the returned tracks must be grouped back to their artist. Key on MediaFile.OrderArtistName — the exact field the query filters on — so collaboration/"feat." tracks (whose display Artist differs from the sort artist) bucket correctly, with a sanitized-Artist fallback when it is unset. OrderArtistName is deprecated in favor of Participants, but the bulk GetAll path does not hydrate participant detail (the rich artist fields come only from the per-record GetWithParticipants JOIN), so the participant order name is empty here and the column is the only populated source. Also adds a TODO in computeSpecificityLevel: its artist-MBID levels read the deprecated, unpopulated MediaFile.MbzArtistID column, so they never fire today.	2026-06-21 03:45:56 -04:00
Deluan Quintão	aa5aa731dc	refactor(lyrics): single ParseLyrics entry point + all-format plugin lyrics (#5632) ... * refactor(lyrics): read sidecar files via library storage FS Routes fromExternalFile reads through storage.For(mf.LibraryPath) instead of os.Open on AbsolutePath, fixing sidecar reads for non-local backends. UTF-16 LE/BE and BOM handling preserved via ioutils.UTF8Reader. * refactor(lyrics): address review feedback on sidecar FS read - Move blank local-storage import from sources.go into lyrics_suite_test.go (the test suite already imports the local package for RegisterExtractor, so local's init() runs; production binaries get the scheme via normal wiring) - Fix misleading comment: model.ParseLyrics → model.ParseLyricsFile - Replace what-comment with why-comment in BeforeSuite explaining the log.Fatal guard that requires the no-op extractor registration * test(lyrics): add subsonic e2e baseline for getLyrics endpoints Establishes a behavioral baseline for getLyricsBySongId (v2 structured) and getLyrics (legacy) before the lyrics parser refactor. Covers embedded formats (LRC synced, plain text, TTML) and sidecar formats (LRC, SRT, YAML), all isolated under a Lyrics/ fixture folder so the new fixtures do not perturb existing test behavior beyond fixture counts. Sidecar files are injected as raw &fstest.MapFile{Data: []byte(...)} entries; the scanner skips non-audio extensions (.lrc, .srt, .yaml) so they are invisible to scanning but reachable via the fake FS at request time through fromExternalFile/storage.For. Update album/artist/song counts in the album-list, multi-library, and search3 empty-query tests to reflect the six new tracks (1 new artist, 1 new album, 6 new songs). * test(lyrics): strengthen e2e lyrics baseline (lang assertions, rename helper) Rename the local helper `main` to `firstLyric` to avoid collision with the reserved-feeling built-in name. Add `Lang` assertions to both embedded and sidecar DescribeTable entries, locking the current observed values: "xxx" (ISO 639-2 "no language specified") for all embedded and LRC/SRT sidecars, and "eng" for the YAML sidecar (which explicitly sets `language: eng`). * feat(lyrics): detect Lyricsfile YAML in content-sniffing * feat(plugins): content-sniff plugin lyrics for all formats Replace model.ToLyrics (LRC/plain only) with model.ParseEmbedded so plugin responses are content-sniffed for TTML, SRT, YAML, LRC, and plain text. ParseEmbedded returns a LyricList, so the loop now flattens multiple tracks per response entry. The test-lyrics WASM plugin gains a "ttml" format mode (configured via pdk.GetConfig) that returns a minimal TTML document; rebuilt with the standard Go wasip1 toolchain (GOOS=wasip1 GOARCH=wasm). A new Ginkgo test asserts Synced==true and the exact cue value, which the old plain-text path could not produce. GetLyrics doc comment updated to reflect content-sniffing; a later task will retarget it to ParseLyrics once that function is introduced. * test(plugins): validate plugin lyrics auto-detect across all formats The test-lyrics WASM plugin now supports per-format modes via the "format" config key: ttml, srt, yaml, lrc, and plain, in addition to the existing default plain-text response. The plugin is rebuilt with the standard Go wasip1 compiler. lyrics_adapter_test.go gains a DescribeTable covering all five formats, asserting both Synced (the discriminator that proves correct format detection) and the exact line value. This validates the full auto-detect chain (TTML → SRT → YAML/Lyricsfile → LRC → plain) end-to-end through the real plugin → adapter → parser flow. * refactor(lyrics): consolidate parsers into model.ParseLyrics * refactor(lyrics): retarget legacy callers to model.ParseLyrics Pin suffix to ".lrc" to preserve byte-identical output for stored plain/LRC text that was previously handled by the now-removed ToLyrics. * test(lyrics): fix lyrics tests after parser consolidation - Rewrite the YAML-fallback test to assert the correct design: a non-Lyricsfile .yaml sidecar returns as plain text and shadows lower-priority sources (rather than falling through to .lrc). - Add LibraryPath + relative Path split to the three subsonic tests that read sidecar files via storage.For(), so they resolve against the correct fixtures directory. - Register a no-op extractor in api_suite_test.go BeforeSuite so newLocalStorage does not fatal when storage.For is called during sidecar-lyrics tests. * test(lyrics): add per-format ParseLyrics benchmarks Baseline measurements (count=2 runs) on M2: BenchmarkParseLyrics_LRC-8 5725 178796 ns/op 49.78 MB/s 427877 B/op 523 allocs/op BenchmarkParseLyrics_Plain-8 5425 230854 ns/op 32.44 MB/s 102508 B/op 16 allocs/op BenchmarkParseLyrics_EnhancedLRC-8 1942 605893 ns/op 17.66 MB/s 860678 B/op 4256 allocs/op BenchmarkParseLyrics_SRT-8 3249 373991 ns/op 25.91 MB/s 1113575 B/op 4407 allocs/op BenchmarkParseLyrics_TTML-8 1483 813027 ns/op 13.86 MB/s 2198052 B/op 8665 allocs/op BenchmarkParseLyrics_YAML-8 1700 678250 ns/op 13.01 MB/s 1235096 B/op 8288 allocs/op BenchmarkParseLyrics_SniffTTML-8 1525 776482 ns/op 14.51 MB/s 2225448 B/op 8681 allocs/op BenchmarkParseLyrics_SniffSRT-8 2528 451210 ns/op 21.48 MB/s 1157000 B/op 4422 allocs/op BenchmarkParseLyrics_SniffYAML-8 1333 827152 ns/op 10.67 MB/s 1337195 B/op 8718 allocs/op BenchmarkParseLyrics_SniffLRC-8 2820 413038 ns/op 21.55 MB/s 588934 B/op 1812 allocs/op BenchmarkParseLyrics_SniffPlain-8 2968 409091 ns/op 18.31 MB/s 254470 B/op 1491 allocs/op Content-sniff path overhead: 1.5–15% depending on format. * test(lyrics): use real public-domain fixtures for parser benchmarks Replace synthetic benchmark payloads with 'Auld Lang Syne' (Robert Burns, 1788, public domain) rendered into every supported format (LRC, plain, enhanced LRC, SRT, TTML, Lyricsfile YAML) so the numbers reflect realistic content. Same song across formats makes per-format cost comparable. Baseline (Apple M-series, -benchmem, real fixtures): LRC ~28 us/op 42 KB 147 allocs Plain ~23 us/op 18 KB 22 allocs EnhancedLRC ~37 us/op 51 KB 374 allocs SRT ~52 us/op 139 KB 581 allocs TTML ~119 us/op 276 KB 1227 allocs YAML ~142 us/op 193 KB 1732 allocs Sniff(LRC) ~47 us/op 57 KB 237 allocs Sniff(TTML) ~122 us/op 282 KB 1250 allocs Sniff(YAML) ~186 us/op 218 KB 1847 allocs Fixtures in tests/fixtures/lyrics/. * fix(lyrics): preserve [] (not null) for empty lyrics in backfill migration ParseLyrics returns nil for zero-line input (whitespace-only stored lyrics). json.Marshal(nil LyricList) produces null, violating the DB invariant that media_file.lyrics uses [] for empty lyrics, never null. Initialize to model.LyricList{} when ParseLyrics returns nil so the marshalled result is always []. * refactor(lyrics): unify parser dispatch and centralize empty-list invariant Apply thermo-nuclear review findings (behavior-preserving): - Replace the suffix switch + three single-use closure adapters (parseTTMLKnown/parseSRTKnown + inline YAML closure) with a bySuffix map of a single lyricParser(lang, contents) signature. Normalize parseTTMLWithDefaultLang/parseSRTWithLanguage to that (lang, contents) order so no adapter glue is needed. - Collapse the parallel sniffLyrics engine into one parseFirstMatch primitive shared by both the suffix and content-sniff paths (sniffOrder candidate list). TTML stays gated via parseTTMLIfDocument in sniff mode to avoid running the XML decoder on plain/LRC text. - Add LyricList.MarshalJSON so empty/nil always serializes to [] (the lyrics column invariant), in one canonical place. Delete the migration's nil-guard, which the marshaler now subsumes. Behavior verified unchanged: full suite + race + e2e green. * refactor(lyrics): single registry drives both suffix dispatch and sniff order Collapse the bySuffix map and sniffOrder slice into one ordered registry: slice order is the content-sniff probe order, each row's suffixes drive sidecar dispatch, and per-row bySuffix/byContent parsers preserve the gated-TTML-when-sniffing distinction. One source of truth, no duplicated parser references. * refactor(lyrics): self-skipping parsers collapse the format table to one column Move the TTML <tt>-document gate into parseTTMLWithDefaultLang itself (after the encoding fixup, so UTF-16-declared docs are still recognized): non-TTML content returns (nil, nil) to skip; a malformed <tt> document still errors. SRT and Lyricsfile YAML already self-skip. With every structured parser self-skipping, the format table drops to one {suffixes, parse} column named lyricFormats — no bySuffix/byContent split, no separate sniff-only TTML gate. Both the suffix and content-sniff paths share the same parser per format. * refactor(lyrics): strip BOM once at ParseLyrics entry for all paths Previously only the content-sniff path stripped the BOM; the suffix path relied on its callers (fromExternalFile via UTF8Reader) having already stripped it. That implicit contract was fragile — a caller passing raw BOM-prefixed bytes with a suffix would reach the parsers with the BOM intact (SanitizeText does not strip it). Strip once at entry so every path and parser sees clean bytes regardless of caller. No-op for already-stripped input. * refactor(lyrics): trim verbose comments to essential why * refactor(lyrics): move LRC parser to its own lyrics_lrc.go Extract parseLRC, the enhanced-LRC helpers (parseEnhancedLine, adjustGroup, stripEnhancedMarkers, shiftELRCCues), parseTime, and the LRC regexes from lyrics.go into lyrics_lrc.go, with the parseLRC tests in lyrics_lrc_test.go. This makes the layout symmetric — one file per format (lrc/srt/ttml/yaml) — and leaves lyrics.go holding only shared types and cue normalization. All moved symbols were already LRC-private; no behavior change. * refactor(lyrics): collapse ParseLyrics suffix/sniff branches into one loop Both modes differ only in which formats to try, so select candidates in a single loop (all formats when sniffing, the suffix's own otherwise) and run them through parseFirstMatch once. Drops the projected-slice make+index and the ContainsFunc closure; unmatched suffixes yield no candidates and fall to the plain-text floor, as before. * refactor(lyrics): apply simplify-review cleanups - stripBOM: bytes.TrimPrefix instead of []byte<->string round-trip (no alloc) - ParseLyrics: pre-size the candidates slice - move isTTMLDocument to lyrics_ttml.go beside its only caller (the dispatch layer should hold no per-format knowledge) * refactor(lyrics): simplify test descriptions for structured lyrics Signed-off-by: Deluan <deluan@navidrome.org> * refactor(lyrics): fold parseLyricsfile into lyricParser signature and rename file - parseLyricsfile now matches the lyricParser signature directly (reads via bytes.NewReader), removing the parseLyricsfileBytes adapter and the string(contents) copy; the lyricFormats table references it directly. - StructuredLyrics drops the vestigial LyricList{} init (json.Unmarshal overwrites; MarshalJSON owns the empty->[] invariant). - Rename lyricsfile.go -> lyrics_lyricsfile.go (and its test) to match the lyrics_<format>.go convention used by lrc/srt/ttml. * refactor(lyrics): move test-only parseTTML/parseSRT wrappers to test files These zero-arg wrappers (defaulting lang to "xxx") had no production callers after the consolidation — only the format tests used them. Move each beside its tests so the production files carry no test-only code. * build: exclude generated *_gen.go files from linting The plugin host *_gen.go files (ndpgen output) were tripping the whitespace linter despite carrying a generated marker. Exclude them by path so make lint and the pre-push hook pass on untouched generated code. * perf(lyrics): drop []byte/string round-trips in parsers Apply code-review feedback to remove avoidable allocations in the lyrics parsers. isTTMLDocument now takes []byte directly, so parseTTMLWithDefaultLang no longer copies its buffer into a string before the TTML probe. parseSRTBlock splits its block with strings.Split instead of converting to []byte and back per line. ParseLyrics hoists strings.ToLower(suffix) out of the format loop. No behavior change; the dropped len(scanner)==0 SRT guard was dead (strings.Split never returns an empty slice, and the existing len(lines)==0 check still covers empty input). Signed-off-by: Deluan <deluan@navidrome.org> * refactor(lyrics): colocate and unexport cue-normalization helpers Move the cue-normalization machinery out of lyrics.go into a dedicated lyrics_normalize.go (with lyrics_normalize_test.go), leaving lyrics.go to hold just the shared lyric types and their methods. lyrics.go was mixing the domain type/contract definitions with format-agnostic post-processing. Unexport normalizeLyrics, normalizeCueLines, and normalizeLineTiming: they have no callers outside the model package, so they should not be part of its public API. NormalizeCueEnds stays exported because the Subsonic enhanced-lyrics serializer (server/subsonic/lyrics.go) resolves cue ends per agent group while building the response; that is the only legitimate cross-package caller. Also includes a small no-op robustness tweak in parseLRC: len(times) == 0 instead of times == nil (equivalent here, more idiomatic). No behavior change. * test(lyrics): add direct coverage for NormalizeCueEnds NormalizeCueEnds is exported and carries the most intricate logic in the normalization cluster (fill-from-next, fill-from-fallback, both clamps, and the all-or-none clear), but was only exercised transitively. Add a focused spec covering each branch plus the empty-input and no-mutation guarantees, bringing the function to 100% coverage. * test(lyrics): cover legacy getLyrics across formats and sources Expand the legacy getLyrics e2e coverage from a single embedded-plain case to a table over all six fixtures: embedded LRC/plain/TTML and sidecar LRC/SRT/YAML. Each case asserts the v1 plain-text fallback contract — the structured lyric is flattened to LRC-style plain text with no timing markup leaking through (no LRC brackets, SRT arrows, or XML tags), regardless of the source format or whether it is embedded or a sidecar file. This pins the behavior that synced TTML/SRT/ YAML formats degrade gracefully to plain text on the legacy endpoint. * test(lyrics): cover songLyrics v1 vs v2 with word-level fixtures Correct and expand the e2e lyrics coverage to match the OpenSubsonic songLyrics extension contract: - v1 (getLyricsBySongId, no enhanced): line-level lyrics with no cueLine, kind, or agents — even for word-level formats (ELRC, Lyricsfile YAML). - v2 (getLyricsBySongId?enhanced=true): word-level cueLine surfaces for ELRC and YAML sources; kind="main" is set; a line-level source (SRT) still yields no cueLine even when enhanced. - legacy getLyrics (artist/title): the original Subsonic endpoint, flattening any format to plain text. A prior commit mislabeled this as the "v1 contract"; getLyrics predates OpenSubsonic and is unrelated to the extension versions. Drive these with the public-domain tests/fixtures/lyrics files (the same set the parser benchmarks use) so the e2e content stays in sync and actually carries the word-level timing needed to distinguish v1 from v2. The embedded "synced LRC" fixture is upgraded to ELRC (word-level); track counts are unchanged, so the rest of the suite is unaffected. * test(lyrics): parameterize v2 enhanced coverage across all formats Convert the v2 (enhanced) e2e block from three ad-hoc cases into a DescribeTable covering all six formats, matching the v1 and legacy tables. Each entry declares whether the source carries word-level timing: ELRC, TTML, and Lyricsfile YAML surface a cueLine; LRC, SRT, and plain text do not. All six get kind="main". Add word-level <span> timing to the first line of the auld-lang-syne.ttml fixture so TTML exercises the word-level cueLine path (the parser already supports <span begin/end>, but the fixture was line-level only). The first line now yields the same five word cues as the ELRC and YAML fixtures, keeping the table assertions uniform across formats. * fix(lyrics): honor caller language when Lyricsfile YAML omits it parseLyricsfile discarded the caller's language argument, so a Lyricsfile YAML parsed from an embedded tag or plugin response with no metadata.language was labeled "xxx" even when ParseLyrics was given a language. The SRT and TTML parsers already use the caller language as their default; fall back to it here too, preferring the document's own metadata.language when present. Also reword a misleading TTML comment: isTTMLDocument still runs an XML decode (it stops at the first element), so the skip avoids the full TTML parse, not the XML decoder entirely. * refactor(lyrics): consolidate lyrics parsing functions names Signed-off-by: Deluan <deluan@navidrome.org> * test(lyrics): drop test-only parse wrappers after parser rename Commit 48c0173e8 renamed the production parsers to parseTTML/parseSRT, which collided with the same-named test-only wrappers and broke the model test build (parseTTML/parseSRT redeclared). Remove the wrappers and call the production parsers directly with the placeholder language at each test site. * test(lyrics): complete the truncated enhanced-LRC fixture The auld-lang-syne.elrc fixture stopped after the first two stanzas (8 lyric lines) while every other format fixture carries the full 24-line song. Extend it to all 24 lines with per-word timing so it is a faithful enhanced-LRC sample and the EnhancedLRC parser benchmark runs on a workload comparable to the others. The first line's word timings are unchanged, so the e2e cueLine assertions still hold. --------- Signed-off-by: Deluan <deluan@navidrome.org>	2026-06-19 18:25:35 -04:00
Yuuta	3a14faa033	feat(subsonic): add structured sidecar lyrics support with OpenSubsonic v2 karaoke cues and agent layers (#5076) ... Expand backend lyrics support with richer sidecar formats and upgrade the OpenSubsonic songLyrics implementation to the version 2 structured karaoke contract, while preserving version 1 behavior by default. Sidecar formats and parsing: - Add a TTML parser (core/lyrics/ttml.go): clock time, offset time, bare decimal seconds, nested timing contexts, and token-level <span> timing for word/syllable karaoke. Parses Apple Music-style metadata tracks (translation and pronunciation/transliteration) and agent metadata into per-track agents[] plus per-cue-line agentId. Hydrates missing line timing from cue timing. - Add an SRT parser (core/lyrics/srt.go). - Add a LRCLIB Lyricsfile (.yaml/.yml) parser (model/lyricsfile.go): maps per-word lines[].words[] to cues with inclusive UTF-8 byte offsets and attributes overlapping lines to synthetic voice agents so parallel vocals split correctly in the enhanced response. - Extend LRC parsing for Enhanced LRC inline <mm:ss.xx> word-timing markers. - Add UTF-8 BOM and UTF-16 LE support for TTML/LRC sidecars. - Parse the above formats from embedded tags as well as sidecar files. Source resolution: - Default lyricspriority is now ".ttml,.yaml,.yml,.elrc,.lrc,.srt,.txt,embedded" so the new formats are discoverable without manual configuration. - Preserve configured source priority across duplicate media-file candidates instead of only checking the first DB match, so higher-priority sidecar lyrics on older duplicates can still win. - Raise the embedded-lyrics tag maxLength to 1 MB to fit word-timed TTML/Enhanced-LRC karaoke for a full song. OpenSubsonic songLyrics v2: - Advertise songLyrics versions [1, 2]. - With enhanced=true, getLyricsBySongId may return structuredLyrics.kind (main/translation/pronunciation), cueLine[] line-level karaoke groupings, cueLine.cue[] timed words/syllables with required UTF-8 byteStart/byteEnd, reusable structuredLyrics.agents[], and cueLine.agentId references. - Without enhanced=true, the response stays v1-compatible: no kind, no cueLine, no agents, no non-main tracks; the existing line[] payload is always populated so legacy clients keep working. Contract details: - cueLine is emitted only for synced lyrics with cue data. - Within a cueLine, cue.end is normalized all-or-none and overlaps are removed; overlaps across separate cueLines remain valid for parallel vocal layers. - Missing cue end-times are filled from the next cue or the parent line. - When cueLines share an index, the one whose agent has role "main" is first. - LyricCue.Value is serialized as XML chardata; cues with nil start are skipped rather than serialized as 0. Refactoring: - Move pure format parsers into model/ (lyrics.go, lyrics_ttml.go, lyrics_srt.go, lyrics_embedded.go, lyricsfile.go) and extract Subsonic response building into server/subsonic/lyrics.go. - Centralize lyric-kind constants and add Lyrics.EffectiveKind/IsMainKind. - Add gg.Clone helper. Spec references: https://github.com/opensubsonic/open-subsonic-api/discussions/213 https://github.com/opensubsonic/open-subsonic-api/pull/218 (songLyrics v2) https://github.com/opensubsonic/open-subsonic-api/pull/228 (cue byte offsets)	2026-06-19 12:00:58 -04:00
Deluan Quintão	6abc2ed517	fix(transcoding): preserve source metadata when transcoding downloads (#5628) ... * fix(transcoding): preserve source metadata when transcoding downloads Default transcoding commands used `-map 0🅰️0` with no metadata mapping, so transcoded files lost all source tags (title, artist, album, etc.). Downloads in the original format were unaffected because the file is copied byte-for-byte. Add `-map_metadata 0 -map_metadata 0:s:0` to the default commands. Both flags are required: `-map_metadata 0` copies format-level tags (MP3/FLAC sources) and `-map_metadata 0:s:0` copies stream-level tags (OPUS/OGG sources), which store tags at different levels. The flags are added in three coordinated places, since for users on the default command the args are built programmatically (buildDynamicArgs) rather than from the stored command string: - consts.go default commands, for new installations - buildDynamicArgs, the active path for default-command users - a migration updating only rows that still hold the exact old default, so customized commands are left untouched AAC is included for consistency but remains a no-op: its `-f adts` container cannot hold metadata, and the MP4 alternative breaks pipe streaming. Fixes #5623 * fix(transcoding): target audio stream for metadata and propagate ctx Address review feedback on the metadata-preservation change: - Use `-map_metadata 0:s:a:0` instead of `0:s:0` to copy tags from the first audio stream specifically. When a source has embedded cover art exposed as a video stream at index 0 (common in music files), `0:s:0` pulls the image stream's metadata and the audio tags are lost. Verified empirically with ffmpeg 7.1.3: a source with video at stream 0 and a tagged audio stream loses its title under `0:s:0` but keeps it under `0:s:a:0`; audio-only OPUS/MP3/FLAC sources are unaffected by the change. - Propagate the migration context via `tx.ExecContext(ctx, ...)` instead of discarding it, so the migration honors cancellation/timeouts. Claude-Session: https://claude.ai/code/session_015iFHDzX53wCKt11qFHMeZk	2026-06-18 09:58:43 -04:00
Deluan Quintão	838ceee26d	perf(subsonic): speed up artist search3 deep-offset pagination (#5620) ... * perf(subsonic): speed up artist search3 deep-offset pagination Empty-query and FTS artist search (search3/search2) paginated via a CROSS JOIN library_artist + DISTINCT in Phase 1 purely for library access control. The DISTINCT forced a temp b-tree over the whole junction table on every page, making deep offsets O(offset): ~200ms at offset 299k on 300k artists. Replace it with a join-free EXISTS predicate keyed on artist.id, backed by a new covering index on library_artist(artist_id, library_id). EXISTS keeps artist as the ordered driver and never fans out rowids, so Phase 1 stays a plain ordered scan that LIMIT/OFFSET can short-circuit. Admin, headless, and all-libraries users skip the filter entirely (the dominant case) for a flat ordered walk over the primary key. Measured on a 300k-artist / 1M-song library: admin/all-libs pagination is ~4.5-5.4x faster at depth (~180ms to ~33ms at offset 400k); restricted subset users keep correct, gap-free pages while also getting faster. The narrowing artist filter is applied at the subsonic layer only when the request targets a strict subset of the user's libraries, so the common case (and the admin fast-path) is never burdened with a redundant predicate. * fix(subsonic): narrow artist search by library set, not count narrowsArtistLibraries decided whether to add the subsonic-layer artist narrowing filter by comparing len(requested) < len(accessible). musicFolderId is not deduplicated, so duplicate IDs inflated the requested count: a user requesting ?musicFolderId=1&musicFolderId=1&musicFolderId=2 against three accessible libraries produced len([1,1,2])==3, which is not < 3, so the filter was skipped and the user saw artists from the third library too. Compare as set membership instead: the request narrows iff some accessible library is absent from it (requested is always a subset of accessible, validated upstream by selectedMusicFolderIds). This is immune to duplicate IDs. Add a regression test that fails against the old length-based check. Also consolidate the repeated EXISTS/no-DISTINCT/O(page) rationale that the prior commit spread across five sites down to a single authoritative comment on ArtistLibraryFilter, with the call sites referencing it. * perf(subsonic): drop redundant library_artist covering index The migration added an index on library_artist(artist_id, library_id) on the theory that the restricted-subset artist-search EXISTS needed it to seek by artist_id. Benchmarking on a 405k-artist / 5-library dataset showed no benefit: the EXISTS subquery constrains both columns (artist_id = and library_id IN), so SQLite already resolves it as a covering-index seek on the existing (library_id, artist_id) UNIQUE autoindex. With the new index present the planner still picks the autoindex and ignores it. Drop the migration and correct the comment. Removing ~11MB of dead index plus its write-amplification on every library_artist insert/delete, for zero query gain. * fix(scanner): mark artists missing when they lose their last library Artist search Phase 1 filters on artist.missing and Phase 2 inner-joins library_artist, so a non-missing artist with no library_artist row (an orphan) takes a pagination slot in Phase 1 and then vanishes in Phase 2, shortening the page and shifting deep offsets. The admin/headless search fast-path walks artist unfiltered, so it is fully exposed to this. Two paths created such orphans without updating artist.missing: - RefreshStats deletes library_artist rows whose stats are '{}' (artist lost all content in a library) after every scan. This is the common source. - Library deletion cascades away the library's library_artist rows. Mark newly-orphaned artists missing at both sources, so the shared 'missing = false' search filter excludes them immediately instead of waiting for a later scan. In RefreshStats the update only runs when the cleanup actually removed rows (the only way a new orphan can appear), so steady-state scans pay nothing; measured ~160ms on 300k artists only when orphans can exist. * refactor(subsonic): address review feedback on artist search filter Code-review follow-ups to the artist search pagination change: - ArtistLibraryFilter: short-circuit to a constant-false predicate when no library IDs are given, avoiding a degenerate empty IN () subquery. - ArtistLibraryFilter: add an inner LIMIT 1 to the correlated EXISTS so SQLite cannot flatten it into a fan-out join (an artist in multiple of the user's libraries would otherwise yield duplicate rowids and corrupt pagination). - narrowsArtistLibraries: compare accessible-vs-requested as a set lookup instead of slices.Contains in a loop. - searchConfig.LibraryFilter: document that a join-free filter is now a correctness requirement (DISTINCT was removed), not just a performance one. * docs: trim verbose comments in artist search/orphan code Condense the over-explained comments added in this PR to the essential 'why', removing repeated cross-references and restatements of the adjacent code. * fix(scanner): heal pre-existing orphan artists on full refresh The orphan-marking added to RefreshStats only ran when its empty-stats cleanup deleted rows, so it reconciled newly-created orphans but not ones already left in the database by older versions (whose library_artist row was deleted before this fix existed). Such legacy orphans would surface in the admin/headless search fast-path as short/gappy pages. Also run the orphan-marking on a full refresh (allArtists), so a full scan — which upgrades commonly trigger and users can run manually — reconciles the backlog. No migration needed; the runtime fixes prevent recurrence. * perf(subsonic): extend artist search fast-path to all-library users applyLibraryFilterToSearchQuery only skipped the library filter for admin and headless processes. A regular (non-admin) user who can access every library has the same result set as an admin, but was still given the EXISTS filter — an O(offset) cost for a predicate that matches every non-missing artist anyway. Skip the filter for them too, using a cheap library CountAll() (a count over the tiny library table) compared against the user's library count. On any error it falls back to the filtered path, which is correct, just slower. * fix(scanner): log error as trailing arg, not explicit error key Signed-off-by: Deluan <deluan@navidrome.org> * test(scanner): e2e guard for orphan artists under PurgeMissing Adds an end-to-end scanner test for the orphan-artist invariant fixed in RefreshStats: with Scanner.PurgeMissing enabled, removing all of an artist's files hard-deletes them, cascades away their media_file_artists rows, and RefreshStats then drops the artist's emptied library_artist row. The test asserts no non-missing artist is left without a library_artist row. Verified it fails without the RefreshStats orphan-marking and passes with it. * test(scanner): assert the orphaned artist is marked missing The orphan e2e test only checked the aggregate no-orphan invariant (orphanCount == 0), which a fully-deleted artist or an un-cleaned row would also satisfy — so it could pass without exercising the fix. Assert Pink Floyd's row specifically: missing=false before, missing=true after, and absent from the non-missing results. Verified it fails without the RefreshStats orphan-marking. * test(scanner): drop misleading non-missing-list assertion for orphan GetAll has no default missing filter, but selectArtist inner-joins library_artist, so an orphaned artist (no junction row) is excluded from the results whether or not it is marked missing. The Not(ContainElement) check therefore passed for the wrong reason. The direct floydMissing() == 1 query is the assertion that actually validates the missing flag; keep that plus the orphan-count invariant and an over-marking guard on The Beatles. * test(scanner): document why orphan check reads the artist row directly Clarify that GetAll cannot observe the orphan: selectArtist inner-joins library_artist, so an artist with no junction row is excluded from results whether or not it is marked missing. Asserting on GetAll would pass even without the fix, so the test reads the artist row directly to check the missing flag. * test(scanner): return descriptive artist state for clearer failures floydState returns PRESENT/MISSING/NOT_FOUND instead of 0/1/-1, so a failure reads '<string>: PRESENT to equal MISSING' rather than '0 to equal 1'. * refactor(subsonic): keep artist library scoping in the repository The search endpoint built a persistence-layer EXISTS predicate (persistence.ArtistLibraryFilter) and injected it into artistOpts.Filters — the only place the subsonic package reached into persistence, leaking a storage detail up two layers. Pass the same Eq{"library_id": ids} filter used for albums and songs, and let the artist repository translate it to the join-free library_artist predicate (scopeSearchToLibraries), where the junction knowledge belongs. The subset-vs- fast-path decision moves there too, so narrowsArtistLibraries and the persistence import are gone from the subsonic layer. Behavior is unchanged; coverage for the translation moves to artist_repository_test. * refactor(persistence): extract canonical markOrphansMissing helper The 'mark non-missing artists with no library_artist row as missing' invariant was hand-written as SQL in two places (RefreshStats and libraryRepository.Delete), in two slightly different dialects (not exists vs id not in). Extract a single artistRepository.markOrphansMissing method next to markMissing and call it from both sites, so the invariant has one definition. * fix(persistence): apply scoped library filter in both search phases Two bugs from moving the artist library-scoping into the repository: - Search() scoped opts.Filters for Phase 1 but still passed the original (unscoped) options to selectArtist, so Phase 2 re-applied the raw Eq{library_id} against the wrong columns and a restricted user's search returned nothing. Pass the scoped opts to both phases. - scopeSearchToLibraries dropped the filter unconditionally for admins, so an admin explicitly narrowing via musicFolderId (e.g. search3?musicFolderId=2) leaked content from other libraries. Compare the request against the user's visible library set (all libraries for admin/headless), narrowing whenever it is a strict subset. Both regressions were caught by the server/e2e multi-library suite. * fix(core): delete library and reconcile orphans in one transaction libraryRepository.Delete runs the FK-cascade delete and the orphaned-artist reconciliation (markOrphansMissing) as two writes on r.db. Called directly they autocommit separately, so an interruption between them could leave non-missing artists with no library_artist row — the orphan state the artist search fast-path forbids. Wrap the deletion in ds.WithTx at the core wrapper so both writes commit atomically; the watcher/scanner/broker side-effects stay post-commit. * refactor(persistence): unify artist search library scoping into one filter Phase 1 previously applied two overlapping library predicates: cfg.LibraryFilter (scoped to the user's libraries) AND options.Filters (the requested subset), producing two correlated EXISTS subqueries per rowid even though the request is always a subset of the user's libraries. And the 'does this user see everything' decision was implemented twice (userHasAllLibraries via CountAll vs scopeSearchToLibraries via set-membership), with applyLibraryFilterToSearchQuery as a third scoping path. Resolve the effective library scope once in Search() via searchScope (intersect the requested set with the user's visible libraries; nil = fast-path), clear opts.Filters, and realize that single scope as the only Phase-1 LibraryFilter. The visibility logic is now one pipeline: requestedLibraryIDs + visibleLibraryIDs + userSeesAllLibraries. Behavior unchanged; one EXISTS instead of two on the hot path, one source of truth for library visibility. * fix(persistence): harden artist search against malformed library_id filter Search consumed only an Eq{"library_id": []int} filter; an Eq whose library_id value wasn't []int slipped through unconsumed and would reach Phase 1's bare artist table (no library_id column) → SQL error. Recognize any Eq carrying a library_id key (isLibraryIDFilter) and always consume it, falling back to the user's visible scope for a malformed value. Non-library filters are still left in place for doSearch. * refactor(persistence): trim redundant comments and unexport artist library filter The artist-search-pagination work left dense explanatory comments, with the join-free / LIMIT-1 anti-flatten rationale and the orphan-artist mechanics each restated in several places. Consolidate each rationale into one canonical home (artistLibraryFilter for the EXISTS/LIMIT-1 trick, markOrphansMissing for the orphan lifecycle) and have the other sites reference it instead of repeating it. Also unexport ArtistLibraryFilter to artistLibraryFilter: its only caller is searchCfg in the same package and no test references it, so it never needed to be part of the package's exported surface. Comments only plus the rename; no behavior change. * refactor: add slice.ToSet and use it for the artist search subset check searchScope's subset test compared the requested libraries against the visible set with a nested slices.Contains, which is O(visible * requested). On an instance with many libraries (e.g. 100 libraries, a user granted 99) and an explicit musicFolderId request, that is ~9.8k comparisons; with a set it is ~200. Add a small reusable slice.ToSet helper (a slice -> map[T]struct{} set, collapsing duplicates) and use it to make the membership lookups O(1), restoring O(n+m) without the throwaway struct{}{} literal that an inline ToMap would need. No behavior change. * refactor(artist): move artistLibraryFilter to artist_repository Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org>	2026-06-16 21:47:15 -04:00
Deluan Quintão	08a027dbcc	fix(transcoding): honor player forced format on the WebUI transcode flow (#5613) ... * feat(stream): add ClientInfo.ForceFormat for browser-aware forced format Restricts the client to a forced transcoding format and suppresses direct play, but only when the client declares it supports that format. Part of #5583. * fix(transcoding): honor player forced format on getTranscodeDecision When the WebUI player has a forced transcoding format configured and the browser declares it can play that format, transcode to it (suppressing direct play). Fall back to normal negotiation with a warning when the format is unsupported. The MaxBitRate cap still applies on top. Fixes #5583. * test(e2e): cover player forced format on getTranscodeDecision Forced format honored when the client supports it, falls back to negotiation otherwise, and the MaxBitRate cap still applies on top. Part of #5583. * feat(ui): remove obsolete 'format ignored' helper text on player form The web player now honors the forced transcoding format, so the caveat added in #5611 no longer applies. Reverts the Transcoding field to a plain selector. Part of #5583.	2026-06-14 20:52:19 -04:00
Deluan Quintão	c4c70519b5	fix(transcoding): enforce server-side player MaxBitRate on /rest/stream (#5611) ... * fix(transcoding): enforce player MaxBitRate on getTranscodeDecision The Web UI streams via getTranscodeDecision, which (since #5473) ignored the server-side player config. Apply the player's MaxBitRate as a bitrate ceiling on the client's declared limits before MakeDecision, restoring per-player bitrate enforcement without reintroducing the forced-format override. Fixes #5583. * test(e2e): assert player MaxBitRate is enforced on getTranscodeDecision Invert the assertions added in #5473 that expected the player cap to be ignored; getTranscodeDecision now enforces it (issue #5583). * feat(ui): clarify web player ignores forced transcoding format Add helper text to the Transcoding field on the player edit form when the player is the NavidromeUI web client, since it enforces only the Max. Bit Rate, not the forced format. Part of issue #5583. * refactor(stream): extract ClientInfo.CapBitrate, share across transcode paths Move the player MaxBitRate ceiling logic into a canonical ClientInfo.CapBitrate method in core/stream, used by both getTranscodeDecision and the legacy ResolveRequest path. Removes handler-layer duplication and corrects a misleading comment that wrongly implied the legacy single-field cap was buggy. * fix(transcoding): downsample on legacy /stream when only player MaxBitRate is set A bare /stream or /download request from a player configured with a server-side MaxBitRate (but no forced format) was served raw, ignoring the cap. buildLegacyClientInfo now triggers DefaultDownsamplingFormat when the player MaxBitRate alone is below the source bitrate, matching the already-correct forced-format and request-bitrate paths. Part of #5583. * fix(ui): add Brazilian Portuguese translation for player transcoding helper text Translates the new resources.player.helperTexts.transcodingId key added for the web player transcoding-format clarification. Part of #5583. * fix(ui): restore Transcoding field styling and render helper text The TranscodingInput wrapper swallowed the variant SimpleForm injects into its direct children (field lost its outlined box) and put helperText on the ReferenceInput, which does not forward it to the input. Spread the form props onto ReferenceInput and move helperText to the SelectInput child so both the outlined styling and the helper text render. Part of #5583. * fix(i18n): update Brazilian Portuguese translation for album artist field Signed-off-by: Deluan <deluan@navidrome.org> * fix(ui): clean up comments in PlayerEdit component Signed-off-by: Deluan <deluan@navidrome.org> * test(ui): mock useTranslate in PlayerEdit test for determinism Avoid depending on ra-core's out-of-provider translation behavior, which can vary by version. Part of #5583. --------- Signed-off-by: Deluan <deluan@navidrome.org>	2026-06-14 16:52:01 -04:00
Deluan Quintão	c466f6b612	fix(artwork): prevent WebP segfault on 32-bit and disable WebP-by-default in Docker (#5606) ... * fix(artwork): avoid WebP segfault on 32-bit ARM On 32-bit ARM, the gen2brain/webp native libwebp path uses ebitengine/purego reverse callbacks, which purego does not support on that architecture. Selecting it crashes the process with a SIGSEGV when encoding or decoding WebP cover art, taking down the whole server on the first web UI artwork request (issue #5597). Force the safe WASM path on armv7/v6 in two layers: build the Docker arm binary with the gen2brain/webp "nodynamic" tag so purego is never linked, and add a runtime GOARCH guard in the init hook so source builds on 32-bit ARM are also protected. arm64 keeps the native libwebp path. * fix(artwork): also disable native WebP on 32-bit x86 purego's callback implementation is built with the constraint !386 && !arm, so 32-bit x86 (386) crashes with the same SIGSEGV as 32-bit ARM when the native libwebp path is used. Navidrome ships linux/386 and windows/386 builds, so guard 386 alongside arm: extend the runtime GOARCH check and the Docker nodynamic build tag to cover both. 64-bit arches keep the native libwebp path. * fix(artwork): rely on nodynamic build tag, drop ineffective runtime guard The previous runtime GOARCH guard did not actually prevent the crash: gen2brain/webp selects the native (purego) vs WASM backend in its own package init() and registers the purego write callback at import time, before any Navidrome hook runs. webp.Dynamic() is only a status getter, and Decode/Encode branch on the library's unexported flag, so the guard merely skipped a log line while the native path stayed active. The effective fix is the nodynamic build tag (applied for 32-bit ARM and x86 in the Dockerfile), which compiles gen2brain/webp WASM-only so purego is never linked. Drop the misleading guard and document that source builds on 32-bit architectures must be built with -tags nodynamic. * fix(artwork): don't enable WebP encoding by default in Docker The Docker image set ND_ENABLEWEBPENCODING=true, which (a) forced cover-art thumbnails through WebP for every install and (b) overrode any EnableWebPEncoding=false set in the user's navidrome.toml, since env vars take precedence over the config file in Viper. On 32-bit platforms the only available WebP backend is the WASM encoder, which is slow on the underpowered hardware those builds typically run on, so enabling it by default is the wrong tradeoff there. Remove the env default and leave EnableWebPEncoding off unless the user opts in. Combined with the nodynamic build tag, 32-bit images neither crash nor pay the WASM cost out of the box. A smarter automatic policy (use WebP only when native libwebp is available) can be revisited separately.	2026-06-13 13:58:26 -04:00
Deluan Quintão	af78bdeb3a	fix(artwork): never serve artist folder images as album art (#5596) ... * test(artwork): add failing e2e tests for artist image leaking as album art Reproduces a v0.62.0 regression (#5451/#5457): the album cover-art parent-folder fallback can include the artist folder, serving the artist thumbnail (e.g. Artist/folder.jpg) as album art for any album without image files in its own folder(s). Covers three scenarios: a plain Artist/Album layout with no album images, a single-disc album spread across sibling folders under the artist folder, and a spread album whose own front.jpg is shadowed by the artist's cover.jpg via CoverArtPriority order. Also adds an albumByName test helper for multi-album layouts. The tests are expected to fail until the parent-folder inclusion is gated by a structural check (skip the common parent when audio from other albums lives under it). * fix(artwork): never serve artist folder images as album art The album cover-art parent-folder fallback (introduced in #5451/#5457) could include the artist folder as a source of album images, serving the artist thumbnail (e.g. Artist/folder.jpg) as cover art for any album without image files in its own folder(s). This affected both plain Artist/Album layouts and single-disc albums spread across sibling folders under the artist folder. Gate the common-parent inclusion with a structural check: the parent only qualifies as an album root when no audio belonging to other albums lives in it or anywhere beneath it. An artist folder contains other albums' tracks, while an album root above disc subfolders contains only this album's, so the check works for any disc folder naming scheme and never affects the multi-disc fixes from #5376/#5456. A single-album artist with no images anywhere remains structurally indistinguishable from an album root and is a known residual case. * refactor(artwork): move album-root audio check into folder repository Replace the raw subtree SQL (LIKE/ESCAPE expression and wildcard escaping) that lived in core/artwork with an explicit FolderRepository.HasAudioOutsideFolders method, implemented in the persistence layer next to the existing folder-subtree query pattern. This also removes the test mock's brittle dispatch that sniffed the generated SQL to recognize the query; the fake now overrides the new method directly. Extract the whole parent-folder resolution from loadAlbumFoldersPaths into an albumRootParent helper, flattening four levels of nesting back into a linear flow. Behavior is unchanged; the unit test for a parent containing audio moved to the persistence suite, with added coverage for subtree boundaries, missing folders, and LIKE-wildcard escaping in folder paths. * refactor(persistence): use exists helper in HasAudioOutsideFolders Replace the hand-rolled count(*) query with the repository's canonical exists helper, as suggested in PR review.	2026-06-13 13:29:29 -04:00
Deluan Quintão	da56df3160	feat(smartplaylist): extend isMissing/isPresent to bpm, bitDepth and many text fields (#5603) ... * feat(smartplaylist): support isMissing/isPresent on mbz_* and lyrics fields Mark the six mbz_* MusicBrainz ID columns and the lyrics column as Nullable in the criteria field map, then extend missingExpr to handle string columns where absence is encoded as NULL or empty string (plus '[]' for lyrics). The Numeric/Boolean path (ReplayGain) is preserved via an explicit type check. * refactor(model): make MediaFile BPM and BitDepth nullable pointers Convert BPM and BitDepth fields in model.MediaFile from int to *int so that 'tag absent' is distinguishable from zero. The metadata mapper now uses NullableFloat for BPM (nil when absent or zero/unparseable) and only sets BitDepth when the audio property is non-zero (lossy codecs report 0). All read sites use gg.V() for zero-fallback deref so Subsonic API output and transcoding behaviour are byte-identical to before. The persistence layer bridges the existing NOT NULL DB columns by coercing nil to 0 on write and 0 back to nil on read in PostMapArgs/PostScan; a later migration task will drop those constraints. Hash upgrade safety is verified by a new MediaFile.Hash describe block: nil *int hashes identically to the old int(0) default via ZeroNil+IgnoreZeroValue, so no files will be spuriously re-imported after this change. Extra files touched beyond the plan's list: core/stream/legacy_client_test.go (BitDepth in model.MediaFile literals), persistence/mediafile_repository.go (NOT NULL bridge). * test(model): pin pre-conversion golden hashes for BPM/BitDepth * feat(smartplaylist): support isMissing/isPresent on bpm and bitDepth * feat(db): make bpm and bit_depth columns nullable, backfill 0 to NULL Drop the NOT NULL constraint on media_file.bpm and bit_depth via a lossless migration that converts legacy 0-means-absent values to real NULL. Remove the temporary shim in PostScan/PostMapArgs that was bridging the old NOT NULL columns to the *int model fields. Add round-trip persistence tests asserting NULL storage for nil pointers and correct value round-trip for non-nil pointers. * test(e2e): verify isMissing/isPresent partition for nullable fields Add DescribeTable covering bpm, bitdepth, lyrics, and mbz_recording_id: for each field, isMissing + isPresent song counts must equal the total library count, proving the nullable-column SQL is exhaustive and correct. * test(e2e): seed bpm tag so isMissing/isPresent partition is non-trivial * fix(model): omit bitDepth from JSON when absent instead of emitting null * feat(smartplaylist): support isMissing/isPresent on more string fields Enable isMissing/isPresent operators for album, comment, catalognumber, discsubtitle, albumcomment, sorttitle, sortalbum, sortartist, sortalbumartist, and explicitstatus by marking them Nullable in fieldMap. * refactor(smartplaylist): unify missingExpr column logic into one flow Collapse the numeric/string fork in missingExpr into a single empties-driven loop (numeric/boolean fields simply have no empties), and replace the duplicated IsTag/IsRole guard with a three-way switch that expresses the dispatch model once. No SQL semantics change for string fields; numeric/boolean fields now emit a single-element Or/And which squirrel parenthesizes (e.g. `(col IS NULL)` instead of bare `col IS NULL`) — update the affected test expectations accordingly.	2026-06-13 13:15:20 -04:00
Deluan Quintão	3b958dd6a7	refactor(stream): remove dead type branches from getIntClaim (#5594) ... The jwx library always deserializes numeric claims from a parsed token as float64, so the int and int64 branches in getIntClaim could never succeed and were dead code. Keep only the float64 path, which is the one actually exercised by the token round-trip, and update the comment to document the library behavior.	2026-06-10 23:15:58 -04:00
Deluan Quintão	bc107d1cee	fix(scrobbler): proxy NowPlaying even when ignoreScrobble is set (#5559) ... * fix(scrobbler): proxy NowPlaying even when ignoreScrobble is set When a client reports playback with ignoreScrobble=true, the reportPlayback handler suppressed both the scrobble submission and the NowPlaying update sent to external agents (Last.fm, ListenBrainz, plugins). These are independent concerns: ignoring the scrobble submission should not stop Navidrome from telling external services what is currently playing. The !params.IgnoreScrobble guard now applies only to the scrobble submission and play-count path; the NowPlaying dispatch is gated solely by the player's ScrobbleEnabled flag. This mirrors the legacy scrobble endpoint, where submission=false has always still set NowPlaying. * test(scrobbler): assert no scrobble dispatch when ignoreScrobble=true Address PR review feedback: explicitly verify that ignoreScrobble=true suppresses the scrobble submission (not just the play count) while NowPlaying is still dispatched, so the flag cannot regress into ignoring nothing. Also expand the NowPlaying gating comment to spell out the IgnoreScrobble vs ScrobbleEnabled rules and identify the external agents involved.	2026-06-03 20:03:08 -04:00
Deluan	2a43c4683e	chore: go fix ... Signed-off-by: Deluan <deluan@navidrome.org>	2026-05-28 22:13:05 -03:00
Deluan	833c50adc7	test(stream): fix data race in MediaStreamer transcoding cap tests ... The three It blocks that build a tight-cap streamer each spawned a fresh transcoding cache without waiting for its background initialization. The init goroutine reads conf.Server.CacheFolder, which races against SnapshotConfig's pointer-swap restore (Server = &restored) fired by DeferCleanup at the end of the spec. CI tripped the race under -shuffle=on -race; locally it reproduced about 10% of the time. Wait for tightCache.Available() before constructing the streamer, mirroring the outer BeforeEach. For the slot-saturation spec, swap in a blocking io.Pipe-backed mock ffmpeg so the cache's background copyAndClose can't drain the source and release the slot — the previous behavior happened to work only because the cache wasn't yet available and the no-cache path was exercised.	2026-05-28 00:07:49 -03:00
Deluan Quintão	74a5c0c6d1	fix(playlists): preserve unchanged fields on partial REST updates (#5542) ... * fix(playlists): preserve unchanged fields on partial REST updates (#5541) The REST adapter for playlists was discarding the `cols` argument that rest.Put provides (the list of fields actually present in the JSON body). updatePlaylistEntity then compared the deserialized entity's zero-valued Name/Comment against the DB row, decided "content changed", and called updateMetadata with &entity.Name — overwriting the name with the empty string. This surfaced via the Playlists list view's bulk "Make Public" action, which sends N parallel `PUT /api/playlist/{id}` requests with body `{"public": true}`. Affected playlists ended up with their names wiped (UI showed "Loading..." indefinitely). The per-row Public toggle was unaffected because it spreads the full record into the payload. Honor the cols list: gate every field-change check and every pointer passed to updateMetadata by whether the field was actually in the request body. Empty cols falls back to the existing "treat as a full record" behavior so non-REST callers are unaffected. * test(playlists): cover rules-only PUT + case-variant owner-change guard Follow-ups from manual testing and code review of the prior commit: - Manual testing confirmed Feishin-style rules-only PUT works correctly on the fix; add ginkgo regression tests for rules-only update, name+ rules combined, idempotent rules PUT (no-op), and bulk Make-Public preserving rules on smart playlists. - Keep the non-admin owner-change permission check gated on the deserialized entity content (not on `sent("ownerId")`) so a case-variant JSON key like {"OwnerId":"x"} can't downgrade the 403 to a silent 200. Go's json decoder is case-insensitive on struct field matching but rest.Put's field-name extraction is case- sensitive; the entity-based guard catches both spellings. The apply-side gating on ownerChanged still prevents the actual mutation, so this was a behavioral (not security) regression, but worth fixing. Adds a regression test asserting the case-variant key still returns rest.ErrPermissionDenied. - Correct misleading doc on applyContentUpdate: the path does not rewrite the backing M3U file; it goes through updateMetadata which bumps updatedAt and invalidates cached cover-art URLs. * fix(playlists): match REST cols case-insensitively (PR #5542 review) Go's encoding/json populates struct fields from case-variant keys like {"Name":"x"} or {"OwnerId":"y"}, but rest.Put's getFieldNames extracts raw JSON keys verbatim. With case-sensitive matching, sentFields would ignore the field on the update side — a request with {"Name":"Renamed"} would parse into entity.Name but then sent("name") returns false and the rename silently no-ops. Normalize both sides to lowercase. The entity-based owner-permission guard added in the previous commit remains as belt-and-suspenders but is now redundant with this change. Also clarify the applyContentUpdate doc comment: namePtr/commentPtr are nil when the field is absent OR present-but-unchanged, while publicPtr only tracks presence (an idempotent public is still forwarded). * refactor(playlists): drop redundant entity-based owner-permission guard The case-insensitive sentFields predicate already prevents case-variant JSON keys like {"OwnerId":"x"} from bypassing the ownerChanged check, so the duplicated entity-content guard is no longer load-bearing. Strengthen the regression test into a DescribeTable covering canonical, PascalCase, all-upper, and all-lower spellings to lock in the case-insensitive contract.	2026-05-27 23:29:17 -03:00
Deluan Quintão	823d851b75	refactor(transcoding): rename EnableTranscodingCancellation to Transcoding.EnableCancellation (#5523) ... Move the option into the nested Transcoding config group alongside the limit knobs it interacts with, so all transcoding-related settings live together. The old top-level name is still honored via the existing mapDeprecatedOption / logDeprecatedOptions plumbing, which forwards the value to the new key and logs a deprecation warning at startup. The old struct field is removed (the new field is the single source of truth); the deprecated default is removed so viper.IsSet correctly distinguishes "user set the legacy option" from "no one set it."	2026-05-24 00:51:58 -03:00