fix: kill a live-but-unreachable broker when replacing it

plugins/codex/scripts/lib/broker-lifecycle.mjs

@@ -8,7 +8,7 @@ import { fileURLToPath } from "node:url";
 import { createBrokerEndpoint, parseBrokerEndpoint } from "./broker-endpoint.mjs";
 import { withBrokerLock } from "./broker-lock.mjs";
 import { probeBroker } from "./broker-probe.mjs";
-import { binaryAvailable, terminateProcessTree } from "./process.mjs";
+import { binaryAvailable, isProcessAlive, terminateProcessTree } from "./process.mjs";
 import { resolveStateDir } from "./state.mjs";
 
 export const PID_FILE_ENV = "CODEX_COMPANION_APP_SERVER_PID_FILE";
@@ -190,11 +190,18 @@ async function loadReusableBrokerSessionUnlocked(cwd, options = {}) {
   }
 
   if (existing) {
-    // Only trust the recorded pid for tree-kill when the endpoint probe confirmed
-    // the broker was actually live. A stale session whose endpoint is not ready
-    // likely points at a dead broker whose pid the OS may have recycled into an
-    // unrelated process — tree-killing there risks killing the wrong process, so
-    // just drop the files and let any survivor exit on its own.
+    // Only trust the recorded pid for tree-kill when either the endpoint probe
+    // confirmed the broker was actually live, or the pid itself is still alive
+    // right now. A stale session whose endpoint is unreachable AND whose pid is
+    // gone likely points at a dead broker whose pid the OS may have recycled
+    // into an unrelated process — tree-killing there risks killing the wrong
+    // process, so just drop the files and let any survivor exit on its own.
+    // But when the pid is still alive, it's the same process continuously
+    // since the state file recorded it (no recycling window exists), so it's
+    // safe to kill even though its endpoint (e.g. a socket file swept by
+    // external tmp cleanup) is no longer reachable — otherwise it's orphaned
+    // for its full idle-timeout window, invisible to every reaper keyed off
+    // the state file the replacement broker is about to overwrite.
     const existingReady = await isBrokerEndpointReady(existing.endpoint);
     if (existingReady) {
       const brokerStatus = await probeBroker(existing.endpoint, cwd);
@@ -210,9 +217,8 @@ async function loadReusableBrokerSessionUnlocked(cwd, options = {}) {
         return null;
       }
     }
-    const killProcess = existingReady
-      ? (options.killProcess ?? terminateProcessTree)
-      : (options.killProcess ?? null);
+    const trustedPid = existingReady || isProcessAlive(existing.pid);
+    const killProcess = trustedPid ? (options.killProcess ?? terminateProcessTree) : (options.killProcess ?? null);
     teardownExistingBroker(cwd, existing, killProcess);
   }
 

tests/broker-lifecycle.test.mjs

@@ -10,10 +10,13 @@ import { buildEnv, installFakeCodex } from "./fake-codex-fixture.mjs";
 import { initGitRepo, makeTempDir } from "./helpers.mjs";
 import { withBrokerLock } from "../plugins/codex/scripts/lib/broker-lock.mjs";
 import {
+  ensureBrokerSession,
   loadBrokerSession,
   loadReusableBrokerSession,
   sendBrokerShutdown
 } from "../plugins/codex/scripts/lib/broker-lifecycle.mjs";
+import { parseBrokerEndpoint } from "../plugins/codex/scripts/lib/broker-endpoint.mjs";
+import { isProcessAlive } from "../plugins/codex/scripts/lib/process.mjs";
 import { resolveStateDir } from "../plugins/codex/scripts/lib/state.mjs";
 
 const ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), "..");
@@ -160,6 +163,44 @@ test("stale reachable brokers are preserved when the broker reports an active tu
   await probeBroker.close();
 });
 
+test("replacing a live broker whose endpoint became unreachable still kills its process", async () => {
+  // Reproduces: broker process is alive and its recorded pid is trustworthy,
+  // but its unix socket file is gone (e.g. swept by external tmp cleanup)
+  // so the readiness probe can't connect within its 150ms budget. That used
+  // to make loadReusableBrokerSessionUnlocked treat the pid as untrustworthy
+  // and skip killProcess entirely, orphaning a live process. It would still
+  // self-exit eventually via its own idle timeout, but stays invisible to
+  // session-lifecycle-hook.mjs (and any other reaper keyed off the state
+  // file) for the whole idle window once the state file is overwritten by
+  // the replacement broker. Use a long idle timeout here so the assertion
+  // below can't pass "by accident" via the broker's own self-shutdown.
+  const repo = makeTempDir();
+  const binDir = makeTempDir();
+  installFakeCodex(binDir);
+  initGitRepo(repo);
+  const env = { ...buildEnv(binDir), CODEX_COMPANION_BROKER_IDLE_TIMEOUT_MS: "600000" };
+
+  const first = await ensureBrokerSession(repo, { env });
+  assert.ok(first, "expected a broker to spawn");
+  assert.equal(isProcessAlive(first.pid), true);
+
+  const target = parseBrokerEndpoint(first.endpoint);
+  fs.unlinkSync(target.path);
+
+  const second = await ensureBrokerSession(repo, { env });
+  assert.ok(second, "expected a replacement broker to spawn");
+  assert.notEqual(second.pid, first.pid);
+
+  const deadline = Date.now() + 2000;
+  while (isProcessAlive(first.pid) && Date.now() < deadline) {
+    await new Promise((resolve) => setTimeout(resolve, 20));
+  }
+
+  assert.equal(isProcessAlive(first.pid), false, "orphaned broker process should have been killed");
+
+  await sendBrokerShutdown(second.endpoint);
+});
+
 test("broker shutdown accepts a response split across socket chunks", async () => {
   const sessionDir = makeTempDir();
   const socketPath = path.join(sessionDir, "broker.sock");