From 878906aa2df33acdc759753403afd142ae80f725 Mon Sep 17 00:00:00 2001 From: Enrique Saurez Date: Sat, 24 Oct 2026 00:22:50 -0700 Subject: [PATCH 1/3] tests: settle the runtime adapter's executions through cgroups The worker ran each program in a process group of its own, which it killed before it collected the program's status. Nothing proved that a descendant that left the group was gone, so every outcome was fenced, and the worker could admit a new execution while such a descendant still ran. Each execution now gets a cgroup of its own below the workload cgroup, which the worker prepares with ExecutionCgroup::prepare, and which the program joins as it spawns. The execution settles through that cgroup alone, so the other executions keep running, and an execution whose program ends on its own is reusable. An execution keeps its place among the worker's limit until its cgroup is empty and removed. One that does not settle keeps its place, and its cgroup, until the host stops the worker, which ends and removes every cgroup that is left. The root's operations name the cgroups that the bootstrap prepared. A fake root names none where the in-process tests cannot create cgroups, and the programs keep their process groups there, as before. Where the tests can, each fake root has a workload cgroup of its own, and new tests check a descendant in a session of its own, which the settlement ends, and an execution that does not settle, which keeps its place until the stop. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a7ad408a-da3b-45c4-aa02-5d25300b1c90 --- tests/runtime-adapter/README.md | 95 ++++-- tests/runtime-adapter/src/worker.rs | 86 +++-- .../runtime-adapter/src/worker/executions.rs | 320 +++++++++++++----- tests/runtime-adapter/tests/guest_session.rs | 259 +++++++++++++- 4 files changed, 615 insertions(+), 145 deletions(-) diff --git a/tests/runtime-adapter/README.md b/tests/runtime-adapter/README.md index 1fd5644c8..fb1173945 100644 --- a/tests/runtime-adapter/README.md +++ b/tests/runtime-adapter/README.md @@ -522,28 +522,48 @@ credit that it needs, never keeps the others' output from the host. runs until it ends, detached from the host, which receives nothing more about it. - The worker admits at most `worker::MAX_EXECUTIONS`, 8, executions at once, - abandoned ones included, and those whose programs' statuses it has not - collected, and none that would hold more than + abandoned ones included, and those that keep their places once they ended, + as the items below describe, and none that would hold more than `worker::MAX_EXECUTION_MEMORY`, 1 MiB: its runner's buffers, a chunk of each output stream and the input's window. It refuses a start beyond those, or - one whose program it cannot spawn, with a terminal record whose launch failed - at stage 0, with `EAGAIN`, `ENOMEM`, or the spawn's `errno`. + one whose cgroup it cannot prepare, or whose program it cannot spawn, with a + terminal record whose launch failed at stage 0, with `EAGAIN`, `ENOMEM`, or + the `errno` of the preparation or the spawn. - Each program runs in the worker's own root and working directories, with its environment, not in the layered root, with its standard input from the host - or the null device, and leads a process group of its own, which its - execution kills as a whole. -- The worker collects the status of every program itself, without waiting for - it, and learns that a program ended from a pidfd of its own. Before it - collects the status, it kills what is left of the program's process group, - as NVX's settlement through a cgroup kills what is left in the cgroup: until - then, the program, a zombie, keeps its process ID, which is the group's, from - any other process. So a descendant that stayed in the group neither outlives - the program nor holds its output open. The program runs without a cgroup, - though, so nothing proves that a descendant that left the group is gone, and - each outcome is fenced: once the program ended, a stream that such a process - may still hold has the settlement time and the grace to close, after which - the runner gives up on it, while a stream that no process holds any more - loses nothing, however slowly the host takes it. + or the null device, and leads a process group of its own. +- Each execution runs in a cgroup of its own below the workload cgroup of the + root configuration, `execution-N`, where N counts the cgroups that the worker + prepared. The worker prepares it with + `nvx::guest::cgroup::ExecutionCgroup::prepare`, and the program joins it as + it spawns, before it runs, so that its descendants start in it, however they + leave its process group or session. The execution settles through that + cgroup alone, which leaves the other executions running: once the program + ended, as its status, which the execution collects itself, tells, the + execution kills what is left in the cgroup, and settles once the cgroup is + empty. So no descendant outlives the program or holds its output open, and + an execution whose program ends on its own loses nothing, however slowly the + host takes its output, and is reusable. +- An execution keeps its place among `worker::MAX_EXECUTIONS` until its cgroup + is empty and the worker removed it, which the worker does once the runner + returned the terminal outcome of an execution that settled. One that did not + settle, whose outcome is fenced, keeps its place and its cgroup until the + host stops the worker. +- Without cgroups, as where the in-process tests cannot create them, the + root's operations name none, and the program's process group stands in for + its cgroup: its execution kills the group as a whole. The worker collects the + status of every program itself then, without waiting for it, and learns that + a program ended from a pidfd of its own. Before it collects the status, it + kills what is left of the program's process group, as the settlement through + a cgroup kills what is left in the cgroup: until then, the program, a + zombie, keeps its process ID, which is the group's, from any other process. + So a descendant that stayed in the group neither outlives the program nor + holds its output open. Nothing proves that a descendant that left the group + is gone, though, so each outcome is fenced: once the program ended, a stream + that such a process may still hold has the settlement time and the grace to + close, after which the runner gives up on it, while a stream that no process + holds any more loses nothing, however slowly the host takes it. Such an + execution keeps its place until the worker collected its program's status. - Before it hands an execution the host's cancellation, the worker gives the execution a turn, as the loop does, which collects the status of a program that ended, and reports the program's end to the runner. So the end of a @@ -554,11 +574,13 @@ credit that it needs, never keeps the others' output from the host. - The worker forgets an execution once its runner returned the terminal outcome, which may come before the program's end, as once the runner gave up on a program that a kill did not end in time: forgetting it kills the - program and its process group again, and the worker collects the program's - status once it ended. On a stop, or once its session ended otherwise, the - worker drops every execution, which kills the process group of each program - whose status it has not collected, and waits up to a second in all for those - programs to end. + program, and what is left in its cgroup or its process group, again, and the + worker collects the program's status once it ended. On a stop, or once its + session ended otherwise, the worker drops every execution, which kills the + program of each one that has not ended, with what is left in its cgroup or + its process group, and what is left in the cgroups of the executions that did + not settle, and waits up to a second in all for those programs to end and + those cgroups to empty, removing each cgroup that emptied. The worker's `entries` probe runs two more commands of the guest agent: `runtime-adapter-guest entries ROOT CGROUP`, its helper, which runs the probe's @@ -1094,8 +1116,14 @@ the binary exited, so that none of them outlives the test or holds its output open, which tests check too. `tests/guest_session.rs` also runs the worker's executions behind its test -broker, whose host side starts the sandbox, then drives the engine's peers, -with frame-counted credit and one output pool: +broker, whose host side starts the sandbox, then drives the engine's peers, with +frame-counted credit and one output pool. Where the tests can create cgroups, as +root in a cgroup v2 hierarchy with the `pids` controller, the fake root gives +the worker a workload cgroup of the test's own, below which each execution runs +in a cgroup of its own, so that an execution whose program ends on its own +settles, and is reusable. Elsewhere, the fake root names no cgroups, and each +program runs in a process group alone, so that such an execution is fenced. The +tests cover: - three executions at once, each byte-exact: standard output beyond a MiB, 1.5 MiB of input through `cat`, and both streams of a program that fails; @@ -1125,9 +1153,20 @@ with frame-counted credit and one output pool: statuses, and a program whose status the worker collects once it ended, after its execution's runner gave up on it; - programs that end while a descendant holds their output: the worker ends - the descendant, so that the output ends at once, losing nothing, and a stop - once the worker collected such a program's status leaves no process of its - group running; + the descendant, through the execution's cgroup or the program's process + group, so that the output ends at once, losing nothing, and a stop once the + worker collected such a program's status leaves no process of its group + running; +- with cgroups, a descendant in a session of its own, which holds a program's + output once the program ended, and which the execution's settlement ends, so + that the output ends at once, losing nothing, and the execution settles; + the worker removed the execution's cgroup before it sent the outcome, and + runs its limit of executions at once again; +- with cgroups, an execution that does not settle within its settlement time, + because a zombie whose parent is outside its cgroup occupies the cgroup: it + keeps its place, so that the worker runs one execution fewer than its limit, + even once the cgroup emptied, until the host stops the worker, which then + removes the cgroup; - a program that a real-time signal ends, whose status the worker collects; - an execution whose start comes in the payload of the root's configuration and the sandbox's start, whose timeout counts from its own start, however diff --git a/tests/runtime-adapter/src/worker.rs b/tests/runtime-adapter/src/worker.rs index f6ecf540c..de7bedfc1 100644 --- a/tests/runtime-adapter/src/worker.rs +++ b/tests/runtime-adapter/src/worker.rs @@ -114,22 +114,40 @@ //! session abandons every execution: the worker drops it, and it runs until it ends, detached //! from the host, which receives nothing more about it. //! - The worker admits at most [`MAX_EXECUTIONS`] executions at once, abandoned ones included, -//! and those whose programs' statuses it has not collected, and none that would hold more than -//! [`MAX_EXECUTION_MEMORY`] bytes. It refuses a start beyond those, or one whose program it -//! cannot spawn, with a terminal record whose launch failed at stage 0, with `EAGAIN`, -//! `ENOMEM`, or the spawn's `errno`. +//! and those that keep their places once they ended, as the items below describe, and none +//! that would hold more than [`MAX_EXECUTION_MEMORY`] bytes. It refuses a start beyond those, +//! or one whose cgroup it cannot prepare, or whose program it cannot spawn, with a terminal +//! record whose launch failed at stage 0, with `EAGAIN`, `ENOMEM`, or the `errno` of the +//! preparation or the spawn. //! - Each program runs in the worker's root and working directories, with its environment, and -//! leads a process group of its own, which its execution kills as a whole. -//! - The worker collects the status of every program itself, without waiting for it, and learns -//! that a program ended from a pidfd of its own. Before it collects the status, it kills what -//! is left of the program's process group, as NVX's settlement through a cgroup kills what is -//! left in the cgroup: until then, the program, a zombie, keeps its process ID, which is the -//! group's, from any other process. So a descendant that stayed in the group neither outlives -//! the program nor holds its output open. The program runs without a cgroup, though, so nothing -//! proves that a descendant that left the group is gone, and each outcome is fenced: once the -//! program ended, a stream that such a process may still hold has the settlement time and the -//! grace to close, after which the runner gives up on it, while a stream that no process holds -//! any more loses nothing, however slowly the host takes it. +//! leads a process group of its own. +//! - Each execution runs in a cgroup of its own below the workload cgroup, `execution-N`, where N +//! counts the cgroups that the worker prepared. The worker prepares it with +//! `nvx::guest::cgroup::ExecutionCgroup::prepare`, and the program joins it as it spawns, +//! before it runs, so that its descendants start in it, however they leave its process group or +//! session. The execution settles through that cgroup alone, which leaves the other executions +//! running: once the program ended, as its status, which the execution collects itself, +//! tells, the execution kills what is left in the cgroup, and settles once the cgroup is empty. +//! So no descendant outlives the program or holds its output open, and an execution whose +//! program ends on its own loses nothing, however slowly the host takes its output, and is +//! reusable. +//! - An execution keeps its place among [`MAX_EXECUTIONS`] until its cgroup is empty and the +//! worker removed it, which the worker does once the runner returned the terminal outcome of an +//! execution that settled. One that did not settle, whose outcome is fenced, keeps its place +//! and its cgroup until the host stops the worker. +//! - Without cgroups, as where the in-process tests cannot create them, the [root's +//! operations](RootOperations::cgroups) name none, and the program's process group stands in +//! for its cgroup: its execution kills the group as a whole. The worker collects the status of +//! every program itself then, without waiting for it, and learns that a program ended from a +//! pidfd of its own. Before it collects the status, it kills what is left of the program's +//! process group, as the settlement through a cgroup kills what is left in the cgroup: until +//! then, the program, a zombie, keeps its process ID, which is the group's, from any other +//! process. So a descendant that stayed in the group neither outlives the program nor holds its +//! output open. Nothing proves that a descendant that left the group is gone, though, so each +//! outcome is fenced: once the program ended, a stream that such a process may still hold has +//! the settlement time and the grace to close, after which the runner gives up on it, while a +//! stream that no process holds any more loses nothing, however slowly the host takes it. Such +//! an execution keeps its place until the worker collected its program's status. //! - Before it hands an execution the host's cancellation, the worker gives the execution a turn, //! as the loop does, which collects the status of a program that ended, and reports the //! program's end to the runner. So the end of a program that ended before its timeout while the @@ -138,11 +156,13 @@ //! timeout's instant. //! - The worker forgets an execution once its runner returned the terminal outcome, which may //! come before the program's end, as once the runner gave up on a program that a kill did not -//! end in time: forgetting it kills the program and its process group again, and the worker -//! collects the program's status once it ended. On a stop, or once its session ended -//! otherwise, the worker drops every execution, which kills the process group of each program -//! whose status it has not collected, and waits up to a second in all for those programs to -//! end. +//! end in time: forgetting it kills the program, and what is left in its cgroup or its process +//! group, again, and the worker collects the program's status once it ended. On a stop, or +//! once its session ended otherwise, the worker drops every execution, which kills the program +//! of each one that has not ended, with what is left in its cgroup or its process group, and +//! what is left in the cgroups of the executions that did not settle, and waits up to a second +//! in all for those programs to end and those cgroups to empty, removing each cgroup that +//! emptied. mod connection; pub mod entries; @@ -200,7 +220,9 @@ pub const SESSION_STATUS: u8 = 1; pub const EXECUTING: &str = "the worker runs executions, and probes nothing until they end"; /// The most executions that the worker runs at once, those that the host abandoned included, and -/// those whose programs the worker forgot before it collected their statuses. +/// those that ended but keep their places: with cgroups, those whose cgroups the worker has not +/// removed, as those of the executions that did not settle, and without them, those whose +/// programs the worker forgot before it collected their statuses. pub const MAX_EXECUTIONS: usize = 8; /// The most bytes that one execution may hold in the worker: what its runner holds, a chunk of @@ -242,6 +264,12 @@ pub trait RootOperations { /// Fails with a [`MapError`] that counts the mappings before the one that failed. fn map(&mut self, mappings: &[Mapping]) -> Result<(), MapError>; + /// Returns the cgroups in which the worker runs its executions once the root is ready: the + /// layout that the bootstrap prepared, below whose workload cgroup each execution gets a + /// cgroup of its own. Without them, as in tests that cannot create cgroups, each program runs + /// in a process group alone. + fn cgroups(&self) -> Option; + /// Runs `probe` on the assembled root, and returns what it found. fn probe(&mut self, probe: &Probe) -> Result; @@ -337,7 +365,7 @@ pub fn work( message, )?, None => { - let (step, ready) = configure_root(operations, message)?; + let (step, ready) = configure_root(operations, &mut executions, message)?; order = ready; step } @@ -436,15 +464,18 @@ fn wait( } /// Handles `message` before the root is ready: a root configuration, whose root it assembles -/// through `operations`, and which it answers with its readiness, or a `Stop`. Returns the step, -/// with the worker's order once the root is ready. +/// through `operations`, after which `executions` run in the cgroups that the operations prepared, +/// and which it answers with its readiness, or a `Stop`. Returns the step, with the worker's order +/// once the root is ready. fn configure_root( operations: &mut dyn RootOperations, + executions: &mut Executions, message: Message, ) -> Result<(Step, Option), ServeError> { match message { Message::RootConfig(body) => Ok(match assemble(operations, &body) { Ok(config) => { + executions.run_in(operations.cgroups()); let ready = Ready::new(GUEST_BUILD).expect("this crate's version is a build"); let step = Step::Continue(vec![Message::Ready(ready.encode())]); (step, Some(Order::new(config.mapping_count))) @@ -750,6 +781,13 @@ impl RootOperations for LibraryRoot { Ok(()) } + /// Returns the cgroups that the bootstrap prepared: those of the root configuration. + fn cgroups(&self) -> Option { + self.bootstrap + .as_ref() + .map(|bootstrap| bootstrap.layout().clone()) + } + /// Runs one of the [`probes`], of the [`mapping`] probes, which inspect the root's mappings /// and map from the host export, or of the [`entries`] probes, which enter it. fn probe(&mut self, probe: &Probe) -> Result { diff --git a/tests/runtime-adapter/src/worker/executions.rs b/tests/runtime-adapter/src/worker/executions.rs index dfae13982..627a31735 100644 --- a/tests/runtime-adapter/src/worker/executions.rs +++ b/tests/runtime-adapter/src/worker/executions.rs @@ -1,8 +1,10 @@ //! The worker's executions, as the worker's module documentation describes them: the table that //! reaches each one by its ID, the generations that the host started, their admission, the -//! refusal of a start, the loop's turns at every execution, the collection of each program's -//! status, before which the worker ends what is left of the program's process group, and that of -//! the statuses of the programs that the worker forgot. +//! refusal of a start, the loop's turns at every execution, the cgroup of each execution, through +//! which it settles, and which keeps its place until the worker removes it, and, without +//! cgroups, the collection of each program's status, before which the worker ends what is left +//! of the program's process group, and that of the statuses of the programs that the worker +//! forgot. use std::collections::{BTreeMap, VecDeque}; use std::ffi::CString; @@ -17,6 +19,7 @@ use nix::poll::{self, PollFd, PollFlags, PollTimeout}; use nix::sys::signal::{self, Signal}; use nix::sys::wait::{self, WaitPidFlag, WaitStatus}; use nvx::exec::{Key, LaunchFailure, Runner, Settlement, Stream, Violation, runner}; +use nvx::guest::cgroup::{ExecutionCgroup, Layout, Scope}; use nvx::guest::exec::binding::{Binding, Event}; use nvx::guest::exec::{Execution, Options, Pipes, Source}; use nvx::guest::process::Child; @@ -56,6 +59,15 @@ pub(super) struct Executions { /// When the worker stops waiting for the programs once it dropped every execution: a while /// after it first did. cleared: Option, + /// The cgroups in which the executions run, once the root is ready: the layout below whose + /// workload cgroup each execution gets a cgroup of its own. Without them, each program runs + /// in a process group alone. + cgroups: Option, + /// How many cgroups of executions the worker prepared, which numbers the next one. + prepared: u64, + /// The cgroups of the executions that did not settle, each of which keeps its place among + /// [`MAX_EXECUTIONS`] until the worker drops every execution. + fenced: Vec, /// The ID from which the next turn serves the executions that the host reaches, before /// those of lower IDs: the ID after that of the execution that the last turn served first, /// so that the executions take turns at being served first. @@ -73,18 +85,25 @@ pub(super) struct Executions { buffer: Vec, } -/// One of the worker's executions: the binding of its runner to its program, which leaves the -/// program's status to the worker, and the worker's own handle of the program. +/// One of the worker's executions: the binding of its runner to its program, and either the +/// execution's cgroup, through which it settles, or, without cgroups, the worker's own handle of +/// the program, whose status the binding leaves to the worker. struct Run { /// The binding of the execution's runner to its program. binding: Binding, - /// The program, through a pidfd of the worker's own, which polls readable once the program - /// ended, until the worker collected its status. + /// Without cgroups, the program, through a pidfd of the worker's own, which polls readable + /// once the program ended, until the worker collected its status. program: Option, + /// The execution's cgroup, if the worker runs it in one. + cgroup: Option, + /// Whether the runner's terminal outcome tells that the execution settled, so that its + /// cgroup is empty, and the worker may remove it. + settled: bool, } impl Executions { - /// Returns no executions. + /// Returns no executions, which run in process groups alone until + /// [`run_in`](Self::run_in) names their cgroups. pub(super) fn new() -> Self { Self { attached: BTreeMap::new(), @@ -92,6 +111,9 @@ impl Executions { reaping: Vec::new(), reaped: None, cleared: None, + cgroups: None, + prepared: 0, + fenced: Vec::new(), turn: 0, owed: None, generations: BTreeMap::new(), @@ -100,6 +122,12 @@ impl Executions { } } + /// Runs the executions that start from now on in `cgroups`: each in a cgroup of its own, + /// below the workload cgroup, or, without cgroups, in a process group alone. + pub(super) fn run_in(&mut self, cgroups: Option) { + self.cgroups = cgroups; + } + /// Hands `message` from the host to its execution at `now`, and queues on `connection` the /// terminal record of a start that the worker refuses. Before a cancellation, it gives the /// execution a turn at `now`, and queues its messages on `connection`: the end of a program @@ -142,8 +170,9 @@ impl Executions { /// Takes a turn at `now`: collects the status of each program that ended, takes every step /// that the executions can take, queues their messages for the host on `connection`, - /// forgets each execution once it returned its terminal outcome, and collects the statuses - /// of the programs that it forgot, of those that ended. + /// forgets each execution once it returned its terminal outcome, which removes the cgroup of + /// one that settled, and collects the statuses of the programs that it forgot, of those that + /// ended. /// /// The executions that the host reaches take turns at being served first: a turn starts with /// the execution after the one that the last turn started with, in the order of their IDs, @@ -162,7 +191,6 @@ impl Executions { let Self { attached, detached, - reaping, turn, owed, buffer, @@ -184,17 +212,32 @@ impl Executions { *turn = first.wrapping_add(1); } *owed = more.then_some(now); - attached.retain(|_, run| running(run, reaping)); for run in detached.iter_mut() { let key = run.binding.runner().key(); while let Some(event) = run.binding.next(now, buffer) { - // A detached runner returns only its terminal outcome, which no host takes. - if !matches!(event, Event::Message(_)) { - report(key, &event); + match event { + // A detached runner returns only its terminal outcome, which no host takes. + Event::Message(runner::Message::Terminal(outcome)) => { + run.settled = outcome.is_reusable(); + } + Event::Message(_) => {} + event => report(key, &event), } } } - detached.retain(|run| running(run, reaping)); + let finished = attached + .iter() + .filter(|(_, run)| run.binding.is_finished()) + .map(|(&id, _)| id) + .collect::>(); + let mut forgotten = finished + .into_iter() + .filter_map(|id| attached.remove(&id)) + .collect::>(); + forgotten.extend(detached.extract_if(.., |run| run.binding.is_finished())); + for run in forgotten { + self.forget(run); + } self.reap(now); Ok(()) } @@ -208,29 +251,41 @@ impl Executions { } /// Returns whether the worker runs no execution: none that the host reaches, and none that it - /// abandoned. It may still collect the statuses of programs that it forgot. + /// abandoned. It may still collect the statuses of programs that it forgot, and hold the + /// cgroups of executions that did not settle. pub(super) fn is_empty(&self) -> bool { self.attached.is_empty() && self.detached.is_empty() } - /// Drops every execution, which ends the program and its process group of each one whose - /// program's status the worker has not collected, and waits for those programs to end, so - /// that the worker collects their statuses, at most [`REAP_WAIT`] after it first dropped - /// every execution: a later call, as once the executions drop after a stop, waits no longer. - /// The worker ended the process groups of the others when it collected their programs' - /// statuses. + /// Drops every execution, which ends the program and its process group, or the processes of + /// its cgroup, of each one that has not ended, and waits until the programs ended and their + /// cgroups emptied, so that the worker collects the programs' statuses and removes the + /// cgroups, those of the executions that did not settle included, whose processes it ends + /// too, at most [`REAP_WAIT`] after it first dropped every execution: a later call, as once + /// the executions drop after a stop, waits no longer. Without cgroups, the worker ended the + /// process groups of the other programs when it collected their statuses. pub(super) fn clear(&mut self) { - let Self { - attached, - detached, - reaping, - .. - } = self; - for run in std::mem::take(attached) + let runs = std::mem::take(&mut self.attached) .into_values() - .chain(detached.drain(..)) - { - forget(&run, reaping); + .chain(self.detached.drain(..)) + .collect::>(); + for run in runs { + let Run { + binding, cgroup, .. + } = run; + if binding.execution().status().is_none() { + self.reaping.push(binding.id()); + } + self.fenced.extend(cgroup); + } + // An execution that settled gave its cgroup back, which its drop then leaves alone. + for cgroup in &self.fenced { + if let Err(error) = cgroup.kill(Scope::Execution) { + eprintln!( + "runtime-adapter-guest: cannot end the processes of {}: {error}", + cgroup.path().display() + ); + } } let deadline = *self .cleared @@ -238,7 +293,8 @@ impl Executions { loop { let now = Instant::now(); self.reap(now); - if self.reaping.is_empty() || now >= deadline { + self.fenced.retain(|cgroup| !removed(cgroup)); + if (self.reaping.is_empty() && self.fenced.is_empty()) || now >= deadline { return; } thread::sleep(REAP_INTERVAL); @@ -292,6 +348,51 @@ impl Executions { self.attached.values().chain(&self.detached) } + /// Returns how many of the places of [`MAX_EXECUTIONS`] the executions take: one for each + /// execution that the worker runs, and, with cgroups, one for each cgroup of an execution that + /// did not settle, or, without them, one for each program that the worker forgot before it + /// collected its status. + fn places(&self) -> usize { + let held = if self.cgroups.is_some() { + self.fenced.len() + } else { + self.reaping.len() + }; + self.attached.len() + self.detached.len() + held + } + + /// Forgets `run`, whose runner returned its terminal outcome. Unless the worker collected + /// the program's status, the program's ID goes to `reaping`, so that the worker collects the + /// status once the program ended. The worker removes the cgroup of an execution that settled, + /// which frees its place, and keeps that of one that did not, or that it cannot remove, which + /// keeps its place until the worker drops every execution. The binding drops last, which ends + /// a program that has not ended, and its process group, or the processes of its cgroup unless + /// the execution settled: one that settled gave its cgroup back, which its drop leaves alone. + fn forget(&mut self, run: Run) { + let Run { + binding, + cgroup, + settled, + .. + } = run; + if binding.execution().status().is_none() { + self.reaping.push(binding.id()); + } + let Some(cgroup) = cgroup else { + return; + }; + if settled { + match cgroup.remove() { + Ok(()) => return, + Err(error) => eprintln!( + "runtime-adapter-guest: execution {}: cannot remove its cgroup: {error}", + name(binding.runner().key()) + ), + } + } + self.fenced.push(cgroup); + } + /// Collects at `now` the statuses of the programs that the worker forgot, of those that /// ended. fn reap(&mut self, now: Instant) { @@ -339,25 +440,56 @@ impl Executions { self.detached.push(abandoned); } let runner = Runner::new(key, start.limits, start.timers, now); - let executions = self.attached.len() + self.detached.len() + self.reaping.len(); - let admitted = if executions >= MAX_EXECUTIONS { + let admitted = if self.places() >= MAX_EXECUTIONS { Err(Errno::EAGAIN as i32) } else if runner.memory_bound() > MAX_EXECUTION_MEMORY { Err(Errno::ENOMEM as i32) } else { - spawn(&start, &mut self.reaping).map_err(|error| error.raw_os_error().unwrap_or(0)) + self.launch(&start) + .map_err(|error| error.raw_os_error().unwrap_or(0)) }; match admitted { - Ok((execution, program)) => { + Ok((execution, program, cgroup)) => { let binding = Binding::new(execution, runner); - let program = Some(program); - self.attached.insert(key.id(), Run { binding, program }); + let run = Run { + binding, + program, + cgroup, + settled: false, + }; + self.attached.insert(key.id(), run); Ok(()) } Err(error) => refuse(now, runner, error, connection), } } + /// Starts the program of `start`: with cgroups, in a cgroup of its own that the worker + /// prepares below the workload cgroup, and otherwise with the worker's own handle of the + /// program. The worker removes the cgroup of a program that did not start, unless a program + /// that started holds it, as once its execution could not be had, which keeps its place. + fn launch( + &mut self, + start: &Start, + ) -> io::Result<(Execution, Option, Option)> { + let Some(cgroups) = &self.cgroups else { + let (execution, program) = spawn(start, None, &mut self.reaping)?; + return Ok((execution, program, None)); + }; + self.prepared += 1; + let name = format!("execution-{}", self.prepared); + let cgroup = ExecutionCgroup::prepare(cgroups, Some(&name), &[])?; + match spawn(start, Some(&cgroup), &mut self.reaping) { + Ok((execution, program)) => Ok((execution, program, Some(cgroup))), + Err(error) => { + if cgroup.remove().is_err() { + self.fenced.push(cgroup); + } + Err(error) + } + } + } + /// Remembers that the host started `key`, the latest generation of its ID, and forgets the /// ID that it heard of first once it remembers more than [`MAX_HISTORY`]. fn remember(&mut self, key: Key) { @@ -386,13 +518,16 @@ impl Drop for Executions { impl Run { /// Collects the program's status without blocking once the program ended, and hands it to - /// the binding. First, it kills what is left of the process group that the program leads, as - /// the binding's settlement through a cgroup kills what is left in the cgroup, so that no - /// descendant that stayed in the group outlives the program, or holds its output open: until - /// its status is collected, the program, a zombie, keeps its process ID, which is the - /// group's, from any other process, so that the kill reaches that group alone. + /// the binding, if the worker collects it: without cgroups. First, it kills what is left of + /// the process group that the program leads, as the binding's settlement through a cgroup + /// kills what is left in the cgroup, so that no descendant that stayed in the group outlives + /// the program, or holds its output open: until its status is collected, the program, a + /// zombie, keeps its process ID, which is the group's, from any other process, so that the + /// kill reaches that group alone. fn collect(&mut self) { - let Self { binding, program } = self; + let Self { + binding, program, .. + } = self; let Some(child) = program else { return; }; @@ -447,10 +582,11 @@ impl Run { } /// Serves the execution in a turn at `now`: takes every step that it can take, and queues its - /// messages for the host on `connection`. Each read of its program's output goes into - /// `buffer`, until it read [`SHARE`] bytes in the turn, or while the connection is - /// backlogged: an empty buffer then lets it read no output, and return none. Returns whether - /// it read its whole share, and so may have more output to read in the next turn. + /// messages for the host on `connection`, and notes whether its terminal outcome tells that + /// it settled. Each read of its program's output goes into `buffer`, until it read [`SHARE`] + /// bytes in the turn, or while the connection is backlogged: an empty buffer then lets it + /// read no output, and return none. Returns whether it read its whole share, and so may have + /// more output to read in the next turn. fn serve( &mut self, now: Instant, @@ -471,8 +607,10 @@ impl Run { }; match event { Event::Message(message) => { - if let runner::Message::Output { length: bytes, .. } = message { - read += bytes; + match message { + runner::Message::Output { length: bytes, .. } => read += bytes, + runner::Message::Terminal(outcome) => self.settled = outcome.is_reusable(), + _ => {} } if let Some(message) = GuestMessage::from_runner(key, message, lent) { connection.send(&message.encode())?; @@ -485,32 +623,18 @@ impl Run { } } -/// Returns whether the runner of `run` has not returned its terminal outcome, and otherwise -/// [forgets](forget) the execution. -fn running(run: &Run, reaping: &mut Vec) -> bool { - if run.binding.is_finished() { - forget(run, reaping); - return false; - } - true -} - -/// Adds the process ID of the program of `run` to `reaping`, unless the worker collected the -/// program's status: the worker drops the execution next, whose binding kills the program and its -/// process group, which the program's ID still names, and the worker collects the program's -/// status once the program ended. -fn forget(run: &Run, reaping: &mut Vec) { - if run.binding.execution().status().is_none() { - reaping.push(run.binding.id()); - } -} - -/// Spawns the program of `start` with pipes, in a process group of its own, and returns its -/// execution, which leaves the program's status to the worker, with the worker's own handle of -/// the program. If the program started, but the execution or the handle could not be had, the -/// program and its process group end, and the program's ID goes to `reaping`, so that the worker -/// collects its status once it ended, without waiting for it. -fn spawn(start: &Start, reaping: &mut Vec) -> io::Result<(Execution, Child)> { +/// Spawns the program of `start` with pipes, in a process group of its own, and in `cgroup`, if +/// any, and returns its execution. In a cgroup, the execution settles through the cgroup, and +/// collects the program's status itself. Otherwise, it leaves the status to the worker, and comes +/// with the worker's own handle of the program. If the program started, but the execution or the +/// handle could not be had, the program ends, with its process group or the processes of its +/// cgroup, and the program's ID goes to `reaping`, so that the worker collects its status once it +/// ended, without waiting for it. +fn spawn( + start: &Start, + cgroup: Option<&ExecutionCgroup>, + reaping: &mut Vec, +) -> io::Result<(Execution, Option)> { let arguments = start .arguments .iter() @@ -522,25 +646,51 @@ fn spawn(start: &Start, reaping: &mut Vec) -> io::Result<(Execution, Child) Some(stdin) => stdin, None => OwnedFd::from(File::open(NULL_DEVICE)?), }; - let spawned = Command::new(program, arguments) + let mut command = Command::new(program, arguments) .stdin(stdin) .stdout(ends.stdout) .stderr(ends.stderr) - .new_process_group() - .spawn()?; + .new_process_group(); + if let Some(cgroup) = cgroup { + command = command.cgroup(cgroup.placement()?); + } + let spawned = command.spawn()?; let id = spawned.id(); - let options = Options::new().process_group().caller_reaps(); + let options = match cgroup { + Some(cgroup) => Options::new().cgroup(cgroup, Scope::Execution), + None => Options::new().process_group().caller_reaps(), + }; let execution = Execution::new(spawned, pipes, options).inspect_err(|error| { - // The execution ended the program and its process group, and the program's status is - // still the worker's, unless `ESRCH` tells that it is gone already. + // The execution ended the program, and the program's status is still the worker's, + // unless `ESRCH` tells that it is gone already. if error.raw_os_error() != Some(Errno::ESRCH as i32) { reaping.push(id); } })?; + if cgroup.is_some() { + return Ok((execution, None)); + } // Nothing collected the program's status, so its ID still names it. Dropping the execution // after a failure ends the program and its process group. let program = Child::with_pidfd(id).inspect_err(|_| reaping.push(id))?; - Ok((execution, program)) + Ok((execution, Some(program))) +} + +/// Removes the cgroup `cgroup` of an execution that the worker dropped, and returns whether it is +/// gone, or false while a process, a zombie included, is left in it. A removal that fails +/// otherwise is reported, and given up. +fn removed(cgroup: &ExecutionCgroup) -> bool { + match cgroup.remove() { + Ok(()) => true, + Err(error) if error.raw_os_error() == Some(Errno::EBUSY as i32) => false, + Err(error) => { + eprintln!( + "runtime-adapter-guest: cannot remove {}: {error}", + cgroup.path().display() + ); + true + } + } } /// Refuses the execution of `runner` at `now`, whose launch failed at stage 0 with `error`: queues diff --git a/tests/runtime-adapter/tests/guest_session.rs b/tests/runtime-adapter/tests/guest_session.rs index 8c01d6cc4..499ce58e4 100644 --- a/tests/runtime-adapter/tests/guest_session.rs +++ b/tests/runtime-adapter/tests/guest_session.rs @@ -26,12 +26,16 @@ //! stream to the host, which stall only what waits on them, executions that take turns at the //! stream, timeouts, cancellations, and stops, the starts, messages, and probes that the worker //! refuses, generations and resets, the descendants that a program leaves in its process group, -//! and executions on a tty. +//! and executions on a tty. Where the tests can create cgroups, each fake root gives the worker a +//! workload cgroup of the test's own, below which the executions run in cgroups of their own, and +//! settle: the tests then also cover a descendant that leaves its program's process group, which +//! the settlement ends, and an execution that does not settle, which keeps its place until the +//! host stops the worker. Elsewhere, the programs run in process groups alone. #![cfg(all(feature = "guest", target_os = "linux"))] use std::collections::BTreeMap; use std::fs::{self, File, OpenOptions}; -use std::io::{self, Read, Write}; +use std::io::{self, BufRead, BufReader, Read, Write}; use std::net::Shutdown; use std::os::fd::OwnedFd; use std::os::unix::fs::OpenOptionsExt; @@ -39,9 +43,9 @@ use std::os::unix::net::UnixStream; use std::os::unix::process::{CommandExt, ExitStatusExt}; use std::path::{Path, PathBuf}; use std::process::{Child, Command, Output, Stdio}; -use std::sync::Arc; -use std::sync::atomic::{AtomicBool, Ordering}; +use std::sync::atomic::{AtomicBool, AtomicU64, Ordering}; use std::sync::mpsc::{self, Receiver, Sender}; +use std::sync::{Arc, OnceLock}; use std::thread::{self, JoinHandle}; use std::time::{Duration, Instant}; @@ -54,6 +58,7 @@ use nix::unistd::Pid; use nvx::exec::{ Cause, Key, LaunchFailure, Limits, Outcome, Peer, Pool, Status, Stream, Timers, Window, peer, }; +use nvx::guest::cgroup::Layout; use nvx::guest::mappings::{Access, Mapping}; use nvx::guest::nvxs::GuestConnectionError; use nvx::protocol::export::{EXPORT_DIRECTORY, ExportMode, HostExport}; @@ -1140,6 +1145,9 @@ struct FakeRoot { gate: Option>, /// The worker's calls, in order. calls: Vec, + /// The workload cgroup of the test's own, below which the worker runs its executions in + /// cgroups of their own, where the test can create cgroups. + workload: Option, } impl FakeRoot { @@ -1162,8 +1170,11 @@ impl FakeRoot { Self::reading(command_line.as_bytes()) } - /// Returns operations whose kernel command line is `command_line`. + /// Returns operations whose kernel command line is `command_line`, with a workload cgroup of + /// the test's own where the test can create cgroups. fn reading(command_line: &[u8]) -> Self { + let workload = + cgroups().then(|| Workload::new().expect("the test creates a workload cgroup")); Self { command_line: command_line.to_vec(), failing: None, @@ -1172,6 +1183,7 @@ impl FakeRoot { assembly: Duration::ZERO, gate: None, calls: Vec::new(), + workload, } } @@ -1263,6 +1275,10 @@ impl RootOperations for FakeRoot { } } + fn cgroups(&self) -> Option { + self.workload.as_ref().map(|workload| workload.0.clone()) + } + fn probe(&mut self, probe: &Probe) -> Result { self.calls.push(Call::Probe(probe.clone())); if let Some(gate) = &self.gate { @@ -1287,6 +1303,66 @@ fn found(probe: &Probe) -> Facts { facts } +/// Returns whether the tests can create cgroups, as where they run as root in a cgroup v2 +/// hierarchy with the `pids` controller. Each fake root then has a [`Workload`] cgroup of its +/// own, below which the worker runs its executions in cgroups, and otherwise none, so that the +/// worker runs each program in a process group alone. +fn cgroups() -> bool { + static CGROUPS: OnceLock = OnceLock::new(); + *CGROUPS.get_or_init(|| Workload::new().is_some()) +} + +/// A workload cgroup of a test's own, in the real hierarchy, which it removes, with whatever is +/// left in it, when it drops. +#[derive(Debug)] +struct Workload(Layout); + +impl Workload { + /// Creates a new workload cgroup below the root of the hierarchy at `/sys/fs/cgroup`, with the + /// test's own cgroup as the agent's, or returns `None` where the test cannot create one, or + /// the hierarchy lacks the `pids` controller, which the cgroups of executions need. + fn new() -> Option { + static CREATED: AtomicU64 = AtomicU64::new(0); + let root = Path::new("/sys/fs/cgroup"); + let controllers = fs::read_to_string(root.join("cgroup.controllers")).ok()?; + if !controllers + .split_whitespace() + .any(|controller| controller == "pids") + { + return None; + } + let own = fs::read_to_string("/proc/self/cgroup").ok()?; + let own = own.lines().find_map(|line| line.strip_prefix("0::"))?; + let agent = root.join(own.trim_start_matches('/')); + let number = CREATED.fetch_add(1, Ordering::Relaxed); + let name = format!("runtime-adapter-test-{}-{number}", std::process::id()); + let layout = Layout::new(agent, root.join(name)).ok()?; + fs::create_dir(layout.workload()).ok()?; + Some(Self(layout)) + } +} + +impl Drop for Workload { + /// Ends whatever is left in the workload cgroup, waits until it is empty, then removes the + /// cgroups of the executions, and the workload cgroup. + fn drop(&mut self) { + let workload = self.0.workload(); + let _ = fs::write(workload.join("cgroup.kill"), b"1"); + let deadline = Instant::now() + TIMEOUT; + while fs::read(workload.join("pids.current")).is_ok_and(|count| count != b"0\n") + && Instant::now() < deadline + { + thread::sleep(Duration::from_millis(10)); + } + for entry in fs::read_dir(workload).into_iter().flatten().flatten() { + if entry.file_type().is_ok_and(|kind| kind.is_dir()) { + let _ = fs::remove_dir(entry.path()); + } + } + let _ = fs::remove_dir(workload); + } +} + /// Returns a layout that the worker assembles: an EROFS image, with the identity that the guest /// expects of it, over an ext4 scratch. fn layout() -> RootLayout { @@ -2731,8 +2807,9 @@ impl Drop for Host { } } -/// Checks that the execution `id` ended on its own with `status`, losing no byte, and fenced, as -/// nothing proves that the descendants of a program without a cgroup are gone. +/// Checks that the execution `id` ended on its own with `status`, losing no byte, and settled if +/// its program ran in a cgroup of its own, where the tests can create cgroups: otherwise it is +/// fenced, as nothing proves that the descendants of a program without a cgroup are gone. fn check_lossless(host: &Host, id: u64, status: Status) { let outcome = host.outcome(id).unwrap(); assert_eq!(outcome.status(), Some(status), "{id}"); @@ -2741,7 +2818,7 @@ fn check_lossless(host: &Host, id: u64, status: Status) { for stream in Stream::ALL { assert!(!outcome.is_truncated(stream), "{id}: {stream:?}"); } - assert!(!outcome.is_reusable(), "{id}"); + assert_eq!(outcome.is_reusable(), cgroups(), "{id}"); let peer = &host.executions[&id].peer; assert!(peer.high_water() <= peer.memory_bound(), "{id}"); } @@ -3173,6 +3250,172 @@ fn the_worker_ends_what_is_left_of_the_process_group_of_a_program_that_ended() { gone(second); } +/// Returns the program's process ID, or that of another process, from the line `line` of the +/// standard output of the execution `id`, and the directory of the program's cgroup from the next +/// line, which `/proc/self/cgroup` gave. +fn process_and_cgroup(host: &Host, id: u64, line: usize) -> (u32, PathBuf) { + let output = String::from_utf8(host.executions[&id].output[0].clone()).unwrap(); + let mut lines = output.lines().skip(line); + let process = lines.next().unwrap().parse().unwrap(); + let cgroup = lines.next().unwrap().strip_prefix("0::/").unwrap(); + (process, Path::new("/sys/fs/cgroup").join(cgroup)) +} + +#[test] +fn an_execution_s_settlement_ends_a_descendant_that_left_its_process_group() { + // Only an execution's cgroup reaches a descendant in a session of its own. + if !cgroups() { + return; + } + // The program starts a descendant in a session of its own, which holds the program's output + // open once it wrote its process ID, and prints the descendant's process ID and its own + // cgroup, then ends. The execution's settlement kills the descendant, so that the output ends + // at once, losing nothing, and the execution settles. + let mut host = Host::new(); + let script = "exec 3>&1; \ + descendant=$(setsid sh -c 'echo $$; exec sleep 60 >&3 3>&-' &); \ + echo \"$descendant\"; cat /proc/self/cgroup; exit 3"; + let started = Instant::now(); + host.start(start_of(1, shell(script)), Vec::new()); + host.run_until(|host| host.outcome(1).is_some()); + assert!(started.elapsed() < SETTLEMENT, "{:?}", started.elapsed()); + check_lossless(&host, 1, Status::Exited(3)); + let (descendant, cgroup) = process_and_cgroup(&host, 1, 0); + reaped(descendant); + // The worker removed the execution's cgroup before it sent the outcome, which freed the + // execution's place: it runs its limit of executions at once again. + assert!(!cgroup.exists(), "{}", cgroup.display()); + let most = u64::try_from(MAX_EXECUTIONS).unwrap(); + for id in 2..=most + 1 { + host.start(start_of(id, shell("exec sleep 60")), Vec::new()); + } + host.run_until(|host| (2..=most + 1).all(|id| host.executions[&id].peer.has_started())); + let (_, worked) = host.stop(); + assert_eq!(worked.unwrap(), Worked::Stopped); +} + +/// A process with a child that it never waits for, so that the child, once it ended, stays a +/// zombie, and occupies its cgroup, until the process ends. The test kills the process when it +/// drops, which leaves the child to the system's collector of orphans. +struct Holder { + /// The process. + process: Child, + /// The process ID of its child. + child: u32, +} + +impl Holder { + /// Starts the process and its child. + fn new() -> Self { + let mut process = Command::new("/bin/sh") + .args(["-c", "sleep 600 & echo $!; exec sleep 600"]) + .stdin(Stdio::null()) + .stdout(Stdio::piped()) + .spawn() + .unwrap(); + let mut line = String::new(); + BufReader::new(process.stdout.take().unwrap()) + .read_line(&mut line) + .unwrap(); + let child = line.trim().parse().unwrap(); + Self { process, child } + } +} + +impl Drop for Holder { + fn drop(&mut self) { + let _ = self.process.kill(); + let _ = self.process.wait(); + } +} + +#[test] +fn an_execution_that_does_not_settle_keeps_its_place_until_the_host_stops_the_worker() { + // A zombie whose parent is outside the cgroup keeps a cgroup occupied, which needs cgroups. + if !cgroups() { + return; + } + // The program prints its process ID and its cgroup, and waits until the test signals it, + // once the test moved a holder's child into the cgroup. The settlement kills the child, which + // stays a zombie, so that the execution does not settle within its settlement time. + let holder = Holder::new(); + let mut host = Host::new(); + let waiting = "trap 'exit 0' USR1; echo $$; cat /proc/self/cgroup; \ + while :; do sleep 0.01; done"; + let settlement = Duration::from_secs(1); + let start = Start::new( + Key::new(1, 1), + shared_limits(), + Timers::new(GRACE, settlement), + shell(waiting), + ); + host.start(start, Vec::new()); + host.run_until(|host| { + let output = &host.executions[&1].output[0]; + output.iter().filter(|&&byte| byte == b'\n').count() == 2 + }); + let (program, cgroup) = process_and_cgroup(&host, 1, 0); + fs::write(cgroup.join("cgroup.procs"), holder.child.to_string()).unwrap(); + let program = Pid::from_raw(i32::try_from(program).unwrap()); + signal::kill(program, Signal::SIGUSR1).unwrap(); + host.run_until(|host| host.outcome(1).is_some()); + let outcome = host.outcome(1).unwrap(); + assert_eq!(outcome.status(), Some(Status::Exited(0))); + assert_eq!(outcome.cause(), None); + for stream in Stream::ALL { + assert!(!outcome.is_truncated(stream), "{stream:?}"); + } + assert!(!outcome.is_reusable()); + assert!(cgroup.exists(), "{}", cgroup.display()); + + // The execution keeps its place: the worker runs one execution fewer than its limit, and + // refuses the next start. It still does once the cgroup is empty, after the holder ended. + let most = u64::try_from(MAX_EXECUTIONS).unwrap(); + fill_but_one(&mut host, 2); + drop(holder); + let deadline = Instant::now() + TIMEOUT; + while fs::read(cgroup.join("pids.current")).unwrap() != b"0\n" { + assert!(Instant::now() < deadline, "the cgroup stays occupied"); + thread::sleep(Duration::from_millis(10)); + } + fill_but_one(&mut host, 2 + most); + + // The stop ends the executions, and removes every cgroup, the fenced one's included. + let (_, worked) = host.stop(); + assert_eq!(worked.unwrap(), Worked::Stopped); + assert!(!cgroup.exists(), "{}", cgroup.display()); +} + +/// Starts one execution fewer than the worker's limit, from the ID `first` on, while another +/// execution keeps its place: checks that they run, and that the worker refuses the next start, +/// then cancels them, and checks that they settled. +fn fill_but_one(host: &mut Host, first: u64) { + let most = u64::try_from(MAX_EXECUTIONS).unwrap(); + let ids = first..first + most - 1; + for id in ids.clone() { + host.start(start_of(id, shell("exec sleep 60")), Vec::new()); + } + host.run_until(|host| { + ids.clone() + .all(|id| host.executions[&id].peer.has_started()) + }); + let refused = ids.end; + host.start(start_of(refused, shell("exit 0")), Vec::new()); + host.run_until(|host| host.outcome(refused).is_some()); + let again = LaunchFailure::new(0, Errno::EAGAIN as i32); + assert_eq!(host.outcome(refused).unwrap().launch_failure(), Some(again)); + let now = Instant::now(); + for id in ids.clone() { + host.execution(id).peer.cancel(now); + } + host.run_until(|host| ids.clone().all(|id| host.outcome(id).is_some())); + for id in ids { + let outcome = host.outcome(id).unwrap(); + assert_eq!(outcome.cause(), Some(Cause::Cancelled), "{id}"); + assert!(outcome.is_reusable(), "{id}"); + } +} + #[test] fn the_worker_collects_the_status_of_a_program_that_a_real_time_signal_ended() { // `nix` decodes no real-time signal in a status, so that the worker learns that the program From 229c58dcef6153033a2b90a1a5a87d02ddf0e83f Mon Sep 17 00:00:00 2001 From: Enrique Saurez Date: Sat, 24 Oct 2026 00:35:39 -0700 Subject: [PATCH 2/3] tests: keep an execution's cgroup until its program is collected A program that the worker spawned, but whose execution it could not have, may still be joining its cgroup, as its launch has yet to report, and removing the cgroup could then hide the program once it ended, which ExecutionCgroup::remove forbids. Such a start's cgroup now keeps its place, as that of an execution that did not settle, and a stop removes the executions' cgroups only once the worker collected the status of every program that it forgot. The holder in the test of an execution that does not settle now runs in a process group of its own, which its drop kills, so that its child no longer outlives a test that fails before the child joins the cgroup. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a7ad408a-da3b-45c4-aa02-5d25300b1c90 --- tests/runtime-adapter/README.md | 8 +- tests/runtime-adapter/src/worker.rs | 7 +- .../runtime-adapter/src/worker/executions.rs | 73 ++++++++++++++----- tests/runtime-adapter/tests/guest_session.rs | 10 ++- 4 files changed, 72 insertions(+), 26 deletions(-) diff --git a/tests/runtime-adapter/README.md b/tests/runtime-adapter/README.md index fb1173945..6a495a07c 100644 --- a/tests/runtime-adapter/README.md +++ b/tests/runtime-adapter/README.md @@ -548,7 +548,10 @@ credit that it needs, never keeps the others' output from the host. is empty and the worker removed it, which the worker does once the runner returned the terminal outcome of an execution that settled. One that did not settle, whose outcome is fenced, keeps its place and its cgroup until the - host stops the worker. + host stops the worker, as does a start whose program the worker spawned, but + whose execution it could not have: until the worker collected the status of + such a program, the program may still join the cgroup, so that a removal + could hide it once it ended. - Without cgroups, as where the in-process tests cannot create them, the root's operations name none, and the program's process group stands in for its cgroup: its execution kills the group as a whole. The worker collects the @@ -580,7 +583,8 @@ credit that it needs, never keeps the others' output from the host. program of each one that has not ended, with what is left in its cgroup or its process group, and what is left in the cgroups of the executions that did not settle, and waits up to a second in all for those programs to end and - those cgroups to empty, removing each cgroup that emptied. + those cgroups to empty, removing each cgroup that emptied once it collected + the statuses of those programs. The worker's `entries` probe runs two more commands of the guest agent: `runtime-adapter-guest entries ROOT CGROUP`, its helper, which runs the probe's diff --git a/tests/runtime-adapter/src/worker.rs b/tests/runtime-adapter/src/worker.rs index de7bedfc1..dbde923e1 100644 --- a/tests/runtime-adapter/src/worker.rs +++ b/tests/runtime-adapter/src/worker.rs @@ -134,7 +134,10 @@ //! - An execution keeps its place among [`MAX_EXECUTIONS`] until its cgroup is empty and the //! worker removed it, which the worker does once the runner returned the terminal outcome of an //! execution that settled. One that did not settle, whose outcome is fenced, keeps its place -//! and its cgroup until the host stops the worker. +//! and its cgroup until the host stops the worker, as does a start whose program the worker +//! spawned, but whose execution it could not have: until the worker collected the status of +//! such a program, the program may still join the cgroup, so that a removal could hide it once +//! it ended. //! - Without cgroups, as where the in-process tests cannot create them, the [root's //! operations](RootOperations::cgroups) name none, and the program's process group stands in //! for its cgroup: its execution kills the group as a whole. The worker collects the status of @@ -162,7 +165,7 @@ //! of each one that has not ended, with what is left in its cgroup or its process group, and //! what is left in the cgroups of the executions that did not settle, and waits up to a second //! in all for those programs to end and those cgroups to empty, removing each cgroup that -//! emptied. +//! emptied once it collected the statuses of those programs. mod connection; pub mod entries; diff --git a/tests/runtime-adapter/src/worker/executions.rs b/tests/runtime-adapter/src/worker/executions.rs index 627a31735..63e14d080 100644 --- a/tests/runtime-adapter/src/worker/executions.rs +++ b/tests/runtime-adapter/src/worker/executions.rs @@ -262,8 +262,11 @@ impl Executions { /// cgroups emptied, so that the worker collects the programs' statuses and removes the /// cgroups, those of the executions that did not settle included, whose processes it ends /// too, at most [`REAP_WAIT`] after it first dropped every execution: a later call, as once - /// the executions drop after a stop, waits no longer. Without cgroups, the worker ended the - /// process groups of the other programs when it collected their statuses. + /// the executions drop after a stop, waits no longer. It removes the cgroups once it + /// collected the status of every program that it forgot: until then, such a program may + /// still join its cgroup, as its launch may not have reported, and a removal could hide it + /// once it ended. Without cgroups, the worker ended the process groups of the other programs + /// when it collected their statuses. pub(super) fn clear(&mut self) { let runs = std::mem::take(&mut self.attached) .into_values() @@ -293,7 +296,9 @@ impl Executions { loop { let now = Instant::now(); self.reap(now); - self.fenced.retain(|cgroup| !removed(cgroup)); + if self.reaping.is_empty() { + self.fenced.retain(|cgroup| !removed(cgroup)); + } if (self.reaping.is_empty() && self.fenced.is_empty()) || now >= deadline { return; } @@ -466,14 +471,17 @@ impl Executions { /// Starts the program of `start`: with cgroups, in a cgroup of its own that the worker /// prepares below the workload cgroup, and otherwise with the worker's own handle of the - /// program. The worker removes the cgroup of a program that did not start, unless a program - /// that started holds it, as once its execution could not be had, which keeps its place. + /// program. The worker removes the cgroup of a program that it could not spawn. One that it + /// spawned, but whose execution could not be had, may still join the cgroup until the worker + /// collected its status, so that a removal could hide it once it ended: its cgroup keeps its + /// place, as that of an execution that did not settle. fn launch( &mut self, start: &Start, ) -> io::Result<(Execution, Option, Option)> { let Some(cgroups) = &self.cgroups else { - let (execution, program) = spawn(start, None, &mut self.reaping)?; + let (execution, program) = + spawn(start, None, &mut self.reaping).map_err(|unstarted| unstarted.error)?; return Ok((execution, program, None)); }; self.prepared += 1; @@ -481,8 +489,8 @@ impl Executions { let cgroup = ExecutionCgroup::prepare(cgroups, Some(&name), &[])?; match spawn(start, Some(&cgroup), &mut self.reaping) { Ok((execution, program)) => Ok((execution, program, Some(cgroup))), - Err(error) => { - if cgroup.remove().is_err() { + Err(Unstarted { error, spawned }) => { + if spawned || cgroup.remove().is_err() { self.fenced.push(cgroup); } Err(error) @@ -623,23 +631,42 @@ impl Run { } } +/// Why the worker could not start a program. +struct Unstarted { + /// The error. + error: io::Error, + /// Whether the worker spawned the program before the error. + spawned: bool, +} + +impl From for Unstarted { + /// Returns the failure of a program that the worker did not spawn. + fn from(error: io::Error) -> Self { + Self { + error, + spawned: false, + } + } +} + /// Spawns the program of `start` with pipes, in a process group of its own, and in `cgroup`, if /// any, and returns its execution. In a cgroup, the execution settles through the cgroup, and /// collects the program's status itself. Otherwise, it leaves the status to the worker, and comes /// with the worker's own handle of the program. If the program started, but the execution or the /// handle could not be had, the program ends, with its process group or the processes of its /// cgroup, and the program's ID goes to `reaping`, so that the worker collects its status once it -/// ended, without waiting for it. +/// ended, without waiting for it; the failure then tells that the worker spawned the program. fn spawn( start: &Start, cgroup: Option<&ExecutionCgroup>, reaping: &mut Vec, -) -> io::Result<(Execution, Option)> { +) -> Result<(Execution, Option), Unstarted> { let arguments = start .arguments .iter() .map(|argument| CString::new(argument.as_slice())) - .collect::, _>>()?; + .collect::, _>>() + .map_err(io::Error::from)?; let program = arguments.first().cloned().unwrap_or_default(); let (pipes, ends) = Pipes::new(start.input)?; let stdin = match ends.stdin { @@ -660,19 +687,27 @@ fn spawn( Some(cgroup) => Options::new().cgroup(cgroup, Scope::Execution), None => Options::new().process_group().caller_reaps(), }; - let execution = Execution::new(spawned, pipes, options).inspect_err(|error| { - // The execution ended the program, and the program's status is still the worker's, - // unless `ESRCH` tells that it is gone already. - if error.raw_os_error() != Some(Errno::ESRCH as i32) { - reaping.push(id); - } - })?; + let unstarted = |error| Unstarted { + error, + spawned: true, + }; + let execution = Execution::new(spawned, pipes, options) + .inspect_err(|error| { + // The execution ended the program, and the program's status is still the worker's, + // unless `ESRCH` tells that it is gone already. + if error.raw_os_error() != Some(Errno::ESRCH as i32) { + reaping.push(id); + } + }) + .map_err(unstarted)?; if cgroup.is_some() { return Ok((execution, None)); } // Nothing collected the program's status, so its ID still names it. Dropping the execution // after a failure ends the program and its process group. - let program = Child::with_pidfd(id).inspect_err(|_| reaping.push(id))?; + let program = Child::with_pidfd(id) + .inspect_err(|_| reaping.push(id)) + .map_err(unstarted)?; Ok((execution, Some(program))) } diff --git a/tests/runtime-adapter/tests/guest_session.rs b/tests/runtime-adapter/tests/guest_session.rs index 499ce58e4..4ebf4bc0f 100644 --- a/tests/runtime-adapter/tests/guest_session.rs +++ b/tests/runtime-adapter/tests/guest_session.rs @@ -3295,8 +3295,9 @@ fn an_execution_s_settlement_ends_a_descendant_that_left_its_process_group() { } /// A process with a child that it never waits for, so that the child, once it ended, stays a -/// zombie, and occupies its cgroup, until the process ends. The test kills the process when it -/// drops, which leaves the child to the system's collector of orphans. +/// zombie, and occupies its cgroup, until the process ends. Both run in a process group that the +/// process leads, which the test kills when it drops, so that neither outlives a test that fails; +/// the system's collector of orphans then collects the child's status. struct Holder { /// The process. process: Child, @@ -3311,6 +3312,7 @@ impl Holder { .args(["-c", "sleep 600 & echo $!; exec sleep 600"]) .stdin(Stdio::null()) .stdout(Stdio::piped()) + .process_group(0) .spawn() .unwrap(); let mut line = String::new(); @@ -3324,7 +3326,9 @@ impl Holder { impl Drop for Holder { fn drop(&mut self) { - let _ = self.process.kill(); + // Until its status is collected, the process keeps its ID, which is the group's. + let group = Pid::from_raw(i32::try_from(self.process.id()).unwrap()); + let _ = signal::killpg(group, Signal::SIGKILL); let _ = self.process.wait(); } } From a8cc6e2d831f701a3984630345f2c7a4896e396c Mon Sep 17 00:00:00 2001 From: Enrique Saurez Date: Sat, 24 Oct 2026 01:18:59 -0700 Subject: [PATCH 3/3] tests: keep the fake root until the checks that follow a stop Once the in-process tests create cgroups, each fake root holds a workload cgroup of the test's own, which ends what is left in it and removes the cgroups of executions when it drops. Two tests discarded the fake root that a stop returns, so that it dropped before their checks: that the stop removed the cgroup of an execution that did not settle, and that it left no process of a program's group running. Those checks then held whatever the worker did. The tests now keep the fake root until their checks are done. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a7ad408a-da3b-45c4-aa02-5d25300b1c90 --- tests/runtime-adapter/tests/guest_session.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/tests/runtime-adapter/tests/guest_session.rs b/tests/runtime-adapter/tests/guest_session.rs index 4ebf4bc0f..73ede8a60 100644 --- a/tests/runtime-adapter/tests/guest_session.rs +++ b/tests/runtime-adapter/tests/guest_session.rs @@ -3245,7 +3245,9 @@ fn the_worker_ends_what_is_left_of_the_process_group_of_a_program_that_ended() { host.run_until(|host| host.executions[&2].output[0].ends_with(b"\n")); let second = host.executions[&2].group(); reaped(second); - let (_, worked) = host.stop(); + // The fake root lives until the check, as its workload cgroup, if any, ends what is left in it + // once it drops. + let (_root, worked) = host.stop(); assert_eq!(worked.unwrap(), Worked::Stopped); gone(second); } @@ -3384,8 +3386,9 @@ fn an_execution_that_does_not_settle_keeps_its_place_until_the_host_stops_the_wo } fill_but_one(&mut host, 2 + most); - // The stop ends the executions, and removes every cgroup, the fenced one's included. - let (_, worked) = host.stop(); + // The stop ends the executions, and removes every cgroup, the fenced one's included. The fake + // root lives until the check, as its workload cgroup removes what is left in it once it drops. + let (_root, worked) = host.stop(); assert_eq!(worked.unwrap(), Worked::Stopped); assert!(!cgroup.exists(), "{}", cgroup.display()); }