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 7 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".
| const remainingOwners = owners.filter((owner) => owner !== sessionId); | ||
| if (remainingOwners.length > 0) { | ||
| fs.writeFileSync( | ||
| stateFile, | ||
| `${JSON.stringify({ ...session, sessionId: remainingOwners[0], sessionIds: remainingOwners }, null, 2)}\n`, |
There was a problem hiding this comment.
When two owners of the same broker exit at nearly the same time, both SessionEnd hooks can read the same sessionIds before either write happens. Each hook then enters this remaining-owner branch, writes a state file that keeps only the other session, and skips shutdown; whichever write wins leaves broker.json pointing at an already-ended owner, so no later hook will tear the broker down. This affects users with multiple Claude sessions sharing one broker who close them concurrently; the owner removal needs a lock or compare/reload before deciding not to shut down.
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
sessionId:sessionIdinbroker.jsonon broker creation (resolveSessionIdreadsCODEX_COMPANION_SESSION_ID, which the SessionStart hook already sets). The reuse path is untouched, so ownership stays with the creator.teardownBrokersForSession(sessionId)scans the shared state root (resolveStateRoot()) and tears down everybroker.jsonwhosesessionIdmatches — independent of cwd.handleSessionEndcalls it withinput.session_idin addition to the existing cwd-based path (kept as a fast path / fallback for legacy brokers withoutsessionId).Scope
In scope: orphaning on graceful exit caused by cwd mismatch (the dominant path).
Out of scope (separate concerns): abnormal exit where SessionEnd never fires (SIGKILL/OOM, see #108), an idle-timeout fallback, and a parent-liveness watchdog. Those are complementary and tracked separately.
Changes
scripts/lib/state.mjs— extractresolveStateRoot()(used to scan all workspaces' state without a cwd);resolveStateDirnow reuses it (behavior unchanged).scripts/lib/broker-lifecycle.mjs—resolveSessionId(); recordsessionIdin the created broker session;teardownBrokersForSession(sessionId, {killProcess}).scripts/session-lifecycle-hook.mjs— exporthandleSessionEnd; callteardownBrokersForSession(input.session_id)after the cwd-based path. A smallisMainguard wraps the top-levelmain()so importing the module for tests does not trigger the blocking stdin read (CLI hook execution is preserved).Backward compatibility
STATE_VERSIONunchanged; thesessionIdfield inbroker.jsonis additive.loadBrokerSession/teardownBrokerSession/clearBrokerSessionsignatures and the cwd-based cleanup path are retained. Legacybroker.jsonfiles withoutsessionIdare simply skipped by the session scan and still handled by the existing cwd path.Tests
New
tests/broker-lifecycle.test.mjs(node:test), 12 cases coveringresolveStateRoot,resolveSessionIdprecedence,teardownBrokersForSession(cross-cwd teardown, non-matching sessionId left intact, legacy/no-sessionId ignored, empty-sessionId no-op), and a regression lock:handleSessionEnd({cwd: <different>, session_id: S})tears down a broker registered for another cwd.node --test tests/*.test.mjs: the new suite is 12/12; no new failures introduced.🤖 Generated with Claude Code