[CLI] Run PHP in clean worker processes#4146
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a runInFreshProcess() execution path for CLI workers so consumers can run PHP in a clean runtime (no inherited WP symbols), safely under worker concurrency, with per-run unique script paths and cleanup.
Changes:
- Introduces
runInFreshProcess()for Blueprint v1/v2 worker threads with per-worker sequencing to avoid queue poisoning on failures. - Executes
coderequests via unique temporary script paths and disposes the per-run request handler after execution. - Adds an integration test validating WP symbols aren’t inherited across fresh runs while still allowing WP to be loaded when required.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| packages/playground/cli/tests/run-cli.spec.ts | Adds an integration test covering fresh-process execution isolation vs. booted worker state. |
| packages/playground/cli/src/blueprints-v2/worker-thread-v2.ts | Implements runInFreshProcess() with serialized execution, unique script paths, and cleanup. |
| packages/playground/cli/src/blueprints-v1/worker-thread-v1.ts | Implements runInFreshProcess() for v1, mirroring v2’s approach (with some config parity gaps). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const requestHandler = await bootRequestHandler({ | ||
| siteUrl: options.siteUrl, | ||
| phpVersion: options.phpVersion, | ||
| maxPhpInstances: 1, | ||
| createPhpRuntime: createPhpRuntimeFactory( | ||
| options, | ||
| this.fileLockManager | ||
| ), | ||
| onPHPInstanceCreated: async (php) => { | ||
| await mountResources(php, options.mountsBeforeWpInstall); | ||
| await mountResources(php, options.mountsAfterWpInstall); | ||
| }, | ||
| sapiName: 'cli', | ||
| cookieStore: false, | ||
| pathAliases: options.pathAliases, | ||
| }); |
There was a problem hiding this comment.
No change here: Blueprint v1 does not expose the v2 networking option, and this fresh handler intentionally mirrors the existing v1 bootRequestHandler() configuration. Adding v2-only CA/INI behavior here would create new v1 semantics rather than preserve the active worker configuration.
| async runInFreshProcess(request: PHPRunOptions): Promise<PHPResponse> { | ||
| const execution = this.freshProcessQueue.then(async () => { | ||
| const options = this.requestHandlerOptions; | ||
| if (!options || !this.fileLockManager) { | ||
| throw new Error('Playground request handler is not booted'); | ||
| } | ||
|
|
||
| const requestHandler = await bootRequestHandler({ | ||
| siteUrl: options.siteUrl, | ||
| phpVersion: options.phpVersion, | ||
| maxPhpInstances: 1, | ||
| createFiles: { | ||
| '/internal/shared/ca-bundle.crt': | ||
| rootCertificates.join('\n'), | ||
| }, | ||
| phpIniEntries: getNetworkingPhpIniEntries( | ||
| options.networking ?? true | ||
| ), | ||
| createPhpRuntime: createPhpRuntimeFactory( | ||
| options, | ||
| this.fileLockManager | ||
| ), |
There was a problem hiding this comment.
Keeping this inline with the existing v2 bootRequestHandler() setup. Joining the platform certificate list is negligible beside constructing a new PHP runtime, and a cached module-level value would add state without changing the behavior under test.
| async runInFreshProcess(request: PHPRunOptions): Promise<PHPResponse> { | ||
| const execution = this.freshProcessQueue.then(async () => { | ||
| const options = this.requestHandlerOptions; | ||
| if (!options || !this.fileLockManager) { | ||
| throw new Error('Playground request handler is not booted'); | ||
| } | ||
|
|
||
| const requestHandler = await bootRequestHandler({ | ||
| siteUrl: options.siteUrl, | ||
| phpVersion: options.phpVersion, | ||
| maxPhpInstances: 1, | ||
| createPhpRuntime: createPhpRuntimeFactory( | ||
| options, | ||
| this.fileLockManager | ||
| ), | ||
| onPHPInstanceCreated: async (php) => { | ||
| await mountResources(php, options.mountsBeforeWpInstall); | ||
| await mountResources(php, options.mountsAfterWpInstall); | ||
| }, | ||
| sapiName: 'cli', | ||
| cookieStore: false, | ||
| pathAliases: options.pathAliases, | ||
| }); |
There was a problem hiding this comment.
The shared execution invariants are deliberately parallel, while handler construction remains version-specific: v2 owns networking/CA configuration and v1 does not. Extracting a helper would require a larger union/config abstraction and obscure those versioned contracts. The integration test exercises both through the typed pooled worker surface, while each implementation remains adjacent to its existing boot path.
| this.freshProcessQueue = execution.catch(() => undefined); | ||
| return await execution; |
There was a problem hiding this comment.
Addressed in 3d89bb2. The integration test now submits invalid PHP, asserts rejection, then verifies a subsequent fresh execution returns recovered, covering the queue-unpoisoning guarantee.
|
@brandonpayton Review follow-up is ready: queue recovery now has explicit failure-then-success coverage in |
|
Downstream full-stack validation completed against this PR plus #4108 using https://github.com/chubes4/wordpress-playground/tree/proof/bet12-clean-worker-abi.
Downstream PR: Automattic/wp-codebox#1938 |
Fixes #4145.
What changed
runInFreshProcess()to Blueprint v1 and v2 CLI workers./internal/eval.php, preventing concurrent parse corruption.Why
Programmatic CLI consumers need project-owned bootstraps to start without PHP symbols inherited from the long-lived worker. The same API also needs to remain safe when several pooled CLI workers execute code concurrently.
Verification
npx nx run playground-cli:typechecknpx nx run playground-cli:lintnpx nx run playground-cli:test-playground-cli --testNamePattern="should run PHP without inheriting booted WordPress symbols"