Repository navigation
Conversation
|
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 (6)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. WalkthroughThe shell runtime now captures SIGTERM and SIGHUP, forwards them to active subprocesses, interrupts command sequences, restores signal dispositions, and reports signal-derived exits. POSIX tests cover forwarding, exit codes, sequencing, direct termination, and inherited ignored signals. ChangesShell signal handling
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The change adds signal forwarding and termination handling for Bun shell runners with targeted POSIX coverage; no actionable merge-blocking 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}
…mmands A signal sent to the pid of bun run --shell=bun, bun exec, or bun x.sh ended bun at once and left the command it was running reparented to init. The runner now records the signal, forwards it to every live subprocess, waits for them, runs no further command, and exits the way the system-shell path does. With no subprocess alive the signal ends the runner at once, as before.
b993a16 to
892be33
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| /// The signal the script was ended by: one arrived and was forwarded, and the | ||
| /// script's exit code is the one a shell reports for a command that signal | ||
| /// killed. A command that handled the signal and exited on its own terms | ||
| /// keeps its exit code. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // `forward_signal` runs between ticks, so no task holds a borrow of | ||
| // the node arena. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Send `sig` to every subprocess that is still running (see | ||
| /// `forward_signals`). Every command the script runs, pipeline members and | ||
| /// command substitutions included, is a `Cmd` node in the arena. Call | ||
| /// between ticks only: no callback holds a borrow of a subprocess then. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// where it is instead of running its next command. After a termination | ||
| /// signal (see `forward_signals`) every sequence stops, so the script ends | ||
| /// with the commands that received it. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
|
Updated 10:03 PM PT - Sep 7th, 2026
⏳ @robobun, your commit 839753f is still building in
|
| //! SIGTERM / SIGHUP sent to the pid of a bun-shell script runner (`bun run | ||
| //! --shell=bun`, `bun exec`, `bun x.sh`): recorded here, forwarded to the live | ||
| //! subprocesses by the interpreter between ticks, which then waits for them and | ||
| //! starts no further command. SIGINT is `bun_spawn::ctrl_c`'s (the terminal | ||
| //! signals the whole process group, so there is nothing to forward). |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
Problem
bun run --shell=bun <script>,bun exec '<cmd>'orbun x.shkills bun at once (exit 143, no message) and leaves the running command reparented to init.docker stopandkillorphan the script. The system-shell path forwards the signal and waits (Bun__registerSignalsForForwarding,c-bindings.cpp:1091).Interpreter::init_and_run_impl) only installs the Ctrl+C deferral (src/spawn/ctrl_c.rs). SIGTERM and SIGHUP staySIG_DFL. As container PID 1 the kernel drops them and the runner never stops.Fix
src/runtime/shell/forward_signals.rshooks both signals around the tick loop (an inheritedSIG_IGNis left alone). The handler records the signal and wakes the loop. Between ticksInterpreter::forward_signalsends it to every live subprocess and keeps waiting.Interpreter::interrupted). With no subprocess alive (a builtin, or nothing) the handler re-raises withSIG_DFLat once,_exit(128 + n)if PID 1 drops it.bun runthen matches the system-shell path: exit code128 + signalprintsscript "x" was terminated by signal SIGTERMand re-raises. A command that handles the signal keeps its exit code.test/cli/run/run-shell.test.ts(6 new tests, 4 fail on 1.4.3 with the child alive), shell andbun runsuites,cargo checkfor linux, darwin, windows. Self-reviewed: 2 concerns, both addressed (Notes).Background
--shell=bun, bunfig[run] shell = "bun", or on Windows by default,bun runinterprets scripts in-process onMiniEventLoop, a JS-free usockets loop. Each external command is aCmdnode owning aShellSubprocess.us_wakeup_loop(eventfd write, mach message on macOS) takes no lock, so it is signal-safe.sh -c <one command>execs the command, so the system-shell path's signal lands on the command itself. SIGINT stays unforwarded: the terminal sends it to the whole group (ctrl_c.rs), whoseChild::alive()counter the handler reads.Notes
Reproduction (bun 1.4.3):
With this branch (debug build):
bun run --shell=bun srv, SIGTERM: printserror: script "srv" was terminated by signal SIGTERM (Polite quit request), dies by SIGTERM (rc 143),sleepgone. SIGHUP: rc 129, same message with SIGHUP.bun exec 'sleep 30'andbun s.sh(sleep 30): rc 143,sleepgone.sleep 30 | sleep 30: both members receive the signal.sleep 30; echo after,sleep 30 && echo after,sleep 30 || echo after: stop atsleep,afteris not printed, rc 143.bun runreportsexited with code 7, rc 7.SA_RESETHAND). The system-shell path behaves the same way.bun exec 'echo ready; yes > /dev/null'(builtin only, never yields to the event loop): SIGTERM ends it at once, death by signal.sh -c 'trap "" TERM; exec bun run --shell=bun wait': SIGTERM stays ignored by the runner and the child, the script completes, rc 0.Review follow-ups:
installnow runs beforeinterp.run(), which spawns the first command before it returns, so no window is left where a child exists and the handler is not armed. Background jobs (&) are rejected by the parser today, so thectrl_c::Childforeground count is the full set of subprocesses that can exist.Self-review concerns and what changed:
ctrl_c::Child::alive() == 0. A builtin can hold the main thread without returning to the event loop (yes > /dev/null), so this cannot wait for a tick.a; bwould have continued tobafteradied from the forwarded signal.Interpreter::interruptednow stops every sequencing state (Script,Stmt,Binary,If) once a signal was received.The handler saves and restores
errnoaround the wakeup write.PREVIOUSdispositions live in aRacyCell<MaybeUninit<[sigaction; 2]>>, written on the CLI thread before the hook and read after the unhook.PID 1: installing a handler is what makes the kernel deliver SIGTERM to init at all, so the runner now reacts there too. The final re-raise of
bun rungoes throughGlobal::raise_ignoring_panic_handler; #38877 turns a dropped re-raise into_exit(128 + signo)inside that function. The handler's own no-subprocess path already has the fallback. Not run here:unshare --pidis not permitted in the development container.Windows:
installanduninstallonly record the loop pointer. Console control events keep going throughbun_spawn::ctrl_c. The new tests are skipped there (proc.kill()isTerminateProcess).Related, not overlapping: #41478 does the same for
--filter/--parallel(a different runner), #38877 covers the PID 1 exit status of the re-raise.Suites run against the debug build:
test/cli/run/run-shell.test.ts(9 pass, 3 runs),test/js/bun/shell/exec.test.ts,bunshell.test.ts,bunshell-file.test.ts,test/cli/run/run_command.test.ts,test/cli/run/shell-keepalive.test.ts.test/js/bun/shell/shell-hang.test.ts(700 ms budgets) and four--buncases oftest/regression/issue/ctrl-c.test.tstime out in this container on main as well, against the ASAN debug build. Neither touches this path.