fix(mcp-client): prevent eager tool wipe on network timeout - #27383
fix(mcp-client): prevent eager tool wipe on network timeout#27383luisfelipe-alt wants to merge 11 commits into
Conversation
Implement atomic update in refreshTools to retain existing tools when discoverTools fails due to network drops, resolving the 'tool not found' error.
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 MCP tools were prematurely removed from the registry during transient network drops or failed refresh attempts. By implementing an atomic update pattern and removing eager cleanup logic, the system now maintains the last known good state of tools, ensuring continuity for the LLM during connection instability. 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 the 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 counterproductive. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces changes to the MCP client to prevent the removal of tools, prompts, and resources from the registry when a server disconnects or when a tool refresh returns an empty list. It includes a new reproduction test file and updates existing tests to reflect these changes. However, the feedback indicates that removing the registry cleanup from the disconnect() method is a regression, as explicit disconnections should clear associated tools to avoid stale states. Additionally, the conditional check for newTools.length > 0 during tool refresh is criticized for preventing legitimate updates to an empty tool list; a more robust approach would be to propagate errors from the discovery layer to distinguish between network failures and empty tool sets.
gemini-cli
Bot
added
the
status/need-issue
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Replaces conditional length check with proper error propagation. Properly distinguishes between network failures (which skip updates) and successful empty tool discovery.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a reproduction test suite for MCP tool disconnection issues and updates the discoverTools function to rethrow errors on failure, ensuring atomic refreshes. The reviewer pointed out that the reproduction test's assertion is weak because it uses a conditional check that could pass silently if the status is success or if the array is empty, and suggested explicitly asserting that the result is defined and successful.
| error, | ||
| mcpServerName, | ||
| ); | ||
| throw error; |
There was a problem hiding this comment.
The following is an explanation how the "throw" is fixing the issue:
Scenario: The user asks a complex question. While the LLM is "thinking" (which takes 30 seconds), the MCP server connection drops, triggering a background refresh.
┌──────┬──────────────────────────────────────┬───────────────────────────────────────┬────────────────────────────────────────────────────────────────────┐
│ Step │ Action │ State BEFORE Fix (Bug) │ State AFTER Fix (PR Solution) │
├──────┼──────────────────────────────────────┼───────────────────────────────────────┼────────────────────────────────────────────────────────────────────┤
│ 1 │ LLM starts generating response. │ Tool Registry has [Tool_A, Tool_B]. │ Tool Registry has [Tool_A, Tool_B]. │
│ 2 │ Background refreshTools starts. │ Enters try block. │ Enters try block. │
│ 3 │ discoverTools is called. │ Fails due to connection drop. │ Fails due to connection drop. │
│ 4 │ discoverTools handles error. │ Logs error, returns []. │ Logs error, throws error. │
│ 5 │ refreshTools reacts. │ Receives []. Continues try block. │ Catches error. Jumps to catch block. │
│ 6 │ Registry Update. │ Executes removeMcpToolsByServer. │ Skips removeMcpToolsByServer. │
│ 7 │ Registry State. │ Registry is now EMPTY. │ Registry still has [Tool_A, Tool_B]. │
│ 8 │ LLM finishes thinking, calls Tool_A. │ Scheduler looks for Tool_A. Fails. │ Scheduler looks for Tool_A. Finds it. │
│ 9 │ Final Outcome │ ❌ CLI crashes with "Tool not found". │ ✅ Tool executes (or fails gracefully with "Server Disconnected"). │
└──────┴──────────────────────────────────────┴───────────────────────────────────────┴────────────────────────────────────────────────────────────────────┘
|
|
||
| // Should try to remove tools | ||
| expect(mockedToolRegistry.removeMcpToolsByServer).toHaveBeenCalled(); | ||
| // Should NOT try to remove tools because discovery failed (atomic refresh) |
There was a problem hiding this comment.
The following is an explanation about the fix and changes in to the testing:
┌───────────────────────────────────┬─────────────────────────────────────────────────────────┬─────────────────────────────────────────────────────────────────────────────────────────────────────────┐
│ Test Name │ Scenario Tested │ How the PR Change Validates It │
├───────────────────────────────────┼─────────────────────────────────────────────────────────┼─────────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ "should handle errors during tool │ A background refresh is triggered, but the network call │ It mocks listTools to reject with an Error. It then asserts that removeMcpToolsByServer is NOT called. │
│ refresh gracefully" │ (listTools) fails. │ This confirms the throw successfully aborted the wipe. │
└───────────────────────────────────┴─────────────────────────────────────────────────────────┴─────────────────────────────────────────────────────────────────────────────────────────────────────────┘
| expect(mockedToolRegistry.removeMcpToolsByServer).toHaveBeenCalledOnce(); | ||
| expect(mockedPromptRegistry.removePromptsByServer).toHaveBeenCalledOnce(); | ||
| expect(resourceRegistry.removeResourcesByServer).toHaveBeenCalledOnce(); | ||
| expect(mockedToolRegistry.removeMcpToolsByServer).toHaveBeenCalledWith( |
There was a problem hiding this comment.
The following is an explanation about the fix and changes into the testing:
┌─────────────────────────────────────┬─────────────────────────────────────────────────────────┬─────────────────────────────────────────────────────────────────────────────────────────────────────────┐
│ Exact Test Name │ Scenario Tested │ How the PR Change Validates It │
├─────────────────────────────────────┼─────────────────────────────────────────────────────────┼─────────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ "should remove tools and prompts on │ The system calls client.disconnect() (e.g., closing the │ It calls the disconnect() method and asserts that removeMcpToolsByServer IS called. This proves the fix │
│ disconnect" │ app or disabling a plugin). │ doesn't break the standard cleanup process. │
└─────────────────────────────────────┴─────────────────────────────────────────────────────────┴─────────────────────────────────────────────────────────────────────────────────────────────────────────┘
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request modifies discoverTools in packages/core/src/tools/mcp-client.ts to rethrow errors caught during tool discovery instead of returning an empty array. Additionally, it updates the corresponding unit tests in packages/core/src/tools/mcp-client.test.ts to verify that tools are not removed when discovery fails, and ensures that registries are cleaned up with the correct server name on disconnect. I have no feedback to provide as there are no review comments.
|
✅ 70 tests passed successfully on gemini-3-flash-preview. 🧠 Model Steering GuidanceThis PR modifies files that affect the model's behavior (prompts, tools, or instructions).
This is an automated guidance message triggered by steering logic signatures. |
|
Hi there! Thank you for your interest in contributing to Gemini CLI. To ensure we maintain high code quality and focus on our prioritized roadmap, we only guarantee review and consideration of pull requests for issues that are explicitly labeled as 'help wanted'. This PR will be closed in 7 days if it remains without that designation. We encourage you to find and contribute to existing 'help wanted' issues in our backlog! Thank you for your understanding. |
🛑 Action Required: Evaluation ApprovalSteering changes have been detected in this PR. To prevent regressions, a maintainer must approve the evaluation run before this PR can be merged. Maintainers:
Once approved, the evaluation results will be posted here automatically. |
galdawave
left a comment
galdawave
left a comment
There was a problem hiding this comment.
Intent Summary: This PR aims to prevent the "tool not found" error during transient network drops by aborting the tool registry refresh when discoverTools() fails, retaining the last known good state.
🚨 Critical Concerns (P0/P1)
Action required before merging.
packages/core/src/tools/mcp-client.ts:287: The PR description explicitly claims to have "Removed the eager registry wipe indisconnect()", but this change is completely missing from the diff. Thedisconnect()method still loops over registries and callsremoveMcpToolsByServer(). If a transient network drop triggers a full disconnect/reconnect lifecycle, tools will still be lost. You must either remove the wipe fromdisconnect()(and update the tests) or clarify the connection lifecycle.
🧹 Refactoring & Nits (P2/P3)
Recommended improvements.
packages/core/src/tools/mcp-client.test.ts: The update from.toHaveBeenCalledOnce()to.toHaveBeenCalledWith('test-server')is a good tightening of the test assertions. No changes needed here unlessdisconnect()is updated.
📝 Metadata Review
Feedback on PR description or commit message clarity.
- Out of Sync Description: The PR description claims the atomic update uses a length check (
if (newTools.length > 0)). The actual implementation correctly uses exception propagation (throw errorindiscoverTools). While the implemented approach is architecturally superior (it correctly allows a server to intentionally remove all tools), the PR description is misleading and needs to be updated to reflect the final code.
d713d34 to
9dab832
Compare
4 participants
Implement atomic update in refreshTools to retain existing tools when discoverTools fails due to network drops, resolving the 'tool not found' error.
Summary
Fixed the "tool not found" error during transient network drops by implementing an atomic update pattern for MCP tools. This ensures that the tool registry remains populated with the last known good state if a refresh attempt fails or returns no tools due to a connection issue.
Details
Related Issues
How to Validate
Run the following commands to verify the fix and ensure no regressions:
Pre-Merge Checklist