Fix browser cleanup consistency and screenshot recovery
Unify browser session teardown so manual close, inactivity cleanup, and emergency shutdown all follow the same cleanup path instead of partially duplicating logic. This changes browser_close() to delegate to cleanup_browser(), which means recording shutdown, Browserbase release, activity bookkeeping cleanup, and local socket-directory removal now happen consistently. It also updates emergency cleanup to route through cleanup_all_browsers() and explicitly clear in-memory tracking state after teardown so stale active-session, last-activity, and recording entries are not left behind on exit. The screenshot fallback path has also been fixed. _extract_screenshot_path_from_text() now matches real absolute PNG paths, including quoted output, so browser_vision() can recover screenshots when agent-browser emits human-readable text instead of JSON. Regression coverage was added in tests/tools/test_browser_cleanup.py for screenshot path extraction, cleanup_browser() state removal, browser_close() delegation, and emergency cleanup state clearing. Verified with: - python -m pytest tests/tools/test_browser_cleanup.py -q - python -m pytest tests/tools/test_browser_console.py tests/gateway/test_send_image_file.py -q
This commit is contained in:
96
tests/tools/test_browser_cleanup.py
Normal file
96
tests/tools/test_browser_cleanup.py
Normal file
@@ -0,0 +1,96 @@
|
||||
"""Regression tests for browser session cleanup and screenshot recovery."""
|
||||
|
||||
from unittest.mock import patch
|
||||
|
||||
|
||||
class TestScreenshotPathRecovery:
|
||||
def test_extracts_standard_absolute_path(self):
|
||||
from tools.browser_tool import _extract_screenshot_path_from_text
|
||||
|
||||
assert (
|
||||
_extract_screenshot_path_from_text("Screenshot saved to /tmp/foo.png")
|
||||
== "/tmp/foo.png"
|
||||
)
|
||||
|
||||
def test_extracts_quoted_absolute_path(self):
|
||||
from tools.browser_tool import _extract_screenshot_path_from_text
|
||||
|
||||
assert (
|
||||
_extract_screenshot_path_from_text(
|
||||
"Screenshot saved to '/Users/david/.hermes/browser_screenshots/shot.png'"
|
||||
)
|
||||
== "/Users/david/.hermes/browser_screenshots/shot.png"
|
||||
)
|
||||
|
||||
|
||||
class TestBrowserCleanup:
|
||||
def setup_method(self):
|
||||
from tools import browser_tool
|
||||
|
||||
self.browser_tool = browser_tool
|
||||
self.orig_active_sessions = browser_tool._active_sessions.copy()
|
||||
self.orig_session_last_activity = browser_tool._session_last_activity.copy()
|
||||
self.orig_recording_sessions = browser_tool._recording_sessions.copy()
|
||||
self.orig_cleanup_done = browser_tool._cleanup_done
|
||||
|
||||
def teardown_method(self):
|
||||
self.browser_tool._active_sessions.clear()
|
||||
self.browser_tool._active_sessions.update(self.orig_active_sessions)
|
||||
self.browser_tool._session_last_activity.clear()
|
||||
self.browser_tool._session_last_activity.update(self.orig_session_last_activity)
|
||||
self.browser_tool._recording_sessions.clear()
|
||||
self.browser_tool._recording_sessions.update(self.orig_recording_sessions)
|
||||
self.browser_tool._cleanup_done = self.orig_cleanup_done
|
||||
|
||||
def test_cleanup_browser_clears_tracking_state(self):
|
||||
browser_tool = self.browser_tool
|
||||
browser_tool._active_sessions["task-1"] = {
|
||||
"session_name": "sess-1",
|
||||
"bb_session_id": None,
|
||||
}
|
||||
browser_tool._session_last_activity["task-1"] = 123.0
|
||||
|
||||
with (
|
||||
patch("tools.browser_tool._maybe_stop_recording") as mock_stop,
|
||||
patch(
|
||||
"tools.browser_tool._run_browser_command",
|
||||
return_value={"success": True},
|
||||
) as mock_run,
|
||||
patch("tools.browser_tool.os.path.exists", return_value=False),
|
||||
):
|
||||
browser_tool.cleanup_browser("task-1")
|
||||
|
||||
assert "task-1" not in browser_tool._active_sessions
|
||||
assert "task-1" not in browser_tool._session_last_activity
|
||||
mock_stop.assert_called_once_with("task-1")
|
||||
mock_run.assert_called_once_with("task-1", "close", [], timeout=10)
|
||||
|
||||
def test_browser_close_delegates_to_cleanup_browser(self):
|
||||
import json
|
||||
|
||||
browser_tool = self.browser_tool
|
||||
browser_tool._active_sessions["task-2"] = {"session_name": "sess-2"}
|
||||
|
||||
with patch("tools.browser_tool.cleanup_browser") as mock_cleanup:
|
||||
result = json.loads(browser_tool.browser_close("task-2"))
|
||||
|
||||
assert result == {"success": True, "closed": True}
|
||||
mock_cleanup.assert_called_once_with("task-2")
|
||||
|
||||
def test_emergency_cleanup_clears_all_tracking_state(self):
|
||||
browser_tool = self.browser_tool
|
||||
browser_tool._cleanup_done = False
|
||||
browser_tool._active_sessions["task-1"] = {"session_name": "sess-1"}
|
||||
browser_tool._active_sessions["task-2"] = {"session_name": "sess-2"}
|
||||
browser_tool._session_last_activity["task-1"] = 1.0
|
||||
browser_tool._session_last_activity["task-2"] = 2.0
|
||||
browser_tool._recording_sessions.update({"task-1", "task-2"})
|
||||
|
||||
with patch("tools.browser_tool.cleanup_all_browsers") as mock_cleanup_all:
|
||||
browser_tool._emergency_cleanup_all_sessions()
|
||||
|
||||
mock_cleanup_all.assert_called_once_with()
|
||||
assert browser_tool._active_sessions == {}
|
||||
assert browser_tool._session_last_activity == {}
|
||||
assert browser_tool._recording_sessions == set()
|
||||
assert browser_tool._cleanup_done is True
|
||||
File diff suppressed because it is too large
Load Diff
Reference in New Issue
Block a user