test: reap orphaned brokers and clean temp dirs on npm test exit (closes openai#163)#5
Merged
Conversation
Closes openai#163. `node --test tests/*.test.mjs` was leaving behind detached app-server-broker.mjs processes (all reparented to PID 1) plus thousands of temp dirs. Root cause: production spawnBrokerProcess spawns the broker `detached:true` + `unref()` so it survives the parent — correct in production, fatal in tests, because node:test does not forward signals/exit to detached grandchildren. The test process cannot capture the broker pid at spawn (it is born inside a grandchild subprocess via production code), so the harness tracks workspace cws instead and recovers {endpoint, pid, sessionDir} deterministically from broker.json at teardown. This matches the sketch Wintersta7e proposed in openai#163. Two-layer teardown, forced by node:test's default --test-isolation=process (each test file is its own worker): - tests/helpers.mjs: a registry of temp dirs and broker-spawning workspaces. makeTempDir registers the dir; run() registers its cwd when it lives under the temp root. Worker process.on('exit')/SIGINT/SIGTERM handlers reap every tracked broker (loadBrokerSession -> sendBrokerShutdown -> teardownBrokerSession -> clearBrokerSession, the SessionEnd sequence) and rm -rf the tracked dirs. Pid-reuse safety: only tree-kill when the endpoint probe confirms the broker is live — mirrors loadReusableBrokerSessionUnlocked (broker-lifecycle.mjs:193-216), so a stale broker.json whose pid the OS recycled into an unrelated process cannot trigger killing the wrong process. - tests/test-global-setup.mjs: a --test-global-setup module that runs once in the parent after all workers exit. Sweeps broker.json under every codex-plugin-test-data-* root for brokers a crashed worker missed, then removes those roots. - tests/fake-codex-fixture.mjs: buildEnv now points CLAUDE_PLUGIN_DATA at one ephemeral per-worker temp root (codex-plugin-test-data-*), so tests no longer inherit the real plugin data dir via `...process.env`. This is the env-inheritance leak axisrow flagged in openai#163 and openai#487 part 1. process.env is set too, so the worker's own resolveStateDir agrees with the companions it spawns. - package.json: test script gains --test-global-setup and --test-force-exit (the latter so process.exit fires and the exit handlers run). Verified: a full `npm test` run adds 0 new broker processes, 0 new cxc-* dirs, and 0 leftover codex-plugin-test-data-* dirs (measured before/after). Same under SIGINT mid-run. Production code (plugins/codex/scripts/) is untouched — this is test-only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0189cGaohsYdpkB4otKZnpAt
Owner
Author
🔍 Local review (cycle 1)Reviewed locally (Claude subagent + Codex companion), no bots pinged. Codex returned
Extra finding surfaced while fixing: All fixes applied + verified empirically (full run: 0 broker leak, 0 test-data leak; SIGINT mid-run: 0 leak; both globalSetup/globalTeardown confirmed firing). Commit follows. |
…e broker state Addresses Codex review of PR #5 (verdict needs-attention / NO-SHIP). All three critical findings verified empirically and fixed: 1. Parent sweep was never activated. `--test-global-setup` placed AFTER the `tests/*.test.mjs` glob was silently ignored by node (the shell-expanded positional paths come first, so the flag never reaches the option parser). Reordered: flags first, paths last. Verified with a bad-path smoke check (before: exit 0 / ignored; after: fails to resolve the module). 2. globalTeardown was returned from globalSetup, but node:test looks for a NAMED `globalTeardown` export — the returned fn is never called. Split into two named exports. Verified both fire (instrumented markers). 3. Worker sync `exit` path destroyed broker.json / pid-file / sessionDir / plugin-data-root WITHOUT terminating the broker first. A live (busy/wedged) broker would be orphaned AND undiscoverable by the parent sweep. The sync exit path now leaves all broker metadata and the plugin-data root intact for the parent globalTeardown to reap; only non-broker temp dirs (workspace, binDir, home) are removed on sync exit. The async signal path (SIGINT/ SIGTERM) was also not awaiting reapWorkerBrokers before process.exit — now awaits the full probe→shutdown→tree-kill→remove sequence, then exits. Also: - Removed --test-force-exit: it aborted globalTeardown mid-await (trace showed START but never rmData OK). Without it, node awaits the async teardown and the sweep completes — verified: parent reaped all brokers and removed all codex-plugin-test-data-* roots. - Raised engines.node to >=24.0.0 (--test-global-setup was added in Node 24). - Removed a duplicated stale comment block in fake-codex-fixture.mjs. Verification (full npm test): brokers 150→150 (0 leaked), test-data roots removed entirely (parent sweep ran to completion). SIGINT mid-run: 0 leaked. globalSetup + globalTeardown both confirmed firing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0189cGaohsYdpkB4otKZnpAt
Cycle-2 review findings (Codex + Claude subagent): 1. pid-reuse guard regressed in cycle 1: reapWorkerBrokers dropped the `await` on `safeAwait(waitForBrokerEndpoint(...))`, so `ready` was an unresolved Promise (always truthy) → killProcess selected `terminateProcessTree` unconditionally, even for a dead endpoint whose pid the OS may have recycled into an unrelated process. The exact invariant the PR claims to protect. `await` restored; killProcess once again only set when the probe confirms the broker is live. 2. --test-global-setup needs Node 24, but CI ran Node 22 and engines/lockfile/ README still said 18.18. Bumped all four in lockstep: CI workflow 22→24, README "Node.js 18.18 or later" → "Node.js 24 or later", package-lock engines mirrored (already >=24 in package.json from cycle 1). Cycle-2 cleanup (non-blocking, applied in the final pass): - reapWorkerBrokers() called with a stray `true` arg from the old signature; removed. - registerTrackedTempDir docstring clarified: it covers non-broker temp dirs only; plugin-data roots go through registerPluginDataDir. Verified: full npm test → 0 broker leak, 0 leftover test-data dirs; SIGINT mid-run → 0 leftover. Both reviewers (Codex + Claude subagent) ship. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0189cGaohsYdpkB4otKZnpAt
Owner
Author
🔍 Local review (cycle 2) — clean, shipReviewed locally (Claude subagent + Codex companion). Both ship.
Cycle-1 fixes re-verified correct (flag ordering, named Empirical: full Review cycle complete. Local mode does not auto-merge — merge is yours to trigger. |
This was referenced Jul 19, 2026
axisrow
added a commit
that referenced
this pull request
Jul 19, 2026
…#8) Closes #7 (rephrased from code, not the original misdiagnosis). The codex:codex-rescue subagent hung because it defaulted to a foreground `task` run; when Codex exceeded Claude's Bash-tool timeout the host auto- backgrounded the call, and the rescue model then violated its one-Bash-call rule by improvising `cat` / `sleep 60` / `until grep ...; do sleep 5; done` polling — which itself violates Claude Code's foreground-sleep restriction. The Codex turn had actually completed; the hang was the subagent's polling, not the companion. This was a behavioral-contract gap, not a broker bug (verified by code trace and the recorded transcript): the companion foreground path correctly blocks on state.completion, an explicit --cwd is preserved unchanged through to the broker process argv, and runtime SessionEnd already reaps brokers/sockets. The original issue's "wait pattern" and "wrong cwd" causes are refuted; the "no reaping" cause conflates a real test-harness leak (fixed in PR #5) with runtime behavior. Fix the contract that actually failed: - rescue now defaults to --background (rescue work is open-ended; a foreground run that the host auto-backgrounds at its Bash-tool timeout cannot be turned back into a foreground wait, so start in background). --wait still selects foreground explicitly. - terminal-on-timeout: if the single task Bash call returns by host Bash-tool timeout, the subagent makes no second call (no status/result/cat/sleep/ until-grep). The Codex turn keeps running and is recovered by the caller via /codex:status or /codex:result — never by the subagent. commands.test.mjs assertions updated to the new contract. Claude-Session: https://claude.ai/code/session_0189cGaohsYdpkB4otKZnpAt Co-authored-by: axisrow <axisrow@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
axisrow
added a commit
that referenced
this pull request
Jul 20, 2026
Security hardening release: leave-branch worktree cleanup (#12) + 4 security fixes (#18 symlink guard, #20 core.symlinks=false neutralize, #22 eliminate info/exclude write, #23 accumulation warn). rescue default --background (#8). test-teardown (#5). All verified with cycle-review. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes openai#163 (upstream tracking issue).
Problem
node --test tests/*.test.mjsleaves behind detachedapp-server-broker.mjsprocesses (all reparented to PID 1) plus thousands of temp dirs. On a developer machine this reached ~150 leaked processes (~2.6 GB RSS) and ~20k temp dirs.Root cause: production
spawnBrokerProcessspawns the brokerdetached:true+unref()so it survives the parent — correct in production, fatal in tests. node:test does not forward signals/exit to detached grandchildren, so when a test (or the whole run) ends, the broker keeps running.Why this is hard
The test process cannot capture the broker pid at spawn. The broker is born inside a grandchild subprocess via production code (
ensureBrokerSession→spawnBrokerProcess), in three independent paths:runtime.test.mjsspawnscodex-companion.mjs, which spawns the broker internally (~65 call sites, only 6 call SessionEnd)broker-lifecycle.test.mjsspawns a node-echild that callsensureBrokerSessionbroker-smoke.test.mjsforks the broker directly (already does proper teardown — the template)So the harness tracks workspace cws instead, and recovers
{endpoint, pid, sessionDir}deterministically frombroker.jsonat teardown (loadBrokerSession). This matches the central-registry sketch Wintersta7e proposed in openai#163.Design — two layers
node:test defaults to
--test-isolation=process(each test file is its own worker), so the teardown splits across scopes:Worker layer (
tests/helpers.mjs): a registry of temp dirs and broker-spawning workspaces.makeTempDirregisters the dir;run()registers its cwd when it lives under the temp root.process.on("exit"/SIGINT/SIGTERM)handlers reap every tracked broker using the SessionEnd sequence (loadBrokerSession→sendBrokerShutdown→teardownBrokerSession(killProcess: terminateProcessTree)→clearBrokerSession) andrm -rfthe tracked dirs.Parent layer (
tests/test-global-setup.mjs): a--test-global-setupmodule that runs once in the parent after all workers exit. Sweepsbroker.jsonunder everycodex-plugin-test-data-*root for brokers a crashed worker missed, then removes those roots.Env isolation (
tests/fake-codex-fixture.mjs):buildEnvnow pointsCLAUDE_PLUGIN_DATAat one ephemeral per-worker temp root, so tests no longer inherit the real plugin data dir via...process.env— the env-inheritance leak flagged in openai#163 / openai#487 part 1.Safety
Pid reuse: a stale
broker.jsonmay point at a pid the OS recycled into an unrelated process. OnlyterminateProcessTreewhen the endpoint probe confirms the broker is actually live — mirrors the safety check production already does inloadReusableBrokerSessionUnlocked(broker-lifecycle.mjs:193-216).No hangs:
sendBrokerShutdownbounded at 500ms,waitForBrokerEndpointat 150ms. The syncexitpath has zeroawaits — process reaping there is deferred to the parent sweep.Verification
Production code (
plugins/codex/scripts/) is untouched — test-only.Upstream
Will open against
openai/codex-plugin-cconce this settles in the fork. Independent of openai#490 (idle-timeout) — cross-linked, not based on it.cc @Wintersta7e @brettimus — you both hit this in openai#163. This implements the central-registry +
process.on("exit")approach you sketched, extended with a parent sweep for node:test process isolation and an endpoint-probe guard against pid reuse. Review welcome.