From 33edc42c6fdc97a9fc974b9a45fe72b901cfe426 Mon Sep 17 00:00:00 2001 From: Enrique Saurez Date: Mon, 9 Nov 2026 14:11:33 -0800 Subject: [PATCH 1/2] cli: spawn the download's curl, tar and git directly The download started its tools with supervisor::spawn_command, which holds the old supervisor's spawn lock while it creates a process, so that on Windows the tool inherits no handle of a VMM that another thread of the process starts meanwhile (#966). The CLI is to stop using the old supervisor, and needs no lock for its tools: run, sandbox run and sandbox start download before they launch, and no CLI command starts a VM on another thread while it downloads. tar, git and curl now start with Command::spawn, with the same standard streams, environment and working directory as before. Their errors stay as they were: a tool that does not run fails the download as unavailable, naming the program; a tool that fails reports its standard error; and the CLI checks for Ctrl-C once a tool exits, which reaches the tools as before, since they still share the CLI's console and process group. The library's tests, though, run the download beside tests that launch stand-in VMs, so #966's protection moves into them: each archive and GitHub test that runs tar or curl holds the tests' spawn lock, which host::tests::hold_spawns returns, while it runs them, and the archive tests' helper that packages a release spawns its own tar with host::tests::spawn_locked, which therefore builds wherever the CLI does. A new GitHub test checks the error of a curl that does not run, which the changed spawn produces. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- nvx-rs/src/host/cli/download/archive.rs | 4 +-- nvx-rs/src/host/cli/download/archive/tests.rs | 20 ++++++++--- nvx-rs/src/host/cli/download/git.rs | 3 +- nvx-rs/src/host/cli/download/github.rs | 3 +- nvx-rs/src/host/cli/download/github/tests.rs | 36 +++++++++++++++++++ nvx-rs/src/host/tests.rs | 12 ++++++- 6 files changed, 67 insertions(+), 11 deletions(-) diff --git a/nvx-rs/src/host/cli/download/archive.rs b/nvx-rs/src/host/cli/download/archive.rs index 97a770409..6d5c01b76 100644 --- a/nvx-rs/src/host/cli/download/archive.rs +++ b/nvx-rs/src/host/cli/download/archive.rs @@ -19,7 +19,6 @@ use super::system_program; use crate::ErrorCode; use crate::host::artifacts::CHECKSUMS_NAME; use crate::host::cli::{Exit, Result, check_interrupt, io_error}; -use crate::host::supervisor::spawn_command; /// The directory beside the package into which `tar` extracts it. const EXTRACTION_DIR: &str = "package"; @@ -83,7 +82,8 @@ fn tar(directory: &Path, arguments: &[&str]) -> Result { .stdin(Stdio::null()) .stdout(Stdio::piped()) .stderr(Stdio::piped()); - let output = spawn_command(&mut command) + let output = command + .spawn() .and_then(Child::wait_with_output) .map_err(|error| { Exit::error( diff --git a/nvx-rs/src/host/cli/download/archive/tests.rs b/nvx-rs/src/host/cli/download/archive/tests.rs index 44a403d3f..838b98494 100644 --- a/nvx-rs/src/host/cli/download/archive/tests.rs +++ b/nvx-rs/src/host/cli/download/archive/tests.rs @@ -1,7 +1,13 @@ //! The tests of packages: the names of their members, their extraction with the host's `tar`, //! and their checksums. +//! +//! Each test that runs `tar` holds the tests' spawn lock while it does: the CLI downloads before it +//! launches a VM, but these tests run beside tests that launch stand-in VMs, and on Windows a `tar` +//! that starts during a VM's start inherits that VM's handles, such as its log, which it holds +//! until it exits. use super::*; +use crate::host::tests::hold_spawns; /// The root directory of the packages of the tests. const ROOT: &str = "nvx-0.1.0-linux-kvm"; @@ -43,10 +49,11 @@ fn archive(source: &Path, archive: &Path) { } else { command.arg("-czf"); } - let status = spawn_command(command.arg(archive).arg(ROOT).current_dir(source)) - .unwrap() - .wait() - .unwrap(); + let status = + crate::host::tests::spawn_locked(command.arg(archive).arg(ROOT).current_dir(source)) + .unwrap() + .wait() + .unwrap(); assert!(status.success()); } @@ -65,6 +72,7 @@ fn staged(directory: &Path, name: &str, change: impl FnOnce(&Path)) -> PathBuf { #[test] fn gzipped_packages_extract_once_their_names_and_checksums_pass() { + let _spawns = hold_spawns(); let directory = tempfile::tempdir().unwrap(); let archive = staged(directory.path(), "nvx-0.1.0-linux-kvm.tar.gz", |_| {}); let package = extract(&archive).unwrap(); @@ -80,6 +88,7 @@ fn gzipped_packages_extract_once_their_names_and_checksums_pass() { #[cfg(windows)] #[test] fn zip_packages_extract_with_the_tar_of_windows() { + let _spawns = hold_spawns(); let directory = tempfile::tempdir().unwrap(); let archive = staged(directory.path(), "nvx-0.1.0-windows-whp.zip", |_| {}); let package = extract(&archive).unwrap(); @@ -91,6 +100,7 @@ fn zip_packages_extract_with_the_tar_of_windows() { #[test] fn packages_whose_checksums_fail_are_refused() { + let _spawns = hold_spawns(); for (change, expected) in [ ( (|root: &Path| fs::write(root.join("guest").join("vmlinux"), "other\n").unwrap()) @@ -120,6 +130,7 @@ fn packages_whose_checksums_fail_are_refused() { #[cfg(unix)] #[test] fn packages_with_symlinks_are_refused() { + let _spawns = hold_spawns(); let directory = tempfile::tempdir().unwrap(); let archive = staged(directory.path(), "nvx-0.1.0-linux-kvm.tar.gz", |root| { std::os::unix::fs::symlink("/etc/passwd", root.join("guest").join("link")).unwrap(); @@ -133,6 +144,7 @@ fn packages_with_symlinks_are_refused() { #[test] fn archives_that_tar_cannot_read_are_refused() { + let _spawns = hold_spawns(); let directory = tempfile::tempdir().unwrap(); let archive = directory.path().join("nvx-0.1.0-linux-kvm.tar.gz"); fs::write(&archive, "not an archive").unwrap(); diff --git a/nvx-rs/src/host/cli/download/git.rs b/nvx-rs/src/host/cli/download/git.rs index bfaff1dd6..b401edd43 100644 --- a/nvx-rs/src/host/cli/download/git.rs +++ b/nvx-rs/src/host/cli/download/git.rs @@ -10,7 +10,6 @@ use std::process::{Command, Output, Stdio}; use super::History; use crate::ErrorCode; use crate::host::cli::{Exit, Result}; -use crate::host::supervisor::spawn_command; /// The `git` of a checkout. pub(super) struct Git { @@ -43,7 +42,7 @@ impl Git { { command.env("GIT_CEILING_DIRECTORIES", ceiling); } - spawn_command(&mut command)?.wait_with_output() + command.spawn()?.wait_with_output() } } diff --git a/nvx-rs/src/host/cli/download/github.rs b/nvx-rs/src/host/cli/download/github.rs index eaa0f27d4..2b713e983 100644 --- a/nvx-rs/src/host/cli/download/github.rs +++ b/nvx-rs/src/host/cli/download/github.rs @@ -18,7 +18,6 @@ use serde_json::Value; use super::{Asset, Releases, remove_stale, system_program}; use crate::ErrorCode; use crate::host::cli::{Exit, Result, check_interrupt, io_error}; -use crate::host::supervisor::spawn_command; /// The base URL of the GitHub REST API. const API: &str = "https://api.github.com"; @@ -323,7 +322,7 @@ impl Curl { } else { Stdio::piped() }); - let mut child = spawn_command(&mut command).map_err(|error| { + let mut child = command.spawn().map_err(|error| { Exit::error( ErrorCode::Unavailable, format!( diff --git a/nvx-rs/src/host/cli/download/github/tests.rs b/nvx-rs/src/host/cli/download/github/tests.rs index 955101aee..b17d31c56 100644 --- a/nvx-rs/src/host/cli/download/github/tests.rs +++ b/nvx-rs/src/host/cli/download/github/tests.rs @@ -1,5 +1,10 @@ //! The tests of the client of the GitHub REST API: its messages, its configuration of `curl`, //! and its requests, which a real `curl` sends to servers on the loopback interface. +//! +//! Each test that runs `curl` holds the tests' spawn lock while it does: the CLI downloads before +//! it launches a VM, but these tests run beside tests that launch stand-in VMs, and on Windows a +//! `curl` that starts during a VM's start inherits that VM's handles, such as its log, which it +//! holds until it exits. use std::io::{BufRead, BufReader}; use std::net::{TcpListener, TcpStream}; @@ -9,6 +14,7 @@ use std::thread; use serde_json::json; use super::*; +use crate::host::tests::hold_spawns; impl Curl { /// Returns a `curl` that also speaks plain HTTP, to the servers of tests. @@ -134,6 +140,7 @@ fn answer( #[test] fn queries_and_downloads_carry_the_token_to_the_origin_of_the_api_alone() { + let _spawns = hold_spawns(); let (api_listener, api) = Server::bind(); let (asset_listener, assets) = Server::bind(); let asset_url = format!("{api}/repos/octo/nvx/releases/assets/7"); @@ -253,6 +260,7 @@ fn queries_and_downloads_carry_the_token_to_the_origin_of_the_api_alone() { #[test] fn requests_without_a_token_carry_none() { + let _spawns = hold_spawns(); let (listener, api) = Server::bind(); let server = Server::serve(listener, Vec::new()); let scratch = tempfile::tempdir().unwrap(); @@ -275,6 +283,7 @@ fn requests_without_a_token_carry_none() { #[test] fn redirects_resolve_relative_locations_against_the_urls_of_their_requests() { + let _spawns = hold_spawns(); let (listener, api) = Server::bind(); let server = Server::serve( listener, @@ -329,6 +338,7 @@ fn redirects_resolve_relative_locations_against_the_urls_of_their_requests() { #[test] fn the_last_response_to_a_retried_request_counts() { + let _spawns = hold_spawns(); let (listener, api) = Server::bind(); let path = "/repos/octo/nvx/releases/tags/v0.1.0-dev.a1b2c3d4e5f6"; // The status, the header, and the message of the first response, which `curl` retries, would @@ -399,6 +409,32 @@ fn requests_to_github_speak_https_alone() { ); } +#[test] +fn a_missing_curl_reports_the_program_and_query_context() { + let scratch = tempfile::tempdir().unwrap(); + let program = scratch.path().join("not-installed-curl"); + let github = GitHub::at( + API, + "octo/nvx", + None, + scratch.path(), + Curl { + program: program.clone(), + ..Curl::new() + }, + ) + .unwrap(); + let error = Exit::from(github.release("v1").unwrap_err()); + assert_eq!(error.code(), Some(ErrorCode::Unavailable)); + assert!( + error.message().unwrap().starts_with(&format!( + "GitHub release query needs curl, which did not run: {}: ", + program.display() + )), + "{error:?}" + ); +} + #[test] fn configurations_quote_their_values_and_carry_the_token_when_asked() { let curl = Curl { diff --git a/nvx-rs/src/host/tests.rs b/nvx-rs/src/host/tests.rs index c35858129..c183ef74d 100644 --- a/nvx-rs/src/host/tests.rs +++ b/nvx-rs/src/host/tests.rs @@ -32,7 +32,7 @@ pub(crate) fn released(mut free: impl FnMut() -> bool) -> bool { /// Spawns `command` under a `SpawnLock`, which it releases once the process exists, so that on /// Windows the process inherits no handle of a VMM that a concurrent test spawns, such as the /// VMM's log, which it would hold until it exits. -#[cfg(any(target_os = "linux", windows))] +#[cfg(any(target_os = "linux", windows, feature = "cli"))] pub(crate) fn spawn_locked( command: &mut std::process::Command, ) -> std::io::Result { @@ -40,6 +40,16 @@ pub(crate) fn spawn_locked( command.spawn() } +/// Returns a `SpawnLock`, which a test holds while the code that it tests spawns processes with +/// `std::process`, as the download spawns `tar` and `curl`, so that on Windows none of them +/// inherits a handle of a VMM that a concurrent test spawns, such as the VMM's log, which it would +/// hold until it exits. A concurrent spawn of a VMM waits for the lock, so a test holds it only +/// while it runs those processes. +#[cfg(feature = "cli")] +pub(crate) fn hold_spawns() -> super::platform::inherit::SpawnLock { + super::platform::inherit::lock_spawns() +} + /// Makes `link` a junction to the directory `target`, which any user may create, with `cmd`'s /// `mklink /J`, quoting both paths, since `cmd` splits its arguments at commas and semicolons. /// From 6262d145367764c78d76cfe1136cd951a4428193 Mon Sep 17 00:00:00 2001 From: Enrique Saurez Date: Mon, 9 Nov 2026 14:11:33 -0800 Subject: [PATCH 2/2] cli: wait for a run's control endpoint on its VM's process A managed cold run waits for its control endpoint with Session::connect_launched_cancellable, which ends the wait once OpenVMM exits, rather than at its timeout. It took the old supervisor's child type, spawn::Launched, which host::vm::Launched lent it through child_mut, and polled that child's status. It now takes the verified process of the run's host::vm::Launched, beside the endpoint and capability that the launch supplies, and asks that process whether it has exited. The process is the same OpenVMM, held through its pidfd or process handle, so the wait notices its exit as the child's status did, without reaping it: the foreground still reaps it through wait_status. A check that fails still keeps the wait going until the timeout. child_mut becomes private to host::vm, whose wait_status alone uses it, and the session's comments no longer name host::instance. sandbox exec and sandbox stop attach through Session::reattach, which verifies a saved VM through host::vm, and do not change. A new test gives the wait a process that runs and then exits: the wait goes on while the process runs, and ends as a closed endpoint once it exits, rather than at its timeout, on Linux and Windows. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- nvx-rs/src/host/cli/run.rs | 4 +- nvx-rs/src/host/cli/session.rs | 16 ++--- nvx-rs/src/host/cli/session/tests.rs | 98 ++++++++++++++++++++++++++++ nvx-rs/src/host/vm.rs | 6 +- 4 files changed, 109 insertions(+), 15 deletions(-) diff --git a/nvx-rs/src/host/cli/run.rs b/nvx-rs/src/host/cli/run.rs index c426dcc54..f6d47e6e1 100644 --- a/nvx-rs/src/host/cli/run.rs +++ b/nvx-rs/src/host/cli/run.rs @@ -1190,12 +1190,12 @@ fn foreground( if managed && output.console.is_some() { let endpoint = launched.outputs.endpoint.clone(); let capability = *launched.capability.as_bytes(); - let child = launched.child_mut().map_err(Exit::from_os_error)?; + let process = &launched.process; match attach_with_output( |cancellation| { Session::connect_launched_cancellable( &endpoint, - child, + process, &capability, 60.0, cancellation, diff --git a/nvx-rs/src/host/cli/session.rs b/nvx-rs/src/host/cli/session.rs index 991f91e14..31cd61672 100644 --- a/nvx-rs/src/host/cli/session.rs +++ b/nvx-rs/src/host/cli/session.rs @@ -37,10 +37,10 @@ use std::time::Duration; use super::deadline::{Deadline, RETRY_INTERVAL, sleep}; use super::{Exit, Result, check_interrupt}; use crate::host::platform::{self, Transport}; +use crate::host::process::Process; use crate::host::runtime::Runtime; use crate::host::sandbox::options::validate_exec_environment; use crate::host::session::{ControlSession, SessionError}; -use crate::host::supervisor::Launched; use crate::host::transport; use crate::host::vm::{self, SavedVm}; use crate::protocol::{ @@ -335,7 +335,7 @@ impl Session { }) } - /// Connects to `endpoint`, which `launched`, the OpenVMM process that a cold run launched, + /// Connects to `endpoint`, which `process`, the OpenVMM process that a cold run launched, /// serves, as [`Self::connect_unless_exited`] does, but stops waiting for the endpoint once /// OpenVMM has exited, as `nvx::host::session::connect` does: an OpenVMM that exits before it /// serves its endpoint, as one that refuses its configuration does, then ends the wait at @@ -344,22 +344,18 @@ impl Session { /// `cancellation`. pub(super) fn connect_launched_cancellable( endpoint: &str, - launched: &mut Launched, + process: &Process, capability: &[u8; CAPABILITY_LEN], timeout: f64, cancellation: AttachmentCancellation, ) -> Result { - let pid = launched.id(); + let pid = process.reference().pid; Self::connect_unless_exited( endpoint, pid, capability, timeout, - || { - launched - .wait_timeout(Duration::ZERO) - .is_ok_and(|status| status.is_some()) - }, + || process.has_exited().is_ok_and(|exited| exited), Some(cancellation), ) } @@ -686,7 +682,7 @@ impl Session { match kind { APP_STDOUT | APP_STDERR => { // The guest agent forwards at most this much output, so more breaks the - // protocol, as `nvx::host::instance` also finds. + // protocol. if stdout.len() + stderr.len() + response.len() > MAX_OUTPUT_BYTES { return Err(violation(&format!( "managed guest exceeded the {MAX_OUTPUT_BYTES}-byte output limit" diff --git a/nvx-rs/src/host/cli/session/tests.rs b/nvx-rs/src/host/cli/session/tests.rs index 50f017f3d..34882c923 100644 --- a/nvx-rs/src/host/cli/session/tests.rs +++ b/nvx-rs/src/host/cli/session/tests.rs @@ -827,6 +827,89 @@ fn check_waits_for_openvmm_that_exits(endpoint: &str) { assert_ne!(checks, 0); } +/// A process that runs for a minute unless it is killed, which dropping it kills and reaps. +#[cfg(any(target_os = "linux", windows))] +struct Sleeper(std::process::Child); + +#[cfg(any(target_os = "linux", windows))] +impl Sleeper { + /// Spawns the sleeper under a `SpawnLock`, so that on Windows it holds no handle of a VMM that + /// another test spawns meanwhile. + fn spawn() -> Self { + let mut command = if cfg!(windows) { + let mut command = std::process::Command::new("ping"); + command.args(["-n", "60", "127.0.0.1"]); + command + } else { + let mut command = std::process::Command::new("sleep"); + command.arg("60"); + command + }; + command + .stdin(std::process::Stdio::null()) + .stdout(std::process::Stdio::null()); + Self(crate::host::tests::spawn_locked(&mut command).unwrap()) + } + + /// Returns the verified process of the sleeper. + fn process(&self) -> Process { + use crate::host::process::{BootId, Opened, ProcessRef}; + + let reference = ProcessRef { + pid: self.0.id(), + start_time: platform::process_start_time(self.0.id()).unwrap().unwrap(), + boot: BootId::current().unwrap(), + }; + let Opened::Running(process) = Process::open(&reference).unwrap() else { + panic!("the sleeper runs"); + }; + process + } +} + +#[cfg(any(target_os = "linux", windows))] +impl Drop for Sleeper { + fn drop(&mut self) { + let _ = self.0.kill(); + let _ = self.0.wait(); + } +} + +/// Checks the wait for `endpoint`, which never appears, of a cold run whose OpenVMM is the +/// verified process that [`Session::connect_launched_cancellable`] receives: the wait goes on +/// while the process runs, and ends at once, as a closed endpoint, once it exits, rather than at +/// the timeout. +#[cfg(any(target_os = "linux", windows))] +fn check_launched_waits_end_once_their_process_exits(endpoint: &str) { + let mut sleeper = Sleeper::spawn(); + let process = sleeper.process(); + let started = Instant::now(); + let failure = thread::scope(|scope| { + let waiting = scope.spawn(|| { + Session::connect_launched_cancellable( + endpoint, + &process, + &CAPABILITY, + 60.0, + AttachmentCancellation::default(), + ) + .err() + }); + thread::sleep(Duration::from_millis(500)); + let ended_early = waiting.is_finished(); + sleeper.0.kill().unwrap(); + sleeper.0.wait().unwrap(); + assert!(!ended_early, "the wait ended while OpenVMM ran"); + waiting.join().unwrap().unwrap() + }); + assert!( + started.elapsed() < Duration::from_secs(30), + "{:?}", + started.elapsed() + ); + assert!(matches!(failure, Exit::EndpointClosed), "{failure:?}"); +} + #[cfg(any(target_os = "linux", windows))] fn check_forward_failure_during_connect(endpoint: &str) { let (attempted, attempt) = mpsc::sync_channel(1); @@ -1211,6 +1294,14 @@ mod endpoints { check_waits_for_openvmm_that_exits(path.to_str().unwrap()); } + #[test] + #[cfg(target_os = "linux")] + fn launched_waits_end_once_their_process_exits() { + let directory = tempfile::tempdir().unwrap(); + let path = directory.path().join("control.sock"); + check_launched_waits_end_once_their_process_exits(path.to_str().unwrap()); + } + #[test] #[cfg(target_os = "linux")] fn forward_failure_cancels_the_connection_before_its_timeout() { @@ -1446,6 +1537,13 @@ mod endpoints { check_waits_for_openvmm_that_exits(r"\\.\pipe\nvx-cli-session-test-exited"); } + #[test] + fn launched_waits_end_once_their_process_exits() { + check_launched_waits_end_once_their_process_exits( + r"\\.\pipe\nvx-cli-session-test-launched-exited", + ); + } + #[test] fn forward_failure_cancels_the_connection_before_its_timeout() { check_forward_failure_during_connect(r"\\.\pipe\nvx-cli-session-test-cancelled"); diff --git a/nvx-rs/src/host/vm.rs b/nvx-rs/src/host/vm.rs index 27c66511f..013d250b4 100644 --- a/nvx-rs/src/host/vm.rs +++ b/nvx-rs/src/host/vm.rs @@ -863,7 +863,7 @@ pub struct Launched { pub capability: Capability, /// The preparation's same restore-ready listener, absent for a cold launch. pub ready: Option, - /// Retains only a foreground caller's child for status and control attachment. + /// Retains only a foreground caller's child, for its status. #[cfg(feature = "cli")] child: ForegroundChild, } @@ -892,8 +892,8 @@ impl Launched { .map(|status| status as i32)) } - /// Gives the CLI connector the same owned child whose exit it observes. - pub(crate) fn child_mut(&mut self) -> io::Result<&mut spawn::Launched> { + /// Returns the owned child, whose status [`Self::wait_status`] polls. + fn child_mut(&mut self) -> io::Result<&mut spawn::Launched> { self.child .0 .as_mut()