fix: kill entire process tree on Windows to prevent GUI program hang (#54201) - #54235
fix: kill entire process tree on Windows to prevent GUI program hang (#54201)#54235Sahil-SS9 wants to merge 1 commit into
Conversation
Duplicate of #49460 — both replace the Windows branch of |
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Fix for Windows process tree kill (2 files, +53/-1). Uses taskkill /F /T /PID instead of proc.terminate() to kill the entire process tree on Windows, preventing GUI programs from hanging the drain thread.
Strengths:
- Falls back to
proc.terminate()if taskkill fails (3s timeout) - Good documentation in the tool description about Windows GUI program behavior
- Addresses a real issue (#54201) where inherited stdout pipes cause indefinite blocking
Notes:
- The
subprocess.runwithcapture_output=Trueand 3s timeout is appropriate - The fallback pattern is correct -- taskkill failure should not crash the session
No security concerns. The fix is scoped to Windows-only code paths.
Reviewed by Hermes Agent
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: LGTM
Kills the entire process tree on Windows using taskkill /F /T instead of proc.terminate() which only kills the bash wrapper. Falls back to proc.terminate() if taskkill fails. Also adds a helpful docstring note about GUI program hangs on Windows.
Reviewed by Hermes Agent
f11686b to
e55a6e7
Compare
|
Thanks for documenting the Windows GUI-process failure mode. This automated hermes-sweeper review verified that the requested process-tree cleanup is already implemented on current
Closing as implemented on main. |
Fixes #54201
Description
On Windows, running GUI .exe programs through the
terminaltool causes the session to hang indefinitely. The root cause is twofold:_kill_processonly callsproc.terminate()on Windows, which kills the bash wrapper but not its children. GUI programs launched from bash inherit the stdout pipe and keep it open even after bash exits, causing the drain thread in_wait_for_processto block forever.No documentation warns users about this behavior or provides the workaround.
Changes
tools/environments/local.py: Replaceproc.terminate()on Windows withtaskkill /F /T /PIDto kill the entire process tree rooted at bash. Falls back toproc.terminate()iftaskkillfails.tools/terminal_tool.py: Add documentation note about Windows GUI programs and the PowerShellStart-Processworkaround.Verification