Skip to content

Commit af2f343

Browse files
committed
fix(mcp): keep the pooled connection warm on a request timeout
Empirically: our client's first connect to these servers consistently hangs to our 30s timeout, then a retry connects in ~100ms (the server answers initialize in ~120ms from a normal client — proven via A/B). Treating that request timeout as a dead connection (isDeadConnectionError) retired the pooled connection, so the next discovery reconnected and paid the stall again — a connect/stall/ reconnect churn loop, and the cause of the intermittent 'Failed to discover' / disconnected states. A streamable-HTTP request timeout aborts only that request's own POST stream; the session stays healthy for the next request. So stop poisoning the lease on a timeout (matches the MCP SDK, OpenCode, and LibreChat, none of which retire a connection on a request timeout). Only a real transport close/404/reset retires.
1 parent 94b2334 commit af2f343

2 files changed

Lines changed: 23 additions & 6 deletions

File tree

apps/sim/lib/mcp/service-pool.test.ts

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -171,6 +171,19 @@ describe('McpService connection reuse wiring', () => {
171171
expect(mockRelease).toHaveBeenCalledWith(false)
172172
})
173173

174+
it('keeps the pooled connection warm on a request timeout (does not retire the session)', async () => {
175+
// A streamable-HTTP request timeout aborts only that request's stream; the session stays
176+
// healthy for the next request, so a timeout must NOT poison the lease (matches every
177+
// production MCP client and avoids a connect/stall/reconnect churn loop).
178+
mockCallTool.mockRejectedValue(new Error('Request timed out'))
179+
180+
await expect(
181+
mcpService.executeTool(USER_ID, 'server-1', { name: 'do', arguments: {} }, WORKSPACE_ID)
182+
).rejects.toThrow()
183+
184+
expect(mockRelease).not.toHaveBeenCalledWith(true)
185+
})
186+
174187
it('poisons the lease on an auth failure so a rotated credential is re-resolved', async () => {
175188
mockCallTool.mockRejectedValue(new UnauthorizedError('token rejected'))
176189

apps/sim/lib/mcp/service.ts

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -99,9 +99,16 @@ function isTimeoutError(error: unknown): boolean {
9999
/**
100100
* A pooled connection is dead and must be retired so the caller's retry rebuilds
101101
* fresh: a stale session (400/404), an auth failure (401 — a rotated/revoked
102-
* credential; the rebuild re-resolves it), a closed transport, a timeout (no
103-
* response — possibly wedged), or a reset socket. Benign tool/consent errors and
104-
* healthy upstream responses (429/5xx) keep the connection warm.
102+
* credential; the rebuild re-resolves it), a closed transport, or a reset socket.
103+
*
104+
* A request timeout is deliberately NOT dead: streamable-HTTP aborts only that
105+
* request's own POST stream, leaving the session healthy for the next request, so
106+
* every production MCP client (SDK, OpenCode, LibreChat) rejects the request and
107+
* keeps the connection rather than tearing it down. Retiring on a timeout instead
108+
* forced a full reconnect on the next discovery — and a fresh connect can stall on
109+
* our end far longer than a warm request — turning one slow response into a
110+
* connect/stall/reconnect churn loop. Benign tool/consent errors and healthy
111+
* upstream responses (429/5xx) also keep the connection warm.
105112
*/
106113
function isDeadConnectionError(error: unknown): boolean {
107114
if (error instanceof UnauthorizedError) {
@@ -113,9 +120,6 @@ function isDeadConnectionError(error: unknown): boolean {
113120
if (error instanceof McpError && error.code === ErrorCode.ConnectionClosed) {
114121
return true
115122
}
116-
if (isTimeoutError(error)) {
117-
return true
118-
}
119123
const message = getErrorMessage(error, '').toLowerCase()
120124
return (
121125
message.includes('econnreset') ||

0 commit comments

Comments
 (0)