Skip to content

Fix Windows E2E hangs: restore per-command CLI shutdown and isolate the daemon per home#4075

Closed
gavande1 wants to merge 8 commits into
trunkfrom
combine-cli-shutdown-fix-and-daemon-isolation
Closed

Fix Windows E2E hangs: restore per-command CLI shutdown and isolate the daemon per home#4075
gavande1 wants to merge 8 commits into
trunkfrom
combine-cli-shutdown-fix-and-daemon-isolation

Conversation

@gavande1

@gavande1 gavande1 commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

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:

  1. Extract the CLI spawn helper to @studio/common #3954 caused the 3-hour hangs. Its killAll() on will-quit runs one listener after the quit handler that spawns site 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.

  2. 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 -d directive. 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:

  • Isolate the process-manager daemon per home on Windows (Isolate the process-manager daemon per home on Windows #4061): the daemon pipe is scoped per home directory instead of machine-global, so even if a session leaks it cannot poison other sessions or other builds on the same agent. Includes that branch's daemon-log diagnostics (adjusted here to keep the daemon home in a short tmp path — the original diagnostic placed the daemon's Unix socket under a long test-results path, which exceeds the macOS/Linux socket path limit and broke every site creation with connect EINVAL; logs are now copied into test-results at cleanup instead) and the PHP-binary re-download fix.
  • CI reporting fixes: one combined "E2E Tests" GitHub status for all platforms (previously last-writer-wins let a green mac job mask a failing Windows job), a 100-minute job timeout, a cancellation trap so a timed-out job cannot record exit status 0 and turn the build green, and per-test/per-action log streaming with failure screenshots and daemon logs as artifacts.

Testing Instructions

  • CI (Windows E2E): the job should finish well under the 100-minute cap with no site stop --all command timed out lines, no CAPACITY_LIMIT_REACHED errors, and no syntax error, unexpected '~' lines in the daemon logs. Browser-driven tests (blueprints, wp-admin shortcuts, homepage) should pass.
  • GitHub status: the "E2E Tests" context stays pending until mac, Windows, and Linux all finish, then reports one combined result.
  • Unit: npm test -- apps/cli/tests/ (817 tests pass).

Pre-merge Checklist

  • Have you checked for TypeScript, React or other console errors?

@gavande1 gavande1 requested a review from a team as a code owner July 3, 2026 11:47
gcsecsey and others added 6 commits July 3, 2026 17:20
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
@gavande1 gavande1 force-pushed the combine-cli-shutdown-fix-and-daemon-isolation branch from 05d1923 to 01bd9b3 Compare July 3, 2026 11:52

@gcsecsey gcsecsey left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for creating this, I think it makes sense to land these fixes together, and continue investigating the failing tests separately. 👍

@fredrikekelund

Copy link
Copy Markdown
Contributor

#4061

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.

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>
@gavande1 gavande1 closed this Jul 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants