fix(updates): a manual check must not discard the update already downloaded

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,31 @@ 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();
+
+    // 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();
   });

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,
@@ -933,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,
@@ -964,7 +997,9 @@ export function createAutoUpdateService({
   updater.on("update-cancelled", onUpdateCancelled);
   updater.on("error", onError);
 
-  async function runUpdateCheck(args: { allowReady?: boolean } = {}): Promise<void> {
+  async function runUpdateCheck(
+    args: { allowReady?: boolean; metadataOnlyWhenReady?: boolean } = {},
+  ): Promise<void> {
     if (checkPromise) {
       await checkPromise;
       return;
@@ -984,6 +1019,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;
@@ -1024,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,
@@ -1046,6 +1092,7 @@ export function createAutoUpdateService({
       .finally(() => {
         checkPromise = null;
         preservedDownloadRetry = null;
+        readyMetadataRefreshInProgress = false;
       });
     await checkPromise;
   }
@@ -1062,7 +1109,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<boolean> {