fix(app-server): unsubscribe task threads after client disconnect - #707
fix(app-server): unsubscribe task threads after client disconnect#707thossullivan wants to merge 9 commits into
Conversation
Codex Review SummaryThis 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. |
sylvesterkaczmarek
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
One race still looks possible here. addThreadOwner() cancels a retry timer, but it doesn't do anything about a thread/unsubscribe already in pendingUnsubscribes. If the last owner disconnects and that request is in flight while another client resumes the same thread, a late unsubscribe can leave the shared app-server connection unsubscribed even though threadSockets has a new owner.
I don't see a test for reacquiring a thread during an in-flight unsubscribe. Could we re-check ownership when the unsubscribe settles (and restore the subscription if needed), or otherwise serialize that transition?
![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: a4548b856d
ℹ️ 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".
a4548b8 to
1203f5d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1203f5d522
ℹ️ 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 thread resumed while its automatic thread/unsubscribe was still in flight was never released again: the later release reused the stale pending request, so the shared app-server connection stayed subscribed after every client had closed. If the app-server had processed the unsubscribe after the resume, the new owner would also have lost its subscription. Never reuse a pending unsubscribe. A release chains a fresh request behind any in-flight one and re-checks ownership before sending. A resume or review that provisionally claims a thread waits, bounded to five seconds, for an in-flight unsubscribe of that thread before it is dispatched, so a late unsubscribe cannot overtake the new subscription and a hung cleanup cannot wedge the broker. Reply to a failed request and clear the busy state before releasing provisional owners for the same reason. Add the unsubscribe-delayed and resume-fails-unsubscribe-hangs fixture behaviors, four regression tests, and assert the retry bound in the existing failure test.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc227197c7
ℹ️ 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".
When the bounded wait for an in-flight thread/unsubscribe expires, do not send the resume anyway: that would race the outstanding unsubscribe and could leave a tracked owner without an upstream subscription. Reject the request with a retryable error, release the provisional claim, and clear the busy state so other clients continue.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f0ddd6b95b
ℹ️ 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 release that followed a timed-out claim installed a fresh cleanup wrapper that waited for the original unsubscribe for only another five seconds, then settled as skipped once a retried claim owned the thread. The retry could then resume upstream while the original unsubscribe was still in flight. Make a wrapper wait for its predecessor without a bound so the pending entry never settles before every underlying request has, and reject retried claims until then.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 93b5660d21
ℹ️ 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".
An explicit thread/unsubscribe for an unowned thread queued behind a hung automatic unsubscribe without a bound while its socket held the busy slot, so every other client stayed busy. Apply the same bounded wait and retryable rejection to explicit unsubscribes that the provisional claims already use. Two related gaps closed in the same sweep: - A release that arrives while an earlier cleanup is still queued now shares that queued request instead of adding another link to the chain. A queued request re-checks ownership when it sends, so this is safe, and repeated retries against a hung upstream no longer grow an unbounded chain of duplicate unsubscribes. - A child thread whose cleanup is still outstanding is no longer handed to new owners of its parent by a later notification.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 68b4029524
ℹ️ 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 child notification that arrived while a provisional resume or review was waiting or in flight attributed the child to the claiming socket, and a rejected or failed claim released only the parent. The child then stayed owned until the socket closed. Record children inherited through an open claim and release them together with the claim when it is rejected or fails.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd45841d78
ℹ️ 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".
…claim A grandchild spawned while a provisional claim was open has an inherited child as its source, not the claimed root, so it was owned but omitted from the rollback. Record descendants whose source is either a claimed root or an already inherited thread.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7fba1e2d0
ℹ️ 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".
…eaves When the final owner sends thread/unsubscribe and disconnects before the upstream request fails, nothing retried the cleanup: the owner was already removed, so the close path had nothing to release, and the explicit path restored ownership only to a still-open socket. Schedule the bounded automatic retry in that case. Test fixture: write state atomically so a test never reads a partial document, and add a delayed single-failure unsubscribe behavior for the new regression test.
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. 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". |
|
Thank you so much for the review and for the catch! Confirmed, it did reproduce deterministically. With a fake app-server that delays the unsubscribe reply, "A closes, unsubscribe in flight, B resumes, B closes" reused the stale pending request, so no second unsubscribe was ever sent and the thread stayed subscribed. The variant where a late unsubscribe overtakes the resume is not reachable on the real app-server, which serializes Since your review the branch gained several commits to cover several of the various edge cases, but they do implement one design rather than seven patches, and I have added a table of them to the description. The rules:
I chose serialization over restoring the subscription from the broker. A broker-issued Tests added for this thread of findings, each failing on the commit before its fix:
The focused suite is 22 tests, Codex review reports no findings at |
|
*
*
3 Eyl 2026 Per 16:51 tarihinde thossullivan ***@***.***> şunu
yazdı:
… *thossullivan* left a comment (openai/codex-plugin-cc#707)
<#707 (comment)>
@sylvesterkaczmarek <https://github.com/sylvesterkaczmarek>
Thank you so much for the review and for the catch! Confirmed, it did
reproduce deterministically. With a fake app-server that delays the
unsubscribe reply, "A closes, unsubscribe in flight, B resumes, B closes"
reused the stale pending request, so no second unsubscribe was ever sent
and the thread stayed subscribed.
The variant where a late unsubscribe overtakes the resume is not reachable
on the real app-server, which serializes thread/unsubscribe and
thread/resume per thread ID on one connection, but the leak variant is.
Since your review the branch gained several commits to cover several of
the various edge cases, but they do implement one design rather than seven
patches, and I have added a table of them to the description.
The rules:
- *A cleanup request that has been sent is never reused.* A later
release chains a fresh request behind it and re-checks ownership before
sending. A release that arrives while a request is still queued shares that
request. (fc22719, 68b4029)
- *Nothing acquires or touches a thread while its cleanup is
outstanding.* A resume, review, or explicit unsubscribe waits up to
five seconds for in-flight cleanup of that thread. On expiry it is rejected
with a retryable error and releases its claim, instead of racing or queuing
behind the outstanding request. Retries keep being rejected until the hung
request settles. (fc22719, f0ddd6b, 93b5660, 68b4029)
- *A claim is transactional.* Everything it acquired, including
subagent threads and their descendants inherited while it was open, rolls
back with it when it is rejected or fails. (fd45841, e7fba1e)
- *The busy slot never waits on cleanup.* Error replies precede
releases. (fc22719)
- *Failed cleanup retries with bounded backoff*, also when an explicit
unsubscribe fails after its requester disconnected. (162f7e8)
*I chose serialization over restoring the subscription from the broker.*
A broker-issued thread/resume would run with default parameters, can fail
on its own, and races the client's next request. Nothing in the final code
depends on the app-server's per-thread ordering.
Tests added for this thread of findings, each failing on the commit before
its fix:
- broker serializes a resume behind an in-flight unsubscribe
- broker fails a resume when an in-flight unsubscribe outlives the
bounded wait
- broker keeps rejecting claims until a hung unsubscribe settles
- broker rejects an explicit unsubscribe that would queue behind a
hung cleanup
- broker rolls back child threads inherited through a failed
provisional claim
- broker retries cleanup when a client disconnects during a failing
explicit unsubscribe
The focused suite is 22 tests, npm test is 113 passed, and real-process
UAT with Codex CLI 0.150.1 passes graceful and abrupt closes, a companion
task through the broker, and six reacquire runs with notSubscribed probes
after every release.
Codex review reports no findings at 162f7e8. Ready for another look if /
when you have time.
—
Reply to this email directly, view it on GitHub
<#707?email_source=notifications&email_token=B3WU6VUILDI5V2H3R7B5XPD5NFZPVA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKNJSGY4DEMJXGU4KM4TFMFZW63VKON2WE43DOJUWEZLEUVSXMZLOOSWGM33PORSXEX3DNRUWG2Y#issuecomment-5526821758>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/B3WU6VRILSFC7ZXBIL463XL5NFZPVAVCNFSNUABGKJSXA33TNF2G64TZHMYTCOJWGMZDCMJWGI5US43TOVSTWNJTGE2DQMBVGM4TRILWAI>
.
Triage notifications, keep track of coding agent tasks and review pull
requests on the go with GitHub Mobile for iOS
<https://github.com/notifications/mobile/ios/B3WU6VRAMG4FY4VDDNMCBF35NFZPVA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKNJSGY4DEMJXGU4KM4TFMFZW63VKON2WE43DOJUWEZLEUVSXMZLOOSVGM33PORSXEX3JN5ZQ>
and Android
<https://github.com/notifications/mobile/android/B3WU6VW2T6D7MXCDGDL53S35NFZPVA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKNJSGY4DEMJXGU4KM4TFMFZW63VKON2WE43DOJUWEZLEUVSXMZLOOSXGM33PORSXEX3BNZSHE33JMQ>.
Download it today!
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
3 participants
Summary
The app-server broker multiplexes plugin tasks through one shared upstream connection. Starting or resuming a thread subscribes that connection to thread events.
Before this change, closing a downstream task socket removed request and stream routing state but retained the thread subscription. Repeated tasks could therefore accumulate thread runtimes and MCP process trees inside one broker session.
This change tracks thread ownership for every downstream socket. The broker sends
thread/unsubscribeonly after the final downstream owner releases a thread, and it never lets a new claim race an unsubscribe that is still in flight.Implementation
The broker maintains ownership in both directions. Downstream sockets reference thread IDs, and thread IDs reference their downstream owners. This structure preserves a shared subscription while another client still owns the thread.
Ownership covers started, resumed, forked, detached review, subagent, and automatically created child threads. Direct request results establish root ownership. Subagent notifications use
parentThreadIdor collaboration sender and receiver IDs to inherit ownership from the causal parent. They do not use whichever downstream client happens to be active when a delayed notification arrives, and a child whose cleanup is still outstanding is not handed to new owners.Requests from each downstream socket run in order. This prevents overlapping resume or review requests from releasing another request's provisional claim. A response received after its downstream socket closes also triggers immediate cleanup.
A
thread/unsubscribethat has already been sent is never reused. A later release chains a fresh request behind any in-flight one, waits for it to settle, and re-checks ownership before sending; releases that arrive while a request is still queued share that request instead of growing the chain. A resume, review, or explicit unsubscribe that touches a thread with an in-flight unsubscribe waits up to five seconds for it before it is dispatched. If that wait expires, the broker rejects the request with a retryable error and releases any provisional claim instead of racing or queuing behind the outstanding unsubscribe. Retried requests keep being rejected until the hung request settles. A failed request replies and clears the busy state before it releases its provisional claim, so a hung upstream cleanup cannot wedge the broker for other clients.Normal socket closure and abrupt disconnection use the same final-owner cleanup path. Automatic unsubscribe failures are logged and retried with bounded backoff, including when an explicit unsubscribe fails after its requester has disconnected. A claim that is rejected or fails rolls back every thread it acquired, including subagent threads inherited while it was open. Explicit downstream unsubscribe requests continue to preserve subscriptions owned by other clients.
Scope
This change does not alter MCP configuration or depend on Basic Memory. Basic Memory only provided visible reproduction evidence for the host-level lifecycle defect.
Codex retains unsubscribed threads during its documented 30-minute inactivity period. This change starts that bounded cleanup period but does not promise immediate MCP process termination.
Testing
The integration suite covers normal completion, shared resumed threads, detached reviews, forks, active-turn disconnection, subagents, late child threads, collaboration-only child discovery, explicit unsubscribe, and upstream failures.
The adversarial regression cases cover delayed child notifications during an unrelated task, overlapping same-socket resume requests, reacquiring a thread during an in-flight unsubscribe, a hung upstream unsubscribe during a resume and during a failed request, retried claims and explicit unsubscribes against a hung cleanup, child and nested subagent rollback after a failed claim, cleanup retry after a disconnecting explicit unsubscribe, retry cancellation on reacquire, the retry bound, transient unsubscribe retries, and test resource cleanup.
node --test tests/broker-subscriptions.test.mjsnpm testnpx tsc -p tsconfig.app-server.json --noEmitReal-process verification
Codex CLI 0.150.1 with Basic Memory.
unsubscribednotSubscribedBoth cases started a real Basic Memory wrapper and Python server. Every isolated test descendant exited during cleanup.
After the review rounds, patched UAT covered both downstream closure modes, a companion task through the broker, and thread reacquisition.
notSubscribednotSubscribednotSubscribedafter every releaseThree further runs of each reacquire scenario, without the exit watch, also returned
notSubscribedafter every release. In the reacquire runs, the reacquiring client's ownthread/unsubscribereturnedunsubscribed, which confirms its subscription was live. Brokers and app-server processes remained alive through natural MCP unload. Final cleanup left zero isolated descendants.Changes since the first review
The first review found that a thread resumed while its automatic
thread/unsubscribewas still in flight was never released again. The commits after 1203f5d close that race and the edges the automated review found around it. Each one is a small, named rule rather than a new subsystem, and each carries a regression test that fails on the commit before it.The broker never records an owner for a thread while cleanup for that thread is outstanding upstream, never lets a socket wait on cleanup without a bound, and rolls back everything a failed claim acquired. It does not depend on the app-server's per-thread request ordering. Codex review reports no findings at 162f7e8.
Related work
This change complements #543, #660, and #680. Those items address broker and app-server lifetime, while this change releases completed task threads inside a running broker. The change does not depend on either lifecycle pull request.
Fixes #706