Skip to content

test(net): the lite-07 datagram queue test reads through its first queue - #8396

Merged
kixelated merged 4 commits into
mainfrom
quest/m0/test-flakes-2/datagram-queues
Nov 10, 2026
Merged

kixelated merged 4 commits into
mainfrom
quest/m0/test-flakes-2/datagram-queues

Conversation

@kixelated

@kixelated kixelated commented Nov 10, 2026 •

Copy link
Copy Markdown
Collaborator

Finishes and deletes quest/m0/test-flakes-2/datagram-queues.md.

Summary

  • Re-enables js/net/src/lite/datagram-tracks.test.ts's "lite-07 bounds and prioritizes independent createWritable datagram queues".
  • Before counting send queues, the test now reads a datagram through the first track's queue (sequence 0), so the later oversize check expects sequence 2 instead of 1.
  • Deletes the quest and its link, and records the cause in the test-flakes-2 README.

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 check on commits based on 5e4a200dc9. That base predates moq-dev/moq#8195 (merged 08:26Z, and only merged into that branch at 09:43Z). Before #8195, Writer.write handed the bytes over and then yielded to the shared write budget. So the publisher could send DATAGRAMS_INFO for audio and stay parked in a host task before #runDatagrams opened its queue. The client meanwhile learned the info, read a datagram on other, 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/Queue run 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 Budget yields (js/net/src/util/yield.ts), which depend on host time. To reproduce, I made every Budget.poll() yield with a given probability (local only, reverted):

Code Test Result
pre-#8195 write order, 30% yields old test 73/300 fail, Expected length: 2, Received length: 1 at the queue count
pre-#8195 write order, 30% yields new test 300/300 pass
current main, 5/20/30/50% yields old test 1200/1200 pass
current main, 30% yields, whole file --rerun-each 50 new test 1850/1850 pass
current main, 10 concurrent full js/net suites x 10 rounds old test 100/100 suites pass

A microtask trace on current main, over 300 runs with random yields, always shows open audio before the client's reader.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 test in js/net: 3183 pass, 5 skip, 0 fail.
  • bunx biome check on 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.ts fails "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_exemptions and test_release_plz_keeps_manual_cut_despite_breaking_commits fail.
    • The host disk was nearly full during this run (about 580 MB free).

Impact

Test and quests only. Public API: none. Wire: none. No MIGRATE.md note.

Choices made without asking

  • Changed the test instead of the publisher. Opening the send queue before writing DATAGRAMS_INFO would make the order hold by construction. But it would restructure the #runDatagrams path shared with lite-05/06 without changing anything a peer can observe.

Follow-ups

  • None for this quest.

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 review after each round.

  • Reviewer: Codex, gpt-6.1-sol, low effort
  • Session: 01a1c38e-11e7-74a1-9dd0-cf4c63b4c94f, round 1
  • Head: 4884353
  • Base: origin/main at 2b388b3
  • Score: 10/10, The 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.
  • Findings: none

kixelated and others added 2 commits November 9, 2026 16:01
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 kixelated changed the title js/net's lite-07 datagram queue test holds up under load test(net): the lite-07 datagram queue test reads through its first queue Nov 10, 2026
@kixelated
kixelated marked this pull request as ready for review November 10, 2026 00:20
@coderabbitai

coderabbitai Bot commented Nov 10, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

You'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.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

Run configuration

  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: cf91e461-3843-4eb0-bee6-0a26c60667b2
Commits

Reviewing files that changed from the base of the PR and between ac3cb8c and 4884353.

Files selected for processing: 3

  • js/net/src/lite/datagram-tracks.test.ts
  • quest/m0/test-flakes-2/README.md
  • quest/m0/test-flakes-2/datagram-queues.md
  • 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.

Share

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No critical issues — one documentation-placement suggestion inline.

Reviewed changes

  • Re-enabled the lite-07 datagram queue test: test.skip becomes test for "lite-07 bounds and prioritizes independent createWritable datagram queues".
  • Queue open is now proven by data: the test appends a 1-byte datagram to audio after reader.info() and waits on reader.recv() for sequence 0, so both sendStreams.datagrams entries 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.md deleted, its ## Required bullet 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.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) | 𝕏

Comment thread quest/m0/test-flakes-2/README.md Resolved
kixelated and others added 2 commits November 9, 2026 19:33
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated
kixelated added this pull request to the merge queue Nov 10, 2026

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

  • Merged origin/main into 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.

Pullfrog  | View workflow run | Using deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) | 𝕏

Merged via the queue into main with commit fb0d5a2 Nov 10, 2026
9 of 10 checks passed
@kixelated
kixelated deleted the quest/m0/test-flakes-2/datagram-queues branch November 10, 2026 04:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant