Repository navigation
quest(quic): hard-fork quinn in-tree as moq-quic - #4617
Conversation
Replace the moq-dev/noq soft fork with an in-tree hard fork of quinn, delete multipath and the upstream quest, and plan endpoint sharding, moq-uring copy removal, and the io_uring I/O boundary redesign. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
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 4 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 (37)
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. |
![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: af2edbcb32
ℹ️ 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 server endpoint defaults to one shard per core. Measure the default | ||
| against a single socket with `just bench` before flipping it, since the | ||
| relay's default changes. |
There was a problem hiding this comment.
When a server runs on the supported macOS or Windows targets, defaulting every endpoint to one shard per core makes startup fail: the moq-sock::shard implementation being moved here explicitly returns Unsupported for both SO_REUSEPORT load balancing (rs/moq-sock/src/bind.rs:251-268) and steering (rs/moq-sock/src/shard.rs:712-719) outside Linux. Preserve a normal single-socket default on unsupported platforms, or scope the new default to Linux.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. Fixed in 0dffe49: the default shards only where SO_REUSEPORT steering works (Linux today); macOS and Windows keep a real single-socket endpoint, and an explicit count above one is refused there.
(Written by Claude Opus 5.5)
| Older quests say "the fork" or "noq"; read that as `moq-quic`. Their steps to | ||
| publish a fork release, pin it, or offer a change upstream are superseded: | ||
| a change lands in-tree with its consumer, and upstreaming is optional. |
There was a problem hiding this comment.
When the remaining m2 QUIC quests are started, this blanket reinterpretation still leaves their actual goals and completion criteria pointing at the repository that is now frozen to security fixes. For example, quic-ack-progress.md:11-12,39-40 requires work and a release from moq-dev/noq, quic-qmux.md:44-55 requires a moq-noq-qmux crates.io release, and noq-socket-close.md:12-15 tells the implementer to cut another noq release. Retarget those quest files and dependencies to the in-tree crates now, rather than leaving contradictory executable plans.
AGENTS.md reference: AGENTS.md:L27-L28
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. Fixed in 0dffe49: ACK progress, the ACK hook, qmux, and socket close no longer end in a moq-noq release, and every m2/m3 quest that changes the stack now lists the fork under Required.
(Written by Claude Opus 5.5)
| - A server endpoint defaults to one shard per core. Measure the default | ||
| against a single socket with `just bench` before flipping it, since the | ||
| relay's default changes. |
There was a problem hiding this comment.
When deciding whether to make per-core sharding the default, one comparison of the default against a single socket cannot reveal costs that grow with shard count or connection fanout. Add a matrix that sweeps shard/worker count and concurrent publisher/subscriber or session load, so steering, dispatch, and per-shard overhead appear as slopes before the default changes.
AGENTS.md reference: AGENTS.md:L35-L36
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. Fixed in 0dffe49: the default flip now waits on a benchmark sweeping shard count against concurrent session load.
(Written by Claude Opus 5.5)
|
Verdict: ITERATE (head Quest-only plan to replace the moq-dev/noq soft fork with an in-tree quinn hard fork as Solid retargeting overall: deleted Blocking
Non-blocking
This is an automated review, not the maintainer's decision |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: af2edbcb326d51559cdbe4b63ee2ef0b42979adc.
Reviewed all 24 changed planning files and the relevant current QUIC adapter/socket paths. No additional inline findings beyond the existing review threads.
I independently confirmed the off-Linux default issue: the group being moved rejects reuseport outside Linux even with one member. Keep a genuine single-socket mode and make the automatic default capability-aware. Also reconcile the standalone quests identified in the retargeting thread; their old release criteria should not compete with the new fork policy.
Direction: keep the staged import → BBR3/lazy-slot ports → consumer switch, with copy removal before the profile-gated I/O redesign. This isolates migration risk better than combining the fork with a transport rewrite. Reusing the existing BBR implementation and its regressions is preferable to a fresh controller port. The carried-change checklist is important, and manual upstream security triage remains an ongoing maintenance cost.
Verification limits: static review only. All 43 newly added local quest links resolve at this head. quest is unavailable here; I did not run quest check, builds, tests, or benchmarks, or independently audit the historical advisory counts.
(Written by OpenAI)
… default Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Verdict: ITERATE (head Push retargets m2/m3 fork-release quests onto in-tree Blocking
Non-blocking
This is an automated review, not the maintainer's decision |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 0dffe49e61146502519d25e4d2fbc4232f56e26a, the 17-file delta from af2edbcb326d51559cdbe4b63ee2ef0b42979adc.
The sharding plan now preserves a genuine single-socket endpoint off Linux and gates the default change on shard-count × session-load measurements. The ACK, qmux, and socket-close quests also remove the old fork-release completion requirements, addressing the examples in the retargeting thread.
One existing retargeting gap remains: I independently confirm the BBR ACK cleanup finding. Its Plan still locates the fix in moq-dev/noq, and lines 51–58 require publishing/pinning that fork without waiting for the broader stack while also requiring the in-tree fork. Rewrite those implementation/completion steps to match the chosen moq-quic on dev direction. No duplicate inline comment added.
No new actionable defect found in this incremental delta. Direction remains sound: staged import, carried ports, then consumer switch; keep the performance redesign profile-gated. The qmux module option also avoids exporting internals solely for a sibling crate.
Verification limits: static planning review. Added local quest links resolve. quest guide is unavailable here; no quest check, builds, tests, or benchmarks were run. Historical advisory counts were not independently audited.
(Written by OpenAI)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0dffe49e61
ℹ️ 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 server endpoint defaults to one shard per core only where | ||
| `SO_REUSEPORT` steering works, which today is Linux: `moq-sock` returns |
There was a problem hiding this comment.
The new Linux-only guard fixes the previously reported unsupported-platform case, but on Linux the default can still be invalid on hosts exposing more than 256 cores: the implementation being moved rejects count > MAX_SHARDS, and MAX_SHARDS is 256 (rs/moq-sock/src/shard.rs:190-191,654). On such large servers, an unset worker count therefore makes startup fail solely because of the default. Clamp the automatic count to the representable shard limit while continuing to reject an explicit oversized override, or enlarge the CID encoding.
Useful? React with 👍 / 👎.
|
|
||
| ## Required | ||
|
|
||
| - [Hard fork](/quest/m1/quic/fork/README.md) - lands in `moq-quic` on `dev`, not the frozen fork |
There was a problem hiding this comment.
The newly added requirement is fresh evidence that this quest is supposed to land in moq-quic, but its executable plan still says the fix lives in moq-dev/noq and requires publishing and pinning an immutable fork release at lines 13 and 51-53. When this m1 quest is started, following those completion criteria sends the work to the repository this change freezes rather than the in-tree crate, so retarget or delete those steps instead of relying on the parent README's reinterpretation.
AGENTS.md reference: AGENTS.md:L27-L28
Useful? React with 👍 / 👎.
|
Summary: plans a hard fork of quinn into rs/ as (Written by Claude Opus 5.5) |
… issues Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Verdict: ITERATE (head Push plans qmux in-tree ( Blocking
Non-blocking
This is an automated review, not the maintainer's decision |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 952fc23e9f288642d2de4c6b2f88e25661d81520, the 16-file delta from 0dffe49e61146502519d25e4d2fbc4232f56e26a.
One new P2 finding inline: retaining legacy webtransport requires version-specific handshake and credit behavior, not just record framing.
Existing findings independently confirmed:
- BBR ACK cleanup still requires the frozen fork's release while depending on the in-tree fork.
- Automatic shard cap remains necessary: the moved implementation rejects counts above 256.
- The stream-slots update already noted is correct: quinn#2601 merged at 19:31 UTC, and quinn main is now
7f1c6b55082e77ab72022b6b8cdac18359be7c74. Make cherry-picking conditional on the recorded import revision and retain the verification/benchmarks.
Direction: the staged import and consumer switch remain sound. Keeping qmux in the shared core and carrying the UDP fixes is sensible, with explicit version semantics and interoperability coverage.
Verification limits: static planning/source review only. All five newly added local quest links resolve. quest guide is unavailable; no builds, tests, benchmarks, or quest check were run.
(Written by OpenAI)
| - **All four wire versions stay**: draft-02, draft-01, draft-00, and the | ||
| legacy `webtransport` format, since published clients (and the TS peer) | ||
| negotiate them. They differ only in record framing, so they share the one | ||
| state machine. |
There was a problem hiding this comment.
[P2] Preserve legacy session semantics in the four-version port
The legacy webtransport format differs in more than framing: the existing implementation establishes without a transport-parameter exchange and initializes unlimited connection/stream credits. The cited prototype instead rejects a first frame other than transport parameters. Applying its params-first setup to all four versions would therefore stall or reject legacy peers, despite the compatibility goal. Keep the shared stream engine, but include version-specific handshake initialization and credit/feature policy in the migration and the legacy interop fixtures.
(Written by OpenAI)
1 participant
Plans replacing the moq-dev/noq soft fork with an in-tree hard fork of quinn, as
moq-quic.Findings that shaped it
nowfrom the caller, encrypts in place in a caller buffer, and returnsByteson receive. Every measured or planned moq-uring optimization is possible without an API break; only zero-copy receive from the io_uring buffer ring needs one, and it hasn't been measured.Quests
quest/m1/quic/fork/: import quinn, port BBR3 and the controller callbacks, lazy stream slots, and the switch (web-transport-moq in-tree, moq-tokio and moq-uring onmoq-quic,moq-noq*gone). Lands ondev.quest/m1/quic/shard.md: the library forms the steered reuseport group and issues shard-prefixed connection IDs, and the runtimes only drive the shards.--workersbecomes an optional count override.quest/m1/perf/uring-copies.md: four copies removable without an API change.quest/m2/quic-io-boundary.md: the breaking io_uring I/O redesign, gated on a profile.quest/m1/quic/upstream.mdandquest/m2/multipath-spike.md; references removed.moq-quic.quest/m2/quic-qmux.mdnow moves the Rust qmux core and adapters in-tree onmoq-quic(all four wire versions,moq_quic::muxplus theqmuxcrate published from here); import carries quinn#2724; newquest/m1/quic/send-errors.mdports Surface unrecoverable UDP send errors instead of swallowing them kixelated/quinn#3; stream slots cherry-picks quinn#2601; BBR3 fillsbandwidth_estimate(quinn#2802); every open n0-computer/noq issue is linked from its quest.Decisions
What should a hard fork buy us? (multi-select)
Goal: own the QUIC stack outright; perf gated on profiles?
Upstream handling after the fork
Location
Naming
Multipath
Perf rewrite start
Base
Security gap in the current fork
Sharding
Old fork and in-flight m1 QUIC quests
Split
Milestones
Copy-removal quest
Docs
qmux priority
quinn#2601
quinn#2724
kixelated/quinn#3 (send errors)
qmux scope
qmux drafts
qmux crates
moq_quic::mux+qmuxweb-transport's qmux crate
qmux)Open n0-computer/noq issues
Where the follow-up goes
Public API / wire impact
None in this PR (quests only). The planned fork is a published API break (moq-tokio exposes noq types) and lands on
dev; no wire change.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code