diff --git a/src/dist/download.rs b/src/dist/download.rs index d97db502cc..14d2c3a7b8 100644 --- a/src/dist/download.rs +++ b/src/dist/download.rs @@ -152,6 +152,8 @@ impl<'a> DownloadCfg<'a> { } } else { debug!(url = url.as_ref(), "checksum passed"); + #[cfg(feature = "test")] + self.process.checkpoint(CHECKPOINT_DOWNLOAD_BEFORE_RENAME); utils::rename( "downloaded", &partial_file_path, @@ -543,3 +545,6 @@ impl ops::Deref for File { self.path.as_path() } } + +#[cfg(feature = "test")] +pub const CHECKPOINT_DOWNLOAD_BEFORE_RENAME: &str = "download-before-rename"; diff --git a/src/test/clitools.rs b/src/test/clitools.rs index b7a81d4a05..77d373268d 100644 --- a/src/test/clitools.rs +++ b/src/test/clitools.rs @@ -7,12 +7,12 @@ use std::{ env::{self, consts::EXE_SUFFIX}, ffi::{OsStr, OsString}, fmt::Debug, - fs, + fs::{self, File}, io::{self, Write}, mem, ops::{Deref, DerefMut}, path::{Path, PathBuf}, - process::{Child, Command, ExitStatus}, + process::{Child, Command, ExitStatus, Stdio}, string::FromUtf8Error, sync::{Arc, LazyLock, RwLock, RwLockWriteGuard}, thread, @@ -1001,7 +1001,7 @@ impl CliTestContext { } /// Run a rustup command until it reaches `checkpoint` and terminate it. - pub fn kill_at>(&self, checkpoint: &str, args: impl AsRef<[S]>) -> ExitStatus { + pub fn kill_at>(&self, checkpoint: &str, args: impl AsRef<[S]>) -> Assert { self.spawn_at(checkpoint, args).kill() } @@ -1034,20 +1034,30 @@ impl CliTestContext { ); cmd.env(CHECKPOINT_ENV, checkpoint); - let mut parked = { + let mut parked = ParkedChild { + marker: marker.clone(), + child: None, + }; + parked.child = Some({ + // Here we are using files instead of pipes, so that a lingering grandchild cannot block us + // from collecting the output once the command is gone. + cmd.stdout(Stdio::from( + File::create(parked.output_path("stdout")) + .expect("failed to create stdout file for checkpoint test"), + )); + cmd.stderr(Stdio::from( + File::create(parked.output_path("stderr")) + .expect("failed to create stderr file for checkpoint test"), + )); + // 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(); - ParkedChild { - child: Some({ - cmd.spawn() - .expect("failed to start command for checkpoint test") - }), - marker: marker.clone(), - } - }; + cmd.spawn() + .expect("failed to start command for checkpoint test") + }); let deadline = Instant::now() + StdDuration::from_secs(30); loop { @@ -1150,7 +1160,7 @@ impl Drop for WorkDirGuard<'_> { /// A spawned rustup command parked at a checkpoint. /// /// The spawned command is created by [`CliTestContext::spawn_at`] and killed on drop unless -/// [`ParkedChild::kill`] was called first. +/// [`ParkedChild::kill`] or [`ParkedChild::resume`] was called first. #[must_use] pub struct ParkedChild { child: Option, @@ -1159,7 +1169,7 @@ pub struct ParkedChild { impl ParkedChild { /// Terminate and reap the parked command immediately. - pub fn kill(mut self) -> ExitStatus { + pub fn kill(mut self) -> Assert { let mut child = self.child.take().unwrap(); child .kill() @@ -1167,18 +1177,43 @@ impl ParkedChild { let status = child .wait() .expect("failed to reap command after checkpoint"); - remove_checkpoint_marker(&self.marker); - status + ensure_removed("checkpoint marker", &self.marker); + self.finish(status) } /// Resume the parked command and wait for it to finish. - pub fn resume(mut self) -> ExitStatus { - remove_checkpoint_marker(&self.marker); - self.child + pub fn resume(mut self) -> Assert { + ensure_removed("checkpoint marker", &self.marker); + let status = self + .child .take() .unwrap() .wait() - .expect("failed to reap resumed checkpoint command") + .expect("failed to reap resumed checkpoint command"); + self.finish(status) + } + + /// Reaps the command and collects what it printed so far into an [`Assert`]. + fn finish(self, status: ExitStatus) -> Assert { + let output = SanitizedOutput::try_from(Output { + status: status.code(), + stdout: fs::read(self.output_path("stdout")) + .expect("failed to read stdout of parked command"), + stderr: fs::read(self.output_path("stderr")) + .expect("failed to read stderr of parked command"), + }) + .unwrap(); + + println!("parked command finished"); + println!("status: {:?}", output.status); + println!("stdout:\n====\n{}\n====\n", output.stdout); + println!("stderr:\n====\n{}\n====\n", output.stderr); + + Assert::new(output) + } + + fn output_path(&self, name: &str) -> PathBuf { + self.marker.with_extension(name) } } @@ -1189,15 +1224,17 @@ impl Drop for ParkedChild { }; let _ = child.kill(); let _ = child.wait(); - remove_checkpoint_marker(&self.marker); + ensure_removed("checkpoint marker", &self.marker); + ensure_removed("stdout of parked command", &self.output_path("stdout")); + ensure_removed("stderr of parked command", &self.output_path("stderr")); } } -fn remove_checkpoint_marker(marker: &Path) { - if let Err(error) = fs::remove_file(marker) +fn ensure_removed(name: &str, path: &Path) { + if let Err(error) = fs::remove_file(path) && error.kind() != io::ErrorKind::NotFound { - panic!("failed to remove checkpoint marker: {error}"); + panic!("failed to remove {name}: {error}"); } } diff --git a/tests/suite/cli_crash.rs b/tests/suite/cli_crash.rs index 70c5c95498..114ff0eb5a 100644 --- a/tests/suite/cli_crash.rs +++ b/tests/suite/cli_crash.rs @@ -1,16 +1,19 @@ use rustup::{ - dist::manifestation::CHECKPOINT_UPDATE_BEFORE_METADATA, + dist::{ + download::CHECKPOINT_DOWNLOAD_BEFORE_RENAME, + 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 status = cx.kill_at( + cx.kill_at( CHECKPOINT_UPDATE_BEFORE_METADATA, ["rustup", "toolchain", "install", "nightly"], - ); - assert!(!status.success()); + ) + .is_err(); cx.config .expect(["rustup", "toolchain", "install", "nightly"]) @@ -21,3 +24,312 @@ async fn interrupted_install_can_be_retried() { .await .is_ok(); } + +// TODO: A modification killed in the middle leaves files behind that the next attempt trips over, +// since the metadata that would have accounted for them was never written. +// +// Unlike [`interrupted_install_can_be_retried()`], there is no "partially installed toolchain" +// recovery path for components, so the toolchain stays stuck. +// +// See: +#[tokio::test] +async fn interrupted_component_add_cannot_be_retried() { + let cx = CliTestContext::new(Scenario::SimpleV2).await; + cx.config + .expect(["rustup", "toolchain", "install", "nightly"]) + .await + .is_ok(); + + cx.kill_at( + CHECKPOINT_UPDATE_BEFORE_METADATA, + ["rustup", "component", "add", "rust-src"], + ) + .is_err(); + + cx.config + .expect(["rustup", "component", "add", "rust-src"]) + .await + .with_stderr(snapbox::str![[r#" +... +error: failed to install component: 'rust-src', detected conflict: 'lib/rustlib/src/rust-src/foo.rs' +... +"#]]) + .is_err(); +} + +// TODO: `rustup component add` races against another `rustup component add` of the same component. +// +// The second process cannot tell that the first one is already installing the component, so it +// tries to install it again on top of the first one's files. +// +// See: +#[tokio::test] +async fn concurrent_component_add_conflicts() { + let cx = CliTestContext::new(Scenario::SimpleV2).await; + cx.config + .expect(["rustup", "toolchain", "install", "nightly"]) + .await + .is_ok(); + + let parked = cx.spawn_at( + CHECKPOINT_UPDATE_BEFORE_METADATA, + ["rustup", "component", "add", "rust-src"], + ); + cx.config + .expect(["rustup", "component", "add", "rust-src"]) + .await + .with_stderr(snapbox::str![[r#" +... +error: failed to install component: 'rust-src', detected conflict: 'lib/rustlib/src/rust-src/foo.rs' +... +"#]]) + .is_err(); + + parked.resume().is_ok(); +} + +// TODO: This failure is similar to [`concurrent_component_add_conflicts()`], but on a component +// that provides a binary in `/bin`. +// +// See: +#[tokio::test] +async fn concurrent_component_add_conflicts_with_bin() { + let cx = CliTestContext::new(Scenario::SimpleV2).await; + cx.config + .expect(["rustup", "toolchain", "install", "nightly"]) + .await + .is_ok(); + cx.config + .expect(["rustup", "component", "remove", "cargo"]) + .await + .is_ok(); + + let parked = cx.spawn_at( + CHECKPOINT_UPDATE_BEFORE_METADATA, + ["rustup", "component", "add", "cargo"], + ); + cx.config + .expect(["rustup", "component", "add", "cargo"]) + .await + .with_stderr(snapbox::str![[r#" +... +error: failed to install component: 'cargo-[HOST_TUPLE]', detected conflict: 'bin/cargo[..]' +... +"#]]) + .is_err(); + + parked.resume().is_ok(); +} + +// TODO: A proxy launched while a modification is in flight. During this time, it sees a toolchain +// whose files and metadata disagree. +// +// In this test, the `cargo` files are already gone, but the toolchain is still advertised as +// having the `cargo` component, hence the unhelpful message. +// +// See: +#[tokio::test] +async fn proxy_during_component_removal_sees_inconsistent_toolchain() { + let cx = CliTestContext::new(Scenario::SimpleV2).await; + cx.config + .expect(["rustup", "toolchain", "install", "nightly"]) + .await + .is_ok(); + + let parked = cx.spawn_at( + CHECKPOINT_UPDATE_BEFORE_METADATA, + ["rustup", "component", "remove", "cargo"], + ); + cx.config + .expect(["cargo", "--version"]) + .await + .with_stderr(snapbox::str![[r#" +error: the 'cargo[EXE]' binary, normally provided by the 'cargo' component, is not applicable to the 'nightly-[HOST_TUPLE]' toolchain + +"#]]) + .is_err(); + + parked.resume().is_ok(); + cx.config + .expect(["cargo", "--version"]) + .await + .with_stderr(snapbox::str![[r#" +error: 'cargo[EXE]' is not installed for the toolchain 'nightly-[HOST_TUPLE]'. +... +"#]]) + .is_err(); +} + +// TODO: When uninstalling a toolchain while it is being installed, the process installing the +// toolchain is not aware of the race, so it happily considers a toolchain directory that lacks most +// of its files as fully installed. +// +// See: +#[tokio::test] +async fn uninstall_during_install_leaves_broken_toolchain() { + let cx = CliTestContext::new(Scenario::SimpleV2).await; + + let parked = cx.spawn_at( + CHECKPOINT_UPDATE_BEFORE_METADATA, + ["rustup", "toolchain", "install", "nightly"], + ); + cx.config + .expect(["rustup", "toolchain", "uninstall", "nightly"]) + .await + .is_ok(); + parked.resume().is_ok(); + + // The toolchain shows up as installed... + cx.config + .expect(["rustup", "toolchain", "list"]) + .await + .with_stdout(snapbox::str![[r#" +nightly-[HOST_TUPLE] (active, default) + +"#]]) + .is_ok(); + // ... but it is not usable. + cx.config + .expect(["rustc", "--version"]) + .await + .with_stderr(snapbox::str![[r#" +error: the 'rustc[EXE]' binary, normally provided by the 'rustc' component, is not applicable to the 'nightly-[HOST_TUPLE]' toolchain + +"#]]) + .is_err(); +} + +// TODO: Two `rustup component add` of *different* components lose one of the writes to the +// toolchain's component list. +// +// Each process reads the list at the start, then writes back its own view of it at the end. The +// first process to finish has its component forgotten by the second one, although its files are on +// disk. Adding it again then trips over its own files. +// +// See: +#[tokio::test] +async fn concurrent_component_adds_lose_config_write() { + let cx = CliTestContext::new(Scenario::SimpleV2).await; + cx.config + .expect(["rustup", "toolchain", "install", "nightly"]) + .await + .is_ok(); + + let parked = cx.spawn_at( + CHECKPOINT_UPDATE_BEFORE_METADATA, + ["rustup", "component", "add", "rust-src"], + ); + cx.config + .expect(["rustup", "component", "add", "rust-analysis"]) + .await + .is_ok(); + parked.resume().is_ok(); + + // Both components should be listed here, but `rust-analysis` is gone. + cx.config + .expect(["rustup", "component", "list", "--installed"]) + .await + .with_stdout(snapbox::str![[r#" +cargo-[HOST_TUPLE] +rust-docs-[HOST_TUPLE] +rust-src +rust-std-[HOST_TUPLE] +rustc-[HOST_TUPLE] + +"#]]) + .is_ok(); + cx.config + .expect(["rustup", "component", "add", "rust-analysis"]) + .await + .with_stderr(snapbox::str![[r#" +... +error: failed to install component: 'rust-analysis-[HOST_TUPLE]', detected conflict: 'lib/rustlib/[HOST_TUPLE]/analysis/libfoo.json' +... +"#]]) + .is_err(); +} + +// TODO: A component removal killed in the middle leaves a toolchain that still claims to have the +// component whose files are already gone. +// +// `rustup component add` then considers the component up to date, so the only way out is to +// remove it first and then add it again. +// +// See: +#[tokio::test] +async fn interrupted_component_remove_leaves_stale_component() { + let cx = CliTestContext::new(Scenario::SimpleV2).await; + cx.config + .expect(["rustup", "toolchain", "install", "nightly"]) + .await + .is_ok(); + + cx.kill_at( + CHECKPOINT_UPDATE_BEFORE_METADATA, + ["rustup", "component", "remove", "cargo"], + ) + .is_err(); + + cx.config + .expect(["cargo", "--version"]) + .await + .with_stderr(snapbox::str![[r#" +error: the 'cargo[EXE]' binary, normally provided by the 'cargo' component, is not applicable to the 'nightly-[HOST_TUPLE]' toolchain + +"#]]) + .is_err(); + cx.config + .expect(["rustup", "component", "list", "--installed"]) + .await + .with_stdout(snapbox::str![[r#" +cargo-[HOST_TUPLE] +... +"#]]) + .is_ok(); + cx.config + .expect(["rustup", "component", "add", "cargo"]) + .await + .with_stderr(snapbox::str![[r#" +info: component cargo is up to date + +"#]]) + .is_ok(); + cx.config.expect(["cargo", "--version"]).await.is_err(); + + cx.config + .expect(["rustup", "component", "remove", "cargo"]) + .await + .is_ok(); + cx.config + .expect(["rustup", "component", "add", "cargo"]) + .await + .is_ok(); + cx.config.expect(["cargo", "--version"]).await.is_ok(); +} + +// TODO: Two processes downloading the same file share the same `.partial` path. Whichever finishes +// first renames it to its final name, and the other one then fails to find it. +// +// See: +#[tokio::test] +async fn concurrent_download_of_same_file_fails() { + let cx = CliTestContext::new(Scenario::SimpleV2).await; + + let parked = cx.spawn_at( + CHECKPOINT_DOWNLOAD_BEFORE_RENAME, + ["rustup", "toolchain", "install", "nightly"], + ); + cx.config + .expect(["rustup", "toolchain", "install", "nightly"]) + .await + .is_ok(); + + parked + .resume() + .with_stderr(snapbox::str![[r#" +... +error: component download failed for [..]: could not rename 'downloaded' file from '[..].partial' to '[..]':[..] +... +"#]]) + .is_err(); +} diff --git a/tests/suite/cli_self_upd.rs b/tests/suite/cli_self_upd.rs index 20ab436055..0676602f1c 100644 --- a/tests/suite/cli_self_upd.rs +++ b/tests/suite/cli_self_upd.rs @@ -738,7 +738,7 @@ async fn managed_updater_survives_concurrent_proxy_cleanup() { cx.config.expect(["rustc", "--version"]).await.is_ok(); assert!(updater.exists()); - assert!(parked.resume().success()); + parked.resume().is_ok(); wait_for_completed_update(&cx.config.rustupdir.rustupdir); assert_ne!(before_hash, calc_hash(&rustup)); } @@ -759,12 +759,13 @@ async fn self_update_replacement_survives_proxy_cleanup() { cx.config.expect(["rustc", "--version"]).await.is_ok(); - let status = parked.resume(); + let resumed = parked.resume(); assert!( rustup.exists(), - "concurrent proxy removed the installed rustup during self-update ({status})" + "concurrent proxy removed the installed rustup during self-update (status {:?})", + resumed.output.status, ); - assert!(status.success(), "self-update failed: {status}"); + resumed.is_ok(); assert_ne!(before_hash, calc_hash(&rustup)); }