Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…dshake fails Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ow-ups Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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: cacaf25ce4
ℹ️ 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".
| - [Closed fallback](/quest/m1/transport-upgrade/closed-fallback.md) - a WebSocket session that closes right after connecting falls back to the pending QUIC dial instead of redialing | ||
| - [JS qmux finish](/quest/m1/transport-upgrade/js-qmux-finish.md) - `@moq/qmux` reports a cleanly finished send stream as closed without error |
There was a problem hiding this comment.
Removing the Rust edge-case quest leaves its GOAWAY-warning goal unresolved: the reviewed tree still pins qmux = 0.5.1 in Cargo.toml, while the commit description states that the clean-finish fix will only arrive with qmux 0.6. Consequently every WebSocket-to-QUIC upgrade can still emit the false failed to send goaway warning from rs/moq-net/src/lite/session.rs; keep this quest open or include the dependency update and regression coverage.
AGENTS.md reference: AGENTS.md:L16-L18
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. qmux 0.6.0 is not published yet (moq-dev/web-transport#402 is still open), so this PR is blocked on that release and will not merge until the qmux bump lands here.
(Written by Claude Opus 5.5)
| let quic = quic.await.map_err(|quic| race_error(quic, err))?; | ||
| // UDP gets through after all, so the next dial gives QUIC its head start. | ||
| crate::websocket::forget(&url); | ||
| return Ok(Dialed::new(connect_session(moq, quic).await?, transport)); |
There was a problem hiding this comment.
When the WebSocket MoQ handshake fails, the pending QUIC transport connects, and the subsequent QUIC MoQ handshake also fails, this expression returns only the QUIC handshake error and discards err. In a mixed failure such as a transient WebSocket error plus a QUIC Unauthorized, the reconnect loop therefore treats the result as terminal even though race_error explicitly keeps mixed auth/non-auth races retryable; route this second handshake failure through the same two-arm error classification.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in 3d38d33: a failed QUIC handshake after a failed WebSocket handshake now goes through race_error, so a mixed auth pair stays retryable.
(Written by Claude Opus 5.5)
When the WebSocket handshake fails and the fallback QUIC handshake also fails, combine both errors through race_error so a mixed auth pair stays retryable instead of returning only the QUIC error. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d38d33589
ℹ️ 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".
| // The fallback got through but its MoQ handshake did not. A QUIC dial still | ||
| // pending may yet connect, bounded by the same deadline as the race. | ||
| let err = Error::from(err); | ||
| let Some(quic) = quic else { return Err(err) }; |
There was a problem hiding this comment.
When a non-auth QUIC failure finishes before the WebSocket transport connects, race_transport_connect returns TransportRace::WebSocket { quic: None } and discards quic_err; if the subsequent WebSocket MoQ handshake fails with Unauthorized, this branch returns that auth error alone, causing dial_any and the reconnect loop to stop even though race_error explicitly classifies a mixed non-auth/auth loss as retryable. Preserve the completed QUIC error in TransportRace and combine it here. This is fresh evidence beyond the earlier double-handshake finding because the QUIC arm has already failed before WebSocket wins, rather than failing during the later fallback handshake.
Useful? React with 👍 / 👎.
This branch has not been deployed
1 participant
Completes /quest/m1/transport-upgrade/polish.md on the transport-upgrade line (#4180).
Draft until qmux 0.6.0 publishes. The GOAWAY fix ships in the next web-transport release from
main(web-transport-trait0.5, qmux 0.6.0, with web-transport#399 and #401). This PR then bumps qmux to 0.6. That is fine here because the line targetsdev.Problem
WARN failed to send goaway: transport: connection closed, although the server receives the GOAWAY.Approach
SendStream::closed()returnsErr(Closed)for every cleanly finished stream, because retiring the stream after its FIN drops the channelclosed()waits on. moq-net's GOAWAY sender waits onclosed()after the FIN, so it warns every time; the lite SETUP stream hits the same error at debug. Fixed upstream in fix(qmux): report a finished send stream as closed cleanly web-transport#402 with a qmux regression test. Against a patched qmux 0.5.1,websocket_upgrades_to_quiclogs neither warning. Not in this PR yet: the qmux 0.6 bump, which waits on that release.race_moq_connectnow falls back to the pending QUIC dial when the MoQ handshake over WebSocket fails. The existing connect deadline still bounds that dial. If QUIC also fails, the error combines the same way as when both race arms fail (race_error, pulled out ofrace_transport_connect). A QUIC dial that lands this way also clears the "WebSocket won" memo, like a successful upgrade (confirmed by the maintainer).failed_websocket_handshake_falls_back_to_quicreuses the held-QUIC forwarder from feat(tokio)!: upgrade a WebSocket fallback session to WebTransport #4189. A fake WebSocket endpoint accepts the client's SETUP stream and closes the session, and only then lets QUIC through. The test is one-shot so a redial can't hide the failure, and it uses draft 16, the newest version whose client handshake waits for the server's SETUP. It fails without the fix (Transport("connection closed")) and passed 15 of 15 stress runs.Fallback::startnow takes aQuicmode enum instead of anopen: bool.Impact
Connectionwhose WebSocket handshake fails now connects over QUIC if that dial lands before the connect deadline, instead of failing the attempt.Alternatives
Err(Closed)as success in moq-net after the FIN. Rejected: that works around a bug in the dependency.Follow-ups
@moq/qmuxreports a finished writable stream as clean.(Written by Opus 5.5)
🤖 Generated with Claude Code