mirror of
https://github.com/navidrome/navidrome.git
synced 2026-08-31 07:30:32 +00:00
207 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Deluan Quintão
|
9ff0058620
|
fix: assorted scanner, plugin, and server fixes from the Go 1.27 work (#6050)
* fix(plugins): stop the cache janitor when a plugin cache is dropped
newCacheService started a ttlcache janitor goroutine that only stopped via the
explicit Close() path, so a cache service that was discarded without being closed
leaked its janitor for the process lifetime. It now registers the same
runtime.AddCleanup safety net that utils/cache.simpleCache already uses.
* fix(scanner): stop splitting multi-byte characters when truncating tags
sanitize() capped tag values with a byte slice, so a value whose limit falls in
the middle of a multi-byte character was stored as invalid UTF-8. defaultMaxTagLength
is 1024, which is not a multiple of 3, so any sufficiently long CJK title hit this.
Only trailing invalid bytes are trimmed, leaving bad bytes elsewhere in the value
untouched.
* fix(scanner): store MusicBrainz ids in their canonical form
uuid.Parse accepts a UUID wrapped in any two bytes, as well as braced and urn:
forms, but sanitize() returned the raw string. A tag like {<mbid>} or a quoted
value was therefore persisted with its wrapper into the mbz_* columns, where the
exact-match MBID search can never find it. The parsed value is now stored, which
also lowercases uppercase ids and adds the dashes to unhyphenated ones.
* fix(plugins): parse IPv6 hosts correctly in the websocket allowlist
isHostAllowed cut the host at the last colon, which mangles an IPv6 literal:
"[::1]:8080" became "[::1]" and "[::1]" became "[:". A plugin manifest could
therefore never allow an IPv6 host. It now uses net.SplitHostPort, falling back to
unwrapping the brackets when there is no port.
* fix(server): serve pprof profiles when a BaseURL is configured
net/http/pprof's Index resolves the profile name by trimming "/debug/pprof/" from
the raw request path, which never matches once MountRouter prepends the BasePath.
Requests for any profile without an explicit chi route fell through to the index
page, returning HTML with a 200 instead of the profile. The handler now strips the
BasePath first.
* test(scanner): run the goroutine leak check unconditionally
The scanner suite's goleak check only ran when the GOLEAK env var was set, so it
never ran in CI and could not catch a regression. It passes with the existing
ignore list, verified over repeated runs, so the gate is removed.
* fix(server): close the background image body on a non-200 response
serveImage returned early on an unexpected status code without closing the response
body, pinning the connection until the 5s client timeout. The nolint:bodyclose
above the request suppressed the linter that would have caught it, and its
justification only holds on the success path, where the body is handed to the
CachedStream wrapper.
* test(scanner): repair BenchmarkScan so it can actually run
The benchmark failed three ways before reaching its first iteration: it reused a
shared temp DB and tried to repoint the default library, it never loaded the config
defaults so the scanner got a concurrency of 0, and it lacked the notify ignore that
the suite already carries. tests.Init now takes a testing.TB so a benchmark can load
the test config the same way the suites do.
* refactor(artwork): drop the unused sourceFunc Stringer
sourceFunc.String derived a label from the closure's symbol name via reflection, but
nothing called it: the trace output builds its candidate labels from explicit strings.
Whole-program analysis confirms it is unreachable, and dropping it removes a
reflection-based dependency on compiler closure-naming details.
* refactor(plugins): reuse extractHostname in the websocket allowlist
The IPv6 host parsing added for isHostAllowed duplicated extractHostname, which
already lives in the same package and backs the HTTP client's identical allowlist
check. Two copies of a security-relevant parser can drift, so the websocket service
now calls the existing helper. The port-stripping specs move into the URL Validation
block that already covered them.
* perf(scanner): bound the tag truncation trim to a partial rune
The trim loop dropped every trailing byte that failed to decode, so a value ending
in a long run of invalid bytes was walked one byte at a time: a 1 MiB lyrics tag
measured 2.58ms against 45ns for a normal cut. A partial rune is at most 3 trailing
bytes, so the loop is capped there, which also stops it consuming a pre-existing
invalid run.
* test: tighten the tests added with the Go 1.27 bugfixes
Drop the testItem stub in favour of the package's own cacheKey, register the pprof
test profile once at package scope, and replace the hand-rolled goroutine settle
loop with Eventually. Also corrects a comment that credited a TestMain the scanner
suite does not have.
* test(scanner): ignore notify's nonrecursive-tree goroutines on Linux
The goroutine leak check only ignored the recursive tree (macOS/FSEvents).
Linux CI uses inotify, whose nonrecursive tree leaks dispatch and internal
goroutines after Stop(), failing the check.
* fix(scanner): avoid a truncation panic when MaxLength is 1 or 2
A value of only UTF-8 continuation bytes drained the partial-rune loop to
empty, then sliced value[:-1] and panicked. Break when DecodeLastRune returns
size 0 (empty string) by testing size != 1 instead of size > 1.
* fix: address Codex review on the pprof base path and scan benchmark
- profilerHandler: treat a root BasePath ("/") as no prefix, so http.StripPrefix
keeps the leading slash chi needs; without this the profiler 404s when BaseURL
is "/". Cover the root case in the test.
- BenchmarkScan: make it run regardless of test/benchmark ordering. Add
singleton.DeleteInstance so a fresh DB is opened after TestScanner closes the
shared one, guard driver registration with sync.Once so the rebuild does not
re-Register, and ignore the Ginkgo interrupt-handler and Linux notify
goroutines the preceding suite leaves behind.
* fix: address Codex round 2 on BasePath trailing slash and benchmark DB cleanup
- profilerHandler: trim all trailing slashes (TrimRight), not just a bare "/", so
a BaseURL like "/music/" strips correctly instead of 404ing. Cover it in the test.
- BenchmarkScan: keep and defer db.Init's closer so the DB is closed before
b.TempDir cleanup, which otherwise cannot delete the open SQLite/WAL files on Windows.
|
||
|
Deluan Quintão
|
ff033d8db6
|
chore(deps): upgrade to Go 1.27 (#5990)
* build: upgrade to Go 1.27 Bumps the toolchain in go.mod, both golang base images in the Dockerfile, and the devcontainer VARIANT. CI needs no change, as the workflows resolve the version through go-version-file: go.mod. Tests, race tests, build and vet all pass on go1.27.0. * build: upgrade golangci-lint to v2.13.0 v2.13.0 is the first release built with Go 1.27, so it can lint a module whose go directive is 1.27. It also enables gosec's G404 on math/rand/v2, which flags the three rand.Shuffle call sites. Shuffle order is not a security decision, and the crypto-backed alternative in utils/random costs 25x and allocates per swap, so the call sites are annotated rather than the rule excluded, keeping G404 active for the cases where it would matter. * chore(deps): update Go dependencies to latest versions Signed-off-by: Deluan <deluan@navidrome.org> * build: bump golangci-lint to v2.13.2 --------- Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
Deluan Quintão
|
3e55886195
|
feat: add optional natural sort order for names and titles (#6015)
* feat: add optional natural sort order for names and titles Album, artist, song and playlist lists sort with a plain text comparison, so names containing numbers come out as "Foo 1, Foo 10, Foo 2" instead of "Foo 1, Foo 2, Foo 10" (issue #4554). Adds an EnableNaturalSorting option, default off, that switches those sorts to a NATSORT collation registered on every connection and backed by natural.CompareFold. natural.Compare gained an ASCII case-folding variant because it replaces 'collate nocase': sort_* columns hold raw tag values, so without folding they would order uppercase before lowercase. Applying the collation only inside mapSortOrder would have missed the default configuration entirely, since that mapper runs only when PreferSortTags is on. setSortMappings now also rewrites the order_* columns when natural sorting is enabled on its own. Sorts over plain text columns that are not order_* columns (playlist.name, album.name, media_file.title, playlist_tracks title) are wrapped explicitly, and qualified with their table because 'user' is joined and also has a 'name' column. The option defaults to off because the collation cannot use the existing indexes: measured on a synthetic 110k album library, the first page of an album-by-name listing goes from 0.03ms to 14ms. Indexing the expression was rejected outright - an index declared with a custom collation makes the whole database unreadable to any tool that does not register it, including the sqlite3 CLI, which fails even on 'select count(*)' and 'pragma integrity_check'. * refactor: fold the two sort-order mappers into one mapSortOrder and mapNaturalOrder shared the same regex and loop, differing only in the expression they substituted, and setSortMappings picked between them with a two-case switch. mapSortOrder now selects the column shape itself and defers to collatedSort for the collation, so the 'collate' clause is emitted in one place and the caller only has to decide whether any mapping is needed at all. The mapper tests were three near-identical cases that each hard-coded one flag combination; they are now a DescribeTable covering all four combinations of PreferSortTags and EnableNaturalSorting, which the previous set did not. The album sorting specs collapse the same way. Behavior is unchanged. * fix: leave plain sort columns alone when natural sorting is off collatedSort wrapped its column unconditionally, so the tiebreakers added for plain text columns picked up 'collate nocase' even with EnableNaturalSorting off. media_file.title, the playlist_tracks alias of it, and user.user_name are all declared without a collation, so a default install would have silently switched those tiebreaks from binary to case-insensitive ordering. Only playlist.name was already NOCASE and genuinely unaffected. The helper is now naturalSort and returns the column untouched unless the option is on, so the default path keeps the collation each column was declared with. sortCollation had a single remaining caller and folded into mapSortOrder. Tests: the CompareFold table body was a verbatim copy of the Compare one, so both now go through one expectOrder helper, and the album sorting specs inline two single-use closures. * fix(natural): defer the leading-zero tie-break to keep ordering transitive Compare applied the padding difference between numerically equal digit runs only when one side ended at the digit boundary, and ignored it mid-string. That made the relation intransitive: CompareFold("1","1a") < 0 and CompareFold("1a","01a") == 0, yet CompareFold("1","01a") > 0. SQLite requires a collating function to be transitive and leaves ORDER BY undefined otherwise, so registering this as NATSORT was not safe. Reproduced with the real driver on three artist names that occur in practice - "3", "3 doors down" and "03 greedo" - where paging one row at a time returned "03 greedo" twice and dropped "3" entirely. The padding difference is now carried as a tie-break that is applied only when the strings are otherwise equal, which restores transitivity while keeping the documented intent (a01 < a1, a0 < a00). Three existing entries changed: each asserted that two distinct strings compare equal, which was the same defect seen from the other side. Found by the Codex review on #6015. |
||
|
Deluan Quintão
|
59810c3d59
|
feat(jellyfin): non-expiring, audience-scoped tokens revocable by password change (#6013)
* feat(auth): add per-user token_epoch column and bump method * feat(auth): add aud and ep claims, omitted when zero * feat(auth): add CreateAPIToken for non-expiring, audience-scoped tokens * feat(auth): add CheckClaims for epoch and audience validation * feat(jellyfin): issue non-expiring, jellyfin-scoped access tokens * fix(subsonic): reject API-scoped and revoked tokens on the jwt path * fix(server): reject API-scoped and revoked tokens on the native API * fix(server): pin the token-subject guard and stop leaking test config Adds a regression spec for the DevAutoLogin/ExtAuth guard in tokenAllowed, switches its comparison to case-insensitive to match the user lookup's own COLLATE NOCASE semantics, and restores Subsonic JWT test config after each spec instead of leaking SessionTimeout. * feat(request): add a token epoch holder for handler-to-middleware signalling * refactor(server): write the refreshed JWT header after the handler runs * feat(auth): revoke all tokens for a user when their password changes * fix(server): restore Unwrap on the JWT refresh writer so SSE write deadlines apply * test(auth): pin that non-session tokens reject API access tokens * test(jellyfin): pin token scoping and epoch revocation end to end Exercises auth.CreateAPIToken and CheckClaims against the real Jellyfin router and SQLite DB: the minted token has no exp and is aud-scoped to jellyfin, and bumping token_epoch through the real UserRepository revokes an already-issued token on the next protected request. * test(nativeapi): pin the token-epoch handoff through a real password-change request Drive a self password change through the real Authenticator/JWTRefresher chain and a real SQLite-backed userRepository, so the epoch handoff between Put and the refreshed-token writer is verified end to end, not as two separately-tested halves. Also fix tokenAllowed to read the enriched ctx it was given instead of r.Context(), so its warning log carries the username. * refactor(server): drop tokenAllowed's now-unused request parameter Finding-2 already moved every use to ctx; r was dead weight. Also note in the new nativeapi test why it must stay the package's only real-DB spec: db.Db() is a process-wide singleton its cleanup closes for good. * refactor(auth): remove duplication in claim decoding and token minting * refactor(auth): group aud with the standard JWT claims * refactor(auth): read aud with the standard-claim accessor pattern * fix(log): redact every api_key spelling the Jellyfin API accepts * fix(auth): bind session tokens to the user id, not just the username * fix(auth): return the token epoch from the same atomic increment * fix(auth): bump the token epoch in the same statement as the password write * chore(auth): trim comments to the why-only budget |
||
|
Deluan Quintão
|
c26f6f9e98
|
feat(artwork): store the resolution trace so artwork explain works offline (#5980)
* feat(artwork): record the resolution trace so explain works without --live The worker never attached a ChainTrace, so `artwork explain` had to re-walk the priority chain at CLI time. That reconstruction could disagree with what actually happened, and without --live it could not report the external tier at all. The worker now traces every acquisition and stores it. `explain` reads the stored trace by default and reports when it was recorded; --live re-walks and calls the agents. Disc artwork keeps no row, so it always walks live. A chain trace alone would have explained almost nothing about failures: six of the seven ways an item can fail happen after the chain has already picked a winner. The trace now covers those stages too, and has somewhere to live when they fail: the retrying queue row carries the last failure, and the state row keeps it in last_failure once the retry budget is spent and the queue row is deleted. Measured on a copy of a 682MB / 43.6k-item library: +9.7MB (+1.4%). No row crosses the WITHOUT ROWID overflow threshold, so list hydration is unchanged; only full scans of item_artwork, which no request performs, read more pages. * test(artwork): pin the give-up ordering that keeps a failure for unresolved items recordGiveUp updates an existing row, and for a kind with a recheck path that row is only created moments earlier by the absent settle. Recording before the settle would lose the failure for every item that never resolved, with nothing to catch it. * refactor(artwork): tighten the trace code after review Four fixes worth taking: The doc comments on ChainTrace and chainState.trace still said the worker never attaches a trace and resolution stays allocation-free — the exact invariant this branch reverses. explain's report field meant both "the chain shown was walked just now" and "go out for real", and was being passed to loadPluginAgents, which --live documents as the only thing that may open external connections. Renamed to `walked` and restored explainLive as the sole input to that decision. A stored Detail is an error string on the failure paths, with no bound. The measured "no row reaches the WITHOUT ROWID overflow limit" only holds while it is bounded, so cap it at 200 runes. offlineGate was a factory returning a constant closure; make it a plain gateFunc like its sibling passthroughGate. Collapse five copies of the age-a-queue-row loop in the worker tests into one helper. * refactor(artwork): drop the offline explain walk, now that traces are stored `artwork explain` reported the external tier without calling it, so a diagnostic could not add load to a provider already rate-limiting us. Reading the stored trace answers that better: it reports what the agents actually returned, not what would be tried. Nothing could reach the offline gate any more. It was installed only for a walk with --live unset, which now happens for disc artwork alone, and disc rejects the external candidate before any gate call. That made the gate, its sentinel error, the would-try outcome and two of explain's verdicts unreachable. Removes offlineGate, errOfflineSkipped, OutcomeWouldTry, the NewTracingResolver live parameter and the CreateArtworkResolver argument threaded through wire. Verified against a copy of a real library: disc artwork with "external" first in DiscArtPriority and external services enabled still records the skip and issues no agent call. * fix(artwork): make explain's no-network guarantee structural, not incidental Serving falls back disc -> album and track -> disc -> album. The resolver layer explain uses has no such fallback today, so dropping the offline gate did not leak. But the guarantee rested on which chains happen to lack an external tier, and the serving layer already shows the fallback shape someone could mirror. Without --live the tracing resolver is now built with no agents at all, so no chain and no fallback added later can reach a provider. That is stronger than the gate it replaces, which only intercepted the call. The test pins it against exactly that regression: with the guard removed and the serving fallback mirrored into resolveDisc, it fails. * refactor(artwork): trim the trace plumbing EncodeTrace was exported for nobody: only this package writes traces, and cmd reads them. It becomes a ChainTrace method, which also drops the copy Steps made for a caller that only wanted to serialize. explain's report carried queuedSteps and failureSteps, both pure functions of the queue and state rows already in the struct, which let a test set the two out of step with each other. formatExplain derives them, as it already does for every other display value. The trace row format and its tabwriter empty-cell rule lived in two places, and the "nothing was ever recorded" predicate in three. * fix(artwork): clear the queue trace on a fresh re-enqueue Enqueue's conflict clause reset attempts to 0 but left the new trace column, so after a scan or refresh re-enqueued a previously-failed item artwork explain showed "Attempts: 0" next to the prior lifecycle's "Last attempt failed" trace. Clear trace in Enqueue (a fresh lifecycle has no last attempt); EnqueuePreservingBackoff still keeps it. * fix(artwork): treat a processing-stage error as indeterminate in explain A read/hash/decode/store failure records an OutcomeError step and writes an absent row, but explainResult only mapped external errors and unreadable candidates to indeterminate, so the default verdict read "not resolved" — presenting a processing failure as a definitive miss. The worker retries these exactly as it retries an unreadable candidate, so classify any OutcomeError as indeterminate too. * fix(artwork): record a trace step when a chainless resolver faults Playlist and radio resolvers walk no priority chain, so a fault (unreadable upload/sidecar, or an m3u fetch error with no grid) returned localError/extError without recording any trace step. The attempt then encoded [], leaving artwork explain with an empty "Last attempt failed" and "Gave up after". Record a fallback step in the faulted-no-image branch when nothing else did, and carry the source label through resolveLocalFile so the step can name it. * fix(artwork): trace the m3u failure at its source, not via the empty guard A playlist's grid sampling records album-chain steps into the shared trace, so the processor's empty-trace fallback no longer fires when the m3u remote image fetch failed — the error that forced the retry was omitted from explain. Record it where it happens, in resolvePlaylist's external step, as external:m3u. * test(artwork): skip the chainless-fault spec on Windows The spec provokes an open fault with a non-directory parent, but Windows maps that to a not-exist error, so localError is never set and the item resolves absent instead of failed. The sibling failed-on-unreadable-upload spec skips Windows for the same class of reason. * fix(artwork): don't label an absent empty-chain row as pre-tracing explain reported "resolved before traces were recorded" for any stored row with an empty chain, but an empty CoverArtPriority records a real, empty [] chain and resolves absent. A recorded resolution that finds an image always records its winning candidate, so only a row with a hash and no chain predates tracing; split on the hash and report an absent empty chain plainly instead. * fix(db): retimestamp the artwork trace migration after rebase master merged a 2026-08-18 migration, so the original 2026-08-16 timestamp is now older than the newest on the base branch and Goose would silently skip it on an already-upgraded database. Bumped past it; the SQL is unchanged. * fix(artwork): keep the m3u error detail in the trace The m3u trace step recorded OutcomeError with no detail because resolveExternalStep collapsed the gate's error to a bool, so explain showed only "external:m3u error -" and could not tell a timeout from an HTTP error or an open breaker. Return the error (normalizing not-found to nil so it stays a definitive miss, not a failure) and store its message as the step detail; encodeSteps already bounds it. * docs(artwork): note the give-up write relies on serial draining recordGiveUp writes last_failure unconditionally; that is only correct because the drain resolves each item serially, so no concurrent success can store artwork between the write and the queue delete. Record the invariant at the call site. |
||
|
Deluan Quintão
|
2e03766a9d
|
fix(playlist): preserve smart playlist song count on re-import (#5907) (#5908)
* fix(playlist): preserve smart playlist counters on re-import (#5907) * perf(playlist): skip re-importing unchanged NSP files (#5907) * feat(playlist): also store content hash for M3U imports (unused for now) * fix(playlist): return stored record when skipping unchanged NSP import Skipping before copying the stored identity broke the ImportFile(sync=false) contract: callers received an ID-less playlist and the requested Sync change was silently dropped. * refactor(playlist): hash imports once at the caller; protect smart counters in Put Move content hashing out of both parsers into the code that owns the file (parsePlaylist and ImportFile), removing the NSP double-buffer and the duplicated hashing idiom. Put now drops song_count/duration/size for smart playlists (PostMapArgs), disarming the counter-zeroing trap for all callers. * fix(playlist): invalidate imported hash when rules are edited via API Without this, a rules edit through the REST API kept the stored file hash, so every scan skipped the unchanged file and never restored the file-backed rules while sync was on. * test(playlist): verify smart counters survive a re-import, end to end The existing Put test seeds the stored counters with a raw SQL update, so it pins the guard in PostMapArgs but not the pipeline around it. This test drives the counters through a real evaluation instead: it saves a smart playlist, reads it with GetWithTracks to populate song_count/duration/size, then saves the playlist the way the scanner rebuilds it after parsing the .nsp file, with the counters back at zero. Both routes fail without the guard, and the new one covers the exact sequence reported in #5907. Test taken from #5970, which diagnosed the same root cause independently. Co-authored-by: Junker der Provinz <133605895+junkerderprovinz@users.noreply.github.com> * test(playlist): build the service with artwork.NewUploader The artwork pipeline in #5847 replaced core.NewImageUploadService() with artwork.NewUploader(ds) and updated every call site it could see. The five call sites this branch adds were written against the old constructor, so the merge applied cleanly but left the package uncompilable. * fix(db): re-stamp the imported_hash migration after the master merge Master gained three migrations while this branch was open, the newest being 20260816180040. The original 20260808200333 stamp now sorts before them, so any database already upgraded past that point would skip this migration entirely and never get the imported_hash column. Same SQL, current timestamp. * refactor(playlist): hash imported playlists with xxh3 and the id encoding ImportedHash is a change detector, not a security boundary, so it does not need a cryptographic digest. xxh3 is already a direct dependency and is used the same way to fingerprint files in the artwork image store. Encoding the 128-bit digest with id.Encode stores it in the same 22-char base62 form as every other id in the schema, down from 64 hex chars. No migration is needed: the imported_hash column has not shipped in a release, so no database holds a value in the old format. * refactor(playlist): extract the imported-playlist fingerprint helper Both import paths encoded the hash inline, so how a playlist file is fingerprinted lived in two places. A third import path that encoded it differently would silently never match the stored value, turning the unchanged-file skip into a no-op. --------- Co-authored-by: Junker der Provinz <133605895+junkerderprovinz@users.noreply.github.com> |
||
|
hotorcelexo
|
ea1e2b95a7
|
fix(db): keep album created_at in the driver's timestamp format when copying (#5867)
* fix(db): keep album created_at in the driver's timestamp format when copying Signed-off-by: IgorPolyakov <igorpolyakov@protonmail.com> * fix(db): move created_at renormalize migration after merged migrations The migration was versioned 20260813140000, which is older than 20260815015320 (already merged). goose.UpContext runs without WithAllowMissing, so any database that already applied the newer migration would fail with "found 1 missing migrations" and db.Init would log.Fatal on startup. --------- Signed-off-by: IgorPolyakov <igorpolyakov@protonmail.com> Co-authored-by: Deluan Quintão <deluan@navidrome.org> |
||
|
Deluan Quintão
|
82fde00ecc
|
feat(scrobbler): add per-user scrobble filter (#5964)
* feat(scrobbler): add scrobble_filter column to user * feat(scrobbler): validate scrobble filter criteria on user save * refactor(persistence): make smart playlist join helpers package-level * feat(scrobbler): add MediaFileRepository.MatchesCriteria * feat(scrobbler): filter external scrobbles with per-user criteria * feat(ui): add scrobble filter field to user form * fix(scrobbler): default scrobble_filter to empty string for existing users * refactor(scrobbler): also gate playback reports on the scrobble filter Playback reports carry the same track metadata to plugin scrobblers, so a filtered track leaked through that third dispatch path. Skip the filter evaluation entirely when no scrobbler is active. * refactor(persistence): move criteria join building into criteria_sql.go The join set a criteria needs was decided in criteria_sql.go but built in smart_playlist_repository.go, so both callers had to pair the two by hand. * refactor(persistence): unexport smartPlaylistCriteria methods The type never leaves the package, so the exported names advertised an API that callers outside persistence could never reach. Also disambiguates where/orderBy from squirrel's SelectBuilder methods of the same name. * fix(ui): cap the scrobble filter field width fullWidth stretched it across the whole page next to 256px inputs. Bounded at 40em, with two rows and a resize handle so JSON rules stay readable. * refactor(ui): move scrobble filter input in UserEdit component * feat(ui): add pt-BR translations for the scrobble filter * fix(scrobbler): take the filter verdict before incPlay incPlay mutates play counts and dates a filter can test on, so evaluating at dispatch time let one play decide differently on either side of the increment: a track could be scrobbled despite matching, or lose only its stopped report and strand presence plugins. Reject limit/offset too, rather than silently ignoring part of a rule copied from a smart playlist. * fix(scrobbler): filter the report from an expired session The expiry callback runs with a stub user carrying no filter, so evaluating there always returned false and leaked the track to plugin scrobblers. That is the normal path for clients that never send stopped, such as legacy Subsonic now-playing. Carry the last verdict on the session instead. * refactor(scrobbler): skip the now-playing enqueue instead of threading the verdict Queuing an entry only to drop it at dispatch also cancelled a pending announcement for the previous, unfiltered track, since the queue is keyed by player and a new entry replaces the old one. * fix(scrobbler): evaluate the filter regardless of active scrobblers The verdict is stored on the session and dispatched at expiry, so skipping evaluation when no scrobbler was active let a plugin enabled mid-session receive a filtered track. The empty-filter guard above already gives servers without scrobbling the same free path, so the shortcut only ever applied to users who had a filter set. |
||
|
Deluan Quintão
|
95b8d9dd04
|
perf(genre): index genre filtering via join tables across all APIs (#5940)
Filtering by genre scanned every media_file/album row and JSON-parsed its `tags` column (a per-row json_tree(tags) EXISTS) with no usable index, so Finamp's genre screen took 1.9-6.5s per tap against a ~97k-track library. Album and album-artist genre queries had the same unindexed shape. Add normalized media_file_tags and album_tags join tables (genre only for now, via an indexedTagNames allowlist), populated by a new updateTags in Put (mirroring updateParticipants) and backfilled in the migration. Genre filtering across the Jellyfin, Subsonic and native APIs now runs as an index-backed semi-join through shared TagIDSemiJoin/TagNameSemiJoin helpers instead of a full scan. On a copy of the production DB the per-request cost for a typical genre drops from ~330ms to sub-millisecond, adding ~10MB. |
||
|
Deluan Quintão
|
7993fb9158
|
perf(persistence): use *_artists join tables for artist participant filters (#5930)
* perf(persistence): use *_artists join tables for artist participant filters The artist_id/artists_id, role_<role>_id and role_total_id filters, plus the AlbumsByArtistID/AlbumsByContributingArtistID/SongsByArtistID helpers, scanned every album or media_file row through json_tree(participants, ...), which no index can serve — the cause of multi-second artist pages on large libraries (discussion #5929). Rewrite them to semi-join the album_artists and media_file_artists tables via a shared ParticipantIDFilter helper. The join tables are written in the same transaction as the participants JSON, so results are unchanged. album_artists' unique constraint led with album_id, so artist-driven lookups had no usable index. Rebuild the table with the constraint reordered to (artist_id, album_id, role, sub_role), mirroring media_file_artists, instead of adding a fourth index: measured within 4% of a dedicated covering index (geomean -92.5% vs json_tree on a 96k-track production copy) while saving ~7MiB and per-scan write amplification. Album-side consumers (participant rewrites, FK cascades, markMissing) keep using album_artists_album_id, and updateParticipants' ON CONFLICT target already names artist_id first. The rebuild is linear work: 1.1s on a 113k-row production copy. * chore(gitignore): add temp benchmark files to ignore list * fix(persistence): clear album_artists when an album is saved without participants albumRepository.Put skipped updateParticipants when the Participants map was empty, so a hypothetical save with no participants would write {} to the JSON column but leave stale album_artists rows behind, now visible through the semi-join filters. No current caller can hit this (albums built by MediaFiles.ToAlbum always have participants), but make Put unconditional anyway, matching mediaFileRepository.Put, so the join table always moves with the JSON. Raised by Codex review on #5930. |
||
|
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> |
||
|
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). |
||
|
Deluan
|
b27d6f61ae |
fix(db): make album ReplayGain backfill scale to large libraries
The windowed-CTE backfill was compiled as a correlated scalar subquery re-evaluated once per album (a full media_file group-by + window sort each time), which never finished on a large library. Stage the ReplayGain-bearing rows into an indexed temp table once and update only the affected albums: ~14s on a 727MB / 6945-album library, vs. effectively never. |
||
|
Deluan Quintão
|
4efd92cf83
|
feat(server): expose album-level ReplayGain in albums (#5816)
* feat(model): aggregate album ReplayGain in ToAlbum * feat(db): add nullable album ReplayGain columns with backfill * feat(persistence): persist album ReplayGain fields * feat(jellyfin): expose album NormalizationGain from ReplayGain * refactor(model): lazy-init mostFrequentPtr map; clean up RG test rows * fix(db): backfill album ReplayGain with most-frequent value, not max A plain max() picked a minority outlier that diverged from MediaFiles.ToAlbum (which uses the most-frequent value), and GetTouchedAlbums never re-derives an unchanged album, so the wrong value would persist. Reproduce the modal aggregation via grouped CTEs, which also groups media_file once instead of a per-album correlated scan. * refactor(model): return an owned pointer from mostFrequentPtr Avoid aliasing a MediaFile field so the resulting Album is independent of the source slice. |
||
|
Deluan Quintão
|
53d54baef0
|
fix(jellyfin): stream collection responses to prevent OOM on large libraries (#5783)
* fix(jellyfin): stream /Items responses to prevent OOM on large libraries
Finamp's library sync issues an unbounded GET /Items?IncludeItemTypes=Audio
with Fields=MediaSources and no Limit. On a large library this built the whole
result set — every MediaFile and every BaseItemDto, each fat with MediaSources —
in memory, and json.Encoder then buffered the entire ~200MB response before
writing a byte. Measured on a 96k-track library, one such request peaked near
1.6GB RSS; in a memory-limited container the resulting slowness made clients
retry, stacking concurrent full-library builds until the process was OOM-killed.
Stream the song listing straight from a DB cursor (MediaFileRepository.GetCursor),
mapping and encoding one row at a time, so peak memory is bounded to about one
item regardless of library size. The cursor is opened lazily, when streaming
begins, so it doesn't hold a DB connection across the CountAll and ServerId
lookups that run first (ServerId can write on first use and would deadlock
against an open reader). Pagination still works — the cursor query carries
LIMIT/OFFSET/ORDER BY from StartIndex/Limit/SortBy — and TotalRecordCount stays
the full count. Other materialized responses stream their JSON too, avoiding the
encoder's buffer-the-whole-output cost.
Also bound artwork image concurrency with the same server.ThrottleBacklog that
Subsonic's getCoverArt uses, so a burst of image requests during sync can't
exhaust memory through concurrent decode/resize.
Verified against a copy of a 96k-track production database: byte-for-byte
identical response, peak RSS 1593MB -> 53MB, time-to-first-byte 8.1s -> 0.2s.
* fix(jellyfin): fail loudly on /Items streaming errors instead of a truncated 200
Addresses code review: a streamed /Items response commits HTTP 200 before an
error can occur, so failures were surfacing as misleadingly successful bodies.
- A cursor-open failure (e.g. a busy DB under connection pressure) is now
surfaced before the first byte is written: the cursor open is deferred into
the item source and run in writeItems after the ServerId lookup but before the
envelope, returning a clean 500 the client can retry — not a 200 with an empty
item list.
- A mid-stream (row-scan) error now aborts without closing the JSON envelope,
leaving the body malformed. A truncated-but-valid response would let a sync
client (Finamp) treat the short list as the whole library and prune local
tracks; malformed JSON forces the client's parser to fail and retry.
* refactor(jellyfin): stream every collection endpoint from a DB cursor
Streaming was applied only to the song listing, which left two write paths, two
ServerId stamping mechanisms and a listXxx return-type split ("songs is
special") that made the code hard to follow.
Add GetCursor to the album, artist, genre and playlist repositories, mirroring
MediaFileRepository.GetCursor: same select builder as GetAll, so each cursor
yields identical, fully-hydrated rows (hydration happens per-row in PostScan,
and toModels is only a deref loop). The playlist cursor keeps GetAll's
owner/public visibility filter. Each repository test asserts the cursor yields
exactly what GetAll returns, including Max/Offset.
With cursors everywhere, every listXxx returns itemsResult and every collection
streams through one writer:
- listAlbums, listArtists and listPlaylists now stream from their cursors;
search paths stay materialized (Search returns a slice).
- listGenres stays materialized: its total is the length of the full list and it
paginates in memory, so there is nothing for a cursor to page over.
- api.ok's QueryResult case delegates to writeItems, so the ~9 inline callers
funnel into the same writer; streamQueryResult and wrapResult are gone.
- /Items/Latest streams as a bare JSON array (its Jellyfin wire shape) via
writeItemsArray, which shares the item loop and stamping. That also closes the
same unbounded hole /Items had: limit=0 disabled its LIMIT.
- ServerId is now stamped in exactly one place, so api.ok only handles single,
non-collection payloads.
Verified against a copy of a 96k-track production database: every endpoint
byte-for-byte identical to master. Unbounded /Items peak RSS 1691MB -> 54MB;
time-to-first-byte 6.3s -> 0.19s, /Artists 1.14s -> 0.13s.
* refactor(jellyfin): route every response through api.ok
Handlers were split between api.ok and api.writeItems with no clear rule for
which to call. api.ok now accepts itemsResult too, so it is the single entry
point: callers hand it whatever they have and it routes collections (cursor
-backed or materialized) to the streaming writer. writeItems is reached only
through api.ok now. The one exception is /Items/Latest, which returns a bare
JSON array rather than a QueryResult envelope and so writes directly — noted in
both doc comments.
Also drop GenreRepository.GetCursor: listGenres derives its total from the
length of the full list and paginates in memory, so there is nothing for a
cursor to page over, leaving the method unused.
* fix(jellyfin): stream the unbounded multi-type /Items merge
The multi-type merge capped each per-type query at offset+limit only when a
Limit was given. Without one (Finamp's favorites screen sends multi-type), every
type ran an unbounded query and collect() drained each cursor into a slice — so
/Items?IncludeItemTypes=MusicAlbum,Audio with no Limit materialized every album
and every song, the same OOM class this branch set out to fix. The comment
claiming the set was "capped at offset+limit per type" was wrong for limit=0.
Without a limit the merged page is just each type's rows in order minus the
first offset, which is exactly what chaining the per-type cursors yields, so
stream that instead of merging in memory. The bounded path is unchanged: with a
Limit each type holds at most offset+limit rows, so merging and paginating
across the combined list is safe. Cursors are opened one at a time (each pins a
DB connection until drained), with the first opened eagerly so the usual failure
is still a clean error before any byte is written.
Measured on a 96k-track library, /Items?IncludeItemTypes=MusicAlbum,Audio with
no Limit (103,393 items): peak RSS 612MB -> 53MB, time-to-first-byte 4.9s ->
0.6s, response byte-for-byte identical.
* refactor(jellyfin): parse /Items params into a struct
The /Items dispatcher was a single 90-line block that parsed a dozen params and
then threaded them positionally through three layers — queryItemsOfType took 11
arguments, listSongs and listAlbums 9 each — so reading any one of them meant
decoding a long argument list.
Parse once into an itemsQuery and pass that instead. queryItems now reads as the
four things it actually does: parse, the id/playlists-folder/playlist-parent
special cases, single-type dispatch, multi-type merge — with the playlist-parent
and merge bodies moved to playlistTracks and mergeTypes. Every listXxx takes
(ctx, opts, q).
No behavior change: parsing, ordering and the entityParent rule are unchanged,
and /Artists still passes favOnly=false explicitly by building the subset of the
query it uses.
* docs(jellyfin): trim comments on the streaming path
Cut the comments back to the non-obvious why: the deferred cursor open (the
ServerId write would deadlock against an open reader), the deliberate malformed
JSON on a mid-stream error, why chained opens one cursor at a time, why
listGenres stays materialized, and why the cursor helpers take the underlying
func type. Dropped the rest — restatements of the code, doc comments that
repeated a test's own name, and the GetCursor interface comments that the
existing MediaFileRepository.GetCursor does without.
* refactor(persistence): fold the cursor wrappers into a generic wrapCursor
wrapAlbumCursor, wrapArtistCursor, wrapMediaFileCursor and wrapFolderCursor were
the same twelve lines four times over, differing only in the type name, and the
playlist cursor had its own inline copy of the loop.
Add one generic wrapCursor. It takes an extractor func rather than a method on
an interface: a type parameter can't reach an embedded field, and methods that
only satisfy a generic constraint are reported by the unused linter. The
extractor also lets both type parameters be inferred, so call sites need no
explicit instantiation. Each entity keeps its named wrapper as a one-liner,
since the model cursor types are defined types and need the conversion — and the
existing wrapper tests call them directly.
The nil-row error now names the model type via %T ("unexpected nil model.Album")
instead of a hand-written per-entity string; the three tests asserting that
message are updated.
* fix(jellyfin): bound concurrent collection streams
A streamed collection holds a DB cursor — and its pooled connection — for the whole client-paced
response, where the old materialize-then-write path released it as soon as the query finished. The
pool is shared with the scanner, Subsonic, the native API and the UI, so enough slow clients take
every connection and everything else blocks waiting for one. Measured against a copy of a 96k-track
production DB, 20 concurrent unbounded streams against a pool of 16 stalled a write for 16.2s (the
other 14 writes in the run took ~2ms — the signature of connection starvation, not lock contention).
Cap concurrent streams at half the pool. Excess requests queue rather than fail, so no client is
rejected: the same 20 streams now all complete and the worst write is 2.5ms.
- conf.MaxOpenConns() now owns the pool sizing (db calls it). It belongs in conf: the pool is a
tunable, db already imports conf, and putting it in db would force server/jellyfin to import db
just to size the cap against it. Expressing the cap as MaxOpenConns()/2 also keeps the two from
drifting apart.
- Uses chi's ThrottleBacklog, not server.ThrottleBacklog: the latter buffers the whole response to
release its token early, which is right for artwork but would undo the streaming. chi's panics on a
non-positive limit, so throttleStreams guards it — setting MaxConcurrentStreams=0 disables the cap
instead of crashing the server at startup.
* refactor(jellyfin): move throttleStreams to middlewares.go
It's a middleware, so it belongs beside normalizeQueryKeys, authenticate and
withPlayer rather than in api.go. Its tests move to middlewares_test.go with it,
keeping one test file per production file.
* fix(jellyfin): abandon the scan when the client stops reading
encodeItems discarded its write errors and relied on the final Flush to report
them, so once bufio's buffer filled and the flush failed, the loop still pulled
every remaining row through the cursor and serialized it for a client that was
gone. A test with a failing writer confirms it: all 20k items were drained.
That wastes CPU, and holds the cursor's pooled DB connection and a stream slot
(now a capped resource) for the length of a full scan nobody is reading. Check
the per-item writes so the first failure ends the scan; the fixed envelope
writes stay unchecked, since bufio latches for them anyway.
|
||
|
Deluan Quintão
|
fe6ac2e577
|
feat(jellyfin): experimental Jellyfin Music API support (#5730)
* feat(sharing): enable sharing by default
Flip the EnableSharing default from false to true so new installations have the sharing feature available out of the box. Users can still disable it via the EnableSharing config option.
The native API only registers the /share route when sharing is enabled, so the nativeapi tests that build the router without wiring a share service now explicitly disable sharing in their setup to avoid registering a route backed by a nil service.
* feat(jellyfin): add config flag and URL path constant
Adds the disabled-by-default Server.Jellyfin config option (Enabled,
ServerName) and consts.URLPathJellyfinAPI, following the existing
LastFM/ListenBrainz patterns. Later tasks will use these to mount the
Jellyfin-compatible API router.
* fix(jellyfin): default Jellyfin ServerName to "Navidrome"
* feat(jellyfin): package skeleton with System handshake endpoints
Adds server/jellyfin: the Router (mirrors server/subsonic and
server/public), its Wire-friendly New(...) constructor, a chi routes()
table, and the ok() JSON response helper. Implements the unauthenticated
handshake surface Jellyfin clients probe first: GET /System/Info/Public,
GET+POST /System/Ping, and GET /QuickConnect/Enabled (quick connect is
unsupported, so it always reports disabled).
The public info payload's Id must be stable across restarts (Jellyfin
clients cache ServerId), so it reuses the get-or-create Property pattern
already used for InsightsID in core/metrics/insights.go: fetch
consts.JellyfinServerIDKey from the Property repository, generating and
persisting a new UUID on first read. A dedicated key (rather than the
existing InsightsID) keeps the anonymous telemetry identifier from being
exposed on this unauthenticated endpoint.
* test(jellyfin): cover ping and quickConnectEnabled handlers
Both were added alongside the System/Info/Public handshake endpoint but
had no direct test coverage.
* fix(jellyfin): memoize stable server Id and cover get-or-create path
* feat(jellyfin): wire and mount the router behind Jellyfin.Enabled
Add jellyfin.New to the shared Wire provider set and a
CreateJellyfinAPIRouter injector, then mount the router at /jellyfin
in startServer(), gated by conf.Server.Jellyfin.Enabled, mirroring the
existing LastFM/ListenBrainz router mounts.
* feat(jellyfin): add Jellyfin DTOs and model mappers
Adds BaseItemDto, QueryResult, UserItemDataDto, UserDto,
AuthenticationResult, MediaSourceInfo, PlaybackInfoResponse,
NameGuidPair and SessionInfo DTOs to server/jellyfin/dto, plus
model-to-DTO mappers for songs, albums, artists and genres that
later browse/stream/write endpoints will consume.
* test(jellyfin): cover GenreToBaseItem mapper
* feat(jellyfin): advertise Jellyfin-compatible version, brand ServerName with Navidrome version
* fix(jellyfin): map LastPlayedDate and omit zero track/disc numbers
* feat(jellyfin): authentication middleware and AuthenticateByName login
* fix(jellyfin): reject empty login password, wire ServerName, add negative auth tests
* refactor(jellyfin): use new(x) builtin instead of intPtr helper
* feat(jellyfin): user views and current-user endpoints
* feat(jellyfin): Items query engine, item detail, and latest
Adds the /Items universal query endpoint, dispatching by IncludeItemTypes
over albums/artists/songs/genres with ParentId, SearchTerm, Filters=IsFavorite,
SortBy/SortOrder and StartIndex/Limit support, plus GET /Items/{itemId} and
/Users/{userId}/Items/Latest. Reuses server/subsonic/filter builders (by
artist/album/starred) instead of hand-rolled squirrel filters, and resolves
sort keys per item type since each repository maps sort names to different
real/aliased columns.
Also adds the missing CountAll to tests.MockArtistRepo, exposed by this task
(the mock embedded a nil ArtistRepository for it and would panic on use).
* refactor(server): extract shared query filter builders to server/filter
Moves server/subsonic/filter to server/filter so server/jellyfin can use
the shared query-option builders without importing server/subsonic,
enforcing the rule that no API package imports another API's package.
* feat(jellyfin): full multi-library support with per-user access scoping
Replace the single hardcoded "music" UserView with one CollectionFolder
view per library the user can access, and scope every /Items browse
query (albums, songs, artists, /Latest) to those libraries via
server/filter's ApplyLibraryFilter/ApplyArtistLibraryFilter.
ParentId is now disambiguated: a numeric value the user has access to is
treated as a library scope (browsing a UserView), otherwise it falls
through as an entity id (artist/album), which safely matches nothing
rather than leaking another library's content. getItem now 404s when
fetching an album or song outside the user's accessible libraries;
artists are skipped (they can span multiple libraries) with a TODO.
Genres remain unscoped since they're global tags, not per-library
entities.
* test(jellyfin): cover admin library-access path and drop nil test contexts
* feat(jellyfin): Artists and Genres endpoints
Add /Artists, /Artists/AlbumArtists, /Genres and /MusicGenres. Artists
listing is library-scoped (defaulting to the user's accessible
libraries, narrowed by an accessible ParentId), delegating access
control to listArtists/ApplyArtistLibraryFilter. Genres are global and
unscoped, matching the ML decision already made for listGenres.
* refactor(jellyfin): share resolveLibraryScope between items and artists
* feat(jellyfin): item image endpoint via artwork service
* fix(jellyfin): let net/http sniff image Content-Type instead of forcing jpeg
* feat(jellyfin): audio streaming and PlaybackInfo with library access control
* feat(jellyfin): favorites and rating write-back with library access control
Adds POST/DELETE handlers for /Users/{userId}/FavoriteItems/{itemId} and
/Users/{userId}/Items/{itemId}/Rating. A shared resolveAnnotated helper
probes album/artist/media file (mirroring getItem's order) and 404s before
writing if the user lacks access to the album/song's library; artists are
exempt since they span multiple libraries. Ratings are halved coming in
(Jellyfin 0-10 -> Navidrome 0-5) to match the doubling in dto.UserData.
Also teaches MockMediaFileRepo and MockArtistRepo's SetStar/SetRating (and
MockAlbumRepo's, previously a no-op) to actually mutate the backing data so
write-back can be asserted in tests.
* feat(jellyfin): playback reporting and scrobbling
Add /Sessions/Playing[/Progress|/Stopped] and /Sessions/Capabilities[/Full]
handlers, backed by core/scrobbler.PlayTracker. A new withPlayer middleware
resolves/registers a model.Player from the Emby DeviceId header (used
directly as the stable player id, unlike Subsonic's cookie fallback) and
injects it into the request context for scrobbling.
The Stopped report calls ReportPlayback with IgnoreScrobble to end the
now-playing session without double-counting, then calls Submit as the
single source of the play-count increment and external scrobble.
* feat(jellyfin): playlist read and write-back
Adds POST /Playlists, GET/POST/DELETE /Playlists/{id}/Items. Tags each
playlist item with PlaylistItemId (the entry's position within the
playlist) so DELETE .../Items?EntryIds=... can remove a specific
occurrence by the id core/playlists.RemoveTracks actually expects,
rather than the song id used everywhere else.
* test(jellyfin): cover empty Ids/EntryIds in playlist add/remove
* feat(jellyfin): unknown-route logging, generic 500s, rating clamp, docs
Hardening pass ahead of real client testing: unmatched routes and
unsupported methods now return a logged, JSON 404 instead of chi's
default plain-text response, so a missing endpoint a client needs is
easy to spot in the logs. Internal errors (ffmpeg output, file paths,
etc.) no longer leak into 500 response bodies -- a shared
internalError helper logs the real error server-side and always
returns a generic message. Inbound Jellyfin ratings are clamped to
0-10 before being halved into Navidrome's 0-5 scale, and /System/Ping
now replies with a bare plain-text body as real Jellyfin servers do.
Adds a README with an enable/curl walkthrough and known limitations.
* docs(jellyfin): clarify public image endpoint and accessibleLibraryIDs comments
* fix(jellyfin): case-insensitive path routing for Jellyfin client compatibility
* refactor(server): extract case-insensitive path routing to a shared helper
* fix(jellyfin): return User Policy and Configuration so Finamp completes login
* fix(jellyfin): support Playlist type, multi-type Items queries, and PlayCount/DatePlayed sort
Real Finamp requests break against three /Items query engine bugs: Playlist
requests fell through to albums, multi-type IncludeItemTypes (e.g. Finamp's
favorites screen) only returned the first requested type, and comma-separated
SortBy lists (e.g. "DateCreated,SortName") were matched as one opaque string
so they always fell through to the repo default.
Also extends tests/mock_playlist_repo.go with SetData/GetAll so playlist
listing is testable like the other mock repos.
* feat(jellyfin): implement /socket WebSocket for real-time client sessions
* fix(jellyfin): resolve library-view ids in getItem so clients can load the library
* fix(jellyfin): serve direct file at /Items/{id}/File and accept ApiKey query param
* fix(jellyfin): resolve playlist ids in getItem
* fix(jellyfin): read lowercase ids/entryIds playlist params and add playlist-users endpoints
* fix(jellyfin): include MediaSources with Size/Bitrate on tracks so clients show download size
* fix(jellyfin): populate all required MediaSourceInfo bool/array fields to match Jellyfin
* fix(jellyfin): hex-encode item ids at the API boundary for Jellyfin client compatibility
Finamp (and presumably other clients) parses ids as radix-16, but Navidrome's
base62 nanoids aren't valid hex and crash its queue packing. Hex-encode every
id emitted by the Jellyfin API and decode every id received, keeping the
transform reversible and stateless at the boundary rather than touching any
model id.
* fix(jellyfin): populate MediaStreams with the audio stream so clients can size/transcode
* fix(jellyfin): support Ids batch-fetch in /Items so clients can fetch items by id
* fix(jellyfin): do not disable Jellyfin server when disabling external services
* fix(jellyfin): emit per-image ImageBlurHashes so clients de-dupe images and stop warning
* fix(jellyfin): support playlist cover upload/delete and display via item image endpoints
* feat(jellyfin): implement GET /Playlists/{id} for playlist visibility
Finamp's playlist edit screen calls GET /Playlists/{id} to read the
OpenAccess (public visibility) flag; we only had the sub-routes, so it
404'd and the edit screen failed to load. Return the Jellyfin PlaylistDto
shape (OpenAccess from Public, empty Shares, media item ids).
* fix(jellyfin): display playlist cover art
Uploaded playlist covers never showed in clients for two reasons: the
playlist BaseItemDto advertised no Primary ImageTag (so clients didn't
know to fetch a cover), and the public image endpoint resolved artwork
under the request's anonymous context, so a private playlist failed its
visibility filter and fell back to the placeholder. Advertise the Primary
image tag/blurhash on playlists, and resolve artwork under an elevated
context (as core/artwork's cache warmer does).
* fix(jellyfin): expand album/artist/playlist ids when building playlists
Jellyfin clients (Finamp) send container ids — an album, artist or
playlist — in a playlist's Ids list and expect the server to expand each
into its child tracks. core/playlists only understands media file ids, so
creating or adding with an album id silently produced an empty playlist.
Expand container ids to their tracks (in order) before create/add; bare
song ids still pass through. Adds filter.SongsByArtistID.
* fix(jellyfin): support playlist deletion via DELETE /Items/{id}
Finamp deletes a playlist with DELETE /Items/{id}, which we didn't route,
so deletion silently failed. Implement it via core/playlists.Delete (which
enforces ownership and removes the cover file). Only playlists are
deletable through this API; non-playlist ids return 404, non-owners 403.
* test(jellyfin): add e2e suite harness + smoke tests
* test(jellyfin): e2e for system, auth, routing, browsing, annotations
* test(jellyfin): e2e for playlists (CRUD, expansion, cover) and item images
* test(jellyfin): e2e for streaming, sessions, and multi-user access control
* fix(jellyfin): artist search 500 (library filter leaked into FTS query)
getArtists/listArtists reused the browse-path filters (notMissing +
ApplyArtistLibraryFilter) for the search path, but artist Search expects a
sole Eq{library_id} filter it can consume as a scope — artists have no
library_id column, so the compound/join filter leaked into the FTS query
and 500'd. Every Finamp artist search failed (the filter is applied even
unscoped, since admins resolve to all library ids). Build search filters
separately. Adds e2e search coverage.
* feat(jellyfin): implement POST /Playlists/{id} to update name, visibility, tracks
Finamp edits a playlist (make public, rename, reorder) via POST
/Playlists/{id}, which we didn't route, so every edit 404'd. Implement it:
Ids present -> replace track list (Create with existing id, preserving
name); otherwise update Name/IsPublic via core/playlists.Update. Adds a
shared playlistError helper (403/404/500) reused by deleteItem, plus e2e
coverage.
* docs(jellyfin): update README for playlists, images, id encoding, and e2e
* fix(jellyfin): default album track listing to track order
Browsing an album's tracks (Items?ParentId=<albumId>&IncludeItemTypes=Audio)
took only SongsByAlbum's filters and dropped its Sort, so tracks came back
in arbitrary order. Default opts.Sort to the album (disc+track) order when
browsing an album without an explicit SortBy — matching Subsonic's GetAlbum
and real Jellyfin. An explicit SortBy still wins.
* fix(jellyfin): filter items by AlbumArtistIds/ArtistIds
An artist's page in Finamp sends ParentId=<libraryId> (scoping) plus
AlbumArtistIds/ArtistIds/contributingArtistIds for the artist itself, but
queryItems only honored ParentId, so an artist's albums and tracks came
back unfiltered (every artist's content). Parse the artist-id params and
apply AlbumsByArtistID (albums) / SongsByArtistID (tracks). Adds e2e
coverage.
* fix(jellyfin): only count a play past the scrobble threshold
reportPlaybackStopped set IgnoreScrobble and force-submitted a play on
every Stopped report, so a briefly-played track (e.g. an immediate skip)
was marked played. Finamp sends Stopped on every track switch, so the
threshold must be applied server-side. Let ReportPlayback's StateStopped
logic decide (play + scrobble only past 50% of the track / 4-minute cap)
instead. Subsonic differs because there the client gates submission.
* fix(jellyfin): sort album tracks by track number when client sends IndexNumber SortBy
Finamp's album view requests SortBy=ParentIndexNumber,IndexNumber,SortName
(disc, track, name), but applySort didn't recognize ParentIndexNumber or
IndexNumber and fell through to SortName, sorting tracks alphabetically by
title. Map both keys to the album (disc+track) sort. The reversed-title
fixture lets the e2e tell track order from title order.
* fix(jellyfin): honor the isFavorite query param for favorites filtering
Finamp's artist 'Favourite tracks' widget requests favorites via the
standalone isFavorite=true query param, not Filters=IsFavorite, so the
filter was ignored and non-favorited tracks were returned. Detect both
forms. (The widget's reshuffling is Finamp's explicit SortBy=Random.)
* feat(jellyfin): emit DateCreated (Date Added) on items
BaseItemDto had no DateCreated, so clients showed 'No Date Added' and had
nothing to sort 'Recently Added' by. Emit it as ISO 8601 from each entity's
CreatedAt (the same field the recently_added sort uses) for songs, albums
and artists.
* fix(jellyfin): set ArtistItems/AlbumArtists on songs (Now Playing artist)
SongToBaseItem only set Artists (names) and AlbumArtist, not the structured
ArtistItems/AlbumArtists. Finamp's Now Playing screen reads ArtistItems and
shows 'Unknown Artist' when it's absent. Populate both (track artist and
album artist) as name+id pairs, mirroring AlbumToBaseItem.
* docs(jellyfin): note synthetic blurhash as a follow-up
Document that ImageBlurHashes are derived from the item id (a solid-color
placeholder), not computed from the cover art like real Jellyfin, and
outline what a proper implementation would take.
* docs(jellyfin): note WebSocket events and favourited playlists as follow-ups
* fix(jellyfin): filter the Artists page by role (album artist vs performer)
/Artists and /Artists/AlbumArtists both called the same role-agnostic
handler, so Navidrome's per-role artist entries (composers, arrangers,
performers) all showed as Album Artists, and the two tabs were identical.
Filter /Artists/AlbumArtists to RoleAlbumArtist and /Artists to RoleArtist
(and the MusicArtist browse to album artists), via filter.ArtistsByRole.
Verified live: Beatles Singles' album-artists dropped 138->5, composers
excluded, performers (Billy Preston) show only under /Artists.
* fix(jellyfin): sort playlists by name (missing Playlist sort mapping)
applySort had no sortColumnsByType entry for the Playlist type, so
SortBy=SortName was ignored and the Playlists screen showed them in the
repo's default order. Map SortName/Name -> name (as Subsonic does) and
DateCreated -> created_at. Verified live: case-insensitive alphabetical.
* fix(jellyfin): read query params case-insensitively (Jellify support)
Jellify (and the official Jellyfin TypeScript SDK) send query params in
camelCase (parentId, albumArtistIds, artistIds, includeItemTypes), where
Finamp sends PascalCase. Our handlers read fixed-case keys, so every
Jellify filter/sort/paging param was silently dropped:
- an artist's page listed albums and tracks from all artists
- opening an album listed every album instead of its tracks
Real Jellyfin binds query params case-insensitively (ASP.NET model
binding), so add a normalizeQueryKeys middleware that folds every query
key to lowercase once, and read params by their lowercase name. This
also removes the scattered dual-case hacks (Ids/ids, IsFavorite,
ContributingArtistIds, api_key/ApiKey, queryParam) that let this bug
class through.
Also infer the child type from an album parent: Jellify browses an album
with only parentId (no IncludeItemTypes), and Jellyfin infers Audio from
the parent; without it we fell back to listing all albums.
Verified live against Finamp+Jellify and with 4 new e2e specs replaying
Jellify's exact request shapes.
* fix(jellyfin): advertise LocalAddress in the public system info handshake
Jellify (and other @jellyfin/sdk clients) that connect by raw address fall
back to HTTP when TLS isn't available, then adopt the handshake's
LocalAddress as their server base URL. We never populated it, so Jellify's
SDK `api` object was undefined and sign-in crashed with "Cannot read
property 'configuration' of undefined" (getUserApi(api!) with an undefined
api). Over an HTTPS connection Jellify uses connectionType=hostname and
never reads LocalAddress, which is why it worked while an HTTPS proxy was
in front.
Populate LocalAddress from the request — scheme + host (honoring
X-Forwarded-*), plus the /jellyfin mount path — matching real Jellyfin,
which always sends it. Export server.ServerAddress for the resolution.
* fix(jellyfin): separate "Featured On" from an artist's own discography
Jellify's artist page fetches the discography via albumArtistIds and the
"Featured On" section via contributingArtistIds, relying on the server to
return disjoint sets. We collapsed albumArtistIds/artistIds/
contributingArtistIds into one AlbumsByArtistID filter, so an artist's own
albums appeared in both sections.
Add AlbumsByContributingArtistID — albums where the artist is a track
artist but NOT the album artist — matching Jellyfin's ContributingArtistIds
(in Artists, not in AlbumArtists), and route contributingArtistIds to it.
* fix(jellyfin): accept a bare Authorization token for audio streaming
Jellify's native player (react-native-nitro-player) authenticates the audio
stream by setting a bare Authorization header carrying the raw access token
({ AUTHORIZATION: api.accessToken }), not the "MediaBrowser ... Token=" scheme
parseEmbyAuth understands. tokenFromRequest didn't recognize it, so every
/Audio/{id}/stream request from the native player 401'd and no audio played
(the JS client still optimistically posted playback progress, masking it).
Accept a bare (or "Bearer <token>") Authorization header, as real Jellyfin
does. Verified live: the native player's exact request now streams 200.
* fix(jellyfin): embed a self-authenticating stream URL in PlaybackInfo
Jellify's native audio player (react-native-nitro-player / ExoPlayer) fetches
the stream without forwarding any auth — captured request headers were only
Connection/Icy-Metadata/Accept-Encoding/User-Agent, no Authorization and no
api_key — so every /Audio/{id}/stream request 401'd and nothing played (the
JS client still optimistically posted progress, masking it).
Real Jellyfin returns stream URLs with the token embedded; we returned none,
so Jellify fell back to a token-less DirectPlay URL. Populate
MediaSources[0].TranscodingUrl with /Audio/{id}/universal?api_key=<caller
token>, which Jellify's player uses verbatim. Direct-play clients (Finamp
builds its own /Items/{id}/File?ApiKey URL) ignore the field, so they're
unaffected.
* feat(jellyfin): serve per-item UserData (GET /UserItems/{id}/UserData)
Jellify fetches this per item to render played/favourite indicators; we 404'd
it. Resolve the item via the existing getItem resolver (which loads the
caller's annotations and enforces the same library-access gate) and return its
UserData, falling back to an empty-but-valid object for items without
annotations (e.g. playlists). Also registers the legacy
/Users/{userId}/Items/{itemId}/UserData spelling.
* refactor(jellyfin): drop unused bare-Authorization token support
Added mid-troubleshooting on the theory that Jellify's native player sends a
bare Authorization header, but the debug capture showed it sends no auth header
at all — the real fix was embedding api_key in PlaybackInfo's TranscodingUrl.
No client reaches this path (Jellify JS uses the MediaBrowser scheme, Finamp
uses X-Emby-Token, the native player uses the api_key URL), so remove it and
its tests per YAGNI. Effectively reverts 4f8ac1e0.
* feat(jellyfin): implement Similar endpoints via the external provider
Add GET /Artists/{id}/Similar (related artists) and GET /Items/{id}/Similar
(similar songs for a track, similar albums for an album, related artists for an
artist), sourced from the same external.Provider (Last.fm etc.) that powers
Subsonic's getArtistInfo2/getSimilarSongs. Only library-present artists are
returned so each is navigable; provider errors and unknown ids degrade to an
empty 200 result, so clients (Jellify) stop hammering these with 404 retries.
Injects external.Provider into the Router (regenerated via make wire).
* perf(jellyfin): make Similar endpoints non-blocking (bounded quick wait)
Each /Similar request fetched from Last.fm synchronously (500ms-1.4s), and
Jellify requests Similar for many items at once on the home/artist screens, so
the whole screen stalled — a home pull-to-refresh dragged for seconds.
Run the external lookup on a background context and return within a 500ms quick
wait: a cached artist resolves instantly, a cold one returns empty now while the
lookup finishes caching in the background, so a later load is fast and
populated. Verified live: cold calls bounded at ~500ms (was up to 1.4s), warm
calls 3-8ms.
* fix(jellyfin): answer ManualPlaylistsFolder so the home stops stalling
Jellify resolves its "playlists library" via IncludeItemTypes=
ManualPlaylistsFolder, then lists playlists with ParentId set to that folder's
id. We didn't recognize the type, so parseTypes fell back to MusicAlbum and
returned the album list. Jellify's query then found no item with
CollectionType=playlists and resolved undefined — which React Query rejects,
retrying it in a backoff loop that stalled the home pull-to-refresh for ~5s
(every response was fast server-side; the delay was the client's retries).
Return a synthetic "playlists" folder (CollectionType=playlists) for the
ManualPlaylistsFolder query, resolve ParentId=<that folder> to the user's
playlists, and give playlists a Path under "data" (Jellify drops playlists
whose Path lacks it). Adds a Path field to BaseItemDto.
* docs(jellyfin): note lyrics and InstantMix/sonic-similarity as follow-ups
* fix(jellyfin): genre paging params and same-key casing collisions
getGenres read StartIndex/Limit in PascalCase, which normalizeQueryKeys had
already folded to lowercase, so genre paging was silently ignored; totals now
come from the full (small) genre list instead of the page length.
normalizeQueryKeys now merges values when two casings of a key collide,
instead of nondeterministically keeping one.
* fix(jellyfin): round ratings to the nearest star instead of truncating
Rating is a nullable double 0-10 in Jellyfin's contract. Truncating integer
division stored 9 as 4 stars, and both Rating=1 and fractional values (which
failed integer parsing) became 0 — silently deleting the rating. Parse as
float, round, and floor nonzero input at one star.
* fix(jellyfin): apply Name/IsPublic sent together with a track replacement
Jellyfin's UpdatePlaylist applies every provided field, but the Ids branch
returned early, silently discarding a rename or visibility change sent in the
same body (core/playlists.Create with an existing id ignores the name).
* fix(jellyfin): don't rotate the stored server id on transient DB errors
serverID treated any Property.Get error as "no id yet" and persisted a fresh
UUID over JellyfinServerID, with sync.Once pinning it for the process lifetime
— a busy DB or canceled request context on the first request would break every
client's cached ServerId. Only ErrNotFound mints a new id now, and failures
yield an uncached temporary value so the next request retries.
* fix(jellyfin): report real search totals instead of page length or unfiltered counts
Artist search returned TotalRecordCount = len(page), so clients stopped after
the first page; album/song search counted via CountAll, which can't see the
search term, so clients paged through phantom results. The repos' Search API
has no match count, so fetch one row beyond the page: offset+len is exact on
the last page and a strictly growing lower bound before it — paging clients
terminate exactly at the last match.
* fix(jellyfin): browse playlists via the generic /Items path and resolve the playlists folder by id
A typeless /Items?ParentId=<playlistId> (legal in real Jellyfin, used by
generic clients) fell through to the MusicAlbum default and returned an empty
list; it now returns the playlist's tracks, paginated, with visibility
enforced by GetWithTracks. resolveItemByID also answers the synthetic
playlists-folder id the server itself advertises instead of 404ing it.
* perf(jellyfin): cap per-type queries in multi-type /Items requests
The multi-type merge path queried each type with a zero-value QueryOptions —
no LIMIT in SQL — materializing every matching row (each with embedded
MediaSources) just to slice out one page in memory. Each type now fetches at
most StartIndex+Limit rows, the worst case one type can contribute to the
merged window; totals still come from CountAll.
* fix(jellyfin): dedupe and bound the background Similar fetches
awaitSimilar spawned a detached, deadline-free goroutine per request; clients
re-polling after the empty quick-wait response piled up duplicate provider
chains (no singleflight anywhere below) racing writes on the same artist row.
Identical in-flight requests now share one fetch — keyed per user, since the
mapped items embed the user's annotations — and the background context gets a
one-minute deadline so a hung provider can't hold goroutines forever.
* fix(jellyfin): gate private playlist covers on the public image route
The unauthenticated image endpoint elevated every request to an admin context,
so anyone who knew or guessed a playlist id could fetch another user's private
uploaded cover. Library artwork still resolves elevated (Jellyfin clients fetch
images without auth headers), but playlist covers are now served only when the
playlist is public or the request's optional token identifies its owner or an
admin; everyone else gets the placeholder.
* fix(log): redact full api_key values, including JWTs
The api_key pattern matched only word characters, stopping at a JWT's first
'.' — the Jellyfin API embeds the session JWT as api_key in TranscodingUrl, so
request logs kept its payload and signature, enough to reconstruct a replayable
token by prepending the constant header. Match to the next query separator
instead, like the sibling s=/p=/jwt= patterns.
* fix(jellyfin): record LastLoginAt on Jellyfin logins
authenticateByName re-implements credential validation and skipped the
UpdateLastLoginAt call the web UI's validateLogin makes, so users who only log
in via Jellyfin clients showed a never/stale Last Login in the admin UI.
* perf(jellyfin): batch song resolution in /Items?ids= and playlist expansion
Both paths probed up to four repositories per client-supplied id. Songs — the
common case — now resolve via chunked media_file.id IN queries (same pattern
as playqueue's loadTracks); only the residue pays the container probes.
* fix(jellyfin): wait for the real Similar result instead of answering a cacheable empty list
The 500ms quick wait returned an empty 200 for any cold lookup —
indistinguishable from "no similar items exist", so clients cached the wrong
answer until an app restart. With fetches deduplicated, wait up to the agents'
HTTP timeout for the actual result; only a hung provider now yields the empty
fallback, and its fetch still warms the cache in the background. Also makes
the dedup test deterministic (the old one raced its release channel).
* refactor(tests): extract the shared e2e harness into tests/harness
The Subsonic and Jellyfin e2e suites each carried their own copy of the
golden-DB lifecycle (boot, seed users/library, scan, WAL snapshot), the
ATTACH-DATABASE restore, fixture-FS registration, and the SpyStreamer /
NoopFFmpeg doubles. Those now live in one importable package (same pattern as
core/storage/storagetest); fixture libraries and request helpers stay
per-suite since they encode each API's test expectations.
* docs(jellyfin): make comments concise
Compress the narrative comments accumulated during live client testing into
short why-only notes; keep the client quirks, security rationales and gotchas,
drop the exposition. Comments-only change (net -141 lines).
* fix(tests): silence gosec taint false-positive in harness snapshot write
The write moved from a _test.go file (which gosec skips) into the importable
harness package; the path derives from GinkgoT().TempDir().
* fix(jellyfin): route the current /UserFavoriteItems favorite endpoint
@jellyfin/sdk 0.13.0 (used by Jellify) posts favorites to
POST/DELETE /UserFavoriteItems/{itemId}, while we only routed the legacy
/Users/{userId}/FavoriteItems/{itemId} (Finamp). Jellify favorites 404'd
("Failed to add favourite"). Route both spellings to the same handlers.
* feat(jellyfin): honor Fields on /Items and add missing conformance fields
Match real Jellyfin's response shape: gate MediaSources (and MediaStreams)
behind Fields=MediaSources instead of always embedding them — clients that
need Size request it, as Finamp does — which cuts a 46-track artist response
from ~87KB to a fraction. Also emit the always-present fields Jellyfin sets:
ServerId (stamped centrally in ok), LocationType, HasLyrics, and SortName
(when Fields=SortName). PlaybackInfo still carries MediaSources (its purpose).
* fix(jellyfin): address code review security and correctness findings
Applies the reviewed findings from PR #5730:
- Gate similar songs/albums on the caller's library access, so the
external provider can't surface metadata from libraries the user
cannot see. Also clamp the client-supplied limit before it sizes any
allocation or provider fetch (CodeQL user-controlled allocation).
- Prepend the /jellyfin mount prefix to the PlaybackInfo TranscodingUrl,
so a client resolving it as an absolute host path still reaches the
mounted router.
- Recognize WebP and GIF magic numbers on raw cover uploads, matching
the formats Navidrome already supports.
- Bound the playlist cover upload: honor EnableArtworkUpload for
non-admins and cap the body with MaxBytesReader/MaxImageUploadSize,
mirroring the native image endpoint.
- Propagate non-not-found repository errors from resolveAnnotated as 500
instead of silently returning 404.
- Normalize the literal prefix of mixed literal.param path segments
(e.g. STREAM.mp3) so case-insensitive routing reaches stream.{container}.
- Rate-limit POST /Users/AuthenticateByName with the same per-IP limiter
as /auth/login when AuthRequestLimit is set.
- Guard against a nil user in authenticateByName.
* fix(jellyfin): correct playlist track browsing and multi-id edits
Fixes three playlist issues, two found testing against Jellify and one
from the PR review:
- Resolve a playlist ParentId to its tracks even when the client sends
IncludeItemTypes=Audio. Jellify opens a playlist with
ParentId=<playlist>&IncludeItemTypes=Audio; the id was routed through
listSongs as an album id, returning an empty list.
- Read repeated id query params (ids=X&ids=Y), not just the first value.
Jellify's @jellyfin/sdk serializes id arrays as repeated params, so
adding an album (which it expands client-side into many ids) only
added the first track. Applies to both add and remove; the
comma-separated form other clients use still works.
- Let an explicit empty Ids array clear a playlist. Ids is now a pointer
so an omitted field still means 'leave unchanged', while an empty list
clears the tracks (via RemoveTracks, since the repository skips track
writes for an empty list).
* fix(jellyfin): register clients as players on any authenticated request
Jellyfin clients did not appear in the players list. Unlike Subsonic,
whose getPlayer middleware runs on every authenticated endpoint, the
Jellyfin router only registered a player on the /Sessions/Playing
reports, so browsing or streaming never created one.
Apply withPlayer to the whole authenticated group, mirroring Subsonic,
so the calling device registers (and scrobbling has a player) as soon as
it makes any authenticated request. Two follow-ups found while testing:
- Skip registration when the request carries no client/device info (no
X-Emby-Authorization, e.g. the /socket handshake that auths via
?api_key= only), which otherwise created a junk player named ' []'.
- URL-decode the X-Emby-Authorization field values. Jellify's
@jellyfin/sdk percent-encodes them (Device='Pixel%208%20Pro') while
Finamp sends them raw, so the player name showed as
'Jellify [Pixel%208%20Pro]'.
* refactor(jellyfin): move case-insensitive routing into the jellyfin package
The case-insensitive path normalization lived in the server package but
was only ever used by the Jellyfin router (its whole purpose is that
Jellyfin clients route case-insensitively while chi does not). Move it
into server/jellyfin and unexport it, so it sits with its only caller
and no longer needs to be exported across a package boundary.
* docs(jellyfin): document player registration, playlist and image behavior
Updates the package README for the behavior added/fixed this round:
- New 'Players and sessions' section: any authenticated request now
registers the device as a player (like Subsonic), with the
Client [Device] naming, URL-decoding of the Emby auth fields, and the
/socket skip that avoids a nameless player.
- Authentication: note AuthenticateByName is rate-limited per IP.
- Playlists: repeated vs comma-separated id params, and that an explicit
empty Ids clears the playlist while an omitted Ids leaves it untouched.
- Cover art: WebP/GIF magic-number detection plus the MaxImageUploadSize
and EnableArtworkUpload gates.
- Endpoints table: add the /Similar and /UserFavoriteItems /
/UserItems/.../UserData routes that were already served but unlisted.
* feat(jellyfin): expose configured users on the login user-picker
Adds Jellyfin.ExposedPublicUsers, a comma-separated allowlist of
usernames that GET /Users/Public advertises so Jellyfin clients (Finamp,
Jellify) can show a login user-picker instead of a blank username field.
The endpoint is unauthenticated, so it defaults to exposing no users and
never lists the full user table: only the admin-configured names are
returned, resolved live per request (a name that doesn't exist is skipped
and logged). Each entry is a minimal DTO (Name, Id) with no
Policy/Configuration, so admin status isn't leaked pre-login, and no
avatar since Navidrome has no per-user profile images.
* style(jellyfin): trim redundant comments
Remove comments that restated the code or duplicated an explanation
already given nearby, keeping the ones that capture non-obvious rationale
(client-specific quirks, gotchas). Comment-only change; no behavior
difference.
* perf(db): keep query planner statistics trustworthy with full ANALYZE
PRAGMA optimize's internal ANALYZE runs with a limited analysis budget
(~2000 rows) that writes wrong sqlite_stat1 entries for low-cardinality
indexes: on a 96K-track library it claimed (missing, library_id) narrows
to ~2000 rows when it matches the whole table. The planner then prefers
that index over the sort index and falls back to a full-table temp
B-tree sort per request, turning paginated song listings into
multi-second queries (reproduced at 5.5s on real hardware; ~90x slower
than with correct stats). Every index-creating migration re-triggered
the poisoning via the post-migration optimize, and the daily optimizer
could re-trigger it on large library changes. Setting analysis_limit on
the connection does not help: optimize ignores it.
Run a plain full ANALYZE instead: after migrations with schema changes,
and in db.Optimize (daily schedule and scan-end). Stats are stored in
the database file, so one connection suffices and the per-connection
pool loop is gone. The Optimize call at shutdown is removed: stats are
maintained at migration/scan/daily points, and an ANALYZE during
shutdown only delays it and races container stop timeouts.
* perf(db): add covering index for title-sorted song listings
Deep pagination over songs sorted by title (WHERE missing/library_id,
ORDER BY order_title LIMIT/OFFSET - the shape Jellyfin clients use to
enumerate the library, and non-admin native/Subsonic song lists share)
walked media_file_order_title and fetched the table row for every
skipped entry just to evaluate the filter and the annotation/bookmark
join keys: offset+limit random reads, seconds per page on cold spinning
disks.
The new (missing, library_id, order_title, id) index makes the offset
skip fully index-resident: filter columns and the join key come from the
index, and only the emitted page touches table rows. Measured on a
96K-track library: offset 50000 drops from ~6.5s (poisoned stats) /
~100ms (good stats, warm) to 24ms, and cold deep pages on NAS hardware
from ~7s to ~0.5s.
* feat(jellyfin): support transcoding via HLS playlist and server-forced player format
- withPlayer now propagates the player's configured transcoding into the
request context (like Subsonic's getPlayer), so a format forced in
Settings > Players applies to the Jellyfin stream endpoints
- new GET /Audio/{itemId}/main.m3u8, the endpoint Finamp plays through
when its transcoding setting is enabled: a single-segment HLS VOD
playlist pointing at the existing progressive transcode endpoint
- streamAudio: treat audioBitRate as bits/sec (Jellyfin convention) and
fall back to audioCodec as target format when no container is given
* fix(jellyfin): honor GenreIds when browsing genre albums and tracks
Finamp's genre screen sends ParentId=<libraryId> plus GenreIds=<genreId>,
but /Items ignored the param, so every genre returned the whole library.
- filter.ByGenreID delegates to persistence.TagIDFilter (exported, was
tagIDFilter), the same mechanism behind the native API's genre_id filter
- id lists are read via queryIDs, covering both spellings clients use
(comma-separated and repeated params); /Items?ids= gains the repeated
form too
* feat(jellyfin): filter album artists by GenreIds
Finamp's artist tab sends GenreIds to /Artists/AlbumArtists when a genre
filter is active; the param was ignored, returning all artists.
filter.ArtistsByGenreID matches artists credited as album artist on an
album with the genre, via a non-correlated semi-join over album
participants (86ms on a 29K-artist library; the correlated EXISTS form
takes 11 minutes). Applied on the browse path of /Artists* and
/Items?IncludeItemTypes=MusicArtist.
The performers variant (/Artists?GenreIds=) still ignores the filter: it
needs the same semi-join against media_file.participants, unmeasured on
large libraries.
* fix(jellyfin): version playlist image tag so clients refresh uploaded covers
Uploads were stored and served correctly, but Finamp kept showing the old
cover: it caches covers keyed by blurHash (its imageId is the item id,
which never changes), and both our image tag and synthetic blurhash were
derived from the playlist id alone.
The tag is now <id>-<UpdatedAt millis hex> (SetImage/RemoveImage go
through a full Put, which bumps UpdatedAt), and the blurhash derives from
the tag, so every cover change rotates both. UpdatedAt over-invalidates
(any playlist edit busts the cover cache), which only costs a refetch;
hashing the actual image remains the proper long-term tag.
An e2e test guards the whole chain, since a partial Put(pls, cols...)
would silently stop bumping UpdatedAt.
* fix(jellyfin): align cover upload limit and validation with the native endpoint
Cover uploads of large photos failed with a silent 400: the
MaxImageUploadSize cap was applied to the wire body, which Jellyfin
clients base64-encode (4/3 inflation), so the effective raw-image limit
was only ~7.5MB — and Finamp uploads picked photos uncompressed.
Align with the native endpoint:
- the limit caps the decoded image; the read cap allows for base64
inflation
- validate by decoding (image.DecodeConfig) and take the storage
extension from the real format instead of the Content-Type header,
which clients get wrong (Finamp falls back to image/jpeg); a HEIC or
corrupt file is now rejected instead of stored as a broken cover
- rejected uploads log the reason (size/limit/decode error); diagnosing
this from production logs previously required guesswork
* fix(jellyfin): optimize SongsByArtistID filter to improve performance at library scale
Signed-off-by: Deluan <deluan@navidrome.org>
* fix(jellyfin): sort tracks by release year for SortBy=PremiereDate
Finamp's "Latest Releases" artist section sends
SortBy=PremiereDate,Album,... descending; PremiereDate wasn't in the
Audio sort map, so applySort fell through to Album and the view came
back in reverse album-name order (a 2002 remix album first, the 2013
release last).
Map premieredate/productionyear to the year sort key, matching the
ProductionYear the DTO exposes for songs and the existing MusicAlbum
mapping (premieredate -> max_year).
* feat(jellyfin): expose PremiereDate on tracks and albums
Finamp's "Latest Releases" artist/genre sections re-sort the merged
server responses client-side by PremiereDate and keep the top 5. Without
the field every comparison returns equal and Dart's unstable sort leaves
the picks in arbitrary order — a 2007 remix could lead the list even
with the server sorting by year correctly.
Serialize PremiereDate as ISO 8601 from the date tag (padding partial
"2007"/"2007-02" values so DateTime.tryParse accepts them), falling
back to the year; omitted when neither exists.
* feat(jellyfin): resolve Finamp-truncated item ids (saved queue restore)
Finamp persists its play queue by packing every item id into exactly 16
bytes (packIds assumes Jellyfin's 32-hex GUID ids), so our longer ids
come back truncated after an app restart and "Failed to restore queue"
loops forever: the /Items?ids= batch resolves nothing.
Navidrome ids can't be made GUID-shaped (nanoid ids can exceed 128 bits),
so compensate server-side: a 16-char id — a length no Navidrome id family
uses — is resolved by unique-prefix range scan, with ambiguity failing
safe. The ids= batch echoes the id as requested (Finamp matches restored
items by its stored ids), and stream, image, item, user-data, favorite,
rating and playback-report endpoints accept truncated ids transparently.
The proper fix belongs upstream in Finamp's packIds; documented in the
README so this layer can be removed once that ships.
* feat(jellyfin): implement Items/{id}/InstantMix
Finamp requests an instant mix on every track tap when its "start
instant mix for individual tracks" setting is on (plus the long-press
menus); the 404 made those taps fail with an error and play nothing.
A track seed returns itself first — Finamp plays exactly what comes
back — followed by the external provider's similar songs, capped at the
requested limit and filtered to the caller's libraries. Container seeds
(artist/album) return the provider's similar-songs blend. Provider
errors and unknown seeds degrade to seed-only/empty results instead of
404s, reusing the Similar endpoints' bounded-wait singleflight (with a
distinct cache key, since mixes and similar lists answer different
shapes). Sonic-similarity backing stays a follow-up (see README).
* style(jellyfin): trim wordy comments
* refactor(jellyfin): apply cleanup review findings
- batch truncated-id resolution in /Items?ids=: one chunked range query
for all media-file prefixes instead of a query per id (a restored
queue sends hundreds of truncated ids)
- resolve truncated ids on the /Similar endpoints too, matching the
neighboring InstantMix; document which entry points don't resolve
- hoist maxImageUploadSize to core, deleting the byte-identical copies
in nativeapi and jellyfin (tests moved to core)
- drop the unused type parameter on premiereDate
* fix(jellyfin): never drop the instant mix seed on a slow provider
The seed track was built inside the awaited provider fetch, so when the
external agent was slow or unreachable the request hit the 10s wait and
answered a fully empty mix — Finamp then played nothing on tap, even
though the seed needs no provider at all (seen live: Last.fm unreachable
from the server, responseSize=49).
Build the seed outside the await: only the similar-songs tail is fetched
and bounded, and a timeout now degrades to a seed-only mix. Also folds
instantMixForSong into getInstantMix, since the tail is exactly
similarSongs.
* fix(jellyfin): prefer the recommended Authorization scheme when picking a token
Jellyfin's authorization guidance deprecates X-Emby-Token,
X-MediaBrowser-Token, X-Emby-Authorization and api_key; the Authorization
MediaBrowser scheme is the recommended form. All spellings stay accepted,
but when a client sends several, the recommended one now wins. Adds
coverage for the canonical Authorization header, which no test exercised
directly.
* refactor(jellyfin): rename parseEmbyAuth to parseMediaBrowserAuth
The scheme is named MediaBrowser; the old name evoked the deprecated
X-Emby-* spellings even though the function also parses the recommended
Authorization header.
* fix(jellyfin): prefer the Authorization header over X-Emby-Authorization
The recommended header now wins when both carry MediaBrowser data — but
only when it actually parses as MediaBrowser: a reverse proxy may inject
Basic/Digest credentials into Authorization while the client sends the
deprecated header, and those must not swallow the client's auth.
* fix(jellyfin): require the MediaBrowser scheme when parsing auth headers
The parser extracted key="value" pairs from any Authorization value; a
foreign scheme whose parameters happened to use our field names would
have been misread as client auth. Validate the scheme word instead
(case-insensitively, per HTTP), accepting the legacy "Emby" spelling
like real Jellyfin. Replaces the any-recognized-field heuristic for
detecting proxy-injected Basic/Digest credentials.
* Revert "perf(db): keep query planner statistics trustworthy with full ANALYZE"
This reverts commit 118563e1053196f0e2e42dd4bee54e39a04ab145.
The planner-statistics work now lives in its own PR (#5740); this branch
keeps only the covering-index migration.
* fix(db): renumber jellyfin covering-index migration after master's latest
* fix(db): renumber Jellyfin covering-index migration
* docs(jellyfin): document playlist annotations
* refactor(log): clarify comments on external services query params
* refactor(e2e): move Subsonic e2e suite to server/subsonic/e2e
All files in server/e2e were Subsonic tests, so relocate the package
under server/subsonic/e2e to sit alongside the API it exercises. The
package name stays 'e2e'; only doc/comment references are updated.
* refactor(filter): consolidate genre filters and unexport tagIDFilter
Route filterByGenre, ByGenreID, and ArtistsByGenreID through a single
genreTagFilter helper so the EXISTS json_tree(tags,"$.genre") predicate
lives in one place. With server/filter no longer using it, persistence's
TagIDFilter is only referenced within the package, so unexport it.
---------
Signed-off-by: Deluan <deluan@navidrome.org>
|
||
|
Deluan Quintão
|
ca27335d06
|
feat(playlists): per-user starred/rating annotations (backend) (#5749)
* feat(playlists): add average_rating column to playlist table
* feat(playlists): store and read per-user starred/rating annotations
* feat(playlists): clean up annotations when a playlist is deleted
* feat(subsonic): route star/unstar of a playlist to the playlist repository
* feat(subsonic): route setRating of a playlist to the playlist repository
* test(subsonic): guard that playlist responses never expose annotations
* fix(playlists): clean stale mis-typed annotations on upgrade; cover GetAll read-back
* fix(playlists): scope annotation join by item_type and harden delete
Address code-review findings on the playlist-annotations branch:
- withAnnotation: add an item_type predicate to the LEFT JOIN so a
mis-typed annotation row sharing an id can no longer leak into (or
duplicate) another entity's read. Correct for every caller since each
repo writes annotations with item_type = tableName. Regression test added.
- migration: reclassify legacy media_file-typed rows for playlist ids to
item_type='playlist' (instead of deleting them), preserving users' prior
playlist star/rating; run before the average_rating backfill so those
ratings are included.
- playlist Delete: replace the per-request full-table cleanAnnotations()
anti-join with a targeted, permission-safe (rows-affected gated),
best-effort delete so a cleanup failure no longer misreports an
already-committed delete as an error.
- MockPlaylistRepo: implement GetAll/IncPlayCount/ReassignAnnotation to
remove the dead All field and the nil-interface panic traps.
- test: use slices.IndexFunc instead of a hand-rolled find loop.
* feat(playlists): streamline playlist deletion by relying on annotation sweep
* docs(playlists): trim comments in annotation migration and test
Condense the verbose comments added in this branch per the project's
comment-minimalism guideline, keeping only the non-obvious rationale.
The migration's reclassify block is shortened while preserving the safety
invariant (playlist and media_file ids never collide, so the item_type
rewrite touches only mis-typed rows and cannot violate the unique key) and
the ordering note. The redundant 'Populate average_rating' comment is
dropped since the UPDATE is self-evident. The repository test's leakage
comment is condensed to two lines. No code behavior changes.
* refactor(subsonic): resolve setStar targets via GetEntityByID
Replace setStar's Album/Artist/Playlist Exists probe chain with a single
model.GetEntityByID lookup and a type switch, mirroring setRating. This
removes three per-id existence queries and keeps the two annotation paths
consistent.
An id that resolves to no known entity is logged and skipped rather than
filed as a spurious media_file annotation, and a lookup failure on one id no
longer aborts the whole batch. Also drop a duplicate empty-ids guard.
* refactor(playlists): drop no-op reclassify/backfill from migration
The average_rating migration carried two data-fix UPDATEs that are no-ops on
any real database:
- The media_file->playlist reclassification only matches rows no released
build ever created: playlists were never annotatable, so star/setRating of
a playlist id was never written as item_type='playlist'. Any stray
media_file-typed row for a playlist id is already removed by the media_file
annotation GC sweep (item_id not in media_file).
- The average_rating backfill runs before any item_type='playlist' row can
exist, so it can only ever write the default 0. Going forward SetRating
keeps average_rating current via updateAvgRating.
Reduce the migration to the column add/drop.
* refactor(persistence): bind annotation join params, derive idField from tableName
Address PR review: use Squirrel parameter binding for item_type/user_id in
the shared withAnnotation join instead of string concatenation, and pass
r.tableName+".id" from selectPlaylist so the join field stays consistent
with the surrounding r.tableName usage.
* fix(subsonic): surface datastore errors in setStar instead of skipping
Address PR review: setStar swallowed every GetEntityByID error and continued,
so a real datastore failure would still commit the transaction and emit a
refresh event as if the star succeeded. Skip only on model.ErrNotFound (an
unknown id); return any other error so the request fails and rolls back.
* test(subsonic): assert absent JSON keys instead of substring matches
Address PR review: substring checks are brittle ("starred" matches "starredAt",
"rating" matches "userRating"). Unmarshal the response and assert the
annotation keys are absent.
* fix(subsonic): skip refresh broadcast when a star request changes nothing
Address PR review (Codex): once setStar began skipping unknown ids, a request
containing only unresolvable ids left the RefreshResource empty, which
SendMessage serializes as a {*:*} wildcard that forces every client to
refresh. Only broadcast when at least one id was actually starred.
* fix(db): rebase playlist average_rating migration timestamp past master
The 20260708011823 migration predated the newest migration merged to
master (20260712211040_add_primary_key...), which Goose would silently
skip on already-upgraded databases. Rename it to a current timestamp so
it applies in order.
|
||
|
Deluan Quintão
|
cc315dcc8c
|
perf(db): keep query planner statistics trustworthy with full ANALYZE (#5740)
* perf(db): keep query planner statistics trustworthy with full ANALYZE PRAGMA optimize's internal ANALYZE runs with a limited analysis budget (~2000 rows) that writes wrong sqlite_stat1 entries for low-cardinality indexes: on a 96K-track library it claimed (missing, library_id) narrows to ~2000 rows when it matches the whole table. The planner then prefers that index over the sort index and falls back to a full-table temp B-tree sort per request, turning paginated song listings into multi-second queries (reproduced at 5.5s on real hardware; ~90x slower than with correct stats). Every index-creating migration re-triggered the poisoning via the post-migration optimize, and the daily optimizer could re-trigger it on large library changes. Setting analysis_limit on the connection does not help: optimize ignores it. Run a plain full ANALYZE instead: after migrations with schema changes, and in db.Optimize (daily schedule and scan-end). Stats are stored in the database file, so one connection suffices and the per-connection pool loop is gone. The Optimize call at shutdown is removed: stats are maintained at migration/scan/daily points, and an ANALYZE during shutdown only delays it and races container stop timeouts. * perf(db): drop startup PRAGMA optimize that re-poisons planner stats The startup PRAGMA optimize=0x10002 runs SQLite's budget-limited internal ANALYZE (bit 0x02), which writes truncated sqlite_stat1 rows for low-cardinality indexes -- the exact statistics-poisoning this PR set out to eliminate. Because DevOptimizeDB defaults to true, a restart with no pending migrations would re-poison the planner until the next scan or daily Optimize. Remove it: statistics are already refreshed with a full ANALYZE after schema-changing migrations (Init) and via Optimize at scan-end and on the daily schedule, so nothing on the startup path needs to touch them. Also clarify that Optimize is a no-op unless DevOptimizeDB is enabled. * chore(db): remove the DevOptimizeDB flag and skip Optimize on quick scans The flag only gated the optimize/ANALYZE maintenance calls and there is no reason to leave planner statistics unmaintained; the guards are gone along with the flag. The scan-end Optimize now runs only after full scans — quick scans barely move the statistics, and the daily schedule covers drift. * style(scanner): drop redundant comment in runOptimize * chore(persistence): drop the no-op PRAGMA optimize from ScanEnd Mask 0x10000 only selects candidate tables by size change; without the 0x02 action bit optimize does nothing (verified: sqlite_stat1 stays stale after a 100x table growth). The scan-end statistics refresh is db.Optimize's full ANALYZE, and the expression-collation-index concern the old comment guarded against no longer applies. * fix(scanner): run the post-scan ANALYZE in the server process With the external scanner (the default), the scan pipeline runs in a subprocess, so its ANALYZE was invisible to the server: SQLite loads sqlite_stat1 into the process's shared schema cache, and an ANALYZE from another process does not refresh it — verified with the production DSN that even brand-new pool connections keep planning with the old statistics until the server restarts. An in-process ANALYZE, by contrast, is immediately visible to every pooled connection through the same shared cache. Move the full-scan Optimize from the scanner pipeline to the scan controller, which always runs in the server process. * fix(scanner): honor promoted full scans in the optimize gate A quick scan resuming an interrupted full scan is promoted inside the scanner (possibly in a subprocess); mirror the promotion in the controller so the post-scan ANALYZE isn't skipped. * refactor: apply cleanup review findings - drop forceFullRescan's inline ANALYZE: Init already runs a full ANALYZE after any migration batch with schema changes, so upgrades including a full-rescan migration analyzed the whole DB twice - resumingFullScan uses a filtered CountAll instead of fetching and scanning all libraries - document why CallScan (CLI) deliberately skips the post-scan Optimize * perf(db): make planner analysis maintenance resilient Check analysis freshness every 30 minutes and refresh statistics when the last successful run is over 24 hours old or a scan marked them pending. Persist successful analysis state, retry skipped or failed maintenance, coordinate checks with scans, and cover standalone CLI full scans. * perf(db): avoid analyzing routine quick-scan changes Reserve pending analysis for full scans, unscanned libraries, and retry state. Incremental quick scans now rely on the 24-hour freshness window instead of triggering a full ANALYZE at the next maintenance check. * fix(scan): analyze resumed full scans in CLI * fix(db): back off failed analysis retries * feat(db): allow disabling scheduled analysis * test(db): remove redundant analysis coverage * refactor(db): split ANALYZE maintenance into optimize.go and dedupe call sites - move query-planner statistics code from db.go to its own optimize.go (and matching optimize_test.go) - log ANALYZE elapsed time inside Optimize/OptimizeIfNeeded instead of repeating the timing block at every call site - drop the LastDBAnalyzeAttemptAt write on success: it is only read while failures >= 1, and every failure rewrites it first - extract runPostScanAnalysis (cmd) and anyIncludedLibrary (scanner) helpers |
||
|
Kendall Garner
|
4998ac2c59
|
feat(server): add scrobble history Native API (#5761)
* initial scrobble api * feat: add scrobble retrieval api * address feedback (1) * fix spelling * be explicit about get * add primary key field, update index, remove rowid references * use unix timestamp for input and output --------- Co-authored-by: Deluan Quintão <deluan@navidrome.org> |
||
|
Deluan Quintão
|
d4387c5502
|
perf(db): index media_file album/artist sort orders (#5706)
* perf(db): add composite indexes for song list album/artist sorts
The media_file sort mappings for album, artist and albumArtist expand to
multi-column ORDER BY clauses that no existing index could satisfy, so SQLite
fell back to a full table scan plus a temp B-tree sort of every row (including
the large lyrics/tags/full_text columns) even for a single 15-item page. On a
96K-track library this made /api/song?_sort=album take 3.6s on a cold cache.
Add composite indexes matching the three sort mappings, allowing the query to
walk the index and stop at the page size, in both directions. Drop the now
redundant single-column order_album_name/order_artist_name indexes (strict
prefixes of the new composites) and three indexes with no query path:
birth_time is only read in Go code, and artist/album_artist text column
lookups go through the media_file_artists table instead.
* fix(ui): make composer and track number columns non-sortable in song list
Clicking the Composer header was a silent no-op: composer is not a media_file
column, so the native API's sanitizeSort drops the sort and returns rows in
table order. Track number sorting across the whole library is not meaningful
and cannot use an index (the existing index leads with disc_number). Mark both
columns sortable={false}, like quality and mood.
* test(persistence): add sort index coverage test for large tables
Guard against sort options silently losing index support: every sort mapping
on media_file, album and artist is now verified with EXPLAIN QUERY PLAN to be
satisfiable by an index (both directions), so adding a mapping or dropping an
index that reintroduces a full-table temp B-tree sort fails the test. Sorts
that genuinely cannot use an index (random, annotation-join columns, JSON
expressions) must be declared in an exceptions list with the reason, keeping
the trade-off visible in review.
To make the sort mappings the complete declared sort surface, add identity
mappings for the media_file columns the UI sorts by without a mapping (year,
genre, duration, channels, bpm, path, comment, play_count, play_date, rating).
These are behaviorally no-ops: the same ORDER BY was previously produced by
the field whitelist fallback.
* perf(db): drop PreferSortTags expression indexes from media_file
The media_file sort_title/sort_artist_name/sort_album_name expression indexes
are only usable when PreferSortTags is enabled - a config reported by ~0.1% of
installations (insights, week of 2026-06-22) - yet every install pays their
storage (~8.6MB on a 96K-track library) and scanner write overhead. Drop them:
PreferSortTags installs fall back to a full sort for title/artist/album orders,
everyone else gets smaller DBs and cheaper writes. The order_album_name and
order_artist_name collation checks remain valid, now satisfied by the composite
sort indexes.
Signed-off-by: Deluan <deluan@navidrome.org>
---------
Signed-off-by: Deluan <deluan@navidrome.org>
|
||
|
Deluan Quintão
|
427d4b9bce
|
fix(search): artists with atomic non-ASCII names unfindable after FTS5 migration (#5703)
* fix(scanner): update artist search_normalized when rescanning The FTS5 migration back-fills artist.search_normalized with a SQL punctuation-strip approximation, relying on the next scan to compute the precise value in Go (normalizeForFTS transliterates atomic letters like Ø/æ/ß that FTS5's remove_diacritics cannot fold). But the scanner persisted artists with an explicit column list that omitted search_normalized, so not even a full scan ever repaired it: an artist migrated from a pre-FTS database (e.g. "GØGGS") stayed unfindable by any ASCII search, while their albums and songs, which are saved with all columns, were fixed by a full scan. Add search_normalized to the column list so a full scan re-indexes the artist via the artist_fts trigger. * refactor(persistence): move normalizeForFTS to utils/str Export it as str.NormalizeForFTS so the upcoming migration can reuse the exact index-time normalization. Migrations cannot import the persistence package (persistence -> db -> db/migrations would be an import cycle). * fix(persistence): backfill artist search_normalized via migration Recompute artist.search_normalized with the precise Go normalization for databases migrated from pre-FTS5 versions, where the SQL back-fill could not transliterate atomic letters (Ø/æ/ß) and the scanner never rewrote the column. Only changed rows are updated, so the artist_fts update trigger re-indexes exactly the affected artists, making artists like GØGGS or MØ findable again without requiring a full scan. * refactor(persistence): share FTS punctuation-strip regex via utils/str Index-time normalization (NormalizeForFTS) and query-time processing (buildFTS5Query/ftsQueryDegraded) must produce matching tokens, so keep the punctuation-strip pattern in a single exported symbol instead of two identical private copies that could drift. Also document that derived columns computed in dbArtist.PostMapArgs must be listed in the scanner's artist Put, which is how search_normalized went stale in the first place. * chore(migrations): announce artist search backfill in the log Match the FTS5 migration's notice() pattern so startup isn't silent while the backfill runs on large libraries. * docs: tighten comments added in this branch * docs: describe FTSPunctStrip by what it matches, not one replacement |
||
|
Deluan Quintão
|
01b7c86f90
|
fix(scanner): stop logging expected lyrics sniff misses as warnings (#5702)
* fix(scanner): stop logging expected lyrics sniff misses as warnings During a scan, embedded lyrics are parsed with an empty suffix, which puts ParseLyrics into content-sniffing mode: it tries the TTML, SRT and Lyricsfile YAML parsers in turn before falling back to plain text. Every plain-text or LRC lyric therefore fails the structured probes on its way to the fallback, and each failure was logged at warning level with no indication of which file triggered it, flooding the scan log with benign "Error parsing lyrics, falling back to plain text" messages. A probe rejecting content it does not own during sniffing is expected control flow, so it is now logged at trace instead. A parse failure under an explicitly requested suffix (e.g. a malformed .yaml/.srt/.ttml sidecar) still warns, since the user declared that format. ParseLyrics gains ctx and path parameters so any warning names the offending file and carries request context where available; all call sites are updated accordingly. Also fixes a test-isolation bug in the new logging spec: the BeforeEach swapped the process-global default logger via SetDefaultLogger but only restored the log level on cleanup, leaking the null logger and its hook into later specs in the shared model suite. * test: use spec-scoped contexts instead of context.Background in lyrics tests Replace context.Background() with GinkgoT().Context() (and b.Context() in the parse benchmarks) across the lyrics-related tests, so contexts are cancelled when each spec ends. The embeddedLyrics fixture in core/lyrics is now a hand-written literal like its sibling fixtures, removing the construction-time ParseLyrics call that could not use a spec-scoped context. * refactor(model): attach lyrics parse log attribution via context Narrow ParseLyrics back to (ctx, suffix, lang, contents), dropping the path parameter added by the previous commit. Attribution now uses the codebase's existing idiom: callers that know the source attach it with log.NewContext (e.g. "file" for the media file or sidecar), and the plugin adapter tags both the plugin name and the track, fixing probe-miss logs that misattributed plugin-returned content to the file's own tags. This removes three adjacent string parameters that were easy to swap silently, and the "" placeholder most call sites had to pass. Also hardens the logging spec from the previous commit: the null test logger is now swapped in before raising the level (SetLevel forces the current default logger to trace, so the old order left the null logger at info and trace entries never reached the hook), the sniff test now asserts probe misses are observable at trace with file attribution instead of only asserting the absence of warnings, and cleanup restores the actual previous logger — via a new return value on log.SetDefaultLogger — instead of a bare logrus.New() that would discard hooks configured on the process-wide logger. * refactor(lyrics): hoist attributed log contexts out of loops Address review feedback on #5702: build the log-attributed context once per operation instead of per iteration, and reuse it on the surrounding log calls so the error/trace lines around ParseLyrics carry the same attribution fields. In fromExternalFile the sidecar path now rides the context for all log lines in the function, replacing the repeated explicit "path" field. * style(model): pass lyrics parse errors as final log arguments Per the project logging convention, errors go as the last argument (the log package normalizes them via its error case) instead of a keyed "error" pair, which stores the raw error value and bypasses that handling. Flagged by review on #5702; the keyed form was inherited from the original warning line. |
||
|
Deluan Quintão
|
0fab1861a0
|
fix(subsonic): make "recently added" order reproducible and consistent with RecentlyAddedByModTime (#5678)
* fix(subsonic): align album `created` with RecentlyAddedByModTime sort The album `created` attribute returned by search3, getAlbumList2 and the other album endpoints was always sourced from the album's CreatedAt (oldest song birth time), while the "recently added" sort is governed by RecentlyAddedByModTime: it orders by album.updated_at when that option is enabled and album.created_at otherwise. As a result, when RecentlyAddedByModTime was enabled, clients that cache album results and sort locally by `created` (e.g. for a "Date added" view) could not reproduce the order returned by getAlbumList2?type=newest, since the exposed value did not match the column driving the sort. Make albumCreatedAt config-aware so the primary timestamp it returns mirrors recentlyAddedSort: UpdatedAt when RecentlyAddedByModTime is set, CreatedAt otherwise. The existing zero-value fallback chain is preserved so this required OpenSubsonic field is never emitted as zero on legacy rows. Note: this is a behavior change for the contractual `created` attribute. With RecentlyAddedByModTime enabled, an album's reported `created` now reflects the newest song modification time and can change when files are modified. Scope is limited to albums; the song-level `created` (BirthTime) is unchanged. * fix(subsonic): order Recently Added by full-precision timestamp with tiebreak The recently_added sort wrapped the timestamp in datetime(), truncating it to whole seconds, and had no secondary sort key. Album timestamps carry sub-second precision (aggregated from song file birth-times), so on a fresh scan many albums tie at the second; SQLite then returns ties in query-plan order, which changes when a library filter is applied. This made the web UI "Recently Added" order invert between library selections and diverge from getAlbumList2?type=newest, and clients receiving the full-precision created value could never reproduce the server order. Sort on the raw, full-precision column with an album.id / media_file.id tiebreak instead. A new migration swaps the album datetime() expression indexes (and the plain media_file indexes) for composite (col, id) indexes that cover the new sort. Timestamps were already normalized to space-format by 20260316000000_normalize_timestamps, so raw-string comparison is safe. * fix(subsonic): make song created follow RecentlyAddedByModTime The song Created field returned BirthTime (file ctime), but the recently_added sort (shared by the Subsonic and native APIs, and the web UI) orders by created_at, or updated_at when RecentlyAddedByModTime is set. A Subsonic client, which can only sort by the created value it receives, could therefore never reproduce the server's "recently added" order in either mode. Add mediaFileCreatedAt mirroring albumCreatedAt and use it for child.Created: CreatedAt by default, UpdatedAt under RecentlyAddedByModTime, with BirthTime as a legacy fallback. This aligns song created with the sort column, matching how album created already works and what the native API/web UI present. |
||
|
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>
|
||
|
Deluan
|
32ac53dc9f |
refactor(migrations): propagate context.Context through all DB calls
Thread the context.Context that goose.UpContext already passes into every migration through to all DB calls: tx.Exec/Query/QueryRow become tx.ExecContext/QueryContext/QueryRowContext with ctx. The shared helpers in migration.go (notice, forceFullRescan, isDBInitialized) gain a ctx parameter and all call sites are updated. No-op migration functions use blank params (_ context.Context, _ *sql.Tx). This is a behavior-preserving change: the SQL, arguments, and ordering of every migration are unchanged; only cancellation/deadline propagation is added. Add a forbidigo lint rule scoped to db/migrations/ that forbids the non-context tx.Exec/Query/QueryRow forms, preventing regression. Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
Deluan Quintão
|
6abc2ed517 |
fix(transcoding): preserve source metadata when transcoding downloads (#5628)
* fix(transcoding): preserve source metadata when transcoding downloads Default transcoding commands used `-map 0🅰️0` with no metadata mapping, so transcoded files lost all source tags (title, artist, album, etc.). Downloads in the original format were unaffected because the file is copied byte-for-byte. Add `-map_metadata 0 -map_metadata 0:s:0` to the default commands. Both flags are required: `-map_metadata 0` copies format-level tags (MP3/FLAC sources) and `-map_metadata 0:s:0` copies stream-level tags (OPUS/OGG sources), which store tags at different levels. The flags are added in three coordinated places, since for users on the default command the args are built programmatically (buildDynamicArgs) rather than from the stored command string: - consts.go default commands, for new installations - buildDynamicArgs, the active path for default-command users - a migration updating only rows that still hold the exact old default, so customized commands are left untouched AAC is included for consistency but remains a no-op: its `-f adts` container cannot hold metadata, and the MP4 alternative breaks pipe streaming. Fixes #5623 * fix(transcoding): target audio stream for metadata and propagate ctx Address review feedback on the metadata-preservation change: - Use `-map_metadata 0:s:a:0` instead of `0:s:0` to copy tags from the first audio stream specifically. When a source has embedded cover art exposed as a video stream at index 0 (common in music files), `0:s:0` pulls the image stream's metadata and the audio tags are lost. Verified empirically with ffmpeg 7.1.3: a source with video at stream 0 and a tagged audio stream loses its title under `0:s:0` but keeps it under `0:s:a:0`; audio-only OPUS/MP3/FLAC sources are unaffected by the change. - Propagate the migration context via `tx.ExecContext(ctx, ...)` instead of discarding it, so the migration honors cancellation/timeouts. Claude-Session: https://claude.ai/code/session_015iFHDzX53wCKt11qFHMeZk |
||
|
Deluan Quintão
|
da56df3160
|
feat(smartplaylist): extend isMissing/isPresent to bpm, bitDepth and many text fields (#5603)
* feat(smartplaylist): support isMissing/isPresent on mbz_* and lyrics fields Mark the six mbz_* MusicBrainz ID columns and the lyrics column as Nullable in the criteria field map, then extend missingExpr to handle string columns where absence is encoded as NULL or empty string (plus '[]' for lyrics). The Numeric/Boolean path (ReplayGain) is preserved via an explicit type check. * refactor(model): make MediaFile BPM and BitDepth nullable pointers Convert BPM and BitDepth fields in model.MediaFile from int to *int so that 'tag absent' is distinguishable from zero. The metadata mapper now uses NullableFloat for BPM (nil when absent or zero/unparseable) and only sets BitDepth when the audio property is non-zero (lossy codecs report 0). All read sites use gg.V() for zero-fallback deref so Subsonic API output and transcoding behaviour are byte-identical to before. The persistence layer bridges the existing NOT NULL DB columns by coercing nil to 0 on write and 0 back to nil on read in PostMapArgs/PostScan; a later migration task will drop those constraints. Hash upgrade safety is verified by a new MediaFile.Hash describe block: nil *int hashes identically to the old int(0) default via ZeroNil+IgnoreZeroValue, so no files will be spuriously re-imported after this change. Extra files touched beyond the plan's list: core/stream/legacy_client_test.go (BitDepth in model.MediaFile literals), persistence/mediafile_repository.go (NOT NULL bridge). * test(model): pin pre-conversion golden hashes for BPM/BitDepth * feat(smartplaylist): support isMissing/isPresent on bpm and bitDepth * feat(db): make bpm and bit_depth columns nullable, backfill 0 to NULL Drop the NOT NULL constraint on media_file.bpm and bit_depth via a lossless migration that converts legacy 0-means-absent values to real NULL. Remove the temporary shim in PostScan/PostMapArgs that was bridging the old NOT NULL columns to the *int model fields. Add round-trip persistence tests asserting NULL storage for nil pointers and correct value round-trip for non-nil pointers. * test(e2e): verify isMissing/isPresent partition for nullable fields Add DescribeTable covering bpm, bitdepth, lyrics, and mbz_recording_id: for each field, isMissing + isPresent song counts must equal the total library count, proving the nullable-column SQL is exhaustive and correct. * test(e2e): seed bpm tag so isMissing/isPresent partition is non-trivial * fix(model): omit bitDepth from JSON when absent instead of emitting null * feat(smartplaylist): support isMissing/isPresent on more string fields Enable isMissing/isPresent operators for album, comment, catalognumber, discsubtitle, albumcomment, sorttitle, sortalbum, sortartist, sortalbumartist, and explicitstatus by marking them Nullable in fieldMap. * refactor(smartplaylist): unify missingExpr column logic into one flow Collapse the numeric/string fork in missingExpr into a single empties-driven loop (numeric/boolean fields simply have no empties), and replace the duplicated IsTag/IsRole guard with a three-way switch that expresses the dispatch model once. No SQL semantics change for string fields; numeric/boolean fields now emit a single-element Or/And which squirrel parenthesizes (e.g. `(col IS NULL)` instead of bare `col IS NULL`) — update the affected test expectations accordingly. |
||
|
Deluan Quintão
|
5ec6e6a8d4
|
fix(opensubsonic): make search3 empty-query pagination fast at large offsets (#5601)
* fix(subsonic): make search3 empty-query pagination fast at large offsets Empty-query search3 (used by clients like Symfonium to sync the whole library) degraded linearly with songOffset: the offset optimization in optimizePagination keeps the original query's LEFT JOINs (annotation, bookmark, library) inside its rowid NOT IN subquery, making it as slow as plain OFFSET (~5s per page at offset 900K on a 920K-track library). Rewrite the empty-query branch of doSearch to use the same two-phase approach as the FTS search: Phase 1 paginates rowids on the bare main table, which SQLite satisfies with a covering index at any offset; Phase 2 hydrates only the page's rows with all JOINs. The Phase 2 hydration logic is extracted into hydrateRowidPage, now shared with ftsSearch.execute. Also replace the media_file_missing index with a composite covering index on (missing, library_id), so Phase 1 stays covering for non-admin users, whose queries include a library_id filter. The composite serves all missing-only lookups via its prefix. With a 920K-track / 85K-album test library, search3 empty-query responses are now flat (~0.1s) at every offset, for both admin and non-admin users (previously 3-5s at offsets above 600K). * refactor(persistence): share search Phase 1 contract and dedup junction fan-out Extract the Phase 1 query assembly that was duplicated between the FTS search and the empty-query search into executeTwoPhase: both paths now supply only their strategy-specific FROM/JOINs and ORDER BY, while the shared contract (missing filter, library access, options.Filters, and Max/Offset semantics) lives in one place. Also fix a pagination integrity bug: the artist library filter joins the library_artist junction table, so an artist present in multiple libraries produced duplicate rowids in Phase 1, corrupting offset-based pagination (short pages and repeated artists during full-library syncs). Phase 1 now applies DISTINCT whenever a junction-based LibraryFilter is set. DISTINCT is used instead of GROUP BY because bm25() cannot be evaluated in a grouped query; plain-filter tables (media_file, album) skip the dedup so their Phase 1 keeps the streaming covering-index plan. This also fixes the same duplication in the pre-existing FTS search path. * fix(persistence): pin artist search Phase 1 join order with CROSS JOIN search3 always filters artists by library (library_artist.library_id IN ...), and with the junction JOIN in the search Phase 1 rowid query SQLite chose to drive from library_artist, sorting every junction row with a temp b-tree on each page — a flat ~200ms penalty per request at 405K artists, even at offset 0 (the previous code avoided this by accident: its GROUP BY artist.id pinned an artist-driven plan). Use CROSS JOIN (SQLite's explicit join-order override) in a search-only variant of the artist library filter, keeping artist as the outer table so Phase 1 streams rowids in artist.id order from the primary key index and LIMIT/OFFSET short-circuits. The DISTINCT dedup stays and costs nothing under the streaming plan. Other artist queries keep the planner's freedom. With 405K artists, empty-query artist search is now 0.07s at offset 0 and 0.25s at offset 399K end-to-end (was 0.31s/0.34s before this fix, and up to 1.2s on master at deep offsets). Artist FTS text search is unaffected. |
||
|
Deluan
|
bd3192be0b |
fix(server): make DB PRAGMA optimize error non-fatal
Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
Deluan Quintão
|
74185dc6d1
|
fix(smartplaylists): optimize smart playlist performance for role and tag criteria (#5515)
* fix(server): optimize smart playlist role queries for large criteria (#5511) Role-based smart playlist criteria (artist, composer, etc.) now query the indexed media_file_artists join table instead of parsing JSON via json_tree() on every row. Multiple conditions for the same role within an OR group are merged into a single EXISTS subquery (batched at 200 to stay under SQLite's expression tree depth limit). A composite index (media_file_id, role) replaces the now-redundant single-column (media_file_id) index on media_file_artists. Benchmark (40k tracks, 500 patterns, 3 artists/track): - Merged join-table: 15ms (9.3x faster) - Merged json_tree: 30ms (4.6x faster) - Unmerged baseline: 137ms * refactor: simplify role condition SQL generation and benchmark Extract shared roleCondSQL/roleExistsSQL helpers to deduplicate the EXISTS template between roleCond and roleCondGroup. Use slices.Chunk for batching per project convention. Extract runBenchQuery helper to eliminate triplicated benchmark execution loop. * chore: raise roleCondBatchSize to 350 The empirical SQLite limit is 496 conditions per merged EXISTS subquery. Raising from 200 to 350 reduces the number of batches (e.g. 500 patterns now splits into 2 batches instead of 3). * fix(server): apply OR-merge optimization to tag conditions too Generalize mergeRoleConds into mergeJsonConds to also collapse multiple tag conditions for the same tag (e.g. genre) within OR groups. This gives the same ~5x speedup for tag-heavy smart playlists as the role optimization gives for artist-heavy ones. * refactor: benchmark uses real criteria pipeline instead of hand-built SQL The "Current" sub-benchmark now builds criteria.Criteria expressions and runs them through the actual newSmartPlaylistCriteria → Where() → ToSql() pipeline, validating the real production code path. The baseline still uses hand-built SQL representing the old json_tree approach. * fix: stabilize merged group ordering and close rows before error check Sort group keys in mergeJsonConds so the merged additions have deterministic order across runs, improving SQLite statement cache reuse. Move rows.Close() before rows.Err() in benchmark helper. |
||
|
Deluan Quintão
|
8f0b4930ff
|
refactor(conf): replace eager dir creation with lazy Dir type (#5495)
* feat(conf): add Dir type with lazy directory creation Introduces the Dir type that wraps a directory path string and defers os.MkdirAll until the first call to Path() or MustPath(), using sync.Once to ensure the creation happens exactly once. Implements fmt.Stringer, encoding.TextMarshaler, and encoding.TextUnmarshaler for config integration. Includes Ginkgo/Gomega tests covering all methods and error paths. * refactor(conf): replace eager dir creation with lazy Dir type Change DataFolder, CacheFolder, Plugins.Folder, and Backup.Path from string to Dir. Remove all os.MkdirAll calls from Load() so directories are created lazily on first Path()/MustPath() call. Artwork folder creation was already handled at point-of-use in image_upload.go. Add SnapshotConfig() to conf package for safe test config save/restore that avoids copying sync.Once inside Dir fields. Fix copy-lock vet warning in nativeapi/config.go by marshalling pointer instead of value. * refactor(conf): migrate tests and db init to lazy Dir type Update all test files to use conf.NewDir() for Dir field assignments. Ensure DataFolder is created lazily when the database is first opened in db.Db(). Remove eager directory creation from conf.Load() tests. * fix(conf): address review findings for Dir type - Use os.ModePerm for DataFolder/CacheFolder (was 0700, should match original behavior). Add NewDirWithPerm for PluginsFolder (0700). - Use Path() instead of MustPath() in db.Prune() to avoid logFatal from background cron job. - Panic on marshal/unmarshal errors in SnapshotConfig (test helper). - Clean up redundant String()/MustPath() calls in plugin manager. - Remove dead code in dir_test.go. Signed-off-by: Deluan <deluan@navidrome.org> * fix(conf): add GoString to Dir for clean config dump output Implement fmt.GoStringer on Dir so pretty.Sprintf shows the path string instead of internal struct fields (sync.Once, perm, err). Also add TODO comment to configtest about removing the indirection. * fix(dir): improve error logging in MustPath method Signed-off-by: Deluan <deluan@navidrome.org> * refactor(tests): remove redundant tests for unwritable DataFolder and CacheFolder Signed-off-by: Deluan <deluan@navidrome.org> * fix(conf): address PR review feedback - Ensure Plugins.Folder always uses 0700, even when user-configured (previously only the derived default got restrictive permissions). - Create LogFile parent directory before opening, so LogFile paths inside a not-yet-created DataFolder work correctly. --------- Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
Deluan Quintão
|
24e526e09a
|
fix(transcoding): place -ss before -i for fast input seeking (#5492)
Move the ffmpeg -ss (seek/offset) parameter before -i in all transcoding commands so ffmpeg uses input seeking instead of output seeking. Per the ffmpeg docs, placing -ss before -i seeks at the demuxer level by keyframe (very fast), and since FFmpeg 2.1 it is also frame-accurate when transcoding. The previous placement after -i caused ffmpeg to decode and discard all audio up to the seek point, which was unnecessarily slow — especially problematic for lengthy files (4+ hours). Both code paths are updated: buildDynamicArgs (for default formats) and createFFmpegCommand (for custom templates without %t). A database migration updates existing default commands in the transcoding table. |
||
|
Deluan
|
1f3a7efa75 |
fix(backup): surface real SQLite error when backup step fails
The error-check ordering after backupOp.Step(-1) checked !done before err, which masked the underlying SQLite error (e.g. SQLITE_BUSY, I/O errors) with a generic "backup not done with step -1" message. On failure, Step returns done=false together with a non-nil err, so the !done branch short-circuited before the real error was ever reported. Swap the checks so the SQLite error is returned first, making failing backups actually diagnosable. Refs https://github.com/navidrome/navidrome/issues/5305#issuecomment-4230470593 |
||
|
Deluan Quintão
|
9b0bfc606b
|
fix(subsonic): always emit required created field on AlbumID3 (#5340)
* fix(subsonic): always emit required `created` field on AlbumID3
Strict OpenSubsonic clients (e.g. Navic via dev.zt64.subsonic) reject
search3/getAlbum/getAlbumList2 responses that omit the `created` field,
which the spec marks as required. Navidrome was dropping it whenever
the album's CreatedAt was zero.
Root cause was threefold:
1. buildAlbumID3/childFromAlbum conditionally emitted `created`, so a
zero CreatedAt became a missing JSON key.
2. ToAlbum's `older()` helper treated a zero BirthTime as the minimum,
so a single track with missing filesystem birth time could poison
the album aggregation.
3. phase_1_folders' CopyAttributes copied `created_at` from the previous
album row unconditionally, propagating an already-zero value forward
on every metadata-driven album ID change. Since sql_base_repository
drops `created_at` on UPDATE, a poisoned row could never self-heal.
Fixes:
- Always emit `created`, falling back to UpdatedAt/ImportedAt when
CreatedAt is zero. Adds albumCreatedAt() helper used by both
buildAlbumID3 and childFromAlbum.
- Guard `older()` against a zero second argument.
- Skip the CopyAttributes call in phase_1_folders when the previous
album's created_at is zero, so the freshly-computed value survives.
- New migration backfills existing broken rows from media_file.birth_time
(falling back to updated_at).
Tested against a real DB: repaired 605/6922 affected rows, no side
effects on healthy rows.
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor(subsonic): return albumCreatedAt by value to avoid heap escape
Returning *time.Time from albumCreatedAt caused Go escape analysis to
move the entire model.Album parameter to the heap, since the returned
pointer aliased a field of the value receiver. For hot endpoints like
getAlbumList2 and search3, this meant one full-struct heap allocation
per album result.
Return time.Time by value and let callers wrap it with gg.P() to take
the address locally. Only the small time.Time value escapes; the
model.Album struct stays on the stack. Also corrects the doc comment
to reflect the actual guarantee ("best-effort" rather than "non-zero"),
matching the test case that exercises the all-zero fallback.
---------
Signed-off-by: Deluan <deluan@navidrome.org>
|
||
|
Barend
|
991bd3ed21
|
fix(db): resolve schema inconsistencies in library_artist and scrobble_buffer tables (#5047)
* fix(db): resolve schema inconsistencies in library_artist and scrobble_buffer tables * fix(db): address PR comments around speed of the migration * fix(db): simplify schema inconsistencies migration Remove ineffective PRAGMA foreign_keys and cache_size statements, which are no-ops inside goose's wrapping transaction. Drop the down migration body (Navidrome does not run down migrations) and document the intent. Rename the file to refresh the timestamp after rebase. --------- Co-authored-by: Deluan Quintão <deluan@navidrome.org> |
||
|
Deluan Quintão
|
ba8d427890
|
feat(ui): add cover art support for internet radio stations (#5229)
* feat(artwork): add KindRadioArtwork and EntityRadio constant * feat(model): add UploadedImage field and artwork methods to Radio * feat(model): add Radio to GetEntityByID lookup chain * feat(db): add uploaded_image column to radio table * feat(artwork): add radio artwork reader with uploaded image fallback * feat(api): add radio image upload/delete endpoints * feat(ui): add radio artwork ID prefix to getCoverArtUrl * feat(ui): add cover art display and upload to RadioEdit * feat(ui): add cover art thumbnails to radio list * feat(ui): prefer artwork URL in radio player helper * refactor: remove redundant code in radio artwork - Remove duplicate Avatar rendering in RadioList by reusing CoverArtField - Remove redundant UpdatedAt assignment in radio image handlers (already set by repository Put) * refactor(ui): extract shared useImageLoadingState hook Move image loading/error/lightbox state management into a shared useImageLoadingState hook in common/. Consolidates duplicated logic from AlbumDetails, PlaylistDetails, RadioEdit, and artist detail views. * feat(ui): use radio placeholder icon when no uploaded image Remove album placeholder fallback from radio artwork reader so radios without an uploaded image return ErrUnavailable. On the frontend, show the internet-radio-icon.svg placeholder instead of requesting server artwork when no image is uploaded, allowing favicon fallback in the player. * refactor(ui): update defaultOff fields in useSelectedFields for RadioList Signed-off-by: Deluan <deluan@navidrome.org> * fix: address code review feedback - Add missing alt attribute to CardMedia in RadioEdit for accessibility - Fix UpdateInternetRadio to preserve UploadedImage field by fetching existing radio before updating (prevents Subsonic API from clearing custom artwork) - Add Reader() level tests to verify ErrUnavailable is returned when radio has no uploaded image * refactor: add colsToUpdate to RadioRepository.Put Use the base sqlRepository.put with column filtering instead of hand-rolled SQL. UpdateInternetRadio now specifies only the Subsonic API fields, preventing UploadedImage from being cleared. Image upload/delete handlers specify only UploadedImage. * fix: ensure UpdatedAt is included in colsToUpdate for radio Put --------- Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
Deluan Quintão
|
e7c6e78dd0
|
fix(db): normalize timestamps and fix recently added album sorting (#5176)
* fix(db): normalize timestamps and fix recently added album sorting
SQLite stores timestamps as TEXT and uses string comparison for ORDER BY.
Timestamps in RFC3339 T-format ('2024-01-01T10:00:00Z') sort incorrectly
against space-format ('2024-01-01 10:00:00+00:00') because 'T' (ASCII 84)
> ' ' (ASCII 32), causing albums with T-format timestamps to appear as
newer than they are in the "Recently Added" list.
This adds a migration to normalize all T-format timestamps across all
tables to the space-format expected by go-sqlite3, wraps the
recently_added sort with datetime() to make it format-agnostic, and
replaces the plain album timestamp indexes with expression indexes to
maintain query performance.
* fix(test): improve recently_added sort test robustness
Use same-date timestamps (2024-01-15T08:00:00Z vs 2024-01-15 20:00:00)
so the T-vs-space character difference at position 10 actually triggers
the sorting bug. Initialize index variables to -1 and assert both test
albums are found before comparing positions.
* chore(db): update migration timestamp to 2026-03-16
|
||
|
Deluan Quintão
|
ab8a58157a
|
feat: add artist image uploads and image-folder artwork source (#5198)
* feat: add shared ImageUploadService for entity image management * feat: add UploadedImage field and methods to Artist model * feat: add uploaded_image column to artist table * feat: add ArtistImageFolder config option * refactor: wire ImageUploadService and delegate playlist file ops to it Wire ImageUploadService into the DI container and refactor the playlist service to delegate image file operations (SetImage/RemoveImage) to the shared ImageUploadService, removing duplicated file I/O logic. A local ImageUploadService interface is defined in core/playlists to avoid an import cycle between core and core/playlists. * feat: artist artwork reader checks uploaded image first * feat: add image-folder priority source for artist artwork * feat: cache key invalidation for image-folder and uploaded images * refactor: extract shared image upload HTTP helpers * feat: add artist image upload/delete API endpoints * refactor: playlist handlers use shared image upload helpers * feat: add shared ImageUploadOverlay component * feat: add i18n keys for artist image upload * feat: add image upload overlay to artist detail pages * refactor: playlist details uses shared ImageUploadOverlay component * fix: add gosec nolint directive for ParseMultipartForm * refactor: deduplicate image upload code and optimize dir scanning - Remove dead ImageFilename methods from Artist and Playlist models (production code uses core.imageFilename exclusively) - Extract shared uploadedImagePath helper in model/image.go - Extract findImageInArtistFolder to deduplicate dir-scanning logic between fromArtistImageFolder and getArtistImageFolderModTime - Fix fileInputRef in useCallback dependency array * fix: include artist UpdatedAt in artwork cache key Without this, uploading or deleting an artist image would not invalidate the cached artwork because the cache key was only based on album folder timestamps, not the artist's own UpdatedAt field. * feat: add Portuguese translations for artist image upload * refactor: use shared i18n keys for cover art upload messages Move cover art upload/remove translations from per-entity sections (artist, playlist) to a shared top-level "message" section, avoiding duplication across entity types and translation files. * refactor: move cover art i18n keys to shared message section for all languages * refactor: simplify image upload code and eliminate redundancies Extracted duplicate image loading/lightbox state logic from DesktopArtistDetails and MobileArtistDetails into a shared useArtistImageState hook. Moved entity type constants to the consts package and replaced raw string literals throughout model, core, and nativeapi packages. Exported model.UploadedImagePath and reused it in core/image_upload.go to consolidate path construction. Cached the ArtistImageFolder lookup result in artistReader to eliminate a redundant os.ReadDir call on every artwork request. Signed-off-by: Deluan <deluan@navidrome.org> * style: fix prettier formatting in ImageUploadOverlay * fix: address code review feedback on image upload error handling - RemoveImage now returns errors instead of swallowing them - Artist handlers distinguish not-found from other DB errors - Defer multipart temp file cleanup after parsing * fix: enforce hard request size limit with MaxBytesReader for image uploads Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
Deluan Quintão
|
d0fbba14ff
|
fix(db): check both name and target_format in default transcodings migration (#5175)
The ensure_default_transcodings migration only checked target_format before inserting, but the transcoding table has UNIQUE constraints on both name and target_format. Older installations may have entries where the name matches a default (e.g., 'opus audio') but the target_format differs (e.g., 'oga' instead of 'opus'), causing a UNIQUE constraint violation on name during the INSERT. Fixes #5174 |
||
|
Deluan Quintão
|
d8bc41fbb1
|
fix: use ADTS for AAC transcoding, temporarily exclude AAC from transcode decisions (#5167)
* fix: use ADTS format for AAC transcoding to avoid silent output on ffmpeg 8.0+ The fragmented MP4 muxer (`-f ipod -movflags frag_keyframe+empty_moov`) produces corrupt/silent audio when ffmpeg pipes to stdout, confirmed on ffmpeg 8.0+. The moof atom offset values are zeroed out in pipe mode, causing AAC decoder errors. Switch to `-f adts` (raw AAC framing) which works reliably via pipe and is widely supported by clients including UPnP/Sonos devices. * fix: exclude AAC from transcode decision, as it is not working for Sonos. Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
Deluan Quintão
|
d7c3a50f86
|
fix: player MaxBitRate cap, format-aware defaults, browser profile filtering (#5165)
* feat(transcode): apply player MaxBitRate cap and use format-aware default bitrates Add player MaxBitRate cap to the transcode decider so server-side player bitrate limits are respected when making OpenSubsonic transcode decisions. The player cap is applied only when it is more restrictive than the client's maxAudioBitrate (or when the client has no limit). Also replace the hardcoded 256 kbps default with a format-aware lookup that checks the DB first (for user-customized values), then built-in defaults, and finally falls back to 256 kbps. For lossless→lossy transcoding, prefer maxTranscodingAudioBitrate over maxAudioBitrate when available. * test(e2e): add tests for player MaxBitRate cap and format-aware default bitrates Add e2e tests covering: - Player MaxBitRate forcing transcode when source exceeds cap - Player MaxBitRate having no effect when source is under cap - Client limit winning when more restrictive than player MaxBitRate - Player MaxBitRate winning when more restrictive than client limit - Player MaxBitRate=0 having no effect - Format-aware defaults: mp3 (192kbps), opus (128kbps) instead of hardcoded 256 - maxAudioBitrate fallback for lossless→lossy when no maxTranscodingAudioBitrate - maxTranscodingAudioBitrate taking priority over maxAudioBitrate - Combined player + client limits flowing correctly through decision→stream * feat(transcode): update transcoding profiles to add flac, filter by supported codecs, and ensure mp3 fallback Signed-off-by: Deluan <deluan@navidrome.org> * fix(db): ensure all default transcodings exist on upgrade Older installations that were seeded before aac/flac were added to DefaultTranscodings may be missing these entries. The previous migration only added flac; this one ensures all default transcodings are present without touching user-customized entries. * test: remove duplication Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
Deluan
|
928741ef25 |
fix(db): recreate probe_data column as NOT NULL with empty string default
The probe_data column was added with DEFAULT NULL in migration 20260307175815, which causes sql.Scan errors when reading into Go string fields. This migration drops and recreates the column with DEFAULT '' NOT NULL to prevent NULL scan errors. |
||
|
Deluan Quintão
|
ae1e0ddb11
|
feat(subsonic): implement OpenSubsonic Transcoding extension (#4990)
* feat(subsonic): implement transcode decision logic and codec handling for media files Signed-off-by: Deluan <deluan@navidrome.org> * fix(subsonic): update codec limitation structure and decision logic for improved clarity Signed-off-by: Deluan <deluan@navidrome.org> * fix(transcoding): update bitrate handling to use kilobits per second (kbps) across transcode decision logic Signed-off-by: Deluan <deluan@navidrome.org> * refactor(transcoding): simplify container alias handling in matchesContainer function Signed-off-by: Deluan <deluan@navidrome.org> * fix(transcoding): enforce POST method for GetTranscodeDecision and handle non-POST requests Signed-off-by: Deluan <deluan@navidrome.org> * feat(transcoding): add enums for protocol, comparison operators, limitations, and codec profiles in transcode decision logic Signed-off-by: Deluan <deluan@navidrome.org> * refactor(transcoding): streamline limitation checks and applyLimitation logic for improved readability and maintainability Signed-off-by: Deluan <deluan@navidrome.org> * refactor(transcoding): replace strings.EqualFold with direct comparison for protocol and limitation checks Signed-off-by: Deluan <deluan@navidrome.org> * refactor(transcoding): rename token methods to CreateTranscodeParams and ParseTranscodeParams for clarity Signed-off-by: Deluan <deluan@navidrome.org> * refactor(transcoding): enhance logging for transcode decision process and client info conversion Signed-off-by: Deluan <deluan@navidrome.org> * refactor(transcoding): rename TranscodeDecision to Decider and update related methods for clarity Signed-off-by: Deluan <deluan@navidrome.org> * refactor(transcoding): enhance transcoding config lookup logic for audio codecs Signed-off-by: Deluan <deluan@navidrome.org> * refactor(transcoding): enhance transcoding options with sample rate support and improve command handling Signed-off-by: Deluan <deluan@navidrome.org> * refactor(transcoding): add bit depth support for audio transcoding and enhance related logic Signed-off-by: Deluan <deluan@navidrome.org> * refactor(transcoding): enhance AAC command handling and support for audio channels in streaming Signed-off-by: Deluan <deluan@navidrome.org> * refactor(transcoding): streamline transcoding logic by consolidating stream parameter handling and enhancing alias mapping Signed-off-by: Deluan <deluan@navidrome.org> * refactor(transcoding): update default command handling and add codec support for transcoding Signed-off-by: Deluan <deluan@navidrome.org> * fix: implement noopDecider for transcoding decision handling in tests Signed-off-by: Deluan <deluan@navidrome.org> * fix: address review findings for OpenSubsonic transcoding PR Fix multiple issues identified during code review of the transcoding extension: add missing return after error in shared stream handler preventing nil pointer panic, replace dead r.Body nil check with MaxBytesReader size limit, distinguish not-found from other DB errors, fix bpsToKbps integer truncation with rounding, add "pcm" to isLosslessFormat for consistency with model.IsLossless(), add sampleRate/bitDepth/channels to streaming log, fix outdated test comment, and add tests for conversion functions and GetTranscodeStream parameter passing. * feat(transcoding): add sourceUpdatedAt to decision and validate transcode parameters Signed-off-by: Deluan <deluan@navidrome.org> * fix: small issues Updated mock AAC transcoding command to use the new default (ipod with fragmented MP4) matching the migration, ensuring tests exercise the same buildDynamicArgs code path as production. Improved archiver test mock to match on the whole StreamRequest struct instead of decomposing fields, making it resilient to future field additions. Added named constants for JWT claim keys in the transcode token and wrapped ParseTranscodeParams errors with ErrTokenInvalid for consistency. Documented the IsLossless BitDepth fallback heuristic as temporary until Codec column is populated. Signed-off-by: Deluan <deluan@navidrome.org> * fix(transcoding): adapt transcode claims to struct-based auth.Claims Updated transcode token handling to use the struct-based auth.Claims introduced on master, replacing the previous map[string]any approach. Extended auth.Claims with transcoding-specific fields (MediaID, DirectPlay, UpdatedAt, Channels, SampleRate, BitDepth) and added float64 fallback in ClaimsFromToken for numeric claims that lose their Go type during JWT string serialization. Also added the missing lyrics parameter to all subsonic.New() calls in test files. * feat(model): add ProbeData field and UpdateProbeData repository method Add probe_data TEXT column to media_file for caching ffprobe results. Add UpdateProbeData to MediaFileRepository interface and implementations. Use hash:"ignore" tag so probe data doesn't affect MediaFile fingerprints. * feat(ffmpeg): add ProbeAudioStream for authoritative audio metadata Add ProbeAudioStream to FFmpeg interface, using ffprobe to extract codec, profile, bitrate, sample rate, bit depth, and channels. Parse bits_per_raw_sample as fallback for FLAC/ALAC bit depth. Normalize "unknown" profile to empty string. All parseProbeOutput tests use real ffprobe JSON from actual files. * feat(transcoding): integrate ffprobe into transcode decisions Add ensureProbed to probe media files on first transcode decision, caching results in probe_data. Build SourceStream from probe data with fallback to tag-based metadata. Refactor decision logic to pass StreamDetails instead of MediaFile, enabling codec profile limitations (e.g., audioProfile) to use probe data. Add normalizeProbeCodec to map ffprobe codec names (dsd_lsbf_planar, pcm_s16le) to internal names (dsd, pcm). NewDecider now accepts ffmpeg.FFmpeg; wire_gen.go regenerated. * feat(transcoding): add DevEnableMediaFileProbe config flag Add DevEnableMediaFileProbe (default true) to allow disabling ffprobe- based media file probing as a safety fallback. When disabled, the decider uses tag-based metadata from the scanner instead. * test(transcode): add ensureProbed unit tests Test probing when ProbeData is empty, skipping when already set, error propagation from ffprobe, and DevEnableMediaFileProbe flag. * refactor(ffmpeg): use command constant and select_streams for ProbeAudioStream Move ffprobe arguments to a probeAudioStreamCmd constant, following the same pattern as extractImageCmd and probeCmd. Add -select_streams a:0 to only probe the first audio stream, avoiding unnecessary parsing of video and artwork streams. Derive the ffprobe binary path safely using filepath.Dir/Base instead of replacing within the full path string. * refactor(transcode): decouple transcode token claims from auth.Claims Remove six transcode-specific fields (MediaID, DirectPlay, UpdatedAt, Channels, SampleRate, BitDepth) from auth.Claims, which is shared with session and share tokens. Transcode tokens are signed parameter-passing tokens, not authentication tokens, so coupling them to auth created misleading dependencies. The transcode package now owns its own JWT claim serialization via Decision.toClaimsMap() and paramsFromToken(), using generic auth.EncodeToken/DecodeAndVerifyToken wrappers that keep TokenAuth encapsulated. Wire format (JWT claim keys) is unchanged, so in-flight tokens remain compatible. Signed-off-by: Deluan <deluan@navidrome.org> * refactor(transcode): simplify code after review Extract getIntClaim helper to eliminate repeated int/int64/float64 JWT claim extraction pattern in paramsFromToken and ClaimsFromToken. Rewrite checkIntLimitation as a one-liner delegating to applyIntLimitation. Return probe result from ensureProbed to avoid redundant JSON round-trip. Extract toResponseStreamDetails helper and mediaTypeSong constant in the API layer, and use transcode.ProtocolHTTP constant instead of hardcoded string. Signed-off-by: Deluan <deluan@navidrome.org> * fix(ffmpeg): enhance bit_rate parsing logic for audio streams Signed-off-by: Deluan <deluan@navidrome.org> * fix(transcode): improve code review findings across transcode implementation - Fix parseProbeData to return nil on JSON unmarshal failure instead of a zero-valued struct, preventing silent degradation of source stream details - Use probe-resolved codec for lossless detection in buildSourceStream instead of the potentially stale scanner data - Remove MediaFile.IsLossless() (dead code) and consolidate lossless detection in isLosslessFormat(), using codec name only — bit depth is not reliable since lossy codecs like ADPCM report non-zero values - Add "wavpack" to lossless codec list (ffprobe codec_name for WavPack) - Guard bpsToKbps against negative input values - Fix misleading comment in buildTemplateArgs about conditional injection - Avoid leaking internal error details in Subsonic API responses - Add missing test for ErrNotFound branch in GetTranscodeDecision - Add TODO for hardcoded protocol in toResponseStreamDetails * refactor(transcode): streamline transcoding command lookup and format resolution Signed-off-by: Deluan <deluan@navidrome.org> * feat(transcode): implement server-side transcoding override for player formats Signed-off-by: Deluan <deluan@navidrome.org> * fix(transcode): honor bit depth and channel constraints in transcoding selection selectTranscodingOptions only checked sample rate when deciding whether same-format transcoding was needed, ignoring requested bit depth and channel reductions. This caused the streamer to return raw audio when the transcode decision requested downmix or bit-depth conversion. * refactor(transcode): unify streaming decision engine via MakeDecision Move transcoding decision-making out of mediaStreamer and into the subsonic Stream/Download handlers, using transcode.Decider.MakeDecision as the single decision engine. This eliminates selectTranscodingOptions and the mismatch between decision and streaming code paths (decision used LookupTranscodeCommand with built-in fallbacks, while streaming used FindByFormat which only checked the DB). - Add DecisionOptions with SkipProbe to MakeDecision so the legacy streaming path never calls ffprobe - Add buildLegacyClientInfo to translate legacy stream params (format, maxBitRate, DefaultDownsamplingFormat) into a synthetic ClientInfo - Add resolveStreamRequest on the subsonic Router to resolve legacy params into a fully specified StreamRequest via MakeDecision - Simplify DoStream to a dumb executor that receives pre-resolved params - Remove selectTranscodingOptions entirely Signed-off-by: Deluan <deluan@navidrome.org> * refactor(transcode): move MediaStreamer into core/transcode and unify StreamRequest Moved MediaStreamer, Stream, TranscodingCache and related types from core/media_streamer.go into core/transcode/, eliminating the duplicate StreamRequest type. The transcode.StreamRequest now carries all fields (ID, Format, BitRate, SampleRate, BitDepth, Channels, Offset) and ResolveStream returns a fully-populated value, removing manual field copying at every call site. Also moved buildLegacyClientInfo into the transcode package alongside ResolveStream, and unexported ParseTranscodeParams since it was only used internally by ValidateTranscodeParams. * refactor(transcode): rename Decider methods and unexport Params type Rename ResolveStream → ResolveRequest and ValidateTranscodeParams → ResolveRequestFromToken for clarity and consistency. The new ResolveRequestFromToken returns a StreamRequest directly (instead of the intermediate Params type), eliminating manual Params→StreamRequest conversion in callers. Unexport Params to params since it is now only used internally for JWT token parsing. * test(transcode): remove redundant tests and use constants Remove tests that duplicate coverage from integration-level tests (toClaimsMap, paramsFromToken round-trips, applyServerOverride direct call, duplicate 410 handler test). Replace raw "http" strings with ProtocolHTTP constant. Consolidate lossy -sample_fmt tests into DescribeTable. * refactor(transcode): split oversized files into focused modules Split transcode.go and transcode_test.go into focused files by concern: - decider.go: decision engine (MakeDecision, direct play/transcode evaluation, probe) - token.go: JWT token encode/decode (params, toClaimsMap, paramsFromToken, CreateTranscodeParams, ResolveRequestFromToken) - legacy_client.go: legacy Subsonic bridge (buildLegacyClientInfo, ResolveRequest) - codec_test.go: isLosslessFormat and normalizeProbeCodec tests - token_test.go: token round-trip and ResolveRequestFromToken tests Moved the Decider interface from types.go to decider.go to keep it near its implementation, and cleaned up types.go to contain only pure type definitions and constants. No public API changes. * refactor(transcode): reorder parameters in applyServerOverride function Signed-off-by: Deluan <deluan@navidrome.org> * test(e2e): add NewTestStream function and implement spyStreamer for testing Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
Deluan Quintão
|
acd69f6a4f
|
feat(playlist): support #EXTALBUMARTURL directive and sidecar images (#5131)
* feat(playlist): add migration for playlist image field rename and external URL * refactor(playlist): rename ImageFile to UploadedImage and ArtworkPath to UploadedImagePath Rename playlist model fields and methods for clarity in preparation for adding external image URL and sidecar image support. Add the new ExternalImageURL field to the Playlist model. * feat(playlist): parse #EXTALBUMARTURL directive in M3U imports * feat(playlist): always sync ExternalImageURL on re-scan, preserve UploadedImage * feat(artwork): add sidecar image discovery and cache invalidation for playlists Add playlist sidecar image support to the artwork reader fallback chain. A sidecar image (e.g., MyPlaylist.jpg next to MyPlaylist.m3u) is discovered via case-insensitive base name matching using model.IsImageFile(). Cache invalidation uses max(playlist.UpdatedAt, imageFile.ModTime()) to bust stale artwork when sidecar or ExternalImageURL local files change. * feat(artwork): add external image URL source to playlist artwork reader Add fromPlaylistExternalImage source function that resolves playlist cover art from ExternalImageURL, supporting both HTTP(S) URLs (via the existing fromURL helper) and local file paths (via os.Open). Insert it in the Reader() fallback chain between sidecar and tiled cover. * refactor(artwork): simplify playlist artwork source functions Extract shared fromLocalFile helper, use url.Parse for scheme check, and collapse sidecar directory scan conditions. * test(artwork): remove redundant fromPlaylistSidecar tests These tests duplicated scenarios already covered by findPlaylistSidecarPath tests combined with fromLocalFile (tested via fromPlaylistExternalImage). After refactoring fromPlaylistSidecar to a one-liner composing those two functions, the wrapper tests add no value. * fix(playlist): address security review comments from PR #5131: - Use url.PathUnescape instead of url.QueryUnescape for file:// URLs so that '+' in filenames is preserved (not decoded as space). - Validate all local image paths (file://, absolute, relative) against known library boundaries via libraryMatcher, rejecting paths outside any configured library. - Harden #EXTALBUMARTURL against path traversal and SSRF by adding EnableM3UExternalAlbumArt config flag (default false, also disabled by EnableExternalServices=false) to gate HTTP(S) URL storage at parse time and fetching at read time (defense in depth). - Log a warning when os.ReadDir fails in findPlaylistSidecarPath for diagnosability. - Extract resolveLocalPath helper to simplify resolveImageURL. Signed-off-by: Deluan <deluan@navidrome.org> * feat(playlist): implement human-friendly filename generation for uploaded playlist cover images Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
adrbn
|
d004f99f8f
|
feat(playlist): add custom playlist cover art upload (#5110)
* feat(playlist): add custom playlist cover art upload - #406 Allow users to upload, view, and remove custom cover images for playlists. Custom images take priority over the auto-generated tiled artwork. Backend: - Add `image_path` column to playlist table (migration with proper rollback) - Add `SetImage`/`RemoveImage` methods to playlist service - Add `POST/DELETE /api/playlist/{id}/image` endpoints - Prioritize custom image in artwork reader pipeline - Clean up image files on playlist deletion - Use glob-based cleanup to prevent orphaned files across format changes - Reject uploads with undetermined image type (400) Frontend: - Hover overlay on playlist cover with upload (camera) and remove (trash) buttons - Lightbox for full-size cover art viewing - Cover art thumbnails in the playlist list view - Loading/error states and i18n strings Closes #406 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: adrbn <128328324+adrbn@users.noreply.github.com> * refactor: rename playlist image path migration file Signed-off-by: Deluan <deluan@navidrome.org> * fix(playlist): address review feedback for cover art upload - #406 - Use httpClient instead of raw fetch for image upload/remove - Revert glob cleanup to simple imagePath check - Add log.Error before all error HTTP responses - Add backend tests for SetImage and RemoveImage Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: adrbn <128328324+adrbn@users.noreply.github.com> * refactor(playlist): use Playlist.ArtworkPath() for image storage Migrate all playlist image path handling to use the new Playlist.ArtworkPath() method as the single source of truth. The DB now stores only the filename (e.g. "pls-1.jpg") instead of a relative path, and images are stored under {DataFolder}/artwork/playlist/ instead of {DataFolder}/playlist_images/. The artwork root directory is created at startup alongside DataFolder and CacheFolder. This also removes the conf dependency from reader_playlist.go since path resolution is now fully encapsulated in the model. Signed-off-by: Deluan <deluan@navidrome.org> * refactor(playlist): streamline artwork image selection logic Signed-off-by: Deluan <deluan@navidrome.org> * refactor: move translation keys, add pt-BR translations Signed-off-by: Deluan <deluan@navidrome.org> * refactor(playlist): rename image_path to image_file Rename the playlist cover art column and field from image_path/ImagePath to image_file/ImageFile across the migration, model, service, tests, and UI. The new name more accurately describes what the field stores (a filename, not a path) and aligns with the existing ImageFiles/IsImageFile naming conventions in the codebase. --------- Signed-off-by: adrbn <128328324+adrbn@users.noreply.github.com> Signed-off-by: Deluan <deluan@navidrome.org> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Deluan Quintão <deluan@navidrome.org> |
||
|
Deluan Quintão
|
d9a215e1e3
|
feat(plugins): allow mounting library directories as read-write (#5122)
* feat(plugins): mount library directories as read-only by default Add an AllowWriteAccess boolean to the plugin model, defaulting to false. When off, library directories are mounted with the extism "ro:" prefix (read-only). Admins can explicitly grant write access via a new toggle in the Library Permission card. * test: add tests to buildAllowedPaths Signed-off-by: Deluan <deluan@navidrome.org> * chore: improve allowed paths logging for library access Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
Deluan Quintão
|
54de0dbc52
|
feat(server): implement FTS5-based full-text search (#5079)
* build: add sqlite_fts5 build tag to enable FTS5 support
* feat: add SearchBackend config option (default: fts)
* feat: add buildFTS5Query for safe FTS5 query preprocessing
* feat: add FTS5 search backend with config toggle, refactor legacy search
- Add searchExprFunc type and getSearchExpr() for backend selection
- Rename fullTextExpr to legacySearchExpr
- Add ftsSearchExpr using FTS5 MATCH subquery
- Update fullTextFilter in sql_restful.go to use configured backend
* feat: add FTS5 migration with virtual tables, triggers, and search_participants
Creates FTS5 virtual tables for media_file, album, and artist with
unicode61 tokenizer and diacritic folding. Adds search_participants
column, populates from JSON, and sets up INSERT/UPDATE/DELETE triggers.
* feat: populate search_participants in PostMapArgs for FTS5 indexing
* test: add FTS5 search integration tests
* fix: exclude FTS5 virtual tables from e2e DB restore
The restoreDB function iterates all tables in sqlite_master and
runs DELETE + INSERT to reset state. FTS5 contentless virtual tables
cannot be directly deleted from. Since triggers handle FTS5 sync
automatically, simply skip tables matching *_fts and *_fts_* patterns.
* build: add compile-time guard for sqlite_fts5 build tag
Same pattern as netgo: compilation fails with a clear error if
the sqlite_fts5 build tag is missing.
* build: add sqlite_fts5 tag to reflex dev server config
* build: extract GO_BUILD_TAGS variable in Makefile to avoid duplication
* fix: strip leading * from FTS5 queries to prevent "unknown special query" error
* feat: auto-append prefix wildcard to FTS5 search tokens for broader matching
Every plain search token now gets a trailing * appended (e.g., "love" becomes
"love*"), so searching for "love" also matches "lovelace", "lovely", etc.
Quoted phrases are preserved as exact matches without wildcards. Results are
ordered alphabetically by name/title, so shorter exact matches naturally
appear first.
* fix: clarify comments about FTS5 operator neutralization
The comments said "strip" but the code lowercases operators to
neutralize them (FTS5 operators are case-sensitive). Updated comments
to accurately describe the behavior.
* fix: use fmt.Sprintf for FTS5 phrase placeholders
The previous encoding used rune('0'+index) which silently breaks with
10+ quoted phrases. Use fmt.Sprintf for arbitrary index support.
* fix: validate and normalize SearchBackend config option
Normalize the value to lowercase and fall back to "fts" with a log
warning for unrecognized values. This prevents silent misconfiguration
from typos like "FTS", "Legacy", or "fts5".
* refactor: improve documentation for build tags and FTS5 requirements
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: convert FTS5 query and search backend normalization tests to DescribeTable format
Signed-off-by: Deluan <deluan@navidrome.org>
* fix: add sqlite_fts5 build tag to golangci configuration
Signed-off-by: Deluan <deluan@navidrome.org>
* feat: add UISearchDebounceMs configuration option and update related components
Signed-off-by: Deluan <deluan@navidrome.org>
* fix: fall back to legacy search when SearchFullString is enabled
FTS5 is token-based and cannot match substrings within words, so
getSearchExpr now returns legacySearchExpr when SearchFullString
is true, regardless of SearchBackend setting.
* fix: add sqlite_fts5 build tag to CI pipeline and Dockerfile
* fix: add WHEN clauses to FTS5 AFTER UPDATE triggers
Added WHEN clauses to the media_file_fts_au, album_fts_au, and
artist_fts_au triggers so they only fire when FTS-indexed columns
actually change. Previously, every row update (e.g., play count, rating,
starred status) triggered an unnecessary delete+insert cycle in the FTS
shadow tables. The WHEN clauses use IS NOT for NULL-safe comparison of
each indexed column, avoiding FTS index churn for non-indexed updates.
* feat: add SearchBackend configuration option to data and insights components
Signed-off-by: Deluan <deluan@navidrome.org>
* fix: enhance input sanitization for FTS5 by stripping additional punctuation and special characters
Signed-off-by: Deluan <deluan@navidrome.org>
* feat: add search_normalized column for punctuated name search (R.E.M., AC/DC)
Add index-time normalization and query-time single-letter collapsing to
fix FTS5 search for punctuated names. A new search_normalized column
stores concatenated forms of punctuated words (e.g., "R.E.M." → "REM",
"AC/DC" → "ACDC") and is indexed in FTS5 tables. At query time, runs of
consecutive single letters (from dot-stripping) are collapsed into OR
expressions like ("R E M" OR REM*) to match both the original tokens and
the normalized form. This enables searching by "R.E.M.", "REM", "AC/DC",
"ACDC", "A-ha", or "Aha" and finding the correct results.
* refactor: simplify isSingleUnicodeLetter to avoid []rune allocation
Use utf8.DecodeRuneInString to check for a single Unicode letter
instead of converting the entire string to a []rune slice.
* feat: define ftsSearchColumns for flexible FTS5 search column inclusion
Signed-off-by: Deluan <deluan@navidrome.org>
* feat: update collapseSingleLetterRuns to return quoted phrases for abbreviations
Signed-off-by: Deluan <deluan@navidrome.org>
* feat: implement extractPunctuatedWords to handle artist/album names with embedded punctuation
Signed-off-by: Deluan <deluan@navidrome.org>
* feat: implement extractPunctuatedWords to handle artist/album names with embedded punctuation
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: punctuated word handling to improve processing of artist/album names
Signed-off-by: Deluan <deluan@navidrome.org>
* feat: add CJK support for search queries with LIKE filters
Signed-off-by: Deluan <deluan@navidrome.org>
* feat: enhance FTS5 search by adding album version support and CJK handling
Signed-off-by: Deluan <deluan@navidrome.org>
* refactor: search configuration to use structured options
Signed-off-by: Deluan <deluan@navidrome.org>
* feat: enhance search functionality to support punctuation-only queries and update related tests
Signed-off-by: Deluan <deluan@navidrome.org>
---------
Signed-off-by: Deluan <deluan@navidrome.org>
|
||
|
Maximilian
|
a704e86ac1
|
refactor: run Go modernize (#5002) | ||
|
Terry Raimondo
|
03120bac32
|
feat(subsonic): Add avgRating from subsonic spec (#4900)
* feat(subsonic): add averageRating to API responses Add averageRating attribute to Subsonic API responses for artists, albums, and songs. The average is calculated across all user ratings. * perf(db): add index for average rating queries Add composite index on (item_id, item_type, rating) to optimize the correlated subquery used for calculating average ratings. Signed-off-by: Terry Raimondo <terry.raimondo@gmail.com> * test: add tests for averageRating feature Add tests for: - Album.AverageRating calculation in persistence layer - MediaFile.AverageRating calculation in persistence layer - AverageRating mapping in subsonic response helpers Signed-off-by: Terry Raimondo <terry.raimondo@gmail.com> * test: improve averageRating rounding test with 3 users Add third test user to fixtures and update rounding test to use 3 ratings (5 + 4 + 4) / 3 = 4.33 for proper decimal rounding coverage. Signed-off-by: Terry Raimondo <terry.raimondo@gmail.com> * perf: store avg_rating on entity tables instead of using subquery - Add avg_rating column to album, media_file, and artist tables - Update SetRating() to recalculate and store average when ratings change - Read avg_rating directly from entity table in withAnnotation() - Remove old annotation index migration (no longer needed) This trades write-time computation for read-time performance by pre-computing the average rating instead of using a correlated subquery on every read. * feat: add Subsonic.EnableAverageRating config option (default true) Allow administrators to disable exposing averageRating in Subsonic API responses if they don't want to expose other users' rating data. The avg_rating column is still updated internally when users rate items, but the value is only included in API responses when this option is enabled. * address PR comments - Use structs:"avg_rating" with db:"avg_rating" tag instead of SQL alias - Remove avg_rating indexes (not needed) - Populate avg_rating columns from existing ratings in migration * Woops * rename avg_rating column to average_rating --------- Signed-off-by: Terry Raimondo <terry.raimondo@gmail.com> |
||
|
Deluan Quintão
|
03a45753e9
|
feat(plugins): New Plugin System with multi-language PDK support (#4833)
* chore(plugins): remove the old plugins system implementation Signed-off-by: Deluan <deluan@navidrome.org> * feat(plugins): implement new plugin system with using Extism Signed-off-by: Deluan <deluan@navidrome.org> * feat(plugins): add capability detection for plugins based on exported functions Signed-off-by: Deluan <deluan@navidrome.org> * feat(plugins): add auto-reload functionality for plugins with file watcher support Signed-off-by: Deluan <deluan@navidrome.org> * feat(plugins): add auto-reload functionality for plugins with file watcher support Signed-off-by: Deluan <deluan@navidrome.org> * refactor(plugins): standardize variable names and remove superfluous wrapper functions Signed-off-by: Deluan <deluan@navidrome.org> * fix(plugins): improve error handling and logging in plugin manager Signed-off-by: Deluan <deluan@navidrome.org> * refactor(plugins): implement plugin function call helper and refactor MetadataAgent methods Signed-off-by: Deluan <deluan@navidrome.org> * fix(plugins): race condition in plugin manager * tests(plugins): change BeforeEach to BeforeAll in MetadataAgent tests Signed-off-by: Deluan <deluan@navidrome.org> * tests(plugins): optimize tests Signed-off-by: Deluan <deluan@navidrome.org> * tests(plugins): more optimizations Signed-off-by: Deluan <deluan@navidrome.org> * test(plugins): ignore goroutine leaks from notify library in tests Signed-off-by: Deluan <deluan@navidrome.org> * feat(plugins): add Wikimedia plugin for Navidrome to fetch artist metadata Signed-off-by: Deluan <deluan@navidrome.org> * feat(plugins): enhance plugin logging and set User-Agent header Signed-off-by: Deluan <deluan@navidrome.org> * feat(plugins): implement scrobbler plugin with authorization and scrobbling capabilities Signed-off-by: Deluan <deluan@navidrome.org> * feat(plugins): integrate logs Signed-off-by: Deluan <deluan@navidrome.org> * refactor(plugins): clean up manifest struct and improve plugin loading logic Signed-off-by: Deluan <deluan@navidrome.org> * feat(plugins): add metadata agent and scrobbler schemas for bootstrapping plugins Signed-off-by: Deluan <deluan@navidrome.org> * feat(hostgen): add hostgen tool for generating Extism host function wrappers - Implemented hostgen tool to generate wrappers from annotated Go interfaces. - Added command-line flags for input/output directories and package name. - Introduced parsing and code generation logic for host services. - Created test data for various service interfaces and expected generated code. - Added documentation for host services and annotations for code generation. - Implemented SubsonicAPI service with corresponding generated code. * feat(subsonicapi): update Call method to return JSON string response Signed-off-by: Deluan <deluan@navidrome.org> * feat(plugins): implement SubsonicAPI host function integration with permissions Signed-off-by: Deluan <deluan@navidrome.org> * fix(generator): error-only methods in response handling Signed-off-by: Deluan <deluan@navidrome.org> * feat(plugins): generate client wrappers for host functions Signed-off-by: Deluan <deluan@navidrome.org> * refactor(generator): remove error handling for response.Error in client templates Signed-off-by: Deluan <deluan@navidrome.org> * feat(scheduler): add Scheduler service interface with host function wrappers for scheduling tasks * feat(plugins): add WASI build constraints to client wrapper templates, to avoid lint errors Signed-off-by: Deluan <deluan@navidrome.org> * feat(scheduler): implement Scheduler service with one-time and recurring scheduling capabilities Signed-off-by: Deluan <deluan@navidrome.org> * refactor(manifest): remove unused ConfigPermission from permissions schema Signed-off-by: Deluan <deluan@navidrome.org> * feat(scheduler): add scheduler callback schema and implementation for plugins Signed-off-by: Deluan <deluan@navidrome.org> * refactor(scheduler): streamline scheduling logic and remove unused callback tracking Signed-off-by: Deluan <deluan@navidrome.org> * refactor(scheduler): add Close method for resource cleanup on plugin unload Signed-off-by: Deluan <deluan@navidrome.org> * docs(scheduler): clarify SchedulerCallback requirement for scheduling functions Signed-off-by: Deluan <deluan@navidrome.org> * fix: update wasm build rule to include all Go files in the directory Signed-off-by: Deluan <deluan@navidrome.org> * feat: rewrite the wikimedia plugin using the XTP CLI Signed-off-by: Deluan <deluan@navidrome.org> * refactor(scheduler): replace uuid with id.NewRandom for schedule ID generation Signed-off-by: Deluan <deluan@navidrome.org> * refactor: capabilities registration Signed-off-by: Deluan <deluan@navidrome.org> * test: add scheduler service isolation test for plugin instances Signed-off-by: Deluan <deluan@navidrome.org> * refactor: update plugin manager initialization and encapsulate logic Signed-off-by: Deluan <deluan@navidrome.org> * feat: add WebSocket service definitions for plugin communication Signed-off-by: Deluan <deluan@navidrome.org> * feat: implement WebSocket service for plugin integration and connection management Signed-off-by: Deluan <deluan@navidrome.org> * feat: add Crypto Ticker example plugin for real-time cryptocurrency price updates via Coinbase WebSocket API Also add the lifecycle capability Signed-off-by: Deluan <deluan@navidrome.org> * fix: use context.Background() in invokeCallback for scheduled tasks Signed-off-by: Deluan <deluan@navidrome.org> * refactor: rename plugin.create() to plugin.instance() Signed-off-by: Deluan <deluan@navidrome.org> * refactor: rename pluginInstance to plugin for consistency across the codebase Signed-off-by: Deluan <deluan@navidrome.org> * refactor: simplify schedule cloning in Close method and enhance plugin cleanup error handling Signed-off-by: Deluan <deluan@navidrome.org> * feat: implement Artwork service for generating artwork URLs in Navidrome plugins - WIP Signed-off-by: Deluan <deluan@navidrome.org> * refactor: moved public URL builders to avoid import cycles Signed-off-by: Deluan <deluan@navidrome.org> * feat: add Cache service for in-memory TTL-based caching in plugins Signed-off-by: Deluan <deluan@navidrome.org> * feat: add Discord Rich Presence example plugin for Navidrome integration Signed-off-by: Deluan <deluan@navidrome.org> * refactor: host function wrappers to use structured request and response types - Updated the host function signatures in `nd_host_artwork.go`, `nd_host_scheduler.go`, `nd_host_subsonicapi.go`, and `nd_host_websocket.go` to accept a single parameter for JSON requests. - Introduced structured request and response types for various cache operations in `nd_host_cache.go`. - Modified cache functions to marshal requests to JSON and unmarshal responses, improving error handling and code clarity. - Removed redundant memory allocation for string parameters in favor of JSON marshaling. - Enhanced error handling in WebSocket and cache operations to return structured error responses. * refactor: error handling in various plugins to convert response.Error to Go errors - Updated error handling in `nd_host_scheduler.go`, `nd_host_websocket.go`, `nd_host_artwork.go`, `nd_host_cache.go`, and `nd_host_subsonicapi.go` to convert string errors from responses into Go errors. - Removed redundant error checks in test data plugins for cleaner code. - Ensured consistent error handling across all plugins to improve reliability and maintainability. * refactor: rename fake plugins to test plugins for clarity in integration tests Signed-off-by: Deluan <deluan@navidrome.org> * feat: add help target to Makefile for plugin usage instructions Signed-off-by: Deluan <deluan@navidrome.org> * feat: add Cover Art Archive plugin as an example of Python plugin Signed-off-by: Deluan <deluan@navidrome.org> * feat: update Makefile and README to clarify Go plugin usage Signed-off-by: Deluan <deluan@navidrome.org> * feat: include plugin capabilities in loading log message Signed-off-by: Deluan <deluan@navidrome.org> * feat: add trace logging for plugin availability and error handling in agents Signed-off-by: Deluan <deluan@navidrome.org> * feat: add Now Playing Logger plugin to showcase calling host functions from Python plugins Signed-off-by: Deluan <deluan@navidrome.org> * feat: generate Python client wrappers for various host services Signed-off-by: Deluan <deluan@navidrome.org> * feat: add generated host function wrappers for Scheduler and SubsonicAPI services Signed-off-by: Deluan <deluan@navidrome.org> * feat: update Python plugin documentation and usage instructions for host function wrappers Signed-off-by: Deluan <deluan@navidrome.org> * feat: add Webhook Scrobbler plugin in Rust to send HTTP notifications on scrobble events Signed-off-by: Deluan <deluan@navidrome.org> * feat: enable parallel loading of plugins during startup Signed-off-by: Deluan <deluan@navidrome.org> * docs: update README to include WebSocket callback schema in plugin documentation Signed-off-by: Deluan <deluan@navidrome.org> * feat: extend plugin watcher with improved logging and debounce duration adjustment Signed-off-by: Deluan <deluan@navidrome.org> * add trace message for plugin recompiles Signed-off-by: Deluan <deluan@navidrome.org> * feat: implement plugin cache purging functionality Signed-off-by: Deluan <deluan@navidrome.org> * test: move purgeCacheBySize unit tests Signed-off-by: Deluan <deluan@navidrome.org> * feat(plugins UI): add plugin repository and database support Signed-off-by: Deluan <deluan@navidrome.org> * feat(plugins UI): add plugin management routes and middleware Signed-off-by: Deluan <deluan@navidrome.org> * feat(plugins UI): implement plugin synchronization with database for add, update, and remove actions Signed-off-by: Deluan <deluan@navidrome.org> * feat(plugins UI): add PluginList and PluginShow components with plugin management functionality Signed-off-by: Deluan <deluan@navidrome.org> * feat(plugins): optimize plugin change detection Signed-off-by: Deluan <deluan@navidrome.org> * refactor(plugins UI): improve PluginList structure Signed-off-by: Deluan <deluan@navidrome.org> * feat(plugins UI): enhance PluginShow with author, website, and permissions display Signed-off-by: Deluan <deluan@navidrome.org> * feat(plugins UI): refactor to use MUI and RA components Signed-off-by: Deluan <deluan@navidrome.org> * feat(plugins UI): add error handling for plugin enable/disable actions Signed-off-by: Deluan <deluan@navidrome.org> * refactor(plugins): inject PluginManager into native API Signed-off-by: Deluan <deluan@navidrome.org> * refactor(plugins): update GetManager to accept DataStore parameter Signed-off-by: Deluan <deluan@navidrome.org> * feat(plugins): add subsonicRouter to Manager and refactor host service registration Signed-off-by: Deluan <deluan@navidrome.org> * refactor(plugins): enhance debug logging for plugin actions and recompile logic Signed-off-by: Deluan <deluan@navidrome.org> * refactor(plugins): break manager.go into smaller, focused files Signed-off-by: Deluan <deluan@navidrome.org> * refactor(plugins): streamline error handling and improve plugin retrieval logic Signed-off-by: Deluan <deluan@navidrome.org> * refactor(plugins): update newWebSocketService to use WebSocketPermission for allowed hosts Signed-off-by: Deluan <deluan@navidrome.org> * refactor(plugins): introduce ToggleEnabledSwitch for managing plugin enable/disable state Signed-off-by: Deluan <deluan@navidrome.org> * docs: update READMEs Signed-off-by: Deluan <deluan@navidrome.org> * feat(library): add Library service for metadata access and filesystem integration Signed-off-by: Deluan <deluan@navidrome.org> * feat(plugins): add Library Inspector plugin for periodic library inspection and file size logging Signed-off-by: Deluan <deluan@navidrome.org> * docs: update README to reflect JSON configuration format for plugins Signed-off-by: Deluan <deluan@navidrome.org> * fix(build): update target to wasm32-wasip1 for improved WASI support Signed-off-by: Deluan <deluan@navidrome.org> * feat(plugins): implement configuration management UI with key-value pairs support Signed-off-by: Deluan <deluan@navidrome.org> * feat(ui): adjust grid layout in InfoRow component for improved responsiveness Signed-off-by: Deluan <deluan@navidrome.org> * feat(plugins): rename ErrorIndicator to EnabledOrErrorField and enhance error handling logic Signed-off-by: Deluan <deluan@navidrome.org> * feat(i18n): add Portuguese translations for plugin management and notifications Signed-off-by: Deluan <deluan@navidrome.org> * feat(plugins): add support for .ndp plugin packages and update build process Signed-off-by: Deluan <deluan@navidrome.org> * docs: update README for .ndp plugin packaging and installation instructions Signed-off-by: Deluan <deluan@navidrome.org> * feat(plugins): implement KVStore service for persistent key-value storage Signed-off-by: Deluan <deluan@navidrome.org> * docs: enhance README with Extism plugin development resources and recommendations Signed-off-by: Deluan <deluan@navidrome.org> * feat(plugins): integrate event broker into plugin manager Signed-off-by: Deluan <deluan@navidrome.org> * feat(plugins): update config handling in PluginShow to track last record state Signed-off-by: Deluan <deluan@navidrome.org> * feat(plugins): add Rust host function library and example implementation of Discord Rich Presence plugin in Rust Signed-off-by: Deluan <deluan@navidrome.org> * feat(plugins): generate Rust lib.rs file to expose host function wrappers Signed-off-by: Deluan <deluan@navidrome.org> * refactor(plugins): update JSON field names to camelCase for consistency Signed-off-by: Deluan <deluan@navidrome.org> * refactor: reduce cyclomatic complexity by refactoring main function Signed-off-by: Deluan <deluan@navidrome.org> * feat(plugins): enhance Rust code generation with typed struct support and improved type handling Signed-off-by: Deluan <deluan@navidrome.org> * feat(plugins): add Go client library with host function wrappers and documentation Signed-off-by: Deluan <deluan@navidrome.org> * feat(plugins): generate Go client stubs for non-WASM platforms Signed-off-by: Deluan <deluan@navidrome.org> * feat(plugins): update client template file names for consistency Signed-off-by: Deluan <deluan@navidrome.org> * feat(plugins): add initial implementation of the Navidrome Plugin Development Kit code generator - Pahse 1 Signed-off-by: Deluan <deluan@navidrome.org> * feat(plugins): implementation of the Navidrome Plugin Development Kit with generated client wrappers and service interfaces - Phase 2 Signed-off-by: Deluan <deluan@navidrome.org> * feat(plugins): implementation of the Navidrome Plugin Development Kit with generated client wrappers and service interfaces - Phase 2 (2) Signed-off-by: Deluan <deluan@navidrome.org> * feat(plugins): implementation of the Navidrome Plugin Development Kit with generated client wrappers and service interfaces - Phase 3 Signed-off-by: Deluan <deluan@navidrome.org> * feat(plugins): implementation of the Navidrome Plugin Development Kit with generated client wrappers and service interfaces - Phase 4 Signed-off-by: Deluan <deluan@navidrome.org> * feat(plugins): implementation of the Navidrome Plugin Development Kit with generated client wrappers and service interfaces - Phase 5 Signed-off-by: Deluan <deluan@navidrome.org> * refactor(plugins): consistent naming/types across PDK Signed-off-by: Deluan <deluan@navidrome.org> * refactor(plugins): streamline plugin function signatures and error handling Signed-off-by: Deluan <deluan@navidrome.org> * refactor(plugins): update scrobbler interface to return errors directly instead of response structs Signed-off-by: Deluan <deluan@navidrome.org> * test: make all test plugins use the PDK Signed-off-by: Deluan <deluan@navidrome.org> * refactor(plugins): reorganize and sort type definitions for consistency Signed-off-by: Deluan <deluan@navidrome.org> * refactor(plugins): update error handling for methods to return errors directly Signed-off-by: Deluan <deluan@navidrome.org> * refactor(plugins): update function signatures to return values directly instead of response structs Signed-off-by: Deluan <deluan@navidrome.org> * refactor(plugins): update request/response types to use private naming conventions Signed-off-by: Deluan <deluan@navidrome.org> * build: mark .wasm files as intermediate for cleanup after building .ndp Signed-off-by: Deluan <deluan@navidrome.org> * refactor(plugins): consolidate PDK module path and update Go version to 1.25 Signed-off-by: Deluan <deluan@navidrome.org> * feat: implement Rust PDK Signed-off-by: Deluan <deluan@navidrome.org> * refactor(plugins): reorganize Rust output structure to follow standard conventions Signed-off-by: Deluan <deluan@navidrome.org> * refactor(plugins): update Discord Rich Presence and Library Inspector plugins to use nd-pdk for service calls and implement lifecycle management Signed-off-by: Deluan <deluan@navidrome.org> * refactor(plugins): update macro names for websocket and metadata registration to improve clarity and consistency Signed-off-by: Deluan <deluan@navidrome.org> * refactor(plugins): rename scheduler callback methods for consistency and clarity Signed-off-by: Deluan <deluan@navidrome.org> * refactor(plugins): update export wrappers to use `//go:wasmexport` for WebAssembly compatibility Signed-off-by: Deluan <deluan@navidrome.org> * docs: update plugin registration docs Signed-off-by: Deluan <deluan@navidrome.org> * fix(plugins): generate host wrappers Signed-off-by: Deluan <deluan@navidrome.org> * test(plugins): conditionally run goleak checks based on CI environment Signed-off-by: Deluan <deluan@navidrome.org> * docs: update README to reflect changes in plugin import paths Signed-off-by: Deluan <deluan@navidrome.org> * refactor(plugins): update plugin instance creation to accept context for cancellation support Signed-off-by: Deluan <deluan@navidrome.org> * fix(plugins): update return types in metadata interfaces to use pointers Signed-off-by: Deluan <deluan@navidrome.org> * fix(plugins): enhance type handling for Rust and XTP output in capability generation Signed-off-by: Deluan <deluan@navidrome.org> * fix(plugins): update IsAuthorized method to return boolean instead of response object Signed-off-by: Deluan <deluan@navidrome.org> * test(plugins): add unit tests for rustOutputType and isPrimitiveRustType functions Signed-off-by: Deluan <deluan@navidrome.org> * feat(plugins): implement XTP JSONSchema validation for generated schemas Signed-off-by: Deluan <deluan@navidrome.org> * fix(plugins): update response types in testMetadataAgent methods to use pointers Signed-off-by: Deluan <deluan@navidrome.org> * docs: update Go and Rust plugin developer sections for clarity Signed-off-by: Deluan <deluan@navidrome.org> * docs: correct example link for library inspector in README Signed-off-by: Deluan <deluan@navidrome.org> * docs: clarify artwork URL generation capabilities in service descriptions Signed-off-by: Deluan <deluan@navidrome.org> * docs: update README to include Rust PDK crate information for plugin developers Signed-off-by: Deluan <deluan@navidrome.org> * fix: handle URL parsing errors and use atomic upsert in plugin repository Added proper error handling for url.Parse calls in PublicURL and AbsoluteURL functions. When parsing fails, PublicURL now falls back to AbsoluteURL, and AbsoluteURL logs the error and returns an empty string, preventing malformed URLs from being generated. Replaced the non-atomic UPDATE-then-INSERT pattern in plugin repository Put method with a single atomic INSERT ... ON CONFLICT statement. This eliminates potential race conditions and improves consistency with the upsert pattern already used in host_kvstore.go. * feat: implement mock service instances for non-WASM builds using testify/mock Signed-off-by: Deluan <deluan@navidrome.org> * refactor: Discord RPC struct to encapsulate WebSocket logic Signed-off-by: Deluan <deluan@navidrome.org> * feat: add support for experimental WebAssembly threads Signed-off-by: Deluan <deluan@navidrome.org> * feat: add PDK abstraction layer with mock support for non-WASM builds Signed-off-by: Deluan <deluan@navidrome.org> * feat: add unit tests for Discord plugin and RPC functionality Signed-off-by: Deluan <deluan@navidrome.org> * fix: update return types in minimalPlugin and wikimediaPlugin methods to use pointers Signed-off-by: Deluan <deluan@navidrome.org> * fix: context cancellation and implement WebSocket callback timeout for improved error handling Signed-off-by: Deluan <deluan@navidrome.org> * feat: conditionally include error handling in generated client code templates Signed-off-by: Deluan <deluan@navidrome.org> * feat: implement ConfigService for plugin configuration management Signed-off-by: Deluan <deluan@navidrome.org> * feat: enhance plugin manager to support metrics recording Signed-off-by: Deluan <deluan@navidrome.org> * refactor: make MockPDK private Signed-off-by: Deluan <deluan@navidrome.org> * refactor: update interface types to use 'any' in plugin repository methods Signed-off-by: Deluan <deluan@navidrome.org> * refactor: rename List method to Keys for clarity in configuration management Signed-off-by: Deluan <deluan@navidrome.org> * test: add ndpgen plugin tests in the pipeline and update Makefile Signed-off-by: Deluan <deluan@navidrome.org> * feat: add users permission management to plugin system Signed-off-by: Deluan <deluan@navidrome.org> * refactor: streamline users integration tests and enhance plugin user management Signed-off-by: Deluan <deluan@navidrome.org> * refactor: remove UserID from scrobbler request structure Signed-off-by: Deluan <deluan@navidrome.org> * test: add integration tests for UsersService enable gate behavior Signed-off-by: Deluan <deluan@navidrome.org> * feat: implement user permissions for SubsonicAPI and scrobbler plugins Signed-off-by: Deluan <deluan@navidrome.org> * fix: show proper error in the UI when enabling a plugin fails Signed-off-by: Deluan <deluan@navidrome.org> * feat: add library permission management to plugin system Signed-off-by: Deluan <deluan@navidrome.org> * feat: add user permission for processing scrobbles in Discord Rich Presence plugin Signed-off-by: Deluan <deluan@navidrome.org> * fix: implement dynamic loading for buffered scrobbler plugins Signed-off-by: Deluan <deluan@navidrome.org> * feat: add GetAdmins method to retrieve admin users from the plugin Signed-off-by: Deluan <deluan@navidrome.org> * feat: update Portuguese translations for user and library permissions Signed-off-by: Deluan <deluan@navidrome.org> * reorder migrations Signed-off-by: Deluan <deluan@navidrome.org> * fix: remove unnecessary bulkActionButtons prop from PluginList component * feat: add manual plugin rescan functionality and corresponding UI action Signed-off-by: Deluan <deluan@navidrome.org> * feat: implement user/library and plugin management integration with cleanup on deletion Signed-off-by: Deluan <deluan@navidrome.org> * feat: replace core mock services with test-specific implementations to avoid import cycles * feat: add ID fields to Artist and Song structs and enhance track loading logic by prioritizing ID matches Signed-off-by: Deluan <deluan@navidrome.org> * feat: update plugin permissions from allowedHosts to requiredHosts for better clarity and consistency * feat: refactor plugin host permissions to use RequiredHosts directly for improved clarity * fix: don't record metrics for plugin calls that aren't implemented at all Signed-off-by: Deluan <deluan@navidrome.org> * fix: enhance connection management with improved error handling and cleanup logic Signed-off-by: Deluan <deluan@navidrome.org> * feat: introduce ArtistRef struct for better artist representation and update track metadata handling Signed-off-by: Deluan <deluan@navidrome.org> * feat: update user configuration handling to use user key prefix for improved clarity Signed-off-by: Deluan <deluan@navidrome.org> * feat: enhance ConfigCard input fields with multiline support and vertical resizing Signed-off-by: Deluan <deluan@navidrome.org> * fix: rust plugin compilation error Signed-off-by: Deluan <deluan@navidrome.org> * feat: implement IsOptionPattern method for better return type handling in Rust PDK generation Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org> |