diff --git a/src/build_helper/src/fs/mod.rs b/src/build_helper/src/fs/mod.rs index 02029846fd147..123df76e6a2e9 100644 --- a/src/build_helper/src/fs/mod.rs +++ b/src/build_helper/src/fs/mod.rs @@ -22,21 +22,27 @@ where /// A wrapper around [`std::fs::remove_dir_all`] that can also be used on *non-directory entries*, /// including files and symbolic links. /// -/// - This will produce an error if the target path is not found. +/// - This will not produce an error if the target path is not found. /// - Like [`std::fs::remove_dir_all`], this helper does not traverse symbolic links, will remove /// symbolic link itself. /// - This helper is **not** robust against races on the underlying filesystem, behavior is /// unspecified if this helper is called concurrently. /// - This helper is not robust against TOCTOU problems. /// -/// FIXME: this implementation is insufficiently robust to replace bootstrap's clean `rm_rf` -/// implementation: -/// -/// - This implementation currently does not perform retries. +/// FIXME: Audit whether this implementation is robust enough to replace bootstrap's clean `rm_rf`. #[track_caller] pub fn recursive_remove>(path: P) -> io::Result<()> { let path = path.as_ref(); - let metadata = fs::symlink_metadata(path)?; + + // If the path doesn't exist, we treat it as a successful no-op. + // From the caller's perspective, the goal is simply "ensure this file/dir is gone" — + // if it's already not there, that's a success, not an error. + let metadata = match fs::symlink_metadata(path) { + Ok(m) => m, + Err(e) if e.kind() == io::ErrorKind::NotFound => return Ok(()), + Err(e) => return Err(e), + }; + #[cfg(windows)] let is_dir_like = |meta: &fs::Metadata| { use std::os::windows::fs::FileTypeExt; @@ -45,11 +51,35 @@ pub fn recursive_remove>(path: P) -> io::Result<()> { #[cfg(not(windows))] let is_dir_like = fs::Metadata::is_dir; - if is_dir_like(&metadata) { - fs::remove_dir_all(path) - } else { - try_remove_op_set_perms(fs::remove_file, path, metadata) + const MAX_RETRIES: usize = 5; + const RETRY_DELAY_MS: u64 = 100; + + let try_remove = || { + if is_dir_like(&metadata) { + fs::remove_dir_all(path) + } else { + try_remove_op_set_perms(fs::remove_file, path, metadata.clone()) + } + }; + + // Retry deletion a few times to handle transient filesystem errors. + // This is unusual for local file operations, but it's a mitigation + // against unlikely events where malware scanners may be holding a + // file beyond our control, to give the malware scanners some opportunity + // to release their hold. + for attempt in 0..MAX_RETRIES { + match try_remove() { + Ok(()) => return Ok(()), + Err(e) if e.kind() == io::ErrorKind::NotFound => return Ok(()), + Err(_) if attempt < MAX_RETRIES - 1 => { + std::thread::sleep(std::time::Duration::from_millis(RETRY_DELAY_MS)); + continue; + } + Err(e) => return Err(e), + } } + + Ok(()) } fn try_remove_op_set_perms<'p, Op>(mut op: Op, path: &'p Path, metadata: Metadata) -> io::Result<()> @@ -67,3 +97,9 @@ where Err(e) => Err(e), } } + +pub fn remove_and_create_dir_all>(path: P) -> io::Result<()> { + let path = path.as_ref(); + recursive_remove(path)?; + fs::create_dir_all(path) +} diff --git a/src/build_helper/src/fs/tests.rs b/src/build_helper/src/fs/tests.rs index 1e694393127cb..7ce1d8928d1cb 100644 --- a/src/build_helper/src/fs/tests.rs +++ b/src/build_helper/src/fs/tests.rs @@ -14,7 +14,7 @@ mod recursive_remove_tests { let tmpdir = env::temp_dir(); let path = tmpdir.join("__INTERNAL_BOOTSTRAP_nonexistent_path"); assert!(fs::symlink_metadata(&path).is_err_and(|e| e.kind() == io::ErrorKind::NotFound)); - assert!(recursive_remove(&path).is_err_and(|e| e.kind() == io::ErrorKind::NotFound)); + assert!(recursive_remove(&path).is_ok()); } #[test] diff --git a/src/tools/compiletest/src/runtest.rs b/src/tools/compiletest/src/runtest.rs index 24fc2ddb74104..e1a5fe523fe1e 100644 --- a/src/tools/compiletest/src/runtest.rs +++ b/src/tools/compiletest/src/runtest.rs @@ -9,6 +9,7 @@ use std::process::{Child, Command, ExitStatus, Output, Stdio}; use std::sync::Arc; use std::{env, iter, str}; +use build_helper::fs::remove_and_create_dir_all; use camino::{Utf8Path, Utf8PathBuf}; use colored::Colorize; use regex::{Captures, Regex}; @@ -207,12 +208,6 @@ pub fn compute_stamp_hash(config: &Config) -> String { format!("{:x}", hash.finish()) } -fn remove_and_create_dir_all(path: &Utf8Path) { - let path = path.as_std_path(); - let _ = fs::remove_dir_all(path); - fs::create_dir_all(path).unwrap(); -} - #[derive(Copy, Clone, Debug)] struct TestCx<'test> { config: &'test Config, @@ -523,7 +518,9 @@ impl<'test> TestCx<'test> { let mut rustc = Command::new(&self.config.rustc_path); let out_dir = self.output_base_name().with_extension("pretty-out"); - remove_and_create_dir_all(&out_dir); + remove_and_create_dir_all(&out_dir).unwrap_or_else(|e| { + panic!("failed to remove and recreate output directory `{out_dir}`: {e}") + }); let target = if self.props.force_host { &*self.config.host } else { &*self.config.target }; @@ -1098,13 +1095,19 @@ impl<'test> TestCx<'test> { let aux_dir = self.aux_output_dir_name(); if !self.props.aux.builds.is_empty() { - remove_and_create_dir_all(&aux_dir); + remove_and_create_dir_all(&aux_dir).unwrap_or_else(|e| { + panic!("failed to remove and recreate output directory `{aux_dir}`: {e}") + }); } if !self.props.aux.bins.is_empty() { let aux_bin_dir = self.aux_bin_output_dir_name(); - remove_and_create_dir_all(&aux_dir); - remove_and_create_dir_all(&aux_bin_dir); + remove_and_create_dir_all(&aux_dir).unwrap_or_else(|e| { + panic!("failed to remove and recreate output directory `{aux_dir}`: {e}") + }); + remove_and_create_dir_all(&aux_bin_dir).unwrap_or_else(|e| { + panic!("failed to remove and recreate output directory `{aux_bin_dir}`: {e}") + }); } aux_dir @@ -1509,7 +1512,9 @@ impl<'test> TestCx<'test> { let set_mir_dump_dir = |rustc: &mut Command| { let mir_dump_dir = self.get_mir_dump_dir(); - remove_and_create_dir_all(&mir_dump_dir); + remove_and_create_dir_all(&mir_dump_dir).unwrap_or_else(|e| { + panic!("failed to remove and recreate output directory `{mir_dump_dir}`: {e}") + }); let mut dir_opt = "-Zdump-mir-dir=".to_string(); dir_opt.push_str(mir_dump_dir.as_str()); debug!("dir_opt: {:?}", dir_opt); @@ -1969,7 +1974,9 @@ impl<'test> TestCx<'test> { let suffix = self.safe_revision().map_or("nightly".into(), |path| path.to_owned() + "-nightly"); let compare_dir = output_base_dir(self.config, self.testpaths, Some(&suffix)); - remove_and_create_dir_all(&compare_dir); + remove_and_create_dir_all(&compare_dir).unwrap_or_else(|e| { + panic!("failed to remove and recreate output directory `{compare_dir}`: {e}") + }); // We need to create a new struct for the lifetimes on `config` to work. let new_rustdoc = TestCx { diff --git a/src/tools/compiletest/src/runtest/rustdoc.rs b/src/tools/compiletest/src/runtest/rustdoc.rs index 2583ae96a6788..637ea833357a2 100644 --- a/src/tools/compiletest/src/runtest/rustdoc.rs +++ b/src/tools/compiletest/src/runtest/rustdoc.rs @@ -7,7 +7,9 @@ impl TestCx<'_> { assert!(self.revision.is_none(), "revisions not relevant here"); let out_dir = self.output_base_dir(); - remove_and_create_dir_all(&out_dir); + remove_and_create_dir_all(&out_dir).unwrap_or_else(|e| { + panic!("failed to remove and recreate output directory `{out_dir}`: {e}") + }); let proc_res = self.document(&out_dir, &self.testpaths); if !proc_res.status.success() { diff --git a/src/tools/compiletest/src/runtest/rustdoc_json.rs b/src/tools/compiletest/src/runtest/rustdoc_json.rs index bf7eb2e109a46..9f88faca89268 100644 --- a/src/tools/compiletest/src/runtest/rustdoc_json.rs +++ b/src/tools/compiletest/src/runtest/rustdoc_json.rs @@ -9,7 +9,9 @@ impl TestCx<'_> { assert!(self.revision.is_none(), "revisions not relevant here"); let out_dir = self.output_base_dir(); - remove_and_create_dir_all(&out_dir); + remove_and_create_dir_all(&out_dir).unwrap_or_else(|e| { + panic!("failed to remove and recreate output directory `{out_dir}`: {e}") + }); let proc_res = self.document(&out_dir, &self.testpaths); if !proc_res.status.success() {