feat(relay): serve QUIC from pinned per-core workers, steered by connection ID (M1) - #2921
Conversation
![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: 2a85de2680
ℹ️ 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".
| std::thread::Builder::new() | ||
| .name(format!("moq-quic-{shard}")) | ||
| .spawn(move || run(shard, core, listen, quic, ready_tx, start_rx, done_tx)) | ||
| .with_context(|| format!("failed to spawn QUIC worker {shard}"))?; |
There was a problem hiding this comment.
If an embedder cancels Workers::serve, or Relay::run returns because the cluster, web, internal, or shared server branch fails, these detached threads retain their servers and continue accepting on the QUIC sockets indefinitely. The discarded JoinHandle and lack of a cancellation path also prevent the caller from reclaiming the port or reliably restarting the relay in-process. Keep an owned shutdown/join guard whose Drop stops every started worker. rs/CLAUDE.mdL128-L128 (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
| for shard in 0..count { | ||
| let listen = shard_config(config, shard, count)?; |
There was a problem hiding this comment.
When listen.bind uses port 0, each call to shard_config preserves :0, so Linux allocates a different ephemeral port for every worker instead of forming one reuseport group. Only the first address is stored and exposed through Relay::addr, leaving the remaining workers unreachable at that address and defeating the configured pool; bind the first shard, then use its selected port for subsequent shards. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
kixelated
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7d666bd13
ℹ️ 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".
| } | ||
|
|
||
| let socket = crate::bind::udp(listen).map_err(Error::BindSocket)?; | ||
| let socket = crate::steer::bind(listen, config.shard).map_err(Error::BindSocket)?; |
There was a problem hiding this comment.
When listen.backend = "noq" and more than one runtime worker is configured, this creates a steered reuseport group but leaves endpoint_config using noq's default random connection IDs (or the QUIC-LB generator). Once the client switches to a server-issued CID, its packets are therefore routed to cid[0] % worker_count rather than necessarily returning to the worker that owns the connection, so handshakes and established connections fail with probability increasing with the worker count. Select ShardIdGenerator when config.shard is set and reject the QUIC-LB combination as the Quinn path does. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
| let strides = 256 / count; | ||
| let stride = rand::rng().random_range(0..strides); |
There was a problem hiding this comment.
When --runtime-workers is greater than 256, Shard::new still accepts the count, but 256 / count is zero here and random_range(0..0) panics as soon as a backend generates a server CID. Even without the panic, a one-byte prefix cannot select workers with indices above 255. Reject these counts while constructing Shard or use a wider steering encoding so this invalid pool cannot reach the serving path. rs/CLAUDE.mdL128-L128 (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
| quic.validate()?; | ||
|
|
||
| let build_quic = config.bind.is_some() || !config.has_stream_listener(); | ||
| let build_quic = config.listeners.quic() && (config.bind.is_some() || !config.has_stream_listener()); |
There was a problem hiding this comment.
When moq-tokio is compiled with tcp or uds but without any QUIC backend, a caller using Listeners::Stream with a populated bind still hits the NoBackend guard below because it tests config.bind rather than build_quic. This contradicts the new stream-only mode's contract that QUIC is suppressed even when the shared config contains a QUIC bind, preventing backend-free consumers from reusing that configuration. Gate the error on build_quic. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
| let ready = ready_rx | ||
| .recv() | ||
| .map_err(|_| anyhow::anyhow!("QUIC worker {shard} exited before binding"))? | ||
| .with_context(|| format!("QUIC worker {shard} failed to listen"))?; |
There was a problem hiding this comment.
Although the new Workers::Drop addresses the prior detached-worker report for successfully constructed pools, this fresh error path still returns after spawning the current thread but before storing its JoinHandle in a Worker; previously stored Worker values also have no individual Drop. If any later worker fails to bind or initialize, all those handles are detached and Workers::bind can return before their sockets close, so an immediate in-process retry can join a reuseport group that still contains orphaned members. Keep partial construction under a guard that signals and joins every spawned thread before returning the error. rs/CLAUDE.mdL128-L128 (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
Benchmarked with perf, before and afterTook this over and measured it on x86-64 Linux (Ryzen 7 5800X, 8 cores / 16 threads, kernel 7.1.3, SetupBuilt the PR head at Load is 100 publishers x 8 subscribers each, every knob a scalar rather than a Efficiency, at 3.02 Gbps egress delivered identically in all three arms
Per-repeat ranges do not overlap (before 14.20-15.48 cyc/byte, after 12.62-13.18). That is 18.6% less relay CPU per delivered Gbps. Capacity, at the knee
7.7% more throughput on 9% less CPU, so 15.8% less CPU per Gbps. Worth noting the ranges as much as the medians: the baseline oscillates at the cliff while the worker pool sits flat across all three repeats. Where the saving actually comes fromNot from doing less work, and not from the syscall or lock path:
The same instructions retire, about 13% faster, because a connection's state stays in one core's cache instead of bouncing between threads. That accounts for essentially the whole 14% drop in cycles per byte. Lock contention does halve ( Two findings that argue with the PR
The workers are imbalanced 1.1x to 1.6x, busiest to idlest (median 1.5x pinned, 1.2x unpinned). Per-worker CPU ticks over one 30s window, pinned: Test coverage on a second architectureThe PR body notes the reuseport and steering paths only ran in an aarch64 container. They pass here on x86-64 Linux: all 200 CaveatsLoopback, both processes on one box, so part of the network stack cost is charged to whichever side sent. Equal across arms, but it is not a NIC. Four workers on four cores with 100 connections. And the shared runtime was idle throughout (0-2 ticks across its four threads) because there is no web or cluster load in this profile, so the PR's own concern about workers competing with a full second pool is untested by these numbers. (Written by Opus 5) |
Follow-up: quinn's single-threaded receive loop, and UDP buffer tuningTwo questions came out of the numbers above. Same box and setup as before (Ryzen 7 5800X, relay pinned to 4 physical cores, load generator on 4 others). 1. The single-threaded receive loop is real, but it is not what limits us at this size
I could not saturate it here. At the highest packet rate this box can produce:
The driver is at roughly 2% of its one-core ceiling, and The reason is structural rather than a bad test: at this workload the driver costs about one core for every 130 cores of other relay work, so it only becomes binding once the rest of the relay wants ~100+ cores. A four-core test saturates everything else long first. Saturating it needs roughly 2.5-5M inbound datagrams/sec, which is ~25-50 Gbps of MTU-sized traffic, or much less if the packets are small (an ACK flood from high fanout is the realistic path). Worth being explicit about two measurement limits. Loopback charges the receiver's IP/UDP/netfilter work to the sender's syscall context, visible in the call graph as So: this PR does fix the limitation structurally, taking the ceiling from one core to N. It is just not the source of the 15-19% gain measured above, which was cache locality. 2. We never tune the UDP socket buffers, and that turns out to be correctThere is no It visibly overflows. A single relay socket dropped 722,122 datagrams in 55 seconds (about 22k/sec), and That looks like a clear bug, so I fixed it: 4 MB request, read the value back, warn once when the kernel caps it. Drops went to zero and throughput fell by half. Three repeats per size, alternating binaries so drift lands on both:
Monotonic, so there is no sweet spot above the default. 3. Why the smaller buffer is fasterTwo mechanisms, measured rather than assumed. I had guessed quinn's 50us
Cache locality is the bigger effect. A deep queue means a packet is touched up to a megabyte of traffic after it arrived, by which point its skb and that connection's state are out of cache. IPC drops 28% and the L3 miss rate climbs by a quarter. The relay runs the same instructions at three quarters of the speed, and burns less CPU while delivering half as much, which is a stall signature rather than extra work. The drop also moves to the most expensive possible place. At 208 KB the kernel discards a packet at the socket before we spend a cycle on it. At 8 MB we admit it, decrypt it, route it, and then discard the group at the moq layer for being too old to serve: shed rate goes from 29/s to 339/s, close to 12x. The two compound, since slower processing deepens the queue, which ages more groups out, which wastes more cycles on data that gets discarded. The arithmetic lines up: instruction throughput ratio is (0.54 x 356) / (0.75 x 381) = 0.67 while goodput ratio is 0.55, and the gap is the extra work thrown away. The small buffer is acting as an accidental AQM, and shedding early is the right behavior for live media where late data has no value. 4. This is the strongest argument yet for the worker splitIn That is also the control proving the regression above is the buffer and not some unrelated slowdown in the patched binary: it only appears when the buffer actually fills. So the worker split fixes the socket drops in the right place. Baseline drops ~270k datagrams at the default size; workers drop zero at the same size, without paying any queueing delay for it. 5. Corrections, and how to read these numbers
(Written by Opus 5) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42460eaebf
ℹ️ 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".
| pub(crate) fn bind(addr: SocketAddr, shard: Option<Shard>) -> io::Result<UdpSocket> { | ||
| let options = crate::bind::Udp::new(addr).with_reuse_port(shard.is_some()); | ||
| let socket = crate::bind::udp(options)?; |
There was a problem hiding this comment.
When two worker-enabled relay processes overlap on the same Linux address, as during a rolling restart, SO_REUSEPORT makes the new sockets join the old group instead of reporting a port conflict. The new pool then installs a filter using only its own worker count, although the old sockets occupy the first kernel indices, so traffic continues selecting the old pool; as those sockets close, Linux compacts the group and the surviving workers' encoded shard indices no longer match their positions, preventing the replacement from accepting connections reliably. The pool needs exclusive ownership of the address or cross-process coordination that preserves stable indices.
Useful? React with 👍 / 👎.
| pub fn split(self) -> (Server, Runner) { | ||
| (self.server, self.runner) | ||
| } |
There was a problem hiding this comment.
When an embedder drops one returned Runner while keeping the other workers alive, this split API removes or stalls an individual reuseport member even though the BPF program and every CID generator retain the original count and indices. Linux may move the last socket into the removed slot, so existing CIDs begin reaching the wrong worker or falling back to tuple hashing; retaining the separately returned Server can instead leave a bound socket with no runtime servicing it. Make partial teardown unrepresentable by keeping the servers and threads under one pool-owned shutdown guard.
AGENTS.md reference: AGENTS.md:L143-L148
Useful? React with 👍 / 👎.
Milestone 1 of the thread-per-core runtime plan (#2875), first half. The relay runs one work-stealing runtime over one UDP socket, so every packet can cross threads and every wakeup is a candidate context switch. `runtime.workers` switches QUIC to the opposite shape: N threads, each pinned to a core, each running a `current_thread` runtime and owning one socket in a `SO_REUSEPORT` group on the listen address. A connection lands on whichever worker the kernel steers its first packet to and stays there, so its QUIC driver, session, and tasks never leave that thread. Everything that is not QUIC (web, internal, tcp/unix, clustering, signals, cert reload) stays on the shared runtime, and the workers reach the rest of the relay through the shared Cluster. Off by default. The kernel picks a group member by hashing the packet's 4-tuple, which holds a connection to one worker only for as long as the client keeps its address: a NAT rebinding or network change hashes somewhere else and reaches a worker that has never seen the connection ID. Steering by connection ID is the second half of M1 and needs a CID codec plus a reuseport filter; until then this is a measurement knob, which is also why the shared runtime stays the default. moq-tokio gains the two primitives the relay drives: - `bind::Udp`, an options struct for `bind::udp`, carrying `with_reuse_port`. It fails with `Unsupported` off Linux rather than binding a group the platform will not balance: macOS and the BSDs accept `SO_REUSEPORT` and then feed a unicast flow to one member, so the workers would come up healthy with one of them serving everything. - `listen::Shard` and `listen::Listeners`, so one process can build a QUIC-only `Server` per group member while a stream-only `Server` keeps tcp/unix on its main runtime. `Listeners` also makes "no QUIC" expressible, which was previously only implied by configuring a stream listener. `--listen-tls-generate` is rejected with workers, since each would generate and serve a certificate of its own; the fingerprint endpoint reads worker 0's, and every worker loads the same files. Verified on Linux (container, kernel 6.19 aarch64), since neither reuseport load balancing nor the integration test compiles on macOS. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Address hashing is the wrong key for QUIC. The kernel picks a reuseport member by hashing the packet's 4-tuple, so a client that changes address (a NAT rebinding, a network change, plain connection migration) hashes to a different worker and its packets arrive at a socket that has never heard of the connection. It dies instead of migrating. Without steering, `runtime.workers` is not a mode with a caveat, it is a mode that drops connections. So the group is steered by connection ID, in the new `moq_tokio::steer`: - Each worker issues connection IDs whose first byte is congruent to its index modulo the group size, keeping 256/count of that byte's randomness and all of the other 19 bytes. - A classic-BPF filter on the group reads that byte back and returns it modulo the group size, which is the index the kernel selects with. `SO_ATTACH_REUSEPORT_CBPF` runs with the UDP payload at offset 0, so the whole rule is seven instructions: branch on the header form bit, load the connection ID's first byte from offset 1 or 6, reduce, return. A client's first packets carry an ID the client invented, which encodes nothing. Those hash arbitrarily, and that is correct: whoever takes the Initial owns the connection and issues IDs naming itself, so every later packet steers to it, and a retransmitted Initial repeats the same client-chosen ID and reaches the same worker rather than starting a second handshake elsewhere. Classic BPF rather than the SK_REUSEPORT + SOCKARRAY eBPF program: the rule is small enough, and cBPF needs no CAP_BPF, no BPF toolchain in the build, and no map to keep alive across restarts. It selects by position, so the group must be built once, in order, and never resized, which is what `Workers::bind` already does. This needs a backend whose connection IDs we choose. quinn and noq allow it; quiche exposes no such hook through `web-transport-quiche`, so it refuses to start with workers rather than silently hashing addresses. QUIC-LB wants the same bytes, so that combination is refused too. Also from review of the first commit: - Workers were detached: `serve` consumed the pool and kept only the `done` receivers, so cancelling it left the threads accepting into a cluster nobody was driving, and a replacement pool would join the same reuseport group as the orphans. The pool now keeps its `JoinHandle`s and a per-worker stop channel, `serve` borrows instead of consuming, and `Drop` signals every worker and joins every thread. - An ephemeral listen port gave each worker a port of its own instead of a shared one, leaving all but the first unreachable behind an address that read as bound. Every worker's bound address is now compared against the first. - The module claimed cert reload stays on the shared runtime; each worker in fact watches the files itself. Documented, with the non-atomic rotation window that implies (#2924). Verified on Linux (container, kernel 6.19 aarch64). The steering test sends short- and long-header packets carrying each member's connection ID from a fresh source port every time and asserts each lands on its own socket; with the filter detached it fails on the first packet. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The thread-per-core mode landed entirely inside moq-relay, which made the
relay the only server that can have it. Everything about it is generic
except three things: the cluster/auth/shutdown a worker serves into, the
accept loop it runs, and the flags an operator types.
So `moq_tokio::worker` now owns the pool: the threads, the pinning, the
ordered bind, and the teardown that joins them. `Workers::bind` returns a
group that is already listening; each `Worker` splits into the `Server` it
bound and the `Runner` that must drive it, so a caller supplies its own
accept loop without moq-tokio needing a callback or a handler trait. The
bind order is the reason this belongs here at all: the cBPF filter selects
a member by *position*, and that invariant was being held by a `for` loop
in another crate.
moq-relay keeps `RuntimeConfig` (the `--runtime-*` flags), which is the
only genuinely relay-specific half, and `Relay::workers` is now a plain
`Vec<Worker>`.
Two shapes came off the public surface on the way:
- `listen::Listeners` is gone. It existed because `bind: None` means two
things ("default QUIC listener" when nothing else is configured, "no
QUIC" when a stream listener is), so "no QUIC listener" was
inexpressible. It is a construction choice, not configuration, so it is
now a crate-private `server::Parts` argument plus one public
constructor, `listen::Config::init_streams`. The enum also could not
classify iroh, which is attached after `Server::new` and escaped the
filter entirely.
- `listen::shard` is gone, and `listen::Shard` is crate-private. It was
`#[arg(skip)] #[serde(skip)]` on a struct whose whole identity is clap
and serde: the relay re-parses its config after the TOML merge, so the
field survived only by being set afterwards. Worse, `Shard` is `Copy`
and `Shard::new` was public, so a caller could clone one into two
configs and bind position 2 twice, breaking steering with no error. The
group is the only thing that can hold that invariant, so it is the only
thing that can mint one.
Fixes a bug in the noq backend while wiring this: it defined
`ShardIdGenerator` but never installed it, so a noq worker group would
issue connection IDs encoding nothing while the filter steered on them.
Every packet after the handshake would hash to an arbitrary member. It
now installs the generator and refuses QUIC-LB, matching quinn.
Also fixes the workers being built with a default `quic::Config` rather
than the operator's transport tuning.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three findings from review, all in the worker/steering path. `cid_prefix` spends one byte of the connection ID on the member index, so a group can have at most 256 members. Nothing enforced that: the count is a `u16`, and at 257 `256 / count` is zero, so `random_range(0..0)` panics on the first server-issued connection ID. A relay configured that way binds fine and then loses a worker to a panic on its first connection. Even without the panic, `byte % count` can never return an index past 255, so those members would sit idle. `Workers::bind` now refuses the group before spawning a thread or binding a socket, and the encoding test covers the 256 boundary where each member has exactly one stride. A stream-only server no longer trips the no-QUIC-backend guard. It keyed off `config.bind`, so a build with `tcp` but no backend feature refused to start with a bind configured, which is precisely the case `Config::init_streams` exists to serve. It now also asks whether the caller wanted QUIC at all; leaving everything else configured is still the same error. A worker that dies before reporting is joined rather than detached. Its siblings were already released by `Runner`'s `Drop` when the group's `Vec` unwinds, but the thread that actually failed was the one left behind. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two problems with the shape `worker::Workers` landed in, both found in review. `IntoIterator for Workers` handed out owned `Worker`s, each releasing its own socket on drop. The kernel numbers a reuseport group by position and moves the last socket into whatever slot a `close` vacates, so one member leaving renumbers a sibling out from under every connection ID already issued against it: the steering filter still reduces modulo the original count, and traffic for the moved member goes somewhere else entirely. The module documents that the group is bound once and never resized, and then handed out the pieces to resize it with. So the group keeps its members. `Workers::split` hands out each bound `Server` paired with a `Spawner<'_>` that borrows the group and can only spawn onto its thread, which keeps the caller supplying its own accept loop without letting a member leave. Sockets are released together, by `shutdown` or by dropping the group. `Runner::drop` also joined its thread, and the relay holds these inside async `Relay::run`, so a shutdown or a failure elsewhere in the `select!` blocked a shared executor thread on a join with no bound. `Workers` now has an async `shutdown` that signals every worker and does the joins on the blocking pool, and `Relay::run` awaits it after the loop resolves. `Drop` still joins, so a group dropped without `shutdown` releases its port rather than leaking threads; it is just no longer the path the stock relay takes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
f91816c to
ca558c3
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ca558c30f3
ℹ️ 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".
| let core = cores.get(index as usize % cores.len().max(1)).copied(); | ||
| let shard = Shard::new(index, count).expect("index is below count"); | ||
|
|
||
| let worker = Worker::spawn(listen.clone(), quic.clone(), shard, core)?; |
There was a problem hiding this comment.
When the documented listen.bind form is a DNS hostname with multiple local A/AAAA addresses whose ordering rotates between queries, every Worker::spawn resolves the cloned hostname independently, so members can bind different addresses and Workers::bind fails with WorkerPortMismatch nondeterministically. Resolve the hostname to one SocketAddr before this loop and pass that same address to every worker.
Useful? React with 👍 / 👎.
`Workers::split` hands back an owned `Server`, and a `Server` owns its socket. The `Spawner` borrow keeps a caller from dropping a worker's thread on its own, but not its socket, so dropping one of these servers (or letting the future built from it return while its siblings serve) still takes a member out of the reuseport group. The kernel then moves the last socket into the vacated slot and every connection ID encoding the moved member steers out of range. Making that unrepresentable means handing the group a closure to build each accept loop from, which this crate keeps out of its public API and which is why `split` exists in the first place. So the obligation is documented and tracked in #2964 rather than half-enforced: run every server, and treat the first one that finishes as the end of the group, which is what `Relay::run` does. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The PR's benchmarks measured pinned and unpinned workers inside run-to-run noise of each other on a single-socket box: the win is the socket-and-runtime -per-worker split, not core affinity. Saying pinning is the point of the mode would steer an operator toward the wrong knob, so the pin docs now say what was measured and why the default stays on (NUMA, and it costs nothing). Also document the measured 1.1-1.6x worker imbalance in the relay config doc: connection placement is the kernel's hash of the first packet, so a worker is somewhat less than a core of capacity, narrowing as connection counts grow. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…g failure A Workers::bind that failed midway (a port mismatch, a member that failed to spawn) dropped the members it had already started without joining them, so its error returned while their sockets were still closing. An owner that immediately rebound the address could join the half-dead reuseport group and be renumbered by the kernel when it finished dying. Worker now signals and joins in Drop; a whole group still tears down through Workers, which takes the threads first and leaves the member Drop a no-op. The regression test drives the real partial-construction path (an ephemeral port with two members) and then plain-binds the first member's address, which only succeeds once that socket is really gone. Mutation-checked on Linux: with the join removed it fails immediately. Also resolve a hostname listen address once for the whole group instead of once per worker, so a DNS answer that rotates between queries cannot hand members different addresses and fail the bind nondeterministically. An unset bind stays unset, since the backends fall back to a literal default and folding it in would flip a stream-only config into opening a QUIC listener. Linux-only paths ran in a container on kernel 6.19 aarch64: the 7 worker integration tests, all 207 moq-tokio lib tests, and the relay's runtime_workers test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 091f846434
ℹ️ 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".
| #[allow(unused_mut)] | ||
| let mut server = config.listen.init(config.quic.clone())?; | ||
| let mut server = match &workers { | ||
| Some(_) => config.listen.clone().init_streams()?, |
There was a problem hiding this comment.
When runtime.workers is set without a listen.tcp or listen.unix listener, as in the documented QUIC-only configuration, init_streams() creates a server with no transports. Relay::run still passes that server to serve, whose first accept() returns None; it then reports stopped accepting connections, wins the select!, and shuts down every QUIC worker immediately. The integration test bypasses Relay::run, so it does not exercise this failure. Skip the shared-server arm when it has no listener, or keep that arm pending in worker-only mode. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
1 participant
Milestone 1 of #2875. Targets
devbecausemoq-tokioonly exists there (M2's rename landed in #2896) and becausebind::udpchanges signature.What this does
runtime.workers = Nmoves QUIC off the shared work-stealing runtime and onto N threads, each pinned to a core, each running acurrent_threadruntime and owning one socket in aSO_REUSEPORTgroup on the listen address. A connection is handled start to finish by one worker: its QUIC driver, its session, and its tasks never leave that thread.Everything that is not QUIC stays on the shared runtime (web, internal,
tcp/unix, clustering, signals), matching decision 4 in the epic. The workers reach the rest of the relay through the sharedCluster, exactly as sessions on one runtime already do, so 1:N fanout across worker threads goes through the existing origin model rather than anything new.Off by default; the shared runtime is unchanged when
runtime.workersis unset.Steering
Address hashing is the wrong key for QUIC. The kernel picks a reuseport member by hashing the packet's 4-tuple, so a client that changes address — a NAT rebinding, a network change, plain connection migration — hashes to a different worker and its packets arrive at a socket that has never heard of the connection. It dies rather than migrates. A worker pool without steering is not a mode with a caveat, it is a mode that drops connections, which is why this landed here rather than in a follow-up.
The new
moq_tokio::steermodule does the standard thing:countvalues of one byte and keeps256 / countof its randomness; the other 19 bytes stay fully random. Connection IDs are not secrets, but they are unlinkable only while they look random, so this spends as little of that byte as it can.SO_ATTACH_REUSEPORT_CBPFruns the program with the UDP payload at offset 0 (the kernel pulls the header off first), so the whole rule is seven instructions: branch on the header form bit, load the connection ID's first byte from offset 1 (short) or 6 (long),mod, return.A client's first packets carry an ID the client invented, which encodes nothing. Those hash arbitrarily, and that is correct: whoever takes the Initial owns the connection and issues IDs naming itself, so every later packet steers to it. A retransmitted Initial repeats the same client-chosen ID and so reaches the same worker, which is what stops a retry from starting a second handshake somewhere else.
Classic BPF rather than the
SK_REUSEPORT+BPF_MAP_TYPE_REUSEPORT_SOCKARRAYprogram named in the epic. The rule is small enough to express in cBPF, which needs noCAP_BPF, no clang or aya in the build, no map lifecycle, and no precompiled object in the repo. The trade is that the kernel selects by position, so the group must be built once, in order, and never resized — which is whatWorkers::bindalready does, and it now says so. eBPF stays available later if dynamic membership or richer steering turns out to matter; thesteermodule's surface hides which one is in use.Steering needs a backend whose connection IDs we choose. quinn (the default) and noq expose a CID generator hook; quiche does not, through
web-transport-quiche'sezbuilder, so it refuses to start with workers rather than silently falling back to address hashing. QUIC-LB wants the same bytes of the connection ID, so that combination is refused too.API
The worker pool lives in
moq_tokio::worker, not in the relay: everything about it is generic except which cluster a worker serves into, what its accept loop does, and the flags an operator types. The bind order is the reason it belongs there at all — the cBPF filter selects a member by position, and that invariant should not be held by aforloop in another crate.worker::Workersowns the threads, the pinning, the ordered bind, and the teardown. It refuses what it cannot serve correctly: a generated certificate, an ephemeral port, and a group larger than the 256 members the one-byte steering prefix can name.worker::Spawner<'_>borrows the group and can only spawn onto its own worker's thread. The lifetime is load-bearing: a member that could be dropped on its own would leave the reuseport group, and the kernel moves the last socket into the vacated slot, renumbering a sibling out from under every connection ID already issued against it. Threads are released together or not at all. Not yet closed:splithands back an ownedServer, which owns the socket, so dropping one without running it — or letting its accept loop return while siblings serve — still resizes the group. The relay stops every worker as soon as one finishes, so its window is the shutdown it was already in, butmoq-tokiodoes not enforce that for an embedder. Documented onsplitand tracked in QUIC workers: dropping one split() Server resizes the reuseport group #2964; theSK_REUSEPORTmap-based selector from Thread-per-core relay runtime: io-uring + quiche, eBPF connection steering, rename moq-native to moq-tokio #2875 retires the whole class.Workers::shutdownis async and hands the thread joins to the blocking pool.Dropstill joins, so a group dropped without it releases its port rather than leaking threads; it is just not the path the relay takes, where the drop would land on a shared executor thread.bind::Udp, an options struct forbind::udp(the repo's "options struct, not positional parameters" rule), carryingwith_reuse_port. It is a signature change, sobind::udp(addr)becomesbind::udp(Udp::new(addr)). I first triedimpl Into<Udp>to keep those compiling and backed it out: it breaks inference at everybind::udp("...".parse().unwrap())site with anE0283, a papercut every downstream caller would hit.listen::Config::init_streams, a server with only thetcp/unixlisteners, for a process whose QUIC lives elsewhere. This makes "no QUIC" expressible:bind: Nonealready means "open the default QUIC listener" when nothing else is configured, so the stream-only case needed a constructor rather than a config field.steer,listen::Shard, and the per-server listener selection are all crate-private. A shard is only meaningful as a member of a group that was bound once, in order, soWorkersis the only thing that can mint one.moq-relaykeepsRuntimeConfig— the--runtime-*flags, the genuinely relay-specific half — andRelay::workersis anOption<moq_tokio::worker::Workers>, bound duringRelay::load(so a port conflict is a startup error, not a worker that quietly died) and split inRelay::run, because the cluster it serves into does not exist yet at bind time.Fixed after adversarial review
Two Codex review passes. Round one, against the first commit:
serveconsumed the pool and kept only thedonereceivers, so cancelling it left the threads accepting into a cluster nobody was driving, and a replacement pool would join the same reuseport group as the orphans. The group now owns its threads and stops them as a unit.dropping_the_workers_releases_the_portis the regression; with the teardown neutered it fails withAddrInUse.:0gave each worker a port of its own, leaving all but the first unreachable behind an address that read as bound. Each worker's bound address is now compared against the first.ShardIdGeneratorand leftendpoint_configon random IDs, so a noq worker group would steer on a byte encoding nothing and blackhole every packet after the handshake. It compiled clean because noq is not a default feature.Round two, against the restructure:
IntoIterator for Workershanded out owned members, each releasing its own socket — the resize the module documents as never happening.split+ a borrowedSpawnerreplaces it.Dropblocked a shared executor thread on an unboundedthread::join.Workers::shutdowndoes the joins on the blocking pool, andRelay::runawaits it.256 / countis zero at 257, sorandom_range(0..0)fired on the first server-issued CID, and members past 255 could never be addressed anyway. Refused at bind, with the 256 boundary covered in the encoding test.config.bindrather than asking whether the caller wanted QUIC at all.Not fixed here, with reasons:
EADDRINUSE, but inherent to reuseport + cBPF rather than to this PR. Filed as QUIC workers: a restarting relay silently joins the old process's reuseport group #2960; the fix is a group-lifetime lock or the eBPFSOCKARRAYpath.splitresult can still leave the group — dropping a returnedServer, or letting its accept loop return while siblings serve, closes a socket and renumbers the array. Enforcing it means handing the group a closure to build each accept loop from, which this crate keeps out of its public API, so the contract is documented onsplitinstead. Filed as QUIC workers: dropping one split() Server resizes the reuseport group #2964. The relay stops every worker on the first one to finish, so its window is a shutdown already in progress.devand untouched here. Filed as moq-relay: SIGINT skips the graceful drain, because the accept loop consumes ctrl_c too #2923.moq-tokioto accept in-memory certificate material. Filed as moq-relay: TLS rotation is not atomic across thread-per-core QUIC workers #2924. The actively-broken case (--listen-tls-generate) is refused outright.Constraints
bind::udpfails withUnsupportedelsewhere rather than binding a group the platform will not balance: macOS and the BSDs acceptSO_REUSEPORTand then deliver a unicast flow to a single member, so the workers would come up looking healthy with one of them serving everything.--listen-quic-lb-id.--listen-tls-generateis refused with workers.--runtime-workers Non an N-core box gives N pinned threads plus a full stealing pool. Documented: setTOKIO_WORKER_THREADSto bound it. Worth revisiting if the M1 profile shows it as noise.Testing
just checkandjust testpass on macOS (3368 tests), rebased onto currentdev. The reuseport and steering paths do not compile there, so they ran in a container on kernel 6.19 aarch64 — 21 tests, all passing:steer::a_connection_id_reaches_its_own_member— the one that matters. Four members; for each, a short-header and a long-header packet carrying that member's connection ID, each sent from a fresh ephemeral source port so the 4-tuple hash cannot be what routes it. Every packet must land on its own socket. Mutation-checked: with the filter detached it fails on the very first packet, so it measures steering rather than luck.steer::a_prefix_reduces_to_its_own_shard/prefixes_cover_every_shard— the codec round-trips for group sizes 1..255, and no member is unreachable.steer::the_program_branches_to_the_right_loads— the cBPF jumps are offsets from the following instruction, so an off-by-one reads the wrong byte instead of failing to load.bind::udp_reuse_port_*— the group forms, a member that forgets the option loses the port, and the kernel really does spread across it.moq-tokio tests/worker.rs— the group releases its port both viashutdownand viaDrop, an ephemeral port is refused, a lone worker may use one, a generated certificate is refused, and a group of 257 is refused.moq-relay tests/runtime_workers.rs— a real relay with 4 workers serving a publisher and 4 subscribers over QUIC, so the frame crosses worker threads through the shared origin.Cross-package sync
moq-relayconfig change →doc/bin/relay/config.md(new[runtime]section). No wire format change — connection IDs are opaque to the peer and this changes only how we choose ours — so nodrafts/update. Nomoq-ffisurface change, so no binding rows apply.moq-benchis unaffected; the M0 suite runs against this mode unchanged.Completes M1 of #2875, which stays open for M3 onward.
🤖 Generated with Claude Code
(written by Opus 5)