mirror of
https://github.com/navidrome/navidrome.git
synced 2026-08-31 07:30:32 +00:00
5053 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Deluan
|
d7ca00d018 |
chore(deps): update fscache fork to the CancelWithErr simplification
stream v1.5.0 added CancelWithErr, which delivers a cancellation cause to blocked reads, future reads, and NextReader. The fscache fork now delegates CloseWithError to it, dropping its own cause recording and reader wrappers. Behavior is unchanged on the Navidrome side. |
||
|
Deluan Quintão
|
4b60b21316
|
fix(scanner): read file birth time via statx on Linux (#6046)
* fix(scanner): read file birth time via statx on Linux On Linux the file birth time is only reachable through statx(2). We were reading it with times.Get(), which looks only at the plain stat() result, where the field does not exist: djherbis/times declares HasBirthTime=false for Linux, so the check was always false and every file fell back to time.Now(). This has been the case since #2553 introduced the feature, which means that PR was a no-op on Linux from day one. macOS and Windows were never affected, as there the birth time does come back from plain stat. BirthTime() now tries times.Get() first, which costs no syscall and is already correct on macOS, Windows and BSD, and only falls back to times.Stat() on the path when that comes back empty. Ordering matters: on Windows times.Stat() opens the file asking for FILE_WRITE_ATTRIBUTES, which fails on a read-only share before falling back. Not every filesystem stores a birth time. Measured with a probe over real mounts: ext4, SMB/CIFS and mergerfs report one, while NFS and rclone/FUSE never do. Asking those on every file is pure overhead, so a miss is remembered per device on the localFS and skipped from then on. The memo is keyed by device rather than by library, so a library spanning two mounts does not lose birth times on the mount that does support them. Cost of the extra call is ~2us per file against ~52us just to open a file for tag reading, so 0.23s across a 97k-file library, and only for files whose tags are actually read. Existing rows keep their current birth_time: the repository drops that column on update, so only newly added files get the real value. * fix(scanner): return the device id opaquely to satisfy unconvert st.Dev is uint64 on Linux and int32 on darwin, so a uint64() cast is redundant on one and required on the other. Returning it as an opaque value drops the cast entirely, which also removes the gosec suppression that came with it. The value is only ever used as a sync.Map key. |
||
|
Deluan
|
b5f530e90c |
chore(deps): update Go dependencies to latest versions
Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
Deluan
|
b0e1943d8b |
fix(ui): always show the Last.fm link on the artist details page
The button only rendered when an agent supplied a real last.fm URL, either embedded in the biography or as artistInfo.lastFmUrl. Neither source is reliable anymore: cleanContent strips the "Read more on Last.fm" anchor out of the biography, and the Last.fm agent does not register at all unless LastFM.ApiKey and LastFM.Secret are set, in which case GetArtistURL falls through to ListenBrainz, which returns the artist's official homepage. The isLastFmURL guard then correctly rejects it and the button disappears. Build the URL from the artist name when no canonical one is available, the same way AlbumExternalLinks already does for albums. A real last.fm URL is still preferred when one is present, and the button stays hidden when Last.fm is disabled or the artist has no name. |
||
|
Deluan
|
23e4c8f580 |
refactor(plugins): build the host HTTP client with httpclient.New
CheckRedirect is set on the returned client, so the plugin service no longer hand-builds an http.Client just to attach the shared transport. |
||
|
Deluan
|
a9962ebe5d |
refactor(artwork): use the shared httpclient for image downloads
Same behavior: httpclient.New sets the Navidrome User-Agent via its transport, so the manual header is no longer needed. |
||
|
Deluan Quintão
|
f08b5297ee
|
feat(ui): add Refresh Metadata to the album and artist context menus (#6036)
* refactor(artwork): move artworkItemName into core/artwork as ItemName * feat(external): add RefreshInfo to force an external info refresh RefreshInfo re-fetches and re-saves external info for one artist or album, bypassing the TTL check that UpdateArtistInfo/UpdateAlbumInfo use. It is synchronous; callers that must not block detach it themselves. Also makes MockArtistRepo/MockAlbumRepo.UpdateExternalInfo persist to Data (previously a no-op) and adds the new method to the e2e noopProvider, both required so the interface addition compiles and is observable in tests. * feat(external): broadcast RefreshResource after external info is saved populateArtistInfo and populateAlbumInfo now emit the same RefreshResource event the artwork worker uses, so the UI learns about both foreground and background metadata refreshes. * feat(nativeapi): replace artwork refresh endpoint with metadata refresh * feat(ui): add refreshMetadata to the data provider * feat(ui): add a Refresh Metadata item to the album and artist context menus * fix(ui): re-fetch artist info when the record is refreshed * test: fix mislabeled spec, add kind-gate negative case, guard nil mock maps - Rename the RefreshInfo spec that claimed to cover the save-failure/broadcast path: SetError(true) fails Get too, so it only proves RefreshInfo bails out early at getArtist. - Add a spec proving playlist refreshes skip the external-info step, since that asymmetry (al/ar only) was documented but unasserted. - Add lazy nil-map init to MockAlbumRepo/MockArtistRepo.UpdateExternalInfo so a composite-literal-constructed mock doesn't panic on first save. * test: relocate discArtworkName specs from cmd to core/artwork artworkItemName moved into core/artwork as ItemName in an earlier commit, but its disc-name specs stayed behind in cmd/artwork_test.go, reaching across packages. Move them to core/artwork/item_name_test.go where the code now lives. * fix(ui): shape refreshMetadata like a react-admin response react-admin validates custom dataProvider methods and rejects any response without a `data` key, so the raw httpClient promise made every click surface an error toast instead of the success message. The unit test mocked useDataProvider, which skips that validation. Also folds "which kinds have external info" into external.HasInfo so the handler stops restating it, drops the nil-broker guard that only existed for tests, and delegates the mocks' UpdateExternalInfo to Put. * refactor(external): unexport infoKinds Only HasInfo is used outside the package, so the slice itself does not need to be exported. * refactor(artwork): fold ItemName into housekeeping.go next to Refresh ItemName exists to guard Refresh from ids that would orphan a queue row, and both callers invoke them back to back. A separate file hid that pairing; it was only split out to keep the move out of cmd/ legible in review. * fix(nativeapi): return 500 when the refresh lookup fails for a non-ErrNotFound reason A transient repository error told the admin the id did not exist, and the error was dropped without a log line, so nothing pointed at the real cause. Also drops the inherited claim that clearing artwork state shows a placeholder. Reads fall back to local resolution, so that only holds when there is no local art. * fix(ui): move Refresh Metadata above Get Info in the context menu Menu order follows key insertion order in the options object, so the new spec pins the position rather than leaving it to be shuffled by the next addition. |
||
|
Deluan Quintão
|
97da9993d7
|
fix(stream): abort the response when a transcoded stream is truncated (#6035)
* fix(stream): abort the response when a transcoded stream is truncated When a transcode failed after some audio had already been sent, Serve logged the error and returned nil, so Go finished the chunked body normally and the client received an apparently complete, silently short file. Symfonium users hit this on large offline syncs, and the worst path, ffmpeg dying mid-write behind the transcoding cache, produced no error and nothing in the log above Debug: the cache writer was closed plainly, so readers drained the truncated entry to a clean EOF. The root cause of that silence is an fscache limitation: Close is the only way to end a cache write, and Close always means "complete". This adopts the deluan/fscache fork, which adds CloseWithError: on failure copyAndClose now cancels the entry with the cause, so every attached reader fails mid-read with the real error instead of EOF, a late Get for the entry is refused, and the entry never reports a final size. The error travels inside the entry each reader holds, which makes per-generation delivery automatic and needs no bookkeeping on our side. With the failure arriving in-band, one change in Serve covers every mode: an io.Copy error after bytes are on the wire panics with http.ErrAbortHandler. Go aborts the response without the terminating chunk (RST_STREAM on HTTP/2), chi's Recoverer re-panics that value, and the deferred stream.Close() still runs, so the transcode limiter slot is released as before. Two behaviors improve as side effects. A transcoder that dies before its first byte now yields a Subsonic error response instead of a 200 with an empty body, since the failure reaches Serve as an error while the status is still unsent; genuinely empty output (clean EOF, exit 0) keeps the 200. And a failed entry's invalidation no longer defers its unlink past a replacement entry re-creating the same file, because canceling already closed its readers. * fix(cache): warn when the cache writer cannot report failures to readers The CloseWithError capability comes from the fscache fork via a go.mod replace directive, and a type assertion picks it up. If that directive is ever lost, the assertion fails silently, readers of a dead writer go back to draining a truncated entry to a clean EOF, and nothing says so. Two layers against that: a warning on the failure path when the writer lacks the capability, and a test that asserts the writer fscache returns carries it, so losing the fork fails CI instead of a listener's download. * build: point the fscache replace at the fork's master deluan/fscache#1 is merged; pin the merge commit instead of the review branch. Pinned by sha because the module proxy still resolves the fork's master ref to its pre-merge commit. * build: reference the upstream fscache PR in the replace comment The replace itself must keep pointing at the fork: the commit only exists in djherbis/fscache under refs/pull/22/head, which the Go module fetcher cannot resolve (verified: unknown revision for both short and full sha). The same commit is advertised on the fork's master, so that is the fetchable source. |
||
|
Deluan Quintão
|
cb0a6cedd6
|
fix(scanner): keep album tag order from the files instead of alphabetical (#5872)
Album-level tags were ordered by frequency and then alphabetically by value. Album.Genre is just the first genre in that list, so any album whose genres tie on frequency, which is the normal case, displayed the alphabetically first genre rather than the first one in the file. A file tagged "Native American New Age; Indigenous American Traditional Music; Ambient" showed up as "Ambient". Break frequency ties on order of appearance instead. This affects all album-level tags, so mood tagged "Happy; Chill" now keeps that order too. MediaFiles.ToAlbum already sorts the files by path before flattening their tags, so the aggregated order stays deterministic across scans. Only album genre was affected; media_file tags already preserved file order. |
||
|
Rob Emery
|
3da2b590e7
|
fix: add Navidrome UserAgent in all outgoing requests (#6020)
* There has been a report about navidrome hitting listenbrainz hard
and the listenbrainz guys wanting to be able to distinguish navidrome
* feat: apply Navidrome User-Agent to all outgoing HTTP requests
Add utils/httpclient, a shared http.Client factory whose transport sets
the User-Agent header (Navidrome/{version} - https://github.com/navidrome)
on any request that does not already have one, and use it at every place
the server builds an HTTP client: Last.fm, ListenBrainz and Deezer agents
and auth routers, insights collector, backgrounds handler, and the plugin
host HTTP service. Plugin-set User-Agent values are preserved. The
per-request header lines from the previous commit are superseded by the
transport.
---------
Co-authored-by: Deluan <deluan@navidrome.org>
|
||
|
Deluan
|
82b9a44a1f |
fix(log): redact sensitive auth headers from request logs
The trace-level request log dumps all headers as a JSON blob, but the redaction hook only had query-param patterns, so Authorization, X-Emby-Token, X-MediaBrowser-Token and X-Nd-Authorization leaked their tokens in plaintext. Add one pattern that blanks those header value arrays at the log sink. |
||
|
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
|
fc9d93d22a
|
fix(plugins): read the loaded plugin from a local, not the shared map (#6014)
Plugins load concurrently through an errgroup. loadPluginWithConfig wrote m.plugins under m.mu but read it back unlocked to pass to callPluginInit, so one goroutine's write raced another's read. Caught by -race on master (run 32608293134): all 640 specs passed, the job failed only on the race. Capture the pointer while holding the lock and use the local. Holding m.mu across callPluginInit would be wrong, since that runs arbitrary plugin code. |
||
|
Deluan
|
3cb9850872 |
feat(artwork): extend stale absent age to 30 days and update test formatting
Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
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
|
295886cb9a
|
feat(artwork): report what a config-fingerprint backfill enqueued (#6010)
* feat(artwork): report what a config-fingerprint backfill enqueued A backfill re-resolves every entity, and on a large library that is tens of thousands of external agent calls. It announced itself with a single line carrying nothing but an elapsed time, so the size of the job was invisible until the request volume showed up hours later. Log the item count, the per-kind breakdown, and a ceiling on the external lookups the queued work can cost. The ceiling reuses ExternalLookupsPerItem, the same estimator behind the `artwork reprocess` preview, so the two agree on what an item can cost. backfill now returns a summary instead of a bare bool, which keeps the counts assertable without capturing log output. Worker.Backfill keeps its (bool, error) signature, so its caller is unchanged, and it reads the agent count off its own resolver. * refactor(artwork): share the image-agent count and take it lazily Counting image agents was written twice, once in the CLI for the `artwork reprocess` preview and again as a resolver method for the backfill log. Two copies of "which agents count as image agents" can drift, and the CLI estimate and the server log would then disagree silently. Move the derivation next to the type it builds, as NewImageAgentCount, and call it from both. The resolver method goes away with it: hanging the census on the resolver forced two nil guards that its only caller could never trigger, because Worker always builds a resolver with agents. The Worker keeps the *agents.Agents it is already handed instead of reaching through the processor and resolver to find it. Pass the count as a func. Building the agent list constructs every enabled agent (each one an HTTP client and a cache goroutine) only to take its length, and a backfill returns early on an unchanged fingerprint, which is what happens on nearly every restart. * docs(artwork): say what the backfill lookup estimate does not bound The comment called the number a ceiling, which the CLI comment on the same estimate already contradicts: externalEstimate "claims no bound". Both are right about the local-source case and only one of them mentions that a retried item asks its agents again. |
||
|
Deluan Quintão
|
07b6411c0b
|
perf(artwork): cap the stale-absent recheck at 100 items per kind per hour (#6007)
* feat(artwork): drip the stale-absent recheck instead of bursting it daily Each hourly housekeeping tick now re-queues at most 100 absent states per kind, oldest attempts first, instead of everything older than 24h at once. External agents see a flat ~100 requests/hour per agent instead of hourly bursts of ~2,000, and the effective recheck interval self-scales with the size of the absent pool (~4 days at 10k absent artists) while small libraries keep the 24h floor. * feat(artwork): trust an absent artwork state for a week before rechecking With the recheck now dripped at 100 items per kind per hour, the 24h floor only governed small libraries, where the drip cap never binds; they still re-asked every agent daily. A 7-day floor cuts that cost 7x and, for large libraries, becomes the binding limit over the drip cycle (~5.7k calls/day instead of ~9.6k at 10k absent artists). Among comparable servers, this is still the second-most-eager recheck: gonic retries misses every 30 days, Jellyfin and Funkwhale never do. * refactor(artwork): state the drip's backpressure contract where it bites Review follow-ups: the recheck limit deliberately caps the *selection*, not the insertions — already-queued rows use up budget, so a stalled drain admits no new work instead of building a recovery burst. Say so in the interface doc, mirror it in the mock by truncating the sorted candidates (matching the SQL's LIMIT-before-ON CONFLICT), and teach `artwork status` and the worker doc the post-drip wording. Also pin the one cmd fixture that still assumed a 24h recheck window. |
||
|
Deluan Quintão
|
ffc68e29db
|
feat(cli): add artwork cancel to call off queued artwork work (#6006)
* feat(cli): add `artwork cancel` to call off queued artwork work A bulk backfill had no off switch. Changing an artwork setting bumps the config fingerprint, which enqueues every entity in the library, and the only way to stop it was to turn agents off -- which changes the fingerprint again and enqueues a second full backfill. The escape hatch was the trap. `artwork cancel` deletes pending queue rows selected by --kind and/or --priority, with the --dry-run/confirm/-y flow `reprocess` already uses. Cancelling by priority is the point: it drops a runaway backfill while leaving the bump-priority rows an operator queued by hand. It only touches the queue. Resolved artwork and the item_artwork state behind `artwork explain` are left alone, and the trace of why a cancelled item last failed goes with its row. Preserving that trace would mean writing it to last_failure, which `explain` prints under "Gave up after" -- reporting a cancellation as an exhausted retry budget. The help text says the trace is discarded instead. Two limits the help text states, because neither is guessable: work already dequeued is not interrupted, and an item with no artwork state yet can be queued again by the hourly missing-artwork recheck. Cancel calls off queued work; it does not stop the worker. --kind validates against RefreshableKinds, not the RecheckKinds `reprocess` uses: the queue holds media file rows, so --all has to reach them. PurgeQueued follows the repository's naming rule -- it finds its own rows and reports how many went -- and ignores retry_at, since a row still backing off is pending work. The preview reuses CountByKindAndPriority rather than adding a counter. reprocessConfirm became confirmUnlessYes(yes, in, verb) now that two commands prompt. * refactor(cli): share the artwork queue filter between the preview and the delete Follow-up cleanup on the previous commit; no change to what the command does, apart from --all, noted below. The "which rows does cancel touch" predicate was written three times: once as SQL in PurgeQueued, once in Go in cmd's matchingQueueStats, and once more in the mock. The preview and the delete could therefore drift, and the mock would keep the tests green while they did. persistence now has one artworkQueueFilter, shared by PurgeQueued and a new CountQueued, and cmd does no filtering at all. That also makes the preview cheaper. It counted the whole queue and filtered in Go, so `artwork cancel --kind al` scanned every row of every kind to print a handful. CountQueued pushes the filter into SQL, which the drain index serves as a range seek. CountByKindAndPriority is gone: it is CountQueued(nil, nil). --all now selects with an empty filter instead of enumerating RefreshableKinds. It is what the flag help already claimed, and the enumeration was narrower than its own documentation -- a queue row whose item_kind this build does not know survived `--all` with no flag combination able to remove it. It also restores SQLite's truncate path: measured with EXPLAIN QUERY PLAN, a bare DELETE plans to nothing, while `WHERE (1=1)` -- which an empty squirrel And renders -- plans to a full index scan. A test pins the filter's emptiness so that cannot regress silently. Also folded together three copies of the parse-and-dedup loop (parseAll), two copies of the queue-stats table (printQueueStats, now shared with `artwork status`), two copies of the stat sum (queueTotal), and four copies of the kind-to-prefix mapping (model.KindPrefixes). The PurgeQueued specs became one DescribeTable that asserts count and delete agree on every selection. * docs(cli): say when `artwork cancel` evaluates its selection The help text covered the two limits that surprise an operator after the fact, but not the one that bites during the prompt: the count is a preview, and the filters run again on confirm. A scan or a manual refresh landing in between is cancelled without ever appearing in the table the operator agreed to. Deleting only the previewed rows was considered and rejected. The exposure is one item re-resolving on next view instead of immediately: clearing an item's artwork state is what every recovery path selects on, so a lost Bump row from artwork.Refresh comes back at the same priority via provisional() on the next request, and otherwise within the hour via EnqueueAllMissing. Buying a guarantee against that costs the truncate path on --all, the flag that exists for a 29k-item backfill. * refactor(cli): share one set of flag targets across the artwork subcommands reprocess and cancel each declared their own kinds/all/dry-run/yes variables, but cobra only ever parses the one subcommand being run, so the two sets could never hold values at the same time. backup.go already binds one backupDir across two subcommands and one force across two more; this follows that. Ten package-level variables become six. Each command keeps its own help string and its own valid-kind list, so --kind still reports RecheckKinds for reprocess and RefreshableKinds for cancel, and --source and --priority stay registered only on the command that has them. The priority lookup table is now knownPriorities, freeing the artworkPriorities name for the flag. The new name also reads better against priorityName's fallback for a value it does not know. |
||
|
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
|
0a55cc8caf
|
docs(plugins): document how a metadata agent signals "not found" (#6001)
A MetadataAgent plugin reports "I have no data for this item" by returning an empty response with a nil error. Any error it returns instead is treated as a plugin fault and retried with backoff. That rule was not documented anywhere, so an author naturally returns an error for a missing item, and Navidrome then retries every item the plugin's source does not cover. This is not hypothetical: the artist-nfo-metadata plugin returned an error for every artist without an artist.nfo, which kept those artists in the artwork retry queue for hours and tripped the artwork circuit breaker for the plugin as a whole. Document the rule on the capability interface, which ndpgen copies into the Go PDK, and in the MetadataAgent section of the plugin README. |
||
|
Deluan Quintão
|
17db7d4077
|
ci: pull base images through mirror.gcr.io instead of ECR Public (#5997)
CI builds were failing at random with:
buildx failed with: toomanyrequests: Rate exceeded
The 429 comes from public.ecr.aws, not Docker Hub. AWS caps unauthenticated
ECR Public pulls at 1 per second per source IP (authenticated: 10/s). The
build matrix starts 11 jobs at once and each resolves 4 base images, so
roughly 44 anonymous pulls land in a couple of seconds — from GitHub runner
IPs that are shared with every other GitHub customer.
Measured against public.ecr.aws with an anonymous token, 60 requests at
concurrency 30 returned 31x 429 in 0.48s.
The same probe against mirror.gcr.io (Google's Docker Hub pull-through
cache) returned zero errors: 750 manifest requests up to ~141 req/s, plus
120 layer blob requests at concurrency 60. Google publishes no rate limit
for it, so this is measured headroom, not a contract — but it is roughly
10x the pipeline's peak rate, and cached pulls do not count against Docker
Hub's limits either.
Authenticating to ECR Public was the alternative. It was rejected because
10 pulls/s is still under the ~44-pull burst, it needs an AWS account plus
a secret, and secrets never reach fork pull requests — so forks would keep
failing. The mirror fixes forks too.
Verified buildkit honours the mirror block by routing a build through a
local logging registry: all 6 requests (manifests and blobs) hit the mirror,
none went to Docker Hub directly. Confirmed mirror.gcr.io answers 200 for
buildkit's "?ns=docker.io" query form on all four images, for both GET and
HEAD. Confirmed buildkit falls back to Docker Hub when the mirror is
unreachable — the build still succeeds, but the resolve takes ~30s instead
of ~0.3s, so a mirror outage means slow builds, not broken ones. The
existing Docker Hub login covers that fallback on main-repo runs.
msitools.dockerfile is only used by the local `make docker-msi` target, but
is switched over too so no ECR Public reference is left behind.
|
||
|
Deluan Quintão
|
fc1c1366dc
|
fix(cli): write pls -p playlist output to stdout (#5996)
The export path used the `println` builtin, which writes to stderr, so `navidrome pls -p X > playlist.m3u8` produced an empty file while the M3U body was interleaved with the startup logs on stderr. `println` also appended a newline that `ToM3U8` already provides, so the piped output had a stray trailing blank line that `-o file` did not. Both destinations are now byte-identical. The stdout/file choice moved into a `writePlaylist` helper shared by `pls -p` and `pls export -p`, which both had the same bug. It takes the destination as an `io.Writer`, matching the existing convention in cmd/artwork.go. |
||
|
Daniel Barrientos Anariba
|
3d3c3ed601
|
ci: lint with the golangci-lint version the Makefile declares (#5994)
* ci: lint with the golangci-lint version the Makefile declares The workflow asked for `version: latest` while the Makefile pins `GOLANGCI_LINT_VERSION ?= v2.12.0`, so `make lint` and CI ran different linters. golangci-lint v2.13.0 started reporting G404 on three existing `rand.Shuffle` calls, which turned every PR red without a line of Go changing. Read the version from the Makefile instead of resolving `latest`, so the two stay in step and a new release cannot break unchanged code. * chore: re-run CI |
||
|
Deluan Quintão
|
881073c183
|
feat(cli): let artwork explain/refresh accept an id without its kind (#5988)
* feat(cmd): make artwork explain/refresh accept an id without its kind The kind can now come from the id itself: a full artwork id (al-<id>) carries it in the prefix, and a bare id is resolved across tables via GetEntityByID. The explicit <kind> <id> leader still works. * fix(cmd): keep refreshing resolvable ids when others fail to resolve resolveArtworkTargets now collects a self-describing id it cannot resolve as a failure instead of aborting, so refresh reports and skips the bad ones and still queues the rest, matching refreshItems per-item behavior. explain stays strict and rejects any unresolved input. |
||
|
Deluan Quintão
|
fd4b3256e4
|
perf(artwork): compute the blurhash DCT separably (#5989)
The cosine basis factors into cosX[i][x] * cosY[j][y], so the pixel loop does not need to visit every (i,j) pair. Each row now collapses to xComp dot products, folded over yComp once per row: w*h*xComp + h*xComp*yComp multiply-accumulates instead of w*h*xComp*yComp. Encoding is ~60% faster at every input size, and ~80% faster at the 128px size the artwork pipeline actually feeds it (263us -> 53us). Hashes are byte-identical, so the existing golden-value specs cover the rewrite. |
||
|
Junker der Provinz
|
dff9e47c2e
|
fix(scanner): detect in-place playlist edits via the folder content hash (#5914)
* fix(scanner): detect in-place playlist edits via the folder content hash Signed-off-by: junkerderprovinz <jdp@braethoria.com> * docs(scanner): clarify the playlist entries in the folder hash Shorten the comment on the playlist loop, and record why the playlist count stays in the hash header: it is redundant with the loop for change detection, but removing it changes the hashed byte stream for every folder, including folders without playlists, which would mark every folder outdated on the first scan after upgrade. Signed-off-by: junkerderprovinz <jdp@braethoria.com> * test(scanner): pin filename and size into the playlist hash assertions The playlist size test called time.Now() twice, so the modtime differed too and carried the assertion — dropping info.Size() from the hash left the suite green. It now shares one baseTime. A new rename test swaps the map key with count, size and modtime held constant, so dropping the filename from the hash fails. Both mutations were verified to fail before this change and pass after. --------- Signed-off-by: junkerderprovinz <jdp@braethoria.com> Co-authored-by: Deluan <deluan@navidrome.org> |
||
|
Junker der Provinz
|
c362519f76
|
test: unskip path-separator tests on Windows (#5381) (#5916)
* test: unskip AbsolutePath and i18n path-separator tests on Windows (#5381) Signed-off-by: junkerderprovinz <jdp@braethoria.com> * test: unskip metadata folder-PID test on Windows via path.Dir (#5381) Signed-off-by: junkerderprovinz <jdp@braethoria.com> * test(storage): make relative-folder assertion cross-platform and unskip on Windows (#5381) Signed-off-by: junkerderprovinz <jdp@braethoria.com> * fix(persistence): normalize folder-update-info paths with forward slashes on Windows (#5381) Signed-off-by: junkerderprovinz <jdp@braethoria.com> * review: drop folder-PID change, trim storage_test comment (#5381) Revert model/metadata/persistent_ids.go to master: switching the `folder` PID attribute from filepath.Dir to path.Dir would change the persistent IDs of existing Windows libraries and needs a migration path, so it is out of scope for this PR. The matching test unskip is reverted with it, leaving #TBD-path-sep-metadata open in #5381. Trim the core/storage/storage_test.go comment to two lines. Signed-off-by: junkerderprovinz <jdp@braethoria.com> --------- Signed-off-by: junkerderprovinz <jdp@braethoria.com> Co-authored-by: Deluan Quintão <deluan@navidrome.org> |
||
|
Deluan
|
bd6b7a6686 | test: increase timeout for cache availability checks to 10 seconds | ||
|
Deluan
|
6d8a3e48ee |
refactor: replace md5 with xxh3 for faster and more efficient hashing
Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
Deluan
|
2dab4b4048 |
chore: remove redundant comment
Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
Deluan Quintão
|
1f3034f022
|
fix(playlist): block track edits on synced playlists across all APIs (#5984)
* fix(playlist): block track edits on synced playlists across all APIs A synced playlist's tracks come from its source file, so any track edit made through the UI or an API was silently reverted on the next scan. Track mutations funnel through two service guards, checkTracksEditable (incremental edits) and Create (wholesale replace, used by Subsonic createPlaylist and Jellyfin's replace path), which each duplicated the smart-playlist check. Both now consult a shared model.Playlist.TracksEditable() predicate, so the native, Subsonic, and Jellyfin paths are all locked: track edits return ErrNotAuthorized (403, or Subsonic error 50) instead of being accepted and lost. Metadata-only edits (name, comment, public, the sync flag itself) still go through checkWritable and are unaffected. In the UI, a synced playlist's track list becomes read-only, mirroring how smart playlists already behave. * fix(playlist): return 409 Conflict for non-editable playlist track edits The previous commit rejected track edits on smart and synced playlists with ErrNotAuthorized (403). That conflates two different things: a 403 says the caller lacks permission, but a synced or smart playlist's tracks are immutable for everyone, including the owner and admins. It is a property of the resource, not the caller. Introduce ErrPlaylistNotEditable and return it from both track-edit guards. The Native and Jellyfin APIs now map it to 409 Conflict; Subsonic maps it to error 50, the closest code it has (it has no read-only concept). The Native track handlers previously mapped this rejection inconsistently (400 on add, 500 on remove, 403 on reorder) through a new shared writePlaylistError helper. Genuine authorization failures (non-owner, non-admin) still return ErrNotAuthorized. * fix(playlist): surface synced read-only state in picker, Jellyfin, and OpenSubsonic Follow-up to the track-edit lock: the read-only state was enforced but not advertised consistently, so clients still offered edits that the server rejects. - UI: the Add to Playlist picker filtered targets by isWritable only, offering synced playlists that then 409 on add. It now filters with canChangeTracks. - Jellyfin: addToPlaylist/removeFromPlaylist hard-coded every error to 404, so a locked playlist reported "not found" instead of 409. They now return 409 for ErrPlaylistNotEditable while keeping the deliberate anti-probing 404 for every other error (a non-owner never reaches ErrPlaylistNotEditable, so 409 leaks nothing). - OpenSubsonic: buildOSPlaylist marked only smart playlists readonly; owned synced playlists advertised readonly=false. Readonly now also covers !TracksEditable(), matching the existing smart-playlist treatment. * fix(jellyfin): report CanEdit from playlist editability in permission probes getPlaylistUsers and getPlaylistUser returned CanEdit: true unconditionally, so Finamp (which probes this before showing edit controls) offered track editing on synced/smart playlists whose add/remove requests now return 409. Both handlers now fetch the playlist and set CanEdit from TracksEditable(), keeping the deliberate non-owner looseness (CanEdit stays true for a normal playlist a non-owner views) and mapping any lookup error to 404 like the sibling probes. * fix(playlist): check ownership before editability when replacing tracks Create checked TracksEditable() before ownership, so a non-owner replacing another user's public smart/synced playlist (Jellyfin updatePlaylist with a non-empty Ids list) received a 409 read-only conflict instead of a 403 authorization failure. The incremental guards check ownership first via checkWritable; Create now matches that order. Subsonic is unaffected (both errors map to code 50). Owners of their own smart/synced playlists still get the read-only conflict. * fix(jellyfin): return 403 for locked playlists, matching Jellyfin Jellyfin itself refuses edits on its file-backed playlists with Forbid() (403): PlaylistsController gates every mutation on OwnerUserId == caller or a share with CanEdit, and playlists imported from .m3u files satisfy neither. Its CanEdit is an ACL field, not a read-only marker, and Jellyfin core has no server-managed playlist type at all. Our Jellyfin routes exist to imitate that API, so ErrPlaylistNotEditable now maps to 403 there instead of 409. The native API keeps 409 (a resource-state conflict is the accurate REST answer where we define the contract) and Subsonic keeps error 50, its closest code. * chore(playlist): trim comments added by this branch Several comments ran to three or four lines and carried rationale that belongs in the commit history rather than the code: what Jellyfin does with its own file-backed playlists, and restatements of the expressions directly below them. Each block is now one or two lines covering only the non-obvious why. |
||
|
Deluan Quintão
|
7a11ca69bb
|
fix(jellyfin): honor the Filters, SortBy and MaxHeight params clients actually send (#5981)
* fix(jellyfin): honor Filters=IsFavorite on /Artists and /Artists/AlbumArtists listArtistsByRole hand-built its itemsQuery and never set favOnly, so the favorites filter was silently dropped on both artist routes while /Items honored it. Finamp's home screen asks for favorite artists once per load and was served the entire artist list instead: 10,298 artists, 6.15 MB, 2.7s on a real library, and the wrong data on screen. Extract the favOnly parsing that parseItemsQuery already did into parseFavOnly and use it in both places. listArtists now adds the starred predicate to notMissing rather than replacing it, matching listAlbums and listSongs, so a favorite artist whose files are gone stays excluded. * fix(jellyfin): map SortBy=Runtime to duration for albums and songs sortColumnsByType had no runtime/runtimeticks key for any type, so Finamp's "Duration" sort silently misbehaved in two different ways. Albums: Finamp sends a bare SortBy=Runtime. Nothing matched, opts.Sort stayed empty, and applyOptions skips OrderBy entirely when Sort is empty — so the query ran with no ORDER BY at all and Ascending and Descending returned identical lists. Songs: Finamp sends SortBy=Runtime,AlbumArtist,Album,SortName. applySort takes the first *recognized* key, so Runtime was skipped and the list came back sorted by album artist while looking correct. Both repos already accept a duration sort (mediafile_repository maps it explicitly; album_repository falls through to the column name), so no migration is needed. Sorting 97k songs by duration costs a temp B-tree (~114ms on a prod-sized copy) — the same cost the Subsonic and UI duration sorts already pay, and correct where the previous behaviour was merely fast. * fix(jellyfin): apply the played/unplayed filters and MaxHeight image bound Filters was matched with a substring test for IsFavorite, so every other token Jellyfin defines was silently dropped and the response kept rows it should have excluded. Finamp sends Filters=IsUnplayed in normal use. Replace the bool with a parsed itemFilters carrying nullable favorite and played flags, so isFavorite=false and isPlayed=false are real filters rather than indistinguishable from an absent param. Standalone params are read first and the Filters list overrides them, the precedence real Jellyfin has. IsFavoriteOrLikes now maps to favorites deliberately instead of by substring accident; Likes, Dislikes, IsFolder, IsNotFolder and IsResumable have no Navidrome equivalent and are dropped rather than half-applied. The negative cases match NULL as well, since annotations are LEFT JOINed and an untouched item has no row. getItemImage read only maxwidth, so a client sending just MaxHeight got the full-size original: measured against a real cover, maxHeight=100 returned 82,570 bytes where maxWidth=100 returned 3,316. Use the tighter of the two bounds. * refactor(jellyfin): share the plain-param parser between /Items and /Artists listArtistsByRole hand-listed the itemsQuery fields it happened to need, which is exactly how the favorites filter went missing: the literal has been amended in four of the five commits that touched it. Extract listParams for the fields that come straight from query params so both paths read one parser, and the next supported param reaches every list path instead of only /Items. Also from the cleanup pass: collapse imageSize to a single clamped comparison and read its bounds through req.Params like the rest of the package, which drops the strconv import; build the artist and playlist filter lists with the flat append shape the album and song paths already use, instead of re-wrapping opts.Filters into a nested And per predicate; drop a nil guard in listPlaylists that no caller can reach, since both paths into queryItemsOfType build QueryOptions without Filters. applySort now logs when no SortBy key resolves at all — a miss inside a fallback list is normal, but none matching means a silently ignored sort, the failure mode that hid the Runtime bug. Its doc comment records why the remaining keys cannot simply be joined. Folds three duplicated test bodies into the tables that already parameterize them, and covers the artist-parent album branch, which reaches notMissing through filter.AlbumsByArtistID rather than the default branch. * docs(jellyfin): correct how applySort describes Jellyfin's SortBy semantics The comment claimed SortBy is a comma-separated fallback list. It is not: RequestHelpers.GetOrderBy (10.10) builds one (ItemSortBy, SortOrder) pair per key, so Jellyfin orders by every key in turn. Navidrome applies only the first recognized one, which is a real divergence — secondary keys never break ties — not the intended reading of the parameter. The assertion that the keys cannot be joined was also wrong. buildSortOrder does split its input on commas; what it maps is the whole string, so joining raw Jellyfin key names misses the mappings. Mapping each key first and joining the results would work, which makes multi-key sorting a real option rather than a blocked one. Documenting the current behaviour as a known divergence until then. * fix(jellyfin): order by every recognized SortBy key, not just the first Jellyfin orders by each SortBy key in turn, so "DatePlayed,SortName" means break ties by name. Navidrome applied only the first recognized key and dropped the rest, which is 28% of the sort traffic on a real server (23 of 82 requests in 12h carry 2-5 keys). Most were harmless because the primary key dominates, but PremiereDate,Album,ParentIndexNumber,IndexNumber,SortName came back unordered within a year. The keys cannot simply be joined: sortMapping keyed on the whole Sort string, so a joined value missed every mapping and fell through to raw column names. Make it resolve a comma list per part, but only when every part is a known key — the four existing callers that pass raw column lists (core/matcher, core/lyrics, core/maintenance, subsonic/browsing) all carry a part that is not a mapping key, several with their own direction, so they keep falling through exactly as before. Verified each one. applySort now collects every recognized key, skipping duplicates so ParentIndexNumber,IndexNumber does not repeat a column. random stays alone: the repo matches it by exact string equality, so joining it would both break that path and emit a bare 'random' column into the ORDER BY. Verified against a prod-sized copy: every multi-key combination seen in real traffic returns 200, and a secondary key now changes the order within a tied year for songs. Albums are unchanged there, because their max_year mapping already ended in ", name". * fix(persistence): resolve sort mappings exactly once Making sortMapping resolve a comma list per part broke an invariant it had been relying on: idempotence. sanitizeSort mapped the sort key up front and applyOptions then ran buildSortOrder over the result, so sortMapping was already being handed its own output. That was harmless only while a mapped value could never look like a key list. media_file's rated_at maps to "rating, rated_at", and both parts are keys, so the second pass expanded it to "rating, rating, rated_at". Found by round-tripping every mapping in all four repositories; it was the only collision, and the duplicate sort key was benign in SQL, but any future mapping of that shape would silently change meaning. sanitizeSort now validates without resolving, leaving buildSortOrder as the single mapping point. The generated SQL is unchanged — the whole suite passes apart from the two specs that asserted the old return value, which are updated and joined by a round-trip guard covering exactly the rated_at shape. Also use the paren-aware splitFunc that buildSortOrder already uses, so an expression carrying commas inside its parentheses cannot be split apart. * refactor(jellyfin,persistence): flatten the sort resolution paths Cleanup pass over the branch, no behavior change. sortMapping loses the len(parts)>1 guard, which existed only to pick between two identical toSnakeCase exits; the single-key case now falls through the same loop. lookupSortMapping hands back the snake_case form it had to derive so the fallback stops recomputing it — toSnakeCase is two regexps, and on a miss it was running twice per call. sanitizeSort now asks lookupSortMapping instead of probing the map itself, so "is this a known sort key" has one answer; the two had already drifted, since sanitizeSort tried one casing where the resolver tries three. applySort folds the nested random branch into the skip condition and the two trailing length tests into one switch. setSortMappings documents the invariant the comma-list rule depends on, where someone adding a mapping will read it. The README line describing SortBy still said only the first key applied, which the commit before last made false. Tests: the twelve near-identical sorting specs become one DescribeTable of (itemType, SortBy, want) triples, 124 lines to 36, and the applyOptions round-trip assertion collapses to the buildSortOrder call its sibling uses. * fix(jellyfin): keep annotation filters out of search, resolve sorts per part Two findings from the Codex review on #5981. The played/unplayed filters turned working requests into 500s when combined with SearchTerm. Search runs a two-phase FTS query whose first phase selects rowids with no annotation join, so a starred or play_count predicate there is "no such column", not a filter. Measured against master: MusicAlbum with SearchTerm and Filters=IsUnplayed went 200 -> 500, likewise IsPlayed and the Audio equivalents. listAlbums and listSongs now skip those predicates on the search path, matching what listArtists already did. That also clears the same 500 master already had for Filters=IsFavorite with SearchTerm. sortMapping resolved a comma list only while every part was a known key, so a list mixing a plain column with a mapped key kept neither: MusicAlbum SortBy=Runtime,SortName arrives as "duration, name", and duration is a plain album column, so name stayed raw instead of expanding to order_album_name. Albums whose name differs from its sort form — 1,366 of 6,987 on a real library — then ordered by the wrong secondary key, and PreferSortTags was ignored. Each part is now resolved on its own, which is what setSortMappings already documents for a single field. Verified every in-tree caller that passes a raw column list still produces its original ORDER BY. Codex also asked for the artist search path to apply the same filters. It would 500 for the reason above, and wrapping the library scope in a compound filter makes requestedLibraryIDs stop recognizing it, silently widening the search past the requested ParentId. * fix(jellyfin): honor the first SortOrder value for a multi-key sort applySort compared the whole SortOrder string with "Descending", so a per-key list like SortOrder=Descending,Ascending failed the match and every key, including the primary, sorted ascending — the exact opposite of the request. Take the first comma-separated value, which Jellyfin also uses for any key past the end of the SortOrder list. True per-key directions can't be expressed through the single opts.Sort string and are left out; no observed client sends a SortOrder list. |
||
|
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> |
||
|
Deluan Quintão
|
4b1218eec0
|
feat(ui): show translation completion percentage in the language selector (#5979)
The language selector now shows how complete each translation is, so users can see at a glance which languages are lagging behind English. The native API's translation resource gained a termCount field holding the number of non-empty terms in each language file; the UI divides that by the term count of the bundled English file to get the percentage. The percentage is wrapped in a Unicode left-to-right isolate, otherwise it renders as "(%61)" beside right-to-left names such as Arabic and Persian. Sorting runs on the plain language name, before the percentage is appended. This also fixes prepareLanguage() mutating the bundled English translations: for the English locale it received the shared en object and aliased albumSong and playlistTrack onto it, growing en by 94 keys at runtime. That inflated the denominator and made every language read about 14 points low. The aliases now go on the merged copy instead. |
||
|
Deluan
|
4c0ab074a3 |
chore(deps): update module dependencies in go.mod and go.sum to latest versions
Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
Deluan Quintão
|
65751d7665
|
fix(playlists): chunk track deletes to stay under the SQLite variable limit (#5977)
PlaylistTrackRepository.Delete built a single IN clause with one bind variable per track, so removing more tracks than SQLITE_MAX_VARIABLE_NUMBER (32766) failed with "too many SQL variables". Clients that sync a large playlist by adding the desired tracks and then removing the stale ones would get the add committed and the removal rejected, leaving the playlist with both sets of tracks and growing it on every sync. Delete now works in chunks of 200, the same size addTracks already uses, and renumbers once after the last chunk. Both callers already run inside a transaction, so the delete stays atomic. |
||
|
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
|
5b758fc20c
|
fix(artwork): re-resolve artwork when image files change on disk (#5965)
* fix(artwork): re-resolve artwork when image files change on disk An image-only folder change (replaced, added, or deleted cover/artist images, with no audio files touched) was detected by the scanner but never reached the artwork queue, so clients kept seeing the old coverArt hash until something else forced a re-resolution. Phase 1 now diffs each changed folder's image list and imagesUpdatedAt against the previously persisted folder row, and at the end of the phase bulk-enqueues re-resolution for the affected entities: albums with tracks in the folder or its direct children (covering disc subfolder layouts), and, when an artist-pattern image is involved, artists with albums under the folder's subtree, mirroring the artist resolver's upward search. The artist mapping mirrors the resolver's sole-album-artist album selection. New repository helpers keep the mapping set-based and light: folder GetAllIDs, media_file GetAlbumIDsByFolder (distinct, indexed by folder_id), and album GetSoleAlbumArtistIDs. * refactor: simplify the image-change artwork enqueue after review Load the previous folder image state through the existing GetFolderUpdateInfo bulk pre-pass instead of a per-folder SELECT inside the persist transaction, and skip the diff for new folders, whose artwork the scanner already enqueues inline. Move the artist-image classification into core/artwork (IsArtistImageFile) so the scanner shares the resolver's ArtistArtPriority token grammar instead of re-parsing it (the copy mistreated image-folder as a filename glob). Move the folder-subtree query into the folder repository (GetSubtreeIDs) with LIKE escaping and expression-tree batching, share the sole-album-artist predicate between the resolver and the album repository (model.SoleAlbumArtistFilter), extract a chunked single-column query helper, and deduplicate the ArtworkQueueItem literals behind scanArtworkItem. * fix(persistence): keep slash-form paths in GetSubtreeIDs subtree predicates The scanner hands GetSubtreeIDs io/fs slash-form paths, but filepath.Clean rewrites them with backslashes on Windows while folder.path is stored with forward slashes, so the descendant predicates matched nothing and nested artist folders were never re-enqueued there. Normalize with path.Clean, like HasAudioOutsideFolders does, and cover a nested path in the repo test. * refactor(persistence): move the sole-album-artist rule into the album repository SQLizer filters belong in the persistence package, not model. The rule becomes an unexported filter shared by GetSoleAlbumArtistIDs and a new GetBySoleAlbumArtist repository method, which the artist artwork resolver now calls instead of building the squirrel filter itself. * perf(scanner): resolve image-change artists in one query over album.folder_ids The artist half of the image-change enqueue walked folder subtree IDs, then media_file rows, then album rows, marshalling thousands of bound IDs through the driver on each hop. Matching albums by their own folder_ids instead is one statement, and folder_ids is the same source the artist resolver uses to compute an artist's folders. Benchmarked against a copy of the production DB (97k tracks, 10k folders, 7k albums): 87ms +/-196% -> 17.4ms +/-8%, 7.1MB -> 172KB, 103k -> 1.5k allocs. The subtree predicate becomes a shared folderSubtreeFilter, so Folder GetSubtreeIDs and Album GetSoleAlbumArtistIDs are no longer needed. * fix(scanner): persist ancestor folders discovered by a quick scan A quick scan skipped any new folder with no files of its own, so an artist folder holding only album subfolders never got a row. Adding artist.jpg to it later then produced no artwork enqueue: the entry was new, so the image diff was skipped, and it has no tracks, so nothing was enqueued inline either. Skip only genuinely empty new folders, matching what a full scan already persists. This also fixes artist artwork resolving as absent for artists first imported by a quick scan, since the resolver's folder climb needs that row. Also normalizes the selective-scan preload paths with path.Clean, so its descendant predicates match the stored slash-form paths on Windows. * fix(persistence): chunk subtree paths and match artist globs by basename Two regressions from earlier commits on this branch. Collapsing the subtree query into a single statement dropped the chunking the old GetSubtreeIDs had: each path expands into 3 OR terms and SQLite rejects an expression tree deeper than 1000, measured at 166 paths. A library with more artist-image folders than that (the prod copy has 158) would fail the whole collect, dropping the album items with it, so the scanner now keeps them when the artist query fails. The artist-image classifier compared whole tokens after stripping album/, so a directory-bearing glob like images/artist.* never matched the basenames the scanner has. Match on path.Base, which is what album/artist.* already reduced to; the resolver climbs parent folders, so an exact prefix is not knowable here and a conservative match is the right failure direction. * refactor(persistence): halve the repository surface this PR adds Research on the four new repository methods found two were avoidable. GetAlbumIDsByFolder now expands the changed folders to their direct children in its own subquery, so Folder.GetAllIDs has no callers and is deleted, one round trip per scan disappears, and the previously unchunked id/parent_id IN lists are covered by the existing chunking. GetBySoleAlbumArtist becomes an exported SoleAlbumArtistFilter, matching the ParticipantIDFilter precedent for sharing a Sqlizer with core/, so the rule still lives in persistence but AlbumRepository gains nothing and the mock shim that ignored the artist filter is gone. Also drops queryAllSliceChunked, now callerless, in favour of the file-local slices.Chunk convention used by the sibling folder queries. Rejected on measurement: matching the album path by album.folder_ids is exactly equivalent (13975 pairs, zero difference) but has no index, so it scans every album and runs 5-200x slower than the media_file route. * refactor(persistence): stop reading the deprecated album_artist_id column Both artist lookups this PR touches now go through participation, matching the precedent in core/archiver.go and share_repository.go. SoleAlbumArtistFilter uses ParticipantIDFilter, which is also faster: the album_artists unique constraint is a covering index for it, while the old column needed album_artist_album_id plus a row fetch. GetSoleAlbumArtistIDsInSubtrees reads the sole artist out of the participants JSON it already parses for the sole-artist check, rather than joining back to album_artists, which measured ~1.6x slower on a prod-sized copy. Verified equivalent on that copy: 6828 sole-artist albums and 1088 subtree artists resolve identically via the column, the join and the JSON. The tests now set a deliberately wrong album_artist_id so they fail if either query starts reading it again. * docs: trim comments that carry rationale belonging in commit messages Five comments had grown past the budget with benchmark numbers, rejected alternatives, and a duplicate of the constant's own explanation. * refactor(scanner): move the image-change enqueue into phase_1_folders The three functions were methods on phaseFolders, so they belong with the type; phase_1_image_changes.go also read like a fifth phase, which it wasn't. * refactor(scanner): extract the image-change collector into its own type phaseFolders no longer owns the per-library map and the mapping methods; it records into a collector and asks it to enqueue once. The collector keeps the library alongside the folders, so enqueue needs only ctx and the datastore. * refactor(scanner): simplify enqueue method by removing redundant datastore parameter Signed-off-by: Deluan <deluan@navidrome.org> * docs(scanner): drop the stale zero-value claim on imageChangeCollector The collector now takes its datastore at construction, so the zero value is no longer usable. * fix(scanner): pin the persist stage to concurrency 1 and guard the collector The stage relied on go-pipeline defaulting to one worker; stating it at the stage makes the constraint visible where someone would change it. The collector takes a mutex too, so the type is safe on its own terms rather than by configuration. --------- Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
Deluan Quintão
|
42dfdf49da
|
feat(lyrics): support [bg:] tag and skip unknown tags in LRC files (#5966)
Unknown [name:value] tag lines (e.g. [al:], [by:]) were being glued onto the previous lyric line, producing junk cues like "\n[bg: " and stretching the line's timing. Now any unrecognized tag line is skipped whole, and the non-standard [bg:] background-vocal tag is parsed into cues attached to the preceding line under a bg agent, using the same agents representation the TTML parser emits for Apple-style background vocals. |
||
|
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
|
5b87a60b5d
|
fix(plugins): load plugin agents in CLI commands (#5959)
* feat(plugins): load plugin agents in CLI commands A CLI that goes through core/agents saw only built-in agents: getEnabledAgentNames asks Manager.PluginNames, which reads a map populated solely by Manager.Start, and only the server calls that. On a plugin-using install the CLI's agent list was quietly short — artwork explain --live could name deezer for an artist whose stored source was external:apple-music, because apple-music was invisible to it. Adds Manager.LoadPlugins, the read-only counterpart to Start: extism/wazero init plus loadEnabledPlugins, without the folder sync, the error clearing, the cache purge or the watcher. It follows what the read-only plugin commands already do — list, info and validate read the DB and never start the manager — except that capabilities are detected from the WASM exports, not declared in the manifest, so instantiating is the only accurate source for what a plugin provides. loadEnabledPlugins disabled a plugin and recorded LastError when a load failed. That is right for the server and wrong for a diagnostic, so it is now gated on the read-only flag: inspecting a plugin must not disable it. No Subsonic router is required. Start log.Fatals without one, but the host function it feeds already nil-checks and reports 'SubsonicAPI router not available' at call time, so a plugin that reaches for it gets an error instead of the process dying. That Fatal's message also claimed the DataStore was missing; it checks the router. * fix(cli): correct the reprocess estimate's plugin caveat imageAgentCount now receives a manager with plugins loaded, so the external estimate already includes plugin image agents — but the disclaimer still said they were not counted, which told operators the opposite of what the number meant. Replaced rather than dropped: loadPluginAgents warns and continues when LoadPlugins fails, and LoadPlugins is a no-op when plugins are disabled or no folder is set, so there are still runs where plugin agents genuinely are not counted. The wording now covers all three cases, and the stale comment above it said the CLI never starts the plugin manager, which is what this branch changed. Found by Codex on 648cf38e9. * fix(plugins): load only the configured agents, and gate plugin init Loading a plugin is not free: the service constructors create a KVStore or TaskQueue database and a Storage directory for any plugin whose manifest declares those permissions, and the plugin's own init then runs arbitrary code. loadEnabledPlugins loads every enabled plugin, so inspecting artwork was starting scrobblers, schedulers and lyrics plugins that could never supply an image. Measured on a copy of a production library: 'artwork explain' created apple-music/kvstore.db, nd-lyrics/kvstore.db and listenbrainz-daily-playlist/taskqueue.db. The last one matters most — CreateQueue resets rows with status='running' to 'pending', which against a live server sets up its in-flight tasks to run twice. LoadPlugins now takes the names to load, and the artwork CLI passes the Agents list: a plugin that is not a configured agent can never win, so there is nothing to gain by instantiating it. The same two runs now create only apple-music, which is a configured agent and therefore the cost of answering the question. Init is gated separately on the caller's intent rather than on read-only. 'explain --live' already means 'reach the provider', so it runs init; plain 'explain' and 'reprocess' promise no external requests and must not. Documented in the --live flag help. Found by Codex on 28eb39ac4. * perf(cli): load plugin agents only when the selection can consult one Explaining disc or media file artwork loaded every configured metadata plugin, though neither resolver ever reaches an agent: resolveMediaFile is embedded-only and discArtworkReader.selectImage refuses external outright. With --live that also ran plugin init for a walk that provably cannot reach the network. The load now sits inside the artist/album branch that already exists, so it is a move rather than a new condition. reprocess did the same for a radio-only selection, whose estimate is unconditionally zero. It is gated on needsImageAgents, which asks exactly what ExternalLookupsPerItem asks, so the two cannot disagree. An artist/album test would look equivalent and would silently zero the playlist estimate, whose generated grid resolves album art through those agents; a test pins that, and reverting the predicate to a whitelist fails it. Found by Codex on b78e67b07. * docs(artwork): trim the comments this branch added to the project budget Six blocks ran past the one-to-two line limit. The LoadPlugins doc was eleven lines over three paragraphs, needsImageAgents spent two of its four explaining an alternative that was rejected, and inspectOpts restated what LoadPlugins already says. What went is reviewer-facing prose that belongs in a commit message: the enumeration of what Start does that this skips, and why an artist/album predicate would have been wrong. What stayed is the reasoning a future reader needs at that line, notably that instantiating a plugin creates its declared services, and that playlists consume the album agent count. * docs(artwork): correct the breaker comment after the recovery ramp It still said a success re-closes the breaker, which stopped being true when closing started requiring breakerRecoveries consecutive answers. * refactor(plugins): rename the scoped-load options to transientLoad inspect claimed the load was only looking, which is false when runInit is true: it instantiates the plugin and runs its init, which may open sockets. transient is accurate for every use of the field, and explains all three behaviours it gates. A load that will not outlive the command has no business persisting findings, instantiating plugins it will never consult, or starting background work it is about to tear down. |
||
|
Deluan Quintão
|
24311918c7
|
fix(artwork): ramp the external circuit breaker back up instead of closing on one answer (#5961)
* fix(artwork): ramp the external circuit breaker back up instead of closing on one answer The breaker went straight from open to fully closed on a single non-transient response, so recovery was a burst: the agent resumed at the limiter's full rate until five consecutive failures reopened it. A not-found counted as that response, and a provider that is blocking still answers the occasional request, so the cycle never settled. Observed on a production library over 100 minutes with apple-music blocked. Of 232 responses, 228 were 403 and 4 were not-found, and those four closed the breaker four times. Each close was followed by another open 1 to 3 seconds later, with about five requests in between: 00:59:05 closed -> 00:59:06 opened 01:23:13 closed -> 01:23:16 opened 01:41:21 closed -> 01:41:24 opened Closing now needs breakerRecoveries consecutive answers, one per probe interval, and any failure discards the count. A not-found still counts, because the provider did answer, but it can no longer close the breaker by itself. Unrelated to the plugin loading in the rest of this PR; it came out of investigating why iTunes kept returning 403 while the breaker was open. * fix(artwork): count only current-episode probes toward breaker recovery The worker drains concurrently, so when the breaker opens there are already calls past allow(), queued in the rate limiter or waiting on a response. Their answers arrive after the open and reached the recovery counter, so breakerRecoveries of them closed the breaker with no probe interval elapsed at all: the burst the ramp exists to prevent. allow() now returns the open episode a call was admitted under, zero when the breaker was closed, and only an answer whose generation matches the current episode counts. The generation also invalidates a probe whose answer lands after the breaker closed and reopened, which a plain probe flag would credit to the wrong episode. The token never crosses the gateFunc seam: allow and record are both called inside Worker.gate, so passthroughGate, tracingGate and offlineGate are untouched. The regression test needs no fake clock. The race is an ordering, not a duration, so it is reproduced by calling allow and record in the order concurrency produces, which is deterministic where a goroutine-based test would pass on a lucky schedule. Found by Codex. * test(artwork): move the breaker ordering spec into the Ginkgo suite The ordering regression does not need a fake clock, so it does not need the plain testing.T runner either. That runner is only used here because testing/synctest requires it; every other spec belongs in the Ginkgo suite. The three specs left in worker_timing_test.go all drive the fake clock. |
||
|
Deluan Quintão
|
dc40bcaf80
|
feat(cli): add an artwork command group for diagnosing and re-driving artwork (#5957)
* feat(artwork): add a resolution chain trace collector * feat(artwork): trace the local priority chain * fix(artwork): record priority candidates the chain never evaluated * refactor(artwork): report never-evaluated candidates as skipped * feat(artwork): trace external agents at the gate seam * feat(artwork): add repository queries to enqueue by current source * feat(artwork): expose a tracing resolver for the CLI * feat(artwork): read a single queue row by item The explain CLI must report whether an item is queued, at what priority and when it retries; the queue repository could only be drained in eligibility batches, which cannot see a row that is still backing off. * feat(cli): add artwork explain Prints why an item has the artwork it has: the stored state, its queue row, the governing config, the resolver's priority-chain walk and the verdict. Offline by default so a diagnostic run cannot add load to an external provider; --live asks the agents for real. Playlists and radios do not walk a priority chain, so they report that instead of an empty chain table. * fix(artwork): trace an external tier that never reaches an agent A configured 'external' token vanished from the chain when no enabled agent provided images for that entity type, and for synthetic artists, leaving the trace unable to say whether the tier was even considered. * fix(cli): never state an artwork outcome the walk did not observe A transient external failure traced as 'error' fell through to 'not resolved', which is the most common state behind a missing-artwork report. It is now indeterminate, and an offline win that a skipped higher-priority external candidate could have taken says so instead of naming a winner the live chain might not pick. * feat(cli): add artwork refresh * feat(cli): add artwork reprocess Bulk re-enqueues artwork by kind and/or by the source an item currently resolves from, previewing the matched count and confirming before queueing. The preview counts with CountBySource (rows matched) and reports separately what EnqueueBySource inserted: its DO NOTHING conflict policy leaves an already-queued row untouched, so the two numbers differ and the output must not claim the skipped rows were re-queued. An unknown --source is rejected against the sources present in item_artwork, rather than silently matching nothing and printing a reassuring 0. * fix(cli): cover the reprocess selection rule and validate sources table-wide The reconciliation that makes --source alone target every kind was only exercised through runReprocess, which no test calls: mutating it to `all := reprocessAll` left the suite green. It is now reprocessSelectsAll, covered for all three selectors. Scoping source validation to the selected kinds made the same well-formed filter valid or invalid depending on which other kinds were selected, and its error read the same for a typo as for a source that simply does not apply to the chosen kind. Validation is now table-wide: a typo still aborts, while a valid-but-inapplicable source falls through to "Nothing matches". Also: the prompt now counts only the kinds that reach an external agent as external cost, and --dry-run on an empty selection reports a dry run. * fix(cli): cover the reprocess --yes guard and preview the external cost Mutating the --yes check to `if true` left the suite green, so the one bypass of the confirmation was unverified. The choice is now reprocessConfirm(yes, in), covered in both directions. The external estimate only reached the operator through the prompt, which --dry-run skips — hiding the number in the one mode that exists to show it before committing. The preview now carries it, and the prompt drops the clause when no lookup will be made. An empty selection says so again under --dry-run. * feat(artwork): add read-only queue and absent counters Both are needed by the artwork status CLI: a queue breakdown by kind and priority, and the absent totals split against the recheck cutoff. * feat(cli): add artwork status Reports the queue, where artwork currently resolves from, absent counts against the 24h recheck window, and the stored config fingerprint versus the current one — the line that turns 'why is my server re-resolving everything?' into one command. fingerprint() and staleAbsentAge are exported so the CLI reports the values backfill itself compares, instead of a second copy of the formula that can silently drift. * fix(cli): lead the artwork status backfill line with the queued backlog By the time anyone runs a diagnostic, backfill has usually already stored the new fingerprint, so 'up to date' was printed while thousands of items churned through external providers. The backlog is the finding; the fingerprint is context. Also echoes the config inputs the fingerprint covers, so a change can be traced to the setting that caused it, and pins the rendered rows: the Absent values, the queue TOTAL and a queue-scoped kind/priority pair were all unasserted, so kindName and priorityName were effectively untested. FingerprintInputs is now the single listing ConfigFingerprint hashes; a pinned hash proves the value did not change. * refactor(artwork): export the trace outcome vocabulary The CLI hardcoded the outcome literals and the "external:" prefix, so renaming a constant's value in core/artwork left cmd compiling and the suite green while `artwork explain` silently degraded its verdict. Renaming a value now fails the golden vocabulary test in core/artwork and the explainResult tests in cmd. * fix(cli): keep the re-enqueue warning when a backfill is already running A stale stored fingerprint with items already queued is the worst state the system can be in: a second full re-enqueue is pending on top of the one running. The line carried the weakest wording of the three, and was untested. * refactor(artwork): drop the unreachable breaker branch from the tracing gate --live wires the tracing gate straight to passthroughGate, so errBreakerOpen can never reach it; the test only passed by injecting a fake gate. * refactor(artwork): delete the never-emitted not-reached outcome Candidates after the winner are lower priority and say nothing about why a source won; the ones that matter sit above it and are already recorded. * refactor(artwork): make the trace nil-safe in one place only add already handles a nil trace, so record's own guard was dead; Steps was the odd one out and would panic where every other method tolerates nil. * refactor(artwork): export the trace types directly ChainTrace and TraceStep were unexported types re-exported through aliases, which existed only so the CLI had a name to refer to them by. The types are public API — Resolver.Steps returns []TraceStep and the CLI constructs a ChainTrace — so name them that way and drop the indirection. Encapsulation is unchanged: add, mu and steps stay unexported, so only this package can write a step. * refactor(cli): simplify parseArtworkKind with slices.Contains Replaces a nested loop and a manual append with slices.Contains and the repo's slice.Map helper. Same behaviour, same error message. * fix(cli): print the absent artwork source under the name --source accepts `artwork explain` rendered the stored empty source as "(absent)", while `artwork reprocess --source` only accepts "absent", so pasting what explain printed straight back into reprocess was rejected as an unknown source. * refactor(artwork): own the kind list and the chain predicate in the package Export RecheckKinds and add WalksPriorityChain so the CLI stops keeping its own copies of both, and unexport externalCandidate, which nothing outside the package consumes. * refactor(cli): drop the artwork command's duplicated state and formatting Reuse artwork.RecheckKinds and artwork.WalksPriorityChain, extract newTabWriter and externalEstimate, fold reprocessSelectsAll into selectedKinds, and derive the queue total and the walks-chain flag instead of carrying them in the report structs. * test(persistence): drop two artwork-queue specs that cannot fail One seeded hash and source together and then asserted the two counts agree, so its setup guaranteed the result; the other repeated the count-does-not- enqueue property already covered by the CountBySource spec. * refactor(artwork): rename Resolver to TracingResolver for clarity * fix(cli): count playlists in the artwork reprocess external estimate The estimate used WalksPriorityChain, which is true only for artist and album, so a playlist-only reprocess reported "External lookups: none" and the confirmation prompt dropped the external-cost warning. Playlists do reach the network: through the m3u ExternalImageURL fetch when EnableM3UExternalAlbumArt is on, and — verified by test — through the generated grid, whose tiles resolve album art via the full album priority chain. Adds artwork.MayFetchExternal, a config-aware predicate for "can this kind's resolver reach the network", and uses it for the estimate. WalksPriorityChain keeps its separate job of deciding whether explain prints a chain block. * fix(cli): estimate artwork reprocess external lookups per agent, not per item The reprocess prompt billed one external lookup per externally-capable item. fetchArtistImage/fetchAlbumImage try every enabled image agent and stop early only on a hit, and resolvePlaylist can fetch the m3u image and then resolve up to four sampled albums for the grid, each walking the album agents again. The number the operator confirmed could understate real provider traffic several fold, in the prompt whose whole job is to stop a provider flood. ExternalLookupsPerItem now multiplies by the visible image-agent count and adds the playlist grid factor. It stays a floor: the CLI never calls Manager.Start(), so the plugin registry is empty and plugin-provided agents are dropped by getEnabledAgentNames. On an install with 5 agents of which 3 are plugins the count is well under the truth, so the wording is now "at least N" rather than "up to N" — a zero visible count still bills one lookup for the same reason. Fixing the plugin visibility is out of scope: Manager.Start() needs a Subsonic router and writes to the DB via syncPlugins, breaking this command group's read-only guarantee. * fix(cli): state the artwork reprocess estimate as an estimate, not a bound Neither bound is true. A ceiling is false because plugin agents are invisible to a CLI that never starts the plugin manager, and a floor is false because a local hit ends the walk before any agent is asked and a hit on the first agent skips the rest. "at least N" traded one wrong claim for another. The line now names its blind spots instead: External lookups: ~340 estimated (plugin agents not counted; local hits may need fewer). The same line is reused in the confirmation prompt, and the zero case still reads "External lookups: none." with the prompt dropping the clause entirely. The count itself is unchanged. * fix(cli): account for every configured agent in artwork explain The Agents: line printed the raw config while the Chain only showed the agents the CLI could construct, with nothing explaining the gap: plugin agents are never registered in a CLI that does not start the plugin manager, and a built-in without credentials returns nil. Three of five agents could vanish, including ones ranked above the one shown. Also treat a live external error before the winning hit like the already-handled would-try case: the resolver serves such a hit provisionally and retries later, so the verdict is indeterminate. The Result line is still not qualified when an unavailable agent might have won; that needs agent ranking, and is left to the follow-up that makes the CLI load plugin agents for real. * fix(cli): do not call an external artwork win indeterminate explainResult qualified the verdict whenever an external OutcomeError appeared before the winning hit. When a later external agent returns an image, fetchArtistImage/fetchAlbumImage discard the earlier error, so extError is false: the worker settles the item and schedules no retry. Telling the operator it may resolve differently on a retry was wrong. The warning is only correct when a lower-priority local source won while an external error was recorded, which is the case that carries extError. * fix(cli): accept --source absent when nothing is currently absent validateSources checks the requested sources against the ones item_artwork actually uses, to catch a typo. The reserved empty source (spelled 'absent' on the CLI) is a valid filter even when it matches nothing, so a scheduled 'artwork reprocess --source absent --yes' stopped working the moment the library finished resolving. Treat it as intrinsically valid and let the existing zero-match path report it. * feat(artwork): explain disc and media file artwork from the CLI `artwork explain` rejected `dc` and `mf` because it validated against RecheckKinds, the list of kinds the backfill revisits. Those are different questions: a kind with no recheck path still has artwork someone can report as wrong. Disc artwork now walks DiscArtPriority under a trace, so explain reports which entry won and why the others lost, including entries that map to no source at all (external is unsupported, a disc with no subtitle, an album folder with no images). Media file artwork traces its single embedded candidate, separating "EnableMediaFileCoverArt is off" from "the track has no embedded art" — stored state cannot tell those apart. Each command now validates against the kinds it can actually serve: explain takes all six, refresh takes artwork.RefreshableKinds (which nativeapi now shares instead of keeping its own copy), reprocess still takes RecheckKinds. Disc artwork stays out of refresh: the worker cannot resolve it, so the queue row would be rejected on every drain. WalksPriorityChain becomes Explainable, and ResolveArtist/ResolveAlbum collapse into Resolve(kind, id). * refactor(artwork): one disc-artwork walk for serving and explain resolveDisc duplicated the loop selectImageReader already ran: try each source in priority order, take the first that yields an image. The serving path and the CLI diverged on two details as a result — only selectImageReader checked ctx between candidates and logged each attempt. Both now call discArtworkReader.selectImage, which takes the chainState the CLI already uses for the other kinds. The serving path passes an untraced one, whose nil trace makes recording a no-op. selectImageReader had no other caller and is gone. The disc tests move from fromDiscArtPriority to discCandidates, so they assert the skip reason for an entry that maps to no source rather than that it silently vanished, and cancellation mid-walk is now covered. * fix(artwork): reject a nil reader in the resize cache instead of panicking resizedItem.Reader closes what open() hands back, so an open() that reports "no image" as (nil, nil) rather than an error takes the request down with a nil-pointer panic. Every caller returns an error today, and no test covered it: the resolution e2e harness stubs the resize reader out entirely, so no e2e path reaches this code at all. Guard it and cover Reader directly. * refactor(artwork): move the keeps-state fact into core, drop a redundant guard keepsArtworkState lived in package cmd and re-derived by hand what RefreshableKinds already encodes: the same five-of-six kinds. It is now artwork.KeepsState, beside the list, with a test pinning the two together — nothing else stopped them drifting, and a drift would have explain report stored state for a kind that keeps none. serveDisc's closure also hand-rolled a nil-reader error that both consumers of open() now produce themselves: serveSource for a full-size request, resizedItem.Reader for a resized one. * fix(artwork): route disc candidates through the shared resolvers openCandidate ran its own source loop and threw the error away, so a disc track that exists but cannot be parsed traced as "miss" — indistinguishable from a track with no embedded art. fromTag and fromFFmpegTag already report that case as errSourceUnreadable; only this loop was discarding it. Telling those two apart is what the trace is for. Candidates now carry a resolve func instead of raw sources: embedded goes to resolveEmbedded, and the folder-backed entries to resolveFolderSource, extracted from resolveFolderFile so both callers classify an unopenable file the same way. openCandidate and its absolute-path special case go away with it. Disc's own fromExternalFile and fromDiscSubtitle still swallow open errors, so folder candidates cannot report unreadable yet; that is a change to their error contracts. * fix(artwork): report an unreadable local candidate as indeterminate processor.acquire treats resolution.localError exactly as it treats extError: a fault is not a definitive "no image", so it retries instead of settling absent. explainResult qualified only the external case, so a chain that ended on an unreadable local candidate printed "not resolved" — the one verdict that says the walk was conclusive. The qualification belongs only to the unresolved branch. chainState.try stamps extErr onto a hit and deliberately drops localErr, so an unreadable step followed by a hit is settled as found and must not carry a warning; a test pins that. Found by Codex on 5f65d7cfa. |
||
|
Deluan Quintão
|
b617a878b9
|
feat(insights): report the app store or hosting platform via ND_PLATFORM (#5956)
* feat(insights): report the app store or hosting platform via ND_PLATFORM Insights had no way to tell where an instance is deployed. The existing `os.package` field is written only by our own packagers and holds just `deb`, `rpm` or `msi`, so it answers "which installer", not "which platform". Overloading it would mix two unrelated dimensions in the same field. This adds a separate top-level `platform` field, self-declared by the deployer through the `ND_PLATFORM` environment variable. App stores and hosting providers (ZimaOS, PikaPods, TrueNAS, Unraid, and others) generally deploy our container image unmodified and can only inject environment variables, so an env var is the one marker they can all set. It is deliberately not a config option: it is a packager marker, not something users should tune, and it stays out of the config surface. Both values are now whitespace-trimmed. The msi packager writes the file with `echo`, so `os.package` has been arriving as `"msi\n"` and sorting separately from `"msi"` in any aggregation. * test(insights): isolate hostingPlatform specs from an inherited ND_PLATFORM The spec asserting an empty result read the real environment, so it failed on any machine that already had ND_PLATFORM set. Unset it per-spec, using Setenv first so Ginkgo restores the original value on cleanup. |
||
|
Deluan Quintão
|
95615bcb18
|
fix(artwork): serve images whose format has no registered decoder (#5952)
* fix(artwork): serve images whose format has no registered decoder The new pipeline derives dimensions, mime and the placeholder hashes at resolution time, so a decode became a precondition for recording artwork at all. An image in a format Go has no decoder for therefore failed acquisition, retried until the 12h budget ran out, and then settled as absent, serving a placeholder from that point on. The old pipeline decoded only to resize and fell back to the original bytes when that failed, so these covers used to work. A local file is picked by matching an image extension, so bytes it cannot decode are most likely a codec we lack: image.ErrFormat on a folder, upload or embedded source now yields an Artwork row carrying just the hash and mime, and the bytes stay servable. An external response carries no such guarantee, so it still fails and retries rather than pinning a non-image body as a cover. A corrupt image of a known format and an over-cap declared size still fail, so the decompression bomb guard is unchanged. Absent rows recorded by earlier builds are re-resolved by the existing stale-absent recheck within a day, so no epoch bump is needed to repair them. Reusing a stored image now re-decodes when it carries no dimensions, so a row recorded while a decoder was missing can still be upgraded later. Registers jxl, heic and heif in mime_types.yaml: image detection resolves the extension through the host MIME table, and the Alpine release image ships no /etc/mime.types, so those covers were never recorded in folder.ImageFiles there and never reached the pipeline at all. * fix(artwork): never record empty bytes as artwork image.DecodeConfig returns image.ErrFormat for an empty payload just as it does for a codec with no registered decoder, so a zero-byte cover file was recorded as found artwork and served as an empty response instead of falling back to the placeholder. A truncated image of a known format already fails with unexpected EOF rather than ErrFormat, so only the empty case needed the guard. |
||
|
Deluan Quintão
|
aa0824e03b
|
feat(jellyfin): add System/Endpoint so Finamp's connection test passes (#5955)
Finamp's connection test GETs /System/Endpoint and treats anything other than a 200 carrying an IsInNetwork key as "not a Jellyfin server", so the test failed against Navidrome and dual-connection setups could never switch to the local address. IsInNetwork mirrors Jellyfin's default LAN set (NetworkManager with no LocalNetworkSubnets configured): loopback, the RFC 1918 ranges, fc00::/7 and fe80::/10. Notably that set omits 169.254.0.0/16, so Go's IsLinkLocalUnicast is deliberately restricted to its IPv6 half. IsLocal mirrors HttpContext.IsLocal(): the caller shares the connection's local address, not merely "is loopback". It falls back to the loopback check when the local address is unavailable. |
||
|
Deluan Quintão
|
59a4ed8e79
|
fix(plugins): stop reporting plugin call failures as not-found (#5953)
* fix(plugins): stop reporting plugin call failures as not-found MetadataAgent joined agents.ErrNotFound onto every failed plugin call, so a transport fault was indistinguishable from a definitive miss. The artwork circuit breaker treats a not-found as a successful, definitive answer and resets its failure counter, so it never opened for a failing plugin and kept calling it on every request. Observed with the apple-music plugin against prod: ~900 iTunes 429s in 27 minutes with the breaker never tripping. Return the underlying error instead. The genuine empty-result branches still return agents.ErrNotFound, and agent fallback is unaffected because callAgentMethod/callAgentSliceMethod continue on any error, not only on ErrNotFound. * test(plugins): fold duplicate metadata agent error specs into one table The error-handling container drove all 11 MetadataAgent methods twice: once to assert the message, once to assert the failure is not an ErrNotFound. The argument lists were identical, so each method cost two WASM instantiations for one method's worth of coverage, and a new capability had to be registered in two places to stay guarded. Fold both assertions into a single DescribeTable, document the ErrNotFound contract at the sentinel where agent implementers will read it, and collapse breaker.record's hand-inlined predicate onto isTransientExternal, which it already duplicated by hand with a keep-in-sync comment. * fix(plugins): keep an unimplemented plugin method a definitive miss Returning the raw plugin error made errNotImplemented and errFunctionNotFound look like provider faults. Every MetadataAgent satisfies ArtistImageRetriever and AlbumImageRetriever regardless of what the plugin actually exports, so artwork resolution calls those stubs on a partially-implemented plugin: each call counted toward the artwork circuit breaker and kept the item in the retry queue instead of settling it absent. Map both sentinels back onto agents.ErrNotFound, joined so the underlying reason survives for diagnostics, and leave real call failures untouched. This matches what ScrobblerPlugin already does for the same two sentinels. The partial-implementation specs asserted only MatchError(errNotImplemented), which the previous errors.Join satisfied incidentally, so nothing caught the lost not-found semantics. They now assert both and are folded into one table. * test(plugins): cover the missing-export arm of agentErr The partial-metadata-agent fixture registers through the Go PDK, which exports every method and answers with the not-implemented code, so no fixture reaches the errFunctionNotFound branch. Building one would mean hand-writing Extism exports to deliberately omit a function, which tests the manager's function lookup rather than the mapping this PR added. Cover agentErr directly instead: both sentinels classify as a definitive miss, a call failure and a non-zero exit stay faults, and the underlying reason survives in every case. * fix(artwork): stop counting a cancelled run against the circuit breaker callPluginFunction returns ctx.Err() when a plugin call is cancelled, and that reached breaker.record as an ordinary error, so cancellations counted toward the five consecutive failures that open a gate. A cancellation says nothing about the provider, so it now neither counts nor clears the failure run. Deliberately scoped to breaker.record rather than isTransientExternal: the latter also drives whether the queue item is rescheduled, and a cancelled item must still be retried rather than settling absent. context.DeadlineExceeded is left counting as a fault, since a provider that blows the budget is one worth backing off from. Reachable today only at shutdown, where the in-memory breaker state is discarded anyway. It becomes live the moment Worker.gate is used on a request-scoped context, which is why it is worth closing now. |
||
|
Deluan Quintão
|
757ca783d3
|
fix(instant-mix): top short mixes up instead of returning what the first source found (#5951)
* fix(instant-mix): top short mixes up instead of returning what the first source found
SimilarSongs returned the first non-empty source's tracks, however few. For a
thinly-represented artist that meant a 3-track mix no matter the requested count:
the artist agent found nothing, the similar-artists fallback matched 3 library
tracks, and `len(res) > 0` kept seed-track sampling from ever running.
Clients treat that as a failed mix and retry with a bigger limit forever. Finamp
cycles limit 34 through 472 and starts over, ~1 request every 2s indefinitely,
each one re-hitting Last.fm, Deezer and AudioMuse.
Sources are now chained rather than raced: each one tops the mix up until it
holds count tracks, so the agent's picks, the similar-artists fallback and
seed-track sampling all contribute instead of the first one winning outright.
* refactor(external): move similar-songs code to its own file
provider.go held two distinct concerns: artist/album external metadata and the
similar-songs mix pipeline. The mix code was already one contiguous block, and
maxSeeds, maxSimilarSongs and dedupByID were used by nothing else.
Moved SimilarSongs and its helpers to provider_similarsongs.go, matching the
existing provider_similarsongs_test.go. Pure code motion: the moved block is
byte-for-byte unchanged and provider.go has no additions, only deletions.
* fix(instant-mix): dedup before deciding a mix is full
topUp measured res before deduplicating it. Matcher.MatchSongs deliberately
re-emits a library track when the same input song repeats, and the similar-artists
fallback can reach one track through several artists, so len(res) could equal count
while holding fewer unique tracks. That returned a mix with duplicates in it and
stopped the top-up early; the caller then deduplicated and handed back a short mix,
which is the client retry loop this branch set out to fix.
Deduplicate first, so the length check counts what the client will actually receive.
* perf(instant-mix): skip a fallback once the mix is already full
The artist path nested one topUp inside another, so the inner one measured only
similarSongsFallback's own result against the full count. With 49 agent matches and
one fallback match for count=50 the mix was already full, yet seed-track sampling
still ran and fired up to five GetSimilarSongsByTrack calls whose results the outer
topUp then truncated away.
topUp now takes the sources as a variadic list and re-checks the accumulated mix
before each one, so a later, costlier source only runs while the mix is still short.
That also flattens the artist case: the agent, the similar-artists fallback and
seed-track sampling are now three peers in one chain instead of two nested calls.
* fix(instant-mix): count distinct tracks when picking the fallback mix
similarSongsFallback stopped after count picks from the weighted chooser, but a
track can sit in that chooser once per artist listing it in their top songs, and
Pick removes the entry it returns. Repeats therefore consumed pick slots and left
unique candidates stranded, so the batch could come back short of count. On the
track path this is the only source, so that short mix reached the client and kept
the retry loop alive.
Track the ids already picked and keep drawing until count distinct tracks are held
or the chooser is empty.
* fix(instant-mix): match the whole agent response before trimming
MatchSongs was capped at count, and it re-emits a track when the same song repeats,
so [A, A, B] with count=2 returned [A, A] and never reached B. topUp then shrank
that to [A] and, with an empty or overlapping fallback, the mix stayed short even
though B had been available all along. seedMix already matched its full merged set
for this reason; mixFromAgent now does the same and leaves the trim to topUp.
Also drop the capacity hint on the picked-ids map. It was sized from the caller's
count, which CodeQL flags as an allocation sized by user input (go/uncontrolled-
allocation-size). SimilarSongs clamps count to maxSimilarSongs long before this
point, so the hint bought nothing worth the alert.
* refactor(instant-mix): tidy the mix chain and its specs
Quality pass over the new code, no behaviour change:
- topUp: drop the first-vs-last error bookkeeping (the value is only read when the
mix is empty, so the distinction is unobservable) and the redundant nil guard
(dedupByID returns nil for an empty result, so both branches already agreed).
- mixFromAgent: assign through the if-scoped err instead of a second error name.
- Hoist the similar-artists fallback closure written verbatim in two switch arms.
- Use map[string]struct{} in the pick loop, matching dedupByID in the same file.
- Trim three comments back within budget; two restated the line below them and one
carried commit-message rationale.
- Tests: add an ids() helper for the ID assertion repeated seven times, and fold
the track-entity stub block copied into three specs into stubTrackEntity. The
block hard-coded .Twice() on GetEntityByID, which pinned an implementation
detail no spec asserts.
* revert(instant-mix): inline the similar-artists fallback closure again
Hoisting it to a shared artistFallback var moved the call away from the arm that
uses it and saved nothing: each arm reads better spelling out its own sources.
|
||
|
Deluan Quintão
|
6c3e7e268b
|
feat(instant-mix): support album, playlist and genre sources (#5948)
* feat(agents): local agent genre-hint similar songs fallback
* feat(external): playlist instant mix via seed-track sampling
* test(external): cover playlist mix never-empty fallback and maxSeeds cap
Adds coverage for the empty-match seed fallback and the maxSeeds
call cap on GetSimilarSongsByTrack, per code review finding.
* feat(external): genre instant mix via seed-track sampling
* feat(external): album instant mix falls back to AudioMuse track similarity
* feat(external): artist instant mix falls back to seed-track sampling
* fix(jellyfin): route genre seeds through instant mix instead of empty
* feat(jellyfin): add /Albums/{id}/Similar route for albumMix radio
* perf(external): bound playlist seed sampling to a random N
samplePlaylistTracks loaded an entire playlist's joined rows just to keep
5 random seeds; push the bound and randomization into the query instead,
matching the other samplers (GetRandom/GetAllByTags with Max).
Fixing this surfaced a real bug: resetSeededRandom's SEEDEDRAND rewrite
assumed every table's id is TEXT, but playlist_tracks.id is an INTEGER
position, so the random sort silently dropped every row. Cast the id to
TEXT before hashing (no-op for the other, TEXT-id tables).
Also trims a changelog-flavored comment and a duplicated rationale in
server/jellyfin/similar_test.go.
* refactor(external): parallelize seed mix and dedup mix helpers
Run the up-to-5 per-seed GetSimilarSongsByTrack calls concurrently (errgroup),
route the four container cases through a shared seedMix helper, flatten the
genre lookup, and sample playlist seeds without forcing a smart-playlist
rebuild. Share the media-file->Song mapping in the local agent.
* perf(agents): use the indexed genre filter for local similarity
Replace GetAllByTags (a json_tree scan of every media_file row) with the
media_file_tags semi-join from #5940, deriving the seed's genre tag ids
locally since they hash from (name, value).
Also carry the library id and the recording MBID on the returned songs:
the matcher resolves by id first and looks up mbz_recording_id, so the
release-track id it got before matched nothing and the local fallback
silently returned no songs.
* refactor: drop redundant MBID and fold mixFromSeeds into seedMix
The local agent returns library tracks, so the id alone resolves them in the
matcher's first phase; the MBID was never consulted. mixFromSeeds had no
caller other than seedMix.
* docs: trim redundant comments
* fix(jellyfin): adopt the GUID id codec in the merged similar routes
getSimilarAlbums still used resolveItemID/DecodeID, which #5942 replaced with
itemIDParam; its tests passed raw ids that the strict codec now rejects.
* fix(external): guard non-positive counts and blend every seed
A negative Subsonic count reached matched[:count] and panicked. The matcher
also keeps input order and stops at count, so seed-grouped results let the
first seed fill the whole mix; interleaving gives every seed a share.
Drops the duplicate playlist-track mock in favour of tests.MockPlaylistTrackRepo,
which pages like the real repository and records the query options.
* fix(external): refresh smart playlists before sampling seeds
A smart playlist materializes no playlist_tracks until it is evaluated, so
sampling without the refresh mixed an empty seed set. The refresh is a no-op
for regular playlists, inside the refresh delay, and for non-owners.
* fix(external): skip missing tracks and a nil playlist-track repo when sampling
Tracks() logs and returns a nil repository when its own lookup fails, so the
chained GetAll panicked. Seeds can also reach the mix verbatim when the agents
find nothing, so a missing file would surface as an unplayable entry.
* fix(jellyfin): never report the seed album as its own similar album
The sampled-seed fallback returns the album's own tracks, which similarAlbums
mapped straight back to the requested album, often as the only result.
* test(agents): assert the genre predicate instead of relying on the mock
MockMediaFileRepo ignores QueryOptions.Filters, so the spec passed even with
no genre filter at all. It now checks the generated predicate carries the
seed's own tag id, the indexed join and the missing exclusion.
* fix(external): clamp the requested count before it becomes a query limit
Subsonic passes the client's count through unbounded. At MaxInt64 the local
agent's count+1 overflows negative, and GetRandom omits the SQL limit unless
Max is positive, so one request would hydrate every matching track. 500 is
what the widest caller (similarAlbums, limit*5) legitimately asks for.
* fix(external): deduplicate playlist seeds by media file
A playlist can hold the same file at several positions, so sampling its rows
could seed the mix twice: a wasted agent call, and a duplicate track whenever
the seed fallback kicks in.
* fix(external): drop tracks two seeds both recommend
The matcher re-emits a track when two inputs are identical, so overlapping
recommendations took several slots in the mix. Match the whole merged set and
dedup before trimming. Playlist sampling now over-fetches before its own
dedup, so repeated positions cannot collapse the seed count.
* test(external): make the seed-blend assertion independent of the shuffle
It matched four tracks and kept two at random, so both could come from the
first seed once in six runs. Keeping three of the four makes a seed-two track
unavoidable.
* fix(external): seed artist mixes from every credited role
media_file.artist_id is the deprecated primary artist, so an artist credited
only on the album, as on compilations, sampled no seeds at all. Use the same
participant filter the artist listings use.
* refactor(external): drop the now-vestigial seed interleaving
Matching the whole merged set removed the early truncation the interleave
guarded against, and the shuffle before the trim makes input order irrelevant.
Its comment described the old behaviour.
* test(agents): give the id-mapping fixture a matching genre
The related track carried no genre, so the real query would never return it;
the spec only passed because the mock ignores QueryOptions.Filters.
* test(agents): drop the MBID from the id-mapping fixture
Local agent candidates are non-missing library rows, so the matcher always
resolves them in its id phase and never reads the MBID. The field guarded a
regression that could not change behaviour.
* test(agents): remove unnecessary comment about MBID in GetArtistTopSongs test
* fix(jellyfin): only let a not-found entity fall through in getInstantMix
Discarding the error conflated a genre id, which never resolves, with a real
lookup failure, which then made a provider call that fails the same way.
* test: pin the invariants the specs only appeared to cover
The missing filter was asserted by substring, so flipping it to true passed
everywhere, including the spec named for it. Matching the whole merged set,
the local agent's over-fetch, and its no-genres early return had no coverage
at all; each is now pinned by a spec that fails when the code is broken.
* test: make the remaining specs say what they actually guard
The playlist-track spec named a sort whitelist it does not exercise; it guards
the integer-id CAST, so it now asserts no rows are dropped. The maxSeeds cap
passed with either bound removed, and the over-fetch was pinned by its literal
value rather than the duplicate positions it exists for. Also drops setup the
count guard returns before reaching.
* fix(external): fall back when the agent's picks are not in this library
A non-empty answer whose songs are all absent locally matched nothing and was
returned as-is, so the mix came back empty with sampleable source tracks
sitting right there.
* refactor(external): name the agent-then-fallback flow once
Each entity case repeated the same error and emptiness plumbing around the
matcher. mixFromAgent states it once and each case supplies only what differs:
how to ask, and what to do when the answer is unusable.
|
||
|
Deluan Quintão
|
c66ef04dd3
|
refactor(jellyfin): emit real 128-bit GUIDs as item ids (#5942)
* test(jellyfin): use canonical ids in dto fixtures
Fixtures used short placeholder strings, which are not valid Navidrome ids. Deriving them from
id.NewHash keeps the labels readable while exercising the real id shape.
* test(jellyfin): use canonical ids in handler fixtures
Fixtures used short placeholder strings, which are not valid Navidrome ids. Deriving them from
id.NewHash keeps the labels readable while exercising the real id shape. Playlist entry positions
stay decimal, matching the integer playlist_tracks.id column.
* test(jellyfin): use canonical ids in e2e fixtures
Fixtures used short placeholder strings, which are not valid Navidrome ids. Deriving them from
id.NewHash keeps the labels readable while exercising the real id shape.
* test(jellyfin): use canonical ids in audiomuse fixtures
audiomuse_test.go passes ids as bare function args (mf(id, ...), call(query, user)) rather than
via ID: struct-literal fields, so the original grep-built file list missed it. Same conversion as
the rest of the fixtures: fake labels through id.NewHash via testID.
* test(jellyfin): convert remaining nonexistent-id sentinels in e2e tests
Reviewer swept for enc("literal") sites the brief's dto.EncodeID grep missed. These "does not
exist" fixtures must stay well-formed GUIDs under the strict codec, or the test degrades from
"resolves to nothing" to "empty path segment".
* refactor(jellyfin): emit real 128-bit GUIDs as item ids
Navidrome ids are now a canonical 22-char base62 encoding of exactly 128 bits, so they map
losslessly onto Jellyfin GUIDs. Previously the API hex-encoded the id string itself, producing
44 hex chars where Jellyfin uses 32.
Integer library ids, the synthetic playlists folder, and playlist entry positions (a
playlist_tracks.id, an integer column) aren't 128-bit values, so they get a reserved GUID space
tagged by kind. DecodeID is now strict: malformed input returns an empty string instead of
passing through unchanged.
BREAKING: Jellyfin clients see entirely new item ids.
* fix(jellyfin): 404 malformed playlist ids instead of silently creating
updatePlaylist decoded a malformed playlistId to "", the same sentinel core/playlists.Create
uses to mean "make a new playlist" — the overload createPlaylist deliberately relies on. A
malformed id now 404s before reaching Create.
Also tightens id-codec test fixtures: several tests set chi params to a raw canonical id, which
now decodes to "" and only passed because the fakes ignore the id argument; and a batch of
not-found sentinels now use well-formed-but-nonexistent GUIDs so they exercise the intended path
instead of the malformed-id path. READMEs "lossless" claim softened to note the reserved space.
* refactor(jellyfin): drop the id truncation workaround
Finamp's saved-queue packing keeps the first 16 bytes of each item id. That was lossy only
because our ids were 44 hex chars; now they are 32, so the packing round-trips exactly and the
server-side prefix recovery is dead code.
Removes an indexed range scan per restored queue and the ambiguous-prefix path that could
resolve to the wrong item.
* fix(jellyfin): emit ServerId and PlaySessionId in Jellyfin's id format
Jellyfin serializes GUIDs without dashes; ServerId was emitting the dashed UUID form. A
ServerId persisted before this change is normalized on read rather than rewritten.
PlaySessionId was emitting a raw internal id instead of the encoded form.
BREAKING: the ServerId change makes clients treat the server as new, so users re-login once.
* fix(jellyfin): 404 on undecodable id filters instead of widening the query
DecodeID collapsed an absent param and an undecodable one into the empty string, and downstream
an empty id means no filter. A client sending a stale pre-upgrade id therefore had its filter
silently dropped: ParentId, ArtistIds and AlbumArtistIds each returned the whole library instead
of a scoped result. Every existing client hits this on first launch after the id format changes.
Scalar id params now distinguish the two cases and report not-found. List-valued params already
failed closed. EncodeID logs a diagnostic when a non-empty id is not canonical, which should not
happen post-migration and would otherwise ship an unaddressable item silently.
* test(jellyfin): drop comments that restate the spec names
* refactor(jellyfin): decode reserved GUIDs from bytes, not hex strings
DecodeID already had the 16 decoded bytes, then re-derived the kind tag and payload by slicing the
hex string and parsing it a second time. Reading them off the byte slice matches how the format is
specified and removes the duplicate parse.
Bounding the payload inside encodeReserved gives both encoders the 32-char guarantee, which only
EncodePlaylistEntryID enforced before.
Playlist entries now decode through DecodePlaylistEntryID, which rejects other kinds. The tag was
being encoded and then discarded, so a song id passed as an EntryId reached RemoveTracks as a
playlist_tracks position.
Drops the per-field log.Warn from EncodeID: it sat in a leaf codec without a ctx and would emit
once per item per request on exactly the bad-data population it was meant to surface.
* refactor(jellyfin): make DecodeID report whether the id was decodable
DecodeID returned the empty string for both an absent param and an undecodable one, and
downstream an empty id means no filter. That conflation is what let a stale id widen /Items to
the whole library; it had been patched at two call sites, leaving three different policies for an
undecodable id in one package and ~16 handlers correct only because a repo Get("") happens to fail.
Returning (string, bool) makes the ambiguity unrepresentable, and the compiler forces each of the
~22 sites to decide. URL params share one itemIDParam helper that 404s; id lists go through
DecodeIDs, which is all-or-nothing because dropping bad entries would empty a list and make its
len() > 0 filter gate vanish — the original bug by another route.
A well-formed but unknown id is still 200 with zero results; only malformed ids 404. Malformed
ids now also 404 on the image and similar/instant-mix routes, which previously answered with a
placeholder or an empty list.
|