* ci: cache the plugins test suite WASM compilation across runs
The 'Test Go code' job was dominated by a single package: 'plugins' took
541s of the 699s test step. The suite builds 25 test plugins as full-Go
wasip1 modules of ~4.5MB each, and wazero must compile every one to machine
code. Under -race that compiler work is instrumented, so each module costs
around 11 seconds.
The suite already shared a wazero compilation cache, but three things kept it
from paying off. It lived in a fresh temp dir, so nothing survived the run.
The default plugins.cachesize of 200MB was smaller than the 334MB the cache
actually needs, so the purge evicted entries mid-run. And the wasm binaries
embedded VCS stamps, so every commit produced different bytes and missed the
content-addressed cache anyway.
Point CacheFolder at plugins/testdata/.wazero-cache, raise the test cache
limit past what the suite needs, build the test plugins with -buildvcs=false,
and restore the directory in CI. Locally the package goes from 256s to 74s
with the cache warm and the wasm rebuilt from scratch.
* ci: key the WASM cache on what actually changes the modules
The test plugins are separate Go modules with their own go.mod and go.sum;
they reach the PDK through a replace directive and never read the root
module. So the root go.sum has no bearing on the wasm bytes, and the wazero
version it pins is already namespaced by wazero itself, which stores entries
under wazero-<version>-<goarch>-<goos>. Keying on it only rotated the cache
on every unrelated dependency bump.
Drop it, and add the go.mod files that were missing: the test plugins' own
and the PDK's. The root go.mod stays, since it selects the toolchain that
builds the modules.
* ci: key the WASM cache on the toolchain version, not go.mod
Only the Go toolchain in the root go.mod affects the built wasm, but the file
also changes on every direct dependency bump, which would rotate the cache for
no reason. Take setup-go's go-version output instead: it is the version that
actually built the modules.
Moves both the xx-build toolchain stage and the final runtime image from
Alpine 3.20 (past end of active support) to 3.22.
3.22 is the last release where ffmpeg is still 6.1.x — it jumps to 8.0 in
3.23 — so transcoding behavior is unchanged by this bump.
Alpine 3.21 repackaged mesa, and from that release on `mpv` requires
so:libEGL.so.1 and so:libgbm.so.1. Those pull mesa -> llvm20-libs (156MB)
plus the gallium drivers (62MB), which took the image from 231MB/62MB
compressed to 578MB/147MB. mesa-egl is the only provider of libEGL.so.1,
and newer Alpine releases do not improve on this.
Navidrome runs mpv headless for jukebox audio and never enters a video
path, so this replaces libEGL/libgbm with generated no-op stubs and drops
the mesa/LLVM stack. The stub symbol list is read from real mesa at build
time and cross-compiled with the existing xx toolchain, so it adapts per
architecture rather than being hardcoded.
Verified with logging stubs across mp3/flac/ogg/opus/m4a/wav driving the
default MPVCmdTemplate (pause, volume, time-pos seek, quit): zero calls
into the stubbed libraries. The final stage now also runs mpv once at
build time, so a broken stub fails the build instead of shipping.
Image size: 323MB -> 325MB (84MB -> 86MB compressed).
* build: upgrade to Go 1.27
Bumps the toolchain in go.mod, both golang base images in the Dockerfile, and
the devcontainer VARIANT. CI needs no change, as the workflows resolve the
version through go-version-file: go.mod.
Tests, race tests, build and vet all pass on go1.27.0.
* build: upgrade golangci-lint to v2.13.0
v2.13.0 is the first release built with Go 1.27, so it can lint a module whose go
directive is 1.27. It also enables gosec's G404 on math/rand/v2, which flags the
three rand.Shuffle call sites. Shuffle order is not a security decision, and the
crypto-backed alternative in utils/random costs 25x and allocates per swap, so the
call sites are annotated rather than the rule excluded, keeping G404 active for the
cases where it would matter.
* chore(deps): update Go dependencies to latest versions
Signed-off-by: Deluan <deluan@navidrome.org>
* build: bump golangci-lint to v2.13.2
---------
Signed-off-by: Deluan <deluan@navidrome.org>
The default AAC transcode emits raw ADTS (`ffmpeg ... -f adts -`), but
the MIME table mapped `.aac` to `audio/mp4`. Clients that dispatch
strictly on Content-Type could reject the stream because the declared
container did not match the payload.
`.m4a` and `.alac` stay on `audio/mp4`, since those really are MP4.
Fixes#5958
Signed-off-by: Aditya Raj Singh <aditya@bncw.in>
Co-authored-by: Deluan Quintão <deluan@navidrome.org>
The agent name used in the `Agents` config option comes from the .ndp file
name, not from the manifest. The Plugins UI showed the ID but never said what
it was for, so renaming a plugin file silently breaks the config with only a
Debug-level "Unknown agent ignored" line to go on.
Add a caption under the ID in the Plugins UI, and list the accepted names
alongside the rejected one in that log line.
Related to navidrome/apple-music-plugin#14
* feat(agents): retry-later error type with optional server delay
Add agents.ErrRetryLater and agents.RetryLaterError, which carries the
delay requested by an external service (e.g. ListenBrainz's
X-RateLimit-Reset-In). scrobbler.ErrRetryLater becomes an alias of the new
sentinel, so existing errors.Is checks and the plugin error-string protocol
keep working unchanged. Groundwork for honoring server-requested retry
delays across scrobbling, metadata agents and artwork.
Song.Equals tests moved to song_test.go to enable external test package.
* fix(scrobbler): honor backoff window and server-requested retry delay
ListenBrainz 429s were decoded into a typed error that classified as
unrecoverable, silently discarding the scrobble (a JSON-bodied 429 was
measured live). The client now maps any 429 to agents.RetryLaterError,
carrying X-RateLimit-Reset-In when present (capped at 1h). Last.fm error 29
(rate limit) is now retryable like 11/16. The buffer's drain loop no longer
lets wake signals bypass an active backoff window - new plays enqueue but
drain only when the window closes - and the wait honors the server delay
via max(backoff, retryIn).
* feat(agents): skip cooling-down agents in aggregate calls
When an agent reports retry-later, remember a per-agent cooldown deadline
(the server-requested delay, or 1 minute when unspecified) and skip that
agent in all aggregate metadata calls until it passes. A round that found
no data but skipped or saw a throttled agent returns ErrRetryLater instead
of ErrNotFound, so callers cannot mistake rate limiting for a definitive
'no data' answer.
* feat(artwork): honor server-requested retry delay when rescheduling
When an external image lookup fails with a retry-later error carrying a
delay (e.g. a 429 with X-RateLimit-Reset-In), the chain trace carries the
largest such hint back to the worker, which reschedules the item at
max(exponential backoff, server delay) instead of backoff alone.
* feat(plugins): retry-later with optional delay for scrobbler and agent plugins
Scrobbler plugins can now return scrobbler(retry_later:N) to request a
retry in N seconds (capped at 1h); the bare token keeps its old meaning.
Metadata-agent plugins, which had no error vocabulary at all, gain the
parallel agent(retry_later[:N]) token, mapped to agents.RetryLaterError so
the aggregate's cooldown and the artwork worker honor plugin throttling
the same way as built-in agents.
* fix: address whole-branch review findings for retry-later handling
Narrow the aggregate's throttled rule to the spec sentence: core.Agents returns
ErrRetryLater only when no agent answered at all (all skipped-cooling or
retry-later). An agent that does not implement the called method now returns an
internal errUnsupported instead of ErrNotFound, so it counts as "did not run" —
without that, the always-appended local agent would answer for biography, URL
and images and make ErrRetryLater unreachable.
Wire the consequence in core/external: a throttled round no longer stamps
ExternalInfoUpdatedAt (artist and album), so the empty result is not cached for
the TTL, and TopSongs maps ErrRetryLater to the same empty-200 the not-found
path already produced instead of a new client-facing error.
Move the Last.fm code-29 mapping into the client's central error construction so
every metadata path produces RetryLaterError, and map ListenBrainz's body-level
code 429 (sent with a non-429 HTTP status) the same way.
Clamp server- and plugin-requested delays in seconds before scaling to a
Duration, in all three parse sites: a header of 18446744074 wrapped past 2^64 and
came out as a 0.29s delay.
Also: extract the artwork worker's reschedule computation into retryDelay() and
cover both it and the trace RetryIn wiring with tests; collapse the double regex
call in mapScrobblerError; drop capabilities.ScrobblerErrorRetryLaterIn (ndpgen
never emits funcs, so plugin authors could not reach it); regenerate the PDKs so
MetadataAgentError reaches the Go and Rust SDKs; de-flake the cooldown tests
(long RetryIn for the skip case, separate expiry spec); and cover the max()
retry-delay aggregation across users in the scrobble buffer.
* refactor: dedupe retry-later parsing and simplify error collection
- Add agents.NewRetryLater and agents.RetryLaterFromSeconds, with a single
1h cap, replacing the parse+clamp+multiply logic and the maxRetryInSeconds
constant duplicated across listenbrainz, plugins and the agent adapter.
- Move HTTP header parsing to httpclient.RetryAfter, so the transport layer
owns it and stays domain-agnostic; drop retryInFromHeaders from the
ListenBrainz client. Covered by a new Ginkgo table in that package.
- Collapse the two near-identical plugin retry_later regexes into one
parseRetryLater(prefix, msg) shared by the agent and scrobbler adapters.
- Fold the duplicated noteRetryIn snippet from fetchArtistImage and
fetchAlbumImage into recordAgent, which already branched on the same
isTransientExternal condition.
- Replace the atomic.Bool + note() closure in populateArtistInfo with
errgroup's own error collection; the group carries no context, so a
returned error does not cancel its siblings.
- Reuse recoveringScrobbler for the per-user delay test instead of a third
double, and switch fakeScrobbler's mutex-guarded error to the
atomic.Pointer idiom already used in the same package.
* refactor(listenbrainz): keep rate-limit header parsing in the adapter
The X-RateLimit-Reset-In header is ListenBrainz's own convention, not a
shared one: Last.fm sends no rate-limit headers at all and reports its
limit as a body code, and no other integration in tree sends Retry-After.
A parser in utils/httpclient implied a uniformity across services that
does not exist, so it moves back next to the only client that can know
which header its service sends.
* refactor(agents): collapse the retry-later sentinel and error into one type
ErrRetryLater is now the zero-delay RetryLaterError rather than a separate
errors.New value, so errors.Is and errors.AsType both match the sentinel and
every delay-carrying variant. That removes the trap where a bare sentinel
silently skipped the AsType path, and lets every consumer read the delay off
the error directly: the RetryIn accessor and the two constructors are gone,
with the policy cap applied where untrusted input is parsed.
* refactor(agents): split the cooldown store from the per-dispatch tally
The cooldown map and mutex become a cooldowns value with active/park, holding
no knowledge of errors; agentAttempts records one dispatch's outcomes and owns
the classification that noteAgentError used to hide behind a bool. The three
dispatch loops now touch a single object: skip folds the cooldown check and the
throttled flag into one call, so the store never appears in the loops.
* refactor(agents): share one dispatch loop between the agent call helpers
callAgentMethod and callAgentSliceMethod ran identical loops, differing only in
how they test a result for emptiness: a slice cannot be compared against its
zero value, so the two could not share a constraint. Both now delegate to
callAgent, which takes that test as a parameter. Keeping the loop in one place
matters more than the lines saved: it holds the cooldown skip, the attempt
recording and the empty-dispatch verdict, and a fix applied to one copy but not
the other would be silent.
* test: cover the two retry-later paths a mutation could break silently
Both gaps were proven, not guessed: making the artwork worker pass 0 instead
of the collected hint left all 386 specs green, and replacing the default
agent cooldown with 0 left the agents suite green. The worker test drives a
throttled image agent through drain and asserts the persisted retry_at, and
the cooldown test parks an agent that asked to be retried without naming a
delay, which is what Last.fm does on every rate limit.
* refactor(artwork): carry the external failure as an error, not a flag plus a trace field
The retry delay was riding on ChainTrace, a diagnostic that gets persisted, while
the very same signal — an external source faulted — already travelled by value as
resolution.extError. That was two mechanisms for one idea, and it put control-flow
state inside a serializable trace.
resolution.extError and chainState.extErr become the error itself, so a caller
checks err != nil for the fault and errors.AsType for the delay the provider asked
for. The agent loops return that error last, per convention, and longerRetry keeps
whichever failure wants the longer wait. ChainTrace goes back to holding only steps
and no longer imports core/agents.
* fix(artwork): check the resolve error before reading its resolution
Reading res.extError before the err check was safe only because every error path
in resolve returns a bare resolution{}; a future path returning a partly-filled
one would have been read silently. The failure path now returns no delay
explicitly.
* test(artwork): assert the delay acquire reports, not just its downstream effect
acquire's retry delay was only covered through the worker's persisted retry_at,
one layer away from where the value is computed. Both outcomes are now pinned at
the processor: a plain failure asks for nothing, a throttled provider's delay is
passed through.
* refactor: share the retry-seconds parse and drop the backoff deadline arithmetic
The clamp-before-scaling invariant lived in two parsers and was independently
re-tested in three files with the same magic number; a fix applied to one copy
would have left the others wrapping a huge value down to a fraction of a second.
It moves to agents.ParseRetryIn.
The buffer tracked an absolute retryDeadline only to re-arm a timer that was
already armed for the same instant; a backingOff flag says the same thing without
the arithmetic. The plugin token regex now carries its capability in the pattern
instead of capturing and comparing, so another capability's token in the same
message cannot mask it. resolution.extError becomes extErr, matching its
chainState counterpart.
* fix(agents): keep the longer cooldown when parks overlap
Calls to one agent overlap, so a short cooldown could land after a long one
started and cut it short. park now keeps whichever deadline is later, matching
the rule longerRetry already applies on the artwork side. No in-tree provider
can currently produce two different delays for the same agent, so this is
hardening rather than a fix for observed behaviour.
* fix(agents): parse the retry delay at a fixed width
strconv.Atoi parses into the native int, so on the 32-bit targets we ship
(linux/386, windows/386, three ARM variants) a delay above MaxInt32 seconds
overflowed and became unspecified instead of being capped. No provider sends a
68-year delay, so this is not user-visible, but the overflow tests asserted the
cap and would have failed on those architectures, where tests never run.
* fix(plugins): anchor the retry_later regex to a word boundary
Prevents a superstring like useragent(retry_later) from matching the
agent capability token.
stream v1.5.0 added CancelWithErr, which delivers a cancellation cause to
blocked reads, future reads, and NextReader. The fscache fork now delegates
CloseWithError to it, dropping its own cause recording and reader wrappers.
Behavior is unchanged on the Navidrome side.
* fix(scanner): read file birth time via statx on Linux
On Linux the file birth time is only reachable through statx(2). We were
reading it with times.Get(), which looks only at the plain stat() result,
where the field does not exist: djherbis/times declares HasBirthTime=false
for Linux, so the check was always false and every file fell back to
time.Now(). This has been the case since #2553 introduced the feature, which
means that PR was a no-op on Linux from day one. macOS and Windows were
never affected, as there the birth time does come back from plain stat.
BirthTime() now tries times.Get() first, which costs no syscall and is
already correct on macOS, Windows and BSD, and only falls back to
times.Stat() on the path when that comes back empty. Ordering matters: on
Windows times.Stat() opens the file asking for FILE_WRITE_ATTRIBUTES, which
fails on a read-only share before falling back.
Not every filesystem stores a birth time. Measured with a probe over real
mounts: ext4, SMB/CIFS and mergerfs report one, while NFS and rclone/FUSE
never do. Asking those on every file is pure overhead, so a miss is
remembered per device on the localFS and skipped from then on. The memo is
keyed by device rather than by library, so a library spanning two mounts
does not lose birth times on the mount that does support them.
Cost of the extra call is ~2us per file against ~52us just to open a file
for tag reading, so 0.23s across a 97k-file library, and only for files
whose tags are actually read.
Existing rows keep their current birth_time: the repository drops that
column on update, so only newly added files get the real value.
* fix(scanner): return the device id opaquely to satisfy unconvert
st.Dev is uint64 on Linux and int32 on darwin, so a uint64() cast is
redundant on one and required on the other. Returning it as an opaque value
drops the cast entirely, which also removes the gosec suppression that came
with it. The value is only ever used as a sync.Map key.
The button only rendered when an agent supplied a real last.fm URL, either
embedded in the biography or as artistInfo.lastFmUrl. Neither source is
reliable anymore: cleanContent strips the "Read more on Last.fm" anchor out of
the biography, and the Last.fm agent does not register at all unless
LastFM.ApiKey and LastFM.Secret are set, in which case GetArtistURL falls
through to ListenBrainz, which returns the artist's official homepage. The
isLastFmURL guard then correctly rejects it and the button disappears.
Build the URL from the artist name when no canonical one is available, the same
way AlbumExternalLinks already does for albums. A real last.fm URL is still
preferred when one is present, and the button stays hidden when Last.fm is
disabled or the artist has no name.
* refactor(artwork): move artworkItemName into core/artwork as ItemName
* feat(external): add RefreshInfo to force an external info refresh
RefreshInfo re-fetches and re-saves external info for one artist or
album, bypassing the TTL check that UpdateArtistInfo/UpdateAlbumInfo
use. It is synchronous; callers that must not block detach it themselves.
Also makes MockArtistRepo/MockAlbumRepo.UpdateExternalInfo persist to
Data (previously a no-op) and adds the new method to the e2e noopProvider,
both required so the interface addition compiles and is observable in tests.
* feat(external): broadcast RefreshResource after external info is saved
populateArtistInfo and populateAlbumInfo now emit the same RefreshResource
event the artwork worker uses, so the UI learns about both foreground and
background metadata refreshes.
* feat(nativeapi): replace artwork refresh endpoint with metadata refresh
* feat(ui): add refreshMetadata to the data provider
* feat(ui): add a Refresh Metadata item to the album and artist context menus
* fix(ui): re-fetch artist info when the record is refreshed
* test: fix mislabeled spec, add kind-gate negative case, guard nil mock maps
- Rename the RefreshInfo spec that claimed to cover the save-failure/broadcast
path: SetError(true) fails Get too, so it only proves RefreshInfo bails out
early at getArtist.
- Add a spec proving playlist refreshes skip the external-info step, since
that asymmetry (al/ar only) was documented but unasserted.
- Add lazy nil-map init to MockAlbumRepo/MockArtistRepo.UpdateExternalInfo so
a composite-literal-constructed mock doesn't panic on first save.
* test: relocate discArtworkName specs from cmd to core/artwork
artworkItemName moved into core/artwork as ItemName in an earlier commit, but
its disc-name specs stayed behind in cmd/artwork_test.go, reaching across
packages. Move them to core/artwork/item_name_test.go where the code now lives.
* fix(ui): shape refreshMetadata like a react-admin response
react-admin validates custom dataProvider methods and rejects any response
without a `data` key, so the raw httpClient promise made every click surface
an error toast instead of the success message. The unit test mocked
useDataProvider, which skips that validation.
Also folds "which kinds have external info" into external.HasInfo so the
handler stops restating it, drops the nil-broker guard that only existed for
tests, and delegates the mocks' UpdateExternalInfo to Put.
* refactor(external): unexport infoKinds
Only HasInfo is used outside the package, so the slice itself does not need
to be exported.
* refactor(artwork): fold ItemName into housekeeping.go next to Refresh
ItemName exists to guard Refresh from ids that would orphan a queue row, and
both callers invoke them back to back. A separate file hid that pairing; it was
only split out to keep the move out of cmd/ legible in review.
* fix(nativeapi): return 500 when the refresh lookup fails for a non-ErrNotFound reason
A transient repository error told the admin the id did not exist, and the error
was dropped without a log line, so nothing pointed at the real cause.
Also drops the inherited claim that clearing artwork state shows a placeholder.
Reads fall back to local resolution, so that only holds when there is no local art.
* fix(ui): move Refresh Metadata above Get Info in the context menu
Menu order follows key insertion order in the options object, so the new spec
pins the position rather than leaving it to be shuffled by the next addition.
* fix(stream): abort the response when a transcoded stream is truncated
When a transcode failed after some audio had already been sent, Serve logged
the error and returned nil, so Go finished the chunked body normally and the
client received an apparently complete, silently short file. Symfonium users
hit this on large offline syncs, and the worst path, ffmpeg dying mid-write
behind the transcoding cache, produced no error and nothing in the log above
Debug: the cache writer was closed plainly, so readers drained the truncated
entry to a clean EOF.
The root cause of that silence is an fscache limitation: Close is the only way
to end a cache write, and Close always means "complete". This adopts the
deluan/fscache fork, which adds CloseWithError: on failure copyAndClose now
cancels the entry with the cause, so every attached reader fails mid-read with
the real error instead of EOF, a late Get for the entry is refused, and the
entry never reports a final size. The error travels inside the entry each
reader holds, which makes per-generation delivery automatic and needs no
bookkeeping on our side.
With the failure arriving in-band, one change in Serve covers every mode: an
io.Copy error after bytes are on the wire panics with http.ErrAbortHandler.
Go aborts the response without the terminating chunk (RST_STREAM on HTTP/2),
chi's Recoverer re-panics that value, and the deferred stream.Close() still
runs, so the transcode limiter slot is released as before.
Two behaviors improve as side effects. A transcoder that dies before its first
byte now yields a Subsonic error response instead of a 200 with an empty body,
since the failure reaches Serve as an error while the status is still
unsent; genuinely empty output (clean EOF, exit 0) keeps the 200. And a failed
entry's invalidation no longer defers its unlink past a replacement entry
re-creating the same file, because canceling already closed its readers.
* fix(cache): warn when the cache writer cannot report failures to readers
The CloseWithError capability comes from the fscache fork via a go.mod
replace directive, and a type assertion picks it up. If that directive is
ever lost, the assertion fails silently, readers of a dead writer go back to
draining a truncated entry to a clean EOF, and nothing says so.
Two layers against that: a warning on the failure path when the writer lacks
the capability, and a test that asserts the writer fscache returns carries
it, so losing the fork fails CI instead of a listener's download.
* build: point the fscache replace at the fork's master
deluan/fscache#1 is merged; pin the merge commit instead of the review
branch. Pinned by sha because the module proxy still resolves the fork's
master ref to its pre-merge commit.
* build: reference the upstream fscache PR in the replace comment
The replace itself must keep pointing at the fork: the commit only exists in
djherbis/fscache under refs/pull/22/head, which the Go module fetcher cannot
resolve (verified: unknown revision for both short and full sha). The same
commit is advertised on the fork's master, so that is the fetchable source.
Album-level tags were ordered by frequency and then alphabetically by value.
Album.Genre is just the first genre in that list, so any album whose genres tie
on frequency, which is the normal case, displayed the alphabetically first genre
rather than the first one in the file. A file tagged
"Native American New Age; Indigenous American Traditional Music; Ambient"
showed up as "Ambient".
Break frequency ties on order of appearance instead. This affects all
album-level tags, so mood tagged "Happy; Chill" now keeps that order too.
MediaFiles.ToAlbum already sorts the files by path before flattening their tags,
so the aggregated order stays deterministic across scans.
Only album genre was affected; media_file tags already preserved file order.
* There has been a report about navidrome hitting listenbrainz hard
and the listenbrainz guys wanting to be able to distinguish navidrome
* feat: apply Navidrome User-Agent to all outgoing HTTP requests
Add utils/httpclient, a shared http.Client factory whose transport sets
the User-Agent header (Navidrome/{version} - https://github.com/navidrome)
on any request that does not already have one, and use it at every place
the server builds an HTTP client: Last.fm, ListenBrainz and Deezer agents
and auth routers, insights collector, backgrounds handler, and the plugin
host HTTP service. Plugin-set User-Agent values are preserved. The
per-request header lines from the previous commit are superseded by the
transport.
---------
Co-authored-by: Deluan <deluan@navidrome.org>
The trace-level request log dumps all headers as a JSON blob, but the
redaction hook only had query-param patterns, so Authorization, X-Emby-Token,
X-MediaBrowser-Token and X-Nd-Authorization leaked their tokens in plaintext.
Add one pattern that blanks those header value arrays at the log sink.
* feat: add optional natural sort order for names and titles
Album, artist, song and playlist lists sort with a plain text comparison, so
names containing numbers come out as "Foo 1, Foo 10, Foo 2" instead of
"Foo 1, Foo 2, Foo 10" (issue #4554).
Adds an EnableNaturalSorting option, default off, that switches those sorts to
a NATSORT collation registered on every connection and backed by
natural.CompareFold. natural.Compare gained an ASCII case-folding variant
because it replaces 'collate nocase': sort_* columns hold raw tag values, so
without folding they would order uppercase before lowercase.
Applying the collation only inside mapSortOrder would have missed the default
configuration entirely, since that mapper runs only when PreferSortTags is on.
setSortMappings now also rewrites the order_* columns when natural sorting is
enabled on its own. Sorts over plain text columns that are not order_* columns
(playlist.name, album.name, media_file.title, playlist_tracks title) are
wrapped explicitly, and qualified with their table because 'user' is joined and
also has a 'name' column.
The option defaults to off because the collation cannot use the existing
indexes: measured on a synthetic 110k album library, the first page of an
album-by-name listing goes from 0.03ms to 14ms. Indexing the expression was
rejected outright - an index declared with a custom collation makes the whole
database unreadable to any tool that does not register it, including the
sqlite3 CLI, which fails even on 'select count(*)' and 'pragma integrity_check'.
* refactor: fold the two sort-order mappers into one
mapSortOrder and mapNaturalOrder shared the same regex and loop, differing only
in the expression they substituted, and setSortMappings picked between them with
a two-case switch. mapSortOrder now selects the column shape itself and defers to
collatedSort for the collation, so the 'collate' clause is emitted in one place
and the caller only has to decide whether any mapping is needed at all.
The mapper tests were three near-identical cases that each hard-coded one flag
combination; they are now a DescribeTable covering all four combinations of
PreferSortTags and EnableNaturalSorting, which the previous set did not. The
album sorting specs collapse the same way. Behavior is unchanged.
* fix: leave plain sort columns alone when natural sorting is off
collatedSort wrapped its column unconditionally, so the tiebreakers added for
plain text columns picked up 'collate nocase' even with EnableNaturalSorting
off. media_file.title, the playlist_tracks alias of it, and user.user_name are
all declared without a collation, so a default install would have silently
switched those tiebreaks from binary to case-insensitive ordering. Only
playlist.name was already NOCASE and genuinely unaffected.
The helper is now naturalSort and returns the column untouched unless the option
is on, so the default path keeps the collation each column was declared with.
sortCollation had a single remaining caller and folded into mapSortOrder.
Tests: the CompareFold table body was a verbatim copy of the Compare one, so
both now go through one expectOrder helper, and the album sorting specs inline
two single-use closures.
* fix(natural): defer the leading-zero tie-break to keep ordering transitive
Compare applied the padding difference between numerically equal digit runs only
when one side ended at the digit boundary, and ignored it mid-string. That made
the relation intransitive: CompareFold("1","1a") < 0 and CompareFold("1a","01a")
== 0, yet CompareFold("1","01a") > 0.
SQLite requires a collating function to be transitive and leaves ORDER BY
undefined otherwise, so registering this as NATSORT was not safe. Reproduced with
the real driver on three artist names that occur in practice - "3", "3 doors
down" and "03 greedo" - where paging one row at a time returned "03 greedo"
twice and dropped "3" entirely.
The padding difference is now carried as a tie-break that is applied only when
the strings are otherwise equal, which restores transitivity while keeping the
documented intent (a01 < a1, a0 < a00). Three existing entries changed: each
asserted that two distinct strings compare equal, which was the same defect seen
from the other side.
Found by the Codex review on #6015.
Plugins load concurrently through an errgroup. loadPluginWithConfig wrote
m.plugins under m.mu but read it back unlocked to pass to callPluginInit,
so one goroutine's write raced another's read. Caught by -race on master
(run 32608293134): all 640 specs passed, the job failed only on the race.
Capture the pointer while holding the lock and use the local. Holding m.mu
across callPluginInit would be wrong, since that runs arbitrary plugin code.
* feat(auth): add per-user token_epoch column and bump method
* feat(auth): add aud and ep claims, omitted when zero
* feat(auth): add CreateAPIToken for non-expiring, audience-scoped tokens
* feat(auth): add CheckClaims for epoch and audience validation
* feat(jellyfin): issue non-expiring, jellyfin-scoped access tokens
* fix(subsonic): reject API-scoped and revoked tokens on the jwt path
* fix(server): reject API-scoped and revoked tokens on the native API
* fix(server): pin the token-subject guard and stop leaking test config
Adds a regression spec for the DevAutoLogin/ExtAuth guard in
tokenAllowed, switches its comparison to case-insensitive to match
the user lookup's own COLLATE NOCASE semantics, and restores Subsonic
JWT test config after each spec instead of leaking SessionTimeout.
* feat(request): add a token epoch holder for handler-to-middleware signalling
* refactor(server): write the refreshed JWT header after the handler runs
* feat(auth): revoke all tokens for a user when their password changes
* fix(server): restore Unwrap on the JWT refresh writer so SSE write deadlines apply
* test(auth): pin that non-session tokens reject API access tokens
* test(jellyfin): pin token scoping and epoch revocation end to end
Exercises auth.CreateAPIToken and CheckClaims against the real Jellyfin
router and SQLite DB: the minted token has no exp and is aud-scoped to
jellyfin, and bumping token_epoch through the real UserRepository revokes
an already-issued token on the next protected request.
* test(nativeapi): pin the token-epoch handoff through a real password-change request
Drive a self password change through the real Authenticator/JWTRefresher
chain and a real SQLite-backed userRepository, so the epoch handoff between
Put and the refreshed-token writer is verified end to end, not as two
separately-tested halves. Also fix tokenAllowed to read the enriched ctx it
was given instead of r.Context(), so its warning log carries the username.
* refactor(server): drop tokenAllowed's now-unused request parameter
Finding-2 already moved every use to ctx; r was dead weight. Also note
in the new nativeapi test why it must stay the package's only real-DB
spec: db.Db() is a process-wide singleton its cleanup closes for good.
* refactor(auth): remove duplication in claim decoding and token minting
* refactor(auth): group aud with the standard JWT claims
* refactor(auth): read aud with the standard-claim accessor pattern
* fix(log): redact every api_key spelling the Jellyfin API accepts
* fix(auth): bind session tokens to the user id, not just the username
* fix(auth): return the token epoch from the same atomic increment
* fix(auth): bump the token epoch in the same statement as the password write
* chore(auth): trim comments to the why-only budget
* feat(artwork): report what a config-fingerprint backfill enqueued
A backfill re-resolves every entity, and on a large library that is tens of thousands of
external agent calls. It announced itself with a single line carrying nothing but an elapsed
time, so the size of the job was invisible until the request volume showed up hours later.
Log the item count, the per-kind breakdown, and a ceiling on the external lookups the queued
work can cost. The ceiling reuses ExternalLookupsPerItem, the same estimator behind the
`artwork reprocess` preview, so the two agree on what an item can cost.
backfill now returns a summary instead of a bare bool, which keeps the counts assertable
without capturing log output. Worker.Backfill keeps its (bool, error) signature, so its caller
is unchanged, and it reads the agent count off its own resolver.
* refactor(artwork): share the image-agent count and take it lazily
Counting image agents was written twice, once in the CLI for the `artwork reprocess` preview and
again as a resolver method for the backfill log. Two copies of "which agents count as image
agents" can drift, and the CLI estimate and the server log would then disagree silently.
Move the derivation next to the type it builds, as NewImageAgentCount, and call it from both.
The resolver method goes away with it: hanging the census on the resolver forced two nil guards
that its only caller could never trigger, because Worker always builds a resolver with agents.
The Worker keeps the *agents.Agents it is already handed instead of reaching through the
processor and resolver to find it.
Pass the count as a func. Building the agent list constructs every enabled agent (each one an
HTTP client and a cache goroutine) only to take its length, and a backfill returns early on an
unchanged fingerprint, which is what happens on nearly every restart.
* docs(artwork): say what the backfill lookup estimate does not bound
The comment called the number a ceiling, which the CLI comment on the same estimate already
contradicts: externalEstimate "claims no bound". Both are right about the local-source case and
only one of them mentions that a retried item asks its agents again.
* feat(artwork): drip the stale-absent recheck instead of bursting it daily
Each hourly housekeeping tick now re-queues at most 100 absent states
per kind, oldest attempts first, instead of everything older than 24h
at once. External agents see a flat ~100 requests/hour per agent
instead of hourly bursts of ~2,000, and the effective recheck interval
self-scales with the size of the absent pool (~4 days at 10k absent
artists) while small libraries keep the 24h floor.
* feat(artwork): trust an absent artwork state for a week before rechecking
With the recheck now dripped at 100 items per kind per hour, the 24h
floor only governed small libraries, where the drip cap never binds;
they still re-asked every agent daily. A 7-day floor cuts that cost 7x
and, for large libraries, becomes the binding limit over the drip
cycle (~5.7k calls/day instead of ~9.6k at 10k absent artists).
Among comparable servers, this is still the second-most-eager recheck:
gonic retries misses every 30 days, Jellyfin and Funkwhale never do.
* refactor(artwork): state the drip's backpressure contract where it bites
Review follow-ups: the recheck limit deliberately caps the *selection*,
not the insertions — already-queued rows use up budget, so a stalled
drain admits no new work instead of building a recovery burst. Say so
in the interface doc, mirror it in the mock by truncating the sorted
candidates (matching the SQL's LIMIT-before-ON CONFLICT), and teach
`artwork status` and the worker doc the post-drip wording. Also pin
the one cmd fixture that still assumed a 24h recheck window.
* feat(cli): add `artwork cancel` to call off queued artwork work
A bulk backfill had no off switch. Changing an artwork setting bumps the config
fingerprint, which enqueues every entity in the library, and the only way to stop
it was to turn agents off -- which changes the fingerprint again and enqueues a
second full backfill. The escape hatch was the trap.
`artwork cancel` deletes pending queue rows selected by --kind and/or --priority,
with the --dry-run/confirm/-y flow `reprocess` already uses. Cancelling by
priority is the point: it drops a runaway backfill while leaving the bump-priority
rows an operator queued by hand.
It only touches the queue. Resolved artwork and the item_artwork state behind
`artwork explain` are left alone, and the trace of why a cancelled item last
failed goes with its row. Preserving that trace would mean writing it to
last_failure, which `explain` prints under "Gave up after" -- reporting a
cancellation as an exhausted retry budget. The help text says the trace is
discarded instead.
Two limits the help text states, because neither is guessable: work already
dequeued is not interrupted, and an item with no artwork state yet can be queued
again by the hourly missing-artwork recheck. Cancel calls off queued work; it
does not stop the worker.
--kind validates against RefreshableKinds, not the RecheckKinds `reprocess` uses:
the queue holds media file rows, so --all has to reach them. PurgeQueued follows
the repository's naming rule -- it finds its own rows and reports how many went --
and ignores retry_at, since a row still backing off is pending work. The preview
reuses CountByKindAndPriority rather than adding a counter. reprocessConfirm
became confirmUnlessYes(yes, in, verb) now that two commands prompt.
* refactor(cli): share the artwork queue filter between the preview and the delete
Follow-up cleanup on the previous commit; no change to what the command does,
apart from --all, noted below.
The "which rows does cancel touch" predicate was written three times: once as SQL
in PurgeQueued, once in Go in cmd's matchingQueueStats, and once more in the mock.
The preview and the delete could therefore drift, and the mock would keep the
tests green while they did. persistence now has one artworkQueueFilter, shared by
PurgeQueued and a new CountQueued, and cmd does no filtering at all.
That also makes the preview cheaper. It counted the whole queue and filtered in
Go, so `artwork cancel --kind al` scanned every row of every kind to print a
handful. CountQueued pushes the filter into SQL, which the drain index serves as a
range seek. CountByKindAndPriority is gone: it is CountQueued(nil, nil).
--all now selects with an empty filter instead of enumerating RefreshableKinds.
It is what the flag help already claimed, and the enumeration was narrower than
its own documentation -- a queue row whose item_kind this build does not know
survived `--all` with no flag combination able to remove it. It also restores
SQLite's truncate path: measured with EXPLAIN QUERY PLAN, a bare DELETE plans to
nothing, while `WHERE (1=1)` -- which an empty squirrel And renders -- plans to a
full index scan. A test pins the filter's emptiness so that cannot regress
silently.
Also folded together three copies of the parse-and-dedup loop (parseAll), two
copies of the queue-stats table (printQueueStats, now shared with `artwork
status`), two copies of the stat sum (queueTotal), and four copies of the
kind-to-prefix mapping (model.KindPrefixes). The PurgeQueued specs became one
DescribeTable that asserts count and delete agree on every selection.
* docs(cli): say when `artwork cancel` evaluates its selection
The help text covered the two limits that surprise an operator after the fact, but
not the one that bites during the prompt: the count is a preview, and the filters
run again on confirm. A scan or a manual refresh landing in between is cancelled
without ever appearing in the table the operator agreed to.
Deleting only the previewed rows was considered and rejected. The exposure is one
item re-resolving on next view instead of immediately: clearing an item's artwork
state is what every recovery path selects on, so a lost Bump row from
artwork.Refresh comes back at the same priority via provisional() on the next
request, and otherwise within the hour via EnqueueAllMissing. Buying a guarantee
against that costs the truncate path on --all, the flag that exists for a
29k-item backfill.
* refactor(cli): share one set of flag targets across the artwork subcommands
reprocess and cancel each declared their own kinds/all/dry-run/yes variables, but
cobra only ever parses the one subcommand being run, so the two sets could never
hold values at the same time. backup.go already binds one backupDir across two
subcommands and one force across two more; this follows that.
Ten package-level variables become six. Each command keeps its own help string
and its own valid-kind list, so --kind still reports RecheckKinds for reprocess
and RefreshableKinds for cancel, and --source and --priority stay registered only
on the command that has them.
The priority lookup table is now knownPriorities, freeing the artworkPriorities
name for the flag. The new name also reads better against priorityName's fallback
for a value it does not know.
* feat(artwork): record the resolution trace so explain works without --live
The worker never attached a ChainTrace, so `artwork explain` had to re-walk the
priority chain at CLI time. That reconstruction could disagree with what actually
happened, and without --live it could not report the external tier at all.
The worker now traces every acquisition and stores it. `explain` reads the stored
trace by default and reports when it was recorded; --live re-walks and calls the
agents. Disc artwork keeps no row, so it always walks live.
A chain trace alone would have explained almost nothing about failures: six of the
seven ways an item can fail happen after the chain has already picked a winner. The
trace now covers those stages too, and has somewhere to live when they fail: the
retrying queue row carries the last failure, and the state row keeps it in
last_failure once the retry budget is spent and the queue row is deleted.
Measured on a copy of a 682MB / 43.6k-item library: +9.7MB (+1.4%). No row crosses
the WITHOUT ROWID overflow threshold, so list hydration is unchanged; only full
scans of item_artwork, which no request performs, read more pages.
* test(artwork): pin the give-up ordering that keeps a failure for unresolved items
recordGiveUp updates an existing row, and for a kind with a recheck path that row is
only created moments earlier by the absent settle. Recording before the settle would
lose the failure for every item that never resolved, with nothing to catch it.
* refactor(artwork): tighten the trace code after review
Four fixes worth taking:
The doc comments on ChainTrace and chainState.trace still said the worker never
attaches a trace and resolution stays allocation-free — the exact invariant this
branch reverses.
explain's report field meant both "the chain shown was walked just now" and "go out
for real", and was being passed to loadPluginAgents, which --live documents as the
only thing that may open external connections. Renamed to `walked` and restored
explainLive as the sole input to that decision.
A stored Detail is an error string on the failure paths, with no bound. The measured
"no row reaches the WITHOUT ROWID overflow limit" only holds while it is bounded, so
cap it at 200 runes.
offlineGate was a factory returning a constant closure; make it a plain gateFunc like
its sibling passthroughGate. Collapse five copies of the age-a-queue-row loop in the
worker tests into one helper.
* refactor(artwork): drop the offline explain walk, now that traces are stored
`artwork explain` reported the external tier without calling it, so a diagnostic
could not add load to a provider already rate-limiting us. Reading the stored trace
answers that better: it reports what the agents actually returned, not what would
be tried.
Nothing could reach the offline gate any more. It was installed only for a walk
with --live unset, which now happens for disc artwork alone, and disc rejects the
external candidate before any gate call. That made the gate, its sentinel error,
the would-try outcome and two of explain's verdicts unreachable.
Removes offlineGate, errOfflineSkipped, OutcomeWouldTry, the NewTracingResolver
live parameter and the CreateArtworkResolver argument threaded through wire.
Verified against a copy of a real library: disc artwork with "external" first in
DiscArtPriority and external services enabled still records the skip and issues no
agent call.
* fix(artwork): make explain's no-network guarantee structural, not incidental
Serving falls back disc -> album and track -> disc -> album. The resolver layer
explain uses has no such fallback today, so dropping the offline gate did not leak.
But the guarantee rested on which chains happen to lack an external tier, and the
serving layer already shows the fallback shape someone could mirror.
Without --live the tracing resolver is now built with no agents at all, so no chain
and no fallback added later can reach a provider. That is stronger than the gate it
replaces, which only intercepted the call.
The test pins it against exactly that regression: with the guard removed and the
serving fallback mirrored into resolveDisc, it fails.
* refactor(artwork): trim the trace plumbing
EncodeTrace was exported for nobody: only this package writes traces, and cmd reads
them. It becomes a ChainTrace method, which also drops the copy Steps made for a
caller that only wanted to serialize.
explain's report carried queuedSteps and failureSteps, both pure functions of the
queue and state rows already in the struct, which let a test set the two out of step
with each other. formatExplain derives them, as it already does for every other
display value.
The trace row format and its tabwriter empty-cell rule lived in two places, and the
"nothing was ever recorded" predicate in three.
* fix(artwork): clear the queue trace on a fresh re-enqueue
Enqueue's conflict clause reset attempts to 0 but left the new trace
column, so after a scan or refresh re-enqueued a previously-failed item
artwork explain showed "Attempts: 0" next to the prior lifecycle's
"Last attempt failed" trace. Clear trace in Enqueue (a fresh lifecycle
has no last attempt); EnqueuePreservingBackoff still keeps it.
* fix(artwork): treat a processing-stage error as indeterminate in explain
A read/hash/decode/store failure records an OutcomeError step and writes an
absent row, but explainResult only mapped external errors and unreadable
candidates to indeterminate, so the default verdict read "not resolved" —
presenting a processing failure as a definitive miss. The worker retries
these exactly as it retries an unreadable candidate, so classify any
OutcomeError as indeterminate too.
* fix(artwork): record a trace step when a chainless resolver faults
Playlist and radio resolvers walk no priority chain, so a fault (unreadable
upload/sidecar, or an m3u fetch error with no grid) returned localError/extError
without recording any trace step. The attempt then encoded [], leaving artwork
explain with an empty "Last attempt failed" and "Gave up after". Record a
fallback step in the faulted-no-image branch when nothing else did, and carry
the source label through resolveLocalFile so the step can name it.
* fix(artwork): trace the m3u failure at its source, not via the empty guard
A playlist's grid sampling records album-chain steps into the shared trace, so
the processor's empty-trace fallback no longer fires when the m3u remote image
fetch failed — the error that forced the retry was omitted from explain. Record
it where it happens, in resolvePlaylist's external step, as external:m3u.
* test(artwork): skip the chainless-fault spec on Windows
The spec provokes an open fault with a non-directory parent, but Windows maps
that to a not-exist error, so localError is never set and the item resolves
absent instead of failed. The sibling failed-on-unreadable-upload spec skips
Windows for the same class of reason.
* fix(artwork): don't label an absent empty-chain row as pre-tracing
explain reported "resolved before traces were recorded" for any stored row
with an empty chain, but an empty CoverArtPriority records a real, empty [] chain
and resolves absent. A recorded resolution that finds an image always records its
winning candidate, so only a row with a hash and no chain predates tracing; split
on the hash and report an absent empty chain plainly instead.
* fix(db): retimestamp the artwork trace migration after rebase
master merged a 2026-08-18 migration, so the original 2026-08-16 timestamp is now
older than the newest on the base branch and Goose would silently skip it on an
already-upgraded database. Bumped past it; the SQL is unchanged.
* fix(artwork): keep the m3u error detail in the trace
The m3u trace step recorded OutcomeError with no detail because resolveExternalStep
collapsed the gate's error to a bool, so explain showed only "external:m3u error -"
and could not tell a timeout from an HTTP error or an open breaker. Return the error
(normalizing not-found to nil so it stays a definitive miss, not a failure) and store
its message as the step detail; encodeSteps already bounds it.
* docs(artwork): note the give-up write relies on serial draining
recordGiveUp writes last_failure unconditionally; that is only correct because
the drain resolves each item serially, so no concurrent success can store artwork
between the write and the queue delete. Record the invariant at the call site.
A MetadataAgent plugin reports "I have no data for this item" by returning an empty response
with a nil error. Any error it returns instead is treated as a plugin fault and retried with
backoff. That rule was not documented anywhere, so an author naturally returns an error for a
missing item, and Navidrome then retries every item the plugin's source does not cover.
This is not hypothetical: the artist-nfo-metadata plugin returned an error for every artist
without an artist.nfo, which kept those artists in the artwork retry queue for hours and
tripped the artwork circuit breaker for the plugin as a whole.
Document the rule on the capability interface, which ndpgen copies into the Go PDK, and in the
MetadataAgent section of the plugin README.
CI builds were failing at random with:
buildx failed with: toomanyrequests: Rate exceeded
The 429 comes from public.ecr.aws, not Docker Hub. AWS caps unauthenticated
ECR Public pulls at 1 per second per source IP (authenticated: 10/s). The
build matrix starts 11 jobs at once and each resolves 4 base images, so
roughly 44 anonymous pulls land in a couple of seconds — from GitHub runner
IPs that are shared with every other GitHub customer.
Measured against public.ecr.aws with an anonymous token, 60 requests at
concurrency 30 returned 31x 429 in 0.48s.
The same probe against mirror.gcr.io (Google's Docker Hub pull-through
cache) returned zero errors: 750 manifest requests up to ~141 req/s, plus
120 layer blob requests at concurrency 60. Google publishes no rate limit
for it, so this is measured headroom, not a contract — but it is roughly
10x the pipeline's peak rate, and cached pulls do not count against Docker
Hub's limits either.
Authenticating to ECR Public was the alternative. It was rejected because
10 pulls/s is still under the ~44-pull burst, it needs an AWS account plus
a secret, and secrets never reach fork pull requests — so forks would keep
failing. The mirror fixes forks too.
Verified buildkit honours the mirror block by routing a build through a
local logging registry: all 6 requests (manifests and blobs) hit the mirror,
none went to Docker Hub directly. Confirmed mirror.gcr.io answers 200 for
buildkit's "?ns=docker.io" query form on all four images, for both GET and
HEAD. Confirmed buildkit falls back to Docker Hub when the mirror is
unreachable — the build still succeeds, but the resolve takes ~30s instead
of ~0.3s, so a mirror outage means slow builds, not broken ones. The
existing Docker Hub login covers that fallback on main-repo runs.
msitools.dockerfile is only used by the local `make docker-msi` target, but
is switched over too so no ECR Public reference is left behind.
The export path used the `println` builtin, which writes to stderr, so
`navidrome pls -p X > playlist.m3u8` produced an empty file while the M3U
body was interleaved with the startup logs on stderr.
`println` also appended a newline that `ToM3U8` already provides, so the
piped output had a stray trailing blank line that `-o file` did not. Both
destinations are now byte-identical.
The stdout/file choice moved into a `writePlaylist` helper shared by
`pls -p` and `pls export -p`, which both had the same bug. It takes the
destination as an `io.Writer`, matching the existing convention in
cmd/artwork.go.
* ci: lint with the golangci-lint version the Makefile declares
The workflow asked for `version: latest` while the Makefile pins
`GOLANGCI_LINT_VERSION ?= v2.12.0`, so `make lint` and CI ran different
linters. golangci-lint v2.13.0 started reporting G404 on three existing
`rand.Shuffle` calls, which turned every PR red without a line of Go
changing.
Read the version from the Makefile instead of resolving `latest`, so the
two stay in step and a new release cannot break unchanged code.
* chore: re-run CI
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
* feat(cmd): make artwork explain/refresh accept an id without its kind
The kind can now come from the id itself: a full artwork id (al-<id>)
carries it in the prefix, and a bare id is resolved across tables via
GetEntityByID. The explicit <kind> <id> leader still works.
* fix(cmd): keep refreshing resolvable ids when others fail to resolve
resolveArtworkTargets now collects a self-describing id it cannot resolve
as a failure instead of aborting, so refresh reports and skips the bad ones
and still queues the rest, matching refreshItems per-item behavior. explain
stays strict and rejects any unresolved input.
The cosine basis factors into cosX[i][x] * cosY[j][y], so the pixel loop
does not need to visit every (i,j) pair. Each row now collapses to xComp
dot products, folded over yComp once per row: w*h*xComp + h*xComp*yComp
multiply-accumulates instead of w*h*xComp*yComp.
Encoding is ~60% faster at every input size, and ~80% faster at the 128px
size the artwork pipeline actually feeds it (263us -> 53us). Hashes are
byte-identical, so the existing golden-value specs cover the rewrite.
* fix(scanner): detect in-place playlist edits via the folder content hash
Signed-off-by: junkerderprovinz <jdp@braethoria.com>
* docs(scanner): clarify the playlist entries in the folder hash
Shorten the comment on the playlist loop, and record why the playlist count
stays in the hash header: it is redundant with the loop for change detection,
but removing it changes the hashed byte stream for every folder, including
folders without playlists, which would mark every folder outdated on the first
scan after upgrade.
Signed-off-by: junkerderprovinz <jdp@braethoria.com>
* test(scanner): pin filename and size into the playlist hash assertions
The playlist size test called time.Now() twice, so the modtime differed too
and carried the assertion — dropping info.Size() from the hash left the suite
green. It now shares one baseTime. A new rename test swaps the map key with
count, size and modtime held constant, so dropping the filename from the hash
fails. Both mutations were verified to fail before this change and pass after.
---------
Signed-off-by: junkerderprovinz <jdp@braethoria.com>
Co-authored-by: Deluan <deluan@navidrome.org>
* test: unskip AbsolutePath and i18n path-separator tests on Windows (#5381)
Signed-off-by: junkerderprovinz <jdp@braethoria.com>
* test: unskip metadata folder-PID test on Windows via path.Dir (#5381)
Signed-off-by: junkerderprovinz <jdp@braethoria.com>
* test(storage): make relative-folder assertion cross-platform and unskip on Windows (#5381)
Signed-off-by: junkerderprovinz <jdp@braethoria.com>
* fix(persistence): normalize folder-update-info paths with forward slashes on Windows (#5381)
Signed-off-by: junkerderprovinz <jdp@braethoria.com>
* review: drop folder-PID change, trim storage_test comment (#5381)
Revert model/metadata/persistent_ids.go to master: switching the `folder`
PID attribute from filepath.Dir to path.Dir would change the persistent IDs
of existing Windows libraries and needs a migration path, so it is out of
scope for this PR. The matching test unskip is reverted with it, leaving
#TBD-path-sep-metadata open in #5381.
Trim the core/storage/storage_test.go comment to two lines.
Signed-off-by: junkerderprovinz <jdp@braethoria.com>
---------
Signed-off-by: junkerderprovinz <jdp@braethoria.com>
Co-authored-by: Deluan Quintão <deluan@navidrome.org>
