fix: prevent Windows NSIS auto-update stall by capping before-quit cleanup time (#1497)

* fix: cap before-quit cleanup to 2 s on Windows to prevent NSIS update stall

* fix: shorten force-kill timeout to 500 ms on Windows in child process kill()

* fix: bound all quit paths and confirm child death

---------

Co-authored-by: Nicolas Echezarreta <nicoecheza@gmail.com>

Author: decentraland-bot

packages/creator-hub/main/src/index.ts

@@ -4,6 +4,7 @@ import { app } from 'electron';
 import {
   init as sentryInit,
   captureException,
+  flush as sentryFlush,
   electronBreadcrumbsIntegration,
   electronContextIntegration,
   childProcessIntegration,
@@ -15,7 +16,8 @@ import {
 import log from 'electron-log/main';
 
 import { restoreOrCreateMainWindow } from '/@/mainWindow';
-import { killAllUtilityProcesses } from '/@/modules/bin';
+import { delay } from '/shared/utils';
+import { FORCE_KILL_TIMEOUT_MS, killAllUtilityProcesses } from '/@/modules/bin';
 import { initIpc } from '/@/modules/ipc';
 import { deployServer, killAllPreviews } from '/@/modules/cli';
 import { killAllRealms } from '/@/modules/bevy-realm';
@@ -128,13 +130,18 @@ app
 
     initIpc({
       beforeQuitCleanup: async () => {
+        // raise the skip flag only after cleanup finishes: raising it earlier disarms
+        // the before-quit guard for the whole cleanup window, and a concurrent quit
+        // would tear the app down mid-killAll with the installer never launched
+        await killAll();
+        await flushTelemetry();
         setSkipBeforeQuitCleanup();
-        try {
-          await killAll();
-        } catch (error) {
+        // quitAndInstall() reports failure through an 'error' event instead of
+        // throwing; if the app is still running after this window the install did
+        // not happen, so re-arm cleanup instead of skipping it on every future quit
+        setTimeout(() => {
           skipBeforeQuitCleanup = false;
-          throw error;
-        }
+        }, SKIP_CLEANUP_REARM_MS);
       },
     });
     log.info('[IPC] Ready');
@@ -159,27 +166,72 @@ export function setSkipBeforeQuitCleanup() {
   skipBeforeQuitCleanup = true;
 }
 
+// The quit budget must stay above bin.ts's per-child FORCE_KILL_TIMEOUT_MS: a smaller
+// cap would exit before the SIGKILL escalation fires and orphan any child that
+// survived the graceful signal. On Windows this lands on the 2 s NSIS close window.
+const QUIT_CLEANUP_TIMEOUT_MS = FORCE_KILL_TIMEOUT_MS + 1500;
+const TELEMETRY_FLUSH_TIMEOUT_MS = 1000;
+const SKIP_CLEANUP_REARM_MS = 5000;
+
 export async function killAll() {
   const promises: Promise<unknown>[] = [killAllPreviews(), killAllRealms()];
   if (deployServer) {
     promises.push(deployServer.stop());
   }
   killInspectorServer();
   promises.push(killAllUtilityProcesses());
-  await Promise.all(promises);
+
+  // Cap cleanup here so every quit path inherits the bound — before-quit,
+  // window-all-closed and the in-app update flow. On Windows, NSIS has a fixed
+  // window to close the app before the "cannot be closed" dialog appears.
+  const cleanup = Promise.all(promises).then(
+    () => 'done' as const,
+    error => {
+      captureException(error, { tags: { source: 'kill-all' } });
+      log.error('[App] Failed to kill all servers:', error);
+      return 'failed' as const;
+    },
+  );
+  const result = await Promise.race([
+    cleanup,
+    delay(QUIT_CLEANUP_TIMEOUT_MS).then(() => 'timed-out' as const),
+  ]);
+  if (result === 'timed-out') {
+    log.warn(
+      `[App] Cleanup still running after ${QUIT_CLEANUP_TIMEOUT_MS}ms, quitting without waiting for it`,
+    );
+  }
+}
+
+/**
+ * Segment batches events (15 events / 10 s flush) and Sentry buffers its transport, so
+ * exiting right after a capped cleanup would drop anything still queued.
+ */
+async function flushTelemetry() {
+  try {
+    const analytics = getAnalytics();
+    await Promise.all([
+      analytics ? analytics.closeAndFlush({ timeout: TELEMETRY_FLUSH_TIMEOUT_MS }) : null,
+      sentryFlush(TELEMETRY_FLUSH_TIMEOUT_MS),
+    ]);
+  } catch (error) {
+    log.error('[App] Failed to flush telemetry:', error);
+  }
 }
 
+let quitInProgress = false;
+
 app.on('before-quit', async event => {
   if (skipBeforeQuitCleanup) {
     return;
   }
   event.preventDefault();
-  try {
-    await killAll();
-  } catch (error) {
-    captureException(error, { tags: { source: 'before-quit' } });
-    log.error('[App] Failed to kill all servers:', error);
+  if (quitInProgress) {
+    return;
   }
+  quitInProgress = true;
+  await killAll();
+  await flushTelemetry();
   log.info('[App] Quit');
   app.exit();
 });

packages/creator-hub/main/src/modules/bin.ts

@@ -4,7 +4,7 @@ import { exec as execSync, spawn } from 'child_process';
 import log from 'electron-log/main';
 import { shell, utilityProcess } from 'electron';
 import treeKill from 'tree-kill';
-import { future } from 'fp-future';
+import { future, type IFuture } from 'fp-future';
 import isRunning from 'is-running';
 import { ErrorBase } from '/shared/types/error';
 import { createCircularBuffer } from '/shared/circular-buffer';
@@ -27,6 +27,12 @@ const exec = promisify(execSync);
 
 const MAX_BUFFER_SIZE = 2048;
 
+// Window for a child to exit gracefully before the kill escalates.
+// On Windows, tree-kill already issues taskkill /F /T so a long graceful wait just delays
+// NSIS-triggered update installs; 500 ms is sufficient for the process tree to collapse.
+// The quit budget in index.ts is derived from this value and must stay above it.
+export const FORCE_KILL_TIMEOUT_MS = process.platform === 'win32' ? 500 : 5000;
+
 type Error = 'COMMAND_FAILED';
 
 export class StreamError extends ErrorBase<Error> {
@@ -107,6 +113,7 @@ type RunOptions = {
 export function run(pkg: string, bin: string, options: RunOptions = {}): Child {
   let isKilling = false;
   let alive = true;
+  let killPromise: IFuture<void> | null = null;
 
   const promise = future<Awaited<ReturnType<Child['wait']>>>();
   const matchers: Matcher[] = [];
@@ -145,10 +152,38 @@ export function run(pkg: string, bin: string, options: RunOptions = {}): Child {
     delete childEnv.ELECTRON_RUN_AS_NODE;
   }
 
+  const ready = future<void>();
+
   const forked: ChildProcessLike = nodePath
     ? spawn(nodePath, [binPath, ...args], { cwd, stdio: 'pipe', env: childEnv })
     : utilityProcess.fork(binPath, [...args], { cwd, stdio: 'pipe', env: childEnv });
 
+  // A plain child process reports a failed launch through 'error', and no 'exit' follows
+  // it; settle everything here so wait() and kill() never block on a process that never
+  // ran. Electron utility processes only emit 'spawn'/'exit'.
+  if (nodePath) {
+    (forked as ReturnType<typeof spawn>).on('error', error => {
+      if (!alive) return;
+      alive = false;
+      log.error(`[UtilityProcess] Process "${name}" failed to start:`, error);
+      if (isKilling) {
+        promise.resolve(Buffer.concat(stdout.getAll()));
+      } else {
+        promise.reject(
+          new StreamError(
+            'COMMAND_FAILED',
+            `Error: process "${name}" failed to start: ${error.message}`,
+            Buffer.concat(stdout.getAll()),
+            Buffer.concat(stderr.getAll()),
+          ),
+        );
+      }
+      cleanup();
+      ready.resolve();
+      killPromise?.resolve();
+    });
+  }
+
   const cleanup = () => {
     for (const matcher of matchers) {
       matcher.enabled = false;
@@ -173,8 +208,6 @@ export function run(pkg: string, bin: string, options: RunOptions = {}): Child {
     stdall.push(Uint8Array.from(data));
   });
 
-  const ready = future<void>();
-
   const name = `${bin} ${args.join(' ')}`.trim();
   let spawnedPid: number | undefined;
 
@@ -212,6 +245,11 @@ export function run(pkg: string, bin: string, options: RunOptions = {}): Child {
       promise.resolve(stdoutBuf);
     }
     cleanup();
+    // a spawn that never happened still needs kill() unblocked, and an exit landing
+    // mid-kill() must settle the kill instead of leaving it to the pid poll, which
+    // can stay truthy forever on Windows pid reuse
+    ready.resolve();
+    killPromise?.resolve();
   });
 
   const child: Child = {
@@ -269,52 +307,69 @@ export function run(pkg: string, bin: string, options: RunOptions = {}): Child {
         }
       }),
     kill: async () => {
+      // a repeat caller shares the in-flight kill instead of getting an instantly
+      // resolved undefined that would let shutdown truncate the first one's cleanup
+      if (killPromise) return killPromise;
+      if (!alive) return;
+
+      const pending = (killPromise = future<void>());
+      isKilling = true;
+
       await ready;
-      const pid = forked.pid!;
 
-      // if child is being killed or already killed then return
-      if (isKilling || !alive) return;
+      const pid = spawnedPid;
+      if (!alive || !pid) {
+        pending.resolve();
+        return pending;
+      }
 
-      isKilling = true;
       log.info(`[UtilityProcess] Killing process "${name}" with pid=${pid}...`);
 
-      // create promise to kill child
-      const killPromise = future<void>();
-
       // kill child gracefully
       treeKill(pid);
 
-      // child successfully killed
-      const die = (force: boolean = false) => {
+      let forced = false;
+
+      // child confirmed dead: settle wait() too — 'exit' early-returns once alive is
+      // false, and may never fire at all after a forced tree kill
+      const die = () => {
+        if (!pending.isPending) return;
         alive = false;
+        processes.delete(pid);
+        log.info(
+          `[UtilityProcess] Process "${name}" with pid=${pid} ${
+            forced ? 'forcefully' : 'gracefully'
+          } killed`,
+        );
+        promise.resolve(Buffer.concat(stdout.getAll()));
         cleanup();
-        clearInterval(interval);
-        clearTimeout(timeout);
-        if (force) {
-          log.info(`[UtilityProcess] Process "${name}" with pid=${pid} forcefully killed`);
-          treeKill(pid, 'SIGKILL');
-        } else {
-          log.info(`[UtilityProcess] Process "${name}" with pid=${pid} gracefully killed`);
-        }
-        killPromise.resolve();
+        pending.resolve();
       };
 
       // interval to check if child still running and flag it as dead when is not running anymore
       const interval = setInterval(() => {
-        if (!pid || !isRunning(pid)) {
+        if (!isRunning(pid)) {
           die();
         }
       }, 100);
 
-      // timeout to stop checking if child still running, kill it with fire
+      // timeout to stop waiting for a graceful exit, kill it with fire. The poll keeps
+      // running afterwards: resolving right here would declare success while the tree
+      // can still be alive holding the very files an NSIS update needs to replace.
       const timeout = setTimeout(() => {
         if (alive) {
-          die(true);
+          forced = true;
+          treeKill(pid, 'SIGKILL');
         }
-      }, 5000);
+      }, FORCE_KILL_TIMEOUT_MS);
+
+      // whether death is confirmed by the poll or by the 'exit' event, stop the timers
+      void pending.then(() => {
+        clearInterval(interval);
+        clearTimeout(timeout);
+      });
 
-      // return promise
-      return killPromise;
+      return pending;
     },
     alive: () => alive,
   };

packages/creator-hub/main/src/modules/cli.ts

@@ -615,7 +615,7 @@ export async function legacyDeploy({
 
   process.waitFor(/close the terminal/gi).then(() => process.kill());
 
-  process.wait().catch(); // handle rejection of main promise to avoid warnings in console
+  process.wait().catch(() => {}); // handle rejection of main promise to avoid warnings in console
 
   deployServer = { stop: () => process.kill() };