install: re-clone git dependency cache folders left incomplete by a killed install - #37145
install: re-clone git dependency cache folders left incomplete by a killed install#37145robobun wants to merge 8 commits into
Conversation
…illed install A git dependency is cached per commit by running git clone --no-checkout into the final cache folder and git checkout inside it, with .bun-tag written last. If the install (or the spawned git process) is killed between those steps, the folder exists but is empty. Later installs trusted any existing folder, hit the allow-git-dependencies-without- package.json fallback, and silently resolved the dependency as an empty package whose name falls back to the URL basename (e.g. "repo.git"). Repository::checkout now clones and checks out into a temporary sibling and renames it into place once fully populated, so a kill at any point cannot leave a half-built folder at the trusted name. On a cache hit it verifies .bun-tag matches the resolved commit and rebuilds the folder otherwise, healing caches that were already poisoned. The preinstall cache checks (hoisted and isolated) now also require .bun-tag for git resolutions so a poisoned folder is checked out again instead of being copied into node_modules.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughGit dependency cache validation now requires a matching ChangesGit cache recovery
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…check The patch.is_some() branch of package_missing_from_cache checked bare directory existence of the stripped @g@<sha> base folder, so a poisoned base folder for a patched git dependency would still be marked Done and skip the re-checkout. Mirror the .bun-tag probe used by the unpatched arm and determine_preinstall_state.
|
Good catch on the patched branch: No new test for that specific combination: there is no git-dependency patch fixture today because The patched-folder check itself ( |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/install/PackageManager/PackageManagerDirectories.rs`:
- Around line 762-769: Centralize the `.bun-tag` completeness check by keeping
`is_git_folder_in_cache` in
`src/install/PackageManager/PackageManagerDirectories.rs:762-769` and adding a
sibling helper accepting an explicit `cache_dir: Fd` and subpath. Replace the
inline checks with that helper in `src/install/PackageInstall.rs:2353-2363` and
`src/install/PackageInstall.rs:2380-2396` (passing
`cache_dir_subpath_without_patch_hash` in the patched-Git branch), and in
`src/install/isolated_install.rs:2373-2386`; remove the duplicated buffer,
marker, existence, and length-reset logic at those sites.
In `@src/install/repository.rs`:
- Around line 925-962: Reuse the existing target path computed before the clone
instead of recomputing the identical cache path as folder. Update the later git
checkout flow to pass target, while preserving target’s use in the git clone
invocation and all existing cleanup behavior.
- Around line 987-1007: Make the `.bun-tag` write in the checkout flow
unconditional, removing the redundant `!resolved.is_empty()` guard because
`is_safe_resolved_tag` already rejects empty values. In the `'insert_tag` block,
propagate failures from `create_file_z`, `write_all`, and `close` as checkout
errors instead of breaking or continuing; delete the temporary checkout tree
before returning on any failure, and only proceed to the rename after the marker
is successfully written.
In `@test/cli/install/bun-install.test.ts`:
- Around line 5304-5318: Update the install helper’s Promise.all destructuring
to retain proc.stdout.text() alongside stderr and exitCode, then include stdout
in the failure assertions together with the exit-code check so both process
streams appear in test failures. Keep the existing retry and stderr assertions
unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ab8583b0-a88c-4f47-9dee-646472b435b8
📒 Files selected for processing (6)
src/install/PackageInstall.rssrc/install/PackageManager/PackageManagerDirectories.rssrc/install/PackageManager/PackageManagerLifecycle.rssrc/install/isolated_install.rssrc/install/repository.rstest/cli/install/bun-install.test.ts
… tag write errors Review follow-ups: one is_git_folder_in_cache_at helper replaces the three inline probes; a .bun-tag create/write failure now fails the checkout and removes the temporary folder instead of publishing a folder every later install would treat as missing; drop the always-true resolved.is_empty() guard; surface stdout in the test install helper's failure output.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/install/PackageManager/PackageManagerDirectories.rs (1)
758-769: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftUse one exact
.bun-tagvalidation contract for all Git cache hits.The checkout producer compares the marker contents, but these consumers only test whether the marker exists. A stale or partial marker can therefore be treated as
PreinstallState::Done.
src/install/PackageManager/PackageManagerDirectories.rs#L758-L769: pass the expected resolved commit tois_git_folder_in_cacheand compare marker contents.src/install/PackageInstall.rs#L2353-L2362: validate the unpatched Git marker before marking the cache complete.src/install/PackageInstall.rs#L2379-L2394: validate the base resolved commit for patched Git dependencies.src/install/isolated_install.rs#L2373-L2384: apply the same exact-marker check and preserve buffer restoration.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/install/PackageManager/PackageManagerDirectories.rs` around lines 758 - 769, Use one exact .bun-tag validation contract for every Git cache hit: in src/install/PackageManager/PackageManagerDirectories.rs lines 758-769, update is_git_folder_in_cache to accept the expected resolved commit and compare it with the marker contents; in src/install/PackageInstall.rs lines 2353-2362, validate the unpatched Git marker before setting PreinstallState::Done; in src/install/PackageInstall.rs lines 2379-2394, validate the base resolved commit for patched Git dependencies; and in src/install/isolated_install.rs lines 2373-2384, apply the same exact-marker check while preserving buffer restoration.src/install/isolated_install.rs (1)
2373-2384: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCompare
.bun-tagwith the resolved commit before marking the cache complete.
is_git_folder_in_cache_atonly checks marker existence. It must read.bun-tagand compare its contents with the resolved commit before settingPreinstallState::Done.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/install/isolated_install.rs` around lines 2373 - 2384, Update the ResolutionTag::Git cache-completeness check to read the existing `.bun-tag` contents and compare them with the resolved commit, rather than treating marker existence alone as sufficient. Return true only when the marker matches the resolved commit, so callers set PreinstallState::Done only for the correct checkout.Source: Coding guidelines
♻️ Duplicate comments (1)
src/install/repository.rs (1)
980-998:⚠️ Potential issue | 🟠 MajorStop publication when checkout finalization fails.
dir.delete_tree(b".git"),.bun-tagcreation,write_all, andclosefailures are ignored. Execution still reaches the rename, so the final cache can contain.gitor a missing or partial.bun-tag. Close the directory, removetmp_name, and return the original error before the rename.As per coding guidelines, propagate I/O and cleanup errors instead of swallowing them.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/install/repository.rs` around lines 980 - 998, Update the checkout finalization flow around dir.delete_tree, .bun-tag creation, write_all, and close to propagate each I/O failure instead of ignoring it or breaking insert_tag. On failure, close the directory, remove tmp_name, and return the original error before reaching the rename; preserve successful finalization and tag creation behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/install/isolated_install.rs`:
- Around line 2373-2384: Update the ResolutionTag::Git cache-completeness check
to read the existing `.bun-tag` contents and compare them with the resolved
commit, rather than treating marker existence alone as sufficient. Return true
only when the marker matches the resolved commit, so callers set
PreinstallState::Done only for the correct checkout.
In `@src/install/PackageManager/PackageManagerDirectories.rs`:
- Around line 758-769: Use one exact .bun-tag validation contract for every Git
cache hit: in src/install/PackageManager/PackageManagerDirectories.rs lines
758-769, update is_git_folder_in_cache to accept the expected resolved commit
and compare it with the marker contents; in src/install/PackageInstall.rs lines
2353-2362, validate the unpatched Git marker before setting
PreinstallState::Done; in src/install/PackageInstall.rs lines 2379-2394,
validate the base resolved commit for patched Git dependencies; and in
src/install/isolated_install.rs lines 2373-2384, apply the same exact-marker
check while preserving buffer restoration.
---
Duplicate comments:
In `@src/install/repository.rs`:
- Around line 980-998: Update the checkout finalization flow around
dir.delete_tree, .bun-tag creation, write_all, and close to propagate each I/O
failure instead of ignoring it or breaking insert_tag. On failure, close the
directory, remove tmp_name, and return the original error before reaching the
rename; preserve successful finalization and tag creation behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0680b56b-cf97-4188-a19b-21577949c080
📒 Files selected for processing (4)
src/install/PackageInstall.rssrc/install/PackageManager/PackageManagerDirectories.rssrc/install/isolated_install.rssrc/install/repository.rs
|
On the two review points that could not be posted inline: Exact Propagating finalization failures in |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/install/PackageManager/PackageManagerDirectories.rs`:
- Around line 767-774: Update is_git_folder_in_cache_at in
src/install/PackageManager/PackageManagerDirectories.rs:767-774 to accept the
expected resolved commit and validate that .bun-tag exactly matches it, rather
than checking existence only. Update the callers in
src/install/PackageInstall.rs:2353-2358 and :2373-2378, and
src/install/isolated_install.rs:2373-2378, to pass the expected Git resolved
commit; use the commit with the patch suffix removed at
PackageInstall.rs:2373-2378.
In `@src/install/repository.rs`:
- Around line 985-1007: Preserve the underlying error from both create_file_z
and write_all in the tag_written handling block. Update the log.add_error_fmt
call to include the captured I/O error alongside the .bun-tag failure context,
while retaining the existing cleanup and InstallFailed return behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 01883745-c206-4817-b10b-fb5dad8ae808
📒 Files selected for processing (5)
src/install/PackageInstall.rssrc/install/PackageManager/PackageManagerDirectories.rssrc/install/isolated_install.rssrc/install/repository.rstest/cli/install/bun-install.test.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/install/repository.rs (3)
980-980: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winFail closed on completion errors.
Handle errors from
dir.delete_tree(b".git")andgit_tag.close()before publishing the temporary directory. If either operation fails, remove the temporary tree and return the installation error instead of marking the cache entry complete.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/install/repository.rs` at line 980, Update the temporary-directory completion flow around dir.delete_tree and git_tag.close to explicitly handle both errors before publishing the directory. On either failure, remove the temporary tree, return the installation error, and avoid marking the cache entry complete; only publish after both operations succeed.Sources: Coding guidelines, MCP tools
950-950: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReport temporary-tree cleanup failures.
delete_treecan stop on filesystem errors and leave part oftmp_namebehind. Centralize thedelete_tree(tmp_name)calls, report cleanup failures with the temporary path, and preserve the original operation error. Also report cleanup failures after a successful rename.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/install/repository.rs` at line 950, Update the temporary-tree cleanup flow around all delete_tree(tmp_name) calls to centralize cleanup, report any deletion failure with the temporary path, and preserve the original operation error when cleanup also fails. Apply the same failure reporting after a successful rename, using the existing repository operation context rather than discarding delete_tree results.Source: Coding guidelines
907-923: 🩺 Stability & Availability | 🔵 TrivialRemove orphaned temporary repository checkouts.
The cache initialization path does not remove
tmpsiblings, andcached_checkout_is_completeonly checks the final folder. Add bounded cleanup for interrupted repository checkouts. Use a dedicated name or ownership lock so active checkouts and unrelated temporary entries are not deleted.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/install/repository.rs` around lines 907 - 923, Add bounded cleanup for orphaned temporary repository checkout siblings in the cache initialization flow around the temporary name creation and cached_checkout_is_complete logic. Use a dedicated checkout-temp naming convention or ownership lock to identify only abandoned checkouts, preserve active ones, avoid deleting unrelated temporary entries, and retain the existing atomic rename behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/install/repository.rs`:
- Line 980: Update the temporary-directory completion flow around
dir.delete_tree and git_tag.close to explicitly handle both errors before
publishing the directory. On either failure, remove the temporary tree, return
the installation error, and avoid marking the cache entry complete; only publish
after both operations succeed.
- Line 950: Update the temporary-tree cleanup flow around all
delete_tree(tmp_name) calls to centralize cleanup, report any deletion failure
with the temporary path, and preserve the original operation error when cleanup
also fails. Apply the same failure reporting after a successful rename, using
the existing repository operation context rather than discarding delete_tree
results.
- Around line 907-923: Add bounded cleanup for orphaned temporary repository
checkout siblings in the cache initialization flow around the temporary name
creation and cached_checkout_is_complete logic. Use a dedicated checkout-temp
naming convention or ownership lock to identify only abandoned checkouts,
preserve active ones, avoid deleting unrelated temporary entries, and retain the
existing atomic rename behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b6bac920-abb4-4d1f-b980-334e16c3dedd
📒 Files selected for processing (1)
src/install/repository.rs
|
On the three outside-diff comments from the latest pass:
|
There was a problem hiding this comment.
No issues found in this pass — my earlier finding (the patched-git branch of package_missing_from_cache still using bare directory_exists_at) was addressed in 905e322, and the CodeRabbit / comment-cop threads are all resolved. Deferring to a human because this restructures Repository::checkout and the git cache-completeness contract across four install-path call sites, which is more than I'd approve unilaterally.
What was reviewed:
renameat_concurrently_ahandles the existing-stale-folder case (NoReplace → Exchange → delete+rename); the trailingdelete_tree(tmp_name)cleans up after an Exchange swap.move_fallback: falseis correct since tmp and destination are siblings in the cache dir.- All four
.bun-tagpresence probes (determine_preinstall_state×2,package_missing_from_cache×2, isolated linker) now route through the sharedis_git_folder_in_cache_athelper. .bun-tagwrite failures now fail the checkout with the underlying errno and remove the temp tree;diris closed before the rename for Windows.
Extended reasoning...
Overview
The PR fixes silent empty-package resolution when a git dependency's per-commit cache folder (@G@<sha>) is left behind by a killed or failed git clone --no-checkout / git checkout. The fix has two parts: (1) Repository::checkout now builds the worktree in a temporary sibling and publishes it via renameat_concurrently_a only after .bun-tag is written, so an interrupted install can never leave a partial folder at the trusted name; (2) the four preinstall cache probes (hoisted + isolated installers, determine_preinstall_state for both patched and un-patched git resolutions) now require .bun-tag presence rather than bare directory existence, so already-poisoned caches heal on the next install. A new helper pair is_git_folder_in_cache{,_at} centralizes the probe. A ~140-line test spins up a local dumb-HTTP git server and exercises the empty-folder, valid-reuse, mismatched-tag, and lockfile-present paths for both linkers.
Security risks
None identified. resolved is already validated by the pre-existing is_safe_resolved_tag guard (rejects empty, leading -, separators, .., NUL) before it is used as a path component or git argument, and the temp name comes from FileSystem::tmpname. The test uses a local Bun.serve({ port: 0 }) static server, no external network.
Level of scrutiny
High. This is production-critical package-manager code: the cache-completeness contract for git dependencies is changed at four call sites plus the checkout writer, and Repository::checkout is substantially restructured (~85 lines rewritten) to introduce the temp-then-rename flow. The fix is well-motivated (real CI failure) and follows the same publish pattern the tarball extractor already uses, but the surface area and cross-platform rename semantics warrant a human look.
Other factors
I previously flagged the missed patched-git sibling in package_missing_from_cache; that was fixed in 905e322 and is covered in the current diff. Subsequent bot rounds tightened the .bun-tag write to fail the checkout on error (a7057ba) and include the errno in the message (d5125ad). The CodeRabbit suggestion to compare .bun-tag contents at the preinstall probes was reasonably declined — the folder name is derived from resolved and the tag is written last before an atomic rename, so presence is the completion signal; content comparison lives in cached_checkout_is_complete at the layer that rebuilds. I verified renameat_concurrently handles an existing destination via Exchange or delete+rename, which the incomplete-leftover-rebuild path relies on. move_fallback: false is safe because source and destination share the cache directory. All review threads on the PR are marked resolved.
|
Updated 10:47 AM PT - Aug 7th, 2026
✅ @robobun, your commit 5b4ac3e91f8512cea6b8093f5031f6f8200ac368 passed in 🧪 To try this PR locally: bunx bun-pr 37145That installs a local version of the PR into your bun-37145 --bun |
The per-URL bare mirror (<hash>.git) had the same non-atomic creation as the per-commit checkout folder: git clone --bare wrote directly to the final name, and the cache-hit path trusted any existing folder. A clone killed partway left a folder that makes every later git fetch fail (or, if the kill landed after init, a structurally valid repository whose refs never heal), permanently, because the folder name is a stable hash of the URL. Clone the mirror into a temporary sibling and rename it into place, and on a cache hit rebuild the mirror when it is missing HEAD, objects/ or refs/ (the layout git writes before transferring data). A structurally complete mirror that fails to fetch is kept as before, so transient network failures do not discard a usable cache. Also cover the git checkout failure path in the test: a checkout that fails must report the error and leave neither a cache folder nor temporary junk behind.
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 (1)
src/install/repository.rs (1)
949-963: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy liftDelete the incomplete checkout folder before you rebuild it.
Line 956 closes the incomplete directory but leaves it at
folder_name. The rebuild renames the temporary checkout onto that name at Lines 1076-1084. If the leftover directory is not empty, the rename fails and the code returnsError::InstallFailed. The mismatched.bun-tagfolder then stays in the cache, so the recovery this PR adds never completes. This is the same root cause as the bare-mirror path indownload.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/cli/install/bun-install.test.ts`:
- Around line 5425-5427: Update the failed-install assertion around the
Promise.all result to bind the process stdout value as out instead of discarding
it, then include out alongside err and exitCode in the nonzero-exit assertion
while preserving the existing error-message check.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0a9a8d6f-da62-4b0b-8460-e6634060b780
📒 Files selected for processing (2)
src/install/repository.rstest/cli/install/bun-install.test.ts
|
On the outside-diff comment claiming the incomplete checkout folder must be deleted before the rebuild: the rename helper used here ( |
There was a problem hiding this comment.
My earlier note on the patch.is_some() branch of package_missing_from_cache is addressed — it now gates on resolution::Tag::Git and routes through is_git_folder_in_cache_at. No further findings this pass, but this reworks git-dependency cache population enough (temp-then-rename in both download and checkout, new .bun-tag completeness probes at four call sites, Windows close-before-rename ordering, several new error/cleanup paths) that a human should sign off.
Also checked: renameat_concurrently_a handles the non-empty destination on the rebuild path (same helper the tarball extract uses); the patch_hash.is_some() branch in determine_preinstall_state correctly falls back to is_folder_in_cache since the patched folder is produced by the patch step, not checkout, and has no .bun-tag; the test's dumb-HTTP git server stays local and hermetic.
Extended reasoning...
Overview
The PR makes git-dependency cache population atomic and self-healing. Repository::download (bare mirror) and Repository::checkout (per-commit worktree) now build into a temporary sibling and renameat_concurrently_a it into place only after everything is written, and both validate an existing folder before trusting it (HEAD/objects/refs for the mirror, .bun-tag == resolved for the checkout). A new is_git_folder_in_cache[_at] helper checks .bun-tag presence, and four preinstall/cache probe sites (determine_preinstall_state × 2, package_missing_from_cache × 2, isolated_install) now use it for ResolutionTag::Git. .bun-tag write failure now fails the checkout instead of publishing an incomplete folder. A ~190-line test drives all scenarios against a local dumb-HTTP git server.
My prior inline finding (the patch.is_some() branch of package_missing_from_cache still checking bare directory existence) is fixed in the current diff.
Security risks
None material. resolved is already gated by the pre-existing is_safe_resolved_tag before it reaches git checkout or the folder name. The temp name comes from FileSystem::tmpname, the same helper the tarball extract path uses. The test server is local (Bun.serve({port:0})) and only serves paths under the bare repo directory.
Level of scrutiny
High. This is core bun install behavior for git dependencies, with several correctness-sensitive concerns: atomic publish vs. concurrent installs, error-path cleanup on every early return, Windows directory-handle constraints (close before rename), and the design choice to probe .bun-tag presence at the preinstall sites vs. content at the resolve site. Those tradeoffs were argued in the thread and look right, but they're the kind of decisions a maintainer should confirm.
Other factors
All CodeRabbit and comment-cop threads are resolved; the author addressed or reasoned-declined each. CI build #90176 was still running at the last timeline event, so test results aren't confirmed yet. The change is well-tested (empty cache folder, mismatched tag, lockfile-present hoisted + isolated linkers, incomplete bare mirror, failing checkout leaving no debris) but the surface area — six files, ~350 lines of production install logic across multiple error paths — is beyond what I'd approve without a human look.
|
Replaced by #38274 (rebased onto current main; same staging+rename, cache-hit marker check unified into one helper shared by the resolve-time hit and both installers). |
|
Sounds good, thanks. The staging plus rename and the shared marker check in #38274 cover everything this PR addressed (the per-commit checkout folder, the bare mirror, and the hoisted and isolated preinstall probes). |
…fix bun patch for non-npm deps and isolated hang (#38269) ### What does this PR do? Install-cache and `bun patch` robustness, consolidated from #37124, #37136 and #37145 (rebased and reshaped; #32749 from the same batch landed separately). **Git dependency cache folders are built in a staging dir and renamed on success.** `Repository::checkout` cloned straight into `<cache>/@g@<sha>` and checked out in place; `Repository::download` cloned the bare mirror straight into `<cache>/<hash>.git`. An install (or its git child — seen OOM-killed in CI) dying between steps left a folder at the trusted name: an empty `@G@` folder resolves as an *empty package* through the "git dependency without package.json" path (exit 0, `bun.lock` name falls back to the URL basename), and a half-cloned mirror fails every later `git fetch`. Both now build under a temporary sibling inside the cache dir (`CacheStaging`, same-filesystem so it's the same `renameat_concurrently` ladder tarball extraction uses) and are renamed into place only when complete; failures remove the staging dir. **Cache hits require the entry's completion marker.** One helper, `is_package_in_cache_at(cache_dir, folder, tag)`: npm folders must contain `package.json`, git checkouts must contain the `.bun-tag` written last, everything else stays a directory probe. Used by `checkout()`'s resolve-time hit, `determine_preinstall_state`, and the hoisted and isolated installers — previously every git hit was a bare directory probe (`.bun-tag` in the cache was written but never read), and `determine_preinstall_state` didn't probe `package.json` for npm either. Folders left by older versions are re-cloned instead of installed. Deletes the hoisted installer's unsafe in-place edit of the shared folder-name buffer and the isolated installer's append/truncate copy. Since the tag is now the marker, `checkout()` unlinks anything a repo checked in as `.bun-tag`, creates it `O_EXCL|O_NOFOLLOW`, and fails the checkout rather than publish an untaggable folder (a repo shipping a symlink named `.bun-tag` now gets a real tag; its target is still never written — test updated). Existing bare mirrors are not validated structurally; there's no marker for them. **`bun patch --commit` on git, github and tarball dependencies** failed with `Could not access '.../@gh@@@@1'` and wrote nothing (#18792, #17945): it loads its own lockfile but computed the cache path against the empty `manager.lockfile`. The loaded lockfile is moved into the manager before the path is computed (`install_with_manager` reloads it afterwards, as it already does for `bun update`; the double parse is left alone). Patch filenames additionally escape NTFS-reserved characters, which only these resolutions contain. Fixes #18792, fixes #17945. **Isolated linker hang** — a plain `bun install` in a workspace with a `patchedDependencies` entry for a git/github dependency hung forever: the installer treated every patched package as missing and re-enqueued a download the resolve phase had already completed, parking on a drained task list. It now probes the unpatched folder (computed with no patch hash) like everything else; also fixes removing an entry. **Tests:** github/git/tarball `patch --commit` flows; add → cold cache → remove → re-add of a patch under isolated for github, git and npm (the suspected stale-`.bun-tag-<hash>` skip on re-add did not reproduce, so these just pin the cycle); git checkout failure leaves only the mirror in the cache and an empty folder at the cache name is re-cloned; a pre-existing test pinned to the bogus `--commit` cache path now asserts the step that genuinely fails. ### How did you verify your code works? `bun-install` + `bun-install-registry` (427), `isolated-install` (65), `bun-install-patch`, `bun-patch` all pass locally; new tests fail on release bun (half-built `@G@` folder left behind; `--commit` error; isolated hang). Each fix was also driven by hand with the debug binary: `patch --commit` on a `file:` tarball fails on main's binary and succeeds here; a workspace with a patched local git dep installs, requires as patched, survives remove and re-add, and re-clones an emptied cache folder, with no `.tmp` residue. Windows rename/escaping legs are left to CI. Local gotcha: these files need `HOME` pointed at an empty dir if `~/.npmrc` sets `install-strategy=hoisted`. --- **Added after CI** (`25855c0fe97`): the npm add/remove/re-add test failed on Linux — the hardlink and copyfile backends overlay files onto an existing isolated store entry, so removing a patch kept every file the patched build had *added* (and its `.bun-tag-<hash>`); clonefile replaces the tree, which is why macOS passed and why the earlier "did not reproduce" was wrong — this is the staleness #37136 mentioned. The task now deletes the previous project-local package tree before rebuilding an entry (only reached when the entry needs a rebuild, so warm installs don't pay for it); the npm cycle test pins the hardlink backend. Also from review: `checkout()` uses `delete_tree` for a checked-in `.bun-tag`, so a directory under that name is replaced instead of failing the install (test added).
2 participants
What
If a
bun install(or thegitchild process it spawns) is killed between thegit clone --no-checkoutandgit checkoutsteps of caching a git dependency, the per-commit cache folder (<cache>/@G@<sha>) is left behind without its files. Every later install then trusts that folder, takes the "git dependencies without package.json" fallback, and silently resolves the dependency as an empty package: no files, no dependencies, exit code 0, and the package name inbun.lockfalls back to the URL basename (repo.git@git+http://host/repo.git#<sha>instead of the realpackage.jsonname).A plain
git checkoutfailure (non-zero exit) poisoned the cache the same way, because the folder was created by the clone step and only populated afterwards.The per-URL bare mirror (
<cache>/<hash>.git) had the same non-atomic creation: agit clone --barekilled partway leaves a folder that makes every latergit fetchfail ("git fetch" for "x" failed), permanently, because the folder name is a stable hash of the URL.This was observed in CI: a loaded machine OOM-killed the spawned git child (
error: "git checkout" for "branch-dep" failed/git failed with signal 9), and the retried install "succeeded" while writingbranch-dep.git@git+http://...#<sha>with an empty package intobun.lock.Cause
Repository::checkoutbuilt the cache folder in place, in three non-atomic steps:git clone --no-checkout <repo> <final-folder>, thengit checkout <sha>inside it, then write.bun-tag. Its cache-hit path (and the preinstall-state checks used when the resolution is already in the lockfile) accepted any existing folder without validating it, so a folder from an interrupted install was indistinguishable from a complete checkout.Repository::downloadcloned the bare mirror the same way and trusted any existing folder at the mirror name.Fix
Repository::checkoutnow clones and checks out into a temporary sibling inside the cache directory and renames it onto@G@<sha>(via the same concurrent-rename helper the tarball extract path uses) only after the worktree,.gitremoval, and.bun-tagare all done. A kill at any point can no longer leave a half-built folder at the trusted name, and a failed checkout cleans up after itself.checkoutverifies that.bun-tagmatches the resolved commit and rebuilds the folder otherwise. This heals caches that are already poisoned.determine_preinstall_state) now require.bun-tagfor git resolutions instead of bare folder existence, so with an existing lockfile a poisoned folder is checked out again rather than copied intonode_modulesas an empty package..bun-taghas been written by every bun version, so existing healthy caches are unaffected.Repository::downloadclones the bare mirror into a temporary sibling and renames it into place the same way, and on a cache hit rebuilds the mirror when it is missingHEAD,objects/orrefs/(the layout git writes before transferring data). A structurally complete mirror that fails to fetch is kept as before, so transient network failures do not discard a usable cache..bun-tagpresence is the right marker because git checkouts may legitimately have nopackage.json; the tag is written as the last step of populating the folder.Verification
New test in
test/cli/install/bun-install.test.ts(local dumb-HTTP git server, no network) covering: an empty cache folder for the resolved commit (what a killed install leaves), a valid folder being reused without re-cloning, a folder with a mismatched.bun-tagbeing rebuilt, the lockfile-present install path for both the hoisted and isolated linkers, an empty bare mirror being rebuilt instead of failinggit fetchforever, and a failinggit checkoutreporting the error while leaving neither a cache folder nor temporary junk behind.Existing git dependency tests in
bun-install.test.ts(18),isolated-install.test.ts(62),bun-install-patch.test.ts(18),bun-patch.test.ts(31), andbun-lock.test.ts(17) pass.bun run rust:check-allclean on all 10 targets.no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/install/bun-install.test.ts