fix(proxy): use e.request_data for logging_obj in ModifyResponseException streaming passthrough - #30701
Conversation
Congrats! CodSpeed is installed 🎉
You will start to see performance impacts in the reports once the benchmarks are run from your default branch.
|
Greptile SummaryThis PR fixes a
Confidence Score: 5/5Safe to merge — the change is two surgical one-line substitutions in existing exception handlers, each using a variable that is already bound and validated in context, with no impact on the non-streaming or non-blocked-request paths. Both changed lines read No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/proxy/proxy_server.py | One-line fix in each of the ModifyResponseException and RejectedRequestError streaming handlers: reads litellm_logging_obj from _data (the processor's data dict, which carries the object) instead of data (the outer request body, which never does). Both context variables are confirmed to be bound correctly before the change. |
| tests/test_litellm/proxy/test_modify_response_streaming_passthrough.py | New regression tests for both streaming passthrough handlers; uses a shared _run_streaming_block_and_get_wrapper helper, patches all network I/O, and asserts that CustomStreamWrapper receives the sentinel logging object from e.request_data rather than None. Both ModifyResponseException and RejectedRequestError paths are covered. |
Reviews (3): Last reviewed commit: "test(proxy): cover RejectedRequestError ..." | Re-trigger Greptile
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
6906a6b to
7004638
Compare
|
Thanks for the PR! Triggering Greptile for a code review: |
|
@seph-barker please resolve the conflicts |
…tion streaming passthrough
When a guardrail blocks a streaming request pre-call by raising
ModifyResponseException (or RejectedRequestError), chat_completion streams the
violation message back as a 200 by building a CustomStreamWrapper. It read the
logging object from the outer request body (`data.get("litellm_logging_obj")`),
but that dict never carries litellm_logging_obj -- it diverges from the
processor's data at function_setup, and only the processor copy (exposed as
e.request_data, already bound to `_data` here) gets the logging object
attached. CustomStreamWrapper.__init__ then dereferences
`logging_obj.model_call_details` on None and 500s the request with
"AttributeError: 'NoneType' object has no attribute 'model_call_details'".
Read logging_obj from `_data` (= e.request_data) in both streaming
passthrough handlers so the refusal streams correctly. The non-streaming and
the anthropic/responses passthrough paths were unaffected.
Adds a regression test asserting the wrapper receives the logging object from
e.request_data rather than None.
The streaming logging_obj fix was applied to both the ModifyResponseException and RejectedRequestError handlers, but only the former had a regression test. Extract a shared helper and add a parallel test for the RejectedRequestError streaming path so both handlers stay guarded against the None-logging_obj crash.
7004638 to
5838387
Compare
Done, thanks for looking |
|
Thanks for the fix, @seph-barker! The root cause explanation is thorough and the regression test covers both exception handlers. Triggering a fresh Greptile review on the latest commit since the current review covers an earlier SHA. |
|
Thanks for the contribution! This has been merged (we have to make a copy of your branch to run e2e tests) |
Relevant issues
A guardrail that blocks a streaming request pre-call (by raising
ModifyResponseExceptionorRejectedRequestError) 500s the request instead of streaming the violation message back as a 200.Pre-Submission checklist
make test-unitType
🐛 Bug Fix
Changes
The bug
When a guardrail blocks a streaming request pre-call,
chat_completion'sexcept ModifyResponseException/except RejectedRequestErrorhandlers stream the violation message back as a 200 by building aCustomStreamWrapper:datahere is the outer request body returned by_read_request_body. It never carrieslitellm_logging_obj: the processor's data dict diverges from the outer body atfunction_setup(which returns a fresh kwargs dict), andlitellm_logging_objis attached only to that processor copy. Sodata.get("litellm_logging_obj")isNone, andCustomStreamWrapper.__init__immediately dereferences it:The non-streaming path is unaffected (it returns a
ModelResponsedirectly), and the anthropic / responses-API passthrough paths are unaffected (they stream via a plain SSE generator, notCustomStreamWrapper). Only streaming + pre-call block hits this.The fix
_data = e.request_datais already bound two lines above and is the processor's data dict, which does carrylitellm_logging_obj. Readlogging_objfrom_datain both streaming passthrough handlers (ModifyResponseExceptionandRejectedRequestError).Added a regression test (
tests/test_litellm/proxy/test_modify_response_streaming_passthrough.py) that driveschat_completion'sModifyResponseExceptionstreaming branch with an outer body lackinglitellm_logging_objand asserts theCustomStreamWrapperreceives the logging object frome.request_data(it would beNonewithout the fix).