fix: improve windows background process handling and cleanup

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';
@@ -106,6 +106,51 @@ const getFullBufferText = (terminal: pkg.Terminal): string => {
 
 export class ShellExecutionService {
   private static activePtys = new Map<number, ActivePty>();
+  private static activeChildProcesses = new Set<number>();
+
+  static {
+    const cleanup = () => {
+      // Cleanup PTYs
+      for (const [pid, pty] of this.activePtys) {
+        try {
+          if (os.platform() === 'win32') {
+            pty.ptyProcess.kill();
+          } else {
+            process.kill(-pid, 'SIGKILL');
+          }
+        } catch {
+          // ignore
+        }
+      }
+
+      // Cleanup child processes
+      for (const pid of this.activeChildProcesses) {
+        try {
+          if (os.platform() === 'win32') {
+            spawnSync('taskkill', ['/pid', pid.toString(), '/f', '/t']);
+          } else {
+            process.kill(-pid, 'SIGKILL');
+          }
+        } catch {
+          // ignore
+        }
+      }
+    };
+
+    process.on('exit', cleanup);
+
+    // Ensure cleanup happens on SIGINT/SIGTERM
+    const signalHandler = () => {
+      process.exit();
+    };
+
+    // We only attach these if we are in a node environment where we can control the process
+    if (typeof process !== 'undefined' && process.on) {
+      process.on('SIGINT', signalHandler);
+      process.on('SIGTERM', signalHandler);
+    }
+  }
+
   /**
    * Executes a shell command using `node-pty`, capturing all output and lifecycle events.
    *
@@ -281,9 +326,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 +359,7 @@ export class ShellExecutionService {
         }
       });
 
-      return { pid: undefined, result };
+      return { pid: child.pid, result };
     } catch (e) {
       const error = e as Error;
       return {

packages/core/src/tools/shell.ts

@@ -29,6 +29,7 @@ import { summarizeToolOutput } from '../utils/summarizer.js';
 import type {
   ShellExecutionConfig,
   ShellOutputEvent,
+  ShellExecutionResult,
 } from '../services/shellExecutionService.js';
 import { ShellExecutionService } from '../services/shellExecutionService.js';
 import { formatMemoryUsage } from '../utils/formatters.js';
@@ -47,6 +48,7 @@ export interface ShellToolParams {
   is_background: boolean;
   description?: string;
   directory?: string;
+  timeout?: number;
 }
 
 export class ShellToolInvocation extends BaseToolInvocation<
@@ -132,6 +134,14 @@ export class ShellToolInvocation extends BaseToolInvocation<
       .toString('hex')}.tmp`;
     const tempFilePath = path.join(os.tmpdir(), tempFileName);
 
+    const timeoutMs = this.params.timeout ?? 3600000;
+    const abortController = new AbortController();
+    const onAbort = () => abortController.abort();
+    signal.addEventListener('abort', onAbort);
+    const timeoutId = setTimeout(() => {
+      abortController.abort();
+    }, timeoutMs);
+
     try {
       // Add co-author to git commit commands
       const processedCommand = this.addCoAuthorToGitCommit(strippedCommand);
@@ -139,11 +149,30 @@ 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, append a keep-alive command to ensure the shell process
+      // stays alive even if the main command exits (e.g. spawns a detached child).
+      // This ensures we always have a valid PID for cleanup.
+      if (isWindows && shouldRunInBackground) {
+        // Remove trailing & if present to avoid syntax errors (e.g. "cmd & & ping")
+        let cmd = finalCommand.trim();
+        while (cmd.endsWith('&')) {
+          cmd = cmd.slice(0, -1).trim();
+        }
+        finalCommand = cmd + ' & ping -n 86400 127.0.0.1 >nul';
+      }
+
       // pgrep is not available on Windows, so we can't get background PIDs
       const commandToExecute = isWindows
         ? finalCommand
@@ -206,7 +235,7 @@ export class ShellToolInvocation extends BaseToolInvocation<
               lastUpdateTime = Date.now();
             }
           },
-          signal,
+          abortController.signal,
           this.config.getShouldUseNodePtyShell(),
           shellExecutionConfig ?? {},
         );
@@ -215,7 +244,34 @@ export class ShellToolInvocation extends BaseToolInvocation<
         setPidCallback(pid);
       }
 
-      const result = await resultPromise;
+      let result: ShellExecutionResult;
+      if (shouldRunInBackground && isWindows) {
+        // For Windows background tasks, we wait a short time to catch immediate errors.
+        // If it's still running, we return early.
+        const startupDelay = 1000;
+        const raceResult = await Promise.race([
+          resultPromise,
+          new Promise<null>((resolve) =>
+            setTimeout(() => resolve(null), startupDelay),
+          ),
+        ]);
+
+        if (raceResult === null) {
+          // Timeout reached, process is still running.
+          const pidMsg = pid ? ` PID: ${pid}` : '';
+          const winHint = isWindows
+            ? ' (Note: Use taskkill /F /T /PID <pid> to stop)'
+            : '';
+          return {
+            llmContent: `Background command started.${pidMsg}${winHint}`,
+            returnDisplay: `Background command started.${pidMsg}${winHint}`,
+          };
+        } else {
+          result = raceResult;
+        }
+      } else {
+        result = await resultPromise;
+      }
 
       const backgroundPIDs: number[] = [];
       if (os.platform() !== 'win32') {
@@ -321,6 +377,8 @@ export class ShellToolInvocation extends BaseToolInvocation<
         ...executionError,
       };
     } finally {
+      clearTimeout(timeoutId);
+      signal.removeEventListener('abort', onAbort);
       if (fs.existsSync(tempFilePath)) {
         fs.unlinkSync(tempFilePath);
       }
@@ -454,6 +512,11 @@ export class ShellTool extends BaseDeclarativeTool<
             description:
               '(OPTIONAL) The absolute path of the directory to run the command in. If not provided, the project root directory is used. Must be a directory within the workspace and must already exist.',
           },
+          timeout: {
+            type: 'number',
+            description:
+              '(OPTIONAL) The timeout in milliseconds for the command. If not provided, a default timeout (1 hour) is applied.',
+          },
         },
         required: ['command', 'is_background'],
       },
@@ -478,6 +541,9 @@ export class ShellTool extends BaseDeclarativeTool<
     if (!params.command.trim()) {
       return 'Command cannot be empty.';
     }
+    if (params.timeout !== undefined && params.timeout <= 0) {
+      return 'Timeout must be a positive number.';
+    }
     if (getCommandRoots(params.command).length === 0) {
       return 'Could not identify command root to obtain permission from user.';
     }