install: stop leaking the displaced cache folder when a tarball is re-extracted

src/install/PackageManager/patchPackage.rs

@@ -344,6 +344,7 @@ pub fn do_patch_commit(
                 random_tempdir.as_bytes(),
                 sys::RenameOptions {
                     move_fallback: true,
+                    ..Default::default()
                 },
             )
             .is_err()
@@ -399,6 +400,7 @@ pub fn do_patch_commit(
                 patch_tag_tmpname.as_bytes(),
                 sys::RenameOptions {
                     move_fallback: true,
+                    ..Default::default()
                 },
             ) {
                 bun_core::warn!(
@@ -431,7 +433,7 @@ pub fn do_patch_commit(
                         random_tempdir.as_bytes(),
                         new_folder_handle.fd,
                         b"node_modules",
-                        sys::RenameOptions { move_fallback: true },
+                        sys::RenameOptions { move_fallback: true, ..Default::default() },
                     ) {
                         bun_core::warn!("failed renaming nested node_modules folder, this may cause issues: {}", e);
                     }
@@ -443,7 +445,7 @@ pub fn do_patch_commit(
                         patch_tag_tmpname.as_bytes(),
                         new_folder_handle.fd,
                         patch_tag,
-                        sys::RenameOptions { move_fallback: true },
+                        sys::RenameOptions { move_fallback: true, ..Default::default() },
                     ) {
                         bun_core::warn!("failed renaming the bun patch tag, this may cause issues: {}", e);
                     }
@@ -588,6 +590,7 @@ pub fn do_patch_commit(
         path_in_patches_dir,
         sys::RenameOptions {
             move_fallback: true,
+            ..Default::default()
         },
     ) {
         Output::err(e, "failed renaming patch file to patches dir", ());

src/install/TarballStream.rs

@@ -133,6 +133,8 @@ pub struct TarballStream {
     /// Owned copy of the temp-directory name.
     // `ZBox` is the owned NUL-terminated counterpart of `&ZStr`.
     tmpname: ZBox,
+    /// Captured in `init`, before the first byte is extracted.
+    cache_publish: CachePublish,
 
     /// Incremental SHA over the *compressed* bytes, matching
     /// `Integrity.verify` / `Integrity.forBytes` in the buffered path.
@@ -230,6 +232,7 @@ impl TarballStream {
             },
             compute_if_missing,
         );
+        let cache_publish = tarball.cache_publish();
 
         // bun.TrivialNew(@This()) → heap::alloc(Box::new(...)). Pointer is
         // recovered via `container_of` from the thread-pool callback and
@@ -254,6 +257,7 @@ impl TarballStream {
             entry_final_offset: 0,
             dest: None,
             tmpname: ZBox::from_bytes(b""),
+            cache_publish,
             hasher,
             resolved_github_dirname: b"",
             want_first_dirname,
@@ -1159,6 +1163,7 @@ impl TarballStream {
                 name,
                 basename,
                 self.resolved_github_dirname,
+                self.cache_publish,
             ) {
                 Ok(r) => r,
                 Err(err) => {
@@ -1452,5 +1457,6 @@ fn tokenize_rest_after_first(s: &[OSPathChar]) -> &[OSPathChar] {
 
 // Resolved Phase-B paths: Resolution::Tag is the real npm/git/tarball
 // discriminant; Data/Status live on PackageManagerTask.
+use crate::extract_tarball::CachePublish;
 use crate::package_manager_task::{Data as TaskData, Status as TaskStatus};
 use crate::resolution::Tag as ResolutionTag;

src/install/extract_tarball.rs

@@ -181,7 +181,42 @@ pub(crate) fn uses_streaming_extraction() -> bool {
         .unwrap_or(false)
 }
 
+/// How `move_to_cache_directory` treats a cache folder that already has the
+/// name being published. Decided before extracting: afterwards a pre-existing
+/// folder and one a concurrent install published meanwhile look the same.
+#[derive(Clone, Copy, PartialEq, Eq)]
+pub(crate) enum CachePublish {
+    /// npm and GitHub: swap the fresh copy in. The swapped-out folder is left
+    /// in the temp dir, as a concurrent install may still be reading it.
+    Replace,
+    /// Tarball re-extracted over its own folder: swap the fresh copy in and
+    /// delete the swapped-out folder.
+    Supersede,
+    /// Tarball whose folder did not exist yet: if one exists by now, a
+    /// concurrent install extracted the same tarball, so keep it and drop ours.
+    KeepExisting,
+}
+
 impl ExtractTarball {
+    pub(crate) fn cache_publish(&self) -> CachePublish {
+        if !self.resolution.tag.is_tarball() {
+            return CachePublish::Replace;
+        }
+        let name_taken = TL_BUFS.with_borrow_mut(|bufs| {
+            let folder_name = directories::cached_tarball_folder_name_print(
+                &mut bufs.folder_name_buf,
+                self.url.slice(),
+                None,
+            );
+            sys::exists_at_type(self.cache_dir, folder_name).is_ok()
+        });
+        if name_taken {
+            CachePublish::Supersede
+        } else {
+            CachePublish::KeepExisting
+        }
+    }
+
     /// Derive the display name and a filesystem-safe basename for this
     /// package. Shared by the buffered `extract()` path below and the
     /// streaming extractor in `TarballStream.rs` so both pick identical
@@ -256,6 +291,7 @@ impl ExtractTarball {
         let mut resolved: &'static [u8] = b"";
         let tmpname =
             FileSystem::tmpname(tmpname_suffix, &mut tmpname_buf.0, bun_core::fast_random())?;
+        let publish = self.cache_publish();
         {
             let extract_destination = match bun_sys::make_path::make_open_path(
                 tmpdir,
@@ -439,7 +475,7 @@ impl ExtractTarball {
             }
         }
 
-        self.move_to_cache_directory(log, tmpname, name, basename, resolved)
+        self.move_to_cache_directory(log, tmpname, name, basename, resolved, publish)
     }
 
     /// Rename the freshly-extracted temp directory into the cache, read
@@ -452,6 +488,7 @@ impl ExtractTarball {
         name: &[u8],
         basename: &[u8],
         resolved: &[u8],
+        publish: CachePublish,
     ) -> Result<ExtractData, Error> {
         let package_manager = self.package_manager.get();
 
@@ -521,7 +558,7 @@ impl ExtractTarball {
 
             // Now that we've extracted the archive, we rename.
             #[cfg(windows)]
-            {
+            let moved: Result<(), Error> = 'moved: {
                 // Windows EBUSY/SHARING_VIOLATION on `NtSetInformationFile` is
                 // transient when a concurrent process (another `bun install`
                 // sharing the cache, AV, the Search Indexer) is closing its
@@ -537,6 +574,10 @@ impl ExtractTarball {
                 }
 
                 let path_to_use = path2;
+                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());
 
                 loop {
                     let dir_to_move = match sys::open_dir_at_windows_a(
@@ -562,7 +603,7 @@ impl ExtractTarball {
                                     bun_fmt::s(folder_name),
                                 ),
                             );
-                            return Err(crate::Error::InstallFailed);
+                            break 'moved Err(crate::Error::InstallFailed);
                         }
                     };
 
@@ -582,6 +623,16 @@ impl ExtractTarball {
                                         // before we attempt to delete the destination, let's close the source dir.
                                         let _ = sys::close(dir_to_move);
 
+                                        if publish == CachePublish::KeepExisting
+                                            && sys::directory_exists_at(
+                                                cache_dir.fd(),
+                                                folder_name_z,
+                                            )
+                                            .unwrap_or(false)
+                                        {
+                                            break;
+                                        }
+
                                         // We tried to move the folder over
                                         // but it didn't work!
                                         // so instead of just simply deleting the folder
@@ -595,12 +646,6 @@ impl ExtractTarball {
                                             .copy_from_slice(&[b't', b'm', b'p', 0]);
                                         let tempdest =
                                             ZStr::from_buf(&tempdest_buf, tmpname.len() + 3);
-                                        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());
                                         match sys::renameat(
                                             Fd::from_std_dir(cache_dir),
                                             folder_name_z,
@@ -637,7 +682,7 @@ impl ExtractTarball {
                                     bun_fmt::s(folder_name),
                                 ),
                             );
-                            return Err(crate::Error::InstallFailed);
+                            break 'moved Err(crate::Error::InstallFailed);
                         }
                         bun_sys::Result::Ok(_) => {
                             let _ = sys::close(dir_to_move);
@@ -646,47 +691,50 @@ impl ExtractTarball {
 
                     break;
                 }
-            }
+                Ok(())
+            };
             #[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.
-                //
-
+            let moved: Result<(), Error> = {
                 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(
+                match sys::renameat_concurrently_a(
                     tmpdir.fd(),
                     tmpname.as_bytes(),
                     cache_dir.fd(),
                     folder_name,
                     sys::RenameatConcurrentlyOptions {
                         move_fallback: true,
+                        keep_existing_destination: publish == CachePublish::KeepExisting,
                     },
                 ) {
-                    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);
+                    Ok(()) => Ok(()),
+                    Err(err) => {
+                        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),
+                            ),
+                        );
+                        Err(crate::Error::InstallFailed)
+                    }
                 }
+            };
+
+            // The temp name now holds our own copy (failed or lost), the
+            // superseded folder, or a `Replace` swap-out (see `CachePublish`).
+            if moved.is_err() || publish != CachePublish::Replace {
+                let _ = tmpdir.delete_tree(tmpname.as_bytes());
             }
+            moved?;
 
             // We return a resolved absolute absolute file path to the cache dir.
             // To get that directory, we open the directory again.

src/install/repository.rs

@@ -468,9 +468,7 @@ impl CacheStaging {
             self.tmp_name(),
             self.cache_dir,
             folder_name,
-            bun_sys::RenameatConcurrentlyOptions {
-                move_fallback: false,
-            },
+            bun_sys::RenameatConcurrentlyOptions::default(),
         );
         // After an exchange the temporary name holds the folder that was replaced.
         self.discard();

src/install/resolution.rs

@@ -965,6 +965,12 @@ impl Tag {
         self == Tag::Git || self == Tag::Github
     }
 
+    /// Cached under a hash of the path or URL, so the folder's contents can
+    /// change from one extraction to the next.
+    pub(crate) fn is_tarball(self) -> bool {
+        self == Tag::LocalTarball || self == Tag::RemoteTarball
+    }
+
     pub(crate) fn can_enqueue_install_task(self) -> bool {
         self == Tag::Npm
             || self == Tag::LocalTarball