fix(core): tolerate optional MCP HTTP event stream failures - #24
Conversation
WalkthroughChangesHTTP SSE session lifecycle
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Registry
participant HTTPTransport
participant MCPServer
Registry->>HTTPTransport: initialize HTTP transport
HTTPTransport->>MCPServer: request inbound SSE
MCPServer-->>HTTPTransport: return SSE response or 404
HTTPTransport->>Registry: report transport error
Registry->>Registry: check error shape and session state
Registry->>MCPServer: remove tools after session expiration
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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
🧹 Nitpick comments (2)
apps/core/src/mcp/registry.ts (2)
565-569: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog tolerated optional inbound SSE errors instead of discarding them silently.
When
isOptionalHttpInboundSseErrorreturnstrue, the handler returns without any logging. The error is real (a rejected inbound GET SSE stream) even though it's non-fatal; discarding it with no trace makes it hard to diagnose recurring inbound-stream failures in production. Add a debug/trace-level log capturing the tolerated error before returning.🪵 Proposed logging addition
onUncaughtError: (error) => { if ( isOptionalHttpInboundSseError(definition, holder.sessionExpired === true, error) ) { + logger.debug(`Ignoring optional inbound SSE failure for MCP server ${definition.id}`, { error }); return; }As per coding guidelines, "Safely convert unknown caught values, avoid silently swallowing errors, and make logged errors informative and traceable."
🤖 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 `@apps/core/src/mcp/registry.ts` around lines 565 - 569, In the handler around isOptionalHttpInboundSseError, add a debug- or trace-level log containing the tolerated error and relevant inbound SSE context before the existing return. Preserve the non-fatal early-return behavior and use the surrounding logger and established safe unknown-error conversion conventions.Source: Coding guidelines
224-232: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDocument the
onSessionExpiredvsonUncaughtErrorordering assumption.
isOptionalHttpInboundSseErrortreats a GET-SSE failure as optional only whenholder.sessionExpiredis stillfalseat the pointonUncaughtErrorruns. The HTTP transport design clears the session before surfacing the 404-based session error, but the explicit ordering guarantee does not apply broadly to allonUncaughtErrordeliveries. Add a short inline note aroundobserveHttpSessionExpirationso future readers keep this implementation dependency visible.🤖 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 `@apps/core/src/mcp/registry.ts` around lines 224 - 232, Add a short inline comment near observeHttpSessionExpiration documenting that the HTTP transport clears the session before onUncaughtError handles the 404-based session error, while this ordering is not a general onUncaughtError guarantee. Keep the existing isOptionalHttpInboundSseError logic unchanged.
🤖 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 `@apps/core/src/mcp/registry.ts`:
- Around line 38-43: Add an acceptance test covering the schema represented by
optionalHttpInboundSseErrorSchema, asserting MCPClientError messages must start
with the exact “MCP HTTP Transport Error: GET SSE failed:” prefix and that a
changed prefix is rejected. Keep the existing statusCode and url validation
unchanged.
---
Nitpick comments:
In `@apps/core/src/mcp/registry.ts`:
- Around line 565-569: In the handler around isOptionalHttpInboundSseError, add
a debug- or trace-level log containing the tolerated error and relevant inbound
SSE context before the existing return. Preserve the non-fatal early-return
behavior and use the surrounding logger and established safe unknown-error
conversion conventions.
- Around line 224-232: Add a short inline comment near
observeHttpSessionExpiration documenting that the HTTP transport clears the
session before onUncaughtError handles the 404-based session error, while this
ordering is not a general onUncaughtError guarantee. Keep the existing
isOptionalHttpInboundSseError logic unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8d3ad5aa-c439-45c6-9317-e8c3d4e9f492
📒 Files selected for processing (2)
apps/core/src/mcp/registry.tsapps/core/tests/mcp/registry.test.ts
| const optionalHttpInboundSseErrorSchema = z.object({ | ||
| name: z.literal("MCPClientError"), | ||
| message: z.string().startsWith("MCP HTTP Transport Error: GET SSE failed:"), | ||
| statusCode: z.number().int(), | ||
| url: z.string(), | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does @ai-sdk/mcp MCPClientError expose a request phase or kind field to distinguish GET SSE stream errors from POST request errors, or only statusCode/url/responseBody?
💡 Result:
No, the @ai-sdk/mcp MCPClientError class does not expose a "request phase kind" field to explicitly distinguish between GET (SSE stream) and POST request errors [1][2]. As of the current implementation, the MCPClientError class exposes the following properties [1][2]: - data: Optional data payload from the error (e.g., from the server's JSON-RPC error response) [1][3]. - code: Optional JSON-RPC error code [1]. - statusCode: Optional HTTP status code, if the error originated from an HTTP transport failure [1][4]. - url: Optional URL of the endpoint that failed [1][4]. - responseBody: Optional body of the failed HTTP response [1][4]. These fields (specifically statusCode, url, and responseBody) were added to provide structured HTTP context for HTTP transport failures, allowing consumers to make decisions based on the HTTP status without needing to parse error message strings [4][5]. However, the class does not include a specific field designed to identify the transport phase (e.g., distinguishing an initial SSE connection/GET request from a tool invocation/POST request) [1][2].
Citations:
- 1: https://app.unpkg.com/@ai-sdk/mcp@1.0.48/files/src/error/mcp-client-error.ts
- 2: https://app.unpkg.com/@ai-sdk/mcp@1.0.46/files/src/error/mcp-client-error.ts
- 3: vercel/ai@bb10a89
- 4: https://github.com/vercel/ai/releases/tag/%40ai-sdk/mcp%402.0.0
- 5: https://github.com/vercel/ai/blob/main/packages/mcp/CHANGELOG.md
Pin the GET-SSE message match to avoid silent behavior change.
@ai-sdk/mcp does not expose a stable request-kind discriminator for MCPClientError; statusCode, url, and responseBody do not distinguish GET-SSE stream failures from POST failures. Add an acceptance test for the exact message.startsWith("MCP HTTP Transport Error: GET SSE failed:") string so a library text change fails CI instead of silently changing whether failures are tolerated.
🤖 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 `@apps/core/src/mcp/registry.ts` around lines 38 - 43, Add an acceptance test
covering the schema represented by optionalHttpInboundSseErrorSchema, asserting
MCPClientError messages must start with the exact “MCP HTTP Transport Error: GET
SSE failed:” prefix and that a changed prefix is rejected. Keep the existing
statusCode and url validation unchanged.
2 participants
Summary
@ai-sdk/mcpreports a rejected optional inbound GET event stream while POST initialization and discovery still workVerification
cd apps/core && bun test tests/mcp/registry.test.ts— 25 passedcd apps/core && bunx tsc -p tsconfig.json --noEmitbun run typecheckbun run lintbun run fmt:checkFull-suite note
bun run test:allwas attempted in the sandbox. The affected MCP suite passed, but unrelated environment-bound tests failed: three Codex OAuth callback tests could not connect to their loopback listener, and six SSH transport tests were denied while spawning/usr/bin/ssh. No files in either failing area are changed by this PR.Summary by CodeRabbit