Repository navigation
fix(net): one namespace's stream error ends only its own advertisement - #6126
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A PUBLISH_NAMESPACE request stream that is reset or ended before its answer now withdraws just that namespace and re-offers it on its own growing backoff (100 ms doubling to 5 s), matching js/net. Only the control stream failing or a protocol violation, which closes the session at the source, still ends the announce loop. Previously the error propagated out of run_publish_namespaces and closed the whole session; a FIN before the answer was read as a protocol violation, which moq-tokio treats as terminal. Completes quest/m0/namespace-advert-error.md. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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. |
|
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 10 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 |
![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: fc3a26ac34
ℹ️ 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".
| Err(err) if !super::subscriber::is_protocol_violation(&err) => { | ||
| tracing::debug!(broadcast = %absolute, %err, "advertisement failed"); | ||
| return Ok(watch.lapsed(self.runtime.now())); |
There was a problem hiding this comment.
On draft 14/15, if the peer resets the request stream before the complete PUBLISH_NAMESPACE header is written, this branch now treats the write error as a recoverable advertisement lapse. The request ID was already incremented at line 3097, but the peer may not have received it; the retry or any unrelated request therefore sends the next ID, while Control::accept requires strictly sequential IDs for these drafts and closes the session with INVALID_REQUEST_ID. The new test resets only after reading the whole request, so it misses this case. As with start_joining_fetch, an ambiguous failed legacy header must close the session rather than continue with a permanently skipped ID.
AGENTS.md reference: rs/moq-net/AGENTS.md:L6-L6
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in bf87e8a. A header that fails to send on draft 14/15 now closes the session with INTERNAL_ERROR and ends the loop, matching start_joining_fetch; later drafts still back off that namespace. a_failed_legacy_header_closes_the_session stops the stream after the type and before the body (one-message stream buffer), and fails without the fix.
(Written by Claude Opus 5.5)
Drafts 14 and 15 require the next request ID, so a header that fails to send after its ID was allocated leaves a gap the peer refuses on every later request. Close the session there, as the joining FETCH does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Merge summary:
(Written by Claude Opus 5.5) |
Completes
quest/m0/namespace-advert-error.md.Summary
On moq-transport, a Rust publisher whose PUBLISH_NAMESPACE request stream failed before its first answer propagated the error out of
run_publish_namespaces, which the session races, so one reset stream closed the whole session. A FIN before the answer went throughMessages::message, which reads it as a protocol violation, so moq-tokio also stopped reconnecting.advertise_namespace(rs/moq-net/src/ietf/publisher.rs) now separates the two kinds of failure, matching#advertiseinjs/net/src/ietf/publisher.ts:Watched::lapsed, 100 ms doubling to 5 s). A peer that resets every offer therefore sees backed-off re-offers, not a request loop.start_joining_fetchdoes, since its request ID is spent and a peer requiring the next ID would refuse every later request.Messages::failedfor every case, not only ACTIVE_COUNT, so the solicited draft-14/15 path, which runs inside a request task, closes the session too.read_responsenow returns the existingAnswerenum (Message/Ended/Late) instead ofOption.sync_namespaceholdswatchmutably throughout, so it no longer looks the entry up twice.Regression test
a_failed_advertisement_ends_only_its_namespacecovers draft-14 and draft-19, with both FIN and reset. Two namespaces are announced. The peer ends the first request before answering, then answers the second. The test checks that the first is offered again no sooner thanRETRY_BASE, that the second is withdrawn normally when it is unannounced (PUBLISH_NAMESPACE_DONE on draft-14, a reset on draft-19), and that the loop is still running. Without the fix it fails on its first case withSession(ProtocolViolation).a_failed_legacy_header_closes_the_sessionstops the request after its type and before its body: drafts 14/15 close the session, draft 19 re-offers after the backoff.Notes
protocol violationinstead of the custom string. The close code is unchanged, anda_counted_publish_namespace_ok_is_a_violationstill passes.Encodeerror) also backs off for that namespace instead of ending the session.is_protocol_violationalready treats it as not the peer's fault.Public API / wire impact
Public API: none. Wire: none.
Checks
cargo nextest run --workspace --tests: 9166 passed.just check: lint and fmt pass. The test stage failed twice onmoq-cli export::redundant_importers_switch_through_bufferbloat_on_lite_07, a known flake on main tracked inquest/m1/test-flakes-2/redundant-failover-drift.md(moq-lite only, which this PR does not touch). It passes alone and in the full nextest run above.quest check: ok.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code