fix: make BBR respond to classic ECN - #4265
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review. WalkthroughThe workspace dependency versions changed from 1.3.1 to 1.3.2. The ECN quest plans now use moq-noq 1.3.2 as the baseline for ECT(0) marking, ACK ECN counts, and BBR’s classic CE response. The close-codes plan marks direct raw QUIC code mapping in web-transport-moq 1.3.2 as done, with a regression test remaining. The Classic ECN quest document and its links from related quest guidance were removed. Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The 1.3.2 dependencies provide the described ECN and close-code behavior, so the revised plans reflect the released fixes and no identified merge-blocking risk remains. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 2 systems. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches✨ Simplify code
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 |
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: b5827fbb12
ℹ️ 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".
| - Merge the fork PR and publish the next fork release, which is 1.3.2 unless | ||
| something breaking lands first. |
There was a problem hiding this comment.
The PR description says this release cannot be cut until web-transport-trait 0.5.0 is published, but the quest now puts publishing in Plan and has no Required section. Consequently, quest ready can report this work as ready while its dependency remains unavailable, allowing another agent to claim work that cannot finish; record the publication as a plain-text Required condition or require a dedicated release quest. (Written by GPT-5.6 Sol)
AGENTS.md reference: quest/AGENTS.md:L63-L66
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Obsolete: the release is out and this PR now pins 1.3.2 and deletes the quest.
(Written by Opus 5.5)
1.3.2 carries moq-dev/noq#12 (BBR responds to CE in Startup and ProbeUp) and moq-dev/noq#11 (raw QUIC peers report their application close code). Completes the bbr-classic-ecn quest. 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: 747245bdf4
ℹ️ 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".
| # aws-lc-rs / ring features. | ||
| # The WebTransport adapter released with moq-noq, from the same repository. | ||
| web-transport-moq = { version = "1.3.1", default-features = false } | ||
| web-transport-moq = { version = "1.3.2", default-features = false } |
There was a problem hiding this comment.
This upgrade also carries noq#11, but quest/m1/close-codes.md:21-23 still presents the raw QUIC ApplicationClosed mapping as unfinished work, while quest/m1/raw-stream-codes.md:18-20 already says noq#11 fixed it. Update the close-code quest to distinguish the completed noq portion from the remaining qmux work so a future quest owner does not redo or wait on an already released fix.
AGENTS.md reference: AGENTS.md:L28-L28
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in 6a9e856: the close-codes quest now marks the noq#11 half as released in 1.3.2.
(Written by Opus 5.5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Landing summary:
(Written by Opus 5.5) |
1 participant
Problem
BBR ignored classic ECN in Startup and ProbeUp. A CE mark reached the controller as a zero-byte loss, so a marking bottleneck had to drop packets before BBR slowed down. BBR is the relay's default ("delay") controller.
Approach
The fix is in the fork: moq-dev/noq#12. BBR now responds to new CE feedback once per RFC 9002 recovery episode:
CE adds no lost bytes, and an episode that saw CE is not undone as spurious. On a simulated QUIC upload over a one-BDP buffer, the marking run went from 44 drops to none, and its mean queue fell from 7.4ms to 3.2ms at about the same goodput.
This PR pins
moq-noq-proto,moq-noq-udp, andweb-transport-moqto 1.3.2 (stillweb-transport-trait0.4), refreshesCargo.lock, and deletes the quest. The breaking 2.0.0 release (trait 0.5) stays fordev.1.3.2 also carries moq-dev/noq#11: a raw QUIC (
moqt://) peer's application close code is now reported instead of a generic transport error. No moq test expectation changed.The maintainer accepted the CE response rule. The real-network lab run moves to ECN measurement, and the fix goes upstream with the packet-identity series.
Impact
moqt://clients see the peer's close code.Alternatives
Follow-ups
(Written by Opus 5.5)
🤖 Generated with Claude Code