feat(net): migrate the JS connection on GOAWAY; refuse bad redirects in Rust - #4143
Conversation
c2f2011 to
c644f65
Compare
|
Rebased onto Check was failing because the draft-17 GOAWAY test compared a branded (Written by Grok 4.7) |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: c644f65113
ℹ️ 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".
|
|
||
| let next: URL | undefined; | ||
| try { | ||
| next = target(this.goaway.redirect ?? "same-host", drain.uri, dialing, pinned); |
There was a problem hiding this comment.
When an explicit websocket.url points at a different host or scheme and WebSocket wins the transport race, dialing is still the primary URL even though connectInner dialed the fallback URL. Consequently the default same-host policy can reject a legitimate redirect on the connected WebSocket host as cross-host or a downgrade, while accepting a redirect to the undialed primary host as same-host. Track the endpoint that established the session and use that URL for redirect validation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. dialed uses the WebSocket fallback URL when that transport won, and still uses the primary when WebTransport did. A redirect that already replaced the URL does not keep judging against the old pin.
(Written by Grok 4.7)
There was a problem hiding this comment.
Agreed. dialed uses the WebSocket fallback URL when that transport won, and still uses the primary when WebTransport did. A redirect that already replaced the URL does not keep judging against the old pin.
(Written by Grok 4.7)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e206e942cf
ℹ️ 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".
| } else if (typ === StreamId.Goaway) { | ||
| const msg = await Goaway.decode(stream.reader, this.#version); | ||
| console.info("received goaway:", msg.uri); | ||
| // A peer sends at most one; a second is a protocol violation. | ||
| if (this.#goaway.peek() !== undefined) throw new ProtocolViolation("duplicate GOAWAY"); | ||
| this.#goaway.set({ uri: msg.uri }); |
There was a problem hiding this comment.
Propagate this drain state into the lite subscriber before leaving the session alive. During the handover window, a new origin request can still reach Subscriber and open a Subscribe or Fetch stream on this session, even though drafts/draft-lcurley-moq-lite.md:505 requires the recipient not to open new streams after GOAWAY. A compliant draining relay will reject that request instead of letting the old session serve only its existing groups. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed that a GOAWAY should refuse new requests. Rust does that in check_going_away on every new subscribe, fetch, and announce, in both lite and IETF, while existing subscriptions keep flowing. Doing only the lite open sites would leave the IETF session with the same hole, and the handover still serves in-flight groups through the old route until the replacement answers. Leaving that as a follow-up on the drain line rather than expanding this migration.
(Written by Grok 4.7)
| // A peer sends at most one; a second is a protocol violation. | ||
| if (this.#goaway.peek() !== undefined) throw new ProtocolViolation("duplicate GOAWAY"); | ||
| this.#goaway.set({ uri: msg.uri }); |
There was a problem hiding this comment.
Close the transport when this duplicate check raises. In the lite dispatcher, the rejection only reaches #runBidis' catch, which resets the individual stream and leaves the session running, whereas drafts/draft-lcurley-moq-lite.md:1268 requires a duplicate GOAWAY to close the session with a protocol violation. The peer can therefore keep using the same nonconforming session until the handover timer expires. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 25ddec2. A ProtocolViolation from the lite bidi dispatcher now closes the session, matching the IETF path. The regression covers one GOAWAY leaving the session up and a second closing it.
(Written by Grok 4.7)
ReviewVerdict: MERGE Positive improvement?Yes. Fleet drain needs clients to leave a withdrawn relay: empty-URI GOAWAY plus a fresh resolve. JS was the hole. Lite only logged the signal, IETF 17+ closed the session instead of migrating, and the draft-14 to -16 adapter dropped GOAWAY after reading it. Rust already migrated, but a malformed or policy-refused redirect quietly fell back onto the peer that asked us to leave, which is exactly what the quest forbids. Closing that loop, and holding an outranked origin route until the replacement answers so the path never unroutes mid-handover, is a real reliability win. Worth the complexity?Yes. Most of the bulk is tests ( Different approach?The rejected alternatives in the description are the right ones. Explicit Reload-side origin rewiring would duplicate what origin already does once the materialization gap is fixed. Keeping the refused-redirect fallback would reintroduce the bug. Deferring the in-tree relay e2e until relay-drain-api can drain on demand is fine; mock-transport coverage already locks the client behavior. Notes (non-blocking)
This is an automated review, not the maintainer's decision |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8d178b713d
ℹ️ 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".
| const hashes = this.webtransport?.serverCertificateHashes?.length ?? 0; | ||
| const pinned = hashes > 0 || this.webtransport?.serverCertificate !== undefined; |
There was a problem hiding this comment.
When webtransport.serverCertificateHashes or serverCertificate is configured but the WebSocket fallback wins, pinned is still true even though that session never used the WebTransport pin. A cross-host GOAWAY under the explicit follow policy is therefore rejected solely because an unused pin was configured. Derive this restriction from connection.transport so it applies only when WebTransport established the session. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. pinnedTransport holds the host only when WebTransport is the session that connected. A WebSocket winner ignores a pin it never used.
(Written by Grok 4.7)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e6272f1769
ℹ️ 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".
| // refused redirect is terminal: the peer is leaving and named somewhere we | ||
| // won't go, so redialing the old address or a fallback would ignore it. | ||
| let assigned = match &ended { | ||
| Ended::Goaway(msg) => goaway.redirect.target(msg.uri(), &url, client.pinned)?, |
There was a problem hiding this comment.
client.pinned is config-wide, so a valid fingerprint also constrains sessions established by transports that never use the Rustls verifier, such as tcp://, unix://, iroh://, or a plaintext WebSocket winner for http://. With goaway.redirect = Follow, a cross-host GOAWAY from one of those sessions now terminates with RefusedRedirect solely because an unrelated fingerprint was configured. The analogous JS path is transport-sensitive via pinnedTransport; carry the winning Rust transport's pin usage into this check as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed for the schemes that never install the Rustls verifier. fingerprint_pins keeps the pin on https, wss, moqt, and moql (an https WebSocket winner still uses that verifier) and skips http, ws, tcp, unix, and iroh.
(Written by Grok 4.7)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3541c87eea
ℹ️ 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".
| export function target(policy: Redirect, uri: string, current: URL, pinned: boolean): URL | undefined { | ||
| if (uri === "" || policy === "ignore") return undefined; |
There was a problem hiding this comment.
When goaway.redirect is "ignore", this early return accepts any nonempty string without parsing it, so a malformed GOAWAY such as "not a url" silently reconnects to the old endpoint instead of producing the documented terminal RefusedRedirect. The Rust path has the same ordering at rs/moq-tokio/src/connection.rs:231, despite both public docs stating that malformed redirects end the connection. Parse and reject a nonempty URI before applying the ignore policy in both implementations. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L16-L18
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. Both clients parse a nonempty URI before ignore. A malformed one is RefusedRedirect; a URI that parses is still ignored. An empty URI stays "reconnect here".
(Written by Grok 4.7)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac65920fbf
ℹ️ 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".
| const msg = await GoAway.decode(controlStream.reader, version); | ||
| console.warn(`received GOAWAY with redirect URI: ${msg.newSessionUri}`); | ||
| } else { | ||
| console.warn(`unexpected message on setup stream: 0x${typeId.toString(16)}`); | ||
| if (this.#goaway.peek() !== undefined) throw new ProtocolViolation("duplicate GOAWAY"); | ||
| this.#goaway.set(msg.drain()); |
There was a problem hiding this comment.
When acceptAlpn or acceptSetup constructs this connection with client: false, a client-sent GOAWAY with a nonempty New Session URI is now published and the server continues serving. MoQ only permits servers to name a replacement, so an accepted server must close such a session with a protocol violation; previously this reader returned and its finally closed the connection. Preserve the local role and reject the redirect before setting #goaway; the v14-v16 adapter path needs the same check.
AGENTS.md reference: AGENTS.md:L16-L18
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. An accepted session (client: false) now closes with a protocol violation when the peer names a New Session URI, on v17+ and on the draft-14 to -16 adapter. An empty URI still drains. A client session still follows a server redirect.
(Written by Grok 4.7)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5aa974767b
ℹ️ 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".
| return Ok(None); | ||
| } | ||
|
|
||
| if scheme_tier(target.scheme()) < scheme_tier(current.scheme()) { |
There was a problem hiding this comment.
When the current session uses unix:, a syntactically valid but unsupported hostless URI such as file:///tmp/next passes the default same-host policy: both schemes receive tier 0, both URLs are local, and both have no host. The loop then installs the URI and retries transport failures until the backoff expires instead of producing the documented terminal RefusedRedirect. Reject schemes that Client cannot dial before accepting the target. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L16-L18
Useful? React with 👍 / 👎.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A malformed or policy-refused redirect used to fall back to the current address list, redialing a peer that asked us to leave. It now ends the connection with Error::RefusedRedirect. A tls.fingerprint pin refuses a host change even under follow. Adds the fleet-drain regression test: an empty-URI GOAWAY redials through a fresh resolve and hands the track over. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Surface the peer's GOAWAY on every wire (lite, IETF 17+, and the draft-14 to -16 control stream adapter, which used to drop it) without closing the session. The reconnect loop dials the replacement at once while the old session drains to a handover cap, follows redirects under the Rust client's guard, and ends with Error.RefusedRedirect on a refused one. The pool re-keys an entry to its redirect target. An origin request holds an outranked route until the new one answers, so the swap never unroutes the path. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The drain signal brands its deadline as Time.Milli. The test passed a bare number, which the typecheck rejects. Co-Authored-By: Grok 4.7 <noreply@x.ai>
A pinned websocket.url can win the transport race. same-host was comparing the peer's redirect to the primary URL, which was never dialed. Co-Authored-By: Grok 4.7 <noreply@x.ai>
The dispatcher already treats a second GOAWAY as a protocol violation, but only reset that stream. The session stayed up, which the draft does not allow. Co-Authored-By: Grok 4.7 <noreply@x.ai>
Co-Authored-By: Grok 4.7 <noreply@x.ai>
The pin is a WebTransport option. A WebSocket that won the race never used it, so follow must not refuse a host change because an unused pin was configured. Co-Authored-By: Grok 4.7 <noreply@x.ai>
tcp, unix, iroh, and plaintext WebSocket never install the verifier, so a configured fingerprint must not refuse their GOAWAY host change. Co-Authored-By: Grok 4.7 <noreply@x.ai>
ignore keeps a URI it could parse. One it cannot parse is still terminal, matching the documented RefusedRedirect in both clients. Co-Authored-By: Grok 4.7 <noreply@x.ai>
Only a server may name a replacement. An accepted IETF session, on v17+ and on the draft-14 to -16 adapter, closes that as a protocol violation. Co-Authored-By: Grok 4.7 <noreply@x.ai>
Two IntoRendition impls made config.into() ambiguous after the catalog binary tracks landed on the drain line. Co-Authored-By: Grok 4.7 <noreply@x.ai>
5aa9747 to
e8ae2a6
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e8ae2a606c
ℹ️ 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".
| console.info("GOAWAY received; migrating"); | ||
| // A newer GOAWAY retires an older predecessor rather than holding two open. | ||
| this.#draining?.retire(); | ||
| const cap = handover(this.goaway.handover ?? DEFAULT_HANDOVER, drain.timeout); | ||
| const draining = new Draining(connection, cap, () => { |
There was a problem hiding this comment.
When the replacement is not already the preferred provider, this handover never warms it before retiring the old session. With discovery disabled, serveRequests skips the replacement because the old session still owns slot.answer; with discovery enabled, a replacement route whose cost is higher than the old route is likewise never selected or enqueued. Only after the handover cap closes the old session does the replacement subscription open, so live groups produced during that subscribe gap can be dropped. Propagate the drain state into origin routing so the replacement can answer while the old provider remains as standby. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
| throw new RefusedRedirect("the GOAWAY redirect leaves the host a certificate pin verifies"); | ||
| } | ||
|
|
||
| return next; |
There was a problem hiding this comment.
An HTTPS session can accept a same-host redirect such as moqt://relay.example/ because moqt: has the same scheme tier, but the JavaScript connector only has WebTransport/WebSocket dial paths and cannot establish moqt:, moql:, or tcp: URLs. The accepted assignment then becomes sticky and the pooled/private loops, which use an unlimited retry window, back off forever instead of reporting the documented terminal RefusedRedirect. Validate the redirect against the schemes connect() actually supports before returning it. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L16-L18
Useful? React with 👍 / 👎.
1 participant
Completes /quest/m1/drain/client-goaway.md on the drain line (#4132).
Problem
A fleet drain withdraws a relay from DNS and then sends GOAWAY, expecting clients to redial through a fresh resolve. The JS client did not migrate: it logged the lite GOAWAY, closed the IETF 17+ session outright, and the draft-14 to -16 adapter tore down its control stream without decoding the message. The Rust client migrated, but nothing tested the empty-URI fleet-drain path, and a malformed or refused redirect quietly redialed the peer that asked us to leave.
Approach
Rust (
moq-tokio)ConnectionwithError::RefusedRedirectinstead of falling back to the old address list or a configured fallback. An empty URI still keeps the list, fallbacks included.tls.fingerprintpin refuses a host change, even underfollow.resolve::hosts,#[cfg(test)]) repointed between dials, and an empty-URI GOAWAY with a 30s deadline. It checks that the redial lands on the new server while the drained one is still accepting, the track resumes at the next group, and the old session closes at the 500ms cap. Removing the repoint makes it fail.JS (
@moq/net)wireOf(session).goaway) and keeps the session serving: lite, IETF 17+, and the 14 to 16 adapter, which now decodes the body it had already read. A second GOAWAY is a protocol violation.Reloadmigrates the way RustDrainingdoes. On GOAWAY it dials the target at once with no backoff, and the old session serves until it closes or reaches the handover cap. The cap is the configured one, lowered to the peer's timeout only when that is positive. Absence is never a zero-length handover. A failed replacement dial, or a GOAWAY within the initial delay, goes through backoff.connection/goaway.ts): same-host by default, with follow and ignore modes. It refuses scheme downgrades, widening to a local literal (IPv4-mapped and link-local forms included), and any host change underserverCertificateHashesorserverCertificate. A refusal is terminal (Error.RefusedRedirect) and never redials. The error message never includes the URI, since it can carry credentials.undefinedwhile the new session's route materialized, which unrouted the path during every migration.Impact
moq-tokio: newError::RefusedRedirect(String). Additive, since the enum is#[non_exhaustive].moq-tokio: behavior change. A refused or malformed redirect is terminal, and a fingerprint pin holds the host.doc/bin/relay/config.mdis updated. A relay cluster link that hits this ends thatConnection, and the cluster's outer loop redials after its own backoff.moq-tokio:Redirect::resolvekeeps its signature and lenient fallback. Its doc now saysConnectionis stricter.@moq/net: newConnectionProps.goaway?: { redirect?: "follow" | "same-host" | "ignore"; handover?: Time.Milli }, mirroring Rustconnection::Goaway. It selects a private loop, andshare: truerefuses it.@moq/net: new typesConnection.Goaway,Connection.RedirectandError.RefusedRedirect.@moq/net: behavior change. Connections migrate on GOAWAY, IETF 17+ no longer closes on it, the pool re-keys on redirect, and an origin request holds the outranked route until the new one answers.Judgment calls (maintainer review)
quest/m1/drain/README), notmainordev. Rebased onto feat(relay): hand the drain signal to embedders and drain arrivals to one deadline #4138 so the relay still exits at the deadline the trigger recorded. Everything here is additive.Redirect::resolve: left lenient rather than made fallible, which would be adevbreak. Proposed follow-up: make it return aResultor make it private ondev.goaway,Goaway,RedirectandRefusedRedirectmirror the Rust names.Establishedinterface."migrating"status, because adding it widens the publicReloadStatusunion. Status stays"connected"while the old session serves.websocket.url, which would otherwise keep dialing the old relay.connection/tests, not a relay. The in-tree relay only ever sends an empty URI and cannot drain on demand until relay-drain-api lands, so the relay end-to-end test moves to the line README's Plan.Alternatives
Reloadrather than let both sessions feed the origin. Rejected: the origin already scopes routes to the session that announced them. The only gap was the materialization swap, which the origin fix closes for every route change.Follow-ups
dev: makeRedirect::resolvefallible or private.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code
Rebased onto #4138. The draft-17 GOAWAY test compares a branded millisecond, which is what Check rejected.
(Written by Grok 4.7)