From 0be9b7cd5725c8cb773bd9623a7cbbdb2319a394 Mon Sep 17 00:00:00 2001 From: luyifan Date: Mon, 27 Jul 2026 04:30:10 +0800 Subject: [PATCH] fix(browser): reset sessions after command timeout --- tests/tools/test_browser_open_timeout.py | 69 +++++++++++++++++++++++- tools/browser_tool.py | 45 ++++++++++++++++ 2 files changed, 113 insertions(+), 1 deletion(-) diff --git a/tests/tools/test_browser_open_timeout.py b/tests/tools/test_browser_open_timeout.py index 570f8f9b6a..2263383ed7 100644 --- a/tests/tools/test_browser_open_timeout.py +++ b/tests/tools/test_browser_open_timeout.py @@ -1,6 +1,7 @@ """Tests for browser first-open timeout and timeout diagnostics.""" -from unittest.mock import patch +import subprocess +from unittest.mock import Mock, patch import pytest @@ -11,9 +12,15 @@ import tools.browser_tool as bt def _reset_browser_caches(): bt._cached_command_timeout = None bt._command_timeout_resolved = False + bt._active_sessions.clear() + bt._session_last_activity.clear() + bt._last_active_session_key.clear() yield bt._cached_command_timeout = None bt._command_timeout_resolved = False + bt._active_sessions.clear() + bt._session_last_activity.clear() + bt._last_active_session_key.clear() class TestOpenCommandTimeout: @@ -81,6 +88,66 @@ class TestReadCommandOutputFiles: assert stderr == "warn" +class TestCommandTimeoutRecovery: + @pytest.mark.parametrize("cloud", [False, True]) + def test_timeout_replaces_only_stuck_client(self, monkeypatch, tmp_path, cloud): + task_id = "stuck-command" + session_info = { + "session_name": "stuck-session", + "bb_session_id": "cloud-session-1" if cloud else None, + "cdp_url": "ws://cloud.invalid/devtools/browser/1" if cloud else None, + } + bt._active_sessions[task_id] = session_info + bt._session_last_activity[task_id] = 1.0 + bt._last_active_session_key[task_id] = task_id + + process = Mock() + process.returncode = 0 + process.wait.side_effect = [subprocess.TimeoutExpired("agent-browser", 1), -9, 0] + supervisor_events = [] + + monkeypatch.setattr(bt, "_find_agent_browser", lambda: "agent-browser") + monkeypatch.setattr(bt, "_requires_real_termux_browser_install", lambda _cmd: False) + monkeypatch.setattr(bt, "_start_browser_cleanup_thread", lambda: None) + monkeypatch.setattr(bt, "_ensure_cdp_supervisor", lambda _: supervisor_events.append("ensure")) + monkeypatch.setattr(bt, "_stop_cdp_supervisor", lambda _: supervisor_events.append("stop")) + monkeypatch.setattr(bt, "_socket_safe_tmpdir", lambda: str(tmp_path)) + monkeypatch.setattr(bt, "_write_owner_pid", lambda *_args: None) + monkeypatch.setattr(bt, "_build_browser_env", lambda: {}) + monkeypatch.setattr(bt, "_merge_browser_path", lambda value: value) + monkeypatch.setattr(subprocess, "Popen", lambda *_args, **_kwargs: process) + monkeypatch.setattr("tools.interrupt.is_interrupted", lambda: False) + + bt._run_browser_command(task_id, "click", ["@e1"], timeout=1) + + assert task_id not in bt._last_active_session_key + assert not (tmp_path / "agent-browser-stuck-session").exists() + if not cloud: + assert task_id not in bt._active_sessions and task_id not in bt._session_last_activity + return + + replacement = bt._active_sessions[task_id] + assert replacement is not session_info + assert replacement["session_name"] != "stuck-session" + assert replacement["bb_session_id"] == "cloud-session-1" + assert bt._get_session_info(task_id) is replacement + + provider = Mock() + monkeypatch.setattr(bt, "_get_cloud_provider", lambda: provider) + bt.cleanup_browser(task_id) + provider.close_session.assert_called_once_with("cloud-session-1") + assert supervisor_events == ["ensure", "stop", "stop"] + + def test_stale_timeout_cannot_remove_concurrent_replacement(self, tmp_path): + stale, replacement = {"session_name": "stale"}, {"session_name": "replacement"} + bt._active_sessions["race"] = replacement + + bt._discard_timed_out_browser_session("race", stale, str(tmp_path)) + + assert bt._active_sessions["race"] is replacement + assert tmp_path.exists() + + class TestBrowserNavigateOpenTimeout: def test_first_navigation_uses_first_open_timeout(self, monkeypatch): captured: dict = {} diff --git a/tools/browser_tool.py b/tools/browser_tool.py index 4f01801fe6..982e2147cc 100644 --- a/tools/browser_tool.py +++ b/tools/browser_tool.py @@ -3142,6 +3142,47 @@ def _extract_screenshot_path_from_text(text: str) -> Optional[str]: return None +def _discard_timed_out_browser_session( + task_id: str, + session_info: Dict[str, Any], + task_socket_dir: str, +) -> None: + """Drop a stuck client generation without losing cloud cleanup state.""" + with _cleanup_lock: + if _active_sessions.get(task_id) is not session_info: + return + _stop_cdp_supervisor(task_id) + if session_info.get("bb_session_id") or session_info.get("cdp_url"): + import uuid + replacement = dict(session_info) + replacement["session_name"] = f"h_{uuid.uuid4().hex[:10]}" + replacement.pop("_first_nav", None) + _active_sessions[task_id] = replacement + else: + _active_sessions.pop(task_id, None) + _session_last_activity.pop(task_id, None) + + bare_task_id = _bare_task_id_for_session_key(task_id) + if _last_active_session_key.get(bare_task_id) == task_id: + _last_active_session_key.pop(bare_task_id, None) + + session_name = str(session_info.get("session_name") or "") + if session_name: + pid_file = os.path.join(task_socket_dir, f"{session_name}.pid") + if os.path.isfile(pid_file): + try: + from tools.process_registry import ProcessRegistry + + daemon_pid = int(Path(pid_file).read_text(encoding="utf-8").strip()) + if not _verify_reapable_browser_daemon(daemon_pid, task_socket_dir, session_name): + return + ProcessRegistry._terminate_host_pid(daemon_pid) + except (ProcessLookupError, ValueError, PermissionError, OSError): + logger.debug("Could not kill timed-out browser daemon for %s", session_name) + return + shutil.rmtree(task_socket_dir, ignore_errors=True) + + def _run_browser_command( task_id: str, command: str, @@ -3215,6 +3256,9 @@ def _run_browser_command( except Exception as e: logger.warning("Failed to create browser session for task=%s: %s", task_id, e) return {"success": False, "error": f"Failed to create browser session: {str(e)}"} + # Cleanup stops the supervisor before closing the backend; keep it stopped. + if command != "close" and session_info.get("cdp_url"): + _ensure_cdp_supervisor(task_id) # Build the command with the appropriate backend flag. # Cloud mode: --cdp connects to Browserbase. @@ -3353,6 +3397,7 @@ def _run_browser_command( proc.wait() stdout, stderr = _read_command_output_files(stdout_path, stderr_path) _unlink_command_output_files(stdout_path, stderr_path) + _discard_timed_out_browser_session(task_id, session_info, task_socket_dir) if stderr and stderr.strip(): logger.warning( "browser '%s' stderr after timeout: %s",