Skip to content

fix(uring): bound io_uring tests by the shared memlock budget - #4167

Merged
kixelated merged 2 commits into
mainfrom
claude/uring-memlock
Sep 26, 2026
Merged

kixelated merged 2 commits into
mainfrom
claude/uring-memlock

Conversation

@kixelated

Copy link
Copy Markdown
Collaborator

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 with prlimit: at a 6 MiB soft limit, not even one worker starts. When a test ran out:

  • on the main thread, Worker::new failed at once with ENOMEM;
  • on a helper thread (the server in teardown, the client in identity, a worker in workers), the test sat for ~8-11s while a dial idled out.

Fix

  • .config/nextest.toml: an io-uring test group caps moq-uring and the relay's uring tests at 4 concurrent.
  • Error::ring: an ENOMEM from ring setup or buf-ring registration now names RLIMIT_MEMLOCK, its current value, and how to raise it. The variant is still Error::Io, with the same ErrorKind.
  • The three tests with helper threads wait for each helper to report in, so a setup failure fails at once.
  • doc/bin/relay/config.md documents the memlock cost of runtime.io_uring workers.

API / wire impact

None. Error::ring is pub(crate). The message of an ENOMEM Error::Io changes.

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.
  • New unit test a_ring_enomem_names_the_memlock_limit.
  • Full just check did not run to completion here because moq-ffi test targets fail to compile on main (E0283 at rs/moq-ffi binary_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

@coderabbitai

coderabbitai Bot commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

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 configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: dda12191-0427-4425-a638-d8e616015481

📥 Commits

Reviewing files that changed from the base of the PR and between e3fce8e and 4375d30.

📒 Files selected for processing (1)
  • doc/bin/relay/config.md
🚧 Files skipped from review as they are similar to previous changes (1)
  • doc/bin/relay/config.md

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review.


Walkthrough

The 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 4375d

The test coordination and error diagnostics appear ready to merge after normal checks. No concrete merge-blocking issue remains.

Security Architecture Review

Security architecture risk: 🔵 Low · up to e3fce

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

  • Low · architecture · observed: Wrapping ring-setup ENOMEM removes its raw operating-system error code from the exported Error::Io value. External callers that use that code for failure handling may no longer recognize it; no such in-repository consumer was identified.
Security review details

Security Blast Radius

  • inferred — The inspected production effect is limited to errors returned on io_uring setup or registration failure. The supplied outside-file name matches do not establish additional reachable consumers.

Trust Boundaries and Controls

  • observed — Worker::new retains its existing classification of unsupported-kernel and blocked-setup errors before the new diagnostic mapping. No new authentication, privilege, or network entrypoint is shown in the changed production ranges.

Hardening Proposals

  • proposed — If callers are expected to recover based on operating-system error codes, preserve the original ENOMEM as the wrapped error source and test the returned raw-error semantics.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the primary change: limiting io_uring tests to respect the shared memlock budget.
Description check ✅ Passed The description directly explains the memlock-related test failures, the concurrency cap, improved ENOMEM diagnostics, test synchronization, documentation updates, and validation results.
Docstring Coverage ✅ Passed Docstring coverage is 85.71% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 6 files. (1 skipped: 1 u…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
✨ Simplify code
  • Commit to this branch
  • Create a new PR

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ⚠️ Failed 2026-09-25T23:16:24.279898Z 4375d30 New commits
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@kixelated

Copy link
Copy Markdown
Collaborator Author

MERGE

This is a positive, well-scoped fix for a real shared-resource failure mode.

Positive improvement? Yes. Parallel moq-uring runs were exhausting the process-wide RLIMIT_MEMLOCK budget (shared with other io_uring users on the machine), which produced ENOMEM on the main thread or ~10s dial timeouts when a helper thread failed to start. The diagnosis and the failure modes match the code changes.

Worth the complexity? Yes. The layers are small and each earns its keep:

  • Cap io_uring-related tests at 4 concurrent via nextest (the right primary fix for a shared kernel budget).
  • Map ring/buf_ring setup ENOMEM through Error::ring so the message names RLIMIT_MEMLOCK and how to raise it.
  • Make helper-thread tests report in before the main thread continues, so setup failures fail immediately instead of looking like stuck dials.
  • Document the memlock cost for relay io_uring workers.

No API/wire surface change beyond a clearer Error::Io message on that path.

Different approach? Raising memlock globally in CI/dev would paper over the limit without explaining it, and fully serializing these tests would be slower than a small concurrent group. Sharing rings across tests would fight the worker model. Capping concurrency plus better errors plus fail-fast helpers is the right mix.

One minor note (not blocking): Error::ring rebuilds the io::Error with Error::new, so raw_os_error() on the wrapped value becomes None even though ErrorKind and the message retain the original. That is fine for diagnostics; only worth revisiting if something downstream still keys off raw_os_error() after this mapping.

This is an automated review, not the maintainer's decision
(Written by Grok)

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines +132 to 133
ready.send(()).expect("test alive");
worker.block_on(stop.wait()).expect("worker loop");

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Comment thread rs/moq-uring/src/error.rs
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=`"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

@kixelated

Copy link
Copy Markdown
Collaborator Author

Heads-up: #4197 halves the io_uring completion queue, so a worker's ring is charged ~56 KiB of RLIMIT_MEMLOCK rather than ~84. The doc/bin/relay/config.md line here saying ~100 KiB, and the ~84 KiB in .config/nextest.toml, should drop to ~56 KiB if #4197 lands first.

(Written by Claude Opus 5.5)

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>
@kixelated

Copy link
Copy Markdown
Collaborator Author

Rebased onto current origin/main (6d637a09d). No conflicts. The earlier Check and Test failure was the moq-ffi E0283 on config.into(), already fixed on main by #4157 (moq_mux::binary::Config::from(config)); this branch does not touch that path.

Landing the shared RLIMIT_MEMLOCK bound as written: nextest caps io_uring tests at 4 concurrent, ring ENOMEM names the limit, and the helper-thread tests fail at setup instead of idling out. The relay doc still says ~100 KiB per ring; #4197 can update that to ~56 KiB when it lands.

(Written by Grok 4.7)

@kixelated
kixelated enabled auto-merge (squash) September 25, 2026 23:19
@kixelated

Copy link
Copy Markdown
Collaborator Author

Remark wanted the new memlock sentence to escape io_uring as io\_uring. That is the only change on top of the rebase (4375d30c9). Squash auto-merge is on; the ~100 KiB ring figure is unchanged.

(Written by Grok 4.7)

@kixelated
kixelated merged commit a05a2d5 into main Sep 26, 2026
3 checks passed
@kixelated
kixelated deleted the claude/uring-memlock branch September 26, 2026 00:03
kixelated added a commit that referenced this pull request Sep 26, 2026
#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>
@moq-bot moq-bot Bot mentioned this pull request Sep 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant