Fix a subprocess which failed to spawn signaling the current process and never ending its streams

Author: sindresorhus

lib/return/early-error.js

@@ -1,4 +1,5 @@
 import {ChildProcess} from 'node:child_process';
+import {callbackify} from 'node:util';
 import {
 	PassThrough,
 	Readable,
@@ -15,6 +16,9 @@ export const handleEarlyError = ({error, command, escapedCommand, fileDescriptor
 	cleanupCustomStreams(fileDescriptors);
 
 	const subprocess = new ChildProcess();
+	// The subprocess never spawned, so there is no process to signal.
+	// `ChildProcess.prototype.kill()` would signal the current process group instead, since its process id is `0`.
+	subprocess.kill = () => false;
 	const all = createDummyStreams(subprocess, fileDescriptors);
 
 	const earlyError = makeEarlyError({
@@ -31,16 +35,7 @@ export const handleEarlyError = ({error, command, escapedCommand, fileDescriptor
 		subprocess,
 		promise,
 		all: options.all ? all : undefined,
-		convertedStreams: {
-			readable,
-			writable,
-			duplex,
-			readableStream,
-			writableStream,
-			transformStream,
-			iterable,
-			[Symbol.asyncIterator]: iterable,
-		},
+		convertedStreams: getConvertedStreams(promise),
 	};
 };
 
@@ -66,12 +61,43 @@ const createDummyStream = () => {
 	return stream;
 };
 
-const readable = () => new Readable({read() {}});
-const writable = () => new Writable({write() {}});
-const duplex = () => new Duplex({read() {}, write() {}});
-const readableStream = () => Readable.toWeb(readable());
-const writableStream = () => Writable.toWeb(writable());
-const transformStream = () => Duplex.toWeb(duplex());
-const iterable = async function * () {};
+// The subprocess never started, so those streams have no contents.
+// Like when the subprocess did start, they end once its promise resolves, or error with its error, as opposed to hanging forever.
+const getConvertedStreams = promise => {
+	const waitForPromise = async () => {
+		await promise;
+	};
+
+	const endReadable = function () {
+		callbackify(waitForPromise)(error => {
+			if (error) {
+				this.destroy(error);
+			} else {
+				this.push(null);
+			}
+		});
+	};
+
+	const endWritable = callbackify(waitForPromise);
+	const writeWritable = (chunk, encoding, done) => {
+		endWritable(done);
+	};
+
+	const readable = () => new Readable({read: endReadable});
+	const writable = () => new Writable({write: writeWritable, final: endWritable});
+	const duplex = () => new Duplex({read: endReadable, write: writeWritable, final: endWritable});
+	const iterable = () => readable()[Symbol.asyncIterator]();
+
+	return {
+		readable,
+		writable,
+		duplex,
+		readableStream: () => Readable.toWeb(readable()),
+		writableStream: () => Writable.toWeb(writable()),
+		transformStream: () => Duplex.toWeb(duplex()),
+		iterable,
+		[Symbol.asyncIterator]: iterable,
+	};
+};
 
 const handleDummyPromise = async (error, verboseInfo, options) => handleResult(error, verboseInfo, options);

lib/terminate/kill-descendants.js

@@ -13,11 +13,18 @@ export const getSpawnOptions = options => options.killDescendants && !isWindows
 	? {...options, detached: true}
 	: options;
 
+/*
+`subprocess.pid` is `undefined` when the subprocess failed to spawn, e.g. with `ENOENT`.
+`ChildProcess.prototype.kill()` then signals the current process group, since its process id is `0`.
+So, like `ChildProcess.prototype.kill()` does for a subprocess which already exited, nothing is signaled and `false` is returned.
+*/
+
 // 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}) => {
 	if (!killDescendants) {
-		return subprocess.kill.bind(subprocess);
+		const killSubprocess = subprocess.kill.bind(subprocess);
+		return (...arguments_) => (subprocess.pid === undefined ? false : killSubprocess(...arguments_));
 	}
 
 	const killDescendantsFunction = isWindows ? killDescendantsWindows : killDescendantsUnix;

test/return/early-error.js

@@ -1,8 +1,10 @@
 import {arch} from 'node:os';
 import process from 'node:process';
+import {finished} from 'node:stream/promises';
 import test from 'ava';
 import {execa, execaSync, $} from '../../index.js';
 import {setFixtureDirectory} from '../helpers/fixtures-directory.js';
+import {foobarString, foobarUint8Array} from '../helpers/input.js';
 import {fullStdio} from '../helpers/stdio.js';
 import {
 	earlyErrorOptions,
@@ -17,6 +19,14 @@ setFixtureDirectory();
 const isWindows = process.platform === 'win32';
 const ENOENT_REGEXP = isWindows ? /failed with exit code 1/ : /spawn.* ENOENT/;
 
+// A nonexistent command fails asynchronously, so it does not go through the early error path, but the subprocess still has no process to signal
+test('kill() does not signal the current process when the command does not exist', async t => {
+	const subprocess = execa('nonexistent-command-for-execa');
+	t.is(subprocess.pid, undefined);
+	t.is(subprocess.kill(), false);
+	t.is((await t.throwsAsync(subprocess)).code, 'ENOENT');
+});
+
 test('execaSync() throws error if ENOENT', t => {
 	t.throws(() => {
 		execaSync('foo');
@@ -35,6 +45,26 @@ const testEarlyErrorShape = async (t, reject) => {
 test('child_process.spawn() early errors have correct shape', testEarlyErrorShape, true);
 test('child_process.spawn() early errors have correct shape - reject false', testEarlyErrorShape, false);
 
+// The subprocess never spawned, so there is no process to signal.
+// `ChildProcess.prototype.kill()` would signal the current process group instead, since its process id is `0`.
+// The handle is replaced by a stub, so that a regression is caught by an assertion instead of killing this test process.
+test('kill() does not signal the current process on early errors', async t => {
+	const subprocess = getEarlyErrorSubprocess({reject: false});
+	let isSignaled = false;
+	subprocess.nodeChildProcess._handle = {
+		kill() {
+			isSignaled = true;
+			return 0;
+		},
+	};
+
+	t.is(subprocess.kill(), false);
+	t.false(isSignaled);
+	const {failed, isCanceled} = await subprocess;
+	t.true(failed);
+	t.false(isCanceled);
+});
+
 test('child_process.spawn() early errors are propagated', async t => {
 	await t.throwsAsync(getEarlyErrorSubprocess(), expectedEarlyError);
 });
@@ -113,6 +143,54 @@ test('child_process.spawn() early errors can use .readableStream()', testEarlyEr
 test('child_process.spawn() early errors can use .writableStream()', testEarlyErrorWebConvertor, 'writableStream');
 test('child_process.spawn() early errors can use .transformStream()', testEarlyErrorWebConvertor, 'transformStream');
 
+// The subprocess never started, so those streams have no contents.
+// They must end right away, like `subprocess.stdout` and `.iterable()` do, instead of hanging forever.
+const testEarlyErrorReadableEnd = async (t, streamMethod) => {
+	const subprocess = getEarlyErrorSubprocess();
+	const chunks = await Array.fromAsync(subprocess[streamMethod]());
+
+	t.deepEqual(chunks, []);
+	await t.throwsAsync(subprocess);
+};
+
+test('child_process.spawn() early errors end .readable()', testEarlyErrorReadableEnd, 'readable');
+test('child_process.spawn() early errors end .duplex()', testEarlyErrorReadableEnd, 'duplex');
+test('child_process.spawn() early errors end .readableStream()', testEarlyErrorReadableEnd, 'readableStream');
+
+const testEarlyErrorWritableEnd = async (t, streamMethod) => {
+	const subprocess = getEarlyErrorSubprocess();
+	const stream = subprocess[streamMethod]();
+	stream.end(foobarString);
+	await finished(stream, {readable: false});
+
+	t.false(stream.writable);
+	await t.throwsAsync(subprocess);
+};
+
+test('child_process.spawn() early errors end .writable()', testEarlyErrorWritableEnd, 'writable');
+test('child_process.spawn() early errors end .duplex() writable side', testEarlyErrorWritableEnd, 'duplex');
+
+test('child_process.spawn() early errors end .writableStream()', async t => {
+	const subprocess = getEarlyErrorSubprocess();
+	const writer = subprocess.writableStream().getWriter();
+	await writer.write(foobarUint8Array);
+	await writer.close();
+
+	await t.throwsAsync(subprocess);
+});
+
+test('child_process.spawn() early errors end .transformStream()', async t => {
+	const subprocess = getEarlyErrorSubprocess();
+	const {readable, writable} = subprocess.transformStream();
+	const writer = writable.getWriter();
+	await writer.write(foobarUint8Array);
+	await writer.close();
+	const chunks = await Array.fromAsync(readable);
+
+	t.deepEqual(chunks, []);
+	await t.throwsAsync(subprocess);
+});
+
 const testEarlyErrorIpc = async (t, runIpcMethod) => {
 	const subprocess = getEarlyErrorSubprocess({ipc: true});
 	t.throws(() => {