Move the end-to-end call/setup/mint helpers to a suite-level test client
and the apiauth login/mustMint helpers to package level. Replace the
hardcoded vacuum x-scope enum with a Go test that every known scope is
a valid spec Scope; the gate already rejects unknown x-scope values.
Load the signer lock-free once cached, read the signing key before
generating one, cap the liveness cache at its limit, share one
grantable-scope predicate, and let CreateFirstAdmin open its own locked
transaction with an optional in-transaction follow-up.
Key operations by a struct, share the validation options, derive the
route method from chi, and set every rule-derived field in buildGateOp.
Build WWW-Authenticate challenges and 413 problems in one place, fetch
the principal through one helper, and refuse gate rules that name an
operation missing from the spec.
Use gg.V, slice.Map, chi's RequestSize, core/auth's encryption key and
ClientIPRateLimiter instead of local copies; share the grant idle expiry
constant and a Grant.LastActivity helper; pass last use as a time value.
changePassword now documents that on Navidrome the change also ends the
user's sessions on its other APIs, regardless of revokeOtherGrants, which
only covers API v1 grants.
The gate passed server.ClientIP to Authenticate and ResolveGrant, which
masks IPv6 addresses to their /64 for rate limiting, so lastUsedIp stored
a prefix. A new server.ClientAddr returns the resolved address unmasked;
ClientIP builds on it and stays the rate limiter key.
A 401 raised after a token was accepted by the gate, such as a password
change whose grant was revoked mid-request, carried a bare Bearer
challenge. writeProblemStatus now answers Bearer error="invalid_token"
whenever the request presented a bearer token.
login, setupFirstAdmin and createAccessToken responses now carry
Cache-Control: no-store (RFC 6749 section 5.1), set by the gate for a
small list of operations so their error responses are covered too.
The v0/v1 setup race test also checks the v1 status is 201 or 409.
Authenticate ran token claims through Expand, so a token claiming `all`
would have gained every known scope. Only a holder of the signing key
could mint one, but tokens should carry concrete scopes only. Claims now
go through Allowed, which keeps known scopes (and admin only for admins)
and never expands `all`.
The HS256 key was 22 base62 characters, about 128 bits, below the 256 bits
RFC 7518 section 3.2 asks for. New keys are 32 bytes from crypto/rand,
hex-encoded before being encrypted and stored. Keys already stored keep
working unchanged.
Logout answered an undeclared 404 when its grant was already gone, for
example revoked by another node inside the liveness cache window or by a
concurrent logout. It now treats a missing grant as success and still
evicts the cache entry, so logout always answers 200.
Grants left on an older user epoch (after a password reset through the
existing UI, or a login that raced a password change) are dead but only
deleted when presented. Listing and counting grants now filter on the
user's current epoch, so those grants no longer show up.
dropGrant now deletes before evicting, like RevokeGrant, so a concurrent
cache fill cannot re-cache a grant that is being dropped.
Adds the seven auth operations to the spec (createAccessToken, listGrants,
revokeGrant and logout in core; login, setupFirstAdmin and changePassword in
the password module), the bearerAuth/grantAuth schemes, and vacuum rules
requiring explicit security and a known x-scope. The strict handlers sit on
core/apiauth and are covered end to end against a real SQLite database.
The first operations with parameters make the generated code import
github.com/oapi-codegen/runtime. An oapi-codegen overlay renames the shared
offset/limit parameter types, since a generated Offset clashes with Ginkgo's
dot-imported Offset in this package's tests; the published spec is unchanged.
* 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
