From 8d9fbaa5a574c3445d028f08f131f325496bb0a6 Mon Sep 17 00:00:00 2001 From: Akrm Al-Hakimi Date: Sun, 13 Sep 2026 15:40:14 -0600 Subject: [PATCH 1/6] test(self-update): reproduce proxy cleanup race --- src/cli/self_update.rs | 3 +++ src/cli/self_update/unix.rs | 2 ++ src/process.rs | 11 +++++++---- src/test/clitools.rs | 27 +++++++++++++++++++++++++-- tests/suite/cli_self_upd.rs | 31 +++++++++++++++++++++++++++++-- 5 files changed, 66 insertions(+), 8 deletions(-) diff --git a/src/cli/self_update.rs b/src/cli/self_update.rs index 2f60fad5b4..690acd6c39 100644 --- a/src/cli/self_update.rs +++ b/src/cli/self_update.rs @@ -1381,6 +1381,9 @@ pub(crate) fn cleanup_self_updater(process: &Process) -> anyhow::Result<()> { Ok(()) } +#[cfg(feature = "test")] +pub const CHECKPOINT_SELF_REPLACE_READY: &str = "self-replace-ready"; + #[cfg(test)] mod tests { use std::collections::HashMap; diff --git a/src/cli/self_update/unix.rs b/src/cli/self_update/unix.rs index 5da3d78ba0..dfb114ff72 100644 --- a/src/cli/self_update/unix.rs +++ b/src/cli/self_update/unix.rs @@ -140,6 +140,8 @@ 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 { + #[cfg(feature = "test")] + process.checkpoint(super::CHECKPOINT_SELF_REPLACE_READY); install_bins(process)?; 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/tests/suite/cli_self_upd.rs b/tests/suite/cli_self_upd.rs index 83ca66b671..0c3e36e2c1 100644 --- a/tests/suite/cli_self_upd.rs +++ b/tests/suite/cli_self_upd.rs @@ -8,6 +8,8 @@ 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::{ @@ -21,8 +23,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; @@ -556,6 +556,31 @@ async fn update_but_delete_existing_updater_first() { assert!(rustup.exists()); } +#[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] async fn update_download_404() { let cx = SelfUpdateTestContext::new(TEST_VERSION).await; @@ -1225,3 +1250,5 @@ async fn install_minimal_profile() { cx.config.expect_component_executable("rustc").await; cx.config.expect_component_not_executable("cargo").await; } + +const TEST_VERSION: &str = "1.1.1"; From 07d6948efe4eb3be4eff2be4125281647f00489f Mon Sep 17 00:00:00 2001 From: Akrm Al-Hakimi Date: Mon, 14 Sep 2026 08:34:27 -0600 Subject: [PATCH 2/6] style(self-update): move test constant --- tests/suite/cli_self_upd.rs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/tests/suite/cli_self_upd.rs b/tests/suite/cli_self_upd.rs index 0c3e36e2c1..b7a47ee114 100644 --- a/tests/suite/cli_self_upd.rs +++ b/tests/suite/cli_self_upd.rs @@ -509,12 +509,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; @@ -1252,3 +1246,8 @@ async fn install_minimal_profile() { } 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", +}; From 40da33c969d312564acf538bca6007f943dfd294 Mon Sep 17 00:00:00 2001 From: Akrm Al-Hakimi Date: Tue, 15 Sep 2026 16:19:17 -0400 Subject: [PATCH 3/6] refactor(self-update): tidy imports and constant placement Import sibling items through `super` in the Windows module and move `DEFAULT_UPDATE_ROOT` below its users, as the coding standards prefer. No functional change. --- src/cli/self_update.rs | 3 +-- src/cli/self_update/windows.rs | 12 ++++-------- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/src/cli/self_update.rs b/src/cli/self_update.rs index 690acd6c39..7e96fd7b4a 100644 --- a/src/cli/self_update.rs +++ b/src/cli/self_update.rs @@ -583,8 +583,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") @@ -1381,6 +1379,7 @@ pub(crate) fn cleanup_self_updater(process: &Process) -> anyhow::Result<()> { Ok(()) } +static DEFAULT_UPDATE_ROOT: &str = "https://static.rust-lang.org/rustup"; #[cfg(feature = "test")] pub const CHECKPOINT_SELF_REPLACE_READY: &str = "self-replace-ready"; diff --git a/src/cli/self_update/windows.rs b/src/cli/self_update/windows.rs index 28fcdcfdac..f22de6a996 100644 --- a/src/cli/self_update/windows.rs +++ b/src/cli/self_update/windows.rs @@ -3,7 +3,7 @@ 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 +18,9 @@ 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, install_bins, report_error}; 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 +37,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) { From 124229aea4397ec73ca80fbfe3b6deeea733f0d1 Mon Sep 17 00:00:00 2001 From: Akrm Al-Hakimi Date: Tue, 15 Sep 2026 16:19:17 -0400 Subject: [PATCH 4/6] fix(self-update): stage the updater under RUSTUP_HOME behind a lock Self-update downloaded every updater to the shared path `$CARGO_HOME/bin/rustup-init`, and every rustup or proxy invocation deleted that path during startup cleanup. A proxy starting while the updater was still replacing rustup could therefore delete the updater out from under it (#5076, #1864). The updater now lives at `$RUSTUP_HOME/self-update/rustup-init`, and both the download and the replacement run under a global self-update lock that the OS releases when the owning process exits. The replacer records a `complete` or `failed` marker next to the updater. Cleanup only removes an updater that is marked finished or has been abandoned for a day, and only when it can take the lock. The legacy path is left alone until it is stale as well, because an older rustup may still be running an updater from there. `install_bins` becomes a method on the lock so that rustup can only be replaced while the lock is held. --- src/cli/self_update.rs | 99 +++----- src/cli/self_update/stage.rs | 412 +++++++++++++++++++++++++++++++++ src/cli/self_update/unix.rs | 30 ++- src/cli/self_update/windows.rs | 22 +- tests/suite/cli_self_upd.rs | 81 +++++-- 5 files changed, 545 insertions(+), 99 deletions(-) create mode 100644 src/cli/self_update/stage.rs diff --git a/src/cli/self_update.rs b/src/cli/self_update.rs index 7e96fd7b4a..6e5c0572c9 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::{PreparedUpdate, 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_update = prepare_update(dl_cfg).await?; - if let Some(setup_path) = &setup_path { - return run_update(setup_path, dl_cfg.process); + if let Some(prepared_update) = prepared_update { + return run_update(prepared_update, dl_cfg.process); } else { // Try again in case we emitted "tool `{}` is already installed" last time. install_proxies(dl_cfg.process)?; @@ -771,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<()> { @@ -1120,21 +1110,10 @@ pub(crate) fn self_update_permitted(explicit: bool) -> anyhow::Result) -> anyhow::Result { common::warn_if_host_is_emulated(cfg.process); @@ -1160,8 +1139,9 @@ 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_update) => { + let Some(version) = get_and_parse_new_rustup_version(prepared_update.updater_path()) + else { error!("failed to get rustup version"); return Ok(ExitCode::FAILURE); }; @@ -1171,7 +1151,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_update, cfg.process); } None => { let _ = common::show_channel_update( @@ -1212,18 +1192,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(); @@ -1263,18 +1239,22 @@ pub(crate) async fn prepare_update(dl_cfg: &DownloadCfg<'_>) -> anyhow::Result) -> anyhow::Result { @@ -1369,18 +1349,13 @@ 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)] @@ -1438,8 +1413,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..1fbf439648 --- /dev/null +++ b/src/cli/self_update/stage.rs @@ -0,0 +1,412 @@ +use std::{ + env::consts::EXE_SUFFIX, + fs::{self, File, OpenOptions}, + io, + path::{Path, PathBuf}, + process::Command, + time::{Duration, SystemTime}, +}; + +use anyhow::Context; +use tracing::{debug, 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) + .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 updater_path = self.updater_path(); + utils::ensure_file_removed("self-updater", &updater_path)?; + for marker in [Marker::Complete, Marker::Failed] { + utils::ensure_file_removed( + "self-update status marker", + &self.directory.join(marker.as_str()), + )?; + } + Ok(PreparedUpdate { + updater_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<()> { + 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) + } + + 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 PreparedUpdate { + updater_path: PathBuf, + _lock: SelfUpdateLock, +} + +impl PreparedUpdate { + pub(super) fn replacer_command(&self) -> anyhow::Result { + let stage = self + .updater_path + .parent() + .context("self-updater path has no parent directory")?; + let mut command = Command::new(&self.updater_path); + command.env(STAGE_ENV, stage); + Ok(command) + } + + pub(super) fn updater_path(&self) -> &Path { + &self.updater_path + } +} + +pub(super) fn mark_result(process: &Process, succeeded: bool) { + 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) = mark_stage(process, &stage, marker) { + warn!("could not record self-update result: {error}"); + } +} + +pub(super) fn cleanup(process: &Process) -> anyhow::Result<()> { + cleanup_at(process, SystemTime::now()) +} + +fn mark_stage(process: &Process, stage: &Path, marker: Marker) -> 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", + &stage.join(marker.as_str()), + "", + ) +} + +fn cleanup_at(process: &Process, now: SystemTime) -> anyhow::Result<()> { + if let Some(lock) = SelfUpdateLock::try_acquire(process)? { + let updater = lock.updater_path(); + if (is_finished(&lock.directory) || is_stale(&updater, now)) + && remove_file_best_effort("self-updater", &updater) + { + for marker in [Marker::Complete, Marker::Failed] { + remove_file_best_effort( + "self-update status marker", + &lock.directory.join(marker.as_str()), + ); + } + } + } + + let updater = process + .cargo_home()? + .join(format!("bin/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) { + remove_file_best_effort("legacy self-updater", &updater); + } + + Ok(()) +} + +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 + } + } +} + +fn is_finished(stage: &Path) -> bool { + [Marker::Complete, Marker::Failed] + .iter() + .any(|marker| stage.join(marker.as_str()).is_file()) +} + +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) +} + +fn stage_root(process: &Process) -> anyhow::Result { + Ok(process.rustup_home()?.join(SELF_UPDATE_DIRECTORY)) +} + +/// Outcome recorded next to the managed updater once replacement has finished. +#[derive(Clone, Copy)] +enum Marker { + Complete, + Failed, +} + +impl Marker { + fn as_str(&self) -> &'static str { + match self { + Self::Complete => "complete", + Self::Failed => "failed", + } + } +} + +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 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(stage.join(Marker::Complete.as_str()), "").unwrap(); + drop(first); + let second = SelfUpdateLock::acquire(&process.process) + .unwrap() + .prepare_updater() + .unwrap(); + + assert_eq!(&first_path, second.updater_path()); + assert!(!stage.join(Marker::Complete.as_str()).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 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(lock.directory.join(Marker::Complete.as_str()), "").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 replacer_command_rejects_parentless_path() { + let root = test_dir().unwrap(); + let process = test_process(root.path()); + let prepared_update = PreparedUpdate { + updater_path: PathBuf::new(), + _lock: SelfUpdateLock::acquire(&process.process).unwrap(), + }; + let error = prepared_update.replacer_command().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_update = SelfUpdateLock::acquire(&process.process) + .unwrap() + .prepare_updater() + .unwrap(); + let updater = prepared_update.updater_path().to_owned(); + fs::write(&updater, "").unwrap(); + drop(prepared_update); + + 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_update = SelfUpdateLock::acquire(&process.process) + .unwrap() + .prepare_updater() + .unwrap(); + let updater = prepared_update.updater_path().to_owned(); + fs::write(&updater, "").unwrap(); + drop(prepared_update); + mark_stage(&process.process, &stage, marker).unwrap(); + + cleanup_at(&process.process, SystemTime::now()).unwrap(); + + assert!(!updater.exists()); + assert!(!stage.join(marker.as_str()).exists()); + } + } + + #[tokio::test] + async fn cleanup_removes_abandoned_updater() { + let root = test_dir().unwrap(); + let process = test_process(root.path()); + let prepared_update = SelfUpdateLock::acquire(&process.process) + .unwrap() + .prepare_updater() + .unwrap(); + let updater = prepared_update.updater_path().to_owned(); + fs::write(&updater, "").unwrap(); + drop(prepared_update); + + 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()); + } + + 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 dfb114ff72..7ec980765e 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, PreparedUpdate, SelfUpdateLock}, }; use crate::{process::Process, utils}; @@ -121,13 +118,21 @@ 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) +pub(super) fn run_update( + prepared_update: PreparedUpdate, + _process: &Process, +) -> anyhow::Result { + let setup_path = prepared_update.updater_path().to_owned(); + let mut updater = prepared_update + .replacer_command()? .arg("--self-replace") - .status() + .spawn() .context(format!("unable to run updater ({})", setup_path.display()))?; + drop(prepared_update); + let status = updater.wait().context(format!( + "unable to wait for updater ({})", + setup_path.display() + ))?; if !status.success() { bail!("self-updated failed to replace rustup executable"); @@ -140,9 +145,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 { + let self_update_lock = SelfUpdateLock::acquire(process)?; #[cfg(feature = "test")] process.checkpoint(super::CHECKPOINT_SELF_REPLACE_READY); - install_bins(process)?; + let result = self_update_lock.install_bins(process); + stage::mark_result(process, result.is_ok()); + result?; Ok(utils::ExitCode(0)) } diff --git a/src/cli/self_update/windows.rs b/src/cli/self_update/windows.rs index f22de6a996..14a044aae4 100644 --- a/src/cli/self_update/windows.rs +++ b/src/cli/self_update/windows.rs @@ -5,7 +5,6 @@ use std::{ fmt, io::{self, Write}, os::windows::ffi::OsStrExt, - path::Path, process::Command, }; @@ -18,7 +17,10 @@ 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, install_bins, report_error}; +use super::{ + InstallOpts, report_error, + stage::{self, PreparedUpdate, SelfUpdateLock}, +}; use crate::{ cli::{common, errors::CliError, markdown::md}, dist::TargetTuple, @@ -654,13 +656,18 @@ 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) +pub(super) fn run_update( + prepared_update: PreparedUpdate, + process: &Process, +) -> anyhow::Result { + prepared_update + .replacer_command()? .arg("--self-replace") .spawn() .context("unable to run updater")?; - let Some(version) = super::get_and_parse_new_rustup_version(setup_path) else { + let Some(version) = super::get_and_parse_new_rustup_version(prepared_update.updater_path()) + else { warn!("failed to get the new rustup version in order to update `DisplayVersion`"); return Ok(utils::ExitCode(1)); }; @@ -671,7 +678,10 @@ pub(crate) fn run_update(setup_path: &Path, process: &Process) -> anyhow::Result 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); + stage::mark_result(process, result.is_ok()); + result?; Ok(utils::ExitCode(0)) } diff --git a/tests/suite/cli_self_upd.rs b/tests/suite/cli_self_upd.rs index b7a47ee114..bb78d80c7d 100644 --- a/tests/suite/cli_self_upd.rs +++ b/tests/suite/cli_self_upd.rs @@ -1,9 +1,13 @@ //! 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, @@ -14,6 +18,7 @@ use rustup::cli::self_update::CHECKPOINT_SELF_REPLACE_READY; 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, @@ -525,9 +530,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 @@ -538,8 +542,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"]) @@ -548,6 +550,33 @@ 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)] @@ -809,11 +838,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] @@ -828,17 +853,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] @@ -853,14 +875,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] @@ -1245,6 +1264,26 @@ async fn install_minimal_profile() { 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 { From acb1093123af22aecc284edc2ba9b09bde1e01c2 Mon Sep 17 00:00:00 2001 From: Akrm Al-Hakimi Date: Tue, 15 Sep 2026 16:19:17 -0400 Subject: [PATCH 5/6] fix(self-update): publish the rustup binary atomically Replacement used to unlink the installed rustup and then copy the updater over the freed path. Any failure in between, such as the updater having been deleted meanwhile, left `$CARGO_HOME/bin` without a rustup at all. The new binary is now copied to a `.rustup-pending-*` sibling, synced to disk, and then renamed over the installed rustup. `std::fs::rename` replaces an existing destination in one step on every platform, so a failure before publication leaves the existing rustup untouched. Pending files abandoned by a crash are cleaned up once they are a day old. --- src/cli/self_update/stage.rs | 153 ++++++++++++++++++++++++++++++++--- 1 file changed, 142 insertions(+), 11 deletions(-) diff --git a/src/cli/self_update/stage.rs b/src/cli/self_update/stage.rs index 1fbf439648..6f170d8807 100644 --- a/src/cli/self_update/stage.rs +++ b/src/cli/self_update/stage.rs @@ -8,6 +8,7 @@ use std::{ }; use anyhow::Context; +use tempfile::TempPath; use tracing::{debug, warn}; use super::install_proxies; @@ -70,18 +71,16 @@ impl SelfUpdateLock { /// 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 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)?; + let pending = stage_pending_binary(this_exe_path, &bin_path)?; + replace_rustup_binary(&pending, &rustup_path)?; install_proxies(process) } @@ -130,6 +129,52 @@ pub(super) fn cleanup(process: &Process) -> anyhow::Result<()> { cleanup_at(process, SystemTime::now()) } +fn stage_pending_binary(this_exe_path: &Path, bin_path: &Path) -> anyhow::Result { + 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")?; + } + + Ok(pending) +} + +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. + fs::rename(replacement, rustup).with_context(|| { + format!( + "failed to replace rustup binary '{}' with '{}'", + rustup.display(), + replacement.display() + ) + })?; + // 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 mark_stage(process: &Process, stage: &Path, marker: Marker) -> anyhow::Result<()> { if stage != stage_root(process)? { warn!( @@ -161,9 +206,31 @@ fn cleanup_at(process: &Process, now: SystemTime) -> anyhow::Result<()> { } } - let updater = process - .cargo_home()? - .join(format!("bin/rustup-init{EXE_SUFFIX}")); + 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) + { + 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) { @@ -233,6 +300,7 @@ impl Marker { 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)] @@ -287,6 +355,47 @@ mod tests { 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(); @@ -402,6 +511,28 @@ mod tests { 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); From c1d76d84d69bd45480c5528d7cf49ae833c1405a Mon Sep 17 00:00:00 2001 From: Akrm Al-Hakimi Date: Tue, 15 Sep 2026 16:19:17 -0400 Subject: [PATCH 6/6] fix(self-update): record DisplayVersion from the replacer on Windows After spawning the replacer, the parent ran the updater a second time with `--version` and wrote the result to the uninstall registry entry. The registry could therefore claim a version that was never installed if the replacer went on to fail. The replacer is the new rustup and knows its own version, so it now updates `DisplayVersion` right after installing the binaries, under the same self-update lock. The test waits for the completion marker because the registry is now written after `rustup self update` has returned. --- src/cli/self_update/windows.rs | 14 +++++--------- tests/suite/cli_self_upd.rs | 1 + 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/src/cli/self_update/windows.rs b/src/cli/self_update/windows.rs index 14a044aae4..0311760a31 100644 --- a/src/cli/self_update/windows.rs +++ b/src/cli/self_update/windows.rs @@ -658,20 +658,14 @@ pub(crate) fn remove_uninstall_registry_entry(process: &Process) -> anyhow::Resu pub(super) fn run_update( prepared_update: PreparedUpdate, - process: &Process, + _process: &Process, ) -> anyhow::Result { prepared_update .replacer_command()? .arg("--self-replace") .spawn() .context("unable to run updater")?; - - let Some(version) = super::get_and_parse_new_rustup_version(prepared_update.updater_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)?; + drop(prepared_update); Ok(utils::ExitCode(0)) } @@ -679,7 +673,9 @@ pub(super) fn run_update( pub(crate) fn self_replace(process: &Process) -> anyhow::Result { wait_for_parent()?; let self_update_lock = SelfUpdateLock::acquire(process)?; - let result = self_update_lock.install_bins(process); + let result = self_update_lock.install_bins(process).and_then(|()| { + update_uninstall_registry_display_version(env!("CARGO_PKG_VERSION"), process) + }); stage::mark_result(process, result.is_ok()); result?; diff --git a/tests/suite/cli_self_upd.rs b/tests/suite/cli_self_upd.rs index bb78d80c7d..89eae6f39a 100644 --- a/tests/suite/cli_self_upd.rs +++ b/tests/suite/cli_self_upd.rs @@ -505,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)