Expose an SSE event size limit in Streamable HTTP clients - #3600
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📚 Documentation preview
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 11 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Thanks for the follow-up commit: re-reading the current code, the GET stream now falls through to the existing reconnect loop on an oversized event, the reconnect path only fails the request when the reply is actually text/event-stream (both branches have tests), and the three error assertions now check only the SDK-authored prefix. Beyond the inline nit, I also checked that the httpx2>=2.10.0 floor matches the version already pinned in uv.lock, so the lowest-direct matrix resolves the same release the lock already tests.
Extended reasoning...
The PR adds a keyword-only max_sse_event_size to StreamableHTTPTransport, streamable_http_client, sse_within_origin and StreamableHttpParameters, threads it into every EventSource construction in src/mcp/client/streamable_http.py, adds SSEError handling on the POST, resumption and reconnection paths, raises the httpx2 floor to 2.10.0, and adds docs plus nine behaviour tests. It touches no auth, crypto or injection surface; the sensitive part is the client's stream-recovery logic, which the second commit reworked in response to the earlier review. A verified test-quality nit is posted inline and the change alters public API signatures and a dependency floor, both maintainer design decisions, so a human look is still warranted rather than approval.
Still open from earlier reviews (2):
- Unresolved: 2 minor or pre-existing.
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
I re-reviewed the latest push and found no new bugs; the points I raised earlier are addressed in the code (GET-stream errors now fall through to reconnection, the reconnect path checks the content type before failing the request, and the tests assert only the SDK-owned prefix). A human look is still worthwhile because this changes the transport's error and reconnection behaviour and raises a dependency floor.
What was reviewed:
- The four SSE reader sites in src/mcp/client/streamable_http.py (POST SSE, GET stream, resumption, reconnection) and the
is_sse_responsesniff at :570-571 — the recursive_handle_reconnectioncall stays bounded byattempt + 1and the nested call has its own handlers, so the newexcept httpx2.SSEErrorcannot loop. - The PR description still says the default is 16 MiB (and lists that as a behaviour change), but after "Preserve the 1 MiB SSE event default" the code, docs and
docs/migration.mdall say 1 MiB — worth updating the description before merge. - The
httpx2>=2.10.0floor:uv.lockalready resolved 2.10.0 before this PR, but I could not confirm from this checkout that 2.10.0 is the first release withEventSource(max_event_size=...), so a maintainer should check that against the dependency policy.
Extended reasoning...
The change adds a max_sse_event_size knob to StreamableHTTPTransport, streamable_http_client and StreamableHttpParameters, threads it through every EventSource construction, adds httpx2.SSEError handling on the POST-SSE, resumption and reconnection paths, raises the httpx2 floor to 2.10.0, and adds docs plus nine tests. It touches no auth, crypto or injection surface; the sensitive part is transport reliability (which requests get resolved with CONNECTION_CLOSED versus retried). All of my earlier inline findings are addressed by commits d3d0ff4 and 3e831d6, but there are unresolved cubic-dev-ai inline threads (docs/client/transports.md:59 and tests/client/test_streamable_http.py:119) with no independent resolution, the PR description is stale about the default, and the dependency-floor claim could not be verified here, so I deferred rather than approved.
Still open from earlier reviews (3):
- Unresolved: 3 minor or pre-existing.
There was a problem hiding this comment.
I re-reviewed the latest push and found no bugs; the points raised in my earlier reviews are now handled in the code, and a human look is still worthwhile because this changes transport reconnection behaviour and raises a dependency floor.
What was reviewed:
handle_get_streamnow lets an oversized event fall through to the reconnect loop, and_handle_reconnectiononly resolves the request on an SSEError when the reply was actuallytext/event-stream; both paths have tests withretry: 0priming so no wall-clock sleeps.- The three error-message assertions now check only the SDK's
"SSE stream failed: "prefix with a comment that httpx2 owns the remainder; the raw-peer tests state why the typed server cannot produce the scenario. max_sse_event_sizeis validated and threaded to all fourEventSourcesites;StreamableHttpParametersmirrors it withgt=0;uv.lockpinshttpx2==2.10.0matching the new floor. I could not independently confirm 2.10.0 is the first release exposingEventSource(max_event_size=), anddocs/migration.mdgets a one-line signature update to an existing entry — a maintainer may want to confirm both are acceptable.
Extended reasoning...
The diff adds a configurable per-event SSE byte cap to the Streamable HTTP client (src/mcp/client/streamable_http.py, src/mcp/shared/_httpx_utils.py, src/mcp/client/session_group.py), raises the httpx2 floor from 2.5.0 to 2.10.0 in pyproject.toml/uv.lock, updates docs/client/transports.md, docs/migration.md and a new docs_src tutorial, and adds roughly 320 lines of tests. It touches no auth, crypto or injection surface; the new logic lives in SSE error handling for POST, GET, resumption and reconnection streams. Approval was not chosen because the reconnection changes are behavioural rather than mechanical, the dependency-floor justification could not be verified from this checkout, migration.md is a file AGENTS.md treats as closed to new entries, and two cubic-dev-ai inline threads remain unresolved in the metadata (though later commits followed them). The bug hunt exited on dry_streak with no findings, and every item from the prior three reviews is addressed in the current code.
maxisbey
left a comment
maxisbey
left a comment
There was a problem hiding this comment.
approved with one non blocking comment
2 participants
Fixes #3332
Motivation and Context
httpx2limits each SSE event to 1 MiB by default, which rejects larger MCP tool results. Exposemax_sse_event_sizeonstreamable_http_clientandStreamableHttpParameterswhile preserving the 1 MiB default. Callers can raise the limit or passNoneto disable it. The setting applies to POST responses, GET streams, resumption, and reconnection. Oversized request events return a clear error; the background GET stream retries after a failed event.This requires
httpx2>=2.10.0. JSON responses and the legacy SSE transport are unchanged.How Has This Been Tested?
./scripts/test: 5983 passed, 9 skipped, 1 xfailed; 100% coverage andstrict-no-coverpassed.uv run --frozen ruff check .,uv run --frozen ruff format --check .,uv run --frozen pyright, anduv lock --check --offlinepassed.Breaking Changes
None. The existing 1 MiB SSE event default remains.
Types of changes
Checklist
AI Disclaimer
This PR was developed with the assistance of either Claude or Codex. I've reviewed and verified the changes.