mirror of
https://github.com/navidrome/navidrome.git
synced 2026-10-08 10:27:08 +02:00
5,036 commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
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
|
||
|
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.
|
||
|
Deluan Quintão
|
5a4a3099f1 |
fix(cli): accept absolute paths in selective scan --target (#5947)
* fix(scanner): accept absolute paths in selective scan --target The scanner's fs.FS only accepts paths relative to the library root, so an absolute --target path (e.g. 2:/jukebox/collection) failed with an opaque "invalid argument" error. Rebase absolute targets onto the library root before scanning; relative paths are unchanged. Fixes #5943 * refactor(scanner): simplify libraryRelativePath with IsLocal and slice.ToMap * fix(scanner): make libraryRelativePath cross-platform Windows CI failed: the tests hardcoded Unix-style absolute paths, which are not absolute on Windows, and filepath.Rel yields backslash-separated paths that the io/fs-based scanner FS rejects. Build the test paths with filepath.Abs so they are absolute on every OS, and normalize the rebased result with filepath.ToSlash. * fix(scanner): resolve relative library root before rebasing target filepath.Rel cannot rebase an absolute target onto a relative library root (e.g. the default MusicFolder=./music), so an absolute --target was left unchanged and rejected by the rooted io/fs. Make the library root absolute first; it resolves against the same cwd as the scanner's fs. |
||
|
Deluan Quintão
|
752b38609c |
feat(artist): add Share and Download actions to the Artist detail page (#5944)
* feat(ui): add Share button to artist detail page * feat(ui): add Download button to artist detail page * fix(ui): scope artist share/download to album-artist content Gate the artist Share/Download actions on album-artist stats and show the album-artist size, since ZipArtist and the share query only cover album_artist_id songs. Previously the total (role-inclusive) size was shown and guest-only artists could produce an empty archive. Applies to the artist toolbar, the shared context menu, and the download dialog title. * fix: match artist download/share to album-artist participation ZipArtist and the artist share query filtered the deprecated album_artist_id column, which only stores the first album artist of a track. Secondary album-artists (co-credited but not first) got an empty download/share even though the UI offered it. Filter by the album-artist role participation instead, matching the artist's album-artist stats used to gate the actions. Also cover the artist-specific size branch of the download dialog. * fix(share): scope artist shares to the owner's libraries The artist share query broadened to album-artist participation, which could pull a secondary album artist's tracks from libraries the (non-admin) share owner cannot access into the public share. Load the artist share as the owner so their library access is applied, mirroring how playlist shares already work. Adds a repository test covering co-album-artist inclusion and library scoping. * test(share): assert album participation branch of artist shares Link the co-album-artist fixtures to albums and assert share.Albums (used by Subsonic getShares) includes the accessible album and excludes the one in a library the owner cannot access, so the album participation + scoping branch is covered too. * fix: exclude missing files from artist download/share actions An artist's stats still count files that went missing, so the toolbar/context menu could offer Download/Share for an artist whose files are all gone, while the share query (missing=false) returns nothing and downloads open dead paths. Hide the actions when the artist is missing and exclude missing files from ZipArtist, matching the share semantics. * refactor: dedupe artist download-size and share-owner lookups Extract the 'album-artist download size (or none when missing)' rule into a single artistDownloadSize() helper shared by the toolbar, context menu, and download dialog, and factor the duplicated share-owner context lookup into a shareRepository.ownerContext() method used by both the artist and playlist share cases. * refactor(ui): move artistDownloadSize helper to common utils is for domain-agnostic, potentially portable code; this helper is Navidrome-specific (artist stats shape), so it belongs in common. Consumers import it directly from common/artist to avoid pulling in the common barrel. |
||
|
Deluan
|
8978c7b9fa |
Revert "feat(jellyfin): send a synthetic placeholder blurhash for unresolved artwork (#5941)"
This reverts commit
|
||
|
Deluan Quintão
|
036c9cab96 |
feat(jellyfin): send a synthetic placeholder blurhash for unresolved artwork (#5941)
* refactor(artwork): let callers pin the blurhash component counts * feat(artwork): synthesize a unique placeholder blurhash from a seed * fix(artwork): base the synthetic-hash no-collision guarantee on the prefix, not length The prior comment and test claimed a synthetic value could never collide with a real one because it's always shorter. That's false: components() targets ~16 tiles by scaling one axis down as the other hits the 9 cap, so an extreme aspect ratio (e.g. 10x200) collapses to 1x9 = 9 components, which encodes to the same 22 characters as a 3x3 synthetic hash. The old test only exercised a square 64x64 gradient, so it never caught this. The real guarantee is structural, not length-based: the first character encodes shape as (xComp-1)+(yComp-1)*9, and components() derives xf*yf = 16 exactly before flooring/capping (xf = sqrt(16w/h), yf = xf*h/w = sqrt(16h/w), so xf*yf = sqrt(256) = 16). Two factors both in [2,3) can't multiply to 16, so Encode can never derive 3x3 - the synthetic prefix 'K' is structurally exclusive to Synthetic. Replaced the length-based test with one that sweeps extreme aspect ratios and asserts no real encode ever produces prefix 'K', alongside the assertion that Synthetic always does. * feat(artwork): tint a synthetic blurhash from a base colour Parses baseColor into HSL and clamps saturation/lightness so the tint stays muted; per-cell hashing (unchanged) is what keeps a shared tint across an album's tracks from colliding, as the new 1M-seed spec proves under a single fixed colour. * fix(artwork): trim over-long comment in synthetic blurhash test Review flagged the collision spec's comment for exceeding the 2-line budget; the Finamp rationale it restated already lives in the design doc and commit history. * feat(jellyfin): send a synthetic blurhash for unresolved artwork Clients render nothing where a placeholder belongs when ImageBlurHashes is omitted for pending artwork. primaryImage now synthesizes a value (seeded on the tag, so a cover swap re-keys it) whenever a tag is present but no blurhash has been computed yet. Known-absent artwork (ImageAbsent) is unaffected: it still emits neither tag nor blurhash, since GetOrPlaceholder would otherwise pin a shared placeholder under a distinct cache key for a year. * fix(jellyfin): avoid computing a synthetic blurhash when a real one exists cmp.Or evaluates both arguments before choosing between them, so blurhash.Synthetic ran (and was discarded) on every call even when img.BlurHash was already set. That's the common case in production: artwork_repository.go populates BlurHash from persisted values once a scan resolves it, so a healthy library paid the synthesis cost (xxh3 hashing, HSL conversion, image alloc, DCT encode) on every mapped item for a value it never used. Branch on emptiness first instead. * feat(jellyfin): tint a pending track's placeholder with its album's colour primaryImage never reaches the embeddedArtPending branch of SongToBaseItem, since there is no resolved image yet to feed it. Seed the synthetic blurhash on mf.ID (so the value stays unique and the client still issues the read-through request) but tint it with the album's DominantColor, which is already hydrated on MediaFile at no extra cost. * style(artwork): trim synthetic blurhash comments to the budget * docs(jellyfin): fix stale blurhash README bullet + two review nits The "Blurhashes are synthetic" bullet under Known limitations described dto/blurhash.go, which was deleted when the real core/artwork-computed blurhash + synthetic-fallback pipeline landed; every claim in it was false. Replaced it with an accurate paragraph in the Images section, since the described behaviour is now the finished design, not a gap. Also: fix a doc/body comment mismatch in blurhash.go (component counts are 2..9, not 1..9), and deduplicate the inline DC-extraction logic in synthetic_test.go by reusing the existing dcOf helper. * refactor(artwork): slice the synthetic cell jitter on byte boundaries The three perturbations came off one hash with mismatched masks and shifts (0xFF at 0, 0x3F at 8, 0x3F at 14), so a reader had to do the arithmetic to confirm the fields did not overlap. Only 20 of the 64 bits were in use either way, so the narrower fields bought nothing. Uniform byte slices at 0/8/16 are non-overlapping by inspection and give each field the full 8 bits. Both 1,000,000-seed collision specs still measure 1,000,000 distinct values. Also drops the local `n` alias, which was a second name for synthComponents inside a 15-line function. * docs(jellyfin): fix the blurhash paragraph's opening sentence It opened with "follow the same principle", pointing back at the preceding paragraph on admin-context artwork resolution — an unrelated subject, so the reader looks for a connection that is not there. * fix(artwork): render the synthetic grid larger than its component count The 3x3 cell grid was handed straight to the encoder as a 3x3 image, so the source had exactly as many samples as basis functions. Blurhash normalises its coefficients by 1/(w*h) and 2/(w*h), which assumes many samples per component, so the AC terms came out far too large. At the bottom-right corner the x and y bases are both [1, -0.5, -0.5], everything lines up negative, and the result clamped to black — a dark blob on every synthetic placeholder. Rendering the same nine colours bilinearly at 8x8 first removes it: measured over three seeds, the darkest corner goes from 13 to 74 and the darkest pixel from 1 to 63. 8px is the smallest size that clears the artefact; 12 and 16 are visually indistinguishable and cost 1.7x and 2.7x more. The interpolation is hand-rolled rather than x/image's scaler, which allocated 528 times per call against 14 for this. Both 1,000,000-seed collision specs still measure 1,000,000 distinct values. * refactor(artwork): tidy the synthetic upscale helpers cellWeight nudged its upper bound with a 1e-9 epsilon so int() could never land on the last cell. Clamping the coordinate and then the index says the same thing without a magic constant, and makes the clamp-don't-extrapolate intent explicit — edge pixels map outside the cell centres, so the fraction would otherwise run past 1. The grid type is spelled once as colorGrid rather than repeated in the local and the upscale signature, and encodeAt's doc now states the source-size contract that Synthetic depends on, so the next caller sees it at the function rather than only in synthetic.go. Output is unchanged: all seven sample hashes match byte for byte. * perf(artwork): make the synthetic upscale separable Both axes are square and constant-sized, so the per-pixel cell index and weight were the same 8 values recomputed 64 times per call. They are now built once by sync.OnceValue, matching the srgbToLinearTable pattern. The interpolation is also separable: stretching each of the 3 grid rows horizontally once and then blending rows vertically does 264 lerps where the per-pixel form did 576. upscale drops from 347ns to 260ns, Synthetic from 1780ns to 1669ns. Output is byte-identical across all seven sample hashes — same operations in the same order, only hoisted. * refactor(artwork): let a caller supply the cosine basis encodeAt built its cosine tables inline, so Synthetic rebuilt bit-identical ones on every call — its shape is always 3 components over 8 pixels. Splitting the table construction into cosBasis and the encoder proper into encodePixels lets Synthetic build the basis once via sync.OnceValue, and leaves encodeAt's signature and behaviour untouched. Synthetic drops from 14 allocations to 6 (1669ns to 1508ns). The time saving is small and this path only runs while artwork is still unresolved; the allocation cut is the point, alongside a shorter encodeAt. Every hash is unchanged: nine real Encode outputs spanning square, 10x200, 200x10, 1x50 and a real JPEG, plus all seven synthetic samples, all byte for byte identical before and after. |
||
|
Deluan Quintão
|
9e95b19a4f |
feat(artwork): make the artwork image size cap configurable (#5931)
* feat(artwork): make the artwork image size cap configurable Replace the hardcoded 20MB cap on resolved image reads with a new MaxImageSize config option. Load floors it at MaxImageUploadSize so an accepted upload can never be too large for the resolver to read back. * fix(conf): reject zero-valued byte-size options at startup ParseBytes accepts "0", but parseSize silently substitutes the default for it, so the accepted config would differ from the effective limit. * fix(conf): reject byte-size options that overflow int64 A raw value above math.MaxInt64 parses as a valid uint64 but wraps to a negative int64 in parseSize, giving readCapped a non-positive LimitReader bound so every artwork read comes back empty. |
||
|
Deluan
|
c4126fa674 |
fix(jellyfin): emit SortName for artists and albums, honoring PreferSortTags
Finamp's A-Z fast scroll re-derives each item's sort key client-side from SortName, paging until it reaches the tapped letter. We only emitted SortName for songs, so Finamp fell back to reconstructing a key from the display name, which diverges from the order_* key the list is actually sorted by (curly quotes, non-English articles). The scan then believed it had passed the letter and scrolled back to the top instead of loading further pages. Emit SortName for artists and albums using the same key the persistence layer sorts by, via a sortName helper that mirrors Subsonic's PreferSortTags handling: order_* names by default, sort tags when the config is enabled. The song mapper now honors the config too, instead of always preferring the sort tag. Also parse Fields in the /Artists* handlers, which ignored the parameter entirely, so no field-gated data could ever be returned there. |
||
|
Deluan Quintão
|
95b8d9dd04 |
perf(genre): index genre filtering via join tables across all APIs (#5940)
Filtering by genre scanned every media_file/album row and JSON-parsed its `tags` column (a per-row json_tree(tags) EXISTS) with no usable index, so Finamp's genre screen took 1.9-6.5s per tap against a ~97k-track library. Album and album-artist genre queries had the same unindexed shape. Add normalized media_file_tags and album_tags join tables (genre only for now, via an indexedTagNames allowlist), populated by a new updateTags in Put (mirroring updateParticipants) and backfilled in the migration. Genre filtering across the Jellyfin, Subsonic and native APIs now runs as an index-backed semi-join through shared TagIDSemiJoin/TagNameSemiJoin helpers instead of a full scan. On a copy of the production DB the per-request cost for a typical genre drops from ~330ms to sub-millisecond, adding ~10MB. |
||
|
Deluan
|
8e0ff1a235 |
fix(jellyfin): resolve genre id as a MusicGenre item
The Jellyfin /Items/{id} endpoint resolved albums, artists, songs and
playlists by id but not genres, so a genre id returned 404. Finamp's
genre "See all" fetches the genre as the track list's parent item, and
that 404 crashed its screen to a blank page after the tracks flashed in.
Add a Genre lookup to resolveItemByID (backed by a new GenreRepository.Get)
so a genre id returns its MusicGenre BaseItemDto, matching real Jellyfin.
|
||
|
Deluan Quintão
|
d080707060 |
fix(jellyfin): return a mixed, globally-limited list for multi-type /Items requests (#5935)
* feat(jellyfin): allow random sort for artist/genre/playlist item types * feat(jellyfin): add round-robin interleave helper for merged item types * fix(jellyfin): mix and globally limit multi-type Items requests Run per-type queries in parallel and round-robin interleave the results so a request for multiple IncludeItemTypes (e.g. Finamp's random favorite) returns a mixed, globally-limited page instead of one type's rows followed by the next. * fix(jellyfin): dedupe repeated IncludeItemTypes to avoid duplicate items and redundant queries * refactor(jellyfin): dedupe via slice.Unique and extract queryTypeWindow helper * perf(jellyfin): serve random multi-type pages from offset 0 A random merge reshuffles every request, so paginating it is meaningless — page N is just another fresh draw (as in real Jellyfin). Serving from offset 0 caps the per-type fetch at limit instead of offset+limit, avoiding deep-offset blow-up for the random case (Finamp's random-favorite quick action). * refactor(jellyfin): resolve random-merge via applySort; simplify merge signatures Detect the random-page shortcut by resolving each type's sort through applySort (matching how the sort is actually chosen) instead of string-matching SortBy, and only when every type is random. Drop the always-zero window param from mergeTypesStreaming and derive the window inside mergeTypesPaged. |
||
|
Deluan Quintão
|
7736bbb545 |
fix(cache): write the completion marker before closing the cache writer (#5927)
* fix(cache): write the completion marker before closing the cache writer Readers of an in-progress cache write see EOF the moment the writer closes, but the .complete marker was created after the close, on the background goroutine — so a fully-read stream did not mean the cache was done touching disk. The new artwork precache spec ends right at EOF, and its GinkgoT().TempDir() cleanup raced the marker creation, failing the Windows CI job with 'unlinkat ...: The directory is not empty' (the race also reproduces on macOS, 2 of 3 runs, with the tightened test). Writing the marker after a clean copy but before Close makes reader-EOF imply every on-disk write for the entry is finished. A failed writer Close still invalidates the entry, which removes both the marker and the data file. The existing marker test now asserts the marker exists immediately at EOF instead of Eventually. * fix(artwork): never dispatch queue items after the drain context is cancelled The 10x Windows stress run for the previous commit surfaced a second flake in the same package: 'leaves undispatched items queued when cancelled mid-batch' lost row alc7 in 4 of 10 runs. In drain, when a semaphore slot is free and the context is already cancelled, both cases of the blocking select are ready and Go picks one at random — so a cancelled drain could still dispatch items. A non-blocking Done check before the select gives cancellation priority. The race was invisible on Linux/macOS only by accident: the spec seeded the album repo with a single album (each SetData overwrote the last), so only the final row (alc7) resolved to absent and got deleted when dispatched; the others fell on the retry path and survived. Nanosecond enqueue timestamps made alc0 always first out of the mock dequeue, masking the race, while Windows' coarse clock ties the timestamps and randomizes the order. The spec now seeds all eight albums, which made the race reproduce locally on the first try (row alc0) and now guards the fix on every platform. * test: give cache-init waits a 10s timeout for loaded CI runners A 10x parallel Windows stress run timed out one artwork spec in BeforeEach: the FileCache init goroutine (mkdir + reload walk) took over Gomega's default 1s Eventually timeout under shared-runner disk contention. Bump the three identical init waits (two artwork suites and the utils/cache helper) to 10s. * test(scanner): widen watcher debounce margins for loaded CI runners The watcher debouncing spec asserts 'no scan yet' inside 20ms Consistently windows while the debounce wait was only 50ms — a 2.5x margin that a loaded Windows runner blows through by delaying the timer-reset notification, firing the scan early (failed all three FlakeAttempts in a 10x stress run). Raise the test debounce wait to 200ms (10x the observation windows) and the scan-fired Eventually timeouts to 2s to match. * refactor(artwork): collapse drain cancellation into a single exit path Replace the non-blocking ctx pre-check plus duplicated select exit with one select and a ctx.Err() check after it. Besides removing the duplication, this closes the residual race: a cancellation landing between the two selects could still let the blocking select randomly pick the free semaphore slot and dispatch the item. Now a dispatch is only possible when the context was live after slot acquisition. * style: trim flaky-test fix comments to single lines Compress each two-line comment added by this PR to the one line that carries the invariant; drop the narration around it. |
||
|
Deluan Quintão
|
7993fb9158 |
perf(persistence): use *_artists join tables for artist participant filters (#5930)
* perf(persistence): use *_artists join tables for artist participant filters The artist_id/artists_id, role_<role>_id and role_total_id filters, plus the AlbumsByArtistID/AlbumsByContributingArtistID/SongsByArtistID helpers, scanned every album or media_file row through json_tree(participants, ...), which no index can serve — the cause of multi-second artist pages on large libraries (discussion #5929). Rewrite them to semi-join the album_artists and media_file_artists tables via a shared ParticipantIDFilter helper. The join tables are written in the same transaction as the participants JSON, so results are unchanged. album_artists' unique constraint led with album_id, so artist-driven lookups had no usable index. Rebuild the table with the constraint reordered to (artist_id, album_id, role, sub_role), mirroring media_file_artists, instead of adding a fourth index: measured within 4% of a dedicated covering index (geomean -92.5% vs json_tree on a 96k-track production copy) while saving ~7MiB and per-scan write amplification. Album-side consumers (participant rewrites, FK cascades, markMissing) keep using album_artists_album_id, and updateParticipants' ON CONFLICT target already names artist_id first. The rebuild is linear work: 1.1s on a 113k-row production copy. * chore(gitignore): add temp benchmark files to ignore list * fix(persistence): clear album_artists when an album is saved without participants albumRepository.Put skipped updateParticipants when the Participants map was empty, so a hypothetical save with no participants would write {} to the JSON column but leave stale album_artists rows behind, now visible through the semi-join filters. No current caller can hit this (albums built by MediaFiles.ToAlbum always have participants), but make Put unconditional anyway, matching mediaFileRepository.Put, so the join table always moves with the JSON. Raised by Codex review on #5930. |
||
|
Deluan Quintão
|
944ca3100f |
feat(artwork): new artwork pipeline with background resolution and Low Quality Image Placeholders (#5847)
* feat(artwork): add artwork, item_artwork and artwork_queue tables * feat(artwork): add artwork models, repository interfaces and mocks * feat(artwork): implement artwork repository * feat(artwork): implement item_artwork repository with batched hydration * feat(artwork): implement artwork_queue repository * feat(artwork): add content-addressed originals store * feat(artwork): add artwork prune (orphan cleanup) * fix(artwork): never sweep files on transient DB errors during prune * refactor(artwork): fold originals package into core/artwork as ImageStore * refactor(artwork): merge item artwork state into ArtworkRepository * fix(artwork): chunk unbounded IN clauses and restore interface docs * refactor(artwork): apply simplify-pass cleanups Internal item_artwork sqlRepository helper, toSQLArgs upserts, batched queue enqueue, EnqueueStaleAbsent moved to queue repo, snapshot-based prune sweep, mock/real semantics aligned. * fix(artwork): address review findings on prune/sweep races and mock fidelity Sweep now honors an mtime grace window (in-flight acquisitions and temp files), reacquired orphans reset the prune grace window, and the queue mock implements real stale-absent semantics. * fix(artwork): atomic orphan deletion and timestamp semantics from review DeleteOrphans re-checks age+references at delete time, PutItemArtwork defaults attempted_at, queue mock timestamps mirror SQL. * fix(artwork): guard orphan file removal with the prune grace window Duplicate ImageStore writes refresh the file mtime and Remove skips files newer than the cutoff, so overlapping acquisitions cannot lose their store files to a concurrent prune. * fix(artwork): rewrite vanished duplicates and sweep stale mime variants Write falls through to a real write when the liveness touch fails, and sweep retention now matches the recorded mime's extension so obsolete variants are reclaimed. * fix(artwork): index artwork_queue in dequeue order The previous leading retry_at range column forced a temp B-tree sort of the whole eligible set on every DequeueBatch; ordering the index by (priority DESC, enqueued_at) lets scans stop after the batch size. * fix(artwork): honor the orphan cutoff in the repository mock The mock's DeleteOrphans now applies createdBefore like the SQL implementation, and a new spec covers a freshly reacquired row surviving prune. * fix(artwork): reject malformed hashes in ImageStore operations Known-absent states carry an empty hash and malformed persisted hashes could panic path sharding or inject separators; Write/Open/Remove now return an error for anything but 16 lowercase hex chars. * fix(artwork): mock PutImage refreshes created_at like the SQL repository Prune specs now age fixtures directly instead of seeding stale timestamps through the upsert. * fix(artwork): store backing-file provenance per item, not per hash * feat(artwork): import blurhash encoder from #5797 * feat(artwork): add worker-side artwork resolvers * fix(artwork): propagate playlist tile failures and dedupe external step * feat(artwork): add acquisition processor Resolves one queue item end to end: hash/dedup, decode + 128px thumbnail blurhash, place bytes (store vs source file), and persist found/absent/ failed state for the worker (Task 4) to act on. * style(artwork): tighten processor comments to budget * feat(artwork): add acquisition worker service * feat(artwork): enqueue artwork resolution from scan and CRUD paths * feat(artwork): artwork backfill, fingerprint re-resolution and scheduled jobs * test(artwork): leak/soak coverage and deferred assertions * fix(artwork): propagate transient artist image errors to the worker callGetImage swallowed all agent errors, so an agent outage surfaced as ErrNotFound and the worker settled artist artwork as a definitive absent (and reset the breaker). Add an additive ArtistImageResult path that returns the underlying agent error on transient failure while keeping ArtistImage byte-identical for existing callers; the worker's artist external step uses it via fromArtistExternalResult. * fix(artwork): resolve full playlist source chain resolvePlaylist only built the generated grid, dropping the uploaded-image, sidecar and ExternalImageURL sources the old reader_playlist.go chain serves. Port the full chain before the grid fallback: uploaded (upload), sidecar (folder), and ExternalImageURL routed through extGate with the same extError semantics as the other external steps. Also rewires the artist external step onto ArtistImageResult. * fix(artwork): purge dangling queue rows and guard concurrent re-enqueues Queue rows for deleted entities failed forever (Get -> ErrNotFound -> failed -> capped retries, unbounded). Add ArtworkQueueRepository.PurgeDangling, called from Prune next to the item_artwork purge. Separately, the found/absent path unconditionally deleted the dequeued row, erasing a concurrent scan re-enqueue; switch to DeleteIfUnchanged, which deletes only while retry_at still matches the dequeued value (verified retry_at is the column an Enqueue upsert resets). * style(artwork): fix comment accuracy and budget; fingerprint ArtistImageFolder Correct the inverted workerDeps.extGate comment, trim over-budget doc comments, and add conf.Server.ArtistImageFolder to the resolution fingerprint so an image-folder change re-resolves artist artwork. * fix(artwork): treat missing local playlist cover as definitive, not transient A playlist ExternalImageURL pointing at a local file that fails to open was routed through extError, causing failed/48h-retry loops that burn a rate limiter token forever instead of falling through to the generated grid. * refactor(artwork): deduplicate purge loop, backfill table, and extGate alias * fix(artwork): cap resolved image reads A user-editable ExternalImageURL can point at an arbitrarily large endpoint; a fast server could make the worker buffer hundreds of MB inside the 5s HTTP timeout. Bound the read to a fixed 20MB cap (no config knob) via io.LimitReader and fail the item if it is exceeded. * fix(artwork): retry higher-priority external art after fallback hit With CoverArtPriority="external,cover.jpg", a transient external failure followed by a folder hit dropped the external error: the worker recorded found and deleted the queue row, so the configured higher-priority external art was never retried. Carry extError onto the fallback resolution and add an outcomeFoundStale that persists+serves the art but reschedules via MarkFailed, giving the external source another chance. When external later answers definitively-not-found, the hit is not stale and the row is deleted. * fix(artwork): treat playlist cover URL 404 as definitive miss The playlist ExternalImageURL step used sources.go's fromURL, which maps any non-200 to a generic error, so a stale URL returning 404/410 was classified transient: infinite backoff plus it counted toward the circuit breaker, blocking valid external work. Add a local fetch in resolve.go that maps 404/410 to model.ErrNotFound (definitive) while keeping other non-200s transient. sources.go is left untouched. * test(artwork): move soak test into the Ginkgo suite * test(artwork): make leak and permission tests pass on linux goleak now ignores notify's nonrecursive-tree goroutines (linux uses inotify, which spawns dispatch+internal instead of darwin's recursive dispatch), and the read-only-dir prune spec skips under root, where permission bits cannot make Remove fail. * fix(artwork): reject decompression-bomb dimensions before decoding * fix(artwork): keep fresh re-enqueues ahead of stale failure backoff * fix(artwork): include M3U external art flag in the config fingerprint * fix(artwork): resolve private playlists with an admin context * test(artwork): convert non-synctest timing tests to Ginkgo specs TestArtworkBackoffSchedule and TestArtworkWorkerRunNoLeak needed no real *testing.T (no synctest), so move them into worker_test.go as Ginkgo specs. TestArtworkBreakerHalfOpen stays plain since testing/synctest requires a real *testing.T, matching core/scrobbler's precedent. * fix(artwork): store backing-file provenance per item, not per hash * fix(artwork): apply image limits to playlist tile decoding decodeTile ran image.Decode on every sampled album's resolved bytes before processItem's maxImageBytes/maxImagePixels guards applied, letting an oversized or decompression-bomb tile fully decode unbounded. Enforce both caps inside decodeTile itself. * refactor(artwork): reuse auth.WithAdminUser and dedupe image cap guards * perf(artwork): fetch only IDs for backfill enumeration Backfill enumerated every album, artist, playlist and radio via GetAll and mapped out just the ID. GetAll materializes full entities (library joins, participant/stats/tags JSON, annotation, artwork hydration), so on a large library it loaded tens of thousands of heavy structs only to read one field each — spiking transient RSS to ~1GB during the one-time upgrade backfill, a memory risk on small NAS/Pi hardware. Add GetAllIDs to the album, artist, playlist and radio repositories: it reuses each repo's base row-set filter (library visibility, artist content join, playlist userFilter) but projects only id, skipping the heavy columns and post-processing. A per-repo parity test asserts GetAllIDs returns exactly the same id set as GetAll. Verified on a 727MB / 29k-artist production DB copy: peak RSS during backfill dropped from ~1012MB to ~89MB, file descriptors flat, same 36,138 items enqueued. * feat(artwork): promote worker concurrency and external rate to real configs The artwork worker's drain speed was governed by two hidden Dev flags, DevArtworkWorkerConcurrency and DevArtworkExternalRPS, both defaulting to 2. On a large library's one-time backfill the external rate limiter is the real ceiling: every art-less item waits on it before the (rate-limited) external lookup, so the drain crawls at ~RPS items/sec while local-art items are unaffected. Promote both to documented, supported options: ArtworkWorkerConcurrency (default 4) sets local-resolution parallelism, ArtworkExternalMaxRPS (default 2, 0 = unlimited) caps external-agent lookups to stay polite to Last.fm/Deezer/etc. Operators can now trade first-backfill speed against external-API rate limits. The old Dev names still map for backward compat. * fix(deezer): never return empty-image-id placeholder pictures * feat(agents): enumerate enabled image-retriever agents per capability * feat(artwork): worker fetches agent images directly with per-agent rate limits and breakers * fix(artwork): treat agent not-found as breaker success * feat(model): content-hash artwork id suffix and hydratable per-entity image state * feat(persistence): hydrate artwork hash and absence onto entity pages * feat(artwork): resolve media_file embedded art in the worker, invalidate on rescan * feat(artwork): broadcast refresh events when artwork lands * fix(artwork): broadcast refresh for stale-found artwork too * feat(artwork): state-backed serving path with provisional read-through * feat(server): serve artwork from persisted state with content-hash caching * feat(subsonic): content-hash coverArt ids, omit artwork on known-absent * refactor(artwork): delete the legacy reader chain, cache warmer, and provider image methods * feat(artwork): precache on acquisition, bump on upload/radio changes, manual re-resolve API * test(artwork): end-to-end coverage for the serving cutover * chore(artwork): generic 500 bodies on refresh endpoint, trim stale test comments * fix(artwork): request read-through must not reset the failure backoff The provisional read-through and dangling re-enqueue used Enqueue, whose upsert resets retry_at, so any browse of an unresolved entity that was backing off after an external failure made it immediately eligible again — defeating the exponential backoff during a provider outage. Add EnqueueBump, which raises priority but leaves an existing row's retry_at intact, and route the serving path through it. Scan and manual re-resolve keep Enqueue's reset (a detected change wants immediate retry). * fix(artwork): keep an eligible track's cover requestable when its album is absent An embedded-eligible track with no resolved item_artwork row inherited the album's ImageAbsent, so when the album resolved absent (e.g. CoverArtPriority without 'embedded') the track's coverArt was omitted permanently — the client never requested it, so the lazy mediafile path never resolved it — even though the serving path would extract and serve the track's own embedded art. Hydration now never copies the album's absence onto an eligible-but-unresolved track. * fix(artwork): validate each agent image URL before picking the largest bestImageURL selected the largest by size and only then parsed it, so a malformed largest URL (e.g. a bad percent-escape) returned nil and shadowed a valid smaller candidate, contradicting the documented skip-unparseable behavior. Parse per candidate and compare sizes only among URLs that parse. * fix(artwork): fall back to disc art, not the album, for multi-disc tracks serveMediaFile delegated an absent/ineligible track straight to AlbumCoverArtID, skipping the disc-specific lookup that MediaFile.CoverArtID (and the deleted legacy reader) use. On multi-disc albums with per-disc images that served the album cover instead of the configured disc artwork. Delegate through DiscCoverArtID. * fix(artwork): enqueue uploaded artwork only after the filename is persisted SetImage cleared state and enqueued the bump before the caller stored the new filename, so a worker drain in that window could resolve against the old (already deleted) file and settle absent, leaving the upload unused until a later scan. Move the invalidate+enqueue into EnqueueArtwork, which each caller now invokes after the entity Put. * fix(artwork): keep multi-disc tracks requestable when the album is absent Round-1's hydration fix still copied the album's known-absent onto a non-eligible (or own-absent) track, but MediaFile.CoverArtID routes a multi-disc track to disc art, which resolves provisionally and is never known-absent. Marking it absent made Subsonic omit coverArt so clients never requested a valid disc image. Only mark a single-disc track absent, and only when its own art won't resolve. * fix(artwork): serve a local playlist ExternalImageURL as a file-backed reference A local ExternalImageURL was resolved through the external step and labelled external, so placeBytes copied it into the content-addressed store and dropped its path/mtime — replacing the file never tripped the staleness check. Classify local references as file-backed (resolved in place, even on the request path) and keep store-backed behaviour only for http(s) URLs. * fix(artwork): requeue playlist cover when its track set changes A generated-grid cover went stale after track mutations: nothing re-resolved the playlist's artwork, and the request path deliberately never rebuilds the grid, so serveEntity kept returning the old grid hash indefinitely. Enqueue pl artwork from refreshCounters (the choke point for every track-set change); no clear, so the old cover keeps serving until the worker rebuilds. * fix(artwork): open library-backed artwork through its on-disk root A library configured with a file:// path stored absRoot as the raw URI, so Abs produced strings like file:/music/cover.jpg that os.Open/os.Stat reject — folder, upload and embedded art were treated as dangling on every request, looping forever. Normalize a file:// path to its parsed OS path (the same root os.DirFS uses); non-local schemes are left unchanged (out of scope, per the artwork-musicfs TODO). * fix(artwork): only use disc resolution for multi-disc albums DiscCoverArtID returns a dc- id for any track with DiscNumber>0, so serveDisc ran the full DiscArtPriority chain even for single-disc albums, where a stray disc*/ embedded image could shadow higher-priority album art. Gate disc resolution on the album having more than one disc, matching the legacy reader; single-disc tracks serve album art directly. * fix(artwork): invalidate artwork when an uploaded image is deleted Deleting an artist/radio/playlist upload cleared the filename but left the found item_artwork row and its hash, so lists kept advertising the deleted cover's hash-suffixed immutable URL and clients could display it indefinitely. Call EnqueueArtwork after the delete-side Put, symmetric with upload, so the state is cleared and re-resolved to the next source (or absent). * fix(artwork): restore synthetic-artist guard and unicode normalization in agent lookups Moving agent calls into the worker bypassed two behaviors of the aggregate provider: Agents.GetArtistImages' guard for Unknown/Various Artists (a direct retriever call could assign an unrelated image to a synthetic artist), and auxAlbum/auxArtist.Name's DevPreserveUnicodeInExternalCalls normalization (records with typographic quotes/dashes missed exact-name searches). Re-apply both before enumerating retrievers. * fix(artwork): enforce entity visibility on the Subsonic getCoverArt path serveEntity reads persisted item_artwork by id, bypassing the library and private- playlist filters that the legacy entity-load applied. On the authenticated Subsonic path a user could fetch artwork for an inaccessible album or someone else's private playlist by guessing an id. getCoverArt now resolves the underlying entity through the request-scoped (filtered) repositories and serves the placeholder when it is not visible, so existence isn't leaked and the always-an-image invariant holds. The public share (JWT-authorized) and Jellyfin (admin) paths are intentionally untouched. * fix(artwork): version the artwork ETag with the served representation The ETag was the pixel hash of the original image, so a CoverArtQuality or EnableWebPEncoding change altered the resized bytes without changing the ETag — revalidating clients got a spurious 304 and kept the old encoding. Resized responses now carry a representation ETag (hash + size + square + encode settings) used for the ETag header and If-None-Match, while the immutable decision stays on the pixel hash (URLs remain pixel-identity per the spec, so hash-suffixed clients keep zero-request caching). Full-size originals fall back to the pixel hash as before. * fix(artwork): don't stamp the album hash onto multi-disc tracks The hydration fallback assigned a found album hash to every fallback track, but a multi-disc track's CoverArtID emits a dc- id served from disc-specific art whose hash is unknown at hydration time. Advertising dc-..._<albumHash> gave clients a content- version that never changes when the disc image does, breaking id-based refresh. Only stamp the album hash for single-disc tracks (DiscNumber == 0); multi-disc tracks stay unhashed and rely on the correct ETag returned by the served response. * fix(artwork): enqueue new empty playlists by id, and refresh on absent outcomes Two worker/enqueue fixes from review: - playlistRepository.Put assigned the generated id to the caller's Playlist but passed the stale copy (empty id) to refreshCounters, enqueueing a pl|"" row the worker failed until the daily dangling purge while the real playlist went unresolved. Set the id on the copy before enqueueing. - The drain refresh batch only included found/foundStale, so a cover removed by a scan (found -> absent) never notified clients, leaving the old immutable image displayed. Broadcast absent outcomes too; precache still only warms found/foundStale. * fix(artwork): honor disabled per-track art at serve time; use nanosecond mtime provenance Two serving-correctness fixes from review: - serveMediaFile served a persisted mf embedded image even after EnableMediaFileCoverArt was turned off (the setting isn't in the config fingerprint, so found rows aren't reprocessed). Direct mf- URLs now honor the setting at serve time and fall back to disc/album art. - The file-backed staleness check compared whole-second mtimes, so a same-second content replacement (two writes in one second, or timestamp-preserving tools) could serve different bytes under the old hash + immutable policy. RefMtime is now unix-nanoseconds (no schema change; int64 column), detecting sub-second changes where the filesystem records them. * fix(artwork): preserve the drive when normalizing Windows file:// library paths url.Parse puts the volume of file://C:/Music in Host, not Path, so localOSRoot dropped it and returned /Music — os.Open/os.Stat then failed and folder/embedded art on Windows looped as dangling. Rejoin the host volume, matching core/storage/local's newLocalStorage. * fix(artwork): clamp negative sizes to full-size; convert imghttp test to Ginkgo - A negative size (Subsonic size / Jellyfin maxwidth accept signed ints) reached resizeStaticImage, where the square path builds image.NewNRGBA(Rect(0,0,size,size)) — a giant rectangle that panics/OOMs. Clamp size<0 to 0 (full-size) at the Service entry. Positive sizes were already clamped to the original. - imghttp used a plain func Test with a table; convert to a Ginkgo DescribeTable with the suite entry point in imghttp_suite_test.go (AGENTS.md test-framework requirement). * test(artwork): use renamed ArtworkWorkerConcurrency in e2e tests * feat(artwork): carry blurhash through item image hydration * feat(nativeapi): expose artwork hash, absence and blurhash * feat(artwork): hydrate the parent album's artwork state onto tracks * test(artwork): add hydrateArtwork regression guard for AlbumImage wiring Drives hydrateArtwork itself (not applyItemImage directly) over tracks that take each of the loop's continue branches, so a future edit moving the AlbumImage fill below a continue would fail loudly instead of passing silently. * feat(jellyfin): version album and artist image tags by content hash * fix(jellyfin): trim primaryImageTag comment to why-only, within budget * feat(jellyfin): emit real blurhashes and drop the synthesized fallback * feat(ui): version cover art urls by content hash and skip absent art * feat(ui): add BlurHashCanvas placeholder component * fix(ui): clear stale blurhash pixels and assert the draw path in tests Clear the canvas before each decode attempt so a hash change that fails to decode doesn't leave the previous frame's pixels on screen once this wires into a list that recycles items. Also strengthen the specs to assert createImageData/putImageData were actually invoked (and with what), instead of only checking that a <canvas> element exists. * feat(ui): show the blurhash while an album cover loads * fix(artwork): hydrate cursor streams via an id pre-pass The album, artist and playlist GetCursor built their own select and never called hydrateArtwork, so every Jellyfin list endpoint (all six stream via GetCursor) emitted entity-id image tags and no blurhash. Only GetAll hydrated, which is why Subsonic and the native API were unaffected. Each cursor now resolves its ordered/filtered/paginated id set with the cheap id-only GetAllIDs query, then streams those ids in chunks through the repo's existing GetAll, which already hydrates and applies the full select. Max/Offset are consumed by the pre-pass alone; the chunk query carries only the caller's filters, Sort and Order. This also removes a pre-existing deep-pagination cost: keeping OFFSET out of the joined query makes the pre-pass a covering index scan instead of paying the library and annotation joins for every skipped row. Benchmarked on a synthetic 100k-album DB with the real schema, page=500 at offset 90,000: 3.9ms via the id pre-pass, 52.5ms for the current shape, 192.7ms for a naive join. An unpaginated full stream costs ~24% more, which is the trade. GetAllIDs gains the annotation join whenever the caller's filters or sort reference an annotation column (same gate CountAll uses), otherwise Filters=IsFavorite and SortBy=PlayCount would fail in the pre-pass. The playlist pre-pass repeats GetAll's columns so ORDER BY keeps resolving to playlist.name rather than the joined user.name. * fix(jellyfin): hydrate artwork on the song cursor Jellyfin's listSongs streamed media files via GetCursor, which never hydrates artwork, so songs emitted entity-id image tags and no blurhash. media_file now uses the same id pre-pass as the other three cursors (album/artist/playlist), for consistency, but on a separate method, GetCursorWithArtwork: GetCursor itself must stay untouched, since it's also the scanner's hot path and the scanner never reads artwork. Measured on 1,000,000 tracks, the pre-pass over all ids costs +41.8 MB heap and +298 ms versus GetCursor's bounded +0.0 MB. The Jellyfin path is paginated, though, so in practice it only ever pre-passes a page's worth of ids, not the full library, and doesn't pay that cost. * feat(jellyfin): emit a song's own cover art when it differs from the album's Real Jellyfin fills ImageTags from each item's own images before falling back to the parent album, and Finamp checks imageTags.Primary before AlbumId. Our mapper read only the album's image, so a track with distinct embedded art silently showed the album cover. Emit exactly one entry under ImageBlurHashes.Primary: Go marshals map[string]string in sorted key order rather than insertion order, so a second entry could pair the wrong blurhash with the image imageId resolves to, and Finamp pins that pairing in its cache for 365 days. * test(persistence): scope the GetCursorWithArtwork full-stream spec to tie-free ids The fixture has title ties (e.g. three "Antenna" tracks), so the unscoped positional comparison against GetAll only passed because SQLite's tie order happened to coincide between the full scan and the pre-pass's id IN (...) fetch. Scope it to onlySongs like the sibling ordering specs already do. * refactor(artwork): route song own-art through primaryImageTag; align chunk size Cleanups surfaced by /simplify: the song mapper's own-art branch reimplemented primaryImageTag's tag+blurhash-map construction (and its one-entry invariant) — route it through the helper so that invariant lives in one place. Tie artworkChunkSize to a whole multiple of artworkBatchSize so a cursor page re-chunks into even hydration batches. Hoist a duplicated imageLoading && blurHash boolean in the album grid. * fix(ui): serve the placeholder for known-absent art instead of a broken icon getCoverArtUrl returned '' for an imageAbsent record, so <img src={undefined}> rendered as the browser's broken-image icon on every absent cover. The server already serves a proper placeholder for absent art, so build the url and let it render. * feat(ui): show the blurhash as the loading placeholder across cover surfaces Add a shared CoverImage component (useImageUrl blob cache + blurhash + fade) and render the blurhash while a cover loads on the list thumbnails (CoverArtAvatar, radio) and the artist/album/playlist detail pages. The detail pages now go through CoverImage instead of a plain CardMedia, so their images come from the in-memory blob cache and survive React remounts without re-fetching. BlurHashCanvas gains an optional style prop. * refactor(ui): unify list cover surfaces onto the shared CoverImage component Route the album grid, CoverArtAvatar (artist/playlist lists) and the radio list's cover field through CoverImage instead of each carrying its own useImageUrl + blurhash-overlay wiring. CoverImage gains a default object-fit: cover. Radio keeps its uploaded-image gate and the generic radio placeholder for stations with no art. * fix(ui): address CoverImage review findings Restructure CoverImage so the size/shape lives on the root and the blurhash + image are absolute fills: the <img> mounts only once its blob is ready, so an unresolved cover never flashes a broken <img>. Add a fit prop (default cover) so album/playlist detail keep their letterbox instead of being cropped by a hardcoded object-fit. Remove the orphaned coverLoading styles and an unused subsonic import; add a CoverImage unit test. * perf(ui): only refetch already-loaded records on SSE refresh The artwork worker broadcasts a RefreshResource event per resolved chunk, carrying every id in the chunk. useResourceRefresh was doing a getMany for all of them, so any open list/detail page fetched hundreds of artists it was not displaying. Filter the event ids to records already in the store; the rest load fresh (with their new artwork) when navigated to. * refactor(artwork): scale worker concurrency with CPU count ArtworkWorkerConcurrency now defaults to max(2, NumCPU()/2) instead of a fixed 4, mirroring MaxOpenConns: local resolution scales with the host but stays at half the SQLite pool so it never starves the scanner/UI. External RPS stays a fixed 2 — it gates third-party API calls and is bounded by their tolerance, not the host, so it must not scale with CPUs. Also drop the DevArtworkWorkerConcurrency/DevArtworkExternalRPS deprecated aliases: those names were never released, so there is nothing to migrate. * feat(artwork): re-queue an absent cover when its page is viewed serveEntity now schedules a Bump recheck for an entity whose art was recorded absent, so viewing a missing cover re-triggers resolution (e.g. after an external source that was down during the scan comes back), matching the request-time bump that already covers never-resolved entities. Throttled by attempted_at against requestRecheckAge (1h) so repeatedly opening a genuinely-absent page can't hammer external services. EnqueueBump preserves an existing failed-state backoff via MAX(priority,...) and inserts a fresh, immediately-eligible recheck for a settled-absent row (whose queue row was already deleted). * refactor(artwork): move ImageUploadService to artwork.Uploader Relocate the image-upload service from core to core/artwork as artwork.Uploader, co-locating it with the resolver/worker/serving that own the artwork state it invalidates. MaxImageUploadSize moves too — its only callers are the two image-upload handlers — which lets core/image_upload.go be deleted entirely. Extract the shared "clear resolved state + re-queue at Bump" invalidation into artwork.Refresh and fold nativeapi's refreshArtwork handler onto it, removing the duplicated DeleteForItem+Enqueue block that had drifted into three places. The wire provider moves from core's set to artwork's; the playlists.ImageUploadService binding moves to the top-level injector so core/artwork stays unaware of playlists. Behavior is unchanged. * refactor(artwork): thread model.Kind through the artwork API Entity-level artwork queries now take a typed model.Kind instead of a bare prefix string. GetItemArtwork, DeleteForItem(s), GetInfoForItems, EnqueueStaleAbsent, hydrateItemImages, enqueueBackfillKind and artwork.Refresh convert to the prefix string only at the two real boundaries: the SQL item_kind column (kind.Prefix() inside each repo) and external string inputs (a new model.ParseKind for the nativeapi URL param, which also validates it). The Backfill/stale-absent kind slices, the resolve.go dispatch switch, and the kind→resource / kind→table lookup maps now use the Kind vars directly. The queue lifecycle methods (MarkFailed/Delete*) keep string kinds — they operate on a dequeued item's raw ItemKind column, which stays a string field, always populated via kind.Prefix(). Removes every bare "al"/"ar"/… prefix literal from non-test code (27 -> 0); behavior is unchanged. * tune(artwork): drop backoff base from 5m to 15s The exponential retry (base × 4^attempts, cap 48h) started at 5 minutes, so a single transient failure — a timeout under load, an external blip — parked a cover for 5 minutes even though a retry seconds later would have resolved it. Start at 15s instead: transient failures recover almost immediately (15s → 1m → 4m → 16m …), while persistent failures still escalate to the 48h cap (now at the 8th attempt instead of the 5th). * tune(artwork): 5s backoff base + 12h give-up, drop the cap Retry backoff now starts at 5s (was 15s) so a transient failure recovers on essentially the next drain, and jitter widens to ±40% so a wave of correlated failures doesn't re-clump into one poll. Add a 12h give-up budget measured from enqueued_at: once the next backoff would land past it, the worker stops retrying instead of grinding at a cap forever. A bare failure settles absent (handed to the 24h stale-absent sweep, and still recoverable on a page view); a found-stale keeps its already-served art. The budget bounds the tail, so the separate 48h backoffCap is removed. * fix(lastfm): match album.getInfo on name+artist only, not MBID Last.fm's album.getInfo by MBID is unreliable: a correct MBID can return a different album, or none. Observed with black midi's "7-eleven" (whose correct MBID returned a FLEETWOOD release) and both missing The Chats albums (one MBID 404s, the other resolves to a different self-titled release). The worker then recorded covers absent — or would fetch the wrong art — even though the correct cover is on Last.fm by name+artist. Stop passing the MBID to album.getInfo; query by name+artist only, which also drops the now-dead error-6 MBID-retry fallback. The low-level client keeps its MBID support for other callers; only the album lookup changes. * fix(lastfm): return agents.ErrNotFound on error 6 (not found) Last.fm returns error 6 for a missing artist/album — a definitive negative — but the agent returned the raw *lastFMError, so the artwork worker treated every not-found as a real fault: it counted toward the per-source circuit breaker (5 in a row opens it, fast-failing all Last.fm calls including valid ones) and was retried as a transient error instead of settling absent. On a first scan of a library with many artists Last.fm lacks, this stalled valid cover lookups and left entities churning in backoff. Translate error 6 to the shared agents.ErrNotFound at the agent boundary (callAlbumGetInfo / callArtistGetInfo), matching how the Deezer agent maps its client's not-found, and log it at Debug instead of Error — which also removes the not-found log spam. * feat(artwork): log external image-lookup failures at debug The worker's res.reader==nil && extError branch returned outcomeFailed with no log, so a failing external cover lookup (agent error, dead image URL, download timeout) was undiagnosable. Log the agent, entity, and underlying error at the fetch site where it's in hand — this surfaced a Last.fm album.getInfo returning an image URL that itself 404s. * fix(artwork): treat a 404/410 image URL as not-found, not a transient fault An agent (notably Last.fm's album.getInfo) can advertise a cover URL that is itself dead — a 404. sources.go's fromURL returned a generic error for any non-200, so a dead URL was treated as a transient failure: it churned in backoff and counted toward the circuit breaker, stalling valid lookups. Map 404/410 to model.ErrNotFound in fromURL so a dead URL settles absent, and collapse the near-identical fetchPlaylistImageURL (which already did this for M3U covers) into it. * feat(artwork): make artwork re-resolution targeted, not blunt Two gaps in when the pipeline re-resolves artwork: The recheck job only requeued absent-state rows (hash=''), so an entity that was never processed — added between scans, or on a server with the scanner disabled — had no periodic safety net and stayed without artwork indefinitely. Add EnqueueMissing(kind): a SQL set-difference enqueueing entities with no item_artwork row at Recheck priority (ON CONFLICT DO NOTHING, so it never disturbs a queued row). Run it once at startup and hourly alongside the stale-absent recheck. Rename staleAbsentKinds -> recheckKinds accordingly. Conversely, the config fingerprint included consts.Version, which embeds the git SHA and so changed on every build, re-enqueueing every entity in the library (~34k here) and re-querying external agents at the configured RPS for anything without local art. Replace it with an explicit artworkEpoch constant, bumped deliberately when resolution semantics change. The cases that motivated the version input — absent art becoming available — are already covered by the stale-absent and missing-row rechecks; only a corrected wrong-pick needs the epoch. A test guards against reintroducing the version. * test(artwork): restore resolution edge-case e2e coverage The serving cutover removed the album/disc/artist/mediafile/playlist/radio e2e specs that documented the folder-selection rules and guarded the #5376/#5456/ #5451/#5457 regressions; nothing replaced them, so compareImageFiles and the parent-fallback logic were left untested. Restore them driving the real pipeline: a real scanner populates the folder graph from an in-memory library, the real Worker drains the queue, and the real Service serves. Folder-backed art is file-backed (served via os.Open, which the in-memory FS can't satisfy) so its selection is asserted on the persisted state row; store-backed and real-disk sources are asserted byte-for-byte. Single-disc disc resolution now serves album art directly, so only multi-disc disc scenarios are ported. * fix(artwork): run disc resolution for single-disc albums too |
||
|
Deluan Quintão
|
5c5b849a4e |
docs(plugins): sync plugins README with the implementation (#5912)
* docs(plugins): document the Matcher host service The Matcher host service (matcher_matchsongs, permission 'matcher') was missing from the README entirely. Add its section with permission example, the matcher-requires-library rule, result ordering/scoping semantics, and a Go usage example. * docs(plugins): add missing nd_scrobbler_playback_report to Scrobbler The Scrobbler capability has four required methods, but the README said three and omitted nd_scrobbler_playback_report from the function table. Add the table row and document the PlaybackReport input payload and its possible playback states. * docs(plugins): remove stale coverartarchive-as example row The AssemblyScript coverartarchive-as example no longer exists in plugins/examples/ (and no AssemblyScript code remains in the repo), so drop its row from the examples table. * docs(plugins): add Command Line Interface section The 'navidrome plugin' CLI (list, info, validate, enable, disable, edit, rescan) was undocumented; the README's only mention was a single inline 'plugin edit --write-access' reference, and it claimed plugins could only be enabled/disabled via the UI. Add a CLI section with all subcommands and the 'plugin edit' flags, and link it from Runtime Management. * docs(plugins): add pdk and types packages to Go PDK table The Go PDK package table was missing two of the packages that ship under plugins/pdk/go/: 'types' (shared DTOs used across capabilities and host services) and 'pdk' (the extism/go-pdk wrapper the README's own examples already import). * docs(plugins): correct capabilities and host services in examples table Several example rows understated what the plugins implement: the scheduler/websocket callback capabilities were omitted for nowplaying-py, library-inspector-rs, crypto-ticker, and discord-rich-presence-rs, and the Discord example also uses the Config host service. * docs(plugins): state that scheduler/taskqueue permissions require callbacks Manifest validation rejects a plugin that declares the scheduler or taskqueue permission without exporting nd_scheduler_callback or nd_task_execute, but the README worded both callbacks as soft suggestions (TaskWorker was even labeled optional). Make the load-time failure explicit in both capability sections. * docs(plugins): document HTTP, WebSocket, and Task limits and defaults Add the runtime limits plugins actually hit: HTTP default 10s timeout, 5-redirect cap with per-hop host re-validation, and 10MB response cap; the 30s timeout on WebSocket callbacks; and the full task_createqueue parameter list (delayMs, retentionMs) with queue defaults, retention bounds, the 1MB payload cap, and task persistence across restarts. * docs(plugins): include scrobbleRetriever in user-scoped authorization note The ScrobbleRetriever host service added in #5795 is user-scoped like subsonicapi and scrobbler, but the Security section's user-scoped authorization item didn't list it. * docs(plugins): clarify matcher user scoping requires the users permission The Matcher usage example set opts.Username while the section's manifest example declares only matcher+library; without the users permission no users can be assigned to the plugin, so userAccess.resolve rejects the call at runtime. Make the example unscoped and state that user scoping requires the users permission with users assigned. Raised by Codex review on PR #5912. |
||
|
Kendall Garner
|
b0c6d2e444 |
feat(plugins): add scrobbles access to PDK (#5795)
* initial scrobble api * feat: add scrobble retrieval api * address feedback (1) * fix spelling * be explicit about get * add primary key field, update index, remove rowid references * use unix timestamp for input and output * initial api, some testing * add tests, add count retrieval * add docs, test for rejected user * add permission validation for scrobble retriever * chore(plugins): fix typos in scrobble retriever Rename newScrobbleRetreverService, and fix FromTImestamp/nonero in the ScrobbleRetriever doc comments, which generate into the Go and Rust PDKs. Also corrects two mislabelled test entries. * fix(plugins): make scrobble pagination order deterministic Sorting only by submission_time left the order of equal timestamps up to the query planner, but the cursor skips ties by offset, so an unstable order can repeat or drop scrobbles between pages. Break ties on scrobbles.id, which the existing scrobbles_user_time index already yields for free. Descending is now honoured for every combination of From/To rather than only when both or neither is set. This changes the default for a lone ToTimestamp from newest-first to oldest-first. * refactor(plugins): return the next page's options from GetScrobbles Paging previously meant reading NextTimestamp and Cursor off the response and deciding where each belonged: NextTimestamp into FromTimestamp when ascending or ToTimestamp when descending, and Cursor copied every time, including when 0. Both are silent data-loss bugs when a plugin gets them wrong. GetScrobbles now returns the options for the following page, or nil when the range is exhausted, so a plugin passes the value straight back and repeats. ScrobbleCursor and ScrobbleList are gone; the query itself is unchanged. * docs(plugins): warn against setting ScrobbleOptions.Offset manually The all-ties carry rule assumes Offset counts already-returned rows at the boundary timestamp, which only holds for the options GetScrobbles returns. A hand-built From+Offset combination can silently skip scrobbles, so document the field as managed pagination state instead of a generic skip. * docs(plugins): document the ScrobbleRetriever host service in the README Covers the manifest permissions (including the users requirement), the four host functions, the options/ref field tables, and the pagination loop with its two gotchas: the host-managed offset and the adjusted range on the returned next options. * chore(plugins): regenerate scrobble retriever stub with nil-safe accessors --------- Co-authored-by: Deluan Quintão <deluan@navidrome.org> |
||
|
Deluan Quintão
|
5a5311a9c0 |
fix(plugins): make generated mock stubs nil-safe for nilable returns (#5909)
The ndpgen mock accessors asserted return types unconditionally, so a plugin test using Return(nil, ...) — the natural way to model an empty result or a pagination terminal page — panicked on the untyped-nil type assertion instead of returning the zero value. Guard pointer, slice, map and any returns in both the host-service client stubs and the PDK stub, and regenerate. |
||
|
ant
|
a0bf78cdea |
fix(contrib): added missing hyphen in OpenRC script that caused crashes on startup (#5906)
Signed-off-by: Ant <marxguey@proton.me> |
||
|
Deluan Quintão
|
0f4c9b8212 |
fix(ui): don't start playback when closing the disc cover lightbox (#5901)
The Lightbox renders through a React portal (react-modal), and React propagates synthetic events up the React tree rather than the DOM tree, so clicking its close button or backdrop bubbled into the ancestor TableRow and triggered playSubset. The existing timestamp guard could not catch this: react-image-lightbox defers onCloseRequest by animationDuration via setTimeout, so lightboxClosedAt was only stamped 200ms after the click had already propagated. The guard is kept because it still covers the separate mobile ghost-click path, where the synthesized click lands on the row after the overlay unmounts. Stopping propagation in onCloseRequest — the approach used by ContextMenus, whose MUI onClose fires synchronously — does not work for the same reason. Instead, wrap the Lightbox in an element that stops propagation at the React tree boundary the portal bubbles through, which also covers the lightbox controls we don't own (zoom buttons, caption, image drag). |