Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe SIGINT integration tests replace the shared Vite fixture with a local long-running package. They test its binary and package-script entry points, with and without ChangesSIGINT integration tests
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to
|
|
Updated 1:36 PM PT - Oct 18th, 2026
✅ @robobun, your commit 594b8a2d6e8c9ba2f578af0c6aef4d26b31cf505 passed in 🧪 To try this PR locally: bunx bun-pr 45542That installs a local version of the PR into your bun-45542 --bun |
|
Status: ready for review. CI is green on 594b8a2. Reproduction: point CI (build 128422): all 181 jobs passed. PR: #45542 |
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked whether the fixture swap narrows what the six cases exercise: the file: dependency still goes through bun install bin linking, the #!/usr/bin/env node shebang matches vite's own bin (so the node-on-PATH requirement and the --bun substitution path are unchanged), and the 300ms Promise.race grace window at test/regression/issue/ctrl-c.test.ts:155 is pre-existing and untouched by this diff.
Extended reasoning...
The change is confined to one regression test file, replacing a registry install of vite with a local file: package whose bin prints a line and idles; it touches no security-sensitive surface. The launch-mode matrix and the post-SIGINT assertions are unchanged, and the dropped hardcoded PORT is a straightforward improvement. The one inline finding (no cleanup of the spawned process on assertion failure) is a pre-existing shape that the new 60s timer actually bounds, so it is a note rather than a blocker.
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
LGTM. The second commit addresses the process-leak point from my earlier review: the spawn is now await using registered before the assertions, and the dispose path in subprocess.rs sends kill_signal only when the process has not already exited, so it cannot affect the signalCode: "SIGINT" assertions on the success path. The file: fixture with a bin is a pattern the install suite already exercises, and the cwd-folder-shadowing note in the comment matches the resolve-before-.bin order in run_command.rs.
Extended reasoning...
The diff touches one test file, test/regression/issue/ctrl-c.test.ts, swapping an unpinned registry install of vite for a local file: dependency with a bin and binding the spawned process with await using plus killSignal SIGKILL; it touches no production code and no security-sensitive surface. The six launch modes and their platform-branched assertions are unchanged, the hardcoded PORT env was dropped since nothing listens anymore, and the only reported finding is a pre-existing 300ms race the diff did not introduce. The prior review's one concern was addressed by the follow-up commit, no CODEOWNERS entry covers the path, and the hunt ran dry, so a human look is not needed.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
test/regression/issue/ctrl-c.test.ts— pre-existing nit: on a slow runner (debug/ASAN CI) all six SIGINT cases can fail withkilled: false, exitCode: null, signalCode: nulleven though bun behaves correctly. The wait at test/regression/issue/ctrl-c.test.ts:159 isPromise.race([proc.exited, Bun.sleep(300)]), a fixed 300ms sleep rather than awaiting the condition, and the comment does not say why no observable signal exists. Fix: awaitproc.exited(the test runner's own timeout bounds a hang) or poll with a deadline well above 300ms, keeping the same{killed, exitCode, signalCode}assertion; if a fail-fast bound is wanted, the comment should state thatexitednever resolves on the regression and that is why a sleep is used.Why this was flagged
The trigger is a CI runner where the spawned
bun runparent takes more than 300ms to forward SIGINT, reap its child and re-raise the signal, which happens on loaded debug+ASAN lanes. The test at test/regression/issue/ctrl-c.test.ts:156 sends SIGINT and then at :159 racesproc.exitedagainstBun.sleep(300); when the sleep wins,proc.exitCodeandproc.signalCodeat :163-164 are still null and thetoEqualat :161 fails although the runtime is not broken. The base branch has the identical race (it was carried over from the vite version), so this is pre-existing; the diff keeps it and addsawait usingwith SIGKILL at :138-145, so a lost race now also SIGKILLs the parent, leaving the orphaned child until its 60s timer at :112. The repository review rules ask that a literal sleep of 50ms or more outside a bounded poll loop carry a comment naming why no observable signal exists; the comment at :158 only says it is the same grace period as before. A correct wait awaitsproc.exitedor polls with a deadline, so a slow but correct bun passes.Verification: pre-existing. Trigger: a runner where the spawned
bun runparent needs more than 300ms to forward SIGINT. test/regression/issue/ctrl-c.test.ts:159 isawait Promise.race([proc.exited, Bun.sleep(300)]);, a fixed sleep rather than awaiting the condition. When the sleep wins, the assertion at :161-177 fails even though bun forwarded the signal correctly. The base file has the identical line, so merging makes nothing worse.
|
On the 300 ms grace after SIGINT: it is unchanged from main, on purpose.
If the grace starts to flake with the lighter fixture, a poll with a longer deadline is the right change, in its own PR. |
b3e6382 to
8d0f238
Compare
|
The review of this change surfaced a bug in bun that the vite fixture hid. This PR does not change bun. It only keeps the six cases away from the bug.
The bug has a patch in #38926 (restore the saved mask with |
The six "kills on SIGINT" cases installed unpinned vite from registry.npmjs.org in beforeAll. When npm could not serve one of the dependencies, the install exited 1 and all six cases failed. The cases now install a local file: package. Its bin prints a line and then idles with no SIGINT handler. bun install still links node_modules/.bin, and the test contacts no registry.
A failed case left bun and its child running. The cases now bind the process with `await using`. The kill signal is SIGKILL, because a bun that ignores SIGINT can ignore SIGTERM too. The wait for its exit would then turn the failed assertion into a test timeout.
`bun run` has the pid of its child only when the spawn returns. A SIGINT that comes earlier takes the pending-signal path. That path has a bug that ends in SIGABRT, and the local bin prints soon enough after the spawn for the cases to reach it. Each POSIX case now sends SIGUSR2 first and waits for the answer of the bin. The answer proves that `bun run` has the pid. The wait also stops with a clear error when bun ends before the expected line.
8d0f238 to
594b8a2
Compare
Problem
test/regression/issue/ctrl-c.test.tsinstalls unpinnedvite@^6.0.1from registry.npmjs.org inbeforeAll. When the install fails, all sixkills on SIGINT in: 'bun ...'cases fail atexpect(viteInstallExitCode).toBe(0).error: GET https://registry.npmjs.org/rollup/-/rollup-4.64.6.tgz - 404on seven Linux lanes, minutes after its publish.bun vite#15545 added the install. No commit in bun caused the break.Fix
long-running, a localfile:package. Its bin prints one line and idles with no SIGINT handler.bun installstill linksnode_modules/.binand contacts no registry.bun rundies from the SIGINT that killed its child, not vite. bun 1.1.36, which has the bug they guard (Cannot exit "bun tsc -w" since v1.1.27 #14375), failsbun devunder both fixtures. Verified:bun bd test test/regression/issue/ctrl-c.test.ts, also with the registry unreachable.Background
bun runforwards a signal to its child, but it has the child's pid only when the spawn returns. A SIGINT that comes earlier makes it die from SIGABRT, a bug that vite's slow start hid (Clear the inherited signal mask at startup and exec children with an empty one #38926 has a patch). The SIGUSR2 answer proves that it has the pid.Behaviour change: none
Notes
Build 125085: npm published rollup@4.64.6 at 09:07:46 UTC. The seven lanes got the 404 from 09:08:42 to 09:11:17 UTC, four attempts each. With the registry unreachable, main fails the six cases in the same way (below).
Other builds with the same cause
Cannot find module @rollup/rollup-win32-x64-msvc, thenkill() failed: ESRCH. npm has rollup@4.64.3 from 18:12:28 UTC and @rollup/rollup-win32-x64-msvc@4.64.3 from 18:17:35 UTC. An install between the two gets rollup without its native package.Resolving dependenciesand thebeforeAllhook timed out. The retry passed.Reproduction that does not depend on npm
What
bun installstill does on the network: it starts the DNS prefetch for the default registry host (src/install/PackageManager/install_with_manager.rs:72) and does not wait for the answer. With a resolver that never answers, the install of the fixture takes about 0.2 s on the debug build and sends two DNS queries. It opens no connection to a registry.Why the cases send SIGUSR2 first
The bug, on main:
src/spawn/process.rs:3267-3268setsBun__currentSyncPIDto 0 and registers the forwarding handlers. Then it spawns, and only after the spawn returns does it store the pid.:3344callsBun__sendPendingSignalIfNecessary().bun runbefore the pid is stored is only recorded (src/jsc/bindings/c-bindings.cpp:1093-1097) and is forwarded later, outside the signal handler (:1074-1083).:1069-1071blocks the signal, sends it, and then unblocks the set that was blocked before. Outside a handler that set does not hold the signal, so the signal stays blocked inbun run.src/bun_core/Global.rs:789-790israise(sig), thenabort(). The raise does nothing for a blocked signal, sobun rundies from SIGABRT.SIG_SETMASKof the saved mask). cli: exit 128 + signo when bun cannot die from a re-raised signal (bun as container PID 1) #38877 is a second guard at the raise.What it meant for this file:
bun <mode>to the first line on stdout, vite takes about 250 ms (193 ms at least) and the local bin about 60 ms (35 ms at least), on a release build. Whenbun rundoes not get CPU time in that span, the SIGINT of the test arrives before the pid is stored.bun runfor 100 ms after the spawn: without the SIGUSR2 step the four cases that spawn a child end withsignalCode: "SIGABRT"in every run. With the step all eight tests pass, with a hold of 100 ms and of 300 ms, on a release build and on the debug build.scripts/runner.node.ts:1858). CI shows that today in another test:test/cli/run/run_command.test.ts("a script killed by Ctrl+C stops bun run") failed withReceived: "SIGABRT"in builds 124378, 124477, 124545, 124556, 124759, 125044 and 125184.How the step works:
bun runforwards. The bin answers it with a line. The answer can only come afterbun runhas the pid, on either forwarding path.bun run. The SIGINT that follows takes the normal path.devcases the signal goes to the pid of the shell.bun runuses bash when it is on PATH, and bash execs the script, so the bin has that pid. The CI images install bash (scripts/build/ci-images/spec.ts). dash 0.5.12 does not exec.The local fixture finds the same regression as vite
A standalone copy of the test loop, run against two bun versions on Linux x64:
bun devandbun --bun devend with{killed: true, exitCode: 0, signalCode: null}.bun viteprintserror: Failed to run "vite" due to signal SIGINT.bun devends with the same state.bun long-runningprints the same error.signalCode: "SIGINT".What the old fixture did besides the six cases
process.report.getReport()on Windows. feat: Implement process.report.getReport() on Windows #22816 added it the next day.What each mode runs (
exec_with_cfginsrc/runtime/cli/run_command.rs)bun long-running: there is no script and no file of that name, so the bin innode_modules/.binruns as a child (run_binary_without_bunx_path).bun dev: the package.json script runs through the shell (run_package_script_foreground_with_shell_path).bun ./node_modules/.bin/long-running: bun opens the file and runs it in its own process (maybe_open_with_bun_js).--bunputs bun in the place ofnodefor the shebang.long-running-srcbecausebun long-runninglooks for that path in the cwd first (src/resolver/resolver.rs:1889).Other changes in the diff
PORT: "9876"is gone from the env. Nothing listens on a port now.await usingwithkillSignal: "SIGKILL". Before, a failed case left bun and its child running. The signal is SIGKILL because the dispose waits for the exit. With the default SIGTERM, a process that ignores the signal makes the test time out, and the failed assertion is lost. Measured with a bin that ignores SIGINT and SIGTERM:this test timed out after 5000mswith SIGTERM, the assertion diff after 0.3 s with SIGKILL. The dispose signals only the pid of the spawned bun. A child that outlives it ends with the 60 s timer.Windows: I cannot run Windows locally. On Windows,
process.kill(pid, "SIGINT")terminates bun, and the child outlives it. The test runner then logsEPERMwhen it removes the temp dir. Main does the same with vite (build 125085, both Windows lanes). The difference: the vite server did not end, andlong-runningends after at most 60 s.The rest of the class
test/regression/issuestill install from npm on main: 00631, 026039, 07740, 11806, 24131, 24364, 26225, 26657 and 28193. 24364 adds four unpinned packages and is the most exposed.15276.test.tsandpatch-bounds-check.test.ts, which main has since made local, and seven of its files importVerdaccioRegistry, which test: replace verdaccio with an npm registry on Bun.serve #44055 removed from the harness on 2026-10-12.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/regression/issue/ctrl-c.test.ts