fix: add default timeout for terminal command tool execution#10550
fix: add default timeout for terminal command tool execution#10550amabito wants to merge 1 commit intocontinuedev:mainfrom
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
There was a problem hiding this comment.
3 issues found across 2 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="core/tools/implementations/runTerminalCommand.ts">
<violation number="1" location="core/tools/implementations/runTerminalCommand.ts:179">
P3: Non-streaming waitForCompletion schedules timeout/sigkill timers but the error handler does not clear them, leaving timers to fire after rejection and potentially kill/mutate output for a failed spawn.</violation>
<violation number="2" location="core/tools/implementations/runTerminalCommand.ts:180">
P2: SIGKILL fallback never runs because childProc.killed becomes true immediately after SIGTERM; it does not indicate process exit. Use exitCode/signalCode to detect whether the process is still running.</violation>
</file>
<file name="core/tools/implementations/runTerminalCommand.timeout.vitest.ts">
<violation number="1" location="core/tools/implementations/runTerminalCommand.timeout.vitest.ts:35">
P2: Extra closing brace ends the describe block before the final test, leaving the last test outside the setup/teardown and out of scope for helpers/mocks.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| // Set up timeout for waitForCompletion mode | ||
| if (waitForCompletion) { | ||
| timeoutId = setTimeout(() => { | ||
| if (!childProc.killed) { |
There was a problem hiding this comment.
P2: SIGKILL fallback never runs because childProc.killed becomes true immediately after SIGTERM; it does not indicate process exit. Use exitCode/signalCode to detect whether the process is still running.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At core/tools/implementations/runTerminalCommand.ts, line 180:
<comment>SIGKILL fallback never runs because childProc.killed becomes true immediately after SIGTERM; it does not indicate process exit. Use exitCode/signalCode to detect whether the process is still running.</comment>
<file context>
@@ -168,6 +174,42 @@ export const runTerminalCommandImpl: ToolImpl = async (args, extras) => {
+ // Set up timeout for waitForCompletion mode
+ if (waitForCompletion) {
+ timeoutId = setTimeout(() => {
+ if (!childProc.killed) {
+ terminalOutput += "
+[Timeout: process killed after 2 minutes]
</file context>
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
e585e06 to
03595e5
Compare
|
I have read the CLA Document and I hereby sign the CLA |
Terminal commands with waitForCompletion=true had no timeout, risking indefinite hangs. Adds a 2-minute default timeout with graceful SIGTERM -> 5s grace period -> SIGKILL escalation. Both streaming and non-streaming code paths are covered. The SIGKILL timer is properly cleared if the process exits during the grace period.
03595e5 to
ce37e50
Compare
|
@sestinj CI is fully green (including both required checks), and this is a small, self-contained safety improvement (default 2-min timeout + proper SIGKILL timer cleanup, tests included). Would appreciate a quick review when you have a moment. Thanks! |
Summary
runTerminalCommandcould hang indefinitely, blocking the entire sessionChanges
core/tools/implementations/runTerminalCommand.ts— timeout logic for both streaming and non-streaming modescore/tools/implementations/runTerminalCommand.timeout.vitest.ts— 5 test cases covering timeout, cleanup, and background process exclusionDetails
DEFAULT_TOOL_TIMEOUT_MS = 120_000(2 minutes)waitForCompletion=false(background) processes are not affected[Timeout: process killed after 2 minutes]Test plan
runTerminalCommand.timeout.vitest.tspassesrunTerminalCommand.vitest.tsunaffected🤖 Generated with Claude Code
Continue Tasks: ✅ 1 no changes — View all
Summary by cubic
Add a default 2-minute timeout to runTerminalCommand so hung commands don’t block sessions. Gracefully terminates processes and cleans up timers to prevent leaks.
Written for commit ce37e50. Summary will update on new commits.