Skip to content
11 changes: 11 additions & 0 deletions src/install/PackageInstall.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2350,6 +2350,17 @@
ZStr::from_buf(&buf[..], subpath_len + 1 + b"package.json".len());
break 'package_json_exists sys::exists_at(self.cache_dir, subpath);
}
resolution::Tag::Git => {
// Git checkouts can legitimately lack `package.json`;
// the `.bun-tag` written last by `Repository::checkout`
// marks the folder complete.
Comment thread
robobun marked this conversation as resolved.
Outdated
let mut join_buf = PathBuffer::uninit();
let tag_path = path::resolve_path::join_z_buf::<path::platform::Auto>(
&mut join_buf.0,
&[self.cache_dir_subpath.as_bytes(), b".bun-tag"],
);
sys::exists_at(self.cache_dir, tag_path)
}

Check warning on line 2363 in src/install/PackageInstall.rs

View check run for this annotation

Claude / Claude Code Review

Patched git dep: base-folder check in package_missing_from_cache still trusts bare directory

The `patch.is_some()` branch of `package_missing_from_cache` (line 2386) still checks the stripped `@G@<sha>` base folder via `directory_exists_at`, without the `.bun-tag` check. For a **patched** git dependency with a poisoned base folder, this branch overrides the preinstall state to `Done`, `enqueue_git_for_checkout` is skipped, and the patch task copies the empty base folder — the same empty-package symptom this PR fixes elsewhere. This branch should mirror the `non_patched_path` fix already
Comment thread
robobun marked this conversation as resolved.
_ => sys::directory_exists_at(self.cache_dir, self.cache_dir_subpath)
.unwrap_or(false),
};
Expand Down
14 changes: 14 additions & 0 deletions src/install/PackageManager/PackageManagerDirectories.rs
Original file line number Diff line number Diff line change
Expand Up @@ -754,6 +754,20 @@ pub fn is_folder_in_cache(this: &mut PackageManager, folder_path: &ZStr) -> bool
sys::directory_exists_at(get_cache_directory(this), folder_path).unwrap_or(false)
}

/// Git checkouts can legitimately lack `package.json`, so their completeness
/// marker is the `.bun-tag` that `Repository::checkout` writes as the last
/// step of populating the folder. A bare folder without it is a leftover from
/// an install that was killed mid-checkout; treating it as cached would
/// silently install an empty package.
Comment thread
robobun marked this conversation as resolved.
Outdated
pub fn is_git_folder_in_cache(this: &mut PackageManager, folder_path: &ZStr) -> bool {
let mut buf = PathBuffer::uninit();
let tag_path = path::resolve_path::join_z_buf::<path::platform::Auto>(
&mut buf.0,
&[folder_path.as_bytes(), b".bun-tag"],
);
sys::exists_at(get_cache_directory(this), tag_path)
}
Comment thread
robobun marked this conversation as resolved.
Comment thread
coderabbitai[bot] marked this conversation as resolved.

// ─────────────────────────── global directories ───────────────────────────────

pub fn setup_global_dir(manager: &mut PackageManager, ctx: &Command::Context) -> Result<(), Error> {
Expand Down
15 changes: 13 additions & 2 deletions src/install/PackageManager/PackageManagerLifecycle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,13 @@ impl PackageManager {
return PreinstallState::Extract;
}

if directories::is_folder_in_cache(self, folder_path) {
let folder_in_cache =
if matches!(pkg.resolution.tag, ResolutionTag::Git) && patch_hash.is_none() {
directories::is_git_folder_in_cache(self, folder_path)
} else {
directories::is_folder_in_cache(self, folder_path)
};
if folder_in_cache {
self.set_preinstall_state(pkg.meta.id, PreinstallState::Done);
return PreinstallState::Done;
}
Expand All @@ -200,7 +206,12 @@ impl PackageManager {
});
// Owned NUL-terminated copy.
let non_patched_path = ZBox::from_bytes(&folder_path.as_bytes()[..idx]);
if directories::is_folder_in_cache(self, &non_patched_path) {
let base_in_cache = if matches!(pkg.resolution.tag, ResolutionTag::Git) {
directories::is_git_folder_in_cache(self, &non_patched_path)
} else {
directories::is_folder_in_cache(self, &non_patched_path)
};
if base_in_cache {
self.set_preinstall_state(pkg.meta.id, PreinstallState::ApplyPatch);
// yay step 1 is already done for us
return PreinstallState::ApplyPatch;
Expand Down
14 changes: 14 additions & 0 deletions src/install/isolated_install.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2370,6 +2370,20 @@ pub(crate) fn install_isolated_packages(
pkg_cache_dir_subpath.set_length(cache_dir_path_save);
exists
}
ResolutionTag::Git => {
// Git checkouts can legitimately lack
// `package.json`; the `.bun-tag` written
// last by `Repository::checkout` marks
// the folder complete.
Comment thread
robobun marked this conversation as resolved.
Outdated
let cache_dir_path_save = pkg_cache_dir_subpath.len();
pkg_cache_dir_subpath.append(b".bun-tag").assume_ok();
let exists = sys::exists_at(
cache_dir,
pkg_cache_dir_subpath.slice_z(),
);
pkg_cache_dir_subpath.set_length(cache_dir_path_save);
exists
}
_ => sys::directory_exists_at(
cache_dir,
pkg_cache_dir_subpath.slice_z(),
Expand Down
223 changes: 149 additions & 74 deletions src/install/repository.rs
Original file line number Diff line number Diff line change
Expand Up @@ -421,6 +421,23 @@ fn exec(env: &bun_dotenv::Map, argv: &[&[u8]]) -> Result<Vec<u8>, Error> {
Err(crate::Error::InstallFailed)
}

/// `.bun-tag` (containing `resolved`) is written as the last step of
/// populating a per-commit cache folder, so it doubles as the completeness
/// marker: a folder without a matching tag is a leftover from an install that
/// was killed or failed between `git clone` and `git checkout` (git cannot
/// clean up after SIGKILL). Such a folder has no `package.json`, and the
/// missing-`package.json` fallback in `checkout` would silently resolve the
/// dependency as an empty package.
Comment thread
robobun marked this conversation as resolved.
Outdated
fn cached_checkout_is_complete(dir: &bun_sys::Dir, resolved: &[u8]) -> bool {
match bun_sys::File::read_file_from(dir.fd(), b".bun-tag") {
Ok((file, contents)) => {
let _ = file.close(); // close error is non-actionable
contents == resolved
}
Err(_) => false,
}
}

impl RepositoryExt for Repository {
fn parse_append_git(input: &[u8], buf: &mut StringBuf<'_>) -> Result<Repository, AllocError> {
let mut remain = input;
Expand Down Expand Up @@ -874,92 +891,150 @@ impl RepositoryExt for Repository {
)
.as_bytes();

let package_dir = match bun_sys::Dir::borrow(&cache_dir)
.open_at(folder_name)
.map_err(Error::from)
{
Ok(d) => d,
Err(not_found) => 'brk: {
if not_found != crate::Error::Sys(bun_errno::SystemErrno::ENOENT) {
return Err(not_found);
let package_dir = 'package_dir: {
match bun_sys::Dir::borrow(&cache_dir).open_at(folder_name) {
Ok(dir) => {
if cached_checkout_is_complete(&dir, resolved) {
break 'package_dir dir;
}
// Leftover from an interrupted or failed clone/checkout.
// Rebuild it; the rename below replaces it.
Comment thread
robobun marked this conversation as resolved.
Outdated
dir.close();
}
Err(err) => {
if err.get_errno() != bun_sys::E::ENOENT {
return Err(err.into());
}
}
}

let target = Path::resolve_path::join_abs_string::<Path::platform::Auto>(
&PackageManager::get().cache_directory_path,
&[folder_name],
);
// Clone + checkout into a temporary sibling and only rename it to
// `folder_name` once fully populated, so a killed install can't
// leave a half-built folder at the name later installs trust.
Comment thread
robobun marked this conversation as resolved.
Outdated
let mut tmp_name_buf = [0u8; 64];
let tmp_name: &[u8] = match bun_resolver::fs::FileSystem::tmpname(
b"tmp",
&mut tmp_name_buf,
bun_core::fast_random(),
) {
Ok(name) => name.as_bytes(),
// max len is 1+16+1+8+1+3, well below 64
Err(_no_space_left) => unreachable!(),
};

let repo_path = bun_sys::get_fd_path(
repo_dir,
// Per-field accessor — disjoint from `folder_name_buf`
// borrow above. See `TlBufs` accessor doc.
TlBufs::final_path_buf(),
)?;
let target = Path::resolve_path::join_abs_string::<Path::platform::Auto>(
&PackageManager::get().cache_directory_path,
&[tmp_name],
);

if let Err(err) = exec(
env,
&[
b"git",
b"clone",
b"-c",
b"core.longpaths=true",
b"--quiet",
b"--no-checkout",
repo_path,
target,
],
) {
log.add_error_fmt(
None,
bun_ast::Loc::EMPTY,
format_args!("\"git clone\" for \"{}\" failed", BStr::new(name)),
);
return Err(err);
}
let repo_path = bun_sys::get_fd_path(
repo_dir,
// Per-field accessor — disjoint from `folder_name_buf`
// borrow above. See `TlBufs` accessor doc.
Comment thread
robobun marked this conversation as resolved.
TlBufs::final_path_buf(),
)?;

let folder = Path::resolve_path::join_abs_string::<Path::platform::Auto>(
&PackageManager::get().cache_directory_path,
&[folder_name],
if let Err(err) = exec(
env,
&[
b"git",
b"clone",
b"-c",
b"core.longpaths=true",
b"--quiet",
b"--no-checkout",
repo_path,
target,
],
) {
log.add_error_fmt(
None,
bun_ast::Loc::EMPTY,
format_args!("\"git clone\" for \"{}\" failed", BStr::new(name)),
);
let _ = bun_sys::Dir::borrow(&cache_dir).delete_tree(tmp_name);
return Err(err);
}

if let Err(err) = exec(
env,
// `is_safe_resolved_tag` above rejects a leading `-`, so
// `resolved` cannot be parsed as a git option.
&[b"git", b"-C", folder, b"checkout", b"--quiet", resolved],
) {
log.add_error_fmt(
None,
bun_ast::Loc::EMPTY,
format_args!("\"git checkout\" for \"{}\" failed", BStr::new(name)),
);
return Err(err);
let folder = Path::resolve_path::join_abs_string::<Path::platform::Auto>(
&PackageManager::get().cache_directory_path,
&[tmp_name],
);
Comment thread
coderabbitai[bot] marked this conversation as resolved.

if let Err(err) = exec(
env,
// `is_safe_resolved_tag` above rejects a leading `-`, so
// `resolved` cannot be parsed as a git option.
Comment thread
robobun marked this conversation as resolved.
&[b"git", b"-C", folder, b"checkout", b"--quiet", resolved],
) {
log.add_error_fmt(
None,
bun_ast::Loc::EMPTY,
format_args!("\"git checkout\" for \"{}\" failed", BStr::new(name)),
);
let _ = bun_sys::Dir::borrow(&cache_dir).delete_tree(tmp_name);
return Err(err);
}
let dir = match bun_sys::Dir::borrow(&cache_dir).open_at(tmp_name) {
Ok(d) => d,
Err(err) => {
let _ = bun_sys::Dir::borrow(&cache_dir).delete_tree(tmp_name);
return Err(err.into());
}
let dir = bun_sys::Dir::borrow(&cache_dir)
.open_at(folder_name)
.map_err(Error::from)?;
let _ = dir.delete_tree(b".git");

if !resolved.is_empty() {
'insert_tag: {
let Ok(git_tag) = dir.create_file_z(
bun_core::zstr!(".bun-tag"),
bun_sys::CreateFlags {
truncate: true,
..Default::default()
},
) else {
break 'insert_tag;
};
if git_tag.write_all(resolved).is_err() {
let _ = dir.delete_file_z(bun_core::zstr!(".bun-tag"));
}
let _ = git_tag.close(); // close error is non-actionable
};
let _ = dir.delete_tree(b".git");

if !resolved.is_empty() {
'insert_tag: {
let Ok(git_tag) = dir.create_file_z(
bun_core::zstr!(".bun-tag"),
bun_sys::CreateFlags {
truncate: true,
..Default::default()
},
) else {
break 'insert_tag;
};
if git_tag.write_all(resolved).is_err() {
let _ = dir.delete_file_z(bun_core::zstr!(".bun-tag"));
}
let _ = git_tag.close(); // close error is non-actionable
}
}

break 'brk dir;
// Close before the rename: Windows can't move a directory while a
// handle into it is open.
Comment thread
robobun marked this conversation as resolved.
dir.close();
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated

if let Err(err) = bun_sys::renameat_concurrently_a(
cache_dir,
tmp_name,
cache_dir,
folder_name,
bun_sys::RenameatConcurrentlyOptions {
move_fallback: false,
},
) {
log.add_error_fmt(
None,
bun_ast::Loc::EMPTY,
format_args!(
"moving git checkout of \"{}\" into the cache failed: {}",
BStr::new(name),
err,
),
);
let _ = bun_sys::Dir::borrow(&cache_dir).delete_tree(tmp_name);
return Err(crate::Error::InstallFailed);
}

// If a concurrent install won the rename race, the swap left its
// folder (or the stale one it replaced) at `tmp_name`; drop it.
Comment thread
robobun marked this conversation as resolved.
Outdated
let _ = bun_sys::Dir::borrow(&cache_dir).delete_tree(tmp_name);

bun_sys::Dir::borrow(&cache_dir)
.open_at(folder_name)
.map_err(Error::from)?
};

let (json_file, json_buf) =
Expand Down
Loading