diff --git a/img.png b/img.png new file mode 100644 index 0000000000..ab9f0bafcd Binary files /dev/null and b/img.png differ diff --git a/packages/core/src/services/shellExecutionService.test.ts b/packages/core/src/services/shellExecutionService.test.ts index 771b0615dc..5805930673 100644 --- a/packages/core/src/services/shellExecutionService.test.ts +++ b/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, + ); } }); }, diff --git a/packages/core/src/services/shellExecutionService.ts b/packages/core/src/services/shellExecutionService.ts index 50052fb781..d92f395706 100644 --- a/packages/core/src/services/shellExecutionService.ts +++ b/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 { diff --git a/packages/core/src/utils/process-utils.test.ts b/packages/core/src/utils/process-utils.test.ts index 9da6048a15..8bfcd506f8 100644 --- a/packages/core/src/utils/process-utils.test.ts +++ b/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'); + }); }); }); diff --git a/packages/core/src/utils/process-utils.ts b/packages/core/src/utils/process-utils.ts index 74f802718f..9ea7b00d0f 100644 --- a/packages/core/src/utils/process-utils.ts +++ b/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 { } 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 { 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