Skip to content

spawn: signal a child that is spawned but not yet watched - #46133

Open
robobun wants to merge 1 commit into
mainfrom
robobun/f6e746b0/spawnsync-kill-before-watch
Open

robobun wants to merge 1 commit into
mainfrom
robobun/f6e746b0/spawnsync-kill-before-watch

Conversation

@robobun

@robobun robobun commented Oct 20, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • On POSIX, Bun.spawnSync can fail to signal a child over maxBuffer. spawn-maxbuf.test.ts "yes is killed > Bun.spawnSync" fails with expect(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 its AbortSignal before it watches. A kill requested there returned Ok(()) with no kill(2).

Fix

  • A spawned, unwatched child has its own poller state, Unwatched, and Process::kill signals it. Nothing has waited for that child, so the pid is still its own.
  • kill still never signals Detached (reaped, or given up on).
  • Verified: 8 new cases (spawn-pipe-start-error.test.ts, spawn-kill-signal.test.ts) fail on main and pass here. Two guard cases pass on both.

Background

Downsides

  • A child over maxBuffer that reaches a second limit always gets killSignal twice. Node sends it once. With timeout: 100: 20 of 20 runs, main 4 of 20.
  • Binary: text +256 B. Per Process: +4 instructions, +0 branches. Syscalls with no kill request: +0.
Notes

Mechanism

  • Process::init_posix started every process as Poller::Detached. watch() replaces that with Fd or WaiterThread. Process::kill sent a signal only in those two states.
  • In spawn_maybe_sync (src/runtime/api/bun/js_bun_spawn_bindings.rs) the sync path reads stdout (:1665) and stderr (:1676) and calls AbortHandle::follow_owner (:1729) before watch_or_reap() (:1760).
  • When the child's output is already there for the first read, the reader charges maxBuffer, Subprocess::on_max_buffer asks for the kill, Process::kill does nothing, and the reader then closes the pipe. Nothing signals the child later. bun exec yes gets EPIPE on its next write and exits 1. A child that does not write again runs on.
  • The deleted comment says that the first read returns EAGAIN, so the overflow is found only after the poller is armed. That is not so: vfork() can return in the parent 1 to 2 ms after the child started, and the child has written by then.
  • ddb764f reverted babbd9d, an unguarded WaiterThread | Fd | Detached widening, nine minutes after it. This PR does not make that widening: Detached stays 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 with exitCode: 1 without the change. 100 end with signalCode: "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 return exitCode: 0 with exitedDueToMaxBuffer: true. Through node:child_process it is 5 of 20. Node: 0 of 20.
  • An AbortSignal that 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.spawn is killed in 47 ms.

Tests

  • spawn-pipe-start-error.test.ts has an LD_PRELOAD shim for epoll_ctl. New mode WAIT_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 assert stderr is 1000 bytes of output before the first read.
  • The 7 cases: stdout, stderr and child_process.spawnSync, each with a pidfd and with the waiter thread, and one where the pidfd registration fails after the kill.
  • The shim now also reports a kill() of a pid that wait4() returned. Every stderr: "" 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.
  • "a test worker that was ended and reaped is not signalled again" pins the Detached no-op: bun test --parallel with a failing pidfd registration asks for a kill of a pid that kill_and_reap already reaped. With a temporary "signal unless exited" arm this case fails with kill(pid, 9) after wait4() returned that pid.
  • spawn-kill-signal.test.ts: the AbortSignal case above.
  • Counts, 10 runs each: the 8 cases fail 80 of 80 times on a release build of main and pass 80 of 80 times on bun bd and on a release build of this branch. The two guard cases pass on main.
  • The 7 shim cases run one at a time. Run together with the rest of that file, tests on a debug build come within 0.1 s of their 5 s timeout.
  • Not covered: the init_posix arm that starts a child reaped at birth as Detached. No test reaches it. It is kept because that pid is already reaped.

Measurements (Linux x64)

  • Syscalls per spawnSync with no kill request: +0 (gdb catch syscall, 5 runs each. sleep 0.2: 14 or 15 on both builds. echo hi: 11 to 11. Blocking fast path: 0 to 0).
  • kill(2) per pre-watch request: exactly one, before the pipe's EPOLL_CTL_DEL and 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.
  • Signals to a pid after its wait4 returned: 0 (main 0) over the spawnSync give-up cases and bun test --parallel with a failing pidfd registration. Children left: 0 and 1 (main 0 and 1).
  • Process::kill on an Unwatched process, 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 new Bun.spawnSync cases.
  • size_of::<PollerPosix>(): 16 to 16 bytes. size_of::<Process>(): 128 to 128. Shallow size of a Subprocess: 728 to 728.
  • Release binary: text +256 B, data +0 B (size). Process::kill 156 to 187 B. kill_and_reap 238 to 238 B (nm -S).
  • Process construction (init_posix, inlined into to_process): +4 instructions, +0 conditional branches (objdump).
  • bun run rust:check-all: 12 of 12 targets pass.
  • macOS and Windows were not run locally. On macOS a kill before watch() sends watch_or_reap into its existing ESRCH and reap_on_thread arm. In CI build #129199, spawn-kill-signal.test.ts (the AbortSignal case) and spawn-maxbuf.test.ts pass on darwin x64 and aarch64. All 181 jobs of that build pass.

The second signal (Downsides)

  • Child: the first SIGHUP starts a 200 ms shutdown, a second one exits 99, no SIGHUP in 2 s exits 7.
  • Both streams over 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.
  • stdout over maxBuffer, then timeout: 100, 20 runs. Without the change: 4 get two. With it: 20 get two. Node: 20 get one.
  • Two signals for two requests is what main does when the overflow is found after the watch. Node's SyncProcessRunner::Kill() acts once.

Consumers of Process::kill

  • 11 POSIX call sites in 8 files. Only two owners can hold an Unwatched process when they ask: Bun.spawnSync (the three sites above) and Worker::start in bun 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_reap is unchanged. It signals a Detached process too: its one caller that runs before a watch (js_bun_spawn_bindings.rs:1396) has closed the process by then on Linux.
  • The window of the waiter thread is unchanged: between the thread's wait4 and the task that reports it, kill can 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

  • Two paths still drop an Unwatched child 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_buffer discards the result of try_kill (subprocess.rs:677). Node falls back to SIGKILL when the signal is refused.
  • The second signal above.
  • Bun.spawn calls watch() on a child that was reaped at birth.
  • The two eager reads cost 2 recvfrom and 2 EPOLL_CTL_MOD per piped spawnSync whose child has not written yet.
  • The elapsed-time bounds of spawn-maxbuf.test.ts (test(spawn-maxbuf): give the elapsed-time upper bounds room for a loaded CI lane #39699) and lazy with maxBuffer (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 and bun-install-lifecycle-scripts also ran.
  • Every failure is a 5 s timeout, and the failing set changes from run to run on a debug build of main too. Files compared against main: 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-error timed 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.
  • 10 test-child-process-* node tests (spawnSync, maxbuf, kill-signal, timeout) exit 0.

Self-review

  • 16 concerns raised, 15 addressed. Not done: opening follow-up issues for the items under "Not in this PR".
  • The design first also moved Unwatched to Detached inside kill_and_reap. That statement never ran on Linux (the caller closes the process first), so it is not in the diff.

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.
@robobun

robobun commented Oct 20, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 11:45 PM PT - Oct 19th, 2026

✅ @robobun, your commit 34755ccbd03d7dfbb3e0dbe4c55d811a9b5d67f2 passed in Build #129199! 🎉


🧪   To try this PR locally:

bunx bun-pr 46133

That installs a local version of the PR into your bun-46133 executable, so you can run:

bun-46133 --bun

@robobun

robobun commented Oct 20, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status: ready for review. PR: #46133. CI is green: build #129199, 181 of 181 jobs pass. The one red check, claude-find-issues, is the issue-search workflow that runs when a PR opens. It does not build or test the change.

How I reproduced it (Linux x64, release build of main):

  • bun test test/js/bun/spawn/spawn-maxbuf.test.ts -t "yes is killed": the Bun.spawnSync case fails in 16 of 30 runs. exitCode is 1 and there is no signalCode.
  • Bun.spawnSync(["sh", "-c", "head -c 100000 /dev/zero; sleep 1"], { maxBuffer: 256 }) blocks the full second in 6 of 20 calls and returns exitCode: 0.
  • Deterministic: Bun.spawnSync(["sleep", "2"], { signal: ac.signal, get timeout() { ac.abort(); } }) returns exit 0 after 2 s. Bun.spawn with the same options is killed at once.

With this branch, each of these ends with the kill signal on every run.

@coderabbitai

coderabbitai Bot commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: oven-sh/bun/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 111e8d9e-6d2f-413f-9347-115a34167dd4
📥 Commits

Reviewing files that changed from the base of the PR and between 3880151 and 34755cc.

📒 Files selected for processing: 3
  • src/spawn/process.rs
  • test/js/bun/spawn/spawn-kill-signal.test.ts
  • test/js/bun/spawn/spawn-pipe-start-error.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.

Walkthrough

POSIX 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.

Changes

POSIX child signaling

Layer / File(s) Summary
Process poller and kill behavior
src/spawn/process.rs
Live children start as Unwatched; already-exited children are reaped and marked Detached. Process::kill signals Unwatched children and leaves Detached processes untouched.
Spawn termination and reaping tests
test/js/bun/spawn/spawn-kill-signal.test.ts, test/js/bun/spawn/spawn-pipe-start-error.test.ts
Tests cover AbortSignal termination with SIGHUP, max-buffer termination, attempts to signal reaped children, and cleanup across pidfd and waiter-thread configurations.

Suggested reviewers: jarred-sumner

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to 34755

The signaling change is mergeable after normal checks; no concrete unresolved failure was established.

Pre-merge checks: Passed 4
Check name Status Explanation
Title check Passed The title clearly and concisely describes the primary change: signaling a spawned POSIX child before it is watched.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Description check Passed The description is complete and technically detailed. It explains the problem, fix, scope, known downsides, and verification results. It does not use the template headings exactly, but it provides the…
Full details: Description check

Explanation

The description is complete and technically detailed. It explains the problem, fix, scope, known downsides, and verification results. It does not use the template headings exactly, but it provides the required information under equivalent sections.

✨ Finishing Touches: 💡 1
🛠️ Fix failing CI checks: 💡
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/spawn/process.rs
Comment thread test/js/bun/spawn/spawn-pipe-start-error.test.ts
@robobun

robobun commented Oct 20, 2026

Copy link
Copy Markdown
Collaborator Author

A second branch reached the same fix on its own: robobun/f7ace02b/spawn-kill-before-watch (one commit, 0cfce6e9bf). No PR is opened for it. This PR covers it.

Diff summary: the same Poller::Unwatched split in src/spawn/process.rs, plus kill_and_reap moving Unwatched to Detached, and forced-order tests that need no C compiler.

Three results from that branch that this PR can use:

  1. A child that keeps writing, with the order forced. The forced cases here use a child that writes once (printf ...; exec sleep 2). With exec yes after the first write, main ends with exit code 1 in 10 of 10 forced calls, which is the "yes is killed" failure. With the Unwatched arm the child ends by SIGHUP in 40 of 40 calls on a release build and 40 of 40 on debug + ASAN, half of them with the waiter thread. A syscall log shows kill(child, SIGHUP) before the close of the read end in all 79 of those calls where the close was logged.
  2. A forced order without cc. test/js/bun/spawn/spawn-maxbuf.test.ts on that branch makes the child's stdin and its other output FIFOs, so the child blocks in open() before exec. The caller is parked in vfork() until that exec, and a SIGSTOP sent meanwhile stops it the moment vfork() returns. It has 6 cases: stdout and stderr, each with a child that keeps writing (pidfd and waiter thread) and a quiet child. They fail on main and pass with the fix. Linux only.
  3. The call-site alternative, measured, for the Process::kill question in the description. A debug build that leaves Process::kill alone and sends the kill again after watch_or_reap() (js_bun_spawn_bindings.rs:1760, when exited_due_to_maxbuf is set) ends 80 of 80 forced calls with SIGHUP (40 on a calm box, 40 under 36 busy loops). It is keyed on maxBuffer, so it does not reach the abort case.

Release builds of main ea6a2c7b7c with and without the process.rs change, same box, Linux x64:

main with the change
sh -c "printf '%01000d' 0; exec sleep 30", maxBuffer: 256: not killed by maxBuffer 39 of 50 0 of 50
the same child through child_process.spawnSync 38 of 50 0 of 50
[bun, "exec", "yes"], maxBuffer: 256: exit code 1 16 of 100 0 of 100
abort from an option getter, child sleep 2 exitCode 0 after 2003 ms SIGHUP after 53 ms
size of bun: text 107,095,284 107,095,284

On kill_and_reap: the extra statement on that branch runs only on macOS, on the Writable::init error path (js_bun_spawn_bindings.rs:1396), and detach() follows it at :1401. So it changes nothing that a caller can see, as the notes here say.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant