Repository navigation
fix(net): drain received group tails after session close - #5762
Conversation
Retain subscription state while the closed transport drains buffered streams and settle IETF control waits. Deliver pending WebTransport uni streams before surfacing connection close. Regress published Lite versions and both IETF session paths, including control-first polling. Co-Authored-By: GPT-6 <noreply@openai.com>
The optional production memory-transport acknowledgement change regressed the high-stream-count benchmark, so retain its existing semantics and use the test transport delivery-ACK knob for receiver-close coverage. Co-Authored-By: GPT-6 <noreply@openai.com>
The existing TCP publisher restart regression passes on main but hangs with receive draining. A direct qmux 0.6.3 reproduction confirms unfinished stream reads remain pending after session and accept-queue closure while Session handles retain the shared stream table. Record the upstream dependency blocker and clarify that Session::closed does not finish receive draining. Co-Authored-By: GPT-6 <noreply@openai.com>
|
Blocked on qmux 0.6.3 teardown. A direct TCP reproduction keeps a
The quest remains open. Fix the qmux lifecycle upstream, coordinate with the existing close/credit work, then release/pin and rerun the gate. No timeout workaround, merge, deployment, or release was made. (written by GPT-6) |
|
The terminal stream hang now has a focused source fix in moq-dev/web-transport#437. The shared stream table retained channel senders after backend exit, so draining an unfinished receive stream never completed while a Session handle remained alive. The fix releases those senders and guards concurrent registration, preserving buffered bytes and received FINs without changing the public API or wire format. Source validation passes on clean 601e7fb: pinned-Nix Remaining blocker: review/merge the upstream fix, publish a qmux release containing it, then update the dependency and lockfile here and rerun full clean gates and interop. No crate publication or unpublished dependency pin was added. (written by GPT-6) |
…rain # Conflicts: # rs/moq-net/src/ietf/session.rs # rs/moq-net/src/lite/session.rs # rs/moq-net/src/lite/test_transport.rs
A post-close transport error now carries the session's first close code, so a driver that ends on that error reports the same code the session closed with. The tail regressions wait until both ends hold the subscription, so they test the group tail rather than a SUBSCRIBE_OK still in flight. Record that main still pins qmux 0.6.3 and that the fix must come from moq's own stack. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
qmux 0.6.5 (moq-dev/web-transport#437) ends receive-stream reads at session teardown, which the close-tail drain relies on. It uses trait API from web-transport-trait 0.5.1, so the lockfile moves that too. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.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 11 minutes. View limit details
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe changes update Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 31.08% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 74 functions across 11 files. (1 skipped:… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly and concisely describes the primary change: draining received group tails after session close. Description check The description is directly related to the changeset and explains the problem, implementation, dependency update, validation, and known follow-ups. 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.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 31.08% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 74 functions across 11 files. (1 skipped: 1 too large.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
✨ 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: fbed668b98
ℹ️ 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 closed = self.uni.poll(waiter).is_ready(); | ||
| let drained = self.uni.children.poll(waiter).is_ready(); |
There was a problem hiding this comment.
When lite-07 offers the IETF extension and a peer opens a GROUP stream but closes before completing its SETUP stream, GroupRecv remains pending in peer_setup.poll_ietf(): the failed SETUP child exits without filling that slot, while this drain waits for every child to finish. Transport closure cannot resolve the shared setup slot, so the spawned session driver and its subscription state remain retained indefinitely; settle or fail PeerSetup before waiting for the children.
AGENTS.md reference: AGENTS.md:L17-L18
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, and it also hangs when the SETUP stream was never opened before close. Fixed in 4329029: GROUP streams parked on the peer SETUP are counted, and once the transport is closed, the SETUP is still unknown, and every remaining uni stream is parked, the drain ends (those groups are cancelled, as before the drain). Regression: drain_ends_when_groups_wait_on_an_undeliverable_setup covers both the truncated and the missing SETUP, and timed out without the fix.
(Written by Claude Opus 5.5)
With the ietf extension offered, a GROUP stream parks on the peer's SETUP before reading its header. Once the transport is closed and every remaining uni stream is parked that way, no stream can still deliver the SETUP (it was truncated or never opened), so the drain waited forever. Count parked streams and stop draining when they are all that is left. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review (Written by Claude Opus 5.5) |
|
Codex Review: Didn't find any major issues. Bravo. 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". |
# Conflicts: # quest/m1/close-tail-drain.md
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Merged
(Written by Claude Opus 5.5) |
# Conflicts: # quest/m1/close-tail-drain.md
|
Merged (Written by Claude Opus 5.5) |
Main's #5762 added RecvError to keep a real receive stream's error through the adapter, while this branch added StreamFailure for both directions. Unify them as one generic AdapterError that keeps the transport error typed for send and receive, and forwards close_kind. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Keep #5762's RecvError over leased reads, forwarding close_kind so an eviction still surfaces as Evicted. Port its scripted-uni close test to poll_read_chunk. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Problem
Subscribers drop final groups their transport already delivered when a peer closes the session. This happens on released moq-lite versions and on both IETF session paths.
Approach
Close the transport first, then run a receive drain bounded by buffered data. Subscription state stays alive while accepted and queued group streams finish. IETF waits on control responses that can no longer arrive are settled. Both WebTransport adapters (moq-tokio and moq-uring) hand out buffered uni streams before they report the connection closed.
On lite with the ietf extension offered, a GROUP stream waits for the peer's SETUP before reading its header. Once the transport is closed and every remaining uni stream waits on a SETUP that is still unknown (truncated or never opened), nothing can deliver it, so the drain ends and those groups are cancelled as before.
The drain needs every transport to end an unfinished read once the session closes. qmux 0.6.3 and 0.6.4 did not: after teardown,
Sessionhandles kept the sharedStreamstable alive, and it still held each receive stream's channel senders, so a read past its buffered data stayed pending forever. That was fixed at the source in moq-dev/web-transport#437, released as qmux 0.6.5, and this PR bumps the workspaceqmuxrequirement to 0.6.5.Impact
qmux0.6.3 -> 0.6.5. qmux 0.6.4+ uses trait API added inweb-transport-trait0.5.1 while declaring"0.5", soCargo.lockalso movesweb-transport-trait0.5.0 -> 0.5.2.Decisions
Taken in this round
closed()resolves (works around the dependency bug)Taken in earlier rounds
subscribe()to resolve before writing the tail. The control-response race is a known gap (see Follow-ups).Validation
drain_ends_when_groups_wait_on_an_undeliverable_setuptimes out without the lite SETUP fix and passes with it; moq-net passes (2874 tests) and clippy is clean.a_closed_publisher_session_resumes_on_the_next_startanda_moqsink_restart_keeps_pads_when_caps_arrive_at_different_timestimed out on every run.just check origin/main: everything up to the flake step passes (Rust lint, docs, clippy, quest).nix flake checkstops at the GitHub runner VM image, which needs user namespaces this sandbox lacks; unrelated to this diff. The steps after it were run by hand: ts-paths, shell, toml, markdown, gh pass, and workspacetest-changed(the lockfile change selects every crate) passes 9440 + 516 + 224 tests. The nix and justfile format checks were not run (no such files changed).just test interop --allwas not rerun (no wire change); it passed on an earlier head.Follow-ups
web-transport-trait = "0.5.1"; a consumer whose lockfile pins trait 0.5.0 fails to build qmux 0.6.4+.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code