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 22 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.
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".
SessionEnd에서 세션 소유 작업을 전체 workspace 상태에서 먼저 정리합니다. 상태 탐색이 불완전한 경우 교차 workspace 브로커 종료를 보류합니다. 잠금 경합, stale lock, 공유 시간 예산 회귀 테스트를 보강합니다.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab2d491a02
ℹ️ 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".
세션 종료와 브로커 재사용이 겹쳐도 새 소유자의 브로커를 종료하지 않도록 종료 의도와 소유자 갱신을 잠금 안에서 재검증합니다. 작업 상태 갱신과 stale lock 회수를 원자화하고 교차 cwd 정리 경쟁을 회귀 테스트로 고정합니다.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3cb8e208ff
ℹ️ 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".
SessionEnd 정리 순서 회귀 수정: - state lock 안에서 세션 job 종료 신호를 먼저 전송 - artifact 삭제 실패 시 job state를 보존해 재시도 가능하게 유지 - 삭제 오류에도 broker teardown이 계속되는 회귀 테스트 보강
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 45d45e81cf
ℹ️ 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".
SessionEnd 정리 안전성 및 테스트 정돈: - job state를 읽거나 lock 할 수 없으면 broker teardown 보류 - 종료된 owner의 fresh state lock을 즉시 안전하게 회수 - 더 강한 lock 경계 테스트에 포함된 중복 테스트 삭제
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e74ed592c1
ℹ️ 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".
브로커 수명주기 정리 강화: - 종료된 세션의 spawn 및 재사용을 marker로 차단 - lock, scan, deadline 실패를 불완전 정리로 전파 - 세션 종료 동시성 회귀 테스트 확장
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1eac442ac6
ℹ️ 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".
브로커 재사용 경쟁 조건 보완: - ready fast-path lock에 세션 소유 정보 게시 - SessionEnd scan이 대기 중 joiner를 marker로 차단 - 동시성 회귀 테스트 추가
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 01c70f57c6
ℹ️ 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".
세션 종료와 broker admission의 경쟁 조건을 구조적으로 차단: - 종료 상태를 전역 단조 tombstone으로 게시해 다른 세션이 소비하지 않도록 변경 - broker, 제공 endpoint, direct fallback의 연결 전후에 종료 세션 admission 검사 - lock 대기와 초기화 경계를 재현하는 결정적 회귀 테스트 추가
1 participant
Summary
Fixes #380.
SessionEndpreviously looked upbroker.jsononly from the hook cwd. A broker started for a different--cwdworktree therefore survived graceful session exit. This change tracks broker ownership by Claude session, cleans session-owned jobs and brokers across the shared state root, and makes an ended session permanently ineligible to re-enter broker or direct app-server startup paths.Fix
sessionId/sessionIdsinbroker.json; a shared broker is shut down only after its last active owner ends.state.jsonsession-job removal so concurrent job creation cannot be overwritten by a stale read/modify/write.Concurrency and filesystem hardening
Compatibility
STATE_VERSIONis unchanged; broker ownership fields are additive.SessionEndstill rely on the broker-side idle-timeout work tracked in Shared/co-owned broker orphaned when an owning session exits without SessionEnd #450 / Broker process not cleaned up on session exit — no idle timeout #108.Validation
npm test— 156/156 passednpm run build— passednpm run check-version— passedgit diff --check— passed