Repository navigation
fix(mcp): reconnect a session's failed MCP server after a test of it connects - #1684
Conversation
…ry connects Review of agegr#1684. Retrying every failed or disconnected server at every prompt cost a session with a server that stays broken, at each prompt: a new spawn or request, the extension's "MCP servers need attention" warning again, and for a `direct` server a wait of up to its timeout or 10 s. ADR 0006 and the pi CLI leave a failed server alone for these reasons, so the prompt no longer retries. What stays is the report's actual gap: a Settings › MCP Test that connects now reaches the sessions holding that entry as failed. - Only `failed`, never `disconnected`: the extension reconnects a dropped server at its next call with its tools registered; unregistering hid them and dropped the tools tool_search had loaded. - The retry keeps the loaded tools, as an idle release does: a registration that failed after it had been ready still has them. - Matched by status key and configKey, not by name: a test of another project's or another file's server of the name retried unrelated sessions. - During a run the registration is only marked, and the next prompt's sync replaces it: a sync applies every pending mcp.json change and replaced other servers mid-run. - The host subscribes at session_start, not in its constructor: an instance whose session was never built stayed on globalThis. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
agegr
left a comment
There was a problem hiding this comment.
Approved with changes I pushed to this branch (df7cae7c, plus merges of main). The gap you found is real, but I kept only half of the fix; the merge itself waits for a second look because it touches ADR 0006.
The bug. Confirmed on main with a real session: an HTTP server whose port was closed at the first prompt stays failed for every later prompt, also after the server comes up and after a Settings › MCP Test that connects. What recovers it today is /mcp reconnect <name>, a changed entry, a reload or the 10-minute idle release. The warning says "Run /mcp to fix", and in Pi Web /mcp opens Settings, where a Test that connects changed nothing for the session. That part is worth fixing.
Removed: the retry at every prompt. ADR 0006 leaves a failed server alone on purpose (so does the pi CLI: its before_agent_start only waits), and the measurements show why. With servers that stay broken:
| main | PR before | |
|---|---|---|
| "MCP servers need attention" warnings over 3 prompts (2 broken stdio servers) | 2 / 2 / 2 | 2 / 4 / 6 |
wait before each prompt, a direct server that never answers initialize (timeout: 2) |
2019 / 2 / 1 ms | 2007 / 2009 / 2012 ms |
a tool the model loaded through tool_search from a deferred server that dropped |
still there | gone at the next prompt |
The last row is the disconnected case: the extension reconnects such a server at its next call with its tools still registered, so unregistering it only hides them.
Kept and tightened: a Test that connects reaches the session.
- Only
failed, neverdisconnected. - Matched by the status key (scope, file, name) and
configKey, not by name: a Test of another project's server of the same name retried unrelated sessions. - The registration is only marked (
ConnectAttempt.retry); the sync runs at once while the session is idle, otherwise at the next prompt. A sync applies every pendingmcp.jsonchange, so the PR's version replaced other servers in the middle of a run. - The retry keeps the tools the model had loaded, as an idle release does.
- The host subscribes at its first
session_startand not in its constructor: an instance whose session was never built stayed onglobalThis.
Tests: five unit tests replace the PR's one, plus an integration test through the real POST /api/mcp/test route with a real session (the server turns ready and offers its tool). Each guard above fails its test when removed. tsc, lint and the full npm test are clean on a merge with main (3515 tests); the two host test files also pass on Node 22.
Docs: docs/agents/mcp-runtime.md, mcp-test-sign-in.md, and one sentence of ADR 0006 (a failed server is not retried by a prompt; a Test that connects is the exception).
Not checked: the Settings panel in a browser, and a project-scope entry through the real route (unit tests only). I retitled the PR to what is left.
Problem
When an MCP server fails its initial handshake during a session's prompt (e.g. a transient network hiccup or a local server not yet started):
registerToolsnever ran), so the model cannot call any tool on that server to trigger the SDK's lazygetClient()reconnect.HostInstance.sync()(lib/mcp-host.ts) checksif (wanted && canonicalJson(wanted.config) === attempt.configKey) continue;and skips the server because its config did not change, even thoughattempt.state === "failed"."connected"inlib/mcp-status.ts, but the live session'sMcpHostkeeps its stale"failed"attempt until the 10-minutePI_WEB_MCP_IDLE_MStimer releases all servers.Reproduction on
mainMCP <name> · failed.connected), but returning to the session's Debug panel still showsfailed.sync()skips the server and it remains stuck infaileduntil the 10-minute idle timer fires.Fix
lib/mcp-host.ts: accept{ retryFailed?: boolean }insync(), passed bywaitForServers()(prepareForPrompt()). WhenretryFailedis true and an existing attempt is in"failed"or"disconnected"state, unregister and re-register it so the new prompt reconnects cleanly (whilebefore_agent_start's secondarysync()keepsretryFailed: falseso a persistently broken server is not retried twice within a single prompt).lib/mcp-status.ts&lib/mcp-host.ts: notify activeHostInstances whenrecordMcpStatus()records a successful test (origin === "test" && state === "connected"). Any active host holding that server in"failed"or"disconnected"state immediately triggerssync({ retryFailed: true }).Tests
lib/mcp-host.test.mjs: a server that failed on the first prompt is re-registered and connects on the next prompt; a successful test recorded inlib/mcp-status.tstriggers an immediate reconnect on a session holding that server infailedstate.lib/mcp-status.test.mjs,lib/mcp-test.test.mjs,lib/mcp-host.test.mjs, andlib/mcp-host.integration.test.mjspass.