mcp: allow disabling stream replay; disable by default #585
+420
−307
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
neild
previously approved these changes
Oct 15, 2025
jba
requested changes
Oct 15, 2025
neild
previously approved these changes
Oct 16, 2025
This CL implements two changes that must be logically connected. The first is to provide a mechanism for disabling stream replay. Previously, our documentation stated that if StreamableServerTransport.EventStore was nil, a default in-memory event store would be used. In that case, there would be no way to completely disable stream replay, and as described in modelcontextprotocol#576, there are many cases where replay is undesirable. But of course changing the behavior of the transport zero value implicitly affects its default behavior, and so this CL also implements the proposal modelcontextprotocol#580: stream replay is disabled by default. Implementing this change required a significant refactoring, as previously we were relying on the event store for serialized message delivery from the JSON-RPC layer to the MCP layer: the connection would write to the event store, and independently the stream (be it an incoming POST or replay GET) would iterate and serve messages in the stream. In order to achieve the goals of this CL, it was necessary to decouple storage from delivery. The 'stream' abstraction now tracks a delivery callback that writes straight to the HTTP response. It would have been convenient to store the ongoing http.ResponseWriter directly in the stream (this is how typescript does it), but due to the design of our EventStore API, only the HTTP handler knows the next event index, so a 'deliver' abstraction was an unfortunate requirement (suggestions for how to further simplify this are welcome). More simplification is possible: in particular, as a result of this refactoring it should be entirely possible to clean up streams once we've received all responses. Any replay would only need access to the EventStore, if at all. This is left to a follow-up CL, to limit this already significant change. Furthermore, a nice consequence of this refactoring is that, when not using event storage, servers can get synchronous feedback that message delivery failed, which should avoid unnecessary work. We can additionally cancel ongoing requests on early client termination, but that is also left to a follow-up CL. Throughout, the terminology 'standalone SSE stream' replaced 'hanging GET' when referring to the non-replay GET request issued by the client. This is consistent with other SDKs. Fixes modelcontextprotocol#576 Updates modelcontextprotocol#580
findleyr
force-pushed
the
streamablefixes
branch
from
d145124 to
db153ed
Compare
Contributor
Author
|
@neild could you please re-approve when you have a chance? I had to rebase and resolve merge conflicts (I also squashed since I had to force-push anyway). No changes other than resolving the conflict. |
neild
approved these changes
Oct 16, 2025
jba
approved these changes
Oct 16, 2025
findleyr
added a commit
to findleyr/go-sdk
that referenced
this pull request
Oct 16, 2025
This was missed in modelcontextprotocol#585. Updates modelcontextprotocol#580
findleyr
added a commit
that referenced
this pull request
Oct 16, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
This CL implements two changes that must be logically connected.
The first is to provide a mechanism for disabling stream replay. Previously, our documentation stated that if
StreamableServerTransport.EventStore was nil, a default in-memory event store would be used. In that case, there would be no way to completely disable stream replay, and as described in #576, there are many cases where replay is undesirable.
But of course changing the behavior of the transport zero value implicitly affects its default behavior, and so this CL also implements the proposal #580: stream replay is disabled by default.
Implementing this change required a significant refactoring, as previously we were relying on the event store for serialized message delivery from the JSON-RPC layer to the MCP layer: the connection would write to the event store, and independently the stream (be it an incoming POST or replay GET) would iterate and serve messages in the stream.
In order to achieve the goals of this CL, it was necessary to decouple storage from delivery. The 'stream' abstraction now tracks a delivery callback that writes straight to the HTTP response. It would have been convenient to store the ongoing http.ResponseWriter directly in the stream (this is how typescript does it), but due to the design of our EventStore API, only the HTTP handler knows the next event index, so a 'deliver' abstraction was an unfortunate requirement (suggestions for how to further simplify this are welcome).
More simplification is possible: in particular, as a result of this refactoring it should be entirely possible to clean up streams once we've received all responses. Any replay would only need access to the EventStore, if at all. This is left to a follow-up CL, to limit this already significant change.
Furthermore, a nice consequence of this refactoring is that, when not using event storage, servers can get synchronous feedback that message delivery failed, which should avoid unnecessary work. We can additionally cancel ongoing requests on early client termination, but that is also left to a follow-up CL.
Throughout, the terminology 'standalone SSE stream' replaced 'hanging GET' when referring to the non-replay GET request issued by the client. This is consistent with other SDKs.
Fixes #576
Updates #580