Repository navigation
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>
kixelated
changed the title
|
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 34 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
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.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
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) ｜ 𝕏
| `js/net`'s lite-07 createWritable datagram queue test failed only on code | ||
| predating [moq-dev/moq#8195](https://github.com/moq-dev/moq/pull/8195): a write | ||
| that yielded after handing DATAGRAMS_INFO over left the first track's queue | ||
| unopened while the reader went on. The test now reads a datagram through that | ||
| queue before counting queues, fixed by | ||
| [moq-dev/moq#8396](https://github.com/moq-dev/moq/pull/8396). |
There was a problem hiding this comment.
This paragraph resolves the 2026-11-08 entry at line 202 but sits at the end of the ledger with no Added/Resolved <date> lead, so it reads as a continuation of the moq-rs socket entry above it and undated. House style folds a fix into the entry that recorded the failure (see the moq-ffi paragraph at line 216, which names #8329 inside its own Added 2026-11-08 block).
Technical details
# Undated resolution paragraph in the epic ledger
## Affected sites
- `quest/m0/test-flakes-2/README.md:244` — new paragraph; no date/attribution, and separated from `:202` ("Added 2026-11-08 (maintainer): ... lite-07 createWritable datagram queue test, which failed under load") that it corrects.
## Required outcome
- The cause and #8396 reference are attributable to a date and sit next to the 2026-11-08 entry they explain, matching how earlier entries record a fix (e.g. `:216`).
## Suggested approach (optional)
- Append the resolution sentence to the paragraph at `:202`, or give the new paragraph a `Resolved 2026-11-10 (maintainer, /quest-complete)` lead.
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
🤖 Generated with Claude Code
(Written by Claude Opus 5.5)
Review
01a1c2d2-2a4b-75b3-a97b-d9056015b6bb, round 1