Skip to content

Keep CLI child close handlers alive when killing on quit#4109

Open
gavande1 wants to merge 2 commits into
trunkfrom
fix-cli-quit-listener-cleanup
Open

Keep CLI child close handlers alive when killing on quit#4109
gavande1 wants to merge 2 commits into
trunkfrom
fix-cli-quit-listener-cleanup

Conversation

@gavande1

@gavande1 gavande1 commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Related issues

How AI was used in this PR

Claude wrote the code, guided by the author. The author tested it locally and manually reviewed it before pushing.

Proposed Changes

The quit-time kill paths (appQuitHandler in the desktop and killChild in the shared runner) called child.removeAllListeners() before killing the child, which also removed the close handler. Since will-quit can be prevented and the app can keep running, this left a stale will-quit listener attached and left callers awaiting the command hanging forever. The kill paths now only kill the child and let the close handler run: it detaches the will-quit listener and emits failure, so awaiting callers settle.

Testing Instructions

  • Run npm start, start a site, and quit the app. Child CLI processes should terminate as before, and the console should show a failure warning from the events subscriber instead of silence.
  • npm test -- apps/studio/src/tests/index.test.ts passes.

Pre-merge Checklist

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

@wpmobilebot

wpmobilebot commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

📊 Performance Test Results

Comparing 5f3b4f9 vs trunk

app-size

Metric trunk 5f3b4f9 Diff Change
App Size (Mac) 1411.96 MB 1411.96 MB +0.00 MB ⚪ 0.0%

site-editor

Metric trunk 5f3b4f9 Diff Change
load 730 ms 1088 ms +358 ms 🔴 49.0%

site-startup

Metric trunk 5f3b4f9 Diff Change
siteCreation 6477 ms 6513 ms +36 ms ⚪ 0.0%
siteStartup 2384 ms 2391 ms +7 ms ⚪ 0.0%

Results are median values from multiple test runs.

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

@gavande1 gavande1 requested a review from a team July 7, 2026 13:47

@fredrikekelund fredrikekelund left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The reasoning here makes sense to me 👍 The close event listener should fall through to the else clause for cases when Studio killed the process (exitCode is null in those cases). That's good, because it means that CLI commands that don't explicitly handle SIGTERM signals will end up emitting failure events anyway, which is what we want.

@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.

The reasoning makes sense, thanks for the detailed description. 👍

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.

4 participants