[CLI] Fix run-cli leak which was revealed by repeated runCLI() calls during test (#2888)

## Motivation for the change, related issues

Prior to this PR, the initial Playground CLI worker was not being
cleaned up when Playground CLI was disposed.

## Implementation details

This PR:
- Adds the initial worker and its Playground handle to the list of items
to be disposed when Playground CLI is disposed.
- Prevents multiple workers from being requested when not running the
`server` command.

## Testing Instructions (or ideally a Blueprint)

- CI

Author: brandonpayton

packages/playground/cli/src/run-cli.ts

@@ -333,6 +333,12 @@ export async function parseOptionsAndRunCLI(argsToParse: string[]) {
 				}
 
 				if (args['experimental-multi-worker'] !== undefined) {
+					const cliCommand = args._[0] as string;
+					if (cliCommand !== 'server') {
+						throw new Error(
+							'The --experimental-multi-worker flag is only supported when running the server command.'
+						);
+					}
 					if (args['experimental-multi-worker'] <= 1) {
 						throw new Error(
 							'The --experimental-multi-worker flag must be a positive integer greater than 1.'
@@ -549,10 +555,10 @@ export async function runCLI(args: RunCLIArgs): Promise<RunCLIServer | void> {
 	let loadBalancer: LoadBalancer;
 	let playground: RemoteAPI<PlaygroundCliWorker>;
 
-	const playgroundsToCleanUp: {
-		playground: RemoteAPI<PlaygroundCliWorker>;
-		worker: Worker;
-	}[] = [];
+	const playgroundsToCleanUp: Map<
+		Worker,
+		RemoteAPI<PlaygroundCliWorker>
+	> = new Map();
 
 	/**
 	 * Expand auto-mounts to include the necessary mounts and steps
@@ -624,12 +630,19 @@ export async function runCLI(args: RunCLIArgs): Promise<RunCLIServer | void> {
 			const serverUrl = `http://${host}:${port}`;
 			const siteUrl = args['site-url'] || serverUrl;
 
-			// Create the blueprints handler
-			const targetWorkerCount = args.experimentalMultiWorker ?? 1;
-			// Account for the initial worker which is discarded after setup.
-			const totalWorkerCountIncludingSetupWorker = targetWorkerCount + 1;
+			const targetWorkerCount =
+				args.command === 'server'
+					? args.experimentalMultiWorker ?? 1
+					: 1;
+			const totalWorkersToSpawn =
+				args.command === 'server'
+					? // Account for the initial worker
+					  // which is discarded by the server after setup.
+					  targetWorkerCount + 1
+					: targetWorkerCount;
+
 			const processIdSpaceLength = Math.floor(
-				Number.MAX_SAFE_INTEGER / totalWorkerCountIncludingSetupWorker
+				Number.MAX_SAFE_INTEGER / totalWorkersToSpawn
 			);
 
 			/*
@@ -875,10 +888,12 @@ export async function runCLI(args: RunCLIArgs): Promise<RunCLIServer | void> {
 
 				disposing = true;
 				await Promise.all(
-					playgroundsToCleanUp.map(async ({ playground, worker }) => {
-						await playground.dispose();
-						await worker.terminate();
-					})
+					[...playgroundsToCleanUp].map(
+						async ([worker, playground]) => {
+							await playground.dispose();
+							await worker.terminate();
+						}
+					)
 				);
 				if (server) {
 					await new Promise((resolve) => server.close(resolve));
@@ -889,7 +904,7 @@ export async function runCLI(args: RunCLIArgs): Promise<RunCLIServer | void> {
 			// Kick off worker threads now to save time later.
 			// There is no need to wait for other async processes to complete.
 			const promisedWorkers = spawnWorkerThreads(
-				totalWorkerCountIncludingSetupWorker,
+				totalWorkersToSpawn,
 				handler.getWorkerType(),
 				({ exitCode, workerIndex }) => {
 					// We are already disposing, so worker exit is expected
@@ -929,6 +944,10 @@ export async function runCLI(args: RunCLIArgs): Promise<RunCLIServer | void> {
 							fileLockManagerPort,
 							nativeInternalDirPath
 						);
+					playgroundsToCleanUp.set(
+						initialWorker.worker,
+						initialPlayground
+					);
 
 					await initialPlayground.isReady();
 					wordPressReady = true;
@@ -968,16 +987,16 @@ export async function runCLI(args: RunCLIArgs): Promise<RunCLIServer | void> {
 					// be configured differently than post-boot workers.
 					// For example, we do not enable Xdebug by default for the initial worker.
 					await loadBalancer.removeWorker(initialPlayground);
-					// TODO: Wrap in a cleanup function and reuse for all worker cleanup.
 					await initialPlayground.dispose();
 					await initialWorker.worker.terminate();
+					playgroundsToCleanUp.delete(initialWorker.worker);
 				}
 
 				logger.log(`Preparing workers...`);
 
 				// Boot additional workers using the handler
 				const initialWorkerProcessIdSpace = processIdSpaceLength;
-				// Just take the first Playground instance to be relayed to others.
+				// Just take the first Playground instance to be returned to the caller.
 				[playground] = await Promise.all(
 					workers.map(async (worker, index) => {
 						const firstProcessId =
@@ -996,11 +1015,10 @@ export async function runCLI(args: RunCLIArgs): Promise<RunCLIServer | void> {
 								nativeInternalDirPath,
 							});
 
-						playgroundsToCleanUp.push({
-							playground: additionalPlayground,
-							worker: worker.worker,
-						});
-
+						playgroundsToCleanUp.set(
+							worker.worker,
+							additionalPlayground
+						);
 						loadBalancer.addWorker(additionalPlayground);
 
 						return additionalPlayground;
@@ -1035,6 +1053,7 @@ export async function runCLI(args: RunCLIArgs): Promise<RunCLIServer | void> {
 				if (await playground?.fileExists(errorLogPath)) {
 					phpLogs = await playground.readFileAsText(errorLogPath);
 				}
+				await disposeCLI();
 				throw new Error(phpLogs, { cause: error });
 			}
 		},

packages/playground/cli/tests/test-running-unbuilt-cli.sh

@@ -47,24 +47,13 @@ function test_playground_cli() {
 	fi
 }
 
-function test_playground_cli_multi_worker() {
-	MULTIWORKER_WP_PATH="$HOME/playground-cli-multi-worker-wp"
-	mkdir -p "$MULTIWORKER_WP_PATH"
-
-	# TODO: Also test with asyncify once we multiple workers there.
-	test_playground_cli unbuilt-jspi \
-		--mountBeforeInstall="$MULTIWORKER_WP_PATH:/wordpress" \
-		--experimentalMultiWorker
-}
-
 echo
 test_playground_cli unbuilt-asyncify
 echo
 test_playground_cli unbuilt-jspi
 echo
 
-
-test_playground_cli_multi_worker
+test_playground_cli unbuilt-asyncify --experimental-multi-worker
+echo
+test_playground_cli unbuilt-jspi --experimental-multi-worker
 echo
-echo 'Retesting multi-worker to test with a pre-existing WordPress installation where we have seen bugs.'
-test_playground_cli_multi_worker

packages/playground/test-built-npm-packages/es-modules-and-vitest/run-tests.ts

@@ -21,6 +21,7 @@ function red(text: string) {
 type Result = {
 	phpVersion: string;
 	code: number | null;
+	timeout?: boolean;
 };
 
 const results: Result[] = [];
@@ -47,7 +48,7 @@ for (const phpVersion of SupportedPHPVersions.filter(
 		}
 	);
 
-	await new Promise<void>((resolve) => {
+	const promiseToClose = new Promise<void>((resolve) => {
 		child.on('close', (code) => {
 			results.push({
 				phpVersion,
@@ -56,25 +57,49 @@ for (const phpVersion of SupportedPHPVersions.filter(
 			resolve();
 		});
 	});
+	const promiseToTimeout = new Promise<void>((resolve, reject) => {
+		setTimeout(() => {
+			console.error(`PHP ${phpVersion}: timed out.`);
+			reject(new Error('Test timed out'));
+		}, 30000);
+	});
+	try {
+		await Promise.race([promiseToClose, promiseToTimeout]);
+	} catch (e) {
+		results.push({
+			phpVersion,
+			code: null,
+			timeout: true,
+		});
+		child.kill('SIGKILL');
+	}
 }
 
 console.log('Results:');
 for (const result of results) {
-	console.log(
-		`PHP ${result.phpVersion}: ${
-			result.code === 0 ? green('PASS') : red('FAIL')
-		} with exit code ${result.code}`
-	);
+	if (result.timeout) {
+		console.log(red(`PHP ${result.phpVersion}: ${red('timed out')}.`));
+	} else {
+		console.log(
+			`PHP ${result.phpVersion}: ${
+				result.code === 0 ? green('PASS') : red('FAIL')
+			} with exit code ${result.code}`
+		);
+	}
 }
 
 const numPassed = results.filter((r) => r.code === 0).length;
-const numFailed = results.filter((r) => r.code !== 0).length;
+const numFailed = results.filter((r) => r.code !== 0 && !r.timeout).length;
+const numTimedOut = results.filter((r) => r.timeout).length;
 if (numPassed > 0) {
 	console.log(green(`${numPassed} / ${results.length} tests passed`));
 }
 if (numFailed > 0) {
 	console.log(red(`${numFailed} / ${results.length} tests failed`));
 }
+if (numTimedOut > 0) {
+	console.log(red(`${numTimedOut} / ${results.length} tests timed out`));
+}
 
 if (numFailed > 0) {
 	process.exit(1);

packages/playground/test-built-npm-packages/es-modules-and-vitest/tsconfig.json

@@ -0,0 +1,13 @@
+{
+	"compilerOptions": {
+		"target": "ES2019",
+		"module": "ES2022",
+		"moduleResolution": "node",
+		"esModuleInterop": true,
+		"forceConsistentCasingInFileNames": true,
+		"strict": true,
+		"skipLibCheck": true
+	},
+	"include": ["**/*.ts"],
+	"exclude": ["node_modules"]
+}