fix(desktop): bump electron-updater and recover from corrupt update cache#3495
Conversation
Bumps electron-updater from 6.7.3 to 6.8.3 and wires a defensive cache clear into the error handler. Addresses reports of users stuck on an old version until they manually reinstall: electron-updater's internal cache only self-invalidates on remote sha512 mismatch, so a silently corrupt cached download (failed install, signature error, Squirrel ShipIt failure) would be retried indefinitely. Non-network errors now call DownloadedUpdateHelper.clear() so the next 4-hour check re-downloads from scratch.
📝 WalkthroughWalkthroughThe pull request updates the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR fixes the "stuck on old version" auto-update regression in the desktop app by bumping
Confidence Score: 4/5Safe to merge — the cache-clearing logic is correctly placed, properly guarded, and the internal API access is confirmed valid against electron-updater source. The fix is well-reasoned and minimally invasive: a null-checked, try/caught fire-and-forget call on a confirmed internal property. The only concern is the inaccurate test plan step (simulateError cannot validate the new code path), which is documentation rather than a code defect. No files require special attention; the test plan in the PR description should be corrected to describe a valid validation path for the cache-clearing behavior.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/main/lib/auto-updater.ts | Adds clearCachedUpdate helper that casts to internal DownloadedUpdateHelper (confirmed valid, protected property in electron-updater) and calls it on non-network errors; the simulateError dev helper cannot actually exercise this new path. |
| apps/desktop/package.json | Bumps electron-updater from 6.7.3 to ^6.8.3; all other deps unchanged. |
Sequence Diagram
sequenceDiagram
participant Timer as 4-hr Timer
participant AU as autoUpdater
participant DUH as DownloadedUpdateHelper
participant Cache as Pending Cache Dir
Timer->>AU: checkForUpdates()
AU->>AU: emit "checking-for-update"
AU->>AU: emit "update-available"
AU->>AU: emit "download-progress"
AU->>DUH: save download + metadata (sha512)
AU->>AU: emit "update-downloaded"
Note over AU,Cache: Install phase (on quit or manual)
AU->>AU: quitAndInstall()
alt Install succeeds
AU-->>AU: (new version running)
else Install fails (signing/hash/Squirrel error)
AU->>AU: emit "error"
AU->>DUH: clearCachedUpdate() [NEW]
DUH->>Cache: emptyDir(cacheDirForPendingUpdate) [NEW]
Note over Cache: Cache cleared — next cycle re-downloads from scratch [NEW]
AU->>AU: emitStatus(ERROR)
end
Comments Outside Diff (1)
-
apps/desktop/src/main/lib/auto-updater.ts, line 216-224 (link)simulateErrorwon't validate the cache-clearing pathThe test plan says: "in a packaged build, trigger
simulateErrorand confirm[auto-updater] Cleared cached updateappears in logs." This step cannot work as described for two independent reasons:simulateErrorguards withif (env.NODE_ENV !== "development") return;— it is a no-op in every packaged/production build.- Even in dev mode,
simulateErroronly callsemitStatus(...). It does not emit anerrorevent onautoUpdater, so theautoUpdater.on("error", ...)listener (whereclearCachedUpdateis invoked) is never triggered.
Additionally,
setupAutoUpdateritself returns early in dev mode, so the"error"event listener is never registered there either.To actually validate the
clearCachedUpdatepath you would need to either (a) emit a syntheticerrorevent on theautoUpdaterinstance in a dev hook, or (b) test in a real packaged build where a corrupt cache triggers the native error event. The test plan step should be updated to reflect a realistic validation approach.Prompt To Fix With AI
This is a comment left during a code review. Path: apps/desktop/src/main/lib/auto-updater.ts Line: 216-224 Comment: **`simulateError` won't validate the cache-clearing path** The test plan says: *"in a packaged build, trigger `simulateError` and confirm `[auto-updater] Cleared cached update` appears in logs."* This step cannot work as described for two independent reasons: 1. `simulateError` guards with `if (env.NODE_ENV !== "development") return;` — it is a no-op in every packaged/production build. 2. Even in dev mode, `simulateError` only calls `emitStatus(...)`. It does **not** emit an `error` event on `autoUpdater`, so the `autoUpdater.on("error", ...)` listener (where `clearCachedUpdate` is invoked) is never triggered. Additionally, `setupAutoUpdater` itself returns early in dev mode, so the `"error"` event listener is never registered there either. To actually validate the `clearCachedUpdate` path you would need to either (a) emit a synthetic `error` event on the `autoUpdater` instance in a dev hook, or (b) test in a real packaged build where a corrupt cache triggers the native error event. The test plan step should be updated to reflect a realistic validation approach. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: apps/desktop/src/main/lib/auto-updater.ts
Line: 216-224
Comment:
**`simulateError` won't validate the cache-clearing path**
The test plan says: *"in a packaged build, trigger `simulateError` and confirm `[auto-updater] Cleared cached update` appears in logs."* This step cannot work as described for two independent reasons:
1. `simulateError` guards with `if (env.NODE_ENV !== "development") return;` — it is a no-op in every packaged/production build.
2. Even in dev mode, `simulateError` only calls `emitStatus(...)`. It does **not** emit an `error` event on `autoUpdater`, so the `autoUpdater.on("error", ...)` listener (where `clearCachedUpdate` is invoked) is never triggered.
Additionally, `setupAutoUpdater` itself returns early in dev mode, so the `"error"` event listener is never registered there either.
To actually validate the `clearCachedUpdate` path you would need to either (a) emit a synthetic `error` event on the `autoUpdater` instance in a dev hook, or (b) test in a real packaged build where a corrupt cache triggers the native error event. The test plan step should be updated to reflect a realistic validation approach.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "chore(desktop): tighten auto-updater cac..." | Re-trigger Greptile
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/main/lib/auto-updater.ts (1)
18-22: Add a warning whendownloadedUpdateHelperis unavailable for better observability.
downloadedUpdateHelperis a protected (not public) property in electron-updater v6.8.3 and remains protected across 6.x versions. Accessing it via unsafe casting is necessary but fragile—if future versions alter the property, the silent return hides the failure. Adding a console warning when the helper is undefined will make cache-clear regressions diagnosable.Proposed change
async function clearCachedUpdate(reason: string): Promise<void> { const helper = (autoUpdater as unknown as AppUpdaterInternals) .downloadedUpdateHelper; - if (!helper) return; + if (!helper) { + console.warn( + "[auto-updater] downloadedUpdateHelper unavailable; skipping cache clear", + ); + return; + } try { await helper.clear(); console.info(`[auto-updater] Cleared cached update (${reason})`); } catch (error) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/auto-updater.ts` around lines 18 - 22, clearCachedUpdate silently returns when the protected downloadedUpdateHelper (accessed via (autoUpdater as unknown as AppUpdaterInternals).downloadedUpdateHelper) is missing; update clearCachedUpdate to log a warning before returning so missing helper cases are observable. Specifically, inside clearCachedUpdate check the helper variable and if undefined call console.warn (or the existing logger) with a clear message referencing downloadedUpdateHelper and the provided reason, then return; keep the existing try/catch behavior and error handling for the rest of the function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/desktop/src/main/lib/auto-updater.ts`:
- Around line 18-22: clearCachedUpdate silently returns when the protected
downloadedUpdateHelper (accessed via (autoUpdater as unknown as
AppUpdaterInternals).downloadedUpdateHelper) is missing; update
clearCachedUpdate to log a warning before returning so missing helper cases are
observable. Specifically, inside clearCachedUpdate check the helper variable and
if undefined call console.warn (or the existing logger) with a clear message
referencing downloadedUpdateHelper and the provided reason, then return; keep
the existing try/catch behavior and error handling for the rest of the function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 34b23600-11db-4fca-ba41-dad56a78d464
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
apps/desktop/package.jsonapps/desktop/src/main/lib/auto-updater.ts
1 participant
Summary
electron-updaterfrom6.7.3→6.8.3(picks up 5 releases of staged-download and validation fixes).DownloadedUpdateHelper.clear()call on non-networkerrorevents so the next update check re-downloads from scratch.Why
Users report auto-update sometimes never "comes through" — the app restarts on the same version and they have to manually reinstall to get updates working again.
electron-updater's internal cache at
~/Library/Caches/sh.superset.app-updater/pending/only self-invalidates when the remotesha512differs from the cached metadata. If a cached download is corrupt but its storedsha512still matches the server's, the updater retries the same broken file on every cycle. Install-phase failures (code-signing, Squirrel ShipIt errors, hash mismatch on read) leave exactly this state.Clearing the cache on error forces a fresh download on the next 4-hour cycle, so affected users recover without a manual reinstall.
This does not fix the initial install failure (almost always macOS notarization/signing). It only prevents the wedged-retry loop after one.
Test plan
bun run typecheckinapps/desktop) ✅simulateErrorand confirm[auto-updater] Cleared cached updateappears in logsSummary by cubic
Prevents desktop auto-update from getting stuck by clearing a corrupt cached update when the updater errors, and upgrades
electron-updaterto 6.8.3 for download/validation fixes.Bug Fixes
Dependencies
electron-updater6.7.3 → 6.8.3.Written for commit 76c2c2c. Summary will update on new commits.
Summary by CodeRabbit