Repository navigation
fix(net): renew draft-16 announcements in place - #6051
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Draft 16 answers a REQUEST_UPDATE by the update's own Request ID, which the control-stream adapter dropped. Route that answer to the request it updated, in Rust and JS, so a swapped route auth renews a draft-16 PUBLISH_NAMESPACE with REQUEST_UPDATE instead of withdrawing and re-announcing it, and draft-16 subscription updates keep one in flight. An unanswered draft-16 update now withdraws with PUBLISH_NAMESPACE_DONE, and a cancel that beats the answer ends only its request. Completes quest/m1/auth/request-update.md. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…date # Conflicts: # quest/m1/auth/request-race.md # quest/m1/auth/request-update.md
|
Outcome: draft-16 announcements now renew in place with REQUEST_UPDATE, with answers routed by the update's own Request ID through the control-stream adapter (Rust and JS). Lite needed no code (#5972). Interop ( Left as a draft for one decision: delete the quest now (recommended) or keep it open for JS concurrent renewals. (Written by Claude Opus 5.5) |
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: 2a5322e386
ℹ️ 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".
|
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 6 minutes. View limit details
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughDraft 16 update handling now includes response routing, in-place namespace authorization renewal, and subscription update tracking. Tests cover success, refusal, cancellation, timeout, retries, and serialized subscription updates. Documentation now identifies Draft 16 as the threshold for Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 77.78% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 18 functions across 9 files. (5 skipped: … | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly and concisely identifies the main change: in-place renewal of draft-16 announcements. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Description check The pull request description directly explains the draft-16 adapter, publisher, subscriber, documentation, quest, and test changes in the changeset.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 77.78% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 18 functions across 9 files. (5 skipped: 3 unsupported, 2 too large.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
✨ Finishing Touches
-
✨ Simplify code
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
A draft-16 PUBLISH_NAMESPACE_CANCEL that beats the answer to our update already ended the request, and draft 16 sends no DONE after a cancel. Drop the request locally and offer it again, in Rust and JS. The new draft-16 JS tests settle the mock transport instead of waiting on wall-clock time. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c2409cd80
ℹ️ 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".
A draft-16 receiver refuses an update with REQUEST_ERROR and then PUBLISH_NAMESPACE_CANCEL, and draft 16 sends no DONE after a cancel. Drop the request locally on a refusal, the same as on a cancel that beat the answer, and re-offer it once the refusal's wait is out, in Rust and JS. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Merge summary: draft-16 announcements renew in place with REQUEST_UPDATE, with answers routed back to the request they updated, in Rust and JS. Review rounds since the last design change:
Decision: on draft 16, a refused namespace update is treated as ending the advertisement with no DONE. A peer that refuses but never cancels would keep its old advertisement while we re-offer; draft 16 does not define this for namespaces, so we follow the subscription rule and our own receivers. Local checks on 853f714: Enabling auto-merge on 853f714. (Written by Claude Opus 5.5) |
Problem
moq-dev/moq#5695 let a route swap its auth and renew the live
PUBLISH_NAMESPACEwithREQUEST_UPDATE, but only from draft 17. On draft 16 the publisher still withdrew the namespace and announced it again, so subscribers saw it go away, even though draft 16 hasREQUEST_UPDATEand our receiver already accepts a renewal there.The reason was routing, not the message. Draft 16 rides the control-stream adapter, and the peer answers an update with
REQUEST_OKorREQUEST_ERRORnaming the update's own Request ID. That ID names no virtual stream, so the adapter dropped the answer. For the same reason our draft-16 subscription updates were never counted, so nothing kept one update in flight at a time there.The quest's lite half needs no code: moq-dev/moq#5972 decided lite puts nothing on the wire for
Route::auth, and #5695 already advertises lite routes unnamed. This completes Live requests follow their auth, so the quest file is deleted.Change
REQUEST_UPDATEwe send is recorded against the request it rides before it leaves. ItsREQUEST_OKorREQUEST_ERRORgoes to that request's stream, and a refusal no longer closes the request. An entry goes with its answer or its request, so our own sends bound the map. Draft 15's answers are still dropped, as before.REQUEST_UPDATEnaming itsPUBLISH_NAMESPACE, like draft 17+. Drafts 14 and 15, and naming or dropping a token, still withdraw and announce again.PUBLISH_NAMESPACE_DONEbefore offering the namespace again. Without it the peer would hold two copies.PUBLISH_NAMESPACE_CANCELthat beats the answer ends only that request, which is offered again with noPUBLISH_NAMESPACE_DONE(draft 16 §6.2 sends none after a cancel). Rust used to end the announce loop withUnexpectedMessage; JS, whose adapter closes the stream on a cancel, used to answer it with a DONE.Route::auth, JSRoute.auth,doc/concept/standard.mdanddoc/concept/moq-lite.mdnow say renewal is in place from draft 16.moq-lite.mdnow describes moq-transport renewal at all.quest/m1/auth/request-update.mdand pointed its references at feat(net)!: let live announcements renew their auth #5695.Tests
Each fails without its fix:
adapter::our_update_answers_reach_their_request: answers reach the updated request, a refusal leaves it open, an update split across writes is still recorded, and nothing is left once the request goes.draft_15_update_answers_are_not_routedcovers draft 15.swapping_a_route_auth_is_a_request_updatenow runs on drafts 16 and 19.an_unanswered_renewal_advertises_againruns on both and checks the draft-16PUBLISH_NAMESPACE_DONE. Newa_draft_16_cancel_ahead_of_the_answer_ends_only_its_requestalso checks no DONE follows the cancel.upstream_priority_follows_the_subscriberscovers the draft-16 subscriber. With routing but no counting, it fails withmoq-transport-16 ... priority stuck.an answer to our update reaches the request it updated on draft 16 only,draft 16 swaps a route's token with one REQUEST_UPDATE(a second renewal waits for the first answer, so it needs routing),an unanswered draft 16 update withdraws its request before offering it again,a draft 16 cancel ahead of the answer offers the namespace again without DONE, anda draft 16 subscription update waits for the last one's answer.Impact
REQUEST_UPDATE(Existing Request ID set) on aPUBLISH_NAMESPACEwhen its route auth changes, where we used to sendPUBLISH_NAMESPACE_DONEand a newPUBLISH_NAMESPACE. We now wait for each draft-16 update's answer before sending the next update on that request; draft 16 section 9.11 requires the peer to send exactly one. A draft-16 peer that never answers stalls further subscription updates (priority, max age) on that subscription, as on draft 17+. An unanswered namespace update times out and is withdrawn.Follow-ups (not in this PR)
PUBLISH_NAMESPACEstream, so a draft-16PUBLISH_NAMESPACE_CANCELor a draft-17+ cancel is only seen at the next update. This predates the PR.Checks
just check: every Rust (nextest, 7663 tests and the scoped suites) and JS suite passed. The one failure wasbench/relay.test.py(test_a_foreign_listener_never_proves_relay_readiness), which times out inside the agent sandbox and passes outside it. This branch does not touchbench/. Recipes after it in the dispatch may not have run locally; CI covers them.just test interop --all: all checks passed.origin/main:cargo test -p moq-net --lib(2169 passed) andjs/netbun test(2346 passed).quest checkis clean.just fixis clean.cargo test -p moq-net --lib(2169 passed), clippy on moq-net lib and tests,js/netbun test(2347 passed),tscand biome, andjust test interop --all(all passed).Decision
request-update.md. JS sending renewals one at a time stays accepted, as in feat(net)!: let live announcements renew their auth #5695, and is recorded under Follow-ups; it becomes its own quest only if a JS publisher with many routes appears.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code