fix(mcp): harden HTTP transport and improve disconnect diagnostics - #356
alvarolordelo wants to merge 1 commit into
Conversation
Refs Waishnav#297. Related to Waishnav#140. Intermittent MCP connection failures can occur while the DevSpace process remains running and both the local endpoint and public tunnel remain reachable. This draft addresses server-side transport and authentication weaknesses encountered during that investigation; it does not claim to resolve every disconnect in Waishnav#297. The listener uses a five-minute keep-alive timeout and a 305-second headers timeout to reduce premature origin socket closure between requests through a reverse proxy. Bearer authentication runs directly as Express middleware, removing a Promise wrapper that could remain unresolved when authentication was rejected. Finite MCP responses use JSON mode and are buffered before sending headers, allowing an explicit Content-Length and preventing body-read failures from producing an already-started successful response. subscriptions/listen retains SSE. The adapter adds a 20-second timeout while awaiting handler.fetch and propagates client disconnection through an abort signal. Loopback-bound servers automatically trust loopback proxies. Logging records request start, completion, premature closure, and MCP protocol/method metadata to help distinguish requests reaching DevSpace from failures earlier in the path. Validation was rerun on this dedicated branch on Windows: pnpm typecheck, pnpm build, and git diff --check passed. The server, server-oauth, and server-shutdown test files passed all 29 tests with no failures or skips. Coverage includes modern/legacy HTTP MCP, OAuth resource enforcement, proxy configuration, listener timeouts, Content-Length, and shutdown behavior. These checks do not establish sustained reliability through the real ChatGPT-to-Funnel path. This is a draft because JSON mode drops intermediate notifications for ordinary calls, the buffering/timeout policy needs maintainer review, and intermittent connector failures were still observed during the earlier investigation. Some failures had no corresponding incoming server request. The separate yield-parameter mismatch in Waishnav#297 is not addressed, and the Windows process-cleanup fallback from our fork is excluded.
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis change introduces failures in Confidence Score: 1/5Not safe to merge: the MCP transport can terminate healthy subscriptions, report valid tool work as failed, lose progress updates, and retain stalled streaming handlers. Four independently reproduced user-visible failures affect MCP request handling and streaming behavior. Files Needing Attention: src/server.ts, especially the handler timeout race at lines 151-164, SSE write backpressure handling at lines 175-185, and MCP response mode at line 978.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const timer = setTimeout(() => { | ||
| abort.abort(); | ||
| reject(new Error(`MCP handler timed out after ${DEVSPACE_MCP_HANDLER_TIMEOUT_MS}ms`)); | ||
| }, DEVSPACE_MCP_HANDLER_TIMEOUT_MS); | ||
| timer.unref?.(); | ||
| abort.signal.addEventListener("abort", () => clearTimeout(timer), { once: true }); |
There was a problem hiding this comment.
handler.fetch can return a healthy subscriptions/listen SSE response before this timer expires, but the timer is never cleared. It subsequently aborts the request signal at 20 seconds, closing an active subscription even while the client remains connected and events continue to flow. This must be corrected before merging.
Artifacts
Focused SSE timeout reproduction script
- The authored runtime script starts a real Express endpoint using the repository handler and compares the unmodified behavior with an isolated timer-cleanup variant; it demonstrates the timeout remains armed.
Healthy SSE stream before timeout cleanup
- Captured command output from the unchanged handler shows HTTP 200 OK and an SSE event before the request signal aborts at 21,599 ms; the timeout aborts a healthy subscription.
Healthy SSE stream after isolated timeout cleanup
- Captured command output from the isolated cleanup comparison shows HTTP 200 OK and no request-signal abort during a 22,431 ms observation; clearing the settled race timer prevents the abort.
| const response = await Promise.race([ | ||
| handler.fetch(webRequest, { | ||
| ...(req.auth !== undefined ? { authInfo: req.auth } : {}), | ||
| ...(parsedBody !== undefined ? { parsedBody } : {}), | ||
| }), | ||
| new Promise<never>((_, reject) => { | ||
| const timer = setTimeout(() => { | ||
| abort.abort(); | ||
| reject(new Error(`MCP handler timed out after ${DEVSPACE_MCP_HANDLER_TIMEOUT_MS}ms`)); | ||
| }, DEVSPACE_MCP_HANDLER_TIMEOUT_MS); |
There was a problem hiding this comment.
This fixed 20-second deadline applies to every MCP request, including bash calls that validly run longer: the tool accepts a 30-second default and up to 300 seconds. A 22-second call now returns HTTP 500 at the transport deadline, while its shell process continues and completes after the client has received the failure. This must be corrected before merging.
Artifacts
Focused SSE timeout reproduction script
- The authored runtime script starts a real Express endpoint using the repository handler and compares the unmodified behavior with an isolated timer-cleanup variant; it demonstrates the timeout remains armed.
Healthy SSE stream before timeout cleanup
- Captured command output from the unchanged handler shows HTTP 200 OK and an SSE event before the request signal aborts at 21,599 ms; the timeout aborts a healthy subscription.
Healthy SSE stream after isolated timeout cleanup
- Captured command output from the isolated cleanup comparison shows HTTP 200 OK and no request-signal abort during a 22,431 ms observation; clearing the settled race timer prevents the abort.
Focused MCP timeout runtime reproducer
- This authored TypeScript script starts the server, obtains OAuth credentials, opens a workspace, invokes the 22-second bash command through `/mcp`, and checks its completion marker; it reproduces the HTTP timeout behavior.
MCP bash response before the handler timeout change
- Executed against `HEAD^`, this capture shows the real 22-second bash MCP request returned HTTP 200 OK and completed normally.
MCP bash response after the handler timeout change
- Executed against the changed checkout, this capture shows the same request returned HTTP 500 Internal Server Error at 20 seconds while the bash completion marker still appeared, confirming the defect.
| return adapter.server; | ||
| }, { | ||
| legacy: "stateless", | ||
| responseMode: "json", |
There was a problem hiding this comment.
Using JSON response mode for ordinary tool calls omits adapter-generated non-terminal notifications such as notifications/progress; clients receive only the terminal result. Clients cannot reliably show progress or other pre-result updates for longer operations. This must be corrected before merging.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Artifacts
Focused MCP progress-notification reproduction script
- The executable TypeScript reproduction creates the real modern adapter and handler, sends an ordinary tool request, and compares response modes; it isolates notification delivery behavior.
Default response mode retains progress notification
- Running the focused ordinary tool call with the default handler mode returned HTTP 200 OK as SSE with both the progress notification and terminal result, establishing the retained-notification baseline.
JSON response mode drops progress notification
- Running the same ordinary tool call with responseMode json returned HTTP 200 OK as JSON with the terminal result but no progress notification, confirming the finding.
| if (!res.write(chunk)) { | ||
| await new Promise<void>((resolve) => res.once("drain", resolve)); |
There was a problem hiding this comment.
When an SSE write is backpressured, this waits only for drain. If the client disconnects first, the response closes and the request signal aborts, but neither settles this wait, leaving the request handler and stream resources pending. Repeated disconnected backpressured streams can accumulate stalled handlers. This must be corrected before merging.
Artifacts
Focused Node HTTP SSE backpressure reproduction script
- A real HTTP server/client script that forces a false response write, records the endpoint status, aborts the client, and compares await-only-drain with a close-aware control; it demonstrates the missing disconnect wake-up.
Disconnected backpressured SSE handler remains pending
- Captured execution of the await-only-drain branch against GET /mcp, showing HTTP 200 OK, a false write result, an abort signal, and an unsettled handler; the handler leak is reproduced.
Close-aware SSE wait settles after disconnect
- Captured execution of the close-aware control against the same GET /mcp response, showing HTTP 200 OK and a settled handler after disconnect; observing close unblocks the wait.
1 participant
Refs #297. Related to #140.
Intermittent MCP connection failures can occur while the DevSpace process remains running and both the local endpoint and public tunnel remain reachable. This draft addresses server-side transport and authentication weaknesses encountered during that investigation; it does not claim to resolve every disconnect in #297.
The listener uses a five-minute keep-alive timeout and a 305-second headers timeout to reduce premature origin socket closure between requests through a reverse proxy. Bearer authentication runs directly as Express middleware, removing a Promise wrapper that could remain unresolved when authentication was rejected.
Finite MCP responses use JSON mode and are buffered before sending headers, allowing an explicit Content-Length and preventing body-read failures from producing an already-started successful response. subscriptions/listen retains SSE. The adapter adds a 20-second timeout while awaiting handler.fetch and propagates client disconnection through an abort signal.
Loopback-bound servers automatically trust loopback proxies. Logging records request start, completion, premature closure, and MCP protocol/method metadata to help distinguish requests reaching DevSpace from failures earlier in the path.
Validation was rerun on this dedicated branch on Windows: pnpm typecheck, pnpm build, and git diff --check passed. The server, server-oauth, and server-shutdown test files passed all 29 tests with no failures or skips. Coverage includes modern/legacy HTTP MCP, OAuth resource enforcement, proxy configuration, listener timeouts, Content-Length, and shutdown behavior. These checks do not establish sustained reliability through the real ChatGPT-to-Funnel path.
This is a draft because JSON mode drops intermediate notifications for ordinary calls, the buffering/timeout policy needs maintainer review, and intermittent connector failures were still observed during the earlier investigation. Some failures had no corresponding incoming server request. The separate yield-parameter mismatch in #297 is not addressed, and the Windows process-cleanup fallback from our fork is excluded.