Repository navigation
Conversation
A stop with forceKillAfter sent SIGTERM to the process group and escalated to SIGKILL only while the leader was still running. When the leader exited on SIGTERM, any child that outlived the signal kept running, and could keep starting more processes, with nothing tracking the group. After the first signal, wait for the whole group to empty. When the grace period ends, send SIGKILL to the group until it has no members, for at most one second.
There was a problem hiding this comment.
Requesting changes: the fix works on a normal Linux host, but it adds a delay of about 4 s to every shell timeout or abort when opencode runs as PID 1, which is what the official Docker image does.
How I checked
- In a PID namespace whose init reaps processes, the new test fails on the base and passes with this change. The whole spawner test file also passes there.
- In this sandbox, PID 1 doesn't reap orphaned processes, so dead group members stay as zombies.
kill(-pgid, 0)keeps succeeding on zombies, so the group never looks empty. - In that setup, I timed
handle.kill({ forceKillAfter: "3 seconds" })onsh -c 'sleep 30 & echo $!; wait', wheresleepexits on SIGTERM. It takes 2 ms ondevand 4002 ms with this change: the full grace period, then the full 1 s SIGKILL loop. Under a reaping init it takes 2 ms. - The new test also fails in that setup.
Who is affected: packages/cli/Dockerfile uses ENTRYPOINT ["opencode"] with no init, so opencode is PID 1 and never reaps orphans (#53564). In that image, any timed-out or aborted command with a background child now costs forceKillAfter + 1 s, as does any scope release that goes through stop. Details and options are in the inline comment.
Public API: unchanged.
Waiting for kill(-pgid, 0) to fail never ends where nothing reaps orphans, such as opencode as PID 1 in its Docker image: zombies keep the group alive, so every stop with forceKillAfter took the grace period plus the 1 s SIGKILL loop. Once the leader has closed, send one SIGKILL to its group instead and return. Members that outlived SIGTERM still die, and a stop where every member exited on SIGTERM returns in milliseconds again. After the grace period, the SIGKILL path is the same as before this PR. The test now waits until the child is gone or a zombie (state Z in /proc/<pid>/stat), so it also passes under a PID 1 that never reaps. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Note 🤖 Opus 5.5 replying on behalf of Victor. Both review threads are answered in ad0abe3. The |
There was a problem hiding this comment.
The zombie concern from my last review is resolved, and I found nothing else that should block this.
How I checked
- With PID 1 never reaping orphans, stopping
sh -c 'sleep 30 & wait'takes 2 ms, the same asdev. - A SIGTERM-ignoring child that keeps starting
sleepis still running afterkillondev. With this change, only zombies are left. - The new test fails on
dev, with and without a reaping init, and passes with this change in both. - The spawner test file and
tscinpackages/utilpass.
Trade-off: a child that redirected its output now gets SIGKILL with no grace period once the leader exits. It was already sent SIGTERM, and this only happens when forceKillAfter is set, so that seems right for a timeout or abort.
Public API: unchanged.
Issue for this PR
Related to #59006, which asks for a way to stop an agent's whole process tree. No issue covers this exact case, so this PR fixes one focused part of it: what a shell timeout or abort leaves behind.
Type of change
What does this PR do?
On a shell timeout or abort,
stopsends SIGTERM to the process group and escalates to SIGKILL only if the leader (sh) or its output is still open whenforceKillAfterends. The leader usually exits at once, so a child that ignores SIGTERM keeps running with no parent. One that keeps starting children can use up the user's process limit.With
forceKillAfterset, once the leader has exited and its output has closed,stopnow sends one SIGKILL to the group. It doesn't wait for the group to empty, since zombies keep it alive where nothing reaps orphans (opencode as PID 1 in the Docker image, #53564). The grace-period path is unchanged.Trade-off: a child that redirected its output gets SIGKILL as soon as the leader exits, with no grace period. A child holding the pipes gets the same wait as before.
This covers every
stopwithforceKillAfter: shell timeout and abort, MCP stdio servers and scope release. Windows,detached: falseand stops withoutforceKillAfterare unchanged, and a command that exited 0 and closed its output still leaves background children running. Separate from #61876.How did you verify your code works?
cross-spawn-spawner.test.ts: the leader exits on SIGTERM and its child ignores SIGTERM with its output redirected. It fails ondevand passes with this change on Linux x86_64 (host, and PID namespaces whose PID 1 reaps or never reaps) and macOS 27 arm64.packages/core: 33 pass, 1 skip, 0 fail; with the shell, MCP and process suites, 206 pass, 24 skip, 0 fail. Type checks, Prettier and oxlint pass.dev, two group members are still starting processes 4 s afterkill. With this change the group is empty. Under a PID 1 that never reaps,killtakes 2 to 4 ms over six runs, against 3 to 10 ms ondev.Not checked: Windows, whose code path is unchanged.
Screenshots / recordings
No UI change, so the evidence is test and runtime output, unedited. Linux x86_64, bun 1.4.4.
devis 1017760240 with only this PR's test file added, and this PR is ad0abe3.Commands and unedited test and runtime output
Commands, from
packages/coreunless named:bun run script/test.ts test/effect/cross-spawn-spawner.test.ts, then with the shell, MCP and process suites added.bun check --no-scriptandtsc --noEmitinpackages/util,bun check --no-script -b -p tsconfig.tests.jsoninpackages/core, and Prettier and oxlint on the two changed files.The new test waits after
killuntil the child is gone or a zombie.Runtime case: a child that ignores SIGTERM and keeps starting
sleep, stopped withhandle.kill({ forceKillAfter: "3 seconds" })after 1 s. The PID 1 timing case iskillonsh -c 'sleep 30 & echo $!; wait'.dev: the new test times out (child still running)The new test on
dev: the child is still running afterkill, so the wait times out.ad0abe3: the new test passes
The new test at ad0abe3: it passes.
dev: two group members still running 4 s afterkillA timeout through the package's spawner: the leader starts a child that ignores SIGTERM and keeps starting
sleep, andhandle.kill({ forceKillAfter: "3 seconds" })runs at 1 s. Ondev,killreturns at once and two group members are still running 4 s later.ad0abe3: the group is empty 4 s after
killAt ad0abe3:
killreturns at once and the group is empty.Checklist
Generated with Claude Opus 5.5 and GPT-6.1 Sol in T3 Code Nightly
Added by the opencode bot (AI-generated).
Related: #59006, #53564