Skip to content

Commit b4d0a3c

Browse files
Fix E2E tests on macOS when STUDIO_RUNTIME=native-php (#3301)
## Related issues <!-- Link a related issue to this PR. If the PR does not immediately resolve the issue, for example, it requires a separate deployment to production, avoid using the "Fixes" keyword and use "Related to" instead. --> - Fixes RSM-1725 ## How AI was used in this PR <!-- Help reviewers understand what to look for and verify that you've reviewed the code yourself. --> Primarily to help me debug the problem. I alternated between Claude and Codex. ## Proposed Changes While working on #3271, @bcotrim discovered that the E2E tests were failing when run with `STUDIO_RUNTIME=native-php`. This PR fixes that by implementing the following fixes: - When Studio clones a site, explicitly update the site URL. Playground can handle this internally, but the native PHP runtime obviously cannot. - On Posix platforms, spawn process daemon children with `detached: true` to assign them to a new process group. This is so we can pass a negative PID to `process.kill` to terminate the entire process group (the Node.js process and the associated PHP process). This approach isn't bulletproof, but it's alright, and it works for the purpose of the E2E tests. - Favor SIGTERM over SIGKILL when killing child processes to give them a chance to clean up. ## Testing Instructions <!-- Add as many details as possible to help others reproduce the issue and test the fix. "Before / After" screenshots can also be very helpful when the change is visual. --> Run the E2E tests locally on macOS by following these steps: 1. `npm run package` 2. `STUDIO_RUNTIME=native-php npm run e2e` 3. Ensure that they pass successfully ## Pre-merge Checklist <!-- Complete applicable items on this checklist **before** merging into trunk. Inapplicable items can be left unchecked. Both the PR author and reviewer are responsible for ensuring the checklist is completed. --> - [ ] Have you checked for TypeScript, React or other console errors?
1 parent 17b8a5c commit b4d0a3c

5 files changed

Lines changed: 63 additions & 13 deletions

File tree

apps/cli/php-server-child.ts

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88

99
import { ChildProcess, spawn } from 'node:child_process';
1010
import fs from 'node:fs';
11+
import os from 'node:os';
1112
import path from 'node:path';
1213
import { DEFAULT_PHP_VERSION } from '@studio/common/constants';
1314
import { DEFAULT_LOCALE } from '@studio/common/lib/locale';
@@ -346,16 +347,16 @@ async function startServer( config: ServerConfig, signal: AbortSignal ): Promise
346347
} );
347348
} );
348349

349-
stopSignal.throwIfAborted();
350-
await waitForServerReady( `http://localhost:${ config.port }/`, stopSignal );
351-
352350
serverChild.once( 'exit', ( code, signalName ) => {
353351
errorToConsole(
354352
`PHP child process exited unexpectedly (code: ${ code }, signal: ${ signalName })`
355353
);
356354
process.exit( code ?? 1 );
357355
} );
358356

357+
stopSignal.throwIfAborted();
358+
await waitForServerReady( `http://localhost:${ config.port }/`, stopSignal );
359+
359360
phpProcess = serverChild;
360361
} catch ( error ) {
361362
if ( spawnedChild && ! spawnedChild.killed ) {
@@ -642,19 +643,34 @@ async function ipcMessageHandler( packet: unknown ) {
642643
function killPhpProcess(): void {
643644
if ( phpProcess && ! phpProcess.killed ) {
644645
try {
646+
// Detach the unexpected-exit listener so the imminent SIGKILL is not logged as a crash.
647+
phpProcess.removeAllListeners( 'exit' );
645648
phpProcess.kill( 'SIGKILL' );
646649
} catch {
647650
// Best effort — nothing useful to do if this fails.
648651
}
649652
}
650653
}
651654

655+
function shutdownOnSignal( signal: NodeJS.Signals ): void {
656+
logToConsole( `Received ${ signal }, shutting down` );
657+
killPhpProcess();
658+
// Follow the Unix convention of `128 + signum` so the exit code reflects the signal.
659+
const signum = os.constants.signals[ signal ] ?? 0;
660+
process.exit( 128 + signum );
661+
}
662+
652663
// If this node process is going down (normal exit or IPC disconnect), make sure PHP goes with it.
653664
process.on( 'exit', killPhpProcess );
654665
process.on( 'disconnect', () => {
655666
killPhpProcess();
656667
} );
657668

669+
// Without explicit signal handlers, the process is terminated abruptly and the 'exit' event
670+
// does not fire — leaving the PHP child orphaned. These handlers ensure cleanup runs.
671+
process.on( 'SIGTERM', shutdownOnSignal );
672+
process.on( 'SIGINT', shutdownOnSignal );
673+
658674
if ( process.send ) {
659675
process.on( 'message', ipcMessageHandler );
660676
process.send( { topic: 'ready' } );

apps/cli/process-manager-daemon.ts

Lines changed: 36 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -217,6 +217,7 @@ export class ProcessManagerDaemon {
217217
env,
218218
stdio: [ 'ignore', 'pipe', 'pipe', 'ipc' ],
219219
windowsHide: true,
220+
detached: process.platform !== 'win32',
220221
} );
221222

222223
const managedProcess: ManagedProcessRunning = {
@@ -293,7 +294,7 @@ export class ProcessManagerDaemon {
293294

294295
await new Promise< void >( ( resolve ) => {
295296
const timeoutId = setTimeout( () => {
296-
managedProcess.child.kill( 'SIGKILL' );
297+
this.forceCleanupChild( managedProcess );
297298
}, STOP_TIMEOUT_MS );
298299

299300
managedProcess.child.once( 'exit', () => {
@@ -308,7 +309,7 @@ export class ProcessManagerDaemon {
308309
resolve();
309310
} );
310311

311-
managedProcess.child.kill( 'SIGTERM' );
312+
this.signalProcessGroup( managedProcess, 'SIGTERM' );
312313
} );
313314
}
314315

@@ -418,8 +419,35 @@ export class ProcessManagerDaemon {
418419
if ( managedProcess.settled ) {
419420
continue;
420421
}
422+
this.forceCleanupChild( managedProcess );
423+
}
424+
}
425+
426+
private forceCleanupChild( managedProcess: ManagedProcess ) {
427+
// On Windows, child.kill() maps any signal to TerminateProcess, so SIGKILL and SIGTERM
428+
// are equivalent there. On non-Windows the helper sends SIGKILL to the whole group.
429+
this.signalProcessGroup( managedProcess, 'SIGKILL' );
430+
}
431+
432+
private signalProcessGroup( managedProcess: ManagedProcess, signal: NodeJS.Signals ): void {
433+
if ( process.platform === 'win32' || ! managedProcess.child.pid ) {
434+
try {
435+
managedProcess.child.kill( signal );
436+
} catch {
437+
// Do nothing
438+
}
439+
return;
440+
}
441+
442+
// Children are spawned with `detached: true` on non-Windows, so each lives in its own
443+
// process group. Signalling the negative PID delivers to every member of that group,
444+
// including grandchildren (e.g. the PHP server spawned by the wrapper).
445+
try {
446+
process.kill( -managedProcess.child.pid, signal );
447+
} catch {
448+
// Group send can fail if the leader has already exited but children remain.
421449
try {
422-
managedProcess.child.kill( 'SIGKILL' );
450+
managedProcess.child.kill( signal );
423451
} catch {
424452
// Do nothing
425453
}
@@ -432,17 +460,18 @@ export class ProcessManagerDaemon {
432460
}
433461

434462
this.shuttingDown = true;
435-
await this.broadcastEvent( {
436-
type: 'daemon-kill',
437-
payload: { reason },
438-
} );
439463

440464
await Promise.allSettled(
441465
Array.from( this.managedProcesses.values() ).map( ( managedProcess ) =>
442466
this.stopProcess( managedProcess.name )
443467
)
444468
);
445469

470+
await this.broadcastEvent( {
471+
type: 'daemon-kill',
472+
payload: { reason },
473+
} );
474+
446475
await new Promise< void >( ( resolve ) => {
447476
void this.controlServer.close().then( () => resolve() );
448477
} );

apps/studio/src/ipc-handlers.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,7 @@ import { setSentryWpcomUserIdMain } from 'src/lib/main-sentry-utils';
8282
import * as oauthClient from 'src/lib/oauth';
8383
import { getAiInstructionsPath } from 'src/lib/server-files-paths';
8484
import { shellOpenExternalWrapper } from 'src/lib/shell-open-external-wrapper';
85+
import { updateSiteUrl } from 'src/lib/update-site-url';
8586
import * as windowsHelpers from 'src/lib/windows-helpers';
8687
import { getLogsFilePath, writeLogToFile, type LogLevel } from 'src/logging';
8788
import { getMainWindow } from 'src/main-window';
@@ -1042,6 +1043,10 @@ export async function copySite(
10421043
noStart: true,
10431044
} );
10441045

1046+
// Playground sets the correct siteurl internally, but for the native-php runtime, we need to
1047+
// explicitly update that option
1048+
await updateSiteUrl( server, `http://localhost:${ details.port }` );
1049+
10451050
// Persist themeDetails to appdata (Studio-only data)
10461051
if ( sourceSite.themeDetails ) {
10471052
server.details.themeDetails = sourceSite.themeDetails;

apps/studio/src/modules/cli/lib/execute-export-command.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ export function executeExportCliCommand(
2424
const { abortSignal } = options;
2525

2626
function abortExportProcess() {
27-
childProcess.kill( 'SIGKILL' );
27+
childProcess.kill( 'SIGTERM' );
2828
emitFailure( new Error( 'Export aborted' ) );
2929
}
3030

tools/common/lib/php-binary-metadata.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -39,13 +39,13 @@ export const PHP_PATCH_VERSIONS: Record< NativePhpSupportedVersion, string > = {
3939
// Windows ARM64 falls back to x64, so there is no separate win32-arm64 entry.
4040
export const PHP_BINARY_HASHES: Record< string, string > = {
4141
// PHP 8.5 (8.5.5)
42-
'8.5-darwin-arm64': 'd66ef557189baa9215cec65dba80eb5dcfa84ac87bfce565059263bce5c9cccb',
42+
'8.5-darwin-arm64': '025d07dc55e806f98fc36871cf8b342228379b9d5ac3d739d66744a2b550de2a',
4343
'8.5-darwin-x64': 'f206193e86c6ca188fbb46532c432ff3f216f203be9fe67bfc839973de64f9b8',
4444
'8.5-linux-x64': '72a877af1cb93d7c14f79344bdbd78dc2a57a2e915a76b13e9a3141700df3f21',
4545
'8.5-linux-arm64': 'a45fc1f818497586bfd8f749c808f1f63c9d10bf48f439985e77bee607a2c76b',
4646
'8.5-win32-x64': 'ddd8098a1e71dfe53c147c392eeaa50eb61259a1c430e3cbe016b0edbdc1cf91',
4747
// PHP 8.4 (8.4.20)
48-
'8.4-darwin-arm64': '63b676b3e638aae10055a795ad25a820451fdfc79afb9f99b1605bad61560679',
48+
'8.4-darwin-arm64': 'e1ce00874e398bcef5884f2b9067a984480e7dd32d256747f89a75e5f183113c',
4949
'8.4-darwin-x64': '6fc09f87d9676bf8f22b05cf77a91b99ee120414e9a028c6f357c18df531fa83',
5050
'8.4-linux-x64': '3624293e0556625e19f4483d74eac21d41b70d21bdbb7e8ea3e1247303886148',
5151
'8.4-linux-arm64': '1be37b0cc533edc691632828ecdaddd84eaa0f71bb9c06a3458347aa13da2987',

0 commit comments

Comments
 (0)