Fix git repository cloning to use temp directory first #23601
Conversation
Changes git clone operations during bun install to: 1. Clone to temporary directory first 2. Use renameatConcurrently to atomically move to cache This prevents race conditions when multiple bun install processes run concurrently and clone the same git dependency. Both download() and checkout() functions now: - Accept a temp_dir parameter - Use FileSystem.tmpname() for temporary directory names - Clone to temp location first - Atomically move to final cache location using renameatConcurrently 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
WalkthroughPasses a temporary-directory handle from PackageManagerTask into Repository download/checkout flows. Repository now clones/checks out into a temp dir, then atomically renames the result into the cache and performs cleanup. Repository struct and download/checkout signatures were updated to accept Changes
Possibly related PRs
Suggested reviewers
Pre-merge checks✅ Passed checks (2 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/install/repository.zig (1)
616-663: Add cleanup guard for checkout temp dir; avoid orphaning on failuresIf clone/checkout/tagging fails, the temp directory is left behind. Add errdefer and set moved=true after successful rename; on rename “already exists,” delete temp explicitly.
// Clone to temp directory first var tmpname_buf: bun.PathBuffer = undefined; 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 {}; @@ _ = exec(allocator, env, &[_]string{ "git", "-C", temp_target_full, "checkout", "--quiet", resolved }) catch |err| { log.addErrorFmt( null, logger.Loc.Empty, allocator, "\"git checkout\" for \"{s}\" failed", .{name}, ) catch unreachable; return err; }; @@ var temp_pkg_dir = try bun.openDir(temp_dir, tmpname); temp_pkg_dir.deleteTree(".git") catch {}; @@ temp_pkg_dir.close();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/install/PackageManagerTask.zig(3 hunks)src/install/repository.zig(10 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/install/PackageManagerTask.zigsrc/install/repository.zig
src/**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: Use the # prefix to declare private fields in Zig structs (e.g., struct { #foo: u32 })
Prefer decl literals when initializing values in Zig (e.g., const decl: Decl = .{ .binding = 0, .value = 0 })
Place @import directives at the bottom of Zig files
Use @import("bun") instead of @import("root").bunWhen adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
Files:
src/install/PackageManagerTask.zigsrc/install/repository.zig
🔇 Additional comments (3)
src/install/repository.zig (2)
5-5: LGTM: threadlocal temp path bufferConsistent with other per-thread buffers; no concerns.
485-485: API: new temp_dir parameter — verify lifetime and cross-device behaviorPassing a std.fs.Dir for temp is right. Ensure manager.getTemporaryDirectory().handle remains valid during the whole clone/checkout and that rename across volumes is handled (you used move_fallback=true, which is good).
Also applies to: 605-605
src/install/PackageManagerTask.zig (1)
165-170: Correctly wires temp_dir into git clone/checkout flowsParameter threading looks right and matches repository.zig changes.
Please confirm:
- manager.getTemporaryDirectory().handle is a std.fs.Dir (or compatible) and remains open for the duration of the callback.
- It points to a location on the same filesystem as the cache when possible; when not, move_fallback handles cross‑device moves.
Also applies to: 193-198, 213-223
- Add errdefer cleanup for temp directories on failure - Handle race condition when destination already exists - Set moved flag to prevent cleanup after successful rename - Log absolute paths instead of relative in error messages - Best-effort cleanup when another process wins the race Co-Authored-By: CodeRabbit AI <[email protected]> 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Adds --verbose logging to track git clone and checkout operations: - Log clone start with temp directory name - Log checkout operations with resolved commit - Log successful completion with timing - Log when another process wins the race Follows same pattern as extract_tarball.zig verbose logs. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/install/repository.zig(11 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/install/repository.zig
src/**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: Use the # prefix to declare private fields in Zig structs (e.g., struct { #foo: u32 })
Prefer decl literals when initializing values in Zig (e.g., const decl: Decl = .{ .binding = 0, .value = 0 })
Place @import directives at the bottom of Zig files
Use @import("bun") instead of @import("root").bunWhen adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
Files:
src/install/repository.zig
🔇 Additional comments (10)
src/install/repository.zig (10)
5-5: Thread-local temp path buffer: good additionAppropriate for per-thread path assembly; avoids allocs and races.
484-485: API surface: temp_dir parameterSignature change is clear and localized; aligns with temp-first workflow.
516-525: Temp-first clone setup and errdefer cleanup: solidGood use of tmpname, absolute temp path, and errdefer to prevent leaks.
538-539: Passing absolute temp target to git clone: LGTMEnsures clone creates the expected temp directory.
581-586: Verbose elapsed timing: LGTMAccurate gating on verbose; duration calc is correct.
Also applies to: 733-737
630-631: API surface: temp_dir parameter (checkout)Matches download(); consistent propagation.
676-679: Checkout progress log: LGTMClear and helpful when verbose is enabled.
681-691: Checkout command against temp clone: LGTMCorrect use of -C with absolute temp_target_full.
739-740: Return opened cache dir: LGTMHappy-path is clear and resource-safe with defer above.
802-803: Output alias import: LGTMUsed by new debug logs; imports remain at bottom.
| 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(); | ||
|
|
There was a problem hiding this comment.
🧹 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.
- Show absolute temp path (temp_target_full) in verbose logs instead of tmpname - Set moved=true on EEXIST path to cancel errdefer cleanup - Log absolute destination path in error messages for better diagnostics Co-Authored-By: CodeRabbit AI <[email protected]> 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/install/repository.zig (2)
695-706: Optional: verbose note on .git removal failure (already noted previously)Silently ignoring .git removal is fine. Consider a verbose-only note if it fails, for diagnostics.
526-529: Prefer a scoped logger for new debug logsPer guidelines, use a scoped logger instead of Output.prettyErrorln/flush for new debug logs. Example:
- Output.prettyErrorln("[{s}] Cloning git repository (bare) to {s}<r>", .{ name, temp_target_full }); - Output.flush(); + const dbg = Output.scoped(.install_repository, .hidden); + dbg("[{s}] Cloning git repository (bare) to {s}<r>", .{ name, temp_target_full });As per coding guidelines
Also applies to: 584-588, 655-658, 679-682, 739-743
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/install/repository.zig(11 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/install/repository.zig
src/**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, manage memory carefully and use defer for cleanup of allocations/resources
src/**/*.zig: Use the # prefix to declare private fields in Zig structs (e.g., struct { #foo: u32 })
Prefer decl literals when initializing values in Zig (e.g., const decl: Decl = .{ .binding = 0, .value = 0 })
Place @import directives at the bottom of Zig files
Use @import("bun") instead of @import("root").bunWhen adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
Files:
src/install/repository.zig
🔇 Additional comments (4)
src/install/repository.zig (4)
5-5: LGTM: thread-local temp path bufferThread-local buffer addition is consistent with existing pattern.
516-525: Good: best-effort cleanup with errdeferGuarded temp-dir cleanup with moved flag prevents leaks on failure paths. Nice.
552-583: Atomic rename + EEXIST handling: solidTemp-first clone, atomic rename with move_fallback, and treating destination-exists as success with cleanup looks correct and concurrency-safe. Absolute path logging is helpful.
Also applies to: 707-737
809-809: Minor: import alias additionOK. If adopting scoped logger, keep Output alias and add a file- or function-scoped logger constant.
⛔ Skipped due to learnings
Learnt from: Jarred-Sumner PR: oven-sh/bun#23373 File: src/bun.js/api/BunObject.zig:2514-2521 Timestamp: 2025-10-08T13:56:00.875Z Learning: For Bun codebase: prefer using `bun.path` utilities (e.g., `bun.path.joinAbsStringBuf`, `bun.path.join`) over `std.fs.path` functions for path operations.
| temp_dir: std.fs.Dir, | ||
| task_id: Install.Task.Id, |
There was a problem hiding this comment.
🧩 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.zigLength 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.
Increased limit from 165 to 167 to account for new temp_dir parameters in Repository.download() and Repository.checkout() functions. These follow the same pattern as extract_tarball.zig which also uses std.fs.Dir for cache_dir and temp_dir parameters. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
| temp_dir.deleteTree(tmpname) catch {}; | ||
| moved = true; // cancel errdefer; already cleaned up |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/install/repository.zig (5)
534-542: Memory leak: captured stdout from exec() is discardedexec() returns an allocated stdout slice; assigning to
_leaks it. Capture and free.- _ = exec(allocator, env, &[_]string{ + const _git_out = exec(allocator, env, &[_]string{ "git", "clone", "-c core.longpaths=true", "--quiet", "--bare", url, temp_target_full, }) catch |err| { if (err == error.RepositoryNotFound or attempt > 1) { ... } return err; }; + defer allocator.free(_git_out);Also apply this pattern to similar exec() calls below.
666-674: Memory leak: captured stdout from exec() is discarded (clone --no-checkout)Same leak as above; free the returned buffer.
- _ = exec(allocator, env, &[_]string{ + const _git_out = exec(allocator, env, &[_]string{ "git", "clone", "-c core.longpaths=true", "--quiet", "--no-checkout", try bun.getFdPath(.fromStdDir(repo_dir), &final_path_buf), temp_target_full, }) catch |err| { ... }; + defer allocator.free(_git_out);
690-700: Memory leak: captured stdout from exec() is discarded (checkout)Free stdout returned by exec().
- _ = exec(allocator, env, &[_]string{ "git", "-C", temp_target_full, "checkout", "--quiet", resolved }) catch |err| { + const _git_out = exec(allocator, env, &[_]string{ "git", "-C", temp_target_full, "checkout", "--quiet", resolved }) catch |err| { log.addErrorFmt( ... ) catch unreachable; return err; }; + defer allocator.free(_git_out);
497-511: Also free stdout in the fetch fast-pathfetch output is ignored; free it to avoid leaks during repeated installs.
- _ = exec( + const _fetch_out = exec( allocator, env, &[_]string{ "git", "-C", path, "fetch", "--quiet" }, ) catch |err| { ... }; + defer allocator.free(_fetch_out);
362-401: Optional: harden exec() to reduce accidental leaksEven with call-site fixes, exec() can leak stderr on success. Free stderr always; free stdout on error.
const result = if (comptime Environment.isWindows) try std.process.Child.run(.{ .allocator = allocator, .argv = argv, .env_map = std_map.get(), }) else try std.process.Child.run(.{ .allocator = allocator, .argv = argv, .env_map = std_map.get(), }); + // Always free stderr; free stdout on error paths. + defer allocator.free(result.stderr); + errdefer allocator.free(result.stdout); switch (result.term) { .Exited => |sig| if (sig == 0) return result.stdout else if ( ... ) { return error.RepositoryNotFound; }, else => {}, } return error.InstallFailed;This minimizes leaks even if a caller forgets to free the returned stdout.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/install/repository.zig(10 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/install/repository.zig
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
When adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
src/**/*.zig: Use private fields in Zig with the#prefix (e.g.,struct { #foo: u32 };)
Prefer decl literals in Zig (e.g.,const decl: Decl = .{ .binding = 0, .value = 0 };)
Prefer placing@importstatements at the bottom of the Zig file (formatter may reorder automatically)
Prefer@import("bun")rather than@import("root").bunor@import("../bun.zig")In Zig code, be careful with allocators and use defer for cleanup
Files:
src/install/repository.zig
🔇 Additional comments (4)
src/install/repository.zig (4)
483-485: API change (temp_dir): LGTMNew temp_dir in download() signature is appropriate for temp-first flow.
515-521: Path buffer pooling and timing: LGTMCorrect use of path_buffer_pool and verbose timing.
Also applies to: 648-654
522-528: errdefer cleanup guard: LGTMTemp dir best-effort cleanup with moved flag is sound.
Also applies to: 655-660
555-586: Atomic rename and EEXIST handling: LGTMRace-safe move, absolute-path logging, and cleanup look correct; moved flag cancels errdefer.
If possible, add a stress test that forces the EEXIST branch (two concurrent installs of the same git dep) to guard regressions.
Also applies to: 713-744
| if (PackageManager.verbose_install) { | ||
| Output.prettyErrorln("[{s}] Cloning git repository (bare) to {s}<r>", .{ name, temp_target_full }); | ||
| Output.flush(); | ||
| } |
There was a problem hiding this comment.
🧹 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.
4 participants
What does this PR do?
Fixes a race condition in git repository cloning during
bun installby cloning to a temporary directory first, then atomically moving to the cache directory. This prevents cache corruption when multiplebun installprocesses run concurrently and try to clone the same git dependency.Problem
Previously, git repositories were cloned directly to the cache directory:
This caused race conditions when multiple
bun installprocesses ran concurrently and tried to clone the same git dependency, potentially corrupting the cache.Solution
Changed both
download()andcheckout()functions to:FileSystem.tmpname()renameatConcurrently()with fallbackerrdeferThis follows the same pattern used in
extract_tarball.zigfor handling npm tarballs.Changes
src/install/repository.zigtemp_dirparameter todownload()andcheckout()temp_path_bufthreadlocal bufferrenameatConcurrently()for atomic move to cacheerrdefercleanup and race condition handlingsrc/install/PackageManagerTask.zigmanager.getTemporaryDirectory().handleHow did you verify your code works?
bun run zig:check-all) - all 13 platforms compile successfullybun installprocesses succeed without conflictsshould edit package json correctly with git dependencies)Example
🤖 Generated with Claude Code
Co-Authored-By: Claude [email protected]