changes from review

Author: yrobla

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

@@ -616,7 +616,7 @@ The default factory implementation follows this pattern:
    - **Client initialization includes MCP handshake**: Each client sends `InitializeRequest` to its backend, and the backend responds with capabilities and its own `Mcp-Session-Id`. The client stores the session ID for protocol compliance (includes it in subsequent request headers).
    - **Capture backend session IDs**: Factory also captures each backend's session ID (via `client.SessionID()`) for observability, storing them in a map to pass to the session
    - **Performance requirement**: Use parallel initialization (e.g., `errgroup` with bounded concurrency) to avoid sequential latency accumulation. Connection initialization (TCP handshake + TLS negotiation + MCP protocol handshake) can take tens to hundreds of milliseconds per backend depending on network latency and backend responsiveness. With 20 backends, sequential initialization could easily exceed acceptable session creation latency.
-   - **Bounded concurrency**: Limit parallel goroutines (e.g., 10 concurrent initializations) to avoid resource exhaustion. This limit is **per-session-creation** (not global), implemented as a semaphore inside the factory. It should be a configurable vMCP server-level parameter (e.g., `max_backend_init_concurrency`, default: 10). Operators with many backends on a fast private network can raise it; resource-constrained deployments or backends with expensive initialization should lower it. A global limit across concurrent session creations is not necessary — the per-session semaphore already bounds the worst case per event.
+   - **Bounded concurrency**: Limit parallel goroutines (e.g., 10 concurrent initializations) per session to avoid resource exhaustion. This limit is **per-session-creation**, implemented as a semaphore inside the factory via a configurable vMCP server-level parameter (`max_backend_init_concurrency`, default: 10). Operators should note that the aggregate system load is protected by the global `TOOLHIVE_MAX_SESSIONS` limit (see [Resource Exhaustion & DoS Protection](#concurrency--resource-safety)). For additional safety during traffic spikes, the factory may optionally implement a global initialization semaphore (e.g., `max_global_backend_init_concurrency`, default: 100) to cap the total number of simultaneous connection attempts across all active session creations (preventing, for example, 100 concurrent session requests from triggering 1,000 backend initializations).
    - **Per-backend timeout**: Apply context timeout (e.g., 5s per backend) so one slow backend doesn't block session creation
    - **Partial initialization**: If some backends fail, log warnings and continue with successfully initialized backends (failed backends not added to clients map)
    - Clients are connection-ready and stateful (each maintains its backend session for protocol use)
@@ -1055,9 +1055,15 @@ When vMCP detects backend session expiration (404 or "session expired" error), a
 func (s *Session) CallTool(ctx context.Context, name string, arguments map[string]any) (*ToolResult, error) {
     backend := s.routingTable.Lookup(name)
 
-    // Hold RLock only while reading from shared state; release before network I/O.
+    // Check if session is closed and increment in-flight counter
     s.mu.RLock()
+    if s.closed {
+        s.mu.RUnlock()
+        return nil, fmt.Errorf("session closed")
+    }
     client := s.clients[backend.ID]
+    s.inFlight.Add(1)
+    defer s.inFlight.Done()
     s.mu.RUnlock()
 
     result, err := client.CallTool(ctx, name, arguments)
@@ -1067,14 +1073,14 @@ func (s *Session) CallTool(ctx context.Context, name string, arguments map[strin
         log.Warn("Backend %s session expired, re-initializing", backend.ID)
 
         // Re-initialize the backend session (acquires write lock internally).
-        if reinitErr := s.reinitializeBackend(ctx, backend.ID); reinitErr != nil {
+        // Pass the failed client to avoid redundant re-initialization if multiple
+        // goroutines detect the failure simultaneously.
+        if reinitErr := s.reinitializeBackend(ctx, backend.ID, client); reinitErr != nil {
             return nil, fmt.Errorf("failed to reinitialize backend %s: %w",
                 backend.ID, reinitErr)
         }
 
         // Retry the operation once with the re-initialized session.
-        // No further retries: if the new session also fails, surface the error
-        // immediately to avoid infinite loops or resource exhaustion.
         // Re-read under RLock: reinitializeBackend replaced the client under write lock.
         s.mu.RLock()
         client = s.clients[backend.ID]
@@ -1091,15 +1097,21 @@ func (s *Session) CallTool(ctx context.Context, name string, arguments map[strin
     return result, err
 }
 
-func (s *Session) reinitializeBackend(ctx context.Context, backendID string) error {
+func (s *Session) reinitializeBackend(ctx context.Context, backendID string, failedClient *Client) error {
     s.mu.Lock()
     defer s.mu.Unlock()
 
+    // Check if another goroutine already re-initialized this backend while we were waiting for the lock
+    if currentClient := s.clients[backendID]; currentClient != failedClient {
+        log.Debug("Backend %s already re-initialized by another goroutine", backendID)
+        return nil
+    }
+
     backend := s.getBackendConfig(backendID)
 
     // Close old client (with expired backend session)
-    if oldClient := s.clients[backendID]; oldClient != nil {
-        oldClient.Close()
+    if failedClient != nil {
+        failedClient.Close()
     }
 
     // Create NEW client (triggers new Initialize handshake with backend)
@@ -1148,7 +1160,7 @@ func (s *Session) reinitializeBackend(ctx context.Context, backendID string) err
    - **Approach**: Best effort - attempt keepalive, gracefully handle backends that don't support it
    - **Configuration**: Enable per backend, configurable interval (default: 5 min)
 
-   The preferred keepalive method is the MCP spec-defined `ping` protocol request, which is side-effect-free and supported by all compliant servers; explicit tool calls should only be used as a fallback. Keepalive failures must not affect healthy sessions — after N consecutive failures the feature should be disabled for that backend, with a periodic probe to re-enable on recovery. Keepalive should default to disabled for stateless backends or where TTL alignment already covers the session lifetime. The keepalive goroutine must hold the backend lock to avoid races with session re-initialization. Operators should be able to observe keepalive health via per-backend metrics covering attempt counts, failure reasons, and auto-disable events.
+   The preferred keepalive method is the MCP spec-defined `ping` protocol request, which is side-effect-free and supported by all compliant servers; explicit tool calls should only be used as a fallback. Keepalive failures must not affect healthy sessions — after N consecutive failures the feature should be disabled for that backend, with a periodic probe to re-enable on recovery. Keepalive should default to disabled for stateless backends or where TTL alignment already covers the session lifetime. The keepalive goroutine must use the same in-flight counter (`sync.WaitGroup`) approach as other operations to avoid races with session re-initialization while ensuring no locks are held during network I/O. Operators should be able to observe keepalive health via per-backend metrics covering attempt counts, failure reasons, and auto-disable events.
 
 2. **Session TTL alignment**:
    - Configure backend session TTLs longer than vMCP session TTL