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");