Harden the dirty-editor quit guard#853
Merged
Merged
Conversation
Follow-up to #840. Three concrete failure modes that round-2 review turned up: 1. `webContents.send` is unguarded. If the renderer is destroyed between the reachability check and the send (e.g. a dying GPU process), the throw escapes the `before-quit` handler with `quitGuardChannelBusy = true` already set and no timeout scheduled yet — the app becomes un-quittable until restart. Wrap the send, and tear the listener/timer down on failure. 2. The timeout vs. response race silently commits a quit on `hasDirty=true`. Once `setTimeout` has already enqueued its callback for the next tick, `clearTimeout` is a no-op and the timeout callback runs even after the response arrived — which unconditionally calls `commitQuit()`, overriding the user's "save first" intent. Funnel both paths through a `settle()` helper that only acts the first time it's called. 3. The reply listener accepted any sender. A rogue or future-buggy `webContents` could decide the quit by sending the channel name first. Validate `evt.sender === wc` and ignore non-matches; switch from `.once` to `.on` + explicit `removeListener` so a rogue early reply doesn't consume the listener slot. Also wrap the renderer-side handler in try/catch so an unexpected throw inside `editorTabStore.getTabs()` reports `hasDirty=false` immediately instead of stranding the main process for 5 s on a silent timeout. Verify `webContents.isCrashed()` before sending so a known-dead renderer skips the round-trip and quits instantly instead of waiting on the timeout fallback. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codex round-2-2 review suggested two small follow-ons: 1. Sender check should reject missing/falsy `evt.sender` outright. In real Electron IPC the sender is always populated; a falsy sender is anomalous and treating it as legit defeats the rogue-reply defence we just added. 2. Wrap `bridge.reportDirtyEditorsResult` in try/catch on the renderer side. If the IPC bridge is in a bad state and the call throws, the rest of the listener body is fine but the React useEffect callback would propagate the error — and an uncaught error in the listener would silently disable the quit guard for the rest of the session. Both are pure tightening; no behaviour change on the happy path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
Follow-up to #840 / #851 review pass. Three concrete failure modes that a multi-agent round-2 review turned up in
electron/main.cjs:1268-1343:webContents.sendis unguarded. If the renderer is destroyed between the reachability check and the send (dying GPU process, etc.), the throw escapes thebefore-quithandler withquitGuardChannelBusy = truealready set and no timeout scheduled yet — the app becomes un-quittable until restart.hasDirty=true. OncesetTimeouthas already enqueued its callback for the next tick,clearTimeoutis a no-op and the timeout still runscommitQuit(), overriding the user's "save first" intent.webContentscould decide the quit by sending the channel name first.Fix
settle()helper that only acts the first time it's called (handles race Bug: 图标太大了 #2).try/catch; on failure, tear the listener/timer down andcommitQuit()synchronously (handles Welcome to Netcatty Discussions! #1)..onceto.on+ explicitremoveListenerand validateevt.sender === wc, so a rogue early reply doesn't consume the slot or decide the quit (handles [BUG]右上角点同步会报错，设置里同步正常 #3).wc.isCrashed?.()to the reachability gate so a known-dead renderer skips the round-trip and quits instantly.editorTabStore.getTabs()intry/catchso an unexpected throw reportshasDirty=falseimmediately instead of stranding the main process on the 5 s timeout.Test plan
npm run lintcleannpm test293/293 ✔🤖 Generated with Claude Code