fix(browser): replace expired cloud sessions
This commit is contained in:
parent
927662e48f
commit
58e85f4314
|
|
@ -26,6 +26,7 @@ Session metadata contract (preserved from the legacy ``CloudBrowserProvider``)::
|
|||
"session_name": str, # unique name for agent-browser --session
|
||||
"bb_session_id": str, # provider session ID (for close/cleanup)
|
||||
"cdp_url": str, # CDP websocket URL
|
||||
"expires_at": str, # optional provider-authoritative ISO timestamp
|
||||
"features": dict, # feature flags that were enabled
|
||||
"external_call_id": str, # optional, managed-gateway billing key
|
||||
}
|
||||
|
|
@ -96,6 +97,7 @@ class BrowserProvider(abc.ABC):
|
|||
"session_name": str, # unique name for agent-browser --session
|
||||
"bb_session_id": str, # provider session ID (for close/cleanup)
|
||||
"cdp_url": str, # CDP websocket URL
|
||||
"expires_at": str, # optional provider-authoritative ISO timestamp
|
||||
"features": dict, # feature flags that were enabled
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -246,6 +246,10 @@ class BrowserUseBrowserProvider(BrowserProvider):
|
|||
"session_name": session_name,
|
||||
"bb_session_id": session_data["id"],
|
||||
"cdp_url": cdp_url,
|
||||
# Browser Use sessions have a fixed server-side lifetime. Preserve
|
||||
# the authority returned by the API so the dispatcher can retire an
|
||||
# expired CDP endpoint instead of reconnecting to it indefinitely.
|
||||
"expires_at": session_data.get("timeoutAt"),
|
||||
"features": {"browser_use": True},
|
||||
"external_call_id": external_call_id,
|
||||
}
|
||||
|
|
|
|||
|
|
@ -0,0 +1,93 @@
|
|||
"""Regression coverage for provider-authoritative cloud browser expiry."""
|
||||
|
||||
from unittest.mock import Mock
|
||||
|
||||
import tools.browser_tool as browser_tool
|
||||
from plugins.browser.browser_use import provider as browser_use_provider
|
||||
|
||||
|
||||
def _isolate_browser_state(monkeypatch):
|
||||
monkeypatch.setattr(browser_tool, "_active_sessions", {})
|
||||
monkeypatch.setattr(browser_tool, "_session_last_activity", {})
|
||||
monkeypatch.setattr(browser_tool, "_start_browser_cleanup_thread", lambda: None)
|
||||
monkeypatch.setattr(browser_tool, "_ensure_cdp_supervisor", lambda task_id: None)
|
||||
|
||||
|
||||
def test_browser_use_preserves_provider_timeout(monkeypatch):
|
||||
provider = browser_use_provider.BrowserUseBrowserProvider()
|
||||
response = Mock(
|
||||
ok=True,
|
||||
headers={},
|
||||
)
|
||||
response.json.return_value = {
|
||||
"id": "browser-session-1",
|
||||
"cdpUrl": "ws://browser-use.example/devtools/browser/1",
|
||||
"timeoutAt": "2030-01-01T00:05:00Z",
|
||||
}
|
||||
|
||||
monkeypatch.setattr(
|
||||
provider,
|
||||
"_get_config",
|
||||
lambda: {
|
||||
"api_key": "test-key",
|
||||
"base_url": "https://api.browser-use.example/api/v3",
|
||||
"managed_mode": False,
|
||||
},
|
||||
)
|
||||
monkeypatch.setattr(browser_use_provider.requests, "post", Mock(return_value=response))
|
||||
|
||||
session = provider.create_session("task-1")
|
||||
|
||||
assert session["expires_at"] == "2030-01-01T00:05:00Z"
|
||||
|
||||
|
||||
def test_live_cloud_session_is_reused(monkeypatch):
|
||||
_isolate_browser_state(monkeypatch)
|
||||
existing = {
|
||||
"session_name": "existing",
|
||||
"bb_session_id": "browser-session-1",
|
||||
"cdp_url": "ws://browser-use.example/devtools/browser/1",
|
||||
"expires_at": "2999-01-01T00:05:00Z",
|
||||
}
|
||||
browser_tool._active_sessions["task-1"] = existing
|
||||
provider = Mock()
|
||||
monkeypatch.setattr(browser_tool, "_get_cloud_provider", lambda: provider)
|
||||
|
||||
session = browser_tool._get_session_info("task-1")
|
||||
|
||||
assert session is existing
|
||||
provider.create_session.assert_not_called()
|
||||
|
||||
|
||||
def test_expired_cloud_session_is_replaced_without_reusing_dead_cdp(monkeypatch):
|
||||
_isolate_browser_state(monkeypatch)
|
||||
browser_tool._active_sessions["task-1"] = {
|
||||
"session_name": "expired",
|
||||
"bb_session_id": "browser-session-old",
|
||||
"cdp_url": "ws://browser-use.example/devtools/browser/old",
|
||||
"expires_at": "2020-01-01T00:05:00Z",
|
||||
}
|
||||
browser_tool._session_last_activity["task-1"] = 1.0
|
||||
|
||||
provider = Mock()
|
||||
provider.create_session.return_value = {
|
||||
"session_name": "replacement",
|
||||
"bb_session_id": "browser-session-new",
|
||||
"cdp_url": "ws://browser-use.example/devtools/browser/new",
|
||||
"expires_at": "2999-01-01T00:05:00Z",
|
||||
}
|
||||
monkeypatch.setattr(browser_tool, "_get_cloud_provider", lambda: provider)
|
||||
monkeypatch.setattr(browser_tool, "_get_cdp_override", lambda: "")
|
||||
monkeypatch.setattr(browser_tool, "_stop_cdp_supervisor", Mock())
|
||||
monkeypatch.setattr(browser_tool, "_maybe_stop_recording", Mock())
|
||||
monkeypatch.setattr(browser_tool, "_run_browser_command", Mock())
|
||||
monkeypatch.setattr(browser_tool.os.path, "exists", lambda path: False)
|
||||
|
||||
session = browser_tool._get_session_info("task-1")
|
||||
|
||||
assert session["bb_session_id"] == "browser-session-new"
|
||||
assert browser_tool._active_sessions["task-1"] is session
|
||||
assert "task-1" in browser_tool._session_last_activity
|
||||
provider.close_session.assert_called_once_with("browser-session-old")
|
||||
provider.create_session.assert_called_once_with("task-1")
|
||||
browser_tool._run_browser_command.assert_not_called()
|
||||
|
|
@ -61,6 +61,7 @@ import sys
|
|||
import tempfile
|
||||
import threading
|
||||
import time
|
||||
from datetime import datetime, timezone
|
||||
from typing import Dict, Any, Optional, List, Tuple, Union
|
||||
from pathlib import Path
|
||||
from agent.redact import redact_cdp_url
|
||||
|
|
@ -1568,6 +1569,42 @@ _cleanup_running = False
|
|||
_cleanup_lock = threading.Lock()
|
||||
|
||||
|
||||
def _session_expiry_timestamp(session_info: Dict[str, Any]) -> Optional[float]:
|
||||
"""Return a provider-authoritative session expiry as epoch seconds.
|
||||
|
||||
Cloud providers may omit ``expires_at``. Unknown or malformed values are
|
||||
therefore treated as having no known expiry, preserving the existing
|
||||
lifecycle for local browsers and providers without an expiry contract.
|
||||
"""
|
||||
value = session_info.get("expires_at")
|
||||
if isinstance(value, (int, float)) and not isinstance(value, bool):
|
||||
return float(value)
|
||||
if not isinstance(value, str) or not value.strip():
|
||||
return None
|
||||
|
||||
normalized = value.strip()
|
||||
if normalized.endswith(("Z", "z")):
|
||||
normalized = f"{normalized[:-1]}+00:00"
|
||||
try:
|
||||
parsed = datetime.fromisoformat(normalized)
|
||||
except ValueError:
|
||||
logger.warning("Ignoring invalid cloud browser session expiry timestamp")
|
||||
return None
|
||||
if parsed.tzinfo is None:
|
||||
parsed = parsed.replace(tzinfo=timezone.utc)
|
||||
return parsed.timestamp()
|
||||
|
||||
|
||||
def _session_has_expired(
|
||||
session_info: Dict[str, Any], *, now: Optional[float] = None
|
||||
) -> bool:
|
||||
"""Return whether a cached browser session crossed its provider deadline."""
|
||||
expires_at = _session_expiry_timestamp(session_info)
|
||||
if expires_at is None:
|
||||
return False
|
||||
return (time.time() if now is None else now) >= expires_at
|
||||
|
||||
|
||||
def _emergency_cleanup_all_sessions():
|
||||
"""
|
||||
Emergency cleanup of all active browser sessions.
|
||||
|
|
@ -2152,7 +2189,22 @@ def _get_session_info(task_id: Optional[str] = None) -> Dict[str, Any]:
|
|||
with _cleanup_lock:
|
||||
# Check if we already have a session for this task
|
||||
if task_id in _active_sessions:
|
||||
return _active_sessions[task_id]
|
||||
existing_session = _active_sessions[task_id]
|
||||
else:
|
||||
existing_session = None
|
||||
|
||||
if existing_session is not None:
|
||||
if not _session_has_expired(existing_session):
|
||||
return existing_session
|
||||
|
||||
logger.info(
|
||||
"Replacing expired cloud browser session for task %s",
|
||||
task_id,
|
||||
)
|
||||
_cleanup_single_browser_session(task_id)
|
||||
# Cleanup removes the activity entry. The replacement session must be
|
||||
# tracked by the inactivity reaper just like an initial session.
|
||||
_update_session_activity(task_id)
|
||||
|
||||
# Hybrid routing: session keys ending with ``::local`` force a local
|
||||
# Chromium regardless of the globally-configured cloud provider. Public
|
||||
|
|
@ -4539,12 +4591,23 @@ def _cleanup_single_browser_session(task_id: str) -> None:
|
|||
# Stop auto-recording before closing (saves the file)
|
||||
_maybe_stop_recording(task_id)
|
||||
|
||||
# Try to close via agent-browser first (needs session in _active_sessions)
|
||||
try:
|
||||
_run_browser_command(task_id, "close", [], timeout=10)
|
||||
logger.debug("agent-browser close command completed for task %s", task_id)
|
||||
except Exception as e:
|
||||
logger.warning("agent-browser close failed for task %s: %s", task_id, e)
|
||||
# An expired cloud CDP URL cannot accept an agent-browser close command.
|
||||
# Avoid feeding it back through _get_session_info(), which would try to
|
||||
# renew the session recursively while cleanup is still in progress.
|
||||
if _session_has_expired(session_info):
|
||||
logger.debug(
|
||||
"Skipping agent-browser close for expired session %s",
|
||||
task_id,
|
||||
)
|
||||
else:
|
||||
try:
|
||||
_run_browser_command(task_id, "close", [], timeout=10)
|
||||
logger.debug(
|
||||
"agent-browser close command completed for task %s",
|
||||
task_id,
|
||||
)
|
||||
except Exception as e:
|
||||
logger.warning("agent-browser close failed for task %s: %s", task_id, e)
|
||||
|
||||
# Now remove from tracking under lock
|
||||
with _cleanup_lock:
|
||||
|
|
|
|||
Loading…
Reference in New Issue