feat(net, relay): GOAWAY with graceful drain and cluster migration - #2490
feat(net, relay): GOAWAY with graceful drain and cluster migration#2490ksletmoe-aws wants to merge 10 commits into
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @ksletmoe-aws, your pull request is larger than the review limit of 150000 diff characters
… the URI The GOAWAY codecs existed but predated the draft-18/19 churn and had no bounds on the New Session URI: - draft-18 (moq-dev#1559) requires a trailing Request ID on the control stream; encode appends 0 (no per-request tracking) and decode tolerates a lenient peer omitting it. draft-19 (moq-dev#1623) removed the field again. - Enforce the 8,192-byte New Session URI cap on the IETF wire (all drafts) and add the same cap to moq-lite, rejected from the length prefix alone so a hostile length cannot force unbounded buffering. The moq-lite draft now specifies the cap (matching moq-transport). - js/net: mirror all of the above, with byte-layout tests locking the draft-18 Request ID parity against the Rust encoder. Test plan: cargo test -p moq-net --lib goaway (11 tests incl. the new cap and draft-18/19 layout tests); bun test on ietf.test.ts + lite/goaway.test.ts (95); kramdown-rfc parses the draft. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe change adds draft-aware GOAWAY encoding and decoding with URI-size validation, shared session APIs for initiating and observing drains, and protocol wiring across IETF and MoQ Lite. New tests cover wire formats, deadlines, duplicate signals, request gating, and transport behavior. Relay shutdown now drains active sessions, while cluster connections reconnect after upstream GOAWAY messages using validated redirects, retry limits, and timeout-based cleanup. Configuration and protocol documentation describe the new behavior. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (13)
rs/moq-net/src/ietf/goaway.rs (1)
47-59: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a test for the Draft-18 "absent Request ID" tolerance.
The decoder claims to "tolerate its absence from lenient peers" (Lines 51-54), but every existing test (
test_goaway_v18_timeout,test_goaway_v18_drains_optional_request_id) exercises a body that includes the trailing Request ID. The actual tolerant-of-absence path is untested.✅ Suggested regression test
#[test] fn test_goaway_v18_tolerates_missing_request_id() { // Hand-construct a draft-18 GOAWAY body WITHOUT the optional Request ID. let mut buf = BytesMut::new(); "moqt://relay.example/".encode(&mut buf, Version::Draft18).unwrap(); 5000u64.encode(&mut buf, Version::Draft18).unwrap(); let mut bytes = bytes::Bytes::from(buf.to_vec()); let decoded: GoAway = GoAway::decode_msg(&mut bytes, Version::Draft18).unwrap(); assert_eq!(decoded.new_session_uri, "moqt://relay.example/"); assert_eq!(decoded.timeout, 5000); }Also applies to: 170-189
🤖 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 `@rs/moq-net/src/ietf/goaway.rs` around lines 47 - 59, Add a regression test alongside the existing Draft-18 GOAWAY tests, using the test suite’s established encoding and decoding APIs. Construct a Draft-18 body containing only the session URI and timeout, omit the trailing Request ID, decode it through GoAway::decode_msg, and assert the URI and timeout are preserved successfully.rs/moq-net/src/session.rs (1)
185-217: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider an options struct for
Drain::start/start_with_timeout.Both methods take positional
uri/timeoutparameters. Per the guideline, growing this API (e.g. a third option later) would require yet another method name rather than adding a field.As per coding guidelines: "Use options/config structs instead of positional parameters when an API could gain additional options; this includes APIs with only one option today."
🤖 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 `@rs/moq-net/src/session.rs` around lines 185 - 217, The Drain API uses positional parameters that would require additional method names as options grow. Introduce an options/config struct for the URI and optional timeout, then update Drain::start, Drain::start_with_timeout, and start_inner to accept and use that struct while preserving the existing no-timeout and timeout behaviors.Source: Coding guidelines
rs/moq-net/tests/goaway.rs (2)
74-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the leftover
eprintln!checkpoints.Debug artifacts; no other test in the file has them.
♻️ Proposed cleanup
- eprintln!("CHECKPOINT: connected"); let draining = pair .server .drain() .expect("drain") .start_with_timeout("moqt://relay.example/", Duration::from_secs(5)); - eprintln!("CHECKPOINT: drain started"); let goaway = pair.client.goaway().await.expect("session closed before GOAWAY"); - eprintln!("CHECKPOINT: goaway observed"); assert_eq!(&*goaway.uri, "moqt://relay.example/");🤖 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 `@rs/moq-net/tests/goaway.rs` around lines 74 - 83, Remove the leftover eprintln! checkpoint statements around the drain and GOAWAY flow in the test, including the connected, drain started, and goaway observed messages, while leaving the test behavior unchanged.
214-227: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHand-rolled handshake duplicates
connect_mock.The only reason for the duplication is keeping a clone of the raw
MockSession. Returning the transports from the harness (e.g. an extra field onMockPair) removes the second copy of the handshake, which otherwise has to be kept in sync with the driver-spawn ordering note inrs/moq-net/tests/support/harness.rslines 77-91.♻️ Sketch
pub struct MockPair { pub client: Session, pub server: Session, + /// Raw transports, for tests that inject wire-level frames. + pub client_transport: MockSession, + pub server_transport: MockSession, }As per coding guidelines, "Refactor awkward internal shapes during the same change instead of preserving them or adding duplicated one-off helpers."
🤖 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 `@rs/moq-net/tests/goaway.rs` around lines 214 - 227, The goaway test duplicates the handshake flow solely to retain the raw MockSession. Extend the mock-session harness, including MockPair and connect_mock as appropriate, to expose the underlying transport while preserving the required driver-spawn ordering, then refactor this test to use the shared handshake helper and returned transport instead of repeating Client/Server connection setup.Source: Coding guidelines
js/net/src/lite/goaway.test.ts (1)
91-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a multi-byte case to the cap tests.
Both cap tests use ASCII, so they pass whether the limit counts UTF-8 bytes or UTF-16 code units. A URI under 8,192 characters but over 8,192 bytes is the case that actually pins the decoder's
TextEncodersemantics.♻️ Suggested additional test
test("Goaway: rejects URI over the 8192-byte cap", async () => { const msg = new Goaway("a".repeat(8193)); const encoded = await encode(msg, Version.DRAFT_04); await expect(decode(encoded, Version.DRAFT_04)).rejects.toThrow(/8,192/); }); + +test("Goaway: cap counts UTF-8 bytes, not characters", async () => { + // 3 bytes per character: 2,731 characters is 8,193 bytes. + const msg = new Goaway("リ".repeat(2731)); + const encoded = await encode(msg, Version.DRAFT_04); + await expect(decode(encoded, Version.DRAFT_04)).rejects.toThrow(/8,192/); +});🤖 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 `@js/net/src/lite/goaway.test.ts` around lines 91 - 104, Add a multi-byte UTF-8 URI case to the Goaway cap tests around the existing 8,192-byte boundary, using a URI whose character count is below 8,192 but whose encoded byte length exceeds it, and assert decoding rejects it. Keep the existing ASCII boundary and over-cap tests unchanged.rs/moq-net/tests/support/mock.rs (1)
177-184: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
stop()reports a clean close to the writer, so stop/reset regressions are invisible to mock tests.Real QUIC surfaces STOP_SENDING to the sender as a stream error, not
Ok(()). As written, neither an explicitstop(code)nor a droppedMockRecvStreamcan be distinguished by the peer, so the failure mode called out inrs/moq-net/src/ietf/session.rs(dropping the GOAWAY reader emits a STOP_SENDING a strict peer treats as a protocol violation) cannot be reproduced here. Consider havingstop(code)recordErr(MockError::stream_reset(code))and leavingDropas the benign path, and note the limitation in the module doc.🤖 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 `@rs/moq-net/tests/support/mock.rs` around lines 177 - 184, Update MockRecvStream::stop to record Err(MockError::stream_reset(code)) in closed.result instead of Ok(()), while preserving its notification and done-state behavior. Keep Drop as the benign close path so explicit STOP_SENDING and dropped streams remain distinguishable, and document this mock limitation in the module documentation.rs/moq-net/src/lite/goaway.rs (1)
95-124: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider covering the other two rejection paths.
rejects_oversized_uripins the cap well. Two neighboring branches added in this change are untested: a within-cap length with insufficientremaining()(DecodeError::Short) and a non-UTF-8 payload. Both are cheap to hand-roll withBytesMut.🤖 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 `@rs/moq-net/src/lite/goaway.rs` around lines 95 - 124, The rejects_oversized_uri test does not cover the neighboring URI decode rejection branches. Extend this test to add a within-cap length prefix with insufficient payload and assert DecodeError::Short, then add a within-cap payload containing invalid UTF-8 and assert DecodeError::InvalidValue, constructing both inputs with BytesMut while preserving the existing cap assertions.js/net/src/lite/goaway.ts (1)
31-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winName the GOAWAY URI cap and remove the matching-assertion comment.
8192is doubled in the check/error, and theReader.string()+Message.decode()path buffers the declared payload before this validation, so a named constant is better. The comment also implies this checks against a Rust matching assertion, but the Rust decoder rejects before reading the payload.♻️ Suggested constant extraction
+/** Maximum UTF-8 byte length of a GOAWAY New Session URI, matching the IETF wire. */ +const MAX_URI_BYTES = 8192; + export class Goaway {static async `#decode`(r: Reader): Promise<Goaway> { const uri = await r.string(); - // The URI is capped at 8,192 bytes, matching the IETF wire and the Rust - // decoder; a longer one is a protocol violation. - if (new TextEncoder().encode(uri).byteLength > 8192) { - throw new Error("GOAWAY URI exceeds 8,192 bytes"); + // A longer URI is a protocol violation. + if (new TextEncoder().encode(uri).byteLength > MAX_URI_BYTES) { + throw new Error(`GOAWAY URI exceeds ${MAX_URI_BYTES} bytes`); } return new Goaway(uri); }🤖 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 `@js/net/src/lite/goaway.ts` around lines 31 - 37, Extract the GOAWAY URI limit used in the `Reader.string()` validation and error message into a named constant, then reuse it in both places. Remove the comment claiming the limit matches a Rust decoder assertion, while preserving the existing byte-length validation and `Goaway` construction.Source: Coding guidelines
rs/moq-net/src/ietf/subscriber.rs (1)
114-121: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a
SubscriberConfighere, matching the lite subscriber.This constructor is now at six positional parameters, and the sibling
Subscriberinrs/moq-net/src/lite/subscriber.rsalready takes aSubscriberConfigstruct (which is wheregoing_awaylanded on that side). Converting this one keeps the two protocol paths symmetric and makes the next added field a non-event at both call sites inrs/moq-net/src/ietf/session.rs.As per coding guidelines: "Use options/config structs instead of positional parameters when an API could gain additional options" and "Refactor awkward internal shapes during the same change instead of preserving them".
🤖 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 `@rs/moq-net/src/ietf/subscriber.rs` around lines 114 - 121, Replace the positional parameters of the IETF subscriber constructor new with a SubscriberConfig struct, matching the lite subscriber’s configuration shape and including session, origin, control, version, tasks, and going_away. Update the related construction call sites in the IETF session flow to build and pass SubscriberConfig while preserving existing behavior.Source: Coding guidelines
rs/moq-relay/tests/goaway_cluster.rs (1)
209-212: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid the deprecated
PublicConfig::Simplein a new test.
#[allow(deprecated)]here creates a fresh dependency on a path the repo keeps only for compatibility. Use the currentPublicConfigvariant so the deprecated form can eventually be dropped without touching tests.As per coding guidelines: "keep deprecated paths functional but hidden from published surfaces."
🤖 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 `@rs/moq-relay/tests/goaway_cluster.rs` around lines 209 - 212, Update the test setup around AuthConfig to replace the deprecated PublicConfig::Simple construction with the current non-deprecated PublicConfig variant, and remove the #[allow(deprecated)] attribute. Preserve the existing public configuration behavior while ensuring the test no longer depends on the compatibility-only path.Source: Coding guidelines
rs/moq-relay/src/main.rs (1)
107-129: 🩺 Stability & Availability | 🔵 TrivialConsider signalling drain to load balancers and the accept loop.
During the drain window the QUIC/web listeners keep accepting: a client that connects at that moment is handed a GOAWAY immediately, and
/healthstill reports healthy so upstream LBs keep routing traffic to a node that is going away. Flipping/healthto unhealthy onshutdown.started()(and optionally stoppingserver.accept()) makes the drain observable to the infrastructure.🤖 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 `@rs/moq-relay/src/main.rs` around lines 107 - 129, Update drain_on_signal to mark the node unhealthy as soon as shutdown.started() triggers, so /health reports draining during the entire shutdown window; also stop or gate the listener accept loop at that point if the existing server API supports it, preventing new clients from being admitted during drain.rs/moq-relay/src/config.rs (1)
60-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the TOML-vs-CLI regression test for
drain_timeout.The repo convention for TOML-loadable configs is a test proving an absent CLI flag does not clobber the TOML value.
Option<u64>makes that true by construction, but the guarded assertion is what keeps it true after refactors.As per coding guidelines: "For TOML-loadable configs, every
#[arg]field must beOption<T>, never a bare scalar; add a regression test ensuring CLI defaults do not clobber TOML values."♻️ Suggested test
#[test] fn drain_timeout_survives_toml_merge() { let toml = "drain_timeout = 42\n"; let dir = std::env::temp_dir().join("moq-relay-config-test"); std::fs::create_dir_all(&dir).unwrap(); let path = dir.join("drain-timeout.toml"); std::fs::write(&path, toml).unwrap(); let args = vec![std::ffi::OsString::from("moq-relay"), std::ffi::OsString::from(&path)]; let config = Config::parse_and_merge(args).expect("config load"); assert_eq!(config.drain_timeout, Some(42)); }🤖 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 `@rs/moq-relay/src/config.rs` around lines 60 - 65, Add a regression test near the configuration parsing tests for Config::parse_and_merge that writes a TOML file containing drain_timeout = 42, invokes parsing without a CLI drain-timeout override, and asserts the merged config retains Some(42). Follow existing temporary-file and argument-construction conventions, and keep the Option<u64> field behavior unchanged.Source: Coding guidelines
rs/moq-relay/src/web.rs (1)
160-166: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
with_shutdownpanics if the builder is used out of order.
Arc::get_mutfails as soon as any clone exists (afterroutes()orserve()), so an embedder that attaches shutdown late gets a runtime panic instead of a compile error. HoldingshutdownonWeband materializing theArc<WebState>inroutes()(or taking it innew) removes the ordering constraint entirely.♻️ Sketch: keep the field on the builder
pub fn with_shutdown(mut self, shutdown: crate::Shutdown) -> Self { - let state = Arc::get_mut(&mut self.state).expect("with_shutdown called after routes were built"); - state.shutdown = shutdown; + // `Web` owns `shutdown` and passes it into `WebState` when `routes()` builds it, + // so attaching it is order-independent. + self.shutdown = shutdown; self }🤖 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 `@rs/moq-relay/src/web.rs` around lines 160 - 166, Refactor the Web builder so with_shutdown does not call Arc::get_mut or panic after routes() or serve() has cloned state. Store the shutdown value directly on Web, then apply it when materializing WebState in routes() (or during new), allowing shutdown configuration at any builder stage while preserving the existing session-draining behavior.
🤖 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.
Inline comments:
In `@rs/moq-net/src/ietf/adapter.rs`:
- Around line 564-570: Update the GOAWAY framing path around the body-length
encoding to convert body.len() to u16 with try_into() instead of a truncating
cast, and on conversion failure return the same Error::BoundsExceeded(...) path
used by the setup send flow. Preserve the existing encoding-error handling for
valid lengths.
In `@rs/moq-net/src/lite/subscriber.rs`:
- Line 1273: Remove the initial Subscribe stream creation and encoding in
establish, retaining only the open_subscribe(&msg) path so no unused SUBSCRIBE
stream or response remains. Move self.subscriber.check_going_away()? into
open_subscribe if establish or its other callers still require that check.
In `@rs/moq-net/src/session.rs`:
- Around line 194-203: Normalize Duration::ZERO to no local deadline in
start_with_timeout before passing the value into start_inner, so
Draining::complete does not immediately force-close the session while the wire
continues to encode zero as “no deadline.” Preserve non-zero timeout behavior
and the existing start_inner/Draining flow.
In `@rs/moq-net/tests/goaway.rs`:
- Around line 306-317: Replace the timing-based assertion around
bc.track("audio").unwrap().subscribe(None) with a deterministic observable check
that the gated subscription does not reach the upstream wire or server-side
track subscriber. If subscription returns an error, match specifically
Error::GoingAway rather than accepting any Err; if the resume model stalls,
assert the corresponding subscriber-side effect without sleeps or timeout-based
waiting.
In `@rs/moq-net/tests/support/mock.rs`:
- Around line 350-353: Update MockConnection::close to preserve the first close
state: only assign (code, reason) when self.side.conn.close_state is currently
None, while retaining notification behavior for waiters.
- Around line 286-307: Fix the shutdown wait in both accept_uni and accept_bi so
they check the connection’s closed state before awaiting close_notify, avoiding
a missed notification when close() ran earlier. Mirror the state-checking loop
used by the closed() implementation, and return close_error() once shutdown is
observed while preserving normal stream delivery.
In `@rs/moq-relay/src/cluster.rs`:
- Around line 1036-1075: Update resolve_redirect so fallback JWT propagation
occurs only when the parsed redirect host matches the configured fallback host.
Preserve redirect handling and scheme-security checks, but leave cross-host
redirects without the fallback JWT while continuing to carry it for same-host
redirects and existing redirect-owned JWTs.
- Around line 932-993: Bound GOAWAY-triggered migrations in the session loop
around session.goaway() and the replacement-session logic. Track consecutive
migrations (or enforce an equivalent minimum interval), reset the bound after
stable operation, and when the limit is exceeded stop migrating in place so
run_remote_once returns and the normal run_remote backoff path handles
reconnection. Ensure each migration cannot continue spawning detached drain
tasks indefinitely.
In `@rs/moq-relay/src/shutdown.rs`:
- Around line 69-74: Update drain_session to distinguish an already-claimed
drain from a session on a GOAWAY-less version instead of treating every None
result as an abort case. Await the existing drain completion or leave an
already-draining session alone, while retaining immediate session.abort with
GoingAway only for versions that do not support GOAWAY.
In `@rs/moq-relay/tests/goaway_cluster.rs`:
- Around line 35-42: The free-port probe is dropped before ServerConfig::init(),
allowing another test to claim the port and causing spurious failures. In
rs/moq-relay/tests/goaway_cluster.rs lines 35-42, add a shared bind_free_port()
helper that retries initialization after bind failures or loops over fresh probe
ports, and use it from spawn_upstream; in lines 200-206, replace the duplicated
probe/drop/bind logic in spawn_relay_with_upstream with the same helper.
- Around line 364-365: Update the FRAMES_PER_GROUP constant or the adjacent
group 0 comment so they agree, preserving the intended frame count for the fully
verified MID-A flow.
---
Nitpick comments:
In `@js/net/src/lite/goaway.test.ts`:
- Around line 91-104: Add a multi-byte UTF-8 URI case to the Goaway cap tests
around the existing 8,192-byte boundary, using a URI whose character count is
below 8,192 but whose encoded byte length exceeds it, and assert decoding
rejects it. Keep the existing ASCII boundary and over-cap tests unchanged.
In `@js/net/src/lite/goaway.ts`:
- Around line 31-37: Extract the GOAWAY URI limit used in the `Reader.string()`
validation and error message into a named constant, then reuse it in both
places. Remove the comment claiming the limit matches a Rust decoder assertion,
while preserving the existing byte-length validation and `Goaway` construction.
In `@rs/moq-net/src/ietf/goaway.rs`:
- Around line 47-59: Add a regression test alongside the existing Draft-18
GOAWAY tests, using the test suite’s established encoding and decoding APIs.
Construct a Draft-18 body containing only the session URI and timeout, omit the
trailing Request ID, decode it through GoAway::decode_msg, and assert the URI
and timeout are preserved successfully.
In `@rs/moq-net/src/ietf/subscriber.rs`:
- Around line 114-121: Replace the positional parameters of the IETF subscriber
constructor new with a SubscriberConfig struct, matching the lite subscriber’s
configuration shape and including session, origin, control, version, tasks, and
going_away. Update the related construction call sites in the IETF session flow
to build and pass SubscriberConfig while preserving existing behavior.
In `@rs/moq-net/src/lite/goaway.rs`:
- Around line 95-124: The rejects_oversized_uri test does not cover the
neighboring URI decode rejection branches. Extend this test to add a within-cap
length prefix with insufficient payload and assert DecodeError::Short, then add
a within-cap payload containing invalid UTF-8 and assert
DecodeError::InvalidValue, constructing both inputs with BytesMut while
preserving the existing cap assertions.
In `@rs/moq-net/src/session.rs`:
- Around line 185-217: The Drain API uses positional parameters that would
require additional method names as options grow. Introduce an options/config
struct for the URI and optional timeout, then update Drain::start,
Drain::start_with_timeout, and start_inner to accept and use that struct while
preserving the existing no-timeout and timeout behaviors.
In `@rs/moq-net/tests/goaway.rs`:
- Around line 74-83: Remove the leftover eprintln! checkpoint statements around
the drain and GOAWAY flow in the test, including the connected, drain started,
and goaway observed messages, while leaving the test behavior unchanged.
- Around line 214-227: The goaway test duplicates the handshake flow solely to
retain the raw MockSession. Extend the mock-session harness, including MockPair
and connect_mock as appropriate, to expose the underlying transport while
preserving the required driver-spawn ordering, then refactor this test to use
the shared handshake helper and returned transport instead of repeating
Client/Server connection setup.
In `@rs/moq-net/tests/support/mock.rs`:
- Around line 177-184: Update MockRecvStream::stop to record
Err(MockError::stream_reset(code)) in closed.result instead of Ok(()), while
preserving its notification and done-state behavior. Keep Drop as the benign
close path so explicit STOP_SENDING and dropped streams remain distinguishable,
and document this mock limitation in the module documentation.
In `@rs/moq-relay/src/config.rs`:
- Around line 60-65: Add a regression test near the configuration parsing tests
for Config::parse_and_merge that writes a TOML file containing drain_timeout =
42, invokes parsing without a CLI drain-timeout override, and asserts the merged
config retains Some(42). Follow existing temporary-file and
argument-construction conventions, and keep the Option<u64> field behavior
unchanged.
In `@rs/moq-relay/src/main.rs`:
- Around line 107-129: Update drain_on_signal to mark the node unhealthy as soon
as shutdown.started() triggers, so /health reports draining during the entire
shutdown window; also stop or gate the listener accept loop at that point if the
existing server API supports it, preventing new clients from being admitted
during drain.
In `@rs/moq-relay/src/web.rs`:
- Around line 160-166: Refactor the Web builder so with_shutdown does not call
Arc::get_mut or panic after routes() or serve() has cloned state. Store the
shutdown value directly on Web, then apply it when materializing WebState in
routes() (or during new), allowing shutdown configuration at any builder stage
while preserving the existing session-draining behavior.
In `@rs/moq-relay/tests/goaway_cluster.rs`:
- Around line 209-212: Update the test setup around AuthConfig to replace the
deprecated PublicConfig::Simple construction with the current non-deprecated
PublicConfig variant, and remove the #[allow(deprecated)] attribute. Preserve
the existing public configuration behavior while ensuring the test no longer
depends on the compatibility-only path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 874256c9-a773-45aa-9e30-2f42e2b36ae3
📒 Files selected for processing (38)
doc/bin/relay/config.mddoc/concept/layer/moq-lite.mddrafts/draft-lcurley-moq-lite.mdjs/net/src/ietf/goaway.tsjs/net/src/ietf/ietf.test.tsjs/net/src/lite/goaway.test.tsjs/net/src/lite/goaway.tsrs/moq-native/tests/broadcast.rsrs/moq-net/src/client.rsrs/moq-net/src/error.rsrs/moq-net/src/goaway.rsrs/moq-net/src/ietf/adapter.rsrs/moq-net/src/ietf/goaway.rsrs/moq-net/src/ietf/session.rsrs/moq-net/src/ietf/subscriber.rsrs/moq-net/src/lib.rsrs/moq-net/src/lite/goaway.rsrs/moq-net/src/lite/publisher.rsrs/moq-net/src/lite/session.rsrs/moq-net/src/lite/subscriber.rsrs/moq-net/src/lite/version.rsrs/moq-net/src/server.rsrs/moq-net/src/session.rsrs/moq-net/src/version.rsrs/moq-net/tests/goaway.rsrs/moq-net/tests/support/harness.rsrs/moq-net/tests/support/mock.rsrs/moq-net/tests/support/mod.rsrs/moq-relay/src/cluster.rsrs/moq-relay/src/config.rsrs/moq-relay/src/connection.rsrs/moq-relay/src/lib.rsrs/moq-relay/src/main.rsrs/moq-relay/src/shutdown.rsrs/moq-relay/src/web.rsrs/moq-relay/src/websocket.rsrs/moq-relay/tests/goaway_cluster.rsrs/moq-relay/tests/smoke.rs
Adds the GOAWAY lifecycle on both wires, replacing the receive-side stubs
(lite logged and ignored; IETF failed with Unsupported):
- Send: Session::drain() claims the one-GOAWAY-per-session slot and
returns a Drain; Drain::start(uri) / start_with_timeout(uri, t) fire
the frame and return a Draining whose complete() force-closes with the
new GoawayTimeout code (33) when the deadline expires.
- Receive: Session::goaway() resolves with GoawayReceived { uri, timeout }
and Session::is_going_away() is a cheap flag. Duplicate GOAWAYs keep
the first payload (an observer may already be acting on its URI) and
are logged.
- Request gating: after a received GOAWAY, new SUBSCRIBE / FETCH / TRACK /
announce-interest opens are rejected with GoingAway (32); PROBE is
silently skipped; existing subscriptions keep flowing.
- Wire channels per version: lite-04+ uses the dedicated Goaway control
stream; IETF draft-14-16 ride the shared control stream through the
adapter; draft-17+ use the SETUP uni streams (ours to send, the peer's
to receive). The receive reader stays alive after decoding, since
closing the peer's SETUP stream mid-session is a protocol violation a
strict peer punishes right in the middle of the drain.
- The lite sender waits for its FIN to be acknowledged before dropping
the Goaway stream: Writer's Drop resets, and on real QUIC a reset
racing the FIN discards the unacked GOAWAY frame (caught by the
real-transport tests; the in-memory mock cannot lose data). Same dance
as send_setup.
- Session::closed() now surfaces the application close code and reason
when the transport carries them (quinn's Display drops both), so a
peer can distinguish a GoawayTimeout force-close from a network error.
All plumbing follows the driver architecture: the send paths are TaskSet
children racing the drain trigger against transport close (nothing
spawned, nothing blocking clean shutdown), and Draining::complete polls
under a kio waiter rather than tokio::select.
Test plan: cargo test -p moq-net (544 lib + 8 goaway integration),
-p moq-relay, -p moq-native (incl. 6 real-QUIC/WebTransport GOAWAY
tests) all green; clippy clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two consumers of the new moq-net GOAWAY support: Upstream (cluster): when a peer sends GOAWAY, the relay dials the replacement (the redirect URI when safe, else the original URL) while the old session keeps serving, then drains the old session in the background (the GOAWAY's own deadline, else --cluster-drain-timeout, default 10s) and force-closes it with GoawayTimeout. The replacement shares the same origin, so its announcements attach as routes to the broadcasts the old session served and the origin hands live tracks over at a group boundary when the old routes detach. Downstream sessions never see a GOAWAY. A redirect is followed only when its scheme is at least as secure as the current connection's (no plaintext downgrade; unknown schemes rank lowest, fail-safe), and the cluster JWT carries onto the redirect unless it brings its own. Constraining the redirect host to known peers is a deliberate follow-up, marked in the code. Downstream (--drain-timeout): the first shutdown signal broadcasts a drain: every accepted session (QUIC/WebTransport and the WebSocket fallback) sends an empty-URI GOAWAY (reconnect to me) and is force-closed after the window; a second signal, or the window elapsing, exits. Clients on pre-GOAWAY versions (moq-lite-03 and earlier) are closed immediately; there is no wire message to warn them with. Test plan: cargo test -p moq-relay (148 lib incl. new scheme-tier and redirect-resolution unit tests + 12 smoke); clippy clean. Cluster migration integration tests land in the next commit on this branch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…test moq-net (in-memory mock transport, deterministic, no sleeps): - send/receive across the full GOAWAY version matrix (lite-04/05, moq-transport-14/16/17/18/19), covering all three wire channels - wire timeout on draft-17+, and the GOAWAY_TIMEOUT force-close observed by an overstaying peer - drain claim exclusivity + release-on-drop; drain() is None pre-lite-04 - regression: a duplicate GOAWAY (injected as raw wire frames; the public API can't send two) keeps the first payload - request gating: a new subscribe after GOAWAY never reaches the wire (delivered as a resume-model stall), while the existing subscription keeps flowing The mock transport and harness are revived from the earlier GOAWAY branch; the harness now spawns each side's Driver as soon as its handshake resolves, since draft-17+ handshakes each block on the peer driver's SETUP. moq-relay (real TCP transports): cluster_migrates_on_upstream_goaway stands up two sibling upstreams sharing one origin, drains sibling A with a redirect to B, and asserts the cluster reconnects, delivery resumes contiguously at the next group, no announce churn leaks to the cluster origin, and the old session drains away. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…knobs - doc/concept/layer/moq-lite.md: GOAWAY send/receive lifecycle on the current API, the route-based seamless resume story, version support, and the js/net wire-only status. - doc/bin/relay/config.md: the top-level drain_timeout (shutdown drain) and cluster.drain_timeout (upstream GOAWAY drain window) keys. Changelogs are release-plz generated from the commit subjects, so no manual CHANGELOG edits ride along. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
It's a good first attempt and most of the stuff is salvageable.
I would want to see client support via Reconnect in moq-native and moq/net. It should be transparent to the client and there should be (at least) two concurrent connections. Starting with the cluster connections is okay but less testable, and really cluster connections should be leveraging moq-native instead.
We also need some way of marking the old connection/route as lower priority so we migrate the subscription. I know it's a follow up, but it seems required for any sort of draining. We need to somehow cost a draining connection as infinite so its never used as a primary.
| # keeps the old upstream alive this long so in-flight groups finish, then | ||
| # force-closes it. A deadline on the received GOAWAY takes precedence. | ||
| # Default: 10. | ||
| drain_timeout = 10 |
| The lifecycle on the sending side (Rust `moq-net`): | ||
|
|
||
| 1. `session.drain()` claims the session for draining (one GOAWAY per session). Returns a `Drain` handle, or `None` on versions without GOAWAY (moq-lite-03 and earlier). | ||
| 2. `drain.start(uri)` or `drain.start_with_timeout(uri, duration)` sends the GOAWAY frame. |
There was a problem hiding this comment.
not a fan of xxx_with_yyy methods. If we ever wanted to make another option, then we're out of luck.
Make a separate options struct with a builder instead.
| &self, | ||
| reader: Reader<S::RecvStream, Version>, | ||
| writer: Writer<S::SendStream, Version>, | ||
| goaway: crate::goaway::Protocol, |
There was a problem hiding this comment.
Should this be in the adapter? I guess, but it should be session.rs or something.
| let mut closed = std::pin::pin!(session.closed()); | ||
| let mut triggered = std::pin::pin!(goaway.triggered()); |
There was a problem hiding this comment.
ideally we can poll_closed and poll_triggered instead.
| /// | ||
| /// Returns `None` if the trigger was dropped without firing (the session is | ||
| /// closing without a drain), so no GOAWAY should be sent. | ||
| pub async fn triggered(&self) -> Option<Payload> { |
| /// With a deadline (from [`Drain::start_with_timeout`]), the session is | ||
| /// force-closed with [`Error::GoawayTimeout`] when it expires; the timer is | ||
| /// cancelled if the peer closes first. | ||
| pub async fn complete(self) { |
There was a problem hiding this comment.
Pretty weird API. Maybe impl Future instead, or return a Result.
| /// force-closes it. A deadline carried on the received GOAWAY takes | ||
| /// precedence over this value. Defaults to 10 seconds. |
There was a problem hiding this comment.
The deadline on the GOAWAY should not take precedence; we should take the min.
|
|
||
| /// Fallback drain time in seconds for an upstream peer that sends a GOAWAY | ||
| /// without its own deadline. After a successful reconnect the relay keeps | ||
| /// the old upstream alive this long so its in-flight groups finish, then |
There was a problem hiding this comment.
I'm kinda unsure why this exists? The idea is that the client should establish the new connection in parallel. I'm not sure who closes the connection when it's fully drained, but I wouldn't keep a dead connection "alive" for this arbitrary amount of time.
|
|
||
| Err(cs.closed().await.into()) | ||
| // Fallback drain window for an upstream GOAWAY that carries no timeout. | ||
| let drain_timeout = |
There was a problem hiding this comment.
IDK, shouldn't this whole thing be in Reconnect instead? The code looks pretty terrible.
| ); | ||
| return fallback.clone(); | ||
| } | ||
| // Carry the cluster JWT over to the redirect target unless it brings its own. |
There was a problem hiding this comment.
Yeah don't do this? The server SHOULD echo back the JWT in the GOAWAY URL. It seems pretty useful for forcing the client to switch auth tokens or something.
Address review feedback on the drain API and add transparent client-side migration when a GOAWAY redirect arrives. API reshape (moq-net, moq-relay): - Replace Drain::start/start_with_timeout with a single start(DrainConfig) options struct plus builder. - Convert the goaway triggered() to poll_triggered and drive the lite and IETF session loops off the poll methods. - Draining::complete now returns Result so callers can distinguish a graceful close from a timeout-forced abort. - Move the GOAWAY protocol plumbing out of the IETF adapter into the session driver. - Rename the relay drain_timeout config to a humantime drain (and cluster.drain), keeping the old keys as hidden deprecated aliases. - Take min(local, goaway) for the drain deadline instead of letting the GOAWAY deadline win outright. - Drop the JWT carry-over on redirect; the sending server echoes the JWT in the GOAWAY URI instead. Reconnect migration (moq-native): - Add a Drain config and a Status::Migrating variant. - On GOAWAY, dial the redirect in parallel, drain the old session for min(drain, goaway), and fall back to backoff if the redirect fails. Route deprioritization (moq-net): - On GOAWAY receipt the subscriber bumps its own routes to broadcast::DRAIN_COST, so best_route migrates live subscribers off the draining connection (lite and IETF).
|
@kixelated — pushed an update working through the review. What's in this revision: API-shape feedback (all addressed):
The part I'd most like your eyes on — the reconnect API: Held back deliberately: I haven't touched Target branch: this includes breaking (Written by claude-opus-4.8) |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rs/moq-relay/src/cluster.rs (1)
1009-1045: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftStart the drain deadline when GOAWAY is received.
Replacement dialing, including potentially unbounded
client.connect, happens before the old session timer starts. A stalled reconnect can therefore retain the old upstream past the configured or peer-advertised deadline. Capture and enforce the deadline before dialing, then use only the remaining budget after swapping sessions. Add a regression test with a delayed replacement dial.
rs/moq-relay/src/cluster.rs#L1009-L1045: race reconnect attempts against the effective deadline and force-close the old session when it expires.doc/bin/relay/config.md#L186-L192: state that the effective drain budget begins on GOAWAY receipt, not after reconnect succeeds.🤖 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 `@rs/moq-relay/src/cluster.rs` around lines 1009 - 1045, In rs/moq-relay/src/cluster.rs:1009-1045, start the effective drain deadline when GOAWAY is received, race each replacement client.connect attempt against that deadline, and force-close the old session if reconnecting or draining exceeds the remaining budget; after swapping sessions, pass only the remaining time to the drain task, and add a regression test covering a delayed replacement dial. In doc/bin/relay/config.md:186-192, document that the effective drain budget starts at GOAWAY receipt rather than after reconnect succeeds.
🧹 Nitpick comments (4)
rs/moq-native/src/reconnect.rs (3)
733-797: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftMigration control flow is untested.
The new tests cover
Drain::defaultandresolve_redirectonly. The riskiest additions,Status::Migratingtransitions,effective_timeout = min(drain.timeout, goaway.timeout), and the nested-GOAWAY path, have no coverage in this crate. A#[tokio::test]driving a real server that drains its session (as in rs/moq-native/tests/broadcast.rs) throughClient::reconnectwould pin the observable status sequence.🤖 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 `@rs/moq-native/src/reconnect.rs` around lines 733 - 797, Add a tokio integration test covering migration control flow by driving a real draining server through Client::reconnect, following the setup pattern in broadcast tests. Assert the observable Status::Migrating transitions, verify the effective timeout uses the minimum of drain.timeout and goaway.timeout, and exercise the nested-GOAWAY path.
349-380: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
Closedhandling; consider hoisting the migration into the outer loop.Lines 350-379 are a near-verbatim copy of Lines 256-276, and the nested monitoring adds a second control-flow level that only handles one extra GOAWAY before falling back to the outer loop. Extracting a helper (e.g.
fn handle_closed(...)) or restructuring so a successful migration just updatessession/current_urlandcontinues into the existing single monitoring path would remove the duplication and the divergence risk (the copies already differ: "session severed immediately, retrying" vs "session severed immediately").As per coding guidelines, "Refactor awkward internal shapes during the same change instead of preserving them or adding duplicated one-off helpers."
🤖 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 `@rs/moq-native/src/reconnect.rs` around lines 349 - 380, The Closed handling logic is duplicated across the reconnect flow and has already diverged. Refactor the outer reconnect loop around the existing Closed-handling branch so a successful migration updates session/current_url and continues through the single monitoring path; otherwise centralize shared cleanup, retry, and error logging in one reusable path, preserving the existing outcomes without adding another one-off duplicate helper.Source: Coding guidelines
596-627: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winShare the GOAWAY redirect policy instead of copying it.
rs/moq-native/src/reconnect.rsandrs/moq-relay/src/cluster.rsboth defineresolve_redirect/scheme_security_tierwith the same security-relevant behavior, including the mirroredresolve_redirect_policycoverage and JWT non-carryover intent. Host one implementation in a shared location likemoq-netand call it from both native and relay paths to avoid policy drift.🤖 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 `@rs/moq-native/src/reconnect.rs` around lines 596 - 627, Extract the shared GOAWAY redirect policy from reconnect.rs and cluster.rs into a reusable moq-net implementation, including resolve_redirect, scheme_security_tier, resolve_redirect_policy behavior, and JWT non-carryover semantics. Update both native and relay call sites to use the shared symbols, remove their duplicate policy definitions, and preserve existing fallback and security-downgrade behavior.rs/moq-net/src/ietf/subscriber.rs (1)
121-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDead
_goaway_receivedparameter and a draining check that only fires on message arrival, not proactively.
Subscriber::new's_goaway_receivedis discarded (never stored), andrun_subscribe_namespace'sdrainingflag is only checked after adecode_maybe()resolves, so on an idle SUBSCRIBE_NAMESPACE stream this local check never fires. Correctness for the draft14-16 path is preserved by a separate watcher task inietf/session.rs'sstart()that racesgoaway.received.consume()directly and callsdrain_all_broadcasts()once, but this makes the parameter and check here misleading dead weight.lite::Subscriberavoids this by racinggoaway_receivedagainst the announce decode withkio::wait(seers/moq-net/src/lite/subscriber.rsrun_announce_prefix), reacting even when the stream is idle.Consider either wiring
_goaway_receivedthe same way here for consistency, or dropping the unused parameter and the redundantgoing_away.is_set()gate in this loop.Also applies to: 226-243
🤖 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 `@rs/moq-net/src/ietf/subscriber.rs` at line 121, Remove the unused _goaway_received parameter from Subscriber::new and update all callers accordingly. Remove the redundant going_away.is_set() check from run_subscribe_namespace, while preserving the existing session-level watcher in start() that drains broadcasts on GOAWAY.
🤖 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.
Inline comments:
In `@rs/moq-native/src/client.rs`:
- Around line 63-67: Remove or rename the `drain` option’s `drain-timeout` alias
in the relay command configuration so it no longer conflicts with the
`ClientConfig` option flattened into `moq relay`. Preserve the existing drain
behavior while ensuring `moq relay --help` and configuration parsing complete
without duplicate Clap arguments.
In `@rs/moq-native/src/reconnect.rs`:
- Around line 381-390: Update the nested GOAWAY arm in the reconnect loop to
apply the same sliding-window churn limit used by Cluster::run_remote_once,
preserving retry timing so repeated handshake redirects cannot spin indefinitely
or bypass backoff.timeout. Before replacing the active connection, drain the
outgoing new_session with the existing success-path deadline and abort behavior,
ensuring state.session’s clone does not keep the connection alive.
In `@rs/moq-relay/src/main.rs`:
- Around line 21-40: Update configuration merging across
rs/moq-relay/src/main.rs lines 21-40, rs/moq-relay/src/config.rs lines 66-74,
and rs/moq-relay/src/cluster.rs lines 356-364 so legacy drain_timeout and
cluster.drain_timeout values accept bare numbers as seconds through CLI, TOML,
and environment inputs. Parse these legacy values during normal precedence-aware
merging, reject and report malformed values instead of silently ignoring them,
remove the late post-merge environment fallback, and add coverage for both
timeout settings across all legacy input sources.
---
Outside diff comments:
In `@rs/moq-relay/src/cluster.rs`:
- Around line 1009-1045: In rs/moq-relay/src/cluster.rs:1009-1045, start the
effective drain deadline when GOAWAY is received, race each replacement
client.connect attempt against that deadline, and force-close the old session if
reconnecting or draining exceeds the remaining budget; after swapping sessions,
pass only the remaining time to the drain task, and add a regression test
covering a delayed replacement dial. In doc/bin/relay/config.md:186-192,
document that the effective drain budget starts at GOAWAY receipt rather than
after reconnect succeeds.
---
Nitpick comments:
In `@rs/moq-native/src/reconnect.rs`:
- Around line 733-797: Add a tokio integration test covering migration control
flow by driving a real draining server through Client::reconnect, following the
setup pattern in broadcast tests. Assert the observable Status::Migrating
transitions, verify the effective timeout uses the minimum of drain.timeout and
goaway.timeout, and exercise the nested-GOAWAY path.
- Around line 349-380: The Closed handling logic is duplicated across the
reconnect flow and has already diverged. Refactor the outer reconnect loop
around the existing Closed-handling branch so a successful migration updates
session/current_url and continues through the single monitoring path; otherwise
centralize shared cleanup, retry, and error logging in one reusable path,
preserving the existing outcomes without adding another one-off duplicate
helper.
- Around line 596-627: Extract the shared GOAWAY redirect policy from
reconnect.rs and cluster.rs into a reusable moq-net implementation, including
resolve_redirect, scheme_security_tier, resolve_redirect_policy behavior, and
JWT non-carryover semantics. Update both native and relay call sites to use the
shared symbols, remove their duplicate policy definitions, and preserve existing
fallback and security-downgrade behavior.
In `@rs/moq-net/src/ietf/subscriber.rs`:
- Line 121: Remove the unused _goaway_received parameter from Subscriber::new
and update all callers accordingly. Remove the redundant going_away.is_set()
check from run_subscribe_namespace, while preserving the existing session-level
watcher in start() that drains broadcasts on GOAWAY.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f38ad31e-a62b-4107-b8c6-864ef93cb9b0
📒 Files selected for processing (23)
doc/bin/relay/config.mddoc/concept/layer/moq-lite.mdrs/moq-native/src/client.rsrs/moq-native/src/lib.rsrs/moq-native/src/reconnect.rsrs/moq-native/tests/broadcast.rsrs/moq-net/src/error.rsrs/moq-net/src/goaway.rsrs/moq-net/src/ietf/adapter.rsrs/moq-net/src/ietf/session.rsrs/moq-net/src/ietf/subscriber.rsrs/moq-net/src/lite/session.rsrs/moq-net/src/lite/subscriber.rsrs/moq-net/src/model/broadcast.rsrs/moq-net/src/model/origin.rsrs/moq-net/src/session.rsrs/moq-net/tests/goaway.rsrs/moq-relay/src/cluster.rsrs/moq-relay/src/config.rsrs/moq-relay/src/lib.rsrs/moq-relay/src/main.rsrs/moq-relay/src/shutdown.rsrs/moq-relay/tests/goaway_cluster.rs
🚧 Files skipped from review as they are similar to previous changes (10)
- rs/moq-relay/src/lib.rs
- rs/moq-net/src/error.rs
- rs/moq-relay/src/shutdown.rs
- doc/concept/layer/moq-lite.md
- rs/moq-relay/tests/goaway_cluster.rs
- rs/moq-net/src/goaway.rs
- rs/moq-net/src/lite/session.rs
- rs/moq-net/tests/goaway.rs
- rs/moq-net/src/ietf/session.rs
- rs/moq-native/tests/broadcast.rs
| /// GOAWAY-driven migration settings for [`Client::reconnect`]. | ||
| #[command(flatten)] | ||
| #[serde(default)] | ||
| pub drain: crate::Drain, | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for every drain-related clap arg id/long/alias across the workspace.
rg -nP --type=rust -C4 '(id|long|alias)\s*=\s*"drain' rs
# Confirm which structs flatten ClientConfig.
rg -nP --type=rust -B3 -A1 'moq_native::ClientConfig' rsRepository: moq-dev/moq
Length of output: 19748
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files 'rs/moq-native/src/reconnect.rs' 'rs/moq-native/src/client.rs' 'rs/moq-relay/src/config.rs' 'rs/moq-relay/Cargo.toml' 'Cargo.lock'
echo
echo "== relevant definitions =="
sed -n '1,120p' rs/moq-native/src/reconnect.rs
echo
sed -n '1,95p' rs/moq-relay/src/config.rs
echo
sed -n '50,75p' rs/moq-native/src/client.rs
echo
echo "== dependency pins =="
rg -n 'name = "clap"\\n|clap' Cargo.lock | head -80 || true
rg -n 'clap' rs/moq-native/Cargo.toml rs/moq-relay/Cargo.toml rs/moq-cli/Cargo.toml rs/moq-bench/Cargo.toml Cargo.toml || trueRepository: moq-dev/moq
Length of output: 8166
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== exact clap versions =="
sed -n '918,924p;1094,1117p;4409,4445p;4624,4639p;8765,8771p' Cargo.lock
echo
echo "== read-only structural merge of drain-related argument data =="
python3 - <<'PY'
from pathlib import Path
files = [
"rs/moq-relay/src/config.rs",
"rs/moq-native/src/client.rs",
"rs/moq-native/src/reconnect.rs",
]
for path in files:
text = Path(path).read_text()
print(f"--- {path} ---")
done = set()
for i, line in enumerate(text.splitlines(), 1):
if any(s in line for s in ('id = "drain', 'long = "drain', "alias = \"drain")):
print(f"{i}: {line.strip()}")
# crude structural block extraction for argument attrs only if desired.
PY
echo
echo "== behavior probe from clap source availability and package metadata only =="
python3 - <<'PY'
from pathlib import Path
cargo = Path("Cargo.lock").read_text()
# If lock file says clap is vendored locally in the repo, read local docs/source snippets instead.
for block in cargo.split("\n[[package]]\n")[:1]:
pass
PYRepository: moq-dev/moq
Length of output: 2148
Remove --drain-timeout from moq-relay's duplicate drain args.
moq relay flattens ClientConfig, which already exposes --drain-timeout, but relay still declares --drain with alias = "drain-timeout". Clap cannot define the same long argument in one command, so moq relay --help (or parse-time config loading) panics instead of loading relay config. Drop the relay alias or rename the relay option to avoid exposing the same long name.
🤖 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 `@rs/moq-native/src/client.rs` around lines 63 - 67, Remove or rename the
`drain` option’s `drain-timeout` alias in the relay command configuration so it
no longer conflicts with the `ClientConfig` option flattened into `moq relay`.
Preserve the existing drain behavior while ensuring `moq relay --help` and
configuration parsing complete without duplicate Clap arguments.
| if std::env::var_os("MOQ_DRAIN_TIMEOUT").is_some() { | ||
| tracing::warn!("MOQ_DRAIN_TIMEOUT is deprecated; use MOQ_DRAIN (a humantime duration, e.g. \"10s\") instead"); | ||
| // Fallback: if the new env isn't set, parse the old one. | ||
| if config.drain.is_none() { | ||
| if let Ok(val) = std::env::var("MOQ_DRAIN_TIMEOUT") { | ||
| if let Ok(secs) = val.parse::<u64>() { | ||
| config.drain = Some(std::time::Duration::from_secs(secs)); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| if std::env::var_os("MOQ_CLUSTER_DRAIN_TIMEOUT").is_some() { | ||
| tracing::warn!("MOQ_CLUSTER_DRAIN_TIMEOUT is deprecated; use MOQ_CLUSTER_DRAIN (a humantime duration, e.g. \"10s\") instead"); | ||
| if config.cluster.drain.is_none() { | ||
| if let Ok(val) = std::env::var("MOQ_CLUSTER_DRAIN_TIMEOUT") { | ||
| if let Ok(secs) = val.parse::<u64>() { | ||
| config.cluster.drain = Some(std::time::Duration::from_secs(secs)); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🌐 Web query:
For humantime 2.3.0 and humantime-serde 1.1.0, confirm whether bare numeric values such as 10are accepted for clap duration parsing and TOMLDuration deserialization.
💡 Result:
In humantime 2.3.0 and humantime-serde 1.1.0, bare numeric values such as 10 are not accepted for duration parsing or deserialization, with the exception of the value 0 [1][2][3]. For humantime 2.3.0, the parse_duration function strictly expects a number followed by a unit (e.g., 10s or 10sec) [1][4]. While version 2.3.0 introduced support for parsing "0" as a valid zero duration, all other bare integers will fail with a parsing error because a time unit is required [2][3][4]. For humantime-serde 1.1.0, the deserialization logic is implemented by calling humantime::parse_duration on the provided string [5]. Because it specifically uses visit_str to process inputs as strings and subsequently invokes the underlying humantime parser, it inherits the same requirement for units [5]. Consequently, bare numeric values in TOML or other formats will not be parsed as durations by humantime-serde [5].
Citations:
- 1: https://docs.rs/humantime/latest/humantime/fn.parse_duration.html
- 2: https://github.com/chronotope/humantime/releases
- 3: chronotope/humantime@v2.2.0...v2.3.0
- 4: https://github.com/tailhook/humantime/blob/49f11fdc2a59746085d2457cb46bce204dec746a/src/duration.rs
- 5: https://doc.cuprate.org/src/humantime_serde/lib.rs.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Cargo humantime versions =="
rg -n 'humantime|humantime_serde' Cargo.toml Cargo.lock rs/moq-relay -g 'Cargo.toml' -g 'Cargo.lock' || true
echo
echo "== Candidate files =="
fd -a 'main\.rs|config\.rs|cluster\.rs' rs/moq-relay/src || true
echo
echo "== Relevant snippets =="
sed -n '1,120p' rs/moq-relay/src/main.rs
echo
sed -n '1,140p' rs/moq-relay/src/config.rs
echo
sed -n '330,385p' rs/moq-relay/src/cluster.rs
echo
echo "== Dep source/type checks =="
python3 - <<'PY'
from pathlib import Path
import re
cargo_lock = Path('Cargo.lock').read_text()
for m in re.finditer(r'(?ms)^\[\[package\]\]\nname = "humantime-?serde"[\S\s]*?version = "([^"]+)"', cargo_lock):
print(m.group(0))
for name in ['humantime-serde', 'humantime_serde', 'humantime']:
if name in cargo_lock:
print(f'--- {name} ---')
for m in re.finditer(r'(?ms)^\[\[package\]\]\nname = "'+re.escape(name)+'"[\S\s]*?version = "([^"]+)"', cargo_lock):
print(m.group(0))
PY
echo
echo "== Inspect humantime-serde dependency source via crates.io source archive (static inspection, not build/run repo code) =="
python3 - <<'PY'
from pathlib import Path
import tarfile
import tempfile
import urllib.request
pkg = 'humantime-serde'
ver = '1.1.0'
url = f'https://crates.io/api/v1/crates/{pkg}/{ver}/download'
tmp = tempfile.mkdtemp()
path = Path(tmp) / f'{pkg}-{ver}.crate'
print(f'fetching {url} to {path}')
urllib.request.urlretrieve(url, path)
with tarfile.open(path, 'r:gz') as tf:
names = [n for n in tf.getnames() if any(n.endswith(x) for x in ['.rs', 'Cargo.toml'])]
for n in names:
if 'src' in n or n == 'Cargo.toml':
print(f'--- {n} ---')
data = tf.extractfile(n).read().decode()
for i,line in enumerate(data.splitlines(),1):
if 'visit_str' in line or 'visit_u64' in line or r'parse_duration' in line or 'Duration' in line or 'humantime' in line.lower():
print(f'{i}: {line}')
PYRepository: moq-dev/moq
Length of output: 15268
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Cargo config definitions =="
sed -n '160,220p' rs/moq-relay/src/config.rs
echo
sed -n '400,760p' rs/moq-relay/src/config.rs
echo
sed -n '300,430p' rs/moq-relay/src/cluster.rs
echo
echo "== config source via crates.io tarball =="
python3 - <<'PY'
import gzip, tarfile, urllib.request, tempfile, pathlib, re
pkg, ver = 'config', '1.7.0'
url = f'https://crates.io/api/v1/crates/config/{ver}/download'
data = urllib.request.urlopen(url).read()
path = pathlib.Path(tempfile.mkdtemp()) / f'{pkg}-{ver}.tar.gz'
path.write_bytes(data)
with tarfile.open(path, 'r:gz') as tf:
for n in tf.getnames():
if 'src' in n and n.endswith('.rs'):
txt = next(tf.extractfile(n)).read().decode(errors='replace')
if 'humantime_serde' in txt or 'value_parser' in txt or 'ParseDuration' in txt:
print(f'--- {n} ---')
for i,line in enumerate(txt.splitlines(),1):
if 'humantime_serde' in line or 'value_parser' in line or 'ParseDuration' in line or 'String' in line or 'Duration' in line or 'default()' in line or 'with_3' in line:
print(f'{i}: {line}')
PYRepository: moq-dev/moq
Length of output: 23535
Keep legacy timeout inputs functional, not just their names.
humantime* requires duration suffixes, so legacy bare-numeric inputs like --drain-timeout 10, drain_timeout = 10, or MOQ_DRAIN_TIMEOUT=10 no longer parse as seconds. The current post-merge environment fallback is also late in merge order, so TOML values can win over legacy env vars and malformed legacy values are ignored. Parse legacy seconds during configuration merging with normal precedence, reject invalid legacy values, and add legacy CLI/TOML/environment coverage for both drain_timeout and cluster.drain_timeout.
📍 Affects 3 files
rs/moq-relay/src/main.rs#L21-L40(this comment)rs/moq-relay/src/config.rs#L66-L74rs/moq-relay/src/cluster.rs#L356-L364
🤖 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 `@rs/moq-relay/src/main.rs` around lines 21 - 40, Update configuration merging
across rs/moq-relay/src/main.rs lines 21-40, rs/moq-relay/src/config.rs lines
66-74, and rs/moq-relay/src/cluster.rs lines 356-364 so legacy drain_timeout and
cluster.drain_timeout values accept bare numbers as seconds through CLI, TOML,
and environment inputs. Parse these legacy values during normal precedence-aware
merging, reject and report malformed values instead of silently ignoring them,
remove the late post-merge environment fallback, and add coverage for both
timeout settings across all legacy input sources.
Source: Coding guidelines
* 'main' of https://github.com/moq-dev/moq: (43 commits) feat(cli): add jemalloc heap profiling (moq-dev#2539) feat(kio): add a poll-native Deadline and adopt it in moq-net (moq-dev#2536) test(moq-srt): size the burst test below the SRT sender's drop window (moq-dev#2540) refactor(net): replace the global LRU cache pool with per-track write-time eviction (moq-dev#2526) ci: compile-check the Windows and macOS code paths on PRs (moq-dev#2531) chore(net): require @moq/qmux 0.3.2 for fallback reset codes (moq-dev#2535) fix(hang,watch): deliver non-sequential group ids incrementally (moq-dev#2484) feat(moq-audio): play decoded PCM out a speaker (moq-dev#2529) fix(js/publish): capture video through a worker MediaStreamTrackProcessor (moq-dev#2528) feat(net): surface a peer's stream reset code as a remote error (moq-dev#2510) feat(mux): expose catalog bitrate/jitter measurement as catalog::Estimator (moq-dev#2530) fix(net): release cache-pool registrations so publishers stop leaking (moq-dev#2525) build(deps): bump the github-actions group with 2 updates (moq-dev#2524) build(deps-dev): bump ruff from 0.15.21 to 0.15.22 in the uv group (moq-dev#2523) build(deps): bump the bun group with 2 updates (moq-dev#2522) chore: bump non-Rust package patch versions (moq-dev#2519) fix(hang): reject a non-hex catalog description instead of misreading it (moq-dev#2516) build(deps): bump the github-actions group across 1 directory with 6 updates (moq-dev#2464) refactor(net): poll kio readiness directly (moq-dev#2515) chore: release (moq-dev#2491) ... # Conflicts: # rs/moq-net/src/client.rs
- lite/subscriber.rs test: add missing goaway_received and going_away fields to SubscriberConfig in test (upstream added test, our branch added new required fields) - moq-relay/config.rs: remove drain-timeout serde/clap compat name from Config::drain to avoid clap collision with moq-native Drain::timeout (both added by goaway branch, compat name no longer needed) - moq-relay/config.rs: add std::sync::Mutex import for test module
MOQ_DRAIN_TIMEOUT now configures the moq-native reconnect drain, so the relay must not also read it as a deprecated alias for its own shutdown drain. Setting it for the client silently reconfigured the server drain and logged a deprecation warning at a correct use. The relay shutdown drain keeps --drain / MOQ_DRAIN. The cluster key still accepts MOQ_CLUSTER_DRAIN_TIMEOUT, which nothing else claims.
The drain keys are new in this branch and have never shipped, so there are no existing configs to keep working. Carrying aliases and env fallbacks for names that were never released is dead weight. Leaves one name per setting: drain / MOQ_DRAIN for the shutdown drain, and cluster.drain / MOQ_CLUSTER_DRAIN for the upstream peer drain.
|
@kixelated pushed a few more commits, mostly housekeeping to get this mergeable again:
I also saw #2542. That looks like a cleaner shape than what I had, and I am happy to just follow up there instead of here if you would rather. Say the word and I will close this one out. One thing that looks missing from #2542 though, and it is the bit you originally flagged: I do not see the draining route getting deprioritized. #2542 keeps the old routes attached at their normal cost and lets tracks hand over at a group boundary, which works when the replacement is genuinely cheaper, but on equal cost What I have on this branch for that: on GOAWAY receipt the subscriber bumps its own routes to (Written by claude-opus-4.8) |
|
Closed in favor of #2542 |
2 participants
Summary
This implements GOAWAY support in
moq-net, on both the moq-lite wire (lite-04+) and the IETF moq-transport wire (draft-14 through draft-19), in both directions. A session can send a GOAWAY to gracefully drain its peer (optionally with a replacement URI and a deadline), and can observe a GOAWAY it receives and react to it.moq-relayconsumes this in both directions:--drain-timeout): the first shutdown signal sends every accepted session (QUIC/WebTransport and the WebSocket fallback) an empty-URI GOAWAY and waits for clients to leave before force-closing; a second signal exits immediately.--cluster-drain-timeout): on a GOAWAY from a cluster peer, the relay dials the replacement while the old session keeps serving, then drains the old session in the background. Downstream sessions never see a GOAWAY of their own, and live subscriptions resume at a group boundary on the new connection.Thanks for the GOAWAY groundwork already in place (the codecs and the receive-path stubs); this completes the lifecycle on top of it.
Built on #2241: migration is the route machinery's job, not GOAWAY's
An earlier draft of this work (pre-#2241) hand-rolled a seamless-failover layer: failover markers on origin paths, silent announce swaps, successor-by-identity tracking, and a
Handofftypestate to drive it. All of that is gone. #2241'sresume/route model already provides exactly those semantics as first-class primitives:resume::Producer::switch), with the first-hop-identity rule deciding route-change vs broadcast-replacement.So GOAWAY here is deliberately thin: the protocol lifecycle in
moq-net, and policy in the relay (when to dial, which redirect to trust, how long to drain). The cluster's migration handler is roughly 60 lines because the model does the heavy lifting. An empty-URI GOAWAY ("reconnect to me") needs no special-casing anymore: a restarted publisher resolves as a different first-hop identity and replaces the broadcast, exactly as the model specifies.moq-net API
Session::drain()claims the one-GOAWAY-per-session slot and returns aDrain(orNoneon versions without GOAWAY, so a caller can't silently no-op into a hang; dropping an unstartedDrainreleases the claim).Drain::start(uri)/start_with_timeout(uri, deadline)send the frame and return aDraining;Draining::complete()waits for the peer to leave, force-closing with the newGoawayTimeoutcode (33) when the deadline expires.Session::goaway()resolves withGoawayReceived { uri, timeout };Session::is_going_away()is a cheap synchronous check. A duplicate GOAWAY (a protocol violation) keeps the first payload and is logged, since an observer may already be acting on the first URI.GoingAway(32); PROBE is silently skipped; existing subscriptions keep flowing until the session closes. Under the resume model the rejection typically surfaces as a stall on the gated route (waiting for another route to serve it) rather than a torn-down subscription.TaskSetchildren racing the drain trigger against transport close (nothing spawned, nothing blocking clean shutdown), andDraining::completepolls under a kio waiter.Two bugs found by the real-transport tests
Writer's drop-reset raced the FIN and discarded the unacked frame; the in-memory mock physically cannot lose data, so only the new quinn tests caught it. Fixed by awaiting FIN acknowledgment before dropping, the same dance assend_setup.Session::closed()stringified the transport error, and quinn'sDisplaydrops the application close code and reason entirely ("connection error: closed").closed()now surfacescode=N: reasonwhen the transport carries them, so a peer can at least distinguish aGoawayTimeoutforce-close from a network failure. See the open question below about going further.Branch target and API compatibility
Target:
main. Everything inmoq-netis strictly additive:Session::{drain, goaway, is_going_away},Drain/Draining,GoawayReceived,Version::has_goaway, and two new variants on the#[non_exhaustive]Error(wire codes 32/33; 31 was already taken byEvicted). The relay config fields land on#[non_exhaustive]structs.One deliberate exception, flagged in the open:
moq_relay::Connectiongains a requiredshutdownfield, andConnectionis a plain pub-field struct that embedders construct with a literal, so this breaks external constructors (the in-repo fix was one line in the smoke test;Shutdown::disabled()is the drop-in value). By the letter of the branch rules that routes todev. We proposemainanyway because the blast radius is one line per embedder and the fix is mechanical. If you'd rather avoid the break entirely, the clean alternatives are an additiveConnection::run_with_shutdown(self, Shutdown), or biting the#[non_exhaustive]+ constructor bullet now (this is the second time a new knob has hit this struct's literal-construction shape). Happy to re-shape either way.Known limitations
RESET_STREAM_AT, unchanged from before and out of scope). Graceful drains are seamless because the in-flight group finishes on the old route; an abrupt mid-group death loses at most the group in flight.Open questions
Session::closed()currently surfaces close reasons as strings (code=33: goaway timeout). GOAWAY introduces the first codes where programmatic matching genuinely matters, and an earlier draft of this work had a fullError::from_codeinverse (payload-free codes round-trip; codes whose variants carry non-wire payloads stayRemote; local-only codes never decode). We deliberately did not include it here: it changes how every close surfaces (not just GOAWAY), it touches the error architecture this repo just rewrote, and it is only as reliable as close-code delivery, which quinn does not currently guarantee (see the race above), suggesting the robust version also wants aweb-transport-trait-level guarantee. If you wantclosed()to return matchable variants, we'll do it as a focused follow-up; thefrom_codesketch is ready.Errorbeing#[non_exhaustive]and the string format being undocumented keeps the migration cost of deferring near zero.unixin the top tier lets an authenticated upstream point the relay at a local socket. A host allowlist is marked as a deliberate follow-up in the code.Scope: Rust behavior + both wires; JS wire-level only
@moq/netdecodes GOAWAY on both wires (with a byte-layout test locking draft-18 Request ID parity against the Rust encoder), so JS clients interoperate safely with a relay that sends GOAWAY. The JS session-level lifecycle (observing a GOAWAY, reconnecting) is deferred to a follow-up PR, consistent with #2241 deferring its own js/net mirror.Test plan
All green: 901 Rust tests across moq-net / moq-relay / moq-native, clippy clean,
cargo doc -D warningsclean,tsc+ biome clean on js/net, kramdown-rfc parses the draft.bun test js/net: all 95 GOAWAY/IETF codec tests pass; the full-suite run carries one pre-existing failure (integration: lite draft-03) that reproduces identically on a cleanmaincheckout (3/3 runs, passes in isolation), so it is not introduced here.GoawayTimeoutforce-close observed by the peer, drain-claim exclusivity and release-on-drop,drain()isNonepre-lite-04, a duplicate-GOAWAY regression (injected as raw wire frames; the public API can't send two), and request gating with an existing subscription proven to keep flowing.test/smoke/smoke.sh, built from this checkout): the cells runnable on the dev host all pass — rust/js publishers x rust/js subscribers, which covers both implementations this PR touches. The python, c, js-native, and gst cells fail identically on a cleanorigin/maincheckout on the same host (missing build deps:just py build,cc, native-libdlopen, glib), so they are environmental; the full matrix should be confirmed vianix develop --command just test smoke-full. Note the smoke flows do not exercise GOAWAY itself (they verify media interop); cross-language GOAWAY behavior testing requires the deferred JS session-level support.cluster_migrates_on_upstream_goaway(two siblings over real TCP),cluster_reconnects_on_empty_uri_goaway, andcluster_diamond_goaway_seamless_failover: a 5-node diamond (TOP -> MID-A/MID-B -> BOTTOM -> subscriber) publishing 24 groups of 3 frames each, 20 of them streamed at a steady cadence through the GOAWAY/reconnect/handover window, asserting every group arrives exactly once, in order, every frame intact and exactly counted; a positive gate that BOTTOM reconnects to MID-B; a post-drain phase with the old leg fully severed proving delivery flows via the new leg; no announce churn; and no GOAWAY cascade to the subscriber (sync flag plus a bounded async probe).Suggested follow-ups
goaway()/ drain lifecycle in@moq/net).RESET_STREAM_AT) for zero-loss mid-group failover. Pre-existing, unchanged by this PR.(Written by Claude Fable 5)