Fix Windows E2E hangs: restore per-command CLI shutdown and isolate the daemon per home#4075
Closed
gavande1 wants to merge 8 commits into
Closed
Fix Windows E2E hangs: restore per-command CLI shutdown and isolate the daemon per home#4075gavande1 wants to merge 8 commits into
gavande1 wants to merge 8 commits into
Conversation
On Windows the daemon's control/events sockets were hardcoded named pipes, so every process shared one machine-global daemon regardless of STUDIO_PROCESS_MANAGER_HOME — silently defeating the per-run isolation that already works on macOS/Linux and that the CLI test harness relies on. Derive the pipe name from the home when a custom one is set; the default home keeps its original fixed name so the desktop app and CLI still share a single daemon exactly as before. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vfk7xXMfsABh5JMYX51wiS
The build skipped the PHP download whenever apps/studio/php-bin/<packageId>/ already existed, so a package republished in place (same packageVersion, new checksum — e.g. adding the Windows VC++ runtime DLLs) was never picked up on reused CI agents, which kept bundling a stale php.exe. Record the archive SHA on install and re-download when it no longer matches the expected SHA. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vfk7xXMfsABh5JMYX51wiS
Shorten stale-PHP log line to satisfy prettier Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vfk7xXMfsABh5JMYX51wiS @
Temporary instrumentation to find why native PHP doesn't serve on Windows CI. Routes the E2E process-manager daemon and its per-process logs under test-results/daemon-logs so they upload as artifacts, and logs any unexpected php.exe exit code (a Windows loader/DLL failure exits 3221225781 with no stderr). Revert once diagnosed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vfk7xXMfsABh5JMYX51wiS
05d1923 to
01bd9b3
Compare
gcsecsey
approved these changes
Jul 3, 2026
gcsecsey
left a comment
Member
There was a problem hiding this comment.
Thanks for creating this, I think it makes sense to land these fixes together, and continue investigating the failing tests separately. 👍
Contributor
|
This does not seem to be the cause of the failing tests. My two cents are that we should zero in on what's causing the failures (which you appear to have done excellently with #4070 and the bug fix for #3988), ship the fixes, and only then look into amendments. The failing E2E tests tell us something is broken in production, and there's always a risk that we'll tape over real issues if we change how they work while trying to fix an outage. |
Merged
1 task
This was referenced Jul 6, 2026
gavande1
added a commit
that referenced
this pull request
Jul 6, 2026
…per-command CLI shutdown (#4082) ## Related issues - AINFRA-2588 (Investigate Studio Windows E2E hangs in Buildkite) - Supersedes the split verification work in #4075; overlaps with #4070 (included as-is) and #4061 (its tilde fix is included; its daemon isolation is not) ## 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()` on `will-quit` runs one listener after the quit handler that spawns `site 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 with `Fatal 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_file` is now quoted and backslash-normalized via the existing `toPhpIniPath()`, 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.sh` traps 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 - **CI**: all three E2E platforms should pass; the "E2E Tests" GitHub status stays pending until mac, Windows, and Linux all finish, then reports one combined result. The Windows job should show no `syntax error, unexpected '~'` in daemon logs, no `site stop --all command timed out` lines, and no `CAPACITY_LIMIT_REACHED` errors. - **Unit**: `npm test -- apps/cli/tests/ apps/studio/src/tests/` passes. - **Manual (Windows)**: on a machine whose user profile path gets 8.3-mangled (username over 8 characters), create and open a native-PHP site — pages render instead of a PHP fatal. ## Pre-merge Checklist - [x] Have you checked for TypeScript, React or other console errors? --------- Co-authored-by: Gergely Csecsey <gergely.csecsey@automattic.com>
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.
Related issues
Proposed Changes
Windows E2E has been broken since June 29 by two independent regressions that merged the same day, which is why earlier single-PR reverts never fixed it:
Extract the CLI spawn helper to @studio/common #3954 caused the 3-hour hangs. Its
killAll()onwill-quitruns one listener after the quit handler that spawnssite stop --all, so the stop command is killed before it can stop any site. Sites leak into the machine-global process-manager daemon until its capacity cap is exhausted and every subsequent site creation times out. Fixed here by restoring per-command CLI shutdown handling (Test restoring per-command CLI shutdown handling #4070): each CLI child registers its own quit-time kill handler when spawned, so the quit-time stop survives. Verified on CI: quit-time stops complete in under a second (previously a 20-second timeout on every session) and zero capacity errors.Fix native PHP sites loading assets from remote siteurl/home (STU-1925) #3988 caused the ~25 test failures. It made every native-PHP site start pass an
auto_prepend_file(the site-url prepend that loads reprint's runtime, including the SQLite loader) as an unquoted-ddirective. On CI Windows agents the file lives under an 8.3 short path (C:\Users\BUILDK~1\...), and the unquoted~fails PHP's INI parsing (syntax error, unexpected '~' in Unknown on line 4— visible in the daemon logs this PR now uploads). PHP drops the directive, WordPress boots without its SQLite driver, and every page renders an instant database error — which is why browser-driven tests saw non-WordPress pages while the app's own UI worked. Fixed by quoting the value like every other path directive. This never reproduced on developer machines because short usernames don't get 8.3-mangled.Also included:
connect EINVAL; logs are now copied into test-results at cleanup instead) and the PHP-binary re-download fix.Testing Instructions
site stop --all command timed outlines, noCAPACITY_LIMIT_REACHEDerrors, and nosyntax error, unexpected '~'lines in the daemon logs. Browser-driven tests (blueprints, wp-admin shortcuts, homepage) should pass.npm test -- apps/cli/tests/(817 tests pass).Pre-merge Checklist