Repository navigation
quest(wildcard): block the line on the #4279 datagram, origin, and benchmark findings #4386
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| # [M] Datagrams behind SUBSCRIBE_START | ||
|
|
||
| ## Goal | ||
|
|
||
| On lite-07, no datagram crosses a subscription's SUBSCRIBE_START in either | ||
| direction, in Rust or JS: a publisher commits the start only for something it | ||
| actually sends, never sends a group below the start it announced, and a | ||
| subscriber delivers a datagram only after that subscription's own | ||
| SUBSCRIBE_START has named an admitted origin. | ||
|
|
||
| ## Plan | ||
|
|
||
| [#4279](https://github.com/moq-dev/moq/pull/4279) made SUBSCRIBE_START | ||
| (SUBSCRIBE_OK) carry the serving origin and go out ahead of the first group | ||
| served by stream or datagram. Its last Codex round left three P1s unanswered, | ||
| and the PR auto-merged before CI. The maintainer ruled in the 09-28 | ||
| merged-PR audit that all three block the line: | ||
|
|
||
| - **An oversized first datagram commits the start** | ||
| ([r4117485530](https://github.com/moq-dev/moq/pull/4279#discussion_r4117485530)). | ||
| Rust's `Recv::Datagram` arm in `rs/moq-net/src/lite/publisher.rs` calls | ||
| `send_start` before `serve_datagram` drops a body over `max_datagram_size`, and | ||
| JS `#runDatagrams` in `js/net/src/lite/publisher.ts` awaits | ||
| `responses.start` before its size check. A dropped datagram at sequence 10 | ||
| can so announce a start of 10 and discard a valid group at 5. Decide | ||
| whether a datagram can be sent before resolving the start from it. | ||
| Since main's held first group (`TrackRun::first`), a group waits for the | ||
| source's start (`track.poll_start`) while datagrams keep flowing with no | ||
| START, and a datagram that goes first still resolves the start from its | ||
| own sequence without consulting the source. Both paths should resolve the | ||
| start the same way. | ||
| - **JS start-sequence race** | ||
| ([r4117485532](https://github.com/moq-dev/moq/pull/4279#discussion_r4117485532)). | ||
| `SubscribeResponses.start` reserves `#started` for whichever loop arrives | ||
| first, but the write runs later through the `#writes` chain, so the group | ||
| loop can pop a lower sequence in between and send group 5 after a START | ||
| for 10. Choose the start and apply its floor atomically, or revalidate a | ||
| group popped while the start was pending. | ||
| - **FETCH_OK admits a datagram before its own SUBSCRIBE_START** | ||
| ([r4117485533](https://github.com/moq-dev/moq/pull/4279#discussion_r4117485533)). | ||
| `route_datagram` in `rs/moq-net/src/lite/subscriber.rs` gates only on the | ||
| shared track `Provenance` being admitted. A FETCH on the same track can | ||
| admit it through FETCH_OK while this subscription has not started, so a | ||
| racing datagram is delivered, and a later START naming another origin is | ||
| caught only after content was exposed. Require this subscription's own | ||
| start as well. Check whether the JS subscriber has the same gap. | ||
|
|
||
| Each fix gets a regression test that fails without it, in the language it | ||
| touches. | ||
|
|
||
| ## Related | ||
|
|
||
| - [Wildcard](/quest/m0/wildcard/README.md) - the line this blocks |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| # [S] JS origin granularity | ||
|
|
||
| ## Goal | ||
|
|
||
| `@moq/net` tracks the origin an upstream reply names at the same granularity | ||
| as Rust, and handles a second, different origin the way Rust does, so a JS | ||
| node that republishes never labels one origin's content as another's. | ||
|
|
||
| ## Plan | ||
|
|
||
| [#4279](https://github.com/moq-dev/moq/pull/4279) had JS record the origin | ||
| an upstream SUBSCRIBE_OK or FETCH_OK names on the consumed broadcast's shared | ||
| state (`js/net/src/broadcast.ts`), and the latest reply wins. Rust records it | ||
| per copy of a track (`track::Provenance`) and only admits a replacement | ||
| naming the same origin. Codex asked for a conflicting name to be refused | ||
| ([r4112837960](https://github.com/moq-dev/moq/pull/4279#discussion_r4112837960)). | ||
| The agent declined, since a later request for the same broadcast can | ||
| legitimately land on a front serving another origin, and left the | ||
| granularity question to the maintainer | ||
| ([r4113954489](https://github.com/moq-dev/moq/pull/4279#discussion_r4113954489)). | ||
| The maintainer decided in the 09-28 merged-PR audit that JS must match Rust. | ||
|
|
||
| - Move the origin to the level Rust keeps it at, so two tracks of one | ||
| broadcast served by different origins stay distinct. | ||
| - A reply naming a different origin for content already labeled follows | ||
| Rust's rule rather than overwriting silently. | ||
| - A republished broadcast advertises, per track, the origin that actually | ||
| served it, or a random one when nothing upstream named one. | ||
|
|
||
| Test the mid-life origin change JS previously absorbed, and pin that JS and | ||
| Rust agree on it in the interop suite if the scenario is reachable there. | ||
|
|
||
| ## Related | ||
|
|
||
| - [Wildcard](/quest/m0/wildcard/README.md) - the line this blocks | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| # [S] Pool resolution benchmark | ||
|
|
||
| ## Goal | ||
|
|
||
| A benchmark sweeps pool size against requested-path count for route | ||
| resolution, so a cost that grows with the pool or the path set instead of the | ||
| touched path shows up as a slope. | ||
|
|
||
| ## Plan | ||
|
|
||
| [#4279](https://github.com/moq-dev/moq/pull/4279) keyed `route_order`'s tie | ||
| break on a hash of the requested path, so resolving a path now scans the | ||
| equal-cost pool and hashes the path with every candidate's hops | ||
| (`rs/moq-net/src/model/origin.rs`). Only a correctness test | ||
| (`equal_cost_pool_spreads_paths`, 4 members by 64 paths) covers it. Codex | ||
| asked for the two-axis sweep AGENTS.md requires for fan-out | ||
| ([r4117485535](https://github.com/moq-dev/moq/pull/4279#discussion_r4117485535)), | ||
| and the PR listed it as a known gap. The maintainer ruled in the 09-28 | ||
| merged-PR audit that it blocks the line. | ||
|
|
||
| Add it beside the existing origin benchmarks in `rs/moq-net/benches/`. If the | ||
| slope shows resolution scaling with the pool in a way that matters, say so in | ||
| the PR rather than optimizing here. The PR's other known gap, tracks per | ||
| front for the driver's per-event admission walk, is worth sweeping in the | ||
| same change if it is cheap. | ||
|
|
||
| ## Related | ||
|
|
||
| - [Wildcard](/quest/m0/wildcard/README.md) - the line this blocks |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When two tracks of one broadcast name different origins, do not preserve both as distinct per-track identities. Rust's
Front::vouchestablishes one sharedFront::named,Front::origin_namedrejects a later conflicting origin, andrun_frontpublishes that single identity throughbroadcast.set_origin; the per-trackProvenanceonly gates each copy against it. Following this plan would diverge from Rust and let one broadcast name represent two content identities, so revise the quest to keep the JS origin sticky at broadcast/front granularity and reject conflicting replies.AGENTS.md reference: AGENTS.md:L62-L64
Useful? React with 👍 / 👎.