fix(tools): skip camofox auto-cleanup when managed persistence is enabled
When managed_persistence is enabled, cleanup_browser() was calling
camofox_close() which destroys the server-side browser context via
DELETE /sessions/{userId}, killing login sessions across cron runs.
Add camofox_soft_cleanup() — a public wrapper that drops only the
in-memory session entry when managed persistence is on, returning True.
When persistence is off it returns False so the caller falls back to
the full camofox_close(). The inactivity reaper still handles idle
resource cleanup.
Also surface a logger.warning() when _managed_persistence_enabled()
fails to load config, replacing a silent except-and-return-False.
Salvaged from #6182 by el-analista (Eduardo Perea Fernandez).
Added public API wrapper to avoid cross-module private imports,
and test coverage for both persistence paths.
Co-authored-by: Eduardo Perea Fernandez <el-analista@users.noreply.github.com>
This commit is contained in:
@@ -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()
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
@@ -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)
|
||||
|
||||
|
||||
Reference in New Issue
Block a user