fix(playback): route v3 direct play and remux through proxy nodes - #620
Conversation
Protocol v3 consulted the node planner only for the HLS deliveries, so
`original_http` and `server_remux_progressive` sessions returned an
API-local `/stream/{session_id}` URL and the API node served the bytes —
ServeDirectPlay for direct play, a locally spawned ffmpeg for the remux.
An operator running dedicated proxy nodes still saw all of that egress on
the API node.
The capability already existed: the proxy implements /stream/direct and
/stream/remux, and the Jellyfin-compat transport already plans a proxy for
exactly these two methods. Native v3 was the only surface skipping it, so
Jellyfin clients routed correctly on a deployment where Silo's own clients
did not. This wires the same shape into the v3 identity transport rather
than inventing a second selection path.
The proxy serves from the stream token alone, so the token now carries the
media path, the file's Dolby Vision profile (a P7 remux must strip the
dangling RPU) and the audio-only flag (which picks audio/mp4 over
video/mp4, the MIME the plan promised). RecipeCard models none of the
three; a missing claim would not fail loudly, it would serve a subtly
different stream than the plan promised.
Two related fixes:
- Proxy direct play served via http.ServeFile, which sets no strong ETag.
direct_stream_resume_v1 depends on the ETag ServeDirectPlay sets before
ServeContent, so routing direct play to a proxy without this would have
silently broken resumable direct streams: If-Range never validates and a
resumed range restarts at 200. The proxy now uses the same serve path.
- playback.local_transcode_fallback was only checked in the HLS branch, so
a progressive remux that converts audio still spawned ffmpeg locally on
an API-only node with the setting disabled. Identity deliveries now
honor the gate too — direct play still falls back locally, since moving
bytes is not transcode work and single-node deployments must keep
working.
Falling back to the API-local path when no proxy is eligible preserves
single-node behavior, and a planner reservation is released whenever the
session does not actually reach a proxy.
Closes #619
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 41 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughProtocol v3 direct-play and progressive-remux sessions now plan proxy delivery, sign proxy stream claims, validate proxy capabilities, enforce local fallback rules, and release reservations during rollback. Remote sessions receive widened idle grace periods. Proxy direct playback uses ChangesProtocol v3 proxy playback
Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: ⚪ Minimal · up to The change routes v3 direct-play and progressive remux through eligible proxy nodes while preserving local fallback, and the supplied evidence identifies no actionable merge-blocking correctness, availability, or compatibility risk. It is merge-ready after normal checks. Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant NodePlanner
participant Proxy
participant SessionManager
Client->>API: Request protocol v3 playback plan
API->>NodePlanner: Reserve proxy session with bitrate
NodePlanner-->>API: Proxy reservation and capabilities
API->>API: Sign stream claims
API->>SessionManager: Mark session as remotely transported
API-->>Client: Return proxy stream URL
Client->>Proxy: Request direct or remux stream
Proxy-->>Client: Serve playback bytes
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/api/handlers/playback_v3_test.go (2)
3724-3724: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo identity-transport tests hardcode transformation identity instead of using the exported constants. Both construct
playback.TransformationV3{Name: "audio_to_aac", Executor: "server", ...}with literals.planRequiresServerTransformationsV3matches on those exact values, so a constant change would leave both tests passing while asserting a transformation the planner no longer emits.
internal/api/handlers/playback_v3_test.go#L3724-L3724: replace the literals withplayback.TransformationAudioToAACV3andplayback.ExecutorServerV3.internal/api/handlers/playback_v3_test.go#L3790-L3790: replace the same literals withplayback.TransformationAudioToAACV3andplayback.ExecutorServerV3.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/api/handlers/playback_v3_test.go` at line 3724, Update both transformation constructions in internal/api/handlers/playback_v3_test.go at lines 3724 and 3790 to use playback.TransformationAudioToAACV3 and playback.ExecutorServerV3 instead of hardcoded name and executor literals, preserving the existing recipe version and test behavior.
3663-3670: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the effective-recipe bitrate branch.
identityProxyPlanV3sets onlySource.BitrateKbps, so every test exercises the source fallback inidentityStreamBitrateKbpsV3. The effective-recipe branch is the one that matters for a downmixing remux, because it reports a lower egress than the source. It is currently untested.Add one case that sets
EffectiveRecipe.BitrateKbpsbelow the source bitrate and asserts the planner receives the effective value.♻️ Proposed helper change
-func identityProxyPlanV3(delivery playback.DeliveryV3, transformations ...playback.TransformationV3) *playback.PlanV3 { - return &playback.PlanV3{ - PlanID: "plan:identity-proxy", - Delivery: delivery, - Transformations: transformations, - Source: playback.SourceDescriptorV3{BitrateKbps: 5_673}, - } -} +func identityProxyPlanV3(delivery playback.DeliveryV3, transformations ...playback.TransformationV3) *playback.PlanV3 { + return &playback.PlanV3{ + PlanID: "plan:identity-proxy", + Delivery: delivery, + Transformations: transformations, + Source: playback.SourceDescriptorV3{BitrateKbps: 5_673}, + } +} + +// identityProxyPlanWithEffectiveBitrateV3 pins the effective-recipe estimate, +// which outranks the source bitrate for proxy bandwidth admission. +func identityProxyPlanWithEffectiveBitrateV3(delivery playback.DeliveryV3, kbps int, transformations ...playback.TransformationV3) *playback.PlanV3 { + plan := identityProxyPlanV3(delivery, transformations...) + plan.EffectiveRecipe.BitrateKbps = &kbps + return plan +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/api/handlers/playback_v3_test.go` around lines 3663 - 3670, Extend identityProxyPlanV3 or add a focused test case so EffectiveRecipe.BitrateKbps is set below Source.BitrateKbps, then assert the planner uses that effective bitrate through identityStreamBitrateKbpsV3 rather than the source fallback. Keep the existing source-only coverage intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/api/handlers/playback_v3_test.go`:
- Line 3724: Update both transformation constructions in
internal/api/handlers/playback_v3_test.go at lines 3724 and 3790 to use
playback.TransformationAudioToAACV3 and playback.ExecutorServerV3 instead of
hardcoded name and executor literals, preserving the existing recipe version and
test behavior.
- Around line 3663-3670: Extend identityProxyPlanV3 or add a focused test case
so EffectiveRecipe.BitrateKbps is set below Source.BitrateKbps, then assert the
planner uses that effective bitrate through identityStreamBitrateKbpsV3 rather
than the source fallback. Keep the existing source-only coverage intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1742a99a-145c-4c97-bcd0-ca190b34e298
📒 Files selected for processing (4)
internal/api/handlers/playback.gointernal/api/handlers/playback_v3.gointernal/api/handlers/playback_v3_test.gointernal/proxy/server.go
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2716856201
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // ServeDirectPlay sets before ServeContent (ServeFile sets none, so | ||
| // If-Range never validates and a resumed range silently restarts at 200), | ||
| // and it carries the rolling write deadline and stream metrics with it. | ||
| _ = playback.ServeDirectPlay(w, r, claims.MediaPath) |
There was a problem hiding this comment.
When a proxy client stops consuming data without closing its connection, this call's RollingDeadlineWriter cannot install its stall deadline: the request arrived through meterEgress, whose meteredResponseWriter implements neither Unwrap nor SetWriteDeadline, while the standalone proxy has WriteTimeout: 0. Consequently ServeContent can block indefinitely, retaining the tracked job, file, goroutine, and connection; add Unwrap to the metered wrapper so http.ResponseController can reach the underlying server writer.
AGENTS.md reference: AGENTS.md:L12-L14
Useful? React with 👍 / 👎.
| if proxyErr != nil { | ||
| return preparedTransportV3{}, proxyErr | ||
| } | ||
| streamURL, servedByProxy := h.identityStreamURLV3(&routeSession, file, proxyNode) |
There was a problem hiding this comment.
When API progress heartbeats are interrupted for more than the 45-second active grace while the absolute proxy stream remains healthy, this routing bypasses HandleStream and therefore never calls BeginTransport or otherwise refreshes the API's session activity. The reconciler can then remove the live session, causing later progress, stop, and replan calls to return session-not-found even though the proxy continues sending media; proxy transport activity needs to participate in the authoritative session-liveness mechanism.
AGENTS.md reference: AGENTS.md:L12-L14
Useful? React with 👍 / 👎.
| // Reserve against the session id the rest of the transport uses, so a | ||
| // re-plan replaces its own reservation instead of double-counting, and the | ||
| // rollback path can release it. | ||
| plan := h.NodePlanner.PlanSession(sessionID, "", false, identityStreamBitrateKbpsV3(result)) |
There was a problem hiding this comment.
When proxy nodes have a different FFmpeg build or configuration from the API node, such as during a rolling upgrade, this selects any healthy proxy using only load and bandwidth. Progressive-remux planning explicitly validates server transformations against the API's local registry, but unlike the HLS offload path no capabilities are fetched or checked for the proxy that will actually execute them; a validated audio conversion or Dolby Vision strip can therefore fail with a 500 on that proxy. Select only a proxy proven to support the frozen transformations, or retain local execution when compatibility is unknown.
AGENTS.md reference: AGENTS.md:L12-L14
Useful? React with 👍 / 👎.
Addresses three P1 findings on the proxy-transport change. Proxies do run ffmpeg — /stream/remux converts audio and strips Dolby Vision RPUs — but they exposed no capability endpoint, so unlike the HLS offload path nothing checked that the selected proxy could execute the transformations a plan froze. A pool whose proxies carry a different ffmpeg build (rolling upgrade, custom image) would fail at stream time: a missing aac encoder 500s, a missing dovi_rpu filter is refused outright by the remux itself. Proxies now serve /hw-capabilities in the same shape and at the same path as a transcode node, and identity planning validates the frozen recipe against the selected proxy, falling back to a node that can do the work. A proxy that does not answer is treated as incapable rather than assumed good: an older proxy predating the endpoint is exactly the mismatched build the check exists to catch. Direct play copies bytes and needs no recipe, so it skips the probe entirely. meteredResponseWriter implemented neither Unwrap nor SetWriteDeadline, so RollingDeadlineWriter could not install its stall deadline on any proxy stream. With the standalone proxy running WriteTimeout 0 there was no server-level guard behind it, so a client that stopped reading without closing its connection would block a write forever, holding the session, the file, the goroutine and the connection. A proxy-served session never produces a transport request on the API node, so activeTransportCount — what protects a local stream from the idle reaper — stays zero and a heartbeat gap longer than the active grace would reap a healthy stream, after which progress, stop and replan all fail with session-not-found while bytes still flow. Sessions are now marked as remotely transported, which widens their idle windows rather than granting immunity: this manager has no absolute session lifetime, so unconditional immunity would leak a session forever when a client disappears without stopping. The mark is always set on commit, so a re-plan that moves a session back onto the API clears a stale one. Also adopts the exported transformation constants in the tests and covers the effective-recipe bitrate branch, per review. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
All three P1 findings were valid and are fixed in 811c85d. Thanks — the capability one in particular was a genuine hole in the design, not just a hardening nit. Do proxy nodes even have remux capabilities? Yes, but not provably, which was the actual bug. The proxy really does run ffmpeg ( Fixed by giving the proxy the same endpoint, in the same shape and at the same path as a transcode node, and validating the frozen recipe against the selected proxy before committing to it. Two deliberate choices:
Write deadlines. Confirmed: Session liveness. Also confirmed, with one clarification: this exposure is pre-existing rather than introduced here — I did not implement it as unconditional immunity. Both CodeRabbit nitpicks applied as well — exported transformation constants in the tests, and coverage for the effective-recipe bitrate branch, which matters because a downmixing remux egresses less than its source. New tests: proxy rejected when it lacks the recipe (asserting the reservation is released), direct play skips the probe, effective-vs-source bitrate selection, proxy capability endpoint + its auth, and four in Verification: Unrelated, but flagging since it will show up in CI: |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 811c85d501
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| base := strings.TrimRight(proxyNode.URL, "/") | ||
| if s.PlayMethod == playback.PlayRemux { | ||
| return base + "/stream/remux/" + token, true | ||
| } | ||
| return base + "/stream/direct/" + token, true |
There was a problem hiding this comment.
When the API process restarts during a proxy-served direct-play or progressive-remux session, these absolute URLs keep all media requests on the proxy, so the reconstruction token never reaches the API node that lost its in-memory session. The next progress, stop, or replan request only calls GetSession and returns session-not-found, whereas the previous API-local /stream/{session} URL carried ?st= through HandleStream and reconstructed the session. The new liveness mark does not cover process restarts; proxy transport needs a way to recreate or restore the authoritative API session.
AGENTS.md reference: AGENTS.md:L12-L14
Useful? React with 👍 / 👎.
| if err := h.proxyCanExecutePlanV3(r.Context(), plan.ProxyNode.URL, result); err != nil { | ||
| slog.WarnContext(r.Context(), "protocol v3 proxy capability mismatch", | ||
| "component", "api", "node", plan.ProxyNode.URL, "error", err) | ||
| if releaser, ok := h.NodePlanner.(sessionReservationReleaserV3); ok { | ||
| releaser.ReleaseSession(sessionID) | ||
| } | ||
| return nil, h.refuseLocalIdentityWorkV3(r, result) |
There was a problem hiding this comment.
In a heterogeneous proxy pool, such as during a rolling FFmpeg upgrade, a capability mismatch on the single round-robin-selected proxy immediately falls back to the API rather than selecting another healthy proxy. If local fallback is disabled this returns capacity_unavailable even when a capable sibling proxy has free capacity; with fallback enabled it unexpectedly runs FFmpeg on the API node. Proxy selection should filter by the frozen recipe or retry the remaining eligible proxies.
AGENTS.md reference: AGENTS.md:L12-L14
Useful? React with 👍 / 👎.
| // idle reaper does not mistake "no local transport" for "abandoned" | ||
| // and kill a healthy stream. Always set it — a re-plan that moves a | ||
| // session back onto the API must clear a stale mark. | ||
| if err := h.sessionMgr.SetRemoteTransport(session.ID, servedByProxy); err != nil && |
There was a problem hiding this comment.
When a proxy-served identity session is replanned to an integrated HLS transport, this mark remains true because only the identity commit calls SetRemoteTransport; neither prepareLocalTransportV3 nor its session-state update clears it. After that local stream disconnects without an explicit stop, the stale mark widens admission and cleanup grace from 45 seconds to five minutes, unnecessarily retaining the transcode and consuming the user's stream/transcode slot. Every committed route transition should update this flag according to its actual serving node.
AGENTS.md reference: AGENTS.md:L12-L14
Useful? React with 👍 / 👎.
| func (h *PlaybackHandler) proxyCanExecutePlanV3(ctx context.Context, proxyURL string, result playback.PlannerResultV3) error { | ||
| if !planRequiresServerTransformationsV3(result.Plan) { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
When a progressive remux only changes the container and therefore has no listed server transformation, this shortcut accepts any healthy proxy without checking that its configured FFmpeg exists or can perform the base remux. A proxy with a missing/broken FFmpeg installation—exactly the kind of heterogeneous or partially upgraded node this endpoint is meant to guard—will then be selected and return a stream-time 500, with no local fallback or alternate-proxy attempt. Advertise and validate a base progressive-remux capability rather than treating an empty transformation list as requiring no executor.
AGENTS.md reference: AGENTS.md:L12-L14
Useful? React with 👍 / 👎.
| // Reserve against the session id the rest of the transport uses, so a | ||
| // re-plan replaces its own reservation instead of double-counting, and the | ||
| // rollback path can release it. | ||
| plan := h.NodePlanner.PlanSession(sessionID, "", false, identityStreamBitrateKbpsV3(result)) |
There was a problem hiding this comment.
For direct play and progressive remux, PlanSession reserves capacity when the plan is returned, but the proxy does not count an active job or emit egress until the client later opens the URL. A health report arriving before that GET causes effectiveJobs to stop counting the reservation, and the bandwidth bridge expires after 60 seconds; an autoplay-disabled or otherwise delayed client can therefore start after the proxy has admitted replacement work and exceed both MaxJobs and MaxBandwidthKbps. Lazy transports need admission at GET time or a reservation that remains valid until the proxy confirms the stream has begun.
AGENTS.md reference: AGENTS.md:L12-L14
Useful? React with 👍 / 👎.
| // ServeDirectPlay sets before ServeContent (ServeFile sets none, so | ||
| // If-Range never validates and a resumed range silently restarts at 200), | ||
| // and it carries the rolling write deadline and stream metrics with it. | ||
| _ = playback.ServeDirectPlay(w, r, claims.MediaPath) |
There was a problem hiding this comment.
When the selected proxy is on a different origin, ServeDirectPlay now emits the strong ETag required by direct_stream_resume_v1, but the proxy's CORS middleware configures no exposed response headers. Browser JavaScript therefore cannot read ETag, Accept-Ranges, or Content-Range and cannot construct the documented subsequent Range plus If-Range request, even though those request headers are allowed. Add the direct-stream response headers to ExposedHeaders for cross-origin proxy delivery.
AGENTS.md reference: AGENTS.md:L12-L14
Useful? React with 👍 / 👎.
…ality Narrow proxy selection by capability *before* selection rather than rejecting a single round-robin pick afterwards. Abandoning the pool on one mismatch meant a capable proxy with free capacity sat unused while playback either ran ffmpeg on the API node or, with playback.local_transcode_fallback disabled, was refused outright — the exact api/proxy split this branch targets, during exactly the rolling ffmpeg upgrade the capability check exists for. PlanSessionWith now applies its eligibility predicate to the proxy on proxy-only plans (the proxy is the executor there), mirroring how HLS filters transcode nodes, and the planner grows ProxyNodeURLs to match TranscodeNodeURLs. Direct play still skips the probe: it copies bytes and needs no recipe. Every committed route now records transport locality, not just the identity-proxy one. A session replanned from a proxy onto the integrated transcoder previously kept a stale remote-transport mark, and the widened idle grace it grants would hold that session's stream and transcode slots for five minutes after the local stream disconnected without an explicit stop. The remote HLS route sets it too — it also hands the client an absolute proxy URL that never reaches this server. The proxy's CORS config exposed no response headers, so cross-origin JavaScript could send the If-Range/Range request headers it already allows but never read the ETag, Accept-Ranges or Content-Range needed to build them. direct_stream_resume_v1 silently degraded to a full restart whenever the proxy was on a different origin than the web app, which is the normal deployment. Also regenerates internal/playback/testdata/protocol_v3 and the schema fixtures, which were stale for output_change_v1 since #613/#617 and failed CI on every branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Round two, in CI: stale playback fixtures. Capable sibling proxies (Cursor + Codex, same finding). Valid, and the more interesting of the two framings: on the api/proxy split this PR targets, with Fixed by narrowing before selection instead of rejecting after, which is what the HLS path already does for transcode nodes. Stale remote-transport mark (P2). Valid. Only the identity commit called CORS validators (P2). Valid and a genuinely good catch: the proxy allowed Two I have not changed, with reasoning: "Preserve reconstruction when returning proxy stream URLs." The premise that the API-local URL previously survived a restart does not hold for the calls named. "Keep lazy proxy reservations until the stream starts." Real, and also pre-existing: the identical window exists today for HLS, where The three P1s re-posted from commit New tests: capable-sibling selection end to end, mark cleared on a proxy→local transition, planner-level proxy filtering (including that an unsatisfiable plan leaves no reservation), and Verification: Still failing on |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d86e7e5. Configure here.
| } | ||
| _, ok := capable[node.URL] | ||
| return ok | ||
| }) |
There was a problem hiding this comment.
Remux skips transport-time capability recheck
Medium Severity
Identity remux now decides proxy eligibility only through pooledNodeTransformationsV3, which uses the 3s planning deadline and honors negatively cached failures. The previous post-selection check used remoteTransformationsV3 (10s, retries cached errors), and HLS still re-validates the chosen node that way after filtering. A proxy /hw-capabilities handler runs DetectHWAccelWithFFmpeg plus two ffmpeg probes before it writes a body, so a cold or slow probe exceeds 3s, the proxy is treated as incapable, and with playback.local_transcode_fallback off remux is refused even though the node could serve it.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit d86e7e5. Configure here.
…usals #617 renamed the terminal a burn-in-forced adaptation reports: when the subtitle burn requirement is the sole trigger, an HDR source that cannot be re-encoded now returns subtitle_conversion_unsupported instead of hdr_transcode_unsupported, so the refusal names the thing the viewer can actually act on. terminalAllowsAlternateFileV3 was not updated to match, and it gates the alternate-version retry on the old reason strings. That silently retired the fallback for exactly the case its own comment describes — a bitmap subtitle needing burn-in that an HDR source cannot support while an SDR alternate can. Playback was refused outright instead of switching to the version that can serve it. Adds the new reason to the gate and covers it directly, so a future rename of a refusal reason fails on the gate rather than only on the end-to-end replan test. Also drops debug instrumentation that was committed by mistake in TestHandleReplanPlaybackV3BitmapSubtitleFallsBackFromHDRToSDRVersion; the assertion is back to its original form and now passes on the merits. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Go is fixed in I was wrong that CI does not run the Go suite. It does — I committed debug instrumentation by mistake. While diagnosing the pre-existing The underlying failure turned out to be a real bug, not a stale test. #617 changed which terminal a burn-in-forced adaptation reports: when the subtitle burn requirement is the sole trigger, an HDR source that cannot be re-encoded now returns But
So selecting a PGS/VOBSUB track on an HDR source with an SDR alternate available refused playback outright instead of switching to the version that can serve it. The test was not asserting stale behavior — it was correctly reporting a regression that had been failing on Fixed by adding the new reason to the gate, plus a direct unit test on Full
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0dbca895eb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if result.Plan == nil || result.Plan.Delivery != playback.DeliveryRemuxProgressiveV3 || | ||
| !planRequiresServerTransformationsV3(result.Plan) || | ||
| nodepool.LocalTranscodeFallbackAllowed(r.Context(), h.SettingsRepo) { |
There was a problem hiding this comment.
When a server_remux_progressive plan only changes the container and therefore has no listed transformations, this condition returns without enforcing playback.local_transcode_fallback. If no proxy is eligible, the plan consequently falls back to the integrated /stream/{session} route, where ServeRemuxWithOptions launches FFmpeg on the API node despite the operator disabling local fallback. Gate every progressive remux that would execute locally, not only remuxes carrying an additional server transformation.
Useful? React with 👍 / 👎.
| if proxyNode == nil || file == nil { | ||
| return h.playbackStreamURL(s), false | ||
| } | ||
| card := identityRecipeCard(s) |
There was a problem hiding this comment.
When a proxy-served direct or progressive-remux replacement opens before the preceding request closes on the same proxy, this card reuses the playback session ID for both transports. The proxy tracker stores active sessions as a set keyed by that ID, so the predecessor's deferred Remove deletes the successor's entry; subsequent health reports advertise zero jobs while the replacement is still streaming and the planner can exceed MaxJobs. Use a per-transport identity or make proxy tracking reference-count concurrent requests for the same session.
AGENTS.md reference: AGENTS.md:L12-L14
Useful? React with 👍 / 👎.
1 participant
Problem
In a multi-node deployment, protocol v3 direct-play and progressive-remux sessions were served by
the API server itself instead of being delegated to a proxy node.
prepareTransportV3consultedthe node planner only for the two HLS deliveries;
original_httpandserver_remux_progressivereturned early with an API-local
/stream/{session_id}URL, andHandleStreamthen served thebytes (
ServeDirectPlay) or spawned a local ffmpeg (ServeRemuxWithOptions) in the API process.An operator running a dedicated proxy node still saw all direct-play and remux egress land on the
API node — which is what the reporting user hit on an api/proxy/transcode split with transcoding
disabled on the API node.
Why this approach
The capability already existed and was already proven on another surface:
/stream/direct/{token}and/stream/remux/{token}(
internal/proxy/server.go).PlanSession(..., needsTranscode=false, ...)andredirects to exactly those routes (
internal/jellycompat/streams.go).Native v3 was the only surface skipping it, so on the same deployment a Jellyfin client routed
correctly while Silo's own clients did not. This wires the existing shape into the v3 identity
transport rather than inventing a second node-selection path.
No client change is required:
stream.urlis already allowed to be absolute (the HLS deliveriesreturn absolute proxy URLs today), and the web player resolves absolute URLs already.
Token claims
The proxy reconstructs from the stream token alone, so the token now carries three things a
RecipeCarddoes not model:MediaPathDVProfileAudioOnlyaudio/mp4overvideo/mp4— the MIME the plan promised.None of these fail loudly when missing. They would produce a stream subtly different from the one
the plan promised, which is why they are set explicitly rather than left to
ToClaims().Related fixes in this PR
Proxy direct-play parity. The proxy served direct play with
http.ServeFile, which sets nostrong ETag.
direct_stream_resume_v1depends on the ETagServeDirectPlaysets beforeServeContent— without itIf-Rangenever validates and a resumed range silently restarts at200. Routing direct play to a proxy without this would have broken resumable direct streams on
exactly the deployments this PR targets. The proxy now uses the same serve path (which also brings
the rolling write deadline and stream metrics with it).
playback.local_transcode_fallbackgap. The gate was only consulted inside the HLS branch, soan audio-converting progressive remux still spawned ffmpeg locally on an API-only node with the
setting disabled. Identity deliveries now honor it too. Direct play deliberately still falls back
locally — moving bytes is not transcode work, and single-node deployments must keep working.
Risks / follow-up
to the current API-local path, so single-node deployments are unchanged.
repeated failed starts cannot pin a node's job/bandwidth budget until the reservation ages out.
Verification
Six new tests cover: proxy routing for direct play (incl. reservation session id and bitrate
estimate), proxy routing for progressive remux with seek + DV profile preserved, local fallback
with no eligible proxy, the remux refusal when local fallback is disabled, direct play still
allowed when it is disabled, and reservation release on rollback. All six were confirmed to fail
against pre-fix behavior and pass with the change.
One pre-existing failure is present and is not from this branch:
TestHandleReplanPlaybackV3BitmapSubtitleFallsBackFromHDRToSDRVersionfails identically on apristine checkout of
ee9356aab(#617) with none of this branch's code. Worth a separate look.Closes #619
AI-use disclosure
Implemented by Claude Opus 5 (Claude Code) on behalf of a maintainer. The report was triaged from
an internal Discord thread; the root cause, the token-claim gaps, and the two related defects were
identified by reading the code, and every claim above was verified by execution as shown.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Note
Cursor Bugbot is generating a summary for commit 811c85d. Configure here.