Add explicit browser detach lifecycle#165
Conversation
|
|
||
| You can use the SDK to launch browsers and run automated tasks using natural language instructions. For more examples and code samples, please explore the [`examples/`](examples/) folder in this repository. | ||
|
|
||
| ### Split-script browser handoff |
There was a problem hiding this comment.
This should be moved into a proper example by itself. Make sure to pull latest main before moving, as our example folder structure has changed.
| self._config = config or BrowserConfig() | ||
| self._context = None | ||
| self._attach_to_existing = attach_to_existing | ||
| self._browser: Browser | None = None |
There was a problem hiding this comment.
| self._browser: Browser | None = None | |
| self._playwright_browser: Browser | None = None |
Just to be clearer, as the term "browser" is very overloaded in this context.
| async with self._get_client_lock(): | ||
| await self._detach_impl() | ||
|
|
||
| def _get_client_lock(self) -> asyncio.Lock: |
There was a problem hiding this comment.
Be more explicit about the purpose:
| def _get_client_lock(self) -> asyncio.Lock: | |
| def _acquire_playwright_lifecycle_lock(self) -> asyncio.Lock: |
Also rename self._client_lock to self._playwright_lifecycle_lock
There was a problem hiding this comment.
Ditto for the other environment classes that have this mechanism
| self._client_lock = asyncio.Lock() | ||
| return self._client_lock | ||
|
|
||
| async def _ensure_playwright_connected(self) -> None: |
There was a problem hiding this comment.
These few functions all look exactly the same in this class and the other environment class. Can we extract them into some mixin class, or utility functions? In any case we shouldn't be duplicating them verbatim.
| headers=self._cdp_auth_headers, | ||
| ) | ||
| self._context = self._browser.contexts[0] | ||
| self._get_side_panel_page() |
There was a problem hiding this comment.
I don't think we care about this? The side panel page handle was only used for reloading and
| async def _close_impl(self, *, timeout: int | None = None) -> None: | ||
| pass | ||
|
|
||
| async def detach(self) -> None: |
There was a problem hiding this comment.
Let's make this private _detach. I'm not sure we want to include this in our public API surface, as it's for internal cleanup behind the scene. End users should only use close.
zizhengtai
left a comment
There was a problem hiding this comment.
One lifecycle gap remains in the local initialization failure path.
| await self._initialize_in_existing_browser_window() | ||
| else: | ||
| await self._open_and_initialize_browser_window() | ||
| try: |
There was a problem hiding this comment.
Could we ensure the Playwright client is detached when local browser initialization raises too? Right now this try starts only after _initialize_in_existing_browser_window() / _open_and_initialize_browser_window() returns successfully, so an exception after _start_playwright() leaves the context manager and driver pipe alive. That can still produce the Windows broken-pipe shutdown noise this PR is intended to prevent. Wrapping the initialization body and cleanup in try/finally, plus a failure-path test asserting __aexit__ is awaited, would cover it.
zizhengtai
left a comment
There was a problem hiding this comment.
Two additional lifecycle issues from the second-pass review: refresh time-sensitive cloud CDP credentials on reconnect, and clean up cloud initialization on cancellation.
| assert self._playwright is not None | ||
| self._browser = await self._playwright.chromium.connect_over_cdp( | ||
| self._cdp_websocket_url, | ||
| headers=self._cdp_auth_headers, |
There was a problem hiding this comment.
These CDP auth headers should be refreshed before every reconnect rather than cached from initial session creation. AgentCore Browser's headers are SigV4-signed, and AWS generally rejects signed requests that arrive more than about five minutes after their timestamp. That means reset_agent_state() can fail after a longer-running agent task even though the browser session itself is healthy. The backend already exposes /cloud-browser/generate-cdp-auth-headers; could we call that with the stored WebSocket URL here and use the newly returned headers? References: AgentCore Browser connection example and SigV4 replay protection.
| exc_info=True, | ||
| ) | ||
| return | ||
| except Exception as error: |
There was a problem hiding this comment.
Could we clean up the cloud session and Playwright client when initialization is cancelled? asyncio.CancelledError bypasses this except Exception, so cancelling or timing out env.start() after session creation skips _cleanup_failed_initialization_attempt(): the cloud session remains allocated and the Playwright driver pipe stays open. Please handle cancellation separately, await cleanup, and re-raise without retrying. A regression test can cancel _initialize_once() after it stores the session/client and assert both session cleanup and __aexit__() occur.
…to volodymyr/add-browser-detach-lifecycle
…to volodymyr/add-browser-detach-lifecycle
…-detach-lifecycle # Conflicts: # packages/narada/src/narada/environment.py # packages/narada/tests/test_browser_environment_login.py
zizhengtai
left a comment
There was a problem hiding this comment.
Reviewed the latest branch changes. The lifecycle fixes are focused, the simplifications keep the implementation straightforward, and all checks pass.
Summary
Adds an explicit
env.detach()lifecycle method and makes SDK-owned browser environments detach their local Playwright/CDP client after initialization.This supports split-script workflows where one process starts a browser and exits, while later processes continue using the same Narada browser window through
RemoteBrowserEnvironment.What changed
Environment.detach()as a lifecycle method.BrowserEnvironmentandCloudBrowserEnvironmentafter successful initialization.browser_window_idbrowser_process_idcloud_browser_session_idBrowserEnvironment.close()still closes the Narada browser window.CloudBrowserEnvironment.close()still stops the cloud browser session.detach()only releases local Playwright/CDP resources.reset_agent_state(), then detaches again.RemoteBrowserEnvironment.Motivation
Previously, scripts that only called
await env.start()and then exited could leave Playwright/CDP transports open, especially visible as Windows asyncio Proactor unclosed-transport warnings.With this change,
start()initializes the browser, records the IDs needed for later reuse, and releases local client resources automatically without ending the backing browser/session.Testing