quest: plan follow-ups from the current PR wave #4304
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| # [M] Apps show "no broadcasts" from the live marker | ||
|
|
||
| ## Goal | ||
|
|
||
| A browser page listing broadcasts shows an empty state once the relay has | ||
| said there are none, never a spinner that never resolves and never a false | ||
| empty state before the first session answered. The demo watch page and | ||
| `@moq/room` use `@moq/net`'s `live` marker, and `@moq/net` settles when an | ||
| origin stream opened before the first connection goes live. | ||
|
|
||
| ## Plan | ||
|
|
||
| - #4261 (on `dev`) adds the `live` event; the consumers it touches | ||
| (`demo/web/src/index.ts`, `js/room/src/room.ts`, `js/watch/src/broadcast.ts`, | ||
| `js/moq-boy`) skip it today. | ||
| - Page load: an origin stream opened before any session connects has no | ||
| session to wait on, so today it goes `live` at once and broadcasts arrive | ||
| after it. Settled: the reconnect loop (`js/net/src/connection/reload.ts`), | ||
| which already answers requests through `expect()`, holds the marker until | ||
| its first session lands `live` or its first dial gives up. An empty list | ||
| then means the relay said so or is unreachable, which a UI can tell apart. | ||
| Once a session is up, its own `live` ends the hold: every wire guarantees | ||
| one (ANNOUNCE_OK, ANNOUNCE_INIT, or the quiet-stream fallback). A peer that | ||
| accepts the announce stream and never answers is a peer bug, so no extra | ||
| timeout. | ||
| Check whether Rust's reconnecting client has the same gap. | ||
| - Apps: loading before `live`, an explicit empty state after it with nothing | ||
| announced, and an error state when the connection gives up. Libraries | ||
| expose the state as a signal; wording stays in the demo. | ||
| - Tests: an origin stream opened before connect is not `live` until the first | ||
| session is; a connection that cannot connect ends the wait. | ||
|
|
||
| Public API: when `@moq/net` emits `live` changes; any state signal on | ||
| `@moq/room` or `@moq/watch` is additive. Lands on `dev` with #4261. Wire: none. | ||
|
|
||
| ## Required | ||
|
|
||
| - [JS caught up](/quest/m1/js-announce-caught-up.md) - the `live` marker (#4261) | ||
|
|
||
| ## Related | ||
|
|
||
| - [Bindings caught up](/quest/m1/announce-live-bindings.md) - the same marker in the bindings |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| # [S] Plan: wall-clock age-out in the track cache | ||
|
|
||
| ## Goal | ||
|
|
||
| A benchmark decides whether a track's cache ages groups out without waiting | ||
| for a write, on `max(wall, pts)` like other time decisions in moq-net, or | ||
| stays the one write-driven exception. The result is an implementation quest | ||
| or a recorded reason to keep the exception. | ||
|
|
||
| ## Plan | ||
|
|
||
| Settled scope: a track's `max_age` retention aging groups out on a timer | ||
| instead of only on a write. The pool's idle expiry is out of scope. | ||
|
|
||
| - Today a track's `max_age` is media time and is applied only when the track | ||
| writes: a group ages out when a later one starts (`is_stale` and the expiry | ||
| scans in `rs/moq-net/src/model/track.rs`). A track that stops writing keeps | ||
| groups past `max_age` until the pool's idle expiry (`Pool::gc`, driven by | ||
| the origin driver without a write) or byte pressure reclaims them. | ||
| `max_age_does_not_drive_wall_eviction` pins that behaviour. | ||
| - Prototype the alternative behind a bench-only switch: each track keeps a | ||
| deadline for its oldest group on `max(wall elapsed, pts)`, armed on the | ||
| timers the origin driver already runs, and evicts on expiry. | ||
| - Bench in `rs/moq-net/benches/track.rs`, swept over tracks (1 to 10k) and | ||
| cached groups per track (1 to 1k): write-path cost, timer cost per driver | ||
| pass, and retained memory for a population of idle tracks. A cost that | ||
| grows with the table should show as a slope. | ||
| - Weigh the semantics too: `max_age` is media time on purpose, so a congestion | ||
| stall cannot age content out (`track::Info::max_age`). A wall term changes | ||
| that for a stalled but live publisher. | ||
| - Record the numbers and the decision in the PR, then rewrite this quest into | ||
| the implementation or delete it. | ||
|
|
||
| Public API: none from the plan. Wire: none. | ||
|
|
||
| ## Related | ||
|
|
||
| - [Cache expiry growth](/quest/m1/cache-expiry-growth.md) - relay memory past the expiry window, in the same cache | ||
| - [Cache shard](/quest/m1/perf/cache-shard.md) - the pool's shared counters under many workers |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| # [M] Bindings stamp data frames with a capture time | ||
|
|
||
| ## Goal | ||
|
|
||
| A moq-ffi publisher, and every wrapper over it (Python, Swift, Kotlin, Go, | ||
| Dart), can pass a capture time with a JSON or binary snapshot `update` or | ||
| stream `append`, so its data tracks advertise `delay` and `jitter` like a Rust | ||
| publisher's. Leaving it out keeps today's behaviour. `moq-json`'s `window` | ||
| producer takes a capture time too. Settled scope: moq-ffi and its wrappers, | ||
| not libmoq. | ||
|
|
||
| ## Plan | ||
|
|
||
| - #4270 gives the Rust producers `moq_net::Timed<P, T>`, built with | ||
| `Timed::from(value).at(t)`. The `moq-mux` data producers take | ||
| `Timed<_, Instant>`, map it onto the broadcast clock, and refuse one ahead of | ||
| now (`Error::InvalidCapture`). The moq-ffi producers in | ||
| `rs/moq-ffi/src/{json,binary}.rs` pass bare values. | ||
| - Settled: the capture time is a media timestamp on the broadcast's | ||
| timeline. moq-ffi exposes the broadcast clock's `now()` as a timestamp; | ||
| callers stamp payloads with values taken from it, and moq refuses one ahead | ||
| of now. That keeps a device or process clock out, as the Rust `Instant` | ||
| mapping does. The `moq-mux` producers take an `Instant` today, so either | ||
| map the timestamp back through the clock inside moq-ffi or give the clock a | ||
| typed timestamp moq-mux accepts; keep a raw `Timestamp` from compiling | ||
| there. Make it optional on the existing methods, never a `_with_capture` | ||
| twin. | ||
| - `moq-json` window: `window::Producer::push` stamps `Timestamp::now()` | ||
| (`rs/moq-json/src/window/producer.rs`). Accept `Timed` as the snapshot and | ||
| stream producers do. Nothing in `moq-mux` publishes window mode, so there is | ||
| no estimator to feed. | ||
| - Wrappers follow per the cross-package sync table, each with a test that a | ||
| past capture time is accepted and a future one refused. Update | ||
| `doc/lib/{py,swift,kt,go,dart}`. | ||
|
|
||
| Public API: breaking, so it lands on `dev`. A new parameter on the generated | ||
| `update` and `append` breaks every published binding caller (Go, for one, has | ||
| no optional arguments), and a `_with_x` twin is ruled out. The broadcast clock | ||
| `now()` is additive; `window::Producer::push` accepts `Timed`, source-compatible. | ||
| Wire: none. | ||
|
|
||
| ## Required | ||
|
|
||
| - [Data jitter](/quest/m1/data-jitter.md) - `Timed` and the mux capture path (#4270) | ||
|
|
||
| ## Related | ||
|
|
||
| - [FFI shape](/quest/m1/ffi-shape/README.md) - moves the data producers into a json namespace | ||
| - [Generated C bindings](/quest/m1/c/README.md) - replaces libmoq, so C inherits this from moq-ffi | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| # [M] Every binding prints MoqError's message | ||
|
|
||
| ## Goal | ||
|
|
||
| Python's `str(err)`, Go's `err.Error()`, and Dart's `toString()` on a | ||
| `MoqError` return the message from moq-ffi's exported `Display`, as Kotlin's | ||
| `toString()` and Swift's `description` already do. No hand-written | ||
| per-variant strings and no extra message function. | ||
|
|
||
| ## Plan | ||
|
|
||
| - #4292 adds `#[uniffi::export(Display)]` on `MoqError` in | ||
| `rs/moq-ffi/src/error.rs`, on the C++ line. If it has not reached `main` | ||
| when this starts, add the same attribute here; it is additive. | ||
| - Go and Dart are fixed in their generators; Python in its wrapper: | ||
| - Go: the `kixelated/uniffi-bindgen-go` fork. The generated `Error()` prints | ||
| `MoqError: <variant>`. Render the exported `Display` for errors, tag, and | ||
| bump every pin site the `flake.nix` comment lists. | ||
| - Dart: the `kixelated/uniffi-dart` fork. The regenerated bindings already | ||
| carry the unused extern; wire it to `toString()`, tag, bump `flake.nix`, | ||
| and regenerate `dart/moq_ffi`. | ||
|
Comment on lines
+17
to
+21
There was a problem hiding this comment.
The Go and Dart work requires fixing and tagging two external generators before this repository can bump their pins and regenerate bindings, but the plan folds those releases into the dependent binding quest. Give each unblocking release/pin operation its own quest and make the applicable wrapper work require it, so this quest is not blocked midway on external releases. AGENTS.md reference: quest/AGENTS.md:L85-L86 Useful? React with 馃憤聽/ 馃憥.
Collaborator
Author
There was a problem hiding this comment. Disagree. That rule covers a release that unblocks other quests. Here nothing else waits on the generator tags: the fork fix, the tag, the pin bump, and the regenerated bindings are all one change in forks this org owns. (Written by Claude Opus 5.5) |
||
| - Python: upstream `mozilla/uniffi-rs` (0.32.2 here) renders no uniffi | ||
| traits on errors (`ErrorTemplate.py`). Settled: no upstream PR. The | ||
| hand-written `py/moq-rs` package defines `__str__` on `MoqError` by | ||
| calling the exported `Display`, so the message still comes from Rust. | ||
| - A test per binding that a known error prints Rust's text (`Closed` prints | ||
| `closed`), and `doc/lib/{py,go,dart}` updated where they show error output. | ||
|
|
||
| Public API: the string form of `MoqError` changes in Python, Go, and Dart. | ||
| Wire: none. | ||
|
|
||
| ## Related | ||
|
|
||
| - [C++ through moq-ffi](/quest/m1/cpp/README.md) - where #4292 adds the export and C++ `to_string()` | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| # [XS] Go cancel test keeps its origin | ||
|
|
||
| ## Goal | ||
|
|
||
| `TestRequestBroadcastCancelKeepsTheOrigin` in `go/wrapper/moq_test.go` passes | ||
| under load, not only alone (30/30 in isolation, `MoqError: Closed` under a | ||
| loaded run). Fixed at the cause, with no retry or longer timeout. | ||
|
|
||
| ## Plan | ||
|
|
||
| Likely cause, found by reading and not yet reproduced: the test never touches | ||
| `origin` after `origin.Dynamic(...)` and `origin.Consume()`, so Go may collect | ||
| it mid-test. The generated finalizer then drops the only `MoqOriginProducer`, | ||
| the origin's driver finishes once every producer handle is gone | ||
| (`origin::Driver::poll` in `rs/moq-net/src/model/origin.rs`), and the next call | ||
| on the consumer or the dynamic handle gets `Error::Closed`. The collector runs | ||
| more often under load, which fits. | ||
|
|
||
| - Reproduce first. `runtime.GC()` after `cancel()` or `GOGC=1` only makes the | ||
| collection likely: finalizers run later on their own goroutine. For a | ||
| deterministic failure, do what the finalizer does: an internal test calls | ||
| the inner handle's `Destroy` before the next call. | ||
| - Keep the producer reachable to the end of the test (`runtime.KeepAlive`, as | ||
| the file already does for `pending`), and audit the other `go/wrapper` tests | ||
| that hold an `OriginProducer` only for setup. | ||
| - Users hit the same trap: a Go `OriginProducer` has no `Close`, so its origin | ||
| ends whenever the collector reaches it. Say so in its doc comment and | ||
| `doc/lib/go`. Whether consumers should keep their producer alive, or the | ||
| wrapper should gain an explicit `Close`, is an API call: propose it to the | ||
| maintainer rather than ship it here. | ||
|
|
||
| Public API: none unless the maintainer picks an API change. Wire: none. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| # [S] NVIDIA tests run on real hardware | ||
|
|
||
| ## Goal | ||
|
|
||
| The NVDEC, NVENC, and CUDA tests run nightly on the maintainer's Linux host | ||
| (RTX 3070 Ti) instead of passing without a GPU on hosted runners, and a local | ||
| `just rs nvidia` runs them against the host driver instead of silently | ||
| skipping inside the Nix shell. | ||
|
|
||
| ## Plan | ||
|
|
||
| - Why they skip: the driver libraries are loaded at runtime (`libcuda` by | ||
| cudarc, `libnvidia-encode` in `rs/moq-nvenc/src/safe/api.rs`, `libnvcuvid` | ||
| in `rs/moq-nvenc/src/cuvid.rs`), and the tests return early when they are | ||
| missing (`hw_available` in `rs/moq-video/src/decode/backend/nvdec.rs`). The | ||
| Nix shell's loader path lacks Ubuntu's `/usr/lib/x86_64-linux-gnu`. | ||
| - Select every test that needs the GPU, not only names containing `nvdec`, | ||
| `nvenc`, or `cuda`: `safe::session::tests::failed_submission_releases_the_session` | ||
| in `rs/moq-nvenc` needs hardware and matches none of them. Find them by | ||
| their driver probes (`hw_available`, `driver_libs_present`, `Api::get` and | ||
| friends in `moq-nvenc` and `moq-video`). Recommendation: follow the | ||
| existing `#[ignore = "requires ..."]` convention (as `frame/vulkan_test.rs` | ||
| does) and put them in `nvidia` test modules, so hosted CI reports them | ||
| ignored instead of passed and one filter, `--run-ignored only -E | ||
| 'test(/::nvidia::/)'`, selects them all without the other ignored hardware | ||
| tests (Android, D3D11, PipeWire). Inside that selection a missing GPU fails | ||
| the test instead of returning early. Keep the no-driver tests | ||
| (`missing_driver_errors_instead_of_panicking`) outside it. | ||
| - `just rs nvidia`: symlink only those three libraries (by soname) from | ||
| `/usr/lib/x86_64-linux-gnu` into a private directory, put that on | ||
| `LD_LIBRARY_PATH`, and run that selection. Fail when a library is missing | ||
| instead of skipping. `just rs vulkan-cuda` puts the whole host directory on | ||
| the path, which lets host libraries shadow the Nix ones; fold it into this | ||
| recipe, since its `vulkan_cuda_` tests are the same kind. Those also need | ||
| the Vulkan loader to find the host NVIDIA ICD: point it at the ICD manifest | ||
| and expose the driver libraries it names, or keep them in their own recipe. | ||
| - Nightly: a job in `.github/workflows/nightly.yml` runs `just rs nvidia` on | ||
| the self-hosted runner. A self-hosted runner on a public repository must | ||
| never run untrusted code: only `schedule` and `workflow_dispatch`, with the | ||
| job gated to `refs/heads/main`, never `pull_request`; a dedicated label only | ||
| this job selects; read-only `permissions`. Read GitHub's self-hosted runner | ||
| hardening guidance before wiring it. | ||
| - Share the runner with the io_uring one that #4132 plans | ||
| (`quest/m1/uring-runner.md` on the drain line, which wants a 6.12+ kernel on | ||
| the same host): one registration and one security posture, a label per | ||
| capability. Whichever quest lands second reuses the first's job shape. | ||
|
|
||
| Public API: none. Wire: none. | ||
|
|
||
| ## Required | ||
|
|
||
| - A self-hosted runner is registered for moq-dev/moq on the maintainer's host, with the NVIDIA driver | ||
|
|
||
| ## Related | ||
|
|
||
| - [Video hardware validation](/quest/m3/video-hardware.md) - hardware paths nothing runs yet | ||
| - [Runtime QA hosts](/quest/m2/runtime-qa-hosts.md) - on-demand jobs on hardware hosts, a broader contract than a nightly |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The plan explicitly excludes libmoq, but
rs/libmoq/src/api.rsexposes the equivalent JSON and binary update/append APIs, so C callers would be the only binding unable to supply capture times anddoc/lib/cwould remain stale. Include the C ABI, documentation, and tests in this quest or a required companion.AGENTS.md reference: AGENTS.md:L92-L96
Useful? React with 馃憤聽/ 馃憥.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Disagree. The maintainer settled the scope as moq-ffi and its wrappers, not libmoq. The generated C bindings questline (
quest/m1/c/README.md) deletes the hand-written libmoq ABI in favor of C generated from moq-ffi, so C picks this up with no libmoq work. Added that link under Related in e16fc6c.(Written by Claude Opus 5.5)