diff --git a/src/cli/self_update.rs b/src/cli/self_update.rs index 2f8b8ca69a..3c540c4cc6 100644 --- a/src/cli/self_update.rs +++ b/src/cli/self_update.rs @@ -13,13 +13,12 @@ //! * update the PATH in a system-specific way //! * run the equivalent of `rustup default stable` //! -//! During upgrade (`rustup self upgrade`): +//! During upgrade (`rustup self update`): //! -//! * download rustup-init to $CARGO_HOME/bin/rustup-init -//! * run rustup-init with appropriate flags to indicate -//! this is a self-upgrade -//! * rustup-init copies bins and hardlinks into place. On windows -//! this happens *after* the upgrade command exits successfully. +//! * download rustup-init to a managed path under `$RUSTUP_HOME` +//! * run the downloaded binary in replacement mode +//! * atomically replace rustup and update its proxy links. On Windows +//! this happens after the update command exits. //! //! During uninstall (`rustup self uninstall`): //! @@ -77,29 +76,32 @@ use crate::{ #[macro_use] mod msg; +mod stage; +use stage::{PreparedUpdater, SelfUpdateLock}; + #[cfg(unix)] mod shell; #[cfg(unix)] mod unix; #[cfg(unix)] -use unix::{do_add_to_path, do_remove_from_path}; +pub(crate) use unix::self_replace; #[cfg(unix)] -pub(crate) use unix::{run_update, self_replace}; +use unix::{do_add_to_path, do_remove_from_path, run_update}; #[cfg(windows)] mod windows; #[cfg(windows)] pub use windows::complete_windows_uninstall; +#[cfg(windows)] +pub(crate) use windows::self_replace; #[cfg(all(windows, feature = "test"))] pub use windows::{RUSTUP_REGISTRY_TEST_ID, RegistryValueId, USER_PATH, get_path}; #[cfg(windows)] use windows::{ add_uninstall_registry_entry, do_add_to_path, do_remove_from_path, - remove_uninstall_registry_entry, + remove_uninstall_registry_entry, run_update, }; -#[cfg(windows)] -pub(crate) use windows::{run_update, self_replace}; pub(crate) struct InstallOpts<'a> { pub default_host_tuple: Option, @@ -533,10 +535,10 @@ impl SelfUpdateMode { SelfUpdatePermission::Permit => {} } - let setup_path = prepare_update(dl_cfg).await?; + let prepared_updater = prepare_update(dl_cfg).await?; - if let Some(setup_path) = &setup_path { - return run_update(setup_path, dl_cfg.process); + if let Some(prepared_updater) = prepared_updater { + return run_update(prepared_updater, dl_cfg.process); } else { // Try again in case we emitted "tool `{}` is already installed" last time. install_proxies(dl_cfg.process)?; @@ -583,8 +585,6 @@ impl fmt::Display for SelfUpdateMode { } } -static DEFAULT_UPDATE_ROOT: &str = "https://static.rust-lang.org/rustup"; - fn update_root(process: &Process) -> String { process .var("RUSTUP_UPDATE_ROOT") @@ -773,19 +773,7 @@ fn warn_if_default_linker_missing(process: &Process) { } fn install_bins(process: &Process) -> anyhow::Result<()> { - let bin_path = process.cargo_home()?.join("bin"); - let this_exe_path = utils::current_exe()?; - let rustup_path = bin_path.join(format!("rustup{EXE_SUFFIX}")); - - utils::ensure_dir_exists("bin", &bin_path)?; - // NB: Even on Linux we can't just copy the new binary over the (running) - // old binary; we must unlink it first. - if rustup_path.exists() { - utils::remove_file("rustup-bin", &rustup_path)?; - } - utils::copy_file_symlink_to_source(&this_exe_path, &rustup_path)?; - utils::make_executable(&rustup_path)?; - install_proxies(process) + SelfUpdateLock::acquire(process)?.install_bins(process) } pub(crate) fn install_proxies(process: &Process) -> anyhow::Result<()> { @@ -1125,21 +1113,10 @@ pub(crate) fn self_update_permitted(explicit: bool) -> anyhow::Result) -> anyhow::Result { common::warn_if_host_is_emulated(cfg.process); @@ -1165,8 +1142,8 @@ pub(crate) async fn update(cfg: &Cfg<'_>) -> anyhow::Result { } match prepare_update(&DownloadCfg::new(cfg)).await? { - Some(setup_path) => { - let Some(version) = get_and_parse_new_rustup_version(&setup_path) else { + Some(prepared_updater) => { + let Some(version) = get_and_parse_new_rustup_version(prepared_updater.path()) else { error!("failed to get rustup version"); return Ok(ExitCode::FAILURE); }; @@ -1176,7 +1153,7 @@ pub(crate) async fn update(cfg: &Cfg<'_>) -> anyhow::Result { PackageUpdate::Rustup, Ok(UpdateStatus::Updated(version)), ); - return run_update(&setup_path, cfg.process); + return run_update(prepared_updater, cfg.process); } None => { let _ = common::show_channel_update( @@ -1217,18 +1194,14 @@ fn parse_new_rustup_version(version: String) -> String { String::from(matched_version) } -pub(crate) async fn prepare_update(dl_cfg: &DownloadCfg<'_>) -> anyhow::Result> { +async fn prepare_update(dl_cfg: &DownloadCfg<'_>) -> anyhow::Result> { let cargo_home = dl_cfg.process.cargo_home()?; let rustup_path = cargo_home.join(format!("bin{MAIN_SEPARATOR}rustup{EXE_SUFFIX}")); - let setup_path = cargo_home.join(format!("bin{MAIN_SEPARATOR}rustup-init{EXE_SUFFIX}")); if !rustup_path.exists() { return Err(CliError::NotSelfInstalled { p: cargo_home }.into()); } - - if setup_path.exists() { - utils::remove_file("setup", &setup_path)?; - } + let self_update_lock = SelfUpdateLock::acquire(dl_cfg.process)?; // Get build tuple let tuple = TargetTuple::from_build(); @@ -1268,18 +1241,23 @@ pub(crate) async fn prepare_update(dl_cfg: &DownloadCfg<'_>) -> anyhow::Result) -> anyhow::Result { @@ -1374,16 +1352,15 @@ pub(crate) async fn check_rustup_update(dl_cfg: &DownloadCfg<'_>) -> anyhow::Res #[tracing::instrument(level = "trace")] pub(crate) fn cleanup_self_updater(process: &Process) -> anyhow::Result<()> { - let cargo_home = process.cargo_home()?; - let setup = cargo_home.join(format!("bin/rustup-init{EXE_SUFFIX}")); - - if setup.exists() { - utils::remove_file("setup", &setup)?; - } - - Ok(()) + stage::cleanup(process) } +static DEFAULT_UPDATE_ROOT: &str = "https://static.rust-lang.org/rustup"; +#[cfg(feature = "test")] +pub const CHECKPOINT_SELF_UPDATE_PREPARED: &str = "self-update-prepared"; +#[cfg(feature = "test")] +pub const CHECKPOINT_SELF_REPLACE_READY: &str = "self-replace-ready"; + #[cfg(test)] mod tests { use std::collections::HashMap; @@ -1439,8 +1416,10 @@ info: default host tuple is {0} fn install_bins_creates_cargo_home() { let root_dir = test_dir().unwrap(); let cargo_home = root_dir.path().join("cargo"); + let rustup_home = root_dir.path().join("rustup"); let mut vars = HashMap::new(); vars.env("CARGO_HOME", cargo_home.to_string_lossy().to_string()); + vars.env("RUSTUP_HOME", rustup_home); let tp = TestProcess::with_vars(vars); super::install_bins(&tp.process).unwrap(); assert!(cargo_home.exists()); diff --git a/src/cli/self_update/stage.rs b/src/cli/self_update/stage.rs new file mode 100644 index 0000000000..2236162bf9 --- /dev/null +++ b/src/cli/self_update/stage.rs @@ -0,0 +1,515 @@ +use std::{ + env::consts::EXE_SUFFIX, + fs::{self, File, OpenOptions}, + io, + path::{Path, PathBuf}, + process::{Child, Command}, + time::{Duration, SystemTime}, +}; + +use anyhow::Context; +use tracing::warn; + +use super::install_proxies; +use crate::{process::Process, utils}; + +/// Exclusive right to download the updater or replace the installed rustup. +pub(super) struct SelfUpdateLock { + directory: PathBuf, + _file: File, +} + +impl SelfUpdateLock { + pub(super) fn acquire(process: &Process) -> anyhow::Result { + let lock = Self::open(process)?; + lock._file.lock().context("failed to lock self-update")?; + Ok(lock) + } + + fn try_acquire(process: &Process) -> anyhow::Result> { + let lock = Self::open(process)?; + match lock._file.try_lock() { + Ok(()) => Ok(Some(lock)), + Err(fs::TryLockError::WouldBlock) => Ok(None), + Err(fs::TryLockError::Error(error)) => Err(error).context("failed to lock self-update"), + } + } + + fn open(process: &Process) -> anyhow::Result { + let directory = stage_root(process)?; + utils::ensure_dir_exists("self-update", &directory)?; + let file = OpenOptions::new() + .read(true) + .write(true) + .create(true) + // The file exists only to be locked; never touch its contents. + .truncate(false) + .open(directory.join(SELF_UPDATE_LOCK_FILE)) + .context("failed to open self-update lock")?; + + Ok(Self { + directory, + _file: file, + }) + } + + /// Clears the previous update's leftovers and reserves the managed updater path. + pub(super) fn prepare_updater(self) -> anyhow::Result { + let path = self.updater_path(); + utils::ensure_file_removed("self-updater", &path)?; + for marker in [Marker::Complete, Marker::Failed] { + utils::ensure_file_removed("self-update status marker", &marker.path(&self.directory))?; + } + Ok(PreparedUpdater { path, _lock: self }) + } + + /// Installs the running executable as `$CARGO_HOME/bin/rustup` and refreshes its proxies. + pub(super) fn install_bins(&self, process: &Process) -> anyhow::Result<()> { + self.install_bins_from(&utils::current_exe()?, process) + } + + fn install_bins_from(&self, this_exe_path: &Path, process: &Process) -> anyhow::Result<()> { + let bin_path = process.cargo_home()?.join("bin"); + let rustup_path = bin_path.join(format!("rustup{EXE_SUFFIX}")); + utils::ensure_dir_exists("bin", &bin_path)?; + + // Stage the new binary as a sibling of the installed one, so that + // publishing it below is a rename within a single directory. + let pending = tempfile::Builder::new() + .prefix(PENDING_BINARY_PREFIX) + .tempfile_in(&bin_path) + .context("failed to reserve a pending rustup binary")? + .into_temp_path(); + + // TempPath reserves a unique name, but preserving a source symlink requires + // an absent destination rather than an existing empty file. + fs::remove_file(&pending).context("failed to prepare the pending rustup path")?; + utils::copy_file_symlink_to_source(this_exe_path, &pending)?; + utils::make_executable(&pending)?; + if !fs::symlink_metadata(&pending)?.file_type().is_symlink() { + OpenOptions::new() + .write(true) + .open(&pending) + .and_then(|file| file.sync_all()) + .context("failed to sync the pending rustup binary")?; + } + + replace_rustup_binary(&pending, &rustup_path)?; + install_proxies(process) + } + + fn updater_path(&self) -> PathBuf { + self.directory.join(format!("rustup-init{EXE_SUFFIX}")) + } +} + +/// The managed updater path, held together with the lock that protects it. +pub(super) struct PreparedUpdater { + path: PathBuf, + _lock: SelfUpdateLock, +} + +impl PreparedUpdater { + /// Starts the updater in `--self-replace` mode and then releases the lock. + /// + /// The lock must be held until the child has been spawned: a concurrent + /// self-update could otherwise replace the updater before it is executed. + pub(super) fn spawn_replacer(self) -> anyhow::Result { + let stage = self + .path + .parent() + .context("self-updater path has no parent directory")?; + Command::new(&self.path) + .env(STAGE_ENV, stage) + .arg("--self-replace") + .spawn() + .with_context(|| format!("unable to run updater ({})", self.path.display())) + } + + pub(super) fn path(&self) -> &Path { + &self.path + } +} + +pub(super) fn mark_result(succeeded: bool, process: &Process) { + let Some(stage) = process.var_os(STAGE_ENV).map(PathBuf::from) else { + return; + }; + let marker = if succeeded { + Marker::Complete + } else { + Marker::Failed + }; + if let Err(error) = marker.record(process, &stage) { + warn!("could not record self-update result: {error}"); + } +} + +pub(super) fn cleanup(process: &Process) -> anyhow::Result<()> { + cleanup_at(process, SystemTime::now()) +} + +fn replace_rustup_binary(replacement: &Path, rustup: &Path) -> anyhow::Result<()> { + // `rename` replaces an existing destination in one step on every platform, + // so a failure here leaves the installed rustup untouched. The copy fallback + // is disabled because it would break that guarantee; it is never needed, + // since the replacement lives in the same directory as the target. + utils::rename("rustup", replacement, rustup, false)?; + // Make the rename durable. Windows has no directory handle to sync. + #[cfg(unix)] + File::open( + rustup + .parent() + .context("installed rustup binary has no parent directory")?, + ) + .and_then(|directory| directory.sync_all()) + .context("failed to sync rustup binary directory")?; + + Ok(()) +} + +fn cleanup_at(process: &Process, now: SystemTime) -> anyhow::Result<()> { + if let Some(lock) = SelfUpdateLock::try_acquire(process)? { + let updater = lock.updater_path(); + // The replacer records an outcome only once it has finished. An unmarked + // updater may still be about to run, or its replacer may have died before + // recording anything; only its age tells those two cases apart. + let markers = [Marker::Complete, Marker::Failed]; + let finished = markers + .iter() + .any(|marker| marker.path(&lock.directory).is_file()); + if (finished || is_stale(&updater, now)) + && utils::remove_file_best_effort("self-updater", &updater) + { + for marker in markers { + utils::remove_file_best_effort( + "self-update status marker", + &marker.path(&lock.directory), + ); + } + } + } + + let bin = process.cargo_home()?.join("bin"); + match fs::read_dir(&bin) { + Ok(entries) => { + for entry in entries.flatten() { + let path = entry.path(); + if entry + .file_name() + .to_string_lossy() + .starts_with(PENDING_BINARY_PREFIX) + && is_stale(&path, now) + { + utils::remove_file_best_effort("pending rustup binary", &path); + } + } + } + Err(error) if error.kind() == io::ErrorKind::NotFound => {} + Err(error) => { + warn!( + "could not inspect pending rustup binaries in {}: {error}", + bin.display() + ); + } + } + + let updater = bin.join(format!("rustup-init{EXE_SUFFIX}")); + // Legacy updaters have no result marker, and an older rustup process may + // still own the shared path. + if is_stale(&updater, now) { + utils::remove_file_best_effort("legacy self-updater", &updater); + } + + Ok(()) +} + +fn is_stale(path: &Path, now: SystemTime) -> bool { + fs::metadata(path) + .and_then(|metadata| metadata.modified()) + .ok() + .and_then(|modified| now.duration_since(modified).ok()) + .is_some_and(|age| age >= ABANDONED_UPDATE_AGE) +} + +/// Outcome recorded next to the managed updater once replacement has finished. +#[derive(Clone, Copy)] +enum Marker { + Complete, + Failed, +} + +impl Marker { + /// Records this outcome in `stage`, ignoring stages outside the managed directory. + fn record(self, process: &Process, stage: &Path) -> anyhow::Result<()> { + if stage != stage_root(process)? { + warn!( + "ignoring self-update stage outside the managed directory: {}", + stage.display() + ); + return Ok(()); + } + + utils::write_file("self-update status marker", &self.path(stage), "") + } + + fn path(self, stage: &Path) -> PathBuf { + stage.join(self.as_str()) + } + + fn as_str(self) -> &'static str { + match self { + Self::Complete => "complete", + Self::Failed => "failed", + } + } +} + +fn stage_root(process: &Process) -> anyhow::Result { + Ok(process.rustup_home()?.join(SELF_UPDATE_DIRECTORY)) +} + +const SELF_UPDATE_DIRECTORY: &str = "self-update"; +const SELF_UPDATE_LOCK_FILE: &str = "self-update.lock"; +const STAGE_ENV: &str = "RUSTUP_SELF_UPDATE_STAGE"; +const PENDING_BINARY_PREFIX: &str = ".rustup-pending-"; +const ABANDONED_UPDATE_AGE: Duration = Duration::from_secs(24 * 60 * 60); + +#[cfg(test)] +mod tests { + use std::collections::HashMap; + + use super::*; + use crate::{ + process::TestProcess, + test::{Env, test_dir}, + }; + + #[tokio::test] + async fn updater_path_is_stable() { + let root = test_dir().unwrap(); + let process = test_process(root.path()); + let first = SelfUpdateLock::acquire(&process.process).unwrap(); + let first_path = first.updater_path(); + let stage = first.directory.clone(); + fs::write(&first_path, "").unwrap(); + fs::write(Marker::Complete.path(&stage), "").unwrap(); + drop(first); + let second = SelfUpdateLock::acquire(&process.process) + .unwrap() + .prepare_updater() + .unwrap(); + + assert_eq!(&first_path, second.path()); + assert!(!Marker::Complete.path(&stage).exists()); + } + + #[tokio::test] + async fn self_update_lock_is_global() { + let root = test_dir().unwrap(); + let process = test_process(root.path()); + let lock = SelfUpdateLock::acquire(&process.process).unwrap(); + let contender = OpenOptions::new() + .read(true) + .write(true) + .open( + stage_root(&process.process) + .unwrap() + .join(SELF_UPDATE_LOCK_FILE), + ) + .unwrap(); + + assert!(matches!( + contender.try_lock(), + Err(fs::TryLockError::WouldBlock) + )); + drop(lock); + contender.try_lock().unwrap(); + } + + #[tokio::test] + async fn install_bins_preserves_existing_rustup_if_source_disappears() { + let root = test_dir().unwrap(); + let process = test_process(root.path()); + let rustup = root.path().join(format!("cargo/bin/rustup{EXE_SUFFIX}")); + fs::create_dir_all(rustup.parent().unwrap()).unwrap(); + fs::write(&rustup, "old rustup").unwrap(); + + SelfUpdateLock::acquire(&process.process) + .unwrap() + .install_bins_from(&root.path().join("missing-updater"), &process.process) + .unwrap_err(); + + assert_eq!(fs::read_to_string(rustup).unwrap(), "old rustup"); + } + + #[test] + fn failed_replace_preserves_existing_rustup() { + let root = test_dir().unwrap(); + let rustup = root.path().join(format!("rustup{EXE_SUFFIX}")); + fs::write(&rustup, "old rustup").unwrap(); + + replace_rustup_binary(&root.path().join("missing"), &rustup).unwrap_err(); + + assert_eq!(fs::read_to_string(rustup).unwrap(), "old rustup"); + } + + #[test] + fn replace_publishes_pending_rustup() { + let root = test_dir().unwrap(); + let rustup = root.path().join(format!("rustup{EXE_SUFFIX}")); + let pending = root.path().join("pending"); + fs::write(&rustup, "old rustup").unwrap(); + fs::write(&pending, "new rustup").unwrap(); + + replace_rustup_binary(&pending, &rustup).unwrap(); + + assert_eq!(fs::read_to_string(rustup).unwrap(), "new rustup"); + assert!(!pending.exists()); + } + + #[tokio::test] + async fn cleanup_keeps_locked_updater() { + let root = test_dir().unwrap(); + let process = test_process(root.path()); + let lock = SelfUpdateLock::acquire(&process.process).unwrap(); + let updater = lock.updater_path(); + fs::write(&updater, "").unwrap(); + fs::write(Marker::Complete.path(&lock.directory), "").unwrap(); + + cleanup_at(&process.process, SystemTime::now()).unwrap(); + + assert!(updater.exists()); + drop(lock); + cleanup_at(&process.process, SystemTime::now()).unwrap(); + assert!(!updater.exists()); + } + + #[tokio::test] + async fn spawn_replacer_rejects_parentless_path() { + let root = test_dir().unwrap(); + let process = test_process(root.path()); + let prepared_updater = PreparedUpdater { + path: PathBuf::new(), + _lock: SelfUpdateLock::acquire(&process.process).unwrap(), + }; + let error = prepared_updater.spawn_replacer().err().unwrap(); + + assert_eq!( + error.to_string(), + "self-updater path has no parent directory" + ); + } + + #[tokio::test] + async fn cleanup_keeps_fresh_updater() { + let root = test_dir().unwrap(); + let process = test_process(root.path()); + let prepared_updater = SelfUpdateLock::acquire(&process.process) + .unwrap() + .prepare_updater() + .unwrap(); + let updater = prepared_updater.path().to_owned(); + fs::write(&updater, "").unwrap(); + drop(prepared_updater); + + cleanup_at(&process.process, SystemTime::now()).unwrap(); + + assert!(updater.exists()); + } + + #[tokio::test] + async fn cleanup_removes_finished_updater() { + let root = test_dir().unwrap(); + let process = test_process(root.path()); + let stage = stage_root(&process.process).unwrap(); + + for marker in [Marker::Complete, Marker::Failed] { + let prepared_updater = SelfUpdateLock::acquire(&process.process) + .unwrap() + .prepare_updater() + .unwrap(); + let updater = prepared_updater.path().to_owned(); + fs::write(&updater, "").unwrap(); + drop(prepared_updater); + marker.record(&process.process, &stage).unwrap(); + + cleanup_at(&process.process, SystemTime::now()).unwrap(); + + assert!(!updater.exists()); + assert!(!marker.path(&stage).exists()); + } + } + + #[tokio::test] + async fn cleanup_removes_abandoned_updater() { + let root = test_dir().unwrap(); + let process = test_process(root.path()); + let prepared_updater = SelfUpdateLock::acquire(&process.process) + .unwrap() + .prepare_updater() + .unwrap(); + let updater = prepared_updater.path().to_owned(); + fs::write(&updater, "").unwrap(); + drop(prepared_updater); + + cleanup_at( + &process.process, + SystemTime::now() + ABANDONED_UPDATE_AGE + Duration::from_secs(1), + ) + .unwrap(); + + assert!(!updater.exists()); + } + + #[tokio::test] + async fn cleanup_delays_removing_legacy_updater() { + let root = test_dir().unwrap(); + let process = test_process(root.path()); + let updater = root + .path() + .join(format!("cargo/bin/rustup-init{EXE_SUFFIX}")); + fs::create_dir_all(updater.parent().unwrap()).unwrap(); + fs::write(&updater, "").unwrap(); + + cleanup_at(&process.process, SystemTime::now()).unwrap(); + assert!(updater.exists()); + + cleanup_at( + &process.process, + SystemTime::now() + ABANDONED_UPDATE_AGE + Duration::from_secs(1), + ) + .unwrap(); + assert!(!updater.exists()); + } + + #[tokio::test] + async fn cleanup_removes_abandoned_pending_binary() { + let root = test_dir().unwrap(); + let process = test_process(root.path()); + let pending = root + .path() + .join("cargo/bin") + .join(format!("{PENDING_BINARY_PREFIX}orphan")); + fs::create_dir_all(pending.parent().unwrap()).unwrap(); + fs::write(&pending, "").unwrap(); + + cleanup_at(&process.process, SystemTime::now()).unwrap(); + assert!(pending.exists()); + + cleanup_at( + &process.process, + SystemTime::now() + ABANDONED_UPDATE_AGE + Duration::from_secs(1), + ) + .unwrap(); + assert!(!pending.exists()); + } + + fn test_process(root: &Path) -> TestProcess { + let mut vars = HashMap::new(); + vars.env("HOME", root); + vars.env("CARGO_HOME", root.join("cargo")); + vars.env("RUSTUP_HOME", root.join("rustup")); + TestProcess::with_vars(vars) + } +} diff --git a/src/cli/self_update/unix.rs b/src/cli/self_update/unix.rs index 5da3d78ba0..75597e39f1 100644 --- a/src/cli/self_update/unix.rs +++ b/src/cli/self_update/unix.rs @@ -1,14 +1,11 @@ -use std::{ - path::{Path, PathBuf}, - process::Command, -}; +use std::path::PathBuf; use anyhow::{Context, bail}; use tracing::{error, warn}; use super::{ - install_bins, shell::{self, Posix, UnixShell}, + stage::{self, PreparedUpdater, SelfUpdateLock}, }; use crate::{process::Process, utils}; @@ -121,13 +118,16 @@ pub(crate) fn do_write_env_files(process: &Process) -> anyhow::Result<()> { Ok(()) } -/// Tell the upgrader to replace the rustup bins, then delete -/// itself. -pub(crate) fn run_update(setup_path: &Path, _process: &Process) -> anyhow::Result { - let status = Command::new(setup_path) - .arg("--self-replace") - .status() - .context(format!("unable to run updater ({})", setup_path.display()))?; +/// Tell the updater to replace the rustup bins, then wait for it to finish. +pub(super) fn run_update( + prepared_updater: PreparedUpdater, + _process: &Process, +) -> anyhow::Result { + let setup_path = prepared_updater.path().to_owned(); + let status = prepared_updater + .spawn_replacer()? + .wait() + .with_context(|| format!("unable to wait for updater ({})", setup_path.display()))?; if !status.success() { bail!("self-updated failed to replace rustup executable"); @@ -140,7 +140,12 @@ pub(crate) fn run_update(setup_path: &Path, _process: &Process) -> anyhow::Resul /// `$CARGO_HOME/bin/rustup` with the running exe, and updates the /// links to it. pub(crate) fn self_replace(process: &Process) -> anyhow::Result { - install_bins(process)?; + let self_update_lock = SelfUpdateLock::acquire(process)?; + #[cfg(feature = "test")] + process.checkpoint(super::CHECKPOINT_SELF_REPLACE_READY); + let result = self_update_lock.install_bins(process); + stage::mark_result(result.is_ok(), process); + result?; Ok(utils::ExitCode(0)) } diff --git a/src/cli/self_update/windows.rs b/src/cli/self_update/windows.rs index 4b36ab43aa..fb8e41ae0f 100644 --- a/src/cli/self_update/windows.rs +++ b/src/cli/self_update/windows.rs @@ -3,9 +3,8 @@ use std::{ env::{consts::EXE_SUFFIX, split_paths}, ffi::{OsStr, OsString}, fmt, - io::Write, + io::{self, Write}, os::windows::ffi::OsStrExt, - path::Path, process::Command, }; @@ -18,13 +17,12 @@ use windows_registry::{CURRENT_USER, HSTRING, Key}; use windows_result::WIN32_ERROR; use windows_sys::Win32::Foundation::{ERROR_FILE_NOT_FOUND, ERROR_INVALID_DATA}; +use super::{ + InstallOpts, report_error, + stage::{self, PreparedUpdater, SelfUpdateLock}, +}; use crate::{ - cli::{ - common, - errors::CliError, - markdown::md, - self_update::{InstallOpts, install_bins, report_error}, - }, + cli::{common, errors::CliError, markdown::md}, dist::TargetTuple, download::DownloadOptions, process::{ColorableTerminal, Process}, @@ -41,7 +39,7 @@ pub(crate) fn ensure_prompt(process: &Process) -> anyhow::Result<()> { fn choice(max: u8, process: &Process) -> anyhow::Result> { write!(process.stdout().lock(), ">")?; - let _ = std::io::stdout().flush(); + let _ = io::stdout().flush(); let input = common::read_line(process)?; let r = match str::parse(&input) { @@ -659,24 +657,23 @@ pub(crate) fn remove_uninstall_registry_entry(process: &Process) -> anyhow::Resu } } -pub(crate) fn run_update(setup_path: &Path, process: &Process) -> anyhow::Result { - Command::new(setup_path) - .arg("--self-replace") - .spawn() - .context("unable to run updater")?; - - let Some(version) = super::get_and_parse_new_rustup_version(setup_path) else { - warn!("failed to get the new rustup version in order to update `DisplayVersion`"); - return Ok(utils::ExitCode(1)); - }; - update_uninstall_registry_display_version(&version, process)?; +pub(super) fn run_update( + prepared_updater: PreparedUpdater, + _process: &Process, +) -> anyhow::Result { + prepared_updater.spawn_replacer()?; Ok(utils::ExitCode(0)) } pub(crate) fn self_replace(process: &Process) -> anyhow::Result { wait_for_parent()?; - install_bins(process)?; + let self_update_lock = SelfUpdateLock::acquire(process)?; + let result = self_update_lock.install_bins(process).and_then(|()| { + update_uninstall_registry_display_version(env!("CARGO_PKG_VERSION"), process) + }); + stage::mark_result(result.is_ok(), process); + result?; Ok(utils::ExitCode(0)) } diff --git a/src/process.rs b/src/process.rs index 177b8a790b..53508789a5 100644 --- a/src/process.rs +++ b/src/process.rs @@ -262,7 +262,7 @@ impl Process { /// Registers a testing checkpoint with the given name and parks the current thread. /// - /// Usually, the current process will be killed by the test driver. + /// The test driver can either remove the marker to resume or kill the process. #[cfg(feature = "test")] pub(crate) fn checkpoint(&self, name: &str) { if self.var(CHECKPOINT_ENV).as_deref() != Ok(name) { @@ -275,13 +275,16 @@ impl Process { 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"); + let marker = checkpoint_path(test_root, name); + fs::write(&marker, name).expect("failed to write test checkpoint marker"); let start_time = Instant::now(); let max_wait = Duration::from_mins(5); while start_time.elapsed() < max_wait { - thread::sleep(Duration::from_secs(10)); + if !marker.exists() { + return; + } + thread::sleep(Duration::from_millis(10)); } panic!( "test checkpoint '{name}' timed out after {max_wait:?} without being killed by the test driver", diff --git a/src/test/clitools.rs b/src/test/clitools.rs index d2dad716b7..190b4b7a1d 100644 --- a/src/test/clitools.rs +++ b/src/test/clitools.rs @@ -1031,6 +1031,7 @@ impl CliTestContext { cmd.spawn() .expect("failed to start command for checkpoint test") }), + marker: marker.clone(), } }; @@ -1138,6 +1139,7 @@ impl Drop for WorkDirGuard<'_> { #[must_use] pub struct ParkedChild { child: Option, + marker: PathBuf, } impl ParkedChild { @@ -1147,9 +1149,21 @@ impl ParkedChild { child .kill() .expect("failed to terminate command at checkpoint"); - child + let status = child + .wait() + .expect("failed to reap command after checkpoint"); + remove_checkpoint_marker(&self.marker); + status + } + + /// Resume the parked command and wait for it to finish. + pub fn resume(mut self) -> ExitStatus { + remove_checkpoint_marker(&self.marker); + self.child + .take() + .unwrap() .wait() - .expect("failed to reap command after checkpoint") + .expect("failed to reap resumed checkpoint command") } } @@ -1160,6 +1174,15 @@ impl Drop for ParkedChild { }; let _ = child.kill(); let _ = child.wait(); + remove_checkpoint_marker(&self.marker); + } +} + +fn remove_checkpoint_marker(marker: &Path) { + if let Err(error) = fs::remove_file(marker) + && error.kind() != io::ErrorKind::NotFound + { + panic!("failed to remove checkpoint marker: {error}"); } } diff --git a/src/utils/mod.rs b/src/utils/mod.rs index fd79701e5e..46f65c38a8 100644 --- a/src/utils/mod.rs +++ b/src/utils/mod.rs @@ -423,6 +423,34 @@ pub(crate) fn format_path_for_display(path: &str) -> String { } } +/// Removes `path` if possible, without failing. +/// +/// Unlike [`remove_file`], a busy file is left alone instead of retried, since +/// callers use this for cleanup that another process may legitimately still be +/// using. Returns whether `path` is gone afterwards. +pub(crate) fn remove_file_best_effort(name: &str, path: &Path) -> bool { + match fs::remove_file(path) { + Ok(()) => { + debug!(path = %path.display(), "removed {name}"); + true + } + Err(error) if error.kind() == io::ErrorKind::NotFound => true, + Err(error) + if matches!( + error.kind(), + io::ErrorKind::PermissionDenied | io::ErrorKind::ResourceBusy + ) => + { + debug!(path = %path.display(), "leaving busy {name}"); + false + } + Err(error) => { + warn!("could not remove {name} {}: {error}", path.display()); + false + } + } +} + #[cfg(target_os = "linux")] fn copy_and_delete(name: &'static str, src: &Path, dest: &Path) -> anyhow::Result<()> { // https://github.com/rust-lang/rustup/issues/1239 diff --git a/tests/suite/cli_self_upd.rs b/tests/suite/cli_self_upd.rs index 83ca66b671..89eae6f39a 100644 --- a/tests/suite/cli_self_upd.rs +++ b/tests/suite/cli_self_upd.rs @@ -1,17 +1,24 @@ //! Testing self install, uninstall and update -use std::{env, env::consts::EXE_SUFFIX, fs, path::Path, process::Command}; +use std::{ + env::consts::EXE_SUFFIX, + fs, + path::{Path, PathBuf}, + process::Command, +}; use remove_dir_all::remove_dir_all; -#[cfg(windows)] use retry::{ delay::{Fibonacci, jitter}, retry, }; +#[cfg(unix)] +use rustup::cli::self_update::CHECKPOINT_SELF_REPLACE_READY; #[cfg(windows)] use rustup::test::RegistryValueId; use rustup::{ DUP_TOOLS, TOOLS, + cli::self_update::CHECKPOINT_SELF_UPDATE_PREPARED, test::{ CROSS_ARCH1, CliTestContext, Scenario, SelfUpdateTestContext, calc_hash, output_release_file, this_host_tuple, @@ -21,8 +28,6 @@ use rustup::{ #[cfg(windows)] use windows_registry::{CURRENT_USER, Value}; -const TEST_VERSION: &str = "1.1.1"; - /// Empty dist server, rustup installed with no toolchain async fn setup_empty_installed() -> CliTestContext { let cx = CliTestContext::new(Scenario::Empty).await; @@ -500,6 +505,7 @@ async fn update_overwrites_programs_display_version() { ) .unwrap(); cx.config.expect(["rustup", "self", "update"]).await.is_ok(); + wait_for_completed_update(&cx.config.rustupdir.rustupdir); assert_eq!( USER_RUSTUP_VERSION .get(test_id, CURRENT_USER) @@ -509,12 +515,6 @@ async fn update_overwrites_programs_display_version() { ); } -#[cfg(windows)] -const USER_RUSTUP_VERSION: RegistryValueId = RegistryValueId { - sub_key: r"Software\Microsoft\Windows\CurrentVersion\Uninstall\Rustup", - value_name: "DisplayVersion", -}; - #[tokio::test] async fn update_but_not_installed() { let cx = SelfUpdateTestContext::new(TEST_VERSION).await; @@ -531,9 +531,8 @@ error: rustup is not installed at '[CARGO_DIR]' } #[tokio::test] -async fn update_but_delete_existing_updater_first() { +async fn update_does_not_reuse_legacy_updater_path() { let cx = SelfUpdateTestContext::new(TEST_VERSION).await; - // The updater is stored in a known location let setup = cx .config .cargodir @@ -544,8 +543,6 @@ async fn update_but_delete_existing_updater_first() { .await .is_ok(); - // If it happens to already exist for some reason it - // should just be deleted. raw::write_file(&setup, "").unwrap(); cx.config .expect(&["rustup", "self", "update"]) @@ -554,6 +551,58 @@ async fn update_but_delete_existing_updater_first() { let rustup = cx.config.cargodir.join(format!("bin/rustup{EXE_SUFFIX}")); assert!(rustup.exists()); + assert!(setup.exists()); + assert!(managed_updater(&cx.config.rustupdir.rustupdir).exists()); +} + +#[tokio::test] +async fn managed_updater_survives_concurrent_proxy_cleanup() { + let mut cx = CliTestContext::new(Scenario::SimpleV2).await; + let _update_server = cx.with_update_server(TEST_VERSION); + cx.config + .expect(["rustup-init", "-y", "--no-modify-path"]) + .await + .is_ok(); + + let rustup = cx.config.cargodir.join(format!("bin/rustup{EXE_SUFFIX}")); + let before_hash = calc_hash(&rustup); + let parked = cx.spawn_at( + CHECKPOINT_SELF_UPDATE_PREPARED, + ["rustup", "self", "update"], + ); + let updater = managed_updater(&cx.config.rustupdir.rustupdir); + + cx.config.expect(["rustc", "--version"]).await.is_ok(); + + assert!(updater.exists()); + assert!(parked.resume().success()); + wait_for_completed_update(&cx.config.rustupdir.rustupdir); + assert_ne!(before_hash, calc_hash(&rustup)); +} + +#[cfg(unix)] +#[tokio::test] +async fn self_update_replacement_survives_proxy_cleanup() { + let mut cx = CliTestContext::new(Scenario::SimpleV2).await; + let _update_server = cx.with_update_server(TEST_VERSION); + cx.config + .expect(["rustup-init", "-y", "--no-modify-path"]) + .await + .is_ok(); + + let rustup = cx.config.cargodir.join(format!("bin/rustup{EXE_SUFFIX}")); + let before_hash = calc_hash(&rustup); + let parked = cx.spawn_at(CHECKPOINT_SELF_REPLACE_READY, ["rustup", "self", "update"]); + + cx.config.expect(["rustc", "--version"]).await.is_ok(); + + let status = parked.resume(); + assert!( + rustup.exists(), + "concurrent proxy removed the installed rustup during self-update ({status})" + ); + assert!(status.success(), "self-update failed: {status}"); + assert_ne!(before_hash, calc_hash(&rustup)); } #[tokio::test] @@ -790,11 +839,7 @@ async fn updater_leaves_itself_for_later_deletion() { .is_ok(); cx.config.expect(["rustup", "self", "update"]).await.is_ok(); - let setup = cx - .config - .cargodir - .join(format!("bin/rustup-init{EXE_SUFFIX}")); - assert!(setup.exists()); + assert!(managed_updater(&cx.config.rustupdir.rustupdir).exists()); } #[tokio::test] @@ -809,17 +854,14 @@ async fn updater_is_deleted_after_running_rustup() { .await .is_ok(); cx.config.expect(["rustup", "self", "update"]).await.is_ok(); + wait_for_completed_update(&cx.config.rustupdir.rustupdir); cx.config .expect(["rustup", "update", "nightly"]) .await .is_ok(); - let setup = cx - .config - .cargodir - .join(format!("bin/rustup-init{EXE_SUFFIX}")); - assert!(!setup.exists()); + assert!(!managed_updater(&cx.config.rustupdir.rustupdir).exists()); } #[tokio::test] @@ -834,14 +876,11 @@ async fn updater_is_deleted_after_running_rustc() { .await .is_ok(); cx.config.expect(["rustup", "self", "update"]).await.is_ok(); + wait_for_completed_update(&cx.config.rustupdir.rustupdir); cx.config.expect(["rustc", "--version"]).await.is_ok(); - let setup = cx - .config - .cargodir - .join(format!("bin/rustup-init{EXE_SUFFIX}")); - assert!(!setup.exists()); + assert!(!managed_updater(&cx.config.rustupdir.rustupdir).exists()); } #[tokio::test] @@ -1225,3 +1264,30 @@ async fn install_minimal_profile() { cx.config.expect_component_executable("rustc").await; cx.config.expect_component_not_executable("cargo").await; } + +fn wait_for_completed_update(rustup_home: &Path) { + let stage = rustup_home.join("self-update"); + retry(Fibonacci::from_millis(1).map(jitter).take(23), || { + if stage.join("complete").is_file() { + Ok(()) + } else if stage.join("failed").is_file() { + Err("self-update failed") + } else { + Err("self-update has not completed") + } + }) + .unwrap(); +} + +fn managed_updater(rustup_home: &Path) -> PathBuf { + rustup_home + .join("self-update") + .join(format!("rustup-init{EXE_SUFFIX}")) +} + +const TEST_VERSION: &str = "1.1.1"; +#[cfg(windows)] +const USER_RUSTUP_VERSION: RegistryValueId = RegistryValueId { + sub_key: r"Software\Microsoft\Windows\CurrentVersion\Uninstall\Rustup", + value_name: "DisplayVersion", +};