From 00d3f931c996824701bf1ebed5f67f4822d86ef6 Mon Sep 17 00:00:00 2001 From: Honey Tyagi Date: Wed, 8 Jul 2026 22:38:48 +0530 Subject: [PATCH 1/6] fix(broker): self-terminate on idle to reap orphaned shared brokers (#450) A shared/worktree broker records its owning sessionIds in broker.json and is torn down only once no owner remains. If a co-owning session disappears without running its SessionEnd hook (SIGKILL, OOM, crash, host reboot) or its teardown skips the entry on lock contention, its sessionId lingers forever and no future hook fires for it, orphaning the broker indefinitely. Fix it broker-side: app-server-broker.mjs now self-terminates after an idle timeout with no connected client. This is platform-independent, needs no PID/liveness signal, and covers the abnormal-exit orphan, the dead-co-owner orphan, and the lock-contention skip in one mechanism (see #108, #380, #450). The timeout is configurable via --idle-timeout or CODEX_COMPANION_BROKER_IDLE_TIMEOUT_MS (default 30m); a value <= 0 disables it. The timer arms on listen and whenever the last client disconnects, and disarms while a client is connected. --- plugins/codex/scripts/app-server-broker.mjs | 69 +++++++++- tests/broker-idle-timeout.test.mjs | 136 ++++++++++++++++++++ 2 files changed, 202 insertions(+), 3 deletions(-) create mode 100644 tests/broker-idle-timeout.test.mjs diff --git a/plugins/codex/scripts/app-server-broker.mjs b/plugins/codex/scripts/app-server-broker.mjs index 1954274fe..e62db2543 100644 --- a/plugins/codex/scripts/app-server-broker.mjs +++ b/plugins/codex/scripts/app-server-broker.mjs @@ -11,6 +11,34 @@ import { parseBrokerEndpoint } from "./lib/broker-endpoint.mjs"; const STREAMING_METHODS = new Set(["turn/start", "review/start", "thread/compact/start"]); +// Broker-side idle timeout. When no client has been connected for this long the +// broker self-terminates. This is the correctness backstop for stale ownership +// records in broker.json: a co-owning session that exits without running its +// SessionEnd hook (SIGKILL, OOM, crash, host reboot) or whose teardown skips the +// entry on lock contention leaves its sessionId behind forever, so no future hook +// will ever tear the broker down. Self-termination on idle is platform-independent +// and needs no PID/liveness signal, so it covers the abnormal-exit orphan, the +// dead-co-owner orphan, and the lock-contention skip in one mechanism. See #108, +// #380, and #450. +const IDLE_TIMEOUT_ENV = "CODEX_COMPANION_BROKER_IDLE_TIMEOUT_MS"; +const DEFAULT_IDLE_TIMEOUT_MS = 30 * 60 * 1000; + +// Resolve the idle timeout from the CLI flag, then the environment, then the +// default. A value <= 0 (or a non-finite/negative override) disables the timeout +// so the broker never self-terminates, which keeps the old behavior available for +// callers that manage lifecycle themselves. +function resolveIdleTimeoutMs(optionValue, env = process.env) { + const raw = optionValue ?? env[IDLE_TIMEOUT_ENV]; + if (raw === undefined || raw === null || raw === "") { + return DEFAULT_IDLE_TIMEOUT_MS; + } + const parsed = Number(raw); + if (!Number.isFinite(parsed) || parsed < 0) { + return DEFAULT_IDLE_TIMEOUT_MS; + } + return parsed; +} + function buildStreamThreadIds(method, params, result) { const threadIds = new Set(); if (params?.threadId) { @@ -48,11 +76,11 @@ function writePidFile(pidFile) { async function main() { const [subcommand, ...argv] = process.argv.slice(2); if (subcommand !== "serve") { - throw new Error("Usage: node scripts/app-server-broker.mjs serve --endpoint [--cwd ] [--pid-file ]"); + throw new Error("Usage: node scripts/app-server-broker.mjs serve --endpoint [--cwd ] [--pid-file ] [--idle-timeout ]"); } const { options } = parseArgs(argv, { - valueOptions: ["cwd", "pid-file", "endpoint"] + valueOptions: ["cwd", "pid-file", "endpoint", "idle-timeout"] }); if (!options.endpoint) { @@ -63,6 +91,7 @@ async function main() { const endpoint = String(options.endpoint); const listenTarget = parseBrokerEndpoint(endpoint); const pidFile = options["pid-file"] ? path.resolve(options["pid-file"]) : null; + const idleTimeoutMs = resolveIdleTimeoutMs(options["idle-timeout"]); writePidFile(pidFile); const appClient = await CodexAppServerClient.connect(cwd, { disableBroker: true }); @@ -70,6 +99,31 @@ async function main() { let activeStreamSocket = null; let activeStreamThreadIds = null; const sockets = new Set(); + let idleTimer = null; + + function disarmIdleTimer() { + if (idleTimer) { + clearTimeout(idleTimer); + idleTimer = null; + } + } + + // Arm the idle timer whenever the broker has no connected clients. A live + // client connection means the broker is still in use, so we only count down + // while idle and cancel the moment a client connects. When the timer fires we + // shut the broker down gracefully and exit. + function armIdleTimer() { + disarmIdleTimer(); + if (idleTimeoutMs <= 0 || sockets.size > 0) { + return; + } + idleTimer = setTimeout(() => { + idleTimer = null; + shutdown(server) + .catch(() => {}) + .finally(() => process.exit(0)); + }, idleTimeoutMs); + } function clearSocketOwnership(socket) { if (activeRequestSocket === socket) { @@ -100,6 +154,7 @@ async function main() { } async function shutdown(server) { + disarmIdleTimer(); for (const socket of sockets) { socket.end(); } @@ -117,6 +172,7 @@ async function main() { const server = net.createServer((socket) => { sockets.add(socket); + disarmIdleTimer(); socket.setEncoding("utf8"); let buffer = ""; @@ -225,11 +281,13 @@ async function main() { socket.on("close", () => { sockets.delete(socket); clearSocketOwnership(socket); + armIdleTimer(); }); socket.on("error", () => { sockets.delete(socket); clearSocketOwnership(socket); + armIdleTimer(); }); }); @@ -243,7 +301,12 @@ async function main() { process.exit(0); }); - server.listen(listenTarget.path); + server.listen(listenTarget.path, () => { + // Start counting down immediately: a broker that is spawned but never + // receives a client (or whose only client connects briefly during the + // readiness probe) must still self-terminate instead of lingering. + armIdleTimer(); + }); } main().catch((error) => { diff --git a/tests/broker-idle-timeout.test.mjs b/tests/broker-idle-timeout.test.mjs new file mode 100644 index 000000000..4a848897b --- /dev/null +++ b/tests/broker-idle-timeout.test.mjs @@ -0,0 +1,136 @@ +import fs from "node:fs"; +import net from "node:net"; +import path from "node:path"; +import test from "node:test"; +import assert from "node:assert/strict"; +import { spawn } from "node:child_process"; +import { fileURLToPath } from "node:url"; + +import { buildEnv, installFakeCodex } from "./fake-codex-fixture.mjs"; +import { makeTempDir } from "./helpers.mjs"; +import { createBrokerEndpoint, parseBrokerEndpoint } from "../plugins/codex/scripts/lib/broker-endpoint.mjs"; +import { waitForBrokerEndpoint } from "../plugins/codex/scripts/lib/broker-lifecycle.mjs"; + +const ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), ".."); +const BROKER_SCRIPT = path.join(ROOT, "plugins", "codex", "scripts", "app-server-broker.mjs"); + +function spawnBroker({ cwd, endpoint, env, idleTimeoutMs }) { + const args = [BROKER_SCRIPT, "serve", "--endpoint", endpoint, "--cwd", cwd]; + if (idleTimeoutMs !== undefined) { + args.push("--idle-timeout", String(idleTimeoutMs)); + } + return spawn(process.execPath, args, { + cwd, + env, + stdio: ["ignore", "pipe", "pipe"] + }); +} + +function waitForExit(child, { timeoutMs = 5000 } = {}) { + return new Promise((resolve, reject) => { + if (child.exitCode !== null || child.signalCode !== null) { + resolve({ code: child.exitCode, signal: child.signalCode }); + return; + } + const timer = setTimeout(() => { + child.removeListener("exit", onExit); + reject(new Error("Timed out waiting for broker process to exit.")); + }, timeoutMs); + function onExit(code, signal) { + clearTimeout(timer); + resolve({ code, signal }); + } + child.once("exit", onExit); + }); +} + +function connectClient(endpoint) { + const target = parseBrokerEndpoint(endpoint); + return new Promise((resolve, reject) => { + const socket = net.createConnection({ path: target.path }); + socket.once("connect", () => resolve(socket)); + socket.once("error", reject); + }); +} + +function delay(ms) { + return new Promise((resolve) => setTimeout(resolve, ms)); +} + +test("broker self-terminates after the idle timeout when no client is connected", async () => { + const binDir = makeTempDir(); + installFakeCodex(binDir); + const sessionDir = makeTempDir("cxc-"); + const endpoint = createBrokerEndpoint(sessionDir); + const child = spawnBroker({ cwd: sessionDir, endpoint, env: buildEnv(binDir), idleTimeoutMs: 300 }); + + try { + const ready = await waitForBrokerEndpoint(endpoint, 3000); + assert.equal(ready, true, "broker should accept connections before it times out"); + + const start = Date.now(); + const result = await waitForExit(child, { timeoutMs: 5000 }); + const elapsed = Date.now() - start; + + assert.equal(result.code, 0, "broker should exit cleanly on idle timeout"); + assert.ok(elapsed >= 200, `broker exited too early (${elapsed}ms)`); + } finally { + if (child.exitCode === null && child.signalCode === null) { + child.kill(); + } + } +}); + +test("broker stays alive while a client is connected and exits after it disconnects", async () => { + const binDir = makeTempDir(); + installFakeCodex(binDir); + const sessionDir = makeTempDir("cxc-"); + const endpoint = createBrokerEndpoint(sessionDir); + const child = spawnBroker({ cwd: sessionDir, endpoint, env: buildEnv(binDir), idleTimeoutMs: 300 }); + + let socket = null; + try { + const ready = await waitForBrokerEndpoint(endpoint, 3000); + assert.equal(ready, true); + + socket = await connectClient(endpoint); + + // Hold the connection open well past the idle timeout; the broker must not + // self-terminate while a client is still connected. + await delay(900); + assert.equal(child.exitCode, null, "broker must stay alive while a client is connected"); + + socket.end(); + socket = null; + + const result = await waitForExit(child, { timeoutMs: 5000 }); + assert.equal(result.code, 0, "broker should exit once the client disconnects and it goes idle"); + } finally { + if (socket) { + socket.destroy(); + } + if (child.exitCode === null && child.signalCode === null) { + child.kill(); + } + } +}); + +test("broker with the idle timeout disabled keeps running while idle", async () => { + const binDir = makeTempDir(); + installFakeCodex(binDir); + const sessionDir = makeTempDir("cxc-"); + const endpoint = createBrokerEndpoint(sessionDir); + const child = spawnBroker({ cwd: sessionDir, endpoint, env: buildEnv(binDir), idleTimeoutMs: 0 }); + + try { + const ready = await waitForBrokerEndpoint(endpoint, 3000); + assert.equal(ready, true); + + await delay(700); + assert.equal(child.exitCode, null, "broker must not self-terminate when the idle timeout is disabled"); + } finally { + if (child.exitCode === null && child.signalCode === null) { + child.kill(); + } + } +}); From 887ed07f271d884abb73e1df9a70743ed2db461c Mon Sep 17 00:00:00 2001 From: CBEPX <458940+CBEPX@users.noreply.github.com> Date: Fri, 28 Aug 2026 00:22:11 +0300 Subject: [PATCH 2/6] docs: v1.1.1 plan Co-Authored-By: Claude Fable 5 --- .../2026-08-28-codex-plugin-cc-v1.1.1.md | 100 ++++++++++++++++++ 1 file changed, 100 insertions(+) create mode 100644 docs/superpowers/plans/2026-08-28-codex-plugin-cc-v1.1.1.md diff --git a/docs/superpowers/plans/2026-08-28-codex-plugin-cc-v1.1.1.md b/docs/superpowers/plans/2026-08-28-codex-plugin-cc-v1.1.1.md new file mode 100644 index 000000000..eabf468a3 --- /dev/null +++ b/docs/superpowers/plans/2026-08-28-codex-plugin-cc-v1.1.1.md @@ -0,0 +1,100 @@ +# codex-plugin-cc v1.1.1 Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Patch release that stops the process leaks (idle brokers in real use, fake app-servers/brokers in the test suite) and fixes the `rm`-alias noise in the rescue shell blocks. + +**Architecture:** Merge upstream PR #457 (broker idle self-terminate; env/flag override) and reuse that mechanism in tests via a 2 s idle timeout in `buildEnv()`, gated in CI by a post-test `pgrep` check. `command rm -f --` in the four `trap` lines. Then bump 1.1.1, CHANGELOG, release. + +**Tech Stack:** Node ≥18.18, ESM `.mjs`, `node --test`, fake Codex fixture, `gh`. + +**Spec:** memory `codex-plugin-cc-fork-backlog` (v1.1.1 items) — leak evidence: 3147 fake `codex app-server` processes after one day of `npm test` runs; 12 real idle brokers + app-servers days old; `trash: : path does not exist` from `trap 'rm -f …'` with the user's `rm→trash` alias. + +## Global Constraints + +- Repo `/Users/g.mehrenin/project/personal/codex-plugin-cc`, `origin`=CBEPX, `upstream`=openai. Branch `release/v1.1.1` from `main` (23942d7 = v1.1.0). +- Test gate: `npm test > /tmp/npm-test.log 2>&1; st=$?; rg -e 'ℹ (tests|pass|fail)' -e '^not ok' /tmp/npm-test.log; test "$st" -eq 0` (142 tests at base). `npm run build`, `npm run check-version`, `claude plugin validate . --strict` before the release commit. +- Tooling rule (user): never `grep` — ripgrep `rg` only. No `git add -A` (`.superpowers/` untracked). Commit trailer `Co-Authored-By: Claude Fable 5 `. No push until the controller says so. +- Merge mechanics for #457: `git fetch upstream pull/457/head:pr/457 && git merge --no-ff --no-edit pr/457`; keep-both on conflicts (the fork's `withAppServer` third param and `runAppServerTurn` `disableBroker` must survive). + +--- + +### Task 1: Merge #457 (broker idle self-terminate) + test idle timeout + CI gate + `command rm` + +**Files:** +- Merge: `plugins/codex/scripts/app-server-broker.mjs`, `tests/broker-idle-timeout.test.mjs` (from PR #457) +- Modify: `tests/fake-codex-fixture.mjs` (`buildEnv`), `tests/runtime.test.mjs` (one assertion in the existing broker-reuse test), `.github/workflows/pull-request-ci.yml`, `plugins/codex/commands/rescue.md` (2 `trap` lines), `plugins/codex/agents/codex-rescue.md` (2 `trap` lines), `tests/commands.test.mjs` + +**Interfaces:** +- Consumes (from #457): env `CODEX_COMPANION_BROKER_IDLE_TIMEOUT_MS`, broker flag `--idle-timeout `, default 30 min; broker exits and shuts down its app-server child when no client is connected for that long. +- Produces: `buildEnv(binDir, …)` sets `CODEX_COMPANION_BROKER_IDLE_TIMEOUT_MS: "2000"` unless the caller passes its own; CI step `sleep 5; ! pgrep -f codex-plugin-test-` after `npm test`; `trap 'command rm -f -- "$ERR" "$OUT" "$PROMPT"' EXIT` in all four shell blocks. + +- [ ] **Step 1: Branch + merge #457** + +```bash +git checkout -b release/v1.1.1 main +git fetch upstream pull/457/head:pr/457 && git merge --no-ff --no-edit pr/457 +``` +Expected conflicts: none or trivial (broker file untouched by the fork). Gate → `fail 0`; `tests/broker-idle-timeout.test.mjs` present and passing. + +- [ ] **Step 2: Failing test — test brokers self-terminate** + +In `tests/runtime.test.mjs`, find the existing broker-reuse test (the one asserting `appServerStarts` stays at 1 across two `task` runs, or `loadBrokerSession`); after its last companion call append: + +```js + const session = JSON.parse(fs.readFileSync(path.join(repo, ".codex-companion", "broker.json"), "utf8")); // adapt: use loadBrokerSession(repo) / the real broker.json path the fixture exposes + const brokerPid = session.pid; + assert.ok(brokerPid > 0); + const deadline = Date.now() + 6000; + let alive = true; + while (alive && Date.now() < deadline) { + try { process.kill(brokerPid, 0); await new Promise((r) => setTimeout(r, 200)); } catch { alive = false; } + } + assert.equal(alive, false, `broker ${brokerPid} should exit within the 2 s test idle timeout`); +``` +(Make the test `async`. If the broker pid lives in `~/.claude/plugins/data` state rather than the repo, read it from `loadBrokerSession(...)` exported by `plugins/codex/scripts/lib/broker-lifecycle.mjs` — check its signature first.) + +- [ ] **Step 3: Run — expect FAIL** (broker still alive: default idle timeout is 30 min). + +- [ ] **Step 4: `buildEnv` sets the 2 s idle timeout** — in `tests/fake-codex-fixture.mjs` `buildEnv(binDir, overrides = {})` add `CODEX_COMPANION_BROKER_IDLE_TIMEOUT_MS: "2000"` before spreading caller overrides (so `broker-idle-timeout.test.mjs` can still set its own value). + +- [ ] **Step 5: Run — expect PASS.** Then full gate → `fail 0`. Then `sleep 5; pgrep -fl codex-plugin-test- | wc -l` → 0. + +- [ ] **Step 6: CI gate** — in `.github/workflows/pull-request-ci.yml` after the `Run test suite` step add: +```yaml + - name: No leaked test processes + run: | + sleep 5 + if pgrep -f codex-plugin-test- ; then echo "leaked test processes" >&2; exit 1; fi +``` + +- [ ] **Step 7: `command rm`** — replace all four `trap 'rm -f "$ERR" "$OUT" "$PROMPT"' EXIT` with `trap 'command rm -f -- "$ERR" "$OUT" "$PROMPT"' EXIT` (rescue.md ×2, codex-rescue.md ×2). In `tests/commands.test.mjs` add to the rescue test: `assert.match(rescue, /trap 'command rm -f -- /); assert.match(agent, /trap 'command rm -f -- /); assert.doesNotMatch(rescue, /trap 'rm -f/); assert.doesNotMatch(agent, /trap 'rm -f/);`. + +- [ ] **Step 8: Gate → `fail 0`; commit** + +```bash +git add tests/fake-codex-fixture.mjs tests/runtime.test.mjs tests/commands.test.mjs .github/workflows/pull-request-ci.yml plugins/codex/commands/rescue.md plugins/codex/agents/codex-rescue.md +git commit -m "fix(broker,tests): idle self-terminate via #457; 2s idle timeout in tests; CI leak gate; command rm in traps" -m "Co-Authored-By: Claude Fable 5 " +``` + +--- + +### Task 2: Release v1.1.1 + +**Files:** `package.json`, `package-lock.json`, `plugins/codex/.claude-plugin/plugin.json`, `.claude-plugin/marketplace.json` (via `npm run bump-version -- 1.1.1`), `CHANGELOG.md`. + +- [ ] **Step 1:** `npm run bump-version -- 1.1.1 && npm run check-version` (lockfile name check included since v1.1.0). +- [ ] **Step 2: CHANGELOG** — insert above `## 1.1.0`: +```markdown +## 1.1.1 — 2026-08-28 + +- Broker idle self-terminate (upstream #457): the shared Codex runtime exits after 30 minutes without a connected client (`CODEX_COMPANION_BROKER_IDLE_TIMEOUT_MS` / `--idle-timeout`), so idle brokers and their app-server children no longer accumulate (#543). +- Test suite no longer leaks fake `codex app-server`/broker processes (2 s idle timeout in the test environment; CI fails if any `codex-plugin-test-*` process survives). +- Rescue shell blocks use `command rm -f --` in their cleanup trap (no noise from `rm` aliases such as `trash`). +``` +- [ ] **Step 3:** gate, `npm run build`, `claude plugin validate . --strict`; commit `chore(release): v1.1.1`. + +## Verification +1. `npm test` then `sleep 5; pgrep -f codex-plugin-test-` → nothing. +2. Real broker: run a companion `task` in a scratch repo with `CODEX_COMPANION_BROKER_IDLE_TIMEOUT_MS=5000`, wait 8 s → broker pid gone, its app-server gone. +3. `/codex:rescue …` in this session → no `trash:` line. From 0d83855b0c00c588cc23a478c7423b0ab2dace8b Mon Sep 17 00:00:00 2001 From: CBEPX <458940+CBEPX@users.noreply.github.com> Date: Fri, 28 Aug 2026 00:33:26 +0300 Subject: [PATCH 3/6] fix(broker,tests): idle self-terminate via #457; 2s idle timeout in tests; CI leak gate; command rm in traps Co-Authored-By: Claude Fable 5 --- .github/workflows/pull-request-ci.yml | 5 +++++ plugins/codex/agents/codex-rescue.md | 4 ++-- plugins/codex/commands/rescue.md | 4 ++-- tests/commands.test.mjs | 8 +++++++- tests/fake-codex-fixture.mjs | 9 +++++++-- tests/runtime.test.mjs | 14 ++++++++++++++ 6 files changed, 37 insertions(+), 7 deletions(-) diff --git a/.github/workflows/pull-request-ci.yml b/.github/workflows/pull-request-ci.yml index 9f54ddfd6..7fecee3a2 100644 --- a/.github/workflows/pull-request-ci.yml +++ b/.github/workflows/pull-request-ci.yml @@ -33,5 +33,10 @@ jobs: - name: Run test suite run: npm test + - name: No leaked test processes + run: | + sleep 5 + if pgrep -f codex-plugin-test- ; then echo "leaked test processes" >&2; exit 1; fi + - name: Run build run: npm run build diff --git a/plugins/codex/agents/codex-rescue.md b/plugins/codex/agents/codex-rescue.md index 08fdbbc9b..9bab35bb6 100644 --- a/plugins/codex/agents/codex-rescue.md +++ b/plugins/codex/agents/codex-rescue.md @@ -25,7 +25,7 @@ Forwarding rules: Launch (one Bash call): ```bash -trap 'rm -f "$ERR" "$OUT" "$PROMPT"' EXIT +trap 'command rm -f -- "$ERR" "$OUT" "$PROMPT"' EXIT ERR=$(mktemp); PROMPT=$(mktemp) cat > "$PROMPT" <<'CODEX_PROMPT_' @@ -43,7 +43,7 @@ echo "JOB=$JOB" Wait and fetch the result (one Bash call, tool `timeout: 600000`). Set `JOB=` literally as the first line, using the id you just read: ```bash -trap 'rm -f "$ERR" "$OUT" "$PROMPT"' EXIT +trap 'command rm -f -- "$ERR" "$OUT" "$PROMPT"' EXIT JOB= [[ "$JOB" =~ ^[A-Za-z0-9_-]+$ ]] || { echo "invalid job id"; exit 1; } OUT=$(mktemp); ERR=$(mktemp) diff --git a/plugins/codex/commands/rescue.md b/plugins/codex/commands/rescue.md index d88ea6767..e6eea5aef 100644 --- a/plugins/codex/commands/rescue.md +++ b/plugins/codex/commands/rescue.md @@ -20,7 +20,7 @@ The request prose and the runtime flags travel in two separate channels of the s 2a. Launch (one Bash call): ```bash -trap 'rm -f "$ERR" "$OUT" "$PROMPT"' EXIT +trap 'command rm -f -- "$ERR" "$OUT" "$PROMPT"' EXIT ERR=$(mktemp); PROMPT=$(mktemp) cat > "$PROMPT" <<'CODEX_PROMPT_' @@ -38,7 +38,7 @@ If this call exits non-zero, its output is the launch failure (Codex missing, un 2b. Wait and fetch the result (one Bash call, tool `timeout: 600000`). Set `JOB=` literally as the first line, using the id you just read from 2a: ```bash -trap 'rm -f "$ERR" "$OUT" "$PROMPT"' EXIT +trap 'command rm -f -- "$ERR" "$OUT" "$PROMPT"' EXIT JOB= [[ "$JOB" =~ ^[A-Za-z0-9_-]+$ ]] || { echo "invalid job id"; exit 1; } OUT=$(mktemp); ERR=$(mktemp) diff --git a/tests/commands.test.mjs b/tests/commands.test.mjs index 6053dee3a..8629e7b5e 100644 --- a/tests/commands.test.mjs +++ b/tests/commands.test.mjs @@ -117,6 +117,12 @@ test("rescue command absorbs continue semantics", () => { assert.match(agent, /thin forwarding wrapper/i); assert.match(agent, /result "\$JOB"/); assert.doesNotMatch(agent, /prefer background execution/i); + // The rescue shell traps must use `command rm -f --` so a shadowing/aliased + // `rm` (or a filename starting with `-`) can't hijack cleanup on EXIT. + assert.match(rescue, /trap 'command rm -f -- /); + assert.match(agent, /trap 'command rm -f -- /); + assert.doesNotMatch(rescue, /trap 'rm -f/); + assert.doesNotMatch(agent, /trap 'rm -f/); assert.match(runtimeSkill, /Launch exactly one job per rescue handoff with `task --background --json`/i); assert.match(agent, /Bash tool's 10-minute cap/i); assert.match(agent, /do not inspect the repository, read files, grep, cancel jobs, summarize output, or do any other follow-up work of your own/i); @@ -335,7 +341,7 @@ test("rescue sends the request prose through --prompt-file, never through the ar //, `${label} still routes the request text through the argument tokenizer` ); - assert.match(body, /rm -f "\$ERR" "\$OUT" "\$PROMPT"/, `${label} must clean up the prose file`); + assert.match(body, /command rm -f -- "\$ERR" "\$OUT" "\$PROMPT"/, `${label} must clean up the prose file`); // A payload line equal to a fixed delimiter would close the heredoc early and // run the rest on the host shell. diff --git a/tests/fake-codex-fixture.mjs b/tests/fake-codex-fixture.mjs index fb058e0b6..042d4327d 100644 --- a/tests/fake-codex-fixture.mjs +++ b/tests/fake-codex-fixture.mjs @@ -663,10 +663,15 @@ rl.on("line", (line) => { } } -export function buildEnv(binDir) { +export function buildEnv(binDir, overrides = {}) { const sep = process.platform === "win32" ? ";" : ":"; return { ...process.env, - PATH: `${binDir}${sep}${process.env.PATH}` + PATH: `${binDir}${sep}${process.env.PATH}`, + // Keep test brokers short-lived so a run that leaves one behind (crash, + // interrupted test, etc.) doesn't linger as an orphaned process for the + // default 30-minute idle timeout. See PR #457. + CODEX_COMPANION_BROKER_IDLE_TIMEOUT_MS: "2000", + ...overrides }; } diff --git a/tests/runtime.test.mjs b/tests/runtime.test.mjs index ce2162dcd..38db5a702 100644 --- a/tests/runtime.test.mjs +++ b/tests/runtime.test.mjs @@ -2365,6 +2365,20 @@ test("commands lazily start and reuse one shared app-server after first use", as const fakeState = JSON.parse(fs.readFileSync(fakeStatePath, "utf8")); assert.equal(fakeState.appServerStarts, 1); + const brokerPid = brokerSession.pid; + assert.ok(brokerPid > 0); + const deadline = Date.now() + 6000; + let alive = true; + while (alive && Date.now() < deadline) { + try { + process.kill(brokerPid, 0); + await new Promise((r) => setTimeout(r, 200)); + } catch { + alive = false; + } + } + assert.equal(alive, false, `broker ${brokerPid} should exit within the 2 s test idle timeout`); + const cleanup = run("node", [SESSION_HOOK, "SessionEnd"], { cwd: repo, env, From 1258ec2cbf598cc13658e42bb24dcb62c655baf0 Mon Sep 17 00:00:00 2001 From: CBEPX <458940+CBEPX@users.noreply.github.com> Date: Fri, 28 Aug 2026 00:46:19 +0300 Subject: [PATCH 4/6] fix(tests): 5 s broker idle timeout with a 12 s exit deadline to avoid CI flakes Co-Authored-By: Claude Fable 5 --- tests/fake-codex-fixture.mjs | 2 +- tests/runtime.test.mjs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/fake-codex-fixture.mjs b/tests/fake-codex-fixture.mjs index 042d4327d..2550356ad 100644 --- a/tests/fake-codex-fixture.mjs +++ b/tests/fake-codex-fixture.mjs @@ -671,7 +671,7 @@ export function buildEnv(binDir, overrides = {}) { // Keep test brokers short-lived so a run that leaves one behind (crash, // interrupted test, etc.) doesn't linger as an orphaned process for the // default 30-minute idle timeout. See PR #457. - CODEX_COMPANION_BROKER_IDLE_TIMEOUT_MS: "2000", + CODEX_COMPANION_BROKER_IDLE_TIMEOUT_MS: "5000", ...overrides }; } diff --git a/tests/runtime.test.mjs b/tests/runtime.test.mjs index 38db5a702..07c06f1f9 100644 --- a/tests/runtime.test.mjs +++ b/tests/runtime.test.mjs @@ -2367,7 +2367,7 @@ test("commands lazily start and reuse one shared app-server after first use", as const brokerPid = brokerSession.pid; assert.ok(brokerPid > 0); - const deadline = Date.now() + 6000; + const deadline = Date.now() + 12000; let alive = true; while (alive && Date.now() < deadline) { try { @@ -2377,7 +2377,7 @@ test("commands lazily start and reuse one shared app-server after first use", as alive = false; } } - assert.equal(alive, false, `broker ${brokerPid} should exit within the 2 s test idle timeout`); + assert.equal(alive, false, `broker ${brokerPid} should exit within the 5 s test idle timeout`); const cleanup = run("node", [SESSION_HOOK, "SessionEnd"], { cwd: repo, From fba169e605357dd0082b7193643e0588a3f56ca3 Mon Sep 17 00:00:00 2001 From: CBEPX <458940+CBEPX@users.noreply.github.com> Date: Fri, 28 Aug 2026 00:52:25 +0300 Subject: [PATCH 5/6] chore(release): v1.1.1 Co-Authored-By: Claude Fable 5 --- .claude-plugin/marketplace.json | 4 ++-- .github/workflows/pull-request-ci.yml | 2 +- CHANGELOG.md | 6 ++++++ package-lock.json | 4 ++-- package.json | 2 +- plugins/codex/.claude-plugin/plugin.json | 2 +- 6 files changed, 13 insertions(+), 7 deletions(-) diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index 19595054b..610b9fb18 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -6,13 +6,13 @@ }, "metadata": { "description": "CBEPX fork of the OpenAI Codex plugin for Claude Code: max/ultra effort, per-thread config overrides, gpt-5.6 aliases, rescue agent fixes.", - "version": "1.1.0" + "version": "1.1.1" }, "plugins": [ { "name": "codex", "description": "Use Codex from Claude Code to review code or delegate tasks.", - "version": "1.1.0", + "version": "1.1.1", "author": { "name": "OpenAI" }, diff --git a/.github/workflows/pull-request-ci.yml b/.github/workflows/pull-request-ci.yml index 7fecee3a2..70cbdcf6a 100644 --- a/.github/workflows/pull-request-ci.yml +++ b/.github/workflows/pull-request-ci.yml @@ -35,7 +35,7 @@ jobs: - name: No leaked test processes run: | - sleep 5 + sleep 10 if pgrep -f codex-plugin-test- ; then echo "leaked test processes" >&2; exit 1; fi - name: Run build diff --git a/CHANGELOG.md b/CHANGELOG.md index 32d8c1490..7df8aea6f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog +## 1.1.1 — 2026-08-28 + +- Broker idle self-terminate (upstream #457): the shared Codex runtime exits after 30 minutes without a connected client (`CODEX_COMPANION_BROKER_IDLE_TIMEOUT_MS` / `--idle-timeout`), so idle brokers and their app-server children no longer accumulate (#543). +- Test suite no longer leaks fake `codex app-server`/broker processes (5 s idle timeout in the test environment; CI fails if any `codex-plugin-test-*` process survives). +- Rescue shell blocks use `command rm -f --` in their cleanup trap (no noise from `rm` aliases such as `trash`). + ## 1.1.0 — 2026-08-27 Fork of [openai/codex-plugin-cc](https://github.com/openai/codex-plugin-cc) 1.0.6 (`db52e28`). Marketplace `cbepx`, plugin name unchanged (`codex`). diff --git a/package-lock.json b/package-lock.json index 224af69a7..1ee96d7dc 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@cbepx/codex-plugin-cc", - "version": "1.1.0", + "version": "1.1.1", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@cbepx/codex-plugin-cc", - "version": "1.1.0", + "version": "1.1.1", "license": "Apache-2.0", "devDependencies": { "@types/node": "^25.5.0", diff --git a/package.json b/package.json index 9422ccaac..671051558 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@cbepx/codex-plugin-cc", - "version": "1.1.0", + "version": "1.1.1", "private": true, "type": "module", "description": "Use Codex from Claude Code to review code or delegate tasks.", diff --git a/plugins/codex/.claude-plugin/plugin.json b/plugins/codex/.claude-plugin/plugin.json index 4838522f0..bb6034608 100644 --- a/plugins/codex/.claude-plugin/plugin.json +++ b/plugins/codex/.claude-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "codex", - "version": "1.1.0", + "version": "1.1.1", "description": "Use Codex from Claude Code to review code or delegate tasks.", "author": { "name": "OpenAI" From 39fc678bede3dac4ad2ebbddfcaa4ce7bd8bbff5 Mon Sep 17 00:00:00 2001 From: CBEPX <458940+CBEPX@users.noreply.github.com> Date: Fri, 28 Aug 2026 01:19:35 +0300 Subject: [PATCH 6/6] fix(broker): clear session record on idle exit, verify pid ownership before teardown, refuse late clients during shutdown The idle self-terminate from #457 unlinked the socket and pid file but left broker.json behind, so a later SessionEnd loaded a dead record and signalled a PID (and process group) the OS may have recycled. The broker now drops the record when it still points at itself, and teardownBrokerSession proves the recorded PID is this session's broker (ps command line contains app-server-broker.mjs and the endpoint) before signalling it. shutdown() also kept listening while it closed the app-server child: a client accepted in that window got the broker-local initialize and then failed its first RPC with "codex app-server client is closed". The listener is now closed synchronously before the first await and late sockets are destroyed. Co-Authored-By: Claude Fable 5 --- CHANGELOG.md | 1 + plugins/codex/scripts/app-server-broker.mjs | 44 +++++- .../codex/scripts/lib/broker-lifecycle.mjs | 19 ++- plugins/codex/scripts/lib/process.mjs | 21 +++ tests/broker-idle-timeout.test.mjs | 62 +++++++++ tests/broker-stale-pid.test.mjs | 130 ++++++++++++++++++ tests/fake-codex-fixture.mjs | 12 ++ tests/process.test.mjs | 14 +- 8 files changed, 297 insertions(+), 6 deletions(-) create mode 100644 tests/broker-stale-pid.test.mjs diff --git a/CHANGELOG.md b/CHANGELOG.md index 7df8aea6f..1ab061eee 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ ## 1.1.1 — 2026-08-28 - Broker idle self-terminate (upstream #457): the shared Codex runtime exits after 30 minutes without a connected client (`CODEX_COMPANION_BROKER_IDLE_TIMEOUT_MS` / `--idle-timeout`), so idle brokers and their app-server children no longer accumulate (#543). +- Broker lifecycle races found in #457: a broker that self-terminates on idle now drops its `broker.json` ownership record (a later `SessionEnd` could otherwise signal a recycled PID, and `status` could advertise a dead endpoint), teardown verifies the recorded PID really is this session's broker before signalling it, and the broker stops listening before it closes its app-server child so a client connecting mid-shutdown is refused instead of being served and then failing its first RPC. - Test suite no longer leaks fake `codex app-server`/broker processes (5 s idle timeout in the test environment; CI fails if any `codex-plugin-test-*` process survives). - Rescue shell blocks use `command rm -f --` in their cleanup trap (no noise from `rm` aliases such as `trash`). diff --git a/plugins/codex/scripts/app-server-broker.mjs b/plugins/codex/scripts/app-server-broker.mjs index e62db2543..b4d0a661d 100644 --- a/plugins/codex/scripts/app-server-broker.mjs +++ b/plugins/codex/scripts/app-server-broker.mjs @@ -8,6 +8,7 @@ import process from "node:process"; import { parseArgs } from "./lib/args.mjs"; import { BROKER_BUSY_RPC_CODE, CodexAppServerClient } from "./lib/app-server.mjs"; import { parseBrokerEndpoint } from "./lib/broker-endpoint.mjs"; +import { clearBrokerSession, loadBrokerSession } from "./lib/broker-lifecycle.mjs"; const STREAMING_METHODS = new Set(["turn/start", "review/start", "thread/compact/start"]); @@ -24,9 +25,10 @@ const IDLE_TIMEOUT_ENV = "CODEX_COMPANION_BROKER_IDLE_TIMEOUT_MS"; const DEFAULT_IDLE_TIMEOUT_MS = 30 * 60 * 1000; // Resolve the idle timeout from the CLI flag, then the environment, then the -// default. A value <= 0 (or a non-finite/negative override) disables the timeout -// so the broker never self-terminates, which keeps the old behavior available for -// callers that manage lifecycle themselves. +// default. Exactly `0` disables the timeout so the broker never self-terminates, +// which keeps the old behavior available for callers that manage lifecycle +// themselves; a negative or non-finite override is treated as garbage and falls +// back to the default. function resolveIdleTimeoutMs(optionValue, env = process.env) { const raw = optionValue ?? env[IDLE_TIMEOUT_ENV]; if (raw === undefined || raw === null || raw === "") { @@ -100,6 +102,7 @@ async function main() { let activeStreamThreadIds = null; const sockets = new Set(); let idleTimer = null; + let shuttingDown = false; function disarmIdleTimer() { if (idleTimer) { @@ -153,13 +156,40 @@ async function main() { } } + // The ownership record in broker.json outlives the process unless we drop it: + // after an idle self-terminate a later SessionEnd hook would load it and + // signal a PID the OS may have recycled, and `status`/`reuseExistingBroker` + // would advertise or dial an endpoint nothing is listening on. Only clear a + // record that still points at this broker — a newer broker may have replaced + // us in it. The state dir derives from --cwd plus the inherited environment, + // exactly as it did in the process that spawned us. + function clearOwnSessionRecord() { + try { + if (loadBrokerSession(cwd)?.endpoint === endpoint) { + clearBrokerSession(cwd); + } + } catch { + // Best-effort: never block shutdown on state-file cleanup. + } + } + + // Closing the app-server child can take a while. Stop listening before the + // first await instead of after it: a client accepted in that window would be + // served the broker-local `initialize` and then fail its first real RPC with + // "codex app-server client is closed", which callers do not retry. async function shutdown(server) { + if (shuttingDown) { + return; + } + shuttingDown = true; disarmIdleTimer(); + clearOwnSessionRecord(); + const serverClosed = new Promise((resolve) => server.close(resolve)); for (const socket of sockets) { socket.end(); } await appClient.close().catch(() => {}); - await new Promise((resolve) => server.close(resolve)); + await serverClosed; if (listenTarget.kind === "unix" && fs.existsSync(listenTarget.path)) { fs.unlinkSync(listenTarget.path); } @@ -171,6 +201,12 @@ async function main() { appClient.setNotificationHandler(routeNotification); const server = net.createServer((socket) => { + if (shuttingDown) { + // Already accepted before the listener finished closing: reset it so the + // client retries or reports a connection error instead of half-working. + socket.destroy(); + return; + } sockets.add(socket); disarmIdleTimer(); socket.setEncoding("utf8"); diff --git a/plugins/codex/scripts/lib/broker-lifecycle.mjs b/plugins/codex/scripts/lib/broker-lifecycle.mjs index ef763819c..bb51f5d91 100644 --- a/plugins/codex/scripts/lib/broker-lifecycle.mjs +++ b/plugins/codex/scripts/lib/broker-lifecycle.mjs @@ -6,6 +6,7 @@ import process from "node:process"; import { spawn } from "node:child_process"; import { fileURLToPath } from "node:url"; import { createBrokerEndpoint, parseBrokerEndpoint } from "./broker-endpoint.mjs"; +import { processCommandLine } from "./process.mjs"; import { resolveStateDir } from "./state.mjs"; export const PID_FILE_ENV = "CODEX_COMPANION_APP_SERVER_PID_FILE"; @@ -170,8 +171,24 @@ export async function ensureBrokerSession(cwd, options = {}) { return session; } +// A recorded PID is only worth signalling while it still belongs to this +// session's broker: an idle self-terminate (or any abnormal exit) can leave the +// record behind long enough for the OS to hand the PID — and with it the process +// group `terminateProcessTree` kills — to something unrelated. Windows has no +// cheap equivalent probe, so it keeps the previous unconditional behavior. +function ownsBrokerProcess(pid, endpoint) { + if (process.platform === "win32") { + return true; + } + const commandLine = processCommandLine(pid); + if (!commandLine || !commandLine.includes("app-server-broker.mjs")) { + return false; + } + return !endpoint || commandLine.includes(endpoint); +} + export function teardownBrokerSession({ endpoint = null, pidFile, logFile, sessionDir = null, pid = null, killProcess = null }) { - if (Number.isFinite(pid) && killProcess) { + if (Number.isFinite(pid) && killProcess && ownsBrokerProcess(pid, endpoint)) { try { killProcess(pid); } catch { diff --git a/plugins/codex/scripts/lib/process.mjs b/plugins/codex/scripts/lib/process.mjs index dd8fc3751..05109a7ff 100644 --- a/plugins/codex/scripts/lib/process.mjs +++ b/plugins/codex/scripts/lib/process.mjs @@ -54,6 +54,27 @@ function looksLikeMissingProcessMessage(text) { return /not found|no running instance|cannot find|does not exist|no such process/i.test(text); } +// Command line of a running process, or null when it is gone (or the platform +// has no `ps`). Callers use it to prove a recorded PID is still the process they +// believe it is before signalling it — PIDs get recycled. +export function processCommandLine(pid, options = {}) { + if (!Number.isFinite(pid)) { + return null; + } + + const platform = options.platform ?? process.platform; + if (platform === "win32") { + return null; + } + + const runCommandImpl = options.runCommandImpl ?? runCommand; + const result = runCommandImpl("ps", ["-o", "command=", "-p", String(pid)]); + if (result.error || result.status !== 0) { + return null; + } + return result.stdout.trim() || null; +} + export function terminateProcessTree(pid, options = {}) { if (!Number.isFinite(pid)) { return { attempted: false, delivered: false, method: null }; diff --git a/tests/broker-idle-timeout.test.mjs b/tests/broker-idle-timeout.test.mjs index 4a848897b..8d17d1255 100644 --- a/tests/broker-idle-timeout.test.mjs +++ b/tests/broker-idle-timeout.test.mjs @@ -57,6 +57,32 @@ function delay(ms) { return new Promise((resolve) => setTimeout(resolve, ms)); } +// Connect and try one `initialize`, reporting whether the broker served it. +function connectAndInitialize(endpoint, { timeoutMs = 1000 } = {}) { + const target = parseBrokerEndpoint(endpoint); + return new Promise((resolve) => { + const socket = net.createConnection({ path: target.path }); + let settled = false; + const finish = (outcome) => { + if (settled) { + return; + } + settled = true; + clearTimeout(timer); + socket.destroy(); + resolve(outcome); + }; + const timer = setTimeout(() => finish({ initialized: false, reason: "timeout" }), timeoutMs); + socket.setEncoding("utf8"); + socket.on("connect", () => { + socket.write(`${JSON.stringify({ id: 1, method: "initialize", params: {} })}\n`); + }); + socket.on("data", (chunk) => finish({ initialized: chunk.includes("result"), reason: chunk.trim() })); + socket.on("error", (error) => finish({ initialized: false, reason: error.code ?? error.message })); + socket.on("close", () => finish({ initialized: false, reason: "closed" })); + }); +} + test("broker self-terminates after the idle timeout when no client is connected", async () => { const binDir = makeTempDir(); installFakeCodex(binDir); @@ -134,3 +160,39 @@ test("broker with the idle timeout disabled keeps running while idle", async () } } }); + +// Shutting down takes as long as the app-server child needs to exit. A client +// that connects during that window used to be accepted and answered locally by +// the broker's own `initialize`, then failed its first real RPC with +// "codex app-server client is closed" — an error the caller does not retry. +test("broker refuses clients that connect after the idle shutdown starts", async () => { + const binDir = makeTempDir(); + installFakeCodex(binDir); + const sessionDir = makeTempDir("cxc-"); + const endpoint = createBrokerEndpoint(sessionDir); + const child = spawnBroker({ + cwd: sessionDir, + endpoint, + env: buildEnv(binDir, { FAKE_CODEX_CLOSE_DELAY_MS: "2000" }), + idleTimeoutMs: 300 + }); + + try { + const ready = await waitForBrokerEndpoint(endpoint, 3000); + assert.equal(ready, true); + + // No endpoint probing between here and the late connect: every probe + // connection re-arms the idle timer, so the window would never open. + await delay(700); + + const outcome = await connectAndInitialize(endpoint); + assert.equal(outcome.initialized, false, `late client must not be served: ${JSON.stringify(outcome)}`); + + const result = await waitForExit(child, { timeoutMs: 10000 }); + assert.equal(result.code, 0, "broker must still exit after refusing the late client"); + } finally { + if (child.exitCode === null && child.signalCode === null) { + child.kill("SIGKILL"); + } + } +}); diff --git a/tests/broker-stale-pid.test.mjs b/tests/broker-stale-pid.test.mjs new file mode 100644 index 000000000..332b17bb0 --- /dev/null +++ b/tests/broker-stale-pid.test.mjs @@ -0,0 +1,130 @@ +import path from "node:path"; +import test from "node:test"; +import assert from "node:assert/strict"; +import { spawn } from "node:child_process"; +import { fileURLToPath } from "node:url"; + +import { buildEnv, installFakeCodex } from "./fake-codex-fixture.mjs"; +import { makeTempDir, run } from "./helpers.mjs"; +import { createBrokerEndpoint } from "../plugins/codex/scripts/lib/broker-endpoint.mjs"; +import { + clearBrokerSession, + loadBrokerSession, + saveBrokerSession, + waitForBrokerEndpoint +} from "../plugins/codex/scripts/lib/broker-lifecycle.mjs"; + +const ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), ".."); +const BROKER_SCRIPT = path.join(ROOT, "plugins", "codex", "scripts", "app-server-broker.mjs"); +const SESSION_HOOK = path.join(ROOT, "plugins", "codex", "scripts", "session-lifecycle-hook.mjs"); + +function waitForExit(child, { timeoutMs = 10000 } = {}) { + return new Promise((resolve, reject) => { + if (child.exitCode !== null || child.signalCode !== null) { + resolve({ code: child.exitCode, signal: child.signalCode }); + return; + } + const timer = setTimeout(() => { + child.removeListener("exit", onExit); + reject(new Error("Timed out waiting for broker process to exit.")); + }, timeoutMs); + function onExit(code, signal) { + clearTimeout(timer); + resolve({ code, signal }); + } + child.once("exit", onExit); + }); +} + +function isAlive(pid) { + try { + process.kill(pid, 0); + return true; + } catch { + return false; + } +} + +// A broker that self-terminates on idle must not leave its ownership record +// behind: a later SessionEnd hook would load it and signal a PID the OS may have +// recycled, and `status` would advertise an endpoint nothing is listening on. +test("broker clears its session record when it self-terminates on idle", async () => { + const binDir = makeTempDir(); + installFakeCodex(binDir); + const workspace = makeTempDir(); + const sessionDir = makeTempDir("cxc-"); + const endpoint = createBrokerEndpoint(sessionDir); + + const child = spawn( + process.execPath, + [BROKER_SCRIPT, "serve", "--endpoint", endpoint, "--cwd", workspace, "--idle-timeout", "300"], + { cwd: workspace, env: buildEnv(binDir), stdio: ["ignore", "pipe", "pipe"] } + ); + + saveBrokerSession(workspace, { + endpoint, + pidFile: path.join(sessionDir, "broker.pid"), + logFile: path.join(sessionDir, "broker.log"), + sessionDir, + pid: child.pid + }); + + try { + assert.equal(await waitForBrokerEndpoint(endpoint, 3000), true); + const result = await waitForExit(child); + assert.equal(result.code, 0); + + assert.equal(loadBrokerSession(workspace), null, "idle exit must clear the persisted broker record"); + } finally { + if (child.exitCode === null && child.signalCode === null) { + child.kill("SIGKILL"); + } + clearBrokerSession(workspace); + } +}); + +// The recorded PID may belong to an unrelated process by the time SessionEnd +// runs (the broker exited on idle and the OS recycled its PID). Teardown must +// prove the PID is this session's broker before it signals the process group. +test("session end teardown does not signal a recycled pid that is not this broker", async (t) => { + if (process.platform === "win32") { + t.skip("PID ownership is not verified on Windows"); + return; + } + + const workspace = makeTempDir(); + const sessionDir = makeTempDir("cxc-"); + const endpoint = createBrokerEndpoint(sessionDir); + // Detached so the impostor leads its own process group: that is what + // terminateProcessTree's `kill(-pid)` actually reaches. + const impostor = spawn("sleep", ["60"], { detached: true, stdio: "ignore" }); + impostor.unref(); + + saveBrokerSession(workspace, { + endpoint, + pidFile: path.join(sessionDir, "broker.pid"), + logFile: path.join(sessionDir, "broker.log"), + sessionDir, + pid: impostor.pid + }); + + try { + const cleanup = run("node", [SESSION_HOOK, "SessionEnd"], { + cwd: workspace, + env: process.env, + input: JSON.stringify({ hook_event_name: "SessionEnd", cwd: workspace }) + }); + assert.equal(cleanup.status, 0, cleanup.stderr); + + await new Promise((resolve) => setTimeout(resolve, 200)); + assert.equal(isAlive(impostor.pid), true, "teardown must not signal a PID that is not the broker"); + assert.equal(loadBrokerSession(workspace), null, "stale broker record must be cleared"); + } finally { + try { + process.kill(-impostor.pid, "SIGKILL"); + } catch { + // Already gone. + } + clearBrokerSession(workspace); + } +}); diff --git a/tests/fake-codex-fixture.mjs b/tests/fake-codex-fixture.mjs index 2550356ad..40a351061 100644 --- a/tests/fake-codex-fixture.mjs +++ b/tests/fake-codex-fixture.mjs @@ -283,6 +283,18 @@ bootState.appServerStarts = (bootState.appServerStarts || 0) + 1; saveState(bootState); const rl = readline.createInterface({ input: process.stdin }); + +// Test knob: linger after stdin closes (and ignore the client's SIGTERM +// escalation) so a test can observe the window while a broker is shutting its +// app-server child down. +const CLOSE_DELAY_MS = Number(process.env.FAKE_CODEX_CLOSE_DELAY_MS || 0); +if (CLOSE_DELAY_MS > 0) { + process.on("SIGTERM", () => {}); + rl.on("close", () => { + setTimeout(() => process.exit(0), CLOSE_DELAY_MS); + }); +} + rl.on("line", (line) => { if (!line.trim()) { return; diff --git a/tests/process.test.mjs b/tests/process.test.mjs index 80e0715b0..f3d9ceca9 100644 --- a/tests/process.test.mjs +++ b/tests/process.test.mjs @@ -1,7 +1,9 @@ +import path from "node:path"; +import process from "node:process"; import test from "node:test"; import assert from "node:assert/strict"; -import { terminateProcessTree } from "../plugins/codex/scripts/lib/process.mjs"; +import { processCommandLine, terminateProcessTree } from "../plugins/codex/scripts/lib/process.mjs"; test("terminateProcessTree uses taskkill on Windows", () => { let captured = null; @@ -53,3 +55,13 @@ test("terminateProcessTree treats missing Windows processes as already stopped", assert.equal(outcome.result.status, 128); assert.match(outcome.result.stdout, /not found/i); }); + +test("processCommandLine reads the command line of a live process", { skip: process.platform === "win32" }, () => { + const line = processCommandLine(process.pid); + assert.ok(line, "expected a command line for the current process"); + assert.ok(line.includes(path.basename(process.execPath)), line); +}); + +test("processCommandLine returns null for a pid that is not running", { skip: process.platform === "win32" }, () => { + assert.equal(processCommandLine(2 ** 31 - 1), null); +});