Skip to content

Fix git repository cloning to use temp directory first #23601

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Fix git repository cloning to use temp directory first #23601

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c61f112
Fix git repository cloning to use temp directory first
Oct 13, 2025
63db502
Address CodeRabbit review: Add cleanup and race handling
Oct 13, 2025
f0b7042
Add verbose logging for git repository operations
Oct 13, 2025
bc6cdda
Address CodeRabbit nitpicks: improve logging clarity
Oct 13, 2025
c7a2c79
Update ban-limits.json for std.fs.Dir usage
Oct 13, 2025
2e062b1
Merge branch 'main' into claude/fix-git-clone-to-temp-first
Jarred-Sumner Oct 14, 2025
8ce582d
Merge branch 'main' into claude/fix-git-clone-to-temp-first
Jarred-Sumner Oct 20, 2025
009adfd
update
dylan-conway Oct 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/install/PackageManagerTask.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,7 @@ pub fn callback(task: *ThreadPool.Task) void {
this.request.git_clone.env,
&this.log,
manager.getCacheDirectory(),
manager.getTemporaryDirectory().handle,
this.id,
name,
https,
Expand Down Expand Up @@ -189,6 +190,7 @@ pub fn callback(task: *ThreadPool.Task) void {
this.request.git_clone.env,
&this.log,
manager.getCacheDirectory(),
manager.getTemporaryDirectory().handle,
this.id,
name,
ssh,
Expand All @@ -213,6 +215,7 @@ pub fn callback(task: *ThreadPool.Task) void {
this.request.git_checkout.env,
&this.log,
manager.getCacheDirectory(),
manager.getTemporaryDirectory().handle,
git_checkout.repo_dir.stdDir(),
git_checkout.name.slice(),
git_checkout.url.slice(),
Expand Down
139 changes: 128 additions & 11 deletions src/install/repository.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -480,6 +480,7 @@ pub const Repository = extern struct {
env: DotEnv.Map,
log: *logger.Log,
cache_dir: std.fs.Dir,
temp_dir: std.fs.Dir,
task_id: Install.Task.Id,
Comment on lines +483 to 484
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Signature change: ensure all call sites pass temp_dir

Both download() and checkout() now require temp_dir. Compile passed, but please double-check all call sites.

Run:

Also applies to: 633-634

🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

echo "Repository.download call sites:"
rg -nP --type=zig -C2 '\bRepository\.download\s*\('

echo
echo "Repository.checkout call sites:"
rg -nP --type=zig -C2 '\bRepository\.checkout\s*\('

echo
echo "Potential method-style calls (may include noise):"
rg -nP --type=zig -C2 '\.\s*(download|checkout)\s*\('

Length of output: 5006

🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

echo "Full Repository.download invocations in PackageManagerTask.zig:"
rg -nP --type=zig -C5 'Repository\.download\(' src/install/PackageManagerTask.zig

echo
echo "Full Repository.checkout invocations in PackageManagerTask.zig:"
rg -nP --type=zig -C5 'Repository\.checkout\(' src/install/PackageManagerTask.zig

echo
echo "Signature of download in repository.zig:"
rg -nP --type=zig -C3 'pub fn download' src/install/repository.zig

echo
echo "Signature of checkout in repository.zig:"
rg -nP --type=zig -C3 'pub fn checkout' src/install/repository.zig

Length of output: 2133

Add missing task_id argument to Repository.download and checkout calls
The invocations in src/install/PackageManagerTask.zig (lines 160, 188, 213) pass allocator, env, log, cacheDir, and tempDir but omit the new task_id parameter. Append the appropriate Install.Task.Id to each call.

🤖 Prompt for AI Agents 
In src/install/repository.zig around lines 484-485, the Repository API now
requires a task_id parameter; update the three calls in
src/install/PackageManagerTask.zig (around lines 160, 188, 213) that currently
pass allocator, env, log, cacheDir, tempDir to also pass the current
Install.Task.Id (e.g., task.id) as the final argument so each
Repository.download and Repository.checkout invocation matches the new
signature.

name: string,
url: string,
Expand Down Expand Up @@ -511,7 +512,24 @@ pub const Repository = extern struct {
} else |not_found| clone: {
if (not_found != error.FileNotFound) return not_found;

const target = Path.joinAbsString(PackageManager.get().cache_directory_path, &.{folder_name}, .auto);
const time_started_for_verbose_logs: u64 = if (PackageManager.verbose_install) bun.getRoughTickCount().ns() else 0;

const temp_path_buf = bun.path_buffer_pool.get();
defer bun.path_buffer_pool.put(temp_path_buf);
const tmpname_buf = bun.path_buffer_pool.get();
defer bun.path_buffer_pool.put(tmpname_buf);

// Clone to temp directory first
const tmpname = try FileSystem.tmpname(folder_name[0..@min(folder_name.len, 32)], tmpname_buf, bun.fastRandom());
const temp_target = try bun.getFdPath(.fromStdDir(temp_dir), temp_path_buf);
const temp_target_full = Path.joinAbsString(temp_target, &.{tmpname}, .auto);
var moved = false;
errdefer if (!moved) temp_dir.deleteTree(tmpname) catch {};

if (PackageManager.verbose_install) {
Output.prettyErrorln("[{s}] Cloning git repository (bare) to {s}<r>", .{ name, temp_target_full });
Output.flush();
}
Comment on lines +529 to +532
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Use a scoped logger for new verbose logs

Switch prettyErrorln/flush to a scoped logger per project guidelines.

Apply once near top-level (or per function) and replace calls:

+const RepoLog = Output.scoped(.install_repository, .hidden);
...
-                Output.prettyErrorln("[{s}] Cloning git repository (bare) to {s}<r>", .{ name, temp_target_full });
-                Output.flush();
+                RepoLog("[{s}] Cloning git repository (bare) to {s}", .{ name, temp_target_full });
...
-                Output.prettyErrorln("[{s}] Cloned bare repository to cache ({})<r>", .{ name, std.fmt.fmtDuration(elapsed) });
-                Output.flush();
+                RepoLog("[{s}] Cloned bare repository to cache ({})", .{ name, std.fmt.fmtDuration(elapsed) });
...
-                Output.prettyErrorln("[{s}] Cloning git repository to {s}<r>", .{ name, temp_target_full });
-                Output.flush();
+                RepoLog("[{s}] Cloning git repository to {s}", .{ name, temp_target_full });
...
-                Output.prettyErrorln("[{s}] Checking out {s}<r>", .{ name, resolved });
-                Output.flush();
+                RepoLog("[{s}] Checking out {s}", .{ name, resolved });
...
-                Output.prettyErrorln("[{s}] Checked out to cache ({})<r>", .{ name, std.fmt.fmtDuration(elapsed) });
-                Output.flush();
+                RepoLog("[{s}] Checked out to cache ({})", .{ name, std.fmt.fmtDuration(elapsed) });

As per coding guidelines

Also applies to: 587-591, 661-664, 685-688, 745-749

🤖 Prompt for AI Agents 
In src/install/repository.zig around lines 529-532 (and similarly for 587-591,
661-664, 685-688, 745-749), replace direct calls to Output.prettyErrorln and
Output.flush with a scoped logger instance per the project guidelines: create or
obtain a scoped logger at the top of the function (or top-level scope for the
file), then call the scoped logger's verbose/pretty logging method instead of
Output.prettyErrorln and remove explicit Output.flush calls (or replace them
with the logger's flush if required). Ensure the logger is passed into any
helper functions that need to log so all verbose logs use the scoped logger
consistently.


_ = exec(allocator, env, &[_]string{
"git",
Expand All @@ -520,7 +538,7 @@ pub const Repository = extern struct {
"--quiet",
"--bare",
url,
target,
temp_target_full,
}) catch |err| {
if (err == error.RepositoryNotFound or attempt > 1) {
log.addErrorFmt(
Expand All @@ -534,6 +552,44 @@ pub const Repository = extern struct {
return err;
};

// Move from temp to cache atomically
if (bun.sys.renameatConcurrently(
.fromStdDir(temp_dir),
tmpname,
.fromStdDir(cache_dir),
folder_name,
.{ .move_fallback = true },
).asErr()) |err| {
// If destination exists, another process likely completed the work
if (cache_dir.openDirZ(folder_name, .{})) |dir2| {
// Best-effort cleanup of our temp path
temp_dir.deleteTree(tmpname) catch {};
moved = true; // cancel errdefer; already cleaned up
Comment on lines +566 to +567
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't we just let the errdefer delete the temp dir? The only difference would be that it runs after printing the verbose install message rather than before, but I don't think that's a big deal.

Then we can get rid of the moved variable.

if (PackageManager.verbose_install) {
Output.prettyErrorln("[{s}] Cache dir already exists (another process completed first)<r>", .{name});
Output.flush();
}
break :clone dir2;
} else |_| {}
const cache_target = try bun.getFdPath(.fromStdDir(cache_dir), &final_path_buf);
const dest_full = Path.joinAbsString(cache_target, &.{folder_name}, .auto);
log.addErrorFmt(
null,
logger.Loc.Empty,
allocator,
"moving \"{s}\" to cache dir failed: {}\n From: {s}\n To: {s}",
.{ name, err, temp_target_full, dest_full },
) catch unreachable;
return error.InstallFailed;
}
moved = true;

if (PackageManager.verbose_install) {
const elapsed = bun.getRoughTickCount().ns() - time_started_for_verbose_logs;
Output.prettyErrorln("[{s}] Cloned bare repository to cache ({})<r>", .{ name, std.fmt.fmtDuration(elapsed) });
Output.flush();
}

break :clone try cache_dir.openDirZ(folder_name, .{});
};
}
Expand Down Expand Up @@ -577,6 +633,7 @@ pub const Repository = extern struct {
env: DotEnv.Map,
log: *logger.Log,
cache_dir: std.fs.Dir,
temp_dir: std.fs.Dir,
repo_dir: std.fs.Dir,
name: string,
url: string,
Expand All @@ -588,7 +645,23 @@ pub const Repository = extern struct {
var package_dir = bun.openDir(cache_dir, folder_name) catch |not_found| brk: {
if (not_found != error.ENOENT) return not_found;

const target = Path.joinAbsString(PackageManager.get().cache_directory_path, &.{folder_name}, .auto);
const time_started_for_verbose_logs: u64 = if (PackageManager.verbose_install) bun.getRoughTickCount().ns() else 0;

const temp_path_buf = bun.path_buffer_pool.get();
defer bun.path_buffer_pool.put(temp_path_buf);
const tmpname_buf = bun.path_buffer_pool.get();
defer bun.path_buffer_pool.put(tmpname_buf);

const tmpname = try FileSystem.tmpname(folder_name[0..@min(folder_name.len, 32)], tmpname_buf, bun.fastRandom());
const temp_target = try bun.getFdPath(.fromStdDir(temp_dir), temp_path_buf);
const temp_target_full = Path.joinAbsString(temp_target, &.{tmpname}, .auto);
var moved = false;
errdefer if (!moved) temp_dir.deleteTree(tmpname) catch {};

if (PackageManager.verbose_install) {
Output.prettyErrorln("[{s}] Cloning git repository to {s}<r>", .{ name, temp_target_full });
Output.flush();
}

_ = exec(allocator, env, &[_]string{
"git",
Expand All @@ -597,7 +670,7 @@ pub const Repository = extern struct {
"--quiet",
"--no-checkout",
try bun.getFdPath(.fromStdDir(repo_dir), &final_path_buf),
target,
temp_target_full,
}) catch |err| {
log.addErrorFmt(
null,
Expand All @@ -609,9 +682,12 @@ pub const Repository = extern struct {
return err;
};

const folder = Path.joinAbsString(PackageManager.get().cache_directory_path, &.{folder_name}, .auto);
if (PackageManager.verbose_install) {
Output.prettyErrorln("[{s}] Checking out {s}<r>", .{ name, resolved });
Output.flush();
}

_ = exec(allocator, env, &[_]string{ "git", "-C", folder, "checkout", "--quiet", resolved }) catch |err| {
_ = exec(allocator, env, &[_]string{ "git", "-C", temp_target_full, "checkout", "--quiet", resolved }) catch |err| {
log.addErrorFmt(
null,
logger.Loc.Empty,
Expand All @@ -621,18 +697,58 @@ pub const Repository = extern struct {
) catch unreachable;
return err;
};
var dir = try bun.openDir(cache_dir, folder_name);
dir.deleteTree(".git") catch {};

var temp_pkg_dir = try bun.openDir(temp_dir, tmpname);
temp_pkg_dir.deleteTree(".git") catch {};

if (resolved.len > 0) insert_tag: {
const git_tag = dir.createFileZ(".bun-tag", .{ .truncate = true }) catch break :insert_tag;
const git_tag = temp_pkg_dir.createFileZ(".bun-tag", .{ .truncate = true }) catch break :insert_tag;
defer git_tag.close();
git_tag.writeAll(resolved) catch {
dir.deleteFileZ(".bun-tag") catch {};
temp_pkg_dir.deleteFileZ(".bun-tag") catch {};
};
}
temp_pkg_dir.close();

Comment on lines +701 to +712
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Remove .git and tag the worktree: LGTM (optional verbose on failure)

Silently ignoring .git removal is acceptable; consider emitting a verbose-only note if removal fails.

// Move from temp to cache atomically
if (bun.sys.renameatConcurrently(
.fromStdDir(temp_dir),
tmpname,
.fromStdDir(cache_dir),
folder_name,
.{ .move_fallback = true },
).asErr()) |err| {
// If destination exists, another process likely completed the work
if (bun.openDir(cache_dir, folder_name)) |_| {
// Best-effort cleanup of our temp path
temp_dir.deleteTree(tmpname) catch {};
moved = true; // cancel errdefer; already cleaned up
if (PackageManager.verbose_install) {
Output.prettyErrorln("[{s}] Cache dir already exists (another process completed first)<r>", .{name});
Output.flush();
}
break :brk try bun.openDir(cache_dir, folder_name);
} else |_| {}
const cache_target = try bun.getFdPath(.fromStdDir(cache_dir), &final_path_buf);
const dest_full = Path.joinAbsString(cache_target, &.{folder_name}, .auto);
log.addErrorFmt(
null,
logger.Loc.Empty,
allocator,
"moving \"{s}\" to cache dir failed: {}\n From: {s}\n To: {s}",
.{ name, err, temp_target_full, dest_full },
) catch unreachable;
return error.InstallFailed;
}
moved = true;

if (PackageManager.verbose_install) {
const elapsed = bun.getRoughTickCount().ns() - time_started_for_verbose_logs;
Output.prettyErrorln("[{s}] Checked out to cache ({})<r>", .{ name, std.fmt.fmtDuration(elapsed) });
Output.flush();
}

break :brk dir;
break :brk try bun.openDir(cache_dir, folder_name);
};
defer package_dir.close();

Expand Down Expand Up @@ -695,6 +811,7 @@ const PackageManager = Install.PackageManager;

const bun = @import("bun");
const OOM = bun.OOM;
const Output = bun.Output;
const Path = bun.path;
const logger = bun.logger;
const strings = bun.strings;
Expand Down
2 changes: 1 addition & 1 deletion test/internal/ban-limits.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
"std.debug.dumpStackTrace": 0,
"std.debug.print": 0,
"std.enums.tagName(": 2,
"std.fs.Dir": 165,
"std.fs.Dir": 167,
"std.fs.File": 62,
"std.fs.cwd": 103,
"std.log": 1,
Expand Down