Fix killDescendants signaling unrelated processes once Execa is done with the subprocess

Author: sindresorhus

docs/api.md

@@ -1183,7 +1183,7 @@ This is useful when the subprocess spawns its own processes, such as when using
 
 On Unix, this spawns the subprocess in its own [process group](https://en.wikipedia.org/wiki/Process_group). On Windows, this uses [`taskkill`](https://learn.microsoft.com/en-us/windows-server/administration/windows-commands/taskkill).
 
-This is best-effort: descendant processes that create their own process group or session are not terminated.
+This is best-effort: descendant processes that create their own process group or session are not terminated. Also, descendants are not terminated once Execa is done with the subprocess, or on Windows once the subprocess exited, since its PID might then belong to an unrelated process.
 
 [More info.](termination.md#killing-descendant-processes)
 

docs/termination.md

@@ -209,7 +209,7 @@ await subprocess;
 
 On Unix, this spawns the subprocess in its own [process group](https://en.wikipedia.org/wiki/Process_group), then sends the signal to that group. On Windows, this uses [`taskkill`](https://learn.microsoft.com/en-us/windows-server/administration/windows-commands/taskkill), which terminates the process tree tracked by the OS.
 
-This is best-effort. On Unix, descendant processes that create their own process group or session (for example, daemons calling [`setsid()`](https://man7.org/linux/man-pages/man2/setsid.2.html)) escape termination. On Unix, because the subprocess runs in its own process group, it is also detached from the terminal, so pressing `CTRL-C` no longer forwards [`SIGINT`](#sigint) to it.
+This is best-effort. On Unix, descendant processes that create their own process group or session (for example, daemons calling [`setsid()`](https://man7.org/linux/man-pages/man2/setsid.2.html)) escape termination. On Unix, because the subprocess runs in its own process group, it is also detached from the terminal, so pressing `CTRL-C` no longer forwards [`SIGINT`](#sigint) to it. Finally, the OS can re-assign the subprocess' PID to an unrelated process once it exited. So, descendants are not signaled anymore once Execa is done with the subprocess, i.e. once its promise has settled. On Windows, this applies as soon as the subprocess exited, which also prevents the [`forceKillAfterDelay`](#forceful-termination) escalation from terminating its descendants.
 
 This option cannot be used with [synchronous methods](execution.md#synchronous-execution).
 

lib/methods/main-async.js

@@ -109,7 +109,7 @@ const spawnSubprocessAsync = ({file, commandArguments, options, startTime, verbo
 	const context = {};
 	const onInternalError = createDeferred();
 	const kill = subprocessKill.bind(undefined, {
-		kill: getKillFunction(subprocess, options),
+		kill: getKillFunction(subprocess, options, controller),
 		options,
 		onInternalError,
 		context,

lib/terminate/kill-descendants.js

@@ -21,20 +21,30 @@ So, like `ChildProcess.prototype.kill()` does for a subprocess which already exi
 
 // Returns the low-level function used to send a signal to the subprocess.
 // With the `killDescendants` option, the signal is sent to the whole process tree.
-export const getKillFunction = (subprocess, {killDescendants}) => {
+export const getKillFunction = (subprocess, {killDescendants}, {signal: controllerSignal}) => {
 	if (!killDescendants) {
 		const killSubprocess = subprocess.kill.bind(subprocess);
 		return (...arguments_) => (subprocess.pid === undefined ? false : killSubprocess(...arguments_));
 	}
 
 	const killDescendantsFunction = isWindows ? killDescendantsWindows : killDescendantsUnix;
-	return killDescendantsFunction.bind(undefined, subprocess);
+	return killDescendantsFunction.bind(undefined, subprocess, controllerSignal);
 };
 
+/*
+Unlike `subprocess.kill()`, signaling a process group or a process tree uses the PID directly, without going through the subprocess' handle. The OS can re-assign that PID once the tree is gone, and signaling it then would terminate an unrelated process, and on Windows its own descendants too.
+On Unix, a PID stays reserved as long as it is the process group ID of a running process, so it can only be re-assigned once every descendant exited too. While Execa is still waiting on the subprocess, the signal is still useful, e.g. for the `forceKillAfterDelay` escalation, which happens after the subprocess exited but while a descendant might still be running. Once Execa is done with the subprocess, no internal logic sends signals anymore, so only `subprocess.kill()` can. Like `ChildProcess.kill()`, it then becomes a noop returning `false`. This narrows the window during which a re-assigned PID could be signaled, without removing it, e.g. when Execa is still waiting on a slow transform after the whole process group exited.
+On Windows, there are no process groups: a PID is only reserved while a handle to the process is open, which Node.js closes once the subprocess exits. `taskkill /T` also cannot find the descendants of a process which exited. So the process tree is never signaled once the subprocess exited.
+*/
+const isAbandoned = (subprocess, controllerSignal) => subprocess.pid === undefined
+	|| (hasExited(subprocess) && (isWindows || controllerSignal.aborted));
+
+const hasExited = ({exitCode, signalCode}) => exitCode !== null || signalCode !== null;
+
 // On Unix, the subprocess is its own process group leader (its PGID equals its PID), since it
 // was spawned with `detached: true`. Sending the signal to `-pid` targets the whole group.
-const killDescendantsUnix = (subprocess, signal) => {
-	if (subprocess.pid === undefined) {
+const killDescendantsUnix = (subprocess, controllerSignal, signal) => {
+	if (isAbandoned(subprocess, controllerSignal)) {
 		return false;
 	}
 
@@ -52,8 +62,8 @@ const killDescendantsUnix = (subprocess, signal) => {
 // fallback: killing the direct subprocess first would orphan its descendants before `taskkill` could enumerate them.
 // If `taskkill` is unavailable or fails, the fallback only terminates the direct subprocess.
 // `taskkill` ignores the signal (`/F` terminates the tree).
-const killDescendantsWindows = (subprocess, signal) => {
-	if (subprocess.pid === undefined) {
+const killDescendantsWindows = (subprocess, controllerSignal, signal) => {
+	if (isAbandoned(subprocess, controllerSignal)) {
 		return false;
 	}
 

test/fixtures/ipc-send-pid-stubborn.js

@@ -0,0 +1,14 @@
+#!/usr/bin/env node
+import {execa, sendMessage} from '../../index.js';
+
+// Spawns a descendant which ignores `SIGTERM` and keeps this process' `stdout` open.
+// This process is not awaiting it, so it exits on `SIGTERM` while that descendant keeps running.
+const subprocess = execa('no-killable.js', {
+	ipc: true,
+	killSignal: 'SIGKILL',
+	stdout: 'inherit',
+	cleanup: false,
+});
+await subprocess.getOneMessage();
+await sendMessage(subprocess.pid);
+setTimeout(() => {}, 1e8);

test/terminate/kill-descendants.js

@@ -87,6 +87,80 @@ if (!isWindows) {
 	});
 }
 
+/*
+The `forceKillAfterDelay` escalation happens after the subprocess exited, since that is when its termination has been noticed.
+A descendant which ignores `SIGTERM` is still running then, so the escalation must still reach it.
+Otherwise, since that descendant keeps the subprocess' `stdout` open, awaiting the subprocess would never settle.
+*/
+test('killDescendants escalates to descendants which outlive the subprocess', async t => {
+	const subprocess = execa('ipc-send-pid-stubborn.js', {
+		ipc: true,
+		killDescendants: true,
+		forceKillAfterDelay: 1000,
+	});
+	const descendantPid = await subprocess.getOneMessage();
+	t.true(isRunning(descendantPid));
+
+	subprocess.kill();
+	const settled = await Promise.race([
+		t.throwsAsync(subprocess).then(() => 'settled'),
+		setTimeout(1e4, 'pending', {ref: false}),
+	]);
+	t.is(settled, 'settled');
+	t.false(isRunning(descendantPid));
+});
+
+if (!isWindows) {
+	// A PID can be re-assigned to an unrelated process once Execa is done with the subprocess, so its process group must not be signaled anymore
+	const testAbandonedProcessGroup = test.macro(async (t, getSubprocess) => {
+		const subprocess = getSubprocess();
+		await t.throwsAsync(subprocess);
+
+		const originalKill = process.kill;
+		t.teardown(() => {
+			process.kill = originalKill;
+		});
+
+		const signaledProcesses = [];
+		process.kill = (pid, signal) => {
+			signaledProcesses.push([pid, signal]);
+			return true;
+		};
+
+		t.false(subprocess.kill('SIGKILL'));
+		t.deepEqual(signaledProcesses, []);
+	});
+
+	test.serial('killDescendants does not signal the process group once Execa is done with the subprocess', testAbandonedProcessGroup, () => execa('fail.js', {killDescendants: true}));
+
+	test.serial('killDescendants does not signal the process group once Execa is done with a terminated subprocess', testAbandonedProcessGroup, () => {
+		const subprocess = execa('forever.js', {killDescendants: true});
+		subprocess.kill('SIGTERM');
+		return subprocess;
+	});
+
+	// While Execa is still waiting on the subprocess, its process group is still its own
+	test.serial('killDescendants signals the process group while Execa is not done with the subprocess', async t => {
+		const subprocess = execa('forever.js', {killDescendants: true});
+
+		const originalKill = process.kill;
+		t.teardown(() => {
+			process.kill = originalKill;
+		});
+
+		const signaledProcesses = [];
+		process.kill = (pid, signal) => {
+			signaledProcesses.push([pid, signal]);
+			return originalKill(pid, signal);
+		};
+
+		t.true(subprocess.kill('SIGTERM'));
+		t.deepEqual(signaledProcesses, [[-subprocess.pid, 'SIGTERM']]);
+		process.kill = originalKill;
+		await t.throwsAsync(subprocess);
+	});
+}
+
 test('Cannot use "killDescendants" option, sync', t => {
 	t.throws(() => {
 		execaSync('empty.js', {killDescendants: true});
@@ -132,78 +206,124 @@ test.serial('taskkill is resolved from the Windows directory when available', t
 	t.is(getTaskkillFile(), undefined);
 });
 
-test.serial('taskkill fallback uses direct subprocess kill when Windows directory is unavailable', async t => {
+/*
+The `taskkill` logic only runs on Windows, so the platform, the Windows directory and `taskkill` itself are faked to test it on any OS.
+`lib/terminate/kill-descendants.js` reads `process.platform` when it is loaded, so it must be re-imported once the platform has been faked.
+*/
+let fakeWindowsCount = 0;
+
+const fakeWindows = async (t, systemRoot, execFile = () => {}) => {
 	const platformDescriptor = Object.getOwnPropertyDescriptor(process, 'platform');
+	const originalExecFile = childProcess.execFile;
 	const {SystemRoot, windir} = process.env;
 	t.teardown(() => {
 		Object.defineProperty(process, 'platform', platformDescriptor);
+		childProcess.execFile = originalExecFile;
+		syncBuiltinESMExports();
 		restoreEnvironment('SystemRoot', SystemRoot);
 		restoreEnvironment('windir', windir);
 	});
 
 	Object.defineProperty(process, 'platform', {value: 'win32'});
-	process.env.SystemRoot = 'Windows';
+	process.env.SystemRoot = systemRoot;
 	delete process.env.windir;
+	childProcess.execFile = execFile;
+	syncBuiltinESMExports();
 
-	const {getKillFunction} = await import(`../../lib/terminate/kill-descendants.js?taskkill-fallback=${Date.now()}`);
-	let killedWith;
+	fakeWindowsCount += 1;
+	const {getKillFunction} = await import(`../../lib/terminate/kill-descendants.js?fake-windows=${fakeWindowsCount}`);
+	return getKillFunction;
+};
+
+// Mimics the `ChildProcess` properties used to send the signal and to detect whether the subprocess is still running
+const createFakeSubprocess = ({exitCode = null, signalCode = null} = {}) => {
 	const subprocess = {
 		pid: 123,
+		exitCode,
+		signalCode,
 		kill(signal) {
-			killedWith = signal;
+			subprocess.killedWith = signal;
 			return true;
 		},
 	};
+	return subprocess;
+};
 
-	const kill = getKillFunction(subprocess, {killDescendants: true});
+const abortedController = () => {
+	const controller = new AbortController();
+	controller.abort();
+	return controller;
+};
+
+test.serial('taskkill fallback uses direct subprocess kill when Windows directory is unavailable', async t => {
+	const getKillFunction = await fakeWindows(t, 'Windows');
+	const subprocess = createFakeSubprocess();
+
+	const kill = getKillFunction(subprocess, {killDescendants: true}, new AbortController());
 	t.true(kill('SIGTERM'));
-	t.is(killedWith, 'SIGTERM');
+	t.is(subprocess.killedWith, 'SIGTERM');
 });
 
 test.serial('taskkill fallback uses direct subprocess kill when taskkill cannot be spawned', async t => {
-	const platformDescriptor = Object.getOwnPropertyDescriptor(process, 'platform');
-	const originalExecFile = childProcess.execFile;
-	const {SystemRoot, windir} = process.env;
-	t.teardown(() => {
-		Object.defineProperty(process, 'platform', platformDescriptor);
-		childProcess.execFile = originalExecFile;
-		syncBuiltinESMExports();
-		restoreEnvironment('SystemRoot', SystemRoot);
-		restoreEnvironment('windir', windir);
-	});
-
-	Object.defineProperty(process, 'platform', {value: 'win32'});
-	process.env.SystemRoot = 'C:\\MissingWindows';
-	delete process.env.windir;
-
 	const taskkillFailure = Promise.withResolvers();
-	childProcess.execFile = (file, arguments_, callback) => {
+	const getKillFunction = await fakeWindows(t, 'C:\\MissingWindows', (file, arguments_, callback) => {
 		t.is(file, path.join('C:\\MissingWindows', 'System32', 'taskkill.exe'));
 		t.deepEqual(arguments_, ['/pid', '123', '/T', '/F']);
 		queueMicrotask(() => {
 			callback(new Error('spawn failed'));
 			taskkillFailure.resolve();
 		});
-	};
+	});
+	const subprocess = createFakeSubprocess();
 
-	syncBuiltinESMExports();
+	const kill = getKillFunction(subprocess, {killDescendants: true}, new AbortController());
+	t.true(kill('SIGTERM'));
+	t.is(subprocess.killedWith, undefined);
 
-	const {getKillFunction} = await import(`../../lib/terminate/kill-descendants.js?taskkill-spawn-fallback=${Date.now()}`);
-	let killedWith;
-	const subprocess = {
-		pid: 123,
-		kill(signal) {
-			killedWith = signal;
-			return true;
-		},
-	};
+	await taskkillFailure.promise;
+	t.is(subprocess.killedWith, 'SIGTERM');
+});
 
-	const kill = getKillFunction(subprocess, {killDescendants: true});
+// Once Execa is done with the subprocess, its PID can be re-assigned by the OS, so its process tree must not be terminated anymore
+test.serial('taskkill is not spawned once Execa is done with the subprocess', async t => {
+	let isTaskkillSpawned = false;
+	const getKillFunction = await fakeWindows(t, 'C:\\Windows', () => {
+		isTaskkillSpawned = true;
+	});
+	const subprocess = createFakeSubprocess({exitCode: 0});
+
+	const kill = getKillFunction(subprocess, {killDescendants: true}, abortedController());
+	t.false(kill('SIGTERM'));
+	t.false(isTaskkillSpawned);
+	t.is(subprocess.killedWith, undefined);
+});
+
+// While Execa is still waiting on the subprocess, its process tree is still its own, even after it exited
+test.serial('taskkill is still spawned after the subprocess exited', async t => {
+	let taskkillArguments;
+	const getKillFunction = await fakeWindows(t, 'C:\\Windows', (file, arguments_) => {
+		taskkillArguments = arguments_;
+	});
+	const subprocess = createFakeSubprocess({exitCode: 0});
+
+	const kill = getKillFunction(subprocess, {killDescendants: true}, new AbortController());
 	t.true(kill('SIGTERM'));
-	t.is(killedWith, undefined);
+	t.deepEqual(taskkillArguments, ['/pid', '123', '/T', '/F']);
+});
 
-	await taskkillFailure.promise;
-	t.is(killedWith, 'SIGTERM');
+// A subprocess which never spawned has no PID to signal
+test.serial('taskkill is not spawned when the subprocess has no PID', async t => {
+	let isTaskkillSpawned = false;
+	const getKillFunction = await fakeWindows(t, 'C:\\Windows', () => {
+		isTaskkillSpawned = true;
+	});
+	const subprocess = createFakeSubprocess();
+	subprocess.pid = undefined;
+
+	const kill = getKillFunction(subprocess, {killDescendants: true}, new AbortController());
+	t.false(kill('SIGTERM'));
+	t.false(isTaskkillSpawned);
+	t.is(subprocess.killedWith, undefined);
 });
 
 const restoreEnvironment = (name, value) => {

types/arguments/options.d.ts

@@ -340,7 +340,7 @@ export type CommonOptions<
 
 	On Unix, this spawns the subprocess in its own [process group](https://en.wikipedia.org/wiki/Process_group). On Windows, this uses [`taskkill`](https://learn.microsoft.com/en-us/windows-server/administration/windows-commands/taskkill).
 
-	This is best-effort: descendant processes that create their own process group or session are not terminated.
+	This is best-effort: descendant processes that create their own process group or session are not terminated. Also, descendants are not terminated once Execa is done with the subprocess, or on Windows once the subprocess exited, since its PID might then belong to an unrelated process.
 
 	@default false
 	*/