From ed9c4dc95d224b07d066ac9b9bb78b6a935f1298 Mon Sep 17 00:00:00 2001 From: Rui Hu Date: Wed, 17 Jun 2026 17:19:44 +0000 Subject: [PATCH] fix(llmagent): do not close shared cached httpx clients in cleanup() LLMAgent.cleanup() previously iterated over hard-coded provider SDK client attributes (root_async_client, root_client, _async_client, _client) and called close() on each. Recent langchain-anthropic and langchain-openai versions cache their default httpx clients via module-level lru_cache functions. Closing those clients poisoned the cache: every subsequent ChatAnthropic/ChatOpenAI instance in the same process received the same closed httpx client and failed with a connection error. Fix: cleanup() now only releases the agent's own reference to the chat model (self._chat_model = None). The removed _KNOWN_CLIENT_ATTRS class variable has been deleted and ClassVar removed from the typing import. The concurrent-idempotency guarantee is preserved: the lock is still acquired before nulling _chat_model, so two concurrent cleanup() calls cannot both see a non-None model and attempt conflicting operations. The four provider SDK client-closing scenarios in credential_injection.feature and llm_missing_coverage.feature are removed as they tested the old (buggy) behaviour. Their step definitions are removed from credential_cleanup_steps.py (now only carries the resolve_class_ref patch step) and llm_missing_coverage_steps.py is updated with the corrected assertions. Six new regression BDD scenarios tagged @tdd_issue @tdd_issue_57 are added in features/llm_cleanup_shared_client.feature, covering all four provider SDK client attribute paths (Anthropic _async_client/_client, OpenAI root_async_client/root_client) and two end-to-end two-agent scenarios that prove a second agent can run successfully after the first is cleaned up. ISSUES CLOSED: #57 --- CHANGELOG.md | 2 + features/credential_injection.feature | 36 -- features/llm_cleanup_shared_client.feature | 66 ++++ features/llm_missing_coverage.feature | 23 +- features/steps/credential_cleanup_steps.py | 249 +------------- .../steps/llm_cleanup_shared_client_steps.py | 317 ++++++++++++++++++ features/steps/llm_missing_coverage_steps.py | 42 +-- src/cleveractors/agents/llm.py | 94 ++---- 8 files changed, 446 insertions(+), 383 deletions(-) create mode 100644 features/llm_cleanup_shared_client.feature create mode 100644 features/steps/llm_cleanup_shared_client_steps.py diff --git a/CHANGELOG.md b/CHANGELOG.md index ee79c0e..d9df082 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,8 @@ and this project adheres to [Clever Semantic Versioning](https://www.w3.org/subm ### Fixed +- **LLMAgent.cleanup() no longer closes shared cached httpx clients (issue #57)**: `LLMAgent.cleanup()` previously iterated over hard-coded provider SDK client attributes (`root_async_client`, `root_client`, `_async_client`, `_client`) and called `close()` on each. Recent versions of `langchain-anthropic` and `langchain-openai` cache their default httpx clients via module-level `lru_cache` functions; closing those clients poisoned the cache so every subsequent `ChatAnthropic`/`ChatOpenAI` instance in the same process received the same closed httpx client and failed with a connection error. `cleanup()` now only releases the agent's own reference to the chat model (`self._chat_model = None`). The removed `_KNOWN_CLIENT_ATTRS` class variable has been deleted. Six regression BDD scenarios (tagged `@tdd_issue_57`) covering all four provider SDK client attribute paths (Anthropic `_async_client`/`_client`, OpenAI `root_async_client`/`root_client`) guard against future regressions. + - **Registry Resolution Bugfixes (PR Review rui.hu)**: Fixed `TemplateRegistry._instantiate_from_registry_ref` hardcoding `package_type="template"` instead of threading the template type, which broke all non-template registry resolutions. `_try_parse_registry_ref` now filters to `ReferenceType.REGISTRY` only, preserving the "local templates unaffected" acceptance criterion. `_original_reference` in resolved results now stores the verbatim original reference string, not the cache-key with type suffix. `application.py` template-type mapping extended to cover all 8 types (previously only AGENT/GRAPH/STREAM — the other 5 silently misclassified as STREAM). Duplicated `_instantiate_from_registry_ref`/`_try_parse_registry_ref` logic extracted into shared `_resolve_registry_ref` helper in `base.py`. `RegistryClient` now URL-encodes namespace/name path components to prevent injection. HTTPS enforcement with `allow_insecure` flag per spec §12.2. `ReferenceResolver.close_all()` closes clients under the async lock to prevent concurrent client leak. Async lock lazily initialised to avoid deprecation warning. Added `plural_name` property to `TemplateType`. - **Registry Error Hierarchy Fixes (PR Review rui.hu Round 5)**: Fixed `details` dropping in non-5xx fallback path of `_request()` — `details` extracted from structured error bodies are now propagated to `RegistryError` for all unrecognised error types. Added `asyncio.Lock` to `RegistryClient._get_client()` to match the project-standardised check-and-create pattern used by `ReferenceResolver._get_client()`. Fixed a no-op BDD scenario ("404 without error type in body falls back to PackageNotFoundError") that had no `Then` step. Fixed HTTPS logging step definitions that constructed a new `RegistryClient` internally, defeating test isolation. Enhanced the "Client with API key" scenario to verify the `Authorization` header is present. Added end-to-end BDD scenario for `details` propagation from structured error bodies. Added `quote(package_id)` URL-encoding to `get_package()`. Replaced misleading `if exc.request is not None:` with `if getattr(exc, "_request", None) is not None:`. Simplified `RegistryNetworkError.__str__` using parts list + join. Replaced deprecated `asyncio.get_event_loop().run_until_complete()` with `asyncio.run()`. Tightened `_CLASS_MAP` type annotation to `dict[str, type[CleverAgentsException]]`. Added comment to `typing.cast(int, code)` explaining intentional non-int test. diff --git a/features/credential_injection.feature b/features/credential_injection.feature index fa49b46..3d75471 100644 --- a/features/credential_injection.feature +++ b/features/credential_injection.feature @@ -540,24 +540,6 @@ Feature: Per-Request Credential Injection and Extended Provider Routing When I access the chat_model property (expecting error) Then a credential ConfigurationError should contain "invalid bracket characters" - # ------------------------------------------------------------------ - # m5: cleanup() exception-handling branches - # ------------------------------------------------------------------ - - Scenario: cleanup logs warning when root_async_client.close raises - Given I construct an LLMAgent for openai with credentials dict containing only api_key - And I inject a mock chat model whose root_async_client.close raises RuntimeError - When I call cleanup on the agent - Then a warning log should be emitted about closing async client error - And _chat_model should still be set to None - - Scenario: cleanup logs warning when root_client.close raises - Given I construct an LLMAgent for openai with credentials dict containing only api_key - And I inject a mock chat model whose root_client.close raises RuntimeError - When I call cleanup on the agent - Then a warning log should be emitted about closing sync client error - And _chat_model should still be set to None - # ------------------------------------------------------------------ # m6: _build_native when resolve_class_ref returns None # ------------------------------------------------------------------ @@ -724,24 +706,6 @@ Feature: Per-Request Credential Injection and Extended Provider Routing When I access the chat_model property (expecting error) Then a credential ConfigurationError should contain "invalid control characters" - # ------------------------------------------------------------------ - # m4: Anthropic/Google cleanup error paths - # ------------------------------------------------------------------ - - Scenario: cleanup logs warning when _async_client.close raises - Given I construct an LLMAgent for openai with credentials dict containing only api_key - And I inject a mock chat model whose _async_client.close raises RuntimeError - When I call cleanup on the agent - Then a warning log should be emitted about closing _async_client error - And _chat_model should still be set to None - - Scenario: cleanup logs warning when _client.close raises - Given I construct an LLMAgent for openai with credentials dict containing only api_key - And I inject a mock chat model whose _client.close raises RuntimeError - When I call cleanup on the agent - Then a warning log should be emitted about closing _client error - And _chat_model should still be set to None - # ------------------------------------------------------------------ # m5: Non-string api_key in standalone mode # ------------------------------------------------------------------ diff --git a/features/llm_cleanup_shared_client.feature b/features/llm_cleanup_shared_client.feature new file mode 100644 index 0000000..26bfbff --- /dev/null +++ b/features/llm_cleanup_shared_client.feature @@ -0,0 +1,66 @@ +Feature: LLMAgent.cleanup() must not close shared cached httpx clients + As a developer running multiple LLM requests in the same process + I want LLMAgent.cleanup() to release only the agent's own reference to the chat model + So that shared cached httpx clients remain open for subsequent LLMAgent instances + + # Regression tests for issue #57: + # langchain-anthropic and langchain-openai cache their default httpx clients + # via module-level lru_cache functions. Calling close() on those clients + # through cleanup() poisoned the cache and broke all subsequent LLM requests + # in the same process. + # + # Test strategy: + # - Async paths (_async_client, root_async_client): inject a Mock SDK client + # whose close() is an AsyncMock, then assert close() was never called. + # - Sync paths (_client, root_client): inject a real httpx.Client and assert + # it is not closed (httpx.Client.close() is a real sync method, so this + # directly proves the fix). + + Background: + Given an LLM agent test environment is ready (scc) + + @tdd_issue @tdd_issue_57 + Scenario: cleanup does not call close() on the async SDK client at _async_client (Anthropic path) + Given an LLMAgent for "anthropic" with a mock model whose _async_client has a tracked async close (scc) + When I call cleanup on the agent (scc) + Then close() should NOT have been called on the _async_client mock (scc) + And the agent's _chat_model should be None (scc) + + @tdd_issue @tdd_issue_57 + Scenario: cleanup does not close the shared sync httpx.Client at _client (Anthropic path) + Given an LLMAgent for "anthropic" with a mock model whose _client is a real httpx.Client (scc) + When I call cleanup on the agent (scc) + Then the real httpx.Client should NOT be closed (scc) + And the agent's _chat_model should be None (scc) + + @tdd_issue @tdd_issue_57 + Scenario: cleanup does not call close() on the async SDK client at root_async_client (OpenAI path) + Given an LLMAgent for "openai" with a mock model whose root_async_client has a tracked async close (scc) + When I call cleanup on the agent (scc) + Then close() should NOT have been called on the root_async_client mock (scc) + And the agent's _chat_model should be None (scc) + + @tdd_issue @tdd_issue_57 + Scenario: cleanup does not close the shared sync httpx.Client at root_client (OpenAI path) + Given an LLMAgent for "openai" with a mock model whose root_client is a real httpx.Client (scc) + When I call cleanup on the agent (scc) + Then the real httpx.Client should NOT be closed (scc) + And the agent's _chat_model should be None (scc) + + @tdd_issue @tdd_issue_57 + Scenario: second LLMAgent reusing a shared sync httpx.Client works after the first is cleaned up (Anthropic) + Given a shared real httpx.Client that simulates the lru_cache client (scc) + And a first LLMAgent for "anthropic" whose mock model holds that shared client as _client (scc) + When I call cleanup on the first agent (scc) + And I create a second LLMAgent for "anthropic" whose mock model holds the same shared client as _client (scc) + Then invoking the second agent's mock model should succeed (scc) + And the shared real httpx.Client should still NOT be closed after both agents run (scc) + + @tdd_issue @tdd_issue_57 + Scenario: second LLMAgent reusing a shared sync httpx.Client works after the first is cleaned up (OpenAI) + Given a shared real httpx.Client that simulates the lru_cache client (scc) + And a first LLMAgent for "openai" whose mock model holds that shared client as root_client (scc) + When I call cleanup on the first agent (scc) + And I create a second LLMAgent for "openai" whose mock model holds the same shared client as root_client (scc) + Then invoking the second agent's mock model should succeed (scc) + And the shared real httpx.Client should still NOT be closed after both agents run (scc) diff --git a/features/llm_missing_coverage.feature b/features/llm_missing_coverage.feature index a284c2b..efd2811 100644 --- a/features/llm_missing_coverage.feature +++ b/features/llm_missing_coverage.feature @@ -62,25 +62,24 @@ Feature: LLM Agent Temperature Override, Cleanup, and Context History When I process a message with multi-role conversation_history in context (llm_gaps) Then the messages sent to chat model should include user and assistant messages in correct order (llm_gaps) - # ---- cleanup() Method (lines 392-403) ---- + # ---- cleanup() Method ---- + # cleanup() only releases the agent's own _chat_model reference; it does NOT + # close any provider SDK clients (those are managed by shared lru_cache + # instances — see issue #57 for details). - Scenario: Cleanup closes root_async_client + Scenario: Cleanup sets _chat_model to None and does not close root_async_client Given I setup an LLM agent with a mock root_async_client (llm_gaps) When I await the cleanup method (llm_gaps) - Then the mock root_async_client close should have been called once (llm_gaps) + Then the mock root_async_client close should NOT have been called (llm_gaps) + And the chat model should be None after cleanup (llm_gaps) - Scenario: Cleanup closes both async and sync clients + Scenario: Cleanup sets _chat_model to None and does not close root_client Given I setup an LLM agent with both mock root_async_client and mock root_client (llm_gaps) When I await the cleanup method (llm_gaps) - Then both the mock async and mock sync client close should have been called (llm_gaps) + Then neither mock client close should have been called (llm_gaps) + And the chat model should be None after cleanup (llm_gaps) - Scenario: Cleanup handles exceptions from async client close gracefully - Given I setup an LLM agent with a failing mock root_async_client and a valid mock root_client (llm_gaps) - When I await the cleanup method (llm_gaps) - Then the cleanup should not propagate an exception (llm_gaps) - And the mock root_client close should still have been attempted (llm_gaps) - - Scenario: Cleanup does nothing when chat_model has no http clients + Scenario: Cleanup is a no-op when chat_model has no http clients Given I setup an LLM agent with a chat model that lacks root_async_client and root_client (llm_gaps) When I await the cleanup method (llm_gaps) Then the cleanup should complete without raising errors (llm_gaps) \ No newline at end of file diff --git a/features/steps/credential_cleanup_steps.py b/features/steps/credential_cleanup_steps.py index a50d0f6..413db9c 100644 --- a/features/steps/credential_cleanup_steps.py +++ b/features/steps/credential_cleanup_steps.py @@ -1,159 +1,21 @@ -"""Step definitions for cleanup() exception-handling and resolve_class_ref error paths. +"""Step definitions for resolve_class_ref error path. Extracted from ``credential_injection_steps.py`` to keep that file under 500 lines (CONTRIBUTING.md §General Principles). -Covers cleanup exception-handling branches (async/sync close errors) -and resolve_class_ref returning None for unavailable packages. +Note: the cleanup() exception-handling branch tests that previously lived +here were removed when the fix for issue #57 changed cleanup() to no longer +close provider SDK clients. The regression tests for the new behaviour live +in ``features/llm_cleanup_shared_client.feature`` and +``features/steps/llm_cleanup_shared_client_steps.py``. """ from __future__ import annotations -import logging from typing import Any -from unittest.mock import AsyncMock, Mock, patch - -from behave import given, then - -from cleveractors.agents.llm import logger as llm_logger - -# --------------------------------------------------------------------------- -# Log-capture helper -# --------------------------------------------------------------------------- - - -class _CaptureHandler(logging.Handler): - """Captures log records for later assertion.""" - - def __init__(self) -> None: - super().__init__() - self.records: list[logging.LogRecord] = [] - - def emit(self, record: logging.LogRecord) -> None: - self.records.append(record) - - -def _install_log_capture(context: Any) -> None: - """Install a WARNING-level capture handler on the LLM agent logger.""" - if getattr(context, "_cleanup_log_handler", None) is not None: - return # already installed - - handler = _CaptureHandler() - handler.setLevel(logging.WARNING) - context._cleanup_log_handler = handler - context._cleanup_log_original_level = llm_logger.level - llm_logger.setLevel(logging.WARNING) - llm_logger.addHandler(handler) - - -def _remove_log_capture(context: Any) -> None: - """Remove the capture handler and restore the logger.""" - handler = getattr(context, "_cleanup_log_handler", None) - if handler is not None: - llm_logger.removeHandler(handler) - context._cleanup_log_handler = None - if hasattr(context, "_cleanup_log_original_level"): - llm_logger.setLevel(context._cleanup_log_original_level) - context._cleanup_log_original_level = None - - -# --------------------------------------------------------------------------- -# m5: cleanup() exception-handling branches -# --------------------------------------------------------------------------- - - -@given("I inject a mock chat model whose root_async_client.close raises RuntimeError") -def step_inject_mock_with_failing_async_close(context: Any) -> None: - """Inject a mock chat model where root_async_client.close() raises RuntimeError.""" - _install_log_capture(context) - - model = Mock() - model.root_async_client = Mock() - model.root_async_client.close = AsyncMock(side_effect=RuntimeError("Boom async")) - model.root_client = Mock() - model.root_client.close = Mock() - model.temperature = 0.7 - context.agent.chat_model = model - - -@given("I inject a mock chat model whose root_client.close raises RuntimeError") -def step_inject_mock_with_failing_sync_close(context: Any) -> None: - """Inject a mock chat model where root_client.close() raises RuntimeError.""" - _install_log_capture(context) - - model = Mock() - model.root_async_client = Mock() - model.root_async_client.close = AsyncMock() - model.root_client = Mock() - model.root_client.close = Mock(side_effect=RuntimeError("Boom sync")) - model.temperature = 0.7 - context.agent.chat_model = model - - -@then("a warning log should be emitted about closing async client error") -def step_warning_async_close_error(context: Any) -> None: - """Verify a warning was logged about the async client close error.""" - try: - handler = getattr(context, "_cleanup_log_handler", None) - assert handler is not None, ( - "Expected log capture handler to be installed before cleanup" - ) - - assert context.cleanup_error is None, ( - f"Expected cleanup to succeed despite close() error, " - f"but got: {context.cleanup_error!r}" - ) - - async_close_warnings = [ - r - for r in handler.records - if r.levelno == logging.WARNING - and "Error closing root_async_client" in r.getMessage() - ] - assert len(async_close_warnings) > 0, ( - "Expected a WARNING log record about 'Error closing async client', " - f"but got records: {[r.getMessage() for r in handler.records]}" - ) - finally: - _remove_log_capture(context) - - -@then("a warning log should be emitted about closing sync client error") -def step_warning_sync_close_error(context: Any) -> None: - """Verify a warning was logged about the sync client close error.""" - try: - handler = getattr(context, "_cleanup_log_handler", None) - assert handler is not None, ( - "Expected log capture handler to be installed before cleanup" - ) - - assert context.cleanup_error is None, ( - f"Expected cleanup to succeed despite sync close() error, " - f"but got: {context.cleanup_error!r}" - ) - - sync_close_warnings = [ - r - for r in handler.records - if r.levelno == logging.WARNING - and "Error closing root_client" in r.getMessage() - ] - assert len(sync_close_warnings) > 0, ( - "Expected a WARNING log record about 'Error closing sync client', " - f"but got records: {[r.getMessage() for r in handler.records]}" - ) - finally: - _remove_log_capture(context) - - -@then("_chat_model should still be set to None") -def step_chat_model_still_none(context: Any) -> None: - """Verify _chat_model is None after cleanup despite close() error.""" - assert context.agent._chat_model is None, ( - f"Expected _chat_model to be None after cleanup, " - f"but got: {context.agent._chat_model!r}" - ) +from unittest.mock import patch +from behave import given # --------------------------------------------------------------------------- # m6: _build_native when resolve_class_ref returns None @@ -171,98 +33,3 @@ def step_patch_resolve_class_ref_return_none(context: Any) -> None: if not hasattr(context, "_active_patches"): context._active_patches = [] context._active_patches.append(patcher) - - -# --------------------------------------------------------------------------- -# m4: Anthropic/Google cleanup error paths -# --------------------------------------------------------------------------- - - -@given("I inject a mock chat model whose _async_client.close raises RuntimeError") -def step_inject_mock_with_failing_anthropic_async_close(context: Any) -> None: - """Inject a mock chat model where _async_client.close() raises RuntimeError. - - Exercises the Anthropic cleanup path: the ``_async_client`` attribute - with ``is_async=True`` in ``_KNOWN_CLIENT_ATTRS``. - """ - _install_log_capture(context) - - model = Mock() - model._async_client = Mock() - model._async_client.close = AsyncMock( - side_effect=RuntimeError("Boom anthropic async") - ) - model.temperature = 0.7 - context.agent.chat_model = model - - -@given("I inject a mock chat model whose _client.close raises RuntimeError") -def step_inject_mock_with_failing_google_sync_close(context: Any) -> None: - """Inject a mock chat model where _client.close() raises RuntimeError. - - Exercises the Google/ChatAnthropic sync cleanup path: the ``_client`` - attribute with ``is_async=False`` in ``_KNOWN_CLIENT_ATTRS``. - """ - _install_log_capture(context) - - model = Mock() - model._client = Mock() - model._client.close = Mock(side_effect=RuntimeError("Boom google sync")) - model.temperature = 0.7 - context.agent.chat_model = model - - -@then("a warning log should be emitted about closing _async_client error") -def step_warning_async_client_error(context: Any) -> None: - """Verify a warning was logged about the _async_client close error.""" - try: - handler = getattr(context, "_cleanup_log_handler", None) - assert handler is not None, ( - "Expected log capture handler to be installed before cleanup" - ) - - assert context.cleanup_error is None, ( - f"Expected cleanup to succeed despite close() error, " - f"but got: {context.cleanup_error!r}" - ) - - async_close_warnings = [ - r - for r in handler.records - if r.levelno == logging.WARNING - and "Error closing _async_client" in r.getMessage() - ] - assert len(async_close_warnings) > 0, ( - "Expected a WARNING log record about 'Error closing _async_client', " - f"but got records: {[r.getMessage() for r in handler.records]}" - ) - finally: - _remove_log_capture(context) - - -@then("a warning log should be emitted about closing _client error") -def step_warning_client_error(context: Any) -> None: - """Verify a warning was logged about the _client close error.""" - try: - handler = getattr(context, "_cleanup_log_handler", None) - assert handler is not None, ( - "Expected log capture handler to be installed before cleanup" - ) - - assert context.cleanup_error is None, ( - f"Expected cleanup to succeed despite close() error, " - f"but got: {context.cleanup_error!r}" - ) - - sync_close_warnings = [ - r - for r in handler.records - if r.levelno == logging.WARNING - and "Error closing _client" in r.getMessage() - ] - assert len(sync_close_warnings) > 0, ( - "Expected a WARNING log record about 'Error closing _client', " - f"but got records: {[r.getMessage() for r in handler.records]}" - ) - finally: - _remove_log_capture(context) diff --git a/features/steps/llm_cleanup_shared_client_steps.py b/features/steps/llm_cleanup_shared_client_steps.py new file mode 100644 index 0000000..f20e684 --- /dev/null +++ b/features/steps/llm_cleanup_shared_client_steps.py @@ -0,0 +1,317 @@ +"""Step definitions for regression tests of issue #57. + +LLMAgent.cleanup() must not close shared cached httpx clients used by +langchain-anthropic and langchain-openai (lru_cache-managed clients). + +Test strategy +------------- +- Async client paths (_async_client, root_async_client): + Inject a Mock SDK client (simulating e.g. anthropic.AsyncAnthropic) whose + ``close`` attribute is an AsyncMock. After cleanup(), assert that + ``close()`` was never called. (httpx.AsyncClient has no ``close()`` method + of its own; the real SDK clients do, which is why the bug only manifests at + production time — the mock captures the call.) +- Sync client paths (_client, root_client): + Inject a real ``httpx.Client`` instance. After cleanup(), assert + ``client.is_closed`` is still ``False``. (httpx.Client.close() is a real + sync method that sets is_closed, so this directly proves the bug/fix.) +""" + +from __future__ import annotations + +import asyncio +from typing import Any +from unittest.mock import AsyncMock, Mock + +import httpx +from behave import given, then, when +from langchain_core.messages import AIMessage + +from cleveractors.agents.llm import LLMAgent +from cleveractors.templates.renderer import TemplateRenderer + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + + +def _make_agent(provider: str) -> LLMAgent: + """Return a minimal LLMAgent for the given provider.""" + cfg = { + "name": f"scc_{provider}_agent", + "provider": provider, + "api_key": "test-key", + } + renderer = Mock(spec=TemplateRenderer) + renderer.render_string.return_value = "system prompt" + return LLMAgent(cfg["name"], cfg, renderer) + + +def _make_mock_model(response: str = "ok") -> Mock: + """Return a minimal mock chat model that can be invoked.""" + model = Mock() + model.temperature = 0.7 + mock_response = AIMessage(content=response) + model.ainvoke = AsyncMock(return_value=mock_response) + return model + + +def _make_sdk_client_with_async_close() -> Mock: + """Return a mock SDK client (e.g. anthropic.AsyncAnthropic) with async close().""" + sdk_client = Mock() + sdk_client.close = AsyncMock() + return sdk_client + + +# --------------------------------------------------------------------------- +# Background +# --------------------------------------------------------------------------- + + +@given("an LLM agent test environment is ready (scc)") +def step_scc_env_ready(context: Any) -> None: + """Ensure test context fields are initialised.""" + context.scc_cleanup_error = None + + +# --------------------------------------------------------------------------- +# Given — async-path agents (mock SDK client with tracked close()) +# --------------------------------------------------------------------------- + + +@given( + 'an LLMAgent for "anthropic" with a mock model whose _async_client ' + "has a tracked async close (scc)" +) +def step_scc_anthropic_mock_async_close(context: Any) -> None: + """Anthropic agent: _async_client is a mock with an async close() for tracking.""" + sdk_client = _make_sdk_client_with_async_close() + context.scc_tracked_async_client = sdk_client + agent = _make_agent("anthropic") + model = _make_mock_model() + model._async_client = sdk_client + agent.chat_model = model + context.scc_agent = agent + context.scc_tracked_attr = "_async_client" + + +@given( + 'an LLMAgent for "openai" with a mock model whose root_async_client ' + "has a tracked async close (scc)" +) +def step_scc_openai_mock_async_close(context: Any) -> None: + """OpenAI agent: root_async_client is a mock with an async close() for tracking.""" + sdk_client = _make_sdk_client_with_async_close() + context.scc_tracked_async_client = sdk_client + agent = _make_agent("openai") + model = _make_mock_model() + model.root_async_client = sdk_client + agent.chat_model = model + context.scc_agent = agent + context.scc_tracked_attr = "root_async_client" + + +# --------------------------------------------------------------------------- +# Given — sync-path agents (real httpx.Client) +# --------------------------------------------------------------------------- + + +@given( + 'an LLMAgent for "anthropic" with a mock model whose _client ' + "is a real httpx.Client (scc)" +) +def step_scc_anthropic_real_sync_client(context: Any) -> None: + """Anthropic agent: _client is a real httpx.Client so is_closed can be verified.""" + context.scc_shared_sync_client = httpx.Client() + agent = _make_agent("anthropic") + model = _make_mock_model() + model._client = context.scc_shared_sync_client + agent.chat_model = model + context.scc_agent = agent + + +@given( + 'an LLMAgent for "openai" with a mock model whose root_client ' + "is a real httpx.Client (scc)" +) +def step_scc_openai_real_sync_client(context: Any) -> None: + """OpenAI agent: root_client is a real httpx.Client so is_closed can be verified.""" + context.scc_shared_sync_client = httpx.Client() + agent = _make_agent("openai") + model = _make_mock_model() + model.root_client = context.scc_shared_sync_client + agent.chat_model = model + context.scc_agent = agent + + +# --------------------------------------------------------------------------- +# Given — two-agent shared-client scenarios (real httpx.Client) +# --------------------------------------------------------------------------- + + +@given("a shared real httpx.Client that simulates the lru_cache client (scc)") +def step_scc_shared_real_client(context: Any) -> None: + """Create the shared sync client both agents will reuse.""" + context.scc_shared_sync_client = httpx.Client() + + +@given( + 'a first LLMAgent for "anthropic" whose mock model holds that shared client ' + "as _client (scc)" +) +def step_scc_first_agent_anthropic_sync(context: Any) -> None: + """First Anthropic agent uses the shared sync client at _client.""" + agent = _make_agent("anthropic") + model = _make_mock_model("first response") + model._client = context.scc_shared_sync_client + agent.chat_model = model + context.scc_first_agent = agent + + +@given( + 'a first LLMAgent for "openai" whose mock model holds that shared client ' + "as root_client (scc)" +) +def step_scc_first_agent_openai_sync(context: Any) -> None: + """First OpenAI agent uses the shared sync client at root_client.""" + agent = _make_agent("openai") + model = _make_mock_model("first response") + model.root_client = context.scc_shared_sync_client + agent.chat_model = model + context.scc_first_agent = agent + + +# --------------------------------------------------------------------------- +# When — single-agent cleanup +# --------------------------------------------------------------------------- + + +@when("I call cleanup on the agent (scc)") +def step_scc_call_cleanup(context: Any) -> None: + """Call cleanup() on the agent under test.""" + context.scc_cleanup_error = None + try: + asyncio.run(context.scc_agent.cleanup()) + except Exception as exc: + context.scc_cleanup_error = exc + + +# --------------------------------------------------------------------------- +# When — two-agent scenarios +# --------------------------------------------------------------------------- + + +@when("I call cleanup on the first agent (scc)") +def step_scc_cleanup_first(context: Any) -> None: + """Call cleanup() on the first agent.""" + asyncio.run(context.scc_first_agent.cleanup()) + + +@when( + 'I create a second LLMAgent for "anthropic" whose mock model holds ' + "the same shared client as _client (scc)" +) +def step_scc_second_agent_anthropic_sync(context: Any) -> None: + """Second Anthropic agent reuses the same shared sync client.""" + agent = _make_agent("anthropic") + model = _make_mock_model("second response") + model._client = context.scc_shared_sync_client + agent.chat_model = model + context.scc_second_agent = agent + + +@when( + 'I create a second LLMAgent for "openai" whose mock model holds ' + "the same shared client as root_client (scc)" +) +def step_scc_second_agent_openai_sync(context: Any) -> None: + """Second OpenAI agent reuses the same shared sync client.""" + agent = _make_agent("openai") + model = _make_mock_model("second response") + model.root_client = context.scc_shared_sync_client + agent.chat_model = model + context.scc_second_agent = agent + + +# --------------------------------------------------------------------------- +# Then — assertions: async mock SDK client +# --------------------------------------------------------------------------- + + +@then("close() should NOT have been called on the _async_client mock (scc)") +def step_scc_async_client_close_not_called(context: Any) -> None: + """Assert cleanup() did not call close() on the _async_client mock.""" + assert context.scc_cleanup_error is None, ( + f"cleanup() raised unexpectedly: {context.scc_cleanup_error!r}" + ) + sdk_client = context.scc_tracked_async_client + sdk_client.close.assert_not_called() # type: ignore[union-attr] + + +@then("close() should NOT have been called on the root_async_client mock (scc)") +def step_scc_root_async_client_close_not_called(context: Any) -> None: + """Assert cleanup() did not call close() on the root_async_client mock.""" + assert context.scc_cleanup_error is None, ( + f"cleanup() raised unexpectedly: {context.scc_cleanup_error!r}" + ) + sdk_client = context.scc_tracked_async_client + sdk_client.close.assert_not_called() # type: ignore[union-attr] + + +# --------------------------------------------------------------------------- +# Then — assertions: real httpx.Client +# --------------------------------------------------------------------------- + + +@then("the real httpx.Client should NOT be closed (scc)") +def step_scc_sync_client_open(context: Any) -> None: + """Assert the real httpx.Client is still open after cleanup.""" + assert context.scc_cleanup_error is None, ( + f"cleanup() raised unexpectedly: {context.scc_cleanup_error!r}" + ) + client: httpx.Client = context.scc_shared_sync_client + assert not client.is_closed, ( + "cleanup() closed the shared httpx.Client — " + "this breaks subsequent LLM requests that reuse the same cached client" + ) + + +# --------------------------------------------------------------------------- +# Then — assertions: _chat_model released +# --------------------------------------------------------------------------- + + +@then("the agent's _chat_model should be None (scc)") +def step_scc_chat_model_none(context: Any) -> None: + """Assert the agent released its own reference to the chat model.""" + assert context.scc_agent._chat_model is None, ( + f"Expected _chat_model to be None after cleanup(), " + f"got: {context.scc_agent._chat_model!r}" + ) + + +# --------------------------------------------------------------------------- +# Then — assertions: two-agent scenarios +# --------------------------------------------------------------------------- + + +@then("invoking the second agent's mock model should succeed (scc)") +def step_scc_second_agent_invokes(context: Any) -> None: + """The second agent must be able to process a message successfully.""" + result = asyncio.run(context.scc_second_agent.process_message("hello")) + assert result is not None and len(result) > 0, ( + "Second agent's process_message() returned empty result — " + "the shared client may have been closed by the first agent's cleanup()" + ) + + +@then( + "the shared real httpx.Client should still NOT be closed after both agents run (scc)" +) +def step_scc_sync_client_still_open(context: Any) -> None: + """Shared sync client must remain open after both agents have run.""" + client: httpx.Client = context.scc_shared_sync_client + assert not client.is_closed, ( + "The shared httpx.Client was closed after the first agent's cleanup() — " + "shared lru_cache clients must not be closed" + ) diff --git a/features/steps/llm_missing_coverage_steps.py b/features/steps/llm_missing_coverage_steps.py index 9ef99c4..62c464c 100644 --- a/features/steps/llm_missing_coverage_steps.py +++ b/features/steps/llm_missing_coverage_steps.py @@ -428,30 +428,32 @@ async def step_ml_await_cleanup(context: Any) -> None: context.ml_cleanup_error = e -@then("the mock root_async_client close should have been called once (llm_gaps)") -def step_ml_async_close_called_once(context: Any) -> None: - context.ml_mock_async_client.close.assert_called_once() - - -@then( - "both the mock async and mock sync client close should have been called (llm_gaps)" -) -def step_ml_both_clients_called(context: Any) -> None: - context.ml_mock_async_client.close.assert_called_once() - context.ml_mock_sync_client.close.assert_called_once() - - -@then("the cleanup should not propagate an exception (llm_gaps)") -def step_ml_cleanup_no_exception(context: Any) -> None: +@then("the mock root_async_client close should NOT have been called (llm_gaps)") +def step_ml_async_close_not_called(context: Any) -> None: + """cleanup() must not close provider SDK clients (shared lru_cache — issue #57).""" assert context.ml_cleanup_error is None, ( - f"Cleanup should not propagate exception, got: {context.ml_cleanup_error}" + f"cleanup() should not raise, got: {context.ml_cleanup_error}" ) - context.ml_mock_sync_client.close.assert_called_once() + context.ml_mock_async_client.close.assert_not_called() -@then("the mock root_client close should still have been attempted (llm_gaps)") -def step_ml_sync_close_attempted(context: Any) -> None: - context.ml_mock_sync_client.close.assert_called_once() +@then("neither mock client close should have been called (llm_gaps)") +def step_ml_neither_client_called(context: Any) -> None: + """cleanup() must not close any client (shared lru_cache — issue #57).""" + assert context.ml_cleanup_error is None, ( + f"cleanup() should not raise, got: {context.ml_cleanup_error}" + ) + context.ml_mock_async_client.close.assert_not_called() + context.ml_mock_sync_client.close.assert_not_called() + + +@then("the chat model should be None after cleanup (llm_gaps)") +def step_ml_chat_model_none_after_cleanup(context: Any) -> None: + """cleanup() must release the agent's own _chat_model reference.""" + assert context.ml_agent._chat_model is None, ( + f"Expected _chat_model to be None after cleanup(), " + f"got: {context.ml_agent._chat_model!r}" + ) @then("the cleanup should complete without raising errors (llm_gaps)") diff --git a/src/cleveractors/agents/llm.py b/src/cleveractors/agents/llm.py index 6b81386..c36c543 100644 --- a/src/cleveractors/agents/llm.py +++ b/src/cleveractors/agents/llm.py @@ -27,7 +27,7 @@ import contextvars import logging import threading from collections.abc import AsyncGenerator -from typing import TYPE_CHECKING, Any, ClassVar, Literal +from typing import TYPE_CHECKING, Any, Literal if TYPE_CHECKING: from langchain_core.language_models.chat_models import BaseChatModel @@ -119,36 +119,6 @@ class LLMAgent(AgentWithMemory): or modifying ``config``. """ - # Known client attribute patterns by provider SDK: - # ChatOpenAI: ("root_async_client", True), ("root_client", False) - # ChatAnthropic: ("_async_client", True), ("_client", False) - # ChatGoogleGenerativeAI: ("_client", False) - # - # The is_async flag controls whether close() is awaited. - # - # Provider coverage notes: - # - ChatOpenAI (langchain-openai): exposes ``root_async_client`` (httpx - # AsyncClient) and ``root_client`` (httpx Client) as instance attributes. - # Both are reliably present after construction. - # - ChatAnthropic (langchain-anthropic): exposes ``_async_client`` and - # ``_client`` as instance attributes. - # - ChatGoogleGenerativeAI (langchain-google-genai): exposes ``_client`` - # as an instance attribute in tested versions. However, the Google SDK - # wraps gRPC channels rather than httpx clients, and the ``_client`` - # attribute may not be consistently present across all SDK versions or - # may not expose a ``close()`` method. The cleanup loop uses - # ``model.__dict__.get(attr_name)`` (instance-dict only, not class - # attrs) so a missing attribute is silently skipped — no resource leak - # occurs in that case because the gRPC channel is managed by the SDK. - # If a future SDK version does expose a closeable ``_client``, this - # entry will handle it automatically. - _KNOWN_CLIENT_ATTRS: ClassVar[list[tuple[str, bool]]] = [ - ("root_async_client", True), - ("root_client", False), - ("_async_client", True), - ("_client", False), - ] - def __init__( self, name: str, @@ -1109,12 +1079,22 @@ class LLMAgent(AgentWithMemory): """ Clean up resources used by the LLM agent. - This method closes HTTP clients used by LangChain chat models to prevent - resource leaks and unawaited coroutine warnings. If the chat model has - not been initialised yet (lazy init), this is a no-op. + Releases the agent's own reference to the chat model + (``self._chat_model = None``) so the model can be garbage-collected + when no other references exist. - The method is idempotent — a second call after a successful cleanup is - a no-op. + **Do not close the provider SDK clients.** Both ``langchain-anthropic`` + and ``langchain-openai`` cache their default httpx clients via + module-level ``lru_cache`` functions + (``langchain_anthropic._client_utils._get_default_async_httpx_client`` + and the OpenAI equivalent). Closing those clients would poison the + cache: every subsequent ``ChatAnthropic`` / ``ChatOpenAI`` instance in + the same process would receive the same closed httpx client and fail + with a connection error. + + If the chat model has not been initialised (lazy init path), this + method is a no-op. The method is idempotent — a second call after a + successful cleanup is also a no-op. .. warning:: **Concurrency constraint:** ``cleanup()`` and ``process_message()`` @@ -1131,43 +1111,9 @@ class LLMAgent(AgentWithMemory): because the model instance has been replaced. Avoid concurrent use of the same agent instance. """ - # Acquire the lock before reading _chat_model and iterating over its - # client attributes. Without the lock, two concurrent cleanup() calls - # could both read the same non-None model reference and attempt to - # close the same underlying HTTP clients twice (double-close). + # Acquire the lock so that two concurrent cleanup() calls both see + # a consistent _chat_model value and we don't null it out twice. with self._chat_model_lock: - model = self._chat_model - if model is None: + if self._chat_model is None: return - - # Use __dict__ to access instance-level attributes only — this - # avoids MagicMock auto-creating missing attributes during tests. - model_dict = getattr(model, "__dict__", {}) - clients_to_close: list[tuple[str, bool, object]] = [] - for attr_name, is_async in self._KNOWN_CLIENT_ATTRS: - client = model_dict.get(attr_name) - if client is not None: - clients_to_close.append((attr_name, is_async, client)) - - # Mark as cleaned up *before* awaiting async closes so that any - # concurrent caller that acquires the lock after us sees None and - # returns immediately (idempotency). - if self._chat_model is model: - self._chat_model = None - - # Close clients outside the lock — awaiting inside a lock would block - # other threads/coroutines for the duration of the I/O operation. - for attr_name, is_async, client in clients_to_close: - try: - if is_async: - await client.close() # type: ignore[union-attr] - else: - client.close() # type: ignore[union-attr] - logger.debug("Closed %s client for agent %s", attr_name, self.name) - except Exception as e: - logger.warning( - "Error closing %s for agent %s: %s", - attr_name, - self.name, - type(e).__name__, - ) + self._chat_model = None -- 2.52.0