fix(mcp): recover after HTTP session failure - #4838
Conversation
Co-authored-by: openhands <openhands@all-hands.dev>
|
🚦 CI is currently failing on this PR's latest commit. Please fix the failing checks before OpenHands reviews it - this is re-checked automatically once you push a new commit. (A maintainer can also request This is an automated check - no AI was used to generate this comment. |
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Summary
This PR fixes a real bug in the MCP client reconnect path. When a transient HTTP error kills FastMCP's background session task, _context_manager's finally block calls _reset_session_state() without full=True, which clears session and initialize_result but leaves nesting_counter at 1 and session_task pointing to the completed task. The SDK's call_tool then sees is_connected() == False and calls connect(), but FastMCP's _connect() refuses to start a new session because nesting_counter != 0, raising RuntimeError -> MCPError("MCP Connection Failure"). The fix correctly resets the stale state under the existing session lock before calling __aenter__().
I verified the fix by tracing through FastMCP 3.4.7's _connect(), _disconnect(), _session_runner(), _context_manager(), and _reset_session_state() source. The reset under the lock is safe -- _connect() re-checks session_task state under the same lock, so the brief window between the reset and __aenter__() cannot cause issues (if another caller starts a session in between, _connect() just reuses it via the reentrant counter).
The regression test is solid -- it uses a real uvicorn HTTP server with middleware that injects a 504 on the first tools/call, then verifies the second call succeeds with pong. I ran it locally and it passes.
No material code issues found.
Risk Assessment: LOW
The production change is 4 lines targeting only the error-recovery path. Normal tool execution (no transient errors) is completely unchanged. The fix is strictly an improvement -- it makes reconnection work instead of failing permanently.
Since this touches MCP tool execution behavior, flagging for a human maintainer to decide whether lightweight evals are warranted. My assessment is that eval impact is negligible given the change only affects the error-recovery path, but the call is yours.
Verdict: Worth merging. The fix is correct, minimal, and well-tested.
HUMAN:
I reproduced the issue and confirmed this fix is ready for review.
AGENT:
Why
A transient HTTP error can terminate FastMCP's session task while leaving its nesting counter nonzero. The SDK already retries disconnected tools, but that retry then fails with
MCP Connection Failurebecause FastMCP refuses to start another session from the stale state.Summary
Issue Number
Fixes #4837
How to Test
End-to-end reproduction used a local FastMCP HTTP app rather than mocked client state. Its middleware returned HTTP 504 for the first
tools/call; the first SDK observation contained504 Gateway Timeout, and the second call returnedpong.Results: the regression passed three consecutive runs; the MCP suite passed with 135 tests; all pre-commit hooks passed.
Video/Screenshots
Not applicable; this is an SDK connection lifecycle fix covered by the real HTTP regression above.
Design Doc
Not applicable; the production change is a four-line state reset using FastMCP's existing lock and reset method.
Type
Notes
Draft PR #3444 reported the same stale FastMCP counter on an older SDK architecture. This PR targets the current automatic reconnect path and keeps the workaround at that shared reconnect boundary. Issue #4837 needs maintainer triage to add the
buglabel, after which the repository workflow can applyready-for-dev.The real HTTP reproduction fails with the repository's locked FastMCP 3.2.0 and also with 3.4.7. FastMCP 4.0.2 does not reproduce it because its session implementation changed; upgrading to that breaking major remains separate from this compatibility fix for supported FastMCP 3.x installations.