add retry support to recursive_remove

Author: Shourya742

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<P: AsRef<Path>>(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<P: AsRef<Path>>(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<()>

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]