refactor(test/clitools)!: refine semantics of CliTestContext::kill_at()

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";

src/test.rs

@@ -28,32 +28,34 @@ 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;
 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();
+        }
     }
 }
 

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,44 +975,72 @@ 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<S: AsRef<OsStr> + 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<S: AsRef<OsStr> + 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
         {
             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<Child>,
+}
+
+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,

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