Complete the launcher update task only when the download lands - #208
Conversation
… lands (#200) The main process rounds the download percentage before it reaches the renderer, so a tick at 99.6 percent arrived as 100 and the task listener treated that as the download finishing. Bytes were still moving at that point, and on Windows the last stretch overlaps the installer signature check, so the task list could show Completed while the update was not yet on disk. The progress listener now only ever moves the bar. update-downloaded, which fires when electron-updater has the verified file, is the only route to the completed state.
18b99dd to
ff14943
Compare
Pixnop
changed the title
Zaldaryon
left a comment
Zaldaryon
left a comment
There was a problem hiding this comment.
Checked out ff14943 in an isolated worktree: typecheck, lint:ci, format:check all clean, and the targeted test file passes 14 of 14 including the new one pinning the fix.
The removal is the whole fix and it is correct: update-downloaded was already completing the task through its own listener, so deleting the progress-tick completion arm leaves exactly one route to completed, closing the race this PR describes without touching the rounding it deliberately leaves alone. The updated test asserts the state that matters, progress at 100 with the task still in-progress and no Discard button, then fires the downloaded event and checks completion, which is a stronger assertion than the one it replaced.
No blocking findings. Approving.
2 participants
Fixes #200 from Zaldaryon's review batch. The launcher update task had two routes to completed: the update-downloaded event, and a progress tick reading 100. Since progress is rounded up, a 99.6 percent tick reported 100 and completed the task while bytes were still moving, and on Windows while the installer signature was still being verified, so a later verification failure would flip an already-completed task back to failed.
The progress listener's completion arm is gone. update-downloaded was already completing the task and is now the only thing that can. Math.round stays: flooring would hide the symptom without removing the second route, and with no tick able to complete anything, a bar reading 100 for the last moment is honest rather than misleading. The existing test that asserted completion on a final tick of 100 encoded the bug and now asserts the opposite; reinstating the deleted arm fails it.
Two notes on the rest of that review batch. #199 is not here: Zaldaryon fixed it in #210 at the same place this branch originally did (the shared readModDetail guard), so that commit was dropped rather than duplicated. #201 needs no change and is closed with the reasoning: electron-updater's constructor attaches its own error listener before our code runs and autoUpdater.logger is assigned at module scope outside the guard, so errors always reach the redacting logger; our listener only resets download state and drives UI, and when canAutoUpdate refuses there is no flow for it to act on, since checkForUpdates is only called in the ok branch and both DOWNLOAD_UPDATE and UPDATE_AND_RESTART refuse on flags only settable from listeners inside the guarded registration.