Skip to content

add fixes from review for THV-0038#42

Merged
jerm-dro merged 3 commits intomainfrom
session-scoped-mcp-client-fix
Feb 23, 2026
Merged

add fixes from review for THV-0038#42
jerm-dro merged 3 commits intomainfrom
session-scoped-mcp-client-fix

Conversation

@yrobla
Copy link
Contributor

@yrobla yrobla commented Feb 18, 2026

No description provided.

@yrobla yrobla force-pushed the session-scoped-mcp-client-fix branch from 961e573 to e4ffff9 Compare February 18, 2026 09:35
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR incorporates review feedback into RFC THV-0038, which proposes a session-scoped client lifecycle architecture for the vMCP (virtual MCP) server. The RFC introduces a new Session interface that owns backend MCP clients throughout the session lifetime, replacing the current per-request client creation pattern.

Changes:

  • Enhanced thread-safety documentation to use WaitGroup pattern instead of context cancellation for coordinating Close() operations
  • Clarified backend initialization concurrency as per-session bounded (not global)
  • Improved error messages for empty-capability sessions to be more actionable
  • Added explicit locking pattern in CallTool to release locks before network I/O
  • Removed redundant log statement in reinitializeBackend
  • Added detailed keepalive implementation guidance (ping preference, failure handling, metrics)
  • Enhanced authentication credential recreation with singleflight, latency documentation, and proactive refresh guidance
  • Improved resource exhaustion mitigation with Retry-After header and security considerations
  • Clarified "Eager session pre-initialization" terminology
  • Added blocking security requirement for token hash binding before production rollout
  • Enhanced testing criteria for high-throughput tests with specific success metrics
  • Clarified session cleanup pattern using optional interface check

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 4 comments.

Comments suppressed due to low confidence (1)

rfcs/THV-0038-session-scoped-client-lifecycle.md:1121

  • This example performs CreateClient(...) (which includes an Initialize handshake / network I/O) while holding s.mu.Lock(). That can block unrelated tool calls and even deadlock if any callbacks touch session state. Consider doing the slow client creation outside the lock, then acquiring the lock briefly to validate state and swap in the new client (or use singleflight/per-backend mutex to serialize only this backend’s reinit).
    // Create NEW client (triggers new Initialize handshake with backend)
    // This sends InitializeRequest to backend MCP server
    newClient, err := s.clientFactory.CreateClient(ctx, backend, s.identity)
    if err != nil {
        return fmt.Errorf("client creation failed: %w", err)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@yrobla yrobla force-pushed the session-scoped-mcp-client-fix branch 2 times, most recently from 301195b to 8308910 Compare February 20, 2026 08:31
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@jerm-dro jerm-dro merged commit 046f32d into main Feb 23, 2026
5 checks passed
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.

3 participants