feat(guest): raise spawned-process RLIMIT_NOFILE with a sane default and env override#826
feat(guest): raise spawned-process RLIMIT_NOFILE with a sane default and env override#826stubbi wants to merge 3 commits into
Conversation
…and env override A bare Linux init inherits a low soft RLIMIT_NOFILE (commonly 1024), and the guest agent (PID 1) passed that limit to every exec, PTY, run_code kernel, and serving-workload child. Data libraries that open many files at import time (pandas, openpyxl, scikit) then die with EMFILE, the exact microsandbox failure (issue #786). Host-side caps (husk pod cgroup, per-sandbox stream caps) do not govern in-guest per-process rlimits; only setrlimit inside the guest does. New sys/rlimit.rs registers an async-signal-safe pre_exec hook that raises the child's soft RLIMIT_NOFILE to a documented default of 65536, clamped to the inherited hard limit and never lowered below the current soft. An operator override is read from the MITOS_RLIMIT_NOFILE environment variable (a decimal file count), read once in the parent so the post-fork closure only performs getrlimit, pure integer arithmetic, and setrlimit. The limit computation and override parsing are pure and unit-tested on any host; the end-to-end proof (a spawned guest process observing the raised limit) is KVM gated. The hook is wired into all four guest spawn paths: exec, exec-stream, PTY, the run_code Python kernel driver, and the serving-workload supervisor. Docs cover the default, the env seam, and the scoped follow-up to plumb a first-class template.spec field into that boot env. Refs #786 Signed-off-by: Jannes Stubbemann <jannes@openclaw.rocks>
|
Warning Review limit reached
Next review available in: 2 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds a Rust module that raises the guest agent’s soft ChangesGuest RLIMIT_NOFILE raising
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@guest/agent-rs/src/sys/rlimit.rs`:
- Around line 209-254: The test spawned_child_sees_raised_soft_nofile mutates
the process-wide RLIMIT_NOFILE and can race with other parallel tests; update
this test to run in isolation. Use an isolation mechanism such as #[serial] on
spawned_child_sees_raised_soft_nofile or otherwise ensure the rlimit.rs test
suite runs single-threaded, while keeping the existing apply_nofile_limit and
resolve_nofile_soft behavior intact.
- Around line 38-51: The NOFILE override path currently accepts 0 and other tiny
values, which can be applied directly by apply_nofile_limit_inner and disable
file descriptors for spawned children. Update parse_nofile_override and/or
resolve_nofile_soft to enforce a documented minimum floor before returning a
value, and keep invalid/too-small overrides falling back to DEFAULT_NOFILE_SOFT.
Add a unit test around parse_nofile_override or resolve_nofile_soft to cover the
boundary case for zero and other sub-minimum inputs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 95d5df30-c762-49ae-97f2-ba5de64de920
📒 Files selected for processing (6)
docs/templates.mdguest/agent-rs/src/kernel/driver.rsguest/agent-rs/src/service/exec.rsguest/agent-rs/src/service/workload.rsguest/agent-rs/src/sys/mod.rsguest/agent-rs/src/sys/rlimit.rs
…nner The guest-agent CI job built the Rust agent but never ran cargo test, so the new RLIMIT_NOFILE unit tests and the Linux-gated integration test that asserts a spawned child's soft limit is actually raised had no automated coverage. Add a scoped `cargo test --lib sys::rlimit` step to the existing firecracker-test job so both run on a real Linux host in a required check. Scoped to the module so it stays deterministic and needs neither the booted-VM nor Python-kernel fixtures the other guest tests require. Refs #786 Signed-off-by: Jannes Stubbemann <jannes@openclaw.rocks>
…ion test Address two review findings on the rlimit module: - A degenerate override (MITOS_RLIMIT_NOFILE=0 or a tiny typo) was applied verbatim, which would set every spawned child's soft limit that low and recreate the EMFILE-on-everything failure this module exists to prevent. Add a MIN_NOFILE_SOFT floor of 64: values below it (or empty or unparseable) are ignored with a warning and the sane default applies. New unit tests cover the zero and sub-floor boundary. - The integration test lowered and restored the test binary's own process-wide RLIMIT_NOFILE, which could race with other parallel tests. Rewrite it to read the limits read-only and assert the spawned child's soft limit equals resolve_nofile_soft of this process's actual limits, touching no shared state. Refs #786 Signed-off-by: Jannes Stubbemann <jannes@openclaw.rocks>
|
Reviewed in full. The code is correct and well-tested: the pre_exec closure keeps the async-signal-safety contract (env read hoisted to the parent, pure integer clamp math post-fork), RLIM_INFINITY and the never-lower-soft arm are handled, hook ordering with the setsid hook is fine, and the new CI step closes a real gap (the guest-agent job previously built but never tested). Three optional nits, none blocking: (1) an invalid MITOS_RLIMIT_NOFILE warns on every spawn; a once-flag would silence the repeat spam on the hot exec path. (2) the Linux integration test applies the env-honoring path but asserts against resolve_nofile_soft(..., None), so a stray MITOS_RLIMIT_NOFILE in the CI environment would fail it spuriously. (3) docs say to set the variable "in the guest agent's environment" but nothing plumbs it there short of hand-editing kernel boot args; the template.spec follow-up is named, so just soften that sentence. This touches guest/agent-rs, a named-human-reviewer path per CLAUDE.md, so it is staged rather than merged: green CI plus this review plus your ack and it goes in. |
1 participant
Thinking Path
Linked Issues or Issue Description
template.specfield is deferred as a named follow-up (see What Changed).What Changed
guest/agent-rs/src/sys/rlimit.rs: pure, unit-testedparse_nofile_overrideandresolve_nofile_softplus an async-signal-safepre_exechook (apply_nofile_limit) that raises the child's softRLIMIT_NOFILE. Linux-only syscall body; a no-op on the darwin developer build, matchingsys/pty.rs.setrlimitcannot fail on the value) and never lowered below the current soft. The hard limit is never raised (that needsCAP_SYS_RESOURCE, out of scope).MITOS_RLIMIT_NOFILEenvironment variable, read once per spawn in the parent so the post-fork closure allocates nothing; an unparseable value is ignored with a warning and the default applies.service/exec.rs(exec, exec-stream, PTY),kernel/driver.rs(the run_code Python kernel), andservice/workload.rs(the serving-workload supervisor). The exec and driver tokio builders were refactored to build astd::process::Command, register the hook, then convert withtokio::process::Command::from, keeping allunsafeinsidesys/(the same pattern the PTY path already uses).docs/templates.md: a new "Guest process resource limits" section documenting the default, the env seam, and a follow-up bullet to plumb a first-classtemplate.specfield (for exampleresources.rlimits.nofile) through CreateTemplate and Fork into the guest boot env.Verification
cargo testinguest/agent-rs: the 7 newsys::rlimitunit tests pass on darwin (parse accept/reject, default raise/clamp/no-lower, override honor/clamp/lower). Pre-existing darwin-only failures insys::signal(needs Linux/procSigCgt) andkernel::driverruncode (needs a Python ipykernel) are unrelated to this change and are in files this PR does not modify.cargo buildinguest/agent-rs: clean (only pre-existing dead_code warnings infork/network.rsandsys/netlink.rs).cargo clippy --all-targets: zero warnings attributable to the changed files; all reported warnings are pre-existing dead_code / unused-import findings elsewhere.cargo fmt --check: the newsys/rlimit.rsis fmt-clean; the repo-wide diff is a toolchain-version skew (local rustfmt 1.8.0 reformats every checked-in file, including untouched ones), not introduced here.cargo test --lib sys::rlimitstep to the existing guest-agent job inkvm-test.yaml, so the 7 unit tests AND the Linux-gated integration testspawned_child_sees_raised_soft_nofile(spawns/bin/sh -c 'ulimit -Sn'and asserts the child's soft limit is actually raised) now run on a real Linux host in required CI. That integration test is compiled out on the darwin developer host, which is why it could not run in my localcargo test.Risks
setrlimitcannot fail on the value and no spawn path can regress into a spawn failure. The change is confined to the guest agent and does not move the host or forkd escape surface: an in-guest process opening more descriptors consumes only its own VM's kernel memory, bounded by the VM's fixed memory budget, and cannot affect other tenants (separate VMs) or the host. No threat-model delta is required for that reason. No fork-correctness hazard: rlimits are per-process and set deterministically at spawn, touching no RNG, clock, or secret inheritance.Model Used
Checklist
#NNN/ mitos-run/mitos URLs)Signed-off-bytrailer (git commit -s)🤖 Generated with Claude Code
Summary by CodeRabbit