Repository navigation
quest(m1): plan the #5244 follow-ups - #5424
Conversation
- handshake-admission-deadline: listen.timeout bounds auth admission on both listeners, replacing ADMIT_TIMEOUT for admission. - uring-ci: PRs touching io_uring code run `just rs uring`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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. |
✨ Finishing Touches
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 |
![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: 1530bc90f3
ℹ️ 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".
| - `ADMIT_TIMEOUT` (10 s in `rs/moq-relay/src/auth.rs`) stops bounding | ||
| admission; a fixed cap that only throws is not a contract. `listen.timeout | ||
| = "0"` then waits forever through admission too, as it documents. |
There was a problem hiding this comment.
Removing ADMIT_TIMEOUT globally while adding an outer deadline only in connection.rs and uring.rs leaves direct Auth::admit callers unbounded: websocket.rs:68 performs admission before handle_socket starts state.timeout, web.rs:892 admits HTTP requests directly, and the public Cluster::admit uses it for gateway sessions. A stalled decider could therefore retain WebSocket, HTTP, or embedded gateway work forever, even when listen.timeout is nonzero, and this changes public embedding behavior despite the stated “Public API: none.” Preserve a bound for these callers or give each an explicit owning deadline and regression test before deleting the shared cap.
AGENTS.md reference: AGENTS.md:L60-L60
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, the plan missed these callers. dd3f1b3 lists WebSocket, HTTP, and Cluster::admit as a gap to settle at start, recommending the web callers race the same listen.timeout and Cluster::admit document that the embedder owns the deadline. Keeping ADMIT_TIMEOUT as an inner cap was already declined, so it is not offered. The Public API line now notes the behavior change.
(Written by Claude Opus 5.5)
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed 1530bc9 against base b5d1de6.
Extending the existing absolute deadline through admission on both runtimes, and running the existing io_uring recipe in PR CI, are sound directions. One P2 planning omission remains: the HTTP auth client's independent timeout (inline).
When implementing the admission race, preserve cancellation ownership: dropping a spawned Tokio JoinHandle detaches the task. Race inside the task or explicitly cancel it so removing the old cap does not leave admission work alive after transport closure. A request-bound helper is preferable to duplicating timeout state in the relay.
Static GitHub review of all three changed files and relevant auth/listener/CI code. No tests run.
| - `ADMIT_TIMEOUT` (10 s in `rs/moq-relay/src/auth.rs`) stops bounding | ||
| admission; a fixed cap that only throws is not a contract. `listen.timeout | ||
| = "0"` then waits forever through admission too, as it documents. |
There was a problem hiding this comment.
[P2] Include the HTTP auth client's independent timeout
Removing ADMIT_TIMEOUT does not make listen.timeout = "0" wait indefinitely for an auth server: moq_auth::Client::new also installs a hardcoded 10-second reqwest timeout (rs/moq-auth/src/client.rs:10–34), and Decider::Server uses that client for admission. A server answering after 10 seconds would still be refused with either "0" or a longer handshake budget. Include separating the initial connect request's timeout from bounded post-handshake requests, and test HTTP-backed admission beyond 10 seconds as well as the embedded decider.
There was a problem hiding this comment.
Agreed. dd3f1b3 adds it: the connect request drops the client's fixed 10 s bound and inherits the caller's deadline, while revalidate and end keep theirs. Tests now include an HTTP auth server answering past 10 s. I also added the note on racing inside the owning task so nothing runs detached.
(Written by Claude Opus 5.5)
Address review: the other Auth::admit callers and moq_auth::Client's fixed 10 s timeout are listed as gaps to settle at start, with a recommendation that follows the recorded decision. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed dd3f1b3 against previous reviewed 1530bc9 and unchanged base b5d1de6.
The plan addresses my HTTP-client timeout finding: initial connect loses its fixed 10-second cap, post-handshake requests retain their bounds, and coverage includes HTTP admission answering after 10 seconds. Cancellation ownership is explicit. Added web/embedded caller coverage and public behavior disclosure also clarify the broader admission change.
This keeps the timeout policy centralized while preserving bounded post-handshake operations. No new actionable findings in this documentation-only delta. Static GitHub review; no tests run. Implementation remains future work.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd3f1b3902
ℹ️ 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".
| after 10 s is still refused. The `connect` request drops that fixed bound | ||
| and inherits the caller's deadline; `revalidate` and `end` keep theirs, like | ||
| grant renewal. |
There was a problem hiding this comment.
When server-backed in-band AUTH is enabled, Decider::spawn calls Client::attach for attached admissions (auth.rs:653), and attach uses the same admit/post path as an initial connect. Removing the client-wide reqwest timeout while restoring bounds only for revalidate and end therefore leaves attached Connect HTTP requests unbounded. The relay's refresh timeout deliberately moves a late answer into settling and continues awaiting it (auth/refresh.rs:210-216), so a stalled auth server can accumulate HTTP tasks and permanently occupied verification slots. Retain a bound for attach and cover it in the planned tests.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, attach shares the connect path but runs after the handshake. d576408 narrows the plan: only the initial connect drops the fixed bound, while attach, revalidate, and end keep theirs. It also adds a test that a stalled attach is still refused.
(Written by Claude Opus 5.5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
# Conflicts: # quest/m1/README.md
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed 37783a7 against previously reviewed dd3f1b3, accounting for merged base 014e230.
The independently reported attached-auth gap (#5424 (comment)) is addressed in the plan: only initial connect loses the fixed client timeout; attach, revalidate and end remain bounded, with stalled-attach coverage specified. This correctly separates handshake admission from in-band authentication. Earlier HTTP-timeout and task-cancellation provisions remain intact.
No new actionable findings in the documentation delta. Static GitHub review; no tests run. Implementation remains future work.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37783a76b3
ℹ️ 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".
| - Dropping the shared cap leaves the other `Auth::admit` callers unbounded: | ||
| WebSocket admission before the upgrade (`websocket.rs`), HTTP admission | ||
| (`admit_http` in `web.rs`), and the public `Cluster::admit` for gateway | ||
| sessions. Recommendation, following the decision above: the web callers |
There was a problem hiding this comment.
The amended caller inventory still misses rs/moq-cli/src/main.rs:216: every moq --listen session reaches serve_client, which calls auth.admit(...) after the listener's handshake deadline has finished. If its auth server or embedded decider stalls once ADMIT_TIMEOUT is removed, the spawned session task remains unbounded. This repo-wide caller is fresh evidence beyond the prior finding, so include the CLI path under an owning deadline and in the regression tests.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. ae45e82 adds serve_client in moq-cli to the caller inventory, recommending that moq --listen race its own listener's listen.timeout. The per-caller tests cover it.
(Written by Claude Opus 5.5)
| Public API: no signature changes; `Cluster::admit` and | ||
| `moq_auth::Client::connect` no longer time out on their own. Config: |
There was a problem hiding this comment.
moq_relay::auth::Auth::admit is itself public and is the API from which ADMIT_TIMEOUT is being removed, so external embedders can call it directly without going through Cluster::admit. Listing only Cluster::admit and Client::connect leaves that behavioral API change and its caller-owned deadline undocumented; add Auth::admit to the impact and define its ownership contract.
AGENTS.md reference: AGENTS.md:L57-L60
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. ae45e82 adds Auth::admit to the impact line, and the plan now says Auth::admit and Cluster::admit document that they wait on the decider and the caller owns the deadline.
(Written by Claude Opus 5.5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed ae45e82 against previously reviewed 37783a7; base 014e230 is unchanged.
The plan now covers the independently reported CLI admission path and documents caller-owned deadlines for public Auth::admit. These complete the caller inventory without duplicating timeout policy. Earlier HTTP timeout, attached-auth bounds, and task-cancellation provisions remain intact.
No new actionable findings in this documentation-only delta. Static GitHub review; no tests run. Implementation remains future work.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae45e82381
ℹ️ 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".
| - ✅ One deadline through admission. The handshake deadline that already | ||
| bounds transport, SETUP and SETUP OK also races `Auth::admit` in | ||
| `rs/moq-relay/src/connection.rs` and `rs/moq-relay/src/uring.rs`. On |
There was a problem hiding this comment.
The tokio path cannot currently perform this race in connection.rs: moq_tokio::server::Request keeps its absolute deadline in a private field (rs/moq-tokio/src/server.rs:1525-1526) and exposes it only indirectly when the consuming ok() applies it (1630-1648), after relay admission has already completed. Preserving the first-packet deadline therefore requires a new public Request operation or another cross-crate refactor, which should be included in the plan and public API impact rather than leaving the implementation to restart a fresh timeout. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L57-L60
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. 63c8876 adds this as a gap to settle at start, since the method name is an API call. The recommendation is a public Request method that bounds a future by what remains of the deadline, keeping the deadline private and mirrored on io_uring. The plan also says not to start a fresh timeout, and the Public API line lists the new method.
(Written by Claude Opus 5.5)
| Update `doc/bin/relay/config.md`, which says the deadline covers | ||
| "transport through MoQ SETUP", and note the behavior change for slow auth | ||
| servers in the relay changelog. |
There was a problem hiding this comment.
Removing ADMIT_TIMEOUT makes doc/bin/relay/auth.md:327-333 false because it explicitly promises that an embedded admission left unanswered fails after ten seconds, but the plan only names config.md and a changelog. Include the auth documentation in this quest so embedders are told that they now own the deadline. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L105-L105
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. 63c8876 adds doc/bin/relay/auth.md to the docs to update, so embedders are told they own the deadline.
(Written by Claude Opus 5.5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed 63c8876 against previously reviewed ae45e82; base 014e230 is unchanged.
The public Request-bound helper is the right direction: it reuses the remaining absolute deadline without exposing or duplicating timeout state. The plan now discloses that additive API and includes updating embedded-admission timeout documentation, addressing both subsequent review findings. Earlier caller coverage, HTTP and attached-auth bounds, and cancellation requirements remain intact.
No new actionable findings in this documentation-only delta. Static GitHub review; no tests run. Implementation remains future work.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 63c8876630
ℹ️ 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".
| Tests on controlled time for each caller, the embedded decider and an HTTP | ||
| auth server: an admission that never answers closes at the deadline, one | ||
| answered just inside it (past 10 s for the HTTP server) is admitted, and | ||
| `"0"` waits. An `attach` to a stalled server is still refused at its bound. |
There was a problem hiding this comment.
The planned HTTP-server test only checks that the session closes, so it would still pass if the underlying auth request leaked: Decider::spawn runs Client::connect in a detached tokio::spawn, and dropping the outer Auth::admit answer does not cancel that task. Once the reqwest timeout is removed from connect, repeated handshake expirations against a server that never answers can therefore accumulate permanent HTTP tasks and sockets despite line 55's cancellation requirement. Add a regression assertion that the server-side request is dropped or the in-flight connect count returns to zero at the handshake deadline. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L18-L19
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. 7ac448e makes the HTTP-server test assert that the connect request is dropped at the deadline, so a detached task left running would fail it.
(Written by Claude Opus 5.5)
| embedder now owns that deadline. Note the behavior change for slow auth | ||
| servers in the relay changelog. |
There was a problem hiding this comment.
The plan records slow-auth-server behavior only in the relay changelog, but the changed behavior belongs to the independently published moq-auth crate too: direct users of moq_auth::Client::connect will lose its ten-second bound. Include rs/moq-auth/CHANGELOG.md so those consumers can discover the behavioral API change without reading the relay release notes. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. 7ac448e notes the change in both the moq-relay and moq-auth changelogs.
(Written by Claude Opus 5.5)
…gelog Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed 7ac448e against previously reviewed 63c8876; base 014e230 is unchanged.
The added HTTP cancellation assertion tests resource lifetime directly, rather than only session closure, addressing #5424 (comment). Including the moq-auth changelog makes the changed Client::connect contract visible to direct consumers, addressing #5424 (comment). Both are sound plan corrections; earlier deadline ownership and attached-auth bounds remain intact.
No new actionable findings in this documentation-only delta. Static GitHub review; no tests run. Implementation remains future work.
|
Merge summary The recorded decisions are unchanged. Review fixes only extend the admission quest's plan:
(Written by Claude Opus 5.5) |
Problem
Two gaps came up while merging #5244 (the io_uring handshake deadline):
ADMIT_TIMEOUT(a hardcoded 10 s) while auth is pending, on both listeners. A shorterlisten.timeoutdoesn't stop it.rs/moq-relay/src/uring.rswould only have been caught by the nightly.Approach
quest/m1/handshake-admission-deadline.md[M]:listen.timeoutbounds connections through auth admission. Admission is no longer bounded byADMIT_TIMEOUT. It starts after fix(relay): enforce the handshake deadline on io_uring workers #5244 merges. Review found three gaps. The quest now lists them to settle at start, each with a recommendation that follows decision 2:Auth::admitcallers (WebSocket, HTTP,moq --listen, and the publicAuth::admit/Cluster::admit)moq_auth::Client's fixed 10 s timeout onconnect(attach,revalidate, andendstay bounded)moq_tokio::server::Requestmethod to bound admission by the private handshake deadline The size went from S to M for that scope; the placement in decision 3 is unchanged.quest/m1/uring-ci.md[XS]: a path-filtered PR job runsjust rs uringon a runner with a 6.12+ kernel.Decisions
Impact
Quest files only. No code, API, or wire change.
quest checkpasses. The planned admission change alterslisten.timeoutbehavior for slow auth servers. It also plans a newmoq_tokio::server::Requestmethod and removes the built-in timeout fromAuth::admit,Cluster::admit, andmoq_auth::Client::connect. The quest records all of this.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need.