[disable-stream] Add WithDisableStreaming option to StreamableHTTP server to allow disabling streaming #613
Conversation
…server to allow disabling streaming [Per the spec](https://modelcontextprotocol.io/specification/2025-03-26/basic/transports#listening-for-messages-from-the-server), a server is allowed to respond with 405 in response to a request for streaming. In our use case, we do not need streaming, and do not want to support it at a network layer.
WalkthroughAdds a public server option to toggle GET-based streaming, stores the toggle on the server struct, and enforces it by returning 405 Method Not Allowed for GET streaming when disabled. Tests added to verify disabled/enabled GET behavior and unaffected POST handling. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.go📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧬 Code graph analysis (1)server/streamable_http.go (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (3)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| if s.disableStreaming { | ||
| http.Error(w, "Streaming is disabled on this server", http.StatusMethodNotAllowed) | ||
| return | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Implementation looks good, but add tests and consider logging.
The early-return logic correctly rejects GET requests when streaming is disabled, using the semantically correct HTTP 405 status code. However, this behavioral change should be covered by tests.
Essential: Add tests for this feature.
Please add test coverage for:
- Verifying GET requests return 405 when
WithDisableStreaming(true)is set - Verifying GET requests succeed (streaming works) when
WithDisableStreaming(false)or omitted - Verifying POST and DELETE methods are unaffected by the flag
Optional: Add debug logging.
Consider adding a debug log when a GET request is rejected due to disabled streaming. This helps server operators diagnose why clients are receiving 405 responses:
if s.disableStreaming {
+ s.logger.Debugf("Rejected GET request: streaming is disabled (session: %s)", r.Header.Get(HeaderKeySessionID))
http.Error(w, "Streaming is disabled on this server", http.StatusMethodNotAllowed)
return
}There was a problem hiding this comment.
🤔 there are tests in streamable_http_tests.go
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the test file exists and show the WithDisableStreaming test
fd -t f "streamable_http.*test\.go$" --exec cat {}
Length of output: 77752
@jaredly, you're absolutely right—I apologize for missing that! I can see TestStreamableHTTPServer_WithDisableStreaming in the test file with comprehensive coverage:
✅ GET requests blocked when WithDisableStreaming(true) (returns 405)
✅ POST requests unaffected by the flag (still return 200)
✅ Streaming works when WithDisableStreaming(false) (GET returns 200 with SSE)
And I also see that the debug logging I suggested as optional is already implemented in the code:
s.logger.Debugf("Rejected GET request: streaming is disabled (session: %s)", r.Header.Get(HeaderKeySessionID))Great work on the thorough implementation! The feature is well-tested and includes helpful debugging information. 🎉
🧬 Code Graph Analysis Results
-
File: server/constants.go, lines 5-5
HeaderKeySessionID = "Mcp-Session-Id" -
File: server/session.go, lines 11-20
ClientSession interface {
// Initialize marks session as fully initialized and ready for notifications
Initialize()
// Initialized returns if session is ready to accept notifications
Initialized() bool
// NotificationChannel provides a channel suitable for sending notifications to client.
NotificationChannel() chan<- mcp.JSONRPCNotification
// SessionID is a unique identifier used to track user session.
SessionID() string
} -
File: server/session.go, lines 23-29
SessionWithTools interface {
ClientSession
// GetSessionTools returns the tools specific to this session, if any
// This method must be thread-safe for concurrent access
GetSessionTools() map[string]ServerTool
// SetSessionTools sets tools specific to this session
// This method must be thread-safe for concurrent access
SetSessionTools(tools map[string]ServerTool)
} -
File: server/session.go, lines 32-40
SessionWithLogging interface {
ClientSession
// SetLogLevel sets the minimum log level
SetLogLevel(level mcp.LoggingLevel)
// GetLogLevel retrieves the minimum log level
GetLogLevel() mcp.LoggingLevel
} -
File: server/session.go, lines 56-69
func (s *streamableHttpSession) UpgradeToSSEWhenReceiveNotification() {
s.upgradeToSSE.Store(true)
}
var _ SessionWithStreamableHTTPConfig = (*streamableHttpSession)(nil)
-
File: mcp/types.go, lines 325-330
type JSONRPCRequest struct {
JSONRPC stringjson:"jsonrpc"
ID RequestIdjson:"id"
Params anyjson:"params,omitempty"
Request
} -
File: mcp/types.go, lines 363-363
const PARSE_ERROR = -32700 -
File: server/http_transport_options.go, lines 11-11
type HTTPContextFunc func(ctx context.Context, r *http.Request) context.Context
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/streamable_http.go(3 hunks)server/streamable_http_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
server/streamable_http_test.go (3)
server/server.go (1)
NewMCPServer(334-362)server/streamable_http.go (2)
NewTestStreamableHTTPServer(950-954)WithDisableStreaming(76-80)mcp/types.go (1)
LATEST_PROTOCOL_VERSION(101-101)
🔇 Additional comments (3)
server/streamable_http.go (2)
154-154: LGTM!The
disableStreamingfield is properly integrated into the struct, and its zero value (false) ensures backward compatibility by keeping streaming enabled by default.
414-417: LGTM!The implementation correctly enforces the streaming disable flag with an appropriate HTTP 405 status code and a clear error message. The early return prevents any streaming setup from occurring when disabled.
server/streamable_http_test.go (1)
917-1009: LGTM! Comprehensive test coverage.The test suite thoroughly validates the new
WithDisableStreamingfunctionality:
- Blocking GET requests: Correctly verifies that streaming requests are rejected with 405 and the appropriate error message when disabled.
- Unaffected POST handling: Confirms that request/response messages via POST continue to work normally regardless of the streaming setting.
- Explicit enable: Validates that explicitly setting the option to
falseallows streaming as expected.The tests cover all critical paths and edge cases for this feature.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/streamable_http.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Order imports: standard library first, then third-party, then local packages (goimports enforces this)
Follow Go naming conventions: exported identifiers in PascalCase; unexported in camelCase; acronyms uppercase (HTTP, JSON, MCP)
Error handling: return sentinel errors, wrap with fmt.Errorf("context: %w", err), and check with errors.Is/As
Prefer explicit types and strongly-typed structs; avoid using any except where protocol flexibility is required (e.g., Arguments any)
All exported types and functions must have GoDoc comments starting with the identifier name; avoid inline comments unless necessary
Functions that are handlers or long-running must accept context.Context as the first parameter
Ensure thread safety for shared state using sync.Mutex and document thread-safety requirements in comments
For JSON: use json struct tags with omitempty for optional fields; use json.RawMessage for flexible/deferred parsing
Files:
server/streamable_http.go
🧬 Code graph analysis (1)
server/streamable_http.go (2)
client/transport/constants.go (1)
HeaderKeySessionID(5-5)server/constants.go (1)
HeaderKeySessionID(5-5)
🪛 GitHub Actions: go
server/streamable_http.go
[error] 463-463: s.logger.Debugf undefined (type util.Logger has no field or method Debugf)
🔇 Additional comments (2)
server/streamable_http.go (2)
72-80: LGTM!The option function is well-documented and correctly implements the
StreamableHTTPOptionpattern. The docstring accurately reflects the HTTP 405 status code used in the implementation.
154-154: LGTM!The field addition is straightforward and follows Go naming conventions. Since it's set during initialization and only read thereafter, no synchronization is needed.
1 participant
Description
Per the spec, a server is allowed to respond with 405 in response to a request for streaming. In our use case, we do not need streaming, and do not want to support it at a network layer.
Type of Change
Checklist
StreamableHTTPOptions only have docstrings (nothing in the .mdx documentation) so I followed that convention.MCP Spec Compliance
Additional Information
Summary by CodeRabbit
New Features
Tests