fix(desktop): guard soft gateway-switch against concurrent invocation - #62624
fix(desktop): guard soft gateway-switch against concurrent invocation#62624pierrenode wants to merge 1 commit into
Conversation
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Looks good! No obvious issues found.
Reviewed by Hermes Agent
Code Review Summary\n\nLooks good! No obvious issues found.\n\n---\nReviewed by Hermes Agent |
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Looks good! No obvious issues found (reviewed in read-only mode).
Reviewed by Hermes Agent
Code Review Summary\n\nLooks good! No obvious issues found (reviewed in read-only mode).\n\n---\nReviewed by Hermes Agent |
|
Thanks for the focused concurrency fix. I found no substantive correctness issue. Current Likewise, current Automated hermes-sweeper review. |
connection-config:apply's global/primary branch (apps/desktop/electron/ connection-apply.ts::applyConnectionChange) tears down the window backend and notifies the renderer via hermes:connection:applied, which drives softSwitch() in useGatewayBoot. Two independent UI triggers (Settings "Apply" and the cloud-agent "Connect" button) each have their own pending-state guard but don't know about each other, so both can call applyConnectionChange back-to-back for the primary scope. Without protection: a second teardownPrimary() races the first's still-pending process exit, fires a second hermes:connection:applied, and the resulting second softSwitch() races the first through getConnection()/adoptPrimaryProfile() with no guard against it. Fix: applyConnectionChange dedupes concurrent primary-scope calls through a single in-flight promise, so a second caller awaits the first's re-home instead of racing its own. softSwitch() gets an early $gatewaySwitching reentrancy check as defense in depth — the flag is already set for the whole switch, just never checked at entry. No behavior change for the common single-call path.
5d9b1d9 to
927b4cf
Compare
|
Substantially rewritten against current `upstream/main`, not a mechanical rebase — the original diff couldn't apply cleanly because `connection-config:apply`'s inline handler in `main.ts` (what this PR originally targeted) has since been extracted into a standalone, independently-tested module (`apps/desktop/electron/connection-apply.ts`, `applyConnectionChange()`). The underlying race is unchanged and still live: the extracted function's plain teardown-primary path (`isPrimary && !rehomePrimary`) still has zero protection against two concurrent primary-scope calls. What changed vs. the original PR:
Test coverage, now easier than the original (which had no test seam for `main.ts`): `connection-apply.test.ts` already existed with a clean-room testing convention (pure function, injected callbacks, no Electron mocking) — added a dedup regression test there. `use-gateway-boot.test.tsx` already has real `onConnectionApplied` fake wiring from an unrelated later commit; added a back-to-back-firing regression test using the existing `beforeConnectionSwitch` spy as the signal (guarded: called once; unguarded: called twice). Both fixes mutation-verified independently (reverting either one reproduces the pre-fix call count in its respective new test). `tsc --noEmit` clean on both `tsconfig.json` and `tsconfig.electron.json`. ESLint couldn't run in this workspace-scoped checkout (missing root-only `globals` peer dependency, pre-existing environment gap, unrelated to this change) — relying on the clean typecheck + passing tests instead. Fresh competitor search found no PR touching `connection-apply.ts` or this reentrancy path. |
What does this PR do?
ipcMain.handle('hermes:connection-config:apply', ...)'s global/primary branch (apps/desktop/electron/main.ts) tears down the window backend viateardownPrimaryBackendAndWait({ soft: true })and notifies the renderer viasendConnectionApplied(), which drivessoftSwitch()inuseGatewayBoot(apps/desktop/src/app/gateway/hooks/use-gateway-boot.ts).Two independent UI triggers can call
window.hermesDesktop.applyConnectionConfig(...)close together — the Settings "Apply" button (guarded by its ownsavingstate) and the cloud-agent "Connect" button (guarded by its owncloudConnectingIdstate). Neither guard knows about the other, so both can fire back-to-back for the same global/primary scope.Without protection, tracing the race:
teardownPrimaryBackendAndWait({ soft: true })starts while the first is still awaiting the real child-process exit (up to 5s). Both toggle the sharedsoftRehomeInProgressflag; the second'sfinallycan flip it back tofalsebefore the first's real process actually exits, so that exit'ssendBackendExit()is no longer suppressed — an intentional teardown surfaces as a spurious "backend stopped" error toast.sendConnectionApplied(), so the renderer'shermes:connection:appliedhandler fires twice.softSwitch()had no guard against running twice concurrently — each invocation independently wipes the session lists and re-dials, racing each other throughgetConnection()/adoptPrimaryProfile()/completeDesktopBoot().previewGatewaySwitch()—softSwitch()'s own dev-preview twin inapps/desktop/src/store/gateway-switch.ts, added in the same feature (feat(desktop): soft gateway switch + gateway-settings polish #61916) — already has exactly this reentrancy guard (if ($gatewaySwitching.get()) return). The production path was missing its sibling's protection.Related Issue
No existing issue — found while auditing the soft gateway-switch feature (#61916/#61912) for sibling gaps after a
#of N sitesreview pass.Type of Change
Changes Made
apps/desktop/electron/main.ts: dedupe concurrentconnection-config:applycalls for the global/primary scope through a singleprimaryRehomeInFlightpromise — a second caller awaits the first's in-flight re-home instead of starting its ownteardownPrimaryBackendAndWait()+sendConnectionApplied().apps/desktop/src/app/gateway/hooks/use-gateway-boot.ts:softSwitch()gets the same$gatewaySwitchingreentrancy check its dev-preview twinpreviewGatewaySwitch()already has, as defense in depth against any other path that could firehermes:connection:appliedtwice.apps/desktop/src/app/gateway/hooks/use-gateway-boot.test.tsx(new test): simulates main firinghermes:connection:appliedtwice back-to-back and assertssoftSwitch()'s body (desktop.getConnection()) only runs once. (gateway.connect()has its own'connecting'-state dedup, so a duplicate literal WebSocket was never the observable symptom — a duplicategetConnection()/session-wipe pass was.)How to Test
Mutation-verified: temporarily reverting just the
softSwitch()guard makes the new test fail (expected 2 to be 1) while the other 4 tests in the file still pass; restoring the guard passes all 5. Also ran the neighboringgateway-switch.test.tsandgateway-connecting-overlay.test.tsxsuites (7 + 4 tests) — all green.tsc --noEmit(both renderer and electron configs) andeslinton both changed source files are clean (0 errors; pre-existing unrelated style warnings elsewhere inmain.tsare untouched).The
main.tshalf of the fix (primaryRehomeInFlight) has no automated regression test —main.tsis a non-modular Electron entry script with zero exports and no existing test seam for its internal IPC-handler logic anywhere in this codebase (everyelectron/*.test.tsfile tests a separate, exported helper module instead). Verified by tracing the promise-dedup logic by hand instead.Checklist
main.tscode — different bug, generation-tagged stale-process ownership — but doesn't touch theconnection-config:applyhandler or add apply-call dedup)