Don't rank a quiche send half the peer already stopped - #391
Conversation
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: 02a3cc43a1
ℹ️ 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".
| let drained: Vec<StreamId> = self | ||
| .draining | ||
| .iter() | ||
| .copied() | ||
| .filter(|id| qconn.stream_capacity((*id).into()).is_err()) | ||
| .collect(); |
There was a problem hiding this comment.
stream_capacity only errors when quiche collects the completed stream, which for a normally finished send is after its data is acknowledged, rather than when its buffered bytes have left the scheduler. If 255 distinct finished-but-uncollected priority levels rank ahead of new active streams—for example with a configured stream limit above 255—the active levels all map to the overflow urgency and round-robin despite having different send orders, even though the retained streams have no bytes left to schedule. Track scheduler drain separately or otherwise exclude ranks that are only awaiting collection so the documented top-256 ordering remains effective.
AGENTS.md reference: AGENTS.md:L48-L49
Useful? React with 👍 / 👎.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review. WalkthroughThe driver now registers an accepted stream in Merge Risk: ⚪ Minimal · up to The change prevents already-closed send halves from occupying scheduling state while preserving the existing application-facing stream behavior; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
…opped `accept_bi` registered the accepted send half unconditionally. If the peer sends STOP_SENDING before we accept, that first `SendState::flush` already came back closed, and nothing will ever flush it again: `notify` skips a closed state and quiche won't report a collected stream writable. The stale entry in the driver's send map predates the priority ranking. What the ranking added is that the stream also holds an urgency band for the life of the connection, so a peer repeating this can push live streams toward the overflow band and flatten their relative order. Track only live send halves. The application still receives the handle and sees `Stop(code)` on first use, or `Closed` if quiche had already discarded the code. `priority.insert` moves out of `push_accept_bi` into the driver, which is the half that knows whether the flush survived. Also bound `tests/priority.rs`, whose every await was unbounded — a stalled stream parked `finished.recv()` or a writer join until CI's job timeout rather than failing the test. Tests: `rebalance_matches_a_full_ranking` drives 10k pseudorandom ranking operations against a from-scratch ranking. The early exit in `rebalance` is the one part of the mapping that isn't locally obvious, and it only runs once the overflow band is occupied, so the run is weighted to climb past `BANDS` levels and `saw_overflow` asserts it got there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
02a3cc4 to
600d80d
Compare
kixelated
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 600d80d289
ℹ️ 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".
| // again would strand it here for the life of the connection: `notify` skips | ||
| // a closed state and quiche won't report a collected stream writable. The | ||
| // application still gets the handle, which reports the stop on first use. | ||
| let live = !state.is_closed(); |
There was a problem hiding this comment.
No test drives the fixed scenario: a peer-opened bidirectional stream whose send half receives STOP_SENDING before accept_bi initializes it. The new randomized test covers only Priorities bookkeeping, while the integration-test change only adds a timeout, so neither would fail if this lifecycle fix were removed or if SendState::flush stopped marking this state closed. Add a regression test that exercises this ordering and verifies the stopped send half is not retained or ranked.
AGENTS.md reference: AGENTS.md:L46-L47
Useful? React with 👍 / 👎.
1 participant
Follow-up to #390, narrowed. That PR's review turned up three lifecycle defects; this lands the one whose fix is unambiguous, plus test hardening. The ranking math in
ez::Prioritiesis unchanged — it was correct.The fix
accept_biregistered the accepted send half unconditionally. If the peer sends STOP_SENDING before we accept, the firstSendState::flushalready comes back closed — and nothing will ever flush it again, sincenotifyskips a closed state and quiche won't report a collected stream writable.The stale entry in the driver's send map predates the ranking. What #390 added is that the stream also holds an urgency band for the life of the connection, so a peer repeating this can push live streams toward the overflow band and flatten their relative order.
Only live send halves are tracked now. The application still receives the handle and sees
Stop(code)on first use (orClosedif quiche already discarded the code).priority.insertmoves out ofpush_accept_biinto the driver, which is the half that knows whether the flush survived.Test hardening
tests/priority.rshad no bounded await anywhere, so a stalled stream parkedfinished.recv()or a writer join until CI's job timeout instead of failing. Now bounded at 30s.rebalance_matches_a_full_ranking— 10k pseudorandom ranking operations checked against a from-scratch ranking after every step. The early exit inrebalanceis the one part of the mapping that isn't locally obvious, and it only executes once the overflow band is occupied, so the run is weighted to climb pastBANDSlevels andsaw_overflowasserts it actually got there.Deliberately not included
Two findings from the #390 review are left open, because the obvious fix for them turned out to be wrong:
SendStatereports closed the moment its FIN reaches quiche, so the band is freed while quiche still has buffered bytes; the promotion that follows can lift a lower-priority stream level with data that outranks it.set_priorityimmediately beforefinishis dropped, sincePriorities::removeclears the pending update.The natural fix — hold the rank until
qconn.stream_capacity(id)errors — is sound but not complete: quiche only drops a stream onceis_complete(), which for a bidirectional stream requires both directions. A bidi stream whose send half is finished and acked keeps returningOkwhile the peer's half stays open, so the rank would never be reaped. That is worse than the bug it fixes, and "finish your send, keep reading the response" is the normal bidi pattern. quiche 0.29 exposes no send-side drain predicate (stream_closedreports FIN written,is_flushableisn't onConnection), so this needs a different mechanism and belongs in its own change.cargo test -p web-transport-quichegreen;cargo clippy --all-targets -D warningsandcargo fmt --checkclean.🤖 Generated with Claude Code
(written by claude-opus-5)