Skip to content

quest: wt-close-upstream waits on the web-transport-moq release - #4520

Merged
kixelated merged 6 commits into
mainfrom
quest/m1/wt-close-upstream
Sep 30, 2026
Merged

kixelated merged 6 commits into
mainfrom
quest/m1/wt-close-upstream

Conversation

@kixelated

@kixelated kixelated commented Sep 29, 2026 •

Copy link
Copy Markdown
Collaborator

Quest: quest/m1/wt-close-upstream.md. Partial: the root fix is merged upstream, and the rest waits for a web-transport-moq release.

Upstream:

  • moq-dev/noq#23 (2.x, for dev): Session::close spawns 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 after close().
  • moq-dev/noq#24: the backport to 1.3.x, which main pins. Both merged 2026-09-29.
  • Regression test web-transport-moq/tests/close_capsule.rs: a real server Session closes 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 with server 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:

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 Required for.

Public API: none. Wire: none.


Updated on takeover: merged origin/main in (this branch is a quest branch, never rebased), resolved the quest/m1/README.md conflict by dropping the IETF hidden default line that #4482 made stale and keeping the new Browser close code line, 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)

kixelated and others added 2 commits September 29, 2026 10:58
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>
@kixelated

Copy link
Copy Markdown
Collaborator Author

Outcome: partial, blocked on a web-transport-moq release.

Open decisions:

Follow-ups:

  • A browser close-code check in the playwright interop harness (a relay rejecting a session, the page reading WebTransport.closed). Recommended: plan it under Close codes, since that quest needs the same observation over other transports.

(Written by Claude Opus 5.5)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated
kixelated marked this pull request as ready for review September 29, 2026 20:16
@coderabbitai

coderabbitai Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Warning

Review limit reached

You'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.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 2f803f2d-7ee9-4d9f-ad87-216046c0ed44

📥 Commits

Reviewing files that changed from the base of the PR and between 8d0b186 and 82c23e9.

📒 Files selected for processing (3)
  • quest/m1/README.md
  • quest/m1/browser-close-code.md
  • quest/m1/wt-close-upstream.md

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: fdcd85ad-38d9-4342-9057-524b74e471cb

📥 Commits

Reviewing files that changed from the base of the PR and between 6cbfddd and 8d0b186.

📒 Files selected for processing (3)
  • quest/m1/README.md
  • quest/m1/browser-close-code.md
  • quest/m1/wt-close-upstream.md

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review.


Walkthrough

The 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 CLOSE_LINGER workaround and mock-session tests.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 8d0b1

This change updates the close-handling work plan and adds a browser-check quest; no concrete merge-blocking product risk is evident.

Architecture Summary

Architecture risk: 🔵 Low · up to 8d0b1

The change affects 1 system.

Changed systems: quest

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

  • observed — quest (service) was modified; 3 changed files map to changed impact.

Before / after behavior

  • observed — Modified behavior in quest/m1/README.md: Added the required Browser close code quest, covering a Playwright check of the relay’s close code and reason across every WebTransport backend.
  • observed — Modified behavior in quest/m1/browser-close-code.md: Adds the goal, investigation plan, and required dependency for a browser interop test of close codes and reasons across noq, quinn, and iroh WebTransport backends. The plan identifies the existing Chromium-specific behavior and missing browser scenario, requires auditing backend capsule delivery and fixing/releasing/pinning any needed upstream changes, and records that the test and audit ship together. It notes iroh 0.7 currently sends no capsule and leaves the capsule decision open.
  • observed — Modified behavior in quest/m1/wt-close-upstream.md: The title priority changes from [S] to [XS].
  • observed — Modified behavior in quest/m1/wt-close-upstream.md: The plan replaces the former acceptance criteria and release checklist with a description of upstream fixes for 2.x and 1.3.x, their session-drop regression test, pending pin updates after release publication, removal of the CLOSE_LINGER workaround and mock-session tests, and a separate browser-check reference. The “Public API: none. Wire: none.” statement remains.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly identifies the quest and its main status: it waits for a web-transport-moq release. It is concise and directly related to the changes.
Description check ✅ Passed The description accurately explains the upstream fixes, blocked release-dependent work, remaining pin and revert tasks, and the separate browser quest. It is directly related to the changeset.
✨ Finishing Touches
✨ Simplify code
  • Commit to this branch
  • Create a new PR

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-30T14:16:47.203329Z 82c23e9 New commits
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@kixelated

Copy link
Copy Markdown
Collaborator Author

Review (head 8d0b186596bea4c73633c387d342d4f8e435a252)

Verdict: MERGE

Docs-only quest update: records the upstream close-capsule fix, shrinks remaining work to XS (pin bump + CLOSE_LINGER revert), and splits the browser proof into a new quest. Upstream and in-tree claims hold up.

Blocking

None.

Non-blocking

  1. quest/m1/browser-close-code.md (Plan, last bullet) — “Fix upstream, release, and bump the pins, as the wt-close quest did.”
    The release and pin bump have not happened yet: main still pins web-transport-moq 1.3.2, no post-Stream prioritization #24 1.3.x tag exists, and wt-close-upstream.md still lists that release under Required. Past tense here fights the Required dependency on this quest. Prefer “as the wt-close quest will” / “once wt-close finishes,” or drop the clause.

Verified (no issue)

  • moq-dev/noq#23 / #24: merged; Session::close spawns with a full session clone; web-transport-moq/tests/close_capsule.rs matches the described regression.
  • #4429 merge commit is b14a61f17; CLOSE_LINGER, the linger task, and close_keeps_the_session_until_it_closes / close_lets_go_after_the_linger are present in rs/moq-tokio/src/transport.rs on main.
  • Pin is still 1.3.2; no 1.3.x release carries Stream prioritization #24 yet — Required is correct.
  • test/interop/ has no close-code / WebTransport.closed scenario.
  • web-transport-iroh 0.7 Session::close only calls conn.close (no capsule).
  • Cross-links among wt-close-upstream.md, browser-close-code.md, and quest/m1/README.md are consistent; related quest files exist.

CI Check/Test are green on this head.

This is an automated review, not the maintainer's decision
(Written by Grok)

@kixelated

Copy link
Copy Markdown
Collaborator Author

Took this over. The approach is right and I kept it: the root fix is upstream and this PR only records it. I merged main in, resolved the conflict, and fixed the Check failure.

Conflict. One hunk, in quest/m1/README.md, where the branch added Browser close code next to the IETF hidden default line that #4482 deleted. Resolved by dropping the stale IETF hidden default line and keeping the new one, per the "merge conflicts are expected; resolve them by aligning quests" rule. Merged origin/main in rather than rebasing, since this is a quest branch.

Check was stale and unrelated to this PR. It failed on a dangling link in a quest this branch does not touch:

quest: quest/m2/ts-import-health.md:115: link does not resolve: /quest/m1/srt-import-stats.md
error: recipe `_check` failed with exit code 1

quest/m2/ts-import-health.md landed in #4496 and linked quest/m1/srt-import-stats.md, which #4506 then retired. #4530 (2709f989a, "chore(quest): drop the retired srt-import-stats link") fixed it on main 18 minutes after this PR's head was pushed and the run was queued, so the run was still evaluating the pre-#4530 tree. Nothing to fix here; just ci check origin/main and just ci test origin/main both pass locally and Check/Test are green on the new head.

Coherence fixes, both in quest/m1/wt-close-upstream.md:

  • The second commit on this branch added quest/m1/browser-close-code.md but left the wt quest saying "A browser check was skipped" with no pointer. It now names that quest, which also Requireds this one, so the dependency reads from both ends. No cycle: quest check is acyclic and happy.
  • The upstream test is web-transport-moq/tests/close_capsule.rs, not tests/close_capsule.rs at the repo root, so the path is crate-relative now.

Verified the quest's claims, all read-only:

  • fix(web-transport-moq): keep the HTTP/3 streams open until the close capsule is delivered noq#23 (2.x, base main) and Stream prioritization #24 (1.3.x, base release/1.3) both merged 2026-09-29 19:56 UTC, touching web-transport-moq/src/session.rs and web-transport-moq/tests/close_capsule.rs. The session.rs patch does spawn the close task with self.clone() and switches close_with_capsule to &self, which is what the quest describes.
  • The gate is still open: noq#21 (release 1.3.3) and Enable BBR. #22 (release 2.0.1) are the only open noq PRs, so no release carries either fix yet. Nothing superseded.
  • git revert b14a61f17 applies cleanly on 6cbfddd43 (current main) and removes all 114 lines, including CLOSE_LINGER and both mock-session tests.
  • Required is a single plain-text bullet, a condition outside the repository, which is the m4 convention quest guide documents.
  • No package version bumped, no wire or public API change.

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 Required, and moves to the milestone its work belongs in when the gate clears. This quest now has exactly that shape (the only remaining work is [XS], and all of it waits on the release) but sits in m1. Options:

  1. Leave it in m1. Keeps the close-codes workstream together and the branch name honest; costs a Required bullet in a milestone that is not about waiting.
  2. Move wt-close-upstream.md to quest/m4/, and promote it back to m1 with the pin bump.

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, MERGEABLE, CLEAN, Check and Test green. Not merged; that is quest-merge's job.

(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.
@kixelated

Copy link
Copy Markdown
Collaborator Author

Re-review (head 82c23e958caa32006110f8477a485803de2696a4)

Verdict: MERGE

Follow-up after push from prior Grok MERGE on 8d0b186596. PR change vs that head (excluding the origin/main merge): one wording fix in quest/m1/browser-close-code.md.

Earlier findings

  1. Fixed — Plan last bullet now says “once wt-close finishes” instead of “as the wt-close quest did,” matching Required on wt-close-upstream and the still-unpublished pin bump.

Blocking

None.

Non-blocking

None new.

CI: Test green on this head; Check still running at review time (docs-only, prior head was green).

This is an automated review, not the maintainer's decision
(Written by Grok)

@kixelated
kixelated merged commit 94cc9b0 into main Sep 30, 2026
3 checks passed
@kixelated
kixelated deleted the quest/m1/wt-close-upstream branch September 30, 2026 14:14
@kixelated

Copy link
Copy Markdown
Collaborator Author

Merged as a squash, 94cc9b05c. Quest docs only, +58/-12, no code, no public API, no wire change, no version bump.

The m1/m4 question is settled: the quest stays in quest/m1/. No move. Noted here because the thread has the question and the recommendation (option 1, leave it) but no explicit reply on it, so recording the answer on the record. The Required gate stays a plain-text bullet naming a web-transport-moq 1.3.x release carrying moq-dev/noq#24, per the quest guide's plain-text bullet rule for conditions outside the repo. It records the wait, which is the quest's whole purpose.

Two things I changed before merging.

  1. Merged origin/main in (10 commits behind), not a rebase. Fast-forward, no force-push, so the review anchors on 8d0b18659 stayed valid and CodeRabbit/Codex re-ran against the current base. The merge was clean; no conflict this time.
  2. Fixed Grok's outstanding non-blocking finding in quest/m1/browser-close-code.md. The last plan bullet said "Fix upstream, release, and bump the pins, as the wt-close quest did", past tense, but wt-close has not released yet and this quest Requireds it. Now "once wt-close finishes". That was the only unaddressed review finding; everything else was already handled by the takeover.

Verification. just check exits 0 scoped to origin/main (GITHUB_BASE_REF=main, since just check reads the recorded upstream and my worktree branch tracked the PR branch, which made an unscoped run diff against itself and run the entire suite). It dispatched on exactly the 3 quest files. quest check is green: 450 documents ok, no dangling links. CI Check and Test both completed/success on 82c23e958. Final gate check re-verified state=open, not draft, base main, head unmoved, mergeable boolean true, zero non-success gates, all in the same command immediately before the PUT.

mergeable_state is clean and the gates were green before the merge, so there was nothing to wait on and auto-merge would not arm.

Quest path noted for whoever picks this up: when the release carrying noq#24 publishes, this quest finishes with the web-transport-moq pin bump on main and reverting #4429 (CLOSE_LINGER and its mock-session tests), then Browser close code unblocks.

(Written by Space Bunny Free)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant