Fix Windows E2E: combined status reporting, PHP INI tilde fixes, and per-command CLI shutdown#4082
Conversation
📊 Performance Test ResultsComparing 7125696 vs trunk app-size
site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change (<50ms diff) |
os.tmpdir() can resolve to a Windows 8.3 short name (e.g. C:\Users\BUILDK~1\...) when the account name exceeds 8 chars, as on CI's buildkite-agent. That path was handed to PHP as auto_prepend_file (and the phpMyAdmin config), and PHP's parser chokes on the tilde with "syntax error, unexpected '~'", so every site failed to serve and its tests timed out on ERR_CONNECTION_REFUSED. Local dev with a short username produced no tilde, which is why this only reproduced on CI. Add getPhpSafeTmpDir() (long-form tmpdir on Windows) and use it for every temp path handed to PHP, and quote the auto_prepend_file directive to match the others. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vfk7xXMfsABh5JMYX51wiS
I forgot that I also added an affordance to the other PR to trigger the appropriate "stop sites" behavior when quitting the app during E2E tests. If that change isn't needed to make CI green again, though, then we don't need it for now |
gcsecsey
left a comment
There was a problem hiding this comment.
Happy to see the E2E tests passing on this.
|
Thanks for the review. I addressed the inline feedback in ee6a7c0: renamed the helper and removed the redundant comments, and the CI config comments are also gone as of 7125696. I agree with landing this PR as the root fix and will close #4070 and #4075 after it merges; the final CI run on the latest head is in progress. |
There was a problem hiding this comment.
Pull request overview
This PR restores Windows E2E stability by fixing CLI shutdown behavior during app quit, hardening native-PHP temp/INI path handling on Windows (8.3 tilde paths), and making CI status reporting accurately reflect the whole E2E matrix.
Changes:
- Switch Studio’s CLI execution back to per-command child process shutdown handling (instead of a shared
killAll()onwill-quit). - Fix native-PHP temp-path handling on Windows by resolving
os.tmpdir()to its long form and quoting/normalizingauto_prepend_filein PHP-dargs. - Update Buildkite E2E job reporting to a single combined GitHub status context and ensure canceled jobs fail rather than reporting success.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/studio/src/modules/cli/lib/execute-command.ts | Replaces shared CLI runner usage with a per-command fork + per-child quit cleanup. |
| apps/cli/lib/native-php/tmp-dir.ts | Adds helper to resolve Windows temp dir short paths to long form for PHP compatibility. |
| apps/cli/lib/native-php/site-setup.ts | Uses the resolved temp dir for site-url prepend file temp directory creation. |
| apps/cli/lib/native-php/phpmyadmin.ts | Uses the resolved temp dir for phpMyAdmin env/session paths. |
| apps/cli/lib/native-php/config.ts | Reuses resolved temp dir for opcache cache dir and quotes/normalizes auto_prepend_file. |
| .buildkite/pipeline.yml | Re-enables Windows E2E, adds timeouts, and moves GitHub status notification to the E2E group for combined reporting. |
| .buildkite/commands/run-e2e-tests.sh | Adds termination trapping so canceled/timed-out runs exit non-zero. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Only callers that opted-in with a `logPrefix` get stdout echoed to | ||
| // the main-process console. Commands like `preview list --format json` | ||
| // dump large structured payloads on stdout that would otherwise spam | ||
| // `npm start` output every time snapshots are fetched. | ||
| const logPrefix = options.logPrefix ? `[CLI - site ID ${ options.logPrefix }]` : null; | ||
| child.stdout?.on( 'data', ( data: Buffer ) => { |
There was a problem hiding this comment.
This restores the pre-#3954 file verbatim, and the same prefix exists in tools/common today. I agree it is worth cleaning up, but as a follow-up rather than widening this revert.
| function appQuitHandler() { | ||
| const pid = child.pid; | ||
| child.removeAllListeners(); | ||
|
|
There was a problem hiding this comment.
In this app the prevented quit always ends in app.exit() within seconds, so the stale-listener window is bounded to shutdown. Since this PR intentionally restores the battle-tested pre-#3954 behavior, I would rather keep it verbatim and revisit listener hygiene in the shared createCliRunner as a follow-up.
Related issues
Proposed Changes
Windows E2E has been broken since June 29 by two independent regressions that merged the same day. This PR carries the minimal combination that fixes both, plus CI reporting fixes so failures can't hide:
1. Per-command CLI shutdown handling (from #4070). #3954's shared
killAll()onwill-quitruns one listener after the quit handler that spawnssite stop --all, killing the stop command before it stops any site. Sites leaked into the machine-global process-manager daemon until its capacity cap was exhausted — the 3-hour hangs. Restoring per-child quit handlers lets the quit-time stop survive; verified on CI: quit-time stops complete in under a second (previously a 20-second timeout on every session) and zero capacity errors.2. PHP INI tilde fixes. #3988 passed the site-url prepend file (reprint's runtime: constants + SQLite loader) to PHP as an unquoted
-d auto_prepend_file=value. On machines where the temp path contains a Windows 8.3 short name (e.g.C:\Users\BUILDK~1\...— any username over 8 characters), PHP's INI parser fails on the~(syntax error, unexpected '~'), keeps only the prefix, and every request dies withFatal error: Failed opening required 'C:\Users\BUILDK'before WordPress boots. This broke every page load of every native-PHP site on affected machines and caused the ~25 Windows E2E failures. Fixed at both ends:auto_prepend_fileis now quoted and backslash-normalized via the existingtoPhpIniPath(), like every other path directive.getPhpSafeTmpDir()resolves the Windows short name to its long form for every temp path handed to PHP (opcache dir, phpMyAdmin config/sessions, site-url prepend dir).3. Honest CI reporting. The mac/Windows/Linux E2E jobs all posted to the same "E2E Tests" GitHub status and the last writer won, so a fast green mac job masked a failing or still-running Windows job. The notify now lives on the E2E group: one status, pending until every platform finishes. Also:
run-e2e-tests.shtraps termination so a canceled/timed-out job can't record exit status 0 and turn the build green (observed in build 18744).4. Windows E2E re-enabled with a 100-minute job cap. Prior verification of these fixes together (#4075, build 18789): 47 passed / 0 failed / 26 minutes — the first green Windows E2E since June 29. This PR's own CI re-verifies the combination as extracted here.
Deliberately not included, pending their own review: #4041 (bounded daemon socket requests, daemon force-settle, leaked-daemon reaping) and #4061's per-home daemon isolation. This PR's CI run doubles as the experiment showing whether they are required for green E2E or are hardening.
Testing Instructions
syntax error, unexpected '~'in daemon logs, nosite stop --all command timed outlines, and noCAPACITY_LIMIT_REACHEDerrors.npm test -- apps/cli/tests/ apps/studio/src/tests/passes.Pre-merge Checklist