fix: kill entire CLI process tree on stop/forceStop (Windows) - #2073
Open
rinceyuan wants to merge 1 commit into
Open
fix: kill entire CLI process tree on stop/forceStop (Windows)#2073rinceyuan wants to merge 1 commit into
rinceyuan wants to merge 1 commit into
Conversation
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Rust is not included - #2292 landed that fix, and this branch takes it as-is.
Fix the process-tree leak when
stop()/forceStop()terminates the CLI. On WindowsChildProcess.kill()/Popen.terminate()only ends the root, leaving grandchildren orphaned. On POSIX a SIGTERM-resistant descendant survives after the root exits.Closes #1804.
Spawn-time isolation
Put the CLI in its own process group so the whole tree can be signalled:
detached: truestart_new_session=TrueSysProcAttr{Setpgid: true}Teardown
Private helpers, no public API change, called from
stop()andforceStop():taskkill /T /F /PIDkill(-pid, signal)on the process groupProcessHandle.descendants()collected before the root is signalledKill(entireProcessTree: true)- unchangedstop()is graceful first and escalates: POSIX sends SIGTERM to the group, waits, then SIGKILLs the group. The escalation is unconditional because the root exiting says nothing about a descendant that ignored SIGTERM.Windows always uses
taskkill /T /F. It has no graceful signal (kill()isTerminateProcessregardless), and/Tcan only enumerate the tree while the root is alive, so a graceful root close would strand the descendants.Failure handling
Every helper falls back to the single-process termination it replaced when the tree-wide path is unavailable or fails - missing pid, non-zero
taskkill,ESRCHfromkillpg. Go propagates the error after the fallback also fails; Rust checks the exit status and reaps the root.Go's
killProcessTreeByPiduses the PID from the atomically swappedosProcess, not the mutex-guardedc.process.Tests
nodejs/test/process_tree_kill.test.tsdrivesCopilotClient.stop()/forceStop()over a real spawned tree, so removing the tree termination fails them:stop()terminates descendants of the owned runtimeforceStop()terminates descendants of the owned runtimestop()reaps a descendant that ignores SIGTERM (POSIX only)python/test_client.pygainsTestKillProcessTreecovering the group signal, thetaskkillinvocation and both fallbacks.Validation
Not run locally: the
internal/e2eGo package and the Ruste2etarget, which need the replay harness and fail identically on a clean checkout.