Skip to content

WIP: fix: end the remote MCP connect wait on a bad reply or no reply - #2007

Draft
chiang-daniel wants to merge 1 commit into
mainfrom
dchiang/mcp-connect-time-limit
Draft

chiang-daniel wants to merge 1 commit into
mainfrom
dchiang/mcp-connect-time-limit

Conversation

@chiang-daniel

Copy link
Copy Markdown
Contributor

Description
REPLACE: what this PR is, in 1 to 2 sentences max.

Author Review (required)

  • I have done a code review

Architecture Review (select 1)

  • I did architecture review before coding
  • Small change, no architecture review needed
  • Requesting architecture review exception for other reason: NA

Review Requested from Code Reviewer (select 1)

  • Sign off on AI-only review: [reason - required if selected]
  • Mixed: human review of some areas, AI on others (details below)
  • Full human: human reads everything

Agentic Code Review (must check all before requesting CR)

  • I have run /spec deep cr on this PR or used /spec CRs throughout
  • I have addressed all AI feedback (“deep cr”, CodeRabbit, etc)

What to Review

  • Key decisions to review
    • REPLACE: 1-4 decisions you made.
  • Paths to review
    • REPLACE: List of paths/files to review. example libs/code/adapters/KevAdapter.py

UI Review (select all that apply)

  • Agentic UI clickthrough done
  • Requires UI review as part of this review
  • UI review already done by: person
  • No UI

Agentic PR Summary

TLDR: A user's remote MCP connect spun forever, because the server sent a web page and the MCP SDK left the request waiting. This PR fails the connect at once with the MCP SDK's error, and gives each request a 30-second limit, as local MCP has. Targets main. No companion PRs.

Before this PR, POST /api/projects/{project_id}/connect_remote_mcp waited with no limit in two cases: the server sent a reply that the MCP SDK cannot read (for example a web page), or the server sent no reply. The spinner in the UI did not stop. The user's log showed Unexpected content type: text/html;charset=utf-8 from mcp.client.streamable_http, and then nothing. After this PR, an unreadable reply fails the connect at once, and the error box shows the MCP SDK's text. A server that sends no reply fails after 30 seconds.

Implementation

  • _create_remote_mcp_session gives ClientSession a message_handler. MCP SDK 1.30.0 sends an unreadable reply to this callback and leaves the request waiting. The callback keeps the error and raises it, so the connect fails with that error and not with "Connection closed".
  • _create_remote_mcp_session sets read_timeout_seconds=timedelta(seconds=30). This is the value that the local MCP session uses.
  • _handle_remote_mcp_error now also looks for McpError, as the local error list does. The user then sees the MCP SDK's text and not "unhandled errors in a TaskGroup".
  • The new tests run the real MCP SDK transport over httpx.MockTransport. They use no sockets.

Before you merge

  • MCPSessionManager.mcp_client is public in libs/core. On a remote server, each request in its block now has a 30-second limit, call_tool included. Kiln itself uses mcp_client only for list_tools. A remote server that needs more than 30 seconds to start now fails to connect.
  • One unreadable message now ends the session, also when no request waits for it. Examples are a bad event on the background GET stream, or a duplicate or late reply. On main, the MCP SDK logs these and the connect continues.
  • Each unreadable reply also writes the MCP SDK's "Unhandled exception in receive loop" traceback to the log.

Not in this PR

  • Agent runs use _create_cached_remote_session. That session has no limit and no callback, so an agent run against the same server still waits with no limit. The McpError change does improve its connect error text.
  • A server that does not answer the notification after initialize gets past the 30-second limit, because the MCP SDK starts the timer only after it sends the next request. The httpx read timeout of 5 minutes ends it, and the error box shows "unhandled errors in a TaskGroup".
  • MCP SDK 2.3.0 fails unreadable replies itself, and only logs a raise from the callback. The move to MCP SDK 2.x removes the callback and keeps the 30-second limit. KIL-859 tracks it.

Related: KIL-859.

Checks: uv run ./checks.sh --agent-mode green. Two agent reviews ran: /spec cr, and Claude Code /code-review at high effort. The only code change from them is the comment on end_on_unreadable_reply. The other findings are the items above. A break of each part of the fix (the callback, the kept error, McpError, the limit) makes a new test fail. Tested against fake servers only, not a real Spring AI server. This final version was not run in the desktop app.

Docs and skills updated: none needed, no doc or skill names the remote MCP session code or its limits.

Errors the user sees
Server behavior main This PR
Sends a web page Spinner never stops Unexpected content type: text/html;charset=utf-8, at once
Sends a JSON-RPC error unhandled errors in a TaskGroup (1 sub-exception) The server's error message
Sends no reply Spinner never stops Timed out while waiting for response to ClientRequest. Waited 30.0 seconds.

🤖 Generated with Claude Code

…mcp_session

The remote MCP connect waited forever when the server sent a reply the
MCP SDK can't read, such as a web page, or sent no reply. The new
message_handler keeps the MCP SDK's error and raises it, so the connect
fails at once with that error. The session uses the local session's
30-second read timeout. McpError joins the remote error list so the
MCP SDK's text reaches the user.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Nov 6, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Ignore keyword(s) in the title.

⛔ Ignored keywords: 2

  • WIP
  • Draft

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

Run configuration

  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: d13ebd90-1248-45d3-97be-882115895408

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • Trigger review
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Tip

🎯 Customize your risk assessments

Guide how CodeRabbit assesses merge risk and security architecture risk. Highlight sensitive changes and explain your security boundaries using Merge Risk Instructions and Security Risk Instructions in your review settings. Security architecture risk assessments require the Advanced plan or above.

✅ Automatic approvals, with your rules

Let CodeRabbit approve pull requests once required threads are resolved, pre-merge checks pass, and your risk limits are met. Enable Automatic approvals in review settings, with overrides for teams or file paths.

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.

Share

@github-actions

github-actions Bot commented Nov 6, 2026

Copy link
Copy Markdown

📊 Coverage Report

Overall Coverage: 94%

Diff: origin/main...HEAD

  • libs/core/kiln_ai/tools/mcp_session_manager.py (100%)

Summary

  • Total: 7 lines
  • Missing: 0 lines
  • Coverage: 100%

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant