mirror of
https://github.com/navidrome/navidrome.git
synced 2026-11-04 01:08:54 +01:00
730 commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Deluan Quintão
|
38960501fd |
Merge a60a9e6642 into caa2f8a0c0
|
||
|
Deluan Quintão
|
758e64c999 |
feat(scanner): per-library PID configuration (#6252)
* 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 |
||
|
Deluan Quintão
|
d3a434a610 |
Merge branch 'master' into apiv1-auth | ||
|
David Davó
|
3a31f702b5 |
feat(ui): add played filter to album list (#6207) | ||
|
Deluan
|
d1b876097f |
refactor(api): use the grant secret as the API v1 bearer credential
API v1 no longer mints short-lived JWT access tokens. Clients send the grant secret from POST /auth/login or /auth/setup as `Authorization: Bearer` on every request. Every request already looked the grant up in the database, so the JWT gave no speed or revocation benefit and only added a refresh loop, which early client authors pushed back on. The grant already is an API key: one per client sign-in, scoped and revocable. Revocation is now immediate on every node; the contract promises "within one minute". Removed: POST /auth/token, the grantAuth scheme, the TokenRequest and AccessToken schemas, the token_expired problem code, the API v1 JWT signer and its signing key, the grant liveness cache, and PropertyRepository.PutIfAbsent. ResolveGrant is now Authenticate. Short-lived tokens return later only as narrow media tokens for ?access_token= on media URLs, together with the media endpoints. Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
Deluan
|
1b5c68a203 |
refactor(log): mark secrets on their own line and cover session keys
Mark secret values with log.WithSecrets on a separate line instead of nesting the call in argument lists. Also mark Last.fm/ListenBrainz session keys written through SessionKeys.Put and the PasswordEncryptionKey checksum, which still reached trace logs, and ignore values shorter than 8 characters so a short plaintext marked after a failed encryption cannot mangle SQL text or the [REDACTED] marker. |
||
|
Deluan
|
ddcc611af3 |
refactor(log): redact caller-marked secret values in the log hook
Replaces the statement-wide SQL arg redaction from the previous commit, which hid every arg of property and password writes (user names, emails, scanner properties) and made troubleshooting harder. log.WithSecrets marks values on a context, log calls now pass their context to the logrus entry, and the redaction hook replaces those values in the message and fields. logSQL logs the real args again; only the encrypted password, the API v1 key and the JWT secrets are marked. |
||
|
Deluan
|
8789561b60 |
fix(api): consolidate grant deletes and harden API v1 auth after review
- GrantRepository keeps three deletes: DeleteForUser, DeleteStaleEpochs
(replaces DeleteOtherEpochs and DeleteIfEpoch) and DeleteIdle. Delete(id)
is gone; the idle path in ResolveGrant now calls DeleteIdle, so a grant
renewed by another node between the read and the delete survives.
settleEpoch deletes the user's grants below the snapshot's epoch, which
is safe outside the transaction because epochs only move forward.
- The "dead grants on an older epoch are only deleted when presented"
policy note moves from the repository to core ListGrants.
- SQL trace logging no longer prints the args of property writes (signing
keys) or user password writes, both encrypted with a key that may be the
public default. The SQL statement is still logged.
- The spec gate rejects JSON body keys that differ from a declared property
only in case. kin-openapi validates exact names while encoding/json
decodes case-insensitively, so {"scopes":[],"Scopes":null} minted a
token with every scope and a "Client" key skipped maxLength.
It also rejects data after the first JSON value, which the handlers'
decoder ignores and which let a body skip the alias check.
- The liveness cache trims its eviction log on evict, not only on put, so
evict-only traffic stays bounded; the floor still drops stale fills.
- createAccessToken, login and setupFirstAdmin declare Cache-Control:
no-store on their success responses.
|
||
|
Deluan
|
a41e656706 |
refactor(api): reuse existing helpers in API v1 auth
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. |
||
|
Deluan
|
b1cfa932be |
fix(api): make logout idempotent and hide grants left on a stale epoch
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. |
||
|
Deluan
|
5688283d78 | feat(persistence): add PropertyRepository.PutIfAbsent | ||
|
Deluan
|
2afe2ffe0a | feat(api): add api_grant storage for API v1 grants | ||
|
DawidKrynski
|
ab5869123d |
fix(playlists): include co-credited album artists when adding an artist to a playlist - #6240 (#6241)
* 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> |
||
|
Deluan Quintão
|
4cdffd5633 |
feat(subsonic): OpenSubsonic API key authentication (#6219)
* 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> |
||
|
Deluan Quintão
|
b293b96256 |
refactor(persistence): stateless repositories with per-call context (#6149)
* 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. |
||
|
Deluan Quintão
|
336fa482f8 |
fix(plugins): skip the startup error reset when no plugin has an error (#6223)
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. |
||
|
Deluan Quintão
|
ee6dd1bc03 |
fix(scanner): stop DB lock starvation during scans on slow storage (#6201)
* 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. |
||
|
Deluan Quintão
|
9e8811e4c5 |
fix(server): enforce player ownership on create and registration (#6184)
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. |
||
|
Deluan Quintão
|
0429fb3d40 |
fix(test): stop the Windows test job from failing at random (#6182)
* 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 |
||
|
Deluan Quintão
|
8e784b6af7 |
fix: apply the per-user library filter to bookmarks, playlists and now-playing (#6179)
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. |
||
|
David Vedvick
|
49f626c00a |
feat(smartplaylists): add support for referencing playlists using paths (#5187)
* 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> |
||
|
Deluan Quintão
|
549dfa7f30 |
feat(jellyfin): advertise Jellyfin 12.1.0 and add the missing 12.x quick wins (#6163)
* 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.
|
||
|
Deluan Quintão
|
3f4b6a642c |
fix(server): return 404 instead of 500 for missing native API resources (#6131)
* 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". |
||
|
Deluan Quintão
|
1a8463f7de |
Merge commit from fork
* 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. |
||
|
Sudo-Ivan
|
4168377b65 |
Merge commit from fork | ||
|
ts
|
c6732e1fdf |
feat(cli): add missing file list and remap subcommands (#5928)
* 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> |
||
|
Deluan Quintão
|
2dc0983629 |
fix: miscellaneous fixes for shares, artwork resize, auth limits, and watcher start (#6098)
* 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. |
||
|
Deluan
|
072331078d |
fix(server): update StoreMusicFolder to skip updates when path is unchanged
Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
Deluan Quintão
|
a7365e119b |
fix(subsonic): update the playlist changed timestamp when renaming a smart playlist (#6082)
`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. |
||
|
Deluan Quintão
|
47bc3c00f3 |
fix(artwork): never retry absent artwork on its own (#6054)
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. |
||
|
Deluan Quintão
|
96b051ffa7 |
fix(nativeapi): stop partial PUTs from clearing untouched columns (#6058)
* 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. |
||
|
Deluan Quintão
|
dbd26ba2e7 |
perf(scanner): improve playlist importing on large libraries (#6055)
* 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. |
||
|
Deluan Quintão
|
3e55886195 |
feat: add optional natural sort order for names and titles (#6015)
* feat: add optional natural sort order for names and titles Album, artist, song and playlist lists sort with a plain text comparison, so names containing numbers come out as "Foo 1, Foo 10, Foo 2" instead of "Foo 1, Foo 2, Foo 10" (issue #4554). Adds an EnableNaturalSorting option, default off, that switches those sorts to a NATSORT collation registered on every connection and backed by natural.CompareFold. natural.Compare gained an ASCII case-folding variant because it replaces 'collate nocase': sort_* columns hold raw tag values, so without folding they would order uppercase before lowercase. Applying the collation only inside mapSortOrder would have missed the default configuration entirely, since that mapper runs only when PreferSortTags is on. setSortMappings now also rewrites the order_* columns when natural sorting is enabled on its own. Sorts over plain text columns that are not order_* columns (playlist.name, album.name, media_file.title, playlist_tracks title) are wrapped explicitly, and qualified with their table because 'user' is joined and also has a 'name' column. The option defaults to off because the collation cannot use the existing indexes: measured on a synthetic 110k album library, the first page of an album-by-name listing goes from 0.03ms to 14ms. Indexing the expression was rejected outright - an index declared with a custom collation makes the whole database unreadable to any tool that does not register it, including the sqlite3 CLI, which fails even on 'select count(*)' and 'pragma integrity_check'. * refactor: fold the two sort-order mappers into one mapSortOrder and mapNaturalOrder shared the same regex and loop, differing only in the expression they substituted, and setSortMappings picked between them with a two-case switch. mapSortOrder now selects the column shape itself and defers to collatedSort for the collation, so the 'collate' clause is emitted in one place and the caller only has to decide whether any mapping is needed at all. The mapper tests were three near-identical cases that each hard-coded one flag combination; they are now a DescribeTable covering all four combinations of PreferSortTags and EnableNaturalSorting, which the previous set did not. The album sorting specs collapse the same way. Behavior is unchanged. * fix: leave plain sort columns alone when natural sorting is off collatedSort wrapped its column unconditionally, so the tiebreakers added for plain text columns picked up 'collate nocase' even with EnableNaturalSorting off. media_file.title, the playlist_tracks alias of it, and user.user_name are all declared without a collation, so a default install would have silently switched those tiebreaks from binary to case-insensitive ordering. Only playlist.name was already NOCASE and genuinely unaffected. The helper is now naturalSort and returns the column untouched unless the option is on, so the default path keeps the collation each column was declared with. sortCollation had a single remaining caller and folded into mapSortOrder. Tests: the CompareFold table body was a verbatim copy of the Compare one, so both now go through one expectOrder helper, and the album sorting specs inline two single-use closures. * fix(natural): defer the leading-zero tie-break to keep ordering transitive Compare applied the padding difference between numerically equal digit runs only when one side ended at the digit boundary, and ignored it mid-string. That made the relation intransitive: CompareFold("1","1a") < 0 and CompareFold("1a","01a") == 0, yet CompareFold("1","01a") > 0. SQLite requires a collating function to be transitive and leaves ORDER BY undefined otherwise, so registering this as NATSORT was not safe. Reproduced with the real driver on three artist names that occur in practice - "3", "3 doors down" and "03 greedo" - where paging one row at a time returned "03 greedo" twice and dropped "3" entirely. The padding difference is now carried as a tie-break that is applied only when the strings are otherwise equal, which restores transitivity while keeping the documented intent (a01 < a1, a0 < a00). Three existing entries changed: each asserted that two distinct strings compare equal, which was the same defect seen from the other side. Found by the Codex review on #6015. |
||
|
Deluan Quintão
|
59810c3d59 |
feat(jellyfin): non-expiring, audience-scoped tokens revocable by password change (#6013)
* feat(auth): add per-user token_epoch column and bump method * feat(auth): add aud and ep claims, omitted when zero * feat(auth): add CreateAPIToken for non-expiring, audience-scoped tokens * feat(auth): add CheckClaims for epoch and audience validation * feat(jellyfin): issue non-expiring, jellyfin-scoped access tokens * fix(subsonic): reject API-scoped and revoked tokens on the jwt path * fix(server): reject API-scoped and revoked tokens on the native API * fix(server): pin the token-subject guard and stop leaking test config Adds a regression spec for the DevAutoLogin/ExtAuth guard in tokenAllowed, switches its comparison to case-insensitive to match the user lookup's own COLLATE NOCASE semantics, and restores Subsonic JWT test config after each spec instead of leaking SessionTimeout. * feat(request): add a token epoch holder for handler-to-middleware signalling * refactor(server): write the refreshed JWT header after the handler runs * feat(auth): revoke all tokens for a user when their password changes * fix(server): restore Unwrap on the JWT refresh writer so SSE write deadlines apply * test(auth): pin that non-session tokens reject API access tokens * test(jellyfin): pin token scoping and epoch revocation end to end Exercises auth.CreateAPIToken and CheckClaims against the real Jellyfin router and SQLite DB: the minted token has no exp and is aud-scoped to jellyfin, and bumping token_epoch through the real UserRepository revokes an already-issued token on the next protected request. * test(nativeapi): pin the token-epoch handoff through a real password-change request Drive a self password change through the real Authenticator/JWTRefresher chain and a real SQLite-backed userRepository, so the epoch handoff between Put and the refreshed-token writer is verified end to end, not as two separately-tested halves. Also fix tokenAllowed to read the enriched ctx it was given instead of r.Context(), so its warning log carries the username. * refactor(server): drop tokenAllowed's now-unused request parameter Finding-2 already moved every use to ctx; r was dead weight. Also note in the new nativeapi test why it must stay the package's only real-DB spec: db.Db() is a process-wide singleton its cleanup closes for good. * refactor(auth): remove duplication in claim decoding and token minting * refactor(auth): group aud with the standard JWT claims * refactor(auth): read aud with the standard-claim accessor pattern * fix(log): redact every api_key spelling the Jellyfin API accepts * fix(auth): bind session tokens to the user id, not just the username * fix(auth): return the token epoch from the same atomic increment * fix(auth): bump the token epoch in the same statement as the password write * chore(auth): trim comments to the why-only budget |
||
|
Deluan Quintão
|
07b6411c0b |
perf(artwork): cap the stale-absent recheck at 100 items per kind per hour (#6007)
* feat(artwork): drip the stale-absent recheck instead of bursting it daily Each hourly housekeeping tick now re-queues at most 100 absent states per kind, oldest attempts first, instead of everything older than 24h at once. External agents see a flat ~100 requests/hour per agent instead of hourly bursts of ~2,000, and the effective recheck interval self-scales with the size of the absent pool (~4 days at 10k absent artists) while small libraries keep the 24h floor. * feat(artwork): trust an absent artwork state for a week before rechecking With the recheck now dripped at 100 items per kind per hour, the 24h floor only governed small libraries, where the drip cap never binds; they still re-asked every agent daily. A 7-day floor cuts that cost 7x and, for large libraries, becomes the binding limit over the drip cycle (~5.7k calls/day instead of ~9.6k at 10k absent artists). Among comparable servers, this is still the second-most-eager recheck: gonic retries misses every 30 days, Jellyfin and Funkwhale never do. * refactor(artwork): state the drip's backpressure contract where it bites Review follow-ups: the recheck limit deliberately caps the *selection*, not the insertions — already-queued rows use up budget, so a stalled drain admits no new work instead of building a recovery burst. Say so in the interface doc, mirror it in the mock by truncating the sorted candidates (matching the SQL's LIMIT-before-ON CONFLICT), and teach `artwork status` and the worker doc the post-drip wording. Also pin the one cmd fixture that still assumed a 24h recheck window. |
||
|
Deluan Quintão
|
ffc68e29db |
feat(cli): add artwork cancel to call off queued artwork work (#6006)
* feat(cli): add `artwork cancel` to call off queued artwork work A bulk backfill had no off switch. Changing an artwork setting bumps the config fingerprint, which enqueues every entity in the library, and the only way to stop it was to turn agents off -- which changes the fingerprint again and enqueues a second full backfill. The escape hatch was the trap. `artwork cancel` deletes pending queue rows selected by --kind and/or --priority, with the --dry-run/confirm/-y flow `reprocess` already uses. Cancelling by priority is the point: it drops a runaway backfill while leaving the bump-priority rows an operator queued by hand. It only touches the queue. Resolved artwork and the item_artwork state behind `artwork explain` are left alone, and the trace of why a cancelled item last failed goes with its row. Preserving that trace would mean writing it to last_failure, which `explain` prints under "Gave up after" -- reporting a cancellation as an exhausted retry budget. The help text says the trace is discarded instead. Two limits the help text states, because neither is guessable: work already dequeued is not interrupted, and an item with no artwork state yet can be queued again by the hourly missing-artwork recheck. Cancel calls off queued work; it does not stop the worker. --kind validates against RefreshableKinds, not the RecheckKinds `reprocess` uses: the queue holds media file rows, so --all has to reach them. PurgeQueued follows the repository's naming rule -- it finds its own rows and reports how many went -- and ignores retry_at, since a row still backing off is pending work. The preview reuses CountByKindAndPriority rather than adding a counter. reprocessConfirm became confirmUnlessYes(yes, in, verb) now that two commands prompt. * refactor(cli): share the artwork queue filter between the preview and the delete Follow-up cleanup on the previous commit; no change to what the command does, apart from --all, noted below. The "which rows does cancel touch" predicate was written three times: once as SQL in PurgeQueued, once in Go in cmd's matchingQueueStats, and once more in the mock. The preview and the delete could therefore drift, and the mock would keep the tests green while they did. persistence now has one artworkQueueFilter, shared by PurgeQueued and a new CountQueued, and cmd does no filtering at all. That also makes the preview cheaper. It counted the whole queue and filtered in Go, so `artwork cancel --kind al` scanned every row of every kind to print a handful. CountQueued pushes the filter into SQL, which the drain index serves as a range seek. CountByKindAndPriority is gone: it is CountQueued(nil, nil). --all now selects with an empty filter instead of enumerating RefreshableKinds. It is what the flag help already claimed, and the enumeration was narrower than its own documentation -- a queue row whose item_kind this build does not know survived `--all` with no flag combination able to remove it. It also restores SQLite's truncate path: measured with EXPLAIN QUERY PLAN, a bare DELETE plans to nothing, while `WHERE (1=1)` -- which an empty squirrel And renders -- plans to a full index scan. A test pins the filter's emptiness so that cannot regress silently. Also folded together three copies of the parse-and-dedup loop (parseAll), two copies of the queue-stats table (printQueueStats, now shared with `artwork status`), two copies of the stat sum (queueTotal), and four copies of the kind-to-prefix mapping (model.KindPrefixes). The PurgeQueued specs became one DescribeTable that asserts count and delete agree on every selection. * docs(cli): say when `artwork cancel` evaluates its selection The help text covered the two limits that surprise an operator after the fact, but not the one that bites during the prompt: the count is a preview, and the filters run again on confirm. A scan or a manual refresh landing in between is cancelled without ever appearing in the table the operator agreed to. Deleting only the previewed rows was considered and rejected. The exposure is one item re-resolving on next view instead of immediately: clearing an item's artwork state is what every recovery path selects on, so a lost Bump row from artwork.Refresh comes back at the same priority via provisional() on the next request, and otherwise within the hour via EnqueueAllMissing. Buying a guarantee against that costs the truncate path on --all, the flag that exists for a 29k-item backfill. * refactor(cli): share one set of flag targets across the artwork subcommands reprocess and cancel each declared their own kinds/all/dry-run/yes variables, but cobra only ever parses the one subcommand being run, so the two sets could never hold values at the same time. backup.go already binds one backupDir across two subcommands and one force across two more; this follows that. Ten package-level variables become six. Each command keeps its own help string and its own valid-kind list, so --kind still reports RecheckKinds for reprocess and RefreshableKinds for cancel, and --source and --priority stay registered only on the command that has them. The priority lookup table is now knownPriorities, freeing the artworkPriorities name for the flag. The new name also reads better against priorityName's fallback for a value it does not know. |
||
|
Deluan Quintão
|
c26f6f9e98 |
feat(artwork): store the resolution trace so artwork explain works offline (#5980)
* feat(artwork): record the resolution trace so explain works without --live The worker never attached a ChainTrace, so `artwork explain` had to re-walk the priority chain at CLI time. That reconstruction could disagree with what actually happened, and without --live it could not report the external tier at all. The worker now traces every acquisition and stores it. `explain` reads the stored trace by default and reports when it was recorded; --live re-walks and calls the agents. Disc artwork keeps no row, so it always walks live. A chain trace alone would have explained almost nothing about failures: six of the seven ways an item can fail happen after the chain has already picked a winner. The trace now covers those stages too, and has somewhere to live when they fail: the retrying queue row carries the last failure, and the state row keeps it in last_failure once the retry budget is spent and the queue row is deleted. Measured on a copy of a 682MB / 43.6k-item library: +9.7MB (+1.4%). No row crosses the WITHOUT ROWID overflow threshold, so list hydration is unchanged; only full scans of item_artwork, which no request performs, read more pages. * test(artwork): pin the give-up ordering that keeps a failure for unresolved items recordGiveUp updates an existing row, and for a kind with a recheck path that row is only created moments earlier by the absent settle. Recording before the settle would lose the failure for every item that never resolved, with nothing to catch it. * refactor(artwork): tighten the trace code after review Four fixes worth taking: The doc comments on ChainTrace and chainState.trace still said the worker never attaches a trace and resolution stays allocation-free — the exact invariant this branch reverses. explain's report field meant both "the chain shown was walked just now" and "go out for real", and was being passed to loadPluginAgents, which --live documents as the only thing that may open external connections. Renamed to `walked` and restored explainLive as the sole input to that decision. A stored Detail is an error string on the failure paths, with no bound. The measured "no row reaches the WITHOUT ROWID overflow limit" only holds while it is bounded, so cap it at 200 runes. offlineGate was a factory returning a constant closure; make it a plain gateFunc like its sibling passthroughGate. Collapse five copies of the age-a-queue-row loop in the worker tests into one helper. * refactor(artwork): drop the offline explain walk, now that traces are stored `artwork explain` reported the external tier without calling it, so a diagnostic could not add load to a provider already rate-limiting us. Reading the stored trace answers that better: it reports what the agents actually returned, not what would be tried. Nothing could reach the offline gate any more. It was installed only for a walk with --live unset, which now happens for disc artwork alone, and disc rejects the external candidate before any gate call. That made the gate, its sentinel error, the would-try outcome and two of explain's verdicts unreachable. Removes offlineGate, errOfflineSkipped, OutcomeWouldTry, the NewTracingResolver live parameter and the CreateArtworkResolver argument threaded through wire. Verified against a copy of a real library: disc artwork with "external" first in DiscArtPriority and external services enabled still records the skip and issues no agent call. * fix(artwork): make explain's no-network guarantee structural, not incidental Serving falls back disc -> album and track -> disc -> album. The resolver layer explain uses has no such fallback today, so dropping the offline gate did not leak. But the guarantee rested on which chains happen to lack an external tier, and the serving layer already shows the fallback shape someone could mirror. Without --live the tracing resolver is now built with no agents at all, so no chain and no fallback added later can reach a provider. That is stronger than the gate it replaces, which only intercepted the call. The test pins it against exactly that regression: with the guard removed and the serving fallback mirrored into resolveDisc, it fails. * refactor(artwork): trim the trace plumbing EncodeTrace was exported for nobody: only this package writes traces, and cmd reads them. It becomes a ChainTrace method, which also drops the copy Steps made for a caller that only wanted to serialize. explain's report carried queuedSteps and failureSteps, both pure functions of the queue and state rows already in the struct, which let a test set the two out of step with each other. formatExplain derives them, as it already does for every other display value. The trace row format and its tabwriter empty-cell rule lived in two places, and the "nothing was ever recorded" predicate in three. * fix(artwork): clear the queue trace on a fresh re-enqueue Enqueue's conflict clause reset attempts to 0 but left the new trace column, so after a scan or refresh re-enqueued a previously-failed item artwork explain showed "Attempts: 0" next to the prior lifecycle's "Last attempt failed" trace. Clear trace in Enqueue (a fresh lifecycle has no last attempt); EnqueuePreservingBackoff still keeps it. * fix(artwork): treat a processing-stage error as indeterminate in explain A read/hash/decode/store failure records an OutcomeError step and writes an absent row, but explainResult only mapped external errors and unreadable candidates to indeterminate, so the default verdict read "not resolved" — presenting a processing failure as a definitive miss. The worker retries these exactly as it retries an unreadable candidate, so classify any OutcomeError as indeterminate too. * fix(artwork): record a trace step when a chainless resolver faults Playlist and radio resolvers walk no priority chain, so a fault (unreadable upload/sidecar, or an m3u fetch error with no grid) returned localError/extError without recording any trace step. The attempt then encoded [], leaving artwork explain with an empty "Last attempt failed" and "Gave up after". Record a fallback step in the faulted-no-image branch when nothing else did, and carry the source label through resolveLocalFile so the step can name it. * fix(artwork): trace the m3u failure at its source, not via the empty guard A playlist's grid sampling records album-chain steps into the shared trace, so the processor's empty-trace fallback no longer fires when the m3u remote image fetch failed — the error that forced the retry was omitted from explain. Record it where it happens, in resolvePlaylist's external step, as external:m3u. * test(artwork): skip the chainless-fault spec on Windows The spec provokes an open fault with a non-directory parent, but Windows maps that to a not-exist error, so localError is never set and the item resolves absent instead of failed. The sibling failed-on-unreadable-upload spec skips Windows for the same class of reason. * fix(artwork): don't label an absent empty-chain row as pre-tracing explain reported "resolved before traces were recorded" for any stored row with an empty chain, but an empty CoverArtPriority records a real, empty [] chain and resolves absent. A recorded resolution that finds an image always records its winning candidate, so only a row with a hash and no chain predates tracing; split on the hash and report an absent empty chain plainly instead. * fix(db): retimestamp the artwork trace migration after rebase master merged a 2026-08-18 migration, so the original 2026-08-16 timestamp is now older than the newest on the base branch and Goose would silently skip it on an already-upgraded database. Bumped past it; the SQL is unchanged. * fix(artwork): keep the m3u error detail in the trace The m3u trace step recorded OutcomeError with no detail because resolveExternalStep collapsed the gate's error to a bool, so explain showed only "external:m3u error -" and could not tell a timeout from an HTTP error or an open breaker. Return the error (normalizing not-found to nil so it stays a definitive miss, not a failure) and store its message as the step detail; encodeSteps already bounds it. * docs(artwork): note the give-up write relies on serial draining recordGiveUp writes last_failure unconditionally; that is only correct because the drain resolves each item serially, so no concurrent success can store artwork between the write and the queue delete. Record the invariant at the call site. |
||
|
Junker der Provinz
|
c362519f76 |
test: unskip path-separator tests on Windows (#5381) (#5916)
* test: unskip AbsolutePath and i18n path-separator tests on Windows (#5381) Signed-off-by: junkerderprovinz <jdp@braethoria.com> * test: unskip metadata folder-PID test on Windows via path.Dir (#5381) Signed-off-by: junkerderprovinz <jdp@braethoria.com> * test(storage): make relative-folder assertion cross-platform and unskip on Windows (#5381) Signed-off-by: junkerderprovinz <jdp@braethoria.com> * fix(persistence): normalize folder-update-info paths with forward slashes on Windows (#5381) Signed-off-by: junkerderprovinz <jdp@braethoria.com> * review: drop folder-PID change, trim storage_test comment (#5381) Revert model/metadata/persistent_ids.go to master: switching the `folder` PID attribute from filepath.Dir to path.Dir would change the persistent IDs of existing Windows libraries and needs a migration path, so it is out of scope for this PR. The matching test unskip is reverted with it, leaving #TBD-path-sep-metadata open in #5381. Trim the core/storage/storage_test.go comment to two lines. Signed-off-by: junkerderprovinz <jdp@braethoria.com> --------- Signed-off-by: junkerderprovinz <jdp@braethoria.com> Co-authored-by: Deluan Quintão <deluan@navidrome.org> |
||
|
Deluan
|
6d8a3e48ee |
refactor: replace md5 with xxh3 for faster and more efficient hashing
Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
Deluan Quintão
|
7a11ca69bb |
fix(jellyfin): honor the Filters, SortBy and MaxHeight params clients actually send (#5981)
* fix(jellyfin): honor Filters=IsFavorite on /Artists and /Artists/AlbumArtists listArtistsByRole hand-built its itemsQuery and never set favOnly, so the favorites filter was silently dropped on both artist routes while /Items honored it. Finamp's home screen asks for favorite artists once per load and was served the entire artist list instead: 10,298 artists, 6.15 MB, 2.7s on a real library, and the wrong data on screen. Extract the favOnly parsing that parseItemsQuery already did into parseFavOnly and use it in both places. listArtists now adds the starred predicate to notMissing rather than replacing it, matching listAlbums and listSongs, so a favorite artist whose files are gone stays excluded. * fix(jellyfin): map SortBy=Runtime to duration for albums and songs sortColumnsByType had no runtime/runtimeticks key for any type, so Finamp's "Duration" sort silently misbehaved in two different ways. Albums: Finamp sends a bare SortBy=Runtime. Nothing matched, opts.Sort stayed empty, and applyOptions skips OrderBy entirely when Sort is empty — so the query ran with no ORDER BY at all and Ascending and Descending returned identical lists. Songs: Finamp sends SortBy=Runtime,AlbumArtist,Album,SortName. applySort takes the first *recognized* key, so Runtime was skipped and the list came back sorted by album artist while looking correct. Both repos already accept a duration sort (mediafile_repository maps it explicitly; album_repository falls through to the column name), so no migration is needed. Sorting 97k songs by duration costs a temp B-tree (~114ms on a prod-sized copy) — the same cost the Subsonic and UI duration sorts already pay, and correct where the previous behaviour was merely fast. * fix(jellyfin): apply the played/unplayed filters and MaxHeight image bound Filters was matched with a substring test for IsFavorite, so every other token Jellyfin defines was silently dropped and the response kept rows it should have excluded. Finamp sends Filters=IsUnplayed in normal use. Replace the bool with a parsed itemFilters carrying nullable favorite and played flags, so isFavorite=false and isPlayed=false are real filters rather than indistinguishable from an absent param. Standalone params are read first and the Filters list overrides them, the precedence real Jellyfin has. IsFavoriteOrLikes now maps to favorites deliberately instead of by substring accident; Likes, Dislikes, IsFolder, IsNotFolder and IsResumable have no Navidrome equivalent and are dropped rather than half-applied. The negative cases match NULL as well, since annotations are LEFT JOINed and an untouched item has no row. getItemImage read only maxwidth, so a client sending just MaxHeight got the full-size original: measured against a real cover, maxHeight=100 returned 82,570 bytes where maxWidth=100 returned 3,316. Use the tighter of the two bounds. * refactor(jellyfin): share the plain-param parser between /Items and /Artists listArtistsByRole hand-listed the itemsQuery fields it happened to need, which is exactly how the favorites filter went missing: the literal has been amended in four of the five commits that touched it. Extract listParams for the fields that come straight from query params so both paths read one parser, and the next supported param reaches every list path instead of only /Items. Also from the cleanup pass: collapse imageSize to a single clamped comparison and read its bounds through req.Params like the rest of the package, which drops the strconv import; build the artist and playlist filter lists with the flat append shape the album and song paths already use, instead of re-wrapping opts.Filters into a nested And per predicate; drop a nil guard in listPlaylists that no caller can reach, since both paths into queryItemsOfType build QueryOptions without Filters. applySort now logs when no SortBy key resolves at all — a miss inside a fallback list is normal, but none matching means a silently ignored sort, the failure mode that hid the Runtime bug. Its doc comment records why the remaining keys cannot simply be joined. Folds three duplicated test bodies into the tables that already parameterize them, and covers the artist-parent album branch, which reaches notMissing through filter.AlbumsByArtistID rather than the default branch. * docs(jellyfin): correct how applySort describes Jellyfin's SortBy semantics The comment claimed SortBy is a comma-separated fallback list. It is not: RequestHelpers.GetOrderBy (10.10) builds one (ItemSortBy, SortOrder) pair per key, so Jellyfin orders by every key in turn. Navidrome applies only the first recognized one, which is a real divergence — secondary keys never break ties — not the intended reading of the parameter. The assertion that the keys cannot be joined was also wrong. buildSortOrder does split its input on commas; what it maps is the whole string, so joining raw Jellyfin key names misses the mappings. Mapping each key first and joining the results would work, which makes multi-key sorting a real option rather than a blocked one. Documenting the current behaviour as a known divergence until then. * fix(jellyfin): order by every recognized SortBy key, not just the first Jellyfin orders by each SortBy key in turn, so "DatePlayed,SortName" means break ties by name. Navidrome applied only the first recognized key and dropped the rest, which is 28% of the sort traffic on a real server (23 of 82 requests in 12h carry 2-5 keys). Most were harmless because the primary key dominates, but PremiereDate,Album,ParentIndexNumber,IndexNumber,SortName came back unordered within a year. The keys cannot simply be joined: sortMapping keyed on the whole Sort string, so a joined value missed every mapping and fell through to raw column names. Make it resolve a comma list per part, but only when every part is a known key — the four existing callers that pass raw column lists (core/matcher, core/lyrics, core/maintenance, subsonic/browsing) all carry a part that is not a mapping key, several with their own direction, so they keep falling through exactly as before. Verified each one. applySort now collects every recognized key, skipping duplicates so ParentIndexNumber,IndexNumber does not repeat a column. random stays alone: the repo matches it by exact string equality, so joining it would both break that path and emit a bare 'random' column into the ORDER BY. Verified against a prod-sized copy: every multi-key combination seen in real traffic returns 200, and a secondary key now changes the order within a tied year for songs. Albums are unchanged there, because their max_year mapping already ended in ", name". * fix(persistence): resolve sort mappings exactly once Making sortMapping resolve a comma list per part broke an invariant it had been relying on: idempotence. sanitizeSort mapped the sort key up front and applyOptions then ran buildSortOrder over the result, so sortMapping was already being handed its own output. That was harmless only while a mapped value could never look like a key list. media_file's rated_at maps to "rating, rated_at", and both parts are keys, so the second pass expanded it to "rating, rating, rated_at". Found by round-tripping every mapping in all four repositories; it was the only collision, and the duplicate sort key was benign in SQL, but any future mapping of that shape would silently change meaning. sanitizeSort now validates without resolving, leaving buildSortOrder as the single mapping point. The generated SQL is unchanged — the whole suite passes apart from the two specs that asserted the old return value, which are updated and joined by a round-trip guard covering exactly the rated_at shape. Also use the paren-aware splitFunc that buildSortOrder already uses, so an expression carrying commas inside its parentheses cannot be split apart. * refactor(jellyfin,persistence): flatten the sort resolution paths Cleanup pass over the branch, no behavior change. sortMapping loses the len(parts)>1 guard, which existed only to pick between two identical toSnakeCase exits; the single-key case now falls through the same loop. lookupSortMapping hands back the snake_case form it had to derive so the fallback stops recomputing it — toSnakeCase is two regexps, and on a miss it was running twice per call. sanitizeSort now asks lookupSortMapping instead of probing the map itself, so "is this a known sort key" has one answer; the two had already drifted, since sanitizeSort tried one casing where the resolver tries three. applySort folds the nested random branch into the skip condition and the two trailing length tests into one switch. setSortMappings documents the invariant the comma-list rule depends on, where someone adding a mapping will read it. The README line describing SortBy still said only the first key applied, which the commit before last made false. Tests: the twelve near-identical sorting specs become one DescribeTable of (itemType, SortBy, want) triples, 124 lines to 36, and the applyOptions round-trip assertion collapses to the buildSortOrder call its sibling uses. * fix(jellyfin): keep annotation filters out of search, resolve sorts per part Two findings from the Codex review on #5981. The played/unplayed filters turned working requests into 500s when combined with SearchTerm. Search runs a two-phase FTS query whose first phase selects rowids with no annotation join, so a starred or play_count predicate there is "no such column", not a filter. Measured against master: MusicAlbum with SearchTerm and Filters=IsUnplayed went 200 -> 500, likewise IsPlayed and the Audio equivalents. listAlbums and listSongs now skip those predicates on the search path, matching what listArtists already did. That also clears the same 500 master already had for Filters=IsFavorite with SearchTerm. sortMapping resolved a comma list only while every part was a known key, so a list mixing a plain column with a mapped key kept neither: MusicAlbum SortBy=Runtime,SortName arrives as "duration, name", and duration is a plain album column, so name stayed raw instead of expanding to order_album_name. Albums whose name differs from its sort form — 1,366 of 6,987 on a real library — then ordered by the wrong secondary key, and PreferSortTags was ignored. Each part is now resolved on its own, which is what setSortMappings already documents for a single field. Verified every in-tree caller that passes a raw column list still produces its original ORDER BY. Codex also asked for the artist search path to apply the same filters. It would 500 for the reason above, and wrapping the library scope in a compound filter makes requestedLibraryIDs stop recognizing it, silently widening the search past the requested ParentId. * fix(jellyfin): honor the first SortOrder value for a multi-key sort applySort compared the whole SortOrder string with "Descending", so a per-key list like SortOrder=Descending,Ascending failed the match and every key, including the primary, sorted ascending — the exact opposite of the request. Take the first comma-separated value, which Jellyfin also uses for any key past the end of the SortOrder list. True per-key directions can't be expressed through the single opts.Sort string and are left out; no observed client sends a SortOrder list. |
||
|
Deluan Quintão
|
2e03766a9d |
fix(playlist): preserve smart playlist song count on re-import (#5907) (#5908)
* fix(playlist): preserve smart playlist counters on re-import (#5907) * perf(playlist): skip re-importing unchanged NSP files (#5907) * feat(playlist): also store content hash for M3U imports (unused for now) * fix(playlist): return stored record when skipping unchanged NSP import Skipping before copying the stored identity broke the ImportFile(sync=false) contract: callers received an ID-less playlist and the requested Sync change was silently dropped. * refactor(playlist): hash imports once at the caller; protect smart counters in Put Move content hashing out of both parsers into the code that owns the file (parsePlaylist and ImportFile), removing the NSP double-buffer and the duplicated hashing idiom. Put now drops song_count/duration/size for smart playlists (PostMapArgs), disarming the counter-zeroing trap for all callers. * fix(playlist): invalidate imported hash when rules are edited via API Without this, a rules edit through the REST API kept the stored file hash, so every scan skipped the unchanged file and never restored the file-backed rules while sync was on. * test(playlist): verify smart counters survive a re-import, end to end The existing Put test seeds the stored counters with a raw SQL update, so it pins the guard in PostMapArgs but not the pipeline around it. This test drives the counters through a real evaluation instead: it saves a smart playlist, reads it with GetWithTracks to populate song_count/duration/size, then saves the playlist the way the scanner rebuilds it after parsing the .nsp file, with the counters back at zero. Both routes fail without the guard, and the new one covers the exact sequence reported in #5907. Test taken from #5970, which diagnosed the same root cause independently. Co-authored-by: Junker der Provinz <133605895+junkerderprovinz@users.noreply.github.com> * test(playlist): build the service with artwork.NewUploader The artwork pipeline in #5847 replaced core.NewImageUploadService() with artwork.NewUploader(ds) and updated every call site it could see. The five call sites this branch adds were written against the old constructor, so the merge applied cleanly but left the package uncompilable. * fix(db): re-stamp the imported_hash migration after the master merge Master gained three migrations while this branch was open, the newest being 20260816180040. The original 20260808200333 stamp now sorts before them, so any database already upgraded past that point would skip this migration entirely and never get the imported_hash column. Same SQL, current timestamp. * refactor(playlist): hash imported playlists with xxh3 and the id encoding ImportedHash is a change detector, not a security boundary, so it does not need a cryptographic digest. xxh3 is already a direct dependency and is used the same way to fingerprint files in the artwork image store. Encoding the 128-bit digest with id.Encode stores it in the same 22-char base62 form as every other id in the schema, down from 64 hex chars. No migration is needed: the imported_hash column has not shipped in a release, so no database holds a value in the old format. * refactor(playlist): extract the imported-playlist fingerprint helper Both import paths encoded the hash inline, so how a playlist file is fingerprinted lived in two places. A third import path that encoded it differently would silently never match the stored value, turning the unchanged-file skip into a no-op. --------- Co-authored-by: Junker der Provinz <133605895+junkerderprovinz@users.noreply.github.com> |
||
|
Deluan Quintão
|
65751d7665 |
fix(playlists): chunk track deletes to stay under the SQLite variable limit (#5977)
PlaylistTrackRepository.Delete built a single IN clause with one bind variable per track, so removing more tracks than SQLITE_MAX_VARIABLE_NUMBER (32766) failed with "too many SQL variables". Clients that sync a large playlist by adding the desired tracks and then removing the stale ones would get the add committed and the removal rejected, leaving the playlist with both sets of tracks and growing it on every sync. Delete now works in chunks of 200, the same size addTracks already uses, and renumbers once after the last chunk. Both callers already run inside a transaction, so the delete stays atomic. |
||
|
hotorcelexo
|
ea1e2b95a7 |
fix(db): keep album created_at in the driver's timestamp format when copying (#5867)
* fix(db): keep album created_at in the driver's timestamp format when copying Signed-off-by: IgorPolyakov <igorpolyakov@protonmail.com> * fix(db): move created_at renormalize migration after merged migrations The migration was versioned 20260813140000, which is older than 20260815015320 (already merged). goose.UpContext runs without WithAllowMissing, so any database that already applied the newer migration would fail with "found 1 missing migrations" and db.Init would log.Fatal on startup. --------- Signed-off-by: IgorPolyakov <igorpolyakov@protonmail.com> Co-authored-by: Deluan Quintão <deluan@navidrome.org> |
||
|
Deluan Quintão
|
5b758fc20c |
fix(artwork): re-resolve artwork when image files change on disk (#5965)
* fix(artwork): re-resolve artwork when image files change on disk An image-only folder change (replaced, added, or deleted cover/artist images, with no audio files touched) was detected by the scanner but never reached the artwork queue, so clients kept seeing the old coverArt hash until something else forced a re-resolution. Phase 1 now diffs each changed folder's image list and imagesUpdatedAt against the previously persisted folder row, and at the end of the phase bulk-enqueues re-resolution for the affected entities: albums with tracks in the folder or its direct children (covering disc subfolder layouts), and, when an artist-pattern image is involved, artists with albums under the folder's subtree, mirroring the artist resolver's upward search. The artist mapping mirrors the resolver's sole-album-artist album selection. New repository helpers keep the mapping set-based and light: folder GetAllIDs, media_file GetAlbumIDsByFolder (distinct, indexed by folder_id), and album GetSoleAlbumArtistIDs. * refactor: simplify the image-change artwork enqueue after review Load the previous folder image state through the existing GetFolderUpdateInfo bulk pre-pass instead of a per-folder SELECT inside the persist transaction, and skip the diff for new folders, whose artwork the scanner already enqueues inline. Move the artist-image classification into core/artwork (IsArtistImageFile) so the scanner shares the resolver's ArtistArtPriority token grammar instead of re-parsing it (the copy mistreated image-folder as a filename glob). Move the folder-subtree query into the folder repository (GetSubtreeIDs) with LIKE escaping and expression-tree batching, share the sole-album-artist predicate between the resolver and the album repository (model.SoleAlbumArtistFilter), extract a chunked single-column query helper, and deduplicate the ArtworkQueueItem literals behind scanArtworkItem. * fix(persistence): keep slash-form paths in GetSubtreeIDs subtree predicates The scanner hands GetSubtreeIDs io/fs slash-form paths, but filepath.Clean rewrites them with backslashes on Windows while folder.path is stored with forward slashes, so the descendant predicates matched nothing and nested artist folders were never re-enqueued there. Normalize with path.Clean, like HasAudioOutsideFolders does, and cover a nested path in the repo test. * refactor(persistence): move the sole-album-artist rule into the album repository SQLizer filters belong in the persistence package, not model. The rule becomes an unexported filter shared by GetSoleAlbumArtistIDs and a new GetBySoleAlbumArtist repository method, which the artist artwork resolver now calls instead of building the squirrel filter itself. * perf(scanner): resolve image-change artists in one query over album.folder_ids The artist half of the image-change enqueue walked folder subtree IDs, then media_file rows, then album rows, marshalling thousands of bound IDs through the driver on each hop. Matching albums by their own folder_ids instead is one statement, and folder_ids is the same source the artist resolver uses to compute an artist's folders. Benchmarked against a copy of the production DB (97k tracks, 10k folders, 7k albums): 87ms +/-196% -> 17.4ms +/-8%, 7.1MB -> 172KB, 103k -> 1.5k allocs. The subtree predicate becomes a shared folderSubtreeFilter, so Folder GetSubtreeIDs and Album GetSoleAlbumArtistIDs are no longer needed. * fix(scanner): persist ancestor folders discovered by a quick scan A quick scan skipped any new folder with no files of its own, so an artist folder holding only album subfolders never got a row. Adding artist.jpg to it later then produced no artwork enqueue: the entry was new, so the image diff was skipped, and it has no tracks, so nothing was enqueued inline either. Skip only genuinely empty new folders, matching what a full scan already persists. This also fixes artist artwork resolving as absent for artists first imported by a quick scan, since the resolver's folder climb needs that row. Also normalizes the selective-scan preload paths with path.Clean, so its descendant predicates match the stored slash-form paths on Windows. * fix(persistence): chunk subtree paths and match artist globs by basename Two regressions from earlier commits on this branch. Collapsing the subtree query into a single statement dropped the chunking the old GetSubtreeIDs had: each path expands into 3 OR terms and SQLite rejects an expression tree deeper than 1000, measured at 166 paths. A library with more artist-image folders than that (the prod copy has 158) would fail the whole collect, dropping the album items with it, so the scanner now keeps them when the artist query fails. The artist-image classifier compared whole tokens after stripping album/, so a directory-bearing glob like images/artist.* never matched the basenames the scanner has. Match on path.Base, which is what album/artist.* already reduced to; the resolver climbs parent folders, so an exact prefix is not knowable here and a conservative match is the right failure direction. * refactor(persistence): halve the repository surface this PR adds Research on the four new repository methods found two were avoidable. GetAlbumIDsByFolder now expands the changed folders to their direct children in its own subquery, so Folder.GetAllIDs has no callers and is deleted, one round trip per scan disappears, and the previously unchunked id/parent_id IN lists are covered by the existing chunking. GetBySoleAlbumArtist becomes an exported SoleAlbumArtistFilter, matching the ParticipantIDFilter precedent for sharing a Sqlizer with core/, so the rule still lives in persistence but AlbumRepository gains nothing and the mock shim that ignored the artist filter is gone. Also drops queryAllSliceChunked, now callerless, in favour of the file-local slices.Chunk convention used by the sibling folder queries. Rejected on measurement: matching the album path by album.folder_ids is exactly equivalent (13975 pairs, zero difference) but has no index, so it scans every album and runs 5-200x slower than the media_file route. * refactor(persistence): stop reading the deprecated album_artist_id column Both artist lookups this PR touches now go through participation, matching the precedent in core/archiver.go and share_repository.go. SoleAlbumArtistFilter uses ParticipantIDFilter, which is also faster: the album_artists unique constraint is a covering index for it, while the old column needed album_artist_album_id plus a row fetch. GetSoleAlbumArtistIDsInSubtrees reads the sole artist out of the participants JSON it already parses for the sole-artist check, rather than joining back to album_artists, which measured ~1.6x slower on a prod-sized copy. Verified equivalent on that copy: 6828 sole-artist albums and 1088 subtree artists resolve identically via the column, the join and the JSON. The tests now set a deliberately wrong album_artist_id so they fail if either query starts reading it again. * docs: trim comments that carry rationale belonging in commit messages Five comments had grown past the budget with benchmark numbers, rejected alternatives, and a duplicate of the constant's own explanation. * refactor(scanner): move the image-change enqueue into phase_1_folders The three functions were methods on phaseFolders, so they belong with the type; phase_1_image_changes.go also read like a fifth phase, which it wasn't. * refactor(scanner): extract the image-change collector into its own type phaseFolders no longer owns the per-library map and the mapping methods; it records into a collector and asks it to enqueue once. The collector keeps the library alongside the folders, so enqueue needs only ctx and the datastore. * refactor(scanner): simplify enqueue method by removing redundant datastore parameter Signed-off-by: Deluan <deluan@navidrome.org> * docs(scanner): drop the stale zero-value claim on imageChangeCollector The collector now takes its datastore at construction, so the zero value is no longer usable. * fix(scanner): pin the persist stage to concurrency 1 and guard the collector The stage relied on go-pipeline defaulting to one worker; stating it at the stage makes the constraint visible where someone would change it. The collector takes a mutex too, so the type is safe on its own terms rather than by configuration. --------- Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
Deluan Quintão
|
82fde00ecc |
feat(scrobbler): add per-user scrobble filter (#5964)
* feat(scrobbler): add scrobble_filter column to user * feat(scrobbler): validate scrobble filter criteria on user save * refactor(persistence): make smart playlist join helpers package-level * feat(scrobbler): add MediaFileRepository.MatchesCriteria * feat(scrobbler): filter external scrobbles with per-user criteria * feat(ui): add scrobble filter field to user form * fix(scrobbler): default scrobble_filter to empty string for existing users * refactor(scrobbler): also gate playback reports on the scrobble filter Playback reports carry the same track metadata to plugin scrobblers, so a filtered track leaked through that third dispatch path. Skip the filter evaluation entirely when no scrobbler is active. * refactor(persistence): move criteria join building into criteria_sql.go The join set a criteria needs was decided in criteria_sql.go but built in smart_playlist_repository.go, so both callers had to pair the two by hand. * refactor(persistence): unexport smartPlaylistCriteria methods The type never leaves the package, so the exported names advertised an API that callers outside persistence could never reach. Also disambiguates where/orderBy from squirrel's SelectBuilder methods of the same name. * fix(ui): cap the scrobble filter field width fullWidth stretched it across the whole page next to 256px inputs. Bounded at 40em, with two rows and a resize handle so JSON rules stay readable. * refactor(ui): move scrobble filter input in UserEdit component * feat(ui): add pt-BR translations for the scrobble filter * fix(scrobbler): take the filter verdict before incPlay incPlay mutates play counts and dates a filter can test on, so evaluating at dispatch time let one play decide differently on either side of the increment: a track could be scrobbled despite matching, or lose only its stopped report and strand presence plugins. Reject limit/offset too, rather than silently ignoring part of a rule copied from a smart playlist. * fix(scrobbler): filter the report from an expired session The expiry callback runs with a stub user carrying no filter, so evaluating there always returned false and leaked the track to plugin scrobblers. That is the normal path for clients that never send stopped, such as legacy Subsonic now-playing. Carry the last verdict on the session instead. * refactor(scrobbler): skip the now-playing enqueue instead of threading the verdict Queuing an entry only to drop it at dispatch also cancelled a pending announcement for the previous, unfiltered track, since the queue is keyed by player and a new entry replaces the old one. * fix(scrobbler): evaluate the filter regardless of active scrobblers The verdict is stored on the session and dispatched at expiry, so skipping evaluation when no scrobbler was active let a plugin enabled mid-session receive a filtered track. The empty-filter guard above already gives servers without scrobbling the same free path, so the shortcut only ever applied to users who had a filter set. |
||
|
Deluan Quintão
|
dc40bcaf80 |
feat(cli): add an artwork command group for diagnosing and re-driving artwork (#5957)
* feat(artwork): add a resolution chain trace collector
* feat(artwork): trace the local priority chain
* fix(artwork): record priority candidates the chain never evaluated
* refactor(artwork): report never-evaluated candidates as skipped
* feat(artwork): trace external agents at the gate seam
* feat(artwork): add repository queries to enqueue by current source
* feat(artwork): expose a tracing resolver for the CLI
* feat(artwork): read a single queue row by item
The explain CLI must report whether an item is queued, at what priority and when it
retries; the queue repository could only be drained in eligibility batches, which
cannot see a row that is still backing off.
* feat(cli): add artwork explain
Prints why an item has the artwork it has: the stored state, its queue row, the
governing config, the resolver's priority-chain walk and the verdict. Offline by
default so a diagnostic run cannot add load to an external provider; --live asks
the agents for real. Playlists and radios do not walk a priority chain, so they
report that instead of an empty chain table.
* fix(artwork): trace an external tier that never reaches an agent
A configured 'external' token vanished from the chain when no enabled agent provided
images for that entity type, and for synthetic artists, leaving the trace unable to
say whether the tier was even considered.
* fix(cli): never state an artwork outcome the walk did not observe
A transient external failure traced as 'error' fell through to 'not resolved', which
is the most common state behind a missing-artwork report. It is now indeterminate, and
an offline win that a skipped higher-priority external candidate could have taken says
so instead of naming a winner the live chain might not pick.
* feat(cli): add artwork refresh
* feat(cli): add artwork reprocess
Bulk re-enqueues artwork by kind and/or by the source an item currently
resolves from, previewing the matched count and confirming before queueing.
The preview counts with CountBySource (rows matched) and reports separately
what EnqueueBySource inserted: its DO NOTHING conflict policy leaves an
already-queued row untouched, so the two numbers differ and the output must
not claim the skipped rows were re-queued.
An unknown --source is rejected against the sources present in item_artwork,
rather than silently matching nothing and printing a reassuring 0.
* fix(cli): cover the reprocess selection rule and validate sources table-wide
The reconciliation that makes --source alone target every kind was only
exercised through runReprocess, which no test calls: mutating it to
`all := reprocessAll` left the suite green. It is now reprocessSelectsAll,
covered for all three selectors.
Scoping source validation to the selected kinds made the same well-formed
filter valid or invalid depending on which other kinds were selected, and its
error read the same for a typo as for a source that simply does not apply to
the chosen kind. Validation is now table-wide: a typo still aborts, while a
valid-but-inapplicable source falls through to "Nothing matches".
Also: the prompt now counts only the kinds that reach an external agent as
external cost, and --dry-run on an empty selection reports a dry run.
* fix(cli): cover the reprocess --yes guard and preview the external cost
Mutating the --yes check to `if true` left the suite green, so the one bypass
of the confirmation was unverified. The choice is now reprocessConfirm(yes, in),
covered in both directions.
The external estimate only reached the operator through the prompt, which
--dry-run skips — hiding the number in the one mode that exists to show it
before committing. The preview now carries it, and the prompt drops the clause
when no lookup will be made.
An empty selection says so again under --dry-run.
* feat(artwork): add read-only queue and absent counters
Both are needed by the artwork status CLI: a queue breakdown by kind and priority, and
the absent totals split against the recheck cutoff.
* feat(cli): add artwork status
Reports the queue, where artwork currently resolves from, absent counts against the 24h
recheck window, and the stored config fingerprint versus the current one — the line that
turns 'why is my server re-resolving everything?' into one command.
fingerprint() and staleAbsentAge are exported so the CLI reports the values backfill
itself compares, instead of a second copy of the formula that can silently drift.
* fix(cli): lead the artwork status backfill line with the queued backlog
By the time anyone runs a diagnostic, backfill has usually already stored the new
fingerprint, so 'up to date' was printed while thousands of items churned through external
providers. The backlog is the finding; the fingerprint is context.
Also echoes the config inputs the fingerprint covers, so a change can be traced to the
setting that caused it, and pins the rendered rows: the Absent values, the queue TOTAL and
a queue-scoped kind/priority pair were all unasserted, so kindName and priorityName were
effectively untested. FingerprintInputs is now the single listing ConfigFingerprint hashes;
a pinned hash proves the value did not change.
* refactor(artwork): export the trace outcome vocabulary
The CLI hardcoded the outcome literals and the "external:" prefix, so renaming a
constant's value in core/artwork left cmd compiling and the suite green while
`artwork explain` silently degraded its verdict.
Renaming a value now fails the golden vocabulary test in core/artwork and the
explainResult tests in cmd.
* fix(cli): keep the re-enqueue warning when a backfill is already running
A stale stored fingerprint with items already queued is the worst state the
system can be in: a second full re-enqueue is pending on top of the one running.
The line carried the weakest wording of the three, and was untested.
* refactor(artwork): drop the unreachable breaker branch from the tracing gate
--live wires the tracing gate straight to passthroughGate, so errBreakerOpen can
never reach it; the test only passed by injecting a fake gate.
* refactor(artwork): delete the never-emitted not-reached outcome
Candidates after the winner are lower priority and say nothing about why a source
won; the ones that matter sit above it and are already recorded.
* refactor(artwork): make the trace nil-safe in one place only
add already handles a nil trace, so record's own guard was dead; Steps was the
odd one out and would panic where every other method tolerates nil.
* refactor(artwork): export the trace types directly
ChainTrace and TraceStep were unexported types re-exported through aliases,
which existed only so the CLI had a name to refer to them by. The types are
public API — Resolver.Steps returns []TraceStep and the CLI constructs a
ChainTrace — so name them that way and drop the indirection.
Encapsulation is unchanged: add, mu and steps stay unexported, so only this
package can write a step.
* refactor(cli): simplify parseArtworkKind with slices.Contains
Replaces a nested loop and a manual append with slices.Contains and the
repo's slice.Map helper. Same behaviour, same error message.
* fix(cli): print the absent artwork source under the name --source accepts
`artwork explain` rendered the stored empty source as "(absent)", while
`artwork reprocess --source` only accepts "absent", so pasting what explain
printed straight back into reprocess was rejected as an unknown source.
* refactor(artwork): own the kind list and the chain predicate in the package
Export RecheckKinds and add WalksPriorityChain so the CLI stops keeping its
own copies of both, and unexport externalCandidate, which nothing outside the
package consumes.
* refactor(cli): drop the artwork command's duplicated state and formatting
Reuse artwork.RecheckKinds and artwork.WalksPriorityChain, extract
newTabWriter and externalEstimate, fold reprocessSelectsAll into
selectedKinds, and derive the queue total and the walks-chain flag instead of
carrying them in the report structs.
* test(persistence): drop two artwork-queue specs that cannot fail
One seeded hash and source together and then asserted the two counts agree,
so its setup guaranteed the result; the other repeated the count-does-not-
enqueue property already covered by the CountBySource spec.
* refactor(artwork): rename Resolver to TracingResolver for clarity
* fix(cli): count playlists in the artwork reprocess external estimate
The estimate used WalksPriorityChain, which is true only for artist and album,
so a playlist-only reprocess reported "External lookups: none" and the
confirmation prompt dropped the external-cost warning. Playlists do reach the
network: through the m3u ExternalImageURL fetch when EnableM3UExternalAlbumArt
is on, and — verified by test — through the generated grid, whose tiles resolve
album art via the full album priority chain.
Adds artwork.MayFetchExternal, a config-aware predicate for "can this kind's
resolver reach the network", and uses it for the estimate. WalksPriorityChain
keeps its separate job of deciding whether explain prints a chain block.
* fix(cli): estimate artwork reprocess external lookups per agent, not per item
The reprocess prompt billed one external lookup per externally-capable item.
fetchArtistImage/fetchAlbumImage try every enabled image agent and stop early
only on a hit, and resolvePlaylist can fetch the m3u image and then resolve up
to four sampled albums for the grid, each walking the album agents again. The
number the operator confirmed could understate real provider traffic several
fold, in the prompt whose whole job is to stop a provider flood.
ExternalLookupsPerItem now multiplies by the visible image-agent count and adds
the playlist grid factor. It stays a floor: the CLI never calls Manager.Start(),
so the plugin registry is empty and plugin-provided agents are dropped by
getEnabledAgentNames. On an install with 5 agents of which 3 are plugins the
count is well under the truth, so the wording is now "at least N" rather than
"up to N" — a zero visible count still bills one lookup for the same reason.
Fixing the plugin visibility is out of scope: Manager.Start() needs a Subsonic
router and writes to the DB via syncPlugins, breaking this command group's
read-only guarantee.
* fix(cli): state the artwork reprocess estimate as an estimate, not a bound
Neither bound is true. A ceiling is false because plugin agents are invisible to
a CLI that never starts the plugin manager, and a floor is false because a local
hit ends the walk before any agent is asked and a hit on the first agent skips
the rest. "at least N" traded one wrong claim for another.
The line now names its blind spots instead:
External lookups: ~340 estimated (plugin agents not counted; local hits may
need fewer).
The same line is reused in the confirmation prompt, and the zero case still
reads "External lookups: none." with the prompt dropping the clause entirely.
The count itself is unchanged.
* fix(cli): account for every configured agent in artwork explain
The Agents: line printed the raw config while the Chain only showed the agents the CLI could
construct, with nothing explaining the gap: plugin agents are never registered in a CLI that does
not start the plugin manager, and a built-in without credentials returns nil. Three of five agents
could vanish, including ones ranked above the one shown.
Also treat a live external error before the winning hit like the already-handled would-try case:
the resolver serves such a hit provisionally and retries later, so the verdict is indeterminate.
The Result line is still not qualified when an unavailable agent might have won; that needs agent
ranking, and is left to the follow-up that makes the CLI load plugin agents for real.
* fix(cli): do not call an external artwork win indeterminate
explainResult qualified the verdict whenever an external OutcomeError
appeared before the winning hit. When a later external agent returns an
image, fetchArtistImage/fetchAlbumImage discard the earlier error, so
extError is false: the worker settles the item and schedules no retry.
Telling the operator it may resolve differently on a retry was wrong.
The warning is only correct when a lower-priority local source won while
an external error was recorded, which is the case that carries extError.
* fix(cli): accept --source absent when nothing is currently absent
validateSources checks the requested sources against the ones item_artwork
actually uses, to catch a typo. The reserved empty source (spelled 'absent' on
the CLI) is a valid filter even when it matches nothing, so a scheduled
'artwork reprocess --source absent --yes' stopped working the moment the
library finished resolving. Treat it as intrinsically valid and let the
existing zero-match path report it.
* feat(artwork): explain disc and media file artwork from the CLI
`artwork explain` rejected `dc` and `mf` because it validated against RecheckKinds,
the list of kinds the backfill revisits. Those are different questions: a kind with no
recheck path still has artwork someone can report as wrong.
Disc artwork now walks DiscArtPriority under a trace, so explain reports which entry won
and why the others lost, including entries that map to no source at all (external is
unsupported, a disc with no subtitle, an album folder with no images). Media file artwork
traces its single embedded candidate, separating "EnableMediaFileCoverArt is off" from
"the track has no embedded art" — stored state cannot tell those apart.
Each command now validates against the kinds it can actually serve: explain takes all six,
refresh takes artwork.RefreshableKinds (which nativeapi now shares instead of keeping its
own copy), reprocess still takes RecheckKinds. Disc artwork stays out of refresh: the
worker cannot resolve it, so the queue row would be rejected on every drain.
WalksPriorityChain becomes Explainable, and ResolveArtist/ResolveAlbum collapse into
Resolve(kind, id).
* refactor(artwork): one disc-artwork walk for serving and explain
resolveDisc duplicated the loop selectImageReader already ran: try each source in
priority order, take the first that yields an image. The serving path and the CLI
diverged on two details as a result — only selectImageReader checked ctx between
candidates and logged each attempt.
Both now call discArtworkReader.selectImage, which takes the chainState the CLI already
uses for the other kinds. The serving path passes an untraced one, whose nil trace makes
recording a no-op. selectImageReader had no other caller and is gone.
The disc tests move from fromDiscArtPriority to discCandidates, so they assert the skip
reason for an entry that maps to no source rather than that it silently vanished, and
cancellation mid-walk is now covered.
* fix(artwork): reject a nil reader in the resize cache instead of panicking
resizedItem.Reader closes what open() hands back, so an open() that reports "no image"
as (nil, nil) rather than an error takes the request down with a nil-pointer panic. Every
caller returns an error today, and no test covered it: the resolution e2e harness stubs
the resize reader out entirely, so no e2e path reaches this code at all.
Guard it and cover Reader directly.
* refactor(artwork): move the keeps-state fact into core, drop a redundant guard
keepsArtworkState lived in package cmd and re-derived by hand what RefreshableKinds
already encodes: the same five-of-six kinds. It is now artwork.KeepsState, beside the
list, with a test pinning the two together — nothing else stopped them drifting, and a
drift would have explain report stored state for a kind that keeps none.
serveDisc's closure also hand-rolled a nil-reader error that both consumers of open()
now produce themselves: serveSource for a full-size request, resizedItem.Reader for a
resized one.
* fix(artwork): route disc candidates through the shared resolvers
openCandidate ran its own source loop and threw the error away, so a disc track that
exists but cannot be parsed traced as "miss" — indistinguishable from a track with no
embedded art. fromTag and fromFFmpegTag already report that case as errSourceUnreadable;
only this loop was discarding it. Telling those two apart is what the trace is for.
Candidates now carry a resolve func instead of raw sources: embedded goes to
resolveEmbedded, and the folder-backed entries to resolveFolderSource, extracted from
resolveFolderFile so both callers classify an unopenable file the same way. openCandidate
and its absolute-path special case go away with it.
Disc's own fromExternalFile and fromDiscSubtitle still swallow open errors, so folder
candidates cannot report unreadable yet; that is a change to their error contracts.
* fix(artwork): report an unreadable local candidate as indeterminate
processor.acquire treats resolution.localError exactly as it treats extError: a fault is
not a definitive "no image", so it retries instead of settling absent. explainResult
qualified only the external case, so a chain that ended on an unreadable local candidate
printed "not resolved" — the one verdict that says the walk was conclusive.
The qualification belongs only to the unresolved branch. chainState.try stamps extErr onto
a hit and deliberately drops localErr, so an unreadable step followed by a hit is settled
as found and must not carry a warning; a test pins that.
Found by Codex on
|
||
|
Deluan Quintão
|
6c3e7e268b |
feat(instant-mix): support album, playlist and genre sources (#5948)
* feat(agents): local agent genre-hint similar songs fallback
* feat(external): playlist instant mix via seed-track sampling
* test(external): cover playlist mix never-empty fallback and maxSeeds cap
Adds coverage for the empty-match seed fallback and the maxSeeds
call cap on GetSimilarSongsByTrack, per code review finding.
* feat(external): genre instant mix via seed-track sampling
* feat(external): album instant mix falls back to AudioMuse track similarity
* feat(external): artist instant mix falls back to seed-track sampling
* fix(jellyfin): route genre seeds through instant mix instead of empty
* feat(jellyfin): add /Albums/{id}/Similar route for albumMix radio
* perf(external): bound playlist seed sampling to a random N
samplePlaylistTracks loaded an entire playlist's joined rows just to keep
5 random seeds; push the bound and randomization into the query instead,
matching the other samplers (GetRandom/GetAllByTags with Max).
Fixing this surfaced a real bug: resetSeededRandom's SEEDEDRAND rewrite
assumed every table's id is TEXT, but playlist_tracks.id is an INTEGER
position, so the random sort silently dropped every row. Cast the id to
TEXT before hashing (no-op for the other, TEXT-id tables).
Also trims a changelog-flavored comment and a duplicated rationale in
server/jellyfin/similar_test.go.
* refactor(external): parallelize seed mix and dedup mix helpers
Run the up-to-5 per-seed GetSimilarSongsByTrack calls concurrently (errgroup),
route the four container cases through a shared seedMix helper, flatten the
genre lookup, and sample playlist seeds without forcing a smart-playlist
rebuild. Share the media-file->Song mapping in the local agent.
* perf(agents): use the indexed genre filter for local similarity
Replace GetAllByTags (a json_tree scan of every media_file row) with the
media_file_tags semi-join from #5940, deriving the seed's genre tag ids
locally since they hash from (name, value).
Also carry the library id and the recording MBID on the returned songs:
the matcher resolves by id first and looks up mbz_recording_id, so the
release-track id it got before matched nothing and the local fallback
silently returned no songs.
* refactor: drop redundant MBID and fold mixFromSeeds into seedMix
The local agent returns library tracks, so the id alone resolves them in the
matcher's first phase; the MBID was never consulted. mixFromSeeds had no
caller other than seedMix.
* docs: trim redundant comments
* fix(jellyfin): adopt the GUID id codec in the merged similar routes
getSimilarAlbums still used resolveItemID/DecodeID, which #5942 replaced with
itemIDParam; its tests passed raw ids that the strict codec now rejects.
* fix(external): guard non-positive counts and blend every seed
A negative Subsonic count reached matched[:count] and panicked. The matcher
also keeps input order and stops at count, so seed-grouped results let the
first seed fill the whole mix; interleaving gives every seed a share.
Drops the duplicate playlist-track mock in favour of tests.MockPlaylistTrackRepo,
which pages like the real repository and records the query options.
* fix(external): refresh smart playlists before sampling seeds
A smart playlist materializes no playlist_tracks until it is evaluated, so
sampling without the refresh mixed an empty seed set. The refresh is a no-op
for regular playlists, inside the refresh delay, and for non-owners.
* fix(external): skip missing tracks and a nil playlist-track repo when sampling
Tracks() logs and returns a nil repository when its own lookup fails, so the
chained GetAll panicked. Seeds can also reach the mix verbatim when the agents
find nothing, so a missing file would surface as an unplayable entry.
* fix(jellyfin): never report the seed album as its own similar album
The sampled-seed fallback returns the album's own tracks, which similarAlbums
mapped straight back to the requested album, often as the only result.
* test(agents): assert the genre predicate instead of relying on the mock
MockMediaFileRepo ignores QueryOptions.Filters, so the spec passed even with
no genre filter at all. It now checks the generated predicate carries the
seed's own tag id, the indexed join and the missing exclusion.
* fix(external): clamp the requested count before it becomes a query limit
Subsonic passes the client's count through unbounded. At MaxInt64 the local
agent's count+1 overflows negative, and GetRandom omits the SQL limit unless
Max is positive, so one request would hydrate every matching track. 500 is
what the widest caller (similarAlbums, limit*5) legitimately asks for.
* fix(external): deduplicate playlist seeds by media file
A playlist can hold the same file at several positions, so sampling its rows
could seed the mix twice: a wasted agent call, and a duplicate track whenever
the seed fallback kicks in.
* fix(external): drop tracks two seeds both recommend
The matcher re-emits a track when two inputs are identical, so overlapping
recommendations took several slots in the mix. Match the whole merged set and
dedup before trimming. Playlist sampling now over-fetches before its own
dedup, so repeated positions cannot collapse the seed count.
* test(external): make the seed-blend assertion independent of the shuffle
It matched four tracks and kept two at random, so both could come from the
first seed once in six runs. Keeping three of the four makes a seed-two track
unavoidable.
* fix(external): seed artist mixes from every credited role
media_file.artist_id is the deprecated primary artist, so an artist credited
only on the album, as on compilations, sampled no seeds at all. Use the same
participant filter the artist listings use.
* refactor(external): drop the now-vestigial seed interleaving
Matching the whole merged set removed the early truncation the interleave
guarded against, and the shuffle before the trim makes input order irrelevant.
Its comment described the old behaviour.
* test(agents): give the id-mapping fixture a matching genre
The related track carried no genre, so the real query would never return it;
the spec only passed because the mock ignores QueryOptions.Filters.
* test(agents): drop the MBID from the id-mapping fixture
Local agent candidates are non-missing library rows, so the matcher always
resolves them in its id phase and never reads the MBID. The field guarded a
regression that could not change behaviour.
* test(agents): remove unnecessary comment about MBID in GetArtistTopSongs test
* fix(jellyfin): only let a not-found entity fall through in getInstantMix
Discarding the error conflated a genre id, which never resolves, with a real
lookup failure, which then made a provider call that fails the same way.
* test: pin the invariants the specs only appeared to cover
The missing filter was asserted by substring, so flipping it to true passed
everywhere, including the spec named for it. Matching the whole merged set,
the local agent's over-fetch, and its no-genres early return had no coverage
at all; each is now pinned by a spec that fails when the code is broken.
* test: make the remaining specs say what they actually guard
The playlist-track spec named a sort whitelist it does not exercise; it guards
the integer-id CAST, so it now asserts no rows are dropped. The maxSeeds cap
passed with either bound removed, and the over-fetch was pinned by its literal
value rather than the duplicate positions it exists for. Also drops setup the
count guard returns before reaching.
* fix(external): fall back when the agent's picks are not in this library
A non-empty answer whose songs are all absent locally matched nothing and was
returned as-is, so the mix came back empty with sampleable source tracks
sitting right there.
* refactor(external): name the agent-then-fallback flow once
Each entity case repeated the same error and emptiness plumbing around the
matcher. mixFromAgent states it once and each case supplies only what differs:
how to ask, and what to do when the answer is unusable.
|
||
|
Deluan Quintão
|
752b38609c |
feat(artist): add Share and Download actions to the Artist detail page (#5944)
* feat(ui): add Share button to artist detail page * feat(ui): add Download button to artist detail page * fix(ui): scope artist share/download to album-artist content Gate the artist Share/Download actions on album-artist stats and show the album-artist size, since ZipArtist and the share query only cover album_artist_id songs. Previously the total (role-inclusive) size was shown and guest-only artists could produce an empty archive. Applies to the artist toolbar, the shared context menu, and the download dialog title. * fix: match artist download/share to album-artist participation ZipArtist and the artist share query filtered the deprecated album_artist_id column, which only stores the first album artist of a track. Secondary album-artists (co-credited but not first) got an empty download/share even though the UI offered it. Filter by the album-artist role participation instead, matching the artist's album-artist stats used to gate the actions. Also cover the artist-specific size branch of the download dialog. * fix(share): scope artist shares to the owner's libraries The artist share query broadened to album-artist participation, which could pull a secondary album artist's tracks from libraries the (non-admin) share owner cannot access into the public share. Load the artist share as the owner so their library access is applied, mirroring how playlist shares already work. Adds a repository test covering co-album-artist inclusion and library scoping. * test(share): assert album participation branch of artist shares Link the co-album-artist fixtures to albums and assert share.Albums (used by Subsonic getShares) includes the accessible album and excludes the one in a library the owner cannot access, so the album participation + scoping branch is covered too. * fix: exclude missing files from artist download/share actions An artist's stats still count files that went missing, so the toolbar/context menu could offer Download/Share for an artist whose files are all gone, while the share query (missing=false) returns nothing and downloads open dead paths. Hide the actions when the artist is missing and exclude missing files from ZipArtist, matching the share semantics. * refactor: dedupe artist download-size and share-owner lookups Extract the 'album-artist download size (or none when missing)' rule into a single artistDownloadSize() helper shared by the toolbar, context menu, and download dialog, and factor the duplicated share-owner context lookup into a shareRepository.ownerContext() method used by both the artist and playlist share cases. * refactor(ui): move artistDownloadSize helper to common utils is for domain-agnostic, potentially portable code; this helper is Navidrome-specific (artist stats shape), so it belongs in common. Consumers import it directly from common/artist to avoid pulling in the common barrel. |
||
|
Deluan Quintão
|
95b8d9dd04 |
perf(genre): index genre filtering via join tables across all APIs (#5940)
Filtering by genre scanned every media_file/album row and JSON-parsed its `tags` column (a per-row json_tree(tags) EXISTS) with no usable index, so Finamp's genre screen took 1.9-6.5s per tap against a ~97k-track library. Album and album-artist genre queries had the same unindexed shape. Add normalized media_file_tags and album_tags join tables (genre only for now, via an indexedTagNames allowlist), populated by a new updateTags in Put (mirroring updateParticipants) and backfilled in the migration. Genre filtering across the Jellyfin, Subsonic and native APIs now runs as an index-backed semi-join through shared TagIDSemiJoin/TagNameSemiJoin helpers instead of a full scan. On a copy of the production DB the per-request cost for a typical genre drops from ~330ms to sub-millisecond, adding ~10MB. |
||
|
Deluan
|
8e0ff1a235 |
fix(jellyfin): resolve genre id as a MusicGenre item
The Jellyfin /Items/{id} endpoint resolved albums, artists, songs and
playlists by id but not genres, so a genre id returned 404. Finamp's
genre "See all" fetches the genre as the track list's parent item, and
that 404 crashed its screen to a blank page after the tracks flashed in.
Add a Genre lookup to resolveItemByID (backed by a new GenreRepository.Get)
so a genre id returns its MusicGenre BaseItemDto, matching real Jellyfin.
|