[Client] Parse CRLF- and CR-delimited SSE events - #401
Open
cbcoutinho wants to merge 1 commit into
Open
Conversation
cbcoutinho
requested review from
CodeWithKyrian,
Nyholm,
chr-hertel and
soyuka
as code owners
July 25, 2026 21:33
Member
|
Hi @cbcoutinho, thanks for reporting & patching - i don't think we need a changelog entry for this - i'd consider it a bugfix only and don't see any behavior change besides that fix - or do i miss sth? Needs a rebase tho, please |
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>
cbcoutinho
force-pushed
the
fix/client-sse-crlf-framing
branch
from
July 27, 2026 19:13
3e8ab27 to
6c24df0
Compare
Author
|
Hi @chr-hertel I agree, I don't think a changelog entry is needed. Let me know if you think one is needed. The branch has been rebased on latest |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
HttpTransportfinds SSE event boundaries withstrpos($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 insseBuffer,handleMessage()is never called, no response is ever stored for the pending request, and afterinitTimeoutthe fiber is resumed with an error: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()returnssse_starlette'sEventSourceResponsewithout asepargument, andsse_starlette'sDEFAULT_SEPARATORis"\r\n". Hexdump of aninitializeresponse frommcp1.28.1: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
\r\n\r\n,\n\nand\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.activeStreamwas 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 forsrc/Client/Transport/. It drivesClient::connect()through a stub PSR-18 client across six framings: LF, CRLF, CR, anid: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/Unit→OK (823 tests, 2464 assertions)vendor/bin/phpstan analyse src/Client/Transport/HttpTransport.php→[OK] No errorsvendor/bin/php-cs-fixer fix --dry-run --diffon the changed file → cleanmain(tests/Inspector) and unrelated to this change.mcp1.28.1 server (nextcloud-mcp-server):connect(),tools/listandtools/callall 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 noUnreleasedsection 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