fix(desktop): hide PowerShell windows during Windows update and fix update race condition - #61899
fix(desktop): hide PowerShell windows during Windows update and fix update race condition#61899aEgoist wants to merge 1 commit into
Conversation
…ace condition Two issues were fixed: 1. **Hidden PowerShell windows**: Changed windowsHide from alse to rue when spawning hermes-setup.exe in both �pplyUpdates and handOffWindowsBootstrapRecovery functions. This prevents multiple visible PowerShell windows from appearing during the update process. 2. **Race condition fix**: Changed the inally block to catch block in �pplyUpdates so that updateInFlight is only reset on error, not on successful handoff. The previous implementation reset the flag immediately when the function returned, but the app waits 2.5 seconds before quitting. This window allowed multiple update triggers. Also added updateInFlight guard to handOffWindowsBootstrapRecovery to prevent concurrent updates.
giggling-ginger
left a comment
There was a problem hiding this comment.
Review
This PR correctly targets #61898 (Windows update PowerShell windows + updateInFlight race during the 2.5s handoff dwell). The direction is right, but it is not ready to merge as written: it introduces a soft-failure lockout and breaks an existing desktop electron test.
What looks good
- Success-path race: replacing
finallywithcatchsoupdateInFlightstays true throughUPDATE_HANDOFF_DWELL_MSbeforeapp.quit()is the right fix for stacked updater spawns. - Explicit
updateInFlight = falseon the POSIX in-app and manual-command early returns (those paths do not quit). windowsHide: trueon bothhermes-setuphandoff sites is consistent with the Tauri updater'sCREATE_NO_WINDOWapproach for console children.- Guard on
handOffWindowsBootstrapRecoveryagainst concurrent handoffs.
Blocking issues
1. Soft-failure path never clears updateInFlight (regression)
!lock.unlocked returns { ok: false, ... } without throwing. Under the old finally, the flag was always cleared. Under the new catch, it is not.
After one aborted update (another process holds the venv), every later applyUpdates call hits:
if (updateInFlight) {
throw new Error('An update is already in progress.')
}…for the rest of the process lifetime, even though the handoff never happened and the app is still running.
Please reset updateInFlight = false on every non-handoff return inside the try (at least the lock-abort path). Same class of issue on handOffWindowsBootstrapRecovery: if spawn / releaseBackendLockForUpdate throws after updateInFlight = true, there is no catch/finally to clear the flag.
2. Existing test still requires windowsHide: false — and now fails
PR only touches main.ts and does not update:
apps/desktop/electron/windows-child-process.test.ts
test('intentional or interactive desktop child processes stay documented', () => {
assert.match(source, /windowsHide: false/)
...
})After this change, main.ts has zero windowsHide: false sites, so that assertion fails.
Tests run locally
| Suite | Result |
|---|---|
electron/windows-child-process.test.ts (via tsx --test) |
FAIL — 4 pass / 1 fail |
| Failing test | intentional or interactive desktop child processes stay documented — assert.match(source, /windowsHide: false/) |
src/store/updates.test.ts (vitest) |
PASS (25/25) — renderer/store only; does not exercise main-process handoff |
| GitHub checks on this PR | none reported at review time |
No Windows E2E was run here (Linux host). The race + windowsHide behavior still needs a packaged Windows smoke if possible.
Suggested fix before merge
- On lock-abort (and any other “stay alive” return), set
updateInFlight = falsebefore returning. - Wrap recovery handoff in try/catch (or try/finally) so exceptions clear the flag.
- Update
windows-child-process.test.tsto assert the two handoff sites usewindowsHide: true(or drop the blanketwindowsHide: falsegrep if those were the only intentional sites). - Re-run
npm run test:desktop:platforms(or at leasttsx --test electron/windows-child-process.test.ts).
Until (1) and (3) land, this does not fully close #61898 without risk of bricking in-session retries and red CI on the desktop platform suite.
teknium1
left a comment
There was a problem hiding this comment.
Thanks for targeting both updater-console visibility and the success-path handoff race. The premise is still present on current main: applyUpdates() and recovery both use windowsHide: false (apps/desktop/electron/main.ts:2599, 2677), and applyUpdates() clears its guard in finally before the 2.5-second dwell (2623-2629).
Problems
- Replacing
finallywithcatchleaves the normal lock-abort return (apps/desktop/electron/main.ts:2572-2585) withupdateInFlight === true. Because that path returns rather than throws, retries then fail at the guard inapplyUpdates(). apps/desktop/electron/windows-child-process.test.ts:105requireswindowsHide: false. This PR changes the only two such occurrences, so the existing test no longer describes the intended behavior and will fail.
Suggested changes
- Clear the guard on every path that keeps the application alive, including the
!lock.unlockedreturn; retain it only once a quit handoff is scheduled. - Update the child-process test to assert
windowsHide: trueat both updater handoff sites, and add coverage for a lock-abort followed by a retry. - Give the new recovery guard equivalent failure cleanup.
Automated hermes-sweeper review.
| @@ -2620,8 +2623,9 @@ async function applyUpdates(opts = {}) { | |||
| }, UPDATE_HANDOFF_DWELL_MS) | |||
There was a problem hiding this comment.
!lock.unlocked returns { ok: false, ... } instead of throwing, so this catch does not clear the guard on that path. Clear updateInFlight before every non-handoff return (including the lock-abort return), otherwise all later update attempts in this still-running process fail as already in progress.
Summary
Fixes two related issues in the Windows update flow (
apps/desktop/electron/main.ts) that cause multiple PowerShell windows to appear on screen during an update. See #61898.Problems
Visible PowerShell windows: When spawning
hermes-setup.exeinapplyUpdatesandhandOffWindowsBootstrapRecovery,windowsHidewas set tofalse, so PowerShell windows became visible.Race condition:
applyUpdatesused afinallyblock that resetupdateInFlighttofalseimmediately on return. But the app waitsUPDATE_HANDOFF_DWELL_MS(2.5s) before quitting. In that window a re-triggered update sawupdateInFlight === falseand spawned another updater process producing many stacked PowerShell windows.Changes
windowsHide: truein both spawn sites.finallyblock with acatchblock soupdateInFlightis only reset on error, keeping the guard held through the 2.5s quit dwell on the success path.updateInFlightexplicitly on the POSIX in-app and manual-command early-return paths (which do not quit the app).updateInFlightguard tohandOffWindowsBootstrapRecoveryto prevent concurrent recovery hand-offs.Notes
The Rust updater (
apps/bootstrap-installer/src-tauri/src/update.rs) already usesCREATE_NO_WINDOW(0x08000000); this change bringsmain.tsin line with that behavior.