fix: harden desktop updater handoff - #37748
Conversation
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Overview
Harden desktop updater handoff by improving robustness of the update process. Well-scoped fix.
✅ Looks Good
- 140 additions, 6 deletions
- Makes the desktop updater handoff more resilient
- Companion to #37789 (remote mode update skip)
- No security concerns
- Clean implementation
Reviewed by Hermes Agent
austinpickett
left a comment
There was a problem hiding this comment.
Request changes - the hardening is valuable but this PR overlaps and conflicts with #38955 on the same applyUpdates gate, and the two encode contradictory routing.
What is good and verified against main: resolveUpdaterBinary() currently does a bare fileExists(candidate) check, so a present-but-non-executable/stale macOS hermes-setup is treated as runnable. Replacing that with isRunnableUpdaterBinary (statSync isFile + X_OK on POSIX, exists on Windows) is a real, independent improvement, as is waitForUpdaterSpawn gating the hand-off on the child actually spawning (with a timeout fallback for detached children that never emit spawn) and the POSIX fallback to applyUpdatesPosixInApp on launch failure. The new updater-handoff.cjs unit tests are solid.
The conflict: this PR keeps the if (!updater && !IS_WINDOWS) routing and only changes the opts plumbing, whereas #38955 rewrites that same gate to if (!IS_WINDOWS) to kill the macOS restart loop. They touch the identical lines and will not merge cleanly. More importantly, #37748’s resolveUpdaterBinary hardening makes a staged-but-broken macOS binary resolve to null - which only changes macOS behavior if the !updater gate still applies on macOS. Under #38955 macOS never consults updater at all, so the two are partially redundant on macOS.
Recommendation: land #38955 first (canonical macOS routing fix), then rebase this PR to drop the routing/opts churn and keep ONLY the Windows-relevant pieces: isRunnableUpdaterBinary (guards a stale hermes-setup.exe on the Windows hand-off path) and waitForUpdaterSpawn + the Windows error surface. After that this is an easy approve.
|
Updated this branch per the requested-changes guidance to avoid overlapping with the canonical macOS routing fix. Changes:
Verification:
Pushed head: |
|
Final refresh pushed after latest |
|
Pushed another update for the requested-change scope cleanup. What changed:
Verification rerun locally:
Head: |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for narrowing this to the Windows handoff case. The failure-handling goal remains relevant, but this branch needs a TypeScript salvage rather than a direct merge.
Problems
apps/desktop/electron/main.ts:1318-1323already definesfileExists()asfs.statSync(...).isFile(), andresolveUpdaterBinary()uses it at:2277-2282. The new CJSisRunnableWindowsUpdaterBinary()therefore does not add validation beyond current behavior.- The proposed spawn check covers only
applyUpdates().handOffWindowsBootstrapRecovery()resolves the same updater atmain.ts:2639, spawns it at:2669-2679, then writes its marker and schedules quit at:2683-2699without observing an asynchronous spawn error. - Current main migrated this surface from CJS to TypeScript in
39d09453f95e8aefc0c97e5d9b30ff341cae9ed8;main.cjsand the proposed CJS helper/test paths are gone.
Suggested changes
- Port a shared Windows handoff check to TypeScript and apply it before marker/quit behavior in both handoff call paths.
- Reuse
fileExists()rather than duplicating its regular-file predicate, and cover emitted spawn errors in the TypeScript platform suite.
Automated hermes-sweeper review.
|
|
||
| try { | ||
| const stat = fsModule.statSync(candidate) | ||
| return stat.isFile() |
There was a problem hiding this comment.
fileExists() already performs this exact statSync(...).isFile() validation in the updater resolver, so this new predicate does not change behavior. On current main that implementation is at apps/desktop/electron/main.ts:1318-1323; please avoid duplicating it when porting the useful spawn-error handling.
fe432a8 to
abc3074
Compare
|
Clean TypeScript salvage pushed on current
Validation:
|
abc3074 to
66d9bf8
Compare
|
Clean TypeScript salvage finalized on current
Validation:
GitHub API reports one commit / three changed files, mergeable and rebaseable. |
Summary
Test Plan
node --check electron/main.cjsnpx eslint electron/updater-handoff.cjs electron/updater-handoff.test.cjsnpm run test:desktop:platformsNote: full
npm run lintstill reports pre-existingno-emptyissues inelectron/main.cjs; this change does not introduce those.