diff --git a/src/fara/agents/fara/fara15_agent.py b/src/fara/agents/fara/fara15_agent.py index c7c1632..2b1c76d 100644 --- a/src/fara/agents/fara/fara15_agent.py +++ b/src/fara/agents/fara/fara15_agent.py @@ -142,6 +142,7 @@ def __init__( self.config: Fara15AgentConfig self.logger = logging.getLogger(__name__) self._client: ChatCompletionClient | None = None + self._owns_client = False self._state: Fara15AgentState | None = None self._pending_observation: str = "" self._allowed_actions: frozenset[str] = frozenset() @@ -171,8 +172,10 @@ async def initialize(self, run_context: RunContext) -> None: if self.config.client_config is not None: self._client = create_client_from_config(self.config.client_config) + self._owns_client = True elif self.config.client is not None: self._client = self.config.client + self._owns_client = False else: raise ValueError("Either client or client_config must be provided") @@ -355,11 +358,20 @@ async def run( async def close(self, run_context: RunContext) -> None: """Cleanup after the agent is done.""" + client = self._client + owns_client = self._owns_client self._state = None self._client = None + self._owns_client = False self._captcha_timeouts = 0 self._captcha_disabled = self.config.captcha_timeout_limit <= 0 - await super().close(run_context) + try: + if owns_client and client is not None: + await client.close() + except Exception as error: + self.logger.warning("Error closing model client: %s", error) + finally: + await super().close(run_context) def _get_final_answer(self, thoughts: str, action_description: str) -> str: return action_description diff --git a/src/fara/clients/wrapper.py b/src/fara/clients/wrapper.py index edecfa1..4cbd95e 100644 --- a/src/fara/clients/wrapper.py +++ b/src/fara/clients/wrapper.py @@ -50,3 +50,12 @@ async def create( usage=usage, finish_reason=response.choices[0].finish_reason, ) + + async def close(self) -> None: + """Close the underlying HTTP client. + + ``AsyncOpenAI`` owns an async httpx connection pool. Leaving it for + garbage collection can make it try to close sockets after + ``asyncio.run`` has already closed the event loop. + """ + await self._client.close() diff --git a/src/fara/environments/playwright/environment.py b/src/fara/environments/playwright/environment.py index d89f922..c114dbb 100644 --- a/src/fara/environments/playwright/environment.py +++ b/src/fara/environments/playwright/environment.py @@ -408,43 +408,74 @@ async def wait_for_captcha_resolution(self) -> None: await self._captcha_event.wait() async def close(self) -> None: - """Close the browser and cleanup.""" + """Close all browser resources, including after a partial startup. + + Each resource is attempted independently so an already-closed page does + not prevent the HTTP/browser transports later in the stack from being + shut down. Clearing references before awaiting also makes this method + safe to call more than once. + """ self.logger.info("Closing browser...") - if self._page: - await self._page.close() - self._page = None + page, self._page = self._page, None + if page: + try: + if not page.is_closed(): + await page.close() + except Exception as error: + self.logger.warning("Error closing browser page: %s", error) - if self._context: - await self._context.close() - self._context = None + context, self._context = self._context, None + if context: + try: + await context.close() + except Exception as error: + self.logger.warning("Error closing browser context: %s", error) - if self._browser: + browser, self._browser = self._browser, None + if browser: if self.config.use_browserbase and self._session and self._bb: project_id = self.config.browserbase_project_id or os.environ.get( "BROWSERBASE_PROJECT_ID" ) session_id = self._session.id - browser_connected = self._browser.is_connected() - self._bb.sessions.update( - self._session.id, - status="REQUEST_RELEASE", - project_id=project_id, - ) - self.logger.info( - f"[BB-END] task={self._task_id} session={session_id} " - f"browser_connected_at_teardown={browser_connected}" - ) - self._session = None - await self._browser.close() - self._browser = None + try: + browser_connected = browser.is_connected() + self._bb.sessions.update( + self._session.id, + status="REQUEST_RELEASE", + project_id=project_id, + ) + self.logger.info( + f"[BB-END] task={self._task_id} session={session_id} " + f"browser_connected_at_teardown={browser_connected}" + ) + except Exception as error: + self.logger.warning( + "Error releasing BrowserBase session %s: %s", + session_id, + error, + ) + finally: + self._session = None + try: + if browser.is_connected(): + await browser.close() + except Exception as error: + self.logger.warning("Error closing browser: %s", error) - if self._playwright: - await self._playwright.stop() - self._playwright = None + playwright, self._playwright = self._playwright, None + if playwright: + try: + await playwright.stop() + except Exception as error: + self.logger.warning("Error stopping Playwright: %s", error) if not self.config.headless: - self._stop_xvfb() + try: + self._stop_xvfb() + except Exception as error: + self.logger.warning("Error stopping Xvfb: %s", error) self._initialized = False diff --git a/src/fara/fara_7b/fara_agent.py b/src/fara/fara_7b/fara_agent.py index 62fbc85..556b858 100644 --- a/src/fara/fara_7b/fara_agent.py +++ b/src/fara/fara_7b/fara_agent.py @@ -600,4 +600,9 @@ async def close(self) -> None: """ if self._page is not None: self._page = None - await self.browser_manager.close() + try: + if self._openai_client is not None: + await self._openai_client.close() + finally: + self._openai_client = None + await self.browser_manager.close() diff --git a/src/fara/run_fara.py b/src/fara/run_fara.py index 18509ee..a03984a 100755 --- a/src/fara/run_fara.py +++ b/src/fara/run_fara.py @@ -108,9 +108,9 @@ async def run_fara15_agent( run_context = RunContext.create( environment=env, task=task, output_dir=output_dir ) - await agent.initialize(run_context) try: + await agent.initialize(run_context) print("Running Fara...\n") final_answer, _, _ = await agent.run(run_context) diff --git a/tests/test_resource_cleanup.py b/tests/test_resource_cleanup.py new file mode 100644 index 0000000..b46d609 --- /dev/null +++ b/tests/test_resource_cleanup.py @@ -0,0 +1,82 @@ +"""Resource ownership and cleanup regression tests.""" + +import asyncio + +from fara.agents.fara.fara15_agent import Fara15Agent, Fara15AgentConfig +from fara.environments.playwright import PlaywrightEnvironment + + +class FakeClient: + def __init__(self): + self.close_calls = 0 + + async def close(self): + self.close_calls += 1 + + +def _make_agent(): + return Fara15Agent( + Fara15AgentConfig(client_config={"model": "m", "base_url": "u", "api_key": "k"}) + ) + + +def test_agent_closes_only_clients_it_owns(): + owned = FakeClient() + agent = _make_agent() + agent._client = owned + agent._owns_client = True + asyncio.run(agent.close(None)) + assert owned.close_calls == 1 + + external = FakeClient() + agent = _make_agent() + agent._client = external + agent._owns_client = False + asyncio.run(agent.close(None)) + assert external.close_calls == 0 + + +def test_browser_cleanup_continues_after_one_resource_fails(): + class FailingPage: + def is_closed(self): + return False + + async def close(self): + raise RuntimeError("page is already gone") + + class FakeContext: + closed = False + + async def close(self): + self.closed = True + + class FakeBrowser: + closed = False + + def is_connected(self): + return not self.closed + + async def close(self): + self.closed = True + + class FakePlaywright: + stopped = False + + async def stop(self): + self.stopped = True + + environment = PlaywrightEnvironment(headless=True) + context = FakeContext() + browser = FakeBrowser() + playwright = FakePlaywright() + environment._page = FailingPage() + environment._context = context + environment._browser = browser + environment._playwright = playwright + + asyncio.run(environment.close()) + asyncio.run(environment.close()) + + assert context.closed + assert browser.closed + assert playwright.stopped