Add SessionManager behind sessionManagementV2 flag (Phase 2)#3906
Add SessionManager behind sessionManagementV2 flag (Phase 2)#3906
Conversation
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.
There was a problem hiding this comment.
Pull request overview
Implements RFC THV-0038 Phase 2 by introducing a feature-flagged vmcpSessionManager that bridges MultiSession/MultiSessionFactory to the MCP SDK SessionIdManager, enabling session-scoped backend client lifecycle while keeping the existing path unchanged when disabled.
Changes:
- Add
vmcpSessionManagerbehindSessionManagementV2to generate/validate/terminate sessions and register per-session tools. - Extend
MultiSessionFactorywithMakeSessionWithID()and addtransportsession.Manager.ReplaceSession()to support two-phase session creation. - Update discovery middleware to bypass Phase 1 routing-table reconstruction for Phase 2
MultiSessionsessions; add unit + integration tests.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/vmcp/server/session_manager.go | New Phase 2 session manager (Generate/CreateSession/Validate/Terminate + tool adaptation). |
| pkg/vmcp/server/server.go | Wires Phase 2 manager behind SessionManagementV2 and registers session tools in OnRegisterSession. |
| pkg/vmcp/session/factory.go | Adds MakeSessionWithID() and refactors shared makeSession() implementation. |
| pkg/transport/session/manager.go | Adds ReplaceSession() upsert for swapping placeholder → real session. |
| pkg/vmcp/discovery/middleware.go | Skips Phase 1 routing-table reconstruction when the stored session is a MultiSession. |
| pkg/vmcp/config/config.go | Adds sessionManagementV2 operational config flag. |
| pkg/vmcp/server/session_manager_test.go | Unit tests for Phase 2 session manager behavior. |
| pkg/transport/session/manager_test.go | Unit tests for ReplaceSession() behavior. |
| pkg/vmcp/server/session_management_v2_integration_test.go | End-to-end tests for v2 initialize + tool routing and “old path unaffected” case. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Large PR justification has been provided. Thank you!
|
✅ Large PR justification has been provided. The size review has been dismissed and this PR can now proceed with normal review. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3906 +/- ##
==========================================
+ Coverage 67.95% 68.04% +0.08%
==========================================
Files 435 436 +1
Lines 43988 44221 +233
==========================================
+ Hits 29893 30088 +195
- Misses 11753 11785 +32
- Partials 2342 2348 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jerm-dro
left a comment
There was a problem hiding this comment.
Thanks for all the testing! No major comments, but some questions to make sure I understand. I also have some suggestions you may want to address in subsequent PRs. Approving so you can continue to make progress.
Implement RFC THV-0038 Phase 2: a vmcpSessionManager that bridges
the MultiSession/MultiSessionFactory domain logic to the MCP SDK's
SessionIdManager interface. The feature is gated behind a
sessionManagementV2 config flag (default false), leaving the
existing code path completely unchanged when disabled.
Key changes:
- pkg/vmcp/server/session_manager.go: new vmcpSessionManager with
two-phase creation — Generate() stores a placeholder, CreateSession()
(called from OnRegisterSession hook) replaces it with a fully-formed
MultiSession via MakeSessionWithID(). Terminate() calls Close() before
deleting to release backend connections. GetAdaptedTools() returns
SDK tools with session-scoped handlers delegating to CallTool().
- pkg/vmcp/session/factory.go: add MakeSessionWithID() to the
MultiSessionFactory interface; refactor MakeSession to share a private
makeSession() implementation.
- pkg/transport/session/manager.go: add ReplaceSession() upsert to
swap a placeholder with a fully-formed MultiSession.
- pkg/vmcp/config/config.go: add SessionManagementV2 bool field.
- pkg/vmcp/server/server.go: wire vmcpSessionManager when flag+factory
are set; add handleSessionRegistrationV2() hook handler.
- pkg/vmcp/discovery/middleware.go: skip Phase 1 routing-table
reconstruction for Phase 2 MultiSession sessions.
- Tests: unit tests for all new methods, ReplaceSession coverage in
transport/session, and an end-to-end integration test covering
initialize, tool call routing, and the old-path-unaffected case.
Closes: #3866
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
pkg/vmcp/conversion/content.go:48
- ConvertMCPContent() defines contentTypeResource but still hard-codes "resource" in several return values. Using the constant consistently would avoid drift if the string ever changes and keeps the conversion symmetric with ToMCPContent().
if res, ok := mcp.AsEmbeddedResource(content); ok {
if textRes, ok := mcp.AsTextResourceContents(res.Resource); ok {
return vmcp.Content{Type: "resource", Text: textRes.Text, URI: textRes.URI, MimeType: textRes.MIMEType}
}
if blobRes, ok := mcp.AsBlobResourceContents(res.Resource); ok {
return vmcp.Content{Type: "resource", Data: blobRes.Blob, URI: blobRes.URI, MimeType: blobRes.MIMEType}
}
slog.Debug("Embedded resource has unknown resource contents type", "type", fmt.Sprintf("%T", res.Resource))
return vmcp.Content{Type: "resource"}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
aponcedeleonch
left a comment
There was a problem hiding this comment.
Great work on this PR! The two-phase session creation pattern is clean, race conditions are thoughtfully handled with the double-check pattern, and the test coverage is thorough. Approving with two minor suggestions that can be addressed in a follow-up.
Implement RFC THV-0038 Phase 2: a vmcpSessionManager that bridges the MultiSession/MultiSessionFactory domain logic to the MCP SDK's SessionIdManager interface. The feature is gated behind a sessionManagementV2 config flag (default false), leaving the existing code path completely unchanged when disabled.
Key changes:
Closes: #3866
Large PR Justification
This is an atomic PR that covers an isolated functionality, and includes complete tests. Cannot be split.