Ad stremamble HTTP transport for MCP server #864
Draft
+314
−18
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
https://modelcontextprotocol.io/specification/2025-03-26/basic/transports#streamable-http Signed-off-by: Erik Jankovič <erik.jankovic@gmail.com>
Contributor
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}
Contributor
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.
Comment on lines
+300
to
+320
| function parseArgv(): { transport?: 'stdio' | 'http'; port?: number; host?: string; help?: boolean } { | ||
| const args = process.argv.slice(2); | ||
| const result: { transport?: 'stdio' | 'http'; port?: number; host?: string; help?: boolean } = {}; | ||
| for (let i = 0; i < args.length; i++) { | ||
| if (args[i] === '--help' || args[i] === '-h') { | ||
| result.help = true; | ||
| } else if (args[i] === '--transport' && args[i + 1]) { | ||
| const v = args[i + 1].toLowerCase(); | ||
| if (v === 'stdio' || v === 'http') result.transport = v; | ||
| i++; | ||
| } else if (args[i] === '--port' && args[i + 1]) { | ||
| const n = Number(args[i + 1]); | ||
| if (!Number.isNaN(n)) result.port = n; | ||
| i++; | ||
| } else if (args[i] === '--host' && args[i + 1]) { | ||
| result.host = args[i + 1]; | ||
| i++; | ||
| } | ||
| } | ||
| return result; | ||
| } |
Contributor
There was a problem hiding this comment.
Argument parsing silently ignores invalid values.
- An unrecognized
--transportvalue (e.g.,--transport grpc) is silently dropped, defaulting tostdiowith no feedback. The user may believe they started an HTTP server when they didn't. --portaccepts floats, negative numbers, and0(e.g.,--port -1or--port 3.14).
Consider validating and emitting an error for invalid values:
Proposed fix
} else if (args[i] === '--transport' && args[i + 1]) {
const v = args[i + 1].toLowerCase();
- if (v === 'stdio' || v === 'http') result.transport = v;
+ if (v === 'stdio' || v === 'http') {
+ result.transport = v;
+ } else {
+ console.error(`Invalid transport: "${args[i + 1]}". Must be "stdio" or "http".`);
+ process.exit(1);
+ }
i++;
} else if (args[i] === '--port' && args[i + 1]) {
const n = Number(args[i + 1]);
- if (!Number.isNaN(n)) result.port = n;
+ if (!Number.isInteger(n) || n < 1 || n > 65535) {
+ console.error(`Invalid port: "${args[i + 1]}". Must be an integer between 1 and 65535.`);
+ process.exit(1);
+ }
+ result.port = n;
i++;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function parseArgv(): { transport?: 'stdio' | 'http'; port?: number; host?: string; help?: boolean } { | |
| const args = process.argv.slice(2); | |
| const result: { transport?: 'stdio' | 'http'; port?: number; host?: string; help?: boolean } = {}; | |
| for (let i = 0; i < args.length; i++) { | |
| if (args[i] === '--help' || args[i] === '-h') { | |
| result.help = true; | |
| } else if (args[i] === '--transport' && args[i + 1]) { | |
| const v = args[i + 1].toLowerCase(); | |
| if (v === 'stdio' || v === 'http') result.transport = v; | |
| i++; | |
| } else if (args[i] === '--port' && args[i + 1]) { | |
| const n = Number(args[i + 1]); | |
| if (!Number.isNaN(n)) result.port = n; | |
| i++; | |
| } else if (args[i] === '--host' && args[i + 1]) { | |
| result.host = args[i + 1]; | |
| i++; | |
| } | |
| } | |
| return result; | |
| } | |
| function parseArgv(): { transport?: 'stdio' | 'http'; port?: number; host?: string; help?: boolean } { | |
| const args = process.argv.slice(2); | |
| const result: { transport?: 'stdio' | 'http'; port?: number; host?: string; help?: boolean } = {}; | |
| for (let i = 0; i < args.length; i++) { | |
| if (args[i] === '--help' || args[i] === '-h') { | |
| result.help = true; | |
| } else if (args[i] === '--transport' && args[i + 1]) { | |
| const v = args[i + 1].toLowerCase(); | |
| if (v === 'stdio' || v === 'http') { | |
| result.transport = v; | |
| } else { | |
| console.error(`Invalid transport: "${args[i + 1]}". Must be "stdio" or "http".`); | |
| process.exit(1); | |
| } | |
| i++; | |
| } else if (args[i] === '--port' && args[i + 1]) { | |
| const n = Number(args[i + 1]); | |
| if (!Number.isInteger(n) || n < 1 || n > 65535) { | |
| console.error(`Invalid port: "${args[i + 1]}". Must be an integer between 1 and 65535.`); | |
| process.exit(1); | |
| } | |
| result.port = n; | |
| i++; | |
| } else if (args[i] === '--host' && args[i + 1]) { | |
| result.host = args[i + 1]; | |
| i++; | |
| } | |
| } | |
| return result; | |
| } |
🤖 Prompt for AI Agents
In `@packages/mcp/src/index.ts` around lines 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.
| }); | ||
| }); | ||
|
|
||
| const transports: Record<string, { transport: StreamableHTTPServerTransport; server: McpServer }> = {}; |
Contributor
There was a problem hiding this comment.
Unbounded session store — no TTL or cap on concurrent sessions.
The transports record grows without limit. A misbehaving or malicious client can open sessions indefinitely, eventually exhausting memory. This is especially relevant for the stated goal of remote/containerized deployments where the server is network-exposed.
Consider adding:
- A maximum session count, rejecting new initializations once the limit is reached.
- A session idle timeout that auto-closes stale sessions.
🤖 Prompt for AI Agents
In `@packages/mcp/src/index.ts` at 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.
Comment on lines
+552
to
+554
| app.listen(port, host, () => { | ||
| console.log(`MCP Streamable HTTP server listening on http://${host}:${port}`); | ||
| }); |
Contributor
There was a problem hiding this comment.
HTTP server handle is discarded — cannot be closed on shutdown.
app.listen() returns a http.Server that is never stored. The SIGINT/SIGTERM handler closes transports but doesn't shut down the HTTP listener itself, so in-flight connections and keep-alive sockets are left dangling until process.exit(0) forcefully tears them down.
Proposed fix
- app.listen(port, host, () => {
+ const httpServer = app.listen(port, host, () => {
console.log(`MCP Streamable HTTP server listening on http://${host}:${port}`);
});Then in the shutdown handler:
+ httpServer.close();
console.log('Server shutdown complete');
process.exit(0);🤖 Prompt for AI Agents
In `@packages/mcp/src/index.ts` around lines 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.
Comment on lines
+556
to
+571
| process.on('SIGINT', async () => { | ||
| console.log('Shutting down server...'); | ||
| for (const sessionId in transports) { | ||
| try { | ||
| console.log(`Closing transport for session ${sessionId}`); | ||
| const { transport, server } = transports[sessionId]!; | ||
| await transport.close(); | ||
| server.close(); | ||
| delete transports[sessionId]; | ||
| } catch (error) { | ||
| console.error(`Error closing transport for session ${sessionId}:`, error); | ||
| } | ||
| } | ||
| console.log('Server shutdown complete'); | ||
| process.exit(0); | ||
| }); |
Contributor
There was a problem hiding this comment.
Missing SIGTERM handler — Docker sends SIGTERM, not SIGINT.
docker stop (and most orchestrators like Kubernetes) send SIGTERM first; SIGINT is typically only sent by interactive terminals. Since the stated purpose of this PR is to support containerized deployments, the graceful shutdown logic will never fire in the primary target environment.
Proposed fix
- process.on('SIGINT', async () => {
+ const shutdown = async () => {
console.log('Shutting down server...');
for (const sessionId in transports) {
try {
console.log(`Closing transport for session ${sessionId}`);
const { transport, server } = transports[sessionId]!;
await transport.close();
server.close();
delete transports[sessionId];
} catch (error) {
console.error(`Error closing transport for session ${sessionId}:`, error);
}
}
console.log('Server shutdown complete');
process.exit(0);
- });
+ };
+
+ process.on('SIGINT', shutdown);
+ process.on('SIGTERM', shutdown);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| process.on('SIGINT', async () => { | |
| console.log('Shutting down server...'); | |
| for (const sessionId in transports) { | |
| try { | |
| console.log(`Closing transport for session ${sessionId}`); | |
| const { transport, server } = transports[sessionId]!; | |
| await transport.close(); | |
| server.close(); | |
| delete transports[sessionId]; | |
| } catch (error) { | |
| console.error(`Error closing transport for session ${sessionId}:`, error); | |
| } | |
| } | |
| console.log('Server shutdown complete'); | |
| process.exit(0); | |
| }); | |
| const shutdown = async () => { | |
| console.log('Shutting down server...'); | |
| for (const sessionId in transports) { | |
| try { | |
| console.log(`Closing transport for session ${sessionId}`); | |
| const { transport, server } = transports[sessionId]!; | |
| await transport.close(); | |
| server.close(); | |
| delete transports[sessionId]; | |
| } catch (error) { | |
| console.error(`Error closing transport for session ${sessionId}:`, error); | |
| } | |
| } | |
| console.log('Server shutdown complete'); | |
| process.exit(0); | |
| }; | |
| process.on('SIGINT', shutdown); | |
| process.on('SIGTERM', shutdown); |
🤖 Prompt for AI Agents
In `@packages/mcp/src/index.ts` around lines 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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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