fix(agent-core): reconnect dropped streamable-HTTP MCP sessions on tool call - #2748
fix(agent-core): reconnect dropped streamable-HTTP MCP sessions on tool call#2748tarikermis wants to merge 2 commits into
Conversation
…ol call When a streamable-HTTP MCP server's session drops mid-session (e.g. a server restart), the legacy engine never re-established it: every call to that server's tools failed for the rest of the session, and the failed server's tools were unregistered, so the model retried its work through other servers' tools instead of getting a loud, server-named error (MoonshotAI#2742). Port the agent-core-v2 recovery pattern (MoonshotAI#1991) to the legacy engine: - The wrapped MCP tool call now classifies the failure: server-answered errors are rethrown, ambiguous transport failures are probed with a ping and retried once in place when alive, and a dead transport triggers one shared reconnect through the connection manager before retrying on the fresh client. - McpConnectionManager gains reconnectAndJoin so parallel failing calls collapse into a single reconnect. - A server that flips to failed keeps its tools registered, so the next call can drive the reconnect and calls fail with the server's own error in the meantime. The default agent-core-v2 engine already recovers this way; this brings the legacy engine (KIMI_CODE_LEGACY_FLAG, SDK v1 client) to parity.
🦋 Changeset detectedLatest commit: 6db3f53 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 858b9104c9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (entry.status === 'failed') { | ||
| this.unregisterMcpServer(entry.name); | ||
| this.agent.emitEvent({ | ||
| type: 'tool.list.updated', | ||
| reason: 'mcp.failed', | ||
| serverName: entry.name, | ||
| }); | ||
| // Keep the tools registered: a dropped connection is recovered through | ||
| // the wrapped call's reconnect-and-retry path, and until then calls | ||
| // fail loudly with the server's error instead of vanishing from the | ||
| // tool list (which made the model retry through *other* servers' | ||
| // tools — see #2742). The tool list itself did not change, so no | ||
| // `tool.list.updated` event is emitted. | ||
| return; |
There was a problem hiding this comment.
When the first call-driven reconnect happens while the server is still unavailable, McpConnectionManager.reconnect() emits pending before ultimately emitting failed; the pending branch below unregisters the tools, so this failed no-op has nothing left to preserve. Consequently, one unsuccessful recovery attempt removes the server's tools for the rest of the session and later calls cannot drive another reconnect—the exact failure mode this change intends to prevent. Preserve existing tools during reconnect-style pending transitions as well, or otherwise distinguish initial/configuration pending states from recovery.
Useful? React with 👍 / 👎.
…state Addresses a P1 review finding on PR MoonshotAI#2748: McpConnectionManager.reconnect() emits 'pending' before 'failed', and the v1 ToolManager unregistered a server's tools on 'pending' too — so one failed call-driven recovery stranded the session without tools and later calls could never drive another reconnect (the exact bug the PR fixes, MoonshotAI#2742). Only 'disabled' now unregisters tools and emits mcp.disconnected; both re-registration paths (connected, needs-auth) already start by unregistering, so keeping tools through pending never leaves stale entries behind. Regression coverage: a real-HTTP-server test drives a call through a failed reconnect (server down), asserts the tools stay registered, no tool-list event fires, and the transport's own error surfaces byte-equivalently, then brings the server back and verifies the next call re-drives the reconnect and heals.
1 participant
Related Issue
Resolve #2742
Problem
When a streamable-HTTP MCP server's session drops mid-session (e.g. a server restart), the legacy agent-core engine never re-established it. Every call to that server's tools failed for the rest of the session — the SDK transport keeps POSTing with the stale session id and the restarted server answers
Bad Request: Server not initialized— and the failed server's tools were unregistered from the model's tool list, so the model retried its work through other servers' tools instead of getting a loud, server-named error (the "different tools execute" symptom in the issue).Note on engine coverage: the default agent-core-v2 engine already recovers exactly this way (reconnect-and-retry on tool-call failure, #1991) — I verified a server-restart scenario recovers transparently there. The defect was specific to the legacy engine (
KIMI_CODE_LEGACY_FLAG, SDK v1 client), which this PR brings to parity.Reproduction evidence (legacy engine, this branch before the fix): connect a real in-process streamable-HTTP MCP server through
McpConnectionManager+ToolManager, restart it mid-session, then call its tool again — the call throwsStreamable HTTP error: ... "Bad Request: Server not initialized"on every subsequent attempt, the entry staysconnected, and no reconnect is ever attempted. After the fix, the same scenario reconnects transparently and the retried call returns the correct payload.What changed
Ports the agent-core-v2 recovery pattern to the legacy engine, reusing its exact failure classification so the two engines behave identically:
mcp/client-shared.ts: addisMcpConnectionClosedError/isMcpTransportFailure/isMcpMalformedResultError/probeMcpLiveness(ported fromagent-core-v2/src/mcpCore/client-shared.ts), andMCPClient.ping(bounded by a 5s probe timeout) implemented by the stdio/SSE/HTTP clients.mcp/connection-manager.ts: addreconnectAndJoin(name)so several parallel failing calls to a dropped server collapse into one reconnect (same as v2).agent/tool/index.ts: the wrapped MCP tool call now (1) rethrows server-answered errors (reconnecting would not change the answer), (2) probes liveness on ambiguous transport failures and retries once in place when the server is alive, and (3) on a provably dead transport drives one shared reconnect and retries on the fresh client. A server that flips tofailedkeeps its tools registered, so the next call can drive the reconnect and calls fail with the server's own error in the meantime instead of the tools silently vanishing from the tool list.Retries are at-least-once (same accepted trade-off as v2): if the transport died after the server processed the call but before the response arrived, the retry may duplicate side effects.
Deliberate boundaries, kept for parity with the v2 behavior: a call-time
RequestTimeoutis still classified as a server answer (no reconnect), and there is no negative caching — each call to a permanently dead server pays one bounded probe plus one failed reconnect attempt before erroring loudly.Tests:
test/mcp/tool-manager-mcp.test.tsgains unit coverage of the classification/retry matrix (dead transport → reconnect + retry on fresh client, transient blip → in-place retry, server-answered and malformed-result errors → rethrow, failed reconnect → combined error, aborted call → no reconnect, connection-closed → probe skipped, reconnect yielding no fresh client → original error,failedstatus keeps tools registered) plus a real-HTTP regression test for the issue scenario (server restart mid-session → the same tool on the same server answers).test/mcp/connection-manager.test.tsgainsreconnectAndJoindedup/unknown-server coverage.Checks run (Node 24.15.0, pnpm 10.33.0): full
@moonshot-ai/agent-coresuite — 4143 passed / 225 files;tsc --noEmitclean foragent-core,@moonshot-ai/kimi-code-sdk, and@moonshot-ai/kimi-code;oxlint --type-aware— 0 errors (only pre-existing repo warnings). The full diff was reviewed by a second model viakiro-cli(claude-opus-5); its actionable findings (untangled port-rebind race in the test helper, a missing type annotation, extra classification test cases) are incorporated.Testing limitations: the reproduction uses in-process streamable-HTTP MCP servers (SDK
StreamableHTTPServerTransport), not a specific production server; the reporter's exact engine (legacy flag vs. SDK embedding) is inferred from the symptoms rather than confirmed, and the wire-level "tool substitution" is addressed by keeping the failed server's tools registered and failing loudly — no evidence of a name-level dispatch bug was found (dispatch is by exact tool name).Checklist
gen-changesetsskill, or this PR needs no changeset.gen-docsskill, or this PR needs no doc update.