Repository navigation
fix(quinn): preserve raw QUIC application close codes - #409
Conversation
Co-Authored-By: GPT-6 <noreply@openai.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 27 minutes. View limit detailsLimit details: You’ve used all 2 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
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 |
![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: c979d8c237
ℹ️ 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 WebTransportError::Closed( | ||
| code, | ||
| String::from_utf8_lossy(&close.reason).into_owned(), | ||
| ) | ||
| .into(); |
There was a problem hiding this comment.
For a raw session, this returns the decoded peer close without storing it in reason. Consequently, after the peer closes, a later cleanup call to Session::close() can successfully store LocallyClosed; subsequent close_reason(), stream operations, and datagram operations then lose the peer's code and reason. Latch the decoded error before returning it so the remote close remains the first shared close reason.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in 9870c44. CloseReason::map now latches the decoded raw peer close, and Session::close() latches any close that already arrived on the connection before storing LocallyClosed, so the peer code survives even if nothing observed it first. Regression test raw_peer_close_survives_local_close fails without the change.
(Written by Claude Opus 5.5)
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.
Review of c979d8c. Two findings, both reproduced with scratch tests in tests/raw.rs (not pushed).
Medium: a local close() after a raw peer close hides the peer's code
rs/web-transport-quinn/src/session.rs:401, rs/web-transport-quinn/src/error.rs:93
On a raw session the peer's close is never stored; it is decoded on each call. Session::close() stores LocallyClosed whenever the slot is empty, and CloseReason::map returns any stored reason first. So once the app calls close() after the peer has already closed, closed(), close_reason(), and every stream error report LocallyClosed, with session_error() == None.
Scenario: the peer closes with 4075. The app sees closed() return Some((4075, ..)), or a stream error, and calls session.close(0, ..) during teardown, which is a common pattern. From then on, session.closed().await.session_error() is None. Scratch test result: after local close: ConnectionError(LocallyClosed) -> None.
The PR body says it preserves "the first stored reason". That holds for HTTP/3, where run_recv stores the capsule. It does not hold for raw sessions, where the first actual reason is the peer's close and nothing stores it.
Fix: in raw close(), return early if self.conn.close_reason().is_some(). Alternatively, have map prefer a raw ApplicationClosed over a stored LocallyClosed.
Low: raw codes in the HTTP/3 WebTransport range are still decoded as HTTP/3
rs/web-transport-quinn/src/error.rs:55 together with the WebTransportError::Closed(..) => None arm of CloseReason::map
Callers build the SessionError with From<quinn::ConnectionError> before calling map. That conversion already runs error_from_http3. When a raw peer (not necessarily this crate) closes with a code >= 0x52e4a40fa8db that falls on a valid WT mapping, the error arrives as WebTransportError::Closed. map then returns it unchanged. Scratch test: peer.close(error_to_http3(7)) on a raw session gives session_error() == Some((7, "x")) instead of None.
This contradicts the unit test's intent that raw codes above u32::MAX give None. The unit test passes only because it uses u32::MAX + 1, which is not in the HTTP/3 range. The impact is small, since only a non-conforming raw peer can hit it. Fix: decode raw closes from the original quinn::ConnectionError, before the From conversion, or make map re-derive the raw code from ConnectionError and never trust a pre-decoded Closed when raw is set.
Checked, no issues
- HTTP/3 decoding is unchanged. With
raw: false,maphas the same match set and the same stored-first rule as the oldmap_error_with, and the streammap_errorpaths behave as before. All 22 quinn tests pass at head. - The regression test fails without the fix. With
src/reverted toorigin/main,raw_peer_close_code_is_reported_by_session_and_streamsfails withleft: None, right: Some((4075, ..)). - Local raw close still reports
LocallyClosed, and the HTTP/3 local close and capsule paths are untouched. - Lifetime:
CloseReasonholds only abooland aOnceLock<SessionError>, so noArccycle is introduced. Thepoll_closed, stopped, and datagram futures clone theArcas before. - Public API and wire: unchanged.
CloseReasonispub(crate), andRecvStream/SendStreamconstructors were alreadypub(crate). - Parity:
web-transport-noq(src/error.rs:53) andweb-transport-iroh(SessionError::session_errorreturnsNoneforConnectionError, see the raw test atsrc/tests.rs:234) still drop raw peer close codes. This is worth a follow-up so all three backends agree.
(Written by Claude Opus 5.5)
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed c979d8c237753fae08086acd8c357e6ae385739a (full diff).
No additional findings beyond the existing reviews. Source inspection confirms the peer-close preservation finding: error.rs:107–112 returns the decoded raw close without storing it, so session.rs:400–402 can subsequently replace it with LocallyClosed during cleanup. Preserve the remote reason before local teardown and cover peer-close → local-close → session/stream error reads.
The existing code-space finding also remains: error.rs:55–60 decodes before CloseReason::map, so raw codes in the HTTP/3-mapped range bypass the new u32 check. This is a remaining pre-existing defect, not a separate newly introduced regression.
Direction: the shared private handle is appropriately small. Decode the original Quinn connection error once at the binding-aware boundary, rather than correcting an already-decoded error, and retain the first close reason there. No duplicate inline threads added.
Verification: static review of all five changed files, adjacent conversions and call sites, and prior comments. No Rust tests/builds run; this environment has no Rust toolchain, so the reported test/clippy results were not independently reproduced.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review (Written by Claude Opus 5.5) |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9870c44c52
ℹ️ 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".
| | SessionError::SendDatagramError(quinn::SendDatagramError::ConnectionLost( | ||
| connection, | ||
| )) => Some(connection), | ||
| SessionError::WebTransportError(WebTransportError::Closed(..)) => None, |
There was a problem hiding this comment.
When a raw peer closes with a QUIC application code in WebTransport's reserved HTTP/3 interval, the earlier ConnectionError -> SessionError conversion has already turned it into WebTransportError::Closed; this arm then discards the underlying ApplicationClosed, so the raw path reports a different u32 code instead of rejecting the original code as larger than u32::MAX. Decode raw ConnectionError::ApplicationClosed before applying the HTTP/3 conversion so all oversized raw codes behave consistently.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e2541d6. The raw CloseReason now holds the quinn::Connection and decodes its own close_reason() instead of the already HTTP/3-decoded SessionError, so codes above u32::MAX (including those in the WebTransport HTTP/3 range) stay ConnectionError with no session_error(). Regression test raw_peer_close_above_u32_is_not_a_session_error fails without it (Some((7, ..)) for error_to_http3(7)).
(Written by Claude Opus 5.5)
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Delta reviewed: c979d8c237753fae08086acd8c357e6ae385739a → 9870c44c521f758c54d005021b4661f7482243aa; base unchanged.
The previously reported raw peer-close/cleanup case is addressed: rs/web-transport-quinn/src/error.rs:108–118 now latches the decoded reason, and session.rs:400–405 observes an already-arrived connection close before recording LocallyClosed. The added regression deliberately waits on the underlying Quinn connection first, covering the previously unobserved-close case. This is a focused fix with no unnecessary new machinery.
No new findings in this delta. The previously noted raw/HTTP3 code-space issue remains unchanged (error.rs:55–61,99): decoding still happens before the binding-aware mapper, so an oversized raw code in the HTTP/3 range can be reported as a valid application code. That existing issue still needs the original-Quinn-error decoding change and a matching regression; no duplicate inline finding added.
Verification: static review of the three changed files and surrounding paths. Tests/builds were not run because the available environment lacks Rust tooling.
…ping Raw application codes in the WebTransport HTTP/3 range were reported as small WebTransport codes. The raw close handle now reads the connection's own close reason, so every code above u32::MAX is refused. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Delta reviewed: 9870c44c521f758c54d005021b4661f7482243aa → e2541d62efbe02cf75ecfcc7e64d878e289e1ab5; base unchanged.
Both previously discussed issues are now addressed by source inspection. Raw closes are decoded and latched from the original connection reason, including oversized values that would otherwise be misdecoded through HTTP/3. The new integration test covers both u32::MAX + 1 and an HTTP/3-mapped code; the prior peer-close/local-cleanup regression and HTTP/3 isolation test remain.
No new actionable findings. Keeping this binding context private preserves the existing public conversions without spreading special cases through every stream call site. The added connection handle has no back-reference to the shared close handle, so it introduces no ownership cycle.
Verification: static delta and surrounding-call-path review only. Rust tests/builds were not run in this environment.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Merge summary:
Follow-ups, not in this PR: the stream (Written by Claude Opus 5.5) |
1 participant
Raw QUIC application closes in Quinn were decoded through the HTTP/3 error space, so
session_error()lost the peer's code. MoQ's abort and reject regressions fail withweb-transport-quinn0.13.0 while restoring Quinn in moq-dev/moq#4646.Share a private close-reason handle carrying the session's binding. A raw session decodes the connection's own close reason as a
u32application code (codes aboveu32::MAXstay aConnectionError) and latches it, so a later localclose()does not replace the peer's code. The first stored reason is preserved across session operations, stream reads/writes, stop/reset waits, and the send close watcher. HTTP/3 keeps its code space.Public API and wire format: unchanged. This fixes decoding of existing raw closes. Stream RESET_STREAM/STOP_SENDING mapping landed separately in #408; this branch merges it, keeping its stream
rawflag alongside the shared close handle.Validation: the new raw peer regression fails without the fix; all 23 Quinn tests and all-target/all-feature clippy pass. Integration tests also cover a peer close followed by a local close, and raw codes above
u32::MAXincluding ones in the WebTransport HTTP/3 range; both fail without the fix. A unit test covers HTTP/3 isolation.just fixpassed. Repositoryjust checkreaches an unrelated existing WASM example lint atrs/web-transport-wasm/examples/harness.rs:512(chunks_exact_to_as_chunks) with Rust 1.98; that file is unchanged here.(Written by GPT-6)
Updated after review: raw peer closes are latched, and raw codes are decoded from the connection rather than the HTTP/3-decoded error.
(Written by Claude Opus 5.5)