Skip to content

fix: apply timeout configuration to all MCP client requests #7837

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
wants to merge 2 commits into from
Closed

fix: apply timeout configuration to all MCP client requests #7837

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b572522
fix: apply timeout configuration to all MCP client requests
roomote Sep 10, 2025
c76fbde
refactor: extract timeout configuration logic into helper method
roomote Sep 10, 2025
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
48 changes: 36 additions & 12 deletions src/services/mcp/McpHub.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -877,6 +877,22 @@ export class McpHub {
connection.server.error = truncatedError
}

/**
* Helper method to get timeout from connection configuration
* @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 {
Copy link
Contributor Author

@roomote roomote bot Sep 10, 2025

Choose a reason for hiding this comment

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

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.

try {
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)
Copy link
Contributor Author

@roomote roomote bot Sep 10, 2025

Choose a reason for hiding this comment

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

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.

// Default to 60 seconds if parsing fails
return 60 * 1000
Copy link
Contributor Author

@roomote roomote bot Sep 10, 2025

Choose a reason for hiding this comment

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

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.

}
}
Copy link
Contributor Author

@roomote roomote bot Sep 10, 2025

Choose a reason for hiding this comment

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

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.


/**
* Helper method to find a connection by server name and source
* @param serverName The name of the server to find
Expand Down Expand Up @@ -911,7 +927,10 @@ export class McpHub {
return []
}

const response = await connection.client.request({ method: "tools/list" }, ListToolsResultSchema)
const timeout = this.getTimeoutFromConnection(connection)
const response = await connection.client.request({ method: "tools/list" }, ListToolsResultSchema, {
timeout,
})

// Determine the actual source of the server
const actualSource = connection.server.source || "global"
Expand Down Expand Up @@ -965,7 +984,11 @@ export class McpHub {
if (!connection || connection.type !== "connected") {
return []
}
const response = await connection.client.request({ method: "resources/list" }, ListResourcesResultSchema)

const timeout = this.getTimeoutFromConnection(connection)
const response = await connection.client.request({ method: "resources/list" }, ListResourcesResultSchema, {
timeout,
})
return response?.resources || []
} catch (error) {
// console.error(`Failed to fetch resources for ${serverName}:`, error)
Expand All @@ -982,9 +1005,14 @@ export class McpHub {
if (!connection || connection.type !== "connected") {
return []
}

const timeout = this.getTimeoutFromConnection(connection)
const response = await connection.client.request(
{ method: "resources/templates/list" },
ListResourceTemplatesResultSchema,
{
timeout,
},
)
return response?.resourceTemplates || []
} catch (error) {
Expand Down Expand Up @@ -1564,6 +1592,8 @@ export class McpHub {
if (connection.server.disabled) {
throw new Error(`Server "${serverName}" is disabled`)
}

const timeout = this.getTimeoutFromConnection(connection)
return await connection.client.request(
{
method: "resources/read",
Expand All @@ -1572,6 +1602,9 @@ export class McpHub {
},
},
ReadResourceResultSchema,
{
timeout,
},
)
}

Expand All @@ -1591,16 +1624,7 @@ export class McpHub {
throw new Error(`Server "${serverName}" is disabled and cannot be used`)
}

let timeout: number
try {
const parsedConfig = ServerConfigSchema.parse(JSON.parse(connection.server.config))
timeout = (parsedConfig.timeout ?? 60) * 1000
} catch (error) {
console.error("Failed to parse server config for timeout:", error)
// Default to 60 seconds if parsing fails
timeout = 60 * 1000
}

const timeout = this.getTimeoutFromConnection(connection)
return await connection.client.request(
{
method: "tools/call",
Expand Down
Loading