The jwx library always deserializes numeric claims from a parsed token as
float64, so the int and int64 branches in getIntClaim could never succeed
and were dead code. Keep only the float64 path, which is the one actually
exercised by the token round-trip, and update the comment to document the
library behavior.
* fix(smartplaylist): support isMissing/isPresent operators on ReplayGain fields
ReplayGain values are stored in dedicated nullable columns (rg_album_gain,
rg_album_peak, rg_track_gain, rg_track_peak) rather than in the media_file.tags
JSON blob. The isMissing/isPresent operators previously only supported tag and
role fields, causing two failure modes:
1. Using the documented alias names (replaygain_album_gain etc.) from PR #5256:
these got registered as JSON tags from mappings.yaml, so isMissing queried
json_tree(media_file.tags, '$.replaygain_album_gain') which is always empty
-> the playlist matched ALL songs.
2. Using the canonical field names (rgalbumgain etc.): not a tag/role, so SQL
generation returned an error. Because refreshSmartPlaylist deletes old tracks
before regenerating, the abort left the playlist empty.
Fix: add Nullable bool to FieldInfo and mark the four ReplayGain fields. Add
static alias entries (replaygain_album_gain -> rgalbumgain etc.) with
Numeric+Nullable set; because AddTagNames skips names already in the field map,
these static entries take precedence over the mappings.yaml tag registration.
missingExpr now emits IS NULL / IS NOT NULL for nullable column fields instead
of the json_tree lookup.
Fixes#5584
* chore(smartplaylist): address code review feedback
- Simplify the isMissing/isPresent unsupported-field error message,
removing the internal "nullable fields" jargon
- Standardize comments on mappings.yaml (the actual filename)
- Clarify the alias precedence comment in the LookupField test
* fix: load ND_DEFAULTLANGUAGE on app startup
Added in to apply on initial mount, ensuring the locale is set even when the login page is skipped by reverse-proxy authentication. Removed the redundant language-init effect from . Fixes#3605.
* style(ui): format App.jsx with Prettier
Ran Prettier on ui/src/App.jsx to satisfy code style checks after adding default-language useEffect.
* fix(ui): move default language initialization to Admin component
Signed-off-by: Deluan <deluan@navidrome.org>
* fix(ui): streamline locale setting in App component
Signed-off-by: Deluan <deluan@navidrome.org>
---------
Signed-off-by: Deluan <deluan@navidrome.org>
Add Catppuccin Latte (the light version) theme based on the existing Catppuccin Macchiato theme.
The palette and player styling are adapted for light mode while staying as close as practical
to the existing Macchiato theme behavior. I've opted to use gray for the
color for controls.
The dark version appears to mix a few control/accent colors,
so for Latte I standardized those choices. This might be worth looking
into in a separate PR. It uses gray and blue.
Signed-off-by: Love Billenius <lovebillenius@disroot.org>
Co-authored-by: Deluan Quintão <deluan@navidrome.org>
Signed-off-by: Deluan <deluan@navidrome.org>
* Add Moonbase theme
A warm dark theme with gold (#d4a039) accents on deep charcoal
backgrounds (#0a0a09/#141413). Features muted cream text (#e5ddd3),
copper error states (#c45c3c), and subtle earthy secondary tones.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* Fix review comments on Moonbase theme
- Fix CSS selector: use :not(.player-delete) instead of :not([class=".player-delete"])
- Fix MuiFormHelperText override structure: target error key directly
- Remove empty icon: {} and avatar: {} from NDLogin overrides
- Use comma-separated rgba syntax and hex for linear-gradient
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* Add Moonbase Alpha (light) and rename dark to Moonbase Bravo
Split the Moonbase theme into a complementary pair:
- Moonbase Alpha: warm cream/stone light theme with deep gold accents
- Moonbase Bravo: the original deep charcoal dark theme
Both share the same gold (#d4a039) brand accent, copper error states,
and earthy neutral palette. Alpha uses darkened gold (#9a7420) for
better contrast on light backgrounds.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: Deluan Quintão <deluan@navidrome.org>
* fix(ui): update Spanish translations and add missing gain keys
- Add missing 'albumGain' and 'trackGain' keys (matches recent additions in pt-br, ru)
- Translate 'Playlists' and 'Shared Playlists' to 'Listas de reproducción' / '...compartidas'
- Translate 'OFFLINE' (server down indicator) to 'DESCONECTADO'
Spanish translation now covers 553/553 keys (was 551/553).
Signed-off-by: Daniel Banariba <banaribad@gmail.com>
* fix(ui): address review feedback on Spanish translations
- Use 'Ganancia del álbum' (with article 'del') for consistency with the existing pattern in line 624 ('album': 'Ganancia del álbum') and 'Artista del álbum'. Thanks @gemini-code-assist for the catch.
- Revert 'playlists' and 'sharedPlaylists' to keep the loanword 'Playlist(s)' which is the form actually used by Spanish-speaking music app users (Spotify ES, etc.) and matches existing usage elsewhere in this same file (e.g. line 48 'Agregar a la playlist').
Signed-off-by: Daniel Banariba <banaribad@gmail.com>
---------
Signed-off-by: Daniel Banariba <banaribad@gmail.com>
Co-authored-by: Deluan Quintão <deluan@navidrome.org>
* test: fix flaky tests in utils/cache
Two tests in the utils/cache suite were timing- and ordering-dependent
and failed intermittently on CI (notably on the Windows runner).
The FileHaunter tests raced the asynchronous cache-cleanup goroutine with
a fixed 400ms sleep, then asserted the directory state once. On slow
runners the haunter had not finished scrubbing, so the assertion saw the
original files and failed. Replace the fixed sleep with Eventually polling
so the assertions wait for the haunter to converge. While doing so, the
exact set and count of reaped files proved nondeterministic (the empty
file is double-counted in the size loop and LRU survivors depend on
OS access-time ordering), so the assertions now check the haunter's
actual guarantees: the empty file is always scrubbed and the cache stays
within the configured maxSize/maxItems bound. This also lets the
previously-disabled maxItems context and its commented-out assertions be
re-enabled.
The HTTPClient 'caches repeated requests' test relied on a shared
requestsReceived counter that was never reset in BeforeEach. Under
randomized spec order another spec could run first and leave the counter
non-zero, breaking the first assertion. Reset the counter and header in
BeforeEach to make the spec independent of execution order.
Verified with: ginkgo -race -repeat=80 --randomize-all ./utils/cache/
* test: surface errors in dirSize and align Eventually with house style
Address code review feedback on the cache flaky-test fix:
- dirSize now returns (uint64, error) and the maxSize spec asserts the
error is nil. Previously a ReadDir/Info failure silently returned 0,
which always satisfies '<= maxSize' and would mask a real filesystem
error as a passing test.
- dirSize skips non-regular entries (info.Mode().IsRegular()) to match
its doc comment and avoid counting directories or symlinks.
- The Eventually blocks now use .WithTimeout()/.WithPolling() with
time.Duration values instead of string-literal durations, matching the
prevailing pattern in the test suite.
Share repository read methods (Get, GetAll, Read, ReadAll, Exists, Count,
CountAll) did not apply an owner filter, so non-admin users saw shares
belonging to other users. The write paths already enforced per-user ownership;
this brings reads in line with them.
Add an addRestriction()/ownerFilter() based scope to share reads, keeping
admins and the headless public-share resolution path unrestricted. Route share
and player Delete through a new base-repo deleteOwned() primitive that applies
the ownership predicate in the DELETE's WHERE clause (atomic, no select-then-
delete window) and classifies a zero-row result as permission-denied vs
not-found, mirroring updateOwned. The addRestriction helper and the write-miss
classifier are hoisted onto the base repository so player and share share one
implementation.
Also map rest.ErrPermissionDenied and rest.ErrNotFound in the Subsonic error
handler so ownership/not-found failures from the rest-backed repositories
return the proper Subsonic codes (50 / 70) instead of a generic error.
Covered by unit tests (persistence, subsonic error mapping) and an end-to-end
cross-user sharing isolation test.
The native API exposes a `path` query param on /api/song, but it was not
registered in the media file filter map. Unmapped real columns fall through
to a default LIKE predicate that emits an unqualified `path LIKE ?`. Since
the song query joins the library table (which also has a `path` column),
SQLite returned "ambiguous column name: path" and the request failed with
HTTP 500.
Register a dedicated path filter qualified to media_file.path, resolving the
ambiguity. The value is matched with startsWith semantics (LIKE arg || '%')
against the library-relative path stored in media_file.path.
To register it inline (without a one-off wrapper), startsWithFilter now takes
a bound field and returns a filterFunc, mirroring containsFilter. The two
existing callers are updated accordingly, and the now-unused withTableName
helper is removed. The user 'name' filter, which previously relied on
withTableName, is now qualified directly as user.name; tests are added to
guard that filter against the same column-ambiguity class (the user query
also joins the library table, which has a name column).
Signed-off-by: Deluan <deluan@navidrome.org>
* fix(security): restrict transcoding config reads to admins
Authenticated non-admin users could read transcoding configs through
the native API (GET /api/transcoding and /api/transcoding/{id}) when
EnableTranscodingConfig was enabled. The responses included the full
command templates, disclosing admin-configured ffmpeg invocations and
local command paths. Write operations were already admin-only.
The /transcoding route was registered in the general authenticated
group, and only the repository's write methods checked IsAdmin. This
applies the boundary at two layers:
- Move the route under adminOnlyMiddleware, alongside the other
admin-only resources (/library, /config, /inspect).
- Add an IsAdmin guard to the repository's rest.Repository read
methods (Read, ReadAll, Count) as defense-in-depth.
The guard is scoped to the REST methods only. The streaming pipeline
resolves profiles via Get/FindByFormat (model.TranscodingRepository),
which stay open so transcoding keeps working for non-admin users.
Adds regression tests covering non-admin read denial and confirming
non-admin streaming lookups (Get/FindByFormat) still succeed.
* fix(security): redact transcoding Command for non-admins instead of blocking reads
Reworks the previous approach after review (Codex P2): moving /transcoding
under adminOnlyMiddleware and denying non-admin reads broke legitimate
non-admin UI flows. The web UI reads the transcoding resource as a regular
user in several places that need only the profile name and target format:
the player edit dropdown (ReferenceInput), the player list (ReferenceField),
and the share/download format pickers (useGetList -> {targetFormat, name}).
The only sensitive field is Command (the admin-owned ffmpeg template). So:
- Revert the route move; /transcoding stays in the authenticated group.
- Read/ReadAll now return the profiles to any authenticated user but blank
the Command field for non-admins (mirrors user_repository's field-level
redaction). Count is no longer denied (the UI needs list pagination).
- Writes remain admin-only (Save/Update/Delete/Put).
- Streaming is unaffected: it resolves profiles via Get/FindByFormat, which
are not redacted, so on-the-fly transcoding keeps working for non-admins.
Tests updated: non-admin reads succeed with Command blank, admin reads keep
Command, non-admin Get/FindByFormat keep Command, writes still denied.
* fix(player): enforce ownership atomically on player update
The native API PUT /api/player/{id} authorized writes using the userId in
the request body via isPermitted, while the actual write targeted the row
by the URL id. A non-admin user could set userId to their own id in the
body to pass the check, then overwrite and reassign ownership of another
user's player row identified by the URL id (cross-tenant takeover).
Add updateOwned on the base repository: an atomic, ownership-restricted
UPDATE that folds the owner predicate (user_id = caller) into the WHERE
clause for non-admins, so a row owned by another user simply does not
match and no write happens. It also never writes user_id, so ownership is
immutable on update and no caller (admin included) can reassign a player
to a different owner. Unlike put, it never falls through to an INSERT, so
a non-matching id returns ErrNotFound instead of creating a row.
playerRepository.Update now uses updateOwned. Extract filterUpdateValues,
shared by put and updateOwned, so the update-column filtering lives in one
place. The create path (Save) keeps the body-based isPermitted check,
which is correct for new records.
Add regression tests covering the spoofed-userId hijack, regular-user and
admin ownership reassignment, legitimate owner updates, and the
nonexistent-player case.
* fix(share): enforce ownership atomically on share update
shareRepository.Update authorized writes with a separate checkOwnership
SELECT, then wrote the row via put(). The check and the write were two
statements (a TOCTOU window), put() could fall through to an INSERT on a
missing id, and put() would write user_id if present in the update
columns, so ownership was mutable on update.
Switch Update to updateOwned, which folds the owner predicate into the
UPDATE's WHERE clause, never writes user_id, and never inserts. This
makes the write atomic and ownership immutable, and drops the extra
ownership SELECT on the happy path.
To preserve the previous 403/404 distinction, updateOwned now classifies
a non-matching id: it runs a follow-up existence check only on the
failure path (count == 0, where no write happened, so no TOCTOU) and
returns ErrPermissionDenied when the row exists but is owned by another
user, ErrNotFound when the id is missing. The player path inherits this:
its tests now expect ErrPermissionDenied for a non-owner targeting an
existing row, and ErrNotFound only for a genuinely missing id.
Add share regression tests for the nonexistent-id and ownership-
reassignment cases. checkOwnership remains in use by Delete.
* refactor(persistence): extract canonical ownerFilter predicate
The non-admin owner-restriction predicate (user_id = me, exempting admins
and headless contexts) was spelled out independently in updateOwned and in
playerRepository.addRestriction. The two copies had drifted: addRestriction
did not exempt the headless/invalid user, so a headless context restricted
to user_id = "-1" (matching nothing) while updateOwned exempted it.
Extract sqlRepository.ownerFilter as the single definition and route both
call sites through it. addRestriction now exempts the headless user too;
that path is only reachable from the authenticated native API, so there is
no production behavior change, but the latent divergence is removed.
playlistRepository.userFilter is intentionally left alone: it encodes a
different policy (public OR owner_id = me, on the owner_id column).
* fix(share): preserve all-columns update path in Update
shareRepository.Update unconditionally appended "updated_at" to cols.
filterUpdateValues treats an empty cols as "update every column", so when
a caller passes no columns, appending "updated_at" turned an all-columns
update into an updated_at-only one, silently dropping every other field.
The REST controller always populates cols from the request-body field
names, so this path is not reachable through the native API and the
behavior was latent (and pre-existing). Guard the append so the
all-columns path is preserved, and add a regression test that updates
with no columns and asserts the other fields persist.
Signed-off-by: Deluan <deluan@navidrome.org>
---------
Signed-off-by: Deluan <deluan@navidrome.org>
* fix(scrobbler): proxy NowPlaying even when ignoreScrobble is set
When a client reports playback with ignoreScrobble=true, the reportPlayback
handler suppressed both the scrobble submission and the NowPlaying update sent
to external agents (Last.fm, ListenBrainz, plugins). These are independent
concerns: ignoring the scrobble submission should not stop Navidrome from
telling external services what is currently playing.
The !params.IgnoreScrobble guard now applies only to the scrobble submission
and play-count path; the NowPlaying dispatch is gated solely by the player's
ScrobbleEnabled flag. This mirrors the legacy scrobble endpoint, where
submission=false has always still set NowPlaying.
* test(scrobbler): assert no scrobble dispatch when ignoreScrobble=true
Address PR review feedback: explicitly verify that ignoreScrobble=true
suppresses the scrobble submission (not just the play count) while NowPlaying
is still dispatched, so the flag cannot regress into ignoring nothing. Also
expand the NowPlaying gating comment to spell out the IgnoreScrobble vs
ScrobbleEnabled rules and identify the external agents involved.
* Add Gruvbox Dark theme
Add Gruvbox Dark color theme including:
- gruvboxDark.js with full palette and component overrides
- gruvboxDark.css.js with custom player styles
* Fix: move error state to MuiFormHelperText
The three It blocks that build a tight-cap streamer each spawned a fresh
transcoding cache without waiting for its background initialization. The
init goroutine reads conf.Server.CacheFolder, which races against
SnapshotConfig's pointer-swap restore (Server = &restored) fired by
DeferCleanup at the end of the spec. CI tripped the race under
-shuffle=on -race; locally it reproduced about 10% of the time.
Wait for tightCache.Available() before constructing the streamer, mirroring
the outer BeforeEach. For the slot-saturation spec, swap in a blocking
io.Pipe-backed mock ffmpeg so the cache's background copyAndClose can't
drain the source and release the slot — the previous behavior happened to
work only because the cache wasn't yet available and the no-cache path was
exercised.
* fix(playlists): preserve unchanged fields on partial REST updates (#5541)
The REST adapter for playlists was discarding the `cols` argument that
rest.Put provides (the list of fields actually present in the JSON
body). updatePlaylistEntity then compared the deserialized entity's
zero-valued Name/Comment against the DB row, decided "content changed",
and called updateMetadata with &entity.Name — overwriting the name with
the empty string.
This surfaced via the Playlists list view's bulk "Make Public" action,
which sends N parallel `PUT /api/playlist/{id}` requests with body
`{"public": true}`. Affected playlists ended up with their names wiped
(UI showed "Loading..." indefinitely). The per-row Public toggle was
unaffected because it spreads the full record into the payload.
Honor the cols list: gate every field-change check and every pointer
passed to updateMetadata by whether the field was actually in the
request body. Empty cols falls back to the existing "treat as a full
record" behavior so non-REST callers are unaffected.
* test(playlists): cover rules-only PUT + case-variant owner-change guard
Follow-ups from manual testing and code review of the prior commit:
- Manual testing confirmed Feishin-style rules-only PUT works correctly
on the fix; add ginkgo regression tests for rules-only update, name+
rules combined, idempotent rules PUT (no-op), and bulk Make-Public
preserving rules on smart playlists.
- Keep the non-admin owner-change permission check gated on the
deserialized entity content (not on `sent("ownerId")`) so a
case-variant JSON key like {"OwnerId":"x"} can't downgrade the 403
to a silent 200. Go's json decoder is case-insensitive on struct
field matching but rest.Put's field-name extraction is case-
sensitive; the entity-based guard catches both spellings. The
apply-side gating on ownerChanged still prevents the actual mutation,
so this was a behavioral (not security) regression, but worth fixing.
Adds a regression test asserting the case-variant key still returns
rest.ErrPermissionDenied.
- Correct misleading doc on applyContentUpdate: the path does not
rewrite the backing M3U file; it goes through updateMetadata which
bumps updatedAt and invalidates cached cover-art URLs.
* fix(playlists): match REST cols case-insensitively (PR #5542 review)
Go's encoding/json populates struct fields from case-variant keys like
{"Name":"x"} or {"OwnerId":"y"}, but rest.Put's getFieldNames extracts
raw JSON keys verbatim. With case-sensitive matching, sentFields would
ignore the field on the update side — a request with {"Name":"Renamed"}
would parse into entity.Name but then sent("name") returns false and
the rename silently no-ops.
Normalize both sides to lowercase. The entity-based owner-permission
guard added in the previous commit remains as belt-and-suspenders but
is now redundant with this change.
Also clarify the applyContentUpdate doc comment: namePtr/commentPtr
are nil when the field is absent OR present-but-unchanged, while
publicPtr only tracks presence (an idempotent public is still forwarded).
* refactor(playlists): drop redundant entity-based owner-permission guard
The case-insensitive sentFields predicate already prevents case-variant
JSON keys like {"OwnerId":"x"} from bypassing the ownerChanged check, so
the duplicated entity-content guard is no longer load-bearing.
Strengthen the regression test into a DescribeTable covering canonical,
PascalCase, all-upper, and all-lower spellings to lock in the
case-insensitive contract.
Dir embedded sync.Once directly and exposed a value-receiver GoString so
that pretty.Sprintf("%# v", Server) could render the path. That meant
every pretty-print copied the entire Dir along with its Once, and a
goroutine concurrently using the original (or any copy) for Path() could
hit a "sync: unlock of unlocked mutex" runtime fatal error. The failure
was reproduced deterministically on Windows CI when test-suite shuffle
ordering raced cache initialization (utils/cache/file_caches.go's
NewFileCache.func1 -> conf.CacheFolder.MustPath) against the
configuration-dump pretty.Sprintf in Load().
Drop the sync.Once entirely. Dir is now a plain {path, perm} value type,
and Path() calls os.MkdirAll on every invocation. MkdirAll is
idempotent, so repeated calls on an existing directory cost one stat
syscall — negligible for the few config paths read at startup and during
cache init.
This removes the entire class of bug:
- No Mutex, so copies (via reflection, pretty-print, etc.) are safe.
- No state pointer, so no nil-state defensive checks scattered across
methods, and no risk of two copies seeing different lifecycle state.
- go vet is happy with the value receivers — the //nolint:govet
suppression on GoString is gone.
Adds two regression tests in conf/dir_test.go:
- GoString renders Dir as a quoted path under pretty.Sprintf (and
does not leak the internal struct fields).
- Concurrent copy + Path() stress test, locking in the copy-safety
property in case the type ever grows non-trivial state again.
Move the option into the nested Transcoding config group alongside the
limit knobs it interacts with, so all transcoding-related settings live
together.
The old top-level name is still honored via the existing
mapDeprecatedOption / logDeprecatedOptions plumbing, which forwards the
value to the new key and logs a deprecation warning at startup. The old
struct field is removed (the new field is the single source of truth);
the deprecated default is removed so viper.IsSet correctly distinguishes
"user set the legacy option" from "no one set it."
* feat(transcoding): add MaxConcurrent and MaxConcurrentPerUser config
Introduce Transcoding.MaxConcurrent (default NumCPU()*2) and
Transcoding.MaxConcurrentPerUser (default 3) to support upcoming
concurrency limits on the streaming pipeline. No behavior change yet.
Refs #5246
* feat(transcoding): add TranscodeLimiter with global and per-user caps
Introduce a non-blocking limiter that gates concurrent transcodes. Returns
ErrTooManyTranscodes immediately when the cap is reached so callers can
translate it into a 429 response, rather than queuing requests.
The per-user reservation is taken first to avoid burning a global slot
that would only be rolled back when the per-user cap rejects the caller.
Release is idempotent so wrapping the transcoder reader's Close is safe.
Refs #5246
* feat(transcoding): cap concurrent transcodes in media streamer
Acquire a TranscodeLimiter slot before spawning ffmpeg in the transcoding
cache's read function, and release it when the resulting reader is closed.
Raw streams and cache hits bypass the limiter so a single saturating client
cannot block ordinary playback.
When the cap is reached, ErrTooManyTranscodes bubbles up through cache.Get,
ready for the HTTP layer to translate into a 429 response.
Refs #5246
* feat(transcoding): return HTTP 429 with Retry-After when transcode cap is hit
Map stream.ErrTooManyTranscodes to HTTP 429 in both the Subsonic API
(/stream, /download) and the public share endpoint, including a 5s
Retry-After hint. The Subsonic response still carries a failed-status
envelope so clients that ignore HTTP codes also see the failure.
Refs #5246
* feat(transcoding): default MaxConcurrent to 0 (disabled)
Ship the limiter opt-in so existing installations are not affected by a
behavior change on upgrade. Users hitting the DoS reported in #5246 can
enable it by setting Transcoding.MaxConcurrent to a positive value
(NumCPU()*2 is a reasonable starting point).
Refs #5246
* fix(transcoding): make global and per-user caps independent
Previously the limiter short-circuited to a no-op whenever MaxConcurrent
was zero, silently ignoring a configured MaxConcurrentPerUser. Treat each
cap independently so an operator can throttle per-user without enforcing
a global ceiling (or vice versa), and only fall back to the no-op limiter
when both caps are disabled.
* fix(archiver): abort archive download when the transcode limiter rejects
The album/artist/playlist zip writers were silently producing zip entries
with headers but no data when ms.NewStream returned ErrTooManyTranscodes,
because the per-file error was discarded by `_ = a.addFileToZip(...)`.
The client received HTTP 200 with a corrupt zip and no indication that
the server was rate-limited.
Now the zip loop bails out as soon as it sees ErrTooManyTranscodes, and
the Download handler swallows the error (the response status and
Content-Disposition are already flushed by the time the limit is hit, so
no 429 can be sent). The truncated zip surfaces the problem to the
client; operators see a clear "transcode cap reached" warning in the
server logs.
Refs #5246
* fix(transcoding): release limiter slot on client close, not ffmpeg EOF
Previously the slot was wrapped around the ffmpeg source reader, so it
was only released by the cache's background copyAndClose goroutine when
ffmpeg finished producing the file — meaning a client that disconnected
after a single byte still held the slot for the full transcode duration.
Under MaxConcurrent=N this serialized fresh requests behind abandoned
encodes for minutes.
Hand the release function back from the cache producer via the streamJob
struct and wire it into the consumer-side Stream.Close. The HTTP handler
already runs `defer stream.Close()`, so disconnect now frees the slot
immediately. Cache hits never enter the producer and still pay no slot,
and singleflight waiters on the same key correctly inherit no release
(only the original producer's job holds the slot).
Refs #5246
* fix(transcoding): skip per-user cap for anonymous requests
Public share viewers have no user in context, so userName(ctx) returned
the literal string "UNKNOWN" and the limiter mapped every anonymous
viewer to the same bucket. With MaxConcurrentPerUser=N, only N
unrelated anonymous clients could stream a viral share at any time —
the opposite of the fairness the per-user cap is meant to provide.
Introduce a limiterKey(ctx) helper that returns "" for anonymous
callers (userName(ctx) is unchanged for logs), and teach Acquire to
skip the per-user reservation when the key is empty. The global cap is
still enforced for anonymous traffic and remains the protection against
runaway anonymous load.
Refs #5246
* refactor(transcoding): tidy limiter struct and centralize Retry-After
Per review feedback:
- Drop the redundant maxConcurrent field on transcodeLimiter; the channel
capacity already enforces the global cap and the field was only used
inside the constructor.
- Only allocate the perUser map when MaxConcurrentPerUser > 0.
- Move the Retry-After value into core/stream as RetryAfterSeconds so the
Subsonic API and public-share handlers cannot drift if the window is
later tuned.
* fix(transcoding): do not log limiter rejections as cache failures
NewStream was emitting an error-level "Error accessing transcoding cache"
log whenever cache.Get returned anything non-nil, including the limiter's
ErrTooManyTranscodes — even though the producer had already logged the
rejection at warn level. The result was double logging and a misleading
"cache failure" classification that buries real cache problems.
Skip the error log when the cause is ErrTooManyTranscodes; the warn line
from the producer is the canonical signal.
* fix(archiver): open stream before writing zip entry header
Per review: addFileToZip previously called z.CreateHeader before
NewStream, so when the limiter rejected a transcode the zip already
contained a 0-byte entry for that track. Open the source first and only
write the header once the read side is ready; rejections now skip the
entry entirely.
The truncation comment in handleArchiveErr was also misleading — z.Close
finalises the central directory, so the client receives a well-formed
zip containing only the tracks written before the rejection, not a
"truncated" archive. Reword to match reality.
* fix(transcoding): hold slot for ffmpeg lifetime, force cancellable ctx
The previous release-on-consumer-close design let a client open many
unique transcodes, disconnect immediately, and still spawn the
configured cap's worth of ffmpeg processes — the cache writer goroutine
continued draining ffmpeg to disk after the client disappeared, defeating
the DoS protection the limiter is meant to provide.
Move the release back onto the source reader so the slot is freed only
when ffmpeg actually exits (either EOF or context cancellation). To keep
disconnects from leaking slots for the full transcode duration, force
the request context into ffmpeg whenever the limiter is enabled — so
client disconnect cancels the process and frees the slot promptly.
When the limiter is disabled, the legacy EnableTranscodingCancellation
behavior is preserved unchanged.
Reported by codex and Copilot reviewers on #5522.
