Repository navigation
fix(responses): keep provider response headers in streaming logging callbacks - #38131
Conversation
🤖 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:
|
Greptile SummaryThe PR preserves provider response headers on the logging copy used for streaming Responses API callbacks without modifying the event returned to callers
Confidence Score: 5/5The PR appears safe to merge No blocking failure remains
|
| Filename | Overview |
|---|---|
| litellm/responses/streaming_iterator.py | Restores provider headers only onto successfully copied logging responses and leaves the caller event untouched when copying fails |
| tests/test_litellm/responses/test_streaming_iterator.py | Adds focused streaming callback tests covering restored headers, copy independence, transform precedence, and fallback behavior |
Reviews (3): Last reviewed commit: "fix: satisfy LIT002 mutable-collection g..." | Re-trigger Greptile
| copied: Final = self._copy_for_logging(completed) | ||
| inner_response: Final = getattr(copied, "response", None) | ||
| if isinstance(inner_response, ResponsesAPIResponse): | ||
| inner_response._hidden_params = dict(self._hidden_params) # mutable-ok: model_dump drops private attrs |
There was a problem hiding this comment.
| submit_times = recording_executor.submit_times_for(logging_obj) | ||
| assert len(submit_times) == 1 | ||
| assert submit_times[0] >= recorder.async_hook_finished | ||
|
|
There was a problem hiding this comment.
The new test declaration exceeds the repository's 120-character Python line limit, as does the httpx.Response construction on line 172, creating avoidable lint and formatting failures.
Context Used: CLAUDE.md (source)
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!
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
9572b60 to
6520b7d
Compare
|
@greptileai please review the current head 6520b7d, the branch was rewritten to a single commit bugbot run |
|
bugbot run |
…allbacks The responses streaming iterator captures the provider's HTTP response headers into its own _hidden_params, but never puts them on the completed response, and the model_validate(model_dump()) copy made for logging drops pydantic private attributes. Success callbacks and StandardLoggingPayload.hidden_params.additional_headers therefore saw an empty dict for streaming /v1/responses, so Azure's apim-request-id was unreadable from the callback payload. Restore the headers on the nested response of the logging copy, preferring any the provider transform already set (the fake_stream path) and falling back to the ones the iterator captured from the stream. Skipped when the copy fell back to the original event, so a serialization failure never leaves logging-only state on the caller's object.
6520b7d to
7a1f2db
Compare
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 6520b7d. Configure here.
|
@greptileai please review the current head d0eab54, it fixes the LIT002 lint gate failure from the staging rebase |
|
bugbot run |
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 d0eab54. Configure here.
TLDR
Problem this solves:
/v1/responseslogs carry no provider response headersHow it solves it:
User Flow
Before: someone streaming responses cannot find the provider's own request id for that call in their logging backend
"stream": true"stream": falseand that log does carry the provider headers, so only streaming is affectedAfter: the same streaming call logs the provider headers, so streaming and non-streaming logs agree
"stream": trueapim-request-idand the serving region"stream": falselog is unchangedRelevant issues
Linear ticket
Resolves LIT-6055
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
uv run pytest tests/test_litellm/<your_test_file>.py -v. Leave the suites (make test-unit-*,make test-unit) to CI: it finishes in ~15 minutes where a laptop takes an hour or more@greptileaito re-request a review after pushing changes)Screenshots / Proof of Fix
Shared setup for every case below. Same config, same commands, only the checked out commit changes.
config.yamlThe gpt-4.1-mini cases are real, paid OpenAI calls. The azure-stub case points at a local Azure-shaped upstream that answers
/openai/v1/responseswith Azure'sapim-request-idandx-ms-regionheaders, because no Azure OpenAI deployment was reachable with the credentials on hand. Every response is read back out of Datadog through its logs search API, so what you see is what a logging integration actually received.Before (31ca4dd)
OpenAI streaming, the regression
OpenAI non-streaming, the control
Azure-shaped upstream, streaming
apim-request-idAfter (d0eab54)
OpenAI streaming, the regression
OpenAI non-streaming, the control
Azure-shaped upstream, streaming
apim-request-idDatadog, side by side
Five streaming
/v1/responsescalls on each side, counted in Datadog by whether the log carries the provider headers. Before is 5 logged and none carrying them, after is 5 logged and 5 carrying themType
🐛 Bug Fix
Caveats (if any)
Medium
x-litellm-response-costLow
additional_headersis now{}where it used to benullFinal Attestation
Credit to the original implementation on this branch by Devin, which found the same root cause. This revision rewrites history so every commit is signed for the CLA, restricts the restore to the two header keys so a chained gateway's other hidden params cannot ride along, copies by value so nothing aliases the dicts the proxy uses for the client's own headers, and skips the restore entirely when the logging copy fell back to the caller's event
ran /live-pr-risk and found no regressions/backward incompatible risks