[CLI] Fix temp dir cleanup when Playground CLI server is killed #2836

brandonpayton · 2025-10-29T04:37:18Z

Motivation for the change, related issues

Today, when Playground CLI is killed via Ctrl-C (SIGINT), the temp dir is left behind. This is a problem because Playground CLI server runs until it is killed in such a manner.

Implementation details

This PR adds explicit temp dir cleanup when receiving the signals SIGINT or SIGTERM.

Testing Instructions (or ideally a Blueprint)

Run Playground CLI server in the console with the --verbose=debug flag.
Note the temp dir it created and printed in the CLI output
Kill the server with Ctrl-C
Confirm that the director no longer exists

Prior to this patch, the temp dir still exists after this test. After this patch, the temp dir is removed.

brandonpayton · 2025-10-29T04:38:27Z

This is pretty straightforward, but the fact that the signal handlers explicitly kill all the workers causes the following to be printed while exiting:

Worker 2 exited with code 1

Worker 3 exited with code 1

Worker 1 exited with code 1

Worker 0 exited with code 1

It would be good to fix this before merging.

brandonpayton

There are some test failures that may be due to these changes, so I'll look at those next. In the meantime, here are some comments about the changes.

brandonpayton · 2025-10-29T19:28:08Z

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

-						return;
-					}
-					process.exit(1);
+					// @TODO: Should we respawn the worker if it exited with an error and the CLI is not shutting down?


I removed the explicit exit for primary worker because I believe that runCLI()'s promise will be rejected if the primary worker fails during boot. And I do not believe the primary worker is special any longer.

If the user requested 5 workers then yes, let's keep that amount of workers around. Otherwise we may run out of workers eventually if one crashes every 5 minutes.

I love not having a special primary worker btw

brandonpayton · 2025-10-29T19:30:04Z

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

+					await disposeCLI();
+					return;
 				} else if (args.command === 'run-blueprint') {
 					logger.log(`Blueprint executed`);
-					process.exit(0);
+					await disposeCLI();
+					return;


Here we remove explicit exits from runCLI() and let the callers like parseOptionsAndRunCLI() decide based on whether commands succeed or fail.

brandonpayton · 2025-10-29T19:34:16Z

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

+export async function runCLI(
+	args: RunCLIArgs & { command: 'build-snapshot' | 'run-blueprint' }
+): Promise<void>;
+export async function runCLI(
+	args: RunCLIArgs & { command: 'server' }
+): Promise<RunCLIServer>;
+export async function runCLI(args: RunCLIArgs): Promise<RunCLIServer | void>;


These overloads are declared for convenience so runCLI() can return different things depending on the CLI command without forcing the callers (mostly automated tests) to check return values.

^ Will add this as an inline comment.

brandonpayton · 2025-10-29T19:37:57Z

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

 	return startServer({
 		port: args['port'] as number,
-		onBind: async (server: Server, port: number): Promise<RunCLIServer> => {
+		onBind: async (server: Server, port: number) => {


The type is removed here because it is easier if TypeScript to infers it. There should be little risk. What this handler returns becomes the return value of runCLI() which is further type-checked to match the runCLI() return value.

brandonpayton · 2025-10-29T19:40:02Z

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

-					process.exit(1);
+					throw new Error('Could not configure Xdebug', {
+						cause: error,
+					});


I removed the explicit exit here so the callers can decide whether to exit or not. In automated tests, this case will no longer cause the test runner process (or worker) to exit, and in regular operation parseOptionsAndRunCLI() will catch this error and exit.

I'm happy as long as we're protected from this scenario:

The CLI user starts CLI and asks for XDebug with configuration

Configuration failed, there's a faint log line in the CLI

The user never noticed it

They're scrambling to get Xdebug to work and start thinking "this Playground CLI thing doesn't work"

I've struggled to get Xdebug to work too many times to let an Xdebug-related failure through.

In automated tests, this case will no longer cause the test runner process (or worker) to exit

Oh, it exited the test runner process? Should we run that in a sub-process instead?

brandonpayton · 2025-10-29T19:43:32Z

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

+			// - we can avoid multiple, conflicting dispose attempts
+			// - logging that a worker exited while the CLI itself is exiting
+			let disposing = false;
+			const disposeCLI = async function disposeCLI() {


This function is now also used to clean up after run-blueprint and build-snapshot.

The tmp-promise lib handles temp dir cleanup in the case of exit, and our full cleanup is async which cannot be guaranteed to finish when exiting via process.exit().

brandonpayton · 2025-10-29T21:01:51Z

The worker-exited messages are no longer printed while the CLI itself exits. This is ready for review.

adamziel · 2025-10-31T13:01:06Z

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


-		await runCLI(cliArgs);
+		const cliServer = await runCLI(cliArgs);
+		if (cliServer === undefined) {


Do we have explicit result modes for every outcome? undefined means "success, no server", Server instance means there's a server running, an exception means an error. That sounds fine for the initial launch. Do we have any way of reporting errors further down the road other than the logger? If not, that may be a good exploration after this PR.

adamziel · 2025-10-31T13:01:23Z

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

+		}
+
+		const cleanUpCliAndExit = (() => {
+			// Remember we are already cleaning up to preclude the possibility


adamziel · 2025-10-31T13:08:46Z

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

+		// clean up after ourselves even if this process is being killed.
+		// NOTE: Windows does not support SIGTERM, but Node.js provides some emulation.
+		process.on('SIGINT', cleanUpCliAndExit);
+		process.on('SIGTERM', cleanUpCliAndExit);


nice. I just confirmed 'SIGKILL' cannot have a listener installed. Would SIGHUP be interesting for us here as well? Also, this caught my eye:

'SIGBUS', 'SIGFPE', 'SIGSEGV', and 'SIGILL', when not raised artificially using kill(2), inherently leave the process in a state from which it is not safe to call JS listeners. Doing so might cause the process to stop responding.

adamziel · 2025-10-31T13:09:03Z

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

 	process.stdout.isTTY ? `\x1b[33m${text}\x1b[0m` : text;

-export async function runCLI(args: RunCLIArgs): Promise<RunCLIServer> {
+// These overloads are declared for convenience so runCLI() can return


good idea, thank you

adamziel · 2025-10-31T13:12:58Z

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

+			// Remember whether we are already disposing so we can avoid:
+			// - we can avoid multiple, conflicting dispose attempts


Suggested change

// Remember whether we are already disposing so we can avoid:

// - we can avoid multiple, conflicting dispose attempts

// Remember whether we are already disposing so we can avoid:

// - multiple, conflicting dispose attempts

adamziel · 2025-10-31T13:17:06Z

I left a few notes, but this is a good change and helps so I'll go ahead and merge it as is.

## Motivation for the change, related issues Today, when Playground CLI is killed via Ctrl-C (SIGINT), the temp dir is left behind. This is a problem because Playground CLI server runs until it is killed in such a manner. ## Implementation details This PR adds explicit temp dir cleanup when receiving the signals SIGINT or SIGTERM. ## Testing Instructions (or ideally a Blueprint) - Run Playground CLI server in the console with the `--verbose=debug` flag. - Note the temp dir it created and printed in the CLI output - Kill the server with Ctrl-C - Confirm that the director no longer exists Prior to this patch, the temp dir still exists after this test. After this patch, the temp dir is removed.

Explicitly clean up temp dir when CLI is killed

734088d

brandonpayton requested a review from a team October 29, 2025 04:37

brandonpayton self-assigned this Oct 29, 2025

brandonpayton added [Type] Bug An existing feature does not function as intended [Package][@wp-playground] CLI labels Oct 29, 2025

brandonpayton added 4 commits October 29, 2025 15:08

Rework exiting so we do not log during expected exit

021fc02

Add Why comment

bfc4fa4

Say why we declare overloads for runCLI function

d2dba29

Add another Why comment

39f4f5f

brandonpayton commented Oct 29, 2025

View reviewed changes

brandonpayton added 2 commits October 29, 2025 16:13

Fix temp-dir tests

71b1595

Stop handling cleanup on exit

27aba3c

The tmp-promise lib handles temp dir cleanup in the case of exit, and our full cleanup is async which cannot be guaranteed to finish when exiting via process.exit().

brandonpayton marked this pull request as ready for review October 29, 2025 21:00

brandonpayton mentioned this pull request Oct 30, 2025

[CLI] Avoid breaks during Playground boot when --xdebug enabled #2835

Open

adamziel reviewed Oct 31, 2025

View reviewed changes

adamziel merged commit 1106074 into trunk Oct 31, 2025
29 checks passed

adamziel deleted the playground-cli-better-temp-dir-cleanup branch October 31, 2025 13:17

		// Remember whether we are already disposing so we can avoid:
		// - we can avoid multiple, conflicting dispose attempts

[CLI] Fix temp dir cleanup when Playground CLI server is killed #2836

[CLI] Fix temp dir cleanup when Playground CLI server is killed #2836

Uh oh!

Conversation

brandonpayton commented Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation for the change, related issues

Implementation details

Testing Instructions (or ideally a Blueprint)

Uh oh!

brandonpayton commented Oct 29, 2025

Uh oh!

brandonpayton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brandonpayton commented Oct 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamziel commented Oct 31, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

brandonpayton commented Oct 29, 2025 •

edited

Loading