Repository navigation
Conversation
…nd wait for them A SIGTERM, SIGINT or SIGHUP sent to `bun install` while a lifecycle script runs ended `bun install` at once by the default action. The script kept running, reparented to init, and kept writing into node_modules. While at least one lifecycle script runs, `bun install` now hooks the three signals. The hook (`bun_spawn::exit_signals`) records the signal and runs a callback on the install thread through the event loop. The callback forwards the signal to every running script and unhooks, so a second signal takes the default action. No new script starts and an exiting script does not chain into its next one. When the last script has exited, `bun install` dies by the same signal, as it already does when a script dies by a signal on its own. On Linux, lifecycle scripts are also spawned with PR_SET_PDEATHSIG set to SIGKILL, like git children already are, so a script does not outlive a `bun install` that had no chance to forward anything.
|
Warning Review limit reached
On-demand reviews are free for the next 12 days. After that, they cost $0.25 per reviewed file. Or wait 12 minutes for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (8)
Comment |
|
Updated 4:52 AM PT - Sep 8th, 2026
❌ @robobun, your commit b5465a2 has 1 failures in 🧪 To try this PR locally: bunx bun-pr 41921That installs a local version of the PR into your bun-41921 --bun |
|
Status Reproduced on bun 1.4.3 (Linux x64), no network needed: D=$(mktemp -d); cd $D
printf '{"name":"app","version":"1.0.0","scripts":{"postinstall":"echo hook pid $$ >> log; exec sleep 40"}}' > package.json
for SIG in TERM INT HUP; do
rm -rf node_modules bun.lock; env --default-signal=INT bun install >out 2>err & B=$!; sleep 1.5
KID=$(cat /proc/$B/task/*/children); kill -$SIG $B; wait $B; rc=$?; sleep 0.3
echo "SIG$SIG -> rc=$rc; hook $KID: $([ -e /proc/$KID ] && echo STILL RUNNING ppid=$(awk '{print $4}' /proc/$KID/stat) || echo gone)"
kill -9 $KID 2>/dev/null
done
# before: rc 143 / 130 / 129, hook STILL RUNNING ppid=1 every time
# after: rc 143 / 130 / 129, hook gone every time (it received the signal first)Verified with the debug build:
CI (build 112758, head |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…hout &mut MiniEventLoop, drain stderr from the start in the test
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Re-reviewed after e820e7a — no new issues found. Given the async-signal-safe handler path and the raw-pointer MiniEventLoop enqueue, a maintainer sign-off on the signal-safety and aliasing invariants is still worthwhile.
What was reviewed:
on_script_exitednow unhooks then readsreceived()directly, so a signal landing during the last script's exit handling is no longer lost — theDRAININGgate is gone.enqueue_task_concurrent_rawuses*const Self+addr_of!field projection, avoiding a&mut MiniEventLoopalias with an interruptedtick.RECEIVEDis never cleared across an unhook/re-hook cycle, so a queued task from a prior hook still finds its signal;SIG_IGNinheritance is preserved.- Tests now
stdout: "ignore", start draining stderr before waiting, and racewaitForFileagainstproc.exitedso a pre-hook crash surfaces as an assertion.
Extended reasoning...
Overview
This PR makes bun install forward SIGINT/SIGTERM/SIGHUP to running lifecycle scripts on Unix, wait for them, and re-raise the same signal, plus sets PR_SET_PDEATHSIG=SIGKILL on Linux so scripts cannot outlive a killed installer. It adds src/spawn/exit_signals.rs (async-signal-safe handler that records the signal and posts a preallocated task via a new raw-pointer MiniEventLoop::enqueue_task_concurrent_raw), src/install/lifecycle_signals.rs (running-count bookkeeping, forward-and-drain, die-by-signal), an Intrusive::for_each heap traversal, wiring in lifecycle_script_runner.rs, and a POSIX-only test file.
Three earlier review rounds surfaced: (1) RECEIVED being cleared on re-hook swallowing a queued signal, (2) the handler forming &mut MiniEventLoop while tick holds one, (3) undrained subprocess pipes and unbounded polling in tests, and (4) pending() gated on DRAINING so a signal caught during the last script's handle_exit was dropped. Commits f8556c9 and e820e7a addressed all four: RECEIVED is now write-once-never-cleared, the enqueue path takes *const Self with addr_of! field projection, tests set stdout: "ignore" and race the pid-file wait against proc.exited, and pending() now reads received() directly with unhook() ordered before the check so no window drops the signal.
Security risks
No auth/crypto/network surface. The signal handler itself is the sensitive part: it runs in async-signal context and touches only atomics, a lock-free queue push, and us_wakeup_loop (eventfd write / kevent), which are async-signal-safe. SIG_IGN inherited from a parent (nohup, backgrounded shell) is left intact. bun_core::Mutex is only taken from the event-loop thread (hook/unhook/run_task), never from the handler. No user-controlled input reaches the new code paths.
Level of scrutiny
High. This is process-lifetime signal handling with unsafe blocks, raw-pointer dereferences of PackageManager from a callback, atomic ordering across a signal-interrupted thread, and a new *const Self FFI-adjacent enqueue on the event loop. REVIEW.md flags thread-affinity, aliasing, and async-signal safety as most-blocked categories. The change is well-reasoned and the PR body documents the invariants, but a maintainer should confirm the UnboundedQueue::push implementation is genuinely async-signal-safe (no hidden allocation) and that us_wakeup_loop is safe to call while the same thread is inside the usockets poll.
Other factors
The test file is new rather than added to bun-install-lifecycle-scripts.test.ts, but the signal-forwarding scenario is distinct enough (POSIX-only, subprocess-kill choreography) that a separate file is defensible. waitForExit still has no explicit deadline, but it only runs after proc.exited and relies on the framework timeout as the ceiling — acceptable given it was flagged optional. The for_each heap traversal is recursive over at most a handful of concurrent scripts, so stack depth is not a concern. This is the fourth review pass; all bot-raised issues have corresponding fix commits, and no human reviewer has left an outstanding objection.
Problem
bun installwhile a lifecycle script runs (CI cancellation,docker stop) endsbun installat once. The script keeps running, reparented to init, still writing intonode_modules.src/install/lifecycle_script_runner.rsspawns scripts on the install event loop, and nothing sets a disposition for these signals. npm forwards TERM and INT and waits.Fix
bun installhooks INT, TERM and HUP through a new shared module,bun_spawn::exit_signals. It records the signal and runs a callback on the event loop thread. The install half forwards the signal to every running script and unhooks, so a second signal takes the default action. No new script starts. When the last script exits,bun installdies by the same signal.PR_SET_PDEATHSIGSIGKILL, as git children already do. A script does not outlive abun installthat could not forward anything.MiniEventLoop::enqueue_task_concurrent_raw, no&mutto the loop). All package manager access runs on the install thread.test/cli/install/bun-install-lifecycle-signals.test.ts(5 tests, fail on 1.4.3, pass here),bun-install-lifecycle-scripts.test.ts(122 pass).Background
bun install, tracked in the intrusive heapPackageManager.active_lifecycle_scripts. The install waits for them by ticking aMiniEventLoop, a JS-free event loop over usockets.MiniEventLoop::enqueue_task_concurrentis the cross-thread entry: anUnboundedQueuepush plusus_wakeup_loop(an eventfd write or a kevent). Both are safe in a signal handler, and the wakeup makes the poll return. The new_rawform does the same through a raw pointer.PR_SET_PDEATHSIGasks the Linux kernel to signal a child when the thread that spawned it dies.Notes
preinstall/install/postinstall/prepareand a trusted dependency's scripts (node-gyp builds, prisma generate, binary downloaders). A terminal Ctrl-C was never affected: the tty signals the whole foreground process group.SIG_IGN(nohup, a&job in a non-interactive shell) is left alone.bun_spawn::exit_signalsis that primitive. Thebun test --parallelcoordinator (Coordinator.rs, INT/TERM today, no HUP) and the--filter/--parallelrunners (run: act on SIGINT, SIGTERM and SIGHUP at once in --filter and --parallel/--sequential #41478) use the same loop wakeup and can move onto it. This PR does not touch them.exit(128+n), throughGlobal::raise_ignoring_panic_handler, the pathlifecycle_script_runner.rsalready takes for a script killed by a signal. cli: exit 128 + signo when bun cannot die from a re-raised signal (bun as container PID 1) #38877 covers the PID 1 case where that re-raise cannot land.bun installby the default action. On Linux the scripts then die with it (pdeathsig). This matches the coordinator and run: act on SIGINT, SIGTERM and SIGHUP at once in --filter and --parallel/--sequential #41478.bun installembedded in a JS event loop (AnyEventLoop::Js) nothing is hooked; the runtime owns process signals there.preinstall→install→postinstall), andspawn_package_scriptsstarts nothing new.Intrusive::for_eachinsrc/io/heap.rsis a pre-order walk overchild/next. The heap holds at most a handful of nodes.PR_SET_PDEATHSIGcovers the direct child only (it is cleared on fork), the same limit the git children have (git_runner.rs).file:dependencies each runningexec sleep,kill -TERMon bun: both scripts get TERM, rc 143, nothing left alive. Isolated linker,kill -INT: rc 130, nothing left alive. Isolated linker,kill -KILLon bun: both scripts gone (pdeathsig).cargo check -p bun_install -p bun_spawnpasses for x86_64-pc-windows-msvc, aarch64-apple-darwin and x86_64-unknown-freebsd.cargo clippyis clean.no test proof · iteration 2 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/cli/install/bun-install-lifecycle-signals.test.ts