From 74b281309e500db70f2d71f61ef1b36c6cc65a1d Mon Sep 17 00:00:00 2001 From: axisrow Date: Mon, 3 Aug 2026 10:16:59 +0800 Subject: [PATCH] fix: kill a live-but-unreachable broker when replacing it Investigated upstream openai/codex-plugin-cc#580 ("give the app-server broker an idle timeout, and kill the one it replaces") for portability to this fork. Two independent defects were claimed; verified each on macOS against this fork's own broker-lifecycle.mjs/broker-controller.mjs rather than trusting the diff. Defect 1 (no idle timeout): NOT reproduced as a gap here. This fork's in-process BrokerController (lib/broker-controller.mjs) already ships a causal, test-driven idle-timeout mechanism (CODEX_COMPANION_BROKER_IDLE_TIMEOUT_MS, default 15 min) predating this investigation, architected around an injected clock rather than the broker polling its own socket count on a wall-clock interval. No port needed. Defect 2 (stale-replacement path never kills the process it replaces): partially already fixed here (478be42 already tree-kills a just-spawned broker that never becomes ready). The remaining gap is narrower than upstream's diff implies: loadReusableBrokerSessionUnlocked only tree-kills the recorded pid when the readiness probe confirms the endpoint is live, out of deliberate caution about killing a recycled pid. When the endpoint is unreachable (e.g. the broker's unix socket file is swept by external tmp cleanup while the process itself is still running -- reproduced locally by deleting the socket file out from under a spawned broker) that pid was never trusted, so the process is orphaned for its full idle-timeout window and invisible to session-lifecycle-hook.mjs once the state file is overwritten by the replacement broker. Fix: trust the pid for tree-kill when either the probe succeeded OR the pid is alive right now (isProcessAlive). A live pid recorded in the state file has been the same process continuously since it was written -- there is no recycling window to guard against -- so killing it is as safe as the existing endpoint-confirmed path. A dead/absent pid still skips the kill, preserving the original PID-reuse guard for the case it actually protects. Did not port upstream's widened stale-probe retry (150ms -> 150ms + 1s): this fork already serializes ensureBrokerSession via a file lock (broker-lock.mjs), so concurrent-load probe races that upstream's retry targets don't reproduce here (confirmed empirically). Did not touch upstream#577 (Windows Git Bash/taskkill fix) -- irrelevant on macOS. Added a regression test that spawns a real broker, deletes its socket file, and asserts the replacement path still terminates the orphaned process (using a long idle timeout so the assertion can't pass "by accident" via the broker's own self-shutdown). Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01HbkiKZR4w8hZUmNNTdb6kB --- .../codex/scripts/lib/broker-lifecycle.mjs | 24 +++++++---- tests/broker-lifecycle.test.mjs | 41 +++++++++++++++++++ 2 files changed, 56 insertions(+), 9 deletions(-) diff --git a/plugins/codex/scripts/lib/broker-lifecycle.mjs b/plugins/codex/scripts/lib/broker-lifecycle.mjs index 003110f7..886e5d2c 100644 --- a/plugins/codex/scripts/lib/broker-lifecycle.mjs +++ b/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); } diff --git a/tests/broker-lifecycle.test.mjs b/tests/broker-lifecycle.test.mjs index dcbd18b7..67864da2 100644 --- a/tests/broker-lifecycle.test.mjs +++ b/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");