feat(sock): extract the shared listener plumbing and steer the uring endpoint - #3078
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: 04b2989cb3
ℹ️ 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".
…endpoint moq-sock is the runtime-neutral crate the M4 plan called for: bind (dual stack UDP/TCP, grown buffers, SO_REUSEPORT) moves out of moq-tokio wholesale and is re-exported as moq_tokio::bind, and steer becomes moq_sock::shard (Shard, the group bind with its port lock and probe, the cBPF steering filter, cid_prefix), with cpu owning core pinning. moq-tokio's workers now form their group from the shared crate, unchanged in behavior. moq-uring is the second consumer: endpoint::Config::shard makes every connection id the endpoint issues (accepts, rotations, and dials alike) lead with the slot's steering byte, so a steered reuseport group of uring workers keeps each connection on the worker that owns it. tests/workers.rs runs a two-worker group end to end: handshakes only complete if the steering holds, and both members must be fed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XLJU7rEfPe7hn7K5jX77mb
04b2989 to
6549ec2
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6549ec20d0
ℹ️ 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".
| /// kernel identifies a member by its position in the group, which is the order | ||
| /// the sockets bound. An unsharded bind is the plain one. | ||
| pub(crate) fn bind(addr: SocketAddr, shard: Option<Shard>) -> io::Result<UdpSocket> { | ||
| pub fn bind(addr: SocketAddr, shard: Option<Shard>) -> io::Result<UdpSocket> { |
There was a problem hiding this comment.
When two same-UID processes concurrently construct the same fixed-port group using this public bind loop, both index-zero probes can finish before either reuseport bind, after which their sockets silently join one interleaved group. Because bind neither acquires nor requires the separate Lock, every call can return Ok while the positional BPF filter routes one process's connection IDs to the other process's sockets during a rolling restart. Make group construction acquire and retain the exclusion handle, or require an owned token, so this invalid sequence cannot be represented. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L157-L162
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed as a real gap in the API's shape, and deliberately deferred to #3092 rather than fixed here.
Not a live bug: both callers (moq_tokio::worker::Workers::new, moq_relay::uring::Workers::bind) acquire the lock before the first member binds. What this PR changed is that the invariant used to be crate-private and is now prose an outside caller cannot be held to, so daa3cb2 at least promotes the precondition from a comment inside the function body onto bind's rendered docs.
The fix you describe (an owned token) is bigger than it looks: moq-tokio never calls shard::bind from its construction loop, it calls Worker::spawn, and the bind happens on the worker thread inside the quinn and noq backends. A Group handing out sockets means moving where moq-tokio creates its sockets across both backends. #3092 carries that, and moq-sock is at 0.0.1 and unpublished, so the reshape stays cheap.
(Written by Claude Opus 5)
`cid_prefix` spends `256 / count` values of the first connection-id byte, so a group larger than `MAX_SHARDS` leaves it a zero-width stride and `random_range(0..0)` panics. The bound used to be held by the one caller that could mint a shard; now that `Shard` is public and `Endpoint::with_shard` takes one, `Shard::new(0, 257)` returns `Some` and panics downstream. Bound it in the constructor, so every shard that exists is one the filter can steer. The count checks in `moq_tokio::worker::Workers::new` and the relay's uring group stay: they turn the same mistake into an error that names the limit, rather than a `None` the caller has to interpret. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The crate is not going to crates.io for a while, and 0.0.1 says so: the shape here is still free to move, which matters because it is currently three prose invariants (hold the lock, bind in index order, never resize) rather than a type that enforces them. `bind`'s doc now carries the lock precondition. The reasoning was already written down, but inside the function body, where nobody reading the rendered docs would find it: the probe only refuses a group that is already bound, so two groups constructing at once each pass it and then interleave. Both callers in this repo acquire the lock; an outside caller had no way to know it had to. `DIALS` goes to 32. Steering cannot be asserted deterministically from the test, since the client picks the connection id the filter hashes, so the count is the only lever: 16 dials left a `2^-15` chance of every Initial landing on one worker and failing a correct implementation. Co-Authored-By: Claude Opus 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: daa3cb289b
ℹ️ 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".
`CoreId` was an alias for `core_affinity::CoreId`, so both `cores` and `pin` handed that crate's type to callers. moq-sock is socket and listener plumbing, not a wrapper for that dependency, and the exemption for a thin wrapper does not stretch to a whole crate: as written, an upstream type change is a breaking moq-sock change nobody here chose. Make it an opaque newtype. Callers only ever take one from `cores` and hand it back to `pin`, so the only thing they needed from the inside was the number for a log line, which `id()` still gives them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Summary
M4 PR 2 on #2875, stacked on #3073: the shared-crate extraction the epic's M2/M4 text committed to ("extract the runtime-neutral ... CID codec, eBPF loader, and socket configuration that remain private in moq-tokio"), validated by its second consumer in the same diff.
moq-sock(new crate, 0.1.0) owns the thread-per-core listener plumbing both runtimes need:bindmoves out of moq-tokio wholesale (dual-stack UDP/TCP, grown socket buffers,SO_REUSEPORT) and is re-exported asmoq_tokio::bind, so the external callers in moq-cli and moq-relay keep their paths.steerbecomesmoq_sock::shard:Shard(absorbed fromlisten.rs, now with its group-discipline invariants documented on the type since it is publicly mintable), the in-order group bind with its portLockand bind probe, the classic-BPF steering filter, andcid_prefix. The kernel-facing behavior is untouched; the module's tests moved with it.cpuowns core pinning (cores,pin), a thin re-export-typed wrapper overcore_affinity.moq-tokio's
worker::Workersforms its group from the shared crate with no behavior change; it drops itssocket2andcore_affinitydependencies.moq-uring is the second consumer:
endpoint::Config::shardmakes every connection id the endpoint issues (initial accepts, rotations for migration, and dials alike) lead with the slot's steering byte. Dials carrying it is the cluster shape: a worker's upstream dial gets its responses steered back to that worker.Validation
tests/workers.rsruns a steered two-worker reuseport group end to end on real threads: sockets bound in index order on the main thread, oneWorker+Endpointper thread, 16 dials against the shared port. The steering assertion is structural: post-Initial packets (handshake continuation included) select by the id's first byte, so a wrong prefix strands the handshake on the wrong worker and the dial times out; every dial completing plus both members being fed is the proof. Passes in ~100ms on this kernel (7.1.3), kernel-gated with the loud-skip pattern like the rest.just checkandjust testpass across the affected crates (moq-tokio's worker/steer suites included).Notes for review
/docpage: moq-sock is internal plumbing with no end-user surface, like kio. The crate map entry inrs/CLAUDE.mdand its README carry the documentation.Shard::newbecoming public softens moq-tokio's old "cannot be minted outside the crate" guarantee; the invariant (bind once, in index order, never resize) now lives in the type's docs and each runtime's group owner. The uring group owner that enforces it structurally comes with the relay-integration PR.(Written by Claude Fable 5)
🤖 Generated with Claude Code
https://claude.ai/code/session_01XLJU7rEfPe7hn7K5jX77mb