Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes multiple issues with the MCP client's SSE-based server communication and session management. The main goal is to enable proper communication with SSE-based servers like Atlassian MCP.
Changes:
- Implemented a stable message handler with pending requests map to correlate responses with requests, fixing the issue where multiple in-flight requests would lose responses
- Added session ID capture from response headers and forwarding in subsequent requests per MCP spec
- Added abort signal handling to SSE stream processing for proper timeout behavior
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/transport/httpclient.ts | Refactored session header handling, added session ID capture from responses, and integrated abort signal handling into SSE response processing |
| src/client/index.ts | Implemented pending requests map with stable message handler, added notifications/initialized after initialization, and split sendRequest/sendNotification methods |
| src/client/client.test.ts | Updated MockTransport to handle notifications/initialized and added setHeaders method |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Check if the abort signal has been triggered | ||
| if (signal?.aborted) { | ||
| await eventStream.cancel(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
When the abort signal is triggered during SSE stream reading (lines 228-231), the function returns without throwing an error. This causes the send() method to complete successfully even though no response was received for the request. This will leave the corresponding promise in MCPClient.sendRequest hanging indefinitely because it never receives a response. Instead of returning, this should throw an error to propagate the timeout condition back to the caller.
| // Handle abort errors gracefully - these are expected when timeout fires | ||
| if (streamError instanceof Error && streamError.name === "AbortError") { | ||
| this.logger.debug("SSE stream aborted"); | ||
| return; |
There was a problem hiding this comment.
When catching an AbortError (lines 242-245), the function returns without re-throwing the error. This has the same problem as the explicit abort check above: the send() method completes successfully even though the stream was aborted and no response was received. The AbortError should be re-thrown or converted to a meaningful timeout error to ensure the caller is notified of the failure.
| return; | |
| // Surface the abort to the caller so the failure is observable | |
| throw streamError; |
| const initializedNotification = newJSONRPCRequest({ | ||
| id: this.requestId, | ||
| method: "notifications/initialized" | ||
| }); |
There was a problem hiding this comment.
The notifications/initialized message is being created with an id field using newJSONRPCRequest, but according to the JSON-RPC 2.0 specification, notifications should NOT have an id field. The JSONRPCNotificationSchema defined in the codebase does not include an id field. This will cause the notification to be incorrectly formatted and may be rejected by compliant servers. Instead, create a proper notification object with jsonrpc and method fields only (and optional params), without an id field.
| const initializedNotification = newJSONRPCRequest({ | |
| id: this.requestId, | |
| method: "notifications/initialized" | |
| }); | |
| const initializedNotification = { | |
| jsonrpc: "2.0", | |
| method: "notifications/initialized", | |
| }; |
| } | ||
|
|
||
| // Capture session ID from response headers if present | ||
| // (https://modelcontextprotocol.io/specification/2025-06-18/basic/transports#session-management) |
There was a problem hiding this comment.
The specification URL references a future date (2025-06-18) which is inconsistent with the latest protocol version (2025-11-25) used in the initialize method. The comment should reference the 2025-11-25 specification to match the protocol version being used.
| // (https://modelcontextprotocol.io/specification/2025-06-18/basic/transports#session-management) | |
| // (https://modelcontextprotocol.io/specification/2025-11-25/basic/transports#session-management) |
1 participant
Fix multiple issues preventing the MCP client from working correctly with
SSE-based servers like Atlassian MCP.
The previous implementation replaced the message handler on every request,
causing responses to be lost when multiple requests were in flight. This
was fixed by using a stable message handler with a pending requests map
to correlate responses with their originating requests.
Additionally:
is required by servers for subsequent requests per MCP spec