fix: call onerror callback for all error responses in StreamableHTTPServerTransport#1687
Conversation
…erverTransport Previously, many error paths in handlePostRequest, handleGetRequest, and handleDeleteRequest would return JSON error responses without calling the onerror callback, making these errors invisible to users who set up error logging via transport.onerror. This fix moves the onerror call into createJsonErrorResponse itself, ensuring every error response consistently triggers the callback. Redundant manual onerror calls before createJsonErrorResponse are removed to avoid double-firing. Fixes modelcontextprotocol#1395
🦋 Changeset detectedLatest commit: 3a1a8b7 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
- Add non-null assertions for errors[0] access after length check (TS2532) - Remove unused catch binding in streamableHttp.ts - Format with prettier
travisbreaks
left a comment
travisbreaks
left a comment
There was a problem hiding this comment.
Good change overall. Centralizing onerror in createJsonErrorResponse() is the right structural call: it guarantees coverage for every path that uses that helper, including validateSession, validateProtocolVersion, and the various 4xx checks that were previously silent. The test coverage is solid.
A few things worth flagging:
1. handleUnsupportedRequest() bypasses the centralized path
handleUnsupportedRequest() (the 405 handler for PUT/PATCH/etc.) builds its own Response.json(...) directly instead of calling createJsonErrorResponse(). After this PR, that error path still won't fire onerror. It may be intentional (unsupported HTTP methods aren't really transport errors), but if the goal is "every error response triggers the callback," this is a gap. Worth either converting it to use createJsonErrorResponse() or documenting the exception.
2. Lost error fidelity in catch blocks
The two catch blocks that previously called onerror with the actual caught error (error as Error) now get new Error(message) where message is a generic string like "Error replaying events" or "Parse error". The real exception (with its original message, type, and stack trace) is discarded from the callback. For the handlePostRequest outer catch, the original error still appears as data in the HTTP response body, but anyone relying on onerror for logging/observability loses the underlying cause.
Consider preserving the original error when one is available. For example, the replayEvents catch could pass the caught error to onerror and still call createJsonErrorResponse separately (accepting the "double call" in that one path), or createJsonErrorResponse could accept an optional cause parameter:
private createJsonErrorResponse(
status: number,
code: number,
message: string,
options?: { headers?: Record<string, string>; data?: string; cause?: Error }
): Response {
this.onerror?.(options?.cause ?? new Error(message));
// ...
}3. Overlap with #1684
PR #1684 fixes a subset of the same issue (the two inner parse catch blocks in handlePostRequest). That PR takes a different approach: it adds targeted onerror calls at each catch site and preserves the actual caught error object. If this PR lands first, #1684 becomes a no-op for those paths. If #1684 lands first, this PR would need a rebase and could potentially double-fire on those two catch blocks. The maintainers should pick one direction and close the other.
Between the two, this PR has better architectural coverage (all createJsonErrorResponse callers get onerror for free going forward). But #1684 preserves error fidelity. The ideal outcome would be this PR's centralized approach combined with #1684's pattern of forwarding the real error object.
Nice tests. The changeset is included, which #1684 is missing.
…n and error cause preservation - Convert handleUnsupportedRequest() to use createJsonErrorResponse() for consistency - Add optional 'cause' parameter to createJsonErrorResponse() to preserve error fidelity - Pass caught errors as 'cause' in catch blocks to maintain original error information - Add test for onerror callback on unsupported HTTP methods
|
Thanks for the thorough review, @travisbreaks! Great catches — I've pushed a commit addressing your feedback:
All tests pass (43/43). Let me know if anything else needs adjustment! |
|
Thanks @travisbreaks and @kai-agent-free — great progress on this. I closed #1684 to consolidate on this PR and avoid parallel fixes for #1395. I’m happy to actively help push this over the line. If useful, I can immediately contribute one focused follow-up commit that:
If you’re good with that scope, I can open the follow-up against this branch today. |
|
Thanks @Maverick-666! Appreciate closing #1684 to consolidate — makes sense to avoid parallel fixes. Your follow-up commit proposal sounds good. The regression test coverage for the new The |
3 participants
Summary
Fixes #1395
Several error paths in
StreamableHTTPServerTransportreturned JSON error responses viacreateJsonErrorResponse()without calling theonerrorcallback, silently swallowing transport errors.Changes
packages/server/src/server/streamableHttp.ts:this.onerror?.(new Error(message))insidecreateJsonErrorResponse()so every error response automatically triggers the callbackthis.onerror?.()calls that precededcreateJsonErrorResponse()to avoid double-firingThis ensures all error types are observable — including parse errors, invalid headers, session validation failures, and protocol version mismatches — which were previously invisible to users.
packages/server/test/server/streamableHttp.test.ts:onerroris called for: invalid Accept header (POST & GET), invalid Content-Type, invalid JSON, and invalid JSON-RPC messagesTest Results
All 42 tests pass (37 existing + 5 new). TypeScript compiles cleanly.