mirror of
https://github.com/navidrome/navidrome.git
synced 2026-08-31 07:30:32 +00:00
5 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Deluan Quintão
|
97da9993d7
|
fix(stream): abort the response when a transcoded stream is truncated (#6035)
* 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. |
||
|
Deluan Quintão
|
823d851b75
|
refactor(transcoding): rename EnableTranscodingCancellation to Transcoding.EnableCancellation (#5523)
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." |
||
|
Deluan Quintão
|
945d0ba1e2
|
fix(transcoding): cap concurrent transcodes to prevent ffmpeg DoS (#5522)
* 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. |
||
|
Deluan Quintão
|
3f7226d253
|
fix(server): improve transcoding failure diagnostics and error responses (#5227)
* fix(server): capture ffmpeg stderr and warn on empty transcoded output When ffmpeg fails during transcoding (e.g., missing codec like libopus), the error was silently discarded because stderr was sent to io.Discard and the HTTP response returned 200 OK with a 0-byte body. - Capture ffmpeg stderr in a bounded buffer (4KB) and include it in the error message when the process exits with a non-zero status code - Log a warning when transcoded output is 0 bytes, guiding users to check codec support and enable Trace logging for details - Remove log level guard so transcoding errors are always logged, not just at Debug level Signed-off-by: Deluan <deluan@navidrome.org> * fix(server): return proper error responses for empty transcoded output Instead of returning HTTP 200 with 0-byte body when transcoding fails, return a Subsonic error response (for stream/download/getTranscodeStream) or HTTP 500 (for public shared streams). This gives clients a clear signal that the request failed rather than a misleading empty success. Signed-off-by: Deluan <deluan@navidrome.org> * test(e2e): add tests for empty transcoded stream error responses Add E2E tests verifying that stream and download endpoints return Subsonic error responses when transcoding produces empty output. Extend spyStreamer with SimulateEmptyStream and SimulateError fields to support failure injection in tests. Signed-off-by: Deluan <deluan@navidrome.org> * refactor(server): extract stream serving logic into Stream.Serve method Extract the duplicated non-seekable stream serving logic (header setup, estimateContentLength, HEAD draining, io.Copy with error/empty detection) from server/subsonic/stream.go and server/public/handle_streams.go into a single Stream.Serve method on core/stream. Both callers now delegate to it, eliminating ~30 lines of near-identical code. * fix(server): return 200 with empty body for stream/download on empty transcoded output Don't return a Subsonic error response when transcoding produces empty output on stream/download endpoints — just log the error and return 200 with an empty body. The getTranscodeStream and public share endpoints still return HTTP 500 for empty output. Stream.Serve now returns (int64, error) so callers can check the byte count. --------- Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
Deluan Quintão
|
767744a301
|
refactor: rename core/transcode to core/stream, simplify MediaStreamer (#5166)
* refactor: rename core/transcode directory to core/stream * refactor: update all imports from core/transcode to core/stream * refactor: rename exported symbols to fit core/stream package name * refactor: simplify MediaStreamer interface to single NewStream method Remove the two-method interface (NewStream + DoStream) in favor of a single NewStream(ctx, mf, req) method. Callers are now responsible for fetching the MediaFile before calling NewStream. This removes the implicit DB lookup from the streamer, making it a pure streaming concern. * refactor: update all callers from DoStream to NewStream * chore: update wire_gen.go and stale comment for core/stream rename * refactor: update wire command to handle GO_BUILD_TAGS correctly Signed-off-by: Deluan <deluan@navidrome.org> * fix: distinguish not-found from internal errors in public stream handler * refactor: remove unused ID field from stream.Request * refactor: simplify ResolveRequestFromToken to receive *model.MediaFile Move MediaFile fetching responsibility to callers, making the method focused on token validation and request resolution. Remove ErrMediaNotFound (no longer produced). Update GetTranscodeStream handler to fetch the media file before calling ResolveRequestFromToken. * refactor: extend tokenTTL from 12 to 48 hours Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org> |