Repository navigation
fix(agent/x/agentmcp): disable standalone SSE for HTTP MCP servers - #30484
jeremyruppel wants to merge 2 commits into
Conversation
The go-sdk opens the standalone SSE GET inside Connect on a context the connect timeout does not bound. A server that never answers that GET (for example, the Storybook dev server's /mcp endpoint) hangs Connect indefinitely, which wedges the shared reload so no workspace MCP server config change is picked up until the agent restarts. The manager registers no handlers for server-initiated messages, so the stream is unused. Tool call responses still arrive on the POST. Generated by Coder Agents.
This comment was marked as resolved.
This comment was marked as resolved.
|
Chat: Review posted | View chat Review history
deep-review v0.13.0 | Round 1 | Last posted: Round 1, 4 findings (1 P3, 2 P4, 1 Nit), COMMENT. Review Finding inventoryFindings
Contested and acknowledgedRound logRound 1Full panel (round 1). Netero: no findings; Law not run (62 effective additions). Panel: ging-go, ryosuke, takumi, melody (specialists), pariston, mafuuu (core), bisky (tests changed), gon, leorio (once-per-PR floor), zoro (wildcard). 9 panel reviewers plus wildcard exceeds the focused target of 6-8 because the four Go/context/SSE specialists, core, Bisky, and the Gon/Leorio floor are all required. 1 P3, 2 P4, 1 Nit posted; 2 Nits dropped; 2 Notes and 7 out-of-scope items in body. Gon's P2 on the test doc comment downgraded to P4: keep-argument was that a reader of a failing test cannot see why the GET matters; downgraded because the reason belongs in the production comment (CRF-2) and Leorio judged the test comment adequate. Reviewed against 6db5a36..560e94b. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
This comment was marked as resolved.
This comment was marked as resolved.
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR sets DisableStandaloneSSE: true on the agent's streamable HTTP MCP transport so a server that never answers the standalone GET cannot hang Connect and the singleflighted reload. The regression test fails with the option set to false (10s RequireReceive: context expired) and passes with it. 1 P3, 2 P4, 1 Nit.
Out of scope (needs a ticket or explicit acceptance by a human):
agent/x/agentmcp/manager.go:922:SSEClientTransport.Connectopens its event-stream GET onconnectCtx, anddefer cancel()inconnectServercancels it on return. A scratch test againstmcp.NewSSEHandlerconnected, thenListToolsreturnedEOF, so every"type": "sse"server in.mcp.jsonconnects and then exposes no working tools.coderd/x/chatd/mcpclient/mcpclient.go:692: chatd'sStreamableClientTransportstill opens the standalone GET with no handlers registered, so each connect to a Storybook-like server leaves a goroutine and session blocked inConnect.aibridge/mcp/proxy_streamable_http.go:43: same transport setup, andInithas no outer budget aroundConnect, so a server that never answers the GET can blockInitwithout limit (inferred from the SDK code, not reproduced).agent/x/agentmcp/manager.go:574: a server that is down at the first connect is not retried until.mcp.jsonchanges. The PR description names this as follow-up; no ticket is linked.agent/x/agentmcp/manager.go:449:doReloadhas no deadline of its own, so any other SDK call that ignores its context hangs every later reload.agent/x/agentmcp/manager.go:853:Manager.Closeholdsm.muwhile closing each server; an HTTP server that ignores the session DELETE stallsCatalog()for up to 5s per server.- go-sdk v1.8.0
mcp/streamable.go:2168:connectStandaloneSSEruns synchronously insideClient.Connecton the detached connection context, soConnectignores the caller's deadline. This is the upstream root cause.
Notes:
agent/x/agentmcp/manager_internal_test.go:276: the test reaches the standalone GET only because the go-sdk test server negotiates a protocol below 2026-07-28 (sessionUpdatedskips the GET at or above it). After an SDK upgrade that changes the negotiated version, the test would pass with the fix reverted.- PR description: the Storybook-restart example for the removed failure mode is incomplete. After a restart the old session ID is gone, so I expect the next POST to fail, and nothing in the agent reconnects a dead session (not verified against Storybook).
🤖 This review was automatically generated with Coder Agents.
| return &mcp.StreamableClientTransport{ | ||
| Endpoint: cfg.URL, | ||
| HTTPClient: httpClientWithHeaders(cfg.Headers), | ||
| // Server-initiated messages are unused, and a server that never |
There was a problem hiding this comment.
P3 [CRF-1] "Server-initiated messages are unused" is true only because connectServer passes nil options to mcp.NewClient (line 890), and nothing at that call points back to this field. (Ryosuke, Pariston, Mafuuu, Gon, Zoro)
If someone adds a ToolListChangedHandler, logging, sampling, or elicitation handler there, it compiles and passes tests, runs for stdio servers, and never fires for HTTP servers. Add a one-line comment at the mcp.NewClient call naming DisableStandaloneSSE as the reason server-initiated handlers will not work over HTTP.
🤖
There was a problem hiding this comment.
Fixed in 93b6ea3: added a comment at the mcp.NewClient call pointing at DisableStandaloneSSE.$\n\n> 🤖 Generated by Coder Agents on behalf of @jeremyruppel.
| return &mcp.StreamableClientTransport{ | ||
| Endpoint: cfg.URL, | ||
| HTTPClient: httpClientWithHeaders(cfg.Headers), | ||
| // Server-initiated messages are unused, and a server that never |
There was a problem hiding this comment.
P4 [CRF-2] "would block Connect" does not say that connectTimeout fails to bound the block, so a reader will assume the worst case is 30s. (Gon, Leorio)
go-sdk v1.8.0 issues the GET with c.connectSSE(c.ctx, ...), where c.ctx is detached from the connect context, so the hang has no limit and stalls every later reload. A maintainer who thinks it costs 30s may remove this option to get tools/list_changed. State that the SDK opens the GET inside Connect on a context connectTimeout does not cancel.
🤖
There was a problem hiding this comment.
Fixed in 93b6ea3: the comment now states the GET runs on a context connectTimeout does not cancel, so the hang is unbounded and stalls later reloads.$\n\n> 🤖 Generated by Coder Agents on behalf of @jeremyruppel.
| assert.Equal(t, "echo", result.Tools[0].Name) | ||
| } | ||
|
|
||
| // TestConnectServer_HTTPIgnoresUnansweredStandaloneSSE verifies that an |
There was a problem hiding this comment.
P4 [CRF-3] The doc comment restates the test name and does not name the bug the test guards against. (Gon)
The sibling TestConnectServer_StdioProcessSurvivesConnect states its bug. Someone seeing this test fail with RequireReceive: context expired needs to know that the SDK's standalone GET is not bounded by connectTimeout. One clause pointing at DisableStandaloneSSE in createTransport is enough.
🤖
There was a problem hiding this comment.
Fixed in 93b6ea3: the doc comment names the unbounded standalone GET and points at DisableStandaloneSSE in createTransport.$\n\n> 🤖 Generated by Coder Agents on behalf of @jeremyruppel.
| // TestConnectServer_HTTPIgnoresUnansweredStandaloneSSE verifies that an | ||
| // HTTP server which never answers the standalone SSE GET does not block | ||
| // connect or tool listing. | ||
| func TestConnectServer_HTTPIgnoresUnansweredStandaloneSSE(t *testing.T) { |
There was a problem hiding this comment.
Nit [CRF-4] HTTPIgnoresUnansweredStandaloneSSE implies the client sends the GET and tolerates no answer, but with the fix the client never sends it. (Gon)
A name for the asserted behavior, such as TestConnectServer_HTTPConnectsWhenStandaloneSSEHangs, matches what the test checks.
🤖
There was a problem hiding this comment.
Fixed in 93b6ea3: renamed to TestConnectServer_HTTPConnectsWhenStandaloneSSEHangs.$\n\n> 🤖 Generated by Coder Agents on behalf of @jeremyruppel.
|
Addressed the R1 inline findings in 93b6ea3. Notes:
Out of scope (CRF-9 to CRF-15): acknowledged, no changes in this PR.$\n\n> 🤖 Generated by Coder Agents on behalf of @jeremyruppel. |
|
my agent also called out CRF-9 as what could be an actual bug. I will keep the chat around so say the word and I'll have the agent implement a fix |
This branch has not been deployed
2 participants
tl;dr is this should fix the storybook mcp server in Coder Agents. the storybook mcp never responds to the SSE GET, so it never connects, and reload fails. this disables that SSE GET and should let storybook reconnect on config reload
HTTP workspace MCP servers whose
/mcpendpoint never answers the standalone SSE GET (for example, the Storybook dev server) hang the agent's MCP reload indefinitely. The go-sdk opens that GET insideConnecton a context the agent's 30s connect timeout does not bound, and because reloads are singleflighted, every later.mcp.jsonchange waits behind the hung one. The server never shows up as connected or failed, and no MCP config change is picked up until the agent restarts.Set
DisableStandaloneSSE: trueon the streamable HTTP transport. The agent's client registers no handlers for server-initiated messages, so the stream is unused; tool call responses and in-request notifications still arrive on the POST.Investigation and decision log
Symptom: a
storybookHTTP server in.mcp.jsonnever producedstorybook__*tools in chats, while stdio servers worked.Findings:
connection refused(Storybook was not up yet), and a later reload failing withstandalone SSE request failed (session ID: ...), meaninginitializehad succeeded and the GET stream was the failure point..mcp.jsonwith Storybook running, no reload result was ever logged (success or warning).Connectpast its 30s timeout. Goroutine dump:Client.Connect -> sessionUpdated -> connectStandaloneSSE -> connectSSE, which uses the connection's lifetime context, not the connect context.curlagainst Storybook:POST initializereturns a session ID;GET /mcpwith that session sends no response headers at all. The spec requires eithertext/event-streamor 405, so Storybook is non-compliant, but the agent should not wedge on it.DisableStandaloneSSE: true, the same probe connected in ~7ms and listed all 8 Storybook tools.Why disable instead of alternatives:
mcp.NewClient(..., nil)inmanager.goregisters notools/list_changed, sampling, elicitation, roots, or logging handlers, so nothing on the standalone stream is consumed today.Connectwith our own timeout would require abandoning a stuck goroutine and session per bad server, since the hang is inside the SDK call on a context we do not control.tools/list_changedrefresh would need the stream back, opened afterConnectand off the reload path.Out of scope, noted for follow-up:
.mcp.jsonchanges.Generated by Coder Agents on behalf of @jeremyruppel.