Repository navigation
test(net): the lite-07 datagram queue test reads through its first queue - #8396
Conversation
Quest-Claim: 9e88c78d2863f5b3
The only recorded failure ran before #8195, when a write yielded after handing DATAGRAMS_INFO over and the reader could count queues before the publisher opened the first track's. Reading a datagram through that queue proves it is open, so the test no longer leans on the publisher's ordering. 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 23 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. |
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed head: 2774bf5
No actionable findings. The added receive at js/net/src/lite/datagram-tracks.test.ts:138 proves the first queue has opened before the queue-count assertion. The warm-up consumes sequence 0, so the oversized datagram is sequence 1 and the valid 900-byte encoded datagram is sequence 2; the updated assertion preserves that boundary check.
Direction: the event-based test synchronization is appropriately scoped and avoids changing the shared publisher path solely for test ordering.
Limits: GitHub source review of the three-file diff and relevant publisher, datagram, and mock-transport paths. I did not run tests or reproduce the load experiment; GitHub Check was still in progress when inspected.
There was a problem hiding this comment.
ℹ️ No critical issues — one documentation-placement suggestion inline.
Reviewed changes
- Re-enabled the lite-07 datagram queue test:
test.skipbecomestestfor "lite-07 bounds and prioritizes independent createWritable datagram queues". - Queue open is now proven by data: the test appends a 1-byte datagram to
audioafterreader.info()and waits onreader.recv()for sequence 0, so bothsendStreams.datagramsentries are known to exist before the count assertion. The later oversize recv expectation moves from sequence 1 to 2 to account for the new sequence 0. - Quest bookkeeping:
quest/m0/test-flakes-2/datagram-queues.mddeleted, its## Requiredbullet removed, and the cause recorded in the epic README.
The fix direction checks out against the tree: runDatagrams writes DATAGRAMS_INFO (js/net/src/lite/publisher.ts:2213) before #runDatagrams calls datagramWriter.open() (:2279), and the queue is opened before the first track.recv(), so a delivered datagram is a sound proof that the queue exists. open() is the only path that pushes onto the mock's sendStreams.datagrams, and lite-07 skips the legacy carry-both path, so the count stays exactly 2. I also ran bun test js/net/src/lite/datagram-tracks.test.ts --rerun-each 20 at this head: 800 pass, 60 skip, 0 fail.
deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) | 𝕏
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Merged
origin/maininto the branch, bringing in the later 2026-11-09 flake entries while leaving the test change and quest deletion intact. - Dated the resolution paragraph
Resolved 2026-11-09 (maintainer, \/quest-complete`)`, so the ledger reads attributed next to the entries beside it and the prior inline concern is settled.
The delta is documentation only: the test change, the deleted quest, and the removed ## Required bullet are unchanged since the last review, no reference to the deleted slug dangles (only the historical prose at README.md:225), and the Quest check passes on this head.
deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) | 𝕏

Finishes and deletes
quest/m0/test-flakes-2/datagram-queues.md.Summary
js/net/src/lite/datagram-tracks.test.ts's "lite-07 bounds and prioritizes independent createWritable datagram queues".Cause
The only recorded failure was the one moq-dev/moq#8276 reported. Its PR body logged it at 2026-11-08 09:44Z, from a
just checkon commits based on5e4a200dc9. That base predates moq-dev/moq#8195 (merged 08:26Z, and only merged into that branch at 09:43Z). Before #8195,Writer.writehanded the bytes over and then yielded to the shared write budget. So the publisher could send DATAGRAMS_INFO foraudioand stay parked in a host task before#runDatagramsopened its queue. The client meanwhile learned the info, read a datagram onother, and counted one queue.The other citation, "failed in the merge queue dropping moq-dev/moq#8280", was a mix-up. That run (42489349640) failed only the lite-06 session-close-after-end case. None of the 92 readable failed
Check/Queuerun logs from 2026-11-06 to 2026-11-09 contain this test.Not a product bug. The publisher's datagram consumer exists before DATAGRAMS_INFO is written, so datagrams appended before the queue opens are buffered and sent once it opens. Nothing is lost or reordered.
Reproduction
Load enters these tests only through the shared
Budgetyields (js/net/src/util/yield.ts), which depend on host time. To reproduce, I made everyBudget.poll()yield with a given probability (local only, reverted):Expected length: 2, Received length: 1at the queue countmain, 5/20/30/50% yieldsmain, 30% yields, whole file--rerun-each 50main, 10 concurrent fulljs/netsuites x 10 roundsA microtask trace on current
main, over 300 runs with random yields, always showsopen audiobefore the client'sreader.info()resolves. Nothing yields between the DATAGRAMS_INFO hand-off and the queue open. The old test still depended on that ordering without saying so, which is why the new test reads a datagram through each queue instead.Validation
bun testinjs/net: 3183 pass, 5 skip, 0 fail.bunx biome checkon the test file: clean.quest check --base origin/main: 577 documents ok.quest gate nix develop --command just check: fails in two places that this diff does not touch (it changes only the test file and two quest files). Both reproduce when run alone in this checkout:doc:bun test ./.vitepress/api/index.test.tsfails "a re-export from another workspace package links to its owner". On this macOS host the fixture's paths resolve under/private/tmp.python3 sh/version.test.py:test_cut_promotes_only_published_library_dependencies_and_preserves_exemptionsandtest_release_plz_keeps_manual_cut_despite_breaking_commitsfail.Impact
Test and quests only. Public API: none. Wire: none. No
MIGRATE.mdnote.Choices made without asking
#runDatagramspath shared with lite-05/06 without changing anything a peer can observe.Follow-ups
Models
Size: XS
Implementer: Claude Code, claude-opus-5-5
Implementer: Claude Code, claude-opus-5-5, low effort (iterate, XS)
🤖 Generated with Claude Code
(Written by Claude Opus 5.5)
Review
Rewritten by
quest reviewafter each round.01a1c38e-11e7-74a1-9dd0-cf4c63b4c94f, round 1The added receive establishes that the first datagram queue is open before inspection, and the sequence assertion correctly accounts for the extra datagram; source review found no actionable defects, and tests were not run as instructed.