fix(electron): tree-kill the server on exit/update to release the omniroute.exe lock (#3347) (#3354)

CHANGELOG.md

@@ -14,6 +14,7 @@ _Development cycle in progress — entries are added as work merges into `releas
 
 ### 🔧 Bug Fixes
 
+- **fix(electron):** clicking "Exit" (or applying an update) now terminates the **whole** server process tree, not just the direct child. The embedded server runs as `omniroute.exe`-as-node (`ELECTRON_RUN_AS_NODE`) and spawns grandchildren (embedded services, MITM proxy, tunnels); on Windows `ChildProcess.kill()` only terminates the direct child, so survivors kept `omniroute.exe` locked — the process "hung in memory" after Exit and updates failed with "file in use". New `killProcessTree()` helper uses `taskkill /PID <pid> /T /F` on Windows (signal-based on POSIX); wired into `stopNextServer`, the `waitForServerExit` force-kill, and `installUpdate`. (#3347 — thanks @Flexible78)
 - **fix(proxy):** proxy auto-selection is now **opt-in** (new `PROXY_AUTO_SELECT_ENABLED` flag, default off). Previously a single proxy in the registry silently became a global fallback for **all** provider connections (the Step-11 fallback listed every registry proxy, ignoring assignments and per-connection `proxy_enabled`). It now no-ops unless the operator enables the flag. (#3332 — thanks @hertznsk)
 - **fix(cli):** write the OpenCode config to `~/.config/opencode/opencode.json` on **all** platforms — on Windows OmniRoute wrote to `%APPDATA%\opencode\` but OpenCode reads from `%USERPROFILE%\.config\opencode\` (XDG), so dashboard-saved config silently had no effect. (#3330 — thanks @abdulkadirozyurt)
 - **fix(catalog):** remove `minimaxai/minimax-m3` from the **NVIDIA NIM** tier — NVIDIA does not host it yet, so every request 404'd (`404 page not found`), while sibling `minimax-m2.7` on the same provider works. MiniMax M3 stays available on the tiers that actually serve it. (#3329 — thanks @mikmaneggahommie)

electron/main.js

@@ -34,6 +34,7 @@ const fs = require("fs");
 const { autoUpdater } = require("electron-updater");
 const { hasEncryptedCredentials } = require("./sqlite-inspection");
 const { loginManager } = require("./loginManager");
+const { killProcessTree } = require("./processTree");
 
 // ── Single Instance Lock ───────────────────────────────────
 const gotTheLock = app.requestSingleInstanceLock();
@@ -200,7 +201,9 @@ async function waitForServerExit(proc, timeoutMs = 5000) {
     new Promise((r) =>
       setTimeout(() => {
         try {
-          proc.kill("SIGKILL");
+          // #3347: force-kill the whole tree (Windows leaves grandchildren alive on a
+          // bare SIGKILL of the direct child, keeping omniroute.exe locked).
+          killProcessTree(proc, { signal: "SIGKILL" });
         } catch {
           /* already dead */
         }
@@ -286,7 +289,9 @@ async function downloadUpdate() {
 
 function installUpdate() {
   if (nextServer) {
-    nextServer.kill("SIGTERM");
+    // #3347: tree-kill before quitAndInstall — a surviving server child (and its
+    // grandchildren) keeps omniroute.exe locked and the updater fails with "file in use".
+    killProcessTree(nextServer, { signal: "SIGTERM" });
     nextServer = null;
   }
   autoUpdater.quitAndInstall();
@@ -666,7 +671,10 @@ function startNextServer() {
 
 function stopNextServer() {
   if (nextServer) {
-    nextServer.kill("SIGTERM");
+    // #3347: kill the whole tree, not just the direct child. On Windows the server
+    // (omniroute.exe-as-node) spawns grandchildren that a bare SIGTERM leaves alive,
+    // holding a lock on omniroute.exe and blocking updates.
+    killProcessTree(nextServer, { signal: "SIGTERM" });
     nextServer = null;
   }
 }

electron/package.json

@@ -52,6 +52,7 @@
       "main.js",
       "preload.js",
       "loginManager.js",
+      "processTree.js",
       "sqlite-inspection.js",
       "package.json",
       "node_modules/**/*"

electron/processTree.js

@@ -0,0 +1,62 @@
+"use strict";
+
+// Cross-platform "kill the whole process tree" helper (#3347).
+//
+// The embedded server is spawned via process.execPath (= omniroute.exe) with
+// ELECTRON_RUN_AS_NODE=1, and it in turn spawns grandchildren (embedded services,
+// MITM proxy, tunnels — several also omniroute.exe-as-node). On Windows, Node's
+// ChildProcess.kill()/SIGTERM/SIGKILL only terminate the DIRECT child via
+// TerminateProcess — they do NOT walk the tree. Surviving grandchildren keep a lock
+// on omniroute.exe, so the process "hangs in memory" after Exit and updates fail with
+// "file in use". Windows needs `taskkill /PID <pid> /T /F` (the /T flag terminates the
+// process AND its descendants). POSIX keeps signal-based termination, which propagates.
+
+const { spawn } = require("child_process");
+
+/**
+ * Terminate a child process and all of its descendants.
+ * @param {{ pid?: number, kill?: (signal?: string) => void } | null | undefined} proc
+ * @param {{ platform?: string, signal?: string, spawnFn?: typeof spawn }} [options]
+ */
+function killProcessTree(proc, options = {}) {
+  if (!proc || proc.pid == null) return;
+  const platform = options.platform || process.platform;
+  const signal = options.signal || "SIGTERM";
+
+  if (platform === "win32") {
+    const spawnFn = options.spawnFn || spawn;
+    try {
+      // Array args + no shell → the pid (an integer we own) is never interpolated into a
+      // shell command string (Hard Rule #13). /T walks the tree, /F forces termination.
+      const killer = spawnFn("taskkill", ["/PID", String(proc.pid), "/T", "/F"], {
+        windowsHide: true,
+      });
+      if (killer && typeof killer.on === "function") {
+        killer.on("error", () => {
+          try {
+            proc.kill(signal);
+          } catch {
+            /* already dead */
+          }
+        });
+      }
+    } catch {
+      // taskkill unavailable (rare) — fall back to the direct kill.
+      try {
+        proc.kill(signal);
+      } catch {
+        /* already dead */
+      }
+    }
+    return;
+  }
+
+  // POSIX: signals propagate to the process group of a normally-spawned child.
+  try {
+    proc.kill(signal);
+  } catch {
+    /* already dead */
+  }
+}
+
+module.exports = { killProcessTree };

tests/unit/electron-processtree.test.ts

@@ -0,0 +1,105 @@
+/**
+ * Regression test for #3347 — Electron "Exit" leaves a process in memory that locks
+ * omniroute.exe on Windows.
+ *
+ * The embedded server is spawned via process.execPath (= omniroute.exe) with
+ * ELECTRON_RUN_AS_NODE=1. On Windows, ChildProcess.kill()/SIGTERM/SIGKILL terminate ONLY
+ * the direct child — NOT its descendants — so server-spawned grandchildren (embedded
+ * services, MITM proxy, tunnels, several also omniroute.exe-as-node) survive and keep the
+ * .exe locked, blocking updates. killProcessTree() must use `taskkill /PID <pid> /T /F`
+ * (the /T flag walks the tree) on win32, and signal-based kill on POSIX (where signals
+ * propagate). This test pins that platform branch, plus a static guard that main.js routes
+ * the server shutdown through killProcessTree (not a raw nextServer.kill).
+ */
+import { describe, it } from "node:test";
+import assert from "node:assert/strict";
+import { readFileSync } from "node:fs";
+import { join } from "node:path";
+import { createRequire } from "node:module";
+
+const require = createRequire(import.meta.url);
+const { killProcessTree } = require("../../electron/processTree.js");
+
+describe("killProcessTree (#3347)", () => {
+  it("win32: kills the whole tree via `taskkill /PID <pid> /T /F` (not proc.kill)", () => {
+    const spawnCalls: Array<{ cmd: string; args: string[] }> = [];
+    let procKillCalled = false;
+    const proc = {
+      pid: 1234,
+      kill: () => {
+        procKillCalled = true;
+      },
+    };
+    const spawnFn = (cmd: string, args: string[]) => {
+      spawnCalls.push({ cmd, args });
+      return { on: () => {} };
+    };
+
+    killProcessTree(proc, { platform: "win32", signal: "SIGTERM", spawnFn });
+
+    assert.equal(spawnCalls.length, 1, "expected exactly one taskkill spawn");
+    assert.equal(spawnCalls[0].cmd, "taskkill");
+    assert.deepEqual(spawnCalls[0].args, ["/PID", "1234", "/T", "/F"]);
+    assert.equal(procKillCalled, false, "must NOT fall back to proc.kill when taskkill spawns");
+  });
+
+  it("posix: uses signal-based proc.kill (signals propagate), never taskkill", () => {
+    let killedWith: string | null = null;
+    let spawned = false;
+    const proc = {
+      pid: 4321,
+      kill: (sig: string) => {
+        killedWith = sig;
+      },
+    };
+    const spawnFn = () => {
+      spawned = true;
+      return { on: () => {} };
+    };
+
+    killProcessTree(proc, { platform: "linux", signal: "SIGTERM", spawnFn });
+
+    assert.equal(killedWith, "SIGTERM");
+    assert.equal(spawned, false, "must not spawn taskkill on POSIX");
+  });
+
+  it("win32 fallback: taskkill spawn throwing falls back to proc.kill", () => {
+    let killedWith: string | null = null;
+    const proc = {
+      pid: 99,
+      kill: (sig: string) => {
+        killedWith = sig;
+      },
+    };
+    const spawnFn = () => {
+      throw new Error("taskkill not found");
+    };
+
+    killProcessTree(proc, { platform: "win32", signal: "SIGKILL", spawnFn });
+
+    assert.equal(killedWith, "SIGKILL", "fallback to proc.kill when taskkill is unavailable");
+  });
+
+  it("no-op on null/pid-less process (does not throw)", () => {
+    assert.doesNotThrow(() => killProcessTree(null, { platform: "win32" }));
+    assert.doesNotThrow(() => killProcessTree({ pid: undefined }, { platform: "win32" }));
+  });
+});
+
+describe("Electron main.js server shutdown routes through killProcessTree (#3347)", () => {
+  const main = readFileSync(join(import.meta.dirname, "../../electron/main.js"), "utf8");
+
+  it("requires the processTree helper", () => {
+    assert.match(main, /require\(["']\.\/processTree["']\)/);
+  });
+
+  it("does not kill the server child with a raw signal kill (must use the tree-kill)", () => {
+    // The two shutdown call sites (stopNextServer + waitForServerExit) must not use a bare
+    // `nextServer.kill(` / `proc.kill("SIGKILL")` on the server proc anymore.
+    assert.doesNotMatch(main, /nextServer\.kill\(/);
+    assert.ok(
+      /killProcessTree\s*\(/.test(main),
+      "main.js must call killProcessTree() for server shutdown"
+    );
+  });
+});