Repository navigation
refactor(hls)!: export background work runs on a kio Driver - #7324
Conversation
Quest-Claim: 94dabcd0e7cc3e47
Broadcaster::new returns a Driver alongside the broadcaster. Its kio::Tasks set runs the catalog watcher, each rendition's record watcher, and init builds, instead of tokio::spawn. Each owner holds a guard that ends its task when dropped. The catalog watcher is polled first in every pass, so a reconfigure retires its rendition before that rendition reads later records. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
changed the title
Broadcaster::new returns (Broadcaster, Driver) instead of an Arc, matching the other handles: clones share a guard whose drop ends the catalog watcher. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 20 minutes. View limit details
No actionable comments were generated in the recent review.
 |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 30.95% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title "refactor(hls)!: export background work runs on a kio Driver" matches the main change. The PR moves the export's catalog watcher, rendition record watchers, and init builds from tokio::spawn… Description check The description matches the changeset. It explains the move from tokio::spawn to kio::Tasks under a Driver, the breaking Broadcaster::new change to (Broadcaster, Driver), the owner guards that replace… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Title check
-
Explanation
The title "refactor(hls)!: export background work runs on a kio Driver" matches the main change. The PR moves the export's catalog watcher, rendition record watchers, and init builds from tokio::spawn to kio::Tasks owned by a new export::Driver. The
!marks the breaking Broadcaster::new signature change. The title is specific and concise.
Full details: Description check
-
Explanation
The description matches the changeset. It explains the move from tokio::spawn to kio::Tasks under a Driver, the breaking Broadcaster::new change to (Broadcaster, Driver), the owner guards that replace JoinHandles and abort calls, the test additions, and the related quest and spawn-audit doc updates. It is detailed and on-topic.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 30.95% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 42 functions across 10 files. (2 skipped: 2 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Finishing Touches
-
Simplify code
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf302afe98
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: bf302afe98a975363e639aad7f0f1ab1be455998 (base bdc47852ad381e46a9d91ba1372597fa97a34dfd).
No new actionable bugs found in the changed production code. The explicit Driver and cheap-clone handle are a coherent direction: task ownership follows broadcaster/rendition lifetimes, and polling the catalog first addresses reconfiguration ordering without rewriting the watcher bodies. The inbox is justified by synchronous callers starting init work; the API migration is documented and wired into the server/CLI.
Existing finding, not duplicated: the new lifecycle test at rs/moq-hls/src/export/mod.rs:1694–1696 should pause Tokio time before creating the origin, as already raised in this thread. It uses real sleeps/timeouts despite being in-memory.
Verification: static inspection of the diff and relevant ownership, watcher, init-build and kio task code only; I did not run tests, builds or throughput benchmarks.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Landing summary:
(Written by Claude Opus 5.5) |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 3f5e0532d7e986cfd5fdb221f8caf46f16016d2d, compared with my previous review at bf302afe98a975363e639aad7f0f1ab1be455998; base unchanged at bdc47852ad381e46a9d91ba1372597fa97a34dfd.
The previously referenced paused-time finding is fixed: both driver lifecycle tests now start with Tokio time paused (rs/moq-hls/src/export/mod.rs:1694,2491), before creating the origin. This is a focused, simpler fix that preserves their assertions; the required test-util feature is already enabled. No new actionable bugs found in this update. The production Driver design is unchanged from the prior review.
Verification: static inspection of the exact one-commit delta, test bodies and Cargo features; no tests or builds run by this review.
|
quest land removed this pull request from the queue: the batch check failed (exit 1). Compiling proc-macro2-diagnostics v0.10.1
Checking moq-sock v0.2.0 (/work/rs/moq-sock)
Checking fs-err v3.3.1
Checking p384 v0.14.0
Checking p256 v0.14.0
Checking arboard v3.7.0
Checking reqwest-middleware v0.5.2
Checking web-transport-trait v0.5.2
Checking axum v0.8.9
Checking moq-auth v0.2.1 (/work/rs/moq-auth)
Checking axum-server v0.8.0
Checking egui-winit v0.36.2
Checking http-cache-reqwest v1.0.0-alpha.9
Checking egui-wgpu v0.36.2
Compiling duplicate v2.0.1
Checking eframe v0.36.2
Checking egui_dock v0.21.1
Checking pipewire v0.10.1
Checking cpal v0.18.2
Checking moq-json v0.5.8 (/work/rs/moq-json)
Checking moq-loc v0.2.19 (/work/rs/moq-loc)
Checking qmux v0.7.0 (/work/rs/qmux)
Checking hang v0.21.11 (/work/rs/hang)
Checking moq-stats v0.2.12 (/work/rs/moq-stats)
Checking moq-tokio v0.19.23 (/work/rs/moq-tokio)
Checking moq-mux v0.10.11 (/work/rs/moq-mux)
Checking moq-relay v0.17.2 (/work/rs/moq-relay)
Checking moq-audio v0.1.10 (/work/rs/moq-audio)
Checking moq-scene v0.1.0 (/work/rs/moq-scene)
Checking moq-studio v0.1.0 (/work/rs/moq-studio)
Finished `dev` profile [unoptimized + debuginfo] target(s) in 24.71s
info: running `cargo clippy --locked --tests --all-features -- -D warnings` on moq-wasm (50/51)
Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.25s
info: running `cargo clippy --locked --tests --all-features -- -D warnings` on uniffi-bindgen (51/51)
Compiling getrandom v0.4.3
Compiling winnow v1.0.4
Compiling serde v1.0.229
Compiling uniffi_internal_macros v0.32.2 (https://github.com/kixelated/uniffi-rs?rev=88d835d#88d835d6)
Compiling libc v0.2.191
Checking indexmap v2.14.2
Compiling fs-err v3.3.1
Compiling askama_parser v0.16.1
Compiling basic-toml v0.1.10
Checking nom v7.1.3
Compiling scroll_derive v0.12.1
Compiling askama_derive v0.16.1
Checking smawk v0.3.3
Checking tempfile v3.27.0
Checking weedle2 v5.0.0 (https://github.com/kixelated/uniffi-rs?rev=88d835d#88d835d6)
Checking scroll v0.12.0
Checking textwrap v0.16.4
Checking serde_json v1.0.154
Checking uniffi_pipeline v0.32.2 (https://github.com/kixelated/uniffi-rs?rev=88d835d#88d835d6)
Compiling askama_macros v0.16.1
Compiling toml_parser v1.1.3+spec-1.1.0
Checking uniffi_meta v0.32.2 (https://github.com/kixelated/uniffi-rs?rev=88d835d#88d835d6)
Compiling toml_datetime v1.1.1+spec-1.1.0
Checking plain v0.2.3
Checking clap_builder v4.6.7
Checking cargo_metadata v0.23.1
Compiling toml v1.1.6+spec-1.1.0
Checking uniffi_udl v0.32.2 (https://github.com/kixelated/uniffi-rs?rev=88d835d#88d835d6)
Checking askama v0.16.1
Checking goblin v0.8.2
Checking uniffi_core v0.32.2 (https://github.com/kixelated/uniffi-rs?rev=88d835d#88d835d6)
Checking clap v4.6.7
Checking uniffi_bindgen v0.32.2 (https://github.com/kixelated/uniffi-rs?rev=88d835d#88d835d6)
Compiling uniffi_macros v0.32.2 (https://github.com/kixelated/uniffi-rs?rev=88d835d#88d835d6)
Checking uniffi v0.32.2 (https://github.com/kixelated/uniffi-rs?rev=88d835d#88d835d6)
Checking uniffi-bindgen v0.1.0 (/work/rs/uniffi-bindgen)
Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.81s
mbx[cache]: object cache: 1121 hits, 253 misses, 52 not looked up; 0 B downloaded, 0 B uploaded, 420.5 MiB stored locally
mbx[cache]: 12m 8s session; 20m 27s estimated compiler time avoided (summed across compilations)
mbx[cache]: 52 compilations had no usable prior inputs or matching prediction for a lookup
mbx[cache]: Cargo artifact reuse and CI cache archive transfers are not included above
mbx[savings]: 1121 compilations in, zero compiled. 20m 27s returned to circulation.
ci: check quest failed
ci: test rs failed
ci: seconds per module
1061 check rs
261 test rs (failed)
1564 check rs_shapes
242 check obs_compile
68 check flake
42 check markdown
4 check gh
120 test gh
0 check drill
4 check quest (failed)
2 check obs
1 check ts_paths
24 check shell
0 check large_files
1 check toml
0 check nix
1 check justfile
2886 wall time
error: recipe `ci` failed on line 104 with exit code 1
. |
|
Codex land fixer: -- Installing: /work/cpp/moq/build/prefix/lib/libmoq_ffi.a
-- Installing: /work/cpp/moq/build/prefix/lib/cmake/moq-cpp/moq-cpp-config.cmake
-- Installing: /work/cpp/moq/build/prefix/lib/cmake/moq-cpp/moq-cpp-config-version.cmake
-- Installing: /work/cpp/moq/build/prefix/lib/pkgconfig/moq-cpp.pc
ci: check shell started
ci: check ts_paths passed in 1s
==> check ts_paths
ci: check large_files started
ci: check large_files passed in 0s
==> check large_files
changed: base 38566f5a84cb07bb756aae43f6dc902f1abacec2
ci: check toml started
ci: check toml passed in 1s
==> check toml
ci: check nix started
ci: check nix passed in 0s
==> check nix
ci: check justfile started
ci: check justfile passed in 1s
==> check justfile
ci: check shell passed in 24s
==> check shell
ci: check rs passed in 1061s
ci: test rs started
ci: test rs failed in 261s
ci: check rs_shapes started
ci: check rs_shapes passed in 1564s
==> check rs
sh/rs/select.sh lint "$LIST"
rs: selecting the workspace
rs: selecting the workspace
python3 sh/rs/stats-docs.py
# The environment is process-wide, so nothing writes it. A text check, because the
# clock lint's test allowances also silence clippy's disallowed_methods.
git grep --untracked -nE '(^|[^[:alnum:]_])(set_var|remove_var)[[:space:]]*\(' -- 'rs/*.rs'; [ $? -eq 1 ] || { echo "error: a write to the process environment: pass the value down, or run a test in a child with test_env::Env" >&2; exit 1; }
cargo fmt --all --check
RUSTDOCFLAGS="-D warnings" bash sh/local.sh cargo doc --locked --no-deps "$@"
Blocking waiting for file lock on package cache
Blocking waiting for file lock on package cache
Blocking waiting for file lock on build directory
Compiling kio-macros v0.1.0 (/work/rs/kio/macros)
Compiling aws-lc-sys v0.46.0
Checking moq-pattern v0.1.2 (/work/rs/moq-pattern)
Checking moq-msf v0.5.1 (/work/rs/moq-msf)
Checking moq-sock v0.2.0 (/work/rs/moq-sock)
Compiling moq-video v0.1.10 (/work/rs/moq-video)
Checking moq-nvenc v0.1.2 (/work/rs/moq-nvenc)
Compiling moq-gst v0.4.12 (/work/rs/moq-gst)
Checking kio v0.6.2 (/work/rs/kio)
Compiling moq-c v0.7.0 (/work/rs/moq-c)
Checking sysinfo v0.39.6
Documenting kio-macros v0.1.0 (/work/rs/kio/macros)
Documenting bench-harness v0.0.1 (/work/rs/bench-harness)
Checking moq-clock v0.11.0 (/work/rs/moq-clock)
Documenting moq-clock v0.11.0 (/work/rs/moq-clock)
Documenting kio v0.6.2 (/work/rs/kio)
Checking moq-net v0.3.10 (/work/rs/moq-net)
Checking moq-json v0.5.8 (/work/rs/moq-json)
Checking moq-loc v0.2.19 (/work/rs/moq-loc)
Checking hang v0.21.11 (/work/rs/hang)
Checking moq-congestion v0.1.0 (/work/rs/moq-congestion)
Checking moq-stats v0.2.12 (/work/rs/moq-stats)
Checking moq-mux v0.10.11 (/work/rs/moq-mux)
Documenting moq-mux v0.10.11 (/work/rs/moq-mux)
Documenting hang v0.21.11 (/work/rs/hang)
Checking moq-audio v0.1.10 (/work/rs/moq-audio)
Documenting moq-srt v0.3.11 (/work/rs/moq-srt)
Checking moq-scene v0.1.0 (/work/rs/moq-scene)
Documenting moq-carla-bridge v0.1.0 (/work/demo/carla/bridge)
Documenting moq-transcode v0.1.10 (/work/rs/moq-transcode)
Documenting moq-scene v0.1.0 (/work/rs/moq-scene)
Documenting moq-audio v0.1.10 (/work/rs/moq-audio)
Documenting moq-video v0.1.10 (/work/rs/moq-video)
Documenting moq-stats v0.2.12 (/work/rs/moq-stats)
Documenting moq-loc v0.2.19 (/work/rs/moq-loc)
Documenting moq-json v0.5.8 (/work/rs/moq-json)
Documenting moq-shaper v0.0.1 (/work/rs/moq-shaper)
Documenting moq-net v0.3.10 (/work/rs/moq-net)
Documenting moq-congestion v0.1.0 (/work/rs/moq-congestion)
Documenting moq-nvenc v0.1.2 (/work/rs/moq-nvenc)
Documenting moq-sock v0.2.0 (/work/rs/moq-sock)
Documenting moq-msf v0.5.1 (/work/rs/moq-msf)
Documenting moq-pattern v0.1.2 (/work/rs/moq-pattern)
Documenting moq-v4l v0.1.0 (/work/rs/moq-v4l
thread 'codex-main' (74) panicked at /rustc/48a229ceaefd4985c50990b14116b6d856af0985/library/std/src/io/stdio.rs:1166:9:
failed printing to stderr: Resource temporarily unavailable (os error 11)
stack backtrace:
0: __rustc::rust_begin_unwind
1: core::panicking::panic_fmt
2: std::io::stdio::_eprint
3: <codex_exec::event_processor_with_human_output::EventProcessorWithHumanOutput as codex_exec::event_processor::EventProcessor>::process_server_notification
4: codex_exec::run_exec_session::{closure#0}
5: <tracing::instrument::Instrumented<codex_exec::run_exec_session::{closure#0}> as core::future::future::Future>::poll
6: codex_exec::run_main::{closure#0}
7: codex::cli_main::{closure#0}
8: <tokio::runtime::park::CachedParkThread>::block_on::<codex_arg0::run_main_with_arg0_guard<codex::main::{closure#0}, codex::main::{closure#0}::{closure#0}>::{closure#0}>
9: tokio::runtime::context::runtime::enter_runtime::<<tokio::runtime::scheduler::multi_thread::MultiThread>::block_on<codex_arg0::run_main_with_arg0_guard<codex::main::{closure#0}, codex::main::{closure#0}::{closure#0}>::{closure#0}>::{closure#0}, core::result::Result<(), anyhow::Error>>
10: <tokio::runtime::runtime::Runtime>::block_on::<codex_arg0::run_main_with_arg0_guard<codex::main::{closure#0}, codex::main::{closure#0}::{closure#0}>::{closure#0}>
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
quest: fixer agent exited 101
(written by Codex) |
|
quest land removed this pull request from the queue: Conflicts with the base: rs/moq-hls/src/export/archive_tests.rs, rs/moq-hls/src/export/mod.rs |
Finishes quest/m2/hls-export-kio.md: the export's catalog watcher, each rendition's record watcher, and the init builds no longer use
tokio::spawn. They now run askio::Taskschildren of aDriverthe export hands back.Summary
Broadcasteris a cheap-clone handle, like the other handles in the codebase: its clones share a guard whose drop, with the last clone, ends the catalog watcher and releases the rendition map.is_clonereplacesArc::ptr_eq.export::Driver(rs/moq-hls/src/export/driver.rs). It owns akio::Tasksset and an inboxkio::Queue, so a rendition can start an init build from any thread, including a synchronous render off the runtime.Owner, akio::Producer<()>). Dropping the guard ends the task on the next poll. These guards replace theJoinHandles andabort()calls, theRendition'sDrop, and thetokio::runtime::Handlethe rendition captured. As a result,Renditionno longer panics when built outside a runtime.Broadcasterkeeps its rendition's watcher running until the records end.kio::Tasksrotates its polling order, unlike tokio's FIFO run queue, so without this a rendition's watcher could read records published after a reconfigure before the catalog watcher retired it. Those records would then be listed under the old init. moq-cli'sa_reconfigured_rendition_continues_its_playlistcaught this.Serverandmoq hls --recordspawn each broadcaster's driver at their tokio edge. The server runs one task per broadcaster rather than per path follower, because a replaced instance keeps serving after the follower that started it moves on.Impact
Broadcaster::new(source, config)now returns(Broadcaster, Driver)instead ofArc<Broadcaster>. Migration: clone theBroadcasterinstead of anArc, compare withis_clone, and run the driver wherever you run tasks, for exampletokio::spawn(driver.run()), or poll it withDriver::poll(&kio::Waiter). Nothing is watched until it runs. moq-hls needs a breaking (minor) bump at the next cut.rs/moq-hls/MIGRATE.mddescribes the change.Decisions
Broadcaster::newreturns aDriverand spawns nothing. The maintainer rejectedArc<Broadcaster>, soBroadcasteris now a cheap-clone handle (internalArc) andnewreturns(Broadcaster, Driver), as other handles do (moq_net::origin::Producer::new).newspawn one root task that runs the driver.Choices made without asking
asyncand are boxed into the task set, the way moq-archive's consumer does it. Rewriting them as poll machines is out of scope.record_cursors_yield_renditions_and_segmentsnow accepts eitherRecordsFailedorCatalogFailedwhen the publisher drops. Both watchers see the close, and which error arrives first was only ever tokio's scheduling order. MIGRATE already tells callers to match both.Tests
driver::tests::tasks_end_with_their_owner,a_dropped_driver_drops_queued_tasksthe_driver_finishes_with_its_owners: after a reconfigure, dropping the broadcaster and both renditions finishes the driver while the publisher stays live, and a held rendition keeps it running.dropping_the_broadcaster_keeps_a_cursor_drainablenow also asserts the driver outlives the held rendition and finishes once the cursor drops.just checkpassed locally onbf302afe98, and so didcargo test -p moq-hls -p moq-cli --all-features. On3f5e0532d7(test attributes only),cargo nextest run -p moq-hls --all-featurespassed 217/217.Follow-ups
#[kio::machine]/ poll functions and drop thetokio::select!andtokio::sync::{watch, Mutex}they still use.Review
mechanical fix by codex / Codex:
Merged base, preserving the PR broadcaster driver API and base kio clock migration in test conflicts.(head eff60cd)quest review), againstorigin/main(1ea4b1a2c3).10544f2ca6had 1 finding, fixed (a dropped driver now drains its inbox,a_dropped_driver_drops_queued_tasks). Round 2 onc27e5e0be8had no findings.bf302afe98had no findings.the_driver_finishes_with_its_owners(thread). Fixed: that test anddropping_the_broadcaster_keeps_a_cursor_drainable, which this PR also extends, now run withstart_paused = true.3f5e0532d7e986cfd5fdb221f8caf46f16016d2dhad no findings.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code