Repository navigation
quest: import #5521, #5563, #5564 and #5565 #5572
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
Show all changes
3 commits
Select commit
Hold shift + click to select a range
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 @@ | ||
| # [S] A debug moq-relay starts on Windows | ||
|
|
||
| ## Goal | ||
|
|
||
| A debug (`dev` profile) `moq-relay` starts on Windows, where the main thread | ||
| has a 1 MiB stack, instead of dying with `STATUS_STACK_OVERFLOW` before it | ||
| logs anything. Embedders that call `Relay::load(..).await` and `.run().await` | ||
| from their own Windows main thread are covered too, not only the binary. | ||
|
|
||
| ## Plan | ||
|
|
||
| Reported in [#5563](https://github.com/moq-dev/moq/issues/5563): `cargo run | ||
| --bin moq-relay -- prod.toml` in PowerShell or the MinGW console overflows the | ||
| main thread, with no output even at `RUST_LOG=trace`. It works under WSL, | ||
| which has an 8 MiB main thread. | ||
|
|
||
| Likely cause, not yet confirmed: `rs/moq-relay/src/main.rs` is one | ||
| `#[tokio::main]` block that inlines `Relay::load`'s future, the `Relay`, and | ||
| `Relay::run`'s future into a single unboxed state machine. `block_on` builds | ||
| and polls that state machine on the main thread, and debug builds copy it by | ||
| value several times. Logging starts inside `Config::load`, which is why | ||
| nothing prints. The relay tests already sidestep this with 32 MiB threads | ||
| (`rs/moq-relay/tests/goaway_cluster.rs` and others), and | ||
| `rs/moq-relay/src/connection.rs` and `rs/moq-tokio/src/client.rs` already box | ||
| large futures for the same reason. | ||
|
|
||
| Decided 2026-10-15 (maintainer): | ||
|
|
||
| - Fix at the source by boxing the large futures inside the library: | ||
| `Relay::load` and `Relay::run` box their large inner futures, following the | ||
| precedent in `connection.rs` and `client.rs`. `main` stays thin. Neither a | ||
| bigger thread in `main` nor a `/STACK` linker flag; both only add headroom | ||
| for the binary. | ||
| - Confirm first: measure `size_of_val` of the main future, `Relay::load(..)` | ||
| and `run()` in a debug build, and box what is actually large. | ||
| - Regression test: drive the relay's load and run with a minimal config on a | ||
| thread with a 1 MiB stack in a debug build. It must overflow without the fix | ||
| on any OS, so Linux CI catches it too. | ||
| - Windows CI adds `-p moq-relay` to `windows-test` (`rs/justfile`), so the | ||
| relay's tests, the regression above included, run on Windows. That needs | ||
| the tests' Unix-only expectations gated first, keeping them on Unix: | ||
| `rs/moq-relay/tests/released_cli.rs` requires the `server-unix-*` flags and | ||
| their env vars, which exist only under `cfg(all(feature = "uds", unix))` | ||
| (`rs/moq-tokio/src/listen.rs`). | ||
|
|
||
| No release backport is planned; release builds have smaller futures. If the | ||
| fix shows `release` overflows too, ask the maintainer. | ||
|
|
||
| Public API: none expected (boxing is internal). Wire: none. | ||
|
|
||
| ## Closes | ||
|
|
||
| - [#5563](https://github.com/moq-dev/moq/issues/5563) - Relay: Stack overflow error launching relay on Windows | ||
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,46 @@ | ||
| # [S] Backport the Lite SETUP violation closes to release | ||
|
|
||
| ## Goal | ||
|
|
||
| On `release`, a moq-lite peer that repeats a SETUP Parameter ID or opens a | ||
| second Setup Stream gets the session closed with PROTOCOL_VIOLATION (0x3), as | ||
| `drafts/draft-lcurley-moq-lite.md` requires, so a later 0.14.x CLI and 0.3.x | ||
| moq-net patch pass moq-dev/smoke's moq-lite conformance rows | ||
| L06-7-3-MUST-112 and L06-6-3-1-MUST-092. | ||
|
|
||
| ## Plan | ||
|
|
||
| Reported in [#5521](https://github.com/moq-dev/moq/issues/5521) against `moq` | ||
| 0.14.4. Verified 2026-10-15 that `main` already closes both cases: | ||
|
|
||
| - A second Setup Stream: `PeerSetup::claim` (`rs/moq-net/src/lite/setup.rs`) | ||
| claims the slot as soon as the stream type is Setup, from 7ddfa101b in | ||
| #4704. Tests: `duplicate_setup_closes_the_session`, | ||
| `accept_setup_refuses_two_incomplete_setup_streams`, | ||
| `accepted_lite_setup_refuses_a_second_setup_stream`. | ||
| - A SETUP that fails to decode (a duplicate parameter returns | ||
| `DecodeError::Duplicate`): the `UniServe` Err branch in | ||
| `rs/moq-net/src/lite/subscriber.rs` returns ProtocolViolation, from | ||
| 0f6a8d1f9 in #4704. No test sends a repeated Parameter ID. | ||
|
|
||
| On `release`, `UniServe` aborts a stream that fails to decode and returns | ||
| `Ok`, and `peer_setup.set` overwrites a second SETUP. The server's | ||
| `accept_setup` path already maps a decode error to ProtocolViolation there. | ||
|
|
||
| Decided 2026-10-15 (maintainer): | ||
|
|
||
| - One PR onto `release` (CONTRIBUTING) that ports only the two closes: the | ||
| `PeerSetup` claim and the close on a SETUP decode error, with their tests. | ||
| Not the rest of #4704 (early stream retention) or #5550. | ||
| - Add a repeated-Parameter-ID regression test on both `main` and `release`, | ||
| one that fails without the close. Check whether js/net closes on a repeated | ||
| parameter too; if it does not, report it as a follow-up rather than | ||
| widening this backport. | ||
| - The crate patch bumps happen at the release cut, not in this PR. | ||
|
|
||
| Public API: none. Wire: none (`release` starts enforcing rules the draft | ||
| already states). | ||
|
|
||
| ## Closes | ||
|
|
||
| - [#5521](https://github.com/moq-dev/moq/issues/5521) - moq-lite-06: a repeated SETUP parameter or a second Setup Stream does not close the session |
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,56 @@ | ||
| # [M] A TS export never exits on start-up latency | ||
|
|
||
| ## Goal | ||
|
|
||
| `moq export ts` never exits with "the source's clock runs ... ppm off ours" | ||
| for a source within the 30 ppm tolerance, however its latency moves while | ||
| the export starts (queueing that builds, or a backlog that drains). A source | ||
| far off real time (the 220,000 ppm case) still fails within seconds. `just | ||
| test ts --full` stops failing this way. | ||
|
|
||
| ## Plan | ||
|
|
||
| Reported in [#5564](https://github.com/moq-dev/moq/issues/5564): 2 of 10 local | ||
| `--full` runs on `main` (3abf0161c) and 1 of 51 CI runs exited within 30 s. | ||
| The first `TS export release clock` lines read wild drift (-155315, +32045 | ||
| ppm), and passing runs log none. | ||
|
|
||
| From reading the code on 5beead687 (not reproduced): `Buffer::steer` | ||
| (`rs/moq-mux/src/jitter.rs`) takes one floor every 2 s, fits an upper convex | ||
| hull, and reads both the rate and `gap` (extrapolated to `now`) from it. A | ||
| rate needs only two floors, with no minimum span or significance gate. `lost` is measured against `startup`, the gap | ||
| at the second floor. The export bails when `|slow| > 30 ppm` and `lost` | ||
|
kixelated marked this conversation as resolved.
|
||
| passes half the delay (450 ms at `--full`'s 900 ms). So a latency ramp after | ||
| the second floor, on a 0 ppm source, can end the export. Before #5490 | ||
| (41f3087e3) the exit also required the steered drift to reach its limit, | ||
| which made start-up noise unable to end it; `--full` (#5516) landed half an | ||
| hour after #5490. | ||
|
|
||
| Decided 2026-10-15 (maintainer): | ||
|
|
||
| - Reproduce first with the issue's loop, and record which floors skew the | ||
| envelope, before choosing a mechanism. | ||
| - Fix the measurement; do not add a minimum span or sample count. That keeps | ||
| the decision recorded when #5490 removed `quest/m1/slow-source.md`. | ||
| - Never widen the 30 ppm tolerance, `--delay`, or the `lost` threshold. | ||
| - Candidate mechanisms, for the implementer to pick from the evidence: | ||
| judge `lost` from observed floors instead of the envelope extrapolated to | ||
| `now`; exit only when the rate's uncertainty excludes 30 ppm; keep | ||
| `startup` floating until the rate is significant; and treat a fast | ||
| source's positive `lost` (a growing buffer) differently from loss. | ||
| - Mocked-time regressions in `jitter.rs`, each failing without the fix: a | ||
| 0 ppm source whose queueing builds 300 to 500 ms over 5 to 20 s and then | ||
| holds, the same draining, and both again with three tracks shaped like | ||
| `--full` (video sent up to 0.65 s ahead). | ||
| `a_source_far_off_real_time_fails_within_seconds` keeps passing. | ||
|
|
||
| Public API: none. Wire: none. | ||
|
|
||
| ## Closes | ||
|
|
||
| - [#5564](https://github.com/moq-dev/moq/issues/5564) - flaky: `just test ts --full` exits on the release clock's first rate estimates | ||
|
|
||
| ## Related | ||
|
|
||
| - [Live TS clock validation](/quest/m1/ts-live-clock.md) - the grader's PCR-rate verdict on `--live`, a different layer | ||
| - [TS decoded content](/quest/m1/test-flakes-2/ts-video-content.md) - the other TS CI flake, on content loss | ||
Oops, something went wrong.
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.
[P2] Include platform-gating the relay CLI expectations in this Windows CI step
Adding -p moq-relay to windows-test runs tests/released_cli.rs, whose RELEASED table unconditionally requires server-unix-bind and server-unix-allow-{uid,gid,pid} plus their environment variables (lines 92–95). Those fields exist only under cfg(all(feature = "uds", unix)) in moq-tokio/src/listen.rs:110–115, so both released_flags_still_parse and released_env_vars_are_still_read will fail on Windows even after the stack fix. Add platform-appropriate expectations to this quest's scope, preserving the Unix assertions, so the new Windows gate can actually go green.
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.
Agreed. The quest now scopes gating the Unix-only expectations in
released_cli.rs(theserver-unix-*flags and env vars) behindcfg(unix)before adding-p moq-relaytowindows-test, keeping the Unix assertions (00c0d2f).(Written by Opus 5.5)