fix(proxy): capture logging_obj before post_call_failure_hook pops it in ModifyResponseException streaming path - #32665
Conversation
…ModifyResponseException The Bedrock-specific GuardrailInterventionNormalStringError predates the unified guardrails refactor and no proxy code path handles it, so a block with the flag set surfaced as an uncaught Exception -> HTTP 500 in pre_call mode and was silently discarded in during_call mode (model call proceeded in the parallel asyncio.gather; the block hook's data["mock_response"] mutation happened after route_request had already unpacked kwargs). Convert the block to ModifyResponseException at the raise site inside make_bedrock_api_request. That exception is the industry-standard proxy contract already caught in proxy_server, anthropic_endpoints, response_api _endpoints, and pass_through_endpoints; it turns into a 200 response with finish_reason=content_filter and the block message as content, which is exactly what the flag was documented to yield. Post-call blocks attach the LLM response to original_response so the synthetic reply reports the upstream call's real token usage instead of zero. Deletes the now-orphaned GuardrailInterventionNormalStringError class and the dead create_guardrail_blocked_response / mock_response plumbing in the Bedrock hooks; updates the existing tests that had locked in the buggy contract. Resolves LIT-4186
…ith_updated_bedrock_guardrail_response Follow-up to the disable_exception_on_block fix. That method used to receive either a BedrockGuardrailResponse or a plain string (the block message, when the flag was set). Now that a block always raises ModifyResponseException before this method runs, the string branch is unreachable; tighten the type to BedrockGuardrailResponse and delete the guard.
…tream instead of surfacing as SSE 500 Regression from the LIT-4186 refactor: pre-refactor, the streaming post_call iterator caught GuardrailInterventionNormalStringError locally and replaced the assembled response with a synthetic content-filter message, then re-emitted it as chunks via MockResponseIterator. After the refactor the exception was re-raised as ModifyResponseException, which async_streaming_data_generator serializes as a proxy 500 error frame because the SSE response headers are already flushed by the time the block fires. Non-streaming paths still let ModifyResponseException propagate to the endpoint handler (which converts it into a 200). Streaming can't do that, so keep the local synthesis: on the exception, rebind the assembled response to a ModelResponse whose single choice carries the block message as content and finish_reason=content_filter, and let the downstream MockResponseIterator emit it as chunks. Same shape a non-streaming block produces. Adds a mapped-file regression test that mutation-kills the raise behavior and locks in the synthetic-stream contract.
…ll block Non-streaming post_call blocks report the upstream LLM call's real token usage via ModifyResponseException.original_response, which the endpoint handler unwraps through _blocked_response_usage. Streaming post_call synthesizes its own ModelResponse locally (the exception can't escape the SSE generator), and previously left .usage unset, so the client saw accurate billing on non-streaming blocks and zero on streaming blocks -- silent revenue leak. Copy the assembled response's .usage onto the synthetic block response before yielding. Pre-refactor code had the same gap (create_guardrail_blocked_response never set usage); this is a net improvement, not a regression fix.
… in ModifyResponseException streaming path post_call_failure_hook removes litellm_logging_obj from request_data before iterating callbacks (it's not serialisable). The streaming branch of the ModifyResponseException handler read it from _data after that call, so it always received None and CustomStreamWrapper.__init__ crashed with AttributeError: NoneType has no attribute model_call_details. Capture it before the hook runs so the streaming path gets a valid object. Co-authored-by: Mateo Wang <mateo-berri@users.noreply.github.com>
…ging_obj capture Covers the bug where logging_obj was read from request_data after post_call_failure_hook had already popped it, causing CustomStreamWrapper to crash with AttributeError. Co-authored-by: Mateo Wang <mateo-berri@users.noreply.github.com>
Greptile SummaryThis PR fixes an
Confidence Score: 5/5Safe to merge — the change is a minimal, targeted capture of a dict value before it is removed, with no effect on any other code path. The two-line fix in proxy_server.py is mechanically correct and the root cause is well-documented. The broader consolidation in bedrock_guardrails.py is well-reasoned: the old GuardrailInterventionNormalStringError had no surviving callers outside this file, the streaming path correctly handles the exception in-place to avoid raising past already-flushed SSE headers, and usage preservation is explicitly tested. All new tests are mock-based, exercise the actual production handler, and would catch a revert of the fix. No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/proxy/proxy_server.py | Core fix: captures _logging_obj from _data before post_call_failure_hook pops it, then passes the captured reference to CustomStreamWrapper; minimal two-line change is correct |
| litellm/proxy/guardrails/guardrail_hooks/bedrock_guardrails.py | Replaces GuardrailInterventionNormalStringError with ModifyResponseException throughout; pre/during-call hooks now let the exception propagate; post-call non-streaming hook re-raises after attaching original response; streaming post-call hook catches and converts to synthetic SSE in-place (since headers are already flushed) |
| litellm/exceptions.py | Removes GuardrailInterventionNormalStringError; no remaining references in production code (only a historical comment in the new test file) |
| tests/test_litellm/proxy/guardrails/guardrail_hooks/test_bedrock_guardrails.py | Adds six new focused mock tests covering the full block lifecycle plus a direct chat_completion integration test that verifies the logging_obj capture fix; a revert of the proxy_server.py fix would cause the integration test to fail |
| tests/guardrails_tests/test_bedrock_guardrails.py | Updates existing tests to expect ModifyResponseException for disable_exception_on_block=True non-streaming and asserts block content/finish_reason for streaming; strengthens rather than weakens coverage |
| tests/enterprise/litellm_enterprise/proxy/guardrails/test_bedrock_apply_guardrail.py | Mechanical update: swaps GuardrailInterventionNormalStringError for ModifyResponseException in the regression test; no logic change |
Reviews (2): Last reviewed commit: "test(proxy): drive real chat_completion ..." | Re-trigger Greptile
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…reaming logging_obj regression The original test inlined the fix pattern (capture before pop) in its own body rather than calling the actual chat_completion handler in proxy_server.py, so a revert of the fix left the test passing. Confirmed via mutation check: reverting the two-line source fix and re-running left the test green. Rewrite the test to drive chat_completion directly: - patch _read_request_body so chat_completion sees the seeded dict - patch ProxyBaseLLMRequestProcessing.base_process_llm_request to raise ModifyResponseException with the same request_data - patch proxy_logging_obj so post_call_failure_hook mutates the dict the way production does (pops litellm_logging_obj) - intercept CustomStreamWrapper.__init__ and assert logging_obj is the non-None object seeded in request_data Mutation-verified: reverting the source fix now surfaces the exact production crash inside CustomStreamWrapper's __init__ (AttributeError: NoneType has no attribute model_call_details) rather than a silently-passing test. Addresses Greptile P1 on PR #32665.
|
Valid finding, fixed in 179734a. The original test inlined the fix pattern (capture before pop) rather than driving the real chat_completion handler; I confirmed via a mutation check that reverting the two-line source fix left the old test green. The rewrite calls chat_completion directly (with _read_request_body, base_process_llm_request, and proxy_logging_obj patched to reproduce the pop behavior); reverting the fix now surfaces the exact production error (AttributeError: NoneType has no attribute model_call_details inside CustomStreamWrapper.init). |
|
bugbot run |
…itellm_bedrock_logging_obj_capture # Conflicts: # tests/test_litellm/proxy/guardrails/guardrail_hooks/test_bedrock_guardrails.py
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 179734a. Configure here.
Relevant issues
Found during live smoke testing of PR #32289 (LIT-4186 Bedrock
disable_exception_on_blockfix)Linear ticket
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaiand received a Confidence Score of at least 4/5 before requesting a maintainer reviewDelays 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
Live proxy on
:4000against a real AWS Bedrock guardrail. The guardrail blocks admin-related prompts withblockedInputMessagingset toSorry, the model cannot answer this question.Before the fix: streaming pre_call block returns HTTP 500
Root cause:
post_call_failure_hook(litellm/proxy/utils.py) popslitellm_logging_objfromrequest_databefore invoking callbacks; the comment reads "Remove before callbacks iterate; not serialisable". The streaming branch of theModifyResponseExceptionhandler inchat_completionreadlogging_objfrom_dataafter that call, always receivingNone.CustomStreamWrapper.__init__then crashed withAttributeError: 'NoneType' object has no attribute 'model_call_details', which surfaced as HTTP 500.After the fix: streaming pre_call block returns HTTP 200 with valid SSE
All other cases confirmed working against live Bedrock:
pre_call block non-streaming returns HTTP 200 with
finish_reason=content_filterand zero usage. during_call block non-streaming and streaming return HTTP 200 with the block message and no leaked LLM response. post_call block non-streaming returns HTTP 200 with the block message and real upstream usage preserved. post_call block streaming returns HTTP 200 with usage preserved and no leaked tokens. Allowed prompts return the normal LLM response.disable_exception_on_block: falsestill returns HTTP 400 with the guardrail policy error. Python openai SDK parses both blocking shapes correctly.Type
🐛 Bug Fix
Changes
litellm/proxy/proxy_server.pycaptureslogging_objfrom_databefore callingpost_call_failure_hook(which pops it) and passes the captured value toCustomStreamWrapperrather than re-reading from the dict.tests/test_litellm/proxy/guardrails/guardrail_hooks/test_bedrock_guardrails.pyaddstest_chat_completion_modify_response_exception_streaming_logging_obj_not_noneto lock in the fix and prevent regression.Note
Medium Risk
Changes guardrail block semantics across pre/during/post-call and streaming paths (billing, whether the LLM runs, and HTTP status), though behavior is heavily covered by new regression tests.
Overview
Bedrock guardrails with
disable_exception_on_blocknow raiseModifyResponseException(and dropGuardrailInterventionNormalStringError) instead of returning a plain string or mutatingmock_response. Pre/during/post-call hooks propagate that exception so the proxy can return HTTP 200 with the block message, cancel parallel LLM work on during-call blocks, and attachoriginal_responsefor accurate usage on post-call blocks.Streaming post-call blocks replace the assembled stream with synthetic
content_filterchunks (and copy upstream usage) because the exception cannot escape after SSE headers are sent.In
chat_completion, theModifyResponseExceptionstreaming path captureslitellm_logging_objbeforepost_call_failure_hookremoves it, fixing HTTP 500 when buildingCustomStreamWrapper.Tests are updated/added for propagation, streaming block shape, usage preservation, and the streaming logging regression.
Reviewed by Cursor Bugbot for commit 179734a. Bugbot is set up for automated code reviews on this repo. Configure here.