diff --git a/tests/tools/test_browser_use_cli.py b/tests/tools/test_browser_use_cli.py index 2ea54e909e..8109b5d58d 100644 --- a/tests/tools/test_browser_use_cli.py +++ b/tests/tools/test_browser_use_cli.py @@ -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) diff --git a/tools/browser_use_cli.py b/tools/browser_use_cli.py index 44410463c8..42220544a9 100644 --- a/tools/browser_use_cli.py +++ b/tools/browser_use_cli.py @@ -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 "