From 5d06fb8a01ab34e0b9fe63c44d470384288f7f11 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Mon, 28 Sep 2026 16:16:54 -0700 Subject: [PATCH 01/37] quest: plan the 14 open issues without a quest (#4388) Co-authored-by: Claude Opus 5.5 --- quest/m0/README.md | 1 + quest/m0/relay-systemd-unit.md | 18 ++++++++++++ quest/m1/README.md | 9 ++++++ quest/m1/cluster-routing.md | 1 + quest/m1/cross-relay-bursts.md | 34 ++++++++++++++++++++++ quest/m1/fin-wait-expiry.md | 31 ++++++++++++++++++++ quest/m1/hop-aligned-import.md | 45 ++++++++++++++++++++++++++++++ quest/m1/moqsink-keyframe-latch.md | 34 ++++++++++++++++++++++ quest/m1/test-flakes-2.md | 4 +++ quest/m1/tooling/justfiles.md | 9 ++++++ quest/m1/ts-programs.md | 37 ++++++++++++++++++++++++ quest/m1/ts-publish-muxdelay.md | 27 ++++++++++++++++++ quest/m1/watch-decoder-recovery.md | 28 +++++++++++++++++++ quest/m1/watch-video-guards.md | 37 ++++++++++++++++++++++++ quest/m1/watch-worklet-file.md | 29 +++++++++++++++++++ quest/m2/redundant-ingest.md | 1 + 16 files changed, 345 insertions(+) create mode 100644 quest/m0/relay-systemd-unit.md create mode 100644 quest/m1/cross-relay-bursts.md create mode 100644 quest/m1/fin-wait-expiry.md create mode 100644 quest/m1/hop-aligned-import.md create mode 100644 quest/m1/moqsink-keyframe-latch.md create mode 100644 quest/m1/ts-programs.md create mode 100644 quest/m1/ts-publish-muxdelay.md create mode 100644 quest/m1/watch-decoder-recovery.md create mode 100644 quest/m1/watch-video-guards.md create mode 100644 quest/m1/watch-worklet-file.md diff --git a/quest/m0/README.md b/quest/m0/README.md index 0f104ffe7e..935edeae68 100644 --- a/quest/m0/README.md +++ b/quest/m0/README.md @@ -30,6 +30,7 @@ Published API or wire breaks still land on dev; each quest's Plan says so. ## Required +- [Packaged relay starts](/quest/m0/relay-systemd-unit.md) - the `.deb` and `.rpm` relay service starts instead of crash-looping on `--file` - [Skip unchanged announce updates](/quest/m0/announce-update-dedupe.md) - a publisher sends an announce update only when the wire route changed - [Wildcard](/quest/m0/wildcard/README.md) - a relay resolves subscriptions against advertised prefixes, a service claims the prefix it could serve and refuses the rest instead of enumerating broadcasts, and the browser player treats a covering claim as availability - [Audio quality harness](/quest/m0/audio-quality-harness/README.md) - a browser playout latency regression fails a nightly run instead of arriving as a bug report, and its recorder supplies the jitter target's replay traces diff --git a/quest/m0/relay-systemd-unit.md b/quest/m0/relay-systemd-unit.md new file mode 100644 index 0000000000..37a7e51a19 --- /dev/null +++ b/quest/m0/relay-systemd-unit.md @@ -0,0 +1,18 @@ +# [XS] Packaged relay starts + +## Goal + +The `.deb` and `.rpm` relay service starts. Today +`packaging/moq-relay/moq-relay.service` runs `moq-relay --file +/etc/moq-relay/relay.toml`, but the config path is positional and the relay +rejects unknown flags, so every packaged install crash-loops. + +## Plan + +`ExecStart=/usr/bin/moq-relay /etc/moq-relay/relay.toml`. Add the reporter's +unit test in the relay crate that parses the unit's `ExecStart` with the +relay's own `Cli`, so the unit and the parser can't drift again. + +## Closes + +- [#4343](https://github.com/moq-dev/moq/issues/4343) - close this issue when the quest finishes diff --git a/quest/m1/README.md b/quest/m1/README.md index 7e397ee3cd..be62e0315b 100644 --- a/quest/m1/README.md +++ b/quest/m1/README.md @@ -33,12 +33,18 @@ transport, benchmark tooling); worktrees isolate commits, not semantics. - [Live in apps](/quest/m1/announce-live-apps.md) - the demo and `@moq/room` show "no broadcasts" from the `live` marker, which waits for the first session on page load - [Watch refusal](/quest/m1/watch-refusal.md) - `` shows an origin refusal as an error instead of sitting offline - [kio waiter overflow](/quest/m1/kio-waiter-lost.md) - a retained `Waiter` past 8 lists stops adding a duplicate entry to lists it already recorded +- [moqsink recoverable errors](/quest/m1/moqsink-keyframe-latch.md) - a leading delta or a timestamp rewind drops frames until a keyframe instead of invalidating a moqsink pad - [Capture re-anchor](/quest/m1/capture-reanchor.md) - a repeating or restarting device clock never rewinds native capture during a fast backlog drain - [Splice edge cases](/quest/m1/splice-edges.md) - an unstamped successor, a pruned segment's boundary group, and a warm head during a takeover are each handled correctly - [Resumed groups](/quest/m1/resume-latest.md) - a half-delivered group ends once the new copy is past it, so a group-only reader never parks after a mid-group failover - [Track tail hardening](/quest/m1/track-tail-hardening.md) - Rust and JS wait out a track's tail by the same rules, with the known hang, count, truncation, grace, and memory holes closed - [SUBSCRIBE_DROP](/quest/m1/subscribe-drop.md) - every stream group in a lite subscription arrives or is dropped by name, and lite-07 drops its stream count for it +- [FIN wait expiry](/quest/m1/fin-wait-expiry.md) - a group awaiting its FIN ack still expires and follows priority updates on lite and IETF +- [Cross-relay bursts](/quest/m1/cross-relay-bursts.md) - bursty small-group tracks cross two relays without lost groups, unanswered FETCHes, or stalls - [Session death parity](/quest/m1/session-death.md) - a local close ends tracks cleanly in both languages, and JS group readers see the session's error on session death +- [Watch video guards](/quest/m1/watch-video-guards.md) - promoting a video track holds the last picture, and an older group never reaches the codec between live deltas +- [Watch decoder recovery](/quest/m1/watch-decoder-recovery.md) - one malformed packet rebuilds the audio or video decoder instead of ending playback +- [Watch audio under CSP](/quest/m1/watch-worklet-file.md) - production builds ship the audio worklet as a file, so `script-src 'self'` pages play audio - [moqsrc stop](/quest/m1/moqsrc-stop.md) - moqsrc's stop blocks until its session ends, without deadlocking on a blocked pad push - [More tests under load](/quest/m1/test-flakes-2.md) - the second round of load-only failures, fixed at the cause - [Auth outage clock](/quest/m1/auth-outage-clock.md) - the relay and moq-auth outage tests run on a paused clock again and assert both bounds of `expires` @@ -50,8 +56,11 @@ transport, benchmark tooling); worktrees isolate commits, not semantics. - [Accept-side flags](/quest/m1/cli-given-flags.md) - dial-only and local verbs refuse every `--listen-*` flag instead of ignoring it - [#2075](/quest/m1/2075-mirror-catalog-reservation-gating-in-moq-hang-js-hang.md) - @moq/publish gates the first catalog snapshot until every reserved track is described - [Full codec string](/quest/m1/publish-codec-string.md) - browser-published video carries the encoder's full RFC 6381 codec string, so native players decode it +- [TS program selection](/quest/m1/ts-programs.md) - `import ts` refuses a multi-program stream unless `--program ` picks one or publishes each - [TS export jitter](/quest/m1/ts-export-jitter.md) - the video reorder bound follows later catalogs and the declared reorder depth, so a late B-frame never reorders TS output; an undeclared stream can reorder once per new maximum depth - [TS import shared shift](/quest/m1/ts-import-shared-shift.md) - unflagged loop wraps move audio and video by one shift, so A/V sync holds across wraps +- [ffmpeg muxdelay](/quest/m1/ts-publish-muxdelay.md) - the documented MPEG-TS publish line adds `-muxdelay 0`, so quiet audio is not clumped +- [Same-hop importers](/quest/m1/hop-aligned-import.md) - importers sharing a `--hop` and fed one stream publish identical groups and timestamps, so failover survives - [PipeWire duplicate cameras](/quest/m1/pipewire-dup-cameras.md) - a webcam lists once with PipeWire enabled - [Catalog wall clock](/quest/m1/catalog-wall-clock.md) - `Clock::wall_clock` keeps the catalog's full precision instead of truncating to milliseconds - [Capture control](/quest/m1/capture-control.md) - on dev, `encode::Capture` replaces `CaptureOptions` without a `clock` field (it reads the catalog's), an unsupported `cut()` errors, and dropping the last `Control` cancels in-flight opens diff --git a/quest/m1/cluster-routing.md b/quest/m1/cluster-routing.md index 983b880eb6..3d52174867 100644 --- a/quest/m1/cluster-routing.md +++ b/quest/m1/cluster-routing.md @@ -122,3 +122,4 @@ make MoQ's common case. - [Skip unchanged announce updates](/quest/m0/announce-update-dedupe.md) - cuts duplicate updates on today's routing - [Redundant ingest](/quest/m2/redundant-ingest.md) - builds on the `--hop` failover this must keep or replace - [Routing cost domains](/quest/m2/routing-cost-domains.md) - cost across the cluster boundaries this keeps path vector +- [Cross-relay delivery under bursts](/quest/m1/cross-relay-bursts.md) - its #4349 report also shows closed broadcasts announced for up to 229 s and flapping between Retracted and Announced across nodes, evidence for per-incarnation seqnos diff --git a/quest/m1/cross-relay-bursts.md b/quest/m1/cross-relay-bursts.md new file mode 100644 index 0000000000..25607b1be8 --- /dev/null +++ b/quest/m1/cross-relay-bursts.md @@ -0,0 +1,34 @@ +# [L] Cross-relay delivery under bursts + +## Goal + +Bursty small-group tracks cross two relays without losing groups or stalling. +On cdn.moq.pro, publishers and a subscriber on different nodes saw groups +never arrive (FETCH for them unanswered for 2 s while the publisher stayed +connected), a subscription opened early deliver only a burst's tail, and +return tracks stall up to 31 s with `Stream(Old)`. The same workload on one +node, or one self-hosted relay, loses nothing. + +## Plan + +Reproduce first, on a local two-relay cluster with the reporter's harness +(offered in the issue) and the relay build cdn.moq.pro ran. Then fix what the +repro shows. Suspects: `Old` expiry or newest-first dropping on the relay hop, +and FETCH not forwarded or answered upstream. Re-measure the stalls after +[FIN wait expiry](/quest/m1/fin-wait-expiry.md), a candidate cause. Keep the +repro as a regression test in the cluster tests. + +The issue's stale and flapping announcements after a clean close are +cluster-routing evidence, not this quest's scope. + +## Required + +- [FIN wait expiry](/quest/m1/fin-wait-expiry.md) - removes one candidate cause of the stalls before measuring + +## Closes + +- [#4349](https://github.com/moq-dev/moq/issues/4349) - close this issue when the quest finishes + +## Related + +- [Cluster routing](/quest/m1/cluster-routing.md) - owns the stale and flapping announcements from the same report diff --git a/quest/m1/fin-wait-expiry.md b/quest/m1/fin-wait-expiry.md new file mode 100644 index 0000000000..eb163fa6fe --- /dev/null +++ b/quest/m1/fin-wait-expiry.md @@ -0,0 +1,31 @@ +# [S] A group awaiting its FIN ack still expires + +## Goal + +A group whose FIN is written but not yet acknowledged still expires with +`Old` and still follows priority updates, on lite and IETF. Today the +`Closed` arm only awaits `writer.poll_close`, so a stale group keeps its +queued bytes and its old send order, which can stall newer groups behind it. + +## Plan + +The issue ships four failing tests, a `with_fin_gate` test hook, and a fix: + +- lite: poll priority above the state match, not only in `Serve`. +- lite and IETF: in `Closed`, poll `poll_expired_while_pending` and abort with + `Old`. +- moq-tokio: `set_priority` in `Closing` fires the existing interrupt so the + new order applies, instead of only storing it. + +Each priority update rebuilds the `closed()` watch; check the publisher +benchmarks for a regression. The flaky +`subscription_end_integrity::a_subscription_cut_by_the_publisher_disconnecting_does_not_end_clean` +the reporter hit is tracked in [More tests under load](/quest/m1/test-flakes-2.md). + +## Closes + +- [#4332](https://github.com/moq-dev/moq/issues/4332) - close this issue when the quest finishes + +## Related + +- [Starvation](/quest/m1/qos/starvation.md) - its frontier relies on FIN acks, which can now end in `Old` diff --git a/quest/m1/hop-aligned-import.md b/quest/m1/hop-aligned-import.md new file mode 100644 index 0000000000..6f6f68b6c6 --- /dev/null +++ b/quest/m1/hop-aligned-import.md @@ -0,0 +1,45 @@ +# [L] Same-hop importers publish identical tracks + +## Goal + +Two importers sharing a `--hop` and fed the same encoded stream publish +identical tracks, so a relay can fail over between them. Today two +`moq import ts` fed one forked stream break that contract twice: a standby +refuses every track until it has parsed its PMT, which aborts every +subscriber the moment it connects (#4352), and each numbers its groups from +its own counter, so after a failover `export ts` sees a timestamp rewind and +exits (#4354). + +## Plan + +Decided: same-hop publishers MUST publish the same broadcasts and tracks. +Keep `--hop`, and make every container importer (ts, fmp4, flv, mkv, and the +SRT, RTMP, and HLS gateways that reuse them) meet the contract when fed one +encoded stream. Capture is out: two encoders never align. + +- A group's sequence derives from its keyframe's media timestamp (PTS in TS), + not a per-process counter. Decide how both processes agree across a + timestamp wrap (TS PTS wraps every 26.5 h) when they started on opposite + sides of it. +- Frame timestamps derive from the input alone too: any re-anchor shift must + come from the stream, not process start or wall clock. +- An importer announces only once it knows its tracks, so it never refuses a + track the incumbent serves. +- Docs (`doc/bin/cli.md` "Redundant publishers", the `--hop` doc comment) + narrow the contract to one encoded stream fed to each publisher. + +Tests: per importer, two instances started at different offsets into the same +input produce the same group sequences and timestamps. End to end: the +issues' 1+1 setup (one relay, two same-hop `import ts`, two `export ts`) +survives the standby joining and the incumbent stopping. + +## Closes + +- [#4352](https://github.com/moq-dev/moq/issues/4352) - close this issue when the quest finishes +- [#4354](https://github.com/moq-dev/moq/issues/4354) - close this issue when the quest finishes + +## Related + +- [Redundant ingest](/quest/m2/redundant-ingest.md) - splicing across first hops and two encoders, which this does not attempt +- [TS import shared shift](/quest/m1/ts-import-shared-shift.md) - the TS re-anchor shift that must stay input-derived +- [Broadcast epochs](/quest/m1/broadcast-epoch/README.md) - a redundant pair shares one epoch diff --git a/quest/m1/moqsink-keyframe-latch.md b/quest/m1/moqsink-keyframe-latch.md new file mode 100644 index 0000000000..5cb8205681 --- /dev/null +++ b/quest/m1/moqsink-keyframe-latch.md @@ -0,0 +1,34 @@ +# [S] moqsink drops frames on recoverable importer errors + +## Goal + +A `moqsink` pad survives the importer errors moq-mux documents as +recoverable: it drops frames until the next keyframe instead of calling +`self.fail()` and losing the rendition for good. Two paths reach that +failure in `Media::write` (`rs/moq-gst/src/sink/pad.rs`) today: + +- A new pad starts unlatched (`keyframe: false`), so a first buffer that is a + delta frame fails it. +- A `TimestampRewind` with no signalled break (an rtspsrc jitterbuffer + re-anchoring) has no guard at all. + +## Plan + +The header-only buffer after a break was fixed in +https://github.com/moq-dev/moq/pull/4356: the latch stays armed after a +successful decode. Reuse that latch here. + +Decided: a video pad starts latched. On `TimestampRewind`, set the latch and +drop until a keyframe lands above the producer's live edge, losing up to one +GOP per re-anchor. Mapping a rewound timeline forward instead belongs to +[#3021](/quest/m1/3021-moq-gst-anchor-generated-media-timelines-to-wall-clock.md). +The reporter offered the guard they run in production. + +Regression tests beside `video_pause_drops_deltas_until_the_next_keyframe`: + +- A new pad's first buffer is a delta: it drops. +- A rewind without a break: deltas drop until a keyframe above the edge. + +## Closes + +- [#4366](https://github.com/moq-dev/moq/issues/4366) - close this issue when the quest finishes diff --git a/quest/m1/test-flakes-2.md b/quest/m1/test-flakes-2.md index c1742598ca..8cc9502a74 100644 --- a/quest/m1/test-flakes-2.md +++ b/quest/m1/test-flakes-2.md @@ -22,6 +22,10 @@ retry. [#4084](https://github.com/moq-dev/moq/pull/4084) removed from its sibling after [#4055](https://github.com/moq-dev/moq/pull/4055) papered over it with a retry. +- moq-tokio + `subscription_end_integrity::a_subscription_cut_by_the_publisher_disconnecting_does_not_end_clean` + ends `Ok(None)` with 10 of 20 frames in 3 of 8 full-suite runs on a clean + tree, and passes alone (#4332). - `just test media` late join failed once after [#4181](https://github.com/moq-dev/moq/pull/4181): "joined at frame 111, 16 frames behind 127", against a budget of one GOP (15). diff --git a/quest/m1/tooling/justfiles.md b/quest/m1/tooling/justfiles.md index 22597e1461..429dedb432 100644 --- a/quest/m1/tooling/justfiles.md +++ b/quest/m1/tooling/justfiles.md @@ -86,8 +86,17 @@ wanted), `_shell`, `_flake` as its one-liner, `clean`, the rs `package` and `fuzz` bodies, and the OBS `compile`, `_includes`, `_unit`, `test`, `check`, and `preset` bodies. +Arguments reach tools intact: `just rs test`, `check-test`, and +`capture-test` interpolate `{{ args }}` today, so a quoted nextest filterset +becomes a shell syntax error (#4342). Recipes that forward arguments use +`[positional-arguments]` and `"$@"`. No self-test for it, per the rule above. + Docs: `doc/setup/dev.md`, `CONTRIBUTING.md`, `test/README.md`, and the `AGENTS.md` mentions of `just wasm` follow the survivors. Verify with `just check`, `just ci test`, and `just check --all`, and confirm every recipe name check.yml, cache.yml, nightly.yml, interop.yml, wasm.yml, obs.yml, swift.yml, and release-*.yml invoke still resolves. + +## Closes + +- [#4342](https://github.com/moq-dev/moq/issues/4342) - close this issue when the quest finishes diff --git a/quest/m1/ts-programs.md b/quest/m1/ts-programs.md new file mode 100644 index 0000000000..14e17a964c --- /dev/null +++ b/quest/m1/ts-programs.md @@ -0,0 +1,37 @@ +# [L] moq import ts: select programs + +## Goal + +A multi-program transport stream is never silently merged. Today `import ts` +keeps the first program's identity, adds every PMT's streams to one broadcast +on one clock, and loses content; `export ts` rebuilds one program. After +this quest `import ts` refuses a multi-program input by default, naming the +programs, and `--program` chooses what to import. + +## Plan + +Decided: + +- Default: error before publishing when the initial PAT lists more than one + non-zero program, naming each program and pointing at `--program`. A later + PAT that adds a program ends the import with the same error; media already + published stays. +- `--program ` imports program `n` only. +- `--program all` publishes one broadcast per program, each with its own clock + and catalog. The catalog suffix stays last so discovery and format detection + still see it: `--broadcast event.hang` publishes `event/1.hang` and + `event/2.hang`. +- `export ts` stays one program per broadcast. + +Tests: a synthetic two-program input with far-apart clocks fails naming both +programs; `--program 2` publishes only program 2's streams on program 2's +clock; `--program all` publishes both broadcasts, each timed correctly. Update +`doc/bin/cli.md` and the `--help` text. + +## Closes + +- [#4353](https://github.com/moq-dev/moq/issues/4353) - close this issue when the quest finishes + +## Related + +- [TS import shared shift](/quest/m1/ts-import-shared-shift.md) - its per-program shift assumes the single program this settles diff --git a/quest/m1/ts-publish-muxdelay.md b/quest/m1/ts-publish-muxdelay.md new file mode 100644 index 0000000000..688904da4d --- /dev/null +++ b/quest/m1/ts-publish-muxdelay.md @@ -0,0 +1,27 @@ +# [XS] Documented ffmpeg line flushes audio + +## Goal + +The documented `ffmpeg ... -f mpegts -pes_payload_size 0 -` publish line stops +holding small audio frames in ffmpeg's muxer for up to 0.35 s. Those clumps +raise the importer's catalog `jitter` for the life of the track, and watch +adds about 230 ms of playout delay to match. + +## Plan + +Decided: add `-muxdelay 0` everywhere the line appears +(`demo/pub/justfile`, `doc/bin/cli.md`, `doc/concept/standard.md`, +`rs/moq-cli/README.md`). `import ts` needs no PCR lead. The reporter's patch +is ready. Verify by re-measuring the catalog jitter on a quiet-audio source; +there is no automated test for a docs line. + +The importer's jitter estimate keeping its maximum forever is a separate +problem; the player side is covered by the audio jitter target. + +## Closes + +- [#4347](https://github.com/moq-dev/moq/issues/4347) - close this issue when the quest finishes + +## Related + +- [Audio jitter target](/quest/m0/audio-jitter-target/README.md) - a decaying playout estimate softens a transient burst diff --git a/quest/m1/watch-decoder-recovery.md b/quest/m1/watch-decoder-recovery.md new file mode 100644 index 0000000000..f4e208676d --- /dev/null +++ b/quest/m1/watch-decoder-recovery.md @@ -0,0 +1,28 @@ +# [M] Watch decoders recover from an error + +## Goal + +One malformed packet no longer ends audio or video for the rest of a +subscription. Today a WebCodecs error closes the decoder, both audio loops +`break` (added in #2415 to stop an `InvalidStateError` loop), and video calls +`effect.close()`; nothing rebuilds the decoder, and playback silently stops. + +## Plan + +Decided: rebuild inside the loop, for audio and video alike. On a decoder +error, rebuild through one shared `build()` helper, skip to the next keyframe, +and give up after three rebuilds in a row with no output. A rebuilt legacy +audio decoder re-runs its warmup priming. Cover the legacy and CMAF loops. + +Tests: a corrupt packet mid-stream is followed by decoded output from the next +keyframe; a stream of only corrupt packets gives up after three rebuilds +instead of spinning. + +## Closes + +- [#4324](https://github.com/moq-dev/moq/issues/4324) - close this issue when the quest finishes + +## Related + +- [Audio warmup](/quest/m1/audio-warmup.md) - the same audio loops and priming +- [Watch video guards](/quest/m1/watch-video-guards.md) - keeps bad input from reaching the video codec diff --git a/quest/m1/watch-video-guards.md b/quest/m1/watch-video-guards.md new file mode 100644 index 0000000000..765b652fc7 --- /dev/null +++ b/quest/m1/watch-video-guards.md @@ -0,0 +1,37 @@ +# [S] Watch video holds its picture and its order + +## Goal + +Two `js/watch/src/video/decoder.ts` defects stop reaching the viewer: + +- Promoting a video track when none is active (after pause, scrolling back + into view, or a hidden rendition returning) holds the last picture instead + of painting black for a round trip plus a keyframe. +- An older finished group accepted behind the live group is never fed to the + codec between live deltas. + +## Plan + +Decided: + +- `#runActive` ignores the new track's initial `undefined` frame; clearing + stays with `#clearCurrentFrame` and `close()`. The held timestamp stays + with the held frame, so the outputs describe what is on screen. +- The guard lives in both video decode loops (legacy and CMAF): skip a frame + whose group is older than the newest group already fed, and reset on a + discontinuity. The consumer keeps serving older groups, since audio's + timestamp-indexed ring needs them. Fix the consumer comment claiming video + drops a late frame at render; that only holds after decode. + +Tests: a promote from nothing keeps the previous frame and timestamp; an older +group arriving after a newer one never reaches the decoder. + +## Closes + +- [#4338](https://github.com/moq-dev/moq/issues/4338) - close this issue when the quest finishes +- [#4339](https://github.com/moq-dev/moq/issues/4339) - close this issue when the quest finishes + +## Related + +- [#3056](/quest/m1/3056-watch-video-decoder-captures-the-rewind-generation-at.md) - another fix in the same decoder +- [Watch decoder recovery](/quest/m1/watch-decoder-recovery.md) - a bad feed today ends video for the subscription diff --git a/quest/m1/watch-worklet-file.md b/quest/m1/watch-worklet-file.md new file mode 100644 index 0000000000..b6a778364a --- /dev/null +++ b/quest/m1/watch-worklet-file.md @@ -0,0 +1,29 @@ +# [S] Watch audio under a strict CSP + +## Goal + +A page with CSP `script-src 'self'` plays watch audio. Today +`js/common/vite-plugin-worklet.ts` always exports the render worklet as a +`blob:` URL, so `addModule` is refused and audio never starts, with only a +console "spawn error". + +## Plan + +Decided: in `vite build`, emit the worklet as its own file and export +`new URL(".js", import.meta.url)`; keep the blob URL only in +`vite serve`. Vite, webpack 5, and esbuild (with a plugin) resolve that +pattern inside a dependency. This applies to the publish capture worklet too +and deletes the runtime blob code. Document the consumer-bundler requirement. +The publish capture worker (`?worker&inline`) is out of scope. + +Verify in a browser under `script-src 'self'` (Chromium and Firefox) and +from a Vite consumer app. + +## Closes + +- [#4323](https://github.com/moq-dev/moq/issues/4323) - close this issue when the quest finishes + +## Related + +- [JS bundle trims](/quest/m1/js-bundle-trims.md) - minifies worklets through the same plugin +- [Plan: watch worker](/quest/m1/plan-watch-worker.md) - notes the capture worker's `worker-src blob:` need diff --git a/quest/m2/redundant-ingest.md b/quest/m2/redundant-ingest.md index 96ce3c0644..241fe5bb33 100644 --- a/quest/m2/redundant-ingest.md +++ b/quest/m2/redundant-ingest.md @@ -25,5 +25,6 @@ decision, with a quest for the chosen mechanism. ## Related +- [Same-hop importers](/quest/m1/hop-aligned-import.md) - identical tracks from one encoded stream under one first hop, the case this generalizes - [Cluster routing](/quest/m1/cluster-routing.md) - decides what replaces first-hop failover inside a cluster - [Broadcast epochs](/quest/m1/broadcast-epoch/README.md) - explicit epochs are what a redundant pair would share From a2d501d276c879fe5972645320370367d8fb500f Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Mon, 28 Sep 2026 16:30:57 -0700 Subject: [PATCH 02/37] chore: bump quest to 8590d2a (#4413) Co-authored-by: Claude Opus 5.5 --- flake.lock | 8 ++++---- flake.nix | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/flake.lock b/flake.lock index 73deb19a16..85980a8b8f 100644 --- a/flake.lock +++ b/flake.lock @@ -65,17 +65,17 @@ ] }, "locked": { - "lastModified": 1790617890, - "narHash": "sha256-k8uvr4/Pt5Hf+Adjv3sp84l6HLkpR5BSKHoeMj4cxRA=", + "lastModified": 1790635831, + "narHash": "sha256-K/X+stFBmlCylJ9wiMCXxKOckoHeSninMvBb0wZa1rM=", "owner": "kixelated", "repo": "quest", - "rev": "46d7fe89247919583632e4963aee1c9a68dfe059", + "rev": "8590d2a1ddd91c2f499adf37b78aad0d673e3228", "type": "github" }, "original": { "owner": "kixelated", "repo": "quest", - "rev": "46d7fe89247919583632e4963aee1c9a68dfe059", + "rev": "8590d2a1ddd91c2f499adf37b78aad0d673e3228", "type": "github" } }, diff --git a/flake.nix b/flake.nix index 7c0920487e..bf676502d6 100644 --- a/flake.nix +++ b/flake.nix @@ -27,7 +27,7 @@ # The quest CLI, which also serves the quest guide and skills the stubs in # .claude/skills call. Bump the rev to upgrade them. quest = { - url = "github:kixelated/quest/46d7fe89247919583632e4963aee1c9a68dfe059"; + url = "github:kixelated/quest/8590d2a1ddd91c2f499adf37b78aad0d673e3228"; inputs.nixpkgs.follows = "nixpkgs"; inputs.flake-utils.follows = "flake-utils"; inputs.crane.follows = "crane"; From 61f642dc103f9962a7d826c83e5040f638620675 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Mon, 28 Sep 2026 17:13:05 -0700 Subject: [PATCH 03/37] refactor(watch): share hang's catalog containment check (#4415) Co-authored-by: Claude Opus 5.5 --- doc/lib/js/hang.md | 3 +- js/hang/src/catalog/consumer.test.ts | 28 ++++++++- js/hang/src/catalog/root.ts | 4 +- js/watch/src/broadcast.test.ts | 85 +++++++++------------------- js/watch/src/broadcast.ts | 48 +--------------- 5 files changed, 62 insertions(+), 106 deletions(-) diff --git a/doc/lib/js/hang.md b/doc/lib/js/hang.md index 07d9325f5b..3487eeb136 100644 --- a/doc/lib/js/hang.md +++ b/doc/lib/js/hang.md @@ -22,7 +22,8 @@ import * as Container from "@moq/hang/container"; `Catalog.watch(broadcast)` iterates validated catalog roots. It throws `Catalog.TooManyRenditions` for an update above the 64 rendition limit, and `Catalog.EscapingBroadcast` for a `broadcast` reference that walks above the -handle's `path`. +handle's `path`. Run the same checks on a catalog from another source with +`Catalog.checkRenditions(root)` and `Catalog.checkResolvable(root, base)`. `Hang.Timeline.Consumer.subscribe(broadcast, root.archive)` reads segment `push`, `pop`, and `skip` events when a root advertises an archive. diff --git a/js/hang/src/catalog/consumer.test.ts b/js/hang/src/catalog/consumer.test.ts index 6b0e328aae..39ecac5811 100644 --- a/js/hang/src/catalog/consumer.test.ts +++ b/js/hang/src/catalog/consumer.test.ts @@ -2,7 +2,15 @@ import { expect, test } from "bun:test"; import * as Json from "@moq/json"; import * as Moq from "@moq/net"; import { TRACK } from "./format"; -import { checkRenditions, EscapingBroadcast, MAX_RENDITIONS, type Root, TooManyRenditions, watch } from "./root"; +import { + checkRenditions, + checkResolvable, + EscapingBroadcast, + MAX_RENDITIONS, + type Root, + TooManyRenditions, + watch, +} from "./root"; function catalog(count: number): Root { return { @@ -29,6 +37,24 @@ test("the shared cap accepts 64 renditions and refuses 65 with a typed error", ( ).toThrow(TooManyRenditions); }); +test("the shared containment check covers every section carrying a broadcast reference", () => { + // @moq/watch runs this same check on the hangz, MSF, and manual catalogs, so a section left + // out here would exempt its tracks everywhere. + const base = Moq.Path.from("a/b"); + for (const [section, key] of [ + ["video", "renditions"], + ["audio", "renditions"], + ["text", "renditions"], + ["json", "tracks"], + ["binary", "tracks"], + ]) { + const reference = (broadcast: string) => + ({ [section]: { [key]: { entry: { broadcast: Moq.Path.normalizeRelative(broadcast) } } } }) as Root; + expect(() => checkResolvable(reference("../../x"), base)).toThrow(EscapingBroadcast); + expect(checkResolvable(reference("../x"), base)).toBeDefined(); + } +}); + test("watch refuses an oversized catalog update", async () => { const broadcast = new Moq.Broadcast.Producer(); const track = broadcast.createTrack(TRACK); diff --git a/js/hang/src/catalog/root.ts b/js/hang/src/catalog/root.ts index 97b1689600..6c52a0a833 100644 --- a/js/hang/src/catalog/root.ts +++ b/js/hang/src/catalog/root.ts @@ -84,8 +84,8 @@ export class EscapingBroadcast extends Error { } } -// Refuse an update with a `broadcast` reference that walks above the root from `base`. -function checkResolvable(root: Root, base: Moq.Path.Valid): Root { +/** Refuse an update with a `broadcast` reference that walks above the root from `base`. */ +export function checkResolvable(root: Root, base: Moq.Path.Valid): Root { // Every section carrying a `broadcast` reference must be listed here; one left out // silently exempts its tracks from the check. const sections = [ diff --git a/js/watch/src/broadcast.test.ts b/js/watch/src/broadcast.test.ts index 4debf75790..3a151f8de4 100644 --- a/js/watch/src/broadcast.test.ts +++ b/js/watch/src/broadcast.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "bun:test"; -import type * as Catalog from "@moq/hang/catalog"; +import * as Catalog from "@moq/hang/catalog"; import * as Moq from "@moq/net"; import { Origin, Path } from "@moq/net"; import { Effect, Signal } from "@moq/signals"; @@ -110,75 +110,46 @@ describe("relativeBroadcast", () => { const manual = (renditions: Record) => manualCatalog({ video: { renditions } } as Catalog.Root); - it("rejects a catalog carrying an escaping rendition", async () => { - // The whole catalog goes, not just the offending rendition: the root is this - // consumer's authorized subtree, so the reference names content it cannot reach, - // and serving the rest would hide a publisher bug behind a track that never fills. - const { source, owner } = manual({ - good: rendition(), - sibling: rendition("./source"), - bad: rendition("../../x"), - }); - + // The error a manual catalog was rejected with, after checking nothing was published. + const rejection = async (catalog: Catalog.Root): Promise => { + const { source, owner } = manualCatalog(catalog); const error = console.error; - console.error = () => {}; + let logged: unknown; + console.error = (...args: unknown[]) => { + logged = args.at(-1); + }; try { // The catalog is validated by an effect, which settles a microtask later. await Promise.resolve(); expect(source.out.catalog.peek()).toBeUndefined(); + return logged; } finally { console.error = error; source.close(); owner.close(); } - }); + }; - it("rejects a catalog whose json or binary track escapes the root", async () => { - // Data tracks carry the same `broadcast` reference as renditions. Rust rejects an - // escaping one; leaving the section out of the check would let it through. - const track = { mode: "snapshot", broadcast: Path.normalizeRelative("../../x") }; - for (const section of ["json", "binary"] as const) { - const { source, owner } = manualCatalog({ - video: { renditions: { good: rendition() } }, - [section]: { tracks: { status: track } }, - } as Catalog.Root); - - const error = console.error; - console.error = () => {}; - try { - await Promise.resolve(); - expect(source.out.catalog.peek()).toBeUndefined(); - } finally { - console.error = error; - source.close(); - owner.close(); - } - } + it("rejects a catalog carrying an escaping rendition", async () => { + // The whole catalog goes, not just the offending rendition: the root is this + // consumer's authorized subtree, so the reference names content it cannot reach, + // and serving the rest would hide a publisher bug behind a track that never fills. + const catalog = { + video: { renditions: { good: rendition(), sibling: rendition("./source"), bad: rendition("../../x") } }, + } as Catalog.Root; + expect(await rejection(catalog)).toBeInstanceOf(Catalog.EscapingBroadcast); }); - it("rejects a catalog whose text rendition escapes the root", async () => { - // The containment check covers every section carrying renditions: a text (caption) - // reference escaping the root rejects the catalog like a video or audio one. - const captions = { - format: "vtt", - role: "subtitle", - container: { kind: "legacy" }, - broadcast: "../../x", - } as Catalog.TextConfig; - const { source, owner } = manualCatalog({ - video: { renditions: { good: rendition() } }, - text: { renditions: { captions } }, - } as Catalog.Root); - - const error = console.error; - console.error = () => {}; - try { - await Promise.resolve(); - expect(source.out.catalog.peek()).toBeUndefined(); - } finally { - console.error = error; - source.close(); - owner.close(); + it("rejects a catalog whose text or data track escapes the root", async () => { + // The shared hang check runs here too, so every section it covers rejects the catalog. + const broadcast = Path.normalizeRelative("../../x"); + for (const section of ["text", "json", "binary"] as const) { + const key = section === "text" ? "renditions" : "tracks"; + const catalog = { + video: { renditions: { good: rendition() } }, + [section]: { [key]: { entry: { broadcast } } }, + } as Catalog.Root; + expect(await rejection(catalog)).toBeInstanceOf(Catalog.EscapingBroadcast); } }); diff --git a/js/watch/src/broadcast.ts b/js/watch/src/broadcast.ts index 1064a0708c..372ee02efc 100644 --- a/js/watch/src/broadcast.ts +++ b/js/watch/src/broadcast.ts @@ -7,48 +7,6 @@ import { Effect, type Getter, getter, type Inputs, type Readonlys, readonlys, Si import { toHang } from "./msf"; -/** - * The name of the first rendition whose `broadcast` reference walks above the root, if any. - * - * The root is the consumer's authorized subtree, so such a reference names content this - * consumer cannot reach. It rejects the whole catalog rather than the one rendition: a - * publisher that emits one has a bug, and quietly serving the rest hides that while the - * missing rendition resurfaces later as a track that never fills. - */ -function broadcastRefs( - section: Record | undefined, -): [string, Path.Relative | undefined][] { - return Object.entries(section ?? {}).map(([name, config]) => [name, config.broadcast]); -} - -function findEscaping(base: Moq.Path.Valid, catalog: Catalog.Root): string | undefined { - // Every section carrying a `broadcast` reference must be listed here, including data - // tracks. One left out silently exempts its tracks from the containment check. - const refs = [ - ...broadcastRefs(catalog.video?.renditions), - ...broadcastRefs(catalog.audio?.renditions), - ...broadcastRefs(catalog.text?.renditions), - ...broadcastRefs(catalog.json?.tracks), - ...broadcastRefs(catalog.binary?.tracks), - ]; - - for (const [name, rel] of refs) { - if (rel && Path.tryResolve(base, rel) === undefined) return name; - } - - return undefined; -} - -/** Throw if any rendition's `broadcast` reference escapes the root. */ -function assertResolvable(base: Moq.Path.Valid, catalog: Catalog.Root): Catalog.Root { - Catalog.checkRenditions(catalog); - const escaping = findEscaping(base, catalog); - if (escaping !== undefined) { - throw new Error(`rendition ${JSON.stringify(escaping)}: broadcast reference escapes the root ${base}`); - } - return catalog; -} - type ReferencedRendition = { broadcast?: Path.Relative; }; @@ -63,7 +21,7 @@ function filterRenditions( return Object.fromEntries(Object.entries(renditions).filter(([, config]) => usable(config.broadcast))); } -// Every section carrying a `broadcast` reference must be listed here, same as `findEscaping`; +// Every section carrying a `broadcast` reference must be listed here, same as `Catalog.checkResolvable`; // one left out silently exempts its tracks from the reachability filter. function filterCatalog(catalog: Catalog.Root, usable: (rel: Path.Relative | undefined) => boolean): Catalog.Root { return { @@ -294,7 +252,7 @@ export class Broadcast { const catalog = effect.get(this.in.catalog); let accepted: Catalog.Root | undefined; try { - accepted = catalog && assertResolvable(name, catalog); + accepted = catalog && Catalog.checkResolvable(Catalog.checkRenditions(catalog), name); } catch (err) { console.error("rejecting catalog", name, err); } @@ -340,7 +298,7 @@ export class Broadcast { console.debug("received catalog", format, this.in.name.peek(), update); - this.#raw.set(assertResolvable(name, update), true); + this.#raw.set(Catalog.checkResolvable(Catalog.checkRenditions(update), name), true); this.#out.status.set("live"); } } catch (err) { From 9048b33be452f008c4b95bdba9b32c803c998d84 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Mon, 28 Sep 2026 17:13:19 -0700 Subject: [PATCH 04/37] fix(net): an origin::Dynamic keeps its origin alive (#4417) Co-authored-by: Claude Opus 5.5 --- doc/lib/go/index.md | 8 ++-- doc/lib/rs/moq-net.md | 6 +++ go/wrapper/moq_test.go | 4 +- go/wrapper/origin.go | 7 ++-- go/wrapper/origin_internal_test.go | 66 ++++++++++++++++++++++++++---- rs/libmoq/src/api.rs | 3 ++ rs/moq-ffi/src/origin.rs | 3 ++ rs/moq-ffi/src/test.rs | 36 +++++++++------- rs/moq-net/src/model/origin.rs | 48 +++++++++++++++++++--- rs/moq-net/src/util.rs | 4 +- 10 files changed, 148 insertions(+), 37 deletions(-) diff --git a/doc/lib/go/index.md b/doc/lib/go/index.md index 3d72c03e8e..2743da61f5 100644 --- a/doc/lib/go/index.md +++ b/doc/lib/go/index.md @@ -87,10 +87,10 @@ Paths with a `.`-prefixed segment below the prefix are [hidden](/concept/moq-lit `Hidden: true`. An `OriginProducer` from `moq.NewOriginProducer` has no `Close`: its origin -ends when the garbage collector reaches the last producer, and every consumer -and `OriginDynamic` made from it then fails with `moq.ErrClosed`. Keep the -producer reachable (a field on a long-lived struct, or `runtime.KeepAlive`) for -as long as the origin should serve. +ends when the garbage collector reaches the last owner (each producer, published +broadcast, and `OriginDynamic`), and every consumer made from it then fails with +`moq.ErrClosed`. Keep an owner reachable (a field on a long-lived struct, or +`runtime.KeepAlive`) for as long as the origin should serve. Every call that can block takes a `context.Context` first. Cancelling it returns `ctx.Err()` promptly and tears the in-flight native work down, so a diff --git a/doc/lib/rs/moq-net.md b/doc/lib/rs/moq-net.md index bd775dbe4b..eb6a5b6aeb 100644 --- a/doc/lib/rs/moq-net.md +++ b/doc/lib/rs/moq-net.md @@ -67,6 +67,12 @@ cleanup time into its returned deadline. A standalone pool needs `gc(now)` called by its owner, at least by the returned deadline; `None` means expiry is disabled. +The origin driver finishes once every owner is gone: each `origin::Producer` +clone, published broadcast, and `origin::Dynamic`. Producer-side children pin +their parent where no cycle exists, so a handler can drop its producer and keep +serving. Read handles (`origin::Consumer`, announce cursors) never pin it: they +see `Closed` once the owners are gone. + Cache activity is dated lazily: reads and writes mark a group active without reading a clock, and the next `gc` pass stamps it with the supplied instant. Expiry is therefore approximate; a late `gc` extends retention. diff --git a/go/wrapper/moq_test.go b/go/wrapper/moq_test.go index 06fef334ef..2c543959fc 100644 --- a/go/wrapper/moq_test.go +++ b/go/wrapper/moq_test.go @@ -19,8 +19,8 @@ import ( const testTimeout = 10 * time.Second // newOrigin returns an origin that lasts the whole test. An OriginProducer has -// no Close: the collector ends its origin once nothing reaches the producer, -// even while consumers and dynamic handles made from it are still in use. +// no Close: the collector ends its origin once nothing reaches an owner, even +// while consumers made from it are still in use. func newOrigin(t *testing.T) *moq.OriginProducer { origin := moq.NewOriginProducer() t.Cleanup(func() { runtime.KeepAlive(origin) }) diff --git a/go/wrapper/origin.go b/go/wrapper/origin.go index 938bbbbda4..5c2de3f047 100644 --- a/go/wrapper/origin.go +++ b/go/wrapper/origin.go @@ -12,9 +12,10 @@ import ( // discover them. Wire one as both a client's/server's publish source and // consume sink for a full-duplex peer. // -// There is no Close: the origin ends once the collector reaches every -// producer, and its consumers and dynamic handles then fail with [ErrClosed]. -// Keep a producer reachable for as long as the origin should live. +// There is no Close: the origin ends once the collector reaches every owner +// (each producer, published broadcast, and [OriginDynamic]), and its consumers +// then fail with [ErrClosed]. Keep an owner reachable for as long as the origin +// should live. type OriginProducer struct { inner *ffi.MoqOriginProducer } diff --git a/go/wrapper/origin_internal_test.go b/go/wrapper/origin_internal_test.go index bd46a0914c..7ae3849a8f 100644 --- a/go/wrapper/origin_internal_test.go +++ b/go/wrapper/origin_internal_test.go @@ -8,13 +8,39 @@ import ( ) // An OriginProducer has no Close: the collector ends its origin by finalizing -// the last producer, even while a consumer or dynamic handle made from it is in -// use. Destroying the handle is what that finalizer does, so this pins the -// behavior the doc comment warns about without waiting on the collector. +// the last owner, even while a consumer made from it is in use. Destroying the +// handle is what that finalizer does, so this pins the behavior the doc comment +// warns about without waiting on the collector. func TestOriginEndsWithItsProducer(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() + origin := NewOriginProducer() + consumer := origin.Consume() + announced, err := consumer.Announced(AnnounceOptions{}) + if err != nil { + t.Fatal(err) + } + defer announced.Cancel() + + origin.inner.Destroy() + + // The teardown runs on the origin's driver; the cursor ending is its signal. + if update, err := announced.Next(ctx); update != nil || err != nil { + t.Fatalf("Next = (%v, %v), want the end of the stream", update, err) + } + if _, err := consumer.RequestBroadcast(ctx, "live"); !errors.Is(err, ErrClosed) { + t.Fatalf("RequestBroadcast err = %v, want ErrClosed", err) + } +} + +// An OriginDynamic owns its origin: once the collector finalizes the last +// OriginProducer, the route still serves and a consumer made earlier still +// resolves through it. +func TestDynamicKeepsTheOrigin(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + origin := NewOriginProducer() dynamic, err := origin.Dynamic("", Route{}) if err != nil { @@ -25,10 +51,36 @@ func TestOriginEndsWithItsProducer(t *testing.T) { origin.inner.Destroy() - if _, err := dynamic.RequestedBroadcast(ctx); !errors.Is(err, ErrClosed) { - t.Fatalf("RequestedBroadcast err = %v, want ErrClosed", err) + type result struct { + broadcast *BroadcastConsumer + err error } - if _, err := consumer.RequestBroadcast(ctx, "live"); !errors.Is(err, ErrClosed) { - t.Fatalf("RequestBroadcast err = %v, want ErrClosed", err) + requested := make(chan result, 1) + go func() { + broadcast, err := consumer.RequestBroadcast(ctx, "live") + requested <- result{broadcast: broadcast, err: err} + }() + + request, err := dynamic.RequestedBroadcast(ctx) + if err != nil { + t.Fatalf("RequestedBroadcast err = %v, want a request", err) + } + served, err := NewBroadcastProducer() + if err != nil { + t.Fatal(err) + } + defer func() { _ = served.Close() }() + if err := request.Accept(served); err != nil { + t.Fatal(err) + } + + var res result + select { + case res = <-requested: + case <-ctx.Done(): + t.Fatal(ctx.Err()) + } + if res.err != nil { + t.Fatalf("RequestBroadcast err = %v, want the served broadcast", res.err) } } diff --git a/rs/libmoq/src/api.rs b/rs/libmoq/src/api.rs index 48cf1e7459..d8c6dbcfda 100644 --- a/rs/libmoq/src/api.rs +++ b/rs/libmoq/src/api.rs @@ -1882,6 +1882,9 @@ pub extern "C" fn moq_origin_request_cancel(task: u32) -> i32 { /// Close an origin and clean up its resources. /// +/// The origin keeps running while a broadcast published on it or a +/// [moq_origin_dynamic] handler lives; close or cancel those to end it. +/// /// Returns a zero on success, or a negative code on failure. #[unsafe(no_mangle)] pub extern "C" fn moq_origin_close(origin: u32) -> i32 { diff --git a/rs/moq-ffi/src/origin.rs b/rs/moq-ffi/src/origin.rs index 253d5a749e..021891d7fb 100644 --- a/rs/moq-ffi/src/origin.rs +++ b/rs/moq-ffi/src/origin.rs @@ -106,6 +106,9 @@ pub struct MoqAnnounceConsumer { #[derive(uniffi::Object)] /// A served route: advertises a path prefix and yields the broadcast requests /// beneath it for the application to accept or reject. +/// +/// Keeps its origin running, like a published broadcast, after every +/// `MoqOriginProducer` is gone. pub struct MoqOriginDynamic { slot: Slot, task: std::sync::Mutex>>>, diff --git a/rs/moq-ffi/src/test.rs b/rs/moq-ffi/src/test.rs index fca53b74b5..6e5fcf0bf5 100644 --- a/rs/moq-ffi/src/test.rs +++ b/rs/moq-ffi/src/test.rs @@ -2534,25 +2534,33 @@ async fn dynamic_serves_a_request_under_a_prefix() { served.close().unwrap(); } -/// Tearing the origin down ends every handler with `Closed`. A parked request -/// keeps the origin's driver alive (its front is lifecycle work the driver -/// drains before resolving), so the teardown never runs underneath one; the -/// case that does happen is a live handler with nothing parked. +/// A dynamic handler keeps its origin alive: dropping (or GC-finalizing) the +/// last `MoqOriginProducer` leaves the route serving, and a consumer made +/// earlier still resolves through it. #[tokio::test] -async fn origin_teardown_closes_dynamic_handlers() { +async fn dynamic_keeps_the_origin_alive() { let origin = MoqOriginProducer::new(MoqOriginConfig::default()); + let consumer = origin.consume(); let dynamic = serve(&origin, ""); - - // The last producer handle: the origin's driver resolves and tears it down. drop(origin); - match tokio::time::timeout(TIMEOUT, dynamic.requested_broadcast()) + + let request_broadcast = { + let consumer = consumer.clone(); + tokio::spawn(async move { consumer.request_broadcast("live".into()).await }) + }; + let request = tokio::time::timeout(TIMEOUT, dynamic.requested_broadcast()) .await - .expect("the handler must observe the teardown") - { - Err(MoqError::Closed) => {} - Err(err) => panic!("unexpected error: {err:?}"), - Ok(_) => panic!("a request was handed out after the teardown"), - } + .expect("the handler must still receive requests") + .unwrap(); + let served = MoqBroadcastProducer::new().unwrap(); + request.accept(&served).unwrap(); + tokio::time::timeout(TIMEOUT, request_broadcast) + .await + .expect("timed out waiting for the request to resolve") + .expect("request task panicked") + .expect("the handler served the path"); + + served.close().unwrap(); } /// Cancelling a handler retracts its route before returning. diff --git a/rs/moq-net/src/model/origin.rs b/rs/moq-net/src/model/origin.rs index c9592defec..d39e9c206f 100644 --- a/rs/moq-net/src/model/origin.rs +++ b/rs/moq-net/src/model/origin.rs @@ -1526,6 +1526,7 @@ impl Producer { Ok(Dynamic { announcement, state: serve, + _keepalive: self.tasks.keepalive(), }) } @@ -1900,9 +1901,12 @@ impl Drop for AnnounceProducer { /// failover, and teardown run here; the route table and announce cursors update /// synchronously when a route is announced or retracted. /// -/// It holds no [`Producer`] clone, so it never keeps the origin alive. Dropping -/// it aborts active fronts, rejects pending requests, ends announcements, and -/// makes subsequent producer mutations fail with [`Error::Closed`]. +/// It holds no [`Producer`] clone, so it never keeps the origin alive. It +/// finishes once every owner is gone: each [`Producer`] clone, published +/// broadcast, and [`Dynamic`]. Read handles ([`Consumer`], [`AnnounceConsumer`]) +/// are not owners. Dropping it aborts active fronts, rejects pending requests, +/// ends announcements, and makes subsequent producer mutations fail with +/// [`Error::Closed`]. /// `moq_tokio::origin::spawn` handles construction and driving for Tokio callers. #[must_use = "poll the driver or the origin makes no progress"] pub struct Driver { @@ -1929,8 +1933,9 @@ impl Driver { /// Process ready origin work using caller-supplied monotonic time. /// /// See [`crate::time::Driver`] for the contract. Finishes with - /// [`Error::Closed`] once every producer handle has dropped and the - /// remaining lifecycle work has drained. + /// [`Error::Closed`] once every owner (a [`Producer`] clone, published + /// broadcast, or [`Dynamic`]) has dropped and the remaining lifecycle work + /// has drained. pub fn poll(&mut self, now: Instant, waiter: &kio::Waiter) -> Result, Error> { self.timers.advance(now); let result = self.state.poll(waiter); @@ -3328,11 +3333,18 @@ struct PendingBroadcast { /// /// Drop it to retract the route and reject the requests still waiting to be /// served; [`update`](Self::update) re-prices it in place. +/// +/// It keeps the origin's [`Driver`] running, like a published broadcast: a +/// handler can drop every [`Producer`] and keep serving. #[must_use = "dropping an origin::Dynamic retracts the route"] pub struct Dynamic { /// The advertisement, retracted on drop. announcement: AnnounceProducer, state: kio::Shared, + /// Producer-side children pin their parent where no cycle exists. The + /// origin's state holds only the [`ServeState`], never this handle, so the + /// driver cannot keep itself alive. + _keepalive: Keepalive, } impl Dynamic { @@ -7464,6 +7476,32 @@ mod tests { assert!(matches!(driver.poll(Instant::now(), &waiter), Err(Error::Closed))); } + /// A handler holding only its `Dynamic` keeps serving once every producer + /// handle drops, and the driver finishes once the handler is gone too. + #[tokio::test] + async fn a_dynamic_keeps_the_driver_running() { + let (producer, driver) = Producer::new(Config::new(origin(1))); + let run = tokio::spawn(crate::time::run(driver)); + let consumer = producer.consume(); + let server = producer.dynamic("room", Route::default()).unwrap(); + drop(producer); + + let pending = consumer.request_broadcast("room/alice"); + let request = queued(&server).await; + let source = broadcast::Info::new().produce(); + request.accept(&source); + let resolved = pending.await.expect("the dynamic still serves"); + + drop(resolved); + drop(source); + drop(server); + tokio::time::timeout(Duration::from_secs(5), run) + .await + .expect("driver must finish once the dynamic is gone") + .unwrap(); + drop(consumer); + } + #[test] fn watch_wakes_only_for_covering_changes() { let producer = origin(1).produce(); diff --git a/rs/moq-net/src/util.rs b/rs/moq-net/src/util.rs index 0d2eab0bab..988a3069ed 100644 --- a/rs/moq-net/src/util.rs +++ b/rs/moq-net/src/util.rs @@ -64,8 +64,8 @@ pub(crate) struct Tasks { } /// A claim on a [`TaskSet`]'s lifetime with no submission queue: what a -/// broadcast published on an origin holds, so the driver outlives the producer -/// handles a session was given and dropped. +/// broadcast published on an origin and an `origin::Dynamic` hold, so the +/// driver outlives the producer handles a session was given and dropped. pub(crate) struct Keepalive { _alive: kio::Producer<()>, } From 0ded2caf126fcac626ba1a51c97014c8feb49849 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Mon, 28 Sep 2026 17:33:56 -0700 Subject: [PATCH 05/37] fix(cli): refuse every MoQ-side flag a verb never reads (#4419) Co-authored-by: Claude Opus 5.5 --- doc/bin/cli.md | 6 +- quest/m1/README.md | 1 - quest/m1/cli-given-flags.md | 23 ---- rs/moq-cli/src/args.rs | 239 ++++++++++++++++++++---------------- 4 files changed, 133 insertions(+), 136 deletions(-) delete mode 100644 quest/m1/cli-given-flags.md diff --git a/doc/bin/cli.md b/doc/bin/cli.md index c9371c78ab..d641191e07 100644 --- a/doc/bin/cli.md +++ b/doc/bin/cli.md @@ -200,9 +200,9 @@ zero-based `frame` and padded standard base64. `` is the literal track name. `/fetch` splits its path on the last `/`, so the two agree only for names without one. Fetch only dials `--connect`, and -refuses a listener or cluster flag. It gives up after 30 seconds, as `/fetch` -does, and exits non-zero when the broadcast or group is not found (before -writing anything), the relay refuses, or the deadline passes. +refuses any listener, cluster, auth, or `--hop` flag. It gives up after 30 +seconds, as `/fetch` does, and exits non-zero when the broadcast or group is not +found (before writing anything), the relay refuses, or the deadline passes. ## Multiple stages diff --git a/quest/m1/README.md b/quest/m1/README.md index be62e0315b..f7a6cf5bf6 100644 --- a/quest/m1/README.md +++ b/quest/m1/README.md @@ -53,7 +53,6 @@ transport, benchmark tooling); worktrees isolate commits, not semantics. - [UnknownSession log flood](/quest/m1/unknown-session-logs.md) - streams reset before their WebTransport header stop being reported as UnknownSession at WARN - [Merge queue](/quest/m1/merge-queue.md) - the required checks run on `merge_group`, so a stale green check can no longer break main - [Wire compatibility](/quest/m1/wire-compat.md) - a nightly run tests this checkout against the last published release for tokens, session wire, and catalog/container -- [Accept-side flags](/quest/m1/cli-given-flags.md) - dial-only and local verbs refuse every `--listen-*` flag instead of ignoring it - [#2075](/quest/m1/2075-mirror-catalog-reservation-gating-in-moq-hang-js-hang.md) - @moq/publish gates the first catalog snapshot until every reserved track is described - [Full codec string](/quest/m1/publish-codec-string.md) - browser-published video carries the encoder's full RFC 6381 codec string, so native players decode it - [TS program selection](/quest/m1/ts-programs.md) - `import ts` refuses a multi-program stream unless `--program ` picks one or publishes each diff --git a/quest/m1/cli-given-flags.md b/quest/m1/cli-given-flags.md deleted file mode 100644 index 1bb750ba4e..0000000000 --- a/quest/m1/cli-given-flags.md +++ /dev/null @@ -1,23 +0,0 @@ -# [XS] Dial-only and local verbs refuse every accept-side flag - -## Goal - -`moq fetch`, `moq ls` (#4032, on `dev`), and the local verbs behind `Invocation::reject` refuse any listener -flag they would never use, as they already do for `--listen` and the cluster -flags. Today `MoqSide::given()` in `rs/moq-cli/src/args.rs` lists only some -of them, so `--listen-version`, `--listen-tls-*`, `--listen-preferred-*`, and -`--listen-quic-lb-*` are silently ignored. Codex found it on -[#4121](https://github.com/moq-dev/moq/pull/4121). - -## Plan - -- Cover every accept-side flag. Prefer a list that cannot fall behind the - server config, such as deriving it from the parsed partial or a test that - walks every `--listen-*` flag clap knows and asserts `given()` reports it, - over extending the hand-written array again. -- While there, check whether the local verbs also ignore `--connect-*` - flags; `reject` is meant to refuse the whole MoQ side. -- Test that each verb refuses a representative flag from every family, - naming it. - -Public API: none (CLI refuses input it used to ignore). Wire: none. diff --git a/rs/moq-cli/src/args.rs b/rs/moq-cli/src/args.rs index 8c3d26d1b9..dd3249bb24 100644 --- a/rs/moq-cli/src/args.rs +++ b/rs/moq-cli/src/args.rs @@ -84,13 +84,15 @@ pub struct Invocation { /// The MoQ attachment, shared by every stage. pub moq: MoqSide, - /// The same attachment, built without consulting the environment. + /// The MoQ-side flags the command line typed, each once, in order. /// - /// Only [`MoqSide::reject`] reads it. A local verb refuses a MoQ side the user - /// asked for, and an exported `MOQ_CONNECT` is not an ask: it is a standing - /// setting for the publishing this shell usually does, and it would otherwise - /// make `moq auth` and `moq completion` fail for everyone who has one. - pub typed: MoqSide, + /// Only [`Self::reject`] and [`Self::dial_only`] read it. A verb refuses a MoQ + /// side the user asked for, and an exported `MOQ_CONNECT` is not an ask: it is a + /// standing setting for the publishing this shell usually does, and it would + /// otherwise make `moq auth` and `moq completion` fail for everyone who has one. + /// Read from the parse itself rather than from the built fields, so a flag added + /// to any flattened config is refused without anyone listing it here. + given: Vec<&'static usage::Flag<'static>>, /// The stages, in the order given. Never empty. pub stages: Vec, @@ -173,18 +175,50 @@ impl Invocation { } } - /// Refuse a MoQ side on a verb that runs locally and takes none. + /// Refuse every MoQ-side flag on a verb that runs locally and takes none, rather + /// than silently ignoring it. /// - /// Answered from what the command line said, never from the environment; see - /// [`Self::typed`] and `MoqSide::reject`. + /// `--broadcast` counts: a local verb has no content, and next to `auth generate` + /// it reads like it scopes the key, which `--root` does. Answered from what the + /// command line said, never from the environment; see [`Self::given`]. pub fn reject(&self, command: &str) -> anyhow::Result<()> { - self.typed.reject(command) + if let Some(flags) = Self::names(self.given.iter()) { + anyhow::bail!("`{command}` runs locally and takes no MoQ side; drop {flags}"); + } + Ok(()) } - /// Refuse every MoQ-side flag but `--connect` and `--broadcast`, on a verb that - /// only reads from a relay. Answered from the command line, like [`Self::reject`]. + /// Refuse every MoQ-side flag but the dial, on a verb that only reads from a + /// relay: a listener, cluster, or auth policy it would never serve is not + /// silently ignored. The dial is `--connect*`, the `--quic-*` and `--iroh-*` + /// settings it dials with, and `--broadcast`. Answered from the command line, + /// like [`Self::reject`]. pub fn dial_only(&self, command: &str) -> anyhow::Result<()> { - self.typed.dial_only(command) + use usage::spec::CommandArgs; + + fn owns(flag: &usage::Flag<'_>) -> bool { + T::COMMAND.flags.iter().any(|own| own.key == flag.key) + } + let dials = |flag: &&&usage::Flag<'_>| { + #[cfg(feature = "iroh")] + if owns::(flag) { + return true; + } + owns::(flag) + || owns::(flag) + || flag.longs.contains(&"broadcast") + }; + + if let Some(flags) = Self::names(self.given.iter().filter(|flag| !dials(flag))) { + anyhow::bail!("`{command}` only dials a relay with --connect; drop {flags}"); + } + Ok(()) + } + + /// `--a, --b` for the flags given, or `None` when there are none. + fn names<'a>(flags: impl Iterator>) -> Option { + let names: Vec = flags.map(|flag| format!("--{}", flag.longs[0])).collect(); + (!names.is_empty()).then(|| names.join(", ")) } /// Split `argv` on `--` and run each chunk through a real parser. @@ -201,7 +235,7 @@ impl Invocation { let first = chunks.next().unwrap_or_default(); let first = first.iter().skip(1).map(OsString::as_os_str).collect::>(); let cli = Cli::parse_from(&first).map_err(|err| parse_error(Cli::spec(), Cli::command(), &first, err))?; - let typed = MoqSide::from_argv(&first, Environment::Ignore).unwrap_or_else(|| cli.moq.clone()); + let given = MoqSide::given(&first); let mut deprecated = cli.moq.deprecated(); deprecated.extend(cli.command.deprecated()); @@ -238,7 +272,7 @@ impl Invocation { Ok(Self { log: cli.log, moq: cli.moq, - typed, + given, stages, }) } @@ -476,10 +510,9 @@ impl MoqSide { /// Build a [`MoqSide`] from one chunk of a command line, leniently. /// - /// Stops at the first thing the grammar cannot take, because the two callers are - /// both looking at an incomplete line: a half-typed one being completed, and (via - /// [`Environment::Ignore`]) a real one whose typed values are being separated from - /// its ambient ones. Whatever was understood before that point is the answer. + /// Stops at the first thing the grammar cannot take, because the caller is looking + /// at a half-typed line being completed. Whatever was understood before that point + /// is the answer. pub(crate) fn from_argv(argv: &[&OsStr], environment: Environment) -> Option { use usage::spec::CommandArgs; @@ -501,85 +534,28 @@ impl MoqSide { ::build(partial).ok() } - /// Reject the MoQ flags on a verb that never touches the network, rather than - /// silently ignoring them. `--broadcast` counts: a local verb has no content, and - /// next to `auth generate` it reads like it scopes the key, which `--root` does. + /// The MoQ-side flags one chunk of a command line typed, each once, in order. /// - /// Private, and reached only through [`Invocation::reject`], so it cannot be asked - /// of the resolved side: every one of these flags has a `MOQ_*` variable, and a - /// shell that exports one for the publishing it usually does has not asked this - /// verb for anything. A call site that picked the wrong view would read correctly - /// and be wrong, so there is only one view to pick. `--hop` is in the list for - /// the same reason it used to be out of it -- an ambient `MOQ_HOP` no longer - /// reaches here, so a typed one can be refused like the rest. - fn reject(&self, command: &str) -> anyhow::Result<()> { - if let Some(flag) = self.given().next() { - anyhow::bail!("`{command}` runs locally and takes no MoQ side; drop {flag}"); - } - Ok(()) - } + /// A flattened config's flags sit in this struct's table under the keys that + /// config minted, so the table is the complete list. The chunk already parsed, + /// so nothing stops the walk early. + fn given(argv: &[&OsStr]) -> Vec<&'static usage::Flag<'static>> { + use usage::spec::CommandArgs; - /// Refuse every MoQ-side flag except the dial, on a verb that only reads from a - /// relay: a listener or cluster it would never serve is not silently ignored. - /// Private for the same reason as [`Self::reject`], and reached through - /// [`Invocation::dial_only`]. - fn dial_only(&self, command: &str) -> anyhow::Result<()> { - if let Some(flag) = self.given().find(|flag| !matches!(*flag, "--connect" | "--broadcast")) { - anyhow::bail!("`{command}` only dials a relay with --connect; drop {flag}"); + let mut given: Vec<&'static usage::Flag<'static>> = Vec::new(); + let mut parser = usage::Parser::new(Cli::command(), argv); + while let Some(Ok(event)) = parser.next_event() { + if let usage::Event::Flag { flag, .. } = event + && ::COMMAND + .flags + .iter() + .any(|own| own.key == flag.key) + && !given.iter().any(|seen| seen.key == flag.key) + { + given.push(flag); + } } - Ok(()) - } - - /// The MoQ-side flags this side was given. - fn given(&self) -> impl Iterator { - #[cfg(feature = "cluster-lan")] - let cluster_secret = self.cluster.lan.secret.is_some(); - #[cfg(not(feature = "cluster-lan"))] - let cluster_secret = false; - #[cfg(feature = "cluster-lan")] - let cluster_app = self.cluster.lan.app.is_some(); - #[cfg(not(feature = "cluster-lan"))] - let cluster_app = false; - - // A legacy `--client-connect` must be rejected here too; the fold has already - // landed it in `url`. - let flags = [ - ("--connect", self.client.url.is_some()), - ("--listen", self.server.bind.is_some()), - ("--listen-tcp-bind", self.server.tcp.bind.is_some()), - ("--cluster-lan", self.lan()), - ("--cluster-lan-secret", cluster_secret), - ("--cluster-lan-app", cluster_app), - ("--cluster-connect", !self.cluster.connect.is_empty()), - ("--cluster-connect-api", self.cluster.connect_api.is_some()), - ("--cluster-node", self.cluster.node.is_some()), - ("--cluster-mesh", self.cluster.mesh.is_some()), - ("--cluster-token", self.cluster.token.is_some()), - ("--cluster-id", self.cluster.id.is_some()), - ("--cluster-tier", self.cluster.tier.is_some()), - ("--auth-url", self.auth.url.is_some()), - ("--auth-public", self.auth_public()), - ("--broadcast", self.broadcast.is_some()), - ("--hop", self.hop.is_some()), - ]; - #[cfg(unix)] - let unix = { - let allow = &self.server.unix.allow; - [ - ("--listen-unix-bind", self.server.unix.bind.is_some()), - ("--listen-unix-allow-uid", !allow.uid.is_empty()), - ("--listen-unix-allow-gid", !allow.gid.is_empty()), - ("--listen-unix-allow-pid", !allow.pid.is_empty()), - ] - }; - #[cfg(not(unix))] - let unix = []; - - flags - .into_iter() - .chain(unix) - .filter(|(_, given)| *given) - .map(|(flag, _)| flag) + given } } @@ -1184,13 +1160,7 @@ mod tests { // A local verb refuses the flag like every other MoQ-side flag. let cli = Invocation::try_parse_from(["moq", "--auth-public", "**", "auth", "generate"]).expect("parse"); - assert!( - cli.moq - .reject("auth") - .unwrap_err() - .to_string() - .contains("--auth-public") - ); + assert!(cli.reject("auth").unwrap_err().to_string().contains("--auth-public")); } /// The grammar Usage can't express: one connection, several endpoints. @@ -1488,7 +1458,7 @@ mod tests { assert!(matches!(cli.stages[0], Command::Auth(_))); // Local verb: it needs no MoQ side, so what every other verb demands... assert!(cli.moq.validate().is_err()); - assert!(cli.moq.reject("auth").is_ok()); + assert!(cli.reject("auth").is_ok()); // ...these it refuses, rather than accepting the flag and ignoring it. for (flag, value, reported) in [ @@ -1507,12 +1477,12 @@ mod tests { ("--cluster-tier", "internal", "--cluster-tier"), ] { let cli = Invocation::try_parse_from(["moq", flag, value, "auth", "generate"]).unwrap(); - let err = cli.moq.reject("auth").unwrap_err().to_string(); + let err = cli.reject("auth").unwrap_err().to_string(); assert!(err.contains(reported), "{err}"); } let cli = Invocation::try_parse_from(["moq", "--cluster-mesh", "auth", "generate"]).unwrap(); - let err = cli.moq.reject("auth").unwrap_err().to_string(); + let err = cli.reject("auth").unwrap_err().to_string(); assert!(err.contains("--cluster-mesh"), "{err}"); #[cfg(unix)] @@ -1524,7 +1494,7 @@ mod tests { ("--listen-unix-allow-pid", "1000", "--listen-unix-allow-pid"), ] { let cli = Invocation::try_parse_from(["moq", flag, value, "auth", "generate"]).unwrap(); - let err = cli.moq.reject("auth").unwrap_err().to_string(); + let err = cli.reject("auth").unwrap_err().to_string(); assert!(err.contains(reported), "{err}"); } } @@ -1532,7 +1502,7 @@ mod tests { #[cfg(feature = "cluster-lan")] { let cli = Invocation::try_parse_from(["moq", "--cluster-lan", "auth", "generate"]).unwrap(); - let err = cli.moq.reject("auth").unwrap_err().to_string(); + let err = cli.reject("auth").unwrap_err().to_string(); assert!(err.contains("--cluster-lan"), "{err}"); // The parser considers the secret's `requires` satisfied when the boolean flag @@ -1547,7 +1517,7 @@ mod tests { "generate", ]) .unwrap(); - let err = cli.moq.reject("auth").unwrap_err().to_string(); + let err = cli.reject("auth").unwrap_err().to_string(); assert!(err.contains("--cluster-lan-secret"), "{err}"); let cli = Invocation::try_parse_from([ @@ -1559,11 +1529,62 @@ mod tests { "generate", ]) .unwrap(); - let err = cli.moq.reject("auth").unwrap_err().to_string(); + let err = cli.reject("auth").unwrap_err().to_string(); assert!(err.contains("--cluster-lan-app"), "{err}"); } } + /// Each verb refuses a flag from every MoQ-side family it never reads, naming it. + /// The listener and dial families had members the old hand-written list missed. + #[test] + fn every_unused_family_is_refused() { + let accept: &[&[&str]] = &[ + &["--listen", "[::]:0"], + &["--listen-version", "moq-lite-02"], + &["--listen-tls-generate", "localhost"], + &["--listen-preferred-v4", "127.0.0.1:443"], + &["--listen-quic-lb-id", "01"], + &["--listen-tcp-bind", "127.0.0.1:0"], + &["--cluster-node", "https://self.example"], + &["--auth-public", "**"], + &["--hop", "1"], + ]; + let dial: &[&[&str]] = &[ + &["--connect", "https://relay.example"], + &["--connect-tls-insecure"], + &["--backoff-initial", "2s"], + &["--quic-idle-timeout", "10s"], + #[cfg(feature = "iroh")] + &["--iroh-enabled"], + &["--broadcast", "room"], + ]; + + let parse = |flag: &[&str], verb: &[&str]| { + let argv = ["moq"].iter().chain(flag).chain(verb).copied(); + Invocation::try_parse_from(argv).unwrap_or_else(|err| panic!("{flag:?}: {err}")) + }; + + for verb in [&["auth", "generate"][..], &["completion", "bash"]] { + for flag in accept.iter().chain(dial) { + let err = parse(flag, verb).reject(verb[0]).unwrap_err().to_string(); + assert!(err.contains(flag[0]), "{verb:?} {flag:?}: {err}"); + } + } + + for flag in accept { + let err = parse(flag, &["fetch", "data"]) + .dial_only("fetch") + .unwrap_err() + .to_string(); + assert!(err.contains(flag[0]), "{flag:?}: {err}"); + } + for flag in dial { + parse(flag, &["fetch", "data"]) + .dial_only("fetch") + .unwrap_or_else(|err| panic!("{flag:?}: {err}")); + } + } + /// `--cluster-connect` / `--cluster-connect-api` attach the process as a /// cluster peer, so they are a MoQ side on their own. `--cluster-node` is /// identity, not an attachment. From de4db075d85ff45272d31166cdee67d9d58a754d Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Mon, 28 Sep 2026 17:48:18 -0700 Subject: [PATCH 06/37] fix(net): skip announce updates the peer cannot tell apart (#4423) Co-authored-by: Claude Opus 5.5 --- js/net/src/ietf/publisher.test.ts | 28 ++++++++++++ js/net/src/ietf/publisher.ts | 28 ++++++++---- js/net/src/lite/publisher.test.ts | 38 ++++++++++++++++ js/net/src/lite/publisher.ts | 9 +++- quest/m0/README.md | 1 - quest/m0/announce-update-dedupe.md | 32 -------------- quest/m1/cluster-routing.md | 1 - rs/moq-net/src/lite/announce.rs | 6 +-- rs/moq-net/src/lite/publisher.rs | 71 +++++++++++++++++++++++++----- rs/moq-net/src/lite/subscriber.rs | 10 ++--- 10 files changed, 159 insertions(+), 65 deletions(-) delete mode 100644 quest/m0/announce-update-dedupe.md diff --git a/js/net/src/ietf/publisher.test.ts b/js/net/src/ietf/publisher.test.ts index 76c0c59d36..55dea4e9a5 100644 --- a/js/net/src/ietf/publisher.test.ts +++ b/js/net/src/ietf/publisher.test.ts @@ -970,6 +970,34 @@ test("an advertisement carries the announced route and re-prices in place", asyn origin.close(); }); +/** + * MoQ Cluster carries the warm cost only, and nothing at all without it, so a route change + * the peer cannot see must not withdraw and advertise the namespace again. + */ +test.each([ + ["a cold-only re-price with Cluster", HopSchema.parse(9n)], + ["any re-price without Cluster", undefined], +])("%s sends nothing", async (_, peer) => { + const self: Hop = HopSchema.parse(7n); + const pair = createMockTransportPair(ALPN.DRAFT_19); + const { pub, origin } = publisher(pair.server, { cluster: { self, peer } }); + const broadcast = origin.createBroadcast(Path.from("mine")); + broadcast.announce({ cost: { warm: 4n, cold: 4n } }); + void pub.runPublishNamespaces(); + + const stream = await nextStream(pair.client); + if (!stream) throw new Error("no PUBLISH_NAMESPACE for the broadcast"); + expect(await stream.reader.u53()).toBe(PublishNamespace.id); + const msg = await PublishNamespace.decode(stream.reader, VERSION, peer !== undefined); + expect(msg.trackNamespace).toBe(Path.from("mine")); + await acceptPublishNamespace(stream); + + broadcast.announce({ cost: { warm: peer === undefined ? 8n : 4n, cold: 9n } }); + expect(await nextStream(pair.client)).toBeUndefined(); + + origin.close(); +}); + test("subscription completion sends PUBLISH_DONE on every supported draft", async () => { const versions = [ Version.DRAFT_14, diff --git a/js/net/src/ietf/publisher.ts b/js/net/src/ietf/publisher.ts index ee12f52118..a4f1988ead 100644 --- a/js/net/src/ietf/publisher.ts +++ b/js/net/src/ietf/publisher.ts @@ -2,7 +2,7 @@ import { type Dispose, type Getter, race, Signal } from "@moq/signals"; import type * as broadcast from "../broadcast.ts"; import { controlTimeout, error, reason, StreamCode, StreamError } from "../error.ts"; import type * as group from "../group.ts"; -import { type Route, routesEqual } from "../hop.ts"; +import type { Route } from "../hop.ts"; import { hiddenBelow, hooks, presented } from "../internal.ts"; import type { Consumer as OriginConsumer } from "../origin.ts"; import * as Path from "../path.ts"; @@ -48,8 +48,17 @@ function clusterFor(base: Cluster.Advert | undefined, route: Route): Cluster.Adv return { hops, cost: route.cost.warm }; } -function sameAdvert(a: Advertised | undefined, b: Advertised | undefined): boolean { - return a !== undefined && b !== undefined && a.identity === b.identity && routesEqual(a.route, b.route); +/** + * Whether the peer would decode the same advertisement: the same broadcast and, with MoQ + * Cluster, the same hop chain and warm cost. The cold cost, and the whole route without + * Cluster, never reach the wire, so a change there must not restart the namespace. + */ +function sameAdvert(base: Cluster.Advert | undefined, a: Advertised | undefined, b: Advertised | undefined): boolean { + if (a === undefined || b === undefined || a.identity !== b.identity) return false; + const x = clusterFor(base, a.route); + const y = clusterFor(base, b.route); + if (x === undefined || y === undefined) return x === y; + return x.cost === y.cost && x.hops.length === y.hops.length && x.hops.every((hop, i) => hop === y.hops[i]); } /** @@ -812,12 +821,12 @@ export class Publisher { // (a restart). Identity change is a new broadcast; a route change is the // same one at a new cost or hop chain. for (const [removed, snap] of active) { - if (sameAdvert(updated.get(removed), snap)) continue; + if (sameAdvert(this.#advert, updated.get(removed), snap)) continue; await withdraw(removed); held.delete(removed); } for (const [added, snap] of updated) { - if (sameAdvert(held.get(added), snap)) continue; + if (sameAdvert(this.#advert, held.get(added), snap)) continue; if (!this.#offerable(Path.join(prefix, added), refused)) continue; offered.set(Path.join(prefix, added), snap.identity); if (await advertise(added, snap)) held.set(added, snap); @@ -830,7 +839,8 @@ export class Publisher { // starting to answer raises a signal this loop is watching. const outstanding = [...updated].some( ([suffix, snap]) => - !sameAdvert(active.get(suffix), snap) && this.#pending(Path.join(prefix, suffix), refused), + !sameAdvert(this.#advert, active.get(suffix), snap) && + this.#pending(Path.join(prefix, suffix), refused), ); retry = outstanding ? Math.min(retry ? retry * 2 : RETRY_BASE, RETRY_MAX) : 0; @@ -935,11 +945,11 @@ export class Publisher { // Withdraw first so a republish or re-price reads as withdraw-then-advertise // (a restart) rather than nothing. for (const [removed, snap] of active) { - if (sameAdvert(updated.get(removed), snap)) continue; + if (sameAdvert(this.#advert, updated.get(removed), snap)) continue; await this.#withdraw(removed, requests); } for (const [added, snap] of updated) { - if (sameAdvert(active.get(added), snap)) continue; + if (sameAdvert(this.#advert, active.get(added), snap)) continue; if (!this.#offerable(added, refused)) continue; offered.set(added, snap.identity); await this.#advertise(added, requests, refused, clusterFor(this.#advert, snap.route)); @@ -958,7 +968,7 @@ export class Publisher { // transient failure clearing, or the peer starting to answer raises no // signal of its own, so the only way back is to ask again on a timer. const outstanding = [...updated].some( - ([path, snap]) => !sameAdvert(active.get(path), snap) && this.#pending(path, refused), + ([path, snap]) => !sameAdvert(this.#advert, active.get(path), snap) && this.#pending(path, refused), ); retry = outstanding ? Math.min(retry ? retry * 2 : RETRY_BASE, RETRY_MAX) : 0; diff --git a/js/net/src/lite/publisher.test.ts b/js/net/src/lite/publisher.test.ts index 2d08823181..2466166cd2 100644 --- a/js/net/src/lite/publisher.test.ts +++ b/js/net/src/lite/publisher.test.ts @@ -84,6 +84,44 @@ test.each([Version.DRAFT_01, Version.DRAFT_03, Version.DRAFT_06])( }, ); +test.each([ + [Version.DRAFT_05, false], + [Version.DRAFT_06, true], +])("a re-price goes out only where the wire carries cost (version %s)", async (version, sent) => { + const pair = createMockTransportPair(ALPN_05); + const origin = new OriginProducer(); + const publisher = new Publisher(pair.server, version, randomHop(), origin.consume()); + const broadcast = origin.createBroadcast(Path.from("cam")); + broadcast.announce({ cost: 7n }); + + const written: Uint8Array[] = []; + const stream = new Stream({ + readable: new ReadableStream(), + writable: new WritableStream({ + write(chunk) { + written.push(chunk); + }, + }), + }); + const settle = () => new Promise((resolve) => setTimeout(resolve, 10)); + const running = publisher.runAnnounce(new AnnounceRequest(Path.empty()), stream); + await settle(); + const initial = written.length; + expect(initial).toBeGreaterThan(0); + + broadcast.announce({ cost: 9n }); + await settle(); + expect(written.length > initial).toBe(sent); + + stream.close(); + await running; + publisher.close(); + broadcast.close(); + origin.close(); + pair.client.close(); + pair.server.close(); +}); + // Delivers `sequences` in the given order, finishes the track, and returns the // SUBSCRIBE_END the publisher put on the wire. async function subscribeEnd(sequences: number[], version: Version = Version.DRAFT_05): Promise { diff --git a/js/net/src/lite/publisher.ts b/js/net/src/lite/publisher.ts index ed2a854dd8..7ed9622222 100644 --- a/js/net/src/lite/publisher.ts +++ b/js/net/src/lite/publisher.ts @@ -2,7 +2,7 @@ import { type Dispose, type Getter, race, Signal } from "@moq/signals"; import type * as broadcast from "../broadcast.ts"; import { error, NotFound, reason, StreamCode, StreamError } from "../error.ts"; import type * as group from "../group.ts"; -import { type Hop, type Route, routesEqual } from "../hop.ts"; +import { Cost, type Hop, type Route, routesEqual } from "../hop.ts"; import { hiddenBelow, hooks, presented } from "../internal.ts"; import type { Consumer as OriginConsumer } from "../origin.ts"; import type * as Path from "../path.ts"; @@ -32,6 +32,7 @@ import { hasAnnounceOk, hasDatagrams, hasProbeRtt, + hasRouteCost, hasStreamCount, resolvesStart, Version, @@ -400,6 +401,10 @@ export class Publisher { return [...route.hops, this.hop]; }; + // What the peer decodes for a route: pre-lite-06 wires carry no cost, so a re-price + // there must not restart. + const onWire = (route: Route): Route => (hasRouteCost(this.version) ? route : { ...route, cost: Cost.zero }); + const announce = async (suffix: Path.Valid, route: Route) => { console.debug(`announce: broadcast=${suffix} active=true`); if (hasAnnounceId(this.version)) announceIds.set(suffix, nextAnnounceId++); @@ -518,7 +523,7 @@ export class Publisher { const prev = active.get(suffix); if (!prev || prev.identity !== snap.identity) { await announce(suffix, snap.route); - } else if (!routesEqual(prev.route, snap.route)) { + } else if (!routesEqual(onWire(prev.route), onWire(snap.route))) { await restart(suffix, snap.route); } } diff --git a/quest/m0/README.md b/quest/m0/README.md index 935edeae68..bfc968f761 100644 --- a/quest/m0/README.md +++ b/quest/m0/README.md @@ -31,7 +31,6 @@ Published API or wire breaks still land on dev; each quest's Plan says so. ## Required - [Packaged relay starts](/quest/m0/relay-systemd-unit.md) - the `.deb` and `.rpm` relay service starts instead of crash-looping on `--file` -- [Skip unchanged announce updates](/quest/m0/announce-update-dedupe.md) - a publisher sends an announce update only when the wire route changed - [Wildcard](/quest/m0/wildcard/README.md) - a relay resolves subscriptions against advertised prefixes, a service claims the prefix it could serve and refuses the rest instead of enumerating broadcasts, and the browser player treats a covering claim as availability - [Audio quality harness](/quest/m0/audio-quality-harness/README.md) - a browser playout latency regression fails a nightly run instead of arriving as a bug report, and its recorder supplies the jitter target's replay traces - [Audio jitter target](/quest/m0/audio-jitter-target/README.md) - the audio playout target is a measured estimate of arrival timing in both languages, not a round-trip guess diff --git a/quest/m0/announce-update-dedupe.md b/quest/m0/announce-update-dedupe.md deleted file mode 100644 index c428466a89..0000000000 --- a/quest/m0/announce-update-dedupe.md +++ /dev/null @@ -1,32 +0,0 @@ -# [S] Skip unchanged announce updates - -## Goal - -A publisher sends an announce update only when what the peer would decode -differs from what it last sent for that announcement. A local change the wire -cannot express (the route's source session, `served`, captures) sends nothing. -This applies on every version, lite and IETF, in Rust and JS, and it is wire -compatible. - -## Plan - -Each announce cursor dedupes its best route on `(hops, cost, source)` plus -`served` and captures (`rs/moq-net/src/model/origin.rs`, the `Updated` kind), -but the lite publisher only remembers each suffix's Announce ID -(`rs/moq-net/src/lite/publisher.rs`, the `self.live` branch) and re-sends -`Restart` for every `Updated` it sees. Only the hops and cost reach the wire, -so a flip of any other field sends an identical update, to every peer, for -every covered broadcast. This comes from reading the code, not from a test. - -- Reproduce first: a test that flips a route's source session with the same - hops and cost, and asserts the peer receives no update. -- Keep the last-sent `(hops, cost)` beside the Announce ID and skip equal - updates. Check the IETF publisher's re-pricing path and the JS publisher for - the same pattern. -- Fix the stale comments on the way: `lite/announce.rs` says restarts are only - ever received, and the `restart_announce` doc in `lite/subscriber.rs` says it - compares the first hop. - -## Related - -- [Cluster routing](/quest/m1/cluster-routing.md) - removes the other big source of updates, reroutes that change only the hop chain diff --git a/quest/m1/cluster-routing.md b/quest/m1/cluster-routing.md index 3d52174867..c93f1f1df5 100644 --- a/quest/m1/cluster-routing.md +++ b/quest/m1/cluster-routing.md @@ -119,7 +119,6 @@ make MoQ's common case. ## Related -- [Skip unchanged announce updates](/quest/m0/announce-update-dedupe.md) - cuts duplicate updates on today's routing - [Redundant ingest](/quest/m2/redundant-ingest.md) - builds on the `--hop` failover this must keep or replace - [Routing cost domains](/quest/m2/routing-cost-domains.md) - cost across the cluster boundaries this keeps path vector - [Cross-relay delivery under bursts](/quest/m1/cross-relay-bursts.md) - its #4349 report also shows closed broadcasts announced for up to 229 s and flapping between Retracted and Announced across nodes, evidence for per-incarnation seqnos diff --git a/rs/moq-net/src/lite/announce.rs b/rs/moq-net/src/lite/announce.rs index e54733e4f7..9e460d1530 100644 --- a/rs/moq-net/src/lite/announce.rs +++ b/rs/moq-net/src/lite/announce.rs @@ -355,8 +355,8 @@ impl AnnounceBroadcast<'_> { AnnounceStatus::Ended => Self::Ended { suffix, hops }, // On lite-05 a restart travels as a duplicate ANNOUNCE (a second `Active`), so accept // the draft's explicit `restart` status and treat it the same. Either way the - // subscriber retires an already-announced path before republishing it; for an unknown - // path it's a fresh announce. Older versions never defined this status, so it's an + // subscriber re-prices an already-announced path in place; for an unknown path it's a + // fresh announce. Older versions never defined this status, so it's an // invalid value there. AnnounceStatus::Restart if restart_supported(version) => Self::Active { suffix: PathRef::literal(suffix), @@ -431,7 +431,7 @@ enum AnnounceStatus { Ended = 0, Active = 1, /// The explicit restart status, accepted on decode for forward/cross-compatibility. We never - /// encode it: a replacement goes out as an `Ended` + `Active` pair. + /// encode it: a lite-05 restart goes out as a duplicate `Active`. Restart = 2, } diff --git a/rs/moq-net/src/lite/publisher.rs b/rs/moq-net/src/lite/publisher.rs index 995a50578d..baf7880b1b 100644 --- a/rs/moq-net/src/lite/publisher.rs +++ b/rs/moq-net/src/lite/publisher.rs @@ -585,11 +585,21 @@ struct AnnounceRun { // were never seen by the peer). Lite07 also picks compression bases here. encoder: lite::AnnounceEncoder, // The routes the peer currently holds, keyed by the suffix under the requested - // prefix. The value is the announce id on versions that assign them. - live: HashMap>, + // prefix. + live: HashMap, phase: AnnouncePhase, } +/// What the peer holds for one advertised suffix. +struct Advertised { + /// The announce id, on versions that assign them. + id: Option, + /// The chain and cost last put on the wire. The origin also reports changes the + /// wire cannot carry (the route's source, servability), which must not restart. + hops: Hops, + cost: crate::origin::Cost, +} + enum AnnouncePhase { /// The version-specific initial burst has not been sent yet. Init, @@ -646,11 +656,11 @@ impl AnnounceRun { hops: Hops, cost: crate::origin::Cost, ) -> Result<(), Error> { - let (id, wire, hops) = self.encoder.start(suffix.clone(), hops); - self.live.insert(suffix, id); + let (id, wire, chain) = self.encoder.start(suffix.clone(), hops.clone()); + self.live.insert(suffix, Advertised { id, hops, cost }); stream.writer.buffer(&lite::AnnounceBroadcast::Active { suffix: wire, - hops, + hops: chain, cost, })?; Ok(()) @@ -663,12 +673,12 @@ impl AnnounceRun { suffix: crate::PathOwned, absolute: &crate::Path, ) -> Result<(), Error> { - let Some(id) = self.live.remove(&suffix) else { + let Some(advertised) = self.live.remove(&suffix) else { // Filtered on the way out; the peer never saw it. return Ok(()); }; tracing::debug!(route = %absolute, "unannounce"); - match id { + match advertised.id { Some(id) => { self.encoder.end(id); stream.writer.buffer(&lite::AnnounceBroadcast::EndedId { id })? @@ -809,12 +819,16 @@ impl AnnounceRun { } match self.outgoing(&update.route, &absolute) { - Some((hops, cost)) => match self.live.get(&suffix) { + Some((hops, cost)) => match self.live.get_mut(&suffix) { + // The peer would decode what it already holds. + Some(advertised) if advertised.hops == hops && advertised.cost == cost => {} // A metadata update on a live advertisement: restart it in // place (lite-05 restarts via a duplicate ANNOUNCE). - Some(&id) if lite::restart_supported(self.version) => { + Some(advertised) if lite::restart_supported(self.version) => { tracing::debug!(route = %absolute, "reannounce"); - match id { + advertised.hops = hops.clone(); + advertised.cost = cost; + match advertised.id { Some(id) => { let hops = self.encoder.update(id, hops); stream @@ -1823,6 +1837,43 @@ mod announce_test { h.assert_idle(); } + /// A new best route the wire cannot tell apart (another session, same chain and + /// cost) sends nothing, in either direction. + #[tokio::test(start_paused = true)] + async fn source_flip_is_quiet() { + let h = harness().await; + let peer = h + .origin + .clone() + .peer() + .announce( + "cam", + crate::origin::Route::default().with_hops(pub_hops()).with_cost(7), + ) + .unwrap(); + settle().await; + h.assert_idle(); + + drop(peer); + settle().await; + h.assert_idle(); + } + + /// Costs past the wire ceiling clamp to the same value, so moving between them + /// sends nothing. + #[tokio::test(start_paused = true)] + async fn clamped_cost_change_is_quiet() { + let mut h = harness().await; + let route = |cost| crate::origin::Route::default().with_hops(pub_hops()).with_cost(cost); + h.announcement.update(route(u64::MAX)).unwrap(); + settle().await; + assert_eq!(h.wire.take_announces().len(), 1, "expected the clamped restart"); + + h.announcement.update(route(u64::MAX - 1)).unwrap(); + settle().await; + h.assert_idle(); + } + /// A route whose chain contains the excluded peer is invisible to that peer's /// announce stream (control-plane split horizon via the cursor). #[tokio::test(start_paused = true)] diff --git a/rs/moq-net/src/lite/subscriber.rs b/rs/moq-net/src/lite/subscriber.rs index 68b2e34e2f..c6944a8e49 100644 --- a/rs/moq-net/src/lite/subscriber.rs +++ b/rs/moq-net/src/lite/subscriber.rs @@ -368,13 +368,9 @@ impl Subscriber { /// Handle a RESTART (an explicit restart status, or a duplicate ANNOUNCE on lite-05). /// - /// The first hop of the chain identifies the original publisher. When it matches - /// the prior advertisement and is a real identity, the broadcast is the same - /// content on a new path: this session's route metadata updates in place, - /// in-flight tracks keep flowing, and the origin only hands over if the winner - /// changed. Consumers observe nothing. When the first hop differs, or is - /// [`Hop::UNKNOWN`](crate::Hop::UNKNOWN), the old route detaches gracefully - /// and a fresh one attaches, so downstream sees a real Ended + Active. + /// A restart carries no content claim, so this session's route re-prices in + /// place whatever the new chain says: in-flight tracks keep flowing and the + /// origin only hands over if the winner changed. /// The advertisement is already live, so this can attach a route even when the /// original advertisement was declined locally. /// From 138a051213adf6f8d71fcd8a2bcffd9a20360ad0 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Mon, 28 Sep 2026 17:49:29 -0700 Subject: [PATCH 07/37] fix: stop logging a stream reset before its header as UnknownSession (#4420) Co-authored-by: Claude Opus 5.5 --- quest/m1/unknown-session-logs.md | 49 +++++++++++++++++--------------- 1 file changed, 26 insertions(+), 23 deletions(-) diff --git a/quest/m1/unknown-session-logs.md b/quest/m1/unknown-session-logs.md index f8285c35a4..b1db43f89a 100644 --- a/quest/m1/unknown-session-logs.md +++ b/quest/m1/unknown-session-logs.md @@ -13,29 +13,32 @@ as `UnknownSession`, and the error a caller sees says what happened. Seen in production at a rate like the old-group warnings https://github.com/moq-dev/moq/pull/4208 fixed, on the same hosts. -Likely root cause, to confirm with a test first: `decode_uni` (and -`decode_bi`) in `web-transport-moq`'s `session.rs` (repository -`moq-dev/noq`) map every failure to read the stream type or session ID to -`UnknownSession`. A read fails whenever the peer resets the stream before -those bytes arrive, which moq does routinely: a publisher resets a group's -stream when the group is superseded or expires, and without reliable reset -the header can be discarded with it. `poll_accept_uni` then logs it at WARN, -though its own comment says the stream "was probably reset early". - -- Reproduce in `web-transport-moq`'s tests: open a WebTransport uni stream, - reset it before the header is delivered, and assert the accept loop - reports a reset rather than `UnknownSession`. -- Fix the mapping at the source: carry the read's real cause (reset, closed, - truncated), keep `UnknownSession` for a session-ID mismatch, and log the - expected reset at debug. Not a log filter on the moq side. -- `web-transport-quinn` in `moq-dev/web-transport` carries the same code; - fix it too if it is still published. -- Release and bump the pin here. Confirm on a moq.pro relay that the rate - drops, and that any remaining `UnknownSession` lines are real mismatches. - -If the reproduction shows something other than early resets (for example a -peer really using another session ID), stop and report before changing the -log level. +Root cause, confirmed by a test: `decode_uni` and `decode_bi` in +`web-transport-moq`'s `session.rs` map every failure to read the stream type +or session ID to `UnknownSession`, and a peer reset before those bytes +arrive is one. moq resets a group's stream when the group is superseded or +expires, and without reliable reset the header is discarded with it. + +The fix keeps the read's cause as `WebTransportError::ReadError`, keeps +`UnknownSession` for a session-ID mismatch, and logs a reset or lost +connection at debug: + +- [moq-dev/noq#19](https://github.com/moq-dev/noq/pull/19) for 2.x (`dev`) + and its backport [moq-dev/noq#20](https://github.com/moq-dev/noq/pull/20) + for 1.3.x (`main`). +- [moq-dev/web-transport#405](https://github.com/moq-dev/web-transport/pull/405) + for `web-transport-quinn`, `web-transport-noq`, and `web-transport-iroh`, + which carry the same code. `main` pins `web-transport-iroh` 0.7, so it + gets the iroh fix when `dev` merges. + +Remaining: bump `web-transport-moq` to the 1.3.3 release on `main` (and +2.0.1 on `dev` if it lands first). Then confirm on a moq.pro relay that the +rate drops, and that any remaining `UnknownSession` lines are real +mismatches. + +## Required + +- `web-transport-moq` 1.3.3 released from moq-dev/noq#20 ## Related From 9537167487817a892ccd9ce9cd66efaca285c378 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Mon, 28 Sep 2026 18:22:09 -0700 Subject: [PATCH 08/37] doc(uring): noq paces inside poll_transmit, not ignored (#4400) Co-authored-by: Claude Opus 5.5 --- rs/moq-uring/src/quic/noq/connection.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/rs/moq-uring/src/quic/noq/connection.rs b/rs/moq-uring/src/quic/noq/connection.rs index 721ed5a3ce..88828d989a 100644 --- a/rs/moq-uring/src/quic/noq/connection.rs +++ b/rs/moq-uring/src/quic/noq/connection.rs @@ -808,8 +808,11 @@ impl Driver { Poll::Pending } - /// Fill one transmit buffer and stage it. Ignores noq's pacing hint; - /// the congestion controller still bounds each train. + /// Fill one transmit buffer and stage it. + /// + /// Pacing happens inside `poll_transmit`: a paced path returns nothing + /// and arms a timer that `poll_timeout` reports, and the driver loop + /// flushes again once it fires. fn flush_one(&mut self, waiter: &kio::Waiter) -> Poll> { let mut tx = match self.socket.poll_acquire(waiter) { Poll::Ready(Ok(tx)) => tx, @@ -839,7 +842,7 @@ impl Driver { .poll_transmit(Instant::now(), segments, &mut self.scratch) { Some(transmit) => transmit, - // Nothing to send; the buffer returns to the pool on drop. + // Nothing to send, or paced; the buffer returns to the pool on drop. None => return Poll::Pending, }; From 4e9c9ed31868db468ebf0978544128f952f0afc1 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Mon, 28 Sep 2026 18:23:50 -0700 Subject: [PATCH 09/37] test: prove stopped relays and worker groups closed their sockets instead of racing a rebind (#4408) Co-authored-by: Claude Opus 5.5 --- Cargo.lock | 2 + rs/moq-relay/Cargo.toml | 5 ++ rs/moq-relay/tests/embed.rs | 105 +++++++++++++++++++++------ rs/moq-tokio/Cargo.toml | 5 ++ rs/moq-tokio/tests/worker.rs | 133 ++++++++++++++++++++++++----------- 5 files changed, 189 insertions(+), 61 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index db0d69c052..27c0076dfc 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4635,6 +4635,7 @@ dependencies = [ "serde", "serde_json", "serde_with", + "socket2 0.6.5", "sysinfo", "tempfile", "thiserror 2.0.21", @@ -4809,6 +4810,7 @@ dependencies = [ "serde", "serde_with", "sha2", + "socket2 0.6.5", "subtle", "tempfile", "thiserror 2.0.21", diff --git a/rs/moq-relay/Cargo.toml b/rs/moq-relay/Cargo.toml index 8d7e7edece..14b5c5c567 100644 --- a/rs/moq-relay/Cargo.toml +++ b/rs/moq-relay/Cargo.toml @@ -97,6 +97,11 @@ rcgen = "0.14" tempfile = { workspace = true } tokio = { workspace = true, features = ["test-util"] } +# Which sockets the process still holds, so the embed tests can prove a stopped +# relay closed its own without rebinding a port another test could take. +[target.'cfg(target_os = "linux")'.dev-dependencies] +socket2 = { workspace = true, features = ["all"] } + # `raise(SIGINT)` at ourselves, so the shutdown test exercises the real signal # path rather than the trigger behind it. [target.'cfg(unix)'.dev-dependencies] diff --git a/rs/moq-relay/tests/embed.rs b/rs/moq-relay/tests/embed.rs index bd8b4bfbb8..4d72931d50 100644 --- a/rs/moq-relay/tests/embed.rs +++ b/rs/moq-relay/tests/embed.rs @@ -5,7 +5,7 @@ //! public API. Each runtime layout (shared Tokio, worker Tokio, Linux //! io_uring) mounts a custom HTTP route, clones the cluster origin, serves a //! live QUIC subscriber, then stops the owner through its shutdown trigger -//! and proves `run` returned with the ports free. +//! and proves `run` returned with its sockets closed. #![cfg(feature = "_quic")] @@ -57,6 +57,74 @@ async fn assert_owner_stopped(quic: SocketAddr, http: SocketAddr) { ); } +/// The socket inodes this process holds bound to `port`: every UDP socket, but +/// only a TCP listener, since accepted connections are not the owner's to close. +/// +/// Asked of this process's own descriptors, for two reasons. Rebinding the port +/// races any concurrent process's ephemeral bind, which may take a freed port. +/// And `/proc/net/udp` is served in chunks that skip entries while other +/// processes churn sockets. +#[cfg(target_os = "linux")] +fn bound(port: u16) -> std::collections::HashSet { + use std::os::fd::FromRawFd; + use std::os::unix::fs::{FileTypeExt, MetadataExt}; + + let mut bound = std::collections::HashSet::new(); + for entry in std::fs::read_dir("/proc/self/fd").expect("read /proc/self/fd") { + let entry = entry.expect("read /proc/self/fd"); + let Some(fd) = entry.file_name().to_str().and_then(|name| name.parse().ok()) else { + continue; + }; + // Duplicated rather than borrowed: another thread may close the original + // meanwhile, which fails the duplicate instead of pulling the descriptor + // out from under a borrow, and the copy pins the socket it names. + let dup = unsafe { libc::fcntl(fd, libc::F_DUPFD_CLOEXEC, 0) }; + if dup < 0 { + continue; + } + // SAFETY: `dup` is a fresh descriptor that nothing else owns. + let file = std::fs::File::from(unsafe { std::os::fd::OwnedFd::from_raw_fd(dup) }); + let Ok(meta) = file.metadata() else { continue }; + if !meta.file_type().is_socket() { + continue; + } + let socket = socket2::SockRef::from(&file); + let owned = match socket.r#type() { + Ok(socket2::Type::DGRAM) => true, + Ok(socket2::Type::STREAM) => socket.is_listener().unwrap_or(false), + _ => false, + }; + let on_port = socket + .local_addr() + .ok() + .and_then(|addr| addr.as_socket()) + .map(|addr| addr.port()) + == Some(port); + if owned && on_port { + bound.insert(meta.ino()); + } + } + bound +} + +/// The inode of the socket at a `/proc/self/fd` entry, or `None` for anything else. +#[cfg(target_os = "linux")] +fn socket_inode(path: &std::path::Path) -> Option { + use std::os::unix::fs::{FileTypeExt, MetadataExt}; + + let meta = std::fs::metadata(path).ok()?; + meta.file_type().is_socket().then(|| meta.ino()) +} + +/// The inodes of every socket this process has open. +#[cfg(target_os = "linux")] +fn open_sockets() -> std::collections::HashSet { + std::fs::read_dir("/proc/self/fd") + .expect("read /proc/self/fd") + .filter_map(|entry| socket_inode(&entry.ok()?.path())) + .collect() +} + /// Fire the embedder stop and wait for `run` to return: the join every /// worker thread and listener goes through, as opposed to aborting the task. async fn stop(trigger: moq_relay::shutdown::Trigger, running: tokio::task::JoinHandle>) { @@ -69,15 +137,14 @@ async fn stop(trigger: moq_relay::shutdown::Trigger, running: tokio::task::JoinH } /// Load, mount a custom route, clone the origin, run the owner, prove QUIC -/// plus HTTP, then stop it through the trigger and rebind both ports with a -/// replacement owner. +/// plus HTTP, then stop it through the trigger and prove its sockets closed. async fn embed_and_stop(mut config: Config) { let _ = rustls::crypto::aws_lc_rs::default_provider().install_default(); // No drain window: the sessions are already gone by the time the owner // stops, and the test should not wait out the default. config.drain_timeout = Duration::ZERO; - let relay = Relay::load(config.clone()).await.expect("load relay"); + let relay = Relay::load(config).await.expect("load relay"); let quic = relay.quic_addr().expect("quic listener bound"); let http = relay.web_addrs().http.expect("http listener bound"); assert_eq!( @@ -85,9 +152,6 @@ async fn embed_and_stop(mut config: Config) { Some(moq_tokio::quic::DEFAULT_MAX_STREAMS) ); assert_eq!(relay.cluster().id(), relay.cluster().origin.hop().id()); - // Pin the replacement to the same ports the `:0` first binds got. - config.listen.bind = Some(moq_tokio::listen::Bind::Addr(quic)); - config.web.http.listen = Some(http); // The application handles: in-process workers publish into the origin the // QUIC sessions see, and the trigger stops the owner from any task. Both @@ -245,22 +309,21 @@ async fn embed_and_stop(mut config: Config) { drop(broadcast); drop(subscriber); + #[cfg(target_os = "linux")] + let owned = { + let quic_sockets = bound(quic.port()); + let http_sockets = bound(http.port()); + assert!(!quic_sockets.is_empty(), "no QUIC socket found on {quic}"); + assert!(!http_sockets.is_empty(), "no HTTP listener found on {http}"); + &quic_sockets | &http_sockets + }; + stop(trigger, running).await; - assert_owner_stopped(quic, http).await; - // A replacement owner can bind the same ports, so the workers joined. - let replacement = Relay::load(config).await.expect("rebind after stop"); - assert_eq!(replacement.addr(), Some(quic), "replacement bound a different address"); - let trigger = replacement.shutdown_trigger().clone(); - let replacing = tokio::spawn(replacement.run()); - let health = reqwest::get(format!("http://127.0.0.1:{}/health", http.port())) - .await - .expect("replacement health") - .text() - .await - .expect("replacement health body"); - assert!(!health.is_empty(), "replacement HTTP did not serve"); - stop(trigger, replacing).await; + // Every worker and listener joined before `run` returned, so none of the + // owner's sockets are still open. + #[cfg(target_os = "linux")] + assert!(owned.is_disjoint(&open_sockets()), "the owner's sockets outlived `run`"); assert_owner_stopped(quic, http).await; } diff --git a/rs/moq-tokio/Cargo.toml b/rs/moq-tokio/Cargo.toml index 0e3a459dc2..7e15142f96 100644 --- a/rs/moq-tokio/Cargo.toml +++ b/rs/moq-tokio/Cargo.toml @@ -120,3 +120,8 @@ tokio = { workspace = true, features = ["test-util"] } tokio-rustls = { workspace = true } toml = "1.1" tracing-test = { workspace = true, features = ["no-env-filter"] } + +# Which sockets the process still holds, so the worker tests can prove a stopped +# group closed its own without rebinding a port another test could take. +[target.'cfg(target_os = "linux")'.dev-dependencies] +socket2 = { workspace = true, features = ["all"] } diff --git a/rs/moq-tokio/tests/worker.rs b/rs/moq-tokio/tests/worker.rs index 70b727d005..00e7b7c8a6 100644 --- a/rs/moq-tokio/tests/worker.rs +++ b/rs/moq-tokio/tests/worker.rs @@ -22,6 +22,72 @@ fn free_udp_port() -> u16 { port } +/// The socket inodes this process holds bound to `port`, of which there must be +/// at least one: every UDP socket, but only a TCP listener. +/// +/// Asked of this process's own descriptors, for two reasons. Rebinding the port +/// races any concurrent process's ephemeral bind, which may take a freed port. +/// And `/proc/net/udp` is served in chunks that skip entries while other +/// processes churn sockets. +fn bound(port: u16) -> std::collections::HashSet { + use std::os::fd::FromRawFd; + use std::os::unix::fs::{FileTypeExt, MetadataExt}; + + let mut bound = std::collections::HashSet::new(); + for entry in std::fs::read_dir("/proc/self/fd").expect("read /proc/self/fd") { + let entry = entry.expect("read /proc/self/fd"); + let Some(fd) = entry.file_name().to_str().and_then(|name| name.parse().ok()) else { + continue; + }; + // Duplicated rather than borrowed: another thread may close the original + // meanwhile, which fails the duplicate instead of pulling the descriptor + // out from under a borrow, and the copy pins the socket it names. + let dup = unsafe { libc::fcntl(fd, libc::F_DUPFD_CLOEXEC, 0) }; + if dup < 0 { + continue; + } + // SAFETY: `dup` is a fresh descriptor that nothing else owns. + let file = std::fs::File::from(unsafe { std::os::fd::OwnedFd::from_raw_fd(dup) }); + let Ok(meta) = file.metadata() else { continue }; + if !meta.file_type().is_socket() { + continue; + } + let socket = socket2::SockRef::from(&file); + let owned = match socket.r#type() { + Ok(socket2::Type::DGRAM) => true, + Ok(socket2::Type::STREAM) => socket.is_listener().unwrap_or(false), + _ => false, + }; + let on_port = socket + .local_addr() + .ok() + .and_then(|addr| addr.as_socket()) + .map(|addr| addr.port()) + == Some(port); + if owned && on_port { + bound.insert(meta.ino()); + } + } + assert!(!bound.is_empty(), "this process holds no socket on port {port}"); + bound +} + +/// The inode of the socket at a `/proc/self/fd` entry, or `None` for anything else. +fn socket_inode(path: &std::path::Path) -> Option { + use std::os::unix::fs::{FileTypeExt, MetadataExt}; + + let meta = std::fs::metadata(path).ok()?; + meta.file_type().is_socket().then(|| meta.ino()) +} + +/// The inodes of every socket this process has open. +fn open_sockets() -> std::collections::HashSet { + std::fs::read_dir("/proc/self/fd") + .expect("read /proc/self/fd") + .filter_map(|entry| socket_inode(&entry.ok()?.path())) + .collect() +} + /// A self-signed certificate on disk. Workers refuse `tls.generate`, since each /// would generate one of its own and serve a different identity. fn certificate(dir: &std::path::Path) -> (std::path::PathBuf, std::path::PathBuf) { @@ -84,11 +150,10 @@ async fn dropping_the_workers_releases_the_port() { std::future::pending::<()>().await; }); } + let owned = bound(addr.port()); group.shutdown().await; - // A plain bind refuses a port any socket still holds, reuseport or not, so this - // succeeds only if every worker's socket is really gone. - moq_tokio::bind::udp(moq_tokio::bind::Udp::new(addr)).expect("workers left the port bound"); + assert!(owned.is_disjoint(&open_sockets()), "workers left the port bound"); } /// The future factory runs on the worker, so the future may hold local state @@ -227,9 +292,10 @@ async fn dropping_unserved_workers_releases_the_port() { let workers = bind_workers(listen_config(&cert, &key, 0), Default::default(), config(WORKERS)).expect("bind workers"); let addr = workers.local_addr(); + let owned = bound(addr.port()); drop(workers); - moq_tokio::bind::udp(moq_tokio::bind::Udp::new(addr)).expect("workers left the port bound"); + assert!(owned.is_disjoint(&open_sockets()), "workers left the port bound"); } /// The group holds one port, so an ephemeral bind is the port its first member @@ -247,13 +313,14 @@ async fn an_ephemeral_port_is_shared_by_the_group() { assert_eq!(workers.len(), usize::from(WORKERS)); let addr = workers.local_addr(); + let owned = bound(addr.port()); assert_ne!(addr.port(), 0, "the group reports the port it bound"); // A plain bind refuses a port any socket holds, so this fails while the // group is alive and succeeds once every member has let go. moq_tokio::bind::udp(moq_tokio::bind::Udp::new(addr)).expect_err("the group must hold its port"); workers.shutdown().await; - moq_tokio::bind::udp(moq_tokio::bind::Udp::new(addr)).expect("the group must release its port"); + assert!(owned.is_disjoint(&open_sockets()), "the group must release its port"); } /// `SO_REUSEPORT` groups by address and UID, so a second group on a served @@ -404,31 +471,6 @@ async fn generated_certificates_are_refused() { ); } -/// How many UDP sockets this process holds on `port`, via procfs. -/// -/// The group retains every reuseport socket until serving has stopped, so -/// dropping a server handle must not change this count while the group is -/// alive: without the retainer the kernel would close the socket and renumber -/// every member after it. A backend may own more than one descriptor per -/// member, so the invariant is the stable count rather than its exact value. -#[cfg(target_os = "linux")] -fn udp_sockets_on(port: u16) -> usize { - let want = format!(":{port:04X}"); - let mut count = 0; - if let Ok(table) = std::fs::read_to_string("/proc/net/udp") { - for line in table.lines().skip(1) { - let mut fields = line.split_whitespace(); - // sl, local_address, rem_address, ...: the second field is the bind. - if let Some(local) = fields.nth(1) - && local.to_ascii_uppercase().ends_with(&want) - { - count += 1; - } - } - } - count -} - /// Dropping a server handle cannot take its socket out of the reuseport group. /// /// The group retains every socket until serving has stopped, so the survivors @@ -445,8 +487,8 @@ async fn dropping_a_server_keeps_its_socket() { let workers = bind_workers(listen_config(&cert, &key, 0), Default::default(), config(2)).expect("bind workers"); let port = workers.local_addr().port(); let mut group = workers.split(); - let sockets = udp_sockets_on(port); - assert!(sockets >= 2, "every member holds at least one socket"); + let sockets = bound(port); + assert!(sockets.len() >= 2, "every member holds at least one socket"); assert_eq!(group.local_addr().port(), port); let mut members = group.members(); @@ -457,7 +499,7 @@ async fn dropping_a_server_keeps_its_socket() { drop(dropped); assert_eq!( - udp_sockets_on(port), + bound(port), sockets, "dropping a server must not lose its socket while the group lives" ); @@ -468,10 +510,13 @@ async fn dropping_a_server_keeps_its_socket() { let member = members.pop().expect("one member"); assert_eq!(member.index(), 0); drop(member); - assert_eq!(udp_sockets_on(port), sockets, "the retainer outlives both handles"); + assert_eq!(bound(port), sockets, "the retainer outlives both handles"); group.shutdown().await; - assert_eq!(udp_sockets_on(port), 0, "stopping the group releases every socket"); + assert!( + sockets.is_disjoint(&open_sockets()), + "stopping the group releases every socket" + ); } /// Completing one serving member ends serving for the whole group. @@ -486,6 +531,7 @@ async fn completing_a_member_stops_its_siblings() { let (cert, key) = certificate(dir.path()); let workers = bind_workers(listen_config(&cert, &key, 0), Default::default(), config(2)).expect("bind workers"); let addr = workers.local_addr(); + let owned = bound(addr.port()); let mut group = workers.split(); let mut members = group.members(); assert_eq!(members.len(), 2); @@ -523,7 +569,7 @@ async fn completing_a_member_stops_its_siblings() { .expect("a stopped sibling must not hang"); group.shutdown().await; - moq_tokio::bind::udp(moq_tokio::bind::Udp::new(addr)).expect("the group must release its port"); + assert!(owned.is_disjoint(&open_sockets()), "the group must release its port"); } /// Cancelling one serving member ends serving for the whole group. @@ -535,6 +581,7 @@ async fn cancelling_a_member_stops_its_siblings() { let (cert, key) = certificate(dir.path()); let workers = bind_workers(listen_config(&cert, &key, 0), Default::default(), config(2)).expect("bind workers"); let addr = workers.local_addr(); + let owned = bound(addr.port()); let mut group = workers.split(); let mut members = group.members(); @@ -566,7 +613,7 @@ async fn cancelling_a_member_stops_its_siblings() { .expect("a cancelled sibling must not hang"); group.shutdown().await; - moq_tokio::bind::udp(moq_tokio::bind::Udp::new(addr)).expect("the group must release its port"); + assert!(owned.is_disjoint(&open_sockets()), "the group must release its port"); } /// A panicking serving member ends serving for the whole group, and the panic @@ -579,6 +626,7 @@ async fn a_panicking_member_stops_its_siblings() { let (cert, key) = certificate(dir.path()); let workers = bind_workers(listen_config(&cert, &key, 0), Default::default(), config(2)).expect("bind workers"); let addr = workers.local_addr(); + let owned = bound(addr.port()); let mut group = workers.split(); let mut members = group.members(); @@ -610,7 +658,7 @@ async fn a_panicking_member_stops_its_siblings() { .expect("a panicking sibling must not hang"); group.shutdown().await; - moq_tokio::bind::udp(moq_tokio::bind::Udp::new(addr)).expect("the group must release its port"); + assert!(owned.is_disjoint(&open_sockets()), "the group must release its port"); } /// Explicit shutdown with work in flight stops every worker and joins. @@ -622,6 +670,7 @@ async fn shutdown_with_work_in_flight_joins() { let (cert, key) = certificate(dir.path()); let workers = bind_workers(listen_config(&cert, &key, 0), Default::default(), config(2)).expect("bind workers"); let addr = workers.local_addr(); + let owned = bound(addr.port()); let mut group = workers.split(); let mut tasks = Vec::new(); for member in group.members() { @@ -638,7 +687,7 @@ async fn shutdown_with_work_in_flight_joins() { .expect("shutdown must stop every member"); } - moq_tokio::bind::udp(moq_tokio::bind::Udp::new(addr)).expect("shutdown must release the port"); + assert!(owned.is_disjoint(&open_sockets()), "shutdown must release the port"); } /// Dropping the owner with work in flight stops every worker without joining @@ -651,6 +700,7 @@ async fn dropping_the_group_with_work_in_flight_stops() { let (cert, key) = certificate(dir.path()); let workers = bind_workers(listen_config(&cert, &key, 0), Default::default(), config(2)).expect("bind workers"); let addr = workers.local_addr(); + let owned = bound(addr.port()); let mut group = workers.split(); let mut tasks = Vec::new(); { @@ -670,5 +720,8 @@ async fn dropping_the_group_with_work_in_flight_stops() { .expect("dropping the group must stop every member"); } - moq_tokio::bind::udp(moq_tokio::bind::Udp::new(addr)).expect("dropping the group must release the port"); + assert!( + owned.is_disjoint(&open_sockets()), + "dropping the group must release the port" + ); } From ea148fe7fcb9828559b0a7dad2e3e56ab7984e2d Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Mon, 28 Sep 2026 18:28:52 -0700 Subject: [PATCH 10/37] feat(net): drain queued stream data before a graceful close (#4430) Co-authored-by: Claude Opus 5.5 --- doc/lib/rs/index.md | 4 + doc/lib/rs/moq-net.md | 11 +- quest/m1/README.md | 1 - quest/m1/drain-before-close.md | 40 ------- quest/m1/session-close.md | 14 +-- quest/m2/README.md | 1 + quest/m2/ietf-drain-before-close.md | 18 ++++ rs/moq-cli/src/main.rs | 43 ++++++-- rs/moq-cli/tests/import.rs | 98 +++++++++++++++++ rs/moq-net/src/driver.rs | 32 ++++-- rs/moq-net/src/lite/publisher.rs | 30 +++++- rs/moq-net/src/lite/session.rs | 5 + rs/moq-net/src/session.rs | 162 +++++++++++++++++++++++----- rs/moq-net/tests/session_close.rs | 157 +++++++++++++++++++++++++++ rs/moq-tokio/src/client.rs | 3 + rs/moq-tokio/src/connection.rs | 23 +++- rs/moq-tokio/tests/backend.rs | 112 +++++++++++++++++++ 17 files changed, 659 insertions(+), 95 deletions(-) delete mode 100644 quest/m1/drain-before-close.md create mode 100644 quest/m2/ietf-drain-before-close.md create mode 100644 rs/moq-cli/tests/import.rs create mode 100644 rs/moq-net/tests/session_close.rs diff --git a/doc/lib/rs/index.md b/doc/lib/rs/index.md index 2a02438809..ac11e13127 100644 --- a/doc/lib/rs/index.md +++ b/doc/lib/rs/index.md @@ -74,6 +74,10 @@ let mut broadcast = origin.publish("my-stream.hang", Default::default())?; // each requested path for the application to accept or reject. ``` +Before exiting, `session.close().await` delivers what was already queued, such as +the tracks you just finished, within one second. Then `Client::close` (on a clone of +the client) sends the QUIC close before the runtime stops. + The examples run the session and the origin work concurrently (`tokio::select!` or `spawn`), since the announcement loop is live. Runnable examples: [`rs/hang/examples/video.rs`](https://github.com/moq-dev/moq/blob/main/rs/hang/examples/video.rs) diff --git a/doc/lib/rs/moq-net.md b/doc/lib/rs/moq-net.md index eb6a5b6aeb..910211481f 100644 --- a/doc/lib/rs/moq-net.md +++ b/doc/lib/rs/moq-net.md @@ -57,9 +57,14 @@ on external activity, `Ok(None)` only on external activity, and `Err` is the terminal error (`Error::Closed` for a clean finish): stop polling. Tests drive the same interface with explicitly advanced instants. -Dropping the last session handle requests closure on the next poll. Dropping -the driver cancels the session. `moq-tokio` and `moq-wasm` drive sessions for -their callers. +Dropping the last session handle requests closure on the next poll, and +`session.abort(err)` closes with `err`'s code. Either discards stream data the +peer has not acknowledged yet. `session.close().await` first waits, up to one +second, for finished tracks to deliver their last groups and FIN, returning +`Error::Timeout` if it gave up. Finish or abort live tracks before calling it. +moq-transport (IETF) sessions close without waiting. Dropping the driver +cancels the session. `moq-tokio` and `moq-wasm` drive sessions for their +callers. `origin::Producer::new` returns a driver with the same `time::Driver` interface. It calls `cache::Pool::gc(now)` after each poll and folds the next diff --git a/quest/m1/README.md b/quest/m1/README.md index f7a6cf5bf6..9b9e5995ef 100644 --- a/quest/m1/README.md +++ b/quest/m1/README.md @@ -27,7 +27,6 @@ transport, benchmark tooling); worktrees isolate commits, not semantics. - [Track demand](/quest/m1/track-demand.md) - Rust and JS watch a track's subscribers through `demand()` alone - [Error messages](/quest/m1/error-display.md) - Python, Go, and Dart print `MoqError` with Rust's message, as Kotlin and Swift do - [Session close](/quest/m1/session-close.md) - a graceful session end withdraws announces and waits one second for the ack -- [Drain before close](/quest/m1/drain-before-close.md) - a closing client delivers its queued stream finishes, so `moq import` ends the catalog cleanly over a real relay - [Close codes](/quest/m1/close-codes.md) - a client sees the peer's application close code over WebSocket and raw QUIC, like WebTransport - [Raw stream codes](/quest/m1/raw-stream-codes.md) - raw QUIC stream resets and stops carry the application's code, not an HTTP/3-mapped one - [Live in apps](/quest/m1/announce-live-apps.md) - the demo and `@moq/room` show "no broadcasts" from the `live` marker, which waits for the first session on page load diff --git a/quest/m1/drain-before-close.md b/quest/m1/drain-before-close.md deleted file mode 100644 index f24ff82b57..0000000000 --- a/quest/m1/drain-before-close.md +++ /dev/null @@ -1,40 +0,0 @@ -# [M] Deliver queued stream data before a client closes - -## Goal - -A process that finishes its tracks and then closes its `moq_tokio::Client` -delivers what it already queued, including each stream's FIN, before the -connection closes, bounded by a deadline. `moq import` at stdin EOF is the -consumer: a subscriber over a real relay sees the catalog finish instead of -`Error::Dropped`. - -## Plan - -- [#4303](https://github.com/moq-dev/moq/pull/4303) made `moq import` finish - the catalog at EOF, but only in-process: over a real session the process - exits and the close discards the queued finish. - [#4287](https://github.com/moq-dev/moq/pull/4287) added `Client::close`, - which sends the CONNECTION_CLOSE but does not wait for stream data. -- A QUIC close discards unacknowledged stream data, so the session has to - wait until its open send streams are written, finished, and acknowledged, - not only handed to the transport. The pending group and control writes live - in moq-net's session tasks; the transport wait lives in moq-tokio. -- Bound the wait so a stalled peer cannot hang an exiting process. Share the - deadline and the graceful path with - [Session close](/quest/m1/session-close.md), which waits for announce - acknowledgements the same way; `abort` and drop stay immediate. -- Cover every backend `Client::close` covers, and say which do not drain - (WebSocket and iroh end on drop today). -- Regression tests: a moq-tokio test, shaped like - `noq_client_close_reaches_server`, where the client finishes a track and - closes on a runtime dropped right after, and the server's subscriber reads - the finish rather than a drop. Then a CLI test piping a file through - `moq import` to a relay. - -Public API: likely additive (`Client::close` gains the drain, or a graceful -close sits beside `abort`). Wire: none. - -## Related - -- [Session close](/quest/m1/session-close.md) - the graceful end that withdraws announces -- [Graceful relay drains](/quest/m1/drain/README.md) - the server-side drain over GOAWAY diff --git a/quest/m1/session-close.md b/quest/m1/session-close.md index 499c51e678..1ac4d9d92e 100644 --- a/quest/m1/session-close.md +++ b/quest/m1/session-close.md @@ -9,12 +9,14 @@ last handle, still end the session immediately and do not wait. ## Plan Rust has `abort` and drop, and both end the session now. Drop sends a bare -`Cancel`, so `PUBLISH_NAMESPACE_DONE` never goes out. Add -`moq_tokio::Connection::close()`. It unannounces what this session published, -waits up to one second for the acknowledgements, then ends the session. If -the peer has not answered by then, the session still ends and `close()` -returns that timeout. There is no existing request-ack timer to reuse; this -one second is its own constant. +`Cancel`, so `PUBLISH_NAMESPACE_DONE` never goes out. +`moq_net::Session::close()` and `moq_tokio::Connection::close()` already +exist: they wait up to `CLOSE_TIMEOUT` (one second, in `moq-net`'s +`session.rs`) for finished tracks to deliver, then end the session, returning +`Error::Timeout` if the peer was too slow. Extend that drain phase to +unannounce what this session published and wait for the acknowledgements, +under the same deadline. Only moq-lite drains today; IETF sessions close at +once. JS `Established.close()` is synchronous today. It becomes the same graceful end and returns a promise, which is a published break, so that change targets diff --git a/quest/m2/README.md b/quest/m2/README.md index ff38a25780..c52acf6f4d 100644 --- a/quest/m2/README.md +++ b/quest/m2/README.md @@ -20,6 +20,7 @@ upstream release waits in [m4](/quest/m4/README.md). - [Catalog track identity](/quest/m2/catalog-tracks.md) - compare immutable track definitions with explicit catalog-to-group binding - [Archive recovery listing](/quest/m2/archive-recovery-listing.md) - a resumed DVR lists what changed since its checkpoint, not every stored group - [Archive backward timestamps](/quest/m2/archive-backward-timestamps.md) - a resumed recording refuses a track whose timestamps go backward +- [IETF drain before close](/quest/m2/ietf-drain-before-close.md) - moq-transport sessions deliver finished tracks before a graceful close, as moq-lite does - [moq play drain tail](/quest/m2/play-drain-tail.md) - retired renditions and finite tracks play their last 10 ms of audio - [Relay io_uring packages](/quest/m2/relay-io-uring-package.md) - Linux relay packages ship io_uring once the ring is on par with tokio - [Mobile ownership](/quest/m2/mobile-ownership.md) - decide whether Rust or platform code owns mobile capture, codecs, and rendering diff --git a/quest/m2/ietf-drain-before-close.md b/quest/m2/ietf-drain-before-close.md new file mode 100644 index 0000000000..e5c034130d --- /dev/null +++ b/quest/m2/ietf-drain-before-close.md @@ -0,0 +1,18 @@ +# [S] IETF sessions drain before close + +## Goal + +`Session::close` on a moq-transport (IETF) session waits, like moq-lite, for +finished tracks to deliver their last groups and FIN before closing, within +the shared one second `CLOSE_TIMEOUT`. + +## Plan + +`Protocol::drained` in `rs/moq-net/src/driver.rs` returns `true` for IETF, so +its sessions close at once. Count the IETF publisher's in-flight subscribe and +fetch serves the way `lite::Publisher::drained` does, and report them there. +Extend `rs/moq-net/tests/session_close.rs` to cover an IETF version. + +## Related + +- [Session close](/quest/m1/session-close.md) - the graceful end that withdraws announces diff --git a/rs/moq-cli/src/main.rs b/rs/moq-cli/src/main.rs index a3730144ac..15809849e4 100644 --- a/rs/moq-cli/src/main.rs +++ b/rs/moq-cli/src/main.rs @@ -387,7 +387,8 @@ impl Directions { /// hops it crossed, and our own Hop ID is one of them, so a broadcast we /// publish is never announced back to us. /// -/// Returns an allocator over the uplink's bandwidth estimate, for the sources that +/// Returns the dialed [`Connection`](moq_tokio::Connection), if any, for a graceful +/// close, and an allocator over the uplink's bandwidth estimate, for the sources that /// share it. Capture encoders follow their slice; passthrough imports reserve /// their peak-hold bitrate so the encoder sees what is left. Only an outbound /// client has an estimate: a `--listen` publisher's sessions are inbound and @@ -406,8 +407,9 @@ async fn spawn_moq( cluster: moq_relay::cluster::Cluster, directions: Directions, tasks: &mut JoinSet>, -) -> anyhow::Result<(moq_net::bandwidth::Allocator, moq_net::origin::Producer)> { +) -> anyhow::Result { let mut bandwidth = moq_net::bandwidth::Allocator::unlimited(); + let mut connection = None; let cluster = cluster .with_client(client.clone()) .with_client_tls(moq.client.tls.build()?) @@ -431,7 +433,9 @@ async fn spawn_moq( // survives reconnects, reading `None` while down, so it can be wired up before // anything connects. bandwidth = moq_net::bandwidth::Allocator::new(reconnect.send_bandwidth()); - tasks.spawn(async move { Ok(reconnect.closed().await?) }); + let closed = reconnect.clone(); + tasks.spawn(async move { Ok(closed.closed().await?) }); + connection = Some(reconnect); } let started = @@ -440,7 +444,19 @@ async fn spawn_moq( tasks.spawn(async move { started.run().await }); } - Ok((bandwidth, origin)) + Ok(Attached { + bandwidth, + origin, + connection, + }) +} + +/// What [`spawn_moq`] attached to the MoQ network. +struct Attached { + bandwidth: moq_net::bandwidth::Allocator, + origin: moq_net::origin::Producer, + /// The relay connection, when `--connect` dialed one. + connection: Option, } /// Report readiness only after every configured MoQ attachment initializes. @@ -472,7 +488,7 @@ async fn run_play(moq: MoqSide, args: play::Args, net: Net) -> anyhow::Result<() ..Default::default() }; let client = net.client(moq.client.clone())?; - let (_, origin) = spawn_moq(&moq, &net, client, cluster, directions, &mut tasks).await?; + let Attached { origin, .. } = spawn_moq(&moq, &net, client, cluster, directions, &mut tasks).await?; play::run(origin.consume(), name, args, tasks) } @@ -491,9 +507,14 @@ async fn run_stages(moq: MoqSide, stages: Vec, net: Net) -> anyhow::Res // The stage combinations were refused up front by `Invocation::validate`, before // anything bound a port or dialed out. let client = net.client(moq.client.clone())?; + let mut connection = None; let result = async { - let (bandwidth, origin) = - spawn_moq(&moq, &net, client.clone(), cluster, Directions::of(&stages), &mut tasks).await?; + let Attached { + bandwidth, + origin, + connection: attached, + } = spawn_moq(&moq, &net, client.clone(), cluster, Directions::of(&stages), &mut tasks).await?; + connection = attached; // stdin and stdout are one resource each, so two stages can't share them. let mut stdin = None; @@ -531,7 +552,13 @@ async fn run_stages(moq: MoqSide, stages: Vec, net: Net) -> anyhow::Res .await; // The process exits next, even on a setup error, so the relay only hears we left - // if the close goes out now. + // if the close goes out now. The connection first delivers what it queued, such + // as the finished tracks at stdin EOF, since the client's close discards it. + if let Some(connection) = connection + && let Err(err) = connection.close().await + { + tracing::warn!(%err, "closed before delivering everything"); + } client.close().await; result } diff --git a/rs/moq-cli/tests/import.rs b/rs/moq-cli/tests/import.rs new file mode 100644 index 0000000000..a6c50ddcdc --- /dev/null +++ b/rs/moq-cli/tests/import.rs @@ -0,0 +1,98 @@ +//! `moq import` over a real relay: at stdin EOF the process finishes its tracks and +//! exits, and a subscriber sees the catalog finish rather than the publisher vanish. + +use std::process::Stdio; +use std::time::Duration; + +use tokio::io::AsyncWriteExt; + +const TIMEOUT: Duration = Duration::from_secs(10); +const BBB: &[u8] = include_bytes!("../../moq-mux/src/container/ts/test_data/bbb.ts"); + +#[tokio::test] +async fn import_delivers_the_catalog_finish_at_eof() { + let _ = moq_tokio::crypto::install_default(); + let fixture = moq_relay::test_relay().await.expect("test relay"); + let ready = fixture.relay.ready(); + tokio::spawn(fixture.relay.run()); + ready.wait().await.expect("relay ready"); + + let mut command = tokio::process::Command::new(env!("CARGO_BIN_EXE_moq")); + // A `MOQ_*` variable in the developer's shell must not reconfigure the child. + for (name, _) in std::env::vars_os() { + if name.to_string_lossy().starts_with("MOQ_") { + command.env_remove(name); + } + } + let mut child = command + .args([ + "--connect", + fixture.url.as_str(), + "--connect-tls-fingerprint", + &fixture.fingerprint, + "--broadcast", + "demo", + "import", + "ts", + ]) + .stdin(Stdio::piped()) + .kill_on_drop(true) + .spawn() + .expect("spawn moq"); + let mut stdin = child.stdin.take().expect("stdin"); + stdin.write_all(BBB).await.expect("write stdin"); + + let origin = moq_tokio::origin::spawn(); + let consumer = origin.consume(); + let mut announced = consumer.announced(); + let mut config = moq_tokio::connect::Config::default(); + config.tls.insecure = Some(true); + config.bind = Some("127.0.0.1:0".parse().unwrap()); + let _connection = config + .init(Default::default()) + .expect("client") + .with_subscriber(origin) + .with_reconnect(false) + .connect(fixture.url.clone()) + .established() + .await + .expect("subscriber connects"); + + tokio::time::timeout(TIMEOUT, announced.next()) + .await + .expect("announce timed out") + .expect("origin closed"); + let broadcast = tokio::time::timeout(TIMEOUT, consumer.request_broadcast("demo")) + .await + .expect("request timed out") + .expect("announced broadcast resolves"); + let mut catalogs = hang::catalog::Catalog::<()>::subscribe(&broadcast) + .await + .expect("subscribe to the catalog"); + + // The relay is serving the catalog before stdin ends, so the finish is queued on a + // live subscription when the process exits. + tokio::time::timeout(TIMEOUT, catalogs.next()) + .await + .expect("catalog timed out") + .expect("catalog read") + .expect("a catalog"); + + drop(stdin); + let status = tokio::time::timeout(TIMEOUT, child.wait()) + .await + .expect("moq never exited") + .expect("wait for moq"); + assert!(status.success(), "moq exited with {status}"); + + loop { + match tokio::time::timeout(TIMEOUT, catalogs.next()) + .await + .expect("the catalog never ended") + { + Ok(Some(_)) => {} + Ok(None) => break, + Err(err) => panic!("the catalog ends with {err} instead of finishing"), + } + } +} diff --git a/rs/moq-net/src/driver.rs b/rs/moq-net/src/driver.rs index d00f74d785..22c677e4bb 100644 --- a/rs/moq-net/src/driver.rs +++ b/rs/moq-net/src/driver.rs @@ -77,6 +77,15 @@ impl Protocol { Self::Ietf(driver) => waiter.poll_future(driver.as_mut()), } } + + /// Whether the protocol owes the peer no queued data, so a draining close can + /// proceed. The IETF driver does not track this and closes at once. + fn drained(&self) -> bool { + match self { + Self::Lite(driver) => driver.drained(), + Self::Ietf(_) => true, + } + } } impl State { @@ -87,13 +96,22 @@ impl State { self.supervisor = None; } - if self.result.is_none() - && let Poll::Ready(result) = self.protocol.poll(waiter) - { - self.result = Some(result); - // The protocol's last act was closing the transport, which wakes the - // supervisor's close watch; poll it now instead of waiting a turn. - if let Some(supervisor) = &mut self.supervisor + if self.result.is_none() { + // The protocol's last act is closing the transport, and so is a drain + // once the protocol that just ran owes the peer nothing. Either wakes + // the supervisor's close watch; poll it now instead of waiting a turn. + let closed = match self.protocol.poll(waiter) { + Poll::Ready(result) => { + self.result = Some(result); + true + } + Poll::Pending => self + .supervisor + .as_mut() + .is_some_and(|supervisor| supervisor.poll_drain(self.protocol.drained(), waiter)), + }; + if closed + && let Some(supervisor) = &mut self.supervisor && supervisor.poll(waiter).is_ready() { self.supervisor = None; diff --git a/rs/moq-net/src/lite/publisher.rs b/rs/moq-net/src/lite/publisher.rs index baf7880b1b..7a3d5ccb25 100644 --- a/rs/moq-net/src/lite/publisher.rs +++ b/rs/moq-net/src/lite/publisher.rs @@ -5,7 +5,7 @@ use std::{ ops::Bound, sync::{ Arc, - atomic::{AtomicU64, Ordering}, + atomic::{AtomicU64, AtomicUsize, Ordering}, }, task::{Context, Poll, ready}, time::Duration, @@ -62,6 +62,8 @@ struct Shared { priority: PriorityQueue, version: Version, goaway: crate::goaway::Protocol, + // Control streams still serving the peer data, which a draining close waits for. + owed: AtomicUsize, } /// Largest millisecond duration every implementation can carry losslessly. @@ -160,6 +162,7 @@ impl Publisher { priority: Default::default(), version: config.version, goaway: config.goaway, + owed: AtomicUsize::new(0), }), runtime: config.runtime, accept, @@ -194,6 +197,13 @@ where let _ = self.children.poll(waiter); Poll::Pending } + + /// Whether no control stream still owes the peer data. Announce, probe, and + /// goaway streams last as long as the session, so only the serves that end on + /// their own count: subscriptions, fetches, and track info replies. + pub fn drained(&self) -> bool { + self.shared.owed.load(Ordering::Relaxed) == 0 + } } #[cfg(test)] @@ -239,6 +249,21 @@ enum ControlState { Done, } +impl ControlState { + /// Whether this serve owes the peer data until it ends on its own. + fn owes(&self) -> bool { + matches!(self, Self::Subscribe(_) | Self::Fetch(_) | Self::TrackInfo(_)) + } +} + +impl Drop for Control { + fn drop(&mut self) { + if self.state.owes() { + self.shared.owed.fetch_sub(1, Ordering::Relaxed); + } + } +} + impl kio::Task for Control { fn poll(&mut self, waiter: &kio::Waiter) -> Poll<()> { if let Err(err) = ready!(self.poll_serve(waiter)) { @@ -276,6 +301,9 @@ impl Control { lite::ControlType::Goaway => ControlState::Goaway { stream }, lite::ControlType::Session => return Poll::Ready(Err(Error::UnexpectedStream)), }; + if self.state.owes() { + self.shared.owed.fetch_add(1, Ordering::Relaxed); + } } ControlState::Announce(serve) => return serve.poll(waiter), ControlState::Subscribe(serve) => return serve.poll(waiter), diff --git a/rs/moq-net/src/lite/session.rs b/rs/moq-net/src/lite/session.rs index 7417c05c2c..f02013ed5e 100644 --- a/rs/moq-net/src/lite/session.rs +++ b/rs/moq-net/src/lite/session.rs @@ -252,6 +252,11 @@ where Poll::Ready(res) } + /// Whether no stream still owes the peer data, for a draining close. + pub(crate) fn drained(&self) -> bool { + self.publisher.drained() + } + fn poll_protocol(&mut self, waiter: &kio::Waiter) -> Poll> { let mut cx = Context::from_waker(waiter.waker()); diff --git a/rs/moq-net/src/session.rs b/rs/moq-net/src/session.rs index 8f89d91a87..ae54e504d2 100644 --- a/rs/moq-net/src/session.rs +++ b/rs/moq-net/src/session.rs @@ -6,11 +6,25 @@ use web_transport_trait::Stats as _; use crate::{Error, SessionError, Version, bandwidth, goaway}; +/// How long [`Session::close`] waits for queued data before closing anyway. +const CLOSE_TIMEOUT: Duration = Duration::from_secs(1); + /// A close requested by a session handle, executed by the driver. #[derive(Clone)] -struct Close { - code: u32, - reason: String, +enum Close { + /// Close now with this code. + Abort { code: u32, reason: String }, + /// Close once the protocol owes the peer nothing, or at [`CLOSE_TIMEOUT`]. + Drain, +} + +/// How the session ended, published by the driver. +#[derive(Clone)] +struct Ended { + /// The transport's terminal error. + err: Error, + /// The drain's outcome, when a drain is what closed the transport. + drain: Option>, } /// The stats cell shared between the driver's sampler and the handles. @@ -76,11 +90,12 @@ pub struct Stats { /// the stats sample) is relayed through the driver. #[derive(Clone)] pub struct Session { - /// Handle side to driver: `Some` once [`abort`](Self::abort) ran; the - /// channel closing (the last handle dropping) is the implicit Cancel. + /// Handle side to driver: `Some` once [`abort`](Self::abort) or + /// [`close`](Self::close) ran; the channel closing (the last handle + /// dropping) is the implicit Cancel, unless a drain was already requested. close: kio::Producer>, - /// Driver to handle side: the transport's terminal error. - closed: kio::Consumer>, + /// Driver to handle side: how the transport ended. + closed: kio::Consumer>, stats: kio::Shared, version: Version, send_bandwidth: Option, @@ -129,21 +144,54 @@ impl Session { } /// Close the transport with an explicit error, instead of waiting for the last - /// clone to drop. Idempotent: the first close wins. + /// clone to drop. Idempotent: the first abort wins, and it cuts short a + /// [`close`](Self::close) still draining. /// /// The close is executed by the session's driver, so it reaches the wire /// once the runtime polls it (immediately on a live runtime). pub fn abort(&self, err: Error) { if let Ok(mut close) = self.close.write() - && close.is_none() + && !matches!(*close, Some(Close::Abort { .. })) { - *close = Some(Close { + *close = Some(Close::Abort { code: SessionError::from(&err).to_code(), reason: err.to_string(), }); } } + /// Close the session once the data it queued has been delivered. + /// + /// Waits until every stream still serving the peer (a subscription whose track + /// finished, a fetch, a track info reply) has written its data and FIN and the + /// peer acknowledged them, then closes the transport. Returns + /// [`Error::Timeout`] if that takes longer than one second, closing anyway, or + /// the session's terminal error if it ended some other way first. A track that + /// is still live never finishes, so finish or abort tracks before closing. + /// + /// moq-transport (IETF) sessions close without waiting. + pub async fn close(self) -> Result<(), Error> { + if let Ok(mut close) = self.close.write() + && close.is_none() + { + *close = Some(Close::Drain); + } + // The drain outlives this handle, so dropping it cannot cut the drain short. + let closed = self.closed.clone(); + drop(self); + + match closed + .wait(|state| match &**state { + Some(ended) => Poll::Ready(ended.clone()), + None => Poll::Pending, + }) + .await + { + Ok(ended) => ended.drain.unwrap_or(Err(ended.err)), + Err(kio::Closed) => Err(Error::Cancel), + } + } + /// Block until the transport session is closed, returning the reason. /// /// A close code the peer sent is decoded through the session registry (so an auth @@ -156,7 +204,7 @@ impl Session { match self .closed .wait(|state| match &**state { - Some(err) => Poll::Ready(err.clone()), + Some(ended) => Poll::Ready(ended.err.clone()), None => Poll::Pending, }) .await @@ -237,6 +285,7 @@ impl Session { stats: stats.clone(), send_bandwidth: send_producer, mode: SamplerMode::Idle, + drain: Drain::Idle, }; let session = Self { @@ -277,13 +326,24 @@ pub(crate) struct Supervisor { /// first close matters, and the channel closing is the last handle /// dropping). close: Option>>, - /// Where the transport's terminal error is published for [`Session::closed`]. - closed: kio::Producer>, + /// Where the transport's end is published for [`Session::closed`]. + closed: kio::Producer>, stats: kio::Shared, /// The send-rate estimate channel, when the backend reports one. `None` /// also once every consumer is gone for good. send_bandwidth: Option, mode: SamplerMode, + drain: Drain, +} + +/// A [`Session::close`] waiting for the protocol to deliver what it queued. +enum Drain { + /// Nobody asked for one. + Idle, + /// Requested: close once drained, or at the deadline. + Waiting(crate::runtime::Deadline), + /// The drain closed the transport, with this outcome. + Done(Result<(), Error>), } enum SamplerMode { @@ -307,30 +367,55 @@ impl Supervisor { // this cell, so leave it holding the session's final counters // rather than whichever sample the last demand happened to catch. self.stats.lock().sample = snapshot(&self.session); + let drain = match std::mem::replace(&mut self.drain, Drain::Idle) { + Drain::Done(res) => Some(res), + _ => None, + }; if let Ok(mut closed) = self.closed.write() { - *closed = Some(Error::from_transport(err)); + *closed = Some(Ended { + err: Error::from_transport(err), + drain, + }); } return Poll::Ready(()); } - // Execute the first handle-side close request. The channel closing is - // the last handle dropping, with an abort written just before winning - // over the implicit cancel. - if let Some(close) = &self.close { - let request = match close.poll(waiter, |state| match &**state { + // Execute the handle-side close requests. The channel closing is the + // last handle dropping, with a request written just before winning over + // the implicit cancel. A drain loops back to watch for an abort, which + // cuts it short. + while let Some(close) = &self.close { + let draining = matches!(self.drain, Drain::Waiting(_)); + let (request, last) = match close.poll(waiter, |state| match &**state { + Some(Close::Drain) if draining => Poll::Pending, Some(request) => Poll::Ready(request.clone()), None => Poll::Pending, }) { - Poll::Ready(Ok(request)) => Some(request), - Poll::Ready(Err(last)) => Some(last.clone().unwrap_or_else(|| Close { - code: SessionError::Cancel.to_code(), - reason: "dropped".to_string(), - })), - Poll::Pending => None, + Poll::Ready(Ok(request)) => (request, false), + Poll::Ready(Err(last)) => ( + last.clone().unwrap_or_else(|| Close::Abort { + code: SessionError::Cancel.to_code(), + reason: "dropped".to_string(), + }), + true, + ), + Poll::Pending => break, }; - if let Some(request) = request { - self.session.close(request.code, &request.reason); - self.close = None; + match request { + Close::Abort { code, reason } => { + self.session.close(code, &reason); + self.drain = Drain::Idle; + self.close = None; + } + Close::Drain => { + if !draining { + self.drain = Drain::Waiting(crate::runtime::Deadline::after(&self.runtime, CLOSE_TIMEOUT)); + } + // No handle is left to abort. + if last { + self.close = None; + } + } } } @@ -338,6 +423,27 @@ impl Supervisor { Poll::Pending } + /// Finish a requested drain once the protocol owes the peer nothing, or at + /// the deadline. Returns whether this closed the transport. + /// + /// Called after the protocol ran this turn, since only then is `drained` + /// current. + pub(crate) fn poll_drain(&mut self, drained: bool, waiter: &kio::Waiter) -> bool { + let Drain::Waiting(deadline) = &mut self.drain else { + return false; + }; + let res = match drained { + true => Ok(()), + false if deadline.poll(waiter).is_ready() => Err(Error::Timeout), + false => return false, + }; + self.session.close(SessionError::Cancel.to_code(), ""); + self.drain = Drain::Done(res); + // The transport is closed, so no later request can change anything. + self.close = None; + true + } + /// Take one sample and arm the next deadline. fn sample(&mut self) { let sample = snapshot(&self.session); diff --git a/rs/moq-net/tests/session_close.rs b/rs/moq-net/tests/session_close.rs new file mode 100644 index 0000000000..9c7308f6ac --- /dev/null +++ b/rs/moq-net/tests/session_close.rs @@ -0,0 +1,157 @@ +//! `Session::close` delivers what the session queued before closing, within a deadline, +//! while `abort` still closes at once. +//! +//! Deterministic: paused time and a single-threaded runtime over the mock transport. + +mod support; + +use std::time::Duration; + +use moq_net::{Error, Hop, Timestamp, Version}; +use support::harness::{MockConnectOptions, MockPair, connect_mock}; + +const TIMEOUT: Duration = Duration::from_secs(10); + +fn produce_origin(hop: u64) -> moq_net::origin::Producer { + let (producer, driver) = moq_net::origin::Producer::new(moq_net::origin::Config::new(Hop::new(hop).unwrap())); + tokio::spawn(support::harness::run(driver)); + producer +} + +/// The payloads the server read, then how the track ended. +type Read = (Vec>, Result<(), Error>); + +/// A client publishing one track to a server that subscribes to it. +struct Setup { + pair: MockPair, + track: moq_net::track::Producer, + reader: tokio::task::JoinHandle, + _broadcast: moq_net::broadcast::Producer, +} + +async fn setup() -> Setup { + let publisher = produce_origin(1); + let broadcast = publisher.create_broadcast("bcast").unwrap(); + let track = broadcast.create_track("video", None).unwrap(); + broadcast.announce(Default::default()).unwrap(); + + let subscriber = produce_origin(2); + let mut options = MockConnectOptions::new("moq-lite-05".parse::().unwrap()); + options.client_publish = Some(publisher.consume()); + options.server_subscribe = Some(subscriber.clone()); + let pair = connect_mock(options).await; + + let consumer = subscriber.consume(); + tokio::time::timeout(TIMEOUT, consumer.routed("bcast")) + .await + .expect("announce timeout") + .expect("routed"); + let remote = tokio::time::timeout(TIMEOUT, consumer.request_broadcast("bcast")) + .await + .expect("resolve timeout") + .expect("broadcast resolves"); + + // The publisher only learns of a subscription once the subscriber polls it. + let reader = tokio::spawn(async move { + let mut sub = remote.track("video").unwrap().subscribe(None).await.expect("subscribe"); + let mut got = Vec::new(); + loop { + let mut group = match sub.recv_group().await { + Ok(Some(group)) => group, + Ok(None) => return (got, Ok(())), + Err(err) => return (got, Err(err)), + }; + loop { + match group.read_frame().await { + Ok(Some(frame)) => got.push(frame.payload.to_vec()), + Ok(None) => break, + Err(err) => return (got, Err(err)), + } + } + } + }); + + tokio::time::timeout(TIMEOUT, track.used()) + .await + .expect("no subscriber appeared") + .unwrap(); + + Setup { + pair, + track, + reader, + _broadcast: broadcast, + } +} + +/// A finished track's last group and FIN reach the subscriber, even though the +/// close is requested before the session wrote them. +#[tokio::test(start_paused = true)] +async fn close_delivers_a_finished_track() { + let Setup { + pair, track, reader, .. + } = setup().await; + + let mut group = track.append_group().unwrap(); + group.write_frame(Timestamp::ZERO, b"last".as_slice()).unwrap(); + group.finish().unwrap(); + track.finish().unwrap(); + + let started = tokio::time::Instant::now(); + tokio::time::timeout(TIMEOUT, pair.client.close()) + .await + .expect("close timed out") + .expect("the close drains"); + assert!( + started.elapsed() < Duration::from_secs(1), + "drained before the deadline" + ); + + let (got, end) = tokio::time::timeout(TIMEOUT, reader) + .await + .expect("reader timed out") + .unwrap(); + assert_eq!(got, vec![b"last".to_vec()]); + end.expect("the track finishes"); +} + +/// A track that never finishes holds the drain until the deadline, then the +/// session closes anyway. +#[tokio::test(start_paused = true)] +async fn close_gives_up_on_a_live_track() { + let Setup { + pair, track: _track, .. + } = setup().await; + + let started = tokio::time::Instant::now(); + let res = tokio::time::timeout(TIMEOUT, pair.client.close()) + .await + .expect("close timed out"); + assert!(matches!(res, Err(Error::Timeout)), "{res:?}"); + assert_eq!(started.elapsed(), Duration::from_secs(1)); +} + +/// An abort from another handle cuts a drain short. +#[tokio::test(start_paused = true)] +async fn abort_cuts_a_drain_short() { + let Setup { + pair, track: _track, .. + } = setup().await; + + let other = pair.client.clone(); + let started = tokio::time::Instant::now(); + let close = tokio::spawn(pair.client.close()); + tokio::time::sleep(Duration::from_millis(100)).await; + other.abort(Error::Cancel); + + let res = tokio::time::timeout(TIMEOUT, close) + .await + .expect("close timed out") + .unwrap(); + assert!(res.is_err() && !matches!(res, Err(Error::Timeout)), "{res:?}"); + assert!( + started.elapsed() < Duration::from_secs(1), + "{res:?} after {:?}", + started.elapsed() + ); +} diff --git a/rs/moq-tokio/src/client.rs b/rs/moq-tokio/src/client.rs index 562d471e48..dd1a0fdceb 100644 --- a/rs/moq-tokio/src/client.rs +++ b/rs/moq-tokio/src/client.rs @@ -244,6 +244,9 @@ impl Client { /// only queues the close, which nothing sends once the runtime stops: a process /// that exits without this leaves each peer waiting out its idle timeout. /// + /// This closes at once, discarding stream data the peer has not acknowledged yet. + /// Call [`Connection::close`] on each connection first to deliver it. + /// /// Only the noq endpoint is closed. WebSocket, TCP, and UDS sessions end when their /// [`Connection`] is dropped (the kernel closes the socket on exit), and an iroh /// endpoint passed to `with_iroh` is closed by its owner. diff --git a/rs/moq-tokio/src/connection.rs b/rs/moq-tokio/src/connection.rs index 2bae530190..8268bf1ef5 100644 --- a/rs/moq-tokio/src/connection.rs +++ b/rs/moq-tokio/src/connection.rs @@ -642,7 +642,7 @@ struct Task { closed: CloseGuard, } -/// Serializes [`Connection::abort`] against the loop publishing a fresh session. +/// Serializes [`Connection::abort`] and [`Connection::close`] against the loop publishing a fresh session. /// /// Aborting a tokio task doesn't interrupt it before its next yield, so a redial /// completing in that window would otherwise hand [`Shared::connected`] a session @@ -723,6 +723,27 @@ impl Connection { self.task.handle.abort(); } + /// Stop the loop for every clone, closing the live session once the data it + /// queued has been delivered. + /// + /// See [`moq_net::Session::close`]: finished tracks deliver their last groups and + /// FIN first, bounded by a one second deadline. Call this before + /// [`Client::close`], which closes the transport without waiting. Returns `Ok` + /// when nothing was live, and the session's error if it did not drain. + pub async fn close(self) -> crate::Result<()> { + // Refuse redials and take the session under one lock: see [`CloseGuard`]. + let session = { + let mut closed = self.task.closed.lock().unwrap(); + *closed = Some(moq_net::Error::Cancel); + self.state.read().session.clone() + }; + self.task.handle.abort(); + match session { + Some(session) => Ok(session.close().await?), + None => Ok(()), + } + } + async fn run(shared: &Shared, client: Client, addrs: Addrs) -> crate::Result<()> { let backoff = client.backoff.clone(); let goaway = client.goaway.resolve(); diff --git a/rs/moq-tokio/tests/backend.rs b/rs/moq-tokio/tests/backend.rs index 50b0d80d4b..f4c3c026a6 100644 --- a/rs/moq-tokio/tests/backend.rs +++ b/rs/moq-tokio/tests/backend.rs @@ -722,6 +722,118 @@ async fn noq_client_close_reaches_server() { assert!(!err.to_string().contains("timed out"), "{err}"); } +/// A client that finishes its track and closes before its runtime stops still delivers +/// the queued group and the track's finish, instead of the close discarding them. +#[cfg(feature = "noq")] +#[tracing_test::traced_test] +#[tokio::test] +async fn noq_client_close_drains_finished_track() { + // Small, since the debug build logging every packet is slow and the drain has + // one second. Queued right before the close, it is still unacknowledged then. + let payload: Vec = (0..1024).map(|i| i as u8).collect(); + + let quic = moq_tokio::quic::Config::default(); + let mut server_config = moq_tokio::listen::Config::default(); + server_config.bind = Some("127.0.0.1:0".parse().unwrap()); + server_config.tls.generate = vec!["localhost".into()]; + let server = server_config.init(quic.clone()).expect("failed to init server"); + let mut server = server.listen().await.expect("failed to listen"); + let url: url::Url = format!("moqt://localhost:{}", server.local_addr().unwrap().port()) + .parse() + .unwrap(); + + // The client gets a runtime of its own, gone as soon as the client returns: nothing + // drives its endpoint afterwards, exactly as when a process exits. + let expected = payload.clone(); + let client = std::thread::spawn(move || { + let runtime = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .expect("client runtime"); + runtime.block_on(async move { + let origin = moq_tokio::origin::spawn(); + let broadcast = origin.create_broadcast("test").expect("failed to create broadcast"); + broadcast.announce(Default::default()).expect("failed to announce"); + let mut track = broadcast.create_track("video", None).expect("failed to create track"); + + let mut config = moq_tokio::connect::Config::default(); + config.tls.insecure = Some(true); + config.bind = Some("127.0.0.1:0".parse().unwrap()); + let client = config + .init(quic) + .expect("failed to init client") + .with_publisher(origin.consume()); + let (client, connection) = connect_once(client, url).await.expect("client connect failed"); + + // Write only once the server's subscription is being served. + while track.subscription().is_none() { + track.subscription_changed().await.expect("track closed"); + } + let mut group = track.append_group().expect("failed to append group"); + group + .write_frame(moq_tokio::moq_net::Timestamp::ZERO, payload) + .expect("failed to write frame"); + group.finish().expect("failed to finish group"); + track.finish().expect("failed to finish track"); + + connection.close().await.expect("the close drains"); + client.close().await; + }); + }); + + let request = tokio::time::timeout(TIMEOUT, server.accept()) + .await + .expect("accept timed out") + .expect("no incoming connection"); + let origin = moq_tokio::origin::spawn(); + let consumer = origin.consume(); + let mut announcements = consumer.announced(); + let _session = request + .with_subscriber(origin) + .ok() + .await + .expect("server handshake failed"); + + tokio::time::timeout(TIMEOUT, announcements.next()) + .await + .expect("announce timed out") + .expect("origin closed"); + let broadcast = tokio::time::timeout(TIMEOUT, consumer.request_broadcast("test")) + .await + .expect("request timed out") + .expect("announced broadcast resolves"); + let mut track = tokio::time::timeout(TIMEOUT, broadcast.track("video").unwrap().subscribe(None)) + .await + .expect("subscribe timed out") + .expect("subscribe failed"); + + let mut group = tokio::time::timeout(TIMEOUT, track.recv_group()) + .await + .expect("recv_group timed out") + .expect("recv_group failed") + .expect("track ended before the group"); + let frame = tokio::time::timeout(TIMEOUT, group.read_frame()) + .await + .expect("read_frame timed out") + .expect("read_frame failed") + .expect("group ended before the frame"); + assert!(frame.payload[..] == expected[..], "the frame arrives whole"); + + let end = tokio::time::timeout(TIMEOUT, track.recv_group()) + .await + .expect("the track end timed out"); + match end { + Ok(None) => {} + Ok(Some(_)) => panic!("an unexpected second group"), + Err(err) => panic!("the track ends with {err} instead of finishing"), + } + + tokio::task::spawn_blocking(move || client.join()) + .await + .unwrap() + .expect("client thread panicked"); +} + #[cfg(feature = "noq")] #[tracing_test::traced_test] #[tokio::test] From bd2840874627d70fd5d33dddacec9ea5d77a7004 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Mon, 28 Sep 2026 18:29:23 -0700 Subject: [PATCH 11/37] chore(quest): JS IETF reprices a namespace in place (#4424) Co-authored-by: Claude Opus 5.5 --- quest/m1/README.md | 1 + quest/m1/js-ietf-reprice.md | 13 +++++++++++++ 2 files changed, 14 insertions(+) create mode 100644 quest/m1/js-ietf-reprice.md diff --git a/quest/m1/README.md b/quest/m1/README.md index 9b9e5995ef..45d4e313c0 100644 --- a/quest/m1/README.md +++ b/quest/m1/README.md @@ -72,6 +72,7 @@ transport, benchmark tooling); worktrees isolate commits, not semantics. - [Go and Dart doc samples](/quest/m1/doc-samples-go-dart.md) - Go and Dart doc samples compile against their wrappers - [Data capture in bindings](/quest/m1/data-capture-bindings.md) - moq-ffi and every wrapper pass a data frame's capture time, and the JSON window producer takes one - [Moxygen compatibility](/quest/m1/moxygen/README.md) - one subgroup per group, whole-group FETCH, and one datagram per group, never a full moxygen pass +- [JS IETF reprice](/quest/m1/js-ietf-reprice.md) - `@moq/net`'s IETF publisher reprices a held namespace with REQUEST_UPDATE, like Rust, instead of withdraw-then-advertise - [JS IETF datagrams](/quest/m1/js-ietf-datagram.md) - `@moq/net` sends and receives datagram groups over moq-transport, like Rust - [#2991](/quest/m1/2991-net-coalesce-dynamic-tracks-and-preserve-sequences-across.md) - one dynamic producer per track name in both languages, with the sequence namespace surviving a replacement - [JavaScript FETCH](/quest/m1/js-fetch.md) - generic on-demand group serving and IETF FETCH for browser publishers diff --git a/quest/m1/js-ietf-reprice.md b/quest/m1/js-ietf-reprice.md new file mode 100644 index 0000000000..55bfd480d8 --- /dev/null +++ b/quest/m1/js-ietf-reprice.md @@ -0,0 +1,13 @@ +# [S] JS IETF reprices a namespace in place + +## Goal + +When a route's price changes, `@moq/net`'s IETF publisher reprices the namespace the peer already holds with REQUEST_UPDATE on the request that carries it, as Rust does, instead of withdrawing it and advertising it again. A subscriber never sees a namespace briefly vanish because its cost or hop chain moved. + +## Plan + +Rust already does this: `rs/moq-net/src/ietf/publisher.rs` reprices with REQUEST_UPDATE and respects MAX_REQUEST_UPDATES. `js/net/src/ietf/publisher.ts` withdraws first, so that "a republish or re-price reads as withdraw-then-advertise". Keep withdraw-then-advertise only for a republish, meaning a different broadcast. For the same broadcast at a new warm cost or MoQ Cluster hop path, send REQUEST_UPDATE. The wire-visible comparison from the announce-dedupe work decides whether anything is sent at all. + +Found while landing the announce update dedupe (#4423). No wire change: REQUEST_UPDATE is already in the IETF drafts the tree negotiates. + +Tests: a price change on a held namespace sends one REQUEST_UPDATE and no PUBLISH_NAMESPACE_DONE; a republish still withdraws; a peer's MAX_REQUEST_UPDATES limit is honored the way Rust honors it. From 785ee4c8f66ba200a6680b494d7f5fdf40af3514 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Mon, 28 Sep 2026 18:53:53 -0700 Subject: [PATCH 12/37] chore(quest): drop the duplicate Opus mapping family on dev (#4427) Co-authored-by: Claude Opus 5.5 --- quest/m1/README.md | 1 + quest/m1/opus-mapping-family.md | 15 +++++++++++++++ 2 files changed, 16 insertions(+) create mode 100644 quest/m1/opus-mapping-family.md diff --git a/quest/m1/README.md b/quest/m1/README.md index 45d4e313c0..755bdb1033 100644 --- a/quest/m1/README.md +++ b/quest/m1/README.md @@ -89,6 +89,7 @@ transport, benchmark tooling); worktrees isolate commits, not semantics. - [OBS native codecs](/quest/m1/obs-moq-video/README.md) - replace FFmpeg video and audio decoding with moq-video and moq-audio, deliver GPU frames, and use native audio/video encoders - [Opus concealment](/quest/m1/opus-conceal.md) - a lost Opus packet conceals the last packet's length, not 120 ms - [Audio codecs](/quest/m1/audio-codecs/README.md) - platform audio codecs, explicit unsupported cases, and channel layouts up to 7.1 +- [Opus mapping family](/quest/m1/opus-mapping-family.md) - on dev, the Opus head config keeps its mapping family only in `mapping` - [Opus catalog rate](/quest/m1/opus-catalog-rate.md) - MKV Opus import publishes the 48 kHz codec rate in the catalog, not the OpusHead input rate - [mp4-atom dOps mapping](/quest/m1/mp4-atom-dops-mapping.md) - a released mp4-atom reads and writes any `dOps` channel mapping family and table - [CMAF surround Opus](/quest/m1/cmaf-opus-surround.md) - fMP4 import and export carry an Opus channel mapping table diff --git a/quest/m1/opus-mapping-family.md b/quest/m1/opus-mapping-family.md new file mode 100644 index 0000000000..f2dec437cd --- /dev/null +++ b/quest/m1/opus-mapping-family.md @@ -0,0 +1,15 @@ +# [XS] Opus head carries its mapping family once + +## Goal + +On dev, `moq_mux::codec::opus::Config` stores the channel mapping family in one place: the audio-codecs line's `mapping: Option`. The `mapping_family: u8` field that #4130 shipped in moq-mux 0.10.8 is removed, and every reader takes the family from `mapping`. + +## Plan + +Decided 2026-09-28 while merging main into the audio-codecs line: that sync keeps both fields so the line stays additive on main, and `encode` refuses a `mapping_family` that disagrees with `mapping`. Removing the duplicate breaks the published moq-mux API, so it lands on dev once the line has merged. Update main's fMP4 and MSF paths that read `mapping_family`, plus the TS importer, which sets both fields. + +Public API: breaking in moq-mux (field removed). Wire: none. + +## Required + +- [Audio codecs](/quest/m1/audio-codecs/README.md) - the line that introduces `mapping` From 5b364811073951f0caec28d75c8a2812814e538b Mon Sep 17 00:00:00 2001 From: shermerL <86936692+shermerL@users.noreply.github.com> Date: Tue, 29 Sep 2026 10:09:44 +0800 Subject: [PATCH 13/37] fix(tokio): handle IPv6 literals in TLS server names (#4322) --- rs/moq-tokio/src/noq.rs | 61 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 60 insertions(+), 1 deletion(-) diff --git a/rs/moq-tokio/src/noq.rs b/rs/moq-tokio/src/noq.rs index 99b4bc48aa..15e3692d75 100644 --- a/rs/moq-tokio/src/noq.rs +++ b/rs/moq-tokio/src/noq.rs @@ -343,7 +343,11 @@ impl NoqClient { let mut config = tls.clone(); let target = url.host().ok_or(Error::InvalidDnsName)?; - let host = target.to_string(); + // URL brackets delimit IPv6 literals, but aren't part of the TLS server name. + let host = match &target { + url::Host::Ipv6(ip) => ip.to_string(), + _ => target.to_string(), + }; let port = url.port().unwrap_or(443); // Resolve, adapted to the local socket's family; the dial below races the @@ -983,6 +987,61 @@ mod tests { use super::*; use url::Url; + #[tokio::test] + async fn pinned_ipv6_connection() { + pinned_connection("[::1]:0", None).await; + } + + #[tokio::test] + async fn pinned_ipv4_connection() { + pinned_connection("127.0.0.1:0", None).await; + } + + #[tokio::test] + async fn pinned_ipv6_connection_with_host_override() { + pinned_connection("[::1]:0", Some("localhost")).await; + } + + async fn pinned_connection(bind: &str, host_name: Option<&str>) { + let quic = crate::quic::Config::default(); + let server = NoqServer::new( + listen::Config { + bind: Some(bind.parse().unwrap()), + tls: crate::tls::Listen { + generate: vec!["localhost".into()], + ..Default::default() + }, + ..Default::default() + }, + &quic, + None, + ) + .expect("server init"); + let addr = server.local_addr().expect("local addr"); + let mut tls_config = crate::tls::Connect::default(); + tls_config.fingerprint = server.certificates().fingerprints(); + assert!(!tls_config.fingerprint.is_empty()); + tls_config.host_name = host_name.map(str::to_owned); + let config = connect::Config { + bind: Some(bind.parse().unwrap()), + tls: tls_config, + ..Default::default() + }; + let tls = config.tls.build().expect("tls config"); + let client = NoqClient::new(&config, &quic).expect("client init"); + let url: Url = format!("moqt://{addr}/.cluster/test").parse().unwrap(); + let versions = moq_net::Versions::default(); + let dial = client.connect(&tls, url.into(), &versions); + let accept = async { + let incoming = server.accept().await.expect("incoming connection"); + super::accept(incoming, versions.alpns()).await + }; + let result = tokio::time::timeout(Duration::from_secs(5), async { tokio::try_join!(dial, accept) }) + .await + .expect("handshake timed out"); + let (_client, _server) = result.expect("pinned connection failed"); + } + /// noq exposes no getters for the flow-control windows, but its `Debug` prints /// them, which is enough to prove each one reached the transport config and that /// an unset knob leaves noq's own default in place. From ecf90c9c4018132bb4069ebbb26d5ad5f7edf08d Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Mon, 28 Sep 2026 19:20:19 -0700 Subject: [PATCH 14/37] docs(quest): drop suffix-based routing from the plans (#4382) Co-authored-by: Claude Opus 5.5 --- doc/bin/relay/cluster.md | 12 +- js/pattern/README.md | 2 +- quest/m0/wildcard/README.md | 208 +++++++++++---------------- quest/m1/auth/README.md | 2 +- quest/m1/auth/lite.md | 4 +- quest/m1/broadcast-epoch/README.md | 8 +- quest/m1/cluster-routing.md | 6 +- quest/m1/path-patterns.md | 10 +- quest/m1/processor/README.md | 4 +- quest/m1/processor/advertise-auth.md | 11 +- rs/moq-net/src/path/mod.rs | 6 +- rs/moq-pattern/README.md | 2 +- 12 files changed, 122 insertions(+), 153 deletions(-) diff --git a/doc/bin/relay/cluster.md b/doc/bin/relay/cluster.md index f4c3466a5d..d313dd825a 100644 --- a/doc/bin/relay/cluster.md +++ b/doc/bin/relay/cluster.md @@ -49,18 +49,18 @@ link costs 1, which reproduces plain hop counting. Each relay adds the price of the link an announcement arrived on before forwarding it, so a route's cost is the sum of what it crossed. -Wildcard advertisements are forwarded and costed the same way as an exact-path +Prefix advertisements are forwarded and costed the same way as an exact-path route: each hop appends its identity, adds the link price, and passes the -claim on. An advertisement must be contained by one of the publisher's granted -prefixes (`grant/**`); an over-wide pattern is refused rather than clamped. +claim on. An advertised prefix must overlap the publisher's grant, or it is +refused. A prefix wider than the grant is accepted, but it only routes requests +for paths the grant covers. -Routing prefers the most specific pattern, then a fully identified hop list +Routing prefers the longest covering prefix, then a fully identified hop list over one that holds a 0 (an anonymous hop) at any depth, then the lowest cost, then the shortest hop list, breaking any remaining tie toward the newest announcement so a reconnecting publisher isn't outranked by the session it replaced. An assigned identity for an anonymous peer is local selection state -and is never written into the hop list. Resolving a non-prefix pattern into a -subscription is not implemented yet. +and is never written into the hop list. ```toml [cluster] diff --git a/js/pattern/README.md b/js/pattern/README.md index 61a687d3fc..48f9f3c12a 100644 --- a/js/pattern/README.md +++ b/js/pattern/README.md @@ -10,7 +10,7 @@ Exact path patterns for [Media over QUIC](https://moq.dev): grammar, matching, and set algebra. A pattern describes a set of broadcast paths. This package provides a shared grammar -for tokens, origin scopes, announce interests, and wildcard advertisements. Integrating +for tokens, origin scopes, and announce interests. Integrating patterns into those consumers is separate work. Literal paths stay coordinates; `@moq/net`'s path module keeps construction, joins, and prefix operations. diff --git a/quest/m0/wildcard/README.md b/quest/m0/wildcard/README.md index 6bc8b6153b..8e9173b6d0 100644 --- a/quest/m0/wildcard/README.md +++ b/quest/m0/wildcard/README.md @@ -5,16 +5,17 @@ A service claims the prefix it could serve rather than enumerating every broadcast under it; the client library filters that claim against the pattern interest the caller asked for, so nothing on the wire spells a -wildcard. A claim is priced at what starting the work would cost. Specificity wins first: a concrete -claim shadows a wildcard regardless of cost, and prices compete within the -same specificity tier. A terminal concrete refusal does not fall through to -a catch-all; its claim must be withdrawn. Retracting a wildcard stops new -work without shedding what is already running. - -Three workloads need this, and they are the three pattern shapes. A transcode +wildcard. A claim is priced at what starting the work would cost. The longest +covering prefix wins first: a concrete announcement shadows a broader claim +regardless of cost, and prices compete only among claims of the same prefix. +A terminal concrete refusal does not fall through to a catch-all; its claim +must be withdrawn. Retracting a claim stops new work without shedding what is +already running. + +Three workloads need this. A transcode worker today announces a standby derivative for every matching live broadcast, -so announcements scale as workers times broadcasts; with a suffix pattern -`**/transcode.pro` it advertises once for the whole fleet. A chat backend +so announcements scale as workers times broadcasts; claiming one service +prefix, it advertises once for the whole fleet. A chat backend cannot enumerate at all: rooms exist independently of any broadcast, and the subtree pattern `/chat/**` expresses them. An archive serving recordings over FETCH wants to say "if nobody is publishing this live, I have it", which @@ -33,11 +34,12 @@ across the fleet in resident memory. Decided in [#3770](https://github.com/moq-dev/moq/pull/3770): publishing is prefix-only on every wire and patterns never leave the token or the client library. `dynamic(prefix, route)` -advertises a prefix; a suffix or catch-all claim is expressed as the -widest prefix that covers it (`**` is the root) and the request is the -authority, so the advertise half of this questline is re-scoped to prefix -claims resolved against pattern interest. The three workloads above still -hold: the transcoder claims the root and refuses what it will not serve. +advertises a prefix; the catch-all claim is the root prefix, and the request +is the authority, so the advertise half of this questline is re-scoped to +prefix claims resolved against pattern interest. The three workloads above +still hold: the transcoder claims its service prefix +([Where derived output lives](#where-derived-output-lives)), and the archive +claims the root and refuses what it does not have. Resolve and Demand are additive and land on main. Both are done on the line branch (#4050 re-resolves on a refusal; 9d059b1b9 lists and demands covered renditions in the browser player), so main no longer lists them; the line @@ -50,16 +52,15 @@ production cost: zero for a live publish, something large for a standby that would have to start working (a cold transcoder)" (`drafts/draft-lcurley-moq-lite.md`). `moq_auth::Claims.publish` and `origin::Producer` gain versioned patterns through -[Path patterns](/quest/m1/path-patterns.md), so advertisements reuse the -same exact containment check. `Cost { warm, cold }` +[Path patterns](/quest/m1/path-patterns.md), so tokens and filters reuse the +same matcher; advertisements stay prefixes. `Cost { warm, cold }` (`rs/moq-net/src/model/origin.rs:426`) is the route cost since [#2925](https://github.com/moq-dev/moq/pull/2925). [moq#3225](https://github.com/moq-dev/moq/pull/3225) moved a long way toward -this. An announcement carries a `Pattern` covering a set of paths. Rust -`announce::Update.pattern` and TypeScript `Announce.Update.pattern` use the -matcher directly, so callers explicitly select prefix-shaped claims when -they need a concrete broadcast path. +this, and #3770 settled the wire: an announcement carries a path prefix on +every protocol, and a consumer filters announced paths against its pattern +interest locally. The routing table exists too. `Consumer::request_broadcast` resolves a local broadcast first, then `best_server`: the longest covering prefix, filtered by @@ -69,11 +70,11 @@ announced it and cached per prefix in `ServeState.served` (`:764`). That is the lookup the old `origin::Dynamic` could not provide, and it is what resolve extends rather than replaces. -Request resolution, by contrast, is still prefix-only (`best_server` in -`rs/moq-net/src/model/origin.rs`). The pattern matcher itself exists: -`moq_net::{Pattern, Patterns, Segment}` and `Path.Pattern` / +Request resolution is prefix-only (`best_server` in +`rs/moq-net/src/model/origin.rs`) and stays that way. The pattern matcher +itself exists: `moq_net::{Pattern, Patterns, Segment}` and `Path.Pattern` / `Path.Patterns` in `js/net/src/path.ts` own the shared matching, containment, -specificity, and rebasing advertisements reuse. +specificity, and rebasing tokens and filters reuse. What is genuinely missing, beyond patterns themselves, is content identity. Announcement `Epoch` was specified into lite-06 by @@ -91,40 +92,37 @@ field. dialect is what tokens and the consume-side filter use, matched by the shared matcher, so nothing resembles a second grammar and nothing on the wire spells a wildcard. -- **Most specific pattern wins, and its refusal is final.** This is the rule +- **Longest prefix wins, and its refusal is final.** This is the rule routing already follows: `best_server` filters to the longest covering prefix before it compares cost, and the lite draft says the same, matching - longest-prefix-match wherever it appears. When several patterns match one - path, only the tier selected by the matcher's shared structural specificity is - consulted; equal-specificity patterns - form one pool that cost and the request hash order. A terminal refusal from - the winning tier IS the answer and never falls through to a less specific - pattern, so a transcoder refusing a path does not leak the request to the - archive's catch-all, and one unserved path still costs one round trip. The - capacity re-resolution below stays within the tier, refuser excluded. The - accepted consequence: an offline derivative (a recording of - `foo.hang/transcode.pro`) is not reachable through the catch-all, because - the more specific transcode pattern shadows it. -- **A wildcard is a POOL, not a competitor.** Several advertisers of one - pattern is the normal state, not a hazard: every transcode worker advertises - `**/transcode.pro` and takes a share. What distributes them is a + longest-prefix-match wherever it appears. Advertisers of one prefix form one + pool that cost and the request hash order. A terminal refusal from the + winning tier IS the answer and never falls through to a shorter prefix, so a + transcoder refusing a path does not leak the request to the archive's + catch-all, and one unserved path still costs one round trip. The capacity + re-resolution below stays within the tier, refuser excluded. The accepted + consequence: an offline derivative is not reachable through the catch-all + while a longer prefix covers it. +- **A claim is a POOL, not a competitor.** Several advertisers of one + prefix is the normal state, not a hazard: every transcode worker claims the + same prefix and takes a share. What distributes them is a deterministic hash of the REQUESTED path against each advertiser, so distinct - paths spread rather than one advertiser winning the whole pattern. + paths spread rather than one advertiser winning the whole prefix. Distribution is the requirement, not any particular pair: a correct hash may legitimately rank the same advertiser first for two given paths, so what must hold is that a large path set spreads and that one path always resolves the same way. Cost orders the pool first, which keeps work local and makes a distant advertiser the overflow rather than an equal peer. -- **A wildcard is priced, not special-cased.** Within a tier, route selection - stays one comparison on one metric. Concrete-versus-wildcard is not decided - by price at all: a concrete claim is maximally specific, so "most specific - wins" above already shadows every pattern behind it at any cost. The +- **A claim is priced, not special-cased.** Within a tier, route selection + stays one comparison on one metric. Concrete-versus-claim is not decided + by price at all: a concrete announcement is the longest prefix, so "longest + prefix wins" above already shadows every claim behind it at any cost. The accepted consequence follows from that rule's finality: a live session's - concrete claim shadows a healthy wildcard pool even when its service is + concrete claim shadows a healthy pool even when its service is broken, its terminal refusal does not fall through, and the shadow lasts exactly as long as the claiming session that carries it. - The seed still has a floor, because standby and running claims of equal - specificity do meet: a standby concrete claim (`with_cost(1000)` is the + The seed still has a floor, because standby and running claims of the same + prefix do meet: a standby concrete claim (`with_cost(1000)` is the existing per-broadcast convention) shares a tier with a running publisher's concrete announcement. The floor MUST exceed the deployment's enforced maximum charged-link count @@ -133,23 +131,21 @@ field. mesh starts a second encode of a stream it is already serving. That floor replaces the ad-hoc standby bias the moq.pro (downstream) transcode worker carries today. -- **One cost varint, not the pair.** `Cost` is `{ warm, cold }` because a relay - that is carrying a broadcast discounts the warm half. A wildcard carries - nothing and can never be warm, so the two halves are provably equal and the - message carries one value. `From` already means exactly this. - **A wildcard is a capability, not an inventory.** It advertises what the sender could serve, never that a given path exists. Refusal is how a specific path is denied. This is why an over-claiming advertisement is not a defect: the catch-all `**` is legal, and answering "not that one" is the mechanism. -- **Containment against the publish scope is what authorization checks.** An - advertised pattern MUST be contained by the sender's granted patterns (the - matcher's containment check). This handles literal-headed and leading-star - patterns identically and refuses any attempted widening rather than clamping - it. Fleet-wide services use the cluster identity; a customer service may - advertise only the exact set its own v1 grant contains. -- **Wildcards are visible to subscribers.** A subscriber sees every pattern - matching under its scope, rebased by the matcher's exact set-valued operation, - and duplicates combine into one. That is the point: it tells a client it may subscribe to +- **Overlap with the publish scope is what authorization checks.** An + advertised prefix MUST overlap the sender's granted patterns or it is + refused. A prefix wider than the grant is accepted, but it only routes + requests for paths the grant covers. Fleet-wide services use the cluster + identity; a customer service serves only what its own v1 grant contains. + Until [Advertise-only authorization](/quest/m1/processor/advertise-auth.md) + lands, the publish scope stands in for advertising; a credential with its own + advertise scope is checked against that instead. +- **Claims are visible to subscribers.** A subscriber sees every advertised + prefix under its scope, filtered locally like any other announcement. That + is the point: it tells a client it may subscribe to matching paths, and its withdrawal tells the client the capability is gone. This is what makes a lazily-produced rendition discoverable without the composer waiting for an announcement that only demand would produce. The @@ -199,71 +195,37 @@ field. ### Where derived output lives -Suffix matching lets a contribution be published where it is addressed, a -descendant of its source. This is the moq.pro (downstream) deployment shape, -and it is what the suffix pattern form exists for: - -```text -pid/foo.hang source -pid/foo.hang/catalog.pro combined catalog, edge-composed -pid/foo.hang/transcode.pro the transcode contribution -pid/foo.hang/transcribe.pro the transcription contribution -``` - -The `.pro` segment suffix is both the routed pattern and the platform-output -marker: `**/transcode.pro` routes every project's transcode demand to the -worker pool, and a segment ending in `.pro` is the one predicate every source -rule matcher excludes, so platform output is never recursively transcoded or -recorded. - -The rejected alternative publishes contributions at mirrored paths in reserved -namespaces (`.transcode//...`) hidden by an origin-consumer overlay, -because prefix-only matching needs the variable part of a path trailing. That -overlay is not a view transform: `pid/foo` and `.transcode/pid/foo` are -separate tree leaves with separate broadcast fronts, so it has to build a -logical front across roots that re-owns route selection, content identity, the -split-horizon guard, and splicing. The suffix pattern needs none of it while -keeping what the mirror buys: - -- **The grant needs no transform.** The customer addresses - `foo.hang/transcode.pro`, a descendant of `foo.hang`, so an existing grant - covers it by ordinary segment-aware prefix. No companion-grant rule, no - atomic `.pro/` scope, and nothing minted differently, which matters because - customer-issued tokens are minted by integrations the platform does not - control. -- **Metering is untouched.** The published path is rooted at `pid`, so the - platform's egress metering sees the customer path with no special case at - all. -- **The wildcard is fleet-wide.** The suffix is project-agnostic, so a worker - advertises once for every project rather than once per project, which is what - removes project discovery entirely. -- **Takeover is single-front.** A worker's concrete announcement lands at the - literal path the wildcard served, so wildcard-versus-concrete and - worker-versus-worker collisions are ordinary route selection at one tree node, - not a cross-root front. - -What the mirror buys and this deliberately gives up: a customer holding -`publish: ["pid/"]` CAN publish `foo.hang/transcode.pro` themselves, competing -with or forging platform output. Both then resolve at one path, cost decides, -and a live customer broadcast beats the worker's standby seed. That is confined -to their own namespace, self-sabotage of their own catalog, never another -project's, and is cheaper to allow and document than a reserved-name registry -or a token transform. The mirror's SUBSCRIBE-only overlay asymmetry existed to -prevent exactly this and goes with it. - -The archive is the same shape at the source path itself: a recording IS the -broadcast, served from storage through the catch-all pattern. A wildcard names -no generation, so a client that must distinguish recording generations reads -the catalog's archive entry ([archive](/quest/m1/archive/README.md)) rather -than announce state. +A prefix claim needs the variable part of a path trailing, so a fleet-wide +service claims its own prefix and mirrors the source path beneath it +(`.pro/transcode//foo.hang`, moq.pro's convention) rather than publishing +beneath the source. The source's catalog reaches the contribution through a +cross-broadcast reference. + +The leading `.` is deliberate. Existing customers on moq-lite-06 or older must +never see `.pro/` broadcasts, which could confuse their business logic. Those +versions cannot opt into hidden routes, so the relay never announces them +there. Hidden routes are a moq-lite-07 feature, so the player's covering check +(Demand, done on the line branch) opts into them and sees a claim only when +lite-07 is negotiated. A customer who wants transcodes upgrades, or +subscribes to the explicit `.pro//...` path, which works on any +version. Grants and metering are the deployment's; moq.pro's are in its +[wildcard questline](https://github.com/moq-dev/moq.pro/blob/main/quest/m2/wildcard/README.md). + +The archive serves the source path itself: a recording IS the broadcast, +served from storage through the root claim, and a live publisher's concrete +announcement shadows it. A claim names no generation, so a client that must +distinguish recording generations reads the catalog's archive entry +([archive](/quest/m1/archive/README.md)) rather than announce state. ## Related - [path-patterns](/quest/m1/path-patterns.md) - owns the pattern dialect - and the shared matcher advertisements reuse -- [archive](/quest/m1/archive/README.md) - an archive advertises the catch-all - pattern, and its catalog names the generations a wildcard cannot + and the shared matcher tokens and filters reuse +- [archive](/quest/m1/archive/README.md) - an archive claims the root, and its + catalog names the generations a claim cannot - [Cluster routing](/quest/m1/cluster-routing.md) - origin selection by cost - with an HRW tie-break, built on this line's specificity -- [Broadcast epochs](/quest/m1/broadcast-epoch/README.md) - derived output moves under the - source's `@` segment, which the suffix patterns still match + with an HRW tie-break, built on this line's longest-prefix rule +- [Broadcast epochs](/quest/m1/broadcast-epoch/README.md) - derived output + mirrors the source path, `@` segment included +- [Suffix announce](/quest/m3/suffix-announce.md) - moq-lite-only suffix + claims, deferred until a service prefix cannot express one diff --git a/quest/m1/auth/README.md b/quest/m1/auth/README.md index b95317d571..ab481d4558 100644 --- a/quest/m1/auth/README.md +++ b/quest/m1/auth/README.md @@ -120,7 +120,7 @@ existing lite-06 ALPN. ## Related -- [Pattern interest](/quest/m1/path-patterns.md) - moves AUTH's legacy wire prefixes to patterns along with ANNOUNCE_REQUEST +- [Pattern interest](/quest/m1/path-patterns.md) - moves AUTH's legacy grant prefixes to patterns; ANNOUNCE_REQUEST stays a prefix - [Expiring media grants](/quest/m1/processor/grant-lease.md) - a worker's lease renewal is a new in-band token - [P2P](/quest/m1/p2p/README.md) - the first consumer of hop-bound peer grants diff --git a/quest/m1/auth/lite.md b/quest/m1/auth/lite.md index 08b65bd7f6..6d54e56217 100644 --- a/quest/m1/auth/lite.md +++ b/quest/m1/auth/lite.md @@ -147,5 +147,5 @@ On main, additive. ## Related -- [Pattern interest](/quest/m1/path-patterns.md) - moves the prefix - fields here and in ANNOUNCE_REQUEST to patterns together +- [Pattern interest](/quest/m1/path-patterns.md) - moves AUTH's grant + prefixes to patterns; ANNOUNCE_REQUEST stays a prefix diff --git a/quest/m1/broadcast-epoch/README.md b/quest/m1/broadcast-epoch/README.md index 9e0ac40d40..337297edc3 100644 --- a/quest/m1/broadcast-epoch/README.md +++ b/quest/m1/broadcast-epoch/README.md @@ -40,10 +40,10 @@ Decided: bare `foo`, since a route covers its descendants, not its parent. A bare-name viewer behind one needs a publisher that opts out with the raw prefix route. Document this rather than promise it works. -- Derived output lives under the epoch it came from - (`pid/foo.hang/@e/transcode.pro`), so nested epochs must parse. This moves - the [wildcard](/quest/m0/wildcard/README.md) line's derived-output example - down one segment, and its suffix patterns still match. +- Derived output mirrors the epoch it came from + (`.pro/transcode//foo.hang/@e`, per the + [wildcard](/quest/m0/wildcard/README.md) line's derived-output layout), so + the service's prefix claim still covers it. This README owns: diff --git a/quest/m1/cluster-routing.md b/quest/m1/cluster-routing.md index c93f1f1df5..7345104e27 100644 --- a/quest/m1/cluster-routing.md +++ b/quest/m1/cluster-routing.md @@ -48,8 +48,8 @@ make MoQ's common case. origin id, and forwards along its shortest path. Distance compares cost, then hop count, so every hop strictly shortens it even across `?cost=0` links. That is a shortest path to a virtual node linked to every origin, so - it is loop-free whenever relays agree on the topology. Specificity still - ranks first, per [Wildcard](/quest/m0/wildcard/README.md). + it is loop-free whenever relays agree on the topology. The longest covering + prefix still ranks first, per [Wildcard](/quest/m0/wildcard/README.md). - The first relay's choice rides the SUBSCRIBE, and transit relays forward toward that origin by topology alone, never re-selecting. Re-selection against another existence view loops: a relay that lost a specific claim @@ -114,7 +114,7 @@ make MoQ's common case. ## Required - moq.pro workers stop electing on hop chains, reading the relay's local origin instead ([moq.pro voice-local-origin](https://github.com/moq-dev/moq.pro/blob/main/quest/m0/voice-local-origin.md)) -- [Wildcard](/quest/m0/wildcard/README.md) - the specificity, pool spread, and reply identity this selection builds on +- [Wildcard](/quest/m0/wildcard/README.md) - the longest-prefix rule, pool spread, and reply identity this selection builds on - moq.pro's routing simulator reports ([quest](https://github.com/moq-dev/moq.pro/blob/main/quest/m1/routing-simulator.md)) ## Related diff --git a/quest/m1/path-patterns.md b/quest/m1/path-patterns.md index 90b43fb304..691bcf1dc3 100644 --- a/quest/m1/path-patterns.md +++ b/quest/m1/path-patterns.md @@ -3,9 +3,9 @@ ## Goal Every predicate over a MoQ broadcast path uses one matcher. Tokens, -origin scopes, announce interests, public access rules, and wildcard -advertisements can express `pid/*/chat` and `**/transcode.pro` without -maintaining competing glob dialects. +origin scopes, announce interests, and public access rules can express +`pid/*/chat` and `**/*.hang` without maintaining competing glob dialects. +Routing is not a predicate here: advertisements stay prefixes. Literal paths remain coordinates, not sets. Roots, joins, exact broadcast names, URL paths, filesystem paths, and object-store keys keep their own @@ -98,5 +98,5 @@ matches, containment refusal, and old-version behavior. ## Related -- [Wildcard advertisements](/quest/m0/wildcard/README.md) - routing adopts the - matcher while retaining its own cost, pool, refusal, and resolution work +- [Wildcard advertisements](/quest/m0/wildcard/README.md) - routes on prefix + claims; the matcher only filters them against consume-side interest diff --git a/quest/m1/processor/README.md b/quest/m1/processor/README.md index 041fc85997..2adda56916 100644 --- a/quest/m1/processor/README.md +++ b/quest/m1/processor/README.md @@ -5,7 +5,9 @@ A customer runs a worker in its own environment, connects outbound to a MoQ deployment, reads only eligible source media, and publishes an on-demand contribution under the processor's own prefix, mirroring the source path -(for example `./`). The platform supplies +(for example `.pro//`, the +[wildcard](/quest/m0/wildcard/README.md) line's derived-output layout). The +platform supplies registration, scoped credentials, routing, demand, status, and usage visibility; it does not upload or execute customer code. diff --git a/quest/m1/processor/advertise-auth.md b/quest/m1/processor/advertise-auth.md index 1c8fc2bead..d3dc514541 100644 --- a/quest/m1/processor/advertise-auth.md +++ b/quest/m1/processor/advertise-auth.md @@ -21,15 +21,20 @@ every wire (Wildcard's decision) and suffix routing is dropped, so leading-star and suffix advertise patterns have nothing to authorize. Token claim patterns keep their suffix support for publish and subscribe. +Decided: an advertised prefix must overlap the advertise scope, not sit +inside it, and a wider claim only routes the requests the scope covers. This +matches how the relay authorizes advertisements today. + Preserve current customer credentials in the wire and authorization design: existing claims retain their current publish-implies-advertise behavior, while the new v1 claim separates the capabilities. Land the claims, SDK, origin-scope, relay authorization, and tests without combining the release or the moq.pro (downstream) pin rollout into this quest. -Cover containment, rebasing, missing versus empty advertise scope, v0 -compatibility, token revalidation, concrete announce, publish, FETCH, and a -prefix demand that receives only an exact short-lived publish grant. +Cover overlap and per-request filtering, rebasing, missing versus empty +advertise scope, v0 compatibility, token revalidation, concrete announce, +publish, FETCH, and a prefix demand that receives only an exact short-lived +publish grant. ## Required diff --git a/rs/moq-net/src/path/mod.rs b/rs/moq-net/src/path/mod.rs index 21acabb44e..ce3f0c930c 100644 --- a/rs/moq-net/src/path/mod.rs +++ b/rs/moq-net/src/path/mod.rs @@ -4,9 +4,9 @@ //! segment-aware prefix operations. [`Pattern`] describes a set of paths with //! wildcards, and [`Patterns`] is a union of them reduced by containment. The //! grammar and algebra live in [`moq-pattern`](moq_pattern); this module -//! re-exports them beside [`Path`] so grants, origin scopes, announce interests, -//! and wildcard advertisements can share one dialect. Literal path construction -//! and wire decoding retain their existing behavior. +//! re-exports them beside [`Path`] so grants, origin scopes, and announce +//! interests can share one dialect. Literal path construction and wire +//! decoding retain their existing behavior. pub use moq_pattern::{InvalidPattern, Pattern, Patterns, Segment, Specificity}; diff --git a/rs/moq-pattern/README.md b/rs/moq-pattern/README.md index bcaeddbcf4..5053dbf492 100644 --- a/rs/moq-pattern/README.md +++ b/rs/moq-pattern/README.md @@ -7,7 +7,7 @@ Exact path patterns for [Media over QUIC](https://moq.dev): grammar, matching, and set algebra. A pattern describes a set of broadcast paths. This crate provides a shared grammar -for tokens, origin scopes, announce interests, and wildcard advertisements. Integrating +for tokens, origin scopes, and announce interests. Integrating patterns into those consumers is separate work. Literal paths stay coordinates; `moq-net`'s `Path` and `@moq/net`'s path module keep construction, joins, and prefix operations. From 1ee10df96d03a2f37274b8e49e657b119d9ad172 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Mon, 28 Sep 2026 19:22:36 -0700 Subject: [PATCH 15/37] fix(kio): keep a lost waiter's recorded lists idempotent (#4422) Co-authored-by: Claude Opus 5.5 --- quest/m1/README.md | 1 - quest/m1/kio-waiter-lost.md | 26 -------------------------- rs/kio/src/waiter.rs | 28 +++++++++++++++++++++++----- 3 files changed, 23 insertions(+), 32 deletions(-) delete mode 100644 quest/m1/kio-waiter-lost.md diff --git a/quest/m1/README.md b/quest/m1/README.md index 755bdb1033..efbaf6b247 100644 --- a/quest/m1/README.md +++ b/quest/m1/README.md @@ -31,7 +31,6 @@ transport, benchmark tooling); worktrees isolate commits, not semantics. - [Raw stream codes](/quest/m1/raw-stream-codes.md) - raw QUIC stream resets and stops carry the application's code, not an HTTP/3-mapped one - [Live in apps](/quest/m1/announce-live-apps.md) - the demo and `@moq/room` show "no broadcasts" from the `live` marker, which waits for the first session on page load - [Watch refusal](/quest/m1/watch-refusal.md) - `` shows an origin refusal as an error instead of sitting offline -- [kio waiter overflow](/quest/m1/kio-waiter-lost.md) - a retained `Waiter` past 8 lists stops adding a duplicate entry to lists it already recorded - [moqsink recoverable errors](/quest/m1/moqsink-keyframe-latch.md) - a leading delta or a timestamp rewind drops frames until a keyframe instead of invalidating a moqsink pad - [Capture re-anchor](/quest/m1/capture-reanchor.md) - a repeating or restarting device clock never rewinds native capture during a fast backlog drain - [Splice edge cases](/quest/m1/splice-edges.md) - an unstamped successor, a pruned segment's boundary group, and a warm head during a takeover are each handled correctly diff --git a/quest/m1/kio-waiter-lost.md b/quest/m1/kio-waiter-lost.md deleted file mode 100644 index ae4e29321b..0000000000 --- a/quest/m1/kio-waiter-lost.md +++ /dev/null @@ -1,26 +0,0 @@ -# [XS] kio waiter overflow stays idempotent - -## Goal - -A `kio::Waiter` that has registered on more than 8 lists still registers for -free on each list it recorded, so a retained standalone `Waiter` polled -repeatedly no longer appends a duplicate entry to those lists every time. - -## Plan - -`Waiter::record` in `rs/kio/src/waiter.rs` returns early once `lost` is set, -before it matches the list's tag against the recorded slots. `Park` retires -such a waiter, so it never notices, but `Waiter` is public and a caller that -keeps one across polls grows every list it sits on, one live entry per -register until the list drains, and gets a duplicate wake for each. - -Match the recorded tags first and only then fall back on `lost`. This was -written with the regression test `overflow_keeps_recorded_lists_idempotent` -during https://github.com/moq-dev/moq/pull/4240, which squash-merged before -it was pushed, so it needs rewriting. Keep the recorded-tag probe the tight -common-case loop it is today; `rs/kio/benches/waiter.rs` shows whether it -moved. - -A list past the 8 slots is unrecorded and still has to append on every -register, which is the pre-#4240 behavior. Say so on `Waiter::register` so a -standalone caller knows the bound, rather than growing the record array. diff --git a/rs/kio/src/waiter.rs b/rs/kio/src/waiter.rs index 1d3755b138..518ac49893 100644 --- a/rs/kio/src/waiter.rs +++ b/rs/kio/src/waiter.rs @@ -73,6 +73,8 @@ impl Waiter { /// Register this waiter with a [`WaiterList`] for future notification. /// /// Delegates to [`WaiterList::register`], a no-op while the list still holds it. + /// Only the first 8 lists at once are remembered: a waiter kept across polls + /// that also sits on more appends a duplicate to those extras on every call. pub fn register(&self, list: &mut WaiterList) { list.register(self); } @@ -95,11 +97,6 @@ impl Waiter { /// still holds this waiter. A record of an older round of the same list is /// replaced, since the drain that ended that round took the entry with it. fn record(&self, tag: u64) -> bool { - // Retiring anyway, so skip the bookkeeping: duplicates die with the waiter. - if self.lost.load(Ordering::Relaxed) { - return true; - } - // The common case, kept a tight loop of its own. if self.parked.iter().any(|slot| slot.load(Ordering::Relaxed) == tag) { return false; @@ -975,6 +972,27 @@ mod tests { assert_eq!(list.entries.len(), 1, "a drained waiter must register again"); } + /// A standalone waiter past its records still skips the lists it recorded, even + /// after one of them drains, while an unrecorded list appends on every call. + #[test] + fn overflow_keeps_recorded_lists_idempotent() { + let waiter = Waiter::noop(); + let mut lists: Vec<_> = (0..=PARKED).map(|_| WaiterList::new()).collect(); + for _ in 0..3 { + for list in &mut lists { + waiter.register(list); + } + } + lists[0].wake(); + for _ in 0..3 { + waiter.register(&mut lists[0]); + } + for list in &lists[..PARKED] { + assert_eq!(list.entries.len(), 1, "a recorded list stacked a duplicate"); + } + assert_eq!(lists[PARKED].entries.len(), 3, "an unrecorded list cannot skip"); + } + /// The relay's shape: a task parks on a few lists, one of them wakes it, and the /// rest stay quiet. Retiring the waiter there cost an `Arc` per poll. #[test] From a0929c90b3b43ae73ccc08800c718a595f285d63 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Mon, 28 Sep 2026 19:26:02 -0700 Subject: [PATCH 16/37] fix(moq-gst): moqsrc waits for its session to end on stop (#4416) Co-authored-by: Claude Opus 5.5 --- quest/m1/README.md | 1 - quest/m1/moqsrc-stop.md | 27 --- rs/moq-gst/src/block.rs | 35 +++ rs/moq-gst/src/lib.rs | 3 + rs/moq-gst/src/sink/session.rs | 23 +- rs/moq-gst/src/source/imp.rs | 401 +++++++++++++++++++++++++++++---- 6 files changed, 394 insertions(+), 96 deletions(-) delete mode 100644 quest/m1/moqsrc-stop.md create mode 100644 rs/moq-gst/src/block.rs diff --git a/quest/m1/README.md b/quest/m1/README.md index efbaf6b247..b2541f5dfc 100644 --- a/quest/m1/README.md +++ b/quest/m1/README.md @@ -43,7 +43,6 @@ transport, benchmark tooling); worktrees isolate commits, not semantics. - [Watch video guards](/quest/m1/watch-video-guards.md) - promoting a video track holds the last picture, and an older group never reaches the codec between live deltas - [Watch decoder recovery](/quest/m1/watch-decoder-recovery.md) - one malformed packet rebuilds the audio or video decoder instead of ending playback - [Watch audio under CSP](/quest/m1/watch-worklet-file.md) - production builds ship the audio worklet as a file, so `script-src 'self'` pages play audio -- [moqsrc stop](/quest/m1/moqsrc-stop.md) - moqsrc's stop blocks until its session ends, without deadlocking on a blocked pad push - [More tests under load](/quest/m1/test-flakes-2.md) - the second round of load-only failures, fixed at the cause - [Auth outage clock](/quest/m1/auth-outage-clock.md) - the relay and moq-auth outage tests run on a paused clock again and assert both bounds of `expires` - [Legacy end overshoot](/quest/m1/legacy-end-overshoot.md) - browser playback survives a group that starts inside the previous group's estimated end diff --git a/quest/m1/moqsrc-stop.md b/quest/m1/moqsrc-stop.md deleted file mode 100644 index d3de732b61..0000000000 --- a/quest/m1/moqsrc-stop.md +++ /dev/null @@ -1,27 +0,0 @@ -# [S] moqsrc waits for its session to end on stop - -## Goal - -`moqsrc`'s PAUSED to READY transition returns only after its session task and -connection have ended, as `moqsink` does since -[#4074](https://github.com/moq-dev/moq/pull/4074). Today -`SessionController::stop` in `rs/moq-gst/src/source/imp.rs` signals shutdown -and spawns a task to await the join, so an application that sets NULL and -exits right away can race aws-lc's exit destructors against an in-flight -dial, the silent SIGABRT the sink fix closed. - -## Plan - -Blocking needs care: the session task pushes buffers into src pads -(`block_in_place(|| pad.push(buffer))`), and a push blocked downstream while -`stop` waits on the task is a deadlock. Unblock the pads first (set them -flushing, or otherwise make a pending push return) before waiting, the way -GStreamer sources normally stop their streaming threads. - -Reuse the sink's wait: it parks the thread on a waker instead of entering an -executor, and uses `block_in_place` when called from a multi-thread runtime -worker. Consider sharing it between the two elements rather than copying. - -Tests mirroring the sink's: stop returns after the connection closes, stop -from a runtime worker does not deadlock, stop inside another executor does -not panic, and stop while a push is blocked downstream returns. diff --git a/rs/moq-gst/src/block.rs b/rs/moq-gst/src/block.rs new file mode 100644 index 0000000000..b720492e20 --- /dev/null +++ b/rs/moq-gst/src/block.rs @@ -0,0 +1,35 @@ +use std::sync::Arc; +use std::task::{Context, Poll, Wake}; + +/// Run `future` to completion on the calling thread, for a state change that must not return early. +/// +/// Parks the thread between polls rather than entering an executor, which would panic under a +/// caller's own. A state change from a notify, bus sync, or pad handler runs on a runtime worker, +/// whose queue may hold the very task being waited on (in its LIFO slot, which no other worker can +/// steal), so a multi-thread worker is handed off first and keeps running while this thread blocks. +pub(crate) fn block_on(future: F) -> F::Output { + let wait = move || { + let waker = Arc::new(Unpark(std::thread::current())).into(); + let mut cx = Context::from_waker(&waker); + let mut future = std::pin::pin!(future); + loop { + if let Poll::Ready(output) = future.as_mut().poll(&mut cx) { + return output; + } + std::thread::park(); + } + }; + match tokio::runtime::Handle::try_current().map(|handle| handle.runtime_flavor()) { + Ok(tokio::runtime::RuntimeFlavor::MultiThread) => tokio::task::block_in_place(wait), + _ => wait(), + } +} + +/// Wakes the thread parked in [`block_on`]. +struct Unpark(std::thread::Thread); + +impl Wake for Unpark { + fn wake(self: Arc) { + self.0.unpark(); + } +} diff --git a/rs/moq-gst/src/lib.rs b/rs/moq-gst/src/lib.rs index 05ece3bf4e..57c518542e 100644 --- a/rs/moq-gst/src/lib.rs +++ b/rs/moq-gst/src/lib.rs @@ -1,8 +1,11 @@ use gst::glib; +mod block; mod sink; mod source; +use block::block_on; + /// The `moqsink` publish connection lifecycle, exposed as its read-only `status` property. pub use sink::ConnectionStatus; /// The media container selected by the `moqsink` `container` property. diff --git a/rs/moq-gst/src/sink/session.rs b/rs/moq-gst/src/sink/session.rs index 58ff9b107a..8b53fd1516 100644 --- a/rs/moq-gst/src/sink/session.rs +++ b/rs/moq-gst/src/sink/session.rs @@ -372,28 +372,7 @@ impl Session { // The status task goes first, so it never reports the loop's end as a failure. self.join.abort(); self.connection.abort(moq_net::Error::Cancel); - // Parks the thread rather than entering an executor, which would panic under a caller's own. - let closed = || { - let waiter = moq_net::kio::Waiter::new(Arc::new(Unpark(std::thread::current())).into()); - while self.connection.poll_closed(&waiter).is_pending() { - std::thread::park(); - } - }; - match tokio::runtime::Handle::try_current().map(|handle| handle.runtime_flavor()) { - // A state change from a notify or bus sync handler runs on a worker, whose queue may hold the - // loop's cancellation. Handing the worker off lets it run while this thread blocks. - Ok(tokio::runtime::RuntimeFlavor::MultiThread) => tokio::task::block_in_place(closed), - _ => closed(), - } - } -} - -/// Wakes a thread parked in [`Session::stop`]. -struct Unpark(std::thread::Thread); - -impl std::task::Wake for Unpark { - fn wake(self: Arc) { - self.0.unpark(); + let _ = crate::block_on(self.connection.closed()); } } diff --git a/rs/moq-gst/src/source/imp.rs b/rs/moq-gst/src/source/imp.rs index cadaadecdb..3d773be299 100644 --- a/rs/moq-gst/src/source/imp.rs +++ b/rs/moq-gst/src/source/imp.rs @@ -81,6 +81,12 @@ impl TrackKind { } } +tokio::task_local! { + /// The element whose session task or pump is running, so [`SessionController::stop`] can tell + /// it was reached from that element's own streaming context. + static STREAMING: glib::WeakRef; +} + /// The session task drives everything: it connects, follows the catalog, and /// runs one [`Pump`] per active rendition. The element just starts and /// stops it. No control-plane channel is needed because pumps push to their pads @@ -89,35 +95,116 @@ impl TrackKind { struct SessionController { shutdown: watch::Sender, join: tokio::task::JoinHandle<()>, + /// The one-shot connection, held so [`stop`](Self::stop) can end it and wait for it to close. + connection: moq_tokio::Connection, } impl SessionController { - fn start(settings: ResolvedSettings, element: glib::WeakRef) -> Self { - let (shutdown_tx, mut shutdown_rx) = watch::channel(false); - let join = RUNTIME.spawn(async move { - if let Err(err) = run_session(settings, element.clone(), &mut shutdown_rx).await + fn start(settings: ResolvedSettings, element: glib::WeakRef) -> Result { + let (connection, origin) = connect(&settings)?; + let task_connection = connection.clone(); + let task_element = element.clone(); + Ok(Self::spawn(connection, element, move |mut shutdown| async move { + run_session( + &task_connection, + origin, + settings.broadcast, + task_element, + &mut shutdown, + ) + .await + })) + } + + /// Run `session` as this element's session task, reporting its error on the bus. + fn spawn(connection: moq_tokio::Connection, element: glib::WeakRef, session: F) -> Self + where + F: FnOnce(watch::Receiver) -> Fut, + Fut: Future> + Send + 'static, + { + let (shutdown_tx, shutdown_rx) = watch::channel(false); + let task = session(shutdown_rx.clone()); + let task_connection = connection.clone(); + let join = RUNTIME.spawn(STREAMING.scope(element.clone(), async move { + let result = task.await; + // A broadcast that ends on its own releases the transport now rather than at stop. + task_connection.abort(moq_net::Error::Cancel); + // Stopping cuts the session short, which is not a failure. + if let Err(err) = result + && !*shutdown_rx.borrow() && let Some(obj) = element.upgrade() { gst::element_error!(obj, gst::CoreError::Failed, ("session error"), ["{err:?}"]); } - }); + })); Self { shutdown: shutdown_tx, join, + connection, } } - fn stop(self) { + /// Stop the session, returning once its pumps have removed their pads and the connection closed. + /// + /// A dial still running once the element reached NULL can outlive `main` (`gst-launch` exits right + /// after), and aws-lc aborts the process when a thread asks it for randomness after its exit + /// destructors ran. + fn stop(self, element: &super::MoqSrc) { let _ = self.shutdown.send(true); - RUNTIME.spawn(async move { - if let Err(err) = self.join.await { + + // A pump blocked in a push returns once its pad flushes, the same unlock a GStreamer source + // gives its streaming thread on stop. A push held up inside a downstream element is released + // by that element leaving PAUSED, which a pipeline does before it reaches its source. + for pad in element.src_pads() { + let _ = pad.set_active(false); + } + + // Reached from this element's own session task or pump (a bus sync or pad handler), waiting + // for the session would wait on the caller's own stack. GStreamer refuses the same for its + // sources; the connection still closes before this returns. + let own = STREAMING + .try_with(|streaming| streaming.upgrade().as_ref() == Some(element)) + .unwrap_or(false); + if own { + gst::warning!( + CAT, + obj = element, + "stopped from its own streaming thread, not waiting for the session to end" + ); + } + + let Self { join, connection, .. } = self; + crate::block_on(async move { + // The connection outlives the session so a stop never reads as a dropped connection. + if !own && let Err(err) = join.await { gst::warning!(CAT, "session task ended with error: {err:?}"); } + connection.abort(moq_net::Error::Cancel); + let _ = connection.closed().await; }); } } +/// Start the one-shot dial, returning it with the origin its announcements land in. +fn connect(settings: &ResolvedSettings) -> Result<(moq_tokio::Connection, moq_net::origin::Consumer)> { + let mut config = moq_tokio::connect::Config::default(); + config.tls.insecure = Some(settings.tls_disable_verify); + + // The origin and the connection loop are both spawned tasks. + let _rt = RUNTIME.enter(); + let origin = moq_tokio::origin::spawn(); + let consumer = origin.consume(); + // One-shot: the catalog subscription dies with the session anyway, so a background redial could + // not resurrect this run. A drop surfaces as the catalog closing and the session winding down. + let connection = config + .init(Default::default())? + .with_subscriber(origin) + .with_reconnect(false) + .connect(settings.url.clone()); + Ok((connection, consumer)) +} + #[derive(Default)] pub struct MoqSrc { settings: Mutex, @@ -266,14 +353,15 @@ impl ElementImpl for MoqSrc { impl MoqSrc { fn start_session(&self) -> Result<()> { let settings = ResolvedSettings::try_from(self.settings.lock().unwrap().clone())?; - let session = SessionController::start(settings, self.obj().downgrade()); + let session = SessionController::start(settings, self.obj().downgrade())?; *self.session.lock().unwrap() = Some(session); Ok(()) } fn stop_session(&self) { - if let Some(session) = self.session.lock().unwrap().take() { - session.stop(); + let session = self.session.lock().unwrap().take(); + if let Some(session) = session { + session.stop(&self.obj()); } } } @@ -347,31 +435,23 @@ impl ActiveTrack { } async fn run_session( - settings: ResolvedSettings, + connection: &moq_tokio::Connection, + origin: moq_net::origin::Consumer, + broadcast: String, element: glib::WeakRef, shutdown: &mut watch::Receiver, ) -> Result<()> { - let mut config = moq_tokio::connect::Config::default(); - config.tls.insecure = Some(settings.tls_disable_verify); - - let origin = moq_tokio::origin::spawn(); - let origin_consumer = origin.consume(); - let client = config.init(Default::default())?.with_subscriber(origin); - - // One-shot: the catalog subscription below dies with the session anyway, so a - // background redial could not resurrect this run. A drop surfaces as the - // catalog closing and the loop below winding down. - let _connection = client - .with_reconnect(false) - .connect(settings.url.clone()) - .established() - .await?; + // Stop closes the connection only after the session ends, so the dial races shutdown here. + tokio::select! { + established = moq_net::kio::wait(|waiter| connection.poll_established(waiter)) => established?, + _ = shutdown.changed() => return Ok(()), + } // Wait for a route to cover the broadcast. Synchronous lookup would race the gossip // of announcements that happens after the session is established. - tracing::info!(broadcast = %settings.broadcast, "waiting for broadcast to be announced"); + tracing::info!(%broadcast, "waiting for broadcast to be announced"); let broadcast = tokio::select! { - routed = origin_consumer.routed_broadcast(&settings.broadcast) => { + routed = origin.routed_broadcast(&broadcast) => { routed.context("broadcast unavailable")? } _ = shutdown.changed() => return Ok(()), @@ -388,10 +468,12 @@ async fn follow_catalog( element: glib::WeakRef, shutdown: &mut watch::Receiver, ) -> Result<()> { - let catalog_track = broadcast - .track(hang::catalog::Catalog::DEFAULT_NAME)? - .subscribe(hang::catalog::Catalog::default_subscription()) - .await?; + let catalog_track = broadcast.track(hang::catalog::Catalog::DEFAULT_NAME)?; + // A publisher that never answers would otherwise hold stop until the connection closes. + let catalog_track = tokio::select! { + track = catalog_track.subscribe(hang::catalog::Catalog::default_subscription()) => track?, + _ = shutdown.changed() => return Ok(()), + }; let mut catalog_consumer = moq_mux::catalog::hang::Consumer::new(catalog_track); // Follow the catalog for the whole session and reconcile our pumps against every update, @@ -422,7 +504,7 @@ async fn follow_catalog( // returning None) while we wait for the remaining pumps to drain. next = catalog_consumer.next(), if !catalog_closed => { match next? { - Some(catalog) => reconcile(&catalog, &mut active, &mut pumps, &broadcast, &element), + Some(catalog) => reconcile(&catalog, &mut active, &mut pumps, &broadcast, &element, shutdown), // Catalog track closed. Don't cancel the pumps: let each reach its // natural Ok(None) -> EOS end so downstream sees a clean EOS rather than a // bare pad drop. We just stop reconciling and wait for them to drain. @@ -466,6 +548,7 @@ fn reconcile( pumps: &mut tokio::task::JoinSet<()>, broadcast: &moq_net::broadcast::Consumer, element: &glib::WeakRef, + shutdown: &watch::Receiver, ) { struct Desired { kind: TrackKind, @@ -556,17 +639,21 @@ fn reconcile( let (cancel_tx, cancel_rx) = watch::channel(false); let state = Arc::new(PumpState::new()); let task = pumps.spawn_on( - Pump { - element: element.clone(), - kind: d.kind, - name: name.clone(), - caps: d.shape.caps.clone(), - track, - container, - state: state.clone(), - cancel: cancel_rx, - } - .run(), + STREAMING.scope( + element.clone(), + Pump { + element: element.clone(), + kind: d.kind, + name: name.clone(), + caps: d.shape.caps.clone(), + track, + container, + state: state.clone(), + cancel: cancel_rx, + shutdown: shutdown.clone(), + } + .run(), + ), RUNTIME.handle(), ); @@ -650,6 +737,8 @@ struct Pump { /// Shared with this rendition's [`ActiveTrack::state`]. state: Arc, cancel: watch::Receiver, + /// The session's shutdown, which [`SessionController::stop`] sets before it flushes the pads. + shutdown: watch::Receiver, } impl Pump { @@ -666,6 +755,7 @@ impl Pump { container, state, mut cancel, + shutdown, } = self; // Resolves once the publisher answers, with the track info or with an error (which is // what ending a broadcast produces for a name nobody served). A publisher that answers @@ -700,6 +790,11 @@ impl Pump { let Some(pad) = create_pad(&element, &descriptor, &caps) else { return; }; + // Stop flushes only the pads it finds, and this one may have been added just after, while + // this pump's cancel is still on its way. Flushing it here keeps a push from blocking stop. + if *shutdown.borrow() { + let _ = pad.set_active(false); + } let mut reference_ts = None; loop { @@ -995,10 +1090,11 @@ mod session_tests { use gst::glib; use gst::prelude::*; + use gst::subclass::prelude::*; use hang::catalog::{AudioCodec, AudioConfig, Container, H264, VideoConfig}; use tokio::sync::watch; - use super::{NEXT_VIDEO_PAD_ID, follow_catalog}; + use super::{NEXT_VIDEO_PAD_ID, ResolvedSettings, SessionController, follow_catalog}; /// The pad-id counters are process-global, so a test reading one has to be the only test /// allocating while it runs. `cargo test` shares a process across tests (nextest doesn't), @@ -1584,4 +1680,217 @@ mod session_tests { let _ = shutdown.send(true); super::RUNTIME.block_on(session).unwrap().unwrap(); } + + /// Settings whose dial goes nowhere, so a stop catches it in flight. + fn unreachable() -> ResolvedSettings { + ResolvedSettings { + url: "https://127.0.0.1:1".parse().unwrap(), + broadcast: "test".into(), + tls_disable_verify: false, + } + } + + fn is_closed(connection: &moq_tokio::Connection) -> bool { + connection.poll_closed(&moq_net::kio::Waiter::noop()).is_ready() + } + + fn started(element: &super::super::MoqSrc) -> (SessionController, moq_tokio::Connection) { + let session = SessionController::start(unreachable(), element.downgrade()).unwrap(); + let connection = session.connection.clone(); + (session, connection) + } + + /// A session following `broadcast` in place of a relay's, beside a dial for stop to end. + fn serve( + element: &super::super::MoqSrc, + broadcast: moq_net::broadcast::Consumer, + ) -> (SessionController, moq_tokio::Connection) { + let (connection, _) = super::connect(&unreachable()).unwrap(); + let weak = element.downgrade(); + let session = SessionController::spawn( + connection.clone(), + element.downgrade(), + move |mut shutdown| async move { follow_catalog(broadcast, weak, &mut shutdown).await }, + ); + (session, connection) + } + + /// A broadcast with one video rendition, and the producer that feeds it. + fn video_broadcast() -> ( + moq_net::broadcast::Producer, + moq_mux::catalog::Producer, + moq_mux::container::Producer, + ) { + let mut broadcast = moq_net::broadcast::Info::new().produce(); + let mut catalog = moq_mux::catalog::Producer::new(&mut broadcast, moq_mux::catalog::Config::default()).unwrap(); + let video = broadcast + .create_track("video", hang::container::track_info(hang::catalog::PRIORITY.video)) + .unwrap(); + { + let mut guard = catalog.modify().unwrap(); + guard.video.renditions = BTreeMap::from([("video".to_string(), video_rendition())]); + } + let producer = moq_mux::container::Producer::new( + video, + moq_mux::catalog::hang::Container::Legacy(moq_mux::container::Kind::Video), + ); + (broadcast, catalog, producer) + } + + fn keyframe() -> moq_mux::container::Frame { + moq_mux::container::Frame { + timestamp: moq_net::Timestamp::from_micros(0).unwrap(), + payload: bytes::Bytes::from_static(&[0; 64]), + keyframe: true, + duration: None, + } + } + + // A dial still running once the element reached NULL can outlive `main`, and aws-lc aborts the + // process when a thread asks it for randomness after its exit destructors ran. + #[test] + fn stop_returns_after_the_connection_closes() { + let element = element(); + let (session, connection) = started(&element); + session.stop(&element); + assert!(is_closed(&connection)); + } + + /// The state change itself is what an application waits on, so it has to be the one that stops. + #[test] + fn paused_to_ready_returns_after_the_connection_closes() { + let element = element(); + element.set_property("url", "https://127.0.0.1:1"); + element.set_property("broadcast", "test"); + element.set_state(gst::State::Paused).expect("start the source"); + let connection = element + .imp() + .session + .lock() + .unwrap() + .as_ref() + .unwrap() + .connection + .clone(); + element.set_state(gst::State::Ready).expect("stop the source"); + assert!(is_closed(&connection)); + } + + // A notify or bus sync handler can stop the element from a runtime worker. With the tasks parked, + // their wakeups land in that worker's own LIFO slot, which no other worker can steal, so blocking + // the worker outright would never let them run. + #[test] + fn stop_from_a_runtime_worker_does_not_deadlock() { + let element = element(); + let (session, connection) = started(&element); + let metrics = super::RUNTIME.metrics(); + // An odd count means that worker is parked. + while metrics.global_queue_depth() > 0 + || (0..metrics.num_workers()).any(|worker| metrics.worker_park_unpark_count(worker).is_multiple_of(2)) + { + std::thread::yield_now(); + } + let stopping = element.clone(); + super::RUNTIME + .block_on(super::RUNTIME.spawn(async move { session.stop(&stopping) })) + .unwrap(); + assert!(is_closed(&connection)); + } + + // An application driving its own executor can reach NULL from inside it, and executors refuse to nest. + #[test] + fn stop_inside_another_executor() { + let element = element(); + let (session, connection) = started(&element); + futures::executor::block_on(async { session.stop(&element) }); + assert!(is_closed(&connection)); + } + + /// Stop waits for every pump to remove its pad, so a pump held in a push has to be let go + /// first, or stop never returns. + #[test] + fn stop_releases_a_blocked_push() { + let _pad_ids = pad_ids(); + let element = element(); + let (broadcast, _catalog, mut producer) = video_broadcast(); + let (session, connection) = serve(&element, broadcast.consume()); + + let pad = await_pad(&element, "video_"); + let (blocked, reached) = std::sync::mpsc::channel(); + pad.add_probe(gst::PadProbeType::BLOCK | gst::PadProbeType::BUFFER, move |_, _| { + let _ = blocked.send(()); + gst::PadProbeReturn::Ok + }); + producer.write(keyframe()).unwrap(); + reached + .recv_timeout(Duration::from_secs(10)) + .expect("the frame never reached the pad"); + + session.stop(&element); + assert!(pad.parent().is_none(), "the pad outlived stop"); + assert!(is_closed(&connection)); + } + + /// A pump whose subscription resolves while stop flushes the pads adds its own just after, with + /// its cancel still on the way. A push on that pad must not block stop either. + #[test] + fn a_pad_added_after_stop_flushed_does_not_block() { + let _pad_ids = pad_ids(); + let element = element(); + let (broadcast, _catalog, mut producer) = video_broadcast(); + element.connect_pad_added(|_, pad| { + pad.add_probe(gst::PadProbeType::BLOCK | gst::PadProbeType::BUFFER, |_, _| { + gst::PadProbeReturn::Ok + }); + }); + producer.write(keyframe()).unwrap(); + + let (_cancel, cancel) = watch::channel(false); + let (_shutdown, shutdown) = watch::channel(true); + let pump = super::Pump { + element: element.downgrade(), + kind: super::TrackKind::Video, + name: "video".into(), + caps: gst::Caps::new_empty_simple("video/x-h264"), + track: broadcast.consume().track("video").unwrap(), + container: moq_mux::catalog::hang::Container::Legacy(moq_mux::container::Kind::Video), + state: std::sync::Arc::new(super::PumpState::new()), + cancel, + shutdown, + }; + super::RUNTIME + .block_on(async { tokio::time::timeout(Duration::from_secs(10), super::RUNTIME.spawn(pump.run())).await }) + .expect("a push on a pad added after the flush blocked") + .unwrap(); + assert!(pads(&element, "video_").is_empty(), "the pad outlived its pump"); + } + + /// A bus sync or pad handler can stop the element from inside a pump's push. Waiting for the + /// session there would wait on that pump, so stop settles for the connection closing. + #[test] + fn stop_from_its_own_pump_does_not_deadlock() { + let _pad_ids = pad_ids(); + let element = element(); + let (broadcast, _catalog, mut producer) = video_broadcast(); + let (session, connection) = serve(&element, broadcast.consume()); + *element.imp().session.lock().unwrap() = Some(session); + + let pad = await_pad(&element, "video_"); + let (stopped, returned) = std::sync::mpsc::channel(); + pad.add_probe(gst::PadProbeType::BUFFER, move |pad, _| { + let element = pad + .parent_element() + .unwrap() + .downcast::() + .unwrap(); + element.imp().stop_session(); + let _ = stopped.send(()); + gst::PadProbeReturn::Drop + }); + producer.write(keyframe()).unwrap(); + returned + .recv_timeout(Duration::from_secs(10)) + .expect("stop from the pump never returned"); + assert!(is_closed(&connection)); + } } From 8e13f46db2f510b6f549b0adc18dda21ed85aaf8 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Mon, 28 Sep 2026 19:40:46 -0700 Subject: [PATCH 17/37] quest(export-linger): tell a finished TS track from a removed one (#4401) Co-authored-by: Claude Opus 5.5 --- quest/m1/export-linger.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/quest/m1/export-linger.md b/quest/m1/export-linger.md index fe0f3a0808..7c590f12a8 100644 --- a/quest/m1/export-linger.md +++ b/quest/m1/export-linger.md @@ -27,6 +27,11 @@ once and exits 1 with `json: dropped`, even while `moq_tokio` is mid-reconnect restart refuses a non-zero `--linger` at startup. - `drive()` currently maps every clean task end and every error the same way; thread the FIN versus drop distinction through to the exit code. +- The fixed-layout exporters report a track that finished before the catalog + as removed: TS as `TS track layout changed ... removed` (reproducible with + `test/ts/run.sh --pair`), FLV as `FLV track ... removed mid-stream`. A clean + end can exit 1 before linger applies. A finished track is not a removed + one; tell them apart in both, with a regression test each (#3926). - `doc/bin/cli.md`: document `--linger` and the exit codes next to `export --max-age`. - Tests: a relay-backed CLI test that restarts the publisher within the linger From a31afc45c77f2f606f20c6100bc4d0bfab610b77 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Mon, 28 Sep 2026 19:42:17 -0700 Subject: [PATCH 18/37] quest: IETF peers without MoQ Hidden see hidden namespaces (#4394) Co-authored-by: Claude Opus 5.5 --- quest/m1/README.md | 1 + quest/m1/ietf-hidden-default.md | 53 +++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+) create mode 100644 quest/m1/ietf-hidden-default.md diff --git a/quest/m1/README.md b/quest/m1/README.md index b2541f5dfc..86ed6642d6 100644 --- a/quest/m1/README.md +++ b/quest/m1/README.md @@ -30,6 +30,7 @@ transport, benchmark tooling); worktrees isolate commits, not semantics. - [Close codes](/quest/m1/close-codes.md) - a client sees the peer's application close code over WebSocket and raw QUIC, like WebTransport - [Raw stream codes](/quest/m1/raw-stream-codes.md) - raw QUIC stream resets and stops carry the application's code, not an HTTP/3-mapped one - [Live in apps](/quest/m1/announce-live-apps.md) - the demo and `@moq/room` show "no broadcasts" from the `live` marker, which waits for the first session on page load +- [IETF hidden default](/quest/m1/ietf-hidden-default.md) - a moq-transport peer without the MoQ Hidden option is advertised hidden namespaces; one with it filters per subscription - [Watch refusal](/quest/m1/watch-refusal.md) - `` shows an origin refusal as an error instead of sitting offline - [moqsink recoverable errors](/quest/m1/moqsink-keyframe-latch.md) - a leading delta or a timestamp rewind drops frames until a keyframe instead of invalidating a moqsink pad - [Capture re-anchor](/quest/m1/capture-reanchor.md) - a repeating or restarting device clock never rewinds native capture during a fast backlog drain diff --git a/quest/m1/ietf-hidden-default.md b/quest/m1/ietf-hidden-default.md new file mode 100644 index 0000000000..0004b8c584 --- /dev/null +++ b/quest/m1/ietf-hidden-default.md @@ -0,0 +1,53 @@ +# [S] IETF peers without MoQ Hidden see hidden namespaces + +## Goal + +A moq-transport peer that did not declare the MoQ Hidden setup option is +advertised every namespace, hidden ones included. A peer that declared it +keeps today's behavior: hidden namespaces are left out unless its +SUBSCRIBE_NAMESPACE opts in. Rust and JS publishers agree, and +`drafts/draft-lcurley-moq-hidden.md` says so. + +Today the rule applies whether or not the peer declared the option, so a +third-party IETF client can never see a hidden namespace except by naming its +dot segment in the prefix. + +## Plan + +Maintainer decisions: + +- Hiding exists to keep existing moq-lite customers (lite-06 and older) from + seeing new `.pro/` broadcasts. None of them use an IETF client, and the + extension will not be adopted by the IETF draft, so an IETF peer that + never heard of it gets everything. A peer that declares it chooses per + subscription whether to filter on the wire. +- Our own IETF clients already declare the option and filter locally when a + reader does not ask for hidden, so their behavior does not change. +- moq-lite is unchanged: lite-06 and older never see hidden paths, and lite-07 + opts in. + +Guidance: + +- Both publishers, both the SUBSCRIBE_NAMESPACE path and the unsolicited + PUBLISH_NAMESPACE path, and draft-14/15 (PUBLISH_NAMESPACE requests) as well + as 16+ (inline NAMESPACE entries). +- Rewrite the draft's "applies whether or not the peer declared the option" + rule and keep `just drafts check` green. `doc/concept/moq-lite.md` also + says a peer without the extension never discovers hidden routes, as do the + module docs in `rs/moq-net/src/ietf/hidden.rs` and `js/net/src/ietf/hidden.ts`; + update them in the same change. +- It is a wire behavior change in both languages, so run + `just test interop --all` before landing. +- Tests: an undeclared peer sees hidden namespaces, a declared peer without + the parameter does not, on at least one pre-16 and one 16+ draft. Also fill + the gaps where nothing is tested today: draft-14/15 with hidden, JS IETF + without solicitation, and local filtering when a peer sends hidden paths the + reader did not ask for. +- Hiddenness is measured from the requested prefix, as the draft already + defines it and JS already does. Rust measures from the publish origin's + scope heads instead, so a publish scope like `.stats/**` exposes + `.stats/node` to a request for the empty prefix; bring Rust in line and test + that case in both languages. + +Public API: none. Wire: behavior change for IETF peers that did not declare +MoQ Hidden. From 42ea3eb37a9bcebe610cb85c3d99a2c1ea08f6bc Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Mon, 28 Sep 2026 19:44:00 -0700 Subject: [PATCH 19/37] chore(quest): graceful close in bindings and the moq fetch flake cause (#4432) Co-authored-by: Claude Opus 5.5 --- quest/m1/README.md | 1 + quest/m1/bindings-graceful-close.md | 15 +++++++++++++++ quest/m1/test-flakes-2.md | 12 +++++++++++- 3 files changed, 27 insertions(+), 1 deletion(-) create mode 100644 quest/m1/bindings-graceful-close.md diff --git a/quest/m1/README.md b/quest/m1/README.md index 86ed6642d6..f5ecede3ec 100644 --- a/quest/m1/README.md +++ b/quest/m1/README.md @@ -27,6 +27,7 @@ transport, benchmark tooling); worktrees isolate commits, not semantics. - [Track demand](/quest/m1/track-demand.md) - Rust and JS watch a track's subscribers through `demand()` alone - [Error messages](/quest/m1/error-display.md) - Python, Go, and Dart print `MoqError` with Rust's message, as Kotlin and Swift do - [Session close](/quest/m1/session-close.md) - a graceful session end withdraws announces and waits one second for the ack +- [Graceful close in bindings](/quest/m1/bindings-graceful-close.md) - on dev, `shutdown` drains a session in moq-ffi and every wrapper like Rust, so the wrappers keep the tail of a publish - [Close codes](/quest/m1/close-codes.md) - a client sees the peer's application close code over WebSocket and raw QUIC, like WebTransport - [Raw stream codes](/quest/m1/raw-stream-codes.md) - raw QUIC stream resets and stops carry the application's code, not an HTTP/3-mapped one - [Live in apps](/quest/m1/announce-live-apps.md) - the demo and `@moq/room` show "no broadcasts" from the `live` marker, which waits for the first session on page load diff --git a/quest/m1/bindings-graceful-close.md b/quest/m1/bindings-graceful-close.md new file mode 100644 index 0000000000..adfa57fac5 --- /dev/null +++ b/quest/m1/bindings-graceful-close.md @@ -0,0 +1,15 @@ +# [M] Bindings close sessions gracefully + +## Goal + +On dev, `shutdown` in moq-ffi and every wrapper (py, swift, kt, go, dart, and the generated C/C++) drains the session the way `moq_net::Session::close` and `moq_tokio::Connection::close` do. Finished tracks deliver their last groups and FIN before the session ends, bounded by the same deadline. The language bindings stop losing the tail of a publish when they stop. OBS gets this only once it moves off hand-written libmoq onto the generated C++, which [C++ through moq-ffi](/quest/m1/cpp/README.md) owns; libmoq takes no more shutdown work. + +## Plan + +`Session::close` and `Connection::close` landed with drain-before-close (#4430). Today the FFI session (`rs/moq-ffi/src/session.rs`) only has `cancel(code)` and `shutdown()`, and `shutdown` is just `cancel(0)`, which ends it immediately. + +Decision (maintainer, 2026-09-28): `shutdown` becomes the async draining close in every binding. The name can't be `close`, because UniFFI's Kotlin generator already emits `AutoCloseable.close()` to release the handle, and `shutdown` already promises a graceful shutdown. Turning it async is breaking, so this targets `dev`; no second method, per the no-shim rule in AGENTS.md. `cancel` stays the immediate path. Like `Session::close`, `shutdown` is fallible: a drain the peer does not acknowledge in time surfaces as a timeout error through each binding's error mechanism rather than a silent success. Test both paths end to end through moq-ffi: a final group and clean track end reach the peer after `shutdown` returns, and an unacknowledged drain returns the timeout. + +Mirror `shutdown` in each wrapper and follow the Cross-Package Sync table in AGENTS.md, including `doc/lib/*`, except `rs/libmoq`, whose shutdown work is abandoned per [Generated C bindings](/quest/m1/c/README.md). IETF sessions still close at once until [IETF drain before close](/quest/m2/ietf-drain-before-close.md) lands. + +Public API: breaking on dev, `shutdown` becomes async, drains, and returns the close error in moq-ffi and every wrapper. Wire: no format change; a binding's shutdown now sends queued data and FIN before closing instead of discarding them. diff --git a/quest/m1/test-flakes-2.md b/quest/m1/test-flakes-2.md index 8cc9502a74..9e7520611f 100644 --- a/quest/m1/test-flakes-2.md +++ b/quest/m1/test-flakes-2.md @@ -8,7 +8,10 @@ reliably, each fixed at its cause, never by raising a timeout or adding a retry. - moq-cli `fetch::tests::a_frame_read_times_out` asserts on a 500 ms wall - deadline ([#4084](https://github.com/moq-dev/moq/pull/4084)). + deadline ([#4084](https://github.com/moq-dev/moq/pull/4084)). The cause + (#4431): `rs/moq-cli/src/fetch.rs` wraps the whole run in one + `timeout_at(deadline, ...)`, so connect, TLS, announce, and subscribe share + the budget with the read, and under load setup alone can spend it. - moq-cli `complete::tests::a_stage_broadcast_picks_the_catalog_to_read` ([#4084](https://github.com/moq-dev/moq/pull/4084)) and `the_catalog_format_on_the_line_is_honored` @@ -36,6 +39,13 @@ retry. subscribe tests already use `#[tokio::test(start_paused = true)]`), or assert on an event instead of a deadline. If a test is slow under load because the code under test is slow, fix that. +- Fetch timeout: test on a paused clock (maintainer decision, 2026-09-28). + The fixture runs real sockets against an in-process relay, where a paused + clock fires QUIC timers while packets are in flight, so first make the + timers mockable: run the fixture over an in-memory transport, or drive + noq's timers from the test clock, whichever is smaller. Keep the one + absolute 30 s deadline, matching the relay's `/fetch`; on a paused clock + setup costs no time, so it can't spend the read's budget. - WARN counting: capture per test (a scoped subscriber or a filter on the test's own span) instead of a process-global count. - The race test shares one port only so both transports sit behind one URL. From 22f6d68f5c7f56189d625ec311966d0d537f4512 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Mon, 28 Sep 2026 19:56:28 -0700 Subject: [PATCH 20/37] fix(sock): resolve an ephemeral reuseport group's port with a plain bind (#4409) Co-authored-by: Claude Opus 5.5 --- doc/bin/relay/config.md | 4 +- rs/moq-relay/src/uring.rs | 3 +- rs/moq-sock/src/shard.rs | 121 +++++++++++++++++++------------ rs/moq-tokio/src/worker/group.rs | 4 +- rs/moq-tokio/tests/worker.rs | 8 +- 5 files changed, 85 insertions(+), 55 deletions(-) diff --git a/doc/bin/relay/config.md b/doc/bin/relay/config.md index d5febea5b8..6cec124121 100644 --- a/doc/bin/relay/config.md +++ b/doc/bin/relay/config.md @@ -77,8 +77,8 @@ io_uring = false # Drive them with io_uring instead of tokio ``` Packets are steered by connection ID, so a client that migrates stays with its -worker. The group shares one port, including an ephemeral (zero) port: the -first worker binds it and the rest join that port. Use an explicit port unless +worker. The group shares one port, including an ephemeral (zero) port, which is +resolved once and joined by every worker. Use an explicit port unless something reads the bound address at startup. `workers` needs the `noq` feature and real certificate files rather than `tls.generate`. A build without QUIC rejects `workers` instead of diff --git a/rs/moq-relay/src/uring.rs b/rs/moq-relay/src/uring.rs index 4dae572f8d..2583ac43de 100644 --- a/rs/moq-relay/src/uring.rs +++ b/rs/moq-relay/src/uring.rs @@ -197,8 +197,7 @@ impl Workers { while let Some(member) = group.member().context("failed to clone a reuseport member")? { members.push(member); } - // Whatever the first member bound, which is the requested address unless - // it asked for an ephemeral port. + // The requested address, with an ephemeral port resolved. let addr = group.addr(); // The moq-lite ALPNs this listener speaks: the operator's version diff --git a/rs/moq-sock/src/shard.rs b/rs/moq-sock/src/shard.rs index 9538159438..adec7f7ecd 100644 --- a/rs/moq-sock/src/shard.rs +++ b/rs/moq-sock/src/shard.rs @@ -129,6 +129,10 @@ pub enum Error { /// The port both groups asked for. port: u16, }, + + /// No port could be resolved for an ephemeral request. + #[error("failed to resolve an ephemeral port")] + Resolve(#[source] io::Error), } /// A `SO_REUSEPORT` group being formed. @@ -173,21 +177,30 @@ impl Group { /// the callers that size a group from a worker count would otherwise each /// clamp it themselves. /// - /// An ephemeral port (`0`) is bound by the first member and shared by the - /// rest, so a group can take whatever port the kernel hands out. Nothing is - /// locked in that case: no second group can be aiming at a port that cannot - /// be named in advance. + /// An ephemeral port (`0`) is resolved here, with a plain bind that is + /// dropped again, and the group then takes that port like a named one. + /// Never a reuseport bind of port `0`: Linux counts a port held only by + /// same-UID reuseport sockets as free for one, so the kernel could hand + /// back a port another group holds and this one would silently join it. A + /// plain bind conflicts with every holder. Another socket taking the port + /// before the first member binds fails that bind with + /// [`io::ErrorKind::AddrInUse`]. pub fn acquire(addr: SocketAddr, count: u16) -> Result { let count = count.max(1); if count > MAX_SHARDS { return Err(Error::Count { count, max: MAX_SHARDS }); } - let lock = match addr.port() { - 0 => None, - port => Lock::acquire(port).map_err(|_| Error::Overlap { port })?, + let addr = match addr.port() { + 0 => crate::bind::udp(crate::bind::Udp::new(addr)) + .and_then(|socket| socket.local_addr()) + .map_err(Error::Resolve)?, + _ => addr, }; + let port = addr.port(); + let lock = Lock::acquire(port).map_err(|_| Error::Overlap { port })?; + Ok(Self { count, next: 0, @@ -200,10 +213,9 @@ impl Group { self.count } - /// The address the group holds: what was asked for until the first member - /// binds, and what it actually bound from there on. + /// The address the group holds, with an ephemeral port already resolved. pub fn addr(&self) -> SocketAddr { - self.state.progress().addr + self.state.addr } /// The next slot to bind, or `None` once every slot has been handed out. @@ -259,8 +271,7 @@ impl Group { /// One member's claim on a slot in a [`Group`], which binding spends. /// /// Send it wherever the socket is served, a worker's own thread included. It -/// carries the group's address, so every member holds one port whatever the -/// caller thought it asked for, and its share of the group's port lock, so a +/// carries the group's address and its share of the group's port lock, so a /// member still waiting to bind cannot be raced by a second group even if its /// group is dropped first. #[derive(Debug)] @@ -285,25 +296,19 @@ impl Member { /// The claim exposes no socket. Pass every claim to [`Group::complete`], /// which attaches the filter before releasing serving handles. pub fn bind(self) -> io::Result { - let mut progress = self.state.progress(); - if progress.bound != self.shard.index() { + let mut bound = self.state.bound(); + if *bound != self.shard.index() { return Err(io::Error::other(format!( "reuseport member {} cannot bind while {} of {} are in: the kernel numbers a group by bind order", self.shard.index(), - progress.bound, + *bound, self.shard.count(), ))); } - let socket = bind(progress.addr, self.shard)?; - - // An ephemeral request gives each member a port of its own, so the rest - // of the group joins the port the first member actually got. - if self.shard.index() == 0 { - progress.addr = socket.local_addr()?; - } - progress.bound += 1; - drop(progress); + let socket = bind(self.state.addr, self.shard)?; + *bound += 1; + drop(bound); Ok(Claim { shard: self.shard, @@ -347,7 +352,7 @@ impl Bound { /// The address every member holds. pub fn addr(&self) -> SocketAddr { - self.state.progress().addr + self.state.addr } /// The next socket to serve, or `None` once every member was handed out. @@ -383,8 +388,8 @@ impl Socket { } } -/// What the members of a forming group share: the port it holds, how far the -/// group has been bound, and the address it holds. +/// What the members of a forming group share: the address and port lock it +/// holds, and how far the group has been bound. /// /// Shared rather than owned by the [`Group`] because a member is bound wherever /// its socket is served, which is usually not where the group lives. The lock @@ -393,36 +398,31 @@ impl Socket { /// is gone. #[derive(Debug)] struct State { - progress: Mutex, - - /// Held until the group and its members are all dropped, and released by the - /// kernel with the process. `None` for an ephemeral port, which cannot be - /// named in advance, and on a host with no lock directory. - _lock: Option, -} + addr: SocketAddr, -/// How far a group has been bound, and the address its members hold. -#[derive(Debug)] -struct Progress { /// How many members have bound, which is also the only index allowed to bind /// next. - bound: u16, - addr: SocketAddr, + bound: Mutex, + + /// Held until the group and its members are all dropped, and released by the + /// kernel with the process. `None` on a host with no lock directory. + _lock: Option, } impl State { fn new(addr: SocketAddr, lock: Option) -> Self { Self { - progress: Mutex::new(Progress { bound: 0, addr }), + addr, + bound: Mutex::new(0), _lock: lock, } } - /// The progress, whatever a panicking member left behind: a failed bind is - /// reported by the count it did not advance, so there is no torn state a + /// The bound count, whatever a panicking member left behind: a failed bind + /// is reported by the count it did not advance, so there is no torn state a /// poisoned lock would be protecting. - fn progress(&self) -> MutexGuard<'_, Progress> { - self.progress.lock().unwrap_or_else(PoisonError::into_inner) + fn bound(&self) -> MutexGuard<'_, u16> { + self.bound.lock().unwrap_or_else(PoisonError::into_inner) } } @@ -444,7 +444,9 @@ fn bind(addr: SocketAddr, shard: Shard) -> io::Result { // The probe only sees a group that is already bound. Two processes // *constructing* concurrently could each probe while the other holds // nothing yet, which is what [`Lock`] excludes; the probe's job is the - // holder the lock cannot see, one that predates it or never took it. + // holder the lock cannot see, one that predates it or never took it, or + // one that took an ephemeral group's port after [`Group::acquire`] + // resolved it. if shard.index() == 0 { drop(crate::bind::udp(crate::bind::Udp::new(addr))?); } @@ -1029,6 +1031,35 @@ mod tests { Group::acquire(addr, 1).expect("the released port must be takeable again"); } + /// An ephemeral request is resolved to a concrete port and locked before any + /// member binds. A reuseport bind of port `0` could land on a port another + /// same-UID group holds, and join it. + #[test] + #[cfg(target_os = "linux")] + fn an_ephemeral_group_takes_its_port_up_front() { + let group = Group::acquire("127.0.0.1:0".parse().unwrap(), 1).unwrap(); + let addr = group.addr(); + assert_ne!(addr.port(), 0, "the port is resolved before any member binds"); + + assert!( + matches!(Group::acquire(addr, 1), Err(Error::Overlap { .. })), + "a second group took an ephemeral group's port" + ); + } + + /// A same-UID reuseport socket that takes an ephemeral group's port between + /// its resolution and the first member's bind must fail that bind, not be + /// joined by it. + #[test] + #[cfg(target_os = "linux")] + fn an_ephemeral_port_taken_before_binding_is_refused() { + let mut group = Group::acquire("127.0.0.1:0".parse().unwrap(), 1).unwrap(); + let _intruder = crate::bind::udp(crate::bind::Udp::new(group.addr()).with_reuse_port(true)).unwrap(); + + let err = group.member().unwrap().bind().expect_err("joined the intruder's group"); + assert_eq!(err.kind(), io::ErrorKind::AddrInUse); + } + /// A member outlives the group that handed it out and can still bind, so the /// exclusion has to outlive the group too. Releasing the port at the group's /// drop would let a second group probe and interleave into this one while its diff --git a/rs/moq-tokio/src/worker/group.rs b/rs/moq-tokio/src/worker/group.rs index 88f94e53ba..85b6ca6bd3 100644 --- a/rs/moq-tokio/src/worker/group.rs +++ b/rs/moq-tokio/src/worker/group.rs @@ -90,6 +90,7 @@ impl Workers { // hands out one member per slot in the order the kernel numbers them by. let mut forming = moq_sock::shard::Group::acquire(requested, config.count).map_err(|err| match err { moq_sock::shard::Error::Count { count, max } => Error::WorkerCount { count, max }, + moq_sock::shard::Error::Resolve(err) => crate::noq::Error::BindSocket(err).into(), // The port lock is the only other way to lose the address, and it is // held by exactly one thing: another group of this UID. _ => Error::WorkerOverlap { addr: requested }, @@ -124,8 +125,7 @@ impl Workers { workers.push(worker); } - // Whatever the first member bound, which is the requested address unless - // it asked for an ephemeral port. + // The requested address, with an ephemeral port resolved. let addr = group.addr(); tracing::info!(workers = count, pinned = !cores.is_empty(), %addr, "bound QUIC workers"); diff --git a/rs/moq-tokio/tests/worker.rs b/rs/moq-tokio/tests/worker.rs index 00e7b7c8a6..c7fce97fd9 100644 --- a/rs/moq-tokio/tests/worker.rs +++ b/rs/moq-tokio/tests/worker.rs @@ -13,8 +13,8 @@ const WORKERS: u16 = 4; /// A UDP port nothing is bound to. /// -/// Only for the port-lock tests: an ephemeral group takes no lock, so the first -/// group has to name its port. Everything else binds `:0` and reads it back. +/// Only for the port-lock tests, where the first group names its port. +/// Everything else binds `:0` and reads it back. fn free_udp_port() -> u16 { let probe = UdpSocket::bind("127.0.0.1:0").expect("bind probe"); let port = probe.local_addr().expect("local addr").port(); @@ -298,8 +298,8 @@ async fn dropping_unserved_workers_releases_the_port() { assert!(owned.is_disjoint(&open_sockets()), "workers left the port bound"); } -/// The group holds one port, so an ephemeral bind is the port its first member -/// drew and the rest join it. A member picking a port of its own would sit +/// The group holds one port, so an ephemeral request resolves to one port that +/// every member joins. A member picking a port of its own would sit /// unreachable behind an address that reads as bound. #[tokio::test] async fn an_ephemeral_port_is_shared_by_the_group() { From bf59c93083e6c95df77fdd3c6cf337a501d77efa Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Mon, 28 Sep 2026 19:58:47 -0700 Subject: [PATCH 21/37] fix(video): re-anchor native capture above its last timestamp (#4418) Co-authored-by: Claude Opus 5.5 --- quest/m1/README.md | 1 - quest/m1/capture-reanchor.md | 21 ----- rs/moq-video/src/capture/channel.rs | 119 ++++++++++++++++++++++------ 3 files changed, 96 insertions(+), 45 deletions(-) delete mode 100644 quest/m1/capture-reanchor.md diff --git a/quest/m1/README.md b/quest/m1/README.md index f5ecede3ec..018b165a01 100644 --- a/quest/m1/README.md +++ b/quest/m1/README.md @@ -34,7 +34,6 @@ transport, benchmark tooling); worktrees isolate commits, not semantics. - [IETF hidden default](/quest/m1/ietf-hidden-default.md) - a moq-transport peer without the MoQ Hidden option is advertised hidden namespaces; one with it filters per subscription - [Watch refusal](/quest/m1/watch-refusal.md) - `` shows an origin refusal as an error instead of sitting offline - [moqsink recoverable errors](/quest/m1/moqsink-keyframe-latch.md) - a leading delta or a timestamp rewind drops frames until a keyframe instead of invalidating a moqsink pad -- [Capture re-anchor](/quest/m1/capture-reanchor.md) - a repeating or restarting device clock never rewinds native capture during a fast backlog drain - [Splice edge cases](/quest/m1/splice-edges.md) - an unstamped successor, a pruned segment's boundary group, and a warm head during a takeover are each handled correctly - [Resumed groups](/quest/m1/resume-latest.md) - a half-delivered group ends once the new copy is past it, so a group-only reader never parks after a mid-group failover - [Track tail hardening](/quest/m1/track-tail-hardening.md) - Rust and JS wait out a track's tail by the same rules, with the known hang, count, truncation, grace, and memory holes closed diff --git a/quest/m1/capture-reanchor.md b/quest/m1/capture-reanchor.md deleted file mode 100644 index df3ebb1fb5..0000000000 --- a/quest/m1/capture-reanchor.md +++ /dev/null @@ -1,21 +0,0 @@ -# [XS] Native capture re-anchors above its last timestamp - -## Goal - -A device clock that repeats or restarts never rewinds native video capture, -even while the pump drains a backlog faster than real time. `FrameChannel::push_native` -in `rs/moq-video/src/capture/channel.rs` re-anchors such a frame to its -arrival time ([#4125](https://github.com/moq-dev/moq/pull/4125)). During a -fast drain the previous frame's mapped timestamp can sit ahead of arrival, so -the re-anchor lands below it: source 0 and 40 ms arriving 1 ms apart publish -near 0 and 40 ms, and an immediate reset to zero publishes near 2 ms. Past a -closed group that is `TimestampRewind`, which stops capture. - -## Plan - -Remember the last mapped timestamp and floor the re-anchor strictly above it, -including the fallback when the checked arithmetic fails. The mapping keeps -advancing with the device clock from there. - -Unit regression in `channel.rs`'s mapping tests: two frames drained 1 ms apart -with a 40 ms source step, then a source reset, maps above the second frame. diff --git a/rs/moq-video/src/capture/channel.rs b/rs/moq-video/src/capture/channel.rs index 9e32692bb4..ded95b29f6 100644 --- a/rs/moq-video/src/capture/channel.rs +++ b/rs/moq-video/src/capture/channel.rs @@ -41,6 +41,8 @@ struct Native { local: Timestamp, /// The previous device timestamp, which the next must exceed to keep the anchor. last: Timestamp, + /// The previous mapped timestamp, which a re-anchor must exceed. + mapped: Timestamp, } impl FrameChannel { @@ -65,10 +67,7 @@ impl FrameChannel { } pub(super) fn push_at(&self, surface: Surface, captured: Instant) { - let micros = captured.saturating_duration_since(self.epoch).as_micros(); - let micros = u64::try_from(micros).unwrap_or(u64::MAX); - let frame = Frame::new(surface, Timestamp::from_micros(micros).expect("capture timestamp fits")); - self.publish(frame); + self.publish(Frame::new(surface, self.at(captured))); } /// Map a device-local timestamp into this stream's private timeline. The @@ -77,28 +76,52 @@ impl FrameChannel { /// `pump` plus `cfg(test)` for the mapping test below. /// /// A device timeline that steps back or stalls (a driver restarting its clock - /// at zero, or one reporting a constant) re-anchors that sample to arrival, so - /// the stream never rewinds or repeats a timestamp it already delivered. + /// at zero, or one reporting a constant) re-anchors that sample to arrival, or + /// just past the last delivered timestamp if that is later, so the stream + /// never rewinds or repeats a timestamp it already delivered. #[cfg(any(target_os = "linux", target_os = "windows", test))] pub(super) fn push_native(&self, surface: Surface, source: Timestamp) { - let local = self.now(); + self.push_native_at(surface, source, Instant::now()); + } + + #[cfg(any(target_os = "linux", target_os = "windows", test))] + fn push_native_at(&self, surface: Surface, source: Timestamp, arrived: Instant) { + let arrived = self.at(arrived); let mut state = self.state.lock().unwrap(); if state.closed { return; } + // A backlog drained faster than real time maps ahead of arrival, so a + // re-anchor floors strictly above the last delivered timestamp. A device + // clock that jumped to the end of the timeline leaves no room above it. + let floor = match state.native { + None => arrived, + Some(native) => match native.mapped.checked_add(Timestamp::from_micros(1).unwrap()) { + Ok(next) => arrived.max(next), + Err(err) => { + drop(state); + return self.fail(err.into()); + } + }, + }; let anchor = match state.native { Some(native) if source > native.last => native, _ => Native { source, - local, + local: floor, last: source, + mapped: floor, }, }; let timestamp = source .checked_sub(anchor.source) .and_then(|elapsed| anchor.local.checked_add(elapsed)) - .unwrap_or(local); - state.native = Some(Native { last: source, ..anchor }); + .unwrap_or(floor); + state.native = Some(Native { + last: source, + mapped: timestamp, + ..anchor + }); state.frame = Some(Frame::new(surface, timestamp)); drop(state); self.notify.notify_one(); @@ -169,7 +192,13 @@ impl FrameChannel { } pub(super) fn now(&self) -> Timestamp { - let micros = u64::try_from(self.epoch.elapsed().as_micros()).unwrap_or(u64::MAX); + self.at(Instant::now()) + } + + /// An instant on this stream's timeline, clamped to its epoch. + fn at(&self, instant: Instant) -> Timestamp { + let micros = instant.saturating_duration_since(self.epoch).as_micros(); + let micros = u64::try_from(micros).unwrap_or(u64::MAX); Timestamp::from_micros(micros).expect("capture timestamp fits") } } @@ -286,35 +315,79 @@ mod tests { async fn native_timestamps_reanchor_when_the_device_clock_restarts() { let chan = FrameChannel::new(); let us = |micros| Timestamp::from_micros(micros).unwrap(); + let at = |millis| chan.epoch + std::time::Duration::from_millis(millis); - chan.push_native(frame(1), us(0)); - chan.recv().await.unwrap().unwrap(); - // Real time passes with the device clock, so the mapping stays at or behind arrival. - tokio::time::sleep(std::time::Duration::from_millis(20)).await; - chan.push_native(frame(2), us(20_000)); - let before = chan.recv().await.unwrap().unwrap().timestamp; + chan.push_native_at(frame(1), us(0), at(0)); + // Real time passes with the device clock, so the mapping stays at arrival. + chan.push_native_at(frame(2), us(20_000), at(20)); + assert_eq!(chan.recv().await.unwrap().unwrap().timestamp.as_micros(), 20_000); - chan.push_native(frame(3), us(0)); + chan.push_native_at(frame(3), us(0), at(25)); let restarted = chan.recv().await.unwrap().unwrap().timestamp; - assert!(restarted >= before, "{restarted:?} rewound behind {before:?}"); + assert_eq!(restarted.as_micros(), 25_000, "re-anchored to arrival"); - chan.push_native(frame(4), us(33_000)); + chan.push_native_at(frame(4), us(33_000), at(58)); let next = chan.recv().await.unwrap().unwrap().timestamp; assert_eq!(next.as_micros() - restarted.as_micros(), 33_000); } + /// A backlog drained faster than real time maps ahead of arrival, so a device + /// clock restart during the drain must re-anchor above the last delivered + /// timestamp rather than at arrival. + #[tokio::test] + async fn native_timestamps_reanchor_above_a_fast_drain() { + let chan = FrameChannel::new(); + let us = |micros| Timestamp::from_micros(micros).unwrap(); + let at = |millis| chan.epoch + std::time::Duration::from_millis(millis); + + chan.push_native_at(frame(1), us(0), at(0)); + chan.push_native_at(frame(2), us(40_000), at(1)); + let drained = chan.recv().await.unwrap().unwrap().timestamp; + assert_eq!(drained.as_micros(), 40_000); + + chan.push_native_at(frame(3), us(0), at(2)); + let restarted = chan.recv().await.unwrap().unwrap().timestamp; + assert!(restarted > drained, "{restarted:?} rewound behind {drained:?}"); + + chan.push_native_at(frame(4), us(33_000), at(3)); + let next = chan.recv().await.unwrap().unwrap().timestamp; + assert_eq!( + next.as_micros() - restarted.as_micros(), + 33_000, + "the device's spacing resumes" + ); + } + + /// A device clock that jumps to the last representable timestamp leaves no room + /// for a later re-anchor, which must end the stream with an error rather than + /// panic the pump thread and leave the consumer parked. + #[tokio::test] + async fn a_device_clock_at_the_end_of_the_timeline_fails_the_stream() { + let chan = FrameChannel::new(); + let us = |micros| Timestamp::from_micros(micros).unwrap(); + let at = |millis| chan.epoch + std::time::Duration::from_millis(millis); + + chan.push_native_at(frame(1), us(0), at(0)); + chan.push_native_at(frame(2), us((1 << 62) - 1), at(1)); + chan.push_native_at(frame(3), us(0), at(2)); + + assert!(matches!(chan.recv().await, Err(Error::TimeOverflow(_)))); + assert!(chan.recv().await.unwrap().is_none()); + } + /// A driver that reports one constant timestamp must not stamp every frame /// identically: each sample falls back to its arrival. #[tokio::test] async fn a_stalled_device_clock_falls_back_to_arrival() { let chan = FrameChannel::new(); let constant = Timestamp::from_micros(0).unwrap(); + let at = |millis| chan.epoch + std::time::Duration::from_millis(millis); - chan.push_native(frame(1), constant); + chan.push_native_at(frame(1), constant, at(0)); let first = chan.recv().await.unwrap().unwrap().timestamp; - tokio::time::sleep(std::time::Duration::from_millis(5)).await; - chan.push_native(frame(2), constant); + chan.push_native_at(frame(2), constant, at(5)); let second = chan.recv().await.unwrap().unwrap().timestamp; assert!(second > first, "a stalled device clock repeated {first:?}"); + assert_eq!(second.as_micros(), 5_000); } } From 539850d8a1b6efcd650df88ee3b78e97bd9e608b Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Mon, 28 Sep 2026 20:01:06 -0700 Subject: [PATCH 22/37] docs(quest): record the routing simulator's findings in cluster routing (#4421) Co-authored-by: Claude Opus 5.5 --- quest/m1/cluster-routing.md | 93 +++++++++++++++++++++++++++++++++++-- 1 file changed, 89 insertions(+), 4 deletions(-) diff --git a/quest/m1/cluster-routing.md b/quest/m1/cluster-routing.md index 7345104e27..35dd9c22f1 100644 --- a/quest/m1/cluster-routing.md +++ b/quest/m1/cluster-routing.md @@ -27,6 +27,16 @@ vector: the same fan-out, the same global knowledge, and no loop freedom when several sources claim a prefix (section 2.7), which pools and wildcards make MoQ's common case. +moq.pro's routing simulator (`just rs sim`, +[moq.pro#2020](https://github.com/moq-dev/moq.pro/pull/2020)) measured worse +than that on live's relay graph (34 relays, mean degree 8.4): a publish costs +about 750 announces, and ending a path's only publisher explores stale +alternatives for about a second and 39k to 80k announces, with subscribes +looping meanwhile. Coalescing each writer for 10 ms cuts that by 7x and leaves +the loops. Hiding the routes through a peer that withdrew the path +([#4399](https://github.com/moq-dev/moq/pull/4399)) reduces that path hunting +but does not end it; see the findings below. + ### Decisions - Existence is split from reachability. An announcement carries the path, its @@ -37,19 +47,39 @@ make MoQ's common case. a stale tree or a failed-over registry cannot revive it. Babel keeps feasibility past withdrawal for the same reason ([RFC 8966 section 3.7.3](https://www.rfc-editor.org/rfc/rfc8966#section-3.7.3)). +- A new incarnation is learned from existence, never inferred from liveness: + it announces a reset (its incarnation at seqno 0) that ends everything the + old one published, and a relay applies a reset and the records after it as + one batch. Incarnations are ordered, so a delayed reset from an older one is + dropped. Purging on a liveness report instead drops a restarted origin's + broadcasts until their re-announce lands, which the simulator showed as live + broadcasts reported offline. - The topology is configured: `--cluster-connect` or the connect API gives the relay graph and link costs. Relays flood per-link liveness among themselves with a per-link seqno. The seqno is scoped to the relay's incarnation, so a restarted relay's links supersede its stale ones instead of looking older. Gossip discovery (`cluster.mesh`) stays for zero-config self-hosting and derives the topology from what it discovers; it need not scale. + - A relay batches the liveness reports it sends, its own and those it + forwards, for a short hold-down (50 ms in the simulator), and recomputes + its trees after a matching delay, as OSPF's SPF delay does. Unbatched, one + relay restart at 340 relays sent half a million messages; batched, 27k. + - On session up, relays exchange a digest (each reporter's incarnation and + its seqno per link) and send only what the other lacks. A reporter's + reports flood separately, so its newest seqno alone would hide a missing + older one. The digest already counts the fresh report of the link that + just came up; sending the database first replays the relay's own report + from when the link went down, and the peer drops the link it is using. - A relay picks the origin with the lowest shortest-path distance plus origin cost, ties broken by rendezvous hashing (HRW) of the requested path and the origin id, and forwards along its shortest path. Distance compares cost, then hop count, so every hop strictly shortens it even across `?cost=0` links. That is a shortest path to a virtual node linked to every origin, so it is loop-free whenever relays agree on the topology. The longest covering - prefix still ranks first, per [Wildcard](/quest/m0/wildcard/README.md). + prefix still ranks first, per [Wildcard](/quest/m0/wildcard/README.md). The + simulator saw no loop while views agreed, and HRW split an equal-cost pool + 63/49 where today's hash of the announced prefix sends all of it to one + sibling. - The first relay's choice rides the SUBSCRIBE, and transit relays forward toward that origin by topology alone, never re-selecting. Re-selection against another existence view loops: a relay that lost a specific claim @@ -60,7 +90,8 @@ make MoQ's common case. - SUBSCRIBE and FETCH carry a visited-relay list end to end. It catches loops while liveness views disagree and names the path for stats. Narrowing it to cluster hops is later work. The serving origin's identity rides the reply, - per Wildcard's Spread quest. + per Wildcard's Spread quest. On live's graph no disagreement looped across + 20 seeds of link, cost, and relay churn; the list is a safety net. - Announcements are on demand. A relay forwards only the union of its clients' ANNOUNCE_REQUEST prefixes, never the empty prefix. A wide prefix that many edges' viewers request is that customer's cost. `.stats` becomes ordinary @@ -76,19 +107,65 @@ make MoQ's common case. - A relay fails over to the next-nearest registry and reconciles its view instead of treating the lost session as ends, so a registry failure never reports a live broadcast offline. + - The reconcile is the relay's full live set at its current seqno, which + ends whatever it leaves out. The registry's snapshot carries each + relevant origin's last reconcile seqno, and the relay names the origins it + already holds, so a view kept through a freeze ends what ended meanwhile. - With no registry reachable, a relay freezes: it keeps its view, learns nothing new, and alerts. Falling back to flooding would cascade the failure. - Without registries (self-hosting), existence floods along the shortest-path tree, one copy per relay. A relay forwards an event only when it changes its view, so a duplicate copy, from trees built on disagreeing liveness, stops - there. + there. A relay that gains a child in its tree (a view change or a new + session) pushes that origin's reset and records to it; without the push a + relay misses events while views disagree. - Between clusters, announcements stay path vector with cluster ids as the hops, like BGP between autonomous systems. A customer's on-prem cluster is one hop, and an announcement naming the receiving cluster is dropped. - The cluster switches versions as a whole; older lite and IETF sessions stay at its edges. +### Simulator findings + +The report is moq.pro's `just rs sim` (every scenario on live's graph) and +`just rs sim sweep` (synthetic regional graphs of 34, 340, and 1020 relays). +It carries messages and bytes by kind, per relay and cross-region, +convergence, loop, stall, and failover windows, and state per relay and per +registry. What decides the wire: + +- Existence costs about one message per relay per event flooded, and one per + interested relay plus one per remote registry with registries. On live, a + wide customer prefix with `.stats` from every relay cost 2.5M announces + today, 4k flooded, and 3.5k with registries, whose relays held 63 records + on average against 94. +- Liveness is the split's dominant cost at scale: flooding every link repeats + each report once per neighbour. At 1020 relays (mean degree 35), a link + loss and restore plus a relay loss and restart cost about 290 MiB of + liveness flooding and 54 MiB of session-up digests against under 1 MiB of + existence for twenty publishes, while a publish costs exactly one message + per relay. A reduced flooding topology + ([RFC 9667](https://www.rfc-editor.org/rfc/rfc9667)) is the known fix for + the flooding. +- Failure detection, not routing, sets every outage window: a silent link or + relay loss is noticed after the 30 s QUIC idle timeout in every candidate, + and subscribes through it go nowhere until then. Cluster sessions need a + short idle timeout; a keepalive only keeps a quiet session open. +- One registry per region and two cost about the same; two halves the + busiest registry's load. A registration sent on a dead registry session + that nobody has noticed yet waits for the failover. +- [#4399](https://github.com/moq-dev/moq/pull/4399) trades path hunting for + flapping. In its own full mesh of 8 it retracts each of 100 withdrawn + broadcasts exactly once per relay, as its tests show. On live's graph, with + each writer coalescing for 1 ms, ending three paths loops subscribes for + 9 s instead of 41 s and edges report the ended paths for 38 s summed + instead of 93, but it costs about the same 60k messages, and clients see + 17k updates instead of 12k, with ended paths re-announced 4185 times + instead of 163: each re-announce by a peer revives every route through it, + stale ones included. Written without coalescing, the revivals cascade, and + the same scenario costs 18.6M messages instead of 189k. Per-origin seqnos, + above, end both. + ### Open questions - Sharding registries by HRW over a prefix key once one registry cannot hold @@ -96,6 +173,12 @@ make MoQ's common case. - A mixed-version bridge, if a fleet cannot switch at once. - How long a relay keeps an ended path's seqno. A new origin incarnation clears it; within one, it must outlive every delayed copy of the start. +- How a push to a new tree child ends what the child missed within the same + incarnation. Its reset only marks a new incarnation, so the push may need a + watermark like the registry reconcile's: it ends only what the child holds + at or below it, and a delayed push cannot erase newer records. +- How long a relay keeps the records of an origin that never comes back. They + stop being reported once it is unreachable, but nothing removes them. - How an edge routes a SUBSCRIBE for a path none of its clients asked to announce. It holds no route for it, and asking a registry first adds a round trip before the first byte. @@ -110,12 +193,14 @@ make MoQ's common case. `doc/concept/use-case/contribution.md`). Inside a cluster no announcement carries a hop list, so two encoders on different ingest relays become two origins. Keep the documented behavior or change the docs in the same PR. +- Whether equal-cost next hops should spread by a hash of the path. A fixed + tie-break sends every path through the same neighbour and its failure takes + them all. ## Required - moq.pro workers stop electing on hop chains, reading the relay's local origin instead ([moq.pro voice-local-origin](https://github.com/moq-dev/moq.pro/blob/main/quest/m0/voice-local-origin.md)) - [Wildcard](/quest/m0/wildcard/README.md) - the longest-prefix rule, pool spread, and reply identity this selection builds on -- moq.pro's routing simulator reports ([quest](https://github.com/moq-dev/moq.pro/blob/main/quest/m1/routing-simulator.md)) ## Related From 2f4b78581a512e623d3cef58874991abd9c09e0c Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Mon, 28 Sep 2026 20:18:12 -0700 Subject: [PATCH 23/37] fix(uring): publish a local close only once its CONNECTION_CLOSE is staged (#4431) Co-authored-by: Claude Opus 5.5 --- rs/moq-uring/src/quic/noq/connection.rs | 45 +++++++++++----------- rs/moq-uring/tests/workers.rs | 51 +++++++++++-------------- 2 files changed, 44 insertions(+), 52 deletions(-) diff --git a/rs/moq-uring/src/quic/noq/connection.rs b/rs/moq-uring/src/quic/noq/connection.rs index 88828d989a..c7e7ea5280 100644 --- a/rs/moq-uring/src/quic/noq/connection.rs +++ b/rs/moq-uring/src/quic/noq/connection.rs @@ -367,7 +367,7 @@ pub(crate) fn launch( key, deadline: owner.timer(), scratch: Vec::with_capacity(TRAIN_SEGMENTS * SEGMENT), - blocked: false, + close_staged: false, }; let future = async move { kio::wait(|waiter| driver.poll(waiter)).await }; (shared, future) @@ -676,9 +676,10 @@ struct Driver { /// Egress staging: noq-proto writes into a `Vec`, so a train is built /// here and copied into the socket's registered buffer. scratch: Vec, - /// The last flush found the transmit pool drained, so nothing it owed the - /// peer has reached the wire yet. - blocked: bool, + /// A CONNECTION_CLOSE has been handed to the socket. Not implied by a + /// flush that came back empty: noq paces the close like any other packet, + /// so it can sit behind a pacing timer after the application asked for it. + close_staged: bool, } impl Driver { @@ -706,6 +707,9 @@ impl Driver { // (and a retransmit for each packet that arrives after), so the // driver runs until noq says the drain is over. if self.shared.conn.borrow().is_drained() { + // A close noq never managed to send (the server's + // anti-amplification limit can withhold it) ends here too. + self.publish_close(); return Poll::Ready(()); } @@ -713,7 +717,9 @@ impl Driver { self.shared.state.borrow_mut().fail(err); return Poll::Ready(()); } - self.publish_close(); + if self.close_staged { + self.publish_close(); + } // Arm, *then* poll: the poll is what registers the waiter, so // polling before the set would leave the firing to wake nobody @@ -777,15 +783,12 @@ impl Driver { /// Publish the terminal error for a close this side asked for. /// /// noq raises no event for it, so the driver is what reports it, and - /// only once the flush above has staged the CONNECTION_CLOSE, since an - /// application is free to stop driving the worker the moment + /// only once the CONNECTION_CLOSE is staged (or the drain is over), since + /// an application is free to stop driving the worker the moment /// `poll_closed` resolves. Staged is not delivered: the send is /// fire-and-forget, so a worker torn down in the same breath can still /// take the packet with it and leave the peer to idle out. fn publish_close(&mut self) { - if self.blocked || !self.shared.conn.borrow().is_closed() { - return; - } let mut state = self.shared.state.borrow_mut(); let Some((code, reason)) = state.local_close.take() else { return; @@ -818,12 +821,8 @@ impl Driver { Poll::Ready(Ok(tx)) => tx, Poll::Ready(Err(err)) => return Poll::Ready(Err(Error::Io(err.to_string()))), // Backpressure: a completed send re-polls us. - Poll::Pending => { - self.blocked = true; - return Poll::Pending; - } + Poll::Pending => return Poll::Pending, }; - self.blocked = false; let segments = (tx.len() / SEGMENT).min(TRAIN_SEGMENTS); if segments == 0 { @@ -835,15 +834,14 @@ impl Driver { let segments = std::num::NonZeroUsize::new(segments).expect("segments was checked above"); self.scratch.clear(); - let transmit = match self - .shared - .conn - .borrow_mut() - .poll_transmit(Instant::now(), segments, &mut self.scratch) - { - Some(transmit) => transmit, + let (transmit, closing) = { + let mut conn = self.shared.conn.borrow_mut(); // Nothing to send, or paced; the buffer returns to the pool on drop. - None => return Poll::Pending, + let Some(transmit) = conn.poll_transmit(Instant::now(), segments, &mut self.scratch) else { + return Poll::Pending; + }; + // Once closed, noq transmits nothing but the CONNECTION_CLOSE. + (transmit, conn.is_closed()) }; tx[..transmit.size].copy_from_slice(&self.scratch[..transmit.size]); @@ -858,6 +856,7 @@ impl Driver { if let Err(err) = tx.send(transmit) { return Poll::Ready(Err(Error::Io(err.to_string()))); } + self.close_staged |= closing; // A flush frees datagram-send queue space. self.shared.state.borrow_mut().datagram_send_waiters.wake(); Poll::Ready(Ok(())) diff --git a/rs/moq-uring/tests/workers.rs b/rs/moq-uring/tests/workers.rs index f3329d9dd2..fb2996c445 100644 --- a/rs/moq-uring/tests/workers.rs +++ b/rs/moq-uring/tests/workers.rs @@ -5,7 +5,7 @@ //! being told which. Whichever worker the Initial hashes to owns the //! connection, and the prefix keeps every later packet (handshake //! continuation included) on that worker; a wrong prefix stalls the -//! handshake, so every dial completing is what proves the steering. +//! handshake, so every dial being accepted is what proves the steering. //! //! Kernel-gated: skips loudly below the Linux 6.12 floor (GitHub-hosted CI), //! and runs everywhere else. @@ -123,10 +123,11 @@ fn a_steered_group_serves_a_shared_port() { .expect("endpoint"); handle.spawn(async move { - // Accepted connections are dropped once counted; the - // client is what closes them. - while endpoint.accept().await.is_ok() { + // Each accepted connection is counted, then closed: that + // close is how the client learns this side accepted it. + while let Ok(mut conn) = endpoint.accept().await { accepted[usize::from(shard.index())].fetch_add(1, Ordering::AcqRel); + web_transport_trait::poll::Session::close(&mut conn, 0, "done"); } }); ready.send(()).expect("test alive"); @@ -139,8 +140,8 @@ fn a_steered_group_serves_a_shared_port() { started.recv().expect("a worker thread failed to start"); } - // Dial the shared port repeatedly from one client worker. Every handshake - // completing is the steering assertion (see the module docs). + // Dial the shared port repeatedly from one client worker. Every dial being + // accepted is the steering assertion (see the module docs). let mut client_worker = client_worker; let handle = client_worker.handle(); let mut dial = quic::client::Config::new(addr, "localhost"); @@ -159,38 +160,30 @@ fn a_steered_group_serves_a_shared_port() { Some(ALPN), "negotiated ALPN" ); - web_transport_trait::poll::Session::close(&mut conn, 0, "done"); - } - - // The server side counts a connection when its accept loop takes - // it, which can trail the client's handshake; wait for the tally. - let deadline = std::time::Instant::now() + std::time::Duration::from_secs(5); - loop { - let total: usize = accepted.iter().map(|count| count.load(Ordering::Acquire)).sum(); - if total == DIALS { - break; + // The client's handshake completes a flight before the + // server's, so closing here could discard the client's Finished + // (still behind the pacer, say) and the server would never + // accept. The server's close is the proof it did. + match std::future::poll_fn(|cx| web_transport_trait::poll::Session::poll_closed(&mut conn, cx)).await { + quic::Error::App { code: 0, .. } => {} + other => panic!("the client saw {other:?} instead of the server's close"), } - assert!( - std::time::Instant::now() < deadline, - "only {total} of {DIALS} dials were accepted" - ); - moq_uring::Timer::after(&handle, std::time::Duration::from_millis(10)) - .wait() - .await; } }) .expect("client worker"); - // Every member has to have been fed, or the group is steering into a - // subset and the rest sit idle. - for (index, count) in accepted.iter().enumerate() { - assert!(count.load(Ordering::Acquire) > 0, "worker {index} accepted nothing"); - } - for stop in &stops { stop.stop(); } for thread in threads { thread.join().expect("worker thread"); } + + // Every member has to have been fed, or the group is steering into a + // subset and the rest sit idle. + let total: usize = accepted.iter().map(|count| count.load(Ordering::Acquire)).sum(); + assert_eq!(total, DIALS, "every dial is accepted exactly once"); + for (index, count) in accepted.iter().enumerate() { + assert!(count.load(Ordering::Acquire) > 0, "worker {index} accepted nothing"); + } } From ed56cab58fccca92af608737eef533134c9d1717 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Mon, 28 Sep 2026 21:38:41 -0700 Subject: [PATCH 24/37] chore(quest): plan the PR-merge follow-ups (#4435) Co-authored-by: Claude Opus 5.5 --- quest/m0/README.md | 1 + quest/m0/audio-quality-harness/README.md | 6 +++++ quest/m0/quest-check-everywhere.md | 27 +++++++++++++++++++ quest/m1/README.md | 7 +++++ quest/m1/ci-hygiene.md | 25 +++++++++++++++++ quest/m1/coding-reader-cap.md | 21 +++++++++++++++ quest/m1/exact-scope.md | 24 +++++++++++++++++ quest/m1/front-deadline-index.md | 34 ++++++++++++++++++++++++ quest/m1/interop-audio-cold-start.md | 29 ++++++++++++++++++++ quest/m1/js-fetch-cancel.md | 25 +++++++++++++++++ quest/m1/js-track-close-end.md | 20 ++++++++++++++ quest/m1/relay-auth-client-ca.md | 4 +++ 12 files changed, 223 insertions(+) create mode 100644 quest/m0/quest-check-everywhere.md create mode 100644 quest/m1/ci-hygiene.md create mode 100644 quest/m1/coding-reader-cap.md create mode 100644 quest/m1/exact-scope.md create mode 100644 quest/m1/front-deadline-index.md create mode 100644 quest/m1/interop-audio-cold-start.md create mode 100644 quest/m1/js-fetch-cancel.md create mode 100644 quest/m1/js-track-close-end.md diff --git a/quest/m0/README.md b/quest/m0/README.md index bfc968f761..5327fdc0e5 100644 --- a/quest/m0/README.md +++ b/quest/m0/README.md @@ -31,6 +31,7 @@ Published API or wire breaks still land on dev; each quest's Plan says so. ## Required - [Packaged relay starts](/quest/m0/relay-systemd-unit.md) - the `.deb` and `.rpm` relay service starts instead of crash-looping on `--file` +- [quest check everywhere](/quest/m0/quest-check-everywhere.md) - `quest check` guards `main`, `dev`, and the line branches on push and PR, not only PRs into `main` - [Wildcard](/quest/m0/wildcard/README.md) - a relay resolves subscriptions against advertised prefixes, a service claims the prefix it could serve and refuses the rest instead of enumerating broadcasts, and the browser player treats a covering claim as availability - [Audio quality harness](/quest/m0/audio-quality-harness/README.md) - a browser playout latency regression fails a nightly run instead of arriving as a bug report, and its recorder supplies the jitter target's replay traces - [Audio jitter target](/quest/m0/audio-jitter-target/README.md) - the audio playout target is a measured estimate of arrival timing in both languages, not a round-trip guess diff --git a/quest/m0/audio-quality-harness/README.md b/quest/m0/audio-quality-harness/README.md index 457844c109..0262b1bc11 100644 --- a/quest/m0/audio-quality-harness/README.md +++ b/quest/m0/audio-quality-harness/README.md @@ -47,6 +47,12 @@ sample rate, which is more than a merge gate should carry, and `nightly.yml` already exists for exactly this trade. Budgets are keyed by the full row, since each of those dimensions moves the expected floor. +The budgets checked in with the browser lane (#4426) were recorded locally. +Once the line lands, re-record `test/audio-quality/budgets.json` from the +nightly runner's first runs, since nightly only runs `main`'s code. Tightening +the auto rows belongs to the [jitter target's watch +quest](/quest/m0/audio-jitter-target/watch.md). + ## Required - [Browser](/quest/m0/audio-quality-harness/browser.md) - upstream the fork's harness, grade it against a budget, run it nightly diff --git a/quest/m0/quest-check-everywhere.md b/quest/m0/quest-check-everywhere.md new file mode 100644 index 0000000000..c62ab16a6f --- /dev/null +++ b/quest/m0/quest-check-everywhere.md @@ -0,0 +1,27 @@ +# [XS] quest check runs on every branch that carries quests + +## Goal + +`quest check` fails any push or PR that breaks quest structure on `main`, +`dev`, and the questline branches, not only PRs into `main`. + +## Plan + +- `dev` has no `quest` flake input and no `quest check` in its justfile; + #4428's main-into-dev sync brings both. After it lands, convert the + old-format quests on `dev` until `quest check` passes there. +- Line branches pin their own `quest` revision (the wildcard line pinned + 46d7fe8 against main's 8590d2a), so an old pin passes an old format. + Merging `main` into each active line bumps the pin; do that for the lines + that fail today and fix what the new check reports. +- `just ci check` runs `quest check` on pull requests only. Also run it on + push to `main`, `dev`, and `quest/**`, so a direct merge commit (such as + `main` merged into a line) can't land a broken tree. Use a dedicated job + that runs `quest check` unconditionally: `check.yml`'s scope steps diff + against `origin/$GITHUB_BASE_REF`, which is empty on a push. + +Public API: none. Wire: none. + +## Required + +- #4428 merged to `dev` diff --git a/quest/m1/README.md b/quest/m1/README.md index 018b165a01..bd6c5064ec 100644 --- a/quest/m1/README.md +++ b/quest/m1/README.md @@ -19,6 +19,7 @@ transport, benchmark tooling); worktrees isolate commits, not semantics. - [Late joiner history](/quest/m1/relay-late-joiner-history.md) - a subscriber joining a relay's track from group 0 later still receives the cached finished group below the live one - [Cluster routing](/quest/m1/cluster-routing.md) - an announcement says where a broadcast originates, not how to reach it, and a relay hears only the prefixes its clients asked for +- [Exact scope](/quest/m1/exact-scope.md) - a reader never sees an exact broadcast outside its scope, in Rust or JS; prefix routes above it still present as the empty path - [Track tail interop](/quest/m1/track-tail-interop.md) - a Rust publisher ending a track with a group in flight is read to its end by the JS subscriber, and the reverse, in `just test interop` - [Worker socket count](/quest/m1/worker-socket-count.md) - the moq-tokio worker test counts only its own listener's sockets - [Binding audio delay](/quest/m1/binding-surface.md) - moq-ffi and every wrapper configure and observe audio playout delay @@ -37,6 +38,7 @@ transport, benchmark tooling); worktrees isolate commits, not semantics. - [Splice edge cases](/quest/m1/splice-edges.md) - an unstamped successor, a pruned segment's boundary group, and a warm head during a takeover are each handled correctly - [Resumed groups](/quest/m1/resume-latest.md) - a half-delivered group ends once the new copy is past it, so a group-only reader never parks after a mid-group failover - [Track tail hardening](/quest/m1/track-tail-hardening.md) - Rust and JS wait out a track's tail by the same rules, with the known hang, count, truncation, grace, and memory holes closed +- [JS close end](/quest/m1/js-track-close-end.md) - a JS track's clean close ends at its own last group, not a sibling producer's - [SUBSCRIBE_DROP](/quest/m1/subscribe-drop.md) - every stream group in a lite subscription arrives or is dropped by name, and lite-07 drops its stream count for it - [FIN wait expiry](/quest/m1/fin-wait-expiry.md) - a group awaiting its FIN ack still expires and follows priority updates on lite and IETF - [Cross-relay bursts](/quest/m1/cross-relay-bursts.md) - bursty small-group tracks cross two relays without lost groups, unanswered FETCHes, or stalls @@ -45,11 +47,13 @@ transport, benchmark tooling); worktrees isolate commits, not semantics. - [Watch decoder recovery](/quest/m1/watch-decoder-recovery.md) - one malformed packet rebuilds the audio or video decoder instead of ending playback - [Watch audio under CSP](/quest/m1/watch-worklet-file.md) - production builds ship the audio worklet as a file, so `script-src 'self'` pages play audio - [More tests under load](/quest/m1/test-flakes-2.md) - the second round of load-only failures, fixed at the cause +- [Interop audio cold start](/quest/m1/interop-audio-cold-start.md) - the interop audio tone check stops failing on cold start, fixed at its cause - [Auth outage clock](/quest/m1/auth-outage-clock.md) - the relay and moq-auth outage tests run on a paused clock again and assert both bounds of `expires` - [Legacy end overshoot](/quest/m1/legacy-end-overshoot.md) - browser playback survives a group that starts inside the previous group's estimated end - [Slow group log](/quest/m1/slow-group-log.md) - a starved viewer reports skipped groups once per catch-up, not once per group - [UnknownSession log flood](/quest/m1/unknown-session-logs.md) - streams reset before their WebTransport header stop being reported as UnknownSession at WARN - [Merge queue](/quest/m1/merge-queue.md) - the required checks run on `merge_group`, so a stale green check can no longer break main +- [CI hygiene](/quest/m1/ci-hygiene.md) - a push to one PR never cancels another's run, and a failed run never saves the Rust cache - [Wire compatibility](/quest/m1/wire-compat.md) - a nightly run tests this checkout against the last published release for tokens, session wire, and catalog/container - [#2075](/quest/m1/2075-mirror-catalog-reservation-gating-in-moq-hang-js-hang.md) - @moq/publish gates the first catalog snapshot until every reserved track is described - [Full codec string](/quest/m1/publish-codec-string.md) - browser-published video carries the encoder's full RFC 6381 codec string, so native players decode it @@ -75,6 +79,7 @@ transport, benchmark tooling); worktrees isolate commits, not semantics. - [JS IETF datagrams](/quest/m1/js-ietf-datagram.md) - `@moq/net` sends and receives datagram groups over moq-transport, like Rust - [#2991](/quest/m1/2991-net-coalesce-dynamic-tracks-and-preserve-sequences-across.md) - one dynamic producer per track name in both languages, with the sequence namespace surviving a replacement - [JavaScript FETCH](/quest/m1/js-fetch.md) - generic on-demand group serving and IETF FETCH for browser publishers +- [JS fetch cancel](/quest/m1/js-fetch-cancel.md) - `FetchGroupOptions.signal` abandons one pending fetch without closing the track - [Archive](/quest/m1/archive/README.md) - record selected tracks to any object_store and replay them over FETCH or derived HLS; the catalog entry and format may break in place, since no archives exist - [Tooling](/quest/m1/tooling/README.md) - justfiles become a one-line menu over `sh/`, one impact map scopes CI, and every workflow step runs a recipe - [Path patterns](/quest/m1/path-patterns.md) - one matcher for every predicate over broadcast paths: tokens, origins, interest @@ -130,10 +135,12 @@ transport, benchmark tooling); worktrees isolate commits, not semantics. - [Plan: cache age-out](/quest/m1/cache-wall-eviction.md) - a swept benchmark decides whether the track cache ages groups out on wall time without a write - [Frame slot charge](/quest/m1/frame-slot-charge.md) - a group's frame slots past the first four count against the cache pool, including capacity a released group keeps - [Relay memory](/quest/m1/relay-memory.md) - remeasure what an announcement costs after prefix routes +- [Front deadlines](/quest/m1/front-deadline-index.md) - a front's per-event cost stops growing with its track count: an expiry index and per-track wakes, proven by a churn benchmark - [Front parking](/quest/m1/origin-front-parks.md) - an unroutable request waits on a front instead of re-asking on every route-table move - [Publish channel count](/quest/m1/publish-audio-channel-count.md) - forcing a channel count on an Audio.Capture stops costing the subscriber gaps of silence - [JS abandonment](/quest/m1/js-subscribe-abandonment.md) - a viewer returning during IETF subscribe setup keeps its track across microtasks - [IETF stream types](/quest/m1/ietf-uni-stream-types.md) - padding streams are discarded stream-only and an unknown uni type closes the session, per draft-21 +- [Rust decode caps](/quest/m1/coding-reader-cap.md) - every Rust decode that allocates from a peer-supplied length is bounded - [Epoch primitive](/quest/m1/epoch.md) - one `Epoch` type in moq-net and @moq/net, carried as a trailing `@` path segment, shared by e2ee and broadcast epochs - [E2EE](/quest/m1/e2ee/README.md) - TypeScript and Rust peers interoperate over encrypted broadcasts no relay can decrypt - [Broadcast epochs](/quest/m1/broadcast-epoch/README.md) - each publish of a name gets a fresh `@` epoch, viewers follow the newest live one at once, and bare names still resolve on every version diff --git a/quest/m1/ci-hygiene.md b/quest/m1/ci-hygiene.md new file mode 100644 index 0000000000..b535f60eb5 --- /dev/null +++ b/quest/m1/ci-hygiene.md @@ -0,0 +1,25 @@ +# [XS] CI runs don't cancel each other or cache a broken build + +## Goal + +A push to one PR never cancels another PR's run, and a failed run never +saves the Rust cache later runs restore. + +## Plan + +- Concurrency: `check.yml`, `obs.yml`, `android.yml`, `wasm.yml`, and + `interop.yml` group by `${{ github.ref }}` only. Key them by event and PR + number the way `platform.yml` does (#4370). A `pull_request` run's + `github.ref` is already `refs/pull//merge`, so first find which runs + actually shared a group, and confirm the new key separates them. +- Cache: `interop.yml` and `swift.yml` set Swatinem's + `cache-on-failure: true`; #4384's interop cache got saved broken that way + (`failed to run custom build command for aws-lc-rs`). Drop it. Check + whether the shared `.github/actions/rust-cache` saves on failure, and make + it save only on success. + +Public API: none. Wire: none. + +## Related + +- [Merge queue](/quest/m1/merge-queue.md) - also touches the concurrency groups for queue refs diff --git a/quest/m1/coding-reader-cap.md b/quest/m1/coding-reader-cap.md new file mode 100644 index 0000000000..68b22a6a79 --- /dev/null +++ b/quest/m1/coding-reader-cap.md @@ -0,0 +1,21 @@ +# [XS] Rust decoders cap a bare byte or string read + +## Goal + +Every Rust decode path that allocates from a peer-supplied length has an +upper bound. #4373 fixed a JS size-cap bypass; the lite and IETF message +decoders in Rust already check every length prefix, but `coding/reader.rs` +has no overall cap for a bare `Vec` or `String` decode. + +## Plan + +- Audit `rs/moq-net/src/coding/` for decodes that allocate from a length + prefix without a bound, and list which call sites reach them from the + wire. +- Where one is reachable, bound it by the message's size limit and refuse + anything larger with a decode error. If none is reachable, add the bound + anyway only if it's one line; otherwise delete this quest with the audit + in the PR. + +Public API: none. Wire: oversized fields are refused, which they already +should be. diff --git a/quest/m1/exact-scope.md b/quest/m1/exact-scope.md new file mode 100644 index 0000000000..3e20a7b706 --- /dev/null +++ b/quest/m1/exact-scope.md @@ -0,0 +1,24 @@ +# [S] An exact broadcast outside a reader's scope stays hidden + +## Goal + +A reader scoped to `/a/b` never sees an exact broadcast at `/a`, in Rust or +JS. A prefix advertisement at or above the scope still presents as the empty +path, because it may serve paths under the scope; an exact broadcast cannot. + +## Plan + +Decided 2026-09-28: exact match everywhere, aligning on the JS local view. + +- Rust: `sync_cursor` in `rs/moq-net/src/model/origin.rs` admits an exact + entry by prefix overlap. Match exact entries against the reader's patterns + instead. `cursor_keeps_an_overlapping_prefix_above_its_scope` covers a + prefix route and stays. +- JS wire view: `Scope.projectRoutes` in `js/net/src/origin.ts` treats every + entry above the root as covering. Skip exact entries there, as `#listed` + already does with `candidate.exact ? pattern.matches(path)`. +- Add the same case to both languages' tests: an exact broadcast at `/a` and + a prefix route at `/a`, read through a `/a/b` scope, yield only the prefix. + +Public API: none. Wire: a narrower reader receives fewer announcements; no +message changes. diff --git a/quest/m1/front-deadline-index.md b/quest/m1/front-deadline-index.md new file mode 100644 index 0000000000..50388b5a36 --- /dev/null +++ b/quest/m1/front-deadline-index.md @@ -0,0 +1,34 @@ +# [M] Front deadlines scale with the touched track + +## Goal + +A relay front's per-event cost no longer grows with the number of tracks it +holds. Today `front.rs` `next_deadline` scans every track to find the next +linger expiry, and `run_front`'s per-wake poll in `origin.rs` scans every +track too, so each event on one track costs O(tracks) and a broadcast that +churns track names pays O(tracks²). After this, finding the next expiry and +reacting to one track's event touch only that track. + +## Plan + +- Benchmark first, in `rs/moq-net/benches/origin.rs`: churn tracks through one + front, swept over tracks per front and readers per track, so today's slope + shows before the fix and the fix's flatness after. +- Keep parked tracks in an expiry index ordered by `since + linger` (a + `BTreeMap` or a heap with lazy deletion), updated on every + `Parked`/unparked transition, so `next_deadline` is its first entry and the + deadline sweep pops only what expired. +- Replace the driver's scan-every-track poll with per-track wakes, so an + event on one track polls that track. +- Keep the front's exhaustive walk test passing, and add a unit test that the + index agrees with a full scan across random transitions. +- Not in scope: kio's level-only demand, which lets a reader that comes and + goes between polls skip restarting the linger. It costs one extra source + request, so it doesn't justify a kio generation counter (decided + 2026-09-28). + +Public API: none. Wire: none. + +## Related + +- [Front parking](/quest/m1/origin-front-parks.md) - also changes what a front holds, and wants a churn benchmark over requesters diff --git a/quest/m1/interop-audio-cold-start.md b/quest/m1/interop-audio-cold-start.md new file mode 100644 index 0000000000..78c2522dd0 --- /dev/null +++ b/quest/m1/interop-audio-cold-start.md @@ -0,0 +1,29 @@ +# [S] Interop audio tone survives a cold start + +## Goal + +The interop "Media output and lifecycle" step stops failing the audio tone +check on cold start. The 2026-09-28 nightly on `main` failed with +`FAIL audio tone: cold start: 55/64 samples carried the fixture tone above the +noise floor`, against a 90% agreement floor. + +## Plan + +- Reproduce it: run the interop media step in a loop on a loaded machine + until the cold-start row fails, and capture which samples miss (the first + ones, or scattered). +- Find the cause before touching the check: the likely suspects are decoder + or playout warm-up after the context resumes. [Publish channel count](/quest/m1/publish-audio-channel-count.md) + saw the same symptom, but `test/interop/clients/js/src/fixture.ts` already + dropped the `channelCount` override, so that's not it. +- Fix it at the source. The cold-start window deliberately starts once the + context runs, without waiting for a tone, so a slow start has to fail + there. Keep that window: if the misses are the first samples, find what + delays the first audible output instead of moving the window. Don't lower + `AGREEMENT` (`test/interop/clients/js/media.ts`) without a measured reason. + +Public API: none. Wire: none. + +## Related + +- [More tests under load](/quest/m1/test-flakes-2.md) - the other load-only failures diff --git a/quest/m1/js-fetch-cancel.md b/quest/m1/js-fetch-cancel.md new file mode 100644 index 0000000000..e7371fb4fd --- /dev/null +++ b/quest/m1/js-fetch-cancel.md @@ -0,0 +1,25 @@ +# [S] A JS fetch can be cancelled + +## Goal + +A `@moq/net` caller can abandon one pending group fetch without closing the +track or session, the follow-up #4357 left. + +## Plan + +Decided 2026-09-28: `FetchGroupOptions` (`js/net/src/track.ts`) gains +`signal?: AbortSignal`, the standard JS idiom, and additive. + +- Thread it through `js/net/src/broadcast.ts` and `lite/subscriber.ts`. + `ietf/subscriber.ts` rejects `fetchGroup` today (moq-transport has no + one-shot group fetch), so it stays unchanged. +- Fetches for the same group share one stream (`lite/subscriber.ts`), so an + abort releases this caller's share and rejects its promise with the + signal's reason. The stream is cancelled only when the last sharer leaves. +- An already-aborted signal rejects before anything is sent. +- Tests: one of two sharers aborts and the other still receives the group; + the last sharer aborting cancels the stream. +- Document the option where fetch is documented under `doc/`. + +Public API: additive `FetchGroupOptions.signal`. Wire: none new; the +existing cancel path. diff --git a/quest/m1/js-track-close-end.md b/quest/m1/js-track-close-end.md new file mode 100644 index 0000000000..c86f3e1661 --- /dev/null +++ b/quest/m1/js-track-close-end.md @@ -0,0 +1,20 @@ +# [XS] A JS track's clean close ends at its own last group + +## Goal + +`close()` on a JS track producer declares the final sequence from the groups +that track actually produced, like Rust's `finish()` uses its own +`max_sequence`. A sibling producer on the same broadcast and name can no +longer push the end past groups this track will ever send. + +## Plan + +- `close()` in `js/net/src/track.ts` calls `#declareFinal(this.#sequence.next)`, + and `#sequence` is shared per broadcast and track name (`bindProducer`). + Use the track's own `#received` high-water mark instead, which #4385 added + and only `#settled` reads today. +- Re-check `finishAt` for the same shared-counter assumption. +- Regression test: two producers on one name, the sibling creates later + groups, and the closed track's final sequence is its own last group + 1. + +Public API: none. Wire: none. diff --git a/quest/m1/relay-auth-client-ca.md b/quest/m1/relay-auth-client-ca.md index 5393c341df..3462393429 100644 --- a/quest/m1/relay-auth-client-ca.md +++ b/quest/m1/relay-auth-client-ca.md @@ -24,6 +24,10 @@ config instead of quietly refusing every session. auth configured, which `MoqSide::validate` permits and whose peers admit through the cluster. Make that case explicit and let any other error stop startup. +- A listener TLS client CA on a stream-only relay (no QUIC owner at all, + the `NoBackend` case in `rs/moq-relay/src/relay.rs`) is never checked, so + refuse that config at load instead of silently ignoring the CA. Worker-owned + QUIC (`quic_owned_elsewhere`) still enforces the CA, so keep accepting it. - Update every caller, the tests #4364 added in both `moq-cli` and `moq-relay`, and `doc/bin/relay/auth.md` or `doc/lib/rs` wherever they name the methods. From 75d93c3e5f967c42342b3e2a5d0a2de2cbd453f5 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Mon, 28 Sep 2026 21:41:27 -0700 Subject: [PATCH 25/37] fix(tokio): drain a GOAWAY predecessor on Connection::close (#4436) Co-authored-by: Claude Opus 5.5 --- rs/moq-tokio/src/connection.rs | 70 +++++++-- rs/moq-tokio/tests/backend.rs | 255 +++++++++++++++++++++++++++++++++ 2 files changed, 310 insertions(+), 15 deletions(-) diff --git a/rs/moq-tokio/src/connection.rs b/rs/moq-tokio/src/connection.rs index 8268bf1ef5..1a5698049c 100644 --- a/rs/moq-tokio/src/connection.rs +++ b/rs/moq-tokio/src/connection.rs @@ -461,6 +461,9 @@ struct State { /// The currently-connected session, or `None` while reconnecting. Read by /// [`Monitor`] to snapshot live connection stats. session: Option, + /// The loop's [`Draining`] predecessor and its handover deadline, so + /// [`Connection::close`] drains it too without overstaying that window. + predecessor: Option<(moq_net::Session, tokio::time::Instant)>, } /// The producer side of everything a [`Connection`] handle can observe. @@ -723,25 +726,45 @@ impl Connection { self.task.handle.abort(); } - /// Stop the loop for every clone, closing the live session once the data it - /// queued has been delivered. + /// Stop the loop for every clone, closing the live session (and any predecessor + /// still finishing after a GOAWAY) once the data it queued has been delivered. /// /// See [`moq_net::Session::close`]: finished tracks deliver their last groups and - /// FIN first, bounded by a one second deadline. Call this before + /// FIN first, bounded by a one second deadline (or a predecessor's remaining + /// handover window, if sooner). Call this before /// [`Client::close`], which closes the transport without waiting. Returns `Ok` - /// when nothing was live, and the session's error if it did not drain. + /// when nothing was live, and a session's error if it did not drain. pub async fn close(self) -> crate::Result<()> { - // Refuse redials and take the session under one lock: see [`CloseGuard`]. - let session = { + // Refuse redials and take the sessions under one lock: see [`CloseGuard`]. + let (session, predecessor) = { let mut closed = self.task.closed.lock().unwrap(); *closed = Some(moq_net::Error::Cancel); - self.state.read().session.clone() + let state = self.state.read(); + (state.session.clone(), state.predecessor.clone()) }; self.task.handle.abort(); - match session { - Some(session) => Ok(session.close().await?), - None => Ok(()), - } + let session = async move { + match session { + Some(session) => session.close().await, + None => Ok(()), + } + }; + let predecessor = async move { + let Some((session, deadline)) = predecessor else { + return Ok(()); + }; + let abort = session.clone(); + let mut close = std::pin::pin!(session.close()); + tokio::select! { + res = &mut close => res, + _ = tokio::time::sleep_until(deadline) => { + abort.abort(moq_net::Error::GoawayTimeout); + close.await + } + } + }; + let (session, predecessor) = tokio::join!(session, predecessor); + Ok(session.and(predecessor)?) } async fn run(shared: &Shared, client: Client, addrs: Addrs) -> crate::Result<()> { @@ -818,13 +841,15 @@ impl Connection { // replacement at a group boundary. Tearing it down here instead // would drop every group published until the replacement caught up. tracing::info!(peer = %Endpoint(&url), "upstream GOAWAY; migrating"); - shared.migrating(); // Retire any predecessor first: overwriting would drop its deadline // on the floor and leave it holding the connection open. if let Some(mut old) = draining.take() { old.retire(); } - draining = Some(Draining::new(session, goaway.handover(msg.timeout()))); + draining = Some(Draining::new(session, goaway.handover(msg.timeout()), &shared.state)); + // After the predecessor is published, so a close woken by this + // status finds it and honors its handover deadline. + shared.migrating(); if healthy { delay = initial; @@ -1192,10 +1217,16 @@ struct Draining { session: moq_net::Session, closed: std::pin::Pin + Send>>, deadline: std::pin::Pin>, + /// Mirrors the session into [`State::predecessor`] for as long as this lives. + state: kio::Producer, } impl Draining { - fn new(session: moq_net::Session, handover: Duration) -> Self { + fn new(session: moq_net::Session, handover: Duration, state: &kio::Producer) -> Self { + let deadline = Box::pin(tokio::time::sleep(handover)); + if let Ok(mut state) = state.write() { + state.predecessor = Some((session.clone(), deadline.deadline())); + } let closed = { let session = session.clone(); Box::pin(async move { @@ -1206,7 +1237,8 @@ impl Draining { Self { session, closed, - deadline: Box::pin(tokio::time::sleep(handover)), + deadline, + state: state.clone(), } } @@ -1230,6 +1262,14 @@ impl Draining { } } +impl Drop for Draining { + fn drop(&mut self) { + if let Ok(mut state) = self.state.write() { + state.predecessor = None; + } + } +} + /// Poll a draining predecessor, retiring it once it closes or overstays its /// handover window. `true` on the pass that retires it. /// diff --git a/rs/moq-tokio/tests/backend.rs b/rs/moq-tokio/tests/backend.rs index f4c3c026a6..01ef2a2bb2 100644 --- a/rs/moq-tokio/tests/backend.rs +++ b/rs/moq-tokio/tests/backend.rs @@ -834,6 +834,261 @@ async fn noq_client_close_drains_finished_track() { .expect("client thread panicked"); } +/// A GOAWAY leaves the old session serving its subscriptions after the replacement +/// connects, and a close then drains that predecessor too rather than dropping it. +#[cfg(feature = "noq")] +#[tracing_test::traced_test] +#[tokio::test] +async fn noq_client_close_drains_migrated_predecessor() { + // Several congestion windows, so it is still in flight when the close starts. + let payload: Vec = (0..64 * 1024).map(|i| i as u8).collect(); + + let quic = moq_tokio::quic::Config::default(); + let mut server_config = moq_tokio::listen::Config::default(); + server_config.bind = Some("127.0.0.1:0".parse().unwrap()); + server_config.tls.generate = vec!["localhost".into()]; + let server = server_config.init(quic.clone()).expect("failed to init server"); + let mut server = server.listen().await.expect("failed to listen"); + let url: url::Url = format!("moqt://localhost:{}", server.local_addr().unwrap().port()) + .parse() + .unwrap(); + + // The client's runtime is gone as soon as it returns, as when a process exits. + let (subscribed_tx, subscribed_rx) = tokio::sync::oneshot::channel::<()>(); + let (migrated_tx, migrated_rx) = tokio::sync::oneshot::channel::<()>(); + let expected = payload.clone(); + let client = std::thread::spawn(move || { + let runtime = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .expect("client runtime"); + runtime.block_on(async move { + let origin = moq_tokio::origin::spawn(); + let broadcast = origin.create_broadcast("test").expect("failed to create broadcast"); + broadcast.announce(Default::default()).expect("failed to announce"); + let mut track = broadcast.create_track("video", None).expect("failed to create track"); + + let mut config = moq_tokio::connect::Config::default(); + config.tls.insecure = Some(true); + config.bind = Some("127.0.0.1:0".parse().unwrap()); + let client = config + .init(quic) + .expect("failed to init client") + .with_publisher(origin.consume()); + let mut connection = client.connect(url).established().await.expect("client connect failed"); + + while track.subscription().is_none() { + track.subscription_changed().await.expect("track closed"); + } + subscribed_tx.send(()).unwrap(); + + // Once the replacement is live, only the predecessor serves the subscription. + while connection.epoch() < 2 || !connection.connected() { + connection.status().await.expect("connection stopped"); + } + migrated_tx.send(()).unwrap(); + + let mut group = track.append_group().expect("failed to append group"); + group + .write_frame(moq_tokio::moq_net::Timestamp::ZERO, payload) + .expect("failed to write frame"); + group.finish().expect("failed to finish group"); + track.finish().expect("failed to finish track"); + + connection.close().await.expect("the close drains"); + client.close().await; + }); + }); + + let request = tokio::time::timeout(TIMEOUT, server.accept()) + .await + .expect("accept timed out") + .expect("no incoming connection"); + let origin = moq_tokio::origin::spawn(); + let consumer = origin.consume(); + let mut announcements = consumer.announced(); + let first = request + .with_subscriber(origin) + .ok() + .await + .expect("server handshake failed"); + + tokio::time::timeout(TIMEOUT, announcements.next()) + .await + .expect("announce timed out") + .expect("origin closed"); + let broadcast = tokio::time::timeout(TIMEOUT, consumer.request_broadcast("test")) + .await + .expect("request timed out") + .expect("announced broadcast resolves"); + let mut track = tokio::time::timeout(TIMEOUT, broadcast.track("video").unwrap().subscribe(None)) + .await + .expect("subscribe timed out") + .expect("subscribe failed"); + + // Receiving is what sends the subscription, so read in the background. + let received = tokio::spawn(async move { + let mut group = track + .recv_group() + .await + .expect("recv_group failed") + .expect("track ended before the group"); + let frame = group + .read_frame() + .await + .expect("read_frame failed") + .expect("group ended before the frame"); + (frame.payload, track.recv_group().await.map(|group| group.is_some())) + }); + tokio::time::timeout(TIMEOUT, subscribed_rx) + .await + .expect("the subscription never reached the client") + .expect("client thread panicked"); + + first + .drain() + .send(moq_tokio::moq_net::goaway::Goaway::new()) + .expect("send goaway"); + let request = tokio::time::timeout(TIMEOUT, server.accept()) + .await + .expect("the replacement never dialed") + .expect("no incoming connection"); + let _second = request + .with_subscriber(moq_tokio::origin::spawn()) + .ok() + .await + .expect("server handshake failed"); + tokio::time::timeout(TIMEOUT, migrated_rx) + .await + .expect("the client never migrated") + .expect("client thread panicked"); + + let (payload, end) = tokio::time::timeout(TIMEOUT, received) + .await + .expect("the track timed out") + .expect("reader panicked"); + assert!(payload[..] == expected[..], "the frame arrives whole"); + match end { + Ok(false) => {} + Ok(true) => panic!("an unexpected second group"), + Err(err) => panic!("the track ends with {err} instead of finishing"), + } + + tokio::task::spawn_blocking(move || client.join()) + .await + .unwrap() + .expect("client thread panicked"); +} + +/// A close cuts a predecessor that cannot drain off at its handover deadline, +/// rather than holding it for the full one second close window. +#[cfg(feature = "noq")] +#[tracing_test::traced_test] +#[tokio::test] +async fn noq_client_close_keeps_predecessor_handover() { + let quic = moq_tokio::quic::Config::default(); + let mut server_config = moq_tokio::listen::Config::default(); + server_config.bind = Some("127.0.0.1:0".parse().unwrap()); + server_config.tls.generate = vec!["localhost".into()]; + let server = server_config.init(quic.clone()).expect("failed to init server"); + let mut server = server.listen().await.expect("failed to listen"); + let url: url::Url = format!("moqt://localhost:{}", server.local_addr().unwrap().port()) + .parse() + .unwrap(); + + let (subscribed_tx, subscribed_rx) = tokio::sync::oneshot::channel::<()>(); + let client = std::thread::spawn(move || { + let runtime = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .expect("client runtime"); + runtime.block_on(async move { + let origin = moq_tokio::origin::spawn(); + let broadcast = origin.create_broadcast("test").expect("failed to create broadcast"); + broadcast.announce(Default::default()).expect("failed to announce"); + // Never finished, so the predecessor serving it cannot drain. + let mut track = broadcast.create_track("video", None).expect("failed to create track"); + + let mut config = moq_tokio::connect::Config::default(); + config.tls.insecure = Some(true); + config.bind = Some("127.0.0.1:0".parse().unwrap()); + // Well under the one second close window. + config.goaway.handover = Duration::from_millis(500); + let client = config + .init(quic) + .expect("failed to init client") + .with_publisher(origin.consume()); + let mut connection = client.connect(url).established().await.expect("client connect failed"); + + while track.subscription().is_none() { + track.subscription_changed().await.expect("track closed"); + } + subscribed_tx.send(()).unwrap(); + + // Close while the old session is still inside its handover window. + while connection.status().await.expect("connection stopped") != moq_tokio::Status::Migrating {} + let start = std::time::Instant::now(); + let err = connection.close().await.expect_err("the predecessor cannot drain"); + let elapsed = start.elapsed(); + client.close().await; + (err, elapsed) + }) + }); + + let request = tokio::time::timeout(TIMEOUT, server.accept()) + .await + .expect("accept timed out") + .expect("no incoming connection"); + let origin = moq_tokio::origin::spawn(); + let consumer = origin.consume(); + let mut announcements = consumer.announced(); + let first = request + .with_subscriber(origin) + .ok() + .await + .expect("server handshake failed"); + + tokio::time::timeout(TIMEOUT, announcements.next()) + .await + .expect("announce timed out") + .expect("origin closed"); + let broadcast = tokio::time::timeout(TIMEOUT, consumer.request_broadcast("test")) + .await + .expect("request timed out") + .expect("announced broadcast resolves"); + let mut track = tokio::time::timeout(TIMEOUT, broadcast.track("video").unwrap().subscribe(None)) + .await + .expect("subscribe timed out") + .expect("subscribe failed"); + // Receiving is what sends the subscription, so read in the background. + let _received = tokio::spawn(async move { track.recv_group().await.map(|group| group.is_some()) }); + tokio::time::timeout(TIMEOUT, subscribed_rx) + .await + .expect("the subscription never reached the client") + .expect("client thread panicked"); + + first + .drain() + .send(moq_tokio::moq_net::goaway::Goaway::new()) + .expect("send goaway"); + + let (err, elapsed) = tokio::task::spawn_blocking(move || client.join()) + .await + .unwrap() + .expect("client thread panicked"); + // The one second close window would have ended it with a timeout instead. + assert!( + !matches!(err, moq_tokio::Error::MoqNet(moq_tokio::moq_net::Error::Timeout)), + "the handover deadline cuts the close short: {err:?}" + ); + // The window opened just before the status flipped, so most of it remains. Loose, + // since this is wall-clock time, but a deadline firing at once would land near zero. + assert!( + elapsed >= Duration::from_millis(250), + "the close waited out the handover window: {elapsed:?}" + ); +} + #[cfg(feature = "noq")] #[tracing_test::traced_test] #[tokio::test] From 57eaf69a7baf386303223708dcbf5502acebecf6 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Mon, 28 Sep 2026 21:47:27 -0700 Subject: [PATCH 26/37] feat(publish): announce once every captured rendition resolves (#4414) Co-authored-by: Claude Opus 5.5 --- doc/lib/js/publish.md | 2 +- js/publish/README.md | 2 +- js/publish/src/announce.test.ts | 124 ++++++++++++++++++ js/publish/src/announce.ts | 46 +++++++ js/publish/src/audio/capture.test.ts | 68 +++++++++- js/publish/src/audio/capture.ts | 117 ++++++++++++----- js/publish/src/audio/encoder.test.ts | 63 ++++++++- js/publish/src/audio/encoder.ts | 45 ++++++- js/publish/src/broadcast.test.ts | 35 ++++- js/publish/src/broadcast.ts | 49 ++++--- js/publish/src/element.ts | 23 ++-- js/publish/src/video/encoder.test.ts | 31 +++++ js/publish/src/video/encoder.ts | 50 +++++-- ...-reservation-gating-in-moq-hang-js-hang.md | 40 ------ quest/m1/README.md | 1 - quest/m1/publish-codec-string.md | 12 +- 16 files changed, 577 insertions(+), 131 deletions(-) create mode 100644 js/publish/src/announce.test.ts create mode 100644 js/publish/src/announce.ts delete mode 100644 quest/m1/2075-mirror-catalog-reservation-gating-in-moq-hang-js-hang.md diff --git a/doc/lib/js/publish.md b/doc/lib/js/publish.md index 6c9c1dd0c5..7bfe1a28a8 100644 --- a/doc/lib/js/publish.md +++ b/doc/lib/js/publish.md @@ -31,7 +31,7 @@ WebCodecs, writes the catalog, and publishes a hang broadcast. | `source` | `camera`, `screen`, or `file`. | | `muted`, `invisible` | Disable audio or video capture. | | `preview` | What the nested element shows: the raw `source` (default), a decoded copy of the `encoded` stream to see what viewers get, or `none`. | -| `announce` | When to advertise: once a `source` is live (default), `always`, or `never`. A camera source waits for every enabled track. The broadcast is created while connected either way, but nobody can see or subscribe to it until it is announced. | +| `announce` | When to advertise: once a `source` is live (default), `always`, or `never`. A camera source waits for every enabled track, and `source` waits until each captured track's config resolves or fails, so the first catalog lists every rendition. The broadcast is created while connected either way, but nobody can see or subscribe to it until it is announced. | A nested `