* fix(scanner): respect FollowSymlinks in watcher-triggered scans
With FollowSymlinks disabled, creating a folder symlink inside the library
made the watcher schedule a selective scan with the link itself as the
target. walkDirTree only checked FollowSymlinks for child entries, so it
walked the link and imported its files as duplicates (or, for links that
point outside the library, files that should never be scanned).
walkDirTree now skips any target folder whose path, or any parent folder,
is a symlink when FollowSymlinks is disabled. The skipped target stays in
lastUpdates, so rows previously imported through it are marked missing,
matching what a full scan does. localFS now implements fs.ReadLinkFS so
fs.Lstat can see symlinks instead of following them.
Fixes#6292
* test(scanner): run the #6292 symlinked target tests on Windows
Remove the SkipOnWindows guard from the symlinked target folder tests, so
the go-windows CI job covers the FollowSymlinks fix for selective scans.
Playlist().Tracks returns nil when its internal Get fails (for example when the
context is canceled at shutdown), and resolvePlaylist called GetAlbumIDs on it,
panicking with a nil pointer dereference. The artwork drain runs on a bare
goroutine, so the panic killed the whole server.
resolvePlaylist now returns an error when Tracks is nil, and the worker recovers
panics per item: it logs the panic with the item details and stack, and marks
the item as a failed attempt so the rest of the batch still runs.
Fixes#6266
* fix(ui): make playlist toggle switches visible in all themes
The Public and Auto-import switches in the playlist list did not set a
color, so Material-UI used the theme's secondary color. Many themes use
secondary as a surface color close to the table background, which made
checked switches nearly invisible (Catppuccin, Rosé Pine, Monokai,
Moonbase and others).
Set color="primary" on the playlist switch, like every other switch in
the app, and make primary the default MuiSwitch color in useCurrentTheme
so future switches cannot regress. Fixes#6272.
* refactor(ui): drop secondary switch overrides from themes
Dracula, Gruvbox Dark, Tokyo Night and Tokyo Night Light styled checked
MuiSwitch colorSecondary to work around the same invisible-switch problem
(Gruvbox in #5064). With primary as the default switch color and every
switch in the app using it, no switch renders with colorSecondary anymore,
so these overrides are dead code.
* fix(share): reuse cached transcodes when streaming from share links
Public share streams built the stream request with only the share's format
and bit rate, leaving sample rate, bit depth and channels at zero. Regular
playback resolves those through the transcode decider (e.g. 48000 Hz for
Opus), and they are part of the transcoding cache key, so a track already
transcoded during normal playback was transcoded again into a separate,
identical cache entry when played through a share link.
The public router now resolves share stream requests with the same
TranscodeDecider.ResolveRequest used by the Subsonic stream endpoint, so
both paths produce the same request and share cache entries.
Fixes#6261
* fix(archiver): reuse cached transcodes when zipping downloads
Zip downloads (album, artist, playlist and share) built the stream request
with only the format and bit rate, leaving sample rate, bit depth and
channels at zero. Those are part of the transcoding cache key, so a track
already transcoded for playback was transcoded again into a separate cache
entry when downloaded in a zip, and vice versa.
The archiver now resolves each request with TranscodeDecider.ResolveRequest,
the same as single-song downloads and streams. This also applies the
decider's defaults, so a zip requested without a bit rate uses the target
format's default bit rate instead of leaving it to ffmpeg.
* fix(archiver): name zip entries after the resolved transcoding format
The transcode decider can pick a different format than the one requested
(for example a player's forced transcoding, or a fallback to the default
downsampling format when the requested one can't be produced). Zip entry
names and the playlist M3U were still built from the requested format, so
an entry could end in .mp3 or .flac while holding Opus data.
Each track's request is now resolved before its entry name is built, and
the name uses the resolved format.
* feat(model): add per-library PID config columns
* refactor(metadata): pass PID config to ToMediaFile and add spec validation
* feat(scanner): rescan only libraries whose PID config changed
* feat(server): validate library PID config and rescan on change
* feat(ui): edit per-library PID config
* fix(ui): label the PID mode selects
* fix: tighten per-library PID rescan edge cases
An interrupted PID rescan no longer upgrades every library to a full scan, a save that loses the race for the scanner logs at debug, the confirm dialog only shows when the effective PID spec changes, and it now gets translation keys.
* refactor(metadata): pass the library to ToMediaFile
ToMediaFile and core.Inspect took the library ID and its PID config as
separate arguments, so a caller could mix values from two libraries. They
now take the model.Library and resolve the effective PID config from it.
* chore: tidy per-library PID comments, PropTypes and migration
Trim comments that restated the code, add PropTypes to the new UI
components, and recreate the migration with make migration-sql.
* fix(ui): show the PID spec help under its input
* feat(cmd): make inspect use the file's library PID config
inspect always used the global PID config, so it showed different IDs than
the scanner for files in a library with an override. It now finds the
file's library in the DB and uses its effective config, falling back to
the global config when there is no DB or the file is outside every
library. It never creates a DB. The library path matcher moves from
core/playlists to model so both can use it.
* refactor: simplify per-library PID code
Share the DB-file check between CLI commands, move ErrAlreadyScanning to
model so core no longer imports scanner, read the libraries once for
insights, and let ValidatePIDSpec accept an empty spec and look tags up
directly. In the scanner, use FullScanInProgress instead of a second
flag, and skip recomputing album IDs when the album spec did not change.
In the UI, share the PID inputs between Create and Edit, and use docsUrl.
* feat(ui): add section titles to Library Create and pre-fill Custom PID specs
Custom now starts from the global spec, so admins edit a working spec
instead of typing one from scratch.
* fix(inspect): map files with the library-relative path the scanner uses
Inspect gave metadata the file's directory as typed, so folder-based PIDs
never matched the DB. It now uses the path relative to the library root,
through the scanner's helper, which moves to model.
* fix(scanner): say when a PID rescan only covers target folders
* fix: reject tag aliases in album PID specs and match root libraries
Tags are stored under canonical names, so an alias in a spec always reads
as empty. In an album spec that gives every album the same ID, so album
specs now require the tag name. Track specs keep accepting aliases, since
the default one uses them. LibraryMatcher now matches paths under a
library at the filesystem root.
* refactor(model): move the tag alias lookup to tag_mappings.go
* test: run the library matcher and inspect tests on Windows
Build test paths with filepath instead of Unix literals, so they use the
OS separator like filepath.Abs output, and drop the Windows skips.
* feat(ui): add pt-BR translations for per-library PID settings
* fix(ui): don't crash playlist list rows that lost their record
react-admin 3 evicts records fetched more than 10 minutes ago whenever
another getList for the same resource completes, but the list keeps its
cached ids. The Datagrid then renders those rows with an undefined record,
and the Public and Auto-import switches crashed reading record.id. This
happened when the playlist list was left open and the sidebar or the add
to playlist dialog reloaded a smaller set of playlists.
Both switches now render nothing when the row has no record; the next list
refresh fills the row in again.
* refactor(ui): merge playlist list toggles into one ToggleField
The Public and Auto-import switches were copies that differed only in the
field they flip. ToggleField now flips its source field, and
ToggleAutoImport just shows it for playlists that have a file path. The
tests render inside TestContext, so they use react-admin's real hooks
instead of mocks.
* fix: honor cover animation setting in Squiddies Glass
Fixes#5170
* fix(ui): move cover animation check into AlbumDetails
Apply a noCoverAnimation class from AlbumDetails when
enableCoverAnimation is off, so every theme gets the fix. Drop the
Squiddies Glass theme changes and its test, and cover the class in
AlbumDetails.test.jsx.
---------
Co-authored-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
Co-authored-by: Deluan Quintão <deluan@navidrome.org>
* fix(persistence): include co-credited album artists when adding an artist to a playlist - #6240
Signed-off-by: Dawid Krynski <188586034+DawidKrynski@users.noreply.github.com>
* test(persistence): cover first album artist and track-artist-only in AddArtists
The joint track now uses a track artist that is not an album artist, and
the AddArtists specs check all three cases: the first album artist still
matches, a co-credited album artist matches, and a track-artist-only ID
adds nothing. The last case guards against widening the role filter.
---------
Signed-off-by: Dawid Krynski <188586034+DawidKrynski@users.noreply.github.com>
Co-authored-by: Dawid Krynski <188586034+DawidKrynski@users.noreply.github.com>
Co-authored-by: Deluan <deluan@navidrome.org>
* feat(persistence): store hashed API keys on players
* feat(core): refresh key-bound players without renaming them
Add Players.Touch, which records usage for a player already identified by
an API key without guessing its identity or overwriting its name. Register
also stops renaming players that have an API key.
Register no longer returns player save errors (or a stale FindMatch
ErrNotFound when the save is rate-limited); save failures are only logged,
and only the transcoding lookup error is returned, same as Touch.
* feat(subsonic): authenticate with OpenSubsonic API keys
Co-authored-by: amCap1712 <amCap1712@users.noreply.github.com>
* feat(subsonic): add tokenInfo and advertise apiKeyAuthentication
* feat(server): add endpoints to generate and revoke player API keys
* feat(ui): manage player API keys
Co-authored-by: amCap1712 <amCap1712@users.noreply.github.com>
* fix(subsonic): throttle API keys per key and IP
A stale key on one device exhausted the shared per-IP bucket and locked out
every valid key from the same IP. The limiter only stores a hash of the bucket
string, so the key is not retained. Also adds e2e coverage of API key auth
through the real repository, and clarifies the player resolution log message.
* fix(ui): keep the new API key dialog open until closed
The key is shown only once, so Escape and backdrop clicks no longer dismiss
it. Also clarifies when the key can be used as a password.
* refactor: simplify API key code paths
Share the player refresh tail between Register and Touch, fold the
ownership-filtered write tail into execOwned, parse the query once for
apiKey conflicts, derive HasAPIKey in the player mock, share the player
form inputs between create and edit, and pick the delete button by key
state instead of spreading conditional props.
* feat(players): set API keys through the player record
The key is a write-only apiKey field applied on save: required and owner-only on create, optional on edit, empty to revoke. Replaces the generate/revoke endpoints.
* fix(players): reject API keys already in use
Creating or editing a player with a key another player already has now returns a validation error instead of a 500, and a create that loses the race no longer leaves a keyless player behind. Ownership is checked before the key on create.
* feat(ui): edit player API keys as a form field
Replaces the show-once dialog, whose icon-less Close button was invisible on mobile. The key is generated in the browser, required and pre-filled on create.
* fix(ui): keep new player API keys out of the record cache
The json-server create response echoes the request body, and undoable edits merge the payload into the cache, so the key could reappear on the edit page. Strip it from the create result and save player edits pessimistically. Also fall back to a prompt when the clipboard write fails.
* fix(ui): polish player API key field
Set userId on the created player record so owner actions show immediately, and show a neutral no-key message to non-owners.
* refactor: simplify player API key create and field
Write the key hash in the create INSERT so the unique index settles
races, re-read the created player instead of hand-building the cached
record, reuse isWritable for the revoke check, and collapse the key
field's derived state and generate/regenerate buttons.
* fix(ui): let the API key field size like other inputs
fullWidth is now opt-in instead of forced.
* fix(ui): align the API key field with other player inputs
Apply react-admin's input className, move the actions (now including Copy) below the field, and use a monospace font so the whole key fits.
* fix(ui): redirect to the player list after create
Matches the other create pages.
* refactor(persistence): name the write-access rule for owned rows
Owned-row writes now say which row they target and who may write it: ownedRow(rowID, ownerOrAdmin|ownerOnly) builds the WHERE, updateOwnedRow applies it, and SetAPIKey uses ownerOnly instead of a hand-built user_id filter. updateOwned/deleteOwned keep their signatures.
* fix(players): apply an edit's key change and fields atomically
Update now runs SetAPIKey and the column update in one transaction. Also shares the key format check, drops FindByAPIKey's unneeded empty-key guard, and sets the context username only on the apiKey path.
* fix(subsonic): treat any credential param sent with apiKey as a conflict
The spec requires error 43 when u, p, t or s is present with apiKey, even with an empty value.
* refactor(subsonic): leave the player cookie code unchanged for key-bound requests
Return early instead of wrapping the cookie block, so the diff (and CodeQL's view of it) matches master.
* fix(subsonic): don't count key lookup errors as failed logins
A database error while checking a key sent as the password now surfaces as a server error instead of a bad password, so it no longer feeds the failed-login limiter.
* feat(players): use nds_ as the API key prefix
Part of a Navidrome secret prefix family (nd + a letter for the kind), alongside ndg_ for API v1 grants.
* feat(ui): make player API keys easier to find
Label the Settings menu entry "Players & API keys", add an API key
filter to the player list, show the key icon in the mobile list, and
add Brazilian Portuguese translations for the new player strings.
Signed-off-by: Deluan <deluan@navidrome.org>
* feat(ui): always show the player API key filter
Signed-off-by: Deluan <deluan@navidrome.org>
* fix(ui): hide the unset Last Seen date in the player list
Players created by hand have no last_seen yet, which showed as 12/31/1.
Signed-off-by: Deluan <deluan@navidrome.org>
---------
Signed-off-by: Deluan <deluan@navidrome.org>
Co-authored-by: amCap1712 <amCap1712@users.noreply.github.com>
When a startup step failed (for example, the port was already in use), runNavidrome only logged the error and returned. In service mode, service.Run() kept waiting for a stop signal, so the process stayed up serving nothing and the service manager never restarted it. A plain run exited with code 0.
runNavidrome now returns the error, unless its context was cancelled by a normal shutdown. Both the plain run and the service goroutine exit with code 1 on that error. The systemd unit no longer lists 1, 2 and 8 in SuccessExitStatus, so Restart=on-failure restarts the service on exit code 1.
Fixes#6235
The startup Configuration dump is rendered with pretty.Sprintf("%# v"), which
pads multi-line struct fields with spaces after the colon. The ApiKey and
Secret redaction patterns required the quote right after the colon, so
LastFM.ApiKey and LastFM.Secret were logged in clear text even with
EnableLogRedacting on. Allow optional whitespace after the colon, like the
other config patterns already do.
Prometheus.Password had no redaction pattern at all. Add one that also skips
escaped quotes, since the password can hold any character and pretty prints
it Go-quoted.
Add tests for the padded and unpadded forms, plus one that redacts a real
pretty.Sprintf dump of LastFM- and Prometheus-shaped structs so a padding
change in pretty can't bring the leak back.
Reported in https://github.com/navidrome/navidrome/discussions/6232
* feat(api): add OpenAPI v1 spec skeleton, lint ruleset and bundle tooling
vacuum v0.30.6's `bundle --composed` mangles component names for this
spec's multi-file layout (duplicates Problem as Problem__schemas etc.),
so api-bundle uses the Redocly CLI (npx @redocly/cli bundle) instead.
* fix(api): pin the Redocly CLI version
Tried moving components out of the root document (per libopenapi's
nested_files example) so vacuum's own bundler could produce clean
names, but any component declared via $ref inside components.* still
gets a __<parent>-suffixed twin regardless of collisions elsewhere, so
vacuum's --composed bundler can't cleanly bundle this spec. Pin the
already-working Redocly fallback to an exact version instead of
@latest.
* fix(api): bundle the OpenAPI spec with vacuum
vacuum's --composed bundler suffixes any component reached via a $ref
written directly inside the root document's own components.* block,
regardless of collisions elsewhere. Dropping the root-level schemas/
parameters/responses declarations (keeping only securitySchemes, and
leaving every component file under api/openapi/components/ untouched)
lets vacuum bundle cleanly with no __ suffixes, going back to Go-only
tooling. Components nothing references yet (ListMeta, offset, limit,
BadRequest, Unauthorized, Forbidden, NotFound) are absent from the
bundle until a later task's operation references them.
* fix(api): make spec lint rules cover all schemas and error codes
nd-schema-property-descriptions targeted $.components.schemas, but our
schemas live in path/response files, not the root document, so it was
dead code; switched to $..properties[*] to walk every resolved schema
wherever it ends up. nd-error-responses-are-problems only checked a
hardcoded status-code list; switched to a patternProperties schema
matching the full 4xx/5xx range. Also: api-diff now diffs against the
merge-base with API_DIFF_BASE (falling back to its tip with a notice
if no merge-base exists), gen no longer depends on api-gen until Task
3 wires up oapi-codegen, and api-lint suppresses vacuum's banner.
* feat(api): embed the bundled OpenAPI spec and expose its version
* feat(api): generate the v1 server interface with oapi-codegen
* feat(api): add RFC 9457 problem responses for API v1
* feat(api): add API v1 router with /server discovery and spec routes
* fix(api): serve the OpenAPI document without range support
* feat(api): mount API v1 behind the DevAPIv1 flag
* chore(ci): lint, regenerate and diff the OpenAPI v1 spec
* refactor(api): tighten spec version access, lint rules and test naming
* refactor(api): simplify spec routes, tests and OpenAPI tooling
Share one If-None-Match parser (utils/req) between the image and spec
routes, declare the YAML spec response as an object so tests need no
decoder override, and reuse ETag/304 spec components.
Install the OpenAPI tools only when missing or at a different version,
fail api-diff when its base ref does not exist, and in CI cache the
tools, fold regeneration into the go generate check, and fetch only the
PR base commit for the breaking-change gate.
* refactor(api): raise the list limit maximum to 2000 and drop the flag test
* feat(api): treat added enum values as non-breaking
Enums in API v1 are open: clients must accept unknown values. api-diff
now downgrades response-property-enum-value-added to INFO, while
removing a value from a request enum stays breaking.
* feat(api): gate breaking changes on x-stability-level
Every operation declares x-stability-level (alpha, beta, stable). oasdiff
ignores breaking changes to alpha operations and rejects lowering a
level, so unreleased endpoints can evolve while beta and stable ones
stay additive. All current operations start as alpha.
* feat(api): declare loginMethods as an enum
Prefix generated enum constants with their type name so enums sharing a
value (for example password) cannot collide in package apiv1.
* feat(api): send Allow on 405 and answer HEAD wherever GET is routed
chi only sets Allow in its default 405 handler, so the problem-format
handler now builds it by matching each method against the v1 router.
HEAD requests fall back to the GET route, as RFC 9110 expects.
* refactor(api): hash the spec ETag with xxh3
The bytes are compiled in, and the digest was truncated to 64 bits
anyway, so this matches the artwork ETags instead of paying for
cryptographic strength we discard.
* docs(api): explain the about:blank problem type
* feat(api): make code the problem identifier and omit a blank type
RFC 9457 says clients switch on the type URI, but no adopter surveyed
ships both a populated type and a separate code. Declare code as an
enum, and send type only once a problem has semantics of its own.
* fix(api): advertise the configured base path in the served OpenAPI spec
With BaseURL=/music the API is mounted at /music/api/v1, but the spec
told clients to call /api/v1 at the host root. The server now rewrites
servers[0].url to BasePath + /api/v1 when it serves the document.
Relative server URLs were tested first: "." and "../v1" work in
openapi-generator, Swagger UI and Redoc, but Scalar resolves them
against the page origin, so it breaks even without a base path. The
committed bundle keeps /api/v1, and a test pins that it appears exactly
once, which the rewrite relies on.
* refactor(scanner): remove the legacy ffmpeg metadata extractor
The ffmpeg extractor in scanner/metadata_old has not been wired into the scanner since the taglib-only rewrite, so it was only exercised by its own tests. Remove the package, the FFmpeg.Probe method and its ffmetadata command that only it used, and the startup fallback for Scanner.Extractor="ffmpeg". Configs that still set it keep working: unknown extractors already fall back to taglib with a warning.
* fix(conf): warn and fall back to taglib for an unknown Scanner.Extractor
Validate the option when loading the config, so invalid values such as the removed "ffmpeg" extractor are reported once at startup instead of only when a library storage is created.
Go 1.27 changed the built-in MIME type for .webm from audio/webm to video/webm, so the scanner stopped treating WebM files as audio after the Go bump in 0.64.0. Map .webm to audio/webm in mime_types.yaml so it no longer depends on the Go version.
* refactor(persistence): adopt generic deluan/rest repository API
Pin deluan/rest to the refactor branch. REST-facing repository methods
take a context and return typed values. Drop DataStore.Resource and
ResourceRepository; the native API names typed repositories directly
through a per-request adapter that later commits remove.
* refactor(persistence): base repository helpers take a context
* refactor(persistence): LibraryRepository takes a context per call
* refactor(persistence): PropertyRepository takes a context per call
* refactor(persistence): UserPropsRepository takes a context per call
* refactor(persistence): TranscodingRepository takes a context per call
* refactor(persistence): ShareRepository takes a context per call
* refactor(persistence): PlayerRepository takes a context per call
* refactor(persistence): RadioRepository takes a context per call
* refactor(persistence): PlayQueueRepository takes a context per call
* refactor(persistence): Tag and Genre repositories take a context per call
* refactor(persistence): PluginRepository takes a context per call
* refactor(persistence): Scrobble repositories take a context per call
* refactor(persistence): FolderRepository takes a context per call
* refactor(persistence): Artwork repositories take a context per call
* refactor(persistence): UserRepository takes a context per call
* refactor(persistence): ArtistRepository takes a context per call
ReadAll no longer rewrites the shared sort mappings for the role filter;
it works on a per-call copy.
* test(persistence): assert artist role sort sanitization in ReadAll
* refactor(persistence): AlbumRepository takes a context per call
* test(persistence): pass the test context to album repository helpers
* refactor(persistence): MediaFileRepository takes a context per call
* refactor(persistence): Playlist repositories take a context per call
* refactor(persistence): build all repositories once per store
* refactor(core): REST repository wrappers are built once
* refactor(persistence): repositories are stateless
Remove the context field from the base repository and the per-request
REST adapter. Enable the containedctx linter so no repository can hold a
request context again.
* chore(lint): skip containedctx in test files
* refactor: share simplifications from the stateless repositories sweep
Add deleteOwnedAll on sqlRepository and use it in player/share Delete
to remove the duplicated bulk-delete loop; have Share.Repository()
return model.ShareRepository so subsonic sharing.go drops its repeated
type assertions.
* chore(core): assert REST wrappers implement Persistable
* chore: reformat imports
* perf(persistence): build repositories on first use
Each transaction store used to construct all 21 repositories up front,
paying for filter and sort mapping setup the block never touched. Fields
are now sync.OnceValue thunks, so a store only builds what it uses.
* fix(persistence): clean plugin references per deleted user
A bulk user delete that fails on a later id had already removed the
earlier rows but skipped their plugin cleanup. Cleanup now runs right
after each successful delete.
* fix(core): unload disabled plugins even when a user delete fails
A bulk delete can fail on a later id after earlier users were removed
and their plugins auto-disabled. The wrapper returned before unloading,
leaving those plugins running until the next successful delete or a
restart.
* chore(deps): pin deluan/rest to v1.0.1
Replaces the pseudo-version of the refactor branch with the tagged
release. REST error messages now name the bare type (Artist, not
model.Artist).
* test: use the spec context instead of context.Background()
Replace the context.Background()/context.TODO() calls this branch added
to tests with the spec's ctx, GinkgoT().Context(), or t/b.Context(), so
repository calls are bound to the running spec's lifetime.
* test: declare the spec context once per Describe
Set ctx from GinkgoT().Context() first in each top-level BeforeEach and reuse it, building user contexts on top of it instead of repeating inline calls.
* feat(archiver): add folder cover image to downloaded zips
Album, artist, playlist and share downloads now include the item's cover as
folder.<ext>, the image most players and car stereos show for the files next to
it. This helps users who copy transcoded downloads to offline devices, since
transcoding drops the embedded artwork (#5841).
Album folders get the album cover, the artist zip root gets the artist image,
and playlist and share zips get the playlist or shared item's cover at the root.
The image is the same one getCoverArt serves, resized to 500px (not square),
and named by its detected type. Items without artwork get no image, and a cover
that fails to load is logged and skipped so it never breaks the archive.
The archiver reads covers through a new core.CoverArtReader interface,
implemented by artwork.CoverArtReader, to avoid an import cycle between core
and core/artwork.
* refactor(archiver): read covers through artwork.Artwork directly
The archiver no longer needs a local CoverArtReader interface and adapter.
The only reason core/artwork imported core was a core.AbsolutePath call in
loadArtistFolder, which now reads the library path from the repository and
cleans it the same way. With the import cycle gone, the archiver takes
artwork.Artwork and treats ErrUnavailable and ErrNotFound as no cover.
Covers are now written after each album's tracks (and after all tracks for
the archive root), so a slow artwork lookup does not delay the first bytes
of the download.
* fix(archiver): read share covers as admin so private playlists keep theirs
Public share downloads run with an anonymous context, and the playlist
repository hides private playlists from anonymous users, so a zip of a shared
private playlist silently had no folder image. Like the public image handler,
the share itself is the authorization: the cover lookup now runs with an admin
user. Only the cover read is elevated; streaming keeps the anonymous context,
so the transcode limiter still keys public downloads the same way.
* style(archiver): trim comments
Shorten the comments added by the folder cover image change and drop the ones the names already explain.
* fix(archiver): add one cover per album folder in artist zips
Albums with the same name share a zip folder (pre-existing naming), so an
artist zip with two such albums wrote two folder.<ext> entries at the same
path. Keep the first cover and skip the rest for that folder.
* refactor: add Tags.First and slice.GroupOrdered helpers
Tags.First returns the first value of a tag or an empty string, replacing the inline len-check-then-index pattern in FullTitle, FullAlbumName, Album.FullName and the Subsonic album version mapping.
slice.GroupOrdered is slice.Group returning the groups in first-seen order, for callers that need a deterministic order the map-based Group cannot give.
* fix(archiver): give same-named albums their own folder in artist zips
Artist zips put every album in a folder named after the album, so two albums with the same name (an original and a deluxe edition, or names that only differ in characters the sanitizer replaces) were merged into one folder, with tracks mixed together and duplicate zip entries when file names collided.
The folder is now named after FullAlbumName(), so with AppendAlbumVersion on (the default) the version is part of the name, matching what clients display. Albums whose sanitized names still clash get a " [suffix]" taken from the first field that has a distinct, non-empty value for all of them: album version, year, release type, record label, catalog number, then a short album id. This follows the shape of beets' %aunique{} path function.
Albums are also grouped with slice.GroupOrdered instead of a map, so the zip is deterministic.
* fix(archiver): use the release year to tell same-named albums apart
Taggers often write an edition's date to the Date tag next to an original date, and the scanner then stores the original year in Year and the edition's year in ReleaseYear. Reissues of the same album therefore share Year, so the year disambiguator could not tell them apart and they fell through to the album id suffix. Prefer ReleaseYear and fall back to Year when it is not set.
Found by downloading an artist zip from a live server built from this branch.
* fix(archiver): let one clashing album keep the plain folder name
A disambiguator was only accepted when every clashing album had a non-empty value, so an original and its deluxe edition (with the version not appended to the name) fell through to the album id suffix. Accept a field whose values are distinct across the group even when one of them is empty, as beets' %aunique{} does: that album keeps the plain name, which the suffixed folders cannot clash with. Two or more empty values still count as a tie.
* test(archiver): refactor tests for album naming conventions and query order
The plugin manager clears last_error on every startup with an UPDATE that takes the SQLite write lock even when no row matches. On slow storage the startup scan often holds the lock at that moment, so the reset waited out the busy timeout and logged "database is locked", even with no plugins installed. ClearErrors now checks for errors with a read first and only writes when there is something to clear.
* docs(jellyfin): refresh the README's known limitations
Rewrite the Known limitations list against the current code and Jellyfin
12.1, dropping stale entries (synthetic blurhash, unchecked artist access,
global genres) and adding the real gaps: search skipping filters, the
one-character minimum, the 2,000-item search cap, position-based playlist
entry ids, the rating param, missing endpoints and unemitted Fields.
Also move the lyrics description into its own section, add the missing
routes and filters to the endpoint table, and drop the stale artist-access
TODO in resolveItemByID.
* docs(jellyfin): list MaxConcurrentStreams env var and all e2e stubs
The dial-time SSRF guard runs on the resolved IP, so the tests that prove a
symbolic hostname cannot reach loopback used "localhost." — a trailing dot never
matches /etc/hosts, so Go queries real DNS. Machines whose resolver does not
answer "localhost." (a VPN DNS, for example) got "no such host" before the dial
guard ever ran, failing three specs.
Add tests.StubResolver, a net.Resolver backed by an in-memory DNS responder over
net.Pipe, and let the plugin dialers take a resolver so tests can inject it.
Name resolution in those specs no longer depends on the machine's DNS.
* fix(artwork): pause the artwork worker while a scan is running
The artwork worker added in 0.64 writes to the database continuously, including while a scan runs. On slow storage the scanner holds the write lock for many seconds per folder, so the two writers keep timing each other out: artwork writes fail with "database is locked", and a single busy timeout on the scanner side aborts the whole scan. The worker now stops dispatching queue items while scanner.IsScanning reports true, including mid-batch, and resumes on the next poll after the scan ends. Artwork requests are unaffected, since they serve local art without the worker.
* fix(db): run ANALYZE one index at a time so writers are not starved
A full ANALYZE is a single write transaction, so every other write waits for it to finish and fails after the 15s busy timeout. On slow NAS storage it was measured taking over 26 minutes. The analysis now runs ANALYZE per index (per table for unindexed and WITHOUT ROWID tables), which produces the same sqlite_stat1 rows as a full ANALYZE, and pauses briefly between steps (up to 150ms, just above SQLite's longest busy-handler sleep) so waiting writers get the lock.
* fix(scanner): ignore Synology @eaDir metadata folders
Synology creates an @eaDir folder next to media files, holding one subfolder per file with generated thumbnails. The scanner and watcher treated them as regular folders, which on one reported library added tens of thousands of extra folders to every scan.
* fix(db): analyze tables with only partial indexes as a whole
A partial index does not record the table's row count, so a table whose only indexes are partial needs a table-level ANALYZE to get the sqlite_stat1 row a full ANALYZE would write. Navidrome's schema has no such table today, but the stepped analysis should match a full ANALYZE for any schema a future migration creates.
* fix(scanner): retry busy folder saves and stop phase 1 on a fatal error
On slow storage, a single SQLITE_BUSY while saving a folder aborted the whole scan, even when another writer held the lock only briefly. The folder save now runs as a retryable unit: on a busy error it waits (5s, 10s, 15s) and reruns the transaction, up to three times, before failing. Side effects that do not survive a rollback (the album ID map consumed by persistAlbum, the artwork queue items, the image-change record) are rebuilt per attempt or recorded only after a successful commit.
When a folder save does fail, phase 1 used to keep walking the library and reading tags for every remaining folder, discarding the results, before reporting the error; a reporter saw 40 silent minutes. The walk now stops as soon as the save fails, and the walker honors cancellation instead of blocking on its channel. Because an early stop leaves folders unvisited, phase 1 no longer marks unvisited folders missing when the phase failed; the resumed scan handles them.
* refactor(persistence): move busy retry into DataStore.WithTxRetry
The scanner retried its folder save itself, which meant it had to know SQLite error codes. WithTxRetry now owns that policy: it reruns the block in a fresh transaction on SQLITE_BUSY, up to three times with growing delays, and runs it only once when already inside a transaction, since the outer transaction would still hold the lock. The block receives the context to use, and attempts that will be retried carry a marker so a busy statement in them is logged as a warning; only the final attempt logs errors. The scanner's inner error logs are folded into wrapped errors, so a recovered retry no longer prints error-level lines, and the folder path travels in the log context.
* fix(persistence): join the enclosing transaction in a nested WithTxRetry
Called on a store that is already inside a transaction, WithTxRetry went through WithTx, which opens a second, independent transaction on another connection. That transaction waits on the lock the outer one holds and fails with SQLITE_BUSY, and if it does succeed the outer transaction cannot roll it back. It now runs the block on the enclosing transaction, which owns the lock, the commit and the rollback. Found by a Codex (gpt-6-sol) review.
* fix(scanner): retry the remaining scan writes on a busy database
Every write step after phase 1 still aborted the whole scan on a single SQLITE_BUSY: phase 1 finalize, phase 2 moves and purge, phase 3 album saves and play count refreshes, the deferred playlist import flag, library ScanBegin, GC, the missing-artwork enqueue, tag counts, and the final library update. They now go through WithTxRetry. The phase 2 move had to be made rerun-safe first: it changed the target track's ID inside the transaction, so a rerun would have deleted the moved track itself, and it marked album annotations as handled even when the transaction rolled back. It now works on a copy per attempt and records the annotation reassignment only after a commit.
Artist.RefreshStats is left alone: it updates artists in batches outside a transaction, and one transaction around all of them would hold the write lock for the whole refresh on slow storage. Phase 4 playlist imports go through the playlist service and are left for a follow-up.
* fix(scanner): claim the album before moving its annotations
The rerun-safe moveMatched checked processedAlbumAnnotations before its transaction and marked the album only after the commit. Phase 2 runs same-library and cross-library moves in separate pipeline stages, so two moves into one album could both pass the check; the second would reassign annotations again and overwrite the album's created_at. The album is now claimed under the lock before the transaction, as the old code effectively did, and the claim is released if the move fails so a later move can still reassign. Found by a Codex (gpt-6-sol) review.
* fix(artwork): keep artwork housekeeping from writing during scans
The artwork worker already pauses while a scan runs, but its housekeeping jobs did not: the hourly missing-artwork recheck (a bulk INSERT ... SELECT over albums and artists), the startup run of the same recheck, and the daily prune all kept competing with the scanner for the write lock. They now run through LockForMaintenance, like the scheduled DB analysis: they skip while a scan is running and keep a scan from starting until they finish. Skipping the recheck loses nothing, since each scan with changes queues missing artwork at its end.
* refactor(scanner): log retried step errors once, from the caller
Blocks passed to WithTxRetry still logged their own errors at error level on every attempt, so a busy error that a retry absorbed printed several error lines (GC printed three). They now return wrapped errors and the callers, which already log them, report the final outcome once. Also: drop a leftover variable in phase 1 finalize, check the walk context once, stop repeating the folder field that is already in the log context, stop shadowing finalize's err in phase 3, and format the WithTxRetry scope the same way as WithTx.
* test(scanner): make the scanner suite's temp DB cleanup best effort
Which DB file the process-wide DB handle opens depends on which spec touches it first. When the Scanner container wins the random order, its temp DB stays open until db.Close after RunSpecs, and on Windows removing the temp dir fails with 'being used by another process'. Ginkgo pins that on the container's last spec, which is now one of the busy-database specs. The sibling suites skip Windows for the same reason; this one now removes its temp dir on a best-effort basis instead, so it keeps running there.
* fix(server): stop swallowing errors and correct two response bugs
Four independent bugs found while reviewing the HTTP layer:
initial_setup.go: createInitialAdminUser assigned the users.Put error to a
shadowed err, so the outer err (always nil by then, since a CountAll failure
panics) was returned instead. A failure to create the admin user was reported
as success, and initialSetup went on to commit the "setup complete" property
in the same transaction — so no admin user existed and initial setup was
skipped on every later boot.
auth.go: createAdminUser logged the Put error but returned nil, so createAdmin
fell through to doLogin and answered 401 "Invalid username or password"
instead of surfacing the real failure. It also logged the whole model.User,
which puts the new admin's password in the log in clear text; every other call
site logs user.UserName.
native_api.go: writeDeleteManyResponse did not return after http.Error when
marshaling failed, then wrote a nil body over the 500. It also built the
single-id body by hand with html.EscapeString, which does not escape
backslashes, so an id ending in one produced `{"id":"a\"}` — invalid JSON.
Both shapes now go through json.Marshal. A failed Write is now logged rather
than answered with http.Error, which could not work once the body had started.
handle_shares.go: handleM3U set Content-Type after WriteHeader, so it was
never sent and shared playlists were served with a sniffed type.
Signed-off-by: zapisanchez <zapisanchez@gmail.com>
* fix(server): address review feedback
- writeDeleteManyResponse uses rest.RespondWithJSON, so the response now
has Content-Type: application/json. This also removes a marshal error
branch that could never run.
- createInitialAdminUser returns the CountAll error instead of panicking,
and wraps its errors. initialSetup now stops the server with log.Fatal
when setup fails. Before, the error was dropped and the server started
with a half-done setup.
- Trim comments that described PR history.
---------
Signed-off-by: zapisanchez <zapisanchez@gmail.com>
Co-authored-by: Deluan <deluan@navidrome.org>
POST /Playlists dropped the client's IsPublic flag, so every playlist was
created private. JellyBox Player's create-playlist form defaults its "public"
checkbox to true, so JellyBox users could never create a public playlist.
Upstream's PlaylistsController passes IsPublic into PlaylistCreationRequest.
core/playlists.Create has no visibility parameter and widening it would ripple
into the Subsonic and native APIs, so createPlaylist follows the same pattern
updatePlaylist already uses: after Create succeeds, a non-nil IsPublic is
applied with a follow-up Update. The field is a pointer so an absent one keeps
today's default instead of forcing private.
If that second write fails the handler surfaces the error through playlistError
rather than returning the id: answering 200 for a playlist that is not as
visible as the client asked is the same silent drop this fixes.
* sec(server): sanitize user-controlled filenames in Content-Disposition
Playlist export, Subsonic download and public share download built the
Content-Disposition header by interpolating a user-controlled name into a
quoted-string with fmt.Sprintf. A name containing a double quote closes the
string early and the rest is parsed as additional parameters, so a playlist
named `party"; filename="evil.html` yielded
attachment; filename="party"; filename="evil.html.m3u"
letting whoever chose the name decide what the browser saves the download as.
The names come from playlists, album/artist names and media file tags.
Go's net/http already rewrites CR and LF in header values to spaces, so
response splitting was not reachable; parameter injection was.
Add str.ContentDispositionAttachment, which emits a sanitized ASCII-only
quoted `filename` plus an RFC 5987 `filename*` carrying the original UTF-8
name, and use it at all four call sites. The `filename*` parameter also fixes
non-ASCII names, which previously went out raw or were mangled by sanitizing.
Signed-off-by: zapisanchez <zapisanchez@gmail.com>
* fix(server): keep download names intact and sanitize filename*
Rework ContentDispositionAttachment after review. Names with no ASCII
letters now fall back to download.<ext> instead of a bare extension
(東京.mp3 gave filename="mp3"). filename* is built from the same
sanitized name as the ASCII fallback, so path separators, reserved
characters, control and bidi characters, and invalid UTF-8 no longer
reach it. The ASCII fallback transliterates accents and typographic
punctuation (Legião -> Legiao, She’s -> She's) through the existing
sanitize.Accents and str.Clear helpers, keeps leading dots, and only
trims trailing ones. Names are capped at 255 bytes, keeping the
extension.
Pure ASCII names now get only the quoted filename parameter, so the
header for them matches the previous output byte for byte. filename*
is encoded with mime.FormatMediaType instead of a hand-written RFC 5987
encoder. Adds tests for the M3U export and Subsonic download headers.
* fix(server): handle dot-only names and long fake extensions
A name made only of dots trimmed down to an empty filename. It now
falls back to download, like an empty stem does.
path.Ext treats anything after the last dot as the extension, so a long
suffix with no real extension was kept whole and replaced the stem with
download, going past the 255-byte cap. Suffixes longer than 16 bytes
are now treated as part of the stem and truncated with it.
Neither case is reachable from the current call sites, which always
append a short extension.
---------
Signed-off-by: zapisanchez <zapisanchez@gmail.com>
Co-authored-by: Deluan <deluan@navidrome.org>
* fix(scanner): keep tag numbers within the int32 range
A track number of 4294967295 (-1 stored as an unsigned 32-bit tag) was saved
as-is by 64-bit builds. 32-bit builds (armv5/6/7, 386) cannot read that value
back into an int, so every scan failed with "converting driver.Value type
int64 to a int: value out of range" when loading the folder's media files.
Track and disc numbers (and their totals) are now parsed as int32 and fall
back to 0 when out of range, matching how unparseable values are handled.
BPM values outside the int32 range are dropped. A migration resets existing
out-of-range track_number, disc_number and bpm values, and removes
out-of-range keys from album.discs, so databases written by 64-bit builds are
readable again by 32-bit ones. Persistent IDs are unaffected because they use
the raw tag text.
Fixes#6200
* fix(scanner): accept the int32 minimum as a BPM value
The BPM range check compared the absolute value against MaxInt32, which
rejected -2147483648 even though it fits in an int32. Compare against
MinInt32 and MaxInt32 separately, matching atoi32 and the migration.
* fix(scanner): treat negative track, disc and BPM values as missing
Track numbers, disc numbers and BPM can never be negative, so negative tag
values now map to 0 (track/disc, including totals) or nil (BPM), the same as
unparseable ones. The migration resets existing negative values as well as the
ones above the int32 range, and keeps only album disc keys from 0 to MaxInt32.
The cleanup job listed artifacts repo-wide, so it deleted digest files
uploaded by any concurrent pipeline run. When the v0.64.1 tag run
overlapped with a master run, the master run's cleanup removed three of
the tag run's digests before the manifest job downloaded them, and
0.64.1/latest shipped with only linux/arm64, arm/v7 and riscv64.
List artifacts for the current run instead, so a run can only delete its
own digests.
GetArtistImages scrapes the og:image tag off the Last.fm artist page, because
the API only ever returns the placeholder image. Last.fm now answers non-browser
clients with a Fastly bot challenge, served as a 200 with valid HTML, so the
query found no og:image and the agent returned an empty list with no error. The
artwork worker read that as a definitive "this artist has no image" and settled
the state as absent, silently and with nothing in the log.
A real artist page always carries an og:image, so its absence now returns an
error instead. The worker keeps the previous state, other agents still get their
turn, and its per-agent circuit breaker bounds the retries. The error is
deliberately not a RetryLaterError: that would park the whole Last.fm agent,
including the API-backed biography, similar-artists and top-songs calls, which
the page block does not affect.
The new fixture is the real 3038-byte challenge page.
Fixes#6192
Navidrome calls out to ffprobe, which in turn may use libblas on some
setups (e.g., Debian 13). The previous syscall filter excluded the
"mbind" syscall (via @resources).
Through direct experimentation, mbind is required by libblas, and so it
is added to the allowed syscall list.
Signed-off-by: Antonio Enrico Russo <aerusso@aerusso.net>
The scrobble endpoint accepts multiple ids, but a nowPlaying notification
(submission=false) describes a single track, so only the first id is used.
The extra ids were dropped silently, which made client bugs invisible. Log a
warning instead, keeping the existing behavior for clients that rely on it.
* fix(subsonic): limit failed authentication attempts per client IP and username
The Subsonic API checked credentials on every request with no limit on failures, so any
account could be brute-forced over /rest/*. Failed u+p, t+s and jwt attempts are now capped
per (client IP, lower-cased username) using AuthRequestLimit and AuthWindowLength, the same
settings that guard the UI login. Every request carries credentials, so only failures count:
a slot is taken before the check and given back on success or on a server error, which also
stops concurrent guesses from overshooting the limit.
Blocked attempts get the same response as a wrong password (HTTP 200, error code 40, no
Retry-After), so an attacker cannot tell a block from a wrong guess. Reverse proxy and
internal authentication are not limited. The client IP helper behind ClientIPRateLimiter is
now exported as server.ClientIP, so spoofed forwarding headers cannot open a fresh bucket.
* refactor(subsonic): simplify failed authentication limiter
Release the limiter slot from a single place in authenticate(), after the user lookup and
credential check, instead of separately in the canceled branch. Store attempt counters by
value instead of by pointer, and drop limiter unit tests that only repeated the middleware
specs.
* fix(subsonic): wait for an in-flight auth check instead of rejecting
Slots were reserved before the credential check and only released afterwards, so once
AuthRequestLimit checks for the same client IP and username overlapped, the next request was
answered with error code 40 even when its credentials were valid. Clients that fan out parallel
requests hit this constantly: a burst of six valid logins lost one, a burst of fifty lost forty
five, and the web UI authenticates its own /rest calls the same way.
A key now carries a slot channel of AuthRequestLimit capacity, and a request waits on it rather
than failing when other checks for that key are in flight. Failures are recorded after the check,
and a request is only rejected when the key already reached the limit within the window. A waiting
request gives up if its context is canceled. Concurrent guesses still cannot run unchecked: at most
AuthRequestLimit checks run at once and the rest are turned away as soon as the failures land.
* docs(subsonic): state the real guess ceiling of the auth limiter
The comment claimed a burst cannot overshoot, which reads as a hard cap of AuthRequestLimit. Allowing concurrent checks means a window admits up to 2*limit-1 guesses, so say that instead.
POST /api/player passed the ownership check using the userId from the request body, then
saved with the body id. When that id belonged to another user's player, the save became an
update with no owner restriction, overwriting the row and moving it to the caller. Save now
always creates a new player and ignores any id in the body; edits keep going through the
owner-scoped Update.
Player registration also reused a player by the id sent in the Subsonic player cookie or the
Jellyfin DeviceId without checking its owner, letting a user attach to another user's player
and overwrite its name, user agent and IP. Register now only reuses a player owned by the
requesting user, falling back to the user's own players otherwise.
* fix(artwork): block private and loopback addresses in remote image fetches
fromURL fetched any URL with a plain HTTP client, and two untrusted inputs reach it. A playlist
can set #EXTALBUMARTURL to an http(s) URL, which the artwork worker later fetches when
EnableM3UExternalAlbumArt is on, so any user who can import a playlist controls the target.
Metadata agents, including WASM plugins without the http permission, return image URLs that the
core fetches too. Either path could make the server request loopback, LAN or link-local
addresses and store the response as artwork that is served back.
Add httpclient.NewExternal, which dials through a net.Dialer Control hook that rejects private,
loopback, link-local and unspecified addresses. The check runs at dial time on the resolved IP,
so DNS names, redirects and DNS rebinding are covered. fromURL now uses one shared client built
with it and treats a refused address as a definitive miss, so the item settles absent instead of
retrying and tripping the agent's circuit breaker. httpclient.New is unchanged for the other callers.
The IP classification moves from plugins to the new utils/netguard package, shared by the plugin
host client and the new constructor. The artwork test suite swaps in a client that allows
loopback so existing specs can keep using httptest servers; the fromURL specs use the production
client to assert the refusal.
* fix(httpclient): keep dialing a configured proxy in the guarded client
The guard runs on the resolved address, and with HTTP_PROXY set that address is the proxy, not
the image host. A proxy on a private address would have had every remote artwork fetch refused,
and a refusal settles the item as absent, so covers would silently disappear for those setups.
Dial the configured proxy endpoint directly and keep the guard for every other dial. A proxy
relays the request itself, so it is the operator's egress policy, the same one every other
httpclient.New caller already goes through.
* fix(httpclient): exempt only the hop that actually goes through the proxy
The exemption matched any dial to a configured proxy's address, but net/http never proxies
loopback targets, so a URL aimed at a loopback proxy was dialed directly and skipped the guard.
That let an image URL reach that one address.
Tag each request with the proxy it resolves to and exempt a dial only when it is that hop.
Redirects re-enter the RoundTripper, so every hop is tagged on its own.
* fix(persistence): don't format a nil-model row in wrapCursor
* test(plugins): assert task queue delay against the first dispatch, not consecutive gaps
* test(artwork): let the e2e worker wait outlast one retry
* chore: trim comments
* fix(persistence): guard dbFolder and dbMediaFile String() against a nil model
* fix(playlists): limit local cover paths to images in owner's libraries
A local #EXTALBUMARTURL path (absolute or file://) was only checked against the union of all
libraries. The artwork resolver then opened it with no further check and served the bytes as the
playlist cover, undecoded. Any user who can upload an M3U could read any file under any library
root, including libraries they were not granted, through getCoverArt (GHSA-vwq6-xrw5-phpg).
resolveImageURL now requires an image extension, and for uploaded playlists (no folder) the
library holding the cover must pass the owner's HasLibraryAccess. Scanner and CLI imports keep
the all-libraries check, since those files are admin-controlled.
resolveLocalFile, used by every file-backed artwork source, now ignores paths without an image
extension, which covers playlists stored before this fix that were not resolved yet. openOriginal
refuses a stored file-backed row whose path is not an image, so the existing dangling path
re-resolves it and the playlist falls back to the generated grid. No migration is needed.
* fix(artwork): skip non-image files matched by folder cover patterns
Album and disc folder sources opened any file in the folder's image list that matched a
cover pattern, without checking its extension. openOriginal now refuses to serve file-backed
rows whose path is not an image, so a stored row like that would be refused, re-resolved to
the same file, and refused again on every view. The list comes from the scanner, which only
records image files, but a database scanned where the OS mime table knows more image types
than the serving process could still reach this.
Both fromExternalFile variants now skip matches that are not image files, so the album falls
back to its next source instead. Also correct the parser comment: a playlist without a folder
can come from an API upload or from a CLI import of a file outside all libraries.
* fix(artwork): check stored source type before using the resize cache
The image-extension check for file-backed rows ran inside openOriginal, which the resize
cache skips on a hit. Before the fix, a resized request for a playlist pointing at a non-image
file cached the raw bytes, because a failed resize falls back to the original data. After the
upgrade the same request still hit that entry and returned the file.
serveHash now refuses a file-backed row whose path is not an image before calling serveSource,
so both full-size and resized requests go through dangling and re-resolve the item. The stale
cache entry is keyed by the old hash and is no longer reachable once the row changes.
* test: register mime_types.yaml in test binaries
Artwork resolution now skips candidates that are not image files, and model.IsImageFile answers
from the process mime table. The server registers the extra image types from
resources/mime_types.yaml through a conf hook, but a test binary only does that if it links
conf/mime, so the artwork e2e suite fell back to the host table: .jxl resolves on macOS and
Linux and does not on Windows, where the #5950 cover spec then found no source.
tests.Init now imports conf/mime for its side effect, so every suite that loads the test config
sees the same image types as the server.
* fix(artwork): drop the image-file guard from the disc art reader
The guard was added to both fromExternalFile variants, but disc artwork keeps no state row and
is never queued, so it cannot hit the refuse-and-re-resolve loop the guard exists to prevent.
The only case where it can fire is a real image whose extension this process's mime table does
not know, and there it drops a disc cover that used to work. The album variant keeps the guard,
since those resolutions are stored and re-served.
On a multi-library instance, a few reads and writes built their own queries
without the per-user library filter that every other media read applies. A
user granted only some libraries could see, and store, tracks from libraries
they had no access to.
- getBookmarks now filters the query. It has to be the query and not the
result: the loop below it pre-sizes the response from the bookmark count,
so a row dropped afterwards would emit an empty bookmark entry.
- createBookmark rejects an id the caller cannot read, returning error 70 to
match getSong. Stored rows are left alone rather than purged, so a
temporary revoke does not lose saved playback positions.
- playlistTrackRepository Read, Count and GetAlbumIDs get the filter their
siblings CountAll and GetMediaFileIDs already had. Read is the one that
mattered most: its id is the integer playlist position, so it needed no
track id at all.
- Playlist track writes are filtered in playlistRepository.addTracks, the
only writer of playlist_tracks rows apart from smart playlists, so Add,
Insert, AddAlbums/AddArtists/AddDiscs and a full replace through Put all
go through it. Insert reserves a slot per requested id, so when the filter
drops one it renumbers to close the hole.
- playTracker.GetNowPlaying honours its context instead of discarding it.
The cache is process-global, so the filter belongs in the tracker rather
than in the Subsonic handler, and any future caller inherits it.
Admins and single-library installs are unaffected: applyLibraryFilter and
HasLibraryAccess both short-circuit for them. Scanner playlist sync runs as
admin, and M3U and CLI imports already resolve tracks through FindByPaths as
the same user, so neither changes.
* Update missing German translations
* Fix typo in idHelp message in German translation
* Update German translations to remove formal "Sie" forms
---------
Co-authored-by: Deluan Quintão <deluan@navidrome.org>
* feat: Add support for referencing playlists using paths
Signed-off-by: David <dvedvick@gmail.com>
* feat: Support relative playlist paths in smartlists
Signed-off-by: David <dvedvick@gmail.com>
* fix(smartplaylists): protect against nil panic
Signed-off-by: David <dvedvick@gmail.com>
* fix(smartplaylists): refreshing child playlists
Signed-off-by: David <dvedvick@gmail.com>
* chore(smartplaylists): log field parsing error
Signed-off-by: David <dvedvick@gmail.com>
* fix(smartplaylists): handle empty playlist paths
Signed-off-by: David <dvedvick@gmail.com>
* refactor(smartplaylists): make NormalizeChildPaths non-mutating
Signed-off-by: David <dvedvick@gmail.com>
* fix(smartplaylists): stop warning on every inPlaylist rule without the looked-up field
Rules that reference a playlist by id have no path field, and the reverse, so
the warning fired on every refresh. The log call also had a bad argument count.
* fix(smartplaylists): ignore empty inPlaylist id and path references
An empty path matched every playlist without a file path, including the
referencing playlist itself, so the refresh recursed until the stack overflowed.
An empty id also shadowed a valid path in the same rule.
* fix(smartplaylists): match inPlaylist paths in both NFC and NFD forms
A playlist path is stored in the Unicode form the filesystem reports, which can
differ from the form typed in the .nsp file. The exact comparison then found no
playlist for names with accents.
* fix(smartplaylists): keep all criteria fields when normalizing child paths
The field-by-field copy dropped RefreshDelay.
* fix(smartplaylists): clean absolute inPlaylist path references
Only relative references were cleaned, so an absolute reference such as
/music/./child.nsp never matched the stored /music/child.nsp.
* fix(smartplaylists): stop infinite recursion on playlists that reference each other
Two smart playlists referencing each other, by id or by path, recursed until the
stack overflowed and the server died. The refresh now tracks visited playlists.
* fix(smartplaylists): resolve inPlaylist path references with OS-native separators
Playlist.Path is OS-native, but references in a .nsp file use forward slashes.
On Windows they never matched, and a leading slash was not seen as absolute.
The specs now build OS-native paths, so they also run on Windows.
* fix(smartplaylists): warn when a relative inPlaylist path cannot be resolved
A playlist created in the UI has no file path, so a relative reference silently
matched nothing.
* refactor(smartplaylists): simplify child playlist reference handling
Share one extractor for child ids and paths, return only the normalized rules
instead of a playlist copy, and resolve each path reference in a single switch.
* test(smartplaylists): store the Unicode child path in OS-native form
Playlist.Path is OS-native, so on Windows the forward-slash fixture never matched
the normalized reference.
---------
Signed-off-by: David <dvedvick@gmail.com>
Co-authored-by: Deluan Quintão <deluan@navidrome.org>
* fix(ui): only redirect to ExtAuth logout URL for proxy-authenticated sessions
react-admin calls authProvider.logout() when the boot-time checkAuth fails
and after a 401, not only when the user clicks Logout. With
ExtAuth.LogoutURL set, every unauthenticated page load (e.g. direct LAN
access that bypasses the auth proxy) was sent to the IdP sign-out page and
the login form was never shown.
Redirect only when the page was authenticated by the reverse proxy
(config.auth is present). Other sessions fall back to the login form.
Fixes#6175
Signed-off-by: Deluan <deluan@navidrome.org>
* fix(server): only warn about untrusted ExtAuth sources when the header is sent
UsernameFromExtAuthHeader checked the source IP before looking for the user
header, so every request from an IP outside ExtAuth.TrustedSources logged a
warning, even when it carried no header at all. With direct LAN access
alongside a forward-auth proxy, a single polling client produced a constant
stream of warnings (twice per Subsonic request, since the middleware chain
resolves the username in both checkRequiredParameters and authenticate).
Look for the header first and warn only when an untrusted source actually
sends it, which is the case worth seeing: a misconfigured proxy or a spoof
attempt.
Signed-off-by: Deluan <deluan@navidrome.org>
---------
Signed-off-by: Deluan <deluan@navidrome.org>
* feat(jellyfin): add Quick Connect sign-in
Jellyfin clients can now sign in without a password: the client shows a
6-digit code, a signed-in user approves it, and the client redeems a secret
for its access token.
- core/quickconnect: in-memory store shared by both routers through wire.
Codes expire after 10 minutes; a secret redeems only once (Jellyfin
allows repeats for 10 minutes); at most 1000 pending requests.
- Jellyfin API: Initiate, Connect, Authorize and AuthenticateWithQuickConnect.
Admins may approve for another user via UserId, like Swiftfin's admin page.
Initiate and redeem share the login rate limiter; Connect does not, since
Finamp and Streamyfin poll it every second.
- Web UI: a Quick Connect item in the user menu looks up the code and shows
the app and device before approving, so a user can't be tricked into
approving an unknown device blindly.
- Jellyfin.QuickConnect option, on by default like Jellyfin. It only matters
when the Jellyfin API is enabled.
* refactor(jellyfin): tidy Quick Connect naming and route guards
Group the Quick Connect routes under one requireQuickConnect guard, make the
request's device a named field so req.Device.ID can't be mistaken for a
request id, and rename the web API response type to quickConnectDevice.
* refactor(jellyfin): remove duplicated Jellyfin date formatting function
* test(jellyfin): set play count and starred in the song fixture literal
* refactor(jellyfin): inline the Quick Connect redeem body and use the shared date helper
* fix(jellyfin): bound the client fields Quick Connect keeps in memory
Initiate is unauthenticated and keeps the Client, Device, DeviceId and Version
header fields for up to ten minutes. With no header size limit, each pending
request could hold about 1 MB, and even a short field kept the whole header
alive because the parsed values are substrings of it. Reject fields over 512
bytes and copy the stored values.
Also answer 500 instead of 401 when the redeem user lookup fails for a reason
other than the user being gone.
* fix(jellyfin): rate-limit Quick Connect code approval
Any signed-in user could try codes without limit on the Jellyfin Authorize
endpoint and the web UI lookup/authorize endpoints, and so could approve
another person's pending device for their own account. Apply the same per-IP
limiter as the login (AuthRequestLimit/AuthWindowLength) to both surfaces.
Dates were rendered with the browser locale, ignoring the language chosen in
Personal settings, so a user browsing in German still saw US-style dates. All
date rendering now resolves its locale through a useDateLocale hook that returns
the selected language, augmented with the region from navigator.languages when
the language carries none (Intl reads a bare "en" as en-US). Applied to
DateField, the rated/loved tooltips, the mobile user list and album release
dates; the three inline timestamp tooltips now share a formatDateTime helper.
Closes#229
* feat(jellyfin): compute the address advertised by auto-discovery
* fix(jellyfin): use TLSEnabled and path.Join for the auto-discovery address
* feat(jellyfin): answer LAN auto-discovery broadcasts
* test(jellyfin): e2e check that auto-discovery matches the public server identity
* feat(jellyfin): add opt-in AutoDiscovery option and start the listener
* fix(jellyfin): close the auto-discovery socket on read errors and quiet reply failures
* refactor(jellyfin): build the auto-discovery address with publicurl and log bind failures in place
discoveryAddress now hands only the discovery-specific part (the requester-facing host) to publicurl.AbsoluteURL, so the BaseURL, scheme and BasePath rules live in one place. ServeDiscovery logs its own bind failure and returns nothing, so the caller cannot route the error into the server errgroup. Tests use a DescribeTable and no longer wait on a fixed timeout to prove a packet was ignored.
* refactor(jellyfin): run auto-discovery as its own service in the main errgroup
Discovery is now a small type that needs only a DataStore, started by startJellyfinDiscovery like the other background services, so the errgroup waits for it on shutdown and startServer is back to a one-line mount. The server id resolution moved to resolveServerID with a package-level lock: the Router and Discovery are separate objects, and a per-Router lock would let them persist two different ids on first boot.
* refactor(jellyfin): build Discovery through wire and drop the serverName forwarder
startJellyfinDiscovery now follows its siblings: negative guard with a DISABLED debug log, and the service comes from a CreateJellyfinDiscovery wire injector instead of an inline constructor. The Router.serverName method only forwarded to the package function, so its four call sites call the function directly.
* fix(jellyfin): skip auto-discovery for unix socket servers without a BaseURL host
With Address set to a unix socket nothing listens on Port, so the route-facing IP plus Port pointed clients at a dead URL. Discovery now logs a warning and does not start in that mode unless BaseURL names the proxy host, which AbsoluteURL already advertises as-is.
* docs(jellyfin): note which address auto-discovery advertises on restricted binds
Also stop using a hostname Address in the fallback spec: a hostname like localhost binds a single interface, so it is not an example of the route-facing fallback being right. An empty Address is.
* feat(jellyfin): advertise Jellyfin server version 12.1.0
Streamyfin, jellyfin-android and jellyfin-androidtv refuse servers older than
10.10, Swiftfin warns below 12.0, and @jellyfin/sdk flags anything below its
minimum as unsupported, so 10.9.11 locked those clients out. 12.1.0 is the
current Jellyfin release (after 10.11 Jellyfin renumbered to 12.0). No client
checked has an upper bound or assumes the major is 10, and the value keeps
three parts because the Kotlin SDK and Swiftfin reject two-part versions.
* feat(jellyfin): acknowledge POST /Sessions/Playing/Ping
Jellyfin clients ping this endpoint to keep a transcode job alive while
paused. Navidrome ties transcodes to the stream request, so there is nothing
to keep alive; answer 204 like Jellyfin instead of a 404. It shares one no-op
handler with Sessions/Capabilities, renamed to acknowledge.
* feat(jellyfin): reorder playlist entries via Items/{entryId}/Move
Adds POST /Playlists/{id}/Items/{entryId}/Move/{newIndex}, which clients use
to reorder playlists. It maps the entry's PlaylistItemId (its position) and
Jellyfin's zero-based newIndex onto the existing core ReorderTrack, which
enforces ownership. As in Jellyfin, an index past the end appends and an
unknown entry is a no-op; out-of-range positions never reach Reorder, which
would otherwise shift unrelated rows.
* feat(jellyfin): honor position when adding items to a playlist
POST /Playlists/{id}/Items takes an optional zero-based position (added to
the Jellyfin spec in 12.0). Match Jellyfin: zero or negative prepends, past
the end appends, otherwise the new items are inserted at that index in the
order they were added.
Adds PlaylistTrackRepository.Insert and core playlists.InsertTracks, which
shift the following entries and insert in one transaction, instead of
appending and moving each new track with its own ReorderTrack call.
* feat(jellyfin): add type-specific InstantMix routes
Jellyfin exposes InstantMix under Songs/, Albums/, Artists/ and Playlists/
as well as Items/, plus the legacy Artists/InstantMix and
MusicGenres/InstantMix forms that take the seed as ?id=. Clients generated
from the Jellyfin SDKs call the type-specific routes, which 404ed. All of them
now share the existing Items/{id}/InstantMix handler; the external provider
already builds mixes from song, album, artist, playlist and genre seeds.
* feat(jellyfin): honor Width, Height and Fill* image size params
The image endpoint only read MaxWidth/MaxHeight, so clients that size covers
with fillWidth/fillHeight (Manet, Finamp) or width/height got the full-size
original on a cold artwork cache: a 578 KB PNG instead of a 13 KB resize.
Jellyfin applies Width/Height, caps them with MaxWidth/MaxHeight, then
shrinks to the smallest size that still covers the Fill box. Navidrome
resizes on one dimension, so the tightest bound wins and a fill box counts
as its larger side.
* feat(jellyfin): answer HEAD on audio, file and image routes
Fintunes sends HEAD to /Audio/{id}/universal to read the content type and
detect direct play (a Content-Length means direct play), and to the audio and
image URLs before a download, aborting the download when it fails. Those routes
were GET-only, so HEAD got a 404. HEAD now reuses the GET handlers. Direct
play goes through Stream.Serve, which already answers HEAD; a transcode
answers with the target content type and no length without starting ffmpeg,
so a probe never costs a transcode.
* fix(jellyfin): clamp playlist move and insert positions before adding one
movePlaylistItem computed min(newIndex+1, SongCount): newIndex=MaxInt wrapped
to a negative position, and Reorder then left the playlist with a gap
(positions 2, 3, 999998), making the moved entry unmovable. Clamp against the
playlist length first. addToPlaylist's min(position, MaxInt32)+1 wrapped on
32-bit builds and req.Int truncated large values there, so a far-past-the-end
position prepended; parse as int64 and clamp before converting.
* fix(playlists): validate reorder positions inside the write transaction
Reorder never checked its positions, so a source outside the playlist or a
destination past its end shifted rows around a missing entry and left a gap
(e.g. ids 1 and 3), which made the moved entry unmovable afterwards. The
Jellyfin Move handler guarded this with a SongCount read before ReorderTrack,
but a concurrent removal between the two reopened it, and the native API
reorder endpoint passed client positions through unchecked.
Reorder now reads the last position in the same transaction, returns
ErrNotFound for a source outside the playlist and clamps the destination.
ReorderTrack uses an immediate transaction so that read and the updates are
atomic against other writers. The Jellyfin handler drops its pre-check and
maps ErrNotFound to Jellyfin's no-op 204; the native API now answers 404 for
an unknown track instead of corrupting the order.
* fix(jellyfin): send SessionInfo on login so JellyBox gets past sign-in
JellyBox parses AuthenticateByName's SessionInfo as a required object and
fails silently when it is missing, leaving the user on the login screen.
Real Jellyfin always sends it (SessionManager.AuthenticateNewSessionInternal,
10.10.7 and master), so the login response now carries a full SessionInfo
built from the user and the MediaBrowser auth header. It includes every field
JellyBox (Id, PlayState) and Finamp (UserId, LastActivityDate, the activity
and control bools, PlayState's CanSeek/IsPaused/IsMuted) require once the
object is present. The session Id is derived from client and device id, so
repeated logins from one install share it.
* fix(jellyfin): ignore IncludeItemTypes names that aren't Jellyfin kinds
JellyBox opens an album with ParentId=<album>&IncludeItemTypes=music. Music
is not a BaseItemKind, and Jellyfin's comma-delimited binder drops values it
cannot parse, so real Jellyfin treats the request as having no type filter and
lists the album's tracks. Navidrome returned an empty list, so every album
opened empty. Entries that aren't BaseItemKind names are now dropped before
type resolution, so an all-unknown list behaves like an absent one. Real kinds
Navidrome doesn't serve, such as Boxset, still return nothing.
* fix(jellyfin): treat universal Container as the direct-play list
On /Audio/{id}/universal, Container lists the "container|codec" entries the
client can direct play, and TranscodingContainer/AudioCodec name the target
when it can't (UniversalAudioController builds DirectPlayProfiles from it).
Navidrome passed the whole list to the decider as one target format, which
matched nothing and fell back to DefaultDownsamplingFormat, so JellyBox got
every MP3 transcoded to Opus. /universal now has its own handler: a source
matching an entry keeps its format (still downsampled under a bitrate cap),
anything else is transcoded to TranscodingContainer, then AudioCodec. The
/stream routes keep treating Container as the target format.
* refactor(jellyfin): let the stream decider resolve universal requests
streamUniversal matched the Container list itself with plain string equality
and then asked the legacy resolver for the source format. That skipped the
decider's container and codec aliases (mp4 vs m4a, ogg vs opus), and a
direct-playable source over the bitrate cap was transcoded to its own format
instead of the client's TranscodingContainer.
The shared part of ResolveRequest (server-side player override, player
MaxBitRate cap, decision to Request mapping) moves to a resolve helper, and a
new ResolveClientRequest exposes it for callers that build their own
ClientInfo. streamUniversal now turns Container into DirectPlayProfiles and
TranscodingContainer/AudioCodec into a transcoding profile, so the decision
uses the same rules as the Subsonic getTranscodeDecision path. streamFile and
the /stream routes share a serveStream helper, NewSessionInfo reads the clock
itself, and duplicate comments and tests are trimmed.
* fix(release): repair root-owned artwork and plugins folders on upgrade
Navidrome 0.57.0, 0.60.x and 0.61.x created the plugins and artwork folders as soon as the configuration loaded. The deb/rpm postinstall script runs navidrome as root, so fresh installs and upgrades on those versions left these folders owned by root. The service runs as the navidrome user and cannot write to them. Since 0.64.0 new artwork is stored under artwork/hashed, so affected installs fail to persist artwork and cannot read the plugins folder.
The postinstall script now changes the owner of these two folders to navidrome, only when they exist and are owned by root. The change is not recursive: root created the folders empty and the service could never write inside them, so fixing the folder itself is enough and stays instant regardless of how much artwork exists. Folders an admin assigned to another user are left untouched.
Fixes#6140
* fix(release): handle root-owned cache folder without install noise
The postinstall script ran an unconditional chown on /var/lib/navidrome/cache during fresh installs. Since folders are created lazily, the cache folder does not exist at that point, so every fresh deb/rpm install printed "chown: cannot access '/var/lib/navidrome/cache': No such file or directory".
The cache folder is now part of the same root-owned folder check used for artwork and plugins: it is fixed when it exists and is owned by root, and skipped silently otherwise. This also covers installs from 0.54.1 and 0.54.2, which created the cache folder as root before the chown was added.
* fix(release): never follow symlinks when repairing folder ownership
The ownership check used find's default -P mode, so -user root tested a symlink itself, while chown dereferenced it. A root-owned symlink pointing to a folder owned by another account made the postinstall script reassign that folder to navidrome, bypassing the root-owner guard.
The check now only matches real directories (-type d without following links) and uses chown -h. Following the link with find -H was rejected: /var/lib/navidrome is owned by navidrome, so the service account could plant a symlink to any root-owned directory and have the next upgrade hand it over. As a trade-off, a symlink to a root-owned folder is no longer repaired; the folders affected by the original bug were always real directories.
Add an e2e spec for an artist whose only album folder has no images of its
own, while the artist folder holds folder.jpg (plus unrelated images) and
ArtistArtPriority starts with folder.*. Before #5856, the album's parent was
promoted into the album paths, so the artist folder resolved to the library
root and the artist got no image. The spec fails if that promotion comes
back, and passes on current code.
Refs #5823
Last.fm decodes the artist and track params of artist.getInfo, artist.getSimilar, artist.getTopTracks and track.getSimilar twice, so a "+" in a name becomes a space. Names like "Florence + The Machine" resolved to a misspelled duplicate page whose bio is Last.fm's "incorrect tag" notice, and names like "+44" were not found at all. Encode "+" as %2B before the normal query encoding for those calls. album.getInfo decodes only once, so it keeps the plain encoding.
* fix(jellyfin): match Jellyfin's item payloads so strict clients can sync
Manet (iOS/macOS) aborted its whole library sync on the first item that was
missing a key its decoder requires, leaving the library empty (#6147). Every
gap was a field real Jellyfin always sends:
- dates now use .NET's round-trip layout with 7 fractional digits, which Manet
requires and plain RFC3339 failed
- playlists carry SortName/DateCreated, and every item carries MediaType,
ImageTags, ChannelId and, when Fields asks, Genres/GenreItems/Tags
- albums carry Artists and LocationType; songs always carry HasLyrics
- the library view is a full CollectionFolder (ChildCount, DateCreated,
SortName, Path, LocationType, UserData), read from the library rows rather
than the user projection, which has no counts
- IncludeItemTypes matches case-insensitively and returns nothing for Jellyfin
kinds Navidrome has none of, instead of falling back to every album
Verified against a real Jellyfin 10.10.7 server and a live Manet client.
* refactor(jellyfin): fold the repeated empty-list defaults into one helper
The three mappers each initialised Genres/GenreItems/Tags the same way, and the
e2e suite grew three near-identical specs walking every item type. Both now go
through a single helper and one table.
* refactor(jellyfin): fill the always-present item fields at the serialization edge
The defaults real Jellyfin puts on every item were spread across three mappers,
so item types nobody had tested yet (playlists, genres, the library view) still
shipped payloads a strict client rejects. stampItem now takes the request's
Fields and fills them for every item, which is provably the only path to JSON.
Also drops the hand-copied BaseItemKind list: only an absent IncludeItemTypes
defaults to albums now, so any type Navidrome does not serve returns nothing,
as it would from Jellyfin. The synthetic playlists folder matches
case-insensitively like the rest, /Items/{libraryId} reads the full library row
instead of the count-less user projection, and PremiereDate and LastPlayedDate
go through jellyfinDate rather than spelling the layout out again.
* fix: return 404 instead of 500 for missing native API resources
The deluan/rest controller only maps rest.ErrNotFound to 404, comparing with ==.
Most repositories return model.ErrNotFound, which had the same message but was a
different value, so requesting a missing playlist, album, artist, song, radio,
player, transcoding or library returned 500. This also applied to other users'
private playlists.
Make model.ErrNotFound the same value as rest.ErrNotFound. This fixes every REST
route at once, with no per-route wrapping. errors.Is checks against either
error keep working, and nothing wraps model.ErrNotFound before it reaches the
controller.
Fixes#6130
* fix(radio): return not found when deleting a missing radio station
radioRepository.Delete used the shared delete helper, which never reports a
missing row because SQL DELETE on zero rows is not an error. Deleting an unknown
id silently succeeded: DELETE /api/radio/{id} returned 200, and the Subsonic
deleteInternetRadioStation endpoint returned ok.
Delete now checks the affected row count and returns model.ErrNotFound when
nothing was deleted. The native API returns 404, and deleteInternetRadioStation
returns error 70 (data not found). This matches Subsonic 6.1.6, gonic (both
verified live) and Ampache (verified in source). Airsonic-Advanced does not
implement this endpoint.
The shared delete helper is unchanged, as several callers rely on deletes of
absent rows succeeding.
* fix(ui): return 404 for missing files that are not missing or do not exist
missingRepository.Read filtered media files by bare "id" and "missing" columns.
The media file query joins the library table, so SQLite rejected the query as
ambiguous and GET /api/missing/{id} returned 500. Read now loads the file with
MediaFileRepository.Get, which qualifies the column, and returns not found
when the file does not exist or is not marked missing.
* fix: report missing rows on single-item deletes and adopt deluan/rest errors.Is
Bump github.com/deluan/rest to the version whose controller matches errors
with errors.Is and errors.As. model.ErrNotFound stays the same value as
rest.ErrNotFound, so the many hand-written conversions from model.ErrNotFound
to rest.ErrNotFound in repositories, core services and test mocks did nothing.
Remove them, along with the duplicate rest.ErrNotFound check in the Subsonic
error mapper. Mappings from model.ErrNotAuthorized stay, as those are
different errors.
User and transcoding deletes had the same silent success as radio: the shared
delete helper never reports a missing row, so their not-found checks never
fired and DELETE /api/user/{id} and /api/transcoding/{id} returned 200 for
unknown ids. Add deleteByID, which returns model.ErrNotFound when no row
matched, and use it for radio, user and transcoding. Also drop the dead
sql.ErrNoRows branch from delete, since a DELETE never returns it.
The Subsonic deleteUser endpoint is not implemented (501), so this does not
change the Subsonic API. Plugin deletes keep the silent helper: there is no
REST route for them, and the plugin manager only deletes rows it just read.
* chore: drop ErrNotFound comment and its identity test
The alias to rest.ErrNotFound is self-explanatory, and the identity test only
restated the declaration.
* refactor: alias model.ErrNotAuthorized to rest.ErrPermissionDenied
Like ErrNotFound, make model.ErrNotAuthorized the same value as the rest
library's error, so REST endpoints map it to 403 directly. This removes the
ErrNotAuthorized to rest.ErrPermissionDenied mappings in the library and
playlist REST adapters and the duplicate check in the Subsonic error mapper.
Handlers that check model.ErrNotAuthorized now also recognize
rest.ErrPermissionDenied returned by repositories, so writePlaylistError, the
image upload handlers and the public share handler return 403 for it instead
of their fallback status. The error message changes from "not authorized" to
"permission denied".
The release notes footer pointed to an outdated PikaPods URL and still
listed Danian as a hosting option. Use the PikaPods run URL and Zenith,
matching the links already used in the published v0.64.0 release notes.
The go-taglib WASM compilation cache lived in a shared $TMPDIR/go-taglib-wasm
directory, created with 0700 by whichever user ran first. A second Navidrome
instance running as another user on the same machine failed to read every
file with "permission denied", so its scan found no files.
The updated fork uses a per-user cache directory (go-taglib-wasm-<uid>) and
falls back to running without the cache when it cannot be created or used.
* fix(plugins): align the Python HTTP example with the repo's host-call pattern
Bind http_send with raw memory offsets like nowplaying-py does, drop
guards for fields the host always sends, and document how plugins
without a PDK call host services and which built-in HTTP APIs are
disabled.
* docs(plugins): document the private-address rules for HTTP requiredHosts
Explain in the README and manifest schema that named hosts can't reach
private addresses while IP/CIDR entries and a bare "*" can.
* docs(plugins): document the private-address rules for requiredHosts
Explain in the README and manifest schema that named hosts can't reach
private addresses while IP/CIDR entries and a bare "*" can, for both
HTTP and WebSocket. Inline the single-use HTTP isHostAllowed wrapper.
* feat(plugins): derive Default for Rust host service structs
The ndpgen client.rs template now adds Default to the derive list of host
service structs, as the capability and shared types templates already do.
Plugin authors can now set only the fields they need, for example
HTTPRequest { method, url, ..Default::default() }. The webhook-rs and
discord-rich-presence-rs examples use this form now. The golden files and
the generated nd-pdk-host crate are updated to match.
* feat(plugins): deprecate pdk.NewHTTPRequest in the Go PDK
Navidrome no longer enables extism's http_request host function, so a
request built with pdk.NewHTTPRequest always fails. ndpgen now reads a small
deprecation table and writes a Deprecated: paragraph for the listed extism
functions, in both the WASM wrapper and the native stub. Linters and IDEs
now point plugin authors to host.HTTPSend. The PDK example tests used to
teach NewHTTPRequest. They now use host.HTTPSend and host.HTTPMock.
* docs(plugins): correct requiredHosts rules for websocket and private addresses
Two statements in the plugin docs did not match the code.
The WebSocket section claimed requiredHosts behaves like HTTP. It does not:
host_httpclient.go only consults the allowlist when the list is non-empty and
otherwise falls back to allowing public addresses, while host_websocket.go
always calls isHostInAllowlist, so an absent list blocks every connection.
The HTTP section claimed a named host can never reach a private address.
checkPrivateDial scans the whole requiredHosts list, so a named host does
reach a private address when the same list also holds a covering IP or CIDR.
Reworded both, plus the matching requiredHosts descriptions in
manifest-schema.json, and regenerated manifest_gen.go.
* fix(plugins): apply the private-address dial guard to WebSocket connections
The WebSocket host service only matched the host string against
requiredHosts, so an allowlisted name resolving (or rebinding) to a
private address was dialed. Share the HTTP client's resolved-IP check
and allowlist matching, so WebSocket follows the same rules: named hosts
can't reach private addresses, literal IP/CIDR entries and a bare "*"
can.
* refactor(plugins): drop redundant WebSocket dial timeout and tidy guard tests
* fix(share): always assign the authenticated user as share owner
A share's UserID was taken from the request body and only defaulted when
empty, so any authenticated user could create a share attributed to
another user. For playlist shares the contents are resolved in the
owner's library-access context, turning the spoofed owner into an
access-escalation vector in multi-library setups.
Force the owner from the request context at both the service boundary
and the persistence layer, ignoring any client-supplied UserID.
* fix(plugins): block SSRF to private IPs resolved from hostnames
The HTTP host client only checked the literal host string, so a symbolic
hostname (or a trailing-dot "localhost.") resolving to a private/loopback
address bypassed the SSRF guard when a plugin declared no requiredHosts.
Enforce the check at dial time via net.Dialer.Control on the resolved IP,
which also covers redirect hops and DNS rebinding. When an explicit
requiredHosts allowlist is set, defer to it as the operator's trust decision.
* fix(plugins): gate private IPs on explicit IP/CIDR allowlist entries
Following review feedback: an allowlisted hostname authorizes the external
service, not whatever private IP it may resolve or rebind to. Enforce the
resolved-IP guard even when requiredHosts is set, permitting a private
address only when a literal IP or CIDR entry explicitly covers it. This
keeps "reach this external API" and "reach my internal network" as two
separate, explicit operator decisions.
* fix(plugins): treat unspecified addresses as private in the SSRF guard
Dialing 0.0.0.0 or :: reaches the local host, so they bypassed the
private/loopback check.
* fix(plugins): let a bare "*" allowlist reach private addresses
Plugins such as AudioMuse-AI declare requiredHosts ["*"] to reach a
user-configured service on the LAN, whose address the manifest cannot
know. Requiring a literal IP/CIDR entry broke them. Named hosts and
subdomain wildcards still cannot resolve to private addresses.
* refactor(plugins): simplify the SSRF-guarded HTTP client and release its pool
Build the client directly around the guarded transport instead of
replacing a throwaway one, fail closed on an unparseable dial address,
and close the per-plugin transport's idle connections when the plugin
unloads. Trim stale comments.
* fix(plugins): stop enabling extism's unguarded http_request host function
Passing requiredHosts as the extism manifest's AllowedHosts enabled
extism's own http_request (pdk.NewHTTPRequest), which only glob-matches
the hostname and follows redirects without re-checking, bypassing the
resolved-IP SSRF guard. Plugins must use host.HTTPSend.
* fix(plugins): move bundled Rust examples to the host HTTP service
Extism's built-in http_request is now disabled, so the webhook and
Discord examples switch to nd_pdk::host::http::send. Update the README
to say host.HTTPSend is the only supported way to make HTTP requests.
* fix(plugins): move the Python example to the host HTTP service
coverartarchive-py used extism's built-in Http.request, which is now
disabled. Call Navidrome's http_send host function instead. The plugin
can no longer run under the standalone extism CLI, so drop the CLI test
targets and instructions.
* feat(cli): add missing file list and remap subcommands
Signed-off-by: zerovox <933064+zerovox@users.noreply.github.com>
* fix: prevent remapping from dropping participants on target track
* fix: after remapping, refresh stats synchronously
* fix: only move album annotations if moving a track would empty the old album
* fix(persistence): keep the new item's annotation when reassigning onto an item the user already annotated
ReassignAnnotation was a plain UPDATE; the annotation table is unique on
(user_id, item_id, item_type), so when a user had annotated both items the
statement aborted and none of the rows moved. In the scanner that surfaced as
a warning; in the missing-file remap it rolled back the whole operation.
UPDATE OR IGNORE moves what it can and leaves the conflicting rows for GC.
* fix(core): keep the target track's history when remapping a missing file onto it
The remap discards the target's row, and GC then dropped its play counts,
stars, ratings, bookmarks and every playlist entry pointing at it. That is
harmless in the scanner, whose target was imported seconds earlier, but the
CLI lets the user pick any existing track. Move those references onto the
surviving id first; where a user already has a row for both, theirs on the
missing file wins.
* fix(persistence): stop FindByPaths dropping plain paths that contain a colon
Any colon was taken as the libraryID separator, and a non-numeric prefix
made the whole path vanish from the lookup. 'missing fix' then rejected the
very paths 'missing list' printed, and M3U imports silently skipped such
tracks. Only a numeric prefix qualifies a path now.
* perf(cli): stream 'missing list' instead of loading every missing file into memory
GetAll materialised the whole result set before a single row was written;
on a library with 97k missing files that peaked at 1.28 GB of RSS. Iterate
the repository cursor and write rows as they arrive.
* refactor(core): tidy the missing-file remap
Drop the log lines copied from deleteMissing that still said 'after deleting
missing files', the debug-on-success branches, and the what-comments; build
the affected album list without slice helpers.
* fix(cli): move path to the last column of 'missing list'
Path is the only variable-width field, so leading with it misaligns every
row that follows. Applies to both csv and json.
* fix(persistence): also try a numeric colon prefix as a plain path
'1999: A Different Life/01.mp3' parsed as library 1999 plus a truncated path
and matched nothing. The prefix is ambiguous, so search both ways.
Also buffer the json branch of 'missing list', which wrote a syscall per row.
* fix(persistence): move scrobbles and buffered scrobbles off a discarded media file
Both tables carry ON DELETE CASCADE on media_file_id, so 'missing fix'
deleting the target erased its play history and dropped scrobbles still
waiting on an external service. scrobble_buffer needs OR IGNORE for its
unique (user_id, service, media_file_id, play_time).
* fix(persistence): recompute the cached average rating after merging annotations
Merging the discarded row's annotations grows the rating population of the
surviving track, so media_file.average_rating no longer matched what the
annotation rows say. Only reachable since the remap started merging those
rows instead of deleting them.
* fix(persistence): recompute the cached average rating inside ReassignAnnotation
Moving annotation rows always changes the new item's rating population, so
the recompute belongs with the move rather than at each call site. Covers
the album reassign in the remap and the two scanner sites, and replaces the
explicit call ReassignReferences was making.
Album was the worse case: rate an album, move its files, and 'missing fix'
handed the rating to an album still caching an average of 0.
* fix(cli): let libraryID:path win over a file literally named like one
FindByPaths searches a numeric-prefixed reference both ways, so a top-level
file named '1:foo.mp3' can tie with library 1's 'foo.mp3'. The CLI then
rejected the reference as ambiguous while advising the exact syntax the
caller had used. Also disambiguates the same path in two libraries, which
is what the qualified form is for.
---------
Signed-off-by: zerovox <933064+zerovox@users.noreply.github.com>
Co-authored-by: Deluan Quintão <deluan@navidrome.org>
* Implementing the RememberProperty pattern for the settings set
through the UI on install. Previously, these got reset on upgrade.
This is as simple as squirrelling them away in the registry for
all settings except for the INSTALLDIR, as the INSTALLDIR depends
on the environment that the msi is being installed into (i.e. the
actual location of ProgramFiles can be anywhere technically), that
needs to be dynamically set after the CostFinalize phase and in the
version of WiX schema supported by wixl needs to be implemented
through a customAction.
This will not fix the upgrade issue for existing installs, as the
information entered doesn't exist in the registry or anything so
the best option imo is to backup the navidrome database and config
uninstall the old version and install the new version with the
desired paths. It should then upgrade from there on correctly.
* Make it possible to build 386 and amd64 on the same machine
* When upgrading from the pre-fix installer, it would dump everything
into the C:\ root as the UI never executes to set the value for
the MSI_INSTALLATIONDIRECTORY, and the custom action doesn't run
on upgrade as we should be reading from the registry in that situation
This will force the customaction to run when the path is the confusingly
named TARGETDIR (which is C:\ in 99% of cases).
All other properties when upgrading from the pre-fix to the fix
will be reset to the default values as well; which was the same
as the previous behaviour anyway.
* build(msi): drop local ffmpeg download cache
The cache key did not include the ffmpeg version, and a partial download
would stick forever. CI runners start clean, so the cache only helped
local builds.
* fix(msi): set install directory during silent installs and upgrades
SetInstallDirProperty only ran in InstallUISequence, which Windows
Installer skips for /passive and /qn (the modes winget uses). With
MSI_INSTALLATIONDIRECTORY unset, the files and the service went to the
root of the drive with the most free space. Upgrades from releases that
did not store MSI_INSTALLATIONDIRECTORY in the registry hit the same
path, even with the full UI.
The action now runs before CostFinalize in both sequences, whenever the
registry search did not find a saved directory, and defaults to
[ProgramFiles64Folder]Navidrome\ (ProgramFilesFolder on x86) so it does
not depend on INSTALLDIR being resolved. The unused INSTALLDIR directory
is removed.
Verified on a GitHub Actions Windows runner: fresh installs with /passive,
/qn and /qr, upgrades from 0.63.1 with /passive and /qr, and an upgrade
between two fixed builds that keeps a custom directory and port.
---------
Co-authored-by: Deluan Quintão <deluan@navidrome.org>
* feat(db): add repair command to rebuild a corrupted FTS5 search index
A corrupted media_file_fts index made every scan fail with 'database disk
image is malformed', and sqlite3's built-in 'rebuild' command cannot repair
contentless FTS5 tables, leaving users to hand-drop tables and triggers.
Add 'navidrome db repair': it runs PRAGMA integrity_check, and when the
reported corruption is confined to the FTS5 search tables, drops and
recreates the three tables and their nine triggers and repopulates them
from the base tables (which hold all the data, so nothing is lost). The
result is verified with the FTS5-native 'integrity-check' command, which
reads only the rebuilt indexes instead of re-scanning the whole database
(on a 761MB production copy: ~9s full check, ~1s rebuild, sub-second
verify). A --rebuild flag forces the rebuild even when the check passes,
for silently desynced indexes. The rebuild refuses to run while migrations
are pending, and a schema-comparison test guards the duplicated DDL against
drifting from the migration.
The DbPath existence check and the YES confirmation prompt, previously
copy-pasted across the backup commands, are extracted into shared cmd
helpers used by both backup and repair.
Part of #6067
* fix(db): type the FTS migration version as int64 for 32-bit builds
The untyped constant defaults to int, which overflows on arm/v7 and 386.
* feat(db): split repair into 'db doctor' and 'search rebuild' commands
A single 'db repair' command promised more than it delivered: the only thing
it could actually repair was the search index, and its diagnosis and its fix
were welded together, so a forced rebuild paid the full integrity check twice.
Split it: 'navidrome db doctor' is strictly read-only, runs both PRAGMA
integrity_check and PRAGMA foreign_key_check, and routes the user (to
'search rebuild' when corruption is FTS-only, to backup/.recover otherwise).
'navidrome search rebuild' just rebuilds and verifies the FTS index, which
takes ~2s on a prod-size library instead of ~19s.
* refactor(cmd): extract a testable doctor function and bound foreign key output
Extract the doctor routing (check, classify, advise) into a function that
takes an io.Writer, so the advice paths are unit-tested and the process exit
happens in the cobra wrapper after the DB is closed (os.Exit was skipping the
deferred close, leaving WAL/SHM files behind on the unhealthy paths).
Aggregate foreign_key_check by (table, parent): the raw pragma emits one row
per orphan, which is unbounded output on a large corrupted library. Also
make confirmYES take an io.Reader, drop the unused return from the renamed
requireExistingDB, share the FTS table list with the tests, and stop the
schema-guard specs from paying for a seeded database they never use.
* docs(cmd): promise 'never alters your data' instead of 'never modifies the database'
Closing the doctor's connection can checkpoint a stale WAL into the main
file (as any SQLite tool does), so the byte-level claim was too strong. The
checks themselves are read-only and no logical content ever changes.
* fix(cmd): make 'db doctor' advice honest when checks are inconclusive
PRAGMA integrity_check stops at 100 errors and emits no marker row, so a
saturated result was being read as the whole picture. IntegrityCheck now sets
the limit itself and reports saturation as a truncated list, and doctor no
longer claims corruption is limited to the search index in that case.
Foreign key violations now print a next step instead of only flipping the
exit code: migrations run with foreign_keys off, so orphan rows are a
realistic leftover on a database that is not corrupt.
Also corrects the 'search rebuild' help, which promised that 'db doctor'
detects when a rebuild is needed -- integrity_check cannot see an index that
is merely out of sync; gives the never-migrated case its intended message
instead of a raw 'no such table: goose_db_version'; and extracts
rebuildSearchIndex so the database is closed before log.Fatal exits.
* refactor(cmd): promote 'db doctor' to a top-level 'doctor' command
The 'db' group held a single subcommand, and the checks planned for it reach
past the database: config, music folder permissions, external tools. None of
those belong under 'db'.
Promoting it also evens out the shape of the pair. The command that finds the
problem is now top-level alongside 'search rebuild', the command that fixes
it, matching the 'brew doctor' convention users already expect.
'db doctor' has never been released, so no alias or deprecation is needed.
* refactor(db): tighten the doctor and search rebuild internals
Follow-up cleanup with no behaviour change except where noted.
integrity_check now asks the pragma for one row beyond the reported limit and
treats that extra row as the proof it truncated, instead of inferring truncation
from a saturated count. That distinguishes a list of exactly 100 issues from one
that was cut short -- the old test could not, and 100 was SQLite's own default,
so passing it was a no-op.
ForeignKeyCheck returns []FKViolation instead of pre-formatted English, moving
the prose to the layer that already owns the CLI vocabulary. The goose table
probe shared with isSchemaEmpty becomes hasGooseTable, so 'has this database
ever been migrated' has one spelling. Also folds ftsMigrationApplied into
requireFTSMigration, lifts printFindings out of a closure that captured nothing,
names the FTS trigger suffixes once, and corrects the ftsSchemaDDL comment: the
drift test compares against the full migration chain, not the single frozen
migration it claimed.
* fix(db): verify the rebuilt search index before committing it
RebuildFTS committed its transaction and only then ran the FTS5 integrity
check, from the caller. A rebuild that produced a bad index was therefore
already persisted by the time anyone noticed, leaving the user worse off than
before they ran the command.
The check now runs inside the transaction, so a rebuild that does not verify
rolls back and leaves the original index in place. VerifyFTS keeps its *sql.DB
signature for callers outside a transaction; the shared body takes the small
execer interface that both *sql.DB and *sql.Tx satisfy.
Adds a spec for the rollback: it removes a column the repopulating SELECT
reads, so the transaction fails after the drops, and asserts the old index
still answers queries.
* refactor(cmd): drop the unused io.Reader parameter from confirmYES
The reader was added as a test seam that no test ever used: all three callers
pass os.Stdin. Back to fmt.Scanln, which drops the parameter and the now-unused
os import from backup.go and search.go.
* fix(cmd): stop promising a scan clears every foreign key violation
doctor told the user to run 'navidrome scan -f' for any foreign key
violation. SQLStore.GC only purges albums, artists, folders, annotations,
bookmarks, tags and playlist tracks, so orphans elsewhere survive it and the
next doctor run still reports them. player.user_id references user(id) and no
scan phase touches that table at all.
The advice now says a scan clears some of them and the rest have to be removed
by hand, which keeps the next step the earlier round asked for without claiming
a cleanup that does not happen.
* docs(db): trim over-long comments on the doctor and rebuild paths
Six comments ran past two lines or repeated something already stated nearby.
The RebuildFTS doc claimed the rebuild rolls back on a column mismatch, which
the new 'verifies before committing' sentence already implies, and a spec
comment restated that same rationale a second time.
* docs: drop em dashes from the comments added in this branch
* fix(db): fail restore when the backup file does not exist instead of wiping the database
`navidrome backup restore -b <file>` passed the flag value straight to the
SQLite driver, which opens databases with SQLITE_OPEN_CREATE by default. If
the file was not found (for example a file name relative to the working
directory instead of the backup directory), the driver silently created an
empty database and the backup API copied that emptiness over the live
database, reporting 'Restore complete' with an empty instance afterwards.
Two changes:
- db.Restore now opens the backup file read-only, so a missing file is an
error and nothing gets created or overwritten.
- A relative --backup-file is resolved against Backup.Path, the same folder
'backup create' writes to; absolute paths keep working as before.
Fixes#6083
* fix(db): stat the backup file instead of opening it read-only
The read-only DSN added in the previous commit works for the reported case but
breaks on other paths: 'file:' + path is parsed as a URI, so a '#' truncates the
path and a '%' sequence is percent-decoded, and a read-only open of a WAL
database leaves '-shm'/'-wal' sidecars next to the backup. Those sidecars then
matched the unanchored prune regex, so 'backup prune -k 3' right after a restore
deleted real backups and kept one.
Stat the file before opening it and keep passing the plain path to the driver.
Paths containing '?' are rejected, since go-sqlite3 splits the DSN there and
would otherwise open (and create) a different file. The prune regex is anchored
so sidecars are never counted as backups.
Also fixes the restore/backup/prune error logs, which printed BasePath (the web
URL prefix) instead of the backup location.
---------
Co-authored-by: Deluan <deluan@navidrome.org>
* fix(artwork): cap declared image dimensions before resizing
resizeStaticImage decoded the image with a raw image.Decode, so a small file declaring huge dimensions (e.g. a PNG header claiming 50k x 50k) forced a multi-gigabyte allocation on the serve-time resize path. The processor already guards its own decodes with decodeCapped; use it here too so the same 64M pixel cap applies to uploaded and sidecar images served through the cache.
* fix(share): validate every resource ID and reject mixed types when saving
Save only resolved the first ID in ResourceIDs to pick the resource type; the remaining IDs were never checked. A non-existent or hidden entity could ride along behind a valid first ID, and IDs of different kinds were accepted as one share. Resolve every ID as the current user and require all of them to be the same kind, returning ErrNotFound or ErrValidation otherwise.
* fix(share): scope album and media file shares to the owner's libraries
loadMedia already loaded artist and playlist shares as the share owner, but album and media_file shares used the repository context. Public share rendering carries no user, so the library filter was skipped and the share listed albums and tracks from libraries the owner cannot access. Streaming was already blocked, so only metadata leaked. Use ownerContext for all resource types.
* fix(server): limit login payload size and surface first-admin creation errors
The unauthenticated /login and /createAdmin handlers decoded the request body
with no size limit. Add a body-limit middleware to the /auth route group that
caps the payload at 8KiB, which is plenty for a username and password. Also
make createAdminUser return the datastore error instead of logging it and
returning nil, which previously let createAdmin proceed to a login attempt for
a user that was never saved.
* fix(conf): create the log file readable only by the owner
The log file was created with mode 0644, so other local users could read it. Logs can contain usernames, paths and, at trace level, request details, so create it with 0600 instead. Existing files keep their current mode.
* fix(lastfm): stop logging the auth token when fetching the session key fails
The Last.fm callback token was written to the log as a structured field on failure. The redaction hook only matches value patterns, so it was not masked. Drop the field; the request ID is enough to correlate the failure.
* fix(db): allow a music folder path containing a single quote on fresh databases
The library table migration interpolated conf.Server.MusicFolder into the SQL with fmt.Sprintf, so a path such as /music/Rock 'n' Roll produced invalid SQL and the migration failed on a brand new database. Bind the path as a parameter instead.
* fix(scanner): return an error when the folder watcher cannot start
When notify.Watch failed, the watcher goroutine logged the error and exited, but never signalled the started channel, so Start blocked until its context was cancelled and left the watching flag set. Call notify.Watch before spawning the event loop, so Start returns the error right away, the started/failed signalling goes away, and the storage can be watched again later.
* fix(jellyfin): limit the login request body size
The Jellyfin AuthenticateByName endpoint decoded its JSON body with no size limit, the same gap the native /auth routes had. Export the login body-limit middleware from the server package and apply it to the Jellyfin login route, before the optional per-IP rate limiter, so both unauthenticated login surfaces share the same 8KiB cap.
* fix(scanner): share one scanner instance across all injectors
Each wire injector built its own scanner controller, so the Subsonic and native API routers held a different instance from the ones used by the startup scan, the periodic scan, the folder watcher and the SIGUSR1 handler. Status reads the in-progress file and folder counters from its own instance, so getScanStatus reported scanning=true with count=0 for every scan not started through the API. Verified live with a startup scan: master reports count 0 while scanning, this branch reports the real counts. Expose the controller through a singleton, as the watcher, broker and play tracker already are, and wire everything to it. New stays available for tests that need isolated controllers.
* fix(share): do not panic when a media file share has no visible tracks
Share.CoverArtID picked a random track for media file shares without checking that any track was loaded. The tracks are empty when the files went missing, were deleted, or the owner lost access to their library, and the public share page then panicked inside the random pick and returned a 500. Return an empty artwork ID instead, so the page renders with the placeholder cover. The old guard on the split resource IDs was dead code, since SplitN always returns at least one element.
* fix(ui): prevent Safari album grid resize when top menus open
* fix(ui): disable scroll lock for all popovers
---------
Co-authored-by: Deluan Quintão <deluan@navidrome.org>
Bumps all 13 direct dependencies that had newer releases, plus the
go-taglib fork pin. No source changes were needed.
The jwx bump to v3.3.0 carries a security fix (GHSA-4cf7-xm37-g63h):
custom claim, header and JWK names were written unescaped, so a name
containing a quote could inject extra members. Navidrome is not
affected - every claim name we emit is a hardcoded literal - but the
fix is worth taking. cascadia v1.3.5 similarly limits selector nesting
to avoid a stack overflow, and our only selector is a constant.
go-sqlite3 v1.14.52 is the only bump with real behavior change: it
flushes the statement cache on schema changes, steps cached statements
eagerly, and drops the per-row goroutine used for query cancellation.
goose v3.28.0 raises its minimum to Go 1.26 and otherwise only touches
MySQL, ClickHouse and Azure SQL, which we do not use. The golang.org/x
bumps are routine. govulncheck reports no reachable vulnerabilities.
The taglib fork pin picks up two fixes. Audio properties are now
clamped with std::max(0, ...) before the unsigned conversion, so a
malformed file no longer reports a duration of ~49 days; this ports
upstream sentriz/go-taglib 0524e91 and additionally covers
bitsPerSample, which is specific to this fork. Bit depth is also now
reported for DSDIFF, TrueAudio and Shorten, which previously returned
0. Both values reach media_file only on re-extraction, so existing
libraries need a full scan to pick them up.
The RealIP middleware rewrote RemoteAddr from the True-Client-IP, X-Real-IP
and X-Forwarded-For headers on every request, including when no trusted
reverse proxy was configured. The login rate limiters on /auth/login and the
Jellyfin /Users/AuthenticateByName derived their bucket from that value, so
an unauthenticated client could rotate a forwarding header and get a fresh
bucket for every password attempt, defeating the brute-force protection.
Resolve the client IP with chi's ClientIPFrom* middlewares instead. The
forwarding headers are only honoured when ExtAuth.TrustedSources is set and
the connecting peer is in that list, reusing the trust check that external
authentication already applies; otherwise the peer address is used. The
X-Forwarded-For chain is now walked against the trusted CIDRs rather than
taking its leftmost entry, so a spoofed value prepended by the client is
skipped.
Both limiters now key on the resolved address. The resolved address is still
mirrored into RemoteAddr, so request logging, player registration and the
Jellyfin local-network check keep reporting the client rather than the proxy.
Reported by gehan-psbc.
* fix(subsonic): honor DefaultDownloadableShare in createShare
The DefaultDownloadableShare option was only sent to the web UI, which used
it to pre-tick the "Allow Downloads?" checkbox. The Subsonic createShare
handler built the model.Share without touching Downloadable, so it fell back
to the Go zero value and every share created through the API was stored as
non-downloadable, regardless of the configured default.
createShare now reads an optional downloadable parameter and falls back to
conf.Server.DefaultDownloadableShare when the client omits it, matching the
web UI. Fixes#6119.
updateShare had a related problem: core's share repository wrapper always
writes the downloadable column, but the handler never set the field, so any
updateShare call silently reset the share to non-downloadable. It now loads
the current share and uses its value as the fallback.
* refactor(subsonic): trim the share downloadable lookup and align with the UI
updateShare fetched the share with Get to recover the stored downloadable
flag, which also runs loadMedia and materializes every album and track the
share points at, just to read one boolean. It now uses Read, which skips
loadMedia, and only queries at all when the client omitted the parameter.
createShare now ANDs the default with EnableDownloads, matching what the web
UI already computes, so both paths apply the same rule.
The specs collapse the create-path matrix into a DescribeTable, reuse the
existing albumIDByName helper, and set the request-time config after
setupTestDB so it does not leak into the config snapshot.
* fix(subsonic): keep the share description on a downloadable-only update
updateShare read the description straight from the request, so a client that
sent only id and downloadable got an empty string written over the stored
description. shareRepositoryWrapper.Update always writes that column, so the
description was silently erased.
This predates the downloadable parameter added earlier in this branch: any
updateShare that omitted description already cleared it. Adding the parameter
just made it easy to hit, since toggling downloads is a natural reason to call
updateShare without touching the description.
Both fields now use the presence-aware accessors and fall back to the stored
share, which still costs at most one read and none when the client sends both.
An explicitly empty description still clears the field.
The Default Bit Rate dropdown on the Transcoding create/edit forms was fed
BITRATE_CHOICES, which starts at 32. There was no way to pick 0, and the
SelectInput was not resettable, so an admin could neither create nor restore a
transcoding with no default bit rate, such as the default FLAC one (seeded with
0 in consts.DefaultTranscodings). Editing that row also rendered a blank
dropdown, since its stored value matched no choice.
Adds TRANSCODING_BITRATE_CHOICES, which prepends a 0 entry labelled 'None' to
the shared list. The forms use it as SelectInput choices, and the list and
read-only show view render it through SelectField, so all four screens resolve
the label from the same array and cannot drift. The shared BITRATE_CHOICES is
left untouched, because 0 is not a meaningful option for the player Max. Bit
Rate or the share dialog.
Reported in discussion #6107, where a user had deleted the default
transcodings and could not recreate the FLAC one.
The theme rounded the cover image directly and set a border radius on
albumContainer, which has no background or clipping, so it rounded
nothing. The hover overlay is a sibling of the image inside the same
link, so it kept square corners that poked out over the rounded cover.
Move the radius to that link and clip it, so both the image and the
overlay follow the same rounded box. This also covers the mobile bar,
which is always visible.
Fixes#6110
The comment justified anonymous access with "item ids are unguessable".
That is not true: an artist id is a deterministic, unsalted hash of the
artist name, id.NewHash(id.NewHash(str.Clear(lower(name)))), so it is
computable offline by anyone who knows the name.
The real reason the route is public is that upstream Jellyfin's is too.
ImageController.GetItemImage carries no [Authorize] attribute (verified on
v12.0, master/13.0.0, v10.11.9 and v10.10.7), and an anonymous request
reaches LibraryManager.ItemIsVisible with a null user, which returns true
unconditionally. Clients build cover URLs with no credentials at all, so
requiring auth here would break them.
No behavior change.
The FLAC muxer writes STREAMINFO before it knows the stream length, then
rewinds at the end to fill total_samples in. Navidrome pipes ffmpeg's stdout
(-f flac -), which is not seekable, so ffmpeg logs "unable to rewrite FLAC
header" and the field stays 0. A decoder needs total_samples to turn a
timestamp into a byte offset, so it reports an unknown duration and refuses to
seek. Online playback hides this because the client re-requests with a new
offset each time, but an offline copy is permanently unseekable, the symptom
reported against Symfonium where seeking a downloaded track jumps back to the
start.
Transcode now wraps its own output and rewrites total_samples as the first
bytes flow past. This lives in core/ffmpeg because the unseekable pipe is that
package's doing: buildDynamicArgs is what appends the trailing '-'. core/stream
only learns a target format and hands back an io.ReadCloser, so compensating
there leaked a transcoder implementation detail one layer up. TranscodeOptions
grows a Duration field alongside the existing Offset, which also puts the
duration-minus-offset arithmetic in the same function that emits -ss.
The wrapper runs on every transcode rather than only FLAC targets: the format
on a transcoding row is a declared target that nothing validates against the
command's actual -f, so a custom command can emit FLAC under any target_format.
The magic-byte check inside the wrapper is the authoritative test and costs a
26-byte peek. The output sample rate is read back out of the header ffmpeg just
wrote rather than taken from the transcode options, so a resampled (-ar) output
still gets the right count. Anything that is not a FLAC stream with an unset
total_samples passes through byte for byte.
Measured on a 177s source: before, total_samples=0 and ffprobe reported
duration N/A; after, total_samples=7807023 and duration 177.03s, with the audio
payload byte-identical. This affects every piped FLAC regardless of the source
format; only FLAC stores an authoritative "unknown", which is why mp3, opus
and aac survive the same pipe.
No SEEKTABLE is synthesised and the MD5 is left zero: both are optional, and
decoders binary-search using total_samples alone.
When a player has a forced transcoding format, ClientInfo.ForceFormat cleared
DirectPlayProfiles unconditionally. A FLAC source on a player configured to
transcode to FLAC was therefore re-encoded to FLAC, wasting CPU and bandwidth
for no gain. Worse, the transcoder pipes ffmpeg output to stdout, so the
resulting FLAC has total_samples=0 and no seek table -- an offline copy of it
can never be seeked. Reported against getTranscodeDecision by the Symfonium
author.
ForceFormat now rebuilds DirectPlayProfiles from the matching transcoding
profiles instead of dropping them: a client declaring a transcoding profile for
a format is proof it can consume that format, so a source already in it is
served as-is. Container and codec come from resolveTargetFormat, so a legacy
"oga" target_format yields an ogg/opus profile, and the profile's
MaxAudioChannels is carried across.
DirectPlayProfile has no bitrate field, so restoring direct play needs a
ceiling to keep an over-bitrate source out of it. GetTranscodeDecision now
seeds that ceiling from the transcoding row's DefaultBitRate when a format was
successfully forced, with the player's own MaxBitRate still taking precedence.
This also closes a gap where the new endpoint ignored DefaultBitRate entirely:
an mp3 320 source on a player forced to mp3@192 was served at 320, while the
legacy /rest/stream path correctly gave 192.
Applied via CapBitrate, which only ever lowers, so a client declaring a
stricter limit keeps it. The legacy path (applyServerOverride) is untouched --
ForceFormat has no other callers.
* added catppuccin mocha theme
* added catppuccin mocha theme to index.js
* fix syntax
* add catppuccin frappé theme
* made frappe and mocha themes more consistant with official color palette and added comments for easy verification
* same for macchiato, seperate commit in case original is preferred
* added comments to .js files
---------
Co-authored-by: Deluan Quintão <deluan@navidrome.org>
* docs: point jftui client link to canonical repository
* docs: fix relative path to webhook-rs example in nd-pdk-host README
* docs: fix capability schema paths in plugin examples README
---------
Co-authored-by: Deluan Quintão <deluan@navidrome.org>
The spec asserted on artistID("Kraftwerk") and intermittently found zero artists
on Windows. The scan did import the artist; it was then made invisible.
RefreshStats selects touched artists with a strict artist.updated_at >
library.last_scan_at (persistence/artist_repository.go:466). Windows' wall clock
has ~15ms granularity, so a new artist written by a quick scan can land in the
same tick as the previous scan's last_scan_at and be excluded. Its
library_artist.stats then stays at the '{}' default and the unscoped cleanup
DELETE removes the row, after which selectArtist's INNER JOIN on library_artist
hides the artist from GetAll.
Backdate last_scan_at before the scan so the comparison is unambiguous, matching
the fix already applied to the search_normalized spec below it.
* fix(i18n): Update Japanese translation
* fix(i18n): fix Japanese translation
* fix(i18n): fix Japanese translation
Update Japanese translations for `recentlyAdded`, `recentlyPlayed`, and `mostPlayed` in album lists
---------
Co-authored-by: Deluan Quintão <deluan@navidrome.org>
Jellify's Discover tab calls GET /Items/Latest and got a 404: we only routed the
/Users/{userId}/Items/Latest form, which real Jellyfin marks [Obsolete] and hides
from its OpenAPI spec, so SDK-generated clients never see it. Add the current
route alongside the legacy one, both served by the same handler.
getLatest also ignored ParentId, so browsing a library or an artist returned the
newest albums across everything the user can see. It now scopes to the library
when ParentId names one, and filters to that artist's albums otherwise, which
also makes a stale id return nothing instead of silently widening back to the
full library set. A malformed ParentId 404s, matching /Items and the contract
decodeFilterParam documents.
PlaybackInfo returned a TranscodingUrl prefixed with the /jellyfin mount path.
Clients concatenate that value onto a server base URL that already carries the
prefix, producing /jellyfin/jellyfin/Audio/{id}/universal and a 404, so playback
never started. Jellify, jellyfin-web, jellyfin-vue and Streamyfin all consume the
field this way; real Jellyfin emits it server-relative (StreamInfo.ToUrl is called
with a nil baseUrl).
Emit the path server-relative to match. Finamp is unaffected: it builds its own
stream URLs and never reads the field.
`buildPlaylist` reported `evaluated_at` as `changed` for smart playlists, so a
rename or comment edit was invisible to clients until the next evaluation. A
never-evaluated smart playlist also reported the current time on every call,
which never settled.
Report `updated_at` for every playlist. `refreshCounters` now syncs the stamp it
writes back onto the model, and `refreshSmartPlaylist` reuses it for
`evaluated_at`. `changed` therefore still equals the evaluation time for a
just-evaluated playlist, and `validUntil` stays anchored to it.
Original Subsonic always bumps `changed` on any playlist update, so this also
aligns the behavior with upstream.
The Refresh Metadata action was only reachable from the Album and Artist context menus, so it could not be triggered from AlbumShow or ArtistShow. This adds an icon-only button, with a tooltip, to the action toolbar on both detail pages. Like the menu entry, it is only rendered for admins.
The button is built on react-admin's Button rather than a plain IconButton: the surrounding toolbars use the former, so the theme colour and the icon-only swap at the xs breakpoint are inherited instead of restated. The dataProvider call and its two notifications move into a new useRefreshMetadata hook, which ContextMenus now shares, keeping a single copy of that logic.
A playlistTrack id is a position in the playlist, not a stable key, so refetching
a row by id after the annotation is saved can return a different song: in a smart
playlist filtered on that annotation the track is gone and every later row has
shifted up. The stale-keyed record then renders as a duplicate of its neighbour.
Signed-off-by: Bjørn A. Andersen <polybjorn@users.noreply.github.com>
Co-authored-by: Bjørn A. Andersen <polybjorn@users.noreply.github.com>
Co-authored-by: Deluan Quintão <deluan@navidrome.org>
The exclusion only worked on master. Coverage profiles name files by import
path; octocov shortens those to repo-relative paths using the checked-out
source, but coverage-on-pr.yml sparse-checks-out only .octocov.yml, so the
paths stay as github.com/navidrome/navidrome/tests/mock_*.go and 'tests/**'
never matched. '**/*_gen.go' matched either way, which is why only the 30
tests/ files leaked.
Every pull request since b77fb45 therefore reported ~-3.7% against master:
447 files on the base side, 477 on the pull request side (#6002, #6069).
* chore: ensure that all durations are nonnegative
* make sure you actually include the test file
* test(conf): use non-zero durations in the valid_duration fixture
Zero is the boundary between the accepted and rejected ranges, so it
passes even if the guard is off by one. 1s exercises an ordinary value.
---------
Co-authored-by: Deluan <deluan@navidrome.org>
An absent artwork state was revisited by an hourly job, by viewing the entity, and
by the startup backfill on any artwork config change. On a large library the last
one queued tens of thousands of external lookups at once and got the provider to
rate-limit us for hours.
Nothing revisits an absent state now. Retrying is explicit: `artwork reprocess` on
the CLI, or the refresh button in the UI. The config fingerprint survives only as an
advisory, warning at startup and naming the command that clears it.
Since absent is terminal, `artwork status` splits it into two disjoint columns, and
`--source failed` targets only the ones that gave up rather than being answered.
Both read through the filter CountBySource and EnqueueBySource already share, so the
reported number is the set the command acts on.
Also fixes the last_failure default left by 20260819204637, which marked every
pre-existing absent row as failed, and removes the code the deleted retry paths
orphaned.
The coverage profile counted the tests/ package and the *_gen.go files, none of
which are code under test: tests/ is the mock and helper package, and generated
code is never hand-tested. Together they added 2926 uncounted statements at 0%,
pulling the reported number down by almost 6 points (70.13% -> 75.98% on the
current master profile).
octocov's coverage.exclude takes doublestar globs matched against git-root-relative
paths. All 26 mock_*.go files live under tests/, so the single 'tests/**' pattern
covers them.
* fix(deezer): treat an exhausted quota as a throttle, not as a missing artist
Deezer reports quota exhaustion in the response body, with HTTP 200 and no
rate-limit headers. The client only looked for errors when the status was
not 200, so a throttled reply was decoded into an empty result type, and an
empty search became ErrNotFound. The agent then compounded it: it tested
`errors.Is(err, ErrNotFound) || len(artists) == 0` before testing err, so
any failed search — which also returns no artists — reported not-found too.
The artwork worker settles an entity as "no image" on agents.ErrNotFound.
So being throttled did not make Navidrome back off; it made it record the
artist as having no artwork, and move on to do the same to the next one.
Errors are now parsed out of the body regardless of status, and the quota
code is joined with agents.RetryLaterError so the circuit breaker and the
artwork retry budget see a throttle for what it is. The agent checks err
before the empty-result case.
Last.fm already handles this exact shape (client.go errCodeRateLimit, with
a comment noting the 200-with-body-error pattern); this brings Deezer in
line with it, including the zero-delay RetryLaterError so both providers
share the default cooldown rather than a per-provider number.
Measured against the live API to pin the shape: a 120-request burst
returned 54 results and 66 quota replies, every one of them HTTP 200 with
{"error":{"type":"Exception","message":"Quota limit exceeded","code":4}}
and no Retry-After or rate-limit headers. A single request 5s later
succeeded, so the window is short and a cooldown fully clears it.
* refactor(deezer): fold the error envelope into one type
The envelope declared the code and message inline, parseBodyError copied
them field by field into a second struct with the same shape, and a zero
Code stood in for "no error reported". Making the envelope hold a pointer
to the error type removes all three: absent is nil, present is the error
itself, and the value returned needs no conversion.
searchArtist loses its empty-result branch. searchArtists converts an
empty result to errNotFound and returns early on any error, so it never
answers with no artists and no error, and the branch could not run. What
it left behind was a comment explaining an ordering that only mattered
while the branch existed.
ErrNotFound is unexported: nothing outside this package referenced it,
and it sat three lines from agents.ErrNotFound, which is a different
error with the opposite meaning for callers.
Throttling now joins agents.ErrRetryLater, the sentinel documented as the
zero-delay RetryLaterError, rather than allocating an equivalent value.
* refactor(deezer): return agents.ErrNotFound from the client
The client raised a package-local sentinel that the agent then translated
into agents.ErrNotFound, one call site each. Deezer was the only adapter
carrying its own: last.fm and listenbrainz have none.
The client already reports throttling with agents.ErrRetryLater, so it
already speaks the agent vocabulary; saying "not found" in the same words
costs nothing and lets searchArtist drop to plain error propagation.
* test(scrobbler): remove a race in the longest-server-delay test
newBufferedScrobbler starts its drain goroutine, and run() drains once
before it ever waits on the wake signal. The test enqueued user2, then
enqueued user1 via Scrobble, so that startup drain could land between
the two: it saw only user2, took its 45s delay, and set backingOff. The
wake from the second enqueue is then deliberately ignored — a wake
during a backoff window must not drain, which is the hammering the
window exists to prevent — so user1 was never attempted and the first
assertion read 1 instead of 2.
Buffering both users before the goroutine exists removes the window.
The test no longer goes through Scrobble, which the sibling tests
already cover; what this one is about is which delay wins.
Reproduced deterministically by forcing the interleaving with a
synctest.Wait between the two enqueues, which fails with the same
"expected both users drained, got 1 attempts" seen in CI. With both
enqueued first, that same forced drain passes.
An explicit RetryLaterError now opens the agent's breaker immediately for the
provider's own delay, instead of counting it as one generic failure that needs
five to open and then always probes after a fixed minute.
* ci: comment coverage on pull requests from forks
A pull_request run from a fork gets a read-only GITHUB_TOKEN, so octocov could not post its comment: it logged a 403 and exited 0, leaving the job green and the PR silent. The 'permissions:' block cannot grant what the token does not have.
The comment now comes from a workflow_run workflow, which runs on the base repository and does get a write token. The pipeline job keeps the job summary and the default-branch baseline, and hands the merged profile and the PR number to it as an artifact.
A workflow_run job otherwise looks like a push to the default branch, so octocov is pointed back at the pull request and at the run that produced the profile via its OCTOCOV_ environment overrides. Without the run id override the test execution time would be read from the wrong run; without the ref override a fork's coverage would be stored as the master baseline.
The job holds a write token, so it reads .octocov.yml from the base branch rather than from the fork.
* ci: stop checking out the fork in the coverage comment workflow
CodeQL flagged the pull request checkout as untrusted code in a privileged context (actions/untrusted-checkout/high): the job holds a write token. The checkout existed only so the code-to-test ratio would reflect the pull request, which does not justify the alert.
The workflow now checks out just .octocov.yml from the base branch, and the ratio is skipped when reporting from there. Coverage and its delta against master, the metrics that motivated the report, are unaffected: they come from the profile the pipeline uploads.
* ci: treat the coverage artifact as untrusted input
A pull_request run executes the fork's own copy of pipeline.yml, so every file in the octocov-pr artifact is attacker-controlled. The artifact was extracted into the workspace root, on top of the base-branch checkout, and download-artifact truncates existing files. A fork could therefore replace .octocov.yml before octocov loaded it.
That is not only a config swap. config.Load expands ${VAR} from the job environment and the action sets OCTOCOV_GITHUB_TOKEN, so a crafted comment.message posts the privileged job's token into a public comment; a body: section rewrites a pull request description, which pull-requests: write allows.
The artifact now lands in a subdirectory and only coverage.out is copied out, after pr_number is checked to be digits and the named pull request's head is confirmed to be the sha that triggered this run. Without that check the artifact could aim the comment at any open pull request, and unvalidated content reached GITHUB_OUTPUT.
* ci: report Go test coverage on pull requests
Adds octocov to the existing 'Test Go code' job. It reads the coverage
profile, posts a PR comment with the coverage percentage and the delta
against master, and writes the same report to the job summary.
The master-branch report is stored as a GitHub Actions artifact, so no
external service or secret is needed.
* ci: merge the plugins job coverage into the same report
The plugins suite runs in its own job, so its coverage was missing from
the report. Both jobs now upload their profile as an artifact and a new
'Report coverage' job merges them into a single PR comment.
* ci: update the coverage comment in place instead of reposting
octocov's default is to collapse the previous comment and create a new
one. updatePrevious edits the existing comment instead, so a PR keeps a
single coverage comment across pushes.
* ci: fix octocov timeout and step-time lookup
Storing the report hit the 30s default timeout: scanning this repo's
artifacts for the baseline consumed it first. Raise it to 5m.
The step-time lookup also matched the Windows job's 'Test' step and
waited for a job that was still running, so execution time was dropped
from the report. Rename the step to make it unique.
* ci: only store the coverage baseline from the default branch
* ci: report statement coverage instead of line coverage
octocov reports statement coverage for a single profile but switches to
line counting when it merges several itself, which made the number
disagree with 'go tool cover -func'. Merge the two job profiles into one
file first, so the reported number matches what developers see locally.
* ci: stop the download-link comment from clobbering the coverage report
Both comments are posted by github-actions[bot], and the download-link
job updated the first bot comment it found. On a new PR the coverage
comment is created first, so it would be overwritten. Match on the body
as well, and keep the coverage profiles out of the download list.
* test(plugins): build test plugins in Go instead of shelling out to make
The plugins suite built its .ndp test packages by running `make -C
plugins/testdata`, which needs make and zip on the PATH. That is the reason
the 26 WASM-dependent spec files are tagged //go:build !windows.
buildTestPlugins now does the same work in Go: the same mtime check make
performed, `GOOS=wasip1 GOARCH=wasm go build` per plugin, and archive/zip
for the package. TinyGo was already optional and unused in CI, so nothing is
lost there. The first plugin builds on its own so the shared wasip1 stdlib
and PDK objects land in the build cache before the rest fan out: on a cold
cache that is 2.2s against 3.4s for the sequential make and 7.3s for an
unrestrained fan-out.
Packaging moved into a writeNdp helper shared with createTestPackage, which
was already writing the same two-entry archive. Entries are written in a
fixed order, so the .ndp bytes are now reproducible; the loader hashes those
bytes, and `zip` also stored file mtimes, so the previous packages differed
on every rebuild.
The Makefile is unchanged and still works for building the plugins by hand.
Removing the !windows tags is a separate step, once CI is green here.
* test(plugins): run the WASM plugin specs on Windows
With the test plugins now built in Go, nothing in the suite needs a Unix
toolchain, so the //go:build !windows tags come off all 25 spec files. The
Windows CI job runs `go test ./...`, so it picks the suite up with no
workflow change.
plugins_suite_windows_test.go existed only to bootstrap the handful of specs
that compiled on Windows; plugins_suite_test.go now serves both.
* test(plugins): skip the planted-symlink spec where symlinks need privileges
os.Symlink needs an elevated token or Developer Mode on Windows, so the
unconditional Expect(...).To(Succeed()) would fail for contributors running
the suite on an ordinary Windows box. The elevated GitHub runner hides this.
The equivalent spec in sandbox_fs_internal_test.go already attempts the
symlink and skips on error; this does the same, keeping the pin live
everywhere it can run, including Windows CI.
* fix(ci): stop the Windows ndpgen test failing silently
The ndpgen suite builds its helper binary to %TEMP%\ndpgen-test, and Windows
will not exec a file without an executable extension, so the "supports
verbose mode" spec has been failing there. Nobody noticed because the
Test ndpgen step ran under pwsh, which carries on after a non-zero exit and
takes the step's status from the last command, so the job stayed green with
a FAIL line in its log.
Add the .exe suffix, and run the step under bash like the Linux job does, so
a failure in any of its three commands fails the job.
* fix(plugins): build public URLs on the caller's address instead of localhost
The artwork host service had no `*http.Request`, so it passed `nil` to
`publicurl.ImageURL`. With neither `ShareURL` nor `BaseURL` configured, that
produced `http://localhost/share/img/...`, which is useless to anything outside
the server. The Discord Rich Presence plugin explicitly drops localhost URLs, so
it fell back to the Navidrome logo instead of the real cover art.
`serverAddressMiddleware` already works out the client-facing scheme and host
from the `X-Forwarded-*` headers. It now also records them in the request
context, and `publicurl` takes a `context.Context` instead of an `*http.Request`
so any caller can reach them. Extism passes the caller's context through to host
functions, so plugins invoked during a request now get a reachable URL with no
configuration.
Switching the parameter also removes the need for a second, parallel entry
point: the package previously wanted only a scheme, a host, and a context, and
took a whole request to get them. `AbsoluteURL` no longer dereferences a
possibly-nil request on its parse-error path.
Plugin calls that start from `context.Background()` (scheduler and websocket
callbacks, the buffered scrobble drain) still fall back to localhost, since they
have no request to learn from. A debug log now points at `ShareURL` when that
happens.
* fix(publicurl): include the configured port in the localhost fallback
The last-resort fallback built `http://localhost/...`, which points at port 80
and so is unreachable for a server listening anywhere else — the default 4533
included. Use `conf.Server.Port` so a consumer on the same machine can actually
fetch the URL.
* fix(publicurl): use https in the localhost fallback when TLS is configured
The fallback hardcoded the http scheme, so a TLS-only server with no BaseURL
advertised a URL it does not answer on. Mirror the server's own switch, which
requires both a certificate and a key.
* refactor(publicurl): tidy the localhost fallback and its tests
Use gg.If for the fallback scheme so it reads as an expression, like the
BaseScheme branch above it, instead of assigning http and overwriting it.
Drop two tests the ctx refactor left redundant: one asserted PublicURL "works
without a request" but became a byte-identical copy of the ShareURL spec once
the *http.Request parameter went away, and the two port specs differed only in
the integer, where the non-default port is the stronger assertion.
* refactor(conf): add TLSEnabled and use it instead of repeating the predicate
Whether the server speaks HTTPS was decided inline in three unconnected
places. This PR added the third, in a URL-building package that has no
business inferring the transport config.
Move the rule to conf, next to the fields it derives from, and call it from
publicurl and the insights collector. server.Run keeps its own expression: it
takes the certificate and key as parameters, and its test passes values that
do not come from the config.
* fix(nativeapi): stop partial PUTs from clearing untouched columns
The REST layer parses the request body's top-level JSON keys and passes them
to Repository.Update as colsToUpdate. The radio and library repositories
discarded that list and issued a full-row UPDATE, so any field absent from
the body was written as its zero value.
For radio this wiped uploaded_image, deleting the station's cover on every
partial update (the Web UI is unaffected because its form submits the whole
record). For library it silently cleared remote_path and default_new_users.
Thread the column list through to Put in both repositories, and extract the
column-selection half of filterUpdateValues into selectUpdateColumns so
library, which hand-builds its update map, shares the same rule instead of
copying it.
Fixes#6057
* refactor(persistence): drop pluginRepository's dead rest.Persistable methods
Save and Update had no callers: PUT /api/plugin/{id} is served by the
hand-written updatePlugin handler over a typed request struct, and the
route only wires rest.GetAll and rest.Get. Both methods delegated to Put,
which upserts all twelve columns, so wiring rest.Put to this repository
would have reintroduced the partial-update clobbering fixed in the previous
commit. Removing them, along with the rest.Persistable assertion, makes
that a compile error instead of a silent data loss.
Put itself is unchanged and still backs plugin discovery.
* perf(persistence): avoid a full media_file scan when resolving playlist paths
FindByPaths built one OR-ed equality term per path. On the real media_file
schema SQLite abandons the path index at just two OR-ed terms and falls back to
SCAN media_file, re-testing every term against every row, so the cost grows with
(rows x terms).
Group the candidates by library and emit one IN list per library instead, which
plans as SEARCH media_file USING INDEX media_file_path_nocase. The NOCASE
collation is kept so ASCII case-insensitive matching still works.
This is the dominant cost of M3U playlist import, which resolves every track on
every scan. Measured with a 1000-track playlist against a migrated DB:
100k media_file rows: 397 -> 51,414 tracks/sec
500k media_file rows: 78.5 -> 47,174 tracks/sec
The rate no longer degrades as the table grows, which is the expected shape for
an index lookup. Reported in #6043, where an 8 hour scan of a 2M-song library
spent 7h52m in the playlist phase.
* docs(playlists): correct the stale reason for the M3U lookup chunk size
The expression-tree depth ceiling applied to the old OR-per-path query, which
capped a batch at roughly 500 terms. The IN form is bound by SQLite's 32766
variable limit instead, which the 400 candidates per chunk sit far below.
* fix(plugins): stop the cache janitor when a plugin cache is dropped
newCacheService started a ttlcache janitor goroutine that only stopped via the
explicit Close() path, so a cache service that was discarded without being closed
leaked its janitor for the process lifetime. It now registers the same
runtime.AddCleanup safety net that utils/cache.simpleCache already uses.
* fix(scanner): stop splitting multi-byte characters when truncating tags
sanitize() capped tag values with a byte slice, so a value whose limit falls in
the middle of a multi-byte character was stored as invalid UTF-8. defaultMaxTagLength
is 1024, which is not a multiple of 3, so any sufficiently long CJK title hit this.
Only trailing invalid bytes are trimmed, leaving bad bytes elsewhere in the value
untouched.
* fix(scanner): store MusicBrainz ids in their canonical form
uuid.Parse accepts a UUID wrapped in any two bytes, as well as braced and urn:
forms, but sanitize() returned the raw string. A tag like {<mbid>} or a quoted
value was therefore persisted with its wrapper into the mbz_* columns, where the
exact-match MBID search can never find it. The parsed value is now stored, which
also lowercases uppercase ids and adds the dashes to unhyphenated ones.
* fix(plugins): parse IPv6 hosts correctly in the websocket allowlist
isHostAllowed cut the host at the last colon, which mangles an IPv6 literal:
"[::1]:8080" became "[::1]" and "[::1]" became "[:". A plugin manifest could
therefore never allow an IPv6 host. It now uses net.SplitHostPort, falling back to
unwrapping the brackets when there is no port.
* fix(server): serve pprof profiles when a BaseURL is configured
net/http/pprof's Index resolves the profile name by trimming "/debug/pprof/" from
the raw request path, which never matches once MountRouter prepends the BasePath.
Requests for any profile without an explicit chi route fell through to the index
page, returning HTML with a 200 instead of the profile. The handler now strips the
BasePath first.
* test(scanner): run the goroutine leak check unconditionally
The scanner suite's goleak check only ran when the GOLEAK env var was set, so it
never ran in CI and could not catch a regression. It passes with the existing
ignore list, verified over repeated runs, so the gate is removed.
* fix(server): close the background image body on a non-200 response
serveImage returned early on an unexpected status code without closing the response
body, pinning the connection until the 5s client timeout. The nolint:bodyclose
above the request suppressed the linter that would have caught it, and its
justification only holds on the success path, where the body is handed to the
CachedStream wrapper.
* test(scanner): repair BenchmarkScan so it can actually run
The benchmark failed three ways before reaching its first iteration: it reused a
shared temp DB and tried to repoint the default library, it never loaded the config
defaults so the scanner got a concurrency of 0, and it lacked the notify ignore that
the suite already carries. tests.Init now takes a testing.TB so a benchmark can load
the test config the same way the suites do.
* refactor(artwork): drop the unused sourceFunc Stringer
sourceFunc.String derived a label from the closure's symbol name via reflection, but
nothing called it: the trace output builds its candidate labels from explicit strings.
Whole-program analysis confirms it is unreachable, and dropping it removes a
reflection-based dependency on compiler closure-naming details.
* refactor(plugins): reuse extractHostname in the websocket allowlist
The IPv6 host parsing added for isHostAllowed duplicated extractHostname, which
already lives in the same package and backs the HTTP client's identical allowlist
check. Two copies of a security-relevant parser can drift, so the websocket service
now calls the existing helper. The port-stripping specs move into the URL Validation
block that already covered them.
* perf(scanner): bound the tag truncation trim to a partial rune
The trim loop dropped every trailing byte that failed to decode, so a value ending
in a long run of invalid bytes was walked one byte at a time: a 1 MiB lyrics tag
measured 2.58ms against 45ns for a normal cut. A partial rune is at most 3 trailing
bytes, so the loop is capped there, which also stops it consuming a pre-existing
invalid run.
* test: tighten the tests added with the Go 1.27 bugfixes
Drop the testItem stub in favour of the package's own cacheKey, register the pprof
test profile once at package scope, and replace the hand-rolled goroutine settle
loop with Eventually. Also corrects a comment that credited a TestMain the scanner
suite does not have.
* test(scanner): ignore notify's nonrecursive-tree goroutines on Linux
The goroutine leak check only ignored the recursive tree (macOS/FSEvents).
Linux CI uses inotify, whose nonrecursive tree leaks dispatch and internal
goroutines after Stop(), failing the check.
* fix(scanner): avoid a truncation panic when MaxLength is 1 or 2
A value of only UTF-8 continuation bytes drained the partial-rune loop to
empty, then sliced value[:-1] and panicked. Break when DecodeLastRune returns
size 0 (empty string) by testing size != 1 instead of size > 1.
* fix: address Codex review on the pprof base path and scan benchmark
- profilerHandler: treat a root BasePath ("/") as no prefix, so http.StripPrefix
keeps the leading slash chi needs; without this the profiler 404s when BaseURL
is "/". Cover the root case in the test.
- BenchmarkScan: make it run regardless of test/benchmark ordering. Add
singleton.DeleteInstance so a fresh DB is opened after TestScanner closes the
shared one, guard driver registration with sync.Once so the rebuild does not
re-Register, and ignore the Ginkgo interrupt-handler and Linux notify
goroutines the preceding suite leaves behind.
* fix: address Codex round 2 on BasePath trailing slash and benchmark DB cleanup
- profilerHandler: trim all trailing slashes (TrimRight), not just a bare "/", so
a BaseURL like "/music/" strips correctly instead of 404ing. Cover it in the test.
- BenchmarkScan: keep and defer db.Init's closer so the DB is closed before
b.TempDir cleanup, which otherwise cannot delete the open SQLite/WAL files on Windows.
* test(plugins): make the suite safe to run in parallel processes
Two things broke when the suite ran across several Ginkgo processes.
buildTestPlugins ran in every process, so N copies of make raced in the same
directory. The packaging rule made that worse by staging every plugin through
one shared plugin.wasm, so concurrent targets clobbered each other and left
orphaned temp files behind. That also ruled out make -j.
Stage each package under its own per-target directory, and move the build into
SynchronizedBeforeSuite so process 1 does it once while the others wait.
* ci: run the plugins suite in parallel processes
With the compilation cache warm the suite is bound by spec execution, which
splits cleanly across processes. Run it as its own step with the ginkgo CLI,
already declared as a tool in go.mod, and drop the package from the main go
test invocation so it is not run twice.
Locally, with -race: 69s to 19s warm, and 256s to 96s cold.
* ci: give the plugins suite its own job so it runs concurrently
Running it as a second step in the go job serialised it against the other 90
packages, which cancelled out the parallel win: the job went from 5m41s to
only 5m29s even though the suite itself dropped from ~175s to 82s.
Move it to its own job so the two run at the same time. The WASM compilation
cache moves with it, since the go job no longer runs the suite.
* ci: cache the plugins test suite WASM compilation across runs
The 'Test Go code' job was dominated by a single package: 'plugins' took
541s of the 699s test step. The suite builds 25 test plugins as full-Go
wasip1 modules of ~4.5MB each, and wazero must compile every one to machine
code. Under -race that compiler work is instrumented, so each module costs
around 11 seconds.
The suite already shared a wazero compilation cache, but three things kept it
from paying off. It lived in a fresh temp dir, so nothing survived the run.
The default plugins.cachesize of 200MB was smaller than the 334MB the cache
actually needs, so the purge evicted entries mid-run. And the wasm binaries
embedded VCS stamps, so every commit produced different bytes and missed the
content-addressed cache anyway.
Point CacheFolder at plugins/testdata/.wazero-cache, raise the test cache
limit past what the suite needs, build the test plugins with -buildvcs=false,
and restore the directory in CI. Locally the package goes from 256s to 74s
with the cache warm and the wasm rebuilt from scratch.
* ci: key the WASM cache on what actually changes the modules
The test plugins are separate Go modules with their own go.mod and go.sum;
they reach the PDK through a replace directive and never read the root
module. So the root go.sum has no bearing on the wasm bytes, and the wazero
version it pins is already namespaced by wazero itself, which stores entries
under wazero-<version>-<goarch>-<goos>. Keying on it only rotated the cache
on every unrelated dependency bump.
Drop it, and add the go.mod files that were missing: the test plugins' own
and the PDK's. The root go.mod stays, since it selects the toolchain that
builds the modules.
* ci: key the WASM cache on the toolchain version, not go.mod
Only the Go toolchain in the root go.mod affects the built wasm, but the file
also changes on every direct dependency bump, which would rotate the cache for
no reason. Take setup-go's go-version output instead: it is the version that
actually built the modules.
Moves both the xx-build toolchain stage and the final runtime image from
Alpine 3.20 (past end of active support) to 3.22.
3.22 is the last release where ffmpeg is still 6.1.x — it jumps to 8.0 in
3.23 — so transcoding behavior is unchanged by this bump.
Alpine 3.21 repackaged mesa, and from that release on `mpv` requires
so:libEGL.so.1 and so:libgbm.so.1. Those pull mesa -> llvm20-libs (156MB)
plus the gallium drivers (62MB), which took the image from 231MB/62MB
compressed to 578MB/147MB. mesa-egl is the only provider of libEGL.so.1,
and newer Alpine releases do not improve on this.
Navidrome runs mpv headless for jukebox audio and never enters a video
path, so this replaces libEGL/libgbm with generated no-op stubs and drops
the mesa/LLVM stack. The stub symbol list is read from real mesa at build
time and cross-compiled with the existing xx toolchain, so it adapts per
architecture rather than being hardcoded.
Verified with logging stubs across mp3/flac/ogg/opus/m4a/wav driving the
default MPVCmdTemplate (pause, volume, time-pos seek, quit): zero calls
into the stubbed libraries. The final stage now also runs mpv once at
build time, so a broken stub fails the build instead of shipping.
Image size: 323MB -> 325MB (84MB -> 86MB compressed).
* build: upgrade to Go 1.27
Bumps the toolchain in go.mod, both golang base images in the Dockerfile, and
the devcontainer VARIANT. CI needs no change, as the workflows resolve the
version through go-version-file: go.mod.
Tests, race tests, build and vet all pass on go1.27.0.
* build: upgrade golangci-lint to v2.13.0
v2.13.0 is the first release built with Go 1.27, so it can lint a module whose go
directive is 1.27. It also enables gosec's G404 on math/rand/v2, which flags the
three rand.Shuffle call sites. Shuffle order is not a security decision, and the
crypto-backed alternative in utils/random costs 25x and allocates per swap, so the
call sites are annotated rather than the rule excluded, keeping G404 active for the
cases where it would matter.
* chore(deps): update Go dependencies to latest versions
Signed-off-by: Deluan <deluan@navidrome.org>
* build: bump golangci-lint to v2.13.2
---------
Signed-off-by: Deluan <deluan@navidrome.org>
The default AAC transcode emits raw ADTS (`ffmpeg ... -f adts -`), but
the MIME table mapped `.aac` to `audio/mp4`. Clients that dispatch
strictly on Content-Type could reject the stream because the declared
container did not match the payload.
`.m4a` and `.alac` stay on `audio/mp4`, since those really are MP4.
Fixes#5958
Signed-off-by: Aditya Raj Singh <aditya@bncw.in>
Co-authored-by: Deluan Quintão <deluan@navidrome.org>
The agent name used in the `Agents` config option comes from the .ndp file
name, not from the manifest. The Plugins UI showed the ID but never said what
it was for, so renaming a plugin file silently breaks the config with only a
Debug-level "Unknown agent ignored" line to go on.
Add a caption under the ID in the Plugins UI, and list the accepted names
alongside the rejected one in that log line.
Related to navidrome/apple-music-plugin#14
* feat(agents): retry-later error type with optional server delay
Add agents.ErrRetryLater and agents.RetryLaterError, which carries the
delay requested by an external service (e.g. ListenBrainz's
X-RateLimit-Reset-In). scrobbler.ErrRetryLater becomes an alias of the new
sentinel, so existing errors.Is checks and the plugin error-string protocol
keep working unchanged. Groundwork for honoring server-requested retry
delays across scrobbling, metadata agents and artwork.
Song.Equals tests moved to song_test.go to enable external test package.
* fix(scrobbler): honor backoff window and server-requested retry delay
ListenBrainz 429s were decoded into a typed error that classified as
unrecoverable, silently discarding the scrobble (a JSON-bodied 429 was
measured live). The client now maps any 429 to agents.RetryLaterError,
carrying X-RateLimit-Reset-In when present (capped at 1h). Last.fm error 29
(rate limit) is now retryable like 11/16. The buffer's drain loop no longer
lets wake signals bypass an active backoff window - new plays enqueue but
drain only when the window closes - and the wait honors the server delay
via max(backoff, retryIn).
* feat(agents): skip cooling-down agents in aggregate calls
When an agent reports retry-later, remember a per-agent cooldown deadline
(the server-requested delay, or 1 minute when unspecified) and skip that
agent in all aggregate metadata calls until it passes. A round that found
no data but skipped or saw a throttled agent returns ErrRetryLater instead
of ErrNotFound, so callers cannot mistake rate limiting for a definitive
'no data' answer.
* feat(artwork): honor server-requested retry delay when rescheduling
When an external image lookup fails with a retry-later error carrying a
delay (e.g. a 429 with X-RateLimit-Reset-In), the chain trace carries the
largest such hint back to the worker, which reschedules the item at
max(exponential backoff, server delay) instead of backoff alone.
* feat(plugins): retry-later with optional delay for scrobbler and agent plugins
Scrobbler plugins can now return scrobbler(retry_later:N) to request a
retry in N seconds (capped at 1h); the bare token keeps its old meaning.
Metadata-agent plugins, which had no error vocabulary at all, gain the
parallel agent(retry_later[:N]) token, mapped to agents.RetryLaterError so
the aggregate's cooldown and the artwork worker honor plugin throttling
the same way as built-in agents.
* fix: address whole-branch review findings for retry-later handling
Narrow the aggregate's throttled rule to the spec sentence: core.Agents returns
ErrRetryLater only when no agent answered at all (all skipped-cooling or
retry-later). An agent that does not implement the called method now returns an
internal errUnsupported instead of ErrNotFound, so it counts as "did not run" —
without that, the always-appended local agent would answer for biography, URL
and images and make ErrRetryLater unreachable.
Wire the consequence in core/external: a throttled round no longer stamps
ExternalInfoUpdatedAt (artist and album), so the empty result is not cached for
the TTL, and TopSongs maps ErrRetryLater to the same empty-200 the not-found
path already produced instead of a new client-facing error.
Move the Last.fm code-29 mapping into the client's central error construction so
every metadata path produces RetryLaterError, and map ListenBrainz's body-level
code 429 (sent with a non-429 HTTP status) the same way.
Clamp server- and plugin-requested delays in seconds before scaling to a
Duration, in all three parse sites: a header of 18446744074 wrapped past 2^64 and
came out as a 0.29s delay.
Also: extract the artwork worker's reschedule computation into retryDelay() and
cover both it and the trace RetryIn wiring with tests; collapse the double regex
call in mapScrobblerError; drop capabilities.ScrobblerErrorRetryLaterIn (ndpgen
never emits funcs, so plugin authors could not reach it); regenerate the PDKs so
MetadataAgentError reaches the Go and Rust SDKs; de-flake the cooldown tests
(long RetryIn for the skip case, separate expiry spec); and cover the max()
retry-delay aggregation across users in the scrobble buffer.
* refactor: dedupe retry-later parsing and simplify error collection
- Add agents.NewRetryLater and agents.RetryLaterFromSeconds, with a single
1h cap, replacing the parse+clamp+multiply logic and the maxRetryInSeconds
constant duplicated across listenbrainz, plugins and the agent adapter.
- Move HTTP header parsing to httpclient.RetryAfter, so the transport layer
owns it and stays domain-agnostic; drop retryInFromHeaders from the
ListenBrainz client. Covered by a new Ginkgo table in that package.
- Collapse the two near-identical plugin retry_later regexes into one
parseRetryLater(prefix, msg) shared by the agent and scrobbler adapters.
- Fold the duplicated noteRetryIn snippet from fetchArtistImage and
fetchAlbumImage into recordAgent, which already branched on the same
isTransientExternal condition.
- Replace the atomic.Bool + note() closure in populateArtistInfo with
errgroup's own error collection; the group carries no context, so a
returned error does not cancel its siblings.
- Reuse recoveringScrobbler for the per-user delay test instead of a third
double, and switch fakeScrobbler's mutex-guarded error to the
atomic.Pointer idiom already used in the same package.
* refactor(listenbrainz): keep rate-limit header parsing in the adapter
The X-RateLimit-Reset-In header is ListenBrainz's own convention, not a
shared one: Last.fm sends no rate-limit headers at all and reports its
limit as a body code, and no other integration in tree sends Retry-After.
A parser in utils/httpclient implied a uniformity across services that
does not exist, so it moves back next to the only client that can know
which header its service sends.
* refactor(agents): collapse the retry-later sentinel and error into one type
ErrRetryLater is now the zero-delay RetryLaterError rather than a separate
errors.New value, so errors.Is and errors.AsType both match the sentinel and
every delay-carrying variant. That removes the trap where a bare sentinel
silently skipped the AsType path, and lets every consumer read the delay off
the error directly: the RetryIn accessor and the two constructors are gone,
with the policy cap applied where untrusted input is parsed.
* refactor(agents): split the cooldown store from the per-dispatch tally
The cooldown map and mutex become a cooldowns value with active/park, holding
no knowledge of errors; agentAttempts records one dispatch's outcomes and owns
the classification that noteAgentError used to hide behind a bool. The three
dispatch loops now touch a single object: skip folds the cooldown check and the
throttled flag into one call, so the store never appears in the loops.
* refactor(agents): share one dispatch loop between the agent call helpers
callAgentMethod and callAgentSliceMethod ran identical loops, differing only in
how they test a result for emptiness: a slice cannot be compared against its
zero value, so the two could not share a constraint. Both now delegate to
callAgent, which takes that test as a parameter. Keeping the loop in one place
matters more than the lines saved: it holds the cooldown skip, the attempt
recording and the empty-dispatch verdict, and a fix applied to one copy but not
the other would be silent.
* test: cover the two retry-later paths a mutation could break silently
Both gaps were proven, not guessed: making the artwork worker pass 0 instead
of the collected hint left all 386 specs green, and replacing the default
agent cooldown with 0 left the agents suite green. The worker test drives a
throttled image agent through drain and asserts the persisted retry_at, and
the cooldown test parks an agent that asked to be retried without naming a
delay, which is what Last.fm does on every rate limit.
* refactor(artwork): carry the external failure as an error, not a flag plus a trace field
The retry delay was riding on ChainTrace, a diagnostic that gets persisted, while
the very same signal — an external source faulted — already travelled by value as
resolution.extError. That was two mechanisms for one idea, and it put control-flow
state inside a serializable trace.
resolution.extError and chainState.extErr become the error itself, so a caller
checks err != nil for the fault and errors.AsType for the delay the provider asked
for. The agent loops return that error last, per convention, and longerRetry keeps
whichever failure wants the longer wait. ChainTrace goes back to holding only steps
and no longer imports core/agents.
* fix(artwork): check the resolve error before reading its resolution
Reading res.extError before the err check was safe only because every error path
in resolve returns a bare resolution{}; a future path returning a partly-filled
one would have been read silently. The failure path now returns no delay
explicitly.
* test(artwork): assert the delay acquire reports, not just its downstream effect
acquire's retry delay was only covered through the worker's persisted retry_at,
one layer away from where the value is computed. Both outcomes are now pinned at
the processor: a plain failure asks for nothing, a throttled provider's delay is
passed through.
* refactor: share the retry-seconds parse and drop the backoff deadline arithmetic
The clamp-before-scaling invariant lived in two parsers and was independently
re-tested in three files with the same magic number; a fix applied to one copy
would have left the others wrapping a huge value down to a fraction of a second.
It moves to agents.ParseRetryIn.
The buffer tracked an absolute retryDeadline only to re-arm a timer that was
already armed for the same instant; a backingOff flag says the same thing without
the arithmetic. The plugin token regex now carries its capability in the pattern
instead of capturing and comparing, so another capability's token in the same
message cannot mask it. resolution.extError becomes extErr, matching its
chainState counterpart.
* fix(agents): keep the longer cooldown when parks overlap
Calls to one agent overlap, so a short cooldown could land after a long one
started and cut it short. park now keeps whichever deadline is later, matching
the rule longerRetry already applies on the artwork side. No in-tree provider
can currently produce two different delays for the same agent, so this is
hardening rather than a fix for observed behaviour.
* fix(agents): parse the retry delay at a fixed width
strconv.Atoi parses into the native int, so on the 32-bit targets we ship
(linux/386, windows/386, three ARM variants) a delay above MaxInt32 seconds
overflowed and became unspecified instead of being capped. No provider sends a
68-year delay, so this is not user-visible, but the overflow tests asserted the
cap and would have failed on those architectures, where tests never run.
* fix(plugins): anchor the retry_later regex to a word boundary
Prevents a superstring like useragent(retry_later) from matching the
agent capability token.
stream v1.5.0 added CancelWithErr, which delivers a cancellation cause to
blocked reads, future reads, and NextReader. The fscache fork now delegates
CloseWithError to it, dropping its own cause recording and reader wrappers.
Behavior is unchanged on the Navidrome side.
* fix(scanner): read file birth time via statx on Linux
On Linux the file birth time is only reachable through statx(2). We were
reading it with times.Get(), which looks only at the plain stat() result,
where the field does not exist: djherbis/times declares HasBirthTime=false
for Linux, so the check was always false and every file fell back to
time.Now(). This has been the case since #2553 introduced the feature, which
means that PR was a no-op on Linux from day one. macOS and Windows were
never affected, as there the birth time does come back from plain stat.
BirthTime() now tries times.Get() first, which costs no syscall and is
already correct on macOS, Windows and BSD, and only falls back to
times.Stat() on the path when that comes back empty. Ordering matters: on
Windows times.Stat() opens the file asking for FILE_WRITE_ATTRIBUTES, which
fails on a read-only share before falling back.
Not every filesystem stores a birth time. Measured with a probe over real
mounts: ext4, SMB/CIFS and mergerfs report one, while NFS and rclone/FUSE
never do. Asking those on every file is pure overhead, so a miss is
remembered per device on the localFS and skipped from then on. The memo is
keyed by device rather than by library, so a library spanning two mounts
does not lose birth times on the mount that does support them.
Cost of the extra call is ~2us per file against ~52us just to open a file
for tag reading, so 0.23s across a 97k-file library, and only for files
whose tags are actually read.
Existing rows keep their current birth_time: the repository drops that
column on update, so only newly added files get the real value.
* fix(scanner): return the device id opaquely to satisfy unconvert
st.Dev is uint64 on Linux and int32 on darwin, so a uint64() cast is
redundant on one and required on the other. Returning it as an opaque value
drops the cast entirely, which also removes the gosec suppression that came
with it. The value is only ever used as a sync.Map key.
The button only rendered when an agent supplied a real last.fm URL, either
embedded in the biography or as artistInfo.lastFmUrl. Neither source is
reliable anymore: cleanContent strips the "Read more on Last.fm" anchor out of
the biography, and the Last.fm agent does not register at all unless
LastFM.ApiKey and LastFM.Secret are set, in which case GetArtistURL falls
through to ListenBrainz, which returns the artist's official homepage. The
isLastFmURL guard then correctly rejects it and the button disappears.
Build the URL from the artist name when no canonical one is available, the same
way AlbumExternalLinks already does for albums. A real last.fm URL is still
preferred when one is present, and the button stays hidden when Last.fm is
disabled or the artist has no name.
* refactor(artwork): move artworkItemName into core/artwork as ItemName
* feat(external): add RefreshInfo to force an external info refresh
RefreshInfo re-fetches and re-saves external info for one artist or
album, bypassing the TTL check that UpdateArtistInfo/UpdateAlbumInfo
use. It is synchronous; callers that must not block detach it themselves.
Also makes MockArtistRepo/MockAlbumRepo.UpdateExternalInfo persist to
Data (previously a no-op) and adds the new method to the e2e noopProvider,
both required so the interface addition compiles and is observable in tests.
* feat(external): broadcast RefreshResource after external info is saved
populateArtistInfo and populateAlbumInfo now emit the same RefreshResource
event the artwork worker uses, so the UI learns about both foreground and
background metadata refreshes.
* feat(nativeapi): replace artwork refresh endpoint with metadata refresh
* feat(ui): add refreshMetadata to the data provider
* feat(ui): add a Refresh Metadata item to the album and artist context menus
* fix(ui): re-fetch artist info when the record is refreshed
* test: fix mislabeled spec, add kind-gate negative case, guard nil mock maps
- Rename the RefreshInfo spec that claimed to cover the save-failure/broadcast
path: SetError(true) fails Get too, so it only proves RefreshInfo bails out
early at getArtist.
- Add a spec proving playlist refreshes skip the external-info step, since
that asymmetry (al/ar only) was documented but unasserted.
- Add lazy nil-map init to MockAlbumRepo/MockArtistRepo.UpdateExternalInfo so
a composite-literal-constructed mock doesn't panic on first save.
* test: relocate discArtworkName specs from cmd to core/artwork
artworkItemName moved into core/artwork as ItemName in an earlier commit, but
its disc-name specs stayed behind in cmd/artwork_test.go, reaching across
packages. Move them to core/artwork/item_name_test.go where the code now lives.
* fix(ui): shape refreshMetadata like a react-admin response
react-admin validates custom dataProvider methods and rejects any response
without a `data` key, so the raw httpClient promise made every click surface
an error toast instead of the success message. The unit test mocked
useDataProvider, which skips that validation.
Also folds "which kinds have external info" into external.HasInfo so the
handler stops restating it, drops the nil-broker guard that only existed for
tests, and delegates the mocks' UpdateExternalInfo to Put.
* refactor(external): unexport infoKinds
Only HasInfo is used outside the package, so the slice itself does not need
to be exported.
* refactor(artwork): fold ItemName into housekeeping.go next to Refresh
ItemName exists to guard Refresh from ids that would orphan a queue row, and
both callers invoke them back to back. A separate file hid that pairing; it was
only split out to keep the move out of cmd/ legible in review.
* fix(nativeapi): return 500 when the refresh lookup fails for a non-ErrNotFound reason
A transient repository error told the admin the id did not exist, and the error
was dropped without a log line, so nothing pointed at the real cause.
Also drops the inherited claim that clearing artwork state shows a placeholder.
Reads fall back to local resolution, so that only holds when there is no local art.
* fix(ui): move Refresh Metadata above Get Info in the context menu
Menu order follows key insertion order in the options object, so the new spec
pins the position rather than leaving it to be shuffled by the next addition.
* fix(stream): abort the response when a transcoded stream is truncated
When a transcode failed after some audio had already been sent, Serve logged
the error and returned nil, so Go finished the chunked body normally and the
client received an apparently complete, silently short file. Symfonium users
hit this on large offline syncs, and the worst path, ffmpeg dying mid-write
behind the transcoding cache, produced no error and nothing in the log above
Debug: the cache writer was closed plainly, so readers drained the truncated
entry to a clean EOF.
The root cause of that silence is an fscache limitation: Close is the only way
to end a cache write, and Close always means "complete". This adopts the
deluan/fscache fork, which adds CloseWithError: on failure copyAndClose now
cancels the entry with the cause, so every attached reader fails mid-read with
the real error instead of EOF, a late Get for the entry is refused, and the
entry never reports a final size. The error travels inside the entry each
reader holds, which makes per-generation delivery automatic and needs no
bookkeeping on our side.
With the failure arriving in-band, one change in Serve covers every mode: an
io.Copy error after bytes are on the wire panics with http.ErrAbortHandler.
Go aborts the response without the terminating chunk (RST_STREAM on HTTP/2),
chi's Recoverer re-panics that value, and the deferred stream.Close() still
runs, so the transcode limiter slot is released as before.
Two behaviors improve as side effects. A transcoder that dies before its first
byte now yields a Subsonic error response instead of a 200 with an empty body,
since the failure reaches Serve as an error while the status is still
unsent; genuinely empty output (clean EOF, exit 0) keeps the 200. And a failed
entry's invalidation no longer defers its unlink past a replacement entry
re-creating the same file, because canceling already closed its readers.
* fix(cache): warn when the cache writer cannot report failures to readers
The CloseWithError capability comes from the fscache fork via a go.mod
replace directive, and a type assertion picks it up. If that directive is
ever lost, the assertion fails silently, readers of a dead writer go back to
draining a truncated entry to a clean EOF, and nothing says so.
Two layers against that: a warning on the failure path when the writer lacks
the capability, and a test that asserts the writer fscache returns carries
it, so losing the fork fails CI instead of a listener's download.
* build: point the fscache replace at the fork's master
deluan/fscache#1 is merged; pin the merge commit instead of the review
branch. Pinned by sha because the module proxy still resolves the fork's
master ref to its pre-merge commit.
* build: reference the upstream fscache PR in the replace comment
The replace itself must keep pointing at the fork: the commit only exists in
djherbis/fscache under refs/pull/22/head, which the Go module fetcher cannot
resolve (verified: unknown revision for both short and full sha). The same
commit is advertised on the fork's master, so that is the fetchable source.
Album-level tags were ordered by frequency and then alphabetically by value.
Album.Genre is just the first genre in that list, so any album whose genres tie
on frequency, which is the normal case, displayed the alphabetically first genre
rather than the first one in the file. A file tagged
"Native American New Age; Indigenous American Traditional Music; Ambient"
showed up as "Ambient".
Break frequency ties on order of appearance instead. This affects all
album-level tags, so mood tagged "Happy; Chill" now keeps that order too.
MediaFiles.ToAlbum already sorts the files by path before flattening their tags,
so the aggregated order stays deterministic across scans.
Only album genre was affected; media_file tags already preserved file order.
* There has been a report about navidrome hitting listenbrainz hard
and the listenbrainz guys wanting to be able to distinguish navidrome
* feat: apply Navidrome User-Agent to all outgoing HTTP requests
Add utils/httpclient, a shared http.Client factory whose transport sets
the User-Agent header (Navidrome/{version} - https://github.com/navidrome)
on any request that does not already have one, and use it at every place
the server builds an HTTP client: Last.fm, ListenBrainz and Deezer agents
and auth routers, insights collector, backgrounds handler, and the plugin
host HTTP service. Plugin-set User-Agent values are preserved. The
per-request header lines from the previous commit are superseded by the
transport.
---------
Co-authored-by: Deluan <deluan@navidrome.org>
The trace-level request log dumps all headers as a JSON blob, but the
redaction hook only had query-param patterns, so Authorization, X-Emby-Token,
X-MediaBrowser-Token and X-Nd-Authorization leaked their tokens in plaintext.
Add one pattern that blanks those header value arrays at the log sink.
* feat: add optional natural sort order for names and titles
Album, artist, song and playlist lists sort with a plain text comparison, so
names containing numbers come out as "Foo 1, Foo 10, Foo 2" instead of
"Foo 1, Foo 2, Foo 10" (issue #4554).
Adds an EnableNaturalSorting option, default off, that switches those sorts to
a NATSORT collation registered on every connection and backed by
natural.CompareFold. natural.Compare gained an ASCII case-folding variant
because it replaces 'collate nocase': sort_* columns hold raw tag values, so
without folding they would order uppercase before lowercase.
Applying the collation only inside mapSortOrder would have missed the default
configuration entirely, since that mapper runs only when PreferSortTags is on.
setSortMappings now also rewrites the order_* columns when natural sorting is
enabled on its own. Sorts over plain text columns that are not order_* columns
(playlist.name, album.name, media_file.title, playlist_tracks title) are
wrapped explicitly, and qualified with their table because 'user' is joined and
also has a 'name' column.
The option defaults to off because the collation cannot use the existing
indexes: measured on a synthetic 110k album library, the first page of an
album-by-name listing goes from 0.03ms to 14ms. Indexing the expression was
rejected outright - an index declared with a custom collation makes the whole
database unreadable to any tool that does not register it, including the
sqlite3 CLI, which fails even on 'select count(*)' and 'pragma integrity_check'.
* refactor: fold the two sort-order mappers into one
mapSortOrder and mapNaturalOrder shared the same regex and loop, differing only
in the expression they substituted, and setSortMappings picked between them with
a two-case switch. mapSortOrder now selects the column shape itself and defers to
collatedSort for the collation, so the 'collate' clause is emitted in one place
and the caller only has to decide whether any mapping is needed at all.
The mapper tests were three near-identical cases that each hard-coded one flag
combination; they are now a DescribeTable covering all four combinations of
PreferSortTags and EnableNaturalSorting, which the previous set did not. The
album sorting specs collapse the same way. Behavior is unchanged.
* fix: leave plain sort columns alone when natural sorting is off
collatedSort wrapped its column unconditionally, so the tiebreakers added for
plain text columns picked up 'collate nocase' even with EnableNaturalSorting
off. media_file.title, the playlist_tracks alias of it, and user.user_name are
all declared without a collation, so a default install would have silently
switched those tiebreaks from binary to case-insensitive ordering. Only
playlist.name was already NOCASE and genuinely unaffected.
The helper is now naturalSort and returns the column untouched unless the option
is on, so the default path keeps the collation each column was declared with.
sortCollation had a single remaining caller and folded into mapSortOrder.
Tests: the CompareFold table body was a verbatim copy of the Compare one, so
both now go through one expectOrder helper, and the album sorting specs inline
two single-use closures.
* fix(natural): defer the leading-zero tie-break to keep ordering transitive
Compare applied the padding difference between numerically equal digit runs only
when one side ended at the digit boundary, and ignored it mid-string. That made
the relation intransitive: CompareFold("1","1a") < 0 and CompareFold("1a","01a")
== 0, yet CompareFold("1","01a") > 0.
SQLite requires a collating function to be transitive and leaves ORDER BY
undefined otherwise, so registering this as NATSORT was not safe. Reproduced with
the real driver on three artist names that occur in practice - "3", "3 doors
down" and "03 greedo" - where paging one row at a time returned "03 greedo"
twice and dropped "3" entirely.
The padding difference is now carried as a tie-break that is applied only when
the strings are otherwise equal, which restores transitivity while keeping the
documented intent (a01 < a1, a0 < a00). Three existing entries changed: each
asserted that two distinct strings compare equal, which was the same defect seen
from the other side.
Found by the Codex review on #6015.
Plugins load concurrently through an errgroup. loadPluginWithConfig wrote
m.plugins under m.mu but read it back unlocked to pass to callPluginInit,
so one goroutine's write raced another's read. Caught by -race on master
(run 32608293134): all 640 specs passed, the job failed only on the race.
Capture the pointer while holding the lock and use the local. Holding m.mu
across callPluginInit would be wrong, since that runs arbitrary plugin code.
* feat(auth): add per-user token_epoch column and bump method
* feat(auth): add aud and ep claims, omitted when zero
* feat(auth): add CreateAPIToken for non-expiring, audience-scoped tokens
* feat(auth): add CheckClaims for epoch and audience validation
* feat(jellyfin): issue non-expiring, jellyfin-scoped access tokens
* fix(subsonic): reject API-scoped and revoked tokens on the jwt path
* fix(server): reject API-scoped and revoked tokens on the native API
* fix(server): pin the token-subject guard and stop leaking test config
Adds a regression spec for the DevAutoLogin/ExtAuth guard in
tokenAllowed, switches its comparison to case-insensitive to match
the user lookup's own COLLATE NOCASE semantics, and restores Subsonic
JWT test config after each spec instead of leaking SessionTimeout.
* feat(request): add a token epoch holder for handler-to-middleware signalling
* refactor(server): write the refreshed JWT header after the handler runs
* feat(auth): revoke all tokens for a user when their password changes
* fix(server): restore Unwrap on the JWT refresh writer so SSE write deadlines apply
* test(auth): pin that non-session tokens reject API access tokens
* test(jellyfin): pin token scoping and epoch revocation end to end
Exercises auth.CreateAPIToken and CheckClaims against the real Jellyfin
router and SQLite DB: the minted token has no exp and is aud-scoped to
jellyfin, and bumping token_epoch through the real UserRepository revokes
an already-issued token on the next protected request.
* test(nativeapi): pin the token-epoch handoff through a real password-change request
Drive a self password change through the real Authenticator/JWTRefresher
chain and a real SQLite-backed userRepository, so the epoch handoff between
Put and the refreshed-token writer is verified end to end, not as two
separately-tested halves. Also fix tokenAllowed to read the enriched ctx it
was given instead of r.Context(), so its warning log carries the username.
* refactor(server): drop tokenAllowed's now-unused request parameter
Finding-2 already moved every use to ctx; r was dead weight. Also note
in the new nativeapi test why it must stay the package's only real-DB
spec: db.Db() is a process-wide singleton its cleanup closes for good.
* refactor(auth): remove duplication in claim decoding and token minting
* refactor(auth): group aud with the standard JWT claims
* refactor(auth): read aud with the standard-claim accessor pattern
* fix(log): redact every api_key spelling the Jellyfin API accepts
* fix(auth): bind session tokens to the user id, not just the username
* fix(auth): return the token epoch from the same atomic increment
* fix(auth): bump the token epoch in the same statement as the password write
* chore(auth): trim comments to the why-only budget
* feat(artwork): report what a config-fingerprint backfill enqueued
A backfill re-resolves every entity, and on a large library that is tens of thousands of
external agent calls. It announced itself with a single line carrying nothing but an elapsed
time, so the size of the job was invisible until the request volume showed up hours later.
Log the item count, the per-kind breakdown, and a ceiling on the external lookups the queued
work can cost. The ceiling reuses ExternalLookupsPerItem, the same estimator behind the
`artwork reprocess` preview, so the two agree on what an item can cost.
backfill now returns a summary instead of a bare bool, which keeps the counts assertable
without capturing log output. Worker.Backfill keeps its (bool, error) signature, so its caller
is unchanged, and it reads the agent count off its own resolver.
* refactor(artwork): share the image-agent count and take it lazily
Counting image agents was written twice, once in the CLI for the `artwork reprocess` preview and
again as a resolver method for the backfill log. Two copies of "which agents count as image
agents" can drift, and the CLI estimate and the server log would then disagree silently.
Move the derivation next to the type it builds, as NewImageAgentCount, and call it from both.
The resolver method goes away with it: hanging the census on the resolver forced two nil guards
that its only caller could never trigger, because Worker always builds a resolver with agents.
The Worker keeps the *agents.Agents it is already handed instead of reaching through the
processor and resolver to find it.
Pass the count as a func. Building the agent list constructs every enabled agent (each one an
HTTP client and a cache goroutine) only to take its length, and a backfill returns early on an
unchanged fingerprint, which is what happens on nearly every restart.
* docs(artwork): say what the backfill lookup estimate does not bound
The comment called the number a ceiling, which the CLI comment on the same estimate already
contradicts: externalEstimate "claims no bound". Both are right about the local-source case and
only one of them mentions that a retried item asks its agents again.
* feat(artwork): drip the stale-absent recheck instead of bursting it daily
Each hourly housekeeping tick now re-queues at most 100 absent states
per kind, oldest attempts first, instead of everything older than 24h
at once. External agents see a flat ~100 requests/hour per agent
instead of hourly bursts of ~2,000, and the effective recheck interval
self-scales with the size of the absent pool (~4 days at 10k absent
artists) while small libraries keep the 24h floor.
* feat(artwork): trust an absent artwork state for a week before rechecking
With the recheck now dripped at 100 items per kind per hour, the 24h
floor only governed small libraries, where the drip cap never binds;
they still re-asked every agent daily. A 7-day floor cuts that cost 7x
and, for large libraries, becomes the binding limit over the drip
cycle (~5.7k calls/day instead of ~9.6k at 10k absent artists).
Among comparable servers, this is still the second-most-eager recheck:
gonic retries misses every 30 days, Jellyfin and Funkwhale never do.
* refactor(artwork): state the drip's backpressure contract where it bites
Review follow-ups: the recheck limit deliberately caps the *selection*,
not the insertions — already-queued rows use up budget, so a stalled
drain admits no new work instead of building a recovery burst. Say so
in the interface doc, mirror it in the mock by truncating the sorted
candidates (matching the SQL's LIMIT-before-ON CONFLICT), and teach
`artwork status` and the worker doc the post-drip wording. Also pin
the one cmd fixture that still assumed a 24h recheck window.
* 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.
* 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.
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.
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.
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.
* 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
* 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.
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.
* 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>
* 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>
* 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.
* 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.
* 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>
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.
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.
* 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>
* 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>
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.
* 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.
* 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.
* 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.
* feat(artwork): add a resolution chain trace collector
* feat(artwork): trace the local priority chain
* fix(artwork): record priority candidates the chain never evaluated
* refactor(artwork): report never-evaluated candidates as skipped
* feat(artwork): trace external agents at the gate seam
* feat(artwork): add repository queries to enqueue by current source
* feat(artwork): expose a tracing resolver for the CLI
* feat(artwork): read a single queue row by item
The explain CLI must report whether an item is queued, at what priority and when it
retries; the queue repository could only be drained in eligibility batches, which
cannot see a row that is still backing off.
* feat(cli): add artwork explain
Prints why an item has the artwork it has: the stored state, its queue row, the
governing config, the resolver's priority-chain walk and the verdict. Offline by
default so a diagnostic run cannot add load to an external provider; --live asks
the agents for real. Playlists and radios do not walk a priority chain, so they
report that instead of an empty chain table.
* fix(artwork): trace an external tier that never reaches an agent
A configured 'external' token vanished from the chain when no enabled agent provided
images for that entity type, and for synthetic artists, leaving the trace unable to
say whether the tier was even considered.
* fix(cli): never state an artwork outcome the walk did not observe
A transient external failure traced as 'error' fell through to 'not resolved', which
is the most common state behind a missing-artwork report. It is now indeterminate, and
an offline win that a skipped higher-priority external candidate could have taken says
so instead of naming a winner the live chain might not pick.
* feat(cli): add artwork refresh
* feat(cli): add artwork reprocess
Bulk re-enqueues artwork by kind and/or by the source an item currently
resolves from, previewing the matched count and confirming before queueing.
The preview counts with CountBySource (rows matched) and reports separately
what EnqueueBySource inserted: its DO NOTHING conflict policy leaves an
already-queued row untouched, so the two numbers differ and the output must
not claim the skipped rows were re-queued.
An unknown --source is rejected against the sources present in item_artwork,
rather than silently matching nothing and printing a reassuring 0.
* fix(cli): cover the reprocess selection rule and validate sources table-wide
The reconciliation that makes --source alone target every kind was only
exercised through runReprocess, which no test calls: mutating it to
`all := reprocessAll` left the suite green. It is now reprocessSelectsAll,
covered for all three selectors.
Scoping source validation to the selected kinds made the same well-formed
filter valid or invalid depending on which other kinds were selected, and its
error read the same for a typo as for a source that simply does not apply to
the chosen kind. Validation is now table-wide: a typo still aborts, while a
valid-but-inapplicable source falls through to "Nothing matches".
Also: the prompt now counts only the kinds that reach an external agent as
external cost, and --dry-run on an empty selection reports a dry run.
* fix(cli): cover the reprocess --yes guard and preview the external cost
Mutating the --yes check to `if true` left the suite green, so the one bypass
of the confirmation was unverified. The choice is now reprocessConfirm(yes, in),
covered in both directions.
The external estimate only reached the operator through the prompt, which
--dry-run skips — hiding the number in the one mode that exists to show it
before committing. The preview now carries it, and the prompt drops the clause
when no lookup will be made.
An empty selection says so again under --dry-run.
* feat(artwork): add read-only queue and absent counters
Both are needed by the artwork status CLI: a queue breakdown by kind and priority, and
the absent totals split against the recheck cutoff.
* feat(cli): add artwork status
Reports the queue, where artwork currently resolves from, absent counts against the 24h
recheck window, and the stored config fingerprint versus the current one — the line that
turns 'why is my server re-resolving everything?' into one command.
fingerprint() and staleAbsentAge are exported so the CLI reports the values backfill
itself compares, instead of a second copy of the formula that can silently drift.
* fix(cli): lead the artwork status backfill line with the queued backlog
By the time anyone runs a diagnostic, backfill has usually already stored the new
fingerprint, so 'up to date' was printed while thousands of items churned through external
providers. The backlog is the finding; the fingerprint is context.
Also echoes the config inputs the fingerprint covers, so a change can be traced to the
setting that caused it, and pins the rendered rows: the Absent values, the queue TOTAL and
a queue-scoped kind/priority pair were all unasserted, so kindName and priorityName were
effectively untested. FingerprintInputs is now the single listing ConfigFingerprint hashes;
a pinned hash proves the value did not change.
* refactor(artwork): export the trace outcome vocabulary
The CLI hardcoded the outcome literals and the "external:" prefix, so renaming a
constant's value in core/artwork left cmd compiling and the suite green while
`artwork explain` silently degraded its verdict.
Renaming a value now fails the golden vocabulary test in core/artwork and the
explainResult tests in cmd.
* fix(cli): keep the re-enqueue warning when a backfill is already running
A stale stored fingerprint with items already queued is the worst state the
system can be in: a second full re-enqueue is pending on top of the one running.
The line carried the weakest wording of the three, and was untested.
* refactor(artwork): drop the unreachable breaker branch from the tracing gate
--live wires the tracing gate straight to passthroughGate, so errBreakerOpen can
never reach it; the test only passed by injecting a fake gate.
* refactor(artwork): delete the never-emitted not-reached outcome
Candidates after the winner are lower priority and say nothing about why a source
won; the ones that matter sit above it and are already recorded.
* refactor(artwork): make the trace nil-safe in one place only
add already handles a nil trace, so record's own guard was dead; Steps was the
odd one out and would panic where every other method tolerates nil.
* refactor(artwork): export the trace types directly
ChainTrace and TraceStep were unexported types re-exported through aliases,
which existed only so the CLI had a name to refer to them by. The types are
public API — Resolver.Steps returns []TraceStep and the CLI constructs a
ChainTrace — so name them that way and drop the indirection.
Encapsulation is unchanged: add, mu and steps stay unexported, so only this
package can write a step.
* refactor(cli): simplify parseArtworkKind with slices.Contains
Replaces a nested loop and a manual append with slices.Contains and the
repo's slice.Map helper. Same behaviour, same error message.
* fix(cli): print the absent artwork source under the name --source accepts
`artwork explain` rendered the stored empty source as "(absent)", while
`artwork reprocess --source` only accepts "absent", so pasting what explain
printed straight back into reprocess was rejected as an unknown source.
* refactor(artwork): own the kind list and the chain predicate in the package
Export RecheckKinds and add WalksPriorityChain so the CLI stops keeping its
own copies of both, and unexport externalCandidate, which nothing outside the
package consumes.
* refactor(cli): drop the artwork command's duplicated state and formatting
Reuse artwork.RecheckKinds and artwork.WalksPriorityChain, extract
newTabWriter and externalEstimate, fold reprocessSelectsAll into
selectedKinds, and derive the queue total and the walks-chain flag instead of
carrying them in the report structs.
* test(persistence): drop two artwork-queue specs that cannot fail
One seeded hash and source together and then asserted the two counts agree,
so its setup guaranteed the result; the other repeated the count-does-not-
enqueue property already covered by the CountBySource spec.
* refactor(artwork): rename Resolver to TracingResolver for clarity
* fix(cli): count playlists in the artwork reprocess external estimate
The estimate used WalksPriorityChain, which is true only for artist and album,
so a playlist-only reprocess reported "External lookups: none" and the
confirmation prompt dropped the external-cost warning. Playlists do reach the
network: through the m3u ExternalImageURL fetch when EnableM3UExternalAlbumArt
is on, and — verified by test — through the generated grid, whose tiles resolve
album art via the full album priority chain.
Adds artwork.MayFetchExternal, a config-aware predicate for "can this kind's
resolver reach the network", and uses it for the estimate. WalksPriorityChain
keeps its separate job of deciding whether explain prints a chain block.
* fix(cli): estimate artwork reprocess external lookups per agent, not per item
The reprocess prompt billed one external lookup per externally-capable item.
fetchArtistImage/fetchAlbumImage try every enabled image agent and stop early
only on a hit, and resolvePlaylist can fetch the m3u image and then resolve up
to four sampled albums for the grid, each walking the album agents again. The
number the operator confirmed could understate real provider traffic several
fold, in the prompt whose whole job is to stop a provider flood.
ExternalLookupsPerItem now multiplies by the visible image-agent count and adds
the playlist grid factor. It stays a floor: the CLI never calls Manager.Start(),
so the plugin registry is empty and plugin-provided agents are dropped by
getEnabledAgentNames. On an install with 5 agents of which 3 are plugins the
count is well under the truth, so the wording is now "at least N" rather than
"up to N" — a zero visible count still bills one lookup for the same reason.
Fixing the plugin visibility is out of scope: Manager.Start() needs a Subsonic
router and writes to the DB via syncPlugins, breaking this command group's
read-only guarantee.
* fix(cli): state the artwork reprocess estimate as an estimate, not a bound
Neither bound is true. A ceiling is false because plugin agents are invisible to
a CLI that never starts the plugin manager, and a floor is false because a local
hit ends the walk before any agent is asked and a hit on the first agent skips
the rest. "at least N" traded one wrong claim for another.
The line now names its blind spots instead:
External lookups: ~340 estimated (plugin agents not counted; local hits may
need fewer).
The same line is reused in the confirmation prompt, and the zero case still
reads "External lookups: none." with the prompt dropping the clause entirely.
The count itself is unchanged.
* fix(cli): account for every configured agent in artwork explain
The Agents: line printed the raw config while the Chain only showed the agents the CLI could
construct, with nothing explaining the gap: plugin agents are never registered in a CLI that does
not start the plugin manager, and a built-in without credentials returns nil. Three of five agents
could vanish, including ones ranked above the one shown.
Also treat a live external error before the winning hit like the already-handled would-try case:
the resolver serves such a hit provisionally and retries later, so the verdict is indeterminate.
The Result line is still not qualified when an unavailable agent might have won; that needs agent
ranking, and is left to the follow-up that makes the CLI load plugin agents for real.
* fix(cli): do not call an external artwork win indeterminate
explainResult qualified the verdict whenever an external OutcomeError
appeared before the winning hit. When a later external agent returns an
image, fetchArtistImage/fetchAlbumImage discard the earlier error, so
extError is false: the worker settles the item and schedules no retry.
Telling the operator it may resolve differently on a retry was wrong.
The warning is only correct when a lower-priority local source won while
an external error was recorded, which is the case that carries extError.
* fix(cli): accept --source absent when nothing is currently absent
validateSources checks the requested sources against the ones item_artwork
actually uses, to catch a typo. The reserved empty source (spelled 'absent' on
the CLI) is a valid filter even when it matches nothing, so a scheduled
'artwork reprocess --source absent --yes' stopped working the moment the
library finished resolving. Treat it as intrinsically valid and let the
existing zero-match path report it.
* feat(artwork): explain disc and media file artwork from the CLI
`artwork explain` rejected `dc` and `mf` because it validated against RecheckKinds,
the list of kinds the backfill revisits. Those are different questions: a kind with no
recheck path still has artwork someone can report as wrong.
Disc artwork now walks DiscArtPriority under a trace, so explain reports which entry won
and why the others lost, including entries that map to no source at all (external is
unsupported, a disc with no subtitle, an album folder with no images). Media file artwork
traces its single embedded candidate, separating "EnableMediaFileCoverArt is off" from
"the track has no embedded art" — stored state cannot tell those apart.
Each command now validates against the kinds it can actually serve: explain takes all six,
refresh takes artwork.RefreshableKinds (which nativeapi now shares instead of keeping its
own copy), reprocess still takes RecheckKinds. Disc artwork stays out of refresh: the
worker cannot resolve it, so the queue row would be rejected on every drain.
WalksPriorityChain becomes Explainable, and ResolveArtist/ResolveAlbum collapse into
Resolve(kind, id).
* refactor(artwork): one disc-artwork walk for serving and explain
resolveDisc duplicated the loop selectImageReader already ran: try each source in
priority order, take the first that yields an image. The serving path and the CLI
diverged on two details as a result — only selectImageReader checked ctx between
candidates and logged each attempt.
Both now call discArtworkReader.selectImage, which takes the chainState the CLI already
uses for the other kinds. The serving path passes an untraced one, whose nil trace makes
recording a no-op. selectImageReader had no other caller and is gone.
The disc tests move from fromDiscArtPriority to discCandidates, so they assert the skip
reason for an entry that maps to no source rather than that it silently vanished, and
cancellation mid-walk is now covered.
* fix(artwork): reject a nil reader in the resize cache instead of panicking
resizedItem.Reader closes what open() hands back, so an open() that reports "no image"
as (nil, nil) rather than an error takes the request down with a nil-pointer panic. Every
caller returns an error today, and no test covered it: the resolution e2e harness stubs
the resize reader out entirely, so no e2e path reaches this code at all.
Guard it and cover Reader directly.
* refactor(artwork): move the keeps-state fact into core, drop a redundant guard
keepsArtworkState lived in package cmd and re-derived by hand what RefreshableKinds
already encodes: the same five-of-six kinds. It is now artwork.KeepsState, beside the
list, with a test pinning the two together — nothing else stopped them drifting, and a
drift would have explain report stored state for a kind that keeps none.
serveDisc's closure also hand-rolled a nil-reader error that both consumers of open()
now produce themselves: serveSource for a full-size request, resizedItem.Reader for a
resized one.
* fix(artwork): route disc candidates through the shared resolvers
openCandidate ran its own source loop and threw the error away, so a disc track that
exists but cannot be parsed traced as "miss" — indistinguishable from a track with no
embedded art. fromTag and fromFFmpegTag already report that case as errSourceUnreadable;
only this loop was discarding it. Telling those two apart is what the trace is for.
Candidates now carry a resolve func instead of raw sources: embedded goes to
resolveEmbedded, and the folder-backed entries to resolveFolderSource, extracted from
resolveFolderFile so both callers classify an unopenable file the same way. openCandidate
and its absolute-path special case go away with it.
Disc's own fromExternalFile and fromDiscSubtitle still swallow open errors, so folder
candidates cannot report unreadable yet; that is a change to their error contracts.
* fix(artwork): report an unreadable local candidate as indeterminate
processor.acquire treats resolution.localError exactly as it treats extError: a fault is
not a definitive "no image", so it retries instead of settling absent. explainResult
qualified only the external case, so a chain that ended on an unreadable local candidate
printed "not resolved" — the one verdict that says the walk was conclusive.
The qualification belongs only to the unresolved branch. chainState.try stamps extErr onto
a hit and deliberately drops localErr, so an unreadable step followed by a hit is settled
as found and must not carry a warning; a test pins that.
Found by Codex on 5f65d7cfa.
* 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.
* 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.
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.
* 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.
* 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.
* 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.
* 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.
* 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.
* 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.
* 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.
* 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.
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.
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.
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.
* 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.
* 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.
* 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.
2026-08-10 11:42:27 -04:00
825 changed files with 41898 additions and 12954 deletions
"description":"Navidrome API v1. Spec-first, additive within v1. Clients discover implemented\ncapability modules through `GET /server` and never sniff versions.\n\nEnums are open: new values may be added to any enum within v1. Clients must\naccept values they do not recognise instead of failing.\n\nEvery operation declares `x-stability-level`: `alpha` operations may change or\ndisappear without notice, `beta` and `stable` operations only change additively.\nA level is only ever raised, never lowered.\n\n`HEAD` is accepted wherever `GET` is. A `405` response lists the allowed methods\nin its `Allow` header.\n",
"license":{
"name":"GPL-3.0",
"url":"https://www.gnu.org/licenses/gpl-3.0.html"
}
},
"servers":[
{
"url":"/api/v1"
}
],
"tags":[
{
"name":"server",
"description":"Server discovery and the published OpenAPI document."
}
],
"paths":{
"/server":{
"get":{
"operationId":"getServerInfo",
"x-module":"core",
"x-stability-level":"alpha",
"tags":[
"server"
],
"summary":"Describe the server",
"description":"Returns the public server description. No authentication required.\nAuthenticated requests will additionally receive the implemented capability modules\nonce authentication is available.\n",
"responses":{
"200":{
"description":"Server description.",
"content":{
"application/json":{
"schema":{
"$ref":"#/components/schemas/ServerInfo"
}
}
}
},
"500":{
"$ref":"#/components/responses/InternalError"
}
}
}
},
"/openapi.json":{
"get":{
"operationId":"getOpenAPISpecJSON",
"x-module":"core",
"x-stability-level":"alpha",
"tags":[
"server"
],
"summary":"Get the OpenAPI document (JSON)",
"description":"The bundled OpenAPI document of the running server version. Supports ETag revalidation.",
"responses":{
"200":{
"description":"The OpenAPI document.",
"headers":{
"ETag":{
"$ref":"#/components/headers/ETag"
}
},
"content":{
"application/json":{
"schema":{
"type":"object",
"description":"OpenAPI 3.0 document."
}
}
}
},
"304":{
"$ref":"#/components/responses/NotModified"
}
}
}
},
"/openapi.yaml":{
"get":{
"operationId":"getOpenAPISpecYAML",
"x-module":"core",
"x-stability-level":"alpha",
"tags":[
"server"
],
"summary":"Get the OpenAPI document (YAML)",
"description":"The bundled OpenAPI document of the running server version. Supports ETag revalidation.",
"responses":{
"200":{
"description":"The OpenAPI document.",
"headers":{
"ETag":{
"$ref":"#/components/headers/ETag"
}
},
"content":{
"application/yaml":{
"schema":{
"type":"object",
"description":"OpenAPI 3.0 document."
}
}
}
},
"304":{
"$ref":"#/components/responses/NotModified"
}
}
}
}
},
"components":{
"securitySchemes":{
"bearerAuth":{
"type":"http",
"scheme":"bearer",
"bearerFormat":"JWT",
"description":"Short-lived access token minted from a device grant. Not yet applied to any operation."
}
},
"schemas":{
"ServerInfo":{
"type":"object",
"description":"Public server description. Everything an add-server screen needs before login.",
"required":[
"name",
"serverVersion",
"specVersion",
"setupRequired",
"loginMethods"
],
"properties":{
"name":{
"type":"string",
"description":"Human-readable server product name."
},
"serverVersion":{
"type":"string",
"description":"Version of the running server build."
},
"specVersion":{
"type":"string",
"description":"Version of the OpenAPI document this server implements."
},
"setupRequired":{
"type":"boolean",
"description":"True until the first admin user has been created."
},
"loginMethods":{
"type":"array",
"description":"Login methods this server accepts. New methods may be added; clients ignore values they do not recognise.",
"items":{
"type":"string",
"enum":[
"password"
]
}
}
}
},
"Problem":{
"type":"object",
"description":"RFC 9457 problem details, returned for every 4xx and 5xx response.",
"required":[
"title",
"status",
"code"
],
"properties":{
"type":{
"type":"string",
"description":"URI reference identifying the problem type. Omitted while the problem carries no semantics\nbeyond its HTTP status code, which RFC 9457 defines as `about:blank`. Problems with their\nown semantics get their own URI; switch on `code` instead.\n"
},
"title":{
"type":"string",
"description":"Short human-readable summary, the same for all occurrences of this problem type."
},
"status":{
"type":"integer",
"description":"HTTP status code of this response."
},
"detail":{
"type":"string",
"description":"Human-readable explanation specific to this occurrence. Omitted for internal errors."
},
"code":{
"type":"string",
"description":"Machine-readable error code, and the value clients switch on. New codes may be added.",
"enum":[
"validation",
"unauthorized",
"forbidden",
"not_found",
"method_not_allowed",
"unavailable",
"internal"
]
},
"errors":{
"type":"array",
"description":"Per-field failures. Present only when `code` is `validation`.",
pruneCmd.Flags().BoolVarP(&force,"force","f",false,"bypass warning when backup count is zero")
backupRoot.AddCommand(pruneCmd)
restoreCommand.Flags().StringVarP(&restorePath,"backup-file","b","","path of backup database to restore")
restoreCommand.Flags().StringVarP(&restorePath,"backup-file","b","","file name of the backup database to restore (resolved against the backup directory unless it is an absolute path)")
