fix: implement Single-Session architecture to resolve MCP stream errors

- Add ConsoleManager to prevent console output interference with StreamableHTTPServerTransport - Implement SingleSessionHTTPServer with persistent session reuse - Create N8NMCPEngine for clean service integration - Add automatic session expiry after 30 minutes of inactivity - Update logger to be HTTP-aware during active requests - Maintain backward compatibility with existing deployments This fixes the "stream is not readable" error by implementing the Hybrid Single-Session architecture as documented in MCP_ERROR_FIX_PLAN.md 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
2025-06-14 15:02:49 +02:00
parent 88dd66bb7a
commit 2cb264fd56
13 changed files with 1894 additions and 51 deletions
--- a/docs/MCP_ARCHITECTURE_ANALYSIS.md
+++ b/docs/MCP_ARCHITECTURE_ANALYSIS.md
@@ -0,0 +1,301 @@
+# MCP Server Architecture Analysis: Stateful vs Stateless
+
+## Executive Summary
+
+After deep analysis of the MCP protocol, StreamableHTTPServerTransport implementation, and our specific use case (single-player repository as an engine for a service), I recommend a **Hybrid Single-Session Architecture** that provides the simplicity of stateless design with the protocol compliance of stateful implementation.
+
+## Context and Requirements
+
+### Project Goals
+1. **Single-player repository** - One user at a time, not concurrent sessions
+2. **Engine for a service** - This repo will be integrated into a larger system
+3. **Simplicity** - Easy to understand, maintain, and deploy
+4. **Separation of concerns** - Multi-user features in separate repository
+
+### Protocol Reality
+- MCP is inherently **stateful by design**
+- StreamableHTTPServerTransport **expects session management**
+- The protocol maintains context across multiple tool invocations
+- Attempting pure stateless breaks protocol expectations
+
+## Architecture Options Analysis
+
+### Option A: Full Stateful Implementation
+
+```typescript
+class StatefulMCPServer {
+  private sessions = new Map<string, SessionData>();
+  
+  // Multiple concurrent sessions
+  // Session cleanup
+  // Memory management
+  // Complexity: HIGH
+}
+```
+
+**Pros:**
+- Full protocol compliance
+- Supports multiple concurrent users
+- Future-proof for scaling
+
+**Cons:**
+- **Over-engineered for single-player use case**
+- Complex session management unnecessary
+- Memory overhead for session storage
+- Cleanup logic adds complexity
+- Conflicts with "engine" design principle
+
+**Verdict:** ❌ Too complex for our needs
+
+### Option B: Pure Stateless Implementation
+
+```typescript
+class StatelessMCPServer {
+  // New instance per request
+  // No session tracking
+  // Complexity: LOW
+}
+```
+
+**Pros:**
+- Very simple implementation
+- No memory overhead
+- Easy to understand
+
+**Cons:**
+- **Breaks MCP protocol expectations**
+- Request ID collisions
+- No context between calls
+- StreamableHTTPServerTransport fights this approach
+- The "stream is not readable" error persists
+
+**Verdict:** ❌ Incompatible with protocol
+
+### Option C: Hybrid Single-Session Architecture (Recommended)
+
+```typescript
+class SingleSessionMCPServer {
+  private currentSession: {
+    transport: StreamableHTTPServerTransport;
+    server: N8NDocumentationMCPServer;
+    lastAccess: Date;
+  } | null = null;
+  
+  async handleRequest(req: Request, res: Response) {
+    // Always use/reuse the single session
+    if (!this.currentSession || this.isExpired()) {
+      await this.createNewSession();
+    }
+    
+    this.currentSession.lastAccess = new Date();
+    await this.currentSession.transport.handleRequest(req, res);
+  }
+  
+  private isExpired(): boolean {
+    // Simple 30-minute timeout
+    const thirtyMinutes = 30 * 60 * 1000;
+    return Date.now() - this.currentSession.lastAccess.getTime() > thirtyMinutes;
+  }
+}
+```
+
+**Pros:**
+- **Protocol compliant** - Satisfies StreamableHTTPServerTransport expectations
+- **Simple** - Only one session to manage
+- **Memory efficient** - Single session overhead
+- **Perfect for single-player** - Matches use case exactly
+- **Clean integration** - Easy to wrap as an engine
+
+**Cons:**
+- Not suitable for concurrent users (but that's handled elsewhere)
+
+**Verdict:** ✅ Perfect match for requirements
+
+## Detailed Implementation Strategy
+
+### 1. Console Output Management
+```typescript
+// Silence console only during transport operations
+class ManagedConsole {
+  silence() {
+    this.originalLog = console.log;
+    console.log = () => {};
+  }
+  
+  restore() {
+    console.log = this.originalLog;
+  }
+  
+  wrapOperation<T>(fn: () => T): T {
+    this.silence();
+    try {
+      return fn();
+    } finally {
+      this.restore();
+    }
+  }
+}
+```
+
+### 2. Single Session Manager
+```typescript
+export class SingleSessionHTTPServer {
+  private session: SessionData | null = null;
+  private console = new ManagedConsole();
+  
+  async handleRequest(req: Request, res: Response): Promise<void> {
+    return this.console.wrapOperation(async () => {
+      // Ensure we have a valid session
+      if (!this.session || this.shouldReset()) {
+        await this.resetSession();
+      }
+      
+      // Update last access
+      this.session.lastAccess = new Date();
+      
+      // Handle the request with existing transport
+      await this.session.transport.handleRequest(req, res);
+    });
+  }
+  
+  private async resetSession(): Promise<void> {
+    // Clean up old session
+    if (this.session) {
+      await this.session.transport.close();
+      await this.session.server.close();
+    }
+    
+    // Create new session
+    const server = new N8NDocumentationMCPServer();
+    const transport = new StreamableHTTPServerTransport({
+      sessionIdGenerator: () => 'single-session', // Always same ID
+    });
+    
+    await server.connect(transport);
+    
+    this.session = {
+      server,
+      transport,
+      lastAccess: new Date(),
+      sessionId: 'single-session'
+    };
+  }
+  
+  private shouldReset(): boolean {
+    // Reset after 30 minutes of inactivity
+    const inactivityLimit = 30 * 60 * 1000;
+    return Date.now() - this.session.lastAccess.getTime() > inactivityLimit;
+  }
+}
+```
+
+### 3. Integration as Engine
+
+```typescript
+// Easy to use in larger service
+export class N8NMCPEngine {
+  private server: SingleSessionHTTPServer;
+  
+  constructor() {
+    this.server = new SingleSessionHTTPServer();
+  }
+  
+  // Simple interface for service integration
+  async processRequest(req: Request, res: Response): Promise<void> {
+    return this.server.handleRequest(req, res);
+  }
+  
+  // Clean shutdown for service lifecycle
+  async shutdown(): Promise<void> {
+    return this.server.shutdown();
+  }
+}
+```
+
+## Why This Architecture Wins
+
+### 1. **Protocol Compliance**
+- StreamableHTTPServerTransport gets the session it expects
+- No fighting against the SDK design
+- Fixes "stream is not readable" error
+
+### 2. **Simplicity**
+- One session = one user
+- No complex session management
+- Clear lifecycle (create, use, expire, recreate)
+
+### 3. **Engine-Ready**
+- Clean interface for integration
+- No leaked complexity
+- Service wrapper handles multi-user concerns
+
+### 4. **Resource Efficient**
+- Single session in memory
+- Automatic cleanup after inactivity
+- No accumulating sessions
+
+### 5. **Maintainable**
+- Easy to understand code
+- Clear separation of concerns
+- No hidden complexity
+
+## Migration Path
+
+### Phase 1: Fix Console Output (1 day)
+- Implement ManagedConsole wrapper
+- Wrap all transport operations
+
+### Phase 2: Implement Single Session (2 days)
+- Create SingleSessionHTTPServer
+- Handle session lifecycle
+- Test with Claude Desktop
+
+### Phase 3: Polish and Document (1 day)
+- Add error handling
+- Performance metrics
+- Usage documentation
+
+## Testing Strategy
+
+```typescript
+describe('Single Session MCP Server', () => {
+  it('should reuse session for multiple requests', async () => {
+    const server = new SingleSessionHTTPServer();
+    const req1 = createMockRequest();
+    const req2 = createMockRequest();
+    
+    await server.handleRequest(req1, mockRes);
+    await server.handleRequest(req2, mockRes);
+    
+    // Should use same session
+    expect(server.getSessionCount()).toBe(1);
+  });
+  
+  it('should reset expired sessions', async () => {
+    const server = new SingleSessionHTTPServer();
+    
+    // First request
+    await server.handleRequest(req1, res1);
+    
+    // Simulate 31 minutes passing
+    jest.advanceTimersByTime(31 * 60 * 1000);
+    
+    // Second request should create new session
+    await server.handleRequest(req2, res2);
+    
+    expect(server.wasSessionReset()).toBe(true);
+  });
+});
+```
+
+## Conclusion
+
+The **Hybrid Single-Session Architecture** is the optimal solution for n8n-MCP because it:
+
+1. **Respects the protocol** - Works with MCP's stateful design
+2. **Matches the use case** - Perfect for single-player repository
+3. **Simplifies implementation** - No unnecessary complexity
+4. **Integrates cleanly** - Ready to be an engine for larger service
+5. **Fixes the core issue** - Eliminates "stream is not readable" error
+
+This architecture provides the best balance of simplicity, correctness, and maintainability for our specific requirements.