fix: send SIGKILL on unresponsive process (#395)

Co-authored-by: Hiroki Osame <[email protected]>

src/cli.ts

@@ -1,3 +1,4 @@
+import type { ChildProcess } from 'child_process';
 import { cli } from 'cleye';
 import {
 	transformSync as esbuildTransformSync,
@@ -10,6 +11,74 @@ import {
 	ignoreAfterArgument,
 } from './remove-argv-flags.js';
 
+const relaySignals = (
+	childProcess: ChildProcess,
+) => {
+	let waitForSignal: undefined | ((signal: NodeJS.Signals) => void);
+
+	childProcess.on(
+		'message',
+		(
+			data: { type: string; signal: NodeJS.Signals },
+		) => {
+			if (data && data.type === 'kill' && waitForSignal) {
+				waitForSignal(data.signal);
+			}
+		},
+	);
+
+	const waitForSignalFromChild = () => {
+		const p = new Promise<NodeJS.Signals | undefined>((resolve) => {
+			setTimeout(() => resolve(undefined), 10);
+			waitForSignal = resolve;
+		});
+
+		p.then(
+			() => {
+				waitForSignal = undefined;
+			},
+			() => {},
+		);
+
+		return p;
+	};
+
+	const relaySignalToChild = async (
+		signal: NodeJS.Signals,
+	) => {
+		/**
+		 * This callback is triggered if the parent receives a signal
+		 *
+		 * Child could also receive a signal at the same time if it detected
+		 * a keypress or was sent a signal via process group
+		 *
+		 * The preflight registers a signal handler on the child to
+		 * tell the parent if it also received a signal which we wait for here
+		 */
+		const signalFromChild = await waitForSignalFromChild();
+
+		/**
+		 * If child didn't receive a signal, it's either because it was
+		 * sent to the parent directly via kill PID or the child is
+		 * unresponsive (e.g. infinite loop). Relay signal to child.
+		 */
+		if (signalFromChild !== signal) {
+			childProcess.kill(signal);
+
+			/**
+			 * If child is unresponsive (e.g. infinite loop), we need to force kill it
+			 */
+			const isChildResponsive = await waitForSignalFromChild();
+			if (isChildResponsive !== signal) {
+				childProcess.kill('SIGKILL');
+			}
+		}
+	};
+
+	process.on('SIGINT', relaySignalToChild);
+	process.on('SIGTERM', relaySignalToChild);
+};
+
 const tsxFlags = {
 	noCache: {
 		type: Boolean,
@@ -102,40 +171,7 @@ cli({
 		},
 	);
 
-	const relaySignal = async (signal: NodeJS.Signals) => {
-		const message = await Promise.race([
-			/**
-			 * If child received a signal, it detected a keypress or
-			 * was sent a signal via process group.
-			 *
-			 * Ignore it and let child handle it.
-			 */
-			new Promise<NodeJS.Signals>((resolve) => {
-				function onKillSignal(data: { type: string; signal: NodeJS.Signals }) {
-					if (data && data.type === 'kill') {
-						resolve(data.signal);
-						childProcess.off('message', onKillSignal);
-					}
-				}
-
-				childProcess.on('message', onKillSignal);
-			}),
-			new Promise((resolve) => {
-				setTimeout(resolve, 10);
-			}),
-		]);
-
-		/**
-		 * If child didn't receive a signal, it was sent to the parent
-		 * directly via kill PID. Relay it to child.
-		 */
-		if (!message) {
-			childProcess.kill(signal);
-		}
-	};
-
-	process.on('SIGINT', relaySignal);
-	process.on('SIGTERM', relaySignal);
+	relaySignals(childProcess);
 
 	childProcess.on(
 		'close',

src/preflight.cts

@@ -18,7 +18,7 @@ require('./cjs/index.cjs');
 
 // If a parent process is detected
 if (process.send) {
-	function relaySignal(signal: NodeJS.Signals) {
+	const relaySignal = (signal: NodeJS.Signals) => {
 		process.send!({
 			type: 'kill',
 			signal,
@@ -31,16 +31,19 @@ if (process.send) {
 		if (process.listenerCount(signal) === 0) {
 			process.exit(128 + osConstants.signals[signal]);
 		}
-	}
+	};
 
 	const relaySignals = ['SIGINT', 'SIGTERM'] as const;
 	type RelaySignals = typeof relaySignals[number];
 	for (const signal of relaySignals) {
 		process.on(signal, relaySignal);
 	}
 
-	// Reduce the listenerCount to hide the one set above
-	const { listenerCount } = process;
+	/**
+	 * Hide relaySignal from process.listeners() and process.listenerCount()
+	 */
+	const { listenerCount, listeners } = process;
+
 	process.listenerCount = function (eventName) {
 		let count = Reflect.apply(listenerCount, this, arguments);
 		if (relaySignals.includes(eventName as RelaySignals)) {
@@ -49,8 +52,6 @@ if (process.send) {
 		return count;
 	};
 
-	// Also hide relaySignal from process.listeners()
-	const { listeners } = process;
 	process.listeners = function (eventName) {
 		const result: BaseEventListener[] = Reflect.apply(listeners, this, arguments);
 		if (relaySignals.includes(eventName as RelaySignals)) {

tests/specs/cli.ts

@@ -1,4 +1,5 @@
 import path from 'path';
+import { setTimeout } from 'timers/promises';
 import { testSuite, expect } from 'manten';
 import { createFixture } from 'fs-fixture';
 import packageJson from '../../package.json';
@@ -128,12 +129,10 @@ export default testSuite(({ describe }) => {
 		}, 10_000);
 
 		describe('Signals', async ({ describe, onFinish }) => {
+			const signals = ['SIGINT', 'SIGTERM'];
 			const fixture = await createFixture({
 				'catch-signals.js': `
-				const signals = [
-					'SIGINT',
-					'SIGTERM',
-				];
+				const signals = ${JSON.stringify(signals)};
 				
 				for (const name of signals) {
 					process.on(name, () => {
@@ -153,12 +152,24 @@ export default testSuite(({ describe }) => {
 				setTimeout(() => {}, 1e5);
 				console.log('READY');
 				`,
+				'infinite-loop.js': `
+				console.log(process.pid);
+				while (true) {}
+				`,
+				'ignores-signals.js': `
+				console.log(process.pid);
+				process.on('SIGINT', () => {
+					console.log('Refusing SIGINT');
+				});
+				process.on('SIGTERM', () => {
+					console.log('Refusing SIGTERM');
+				});
+				setTimeout(() => {}, 1e5);
+				`,
 			});
 			onFinish(async () => await fixture.rm());
 
 			describe('Relays kill signal', ({ test }) => {
-				const signals = ['SIGINT', 'SIGTERM'];
-
 				for (const signal of signals) {
 					test(signal, async ({ onTestFail }) => {
 						const tsxProcess = tsx({
@@ -201,6 +212,60 @@ export default testSuite(({ describe }) => {
 				}
 			});
 
+			test('Kills child when unresponsive (infinite loop)', async () => {
+				const tsxProcess = tsx({
+					args: [
+						path.join(fixture.path, 'infinite-loop.js'),
+					],
+				});
+
+				const childPid = await new Promise<number>((resolve) => {
+					tsxProcess.stdout!.once('data', (data) => {
+						resolve(Number(data.toString()));
+					});
+				});
+
+				// Send SIGINT to child
+				tsxProcess.kill('SIGINT', {
+					forceKillAfterTimeout: false,
+				});
+
+				await tsxProcess;
+
+				// Enforce that child process is killed
+				expect(() => process.kill(childPid!, 0)).toThrow();
+			}, 10_000);
+
+			test('Doesn\'t kill child when responsive (ignores signal)', async () => {
+				const tsxProcess = tsx({
+					args: [
+						path.join(fixture.path, 'ignores-signals.js'),
+					],
+				});
+
+				const childPid = await new Promise<number>((resolve) => {
+					tsxProcess.stdout!.once('data', (data) => {
+						resolve(Number(data.toString()));
+					});
+				});
+
+				// Send SIGINT to child
+				tsxProcess.kill('SIGINT', {
+					forceKillAfterTimeout: false,
+				});
+
+				await setTimeout(100);
+
+				if (process.platform === 'win32') {
+					// Enforce that child process is killed
+					expect(() => process.kill(childPid!, 0)).toThrow();
+				} else {
+					// Kill child process
+					expect(() => process.kill(childPid!, 'SIGKILL')).not.toThrow();
+				}
+				await tsxProcess;
+			}, 10_000);
+
 			describe('Ctrl + C', ({ test }) => {
 				test('Exit code', async () => {
 					const output = await ptyShell(