Repository navigation
fix(net): finish legacy tracks whose FIN races a graceful close - #6969
Conversation
Quest-Claim: 65071a8e756010dc
Co-Authored-By: GPT-6 <noreply@openai.com>
kixelated
changed the title
Before lite-05 the subscribe stream's FIN is a track's only end, but js/net released the copy without declaring one, and the driver aborted every subscription as soon as the transport closed. Mirror moq-net: a FIN finishes the track once its tail settles, and a closing session first drains the group streams and subscribe responses the closed transport already delivered, so a track its publisher ended finishes rather than aborts. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
changed the title
…fecycle/js-close-fin # Conflicts: # js/net/src/lite/connection.ts
Since #6978, closing a track truncates its unfinished groups, so the test's never-finished first group made lite-01..04 abort at session close. The test checks delivery of the final group, so finish the first one explicitly. Also repoint the cut-audit quest's link to the deleted js-close-fin quest at this PR. 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 10 minutes. View limit details
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: 4022ae7399
ℹ️ 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".
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 4022ae7
P1, already independently reported: actively read the legacy response stream through EOF. Confirmed in js/net/src/lite/subscriber.ts:973-978 and the new drain at js/net/src/lite/connection.ts:332: QMux's zero-high-water-mark receive stream only closes inside pull(), so awaiting reader.closed after SUBSCRIBE_OK can leave the drain pending forever and prevent session cleanup. Use an EOF read that rejects unexpected trailing bytes, and cover the path with a QMux-backed legacy-version regression. Referencing the existing thread rather than adding a duplicate inline comment.
Direction: draining delivered groups and responses before terminating subscriptions is appropriate and avoids an arbitrary timer, but transport closure alone is not a sufficient bound until the legacy response path actively reads. No additional actionable bugs found in the inspected changes.
Verification: static inspection of connection/subscriber lifecycle, Reader, QMux receive behavior and regression tests only; tests and real-transport scenarios were not run.
Before lite-05 the subscriber only awaited the subscribe stream's reader.closed after SUBSCRIBE_OK. A QMux receive stream closes only on a read, so that promise never settled there: the track never finished and the closing session's drain waited forever. Run runResponses on every version instead, which reads to the FIN, applies a lite-03/04 SUBSCRIBE_DROP like Rust does, and rejects a second SUBSCRIBE_OK. 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 commit: a8299c0
The prior P1 QMux drain finding is fixed: legacy subscriptions now actively read responses through EOF instead of awaiting reader.closed. The new real-QMux-pair regression covers lite-01..04. Allowing defined lite-03/04 SUBSCRIBE_DROP messages while rejecting a duplicate SUBSCRIBE_OK is the appropriate refinement of the earlier trailing-data recommendation.
No new actionable bugs found in the delta. Reusing the existing response loop is simpler than a separate legacy drain path and preserves the intended finish-after-tail behavior.
Verification: static inspection of the one-commit delta, response decoder, lifecycle context, and added tests; tests and reported CI results were not independently executed.
|
@codex review (Written by Claude Opus 5.5) |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. 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 head a8299c0:
Suggested follow-ups, not holding this merge:
(Written by Claude Opus 5.5) |
Problem
Before lite-05, the subscribe stream's FIN is the only signal that a track ended. When a peer gracefully closed a session right after finishing its tracks, a JS receiver on lite-01 through lite-04 delivered the final group and then failed the track with
SessionCode.Cancelinstead of finishing it. Two causes:#serveSubscribereleased the copy after a FIN without declaring an end, so the reader never saw the track finish, even on a session that stayed open.Subscriber.close(err)as soon as the transport closed. That aborted every subscription, including ones whose publisher had already ended them, and it could run before the group streams and subscribe responses the closed transport had already delivered were read.Approach
Mirror moq-net (
SubscriptionCleanupandpoll_drain, moq-dev/moq#6741):runResponses, rather than awaitingreader.closed: a pull-driven QMux stream closes only on a read. Before lite-05 that FIN marks the subscriptionfinished, a lite-03/04 SUBSCRIBE_DROP is applied as Rust does, and a second SUBSCRIBE_OK is a protocol violation.finished. The track finishes once the tail settles, andSubscriber.closefinishes afinishedtrack before ending it, whether the close was clean or an error.Connectionfirst closes the transport, which bounds every read. It then drains the unidirectional streams still being read and each subscribe stream's responses, and only then ends the subscriptions. The bound comes from the closed transport, not from a timer.QMux keeps a finished stream's unread bytes across a CONNECTION_CLOSE, so the drain matters on a real transport, not only in tests.
Tests
a peer close drains the overtaken group: incoming uni streams are buffered outside the mock and delivered only after their FIN, so the close lands while the final group is still being decoded. Without the uni drain it fails on lite-01..06; lite-07 waits for the subscriber's FIN anyway.a peer close drains the overtaken fin(lite-01..04): bidi FINs are delivered a task after the close. It fails without the subscribe-response drain.a peer close finishes a FIN'd track over qmux(lite-01..04, real QMux pair): awaitingreader.closedinstead of reading to the FIN times out, since the drain never ends.a lite-0N publisher FIN finishes the track(subscriber unit test, mocked clock): a legacy FIN finishes the track on a session that stays open.Each piece of the fix has a test that fails without it.
Impact
Subscriber.drainis@internal.Choices made without the maintainer
quest/m1/broadcast-lifecycle, and every other item in that epic's Plan already landed (moq-dev/moq#6660, #6636, #6656, #6669, #6740, #6741), so the epic is deleted too. Its links inquest/m1/README.mdandquest/m2/ietf-finish.mdare removed.#failhelper from moq-dev/moq#6978, still after the drain. A lite protocol violation still calls#failright away, skipping the drain, since the session is already broken.quest/m1/cut-audit/js-close-cause.md(added on main) linked the deleted quest; it now links this PR.Subscriber.closefinishes afinishedtrack and then applies the session error. JS's existing rule then decides: the track ends cleanly only if its tail settled, the same way a lite-05+ track with SUBSCRIBE_END already behaves. Rust's cleanup callsfinish()on it instead of aborting. I kept the JS rule rather than change lite-05+ semantics here.Follow-ups (not quests yet)
a server refuses a client offering only IETF versions in the moql SETUP(js/net/src/connection/connect.test.ts) failed once underjust checkload, then passed five reruns. Theacceptpromise looks like it can reject before the test attaches.rejects.The mock resolves a stream's FIN acknowledgement once the bytes are queued at the peer, not once they're read, and its close discards unread queued chunks. WebTransport likewise errors a session's receive streams on close, dropping their unread bytes. So on lite-01..06, a publisher's close after the ack can still cut a final group the subscriber's JS hasn't read yet; lite-07's subscriber FIN avoids this. Worth a quest to check whether the lite-01..06 publisher drain should wait for more than the transport ack.
JS and Rust may disagree on a publisher-ended track whose tail is unsettled at session death: JS aborts it, while Rust's cleanup calls
finish(). Worth checking which one is right.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code