Test restoring per-command CLI shutdown handling#4070
Conversation
…tween sessions Every session's quit-time 'site stop --all' hangs past its 20s timeout on Windows CI, leaving that session's site servers running in the machine-global process-manager daemon. Playground sites weigh 6 capacity units, so six leaked sites exhaust the 36-unit cap; every later createSite then fails by timeout, stretching the suite past the 180-minute job limit, and leaked php.exe processes block session cleanup and runner exit. - SocketRequestClient now times out waiting for a response, so a wedged daemon can no longer hang CLI commands forever - The daemon's stopProcess settles even if a child never reports exit, so kill-daemon always completes and capacity is freed - E2E cleanup reaps any surviving daemon tree between sessions and no longer aborts when the app failed to launch - The quit-time stop logs CLI progress events for future diagnosis - Re-enable Windows E2E in CI to verify (AINFRA-2588) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
On Windows the `site stop --all` CLI stops the sites and reports success within a few hundred ms, but its process can linger without self-exiting — so stopAllServers waited out the full quit timeout (20s in E2E) and force-killed it on every session, adding ~20s per session to the suite. Act on the CLI's reported completion event and reap the process then, instead of waiting for it to exit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vfk7xXMfsABh5JMYX51wiS
📊 Performance Test ResultsComparing 6e02271 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) |
…o test-revert-cli-runner-shutdown # Conflicts: # .buildkite/pipeline.yml
fredrikekelund
left a comment
fredrikekelund
left a comment
There was a problem hiding this comment.
Reverting the changes to apps/studio/src/modules/cli/lib/execute-command.ts was the right call, IMO 👍
Moreover, I suspect #3837 was at fault for the "maximum number of sites" error we saw. I've applied a narrower fix to that problem. Fingers crossed it does what it should.
I called out a couple of changes in this PR that I think we should revert before landing. Ultimately, I believe fixing the regressions from #3954 and #3837 should be the focus of this PR. Any further tweaks in how we handle site shutdown should be deferred.
I would approve this PR straight away to unblock you, @gavande1, but the one thing that sticks out to me is the response timeout in SocketRequestClient. I think that can easily break things, and I strongly suggest reverting it.
There was a problem hiding this comment.
I'm skeptical about this approach… I don't think the issue here is really that we aren't receiving an exit event – it's that the process doesn't die. The fact that the process doesn't die can cause issues down the line (for example, interfering with the auto-updater because the running process still has a handle on node.exe). Taping over this fact is not helpful.
If this is not explicitly needed for this PR, I would just defer this for another PR/discussion.
There was a problem hiding this comment.
This change could easily interfere with wp-cli-command IPC messages. I suggest reverting it.
Unless we know this change is truly needed to fix the E2E issues on Windows, I would defer this to another PR/discussion and apply the change more narrowly.
| /** | ||
| * The CLI process-manager daemon is machine-global (fixed named pipe on Windows, ~/.studio/daemon | ||
| * socket elsewhere) and spawned detached, so it outlives the app. When `site stop --all` fails on | ||
| * quit, the daemon keeps that session's WordPress servers running; they accumulate across test | ||
| * sessions until the daemon's site capacity is exhausted (36/36) and every subsequent createSite | ||
| * fails. Leaked php.exe processes also hold DLL locks that break session dir cleanup on Windows. | ||
| * Reap the daemon tree after each session so the next one starts from a clean slate. | ||
| * | ||
| * Note: this also kills the daemon of any locally running Studio app — acceptable for e2e runs, | ||
| * which already share the global daemon with it. | ||
| */ | ||
| function killLeakedDaemonProcesses() { | ||
| try { | ||
| if ( process.platform === 'win32' ) { | ||
| // `-ne $PID` excludes this PowerShell process itself, whose own command line | ||
| // also contains the match string. | ||
| execSync( | ||
| 'powershell -NoProfile -Command "Get-CimInstance Win32_Process | ' + | ||
| "Where-Object { $_.CommandLine -match 'process-manager-daemon' -and $_.ProcessId -ne $PID } | " + | ||
| 'ForEach-Object { taskkill /F /T /PID $_.ProcessId }"', | ||
| { stdio: 'ignore' } | ||
| ); | ||
| } else { | ||
| // SIGTERM lets the daemon run its graceful shutdown, which stops its site children. | ||
| execSync( 'pkill -f process-manager-daemon', { stdio: 'ignore' } ); | ||
| } | ||
| } catch { | ||
| // No daemon running (pkill exits 1 when nothing matches) — nothing to reap. | ||
| } | ||
| } |
There was a problem hiding this comment.
The normal shutdown routine in Studio is supposed to kill all processes. To the best of my knowledge, this was true in Studio until very recently (I know I was battling this exact problem way back in #2299).
I think we should look at #3837 as another potential culprit here. Looking at that changeset now, it appears we did not properly consider E2E tests.
I'll make a small change there (and revert this change) to see if it has the desired effect.
There was a problem hiding this comment.
Good call reverting the changes to this file rather than trying to salvage tools/common/lib/cli-process.ts. There's a lot of nuance here. Reverting is the simpler and safer approach 👍
There was a problem hiding this comment.
The changes to this file look related to what I was saying in E2ESession about #3837 having introduced a bug in how sites are stopped when Studio quits in E2E tests. I suggest reverting them.
If there is clear evidence that something is failing with the studio site stop --all command, then we should look into the underlying causes. Killing the CLI processes that runs that command won't do us much good. As we've seen elsewhere in this thread: lingering site processes on Windows keep open handles on certain files (node.exe or certain DLLs), so we really do need the stop command to work, rather than taping over cases when it doesn't.
I take that back. It looks like that PR didn't change the logic for how sites are killed during E2E tests. However, there still appears to be a flaw in the logic there, so maybe it was more of a latent bug… |
| const dir = fs.mkdtempSync( path.join( os.tmpdir(), 'studio-siteurl-prepend-' ) ); | ||
| const tmpRoot = | ||
| process.platform === 'win32' ? fs.realpathSync.native( os.tmpdir() ) : os.tmpdir(); | ||
| const dir = fs.mkdtempSync( path.join( tmpRoot, 'studio-siteurl-prepend-' ) ); |
There was a problem hiding this comment.
This change is meant to address the regression from #3988
We've had issues elsewhere with shortened tmp directory paths on Windows. fs.realpathSync.native() unfurls the path.
|
The Windows E2E tests are legitimately green 👍 I've pushed one additional concrete fix (#4070 (comment)), along with the reverts I suggested. I suggest landing this PR once Buildkite reports green on the most recent commit, and closing #4075 for now. We can keep pursuing #4061, but I would hold off with merging it |
…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>
4 participants
Related issues
How AI was used in this PR
AI helped compare Windows E2E logs, identify the shutdown-ordering hypothesis, and prepare this small diagnostic branch. The code change was reviewed locally before opening the PR.
Proposed Changes
Testing Instructions
Local checks:
Pre-merge Checklist