prevent SSE upgrade with HTTP Streaming servers #393
Conversation
It appears that the current code attempts to "upgrade" to SSE if progress notifications are used. This behavior is incompatible with the TypeScript MCP SDK.
|
""" WalkthroughA configuration option was added to the streamable HTTP server to allow disabling automatic upgrades from JSON HTTP responses to Server-Sent Events (SSE) when notifications are sent. The server and session structures were updated to support this flag, and a test was introduced to verify the new behavior. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
server/streamable_http.go (2)
93-101:WithDisableSSEUpgradecould be a flag-less optionHaving the caller pass an argument that is always
truein practice (WithDisableSSEUpgrade(true)) is slightly awkward and error-prone. A common Go patterns is to expose a flag-less option that enables the behaviour and keep the default field value (false) for the regular case.-func WithDisableSSEUpgrade(disable bool) StreamableHTTPOption { - return func(s *StreamableHTTPServer) { - s.disableSSEUpgrade = disable - } -} + +// WithDisableSSEUpgrade disables automatic SSE upgrades. +func WithDisableSSEUpgrade() StreamableHTTPOption { + return func(s *StreamableHTTPServer) { + s.disableSSEUpgrade = true + } +}This eliminates the possibility of somebody writing
WithDisableSSEUpgrade(false)and thinking the flag is off while silently doing nothing.
267-268: Embedding server pointer inside session introduces a hidden cycle
streamableHttpSessionnow keeps a pointer toStreamableHTTPServer, and the server keeps references to sessions insessionToolsStore/ maps.
This is fine but creates an object graph cycle that prevents either side from becoming GC-eligible until both are released. Make suresessionTools.delete(sessionID)(andUnregisterSession) actually removes all references to the session to avoid leaks in long-running servers.server/streamable_http_test.go (2)
780-895: Great coverage – but assert header keys case-insensitivelyThe test validates
Content-Typeusing:contentType := resp.Header.Get("Content-Type")Earlier assertions use the lower-case key
"content-type". HTTP headers are case-insensitive, but mixing cases makes the test brittle and slightly confusing. Prefer the canonical form consistently:-contentType := resp.Header.Get("Content-Type") +contentType := resp.Header.Get("content-type") // or always CanonicalMIMEHeaderKeyVery minor, but helps readability.
780-895: Missing parallelisation guardThe new test spins up an HTTP server and performs network operations. Marking the sub-test (
t.Run) or the whole function witht.Parallel()improves overall suite speed and prevents accidental shared-state coupling with other tests.func TestStreamableHTTPServer_DisableSSEUpgrade(t *testing.T) { t.Parallel() ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/streamable_http.go(6 hunks)server/streamable_http_test.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
server/streamable_http.go (1)
mcp/types.go (1)
JSONRPCNotification(318-321)
🔇 Additional comments (1)
server/streamable_http.go (1)
140-141: Potential data race ondisableSSEUpgrade
disableSSEUpgradeis read from goroutines that out-live request handling (UpgradeToSSEWhenReceiveNotification).
Although the field is written only at construction time (before other goroutines start) and plain reads of aligned booleans are race-free on current architectures, the Go race detector still flags unsynchronised concurrent access as a race.If you ever plan to make this option mutable at runtime, guard the field with
atomic.Boolor move the value into the session at construction.
|
Let me know about the nitpicks. I'll be going on vacation next week at which point I'll be of little help; I'd like to see the problem fixed whether or not my pull request is the basis for that fix. |
|
I asked Claude Code to build a simple MCP server with progress indicators that supports all three transports, and then python and typescript clients, and produce a report of what works. You can see the results here: |
|
Update: I added a branch https://github.com/rubys/mcp-time-progress-server/tree/cancelable which adds:
This allows me to demonstrate the original problem I was seeing that caused me to explore HTTP streaming. If you run Running A few other things I noted which are protocol or client SDK related, but not directly related to mcp-go:
For context, I work at fly.io. and we have a |
|
hi @rubys If we disableSSEUpgrade, client will not receive notification related to concurrently-running request, I think that is a problem. And the official SDK way is to use
It is different way from mcp-go, but it seems that client should support both stream and json. could you please explain more why auto-upgrade is not incompatible with the TypeScript MCP SDK
|
|
Thanks for responding! If you run the tests you can see the problem. While a request that sends progress updates is active, no other request will be accepted from any client using stdio or Streaming HTTP, only with SSE. Disabling the upgrade was a Claude Code suggestion and it very well might be a red herring. I asked Claude Code to build a small MCP server from scratch with no mcp library using go routines and channels so that no i/o is blocked, and it worked for this small test case. I can push the results if they are of interest. |
|
While on vacation I vibe coded an entire MCP SDK with an emphasis on concurrency and compatibility to prove that it could be done: https://github.com/rubys/mcp-go-sdk I didn't do it because I believe that the world needs yet another SDK; I did it because I need concurrency. Perhaps there are some ideas in that implementation that can be adapted? |
Description
It appears that the current code attempts to "upgrade" to SSE if progress notifications are used. This behavior is incompatible with the TypeScript MCP SDK. This change prevents the upgrade
Type of Change
Checklist
MCP Spec Compliance
Additional Information
Summary by CodeRabbit
New Features
Tests