fix(mcpproxy): skip non-response SSE events during initialize (#2219) - #2267
Conversation
…roxy#2219) Signed-off-by: pjdurden <prajjwalchittori1@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes MCP proxy session initialization failures when a backend’s initialize SSE stream begins with a non-response keep-alive event (e.g., an empty data: line) before the actual JSON-RPC response, as reported in #2219.
Changes:
- Skip empty
data:lines in the SSE event parser so keep-alive/heartbeat events don’t trigger JSON-RPC decode errors. - Update the initialize SSE reader loop to ignore non-response events and keep reading until a JSON-RPC
*Responseis encountered. - Add targeted regression tests covering both the parser behavior and end-to-end session creation.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| internal/mcpproxy/sse.go | Skips empty SSE data: lines to avoid decode failures on keep-alives. |
| internal/mcpproxy/sse_test.go | Adds a regression test ensuring empty data: lines are skipped without error. |
| internal/mcpproxy/mcpproxy.go | Initialize loop now skips non-response events until it finds a JSON-RPC response. |
| internal/mcpproxy/mcpproxy_test.go | Adds a regression test reproducing the leading keep-alive initialize stream scenario. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if parseErr != nil { | ||
| if errors.Is(parseErr, io.EOF) || strings.Contains(parseErr.Error(), "context deadline exceeded") { |
There was a problem hiding this comment.
Addressed in 115b66d. The initialize reader now tracks the terminating read error and, when the SSE stream ends before any JSON-RPC response arrives, returns a dedicated error instead of falling through to the misleading "MCP message is not a response: ": a wrapped read error for non-EOF/deadline cases, and "MCP initialize stream from backend ... ended before a JSON-RPC response was received" for clean EOF/deadline. Added TestInitializeSession_SSEEndsBeforeResponse to cover it.
…esponse Address review feedback: the initialize reader broke on EOF/deadline with rawMsg still nil, producing the misleading "MCP message is not a response: <nil>". Track the read error and surface a dedicated error instead, and add a regression test for an initialize stream that closes before any JSON-RPC response arrives. Signed-off-by: pjdurden <prajjwalchittori1@gmail.com>
115b66d to
e7c7446
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2267 +/- ##
=======================================
Coverage 84.86% 84.87%
=======================================
Files 144 144
Lines 21308 21317 +9
=======================================
+ Hits 18084 18093 +9
+ Misses 2139 2138 -1
- Partials 1085 1086 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…roxy#2219) (envoyproxy#2267) **Description** Fixes envoyproxy#2219. The MCP proxy fails `initialize` with `500 failed to create MCP session to any backend` and logs `MCP message is not a response: <nil>` when a backend's `initialize` SSE response begins with a non-response event (an empty/keep-alive `data:` line) before the JSON-RPC result. Some FastMCP backends do this depending on the requested `protocolVersion` (e.g. firecrawl with `2025-11-25`). Two layers caused it: 1. `sseEventParser.parseEvent` (`internal/mcpproxy/sse.go`) called `jsonrpc.DecodeMessage` on an empty `data:` line, which errors, so the whole leading event was returned as a parse error. 2. The initialize reader loop (`internal/mcpproxy/mcpproxy.go`) treated that non-EOF error as fatal and broke with `rawMsg` still nil, then reported it as "not a response". This change: - Skips empty `data:` lines in the SSE parser (keep-alive/heartbeat events carry no message to decode). Non-empty but malformed data still errors as before. - Makes the initialize reader skip non-response events and keep reading until it finds the JSON-RPC `*Response`, instead of requiring the first event to be the response. **Testing** - `TestSSEEventParser_EmptyDataLineSkipped` — a leading event with an empty data line yields an event with no messages and no error; the following event still decodes. - `TestNewSession_SSEWithLeadingKeepAlive` — a backend whose initialize SSE response starts with an empty keep-alive event before the real response now initializes successfully (this test reproduced the `MCP message is not a response: <nil>` failure before the fix). ``` go test ./internal/mcpproxy/ go vet ./internal/mcpproxy/ go build ./... ``` Done with AI assistance; I have reviewed every line and understand the change. --------- Signed-off-by: pjdurden <prajjwalchittori1@gmail.com> Co-authored-by: Ignasi Barrera <ignasi@tetrate.io> Signed-off-by: Daniel Chernovsky <daniel.chernovsky@doubleverify.com>
Description
Fixes #2219.
The MCP proxy fails
initializewith500 failed to create MCP session to any backendand logsMCP message is not a response: <nil>when a backend'sinitializeSSE response begins with a non-response event (an empty/keep-alivedata:line) before the JSON-RPC result. Some FastMCP backends do this depending on the requestedprotocolVersion(e.g. firecrawl with2025-11-25).Two layers caused it:
sseEventParser.parseEvent(internal/mcpproxy/sse.go) calledjsonrpc.DecodeMessageon an emptydata:line, which errors, so the whole leading event was returned as a parse error.internal/mcpproxy/mcpproxy.go) treated that non-EOF error as fatal and broke withrawMsgstill nil, then reported it as "not a response".This change:
data:lines in the SSE parser (keep-alive/heartbeat events carry no message to decode). Non-empty but malformed data still errors as before.*Response, instead of requiring the first event to be the response.Testing
TestSSEEventParser_EmptyDataLineSkipped— a leading event with an empty data line yields an event with no messages and no error; the following event still decodes.TestNewSession_SSEWithLeadingKeepAlive— a backend whose initialize SSE response starts with an empty keep-alive event before the real response now initializes successfully (this test reproduced theMCP message is not a response: <nil>failure before the fix).Done with AI assistance; I have reviewed every line and understand the change.