-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Lift httpx2's default SSE event size cap on every client SSE reader #3338
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -130,7 +130,7 @@ | |
| # stderr (agronholm/anyio#816, fixed in 4.10). | ||
| "anyio>=4.10; python_version >= '3.14'", | ||
| "anyio>=4.9; python_version < '3.14'", | ||
| "httpx2>=2.5.0", | ||
| "httpx2>=2.10.0", | ||
|
Check failure on line 133 in pyproject.toml
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: This dependency-floor change must be accompanied by a regenerated Prompt for AI agents |
||
| "mcp-types=={{ version }}", | ||
| "pydantic>=2.12.0", | ||
| "starlette>=0.48.0; python_version >= '3.14'", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,13 +14,23 @@ | |
| import mcp_types as types | ||
| import pytest | ||
| from inline_snapshot import snapshot | ||
| from mcp_types import INVALID_REQUEST, CallToolResult, ErrorData, ListToolsResult, TextContent, Tool | ||
| from mcp_types import ( | ||
| INVALID_REQUEST, | ||
| CallToolResult, | ||
| ErrorData, | ||
| ListToolsResult, | ||
| LoggingMessageNotification, | ||
| LoggingMessageNotificationParams, | ||
| TextContent, | ||
| Tool, | ||
| ) | ||
| from starlette.types import Receive, Scope, Send | ||
|
|
||
| from mcp import MCPError | ||
| from mcp.client.client import Client | ||
| from mcp.client.streamable_http import streamable_http_client | ||
| from mcp.server import Server, ServerRequestContext | ||
| from mcp.server.mcpserver import Context, MCPServer | ||
| from tests.interaction._connect import BASE_URL, NO_DNS_REBINDING_PROTECTION, client_via_http, mounted_app | ||
| from tests.interaction._requirements import requirement | ||
| from tests.interaction.transports._bridge import StreamingASGITransport | ||
|
|
@@ -158,6 +168,67 @@ async def call(n: int) -> None: | |
| assert len(tools_call_posts) == 3 | ||
|
|
||
|
|
||
| # One byte past the 1 MiB that httpx2 >= 2.10 allows a single SSE event by default. | ||
| _OVERSIZED_TEXT = "x" * (1024 * 1024 + 1) | ||
|
|
||
|
|
||
| @requirement("client-transport:http:post-stream-large-event") | ||
| async def test_a_post_stream_delivers_a_tool_result_larger_than_one_mebibyte() -> None: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3: These two new tests contradict the file's stated scope. Its docstring says every test records the HTTP requests the SDK client issues and asserts on what the transport sends (headers, methods, ordering), never on what the protocol layer returns — but both new tests assert on Client-visible protocol results (result.content and the logging callback) and record no wire traffic. Either move them to a behavioural transport-test file or update the docstring so the file's stated purpose stays accurate. Prompt for AI agents |
||
| """A tool result bigger than httpx2's default per-event SSE cap arrives intact over the request's | ||
| POST stream. SDK-defined: MCP sets no message size limit, so the transport lifts the cap (#3332).""" | ||
| mcp = MCPServer("bulky") | ||
|
|
||
| @mcp.tool() | ||
| def bulk() -> str: | ||
| """Return more than one SSE event may carry by default.""" | ||
| return _OVERSIZED_TEXT | ||
|
|
||
| async with mounted_app(mcp) as (http, _), client_via_http(http) as client: | ||
| with anyio.fail_after(5): | ||
| result = await client.call_tool("bulk", {}) | ||
|
|
||
| assert result.content == [TextContent(text=_OVERSIZED_TEXT)] | ||
|
|
||
|
|
||
| @requirement("client-transport:http:get-stream-large-event") | ||
| async def test_the_standalone_get_stream_delivers_a_notification_larger_than_one_mebibyte() -> None: | ||
| """A server-initiated notification bigger than httpx2's default per-event SSE cap arrives intact | ||
| over the standalone GET stream, which the transport opens with the same lifted cap (#3332).""" | ||
| mcp = MCPServer("bulky") | ||
|
|
||
| @mcp.tool() | ||
| async def shout(ctx: Context) -> str: | ||
| """Emit one unrelated notification, which the server routes to the standalone stream.""" | ||
| params = LoggingMessageNotificationParams(level="info", data=_OVERSIZED_TEXT) | ||
| await ctx.session.send_notification(LoggingMessageNotification(params=params)) | ||
| return "sent" | ||
|
|
||
| get_stream_open = anyio.Event() | ||
|
|
||
| async def on_response(response: httpx2.Response) -> None: | ||
| if response.request.method == "GET": | ||
| get_stream_open.set() | ||
|
|
||
| received: list[object] = [] | ||
| delivered = anyio.Event() | ||
|
|
||
| async def collect(params: LoggingMessageNotificationParams) -> None: | ||
| received.append(params.data) | ||
| delivered.set() | ||
|
|
||
| async with ( | ||
| mounted_app(mcp, on_response=on_response) as (http, _), | ||
| client_via_http(http, logging_callback=collect) as client, | ||
| ): | ||
| with anyio.fail_after(5): | ||
| # The server drops standalone messages emitted before the GET stream is established. | ||
| await get_stream_open.wait() | ||
| await client.call_tool("shout", {}) | ||
| await delivered.wait() | ||
|
|
||
| assert received == [_OVERSIZED_TEXT] | ||
|
|
||
|
|
||
| @requirement("client-transport:http:sse-405-tolerated") | ||
| @requirement("client-transport:http:terminate-405-ok") | ||
| async def test_client_tolerates_405_on_get_and_delete() -> None: | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 httpx2 floor raised to >=2.10.0 but uv.lock is not regenerated and still pins httpx2 2.5.0, which has no max_event_size parameter
Extended reasoning...
The diff does not touch uv.lock (it still locks httpx2-2.5.0 with specifier '>=2.5.0', see uv.lock lines 676-688 and the '>=2.5.0' specifier entries). Any environment built from the lock (
uv run --frozen,uv sync --locked, CI,./scripts/test) installs httpx2 2.5.0, whoseclient.sse()andEventSource.__init__do not acceptmax_event_size. Every SSE connection the client opens — legacy sse_client (src/mcp/client/sse.py:58) and all four sites in src/mcp/client/streamable_http.py (lines 213, 256, 426, 504) — raisesTypeError: unexpected keyword argument 'max_event_size', so all streamable-HTTP/SSE client tests fail; additionally pyright fails on the unknown parameter and the pre-commit uv.lock consistency check rejects the mismatched pyproject/lock. The PR is broken as-is untiluv lock --upgrade-package httpx2is run and committed.Verification: normal — The diff raises the floor in pyproject.toml line 133 ("httpx2>=2.5.0" -> "httpx2>=2.10.0") but does not touch uv.lock: the changed-file list is 7 files with no uv.lock, and /home/claude/python-sdk/uv.lock still locks httpx2 at 2.5.0 (lines 676-677: name = "httpx2" / version = "2.5.0", wheel httpx2-2.5.0-py3-none-any.whl at line 688) with every specifier entry still ">=2.5.0" (li
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea this pushw as a mistake
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
didn't mean to make a pr just yet