Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
62 changes: 62 additions & 0 deletions packages/core/src/core/coreToolScheduler.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -838,6 +838,68 @@ describe('CoreToolScheduler', () => {
);
});

it('aborts and fails a tool call that exceeds the execution timeout', async () => {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] Only one test covers the timeout feature. Consider adding tests for:

  • Happy path — tool completes before timeout (timer is set up and torn down without firing)
  • Parent-abort forwarding — user cancels while timeout is active; verify the derived signal also aborts and status is 'cancelled', not EXECUTION_TIMEOUT
  • Tool ignores abort — tool returns a promise that never resolves and ignores the signal; verify the scheduler still unblocks with EXECUTION_TIMEOUT

The "tool ignores abort" path is the exact scenario the feature is designed for (stuck tools).

— qwen3.7-max via Qwen Code /review

const previousTimeout = process.env['QWEN_CODE_TOOL_EXECUTION_TIMEOUT_MS'];
process.env['QWEN_CODE_TOOL_EXECUTION_TIMEOUT_MS'] = '30';

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] The 30ms real timer is fragile under heavy CI load. This file already uses vi.useFakeTimers() at line 667 for another test. Consider using fake timers here for determinism:

Suggested change
process.env['QWEN_CODE_TOOL_EXECUTION_TIMEOUT_MS'] = '30';
process.env['QWEN_CODE_TOOL_EXECUTION_TIMEOUT_MS'] = '300';
vi.useFakeTimers();

Then advance timers with await vi.advanceTimersByTimeAsync(300) after scheduling, and restore with vi.useRealTimers() in the finally block.

— qwen3.7-max via Qwen Code /review

try {
let toolSawAbort = false;
// A tool that never settles on its own — it resolves only once its
// AbortSignal fires, proving the timeout actually cancels the tool
// rather than merely abandoning it.
const execute = vi.fn(
(_params: unknown, signal?: AbortSignal) =>
new Promise<ToolResult>((resolve) => {
signal?.addEventListener('abort', () => {
toolSawAbort = true;
resolve({
llmContent: 'aborted late',
returnDisplay: 'aborted late',
});
});
}),
);
const toolsByName = new Map<string, MockTool>([
[
'read_file',
new MockTool({ name: 'read_file', canUpdateOutput: true, execute }),
],
]);
const { scheduler, onAllToolCallsComplete } =
createSchedulerForLegacyToolTests({ toolsByName });

await scheduler.schedule(
[
{
callId: 'timeout-1',
name: 'read_file',
args: { file_path: 'a.ts' },
isClientInitiated: false,
prompt_id: 'prompt-timeout',
},
],
new AbortController().signal,
);

const completedCall = (
onAllToolCallsComplete.mock.calls[0][0] as ToolCall[]
)[0];
expect(completedCall.status).toBe('error');
if (completedCall.status === 'error') {
expect(completedCall.response.errorType).toBe(
ToolErrorType.EXECUTION_TIMEOUT,
);
expect(completedCall.response.error?.message).toContain('timed out');
}
expect(toolSawAbort).toBe(true);
} finally {
if (previousTimeout === undefined) {
delete process.env['QWEN_CODE_TOOL_EXECUTION_TIMEOUT_MS'];
} else {
process.env['QWEN_CODE_TOOL_EXECUTION_TIMEOUT_MS'] = previousTimeout;
}
}
});

it('executes only the first request for duplicate callIds in one batch', async () => {
const execute = vi.fn().mockResolvedValue({
llmContent: 'first result',
Expand Down
87 changes: 84 additions & 3 deletions packages/core/src/core/coreToolScheduler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,22 @@ const TOOL_FAILURE_KIND_NON_INTERACTIVE_DENIED = 'non_interactive_denied';
const TOOL_FAILURE_KIND_BACKGROUND_AGENT_DENIED = 'background_agent_denied';

const TOOL_SPAN_STATUS_PRE_HOOK_BLOCKED = 'Tool execution blocked by hook';

/**
* Builds the failure ToolResult surfaced when a tool call exceeds the
* execution timeout. Reported as a normal tool error so the model can adapt
* (narrow scope, retry, etc.) instead of the session hanging.
*/

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] Math.round(timeoutMs / 1000) renders any timeout under 500 ms as "0s". The new test uses 30 ms, so the model-facing message reads "Tool execution timed out after 0s." — factually wrong and confusing both to the LLM and to operators reading returnDisplay.

Suggested change
*/
function createToolTimeoutResult(timeoutMs: number): ToolResult {
const duration =
timeoutMs < 1000 ? `${timeoutMs}ms` : `${Math.round(timeoutMs / 1000)}s`;
const message =
`Tool execution timed out after ${duration}. ` +
`The tool may be stuck or operating on too large a scope.`;
return {
llmContent: message,
returnDisplay: message,
error: { message, type: ToolErrorType.EXECUTION_TIMEOUT },
};
}

— qwen3.7-max via Qwen Code /review

function createToolTimeoutResult(timeoutMs: number): ToolResult {
const message =
`Tool execution timed out after ${Math.round(timeoutMs / 1000)}s. ` +
`The tool may be stuck or operating on too large a scope.`;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Nice to have] Math.round(timeoutMs / 1000) rounds to 0 for any timeout < 500ms, producing "timed out after 0s." Consider displaying milliseconds for sub-second values:

Suggested change
`The tool may be stuck or operating on too large a scope.`;
`Tool execution timed out after ${timeoutMs >= 1000 ? Math.round(timeoutMs / 1000) + 's' : timeoutMs + 'ms'}. ` +

— qwen3.7-max via Qwen Code /review

return {
llmContent: message,
returnDisplay: message,
error: { message, type: ToolErrorType.EXECUTION_TIMEOUT },
};
}
const TOOL_SPAN_STATUS_POST_HOOK_STOPPED = 'Tool execution stopped by hook';
const TOOL_SPAN_STATUS_PERMISSION_DENIED = 'Permission denied for tool';
const TOOL_SPAN_STATUS_PERMISSION_HOOK_DENIED =
Expand Down Expand Up @@ -3315,6 +3331,39 @@ export class CoreToolScheduler {
);
try {
let promise: Promise<ToolResult>;

// Per-tool-call execution timeout. Disabled by default (experimental):
// set QWEN_CODE_TOOL_EXECUTION_TIMEOUT_MS to a positive number of
// milliseconds to cap how long a single tool call may run.
const toolExecutionTimeoutMs = parsePositiveIntegerEnv(
process.env['QWEN_CODE_TOOL_EXECUTION_TIMEOUT_MS'],
0,
);

// When a timeout is active, run the tool under a derived AbortSignal so
// that on timeout we actually cancel the in-flight work (cooperative
// tools stop; the shell kills its subprocess) instead of abandoning it
// to run on unobserved. A user abort on the parent signal is forwarded
// to the derived signal. The forwarding listener is torn down once the
// tool settles (see finally below) so it never accumulates on the
// long-lived parent (turn) signal across tool calls.
let execSignal = signal;
let timeoutController: AbortController | undefined;
let removeParentAbortForward: (() => void) | undefined;
if (toolExecutionTimeoutMs > 0) {
timeoutController = new AbortController();
execSignal = timeoutController.signal;
if (signal.aborted) {
timeoutController.abort(signal.reason);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] This manual AbortController + addEventListener('abort', ...) + removeEventListener pattern reimplements AbortSignal.any(), which is used in 29 other locations in this codebase and is natively available on Node.js ≥22. Using it would eliminate ~15 lines of signal plumbing and the removeParentAbortForward cleanup:

Suggested change
timeoutController.abort(signal.reason);
let execSignal = signal;
let timeoutController: AbortController | undefined;
if (toolExecutionTimeoutMs > 0) {
timeoutController = new AbortController();
execSignal = AbortSignal.any([signal, timeoutController.signal]);
}

This also removes the need for the finally teardown of the forwarding listener and the if (signal.aborted) early-abort branch.

— qwen3.7-max via Qwen Code /review

} else {
const controller = timeoutController;
const forwardAbort = () => controller.abort(signal.reason);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Critical] Listener/timer leak on synchronous throw from invocation.execute(). The abort listener registered on the parent signal here, and the setTimeout started later in the wrapper Promise, are only cleaned up by the inner finally block that brackets the await of the wrapper Promise. If invocation.execute() throws synchronously — a scenario the outer try/catch at ~line 3857 explicitly guards against ("synchronous throws (e.g. shell setup failure)") — control jumps to the outer catch and neither removeParentAbortForward?.() nor clearTimeout(timeoutTimer) runs. The dangling abort listener accumulates on the long-lived parent (turn) signal across tool calls.

Fix: hoist timeoutTimer to the outer scope and move the removeParentAbortForward?.() + clearTimeout(timeoutTimer) cleanup into the outer finally (the one that already releases sleepInhibitorHandle), so it runs on both sync-throw and async-settle paths.

— qwen3.7-max via Qwen Code /review

signal.addEventListener('abort', forwardAbort, { once: true });
removeParentAbortForward = () =>
signal.removeEventListener('abort', forwardAbort);
}
}

if (invocation instanceof ShellToolInvocation) {
const setPidCallback = (pid: number) => {
this.toolCalls = this.toolCalls.map((tc) =>
Expand Down Expand Up @@ -3348,7 +3397,7 @@ export class CoreToolScheduler {
);
};
promise = invocation.execute(
signal,
execSignal,
liveOutputCallback,
shellExecutionConfig,
setPidCallback,
Expand All @@ -3357,13 +3406,45 @@ export class CoreToolScheduler {
);
} else {
promise = invocation.execute(
signal,
execSignal,
liveOutputCallback,
shellExecutionConfig,
);
}

const toolResult: ToolResult = await promise;
let toolResult: ToolResult;
if (timeoutController) {
let timeoutTimer: ReturnType<typeof setTimeout> | undefined;
try {
toolResult = await new Promise<ToolResult>((resolve, reject) => {
timeoutTimer = setTimeout(() => {
debugLogger.warn(
`Tool ${canonicalName} (${callId}) timed out after ` +
`${toolExecutionTimeoutMs}ms — aborting`,
);
// Cancel the in-flight tool via the derived signal, then resolve
// with a timeout error so the scheduler is unblocked even if the
// tool ignores the abort. A later settle from `promise` is a
// no-op once this wrapper Promise has already resolved.
timeoutController?.abort(
new Error(
`Tool execution timed out after ${toolExecutionTimeoutMs}ms`,
),
);
resolve(createToolTimeoutResult(toolExecutionTimeoutMs));
}, toolExecutionTimeoutMs);
timeoutTimer.unref?.();
promise.then(resolve, reject);
});
} finally {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] After the timeout fires and resolves the wrapper, promise continues running in the background. If it later rejects, the error is silently swallowed (the wrapper is already resolved, so the reject callback in .then(resolve, reject) is a no-op). Consider adding a .catch() for observability:

Suggested change
} finally {
promise.then(resolve, reject);
promise.catch((err) => {
debugLogger.warn(
`Tool ${canonicalName} (${callId}) rejected after timeout: ${err?.message ?? err}`,
);
});

— qwen3.7-max via Qwen Code /review

if (timeoutTimer) clearTimeout(timeoutTimer);
// Tear down the parent-abort forwarding listener now that the tool
// has settled (no-op if a user abort already removed it via `once`).
removeParentAbortForward?.();
}
} else {
toolResult = await promise;
}
// A tool that observes signal.aborted and resolves with a normal
// ToolResult (no .error field) would otherwise close the execution
// sub-span as success while the parent tool span ends as cancelled.
Expand Down
2 changes: 2 additions & 0 deletions packages/core/src/tools/tool-error.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ export enum ToolErrorType {
UNHANDLED_EXCEPTION = 'unhandled_exception',
TOOL_NOT_REGISTERED = 'tool_not_registered',
EXECUTION_FAILED = 'execution_failed',
// A tool call exceeded the per-tool execution timeout and was aborted.
EXECUTION_TIMEOUT = 'execution_timeout',
// Try to execute a tool that is excluded due to the approval mode
EXECUTION_DENIED = 'execution_denied',

Expand Down
2 changes: 1 addition & 1 deletion packages/vscode-ide-companion/NOTICES.txt
Original file line number Diff line number Diff line change
Expand Up @@ -1665,7 +1665,7 @@ SOFTWARE.


============================================================
hasown@2.0.2
hasown@2.0.4
(git+https://github.com/inspect-js/hasOwn.git)

MIT License
Expand Down
Loading