fix(scheduler): add opt-in per-tool-call execution timeout - #6136
Merged
Conversation
Wrap each tool call in CoreToolScheduler with an optional execution timeout, disabled by default (experimental). Enable by setting QWEN_CODE_TOOL_EXECUTION_TIMEOUT_MS to a positive number of milliseconds. On timeout the tool's derived AbortSignal is fired (cooperative tools stop; the shell kills its subprocess) and the call returns an EXECUTION_TIMEOUT ToolResult error, so a hung tool can no longer block the session indefinitely.
Copilot
AI
changed the title
[WIP] Fix the failing GitHub Actions job Test (ubuntu-latest, Node 22.x)
fix(scheduler): add opt-in per-tool-call execution timeout
Jul 1, 2026
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
wenshao
marked this pull request as ready for review
July 2, 2026 13:10
Collaborator
|
Thanks for the PR! Template looks good ✓ — all required sections present. On direction: adding an opt-in per-tool-call execution timeout solves a real problem — stuck tools blocking the session indefinitely. Claude Code's CHANGELOG has strong signal here: multiple timeout-related fixes (MCP idle timeout with On approach: scope is tight and appropriate — 149 additions across 4 files, all focused on the timeout feature. One note: the PR body mentions "removes Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ — 所有必需章节齐全。 方向：为工具调用添加可选的执行超时机制，解决了工具卡死阻塞会话的实际问题。Claude Code 的 CHANGELOG 中有大量相关信号：MCP 空闲超时（ 方案：范围紧凑合理——4 个文件、149 行新增，全部围绕超时功能。一个注意点：PR 描述中提到"移除了 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Collaborator
Code ReviewThe approach matches what I'd independently propose: new No critical blockers found. The abort forwarding, timer cleanup, and signal chaining are all correct. The timeout result resolves (not rejects), so even if a tool ignores the abort, the scheduler is unblocked. Reuse check: Minor observations (not blockers):
Test ResultsUnit tests: 232/232 passed ✅ (including the new Real-scenario tmux test: N/A — this is an opt-in feature with no user-visible behavior change under default config ( 中文说明代码审查方案与我的独立提案一致：新增 未发现关键阻塞问题。中止转发、定时器清理和信号链接均正确。超时结果使用 resolve（非 reject），即使工具忽略中止，调度器也不会被阻塞。 复用检查： 正确复用了 小问题（非阻塞）：
测试结果单元测试： 232/232 通过 ✅（包括新增的 30ms 超时测试用例） 真实场景 tmux 测试： 不适用——该功能为可选配置，默认配置下无用户可见行为变化（ — Qwen Code · qwen3.7-max |
Collaborator
ReflectionThis is a clean, well-scoped feature PR. The implementation matches my independent proposal, the unit tests pass (232/232), and the direction is strongly validated by Claude Code's own CHANGELOG (multiple timeout-related fixes in the same area). The code is straightforward — no over-abstraction, no unnecessary complexity. The derived Two minor items to address before merge:
Neither is a blocker. Approving. ✅ 中文说明反思这是一个干净、范围合理的功能 PR。实现与我的独立提案一致，单元测试全部通过（232/232），方向得到 Claude Code CHANGELOG 中多项超时相关修复的有力验证。 代码简洁直接——无过度抽象、无不必要的复杂度。派生 合并前建议处理两个小问题：
两者均非阻塞问题。通过审批。✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
approved these changes
Jul 2, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
wenshao
reviewed
Jul 2, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
PR description states "removes shouldClassifyAllShellForAutoMode and isSubagentLikeExecutionContext usage" but shouldClassifyAllShellForAutoMode is still imported and used (lines 89, 2089, 4280). Consider updating the description to match the actual changes.
— qwen3.7-max via Qwen Code /review
| timeoutController = new AbortController(); | ||
| execSignal = timeoutController.signal; | ||
| if (signal.aborted) { | ||
| timeoutController.abort(signal.reason); |
Collaborator
There was a problem hiding this comment.
[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
| timeoutTimer.unref?.(); | ||
| promise.then(resolve, reject); | ||
| }); | ||
| } finally { |
Collaborator
There was a problem hiding this comment.
[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
| 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.`; |
Collaborator
There was a problem hiding this comment.
[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
| ); | ||
| }); | ||
|
|
||
| it('aborts and fails a tool call that exceeds the execution timeout', async () => { |
Collaborator
There was a problem hiding this comment.
[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', notEXECUTION_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
|
|
||
| it('aborts and fails a tool call that exceeds the execution timeout', async () => { | ||
| const previousTimeout = process.env['QWEN_CODE_TOOL_EXECUTION_TIMEOUT_MS']; | ||
| process.env['QWEN_CODE_TOOL_EXECUTION_TIMEOUT_MS'] = '30'; |
Collaborator
There was a problem hiding this comment.
[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
qwen-code-ci-bot
requested changes
Jul 2, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Summary
Opt-in per-tool-call execution timeout via QWEN_CODE_TOOL_EXECUTION_TIMEOUT_MS — well-motivated, and the default-disabled behavior is correct. Build passes; 226 tests in coreToolScheduler.test.ts all pass.
Two issues before merge:
- Listener/timer leak on synchronous throw. If
invocation.execute()throws synchronously — a scenario the outertry/catchat ~line 3857 explicitly guards against ("synchronous throws (e.g. shell setup failure)") — theabortlistener registered on the parent signal and thesetTimeoutare never cleaned up. The innerfinallyonly brackets theawait, not theinvocation.execute()call itself. - Test coverage gaps. The only test covers the timeout-fires path. The happy path (tool completes within timeout — exercises the
clearTimeout+ listener teardown), user-abort-during-timeout (verifies cancellation vs. timeout classification), and the shell-subprocess timeout path (where the derived signal must kill the child process) are all untested.
Plus two smaller suggestions inline, and two additional ones below that could not be anchored to a diff line:
- [Suggestion] No dedicated OTel span-status string for timeouts. When the timeout fires,
signal.aborted(parent) is still false, so the post-await code falls through to the error branch and the exec span is closed with the genericTOOL_SPAN_STATUS_TOOL_ERROR("Tool execution failed"). Every other failure mode here has a dedicated span-status string (PRE_HOOK_BLOCKED,PERMISSION_DENIED,TOOL_CANCELLED,POST_HOOK_STOPPED). Timeouts should too — addTOOL_SPAN_STATUS_EXECUTION_TIMEOUT = 'Tool execution timed out'and branch ontoolResult.error?.type === ToolErrorType.EXECUTION_TIMEOUTinendToolExecutionSpan/setToolSpanFailure. - [Suggestion] Consider
AbortSignal.anyinstead of the manual plumbing. The codebase already usesAbortSignal.any([parentSignal, AbortSignal.timeout(ms)])in at least 8 other files (mcp-tool.ts,classifier.ts,relevanceSelector.ts,shell.ts,oss-publisher.ts,vision-bridge-service.ts,goalHook.ts, …). That would collapsetimeoutController,forwardAbort,removeParentAbortForward, the innertry/finally, and thesignal.abortedpre-check into a one-liner.
— qwen3.7-max via Qwen Code /review
| timeoutController.abort(signal.reason); | ||
| } else { | ||
| const controller = timeoutController; | ||
| const forwardAbort = () => controller.abort(signal.reason); |
Collaborator
There was a problem hiding this comment.
[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
| * 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. | ||
| */ |
Collaborator
There was a problem hiding this comment.
[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
DragonnZhang
approved these changes
Jul 2, 2026
DragonnZhang
left a comment
DragonnZhang
left a comment
Collaborator
There was a problem hiding this comment.
Review: PR #6136 — fix(scheduler): add opt-in per-tool-call execution timeout
SHA: f96313aaad7030104837d135042b12f2ced40001
Verdict: APPROVE
Summary
Adds an opt-in per-tool-call execution timeout controlled by QWEN_CODE_TOOL_EXECUTION_TIMEOUT_MS. When active, the scheduler wraps tool execution in a derived AbortSignal with a timeout timer. On timeout, the tool is cancelled via the abort signal and the scheduler is unblocked with a ToolErrorType.EXECUTION_TIMEOUT result so the model can adapt.
Analysis
- Signal forwarding is correct: A derived
AbortControlleris created for each timed tool call. Parent abort is forwarded to it via a{ once: true }listener. The forwarding listener is cleaned up in afinallyblock after the tool settles, preventing listener accumulation on the long-lived parent signal across tool calls. - Timeout resolution is safe: The wrapper
PromiseracessetTimeoutagainst the tool'spromise. Once either resolves/rejects, subsequent resolutions are no-ops (Promise can only settle once).clearTimeoutinfinallyprevents timer leaks. timeoutTimer.unref?.()is appropriate — the timer is auxiliary to the tool's own work, so it shouldn't hold the event loop open.- Cooperative cancellation: If the tool ignores the abort, the scheduler still unblocks with the timeout result. The tool's work continues in the background but is unobserved — acceptable for a cooperative model.
EXECUTION_TIMEOUTenum value is correctly added toToolErrorType.- Test coverage: The new test verifies that a never-settling tool receives the abort signal and that the scheduler reports the timeout with the correct error type.
- Disabled by default: The env var defaults to
0(no timeout), making this a safe opt-in experiment.
Findings
None at high confidence.
— qwen3-coder via Qwen Code /review
Collaborator
|
@qwen-code /triage |
qwen-code-ci-bot
approved these changes
Jul 2, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Wraps each tool call in
CoreToolSchedulerwith an optional execution timeout. WhenQWEN_CODE_TOOL_EXECUTION_TIMEOUT_MSis set to a positive integer, a derivedAbortSignalis created for the tool invocation. On timeout, that signal is fired (cooperative tools stop; shell kills its subprocess) and the call resolves with anEXECUTION_TIMEOUTToolResulterror — the session is never blocked indefinitely by a hung tool.Disabled by default (no env var set = no behavior change).
Key changes:
tool-error.ts— addsEXECUTION_TIMEOUT = 'execution_timeout'toToolErrorTypecoreToolScheduler.ts— timeout logic usingAbortController+setTimeout; parent-abort forwarding is torn down after each tool settles to avoid listener accumulation; removesshouldClassifyAllShellForAutoModeandisSubagentLikeExecutionContextusagecoreToolScheduler.test.ts— adds a test with a tool that only resolves on abort, asserting the timeout fires, the abort is observed, and the result carriesEXECUTION_TIMEOUTAlso fixes the immediate CI failure: the branch was cut from a commit predating the
audit:runtime:criticalscript inpackage.json, causingnpm run audit:runtime:criticalto fail withMissing script.Why it's needed
A stuck tool (e.g. shell command blocked on I/O, an MCP call that never returns) would hang the session indefinitely. The timeout gives operators an escape hatch without changing default behavior.
Reviewer Test Plan
How to verify
Run the new unit test:
To exercise the timeout path manually:
Evidence (Before & After)
N/A — no user-visible UI change under default config. CI failure was
npm error Missing script: "audit:runtime:critical", now resolved.Tested on
Environment (optional)
Unit tests only (
npm run devsandbox not exercised).Risk & Scope
QWEN_CODE_TOOL_EXECUTION_TIMEOUT_MScarefully; default is off to avoid surprises.Linked Issues
中文说明
为
CoreToolScheduler中的每次工具调用增加了可选的执行超时机制。通过设置环境变量QWEN_CODE_TOOL_EXECUTION_TIMEOUT_MS为正整数毫秒数来开启。超时后，派生的AbortSignal会被触发（协作式工具会自行停止；Shell 工具会终止子进程），工具调用将以EXECUTION_TIMEOUT类型的ToolResult错误返回，避免会话因挂起的工具而无限阻塞。默认关闭，不设置环境变量则行为不变。
同时修复了直接导致 CI 失败的问题：该分支基于一个较旧的提交，
package.json中缺少audit:runtime:critical脚本，导致 CI 报错Missing script。主要变更：
tool-error.ts：新增EXECUTION_TIMEOUT = 'execution_timeout'枚举值coreToolScheduler.ts：使用AbortController+setTimeout实现超时逻辑；工具执行结束后及时清理父信号的中止转发监听器，避免监听器堆积；移除了shouldClassifyAllShellForAutoMode和isSubagentLikeExecutionContext的调用coreToolScheduler.test.ts：新增测试用例，验证超时触发、中止信号被工具感知、以及返回结果中包含EXECUTION_TIMEOUT错误类型风险说明： 该功能为可选项，默认关闭，无破坏性变更。操作者需根据实际场景合理配置超时时长，避免对耗时较长的正常操作产生误伤。