Skip to content

Test restoring per-command CLI shutdown handling#4070

Open
gavande1 wants to merge 11 commits into
trunkfrom
test-revert-cli-runner-shutdown
Open

Test restoring per-command CLI shutdown handling#4070
gavande1 wants to merge 11 commits into
trunkfrom
test-revert-cli-runner-shutdown

Conversation

@gavande1

@gavande1 gavande1 commented Jul 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

  • Restore the desktop app to spawning CLI commands with per-command quit cleanup instead of a persistent shared CLI runner quit listener. This is intended to validate whether the Jun 29 shutdown refactor is killing the site stop --all cleanup command during Windows E2E teardown, causing leaked site processes and eventual job timeout.
  • Temporarily re-enable Windows x64 E2E in the Buildkite matrix so this PR can exercise the affected job. This CI toggle is diagnostic and should not be merged as-is unless the team wants Windows E2E enabled again.
  • Temporarily cap the diagnostic E2E matrix at 30 minutes so a hung Windows run fails fast instead of waiting for the Buildkite agent's 3-hour timeout.

Testing Instructions

  • Let the Windows x64 E2E CI job run and compare whether site stop --all still times out on every app shutdown.
  • Specifically check whether the e2e-tests-on-windows-x64 job completes under 30 minutes, or fails at the diagnostic timeout instead of running until the 3-hour Buildkite timeout.

Local checks:

  • npx eslint --fix apps/studio/src/modules/cli/lib/execute-command.ts
  • ruby -e 'require "yaml"; YAML.load_file(".buildkite/pipeline.yml"); puts "ok"'
  • git diff --check
  • npm test -- apps/studio/src/tests/index.test.ts apps/studio/src/tests/execute-wp-cli.test.ts apps/studio/src/modules/cli/lib/tests/cli-site-creator.test.ts
  • npm run typecheck currently fails on existing workspace resolution/type issues unrelated to this change: duplicate ignore package types and @studio/common path resolution errors.

Pre-merge Checklist

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

gavande1 and others added 3 commits July 2, 2026 12:25
@gavande1 @claude
Fix Windows E2E hang: bound daemon requests and reap leaked daemon be…
e60bff5 
…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>
@gcsecsey @claude
Resolve quit-time stop as soon as the CLI reports success
c3bc2b7 
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
@gavande1
Restore per-command CLI shutdown handling
8eb6411
@gavande1 gavande1 requested a review from a team as a code owner July 3, 2026 07:28
@wpmobilebot

wpmobilebot commented Jul 3, 2026

Copy link
Copy Markdown
Collaborator

📊 Performance Test Results

Comparing e28d40a vs trunk

app-size

Metric trunk e28d40a Diff Change
App Size (Mac) 1317.24 MB 1317.24 MB 0.00 MB ⚪ 0.0%

site-editor

Metric trunk e28d40a Diff Change
load 1099 ms 1117 ms +18 ms ⚪ 0.0%

site-startup

Metric trunk e28d40a Diff Change
siteCreation 6490 ms 6524 ms +34 ms ⚪ 0.0%
siteStartup 1864 ms 1854 ms 10 ms ⚪ 0.0%

Results are median values from multiple test runs.

Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change (<50ms diff)

@gavande1 gavande1 changed the base branch from trunk to fix-windows-e2e-daemon-leak July 3, 2026 09:05
@gavande1 gavande1 changed the base branch from fix-windows-e2e-daemon-leak to trunk July 3, 2026 11:43
@gavande1 gavande1 mentioned this pull request Jul 3, 2026
Open
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@gavande1 gavande1

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@gavande1 @wpmobilebot @gcsecsey