Skip to content

[Client] Parse CRLF- and CR-delimited SSE events#401

Open
cbcoutinho wants to merge 1 commit into
modelcontextprotocol:mainfrom
cbcoutinho:fix/client-sse-crlf-framing
Open

[Client] Parse CRLF- and CR-delimited SSE events#401
cbcoutinho wants to merge 1 commit into
modelcontextprotocol:mainfrom
cbcoutinho:fix/client-sse-crlf-framing

Conversation

@cbcoutinho

Copy link
Copy Markdown

The bug

HttpTransport finds SSE event boundaries with strpos($this->sseBuffer, "\n\n") and splits event lines on "\n". A server that terminates SSE lines with CRLF therefore never produces a recognised event: the response accumulates in sseBuffer, handleMessage() is never called, no response is ever stored for the pending request, and after initTimeout the fiber is resumed with an error:

Mcp\Exception\ConnectionException: Initialization failed: Request timed out

thrown from Client::connect().

Why it has not been noticed

This repository's own server emits LF (Server/Transport/StreamableHttpTransport.php), so client and server agree throughout the test suite — while disagreeing with most servers in the wild.

Every MCP server built on the Python SDK is affected. FastMCP.streamable_http_app() returns sse_starlette's EventSourceResponse without a sep argument, and sse_starlette's DEFAULT_SEPARATOR is "\r\n". Hexdump of an initialize response from mcp 1.28.1:

6576 656e 743a 206d 6573 7361 6765 0d0a   event: message\r\n
6461 7461 3a20 7b22 6a73 6f6e ...  0d0a   data: {"jsonrpc"...\r\n
0d0a                                      \r\n

There is no "\n\n" substring anywhere in that payload.

The SSE specification requires a parser to accept CRLF, LF or CR, so the previous behaviour was non-compliant rather than merely unlucky.

The fix

  • Event extraction accepts \r\n\r\n, \n\n and \r\r, choosing whichever occurs first so a buffer split mid-sequence is handled correctly. None of the three is a substring of another, so a partial terminator at the end of a read simply waits for more data.
  • Line splitting within an event uses the same set.
  • A stream that reaches EOF with a trailing partial event now dispatches it rather than discarding it. Previously activeStream was only cleared when the buffer was empty, so a stream ending without a blank line also hung.

Tests

Adds tests/Unit/Client/Transport/HttpTransportTest.php — the first tests for src/Client/Transport/. It drives Client::connect() through a stub PSR-18 client across six framings: LF, CRLF, CR, an id: field, a comment preamble, and a missing trailing blank line.

Without the fix, four of the six fail with the timeout above. With it, all pass.

Verification on my machine:

  • vendor/bin/phpunit tests/UnitOK (823 tests, 2464 assertions)
  • vendor/bin/phpstan analyse src/Client/Transport/HttpTransport.php[OK] No errors
  • vendor/bin/php-cs-fixer fix --dry-run --diff on the changed file → clean
  • Full suite: 32 errors before, 28 after — the difference is exactly the four new tests. The remaining failures are pre-existing on main (tests/Inspector) and unrelated to this change.
  • End to end against a live mcp 1.28.1 server (nextcloud-mcp-server): connect(), tools/list and tools/call all succeed, with both a streaming PSR-18 client (Symfony\Component\HttpClient\Psr18Client) and a fully-buffered one. Before the fix both fail.

Notes

I have not touched CHANGELOG.md, since there is no Unreleased section and I did not want to guess at your release process — happy to add an entry wherever you prefer.


This PR was generated with the help of AI, and reviewed by a Human

HttpTransport looked for an event boundary with strpos($buffer, "\n\n") and
split event lines on "\n". Servers that terminate SSE lines with CRLF therefore
never produced a recognised event: the response accumulated in the buffer, no
message was ever dispatched, and connect() failed with "Initialization failed:
Request timed out".

This affects every MCP server built on the Python SDK. FastMCP's
streamable_http_app() returns sse_starlette's EventSourceResponse without a sep
argument, and its DEFAULT_SEPARATOR is "\r\n". The bug is invisible from within
this repository because the PHP server implementation emits LF, so client and
server agree in the test suite while disagreeing with most servers in the wild.

The SSE specification requires a parser to accept CRLF, LF or CR, so the
previous behaviour was non-compliant rather than merely unlucky.

Event extraction now accepts all three terminators, choosing whichever appears
first so a buffer split mid-sequence is handled correctly, and line splitting
uses the same set. A stream that ends without a trailing blank line now
dispatches its final event instead of discarding it.

Adds the first tests for src/Client/Transport/, covering LF, CRLF and CR
framing, an id field, a comment preamble, and a missing trailing blank line.
Without the fix four of the six fail with the timeout above.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant