Repository navigation
fix(router): stop retries and fallbacks from re-running executed MCP tools - #43159
Atharva-Kanherkar wants to merge 15 commits into
Conversation
A retry or fallback re-runs the whole MCP gateway loop, so a failure raised after the loop already executed tool calls ran those tools again on every replay. Add a tag for such errors and have the router surface a tagged error from the retry loop and the fallback path instead of replaying the request The error is tagged rather than wrapped, like the advisor orchestration tag, so its type, status, and message reach the client unchanged. Untagged errors keep their current retry and fallback behavior Signed-off-by: Atharva-Kanherkar <142440039+Atharva-Kanherkar@users.noreply.github.com>
When the non-streaming Chat Completions MCP loop has executed tool calls and the follow-up model call fails, tag the error so the router surfaces it instead of replaying the loop and executing the tools again The regression test drives a real Router through the MCP gateway with a counting tool and a 500 from the follow-up call. It asserts one tool execution and one follow-up call with retries and with a fallback group, and a control asserts that a 500 before any tool ran is still retried Signed-off-by: Atharva-Kanherkar <142440039+Atharva-Kanherkar@users.noreply.github.com>
The non-streaming Responses MCP loop has the same replay problem as Chat Completions: once it has executed tool calls, a failed follow-up call made the router re-run the loop. Tag that failure so the router surfaces it, and leave a failure of the initial call untagged so it is still retried Signed-off-by: Atharva-Kanherkar <142440039+Atharva-Kanherkar@users.noreply.github.com>
The /v1/messages MCP loop runs inside one router attempt for both streaming and non-streaming requests, so a failed follow-up call after tool execution made the router re-run the loop. Tag that failure so the router surfaces it, and leave a failure of the initial call untagged so it is still retried Signed-off-by: Atharva-Kanherkar <142440039+Atharva-Kanherkar@users.noreply.github.com>
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
The tag was set whenever tool results existed, including results for calls that a pre-call guardrail rejected before any MCP server ran them, so those requests lost their retries and fallbacks. The executor now reports each call an MCP server returned for, and a small guard tags failures only after that The guard also covers the Responses tool re-listing that runs after the follow-up call, and the router finds the tag through an exception's cause and context, because exception_type re-raises errors that are not LiteLLM exceptions as new objects The regression tests move to a test file for the new module. They drive a real Router through the MCP gateway for Chat Completions, Responses and /v1/messages with respx at the HTTP boundary, so no test patches a private class method Signed-off-by: Atharva-Kanherkar <142440039+Atharva-Kanherkar@users.noreply.github.com>
The tagged-error tests pass their failing attempt as original_function and let the real retry loop, healthy deployment lookup and backoff run, instead of patching make_call and other router methods Signed-off-by: Atharva-Kanherkar <142440039+Atharva-Kanherkar@users.noreply.github.com>
Follow the cause, or else the context, the same way Python walks an exception chain, and state exactly when the executor calls on_tool_executed Signed-off-by: Atharva-Kanherkar <142440039+Atharva-Kanherkar@users.noreply.github.com>
|
@greptileai please review again: the tag now requires an executed tool, the Responses re-listing is guarded, and tests avoid private patches |
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 2 · PR risk: 0/10 |
The code quality job forbids recursive functions, so follow the chain with a bounded loop instead, the same way the database exception handler does. The depth cap also keeps a malformed cyclic chain from looping forever Signed-off-by: Atharva-Kanherkar <142440039+Atharva-Kanherkar@users.noreply.github.com>
A tool can write and still fail to return, for example on a timeout, so waiting for the server's result left that write unrecorded and its failed follow-up replayable. MCPServerManager.call_tool now reports the moment a call passes its pre-call checks and is sent, and the replay guard records that instead. A call a pre-call guardrail rejects is still never sent, so its request keeps its retries Signed-off-by: Atharva-Kanherkar <142440039+Atharva-Kanherkar@users.noreply.github.com>
In a streaming Chat Completions request the tools run inside the stream, and a follow-up stream that breaks before its first chunk triggers the router's mid-stream fallback, which re-runs the whole MCP loop. Record the streamed tool calls and read the follow-up stream inside the replay guard, so that failure is surfaced instead Signed-off-by: Atharva-Kanherkar <142440039+Atharva-Kanherkar@users.noreply.github.com>
|
@greptileai please review again: tool calls now count once sent to the MCP server, and streamed chat follow-ups are guarded too |
A call was recorded before the MCP client resolved its headers and opened a session, so a call that failed there, for example on a per-user credential the caller had not set, still blocked retries although no tool could have run. MCPClient.call_tool now reports the dispatch right before the session sends the call, and the OpenAPI path right before its tool handler runs Signed-off-by: Atharva-Kanherkar <142440039+Atharva-Kanherkar@users.noreply.github.com>
|
@greptileai please review again: a tool call now counts only when the MCP session sends it, so unsent calls keep retries |
The repository extends the existing mapped test file for a bug fix, so the end-to-end MCP replay tests move next to the router's tagged-error tests in test_router_retry_non_retryable_errors.py instead of a new file Signed-off-by: Atharva-Kanherkar <142440039+Atharva-Kanherkar@users.noreply.github.com>
|
@greptileai please review again: the replay tests now extend the existing router retry test file instead of adding a new one |
Resolve the conflicts with the MCP result conversion change (BerriAI#43089). Its allow_input_required and wire_compat parameters now sit next to on_dispatch in the MCP client, the OBO retry helper, the regular tool path and call_tool, and the test fakes accept both keywords Signed-off-by: Atharva-Kanherkar <142440039+Atharva-Kanherkar@users.noreply.github.com>
The repository keeps comments for complex business logic, tool directives and TODOs, so the notes on the chain depth cap and the test fixture's setup go, and the names carry that meaning Signed-off-by: Atharva-Kanherkar <142440039+Atharva-Kanherkar@users.noreply.github.com>
|
@greptileai please review again: the routine comments are gone, and the branch now merges the latest main |
…reexecution Signed-off-by: Atharva-Kanherkar <142440039+Atharva-Kanherkar@users.noreply.github.com> # Conflicts: # litellm/router.py
|
@joshua-berri I have resolved the conflicts with main, could you please take a look when you get a chance? |
|
The |
|
can anyone in the maintainers see this? |
TLDR
Problem this solves:
How it solves it:
Intentional product change: when the model call that follows an auto-approved MCP tool call fails after that call was sent to its MCP server, the client now gets the error right away. Before, the router retried or fell back to another model group, which re-ran the whole MCP loop and executed the tool again on every attempt. Requests where no tool call reached a server, for example because a guardrail blocked every call or a per-user credential was missing, are retried exactly as before
User Flow
Before: an app that records entries through an auto-approved MCP tool gets one error and three writes
num_retries: 2and adds an MCP serverledgerwhosecommit_writetool appends a line to a filealphawith that tool (/v1/responses and /v1/messages behave the same)alphalinesAfter: the same request gets the same 500 and exactly one write
num_retries: 2and adds an MCP serverledgerwhosecommit_writetool appends a line to a filealphawith that tool (/v1/responses and /v1/messages behave the same)alphaline, so the app decides whether to try againRelevant issues
Fixes #43153
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
pytest tests/test_litellm/test_router_retry_non_retryable_errors.py tests/test_litellm/experimental_mcp_client/test_mcp_client.py tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server_manager.py tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_max_concurrent_requests.py(1207 passed). The tests only extend existing test files. The end-to-end ones drive a real Router through the MCP gateway with respx at the HTTP boundary, without patching private class methods. They fail on the merge base, and each earlier revision of this PR fails the tests added for the finding raised on it99b3c06. On4ad4a4devery check passed except two tests in the fork-only part ofmcp-integration,test_mcp_client_unit.py::test_call_toolandtest_mcp_server.py::test_sse_mcp_handler_mock. They fail the same way onmainat61953318without this PR, because feat(mcp): share compatibility-aware result conversion across tool surfaces #43089 addedallow_input_requiredto the session call and moved SSE toserve_loop, and these CircleCI-only tests still expect the old calls. Every script of the code quality workflow,ruff check,ruff format --check, the strict ruff, type discipline and test quality gates, the MCP operation boundary check, and the circular import and import safety checks pass locally, with no new basedpyright errors in any changed file)99b3c06, and a new review is requested)Delays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
Screenshots / Proof of Fix
Shared setup: the proxy runs from this branch's merge base and from its head with
python litellm/proxy/proxy_cli.py --config config.yaml --port 4000. Every model call is a real, paidgpt-5.4-minicall to OpenAI, relayed by a small local forwarder on 127.0.0.1:8765 that logs every request asinitialorfollow_up. It forces the otherwise rare transient failures with flag files: whilefault_onexists it answers every follow-up request (the one that carries the tool result) with HTTP 500 instead of relaying it,fault_oncedoes the same for one follow-up and is then removed, anddrop_streammakes it send the headers of a streamed follow-up and then close the connection. The MCP server is a real FastMCP streamable HTTP server (mcp==1.30.0) on 127.0.0.1:8766 whose one tool,commit_write, appends its entry toledger.txt. Theblock-mcpguardrail is a documented custom guardrail that rejects every MCP tool call before it runs; a request opts in with"guardrails": ["block-mcp"]. The streaming case usesconfig-stream.yaml, which isconfig.yamlplus a router fallback fromrepro-modeltobackup-model. With an OpenAI model the proxy sends /v1/messages upstream as a Responses call, so that case logs /v1/responsesMCP server, forwarder and guardrail
Before (6195331)
/v1/chat/completions
Send the request and read the error
Count the tool runs and the upstream calls: three writes, and the whole loop ran three times
/v1/responses
Send the request and read the error
Count the tool runs and the upstream calls: three writes, and the whole loop ran three times
/v1/messages
Send the request and read the error
Count the tool runs and the upstream calls: three writes, and the whole loop ran three times
Guardrail blocks the tool and the follow-up fails once
Send each request with the guardrail selected, failing its first follow-up call
Count the tool runs and the upstream calls: no write, and each request was retried once after its failed follow-up
No injected failure
Send each request once without any fault
Count the tool runs and the upstream calls: one write and one initial plus one follow-up call per request
Streaming chat whose follow-up stream drops, with a router fallback
Start the proxy with
config-stream.yaml, stream the chat request, and let the forwarder drop the follow-up stream right after its headersCount the tool runs and the upstream calls: two writes, because the fallback re-ran the whole loop and its chunks were spliced after the original ones
After (99b3c06)
/v1/chat/completions
Send the request and read the error
Count the tool runs and the upstream calls: one write, one initial call and one follow-up call
/v1/responses
Send the request and read the error
Count the tool runs and the upstream calls: one write, one initial call and one follow-up call
/v1/messages
Send the request and read the error
Count the tool runs and the upstream calls: one write, one initial call and one follow-up call
Guardrail blocks the tool and the follow-up fails once
Send each request with the guardrail selected, failing its first follow-up call
Count the tool runs and the upstream calls: the same as the merge base, so a request where no tool call reached a server is still retried
No injected failure
Send each request once without any fault
Count the tool runs and the upstream calls: one write and one initial plus one follow-up call per request
Streaming chat whose follow-up stream drops, with a router fallback
Start the proxy with
config-stream.yaml, stream the chat request, and let the forwarder drop the follow-up stream right after its headersCount the tool runs and the upstream calls: one write, and the client gets the error after the original chunks instead of a fallback that runs the tool again
Type
Bug Fix
Caveats (if any)
Medium
Low
disable_fallbacksexit skips it the same waynum_retrieswrapper, used without a router, is unchangedFinal Attestation