🤖 fix: use PGID for Windows MSYS2 process termination (no /proc on macOS)

Author: ethanndickson

src/node/runtime/LocalBackgroundHandle.ts

@@ -21,6 +21,12 @@ export class LocalBackgroundHandle implements BackgroundHandle {
 
   constructor(
     private readonly pid: number,
+    /**
+     * Process group ID for termination.
+     * - Unix (setsid=true): equals pid, since setsid makes process a session/group leader
+     * - Windows MSYS2 (setsid=false): actual PGID from /proc, used with kill -PGID
+     */
+    private readonly pgid: number,
     public readonly outputDir: string
   ) {}
 
@@ -53,31 +59,23 @@ export class LocalBackgroundHandle implements BackgroundHandle {
 
     const exitCodePath = path.join(this.outputDir, "exit_code");
 
-    // Windows: convert MSYS2 PID to Windows PID and use taskkill
-    // Must run via bash because /proc/PID/winpid is MSYS2's virtual filesystem,
-    // not visible to Node.js running as a native Windows process
+    // Windows: use MSYS2's kill command with negative PGID to kill process group
+    // taskkill doesn't work because MSYS2's process tree doesn't match Windows' process tree
+    // MSYS2's kill understands its own process groups, so kill -PGID works correctly
     if (process.platform === "win32") {
       try {
-        // Script reads winpid from MSYS2's /proc, then uses taskkill
-        // taskkill /F = force, /T = kill child processes
-        // Double slashes needed because bash interprets single slash as path
-        const terminateScript = `
-          if [ -f /proc/${this.pid}/winpid ]; then
-            winpid=$(cat /proc/${this.pid}/winpid)
-            taskkill //PID $winpid //F //T 2>/dev/null
-          fi
-          kill -9 ${this.pid} 2>/dev/null || true
-        `;
-        log.debug(`LocalBackgroundHandle: Terminating MSYS2 PID ${this.pid} via bash`);
+        // Use PGID to kill entire process group via MSYS2's kill command
+        const terminateScript = `kill -9 -${this.pgid} 2>/dev/null || true`;
+        log.debug(`LocalBackgroundHandle: Terminating MSYS2 process group ${this.pgid} via bash`);
         using proc = execAsync(terminateScript, { shell: getBashPath() });
         await proc.result;
       } catch (error) {
-        // Process already dead or winpid unavailable
+        // Process already dead
         log.debug(
           `LocalBackgroundHandle: Windows terminate error: ${error instanceof Error ? error.message : String(error)}`
         );
       }
-      // Write exit code - trap may not fire with taskkill
+      // Write exit code - trap may not fire with kill -9
       try {
         await fs.access(exitCodePath);
       } catch {
@@ -89,11 +87,11 @@ export class LocalBackgroundHandle implements BackgroundHandle {
     }
 
     // Unix: use process group signals
-    const pgid = -this.pid; // Negative PID = process group
+    const negativePgid = -this.pgid; // Negative PGID = process group
 
     try {
-      log.debug(`LocalBackgroundHandle: Sending SIGTERM to process group ${pgid}`);
-      process.kill(pgid, "SIGTERM");
+      log.debug(`LocalBackgroundHandle: Sending SIGTERM to process group ${negativePgid}`);
+      process.kill(negativePgid, "SIGTERM");
 
       // Wait 2 seconds for graceful shutdown
       await new Promise((resolve) => setTimeout(resolve, 2000));
@@ -108,8 +106,8 @@ export class LocalBackgroundHandle implements BackgroundHandle {
       }
 
       if (stillRunning) {
-        log.debug(`LocalBackgroundHandle: Process still running, sending SIGKILL to group ${pgid}`);
-        process.kill(pgid, "SIGKILL");
+        log.debug(`LocalBackgroundHandle: Process still running, sending SIGKILL to group ${negativePgid}`);
+        process.kill(negativePgid, "SIGKILL");
 
         // Write exit code for SIGKILL since we had to force kill
         await fs.writeFile(exitCodePath, String(EXIT_CODE_SIGKILL)).catch(() => {

src/node/runtime/LocalBaseRuntime.ts

@@ -382,14 +382,19 @@ export abstract class LocalBaseRuntime implements Runtime {
       using proc = execAsync(spawnCommand, { shell: getBashPath() });
       const result = await proc.result;
 
-      const pid = parseInt(result.stdout.trim(), 10);
+      // Parse "PID PGID" from output
+      // On Unix with setsid, both values are the same (process is group leader)
+      // On Windows MSYS2, PGID comes from /proc/$!/pgid
+      const [pidStr, pgidStr] = result.stdout.trim().split(/\s+/);
+      const pid = parseInt(pidStr, 10);
+      const pgid = parseInt(pgidStr, 10);
       if (isNaN(pid) || pid <= 0) {
         log.debug(`LocalBaseRuntime.spawnBackground: Invalid PID: ${result.stdout}`);
         return { success: false, error: `Failed to get valid PID from spawn: ${result.stdout}` };
       }
 
-      log.debug(`LocalBaseRuntime.spawnBackground: Spawned with PID ${pid}`);
-      const handle = new LocalBackgroundHandle(pid, outputDir);
+      log.debug(`LocalBaseRuntime.spawnBackground: Spawned with PID ${pid}, PGID ${pgid}`);
+      const handle = new LocalBackgroundHandle(pid, isNaN(pgid) ? pid : pgid, outputDir);
       return { success: true, handle, pid };
     } catch (e) {
       const err = e as Error;

src/node/runtime/backgroundCommands.ts

@@ -94,19 +94,28 @@ export interface SpawnCommandOptions {
  * setsid: creates new session, process becomes group leader (enables kill -PID)
  * nohup: ignores SIGHUP (survives terminal hangup)
  *
- * Returns PID via echo $! (correct because & is inside subshell)
+ * Returns "PID PGID" via echo:
+ * - Unix with setsid: outputs "PID PID" (process is session leader, so PID == PGID)
+ * - Windows MSYS2 without setsid: outputs "PID PGID" where PGID is read from /proc/$!/pgid
+ *   (MSYS2 provides /proc filesystem; macOS does not have /proc)
  */
 export function buildSpawnCommand(options: SpawnCommandOptions): string {
   const bash = options.bashPath ?? "bash";
   const nicePrefix = options.niceness !== undefined ? `nice -n ${options.niceness} ` : "";
   const setsidPrefix = options.useSetsid === false ? "" : "setsid ";
   const quotePath = options.quotePath ?? shellQuote;
 
+  // With setsid (Unix): process becomes session leader, so PID == PGID
+  // Without setsid (Windows MSYS2): must read PGID from /proc (MSYS2 provides this)
+  // CRITICAL: We can't run this on macOS, since it doesn't have /proc
+  const pgidExpr =
+    options.useSetsid === false ? '$(cat /proc/$!/pgid 2>/dev/null || echo $!)' : '$!';
+
   return (
     `(${nicePrefix}${setsidPrefix}nohup ${shellQuote(bash)} -c ${shellQuote(options.wrapperScript)} ` +
     `> ${quotePath(options.stdoutPath)} ` +
     `2> ${quotePath(options.stderrPath)} ` +
-    `< /dev/null & echo $!)`
+    `< /dev/null & echo "$! ${pgidExpr}")`
   );
 }