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,49 @@ pub(crate) fn uses_streaming_extraction() -> bool {
         .unwrap_or(false)
 }
 
+/// What `move_to_cache_directory` does when the cache already has a folder
+/// under the name being published. Decided by `cache_publish()` before
+/// extraction starts; by the time the extracted tree is renamed into place,
+/// a folder that was there all along and one a concurrent install published
+/// in the meantime look the same.
+#[derive(Clone, Copy, PartialEq, Eq)]
+pub(crate) enum CachePublish {
+    /// Swap the fresh copy in and leave the one swapped out in the temp
+    /// directory: it may have been published by a concurrent install that is
+    /// still installing from it. npm and GitHub folders are published this way.
+    Replace,
+    /// The folder was already there when this tarball's task started, so this
+    /// is a re-extraction of the same tarball path or URL (no lockfile, or the
+    /// tarball behind it changed): swap the fresh copy in and delete the one
+    /// swapped out.
+    Supersede,
+    /// The folder was missing when this tarball's task started. If it exists
+    /// now, a concurrent install extracted the same tarball first: keep its
+    /// copy and delete ours, which nothing else can have opened.
+    KeepExisting,
+}
+
 impl ExtractTarball {
+    /// See [`CachePublish`]. Must run before anything is extracted.
+    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 +298,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 +482,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 +495,7 @@ impl ExtractTarball {
         name: &[u8],
         basename: &[u8],
         resolved: &[u8],
+        publish: CachePublish,
     ) -> Result<ExtractData, Error> {
         let package_manager = self.package_manager.get();
 
@@ -521,7 +565,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 +581,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 +610,7 @@ impl ExtractTarball {
                                     bun_fmt::s(folder_name),
                                 ),
                             );
-                            return Err(crate::Error::InstallFailed);
+                            break 'moved Err(crate::Error::InstallFailed);
                         }
                     };
 
@@ -582,6 +630,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 +653,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 +689,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 +698,54 @@ 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(
+                // An existing folder is normally swapped out atomically (see
+                // `renameat_concurrently`); `KeepExisting` leaves it alone.
+                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)
+                    }
                 }
+            };
+
+            // Whatever is left under the temp name is either our own copy (the
+            // move failed, or a concurrent install published first) or the
+            // folder we superseded. `Replace` leaves a swapped-out folder
+            // alone, 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,13 @@ impl Tag {
         self == Tag::Git || self == Tag::Github
     }
 
+    /// A tarball addressed by path or URL. Its cache folder is named after
+    /// that address, not the contents, so the same folder can hold different
+    /// versions of the tarball over time.
+    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