Skip to content

fix(mcp-client): prevent eager tool wipe on network timeout #27383

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.

Sign up for GitHub

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

Closed
luisfelipe-alt wants to merge 11 commits into from
Closed

fix(mcp-client): prevent eager tool wipe on network timeout #27383

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
50f36a2
fix(mcp-client): prevent eager tool wipe on network timeout
luisfelipe-alt May 22, 2026
17b222a
Update packages/core/src/tools/mcp-client.ts
luisfelipe-alt May 22, 2026
fae56f3
fix(mcp-client): implement robust error handling in tool discovery
luisfelipe-alt May 22, 2026
7f4055d
Merge branch 'main' into bugfix/WT-102at2_488476022
luisfelipe-alt May 22, 2026
520363f
test(mcp-client): remove reproduction test artifact
luisfelipe-alt May 26, 2026
d2d3f65
Merge branch 'main' into bugfix/WT-102at2_488476022
luisfelipe-alt May 26, 2026
47d23e9
Merge branch 'main' into bugfix/WT-102at2_488476022
luisfelipe-alt May 26, 2026
394fbbf
Merge branch 'main' into bugfix/WT-102at2_488476022
luisfelipe-alt May 27, 2026
7c7b964
Merge branch 'main' into bugfix/WT-102at2_488476022
luisfelipe-alt May 27, 2026
59e74d0
Merge branch 'main' into bugfix/WT-102at2_488476022
galdawave Jun 1, 2026
9dab832
fix(core): remove eager registry wipe in disconnect
luisfelipe-alt Jun 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 11 additions & 5 deletions packages/core/src/tools/mcp-client.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1244,9 +1244,15 @@ describe('mcp-client', () => {
await client.disconnect();

expect(mockedClient.close).toHaveBeenCalledOnce();
expect(mockedToolRegistry.removeMcpToolsByServer).toHaveBeenCalledOnce();
expect(mockedPromptRegistry.removePromptsByServer).toHaveBeenCalledOnce();
expect(resourceRegistry.removeResourcesByServer).toHaveBeenCalledOnce();
expect(mockedToolRegistry.removeMcpToolsByServer).toHaveBeenCalledWith(

@luisfelipe-alt luisfelipe-alt May 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.                                                             │
  └─────────────────────────────────────┴─────────────────────────────────────────────────────────┴─────────────────────────────────────────────────────────────────────────────────────────────────────────┘

'test-server',
);
expect(mockedPromptRegistry.removePromptsByServer).toHaveBeenCalledWith(
'test-server',
);
expect(resourceRegistry.removeResourcesByServer).toHaveBeenCalledWith(
'test-server',
);
});
});

Expand Down Expand Up @@ -1570,8 +1576,8 @@ describe('mcp-client', () => {
// Trigger notification - should fail internally but catch the error
await notificationCallback();

// Should try to remove tools
expect(mockedToolRegistry.removeMcpToolsByServer).toHaveBeenCalled();
// Should NOT try to remove tools because discovery failed (atomic refresh)

@luisfelipe-alt luisfelipe-alt May 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).not.toHaveBeenCalled();

// Should NOT emit success feedback
expect(coreEvents.emitFeedback).not.toHaveBeenCalledWith(
Expand Down
1 change: 1 addition & 0 deletions packages/core/src/tools/mcp-client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1404,6 +1404,7 @@ export async function discoverTools(
error,
mcpServerName,
);
throw error;

@luisfelipe-alt luisfelipe-alt May 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"). │
  └──────┴──────────────────────────────────────┴───────────────────────────────────────┴────────────────────────────────────────────────────────────────────┘

}
return [];
}
Expand Down
Loading