feat(core): add signal.reason convention for ShellExecutionService (#3831 PR-1 of 3)

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

@@ -17,7 +17,10 @@ import EventEmitter from 'node:events';
 import type { Readable } from 'node:stream';
 import { type ChildProcess } from 'node:child_process';
 import pkg from '@xterm/headless';
-import type { ShellOutputEvent } from './shellExecutionService.js';
+import type {
+  ShellAbortReason,
+  ShellOutputEvent,
+} from './shellExecutionService.js';
 import { ShellExecutionService } from './shellExecutionService.js';
 import type { AnsiOutput } from '../utils/terminalSerializer.js';
 
@@ -225,8 +228,12 @@ describe('ShellExecutionService', () => {
     };
     mockPtyProcess.pid = 12345;
     mockPtyProcess.kill = vi.fn();
-    mockPtyProcess.onData = vi.fn();
-    mockPtyProcess.onExit = vi.fn();
+    // node-pty's onData/onExit return IDisposable; the production
+    // background-promote path calls .dispose() on those handles to detach
+    // its listeners cleanly. Mock them to return a disposable stub so the
+    // promote path doesn't crash on `undefined.dispose()`.
+    mockPtyProcess.onData = vi.fn().mockReturnValue({ dispose: vi.fn() });
+    mockPtyProcess.onExit = vi.fn().mockReturnValue({ dispose: vi.fn() });
     mockPtyProcess.write = vi.fn();
     mockPtyProcess.resize = vi.fn();
 
@@ -608,6 +615,113 @@ describe('ShellExecutionService', () => {
       expect(result.aborted).toBe(true);
       // The process kill is mocked, so we just check that the flag is set.
     });
+
+    it('signal.reason = { kind: "cancel" } still tree-kills (same as default)', async () => {
+      const { result } = await simulateExecution(
+        'sleep 10',
+        (pty, abortController) => {
+          abortController.abort({ kind: 'cancel' } satisfies ShellAbortReason);
+          pty.onExit.mock.calls[0][0]({ exitCode: 1, signal: null });
+        },
+      );
+
+      expect(result.aborted).toBe(true);
+      expect(result.promoted).toBeUndefined();
+      // The default kill path runs: SIGTERM via process.kill on the
+      // process-group pid. Pinning that we DID try to kill — i.e., reason
+      // === 'cancel' is NOT mistakenly routed through the background branch.
+      expect(mockProcessKill).toHaveBeenCalledWith(
+        -mockPtyProcess.pid,
+        'SIGTERM',
+      );
+    });
+
+    it('signal.reason = { kind: "background" } skips kill and resolves with promoted: true', async () => {
+      // Critical: do NOT fire onExit — the child is still alive after the
+      // background-promote abort. The result Promise must resolve via the
+      // abort handler's own immediate resolve, not via the exit handler.
+      const { result } = await simulateExecution(
+        'tail -f /tmp/never.log',
+        (_pty, abortController) => {
+          abortController.abort({
+            kind: 'background',
+            shellId: 'bg_test123',
+          } satisfies ShellAbortReason);
+        },
+      );
+
+      expect(result.aborted).toBe(true);
+      expect(result.promoted).toBe(true);
+      expect(result.exitCode).toBeNull();
+      expect(result.signal).toBeNull();
+      expect(result.error).toBeNull();
+      expect(result.pid).toBe(mockPtyProcess.pid);
+      // Verify the kill path did NOT run: neither the PTY's own kill() nor
+      // process.kill on the group pid. Caller now owns the child.
+      expect(mockPtyProcess.kill).not.toHaveBeenCalled();
+      expect(mockProcessKill).not.toHaveBeenCalledWith(
+        -mockPtyProcess.pid,
+        'SIGTERM',
+      );
+      expect(mockProcessKill).not.toHaveBeenCalledWith(
+        -mockPtyProcess.pid,
+        'SIGKILL',
+      );
+    });
+
+    it('post-promotion: PTY data is no longer routed to onOutputEvent (handoff boundary)', async () => {
+      // Pin the ownership contract: after background-promote, PTY data
+      // arriving on the still-running child must NOT surface through the
+      // foreground execute()'s onOutputEvent (the caller has its own
+      // listeners now). Without dataDisposable.dispose() in the abort
+      // handler, the listener-retention bug would let post-promote bytes
+      // leak into the foreground consumer.
+      const { result } = await simulateExecution(
+        'tail -f /tmp/never.log',
+        (pty, abortController) => {
+          // Data BEFORE promote — fed via the live onData listener so it
+          // reaches the foreground onOutputEvent normally.
+          pty.onData.mock.calls[0][0]('pre-promote-data\n');
+          abortController.abort({
+            kind: 'background',
+            shellId: 'bg_test123',
+          } satisfies ShellAbortReason);
+        },
+      );
+      expect(result.promoted).toBe(true);
+
+      // The disposable returned by mockPtyProcess.onData was disposed by
+      // the abort handler — verify by calling .dispose's mock.
+      const dataDisposableStub = mockPtyProcess.onData.mock.results[0]
+        .value as { dispose: Mock };
+      expect(dataDisposableStub.dispose).toHaveBeenCalled();
+      const exitDisposableStub = mockPtyProcess.onExit.mock.results[0]
+        .value as { dispose: Mock };
+      expect(exitDisposableStub.dispose).toHaveBeenCalled();
+    });
+
+    it('post-promotion: PTY exit does NOT re-resolve the result (already resolved with promoted)', async () => {
+      // Pin: even if the still-running child later exits naturally and the
+      // caller's own exit listener fires, our foreground result Promise
+      // must NOT be re-resolved with a different shape (Promise can only
+      // resolve once). The exit disposable being disposed prevents our
+      // own onExit from firing at all in the first place — but verify the
+      // final resolved shape stays `promoted: true` regardless.
+      const { result } = await simulateExecution(
+        'tail -f /tmp/never.log',
+        (_pty, abortController) => {
+          abortController.abort({
+            kind: 'background',
+            shellId: 'bg_test123',
+          } satisfies ShellAbortReason);
+        },
+      );
+
+      // Resolved as promoted, with no exit info from a post-promote exit.
+      expect(result.promoted).toBe(true);
+      expect(result.exitCode).toBeNull();
+      expect(result.signal).toBeNull();
+    });
   });
 
   describe('Binary Output', () => {
@@ -1139,6 +1253,129 @@ describe('ShellExecutionService child_process fallback', () => {
       },
     );
 
+    it('signal.reason = { kind: "cancel" } still tree-kills (same as default)', async () => {
+      mockPlatform.mockReturnValue('linux');
+      const { result } = await simulateExecution(
+        'sleep 10',
+        (cp, abortController) => {
+          abortController.abort({ kind: 'cancel' } satisfies ShellAbortReason);
+          cp.emit('exit', null, 'SIGKILL');
+          cp.emit('close', null, 'SIGKILL');
+        },
+      );
+
+      expect(result.aborted).toBe(true);
+      expect(result.promoted).toBeUndefined();
+      // Default kill path ran — pin that reason === 'cancel' is NOT
+      // mistakenly routed through the background branch.
+      expect(mockProcessKill).toHaveBeenCalledWith(
+        -mockChildProcess.pid!,
+        'SIGTERM',
+      );
+    });
+
+    it('signal.reason = { kind: "background" } skips kill and resolves with promoted: true', async () => {
+      mockPlatform.mockReturnValue('linux');
+      // Critical: do NOT fire 'exit' — the child is still alive after the
+      // background-promote abort. The result Promise must resolve via the
+      // abort handler's own immediate resolve.
+      const { result } = await simulateExecution(
+        'tail -f /tmp/never.log',
+        (cp, abortController) => {
+          // Emit some output first so the snapshot has content.
+          cp.stdout?.emit('data', Buffer.from('line1\nline2\n'));
+          abortController.abort({
+            kind: 'background',
+            shellId: 'bg_test123',
+          } satisfies ShellAbortReason);
+        },
+      );
+
+      expect(result.aborted).toBe(true);
+      expect(result.promoted).toBe(true);
+      expect(result.exitCode).toBeNull();
+      expect(result.signal).toBeNull();
+      expect(result.error).toBeNull();
+      expect(result.pid).toBe(mockChildProcess.pid);
+      // Output captured up to the promote moment is preserved as the
+      // snapshot for the caller to seed the BackgroundShellEntry's output
+      // file from.
+      expect(result.output).toContain('line1');
+      expect(result.output).toContain('line2');
+      // Verify the kill path did NOT run.
+      expect(mockProcessKill).not.toHaveBeenCalledWith(
+        -mockChildProcess.pid!,
+        'SIGTERM',
+      );
+      expect(mockProcessKill).not.toHaveBeenCalledWith(
+        -mockChildProcess.pid!,
+        'SIGKILL',
+      );
+      expect(mockChildProcess.kill).not.toHaveBeenCalled();
+    });
+
+    it('post-promotion: stdout / stderr data is no longer routed to onOutputEvent (handoff boundary)', async () => {
+      mockPlatform.mockReturnValue('linux');
+      // Pin the ownership contract: after background-promote, stdout/stderr
+      // arriving on the still-running child must NOT surface through the
+      // foreground execute()'s onOutputEvent. Without off()'ing the
+      // stdoutHandler / stderrHandler in the abort handler, post-promote
+      // bytes would re-enter handleOutput, which then calls
+      // decoder.decode() on a now-finalized decoder (cleanup() called
+      // .decode() without stream:true) → TypeError crash, OR routes to
+      // onOutputEvent → ownership leak / duplicated emit.
+      const { result } = await simulateExecution(
+        'tail -f /tmp/never.log',
+        (cp, abortController) => {
+          cp.stdout?.emit('data', Buffer.from('pre-promote\n'));
+          abortController.abort({
+            kind: 'background',
+            shellId: 'bg_test123',
+          } satisfies ShellAbortReason);
+          // Capture call count at the moment of promote, then emit more
+          // data on the still-live child stream and assert onOutputEvent
+          // was NOT called again. (Also verifies no TypeError from
+          // decoding through the finalized decoder.)
+          const eventCountAtPromote = onOutputEventMock.mock.calls.length;
+          cp.stdout?.emit('data', Buffer.from('post-promote-stdout\n'));
+          cp.stderr?.emit('data', Buffer.from('post-promote-stderr\n'));
+          expect(onOutputEventMock.mock.calls.length).toBe(eventCountAtPromote);
+        },
+      );
+
+      expect(result.promoted).toBe(true);
+      // Pre-promote data made it into the snapshot; post-promote did not.
+      expect(result.output).toContain('pre-promote');
+      expect(result.output).not.toContain('post-promote-stdout');
+      expect(result.output).not.toContain('post-promote-stderr');
+    });
+
+    it('post-promotion: child exit does NOT re-resolve the result with a non-promoted shape', async () => {
+      mockPlatform.mockReturnValue('linux');
+      // Pin: even if the still-running child later exits naturally and the
+      // caller's own exit listener fires, our foreground result Promise
+      // must NOT be re-resolved (Promise can only resolve once). The
+      // detached exit handler prevents our own handler from firing.
+      const { result } = await simulateExecution(
+        'tail -f /tmp/never.log',
+        (cp, abortController) => {
+          abortController.abort({
+            kind: 'background',
+            shellId: 'bg_test123',
+          } satisfies ShellAbortReason);
+          // Simulate the still-running child exiting later; this should
+          // NOT route through our handleExit because the exit listener
+          // was off()'d in the background-promote branch.
+          cp.emit('exit', 42, null);
+          cp.emit('close', 42, null);
+        },
+      );
+
+      expect(result.promoted).toBe(true);
+      expect(result.exitCode).toBeNull();
+      expect(result.signal).toBeNull();
+    });
+
     it('should gracefully attempt SIGKILL on linux if SIGTERM fails', async () => {
       mockPlatform.mockReturnValue('linux');
       vi.useFakeTimers();
@@ -1335,8 +1572,12 @@ describe('ShellExecutionService execution method selection', () => {
     };
     mockPtyProcess.pid = 12345;
     mockPtyProcess.kill = vi.fn();
-    mockPtyProcess.onData = vi.fn();
-    mockPtyProcess.onExit = vi.fn();
+    // node-pty's onData/onExit return IDisposable; the production
+    // background-promote path calls .dispose() on those handles to detach
+    // its listeners cleanly. Mock them to return a disposable stub so the
+    // promote path doesn't crash on `undefined.dispose()`.
+    mockPtyProcess.onData = vi.fn().mockReturnValue({ dispose: vi.fn() });
+    mockPtyProcess.onExit = vi.fn().mockReturnValue({ dispose: vi.fn() });
     mockPtyProcess.write = vi.fn();
     mockPtyProcess.resize = vi.fn();