diff --git a/packages/core/src/services/shellExecutionService.test.ts b/packages/core/src/services/shellExecutionService.test.ts index 77de13de3a..c99bb43292 100644 --- a/packages/core/src/services/shellExecutionService.test.ts +++ b/packages/core/src/services/shellExecutionService.test.ts @@ -870,6 +870,77 @@ describe('ShellExecutionService', () => { expect(ShellExecutionService['activePtys'].size).toBe(0); }); + + it('should destroy the PTY when kill() is called', async () => { + // Execute a command to populate activePtys + const abortController = new AbortController(); + await ShellExecutionService.execute( + 'long-running', + '/test/dir', + onOutputEventMock, + abortController.signal, + true, + shellExecutionConfig, + ); + await new Promise((resolve) => process.nextTick(resolve)); + + const pid = mockPtyProcess.pid; + const activePty = ShellExecutionService['activePtys'].get(pid); + expect(activePty).toBeTruthy(); + + // Spy on the actual stored object's destroy + const storedDestroySpy = vi.spyOn( + activePty!.ptyProcess as never as { destroy: () => void }, + 'destroy', + ); + + ShellExecutionService.kill(pid); + + expect(storedDestroySpy).toHaveBeenCalled(); + expect(ShellExecutionService['activePtys'].has(pid)).toBe(false); + }); + + it('should destroy the PTY when an exception occurs after spawn in executeWithPty', async () => { + // Simulate: spawn succeeds, Promise executor runs fine (pid accesses 1-2), + // but the return statement `{ pid: ptyProcess.pid }` (access 3) throws. + // The catch block should call spawnedPty.destroy() to release the fd. + const destroySpy = vi.fn(); + let pidAccessCount = 0; + const faultyPty = { + onData: vi.fn(), + onExit: vi.fn(), + write: vi.fn(), + kill: vi.fn(), + resize: vi.fn(), + destroy: destroySpy, + get pid() { + pidAccessCount++; + // Accesses 1-2 are inside the Promise executor (setup). + // Access 3 is at `return { pid: ptyProcess.pid, result }`, + // outside the Promise — caught by the outer try/catch. + if (pidAccessCount > 2) { + throw new Error('Simulated post-spawn failure on pid access'); + } + return 77777; + }, + }; + mockPtySpawn.mockReturnValueOnce(faultyPty); + + const handle = await ShellExecutionService.execute( + 'will-fail-after-spawn', + '/test/dir', + onOutputEventMock, + new AbortController().signal, + true, + shellExecutionConfig, + ); + + const result = await handle.result; + expect(result.exitCode).toBe(1); + expect(result.error).toBeTruthy(); + // The catch block must call destroy() on spawnedPty to prevent fd leak + expect(destroySpy).toHaveBeenCalled(); + }); }); }); diff --git a/packages/core/src/services/shellExecutionService.ts b/packages/core/src/services/shellExecutionService.ts index fdb2ca79b5..e393767148 100644 --- a/packages/core/src/services/shellExecutionService.ts +++ b/packages/core/src/services/shellExecutionService.ts @@ -552,6 +552,8 @@ export class ShellExecutionService { // This should not happen, but as a safeguard... throw new Error('PTY implementation not found'); } + let spawnedPty: IPty | undefined; + try { const cols = shellExecutionConfig.terminalWidth ?? 80; const rows = shellExecutionConfig.terminalHeight ?? 30; @@ -585,6 +587,8 @@ export class ShellExecutionService { }, handleFlowControl: true, }); + // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion + spawnedPty = ptyProcess as IPty; const result = new Promise((resolve) => { this.activeResolvers.set(ptyProcess.pid, resolve); @@ -882,6 +886,15 @@ export class ShellExecutionService { } catch (e) { // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion const error = e as Error; + + if (spawnedPty) { + try { + (spawnedPty as IPty & { destroy?: () => void }).destroy?.(); + } catch { + // Ignore errors during cleanup + } + } + if (error.message.includes('posix_spawnp failed')) { onOutputEvent({ type: 'data', @@ -1008,6 +1021,11 @@ export class ShellExecutionService { this.activeChildProcesses.delete(pid); } else if (activePty) { killProcessGroup({ pid, pty: activePty.ptyProcess }).catch(() => {}); + try { + (activePty.ptyProcess as IPty & { destroy?: () => void }).destroy?.(); + } catch { + // Ignore errors during cleanup + } this.activePtys.delete(pid); }