fix(mcpProxy): propagate server send errors back to client as JSON-RPC errors - #297
fix(mcpProxy): propagate server send errors back to client as JSON-RPC errors#297sahilm-ti wants to merge 2 commits into
Conversation
…C errors When transportToServer.send(message) rejects (e.g. due to a transport-level HTTP error on the remote endpoint), the error was caught by onServerError which logs it to stderr and returns — leaving the client's awaiting request with no response. Any client blocking on that request id parks indefinitely. Fix: capture message.id before dispatching to the server, then in the catch handler send a JSON-RPC error response back to the client so the awaiter is woken immediately. Notifications (no id) are unaffected. The existing onServerError logging is preserved so the error still appears in stderr for diagnostics.
Pattern 2 of the 429 hang: when _startOrAuthSse (SSE GET) receives a 429 it throws StreamableHTTPError, caught by the transport's catch block which calls this.onerror?.(). This routes to mcpProxy's onServerError, which previously only logged -- leaving any in-flight requests parked indefinitely. Fix: track all in-flight request ids in a pendingRequests Map within mcpProxy scope. When a message with an id is sent to the server it is added to the map; when a server response arrives (onmessage) or the send().catch() fires (Pattern 1) it is removed. onServerError now drains the entire map, sending a JSON-RPC -32603 error for each pending id so client awaiters are unblocked immediately. This covers Pattern 2 (bare 'code: 429' in proxy logs with no 'Connection error:' prefix, child stays alive, session hangs until client disconnects). Pattern 1 (send().catch() path) is already handled; the pendingRequests delete in that path ensures no double-send. 103 unit tests pass.
Update: also fixes Pattern 2 (SSE/onerror path)Testing after the original commit revealed a second 429 hang path that this PR did NOT fix: Pattern 1 (fixed by original commit): Pattern 2 (now also fixed): Pattern 2 appeared in proxy logs as bare Fix (just pushed): Added a
Verified: 5× 40-session parallel hammer test (mix of |
|
I hit #293 independently and verified this PR against it (79f5f10). It does fix the hang, but Pattern 1 and Pattern 2 aren't mutually exclusive, and when both fire for the same request the client gets two responses for one ID. Repro, any non- (echo '{"jsonrpc":"2.0","id":1,"method":"tools/list","params":{}}'; sleep 20) \
| node dist/proxy.js https://mcp.atlassian.com/v1/mcp/authv2On
The pairing shows the order: |
|
We recently forked mcp-remote and fixed a number of auth-related problems. If it's helpful, you can try the fork here: https://github.com/abluva/mcp-remote We'll be maintaining it going forward, so feel free to open issues or PRs there if you run into anything else. |
Fixes #293.
Problem
When
transportToServer.send(message)rejects due to a transport-level error (HTTP 4xx/5xx, connection refused, timeout, etc.),mcpProxycatches the error viaonServerError, logs it to stderr, and returns. The local client's awaiting request receives no response and parks indefinitely.message.idis not passed toonServerErrorso it cannot send a response even if it wanted to. TheignoredToolspath in the same function already demonstrates the correct pattern — it sends a JSON-RPC error back to the client — but that pattern wasn't applied to the general error case.Fix
Capture
message.idbefore dispatching, then use it in the.catch()handler to send a JSON-RPC error back to the client:Notes
id) are unaffectedonServerErrorstderr logging is unchangedtsc --noEmitpasses cleanlymcpProxydirectly; happy to add one if wanted