More safety checks around IPC communication - #849
Merged
Conversation
wojtekn
approved these changes
Jan 28, 2025
wojtekn
left a comment
wojtekn
left a comment
Contributor
There was a problem hiding this comment.
The code change looks clear, and it's more readable now.
I smoke tested different Studio features and haven't noticed any issues.
bgrgicak
pushed a commit
that referenced
this pull request
Jan 29, 2025
* WIP * withMainWindow -> getMainWindow * Refinements
3 tasks
fredrikekelund
pushed a commit
that referenced
this pull request
May 29, 2026
## Related issues - Fixes [STU-1750](https://linear.app/a8c/issue/STU-1750/studio-reopens-multiple-times-when-quitting-during-site-startup) ## How AI was used in this PR Claude Code investigated the bug, identified the root cause (create-if-missing branch in `getMainWindow()` racing with async IPC fan-out during the `will-quit` shutdown window), proposed the minimal gate at `sendIpcEventToRenderer`, and applied the change. I reviewed the diff and confirmed both the fix and its placement before committing. ## Proposed Changes - Add an `isAppQuitting` flag in `apps/studio/src/ipc-utils.ts` with an exported `markAppQuitting()` setter. While set, `sendIpcEventToRenderer` returns early without calling `getMainWindow()`. - Call `markAppQuitting()` at the start of the `will-quit` handler in `apps/studio/src/index.ts`, before any other cleanup. ### Why this works When the user confirms "Stop sites" on quit, `will-quit` calls `event.preventDefault()` and waits up to 6 s for `stopAllServers()` to drain. During that window, in-flight async callbacks (site-startup events, snapshot events, sync progress, etc.) keep firing through `sendIpcEventToRenderer()`. Each call routes through `getMainWindow()` — and because the original window has typically already been closed by Electron's shutdown sequence, `getMainWindow()` falls through to `createMainWindow()` and spawns a fresh `BrowserWindow`. One pending event = one ghost window. With ~6 sites starting simultaneously on a Linux VM, you reliably see ~6 windows reopen. Blocking the fan-out at the `sendIpcEventToRenderer` chokepoint is the smallest fix: it catches every async-IPC path without changing `getMainWindow`'s signature or touching its 15+ other call sites (menus, deeplinks, update dialog, etc.), which are user-triggered and can't fire post-`will-quit` anyway. `will-quit` (not `before-quit`) is the right gate point: `before-quit` can still be cancelled by the sync-in-progress dialog or the "Cancel" button on the running-sites dialog. Once `will-quit` fires, the app is committed. ### Regression origin The mechanic was introduced in #86 (May 2024) for OAuth deep-link handling. It became today's user-visible bug in #849 (Jan 2025), which routed `sendIpcEventToRenderer` through `getMainWindow()`. The bug got more reproducible recently due to Linux VM testing (slower shutdown widens the race window) plus PR #3350 removing an accidental escape hatch that bypassed the quit dialog when the CLI wasn't installed. ## Testing Instructions Reproduce on `trunk` first, then verify the fix on this branch. **Repro on trunk (Linux/Windows easier; macOS rarer):** 1. Have several Studio sites created (~6 makes it obvious). 2. Quit Studio so all sites stop. 3. Re-launch Studio. 4. Start all sites at once. 5. **Before** they finish booting, quit Studio. 6. Observe: Studio reopens 4–6 times in quick succession. **Verify on this branch:** 1. Repeat steps 1–5 above. 2. Studio should quit cleanly with no ghost windows. ## Pre-merge Checklist - [x] Have you checked for TypeScript, React or other console errors? - [x] Manual repro on Linux (with several sites) - [x] Manual repro on Windows (with several sites)
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
2 participants
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
With the help of some AI debugging in Sentry, I determined that https://github.com/Automattic/dotcom-forge/issues/10346 seems to be happening because we send IPC messages to windows that have been destroyed. This PR adds some additional
isDestroyedchecks to mitigate that (see 0ae3042).I also took the opportunity to refactor
withMainWindowinto a function that returns a promise instead. There's really no reason for it to accept a callback, so this helps keep our code clean and simple.Testing Instructions
Smoke test the app. Ensure that the About menu, the settings, and the fullscreen view work as expected.
Pre-merge Checklist