diff --git a/src/dist/mod.rs b/src/dist/mod.rs index f348188943..d8f49d62dc 100644 --- a/src/dist/mod.rs +++ b/src/dist/mod.rs @@ -17,7 +17,7 @@ use itertools::Itertools; use regex::{Match, Regex}; use serde::{Deserialize, Serialize}; use thiserror::Error as ThisError; -use tracing::{debug, info, warn}; +use tracing::{debug, info}; use crate::{ config::Cfg, @@ -944,17 +944,10 @@ impl<'cfg, 'a> DistOptions<'cfg, 'a> { pub(crate) async fn install_into( &self, prefix: &InstallPrefix, + update_hash: &Path, manifest: Option, ) -> Result> { let fresh_install = !prefix.path().exists(); - // fresh_install means the toolchain isn't present, but hash_exists means there is a stray hash file - if fresh_install && self.update_hash.exists() { - warn!( - "removing stray hash file in order to continue: {}", - self.update_hash.display() - ); - std::fs::remove_file(&self.update_hash)?; - } let mut fetched = String::new(); let mut first_err = None; @@ -1007,7 +1000,7 @@ impl<'cfg, 'a> DistOptions<'cfg, 'a> { let res = loop { let result = try_update_from_dist_( &self.dl_cfg, - &self.update_hash, + update_hash, &toolchain, match self.exists { false => Some(self.profile), diff --git a/src/install.rs b/src/install.rs index bb5e275c8b..0dfd596ea0 100644 --- a/src/install.rs +++ b/src/install.rs @@ -1,9 +1,12 @@ //! Installation and upgrade of both distribution-managed and local //! toolchains -use std::path::{Path, PathBuf}; +use std::{ + fs, + path::{Path, PathBuf}, +}; -use anyhow::Result; -use tracing::debug; +use anyhow::{Context, Result, bail}; +use tracing::{debug, warn}; use crate::{ config::Cfg, @@ -13,6 +16,105 @@ use crate::{ utils, }; +#[cfg(feature = "test")] +use crate::test::checkpoint; + +impl StagedToolchain { + fn new(destination: &Path) -> Result { + let parent = destination + .parent() + .expect("toolchain destination must have a parent"); + let name = destination + .file_name() + .and_then(|name| name.to_str()) + .expect("toolchain destination must have a UTF-8 base name"); + utils::ensure_dir_exists("toolchains", parent)?; + + // The stage lives at a deterministic path so that interrupted + // installs of the same toolchain reuse a single staging directory + // instead of accumulating abandoned ones. + let root = parent.join(format!("{STAGING_DIR_PREFIX}{name}")); + utils::ensure_dir_exists("staging toolchain", &root)?; + + // The lock is held for the lifetime of the stage and released by the + // OS if this process dies, so an abandoned stage can be reclaimed + // safely. Blocking on (rather than failing under) contention and + // serializing whole operations is left to the locking work tracked + // in rust-lang/rustup#988. + let lock = fs::File::create(root.join(STAGING_LOCK_FILE)) + .with_context(|| format!("failed to create lock file in {}", root.display()))?; + match lock.try_lock() { + Ok(()) => {} + Err(fs::TryLockError::WouldBlock) => bail!( + "toolchain '{name}' is already being installed by another rustup process; \ + wait for it to finish and retry" + ), + Err(fs::TryLockError::Error(error)) => { + return Err(error).with_context(|| { + format!("failed to lock staging directory {}", root.display()) + }); + } + } + + // This process owns the stage: clear anything left over from an + // interrupted attempt so installation starts from a clean prefix. + let prefix = root.join("toolchain"); + if utils::path_exists(&prefix) { + utils::remove_dir("staging toolchain", &prefix)?; + } + utils::ensure_file_removed("staging update hash", &root.join(STAGING_HASH_FILE))?; + + Ok(Self { + root, + prefix, + lock: Some(lock), + }) + } + + fn prefix(&self) -> &Path { + &self.prefix + } + + /// Path of the update hash tracked next to the stage. The hash under + /// `$RUSTUP_HOME/update-hashes` is alias-scoped metadata and must not be + /// touched before the staged toolchain is published. + fn update_hash(&self) -> PathBuf { + self.root.join(STAGING_HASH_FILE) + } + + fn publish(self, destination: &Path) -> Result<()> { + // Staging is a sibling of the destination inside `toolchains/`, so + // publication must be a same-filesystem rename. Never permit the + // copy-and-delete fallback. + match utils::rename("toolchain", &self.prefix, destination, false) { + Err(error) if utils::path_exists(destination) => Err(error).with_context(|| { + format!( + "toolchain directory {} was created by another process \ + while this installation was in progress", + destination.display() + ) + }), + result => result, + } + } +} + +impl Drop for StagedToolchain { + fn drop(&mut self) { + // Release the lock before removing the stage: an open handle inside + // the directory can prevent its removal on Windows. + drop(self.lock.take()); + if utils::path_exists(&self.root) + && let Err(error) = utils::remove_dir("staging toolchain", &self.root) + { + warn!( + path = %self.root.display(), + "could not remove staging toolchain: {error}" + ); + } + } +} + #[derive(Clone, Debug)] pub(crate) enum UpdateStatus { Installed, @@ -53,8 +155,9 @@ impl InstallMethod<'_, '_> { _ => debug!("updating existing install for '{}'", self.dest_basename()), } - debug!("toolchain directory: {}", self.dest_path().display()); - let updated = self.run(&self.dest_path(), manifest).await?; + let destination = self.dest_path(); + debug!("toolchain directory: {}", destination.display()); + let updated = self.run(&destination, manifest).await?; let status = match updated { false => { @@ -102,20 +205,55 @@ impl InstallMethod<'_, '_> { utils::symlink_dir(src, path)?; Ok(true) } - InstallMethod::Dist(opts) => { + // Updates modify the published toolchain in place, while fresh + // installs are staged and only published once complete. + InstallMethod::Dist(opts) if opts.exists => { let prefix = &InstallPrefix::from(path.to_owned()); - let maybe_new_hash = opts.install_into(prefix, manifest).await?; + let Some(hash) = opts + .install_into(prefix, &opts.update_hash, manifest) + .await? + else { + return Ok(false); + }; - if let Some(hash) = maybe_new_hash { - utils::write_file("update hash", &opts.update_hash, &hash)?; - Ok(true) - } else { - Ok(false) - } + utils::write_file("update hash", &opts.update_hash, &hash)?; + Ok(true) } + InstallMethod::Dist(opts) => Self::run_staged_dist(opts, path, manifest).await, } } + /// Constructs a fresh toolchain under a private stage and only publishes + /// it to `destination` once complete, so that an interruption can never + /// leave a partial toolchain behind at a selectable path. + async fn run_staged_dist( + opts: &DistOptions<'_, '_>, + destination: &Path, + manifest: Option, + ) -> Result { + let staging = StagedToolchain::new(destination)?; + let prefix = InstallPrefix::from(staging.prefix().to_owned()); + // A stray alias-scoped update hash (e.g. left behind by an + // interrupted uninstall) is deliberately ignored rather than removed: + // the staged install only ever reads the staging-local hash, and the + // alias-scoped one is rewritten after publication. + let staging_hash = staging.update_hash(); + let Some(hash) = opts.install_into(&prefix, &staging_hash, manifest).await? else { + return Ok(false); + }; + + #[cfg(feature = "test")] + checkpoint(opts.cfg.process, CHECKPOINT_INSTALL_BEFORE_PUBLISH); + + staging.publish(destination)?; + + #[cfg(feature = "test")] + checkpoint(opts.cfg.process, CHECKPOINT_INSTALL_AFTER_PUBLISH); + + utils::write_file("update hash", &opts.update_hash, &hash)?; + Ok(true) + } + fn cfg(&self) -> &Cfg<'_> { match self { InstallMethod::Copy { cfg, .. } => cfg, @@ -156,3 +294,32 @@ impl InstallMethod<'_, '_> { pub(crate) fn uninstall(path: &Path) -> Result<()> { utils::remove_dir("install", path) } + +/// Prefix of staging directory names inside `toolchains/`. `+` is not valid +/// at the start of a toolchain name, so stages are identifiable and excluded +/// by the existing toolchain enumeration. +pub const STAGING_DIR_PREFIX: &str = "+rustup-staging-"; + +/// Lock file inside a stage, held exclusively by the owning process. +const STAGING_LOCK_FILE: &str = "lock"; + +/// Update hash file inside a stage (see [`StagedToolchain::update_hash`]). +const STAGING_HASH_FILE: &str = "update-hash"; + +/// Checkpoint reached once a staged toolchain is complete, right before its +/// publication. +#[cfg(feature = "test")] +pub const CHECKPOINT_INSTALL_BEFORE_PUBLISH: &str = "install-before-publish"; + +/// Checkpoint reached right after publication, before the alias-scoped +/// update hash is written. +#[cfg(feature = "test")] +pub const CHECKPOINT_INSTALL_AFTER_PUBLISH: &str = "install-after-publish"; + +struct StagedToolchain { + root: PathBuf, + prefix: PathBuf, + /// Exclusive lock marking the stage as owned by a live process; `None` + /// only while dropping. + lock: Option, +} diff --git a/src/lib.rs b/src/lib.rs index 0daf2f6ede..7204044d69 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -75,7 +75,7 @@ mod download; pub mod env_var; pub mod errors; mod fallback_settings; -mod install; +pub mod install; pub mod process; mod settings; #[cfg(feature = "test")] diff --git a/tests/suite/cli_crash.rs b/tests/suite/cli_crash.rs index f954310384..a1399628dd 100644 --- a/tests/suite/cli_crash.rs +++ b/tests/suite/cli_crash.rs @@ -1,13 +1,200 @@ -use rustup::test::{CliTestContext, Scenario}; +use std::{fs, path::PathBuf, process::Command, time::Duration}; + +use rustup::install::{ + CHECKPOINT_INSTALL_AFTER_PUBLISH, CHECKPOINT_INSTALL_BEFORE_PUBLISH, STAGING_DIR_PREFIX, +}; +use rustup::test::{CliTestContext, Scenario, this_host_tuple}; +use wait_timeout::ChildExt; + +fn assert_completes_successfully(mut command: Command) { + let mut child = command.spawn().expect("failed to start command"); + let Some(status) = child + .wait_timeout(Duration::from_secs(10)) + .expect("failed to wait for command") + else { + let _ = child.kill(); + let _ = child.wait(); + panic!("command did not complete within 10 seconds"); + }; + assert!(status.success(), "command failed with status {status}"); +} + +fn nightly_path(cx: &CliTestContext) -> PathBuf { + cx.config + .rustupdir + .join("toolchains") + .join(format!("nightly-{}", this_host_tuple())) +} + +fn nightly_update_hash_path(cx: &CliTestContext) -> PathBuf { + cx.config + .rustupdir + .join("update-hashes") + .join(format!("nightly-{}", this_host_tuple())) +} + +fn nightly_staging_root(cx: &CliTestContext) -> PathBuf { + cx.config + .rustupdir + .join("toolchains") + .join(format!("{STAGING_DIR_PREFIX}nightly-{}", this_host_tuple())) +} + +fn staging_paths(cx: &CliTestContext) -> Vec { + fs::read_dir(cx.config.rustupdir.join("toolchains")) + .expect("failed to read toolchains directory") + .map(|entry| entry.expect("failed to read toolchains entry").path()) + .filter(|path| { + path.file_name() + .and_then(|name| name.to_str()) + .is_some_and(|name| name.starts_with(STAGING_DIR_PREFIX)) + }) + .collect() +} + +async fn assert_nightly_is_complete(cx: &CliTestContext) { + cx.config + .expect(["rustup", "+nightly", "component", "list", "--installed"]) + .await + .with_stdout(snapbox::str![[r#" +cargo-[HOST_TUPLE] +rust-docs-[HOST_TUPLE] +rust-std-[HOST_TUPLE] +rustc-[HOST_TUPLE] + +"#]]) + .is_ok(); + cx.config + .expect(["rustc", "+nightly", "--version"]) + .await + .with_stdout(snapbox::str![[r#" +1.3.0 (hash-nightly-2) + +"#]]) + .is_ok(); +} + +async fn assert_unpublished_install_can_be_retried(checkpoint: &str) { + let cx = CliTestContext::new(Scenario::SimpleV2).await; + let command = cx.config.cmd("rustup", ["toolchain", "install", "nightly"]); + + let status = cx.kill_at_checkpoint(command, checkpoint); + assert!(!status.success()); + assert!( + !nightly_path(&cx).exists(), + "an interrupted staging operation published the toolchain" + ); + assert_eq!(staging_paths(&cx), vec![nightly_staging_root(&cx)]); + cx.config + .expect(["rustup", "toolchain", "list"]) + .await + .without_stdout("rustup-staging") + .without_stdout("nightly") + .is_ok(); + + assert_completes_successfully(cx.config.cmd("rustup", ["toolchain", "install", "nightly"])); + assert!(nightly_path(&cx).is_dir()); + assert_nightly_is_complete(&cx).await; +} #[tokio::test] async fn interrupted_install_can_be_retried() { + assert_unpublished_install_can_be_retried(BEFORE_METADATA).await; +} + +#[tokio::test] +async fn interrupted_install_before_publication_can_be_retried() { + assert_unpublished_install_can_be_retried(CHECKPOINT_INSTALL_BEFORE_PUBLISH).await; +} + +#[tokio::test] +async fn interrupted_installs_do_not_accumulate_stages() { + let cx = CliTestContext::new(Scenario::SimpleV2).await; + + for _ in 0..2 { + let command = cx.config.cmd("rustup", ["toolchain", "install", "nightly"]); + let status = cx.kill_at_checkpoint(command, CHECKPOINT_INSTALL_BEFORE_PUBLISH); + assert!(!status.success()); + assert_eq!(staging_paths(&cx), vec![nightly_staging_root(&cx)]); + } + + assert_completes_successfully(cx.config.cmd("rustup", ["toolchain", "install", "nightly"])); + assert!(staging_paths(&cx).is_empty()); + assert_nightly_is_complete(&cx).await; +} + +#[tokio::test] +async fn stale_stage_is_reclaimed_by_the_next_install() { + let cx = CliTestContext::new(Scenario::SimpleV2).await; + let junk = nightly_staging_root(&cx) + .join("toolchain") + .join("bin") + .join("stale-junk"); + fs::create_dir_all(junk.parent().unwrap()).unwrap(); + fs::write(&junk, "junk").unwrap(); + fs::write(nightly_staging_root(&cx).join("update-hash"), "stale-hash").unwrap(); + + assert_completes_successfully(cx.config.cmd("rustup", ["toolchain", "install", "nightly"])); + assert!(staging_paths(&cx).is_empty()); + assert!( + !nightly_path(&cx).join("bin").join("stale-junk").exists(), + "contents of a stale stage leaked into the published toolchain" + ); + assert_nightly_is_complete(&cx).await; +} + +#[tokio::test] +async fn unpublished_install_does_not_change_update_hash() { + let cx = CliTestContext::new(Scenario::SimpleV2).await; + let update_hash = nightly_update_hash_path(&cx); + fs::create_dir_all(update_hash.parent().unwrap()).unwrap(); + fs::write(&update_hash, "stale-hash").unwrap(); + + let command = cx.config.cmd("rustup", ["toolchain", "install", "nightly"]); + let status = cx.kill_at_checkpoint(command, CHECKPOINT_INSTALL_BEFORE_PUBLISH); + + assert!(!status.success()); + assert_eq!(fs::read_to_string(update_hash).unwrap(), "stale-hash"); + assert!(!nightly_path(&cx).exists()); +} + +#[tokio::test] +async fn interrupted_install_after_publication_is_complete() { 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(command, CHECKPOINT_INSTALL_AFTER_PUBLISH); assert!(!status.success()); + assert!(nightly_path(&cx).is_dir()); + assert!(staging_paths(&cx).is_empty()); + assert_nightly_is_complete(&cx).await; + assert_completes_successfully(cx.config.cmd("rustup", ["toolchain", "install", "nightly"])); + assert_nightly_is_complete(&cx).await; +} + +#[tokio::test] +async fn failed_install_removes_staging_directory() { + let cx = CliTestContext::new(Scenario::UnavailableRls).await; + cx.config.set_current_dist_date("2015-01-01"); + cx.config + .expect(["rustup", "set", "profile", "complete"]) + .await + .is_ok(); + + cx.config + .expect(["rustup", "toolchain", "install", "nightly"]) + .await + .is_err(); + + assert!(!nightly_path(&cx).exists()); + assert!(staging_paths(&cx).is_empty()); +} + +#[tokio::test] +async fn interrupted_update_can_be_retried() { + let cx = CliTestContext::new(Scenario::ArchivesV2).await; + cx.config.set_current_dist_date("2015-01-01"); cx.config .expect(["rustup", "toolchain", "install", "nightly"]) .await @@ -15,5 +202,22 @@ async fn interrupted_install_can_be_retried() { cx.config .expect(["rustc", "+nightly", "--version"]) .await + .with_stdout(snapbox::str![[r#" +1.2.0 (hash-nightly-1) + +"#]]) .is_ok(); + + cx.config.set_current_dist_date("2015-01-02"); + let command = cx.config.cmd("rustup", ["update", "nightly"]); + let status = cx.kill_at_checkpoint(command, BEFORE_METADATA); + assert!(!status.success()); + + assert_completes_successfully(cx.config.cmd("rustup", ["update", "nightly"])); + + assert_nightly_is_complete(&cx).await; } + +// TODO(rust-lang/rustup#4966): import this from `manifestation` once the +// checkpoint constants are exported next to their checkpoint sites. +const BEFORE_METADATA: &str = "manifestation-update-before-metadata"; diff --git a/tests/suite/cli_v2.rs b/tests/suite/cli_v2.rs index c4a0efd495..aaece47b83 100644 --- a/tests/suite/cli_v2.rs +++ b/tests/suite/cli_v2.rs @@ -1949,9 +1949,11 @@ async fn remove_target_missing_update_hash() { .is_ok(); } -// Issue #1777 +// Issue #1777: a stray hash file must not prevent installation. Staged +// installs never consult the alias-scoped hash, so it is silently refreshed +// after publication rather than removed up front. #[tokio::test] -async fn warn_about_and_remove_stray_hash() { +async fn stray_hash_is_ignored_and_refreshed() { let mut cx = CliTestContext::new(Scenario::None).await; let mut hash_path = cx.config.rustupdir.join("update-hashes"); fs::create_dir_all(&hash_path).expect("Unable to make the update-hashes directory"); @@ -1965,12 +1967,13 @@ async fn warn_about_and_remove_stray_hash() { cx.config .expect(["rustup", "toolchain", "install", "nightly"]) .await - .with_stderr(snapbox::str![[r#" -... -warn: removing stray hash file in order to continue: [..]/update-hashes/nightly-[HOST_TUPLE] -... -"#]]) .is_ok(); + + let refreshed = fs::read_to_string(&hash_path).expect("Unable to read update-hash file"); + assert_ne!( + refreshed, "LEGITHASH", + "stray hash file was not refreshed after installation" + ); } fn make_component_unavailable(config: &Config, name: &str, target: String) {