feat(net): solicit announces lazily, on first announce interest - #2762
feat(net): solicit announces lazily, on first announce interest#2762kixelated wants to merge 1 commit into
Conversation
![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
moq/rs/moq-net/src/lite/subscriber.rs
Lines 432 to 434 in 3090eb4
Keep Lite04 requests pending while announcements arrive
For Lite03/04, this marks the interest driver synced immediately after sending ANNOUNCE_REQUEST, before reading even one announcement. When a cold request_broadcast created the interest, its AnnounceWait can therefore observe every driver as settled and return Unroutable while the peer's existing announcement is still in flight, which makes requests for live broadcasts race on normal asynchronous transports. Keep the driver unsettled until the solicitation has had a meaningful opportunity to populate the table. (Written by GPT-5.6 Sol)
ℹ️ 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".
| // The requests are opening; nothing further marks a boundary, so | ||
| // pending requests stop waiting on this session now. | ||
| interest_driver.synced(); |
There was a problem hiding this comment.
When the first cold request raises interest on a moq-transport session, these per-prefix futures have only been inserted into FuturesUnordered; none has been polled, so no SUBSCRIBE_NAMESPACE stream has opened or sent a request when synced() wakes AnnounceWait. The request can consequently fall through to Unroutable before the peer has any chance to advertise an existing namespace. Report synchronization only after the per-prefix solicitations have reached an actual response or other defensible warm-state point; the modern-version block repeats the same ordering. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
For Lite03/04 the request being on the wire is the only boundary the protocol offers: there is no AnnounceInit (Lite01/02) and no ANNOUNCE_OK count (Lite05+), so any later sync point would be an arbitrary timer, and the race it leaves is the one those versions have always had for a request issued right after connect. The comment above the initial_remaining match documents exactly this. Versions with a real boundary promise more, and as of 4177419 that now includes moq-transport, which syncs on the per-prefix request acknowledgment.
(written by Fable 5)
|
|
||
| Path prefix matching and equality is done on a byte-by-byte basis. | ||
| There MAY be multiple Announce Streams, potentially containing overlapping prefixes, that get their own ANNOUNCE_OK + announcements. | ||
| A subscriber MAY open and close Announce Streams at any point during the session, e.g. opening one only once it has a consumer for the announcements and closing it when the last one leaves; closing the stream is how announce interest is withdrawn. |
There was a problem hiding this comment.
This sentence adds protocol semantics allowing announce interest to start and stop during a session, but the existing moq-lite-06 changelog has no corresponding entry. That leaves the draft's revision history unable to identify when the lifecycle rule was introduced; add a concise factual bullet under the in-progress version. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Done in 3527876: the in-progress moq-lite-06 changelog carries the bullet "Stated that a subscriber MAY open and close Announce Streams at any point in a session, and that closing one withdraws its announce interest."
(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: 2f568e3e34
ℹ️ 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".
| // initial set (there is no ANNOUNCE_OK counterpart), so this is the only | ||
| // boundary available: a cold `request_broadcast` still races the peer's | ||
| // NAMESPACE messages here, exactly as it did before announces went lazy. | ||
| interest_driver.synced(); |
There was a problem hiding this comment.
When the first cold request raises IETF announce interest, this wakes AnnounceWait even though each run_subscribe_namespace future is still only queued in FuturesUnordered; that function allocates the request ID and writes SUBSCRIBE_NAMESPACE, so no request bytes have been sent yet and request_broadcast can return Unroutable for a broadcast the peer would advertise. Fresh evidence relative to the earlier comment is that the streams are now opened eagerly, but the message send remains inside the unpolled futures. Poll the solicitations through the request write before calling synced(); the modern-version arm repeats this ordering.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4177419, along with a second bug the regression test surfaced: the solicitor (previously "interest driver") was attached inside the driver future rather than in start(), so a request racing the spawned driver saw no solicitor at all and settled Unroutable without soliciting. Now the solicitor attaches synchronously in start() (mirroring the lite subscriber), and run_subscribe_namespace releases a ConnectingProducer once the peer acknowledges the request (or it fails); the session reports synced only when every prefix has reported in, the same drop-based countdown connect() uses. A cold request still races the NAMESPACE messages behind the acknowledgment, since the protocol marks no end to the initial set and the peer sends the acknowledgment before its initial batch, but it can no longer settle before the peer has received the solicitation. The new request_waits_for_the_acknowledgment test fails without either half.
(written by Fable 5)
4177419 to
e4644a5
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e4644a5353
ℹ️ 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".
| // draining). | ||
| struct SolicitorEntry { | ||
| session: SessionId, | ||
| synced: bool, |
There was a problem hiding this comment.
When one session is allowed disjoint prefixes such as a and b, this single synced bit is set only after every prefix future completes in both the Lite and IETF drivers. If the a initial set completes without the requested broadcast but the peer never answers the independent b announce stream, request_broadcast("a/missing") remains pending forever even though b cannot answer that path. Track synchronization per prefix and have settled(path) consult only the covering prefix state. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L98-L98
Useful? React with 👍 / 👎.
Sessions open announce-interest streams (lite ANNOUNCE_PLEASE, IETF SUBSCRIBE_NAMESPACE) only once the origin has announce interest to serve, so publish-only and request-only endpoints stop paying announce traffic for every broadcast they could see. The announce-interest ledger lives in model/interest.rs: AnnounceConsumers register per-prefix demand (interest::Guard), each session's subscriber half attaches an interest::Solicitor that parks on needed() and reports synced() once its initial announce set lands. Demand is attributed by session (Option<interest::SessionId>, tagged via Consumer::on_behalf_of) so a peer's own solicitation is never reciprocated. request_broadcast on a cold table raises exact-path interest and settles only once every solicitor that could answer has synced. The relay opts out with origin::Producer::solicit() (an origin::Interest guard): a mesh wants every route up front. Solicitors attach synchronously (lite: Subscriber::new inside connect(); ietf: start() before the driver future) so a request racing the session setup still waits instead of settling on a cold table. The IETF session syncs once every SUBSCRIBE_NAMESPACE is acknowledged, the same drop-based countdown connect() uses (connecting.rs, promoted from lite/); lite 03/04 mark no boundary and sync when the request is on the wire. No wire change; the moq-lite draft gains a clarifying sentence plus a changelog bullet that announce interest may start and stop mid-session. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
e4644a5 to
f1857d0
Compare
|
Rebased onto (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: f1857d0020
ℹ️ 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".
| // than error (the session is still serving what it has); the interest is | ||
| // served by the next session after the reconnect. | ||
| if self.subscriber.going_away.is_set() { | ||
| self.subscriber.solicitor.synced(); |
There was a problem hiding this comment.
When the first announce interest arrives after the peer's GOAWAY but before the replacement session attaches, marking the draining solicitor synced makes AnnounceWait conclude that every covering session is settled and return Unroutable. That drops the request's interest before the replacement can solicit the broadcast, breaking requests during a graceful handover. The IETF path has the same outcome when run_subscribe_namespace returns GoingAway and drops its countdown, so draining sessions must not report this unsent initial set as synchronized. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L98-L98
Useful? React with 👍 / 👎.
1 participant
Implements the Rust half of #2708, PR 1 of the plan in the design comment.
Problem
A session wired to feed an origin opened announce-interest streams for every allowed prefix at attach time (lite: one ANNOUNCE_PLEASE per prefix; IETF: one SUBSCRIBE_NAMESPACE), whether or not anything ever read an announcement. A publish-only endpoint, or one that only consumes known paths, paid announce traffic and per-session announce state for every broadcast it could see.
Design
An announce-interest ledger, its own module at
model/interest.rs, the shared foundation for #2610'srequested_announce()and #2756. Two sides write to it:AnnounceConsumerregisters its absolute prefixes (aninterest::Guard, refcounted, released on drop). That coversannounced(),announced_broadcast, the wait insiderequest_broadcast, and the publisher half serving a peer's announce interest, which is what will let interest cascade hop by hop.interest::Solicitorat construction (so a request racingconnect()'s return still sees it), parks onSolicitor::needed()until demand it should serve exists, then solicits every allowed prefix for the rest of the session, reportingSolicitor::synced()once the initial announce set lands (or provably never will). A GOAWAY while parked parks for good instead of erroring; the reconnect's session serves the interest.Demand is attributed by session: interest is tagged with an
interest::SessionId(None= the application itself), and the publisher half tags its egress consumer viaConsumer::on_behalf_of(session_id), so a session's subscriber half never treats its own peer's solicitation as demand. Without this, a relay soliciting a publish-only client would make the client solicit right back, and the saving would evaporate.Sticky, not per-prefix: announce streams feed routes, so closing one would yank routes from under handed-out broadcasts. Once opened, streams stay for the session's lifetime. Per-prefix scoping and close-on-last-consumer need route-lifetime accounting and are follow-ups.
request_broadcastkeeps its connect-warm behavior without the warmth: on a cold table it raises exact-path interest (waking a parked solicitor), resolves the moment the announcement lands, and falls through to the dynamic queue /Unroutableonly once every attached solicitor that could answer for the path has delivered its initial set (Ledger::settled, scoped per path so a session authorized elsewhere can't hold a request hostage). With no attached session it resolves immediately, exactly as before. Settling ignores solicitors whose only qualifying interest is the requester's own session.The initial-set boundary per protocol: moq-lite 01/02 use ANNOUNCE_INIT and 05+ use ANNOUNCE_OK's count; lite 03/04 mark no end, so they sync once the request is on the wire and a cold request still races the announcements, exactly as those versions always have. moq-transport also marks no end, but each SUBSCRIBE_NAMESPACE is acknowledged, so the IETF session syncs once every prefix has its response (the same drop-based countdown
connect()uses,src/connecting.rs, promoted fromlite/): a cold request still races the NAMESPACE messages behind the acknowledgment, but can no longer settle before the peer has even received the solicitation.The relay opts out via the new
origin::Producer::solicit()guard (origin::Interest, standing demand, no event queue): a mesh needs every route up front, since redundant-route failover depends on alternatives being present before anything fails, and a cold mesh warms hop by hop, which per-request settling would race. This is what keeps the full relay test suite (diamond failover, republish, cross-version mesh) green. Making relays lazy end-to-end needs multi-hop warm-state propagation and is future work tracked on #2708/#2610.connect()gates on the initial announce set only when interest already exists at session start (a reconnect, or an origin consumed before connecting); otherwise it completes immediately.Wire
No format change. moq-lite already allows multiple Announce Streams with overlapping prefixes, and closing the stream is the existing interest cancellation; old peers interoperate untouched. The draft gains one clarifying sentence that announce interest may start and stop mid-session, plus a changelog bullet (
drafts/draft-lcurley-moq-lite.md).Tests
model/origin.rs): session-attributed demand (own-peer interest never reciprocates), request waits on a lazy solicitor and resolves on announce, settlesUnroutableon a synced/dropped solicitor, resolves immediately with no solicitors, own-session solicitors don't block settling, scoped/rooted handles register absolute prefixes.lite/subscriber.rs): no stream opens while nothing is interested; the first consumer opens exactly one ANNOUNCE_PLEASE per allowed prefix.ietf/session.rs): regression testrequest_waits_for_the_acknowledgment, which fails both without the synchronous solicitor attach instart()(the request settles on a cold table before the spawned driver runs) and without the acknowledgment sync boundary (the request settlesUnroutablebefore the solicitation reaches the wire).tests/lazy_announce.rs, lite-04 / lite-05 / moqt-19): a peer's solicitation is served but never reciprocated (deterministic via paused-time quiescence + the synchronous initial replay of a fresh consumer); a coldrequest_broadcasttriggers solicitation, resolves, and the data path works; a request for a missing path settlesUnroutableinstead of hanging.Cross-package sync
js/net: not in this PR; the JS forwarder only exists on feat(js/net)!: route publish and consume through Origins, share one connection per relay #2705, so the mirror stacks on that branch (feat(js/net): solicit announcements lazily, per interested prefix #2775, PR 2 of the plan). The vocabulary here matches itsinterest/ solicit naming.moq-ffi/wrappers: no surface change (solicit()is additive and not yet exposed); compile-checked.doc/conceptalready describesannounced()as demand-driven; no change needed.cluster_diamond_goaway_seamless_failoverflake, reproduces identically on basedev).Targets
dev: behaviorally this changes whenconnect()returns and when announce streams open, and it builds on the dev-state session code.(written by Fable 5)