Skip to content

fix: reliably kill shell process tree on Windows timeout #8822

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
muhamedfazalps wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: reliably kill shell process tree on Windows timeout #8822

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 17 additions & 6 deletions astrbot/core/computer/booters/local.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,18 +118,29 @@ def _run() -> dict[str, Any]:
# `command` is intentionally executed through the current shell so
# local computer-use behavior matches existing tool semantics.
# Safety relies on `_is_safe_command()` and the allowed-root checks.
result = subprocess.run( # noqa: S602 # nosemgrep: python.lang.security.audit.dangerous-subprocess-use-audit
proc = subprocess.Popen( # noqa: S602 # nosemgrep: python.lang.security.audit.dangerous-subprocess-use-audit
command,
shell=shell,
cwd=working_dir,
env=run_env,
timeout=timeout or 300,
capture_output=True,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
)
try:
stdout, stderr = proc.communicate(timeout=timeout or 300)
except subprocess.TimeoutExpired:
if sys.platform == "win32":
import subprocess as _sp
_sp.run(["taskkill", "/F", "/T", "/PID", str(proc.pid)],
capture_output=True, timeout=5)
else:
proc.kill()
proc.wait()
raise
Comment on lines +131 to +139

@gemini-code-assist gemini-code-assist Bot Jun 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

There are two issues with the current implementation of the timeout handling on Windows:

  1. Redundant Import: subprocess is already imported at the top of the file (line 7), so import subprocess as _sp is unnecessary.
  2. Exception Masking: If taskkill fails (e.g., if the executable is not found or if the taskkill command itself times out), it will raise an exception. This will mask the original TimeoutExpired exception and prevent proc.wait() from being called, potentially leaking resources. Wrapping the taskkill call in a try...except block ensures that cleanup failures do not interfere with the propagation of the original timeout exception.
            except subprocess.TimeoutExpired:
                if sys.platform == "win32":
                    try:
                        subprocess.run(["taskkill", "/F", "/T", "/PID", str(proc.pid)],
                                       stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, timeout=5)
                    except Exception:
                        pass
                else:
                    proc.kill()
                proc.wait()
                raise

return {
"stdout": _decode_shell_output(result.stdout),
"stderr": _decode_shell_output(result.stderr),
"exit_code": result.returncode,
"stdout": _decode_shell_output(stdout),
"stderr": _decode_shell_output(stderr),
"exit_code": proc.returncode,
}

return await asyncio.to_thread(_run)
Expand Down
Loading