feat(net): announce namespaces unasked, with a SETUP opt-out - #2748
feat(net): announce namespaces unasked, with a SETUP opt-out#2748kixelated wants to merge 7 commits into
Conversation
|
You have reached your Codex usage limits for security reviews. Please try again later. |
|
Warning Review limit reached
Next review available in: 6 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
WalkthroughThe change adds the MoQ SOLICIT setup option and documents its negotiation rules. JavaScript and Rust setup exchanges now decode, encode, and propagate solicitation declarations. Peer state combines Cluster and SOLICIT data. Namespace publishers select unsolicited or solicited delivery, support inline and request-stream targets, reconcile updates and withdrawals, and avoid duplicate advertisements. Subscribers adjust prefix requests based on peer declarations. Tests cover setup encoding, solicitation behavior, cleanup, and exactly-once delivery. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches✨ Simplify code
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 |
On the IETF path we only advertised a namespace in response to a SUBSCRIBE_NAMESPACE. No third-party relay sends one to a publisher, so `moq import` against moxygen, imquic, moqx, or Cloudflare connected, negotiated a version, and then emitted no control messages at all. It only worked against moq-relay because we solicit every session ourselves. Announcing unprompted is what made announces solicited in the first place: unsolicited PUBLISH_NAMESPACE plus inline NAMESPACE meant a draft-16+ peer heard each namespace twice, and whichever arrived second replaced the source the first attached. The root cause of both is that moq-transport carries no statement of intent, so neither side can tell whether the peer will announce, ask, both, or neither. Announce and ask by default, and let the peer opt out with a new extension: the SOLICIT Setup Option declares what an endpoint requires to be solicited (0x1: advertisements must be asked for; 0x2: asking me returns nothing). Absent means no requirements, so a peer that has never heard of it keeps today's behavior. The declaration is derived from the session's own halves rather than configured, so a publish-only client automatically asks the relay to stop announcing at it. The peer's declaration also picks exactly one announce loop per session, which is what keeps the double-advertise dead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
defd926 to
ad3f541
Compare
|
You have reached your Codex usage limits for security reviews. Please try again later. |
![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: ad3f54154b
ℹ️ 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".
| # Acknowledgments | ||
| {:numbered="false"} | ||
|
|
||
| This document was drafted with the assistance of Claude, an AI assistant by Anthropic. |
There was a problem hiding this comment.
This draft is rendered into the documentation site, so the acknowledgment publishes an AI source marker in user-facing prose. Remove the attribution as required by the repository guidance.
AGENTS.md reference: AGENTS.md:L89-L89
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not changing this one. That acknowledgment is the existing convention across the drafts: moq-cluster, moq-broadcast, moq-timestamp and moq-probe all carry the identical line, so removing it here would leave one draft inconsistent with four siblings. The AGENTS.md rule covers code comments, doc comments and /doc pages; an IETF Acknowledgments section is document prose whose canonical home is the datatracker, not a source marker that rots. Happy to strip it from all five if @kixelated wants the rule read that broadly.
🤖 Addressed by Claude Code
Both announce loops registered their `changed` listener only after reconciling, so a broadcast published while an advertisement waited for the peer's RequestOk notified nobody. The namespace then stayed unadvertised (or a removed one stayed advertised) until some unrelated later mutation woke the loop. Subscribe before reconciling instead: a notification that lands during the round trip resolves the promise we already hold, so the next turn picks it up. Found by Codex review on #2748. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for security reviews. Please try again later. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be6de558b0
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
rs/moq-net/src/ietf/session.rs (1)
429-451: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract one helper for building
peer::Peerfrom decoded parameters.
accept_setupanddecode_peer_setupbuild the samepeer::Peerfromcluster::peer_from_setupandsolicit::from_setup. A future option added topeer::Peermust be wired into both sites. Add afn peer_from_params(params: &ietf::Parameters, version: Version) -> Result<peer::Peer, crate::DecodeError>and call it from both.♻️ Proposed refactor
- let declared = peer::Peer { - cluster: cluster::peer_from_setup(¶ms, version)?, - solicit: solicit::from_setup(¶ms, version)?, - }; + let declared = peer_from_params(¶ms, version)?; return Ok(PeerSetup { stream: reader, path, declared, }); } } +/// The Setup Options we act on, read out of already-decoded parameters. +fn peer_from_params(params: &ietf::Parameters, version: Version) -> Result<peer::Peer, crate::DecodeError> { + Ok(peer::Peer { + cluster: cluster::peer_from_setup(params, version)?, + solicit: solicit::from_setup(params, version)?, + }) +} + /// Parse the Setup Options we act on out of a raw SETUP parameter block. fn decode_peer_setup(parameters: bytes::Bytes, version: Version) -> Result<peer::Peer, crate::DecodeError> { let mut bytes = parameters; let params = ietf::Parameters::decode(&mut bytes, version)?; - - Ok(peer::Peer { - cluster: cluster::peer_from_setup(¶ms, version)?, - solicit: solicit::from_setup(¶ms, version)?, - }) + peer_from_params(¶ms, version) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rs/moq-net/src/ietf/session.rs` around lines 429 - 451, Add a shared peer_from_params helper accepting &ietf::Parameters and Version, constructing peer::Peer with the cluster and solicit values. Replace the duplicated construction in both accept_setup and decode_peer_setup with calls to this helper, preserving their existing error propagation and return behavior.js/net/src/ietf/publisher.ts (2)
63-67: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer an options object for the
Publisherconstructor.The constructor now takes three positional parameters, and
solicitis the second capability knob added to it.Ietf.Connectionalready uses an options object for the same data (js/net/src/ietf/connection.ts, lines 69-89). Switch toconstructor(options: { quic: WebTransport; session: Session; solicit: Solicit })so the next option does not change call-site ordering.As per coding guidelines: "Use an options/config object instead of positional parameters when an API could gain options later."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@js/net/src/ietf/publisher.ts` around lines 63 - 67, Update the Publisher constructor to accept a single options object containing quic, session, and solicit, then read those fields when initializing the instance. Update all Publisher call sites to pass the named options object while preserving existing values and behavior.Source: Coding guidelines
313-348: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared reconcile loop.
runSubscribeNamespace(lines 313-348) andrunPublishNamespaces(lines 388-420) run the same loop: register thechangedlistener, peek#broadcasts, diffupdatedagainstactive, advertise additions, withdraw removals, then await the next change. Only the path mapping and the extra race withstream.reader.closeddiffer. Both copies carry the sameTODO Make a better helper within Signals.Extract one private method that takes the path mapper, the advertise and withdraw callbacks, and an optional cancellation promise. A future fix then lands in one place.
Do you want me to open an issue to track the
TODO Make a better helper within Signalshelper?As per coding guidelines: "Refactor awkward internal shapes while changing code: ... extend or generalize existing primitives, and avoid duplicated fixes."
Also applies to: 388-420
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@js/net/src/ietf/publisher.ts` around lines 313 - 348, The reconciliation logic duplicated by runSubscribeNamespace and runPublishNamespaces should be extracted into one private method. Have it accept the path-mapping function, advertise and withdraw callbacks, and an optional cancellation promise, while preserving each caller’s path behavior and cancellation semantics; update both methods to delegate to it and retain the shared changed-listener handling and TODO in the helper.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@js/net/src/ietf/connection.ts`:
- Around line 111-112: Update the connection setup around the Subscriber
constructor to pass solicit.interest into Subscriber, then enforce that policy
in Subscriber.announced() by skipping stream creation and SUBSCRIBE_NAMESPACE
transmission when interest is enabled. Add a regression test covering the
announced flow with solicit.interest set to true.
In `@js/net/src/ietf/parameters.ts`:
- Around line 13-14: Replace the triple-slash comment immediately above the
public SetupOption.Solicit member with a JSDoc block comment, preserving its
existing description and reference to solicit.ts.
In `@js/net/src/ietf/publisher.test.ts`:
- Around line 57-59: Bound the second Stream.accept call in the
mid-advertisement broadcast test with a timeout race so it rejects or returns a
clear failure when no announcement arrives. Preserve the existing “broadcast
published mid-advertisement was never announced” diagnostic and continue
validating the accepted stream with readPublishNamespace.
In `@js/net/src/ietf/publisher.ts`:
- Around line 406-414: Update the namespace synchronization logic around
`#advertise` and runSubscribeNamespace so active records only paths whose publish
request was successfully retained in requests, rather than assigning the entire
updated set unconditionally. Preserve failed or request-ID-less advertisements
as inactive, allowing later route changes to retry them in both the current and
draft-14/15 subscription branches.
In `@rs/moq-net/src/ietf/session.rs`:
- Line 845: The test around announces_wait_for_a_subscribe_namespace has a stale
premise and lacks coverage for the solicited path. Rename it and update its
assertion message to verify that no advertisement is sent before the peer’s
SETUP when peer_declared is None, then add a companion test configuring
peer_declared with solicit.announce = true so SETUP settles and the solicited
advertisement behavior is exercised.
---
Nitpick comments:
In `@js/net/src/ietf/publisher.ts`:
- Around line 63-67: Update the Publisher constructor to accept a single options
object containing quic, session, and solicit, then read those fields when
initializing the instance. Update all Publisher call sites to pass the named
options object while preserving existing values and behavior.
- Around line 313-348: The reconciliation logic duplicated by
runSubscribeNamespace and runPublishNamespaces should be extracted into one
private method. Have it accept the path-mapping function, advertise and withdraw
callbacks, and an optional cancellation promise, while preserving each caller’s
path behavior and cancellation semantics; update both methods to delegate to it
and retain the shared changed-listener handling and TODO in the helper.
In `@rs/moq-net/src/ietf/session.rs`:
- Around line 429-451: Add a shared peer_from_params helper accepting
&ietf::Parameters and Version, constructing peer::Peer with the cluster and
solicit values. Replace the duplicated construction in both accept_setup and
decode_peer_setup with calls to this helper, preserving their existing error
propagation and return behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0078ad43-5afb-4835-8eae-b5f5bc339f1d
📒 Files selected for processing (21)
doc/concept/standard/interop.mddrafts/draft-lcurley-moq-solicit.mdjs/net/src/connection/accept.tsjs/net/src/connection/connect.tsjs/net/src/connection/handshake.tsjs/net/src/ietf/connection.tsjs/net/src/ietf/index.tsjs/net/src/ietf/parameters.tsjs/net/src/ietf/publisher.test.tsjs/net/src/ietf/publisher.tsjs/net/src/ietf/solicit.tsrs/moq-net/src/client.rsrs/moq-net/src/ietf/cluster.rsrs/moq-net/src/ietf/mod.rsrs/moq-net/src/ietf/parameters.rsrs/moq-net/src/ietf/peer.rsrs/moq-net/src/ietf/publisher.rsrs/moq-net/src/ietf/session.rsrs/moq-net/src/ietf/solicit.rsrs/moq-net/src/ietf/subscriber.rsrs/moq-net/src/server.rs
💤 Files with no reviewable changes (1)
- rs/moq-net/src/ietf/cluster.rs
… in tests A peer may refuse a PUBLISH_NAMESPACE and stay connected, but the JS loops recorded the namespace as advertised regardless. Nothing re-added it to the diff, so it stayed unadvertised for the rest of the session. Track what the peer actually holds instead, matching what `sync_namespace` already does on the Rust side. `announces_wait_for_a_subscribe_namespace` had gone vacuous: with no peer SETUP the announce loop parks, so the test proved only that, not the policy it named. Replaced with three cases over a shared helper, covering silence before the SETUP, silence toward a peer that requires solicitation, and the unsolicited announce toward a peer that declared nothing. Also honor the INTEREST flag in the JS subscriber, which the Rust side already did, and fold the two `peer::Peer` construction sites into one. Found by CodeRabbit review on #2748. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for security reviews. Please try again later. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 82ed294217
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
js/net/src/ietf/publisher.test.ts (1)
12-19: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueName the test timing budgets.
Use named constants for each timeout or scheduling budget. This makes the purpose of each value clear and prevents unreviewed timing drift.
js/net/src/ietf/publisher.test.ts#L12-L19: define and use a named stream wait timeout.js/net/src/ietf/publisher.test.ts#L81-L84: define and use a named scheduling delay, or remove the delay if it is not required.js/net/src/ietf/subscriber.test.ts#L9-L16: define and use a named stream wait timeout.As per coding guidelines, "Avoid using magic numbers; use named constants instead."
(Written by CodeRabbit)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@js/net/src/ietf/publisher.test.ts` around lines 12 - 19, Replace the magic stream-wait timeout in nextStream with a named constant and use it in the setTimeout call. In js/net/src/ietf/publisher.test.ts lines 81-84, name and use the scheduling delay, or remove the delay if unnecessary; in js/net/src/ietf/subscriber.test.ts lines 9-16, likewise define and use a named stream-wait timeout. Keep each timing budget’s existing behavior unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rs/moq-net/src/ietf/session.rs`:
- Around line 854-855: Update the announce_occurrences tests around the fixed 50
ms sleep to pause Tokio time, wait explicitly until the driver is ready or the
expected announcement is observable, and advance simulated time only when
needed. Ensure zero-occurrence cases still exercise their branch and positive
cases reliably observe the announcement without relying on wall-clock timing.
---
Nitpick comments:
In `@js/net/src/ietf/publisher.test.ts`:
- Around line 12-19: Replace the magic stream-wait timeout in nextStream with a
named constant and use it in the setTimeout call. In
js/net/src/ietf/publisher.test.ts lines 81-84, name and use the scheduling
delay, or remove the delay if unnecessary; in js/net/src/ietf/subscriber.test.ts
lines 9-16, likewise define and use a named stream-wait timeout. Keep each
timing budget’s existing behavior unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1bfc6b34-6956-4576-904d-0a296d8dfc8f
📒 Files selected for processing (6)
js/net/src/ietf/connection.tsjs/net/src/ietf/publisher.test.tsjs/net/src/ietf/publisher.tsjs/net/src/ietf/subscriber.test.tsjs/net/src/ietf/subscriber.tsrs/moq-net/src/ietf/session.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- js/net/src/ietf/connection.ts
- js/net/src/ietf/publisher.ts
The flag is advisory: a peer that said it advertises nothing may still send an unsolicited PUBLISH_NAMESPACE, and the draft requires handling one exactly as without the declaration. Closing the feed made us deaf to it. Skip only the outgoing SUBSCRIBE_NAMESPACE and leave the feed live. Nothing then ends that feed, since its stream is what normally does, so add `Subscriber.close()` and call it from `Connection.close()`. Also make the session announce tests deterministic. A fixed 50ms sleep let a loaded machine miss the announcement, so the silent cases could pass without exercising anything; they now pause time and drive a bounded readiness loop. Found by Codex and CodeRabbit review on #2748. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for security reviews. Please try again later. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8d999af9c1
ℹ️ 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".
| let ns = Namespaces::new(peer, Target::Requests(None)); | ||
| self.run_namespaces(origin, crate::Path::empty().to_owned(), ns).await |
There was a problem hiding this comment.
When a modern peer grants fewer concurrent bidirectional streams than there are visible namespaces, this target opens one PUBLISH_NAMESPACE request per namespace and retains every accepted stream until withdrawal. Because run_namespaces awaits these opens serially, exhausting stream credit blocks the loop from consuming further origin updates or withdrawing removed namespaces, leaving the remainder unadvertised for the session. This is especially likely with browser peers whose concurrent stream limits are relatively small, so unsolicited discovery needs a strategy that cannot be stalled by persistent per-namespace streams. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed as a real consequence, and the mechanism you describe is right: run_namespaces awaits Stream::open inline, so a peer that runs out of bidi credit stalls the loop for origin updates and withdrawals alike, and our request streams are held until withdrawal so credit never frees on its own.
Not fixing it here, because it isn't a defect in this change so much as the cost of the policy the PR implements, and every fix is a design call for @kixelated rather than mine: cap or batch unsolicited advertisements, have a relay declare ANNOUNCE on links where it will solicit anyway (needs a public knob), or give moq-relay an operator flag to force the solicited path for peers that don't implement the extension. Worth noting the extension exists precisely so a peer that can't take N streams says so, but a peer that doesn't implement it can't, and the failure mode is silent. I've raised it alongside two related follow-ups; leaving this thread open so it doesn't get lost.
🤖 Addressed by Claude Code
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@js/net/src/ietf/subscriber.test.ts`:
- Around line 59-60: Update the test assertion for the value returned by
announced.next() to verify the announcement is active, not only that its path
equals Path.from("surprise"). Add an assertion that next?.active is true while
preserving the existing path assertion.
- Around line 12-20: Update nextStream to use a cancellable Stream.accept wait:
when the timeout wins, cancel or release the underlying
incomingBidirectionalStreams reader so no accept operation remains pending or
consumes a later stream. Clear the timeout when either the stream or timeout
branch completes, while preserving the existing Stream | undefined result and
STREAM_WAIT behavior.
- Around line 62-63: Update the test around subscriber.close() to await the next
feed result from announced and assert the terminal value specified by
announce.Consumer.next(). Ensure the test verifies that closing the subscriber
terminates the feed rather than only invoking close().
- Around line 53-57: Update the test around subscriber.runPublishNamespace to
retain its returned promise instead of discarding it, ensure the stream is
closed or aborted during cleanup, and await the handler promise so failures are
observed and no pending task remains.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 76bdaffd-f079-4b82-af29-3f27a8259a96
📒 Files selected for processing (5)
js/net/src/ietf/connection.tsjs/net/src/ietf/publisher.test.tsjs/net/src/ietf/subscriber.test.tsjs/net/src/ietf/subscriber.tsrs/moq-net/src/ietf/session.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- js/net/src/ietf/publisher.test.ts
- js/net/src/ietf/connection.ts
- js/net/src/ietf/subscriber.ts
- rs/moq-net/src/ietf/session.rs
… handler Racing `Stream.accept` left its read pending on the timeout path, holding the reader lock and possibly swallowing a later stream. Read the queue directly and release in a finally instead. The announcement test also discarded the handler promise, so a failure inside it went unobserved and the task stayed pending. Await it, and assert the withdrawal it emits on the way out plus the active flag on the announcement. Found by CodeRabbit review on #2748. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for security reviews. Please try again later. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69eabcfdf1
ℹ️ 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".
`#advertise` opened its request stream outside the try, so a peer out of bidirectional stream credit rejected there, unwound the announce loop, and hit the outer catch. That withdrew every advertisement and returned, leaving the connection up with no discovery and nothing to restart it, so every later publish went unannounced too. At debug level. A refused open is just a failed advertisement: keep it inside the try so it costs that namespace a turn and the next change retries it. Raise the outer catch to a warning, since reaching it means the session lost discovery. Found by Codex review on #2748. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for security reviews. Please try again later. |
An unsolicited advertisement holds its stream for as long as the namespace lives, so a peer's concurrent-stream limit caps how many we can have up at once. Past that the open blocks, and it blocks inside the loop that also processes unannounces, which are the only thing that frees a slot. That is a deadlock rather than a delay: nothing else retires our streams. Bound the open so a full peer costs that namespace a turn instead, and retry the deferred ones on a jittered backoff, since credit returning raises no signal the loop is watching. The JS loop had the same gap in weaker form: a refused advertisement was retried only when some unrelated broadcast changed. Two Rust tests cover it: a withdrawal reaching the wire while every open is blocked, and a namespace coming back on its own once credit returns. A third blocking point remains, the inline wait for the peer's reply, which argues for decoupling advertisement I/O from reconciliation entirely. Found by Codex adversarial review on #2748. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
moq/rs/moq-net/src/ietf/publisher.rs
Lines 1052 to 1055 in a352794
Honor retry intervals for rejected advertisements
When a peer rejects PUBLISH_NAMESPACE with REQUEST_ERROR, this branch decodes but discards msg.retry_interval; sync_namespace then marks the namespace deferred and the shared retry loop reissues it after its own 100 ms to 5 s schedule. A peer requesting a longer interval, such as during overload, is therefore retried too early and indefinitely for every namespace, consuming streams and request IDs despite the protocol's explicit backpressure signal. Preserve the decoded interval and use it to decide whether and when to retry. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L102-L104
ℹ️ 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".
| if (this.#solicit.interest) { | ||
| // No SUBSCRIBE_NAMESPACE to run, so nothing here ends the feed; {@link close} | ||
| // does it when the session goes away. | ||
| return announced.consume(); |
There was a problem hiding this comment.
When the peer declares INTEREST and a caller closes the consumer returned by announced(), this early return bypasses the normal finally cleanup, leaving the closed producer in #announcedConsumers until the entire session closes. If the peer later sends the advisory PUBLISH_NAMESPACE that this path is explicitly intended to tolerate, runPublishNamespace calls append() on that closed producer, throws after acknowledging the advertisement, and immediately removes the namespace, potentially preventing other live feeds from observing it; repeated calls also retain closed feeds. Register cleanup on announced.closed before returning from this branch. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
1 participant
Fixes #2730.
The problem
On the IETF path we only advertised a namespace in response to a
SUBSCRIBE_NAMESPACE. No third-party relay sends one to a publisher, somoq importagainst moxygen, imquic, moqx, or Cloudflare connects, negotiates a version, and then emits no control messages at all for the life of the process. It only worked againstmoq-relaybecause we solicit every session ourselves.The obvious fix, announcing unprompted, is what made announces solicited in the first place: unsolicited
PUBLISH_NAMESPACEplus inlineNAMESPACEmeant a draft-16+ peer heard each namespace twice, and whichever arrived second replaced the source the first attached. It also annoys peers that only publish, since they get told about every namespace we know.The root cause of both is that moq-transport carries no statement of intent. Neither side can tell whether the peer will announce, ask, both, or neither, so every implementation guesses, and both guesses are wrong somewhere. See moq-wg/moq-transport#1854.
The fix
Announce and ask by default, and let the peer opt out.
A new extension, MoQ Solicit (
drafts/draft-lcurley-moq-solicit.md), adds oneSETUPoption where an endpoint declares its solicitation requirements:0x1ANNOUNCESUBSCRIBE_NAMESPACEfor what I want.0x2INTERESTAbsent is identical to 0, so a peer that has never heard of the extension keeps exactly today's behavior, and a declaration only ever asks for less. Both flags are advisory: receiving a message you asked to be spared is handled normally, never fatal. Declaring
ANNOUNCEgets you the relay behavior the IETF draft describes, without anyone having to configure what the peer is.Deliberately not a role parameter, which the WG removed. It says what an endpoint expects delivered, not what it is, and the same code declares different things on different sessions.
Our own declaration is derived, never configured.
solicit::from_originsmaps a session half we don't have to a flag: no subscribe half means an announcement is useless to us, no publish half means we have none to give. SoClient::publish(origin)automatically tells the relay "ask me first", which is the interop complaint that motivated the solicited-only behavior, andClient::consume(origin)tells it "don't bother asking". A relay has both halves and declares nothing.The double-advertise stays dead because the peer's declaration picks exactly one announce loop per session, not per namespace:
SUBSCRIBE_NAMESPACEstream carries the advertisements, as today.run_publish_namespacescarries them, and aSUBSCRIBE_NAMESPACEis answered with OK over an empty origin, so it holds the stream open and advertises nothing.Both loops now share one
run_namespacesimplementation, differing only in aTarget(inlineNAMESPACEon a subscription stream, or aPUBLISH_NAMESPACErequest per namespace) and where the origin is rooted.Unlike MoQ Cluster, the option rides every draft we speak: draft-14/15/16 exchange Setup Options too, so an old peer can opt out as well.
ietf/peer.rsnow owns the shared SETUP slot (peer::Peer { cluster, solicit }), moved out ofcluster.rssince it carries both extensions.The subscriber honors
INTERESTwithout blocking on the peer's SETUP: asking costs one stream and one empty answer, while waiting costs a round trip on every connection. A server has already read the client's SETUP by then, which is the direction where the pointless question actually gets asked.Tests
a_peer_that_declared_nothing_is_told_unsolicited: the IETF path: publisher withholds PUBLISH_NAMESPACE until asked, so it never publishes to third-party relays #2730 scenario, an announce with no subscription in sight.each_namespace_is_advertised_exactly_once: both loops live, both declarations, asserting one wire message either way and which mechanism carried it. Mutating the check so both paths advertise fails it at 2 occurrences.a_peer_that_advertises_nothing_is_not_asked,declaration_follows_the_wired_halves, plus round-trip and forward-compat coverage for the option itself.just check, the Rust suite (914 tests across moq-net and moq-relay),bun test,just drafts check, andjust test smokeall pass.Cross-package sync
js/netmirrors it:solicitFromSetupthreaded from both handshakes (exchangeSetupnow returns the peer's declaration alongside the control stream), and the publisher runs one of the two loops. The browser declares nothing, since aConnectioncan publish and subscribe at any time.doc/concept/standard/interop.mddocumented the old solicited-only rule and now documents this one.(Written by Opus 5)