Repository navigation
fix(net): withdraw timed-out draft 14-16 advertisements - #6133
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
On drafts 14-16 a PUBLISH_NAMESPACE rides the control stream, where a reset tells the peer nothing, so one that timed out unanswered was never withdrawn and the peer could hold it until the session closed. JS: Deadline.send records the request write and the message that withdraws it; on a deadline failure it waits out an in-flight write, sends the withdrawal, then resets. The advertisement passes PUBLISH_NAMESPACE_DONE on drafts 14-16 and the reset alone after. Rust: advertise_namespace ends a request that got no answer through end_request, which sends PUBLISH_NAMESPACE_DONE on drafts 14-16 and cancels both directions after, as the drop did. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…st-timeout-cleanup
|
Quest start outcome: implemented, with
The UNSUBSCRIBE_NAMESPACE part of the goal has no timeout to clean up after: neither language bounds the SUBSCRIBE_NAMESPACE answer once it is sent. (Written by Claude Opus 5.5) |
|
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 4 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 |
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: 0ff52d2cd7
ℹ️ 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".
| static async #reset({ stream, written, withdraw }: Recorded, failure: Error) { | ||
| // The withdrawal names a request the peer must already have, so a write still in | ||
| // flight finishes first, and one that never landed is not withdrawn. | ||
| if (withdraw && written && (await written)) await withdraw().catch(() => undefined); |
There was a problem hiding this comment.
When a draft-14–16 peer stops reading the shared control stream while the initial PUBLISH_NAMESPACE write is backpressured, written never resolves, so the timeout path now waits forever here instead of aborting at 10 seconds; the subsequent withdrawal can likewise block without a bound. This wedges the single publish-namespace loop and prevents every queued advertisement or withdrawal from progressing, so cleanup must remain bounded even when the peer is not consuming control data.
AGENTS.md reference: AGENTS.md:L18-L18
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Declining: the wait is already bounded by the session, and a deadline here would not unwedge anything.
- On drafts 14-16 every request shares one control-stream writer behind a single write mutex. A peer that stops reading it blocks every later control write, including the next advertisement and every withdrawal. Aborting at 10 seconds would let this loop move on only to queue its next write behind the same mutex.
- The loop's normal withdrawal already waits the same way:
#withdraw→#endawaits#doneand thenwriter.closedwith no deadline. Rust'send_requestalso awaits the DONE encode and the close. - The cost to us is one parked task with no state growth. Waiting actually removes growth: before this change, each timed-out advertisement left its write queued and the loop went on to queue more.
- The wait ends with the session. When the transport or the adapter closes, the queued write rejects, so
writtenresolvesfalseand the reset goes through. - Aborting without waiting would be wrong. A write already in the mutex queue still reaches the peer after the virtual stream is aborted, so a request the peer later sees would never be withdrawn. That stale state is what this PR fixes.
Waiting before throwing was a maintainer decision on this PR (Decisions, item 2).
(Written by Claude Opus 5.5)
|
Merge summary for
(Written by Claude Opus 5.5) |
# Conflicts: # js/net/src/ietf/publisher.ts # rs/moq-net/src/ietf/publisher.rs
…on test A draft-14 advertisement that times out is now withdrawn, and over the test's real memory streams that withdrawal waits for the peer to acknowledge the close. The test held the re-offered stream without reading it, so the loop never got to withdraw the other namespace. Answer it instead, as the test only checks that it is offered again. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Problem
On drafts 14-16 a PUBLISH_NAMESPACE rides the control stream, where a stream reset never reaches the peer. An advertisement that timed out after it was sent was never withdrawn, so the peer could hold the namespace until the session closed. This affected both JS (
Deadlinereset every stream it recorded) and Rust (advertise_namespacedropped the request onceread_responsegave up).Approach
Deadline's timeout path, as the #5741 review decided. A new internalDeadline.send(stream, write, withdraw?)records the request write and the message that withdraws it. When the deadline or credit fails, it waits for any write still in flight, sends the withdrawal if the request actually went out, and then resets.#advertisepasses PUBLISH_NAMESPACE_DONE on drafts 14-16 and nothing on 17+, so 17+ still just resets.#endand the timeout share a new#donewriter, which writes through the request'sRequestStreamfrom moq-dev/moq#6130. TheRequestStreamcloser stays the only way the advertisement ends: the caller'sabortafter a deadline failure is unchanged.read_responsegives up, the request now goes throughend_request, which now takes the stream, path, and request ID instead of a fullNamespaceRequest. That sends PUBLISH_NAMESPACE_DONE on drafts 14-16. On 17+ it cancels both directions with the same code the drop used.Other timeouts checked on drafts 14-16
Deadline, and its timeout cleanup already sends UNSUBSCRIBE (#cancelSubscribe). That had no test, so this PR adds a mocked-time test for drafts 14, 15 and 16.Impact
Deadlineis@internal).Decisions (both accepted by the maintainer on 2026-10-20)
Deadlinedeliberately leaves to the caller. Routing only its timeout throughDeadlinewould send UNSUBSCRIBE twice or need a second guard. Merging it intoDeadlineis out of scope for this PR.Deadline.sendnow and drop the timeout branch from its caller cleanup.Deadline.waitnow waits for the withdrawal before it throws, so the caller's ownabortin its catch block cannot reset the stream before PUBLISH_NAMESPACE_DONE is written:end_requestwaits the same way.Validation
an unanswered draft {14,15,16} advertisement is withdrawn at its deadlinean_unanswered_advertisement_is_withdrawn(drafts 14-16 send DONE; 17 resets)an unanswered draft {14,15,16} subscribe is unsubscribed at its deadlinepins the existing SUBSCRIBE cleanup.a_failed_advertisement_ends_only_its_namespace(new onmain) hung on draft 14. Its peer held the re-offered advertisement on a real memory stream without reading it, so the timed-out withdrawal waited forever for the close to be acknowledged. The test now answers that advertisement, since it only checks that the advertisement is offered again. In production, drafts 14-16 run over the control-stream adapter, whose virtual close completes at once, so the loop does not wait there.just checkpassed (@moq/net: 2,419 pass; Rust: 7,813 pass), and again after mergingmain, including moq-dev/moq#6130 (@moq/net2,434; Rust 7,923).quest checkpassed.just checkrun hit the known flakeexport::redundant_importers_switch_through_bufferbloat_on_lite_07(/quest/m1/test-flakes-2/redundant-failover-drift.md). It is in moq-cli on lite and unrelated to this change. It passed when run alone and on the rerun.Follow-ups
run_subscribe_namespacelasts as long as the session, so this is not urgent, but if a namespace subscription could ever end early, dropping its virtual stream would tell the peer nothing.Deadline(shared with lite) still resets the raw stream at its deadline, as it did before moq-dev/moq#6130. Moving that reset, the withdrawal, and SUBSCRIBE's#cancelSubscribeintoRequestStreamwould leave one closer for every IETF timeout. That is a separate refactor.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code