fix(broker): terminate the broker on teardown instead of only unlinking its pid file

[srdrkr]

The leak

The broker is spawned detached: true and unref()'d so it can outlive the process that started it. That is correct for a broker, but it makes explicit termination mandatory.

teardownBrokerSession unlinks the pid file, the log file and the endpoint socket. It only terminates the process when a caller injects killProcess, whose default was null:

export function teardownBrokerSession({ ..., pid = null, killProcess = null }) {
  if (Number.isFinite(pid) && killProcess) {   // false when nothing is injected
    killProcess(pid);
  }
  // ... pid file, log file and socket are unlinked unconditionally

session-lifecycle-hook.mjs does pass terminateProcessTree, so session end was fine. ensureBrokerSession does not. Both of its teardown paths pass options.killProcess ?? null, and its only caller (app-server.mjs:344) passes just { env: options.env }.

So two paths leaked:

1. Stale-session replacement — an existing session whose endpoint is not ready is torn down before a new one is spawned. The old broker survives.
2. Spawn readiness timeout — a broker that does not open its endpoint within the timeout is torn down and null is returned. The broker it just spawned survives.

In both cases the pid file is deleted while the process keeps running, so nothing can ever find it again. There is no reaper and no age-based sweep.

Observed impact

On one workstation, after roughly a week of ordinary use:

• 206 orphaned app-server-broker.mjs processes, all PPID 1, oldest cohort 7 to 9 days old, one per invocation
• ~1050 total user processes, 419 of them node
• New process spawns began failing with fork failed: Device not configured

Terminating just those 206 dropped the machine from 1047 processes to 565, since each broker also held child processes. Likely a contributor to battery drain as well.

The fix

Default killProcess to terminateProcessTree in teardownBrokerSession, and stop coercing an absent injection to null at both ensureBrokerSession call sites. Injection still works unchanged, so the session hook and existing callers are unaffected.

Eight lines changed in one file.

Tests

tests/broker-lifecycle.test.mjs, three cases. The behavioural one spawns a real stub process that never opens its endpoint, lets readiness time out, and asserts nothing survives afterwards. It deliberately injects no killer, because injecting one exercises the path that always worked.

Verified A/B/A against the unfixed tree:

unfixed:  teardown left 1 broker process(es) running after unlinking the pid file
fixed:    3 pass, 0 fail
unfixed:  fails again

Full suite on this branch: 90 pass, 4 fail. Clean main at db52e28: 88 pass, 6 fail. The same four failures are present on main and are unrelated to this change (status shows phases…, status preserves adversarial review kind labels, result returns the stored output…, resolveStateDir uses a temp-backed per-workspace directory). This branch fixes two and breaks none.

Note on scope

This stops new orphans. It does not reap ones already on disk from earlier versions, which have to be cleared by hand. If you would like a startup sweep for pre-existing strays, I am happy to add it in a separate PR rather than widen this one.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1b3264f9ac

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

Verify stale broker PID before terminating it

When broker.json outlives the broker (for example after a crash, SIGKILL, or stale state under CLAUDE_PLUGIN_DATA) and the OS later reuses existing.pid, this new default terminator runs solely because the old endpoint is not ready. terminateProcessTree sends SIGTERM to -pid/pid, so the next plugin command can kill an unrelated process or process group before clearing the stale session; validate that the pid file still exists and matches the stored pid (or otherwise verify the process) before killing, and just clear stale state when it cannot be validated.

Useful? React with 👍 / 👎.

[srdrkr]

I'm a lowly pm, hire me please!

[afspies]

Bumping this — we're independently affected and would like to see it land. On one macOS machine the missing default kill (plus the cwd-keyed lookup in #380) had accumulated 25 orphaned brokers, 1.31 GB RSS, oldest 15 days, 15 of them owned by no live session.

On the bot's P2, "Verify stale broker PID before terminating it" — that risk is real, but the suggested remedy ("validate that the pid file still exists and matches the stored") is unreliable in practice, at least on macOS.

broker.pid disappears on its own. We found it missing from every cxc-* session dir older than ~3 days while broker.sock and broker.log were still present. Cause is macOS periodic cleanup of untouched files under /var/folders/.../T (confirmed via atime/mtime — the pidfile is never touched after creation, the socket is). Teardown reads the pid from broker.json rather than the pidfile, so this isn't a leak cause today, but it does mean a pidfile-existence check would refuse to terminate exactly the long-lived stale brokers this PR is meant to reap, on any machine that has been up a few days.

An alternative that doesn't depend on the pidfile: match the --endpoint path in the process's own argv. It's unique per broker (mkdtemp), so it identifies this instance rather than merely "a broker" — which matters, because a script-name-only check would happily accept a different live broker that inherited the recycled PID, and killing that drops another session's in-flight request.

Two details that turned out to matter:

1. Make it tri-state and fail closed. If ps itself fails, signal nothing and delete no tracking files, so a later teardown can retry. Deleting the pidfile/socket/session dir while failing to signal is precisely what converts a live broker into an untracked orphan — the bug this PR fixes — so an unverifiable pid should leave everything reapable rather than half-cleaned.
2. Windows can't do this cheaply. argv isn't readily readable there, so it stays permissive: no worse than current behaviour, but it cannot detect a recycled PID. Worth a comment rather than silence, given Windows: zombie broker + codex app-server trees accumulate — broker/shutdown acks before unbounded cleanup, and taskkill tree-kill breaks under Git Bash SHELL #416/Windows: broker child processes leak and stale broker.json poisons setup tests on persistent machines #487.

// "match" | "mismatch" | "unknown" — callers must treat "unknown" as fail-closed.
export function verifyBrokerProcess(pid, endpoint, platform = process.platform) {
  if (!Number.isFinite(pid)) return "mismatch";
  if (platform === "win32") return "unknown-permissive";

  const result = runCommand("ps", ["-p", String(pid), "-o", "command="]);
  if (result.error) return "unknown";          // ps unavailable: do not guess
  if (result.status !== 0) return "mismatch";  // no such process: definitively gone

  const command = result.stdout;
  if (!command.includes("app-server-broker.mjs")) return "mismatch";
  if (endpoint) {
    const socketPath = endpoint.startsWith("unix:") ? endpoint.slice("unix:".length) : endpoint;
    return command.includes(socketPath) ? "match" : "mismatch";  // not just *a* broker
  }
  return "match";
}

Then in teardownBrokerSession, unknown returns early without touching any file, and only match (or the Windows permissive case) reaches killProcess(pid).

We've been running this together with idle self-termination (#450/#457/#490) and session-id teardown (#380/#381) locally for a day, with a regression harness covering the PID-identity case specifically: two live brokers, teardown called with broker A's pid and broker B's endpoint, asserting A survives. Happy to open a PR against this branch with the guard and that test if useful.

---

Written by Claude (Claude Code) on behalf of @afspies, who reviewed and sent it. The measurements and file paths above are from a real machine, not inferred.