Repository navigation
guest: settle direct-mode execs only once their processes are reaped - #363
Merged
Pedro Henrique Penna (ppenna) merged 1 commit intoOct 5, 2026
Merged
Pedro Henrique Penna (ppenna) merged 1 commit into
Pedro Henrique Penna (ppenna) merged 1 commit into
Conversation
In direct mode, the managed agent runs as PID 1 and contains each workload in the nvx-exec cgroup. After an exec, settle_exec_cgroup() killed the cgroup and returned as soon as cgroup.events reported "populated 0". That field counts only live processes: do_exit() drops a killed task from it in cgroup_exit(), before exit_notify() turns the task into a zombie that PID 1 must reap. A process that passed both points between the agent's last reap and its read of cgroup.events therefore stayed in /proc as an unreaped zombie after the agent had reported the exit. The agent reaps only inside run_exec(), and the next exec's containment pre-check read the same signal, so the next workload started while the zombie was still visible. That is how `pgrep -x sleep` in openvmm_lifecycle_on_a_real_hypervisor printed PID 61 on Linux/MSHV. Every leftover had already been killed, so nothing escaped containment. When nvx-exec lacks pids.current, the agent now enables the pids controller in the root cgroup's cgroup.subtree_control, and it settles on that count instead. The pids controller charges a task from its fork, or its migration into the cgroup, until release_task() frees it, which for a process happens when it is reaped. Once the agent has reaped what it inherited, pids.current therefore reads zero only when every process of the workload has exited and been reaped. Settlement keeps its 2 s budget. It still reports containment-failed when the count stays above zero, when cgroup.kill fails, or when pids.current is missing or malformed. The direct-mode pre-check reaps first and then requires the same zero count. It refuses an exec cgroup that still holds unreaped tasks, while a workload that exited after its settlement timed out no longer blocks later execs as a zombie that nothing reaps. Failing to enable the controller refuses the exec as launch-failed, like the other failures to prepare the cgroup, and the kernel build now requires CONFIG_CGROUP_PIDS=y with the other sandbox options. Replace the fake-cgroup tests of cgroup.events parsing with tests of pids.current parsing, and check that preparation enables the pids controller only while nvx-exec lacks pids.current. A harness whose waitpid() stops charging a process when it reaps it, as the pids controller does, checks that settlement waits for an exiting workload process and reaps it, and that the pre-check reaps an exited one before it counts. A cgroup that reports populated 0 while it still charges a task must not settle. Document the barrier in the agent design. Validation outside CI: - A harness ran the real run_exec() as a child subreaper in a delegated cgroup v2 scope on WSL2 (Linux 6.18.33, 20 CPUs), with 5,000 iterations of each scenario from the issue. The fixed agent left nothing behind in 25,000 iterations. The previous agent left leftovers in 1,925 of them, and the next exec's pgrep found 1,762 of those: 5 leftovers for the L839 workload under load, 4 for it with 500 us between the reap and the read, and 64 and 1,852 for the detached variant, idle and with the gap. - On Windows/WHP with a single-vCPU guest, the aci_edge_sandboxes e2e tests pass. In 10,000 detached execs, each followed by `pgrep -x sleep || echo none`, the fixed agent left nothing behind. With a 500 us sleep injected between the reap and the read, the previous agent left a sleep for pgrep in 662 of 2,000 execs, and the fixed agent left none in 10,000. Fixes #360. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The focused implementation addresses the race with comprehensive regression coverage and consistent documentation.
Review effort: Balanced
Findings: None
What changed in this PR
Fixes #360 by ensuring direct-mode workloads are reaped before an execution is reported complete.
Changes:
- Uses
pids.currentas the settlement and pre-check barrier. - Requires pids-controller kernel support and adds regression tests.
- Updates the managed-agent architecture documentation.
| File | Description |
|---|---|
guest/common/nvx-managed-agent.c |
Enables pids accounting and waits for reaping. |
scripts/test_nvx_tools.py |
Tests parsing, controller setup, reaping, and timeouts. |
scripts/nvx_tools/build_constants.py |
Requires CONFIG_CGROUP_PIDS=y. |
doc/design/sandbox-filesystem-and-agent-architecture.md |
Documents the new settlement barrier. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Pedro Henrique Penna (ppenna)
deleted the
agents/fix-issue-360-validate-implementation
branch
October 5, 2026 01:39
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #360.
In direct mode, the managed agent runs as PID 1 and contains each workload in the
nvx-execcgroup.settle_exec_cgroup()returned as soon ascgroup.eventsreportedpopulated 0. That field drops a killed task incgroup_exit(), beforeexit_notify()turns it into a zombie that PID 1 has to reap. Asleepthat finished exiting between the agent's last reap and its read ofcgroup.eventstherefore stayed in/procas an unreaped zombie after the agent had reported the exit. The next exec's pre-check read the same signal, so the next workload started while the zombie was still visible. That is howpgrep -x sleepinopenvmm_lifecycle_on_a_real_hypervisorprinted61on Linux/MSHV. Nothing escaped containment: every leftover had already been killed.Changes
guest/common/nvx-managed-agent.c): whennvx-exechas nopids.current,prepare_exec_cgroup()enables thepidscontroller in the root cgroup'scgroup.subtree_control. Settlement now reaps and then waits untilpids.currentreads0. The pids controller charges a task untilrelease_task()frees it, which for a process happens when it is reaped. A zero count therefore means that every workload process has exited and been reaped. The 2 s budget and thecontainment-failedoutcome are unchanged, and a missing or malformedpids.currentalso reportscontainment-failed.CONFIG_CGROUP_PIDS=yjoinsREQUIRED_SANDBOX_CONFIG, andkernel/config-microvmalready sets it. The guard changes the kernel cache key, so CI rebuilds the kernel once for this PR.scripts/test_nvx_tools.py): the fake-cgroup tests now cover:pids.currentparsing;pids.currentis missing;waitpid()drops the charge on reap, as the kernel does;populated 0while it still charges a task does not settle.The e2e
pgrepassertions inopenvmm_e2e.rsstay unchanged.Validation
Stress harness with the real
run_exec()(WSL2, Linux 6.18.33, 20 CPUs)The harness
#includes the agent and drives its realrun_exec()in direct mode. It runs as a child subreaper, standing in for PID 1, in a delegated cgroup v2 scope, and doubles as thesetprivshim and the launch helper. After each detached exec, it lists its own children and readspids.current. It then runspgrep -x -P <harness> sleep || echo nonethroughrun_exec(). Each scenario ran 5,000 iterations:>/dev/null), idleIn every scenario, the iterations with leftovers were exactly those in which
pids.currentwas nonzero when the exec reported its exit. The sampled leftovers were zombies (ZwithPF_EXITING). No live process outlived an exec in either build, and the fix caused no settlement timeouts.Real hypervisor (Windows/WHP, single-vCPU guest)
The kernel and OpenVMM (
4355c010) come from a dev release whose kernel provenance matches this branch's kernel inputs. The Alpine initramfs was built from this branch.python scripts\nvx.py test-aci-edge-sandboxes --backend whp: all 7 tests pass, includingopenvmm_lifecycle_on_a_real_hypervisorandenvironments_leave_the_workload_contained.pgrep -x sleep || echo none:Local checks
ruff check,ruff format --check, andpyrightfor Linux and Windows pass.validate-nvxunit tests pass on Windows and on Linux (551 tests, run in WSL from a shallow clone of this commit).-static -Os -s -Wall -Wextra -Werror).I did not run anything locally on Linux/KVM or Linux/MSHV hardware. CI covers both.
Notes
nvx-managed-agentto Rust, should carry this barrier over instead of waiting for the cgroup to empty.Harness:
settle_stress.cRunner:
run.shandscenario.sh