install: fix ENOENT race on shared cache when RENAME_EXCHANGE is unsupported (NFS)

src/install/extract_tarball.rs

@@ -678,42 +678,74 @@ impl ExtractTarball {
             }
             #[cfg(not(windows))]
             {
-                // Attempt to gracefully handle duplicate concurrent `bun install` calls
-                //
-                // By:
-                // 1. Rename from temporary directory to cache directory and fail if it already exists
-                // 2a. If the rename fails, swap the cache directory with the temporary directory version
-                // 2b. Delete the temporary directory version ONLY if we're not using a provided temporary directory
-                // 3. If rename still fails, fallback to racily deleting the cache directory version and then renaming the temporary directory version again.
-                //
+                // An existing destination is a complete entry from a concurrent
+                // `bun install` (entries only appear via atomic rename): keep it,
+                // since replacing it unlinks files another process may be copying
+                // out of the cache (#36227). Only an incomplete entry (no
+                // package.json, e.g. a crashed copy) is replaced.
 
                 if create_subdir {
                     if let Some(folder) = bun_paths::Dirname::dirname(folder_name) {
                         let _ = bun_sys::make_path::make_path(cache_dir, folder);
                     }
                 }
 
-                if let Err(err) = sys::renameat_concurrently_a(
+                let mut folder_name_z_buf = PathBuffer::uninit();
+                folder_name_z_buf[0..folder_name.len()].copy_from_slice(folder_name);
+                folder_name_z_buf[folder_name.len()] = 0;
+                let folder_name_z = ZStr::from_buf(&folder_name_z_buf, folder_name.len());
+
+                let renamed = sys::renameat2(
                     tmpdir.fd(),
-                    tmpname.as_bytes(),
+                    tmpname,
                     cache_dir.fd(),
-                    folder_name,
-                    sys::RenameatConcurrentlyOptions {
-                        move_fallback: true,
+                    folder_name_z,
+                    sys::Renameat2Flags {
+                        exclude: true,
+                        ..Default::default()
                     },
-                ) {
-                    log.add_error_fmt(
-                        None,
-                        bun_ast::Loc::EMPTY,
-                        format_args!(
-                            "moving \"{}\" to cache dir failed: {}\n  From: {}\n    To: {}",
-                            bun_fmt::s(name),
-                            err,
-                            bun_fmt::s(tmpname.as_bytes()),
-                            bun_fmt::s(folder_name),
-                        ),
-                    );
-                    return Err(crate::Error::InstallFailed);
+                )
+                .is_ok();
+
+                let keep_existing = !renamed
+                    && match self.resolution.tag {
+                        ResolutionTag::Npm => {
+                            let mut pkg_json_buf = PathBuffer::uninit();
+                            let pkg_json = path::resolve_path::join_z_buf::<path::platform::Auto>(
+                                &mut pkg_json_buf.0,
+                                &[folder_name, b"package.json"],
+                            );
+                            sys::exists_at(cache_dir.fd(), pkg_json)
+                        }
+                        _ => sys::directory_exists_at(cache_dir.fd(), folder_name_z)
+                            .unwrap_or(false),
+                    };
+
+                if keep_existing {
+                    let _ = tmpdir.delete_tree(tmpname.as_bytes());
+                } else if !renamed {
+                    if let Err(err) = sys::renameat_concurrently_a(
+                        tmpdir.fd(),
+                        tmpname.as_bytes(),
+                        cache_dir.fd(),
+                        folder_name,
+                        sys::RenameatConcurrentlyOptions {
+                            move_fallback: true,
+                        },
+                    ) {
+                        log.add_error_fmt(
+                            None,
+                            bun_ast::Loc::EMPTY,
+                            format_args!(
+                                "moving \"{}\" to cache dir failed: {}\n  From: {}\n    To: {}",
+                                bun_fmt::s(name),
+                                err,
+                                bun_fmt::s(tmpname.as_bytes()),
+                                bun_fmt::s(folder_name),
+                            ),
+                        );
+                        return Err(crate::Error::InstallFailed);
+                    }
                 }
             }
 

src/install/patch_install.rs

@@ -533,6 +533,19 @@ impl PatchTask {
             }
         }
 
+        // `.bun-tag-<hash>`: written into the tree before the rename publishes
+        // it, so its presence marks a complete entry.
+        let mut buntagbuf: BuntagHashBuf = [0; MAX_BUNTAG_HASH_BUF_LEN];
+        let buntag_len = {
+            use std::io::Write as _;
+            buntagbuf[..bun_hash_tag.len()].copy_from_slice(bun_hash_tag);
+            let mut cursor = &mut buntagbuf[bun_hash_tag.len()..];
+            let before = cursor.len();
+            write!(&mut cursor, "{:x}", patch.patch_hash).expect("unreachable");
+            bun_hash_tag.len() + (before - cursor.len())
+        };
+        buntagbuf[buntag_len] = 0;
+
         {
             let patch_pkg_dir = match sys::openat(
                 system_tmpdir,
@@ -564,18 +577,7 @@ impl PatchTask {
             }
 
             // 5. Add bun tag
-            let bun_tag_prefix = bun_hash_tag;
-            let mut buntagbuf: BuntagHashBuf = [0; MAX_BUNTAG_HASH_BUF_LEN];
-            buntagbuf[..bun_tag_prefix.len()].copy_from_slice(bun_tag_prefix);
-            let hashlen = {
-                use std::io::Write as _;
-                let mut cursor = &mut buntagbuf[bun_tag_prefix.len()..];
-                let before = cursor.len();
-                write!(&mut cursor, "{:x}", patch.patch_hash).expect("unreachable");
-                before - cursor.len()
-            };
-            buntagbuf[bun_tag_prefix.len() + hashlen] = 0;
-            let buntag_zstr = ZStr::from_buf(&buntagbuf, bun_tag_prefix.len() + hashlen);
+            let buntag_zstr = ZStr::from_buf(&buntagbuf, buntag_len);
             if let Err(e) = sys::File::write_file(patch_pkg_dir, buntag_zstr, b"") {
                 log.add_error_fmt_opts(
                     format_args!(
@@ -599,24 +601,51 @@ impl PatchTask {
         );
 
         let cache_dir_subpath_z: &ZStr = patch.cache_dir_subpath.as_zstr();
-        if let Err(e) = sys::renameat_concurrently(
+        let renamed = sys::renameat2(
             system_tmpdir,
             path_in_tmpdir,
             patch.cache_dir,
             cache_dir_subpath_z,
-            sys::RenameOptions {
-                move_fallback: true,
+            sys::Renameat2Flags {
+                exclude: true,
                 ..Default::default()
             },
-        ) {
-            log.add_error_fmt_opts(
-                format_args!(
-                    "renaming changes to cache dir: {}",
-                    e.with_path(cache_dir_subpath_z.as_bytes())
-                ),
-                Default::default(),
+        )
+        .is_ok();
+        let keep_existing = !renamed && {
+            let mut tag_path_buf = PathBuffer::uninit();
+            let tag_path = path::resolve_path::join_z_buf::<path::platform::Auto>(
+                &mut tag_path_buf.0,
+                &[cache_dir_subpath_z.as_bytes(), &buntagbuf[..buntag_len]],
             );
-            return Ok(());
+            sys::exists_at(patch.cache_dir, tag_path)
+        };
+        if keep_existing {
+            // A concurrent `bun install` created the same complete entry (the
+            // tag is written before the rename publishes it, and the name embeds
+            // the patch hash, so contents are equivalent): keep it, since
+            // replacing it unlinks files another process may be copying out.
+            let _ = sys::Dir::borrow(&system_tmpdir).delete_tree(path_in_tmpdir.as_bytes());
+        } else if !renamed {
+            if let Err(e) = sys::renameat_concurrently(
+                system_tmpdir,
+                path_in_tmpdir,
+                patch.cache_dir,
+                cache_dir_subpath_z,
+                sys::RenameOptions {
+                    move_fallback: true,
+                    ..Default::default()
+                },
+            ) {
+                log.add_error_fmt_opts(
+                    format_args!(
+                        "renaming changes to cache dir: {}",
+                        e.with_path(cache_dir_subpath_z.as_bytes())
+                    ),
+                    Default::default(),
+                );
+                return Ok(());
+            }
         }
         Ok(())
     }

src/sys/lib.rs

@@ -9074,7 +9074,7 @@ pub fn exists(path: &[u8]) -> bool {
 }
 /// `moveFileZ`. Routes through
 /// [`renameat_concurrently_without_fallback`] (renameat2 NOREPLACE → EXCHANGE →
-/// delete-tree + rename); on EISDIR removes the dest dir and
+/// rename-aside + rename); on EISDIR removes the dest dir and
 /// retries; on EXDEV falls back to the slow open+copy path. Only opens the
 /// source inside the EXDEV branch.
 pub fn move_file_z(from_dir: Fd, filename: &ZStr, to_dir: Fd, destination: &ZStr) -> Maybe<()> {
@@ -9172,8 +9172,9 @@ pub fn move_file_z_slow_maybe(
 }
 
 /// `renameatConcurrently`. Tries an atomic NOREPLACE rename,
-/// then EXCHANGE, then a racy delete-tree + rename. With `move_fallback` set,
-/// an EXDEV result falls through to a slow open/copy.
+/// then EXCHANGE, then moving the destination aside before renaming into
+/// place. With `move_fallback` set, an EXDEV result falls through to a slow
+/// open/copy.
 pub fn renameat_concurrently(
     from_dir_fd: Fd,
     from: &ZStr,
@@ -9253,21 +9254,93 @@ pub fn renameat_concurrently_without_fallback(
             }
         }
 
-        //  sad path: let's try to delete the folder and then rename it
-        if to_dir_fd.is_valid() {
-            let _ = Dir::borrow(&to_dir_fd).delete_tree(to.as_bytes());
-        } else {
-            let _ = delete_tree_absolute(to.as_bytes());
-        }
-        match renameat(from_dir_fd, from, to_dir_fd, to) {
-            Err(err) => return Err(err),
-            Ok(()) => {}
+        // Sad path (no atomic exchange, e.g. NFS/FUSE or Windows): move the
+        // destination aside, rename the source into place, then delete the old
+        // tree. An in-place delete_tree(dest) would ENOENT concurrent readers.
+        let delete_tree_at = |path: &[u8]| {
+            if to_dir_fd.is_valid() {
+                let _ = Dir::borrow(&to_dir_fd).delete_tree(path);
+            } else {
+                let _ = delete_tree_absolute(path);
+            }
+        };
+        let mut aside_buf = bun_paths::path_buffer_pool::get();
+        let mut attempts_left: u32 = 8;
+        loop {
+            let Some(aside) = rename_aside_name(to, &mut aside_buf) else {
+                // No room to append a suffix; fall back to the in-place delete.
+                delete_tree_at(to.as_bytes());
+                renameat(from_dir_fd, from, to_dir_fd, to)?;
+                break;
+            };
+            let moved_aside = match renameat(to_dir_fd, to, to_dir_fd, aside) {
+                Ok(()) => true,
+                // The destination vanished; go straight to the rename.
+                Err(err) if err.get_errno() == E::ENOENT => false,
+                Err(err) => return Err(err),
+            };
+            match renameat(from_dir_fd, from, to_dir_fd, to) {
+                Ok(()) => {
+                    if moved_aside {
+                        delete_tree_at(aside.as_bytes());
+                    }
+                    break;
+                }
+                Err(err)
+                    if matches!(err.get_errno(), E::EEXIST | E::ENOTEMPTY) && attempts_left > 0 =>
+                {
+                    // A concurrent process recreated the destination between
+                    // the two renames; retry against the new one.
+                    if moved_aside {
+                        delete_tree_at(aside.as_bytes());
+                    }
+                    attempts_left -= 1;
+                }
+                Err(err) => {
+                    // Best-effort restore of the displaced destination; if it
+                    // was recreated meanwhile, drop the displaced tree instead
+                    // of leaking it.
+                    if moved_aside && renameat(to_dir_fd, aside, to_dir_fd, to).is_err() {
+                        delete_tree_at(aside.as_bytes());
+                    }
+                    return Err(err);
+                }
+            }
         }
     }
 
     Ok(())
 }
 
+/// Builds `{to}.{16 random hex chars}.tmp\0` into `buf` for the rename-aside
+/// fallback in [`renameat_concurrently_without_fallback`]. Returns `None`
+/// when the result would not fit in the buffer or would push the filename
+/// component past NAME_MAX.
+fn rename_aside_name<'a>(to: &ZStr, buf: &'a mut bun_paths::PathBuffer) -> Option<&'a ZStr> {
+    const HEX: &[u8; 16] = b"0123456789abcdef";
+    const SUFFIX_LEN: usize = 21; // "." + 16 hex + ".tmp"
+    const NAME_MAX: usize = 255;
+    let to_bytes = to.as_bytes();
+    if to_bytes.len() + SUFFIX_LEN + 1 > buf.0.len()
+        || bun_paths::resolve_path::basename(to_bytes).len() + SUFFIX_LEN > NAME_MAX
+    {
+        return None;
+    }
+    buf.0[..to_bytes.len()].copy_from_slice(to_bytes);
+    let mut pos = to_bytes.len();
+    buf.0[pos] = b'.';
+    pos += 1;
+    let r = bun_core::fast_random();
+    for i in 0..16 {
+        buf.0[pos + i] = HEX[((r >> ((15 - i) * 4)) & 0xf) as usize];
+    }
+    pos += 16;
+    buf.0[pos..pos + 4].copy_from_slice(b".tmp");
+    pos += 4;
+    buf.0[pos] = 0;
+    Some(ZStr::from_buf(&buf.0[..], pos))
+}
+
 /// `eventfd(initval, flags)` — kernel notification fd. Linux native (Android
 /// included since API 8); FreeBSD 13+ gained a Linux-compatible `eventfd(2)`
 /// via the `libc` shim.
@@ -9563,10 +9636,10 @@ bun_core::link_impl_OutputSink! {
 mod owned_handle_tests {
     use super::*;
 
-    /// `renameat_concurrently_without_fallback` falls back to `delete_tree` +
-    /// retry when the destination exists. The `delete_tree` is run via a `Dir`
-    /// borrowed from the caller's `to_dir_fd`; if it took ownership instead,
-    /// `to_dir_fd` would be closed out from under the caller.
+    /// `renameat_concurrently_without_fallback` falls back to rename-aside +
+    /// `delete_tree` when the destination exists. The `delete_tree` is run via
+    /// a `Dir` borrowed from the caller's `to_dir_fd`; if it took ownership
+    /// instead, `to_dir_fd` would be closed out from under the caller.
     #[test]
     fn renameat_concurrently_does_not_close_caller_fd() {
         let _g = crate::file::tests::FD_TEST_LOCK.lock();