Conversation
|
Updated 2:31 AM PT - Aug 22nd, 2026
❌ @robobun, your commit 3cc2929 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 29496That installs a local version of the PR into your bun-29496 --bun |
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
|
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:
WalkthroughReplaced a mutable global install method with an atomic-backed accessor API and updated call sites. Added a POSIX-only thread-pool parallel hoisted-install path with scheduling and completion/drain, cache-miss reroute to serial installs, extracted shared result handling, a serial-hoisted env escape hatch, tests, and minor docs formatting fixes. Changes
🚥 Pre-merge checks | ✅ 2✅ 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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/install/PackageInstaller.zig`:
- Around line 687-691: The check in canUseParallelHoistedInstall currently calls
bun.getenvZ("BUN_INSTALL_SERIAL_HOISTED") directly; replace this with the
repository's cached, type-safe env-var accessor by adding/using
bun.env_var.BUN_INSTALL_SERIAL_HOISTLED (or the correct accessor name following
the existing pattern) and call its .get() (or .getBool() if available) in
canUseParallelHoistedInstall instead of bun.getenvZ; update the accessor
definition in the bun.env_var declarations if it doesn't exist so the code reads
the env var via bun.env_var.BUN_INSTALL_SERIAL_HOISTED.get() and returns false
when set.
- Around line 1535-1540: handleInstallResult() currently only treats trust from
trusted_dependencies_from_update_requests and lockfile.hasTrustedDependency(...)
as trusted, which drops newly added trust entries in
manager.summary.added_trusted_dependencies and causes installed packages to miss
persisting trust. Update the trust computation (where is_trusted /
is_trusted_through_update_request is set around dependency_id and
truncated_dep_name_hash) to also consider
this.manager.summary.added_trusted_dependencies (or
this.manager.summary.added_trusted_dependencies.contains(truncated_dep_name_hash))
so that packages newly marked trusted in added_trusted_dependencies follow the
same success path and persist; ensure both the skipped-install and
install-success branches check the same combined-trust predicate used by
handleInstallResult().
- Around line 143-149: The code currently sets self.missing_from_cache whenever
pi.install() fails with step == .opening_cache_dir; change it to only set
self.missing_from_cache when the failure reason actually indicates a cache miss
(e.g., ENOENT / “not found” or the corresponding error enum on the Result
object) rather than any .opening_cache_dir error; update the conditional that
checks result.isFail() && result.failure.step == .opening_cache_dir to
additionally inspect result.failure's error/code (or errno) and only flip
self.missing_from_cache for the specific "not found" error, leaving other errors
to propagate to completeParallelInstalls()/packageMissingFromCache() unchanged.
In `@test/cli/install/parallel-hoisted-install.test.ts`:
- Around line 72-76: The test file is missing the required 5-minute default
timeout for install CLI tests; add a top-level beforeAll that calls
setDefaultTimeout(1000 * 60 * 5) before other setup (i.e., place it near the top
of the "parallel hoisted install" suite, above the existing beforeAll that calls
makeTarballFixture) so the suite uses the 5-minute timeout for installs.
- Around line 50-58: fingerprintNodeModules currently returns only relative
paths, so change the function to record each entry's type and content signature:
for each entry from Glob.scan (using the same cwd: dir), lstat the entry to
determine if it's a file, directory, or symlink; for files compute a content
hash (e.g., SHA-256 of fs.readFile), for symlinks record fs.readlink target, and
for directories record a distinct marker (e.g., "dir"); return/sort strings
combining the relative path, the entry type, and the hash/target marker so tests
compare actual contents and symlink targets, not just paths.
🪄 Autofix (Beta)
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: 0cf5c47b-f899-4f05-a8e6-7395e8caaccb
📒 Files selected for processing (6)
src/install/PackageInstall.zigsrc/install/PackageInstaller.zigsrc/install/PackageManager/PackageManagerOptions.zigsrc/install/hoisted_install.zigsrc/install/isolated_install.zigtest/cli/install/parallel-hoisted-install.test.ts
There was a problem hiding this comment.
♻️ Duplicate comments (2)
test/cli/install/parallel-hoisted-install.test.ts (2)
72-76:⚠️ Potential issue | 🟡 MinorAdd the standard 5-minute timeout for install CLI tests.
This suite performs repeated install/cache operations; default timeout is too tight and causes flaky harness failures.
Proposed fix
-import { afterAll, beforeAll, describe, expect, test } from "bun:test"; +import { afterAll, beforeAll, describe, expect, setDefaultTimeout, test } from "bun:test"; describe.skipIf(!isPosix)("parallel hoisted install", () => { + beforeAll(() => { + setDefaultTimeout(1000 * 60 * 5); + }); + let fixture: { dir: string; deps: Record<string, string>; count: number };As per coding guidelines: “For install CLI tests under
test/cli/install/… set a 5-minute default timeout usingbeforeAll(() => setDefaultTimeout(1000 * 60 * 5)).”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/cli/install/parallel-hoisted-install.test.ts` around lines 72 - 76, The test suite "parallel hoisted install" currently initializes a fixture in beforeAll but lacks the 5-minute Jest timeout; add a call to setDefaultTimeout(1000 * 60 * 5) at the start of the suite (e.g., in a beforeAll block before calling makeTarballFixture()) so the describe.skipIf(!isPosix) block uses the extended timeout; locate the suite by describe.skipIf and the beforeAll that calls makeTarballFixture() and insert the setDefaultTimeout call there.
50-58:⚠️ Potential issue | 🟠 MajorFingerprint is too weak for a byte-identical claim.
Line 50 currently fingerprints only entry paths. That can pass even when file bytes or symlink targets differ, so the serial-vs-parallel parity check is incomplete.
Proposed fix
-import { mkdir, rm } from "fs/promises"; +import { lstat, mkdir, readFile, readlink, rm } from "fs/promises"; +import { createHash } from "crypto"; async function fingerprintNodeModules(dir: string): Promise<string[]> { const entries: string[] = []; const glob = new Glob("node_modules/**/*"); for await (const entry of glob.scan({ cwd: dir, onlyFiles: false, dot: true, followSymlinks: false })) { - entries.push(entry); + const abs = join(dir, entry); + const st = await lstat(abs); + if (st.isSymbolicLink()) { + entries.push(`symlink:${entry}:${await readlink(abs)}`); + continue; + } + if (st.isDirectory()) { + entries.push(`dir:${entry}`); + continue; + } + if (st.isFile()) { + const hash = createHash("sha256").update(await readFile(abs)).digest("hex"); + entries.push(`file:${entry}:${hash}`); + continue; + } + entries.push(`other:${entry}`); } entries.sort(); return entries; }
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/cli/install/parallel-hoisted-install.test.ts`:
- Around line 72-76: The test suite "parallel hoisted install" currently
initializes a fixture in beforeAll but lacks the 5-minute Jest timeout; add a
call to setDefaultTimeout(1000 * 60 * 5) at the start of the suite (e.g., in a
beforeAll block before calling makeTarballFixture()) so the
describe.skipIf(!isPosix) block uses the extended timeout; locate the suite by
describe.skipIf and the beforeAll that calls makeTarballFixture() and insert the
setDefaultTimeout call there.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3c83c81c-926d-4a87-b190-03a0726a6a1c
📒 Files selected for processing (2)
docs/runtime/bunfig.mdxtest/cli/install/parallel-hoisted-install.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/install/PackageInstaller.zig`:
- Around line 67-76: Worker-install code opens directories using bun.FD.cwd()
which ignores the intended install root; update the worker path logic in the
PackageInstaller worker code (e.g., runFromThreadPool and any helper that opens
node_modules, cache_dir, or destination dirs) to resolve and open paths relative
to the instance's root_node_modules_folder / node_modules_path fields instead of
bun.FD.cwd(); specifically, use the PackageInstaller's root_node_modules_folder
(or node_modules_path) to call openDir/openFile for node_modules,
destination_dir_subpath and cache_dir_subpath so worker tasks create/link
packages inside the same root as the serial installer (apply same change to the
other occurrences referenced around lines 107-114 and 1222-1233).
In `@test/cli/install/parallel-hoisted-install.test.ts`:
- Around line 137-140: The countFrom helper currently mismatches the install
summary text; update countFrom (used in parallel-hoisted-install.test.ts) to
parse the actual installer output format by matching the "Installed N package"
pattern (e.g. /Installed\s+(\d+)\s+package/ with optional plural handling) so it
returns the numeric N from both serial.stdout and parallel.stdout and then
assert equality with fixture.count.
🪄 Autofix (Beta)
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: 8ebe76c5-3828-4d05-b55d-ff890c1f9484
📒 Files selected for processing (3)
src/env_var.zigsrc/install/PackageInstaller.zigtest/cli/install/parallel-hoisted-install.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/install/PackageInstaller.zig`:
- Around line 1527-1538: The function handleInstallResult currently accepts an
unused parameter destination_dir_subpath (it is immediately discarded with `_ =
destination_dir_subpath;` and alias is derived from the lockfile instead);
remove destination_dir_subpath from the handleInstallResult signature and all
its call sites, and adjust any callers to stop passing that argument so the
parameter list matches the new signature (refer to handleInstallResult and
places that invoke it to update them consistently).
🪄 Autofix (Beta)
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: 0fca6f00-b317-4c1b-861c-83f3ad6e0baf
📒 Files selected for processing (1)
src/install/PackageInstaller.zig
There was a problem hiding this comment.
All bugs filed during review were addressed in commits feeb7ab and 98ad064; the remaining added_trusted_dependencies asymmetry is a confirmed pre-existing issue not introduced by this PR. The parallelization logic looks correct. This is complex threading work in the critical install path, so routing to a human reviewer.
Extended reasoning...
Overview
This PR fans hoisted node_modules hardlinking from a serial main-thread loop onto the existing thread pool. It introduces ParallelHoistedTask, parallel_wait_group / parallel_batch coordination, an atomic replacement for the previously-mutable supported_method global, cache-miss re-routing from workers, and a new test suite. Eight files are touched: docs, env_var, PackageInstall, PackageInstaller, PackageManagerOptions, hoisted_install, isolated_install, and a new test file.
Security Risks
No auth, crypto, or permission-elevation concerns. The main risks are concurrency correctness (use-after-free on parallel task structs, data races on shared installer fields). All three UAF / race issues I filed were fixed: package_name is now captured at task-creation time, deinit() drains in-flight workers before freeing task structs, and the cache-miss guard uses the narrower isPackageMissingFromCache() predicate.
Level of Scrutiny
High. This is not a config change — it is new concurrent code in the hoisted install path, which runs on every bun install. Correctness bugs here silently corrupt node_modules. The threading model (ThreadPool.Batch, WaitGroup, atomic method selection, main-thread re-routing for cache misses) is non-trivial and deserves eyes from someone familiar with the broader install pipeline before merging.
Other Factors
- All six inline bug comments I submitted are marked resolved; robobun addressed each one with either a code fix or a technically-sound explanation.
- The
added_trusted_dependenciesasymmetry inhandleInstallResult()is a pre-existing latent bug confirmed byte-identical to the original.successarm onmain; fixing it is correctly scoped out of this PR. - The unused
destination_dir_subpathparameter was cleaned up in 98ad064. - Build CI showed one failure on an earlier commit (6c2ca81); the subsequent fixes in feeb7ab and 98ad064 should resolve it, but CI status on the final commits should be confirmed before merge.
80cc16e to
adf0ea5
Compare
There was a problem hiding this comment.
All bugs identified in previous rounds have been fixed, but this PR introduces non-trivial concurrent logic (WaitGroup, BatchScheduling, atomic method selection) into a critical code path — worth a human look before merging.
Extended reasoning...
Overview
This PR fans out cached package hardlinking to the existing thread pool via a new ParallelHoistedTask struct, replacing a serial main-thread loop. It touches PackageInstaller.zig, PackageInstall.zig, hoisted_install.zig, isolated_install.zig, env_var.zig, PackageManagerOptions.zig, and adds a new integration test file. The ban-limits bump for std.fs.Dir (+3) and .stdDir() (+3) are a natural consequence of the new fd-management code.
Security Risks
No auth, crypto, or permission-checking code is modified. The primary risk is memory safety: workers accessing shared installer state concurrently. The previously identified UAF issues (worker reading installer.names at runtime, deinit() freeing tasks without waiting for in-flight workers) were both fixed. No new security risks are introduced.
Level of Scrutiny
This is high — the PR introduces new concurrency patterns (WaitGroup, atomic supported_method, thread-pool batch dispatch) into the package installer, which is performance-critical and correctness-critical infrastructure. The bug hunting system caught several non-trivial issues that were fixed, but concurrent code has subtle failure modes that are hard to find through static analysis alone. A human reviewer familiar with the existing installer architecture should validate the overall design.
Other Factors
All 9 bugs found across two review rounds have been addressed. The one remaining concern — the parallelism test potentially failing on cgroup-throttled runners — was acknowledged by the author as a known risk with a plan to address if it manifests. CI passed on all POSIX targets in build 46561. The PR is well-motivated (3.4x speedup on warm-cache CI installs) and the implementation follows the existing thread-pool patterns in the codebase.
45458bf to
ac0e2d8
Compare
ac0e2d8 to
2b9a710
Compare
There was a problem hiding this comment.
All automated review concerns have been resolved across multiple rounds of fixes, but this PR introduces significant parallelism into a critical hot path (800+ lines of new thread-pool scheduling, atomic operations, and fd management in PackageInstaller.zig) and warrants a human look before merging.
Extended reasoning...
Overview\nThis PR adds a ParallelHoistedTask fan-out to the hoisted node_modules linker, dispatching per-package hardlink operations to the existing thread pool instead of running serially on the main thread. It touches PackageInstaller.zig (~800+ net new lines), hoisted_install.zig, PackageInstall.zig (atomic supported_method), env_var.zig (new escape hatch), and adds a new test file. The scope is substantial.\n\n### Security risks\nNo authentication, crypto, or permission-boundary changes. The main risks are concurrency bugs (data races, use-after-free, fd leaks) rather than security vulnerabilities in the traditional sense — all of which were the subject of the prior automated review rounds.\n\n### Level of scrutiny\nThis is production-critical code on the hot path for every fresh bun install v1.3.13 (bf2e2ce)
- @lezer/common@1.3.0
- @lezer/cpp@1.1.3
- bun-tracestrings@github:oven-sh/bun.report#912ca63
- esbuild@0.21.5
- mitata@0.1.14
- peechy@0.4.34
- prettier@3.6.2
- prettier-plugin-organize-imports@4.3.0
- react@18.3.1
- react-dom@18.3.1
- source-map-js@1.2.1
- typescript@6.0.2
232 packages installed [684.00ms]. The change introduces real parallelism (thread pool, WaitGroup, atomic), which is notoriously difficult to get right. While all prior automated review bugs have been addressed (use-after-free on installer.names, broad cache-miss detection, deinit() without draining workers, fd leak in makePath fallback), the complexity and production criticality of this code warrants a human reviewer with deep knowledge of Bun's threading model.\n\n### Other factors\nAll 42 inline review comments across multiple review rounds are marked resolved. The bug-hunting system found 0 bugs in the current HEAD. The false-positive concerns about the bad force-push (commit 45458bf) were superseded within minutes by ac0e2d8, and the robobun bot confirmed the implementation is intact. The trusted_dependencies asymmetry concern was confirmed as pre-existing behavior unrelated to this PR. No outstanding unresolved reviewer threads remain.
8c738c4 to
8119ede
Compare
There was a problem hiding this comment.
No remaining issues from automated review, but this introduces thread-pool concurrency into the default bun install hoisted linker path — worth a human pass on the worker/main-thread data sharing and the completeParallelInstalls drain ordering before merge.
Extended reasoning...
Overview
This PR parallelizes hoisted node_modules linking on fresh installs by fanning per-package PackageInstall.install() calls out to the thread pool via a new ParallelHoistedTask. It touches src/install/PackageInstaller.zig (~500 net new lines: task struct, batch scheduling, wait-group drain, extracted handleInstallResult()), src/install/hoisted_install.zig (per-tree batch flush + post-loop drain with a nested runTasks closure), PackageInstall.zig (converts supported_method to an atomic), env_var.zig (escape hatch), plus a new test suite and ban-limits bumps.
Security risks
None identified. The change does not touch auth, network input parsing, or path handling beyond what the existing serial installer already does — workers call the same PackageInstall.install() with owned copies of paths derived from the lockfile. No new untrusted-input surfaces.
Level of scrutiny
High. This changes the default behavior of bun install on POSIX for the most common scenario (fresh node_modules, warm cache). It introduces shared state between the main thread and thread-pool workers: workers read installer.root_node_modules_folder, installer.lockfile, and write parallel_wait_group; the main thread concurrently runs runTasks() (which can call fixCachedLockfilePackageSlices()) between batch flushes. The PR's review history bears this out — earlier revisions had a UAF reading installer.names from workers, an fd leak in the makePath retry path, and deinit() freeing tasks before workers finished, all of which were fixed. The completeParallelInstalls() → re-enter installPackageWithNameAndResolution → nested sleepUntil/runTasks drain in hoisted_install.zig is a new control-flow path that interleaves with the existing pending-install drain below it.
Other factors
- The
handleInstallResult()extraction is a verbatim move of the pre-existing.success/.failureswitch arms (verified against the diff), so no behavior change there. - All prior automated findings (12+ inline comments across multiple revisions) have been addressed and resolved by the author.
- The new test compares parallel vs. serial
node_modulesfingerprints (paths + symlink targets + sizes) and exercises the missing-cache fallback; the cpu/wall parallelism assertion is gated off debug/ASAN/single-core but is inherently timing-based. - This is a performance-critical, user-facing default — the kind of change where a maintainer should sign off on the concurrency model and the gating conditions (
skip_delete, resolution-tag whitelist,needs_verify) rather than relying solely on automated review.
There was a problem hiding this comment.
No new issues found, but this introduces thread-pool concurrency to the default bun install hoisted linker path (new task struct, wait-group lifecycle, atomic backend selection, error-path drains) and the new test is still failing on 🐧 3.23 x64-baseline in the latest build — worth a human pass on the concurrency/ownership model before merging.
Extended reasoning...
Overview
This PR parallelizes the hoisted node_modules linker on POSIX when node_modules is being created fresh. It adds ~500 lines to src/install/PackageInstaller.zig (new ParallelHoistedTask struct, parallel_tasks/parallel_wait_group/parallel_batch state, scheduleParallelBatch(), completeParallelInstalls(), and an extracted handleInstallResult()), wires the drain into hoisted_install.zig, converts PackageInstall.supported_method to an atomic accessor pair, registers two new env vars, and adds a 260-line test plus ban-limits bumps.
Security risks
None identified. The change is local filesystem I/O (hardlinking from the bun cache into node_modules) with no new network, auth, or path-derivation logic — it reuses the existing PackageInstall.install() for the actual link work. No injection or traversal surface is introduced.
Level of scrutiny
High. This changes the default behavior of bun install on every POSIX fresh install — one of the most-exercised user-facing code paths. The change is fundamentally about concurrency: tasks own duped path buffers, share the installer's root_node_modules_folder fd and lockfile across threads, race on supported_method (now atomic), and rely on deinit() flushing/draining the wait group on every early-error return. The PR has already absorbed fixes for a UAF on installer.names, a UAF in deinit() on error paths, an fd leak in the makePath retry, and an over-broad cache-miss classification — all of which surfaced through bot review rather than the initial implementation. That history suggests the concurrency model deserves a careful human read.
Other factors
- CI: Build #49563 shows
test/cli/install/parallel-hoisted-install.test.tsfailing (code 1, marked new) on🐧 3.23 x64-baseline. The other failure (dev-and-prod.test.tson Windows aarch64) appears unrelated. - Test coverage: The new test asserts layout equivalence vs. the serial path, the cache-miss reroute, and a CPU/wall parallelism ratio (skipped on debug/ASAN/single-core). The deterministic
[ParallelHoistedInstall] N tasksmarker is a good guard against silent regression to serial. - Refactor risk:
handleInstallResult()is a verbatim extraction of the pre-existing.success/.failureswitch arms (confirmed in the thread), so the serial path's behavior should be preserved — but it's a large block move that's hard to diff visually. - All prior inline review comments (coderabbit and claude) are marked resolved.
6ec5b73 to
4a75ae4
Compare
There was a problem hiding this comment.
No issues found on this pass — all prior review feedback has been addressed. Deferring to a human reviewer since this introduces new thread-pool concurrency into the default bun install hoisted-linking path, which is high-traffic enough that the design and the worker/main-thread data-sharing invariants deserve a human sign-off.
Extended reasoning...
Overview
This PR parallelizes the hoisted node_modules linker by fanning per-package cache→node_modules linking out to the existing thread pool when node_modules is being created fresh. It adds a new ParallelHoistedTask worker type, a wait-group/batch on PackageInstaller, scheduling/drain hooks in hoisted_install.zig, an atomic wrapper around PackageInstall.supported_method, an extracted handleInstallResult() shared between serial and parallel paths, two new env vars (BUN_INSTALL_SERIAL_HOISTED escape hatch and BUN_INTERNAL_PARALLEL_HOISTED_MARKER test hook), a new ~260-line test suite, and ban-limits bumps. Net change is roughly +600/−190 across 8 files, concentrated in src/install/PackageInstaller.zig.
Security risks
None identified. The change does not touch auth, network, or untrusted-input parsing; it reorders existing filesystem operations (hardlink/clonefile from the local cache into node_modules) onto worker threads. Path construction is unchanged from the serial path (same cache_dir_subpath / destination_dir_subpath derivation), and the parallel path is gated to cached resolution tags only — workspaces, folder:, symlink backend, and patched packages stay on the serial path.
Level of scrutiny
High. This is new concurrency in the default code path of bun install, which runs for essentially every Bun user. The review history bears this out: earlier revisions had a use-after-free on installer.names via fixCachedLockfilePackageSlices(), a UAF in deinit() on early-error returns, and an fd leak in the makePath retry — all real and all fixed, but they demonstrate the subtlety of the shared-state boundaries here (workers read installer.lockfile, installer.root_node_modules_folder, and the global supported_method atomic while the main thread continues iterating trees and running runTasks()). The handleInstallResult() extraction also moves ~190 lines of result-handling logic; while it's intended to be verbatim, that's the kind of refactor where a maintainer eyeballing the before/after is valuable.
Other factors
- All 14 prior inline review comments (from coderabbit and the bug-hunting system) are resolved, with the author providing clear rationale for the two that were rejected as false positives.
- The PR includes a dedicated test suite that compares parallel vs. serial layouts byte-for-byte (paths + sizes + symlink targets), exercises the cache-miss fallback, and asserts the parallel path was actually taken via a deterministic stderr marker rather than only timing.
- The cpu/wall parallelism test is appropriately gated (skipped on debug/ASAN/single-core) but is inherently timing-sensitive; the author has acknowledged the cgroup-quota edge case and chosen to address it reactively if it flakes.
- This is a performance-motivated design change (3.4× speedup on the target benchmark) with an env-var escape hatch, which is a reasonable rollout posture — but the decision to ship parallel-by-default vs. opt-in is one a maintainer should ratify.
There was a problem hiding this comment.
No new issues found after the latest round of fixes — but this is a ~600-line concurrency change to the default bun install linking path (thread-pool fan-out, WaitGroup draining, atomic backend selection, cache-miss reroute), so it warrants a human sign-off on the overall design before merge.
Extended reasoning...
Overview
This PR parallelizes the hoisted node_modules linker by fanning per-package hardlink work out to manager.thread_pool when node_modules is being created fresh on POSIX. It adds a ParallelHoistedTask struct, parallel_tasks / parallel_wait_group / parallel_batch state and scheduleParallelBatch() / completeParallelInstalls() to PackageInstaller.zig; extracts the ~190-line result-handling switch into a shared handleInstallResult(); adds a post-tree DrainClosure in hoisted_install.zig; converts PackageInstall.supported_method to an atomic with accessor functions; and registers two new env vars (BUN_INSTALL_SERIAL_HOISTED, BUN_INTERNAL_PARALLEL_HOISTED_MARKER). A new 260-line test compares parallel vs. serial layouts, exercises the cache-miss reroute, and asserts CPU/wall fan-out on release builds.
Security risks
None identified. The change does not touch auth, network, or untrusted input parsing. Workers operate on owned copies of paths derived from the lockfile and only call the existing PackageInstall.install() (same hardlink/clonefile/copyfile logic as before). The destination path is the same absolute top_level_dir/node_modules the serial path uses.
Level of scrutiny
High. bun install is one of the most-used commands and this changes its default behavior on every fresh POSIX install. The PR introduces real concurrency: worker threads run PackageInstall.install() while the main thread continues iterating trees and may call runTasks() / fixCachedLockfilePackageSlices(). Several races/UAFs were caught and fixed across review rounds (stale installer.names slice, deinit() freeing tasks before workers finish, fd leak in the makePath retry, over-broad cache-miss classification). The current revision looks correct to me, but concurrency in a hot path like this benefits from a maintainer's eyes on the gating conditions (canUseParallelHoistedInstall), the interaction with canInstallPackageForTree / pending_installs for transitive folder: deps, and the DrainClosure ordering relative to bin linking and lifecycle scripts.
Other factors
All 15+ inline issues raised across prior bot review rounds have been addressed and resolved by the author. The handleInstallResult() extraction is verbatim (verified against the diff — the added_trusted_dependencies asymmetry is pre-existing). The new test suite is solid (layout fingerprint with symlink targets, deterministic task-count marker, cache-miss fallback coverage), though the CPU/wall parallelism assertion is inherently environment-sensitive and skipped on debug/ASAN. Net: I don't see anything blocking, but the scope and concurrency surface area put this well outside "simple/mechanical" territory.
There was a problem hiding this comment.
No new issues found on this revision, but this changes the default bun install linking path to run concurrently on the thread pool — given the shared-state access between workers and the main thread (and the several memory-safety fixes it took to get here), this one really wants a human pass before merge.
Extended reasoning...
Overview
This PR parallelizes the hoisted node_modules linker in bun install by fanning per-package PackageInstall.install() calls out to the existing thread pool when node_modules is being created fresh on POSIX. It adds a ~120-line ParallelHoistedTask worker struct, three new fields on PackageInstaller (parallel_tasks/parallel_wait_group/parallel_batch), per-tree batch scheduling, a post-loop completeParallelInstalls() drain that runs result handling serially, and a cache-miss fallback that re-routes ENOENT'd packages back through the serial download path. The pre-existing .success/.failure result-handling switch (~190 lines) is extracted verbatim into handleInstallResult() so both paths share it. PackageInstall.supported_method becomes an atomic since workers write it on EXDEV/OPNOTSUPP fallback. A new DrainClosure is added to hoisted_install.zig, and deinit() now flushes/waits on in-flight workers before freeing task structs on error paths. Two env vars are added (BUN_INSTALL_SERIAL_HOISTED escape hatch, BUN_INTERNAL_PARALLEL_HOISTED_MARKER test hook), plus a 268-line test suite and ban-limits bumps.
Security risks
None identified. The change is internal to the install linker; no new inputs are parsed, no auth/crypto/permissions surface is touched. Workers only operate on absolute paths derived from FileSystem.instance.top_level_dir and the already-open cache directory — the same inputs the serial path uses.
Level of scrutiny
High. This is a default-on behavior change to one of Bun's most-used code paths (every fresh bun install on POSIX). It introduces real concurrency: workers share root_node_modules_folder and cache_dir fds, read installer.lockfile, and write a global atomic, while the main thread can concurrently call runTasks() → fixCachedLockfilePackageSlices() (which reallocates lockfile MultiArrayList backing storage). The PR's review history bears this out — it took several rounds to fix a UAF on installer.names (worker reading a slice the main thread rewrites), a UAF in deinit() on early-error returns, and an fd leak in the makePath retry path. All of those are now fixed and all inline comments are resolved, but the density of memory-safety issues found during review is itself a signal that the shared-state surface deserves a careful human read, particularly around what else workers transitively touch via PackageInstall.install() and whether completeParallelInstalls() ordering relative to pending_installs / bin linking / lifecycle scripts is fully equivalent to the serial path.
Other factors
- The
handleInstallResult()extraction is asserted byte-identical to the original.success/.failurearms; I spot-checked and it appears to be, but it's ~190 lines and worth a second pair of eyes. - Test coverage is solid for the happy path (layout equivalence, cache-miss fallback, .bin symlinks) but the concurrency edge cases (lockfile growth during extraction, error-path drain) are not directly exercised.
- No human reviewer has looked at this yet — only bot reviewers.
- There is a
BUN_INSTALL_SERIAL_HOISTED=1escape hatch, which mitigates rollout risk.
087fc6f to
313451b
Compare
|
@coderabbitai review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
908334a to
4fffa44
Compare
Status at 3cc2929a56fec6, the previous head, finished fully green: build 102091, 179/179 (fourth consecutive complete green run). Its bot re-review found no issues; 0 unresolved threads. Sixth rebase (106 commits), with one real interaction. #40014 (self-contained workspaces) gives the hoisted linker a per-tree #40010 ( Verification on this head: Earlier roundsPrevious status (a56fec6)45a8848, the previous head, finished fully green: build 102037, 179/179 (third consecutive complete green run, after 101962 and 100976). Its bot re-review found nothing new; 0 unresolved threads. This head's run is build 102091: mordant is green again, every other GitHub check is green, and Buildkite is at 177/179 with the two darwin aarch64 lanes waiting for an agent; the bot re-review of this head found no issues. This push answers the one red check on that head. The advisory mordant job flagged While checking what pending = true means for the Verification: Previous status (45a8848)6e038a8, the previous head, also finished fully green: build 101962, 179/179 (second consecutive complete green run, after 100976 on the head before it). This push answers the bot review of the rebased worker; two of its three findings were real.
Verification: Previous status (6e038a8)d8ec57c, the previous head, finished CI fully green: build 100976, 179/179. That is the first complete run since the Rust-port rounds and covers the #36298 resolution end to end. Fifth rebase (38 commits). One upstream commit touched this code: #39770 de-monomorphized Verification on this head: Previous status (d8ec57c)Rebase onto current main (291 commits) found one real interaction. #36298 made the
The rebase also re-added d8ec57c on top is test-only, from the bot re-review of 1b09286: the two reroute tests now wait on observable signals (the ordering test polls the marker and fails as soon as it appears; the two-tree test waits for the nested package's cache entry, which makes its park deterministic, re-verified 5/5 with the old condition), the block runs concurrently, and a misplaced doc comment is back on the right function. Verification on this head: Previous status (53cbbed)Lifecycle-script ordering fix. The review on the previous revision found a real regression: with the ancestor gate bypassed by the parallel enqueue, a cache-miss reroute let a nested tree complete, and run its trusted postinstalls, during result replay while a root-tree package was still downloading. Comment pass (908334a). comment-cop flagged 28 multi-line comment groups. The 24 that were this PR's are now one line or deleted, with the worker aliasing contract consolidated into the one 53cbbed on top only touches the new test: its wait for the held tarball request now also settles on process exit, so an early bun failure reports code + stderr instead of hanging to the timeout. Verification on this revision: |
There was a problem hiding this comment.
Beyond the inline nit, I re-checked the two open threads from the previous pass and both are addressed: can_run_scripts() now also gates on can_install_package_for_tree() so a nested tree's scripts wait for every ancestor, with a dedicated regression test that holds the rerouted root download and asserts the nested postinstall is deferred; and PackageInstall.lockfile is now Option<&Lockfile> with workers passing None, so no &Lockfile is materialized on the pool. Given the scope — thread-pool fan-out with raw BACKREF pointers in the installer hot path — a maintainer look at the concurrency design is still warranted.
Extended reasoning...
The two unresolved inline threads from the 2026-08-13 19:31 pass are both fixed in the current diff: (1) the lifecycle-script ancestor-ordering bug is closed by adding can_install_package_for_tree(completed_trees, trees, scripts_tree_id) to can_run_scripts(), and the new lifecycle scripts wait for ancestor trees test in parallel-hoisted-install.test.ts reproduces the exact reroute-while-nested-tree-completes scenario and asserts the marker only appears after the held root download is released; (2) the worker-side &Lockfile aliasing is closed by making PackageInstall.lockfile an Option and passing None in ParallelHoistedTask::run(), with the SAFETY note updated to state the lockfile is main-thread-only. This run's only remaining finding is a test-robustness nit (unwired process-exit on await libRequested.promise). The PR remains a ~1000-line concurrency change to the production install path with substantial unsafe (raw *mut PackageInstaller backrefs, intrusive thread-pool tasks, Drop-joins), so I'm not auto-approving.
There was a problem hiding this comment.
I re-reviewed after 53cbbed — the test's libRequested is now settle-once with an early-exit branch that surfaces stderr, and the earlier lifecycle-ordering and Option<&Lockfile> fixes from 4fffa44 look right. No new issues found. Given the scope (thread-pool workers reaching into PackageInstaller via raw-pointer projections, the Drop-joins-workers contract, and the can_run_scripts() ancestor gate), a maintainer sign-off on the concurrency design is still worthwhile before merge.
What was reviewed:
ParallelHoistedTask::run()aliasing: workers now project onlyroot_node_modules_folder+ the atomic wait group;lockfile: Noneon the worker path, so no&Lockfileoverlaps main-thread&mut.can_run_scripts()now also walks ancestors viacan_install_package_for_tree();run_available_scripts()rescans andcomplete_remaining_scripts()drains, so deferred scripts aren't stranded.Drop for PackageInstallerschedules the pending batch and waits before freeing tasks, so an error return fromhoisted_installcan't leave workers touching freed state.- Verifier ruled out the
debug_assert(pending_installs.len()==0)firing after replay — the reroute path pushes topending_installsonly under!IS_PENDING_PACKAGE_INSTALLand the drain closure runsrun_tasksbefore that assertion loop.
Extended reasoning...
Overview
This PR parallelizes the hoisted node_modules linker on POSIX when node_modules is fresh. It adds ParallelHoistedTask (a thread-pool work item that owns copies of the per-package paths and calls PackageInstall::install(skip_delete=true, ...) on a worker), per-tree batching via schedule_parallel_batch(), a join+replay in complete_parallel_installs(), and a Drop impl on PackageInstaller that joins outstanding workers before any field is freed. The ~300-line inline result-handling block is extracted verbatim into handle_install_result() so the serial and parallel paths share it. PackageInstall.lockfile becomes Option<&Lockfile> so workers pass None and never materialize a &Lockfile that could alias main-thread &mut. can_run_scripts() gains an ancestor-completion check. Two env vars are added (a serial-fallback escape hatch and a test-only marker), plus a three-test file covering parallel/serial layout equivalence, cache-miss reroute, and the lifecycle-ordering regression.
Security risks
None identified. The change is internal to the installer's link-from-cache step; inputs are cache directories and lockfile-derived paths already trusted at this layer. No new parsing of untrusted data, no network handling, no auth/crypto/permissions surface. The env vars added are debug/escape-hatch knobs.
Level of scrutiny
High. bun install is a production-critical path, and this introduces genuine cross-thread concurrency with an unsafe-mediated aliasing contract: workers hold a lifetime-erased *mut PackageInstaller<'static> and project fields via addr_of! while the main thread holds &mut PackageInstaller in run_tasks and the next tree's install_package. The correctness of that contract depends on workers touching exactly the two fields the SAFETY note names, on the WaitGroup join happening before any replay or drop, and on the Drop impl being the only teardown path. Earlier review rounds on this PR found a real ordering regression (nested-tree lifecycle scripts firing before an ancestor's rerouted download landed), so the invariants here are subtle enough to have already broken once.
Other factors
- All three of my prior findings (🔴 lifecycle ordering, 🟡
&*lockfilealiasing, 🟡 test hang on early exit) are addressed in 4fffa44 and 53cbbed, and each fix matches the suggested shape. - The
handle_install_result()extraction is a large code move; I spot-checked that the moved block matches the deleted block (trusted-dep recording, EACCES fstat branch, DanglingSymlink/Folder error arms,increment_tree_install_counton both arms) and thatIS_PENDING_PACKAGE_INSTALLthreads through asis_pending_package_install. - The verifier separately examined and refuted a candidate
debug_assert(pending_installs.len()==0)firing after parallel replay. - Test coverage is solid for what it targets (byte-identical layout, reroute fallback, ancestor-gated scripts) and hermetic (local tarballs / in-test registry,
port: 0, own cache dir). - The author's own status comment explicitly asks for a maintainer to review the concurrency design; that plus the size (~650 net new lines in
PackageInstaller.rs) puts this outside auto-approval.
53cbbed to
1b09286
Compare
There was a problem hiding this comment.
The two test-quality nits from my previous pass (misplaced JSDoc on installedCount; settle() sleep-then-check) are addressed in d8ec57c — the doc is now on parallelTaskCount(), and the reroute tests poll (appearsWithinBound, until(cacheHas(...))) instead of using a wall-clock proxy. This pass found nothing new.
Given the scope — ~900 lines in PackageInstaller.rs introducing thread-pool workers that share a *mut PackageInstaller with the main thread via addr_of! field projection, plus the change to can_run_scripts()'s ancestor-completion invariant — a maintainer look at the concurrency design is still worthwhile (the author's own status note at 1b09286 says the same).
What was reviewed:
handle_install_result()extraction is byte-equivalent to the removed inline block (const-genericIS_PENDING_PACKAGE_INSTALL→ runtime bool,destination_dir.fd()→ passedFd).- Worker/main-thread aliasing:
run()projects onlyroot_node_modules_folderand the atomic wait-group;lockfile: Noneon workers so no&Lockfileis materialized. - Task-creation gate (
NEEDS_VERIFY && !IS_PENDING_PACKAGE_INSTALL) plus the debug assertion ensure no task is enqueued aftercomplete_parallel_installs();Drop for PackageInstallerjoins on early-return paths.
Extended reasoning...
Overview
This PR parallelizes the hoisted node_modules linker for the fresh-install case (POSIX, skip_delete=true). It adds ParallelHoistedTask (per-package cache→node_modules link on the thread pool), schedule_parallel_batch() / complete_parallel_installs(), and extracts the ~280-line result-handling block into handle_install_result() so serial and parallel paths share it. PackageInstall.lockfile becomes Option so workers pass None; can_run_scripts() now also checks ancestor-tree completion (the invariant the parallel enqueue's bypass of can_install_package_for_tree() broke). A DrainClosure in hoisted_install.rs handles rerouted downloads. Two env vars are added (BUN_INSTALL_SERIAL_HOISTED escape hatch, BUN_INTERNAL_PARALLEL_HOISTED_MARKER test-only). A new 460-line test file covers layout equivalence, cache-miss reroute, lifecycle-script deferral, and the pending_installs drain interaction with #36298.
Security risks
None specific to this change. The parallel path is gated to cache→node_modules hardlinking of packages already in the local cache; no new network, path-resolution, or tarball-extraction surface. The unsafe is aliasing-model discipline (raw *mut PackageInstaller shared across threads, addr_of! field projection, heap::into_raw/take for task ownership), not input validation.
Level of scrutiny
High. This is production-critical (bun install's default linker), ~1160 lines touching concurrency in a subsystem with hand-managed raw-pointer lifetimes and Stacked-Borrows-sensitive BackRef patterns. Earlier review rounds found two real bugs (lifecycle-script ordering on cache-miss reroute; &Lockfile materialized on a worker overlapping &mut Lockfile on the main thread), both fixed with tests. The extraction of handle_install_result() is verifiable as behavior-preserving, but the surrounding invariants (task creation only from the tree loop; Drop joining on error returns; the SAFETY contract that workers touch exactly two fields) are the kind a maintainer familiar with the isolated-install precedent should confirm.
Other factors
The PR has been through 29 iterations with extensive bot review; every prior finding is resolved with a fix commit and a regression test. The author's own status note explicitly asks for a maintainer look at the concurrency design. My two most-recent nits (test-only: JSDoc placement, sleep-then-check) are addressed in the current head d8ec57c, and this run's bug-hunting pass found nothing. No human reviewer has weighed in yet. Given the repo's REVIEW.md emphasis on memory-safety and thread-affinity being the most-blocked category, this is not a change I should approve without human sign-off on the design.
d8ec57c to
6e038a8
Compare
There was a problem hiding this comment.
Re-reviewed after 45a8848 — the three nits from the previous pass are addressed (worker now checks the completion marker via is_package_in_cache_at() with a covering test, finish_raw replaces the autoref'd finish(), and the macOS clonefile trade-off is documented at the make_path call). No new issues found this round.
Given the scope — ~900 net lines across the hoisted installer's core loop, a new thread-pool task type with raw-pointer BACKREFs and hand-reasoned Stacked Borrows discipline, and the dropped ancestor barrier — this still warrants a human sign-off before merge.
What was reviewed
handle_install_result()extraction verified against the removed inline block; behavior-preserving.ParallelHoistedTask::run()aliasing: workers project onlyroot_node_modules_folderand the atomic wait-group viaaddr_of!;lockfile: Noneon the worker.- Enqueue gate (
needs_verify && !is_pending_package_install) plus the debug assertion inhoisted_install.rskeep tasks from being created after replay; the two-tree drain test exercises it.
Extended reasoning...
Overview
This PR parallelizes the hoisted node_modules linker on POSIX when node_modules is fresh (skip_delete). It adds ParallelHoistedTask (a thread-pool task that hardlinks one cached package on a worker), three new fields on PackageInstaller (parallel_tasks, parallel_wait_group, parallel_batch), schedule_parallel_batch() / complete_parallel_installs(), a Drop impl that joins in-flight workers on early error return, and extracts the ~290-line result-handling block into handle_install_result() so the serial and replay paths share it. PackageInstall.lockfile becomes Option<&Lockfile> so workers can pass None. can_run_scripts() gains an ancestor-tree check because the parallel path bypasses can_install_package_for_tree(). Two env vars are added (a serial escape hatch and a test-only marker), plus a 486-line test file with four tests.
Since my previous inline comments earlier today, commit 45a8848 landed and addresses all three: the worker now calls is_package_in_cache_at() before opening the cache dir (with a new test that strips package.json from a cache entry and asserts re-download); run_from_thread_pool now uses WaitGroup::finish_raw(addr_of!(...)) matching the in-tree convention for non-static wait groups; and the nested-tree make_path call now carries a one-line comment naming the macOS whole-dir-clonefile cost.
Security risks
None identified. The change is filesystem-only (hardlink/clonefile from a local cache into node_modules), gated on skip_delete (fresh tree), and does not touch network, auth, or path validation — the existing alias-name sanitization and cache-subpath computation are unchanged. The new env vars are a boolean escape hatch and a test-only stderr marker.
Level of scrutiny
High. This is the core bun install linker, run on every fresh install. The change introduces concurrent filesystem writes coordinated through unsafe raw-pointer backrefs into a stack-local PackageInstaller that the main thread simultaneously holds &mut to, with correctness depending on hand-reasoned Stacked/Tree Borrows discipline (addr_of!-only field projection, finish_raw vs finish, Option<&Lockfile> to keep workers off lockfile state). It also drops an ordering invariant (can_install_package_for_tree() before nested trees) and compensates elsewhere (can_run_scripts()). Each prior review round on this PR surfaced a distinct correctness or aliasing concern that was then fixed — the change is well-tested and iteration 29 looks sound to me, but the blast radius of a miss (silent partial installs, UAF under load, lifecycle scripts firing early) puts it well outside auto-approval scope.
Other factors
The PR has been through 29 iterations over four months with extensive back-and-forth; the handle_install_result() extraction is a verbatim move (I diffed it against the removed block — matches modulo the destination_dir_fd parameter and the alias/pkg_name_hash re-derivation). Test coverage is solid: byte-identical layout vs serial, cache-miss reroute, completion-marker miss, the pending-installs drain race, and lifecycle-script deferral. The remaining macOS perf trade-off is documented and correctness-preserving. No outstanding unaddressed reviewer comments.
There was a problem hiding this comment.
I reviewed this PR and the multi-agent bug hunt found no issues this pass; all prior inline findings have been addressed. Given the scope — new cross-thread unsafe (lifetime-erased *mut PackageInstaller backrefs, WaitGroup join-on-Drop, aliasing discipline against a live &mut) on the core install path, plus the open design question the author flagged (per-tree barrier vs. the can_run_scripts() ancestor check) — a human sign-off is still warranted.
What was reviewed: worker/main-thread aliasing (addr_of!-only field projection, finish_raw contract, Drop ordering); the enqueue gate needs_verify && !is_pending_package_install against post-replay task creation; cache-miss reroute now runs the same is_package_in_cache_at() completion-marker check as serial; handle_install_result() verified as a verbatim extraction of the removed inline block; test file checked for hang-on-exit wiring and sleep-then-check patterns.
Extended reasoning...
Overview
This PR parallelizes the hoisted node_modules linker for fresh installs on POSIX. It adds ParallelHoistedTask (a thread-pool work item that runs PackageInstall::install() on a worker), three new fields on PackageInstaller (parallel_tasks, parallel_wait_group, parallel_batch), schedule_parallel_batch() / complete_parallel_installs(), a Drop impl that joins in-flight workers on early return, and extracts the ~290-line install-result match into handle_install_result() so both paths share it. PackageInstall.lockfile becomes Option<&Lockfile> so workers can pass None. can_run_scripts() gains an ancestor-tree completion check. Two env vars are added (a serial escape hatch and a test-only marker). A 525-line test file exercises layout parity, cache-miss reroute, completion-marker miss, pending-install drain, and lifecycle-script deferral. Net: ~1,230 insertions / ~300 deletions across 6 files.
Security risks
No auth/crypto/network-trust surface. The salient risk is memory safety: workers hold a lifetime-erased *mut PackageInstaller<'static> while the main thread holds &mut PackageInstaller and continues calling run_tasks / install_package. The design confines workers to two fields via addr_of! (the read-only root_node_modules_folder fd and the atomic parallel_wait_group), uses WaitGroup::finish_raw per its documented contract, and joins in Drop before any field is freed. The SUPPORTED_METHOD global written on the EXDEV fallback was already atomic. These were the specific areas prior review rounds probed; the current revision addresses each raised point.
Level of scrutiny
High. This is production-critical (every bun install on a fresh tree), concurrency-heavy, and unsafe-heavy Rust in what REVIEW.md calls the most-blocked category. It has been through 29 iterations and ~25 resolved inline findings across Zig and Rust versions, which is evidence of careful iteration but also of how many subtle edges this change has. The author has explicitly left a design choice open for a maintainer ("left as is pending the design review, easy to switch if preferred" — whether to sequence batches per tree, which would restore the ancestor-install invariant structurally instead of via the added can_run_scripts() check and eliminate the documented macOS clonefile-EEXIST race). That is a decision a human should make.
Other factors
The extracted handle_install_result() diffs cleanly against the removed inline block (the only intentional change is destination_dir.fd() → an Fd parameter). The test suite is thorough and each test's failure mode on a regressed build is documented. The debug_assert! after the pending-install drain guards the invariant that no task is created post-replay. All of my prior inline findings (finish_raw vs finish, completion-marker check on workers, macOS clonefile race, test hang wiring, sleep-then-check) are resolved in the current revision. No outstanding reviewer comments remain. Given the complexity, the unsafe surface, and the author-flagged open design question, deferring to a human is the right call.
a56fec6 to
f636573
Compare
There was a problem hiding this comment.
This automated review pass found no new issues. Given the scope (~950 lines in PackageInstaller.rs, thread-pool workers holding raw backrefs into a live &mut PackageInstaller, and the per-tree-barrier design question the PR body explicitly leaves open), a maintainer sign-off on the concurrency model is still the right next step.
What was reviewed this round:
- The three Aug-21 fixes landed as described: workers now run
is_package_in_cache_at()before linking (with a covering test),finish_raw(addr_of!(...))replaces the autoref'd.finish(), and the macOS clonefile/EEXIST tradeoff is documented at themake_path()site. - The new
copy_treehandling for self-contained workspaces: verified the enqueue snapshotsis_copy_tree()and the worker picksCopyfile, matching the serial site's method selection; the nlink test forces--backend=hardlinkso the assertion is meaningful on macOS. Drop for PackageInstallervs.complete_parallel_installs(): both drainparallel_tasks, so the second wait/drain is a no-op on the happy path and only fires on early-error unwind — no double-free of the task boxes.
Extended reasoning...
Overview
The PR parallelizes hoisted node_modules linking on POSIX when node_modules is fresh. It adds ParallelHoistedTask (heap-allocated, run on manager.thread_pool), a WaitGroup join in complete_parallel_installs(), and extracts the ~290-line inline result-handling block into handle_install_result() so both paths share it. PackageInstall.lockfile becomes Option so workers can pass None. can_run_scripts() gains an ancestor-tree check to compensate for bypassing can_install_package_for_tree(). Two env vars (BUN_INSTALL_SERIAL_HOISTED, BUN_INTERNAL_PARALLEL_HOISTED_MARKER) and a 561-line test file are added.
Security risks
None identified. The change is local-filesystem I/O (hardlink/clonefile/copyfile from Bun's own cache into node_modules); no network input is parsed on workers, and the destination-subpath validation (reject anything that could escape node_modules) runs on the main thread before enqueue. Workers copy their path buffers by value at enqueue time.
Level of scrutiny
High. This is production package-manager code with substantial unsafe (raw *mut PackageInstaller<'static> backrefs, lifetime erasure, addr_of! field projection while the main thread holds &mut). The SAFETY comments are detailed and internally consistent, and every prior review finding (across seven rounds since April) was addressed with a fix and a test. The bug-hunting system found nothing this round. But the aliasing discipline (workers must never form &PackageInstaller; only root_node_modules_folder and the atomic wait-group are touched via addr_of!) and the lifecycle-script ordering invariant (can_run_scripts() now checking ancestors) are subtle enough that a maintainer familiar with the installer's threading model should confirm them.
Other factors
- The PR body explicitly flags an open design choice (per-tree barrier vs. barrier-free with the
can_run_scripts()ancestor check) for whoever does the design pass. That alone puts this outside auto-approval scope. handle_install_result()is a verbatim extraction of the pre-existing inline block; I diffed it against the removed lines and the only non-mechanical change is threadingdestination_dir_fdas a parameter (with the parallel path passing the root fd, justified inline).- Test coverage is thorough for the new paths (layout parity, cache-miss reroute, completion-marker miss, pending-install drain, ancestor-gated postinstall, self-contained-workspace copy) and each was verified to fail on a regressed build per the timeline.
- Iteration 29; every prior automated finding is resolved and marked as such in the thread.
f636573 to
8969f93
Compare
There was a problem hiding this comment.
I reviewed this PR and the automated pass found no bugs; every prior inline finding (completion-marker check, finish_raw, lifecycle-script ordering, test flakiness) has been addressed. Given the scope — ~1,300 lines of new concurrency in the hoisted installer with unsafe raw-pointer aliasing across the thread pool, plus the open design question about per-tree batch sequencing that the description explicitly leaves for a maintainer — a human sign-off is still warranted.
What was reviewed
ParallelHoistedTask::run's aliasing discipline (addr_of!only,finish_raw, no&PackageInstalleron workers) and theDropjoin on early-error returns.- The reroute/replay path:
needs_verify && !is_pending_package_installgate,can_run_scripts()ancestor check, and the post-replay debug assertion — no path enqueues aftercomplete_parallel_installs(). handle_install_result()extraction verified byte-identical to the removed inline block (including the EACCES fstat and trusted-dep branches).- The seven tests: hermetic (local tarballs / in-process registry),
orExit-guarded awaits, bounded polls; thecopy_treeslink-count check forces--backend=hardlinkso it's meaningful on macOS too.
Extended reasoning...
Overview
This PR parallelizes the hoisted node_modules linker on POSIX when node_modules is fresh. It adds ParallelHoistedTask (thread-pool work item that hardlinks/clones one package from cache), three new PackageInstaller fields (parallel_tasks, parallel_wait_group, parallel_batch) plus a Drop impl that joins in-flight workers, schedule_parallel_batch() / complete_parallel_installs(), and extracts the ~290-line result-handling switch into handle_install_result() shared by both paths. hoisted_install.rs gains a post-tree-loop drain closure for rerouted downloads. PackageInstall.lockfile becomes Option so workers can pass None. Two env vars are added (BUN_INSTALL_SERIAL_HOISTED escape hatch, BUN_INTERNAL_PARALLEL_HOISTED_MARKER test-only). Seven new tests cover layout parity, cache-miss reroute, completion markers, pending-install drains, lifecycle-script ordering, and self-contained-workspace copy trees.
Security risks
None identified. The change is filesystem-local (hardlink/clonefile/copyfile from Bun's own cache into node_modules); no new network, auth, or user-input parsing surface. Path components (destination_dir_subpath, cache_dir_subpath) are the same values the serial path already uses, copied with their trailing NUL at enqueue time.
Level of scrutiny
High. bun install is the most-run command in the product, and this is its default hot path on POSIX. The change is concurrency-heavy: a lifetime-erased *mut PackageInstaller<'static> is handed to worker threads while the main thread holds &mut PackageInstaller, with correctness resting on the SAFETY discipline that workers only project two fields via addr_of! and finish via finish_raw. That discipline looks right after the prior review rounds, but it's exactly the kind of invariant a human maintainer should confirm. The PR body also explicitly leaves a design choice open ("left as is pending the design review"): whether to add a per-tree join to eliminate the macOS clonefile-EEXIST fallback race and structurally restore the ancestor-tree ordering that can_run_scripts() now checks explicitly.
Other factors
The PR has been through 29 iterations and many automated review rounds; every finding I raised (worker completion-marker check, WaitGroup::finish vs finish_raw, lifecycle-script ancestor ordering, test await-vs-exit wiring, sleep-then-check → bounded poll, misplaced JSDoc) was addressed and is visible in the current diff. The handle_install_result() extraction is a verbatim move of the pre-existing block. Tests pass under ASAN and release per the evidence block. What remains is a maintainer call on the overall design and the open per-tree-barrier question, not a correctness gap I can point at.
When node_modules is being created fresh on POSIX, fan per-package cache->node_modules link operations out to manager.thread_pool instead of running them one at a time on the main thread. Each worker owns a ParallelHoistedTask with duped paths and runs PackageInstall::install() (the expensive hardlink/clonefile walk); the main thread blocks on a WaitGroup after the tree iteration and feeds each stored InstallResult through a shared handle_install_result() so summary, bin links, tree counts, and lifecycle scripts stay serial. Workers that hit ENOENT opening the cache subpath set missing_from_cache; complete_parallel_installs() re-routes those packages through the serial download path and hoisted_install drains the resulting tasks before bin linking. Gated to POSIX (Windows already fans out per file via HardLinkWindowsInstallTask), fresh node_modules only (skip_delete), cached resolution tags only (npm/git/github/local_tarball/ remote_tarball), and non-patched packages with a non-symlink backend. BUN_INSTALL_SERIAL_HOISTED=1 opts out. On a 6-core overlayfs host, warm bun install --frozen-lockfile of a Next.js template (~700 packages, ~42k files) drops from 917ms to 271ms (3.4x).
ParallelHoistedTask::run() now projects root_node_modules_folder and lockfile via addr_of! instead of forming a whole-struct &PackageInstaller, so it never aliases the main thread's &mut PackageInstaller under Stacked Borrows (matches the isolated-install worker at Installer.rs:790-798 and this file's own run_from_thread_pool()). Document that complete_parallel_installs() passes the root node_modules fd to handle_install_result() for nested trees (only consumed by the EACCES fstat heuristic; on a fresh node_modules every nested dir has the same owner/mode). Drop the cpu/wall ratio test: on macOS, clonefile is one syscall per package so 60 packages link in a handful of ms and the ratio (1.17-1.19 on darwin-14/26 CI) can't clear 1.25 even though the thread-pool fan-out is happening. Already skipped on musl/debug/ASAN for similar reasons. The deterministic [ParallelHoistedInstall] N tasks marker in test 1 proves the parallel path and satisfies the gate.
Every multi-line comment this PR adds is cut to one line or removed; the aliasing contract for worker threads now lives in one SAFETY note in ParallelHoistedTask::run(). The deinit note is restored to main's text. The four multi-line comments still present in the diff are main's own, relocated verbatim by the handle_install_result() extraction.
can_run_scripts() only required a tree's own subtree to be installed. That was sufficient because can_install_package_for_tree() would not install a nested tree's packages until every ancestor tree was complete, so a tree could not finish before its ancestors. The parallel linker bypasses that gate, so when a root-tree package is rerouted to a download, result replay could complete a nested tree and spawn its postinstall while the root package was still in flight; a script that required a dependency hoisted into the root then failed with MODULE_NOT_FOUND. can_run_scripts() now also walks the ancestors, via the same helper; this is a no-op for the serial linker, and complete_remaining_scripts() still runs anything deferred. The new test serves a small registry from the test, evicts a root-tree package from the cache, holds its tarball response, and checks that the nested tree's trusted postinstall has not run by the time bun requests it (6/6 failures on the previous revision), then releases it and checks the script does run afterwards. The existing tests pass --ignore-scripts and could not observe this. Also stop materializing &Lockfile on the worker: PackageInstall.lockfile is now Option, read only by verify() and Folder installs, and the worker passes None.
…eld tarball The wait for lib's tarball request is now settled by either the request or process exit, so a bun that dies earlier fails the test immediately with its exit code and stderr instead of hanging to the file timeout. The hold is released in a finally so it can never outlive the check.
Since #36298 the pending_installs drain re-enters install_package_with_name_and_resolution with NEEDS_VERIFY, which the parallel enqueue used as its entry-point check. A package parked in pending_installs during a reroute (a nested-tree miss landing while a root-tree miss is still downloading) would then be handed to the thread pool after complete_parallel_installs() had already run, and nothing would replay it: linked on disk, but never counted, bin linked, or given its scripts. The enqueue now also requires !IS_PENDING_PACKAGE_INSTALL, so only the tree loop creates tasks, and hoisted_install asserts no task survives past the replay. The test evicts both a root and a nested package, holds the root tarball until the nested one has been served and parked, then releases it and checks the nested package's postinstall ran and the summary count matches a warm install. With the old condition it exits 134 on the new assertion.
The ordering test now polls for the marker and fails as soon as it appears; the bound is still measured in bun startups, and the comment says why no other signal exists. The two-tree test waits for the nested package's cache entry to reappear, which means its extraction finished and its install attempt is queued ahead of the held root package, so the park in pending_installs no longer depends on a delay; with the old enqueue condition it still exits 134, 5/5. The block's tests own their directories and servers, so they run concurrently. Also moves the parallelTaskCount() doc back onto that function.
The serial path treats an npm entry without package.json, or a git entry without .bun-tag, as a cache miss; the worker only noticed a missing directory, so such an entry would have been linked and reported as installed. The worker now runs the same is_package_in_cache_at() check before touching the destination and flags the task for re-download, which keeps that faccessat off the main thread. Test strips package.json from a cached entry and checks the reinstall has it; it passes on the previous revision's link of the broken entry. run_from_thread_pool() now uses WaitGroup::finish_raw() through addr_of!: Drop for PackageInstaller frees the group as soon as wait() returns, which is finish_raw()'s documented case, and the &self of finish() would assert the group live until the call returned. This also stops the worker forming a reference into the PackageInstaller. Note at the make_path() site that a nested-tree worker may pre-create its parent package's directory, which on macOS costs that package the whole-directory clonefile fast path.
… walk The rerouted replay call passed bare bools for needs_verify and is_pending_package_install. Every other call site names them with locals, which is also what the bare_bool_args lint asks for. Both replay branches now share one is_pending_package_install local. The new test patches the nested scripted@1 so it takes the serial path and parks in pending_installs during the tree walk while root still waits on its parallel tasks. Replay completes root without a drain, so the drain after replay has to install it, count it and run its scripts.
#40014 makes the hoisted linker use the copyfile backend for trees that belong to a self-contained workspace (copy_trees). The parallel worker chose its backend on its own and hardlinked those trees from the cache. Each task now records whether its tree is a copy tree, decided on the main thread at enqueue, and the worker picks Copyfile for it. The serial site and the enqueue share one is_copy_tree() helper. The new test warms the cache, removes both node_modules and reinstalls with --backend=hardlink: the root's copy of lib keeps a link count above one while the workspace's copy has a link count of one.
8969f93 to
3cc2929
Compare
There was a problem hiding this comment.
I reviewed the latest revision and found no new issues — the three nits from the 2026-08-21 pass (worker-side completion-marker check, finish_raw for the wait group, and the macOS clonefile note) are all addressed. Given the scope (new thread-pool fan-out in the hoisted linker, ~960 lines in PackageInstaller.rs with raw-pointer BACKREFs and a Drop that joins in-flight workers) and the design tradeoff the PR body explicitly leaves open (per-tree batch sequencing vs. the can_run_scripts() ancestor check), a maintainer should still do the design pass.
Checked this round: the handle_install_result() extraction is byte-identical to the inline block it replaces; workers form no &PackageInstaller (all field access is addr_of!); copy_tree is snapshotted at enqueue so the self-contained-workspace backend matches the serial site; and the needs_verify && !is_pending_package_install gate plus the post-replay debug_assert close the reroute-after-replay hole.
Extended reasoning...
Overview
This PR parallelizes the hoisted node_modules linker on POSIX when node_modules is being created fresh. It adds ParallelHoistedTask (a thread-pool work item that owns copies of the paths and calls PackageInstall.install() on a worker), three new #[cfg(unix)] fields on PackageInstaller (parallel_tasks, parallel_wait_group, parallel_batch), schedule_parallel_batch() / complete_parallel_installs(), and a Drop for PackageInstaller that joins any in-flight workers on early error return. The ~290-line inline result-handling block is extracted verbatim into handle_install_result() so the serial and replay paths share it. PackageInstall.lockfile becomes Option<&Lockfile> so workers can pass None. can_run_scripts() gains an ancestor-tree check. Two env vars are added (BUN_INSTALL_SERIAL_HOISTED escape hatch, BUN_INTERNAL_PARALLEL_HOISTED_MARKER test hook). Seven tests in a new file cover layout parity, cache-miss reroute, completion-marker miss, pending-install drain during and after replay, lifecycle-script ordering, and the self-contained-workspace copy backend.
Security risks
None identified. The change is confined to how already-cached package contents are linked into node_modules; no new parsing of untrusted input, no network handling, no path validation changes. The worker-side is_package_in_cache_at() check preserves the completion-marker gate that was previously on the main thread.
Level of scrutiny
High. This introduces concurrency into a critical package-manager path with substantial unsafe (raw *mut PackageInstaller<'static> back-references, addr_of! field projection under an aliased &mut, WaitGroup::finish_raw, intrusive thread-pool tasks, a Drop that must join workers before fields are freed). REVIEW.md's "Native code: memory safety" section applies directly: thread affinity of every line, refcounts balanced on every terminal path, no pointer outliving what it points into. The extracted handle_install_result() also had to track six upstream changes across rebases, so behaviour-preservation of that extraction is load-bearing.
Other factors
The PR has been through many review rounds; every prior finding (my own and CodeRabbit's) is resolved. Test coverage is substantial and each test's failure mode on a regressed build is documented. However, the PR body itself flags an open design choice — releasing tree batches without a barrier vs. sequencing them — and defers to "whoever does the design pass". That, combined with the size and the unsafe/threading surface, puts this outside what an automated review should approve without a maintainer sign-off.
This branch has not been deployed
1 participant
What
Parallelize the hoisted
node_moduleslinker whennode_modulesis being created fresh (the CI / fresh-clone scenario: lockfile present, cache warm,node_modulesdeleted). Previously every package was hardlinked from the cache intonode_modulesserially on the main thread; this PR fans each cached package out to the existing thread pool.Why
aube's published benchmarks show Bun ~3× slower than aube on warm-cache CI install (416ms vs 139ms). Profiling with
strace -c -fon their fixture (~1,200 packages, ~42,000 files) shows:linkat+ 11,576getdents64+ 10,233openat+ 6,114mkdirat+ 8,641close≈ 80ksymlink+ 10,415statx+ 3,854openat≈ 26kaube's default is a pnpm-style isolated layout with a global virtual store: once warm, "installing" is one
symlinkper package intonode_modules/.aube/<dep>plus one per direct dep — no per-file work. Bun's default is a real hoistednode_modules(42k files). The outputs aren't equivalent, but the serial loop was the low-hanging fruit either way.The isolated installer (
nodeLinker=isolated) already runs per-package tasks onmanager.thread_pool; this brings the same pattern to hoisted.How
ParallelHoistedTaskowns copies ofnode_modules_path/destination_dir_subpath/cache_dir_subpath, opens the tree'snode_modulesdir on a worker, and callsPackageInstall.install(skip_delete=true, …).scheduleParallelBatch()so workers start as soon as the root tree is enumerated.completeParallelInstalls()waits on aWaitGroup, then runs the existing result handling (summary,.binlinking, lifecycle scripts, tree-install counts) serially on the main thread — extracted intohandleInstallResult()so both the serial and parallel paths share it.packageMissingFromCache()faccessat(1,230 calls on the main thread) moves onto the workers: each runs the sameis_package_in_cache_at()check (npmpackage.json/ git.bun-tagcompletion markers, directory existence for other tags) before linking, and an entry that fails it, or that turns out to be gone when opened, is flaggedmissing_from_cacheand re-routed through the serial download/extract path on the main thread. Tasks are only ever created from the tree loop (needs_verify && !is_pending_package_install); the reroute replay, the post-download path, and thepending_installsdrain all install serially, sincecomplete_parallel_installs()is the only thing that replays tasks.install()can run on workers and writes the process-wide install method on EXDEV /OPNOTSUPPfallback; that global was already an atomic on main by the time of the Rust port, so this PR only relies on it. Workers also never touch the lockfile:PackageInstall.lockfileis nowOption, read only byverify()and Folder installs, and workers passNone.can_install_package_for_tree(), so nested trees no longer wait for their ancestors to be installed first. That gate was only documented as protecting the rename-aside step (irrelevant withskip_delete), but it was also what kept a tree from completing, and so running its lifecycle scripts, before the ancestor trees its dependencies may be hoisted into were on disk.can_run_scripts()now checks the ancestors explicitly (a no-op for the serial linker), and a test covers the reroute case that exposed it.node_modules(skip_delete). Windows already fans out per-file viaHardLinkWindowsInstallTask. Workspaces / folder deps / patched packages /--backend=symlinkfall through to the existing serial path.make_path()can create its parent package's directory before the parent's own worker runs. On macOS that package then takesinstall_with_clonefile()'s EEXIST fallback (per-file clones instead of one whole-directory clone); Linux's hardlink path is unaffected, and the result is identical either way. It only affects packages that are hoist points for a nested tree, and only when the race lands. Sequencing the batches per tree would remove it (and would restore the ancestor invariant structurally, making thecan_run_scripts()check belt-and-braces) at the cost of a join per tree; left as is pending the design review, easy to switch if preferred.BUN_INSTALL_SERIAL_HOISTED=1forces the old serial path for comparison / debugging.Results
6-core Linux (overlayfs), aube's
benchmarks/fixture.package.json,hyperfine --warmup 3 --runs 12:add is-oddMain-thread strace after the change:
faccessatdrops from 1,230 → 0; main thread is now just task setup +futexwait + bin linking. Worker load is balanced across all cores.On warm-cache CI, aube still edges ahead on overlayfs because it does ~26k syscalls (symlinks into a pre-materialized virtual store) vs bun's ~78k (real 42k-file hoisted tree). Extrapolating from aube's own published ext4 number (bun 416ms → ÷3.4 ≈ 123ms vs aube 139ms), this should flip on a native filesystem.
Correctness
test/cli/install/parallel-hoisted-install.test.ts, seven tests. Parallel and serial installs produce byte-identicalnode_modules(60 scoped/unscoped packages with nested dirs and.binentries; the marker asserts that all 60 took the parallel path). A partial cache (3 entries deleted) still installs every package through the reroute. An entry without its completion marker is re-downloaded rather than linked. With an in-test registry that can hold a tarball: a nested tree's postinstall waits for a rerouted root package; a package parked inpending_installsduring a reroute is installed, counted and has its scripts run after replay; a package parked during the tree walk itself (a patched nested dependency, which stays on the serial path while root waits on its tasks) is installed by the drain that follows replay; and a self-contained workspace's packages come out as copies (link count 1) while the root's stay hardlinked.bun-install-registry.test.ts243/243; the patch, tarball-integrity, bad-workspace and hoist suites all pass; the lifecycle suite matches main's baseline (119/122, the 3 failures are the pre-existingnode-binary tests); clippy is clean on linux and on the windows target.BUN_INSTALL_SERIAL_HOISTED=1gives the old behaviour for A/B comparison; the layout test uses it as the reference.Related
Possibly addresses (warm-cache / frozen-lockfile install slowness reports):
bun install --frozen-lockfile --productionunusually slow on Docker build #28278bun install --frozen-lockfileis exceptionally slow in docker [bun 1.1.4] #10371[review] gate passed · iteration 30 · 6 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 11 passed · 0 rejected · iteration 30
evidence per changed file