mcp: keep a long-running POST stream visibly alive - #1197
Open
spkenny455 wants to merge 2 commits into
Open
Conversation
The SSE response to a POST writes nothing until the request it carries completes. A tool call that runs for minutes therefore produces no bytes at all in the meantime, not even the response headers, so a client that applies a first-byte or idle timeout cannot tell a working call apart from a dead connection and hangs up on it. Intermediaries that buffer idle responses have the same problem. The standalone GET stream already handles this (modelcontextprotocol#410) by committing the headers and writing an SSE comment up front. Do the same for streams created by servePOST, after a short delay: committing the headers fixes the HTTP status, and a SEP-2575 protocol-level error must still be able to set its own. Those errors are produced without any I/O, so a stream still silent after the delay is a genuinely long-running call. If the stream has already been flushed, such an error is delivered as an ordinary SSE event instead, which is the only option once the status is fixed. The flush holds the stream mutex, serialising it with deliverLocked and close (the other writers to s.w) and with release, which clears the writer and the per-request headersFlushed flag so a resumed stream can still set an error status. Fixes modelcontextprotocol#1155
acquireStream only wrote an SSE comment for the standalone GET (s.id == ""). A Last-Event-ID resume of an in-flight POST with an empty replay therefore hung with uncommitted headers, the same first-byte bug as the original POST. Flush once when claiming a live stream whose headers are still uncommitted. Also drop the idle-timeout overclaim (the write is one-shot), take the stream mutex around the priming write, and record implicit WriteHeader(200) in the test helper.
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
2 participants
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 SSE response to a POST writes nothing until the request it carries completes. A tool call that runs for minutes therefore produces no bytes at all in the meantime, not even the response headers, so a client that applies a first-byte or idle timeout cannot tell a working call apart from a dead connection and hangs up on it. Intermediaries that buffer idle responses have the same problem.
The standalone GET stream already handles this (#410) by committing the headers and writing an SSE comment up front. Do the same for streams created by
servePOST, after a short delay: committing the headers fixes the HTTP status, and a SEP-2575 protocol-level error must still be able to set its own. Those errors are produced without any I/O, so a stream still silent after the delay is a genuinely long-running call. If the stream has already been flushed, such an error is delivered as an ordinary SSE event instead.An immediate flush at stream setup is wrong: it breaks
TestStreamableStateless_NewProtocolSession_NoFakeInitwith a superfluousWriteHeaderfromdeliverLocked.The delay is hardcoded at 1s (not a
StreamableHTTPOptionsknob). Happy to change the delay or make it configurable if that's preferred.Test plan
go test ./mcp/ -count=1go test -race ./mcp/ -count=1TestPOSTStreamFlushesHeadersEarly— headers and an SSE comment arrive while the tool is still blockedTestPOSTProtocolErrorKeepsOverrideStatus— in-streamInvalidParamsstill returns HTTP 400 before the delayTestPOSTUnknownMethodKeeps404— pre-streamMethodNotFoundstill returns HTTP 404release()reset, JSON buffering,close(), and concurrent flush vsdeliverLockedFixes #1155