Skip to content

fix: reuse pooled MCP client connections in pi extension - #52

Open
EdwardIrby wants to merge 6 commits into
mainfrom
fix/pi-mcp-client-reuse
Open

fix: reuse pooled MCP client connections in pi extension#52
EdwardIrby wants to merge 6 commits into
mainfrom
fix/pi-mcp-client-reuse

Conversation

@EdwardIrby

Copy link
Copy Markdown
Member

Summary

  • reuse pooled You.com MCP client connections across Pi tool executions
  • reconnect and retry once when a pooled connection drops
  • close pooled clients on session_shutdown
  • add tests for connection reuse, reconnect retry, and shutdown cleanup

Validation

  • bun run --cwd packages/pi test

EdwardIrby and others added 3 commits August 17, 2026 17:08
- Migrate from @modelcontextprotocol/sdk v1 to @modelcontextprotocol/client
  v2 (plus @modelcontextprotocol/server as a devDependency for tests)
- Pool one lazily connected MCP client per server so callTool no longer pays
  the full initialize handshake on every execution
- Reset and retry once on a fresh connection when the pooled session goes stale
- Close pooled clients on session_shutdown (reload/quit/new/resume/fork),
  terminating the server-side session before tearing down the transport
- Add optional server-config injection and a transport fetch seam so the
  lifecycle is tested against a real in-process MCP server
Add a source-grounded You.com search and contents skill for coding-agent web lookup and cited synthesis.

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Reverts 80a8c90 feat: add You web search skill.

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
@EdwardIrby
EdwardIrby requested a review from a team as a code owner August 20, 2026 18:26
@factory-droid

factory-droid Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Droid finished @EdwardIrby's task —— View job


Droid review complete.

The pooled-connection design is correct in the serial case and the new lifecycle tests pass, but the reset/retry and session_shutdown teardown paths close shared connections without coordinating with concurrent in-flight callers, a real concurrency hazard. No security issues were found.

Comment thread packages/pi/main.ts
const withPooledMcpClient = async <T>(server: McpServerTarget, fn: (client: Client) => Promise<T>): Promise<T> => {
try {
return await fn((await getMcpClient(server)).client)
} catch {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] resetMcpClient closes a shared pooled connection out from under concurrent in-flight callers

mcpClients is module-level shared state, and getMcpClient returns the same Client object to every concurrent caller keyed by server. When two callTool invocations to the same server run concurrently and one throws, withPooledMcpClient's catch calls resetMcpClient, which calls closeMcpConnection (transport.terminateSession() + client.close()) on that shared connection while the other caller's callTool is still in flight on it. That in-flight call then fails or hangs on a torn-down transport; that caller's own catch then calls resetMcpClient again and can close the fresh connection the first retrier just cached, cascading failures. The same class of hazard exists in closeMcpClients on session_shutdown. There is no in-flight reference counting to prevent closing a connection that outstanding callers are using. Trigger: two concurrent tool calls to the same MCP URL where one fails. Fix: track in-flight callers per connection and defer close until the last caller drains, or have each caller snapshot its own connection and only close the old one once all outstanding callers have finished.

- pass successful structuredContent to the model as pretty JSON
- keep tool error content unchanged when calls fail
- store structuredContent in details instead of the full MCP result
- fold MCP connection lifecycle tests into the main Pi spec

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
@factory-droid

factory-droid Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Droid encountered an error —— View job


Droid is reviewing code and running a security check…

Track active calls per pooled MCP client entry and defer closing stale entries until all concurrent users drain. This prevents one failing tool call from closing a shared connection while another call is still in flight.

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
@factory-droid

factory-droid Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Droid review complete.

LGTM — no new issues found. The previously reported concurrency hazard (resetMcpClient closing shared connections under concurrent in-flight callers) has been properly resolved by commit 01c5a9b, which adds activeCalls reference counting and closeWhenIdle deferral so connections are only torn down after the last caller drains. All six lifecycle tests pass and type checks are clean. The toToolResult details field change from full MCP result to structuredContent is a deliberate API change per the PR scope, with tests updated to match.

View job run

Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
@factory-droid

factory-droid Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Droid finished @EdwardIrby's task —— View job



Pass 2 validation complete. No candidate comments were produced by Pass 1, so there are no inline review comments to post.

Summary: The pooled MCP client connection design is correct: activeCalls reference counting and closeWhenIdle deferral properly prevent closing shared connections while concurrent callers are in flight, resolving the concurrency hazard reported against an earlier commit on this branch. The retry-once logic, session_shutdown cleanup, toToolResult error/structuredContent handling, and auth-vs-public cache key isolation are all sound. No high-confidence bugs or security vulnerabilities were found.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant