fix(desktop): retry a transient update-check failure once before surfacing it - #4811
fix(desktop): retry a transient update-check failure once before surfacing it#4811Adarsh-Me wants to merge 2 commits into
Conversation
…acing it The packaged update check intermittently fails with "Cannot parse releases feed": electron-updater resolves "latest" through GitHub's releases Atom feed, and GitHub intermittently answers that request with HTTP 406 and an empty body — the identical request succeeds seconds later (apache#4790). The app surfaced that first failure as an error toast even though the check was never going to stay broken. A check now retries once after a two-second backoff, on the injected clock. While the retry is pending, the 'error' listener holds a check failure back instead of publishing it, so a transient refusal never reaches the renderer as an error state or toast; a retry that also fails publishes exactly as before. Download and install errors are unchanged — they surface immediately. dispose() clears the retry timer alongside the schedule timer. Closes apache#4790
|
This is ready for review. It addresses the user-visible false alarm from #4790: the startup update check intermittently surfaced "Cannot parse releases feed" because electron-updater's releases-feed request can get a transient HTTP 406 from GitHub, and the first failure was surfaced as an error even though the identical request succeeds seconds later. The change is small and self-contained: Marked |
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed current head b7c0bedc70366e03ca2a9f36764c251bf7dedd23 (OPEN, MERGEABLE/BLOCKED awaiting human review). Technical GO — no P0–P2, three P3s below. A well-aimed small fix: the retry state machine was walked through and no path lets an error slip or misreport. Checks green on this head.
P3 — a dispose inside the retry window leaves a promise unsettled forever
The catch branch schedules the retry as new Promise((resolve) => { checkRetryTimer = clock.setTimeout(() => { ... resolve(...); }, ...) }) — resolve only fires in the timer callback, but dispose() does clock.clearTimeout(checkRetryTimer), after which nobody ever resolves it (and the .finally resetting checkAttemptsRemaining/checkInFlight never runs either). Not a P2 only because dispose() is terminal and checkForUpdates() early-returns on disposed, so no later check can hang on the stuck checkInFlight.
P3 — disposed decides "don't retry" but then still publishes the error
if (disposed || checkAttemptsRemaining <= 0) { ...; return status.state === 'error' ? status : publishError('check', error); } — publish has no dispose guard and calls deps.onStatusChange?.(next) directly, pushing state to subscribers of an already-disposed service. Pre-existing pattern (publish never had the guard), but this is the first call site that explicitly checks disposed and then publishes anyway — self-contradictory to read; returning status outright would match the intent.
P3 — "check now" during the retry window silently joins the pending retry
During the 2 s wait checkInFlight is still set, so a manual "check for updates" reuses the retry instead of starting a fresh check. The behavior is right (no concurrent feed hits); just worth knowing the button can appear to lag up to 2 s with no UI hint. No code change needed.
What I could not judge
Desktop not run and the 406 not reproduced — whether 2 s covers the observed recovery window ("succeeds after a few seconds" per the description) and whether retrying exactly once is enough both need live data; the code cannot prove either.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
简体中文
本条结论全部来自 @Opus-Qronos-AstroHan 的审查。我自己没有读这份 diff;我核的是当前 head 有没有漂移。当前 head 是 b7c0bed,未关闭。修得准,技术上无阻断问题,三条小的都是重试状态机的边角。等人类拍板。
…he#4790) Review on apache#4811 (P3s): if the service was disposed inside the two-second retry backoff, dispose() cleared the retry timer and nothing ever resolved the retry promise, so checkInFlight and its .finally cleanup dangled. And the catch branch tested `disposed` but then still published the error to a disposed service's subscribers. The retry timer is no longer cleared in dispose(): its callback checks `disposed`, resolves the in-flight check with the current status instead of retrying or publishing, and lets the .finally reset checkInFlight. The catch branch now returns `status` outright when disposed rather than publishing. A regression test covers the dispose-during-backoff path.
|
Thanks for the careful read — the two actionable P3s are addressed on
A regression test covers the dispose-during-backoff path (the in-flight promise settles, the retry does not fire, and no On the two live-data items: agreed, addressable only by observing real GitHub behavior. My notes — neither |
Summary
Fixes #4790's user-visible failure mode: the startup update check intermittently surfaces "检查更新失败 / Cannot parse releases feed" when GitHub transiently answers electron-updater's releases-feed request with HTTP 406 and an empty body — the identical request succeeds seconds later, so the first failure is noise, not an outage.
AppUpdateService.checkForUpdatesnow retries once after a two-second backoff (on the injected clock). While the retry is pending, theerrorlistener holds a check failure back instead of publishing it, so a transient refusal never reaches the renderer as an error state or toast; a retry that also fails publishes exactly as before. Download and install errors are unchanged — they surface immediately.dispose()clears the retry timer alongside the schedule timer.Two scope notes from #4790:
Maka-Agent/maka-agentpublish config is already fixed onmain(electron-builder.config.mjspublishes toowner: apache, repo: maka); the staleapp-update.ymlonly exists inside installed v0.1.11 packages and is replaced by the next install. This PR therefore changes only the failure handling.allowPrereleasechannel gating, unchanged here.Verification
biome checkon both changed files: clean.tsc --strictpass overapp-update-service.ts: no errors beyond missing-node_modules artifacts (NodeJSnamespace,setImmediate), which the workspace toolchain supplies.app-update-service.test.tscover both paths: a first-attempt 406-style failure (error emitted and thrown, as electron-updater does) that recovers on retry with noerrorstatus ever published, and a persistent failure that surfaces exactly oneerrorstatus after twocheckForUpdatescalls. Full file: 17/17 pass. Run standalone withtsx --testagainst realelectron-updater@6; the workspacetestpipeline will re-run it under CI.AI use
Select exactly one:
Tool(s) and scope:
Checklist
errorstatus and a singlecheckForUpdatescall; the second fails with zeroerrorstatuses published.Does this PR entail a change in behavior?