vMCP does not follow standard MCP Streamable HTTP behavior#3624
vMCP does not follow standard MCP Streamable HTTP behavior#3624juancarlosm wants to merge 2 commits intostacklok:mainfrom
Conversation
- Reject GET requests missing Accept: text/event-stream with HTTP 406 and JSON-RPC error, avoiding the 15s discovery timeout on invalid requests - Enable SDK heartbeat pings (30s interval) to prevent proxies and load balancers from closing idle streaming connections - Remove WriteTimeout from http.Server since it kills long-lived streaming connections after 30s Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
juancarlosm
requested review from
JAORMX,
amirejaz,
jerm-dro,
jhrozek and
yrobla
as code owners
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3624 +/- ##
==========================================
- Coverage 65.71% 65.70% -0.02%
==========================================
Files 410 410
Lines 40624 40632 +8
==========================================
- Hits 26697 26696 -1
- Misses 11846 11853 +7
- Partials 2081 2083 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jerm-dro
left a comment
jerm-dro
left a comment
There was a problem hiding this comment.
Thanks @juancarlosm for clearly documenting this issue and contributing the fix 🙏
I left a few blocking comments in the review. Two high level comments:
- It'd be great to add a test for this. This can be tested on a server with pretty minimal config (e.g. like here).
- The
streamableServerwhich acts as our MCP server comes from this repo: https://github.com/mark3labs/mcp-go. Do you think it would make more sense to make the change there? I imagine other servers that build upon the sdk also have bugs like this. From my perspective, an MCP server sdk ought to validate headers conform to the spec, but I'm curious what you think.
Thanks again for contributing 😄
| //nolint:errcheck // Best-effort write of error response body | ||
| _, _ = w.Write([]byte(`{"jsonrpc":"2.0","id":"server-error","error":{"code":-32600,"message":"Not Acceptable: Client must accept text/event-stream"}}`)) |
There was a problem hiding this comment.
nit: let's log the error instead of ignoring it entirely.
| // Accept header validation is the innermost wrapper so invalid GETs are rejected | ||
| // immediately without triggering discovery timeout or middleware overhead. |
There was a problem hiding this comment.
Blocker: The comment contradicts the code. As the code is written, this validation will actually execute last, because the middlewares below wrap it.
The comment is correct this is the innermost, but we actually want it to be the outermost, so it can fail requests immediately.
There was a problem hiding this comment.
The comment above re: code wrapping and execution order also appears incorrect. The actual execution order is:
// Execution order: recovery → auth → audit → discovery → backend enrichment → telemetry → handler
Let's remove the statement about "Code wraps," since the ordering of wrapping is clearly documented below. The statement about execution order is good to keep, since it documents our intention.
| // | ||
| // Accept header validation is the innermost wrapper so invalid GETs are rejected | ||
| // immediately without triggering discovery timeout or middleware overhead. | ||
| var mcpHandler http.Handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { |
There was a problem hiding this comment.
nit: can you pull this out into a headerValidatingMiddleware function?
This server.Start code already has a lot going on. It's fine to keep the new function in this file.
| } | ||
|
|
||
| // acceptsEventStream checks whether the request's Accept header includes text/event-stream. | ||
| func acceptsEventStream(r *http.Request) bool { |
There was a problem hiding this comment.
suggestion: delete this thin function and inline it in the proposed headerValidatingMiddleware.
| WriteTimeout: defaultWriteTimeout, | ||
| IdleTimeout: defaultIdleTimeout, | ||
| MaxHeaderBytes: defaultMaxHeaderBytes, | ||
| // WriteTimeout is intentionally omitted. SSE connections are long-lived |
There was a problem hiding this comment.
Question, Blocking: can you write a test to confirm this needs to be removed? From my reading of the WriteTimeout, this is actually how long it takes to respond to each request on the stream.
// WriteTimeout is the maximum duration before timing out
// writes of the response. It is reset whenever a new
// request's header is read. Like ReadTimeout, it does not
// let Handlers make decisions on a per-request basis.
// A zero or negative value means there will be no timeout.
In other words, I expect a session can live for more than 30s if each request on the stream is responded to in less than 30s.
2 participants
Added:
fixes: #3623
Tested:
curl http://vmcp-search.mcp.svc.cluster.local:4483 {"jsonrpc":"2.0","id":"server-error","error":{"code":-32600,"message":"Not Acceptable: Client must accept text/event-stream"}} curl http://vmcp-search.mcp.svc.cluster.local:4483 -H "Accept: text/event-stream" event: message data: {"jsonrpc":"2.0","id":1,"method":"ping"}