test(passthrough): regression coverage for Bedrock Content-Length override - #29549
test(passthrough): regression coverage for Bedrock Content-Length override#29549mateo-berri wants to merge 4 commits into
Conversation
The non-streaming passthrough finalizer merges dict(fastapi_response.headers) into the outgoing response, and an empty FastAPI Response carries a default content-length: 0. Before the get_response_headers sanitization fix, that leaked through and overrode the real upstream Content-Length, so uvicorn/h11 aborted any non-empty Bedrock Invoke body and the client saw a 200 with an empty/truncated payload. Adds a unit test pinning the custom_headers exclusion path and an integration test that drives a non-empty passthrough body through a real ASGI server, which is the only level at which the Content-Length mismatch surfaces. Both fail against the pre-fix behavior and pass with the sanitization in place.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…sion The transport test reconstructed the header assembly inline, so it pinned get_response_headers over a socket but never exercised the finalizer that actually leaked content-length: 0 (base_passthrough_process_llm_request passing dict(fastapi_response.headers)). Drive that finalizer directly via a subclass that overrides the upstream call, with a real placeholder Response as the leak source, and serve its output over a real uvicorn/h11 server so a regression in the finalizer itself is caught, not just in the helper.
Greptile SummaryThis is a test-only PR adding regression coverage for the Bedrock passthrough
Confidence Score: 5/5Safe to merge — test-only change, no production code altered. The two new tests are narrowly scoped to the bug they document, use an in-process ASGI transport (no real sockets, satisfying the mock-only folder rule), correctly exercise the exact code path that contained the bug, and would have caught the original regression. No production code is touched. No files require special attention.
|
| Filename | Overview |
|---|---|
| tests/test_litellm/proxy/pass_through_endpoints/test_pass_through_endpoints.py | Adds two regression tests: a unit test for get_response_headers custom-header sanitization and an in-process ASGI integration test that verifies Content-Length is never leaked from the FastAPI placeholder response into a non-empty Bedrock body. |
Reviews (3): Last reviewed commit: "Merge remote-tracking branch 'origin/lit..." | Re-trigger Greptile
…cess tests/test_litellm is documented as mock-only (tests/test_litellm/readme.md), but the Bedrock passthrough regression test bound a real loopback socket and ran a live uvicorn server in a background thread. Drive the same finalizer output through an in-process ASGI server (httpx.ASGITransport) instead, asserting the recomputed Content-Length the client receives against the served body. The test still fails against the pre-#29120 leak, where the placeholder Response's content-length: 0 overrides the real upstream length, and passes once get_response_headers sanitizes custom headers, without opening a real socket.
|
Addressed the folder-rule concern from the last review. Generated by Claude Code |
…itellm_bedrock_passthrough_content_length_regression
|
Merged the latest Generated by Claude Code |
|
🚅 Hi, thanks for the PR! I'm Agent Shin, the automated triage bot for this repository. What's this and why am I getting it? I read the description against our contribution rubric. Here's how it lined up: What you got right:
What's still missing:
If the description isn't updated in the next 24 hours, I'll auto-close this PR. That's not us saying we don't care about the change; we want the open-PR list to mirror what a maintainer can act on right now, so contributors don't get lost in a backlog. A closed PR is a soft "park this for later," not a rejection. Take your time; everything below still works after the close. During the grace period: just update the PR description with the missing pieces. No need to ping me; I'll re-check on the next sweep and skip the auto-close if it now passes. See what counts as QA proof for the full rubric (a linked issue alone isn't enough; it covers context, not proof). If the PR does get auto-closed in 24 hours, you still have easy recovery paths:
Internal BerriAI contributors: this rubric doesn't apply to you; ping a maintainer. (I'm an LLM, so I'm not infallible. If you think I got this wrong, ping a maintainer; they'll override me.) |
Relevant issues
Follow-up to the Bedrock passthrough
RuntimeError: Response content longer than Content-Lengthregression fixed in #29120 (introduced by #27412). The fix shipped without a test that exercises the response serialization layer, so this adds that coverage. Reported via an internal bug report; affected v1.85.0 through v1.88.0-dev.1, fixed in v1.88.0-rc.1Linear ticket
N/A
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
make test-unit@greptileaiand received a Confidence Score of at least 4/5 before requesting a maintainer reviewBackground
The non-streaming passthrough finalizer
base_passthrough_process_llm_requestincommon_request_processing.pymergesdict(fastapi_response.headers)into the outgoing response so LiteLLM metadata headers likex-litellm-call-idsurvive. At that pointfastapi_responseis an empty placeholderResponse(), and an empty FastAPI Response carries a defaultcontent-length: 0. Before #29120,get_response_headerssanitized the upstream headers against an exclusion list (which dropscontent-length) but did not runcustom_headersthrough that same list, socontent-length: 0leaked through and overrode the real upstream Content-Length. For empty bodies this was harmless; for any non-empty Bedrock Invoke body uvicorn/h11 saw a body longer than the declared length and aborted, leaving the client with a 200 and an empty/truncated payloadThe unit test added in #27412 asserted on the response object in-process and never re-derived the Content-Length a client receives, so the Content-Length/body mismatch was invisible to it. That gap is what this PR closes
CI (LiteLLM team)
Link:
Link:
Links:
Screenshots / Proof of Fix
This is a test-only change, so a live-proxy curl can't demonstrate it (the fix is already on staging and there is no behavior change to curl). The meaningful proof is that the new tests fail against the pre-#29120 behavior and pass with the sanitization in place
With the sanitization in place:
After temporarily reverting #29120 (
return_headers.update(custom_headers), no exclusion filter) and re-running, both tests fail on the leakedcontent-length: 0:The integration test serializes the finalizer's output through an in-process ASGI server (
httpx.ASGITransport), so the Content-Length the client receives is the leaked0while the served body is 91 bytes. That declared-length/body mismatch is exactly what aborted non-empty Bedrock bodies in production once uvicorn/h11 enforced the declared length over a real socketType
✅ Test
Changes
Adds two tests to
tests/test_litellm/proxy/pass_through_endpoints/test_pass_through_endpoints.pytest_get_response_headers_sanitizes_custom_headerspins thecustom_headersexclusion branch that #29120 added; the existingget_response_headerstest passes no custom headers, so that branch had no coveragetest_bedrock_passthrough_nonempty_body_survives_http_serializationdrives the actualbase_passthrough_process_llm_requestfinalizer rather than reconstructing the header assembly inline. A subclass overrides the upstream call (no monkeypatching) to return a stubbed non-empty Bedrock-style body, and a real placeholderResponse()is passed in as the leak source, so the test exercises the exactget_response_headers(headers=result.headers, custom_headers=dict(fastapi_response.headers))call the bug lived in. The finalizer's output is then serialized through an in-process ASGI server (httpx.ASGITransport) and the served body, recomputed Content-Length, upstreamx-amzn-requestid, and injectedx-litellm-call-idare asserted to arrive intact.tests/test_litellm/is documented as mock-only (tests/test_litellm/readme.md), so the test stays in-process instead of binding a real loopback socket; the leakedcontent-length: 0still surfaces as the value the client receives, which is the mismatch the object-level assertion in #27412 never checked