Repository navigation
Conversation
On POSIX, Process::kill sent a signal only while the process was watched (Poller::Fd or Poller::WaiterThread). A process started as Poller::Detached, and Bun.spawnSync reads the child's stdout and stderr and attaches its AbortSignal before watch_or_reap() arms the poller. A kill requested in that window returned Ok without a kill(2): a child that was over maxBuffer at the first read got no signal, and spawnSync waited for it to end by itself. A spawned child that is not reaped and not watched yet now has its own poller state, Unwatched, and kill signals it. Detached keeps one meaning: exited and reaped, or given up on, so the pid can be reused and kill does nothing.
|
Updated 11:45 PM PT - Oct 19th, 2026
✅ @robobun, your commit 34755ccbd03d7dfbb3e0dbe4c55d811a9b5d67f2 passed in 🧪 To try this PR locally: bunx bun-pr 46133That installs a local version of the PR into your bun-46133 --bun |
|
Status: ready for review. PR: #46133. CI is green: build #129199, 181 of 181 jobs pass. The one red check, How I reproduced it (Linux x64, release build of main):
With this branch, each of these ends with the kill signal on every run. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughPOSIX process initialization now distinguishes live, unreaped children from detached or reaped processes. Kill handling signals unreaped children. Spawn tests cover AbortSignal and max-buffer termination, reaped-child signaling, and cleanup behavior. ChangesPOSIX child signaling
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
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 findings, I also checked that the Unwatched arm cannot signal a reaped pid: the only write to status is in on_exit, which runs detach() -> close() -> Poller::Detached whenever the status is exited, so a process still in Unwatched is still Running and the debug_assert invariant holds on release builds too. Every other match/if let on the poller in process.rs uses a wildcard or if let, so no other site needs the new variant, and the watch() failure path that leaves the poller in Fd is pre-existing, not introduced here.
Extended reasoning...
The change adds an Unwatched variant to the POSIX poller enum in src/spawn/process.rs so Process::kill signals a spawned-but-not-yet-watched child (the spawnSync maxBuffer/AbortSignal case), keeps Detached as a no-op, and adds LD_PRELOAD-shim tests covering stdout/stderr/node:child_process with pidfd and waiter-thread. It touches process-lifecycle and signal delivery semantics, no injection/auth surface. The posted double-signal finding and the author's own open question about the intended Process::kill contract mean a maintainer should weigh in, so this is not an approve.
|
A second branch reached the same fix on its own: Diff summary: the same Three results from that branch that this PR can use:
Release builds of main
On |
Problem
Bun.spawnSynccan fail to signal a child overmaxBuffer.spawn-maxbuf.test.ts"yes is killed > Bun.spawnSync" fails withexpect(proc.exitCode).toBe(null),Received: 1, in 16 of 30 runs. A child that stops writing blocks the call.Process::kill(src/spawn/process.rs:777) signalled only a watched process. spawnSync reads stdout and stderr and attaches itsAbortSignalbefore it watches. A kill requested there returnedOk(())with nokill(2).Fix
Unwatched, andProcess::killsignals it. Nothing has waited for that child, so the pid is still its own.killstill never signalsDetached(reaped, or given up on).spawn-pipe-start-error.test.ts,spawn-kill-signal.test.ts) fail on main and pass here. Two guard cases pass on both.Background
Processthat its child exited (pidfd, kqueue or waiter thread).watch()arms it. Windows has it from spawn.Bun.spawnwatches before it reads.Process::killstill drops other owners' requests. Considered a signal unless exited (spawn: kill child on POSIX when poller is still .detached #30079): a given-up child has not exited either, and its pid can be reused.killleaves alone what is not watched". @Jarred-Sumner, is this theProcess::killcontract you want?Downsides
maxBufferthat reaches a second limit always getskillSignaltwice. Node sends it once. Withtimeout: 100: 20 of 20 runs, main 4 of 20.Process: +4 instructions, +0 branches. Syscalls with no kill request: +0.Notes
Mechanism
Process::init_posixstarted every process asPoller::Detached.watch()replaces that withFdorWaiterThread.Process::killsent a signal only in those two states.spawn_maybe_sync(src/runtime/api/bun/js_bun_spawn_bindings.rs) the sync path reads stdout (:1665) and stderr (:1676) and callsAbortHandle::follow_owner(:1729) beforewatch_or_reap()(:1760).maxBuffer,Subprocess::on_max_bufferasks for the kill,Process::killdoes nothing, and the reader then closes the pipe. Nothing signals the child later.bun exec yesgets EPIPE on its next write and exits 1. A child that does not write again runs on.vfork()can return in the parent 1 to 2 ms after the child started, and the child has written by then.WaiterThread | Fd | Detachedwidening, nine minutes after it. This PR does not make that widening:Detachedstays a no-op, which is the pid-reuse rule of spawnSync: stop pointing the VM at the loop it waits on #44581. The "async-path hang" the comment mentions is not claimed here.Reproduction (release builds of one tree, ad4089b, with and without this change to
process.rs)spawn-maxbuf.test.ts -t "yes is killed": the spawnSync case fails in 16 of 30 runs without the change and in 0 of 70 with it.Bun.spawnSync([bunExe(), "exec", "yes"], { maxBuffer: 256, killSignal: "SIGHUP" }), 100 calls: 79 end withexitCode: 1without the change. 100 end withsignalCode: "SIGHUP"with it.Bun.spawnSync(["sh", "-c", "head -c 100000 /dev/zero; sleep 1"], { maxBuffer: 256 }): on 1.4.3-canary, 6 of 20 calls block the full second and returnexitCode: 0withexitedDueToMaxBuffer: true. Throughnode:child_processit is 5 of 20. Node: 0 of 20.AbortSignalthat fires while the options are read (a getter on a later option) is deterministic:spawnSync(["sleep", "2"])takes 2003 ms and returns exit 0.Bun.spawnis killed in 47 ms.Tests
spawn-pipe-start-error.test.tshas an LD_PRELOAD shim forepoll_ctl. New modeWAIT_FOR_OUTPUT=n: the nth registration of a readable unix socket waits until the child has written, and reports the pending bytes. This puts the overflow into the first read every time. The 7 cases assertstderris1000 bytes of output before the first read.child_process.spawnSync, each with a pidfd and with the waiter thread, and one where the pidfd registration fails after the kill.kill()of a pid thatwait4()returned. Everystderr: ""in the file therefore asserts that no reaped pid was signalled. "the shim reports a kill() of a pid that wait4() returned" is the control for that report.Detachedno-op:bun test --parallelwith a failing pidfd registration asks for a kill of a pid thatkill_and_reapalready reaped. With a temporary "signal unless exited" arm this case fails withkill(pid, 9) after wait4() returned that pid.spawn-kill-signal.test.ts: theAbortSignalcase above.bun bdand on a release build of this branch. The two guard cases pass on main.init_posixarm that starts a child reaped at birth asDetached. No test reaches it. It is kept because that pid is already reaped.Measurements (Linux x64)
sleep 0.2: 14 or 15 on both builds.echo hi: 11 to 11. Blocking fast path: 0 to 0).EPOLL_CTL_DELand before the watch, in 50/50 stdout, 50/50 stderr and 50/50 abort runs with a pidfd, and 50/50, 50/50, 50/50 with the waiter thread. 0 kill calls in 20 runs with no request. Without the change: 0 kill calls in 300 runs.wait4returned: 0 (main 0) over the spawnSync give-up cases andbun test --parallelwith a failing pidfd registration. Children left: 0 and 1 (main 0 and 1).Process::killon anUnwatchedprocess, logged over 86 test files (spawn, child_process, shell,--filter,--parallel, install lifecycle scripts,bun test --parallel): 5 hits from 1 call site,Subprocess::try_kill, all in the newBun.spawnSynccases.size_of::<PollerPosix>(): 16 to 16 bytes.size_of::<Process>(): 128 to 128. Shallow size of aSubprocess: 728 to 728.size).Process::kill156 to 187 B.kill_and_reap238 to 238 B (nm -S).Processconstruction (init_posix, inlined intoto_process): +4 instructions, +0 conditional branches (objdump).bun run rust:check-all: 12 of 12 targets pass.watch()sendswatch_or_reapinto its existing ESRCH andreap_on_threadarm. In CI build #129199,spawn-kill-signal.test.ts(theAbortSignalcase) andspawn-maxbuf.test.tspass on darwin x64 and aarch64. All 181 jobs of that build pass.The second signal (Downsides)
maxBuffer, 50 runs. Without the change: 37 get no signal, 6 get one, 7 get two. With it: 34 get one, 16 get two. Node: 50 get one.maxBuffer, thentimeout: 100, 20 runs. Without the change: 4 get two. With it: 20 get two. Node: 20 get one.SyncProcessRunner::Kill()acts once.Consumers of
Process::killUnwatchedprocess when they ask:Bun.spawnSync(the three sites above) andWorker::startinbun test --parallel.Worker::start's error guard (src/runtime/cli/test/parallel/Worker.rs:104) now delivers its SIGKILL when the worker's setup fails before the worker is watched, for example when its IPC channel cannot be attached. No test reaches that. The worker is still not reaped, as before.kill_and_reapis unchanged. It signals aDetachedprocess too: its one caller that runs before a watch (js_bun_spawn_bindings.rs:1396) has closed the process by then on Linux.wait4and the task that reports it,killcan still signal a reaped pid. spawn: reap on the owning event loop in the waiter-thread fallback #36188 and spawn: report a waiter-thread child exit after the loop's next poll #43888 are open PRs on the waiter thread.Not in this PR
Unwatchedchild without a reap: the security scanner when its JSON pipe writer fails to start (src/install/PackageManager/security_scanner.rs), and the test worker above.on_max_bufferdiscards the result oftry_kill(subprocess.rs:677). Node falls back to SIGKILL when the signal is refused.Bun.spawncallswatch()on a child that was reaped at birth.recvfromand 2EPOLL_CTL_MODper piped spawnSync whose child has not written yet.spawn-maxbuf.test.ts(test(spawn-maxbuf): give the elapsed-time upper bounds room for a loaded CI lane #39699) andlazywithmaxBuffer(spawn: enforce maxBuffer when lazy is set #42256) are other failures. This PR fixes the sync case only.Other suites, debug build
test/js/bun/spawn(44 files): 341 pass, 6 fail in 3 files.test/js/node/child_process,filter-workspace,multi-run,test/cli/test/parallel*, 28 shell files andbun-install-lifecycle-scriptsalso ran.spawnsync-isolated-event-loop(2 or 3 fail on both),spawn-stdin-readable-stream(0 to 3 on both),child_process.test.ts(6 here, 7 on main),multi-run(0 to 7 here, 2 to 4 on main),parallel.test.ts(1 or 2 on both),leak.test.ts,shell-hang,shell-leak-args,shell-load.spawn-stdio-syscall-errortimed out in 3 cases in 2 of 5 runs here and in 0 of 6 runs on main. Those cases take 2.0 to 4.7 s on both builds.test-child-process-*node tests (spawnSync, maxbuf, kill-signal, timeout) exit 0.Self-review
UnwatchedtoDetachedinsidekill_and_reap. That statement never ran on Linux (the caller closes the process first), so it is not in the diff.