From 10956dd6a48b38d2377773d2a21372335f33a6a0 Mon Sep 17 00:00:00 2001 From: Edgar Arakelyan Date: Sat, 14 Nov 2026 21:22:27 -0800 Subject: [PATCH 1/3] fix(website_browser): a context's disposal waits for agentenv's own pages in it to close (FD-5842) Chromium crashes (a NOTREACHED in browser_context_impl.cc) when a browser context is disposed while it holds a hidden page. browser/v1 opens one in a context to delete cookies and to read or write local storage, and nothing kept a disposal from landing meanwhile. BrowserControl.disposing(context) now marks a context as being disposed and waits for the hidden pages open in it to close; hidden_page refuses a context being disposed with TargetUnavailable. Both ways a context is disposed go through it: Contexts.release, and the CDP relay, which holds an agent's Target.disposeBrowserContext (and that connection's later commands, keeping their order) until agentenv's pages in that context have closed, then forwards it. Other contexts aren't held. Co-Authored-By: Claude Opus 5.5 --- .../agentenv_protocol/website_browser/cdp.py | 16 ++++-- .../website_browser/contexts.py | 10 ++-- .../website_browser/control.py | 45 +++++++++++---- .../tests/test_browser_cdp.py | 56 ++++++++++++++++++- .../tests/test_browser_contexts.py | 56 +++++++++++++++++++ 5 files changed, 163 insertions(+), 20 deletions(-) diff --git a/packages/agentenv-protocol/src/agentenv_protocol/website_browser/cdp.py b/packages/agentenv-protocol/src/agentenv_protocol/website_browser/cdp.py index f935be119..a70dd89ee 100644 --- a/packages/agentenv-protocol/src/agentenv_protocol/website_browser/cdp.py +++ b/packages/agentenv-protocol/src/agentenv_protocol/website_browser/cdp.py @@ -275,11 +275,12 @@ class Relay: who connected when, and what the guard did to their commands. ``dialogs_answered`` says agentenv answers the browser's dialogs itself. With a ``log``, each command is an entry in it, on the tab its session is on, and ``control``, agentenv's own connection, tells its tab's URL, whether a password field has focus there, and what - causes the tab events a command brings. An agent's tool call a driver serves through a CDP user's connections, - such as Playwright MCP's, is an entry too, which that user's commands are credited to while it's open. With - ``contexts``, a context a connection makes is leased to its user, only one it made may it dispose, and a lease's - downloads stay in its folder. A connection is scoped to a lease or to the default context, and sees only its scope - and the contexts its user made (``scope``).""" + causes the tab events a command brings; an agent's disposal of a context waits for ``control``'s own pages open + there to close (``BrowserControl.disposing``), and its connection's later commands wait with it. An agent's tool + call a driver serves through a CDP user's connections, such as Playwright MCP's, is an entry too, which that user's + commands are credited to while it's open. With ``contexts``, a context a connection makes is leased to its user, + only one it made may it dispose, and a lease's downloads stay in its folder. A connection is scoped to a lease or to + the default context, and sees only its scope and the contexts its user made (``scope``).""" def __init__(self, devtools: DevToolsPipe, *, dialogs_answered: bool = False, log: Optional[BrowserLog] = None, control: Optional[BrowserControl] = None, contexts: Optional[Contexts] = None) -> None: @@ -400,6 +401,7 @@ async def _command(self, connection: _Connection, message: Any) -> None: logged = self._begin(connection, session, method, params) answer, guarded = {"error": {"code": SERVER_ERROR, "message": ENDED_UNANSWERED}}, None sending: Optional[asyncio.Future] = None + disposal = contextlib.AsyncExitStack() replied = False def reply(answer: dict) -> None: @@ -423,6 +425,9 @@ def reply(answer: dict) -> None: elif verdict.action != "send": answer = {"result": {}} else: + if method == ENDS_CONTEXT and self.control is not None and \ + isinstance(context := verdict.params.get("browserContextId"), str): + await disposal.enter_async_context(self.control.disposing(context)) sending = asyncio.ensure_future(self.devtools.request(method, verdict.params, session_id=session, answered=reply)) await asyncio.sleep(0) @@ -441,6 +446,7 @@ def reply(answer: dict) -> None: finally: if sending is not None and not sending.done(): sending.cancel() + await disposal.aclose() if logged is not None: logged.finish(answer, guarded) diff --git a/packages/agentenv-protocol/src/agentenv_protocol/website_browser/contexts.py b/packages/agentenv-protocol/src/agentenv_protocol/website_browser/contexts.py index 3e58a0978..2405de600 100644 --- a/packages/agentenv-protocol/src/agentenv_protocol/website_browser/contexts.py +++ b/packages/agentenv-protocol/src/agentenv_protocol/website_browser/contexts.py @@ -133,15 +133,17 @@ def grant(self, label: str, user: str) -> Lease: return lease async def release(self, label: str) -> bool: - """The lease ``label`` ended: its context disposed, which closes its tabs, and its folder deleted. Whether - there was one: releasing a label that isn't leased does nothing, so a retried step is harmless.""" + """The lease ``label`` ended: its context disposed once agentenv's own pages in it have closed, which closes its + tabs, and its folder deleted. Whether there was one: releasing a label that isn't leased does nothing, so a + retried step is harmless.""" async with self._lock: if (lease := self.leases.pop(label, None)) is None: return False self.control.closing_context(lease.id) self._ended(lease) - with contextlib.suppress(DevToolsError): - await self.devtools.call("Target.disposeBrowserContext", {"browserContextId": lease.id}) + async with self.control.disposing(lease.id): + with contextlib.suppress(DevToolsError): + await self.devtools.call("Target.disposeBrowserContext", {"browserContextId": lease.id}) await asyncio.to_thread(shutil.rmtree, lease.folder(self.home()), True) return True diff --git a/packages/agentenv-protocol/src/agentenv_protocol/website_browser/control.py b/packages/agentenv-protocol/src/agentenv_protocol/website_browser/control.py index de05263b3..d36d041cf 100644 --- a/packages/agentenv-protocol/src/agentenv_protocol/website_browser/control.py +++ b/packages/agentenv-protocol/src/agentenv_protocol/website_browser/control.py @@ -15,7 +15,7 @@ from urllib.parse import urlsplit from .log import ACTOR, KEY_FIELDS, TYPING_METHODS, BrowserLog, Causes, caused, log_time, redact_url -from ..computer.errors import DeviceUnavailable, InvalidArgument +from ..computer.errors import DeviceUnavailable, InvalidArgument, TargetUnavailable from .devtools import DevToolsError, DevToolsPipe from .shapes import (ClosedTab, Cookie, CookieToSet, Dialog, Download, OpenTab, OriginStorage, StartTab, StorageItem, origin_of) @@ -108,6 +108,10 @@ def __init__(self, devtools: DevToolsPipe, dialogs: str, viewport: Optional[tupl self.dialog_log: list[dict] = [] self._sessions: dict[str, str] = {} self._hidden: set[str] = set() + #: What each page of agentenv's own open now sets as it closes, with its context, None for the default one; and + #: the contexts being disposed, once per disposal under way, which open no more (``disposing``). + self._hidden_pages: dict[asyncio.Event, Optional[str]] = {} + self._disposing: list[str] = [] self._closing: set[str] = set() self._open_dialogs: dict[str, dict] = {} self._answering: set[str] = set() @@ -550,17 +554,38 @@ async def _bring_to_front(self, target: str) -> None: @contextlib.asynccontextmanager async def hidden_page(self, url: str = "about:blank", context: Optional[str] = None) -> AsyncIterator[str]: """A page of agentenv's own for reading state, in the context ``context``, else the default one, in no window - and never in the record; its session's id. It's closed after.""" - target = (await self.devtools.call("Target.createTarget", { - "url": url, "hidden": True, "background": True, - **({"browserContextId": context} if context else {})}))["targetId"] - self._hidden.add(target) - self.tabs.pop(target, None) + and never in the record; its session's id. It's closed after, before its context is disposed; + TargetUnavailable for a context being disposed.""" + if context in self._disposing: + raise TargetUnavailable(f"the context {self.other_contexts.get(context, context)!r} is being disposed") + closed = asyncio.Event() + self._hidden_pages[closed] = context try: - yield (await self.devtools.call("Target.attachToTarget", {"targetId": target, "flatten": True}))["sessionId"] + target = (await self.devtools.call("Target.createTarget", { + "url": url, "hidden": True, "background": True, **_in(context)}))["targetId"] + self._hidden.add(target) + self.tabs.pop(target, None) + try: + attached = await self.devtools.call("Target.attachToTarget", {"targetId": target, "flatten": True}) + yield attached["sessionId"] + finally: + with contextlib.suppress(ConnectionError, DevToolsError): + await self.devtools.call("Target.closeTarget", {"targetId": target}) + finally: + del self._hidden_pages[closed] + closed.set() + + @contextlib.asynccontextmanager + async def disposing(self, context: str) -> AsyncIterator[None]: + """The context ``context`` disposed within: from its start no page of agentenv's own opens in it, and it waits + for those open there to close, as Chromium crashes disposing a context that holds a hidden page.""" + self._disposing.append(context) + try: + for closed in [closed for closed, of in self._hidden_pages.items() if of == context]: + await closed.wait() + yield finally: - with contextlib.suppress(ConnectionError, DevToolsError): - await self.devtools.call("Target.closeTarget", {"targetId": target}) + self._disposing.remove(context) async def active_tab(self, context: Optional[str] = None) -> Optional[str]: """The active tab of the context ``context``, else of the default one, as ``page_tab`` picks among its tabs.""" diff --git a/packages/agentenv-protocol/tests/test_browser_cdp.py b/packages/agentenv-protocol/tests/test_browser_cdp.py index 48412ae9d..93a33845a 100644 --- a/packages/agentenv-protocol/tests/test_browser_cdp.py +++ b/packages/agentenv-protocol/tests/test_browser_cdp.py @@ -30,7 +30,7 @@ from agentenv_protocol.computer import Launch, Platform, Ready, app, computer_app, linux # noqa: E402 from agentenv_protocol.computer.app import APP_SETTINGS_ENV # noqa: E402 from agentenv_protocol.computer.app_runtime import AppReadiness # noqa: E402 -from agentenv_protocol.computer.errors import DeviceUnavailable, InvalidArgument # noqa: E402 +from agentenv_protocol.computer.errors import DeviceUnavailable, InvalidArgument, TargetUnavailable # noqa: E402 from agentenv_protocol.website_browser import SITE_PROXY_ENV, WebsiteBrowserEnvironment # noqa: E402 FORWARDED = {"x-forwarded-host": "computer.test:443", "x-forwarded-proto": "https", @@ -112,6 +112,30 @@ async def request(self, method, params=None, *, session_id=None, answered=None): return await super().request(method, params, session_id=session_id, answered=answered) +class PagesPipe(ContextPipe): + """A ContextPipe on which agentenv's own pages are ``H``, keeping in ``ended``, in order, each page agentenv + closes and each context an agent disposes.""" + + def __init__(self) -> None: + super().__init__() + self.pages, self.ended = 0, [] + + async def call(self, method, params=None, *, session_id=None, timeout_s=None): + if method == "Target.createTarget": + self.pages += 1 + return {"targetId": f"H{self.pages}"} + if method == "Target.attachToTarget": + return {"sessionId": f"s-{params['targetId']}"} + if method == "Target.closeTarget": + self.ended.append(("closed", params["targetId"])) + return await super().call(method, params, session_id=session_id, timeout_s=timeout_s) + + async def request(self, method, params=None, *, session_id=None, answered=None): + if method == "Target.disposeBrowserContext": + self.ended.append(("disposed", params["browserContextId"])) + return await super().request(method, params, session_id=session_id, answered=answered) + + class ScopePipe(ContextPipe): """A browser whose default context ``DEF`` has the page ``P0``, and whose task leases ``L1`` and ``L2`` have the pages ``PB`` and ``PS`` and ``L1`` the worker ``WB``: a connection's setDiscoverTargets announces them, getTargets @@ -398,6 +422,36 @@ async def test_a_context_an_agent_makes_is_its_lease_keeping_downloads_in_its_fo assert [entry["did"] for entry in relay.connections[0]["guarded"]] == ["changed", "refused"] +@pytest.mark.asyncio +async def test_an_agents_dispose_waits_for_agentenvs_own_page_in_its_context_which_takes_no_new_one_meanwhile( + tmp_path): + pipe = PagesPipe() + control = BrowserControl(pipe, "agent") + contexts = Contexts(pipe, control, home=lambda: tmp_path) + async with _relay(pipe=pipe, contexts=contexts, control=control) as (_, relay, base): + async with connect(f"{base}/ada") as ada, connect(f"{base}/ada") as again: + for n in (1, 2): + await _ask(ada, {"id": n, "method": "Target.createBrowserContext", "params": {}}) + async with control.hidden_page(context="A1"): + await ada.send(json.dumps({"id": 3, "method": "Target.disposeBrowserContext", + "params": {"browserContextId": "A1"}})) + for _ in range(500): + if "A1" in control._disposing: + break + await asyncio.sleep(0.01) + with pytest.raises(TargetUnavailable, match="the context 'agent-ada-1' is being disposed"): + async with control.hidden_page(context="A1"): + pass + other = await _ask(again, {"id": 1, "method": "Target.disposeBrowserContext", + "params": {"browserContextId": "A2"}}) + held = list(pipe.ended) + disposed = json.loads(await asyncio.wait_for(ada.recv(), 5)) + + assert held == [("disposed", "A2")] and "result" in other + assert pipe.ended == [("disposed", "A2"), ("closed", "H1"), ("disposed", "A1")] + assert disposed["id"] == 3 and "result" in disposed and list(contexts) == [] and control._disposing == [] + + @pytest.mark.asyncio async def test_a_scoped_address_sees_its_lease_as_the_default_and_nothing_of_another_context(tmp_path): pipe, contexts = await _scoped(tmp_path) diff --git a/packages/agentenv-protocol/tests/test_browser_contexts.py b/packages/agentenv-protocol/tests/test_browser_contexts.py index acf473c09..d802ad35c 100644 --- a/packages/agentenv-protocol/tests/test_browser_contexts.py +++ b/packages/agentenv-protocol/tests/test_browser_contexts.py @@ -58,6 +58,25 @@ def send(self, method, params, session=None): listener({"method": method, "params": params, **({"sessionId": session} if session else {})}) +class HeldDevTools(StubDevTools): + """A StubDevTools whose pages of agentenv's own are ``H``, which holds each call of ``held`` until ``go`` is set, + ``reached`` set as one arrives.""" + + def __init__(self, held, answers=None): + super().__init__(answers) + self.held, self.reached, self.go, self.pages = held, asyncio.Event(), asyncio.Event(), 0 + + async def call(self, method, params=None, *, session_id=None, timeout_s=None): + if method == "Target.createTarget": + self.pages += 1 + self.calls.append((method, params or {}, session_id)) + return {"targetId": f"H{self.pages}"} + if method == self.held: + self.reached.set() + await self.go.wait() + return await super().call(method, params, session_id=session_id, timeout_s=timeout_s) + + def _sent(devtools, method): return [params for called, params, _ in devtools.calls if called == method] @@ -217,6 +236,43 @@ async def test_a_release_closes_its_tabs_as_agentenvs_deletes_its_folder_and_doe assert await control.tab_list() == [] +@pytest.mark.asyncio +async def test_a_release_waits_for_agentenvs_own_page_in_its_context_which_takes_no_new_one_and_no_other_context_waits( + browser_home): + devtools = HeldDevTools("Network.deleteCookies", { + "Storage.getCookies": {"cookies": [{"name": "sid", "value": "v", "domain": "shop.test", "path": "/", + "expires": -1, "httpOnly": False, "secure": False}]}, + "Target.attachToTarget": {"sessionId": "s-hidden"}, + "Runtime.evaluate": {"result": {"value": "https://shop.test"}}}) + control = BrowserControl(devtools, "agent") + contexts = Contexts(devtools, control, home=lambda: browser_home) + await contexts.lease("buyer") + await contexts.lease("seller") + + async with control.hidden_page(context="C2"): + deleting = asyncio.ensure_future(control.delete_cookies(context="C1")) + await asyncio.wait_for(devtools.reached.wait(), 5) + releasing = asyncio.ensure_future(contexts.release("buyer")) + for _ in range(500): + if "C1" in control._disposing: + break + await asyncio.sleep(0.01) + with pytest.raises(TargetUnavailable, match="the context 'buyer' is being disposed"): + await control.clear_local_storage(["https://shop.test"], "C1") + cleared = await control.clear_local_storage(["https://shop.test"], "C2") + held = _sent(devtools, "Target.disposeBrowserContext") + devtools.go.set() + deleted, released = await asyncio.wait_for(asyncio.gather(deleting, releasing), 5) + ended = [(method, params) for method, params, _ in devtools.calls + if method in ("Target.closeTarget", "Target.disposeBrowserContext")] + + assert held == [] and cleared == ["https://shop.test"] and (deleted, released) == (1, True) + assert ended == [("Target.closeTarget", {"targetId": "H3"}), ("Target.closeTarget", {"targetId": "H2"}), + ("Target.disposeBrowserContext", {"browserContextId": "C1"}), + ("Target.closeTarget", {"targetId": "H1"})] + assert [params.get("browserContextId") for params in _sent(devtools, "Target.createTarget")] == ["C2", "C1", "C2"] + + @pytest.mark.asyncio async def test_a_label_released_and_leased_again_at_once_keeps_the_new_leases_folder(browser_home): _, _, contexts = _contexts(browser_home) From b0e063239796303fffae147c9d37dc1e4ba86e5e Mon Sep 17 00:00:00 2001 From: Edgar Arakelyan Date: Sat, 14 Nov 2026 21:28:25 -0800 Subject: [PATCH 2/3] test(website_browser): a context ended while browser/v1 reads its storage leaves the browser up (FD-5842) The contexts check ends a context six times while three local-storage reads run in it, by a lease's release and by an agent's own dispose over CDP, and asks that every read is answered or refused cleanly and the browser's tabs are still there after. Co-Authored-By: Claude Opus 5.5 --- tst/integration/computer/browser_contexts.py | 39 +++++++++++++++++++- 1 file changed, 37 insertions(+), 2 deletions(-) diff --git a/tst/integration/computer/browser_contexts.py b/tst/integration/computer/browser_contexts.py index bfd57c53c..a58bfedc6 100644 --- a/tst/integration/computer/browser_contexts.py +++ b/tst/integration/computer/browser_contexts.py @@ -2,8 +2,10 @@ to a loopback site as its own user, by cookies and local storage of its own, in tabs of its own the default context doesn't list; five leases of one label at once are one; a lease's download lands in its own folder, listed in it alone; a context an agent makes over CDP is leased to it as agent--, another agent and the task's lease can't be -disposed by it, its own can; data/get carries each lease, and once agents have acted a load still leases a missing one -back with its cookies; and a release, then a reset, end them and delete their folders.""" +disposed by it, its own can; a lease released, or a context an agent disposes, while browser/v1 reads its local storage +through pages of its own in it leaves the browser up, each read answered or refused cleanly; data/get carries each +lease, and once agents have acted a load still leases a missing one back with its cookies; and a release, then a reset, +end them and delete their folders.""" import asyncio import base64 @@ -24,6 +26,11 @@ from agent_env.task_step.task_step import TaskStep from tst.integration.computer.browser_data import ORIGIN, POLL_S, SERVE_S, SERVE_SITE, SITE, WAIT_S +#: How many times a context is ended while browser/v1 reads its local storage, by a release and by an agent's dispose. +ROUNDS = 6 +#: How a read whose context ends under it may answer: read, or refused as the context it names is gone. +SETTLED = {200, 404} + #: A loopback server, for the seconds it's given, whose /file is a download. FILE_SITE = "http://127.0.0.1:8786/file" SERVE_FILE = r"""import http.server, sys, threading, time @@ -137,6 +144,25 @@ async def titled(label: str, title: str) -> dict: ("Target.disposeBrowserContext", {"browserContextId": agents})]) after_dispose = [lease["context"] for lease in (await call("contexts/list", method="GET"))["contexts"]] + ended_under_reads = [] + for n in range(ROUNDS): + label = f"churn-{n}" + await call("contexts/lease", {"label": label}) + reads = [_answer(deployed, op_url, "local_storage/get", {"origins": [ORIGIN], "context": label}) + for _ in range(3)] + *read, _ = await asyncio.gather(*reads, call("contexts/release", {"context": label})) + ended_under_reads.append(read) + [made] = await _cdp_commands(ada, [("Target.createBrowserContext", {})]) + mine = made["result"]["browserContextId"] + lease = next(entry["context"] for entry in (await call("contexts/list", method="GET"))["contexts"] + if entry["id"] == mine) + reads = [_answer(deployed, op_url, "local_storage/get", {"origins": [ORIGIN], "context": lease}) + for _ in range(3)] + *read, disposed = await asyncio.gather( + *reads, _cdp_commands(ada, [("Target.disposeBrowserContext", {"browserContextId": mine})])) + ended_under_reads.append([*read, (200 if "result" in disposed[0] else 500, disposed[0])]) + still_up = [tab["id"] for tab in await tabs()] + state = await _state(deployed, data_url) context = await step("browser_release_context", label="seller").execute(context) released = context.metadata["browser_steps"]["browser_release_context"]["released"] @@ -167,6 +193,8 @@ async def titled(label: str, title: str) -> dict: assert {"context": "buyer", "id": buyer_id, "made_by": "task", "grants": ["ada"]} in listed, listed assert "is another agent's" in theirs["error"]["message"] and "is the task's" in tasks["error"]["message"] assert "result" in own and sorted(after_dispose) == ["buyer", "race", "seller"], (own, after_dispose) + assert all(status in SETTLED for read in ended_under_reads for status, _ in read), ended_under_reads + assert still_up == default_after, (still_up, default_after) assert state["contexts"]["buyer"]["cookies"][0]["value"] == "buyer", state["contexts"] assert any(path.startswith("Contexts/buyer/Downloads/") for path in state["files"]), state["files"] assert released is True and again.metadata["browser_steps"]["browser_release_context"]["released"] is False @@ -175,6 +203,13 @@ async def titled(label: str, title: str) -> dict: assert "target_unavailable" in json.dumps(refused), refused +async def _answer(deployed, op_url: Callable[[str], str], op: str, params: dict) -> tuple[int, dict]: + """``op``'s status and answer, whatever the status.""" + async with httpx.AsyncClient(timeout=120, headers=deployed.control_headers) as http: + answer = await http.post(op_url(op), json=params) + return answer.status_code, answer.json() if answer.content else {} + + async def _state(deployed, data_url: str) -> dict: """The browser's data/get state, with the paths of the files it returned beside it.""" answer = await protocol_v1.get_data(data_url, timeout=300, headers=deployed.control_headers) From 4ce71a7b38306907c8e088e9bfcae66449998adc Mon Sep 17 00:00:00 2001 From: Edgar Arakelyan Date: Sat, 14 Nov 2026 22:09:18 -0800 Subject: [PATCH 3/3] fix(website_browser): a connection's end waits as a dispose does, and a release holds only its own label A connection's detach disposes the contexts it made with disposeOnDetach, so it now waits, as an agent's dispose does, for agentenv's own pages in them to close. Leases take a lock per label, so a release waiting for agentenv's page in its context keeps no other label's lease or release waiting. Co-Authored-By: Claude Opus 5.5 --- .../agentenv_protocol/website_browser/cdp.py | 8 +++-- .../website_browser/contexts.py | 8 +++-- .../tests/test_browser_cdp.py | 32 +++++++++++++++++++ .../tests/test_browser_contexts.py | 11 +++++-- 4 files changed, 51 insertions(+), 8 deletions(-) diff --git a/packages/agentenv-protocol/src/agentenv_protocol/website_browser/cdp.py b/packages/agentenv-protocol/src/agentenv_protocol/website_browser/cdp.py index a70dd89ee..33a991d5c 100644 --- a/packages/agentenv-protocol/src/agentenv_protocol/website_browser/cdp.py +++ b/packages/agentenv-protocol/src/agentenv_protocol/website_browser/cdp.py @@ -372,8 +372,12 @@ async def serve(self, websocket: WebSocket, user: str, context: Optional[str] = for task in [*connection.commands, sending]: task.cancel() await asyncio.gather(*connection.commands, sending, return_exceptions=True) - with contextlib.suppress(Exception): - await self.devtools.call("Target.detachFromTarget", {"sessionId": session}) + # Its detach disposes the contexts it made to end on detach, so it waits as a dispose does. + async with contextlib.AsyncExitStack() as disposal: + for made in connection.ends_on_detach if self.control is not None else (): + await disposal.enter_async_context(self.control.disposing(made)) + with contextlib.suppress(Exception): + await self.devtools.call("Target.detachFromTarget", {"sessionId": session}) self._detached(connection) def ended(self) -> None: diff --git a/packages/agentenv-protocol/src/agentenv_protocol/website_browser/contexts.py b/packages/agentenv-protocol/src/agentenv_protocol/website_browser/contexts.py index 2405de600..60b00a7b1 100644 --- a/packages/agentenv-protocol/src/agentenv_protocol/website_browser/contexts.py +++ b/packages/agentenv-protocol/src/agentenv_protocol/website_browser/contexts.py @@ -6,6 +6,7 @@ from __future__ import annotations import asyncio +import collections import contextlib import logging import re @@ -85,7 +86,8 @@ def __init__(self, devtools: DevToolsPipe, control: BrowserControl, self.on_made: list[Callable[[Lease, Optional[str]], None]] = [] self._made: dict[str, int] = {} self._tasks: set[asyncio.Future] = set() - self._lock = asyncio.Lock() + #: A lock per label, so leasing or releasing one waits only for another of the same label. + self._locks: dict[str, asyncio.Lock] = collections.defaultdict(asyncio.Lock) def __iter__(self): return iter(list(self.leases.values())) @@ -110,7 +112,7 @@ async def lease(self, label: str, *, made_by: str = TASK, grants: tuple[str, ... """The lease ``label``: the one leased under it before, as a retried step finds it, else a new context, its downloads going to its own folder. A task's label is checked; one ``made_by`` an agent may start ``agent-``, as a load of a snapshot restores such leases.""" - async with self._lock: + async with self._locks[label]: if (lease := self.leases.get(label)) is None: if problem := label_problem(label, by_task=made_by == TASK): raise InvalidArgument(problem) @@ -136,7 +138,7 @@ async def release(self, label: str) -> bool: """The lease ``label`` ended: its context disposed once agentenv's own pages in it have closed, which closes its tabs, and its folder deleted. Whether there was one: releasing a label that isn't leased does nothing, so a retried step is harmless.""" - async with self._lock: + async with self._locks[label]: if (lease := self.leases.pop(label, None)) is None: return False self.control.closing_context(lease.id) diff --git a/packages/agentenv-protocol/tests/test_browser_cdp.py b/packages/agentenv-protocol/tests/test_browser_cdp.py index 93a33845a..9042c390b 100644 --- a/packages/agentenv-protocol/tests/test_browser_cdp.py +++ b/packages/agentenv-protocol/tests/test_browser_cdp.py @@ -136,6 +136,15 @@ async def request(self, method, params=None, *, session_id=None, answered=None): return await super().request(method, params, session_id=session_id, answered=answered) +class DetachPipe(PagesPipe): + """A PagesPipe that keeps in ``ended`` each session the relay detaches too.""" + + async def call(self, method, params=None, *, session_id=None, timeout_s=None): + if method == "Target.detachFromTarget": + self.ended.append(("detached", params["sessionId"])) + return await super().call(method, params, session_id=session_id, timeout_s=timeout_s) + + class ScopePipe(ContextPipe): """A browser whose default context ``DEF`` has the page ``P0``, and whose task leases ``L1`` and ``L2`` have the pages ``PB`` and ``PS`` and ``L1`` the worker ``WB``: a connection's setDiscoverTargets announces them, getTargets @@ -452,6 +461,29 @@ async def test_an_agents_dispose_waits_for_agentenvs_own_page_in_its_context_whi assert disposed["id"] == 3 and "result" in disposed and list(contexts) == [] and control._disposing == [] +@pytest.mark.asyncio +async def test_a_connection_ending_waits_for_agentenvs_own_page_in_a_context_it_made_to_end_on_detach(tmp_path): + pipe = DetachPipe() + control = BrowserControl(pipe, "agent") + contexts = Contexts(pipe, control, home=lambda: tmp_path) + async with _relay(pipe=pipe, contexts=contexts, control=control) as (_, relay, base): + async with control.hidden_page(context="A1"): + async with connect(f"{base}/ada") as ada: + await _ask(ada, {"id": 1, "method": "Target.createBrowserContext", "params": {"disposeOnDetach": True}}) + for _ in range(500): + if "A1" in control._disposing: + break + await asyncio.sleep(0.01) + held = [event for event in pipe.ended if event[0] == "detached"] + for _ in range(500): + if any(event[0] == "detached" for event in pipe.ended): + break + await asyncio.sleep(0.01) + + assert held == [] and [event[0] for event in pipe.ended] == ["closed", "detached"] + assert list(contexts) == [] and control._disposing == [] + + @pytest.mark.asyncio async def test_a_scoped_address_sees_its_lease_as_the_default_and_nothing_of_another_context(tmp_path): pipe, contexts = await _scoped(tmp_path) diff --git a/packages/agentenv-protocol/tests/test_browser_contexts.py b/packages/agentenv-protocol/tests/test_browser_contexts.py index d802ad35c..e52a3f1b7 100644 --- a/packages/agentenv-protocol/tests/test_browser_contexts.py +++ b/packages/agentenv-protocol/tests/test_browser_contexts.py @@ -260,14 +260,19 @@ async def test_a_release_waits_for_agentenvs_own_page_in_its_context_which_takes with pytest.raises(TargetUnavailable, match="the context 'buyer' is being disposed"): await control.clear_local_storage(["https://shop.test"], "C1") cleared = await control.clear_local_storage(["https://shop.test"], "C2") - held = _sent(devtools, "Target.disposeBrowserContext") + courier = await asyncio.wait_for(contexts.lease("courier"), 5) + courier_released = await asyncio.wait_for(contexts.release("courier"), 5) + held = [sent for sent in _sent(devtools, "Target.disposeBrowserContext") if sent["browserContextId"] == "C1"] devtools.go.set() deleted, released = await asyncio.wait_for(asyncio.gather(deleting, releasing), 5) ended = [(method, params) for method, params, _ in devtools.calls if method in ("Target.closeTarget", "Target.disposeBrowserContext")] assert held == [] and cleared == ["https://shop.test"] and (deleted, released) == (1, True) - assert ended == [("Target.closeTarget", {"targetId": "H3"}), ("Target.closeTarget", {"targetId": "H2"}), + assert courier.id == "C3" and courier_released + assert ended == [("Target.closeTarget", {"targetId": "H3"}), + ("Target.disposeBrowserContext", {"browserContextId": "C3"}), + ("Target.closeTarget", {"targetId": "H2"}), ("Target.disposeBrowserContext", {"browserContextId": "C1"}), ("Target.closeTarget", {"targetId": "H1"})] assert [params.get("browserContextId") for params in _sent(devtools, "Target.createTarget")] == ["C2", "C1", "C2"] @@ -367,7 +372,7 @@ async def test_a_load_waiting_on_an_agents_lease_that_ends_meanwhile_cant_lease_ _, _, contexts = _contexts(browser_home) contexts.made_by_agent("ada", "X1") - async with contexts._lock: + async with contexts._locks["agent-ada-1"]: waiting = asyncio.ensure_future(contexts.lease("agent-ada-1")) await asyncio.sleep(0) contexts.disposed("X1")