fix(responses): fail loudly on MCP gateway failures (initial call, mid-stream, zero resolved tools) - #32648
Conversation
…out (#31632) * fix(prometheus): bound per-request budget metric emission with a timeout Wrap the per-request budget-metric gather in asyncio.wait_for so a slow Redis or DB lookup cannot consume the whole LoggingWorker watchdog and get the success-logging event cancelled. On timeout the emission is skipped in isolation; budget gauges are still refreshed by the periodic cron. The timeout is configurable via PROMETHEUS_BUDGET_METRICS_PER_REQUEST_TIMEOUT and defaults to 5.0 seconds, falling back to the default on an invalid value instead of raising * fix(prometheus): reject non-finite and non-positive budget-metrics timeout env float() accepts 0, negatives, nan and inf, which bypass the fallback: a value <= 0 makes asyncio.wait_for time out immediately and skip every per-request emission, and inf reintroduces the unbounded wait the timeout was meant to bound. Validate the parsed value is finite and greater than zero before using it, otherwise fall back to the default
When a guardrail blocks a post-call response, the synthetic violation response reported hard-coded zero usage, discarding the token usage the upstream call had already consumed. Fix the root cause rather than re-counting tokens: - Add an optional `original_response` field to ModifyResponseException. - The unified guardrail's post-call success hook attaches the blocked LLM response to the exception. - The /v1/messages and OpenAI-format (/v1/chat/completions, /v1/completions) block handlers report `original_response.usage` directly. Pre-call blocks never invoked the LLM, so usage is zero. Mock-based tests cover the helper (returns original usage / zero), the success hook attaching original_response, and the endpoint reporting it end-to-end. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ock (#31389) Streaming moderation improvements for the unified guardrail post-call streaming iterator hook: - streaming_buffer_until_moderated: withhold all chunks until end-of-stream moderation passes, then release the original response (clean) or only the block message (blocked) -- the original content is never delivered on a block. Snapshot chunks with a shallow list() copy (end-of-stream builds a separate assembled response; chunks aren't mutated in place). - Clean Anthropic SSE on block: synthesize a well-formed termination sequence instead of a bare data: {"error": ...} blob that truncates the stream. Provider-specific synthesis lives in AnthropicMessagesHandler via build_block_sse_chunks (format-agnostic routing stays in the hook). - Mid-stream blocks continue the in-progress message (close open content block, append block message, terminate) rather than emitting a second message_start, which clients reject. Standalone envelope only when no chunks were sent (buffered path). - ModifyResponseException imported under TYPE_CHECKING + locally at runtime to avoid a module-level cyclic import. Adds regression tests for buffering (content withheld on block) and mid-stream continuation (single message_start). Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… content-rewriting guardrails - _standalone_block_chunks and _block_continuation_chunks now read real token usage from ModifyResponseException.original_response instead of hardcoding zero, matching the non-streaming _blocked_response_usage path. Shared helper moved to guardrail_translation/utils.py. - streaming_buffer_until_moderated is now forced off when the guardrail has mask_response_content=True, since buffered replay releases the withheld original chunks verbatim -- unsafe for a guardrail that rewrites content (e.g. PII masking). - Fix inverted streaming-flag precedence comment.
…-of-stream detection _check_streaming_has_ended assumed responses_so_far held ModelResponse objects with .choices, but for the Responses API the accumulated chunks are raw SSE event dicts, causing an AttributeError on every call
…emitting a broken stream When the initial LLM call inside MCPEnhancedStreamingIterator fails (e.g. an invalid previous_response_id -> provider 400 'No tool output found for function call ...'), the proxy returned HTTP 200 and the stream emitted the pre-generated mcp_list_tools discovery events with no response.created before them. That violates the Responses API streaming contract and crashes SDK stream accumulators (openai-node: "expected 'response.created' event, got response.mcp_list_tools.in_progress"). - aresponses_api_with_mcp now makes the initial call eagerly, before any SSE bytes are written, and re-raises the stashed failure so the client gets a real 4xx/5xx with the provider error body. - If a creation failure still surfaces during iteration, the stream emits a single terminal 'error' event instead of discovery events. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…low-up failures When tool execution failed as a batch, the stream proceeded to a follow-up call carrying function_call items with no outputs — rejected by the provider with 'No tool output found for function call ...' — and when the follow-up call itself failed, the stream simply ended with no terminal event. In both cases the client received HTTP 200 and a stream that looks like a truncated success: tool events, then silence. - Stash tool-execution and follow-up failures on the iterator. - Skip the doomed follow-up call entirely after a tool-execution failure. - Emit a single terminal OpenAI-style 'error' stream event carrying the mapped failure instead of ending silently. Builds on the initial-call failure handling from the previous commit (shares the _stream_error stash and _make_stream_error_event helper). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A request that explicitly asks for MCP tools via server_url litellm_proxy/... but resolves none of them (the API key/team has no access to the MCP server via allow_all_keys=false and no object-permission grant, the server name does not exist, or allowed_tools matches nothing) was silently sent to the model with no tools. The model then hallucinates, and the only trace is a list_mcp_tools spend log with status success and an empty response — the request looks healthy end to end while being completely broken. Raise a 400 BadRequestError naming the requested server URLs and the likely causes instead. Guard scope: - Mixed requests are exempt: with other (function) tools present, the request proceeds using those tools, matching the previous fallback behaviour. - Opt-out via litellm.reject_empty_mcp_resolved_tools = False (default True, per maintainer guidance). The auth-header pass-through test in tests/mcp_tests now resolves a dummy tool, since its purpose is header propagation, not zero-tool behaviour. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rop orphaned tool events on batch failure Review feedback (Greptile on #32579): - The terminal error event was numbered sequence_number=1, out of order after tool-execution events. __anext__ now tracks the highest sequence_number that passed through the stream and the error event is numbered after it. - A batch tool-execution failure queued mcp_call.in_progress events that never received a terminal per-item event. Those queued events are now dropped; the terminal error event carries the failure instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Drop test_initial_call_success_does_not_emit_error_event: the tool-call happy path (test_tool_call_happy_path_emits_no_error_event) already guards against false-positive error events and exercises more of the changed code (tool-exec + follow-up success paths). - Drop the stream=True parametrization on the zero-resolved-tools guard: the guard runs before the stream/non-stream branch in aresponses_api_with_mcp, so both cases hit identical code. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Removed MCP server configuration for deepwiki.
|
|
Greptile SummaryThis PR fixes three distinct swallow-points in the MCP gateway where client-visible failures were silently turned into HTTP 200 responses with broken or incomplete streams, or into silent no-tools model calls that caused hallucinations.
Confidence Score: 5/5Safe to merge; all three failure modes are well-guarded, opt-out flags are in place, and the existing happy path is covered by a dedicated regression test. The changes are tightly scoped to the MCP streaming path, introduce no new external dependencies, and are defended by 9 new mock-only tests that cover both the failure paths and the success path. The backwards-incompatible zero-tools behaviour is gated behind a feature flag. No auth, data-integrity, or protocol-contract issues were identified. No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/init.py | Adds reject_empty_mcp_resolved_tools: bool = True flag with a clear docstring; follows the feature-flag pattern required by the backwards-compat rule. |
| litellm/responses/main.py | Adds zero-resolved-tools guard (raises BadRequestError before any LLM call) and the eager initial-call path (_create_initial_response_iterator + re-raise) so pre-stream failures surface as HTTP 4xx rather than HTTP 200 with a broken stream. |
| litellm/responses/mcp/mcp_streaming_iterator.py | Introduces _stream_error / _initial_creation_error stash, _make_stream_error_event helper, and _last_sequence_number tracker; updates _handle_initial_response_phase, _generate_tool_execution_events, and _create_follow_up_iterator to surface failures as terminal error events instead of silent stream ends. |
| tests/mcp_tests/test_aresponses_api_with_mcp.py | Existing auth-header test now returns a dummy tool from mock_process so it does not trip the new zero-tools guard; the test purpose (header propagation) is preserved. |
| tests/test_litellm/responses/mcp/test_mcp_empty_resolved_tools.py | Five new mock-only tests covering the zero-resolved-tools guard: raises before model call, falls back on function tools, respects the opt-out flag, passes through when tools resolve, and ignores requests with no MCP tools at all. |
| tests/test_litellm/responses/mcp/test_mcp_streaming_iterator.py | Four new mock-only tests for the streaming error-surfacing paths (initial call failure, eager re-raise, tool execution batch failure, follow-up call failure) plus a happy-path regression guard; all tests use monkeypatch/AsyncMock with no real network calls. |
Reviews (3): Last reviewed commit: "Merge litellm_internal_staging into lite..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| Check if the streaming has ended. | ||
| """ | ||
| return all(response.choices[0].finish_reason is not None for response in responses_so_far) | ||
| if not responses_so_far: | ||
| return False | ||
| terminal_types = { | ||
| ResponsesAPIStreamEvents.RESPONSE_COMPLETED.value, | ||
| ResponsesAPIStreamEvents.RESPONSE_FAILED.value, | ||
| ResponsesAPIStreamEvents.RESPONSE_INCOMPLETE.value, | ||
| } | ||
| return responses_so_far[-1].get("type") in terminal_types | ||
|
|
||
| def get_streaming_string_so_far(self, responses_so_far: List[Any]) -> str: |
There was a problem hiding this comment.
AttributeError on non-dict stream items in _check_streaming_has_ended
The rewritten method calls responses_so_far[-1].get("type"), which assumes every accumulated item is a dict. For the OpenAI Responses API, stream events may arrive as Pydantic model instances (or other objects), which don't have a .get() method — causing an unhandled AttributeError that propagates out of the async_post_call_streaming_iterator_hook in unified_guardrail.py. The hasattr(_check_streaming_has_ended) guard in the caller only confirms the method exists, not that it executes safely.
The old implementation (all(response.choices[0].finish_reason ...)) was certainly broken for the Responses API format, so this is an improvement — but the new path should guard against non-dict items, e.g. using getattr(responses_so_far[-1], "type", None) instead of .get("type").
| # Silently calling the model with no tools makes it hallucinate, and | ||
| # the only trace is a list_mcp_tools spend log with an empty response — | ||
| # so fail loudly instead. | ||
| requested_mcp_urls = [tool.get("server_url") for tool in mcp_tools_with_litellm_proxy if isinstance(tool, dict)] |
There was a problem hiding this comment.
server_url key, so tool.get("server_url") silently populates the list with None values, making the error message less actionable.
| requested_mcp_urls = [tool.get("server_url") for tool in mcp_tools_with_litellm_proxy if isinstance(tool, dict)] | |
| requested_mcp_urls = [tool["server_url"] for tool in mcp_tools_with_litellm_proxy if isinstance(tool, dict) and "server_url" in tool] |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| def _blocked_response_usage(original_response: Optional[Any]) -> "litellm.Usage": | ||
| """ | ||
| Token usage for a synthetic guardrail-blocked response. | ||
|
|
||
| A post-call block replaces the LLM's response with the violation message, | ||
| but the upstream call already consumed tokens -- report that real usage | ||
| (carried on ``ModifyResponseException.original_response``) rather than | ||
| discarding it. Pre-call blocks never invoked the LLM (no original_response), | ||
| so usage is zero. | ||
| """ | ||
| usage = getattr(original_response, "usage", None) if original_response is not None else None | ||
| if isinstance(usage, litellm.Usage): | ||
| return usage | ||
| return litellm.Usage(prompt_tokens=0, completion_tokens=0, total_tokens=0) |
There was a problem hiding this comment.
_blocked_response_usage here only handles the case where original_response carries a litellm.Usage object. For a streaming chat completion block where the unified guardrail sets e.original_response = responses_so_far (a list of raw chunks), getattr(list, "usage", None) returns None and usage is silently zeroed out. The more general blocked_response_usage in utils.py handles streaming chunks and could be reused here.
Greptile SummaryThis PR fixes three silent failure modes in the MCP gateway where failures were swallowed and returned HTTP 200 with malformed or empty streams. It also adds real token-usage reporting for guardrail-blocked responses (instead of hardcoded zeros) and a configurable timeout for Prometheus per-request budget metric emission.
Confidence Score: 4/5Safe to merge; all three MCP gateway failure modes are addressed with tests and the changes are well-scoped. The guardrail-side additions are larger but covered by new tests. The core MCP streaming iterator changes are clean and well-tested. Two minor issues exist: a private litellm/proxy/proxy_server.py (_blocked_response_usage diverges from utils.py), tests/test_litellm/responses/mcp/test_mcp_streaming_iterator.py (mid-file imports), litellm/proxy/guardrails/guardrail_hooks/unified_guardrail/unified_guardrail.py (complex flag interactions worth a second read).
|
| Filename | Overview |
|---|---|
| litellm/responses/mcp/mcp_streaming_iterator.py | Core MCP fix: adds _stream_error/_initial_creation_error stash, converts silent stream ends and pre-stream LLM failures into terminal error events, and skips the doomed follow-up call after batch tool-execution failures. Logic is sound and well-commented. |
| litellm/responses/main.py | Adds zero-resolved-tools guard (raises BadRequestError before any SSE bytes) and eagerly creates the initial LLM response so pre-stream failures surface as HTTP 4xx instead of HTTP 200 with a broken stream. Both changes are correctly scoped and gated by a flag. |
| litellm/proxy/guardrails/guardrail_hooks/unified_guardrail/unified_guardrail.py | Adds streaming_buffer_until_moderated mode, mid-stream and end-of-stream ModifyResponseException handling that emits a proper SSE block sequence, and token usage propagation. Large diff with interacting state variables (chunks_yielded, responses_yielded, pending_end_of_stream_items); logic appears correct but warrants careful testing under each combination of flags. |
| litellm/llms/anthropic/chat/guardrail_translation/handler.py | Adds build_block_sse_chunks, _block_continuation_chunks and _content_block_state helpers for emitting a well-formed Anthropic SSE block sequence. SSE event parsing is duplicated from utils.py; otherwise the implementation is well-tested. |
| litellm/llms/base_llm/guardrail_translation/utils.py | Adds blocked_response_usage helper for extracting real token usage from blocked LLM responses (supports streaming SSE chunks, dicts, and attribute access). SSE parsing logic is partially duplicated from handler.py. |
| litellm/proxy/proxy_server.py | Adds _blocked_response_usage (narrow: litellm.Usage-only) and wires it into the chat/text-completion block paths. Removes the hardcoded zero-usage return before the non-streaming response. A second, richer implementation lives in utils.py — the two may drift. |
| litellm/integrations/prometheus.py | Wraps asyncio.gather for per-request budget metrics in asyncio.wait_for with a configurable timeout (PROMETHEUS_BUDGET_METRICS_PER_REQUEST_TIMEOUT, default 5 s) to prevent slow Redis/DB lookups from consuming the logging watchdog budget. Correctly skips the emit (not errors) on timeout. |
| litellm/exceptions.py | Adds original_response field to ModifyResponseException so post-call guardrail blocks can carry the LLM response's real token usage. The field is Optional[Any] with a None default, making it backward-compatible. |
| tests/test_litellm/responses/mcp/test_mcp_streaming_iterator.py | New test file covering all three MCP streaming failure modes plus the happy path; correctly uses mocks. Three module-level imports (types, MagicMock, mcp.types) appear mid-file after the second test function instead of at the top. |
| tests/mcp_tests/test_aresponses_api_with_mcp.py | Auth-header pass-through test updated to return a dummy resolved tool so the new zero-tools guard does not interrupt it. The change is correct and does not weaken test coverage — the test still validates auth header propagation. |
Reviews (2): Last reviewed commit: "Remove deepwiki MCP server configuration" | Re-trigger Greptile
| monkeypatch.setattr( | ||
| "litellm.proxy._experimental.mcp_server.mcp_server_manager.global_mcp_server_manager", | ||
| fake_manager, | ||
| ) | ||
| monkeypatch.setitem( | ||
| sys.modules, |
There was a problem hiding this comment.
Three import statements — import types, from unittest.mock import MagicMock, and from mcp.types import CallToolResult, TextContent — appear after the second test function instead of at the top of the file. Python evaluates them at module load time regardless of position, but linters (isort, ruff I) will flag them, and a future import failure on one of those symbols would raise at load time, silently preventing the entire module from being collected and making all tests look like they passed (they'd simply not run).
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| ) | ||
|
|
||
|
|
||
| def _blocked_response_usage(original_response: Optional[Any]) -> "litellm.Usage": | ||
| """ | ||
| Token usage for a synthetic guardrail-blocked response. | ||
|
|
||
| A post-call block replaces the LLM's response with the violation message, | ||
| but the upstream call already consumed tokens -- report that real usage | ||
| (carried on ``ModifyResponseException.original_response``) rather than | ||
| discarding it. Pre-call blocks never invoked the LLM (no original_response), | ||
| so usage is zero. | ||
| """ | ||
| usage = getattr(original_response, "usage", None) if original_response is not None else None | ||
| if isinstance(usage, litellm.Usage): | ||
| return usage | ||
| return litellm.Usage(prompt_tokens=0, completion_tokens=0, total_tokens=0) | ||
|
|
There was a problem hiding this comment.
_blocked_response_usage silently zeroes dict-shaped usage
The local _blocked_response_usage in proxy_server.py only returns real token counts when original_response.usage is already a litellm.Usage instance; any other representation (plain dict, Pydantic model from a non-standard provider, etc.) falls back to zeros. The richer blocked_response_usage in litellm/llms/base_llm/guardrail_translation/utils.py handles dicts and attribute access. The two helpers were born in the same PR but are already diverging — consider importing and wrapping the utils version rather than maintaining a separate implementation, to avoid usage being silently discarded as the guardrail feature evolves.
| from __future__ import annotations | ||
|
|
||
| from typing import Any, List | ||
| import json | ||
| from typing import Any, List, Optional | ||
|
|
||
| from litellm.types.llms.anthropic_messages.anthropic_response import AnthropicUsage | ||
| from litellm.types.llms.openai import AllMessageValues | ||
|
|
||
|
|
||
| def _anthropic_stream_chunk_events(item: Any) -> list[dict]: | ||
| if isinstance(item, dict): | ||
| return [item] | ||
| if isinstance(item, bytes): | ||
| chunk = item.decode("utf-8", errors="replace") | ||
| elif isinstance(item, str): | ||
| chunk = item | ||
| else: | ||
| return [] | ||
|
|
||
| events: list[dict] = [] | ||
| for block in chunk.split("\n\n"): | ||
| for line in block.splitlines(): | ||
| stripped = line.strip() | ||
| if not stripped.startswith("data:"): | ||
| continue | ||
| payload = stripped[len("data:") :].strip() | ||
| if not payload or payload == "[DONE]": | ||
| continue | ||
| try: | ||
| parsed = json.loads(payload) | ||
| except json.JSONDecodeError: | ||
| continue | ||
| if isinstance(parsed, dict): | ||
| events.append(parsed) | ||
| return events | ||
|
|
||
|
|
||
| def _usage_from_anthropic_stream_chunks(original_response: list[Any]) -> Optional[AnthropicUsage]: | ||
| input_tokens = 0 | ||
| output_tokens = 0 | ||
| found_usage = False | ||
|
|
There was a problem hiding this comment.
_anthropic_stream_chunk_events here and AnthropicMessagesHandler._iter_sse_events in litellm/llms/anthropic/chat/guardrail_translation/handler.py implement the same SSE-bytes-to-event-dicts conversion with slightly different splitting/stripping logic. There is now a third reading of the same bytes in _block_continuation_chunks. Having multiple independent parsers for the same wire format increases the chance of subtle behavioural differences (e.g., one handles \r\n line endings, another does not). Centralising to a single helper and delegating from the other call sites would reduce this risk.
…andling Resolve conflicts in the MCP streaming iterator and its test after internal_staging removed the standalone follow_up_response phase (follow-ups now loop back through continue_initial_response with a round cap). Re-apply the failure-surfacing semantics on top of that flow: stash internal failures and emit a single terminal error event (or skip the doomed follow-up) instead of silently ending the stream, and set base_iterator to None on a stashed error so the new phase-4 loop stops. Union the two independently-added iterator test suites (round-cap loop plus error-surfacing) into one file, deduping shared helpers.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
7 participants
Note
Copy of #32579 by @thibault-linktree, pushed to a
litellm_-prefixed branch so the full CircleCI suite (which needs secrets and is skipped on fork PRs) runs. Commit authorship is preserved (commits pushed by SHA); full credit to the original author. Based onlitellm_internal_staging.Original PR: #32579
Relevant issues
Fixes #32561, fixes #32562, fixes #32563
Combines #32565, #32566 and #32567 into a single PR per review logistics (the three fixes share the
_stream_errorstash /_make_stream_error_eventhelper and their tests interact). All Greptile feedback from those PRs is already incorporated; see the closed PRs for the per-fix review threads.Pre-Submission checklist
Screenshots / Proof of Fix
Live-repro evidence below is from the original author (@thibault-linktree). Real proxy (
litellm --config, real OpenAI calls, deepwiki MCP server registered). Commit hashes are from the original per-fix branches; the combined branch is the same changes.1. Initial LLM call fails (bogus
previous_response_id), streamingBefore (main @
999637883c) — HTTP 200 + discovery events with noresponse.created(crashes openai-node:expected 'response.created' event, got response.mcp_list_tools.in_progress):After (@
b6bbee9c92) — real 400 with the provider error body, before any SSE bytes:2. Tool execution / follow-up failure mid-stream (requires fault injection; captures drive the real iterator with only the failing dependency mocked)
Before — tool events, then the stream ends with no terminal event; a batch tool-execution failure additionally attempted the doomed follow-up (
follow-up model call attempted: True), rejected by the provider with"No tool output found for function call ...".After (@
bd9e13914e) — terminalerrorevent; doomed follow-up skipped:3. MCP tools requested, zero resolved (key lacks server access / unknown server)
Before (main @
999637883c) — HTTP 200, model called withtools=None, fabricated answer ("It seems I can't access external tools or links, including Linktree. You can check the number of links on your Linktree by logging into your account ..."); only trace is a "success"list_mcp_toolsspend log with[].After (@
60dc62fb84) — 400 with an actionable message:Happy path, fully e2e (real deepwiki
read_wiki_structurecall): unchanged on every fix — full two-phase flow, real answer ("The first topic name is "Overview.""), zeroerrorevents.Type
🐛 Bug Fix
Changes
Three swallow-points in the MCP gateway made failures invisible to clients (HTTP 200 + broken/silent streams, or silent no-tools model calls):
aresponses_api_with_mcp+MCPEnhancedStreamingIterator): the initial call is now made eagerly, before any SSE bytes are written, and a stashed creation failure is re-raised so the proxy returns a real 4xx/5xx. If a failure still surfaces during iteration, the stream emits a single terminalerrorevent instead ofmcp_list_toolsdiscovery events with noresponse.created.errorevent instead of silently ending the stream; after a batch tool-execution failure the doomed follow-up call (function_calls with no outputs → provider 400) is skipped entirely.litellm_proxyMCP tools but resolved none (and carry no other tools to fall back on) now raise a 400 naming the requested server URLs and likely causes. Mixed requests with function tools fall back to those tools (previous behaviour). Opt-out:litellm.reject_empty_mcp_resolved_tools = False(default True per maintainer guidance).Tests: 9 new tests across
tests/test_litellm/responses/mcp/test_mcp_streaming_iterator.py(new file) andtests/test_litellm/responses/mcp/test_mcp_empty_resolved_tools.py(new file); the auth-header pass-through test intests/mcp_tests/test_aresponses_api_with_mcp.pynow resolves a dummy tool (its purpose is header propagation, not zero-tool behaviour). Full local run: 50 passed.