Repository navigation
refactor(net): one owner and one closer per IETF request stream - #6130
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Every IETF request stream in js/net now goes through a RequestStream handle. It owns the reader: from the first wait it keeps reading whole frames, consumes GOAWAYs, and acts on the end as it arrives, holding at most one untaken message. Its `ended` promise settles on cancel, reset, or a local close and never rejects, so every consumer races the same terminal signal. Its close, abort, and serve are the single closer: a protocol violation closes the session with its code once, and the dispatcher no longer closes a request again after handing it off. SUBSCRIBE, FETCH, TRACK_STATUS, SUBSCRIBE_NAMESPACE, SUBSCRIBE_TRACKS and PUBLISH_NAMESPACE move onto it on both sides, replacing the cancelled() and ended() helpers and the inline quic.close and #violated calls. Fixes found on the way, each with a regression test: - A second GOAWAY on a request nobody was reading yet went unnoticed. - A namespace subscription violation closed the session twice. Public API: none. Wire: none. 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 33 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: 6d6a84a06d
ℹ️ 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".
| if (this.#session.version > Version.DRAFT_16) | ||
| this.writer.closed.then( | ||
| () => this.#end(), | ||
| () => this.#end(), | ||
| ); |
There was a problem hiding this comment.
When a peer sends one complete follow-up message and then FINs or resets only its sending direction, #readAhead() parks that message in #ahead, while this code observes only the response writer. Until a consumer calls next() again, ended never settles. In Publisher.#serveSubscribe, only request.ended is watched while broadcast or track resolution may wait forever, so such a peer can prevent cancellation from aborting the lookup and permanently retain a charged subscription slot. Watch reader.closed independently, as the previous cancellation helper did, so stream termination is noticed even behind an untaken message.
AGENTS.md reference: AGENTS.md:L17-L19
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in dd227b3. The first wait now also watches reader.closed apart from the read ahead: a reset ends the request on every draft, and a FIN ends it where FIN cancels, matching the old cancelled() helper. Regression test: request_stream.test.ts "a request ends behind a message nobody took" (draft 18 FIN and reset, draft 21 reset), which hangs without the fix.
(Written by Claude Opus 5.5)
The read ahead parks one untaken message and stops reading, so a peer's FIN or reset after it went unseen until the next take. Watch the reader's end apart from the read ahead, as the cancellation helper this replaced did. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Merge summary:
Follow-up still open from the description: an outbound SUBSCRIBE_NAMESPACE closed locally during its counted initial set may close the session over our own close (predates this PR, unconfirmed). (Written by Claude Opus 5.5) |
Completes the
quest/m1/js-request-ownerquest, a follow-up of moq-dev/moq#5998.Summary
Every IETF request stream in
js/netnow has a single owner:RequestStreaminjs/net/src/ietf/request_stream.ts.next()allows one taker at a time, so no reader can be left detached or read twice.endedsettles when the peer cancels the request: by FIN on drafts that treat FIN as cancel, or by resetting either direction. It also settles when the request closes locally or over a violation. It never rejects, and every consumer races it.close(),abort(err)andserve(task)are the only ways a request ends. A protocol violation closes the session with its own code exactly once, even if the request has already ended. The dispatcher inconnection.tsno longer closes a request again after handing it off, because a second close could only send a competing code.All of these move onto the handle, on both sides:
Removed:
cancelled()andended()helpersquic.close+session.closepairs in the outbound PUBLISH_NAMESPACE path#violatedre-closes in the outbound SUBSCRIBE and SUBSCRIBE_NAMESPACE pathsnextMessage+u16+ skip-body readsHandler entry points keep their
(msg, stream: Stream)signatures, so existing tests drive them unchanged.Line count: the per-request code in publisher, subscriber and connection is about net-flat (-294 / +293). The handle itself adds about 140 lines to
request_stream.ts. In exchange, read and close behavior lives in one place instead of each handler wiring its own.Fixes, each with a regression test that fails without it
publisher.test.ts"a second GOAWAY on an unanswered SUBSCRIBE closes the session", and therequest_stream.test.tscases.subscriber.test.ts"a namespace subscription closes the session once over a violation".malformed()treated a stream reset whose code the browser did not report (source: "stream") as a protocol violation. It now passes it through as a reset. Covered by the existingsession-limits.test.tsSUBSCRIBE cancel case, which now uses that realistic reset shape.Rust
I checked whether the Rust request tasks close twice the same way.
Messages::failedinrs/moq-net/src/ietf/request_stream.rsalready closes the session at the source.is_protocol_violation(publisher.rsSUBSCRIBE_NAMESPACE and the dispatcher,subscriber.rs). That second close uses the code from the same error, so it never competes with the first.serve_bidisalready asserts a single code.Messagesis already the single reader that JS now mirrors: framed reads plus GOAWAY state.So Rust needs no change. Closing at the source is the shape chosen in moq-dev/moq#5998, and this PR moves JS onto it.
Public API and wire impact
Public API: none. Everything is
@internal. Wire: none.Checks
just check: passed (@moq/net2418 pass, 0 fail).bunx tsc --noEmitinjs/net: clean.just test interop --all: not run. There is no wire or FFI change.Follow-ups
(Written by Claude Opus 5.5)
🤖 Generated with Claude Code