More robust PHP child process killing

Author: fredrikekelund

apps/cli/php-server-child.ts

@@ -8,6 +8,7 @@
 
 import { ChildProcess, spawn } from 'node:child_process';
 import fs from 'node:fs';
+import os from 'node:os';
 import path from 'node:path';
 import { DEFAULT_PHP_VERSION } from '@studio/common/constants';
 import { DEFAULT_LOCALE } from '@studio/common/lib/locale';
@@ -642,19 +643,34 @@ async function ipcMessageHandler( packet: unknown ) {
 function killPhpProcess(): void {
 	if ( phpProcess && ! phpProcess.killed ) {
 		try {
+			// Detach the unexpected-exit listener so the imminent SIGKILL is not logged as a crash.
+			phpProcess.removeAllListeners( 'exit' );
 			phpProcess.kill( 'SIGKILL' );
 		} catch {
 			// Best effort — nothing useful to do if this fails.
 		}
 	}
 }
 
+function shutdownOnSignal( signal: NodeJS.Signals ): void {
+	logToConsole( `Received ${ signal }, shutting down` );
+	killPhpProcess();
+	// Follow the Unix convention of `128 + signum` so the exit code reflects the signal.
+	const signum = os.constants.signals[ signal ] ?? 0;
+	process.exit( 128 + signum );
+}
+
 // If this node process is going down (normal exit or IPC disconnect), make sure PHP goes with it.
 process.on( 'exit', killPhpProcess );
 process.on( 'disconnect', () => {
 	killPhpProcess();
 } );
 
+// Without explicit signal handlers, the process is terminated abruptly and the 'exit' event
+// does not fire — leaving the PHP child orphaned. These handlers ensure cleanup runs.
+process.on( 'SIGTERM', shutdownOnSignal );
+process.on( 'SIGINT', shutdownOnSignal );
+
 if ( process.send ) {
 	process.on( 'message', ipcMessageHandler );
 	process.send( { topic: 'ready' } );

apps/cli/process-manager-daemon.ts

@@ -309,7 +309,7 @@ export class ProcessManagerDaemon {
 				resolve();
 			} );
 
-			managedProcess.child.kill( 'SIGTERM' );
+			this.signalProcessGroup( managedProcess, 'SIGTERM' );
 		} );
 	}
 
@@ -424,23 +424,33 @@ export class ProcessManagerDaemon {
 	}
 
 	private forceCleanupChild( managedProcess: ManagedProcess ) {
-		if ( process.platform === 'win32' ) {
+		// On Windows, child.kill() maps any signal to TerminateProcess, so SIGKILL and SIGTERM
+		// are equivalent there. On non-Windows the helper sends SIGKILL to the whole group.
+		this.signalProcessGroup( managedProcess, 'SIGKILL' );
+	}
+
+	private signalProcessGroup( managedProcess: ManagedProcess, signal: NodeJS.Signals ): void {
+		if ( process.platform === 'win32' || ! managedProcess.child.pid ) {
 			try {
-				managedProcess.child.kill( 'SIGTERM' );
+				managedProcess.child.kill( signal );
 			} catch {
 				// Do nothing
 			}
 			return;
 		}
 
-		if ( ! managedProcess.child.pid ) {
-			return;
-		}
-
+		// Children are spawned with `detached: true` on non-Windows, so each lives in its own
+		// process group. Signalling the negative PID delivers to every member of that group,
+		// including grandchildren (e.g. the PHP server spawned by the wrapper).
 		try {
-			process.kill( -managedProcess.child.pid, 'SIGKILL' );
+			process.kill( -managedProcess.child.pid, signal );
 		} catch {
-			// Do nothing
+			// Group send can fail if the leader has already exited but children remain.
+			try {
+				managedProcess.child.kill( signal );
+			} catch {
+				// Do nothing
+			}
 		}
 	}