-
Notifications
You must be signed in to change notification settings - Fork 10.9k
fix(core): handle unhandled promise rejection in mcp-client-manager #14701
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(core): handle unhandled promise rejection in mcp-client-manager #14701
Conversation
- Add outer try-catch block to catch initialization errors - Connect async IIFE rejection to Promise reject handler - Add tests for error scenarios during client initialization - Prevents unhandled promise rejection when McpClient constructor throws - Ensures one server failure doesn't stop other servers from starting Fixes google-gemini#14700
Summary of ChangesHello @kamja44, 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 a critical structural flaw in the 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
I also left this explanation in the ISSUE, but I am attaching it here in the PR comments as well to clarify why this is not a duplicate. Why this is NOT a duplicate of #8365 #8365 (Closed): Issue: MCP timeout error during user confirmation Location: gemini.mjs timeout handler Cause: MCP error -32001: Request timed out Status: Already fixed and closed This PR (#14700): Issue: Structural problem in promise error handling during MCP initialization Location: packages/core/src/tools/mcp-client-manager.ts (addOrUpdateServer method) Cause: Unused _reject parameter + missing .catch() in the async IIFE Status: Preventive fix to avoid potential crashes Key differences: Different code files and methods Different root causes (timeout vs improper promise error handling) Different scenarios (user confirmation timeout vs client initialization failures) This PR is a preventive robustness improvement, not a fix for a previously reported crash This PR addresses a structural issue that may lead to similar symptoms but has an entirely different root cause. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request effectively addresses an unhandled promise rejection within McpClientManager by introducing proper error handling inside the promise executor. The new tests also correctly validate the fix for server initialization failures. However, my review has identified a critical side effect of the change: it introduces a new path for promise rejection that leads to another unhandled promise rejection later in the same function. My comment provides a detailed explanation of this issue and a recommended solution to make the fix complete.
Address review feedback: Change .then() to .finally() on line 238 to ensure cleanup logic runs regardless of promise resolution, preventing unhandled rejection when currentDiscoveryPromise rejects. - Replace .then((_) => ...) with void finally(() => ...) - Remove eslint-disable comment (no longer needed) - Ensures discoveryState cleanup happens on both success and failure
|
hi, sorry that no one has reviewed your PR! I can take a look at this if you are still working on it! |
Hi, thanks so much for taking a look — I really appreciate it! |
| const currentPromise = this.discoveryPromise; | ||
| // eslint-disable-next-line @typescript-eslint/no-floating-promises | ||
| currentPromise.then((_) => { | ||
| void currentPromise.finally(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we attach a catch to this promise?
void currentPromise
.finally(() => {
if (currentPromise === this.discoveryPromise) {
this.discoveryPromise = undefined;
this.discoveryState = MCPDiscoveryState.COMPLETED;
}
})
.catch(() => {}); // Prevents unhandled rejection from the .finally branchThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the .catch() to handle any errors from the finally block. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have another unhandled promise on line 234
| this.discoveryPromise = this.discoveryPromise.then( |
If the promise gets rejected there's nothing to handle this, so can we add the catch as well?
if (this.discoveryPromise) {
// Ensure the next discovery starts regardless of the previous one's success/failure
this.discoveryPromise = this.discoveryPromise
.catch(() => {})
.then(() => currentDiscoveryPromise);
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without the .catch(), the next discovery wouldn't run if the previous one failed. Fixed by adding .catch() before .then(). Thanks for catching this important issue!
… chain - Add catch to finally chain to prevent unhandled rejections - Add catch before then to ensure discovery continues on previous failure
Adib234
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this! LGTM 🚀
…on in mcp-client-manager (google-gemini#14701)
Fixes #14700
Summary
Fixes a structural issue in
McpClientManager.addOrUpdateServer()that causes unhandled promise rejections during MCP server initialization. The Promise constructor received an unused_rejectparameter, and the async IIFE lacked proper error handling, causing crashes when errors occurred in the McpClient constructor or disconnect method.Details
Root cause: The current implementation creates a Promise with
_rejectparameter but never uses it. The async IIFE has no.catch()handler, so errors thrown before the try block become unhandled rejections.Changes made:
_rejecttorejectand properly connected it.catch(reject)to async IIFE to handle any uncaught errorscoreEvents.emitFeedbackFiles modified:
packages/core/src/tools/mcp-client-manager.ts(lines 162-220)packages/core/src/tools/mcp-client-manager.test.ts(added 2 test cases)Note: This is NOT a duplicate of #8365. That issue was about MCP timeout errors in
gemini.mjsand is already closed. This PR fixes a different structural problem inmcp-client-manager.tsthat could cause unhandled rejections during initialization.Related Issues
Fixes #14700
How to Validate