fix(uring): bound io_uring tests by the shared memlock budget - #4167
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review. WalkthroughThe changes add RLIMIT_MEMLOCK details to io_uring setup and buffer-ring registration errors. The configuration documentation escapes the underscore in io_uring. The nextest profile groups selected io_uring tests and limits concurrency to four threads. Identity, teardown, and worker tests add readiness signals before proceeding with connections. Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The test coordination and error diagnostics appear ready to merge after normal checks. No concrete merge-blocking issue remains. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to There is no identified new security exposure. The limited design risk is that callers inspecting the underlying operating-system error may handle an out-of-memory setup failure differently. Retained concerns
Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Hardening Proposals
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches✨ Simplify code
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. |
|
MERGE This is a positive, well-scoped fix for a real shared-resource failure mode. Positive improvement? Yes. Parallel Worth the complexity? Yes. The layers are small and each earns its keep:
No API/wire surface change beyond a clearer Different approach? Raising One minor note (not blocking): This is an automated review, not the maintainer's decision |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 65c38b7de2
ℹ️ 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".
| ready.send(()).expect("test alive"); | ||
| worker.block_on(stop.wait()).expect("worker loop"); |
There was a problem hiding this comment.
Drop each ready sender after reporting startup
If one helper worker succeeds and the other panics during setup, as can happen under partial memlock exhaustion, the successful thread keeps its cloned Sender alive while blocked in worker.block_on(stop.wait()). The second recv() therefore sees neither a message nor channel disconnection, so the test hangs until nextest kills it. Explicitly drop the sender after send, or send a per-thread Result, so partial setup failure is reported immediately. (Written by GPT-5.6 Sol)
AGENTS.md reference: AGENTS.md:L17-L18
Useful? React with 👍 / 👎.
| err.kind(), | ||
| format!( | ||
| "{err}: io_uring memory counts against RLIMIT_MEMLOCK ({limit}), shared by every process of this user; \ | ||
| raise it with `ulimit -l` or systemd's `LimitMEMLOCK=`" |
There was a problem hiding this comment.
Supply a new value when suggesting
ulimit -l
When ring setup hits ENOMEM outside systemd, the suggested ulimit -l command only prints the current soft limit and exits successfully. Bash's help ulimit states that the current value is printed when LIMIT is omitted, so following this recovery instruction changes nothing and the retry encounters the same error. Include an explicit limit or describe this command as inspection rather than remediation. (Written by GPT-5.6 Sol)
Useful? React with 👍 / 👎.
Every io_uring ring is charged to the user's RLIMIT_MEMLOCK (8 MiB by default), shared with every other process that user runs. Desktop apps on libuv already spend most of it, leaving room for ~28 workers. Parallel test runs exhausted it: Worker::new failed with ENOMEM, instantly on the main thread or, on a helper thread, as a ~10s wait for a dial to idle out. - nextest `io-uring` test group caps io_uring tests at 4 concurrent. - ENOMEM from ring setup or buf-ring registration names RLIMIT_MEMLOCK. - teardown/identity/workers tests wait for helper threads to report in, so a setup failure fails at once. - Relay docs note the memlock cost of io_uring workers. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
65c38b7 to
e3fce8e
Compare
|
Rebased onto current Landing the shared (Written by Grok 4.7) |
Co-authored-by: Grok 4.7 <noreply@x.ai>
|
Remark wanted the new memlock sentence to escape (Written by Grok 4.7) |
#4167 documented each ring at about 100 KiB. This branch halves the completion queue, so that figure is 56 KiB. Co-authored-by: Grok 4.7 <noreply@x.ai>
Problem
The moq-uring tests failed under parallel load (
just check, or several sessions at once). At--test-threads 64, 25 of 69 failed.Root cause: the kernel charges every io_uring ring to the user's
RLIMIT_MEMLOCK(8 MiB by default), and every process that user runs shares that budget. Desktop apps built on libuv already use more than 6 MiB of it, which leaves room for about 28 workers (~84 KiB each, mostly the 4096-entry CQ). I confirmed this withprlimit: at a 6 MiB soft limit, not even one worker starts. When a test ran out:Worker::newfailed at once withENOMEM;teardown, the client inidentity, a worker inworkers), the test sat for ~8-11s while a dial idled out.Fix
.config/nextest.toml: anio-uringtest group caps moq-uring and the relay's uring tests at 4 concurrent.Error::ring: anENOMEMfrom ring setup or buf-ring registration now namesRLIMIT_MEMLOCK, its current value, and how to raise it. The variant is stillError::Io, with the sameErrorKind.doc/bin/relay/config.mddocuments the memlock cost ofruntime.io_uringworkers.API / wire impact
None.
Error::ringispub(crate). The message of anENOMEMError::Iochanges.Testing
cargo nextest run -p moq-uring --test-threads 64: 69/69 pass (before: 25 failures). Two such runs at once, three times over: all pass.prlimit --memlock=65536:shows the new message.a_ring_enomem_names_the_memlock_limit.just checkdid not run to completion here becausemoq-ffitest targets fail to compile onmain(E0283 atrs/moq-ffibinary_stream, from feat(ffi): advertise JSON tracks in the catalog, add binary data tracks #4137). That is unrelated to this PR.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code