Fix concurrent WP-CLI wrapper collisions#2004
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Root cause
runWpCli()derived/tmp/wp-codebox-wp-cli-<n>.phpfromthis.commands.length. Executions are appended tothis.commandsonly after completion, so all concurrent invocations starting from the same runtime state observed the same value (2in the reported fuzz run). The bridge and argv helper variants added onlyDate.now(), which retained the same collision class for calls in one process tick. Concurrent writes could therefore fail withFile existsor expose a partially written payload to another worker.Ownership and atomicity
Every wrapper now uses:
/tmp/wp-codebox-wp-cli-<sanitized-runtime-id>-<128-bit-random-token>.phpThe runtime namespace is restricted to fixed safe filename characters and the token comes from
crypto.randomBytes(16). The existing PlaygroundwriteFileoperation completes before execution starts. Ownership begins only after that write resolves, so a rejected write never triggers deletion of a path the invocation did not create.All public, bridge, theme/plugin helper, and argv-based WP-CLI calls use the same helper. The sibling audit found no non-WP-CLI adapter reusing this wrapper allocation pattern.
Cleanup and errors
The runtime owner unlinks its exact path in
finally, covering successful execution, structured nonzero responses, wrapper crashes, timeouts/cancellation, and adapter exceptions without changing existing WP-CLI output cleaning or error classification.An initial revision also deleted the wrapper from a PHP shutdown handler. Fresh CI exposed that as a runtime-source integration regression because it could run before WP-CLI's own later shutdown handlers. Commit
3237ca76removed that redundant in-script cleanup and leaves lifecycle ownership exclusively with the host invocation. The base SHA's contracts run was green (30029484653), the focused integration passed twice locally after the correction, and the next fresh CI run passed.The programmatic Playground adapter now exposes the existing PHP WASM
unlinkprimitive; no parallel temp subsystem was introduced.Original evidence
The isolated two-factor bounded plan used concurrency 6 with 13 entries and produced 6 successes / 7 failures. Concurrent workers reused
/tmp/wp-codebox-wp-cli-2.php, including:The regression test explicitly reproduces the old deterministic allocator yielding one path for 16 concurrent allocations before exercising the replacement seam.
Stress results
Each invocation used a unique payload marker. All outputs matched only their own marker; no
File exists, parse errors, missing scripts, cross-payload execution, or orphan wrappers were observed. Both real runtimes also exercised intentional WP-CLI failure and returned/tmp/wp-codebox-wp-cli-*.phpto an empty baseline.The exact two-factor recipe was attempted as a final replay but Playground failed during bootstrap, before any workload command, because
https://playground.wordpress.net/wp-cli.pharcould not be downloaded. A cached WP 6.8 attempt reached plugin activation but the current two-factor branch requires WP 6.9. This is an external bootstrap limitation, not a wrapper failure; the same 13-entry/concurrency-6 execution shape is covered by the successful real-runtime stress above.Checks
npm run test:runtime-sources-playground-integrationpassed twice consecutively after the cleanup correctionnpm run test:wp-cli-temporary-scriptpassed: 3,200 invocationsnode --test tests/wp-cli-temporary-script.test.tspassednpx tsx tests/runtime-wp-cli-bridge.test.tspassednpm run buildpassedhomeboy review --changed-since origin/mainpassed audit, lint, and changed tests with no introduced findings (run2d0f10a3-50c4-4ae0-a325-051cba924dad)30059238805passedcontractsandworkflow-lint, includingtest:runtime-sources-playground-integrationnpm run checkreached a pre-existing untouchedcommand-registry-smokefailure:wordpress.collect-workload-result outputShape should mention outputSchema idplayground-command-errors-smokeCloses #2003