ci: kill in-flight test subprocesses on runner SIGINT/SIGTERM - #29317
ci: kill in-flight test subprocesses on runner SIGINT/SIGTERM#29317alii wants to merge 4 commits into
Conversation
|
Updated 7:23 PM PT - Apr 16th, 2026
❌ @robobun, your commit 537efef has 3 failures in
🧪 To try this PR locally: bunx bun-pr 29317That installs a local version of the PR into your bun-29317 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
- The fallback will always fail or (worse) kill an unrelated process
- On Windows, this isn't necessary because processes by default killall their children on exit
ab88002 to
9d417a6
Compare
483a2be to
af75f88
Compare
9d417a6 to
9340cb3
Compare
86708d1 to
cd80459
Compare
alii
changed the title
abbeb33 to
cde4098
Compare
cde4098 to
597882c
Compare
3e59d40 to
d29f055
Compare
3bd443e to
6fe0ef8
Compare
There was a problem hiding this comment.
LGTM — all critical issues from prior rounds are addressed. The remaining sequential-taskkill nit in parallel mode is bounded and only pathological.
Extended reasoning...
Overview
The PR adds a module-level activeSubprocesses Set, populated on the async 'spawn' event and cleared in done(), and iterates it in onExit() to kill in-flight test subprocesses when the runner receives SIGINT/SIGTERM/SIGHUP. Single file changed: scripts/runner.node.mjs (CI tooling only, not production Bun code).
Security risks
None. This is CI runner cleanup logic with no auth, crypto, or data-exposure surface.
Level of scrutiny
Low-to-medium. The change is CI infrastructure (runner.node.mjs), not the Bun runtime itself. The logic is straightforward signal-handler cleanup. The PR description is transparent about the POSIX grandchild limitation being deliberate.
Other factors
All 🔴 issues from prior review rounds are resolved in the committed code: the TDZ placement is correct (line 225, before the first top-level await spawnSafe), a Set is used instead of a single variable, and the taskkill call has timeout: 5000. The one open 🟡 nit (sequential spawnSync calls in --parallel mode) is minor: worst-case exit delay is bounded at N×5 s, only possible under pathological AV-hook conditions, and --parallel defaults to false. The conservative design (no detached, no process.kill(-pid)) is intentional and documented.
Track spawnSafe's in-flight subprocesses in a module-level Set, and on SIGINT/SIGTERM/SIGHUP kill them (taskkill /T on Windows, kill(9) on POSIX) before exiting. Currently onExit() just calls process.exit() without killing anything, so a Buildkite job-cancel orphans the running test process. This is the conservative subset of the original process-group-kill attempt — no spawn-option changes (no `detached`, no `kill(-pid)`), so it won't reap POSIX grandchildren on the timeout path; it only handles the cancel path. The full process-group story can follow in a separate PR. The activeSubprocesses Set is declared above the first top-level `await spawnSafe(...)` (the Linux-only coredump sysctl probe at ~line 223). spawnSafe is a hoisted function declaration; with the const placed next to spawnSafe lower in the file, Linux CI hit a TDZ ReferenceError before any tests ran.
spawnSync serialized N×5s worst-case under --parallel (8 on Windows). spawn() with detached+unref launches each taskkill as an independent process that outlives our process.exit() — CreateProcess is synchronous inside spawn() so the child exists before it returns.
6fe0ef8 to
8c0034a
Compare
|
@robobun adopt |
|
Adopted. Pushed 537efef addressing the review:
Verified locally: with the base-branch runner, SIGTERM to the runner leaves the in-flight CI on 537efef (#45950) — all three hard failures are unrelated to this runner signal-handler change:
This PR only touches |
On Windows, children spawned without `detached` are placed in the runner's Job Object with JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE, so process.exit() already terminates the whole tree. The taskkill by PID was redundant and, in the race where the child exits first, could hit an unrelated reused PID. POSIX keeps the explicit proc.kill(9) via the ChildProcess handle (safe against PID reuse) since Buildkite cancel signals only the runner PID, not the process group.
3 participants
Summary
Track
spawnSafe's in-flight subprocesses in a module-levelSet, and on SIGINT/SIGTERM/SIGHUP kill them (taskkill /Ton Windows,proc.kill(9)on POSIX) before exiting. CurrentlyonExit()just callsprocess.exit()without killing anything, so a Buildkite job-cancel (or local Ctrl+C) orphans the running test process.This is the conservative subset of the original process-group-kill attempt — no spawn-option changes (no
detached, nokill(-pid)), so it won't reap POSIX grandchildren on the timeout path. The full process-group story can follow separately once verified on Linux CI.The
activeSubprocessesSet is declared above the first top-levelawait spawnSafe(...)(the Linux-only--coredump-uploadsysctl probe at ~line 223).spawnSafeis a hoisted function declaration; with the const next tospawnSafelower in the file, Linux CI hitReferenceError: Cannot access 'activeSubprocesses' before initializationbefore any tests ran.Part 3 of the macOS-CI Phase 0 stack (depends on #29315 → #29314), though this change is platform-agnostic.
Test plan
node --check scripts/runner.node.mjsnode scripts/runner.node.mjs --exec-path=$(which bun) --include="which.test"runs and passes--coredump-uploadpath reaches the sysctl call (no TDZ)