fix(bedrock): forward provider response headers on chat completions - #37003
Conversation
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
🤖 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 forwards upstream provider response headers through Bedrock Converse, Bedrock Invoke, and generic HTTP streaming completion paths.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| litellm/llms/bedrock/chat/converse_handler.py | Propagates Converse response headers through synchronous, asynchronous, streaming, and non-streaming responses. |
| litellm/llms/bedrock/chat/invoke_handler.py | Returns provider headers alongside decoded Invoke completion streams. |
| litellm/llms/bedrock/chat/invoke_transformations/base_invoke_transformation.py | Eagerly constructs Invoke streams and supplies their upstream headers to CustomStreamWrapper. |
| litellm/llms/custom_httpx/llm_http_handler.py | Passes generic HTTP provider response headers into streaming wrappers. |
| litellm/types/utils.py | Adds a shared model-response helper for processing and storing provider response headers. |
Reviews (3): Last reviewed commit: "chore: merge litellm_internal_staging in..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Add sync and async regression tests for the BaseLLMHTTPHandler streaming path, which forwards provider response headers for the ~30 providers that ride the generic handler and had no coverage. Also drop redundant setup prose from the moonshot invoke test docstring.
|
bugbot run |
…itellm_forward_bedrock_response_headers
…sponse_headers Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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 722c650. Configure here.
|
bugbot run |
|
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 58c4fa6. Configure here.
2 participants
TLDR
Problem this solves:
x-amzn-requestidon/chat/completionsHow it solves it:
User Flow
Before: a developer debugging a slow Bedrock deployment can't get the AWS request id back out of the gateway, so AWS support has nothing to look up
return_response_headers: trueand points a model atbedrock/converse/us.anthropic.claude-haiku-4-5-20251001-v1:0llm_provider-*headers at all, so there is no AWS request id to hand to support"stream": true, and same thing again for abedrock/invoke/...model with"stream": trueAfter: the same requests come back with the AWS request id in the response headers
return_response_headers: trueand points a model atbedrock/converse/us.anthropic.claude-haiku-4-5-20251001-v1:0llm_provider-x-amzn-requestid: 836b894b-b89a-40e2-8522-532c4e71f882, which they can quote verbatim to AWS support"stream": true, and forbedrock/invoke/...in both streaming and non-streaming, which also exposes thex-amzn-bedrock-*token and latency headersRelevant issues
Linear ticket
Resolves LIT-5145
Pre-Submission checklist
@greptileaito re-request a review after pushing changes)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
Live proxy against real AWS bedrock-runtime
us-east-1, real Anthropic, and real OpenAI. Real spend, no mocks, no stubs, no recorded fixtures.git status --porcelain --untracked-files=nowas empty in both worktrees, so no source was touched on either side. Same config, same script, same order, same proxy boot environment on both legs.a66a10b1b5db1627cf8eef5a789d626136f3adbe, the merge base of this branch andlitellm_internal_staging58c4fa6ae9839736c458127b982c82c82a85f04f, the current tipThe merge base is the right BEFORE because
git diff a66a10b1b5 58c4fa6ae9 -- litellm/is exactly and only this PR's six source files. The two legs therefore differ by this PR alone, with none of the staging churn from the merge commit confounding the comparison.Config used by both legs:
Command, run identically against each proxy:
1. Headline symptom:
llm_provider-x-amzn-requestidon/v1/chat/completionsControl at the BEFORE commit, so every
(NONE)above is a real absence rather than a broken harness:2. Full grid,
llm_provider-*header count per cellEvery cell returned HTTP 200 on both legs. 26 cells per leg.
Six cells gained headers. No cell went the other way: nothing that returned headers before returns fewer after.
3. The generic handler path (~30 providers share it)
generic-handler-openaistreaming went from zero headers to 24, matching what its own non-streaming path already returned:date, content-type, transfer-encoding, connection, cf-ray, cf-cache-status, server, strict-transport-security, x-content-type-options, access-control-expose-headers, openai-organization, openai-processing-ms, openai-project, openai-version, x-openai-proxy-wasm, x-ratelimit-limit-requests, x-ratelimit-limit-tokens, x-ratelimit-remaining-requests, x-ratelimit-remaining-tokens, x-ratelimit-reset-requests, x-ratelimit-reset-tokens, x-request-id, set-cookie, alt-svc.4. No clobber of
x-litellm-*set_provider_response_headersassigns_hidden_params["additional_headers"]rather than merging into it, so the full response header set was dumped on every model in both stream modes and compared key for key against the BEFORE leg.Every
x-litellm-*header survives on every cell:call-id,model-id,model-group,model-name,model-api-base,version,key-spend,attempted-retries,attempted-fallbacks,response-duration-ms,overhead-duration-ms,callback-duration-ms, and the sevenresponse-cost-*members.x-litellm-model-groupis written byrouter.pyinto that same dict and is present on both converse cells, which is direct live evidence for the writer that shares the dict most directly.This holds by construction as well:
set_provider_response_headersruns in the innermost handler on a freshly builtModelResponse, and every other writer of that dict runs strictly later and merges viasetdefault(...).update(...). Two writers were not reachable on this rig and are noted under Caveats.Two absences that are baselines rather than regressions, identical on both legs: bare
x-litellm-response-costis absent on every streaming cell (only the-original/-input/-outputbreakdown is emitted), andx-litellm-timeoutis absent except on error responses.5. Sensitive-header audit, run at both commits
Non-streaming already returned this set before the PR. Streaming was clean only because it forwarded nothing at all, so the PR widens an existing forwarding behavior to streaming rather than creating one, and
authorizationis absent on both legs. Called out under Caveats.6. Abandoned streams, the eager-construction risk
Invoke streaming now issues the upstream request eagerly instead of deferring it to a
make_callpartial, so the case to check is a client that opens a stream and walks away. Eight streaming requests tobedrock-invoke-haikuwere hung up at 0.35s (curl exit 28 on each), at both commits, twice per leg.The proxy stayed healthy every time:
/health/liveliness200,/health/readiness200, the next non-streaming request 200 with all 10 provider headers, and the next streaming request 200 with 5 chunks and its ownllm_provider-x-amzn-requestid. No hang, no degradation, no file-descriptor symptom.7. Error parity
bedrock-invoke-badmodelreturns HTTP 400 in all four combinations, with zerollm_provider-*headers. Non-streaming is byte-identical across the two legs. Streaming is not, and this is the one user-visible change the PR makes beyond headers:Three differences on the invoke streaming error path: the message gains the
litellm.BadRequestError: BedrockException -prefix,typeandparamgo from the JSON string"None"to real JSONnull, andx-litellm-timeoutnow appears where it was absent. Eager construction makes the error raise from the same place the non-streaming path raises it, so it picks up the normal exception mapping. Streaming errors now match non-streaming errors, which is the better shape, but it is a response-body change and is called out under Caveats. The status stays 400 in all four cases, so nothing turned into a 500 or a hang.8. Unit tests and mutation check
At the tip, the four test files this PR touches:
Mutation check, reverting only the six source files to the merge base and rerunning the identical command:
Nine tests die without the fix, one for each changed path: converse sync and async in both stream modes, invoke sync and async streaming, generic-handler sync and async streaming, and moonshot invoke streaming. These are real regression tests, not tests that pass either way.
make checkpasses at the tip.9. Gaps that are not regressions
Identical on both legs, so this PR does not reach them and does not cause them:
/v1/responsesstreaming returns zero provider headers for every provider/v1/messagesreturns zero provider headers in both modesbedrock/invokeNova returns zero provider headers when not streamingType
🐛 Bug Fix
Caveats (if any)
base_invoke_transformation.pyused to handCustomStreamWrapperamake_callpartial; it now issues the upstream request first so the headers exist before the first chunk. A caller that builds the stream and never iterates it holds that connection until something closes it, andCustomStreamWrapper.__del__does not closecompletion_stream(there is no syncclose(), onlyaclose()). The proxy is unaffected because it callsaclose()on the generator incommon_request_processing.py, so the exposure is direct sync SDK callers only. Converse and the generic HTTP handler already worked this way before the PR.bedrock/invokestreaming the 400 body gains thelitellm.BadRequestError: BedrockException -message prefix,error.typeanderror.paramgo from the JSON string"None"to real JSONnull, and anx-litellm-timeoutresponse header appears. The status code is unchanged and the new shape matches what non-streaming has always returned, so this is an alignment rather than a break, but a client matching onerror.type == "None"or on the unprefixed message will stop matching with no error._get_llm_provider_headersre-emits every upstream header verbatim asllm_provider-*, so the streaming path now returns the sameset-cookie(__cf_bm),openai-organization,openai-project, andcf-rayvalues that the non-streaming path already returned before this PR.authorizationis not among them. This is a pre-existing leak that the PR widens from non-streaming to streaming; narrowing it needs its own change toget_headers.py.return_response_headers. That flag only switches the OpenAI and Azure SDK paths towith_raw_response; the Bedrock and generic httpx handlers forward unconditionally, and the proxy splatsadditional_headerswith no gate. So every deployment on these providers starts seeing the new headers on upgrade.set_provider_response_headersassigns_hidden_params["additional_headers"]instead of merging into it. That is safe here because it runs in the innermost handler on a freshly builtModelResponse, before any other writer of that dict exists, and every later writer merges withsetdefault(...).update(...). The live run confirms it forrouter.py(x-litellm-model-groupsurvives on both converse cells). Two writers could not be exercised on the QA rig,parallel_request_limiter.py:837anddynamic_rate_limiter_v3.py:667, because both need key-level rpm/tpm limits, virtual keys, and a database; for that pair the claim rests on the ordering argument rather than on evidence. Anyone adding a writer that runs before the LLM handler returns would need to merge rather than assign./v1/responsesstreaming returns no provider headers for any provider, converse on/v1/messagesreturns none in either mode, andbedrock/invokeNova returns none when not streaming.Final Attestation
Link to Devin session: https://app.devin.ai/sessions/698b5dfc69ee439e810c80a77a5e35d7
Note
Medium Risk
Touches Bedrock converse/invoke and the shared HTTP streaming path, and makes invoke streaming eager. Header plumbing is not auth-critical but can affect stream lifecycle and what is exposed to clients.
Overview
Surfaces upstream provider response headers (e.g. Bedrock
x-amzn-requestid) on/chat/completionsso they can be returned asllm_provider-*headers for debugging and AWS correlation.Converse now copies
response.headersonto non-streamingModelResponseviaset_provider_response_headers, and streaming wrappers receive those headers frommake_sync_call/make_call.Invoke streaming no longer defers the HTTP call through a
make_callpartial. The stream is opened immediately so headers exist whenCustomStreamWrapperis built. The generic HTTP handler streaming path also passes_response_headersthrough.Caveat: invoke streaming is now eager rather than lazy.
Reviewed by Cursor Bugbot for commit 58c4fa6. Bugbot is set up for automated code reviews on this repo. Configure here.