feat(net)!: resume subscriptions across routes sharing a first hop - #3312
Conversation
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. |
![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: 22777b41d3
ℹ️ 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".
| match serve.closed { | ||
| // Retracted between the table read and its lock: the table has | ||
| // already moved on, so just re-decide. | ||
| true => continue 'run, |
There was a problem hiding this comment.
When a RouteServer is dropped while its AnnounceProducer remains alive, which RouteServer explicitly supports, the route stays in the table but ServeState::closed remains true. This branch immediately re-enters the loop, best_route selects the same entry again, and no mutation or await occurs, so the origin driver spins indefinitely and prevents all other origin work from progressing. Mark this route refused or exclude closed servers from selection instead of retrying it unconditionally.
AGENTS.md reference: AGENTS.md:L142-L146
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 98bcf2d: a closed server now marks the route refused (retained while the entry stands), with a regression test that a dropped RouteServer under a live announcement resolves Unroutable instead of spinning. (Written by Fable 5)
| origin: producer.info(), | ||
| tasks: producer.tasks.clone(), | ||
| timers: producer.timers.clone(), |
There was a problem hiding this comment.
Every Consumer now clones the strong Tasks submission handle, whose Clone increments Submissions::senders; consequently TaskSet::poll cannot finish while any consumer clone exists. A caller that retains a read handle, drops every Producer, and awaits Driver::run now waits forever instead of observing the documented completion after producer handles and submitted work drain. Use a non-owning submission handle or change the public lifecycle contract and documentation consistently.
AGENTS.md reference: AGENTS.md:L124-L126
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 98bcf2d: Consumer (and the front watcher) now hold a non-owning TasksWeak, so the documented Driver completion holds; regression test drops every producer with a consumer clone live and awaits Run. (Written by Fable 5)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98bcf2d066
ℹ️ 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".
| } | ||
| refused.retain(|id| table.routes.iter().any(|entry| entry.id == *id)); | ||
| let best = match front.identity.routable() { | ||
| true => table.best_route(&path.as_path(), exclude, front.identity.pin(), &refused), |
There was a problem hiding this comment.
This origin path is shared by both lite and IETF subscribers, so pinning failover to the first hop here also splices IETF subscriptions. drafts/draft-lcurley-moq-cluster.md:219-220 still states that advertisements carry no content identity and that receivers MUST NOT splice across sessions. In an IETF deployment this can join streams that the negotiated protocol explicitly treats as unrelated, potentially mixing content. Keep this behavior lite-only or update the cluster protocol and compatibility story in the same change. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L203-L203
Useful? React with 👍 / 👎.
|
|
||
| match step { | ||
| Step::Serve(name, resume) => { | ||
| tasks.push(serve_track(front.state.clone(), name, resume, timers.clone())); |
There was a problem hiding this comment.
When the last Producer is dropped after a remote front has attached, but its AnnounceProducer, route server, and source remain live, run_remote_front remains active while Submissions::senders becomes zero. The new TasksWeak::push gate then silently discards this serve_track submission, so any track first requested afterward hangs indefinitely even though the driver and source are still running. Give an active front a child-task facility that remains usable until that front exits without making a lingering read handle own the driver's lifetime. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
| true => { | ||
| refused.insert(route); | ||
| last_err = Some(Error::Unroutable); | ||
| continue 'run; |
There was a problem hiding this comment.
When a RouteServer is dropped while its announcement remains and the origin has a live Dynamic fallback, request_broadcast still creates a remote front because the route retains server: Some(...). This branch avoids the previously reported spin by refusing that route, but the next pass ends the front with Unroutable; control never reaches the fallback queue that handled this case before the change. Exclude closed servers before committing to a remote front or forward an unresolved initial request to the fallback handler. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
A subscription whose serving route dies now resumes through the next best covering route when both routes name the same non-zero first hop, re-splicing at a group boundary. Different or unknown (0) first hops still end the subscription, so one publisher's subscribers are never spliced onto another's frames and two anonymous peers never pass for one reconnecting. This restores the seamless failover that #3225 removed, keyed on the routing identity that stayed on the wire. request_broadcast now resolves a routed path to a per-(path, exclusion) front: a spliced broadcast whose watcher materializes it from the best covering route through the existing per-route request queues, re-selects when the table changes (a drain reprice migrates before the session dies; a metadata-only reprice changes nothing), and retries a retracted route's failure through the survivor. Fronts are keyed per split-horizon exclusion so a failover can never adopt a route flowing back through one of its own readers. The spliced-track machinery (resume.rs, serve_track) is reused unchanged. The draft's no-splice paragraph becomes the first-hop rule, recorded in the moq-lite-06 changelog. The goaway cluster and route migration tests return to their pre-#3225 seamless expectations. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two Codex review findings on the front watcher: A RouteServer dropped while its announcement stands leaves the entry in the table with a closed queue; the watcher re-picked it and spun the origin driver on one core. A closed server now marks the route refused, retained for as long as the entry stands. Consumers cloned the owning Tasks handle, so a lingering read handle kept TaskSet::poll (and the documented Driver completion) from ever resolving after the producers dropped. Read handles now carry a non-owning TasksWeak that submits while the driver lives but never extends it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
98bcf2d to
5bc765e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5bc765efe9
ℹ️ 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".
| // request. The watcher materializes the path from the best covering | ||
| // route, resolves the channel, and re-splices the front through | ||
| // routes sharing its first hop for as long as one serves. | ||
| let broadcast = broadcast::Producer::new_spliced(broadcast::Info { |
There was a problem hiding this comment.
When a client requests many distinct nonexistent track names from a remotely routed broadcast, creating it as a spliced front makes every lookup allocate a resume::Producer in SplicedState::tracks. After serve_track aborts a refused track, broadcast::Consumer::track_inner removes that entry only if the same name is requested again, so one-off names remain for the lifetime of the route-backed front. An authorized viewer can therefore grow relay memory without bound by scanning unique track names; prune aborted or unused logical tracks instead of retaining them indefinitely. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
1 participant
What
Implements the Route resume identity quest (#3274), per Luke's decision: routes keep carrying the hops/cost of what they serve, and a relay resumes/stitches a broadcast across routes when the first hop is the same. Epoch stays dead; the identity is the routing-layer first hop that never left the wire.
Root cause
#3225 made announcements prefix routes and deleted the cross-session front (
FrontState.publisher/same_identity), specifying abort-and-resubscribe into the draft. The first hop stayed on the wire, inRouteEntry, and inroute_order; what was lost was the per-path structure that read it. On dev,ServeStateis per-announcer andcreate_sourcebroadcasts never enter the tree, so nothing spanned routes and every reader saw the subscription end.Design
request_broadcaston a routed path now resolves to a per-(path, exclusion) front: a spliced broadcast (the machinery that survived #3225 for local sources) plus a watcher task that:ServeState), so session code is untouched;Identity::Publisher(first_hop); empty orHop::UNKNOWNchains areAnonymousand never resume, so two anonymous peers cannot pass for one reconnecting);Fronts are keyed per split-horizon exclusion, so a failover can never adopt a route flowing back through one of its own readers - the per-exclusion-front design from the July resumption review, with the taint/
ExclusionGuardmachinery staying dead.serve_track/resume.rsare reused unchanged.Resolution through a route is now watcher-driven (async), so the origin's inline tests pump the driver instead of asserting synchronous resolution.
Draft
The no-splice paragraph in
draft-lcurley-moq-lite.mdbecomes the first-hop rule (MAY splice across same non-zero first hops at a group boundary; MUST NOT otherwise), with a moq-lite-06 changelog bullet. Note #3278 edits adjacent text in the same section; whichever lands second has a trivial conflict.Tests
cluster_diamond_goaway_seamless_failoverandcluster_reconnects_on_empty_uri_goawayreturn to their pre-feat(net)!: announcements are prefix routes #3225 seamless expectations (same subscription rides the failover; nothing re-delivered).broadcast_route_migration(moq-tokio) updated from abort-and-resubscribe to the resume contract.Also
moq-transport-20to the four version choice lists in moq-tokio; the draft-20 support reached dev without them, soversion_choices_match_the_parserfails on every branch.just checkandjust testpass (3428/3428). The moq-cli completion tests (a_stage_broadcast_picks_the_catalog_to_read,the_catalog_format_on_the_line_is_honored) flaked once under full-suite load against their 500ms wall-clock budget and pass consistently otherwise, including the final full run; they exercise this path (a network-backed catalog read), so flagging for visibility.just drafts checkis red on dev until docs(drafts): drop the warm discount and adoption rank from moq-lite #3278's hang-draft fix lands; this branch may need a rebase once it does.quest/m0/route-resume.mdlives on the chore(quest): rescope routing quests after prefix routes #3274 branch; it should be dropped there (like lite-draft-routing was) once this merges, with rank.md/warm-advertise.md's Required entries updated.(Written by Fable 5)