feat: add session tracking and fix reinitialization for AgentCoreBrowser #319
Conversation
bennorwood
had a problem deploying
to
manual-approval
GitHub Actions
Failure
Implement proper session lifecycle management for browser instances: Code changes: - Add _client_dict to track active browser sessions by session_id - Populate _client_dict in create_browser_session() for session tracking - Register atexit handler in __init__() to ensure cleanup on process exit - Ensures browser sessions are properly closed when process terminates Tests: - test_bedrock_browser_create_browser_session_hydrates_client_dict: Verifies _client_dict is populated when sessions are created - test_bedrock_browser_registers_atexit_handler: Confirms cleanup handler is registered at initialization This prevents resource leaks from orphaned browser sessions and ensures all active sessions are tracked and cleaned up properly.
Set _started = False in _async_cleanup() to allow playwright to reinitialize when creating new sessions after close. Previously, _started remained True after cleanup, preventing _start() from being called and leaving _playwright as None, which caused "Playwright not initialized" errors. Add test to verify sessions can be created successfully after close.
bennorwood
force-pushed
the
bennorw/agentcore-browser/bugfixes/client-session-handling
branch
from
61caf4a to
2437bc8
Compare
bennorwood
had a problem deploying
to
manual-approval
GitHub Actions
Failure
| asyncio.set_event_loop(self._loop) | ||
| self._nest_asyncio_applied = False | ||
| self._sessions: Dict[str, BrowserSession] = {} | ||
| atexit.register(self.close_platform) |
There was a problem hiding this comment.
Question: Is this session close at exit needed? It makes sense to leave the invoking of the close_platform method up to the end user of this class.
Put another way: Should this class leave the lifecycle management of the session to the user, or assume that if the process ends so does the remote session?
There was a problem hiding this comment.
Yes, I believe the atexit handler is the correct approach. From a user's perspective, especially during development and testing, it's very common to stop a script midway through execution. This handler acts as a crucial safeguard to prevent orphaned resources and unnecessary costs, which is exactly the problem I was facing. It makes the tool much more robust and user-friendly.
|
Thank you, @bennorwood , for this excellent fix. I pulled this branch locally and can confirm that it resolves the resource leak issue with browser_tool = AgentCoreBrowser(identifier="aws.browser.v1", region="us-west-2")
agent = Agent(tools=[browser_tool.browser])
agent("open the youtube then close it")This is a great contribution that solves several critical bugs. Hoping to see this merged soon! @dbschmigelski @mkmeral |
2 participants
Description
This PR adds session lifecycle management for AgentCoreBrowser and fixes a bug preventing session reinitialization after close.
Changes
Session Tracking and Cleanup
_client_dictto track active browser sessions by session_id_client_dictincreate_browser_session()for proper session tracking__init__()to ensure cleanup on process exitBug Fix: Session Reinitialization
_started = Falsein_async_cleanup()to allow playwright to reinitialize_startedremainedTrueafter cleanup, preventing_start()from being called againTesting
test_bedrock_browser_create_browser_session_hydrates_client_dict: Verifies_client_dictis populated when sessions are createdtest_bedrock_browser_registers_atexit_handler: Confirms cleanup handler is registered at initializationtest_bedrock_browser_reinitializes_playwright_after_close: Verifies sessions can be created, closed, and recreated successfullyAll tests pass locally with
hatch test tests/browser/test_agent_core_browser.pyChecklist
hatch fmt --formatter)hatch fmt --linter)hatch test)Related Issues
Closes #286
Closes #205