From 1b3264f9aca1bd6158848f9d3b1f451630c7dd62 Mon Sep 17 00:00:00 2001 From: srdrkr Date: Mon, 27 Jul 2026 19:09:04 -0700 Subject: [PATCH] fix(broker): terminate the broker on teardown instead of only unlinking its pid file The broker is spawned detached and unref'd so it can outlive the process that started it. teardownBrokerSession unlinks the pid file, the log and the endpoint socket, but only terminated the process when a caller injected killProcess. Its default was null. session-lifecycle-hook passes terminateProcessTree, so session end was fine. ensureBrokerSession did not: both of its teardown paths passed options.killProcess ?? null, and its only caller (app-server.mjs) passes just { env }. So every stale-session replacement and every spawn that misses its readiness window unlinked the pid file and left the broker running, unreachable, because nothing else knows the pid. Observed on one workstation: 206 orphaned app-server-broker.mjs processes, PPID 1, oldest cohort 7 to 9 days, one per invocation. They accumulate until process and pty exhaustion. Default killProcess to terminateProcessTree in both places. Injection still works, so existing tests and the session hook are unaffected. --- .../codex/scripts/lib/broker-lifecycle.mjs | 11 +- tests/broker-lifecycle.test.mjs | 111 ++++++++++++++++++ 2 files changed, 119 insertions(+), 3 deletions(-) create mode 100644 tests/broker-lifecycle.test.mjs diff --git a/plugins/codex/scripts/lib/broker-lifecycle.mjs b/plugins/codex/scripts/lib/broker-lifecycle.mjs index ef763819c..92663a2d7 100644 --- a/plugins/codex/scripts/lib/broker-lifecycle.mjs +++ b/plugins/codex/scripts/lib/broker-lifecycle.mjs @@ -7,6 +7,7 @@ import { spawn } from "node:child_process"; import { fileURLToPath } from "node:url"; import { createBrokerEndpoint, parseBrokerEndpoint } from "./broker-endpoint.mjs"; import { resolveStateDir } from "./state.mjs"; +import { terminateProcessTree } from "./process.mjs"; export const PID_FILE_ENV = "CODEX_COMPANION_APP_SERVER_PID_FILE"; export const LOG_FILE_ENV = "CODEX_COMPANION_APP_SERVER_LOG_FILE"; @@ -123,7 +124,7 @@ export async function ensureBrokerSession(cwd, options = {}) { logFile: existing.logFile ?? null, sessionDir: existing.sessionDir ?? null, pid: existing.pid ?? null, - killProcess: options.killProcess ?? null + killProcess: options.killProcess ?? terminateProcessTree }); clearBrokerSession(cwd); } @@ -154,7 +155,7 @@ export async function ensureBrokerSession(cwd, options = {}) { logFile, sessionDir, pid: child.pid ?? null, - killProcess: options.killProcess ?? null + killProcess: options.killProcess ?? terminateProcessTree }); return null; } @@ -170,7 +171,11 @@ export async function ensureBrokerSession(cwd, options = {}) { return session; } -export function teardownBrokerSession({ endpoint = null, pidFile, logFile, sessionDir = null, pid = null, killProcess = null }) { +export function teardownBrokerSession({ endpoint = null, pidFile, logFile, sessionDir = null, pid = null, killProcess = terminateProcessTree }) { + // The broker is spawned detached and unref'd so it can outlive the process that + // started it. That makes termination here mandatory rather than optional: this + // function also unlinks the pid file, so a broker left running after its pid file + // is gone can never be found again by anything. if (Number.isFinite(pid) && killProcess) { try { killProcess(pid); diff --git a/tests/broker-lifecycle.test.mjs b/tests/broker-lifecycle.test.mjs new file mode 100644 index 000000000..637a2c887 --- /dev/null +++ b/tests/broker-lifecycle.test.mjs @@ -0,0 +1,111 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { execFileSync } from "node:child_process"; + +import { + ensureBrokerSession, + teardownBrokerSession +} from "../plugins/codex/scripts/lib/broker-lifecycle.mjs"; + +// The broker is spawned detached and unref'd, so it outlives the process that +// started it by design. Teardown also unlinks the pid file. If teardown removes +// the pid file without terminating the process, the broker is unreachable +// forever: nothing knows its pid and nothing will ever reap it. These tests pin +// that termination happens by default rather than only when a caller remembers +// to inject a killer. + +function tempSessionFiles() { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "cxc-test-")); + const pidFile = path.join(dir, "broker.pid"); + const logFile = path.join(dir, "broker.log"); + fs.writeFileSync(pidFile, "4242"); + fs.writeFileSync(logFile, ""); + return { dir, pidFile, logFile }; +} + +test("teardownBrokerSession terminates the broker when no killer is injected", () => { + const { pidFile, logFile } = tempSessionFiles(); + const killed = []; + + teardownBrokerSession({ + pidFile, + logFile, + pid: 4242, + killProcess: (pid) => killed.push(pid) + }); + + assert.deepEqual(killed, [4242], "an injected killer must still be honoured"); + assert.equal(fs.existsSync(pidFile), false, "pid file is removed"); +}); + +test("teardownBrokerSession does not unlink the pid file while leaving the process alive", () => { + // The regression: the default was `killProcess = null`, so the guard + // `Number.isFinite(pid) && killProcess` was false and nothing was terminated, + // while the pid file was unlinked regardless. Reproduce the shape by asserting + // the default parameter is a callable rather than null. + const source = fs.readFileSync( + new URL("../plugins/codex/scripts/lib/broker-lifecycle.mjs", import.meta.url), + "utf8" + ); + + assert.match( + source, + /killProcess = terminateProcessTree/, + "teardownBrokerSession must default killProcess to a real terminator, not null" + ); + assert.doesNotMatch( + source, + /killProcess: options\.killProcess \?\? null/, + "ensureBrokerSession must not coerce an absent injection to null; that orphans the broker it just tore down" + ); +}); + +test("ensureBrokerSession does not orphan a broker that never becomes ready", async () => { + // Behavioural, and deliberately injects nothing: injecting a killer would + // exercise the path that always worked. This spawns a REAL process that never + // opens the endpoint, so readiness fails and teardown runs with whatever the + // default is. Before the fix that default was null and this process survived + // with its pid file already unlinked, which is the leak. + const cwd = fs.mkdtempSync(path.join(os.tmpdir(), "cxc-cwd-")); + const stubPath = path.join(cwd, "never-ready-broker.mjs"); + fs.writeFileSync(stubPath, "setTimeout(() => {}, 30_000);\n"); + + const session = await ensureBrokerSession(cwd, { + timeoutMs: 250, + scriptPath: stubPath + // The real endpoint factory is used deliberately: overriding it with a bare + // path makes parseBrokerEndpoint throw before teardown is ever reached, so + // the test would fail for an unrelated reason in both directions. + }); + + assert.equal(session, null, "a broker that never becomes ready yields no session"); + + // The pid file is unlinked by teardown, so recover the pid from the process + // table instead: any surviving stub is by definition an orphan. + let survivors = []; + try { + const out = execFileSync("pgrep", ["-f", stubPath], { encoding: "utf8" }); + survivors = out.trim().split("\n").filter(Boolean); + } catch { + // pgrep exits non-zero when nothing matches, which is the passing case. + } + + try { + assert.equal( + survivors.length, + 0, + `teardown left ${survivors.length} broker process(es) running after unlinking the pid file` + ); + } finally { + for (const pid of survivors) { + try { + process.kill(Number(pid), "SIGKILL"); + } catch { + // Already gone. + } + } + } +});