fix(broker): self-terminate on idle to reap orphaned shared brokers (#450)#457
fix(broker): self-terminate on idle to reap orphaned shared brokers (#450)#457HoneyTyagii wants to merge 1 commit into
Conversation
…penai#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 openai#108, openai#380, openai#450). The timeout is configurable via --idle-timeout <ms> 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.
There was a problem hiding this comment.
Pull request overview
Adds a broker-side idle timeout to prevent orphaned shared/co-owned broker processes when session-side teardown is skipped or never runs (e.g., SIGKILL/OOM/crash), aligning with the remediation proposed in issues #108 / #450.
Changes:
- Implement broker self-termination after a configurable idle period with no connected clients (
--idle-timeout/CODEX_COMPANION_BROKER_IDLE_TIMEOUT_MS, default 30m). - Arm/disarm the idle timer based on client connect/disconnect events and on initial listen.
- Add integration-style tests that spawn the real broker against the fake codex fixture to validate timeout, “stay alive while connected”, and “disabled timeout” behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| plugins/codex/scripts/app-server-broker.mjs | Adds idle-timeout flag/env parsing and lifecycle logic to self-terminate when idle. |
| tests/broker-idle-timeout.test.mjs | New tests that spawn the broker and validate idle-timeout behavior end-to-end. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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; | ||
| } |
| import fs from "node:fs"; | ||
| import net from "node:net"; |
![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: 00d3f931c9
ℹ️ 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".
| if (!Number.isFinite(parsed) || parsed < 0) { | ||
| return DEFAULT_IDLE_TIMEOUT_MS; |
There was a problem hiding this comment.
When callers set --idle-timeout -1 or CODEX_COMPANION_BROKER_IDLE_TIMEOUT_MS=-1 to follow the new <= 0 disable contract, this branch replaces the value with the 30-minute default. Since armIdleTimer() only disables when idleTimeoutMs <= 0, those callers still get an idle shutdown instead of preserving externally managed lifecycle; return a non-positive value for parsed negatives instead of the default.
Useful? React with 👍 / 👎.
Summary
Fixes the shared/co-owned broker orphan described in #450 by adding a broker-side idle timeout, the resolution proposed in that issue (and the umbrella #108).
PR #381 keys broker teardown on the owning session(s):
broker.jsonrecordssessionIds, a reused broker adds the reusing session as a co-owner, andteardownBrokersForSessionshuts a broker down only once no owner remains. That correctly stops one session from tearing down a broker its siblings still use, but it leaves a gap:SessionEndhook (SIGKILL, OOM, crash, host reboot) leaves itssessionIdinbroker.jsonforever. Later owners exit gracefully, still see the dead owner as "remaining", and preserve the broker — but no future hook ever fires for the dead owner, so the broker is orphaned indefinitely.broker.jsonlock within theSessionEndbudget, it skips the entry and never retries, leaving itssessionIdbehind with the same result.Both share the same unresolvable core: a departed session's
sessionIdlingers and later teardown has no reliable, cross-platform way to tell it is dead. A hook-side liveness signal is not a sound fix (see the revertedprocess.ppid/kill(pid, 0)approach discussed in #381).Fix
Solve it broker-side:
app-server-broker.mjsnow self-terminates after an idle period 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. The session-side teardown from #381 remains a best-effort eager optimization; the idle timeout is the correctness backstop.Behavior
--idle-timeout <ms>or theCODEX_COMPANION_BROKER_IDLE_TIMEOUT_MSenv var. Default is 30 minutes. A value<= 0(or non-finite/negative) disables self-termination, preserving the previous behavior for callers that manage lifecycle themselves.shutdown()used bybroker/shutdown/SIGTERM(closes client sockets, closes the app-server client, removes the socket and pid file), then exits 0. A reuse check by later commands simply sees the endpoint gone and spawns a fresh broker.Tests
Added
tests/broker-idle-timeout.test.mjs(spawns the real broker against the fake codex fixture):--idle-timeout 0)All three pass locally. (Two unrelated suite failures are pre-existing Windows-only artifacts: the Unix-socket path assertion in
broker-endpoint.test.mjsand the symlink test in the review-context suite — neither touches this change.)Relates to #108, #380, #381.