Conversation
… collides on Windows
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 48 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughHardlink installation now handles concurrent filesystem races on Windows and Unix with bounded retries, exponential backoff, file identity checks, and destination directory reopening. Windows cache moves distinguish complete and incomplete entries. Concurrent-install tests cover warm- and cold-cache behavior. ChangesConcurrent installation handling
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}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
src/install/PackageInstall.rs (1)
1755-1774: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPropagate destination recovery errors.
Handle errors from
make_open_pathandMakePath::make_pathexplicitly. Retry only for expected concurrent-rename errors. Return permission, I/O, path, and other recovery errors instead of retrying with a stale directory and reporting the earlierlinkaterror.🤖 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/PackageInstall.rs` around lines 1755 - 1774, Update the destination recovery flow around destbase.make_open_path and bun_sys::MakePath::make_path to handle their Result values explicitly: retry only when the error represents the expected concurrent-rename condition, and immediately propagate permission, I/O, path, or other errors. Do not continue using the stale destination_dir or report the earlier linkat error after an unrecoverable recovery failure.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/PackageInstall.rs`:
- Around line 1755-1774: Update the destination recovery flow around
destbase.make_open_path and bun_sys::MakePath::make_path to handle their Result
values explicitly: retry only when the error represents the expected
concurrent-rename condition, and immediately propagate permission, I/O, path, or
other errors. Do not continue using the stale destination_dir or report the
earlier linkat error after an unrecoverable recovery failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 61607c84-910f-411b-a4d7-ff8b537a1864
📒 Files selected for processing (1)
src/install/PackageInstall.rs
|
CI status on 9e99ab2: the new concurrent-install test passes on every lane, including the macOS lanes running the clonefile arm. The remaining failures are unrelated to this diff: the AsyncLocalStorage-tracking ASAN leak (pre-existing on main, reported separately) and four tests that passed on retry. Ready for review. |
There was a problem hiding this comment.
No new issues found; my earlier notes (isolated Hardlinker same-inode acceptance, symlink retry target, copyfile sibling deferral) have all been addressed or documented. Deferring to a human reviewer given the scope: this rewrites concurrent-install race handling across four backends (Windows/POSIX hardlink, macOS clonefile, isolated linker) plus the Windows extract-publish semantics, with bounded retry loops and dirfd re-open recovery that are hard to fully validate without cross-platform CI results.
What was reviewed:
- Retry loops are bounded (4/6 iterations) and backoff shifts can't overflow; first retries stay immediate so single-process EEXIST isn't slowed.
same_file_wuses attribute-only opens (0 access, all share bits) so it works on exclusively-held files; handle is closed on both success and query failure.- Windows extract-publish now keeps a complete existing cache entry instead of deleting it out from under peers;
dir_to_moveis closed on every branch before the loop continues or errors. - POSIX hardlink ENOENT arm re-opens from
dest_dirand recreates the entry's parent path before retrying; non-transient re-open errors propagate rather than looping.
Extended reasoning...
Overview
~350 lines of changes across src/install/PackageInstall.rs, src/install/extract_tarball.rs, src/install/isolated_install/Hardlinker.rs, src/sys/windows/mod.rs, and a new concurrent-install stress test. The core change: when N bun install processes race on the same packages/destination/cache, treat a peer's already-correct output (same file object / same inode) as success rather than a conflict to destroy, and retry transient EBUSY/ENOENT with bounded backoff. Also changes the Windows tarball-extract publish to keep an existing complete cache entry rather than rename-and-delete it.
Security risks
None identified. No new user-controlled input reaches these paths beyond what already did; the new same_file_w helper is a read-only identity check via GetFileInformationByHandle. The "keep existing cache entry" branch only fires on a rename collision and still replaces entries lacking package.json, so it doesn't newly accept incomplete/corrupt cache state.
Level of scrutiny
High. This is the core bun install file-materialization path across every platform, with per-platform #[cfg] branches that only get exercised on their own CI lanes. The correctness argument rests on a concurrency invariant ("all racers install identical content, so an existing file is the desired end state") that's sound for the motivating bunx scenario but is a semantic change for the Windows cache-publish path (previously replaced, now kept). The retry loops introduce sleeps on the install hot path (only after 2 immediate retries on POSIX, but still). The new test spawns 30+ subprocesses with a 120s timeout — potential CI-time and flakiness concerns a maintainer should weigh.
Other factors
All three of my earlier inline notes were addressed in follow-up commits (c99bfd9, 06c4eae, plus PR description updates for the deferred copyfile sibling). The PR description is thorough, cites soak-test numbers on both platforms, and rust:check-all passed. The test asserts both file count and content per package after each round. Given the cross-platform surface and the design decisions embedded here (retry counts, which errnos are treated as transient, keep-vs-replace on cache collision), a human maintainer should sign off.
|
This PR has been closed because it was flagged as AI slop. Many AI-generated PRs are fine, but this one was identified as having one or more of the following issues:
If you believe this was done in error, please leave a comment explaining why. |
github-actions
Bot
changed the title
|
Responding to the flag with the verification record, since each listed criterion was covered during development: The problem was verified before any fix. The starting point was CI build 89146, where two napi suites running The fix was tested in both directions. The new test in Assumptions were checked against the codebase. The convergence design follows the same reasoning already merged for the POSIX extract path discussion in #36227/#36229 (an existing cache entry or destination file produced by a concurrent install of identical content is the desired end state), and the retry/backoff shape mirrors the existing cache-move retries in The review bots on this PR raised several findings (isolated linker, symlink retry target, clonefile backend, recovery-error propagation); each was either fixed in a follow-up commit or explicitly documented. Happy to address anything further, and leaving reopening to a maintainer's judgment. |
2 participants
This PR has been marked as AI slop and the description has been updated to avoid confusion or misleading reviewers.
Many AI PRs are fine, but sometimes they submit a PR too early, fail to test if the problem is real, fail to reproduce the problem, or fail to test that the problem is fixed. If you think this PR is not AI slop, please leave a comment.