Repository navigation
Conversation
With a limit of 0 no lifecycle script can start. `bun install` and
`bun pm trust` then spin in `while alive_count >= max { sleep() }`,
because `sleep()` returns at once when nothing is alive.
bunfig.toml already maps `[install] concurrentScripts = 0` to the
default (2x CPU count). The CLI parser now does the same.
|
Updated 4:22 PM PT - Sep 11th, 2026
✅ @robobun, your commit 8255d58591117fcc9e4c2a31ae52e76b5f781aa8 passed in 🧪 To try this PR locally: bunx bun-pr 42379That installs a local version of the PR into your bun-42379 --bun |
|
Status: fix pushed, waiting for CI. Reproduced on 1.4.3-canary.1 (6a92015) with a The new case in |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review. WalkthroughThe CLI now treats ChangesConcurrent Scripts Configuration
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The zero-value concurrency flag now uses normal default scheduling and is covered for installation and trust flows. No actionable merge risk remains. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — one-line parser fix that maps --concurrent-scripts=0 to the default, matching bunfig's existing concurrentScripts = 0 handling.
What was reviewed:
- Confirmed
.filter(|&n| n != 0)mirrorssrc/bunfig/bunfig.rs:1364and thatNonefalls through to the CPU-count default; unparseable/negative values already returnedNoneviaparse_int::<usize>().ok(). - Checked the fix covers the whole class: parsing is shared, so both
installandpm trustare fixed at the source; the test exercises both commands. - Test follows file conventions (reuses
createPackagesWithScripts,touchvia Bun shell like neighboring tests), drains pipes concurrently, and assertssignalCode: nullso the hang-guard timeout produces a real failure rather than a pass.
Extended reasoning...
Overview
The PR changes one expression in src/install/PackageManager/CommandLineArguments.rs: after parsing --concurrent-scripts, it now applies .filter(|&n| n != 0) so a parsed 0 becomes None and the option falls back to the default (2× CPU count). This prevents complete_remaining_scripts and the pm trust wait loop from spinning forever on 0 >= 0. A new test in test/cli/install/bun-install-lifecycle-scripts.test.ts exercises both bun install --concurrent-scripts=0 and bun pm trust --all --concurrent-scripts=0 with a 30s SIGKILL spawn timeout as a hang guard.
Security risks
None. This is CLI option parsing for a concurrency limit; no auth, crypto, path handling, or untrusted-data parsing is touched. The value is a usize and the only new behavior is treating 0 the same as "unset".
Level of scrutiny
Low. The functional change is a single .filter() on an Option<usize>, and it brings the CLI flag into parity with the bunfig.toml path at src/bunfig/bunfig.rs:1364, which already does if n == 0 { None } else { Some(n) }. The failure mode being fixed (busy-spin on 0 >= 0) is well-explained and the fix is placed at the parsing layer that both affected commands share, satisfying the "fix the whole class" rule without touching each wait loop.
Other factors
The test is placed in the existing lifecycle-scripts test file inside the forceWaiterThread describe block, reuses the file's createPackagesWithScripts helper and the same touch postinstall pattern already used by neighboring tests, drains stdout/stderr/exited via Promise.all, and asserts a combined {err, out, signalCode, exitCode} object with signalCode: null — so if the hang regresses, the SIGKILL timeout fires and the assertion fails rather than silently passing. It also verifies the positive contract (postinstall.txt written for the trusted dep, then for the untrusted one after pm trust). No CODEOWNERS entries cover these paths, no prior review threads exist, and the bug hunt exited on dry_streak with no findings.
|
This is Claude (an AI assistant) writing on Jarred's behalf. Included in #44968. |
Problem
bun install --concurrent-scripts=0never exits once a dependency lifecycle script is queued. One core spins until the process is killed.bun pm trust --concurrent-scripts=0does the same.0(src/install/PackageManager/CommandLineArguments.rs:1604). With a limit of 0 no script can start, so each one is deferred.complete_remaining_scripts(src/install/PackageInstaller.rs:920) then loops onwhile alive_count >= max_concurrent_lifecycle_scripts { manager.sleep() }. That is0 >= 0, andsleep()returns at once because nothing is alive.src/runtime/cli/pm_trusted_command.rs:472has the same loop.Fix
0toNone. The limit then falls back to the default, 2x the CPU count.[install] concurrentScripts = 0(src/bunfig/bunfig.rs:1364), and the docs call the two settings equivalent. The flag already falls back to the default for a value it cannot parse, for exampleabc.test/cli/install/bun-install-lifecycle-scripts.test.tsrunsbun installandbun pm trust --allwith the flag. Release 6a92015 fails it (killed at the 30 s spawn timeout). The fixed debug build passes in about 1 s.Background
preinstall,install, orpostinstallcommand from a dependency's package.json. Bun runs them for trusted dependencies only.--concurrent-scriptscaps how many of these child processes run at once.LifecycleScriptSubprocess::alive_count()counts the live ones.complete_remaining_scripts, which waits for a slot withmanager.sleep()(one event loop tick).Notes
0with an error, the way--network-concurrencyrejects a value that is not a number. This PR picks parity with bunfig.toml. An error is a one-line change if that is preferred.get_thread_count()is clamped to a minimum of 2.file:dependency):bun pm trust dep --concurrent-scripts=0on release 6a92015: also killed by the timeout (exit=137). The new test runs it after the install, so on an unfixed build the test stops at the install.--linker=isolated --concurrent-scripts=0did not hang before.bun bd teston the whole file: 121 pass, 3 fail ("node -p should work in postinstall scripts" and "ensureTempNodeGypScript works" x2). Those 3 fail the same way on main underbun bd test, becausebun runexportsNODEto the test process. install: stop debug builds from wiping the shared node shim dir on every start #37384 covers them. They pass with a release build.[install] concurrentScriptsin bunfig.toml overrides the--concurrent-scriptsflag (src/install/PackageManager/PackageManagerOptions.rs:569).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-lifecycle-scripts.test.ts