[kixelated]

Problem

Four loose ends from #4062 left some AUTH stream endings reporting something other than what happened:

• A lite AUTH_ERROR code wider than u32 was saturated to u32::MAX, so distinct peer codes collapsed into one the peer never sent.
• JS AuthSession.close() cleared its tokens without recomputing the union, so auth.grant kept a live grant after the session closed.
• A JS presenter whose acceptor ended the grant with a clean FIN left its own write half open, so the acceptor's Issued.closed stayed pending until the session ended.
• A Rust Issued::closed() never resolved when the session dropped the task serving that token (AuthServe in lite, Serve in IETF).

Approach

• Out-of-range code: a lite AUTH_ERROR code that does not fit u32 ends the token with ProtocolViolation, and a token ending in ProtocolViolation now closes the session with PROTOCOL_VIOLATION. This also applies to the existing out-of-range AUTH_OK expiry, which used to end only the token. The lite draft says so in one line.
• JS close: AuthSession.close() recomputes the union, so it becomes empty, matching Rust.
• JS FIN: the presenter closes its side when the acceptor finishes the stream, the same as the AUTH_ERROR branch.
• Dropped serve task: a new auth::Serving guard owns the shared Issue. When it drops, it records the session's close error, unless the task already recorded one. The IETF driver now closes the auth handle before its Auth serve tasks drop, so they settle with the session's real error rather than a bare cancel. Lite already had that ordering.

Each fix has a regression test that fails without it:

• lite::session::tests::an_auth_error_code_past_u32_closes_the_session
• an_issued_grant_ends_with_its_session, run on lite-06, moqt-17 and moqt-22
• JS closing the session drops the grant and an acceptor that ends a grant sees its stream close, each run on all three protocols

Impact

• Public API: none. auth::Serving is pub(crate).
• Wire: none. The lite draft now states that an AUTH_ERROR code that does not fit in 32 bits is a PROTOCOL_VIOLATION that closes the session. This is a receiver rule for a field that already exists.

Alternatives

• Widening the error type to carry a u64 code. The quest decided against it, since session codes are u32 everywhere else.
• Having Issued::closed() also watch the session handle, instead of using a drop guard. That would need no change to the IETF drop order, but the quest chose a drop path so that no exit path can forget to settle.

Follow-ups

Tracked by the AUTH violations quest in #4554:

• A malformed AUTH_OK or AUTH_ERROR that fails to decode (for example a bad pattern) still ends only the token, like every other lite stream. The draft calls a bad pattern a PROTOCOL_VIOLATION.
• JS does not yet close the session on an out-of-range AUTH_ERROR code. It reports the exact code rather than saturating it.
• A token ending in ProtocolViolation on the IETF side (an out-of-range AUTH_OK expiry) still ends only that token.

(Written by Claude Opus 5.5)

🤖 Generated with Claude Code

[chatgpt-codex-connector]

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-30T13:53:57.388656Z	6857be9	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.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7bc0dd0f0f

ℹ️ 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".

[kixelated]

Automated review by review (OpenAI)

Reviewed head: 39d5f08

Direction: the token-lifetime fixes are well targeted. A serving guard that settles Issued.closed on every exit is preferable to adding cleanup independently to each return path; recomputing grants on session close is also necessary.

No additional actionable defect found in the changed serving/drop and presenter termination paths. The current head includes the changelog and test-cleanup fixes from earlier review. JS/IETF session-fatal propagation remains deliberately deferred to #4554, so this PR should be described as exact AUTH endings, not complete cross-language protocol-violation parity.

Verification: static review of changed Rust/JS paths and regression tests; no Rust/Bun tests executed here.

(Written by OpenAI)

[chatgpt-codex-connector]

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

[chatgpt-codex-connector]

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

[kixelated]

Review (head a0ef1f1a9d31002333c90a518c0f65577c721a99)

Exact AUTH endings on both sides: out-of-range lite AUTH_ERROR → session PROTOCOL_VIOLATION, JS grant recompute on session close, presenter FIN closes write half, and auth::Serving settles Issued::closed when the serve task is dropped. Solid, and each fix has a regression test.

Findings

No blocking correctness, race, or API issues in the changed paths.

Checked in particular:

• Serving::Drop reads Handle::closed then end(); get_or_insert keeps the task’s reason; kio::Shared mutation wakes Issued::closed.
• Lite Driver::drop / poll close the auth handle before publisher (and thus Serving) drops; IETF now closes auth before Auth serve tasks drop in the same arm — matches the quest’s drop-order fix.
• Out-of-range AUTH_ERROR and out-of-range AUTH_OK expiry both return Error::ProtocolViolation from PresentToken, which sets Present.violation and ends the session via poll_protocol.
• JS AuthSession.close() #recompute() after clearing tokens; presenter stream.close() on clean FIN settles acceptor Issued.closed (same as the AUTH_ERROR branch).
• Draft rule + moq-lite-07 changelog bullet are present; quest card removed cleanly with follow-ups parked on quest(auth): AUTH protocol violations close the session everywhere #4554.

Notes (non-blocking)

• Head CI still running (Android / Check / Test / WASM / Platform / Release JS queued or in progress). Prior head 39d5f08 was fully green.
• Latest commits after 39d5f08 are base-branch merges into the PR branch; no further logic churn in the AUTH paths.
• Remaining JS oversized AUTH_ERROR session-fatal and IETF token-only ProtocolViolation cases are explicitly deferred to quest(auth): AUTH protocol violations close the session everywhere #4554 — fine for this PR’s scope.

MERGE

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

[chatgpt-codex-connector]

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

[kixelated]

Takeover: base merge, CI, and the three review threads

Pushed d2755ac02 on top of a0ef1f1a9. No force-push, no close and reopen, so the original commits still stand.

The conflict

The branch had merged quest/m1/auth/README twice already, and the base moved again with #4560 (narrowing, 1766 lines across the same auth paths). Three conflicts, all real overlaps rather than textual noise:

1. quest/m1/auth/error-codes.md (modify/delete). The base trimmed a paragraph from it while feat(net): re-authorize a live session's grant in place #4560 landed; this branch had already deleted the card because this PR finishes the quest. Kept the deletion, so the quest is closed on completion rather than resurrected. README.md auto-merged, so the Required list correctly drops AUTH endings and, from the base, Origin narrowing.
2. rs/moq-net/src/ietf/auth.rs. The base replaced the one-shot default grant with a DefaultGrant that re-sends on every new limit, and this branch replaced Issue::shared() with the Serving drop guard. Kept both: Serve::run builds the Serving, takes &serving.issue, and passes default.as_mut() through to serve_issue.
3. rs/moq-net/src/lite/publisher.rs. Same pair of changes on AuthServe. Kept both fields: serving: Option<Serving> next to default: Option<DefaultGrant>, with the drop guard still settling Issue.peer when the task is dropped mid-stream.

Quest alignment check afterwards: quest check reports 434 documents ok, and every still-valid item survived. The violations.md and not-supported.md cross-references now point at each other with a note that AUTH endings landed in #4550, and the four follow-ups stay parked on #4554.

CI

There was no red check: head a0ef1f1a9 was already green across Android, Check, macOS, Release JS Packages, Test, WASM, and Windows. The only thing standing in the way was CONFLICTING / DIRTY.

After the merge, nix develop --command just check exits 0, plus quest check and just drafts check on their own. Verified the three regression tests still bite:

• lite::session::tests::an_auth_error_code_past_u32_closes_the_session passes.
• an_issued_grant_ends_with_its_session passes on lite-06, moqt-17, moqt-22, and all three fail with the Serving::drop body emptied. That one matters here: the guard is exactly what the merge touched.
• Both JS regressions fail on all three protocols with the fixes reverted.

No unrelated breakage found.

Review threads

• JS receiver not closing the session on an out-of-range AUTH_ERROR code. Declining, with reasoning, on the thread. It is the first gap in the AUTH violations quest on the base branch, that quest generalizes the rule across the whole violation family in both languages, and a partial JS implementation here would just be work the quest has to merge into. Worth noting the cross-package rule does not bite: no implementation can put a >32-bit code on the wire, since js/net/src/lite/auth.ts only ever encodes a SessionCode, and the JS side already reports the peer's exact code rather than a saturated one. If you would rather have it here, say so and I will pull it forward.
• Draft changelog bullet. Already present at the head you reviewed; pointed at the exact line.
• Raw timer and Promise.race in the JS test. Agreed and partly fixed already. Took the rest in d2755ac02: the wait is now bounded by the repo's withTimeout helper instead of hanging until the runner's timeout, and the test fails loud when the setup token never arrives. A regression now fails in 1s per protocol rather than 5s.

State

Mergeable, no blocking findings outstanding, no public API or wire change beyond what the body already claims. Marking it ready for review.

(Written by Space Bunny Free)

[kixelated]

Re-review (head d2755ac02b3e0adcc72f5acd9749337af18782c1)

Follow-up after push. Since a0ef1f1 (prior Grok MERGE): a base merge of quest/m1/auth/README (narrowing from #4560 — left out of this review) plus d2755ac, which bounds the JS grant-stream regression wait.

What changed in the PR itself

js/net/src/auth.test.ts — an acceptor that ends a grant sees its stream close:

• Setup moved into try/finally so client/server still close if setup fails.
• Throws if the setup token never arrives (no more optional chain on a missing request).
• Awaits issued.closed via withTimeout(..., 1000, ...) instead of the runner’s own timeout, so a hang fails in ~1s per protocol rather than the suite timeout × 3.
• Promise.resolve(issued.closed) is required because Issued.closed is PromiseLike and withTimeout takes Promise.

Findings

No new blocking issues. This correctly undoes the hang from the earlier “drop the raw timer” fix without bringing back a leaky raw timer — withTimeout clears its timer in finally.

Earlier MERGE findings still hold; no AUTH logic churn in this push.

Notes (non-blocking)

• Head CI still pending (Android / Check / Test / WASM / Platform / Release JS).
• 1000ms is plenty for a local issued.close() → stream settle; if CI ever flakes under load, bump rather than drop the bound.

MERGE

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

[kixelated]

Automated review by review (OpenAI)

Reviewed head: d2755ac

P2: Record dispatcher failures before dropping its AUTH handlers. This is a newly identified gap in the original PR, also present at 39d5f08, not a regression from this update. The auth.close at ietf/session.rs:472–477 is too late when run_dispatch itself fails. That function owns its TaskSet and returns UnexpectedStream for an unknown bidi request, dropping active AUTH handlers before its caller receives the error. With an existing issued grant, Serving::drop consequently records Cancel because the auth handle is still open; its first-reason-wins settlement prevents the later session close from correcting Issued.closed. Record the dispatcher error before its local tasks unwind, or keep those tasks owned by the outer driver. Add a regression that issues a grant, sends an unknown bidi request, and checks that Issued.closed reports UnexpectedStream rather than Cancel. The existing driver-abort test only expects Cancel, so it cannot catch this.

Direction: keep this PR and the private Serving guard; the lifetime fix is worth its small complexity. The merge preserves the base's DefaultGrant refresh path, and the JS timeout change cleans up both sessions on failure. Fix the teardown ordering above; no broader API redesign is needed. Previously deferred JS/IETF protocol-violation parity remains separate.

Verification: static review of the base merge, changed code, task ownership, and regression tests. Rust/Bun tests were not run here; head CI is not yet complete.

(Written by OpenAI)

[kixelated]

Re-review (head 6857be9ba85ec349c1168ee2c0a46ea8897247d5)

Follow-up after push. Since d2755ac02 (prior Grok MERGE): fix(net): settle grants with the error that ended the session — closes the IETF auth handle inside run_dispatch before its owned AUTH serve tasks drop.

What changed

rs/moq-net/src/ietf/session.rs — run_dispatch:

• Clones the auth Handle before the accept loop.
• Scopes the loop in an async block so tasks: TaskSet outlives the close.
• On any Err from the loop (including UnexpectedStream / WrongSize / decode failures), calls handle.close(err) while child AUTH serve tasks are still alive; then returns so TaskSet drop runs Serving::drop, which reads the closed handle instead of inventing Cancel.
• Driver’s later auth.close with the same error is a no-op (first close wins).

This matches the failure OpenAI flagged at d2755ac: run_dispatch owned the serve TaskSet, so an error return dropped Serving before the driver’s auth.close, and first-reason-wins locked in bare Cancel.

Findings

No blocking issues. Ordering is right: TaskSet::drive returning Err does not drop children; only TaskSet’s own Drop cancels them, and that runs after the new handle.close.

Notes (non-blocking)

• Still no regression that issues a grant, sends an unknown bidi type, and asserts Issued.closed is UnexpectedStream rather than Cancel. The existing an_issued_grant_ends_with_its_session only covers driver-abort → Cancel, so it cannot catch this class of bug. Worth adding given this push’s own failure scenario.
• Head CI still pending (Android / Check / Test / WASM / Platform / Release JS).

Earlier MERGE findings still hold; JS/IETF protocol-violation parity remains parked on #4554.

MERGE

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

[kixelated]

Automated review by review (OpenAI)

Reviewed head: 6857be9

The previous P2 is fixed in the code: the scoped dispatch loop borrows its TaskSet, and ietf/session.rs:945–950 records the dispatcher error before that set drops. UnexpectedStream and the other error returns now reach Serving::drop with the correct reason; the outer driver's close remains idempotent. No new actionable defect found in this one-commit update from d2755ac.

The requested regression is still missing. tests/auth.rs:614–632 only aborts the driver and expects Cancel, so it cannot protect this ordering fix. Add the previously suggested issued-grant + unknown-bidi test asserting UnexpectedStream.

Direction: keep this focused fix and the private Serving guard. The scoped loop adds little complexity and fixes the ownership boundary without changing the public API or wire contract.

Verification: static review of the complete incremental diff, dispatcher/driver teardown, TaskSet, Serving, and the existing test. No Rust/Bun tests run here. Head CI is still running/queued.

(Written by OpenAI)

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6857be9ba8

ℹ️ 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".

[chatgpt-codex-connector]

Settle lite grants when the driver completes

When a lite session driver reaches a terminal result but its caller retains the Driver, Driver::State::poll caches the completed Protocol::Lite instead of dropping it. The Publisher and its AuthServe children therefore remain parked, so this drop-only guard never records the session error and Issued::closed() can still wait indefinitely after the session has ended. Settle the issues when the lite driver completes, rather than relying on the public driver being dropped.

AGENTS.md reference: rs/AGENTS.md:L13-L15

Useful? React with 👍 / 👎.

[kixelated]

Merged as a merge commit into quest/m1/auth/README (questline, not main), per CONTRIBUTING. Merge commit 363367dcf, parents 67e16cba1 (base tip) and 6857be9ba8 (head). main is untouched.

One commit past the reviewed head

The automated review posted a P2 at 05:06 UTC, after the takeover summary, so it had not been addressed yet. It is real, and I confirmed it against the code:

run_dispatch owns the TaskSet holding the AUTH serve tasks, so returning Err dropped them before the driver reached its own auth.close. The new Serving guard then read a still-open handle and recorded Cancel, and because the first reason wins, the driver's later close with the real error could not correct it. So on that path every grant reported a bare cancel instead of, say, UnexpectedStream. (Handle::close never touches Issue.peer, so the guard is still required; before this PR the same path left Issued::closed() pending forever.)

Fixed in 6857be9ba by scoping the loop in rs/moq-net/src/ietf/session.rs so the handle closes while tasks is still alive. 12 net lines, no public API or wire change, the driver still closes again with the same error and that call becomes a no-op. just check is green on the new head: 4821 Rust tests, JS, drafts check, and quest check (quest: 434 documents ok).

What I did not do

I did not add the regression the review asked for, which issues a grant, sends an unknown bidi request, and asserts Issued::closed reports UnexpectedStream rather than Cancel. coding is private, so an integration test has to hand-roll the varint encoding and drive the raw poll transport in tests/support/mock.rs, shared by every test in that directory. I would rather flag that than guess at low-level wire plumbing in the last minutes of a merge. Happy to add it in a follow-up if you want the test on this questline.

The JS oversized-AUTH_ERROR rule stays in the AUTH violations quest, as settled: the Rust and JS halves land together there. The existing reply on that thread already says so, so I left it alone.

No package versions bumped.

(Written by Space Bunny Free)