fix(core): prevent unhandled promise rejection on IDE MCP fetch failure#26484
fix(core): prevent unhandled promise rejection on IDE MCP fetch failure#26484SAY-5 wants to merge 1 commit intogoogle-gemini:mainfrom
Conversation
Register transport onerror/onclose handlers before calling client.connect so that fetch failures during the streamable-HTTP SSE listener (e.g. undici HeadersTimeoutError on a stalled IDE companion) are routed through the client's error handler instead of escaping as unhandled promise rejections. Fixes google-gemini#25751 Signed-off-by: SAY-5 <say.apm35@gmail.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an issue where IDE connection failures resulted in unhandled promise rejections. By reordering the initialization sequence to register transport-level error and close handlers before the client connection is established, the system can now gracefully capture and route connection-related errors through the existing state management logic. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request refactors the IdeClient to separate transport-level error handling from application-level notification handlers. It introduces registerTransportHandlers to be called before the client connects, ensuring that connection errors and SSE stream issues are properly captured. I have provided feedback regarding a potential race condition where notifications might be missed if handlers are registered after the connection is established, and a suggestion to improve error logging by capturing the actual error message instead of a static string.
| }, | ||
| ); | ||
| this.client.onerror = (_error) => { | ||
| const errorMessage = _error instanceof Error ? _error.message : `_error`; |
There was a problem hiding this comment.
The fallback value _error is currently a string literal, which prevents detailed error logging. It should be updated to capture the actual error details for debugging. Additionally, ensure the resulting string is trimmed and falls back to a non-empty default to avoid uninformative messages from whitespace-only strings.
| const errorMessage = _error instanceof Error ? _error.message : `_error`; | |
| const errorMessage = (_error instanceof Error ? _error.message : String(_error)).trim() || "Unknown error"; |
References
- When catching exceptions, log the detailed error for debugging instead of providing only a generic error message.
- When using an optional string with a fallback value, trim the optional string and use the fallback if the result is empty to avoid uninformative messages from whitespace-only strings.
| }; | ||
| } | ||
|
|
||
| private registerClientHandlers() { |
There was a problem hiding this comment.
Registering handlers after connection introduces a race condition where early notifications might be missed. Since the MCP SDK allows registering handlers before connection, moving registerClientHandlers() before client.connect(transport) ensures the startup sequence correctly handles incoming messages without needing additional state management for race conditions.
References
- Avoid redundant internal state management for race conditions if the application's startup sequence already guarantees the correct order of operations.
Summary
Register the MCP client's transport-level
onerror/onclosehandlers before callingclient.connect(transport)so that fetch failures from the streamable-HTTP SSE listener are routed through the client's error handler instead of escaping as unhandled promise rejections.Details
Issue #25751 reports a
CRITICAL: Unhandled Promise Rejection!withTypeError: fetch failedwhosecauseis undici'sHeadersTimeoutError (UND_ERR_HEADERS_TIMEOUT). The fetch in question is the long-lived SSE listening request opened byStreamableHTTPClientTransportagainst the local IDE companion athttp://127.0.0.1:<port>/mcp. When that stream's headers timer fires (e.g. the IDE companion stops responding), the rejection bubbles up through undici with no.catchin the chain.In
ide-client.ts, the existingregisterClientHandlers()was called afterawait client.connect(transport). That left a window — covering both the connect handshake and any post-connect SSE failure routed viatransport.onerror— in which the client had noonerror/onclosedefined, so the SDK had nowhere to forward the failure.This change splits handler registration in two:
registerTransportHandlers(): assignsclient.onerror/client.onclose, invoked beforeclient.connect(transport). These map to transport-level errors and route them throughsetState(Disconnected, ...).registerClientHandlers(): registers the notification handlers (which require an active session), invoked after connect, as before.Both
establishHttpConnectionandestablishStdioConnectioncall the new helper before connect.Related Issues
Fixes #25751
How to Validate
/ide enableconnected to a VS Code companion extension.CRITICAL: Unhandled Promise Rejection!panel is rendered withTypeError: fetch failed/HeadersTimeoutError.Disconnectedwith the standard "IDE connection error" message and the user is prompted to run/ide enableto reconnect.Pre-Merge Checklist