Make MCP connections survive transient transport failures - #1184
Merged
Conversation
Contributor
Author
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Contributor
Greptile SummaryThis PR makes MCP HTTP connections recover from transient transport failures while immediately retiring authentication failures.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (2): Last reviewed commit: "Recover MCP connections from transient d..." | Re-trigger Greptile |
Contributor
Track whether MCP failures occur while connecting or running a tool call. A provider can map a per-call authorization denial to HTTP 403, so status alone is not a session verdict. Keep request-level permission and protocol failures local to the call while retiring connections for connection-level failures.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A single non-2xx response from a remote MCP server currently takes the connection out for the rest of a run. The streamable-HTTP SDK runs each request inside the transport's task group and calls
raise_for_status()there, so an error response tears the group down and the in-flight caller sees a bareCancelledErrorwith no status attached.SupervisedMcpSession._executethen reconnects once, replays the call immediately, and treats a second failure as permanent — so a transient 429 or 5xx is indistinguishable from a revoked token, and both end the connection.Three things change: failures get classified, retries get a policy, and death gets a cooldown.
Classification. The status is captured before the SDK swallows it, via an httpx response hook installed by
_build_server:classify()maps httpx/SDK errors (including nestedBaseExceptionGroups, by specificityauth > permission > rate_limit > server > protocol > timeout > transport) to aFailureInfo, and when a cancellation arrives with no exception the supervisor reads the recorder instead of guessing. Only status, reason,Retry-After, request method and URL path are retained — never headers, query strings or bodies.Timeouts were both defaulted to 5s — the SDK defaults
timeoutandclient_session_timeout_secondsto 5, and_build_serverpassed neither. Any tool slower than that was killed by our own configuration. They are now explicit and per-connection (http_timeout_seconds=30,session_timeout_seconds=60,sse_read_timeout_seconds=300).Retry and quarantine replace the one-shot strike counter:
Quarantine cleans up the failed server and clears it, so a lazy revive on the next dispatch builds a fresh session rather than reusing a known-broken one, and the supervising task now stays alive across it — including on an idle session death, which previously ended the task outright.
is_deadkeeps its meaning: permanent for the run.Concurrency is bounded per connection name (
max_concurrent_calls=4) so several agents sharing one provider session cannot fan out unboundedly into a rate limit. Semaphores are keyed per event loop through aWeakKeyDictionary, since one connection name can be served from more than one loop.A status is not a session verdict
Deciding permanence from the HTTP status alone retires healthy connections. A provider may map a per-request authorization denial onto an HTTP 403 of the tool-call POST — the credential is fine and the session is fine, but this request's arguments named a resource the credential may not read. Observed in practice: the same tool succeeded and then returned 403 twelve seconds later on the same session, differing only in one argument. The connection was retired, and every later call on it failed with "unavailable" for the rest of the run.
So a failure now carries the phase that produced it, and only session-level failures are session verdicts:
_Outcomegrows a third possibility next to value and dead:call_failure, meaning the session is fine and this request was rejected.dispatch()turns it into the standard failed-tool output; it neither retires the connection nor spends a quarantine strike, and the next call rebuilds the session lazily as any post-failure call already did.list_tools()runs in the connect phase, so its behavior is unchanged.The message the agent gets says whose fault it is, since the previous copy ("connection is unavailable... it will not be retried") sent the model chasing a connection problem instead of its own arguments:
The classifier splits
403out ofauthinto its ownpermissionkind to express this; both remain non-retryable, soretryableis unchanged for every existing caller.Tests
Cover classification and nested groups,
Retry-Afterin both seconds and HTTP-date form, 429 retry, 5xx quarantine and revival, immediate retirement on 401, cancellation combined with a recorded status, timeout propagation, the concurrency cap, and redaction. One drives a realhttpx.AsyncClientbuilt through_build_serverover aMockTransport— without it a sync response hook passes every mocked test and fails on every real request, becauseAsyncClientawaits its hooks.For the phase split: a 403 and a 400 raised by a dispatched call each leave the connection alive, unquarantined and with no strike, and the next call on the rebuilt session succeeds; a 403 raised by
list_toolsstill retires the connection; a 401 raised by a call still retires it immediately.Link to Devin session: https://app.devin.ai/sessions/cf9f20a751754792b512e722b852b6f8
Open in Devin Desktop: https://app.devin.ai/desktop/session/cf9f20a751754792b512e722b852b6f8?variant=devin
Requested by: @yoni-at-strix