Update http-server.ts - #22
Conversation
📝 WalkthroughWalkthroughAdds a Streamable HTTP MCP transport on a new ChangesStreamable HTTP MCP Transport and Routing
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/http-server.ts`:
- Around line 358-388: Both the POST and GET route handlers for '/mcp' are
missing explicit cleanup of the StreamableHTTPServerTransport resource after the
request is handled. Add await transport.close() after the
transport.handleRequest() call completes in both the POST handler (which calls
handleRequest with req, res, and req.body) and the GET handler (which calls
handleRequest with req and res). Place the close() call after the
handleRequest() await but before exiting the try block in each handler to ensure
proper resource cleanup on every request.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| this.app.post('/mcp', async (req: express.Request, res: express.Response) => { | ||
| console.log('[GHL MCP HTTP] Streamable HTTP POST /mcp'); | ||
| try { | ||
| const transport = new StreamableHTTPServerTransport({ | ||
| sessionIdGenerator: undefined, // stateless mode | ||
| }); | ||
| await this.server.connect(transport); | ||
| await transport.handleRequest(req, res, req.body); | ||
| } catch (error) { | ||
| console.error('[GHL MCP HTTP] Streamable HTTP error:', error); | ||
| if (!res.headersSent) { | ||
| res.status(500).json({ error: 'MCP transport error' }); | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| this.app.get('/mcp', async (req: express.Request, res: express.Response) => { | ||
| console.log('[GHL MCP HTTP] Streamable HTTP GET /mcp'); | ||
| try { | ||
| const transport = new StreamableHTTPServerTransport({ | ||
| sessionIdGenerator: undefined, // stateless mode | ||
| }); | ||
| await this.server.connect(transport); | ||
| await transport.handleRequest(req, res); | ||
| } catch (error) { | ||
| console.error('[GHL MCP HTTP] Streamable HTTP GET error:', error); | ||
| if (!res.headersSent) { | ||
| res.status(500).json({ error: 'MCP transport error' }); | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
MCP SDK StreamableHTTPServerTransport stateless mode server.connect pattern
💡 Result:
In the MCP TypeScript SDK Streamable HTTP server transport, “stateless mode” is enabled by constructing the transport with sessionIdGenerator set to undefined; in that mode no Mcp-Session-Id/session validation is used. [1][2] 1) How server.connect fits the pattern - You still use the normal MCP pattern: create an McpServer, create a StreamableHTTPServerTransport (NodeStreamableHTTPServerTransport or WebStandardStreamableHTTPServerTransport), and attach it with server.connect(transport). The SDK explicitly describes connect(transport) as “Attaches to the given transport, starts it, and starts listening for messages.” [3][4] 2) What “stateless mode” changes for request handling - Stateful mode: the transport generates/uses a session id, includes it in responses (including initialization), and rejects requests with invalid/missing session ids. [5][6] - Stateless mode: no session id is included in any responses and “No session validation is performed.” [7][5] Practical consequence: because connections/messages are managed per-request, Streamable HTTP transport start is a no-op (“connections are managed per-request”). [1] 3) The server.connect + stateless server “pattern” - Create the transport with sessionIdGenerator: undefined (this signals stateless mode) and then call server.connect(transport) once per server instance. [1][4] - Then route incoming HTTP requests to transport.handleRequest(...). In the SDK’s stateless example, the server.connect(transport) call happens inside the HTTP route handler before calling transport.handleRequest(req, res, req.body), and cleanup happens on request close. [8] 4) Minimal TypeScript shape (Node.js) - Stateless transport: set sessionIdGenerator to undefined. [1][4] - Connect the MCP server: await server.connect(transport). [4][3] - Handle each HTTP request with transport.handleRequest(...). [8][1] Example (structurally matching the SDK example): - const transport = new NodeStreamableHTTPServerTransport({ sessionIdGenerator: undefined }); - await server.connect(transport); - await transport.handleRequest(req, res, req.body); 5) “server.connect pattern” with stateless and multiple requests - Because stateless mode doesn’t include/validate a Mcp-Session-Id, you should treat each HTTP request independently (no reusable session-based transport state). The transport/session-id generator behavior is what drives this. [7][5] Caveat (important): - Some SDK versions/history discussed issues around stateless behavior and reusing initialized transport state; the recommended approach in the current examples is to create/connect the right transport context for handling the request, and then close/cleanup after the request. [9][8] Bottom line: For StreamableHTTPServerTransport/NodeStreamableHTTPServerTransport stateless mode, “stateless” = sessionIdGenerator undefined (no Mcp-Session-Id, no session validation), while the overall server.connect attachment pattern stays the same: server.connect(transport) + per-request transport.handleRequest(...). [1][5][4][8]
Citations:
- 1: https://ts.sdk.modelcontextprotocol.io/v2/classes/_modelcontextprotocol_node.streamableHttp.NodeStreamableHTTPServerTransport.html
- 2: https://github.com/modelcontextprotocol/typescript-sdk/blob/621ccea997bf318ee99c7f64ce19609e838615fa/src/server/streamableHttp.ts
- 3: https://ts.sdk.modelcontextprotocol.io/classes/server.Server.html
- 4: https://github.com/modelcontextprotocol/typescript-sdk/blob/HEAD/docs/server.md
- 5: https://github.com/modelcontextprotocol/typescript-sdk/blob/2c0c481cb9dbfd15c8613f765c940a5f5bace94d/packages/server/src/server/streamableHttp.ts
- 6: https://cdn.jsdelivr.net/npm/@modelcontextprotocol/sdk@1.20.2/dist/esm/server/streamableHttp.d.ts
- 7: https://ts.sdk.modelcontextprotocol.io/v2/classes/_modelcontextprotocol_server.server_streamableHttp.WebStandardStreamableHTTPServerTransport.html
- 8: https://github.com/modelcontextprotocol/typescript-sdk/blob/4d6197ac07776ab95a2d63a781514a75740cf746/src/examples/server/simpleStatelessStreamableHttp.ts
- 9: sdk/server/streamableHttp.js cannot support stateless mode modelcontextprotocol/typescript-sdk#340
🏁 Script executed:
# Search for StreamableHTTPServerTransport class definition and any cleanup methods
rg -A 20 "class StreamableHTTPServerTransport" --type ts --type js
# Also search for any cleanup/close/disconnect patterns
rg "transport\.(close|disconnect|cleanup)" --type ts --type jsRepository: mastanley13/GoHighLevel-MCP
Length of output: 53
🏁 Script executed:
# Check if there's a connect() method signature and what it does
rg "connect\(" src/http-server.ts -B 2 -A 5Repository: mastanley13/GoHighLevel-MCP
Length of output: 1161
🏁 Script executed:
# Look for any imports or types related to StreamableHTTPServerTransport
rg "StreamableHTTPServerTransport" --type ts --type js -B 2 -A 2Repository: mastanley13/GoHighLevel-MCP
Length of output: 1058
🏁 Script executed:
# Search for any close/disconnect patterns on transport or server
rg "(transport|this\.server)\.(close|disconnect|end)" --type ts --type js
# Check if there's any cleanup after handleRequest in the codebase
rg -A 10 "handleRequest" src/http-server.tsRepository: mastanley13/GoHighLevel-MCP
Length of output: 936
🏁 Script executed:
# Look at the SSE endpoint pattern for comparison to see how cleanup is handled
rg -B 5 -A 15 "SSEServerTransport" src/http-server.tsRepository: mastanley13/GoHighLevel-MCP
Length of output: 1805
🌐 Web query:
StreamableHTTPServerTransport close cleanup resource management per-request
💡 Result:
In the Model Context Protocol (MCP) SDKs, StreamableHTTPServerTransport manages resources through both transport-level lifecycle methods and granular per-request cleanup mechanisms [1][2][3][4]. Per-request resource management is primarily handled via SSE stream management [3][4]: - Request-specific cleanup: The transport provides methods such as closeSSEStream(requestId) (in TypeScript) or closeSseStream(requestId) (in Kotlin), which close the specific SSE stream associated with a request [1][2][3][4]. This action triggers client reconnection, which is often used to implement polling behavior for long-running operations [1][2][3][4]. - Internal Mapping: The transport maintains internal mappings (e.g., _requestToStreamMapping and _streamMapping) to track and clean up these individual stream resources when requested or when an error occurs [3][4]. Lifecycle and Session management: - Transport-level close: The close method shuts down the transport and all its active connections [1]. This is used for broader resource reclamation, such as during server shutdown [5]. - Session-level cleanup: The transport supports an onsessionclosed callback, which is invoked when a session is closed (e.g., via an HTTP DELETE request) [3][4][6]. This callback is specifically designed for developers to perform custom cleanup of resources associated with a particular session [2][3][4]. - Distinction: It is important to distinguish between closing a transport and closing a session; for instance, if handling HTTP requests from multiple nodes, one might close a specific StreamableHTTPServerTransport instance after a single request finishes, while keeping the broader session active [2][3][4]. In practice, developers should leverage the onsessionclosed callback for session-associated resource management and use the provided SSE stream closing methods for granular, per-request stream control [2][3][4].
Citations:
- 1: https://ts.sdk.modelcontextprotocol.io/v2/classes/_modelcontextprotocol_node.streamableHttp.NodeStreamableHTTPServerTransport.html
- 2: https://kotlin.sdk.modelcontextprotocol.io/kotlin-sdk-server/io.modelcontextprotocol.kotlin.sdk.server/-streamable-http-server-transport/index.html
- 3: https://github.com/modelcontextprotocol/typescript-sdk/blob/ab552c30d5499b5e0aaa1ab47ee2481028910c3d/packages/server/src/server/streamableHttp.ts
- 4: https://github.com/modelcontextprotocol/kotlin-sdk/blob/main/kotlin-sdk-server/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/server/StreamableHttpServerTransport.kt
- 5: https://github.com/modelcontextprotocol/servers/blob/main/src/everything/transports/streamableHttp.ts
- 6: https://github.com/modelcontextprotocol/typescript-sdk/blob/e74a358728991216391995e8daa5d0573614abc5/src/server/streamableHttp.ts
Add explicit cleanup after handling stateless HTTP requests.
The /mcp POST and GET endpoints call server.connect(transport) on each request but do not clean up the transport resources afterward. While the pattern of calling connect() per-request is documented in the MCP SDK examples for stateless mode, the SDK also expects explicit resource management—either by calling transport.close() after handleRequest() completes or by leveraging the onsessionclosed callback for cleanup.
Without cleanup, repeated calls to server.connect() without releasing the previous transport may accumulate internal transport state and connections, leading to resource leaks or undefined behavior in the MCP Server instance.
Add await transport.close() after transport.handleRequest() completes in both handlers, or implement proper session lifecycle management using the SDK's cleanup callbacks.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/http-server.ts` around lines 358 - 388, Both the POST and GET route
handlers for '/mcp' are missing explicit cleanup of the
StreamableHTTPServerTransport resource after the request is handled. Add await
transport.close() after the transport.handleRequest() call completes in both the
POST handler (which calls handleRequest with req, res, and req.body) and the GET
handler (which calls handleRequest with req and res). Place the close() call
after the handleRequest() await but before exiting the try block in each handler
to ensure proper resource cleanup on every request.
1 participant
Summary by CodeRabbit
New Features
Infrastructure