Repository navigation
test(net): pin END_OF_GROUP decode on both header shapes - #5378
Conversation
…p end The End-of-Group header bit and an explicit END_OF_GROUP object (status 0x03) are two independent ways to state where a group ends, and draft-20 makes neither conditional on the other. A publisher may legally use both: the bit says this stream carries the group to its end, the object says which object that end is at. Frame.decode accepted status 0x03 only on the branch where the header did NOT mark the end. The combination matched no branch and fell through to `Unsupported object status: 3`, which propagates out of handleGroup's read loop; the catch then closes the group with that error. So a publisher that stated the end twice -- correctly, both times -- lost the whole group, including the objects already delivered on that stream, not just the one redundant marker. Accept GROUP_END inside the hasEnd branch, returning the same payload-less Frame the other branch already returns: the subscriber breaks its read loop on `payload === undefined`. This stays distinct from the `status === 0` case, which is an empty frame rather than a group end; the two are different objects and are not merged. Tested at both levels, because they make different claims. The codec test pins both header shapes plus the status-0 case. The subscriber test pins that the group closes cleanly rather than merely stopping -- a torn-down group also stops producing frames, so asserting "nothing more arrives" would pass on the bug. Both go red with the fix reverted.
Main already accepts END_OF_GROUP on a header-marked stream via #5081, so keep main's decoder and subscriber test, and keep the codec-level test from this PR. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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. |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 08fdb8b
No actionable bugs found. The focused codec regression test in js/net/src/ietf/ietf.test.ts:1766–1797 is the right direction: it exercises END_OF_GROUP with both hasEnd values and distinguishes a normal empty payload from the terminal marker, complementing the subscriber test from #5081 without changing runtime behavior.
Verification: inspected the complete PR diff, Frame.decode, the buffered Reader path, and the subscriber's terminal handling. Static GitHub-only review; tests and builds were not run. Rechecked the open/non-draft state, head, and existing reviews immediately before posting.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughAdds draft 20 frame-decoding regression tests. The tests check the payload and track-ending results for statuses 0x03 and 0x00 under the specified group-header end-bit conditions. Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The added tests cover the stated header and status combinations; no merge-blocking concern is identified. Pre-merge checks ·
 |
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly and concisely describes the codec test that covers END_OF_GROUP decoding with both header shapes. |
| Description check | |
The description directly explains the END_OF_GROUP decoding behavior, the codec-level test coverage, and the test-only impact. |
| Docstring Coverage | |
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1… |
| 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. |
✨ 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.
Problem
An END_OF_GROUP (0x03) status and the End-of-Group header bit are independent, so a publisher may send both. #5081 already fixed the decoder to accept that combination and added a subscriber-level test, but nothing pins it at the codec level.
Approach
Carries the codec test from #5368 by @kkroo, whose commit is preserved on this branch.
Frame.decodemust return a payload-less frame for 0x03 with or without the header bit, and status 0 on a marked stream must stay an empty frame. The decoder change and subscriber test from #5368 are dropped in favor of main's from #5081.Replaces #5368, which could not be updated because its head is on an organization-owned fork. Thanks to @kkroo for the report and test.
Impact
Alternatives
None.
Follow-ups
None.
(Written by Claude Opus 5.5)
Co-authored-by: Omar Ramadan 1845185+kkroo@users.noreply.github.com
Need help on this PR? Tag
@codesmith-botwith what you need.