mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-08-12 18:26:26 +00:00
feat(core): add configurable idle timeout for MCP tool calls (#6061)
* feat(core): add configurable idle timeout for MCP tool calls Adds an idle timeout mechanism that aborts MCP tool calls when the server does not produce any response or progress update within a configurable window. This prevents hung tool calls from blocking the session indefinitely. - Configurable via mcpToolIdleTimeoutMs config parameter or QWEN_CODE_MCP_TOOL_IDLE_TIMEOUT_MS environment variable (default 5 min) - Implemented via AbortController + AbortSignal.any() to combine external cancellation with idle timeout - Timeout resets on each progress notification, distinguishing between slow-but-working servers and truly hung servers - Added settingsSchema entry with min 10s, max 1h validation - 3 test cases covering timeout abort, progress reset, and disabled state Closes #6047 * fix(cli): wire mcpToolIdleTimeoutMs from settings to ConfigParameters The settings schema defines mcp.toolIdleTimeoutMs and Config reads mcpToolIdleTimeoutMs from ConfigParameters with an env-var fallback, but the CLI config builder was not passing settings.mcp?.toolIdleTimeoutMs into ConfigParameters. This meant configuring it via settings.json had no effect — only the env var and constructor default (300000ms) worked. Add the missing wiring line near the other mcp fields. * fix(core): address PR #6061 review feedback for MCP idle timeout - Fix env var parsing: use explicit Number.isFinite check instead of || to allow 0 (disable) - Add timer.unref() to prevent idle timeout from blocking process exit - Move clearTimeout to finally block to avoid duplication - Fix progress reset test: mock now observes abort signal and timing is correct
This commit is contained in:
parent
df3e753a51
commit
def96c83a1
6 changed files with 227 additions and 3 deletions
|
|
@ -1951,6 +1951,7 @@ export async function loadCliConfig(
|
|||
bareMode || safeMode ? undefined : settings.tools?.callCommand,
|
||||
mcpServerCommand:
|
||||
bareMode || safeMode ? undefined : settings.mcp?.serverCommand,
|
||||
mcpToolIdleTimeoutMs: settings.mcp?.toolIdleTimeoutMs,
|
||||
mcpServers,
|
||||
topTierMcpServers,
|
||||
pendingMcpServers,
|
||||
|
|
|
|||
|
|
@ -2458,6 +2458,18 @@ const SETTINGS_SCHEMA = {
|
|||
showInDialog: false,
|
||||
mergeStrategy: MergeStrategy.CONCAT,
|
||||
},
|
||||
toolIdleTimeoutMs: {
|
||||
type: 'number',
|
||||
label: 'MCP Tool Idle Timeout (ms)',
|
||||
category: 'MCP',
|
||||
requiresRestart: false,
|
||||
default: 300000,
|
||||
minimum: 10000,
|
||||
maximum: 3600000,
|
||||
description:
|
||||
'Idle timeout in milliseconds for MCP tool calls. If the MCP server does not produce any response or progress update within this time, the call is aborted. Default: 300000 (5 minutes). Can be overridden via QWEN_CODE_MCP_TOOL_IDLE_TIMEOUT_MS environment variable.',
|
||||
showInDialog: false,
|
||||
},
|
||||
},
|
||||
},
|
||||
security: {
|
||||
|
|
|
|||
|
|
@ -962,6 +962,13 @@ export interface ConfigParameters {
|
|||
*/
|
||||
cliAllowedMcpServerNames?: string[];
|
||||
excludedMcpServers?: string[];
|
||||
/**
|
||||
* Idle timeout in milliseconds for MCP tool calls. If the MCP server does
|
||||
* not produce any response or progress update within this time, the call
|
||||
* is aborted. Default: 300000 (5 minutes). Can be overridden via
|
||||
* QWEN_CODE_MCP_TOOL_IDLE_TIMEOUT_MS environment variable.
|
||||
*/
|
||||
mcpToolIdleTimeoutMs?: number;
|
||||
/**
|
||||
* Names of project-scoped (`.mcp.json`) servers that are NOT yet approved
|
||||
* (pending or rejected). These are loaded so they can be listed, but the
|
||||
|
|
@ -1392,6 +1399,7 @@ export class Config {
|
|||
private readonly cliAllowedMcpServerNames?: string[];
|
||||
private excludedMcpServers?: string[];
|
||||
private pendingMcpServers?: string[];
|
||||
private readonly mcpToolIdleTimeoutMs: number;
|
||||
/**
|
||||
* Guards against concurrent MCP reconcile passes (hot-reload watcher vs.
|
||||
* `/reload`). `SettingsWatcher` serializes its own listeners, but `/reload`
|
||||
|
|
@ -1617,6 +1625,11 @@ export class Config {
|
|||
this.cliAllowedMcpServerNames = params.cliAllowedMcpServerNames;
|
||||
this.excludedMcpServers = params.excludedMcpServers;
|
||||
this.pendingMcpServers = params.pendingMcpServers;
|
||||
const envTimeout = process.env['QWEN_CODE_MCP_TOOL_IDLE_TIMEOUT_MS'];
|
||||
const parsedEnv = envTimeout !== undefined ? Number(envTimeout) : NaN;
|
||||
this.mcpToolIdleTimeoutMs =
|
||||
params.mcpToolIdleTimeoutMs ??
|
||||
(Number.isFinite(parsedEnv) && parsedEnv >= 0 ? parsedEnv : 300000); // 5 minutes default
|
||||
this.sessionSubagents = params.sessionSubagents ?? [];
|
||||
this.sdkMode = params.sdkMode ?? false;
|
||||
this.userMemory = params.userMemory ?? '';
|
||||
|
|
@ -3830,6 +3843,10 @@ export class Config {
|
|||
this.excludedMcpServers = excluded;
|
||||
}
|
||||
|
||||
getMcpToolIdleTimeoutMs(): number {
|
||||
return this.mcpToolIdleTimeoutMs;
|
||||
}
|
||||
|
||||
isMcpServerDisabled(serverName: string): boolean {
|
||||
if (matchesAnyServerPattern(serverName, this.excludedMcpServers))
|
||||
return true;
|
||||
|
|
|
|||
|
|
@ -1031,6 +1031,7 @@ export async function discoverTools(
|
|||
cliConfig,
|
||||
mcpClient, // raw MCP Client for direct callTool with progress
|
||||
mcpTimeout,
|
||||
cliConfig?.getMcpToolIdleTimeoutMs(),
|
||||
annotationsMap.get(funcDecl.name!),
|
||||
),
|
||||
);
|
||||
|
|
@ -1725,7 +1726,8 @@ export async function createTransport(
|
|||
) {
|
||||
const provider = new ServiceAccountImpersonationProvider(mcpServerConfig);
|
||||
const transportOptions:
|
||||
StreamableHTTPClientTransportOptions | SSEClientTransportOptions = {
|
||||
| StreamableHTTPClientTransportOptions
|
||||
| SSEClientTransportOptions = {
|
||||
authProvider: provider,
|
||||
};
|
||||
|
||||
|
|
@ -1753,7 +1755,8 @@ export async function createTransport(
|
|||
) {
|
||||
const provider = new GoogleCredentialProvider(mcpServerConfig);
|
||||
const transportOptions:
|
||||
StreamableHTTPClientTransportOptions | SSEClientTransportOptions = {
|
||||
| StreamableHTTPClientTransportOptions
|
||||
| SSEClientTransportOptions = {
|
||||
authProvider: provider,
|
||||
};
|
||||
if (mcpServerConfig.httpUrl) {
|
||||
|
|
|
|||
|
|
@ -1610,4 +1610,150 @@ describe('DiscoveredMCPTool', () => {
|
|||
expect(discoverToolsForServer).toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
describe('MCP Tool Idle Timeout', () => {
|
||||
it('should abort when MCP server does not respond within idle timeout', async () => {
|
||||
vi.useFakeTimers();
|
||||
|
||||
const idleTimeoutMs = 1000; // 1 second for testing
|
||||
const mockMcpClient: McpDirectClient = {
|
||||
callTool: vi.fn().mockImplementation(
|
||||
(_params, _schema, options) =>
|
||||
new Promise((_resolve, reject) => {
|
||||
// Simulate SDK behavior: reject when signal is aborted
|
||||
options?.signal?.addEventListener('abort', () => {
|
||||
const error = new Error(
|
||||
(options?.signal as AbortSignal & { reason?: Error })?.reason
|
||||
?.message ?? 'The operation was aborted',
|
||||
);
|
||||
error.name = 'AbortError';
|
||||
reject(error);
|
||||
});
|
||||
}),
|
||||
),
|
||||
};
|
||||
|
||||
const tool = new DiscoveredMCPTool(
|
||||
mockCallableToolInstance,
|
||||
serverName,
|
||||
serverToolName,
|
||||
baseDescription,
|
||||
inputSchema,
|
||||
true,
|
||||
undefined,
|
||||
undefined,
|
||||
mockMcpClient,
|
||||
undefined,
|
||||
idleTimeoutMs,
|
||||
);
|
||||
|
||||
const invocation = tool.build({ param: 'test' });
|
||||
const abortController = new AbortController();
|
||||
const executePromise = invocation.execute(abortController.signal);
|
||||
|
||||
// Advance time to trigger the idle timeout
|
||||
vi.advanceTimersByTime(idleTimeoutMs + 100);
|
||||
|
||||
await expect(executePromise).rejects.toThrow(
|
||||
/did not respond within.*idle timeout/,
|
||||
);
|
||||
// The external abort signal should not have been triggered
|
||||
expect(abortController.signal.aborted).toBe(false);
|
||||
|
||||
vi.useRealTimers();
|
||||
});
|
||||
|
||||
it('should reset idle timeout on progress updates', async () => {
|
||||
vi.useFakeTimers();
|
||||
|
||||
const idleTimeoutMs = 1000;
|
||||
let onProgressCallback: ((progress: any) => void) | undefined;
|
||||
|
||||
const mockMcpClient: McpDirectClient = {
|
||||
callTool: vi.fn().mockImplementation((_params, _schema, options) => {
|
||||
onProgressCallback = options?.onprogress;
|
||||
return new Promise((resolve, reject) => {
|
||||
// Listen for abort signal to properly reject when timeout fires
|
||||
options?.signal?.addEventListener('abort', () => {
|
||||
reject(options.signal!.reason);
|
||||
});
|
||||
// Resolve after 2.5 seconds (would timeout without progress)
|
||||
setTimeout(() => {
|
||||
resolve({ content: [{ type: 'text', text: 'Success' }] });
|
||||
}, 2500);
|
||||
});
|
||||
}),
|
||||
};
|
||||
|
||||
const tool = new DiscoveredMCPTool(
|
||||
mockCallableToolInstance,
|
||||
serverName,
|
||||
serverToolName,
|
||||
baseDescription,
|
||||
inputSchema,
|
||||
true,
|
||||
undefined,
|
||||
undefined,
|
||||
mockMcpClient,
|
||||
undefined,
|
||||
idleTimeoutMs,
|
||||
);
|
||||
|
||||
const invocation = tool.build({ param: 'test' });
|
||||
const executePromise = invocation.execute(new AbortController().signal);
|
||||
|
||||
// Send progress at 500ms, 1400ms, 2300ms to reset the timeout
|
||||
// Each progress must arrive BEFORE the 1000ms idle timeout fires
|
||||
vi.advanceTimersByTime(500);
|
||||
onProgressCallback?.({ progress: 0.25 });
|
||||
|
||||
vi.advanceTimersByTime(900);
|
||||
onProgressCallback?.({ progress: 0.5 });
|
||||
|
||||
vi.advanceTimersByTime(900);
|
||||
onProgressCallback?.({ progress: 0.75 });
|
||||
|
||||
// Advance past the mock's 2500ms resolve time
|
||||
vi.advanceTimersByTime(200);
|
||||
|
||||
const result = await executePromise;
|
||||
|
||||
expect(result.error).toBeUndefined();
|
||||
expect(result.llmContent).toBeDefined();
|
||||
|
||||
vi.useRealTimers();
|
||||
});
|
||||
|
||||
it('should not apply idle timeout when set to 0 or undefined', async () => {
|
||||
vi.useFakeTimers();
|
||||
|
||||
const mockMcpClient: McpDirectClient = {
|
||||
callTool: vi.fn().mockResolvedValue({
|
||||
content: [{ type: 'text', text: 'Success' }],
|
||||
}),
|
||||
};
|
||||
|
||||
const tool = new DiscoveredMCPTool(
|
||||
mockCallableToolInstance,
|
||||
serverName,
|
||||
serverToolName,
|
||||
baseDescription,
|
||||
inputSchema,
|
||||
true,
|
||||
undefined,
|
||||
undefined,
|
||||
mockMcpClient,
|
||||
undefined,
|
||||
undefined, // No idle timeout
|
||||
);
|
||||
|
||||
const invocation = tool.build({ param: 'test' });
|
||||
const result = await invocation.execute(new AbortController().signal);
|
||||
|
||||
expect(result.error).toBeUndefined();
|
||||
expect(result.llmContent).toBeDefined();
|
||||
|
||||
vi.useRealTimers();
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -136,6 +136,7 @@ class DiscoveredMCPToolInvocation extends BaseToolInvocation<
|
|||
private readonly cliConfig?: Config,
|
||||
private readonly mcpClient?: McpDirectClient,
|
||||
private readonly mcpTimeout?: number,
|
||||
private readonly mcpToolIdleTimeoutMs?: number,
|
||||
private readonly annotations?: McpToolAnnotations,
|
||||
private readonly retryCount: number = 0,
|
||||
) {
|
||||
|
|
@ -264,6 +265,7 @@ class DiscoveredMCPToolInvocation extends BaseToolInvocation<
|
|||
this.cliConfig,
|
||||
newTool['mcpClient'],
|
||||
this.mcpTimeout,
|
||||
this.mcpToolIdleTimeoutMs,
|
||||
this.annotations,
|
||||
this.retryCount + 1,
|
||||
);
|
||||
|
|
@ -317,7 +319,38 @@ class DiscoveredMCPToolInvocation extends BaseToolInvocation<
|
|||
signal: AbortSignal,
|
||||
updateOutput?: (output: ToolResultDisplay) => void,
|
||||
): Promise<ToolResult> {
|
||||
// Create an AbortController for idle timeout
|
||||
const idleTimeoutController = new AbortController();
|
||||
let idleTimeoutId: ReturnType<typeof setTimeout> | undefined;
|
||||
|
||||
// Combine the external signal with our idle timeout controller
|
||||
const combinedSignal = AbortSignal.any([
|
||||
signal,
|
||||
idleTimeoutController.signal,
|
||||
]);
|
||||
|
||||
const resetIdleTimeout = () => {
|
||||
if (idleTimeoutId) {
|
||||
clearTimeout(idleTimeoutId);
|
||||
}
|
||||
if (this.mcpToolIdleTimeoutMs && this.mcpToolIdleTimeoutMs > 0) {
|
||||
const timer = setTimeout(() => {
|
||||
const error = new Error(
|
||||
`MCP tool '${this.serverToolName}' on server '${this.serverName}' ` +
|
||||
`did not respond within ${this.mcpToolIdleTimeoutMs}ms idle timeout`,
|
||||
);
|
||||
error.name = 'AbortError';
|
||||
idleTimeoutController.abort(error);
|
||||
}, this.mcpToolIdleTimeoutMs);
|
||||
timer.unref();
|
||||
idleTimeoutId = timer;
|
||||
}
|
||||
};
|
||||
|
||||
try {
|
||||
// Start the idle timeout
|
||||
resetIdleTimeout();
|
||||
|
||||
const callToolResult = await this.mcpClient!.callTool(
|
||||
{
|
||||
name: this.serverToolName,
|
||||
|
|
@ -326,6 +359,9 @@ class DiscoveredMCPToolInvocation extends BaseToolInvocation<
|
|||
undefined,
|
||||
{
|
||||
onprogress: (progress) => {
|
||||
// Reset idle timeout on progress
|
||||
resetIdleTimeout();
|
||||
|
||||
if (updateOutput) {
|
||||
const progressData: McpToolProgressData = {
|
||||
type: 'mcp_tool_progress',
|
||||
|
|
@ -337,7 +373,7 @@ class DiscoveredMCPToolInvocation extends BaseToolInvocation<
|
|||
}
|
||||
},
|
||||
timeout: this.mcpTimeout,
|
||||
signal,
|
||||
signal: combinedSignal,
|
||||
},
|
||||
);
|
||||
|
||||
|
|
@ -374,6 +410,11 @@ class DiscoveredMCPToolInvocation extends BaseToolInvocation<
|
|||
};
|
||||
} catch (error) {
|
||||
return this.handleReconnectOnError(error, signal, updateOutput);
|
||||
} finally {
|
||||
// Clear the idle timeout in all cases
|
||||
if (idleTimeoutId) {
|
||||
clearTimeout(idleTimeoutId);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -510,6 +551,7 @@ export class DiscoveredMCPTool extends BaseDeclarativeTool<
|
|||
private readonly cliConfig?: Config,
|
||||
private readonly mcpClient?: McpDirectClient,
|
||||
private readonly mcpTimeout?: number,
|
||||
private readonly mcpToolIdleTimeoutMs?: number,
|
||||
readonly annotations?: McpToolAnnotations,
|
||||
) {
|
||||
super(
|
||||
|
|
@ -542,6 +584,7 @@ export class DiscoveredMCPTool extends BaseDeclarativeTool<
|
|||
this.cliConfig,
|
||||
this.mcpClient,
|
||||
this.mcpTimeout,
|
||||
this.mcpToolIdleTimeoutMs,
|
||||
this.annotations,
|
||||
);
|
||||
}
|
||||
|
|
@ -577,6 +620,7 @@ export class DiscoveredMCPTool extends BaseDeclarativeTool<
|
|||
this.cliConfig,
|
||||
this.mcpClient,
|
||||
this.mcpTimeout,
|
||||
this.mcpToolIdleTimeoutMs,
|
||||
this.annotations,
|
||||
);
|
||||
}
|
||||
|
|
@ -594,6 +638,7 @@ export class DiscoveredMCPTool extends BaseDeclarativeTool<
|
|||
this.cliConfig,
|
||||
this.mcpClient,
|
||||
this.mcpTimeout,
|
||||
this.mcpToolIdleTimeoutMs,
|
||||
this.annotations,
|
||||
);
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue