modelcontextprotocol / modelcontextprotocol/python-sdk

ClientSessionGroup: a rejected connect_to_server leaves its transport running — the session is established before its components are validated

Đang mở
#3,490 2 bình luận 0 reaction 0 người được giao Xem trên GitHub

Chưa có ai nhận issue này.

v1 v2
Ngôn ngữ chính
Python
Star
24.3k
Fork
4k
Merge trung bình
1 ngày 1 giờ
Pull request đã merge (30 ngày)
31

Mô tả

Initial Checks
Release line

2.x (current stable), reproduced at 9972c21a on mcp 2.2.0.

Description

ClientSessionGroup.connect_to_server opens the transport first and validates the server's components second. When the duplicate-name check rejects the server, the transport it already opened is never closed, and the caller is never handed the session, so nothing else can close it either.

The order in session_group.py is:

  1. _establish_session launches the transport, runs initialize, stores the stack in self._session_exit_stacks[session], and enters it into self._exit_stack.
  2. _aggregate_components lists the components and hits raise MCPError(..., message=f"{matching_tools} already exist in group tools.").
  3. self._sessions[session] = component_names is on the line after that raise, so it never runs.

That leaves the connection live but unreachable:

  • group.sessions reads self._sessions, which the rejected server never reached, so it is not listed.
  • disconnect_from_server(session) would close it, but it needs the ClientSession object and connect_to_server raised instead of returning one.
  • self._exit_stack still holds the stack, so it is released only when the whole group tears down.

For stdio that is a live child process. For streamable HTTP it is an initialized session on a server that counts sessions against max_sessions. Both grow by one per rejection.

This is the case the docs treat as ordinary rather than exotic. docs/client/session-groups.md says two servers you don't control "will collide eventually", and its !!! check fence tells the reader to run exactly this and see the MCPError. The same page says the error is "raised before anything from the second server is registered" — true of the three component dicts, but not of the connection that was opened to read them.

It costs a long-lived host that connects servers dynamically, or retries a failed connect: one more process or session per attempt, none of them reclaimable until the group closes.

Expected: a connect_to_server that raises should close the transport it opened before the exception leaves, so "nothing from the second server is registered" covers its connection too. If holding it is deliberate, the session needs to be reachable — attached to the raised MCPError, or listed by group.sessions — so the caller can close it.

Two notes to save review time:

  • connect_with_session, the other caller of _aggregate_components, is unaffected: the caller owns that session and still holds it. That is also why existing coverage misses this. tests/docs_src/test_session_groups.py says so directly — "connect_to_server opens a real transport (a subprocess or a socket), so these tests drive the exact same aggregation path through connect_with_session with in-memory sessions instead." The leak exists only on the path the tests substitute away.
  • Not a duplicate of #3384. That one is a KeyError from del self._session_exit_stacks[session] in the empty-server branch. This is the duplicate-name branch, which raises MCPError by design — the defect is what stays running afterwards. The three PRs written for #3384 (#3386, #3419, #3428) all delete that one block and leave this path untouched, so fixing #3384 does not fix this.

#3228 looks like the same shape one layer down — a request the server refuses still leaves a registered session behind because the session is created before validation.

Example Code
import asyncio
import os
import subprocess
import sys
import tempfile
from pathlib import Path

from mcp import ClientSessionGroup, MCPError, StdioServerParameters

SERVER = """
from mcp.server import MCPServer

mcp = MCPServer({name!r})


@mcp.tool()
def search(query: str) -> str:
    \"\"\"Search.\"\"\"
    return f"{name} got {{query!r}}"


if __name__ == "__main__":
    mcp.run()
"""


def children() -> set[int]:
    out = subprocess.run(["pgrep", "-P", str(os.getpid())], capture_output=True, text=True).stdout.split()
    return {int(p) for p in out}


async def main() -> None:
    tmp = Path(tempfile.mkdtemp())
    for name in ("Library", "Web"):
        (tmp / f"{name}.py").write_text(SERVER.format(name=name))

    py = sys.executable
    library = StdioServerParameters(command=py, args=[str(tmp / "Library.py")])
    web = StdioServerParameters(command=py, args=[str(tmp / "Web.py")])

    async with ClientSessionGroup() as group:
        await group.connect_to_server(library)
        base = children()

        for attempt in range(1, 4):
            try:
                await group.connect_to_server(web)
            except MCPError as err:
                print(f"rejection #{attempt}: {err}")
            print(
                f"   live subprocesses left behind : {len(children() - base)}\n"
                f"   group.sessions                : {len(group.sessions)}\n"
                f"   group.tools                   : {sorted(group.tools)}"
            )


if __name__ == "__main__":
    asyncio.run(main())

Output on 2.2.0:

rejection #1: {'search'} already exist in group tools.
   live subprocesses left behind : 1
   group.sessions                : 1
   group.tools                   : ['search']
rejection #2: {'search'} already exist in group tools.
   live subprocesses left behind : 2
   group.sessions                : 1
   group.tools                   : ['search']
rejection #3: {'search'} already exist in group tools.
   live subprocesses left behind : 3
   group.sessions                : 1
   group.tools                   : ['search']

Same growth on StreamableHttpParameters against two HTTP servers, where what accumulates is an initialized session rather than a process — len(group._session_exit_stacks) - len(group._sessions) goes 1, 2, 3 while group.sessions stays at 1.

origin/v1.x has the same ordering — stack stored at session_group.py:348, entered into _exit_stack at :351, the duplicate check raises at :433, and self._sessions[session] is set at :438 — so 1.x looks affected too, though I only ran the reproduction on 2.2.0.

Python & MCP Python SDK
Python 3.10.20, mcp 2.2.0, starlette via httpx2, macOS 26.5.2 (arm64)

I used AI assistance to narrow this down and to build the reproduction; I ran it myself and can walk through the code path. If you'd like an outside PR for it, I'd like to take the fix.

Hướng dẫn đóng góp

Mở hướng dẫn đóng góp

Bắt đầu từ đâu

  1. Đọc hết issue, rồi đọc hướng dẫn đóng góp của dự án.
  2. Bình luận trên issue rằng bạn sẽ nhận — tránh hai người làm cùng một việc.
  3. Fork repository và làm thay đổi trên một nhánh.
  4. Mở pull request có tham chiếu số hiệu của issue.

Hướng nghiên cứu

Bắt đầu trong session_group.py với _establish_session, _aggregate_components và đường dẫn connect_to_server để theo dõi quyền sở hữu của exit stack khi các component trùng lặp gây ra MCPError. Xem xét tests/docs_src/test_session_groups.py và chạy bản tái hiện được cung cấp; bổ sung coverage cho một kết nối thực bị từ chối. Hoàn tất có nghĩa là transport đã mở được đóng khi xảy ra lỗi và không còn session nào không thể truy cập.

Do mô hình lập chỉ mục viết ra từ nội dung của issue.

Đánh giá

Công nghệ
python
Lĩnh vực
api, backend
Loại issue
Lỗi
Độ khó
3/5
Thời gian dự kiến
1-2 ngày
Mức độ hoạt động
Sôi nổi
Độ rõ ràng
Đặc tả rõ ràng
Mức phù hợp với người mới
76/100

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.