fix: tear down brokers by sessionId on SessionEnd to avoid orphaned worktree brokers (#380)#381
fix: tear down brokers by sessionId on SessionEnd to avoid orphaned worktree brokers (#380)#381hongsu wants to merge 15 commits into
Conversation
브로커 생성 시 CODEX_COMPANION_SESSION_ID를 broker.json에 sessionId 필드로 기록. resolveSessionId(options)는 options.sessionId → options.env → process.env → null 순서로 해소. 기존 세션 재사용 경로(early-return)는 변경 없음. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![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: 8aa56c3f48
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Compare the hook entrypoint and argv path after resolving realpaths so Node's import.meta.url canonicalization does not bypass direct execution when CLAUDE_PLUGIN_ROOT is a symlink. Add a runtime regression test that invokes SessionStart through a symlinked plugin root and verifies the exported session environment. Constraint: Node resolves imported module URLs to real paths while argv preserves the invoked symlink path. Rejected: Raw URL pathname comparison | fails for symlinked plugin installations. Confidence: high Scope-risk: narrow Directive: Keep hook direct-execution checks symlink-aware; SessionEnd cleanup depends on main() running from installed hook commands. Tested: node --test tests/runtime.test.mjs tests/broker-lifecycle.test.mjs; npm test Not-tested: npm run build fails before this change because generated app-server types do not match current source types.
hongsu
force-pushed
the
fix/broker-session-cleanup
branch
from
817197c to
82a372f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 82a372fc1f
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 55b5892f52
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Serialize broker owner updates with a per-state-file lock so concurrent SessionEnd hooks cannot leave an already-ended owner behind. Ensure broker teardown still runs when job cleanup throws, and bound broker shutdown RPC waits so unresponsive endpoints cannot hang SessionEnd. Add regressions for each path.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c22bb77eab
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A co-owner that dies without SessionEnd (SIGKILL, OOM, crash) stays in sessionIds forever, so the last graceful SessionEnd early-returns and the broker is orphaned even though every owning session is gone. Record a best-effort session pid (the SessionStart hook's parent, exported as CODEX_COMPANION_SESSION_PID) alongside each owner. teardownBrokersForSession now prunes co-owners whose recorded pid no longer exists before deciding whether the broker must stay up, and handleSessionEnd's early-return only respects owners that are still live. Owners without a recorded pid keep the previous assume-alive behavior, so legacy broker.json files are unaffected. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
fs.realpathSync throws ENOENT when process.argv[1] does not resolve to a real path (deleted script, loader indirection), which would crash the hook module at import time. Treat an unresolvable path as not-executed-directly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19a722c638
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The reuse path probed readiness before acquiring the state-file lock and then fell back to the pre-lock snapshot (loadBrokerSession(cwd) ?? existing). When the last owner's SessionEnd won the lock first, the fallback resurrected the just-shut-down broker: the caller got a dead endpoint and broker.json was rewritten to point at it. Reuse now trusts only the locked re-read: if broker.json is gone or points at a different endpoint, the attempt is retried once (a live replacement broker is reused) and otherwise falls through to spawning a fresh broker. The spawn path also re-reads the current state instead of tearing down the stale pre-lock snapshot. Regression-locked with a deterministic lock-hold test (torn-down broker is not resurrected; a fresh broker is spawned) plus a live-replacement reuse test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: da7a24beb1
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…urrency Revert the session-PID owner-liveness mechanism (recording process.ppid at SessionStart and pruning owners whose pid is gone). Under the packaged shell-form SessionStart hook, node's process.ppid is the ephemeral sh that wraps the command, not the Claude session, so the recorded pid is dead almost immediately. That made every shared-broker co-owner look dead, so any single session exit tore down a broker still in use by its siblings — regressing the shared-ownership guarantee. The dead-co-owner-orphan case it targeted is the abnormal-exit concern already deferred to the broker idle-timeout (openai#108). Also fix two concurrency defects surfaced while reviewing that code: - teardownBrokersForSession aborted the entire state-root scan when any single broker.json.lock could not be acquired within the timeout, so a fresh lock held by an unrelated/stuck hook left the ending session's own brokers uncollected. Lock timeouts are now caught per entry and the scan continues. - ensureBrokerSession's fresh-broker spawn+persist ran outside the state-file lock, so two racing sessions could each spawn a broker and clobber broker.json, orphaning one broker with no record to clean it later. Spawn is now serialized under the lock (adopt a live broker written by a racing session, else spawn), with an unlocked best-effort fallback only when the lock cannot be acquired. - The state-file lock now stamps an owner token so a stale reclaim by another waiter cannot have its lock directory deleted by the original holder's release path. Tests: drop the pid-injection tests; add a locked-entry-skip teardown test and a dead-record fresh-spawn test. Broker suite 19/19; no new failures in the full suite (the 9 remaining failures are pre-existing environmental cases present at the merge base). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60671d2b81
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Follow-up hardening from self-review of the session-keyed broker teardown: - SessionEnd previously skipped the cwd fallback whenever broker.json still had any owner. When the session-keyed teardown was skipped under lock contention the ending session was still listed as owner, so the fallback was skipped and the broker left behind. The guard now checks for an owner OTHER than the ending session (hasOtherBrokerSessionOwners), so a broker owned only by the ended session still falls through to the cwd teardown. - teardownBrokersForSession bounded only lock acquisition, not the graceful shutdown wait, so several unresponsive endpoints could each burn the full shutdownTimeoutMs and push the scan past the SessionEnd hook budget. The shutdown wait is now capped by the remaining scan budget; out of budget skips the RPC and terminates the broker process directly. Remove the now-unused hasBrokerSessionOwners export. Add regression tests for the cwd fallthrough under lock contention and the budget-capped shutdown wait. A lock-contention variant of the dead-co-owner orphan (a skipped owner lingers in sessionIds) is a best-effort limitation backstopped by the broker idle timeout, tracked in openai#450. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee60490e14
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Two more correctness fixes from self-review of the SessionEnd cleanup path: - A stale broker.json with a corrupt/unsupported endpoint made sendBrokerShutdown throw synchronously (parseBrokerEndpoint), which — since the teardown scan only swallows lock timeouts — aborted the entire scan and skipped the forced process/file teardown for that record and every later same-session broker. sendBrokerShutdown is now best-effort: a connect/parse failure resolves instead of rejecting, so callers always fall through to teardownBrokerSession (which already guards its own endpoint parsing). - The unlocked ownership pre-check could observe broker.json mid-write (plain writeFileSync) and treat the resulting JSON.parse failure as "not owned", skipping a broker the ending session actually owns. broker.json is now written atomically (temp + rename) so readers never see a torn file, and a pre-check parse failure falls through to the authoritative locked re-read instead of skipping the entry. Add regression tests: corrupt-endpoint teardown, unparseable broker.json in the scan, and atomic saveBrokerSession (no temp-file litter). Broker suite 27/27. The lock-contention stale-owner orphan remains a best-effort limitation backstopped by the broker idle timeout (openai#450). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 67cad78570
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| if (sessionId) { | ||
| await teardownBrokersForSession(sessionId, { |
There was a problem hiding this comment.
When a session has a background task launched with --cwd pointing at a worktree, that job is stored under the worktree state, but cleanupSessionJobs(cwd, sessionId) only inspects the hook's session cwd. This new session-wide broker teardown still finds the worktree broker by sessionId and shuts it down, so the detached worker is interrupted while SessionEnd never cancels/removes the corresponding job record; scan the same state root for session-owned jobs before tearing down cross-cwd brokers, or avoid shutting down brokers that still have live jobs for this session.
Useful? React with 👍 / 👎.
1 participant
Summary
Fixes #380.
handleSessionEndtears down the broker vialoadBrokerSession(input.cwd), andbroker.jsonis stored under a cwd-derived hash (resolveStateDir). When a broker was spawned for a different cwd than the session's cwd, the lookup misses, nobroker/shutdownis sent, and the broker is orphaned — even on graceful exit (/quit, SIGTERM).This is structural in a common workflow: a session runs in the main tree (session cwd = repo root) but runs reviews against a git worktree (broker cwd = worktree). Reviewing a worktree's uncommitted working tree requires broker cwd = worktree, so the mismatch is unavoidable, and graceful exit still leaks the broker.
Fix
Decouple broker cleanup from cwd by keying it on the owning session(s):
broker.json: broker creation records the creating session (resolveSessionIdreadsCODEX_COMPANION_SESSION_ID, set by the SessionStart hook), and reusing a ready broker adds the reusing session as a co-owner (sessionIds). A broker shared by several sessions is torn down by the last owner to end, not by whichever exits first.teardownBrokersForSession(sessionId)scans the shared state root (resolveStateRoot()) and, for everybroker.jsonowned by the ending session, removes that owner and shuts the broker down once no owner remains — independent of cwd.handleSessionEndcalls it withinput.session_idbefore the existing cwd-based path. The cwd fallback still runs unless the broker has an owner other than the ending session, so a broker left owned only by the ending session (e.g. its session-keyed teardown was skipped under lock contention) is not stranded.Scope and the best-effort contract
In scope: orphaning on graceful exit caused by cwd mismatch (the dominant path), and keeping shared/concurrent broker access consistent.
The session-side teardown here is a best-effort eager optimization, not the correctness guarantee. It cannot cover a session that owns/co-owns a broker but disappears without running
SessionEnd(SIGKILL/OOM/crash, or an owner whose teardown was skipped under lock contention): itssessionIdlingers inbroker.jsonand no later hook can tell it is dead. There is no reliable cross-platformsessionId→liveness signal from a hook (an earlier iteration recordedprocess.ppid, but under the packaged shell-form SessionStart hook that is the ephemeral wrapping shell, dead on arrival; it was reverted). The correctness backstop for all stale-owner cases is a broker-side idle timeout, tracked in #450 / #108.Concurrency hardening
Broker state is shared across sessions, so
broker.jsonmutations are serialized with a per-file lock (withBrokerStateFileLock, mkdir-based) plus:broker.json, orphaning one. If the lock can't be acquired in time, the fallback still adopts a live recorded broker and only discards one that fails its readiness probe.broker/shutdownwaits are capped by a total scan budget kept under the 5s SessionEnd hook timeout.isExecutedDirectly()no longer crashes at import whenprocess.argv[1]doesn't resolve.Changes
scripts/lib/state.mjs— extractresolveStateRoot()(scan all workspaces' state without a cwd);resolveStateDirreuses it (behavior unchanged).scripts/lib/broker-lifecycle.mjs—resolveSessionId();sessionIdsco-ownership on create/reuse;withBrokerStateFileLock(owner token, atomic stale reclaim, typed timeout); lock-serializedensureBrokerSession;teardownBrokersForSession(sessionId, {killProcess, shutdownTimeoutMs, lockTimeoutMs, budgetMs})with unlocked ownership pre-check, per-entry lock-timeout skip, and budget-capped shutdown;hasOtherBrokerSessionOwners; boundedsendBrokerShutdown.scripts/session-lifecycle-hook.mjs— exporthandleSessionEnd; run session-keyed teardown (short lock timeout + scan budget under the hook's 5s) before the cwd path, which now only backs off for an owner other than the ending session; job-cleanup failures no longer skip broker teardown. AnisMainguard wrapsmain()so importing for tests doesn't block on stdin (CLI hook execution preserved).Backward compatibility
STATE_VERSIONunchanged;sessionId/sessionIdsinbroker.jsonare additive.loadBrokerSession/teardownBrokerSession/clearBrokerSessionsignatures and the cwd-based path are retained. Legacybroker.jsonfiles without ownership records are skipped by the session scan and still handled by the cwd path.Tests
tests/broker-lifecycle.test.mjs(node:test) coversresolveStateRoot,resolveSessionId,teardownBrokersForSession(cross-cwd teardown, non-matching sessionId intact, legacy/no-sessionId ignored, empty-sessionId no-op, per-entry lock-timeout skip, unrelated-workspace lock never waited on, budget-bounded scan, budget-capped shutdown, unresponsive-endpoint timeout, stale-lock reclaim, concurrent-SessionEnd race via child processes), shared-broker ownership transfer on reuse, reuse-path lock-race (no resurrection; live replacement reused), lock-serialized fresh spawn, andhandleSessionEndregressions (cwd-mismatch teardown, shared-cwd owner removal, cwd fallthrough when session teardown is skipped under contention, teardown despite job-cleanup failure), plus runtime coverage for SessionStart env exports and symlinked plugin roots.node --test tests/*.test.mjs: broker suite green; no new failures (the remaining failures are pre-existing environmental cases present at the merge base).🤖 Generated with Claude Code