fix: terminate shell process trees on timeout#2327
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if pid > 0 and self._on_windows: | ||
| await asyncio.to_thread( | ||
| subprocess.run, | ||
| ["taskkill", "/PID", str(pid), "/T", "/F"], | ||
| stdout=subprocess.DEVNULL, | ||
| stderr=subprocess.DEVNULL, | ||
| check=False, | ||
| ) | ||
| elif pid > 0 and os.name != "nt": |
There was a problem hiding this comment.
🟡 _terminate_shell_process uses self._on_windows instead of os.name for platform detection, mismatching process creation
_terminate_shell_process checks self._on_windows (derived from environment.os_kind) to decide the termination strategy, but the process was created in packages/kaos/src/kaos/local.py:182 using os.name == "nt" to decide whether to set CREATE_NEW_PROCESS_GROUP (Windows) or start_new_session (Unix). The existing background worker at src/kimi_cli/background/worker.py:94 correctly uses os.name == "nt" consistently for both creation and termination.
If self._on_windows ever disagrees with os.name (e.g., a test constructs a Shell with os_kind="Windows" on Linux, or an SSH-backed scenario), _terminate_shell_process would try to run taskkill on a Unix machine. Since the taskkill path has no exception handling (unlike the Unix os.killpg path which catches OSError), subprocess.run(["taskkill", ...]) would raise FileNotFoundError, which propagates uncaught from _terminate_shell_process and replaces the original CancelledError/TimeoutError — leaving the process group alive and producing an unexpected exception type.
| if pid > 0 and self._on_windows: | |
| await asyncio.to_thread( | |
| subprocess.run, | |
| ["taskkill", "/PID", str(pid), "/T", "/F"], | |
| stdout=subprocess.DEVNULL, | |
| stderr=subprocess.DEVNULL, | |
| check=False, | |
| ) | |
| elif pid > 0 and os.name != "nt": | |
| if pid > 0 and os.name == "nt": | |
| await asyncio.to_thread( | |
| subprocess.run, | |
| ["taskkill", "/PID", str(pid), "/T", "/F"], | |
| stdout=subprocess.DEVNULL, | |
| stderr=subprocess.DEVNULL, | |
| check=False, | |
| ) | |
| elif pid > 0 and os.name != "nt": |
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary
Fixes #2310
To verify
uv run ruff check packages\kaos\src\kaos\__init__.py packages\kaos\src\kaos\local.py packages\kaos\src\kaos\ssh.py src\kimi_cli\acp\kaos.py src\kimi_cli\tools\shell\__init__.py tests\tools\test_shell_bash.py tests\tools\test_shell_process_lifecycle.pyuv run ruff format --check packages\kaos\src\kaos\__init__.py packages\kaos\src\kaos\local.py packages\kaos\src\kaos\ssh.py src\kimi_cli\acp\kaos.py src\kimi_cli\tools\shell\__init__.py tests\tools\test_shell_bash.py tests\tools\test_shell_process_lifecycle.pyNew-Item -ItemType Directory -Force .tmp\pytest-temp | Out-Null; $env:TMP=(Resolve-Path .tmp\pytest-temp).Path; $env:TEMP=$env:TMP; uv run pytest tests\tools\test_shell_process_lifecycle.py packages\kaos\tests\test_local_kaos.py -q -k "foreground_shell_uses_new_session or exec" -o cache_dir=.tmp\pytest-cachepython -m py_compile packages\kaos\src\kaos\__init__.py packages\kaos\src\kaos\local.py packages\kaos\src\kaos\ssh.py src\kimi_cli\acp\kaos.py src\kimi_cli\tools\shell\__init__.py tests\tools\test_shell_bash.py tests\tools\test_shell_process_lifecycle.py