Repository navigation
feat!: one Route handle owns its broadcast - #7806
Conversation
Quest-Claim: f9fb7474d21c4d61
There was a problem hiding this comment.
Important
Two narrow ordering gaps in the new retraction path and one JS/Rust parity break. Each fix is small, but the first can violate the documented "announcing nothing once its prefix route retracted" guarantee under concurrency.
Reviewed changes
Routeowns its broadcast (Rust): neworigin::Routehandle for exact and prefix routes,RouteOptions,Request::route, crate-privateforward/update;Publication,Prepared,Requests,Producer::{create_broadcast, publish, requests, requests_route, update_route}removed, and the old metadataRouterenamedRouteInfo.- JS parity:
Producer.route/routePrefix,class Route,Request.route, read-onlyRouteInfo; the removed exports and the non-exported wire view. - Bindings and callers: moq-ffi/moq-c route handles plus all wrappers, and every publishing crate migrated (hidden-until-ready for catalog fills, route handles held where a broadcast clone used to be).
- Docs/tests: MIGRATE entries across packages, doc updates, and tests rewritten to the new ownership semantics.
⚠️ retract drains the served route after releasing the table lock
AnnounceProducer::retract removes the prefix route's table entries under the table lock, then drop(shared) before setting server.closed and draining (rs/moq-net/src/model/origin.rs:3382-3398). A concurrent job Route::announce enters update_if, whose entries check covers only that job route's own entries, then runs admit, which takes the request out of the still-open ServeState (origin.rs:3213-3232, 6170-6176). If that lands in the window after the prefix entry is removed but before the drain, the job route advertises and answers a request its prefix route already retracted, and the drain afterwards finds nothing to refuse.
The withdraw path already does this correctly by draining under the table lock (origin.rs:3337-3339), which is why the sequential tests pass; this only shows under concurrency.
Technical details
# Retraction is not atomic with releasing pending requests
## Affected sites
- `rs/moq-net/src/model/origin.rs:3355-3399` — `AnnounceProducer::retract`: entries removed under `shared`, then `drop(shared)` at 3382, then `server.closed = true` / `drain_all` at 3387-3398.
- `rs/moq-net/src/model/origin.rs:2977-3010` — `Route::announce` job branch; admit closure takes from `request.home` at 2993-2998.
- `rs/moq-net/src/model/origin.rs:3213-3232` — `update_if`: only checks `self.entries`, then calls `admit()`.
- `rs/moq-net/src/model/origin.rs:6170-6176` — `ServeState::take` guards on `self.closed` only, not on the owning prefix route still being live.
- Same class: `update_if` installs the ingress guard after releasing the table lock (`3279-3280`) while `withdraw`/`retract` clear it before taking it (`3326`, `3356`).
## Required outcome
- Once a prefix route's table entries are gone, no request handed off from its `ServeState` (job `announce`, `serve`, or `reject`) may resolve or advertise.
- `Route::announce` must return `Err(Unroutable)` for a request whose prefix route was retracted, in every interleaving, matching `origin.rs:2973-2976` and `6245-6248`.
## Suggested approach
- In `retract`, set `closed` and collect `drain_all()` while still holding the table lock (the `server` lock is taken after the table lock here, same order as `update_if`'s `admit`), then write the refusals after `drop(shared)`. Move the ingress-guard clear under the lock too, or clear it after taking `shared`.⚠️ RouteState::ended releases the control lock before retracting
ended takes the job's pending request under control but drops that guard at the end of the statement, then calls announcement.retract (rs/moq-net/src/model/origin.rs:2936-2941). A concurrent Route::announce acquires control, sees request == None, and takes the non-job branch, which can still find the entries and re-advertise (origin.rs:2977-2982). That advertises a route whose broadcast already closed, which the docs say must fail Error::Closed.
Technical details
# Close hook is not serialized with announce
## Affected sites
- `rs/moq-net/src/model/origin.rs:2933-2942` — `RouteState::ended`.
- `rs/moq-net/src/model/origin.rs:2406-2411` — `broadcast::Producer::on_close` hook calls `ended`.
- `rs/moq-net/src/model/origin.rs:2977-2982` — `Route::announce` non-job branch.
## Required outcome
- Once the broadcast close hook starts, no `announce` may succeed on that route.
## Suggested approach
- Hold the `control` guard across `retract` in `ended`, so a racing `announce` blocks and then finds the entries gone (`Err(Closed)`). Alternatively check `broadcast.is_closed()` while holding `control` in `announce` and `change`.deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) | 𝕏
|
Fixed the Cause: Fix (commit Verified with the package's own check ( Task list (6/6 completed)
|
Resolves the conflicts with main's zero max-age default and datagram tracks, and migrates code main added since the last merge to the Route handle: socket endpoints, HLS contributions and their bench, the follow refusal tests, the stitched TS republish test, and the new range-fill, socket, fetch and interop clients. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Rust: retracting a prefix route now closes its request queue under the table lock that removes its entries, so a job route announcing concurrently either lands first or finds its request released, never answers one its prefix route already retracted. A route's broadcast closing retracts it under the route's control lock, so a racing announce cannot advertise after the close. The ingress announce guard is set and cleared under the table lock too. JS: a prefix route notices its origin closed, so announce() and setCost() throw "origin is closed" instead of silently succeeding, matching Rust and the exact route. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Closing the retracted prefix route's queue under the table lock woke its waiters there, where an inline executor can re-enter the table. Retraction closes the queue after the table unlocks again, and a job route's answer is admitted only while its prefix route is still in the table, which the retraction leaves first. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A route created visible announced before the binding attached the catalog track every FFI broadcast carries, so a reader finding it at once could ask for catalog.json and get NotFound. The bindings create the native route hidden, attach the catalog, then announce it when asked visible. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 01e7a80
No new actionable findings in the merge-resolution delta since ad72053, checked against base a14995f.
The HLS conflict resolutions preserve main's include names/URLs and the Route ownership lifecycle. The three newly added test setups now use the replacement API with the intended visibility and retained route handles. The earlier failed-admission and front-resolution ordering fixes remain intact. The single-Route direction still fits; no change of approach suggested.
Verification: GitHub-only static review of the integration changes, not a fresh audit of all 460 files. I did not run tests. The current-head Check workflow is in progress. The PR reports its local CI gate passed with Swift skipped for lack of a toolchain.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Merged
origin/main(a14995f) onto the route handle: main's include rename, per-track Sync timelines, lite-07 route-key proofs, the C pre-encoded simulcast API, andPeerSetup::setbecoming fallible now sit on the branch with itsRoute/RouteOptions/requestedrenames intact. - Ported main's new call sites to the route API:
widened_max_age's legacy live-edge test,copy_fill's new fill test, and the lite publisher's pending-subscribe test publish through a hidden route created before the tracks and announced once filled, matching the oldcreate_broadcast+announceorder, and hold that route for the test's lifetime. moq-hlsand@moq/watchtests follow main's contribution → include rename while keeping the branch'sroute.broadcast(),route.announce(),route.withdraw()androute_prefix()calls;unannouncebecamewithdrawandrequested_broadcastbecamerequested.- Re-exported
RouteKeyfrommoq_net::originfor main's lite-07 setup proof.
I read all nine conflict files against both parents. The auto-merged rs/moq-net/src/model/origin.rs keeps main's Config::route_key / OriginState::route_key plumbing intact (the field, the OriginState::new argument, and the Producer::config/route_key accessors), and no code call to the removed publishing API survives the merge. cargo check -p moq-net --all-features is clean.
deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) | 𝕏
|
Fixed the failing Diagnosis: Confirmed deterministically: dropping the route and waiting 200 ms reproduces the exact assertion failure, while holding the route passes 15/15 runs. Fix:
Verification: Note: the branch was briefly locked in the merge queue, so the fix was first proposed as #7989; that landed on the PR branch and #7806 now points at
|
The container-importer supervision test dropped the route it created as a temporary, leaving only broadcast handles. Under the new ownership the route is the origin's owner, so the driver could observe every owner gone and finish cleanly before the injected panic, making the test race the driver and fail under load. Hold the route until the failure is recorded, then drop it before the importer assertions. Also spell out that the route, not the broadcast, keeps the driver running.
|
quest land watch: the merge queue removed this pull request at 01e7a80 because it conflicts with The land fixer works on it in the background, and the watcher re-enqueues it at the back of the queue once it is fixed or judged flaky, or hands it back here with what it needs. (written by quest land watch) |
Drop the muted-rendition hold test that #7942 removed, and keep this branch's deletion of the completed route/handle quest. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: da3f1b0
No new actionable findings in the two-file delta since 01e7a80.
Holding the route until completion.finished() records the injected failure prevents the origin driver from ending cleanly first. Dropping it before the importer assertions preserves the test's failure-retention coverage. The clarified lifetime documentation matches RouteState::_keepalive. This is consistent with the existing Route ownership direction; production behavior is unchanged.
Verification: GitHub-only static review of the patch, surrounding supervision tests and driver completion logic; I did not run tests. GitHub currently reports merge conflicts and the current-head Check runs are skipped, so this is not a verified CI pass.
|
codex / Codex land fixer note:
(written by codex / Codex) |
…ndle Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Merged
origin/mainat f3ddaee5b onto the route handle: main's #7975 quest audit and #7942 push-to-talk change (enabled: falsenow means "not selectable", so held disabled renditions are gone from moq-cli, moq-gst, moq-ffi, moq-c and the JS watch decoders) now sit on the branch with itsRoute/RouteOptions/requestednames intact. - Kept the branch's route migration through the merge: all 28 files both sides touched retain the branch's changes relative to main's parent — checked per file against both parents rather than by eye. The catalog-before-announce,
set_cost,withdrawandroute_prefix/requestmigrations all survive. - Took main's
moq-climedia test rewrite and re-applied the route migration to the surviving tests: main'sstop_disabledcoverage keeps the branch's route helpers, and the muted-rendition hold test the branch had migrated is deleted because #7942 removed that behaviour. - Merged #7989: the moq-ffi supervision test holds the
Routethrough the injected panic and drops it before the importer assertions, so the origin driver cannot finish clean before the failure is recorded; theRoutedoc now credits the route rather than a bareProducerwith keeping theDriverrunning, matchingRouteState::_keepalive. - Quest cleanup:
quest/m1/api-audit/route/handle.mdstays deleted against #7975's edit, androutes/session.mddrops the landed lite-07 wire quest link.
I re-checked the merge resolutions structurally: for each file touched by both main and the branch, the merge differs from main by exactly the branch's migration counts, and a tree-wide grep finds no surviving call to the removed publishing API. Holding the route in the FFI test is the right fix — the route, not the broadcast, is the origin driver's remaining owner there — and dropping it before the importer assertions preserves the failure-retention coverage the test is about.
deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) | 𝕏
#7806 replaced OriginProducer.create_broadcast with route(). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
#7806 replaced origin::Producer::publish with route(). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Implements One Route handle and deletes the quest. The
Hop/Hops/HopLimitprivatization, plus hiding JSRouteInfo'sepochandauth, is split into a new quest, Transit facts stay read-only. That work is mechanical and lands independently, and it would have added roughly 100 more call sites to an already large diff.On the publishing side, ownership now runs origin → route → broadcast. An application creates a
Routeon the origin, and the route creates and owns its broadcast.Publication,Prepared,Requests, the broadcast's own announcer andProducer::{create_broadcast, publish, requests}are gone. This covers Rust, JS, moq-ffi, moq-c and every wrapper.Changes
Rust (
moq-net)origin::Routeis the handle, for both exact and prefix routes. It isClone, and dropping the last clone retracts the route. Closing its broadcast also retracts it.broadcast(),instance(),announce(),withdraw(),is_announced(),set_cost(),set_auth(),requested()andpoll_requested().RouteOptionscovershidden(),with_cost,with_authandwith_instance(instance, epoch).Request::route(options)replacesprepare/Prepared::publish. It mints a fresh instance by default, as decided on moq-dev/moq#7770.announce().announce()failUnroutable, and that route can never announce.origin::Routeis noworigin::RouteInfo, with read-only getters:instance() -> &Instance,cost(),hops(),is_anonymous()andsource(). It has no public builders.Producer::forward(prefix, info, predecessor)andRoute::update(info)in place ofrequests_route/update_route.moq_net::fuzzgainsroute_info(hops, cost, instance)for benches and tests that need relay metadata.JS (
@moq/net)origin.route(path, options?),origin.routePrefix(prefix, options?),class RouteandRequest.route(options?).class Routehasbroadcast(throws on a prefix route),instance,announced,announce(),withdraw(),setCost(),setAuth(),requested(),close()and[Symbol.dispose].RouteOptionsis{ hidden, cost, auth, instance, epoch }.RouteInfois the read-only received metadata.createBroadcast,requests,requestsRoute,updateRoute,Publication,PreparedandRequests.RequestSlotandRouteEntry. The orphaned@internalblocks are deleted, and sessions reach routes through a non-exported wire view.@moq/publish's on/off toggle iswithdraw()/announce()on one hidden-created route, so viewers continue on the same instance.@moq/crypto's signer takes the route's instance ({ path, instance }) instead of minting its own.Bindings
MoqOriginProducer::{route, route_prefix}withMoqRouteOptions { hidden, cost, instance, epoch }, andMoqBroadcastRequest::route(options).MoqRoutehasbroadcast(),instance_id(),announce(),withdraw(),set_cost(), asyncrequested()andcancel().MoqRouteis nowMoqRouteInfo.moq_origin_route,moq_origin_route_prefix(takes the request callback),moq_route_{broadcast,instance,announce,withdraw,set_cost,free}andmoq_broadcast_request_route.moq_announce_routeis nowmoq_route_options(addshidden), andmoq_routeis nowmoq_route_info.Callers
ts/programs.rs, the moq-cli imports, and WHIP after SDP.IngestSink::routed), moq-cli transcode/hls/archive/rtc/rtsp/publish, rtmp, srt, the gst sink, the mux TS programs and the bench connections.Review fixes:
announce()can't advertise after the close (Rust).announce()andsetCost()throw "origin is closed", as Rust and the JS exact route do.catalog.json.Docs:
doc/concept/identity.md(lazy jobs),doc/lib/rs,doc/lib/js,doc/lib/{c,cpp,py,kt,swift,dart}, binding READMEs,doc/bin/obs.md, and MIGRATE entries for moq-net, @moq/net, moq-ffi, moq-c, c/moq, cpp/moq, moq-crypto and the wrappers.Impact
rs/moq-net/MIGRATE.mdandjs/net/MIGRATE.mdentries give the full mapping:publish(path, Route::default())→route(path, RouteOptions::default())create_broadcast+announce→route(path, RouteOptions::hidden())+announce()unannounce→withdrawset_costrequests(prefix)→route_prefix(prefix, ..)prepare/publish→request.route(..)/announce()Route..dashclaim moves toroute_prefix+request.route(RouteOptions::hidden())+announce(). A refused announce means the request was released, which replaces the prepare/publish workaround.route_prefix(prefix, RouteOptions::default().with_instance(forwarded, epoch))+request.serve(held), all public now. Only the hop chain stays crate-private.Choices made without asking
Routetype for exact and prefix routes, per the epic decision. On a prefix route,broadcast()panics in Rust, throws in JS and errors in the bindings. On an exact route,requested()yields nothing.instance_id(), notinstance(). uniffi-bindgen-cpp keeps every generated C/C++ object's handle in a private field namedinstance, so a method with that name does not compile. Kotlin renames it back toinstance(). The Python, Go and Swift wrappers exposeinstance. Only the generated C/C++ and Dart surfaces showinstance_id. Fixing the generator is a suggested follow-up.set_auth, as before.moq_origin_route_prefix, and keeps themoq_broadcast_request_*names to mirror moq-ffi'sMoqBroadcastRequest.MoqRoute, becauseRouteclashes with Flutter. Swift takes labeled arguments, per its binding conventions.moq-crypto'ssign::broadcast::Producer::new(inner, key, path, instance)takes the route's instance and mints none, mirroring JS's{ path, instance }.moq_mux::Lingerstill takes broadcasts. Its owner must hold the routes, which the doc now says. Taking routes would leave moq-rtc's WHEP ingest, which has no route, with nothing to pass.routePrefixon a closed origin returns an already-retracted route instead of throwing.set_cost/set_authon a hidden route keep the value for the next announce.standalone_broadcast_cannot_announceand moq-c'sadmission_must_match_the_broadcast_it_filledare deleted, because the API they guarded no longer exists.moq_net::fuzzforwarded routes. Two of them moved tors/moq-net-tests/tests/integration/hop_limit.rs.Checks
On head 01e7a80 (the merge of
origin/maina14995f):quest gate nix develop --command just ci origin/mainexited 0, and every module passed, includingcheck js,test js,test rs, py, c, cpp, kt, go and dart.check swiftskipped itself because this host has no Swift toolchain (swift check: no swift toolchain on PATH, skipping). The merge changes no Swift, Kotlin, Go, Python, Dart, C or C++ binding source relative to ad72053, so the Swift surface is still the one reviewed earlier, and it is still not compiled locally.Earlier runs:
On head ad72053 (the merge of
origin/mainf4bccd1 plus two fixes):quest gate just ci origin/mainon c26cd13: every module passed except three.check rsfailed only on rustfmt of the new front fix.check ktfailed on oneAnnounceEventEndthe merge left behind.test jstimed out once inthe PROBE stream sends the viewers' target to an Increase peer(5 s), which passes 3/3 run alone and touches no routes. ad72053 fixes the first two. On it,just rs check,just kt checkandjust js testpass. The Rust tests (test rs) passed in that run, and the run before it passed every other module (py, c, cpp, obs, go, dart selection, markdown, bench, flake, quest).cargo check -p moq-net(poll-only, noasync) andcargo checkof the out-of-workspacebench/fanout/sessionharness: clean.follow_hands_out_the_instance_again_after_an_endfailed 7/40 runs before the front fix below and passed 60/60 after it.Merges
The 2026-11-05 merge of
origin/main(8022bf6) collided with moq-dev/moq#7678 (announcements are a path listing; follow reports offline), moq-dev/moq#7607 (generated async wrappers, poll-machine sessions) and moq-dev/moq#7846 (split JS datagram tracks), across 66 files:RouteInfo,forward,update,poll_requested).Route::requestedis now generated by#[cfg_attr(feature = "async", kio::wrapper)]onpoll_requested, per feat!: generate async wrappers behind default-off async features #7607.routes(). Main's new tests (follow offline/refusal, the listing filter inannounced.rs, flights, moq-boy, moq-room, moq-cli echo/archive, the JS datagram tracks, the Go, Kotlin, Python, Swift and Dart follower tests) publish through routes.Updateevent, which apps no longer see. A fresh listing reads the new cost under the same instance instead. "A new route restarts its readers" tests now follow the path: the path stays listed, the follower is handed the new route's broadcast, and the listing next reportsAbsentonce both routes withdraw.Two fixes on top:
followexposed this throughfollow_hands_out_the_instance_again_after_an_end. The front's first resolution now applies a takeover that already landed, under the table lock, asselect()andFrontWatchalready did once a front had resolved.A second merge of
origin/main(a14995f), after the merge queue dropped the PR on a conflict:rs/moq-hls/benches/include.rsandrs/moq-hls/src/server/include_tests.rsconflicted with main's rename of contributions to include. They keep main'sincludenames and URLs, and this branch'sroute.broadcast(),route.announce(),route.withdraw()androute_prefix().widened_max_age.rs's legacy live-edge test, the copy-fill test injs/net/src/lite/copy_fill.test.ts(a visible route), anda_pending_subscribe_joins_the_newest_group_once_resolvedinrs/moq-net/src/lite/publisher.rs. This matches pullfrog's candidate onpullfrog/7806-merge-tests-fix, written independently here. A tree-wide grep finds no other use ofcreate_broadcast,createBroadcast,requested_broadcastorunannounceoutside docs, changelogs, MIGRATE entries and the smoke and compat clients that drive released packages.Suggested follow-ups
instance()everywhere.Review
Head 9319c47352 adds two merges onto the reviewed 01e7a80:
origin/mainafter feat!: push-to-talk suppresses silence instead of toggling the catalog #7942: keeps this branch's deletion of the completedroute/handlequest, and drops the muted-rendition hold test that feat!: push-to-talk suppresses silence instead of toggling the catalog #7942 removed.just ci origin/mainpasses on that merge.Routedoc. Read and kept;cargo test -p moq-ffi --lib(304) and clippy pass.Codex
gpt-6.1-sol, medium effort, viaquest review --base origin/main, session 01a1adb8, round 1: reviewed head 01e7a80 and reported no findings.Earlier rounds:
Codex
gpt-6.1-sol, medium effort, viaquest review --base origin/main, session 01a1aa3b, round 4: reviewed head ad72053 and reported no findings. Round 4 is the design checkpoint: the approach is unchanged, and the merge only adapted it to main's listing, so continuing as is.The automated review and pullfrog's shared finding on edbfb67 (a failed job admission released
controlbefore retracting) is fixed in c26cd13 and answered in this comment.Earlier rounds:
Codex
gpt-6.1-sol, medium effort, viaquest review --base origin/main, session 01a1aa3b. It reviewed head 08aa622 in round 3 and reported no findings. The pushed head adds only a conflict-free merge oforigin/main.Round 1 (head d61dbdc), fixed:
rs/moq-ffi/src/origin.rsMoqOriginProducer::route: a visible route announced before the binding attached its catalog track, so a reader finding it at once gotNotFoundforcatalog.json. The same held forMoqBroadcastRequest::routeand moq-c. Fixed in d03c388: both bindings create the route hidden, attach the catalog, then announce.Round 2 (head d03c388): no findings. It covered the reworked retraction fix (a0e908f).
Pullfrog's review of 05f1964 (review), all fixed:
AnnounceProducer::retractreleased the prefix route's requests after dropping the table lock, so a job route could announce in between. Fixed in a0e908f: a job answer is admitted only while its prefix route is still in the table.RouteState::endedreleased the control lock before retracting. Fixed in d61dbdc: it retracts under that lock.js/net/src/origin.tsinsert: a prefix route stayed "inserted" afterorigin.close(). Fixed in d61dbdc, with a test.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code