Stop brokers started by the test suite - #642
Conversation
The broker is deliberately long-lived so it can be reused across companion invocations. Nothing in the suite shuts it down, so every workspace a test touches leaves a broker and its app-server child running for the lifetime of the machine, and the count grows with every run. Track the workspaces handed out by makeTempDir, and add an `after` hook in runtime.test.mjs that stops each workspace's broker via the existing terminateProcessTree, then removes the temp directory. The repository root is passed in explicitly because some tests use it as the companion cwd. Measured on `node --test tests/runtime.test.mjs`, counting leftover processes matching "codex-plugin-test" three seconds after the run: before 58, after 0. Test results are unchanged at 61 total, 58 passing; the same 3 failures reproduce on an unmodified checkout. Co-Authored-By: Claude Opus 5 <[email protected]>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 54da005e6e
ℹ️ 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".
Signalling the saved PID and leaving broker.json in place means a later run against a stable workspace, such as the repository root the after hook passes in, can reread that PID and signal whatever process has since inherited it. It also leaves the pid file, log and session directory behind. Use the existing teardownBrokerSession to stop the process and remove those artifacts, then clearBrokerSession to drop the session file. Verified on a clean slate: after `node --test tests/runtime.test.mjs`, leftover processes 0 and leftover broker.json files 0. Test results unchanged at 61 total, 58 passing. Co-Authored-By: Claude Opus 5 <[email protected]>
54da005 to
325d5e7
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 325d5e7de4
ℹ️ 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".
ROOT is a stable workspace, so a developer may already have a live companion session registered for the checkout. Stopping it unconditionally meant a filtered run of this file that never starts a broker for ROOT would still kill that session. Sample the ROOT broker PID before any test runs, and tear it down only when the session afterwards is present and different, which is exactly the case where this run created it. Temp workspaces are unaffected: they are created by makeTempDir, so they always belong to the run. Verified from a clean slate: full `node --test tests/runtime.test.mjs` still leaves 0 processes and 0 broker.json files, results unchanged at 61 total, 58 passing. Co-Authored-By: Claude Opus 5 <[email protected]>
|
@codex review |
💡 Codex Reviewcodex-plugin-cc/tests/helpers.mjs Line 53 in bbf6ba9  Keep fixture cleanup from unlinking shared /tmp sockets
When cleanup runs after the ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Fixes the process leak reported in #640 (see this comment).
Problem
The broker is deliberately long-lived so it can be reused across companion invocations. Nothing in the test suite shuts it down, so every workspace a test touches leaves a broker plus its app-server child running for the lifetime of the machine — and the count grows with every run:
After a handful of full-suite runs I had 114 orphaned node processes.
Change
tests/helpers.mjsnow tracks the workspacesmakeTempDirhands out, and exposescleanupTempWorkspaces(), which stops each workspace's broker through the existingterminateProcessTreeand then removes the directory.tests/runtime.test.mjsregisters it in anafterhook, passing the repository root explicitly because some tests use it as the companion cwd.No production code is touched — this is test teardown only.
Verification
Counting processes matching
codex-plugin-testthree seconds afternode --test tests/runtime.test.mjs:Test results are unchanged. The same 3 failures (
status shows phases, hints, and the latest finished job,status preserves adversarial review kind labels,result returns the stored output for the latest finished job by default) reproduce identically on an unmodified checkout, so they are pre-existing and unrelated.Note
While looking into this I noticed the suite does not isolate
CLAUDE_PLUGIN_DATA:buildEnvonly prepends toPATH, so broker state is written into the developer's real plugin data directory rather than a temp one. That is a separate concern and out of scope here, but it may be worth isolating so a test run cannot disturb a live session.