diff --git a/packages/core/src/services/shellExecutionService.test.ts b/packages/core/src/services/shellExecutionService.test.ts index 4dfc48918..e63fba28d 100644 --- a/packages/core/src/services/shellExecutionService.test.ts +++ b/packages/core/src/services/shellExecutionService.test.ts @@ -589,7 +589,7 @@ describe('ShellExecutionService child_process fallback', () => { expect(result.error).toBeNull(); expect(result.aborted).toBe(false); expect(result.output).toBe('file1.txt\na warning'); - expect(handle.pid).toBe(undefined); + expect(handle.pid).toBe(12345); expect(onOutputEventMock).toHaveBeenCalledWith({ type: 'data', @@ -829,7 +829,7 @@ describe('ShellExecutionService child_process fallback', () => { [], expect.objectContaining({ shell: true, - detached: false, + detached: true, }), ); }); diff --git a/packages/core/src/services/shellExecutionService.ts b/packages/core/src/services/shellExecutionService.ts index be0c26ff7..c870b5f4e 100644 --- a/packages/core/src/services/shellExecutionService.ts +++ b/packages/core/src/services/shellExecutionService.ts @@ -7,7 +7,7 @@ import stripAnsi from 'strip-ansi'; import type { PtyImplementation } from '../utils/getPty.js'; import { getPty } from '../utils/getPty.js'; -import { spawn as cpSpawn } from 'node:child_process'; +import { spawn as cpSpawn, spawnSync } from 'node:child_process'; import { TextDecoder } from 'node:util'; import os from 'node:os'; import type { IPty } from '@lydell/node-pty'; @@ -98,6 +98,48 @@ const getFullBufferText = (terminal: pkg.Terminal): string => { return lines.join('\n').trimEnd(); }; +interface ProcessCleanupStrategy { + killPty(pid: number, pty: ActivePty): void; + killChildProcesses(pids: Set): void; +} + +const windowsStrategy: ProcessCleanupStrategy = { + killPty: (_pid, pty) => { + pty.ptyProcess.kill(); + }, + killChildProcesses: (pids) => { + if (pids.size > 0) { + try { + const args = ['/f', '/t']; + for (const pid of pids) { + args.push('/pid', pid.toString()); + } + spawnSync('taskkill', args); + } catch { + // ignore + } + } + }, +}; + +const posixStrategy: ProcessCleanupStrategy = { + killPty: (pid, _pty) => { + process.kill(-pid, 'SIGKILL'); + }, + killChildProcesses: (pids) => { + for (const pid of pids) { + try { + process.kill(-pid, 'SIGKILL'); + } catch { + // ignore + } + } + }, +}; + +const getCleanupStrategy = () => + os.platform() === 'win32' ? windowsStrategy : posixStrategy; + /** * A centralized service for executing shell commands with robust process * management, cross-platform compatibility, and streaming output capabilities. @@ -106,6 +148,29 @@ const getFullBufferText = (terminal: pkg.Terminal): string => { export class ShellExecutionService { private static activePtys = new Map(); + private static activeChildProcesses = new Set(); + + static cleanup() { + const strategy = getCleanupStrategy(); + // Cleanup PTYs + for (const [pid, pty] of this.activePtys) { + try { + strategy.killPty(pid, pty); + } catch { + // ignore + } + } + + // Cleanup child processes + strategy.killChildProcesses(this.activeChildProcesses); + } + + static { + process.on('exit', () => { + ShellExecutionService.cleanup(); + }); + } + /** * Executes a shell command using `node-pty`, capturing all output and lifecycle events. * @@ -164,7 +229,7 @@ export class ShellExecutionService { stdio: ['ignore', 'pipe', 'pipe'], windowsVerbatimArguments: true, shell: isWindows ? true : 'bash', - detached: !isWindows, + detached: true, env: { ...process.env, QWEN_CODE: '1', @@ -281,9 +346,13 @@ export class ShellExecutionService { abortSignal.addEventListener('abort', abortHandler, { once: true }); + if (child.pid) { + this.activeChildProcesses.add(child.pid); + } + child.on('exit', (code, signal) => { if (child.pid) { - this.activePtys.delete(child.pid); + this.activeChildProcesses.delete(child.pid); } handleExit(code, signal); }); @@ -310,7 +379,7 @@ export class ShellExecutionService { } }); - return { pid: undefined, result }; + return { pid: child.pid, result }; } catch (e) { const error = e as Error; return { diff --git a/packages/core/src/tools/shell.test.ts b/packages/core/src/tools/shell.test.ts index 8c98045dd..8b6788a70 100644 --- a/packages/core/src/tools/shell.test.ts +++ b/packages/core/src/tools/shell.test.ts @@ -248,7 +248,7 @@ describe('ShellTool', () => { wrappedCommand, '/test/dir', expect.any(Function), - mockAbortSignal, + expect.any(AbortSignal), false, {}, ); @@ -275,7 +275,7 @@ describe('ShellTool', () => { wrappedCommand, expect.any(String), expect.any(Function), - mockAbortSignal, + expect.any(AbortSignal), false, {}, ); @@ -300,7 +300,7 @@ describe('ShellTool', () => { wrappedCommand, expect.any(String), expect.any(Function), - mockAbortSignal, + expect.any(AbortSignal), false, {}, ); @@ -325,7 +325,7 @@ describe('ShellTool', () => { wrappedCommand, expect.any(String), expect.any(Function), - mockAbortSignal, + expect.any(AbortSignal), false, {}, ); @@ -350,7 +350,7 @@ describe('ShellTool', () => { wrappedCommand, '/test/dir/subdir', expect.any(Function), - mockAbortSignal, + expect.any(AbortSignal), false, {}, ); @@ -378,7 +378,7 @@ describe('ShellTool', () => { 'dir', '/test/dir', expect.any(Function), - mockAbortSignal, + expect.any(AbortSignal), false, {}, ); @@ -471,7 +471,7 @@ describe('ShellTool', () => { expect(summarizer.summarizeToolOutput).toHaveBeenCalledWith( expect.any(String), mockConfig.getGeminiClient(), - mockAbortSignal, + expect.any(AbortSignal), 1000, ); expect(result.llmContent).toBe('summarized output'); @@ -580,7 +580,7 @@ describe('ShellTool', () => { ), expect.any(String), expect.any(Function), - mockAbortSignal, + expect.any(AbortSignal), false, {}, ); @@ -610,7 +610,7 @@ describe('ShellTool', () => { ), expect.any(String), expect.any(Function), - mockAbortSignal, + expect.any(AbortSignal), false, {}, ); @@ -640,7 +640,7 @@ describe('ShellTool', () => { ), expect.any(String), expect.any(Function), - mockAbortSignal, + expect.any(AbortSignal), false, {}, ); @@ -699,7 +699,7 @@ describe('ShellTool', () => { expect.stringContaining('npm install'), expect.any(String), expect.any(Function), - mockAbortSignal, + expect.any(AbortSignal), false, {}, ); @@ -728,7 +728,7 @@ describe('ShellTool', () => { expect.stringContaining('git commit'), expect.any(String), expect.any(Function), - mockAbortSignal, + expect.any(AbortSignal), false, {}, ); @@ -758,7 +758,7 @@ describe('ShellTool', () => { ), expect.any(String), expect.any(Function), - mockAbortSignal, + expect.any(AbortSignal), false, {}, ); @@ -794,7 +794,7 @@ describe('ShellTool', () => { expect.stringContaining('git commit -m "Initial commit"'), expect.any(String), expect.any(Function), - mockAbortSignal, + expect.any(AbortSignal), false, {}, ); @@ -831,7 +831,7 @@ describe('ShellTool', () => { ), expect.any(String), expect.any(Function), - mockAbortSignal, + expect.any(AbortSignal), false, {}, ); @@ -962,4 +962,41 @@ spanning multiple lines"`; expect(shellTool.description).toMatchSnapshot(); }); }); + + describe('Windows background execution', () => { + it('should clean up trailing ampersand on Windows for background tasks', async () => { + vi.mocked(os.platform).mockReturnValue('win32'); + const mockAbortSignal = new AbortController().signal; + + const invocation = shellTool.build({ + command: 'npm start &', + is_background: true, + }); + + const promise = invocation.execute(mockAbortSignal); + + // Simulate immediate success (process started) + resolveExecutionPromise({ + rawOutput: Buffer.from(''), + output: '', + exitCode: 0, + signal: null, + error: null, + aborted: false, + pid: 12345, + executionMethod: 'child_process', + }); + + await promise; + + expect(mockShellExecutionService).toHaveBeenCalledWith( + 'npm start', + expect.any(String), + expect.any(Function), + expect.any(AbortSignal), + false, + {}, + ); + }); + }); }); diff --git a/packages/core/src/tools/shell.ts b/packages/core/src/tools/shell.ts index 5354f9251..d7afae599 100644 --- a/packages/core/src/tools/shell.ts +++ b/packages/core/src/tools/shell.ts @@ -143,11 +143,24 @@ export class ShellToolInvocation extends BaseToolInvocation< const shouldRunInBackground = this.params.is_background; let finalCommand = processedCommand; - // If explicitly marked as background and doesn't already end with &, add it - if (shouldRunInBackground && !finalCommand.trim().endsWith('&')) { + // On non-Windows, use & to run in background. + // On Windows, we don't use start /B because it creates a detached process that + // doesn't die when the parent dies. Instead, we rely on the race logic below + // to return early while keeping the process attached (detached: false). + if ( + !isWindows && + shouldRunInBackground && + !finalCommand.trim().endsWith('&') + ) { finalCommand = finalCommand.trim() + ' &'; } + // On Windows, we rely on the race logic below to handle background tasks. + // We just ensure the command string is clean. + if (isWindows && shouldRunInBackground) { + finalCommand = finalCommand.trim().replace(/&+$/, '').trim(); + } + // pgrep is not available on Windows, so we can't get background PIDs const commandToExecute = isWindows ? finalCommand @@ -169,10 +182,6 @@ export class ShellToolInvocation extends BaseToolInvocation< commandToExecute, cwd, (event: ShellOutputEvent) => { - if (!updateOutput) { - return; - } - let shouldUpdate = false; switch (event.type) { @@ -201,7 +210,7 @@ export class ShellToolInvocation extends BaseToolInvocation< } } - if (shouldUpdate) { + if (shouldUpdate && updateOutput) { updateOutput( typeof cumulativeOutput === 'string' ? cumulativeOutput @@ -219,6 +228,21 @@ export class ShellToolInvocation extends BaseToolInvocation< setPidCallback(pid); } + if (shouldRunInBackground) { + // For background tasks, return immediately with PID info + // Note: We cannot reliably detect startup errors for background processes + // since their stdio is typically detached/ignored + const pidMsg = pid ? ` PID: ${pid}` : ''; + const killHint = isWindows + ? ' (Use taskkill /F /T /PID to stop)' + : ' (Use kill to stop)'; + + return { + llmContent: `Background command started.${pidMsg}${killHint}`, + returnDisplay: `Background command started.${pidMsg}${killHint}`, + }; + } + const result = await resultPromise; const backgroundPIDs: number[] = [];