fix: apply timeout configuration to all MCP client requests #7837
Conversation
- Added timeout parameter to readResource method - Added timeout parameter to fetchToolsList method - Added timeout parameter to fetchResourcesList method - Added timeout parameter to fetchResourceTemplatesList method Previously, only the callTool method was using the configured timeout, causing other MCP operations to use the default 30-second timeout instead of the user-configured value. Fixes #7836
- Created getTimeoutFromConnection helper method to reduce code duplication - Applied helper method to all 5 locations where timeout is extracted - Improves maintainability and reduces repetitive code
dosubot
bot
added
size:M
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
| const parsedConfig = ServerConfigSchema.parse(JSON.parse(connection.server.config)) | ||
| return (parsedConfig.timeout ?? 60) * 1000 | ||
| } catch (error) { | ||
| console.error("Failed to parse server config for timeout:", error) |
There was a problem hiding this comment.
Is this error handling approach intentional? The helper logs errors but doesn't propagate them, defaulting to 60 seconds instead. While this provides resilience, callers might benefit from knowing when config parsing fails. Consider whether this silent fallback is the desired behavior for all use cases.
| } catch (error) { | ||
| console.error("Failed to parse server config for timeout:", error) | ||
| // Default to 60 seconds if parsing fails | ||
| return 60 * 1000 |
There was a problem hiding this comment.
The default timeout of 60 seconds appears here and in the original callTool implementation. Could we extract this to a named constant like DEFAULT_TIMEOUT_SECONDS = 60 at the class level? This would make it easier to maintain and modify the default timeout across the codebase.
| * @param connection The MCP connection to get timeout from | ||
| * @returns The timeout in milliseconds, defaults to 60000ms if parsing fails | ||
| */ | ||
| private getTimeoutFromConnection(connection: McpConnection): number { |
There was a problem hiding this comment.
Consider adding explicit return type annotation for better type safety:
Although TypeScript can infer it, explicit typing improves code clarity and catches potential type mismatches earlier.
| // Default to 60 seconds if parsing fails | ||
| return 60 * 1000 | ||
| } | ||
| } |
There was a problem hiding this comment.
Good refactoring! This helper method effectively reduces code duplication across all MCP client request methods. Consider adding unit tests for this helper to ensure it handles edge cases like invalid JSON, missing timeout field, and various error scenarios.
hannesrudolph
added
the
Issue/PR - Triage
|
Issue needs info |
Description
This PR fixes an issue where MCP tool response times that exceed 1 minute were timing out at 30 seconds instead of respecting the user-configured timeout value.
Problem
The user reported in #7836 that their MCP tool response time exceeds 1 minute, but when they set a 1-minute timeout in the configuration, it was still timing out at 30 seconds. This was happening because the timeout configuration was only being applied to the
callToolmethod, while other MCP operations (readResource,fetchToolsList,fetchResourcesList,fetchResourceTemplatesList) were using the default 30-second timeout from the MCP SDK.Solution
Added timeout parameter to all MCP client requests:
readResourcemethodfetchToolsListmethodfetchResourcesListmethodfetchResourceTemplatesListmethodcallToolmethod (already had it)Refactored timeout extraction logic:
getTimeoutFromConnectionto reduce code duplicationChanges
src/services/mcp/McpHub.tsto ensure all MCP client requests respect the configured timeoutTesting
Fixes #7836
Important
Fixes timeout issue in MCP client requests by applying user-configured timeout to all relevant methods in
McpHub.ts.readResource,fetchToolsList,fetchResourcesList,fetchResourceTemplatesList,callTool) respect user-configured timeout.getTimeoutFromConnectionhelper method inMcpHub.tsto extract timeout configuration.getTimeoutFromConnectionto all relevant methods to reduce code duplication.This description was created by
for c76fbde. You can customize this summary. It will automatically update as commits are pushed.