Repository navigation
fix(tokio)!: keep the transport that won the connect race - #5484
Conversation
|
Quest start outcome: the implementation is done and the PR stays a draft until the maintainer decides. Two calls made while implementing, to check in review:
Open follow-up: should a later QUIC win clear the (Written by Claude Opus 5.5) |
621655a to
a84eeb6
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe transport race now returns the first connected transport and cancels the losing dial. A WebSocket session keeps its transport until closure or GOAWAY, after which the connection follows the normal reconnect flow. Session SETUP tracking and its public wait API were removed. Replacement tests and documentation were updated to describe session replacement and transport racing. Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly identifies the main change: the connection keeps the transport that wins the connection race. It is concise and specific. |
| Description check | |
The description accurately explains the transport-race change, removal of WebSocket-to-QUIC upgrades, related API changes, tests, and validation results. |
| Docstring Coverage | |
Docstring coverage is 84.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 75 functions across 8 files. (8 skipped: 8 … |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
✨ Finishing Touches
-
✨ Simplify code
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Warning
Some tools did not complete. Review the errors below.
🔧 Clippy (1.98.1)
-
Clippy execution timed out
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 help to get the list of available commands.
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. |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: a84eeb6
No actionable bugs found. Keeping the winning transport removes substantial handover complexity and the associated epochless restart path. The race owns and drops its losing future before returning, and a later QUIC win restores its head start. GOAWAY migration, predecessor deadlines, retry accounting and close/abort handling remain coherent; removing Session::setup removes observation plumbing without removing protocol SETUP/auth processing. The renamed replacement tests retain the Release/same-epoch assertions.
The documented removal of same-attempt handshake fallback matches the maintainer's explicit choice. Regression coverage directly observes loser cancellation rather than an instantaneously empty queue.
Limits: static GitHub review of the changes and relevant connection, auth and transport ownership paths. No tests independently run; exact-head CI was still in progress. External dependency cancellation internals were outside this repository-only review.
![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: a84eeb693c
ℹ️ 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".
| client; the relay enables it with `[web.https]`. A session that lands on | ||
| WebSocket stays there until it ends; the next connect races again. |
There was a problem hiding this comment.
After this paragraph establishes that sessions no longer upgrade, a repo-wide search still finds test/portrait/relay.toml:8-9 and test/ts/README.md:596-603 justifying version selection and CI coverage in terms of WebSocket-to-QUIC upgrades; the README even points readers to transport-upgrade tests that this commit deletes. Update those references, along with the stale statements in quest/m1/slow-source.md:8-9 and quest/m3/quic-careful-resume.md:13-15, so the harness rationale and documented coverage describe the behavior that now ships.
AGENTS.md reference: AGENTS.md:L28-L29
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in 22fa4f2. test/portrait/relay.toml and test/ts/README.md now justify lite-07 by a replacement session instead of the upgrade, and the README no longer points to the deleted transport-upgrade tests. quest/m1/slow-source.md, quest/m3/quic-careful-resume.md and quest/m2/quic-sim.md no longer describe the upgrade as current behavior. The two remaining mentions (restart-followers.md, uring-tcp/README.md) record the removal itself.
(Written by Claude Opus 5.5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A connection whose race WebSocket wins now stays on WebSocket for the session's lifetime. The QUIC dial is cancelled the moment WebSocket wins, as WebSocket is when QUIC wins, so an epochless viewer no longer sees a Restart right after connecting. Removes the WebSocket-to-QUIC upgrade from #4180, the closed-fallback handover from #5284, and moq_net::Session::setup(), which only the upgrade used. The WEBSOCKET_WON memo stays and now lasts for the process. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The WEBSOCKET_WON memo lost its only eviction with the upgrade. A later race that QUIC wins now clears the entry, since UDP gets through after all, so the next dial gives QUIC its head start again. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The TS round-trip rig and the portrait relay still justified lite-07 by the upgrade; their epochs now matter only for a replacement session. Three quests described the upgrade as current behavior. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
a84eeb6 to
22fa4f2
Compare
|
Merge summary for head 22fa4f2. Changes: a connection keeps the transport that won the race. The WebSocket-to-QUIC upgrade (#4180) and the closed-fallback handover (#5284) are removed, the losing dial drops the moment the other arm wins, and Decisions (maintainer):
Verification: Public API: moq-net (Written by Claude Opus 5.5) |
Main dropped the WebSocket-to-QUIC upgrade (#5484), so the request token no longer re-attaches after a failed upgrade; both race arms seed the session they keep. Co-authored-by: Kyle Sletmoe <sletmoe@amazon.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Completes the m0 quest "Keep the winning transport" (deleted here).
A
moq-tokioconnection whosehttps://race is won by WebSocket now stays on WebSocket for that session's lifetime. The pending QUIC dial is dropped the moment WebSocket wins, the same way the WebSocket dial is dropped when QUIC wins. An epochless viewer no longer sees aRestartright after connecting, because no second session replaces the first.Changes
rs/moq-tokio: removes the WebSocket-to-QUIC upgrade from feat(tokio)!: upgrade a WebSocket fallback session to QUIC #4180. That coversUpgradeand its stages, theNext::Upgradedhandover, andState::upgradealong with itsabort/closehandling. Also removes fix(tokio): retain QUIC fallback after an early WebSocket close #5284's closed-fallback path (finish_upgrade) and the fallback where a failed MoQ handshake over WebSocket waited on the pending QUIC dial. A WebSocket session that fails or closes early now goes through the ordinary reconnect, which races again.race_transport_connectreturns only the winner. The losing arm drops before it returns.WEBSOCKET_WONmemo stays. Its only eviction belonged to the upgrade, so a later race that QUIC wins now clears the entry (websocket::forgetinrace_transport_connect), and the next dial gives QUIC its head start again. Documented on the static.rs/moq-net: removesSession::setup()and its plumbing (session::Setup, thepoll_seenhelpers, the extraietf::startreturn value) along withtests/session_setup.rs. Only the upgrade used it; GOAWAY drain migration does not gate on it.Connection::transport()andTransport::WebTransportin the bindings, fix(net): retire epochless handles only once their Restart is deliverable #5443'sReleaseorigin fix, andjs/net.rs/moq-net/tests/upgrade.rsis nowreplacement.rsand describes a same-peer replacement session (GOAWAY drain or publisher replacement). TheReleasebehavior it covers is unchanged.doc/concept/transport.md, thewebsocketmodule doc, and theConnection::transport()andStatus::Migratingdocs no longer describe an upgrade.Public API and wire impact
Session::setup()is removed. It was never released (feat(tokio)!: upgrade a WebSocket fallback session to QUIC #4180 is not onrelease), so there is no upgrade note.Status::Migratingnow only means a GOAWAY handover.MoqTransport::WebTransportstays.Tests
client::tests::websocket_winning_cancels_the_quic_dialfails onmain("the QUIC dial outlived WebSocket's win") and passes here. It watches a drop signal on the losing arm.quic_winning_cancels_the_websocket_dialcovers the other direction.connection::tests::quic_winning_cancels_the_websocket_dialreplaces the flakyquic_winning_opens_one_session. The fixture accepts the fallback's TCP socket and only then releases QUIC, so QUIC always wins with both dials in flight. The test then waits for that socket to close, which shows the cancellation directly; the oldtry_recvonly showed an empty queue at one moment.websocket_upgrades_to_quic,a_refused_upgrade_keeps_websocket,abort_ends_an_upgrade_in_flight,close_ends_an_upgrade_in_flight,failed_websocket_handshake_falls_back_to_quic,closed_websocket_session_falls_back_to_pending_quic, theclosed_fallback_*anda_failed_upgrade_stays_connectedtests, the upgradeRetrytests,a_stalled_upgrade_gives_up_at_the_deadline, andrace_transport_connect_keeps_a_pending_quic_dial.client::tests::quic_winning_restores_its_head_startfails with the eviction line removed ("QUIC won, yet the next dial still skips its head start") and passes with it.just check: passes on the final head (6262 Rust tests plus lint, docs, and JS). Earlier runs hitmoq-relaywall-clock flakes already tracked byquest/m1/test-flakes-2/auth-refresh-mock-time.md(a_fixed_lease_still_expires,an_outage_refuses_transiently_and_never_extends_the_expiry,renewal_outlives_the_initial_credential_and_withdrawal_ends_only_its_grant); none touch the race, and each passed in isolation.just test interop --all: all checks passed on the final head.Decisions
WEBSOCKET_WONentry, restoring QUIC's head start (maintainer).🤖 Generated with Claude Code
(Written by Claude Opus 5.5)