From 439ac10edbf6f6d408fc5adc1480688c43ad553f Mon Sep 17 00:00:00 2001 From: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com> Date: Thu, 17 Sep 2026 07:48:52 -0700 Subject: [PATCH] Close the transport a rejected connect_to_server opened Motivation: ClientSessionGroup.connect_to_server opens a transport (subprocess or HTTP session) and initializes a ClientSession before validating the server's prompts/resources/tools against names already registered in the group. When that validation rejects the server for a duplicate name, connect_with_session raises MCPError before the session is recorded in self._sessions, so the caller has no session object to close and the group's own tracking never lists it. The already-opened transport stays alive until the whole group tears down: a leaked child process for stdio, or an initialized session counted against a server's max_sessions for streamable HTTP. A long-lived host that retries a failed connect leaks one more resource per attempt. Approach: connect_to_server is the only caller of connect_with_session that owns the transport it opened (via _establish_session); the other caller, connect_with_session used directly, is handed an already-owned session by its caller and must not have it closed out from under them. So the fix is scoped to connect_to_server: wrap the call to connect_with_session in try/except, and on any exception pop the session's entry out of self._session_exit_stacks and close it before re-raising, mirroring the cleanup disconnect_from_server already does for a session the group decides to drop. Validation: Added test_client_session_group_connect_to_server_closes_transport_on_duplicate to tests/client/test_session_group.py, which mocks _establish_session to register a mock transport stack the way the real implementation does, triggers the existing duplicate-tool-name rejection path, and asserts the mock stack's aclose is awaited and the session is removed from _session_exit_stacks. Confirmed the test fails on the pre-fix code with "Expected aclose to have been awaited once. Awaited 0 times." and passes after the fix. Ran the full suite: `uv run --frozen pytest` and `./scripts/test` both pass (5969 passed, 10 skipped, 1 xfailed), coverage stays at 100.00% with no new pragma: no cover, and `ruff format --check`, `ruff check`, and `pyright` are clean on both changed files. Report: https://github.com/modelcontextprotocol/python-sdk/issues/3490 Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com> Assisted-by: claude-sonnet-5 (via Claude Code) --- src/mcp/client/session_group.py | 20 ++++++++++++-- tests/client/test_session_group.py | 43 ++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 2 deletions(-) diff --git a/src/mcp/client/session_group.py b/src/mcp/client/session_group.py index a544cecbe8..d06ef9cd39 100644 --- a/src/mcp/client/session_group.py +++ b/src/mcp/client/session_group.py @@ -296,9 +296,25 @@ async def connect_to_server( server_params: ServerParameters, session_params: ClientSessionParameters | None = None, ) -> mcp.ClientSession: - """Connects to a single MCP server.""" + """Connects to a single MCP server. + + Raises: + MCPError: If the server's prompts, resources, or tools collide + with names already in the group. The transport opened for + this connection is closed before the error propagates. + """ server_info, session = await self._establish_session(server_params, session_params or ClientSessionParameters()) - return await self.connect_with_session(server_info, session) + try: + return await self.connect_with_session(server_info, session) + except Exception: + # connect_with_session validates components against names already + # in the group and can reject the session. We own the transport + # established above, so close it here rather than leaking it + # until the whole group tears down. + session_stack = self._session_exit_stacks.pop(session, None) + if session_stack is not None: + await session_stack.aclose() + raise async def _establish_session( self, diff --git a/tests/client/test_session_group.py b/tests/client/test_session_group.py index b75d22b7a0..fb834793d9 100644 --- a/tests/client/test_session_group.py +++ b/tests/client/test_session_group.py @@ -286,6 +286,49 @@ async def test_client_session_group_connect_to_server_duplicate_tool_raises_erro assert group._tools[existing_tool_name] is not duplicate_tool # Ensure it's the original mock +@pytest.mark.anyio +async def test_client_session_group_connect_to_server_closes_transport_on_duplicate( + mock_exit_stack: contextlib.AsyncExitStack, +): + """A session rejected for a duplicate name must have its transport closed. + + connect_to_server is the only caller that owns the transport it opens (via + _establish_session); connect_with_session callers bring their own session and + must keep owning it even if the group rejects it. + """ + # --- Setup Pre-existing State --- + group = ClientSessionGroup(exit_stack=mock_exit_stack) + existing_tool_name = "shared_tool" + group._tools[existing_tool_name] = mock.Mock(spec=types.Tool) + group._tools[existing_tool_name].name = existing_tool_name + + # --- Mock New Connection Attempt --- + mock_server_info_new = mock.Mock(spec=types.Implementation) + mock_server_info_new.name = "ServerWithDuplicate" + mock_session_new = mock.AsyncMock(spec=mcp.ClientSession) + duplicate_tool = mock.Mock(spec=types.Tool) + duplicate_tool.name = existing_tool_name + mock_session_new.list_tools.return_value = mock.AsyncMock(tools=[duplicate_tool]) + mock_session_new.list_resources.return_value = mock.AsyncMock(resources=[]) + mock_session_new.list_prompts.return_value = mock.AsyncMock(prompts=[]) + + # _establish_session registers the new session's transport stack as a side + # effect of opening it, exactly like the real implementation does. + new_session_stack = mock.AsyncMock(spec=contextlib.AsyncExitStack) + + async def fake_establish_session(*args: object, **kwargs: object) -> tuple[types.Implementation, mcp.ClientSession]: + group._session_exit_stacks[mock_session_new] = new_session_stack + return mock_server_info_new, mock_session_new + + # --- Test Execution and Assertion --- + with pytest.raises(MCPError): + with mock.patch.object(group, "_establish_session", side_effect=fake_establish_session): + await group.connect_to_server(StdioServerParameters(command="test")) + + new_session_stack.aclose.assert_awaited_once() + assert mock_session_new not in group._session_exit_stacks + + @pytest.mark.anyio async def test_client_session_group_disconnect_non_existent_server(): """Test disconnecting a server that isn't connected."""