Repository navigation
refactor(mcp): split MCPClientManager and serialize connection work per server - #2661
mattzcarey wants to merge 5 commits into
Conversation
|
|
✅ agents import sizes: no significant changes ( |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
agents
@cloudflare/ai-chat
@cloudflare/codemode
hono-agents
@cloudflare/shell
@cloudflare/think
@cloudflare/voice
@cloudflare/worker-bundler
commit: |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
67c61e4 to
3974b04
Compare
3974b04 to
a0157df
Compare
a0157df to
e18bafd
Compare
e4d8bb6 to
28c1144
Compare
There was a problem hiding this comment.
Devin Review found 1 new potential issue.
⚠️ 1 issue in files not directly in the diff
⚠️ MCP client changes lose release entry
With the agents changeset removed, the MCP client fixes and changed connection behavior have no release entry. Changesets cannot include this PR in an agents version bump.
- ServerLanes queues connect, discovery, establish, OAuth callback and restore work per server; nested calls run inline, listeners run outside - OAuth callback parsing, state checks, code exchange and response rendering move to oauth-callback.ts - settleServer()/connectAndDiscover() replace Agent-side orchestration in addMcpServer - work that outlives removeServer no longer writes back; a code exchange that finishes after removal clears its tokens - RPC servers restore in the background instead of holding onStart
- forget() bumps a per-server generation so work still queued behind a removed server rejects with ServerLaneCancelledError instead of running - callback failure and stale-success paths check they still own the connection before touching state, so a re-registered server keeps its link
A callback queued behind work for a server that is then removed returns a failed callback result instead of rejecting, and settleServer / connectAndDiscover report the server as removed.
28c1144 to
0406eaf
Compare
MCPClientManagernow runs all connection work for a server through a per-server queue, so connect, discovery, OAuth callback completion and restore never overlap on one connection. Removing a server cancels its queued work, and work that is already running can't write back once the server is gone. RPC servers restore in the background afteronStart, the same way HTTP servers do.Stacked on #2660, and part of the same work on #2658. The manager was one 2,600-line file that kept background work as loose promises in a map, and that allowed these races:
conn.init()calls for one server ran at once and closed each other's clientremoveServer()wroteauth_urlor a session onto a server re-registered under the same idonStartawaited RPC restore, whose timeout is 60s, inside a 30s startup budget, so a hungMcpAgentcould reset the host object again and againThings callers will notice:
waitForConnections()first.AIChatAgentwaits by default; Think waits whenwaitForMcpConnectionsis set.connectToServer(),discoverIfConnected()orestablishConnection()still queued when its server is removed rejects withServerLaneCancelledError, and a queued OAuth callback returnsauthSuccess: falsewaitForConnections()also waits for user-started connects and discoveriesThe SQL, OAuth callback handling, AI tool conversion and URL policy moved out into
server-store.ts,oauth-callback.ts,ai-tools.tsandurl-policy.ts. The queue lives inserver-lanes.ts.pnpm run check, the agents workers suite and the client and server conformance lanes pass. New tests cover the queue, cancelling queued work on removal, the removal and re-registration races, and RPC restore not blocking startup.