fix: persist MCP connections across turns by eager-entering MCPToolset - #178
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces extensive documentation, design specifications, and benchmarks for the AgentPool framework, detailing the OpenSpec workflow, graph architecture, session orchestration, and lifecycle dimensions. It also adds several experimental commands and skills. The review feedback points out a few key issues: a potential FileNotFoundError in the benchmark script when writing to a gitignored directory, a hardcoded absolute path in the diagnostic script, a potential runtime error in the EventBus design due to frozen dataclasses, and a type mismatch in the YAML graph syntax design.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
#175) pydantic-ai's agent.iter() creates a new AsyncExitStack per turn that enters/exits MCPToolset. Without a persistent reference, _running_count goes 0→1→0 each turn, causing full connection teardown + cache clearing. Fix: In MCPManager.get_capabilities(), eagerly call __aenter__() on cache miss. This holds one reference open, so per-turn enter/exit goes 1→2→1 instead of 0→1→0. Connection persists until cleanup_session() or disconnect_all() brings the count to 0. Changes: - manager.py: _make_capability() is now async; on cache miss, calls await toolset.__aenter__() before caching. On failure, toolset is NOT cached (enables retry). disconnect_all() now also closes per-session toolset caches (edge-case fix). - tests/conftest.py: Autouse fixture patches MCPToolset.__aenter__/ __aexit__ to avoid real MCP connections in unit tests. Tests needing real connections opt out with @pytest.mark.real_mcp. - tests/mcp_server/test_mcp_persistence.py: 6 new tests verifying eager enter, persistence across calls, cleanup, failure handling, and session-scoped toolset cleanup. - pyproject.toml: Register real_mcp marker. Closes #175
cb6b70c to
abbfdd6
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request resolves issue #175 by ensuring MCP connection persistence across turns. This is achieved by eagerly entering MCPToolset instances via __aenter__ on cache miss within MCPManager.get_capabilities(), holding a persistent reference open for the session/pool lifetime. Additionally, disconnect_all() has been updated to clean up both global and per-session toolset caches to prevent connection leaks. A new test suite has been added to verify this lifecycle behavior, along with a pytest fixture to mock the toolset lifecycle and avoid real MCP connections in unit tests. Feedback from the reviewer suggests improving the robustness of the cleanup process in disconnect_all() by wrapping the __aexit__ calls for per-session toolsets in a try-except block with a timeout and logging unexpected exceptions, preventing potential hangs or aborts from blocking the entire shutdown process.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| for ctx in self._session_contexts.values(): | ||
| for toolset in ctx.toolset_cache.values(): | ||
| with contextlib.suppress(ValueError): | ||
| await toolset.__aexit__(None, None, None) | ||
| ctx.toolset_cache.clear() |
There was a problem hiding this comment.
In disconnect_all(), the per-session toolsets are cleaned up by calling await toolset.__aexit__(None, None, None) without a timeout and only suppressing ValueError. As noted in the PR description, __aexit__ calls can hang (e.g., when HTTP proxies do not promptly close TCP connections). If any single toolset hangs or raises an unexpected exception (such as TimeoutError or OSError), it will block or abort the entire disconnect_all() process, preventing other sessions and the global pool from being cleaned up. To prevent deadlocks and resource leaks, wrap each __aexit__ call in a try-except block with a timeout (using _MCP_CLEANUP_TIMEOUT) and catch/log all unexpected exceptions, matching the robust pattern already used in cleanup_session().
| for ctx in self._session_contexts.values(): | |
| for toolset in ctx.toolset_cache.values(): | |
| with contextlib.suppress(ValueError): | |
| await toolset.__aexit__(None, None, None) | |
| ctx.toolset_cache.clear() | |
| for ctx in self._session_contexts.values(): | |
| for toolset in ctx.toolset_cache.values(): | |
| with contextlib.suppress(ValueError): | |
| try: | |
| async with asyncio.timeout(_MCP_CLEANUP_TIMEOUT): | |
| await toolset.__aexit__(None, None, None) | |
| except TimeoutError: | |
| logger.warning("MCP session toolset cleanup timed out during disconnect_all") | |
| except Exception: | |
| logger.exception("Error cleaning up session toolset during disconnect_all") | |
| ctx.toolset_cache.clear() |
References
- When closing multiple streams or resources in a loop, use an explicit try-except block to catch expected library-specific exceptions silently, while logging any other unexpected exceptions to prevent them from masking failures or halting the cleanup of remaining resources.
…p_session() Address Gemini Code Assist review: __aexit__ calls in disconnect_all() had no timeout protection and only suppressed ValueError. If __aexit__ hung (HTTP proxy not closing TCP) or raised unexpected exceptions, it would block the entire shutdown process. Changes: - disconnect_all(): wrap each __aexit__ in asyncio.timeout(_MCP_CLEANUP_TIMEOUT) + catch TimeoutError and Exception separately, log and continue - cleanup_session(): add catch-all except Exception to toolset cleanup loop (previously only caught TimeoutError, other exceptions would abort remaining cleanup) - 3 new tests: failing toolset isolation, timeout doesn't block, cleanup_session exception isolation
Snapshot test uses a real MCP server (uv run server.py) and needs the real MCPToolset.__aenter__/__aexit__ to connect. The autouse fixture in conftest.py mocks these methods globally; @pytest.mark.real_mcp opts out of the mock for this test class.
Problem
pydantic-ai's
agent.iter()creates a newAsyncExitStackper turn that enters/exitsMCPToolset. Without a persistent reference,_running_countgoes0→1→0each turn, causing:streamable_http_client.__aexit__()can hang when HTTP proxy doesn't promptly close TCP → deadlock chain (StreamCompleteEvent never yielded → wait_for_completion hangs → per-session lock never released → all subsequent messages deadlock)Closes #175.
Solution
Pre-enter MCPToolset on cache miss. In
MCPManager.get_capabilities(), when a newMCPToolsetis created and cached, eagerly callawait toolset.__aenter__(). This holds one reference open, so pydantic-ai's per-turn enter/exit goes1→2→1instead of0→1→0.Connection persists until
cleanup_session()ordisconnect_all()brings the count to0and closes the connection.This leverages pydantic-ai's built-in
_running_countreference counting — the same mechanism used byasync with agent:(pydantic-ai's intended cross-run persistence pattern, see issue #2983). No wrappers, no new classes, no config changes.Changes
src/agentpool/mcp_server/manager.py_make_capability()→ async; eager__aenter__()on cache miss;disconnect_all()also closes session-scoped cachestests/conftest.pyMCPToolset.__aenter__/__aexit__for unit tests;@pytest.mark.real_mcpopts outtests/mcp_server/test_mcp_persistence.pypyproject.tomlreal_mcpmarkertests/mcp_server/test_e2e_*.py@pytest.mark.real_mcpto E2E tests needing real MCPToolsetKey design decisions
__aenter__fails, toolset is NOT cached — enables retry on next call.disconnect_all()now also iteratesself._session_contextsto close per-session toolset caches (edge case found during analysis).Ref counting trace
Testing
test_mcp_persistence.py:test_eager_enter_sets_running_count— verifies_running_count == 1afterget_capabilities()test_eager_enter_persists_across_calls— multiple calls don't re-entertest_disconnect_all_closes_eagerly_entered_toolset—disconnect_all()brings count to 0test_cleanup_session_closes_session_scoped_toolset— session cleanup closes session-scoped toolsetstest_eager_enter_failure_does_not_cache— failed enter doesn't leave stale cache entrytest_disconnect_all_also_cleans_session_scoped_toolsets—disconnect_all()cleans session caches tooMCPToolset.__aenter__/__aexit__globally to avoid real MCP connections in unit tests