Skip to content

Conversation

@waleedlatif1
Copy link
Collaborator

Summary

  • remove client-side mcp tools cache and reduce server cache from 5m to 30s
  • remove sessionId and disconnect after every request like original

Type of Change

  • Bug fix

Testing

Tested manually

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

@vercel
Copy link

vercel bot commented Dec 4, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Preview Comments Updated (UTC)
docs Skipped Skipped Dec 4, 2025 4:50pm

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Dec 4, 2025

Greptile Overview

Greptile Summary

This PR reverts to a stateless connection model by removing session ID caching and reducing server-side tool cache from 5 minutes to 30 seconds.

Key Changes:

  • Removed sessionId parameter from McpClient constructor - connections now disconnect after each operation
  • Removed session caching logic (sessionCache Map and related methods) from McpService
  • Reduced CACHE_TIMEOUT constant from 5 minutes to 30 seconds
  • Added intelligent cache-skipping logic - if any server fails during tool discovery, the result is not cached (will retry on next request)
  • Improved logging to show success/failure ratio for multi-server tool discovery

Impact:

  • Simplified architecture by removing session management complexity
  • Shorter cache duration means fresher data but potentially more server requests
  • Better error recovery through cache-skipping on partial failures

Confidence Score: 4/5

  • This PR is safe to merge with minor style improvements recommended
  • The changes successfully simplify the MCP connection architecture by removing session management. The logic is sound and includes good error handling (cache-skipping on failures). Minor issues: (1) hardcoded cache timeout instead of using constant creates inconsistency, (2) unused getSessionId() method remains as dead code. These are style issues that don't affect functionality.
  • Pay attention to apps/sim/lib/mcp/service.ts for cache timeout constant usage

Important Files Changed

File Analysis

Filename Score Overview
apps/sim/lib/mcp/client.ts 4/5 removed sessionId parameter from constructor, updated comments; contains unused getSessionId() method
apps/sim/lib/mcp/service.ts 4/5 removed session caching logic, added partial failure handling for tool discovery; hardcoded cache timeout instead of using constant
apps/sim/lib/mcp/utils.ts 5/5 reduced CACHE_TIMEOUT from 5 minutes to 30 seconds

Sequence Diagram

sequenceDiagram
    participant Client as Frontend
    participant API as API Route
    participant Service as McpService
    participant McpClient as McpClient
    participant Server as MCP Server

    Note over Service: Server-side cache: 30s

    Client->>API: Request tools/execute
    API->>Service: discoverTools() or executeTool()
    
    alt Cache hit (< 30s)
        Service-->>API: Return cached tools
        API-->>Client: Response
    else Cache miss or expired
        Service->>McpClient: new McpClient(config)
        Note over McpClient: No sessionId parameter
        McpClient->>Server: connect()
        Server-->>McpClient: Session established (SDK manages sessionId)
        McpClient->>Server: listTools() or callTool()
        Server-->>McpClient: Response
        McpClient->>Server: disconnect()
        Note over McpClient: Connection closed after operation
        McpClient-->>Service: Result
        
        alt All servers succeeded
            Service->>Service: Cache result (30s)
        else Any server failed
            Note over Service: Skip caching to retry next time
        end
        
        Service-->>API: Result
        API-->>Client: Response
    end
Loading

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Additional Comments (1)

  1. apps/sim/lib/mcp/client.ts, line 267-269 (link)

    style: dead code - getSessionId() is no longer used after removing session management

3 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

@waleedlatif1 waleedlatif1 merged commit 9df87d9 into staging Dec 4, 2025
5 checks passed
@waleedlatif1 waleedlatif1 deleted the fix/mcp branch December 4, 2025 16:51
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.

2 participants