fix(core)#20941: reap orphaned descendant processes on PTY abort (#21124)

Co-authored-by: Spencer <spencertang@google.com>

packages/core/src/services/shellExecutionService.test.ts

@@ -1398,12 +1398,11 @@ describe('ShellExecutionService child_process fallback', () => {
               expectedSignal,
             );
           } else {
-            expect(mockCpSpawn).toHaveBeenCalledWith(expectedCommand, [
-              '/pid',
-              String(mockChildProcess.pid),
-              '/f',
-              '/t',
-            ]);
+            expect(mockCpSpawn).toHaveBeenCalledWith(
+              expectedCommand,
+              ['/pid', String(mockChildProcess.pid), '/f', '/t'],
+              undefined,
+            );
           }
         });
       },

packages/core/src/services/shellExecutionService.ts

@@ -1181,10 +1181,12 @@ export class ShellExecutionService {
     await this.cleanupLogStream(pid);
 
     if (activeChild) {
-      killProcessGroup({ pid }).catch(() => {});
+      await killProcessGroup({ pid }).catch(() => {});
       this.activeChildProcesses.delete(pid);
     } else if (activePty) {
-      killProcessGroup({ pid, pty: activePty.ptyProcess }).catch(() => {});
+      await killProcessGroup({ pid, pty: activePty.ptyProcess }).catch(
+        () => {},
+      );
       try {
         (activePty.ptyProcess as IPty & { destroy?: () => void }).destroy?.();
       } catch {

packages/core/src/utils/process-utils.test.ts

@@ -4,27 +4,37 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
-import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
+import {
+  describe,
+  it,
+  expect,
+  vi,
+  beforeEach,
+  afterEach,
+  type Mock,
+  type MockInstance,
+} from 'vitest';
 import os from 'node:os';
-import { spawn as cpSpawn } from 'node:child_process';
 import { killProcessGroup, SIGKILL_TIMEOUT_MS } from './process-utils.js';
+import { spawnAsync } from './shell-utils.js';
 
 vi.mock('node:os');
-vi.mock('node:child_process');
+vi.mock('./shell-utils.js');
 
 describe('process-utils', () => {
-  const mockProcessKill = vi
-    .spyOn(process, 'kill')
-    .mockImplementation(() => true);
-  const mockSpawn = vi.mocked(cpSpawn);
+  let mockProcessKill: MockInstance;
+  let mockSpawnAsync: Mock;
 
   beforeEach(() => {
     vi.clearAllMocks();
     vi.useFakeTimers();
+    mockProcessKill = vi.spyOn(process, 'kill').mockImplementation(() => true);
+    mockSpawnAsync = vi.mocked(spawnAsync);
   });
 
   afterEach(() => {
     vi.useRealTimers();
+    vi.restoreAllMocks();
   });
 
   describe('killProcessGroup', () => {
@@ -33,7 +43,7 @@ describe('process-utils', () => {
 
       await killProcessGroup({ pid: 1234 });
 
-      expect(mockSpawn).toHaveBeenCalledWith('taskkill', [
+      expect(mockSpawnAsync).toHaveBeenCalledWith('taskkill', [
         '/pid',
         '1234',
         '/f',
@@ -42,14 +52,20 @@ describe('process-utils', () => {
       expect(mockProcessKill).not.toHaveBeenCalled();
     });
 
-    it('should use pty.kill() on Windows if pty is provided', async () => {
+    it('should use pty.kill() on Windows if pty is provided and also taskkill for descendants', async () => {
       vi.mocked(os.platform).mockReturnValue('win32');
       const mockPty = { kill: vi.fn() };
 
       await killProcessGroup({ pid: 1234, pty: mockPty });
 
       expect(mockPty.kill).toHaveBeenCalled();
-      expect(mockSpawn).not.toHaveBeenCalled();
+      // taskkill is also called to reap orphaned descendant processes
+      expect(mockSpawnAsync).toHaveBeenCalledWith('taskkill', [
+        '/pid',
+        '1234',
+        '/f',
+        '/t',
+      ]);
     });
 
     it('should kill the process group on Unix with SIGKILL by default', async () => {
@@ -130,5 +146,23 @@ describe('process-utils', () => {
 
       expect(mockPty.kill).toHaveBeenCalledWith('SIGKILL');
     });
+
+    it('should attempt process group kill on Unix after pty fallback to reap orphaned descendants', async () => {
+      vi.mocked(os.platform).mockReturnValue('linux');
+      // First call (group kill) throws to trigger PTY fallback
+      mockProcessKill.mockImplementationOnce(() => {
+        throw new Error('ESRCH');
+      });
+      // Second call (group kill retry after pty.kill) should succeed
+      mockProcessKill.mockImplementationOnce(() => true);
+      const mockPty = { kill: vi.fn() };
+
+      await killProcessGroup({ pid: 1234, pty: mockPty });
+
+      // Group kill should be called first to ensure it's hit before PTY leader dies
+      expect(mockProcessKill).toHaveBeenCalledWith(-1234, 'SIGKILL');
+      // Then PTY kill should be called
+      expect(mockPty.kill).toHaveBeenCalledWith('SIGKILL');
+    });
   });
 });

packages/core/src/utils/process-utils.ts

@@ -5,7 +5,8 @@
  */
 
 import os from 'node:os';
-import { spawn as cpSpawn } from 'node:child_process';
+
+import { spawnAsync } from './shell-utils.js';
 
 /** Default timeout for SIGKILL escalation on Unix systems. */
 export const SIGKILL_TIMEOUT_MS = 200;
@@ -44,8 +45,12 @@ export async function killProcessGroup(options: KillOptions): Promise<void> {
       } catch {
         // Ignore errors for dead processes
       }
-    } else {
-      cpSpawn('taskkill', ['/pid', pid.toString(), '/f', '/t']);
+    }
+    // Invoke taskkill to ensure the entire tree is terminated and any orphaned descendant processes are reaped.
+    try {
+      await spawnAsync('taskkill', ['/pid', pid.toString(), '/f', '/t']);
+    } catch (_e) {
+      // Ignore errors if the process tree is already dead
     }
     return;
   }
@@ -73,14 +78,24 @@ export async function killProcessGroup(options: KillOptions): Promise<void> {
       if (pty) {
         if (escalate) {
           try {
+            // Attempt the group kill BEFORE the pty session leader dies
+            process.kill(-pid, 'SIGTERM');
             pty.kill('SIGTERM');
             await new Promise((res) => setTimeout(res, SIGKILL_TIMEOUT_MS));
-            if (!isExited()) pty.kill('SIGKILL');
+            if (!isExited()) {
+              try {
+                process.kill(-pid, 'SIGKILL');
+              } catch {
+                // Ignore
+              }
+              pty.kill('SIGKILL');
+            }
           } catch {
             // Ignore
           }
         } else {
           try {
+            process.kill(-pid, 'SIGKILL'); // Group kill first
             pty.kill('SIGKILL');
           } catch {
             // Ignore