Repository navigation
feat(net)!: own broadcast discovery during resolution - #6445
Conversation
Quest-Claim: 12931269bf9f0263
Keep one-instance requests scoped to synchronized discovery, follow pinned instances through gaps, and preserve explicit refusal provenance. Use the shared native resolver for mux references and stats while recording the remaining public API rollout. Co-Authored-By: GPT-6 <noreply@openai.com>
Resolve against main's renames (Path, requests(), Requests) and its MIGRATE.md-at-cut policy: drop per-PR MIGRATE.md edits, keep the mux binding's pinned routed_broadcast wait, and drop the JS synced-absence extension of Requesting.unroutable, which main removed. Sessions keep the private immediate lookup, including the new IETF resolve path. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…thout a route A pending plain request now settles `closed` with `Unroutable` once every attached session discovers and none still owes an initial set covering its path, matching Rust's `request_broadcast`. The check is slot-scoped: snapshot releases and session changes recheck only the requests under the affected prefix, in one microtask. A session releases its snapshot on a failed announce stream together with the downgrade, so a refusal never reads as synced absence. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
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 18 minutes. View limit details
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. Comment |
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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 384668ddb0
ℹ️ 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
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 384668d
- P2 — Keep blind answers out of announced handles sharing a slot (
js/net/src/origin.ts:1880–1885,1903–1905). With a non-discovering session, create bothrequest(path, { announced: true })and a plainrequest(path). The plain handle makesblind(slot)true, and#answerinstalls its response in the shared slot.route()then returnsslot.answer, andown()filters only by epoch, so the unpinned announced follower also becomes active despite no announcement. The same happens when it joins an already answered plain request. Enforce discovery eligibility per handle (while retaining shared transport work), and add a mixed plain/announced regression that actually delivers the blind answer; the new test currently only checks the slot's blind flag. - The independent quadratic discovery-settlement finding also applies; no duplicate inline comment added.
Direction: owning discovery in the request and separating peer lookup is sound; retaining refusal provenance avoids brittle generation heuristics. The behavioral checkpoint is a reasonable boundary before the naming rollout. The shared-slot policy leak and table-wide settlement work should be addressed without another public policy API.
Verification: static inspection of changed Rust/JS resolution, forwarding, bindings, callers and regression tests; I did not run tests or independently verify the reported CI results.
…llowers Synced-absence settlement walks the watch trie beneath each landed prefix instead of scanning every request slot, and a resolved handle drops its check. A follower sharing a slot with a plain request no longer takes the slot's blind answer. A new requests bench sweeps pending requests against initial sets landing at once. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Addressed the automated review in fdb3d53:
(Written by Claude Opus 5.5) |
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: fdb3d53
Compared the single follow-up commit against my prior review at 384668d (base unchanged).
- Fixed: the shared-slot blind-answer leak.
own()now excludes source-less answers for announced handles, and the regression delivers a blind answer while checking both existing and newly joining followers. - Fixed: the independent quadratic settlement finding. Settlement walks affected watch-trie subtrees with deduplicated nodes; resolved handles remove their absence checks. The added benchmark varies unrelated pending requests and landed snapshots and checks settlement isolation.
No new actionable bugs found in this update. Reusing the existing watch trie and filtering eligibility per handle are appropriately small fixes that preserve transport sharing without expanding the public API.
Verification: static review of the three changed JS files and surrounding ownership/settlement code. Tests and benchmarks were not run; reported timings were not independently verified.
|
Merge summary for fdb3d53:
(Written by Claude Opus 5.5) |
Problem
Requesting an unwatched path can return
Unroutablebefore a watch-driven peer discovers it. Epoch-pinned announcement followers also close across a gap instead of handing out their instance when it returns.Approach
request_broadcastown scoped discovery, report table absence after sync, and retry route-generation churn. The immediate table lookup becomes the privateLookupused by sessions; lite and IETF publishers answer peers through it, so serving a peer never starts discovery.Unroutableonce every session discovers and none still owes an initial set covering its path. That check runs when a snapshot lands or the session mix changes, coalesced into one microtask, and walks only the watch trie beneath the landed prefix, so settlement stays path-local. A follower sharing a slot with a plain request never takes the slot's blind answer. A session releases its snapshot on a failed announce stream together with the downgrade, so a refusal never reads as synced absence.is_queuedretry heuristic with the shared request. The mux binding keeps main's pinnedrouted_broadcastwait (moq-dev/moq#6254), now typed asRequesting.routed_broadcastroute-change retry policy for CLI, gateway, mux binding, relay HTTP, and benchmark consumers. Its removal belongs to the remaining rollout.Merge with main (2026-10-28)
Main moved 580 commits, so this round merged it and re-applied the change onto main's renames (
Path,requests()/Requests). Per the MIGRATE.md-at-cut policy (moq-dev/moq#6610), the per-package MIGRATE.md edits are dropped and their guidance moved into Impact. Main removed JSRequesting.unroutable(moq-dev/moq#6470), so this PR's synced-absence extension of that getter, its absence benchmark, and the tests reading it are gone; synced absence now settlesclosedinstead (decision 2). Main's new IETF resolve path (moq-dev/moq#6964) now uses the private lookup.Impact
request_broadcastandrouted_broadcastreturnPending<origin::Requesting>, replacing publicorigin::Routing.request_broadcastowns discovery of its path and failsUnroutableonly once discovery has synced without a route for the requested instance; drop any watch held only to make it resolve.Requesting::poll_okneeds a mutable receiver (keep the taskmutwhen polling; awaiting is unchanged).Requesting::is_queuedis removed: delete manualUnroutableretries, since the request watches route changes itself. A selected handler's explicit refusal is returned directly byrequest_broadcastandannounced_broadcast;routed_broadcastkeeps retrying refusals only after its covering routes change. Paths discovery cannot name fail withInvalidPath.@moq/net, behavior):Origin.Consumer.requestwatches its path from creation until close. Paths with wildcard characters are refused before registering.{ announced: true, epoch }follows that instance through gaps and returns it when announced again. Announced requests no longer fall back to blind answers on a session without discovery; use a plain request there. With a discovering session, a plain request waits for the path's announcement instead of taking a blind answer, and failsclosedwithUnroutableonce that discovery has synced with no route for it (or none for its pinned epoch). Handle that error where main left the request pending.request_broadcastin moq-ffi,moq_origin_requestin C, and every wrapper inherit owned discovery and refusal behavior; calling it straight after connecting no longer races the session's announcements. C ABI and UniFFI method signatures are unchanged; the Dart checksums change with the corrected docs.Alternatives
Inferring refusal from route identity or watch generation loses provenance when preparing an answer changes the table or an end/start is coalesced. A private marker at the existing refusal decision preserves both explicit refusal and genuine churn. Existing legacy route-wait behavior remains local to
routed_broadcastrather than adding another public policy or compatibility API.Decisions
The earlier prerequisite, finishing reference announcements (
Source::follow) before the rollout, has landed: moq-dev/moq#6254 and moq-dev/moq#6649 movedmoq play,moqsrc, and the mux follower ontomoq_mux::Source::follow. The quest'sRequiredlink is removed. The maintainer settled both remaining decisions on 2026-10-28:request/follownaming rollout, deletingrouted/routed_broadcast, and caller migrations. (The diff already crosses Rust, JS, and every binding, and the rollout breaks every wrapper's names, so it reviews better on its own.)quest/m1/api-audit/resolve.mdis rewritten to what remains.Requesting.unroutablegone, a plain JS request on a discovering session that found no route stayed pending with no signal.closedwithUnroutableonce scoped discovery syncs with no covering route, in this PR. (Matches Rust and the quest goal, and closes the silent wait the merge exposed.) Implemented, with a test that fails without it.Follow-ups
Continue the resolve quest: method naming and optional-epoch bindings, removing
routed/routed_broadcastand the route-levelFollow(remaining consumer: the moq-hls server'sRESOLVE_TIMEOUTresolve), and the WASM, CLI, SRT, transcode, bench, relay HTTP, e2ee, and demo callers it lists.Validation
Round of 2026-10-28 (head after merging main):
nix develop --command just ci origin/mainpassed (all modules), andnix develop --command just test interop --allpassed. The new JS testa pending plain request fails Unroutable once its discovery syncs without a routefails on the previous head. After the automated review, fdb3d53 fixed the follower blind-answer leak (the extendedonly a plain request can use a session without discoveryfails without it) and made settlement path-local; the newpending,landed,settle_ussection ofjs/net/bench/requests.tsdrops from 865 to 38 µs at 8,192 pending requests and one landing.just ci origin/mainpassed again on that head. Releasing the snapshot only while the announce stream is open fixes three integration tests and two origin tests that otherwise read a refused discovery stream as synced absence.Earlier rounds:
Before the fix, the unwatched native request reproduction failed with
Unroutableand the JS pinned follower failed across a gap. Coverage includes scoped discovery sync and release over lite-05/06/07-wip, route churn, pinned return, unsupported discovery and scopes, non-discovery peers, and explicit/dropped-Prepared refusal.On head after the merge:
nix develop --command just ci origin/main: passed (Rust 8,968 + 1,236 + 126 + others, Python 108, JS, C, C++, Go, Kotlin, Dart, quest).nix develop --command just test interop --all: passed.cargo clippy -p moq-net -p moq-mux -p moq-stats -p moq-ffi -p moq-c --all-targets --all-features -D warningsand a workspacecargo check --all-features: passed.+blind/-blindinterest, and the coalesced-wakeup forward test downgrades discovery so it still exercises blind answers.🤖 Generated with Claude Code
(Written by Claude Opus 5.5)