Merge pull request #838 from Wibias/fix/update-proxy-recovery

fix(update): recover Windows dashboard proxy after self-update

Author: Wibias

bin/ocx.mjs

@@ -250,12 +250,40 @@ function runNpmSelfUpdate() {
       try {
         const svcArgs = serviceReinstallArgs();
         const svc = spawnSync(process.execPath, svcArgs, { stdio: "inherit", windowsHide: true });
-        if (svc.status !== 0) {
+        let needDirectStart = svc.status !== 0;
+        if (!needDirectStart) {
+          // Exit 0 can still leave stale/missing assets that never bring the proxy
+          // back — match the GUI/CLI fallthrough so /healthz is not left dead.
+          try {
+            const st = spawnSync(process.execPath, [launcher, "status", "--json"], {
+              encoding: "utf8",
+              timeout: 20_000,
+              windowsHide: true,
+            });
+            if (st.status === 0 && typeof st.stdout === "string" && st.stdout.trim()) {
+              const parsed = JSON.parse(st.stdout);
+              const proxyUp = parsed?.proxy?.running === true || parsed?.proxy?.health?.ok === true;
+              const viable = parsed?.startup?.serviceViable === true;
+              if (!proxyUp && !viable) needDirectStart = true;
+            } else {
+              // status failed or empty — fail closed to direct start (match CLI).
+              needDirectStart = true;
+            }
+          } catch {
+            needDirectStart = true;
+          }
+        }
+        if (needDirectStart) {
           // On Windows, schtasks /create requires elevation. The launcher inherits the
           // user's (non-admin) token, so the service reinstall can fail with access
-          // denied. Fall back to a direct detached proxy start so the update never
-          // leaves the user without a running proxy.
-          console.warn("opencodex: service refresh failed — starting the proxy directly instead.");
+          // denied — or exit 0 while leaving a non-viable manager. Fall back to a
+          // direct detached proxy start so the update never leaves the user without
+          // a running proxy.
+          console.warn(
+            svc.status === 0
+              ? "opencodex: service refresh left a non-viable manager — starting the proxy directly instead."
+              : "opencodex: service refresh failed — starting the proxy directly instead.",
+          );
           console.warn("  Run 'ocx service install' as administrator to refresh the background service.");
           const env = { ...process.env };
           delete env.OCX_SERVICE;

src/adapters/cursor/native-exec-shell.ts

@@ -335,17 +335,17 @@ function waitForBackgroundShellClose(entry: BackgroundShellEntry): Promise<boole
   if (backgroundShells.get(entry.shellId) !== entry) return Promise.resolve(true);
   return new Promise(resolveWait => {
     let settled = false;
-    const timer = backgroundShellRuntime.setTimer(() => {
-      if (settled) return;
-      settled = true;
-      resolveWait(false);
-    }, CURSOR_BACKGROUND_SHELL_TERM_GRACE_MS);
     // Deliberately REF'D: this is the bounded kill-grace wait that shutdown
     // drain awaits. Bun on Windows can starve unref'd timers when a pending
     // promise is the only other work, which would leave drainAndShutdown
     // waiting on this resolution forever. The timer self-clears within the
     // 2-second grace window (or earlier on close), so a ref cannot keep the
     // process alive beyond that bound.
+    const timer = backgroundShellRuntime.setTimer(() => {
+      if (settled) return;
+      settled = true;
+      resolveWait(false);
+    }, CURSOR_BACKGROUND_SHELL_TERM_GRACE_MS);
     void entry.closePromise.then(() => {
       if (settled) return;
       settled = true;

src/cli/index.ts

@@ -129,16 +129,21 @@ async function chooseListenPort(requestedPort?: number): Promise<number> {
   if (hardPin && preferred > 0) {
     const { reclaimListenPort } = await import("../server/port-reclaim");
     await reclaimListenPort(preferred, config.hostname ?? "127.0.0.1", {
-      timeoutMs: 30_000,
+      // Ghost LISTEN rows with a dead PID can outlive the process for a while.
+      // SetTcpEntry(DELETE_TCB) needs elevation (often returns 317), so the only
+      // reliable non-admin recovery is to wait for the OS to release the TCB.
+      timeoutMs: 60_000,
       intervalMs: 100,
       scanIntervalMs: 500,
       killOcxHolders: false,
-      dropTcpRows: false,
+      dropTcpRows: true,
     });
   }
   try {
     const selected = await findAvailablePort(preferred, config.hostname ?? "127.0.0.1", {
-      preferRetryMs: hardPin ? 0 : 750,
+      // After reclaim, keep probing briefly — ghost rows sometimes clear between
+      // the reclaim deadline and the final listen. Still never hop off `--port`.
+      preferRetryMs: hardPin ? 5_000 : 750,
       preferRetryIntervalMs: 50,
       allowEphemeralFallback: !hardPin,
     });

src/config.ts

@@ -2173,9 +2173,13 @@ export function parsePidFile(raw: string): number | null {
 export function isOcxStartCommandLine(commandLine: string): boolean {
   const normalized = commandLine.toLowerCase().replace(/\\/g, "/");
   // "src/cli.ts" matches pre-restructure installs still running; "src/cli/index.ts" is current.
+  // `@bitkyc08/.opencodex-*` is npm's in-place rename of the global package during
+  // `npm install -g` — a Windows service wrapper can respawn from that temp tree
+  // mid-update, and must still count as ocx for port reclaim.
   const hasOcxEntrypoint = normalized.includes("src/cli.ts")
     || normalized.includes("src/cli/index.ts")
     || normalized.includes("@bitkyc08/opencodex")
+    || /@bitkyc08\/\.opencodex-/.test(normalized)
     || /(?:^|[\s/"'])(?:ocx|opencodex)(?:\.cmd)?(?:$|[\s"'])/.test(normalized);
   return hasOcxEntrypoint && /(?:^|[\s"'])start(?:$|[\s"'])/.test(normalized);
 }

src/server/management/system-restart.ts

@@ -6,11 +6,14 @@
  * recycle to reclaim RSS, not a teardown.
  *
  * Respawn policy (matches real supervisor configs in src/service.ts):
- * - Supervised child (`OCX_SERVICE=1` + service installed): exit(1) so
+ * - Supervised child (`OCX_SERVICE=1` + viable service): exit(1) so
  *   failure-only supervisors (systemd Restart=on-failure, WinSW onfailure,
  *   Task Scheduler ERRORLEVEL loop) bring the proxy back.
  * - Otherwise: detached `ocx start --port <live>` (bypasses ensure's
  *   codexAutoStart gate), mark recycle so exit cleanup keeps injection, exit(0).
+ *   Installed-but-stale/missing service assets are NOT treated as supervised —
+ *   exit(1) would leave the proxy dead with `Service: installed, stale or missing
+ *   service assets` and a /healthz timeout.
  * - If detached spawn fails (sync throw or pre-start `error`): exit(1) without
  *   markRecycling — after drain the listen socket is already closed, so a latch
  *   reset cannot recover serving. Clear inherited `OCX_SERVICE` so exit cleanup
@@ -27,15 +30,16 @@ import {
   markRecyclingForExit,
   setDraining,
 } from "../lifecycle";
-import { isServiceInstalled } from "../../service";
+import { isServiceViable } from "../../service";
 import { readRuntimePort } from "../../config";
 
 /** Fixed v1 drain window for the memory-card action (not config-driven). */
 export const MEMORY_DRAIN_RESTART_MS = 60_000;
 
 export interface SystemRestartIo {
   drainAndShutdown?: typeof drainAndShutdown;
-  isServiceInstalled?: () => boolean;
+  /** True when a background service can actually respawn this process after exit(1). */
+  isServiceViable?: () => boolean;
   isSupervisedServiceChild?: () => boolean;
   /** Must resolve only after the replacement process has actually started. */
   spawnStart?: (port?: number) => void | Promise<void>;
@@ -66,8 +70,11 @@ function resolveListenPort(): number | undefined {
   return undefined;
 }
 
-function isSupervisedServiceChild(): boolean {
-  return process.env.OCX_SERVICE === "1" && isServiceInstalled();
+function isSupervisedServiceChild(io: SystemRestartIo = {}): boolean {
+  if (process.env.OCX_SERVICE !== "1") return false;
+  // Presence is not enough: stale/missing service assets report installed but will not
+  // respawn after exit(1). Dashboard status/recovery must fall through to detached start.
+  return (io.isServiceViable ?? isServiceViable)();
 }
 
 /** Stable, path-free spawn failure label for logs (never interpolate err.message). */
@@ -137,7 +144,7 @@ export function acceptSystemRestart(io: SystemRestartIo = restartIo): {
     schedule(async () => {
       const drain = io.drainAndShutdown ?? drainAndShutdown;
       await drain(undefined, MEMORY_DRAIN_RESTART_MS);
-      const supervised = (io.isSupervisedServiceChild ?? isSupervisedServiceChild)();
+      const supervised = (io.isSupervisedServiceChild ?? (() => isSupervisedServiceChild(io)))();
       if (supervised) {
         // Failure-only supervisors ignore exit(0); intentional non-zero triggers respawn.
         (io.exitProcess ?? ((code: number) => { process.exit(code); }))(1);

src/server/port-reclaim.ts

@@ -2,8 +2,11 @@
  * Reclaim a listen port after stop/update so restart can stay on the configured
  * port instead of hopping to an ephemeral one (Windows CLOSE_WAIT / leftover ocx).
  *
- * Killing is never the default: a process may be killed only when the caller
- * supplies a non-empty explicit PID allowlist for a process it just stopped.
+ * Killing is never the default. A process may be killed only when the caller
+ * sets `killOcxHolders` and either supplies a non-empty `onlyKillPids` allowlist
+ * (trusted teardown PIDs, including allowlisted holders that fail ocx revalidate)
+ * or enables `killAllOcxOnPort` for revalidated ocx listeners. Unknown foreign
+ * (non-ocx, non-allowlisted) processes are never killed.
  */
 import { execFileSync } from "node:child_process";
 import { verifyPidIdentity } from "../config";
@@ -19,13 +22,24 @@ export type ReclaimListenPortOptions = WaitForPortOptions & {
   /**
    * When true AND `onlyKillPids` is a non-empty allowlist, those PIDs may be
    * killed after revalidation. Default false — never kill without an allowlist.
+   * When {@link killAllOcxOnPort} is also true, any ocx listener on this port
+   * may be killed even if it is not in `onlyKillPids`.
    */
   killOcxHolders?: boolean;
   /**
    * Explicit PIDs the caller just stopped / hard-killed. An omitted or empty
-   * list means no process may be killed.
+   * list means no process may be killed — unless {@link killAllOcxOnPort} is set.
    */
   onlyKillPids?: number[];
+  /**
+   * When true with `killOcxHolders`, every live ocx listener on this port may be
+   * killed (re-checked each scan). Used by post-update restart so a Windows
+   * service wrapper that respawns a *new* bun PID mid-reclaim cannot stay
+   * protected just because it was absent from the pre-wait allowlist snapshot.
+   * Never kills foreign (non-ocx) processes — only allowlisted teardown PIDs
+   * and revalidated ocx listeners.
+   */
+  killAllOcxOnPort?: boolean;
   /**
    * On Windows, force-delete IPv4 TCP rows for this local port via SetTcpEntry.
    * Default true on win32. Never kills foreign processes, never runs while a
@@ -157,9 +171,9 @@ export function listListenPids(port: number): number[] {
 
 /**
  * Wait until `port` can bind.
- * Never kills a process unless `killOcxHolders === true` and `onlyKillPids` is a
- * non-empty allowlist of PIDs the caller itself just stopped — then revalidates
- * immediately before each kill.
+ * Never kills a process unless `killOcxHolders === true` and either
+ * `onlyKillPids` is a non-empty allowlist or `killAllOcxOnPort` is set — then
+ * revalidates immediately before each kill.
  * Never kills foreign processes. Never drops TCP rows while a live foreign or
  * protected ocx listener owns the port, or when the listener scan failed.
  */
@@ -174,7 +188,9 @@ export async function reclaimListenPort(
   const allowedKillPids = new Set(
     (opts.onlyKillPids ?? []).filter(pid => Number.isSafeInteger(pid) && pid > 0),
   );
-  const mayKill = opts.killOcxHolders === true && allowedKillPids.size > 0;
+  const killAllOcx = opts.killAllOcxOnPort === true;
+  const mayKill = opts.killOcxHolders === true
+    && (allowedKillPids.size > 0 || killAllOcx);
   const dropTcpRows = opts.dropTcpRows ?? process.platform === "win32";
   const listFn = opts.listListenPidsFn ?? scanListenPids;
   const isAliveFn = opts.isAliveFn ?? isProcessAlive;
@@ -207,20 +223,44 @@ export async function reclaimListenPort(
 
       for (const pid of scan.pids) {
         if (pid === process.pid) continue;
-        if (!isAliveFn(pid)) continue; // Windows may still list a dead owner briefly
+        if (!isAliveFn(pid)) {
+          // Clear "already tried" so a later respawn that reuses this PID slot
+          // is not skipped (Windows service :loop / npm rename respawns).
+          killed.delete(pid);
+          continue; // Windows may still list a dead owner briefly
+        }
         const isOcx = verifyOcxFn(pid) === pid;
+        const allowlisted = allowedKillPids.has(pid);
+        // Pre-update PIDs can fail verify while still LISTENing (dead owner still
+        // listed, or cmdline probe raced). Allowlisted teardown PIDs may be killed;
+        // unknown foreign claimants must remain fail-closed.
         if (!isOcx) {
+          if (mayKill && allowlisted) {
+            if (!killed.has(pid)) {
+              try {
+                killFn(pid);
+                killed.add(pid);
+              } catch {
+                // Kill failed: never SetTcpEntry while the process may still own the port.
+                protectedOcxListener = true;
+              }
+            }
+            if (!isAliveFn(pid)) killed.delete(pid);
+            else protectedOcxListener = true;
+            continue;
+          }
           foreignLive = true;
           continue;
         }
-        if (!mayKill || !allowedKillPids.has(pid)) {
+        const mayKillThis = allowlisted || killAllOcx;
+        if (!mayKill || !mayKillThis) {
           // Healthy / intentional ocx proxy — never steal its port.
           protectedOcxListener = true;
           continue;
         }
         if (!killed.has(pid)) {
           // Revalidate immediately before termination.
-          if (isAliveFn(pid) && verifyOcxFn(pid) === pid && allowedKillPids.has(pid)) {
+          if (isAliveFn(pid) && verifyOcxFn(pid) === pid && mayKillThis) {
             try {
               killFn(pid);
               killed.add(pid);
@@ -233,8 +273,14 @@ export async function reclaimListenPort(
             protectedOcxListener = true;
           }
         }
-        // Only reset TCP rows after confirmed process death.
-        if (isAliveFn(pid)) protectedOcxListener = true;
+        // Respawning supervisors (Windows service :loop) mint a new PID after each
+        // kill — clear the per-PID "already tried" bit once the process is gone so a
+        // later child with a reused slot is not skipped, and keep reclaiming while live.
+        if (!isAliveFn(pid)) {
+          killed.delete(pid);
+        } else {
+          protectedOcxListener = true;
+        }
       }
 
       if (foreignLive || protectedOcxListener) {

src/server/ports.ts

@@ -16,6 +16,8 @@ export function isAddrInUse(err: unknown): boolean {
 export async function isPortAvailable(port: number, hostname = "127.0.0.1"): Promise<boolean> {
   return await new Promise(resolve => {
     const server = createServer();
+    // Fail closed: EACCES / EADDRNOTAVAIL / EPERM / unknown listen errors mean the
+    // requested bind is not available. Only the listening event reports free.
     server.once("error", () => resolve(false));
     server.once("listening", () => {
       server.close(() => resolve(true));

src/server/windows-tcp-drop.ts

@@ -79,6 +79,12 @@ export type WindowsTcpDropResult = {
   dropped: number;
   /** IPv6 (or unparseable) rows skipped — never coerced into IPv4 wildcards. */
   skippedIpv6: number;
+  /**
+   * SetTcpEntry returned ERROR_ACCESS_DENIED (5) or the UAC-non-elevated code (317).
+   * On a normal (non-admin) update worker this is expected — ghost LISTEN rows must
+   * clear by waiting for the OS, not by SetTcpEntry.
+   */
+  accessDenied: number;
 };
 
 function htons(port: number): number {
@@ -133,21 +139,22 @@ function readNetstatAno(): string {
  */
 export function dropWindowsTcpRowsForLocalPort(port: number): WindowsTcpDropResult {
   if (process.platform !== "win32" || !Number.isFinite(port) || port <= 0) {
-    return { dropped: 0, skippedIpv6: 0 };
+    return { dropped: 0, skippedIpv6: 0, accessDenied: 0 };
   }
   const setTcpEntry = loadSetTcpEntry();
-  if (!setTcpEntry) return { dropped: 0, skippedIpv6: 0 };
+  if (!setTcpEntry) return { dropped: 0, skippedIpv6: 0, accessDenied: 0 };
 
   let output = "";
   try {
     output = readNetstatAno();
   } catch {
-    return { dropped: 0, skippedIpv6: 0 };
+    return { dropped: 0, skippedIpv6: 0, accessDenied: 0 };
   }
 
   const rows = parseTcpQuadsForLocalPort(output, Math.trunc(port));
   let dropped = 0;
   let skippedIpv6 = 0;
+  let accessDenied = 0;
   for (const row of rows) {
     const localDw = ipv4ToWinUint32(row.localAddr);
     const remoteDw = ipv4ToWinUint32(row.remoteAddr);
@@ -165,10 +172,13 @@ export function dropWindowsTcpRowsForLocalPort(port: number): WindowsTcpDropResu
     view.setUint32(12, remoteDw, true);
     view.setUint32(16, htons(row.remotePort), true);
     try {
-      if (setTcpEntry(ptr(buf)) === 0) dropped += 1;
+      const rc = setTcpEntry(ptr(buf));
+      if (rc === 0) dropped += 1;
+      // 5 = ERROR_ACCESS_DENIED, 317 = non-elevated SetTcpEntry (MSDN).
+      else if (rc === 5 || rc === 317) accessDenied += 1;
     } catch {
       /* keep going */
     }
   }
-  return { dropped, skippedIpv6 };
+  return { dropped, skippedIpv6, accessDenied };
 }

src/service.ts

@@ -1862,6 +1862,15 @@ export function isServiceInstalled(): boolean {
   return diagnoseService().installed;
 }
 
+/**
+ * True when an installed background service can actually supervise the proxy.
+ * Presence alone is not enough: stale/missing assets, conflicts, and disabled
+ * registrations report `installed` but will not bring the proxy back after exit.
+ */
+export function isServiceViable(): boolean {
+  return diagnoseService().viable;
+}
+
 export interface ServiceDiagnostic {
   supported: boolean;
   installed: boolean;