fix: bound the upstream subscribe handshake with a wall-clock timeout - #204
Open
thexeos wants to merge 1 commit into
Open
fix: bound the upstream subscribe handshake with a wall-clock timeout#204thexeos wants to merge 1 commit into
thexeos wants to merge 1 commit into
Conversation
`subscribe_open` waits for the upstream to answer a SUBSCRIBE, which a peer is under no obligation to ever do, and neither relay path bounded that wait. On the local pull-through path a never-answered subscribe pinned the task, the `TrackWriter` it carries, and every downstream subscriber blocked on `UpstreamReady::established()`, until the session died; the cross-relay path in `remote.rs` raced only connection cancellation. The `lease.released()` arm existed but only became live after the handshake returned, so the window it was meant to cover was exactly the window it did not cover. Bound the handshake two ways on both paths. `lease.released()` now races the handshake itself, covering "nobody downstream is waiting for this track any more" without inventing a constant. `--subscribe-timeout` (default 10s) covers the rest: a subscriber that is still waiting cannot wait forever on a peer that never answers. Either way the in-flight future is dropped, which drops the `Subscribe` and sends UNSUBSCRIBE, so giving up leaves nothing dangling upstream; the local path also fails the downstream request with REQUEST_ERROR (Timeout) instead of stranding it. Expiry is counted by `moq_relay_subscribe_timeouts_total`, labelled by source. Unlike `--cache-idle-timeout`, where zero meaningfully means "never evict", there is deliberately no zero-disables setting: an unbounded wait on a peer is the thing this timeout exists to prevent, so zero is rejected by clap and again by `Relay::new_with_tuning`. Config plumbing changes shape to carry a second knob: `new_with_cache_idle_timeout` / `build_with_cache_idle_timeout` become `new_with_tuning` / `build_with_tuning`, taking a `#[non_exhaustive]` `RelayTuning` with `Default` and builder methods, so further knobs need no new constructor. `RelayConfig` is untouched, so embedders building it as an exhaustive struct literal are unaffected. This forward-ports the same protection from the `draft-ietf-moq-transport-14` maintenance branch, adapted to this branch's `UpstreamReady` plumbing and its `Locals`-side eviction accounting.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
subscribe_openwaits for an upstream to answer a SUBSCRIBE, which a peer is under no obligation to ever do, and neither relay path bounded that wait — a never-answered subscribe pinned the task, itsTrackWriter, and every downstream subscriber waiting on it until the session died. This bounds the handshake two ways on both the local pull-through path and the cross-relay path: it raceslease.released()against the handshake itself (covering "nobody downstream is waiting any more"), and adds--subscribe-timeout, defaulting to 10s, for the case where a subscriber is still waiting. Either way the in-flight future is dropped, which drops theSubscribeand sends UNSUBSCRIBE, so giving up leaves nothing dangling upstream; expiry is counted bymoq_relay_subscribe_timeouts_total.This forward-ports the same protection from the
draft-ietf-moq-transport-14branch, adapted to this branch'sUpstreamReadyplumbing. Note that config plumbing changes shape to carry a second knob, withnew_with_cache_idle_timeoutbecomingnew_with_tuning(config, RelayTuning)— happy to keep a deprecated alias instead if you would rather not break embedders.