Repository navigation
feat(net): restore IETF datagram tracks - #7959
Conversation
Quest-Claim: 72249e0c9b1fec56
kixelated
changed the title
|
Design change: the maintainer decided on 2026-11-05 that “7959 should make one upstream IETF subscription like 05/06”. Replace the separate upstream datagram request with shared stream/datagram interest in the IETF adapter, preserving full relay support, aggregate stream demand, and delivery by requested kind. The scope choice is settled and its Waiting item is removed. (Written by GPT-6.1 Sol with high effort) |
|
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 14 seconds. 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 11bc10a3076d6b602428bbe2bb82d42c1a29fd0f against base 62ae716265b59681faa82b7f433d91a2f2e5f2e6. No prior published review to supersede.
Four findings:
-
[P2] Share the legacy datagram writer across subscriptions. JS publisher:1177–1182 calls
datagramWriter()separately for every subscription. Its legacydatagrams.writablefallback callsgetWriter(), so the first live track holds the exclusive lock and the second throws. A datagram-only second track fails; a mixed track silently loses its datagrams. Acquire the helper once per publisher/session, as the lite publisher does, and test two simultaneous names with the writable-only transport. -
[P2] Reattach relay datagrams after resume resolves. JS publisher:579–583 looks up the passive datagram consumer before awaiting the new upstream response. After a pause has released the last upstream subscriber, that producer is gone. The response creates its replacement, but this path omits the initial setup's
datagrams ??= datagramOf(). The resumed request acknowledges successfully and forwards streams without datagrams. Reacquire after resolution and cover pause/resume through a relay after upstream teardown. -
[P2] Keep cancellation active while waiting for datagram metadata. Rust publisher:1491–1499 stops polling
SubscribeTrackonce the stream lookup fails. With no stream track and an application holding an unanswered datagram request, the remaining metadata wait never checks request withdrawal; the outer guard checks authorization only. Canceling/resetting the request leaves demand and request credit held until the handler answers or authority ends. Race this wait against withdrawal even after stream failure, and test cancellation before datagram acceptance. -
[P2] Apply the supported Location Filter to outgoing datagrams. Rust publisher:6882–6894 emits every packet without checking the subscription range; JS
#sendDatagramslikewise receives no range. A draft-22 absolute filter starting at group 10 still forwards newly produced datagrams numbered 0–9, including after narrowing updates. Preserve the requested bounds in datagram-only setup and check each(sequence, 0)against the current filter before sending. Cover both initial and updated filters.
Direction: keep the shared-upstream design, but fix these delivery and cancellation cases before merging.
Verification limits: GitHub-only static review of the diff, surrounding code, and tests. No builds or tests executed; the latest Check workflow is still in progress. The PR's reported local validation and baseline failures were not independently reproduced.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The JS relay path drops its datagram half when a paused subscription resumes. One other rough edge in the new send loop is inline too.
Reviewed changes
- One upstream IETF SUBSCRIBE per name. Rust
Serving/TrackServeand JS#forwardattach the datagram half to the stream subscription instead of opening a second request, answering from whichever kind resolves, and synthesizing a metadata stream track when only datagrams exist. - Passive datagram forwarding.
datagram::Consumerpriorities becomeOption<i8>(Rust) /number | undefined(JS), so a relay observes the replicated datagram track without adding phantom subscriber priority; lite-05/06 forwarding gets the samehas_local_datagrams/localDatagramsgate. - Datagrams on the receive path.
recv_datagram/#recvDatagramroute by alias to the subscription's datagram producer, drop nonzero object IDs, status objects, unreadable properties and (on timed tracks) missing timestamps, and refuse undefined Object Status. - Demand lifecycle.
ietf/subscriber/datagrams.rs(Requested,Shared) keeps the initial stream interest until the answer accepts the datagram request, then holds stream demand only while a datagram reader has a priority; pause releases it. - Coverage and docs. moq-crypto
datagrams_over_ietf, delivery benches, Rust/JS interop taildatagramlanes,MIGRATE.mdentries,standard.md, and the quest tree edit.
One validation note carried from the body: the restored JS interop tail lanes and the end-to-end matrix did not run in this environment, so they land unexercised.
deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Shared JS datagram writer. The IETF
Publishernow acquires oneDatagramStream.datagramWriterper session and releases it inclose(), so two concurrent subscriptions both forward on the legacydatagrams.writableshape; each sender races itsready/write/closeagainst the consumer's close so a pause is answered even while a transport write is blocked. - Relay resume restored.
resolve()is reused on resume and the relay re-looks-updatagramOf()after the replacement upstream subscription answers, matching the initial serve and Rust's post-subscribedatagrams_livelookup. - Datagrams received with their answer or end. Rust buffers an unknown-alias datagram for one budget in
ietf/session.rs::Datagramsand re-routes it after the control tasks poll, then gates a clean end throughState.datagram_endsuntil the driver reads the datagram queue empty (caught_up, also drained on session shutdown). JS mirrors it with a tracked receive yield plus oneyieldNow()retry for an unknown alias, andPUBLISH_DONEawaits#datagramsRouted()before finishing the producer. - Publisher poll order. Rust splits
TrackServe::pollintopoll_datagrams/poll_groups, so a bounded stream range running out no longer stops datagram forwarding, and a datagram-only name still polls its sender first. - Clean end for a withdrawn resume. A resumed IETF request whose subscribe was withdrawn now ends with
PUBLISH_DONE(Served::Track(Ok(())), opened stream count) instead ofWithdrawn, without holding its demand. - Coverage and docs. New tests cover the shared/legacy writer, a pause leaving a blocked write, resume through a relay, a burst arriving with its end, and the IETF answer-race lanes; the interop tails move to
origin.route(..., hidden), and the quest tree and migration notes follow the shared-subscription decision.
Both prior pullfrog threads are fixed and resolved by the author. One carried note for a later run: the human review's Location Filter item (outgoing OBJECT_DATAGRAM ignores the subscription range) predates this delta and remains open.
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed head 2f35155c92fcbdd94aed777e78fd99a93d16a0a6 against base 1a6ac473d75fcbfd5d23bdbdfd64eb4750b077ad, including changes since the previous review at 11bc10a3076d6b602428bbe2bb82d42c1a29fd0f.
Prior findings:
- #1 and #2 fixed. The session-wide JS datagram writer and post-resolution relay lookup address both failures, with regression coverage for concurrent subscriptions, blocked writes, and idle relay resume.
- #4 withdrawn. The documented datagram design intentionally uses independent, live-only sequences without bounded ranges. My prior review incorrectly treated stream Location Filters as a datagram requirement; that item should not remain an open defect.
- #3 remains:
[P2] Keep cancellation active while waiting for datagram metadata. Rust publisher:1500–1507 bypasses SubscribeTrack::poll once the stream lookup fails. With no stream track and an application holding an unanswered datagram request, the fallback only waits on datagrams.poll_info; the outer guard checks authorization, not request withdrawal. Canceling/resetting that request therefore leaves its demand and request credit held until the handler answers or authority ends. Poll withdrawal independently of stream selection while metadata is pending, and add a regression that cancels before datagram acceptance.
No additional concrete findings from the receive-ordering/end-drain, mixed-range progress, or resumed PUBLISH_DONE changes.
Direction: keep the shared-upstream design and fix the pending-request cancellation case before merging.
Verification limits: GitHub-only static review of changes, surrounding code, and tests. No builds or tests executed. Check workflows remain queued/in progress; the author's reported local results and environment failures were not independently reproduced.
Keeps main's passive legacy join (#7959) and makes the datagram hold a passive observer too, so it keeps a position without adding demand: a relayed SUBSCRIBE that adopts it stays passive as main intends. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Problem
moq-transport stopped delivering datagrams after stream and datagram tracks split. A separate upstream request for each kind also changed aggregate stream demand through relays.
Approach
Share one upstream IETF SUBSCRIBE per name across stream and datagram consumers, as on lite-05/06. Reuse the stream subscription machinery and privately attach its datagram half before publishing the answer. Passive relay forwarding adds no stream demand; actual datagram readers hold and update the shared subscription. Each model consumer receives only its requested kind.
Resume relay datagrams after an idle upstream subscription and share the legacy WebTransport datagram writer across subscriptions. A paused subscription releases its send task even when its write is blocked.
Preserve main’s answer/end routing work: JS waits one host task before dropping an unknown IETF alias and drains the datagram receive loop before a clean end. The receive slot bounds what it holds. Rust receives in two passes around its source machines, holds unknown aliases for at most one receive budget, and retains the alias and request stream until datagrams received with a clean end are routed. Mixed subscriptions keep forwarding datagrams when their stream range is exhausted, and withdrawal during resume preserves PUBLISH_DONE.
Restore encrypted IETF datagram transport coverage, the delivery benchmark rows, and Rust/JS interop tail lanes. Preserve the advertised timescale across mixed-draft relays. Reject undefined OBJECT_DATAGRAM status on both JS receive paths.
Design decision
Maintainer, 2026-11-05: “7959 should make one upstream IETF subscription like 05/06”. This replaces the separate-subscription implementation. Full relay support remains in scope. The resolved Waiting item is removed, and the completed quest is deleted; the epic records the decision.
Impact
Alternatives
Separate upstream requests were rejected by the maintainer. Matching missing-track errors was removed: local publishers select a successful stream or datagram answer, and relays reuse the shared stream answer and datagram producer.
Validation
All commands run in the repository's Nix dev shell.
quest gate just ci: terminated with exit 143 after its JS lane failed. All 10,958 Rust tests passed (136 skipped), Rust checks and the formatting/documentation/workflow/quest checks passed. The tooling test lane was stopped whilebench/build.test.pywaited on another worktree’s live host benchmark lock; that lane is incomplete, not passing. The run was admitted and executing, not waiting for quest gate admission. No local process from these checks remains running. JS workspace checking fails because the local Codex BASH_ENV hook restores PATH without Bun’s workspace binaries (tsc: command not found, exit 127). Superseded runs passed Rust checks, all 10,954 Rust tests (136 skipped), workspace JS checks/tests, formatting, quest validation, Markdown, shell, text, TOML, Nix, workflow, and justfile checks. They were stopped when source or main advanced; their tooling test lane was waiting on a live benchmark in another worktree. None is claimed as a complete CI pass.quest gate just test interop --allexited 1: the same shell-hook PATH loss prevents the three JS clients from building (tsc: command not found), so their matrix and tail lanes are unavailable. All ten wire/crypto checks passed, including IETF datagrams; available non-JS matrix, finished-media, Rust FETCH/tail and Python/Rust socket checks passed. The earlier run atf340141b3cpassed the full matrix, including IETF datagrams and restored Rust/JS tail lanes (214 PASS records, including ten wire/crypto checks); that pass does not validate the later receive-ordering changes.bun test js/net/src/ietf/publisher.test.ts js/net/src/ietf/datagram.test.ts: 146 passed, two existing skips. Covers relay resume opening exactly one upstream request, concurrent legacy/modern writers, a blocked write during pause, immediate/delayed answers and ends on all IETF drafts, and a queued burst crossing receive yields.quest gate cargo test -p moq-net --all-features --test integration datagram::: 22 passed.quest gate cargo test -p moq-net --all-features --lib ietf::: 849 passed, two existing ignores.bunx tsc --noEmit --project js/net/tsconfig.jsonand Biome: passed.nix develop -c bun run --cwd test/audio-quality/clients/js checkexits 127 on both heads when the base checkout uses this checkout’s unchanged.direnv/codex-env.shsnapshot. Without that local snapshot, unchanged main’squest gate just js checkpasses. No harness environment was disabled to get a pass. Open PRs were searched; no fix was identified.WritableStream is locked; the answer/end reproducer initially failed all 12 cases.Review
Claude Code,
claude-opus-5-5, medium effort, local round 1 reviewed headf744dc228a78384a7c6beced79c4a904d1af3df5against origin/main and reported eight findings.claude-opus-5-5, medium effort, local round 2 reviewed committed head2f35155c92fcbdd94aed777e78fd99a93d16a0a6against main1a6ac473d75fcbfd5d23bdbdfd64eb4750b077ad: no findings. All first-round dispositions were accepted.Follow-ups
Isolate the synthetic
bench/build.test.pyfixture from the host-wide performance benchmark lock. This tooling issue delayed local checks behind another worktree's live benchmark; it is outside this datagram PR. Repair the Codex shell hook’s PATH preservation separately. No maintainer design decision is open.(Written by GPT-6.1 Sol with high effort)