Repository navigation
fix(net): keep opening requests on a draining session until the replacement wins - #4985
Conversation
Co-Authored-By: Grok 4.7 <noreply@x.ai>
Co-Authored-By: Grok 4.7 <noreply@x.ai>
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (18)
💤 Files with no reviewable changes (3)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review. WalkthroughJavaScript and Rust subscribers now continue opening requests after GOAWAY while assigning draining-session routes a high cost. JavaScript connections pass the GOAWAY signal to subscribers, which reprice existing and incoming routes. Rust request paths no longer reject the listed requests solely due to GOAWAY; PROBE creation remains gated. Tests cover requests on draining sessions and handover to replacement sessions. Protocol guidance and quest plans were updated. Estimated code review effort Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The change is mergeable based on the supplied evidence, subject to normal checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 59.09% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 22 functions across 10 files. (5 skipped: 5 unsupported.) ✨ Finishing Touches 💡 1✨ Simplify code
🛠️ Fix failing CI checks 💡
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 |
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 2d5624b
[P2] Recheck GOAWAY after allocating the announce request ID. In js/net/src/ietf/subscriber.ts:364–373 the new guard runs before the await of nextRequestId at line 390; after that await, lines 395–398 open openNativeBi/openBi unconditionally. An announcement started before GOAWAY can be stalled on MAX_REQUEST_ID, receive GOAWAY, then obtain credit and still open a new request on the draining session. The adapter deliberately keeps processing control messages after GOAWAY, so this is reachable. Repeat the guard immediately before the open and preserve the pending-until-closed behavior; add a test whose nextRequestId promise resolves only after GOAWAY. The subscribe setup path already has a second guard.
Direction: stopping new opens and repricing live routes while preserving existing streams is appropriate, but this async boundary leaves the announce path incomplete. Reviewed the complete diff, tests and surrounding allocator/GOAWAY handling. Verification limits: static review only; no tests or live handover run.
After GOAWAY, subscribe, fetch, announce-interest, and TRACK opens proceed on the old session instead of failing, in @moq/net and moq-net alike. Routes stay repriced to the drain cost, so a replacement session outranks the old one and requests move once it answers. The lite draft relaxes the GOAWAY rule to match. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…et with GOING_AWAY Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Automated review:
|
Automated follow-up review:
|
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 327153b
Follow-up to 2d5624b, including the semantic changes inside the main-merge commit. The branch now intentionally keeps opening requests on a draining session and reprices its routes. My previous recheck-after-nextRequestId finding is therefore superseded; restoring that guard would contradict the new direction.
The existing stale-mutation blocker remains independently confirmed: test/drill/mutations/fetch-never-sent.patch still anchors its hunk to the removed going_away guard, while FetchRunState::Open now proceeds directly to Stream::poll_open (rs/moq-net/src/lite/subscriber.rs:4468 onward). Retarget that context and verify the sensitivity patch before merging. This is a static context check, not a local patch execution.
The already-reported per-interest JS listener lifetime also remains: lite/subscriber.ts:332 attaches a #goaway.then callback that retains advertised/announced until GOAWAY, while neither normal completion nor the catch path disposes it. The draft's new retry wording should be a clearly scoped recipient requirement; the current JS behavior does not establish the universal retry the prose describes.
Direction: preserving service until a replacement wins is coherent, but these known integration/lifetime issues still need attention. No duplicate inline finding and no additional distinct defect found.
Verification: GitHub-only static incremental diff, relevant source and discussion review. No builds, tests, benchmarks or interop runs executed; no claim of current CI success or merge readiness. Open state, exact head and prior reviews rechecked immediately before posting.
…n listener - test/drill/mutations/fetch-never-sent.patch anchors on the FETCH open arm now that the GOAWAY gate is gone; `just test drill-sensitivity fetch-never-sent` passes. - The lite announce interest disposes its GOAWAY listener when it ends, so a session that never drains doesn't retain every closed interest. - The lite draft makes the GOING_AWAY retry a recipient SHOULD on another route. - Note at the IETF sites that requests keep opening past draft-19's SHOULD NOT. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Addressed the open findings from the OpenAI review of
(Written by Claude Opus 5.5) |
Automated follow-up review:
|
- Lite draft01/02 initial announces take main's single UNKNOWN_HOP chain and keep this branch's drain cost. - The goaway request test accepts its tracks in milliseconds, since a track has no default timescale after #4968. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 0ea5fe0
Follow-up to 327153b, including the main-merge resolution. The earlier stale GOAWAY hunk context is gone; the mutation now targets FetchRunState::Open. The per-interest listener is disposed in finally (js/net/src/lite/subscriber.ts:329, 513), and the draft now makes retry a recipient SHOULD. The merge preserves drain pricing with UNKNOWN_HOP and supplies the test tracks' required timescale. My original request-ID guard finding remains superseded by the deliberate continue-until-replacement policy.
No new actionable bug found. The remaining malformed context prefixes in test/drill/mutations/fetch-never-sent.patch:14–24 are already covered by Grok's item 6: restore the leading unified-diff spaces. The current sensitivity runner uses patch, so this is a portability cleanup rather than the old stale-context blocker; no duplicate inline comment.
Direction: retain the existing cost-based route selection; it avoids a separate queue/gating mechanism. JS in-flight handover and retry remain explicitly scoped follow-ups.
Verification: GitHub-only static diff and surrounding-source review. No builds, tests, patch application, or interop runs executed. Open state, exact head and reviews rechecked before posting.
(Written by OpenAI)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Merge summary:
The OpenAI review of (Written by Claude Opus 5.5) |
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: e3f82ce
The only change since 0ea5fe0 repairs test/drill/mutations/fetch-never-sent.patch:13–24. Its context lines now have the required unified-diff prefixes, the six-line context matches FetchRunState::Open at the head, and the hunk offset is updated. This addresses Grok's item 6, referenced in my previous review; the mutation itself is unchanged.
No new actionable bug found. The prior listener-lifetime, stale-context and retry-wording fixes remain unchanged. Direction remains the deliberate cost-based continue-until-replacement policy; my original request-ID guard finding stays superseded.
Verification: GitHub-only static incremental diff and source/context comparison. I did not run patch application, builds, tests or interop, and this is not a CI or merge-readiness claim. Open/non-draft state, exact head and reviews rechecked before posting.
(Written by OpenAI)
|
Post-merge summary (
(Written by Claude Opus 5.5) |
Problem
After GOAWAY,
@moq/netkept serving existing subscriptions, but nothing priced the draining session below a replacement, so the origin kept routing to it. The first revision of this PR refused new opens after GOAWAY to match moq-net'sError::GoingAway. The maintainer rejected that: a request that lands in the window before the replacement session is up should not fail at random.Approach
After GOAWAY, new subscribe, fetch, announce-interest, and TRACK opens proceed on the draining session in both
@moq/netand moq-net, on lite and IETF. Every route learned from that session is repriced in place to the drain cost (2^62 - 1, RustCost::DRAIN), including routes announced after the GOAWAY. A replacement at a normal cost then outranks it, and requests move once the replacement answers. A broadcast reachable only through the draining session keeps working until the session closes.The lite draft drops "MUST NOT open new streams after receiving a GOAWAY": the recipient MAY keep opening requests until it has moved to a replacement session. The sender SHOULD keep answering them and MAY reset one with
GOING_AWAY, which the recipient retries on the replacement. This matches moq-transport (draft-19 sect 10.4: the recipient SHOULD NOT start new requests, and an endpoint MAY reject them withGOING_AWAY). Neither the Rust nor the JS publisher rejects requests after a GOAWAY, so both keep answering. A Rust subscriber already counts aGOING_AWAYrejection as a failed route (route_failed), so it moves to the replacement.PROBE stays skipped after GOAWAY on both sides: it is telemetry, not a request.
Decisions
GoingAway(first revision)GOING_AWAY, and the recipient retries on the replacement (maintainer, 2026-10-07)2d5624b(recheck GOAWAY after allocating the announce request id): moot, since the gate is gone.Impact
GOING_AWAY, which the recipient retries on the replacement. No encoding change.Error::GoingAwayafter a received GOAWAY.Error::GoingAwaystays (the relay's shutdown still aborts with it). No API change.@moq/net: routes from a draining session reprice to the drain cost and requests keep opening on it. No new export.Alternatives
Follow-ups
route_faileddoes. JS track handover covers continuity across the swap.GOING_AWAYrejection of a track subscribe fails it instead of retrying on the replacement, because JS has noroute_failedsplice. This belongs with JS track handover. It is not a small change, and no JS or Rust publisher sends that rejection today.moq-relay::drills bursts_cross_a_cluster::impairedfailed once locally with an h3 settings reset during connect, then passed on rerun. It looks unrelated to this change.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code