Conversation
before-quit never called event.preventDefault(), so Electron didn't wait for closeVault() to finish, and window-all-closed could trigger a second, parallel closeVault() via app.quit(). Adds a two-flag reentrancy guard: preventDefault() runs synchronously on every pass until the close settles (isQuitting guards against starting a second closeVault() while one is in flight; readyToQuit is set only in the finally, once closeVault() has settled, and is what finally lets a before-quit signal through). This also closes a narrower window a single-flag guard would leave open: a genuine second quit signal (e.g. a duplicate Cmd+Q) arriving while the first closeVault() is still in flight would otherwise slip through unprevented. window-all-closed now only calls app.quit() on Windows/Linux, leaving before-quit as the sole owner of the shutdown sequence; macOS keeps its existing dock-aware closeVault() call — safe even when a real quit is already in progress (e.g. during autoUpdater.quitAndInstall(), which closes windows before before-quit fires) because closeVault() is serialized (withVaultLock) and idempotent, not because no quit could be in progress.
…lt() Proves the readyToQuit guard actually blocks a genuine second quit signal arriving before the first closeVault() settles, not just the internal re-entrant app.quit() from the finally. Confirmed this test fails against the old single-flag guard before restoring the fix.
|
React Doctor found no new issues. 🎉 Reviewed by React Doctor for commit |
2 tasks
GFrancV
changed the title
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
1 participant
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.
Summary
before-quitnever calledevent.preventDefault(), so Electron didn't wait forcloseVault()to finish before the process could die, andwindow-all-closedcould trigger a second, parallelcloseVault()viaapp.quit(). This closes both gaps with a two-flag reentrancy guard (isQuitting/readyToQuit) insrc/main/index.ts:preventDefault()now runs synchronously on everybefore-quitpass until the close has actually settled.window-all-closedonly callsapp.quit()on Windows/Linux, leavingbefore-quitas the sole owner of the shutdown sequence; macOS keeps its dock-awarecloseVault()call (safe even mid-shutdown, sincecloseVault()is serialized viawithVaultLockand idempotent).Why a two-flag guard instead of one
The original single-flag design (
isQuittingonly) closes the bug described in the issue, but leaves a narrower window open: a genuine second quit signal (e.g. a duplicate Cmd+Q, or an OS logoff) arriving while the firstcloseVault()is still in flight would skippreventDefault()entirely and let Electron's default action (terminate) proceed — reintroducing the same class of data-loss bug the issue reports, just requiring two quit signals instead of one. This surfaced during review, not from the original issue text, soreadyToQuitwas added to distinguish "a close is in progress" from "the close has settled and it's safe to let the real quit through."Why there's a test beyond what the issue specifies
The issue only describes the single-signal race. The extra test (
a duplicate before-quit signal while closeVault() is still in flight is still prevented) exists to pin down the two-flag fix above — it reproduces the narrower race by holdingcloseVault()unresolved and firing a secondbefore-quitwhile it's pending, asserting that signal still getspreventDefault()'d. Confirmed this test fails against the original single-flag guard before restoring the fix, so it's a real regression guard, not incidental coverage.Test plan
pnpm check:task(typecheck, lint, format, depcruise,test:vault) passes cleantests/lifecycle.test.tscases pass, including the new duplicate-signal test (confirmed it fails against the pre-fix single-flag guard)vitest run(including the real SQLCipher/Argon2idvault.test.tssuite) passes with no regressions.nvxor orphaned temp DB inos.tmpdir()(not automatable without a running GUI)Closes #18