Repository navigation
quest: wt-close-upstream waits on the web-transport-moq release - #4520
Conversation
The fix is open upstream as moq-dev/noq#23 (2.x) and #24 (1.3.x). The pin bump and the CLOSE_LINGER removal wait for a published release. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Outcome: partial, blocked on a Open decisions:
Follow-ups:
(Written by Claude Opus 5.5) |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 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 13 minutes. View limit detailsLimit details: You’ve used all 4 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review. WalkthroughThe M1 quest list adds a required browser check for WebTransport close codes and reasons across all listed backends. The upstream close-task quest now describes session-retention fixes, regression testing, release requirements, pin updates, and removal of the Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to This change updates the close-handling work plan and adds a browser-check quest; no concrete merge-blocking product risk is evident. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 1 system. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 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 |
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. |
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit 42f9842.
Direction: appropriate as a tracking/documentation-only change. Keep the local linger workaround until a released dependency contains the upstream lifecycle fix, then remove it in the pin-bump change. The separate browser/backend verification work is valuable because the Rust peer cannot prove Chromium behavior.
No code correctness finding: this PR records the dependency and remaining work; it does not itself ship the runtime fix.
Verification: static diff and targeted source inspection. I did not run the repository test/build suites or reproduce runtime scenarios in this review. This is a COMMENT review, not a merge approval.
…tream Align the quest with the new browser close-code quest and drop the IETF hidden default line, whose quest #4482 deleted on main. Co-Authored-By: OpenCode (space-bunny-free) <noreply@opencode.ai>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Review (head
|
|
Took this over. The approach is right and I kept it: the root fix is upstream and this PR only records it. I merged Conflict. One hunk, in
Coherence fixes, both in
Verified the quest's claims, all read-only:
Reviews. Agreeing with the Codex review as written: keep the linger workaround until a release carries the fix, and remove it in the pin-bump change. No change made for it. One decision for the maintainer, not taken here. The m4 README says work waiting on an upstream release lives in m4, gated by a plain-text
Recommendation: 1. The author already decided to finish this quest on this branch when the release lands, and m1 lists the sibling close-code quests, so a move would split the workstream across two milestones for the length of a wait. Reversible in one commit either way, so I left it. State. Ready for review, (Written by Space Bunny Free) |
Grok's review flagged the past tense in the last plan bullet: wt-close has not released yet, and browser-close-code Required's it. Say so.
Re-review (head
|
|
Merged as a squash, The m1/m4 question is settled: the quest stays in Two things I changed before merging.
Verification.
Quest path noted for whoever picks this up: when the release carrying noq#24 publishes, this quest finishes with the (Written by Space Bunny Free) |
Quest:
quest/m1/wt-close-upstream.md. Partial: the root fix is merged upstream, and the rest waits for aweb-transport-moqrelease.Upstream:
dev):Session::closespawns its capsule task with a clone of the whole session instead of just the connection. The H3 control and QPACK streams then stay open until the peer closes the connection or the existing max(3 RTT, 100ms) force-close fires, even if the caller drops its last handle right afterclose().mainpins. Both merged 2026-09-29.web-transport-moq/tests/close_capsule.rs: a real serverSessioncloses and drops, and a hand-rolled HTTP/3 client standing in for a browser must read the capsule before the server's control stream ends. It fails on 1.3.2 and 2.0.0 withserver control stream ended before the connection: Ok(None)and passes with the fix.Remaining, once a release carrying moq-dev/noq#24 (and moq-dev/noq#23 for
dev) is published:web-transport-moqpin.b14a61f17), which deletesCLOSE_LINGER, its task, and its two mock-session tests.git revertapplies cleanly onmaintoday (verified against6cbfddd43). With it applied andweb-transport-moqpatched to fix(web-transport-moq): keep the HTTP/3 streams open until the close capsule is delivered (1.3) noq#24,cargo test -p moq-tokio --features noqand clippy pass.The browser case is not in this quest: the playwright interop harness has no close-code scenario, so it is its own quest, browser close code, which this quest is a
Requiredfor.Public API: none. Wire: none.
Updated on takeover: merged
origin/mainin (this branch is a quest branch, never rebased), resolved thequest/m1/README.mdconflict by dropping theIETF hidden defaultline that #4482 made stale and keeping the newBrowser close codeline, pointed the wt quest's browser-check sentence at that quest instead of leaving it orphaned, and made the upstream test path crate-relative.(Written by Space Bunny Free)