From f7742c4c5b911c4b67aa17d4bc835a582a3f6c9e Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Thu, 20 Aug 2026 03:15:48 -0400 Subject: [PATCH 1/2] fix(updates): a manual check must not discard the update already downloaded MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #1134 let the Settings button check while an update was staged, and claimed the staged download was left untouched. It was not. electron-updater emits `update-available` BEFORE checkForUpdates() resolves, so for a newer version onUpdateAvailable took the supersede branch: it cleaned the updater cache ("superseded_ready_update") — deleting the finished download — and patched the status off `ready`. The resolve path's `status === "ready"` guard then no longer held, so it started downloading the new version. Pressing "Check for updates" with 1.2.61 staged therefore threw away ~200 MB of completed download and began again, and the pending restart was lost. A user-initiated check while ready is now metadata-only: it records the newest version and returns, leaving the cache, the status and the staged version alone. The pre-install refresh and the automatic timers are unchanged. The regression test now emits the real event order (checking-for-update and update-available before resolution) instead of only resolving, and asserts downloadUpdate is never called. Verified failing without the fix, with the snapshot reaching `downloading` — the mock in #1134 could not observe this, which is why the test passed against broken behaviour. Co-Authored-By: Claude Opus 5 --- .../updates/autoUpdateService.test.ts | 17 +++++++++-- .../services/updates/autoUpdateService.ts | 30 +++++++++++++++++-- 2 files changed, 42 insertions(+), 5 deletions(-) diff --git a/apps/desktop/src/main/services/updates/autoUpdateService.test.ts b/apps/desktop/src/main/services/updates/autoUpdateService.test.ts index fc593cae0..07633f8b0 100644 --- a/apps/desktop/src/main/services/updates/autoUpdateService.test.ts +++ b/apps/desktop/src/main/services/updates/autoUpdateService.test.ts @@ -447,9 +447,17 @@ describe("createAutoUpdateService", () => { await Promise.resolve(); expect(updater.checkForUpdates).not.toHaveBeenCalled(); - updater.checkForUpdates.mockImplementation(async () => ({ - updateInfo: { version: "1.2.63" }, - })); + // electron-updater emits these BEFORE checkForUpdates() resolves. A mock + // that only resolves cannot see the bug this test exists for: the + // update-available handler is what supersedes the staged update, deletes + // the finished download, and frees the resolve path to start a new one. + const downloadUpdate = vi.fn(async () => null); + (updater as unknown as { downloadUpdate: unknown }).downloadUpdate = downloadUpdate; + updater.checkForUpdates.mockImplementation(async () => { + updater.emit("checking-for-update"); + updater.emit("update-available", { version: "1.2.63" }); + return { updateInfo: { version: "1.2.63" } }; + }); service.checkForUpdates({ userInitiated: true }); await vi.waitFor(() => { @@ -461,6 +469,9 @@ describe("createAutoUpdateService", () => { latestKnownVersion: "1.2.63", }); }); + // Asking what the newest version is must not throw away the update the + // user already downloaded and is one restart away from installing. + expect(downloadUpdate).not.toHaveBeenCalled(); service.dispose(); }); diff --git a/apps/desktop/src/main/services/updates/autoUpdateService.ts b/apps/desktop/src/main/services/updates/autoUpdateService.ts index 29b80e434..dd72351ec 100644 --- a/apps/desktop/src/main/services/updates/autoUpdateService.ts +++ b/apps/desktop/src/main/services/updates/autoUpdateService.ts @@ -513,6 +513,12 @@ export function createAutoUpdateService({ let installReadySnapshot: AutoUpdateSnapshot | null = null; let ignoredDownloadVersion: string | null = null; let readyRefreshInProgress = false; + /** + * A user-initiated check while an update is already staged. The answer they + * want is "what is the newest version" — throwing away the update they have + * already downloaded in order to answer it is not a service. + */ + let readyMetadataRefreshInProgress = false; const readyRefreshFailure: { current: { error: unknown; phase: AutoUpdatePhase } | null; } = { current: null }; @@ -815,6 +821,20 @@ export function createAutoUpdateService({ }); return; } + if (readyMetadataRefreshInProgress) { + // electron-updater emits `update-available` BEFORE checkForUpdates() + // resolves, so without this the newer version would supersede the + // staged one here — deleting the finished download from the cache and + // leaving the resolve path free to start a fresh one — purely because + // someone pressed a button to ask a question. + ignoredDownloadVersion = info.version; + patchSnapshot({ latestKnownVersion: info.version }); + logger.info("autoUpdate.update_available_metadata_only", { + version: info.version, + readyVersion: snapshot.version, + }); + return; + } if (!readyRefreshInProgress) { cleanupUpdaterCacheDir({ updaterCacheDir, @@ -964,7 +984,9 @@ export function createAutoUpdateService({ updater.on("update-cancelled", onUpdateCancelled); updater.on("error", onError); - async function runUpdateCheck(args: { allowReady?: boolean } = {}): Promise { + async function runUpdateCheck( + args: { allowReady?: boolean; metadataOnlyWhenReady?: boolean } = {}, + ): Promise { if (checkPromise) { await checkPromise; return; @@ -984,6 +1006,8 @@ export function createAutoUpdateService({ preservedDownloadRetry = reusableDownloadedVersion ? { version: reusableDownloadedVersion, releaseNotesUrl: snapshot.releaseNotesUrl } : null; + readyMetadataRefreshInProgress = args.metadataOnlyWhenReady === true + && snapshot.status === "ready"; checkPromise = updater.checkForUpdates() .then(async (result) => { const updateInfo = isUpdateCheckResultLike(result) ? result.updateInfo : undefined; @@ -1046,6 +1070,7 @@ export function createAutoUpdateService({ .finally(() => { checkPromise = null; preservedDownloadRetry = null; + readyMetadataRefreshInProgress = false; }); await checkPromise; } @@ -1062,7 +1087,8 @@ export function createAutoUpdateService({ * refreshes the newest known version without disturbing the staged download. */ function checkForUpdates(options: { userInitiated?: boolean } = {}): void { - void runUpdateCheck({ allowReady: options.userInitiated === true }); + const userInitiated = options.userInitiated === true; + void runUpdateCheck({ allowReady: userInitiated, metadataOnlyWhenReady: userInitiated }); } async function refreshReadyUpdateBeforeInstall(): Promise { From b091ad7463580ed9f9fc6502b19961d5f7fd2326 Mon Sep 17 00:00:00 2001 From: Arul Sharma <31745423+arul28@users.noreply.github.com> Date: Thu, 20 Aug 2026 03:31:25 -0400 Subject: [PATCH 2/2] fix(updates): a failed manual check must also leave the staged update alone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both review bots caught the same hole independently: the metadata-only path covered success and not failure. onError and the runUpdateCheck catch special- cased only readyRefreshInProgress, so a feed error during a user-initiated check fell through to setErrorSnapshot — replacing the `ready` snapshot with `error` and, with currentPhase already moved to "download" by `checking-for-update`, taking the finished download with it. A network blip while pressing "Check for updates" would therefore destroy a completed update: worse than the silent no-op this all started as. Both paths now return early during a metadata refresh, logging autoUpdate.metadata_refresh_failed and leaving status, version and cache untouched. The user is told nothing new, which is the honest answer when the feed did not respond. Test extended to the failure path and verified failing without the fix (snapshot reached `error`). The added settle before the second check is deliberate: while checkPromise is set the in-flight guard swallows the call and the assertion would prove nothing. Co-Authored-By: Claude Opus 5 --- .../updates/autoUpdateService.test.ts | 22 +++++++++++++++++++ .../services/updates/autoUpdateService.ts | 22 +++++++++++++++++++ 2 files changed, 44 insertions(+) diff --git a/apps/desktop/src/main/services/updates/autoUpdateService.test.ts b/apps/desktop/src/main/services/updates/autoUpdateService.test.ts index 07633f8b0..37fed4a16 100644 --- a/apps/desktop/src/main/services/updates/autoUpdateService.test.ts +++ b/apps/desktop/src/main/services/updates/autoUpdateService.test.ts @@ -473,6 +473,28 @@ describe("createAutoUpdateService", () => { // user already downloaded and is one restart away from installing. expect(downloadUpdate).not.toHaveBeenCalled(); + // Let the first check's promise chain settle: while `checkPromise` is + // still set, a second call is swallowed by the in-flight guard and would + // assert nothing. + await new Promise((resolve) => setTimeout(resolve, 0)); + + // ...and neither must a check that fails. A feed error here means "nothing + // new to tell you", not "discard the finished download". + updater.checkForUpdates.mockImplementation(async () => { + updater.emit("checking-for-update"); + updater.emit("error", new Error("net::ERR_INTERNET_DISCONNECTED")); + throw new Error("net::ERR_INTERNET_DISCONNECTED"); + }); + service.checkForUpdates({ userInitiated: true }); + + await vi.waitFor(() => expect(updater.checkForUpdates).toHaveBeenCalledTimes(2)); + expect(service.getSnapshot()).toMatchObject({ + status: "ready", + version: "1.2.61", + latestKnownVersion: "1.2.63", + }); + expect(downloadUpdate).not.toHaveBeenCalled(); + service.dispose(); }); diff --git a/apps/desktop/src/main/services/updates/autoUpdateService.ts b/apps/desktop/src/main/services/updates/autoUpdateService.ts index dd72351ec..9a5105bf8 100644 --- a/apps/desktop/src/main/services/updates/autoUpdateService.ts +++ b/apps/desktop/src/main/services/updates/autoUpdateService.ts @@ -953,6 +953,19 @@ export function createAutoUpdateService({ phase: classified.phase, }); ignoredDownloadVersion = null; + if (readyMetadataRefreshInProgress) { + // The user asked what the newest version is and the feed did not answer. + // That is a reason to tell them nothing new, not a reason to throw away + // the update they already downloaded: setErrorSnapshot would replace the + // `ready` snapshot and, with currentPhase already moved to "download" by + // `checking-for-update`, take the finished download with it. + logger.warn("autoUpdate.metadata_refresh_failed", { + message, + kind: classified.kind, + readyVersion: snapshot.version, + }); + return; + } if (readyRefreshInProgress) { readyRefreshFailure.current = { error: err, @@ -1048,6 +1061,15 @@ export function createAutoUpdateService({ } }) .catch((error) => { + // Same reasoning as onError: a metadata-only refresh must leave the + // staged update exactly as it found it, however it fails. + if (readyMetadataRefreshInProgress) { + logger.warn("autoUpdate.metadata_refresh_failed", { + message: formatErrorMessage(error), + readyVersion: snapshot.version, + }); + return; + } if (readyRefreshInProgress) { readyRefreshFailure.current = { error,