fix(mcp): reconnect Streamable HTTP after MCP server restart - #12777
Conversation
…without bogus SSE fallback When the remote MCP process restarts, Langflow kept a stale ClientSession: tool list updates could fail with "Session terminated", the session health check could re-raise instead of discarding the session, and any Streamable HTTP failure triggered SSE fallback and could lock transport preference to SSE—masking the real issue and producing misleading TaskGroup/dual-transport errors. Invalidate all sessions for the server URL on list_tools failure, treat any health-check failure as a dead session, retry transient Streamable HTTP errors before considering SSE, only fall back to SSE for clear transport-mismatch signals (e.g. 404/405/406), reset and re-record transport preference accordingly, and bust sessions on run_tool using the same termination/connection signals. Add unit tests with mocked streamablehttp_client and sse_client.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
erichare
left a comment
There was a problem hiding this comment.
Overall LGTM — the transient-vs-transport-mismatch split is the right model, invalidate_server_key cleanly fixes the stale-session bug, and tests cover the key paths. No blockers from my read — merge-worthy.
Two things worth addressing (non-blocking, could be follow-ups):
-
Substring matching on
str(exc)for "404/405/406" in_should_attempt_sse_after_streamable_failurewill false-positive on unrelated errors whose message happens to contain those digits, silently flipping to SSE. Prefer typed checks only, or word-boundary match. -
_validate_session_connectivitynow catches bareException. Solves the real bug, but a targeted widening (McpError,ClosedResourceError,BaseExceptionGroup) would avoid masking unrelated errors.
Nits: _iter_exception_leaves can use plain import builtins; invalidate_server_key is public but callers reach into private _get_server_key — consider a public helper.
16432b6
…without bogus SSE fallback When the remote MCP process restarts, Langflow kept a stale ClientSession: tool list updates could fail with "Session terminated", the session health check could re-raise instead of discarding the session, and any Streamable HTTP failure triggered SSE fallback and could lock transport preference to SSE—masking the real issue and producing misleading TaskGroup/dual-transport errors. Invalidate all sessions for the server URL on list_tools failure, treat any health-check failure as a dead session, retry transient Streamable HTTP errors before considering SSE, only fall back to SSE for clear transport-mismatch signals (e.g. 404/405/406), reset and re-record transport preference accordingly, and bust sessions on run_tool using the same termination/connection signals. Add unit tests with mocked streamablehttp_client and sse_client.
This is the rebase of #12270 onto 1.10.0 as #12270 never seemed to have landed in 1.9.0
This PR contains the fix to reconnect streamable HTTP after MCP server restart without bogus SSE fallback
When the remote MCP process restarts, Langflow kept a stale ClientSession: tool list updates could fail with "Session terminated", the session health check could re-raise instead of discarding the session, and any Streamable HTTP failure triggered SSE fallback and could lock transport preference to SSE—masking the real issue and producing misleading TaskGroup/dual-transport errors.
This invalidates all sessions for the server URL on list_tools failure, treat any health-check failure as a dead session, retry transient Streamable HTTP errors before considering SSE, only fall back to SSE for clear transport-mismatch signals (e.g. 404/405/406), reset and re-record transport preference accordingly, and bust sessions on run_tool using the same termination/connection signals.
Add unit tests with mocked streamablehttp_client and sse_client.
Please check if this makes sense and if we should do those retries…