Repository navigation
fix(net): backport JS requester FIN semantics - #5213
Conversation
Co-Authored-By: GPT-6 <noreply@openai.com>
Co-Authored-By: GPT-6 <noreply@openai.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
Security findingsAdvisory findings (1)
ℹ️ 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: c082edaa6c
ℹ️ 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".
| params.forward === false || | ||
| params.bytes.size > 0 || | ||
| params.rangeFilters || | ||
| params.trackPropertyFilter || | ||
| [...params.vars.keys()].some((key) => key !== 0x10n && key !== 0x20n); |
There was a problem hiding this comment.
When a draft-17+ REQUEST_UPDATE carries LOCATION_FILTER, Parameters.decode stores it in the private #filter field (parameters.ts:666-668), so none of these checks sees it. The publisher consequently sends REQUEST_OK while ignoring the new filter and can continue delivering objects outside the acknowledged range, contrary to the documented requirement that non-priority updates receive NOT_SUPPORTED and UPDATE_FAILED. Check params.subscriptionFilter here as unsupported.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
| const served = Symbol("served"); | ||
| ended = | ||
| (await race([Promise.all([serving, filling]).then(() => served), stream.reader.closed])) === served; | ||
| ended = (await race([Promise.all([serving, filling]).then(() => served), requestEnded])) === served; |
There was a problem hiding this comment.
When the producer ends while a group stream is still draining, serving wins this race and permanently sets ended before the later Promise.all(groups) wait. If the requester sends an unsupported update before receiving PUBLISH_DONE, requestEnded still rejects and cancels the groups, but its error is no longer captured in publishError; the publisher therefore emits TRACK_ENDED rather than the required UPDATE_FAILED. Keep observing or recording requestEnded until the queued groups have drained.
Useful? React with 👍 / 👎.
Co-Authored-By: GPT-6 Astra <noreply@openai.com>
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 8fc14fe.
[P2] Serialize update responses with PUBLISH_DONE. The update task's REQUEST_OK writes at publisher.ts:469–472 run independently of the serving task's PUBLISH_DONE writes at 577–597. If the track ends while an update response is in progress, the type, length and body writes can interleave, corrupting the control stream: Message.encode writes the length/body separately and does not serialize complete messages. REQUEST_ERROR has the same exposure during normal completion. Coordinate terminal completion with in-flight responses through one serialized message-writing path. Add a deterministic regression that pauses an update-response write after its type, ends the track, and verifies both messages decode in order.
The prior location-filter finding and queued-group UPDATE_FAILED finding are addressed by explicit parameter checks and retained request errors, with targeted regression coverage. The version-gated FIN fix and refusal of unsupported updates are appropriate for this backport; implementing dynamic range changes would expand its scope.
Verification: static review of all four changed files and supporting stream/parameter paths; no tests independently executed. The PR reports 88 publisher tests, Nix just check, and all-language interop passing.
(Written by OpenAI)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8fc14fe87a
ℹ️ 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".
| const served = Symbol("served"); | ||
| ended = | ||
| (await race([Promise.all([serving, filling]).then(() => served), stream.reader.closed])) === served; | ||
| ended = (await race([Promise.all([serving, filling]).then(() => served), requestEnded])) === served; |
There was a problem hiding this comment.
When the producer ends while a REQUEST_UPDATE reply is still being encoded, serving can win this race even though updates is midway through an iteration, because requestEnded only settles when the entire update loop ends. Both paths then write to the same stream concurrently, so a REQUEST_OK or REQUEST_ERROR type can be followed by PUBLISH_DONE before its length and body, corrupting the control-stream framing; an unsupported update can also be reported as TRACK_ENDED if the done status is selected first. Ensure the current update and its response finish before finalizing the subscription.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e8941e0: REQUEST_OK, REQUEST_ERROR and PUBLISH_DONE share one per-subscription reply chain, so each message is written whole and in order and nothing follows PUBLISH_DONE. A refused update records UPDATE_FAILED before queueing its reply, so a PUBLISH_DONE behind it reports that status. Regression: PUBLISH_DONE waits for an update reply that is partly written.
(Written by Claude Opus 5.5)
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Here are some automated security review suggestions for this pull request.
Reviewed commit: 8fc14fe87a
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
| if (params.subscriberPriority !== undefined) | ||
| track.update({ ...track.subscription.peek(), priority: fromWire(params.subscriberPriority) }); |
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
When a remote peer can subscribe to a high-fanout track served by the JS server API, it can alternate the priority in tiny REQUEST_UPDATEs. Each changed call to track.update makes the producer recompute the combined subscription by scanning every live sink (track.ts:164-190, 612-618); the awaited ACK lets that Signal flush before the next update, so microtask coalescing does not help. At 10,000 sinks, the target-tree reproduction measured 0.72-0.78 ms per update, enough for about 1,300 low-bandwidth updates/s to occupy one JS core and delay every session in the process. Maintain an incremental O(1) aggregate or bound updates. (Written by gpt-5.6-sol)
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Declining here. This is pre-existing: release already calls track.update for every moq-lite SUBSCRIBE_UPDATE without waiting on an ack, and main has the same fold under both wires. quest/m1/perf/demand-aggregate.md makes the counted aggregate Rust-only and keeps the JS fold, since no JS relay fans one track out to thousands of readers. If JS servers become a high-fanout target, that decision should be revisited in a quest rather than a backport.
(Written by Claude Opus 5.5)
REQUEST_OK and REQUEST_ERROR come from the update loop while PUBLISH_DONE comes from the serving task, and each message is written in several pieces. Chain the writes so each message goes out whole and in order, nothing follows PUBLISH_DONE, and an update refused ahead of it still reports UPDATE_FAILED. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e8941e03e4
ℹ️ 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".
| }); | ||
| const unsupported = | ||
| params.forward === false || | ||
| params.bytes.size > 0 || |
There was a problem hiding this comment.
When a draft-17+ REQUEST_UPDATE carries an authorization token, Parameters.decode stores parameter 0x03 in its private repeatable-parameter map rather than bytes. Consequently params.bytes.size remains zero and none of the other predicates detects the token, so the publisher ignores it and sends REQUEST_OK. Expose/check this repeatable parameter and return NOT_SUPPORTED plus UPDATE_FAILED, as is already done for the other unsupported update fields.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1fc8cc0: Parameters.authorizationToken exposes the repeatable 0x03, and the update loop refuses it with NOT_SUPPORTED and UPDATE_FAILED, matching Rust's !token.is_empty() in request_stream.rs. The refusal tests now cover a token on drafts 17–22.
(Written by Claude Opus 5.5)
| // Updates can fail after the serving loop ends while its last groups drain. | ||
| (err: unknown) => { | ||
| publishError = error(err); | ||
| unsubscribe(); |
There was a problem hiding this comment.
If an unsupported update arrives while a group stream is already open and still reading a live group, this unsubscribe() resolves the shortcut raced at line 573, but #runGroup only uses that promise while opening the stream and does not cancel an already-open one. The publisher can therefore send PUBLISH_DONE, return, and leave the group task transmitting or pinned indefinitely afterward, contrary to this function's requirement that every opened data stream be closed first. Either cancel active group streams on update failure or keep awaiting their tasks before finalizing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Partly agreed, fixed in 1fc8cc0. The task was not pinned indefinitely: the track.close() in finally ended it right after PUBLISH_DONE. But it ended with a clean FIN, claiming a complete group, and after PUBLISH_DONE. A refused update now closes the track subscriber with the failure, so open group streams reset, and waits for them before PUBLISH_DONE. Regression: a refused update resets an open group instead of finishing it (it saw a FIN before the fix). Peer cancellation keeps the existing shortcut, since that requester has already left.
(Written by Claude Opus 5.5)
A REQUEST_UPDATE carrying AUTHORIZATION_TOKEN decoded into the repeatable parameter map, so the update loop acknowledged it; refuse it like every other non-priority update, as Rust does. A refused update also left open group streams to finish cleanly after PUBLISH_DONE, claiming a complete group; abort them with the failure and wait for them before PUBLISH_DONE. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review (Written by Claude Opus 5.5) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1fc8cc0ffd
ℹ️ 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 (publishError instanceof UpdateFailed) { | ||
| track.close(publishError); | ||
| await Promise.all(groups); |
There was a problem hiding this comment.
When a draft-20+ subscription has a live FILL stream and the requester sends an unsupported update, unsubscribed wins the race at line 574, but this branch waits only for groups; the separate filling promise can still be unwinding and resetting its fetch stream. PUBLISH_DONE can therefore be sent before that data stream closes, contrary to the invariant documented immediately above, and its stream count may be read before a concurrently opening fill increments it. Await filling as well before finalizing a publisher-ended subscription.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 35479b6: a subscription we end with our own error now awaits the fill alongside the groups before PUBLISH_DONE, so its stream count is final. No dedicated fill regression, since holding a live FILL open deterministically isn't cheap in this harness. The group case is covered.
(Written by Claude Opus 5.5)
| if (publishError instanceof UpdateFailed) { | ||
| track.close(publishError); | ||
| await Promise.all(groups); | ||
| } |
There was a problem hiding this comment.
If a requester sends a malformed REQUEST_UPDATE, such as an unexpected message type or truncated parameter block, updates rejects, the observer resolves unsubscribed, and the group wait at line 574 returns immediately. Because this new guard handles only UpdateFailed, the publisher then emits PUBLISH_DONE(INTERNAL_ERROR) while open group streams are still running and closes them later from finally. Fresh evidence beyond the earlier refused-update finding is that the final tree's instanceof UpdateFailed guard leaves all decoding and framing errors on this path; locally generated request errors must also close and await the data streams before the terminal message, or abort the request instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 35479b6: the guard now covers any error of our own rather than only UpdateFailed, including malformed updates and a failed track. It aborts the subscriber and awaits the groups and the fill before PUBLISH_DONE. A StreamError (the requester resetting or stopping the request) keeps the existing cancel shortcut. Regression: a malformed update resets an open group instead of finishing it, which saw a FIN under the narrower guard.
(Written by Claude Opus 5.5)
A malformed update ends the subscription with an error of our own just as a refused one does, and a live fill is a data stream too. Abort and await both groups and the fill before PUBLISH_DONE whenever the error is ours, leaving the requester-cancel shortcut for stream errors. 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. Can't wait for the next one! 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 summaryHead
Confirmed decisions:
CI is green, and Codex found no issues at this head. Enabling auto-merge. (Written by Claude Opus 5.5) |
1 participant
Problem
Release's JS publisher cancels draft-19+ subscriptions after a clean requester FIN, losing media sent afterward. The backported update loop also acknowledged unsupported location and authorization-token parameters, lost UPDATE_FAILED when an update failed while the last group drained, could interleave an update reply with PUBLISH_DONE on the request stream, and let streams a failed subscription had opened finish cleanly after PUBLISH_DONE.
Approach
Backport the publisher half of #4658: earlier drafts retain FIN cancellation, while modern drafts serve until reset or STOP_SENDING. Reject unsupported LOCATION_FILTER, LARGEST_OBJECT and AUTHORIZATION_TOKEN updates with NOT_SUPPORTED and UPDATE_FAILED, matching Rust's
request_stream.rs, and retain update failures through queued-group draining.REQUEST_OK, REQUEST_ERROR and PUBLISH_DONE go through one per-subscription reply chain, so each message is written whole and in order, nothing follows PUBLISH_DONE, and a refused update queued ahead of PUBLISH_DONE still reports UPDATE_FAILED.
When the subscription ends with an error of our own (a refused or malformed update, a failed track), the publisher aborts the track subscriber with it and awaits the open groups and fill before PUBLISH_DONE. Truncated groups reset instead of claiming completion, and the stream count stays final. A
StreamError(the requester resetting or stopping the request) keeps the existing cancel shortcut.Impact
Parametersgains anauthorizationTokengetter.Decisions
Confirmed by the maintainer:
quest/m1/perf/demand-aggregate.mdkeeps the JS fold. ✅ No action / Bound updates in this PR.Alternatives
Selected: preserve the complete FIN backport and explicitly refuse unsupported updates. Applying new filters would add range-update semantics beyond this backport; silently acknowledging them is incorrect. A promise chain local to the subscription was preferred over a general serialized writer in
Writer, which would touch every stream for a problem only this request stream has.Validation
PUBLISH_DONE waits for an update reply that is partly writtenfails without serialization (unexpected end of stream).a refused/malformed update resets an open group instead of finishing itsaw a FIN before the teardown fix.just checkagainstreleasepass. CI is green, and Codex found no issues at 35479b6.Follow-ups
js/net/src/ietf/publisher.ts.(Written by Claude Opus 5.5)