Run taskkill without a shell so a Windows kill reaches the tree - #771
Closed
vibecodedapps-dev wants to merge 1 commit into
Closed
vibecodedapps-dev wants to merge 1 commit into
vibecodedapps-dev wants to merge 1 commit into
Conversation
runCommand passes $SHELL to spawnSync as the shell on Windows. Under Git Bash, which Claude Code uses there, MSYS rewrites taskkill's /PID into C:/Program Files/Git/PID and taskkill exits 1 without killing anything. terminateProcessTree then throws, so a cancel leaves the job running and the teardown paths that swallow the error leak their process trees. taskkill is directly executable and needs no shim resolution, so it takes the same shell: false opt-out git already uses.
Author
|
Closing this; not pursuing an upstream contribution. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
On Windows, cancelling a run does not stop it, and the paths that clean up at the end of a session leave their processes running.
The plugin stops a run by asking Windows to kill the process and everything it started. That command is a Windows built-in whose options begin with a slash. The helper that runs it passes the value of
SHELLtospawnSyncas the shell to run commands through, and Claude Code drives the plugin through Git Bash on Windows, which setsSHELL. Git Bash rewrites the first slash option into a file path on the way through, so the command refuses to run:Nothing is stopped. The helper then throws, so cancelling a job reports that text as an error and never marks the job cancelled, while the run carries on. The session-end cleanup and the app-server teardown both discard the error, so they leave their processes running with no sign of a problem.
The kill command is directly executable and needs no shell to resolve it, so it now takes the same
shell: falseopt-out that Git calls already use. Nothing else changes: no other command in the plugin passes slash-style options through the shell, and theSHELLdefault stays as it is for the commands that need it to resolve.cmdshims.How this was checked
On Windows 11 with Node 26, against a two-deep chain of processes driven through the real helper with
SHELLset the way Git Bash sets it. Before the change the kill reported the error above and both processes were still running a second later. After it, both were gone.The suite on this branch adds two assertions. One holds the kill command to running without a shell and fails without the fix on every operating system. The other builds that real chain on Windows and requires both processes to be gone; it is skipped elsewhere, because only a real Windows kill shows whether the chain died.
The change also repairs a test that already fails on
mainon Windows. A full run ofmainon this machine is 91 tests with 12 failures; the same run on this branch is 92 with 11, and the one that flips iscancel sends turn interrupt to the shared app-server before killing a brokered task. The other 11 failures are identical before and after and are not related to this change: they are a Unix socket assertion, a symlink test that needs Developer Mode, six setup tests, and three transfer tests.What to watch after merge
A cancel on Windows should now stop the run and mark the job cancelled. Anyone seeing the error text above after this lands has a second path passing slash options through a shell, which would be worth reporting.