Repository navigation
fix: release idle companion locks and recover broker startup - #836
pecord-ent wants to merge 16 commits into
Conversation
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 37cfcaa5a0
ℹ️ 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".
Idle shutdown now stops admitting clients before the slow app-server close, so a command arriving mid-drain starts a replacement broker instead of failing its first request. Reuse-existing callers verify the saved endpoint is reachable and fall back to a direct app-server when the broker expired. Startup lock recovery treats partial owner JSON like an empty lock (age check), and lock release no longer throws if the lock was reclaimed. Co-Authored-By: Claude Sonnet 5.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: 65b1926b4b
ℹ️ 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".
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f1a5f9609
ℹ️ 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".
…nore zombies in test teardown Co-Authored-By: Claude Sonnet 5.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: eb7e0cfc6f
ℹ️ 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".
Co-Authored-By: Claude Sonnet 5.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: 7bfff43d43
ℹ️ 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".
…perl Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
@agammann - ran into this bug again locally where I couldn't archive out some old chat sessions - it's not a big deal but it reminded me to go ahead and update the PR based on comments and to clean it up - let me know if there is anything else you'd like to see |
pecord-ent
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 36b93a876e
ℹ️ 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".
…onEnd cleanup, judge reclaim guard by owner liveness Co-Authored-By: Claude Sonnet 5.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: 555ec0c746
ℹ️ 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".
|
@codex review |
…ty collaboration sets Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
…ends Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3bd48081da
ℹ️ 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".
URL.pathname keeps %20 escapes and a leading slash before Windows drive letters, so the session-end e2e test could not spawn the hook from a checkout path containing spaces. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…ecting An empty write callback plus a 50ms sleep did not prove the broker had registered the request, so a slow runner could disconnect first and test idle expiry instead of pending-request retention. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Keep the idle timer with its active-turn and pending-RPC guards and the saved-endpoint readiness check. Drop the startup lock, reclaim guard, zombie detection, SessionEnd changes and collaboration tracking: with idle expiry a duplicate broker from a startup race exits on its own. The full version lives on pecord/companion-broker-hardening. Co-Authored-By: Claude Sonnet 5.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: 5301a69f67
ℹ️ 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".
| idleTimer = null; | ||
| if (shuttingDown || sockets.size || pendingRequests || activeTurns.size) return; | ||
| idleTimer = setTimeout(() => { | ||
| shutdown(server).then(() => process.exit(0)).catch((error) => { |
There was a problem hiding this comment.
On POSIX, if the OS reuses the expired broker's PID for another process-group leader before Claude's SessionEnd, this idle exit leaves broker.json pointing at that unrelated process. The still-unchecked consumer is handleSessionEnd() in session-lifecycle-hook.mjs: it passes the saved PID to teardownBrokerSession() with terminateProcessTree, which sends SIGTERM to that process group even when the broker endpoint is gone. The new endpoint-reachability check only protects connections, not teardown. Clear the matching saved session/PID on idle exit or verify process ownership before terminating it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5f2688c: on exit the broker removes broker.json when it still names its own endpoint, so SessionEnd no longer reads the expired pid, and a replacement's state is kept. Tests: an expired broker leaves no saved session; clearing for a different endpoint keeps the replacement's.
agammann
left a comment
agammann
left a comment
There was a problem hiding this comment.
Requesting changes on head 5301a69. Four lifecycle issues remain in the current code:
- Idle exit leaves
broker.jsonwith the expired PID. SessionEnd can later pass that PID toterminateProcessTreeafter POSIX has reused it for an unrelated process group. Please clear the matching saved session on exit or verify ownership before signaling it. Current finding. shutdown()still awaitsappClient.close()before closing the listener, so a replacement request can connect to a draining broker and fail on its first real RPC. Earlier finding is present again.activeTurnsis cleared only byturn/completed, but the supportedfinal_answerinferred-completion path can finish without that notification. The broker then retains the writer lock indefinitely. Earlier finding is present again.- Concurrent idle cleanup and replacement still use
existsSync()followed by unguardedunlinkSync()for the same files; the loser can throwENOENTand fail startup. Earlier finding is present again.
The old SessionEnd lock-wait comment no longer applies after the lock code was removed.
|
Tested this patch (the three Before: after about a week of normal use (reviews from several git worktrees plus unattended After: I started a broker by hand with Thanks for the fix. It would be great to see it in a release, since 1.0.6 is still the latest. |
…ear saved pid on exit The trim to the idle-shutdown fix dropped four behaviours the idle exit depends on: - An idle broker that kept its listener open while the app-server closed admitted a replacement client that then failed its first RPC. Stop accepting before the close. - A turn finished by its final answer alone (no turn/completed) pinned the broker, and so the conversation's writer lock, forever. Mirror the client's inferred completion, including waiting for pending collaborations and not letting a resumed thread inherit an interrupted turn's. - Cleanup used existsSync then unlinkSync on files a replacement's teardown removes concurrently; the loser threw ENOENT. Use rmSync with force. An expired broker also left broker.json pointing at its pid, which SessionEnd passes to terminateProcessTree after the OS may have reused it. The broker now clears the saved session on exit, only when it still names this broker's endpoint, so a replacement's state is kept. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f2688cba8
ℹ️ 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".
| function settleFinalAnswer(turnKey, threadId) { | ||
| if (pendingCollaborations.get(threadId)?.size) return; | ||
| setTimeout(() => { | ||
| if (pendingCollaborations.get(threadId)?.size || !finalAnswered.has(turnKey)) return; |
There was a problem hiding this comment.
When a final-answer-only turn's collaborations drain repeatedly within 250 ms, each call leaves the previous timeout armed, unlike scheduleInferredCompletion() in lib/codex.mjs. An older timeout can remove the turn between collaborations before the latest grace period expires; a collaboration starting in that remaining window is then unprotected after the caller disconnects, allowing idle shutdown to terminate unfinished work. A Linux fake-server reproduction with drains at 200/350 ms and another collaboration starting at 500 ms closed the app-server at approximately 700 ms with a 200 ms idle timeout, before that collaboration's 1200 ms completion; runAppServerTurn() correctly waited. Keep one resettable inference timer per turn.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1860c00: settleFinalAnswer cancels the turn's previous timer before arming a new one, so repeated drains share one grace period like scheduleInferredCompletion(). Test: collaborations drain at 100 and 200 ms and a third starts at 400 ms; without the fix the first timer releases the turn at ~350 ms and the app-server closes while the third is running, with the fix it stays up until that collaboration completes.
| if (loadBrokerSession(cwd)?.endpoint === endpoint) { | ||
| clearBrokerSession(cwd); |
There was a problem hiding this comment.
When a command replaces an idle broker while it is draining, the replacement can publish its state between loadBrokerSession() and clearBrokerSession(). The exiting broker then deletes the replacement's broker.json despite the endpoint comparison. Pausing the real detached broker immediately before this removal reproduced a null saved session while the replacement remained alive. Subsequent ensureBrokerSession() calls start another app-server instead of reusing it, and the normal lazy-broker SessionEnd path loses the saved reference needed to stop the replacement. Serialize conditional deletion with state publication, or use an ownership-safe state layout.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not changing this one. The window is the gap between a synchronous loadBrokerSession and the rmSync in clearBrokerSessionIfCurrent, and it only matters when a replacement publishes inside it. The result is a lost broker.json, so the next ensureBrokerSession starts one extra broker, and the unreferenced one exits after its 60 s idle timeout. Closing the window needs an atomic take-and-restore (rename, compare, relink) or a locked state layout, which this PR dropped from scope in 5301a69. Happy to take it as a follow-up if you want it.
…rain Each drain armed a new 250ms timeout without cancelling the previous one, so an older timeout could release the turn before the latest grace period ended. A collaboration starting in that window was then unprotected once the caller disconnected, and idle expiry could close its app-server. lib/codex.mjs keeps a single resettable timer per turn; the broker now does the same. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
@codex review |
agammann
left a comment
agammann
left a comment
There was a problem hiding this comment.
I rechecked head 1860c00 against my earlier review. The listener now closes before the app-server, the final-answer path tracks pending collaborations and resets its grace timer, cleanup uses forceful removals, and idle exit clears the matching saved endpoint. The added tests exercise those paths. I have not run them locally.
The ownership race raised in the later Codex review remains: a replacement can publish between the read and delete, losing its saved session. The stated 60-second cleanup bound only applies once that replacement is idle; an active turn or connected client keeps it alive, and SessionEnd can no longer find it through broker.json. I understand that a coordinated state layout was removed from this PR's scope, but I would retain this as an explicit follow-up rather than treat cleanup as guaranteed. AI-assisted review.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1860c00e0e
ℹ️ 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 (pendingCollaborations.get(threadId)?.size) return; | ||
| const timer = setTimeout(() => { | ||
| settleTimers.delete(turnKey); | ||
| if (pendingCollaborations.get(threadId)?.size || !finalAnswered.has(turnKey)) return; |
There was a problem hiding this comment.
For a disconnected caller whose main thread emits final_answer while a subagent is still active, this predicate can retire the parent turn before that subagent finishes; scheduleInferredCompletion() in lib/codex.mjs:378-389 explicitly waits for activeSubagentTurns as well. A parent collaboration starting after the 250-ms timer is then unprotected because the parent has already been removed from both activeTurns and finalAnswered. The last subagent's completion consequently makes the broker idle despite that pending collaboration. On Linux with a 200-ms idle timeout, I reproduced shutdown at approximately 900 ms for a collaboration running from 400 to 1500 ms, while runAppServerTurn() with the same events correctly waited and succeeded. Keep the parent's guard until its active subagents drain, then start the inference grace period.
Useful? React with 👍 / 👎.
Completed companion conversations can stay impossible to archive because detached app-servers retain their writer locks after the caller exits. Five affected conversations originally reproduced the problem; archiving through their owning servers succeeded. The same workaround cleared four more during the latest cleanup.
The broker now exits after 60 seconds (
CODEX_COMPANION_BROKER_IDLE_TIMEOUT_MS) with no connected clients, pending RPCs, or running turns, which releases the app-server and its writer lock. Saved conversations remain resumable through a replacement broker. The exit has to preserve these invariants:turn/completedis released the waylib/codex.mjscompletes it: after the thread's pending collaborations drain (one grace timer per turn, restarted by each drain), and a resumed thread does not inherit an interrupted turn's collaboration.broker.jsonwhen it still names its own endpoint, so SessionEnd cannot signal a pid the OS has reused, and a replacement's state is kept. The check and the delete are not atomic: a replacement publishing between them loses itsbroker.json, which costs one extra broker that idles out.Not included: serializing broker startup across processes. Simultaneous first commands can still each start a broker; the extras exit after the idle interval.
Validation: lifecycle suite 12 passed; the eight final-answer, collaboration, resume, drain, cleanup-race and saved-pid tests fail on the previous head. Full suite: 99 passed, 3 failed (
statusandresultruntime tests), which fail identically on the unmodified previous head in a fresh checkout. TypeScript build passes. Windows and Linux runtime validation is outstanding.