fixes from review

Author: yrobla

…-0034-session-scoped-client-lifecycle.md …-0038-session-scoped-client-lifecycle.mdrfcs/THV-0034-session-scoped-client-lifecycle.md renamed to rfcs/THV-0038-session-scoped-client-lifecycle.md rfcs/THV-0034-session-scoped-client-lifecycle.md renamed to rfcs/THV-0038-session-scoped-client-lifecycle.md

@@ -1,4 +1,4 @@
-# RFC: Session-Scoped MCP Client Lifecycle Management
+# THV-0038: Session-Scoped MCP Client Lifecycle Management
 
 - **Status**: Draft
 - **Author(s)**: @yrobla, @jerm-dro
@@ -9,7 +9,7 @@
 
 ## Summary
 
-Move MCP backend client lifecycle from per-request creation/destruction to session-scoped management. Clients will be created once during session initialization, reused throughout the session lifetime, and closed during session cleanup. This simplifies the client pooling architecture and ensures consistent backend state preservation.
+Move MCP backend client lifecycle from per-request creation/destruction to session-scoped management. Clients will be created once during session initialization, reused throughout the session lifetime, and closed during session cleanup. This simplifies the architecture and ensures consistent backend state preservation.
 
 ## Problem Statement
 
@@ -130,14 +130,17 @@ Sessions already exist and have defined lifetimes (TTL-based expiration). Aligni
 
 **Extend AfterInitialize Hook**:
 
-The `AfterInitialize` hook in the discovery middleware already performs capability discovery for each backend. Extend this to also create and initialize MCP clients:
+The `AfterInitialize` hook in the discovery middleware already performs capability discovery for each backend. Restructure this to create clients once and reuse them:
 
-1. After capability discovery completes, iterate through all backends in the group
-2. For each backend, create a client using the backend target configuration
+1. Iterate through all backends in the group
+2. For each backend, create a client using the backend target configuration via `createAndInitializeClient()`
 3. Initialize the client immediately (MCP handshake)
-4. Store successful clients in a map keyed by workload ID
-5. For failed initializations: log warning and continue (partial initialization is acceptable)
-6. Store the complete client map in the session via `SetBackendClients()`
+4. Use this same client to perform capability discovery (call `ListCapabilities`)
+5. Store the initialized client in a map keyed by workload ID
+6. For failed initializations: log warning and continue (partial initialization is acceptable)
+7. Store the complete client map in the session via `SetBackendClients()`
+
+**Key Improvement**: By reusing the discovery client as the session-scoped client, we avoid redundant connection setup and better meet the stated goal of reducing overhead.
 
 **Error Handling**:
 - Individual client initialization failures don't fail session creation
@@ -164,18 +167,25 @@ The `AfterInitialize` hook in the discovery middleware already performs capabili
   - Called only during session setup, not per-request
 
 **Refactor Request Methods**:
-- `CallTool`, `ReadResource`, `GetPrompt`: Replace client creation pattern with:
+- `CallTool`, `ReadResource`, `GetPrompt`, `ListCapabilities`: Replace client creation pattern with:
   - Call `getSessionClient()` to retrieve pre-initialized client
   - Remove `defer c.Close()` (client managed by session)
   - Remove `initializeClient()` call (already initialized)
   - Use client directly for the operation
-- `ListCapabilities`: Keep existing per-request pattern (only called during session init)
+- **Note**: During session initialization in the discovery middleware, `createAndInitializeClient()` is called once per backend, and that same client is used for capability discovery and registered as the session-scoped client, avoiding redundant connection setup
 
 **Location**: `pkg/vmcp/client/client.go`
 
 #### 4. Session Cleanup Integration
 
-The transport layer's `LocalStorage` implementation already calls `Close()` on sessions before deletion (implemented in a previous PR). We leverage this existing mechanism to close backend clients.
+**Current State**: The `Session` interface does not currently have a `Close()` method. This RFC will add it.
+
+**Required Changes**:
+1. Add `Close() error` method to the `Session` interface in `pkg/transport/session/manager.go`
+2. Implement `Close()` in `VMCPSession` to close all backend clients
+3. Update `LocalStorage` to call `session.Close()` before deletion in `Delete()` and `DeleteExpired()` methods
+
+**Reference**: The storage layer was refactored to use a pluggable interface in [PR #1989](https://github.com/stacklok/toolhive/pull/1989), but session cleanup hooks were not added at that time.
 
 **VMCPSession.Close() Implementation**:
 - Iterate through all clients in the `backendClients` map
@@ -185,12 +195,12 @@ The transport layer's `LocalStorage` implementation already calls `Close()` on s
 
 **Cleanup Triggers**:
 
-The existing session cleanup infrastructure ensures clients are closed when:
-- **TTL Expiration**: Session manager's cleanup worker calls `DeleteExpired()`, which calls `Close()` on expired sessions
-- **Explicit Deletion**: Manual session deletion via `Delete()` calls `Close()` before removing the session
-- **Manager Shutdown**: `Stop()` calls `Close()` on all remaining sessions
+By adding `Close()` to the `Session` interface and calling it from `LocalStorage`, clients will be closed when:
+- **TTL Expiration**: Session manager's cleanup worker calls `DeleteExpired()`, which will call `session.Close()` on expired sessions
+- **Explicit Deletion**: Manual session deletion via `Delete()` will call `session.Close()` before removing the session
+- **Manager Shutdown**: `Manager.Stop()` will call `Close()` on all remaining sessions
 
-No changes needed to the transport layer - it already has the hooks in place.
+The session cleanup infrastructure already exists (from [PR #1989](https://github.com/stacklok/toolhive/pull/1989)); we're adding the `Close()` hook to integrate with it.
 
 #### 5. Error Handling
 
@@ -201,14 +211,14 @@ No changes needed to the transport layer - it already has the hooks in place.
 
 **Connection Failures During Tool Calls**:
 - Return error to client (existing behavior)
-- Health monitor marks backend unhealthy
-- Subsequent sessions won't attempt to initialize unhealthy backends
+- Health monitor marks backend unhealthy (existing behavior)
+- Client initialization attempts continue for unhealthy backends (health-based filtering is future work, see Phase 4)
 
 **Client Already Closed**:
 - If session expired and client is closed, return clear error
 - Client should refresh session via `/initialize` endpoint
 
-## Security
+## Security Considerations
 
 ### Threat Model
 
@@ -249,60 +259,64 @@ The only difference is **when** clients are created (session init vs first use),
 
 ## Alternatives Considered
 
-### Alternative 1: Keep Pooling but Decouple from httpBackendClient
+### Alternative 1: Keep Per-Request Pattern with Connection Pooling
 
-**Approach**: Make `pooledBackendClient` accept an interface instead of embedding `*httpBackendClient`.
+**Approach**: Continue creating/closing clients per request but add a connection pool underneath to reuse TCP connections.
 
 **Pros**:
-- Less refactoring required
-- Maintains lazy initialization pattern
+- Minimal changes to existing code
+- Reduces TCP handshake overhead
 
 **Cons**:
-- Still has pooling complexity
-- Doesn't address first-call latency
-- Doesn't eliminate redundant initialization logic
+- Doesn't address MCP protocol initialization overhead (still happens per request)
+- Doesn't solve state preservation problem (each request still gets fresh MCP client)
+- Authentication still resolved and validated per request
+- Adds complexity at wrong layer
 
-**Decision**: Rejected. Decoupling addresses tight coupling but not the fundamental architectural issue.
+**Decision**: Rejected. This addresses symptoms but not the root cause.
 
-### Alternative 2: Lazy Pool Initialization with Eager Client Creation
+### Alternative 2: Lazy Client Creation on First Use
 
-**Approach**: Keep pool but create all clients immediately when pool is first accessed.
+**Approach**: Create clients on first tool call to a backend, then store in session for reuse.
 
 **Pros**:
-- Minimal code changes
-- Backward compatible with existing pool interface
+- Avoids creating clients for unused backends
+- Delays initialization until needed
 
 **Cons**:
-- Pool abstraction still adds unnecessary indirection
-- First tool call per session still incurs initialization cost
-- Doesn't simplify architecture
+- First tool call to each backend still has initialization latency
+- More complex state management (need to track which clients exist)
+- Doesn't leverage existing capability discovery phase
+- Inconsistent performance (first vs subsequent calls)
 
-**Decision**: Rejected. This is a middle ground that retains most of the complexity.
+**Decision**: Rejected. Complexity outweighs benefits. Capability discovery already knows which backends exist.
 
-### Alternative 3: Global Client Pool Across Sessions
+### Alternative 3: Global Client Cache Across Sessions
 
-**Approach**: Share clients across sessions to amortize initialization cost.
+**Approach**: Share clients across all sessions using a global cache, keyed by backend + auth identity.
 
 **Pros**:
 - Maximum connection reuse
-- Lowest per-session overhead
+- Lowest initialization overhead
 
 **Cons**:
 - **State Pollution**: Backend state (Playwright contexts, DB transactions) would leak across sessions
-- **Security Risk**: Potential for cross-session data exposure
-- **Complexity**: Requires complex client reset/sanitization logic
+- **Security Risk**: Potential for cross-session data exposure if keying is incorrect
+- **Complexity**: Requires complex cache eviction, client sanitization, and identity tracking
+- **Violates Session Isolation**: Sessions should be independent
 
-**Decision**: Rejected. Session isolation is a core requirement for vMCP.
+**Decision**: Rejected. Session isolation is a core requirement for vMCP. The security and complexity risks outweigh performance gains.
 
-## Backward Compatibility
+## Compatibility
 
-### Breaking Changes
+### Backward Compatibility
 
-**None for External APIs**: The `/vmcp/v1/*` HTTP API remains unchanged. Clients see identical behavior.
+**External APIs**: No breaking changes. The `/vmcp/v1/*` HTTP API remains unchanged. Clients see identical behavior.
 
-**Internal API Changes**:
+**Internal APIs**:
 - `BackendClient` interface: No signature changes, but behavior changes from "create on demand" to "retrieve from session"
-- Components depending on `pooledBackendClient` or `BackendClientPool` types will need updates
+- `httpBackendClient` constructor: Requires session manager parameter (added field)
+- Internal refactoring only - no downstream impact on other packages
 
 ### Forward Compatibility
 
@@ -314,31 +328,43 @@ Future enhancements are easier with this design:
 
 ## Implementation
 
-### Phase 1: Session Client Storage
+### Phase 1: Session Client Storage and Cleanup Hooks
+
+Add client storage capabilities and cleanup infrastructure:
 
-Add client storage capabilities to VMCPSession:
+**Session Interface Changes**:
+- Add `Close() error` method to `Session` interface in `pkg/transport/session/manager.go`
+- Update `LocalStorage.Delete()` to call `session.Close()` before deletion
+- Update `LocalStorage.DeleteExpired()` to call `session.Close()` on each expired session before deletion
+- Update `Manager.Stop()` to call `Close()` on all remaining sessions
 
+**VMCPSession Implementation**:
 - Add `backendClients map[string]*client.Client` field to `VMCPSession` struct
 - Implement `GetBackendClient(workloadID string)` method for retrieving clients
 - Implement `SetBackendClients(clients map[string]*client.Client)` for storing client map
-- Implement `Close()` method to iterate and close all clients
+- Implement `Close()` method to iterate through all clients, close them, and collect errors
 
 **Files to modify**:
-- `pkg/vmcp/session/vmcp_session.go`
+- `pkg/transport/session/manager.go` (add Close() to interface)
+- `pkg/transport/session/storage_local.go` (call session.Close() before deletion)
+- `pkg/vmcp/session/vmcp_session.go` (implement Close() and client storage)
 
 **Testing**:
 - Unit tests for client storage/retrieval
 - Unit tests for Close() error handling
+- Tests verifying LocalStorage calls session.Close() before deletion
+- Tests for cleanup on TTL expiration and explicit deletion
 
 ### Phase 2: Client Initialization at Session Creation
 
-Extend the discovery middleware to initialize clients during session setup:
+Restructure the discovery middleware to create clients once and reuse them:
 
 - Modify `AfterInitialize` hook in discovery middleware
-- After capability discovery, create clients for each backend
-- Initialize each client immediately (MCP handshake)
-- Store successful clients in session via `SetBackendClients()`
+- For each backend, create and initialize client via `createAndInitializeClient()`
+- Use the same client to perform capability discovery (call `ListCapabilities`)
+- Store the initialized client in session via `SetBackendClients()`
 - Log warnings for failed initializations but continue (partial initialization acceptable)
+- **Key**: Avoids redundant connection setup by reusing discovery client as session client
 
 **Files to modify**:
 - `pkg/vmcp/server/middleware/discovery/discovery.go`
@@ -355,12 +381,10 @@ Modify httpBackendClient to retrieve clients from session instead of creating pe
 - Add `sessionManager` field to `httpBackendClient` struct
 - Pass session manager to `NewHTTPBackendClient()` constructor
 - Create `getSessionClient(ctx, workloadID)` helper method
-- Create `createAndInitializeClient(ctx, target)` method for session init use
-- Refactor `CallTool` to use `getSessionClient()` instead of `clientFactory()`
-- Refactor `ReadResource` to use `getSessionClient()`
-- Refactor `GetPrompt` to use `getSessionClient()`
+- Create `createAndInitializeClient(ctx, target)` method for session init use (called by discovery middleware)
+- Refactor `CallTool`, `ReadResource`, `GetPrompt`, `ListCapabilities` to use `getSessionClient()` instead of `clientFactory()`
 - Remove `defer c.Close()` calls from all methods
-- Remove `initializeClient()` calls from all methods (except `ListCapabilities`)
+- Remove `initializeClient()` calls from all methods
 
 **Files to modify**:
 - `pkg/vmcp/client/client.go`
@@ -391,7 +415,7 @@ Add observability for client lifecycle:
 
 ### Dependencies
 
-- Existing transport session cleanup infrastructure (sessions already call `Close()` on expiration)
+- Existing transport session cleanup infrastructure from [PR #1989](https://github.com/stacklok/toolhive/pull/1989) (provides deletion hooks where we'll add `Close()` calls)
 - Existing discovery middleware (`AfterInitialize` hook)
 - Existing health monitoring system
 
@@ -415,18 +439,57 @@ Add observability for client lifecycle:
 - Client resource cleanup on TTL expiration
 - High-throughput scenarios verify no connection leaks
 
-## Review History
+## Documentation
+
+**Architecture Documentation**:
+- Update `docs/arch/vmcp-architecture.md` with session-scoped client lifecycle diagrams
+- Document client initialization during session setup phase
+- Add sequence diagrams showing client creation timing
+
+**Code Documentation**:
+- Add package-level comments explaining session-client relationship
+- Document `VMCPSession.GetBackendClient()` and `SetBackendClients()` methods
+- Add examples of client retrieval in method documentation
+
+**Operational Guides**:
+- Update troubleshooting guide with client lifecycle debugging tips
+- Document metrics and logs for monitoring client initialization
+- Add runbook for investigating client initialization failures
+
+## Open Questions
+
+All major design questions have been resolved during RFC development:
+
+1. ~~Should clients be created eagerly or lazily?~~ → **Resolved: Eager initialization during session setup alongside capability discovery**
+2. ~~What happens if backend client initialization fails during session creation?~~ → **Resolved: Log warning, mark backend unhealthy, continue with partial initialization**
+3. ~~Should we share clients across sessions for efficiency?~~ → **Resolved: No, session isolation is more important than marginal efficiency gains**
+4. ~~How should client closure be triggered?~~ → **Resolved: Leverage existing session cleanup infrastructure (TTL expiration, explicit deletion, server shutdown)**
+
+## References
+
+- [MCP Specification](https://modelcontextprotocol.io/specification/2025-06-18)
+- [ToolHive Issue #3062](https://github.com/stacklok/toolhive/issues/3062)
+- [mark3labs/mcp-go SDK](https://github.com/mark3labs/mcp-go) - Backend client implementation
+- [vMCP Architecture Documentation](../docs/arch/vmcp-architecture.md)
+- Related RFCs:
+  - Session management infrastructure
+  - Health monitoring system
+  - Backend discovery mechanism
+
+---
+
+## RFC Lifecycle
+
+<!-- This section is maintained by RFC reviewers -->
+
+### Review History
 
-| Date | Reviewer | Status | Comments |
-|------|----------|--------|----------|
-| 2026-02-04 | @yrobla | Author | Initial draft |
-| 2026-02-04 | @jerm-dro | Contributor | Original proposal |
+| Date | Reviewer | Decision | Notes |
+|------|----------|----------|-------|
+| 2026-02-04 | @yrobla, @jerm-dro | Draft | Initial submission |
 
-## Implementation Tracking
+### Implementation Tracking
 
-| Repository | PR | Status | Notes |
-|------------|-------|--------|-------|
-| toolhive | #TBD | Not Started | Phase 1: Session client storage |
-| toolhive | #TBD | Not Started | Phase 2: Client initialization at session creation |
-| toolhive | #TBD | Not Started | Phase 3: Refactor backend client methods |
-| toolhive | #TBD | Not Started | Phase 4: Observability & monitoring |
+| Repository | PR | Status |
+|------------|-----|--------|
+| toolhive | - | Pending |