* fix(subsonic): honor DefaultDownloadableShare in createShare
The DefaultDownloadableShare option was only sent to the web UI, which used
it to pre-tick the "Allow Downloads?" checkbox. The Subsonic createShare
handler built the model.Share without touching Downloadable, so it fell back
to the Go zero value and every share created through the API was stored as
non-downloadable, regardless of the configured default.
createShare now reads an optional downloadable parameter and falls back to
conf.Server.DefaultDownloadableShare when the client omits it, matching the
web UI. Fixes#6119.
updateShare had a related problem: core's share repository wrapper always
writes the downloadable column, but the handler never set the field, so any
updateShare call silently reset the share to non-downloadable. It now loads
the current share and uses its value as the fallback.
* refactor(subsonic): trim the share downloadable lookup and align with the UI
updateShare fetched the share with Get to recover the stored downloadable
flag, which also runs loadMedia and materializes every album and track the
share points at, just to read one boolean. It now uses Read, which skips
loadMedia, and only queries at all when the client omitted the parameter.
createShare now ANDs the default with EnableDownloads, matching what the web
UI already computes, so both paths apply the same rule.
The specs collapse the create-path matrix into a DescribeTable, reuse the
existing albumIDByName helper, and set the request-time config after
setupTestDB so it does not leak into the config snapshot.
* fix(subsonic): keep the share description on a downloadable-only update
updateShare read the description straight from the request, so a client that
sent only id and downloadable got an empty string written over the stored
description. shareRepositoryWrapper.Update always writes that column, so the
description was silently erased.
This predates the downloadable parameter added earlier in this branch: any
updateShare that omitted description already cleared it. Adding the parameter
just made it easy to hit, since toggling downloads is a natural reason to call
updateShare without touching the description.
Both fields now use the presence-aware accessors and fall back to the stored
share, which still costs at most one read and none when the client sends both.
An explicitly empty description still clears the field.
The theme rounded the cover image directly and set a border radius on
albumContainer, which has no background or clipping, so it rounded
nothing. The hover overlay is a sibling of the image inside the same
link, so it kept square corners that poked out over the rounded cover.
Move the radius to that link and clip it, so both the image and the
overlay follow the same rounded box. This also covers the mobile bar,
which is always visible.
Fixes#6110
The comment justified anonymous access with "item ids are unguessable".
That is not true: an artist id is a deterministic, unsalted hash of the
artist name, id.NewHash(id.NewHash(str.Clear(lower(name)))), so it is
computable offline by anyone who knows the name.
The real reason the route is public is that upstream Jellyfin's is too.
ImageController.GetItemImage carries no [Authorize] attribute (verified on
v12.0, master/13.0.0, v10.11.9 and v10.10.7), and an anonymous request
reaches LibraryManager.ItemIsVisible with a null user, which returns true
unconditionally. Clients build cover URLs with no credentials at all, so
requiring auth here would break them.
No behavior change.
* added catppuccin mocha theme
* added catppuccin mocha theme to index.js
* fix syntax
* add catppuccin frappé theme
* made frappe and mocha themes more consistant with official color palette and added comments for easy verification
* same for macchiato, seperate commit in case original is preferred
* added comments to .js files
---------
Co-authored-by: Deluan Quintão <deluan@navidrome.org>
* docs: point jftui client link to canonical repository
* docs: fix relative path to webhook-rs example in nd-pdk-host README
* docs: fix capability schema paths in plugin examples README
---------
Co-authored-by: Deluan Quintão <deluan@navidrome.org>
The spec asserted on artistID("Kraftwerk") and intermittently found zero artists
on Windows. The scan did import the artist; it was then made invisible.
RefreshStats selects touched artists with a strict artist.updated_at >
library.last_scan_at (persistence/artist_repository.go:466). Windows' wall clock
has ~15ms granularity, so a new artist written by a quick scan can land in the
same tick as the previous scan's last_scan_at and be excluded. Its
library_artist.stats then stays at the '{}' default and the unscoped cleanup
DELETE removes the row, after which selectArtist's INNER JOIN on library_artist
hides the artist from GetAll.
Backdate last_scan_at before the scan so the comparison is unambiguous, matching
the fix already applied to the search_normalized spec below it.
* fix(i18n): Update Japanese translation
* fix(i18n): fix Japanese translation
* fix(i18n): fix Japanese translation
Update Japanese translations for `recentlyAdded`, `recentlyPlayed`, and `mostPlayed` in album lists
---------
Co-authored-by: Deluan Quintão <deluan@navidrome.org>
Jellify's Discover tab calls GET /Items/Latest and got a 404: we only routed the
/Users/{userId}/Items/Latest form, which real Jellyfin marks [Obsolete] and hides
from its OpenAPI spec, so SDK-generated clients never see it. Add the current
route alongside the legacy one, both served by the same handler.
getLatest also ignored ParentId, so browsing a library or an artist returned the
newest albums across everything the user can see. It now scopes to the library
when ParentId names one, and filters to that artist's albums otherwise, which
also makes a stale id return nothing instead of silently widening back to the
full library set. A malformed ParentId 404s, matching /Items and the contract
decodeFilterParam documents.
PlaybackInfo returned a TranscodingUrl prefixed with the /jellyfin mount path.
Clients concatenate that value onto a server base URL that already carries the
prefix, producing /jellyfin/jellyfin/Audio/{id}/universal and a 404, so playback
never started. Jellify, jellyfin-web, jellyfin-vue and Streamyfin all consume the
field this way; real Jellyfin emits it server-relative (StreamInfo.ToUrl is called
with a nil baseUrl).
Emit the path server-relative to match. Finamp is unaffected: it builds its own
stream URLs and never reads the field.
`buildPlaylist` reported `evaluated_at` as `changed` for smart playlists, so a
rename or comment edit was invisible to clients until the next evaluation. A
never-evaluated smart playlist also reported the current time on every call,
which never settled.
Report `updated_at` for every playlist. `refreshCounters` now syncs the stamp it
writes back onto the model, and `refreshSmartPlaylist` reuses it for
`evaluated_at`. `changed` therefore still equals the evaluation time for a
just-evaluated playlist, and `validUntil` stays anchored to it.
Original Subsonic always bumps `changed` on any playlist update, so this also
aligns the behavior with upstream.
The Refresh Metadata action was only reachable from the Album and Artist context menus, so it could not be triggered from AlbumShow or ArtistShow. This adds an icon-only button, with a tooltip, to the action toolbar on both detail pages. Like the menu entry, it is only rendered for admins.
The button is built on react-admin's Button rather than a plain IconButton: the surrounding toolbars use the former, so the theme colour and the icon-only swap at the xs breakpoint are inherited instead of restated. The dataProvider call and its two notifications move into a new useRefreshMetadata hook, which ContextMenus now shares, keeping a single copy of that logic.
A playlistTrack id is a position in the playlist, not a stable key, so refetching
a row by id after the annotation is saved can return a different song: in a smart
playlist filtered on that annotation the track is gone and every later row has
shifted up. The stale-keyed record then renders as a duplicate of its neighbour.
Signed-off-by: Bjørn A. Andersen <polybjorn@users.noreply.github.com>
Co-authored-by: Bjørn A. Andersen <polybjorn@users.noreply.github.com>
Co-authored-by: Deluan Quintão <deluan@navidrome.org>
The exclusion only worked on master. Coverage profiles name files by import
path; octocov shortens those to repo-relative paths using the checked-out
source, but coverage-on-pr.yml sparse-checks-out only .octocov.yml, so the
paths stay as github.com/navidrome/navidrome/tests/mock_*.go and 'tests/**'
never matched. '**/*_gen.go' matched either way, which is why only the 30
tests/ files leaked.
Every pull request since b77fb45 therefore reported ~-3.7% against master:
447 files on the base side, 477 on the pull request side (#6002, #6069).
* chore: ensure that all durations are nonnegative
* make sure you actually include the test file
* test(conf): use non-zero durations in the valid_duration fixture
Zero is the boundary between the accepted and rejected ranges, so it
passes even if the guard is off by one. 1s exercises an ordinary value.
---------
Co-authored-by: Deluan <deluan@navidrome.org>
An absent artwork state was revisited by an hourly job, by viewing the entity, and
by the startup backfill on any artwork config change. On a large library the last
one queued tens of thousands of external lookups at once and got the provider to
rate-limit us for hours.
Nothing revisits an absent state now. Retrying is explicit: `artwork reprocess` on
the CLI, or the refresh button in the UI. The config fingerprint survives only as an
advisory, warning at startup and naming the command that clears it.
Since absent is terminal, `artwork status` splits it into two disjoint columns, and
`--source failed` targets only the ones that gave up rather than being answered.
Both read through the filter CountBySource and EnqueueBySource already share, so the
reported number is the set the command acts on.
Also fixes the last_failure default left by 20260819204637, which marked every
pre-existing absent row as failed, and removes the code the deleted retry paths
orphaned.
The coverage profile counted the tests/ package and the *_gen.go files, none of
which are code under test: tests/ is the mock and helper package, and generated
code is never hand-tested. Together they added 2926 uncounted statements at 0%,
pulling the reported number down by almost 6 points (70.13% -> 75.98% on the
current master profile).
octocov's coverage.exclude takes doublestar globs matched against git-root-relative
paths. All 26 mock_*.go files live under tests/, so the single 'tests/**' pattern
covers them.
* fix(deezer): treat an exhausted quota as a throttle, not as a missing artist
Deezer reports quota exhaustion in the response body, with HTTP 200 and no
rate-limit headers. The client only looked for errors when the status was
not 200, so a throttled reply was decoded into an empty result type, and an
empty search became ErrNotFound. The agent then compounded it: it tested
`errors.Is(err, ErrNotFound) || len(artists) == 0` before testing err, so
any failed search — which also returns no artists — reported not-found too.
The artwork worker settles an entity as "no image" on agents.ErrNotFound.
So being throttled did not make Navidrome back off; it made it record the
artist as having no artwork, and move on to do the same to the next one.
Errors are now parsed out of the body regardless of status, and the quota
code is joined with agents.RetryLaterError so the circuit breaker and the
artwork retry budget see a throttle for what it is. The agent checks err
before the empty-result case.
Last.fm already handles this exact shape (client.go errCodeRateLimit, with
a comment noting the 200-with-body-error pattern); this brings Deezer in
line with it, including the zero-delay RetryLaterError so both providers
share the default cooldown rather than a per-provider number.
Measured against the live API to pin the shape: a 120-request burst
returned 54 results and 66 quota replies, every one of them HTTP 200 with
{"error":{"type":"Exception","message":"Quota limit exceeded","code":4}}
and no Retry-After or rate-limit headers. A single request 5s later
succeeded, so the window is short and a cooldown fully clears it.
* refactor(deezer): fold the error envelope into one type
The envelope declared the code and message inline, parseBodyError copied
them field by field into a second struct with the same shape, and a zero
Code stood in for "no error reported". Making the envelope hold a pointer
to the error type removes all three: absent is nil, present is the error
itself, and the value returned needs no conversion.
searchArtist loses its empty-result branch. searchArtists converts an
empty result to errNotFound and returns early on any error, so it never
answers with no artists and no error, and the branch could not run. What
it left behind was a comment explaining an ordering that only mattered
while the branch existed.
ErrNotFound is unexported: nothing outside this package referenced it,
and it sat three lines from agents.ErrNotFound, which is a different
error with the opposite meaning for callers.
Throttling now joins agents.ErrRetryLater, the sentinel documented as the
zero-delay RetryLaterError, rather than allocating an equivalent value.
* refactor(deezer): return agents.ErrNotFound from the client
The client raised a package-local sentinel that the agent then translated
into agents.ErrNotFound, one call site each. Deezer was the only adapter
carrying its own: last.fm and listenbrainz have none.
The client already reports throttling with agents.ErrRetryLater, so it
already speaks the agent vocabulary; saying "not found" in the same words
costs nothing and lets searchArtist drop to plain error propagation.
* test(scrobbler): remove a race in the longest-server-delay test
newBufferedScrobbler starts its drain goroutine, and run() drains once
before it ever waits on the wake signal. The test enqueued user2, then
enqueued user1 via Scrobble, so that startup drain could land between
the two: it saw only user2, took its 45s delay, and set backingOff. The
wake from the second enqueue is then deliberately ignored — a wake
during a backoff window must not drain, which is the hammering the
window exists to prevent — so user1 was never attempted and the first
assertion read 1 instead of 2.
Buffering both users before the goroutine exists removes the window.
The test no longer goes through Scrobble, which the sibling tests
already cover; what this one is about is which delay wins.
Reproduced deterministically by forcing the interleaving with a
synctest.Wait between the two enqueues, which fails with the same
"expected both users drained, got 1 attempts" seen in CI. With both
enqueued first, that same forced drain passes.
An explicit RetryLaterError now opens the agent's breaker immediately for the
provider's own delay, instead of counting it as one generic failure that needs
five to open and then always probes after a fixed minute.
* ci: comment coverage on pull requests from forks
A pull_request run from a fork gets a read-only GITHUB_TOKEN, so octocov could not post its comment: it logged a 403 and exited 0, leaving the job green and the PR silent. The 'permissions:' block cannot grant what the token does not have.
The comment now comes from a workflow_run workflow, which runs on the base repository and does get a write token. The pipeline job keeps the job summary and the default-branch baseline, and hands the merged profile and the PR number to it as an artifact.
A workflow_run job otherwise looks like a push to the default branch, so octocov is pointed back at the pull request and at the run that produced the profile via its OCTOCOV_ environment overrides. Without the run id override the test execution time would be read from the wrong run; without the ref override a fork's coverage would be stored as the master baseline.
The job holds a write token, so it reads .octocov.yml from the base branch rather than from the fork.
* ci: stop checking out the fork in the coverage comment workflow
CodeQL flagged the pull request checkout as untrusted code in a privileged context (actions/untrusted-checkout/high): the job holds a write token. The checkout existed only so the code-to-test ratio would reflect the pull request, which does not justify the alert.
The workflow now checks out just .octocov.yml from the base branch, and the ratio is skipped when reporting from there. Coverage and its delta against master, the metrics that motivated the report, are unaffected: they come from the profile the pipeline uploads.
* ci: treat the coverage artifact as untrusted input
A pull_request run executes the fork's own copy of pipeline.yml, so every file in the octocov-pr artifact is attacker-controlled. The artifact was extracted into the workspace root, on top of the base-branch checkout, and download-artifact truncates existing files. A fork could therefore replace .octocov.yml before octocov loaded it.
That is not only a config swap. config.Load expands ${VAR} from the job environment and the action sets OCTOCOV_GITHUB_TOKEN, so a crafted comment.message posts the privileged job's token into a public comment; a body: section rewrites a pull request description, which pull-requests: write allows.
The artifact now lands in a subdirectory and only coverage.out is copied out, after pr_number is checked to be digits and the named pull request's head is confirmed to be the sha that triggered this run. Without that check the artifact could aim the comment at any open pull request, and unvalidated content reached GITHUB_OUTPUT.
* ci: report Go test coverage on pull requests
Adds octocov to the existing 'Test Go code' job. It reads the coverage
profile, posts a PR comment with the coverage percentage and the delta
against master, and writes the same report to the job summary.
The master-branch report is stored as a GitHub Actions artifact, so no
external service or secret is needed.
* ci: merge the plugins job coverage into the same report
The plugins suite runs in its own job, so its coverage was missing from
the report. Both jobs now upload their profile as an artifact and a new
'Report coverage' job merges them into a single PR comment.
* ci: update the coverage comment in place instead of reposting
octocov's default is to collapse the previous comment and create a new
one. updatePrevious edits the existing comment instead, so a PR keeps a
single coverage comment across pushes.
* ci: fix octocov timeout and step-time lookup
Storing the report hit the 30s default timeout: scanning this repo's
artifacts for the baseline consumed it first. Raise it to 5m.
The step-time lookup also matched the Windows job's 'Test' step and
waited for a job that was still running, so execution time was dropped
from the report. Rename the step to make it unique.
* ci: only store the coverage baseline from the default branch
* ci: report statement coverage instead of line coverage
octocov reports statement coverage for a single profile but switches to
line counting when it merges several itself, which made the number
disagree with 'go tool cover -func'. Merge the two job profiles into one
file first, so the reported number matches what developers see locally.
* ci: stop the download-link comment from clobbering the coverage report
Both comments are posted by github-actions[bot], and the download-link
job updated the first bot comment it found. On a new PR the coverage
comment is created first, so it would be overwritten. Match on the body
as well, and keep the coverage profiles out of the download list.
* test(plugins): build test plugins in Go instead of shelling out to make
The plugins suite built its .ndp test packages by running `make -C
plugins/testdata`, which needs make and zip on the PATH. That is the reason
the 26 WASM-dependent spec files are tagged //go:build !windows.
buildTestPlugins now does the same work in Go: the same mtime check make
performed, `GOOS=wasip1 GOARCH=wasm go build` per plugin, and archive/zip
for the package. TinyGo was already optional and unused in CI, so nothing is
lost there. The first plugin builds on its own so the shared wasip1 stdlib
and PDK objects land in the build cache before the rest fan out: on a cold
cache that is 2.2s against 3.4s for the sequential make and 7.3s for an
unrestrained fan-out.
Packaging moved into a writeNdp helper shared with createTestPackage, which
was already writing the same two-entry archive. Entries are written in a
fixed order, so the .ndp bytes are now reproducible; the loader hashes those
bytes, and `zip` also stored file mtimes, so the previous packages differed
on every rebuild.
The Makefile is unchanged and still works for building the plugins by hand.
Removing the !windows tags is a separate step, once CI is green here.
* test(plugins): run the WASM plugin specs on Windows
With the test plugins now built in Go, nothing in the suite needs a Unix
toolchain, so the //go:build !windows tags come off all 25 spec files. The
Windows CI job runs `go test ./...`, so it picks the suite up with no
workflow change.
plugins_suite_windows_test.go existed only to bootstrap the handful of specs
that compiled on Windows; plugins_suite_test.go now serves both.
* test(plugins): skip the planted-symlink spec where symlinks need privileges
os.Symlink needs an elevated token or Developer Mode on Windows, so the
unconditional Expect(...).To(Succeed()) would fail for contributors running
the suite on an ordinary Windows box. The elevated GitHub runner hides this.
The equivalent spec in sandbox_fs_internal_test.go already attempts the
symlink and skips on error; this does the same, keeping the pin live
everywhere it can run, including Windows CI.
* fix(ci): stop the Windows ndpgen test failing silently
The ndpgen suite builds its helper binary to %TEMP%\ndpgen-test, and Windows
will not exec a file without an executable extension, so the "supports
verbose mode" spec has been failing there. Nobody noticed because the
Test ndpgen step ran under pwsh, which carries on after a non-zero exit and
takes the step's status from the last command, so the job stayed green with
a FAIL line in its log.
Add the .exe suffix, and run the step under bash like the Linux job does, so
a failure in any of its three commands fails the job.
* fix(plugins): build public URLs on the caller's address instead of localhost
The artwork host service had no `*http.Request`, so it passed `nil` to
`publicurl.ImageURL`. With neither `ShareURL` nor `BaseURL` configured, that
produced `http://localhost/share/img/...`, which is useless to anything outside
the server. The Discord Rich Presence plugin explicitly drops localhost URLs, so
it fell back to the Navidrome logo instead of the real cover art.
`serverAddressMiddleware` already works out the client-facing scheme and host
from the `X-Forwarded-*` headers. It now also records them in the request
context, and `publicurl` takes a `context.Context` instead of an `*http.Request`
so any caller can reach them. Extism passes the caller's context through to host
functions, so plugins invoked during a request now get a reachable URL with no
configuration.
Switching the parameter also removes the need for a second, parallel entry
point: the package previously wanted only a scheme, a host, and a context, and
took a whole request to get them. `AbsoluteURL` no longer dereferences a
possibly-nil request on its parse-error path.
Plugin calls that start from `context.Background()` (scheduler and websocket
callbacks, the buffered scrobble drain) still fall back to localhost, since they
have no request to learn from. A debug log now points at `ShareURL` when that
happens.
* fix(publicurl): include the configured port in the localhost fallback
The last-resort fallback built `http://localhost/...`, which points at port 80
and so is unreachable for a server listening anywhere else — the default 4533
included. Use `conf.Server.Port` so a consumer on the same machine can actually
fetch the URL.
* fix(publicurl): use https in the localhost fallback when TLS is configured
The fallback hardcoded the http scheme, so a TLS-only server with no BaseURL
advertised a URL it does not answer on. Mirror the server's own switch, which
requires both a certificate and a key.
* refactor(publicurl): tidy the localhost fallback and its tests
Use gg.If for the fallback scheme so it reads as an expression, like the
BaseScheme branch above it, instead of assigning http and overwriting it.
Drop two tests the ctx refactor left redundant: one asserted PublicURL "works
without a request" but became a byte-identical copy of the ShareURL spec once
the *http.Request parameter went away, and the two port specs differed only in
the integer, where the non-default port is the stronger assertion.
* refactor(conf): add TLSEnabled and use it instead of repeating the predicate
Whether the server speaks HTTPS was decided inline in three unconnected
places. This PR added the third, in a URL-building package that has no
business inferring the transport config.
Move the rule to conf, next to the fields it derives from, and call it from
publicurl and the insights collector. server.Run keeps its own expression: it
takes the certificate and key as parameters, and its test passes values that
do not come from the config.
* fix(nativeapi): stop partial PUTs from clearing untouched columns
The REST layer parses the request body's top-level JSON keys and passes them
to Repository.Update as colsToUpdate. The radio and library repositories
discarded that list and issued a full-row UPDATE, so any field absent from
the body was written as its zero value.
For radio this wiped uploaded_image, deleting the station's cover on every
partial update (the Web UI is unaffected because its form submits the whole
record). For library it silently cleared remote_path and default_new_users.
Thread the column list through to Put in both repositories, and extract the
column-selection half of filterUpdateValues into selectUpdateColumns so
library, which hand-builds its update map, shares the same rule instead of
copying it.
Fixes#6057
* refactor(persistence): drop pluginRepository's dead rest.Persistable methods
Save and Update had no callers: PUT /api/plugin/{id} is served by the
hand-written updatePlugin handler over a typed request struct, and the
route only wires rest.GetAll and rest.Get. Both methods delegated to Put,
which upserts all twelve columns, so wiring rest.Put to this repository
would have reintroduced the partial-update clobbering fixed in the previous
commit. Removing them, along with the rest.Persistable assertion, makes
that a compile error instead of a silent data loss.
Put itself is unchanged and still backs plugin discovery.
* perf(persistence): avoid a full media_file scan when resolving playlist paths
FindByPaths built one OR-ed equality term per path. On the real media_file
schema SQLite abandons the path index at just two OR-ed terms and falls back to
SCAN media_file, re-testing every term against every row, so the cost grows with
(rows x terms).
Group the candidates by library and emit one IN list per library instead, which
plans as SEARCH media_file USING INDEX media_file_path_nocase. The NOCASE
collation is kept so ASCII case-insensitive matching still works.
This is the dominant cost of M3U playlist import, which resolves every track on
every scan. Measured with a 1000-track playlist against a migrated DB:
100k media_file rows: 397 -> 51,414 tracks/sec
500k media_file rows: 78.5 -> 47,174 tracks/sec
The rate no longer degrades as the table grows, which is the expected shape for
an index lookup. Reported in #6043, where an 8 hour scan of a 2M-song library
spent 7h52m in the playlist phase.
* docs(playlists): correct the stale reason for the M3U lookup chunk size
The expression-tree depth ceiling applied to the old OR-per-path query, which
capped a batch at roughly 500 terms. The IN form is bound by SQLite's 32766
variable limit instead, which the 400 candidates per chunk sit far below.
* fix(plugins): stop the cache janitor when a plugin cache is dropped
newCacheService started a ttlcache janitor goroutine that only stopped via the
explicit Close() path, so a cache service that was discarded without being closed
leaked its janitor for the process lifetime. It now registers the same
runtime.AddCleanup safety net that utils/cache.simpleCache already uses.
* fix(scanner): stop splitting multi-byte characters when truncating tags
sanitize() capped tag values with a byte slice, so a value whose limit falls in
the middle of a multi-byte character was stored as invalid UTF-8. defaultMaxTagLength
is 1024, which is not a multiple of 3, so any sufficiently long CJK title hit this.
Only trailing invalid bytes are trimmed, leaving bad bytes elsewhere in the value
untouched.
* fix(scanner): store MusicBrainz ids in their canonical form
uuid.Parse accepts a UUID wrapped in any two bytes, as well as braced and urn:
forms, but sanitize() returned the raw string. A tag like {<mbid>} or a quoted
value was therefore persisted with its wrapper into the mbz_* columns, where the
exact-match MBID search can never find it. The parsed value is now stored, which
also lowercases uppercase ids and adds the dashes to unhyphenated ones.
* fix(plugins): parse IPv6 hosts correctly in the websocket allowlist
isHostAllowed cut the host at the last colon, which mangles an IPv6 literal:
"[::1]:8080" became "[::1]" and "[::1]" became "[:". A plugin manifest could
therefore never allow an IPv6 host. It now uses net.SplitHostPort, falling back to
unwrapping the brackets when there is no port.
* fix(server): serve pprof profiles when a BaseURL is configured
net/http/pprof's Index resolves the profile name by trimming "/debug/pprof/" from
the raw request path, which never matches once MountRouter prepends the BasePath.
Requests for any profile without an explicit chi route fell through to the index
page, returning HTML with a 200 instead of the profile. The handler now strips the
BasePath first.
* test(scanner): run the goroutine leak check unconditionally
The scanner suite's goleak check only ran when the GOLEAK env var was set, so it
never ran in CI and could not catch a regression. It passes with the existing
ignore list, verified over repeated runs, so the gate is removed.
* fix(server): close the background image body on a non-200 response
serveImage returned early on an unexpected status code without closing the response
body, pinning the connection until the 5s client timeout. The nolint:bodyclose
above the request suppressed the linter that would have caught it, and its
justification only holds on the success path, where the body is handed to the
CachedStream wrapper.
* test(scanner): repair BenchmarkScan so it can actually run
The benchmark failed three ways before reaching its first iteration: it reused a
shared temp DB and tried to repoint the default library, it never loaded the config
defaults so the scanner got a concurrency of 0, and it lacked the notify ignore that
the suite already carries. tests.Init now takes a testing.TB so a benchmark can load
the test config the same way the suites do.
* refactor(artwork): drop the unused sourceFunc Stringer
sourceFunc.String derived a label from the closure's symbol name via reflection, but
nothing called it: the trace output builds its candidate labels from explicit strings.
Whole-program analysis confirms it is unreachable, and dropping it removes a
reflection-based dependency on compiler closure-naming details.
* refactor(plugins): reuse extractHostname in the websocket allowlist
The IPv6 host parsing added for isHostAllowed duplicated extractHostname, which
already lives in the same package and backs the HTTP client's identical allowlist
check. Two copies of a security-relevant parser can drift, so the websocket service
now calls the existing helper. The port-stripping specs move into the URL Validation
block that already covered them.
* perf(scanner): bound the tag truncation trim to a partial rune
The trim loop dropped every trailing byte that failed to decode, so a value ending
in a long run of invalid bytes was walked one byte at a time: a 1 MiB lyrics tag
measured 2.58ms against 45ns for a normal cut. A partial rune is at most 3 trailing
bytes, so the loop is capped there, which also stops it consuming a pre-existing
invalid run.
* test: tighten the tests added with the Go 1.27 bugfixes
Drop the testItem stub in favour of the package's own cacheKey, register the pprof
test profile once at package scope, and replace the hand-rolled goroutine settle
loop with Eventually. Also corrects a comment that credited a TestMain the scanner
suite does not have.
* test(scanner): ignore notify's nonrecursive-tree goroutines on Linux
The goroutine leak check only ignored the recursive tree (macOS/FSEvents).
Linux CI uses inotify, whose nonrecursive tree leaks dispatch and internal
goroutines after Stop(), failing the check.
* fix(scanner): avoid a truncation panic when MaxLength is 1 or 2
A value of only UTF-8 continuation bytes drained the partial-rune loop to
empty, then sliced value[:-1] and panicked. Break when DecodeLastRune returns
size 0 (empty string) by testing size != 1 instead of size > 1.
* fix: address Codex review on the pprof base path and scan benchmark
- profilerHandler: treat a root BasePath ("/") as no prefix, so http.StripPrefix
keeps the leading slash chi needs; without this the profiler 404s when BaseURL
is "/". Cover the root case in the test.
- BenchmarkScan: make it run regardless of test/benchmark ordering. Add
singleton.DeleteInstance so a fresh DB is opened after TestScanner closes the
shared one, guard driver registration with sync.Once so the rebuild does not
re-Register, and ignore the Ginkgo interrupt-handler and Linux notify
goroutines the preceding suite leaves behind.
* fix: address Codex round 2 on BasePath trailing slash and benchmark DB cleanup
- profilerHandler: trim all trailing slashes (TrimRight), not just a bare "/", so
a BaseURL like "/music/" strips correctly instead of 404ing. Cover it in the test.
- BenchmarkScan: keep and defer db.Init's closer so the DB is closed before
b.TempDir cleanup, which otherwise cannot delete the open SQLite/WAL files on Windows.
* test(plugins): make the suite safe to run in parallel processes
Two things broke when the suite ran across several Ginkgo processes.
buildTestPlugins ran in every process, so N copies of make raced in the same
directory. The packaging rule made that worse by staging every plugin through
one shared plugin.wasm, so concurrent targets clobbered each other and left
orphaned temp files behind. That also ruled out make -j.
Stage each package under its own per-target directory, and move the build into
SynchronizedBeforeSuite so process 1 does it once while the others wait.
* ci: run the plugins suite in parallel processes
With the compilation cache warm the suite is bound by spec execution, which
splits cleanly across processes. Run it as its own step with the ginkgo CLI,
already declared as a tool in go.mod, and drop the package from the main go
test invocation so it is not run twice.
Locally, with -race: 69s to 19s warm, and 256s to 96s cold.
* ci: give the plugins suite its own job so it runs concurrently
Running it as a second step in the go job serialised it against the other 90
packages, which cancelled out the parallel win: the job went from 5m41s to
only 5m29s even though the suite itself dropped from ~175s to 82s.
Move it to its own job so the two run at the same time. The WASM compilation
cache moves with it, since the go job no longer runs the suite.
* ci: cache the plugins test suite WASM compilation across runs
The 'Test Go code' job was dominated by a single package: 'plugins' took
541s of the 699s test step. The suite builds 25 test plugins as full-Go
wasip1 modules of ~4.5MB each, and wazero must compile every one to machine
code. Under -race that compiler work is instrumented, so each module costs
around 11 seconds.
The suite already shared a wazero compilation cache, but three things kept it
from paying off. It lived in a fresh temp dir, so nothing survived the run.
The default plugins.cachesize of 200MB was smaller than the 334MB the cache
actually needs, so the purge evicted entries mid-run. And the wasm binaries
embedded VCS stamps, so every commit produced different bytes and missed the
content-addressed cache anyway.
Point CacheFolder at plugins/testdata/.wazero-cache, raise the test cache
limit past what the suite needs, build the test plugins with -buildvcs=false,
and restore the directory in CI. Locally the package goes from 256s to 74s
with the cache warm and the wasm rebuilt from scratch.
* ci: key the WASM cache on what actually changes the modules
The test plugins are separate Go modules with their own go.mod and go.sum;
they reach the PDK through a replace directive and never read the root
module. So the root go.sum has no bearing on the wasm bytes, and the wazero
version it pins is already namespaced by wazero itself, which stores entries
under wazero-<version>-<goarch>-<goos>. Keying on it only rotated the cache
on every unrelated dependency bump.
Drop it, and add the go.mod files that were missing: the test plugins' own
and the PDK's. The root go.mod stays, since it selects the toolchain that
builds the modules.
* ci: key the WASM cache on the toolchain version, not go.mod
Only the Go toolchain in the root go.mod affects the built wasm, but the file
also changes on every direct dependency bump, which would rotate the cache for
no reason. Take setup-go's go-version output instead: it is the version that
actually built the modules.
Moves both the xx-build toolchain stage and the final runtime image from
Alpine 3.20 (past end of active support) to 3.22.
3.22 is the last release where ffmpeg is still 6.1.x — it jumps to 8.0 in
3.23 — so transcoding behavior is unchanged by this bump.
Alpine 3.21 repackaged mesa, and from that release on `mpv` requires
so:libEGL.so.1 and so:libgbm.so.1. Those pull mesa -> llvm20-libs (156MB)
plus the gallium drivers (62MB), which took the image from 231MB/62MB
compressed to 578MB/147MB. mesa-egl is the only provider of libEGL.so.1,
and newer Alpine releases do not improve on this.
Navidrome runs mpv headless for jukebox audio and never enters a video
path, so this replaces libEGL/libgbm with generated no-op stubs and drops
the mesa/LLVM stack. The stub symbol list is read from real mesa at build
time and cross-compiled with the existing xx toolchain, so it adapts per
architecture rather than being hardcoded.
Verified with logging stubs across mp3/flac/ogg/opus/m4a/wav driving the
default MPVCmdTemplate (pause, volume, time-pos seek, quit): zero calls
into the stubbed libraries. The final stage now also runs mpv once at
build time, so a broken stub fails the build instead of shipping.
Image size: 323MB -> 325MB (84MB -> 86MB compressed).
* build: upgrade to Go 1.27
Bumps the toolchain in go.mod, both golang base images in the Dockerfile, and
the devcontainer VARIANT. CI needs no change, as the workflows resolve the
version through go-version-file: go.mod.
Tests, race tests, build and vet all pass on go1.27.0.
* build: upgrade golangci-lint to v2.13.0
v2.13.0 is the first release built with Go 1.27, so it can lint a module whose go
directive is 1.27. It also enables gosec's G404 on math/rand/v2, which flags the
three rand.Shuffle call sites. Shuffle order is not a security decision, and the
crypto-backed alternative in utils/random costs 25x and allocates per swap, so the
call sites are annotated rather than the rule excluded, keeping G404 active for the
cases where it would matter.
* chore(deps): update Go dependencies to latest versions
Signed-off-by: Deluan <deluan@navidrome.org>
* build: bump golangci-lint to v2.13.2
---------
Signed-off-by: Deluan <deluan@navidrome.org>
The default AAC transcode emits raw ADTS (`ffmpeg ... -f adts -`), but
the MIME table mapped `.aac` to `audio/mp4`. Clients that dispatch
strictly on Content-Type could reject the stream because the declared
container did not match the payload.
`.m4a` and `.alac` stay on `audio/mp4`, since those really are MP4.
Fixes#5958
Signed-off-by: Aditya Raj Singh <aditya@bncw.in>
Co-authored-by: Deluan Quintão <deluan@navidrome.org>
The agent name used in the `Agents` config option comes from the .ndp file
name, not from the manifest. The Plugins UI showed the ID but never said what
it was for, so renaming a plugin file silently breaks the config with only a
Debug-level "Unknown agent ignored" line to go on.
Add a caption under the ID in the Plugins UI, and list the accepted names
alongside the rejected one in that log line.
Related to navidrome/apple-music-plugin#14
* feat(agents): retry-later error type with optional server delay
Add agents.ErrRetryLater and agents.RetryLaterError, which carries the
delay requested by an external service (e.g. ListenBrainz's
X-RateLimit-Reset-In). scrobbler.ErrRetryLater becomes an alias of the new
sentinel, so existing errors.Is checks and the plugin error-string protocol
keep working unchanged. Groundwork for honoring server-requested retry
delays across scrobbling, metadata agents and artwork.
Song.Equals tests moved to song_test.go to enable external test package.
* fix(scrobbler): honor backoff window and server-requested retry delay
ListenBrainz 429s were decoded into a typed error that classified as
unrecoverable, silently discarding the scrobble (a JSON-bodied 429 was
measured live). The client now maps any 429 to agents.RetryLaterError,
carrying X-RateLimit-Reset-In when present (capped at 1h). Last.fm error 29
(rate limit) is now retryable like 11/16. The buffer's drain loop no longer
lets wake signals bypass an active backoff window - new plays enqueue but
drain only when the window closes - and the wait honors the server delay
via max(backoff, retryIn).
* feat(agents): skip cooling-down agents in aggregate calls
When an agent reports retry-later, remember a per-agent cooldown deadline
(the server-requested delay, or 1 minute when unspecified) and skip that
agent in all aggregate metadata calls until it passes. A round that found
no data but skipped or saw a throttled agent returns ErrRetryLater instead
of ErrNotFound, so callers cannot mistake rate limiting for a definitive
'no data' answer.
* feat(artwork): honor server-requested retry delay when rescheduling
When an external image lookup fails with a retry-later error carrying a
delay (e.g. a 429 with X-RateLimit-Reset-In), the chain trace carries the
largest such hint back to the worker, which reschedules the item at
max(exponential backoff, server delay) instead of backoff alone.
* feat(plugins): retry-later with optional delay for scrobbler and agent plugins
Scrobbler plugins can now return scrobbler(retry_later:N) to request a
retry in N seconds (capped at 1h); the bare token keeps its old meaning.
Metadata-agent plugins, which had no error vocabulary at all, gain the
parallel agent(retry_later[:N]) token, mapped to agents.RetryLaterError so
the aggregate's cooldown and the artwork worker honor plugin throttling
the same way as built-in agents.
* fix: address whole-branch review findings for retry-later handling
Narrow the aggregate's throttled rule to the spec sentence: core.Agents returns
ErrRetryLater only when no agent answered at all (all skipped-cooling or
retry-later). An agent that does not implement the called method now returns an
internal errUnsupported instead of ErrNotFound, so it counts as "did not run" —
without that, the always-appended local agent would answer for biography, URL
and images and make ErrRetryLater unreachable.
Wire the consequence in core/external: a throttled round no longer stamps
ExternalInfoUpdatedAt (artist and album), so the empty result is not cached for
the TTL, and TopSongs maps ErrRetryLater to the same empty-200 the not-found
path already produced instead of a new client-facing error.
Move the Last.fm code-29 mapping into the client's central error construction so
every metadata path produces RetryLaterError, and map ListenBrainz's body-level
code 429 (sent with a non-429 HTTP status) the same way.
Clamp server- and plugin-requested delays in seconds before scaling to a
Duration, in all three parse sites: a header of 18446744074 wrapped past 2^64 and
came out as a 0.29s delay.
Also: extract the artwork worker's reschedule computation into retryDelay() and
cover both it and the trace RetryIn wiring with tests; collapse the double regex
call in mapScrobblerError; drop capabilities.ScrobblerErrorRetryLaterIn (ndpgen
never emits funcs, so plugin authors could not reach it); regenerate the PDKs so
MetadataAgentError reaches the Go and Rust SDKs; de-flake the cooldown tests
(long RetryIn for the skip case, separate expiry spec); and cover the max()
retry-delay aggregation across users in the scrobble buffer.
* refactor: dedupe retry-later parsing and simplify error collection
- Add agents.NewRetryLater and agents.RetryLaterFromSeconds, with a single
1h cap, replacing the parse+clamp+multiply logic and the maxRetryInSeconds
constant duplicated across listenbrainz, plugins and the agent adapter.
- Move HTTP header parsing to httpclient.RetryAfter, so the transport layer
owns it and stays domain-agnostic; drop retryInFromHeaders from the
ListenBrainz client. Covered by a new Ginkgo table in that package.
- Collapse the two near-identical plugin retry_later regexes into one
parseRetryLater(prefix, msg) shared by the agent and scrobbler adapters.
- Fold the duplicated noteRetryIn snippet from fetchArtistImage and
fetchAlbumImage into recordAgent, which already branched on the same
isTransientExternal condition.
- Replace the atomic.Bool + note() closure in populateArtistInfo with
errgroup's own error collection; the group carries no context, so a
returned error does not cancel its siblings.
- Reuse recoveringScrobbler for the per-user delay test instead of a third
double, and switch fakeScrobbler's mutex-guarded error to the
atomic.Pointer idiom already used in the same package.
* refactor(listenbrainz): keep rate-limit header parsing in the adapter
The X-RateLimit-Reset-In header is ListenBrainz's own convention, not a
shared one: Last.fm sends no rate-limit headers at all and reports its
limit as a body code, and no other integration in tree sends Retry-After.
A parser in utils/httpclient implied a uniformity across services that
does not exist, so it moves back next to the only client that can know
which header its service sends.
* refactor(agents): collapse the retry-later sentinel and error into one type
ErrRetryLater is now the zero-delay RetryLaterError rather than a separate
errors.New value, so errors.Is and errors.AsType both match the sentinel and
every delay-carrying variant. That removes the trap where a bare sentinel
silently skipped the AsType path, and lets every consumer read the delay off
the error directly: the RetryIn accessor and the two constructors are gone,
with the policy cap applied where untrusted input is parsed.
* refactor(agents): split the cooldown store from the per-dispatch tally
The cooldown map and mutex become a cooldowns value with active/park, holding
no knowledge of errors; agentAttempts records one dispatch's outcomes and owns
the classification that noteAgentError used to hide behind a bool. The three
dispatch loops now touch a single object: skip folds the cooldown check and the
throttled flag into one call, so the store never appears in the loops.
* refactor(agents): share one dispatch loop between the agent call helpers
callAgentMethod and callAgentSliceMethod ran identical loops, differing only in
how they test a result for emptiness: a slice cannot be compared against its
zero value, so the two could not share a constraint. Both now delegate to
callAgent, which takes that test as a parameter. Keeping the loop in one place
matters more than the lines saved: it holds the cooldown skip, the attempt
recording and the empty-dispatch verdict, and a fix applied to one copy but not
the other would be silent.
* test: cover the two retry-later paths a mutation could break silently
Both gaps were proven, not guessed: making the artwork worker pass 0 instead
of the collected hint left all 386 specs green, and replacing the default
agent cooldown with 0 left the agents suite green. The worker test drives a
throttled image agent through drain and asserts the persisted retry_at, and
the cooldown test parks an agent that asked to be retried without naming a
delay, which is what Last.fm does on every rate limit.
* refactor(artwork): carry the external failure as an error, not a flag plus a trace field
The retry delay was riding on ChainTrace, a diagnostic that gets persisted, while
the very same signal — an external source faulted — already travelled by value as
resolution.extError. That was two mechanisms for one idea, and it put control-flow
state inside a serializable trace.
resolution.extError and chainState.extErr become the error itself, so a caller
checks err != nil for the fault and errors.AsType for the delay the provider asked
for. The agent loops return that error last, per convention, and longerRetry keeps
whichever failure wants the longer wait. ChainTrace goes back to holding only steps
and no longer imports core/agents.
* fix(artwork): check the resolve error before reading its resolution
Reading res.extError before the err check was safe only because every error path
in resolve returns a bare resolution{}; a future path returning a partly-filled
one would have been read silently. The failure path now returns no delay
explicitly.
* test(artwork): assert the delay acquire reports, not just its downstream effect
acquire's retry delay was only covered through the worker's persisted retry_at,
one layer away from where the value is computed. Both outcomes are now pinned at
the processor: a plain failure asks for nothing, a throttled provider's delay is
passed through.
* refactor: share the retry-seconds parse and drop the backoff deadline arithmetic
The clamp-before-scaling invariant lived in two parsers and was independently
re-tested in three files with the same magic number; a fix applied to one copy
would have left the others wrapping a huge value down to a fraction of a second.
It moves to agents.ParseRetryIn.
The buffer tracked an absolute retryDeadline only to re-arm a timer that was
already armed for the same instant; a backingOff flag says the same thing without
the arithmetic. The plugin token regex now carries its capability in the pattern
instead of capturing and comparing, so another capability's token in the same
message cannot mask it. resolution.extError becomes extErr, matching its
chainState counterpart.
* fix(agents): keep the longer cooldown when parks overlap
Calls to one agent overlap, so a short cooldown could land after a long one
started and cut it short. park now keeps whichever deadline is later, matching
the rule longerRetry already applies on the artwork side. No in-tree provider
can currently produce two different delays for the same agent, so this is
hardening rather than a fix for observed behaviour.
* fix(agents): parse the retry delay at a fixed width
strconv.Atoi parses into the native int, so on the 32-bit targets we ship
(linux/386, windows/386, three ARM variants) a delay above MaxInt32 seconds
overflowed and became unspecified instead of being capped. No provider sends a
68-year delay, so this is not user-visible, but the overflow tests asserted the
cap and would have failed on those architectures, where tests never run.
* fix(plugins): anchor the retry_later regex to a word boundary
Prevents a superstring like useragent(retry_later) from matching the
agent capability token.
stream v1.5.0 added CancelWithErr, which delivers a cancellation cause to
blocked reads, future reads, and NextReader. The fscache fork now delegates
CloseWithError to it, dropping its own cause recording and reader wrappers.
Behavior is unchanged on the Navidrome side.
* fix(scanner): read file birth time via statx on Linux
On Linux the file birth time is only reachable through statx(2). We were
reading it with times.Get(), which looks only at the plain stat() result,
where the field does not exist: djherbis/times declares HasBirthTime=false
for Linux, so the check was always false and every file fell back to
time.Now(). This has been the case since #2553 introduced the feature, which
means that PR was a no-op on Linux from day one. macOS and Windows were
never affected, as there the birth time does come back from plain stat.
BirthTime() now tries times.Get() first, which costs no syscall and is
already correct on macOS, Windows and BSD, and only falls back to
times.Stat() on the path when that comes back empty. Ordering matters: on
Windows times.Stat() opens the file asking for FILE_WRITE_ATTRIBUTES, which
fails on a read-only share before falling back.
Not every filesystem stores a birth time. Measured with a probe over real
mounts: ext4, SMB/CIFS and mergerfs report one, while NFS and rclone/FUSE
never do. Asking those on every file is pure overhead, so a miss is
remembered per device on the localFS and skipped from then on. The memo is
keyed by device rather than by library, so a library spanning two mounts
does not lose birth times on the mount that does support them.
Cost of the extra call is ~2us per file against ~52us just to open a file
for tag reading, so 0.23s across a 97k-file library, and only for files
whose tags are actually read.
Existing rows keep their current birth_time: the repository drops that
column on update, so only newly added files get the real value.
* fix(scanner): return the device id opaquely to satisfy unconvert
st.Dev is uint64 on Linux and int32 on darwin, so a uint64() cast is
redundant on one and required on the other. Returning it as an opaque value
drops the cast entirely, which also removes the gosec suppression that came
with it. The value is only ever used as a sync.Map key.
The button only rendered when an agent supplied a real last.fm URL, either
embedded in the biography or as artistInfo.lastFmUrl. Neither source is
reliable anymore: cleanContent strips the "Read more on Last.fm" anchor out of
the biography, and the Last.fm agent does not register at all unless
LastFM.ApiKey and LastFM.Secret are set, in which case GetArtistURL falls
through to ListenBrainz, which returns the artist's official homepage. The
isLastFmURL guard then correctly rejects it and the button disappears.
Build the URL from the artist name when no canonical one is available, the same
way AlbumExternalLinks already does for albums. A real last.fm URL is still
preferred when one is present, and the button stays hidden when Last.fm is
disabled or the artist has no name.
* refactor(artwork): move artworkItemName into core/artwork as ItemName
* feat(external): add RefreshInfo to force an external info refresh
RefreshInfo re-fetches and re-saves external info for one artist or
album, bypassing the TTL check that UpdateArtistInfo/UpdateAlbumInfo
use. It is synchronous; callers that must not block detach it themselves.
Also makes MockArtistRepo/MockAlbumRepo.UpdateExternalInfo persist to
Data (previously a no-op) and adds the new method to the e2e noopProvider,
both required so the interface addition compiles and is observable in tests.
* feat(external): broadcast RefreshResource after external info is saved
populateArtistInfo and populateAlbumInfo now emit the same RefreshResource
event the artwork worker uses, so the UI learns about both foreground and
background metadata refreshes.
* feat(nativeapi): replace artwork refresh endpoint with metadata refresh
* feat(ui): add refreshMetadata to the data provider
* feat(ui): add a Refresh Metadata item to the album and artist context menus
* fix(ui): re-fetch artist info when the record is refreshed
* test: fix mislabeled spec, add kind-gate negative case, guard nil mock maps
- Rename the RefreshInfo spec that claimed to cover the save-failure/broadcast
path: SetError(true) fails Get too, so it only proves RefreshInfo bails out
early at getArtist.
- Add a spec proving playlist refreshes skip the external-info step, since
that asymmetry (al/ar only) was documented but unasserted.
- Add lazy nil-map init to MockAlbumRepo/MockArtistRepo.UpdateExternalInfo so
a composite-literal-constructed mock doesn't panic on first save.
* test: relocate discArtworkName specs from cmd to core/artwork
artworkItemName moved into core/artwork as ItemName in an earlier commit, but
its disc-name specs stayed behind in cmd/artwork_test.go, reaching across
packages. Move them to core/artwork/item_name_test.go where the code now lives.
* fix(ui): shape refreshMetadata like a react-admin response
react-admin validates custom dataProvider methods and rejects any response
without a `data` key, so the raw httpClient promise made every click surface
an error toast instead of the success message. The unit test mocked
useDataProvider, which skips that validation.
Also folds "which kinds have external info" into external.HasInfo so the
handler stops restating it, drops the nil-broker guard that only existed for
tests, and delegates the mocks' UpdateExternalInfo to Put.
* refactor(external): unexport infoKinds
Only HasInfo is used outside the package, so the slice itself does not need
to be exported.
* refactor(artwork): fold ItemName into housekeeping.go next to Refresh
ItemName exists to guard Refresh from ids that would orphan a queue row, and
both callers invoke them back to back. A separate file hid that pairing; it was
only split out to keep the move out of cmd/ legible in review.
* fix(nativeapi): return 500 when the refresh lookup fails for a non-ErrNotFound reason
A transient repository error told the admin the id did not exist, and the error
was dropped without a log line, so nothing pointed at the real cause.
Also drops the inherited claim that clearing artwork state shows a placeholder.
Reads fall back to local resolution, so that only holds when there is no local art.
* fix(ui): move Refresh Metadata above Get Info in the context menu
Menu order follows key insertion order in the options object, so the new spec
pins the position rather than leaving it to be shuffled by the next addition.
* fix(stream): abort the response when a transcoded stream is truncated
When a transcode failed after some audio had already been sent, Serve logged
the error and returned nil, so Go finished the chunked body normally and the
client received an apparently complete, silently short file. Symfonium users
hit this on large offline syncs, and the worst path, ffmpeg dying mid-write
behind the transcoding cache, produced no error and nothing in the log above
Debug: the cache writer was closed plainly, so readers drained the truncated
entry to a clean EOF.
The root cause of that silence is an fscache limitation: Close is the only way
to end a cache write, and Close always means "complete". This adopts the
deluan/fscache fork, which adds CloseWithError: on failure copyAndClose now
cancels the entry with the cause, so every attached reader fails mid-read with
the real error instead of EOF, a late Get for the entry is refused, and the
entry never reports a final size. The error travels inside the entry each
reader holds, which makes per-generation delivery automatic and needs no
bookkeeping on our side.
With the failure arriving in-band, one change in Serve covers every mode: an
io.Copy error after bytes are on the wire panics with http.ErrAbortHandler.
Go aborts the response without the terminating chunk (RST_STREAM on HTTP/2),
chi's Recoverer re-panics that value, and the deferred stream.Close() still
runs, so the transcode limiter slot is released as before.
Two behaviors improve as side effects. A transcoder that dies before its first
byte now yields a Subsonic error response instead of a 200 with an empty body,
since the failure reaches Serve as an error while the status is still
unsent; genuinely empty output (clean EOF, exit 0) keeps the 200. And a failed
entry's invalidation no longer defers its unlink past a replacement entry
re-creating the same file, because canceling already closed its readers.
* fix(cache): warn when the cache writer cannot report failures to readers
The CloseWithError capability comes from the fscache fork via a go.mod
replace directive, and a type assertion picks it up. If that directive is
ever lost, the assertion fails silently, readers of a dead writer go back to
draining a truncated entry to a clean EOF, and nothing says so.
Two layers against that: a warning on the failure path when the writer lacks
the capability, and a test that asserts the writer fscache returns carries
it, so losing the fork fails CI instead of a listener's download.
* build: point the fscache replace at the fork's master
deluan/fscache#1 is merged; pin the merge commit instead of the review
branch. Pinned by sha because the module proxy still resolves the fork's
master ref to its pre-merge commit.
* build: reference the upstream fscache PR in the replace comment
The replace itself must keep pointing at the fork: the commit only exists in
djherbis/fscache under refs/pull/22/head, which the Go module fetcher cannot
resolve (verified: unknown revision for both short and full sha). The same
commit is advertised on the fork's master, so that is the fetchable source.
Album-level tags were ordered by frequency and then alphabetically by value.
Album.Genre is just the first genre in that list, so any album whose genres tie
on frequency, which is the normal case, displayed the alphabetically first genre
rather than the first one in the file. A file tagged
"Native American New Age; Indigenous American Traditional Music; Ambient"
showed up as "Ambient".
Break frequency ties on order of appearance instead. This affects all
album-level tags, so mood tagged "Happy; Chill" now keeps that order too.
MediaFiles.ToAlbum already sorts the files by path before flattening their tags,
so the aggregated order stays deterministic across scans.
Only album genre was affected; media_file tags already preserved file order.
