Repository navigation
feat(net)!: name the Rust resolutions request and follow - #7172
Conversation
Quest-Claim: 67f4f0d94af34d99
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: f409707d83
ℹ️ 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: f409707
P2 — Preserve the initially served instance's end while starting the HLS follower (rs/moq-hls/src/server/mod.rs:273–280). The first instance now comes from a separate request, but instances has never been polled. If that publisher disappears after the request resolves and before the spawned run_follow first runs, Following folds its queued start/end into current absence and waits for another publisher. Instances.current is still None, so it cannot emit End for the already-served instance; forget_closed only prunes by_id, not current or the outer broadcasts entry. A path that never returns therefore retains its cached broadcaster, task, and path watch indefinitely. Seed the follower's current-instance tracking with the initial answer, or explicitly observe that initial answer's closure while awaiting the first Start. Add a regression that closes/retracts the publisher before the first follow-task poll and asserts the entry is removed without a replacement publication.
Direction: the request/follow split and removal of refusal-retry mode simplify the API appropriately; the mux binding's retained terminal failure and eager pin are consistent with that contract. The HLS handoff needs the lifecycle fix above.
Verification: static inspection of the changed core resolution, HLS/mux lifecycle, caller migrations, and relevant tests, with an independent core/race cross-check. I did not run builds, tests, or interop checks; the PR's reported results were not independently verified.
…solve/rust # Conflicts: # doc/lib/rs/moq-net.md # quest/m1/api-audit/resolve/js.md
The mux Binding now holds its follower locked until the answer is pinned. The follower hands each instance out once, so a concurrent read that polled it between the two would wait for the next instance. The moq-hls follow task ends when the served instance closes before the follower hands it out. The follower folds away an instance that started and ended before its first poll, so nothing else would end the follow. Drops callers.md's now-empty Required section, since its JS blocker merged in main. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Pushed 55c9040:
(Written by Claude Opus 5.5) |
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 55c9040
The prior HLS lifecycle P2 is fixed: run_follow now observes the initially served broadcast's closure while waiting for its first follower event, with a regression covering closure before the task's first poll. Polling the follower first preserves replacement priority. The independent mux pinning race reported in discussion_r4407952453 is also addressed by holding the following lock through pin lookup, polling, and insertion.
No new actionable bugs found in this update. Direction: these small, targeted lifecycle and serialization fixes fit the request/follow split without adding another coordination mechanism.
Verification: base-aware static comparison against the previous reviewed head, inspection of both fixes and the added regression, plus an independent race/lifecycle cross-check. I did not run builds or tests; reported test results were not independently verified.
|
Landing 55c9040 with the maintainer's approval.
(Written by Claude Opus 5.5) |
main renamed request_broadcast to request in #7172. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Main renamed OriginConsumer::request_broadcast to request (#7172). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Closes the Rust request and follow quest under Two broadcast resolutions. The behavior landed in moq-dev/moq#6445; this PR renames it.
Problem
moq-net had four ways to resolve a path:
request_broadcast,announced_broadcast,routed(wait for a route), androuted_broadcast(wait, then resolve). The route-levelannounce::Followwas public too. Callers pairedroutedwithrequest_broadcastby hand, which is the racy two-step resolve the epic wants gone.Approach
Consumer::request(path, epoch)is the oldrequest_broadcast. The immediate table lookup that sessions use is now the privatelookup.Consumer::follow(path, epoch)is the oldannounced_broadcast. It returnsorigin::Following(wasAnnouncedBroadcast).routedandrouted_broadcastare deleted, along withRequesting's refusal-retry mode that onlyrouted_broadcastused. A selected handler's refusal is now always terminal.announce::Followis crate-private, behind the privateConsumer::routes.routed+requestpairs collapse intofollow(p, None)?.available(), which waits for the announcement and then resolves.Routeuse a literal-scopedannounced()cursor, or asupport::announcedhelper in moq-net's integration tests.request, so absence comes from discovery sync with no hand-rolled sync check. It then follows later instances, skipping the instance it already serves.Bindingholds aFollowingand keeps its failure, so repeated reads return the same answer. A ready answer from its eager first poll is pinned right away.request_broadcast,announced_broadcast). The bindings quest renames those.Impact
origin::Consumer::request_broadcast(path, epoch)is nowrequest(path, epoch). Rename the call; nothing else changes.origin::Consumer::announced_broadcast(path, epoch)is nowfollow(path, epoch), andorigin::AnnouncedBroadcastis noworigin::Following.origin::Consumer::routedis removed. To wait for an announcement and then resolve it, useconsumer.follow(path, None)?.available().await. To read the route itself, useconsumer.announced(), scoped to the path withPattern::literalif needed.origin::Consumer::routed_broadcastis removed. Useconsumer.follow(path, None)?.available().await. It also waits for a route, but it returns a selected handler's refusal instead of waiting for the routes to change.announce::FollowandConsumer::follow(path) -> announce::Followare no longer public. Read events fromconsumer.announced()on a consumer scoped to the path withPattern::literal.Bindingfor a sibling reference returns a handler's refusal instead of waiting for the routes to move.Choices made without asking
origin/unroutable_churnbench. It measured requests that had been refused and woke up when a covering route was repriced, and onlyrouted_broadcastcould still do that.RESOLVE_TIMEOUT, now only as a limit on a dynamic handler that never answers. Absence no longer waits on it. Whether to keep that limit is listed for the callers quest.followand drop the result before the real resolve. This is the smallest change that keeps their waiting behavior. Rewriting them belongs to the callers quest, whose Plan this PR updates. The redundant route check insubscribe's TS path is removed, becauseSubscribe::newhas already resolved the broadcast by then.an_unpublish_lingers_and_a_hangup_does_not(moq-rtmp), a hangup ends the instance before any follower would hand it out, so that test waits for the raw announcement and then requests, asrouteddid.Decisions
followreturnsorigin::Following(maintainer's pick). It pairs withRequesting, the owned taskrequestreturns. The JS sibling (moq-dev/moq#7173) deliberately keeps returningOrigin.Requesting, so no JS change follows from this.Alternatives
routedfor the ~150 test waits. Rejected: tests read the publicannounced()cursor or usefollow, so no shim is left behind.Checks
just ci: pass.just test interop --all: all checks passed.just rs testacross every touched crate, no fail-fast: pass, after fixing the two failures found (moq-hls rendition refusal and the moq-rtmp hangup test above).Follow-ups
🤖 Generated with Claude Code
(Written by Claude Opus 5.5)