Ad stremamble HTTP transport for MCP server#864
Ad stremamble HTTP transport for MCP server#864kirecek wants to merge 2 commits intosourcebot-dev:mainfrom
Conversation
https://modelcontextprotocol.io/specification/2025-03-26/basic/transports#streamable-http Signed-off-by: Erik Jankovič <erik.jankovic@gmail.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe changes introduce HTTP transport support to the MCP server alongside the existing stdio transport. Updates include Docker configuration for port exposure, CLI argument parsing for transport selection, per-session HTTP request handling with Express routes, session management with cleanup on process exit, and documentation describing both transport options. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as HTTP Client
participant Server as Express Server
participant SessionStore as Session Store
participant Transport as StreamableHTTPServerTransport
participant MCP as MCP Implementation
Client->>Server: POST /mcp (sessionId, request)
Server->>SessionStore: Get or create session
alt Session exists
SessionStore-->>Server: Return existing session
else New session
SessionStore->>Transport: Create per-session transport
Transport-->>SessionStore: Transport ready
SessionStore-->>Server: Session created
end
Server->>MCP: Process MCP request
MCP-->>Server: MCP response
Server-->>Client: HTTP 200 with response
Client->>Server: GET /health
Server-->>Client: Health status
Client->>Server: DELETE /mcp (sessionId)
Server->>SessionStore: Cleanup session
SessionStore->>Transport: Close transport
Transport-->>SessionStore: Closed
SessionStore-->>Server: Session removed
Server-->>Client: HTTP 200 deleted
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@packages/mcp/src/index.ts`:
- Line 369: The transports map (const transports) is unbounded and can be
exhausted by many open sessions; add a cap and idle TTL: introduce a
configurable maxSessions constant and check its count before creating a new
StreamableHTTPServerTransport/McpServer pair, returning an error when the limit
is reached, and add per-transport lastActivity tracking with a
sessionIdleTimeout that auto-closes the StreamableHTTPServerTransport and
McpServer (and removes the key from transports) when no activity for the TTL;
ensure any explicit session close also clears timers and removes the entry so
the transport map cannot grow indefinitely.
- Around line 556-571: The shutdown logic is only registered for 'SIGINT' so
containers (which receive SIGTERM) won't perform graceful shutdown; extract the
body into a reusable async function (eg. shutdownServer) and register it for
both 'SIGINT' and 'SIGTERM' by calling process.on('SIGINT', shutdownServer) and
process.on('SIGTERM', shutdownServer); inside that function iterate the
transports map (transports), call await transport.close() and server.close(),
delete transports[sessionId], and log/handle errors as currently done to ensure
the same cleanup runs for SIGTERM as for SIGINT.
- Around line 300-320: In parseArgv, validate and reject invalid --transport and
--port values instead of silently dropping them: when parsing --transport in
parseArgv(), check args[i+1] and if it is not 'stdio' or 'http' emit a clear
error (e.g., console.error/process.exit or throw) referencing the bad value
instead of ignoring it; for --port, parse with Number, ensure it is an integer
within the valid TCP range (1–65535) and reject floats, zero or negatives (emit
error on invalid), and keep existing handling for --host and --help; update the
function to return/exit only after successful validation so callers see explicit
error feedback rather than defaulting silently.
- Around line 552-554: The HTTP server returned by app.listen is currently
discarded so it can't be closed on SIGINT/SIGTERM; store the returned
http.Server (e.g., const server = app.listen(...)) and in the existing
shutdown/SIGINT/SIGTERM handler call server.close(callback) to gracefully stop
accepting new connections and wait for existing requests to finish (optionally
add a short forced-exit timeout). Update references in the shutdown logic that
currently close transports to also close the stored server and handle any close
errors in the same logger used elsewhere.
🧹 Nitpick comments (3)
packages/mcp/src/index.ts (2)
371-442: Consider extracting shared session-validation logic into middleware.The session-ID header validation (bad multi-value check → missing-session check → session-not-found check) is duplicated across all three route handlers (POST, GET, DELETE). Extracting this into an Express middleware or a shared helper would reduce repetition and make it easier to keep the error responses consistent.
Example middleware sketch
function validateSession( req: Request, res: Response, transports: Record<string, { transport: StreamableHTTPServerTransport; server: McpServer }>, { requireSession = true }: { requireSession?: boolean } = {}, ): { sessionId: string | undefined; entry?: { transport: StreamableHTTPServerTransport; server: McpServer } } | null { const raw = req.headers['mcp-session-id']; const sessionId = getSessionId(req); if (raw !== undefined && sessionId === undefined) { res.status(400).json({ jsonrpc: '2.0', error: { code: -32000, message: 'Bad Request: Mcp-Session-Id header must be a single value' }, id: null }); return null; } if (requireSession && !sessionId) { res.status(400).json({ jsonrpc: '2.0', error: { code: -32000, message: 'Bad Request: Mcp-Session-Id header is required' }, id: null }); return null; } if (sessionId && !transports[sessionId]) { res.status(404).json({ jsonrpc: '2.0', error: { code: -32001, message: 'Session not found' }, id: null }); return null; } return { sessionId, entry: sessionId ? transports[sessionId] : undefined }; }Also applies to: 444-496, 498-550
348-357: Consider warning when--portor--hostis set but transport isstdio.If a user passes
--port 8080without--transport http, the port argument is silently ignored. A warning would help avoid confusion.packages/mcp/README.md (1)
163-175: Documentation is clear. Consider mentioning the/healthendpoint.The HTTP transport exposes a
GET /healthendpoint (implemented insrc/index.tsLines 363-367) which is useful for container orchestration liveness/readiness probes. Documenting it here would help users setting up Kubernetes or Docker health checks.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
1 participant
This PR adds an optional Streamable HTTP transport to the MCP server so it can run over a single HTTP endpoint instead of stdio.
https://modelcontextprotocol.io/specification/2025-03-26/basic/transports#streamable-http
Use-case for this is mostly to support remote and containerized deployments where currently users needs to wrap stdio server with thirdparty utility.
Changes
Transport selection – New --transport CLI flag: stdio (default) or http. Existing behavior is unchanged when no flag is set.
HTTP server – When --transport http is used:
Usage
I believe it was proposed also here: #448
Let me know what else I can do to get this across the finish line.
Summary by CodeRabbit
New Features
Documentation