From 2747e452f1238844eb1558b1b07ee463c9414cd3 Mon Sep 17 00:00:00 2001 From: Akrm Al-Hakimi Date: Sun, 13 Sep 2026 15:40:14 -0600 Subject: [PATCH 01/10] test(process): allow resuming a parked checkpoint A process parked at a checkpoint could only be killed by the test driver. Poll for the checkpoint marker instead of sleeping, and add `ParkedChild::resume()`, which removes the marker and waits for the process to finish. This lets a test race another command against a paused operation and then observe how the paused one completes. --- src/process.rs | 11 +++++++---- src/test/clitools.rs | 27 +++++++++++++++++++++++++-- 2 files changed, 32 insertions(+), 6 deletions(-) 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}"); } } From 4620cb7330cefbfae27e74221cf000becd82a41e Mon Sep 17 00:00:00 2001 From: Akrm Al-Hakimi Date: Sun, 13 Sep 2026 15:40:14 -0600 Subject: [PATCH 02/10] test(self-update): reproduce proxy cleanup race Park the replacer right before it installs the new binaries and run a proxy in the meantime. The proxy's startup cleanup deletes `$CARGO_HOME/bin/rustup-init`, which is the replacer's own executable, so the replacer unlinks the installed rustup and then fails to copy itself over it, leaving no rustup behind (#5076, #1864). The test asserts this current behaviour so that it passes on its own; the fix later in this series updates it to assert that the replacement succeeds. --- src/cli/self_update.rs | 3 +++ src/cli/self_update/unix.rs | 2 ++ tests/suite/cli_self_upd.rs | 30 ++++++++++++++++++++++++++++-- 3 files changed, 33 insertions(+), 2 deletions(-) diff --git a/src/cli/self_update.rs b/src/cli/self_update.rs index 0e2b70ab39..03386f2c69 100644 --- a/src/cli/self_update.rs +++ b/src/cli/self_update.rs @@ -1392,6 +1392,9 @@ pub(crate) fn cleanup_self_updater(bin_path: &Path) -> anyhow::Result<()> { Ok(()) } +#[cfg(feature = "test")] +pub const CHECKPOINT_SELF_REPLACE_READY: &str = "self-replace-ready"; + #[cfg(test)] mod tests { use std::{collections::HashMap, path::Path}; diff --git a/src/cli/self_update/unix.rs b/src/cli/self_update/unix.rs index f9378261a4..69a5af8a47 100644 --- a/src/cli/self_update/unix.rs +++ b/src/cli/self_update/unix.rs @@ -133,6 +133,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.cargo_home()?.join("bin"), super::force_hard_links(process), diff --git a/tests/suite/cli_self_upd.rs b/tests/suite/cli_self_upd.rs index 827fa19ff8..33400fc14c 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; @@ -548,6 +548,30 @@ async fn update_but_delete_existing_updater_first() { assert!(rustup.exists()); } +#[cfg(unix)] +#[tokio::test] +async fn self_update_replacement_races_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 parked = cx.spawn_at(CHECKPOINT_SELF_REPLACE_READY, ["rustup", "self", "update"]); + + // The proxy's startup cleanup deletes `$CARGO_HOME/bin/rustup-init`, + // which is the parked replacer's own executable. + cx.config.expect(["rustc", "--version"]).await.is_ok(); + + // The replacer unlinks the installed rustup before copying itself over + // it, so it fails and leaves no rustup behind. + let status = parked.resume(); + assert!(!status.success()); + assert!(!rustup.exists()); +} + #[tokio::test] async fn update_download_404() { let cx = SelfUpdateTestContext::new(TEST_VERSION).await; @@ -1217,3 +1241,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 d6ddc146249a438bd5d0cabbdaa21798d142bceb Mon Sep 17 00:00:00 2001 From: Akrm Al-Hakimi Date: Mon, 14 Sep 2026 08:34:27 -0600 Subject: [PATCH 03/10] 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 33400fc14c..4ecacce1f9 100644 --- a/tests/suite/cli_self_upd.rs +++ b/tests/suite/cli_self_upd.rs @@ -501,12 +501,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; @@ -1243,3 +1237,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 f7950ef140706cd1afa852df1e625f32b0e8355d Mon Sep 17 00:00:00 2001 From: Akrm Al-Hakimi Date: Tue, 15 Sep 2026 16:19:17 -0400 Subject: [PATCH 04/10] 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 | 8 ++------ 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/src/cli/self_update.rs b/src/cli/self_update.rs index 03386f2c69..e10b959f55 100644 --- a/src/cli/self_update.rs +++ b/src/cli/self_update.rs @@ -579,8 +579,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") @@ -1392,6 +1390,7 @@ pub(crate) fn cleanup_self_updater(bin_path: &Path) -> 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 bdd40e1505..7ddaee431c 100644 --- a/src/cli/self_update/windows.rs +++ b/src/cli/self_update/windows.rs @@ -44,13 +44,9 @@ use windows_sys::Win32::{ }, }; +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}, From 63a15db2adb305ade6af5541a0e896b446b345b6 Mon Sep 17 00:00:00 2001 From: Akrm Al-Hakimi Date: Thu, 17 Sep 2026 08:12:08 -0400 Subject: [PATCH 05/10] fix(self-update): serialize self-updates with a lock Two concurrent `rustup self update` invocations shared one updater path and could overwrite each other's download or replacement (#1864). `prepare_update` now takes a global self-update lock before downloading and hands it to `run_update` inside a `PreparedUpdater`, which releases it only once the replacer has been spawned. The replacer takes the same lock before replacing rustup, so `install_bins` becomes a method on the lock and can only run while it is held. The lock file lives under `$RUSTUP_HOME/self-update/` and is released by the OS when the owning process exits, so a crash can never leave it held. --- src/cli/self_update.rs | 66 +++++++-------- src/cli/self_update/stage.rs | 145 +++++++++++++++++++++++++++++++++ src/cli/self_update/unix.rs | 27 +++--- src/cli/self_update/windows.rs | 20 +++-- 4 files changed, 202 insertions(+), 56 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 e10b959f55..2c21dc91b5 100644 --- a/src/cli/self_update.rs +++ b/src/cli/self_update.rs @@ -78,28 +78,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::{add_to_path, remove_from_path}; +pub(crate) use unix::self_replace; #[cfg(unix)] -pub(crate) use unix::{run_update, self_replace}; +use unix::{add_to_path, 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_to_path, add_uninstall_registry_entry, remove_from_path, 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, @@ -239,7 +243,7 @@ impl InstallOpts<'_> { process: &Process, ) -> anyhow::Result<()> { let cargo_bin = process.cargo_home()?.join("bin"); - install_bins(&cargo_bin, force_hard_links(process))?; + install_bins(process, &cargo_bin, force_hard_links(process))?; #[cfg(unix)] unix::write_env_files(process)?; @@ -529,10 +533,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)?; @@ -783,19 +787,8 @@ fn warn_if_default_linker_missing(process: &Process) { } } -fn install_bins(bin_path: &Path, force_hard_links: bool) -> anyhow::Result<()> { - 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_with_opts(bin_path, force_hard_links) +fn install_bins(process: &Process, bin_path: &Path, force_hard_links: bool) -> anyhow::Result<()> { + SelfUpdateLock::lock(process)?.install_bins(bin_path, force_hard_links) } pub(crate) fn install_proxies(process: &Process) -> anyhow::Result<()> { @@ -1172,8 +1165,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) else { error!("failed to get rustup version"); return Ok(ExitCode::FAILURE); }; @@ -1183,7 +1176,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( @@ -1224,18 +1217,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::lock(dl_cfg.process)?; // Get build tuple let tuple = TargetTuple::from_build(); @@ -1275,18 +1264,20 @@ pub(crate) async fn prepare_update(dl_cfg: &DownloadCfg<'_>) -> anyhow::Result) -> anyhow::Result { @@ -1405,7 +1396,7 @@ mod tests { dist::{PartialToolchainDesc, Profile}, for_host, process::TestProcess, - test::{test_dir, with_rustup_home}, + test::{Env, test_dir, with_rustup_home}, }; #[test] @@ -1492,7 +1483,12 @@ 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"); - super::install_bins(&cargo_home.join("bin"), false).unwrap(); + 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, &cargo_home.join("bin"), false).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..9d4370aa72 --- /dev/null +++ b/src/cli/self_update/stage.rs @@ -0,0 +1,145 @@ +use std::{ + env::consts::EXE_SUFFIX, + fs::{File, OpenOptions}, + ops::Deref, + path::{Path, PathBuf}, + process::{Child, Command}, +}; + +use anyhow::Context; + +use super::install_proxies_with_opts; +use crate::{process::Process, utils}; + +/// Exclusive right to download the updater or replace the installed rustup. +pub(super) struct SelfUpdateLock { + _file: File, +} + +impl SelfUpdateLock { + pub(super) fn lock(process: &Process) -> anyhow::Result { + let lock = Self::open(process)?; + lock._file.lock().context("failed to lock self-update")?; + Ok(lock) + } + + 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 { _file: file }) + } + + /// Removes any leftover updater and reserves its path for the download. + pub(super) fn prepare_updater(self, process: &Process) -> anyhow::Result { + let path = process + .cargo_home()? + .join(format!("bin/rustup-init{EXE_SUFFIX}")); + utils::ensure_file_removed("self-updater", &path)?; + Ok(PreparedUpdater { path, _lock: self }) + } + + /// Installs the running executable as `rustup` in `bin_path` and refreshes its proxies. + pub(super) fn install_bins( + &self, + bin_path: &Path, + force_hard_links: bool, + ) -> anyhow::Result<()> { + 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_with_opts(bin_path, force_hard_links) + } +} + +/// The 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 { + Command::new(&self.path) + .arg("--self-replace") + .spawn() + .with_context(|| format!("unable to run updater ({})", self.path.display())) + } +} + +impl Deref for PreparedUpdater { + type Target = Path; + + fn deref(&self) -> &Path { + &self.path + } +} + +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"; + +#[cfg(test)] +mod tests { + use std::{collections::HashMap, fs}; + + use super::*; + use crate::{ + process::TestProcess, + test::{Env, test_dir}, + }; + + #[tokio::test] + async fn self_update_lock_is_global() { + let root = test_dir().unwrap(); + let process = test_process(root.path()); + let lock = SelfUpdateLock::lock(&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(); + } + + 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 69a5af8a47..483059e3c6 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::{Path, PathBuf}; use anyhow::{Context, bail}; use tracing::{error, warn}; use super::{ - install_bins, shell::{self, Posix, UnixShell}, + stage::{PreparedUpdater, SelfUpdateLock}, }; use crate::{process::Process, utils}; @@ -114,13 +111,16 @@ pub(crate) fn 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.to_path_buf(); + 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"); @@ -133,9 +133,10 @@ 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::lock(process)?; #[cfg(feature = "test")] process.checkpoint(super::CHECKPOINT_SELF_REPLACE_READY); - install_bins( + self_update_lock.install_bins( &process.cargo_home()?.join("bin"), super::force_hard_links(process), )?; diff --git a/src/cli/self_update/windows.rs b/src/cli/self_update/windows.rs index 7ddaee431c..55054aa3d0 100644 --- a/src/cli/self_update/windows.rs +++ b/src/cli/self_update/windows.rs @@ -44,7 +44,10 @@ use windows_sys::Win32::{ }, }; -use super::{InstallOpts, install_bins, report_error}; +use super::{ + InstallOpts, report_error, + stage::{PreparedUpdater, SelfUpdateLock}, +}; use crate::{ cli::{common, errors::CliError, markdown::md}, dist::TargetTuple, @@ -651,13 +654,14 @@ 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")?; +pub(super) fn run_update( + prepared_updater: PreparedUpdater, + process: &Process, +) -> anyhow::Result { + let updater_path = prepared_updater.to_path_buf(); + prepared_updater.spawn_replacer()?; - let Some(version) = super::get_and_parse_new_rustup_version(setup_path) else { + let Some(version) = super::get_and_parse_new_rustup_version(&updater_path) else { warn!("failed to get the new rustup version in order to update `DisplayVersion`"); return Ok(utils::ExitCode(1)); }; @@ -668,7 +672,7 @@ 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( + SelfUpdateLock::lock(process)?.install_bins( &process.cargo_home()?.join("bin"), super::force_hard_links(process), )?; From b67072c672f03b8ceb526bdd664b6d3f64a9b2de Mon Sep 17 00:00:00 2001 From: Akrm Al-Hakimi Date: Thu, 17 Sep 2026 08:12:08 -0400 Subject: [PATCH 06/10] fix(self-update): move the updater under RUSTUP_HOME and mark its outcome Every rustup or proxy invocation deleted `$CARGO_HOME/bin/rustup-init` during startup cleanup. A proxy starting while the updater was still replacing rustup could therefore delete the updater out from under it (#5076). The updater now lives at `$RUSTUP_HOME/self-update/rustup-init`, and the replacer records a `complete` or `failed` marker next to it once it is done. Startup cleanup removes the managed updater only when such a marker exists and the self-update lock is free, so an update in progress is never touched. The legacy path is still cleaned as before. The proxy cleanup race test now asserts that the replacement succeeds and the installed rustup is updated. --- src/cli/proxy_mode.rs | 2 +- src/cli/rustup_mode.rs | 2 +- src/cli/self_update.rs | 47 +++---- src/cli/self_update/stage.rs | 228 +++++++++++++++++++++++++++++++-- src/cli/self_update/unix.rs | 11 +- src/cli/self_update/windows.rs | 12 +- src/utils/mod.rs | 28 ++++ tests/suite/cli_self_upd.rs | 95 ++++++++++---- 8 files changed, 346 insertions(+), 79 deletions(-) diff --git a/src/cli/proxy_mode.rs b/src/cli/proxy_mode.rs index 406cd3fa86..27cf03ec0f 100644 --- a/src/cli/proxy_mode.rs +++ b/src/cli/proxy_mode.rs @@ -14,7 +14,7 @@ pub async fn main( current_dir: PathBuf, process: &Process, ) -> anyhow::Result { - self_update::cleanup_self_updater(&process.cargo_home()?.join("bin"))?; + self_update::cleanup_self_updater(process, &process.cargo_home()?.join("bin"))?; let _setup = job::setup(); let mut args = process.args_os().skip(1); diff --git a/src/cli/rustup_mode.rs b/src/cli/rustup_mode.rs index e0a27f6bdf..50721aa8f6 100644 --- a/src/cli/rustup_mode.rs +++ b/src/cli/rustup_mode.rs @@ -703,7 +703,7 @@ pub async fn main( .bin("rustup") .complete(); - self_update::cleanup_self_updater(&process.cargo_home()?.join("bin"))?; + self_update::cleanup_self_updater(process, &process.cargo_home()?.join("bin"))?; use clap::error::ErrorKind::*; let matches = match Rustup::try_parse_from(process.args_os()) { diff --git a/src/cli/self_update.rs b/src/cli/self_update.rs index 2c21dc91b5..d0637ab268 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`): //! @@ -79,6 +78,8 @@ use crate::{ mod msg; mod stage; +#[cfg(feature = "test")] +pub use stage::{Marker, SELF_UPDATE_DIRECTORY, updater_path}; use stage::{PreparedUpdater, SelfUpdateLock}; #[cfg(unix)] @@ -1125,21 +1126,10 @@ pub(crate) fn self_update_permitted(explicit: bool) -> anyhow::Result) -> anyhow::Result { common::warn_if_host_is_emulated(cfg.process); @@ -1264,7 +1254,7 @@ async fn prepare_update(dl_cfg: &DownloadCfg<'_>) -> anyhow::Result) -> anyhow::Result) -> anyhow::Res } #[tracing::instrument(level = "trace")] -pub(crate) fn cleanup_self_updater(bin_path: &Path) -> anyhow::Result<()> { - let setup = bin_path.join(format!("rustup-init{EXE_SUFFIX}")); - - if setup.exists() { - utils::remove_file("setup", &setup)?; - } - - Ok(()) +pub(crate) fn cleanup_self_updater(process: &Process, bin_path: &Path) -> anyhow::Result<()> { + stage::cleanup(process, bin_path) } 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)] diff --git a/src/cli/self_update/stage.rs b/src/cli/self_update/stage.rs index 9d4370aa72..b4266b04ac 100644 --- a/src/cli/self_update/stage.rs +++ b/src/cli/self_update/stage.rs @@ -1,18 +1,20 @@ use std::{ env::consts::EXE_SUFFIX, - fs::{File, OpenOptions}, + fs::{self, File, OpenOptions}, ops::Deref, path::{Path, PathBuf}, process::{Child, Command}, }; use anyhow::Context; +use tracing::warn; use super::install_proxies_with_opts; use crate::{process::Process, utils}; /// Exclusive right to download the updater or replace the installed rustup. pub(super) struct SelfUpdateLock { + directory: PathBuf, _file: File, } @@ -23,6 +25,15 @@ impl SelfUpdateLock { Ok(lock) } + fn try_lock(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)?; @@ -35,15 +46,19 @@ impl SelfUpdateLock { .open(directory.join(SELF_UPDATE_LOCK_FILE)) .context("failed to open self-update lock")?; - Ok(Self { _file: file }) + Ok(Self { + directory, + _file: file, + }) } - /// Removes any leftover updater and reserves its path for the download. - pub(super) fn prepare_updater(self, process: &Process) -> anyhow::Result { - let path = process - .cargo_home()? - .join(format!("bin/rustup-init{EXE_SUFFIX}")); + /// Clears the previous update's leftovers and reserves the managed updater path. + pub(super) fn prepare_updater(self) -> anyhow::Result { + let path = updater_path(&self.directory); 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 }) } @@ -68,7 +83,7 @@ impl SelfUpdateLock { } } -/// The updater path, held together with the lock that protects it. +/// The managed updater path, held together with the lock that protects it. pub(super) struct PreparedUpdater { path: PathBuf, _lock: SelfUpdateLock, @@ -80,7 +95,12 @@ impl PreparedUpdater { /// 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())) @@ -95,16 +115,96 @@ impl Deref for PreparedUpdater { } } +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, bin_path: &Path) -> anyhow::Result<()> { + if let Some(lock) = SelfUpdateLock::try_lock(process)? { + let updater = updater_path(&lock.directory); + // The replacer records an outcome only once it has finished, so an + // unmarked updater may still be about to run and is left alone. + let markers = [Marker::Complete, Marker::Failed]; + let finished = markers + .iter() + .any(|marker| marker.path(&lock.directory).is_file()); + if finished && 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 updater = bin_path.join(format!("rustup-init{EXE_SUFFIX}")); + if updater.exists() { + utils::remove_file("legacy self-updater", &updater)?; + } + + Ok(()) +} + +/// Outcome recorded next to the managed updater once replacement has finished. +#[derive(Clone, Copy)] +pub 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), "") + } + + pub 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", + } + } +} + +/// The managed updater inside the `stage` directory. +pub fn updater_path(stage: &Path) -> PathBuf { + stage.join(format!("rustup-init{EXE_SUFFIX}")) +} + fn stage_root(process: &Process) -> anyhow::Result { Ok(process.rustup_home()?.join(SELF_UPDATE_DIRECTORY)) } -const SELF_UPDATE_DIRECTORY: &str = "self-update"; +pub const SELF_UPDATE_DIRECTORY: &str = "self-update"; const SELF_UPDATE_LOCK_FILE: &str = "self-update.lock"; +const STAGE_ENV: &str = "RUSTUP_SELF_UPDATE_STAGE"; #[cfg(test)] mod tests { - use std::{collections::HashMap, fs}; + use std::collections::HashMap; use super::*; use crate::{ @@ -112,6 +212,25 @@ mod tests { 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::lock(&process.process).unwrap(); + let first_path = updater_path(&first.directory); + let stage = first.directory.clone(); + fs::write(&first_path, "").unwrap(); + fs::write(Marker::Complete.path(&stage), "").unwrap(); + drop(first); + let second = SelfUpdateLock::lock(&process.process) + .unwrap() + .prepare_updater() + .unwrap(); + + assert_eq!(first_path, *second); + assert!(!Marker::Complete.path(&stage).exists()); + } + #[tokio::test] async fn self_update_lock_is_global() { let root = test_dir().unwrap(); @@ -135,6 +254,95 @@ mod tests { 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::lock(&process.process).unwrap(); + let updater = updater_path(&lock.directory); + fs::write(&updater, "").unwrap(); + fs::write(Marker::Complete.path(&lock.directory), "").unwrap(); + + cleanup( + &process.process, + &process.process.cargo_home().unwrap().join("bin"), + ) + .unwrap(); + + assert!(updater.exists()); + drop(lock); + cleanup( + &process.process, + &process.process.cargo_home().unwrap().join("bin"), + ) + .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::lock(&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::lock(&process.process) + .unwrap() + .prepare_updater() + .unwrap(); + let updater = prepared_updater.to_path_buf(); + fs::write(&updater, "").unwrap(); + drop(prepared_updater); + + cleanup( + &process.process, + &process.process.cargo_home().unwrap().join("bin"), + ) + .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::lock(&process.process) + .unwrap() + .prepare_updater() + .unwrap(); + let updater = prepared_updater.to_path_buf(); + fs::write(&updater, "").unwrap(); + drop(prepared_updater); + marker.record(&process.process, &stage).unwrap(); + + cleanup( + &process.process, + &process.process.cargo_home().unwrap().join("bin"), + ) + .unwrap(); + + assert!(!updater.exists()); + assert!(!marker.path(&stage).exists()); + } + } + fn test_process(root: &Path) -> TestProcess { let mut vars = HashMap::new(); vars.env("HOME", root); diff --git a/src/cli/self_update/unix.rs b/src/cli/self_update/unix.rs index 483059e3c6..e2ab288f1a 100644 --- a/src/cli/self_update/unix.rs +++ b/src/cli/self_update/unix.rs @@ -5,7 +5,7 @@ use tracing::{error, warn}; use super::{ shell::{self, Posix, UnixShell}, - stage::{PreparedUpdater, SelfUpdateLock}, + stage::{self, PreparedUpdater, SelfUpdateLock}, }; use crate::{process::Process, utils}; @@ -136,10 +136,11 @@ pub(crate) fn self_replace(process: &Process) -> anyhow::Result let self_update_lock = SelfUpdateLock::lock(process)?; #[cfg(feature = "test")] process.checkpoint(super::CHECKPOINT_SELF_REPLACE_READY); - self_update_lock.install_bins( - &process.cargo_home()?.join("bin"), - super::force_hard_links(process), - )?; + let result = process.cargo_home().and_then(|cargo_home| { + self_update_lock.install_bins(&cargo_home.join("bin"), super::force_hard_links(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 55054aa3d0..c13166e64c 100644 --- a/src/cli/self_update/windows.rs +++ b/src/cli/self_update/windows.rs @@ -46,7 +46,7 @@ use windows_sys::Win32::{ use super::{ InstallOpts, report_error, - stage::{PreparedUpdater, SelfUpdateLock}, + stage::{self, PreparedUpdater, SelfUpdateLock}, }; use crate::{ cli::{common, errors::CliError, markdown::md}, @@ -672,10 +672,12 @@ pub(super) fn run_update( pub(crate) fn self_replace(process: &Process) -> anyhow::Result { wait_for_parent()?; - SelfUpdateLock::lock(process)?.install_bins( - &process.cargo_home()?.join("bin"), - super::force_hard_links(process), - )?; + let self_update_lock = SelfUpdateLock::lock(process)?; + let result = process.cargo_home().and_then(|cargo_home| { + self_update_lock.install_bins(&cargo_home.join("bin"), super::force_hard_links(process)) + }); + stage::mark_result(result.is_ok(), process); + result?; Ok(utils::ExitCode(0)) } 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 4ecacce1f9..358e3b0c1c 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,9 @@ 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, Marker, SELF_UPDATE_DIRECTORY, updater_path, + }, test::{ CROSS_ARCH1, CliTestContext, Scenario, SelfUpdateTestContext, calc_hash, output_release_file, this_host_tuple, @@ -517,9 +524,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 @@ -530,8 +536,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"]) @@ -540,11 +544,37 @@ async fn update_but_delete_existing_updater_first() { let rustup = cx.config.cargodir.join(format!("bin/rustup{EXE_SUFFIX}")); assert!(rustup.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_races_proxy_cleanup() { +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 @@ -553,17 +583,18 @@ async fn self_update_replacement_races_proxy_cleanup() { .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"]); - // The proxy's startup cleanup deletes `$CARGO_HOME/bin/rustup-init`, - // which is the parked replacer's own executable. cx.config.expect(["rustc", "--version"]).await.is_ok(); - // The replacer unlinks the installed rustup before copying itself over - // it, so it fails and leaves no rustup behind. let status = parked.resume(); - assert!(!status.success()); - assert!(!rustup.exists()); + 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] @@ -800,11 +831,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] @@ -819,17 +846,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] @@ -844,14 +868,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] @@ -1236,6 +1257,24 @@ 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_DIRECTORY); + retry(Fibonacci::from_millis(1).map(jitter).take(23), || { + if Marker::Complete.path(&stage).is_file() { + Ok(()) + } else if Marker::Failed.path(&stage).is_file() { + Err("self-update failed") + } else { + Err("self-update has not completed") + } + }) + .unwrap(); +} + +fn managed_updater(rustup_home: &Path) -> PathBuf { + updater_path(&rustup_home.join(SELF_UPDATE_DIRECTORY)) +} + const TEST_VERSION: &str = "1.1.1"; #[cfg(windows)] const USER_RUSTUP_VERSION: RegistryValueId = RegistryValueId { From d66095f745cb59ba6b0112077456a3acba1e2e41 Mon Sep 17 00:00:00 2001 From: Akrm Al-Hakimi Date: Thu, 17 Sep 2026 08:12:08 -0400 Subject: [PATCH 07/10] fix(self-update): remove abandoned updaters only once stale An updater whose replacer never ran, or crashed before recording an outcome, has no marker and was left behind forever. A legacy `$CARGO_HOME/bin/rustup-init` may still belong to an older rustup that is running it, so deleting it on sight is the very race being fixed. Both are now removed only after they have gone untouched for a day. --- src/cli/self_update/stage.rs | 84 ++++++++++++++++++++++++++++++++---- tests/suite/cli_self_upd.rs | 1 + 2 files changed, 76 insertions(+), 9 deletions(-) diff --git a/src/cli/self_update/stage.rs b/src/cli/self_update/stage.rs index b4266b04ac..d04eedfbca 100644 --- a/src/cli/self_update/stage.rs +++ b/src/cli/self_update/stage.rs @@ -4,6 +4,7 @@ use std::{ ops::Deref, path::{Path, PathBuf}, process::{Child, Command}, + time::{Duration, SystemTime}, }; use anyhow::Context; @@ -130,15 +131,22 @@ pub(super) fn mark_result(succeeded: bool, process: &Process) { } pub(super) fn cleanup(process: &Process, bin_path: &Path) -> anyhow::Result<()> { + cleanup_at(process, bin_path, SystemTime::now()) +} + +fn cleanup_at(process: &Process, bin_path: &Path, now: SystemTime) -> anyhow::Result<()> { if let Some(lock) = SelfUpdateLock::try_lock(process)? { let updater = updater_path(&lock.directory); - // The replacer records an outcome only once it has finished, so an - // unmarked updater may still be about to run and is left alone. + // 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 && utils::remove_file_best_effort("self-updater", &updater) { + 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", @@ -149,13 +157,23 @@ pub(super) fn cleanup(process: &Process, bin_path: &Path) -> anyhow::Result<()> } let updater = bin_path.join(format!("rustup-init{EXE_SUFFIX}")); - if updater.exists() { - utils::remove_file("legacy self-updater", &updater)?; + // 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)] pub enum Marker { @@ -201,6 +219,7 @@ fn stage_root(process: &Process) -> anyhow::Result { pub 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 { @@ -263,17 +282,19 @@ mod tests { fs::write(&updater, "").unwrap(); fs::write(Marker::Complete.path(&lock.directory), "").unwrap(); - cleanup( + cleanup_at( &process.process, &process.process.cargo_home().unwrap().join("bin"), + SystemTime::now(), ) .unwrap(); assert!(updater.exists()); drop(lock); - cleanup( + cleanup_at( &process.process, &process.process.cargo_home().unwrap().join("bin"), + SystemTime::now(), ) .unwrap(); assert!(!updater.exists()); @@ -307,9 +328,10 @@ mod tests { fs::write(&updater, "").unwrap(); drop(prepared_updater); - cleanup( + cleanup_at( &process.process, &process.process.cargo_home().unwrap().join("bin"), + SystemTime::now(), ) .unwrap(); @@ -332,9 +354,10 @@ mod tests { drop(prepared_updater); marker.record(&process.process, &stage).unwrap(); - cleanup( + cleanup_at( &process.process, &process.process.cargo_home().unwrap().join("bin"), + SystemTime::now(), ) .unwrap(); @@ -343,6 +366,49 @@ mod tests { } } + #[tokio::test] + async fn cleanup_removes_abandoned_updater() { + let root = test_dir().unwrap(); + let process = test_process(root.path()); + let prepared_updater = SelfUpdateLock::lock(&process.process) + .unwrap() + .prepare_updater() + .unwrap(); + let updater = prepared_updater.to_path_buf(); + fs::write(&updater, "").unwrap(); + drop(prepared_updater); + + cleanup_at( + &process.process, + &process.process.cargo_home().unwrap().join("bin"), + 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 bin_path = root.path().join("cargo/bin"); + let updater = bin_path.join(format!("rustup-init{EXE_SUFFIX}")); + fs::create_dir_all(&bin_path).unwrap(); + fs::write(&updater, "").unwrap(); + + cleanup_at(&process.process, &bin_path, SystemTime::now()).unwrap(); + assert!(updater.exists()); + + cleanup_at( + &process.process, + &bin_path, + 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); diff --git a/tests/suite/cli_self_upd.rs b/tests/suite/cli_self_upd.rs index 358e3b0c1c..9ece91b62b 100644 --- a/tests/suite/cli_self_upd.rs +++ b/tests/suite/cli_self_upd.rs @@ -544,6 +544,7 @@ async fn update_does_not_reuse_legacy_updater_path() { 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()); } From 113c8d884f19669d0c5099a9fe7c1e190e42fccd Mon Sep 17 00:00:00 2001 From: Akrm Al-Hakimi Date: Thu, 17 Sep 2026 08:12:09 -0400 Subject: [PATCH 08/10] 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. --- src/cli/self_update/stage.rs | 104 ++++++++++++++++++++++++++++++++--- 1 file changed, 96 insertions(+), 8 deletions(-) diff --git a/src/cli/self_update/stage.rs b/src/cli/self_update/stage.rs index d04eedfbca..876d550bdc 100644 --- a/src/cli/self_update/stage.rs +++ b/src/cli/self_update/stage.rs @@ -69,17 +69,40 @@ impl SelfUpdateLock { bin_path: &Path, force_hard_links: bool, ) -> anyhow::Result<()> { - let this_exe_path = utils::current_exe()?; - let rustup_path = bin_path.join(format!("rustup{EXE_SUFFIX}")); + self.install_bins_from(&utils::current_exe()?, bin_path, force_hard_links) + } + fn install_bins_from( + &self, + this_exe_path: &Path, + bin_path: &Path, + force_hard_links: bool, + ) -> anyhow::Result<()> { + 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)?; + + // 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")?; } - utils::copy_file_symlink_to_source(&this_exe_path, &rustup_path)?; - utils::make_executable(&rustup_path)?; + + replace_rustup_binary(&pending, &rustup_path)?; install_proxies_with_opts(bin_path, force_hard_links) } } @@ -134,6 +157,25 @@ pub(super) fn cleanup(process: &Process, bin_path: &Path) -> anyhow::Result<()> cleanup_at(process, bin_path, 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, bin_path: &Path, now: SystemTime) -> anyhow::Result<()> { if let Some(lock) = SelfUpdateLock::try_lock(process)? { let updater = updater_path(&lock.directory); @@ -219,6 +261,7 @@ fn stage_root(process: &Process) -> anyhow::Result { pub 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)] @@ -273,6 +316,51 @@ 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::lock(&process.process) + .unwrap() + .install_bins_from( + &root.path().join("missing-updater"), + rustup.parent().unwrap(), + false, + ) + .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(); From 83e14dd270897003ff53f7f14ad7744f31182a50 Mon Sep 17 00:00:00 2001 From: Akrm Al-Hakimi Date: Thu, 17 Sep 2026 08:12:09 -0400 Subject: [PATCH 09/10] fix(self-update): remove abandoned pending binaries once stale A crash between staging and publishing leaves a `.rustup-pending-*` file in `$CARGO_HOME/bin`. Startup cleanup now removes such files once they have gone untouched for a day, the same threshold used for abandoned updaters. --- src/cli/self_update/stage.rs | 45 ++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/src/cli/self_update/stage.rs b/src/cli/self_update/stage.rs index 876d550bdc..401a9dff72 100644 --- a/src/cli/self_update/stage.rs +++ b/src/cli/self_update/stage.rs @@ -1,6 +1,7 @@ use std::{ env::consts::EXE_SUFFIX, fs::{self, File, OpenOptions}, + io, ops::Deref, path::{Path, PathBuf}, process::{Child, Command}, @@ -198,6 +199,29 @@ fn cleanup_at(process: &Process, bin_path: &Path, now: SystemTime) -> anyhow::Re } } + match fs::read_dir(bin_path) { + 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_path.display() + ); + } + } + let updater = bin_path.join(format!("rustup-init{EXE_SUFFIX}")); // Legacy updaters have no result marker, and an older rustup process may // still own the shared path. @@ -497,6 +521,27 @@ 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 bin_path = root.path().join("cargo/bin"); + let pending = bin_path.join(format!("{PENDING_BINARY_PREFIX}orphan")); + fs::create_dir_all(&bin_path).unwrap(); + fs::write(&pending, "").unwrap(); + + cleanup_at(&process.process, &bin_path, SystemTime::now()).unwrap(); + assert!(pending.exists()); + + cleanup_at( + &process.process, + &bin_path, + 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 779a067c766f862083c8f9541a4c95175c937499 Mon Sep 17 00:00:00 2001 From: Akrm Al-Hakimi Date: Tue, 15 Sep 2026 16:19:17 -0400 Subject: [PATCH 10/10] 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 | 12 +++--------- tests/suite/cli_self_upd.rs | 1 + 2 files changed, 4 insertions(+), 9 deletions(-) diff --git a/src/cli/self_update/windows.rs b/src/cli/self_update/windows.rs index c13166e64c..c4ca3d538a 100644 --- a/src/cli/self_update/windows.rs +++ b/src/cli/self_update/windows.rs @@ -656,17 +656,10 @@ pub(crate) fn remove_uninstall_registry_entry(process: &Process) -> anyhow::Resu pub(super) fn run_update( prepared_updater: PreparedUpdater, - process: &Process, + _process: &Process, ) -> anyhow::Result { - let updater_path = prepared_updater.to_path_buf(); prepared_updater.spawn_replacer()?; - let Some(version) = super::get_and_parse_new_rustup_version(&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)?; - Ok(utils::ExitCode(0)) } @@ -674,7 +667,8 @@ pub(crate) fn self_replace(process: &Process) -> anyhow::Result wait_for_parent()?; let self_update_lock = SelfUpdateLock::lock(process)?; let result = process.cargo_home().and_then(|cargo_home| { - self_update_lock.install_bins(&cargo_home.join("bin"), super::force_hard_links(process)) + self_update_lock.install_bins(&cargo_home.join("bin"), super::force_hard_links(process))?; + update_uninstall_registry_display_version(env!("CARGO_PKG_VERSION"), process) }); stage::mark_result(result.is_ok(), process); result?; diff --git a/tests/suite/cli_self_upd.rs b/tests/suite/cli_self_upd.rs index 9ece91b62b..bba052364c 100644 --- a/tests/suite/cli_self_upd.rs +++ b/tests/suite/cli_self_upd.rs @@ -499,6 +499,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)