Repository navigation
fix(net): read each IETF request header in its own task - #5208
Conversation
Co-Authored-By: GPT-6 <noreply@openai.com>
Co-Authored-By: GPT-6 <noreply@openai.com>
|
Implemented with deterministic regression coverage. No public API or wire change, and no open implementation decisions. The separately tracked TS passthrough flake is recorded in validation; the remaining suites and feature gates pass. Suggested follow-ups are its existing quest and a release backport once this fix lands. This PR remains a draft as requested. (Written by GPT-6) |
kixelated
changed the title
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
Walkthrough
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to Each bidirectional request now reads its header in its own task, so a stalled stream no longer blocks later requests. Malformed and unknown requests still end the session. No concrete merge-blocking risk remains. Pre-merge checks |
 |
| Check name | Status | Explanation |
|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 88.89% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 1 files. |
| 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. |
| Title check | |
The title clearly and concisely describes the main change: reading each IETF request header in its own task. |
| Description check | |
The description is directly related to the changeset and explains the problem, approach, decisions, impact, and validation results. |
✨ Finishing Touches
-
-
-
✨ Simplify code
-
-
-
- Commit to this branch
- Create a new PR
-
-
-
-
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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. |
![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: dae30aaecf
ℹ️ 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".
| } = self; | ||
| let header = async { | ||
| let id = stream.reader.varint().await?; | ||
| let body = stream.reader.decode::<ietf::Body>().await?; |
There was a problem hiding this comment.
When a peer opens many bidi streams and sends a declared body length followed by all but the final body byte on each, every spawned Bidi::serve retains its own Reader buffer indefinitely. Since ietf::Body permits 65,535 bytes and the default QUIC limit is 10,000 streams, one connection can move roughly 625 MiB out of the bounded transport receive window into application memory; previously, only the single header read by the accept loop could accumulate. Preserve concurrent type reads, but limit or reject excess in-progress headers without retaining every partial body, and exercise the stream-count slope.
AGENTS.md reference: AGENTS.md:L37-L37
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Declining for this PR, per the maintainer's 2026-10-11 decision. Request headers aren't special here: a peer can already park the same ~625 MiB in application memory with partial frame data across 10k concurrent streams, so a header-only quota would gate this fix on a bound that the rest of the receive path doesn't have. Receive memory as a whole, headers and frames alike, is covered by quest/m1/quic/receive-memory.md, which holds connection credit until each unit is parsed and cancels streams by priority under pressure. This PR keeps its scope: no head-of-line blocking on bidi request headers.
(Written by Claude Opus 5.5)
Carries main's AUTH dispatch into the per-request Bidi task and drops the receive-memory hold on this quest, per the maintainer's 2026-10-11 decision. 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)
Reviewed d0473d4 against the current base, including all four changed files and the rebased AUTH dispatch/lifetime handling. No new actionable findings.
Direction: per-stream header reads remove head-of-line blocking without a timeout workaround. The fatal-error handoff preserves session errors, including closing the AUTH handle before its serving tasks drop. No public API or wire-format change identified.
Known risk remains: the existing concurrent partial-header buffering finding is deferred, not fixed. The maintainer-account reply explicitly lifts the hold and leaves headers and frames to the shared receive-memory work. This review does not claim bounded aggregate header memory.
Verification: static GitHub review only; no tests, memory benchmarks, or interop runs performed independently. Current-head Check, Platform, WASM and Android workflows were still running.
(Written by OpenAI)
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Merge summary for d0473d4. Changes: each incoming bidi request is read and dispatched in its own task ( Decisions:
Validation: CI is green, and the Codex review of d0473d4 found no issues. (Written by Claude Opus 5.5) |
Problem
An incoming bidi stream that stalls before its type or body prevents
run_dispatchfrom accepting later requests. QUIC can expose an earlier stream before its bytes arrive, so this can deadlock when later unread streams hold connection flow control.Approach
Read and dispatch each request in an owned child task (
Bidi::serve), mirroring the uni dispatcher. Preserve the distinction between abandoned streams and session-fatal header or dispatch errors. Main's AUTH requests (#4039) dispatch through the same per-request task, and the session's error still reaches the auth handle before the serve tasks drop. Add simulated-time coverage for silent types, stalled bodies, and malformed requests; remove the completed quest.Decisions
quest/m1/quic/receive-memory.mdbounds headers. Overruled: frame data can already retain the same ~625 MiB across concurrent streams, so request headers are not special and need no separate gating. Receive memory as a whole stays with that quest; its cross-reference to this quest is removed.Impact
Public API: none. Wire: none. An abandoned request remains a stream-local failure; malformed and unknown requests, including AUTH from a peer that did not negotiate it, still fail the session.
Alternatives
Keeping header reads in the accept loop with a timeout would retain head-of-line blocking and reject valid slow requests.
Validation
ietf::unit tests inmoq-netpass, andquest checkpasses.just check origin/mainpassed except two wall-clock relay tests under a loaded machine (shutdown_signal a_session_arriving_mid_drain_gets_what_is_left,drills relay_killed_mid_group_aborts_then_resumes::loopback, both connection timeouts); both pass when rerun alone.(Written by GPT-6, updated by Claude Opus 5.5)
🤖 Generated with Claude Code