fix(mcp): clean failed servers before reconnecting - #3939
Conversation
![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: 87d8d5c206
ℹ️ 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".
87d8d5c to
9ec0ad0
Compare
9ec0ad0 to
e0249ce
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e0249ce902
ℹ️ 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".
e0249ce to
37bf63c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37bf63cb18
ℹ️ 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".
baff106 to
47f0aae
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 47f0aaee3e
ℹ️ 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".
90a14ed to
ac7fb14
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac7fb14702
ℹ️ 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".
ac7fb14 to
a551db3
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a551db374c
ℹ️ 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".
a551db3 to
79e87fc
Compare
seratch
left a comment
seratch
left a comment
There was a problem hiding this comment.
Thanks for iterating on this. The failed-only cleanup need is valid, and the current worker and cancellation changes address the earlier findings. One lifecycle blocker remains: connect_all(), reconnect(), and cleanup_all() can still mutate the same manager and worker state concurrently.
After two reconnects share cleanup, both can pass the connection check and enqueue a new connect for the same server. A cleanup_all() racing with reconnect() can likewise stop the replacement worker or allow reconnect to recreate one after final cleanup. The concurrent-cleanup test proves command deduplication, but not the surviving operation or final manager state.
Please serialize all three public lifecycle entry points with a manager-owned lock, use private unlocked helpers for internal composition, and add controlled regressions for concurrent reconnects and reconnect versus cleanup_all(). Assert one connect, deterministic active/failed state, and no worker surviving final cleanup. Once covered, this should be ready for another review.
79e87fc to
1230746
Compare
|
Implemented in
The new sequential and parallel regressions cover concurrent failed-only |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 123074697b
ℹ️ 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".
1230746 to
15a98cf
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 15a98cfc5f
ℹ️ 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".
15a98cf to
a9a509f
Compare
There was a problem hiding this comment.
💡 Codex Review
Block connect_all after failed terminal cleanup
Fresh evidence in this revision: the new _blocked_reconnect_servers state is honored by failed-only and full reconnect, but connect_all() still rebuilds servers_to_connect from every disconnected server and clears errors without checking that block. After strict connect_all() rolls back a partial connect and terminal cleanup fails, a later connect_all() can call connect() again on the same server object even though its previous transport/session was never confirmed cleaned; route this path through the same failed-closed handling used by reconnect before retrying. .agents/references/local-mcp-server-lifecycle.mdL7-L11
ℹ️ 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".
a9a509f to
8cb2102
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8cb21021d7
ℹ️ 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".
8cb2102 to
3828517
Compare
There was a problem hiding this comment.
Thanks for addressing the previous lifecycle feedback. Re-reviewing the complete diff from v0.19.0, the original failed-only reconnect bug is valid, but this PR has expanded into a broader lifecycle redesign: default sequential mode now uses dedicated workers, all public lifecycle calls are serialized, and cleanup failures introduce terminal blocked state and new cancellation semantics.
Please reset this to the released manager structure and keep the fix focused: clean the deduplicated failed servers before reconnect(failed_only=True), retry only servers whose cleanup succeeds, preserve healthy servers and the existing sequential/parallel ownership model, and treat cleanup failure as terminal for that server without another connect attempt. Please retain focused regressions for successful partial-failure recovery and cleanup-failure no-retry in both modes.
Cross-caller sequential lifecycle, concurrent lifecycle operations, and cleanup-failure recovery should be handled separately if concrete need is established.
Signed-off-by: King Star <mcxin.y@gmail.com>
3828517 to
e906a57
Compare
|
Thanks for the clear scope direction. I reset the branch to the v0.19.1 manager The remaining fix is limited to The new sequential and parallel regressions cover successful partial-failure |
2 participants
Summary
MCPServerManager.reconnect(failed_only=True)retries failed servers withoutfirst cleaning resources that may have been opened before
connect()failed.For transports that require cleanup between attempts, the retry fails again or
runs against stale state.
This change keeps the released manager structure and existing sequential and
parallel ownership models. Failed servers are deduplicated, cleaned through the
existing cleanup path, and retried in their original order only when cleanup
succeeds. If cleanup fails or is cancelled, the server remains failed, the
cleanup error remains available in
manager.errors, and no new connection isattempted. Healthy servers are not cleaned or reconnected.
Test plan
before its first connection failure and succeeds only after cleanup.
second connection attempt and preserves failed/error state.
mainand pass with the fix..agents/skills/code-change-verification/scripts/run.sh; format, lint,typecheck, and the complete test suite passed.
Issue number
N/A - found through code-path review; no existing issue or pull request covers
the failed-only reconnect cleanup path.
Checks
.agents/skills/code-change-verification/scripts/run.shScope and risk
callers that perform rollback cleanup ignore that result and retain their
previous behavior.