Handle streamable POSTs to properly support new streamable transport protocol spec #91
Conversation
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
|
@mattt trying to follow from my phone while offline today so may have missed it, but while you are in there I believe we are suppose to wait until after we are initialized before starting up the GET. Wasn’t a clean way to do that since the transport itself isn’t aware. If you haven’t done that and can think of a good way, that would let us avoid the error on initial GET (that works on retry because it then has the session id) |
9 tasks
|
@stallent Thank you for digging into the details of how the MCP specifies streamable HTTP transport. I opened another PR (#96) which incorporates these changes while replacing our ad hoc SSE implementation to a more robust package. Please take a look and let me know if you see anything here that's missing in #96 ✌️ |
mattt
added a commit
that referenced
this pull request
May 6, 2025
- Add a signaling mechanism to ensure the SSE streaming task waits for the initial session ID to be set before attempting the GET connection. - Use a TaskGroup to race between session ID signal and a 10-second timeout, logging the outcome. - Trigger the signal when a session ID is received for the first time from any response. - Add detailed logging for all code paths to aid debugging and clarify connection timing. - Clean up signal resources on disconnect to prevent leaks. This addresses the initialization timing issue where the SSE GET could be attempted before the session was established, as discussed in MCP PR #91.
mattt
added a commit
that referenced
this pull request
May 6, 2025
- Add a signaling mechanism to ensure the SSE streaming task waits for the initial session ID to be set before attempting the GET connection. - Use a TaskGroup to race between session ID signal and a 10-second timeout, logging the outcome. - Trigger the signal when a session ID is received for the first time from any response. - Add detailed logging for all code paths to aid debugging and clarify connection timing. - Clean up signal resources on disconnect to prevent leaks. This addresses the initialization timing issue where the SSE GET could be attempted before the session was established, as discussed in MCP PR #91.
mattt
added a commit
that referenced
this pull request
May 6, 2025
* Delay SSE GET connection until after session ID is established - Add a signaling mechanism to ensure the SSE streaming task waits for the initial session ID to be set before attempting the GET connection. - Use a TaskGroup to race between session ID signal and a 10-second timeout, logging the outcome. - Trigger the signal when a session ID is received for the first time from any response. - Add detailed logging for all code paths to aid debugging and clarify connection timing. - Clean up signal resources on disconnect to prevent leaks. This addresses the initialization timing issue where the SSE GET could be attempted before the session was established, as discussed in MCP PR #91. * Update tests for SSE timing * Use configurable timeout for acquiring session ID * Remove Actor protocol conformance for HTTPClientTransport * Configure shorter session ID timeout for tests * Add documentation comments to HTTPClientTransport * Rename sessionIDWaitTimeout to sseInitializationTimeout
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 streamable http transport spec handles SSE a bit different than the original sse transport spec. In the original spec, POSTs returned their responses as sse messages on the GET stream. Now the POSTs themselves can sse streams so the GET becomes primarily for notifications (or resuming a POST that experienced a broken connection, but thats a separate can of worms that has not been properly addressed yet).
Motivation and Context
The current HTTPClientTransport does not support streams coming back from POSTs and it must to work with latest spec servers.
How Has This Been Tested?
We have 3 sandbox servers set up with latest server sdk from anthropic. Each configured differently to exercise expected variations.
// streaming response + session management
https://hgai-sandbox.aks.stage.mercury.io/streamable/mcp
// JSON response + session management
https://hgai-sandbox.aks.stage.mercury.io/streamable/json/mcp
// JSON response with no session management
https://hgai-sandbox.aks.stage.mercury.io/streamable/stateless/mcp
Breaking Changes
none
Types of changes
Checklist
Additional context
Supporting true resumability will require some additional work that was beyond the scope of this initial PR