fix(tools): kill the browser_exec CLI tree on timeout on Windows too
The salvaged fix only ran the CLI in its own session on POSIX and kept plain subprocess.run on Windows — the one platform where the wedge in #106244 is actually reproducible: CPython's run() retries an unbounded communicate() after kill() there, so a grandchild holding the capture pipes blocks the worker forever. On POSIX run() wait()s the PID and returns promptly; the grandchild merely leaks (live-reproduced on Linux). One code path for both platforms: - _group_popen_kwargs: start_new_session=True on POSIX, CREATE_NEW_PROCESS_GROUP + hide flags on Windows (replaces the hide-only _windows_popen_kwargs). - _kill_cli_process_group: os.killpg SIGKILL on POSIX, taskkill /T /F on Windows (same kwarg set as the sibling taskkill sites). - The Popen decodes with encoding="utf-8", errors="replace" like every other subprocess call in this file (windows footgun rule). - Drain test patches the kill helper instead of os.killpg so it runs on every host. Windows behaviour is not live-verifiable on this Linux host.
This commit is contained in:
@@ -1376,7 +1376,6 @@ class TestTimeoutProcessGroupKill:
|
||||
with pytest.raises(OSError):
|
||||
os.kill(pid, 0)
|
||||
|
||||
@pytest.mark.skipif(sys.platform == "win32", reason="POSIX process groups")
|
||||
def test_post_kill_drain_is_bounded(self, monkeypatch):
|
||||
"""If even the post-kill drain misses its deadline, give up instead of wedging."""
|
||||
|
||||
@@ -1388,6 +1387,6 @@ class TestTimeoutProcessGroupKill:
|
||||
raise subprocess.TimeoutExpired("browser-use", timeout)
|
||||
|
||||
monkeypatch.setattr(bu_cli.subprocess, "Popen", lambda *a, **k: _StuckProc())
|
||||
monkeypatch.setattr(bu_cli.os, "killpg", lambda pgid, sig: None)
|
||||
monkeypatch.setattr(bu_cli, "_kill_cli_process_group", lambda proc: None)
|
||||
with pytest.raises(subprocess.TimeoutExpired):
|
||||
bu_cli._run_cli_killing_process_group(["x"], "code", {}, 5)
|
||||
|
||||
@@ -500,14 +500,17 @@ def _route_backend(env: dict, session: str, task_id: Optional[str], local: bool)
|
||||
return _resolve_backend_cdp(env, task_id, session_name=session)
|
||||
|
||||
|
||||
def _windows_popen_kwargs() -> dict:
|
||||
"""Hide the console the .cmd shim would flash on Windows (as browser_tool does)."""
|
||||
def _group_popen_kwargs() -> dict:
|
||||
"""Popen kwargs starting the CLI in its own process group (a new session on POSIX) so a
|
||||
timeout can take down every process that inherited the capture pipes, not just the CLI
|
||||
child. Windows also hides the console the .cmd shim would flash (as browser_tool does)."""
|
||||
def _flags() -> dict:
|
||||
from hermes_cli._subprocess_compat import windows_hide_flags
|
||||
si = subprocess.STARTUPINFO()
|
||||
si.dwFlags |= subprocess.STARTF_USESHOWWINDOW
|
||||
return {"creationflags": windows_hide_flags(), "startupinfo": si}
|
||||
return _quiet(_flags, {}, "Windows hide-flags unavailable") if os.name == "nt" else {}
|
||||
return {"creationflags": windows_hide_flags() | getattr(subprocess, "CREATE_NEW_PROCESS_GROUP", 0),
|
||||
"startupinfo": si}
|
||||
return _quiet(_flags, {}, "Windows hide-flags unavailable") if os.name == "nt" else {"start_new_session": True}
|
||||
|
||||
|
||||
def _clamp_timeout(timeout_s: Any) -> int:
|
||||
@@ -522,28 +525,38 @@ def _clamp_timeout(timeout_s: Any) -> int:
|
||||
_POST_KILL_DRAIN_S = 10.0
|
||||
|
||||
|
||||
def _run_cli_killing_process_group(cmd, code, env, timeout):
|
||||
"""Run the CLI in its own session and SIGKILL the whole process group on timeout.
|
||||
def _kill_cli_process_group(proc) -> None:
|
||||
"""SIGKILL the CLI's whole process group (POSIX; ``start_new_session`` made pgid == pid) or,
|
||||
on Windows, its process tree via ``taskkill /T /F`` — the only group-wide kill it offers."""
|
||||
if os.name == "nt":
|
||||
from hermes_cli._subprocess_compat import windows_hide_flags
|
||||
with contextlib.suppress(OSError, subprocess.SubprocessError):
|
||||
subprocess.run(["taskkill", "/T", "/F", "/PID", str(proc.pid)], stdin=subprocess.DEVNULL,
|
||||
capture_output=True, text=True, encoding="utf-8", errors="replace", timeout=10,
|
||||
check=False, creationflags=windows_hide_flags())
|
||||
return
|
||||
with contextlib.suppress(ProcessLookupError, PermissionError):
|
||||
os.killpg(proc.pid, signal.SIGKILL) # windows-footgun: ok — POSIX only, the nt branch returned above
|
||||
|
||||
``subprocess.run`` only kills the direct child on ``TimeoutExpired``; a browser_harness
|
||||
daemon / Chrome helper grandchild inherits the stdout/stderr pipes and keeps them open,
|
||||
so the internal ``communicate()`` blocks on pipe EOF forever and the tool call — plus its
|
||||
activity heartbeat — wedges permanently (#106244).
|
||||
|
||||
def _run_cli_killing_process_group(cmd, code, env, timeout):
|
||||
"""Run the CLI in its own process group and kill the whole group on timeout.
|
||||
|
||||
``subprocess.run`` only kills the direct child on ``TimeoutExpired``; a grandchild that
|
||||
inherited the stdout/stderr pipes (browser_harness daemon / Chrome helper) is orphaned
|
||||
still holding them, and on Windows ``run()``'s unbounded post-kill ``communicate()`` then
|
||||
blocks on pipe EOF forever — so the tool call, plus its activity heartbeat, wedges (#106244).
|
||||
"""
|
||||
proc = subprocess.Popen(
|
||||
cmd, stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE,
|
||||
text=True, env=env, start_new_session=True,
|
||||
text=True, encoding="utf-8", errors="replace", env=env, **_group_popen_kwargs(),
|
||||
)
|
||||
try:
|
||||
stdout, stderr = proc.communicate(input=code, timeout=timeout)
|
||||
except subprocess.TimeoutExpired:
|
||||
with contextlib.suppress(ProcessLookupError, PermissionError):
|
||||
# start_new_session=True: pgid == pid; POSIX-only, browser_exec gates this helper behind os.name != "nt".
|
||||
os.killpg(proc.pid, signal.SIGKILL) # windows-footgun: ok
|
||||
try:
|
||||
_kill_cli_process_group(proc)
|
||||
with contextlib.suppress(subprocess.TimeoutExpired):
|
||||
proc.communicate(timeout=_POST_KILL_DRAIN_S)
|
||||
except subprocess.TimeoutExpired:
|
||||
pass
|
||||
raise
|
||||
return subprocess.CompletedProcess(cmd, proc.returncode, stdout, stderr)
|
||||
|
||||
@@ -593,14 +606,7 @@ def browser_exec(code: str, session: str = "", timeout_s: int = _DEFAULT_TIMEOUT
|
||||
timeout = _clamp_timeout(timeout_s)
|
||||
started = time.time()
|
||||
try:
|
||||
if os.name == "nt":
|
||||
proc = subprocess.run(
|
||||
cmd, input=code, capture_output=True, text=True, timeout=timeout, env=env,
|
||||
**_windows_popen_kwargs(),
|
||||
)
|
||||
else:
|
||||
# Plain run() strands the pipe-holding grandchildren above (#106244).
|
||||
proc = _run_cli_killing_process_group(cmd, code, env, timeout)
|
||||
proc = _run_cli_killing_process_group(cmd, code, env, timeout)
|
||||
except subprocess.TimeoutExpired:
|
||||
return tool_error(f"browser-use exec timed out after {timeout}s. The daemon may still be working; retry "
|
||||
f"with a larger timeout_s (max {_MAX_TIMEOUT_S}), or split the work into several calls that "
|
||||
|
||||
Reference in New Issue
Block a user