fix(bedrock): resolve the managed-batch output bucket on every path that reads it - #37047
Conversation
…uted and cost-poller paths get_configured_s3_bucket_name accepts the output bucket only from the immutable _litellm_internal_model_credentials snapshot or AWS_S3_BUCKET_NAME. That refusal to read litellm_params is deliberate: the bucket is what validate_managed_cloud_file_id checks a file id against, so trusting a request-supplied value would let a caller redirect reads to a bucket of their choosing Two live entry points reach the Bedrock file-content transformation without ever building that snapshot. The managed-files pre-call hook sets data["model"] for any id carrying llm_output_file_id, which is every batch output, so get_file_content always takes the model-routed branch; that branch called llm_router.afile_content directly, and managed_files_obj.afile_content, the only caller that built the snapshot, is therefore unreachable for batch output. CheckBatchCost spread the deployment credentials as plain kwargs, and get_litellm_params does not carry s3_bucket_name across (gcs_bucket_name is listed for exactly this reason, its S3 counterpart is not), so the poller lost the bucket the same way The result was that every completed Bedrock managed batch failed files.content with "S3 bucket_name is required" and never had its cost tracked, leaving the row to be re-polled every cycle. Both paths now resolve the deployment credentials and pass the same MappingProxyType snapshot the managed-files hook already builds
The mock merged every call into one shared dict, so a second routed retrieval would overwrite the first and the assertions would still pass. Keep one frozen snapshot per call and assert exactly one call, which also makes an unintended second retrieval a failure rather than something the merge hides
…ccounting path too A third path reads a completed batch's output file, and it could not resolve the bucket either. When cost is accounted from the retrieve itself rather than from the poller, the batch success handler calls _handle_completed_batch, which fetches the output file through _extract_file_access_credentials. That helper forwarded a whitelist covering Azure and Vertex, gcs_bucket_name included, but nothing for Bedrock, and retrieve_batch built its litellm_params through get_litellm_params, whose fixed signature drops the trusted credential snapshot. So the snapshot never reached the file read and it failed with "S3 bucket_name is required" for a bucket the deployment had configured, leaving the batch's cost unrecorded. Adding s3_bucket_name to that whitelist would not have worked. The Bedrock file config deliberately resolves the bucket only from the immutable server-side snapshot or the environment, never from a request param, because the bucket is what managed file ids are validated against. The snapshot is therefore what has to flow, exactly as it already does for the model-routed and cost-poller paths. retrieve_batch now re-adds the snapshot after get_litellm_params, the same way the file operations already do, the whitelist forwards it, and the proxy attaches it for router-routed managed batches from the deployment behind the unified id. Verified against a live proxy reading a real completed Bedrock batch: the cost row appears within seconds of the retrieve carrying the batch's real spend and usage, where before the read raised and no row was written. Resolving those credentials is best effort. A batch whose deployment no longer resolves, which happens when a model group is removed while batches are in flight, still serves its status instead of failing the request on the lookup. This matters for the OSS and polling-disabled configurations, where the retrieve path is the only thing that accounts for a batch at all.
…all paths The helper that carries the credential snapshot into litellm_params lived private in files/main.py, and the batch retrieve needed it too. It now sits beside get_litellm_params, which is what it augments, so neither caller reaches into the other's private surface. Typed as Mapping/MutableMapping of object rather than Any, which the strict import rules ban. The file-content route builds the snapshot through the same helper as the batch route instead of assembling a conditional mapping inline, which drops two mutable constructions and leaves one way to attach it. Its name loses the batch suffix now that both routes use it.
Greptile SummaryThe PR propagates immutable deployment credential snapshots through Bedrock managed-batch retrieval and cost-accounting paths
Confidence Score: 5/5The PR appears safe to merge No blocking failure remains
|
| Filename | Overview |
|---|---|
| litellm/proxy/openai_files_endpoints/common_utils.py | Adds best-effort construction of an immutable deployment credential snapshot for routed operations |
| litellm/proxy/openai_files_endpoints/files_endpoints.py | Attaches deployment credentials before model-routed managed-file content retrieval |
| litellm/proxy/batches_endpoints/endpoints.py | Resolves the deployment snapshot before retrieving a unified batch through the router |
| litellm/batches/main.py | Preserves trusted credentials in retrieve-batch logging parameters for completed-batch accounting |
| litellm/batches/batch_utils.py | Forwards the trusted snapshot and deployment AWS credentials when reading batch output |
| enterprise/litellm_enterprise/proxy/common_utils/check_batch_cost.py | Supplies the immutable credential snapshot to the enterprise batch-cost poller's output read |
| litellm/litellm_core_utils/get_litellm_params.py | Centralizes validation and propagation of immutable server-side credential snapshots |
| litellm/files/main.py | Reuses the centralized trusted-credential propagation helper across file operations |
Reviews (3): Last reviewed commit: "Merge remote-tracking branch 'origin/lit..." | Re-trigger Greptile
|
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 2a75381. Configure here.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…s AWS credentials
|
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 1524880. Configure here.
…itellm_do_36634 # Conflicts: # litellm/batches/batch_utils.py
|
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 f930980. Configure here.
TLDR
Problem this solves:
How it solves it:
User Flow
Before: a developer whose batch job finished cannot download its results, and the error tells them to configure a bucket their admin already configured
purpose=batch) and gets a file id back"status": "completed"with anoutput_file_idS3 bucket_name is required. Set 's3_bucket_name' in proxy config or AWS_S3_BUCKET_NAME for Bedrock file content retrievalAfter: the same fetch returns the batch records
"status": "completed"Relevant issues
Supersedes #36634 and carries its commits unchanged, plus one import-order fix and one follow-on fix. That PR's fork is organization-owned, so maintainers cannot push the CI fix to it
The follow-on fix (1524880): once the retrieve path could resolve the bucket, its cost read still failed with
Missing Dependency: ... botocore[crt]because_extract_file_access_credentialsonly forwarded a fixed key list and dropped the deployment'saws_*credentials, so the S3 signer fell through to environment auth. The keys inAWS_CREDENTIAL_KWARGS_KEYSare now forwarded#37050 (which supersedes #36877) decides which component accounts for a batch's cost, and routes that work to the retrieve path whenever the cost poller cannot be relied on. This PR is what makes that path able to read the output file at all, so the two are complementary
Linear ticket
Resolves LIT-5599
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@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
Two live proxies from two worktrees, each with a fresh database, against real Bedrock (
bedrock/us.anthropic.claude-haiku-4-5-20251001-v1:0, cross-region inference profile in us-west-2) with a real 100-record batch job per leg. The deployment carriesaws_access_key_id/aws_secret_access_key/aws_session_token,s3_bucket_name,s3_region_name,s3_encryption_key_id, andaws_batch_role_arnin itslitellm_params. NoAWS_*variable is set in the proxy's environment, so the per-deployment values are the only sourceConfig used on both legs:
Same commands on both legs (
$PORTand$KEYdiffer per leg):Before, at the merge base 6704a10:
grep -c "S3 bucket_name is required" proxy_before.logreturns 8 across the endpoint hit's traceback and the logging worker's_handle_completed_batchfailure (LoggingWorker error: S3 bucket_name is required ...), so no batch usage row was ever written on that leg (the other 5 rows are retrieve and status rows carrying no usage)After, at f930980:
grep -c "S3 bucket_name is required" proxy_after.logreturns 0. Thearetrieve_batchrow is the batch usage the retrieve path computed from the output file (100 records, 2200 input and 400 output tokens, matching Bedrock's own manifest), written by the logging worker without any environment credentialsThe intermediate commit 2a75381 (this branch before 1524880) served the same HTTP 200 with 100 records, but its logging worker failed on that read with
LoggingWorker error: ... _handle_completed_batch ... _auth_with_env_vars ... Missing Dependency: ... botocore[crt]and wrote no usage row, which is what 1524880 fixesType
🐛 Bug Fix
Caveats (if any)
On the last point:
_batch_cost_calculatorprices each record by the model name Bedrock writes intomodelOutput.model(claude-haiku-4-5-20251001), andbedrock/claude-haiku-4-5-20251001is not a cost-map key (the Bedrock keys are theanthropic./us.anthropic.ids), so the poller and the retrieve path both record tokens with spend 0.0 for this deployment. That is independent of reading the file and is left for a follow-upOn the authorization point: which callers may read a managed file id is decided before this code runs, by
validate_managed_id_requirementandlitellm.require_managed_files, and this change neither adds nor removes a check. What it does do is bring Bedrock to parity with the providers whose reads already worked, so the same model now applies to one more provider rather than to one fewer. The id itself stays bounded byvalidate_managed_cloud_file_id, which rejects any file id outside the configured bucket and its managed object prefixes. Making ownership validation unconditional is a worthwhile discussion and a breaking one, since it would reject raw provider ids on every file and batch route for every provider; it belongs in its own change rather than riding along with a bucket-resolution fixFinal Attestation
Note
Cursor Bugbot is generating a summary for commit 2a75381. Configure here.