Build/Test Tools: Propagate and retry the local environment commands - #12735
Build/Test Tools: Propagate and retry the local environment commands#12735adimoldovan wants to merge 13 commits into
Conversation
…commands.
`start.js` runs `docker compose up` through `spawnSync` and never inspects the
result, so a failed pull does not fail the script. Execution continues into
`composer update -W` with containers that may not exist, and the error surfaces
later and in the wrong place.
`docker.js` calls `process.exit( returns.status )`, and `status` is `null` when
Docker cannot be spawned. `process.exit( null )` exits 0, so `npm run env:pull`
reports success when the Docker CLI is missing.
Reproduce with:
LOCAL_PHP=this-tag-does-not-exist npm run env:start; echo $?
See #65745.
There was a problem hiding this comment.
Pull request overview
This PR updates the local Docker environment helper scripts to correctly propagate failures from spawnSync() so npm commands fail at the correct step (instead of continuing after Docker failures and surfacing errors later).
Changes:
- Capture and check the result of
docker compose upinstart.js, exiting non-zero when container startup fails. - Avoid exiting 0 when the Docker CLI cannot be spawned in
docker.jsby handlingstatus === nulland reporting a clearer error.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tools/local-env/scripts/start.js | Capture spawnSync() result for docker compose up and exit non-zero on failure to start/pull containers. |
| tools/local-env/scripts/docker.js | Emit an error when Docker cannot be spawned and ensure a non-zero exit code when appropriate. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
…nment command. `docker.js` exited 0 for any signal, so a command killed by SIGTERM or SIGKILL reported success. That reintroduced the false success this ticket set out to remove: the retry loop in `reusable-phpunit-tests-v3.yml` branches on the status of `npm run env:pull` and would treat a killed pull as a completed one. Restrict the exemption to SIGINT, which is how a long-running command such as `env:logs` is normally ended. Report every other signal and exit non-zero. `start.js` exempts no signal, because `env:start` runs `composer update -W` next and that must not run against containers that never came up. Say so in the comment, so the difference between the two files is deliberate. Also fold the unreachable `up.error` branch in `start.js` into the failure message. The `docker info` check above it already throws when the Docker CLI is missing or the daemon is down.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
There was a problem hiding this comment.
Tested with head a88479c
Confirmed failed image pulls and a missing Docker executable exit non-zero. SIGTERM exits non-zero, SIGINT remains clean for interactive commands, and a normal environment start and install > succeeded
The affected code shape was also verified on 6.8, 6.9, and 7.0.
Good to land.
|
This change looks correct, I'd like a 2nd review from a core committer on the backports since that will involve SVN commits to 7.0, 6.9, and 6.8. CC @aaronjorbin @desrosj @johnbillion |
…egistry. Moves the retry loop from [63163] into `utils.js` so `start.js` can use it too, and adds `ensure_env_file()` so every script creates `.env` before Compose reads it. `docker.js` retries `pull` and any command containing `composer`. `start.js` retries `docker compose up`. Folds in the changes from PR WordPress#12736. See #65745.
adimoldovan
changed the title
@lancewillett this needs a new round of review, I just folded #12736 into it. |
…mpted. Stop the retry loop when the command could not be spawned at all. That error fails the same way every time, so re-attempting it only delays the report by 30 seconds. Re-attempt `composer install` and `composer update`, rather than every Composer run. `typecheck:php` and `typecheck:php:baselines` reach no registry, so a PHPStan failure is a real result and was being reported three times. See #65745.
Composer accepts global options before the subcommand, so `env:composer -- -n update` placed `-n` where the subcommand was expected and lost the retry. See #65745.
Global options can precede the command, and `--working-dir` takes a separate value, so searching the arguments matched tokens that were never the command. Both `env:composer -- --working-dir update validate` and `typecheck:php -- update` were re-attempted three times despite reaching no registry. See #65745.
Trac ticket: https://core.trac.wordpress.org/ticket/65745
start.jsanddocker.jspropagate the exit status of the Docker Compose command they run, falling back to 1 whenstatusisnull.docker.jstreats SIGINT as cancellation rather than failure.start.jsstops beforecomposer update -Wwhen the containers did not come up.compose_with_retry()inutils.jsre-attempts a command three times, 10 and 20 seconds apart. It coversdocker compose pullanddocker compose up, which reach Docker Hub, andcomposer installandcomposer update, which reach repo.packagist.org. Every other command runs once, includingtypecheck:phpandtypecheck:php:baselines. The loop stops early when the command was killed by a signal or could not be spawned.ensure_env_file()inutils.jscreates.envfrom.env.example, resolved against the repository root.start.js,docker.jsandinstall.jsall call it, sonpm run env:pullno longer resolves image tags without it.Backport
execSync, which throws.envcopycopyFileSyncTesting instructions
The exit status of a failed start. Before this change it is 0. After it, 1, after three attempts (~33s).
The exit status when Docker cannot be run. Before this change it is 0, reported after 30s of retries. After it, 1, reported immediately.
The pull retry. Three attempts, 10 and 20 seconds apart, ending with
docker compose pull failed after 3 attempts.and exit 1.Commands that are not re-attempted run once. The first exits 1 in about a second with no retry message.
Ctrl-C ends
env:logsand does not restart it..envis created before the image tags are resolved. Delete.env, then run the pull and confirm.envexists afterwards. Before this change onlynpm run env:startcreated it.A cold start still works.
Use of AI Tools
AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Opus 5
Used for: Investigating the CI failures, writing the patch, and iterative code review. Every behaviour and branch range described above was verified locally.
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.