Merge pull request lidge-jun#805 from lidge-jun/fix/764-winsw-env-and-liveness-probe

Wibias · web-flow · commit b1f33ab8e17c · 2026-07-31T10:26:55.000+02:00
fix(service): bake WinSW admin env and retry stop-path probes
diff --git a/src/lib/winsw.ts b/src/lib/winsw.ts
@@ -87,12 +87,19 @@ export function buildWinswXml(entry: WinswEntry, env: NodeJS.ProcessEnv = proces
   })();
   // Services never bake `--port 0` (parsePortOption rejects it); treat as default.
   const safeListenPort = listenPort > 0 && listenPort <= 65535 ? listenPort : 10100;
+  // SCM services do not inherit the interactive user environment (#764). Bake:
+  // - OPENCODEX_HOME so file-backed admin auth (`admin-api-token`) resolves
+  // - OPENCODEX_ACL_TIMEOUT_MS when set (not a secret)
+  // Never embed OPENCODEX_ADMIN_AUTH_TOKEN or OPENCODEX_API_AUTH_TOKEN values in XML —
+  // those stay file-pointer / generated-file only (uninstall retains the XML).
+  const aclTimeout = env.OPENCODEX_ACL_TIMEOUT_MS?.trim();
   const envLines = [
     `  <env name="OCX_SERVICE" value="1"/>`,
     `  <env name="OCX_API_TOKEN_FILE" value="${xmlEscape(serviceApiTokenFilePath())}"/>`,
     `  <env name="PATH" value="${xmlEscape(env.PATH ?? "")}"/>`,
     env.CODEX_HOME?.trim() ? `  <env name="CODEX_HOME" value="${xmlEscape(currentCodexHomeAbsolute())}"/>` : null,
-    env.OPENCODEX_HOME?.trim() ? `  <env name="OPENCODEX_HOME" value="${xmlEscape(getConfigDir())}"/>` : null,
+    `  <env name="OPENCODEX_HOME" value="${xmlEscape(getConfigDir())}"/>`,
+    aclTimeout ? `  <env name="OPENCODEX_ACL_TIMEOUT_MS" value="${xmlEscape(aclTimeout)}"/>` : null,
   ].filter((line): line is string => Boolean(line));
   return `<?xml version="1.0" encoding="UTF-8"?>
 <service>
diff --git a/src/server/proxy-liveness.ts b/src/server/proxy-liveness.ts
@@ -30,8 +30,29 @@ export interface LivenessIo {
   readRuntimeFn?: (pid?: number) => { pid?: number; port: number; hostname?: string } | null;
   configFn?: () => { port?: number; hostname?: string };
   timeoutMs?: number;
+  /**
+   * How many times to retry a probe that failed with a transport error (timeout /
+   * connection refused). Definitive answers (non-OK HTTP, foreign /healthz body, pid
+   * mismatch) do not retry. Default 1 = no retry. Stop paths should pass 2–3 (#764).
+   */
+  attempts?: number;
+  sleepFn?: (ms: number) => Promise<void>;
+  /**
+   * Absolute wall-clock deadline for discovery. When set, each probe attempt aborts
+   * once the remaining budget cannot cover another fetch — so multi-candidate
+   * `findLiveProxy` under `SERVICE_STOP_LIVENESS` cannot overrun the stop-path
+   * verification window (#764 / CodeRabbit).
+   */
+  deadlineAt?: number;
+  nowFn?: () => number;
 }
 
+/** Default probe options for service stop / orphan cleanup — a just-bound proxy can miss a single 750ms probe. */
+export const SERVICE_STOP_LIVENESS: Pick<LivenessIo, "timeoutMs" | "attempts"> = {
+  timeoutMs: 1500,
+  attempts: 3,
+};
+
 export interface LiveProxy {
   pid: number | null;
   port: number;
@@ -72,19 +93,37 @@ export async function proxyIdentityAt(
   io: LivenessIo = {},
 ): Promise<{ pid: number | null } | null> {
   const fetchFn = io.fetchFn ?? fetch;
-  try {
-    const res = await fetchFn(`http://${probeHostname(opts.hostname)}:${port}/healthz`, {
-      signal: AbortSignal.timeout(io.timeoutMs ?? 750),
-    });
-    if (!res.ok) return null;
-    const body = (await res.json().catch(() => null)) as HealthzIdentity | null;
-    if (!isOpencodexHealthz(body)) return null;
-    const pid = typeof body?.pid === "number" ? body.pid : null;
-    if (opts.expectedPid !== undefined && pid !== null && pid !== opts.expectedPid) return null;
-    return { pid };
-  } catch {
-    return null;
+  const sleepFn = io.sleepFn ?? ((ms: number) => new Promise<void>(r => setTimeout(r, ms)));
+  const nowFn = io.nowFn ?? Date.now;
+  const baseTimeoutMs = io.timeoutMs ?? 750;
+  const requestedAttempts = Math.trunc(io.attempts ?? 1);
+  const attempts = Number.isNaN(requestedAttempts)
+    ? 1
+    : Math.max(1, Math.min(requestedAttempts, 5));
+
+  for (let attempt = 1; attempt <= attempts; attempt++) {
+    const remainingMs = io.deadlineAt === undefined ? baseTimeoutMs : io.deadlineAt - nowFn();
+    if (remainingMs <= 0) return null;
+    const timeoutMs = Math.min(baseTimeoutMs, remainingMs);
+    try {
+      const res = await fetchFn(`http://${probeHostname(opts.hostname)}:${port}/healthz`, {
+        signal: AbortSignal.timeout(timeoutMs),
+      });
+      if (!res.ok) return null;
+      const body = (await res.json().catch(() => null)) as HealthzIdentity | null;
+      if (!isOpencodexHealthz(body)) return null;
+      const pid = typeof body?.pid === "number" ? body.pid : null;
+      if (opts.expectedPid !== undefined && pid !== null && pid !== opts.expectedPid) return null;
+      return { pid };
+    } catch {
+      // Transport failure (timeout / refused) — retry while budget remains; a proxy that
+      // has only just begun listening can miss a single short probe (#764).
+      if (attempt >= attempts) return null;
+      if (io.deadlineAt !== undefined && io.deadlineAt - nowFn() <= 0) return null;
+      await sleepFn(100);
+    }
   }
+  return null;
 }
 
 /**
diff --git a/src/service.ts b/src/service.ts
@@ -6,7 +6,7 @@
  * restore it via the command.
  */
 import { execFileSync, execSync } from "node:child_process";
-import { findLiveProxy } from "./server/proxy-liveness";
+import { findLiveProxy, SERVICE_STOP_LIVENESS } from "./server/proxy-liveness";
 import { chmodSync, existsSync, mkdirSync, readFileSync, unlinkSync, writeFileSync } from "node:fs";
 import { homedir } from "node:os";
 import { dirname, join, resolve } from "node:path";
@@ -1723,11 +1723,18 @@ export async function proxyStillLiveAfterStop(deps: {
   /** Whether the stopped supervisor can respawn its child; only then is polling worth the wait. */
   canRespawn?: boolean;
 } = {}): Promise<{ port: number } | null> {
-  const findProxy = deps.findProxy ?? findLiveProxy;
   const sleep = deps.sleep ?? ((ms: number) => new Promise<void>(r => setTimeout(r, ms)));
   const now = deps.now ?? Date.now;
   const canRespawn = deps.canRespawn ?? process.platform === "win32";
   const deadline = now() + (canRespawn ? 7000 : 0);
+  // Single-shot (non-respawn) still needs one full SERVICE_STOP_LIVENESS budget; respawn
+  // polling shares the outer deadline so multi-candidate discovery cannot overrun it.
+  const findProxy = deps.findProxy ?? (() => {
+    const probeDeadline = canRespawn
+      ? deadline
+      : now() + (SERVICE_STOP_LIVENESS.timeoutMs! * SERVICE_STOP_LIVENESS.attempts! + 250);
+    return findLiveProxy({ ...SERVICE_STOP_LIVENESS, deadlineAt: probeDeadline, nowFn: now });
+  });
   for (;;) {
     try {
       const live = await findProxy();
@@ -1755,7 +1762,11 @@ async function stopTrackedProxyIfRunning(): Promise<TrackedProxyCleanupResult> {
   }
   // Orphan recovery: the pid file can be missing/stale while the service wrapper keeps
   // a live proxy running — mirror `ocx stop`'s identity-checked findLiveProxy fallback.
-  const live = await findLiveProxy({ timeoutMs: 1500 });
+  // Cap multi-candidate discovery so stop cleanup cannot hang for three full retry budgets.
+  const live = await findLiveProxy({
+    ...SERVICE_STOP_LIVENESS,
+    deadlineAt: Date.now() + 7000,
+  });
   const liveKillPid = verifiedKillTarget(live?.pid);
   if (liveKillPid !== null) {
     await stopProxy(liveKillPid);
diff --git a/tests/api-storage-cleanup.test.ts b/tests/api-storage-cleanup.test.ts
@@ -1,4 +1,4 @@
-import { afterEach, beforeEach, describe, expect, test } from "bun:test";
+import { afterEach, beforeEach, describe, expect, setDefaultTimeout, test } from "bun:test";
 import { managementFetch as fetch } from "./helpers/management-auth";
 import { Database } from "bun:sqlite";
 import { existsSync, mkdirSync, mkdtempSync, rmSync, utimesSync, writeFileSync } from "node:fs";
@@ -9,6 +9,10 @@ import { startServer } from "../src/server";
 import type { OcxConfig } from "../src/types";
 import { installIsolatedCodexHome, type IsolatedCodexHome } from "./helpers/isolated-codex-home";
 
+// Windows CI under load can spend >5s just binding the proxy + previewing cleanup;
+// Bun's default test budget then fails the suite before the assertion runs.
+setDefaultTimeout(20_000);
+
 let testDir = "";
 let previousHome: string | undefined;
 let isolatedCodexHome: IsolatedCodexHome | null = null;
diff --git a/tests/proxy-liveness.test.ts b/tests/proxy-liveness.test.ts
@@ -51,6 +51,70 @@ describe("proxyIdentityAt", () => {
     expect(await proxyIdentityAt(10100, { expectedPid: 1 }, { fetchFn: (async () => healthz(OURS)) as typeof fetch })).toBeNull();
     expect(await proxyIdentityAt(10100, {}, { fetchFn: (async () => { throw new Error("refused"); }) as typeof fetch })).toBeNull();
   });
+
+  test("retries transport failures and succeeds on a later attempt (#764)", async () => {
+    let calls = 0;
+    const sleeps: number[] = [];
+    const identity = await proxyIdentityAt(10100, {}, {
+      attempts: 3,
+      timeoutMs: 50,
+      sleepFn: async (ms) => { sleeps.push(ms); },
+      fetchFn: (async () => {
+        calls += 1;
+        if (calls < 3) throw new Error("timeout");
+        return healthz(OURS);
+      }) as typeof fetch,
+    });
+    expect(identity).toEqual({ pid: 4242 });
+    expect(calls).toBe(3);
+    expect(sleeps).toEqual([100, 100]);
+  });
+
+  test("does not retry a definitive foreign /healthz body", async () => {
+    let calls = 0;
+    const identity = await proxyIdentityAt(10100, {}, {
+      attempts: 3,
+      fetchFn: (async () => {
+        calls += 1;
+        return healthz({ ok: true });
+      }) as typeof fetch,
+    });
+    expect(identity).toBeNull();
+    expect(calls).toBe(1);
+  });
+
+  test("NaN attempts fall back to a single probe", async () => {
+    let calls = 0;
+    const identity = await proxyIdentityAt(10100, {}, {
+      attempts: Number.NaN,
+      fetchFn: (async () => {
+        calls += 1;
+        return healthz(OURS);
+      }) as typeof fetch,
+    });
+    expect(identity).toEqual({ pid: 4242 });
+    expect(calls).toBe(1);
+  });
+
+  test("honors an aggregate deadline across transport retries", async () => {
+    let calls = 0;
+    let clock = 1_000;
+    const identity = await proxyIdentityAt(10100, {}, {
+      attempts: 3,
+      timeoutMs: 1_500,
+      deadlineAt: 1_000 + 1_200,
+      nowFn: () => clock,
+      sleepFn: async () => { clock += 100; },
+      fetchFn: (async () => {
+        calls += 1;
+        clock += 1_500;
+        throw new Error("timeout");
+      }) as typeof fetch,
+    });
+    expect(identity).toBeNull();
+    // First attempt spends the budget; remaining retries must not fire.
+    expect(calls).toBe(1);
+  });
 });
 
 describe("findLiveProxy", () => {
diff --git a/tests/service.test.ts b/tests/service.test.ts
@@ -661,10 +661,12 @@ describe("service lifecycle cleanup ordering", () => {
     expect(service).toContain('verifyPidIdentity');
     expect(service).toContain("removeRuntimePort(pid);");
     expect(service).toContain('import { isProcessAlive, stopProxy } from "./lib/process-control";');
-    expect(service).toContain('import { findLiveProxy } from "./server/proxy-liveness";');
+    expect(service).toContain('import { findLiveProxy, SERVICE_STOP_LIVENESS } from "./server/proxy-liveness";');
     expect(service).toContain('type TrackedProxyCleanupResult = "none" | "stale" | "stopped";');
     expect(service).toContain("async function stopTrackedProxyIfRunning(): Promise<TrackedProxyCleanupResult>");
-    expect(service).toContain("await findLiveProxy({ timeoutMs: 1500 })");
+    expect(service).toContain("...SERVICE_STOP_LIVENESS");
+    expect(service).toContain("deadlineAt:");
+    expect(service).toContain("SERVICE_STOP_LIVENESS");
     expect(service).toContain("await stopProxy(trackedKillPid);");
     expect(service).toContain("await stopProxy(liveKillPid);");
     expect(service).toContain("removePid(pid);");
diff --git a/tests/winsw.test.ts b/tests/winsw.test.ts
@@ -2,12 +2,23 @@ import { describe, expect, test } from "bun:test";
 import { buildWinswXml, ensureWinswBinary, parseWinswStatus, probeScmRegistration, sha256Hex, installWinswService, statusWinswRaw, WINSW_SHA256, WINSW_SERVICE_ID } from "../src/lib/winsw";
 import { parseServiceArgs, serviceReinstallArgs } from "../src/service";
 import { loadServiceTokenFromFile } from "../src/lib/service-secrets";
+import { getConfigDir } from "../src/config";
 import { mkdtempSync, readFileSync, writeFileSync, rmSync } from "node:fs";
 import { tmpdir } from "node:os";
 import { join } from "node:path";
 
 const entry = { bun: "C:\\OpenCodex\\bun.exe", cli: "C:\\Open Codex\\cli & co\\index.ts" };
 
+function winswEnvValue(xml: string, name: string): string | null {
+  const match = xml.match(new RegExp(`<env name="${name}" value="([^"]*)"/>`));
+  if (!match) return null;
+  return match[1]!
+    .replace(/&amp;/g, "&")
+    .replace(/&lt;/g, "<")
+    .replace(/&gt;/g, ">")
+    .replace(/&quot;/g, '"');
+}
+
 describe("winsw xml", () => {
   const env = { USERDOMAIN: "WORKGROUP", USERNAME: "jun", PATH: "C:\\bin;C:\\tools & more" } as NodeJS.ProcessEnv;
 
@@ -30,8 +41,26 @@ describe("winsw xml", () => {
     expect(xml).toContain('<env name="OCX_SERVICE" value="1"/>');
     expect(xml).toContain('<env name="OCX_API_TOKEN_FILE"');
     expect(xml).toContain('<env name="PATH" value="C:\\bin;C:\\tools &amp; more"/>');
-    // The token VALUE never lands in the XML — only the file pointer.
+    expect(winswEnvValue(xml, "OPENCODEX_HOME")).toBe(getConfigDir());
+    // Token VALUES never land in the XML — only file pointers / non-secret budgets.
+    expect(xml).not.toContain("OPENCODEX_API_AUTH_TOKEN");
+    expect(xml).not.toContain("OPENCODEX_ADMIN_AUTH_TOKEN");
+  });
+
+  test("bakes install-time ACL timeout and never embeds the admin token (#764)", () => {
+    const xml = buildWinswXml(entry, {
+      ...env,
+      OPENCODEX_API_AUTH_TOKEN: "api-secret-value",
+      OPENCODEX_ADMIN_AUTH_TOKEN: "admin-secret & more",
+      OPENCODEX_ACL_TIMEOUT_MS: "10000",
+    });
+
+    expect(xml).toContain('<env name="OPENCODEX_ACL_TIMEOUT_MS" value="10000"/>');
+    expect(winswEnvValue(xml, "OPENCODEX_HOME")).toBe(getConfigDir());
+    expect(xml).not.toContain("OPENCODEX_ADMIN_AUTH_TOKEN");
     expect(xml).not.toContain("OPENCODEX_API_AUTH_TOKEN");
+    expect(xml).not.toContain("admin-secret");
+    expect(xml).not.toContain("api-secret-value");
   });
 
   test("escapes executable/arguments and configures restart + graceful stop", () => {
