Skip to content

Commit ce37e50

Browse files
amabitoamabito
authored andcommitted
fix: add default timeout for terminal command tool execution
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.
1 parent 86a37f9 commit ce37e50

2 files changed

Lines changed: 272 additions & 0 deletions

File tree

Lines changed: 186 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,186 @@
1+
import { EventEmitter } from "node:events";
2+
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
3+
import { IDE, ToolExtras } from "../..";
4+
import { runTerminalCommandImpl } from "./runTerminalCommand";
5+
6+
// Hoist mock function so it can be referenced in vi.mock factory
7+
const { mockSpawn } = vi.hoisted(() => ({
8+
mockSpawn: vi.fn(),
9+
}));
10+
11+
vi.mock("node:child_process", () => ({
12+
default: {
13+
spawn: mockSpawn,
14+
},
15+
spawn: mockSpawn,
16+
}));
17+
18+
describe("runTerminalCommand timeout functionality", () => {
19+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
20+
let mockChildProc: any;
21+
let mockGetIdeInfo: ReturnType<typeof vi.fn>;
22+
let mockGetWorkspaceDirs: ReturnType<typeof vi.fn>;
23+
let mockOnPartialOutput: ReturnType<typeof vi.fn>;
24+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
25+
let setTimeoutSpy: any;
26+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
27+
let clearTimeoutSpy: any;
28+
29+
beforeEach(() => {
30+
vi.resetAllMocks();
31+
vi.clearAllTimers();
32+
vi.useFakeTimers();
33+
34+
// Spy on setTimeout and clearTimeout
35+
setTimeoutSpy = vi.spyOn(global, "setTimeout");
36+
clearTimeoutSpy = vi.spyOn(global, "clearTimeout");
37+
38+
// Create mock child process with EventEmitter behavior
39+
mockChildProc = new EventEmitter();
40+
mockChildProc.stdout = new EventEmitter();
41+
mockChildProc.stderr = new EventEmitter();
42+
mockChildProc.killed = false;
43+
mockChildProc.kill = vi.fn((signal?: NodeJS.Signals) => {
44+
mockChildProc.killed = true;
45+
setTimeout(() => {
46+
mockChildProc.emit("close", signal === "SIGKILL" ? 137 : 143);
47+
}, 100);
48+
return true;
49+
});
50+
51+
mockSpawn.mockReturnValue(mockChildProc);
52+
53+
mockGetIdeInfo = vi.fn().mockResolvedValue({ remoteName: "local" });
54+
mockGetWorkspaceDirs = vi
55+
.fn()
56+
.mockResolvedValue(["file:///tmp/test-workspace"]);
57+
mockOnPartialOutput = vi.fn();
58+
});
59+
60+
afterEach(() => {
61+
vi.restoreAllMocks();
62+
vi.useRealTimers();
63+
});
64+
65+
const createMockExtras = (
66+
overrides: Partial<ToolExtras> = {},
67+
): ToolExtras => {
68+
const mockIde = {
69+
getIdeInfo: mockGetIdeInfo,
70+
getWorkspaceDirs: mockGetWorkspaceDirs,
71+
getIdeSettings: vi.fn(),
72+
getDiff: vi.fn(),
73+
readFile: vi.fn(),
74+
readRangeInFile: vi.fn(),
75+
isTelemetryEnabled: vi.fn(),
76+
getProblems: vi.fn(),
77+
subprocess: vi.fn(),
78+
getWorkspaceConfigs: vi.fn(),
79+
showToast: vi.fn(),
80+
listWorkspaceContents: vi.fn(),
81+
getTerminalContents: vi.fn(),
82+
listFolders: vi.fn(),
83+
getSessionId: vi.fn(),
84+
runCommand: vi.fn(),
85+
showLines: vi.fn(),
86+
saveFile: vi.fn(),
87+
getBranch: vi.fn(),
88+
showDiff: vi.fn(),
89+
getOpenFiles: vi.fn(),
90+
showVirtualFile: vi.fn(),
91+
openFile: vi.fn(),
92+
getRepo: vi.fn(),
93+
pathSep: vi.fn(),
94+
fileExists: vi.fn(),
95+
} as unknown as IDE;
96+
97+
return {
98+
ide: mockIde,
99+
llm: {} as any,
100+
fetch: vi.fn() as any,
101+
tool: {} as any,
102+
config: {} as any,
103+
onPartialOutput: mockOnPartialOutput,
104+
toolCallId: "test-tool-call-id",
105+
...overrides,
106+
};
107+
};
108+
109+
it("should set up timeout when waitForCompletion is true", async () => {
110+
const extras = createMockExtras();
111+
const args = { command: "echo test", waitForCompletion: true };
112+
const resultPromise = runTerminalCommandImpl(args, extras);
113+
await vi.runOnlyPendingTimersAsync();
114+
expect(setTimeoutSpy).toHaveBeenCalledWith(expect.any(Function), 120_000);
115+
mockChildProc.emit("close", 0);
116+
await resultPromise;
117+
expect(clearTimeoutSpy).toHaveBeenCalled();
118+
});
119+
120+
it("should NOT set up timeout when waitForCompletion is false", async () => {
121+
const extras = createMockExtras();
122+
const args = { command: "sleep 10", waitForCompletion: false };
123+
const result = await runTerminalCommandImpl(args, extras);
124+
const timeoutCalls = setTimeoutSpy.mock.calls.filter(
125+
(call: any[]) => call[1] === 120_000,
126+
);
127+
expect(timeoutCalls.length).toBe(0);
128+
expect(result[0].status).toContain("background");
129+
});
130+
131+
it("should kill process when timeout fires (streaming)", async () => {
132+
const extras = createMockExtras();
133+
const args = { command: "sleep 300", waitForCompletion: true };
134+
const resultPromise = runTerminalCommandImpl(args, extras);
135+
await vi.runOnlyPendingTimersAsync();
136+
await vi.advanceTimersByTimeAsync(120_000);
137+
expect(mockChildProc.kill).toHaveBeenCalledWith("SIGTERM");
138+
await vi.advanceTimersByTimeAsync(5_000);
139+
await vi.runAllTimersAsync();
140+
const result = await resultPromise;
141+
expect(result[0].content).toContain(
142+
"[Timeout: process killed after 2 minutes]",
143+
);
144+
});
145+
146+
it("should clear timeout on normal process exit", async () => {
147+
const extras = createMockExtras();
148+
const args = { command: "echo quick", waitForCompletion: true };
149+
const resultPromise = runTerminalCommandImpl(args, extras);
150+
// Flush async setup (getIdeInfo, getWorkspaceDirs) without advancing timer time
151+
await vi.advanceTimersByTimeAsync(0);
152+
mockChildProc.stdout.emit("data", Buffer.from("quick\n"));
153+
mockChildProc.emit("close", 0);
154+
await resultPromise;
155+
expect(clearTimeoutSpy).toHaveBeenCalled();
156+
expect(mockChildProc.kill).not.toHaveBeenCalled();
157+
});
158+
159+
it("should clear SIGKILL timeout when process exits between SIGTERM and SIGKILL grace period", async () => {
160+
const extras = createMockExtras();
161+
const args = { command: "sleep 300", waitForCompletion: true };
162+
const resultPromise = runTerminalCommandImpl(args, extras);
163+
164+
// Let initial setup complete
165+
await vi.runOnlyPendingTimersAsync();
166+
167+
// Advance to trigger main timeout (120s) — SIGTERM sent, SIGKILL timer started
168+
await vi.advanceTimersByTimeAsync(120_000);
169+
expect(mockChildProc.kill).toHaveBeenCalledWith("SIGTERM");
170+
expect(mockChildProc.kill).toHaveBeenCalledTimes(1);
171+
172+
// Process exits gracefully after 2 seconds (before 5s grace period)
173+
await vi.advanceTimersByTimeAsync(2_000);
174+
mockChildProc.emit("close", 143); // SIGTERM exit code
175+
176+
// Wait for promise to resolve
177+
await resultPromise;
178+
179+
// Advance past the SIGKILL grace period (3 more seconds)
180+
await vi.advanceTimersByTimeAsync(3_000);
181+
182+
// Verify SIGKILL was NOT called (timer was cleared)
183+
expect(mockChildProc.kill).toHaveBeenCalledTimes(1); // Only SIGTERM
184+
expect(mockChildProc.kill).not.toHaveBeenCalledWith("SIGKILL");
185+
});
186+
});

core/tools/implementations/runTerminalCommand.ts

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,10 @@ import iconv from "iconv-lite";
22
import childProcess from "node:child_process";
33
import os from "node:os";
44
import { ContinueError, ContinueErrorReason } from "../../util/errors";
5+
6+
// Default timeout for terminal commands (2 minutes)
7+
const DEFAULT_TOOL_TIMEOUT_MS = 120_000;
8+
59
// Automatically decode the buffer according to the platform to avoid garbled Chinese
610
function getDecodedOutput(data: Buffer): string {
711
if (process.platform === "win32") {
@@ -133,6 +137,8 @@ export const runTerminalCommandImpl: ToolImpl = async (args, extras) => {
133137

134138
return new Promise((resolve, reject) => {
135139
let terminalOutput = "";
140+
let timeoutId: ReturnType<typeof setTimeout> | undefined;
141+
let sigkillTimeoutId: ReturnType<typeof setTimeout> | undefined;
136142

137143
if (!waitForCompletion) {
138144
const status = "Command is running in the background...";
@@ -168,6 +174,41 @@ export const runTerminalCommandImpl: ToolImpl = async (args, extras) => {
168174
);
169175
}
170176

177+
// Set up timeout for waitForCompletion mode
178+
if (waitForCompletion) {
179+
timeoutId = setTimeout(() => {
180+
if (!childProc.killed) {
181+
terminalOutput +=
182+
"\n[Timeout: process killed after 2 minutes]\n";
183+
184+
// Update UI with timeout message
185+
if (extras.onPartialOutput) {
186+
extras.onPartialOutput({
187+
toolCallId,
188+
contextItems: [
189+
{
190+
name: "Terminal",
191+
description: "Terminal command output",
192+
content: terminalOutput,
193+
status: "Command timed out",
194+
},
195+
],
196+
});
197+
}
198+
199+
// Try graceful termination first
200+
childProc.kill("SIGTERM");
201+
202+
// Force kill after 5 seconds if still running
203+
sigkillTimeoutId = setTimeout(() => {
204+
if (!childProc.killed) {
205+
childProc.kill("SIGKILL");
206+
}
207+
}, 5_000);
208+
}
209+
}, DEFAULT_TOOL_TIMEOUT_MS);
210+
}
211+
171212
childProc.stdout?.on("data", (data) => {
172213
// Skip if this process has been backgrounded
173214
if (isProcessBackgrounded(toolCallId)) return;
@@ -240,6 +281,16 @@ export const runTerminalCommandImpl: ToolImpl = async (args, extras) => {
240281
}
241282

242283
childProc.on("close", (code) => {
284+
// Clear timeout on normal completion
285+
if (timeoutId) {
286+
clearTimeout(timeoutId);
287+
}
288+
289+
// Clear inner SIGKILL timeout if process exits before grace period
290+
if (sigkillTimeoutId) {
291+
clearTimeout(sigkillTimeoutId);
292+
}
293+
243294
// Clean up process tracking
244295
if (toolCallId) {
245296
if (isProcessBackgrounded(toolCallId)) {
@@ -296,6 +347,11 @@ export const runTerminalCommandImpl: ToolImpl = async (args, extras) => {
296347
});
297348

298349
childProc.on("error", (error) => {
350+
// Clear timeout on error
351+
if (timeoutId) {
352+
clearTimeout(timeoutId);
353+
}
354+
299355
// Clean up process tracking
300356
if (toolCallId) {
301357
if (isProcessBackgrounded(toolCallId)) {
@@ -325,6 +381,9 @@ export const runTerminalCommandImpl: ToolImpl = async (args, extras) => {
325381
getShellCommand(command);
326382
const output = await new Promise<{ stdout: string; stderr: string }>(
327383
(resolve, reject) => {
384+
let timeoutId: ReturnType<typeof setTimeout> | undefined;
385+
let sigkillTimeoutId: ReturnType<typeof setTimeout> | undefined;
386+
328387
const childProc = childProcess.spawn(
329388
nonStreamingShell,
330389
nonStreamingArgs,
@@ -342,6 +401,23 @@ export const runTerminalCommandImpl: ToolImpl = async (args, extras) => {
342401
let stdout = "";
343402
let stderr = "";
344403

404+
// Set up timeout
405+
timeoutId = setTimeout(() => {
406+
if (!childProc.killed) {
407+
stderr += "\n[Timeout: process killed after 2 minutes]\n";
408+
409+
// Try graceful termination first
410+
childProc.kill("SIGTERM");
411+
412+
// Force kill after 5 seconds if still running
413+
sigkillTimeoutId = setTimeout(() => {
414+
if (!childProc.killed) {
415+
childProc.kill("SIGKILL");
416+
}
417+
}, 5_000);
418+
}
419+
}, DEFAULT_TOOL_TIMEOUT_MS);
420+
345421
childProc.stdout?.on("data", (data) => {
346422
stdout += getDecodedOutput(data);
347423
});
@@ -351,6 +427,16 @@ export const runTerminalCommandImpl: ToolImpl = async (args, extras) => {
351427
});
352428

353429
childProc.on("close", (code) => {
430+
// Clear outer timeout
431+
if (timeoutId) {
432+
clearTimeout(timeoutId);
433+
}
434+
435+
// Clear inner SIGKILL timeout if process exits before grace period
436+
if (sigkillTimeoutId) {
437+
clearTimeout(sigkillTimeoutId);
438+
}
439+
354440
// Clean up process tracking
355441
if (toolCallId) {
356442
removeRunningProcess(toolCallId);

0 commit comments

Comments
 (0)