diff --git a/tests/tools/test_browser_camofox_persistence.py b/tests/tools/test_browser_camofox_persistence.py index 0fa5723c67..0e9c863727 100644 --- a/tests/tools/test_browser_camofox_persistence.py +++ b/tests/tools/test_browser_camofox_persistence.py @@ -16,6 +16,7 @@ from tools.browser_camofox import ( _managed_persistence_enabled, camofox_close, camofox_navigate, + camofox_soft_cleanup, check_camofox_available, cleanup_all_camofox_sessions, get_vnc_url, @@ -240,3 +241,50 @@ class TestVncUrlDiscovery: assert result["vnc_url"] == "http://localhost:6080" assert "vnc_hint" in result + + +class TestCamofoxSoftCleanup: + """camofox_soft_cleanup drops local state only when managed persistence is on.""" + + def test_returns_true_and_drops_session_when_enabled(self, tmp_path, monkeypatch): + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + monkeypatch.setenv("CAMOFOX_URL", "http://localhost:9377") + + with _enable_persistence(): + _get_session("task-1") + result = camofox_soft_cleanup("task-1") + + assert result is True + # Session should have been dropped from in-memory store + import tools.browser_camofox as mod + with mod._sessions_lock: + assert "task-1" not in mod._sessions + + def test_returns_false_when_disabled(self, tmp_path, monkeypatch): + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + monkeypatch.setenv("CAMOFOX_URL", "http://localhost:9377") + + _get_session("task-1") + config = {"browser": {"camofox": {"managed_persistence": False}}} + with patch("tools.browser_camofox.load_config", return_value=config): + result = camofox_soft_cleanup("task-1") + + assert result is False + # Session should still be present — not dropped + import tools.browser_camofox as mod + with mod._sessions_lock: + assert "task-1" in mod._sessions + + def test_does_not_call_server_delete(self, tmp_path, monkeypatch): + """Soft cleanup must never hit the Camofox /sessions DELETE endpoint.""" + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + monkeypatch.setenv("CAMOFOX_URL", "http://localhost:9377") + + with ( + _enable_persistence(), + patch("tools.browser_camofox.requests.delete") as mock_delete, + ): + _get_session("task-1") + camofox_soft_cleanup("task-1") + + mock_delete.assert_not_called() diff --git a/tests/tools/test_browser_cleanup.py b/tests/tools/test_browser_cleanup.py index df21f3a0ea..817927903e 100644 --- a/tests/tools/test_browser_cleanup.py +++ b/tests/tools/test_browser_cleanup.py @@ -65,6 +65,62 @@ class TestBrowserCleanup: mock_stop.assert_called_once_with("task-1") mock_run.assert_called_once_with("task-1", "close", [], timeout=10) + def test_cleanup_camofox_managed_persistence_skips_close(self): + """When camofox mode + managed persistence, soft_cleanup fires instead of close.""" + 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._is_camofox_mode", return_value=True), + patch("tools.browser_tool._maybe_stop_recording") as mock_stop, + patch( + "tools.browser_tool._run_browser_command", + return_value={"success": True}, + ), + patch("tools.browser_tool.os.path.exists", return_value=False), + patch( + "tools.browser_camofox.camofox_soft_cleanup", + return_value=True, + ) as mock_soft, + patch("tools.browser_camofox.camofox_close") as mock_close, + ): + browser_tool.cleanup_browser("task-1") + + mock_soft.assert_called_once_with("task-1") + mock_close.assert_not_called() + + def test_cleanup_camofox_no_persistence_calls_close(self): + """When camofox mode but managed persistence is off, camofox_close fires.""" + 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._is_camofox_mode", return_value=True), + patch("tools.browser_tool._maybe_stop_recording") as mock_stop, + patch( + "tools.browser_tool._run_browser_command", + return_value={"success": True}, + ), + patch("tools.browser_tool.os.path.exists", return_value=False), + patch( + "tools.browser_camofox.camofox_soft_cleanup", + return_value=False, + ) as mock_soft, + patch("tools.browser_camofox.camofox_close") as mock_close, + ): + browser_tool.cleanup_browser("task-1") + + mock_soft.assert_called_once_with("task-1") + mock_close.assert_called_once_with("task-1") + def test_emergency_cleanup_clears_all_tracking_state(self): browser_tool = self.browser_tool browser_tool._cleanup_done = False diff --git a/tools/browser_camofox.py b/tools/browser_camofox.py index 226e99b56b..3a305bbcb1 100644 --- a/tools/browser_camofox.py +++ b/tools/browser_camofox.py @@ -101,7 +101,8 @@ def _managed_persistence_enabled() -> bool: """ try: camofox_cfg = load_config().get("browser", {}).get("camofox", {}) - except Exception: + except Exception as exc: + logger.warning("managed_persistence check failed, defaulting to disabled: %s", exc) return False return bool(camofox_cfg.get("managed_persistence")) @@ -172,6 +173,22 @@ def _drop_session(task_id: Optional[str]) -> Optional[Dict[str, Any]]: return _sessions.pop(task_id, None) +def camofox_soft_cleanup(task_id: Optional[str] = None) -> bool: + """Release the in-memory session without destroying the server-side context. + + When managed persistence is enabled the browser profile (and its cookies) + must survive across agent tasks. This helper drops only the local tracking + entry and returns ``True``. When managed persistence is *not* enabled it + does nothing and returns ``False`` so the caller can fall back to + :func:`camofox_close`. + """ + if _managed_persistence_enabled(): + _drop_session(task_id) + logger.debug("Camofox soft cleanup for task %s (managed persistence)", task_id) + return True + return False + + # --------------------------------------------------------------------------- # HTTP helpers # --------------------------------------------------------------------------- diff --git a/tools/browser_tool.py b/tools/browser_tool.py index 012b8eb020..e62a586c11 100644 --- a/tools/browser_tool.py +++ b/tools/browser_tool.py @@ -1935,11 +1935,15 @@ def cleanup_browser(task_id: Optional[str] = None) -> None: if task_id is None: task_id = "default" - # Also clean up Camofox session if running in Camofox mode + # Also clean up Camofox session if running in Camofox mode. + # Skip full close when managed persistence is enabled — the browser + # profile (and its session cookies) must survive across agent tasks. + # The inactivity reaper still frees idle resources. if _is_camofox_mode(): try: - from tools.browser_camofox import camofox_close - camofox_close(task_id) + from tools.browser_camofox import camofox_close, camofox_soft_cleanup + if not camofox_soft_cleanup(task_id): + camofox_close(task_id) except Exception as e: logger.debug("Camofox cleanup for task %s: %s", task_id, e)