ensureBrokerSession() deletes a live broker's state without killing it — the only production caller passes no killProcess

Summary

teardownBrokerSession() guards the kill behind killProcess:

// scripts/lib/broker-lifecycle.mjs:174
if (Number.isFinite(pid) && killProcess) {

ensureBrokerSession() passes options.killProcess ?? null into both of its teardown
branches — but the only production caller passes no killProcess at all:

// scripts/lib/app-server.mjs:344
const brokerSession = await ensureBrokerSession(cwd, { env: options.env });

(grep -rn "ensureBrokerSession(" --include=*.mjs returns exactly this one call site
besides the definition.)

So killProcess is null, the guard is false, and both teardown branches proceed to
unlink pidFile, logFile and sessionDir without terminating anything. The broker
survives, and deleting pidFile destroys the last handle anyone had on it.

This is a plain wiring gap, independent of the state-root split in #636 and of the missing
idle timeout in #108 / #543 — but it produces the same end state, and unlike those it can
fire on a perfectly healthy session.

The path that actually bites: the readiness timeout

ensureBrokerSession() spawns the broker, then waits 2 s for its endpoint:

// broker-lifecycle.mjs:149
const ready = await waitForBrokerEndpoint(endpoint, options.timeoutMs ?? 2000);
if (!ready) {
  teardownBrokerSession({ endpoint, pidFile, logFile, sessionDir, pid: child.pid ?? null,
                          killProcess: options.killProcess ?? null });   // null -> no kill
  return null;
}

If the broker is merely slow, not broken, this deletes its session directory and
returns null. The process then finishes starting a moment later and listens on a pipe
whose name no longer exists anywhere on disk. Nothing can address it, and
ensureBrokerSession() will spawn a fresh one on the next call.

The same applies to the earlier branch (:119-128), where an existing broker that does
not answer within 150 ms has its state removed but is likewise never killed.

Reproduction

Verified on Windows 11 / Node 22.16 / plugin 1.0.6. timeoutMs: 1 stands in for a
loaded machine; killProcess is omitted exactly as app-server.mjs:344 omits it:

const { ensureBrokerSession } = await import("./lib/broker-lifecycle.mjs");
const before = brokerPids();
const s = await ensureBrokerSession(cwd, { env: process.env, timeoutMs: 1 });
await new Promise(r => setTimeout(r, 3000));   // let the broker finish starting
console.log(s, brokerPids().filter(p => !before.includes(p)));

ensureBrokerSession returned: null
new broker processes still alive: 130224
broker.json written: false

=> live process, recorded nowhere

How much headroom does the 2 s actually have?

Measured five broker starts on an otherwise busy machine (14 concurrent Claude Code
sessions):

1413 ms · 1315 ms · 1405 ms · 1366 ms · 1334 ms
median 1366 ms   limit 2000 ms
runs over the limit: 0 of 5

So the timeout did not trip during this measurement — I am not claiming it is the
dominant trigger in practice, only that the margin is about 630 ms on a machine that is
already loaded, and that when it does trip the result is a permanently unreachable
process rather than a retry. Cold start, antivirus on node.exe, or heavier load would
consume that margin.

Impact

On this machine the various leak paths together accumulated 705 orphaned node.exe
processes / 17.8 GB RSS over roughly two days, oldest 31.1 h. Each broker carries a
codex app-server plus its children, so one leaked broker costs about 7 processes.

Of 29 live brokers at one measurement, 7 had their broker.pid already deleted while the
process kept running — the signature this bug produces.

Suggested fix

Pass a real terminator at the call site, and do not delete the pid file before a kill has
been attempted:

// app-server.mjs
const brokerSession = await ensureBrokerSession(cwd, {
  env: options.env,
  killProcess: terminateProcessTree,
});

Better still, default it inside ensureBrokerSession() rather than relying on every
caller to remember — teardownBrokerSession() already ignores a missing process, so a
default is safe. terminateProcessTree() is right there in lib/process.mjs and is what
session-lifecycle-hook.mjs already hands in.

Two smaller hardening notes:

• teardownBrokerSession() could recover the PID from pidFile when the caller has none,
  and should unlink pidFile only after the kill attempt.
• A silent no-op is hard to notice here; having ensureBrokerSession() report that it
  abandoned a live child would have surfaced this much sooner.

Related

• #636 — state root split via CLAUDE_PLUGIN_DATA. Confirmed on Windows too: all 4
  workspaces present in %CLAUDE_PLUGIN_DATA%\state also existed under
  %TEMP%\codex-companion, each with a different, also-running PID. I reproduced the
  duplicate-broker sequence directly rather than inferring it.
• #108, #543 — no idle timeout. An idle self-exit would also reclaim the brokers this bug
  strands.
• #380 — cwd mismatch (different hash); distinct from both of the above.

Note on spawnDetachedTaskWorker

detached: true + unref() on the task workers looks like the obvious suspect but is not
implicated: of 29 orphaned processes, 0 were task workers — they are tracked as jobs
and cleanupSessionJobs() terminates them. The brokers are the leak.

Start at scripts/lib/app-server.mjs:344 and trace into scripts/lib/broker-lifecycle.mjs, especially the teardown branches around lines 119-128 and 149. Re-run the timeoutMs: 1 reproduction on Node/Windows and inspect scripts/lib/process.mjs and session-lifecycle-hook.mjs for the existing terminator. Done means teardown cannot leave a live broker with its state removed, including when readiness times out.