feat: implement kill_process_tree to recursively terminate processes and their descendants#35163
feat: implement kill_process_tree to recursively terminate processes and their descendants#35163
Conversation
…and their descendants
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a kill_process_tree function to recursively terminate a process and its descendants, specifically addressing issues on Windows where child processes of a shell remain active after the shell is killed. The reviewer suggested several improvements to make the function more robust, including adding a None check for the process argument, handling psutil.AccessDenied exceptions during iteration, and calling proc.wait() to ensure the process is properly reaped. Additionally, a potential race condition involving the signal handler was identified.
There was a problem hiding this comment.
Pull request overview
This PR improves Windows CI test-runner cleanup by introducing a kill_process_tree helper (psutil-based) and using it instead of killing only the immediate subprocess, reducing the chance of orphaned child processes when tests timeout or are interrupted.
Changes:
- Added
kill_process_tree(proc)to recursively kill a subprocess and its descendants. - Replaced direct
current_process.kill()calls withkill_process_tree(current_process)in interruption/timeout paths.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…rocess_tree robustness
Signed-off-by: WANG Xu <xwang@taosdata.com>
feici02
left a comment
feici02
left a comment
There was a problem hiding this comment.
I made following changes for this PR:
- run
ruff format - run
ruff check --fix
There was a problem hiding this comment.
Pull request overview
This PR improves the Windows CI test runner’s cleanup behavior by introducing a process-tree termination helper so that interrupt/timeout handling kills the full subprocess tree (not just the top-level cmd.exe created by shell=True).
Changes:
- Added
kill_process_tree(proc)usingpsutilto kill a process and its descendants. - Replaced direct
current_process.kill()calls withkill_process_tree(current_process)in interrupt/timeout paths. - Included various small formatting/consistency updates (logging config, string quoting, wrapping).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # 终止当前子进程（使用 kill_process_tree 确保子进程也被清理） | ||
| if current_process is not None: | ||
| try: | ||
| current_process.terminate() | ||
| kill_process_tree(current_process) | ||
| except: | ||
| pass |
| for child in children: | ||
| try: | ||
| child.kill() | ||
| except (psutil.NoSuchProcess, psutil.AccessDenied): | ||
| pass |
| proc.kill() | ||
| gone, still_alive = psutil.wait_procs([parent] + children, timeout=5) | ||
| for p in still_alive: | ||
| try: | ||
| p.kill() | ||
| except (psutil.NoSuchProcess, psutil.AccessDenied): | ||
| pass | ||
| try: | ||
| proc.wait(timeout=3) |
This pull request improves the reliability of process termination in the Windows test runner by ensuring that all child processes are properly killed when a test is interrupted or times out. The main enhancement is the addition of a
kill_process_treefunction, which recursively kills a process and all its descendants, addressing issues where child processes could be left running.Process management improvements:
kill_process_treefunction to recursively terminate a process and all its child processes usingpsutil, ensuring no orphaned subprocesses remain after test interruption or timeout.current_process.kill()withkill_process_tree(current_process)in the test execution and cleanup logic, so that process trees are reliably cleaned up on timeout, interruption, or manual stop. [1] [2]