Implement backend session persistence with client pooling #3571
Conversation
Add session-scoped MCP client pooling to preserve backend state across
multiple tool calls within the same vMCP session. Previously, each tool
call created a new backend client and session, breaking stateful backends
that rely on session persistence (e.g., Playwright browser contexts,
database transactions, conversation state).
Implementation:
- BackendClientPool: Thread-safe pool that caches initialized MCP clients
per (sessionID, backendID) tuple with GetOrCreate, MarkUnhealthy, and
Close methods
- pooledBackendClient: Decorator that wraps httpBackendClient to add
session-scoped pooling without changing the BackendClient interface
- Refactor client.go to extract callToolWithClient, readResourceWithClient,
and getPromptWithClient helpers for code reuse
- Add clientPool field to VMCPSession for storing pool per session
- Propagate session ID through context via SessionIDContextKey
- Automatic cleanup via session TTL (no manual lifecycle management)
- Connection error detection with MarkUnhealthy for pool eviction
Testing:
- Comprehensive test coverage (~700 lines across pool_test.go and
pooled_client_test.go)
- Unit tests for pool operations, concurrency, error handling
- Integration tests for session-scoped pooling and lifecycle
- All tests passing with zero lint issues
Fixes #3062
yrobla
requested review from
ChrisJBurns,
JAORMX,
amirejaz,
blkt,
dmjb,
jerm-dro and
jhrozek
as code owners
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Large PR Detected
This PR exceeds 1000 lines of changes and requires justification before it can be reviewed.
How to unblock this PR:
Add a section to your PR description with the following format:
## Large PR Justification
[Explain why this PR must be large, such as:]
- Generated code that cannot be split
- Large refactoring that must be atomic
- Multiple related changes that would break if separated
- Migration or data transformationAlternative:
Consider splitting this PR into smaller, focused changes (< 1000 lines each) for easier review and reduced risk.
See our Contributing Guidelines for more details.
This review will be automatically dismissed once you add the justification section.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3571 +/- ##
==========================================
+ Coverage 65.49% 65.53% +0.04%
==========================================
Files 404 406 +2
Lines 39676 39930 +254
==========================================
+ Hits 25984 26169 +185
- Misses 11688 11739 +51
- Partials 2004 2022 +18 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR implements backend session persistence by introducing session-scoped MCP client pooling, enabling stateful backend operations (e.g., Playwright browser contexts, database transactions) across multiple tool calls within the same session.
Changes:
- Added
BackendClientPoolto cache and reuse initialized MCP clients per (sessionID, backendID) tuple - Introduced
pooledBackendClientdecorator wrappinghttpBackendClientto enable pooling without interface changes - Refactored
httpBackendClientto extract helper methods (callToolWithClient,readResourceWithClient,getPromptWithClient) for code reuse between pooled and ephemeral clients
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/vmcp/session/vmcp_session.go | Added clientPool field and getter/setter methods to store pool per session |
| pkg/vmcp/server/server.go | Wrapped backend client with pooling decorator during server initialization |
| pkg/vmcp/discovery/middleware.go | Added session ID to context for downstream pooled client access |
| pkg/vmcp/client/pool.go | Implemented BackendClientPool with thread-safe GetOrCreate, MarkUnhealthy, and Close methods |
| pkg/vmcp/client/pooled_client.go | Implemented pooledBackendClient decorator with connection error detection |
| pkg/vmcp/client/pooled_client_test.go | Added comprehensive tests for pooled client behavior and integration |
| pkg/vmcp/client/pool_test.go | Added comprehensive tests for pool operations and concurrency |
| pkg/vmcp/client/client.go | Refactored methods to extract reusable helpers for both pooled and ephemeral clients |
| pkg/transport/session/manager.go | Added SessionIDContextKey constant for context-based session ID propagation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // the same session, preserving backend state (e.g., Playwright browser contexts, | ||
| // database transactions, conversation history). | ||
| type pooledBackendClient struct { | ||
| *httpBackendClient // Embed to inherit ListCapabilities |
There was a problem hiding this comment.
I think pooledBackendClient is too coupled to the httpBackendClient. The reason it needs to depend on it directly is because the current implementation opens/closes clients per-request. Now, those public methods on the original httpBackendClient are never called, because we always decorate with this pooling client?
Stepping back, we need to move the responsibility for opening and closing connections to the backends from the client.go to something that has the same lifetime as a session. I imagine we can do something simpler:
- On session creation, create the clients for the lifetime of the session. We aggregate capabilities and such on session startup. It seems like a reasonable place to initialize client connections as well.
- Update the existing httpBackendClient so it doesn't close connections on each request.
- On session end, close all the clients.
What do you think? It will take some digging to flesh this out more (e.g. finding the appropriate construction/destruction spots, plumbing the clients everywhere they need to go).
I recognize this is easier said than done. Clients are constructed in quite a few places. However, I think that will make the code clearer clients are injected into a session and used throughout.
There was a problem hiding this comment.
i like your proposal but i see that's one with higher risk because of what you said, there are too many places where we construct clients. We can go with first option as i designed if we want to solve it now, or create some rfc with the architectural change. I can investigate it and we can discuss together
github-actions
bot
added
size/XL
Add session-scoped MCP client pooling to preserve backend state across multiple tool calls within the same vMCP session. Previously, each tool call created a new backend client and session, breaking stateful backends that rely on session persistence (e.g., Playwright browser contexts, database transactions, conversation state).
Implementation:
Testing:
Fixes #3062