Build/Test Tools: Wait for the database before running WP-CLI commands. - #12734
Build/Test Tools: Wait for the database before running WP-CLI commands.#12734adimoldovan wants to merge 4 commits into
Conversation
`npm run env:install` began with `wp config create`, which performs a database connectivity check. The script's only readiness wait ran six WP-CLI calls later, and waited on the web port rather than the database, so nothing actually confirmed the database was reachable before it was used. The `cli` service does depend on `mysql` being `service_healthy`, but that healthcheck runs `mysqladmin ping -h localhost` inside the mysql container, which connects over the unix socket. While a cold volume is initialising, that socket is served by the entrypoint's temporary server, which listens on socket only (`port: 0`). The healthcheck therefore reports healthy while the real server is not yet listening on TCP, and connections from the `cli` container are refused. Retry the `config create` call until it succeeds. Its built-in connectivity check exercises exactly the path that matters, cli container to host `mysql` over TCP, so retrying it doubles as the readiness probe without adding a separate wait or relying on a published host port. Failures that retrying cannot fix, such as the environment never having been started, still exit immediately, and the timeout reports the underlying error.
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. |
There was a problem hiding this comment.
Pull request overview
This PR hardens the local Docker environment installer (npm run env:install) against intermittent cold-start database readiness races by retrying the initial WP-CLI config create step until the MySQL service is actually accepting TCP connections.
Changes:
- Wraps
wp config createin a newwp_cli_retry()helper to retry DB connectivity checks for up to 120s on cold volumes/slow CI. - Updates
wp_cli()to support configurablestdiohandling so retries can suppress noisy output while still preserving success output. - Adds timeout failure messaging with a pointer to MySQL container logs.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
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.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
tools/local-env/scripts/install.js:101
err.stderris treated as the sole source of output, but it's truthy even when it's an empty Buffer. In that caseoutputbecomes an empty string and you lose the actual failure details inerr.message(and sometimeserr.stdout), which can also cause theis not runningdetection to be missed and contradicts the goal of surfacing the underlying error after the timeout.
const output = err.stderr ? err.stderr.toString() : err.message;
// Retrying only helps while the environment is still starting up. A missing container
// means it was never started, so there is nothing to wait for.
if ( output.includes( 'is not running' ) ) {
There was a problem hiding this comment.
The cold-database retry worked and all 197 CI checks passed or skipped.
But an empty stderr buffer discards available error details, potentially producing a silent two-minute retry.
Testing with head `8a369601bcb0921568a7ae2e030aac0f027540aa
The forced cold-database path waited and successfully generated wp-config.php; the missing-environment path also retained its fast failure.
The one blocker is in tools/local-env/scripts/install.js: err.stderr is a truthy Buffer even when empty. The current expression can therefore discard stdout and err.message, retry for two > minutes, then report no underlying error.
Could we select the first non-empty value from stderr, stdout, and err.message before checking and reporting the failure?
`err.stderr` is a Buffer, which is truthy even when empty, so an error with no stderr output resolved to an empty string rather than falling back to `err.message`. That discarded the underlying error at the end of the retry window, and also defeated the `is not running` check, so a missing container could retry for the full timeout and then report nothing. Use the first of stderr, stdout, and `err.message` that actually captured something.
Good catch. Fixed with 921d2a9 - now takes the first non-empty of stderr, stdout, err.message, with a fallback string. Note: current failure in this run should be addressed by #12735 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
tools/local-env/scripts/install.js:120
wp_cli_retry()retries on any WP-CLI failure (except the "is not running" case). That means deterministic failures (bad arguments, missing files/paths, permissions, etc.) will pause for up to 120s before exiting, which can be confusing and makes non-DB errors slower to diagnose. Consider only retrying on known transient DB-connectivity errors and failing fast for everything else.
if ( ! notified ) {
notified = true;
console.log( waiting );
}
Trac ticket: https://core.trac.wordpress.org/ticket/65742
npm run env:installintermittently fails on a cold database volume:The script's first action,
wp config create, connects to the database. Its only readiness wait runs six WP-CLI calls later and waits on the web port, never the database. Nothing retries.clidoes depend onmysqlbeingservice_healthy, but that healthcheck pings over the unix socket, which the entrypoint's temporary server answers while a cold volume is still initialising — before the real server listens on TCP.This retries
config createuntil it succeeds. Its own connectivity check exercises the path that matters,clitomysqlover TCP, so retrying it is the readiness probe.Happy path is unchanged and adds no time. Forgetting
npm run env:startstill fails immediately with the existing hint. A database that never comes up fails after 2 minutes with the underlying error.Testing Instructions
Only reproduces on a cold volume.
npm run env:starthides it, becausecomposer updatetakes long enough to warm the database:Timing-dependent, so repeat it. To force the window open, add a
docker-compose.override.yml:With that, trunk fails every run and this branch passes every run. Also check
npm run env:start && npm run env:installstill works and is no slower.Use of AI Tools
AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Opus 5
Used for: Investigating the root cause, writing the change, and building the reproduction described above. The diagnosis was verified against container logs and probe timings rather than assumed. I reviewed the implementation and the test results and take responsibility for them.
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.