diff --git a/src/dist/manifestation.rs b/src/dist/manifestation.rs index ce763b93c9..cae9478bc1 100644 --- a/src/dist/manifestation.rs +++ b/src/dist/manifestation.rs @@ -21,8 +21,6 @@ use futures_util::{ use tokio::task::{JoinHandle, spawn_blocking}; use tracing::{debug, info, warn}; -#[cfg(feature = "test")] -use crate::test::checkpoint; use crate::{ diskio::{Executor, IO_CHUNK_SIZE, get_executor, unpack_ram}, dist::{ @@ -294,7 +292,9 @@ impl Manifestation { } #[cfg(feature = "test")] - checkpoint(download_cfg.process, "manifestation-update-before-metadata"); + download_cfg + .process + .checkpoint(CHECKPOINT_UPDATE_BEFORE_METADATA); // Install new distribution manifest let new_manifest_str = new_manifest.clone().stringify()?; @@ -884,3 +884,6 @@ impl ComponentInstall { tx } } + +#[cfg(feature = "test")] +pub const CHECKPOINT_UPDATE_BEFORE_METADATA: &str = "update-before-metadata"; diff --git a/src/test.rs b/src/test.rs index f58a781cf0..7c96bff611 100644 --- a/src/test.rs +++ b/src/test.rs @@ -28,7 +28,7 @@ pub use crate::cli::self_update::{RegistryGuard, RegistryValueId, USER_PATH, get mod clitools; pub use clitools::{ - Assert, CliTestContext, Config, SanitizedOutput, Scenario, SelfUpdateTestContext, + Assert, CliTestContext, Config, ParkedChild, SanitizedOutput, Scenario, SelfUpdateTestContext, output_release_file, print_command, print_indented, }; pub(super) mod dist; @@ -36,24 +36,26 @@ pub use dist::DistContext; pub(super) mod mock; pub use mock::{MockComponentBuilder, MockFile, MockInstallerBuilder}; -/// Signal that a selected test checkpoint has been reached, then wait for the -/// test driver to terminate this process. -pub(crate) fn checkpoint(process: &Process, name: &str) { - if process.var(CHECKPOINT_ENV).as_deref() != Ok(name) { - return; - } +impl Process { + /// Signal that a selected test checkpoint has been reached, then wait for the + /// test driver to terminate this process. + pub(crate) fn checkpoint(&self, name: &str) { + if self.var(CHECKPOINT_ENV).as_deref() != Ok(name) { + return; + } - let rustup_home = process - .rustup_home() - .expect("selected test checkpoint requires RUSTUP_HOME"); - let test_root = rustup_home - .parent() - .expect("test RUSTUP_HOME must be inside the test root"); - fs::write(checkpoint_path(test_root, name), name) - .expect("failed to write test checkpoint marker"); - - loop { - std::thread::park(); + let rustup_home = self + .rustup_home() + .expect("selected test checkpoint requires RUSTUP_HOME"); + let test_root = rustup_home + .parent() + .expect("test RUSTUP_HOME must be inside the test root"); + fs::write(checkpoint_path(test_root, name), name) + .expect("failed to write test checkpoint marker"); + + loop { + std::thread::park(); + } } } diff --git a/src/test/clitools.rs b/src/test/clitools.rs index 46c2f23582..b4a344cd45 100644 --- a/src/test/clitools.rs +++ b/src/test/clitools.rs @@ -12,7 +12,7 @@ use std::{ mem, ops::{Deref, DerefMut}, path::{Path, PathBuf}, - process::{Command, ExitStatus}, + process::{Child, Command, ExitStatus}, string::FromUtf8Error, sync::{Arc, LazyLock, RwLock, RwLockWriteGuard}, thread, @@ -975,7 +975,22 @@ impl CliTestContext { /// Run a rustup command until it reaches `checkpoint`, then terminate it /// without giving destructors an opportunity to run. - pub fn kill_at_checkpoint(&self, mut command: Command, checkpoint: &str) -> ExitStatus { + pub fn kill_at + Clone + Debug>( + &self, + checkpoint: &str, + args: impl AsRef<[S]>, + ) -> ExitStatus { + self.spawn_at(checkpoint, args).kill() + } + + /// Run a rustup command until it reaches `checkpoint` and keep it parked + /// there, e.g. to observe or race the paused operation from another + /// process. The returned guard kills the command when dropped. + pub fn spawn_at + Clone + Debug>( + &self, + checkpoint: &str, + args: impl AsRef<[S]>, + ) -> ParkedChild { let marker = checkpoint_path(&self.config.test_root_dir, checkpoint); if let Err(error) = fs::remove_file(&marker) && error.kind() != io::ErrorKind::NotFound @@ -983,36 +998,49 @@ impl CliTestContext { panic!("failed to remove stale checkpoint marker: {error}"); } - command.env(CHECKPOINT_ENV, checkpoint); - let mut child = command - .spawn() - .expect("failed to start command for checkpoint test"); + let (program, args) = args + .as_ref() + .split_first() + .expect("args should not be empty"); + let mut cmd = self.config.cmd( + program + .as_ref() + .to_str() + .expect("invalid UTF-8 in program name"), + args, + ); + cmd.env(CHECKPOINT_ENV, checkpoint); + + let child = { + // The lock covers the spawn itself. It must not be held while the + // child stays parked: a writer queued behind it would then block + // every command spawned in the meantime, including the ones a test + // wants to run against the parked process. + let _lock = CMD_LOCK.read().unwrap(); + cmd.spawn() + .expect("failed to start command for checkpoint test") + }; + let mut parked = ParkedChild { child: Some(child) }; let deadline = Instant::now() + StdDuration::from_secs(10); loop { if matches!(fs::read_to_string(&marker), Ok(contents) if contents == checkpoint) { - break; + break parked; } - if let Some(status) = child + if let Some(status) = parked + .child + .as_mut() + .unwrap() .try_wait() .expect("failed to query checkpoint test command") { panic!("command exited before reaching checkpoint {checkpoint:?}: {status}"); } if Instant::now() >= deadline { - let _ = child.kill(); - let _ = child.wait(); panic!("timed out waiting for checkpoint {checkpoint:?}"); } thread::sleep(StdDuration::from_millis(10)); } - - child - .kill() - .expect("failed to terminate command at checkpoint"); - child - .wait() - .expect("failed to reap command after checkpoint") } /// Move the dist server to the specified scenario and restore it @@ -1091,6 +1119,37 @@ impl Drop for WorkDirGuard<'_> { } } +/// A rustup command spawned by [`CliTestContext::spawn_at`], parked at a +/// checkpoint. Killed on drop unless [`ParkedChild::kill`] was called first. +#[must_use] +pub struct ParkedChild { + child: Option, +} + +impl ParkedChild { + /// Terminate the parked command without giving destructors an opportunity + /// to run, then reap it. + pub fn kill(mut self) -> ExitStatus { + let mut child = self.child.take().unwrap(); + child + .kill() + .expect("failed to terminate command at checkpoint"); + child + .wait() + .expect("failed to reap command after checkpoint") + } +} + +impl Drop for ParkedChild { + fn drop(&mut self) { + let Some(mut child) = self.child.take() else { + return; + }; + let _ = child.kill(); + let _ = child.wait(); + } +} + #[must_use] pub struct DistDirGuard<'a> { inner: &'a mut CliTestContext, diff --git a/tests/suite/cli_crash.rs b/tests/suite/cli_crash.rs index f954310384..70c5c95498 100644 --- a/tests/suite/cli_crash.rs +++ b/tests/suite/cli_crash.rs @@ -1,11 +1,15 @@ -use rustup::test::{CliTestContext, Scenario}; +use rustup::{ + dist::manifestation::CHECKPOINT_UPDATE_BEFORE_METADATA, + test::{CliTestContext, Scenario}, +}; #[tokio::test] async fn interrupted_install_can_be_retried() { let cx = CliTestContext::new(Scenario::SimpleV2).await; - let command = cx.config.cmd("rustup", ["toolchain", "install", "nightly"]); - - let status = cx.kill_at_checkpoint(command, "manifestation-update-before-metadata"); + let status = cx.kill_at( + CHECKPOINT_UPDATE_BEFORE_METADATA, + ["rustup", "toolchain", "install", "nightly"], + ); assert!(!status.success()); cx.config