Skip to content

fix(proxy/batches): make managed-file resolution additive, restore fall-back for missing-row and lookup errors - #34584

Merged
yucheng-berri merged 1 commit into
litellm_internal_stagingfrom
litellm_batches_managed_file_additive
Jul 25, 2026
Merged

fix(proxy/batches): make managed-file resolution additive, restore fall-back for missing-row and lookup errors#34584
yucheng-berri merged 1 commit into
litellm_internal_stagingfrom
litellm_batches_managed_file_additive

Conversation

@yucheng-berri

@yucheng-berri yucheng-berri commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Relevant issues

Follow-up to #34474

Linear ticket

Pre-Submission checklist

Please complete all items before asking a LiteLLM maintainer to review your PR

  • I have added meaningful tests
  • My PR passes all CI/CD checks (e.g., lint, format, unit tests)
  • My PR's scope is as isolated as possible; it only solves 1 specific problem
  • I have received a Greptile Confidence Score of at least 4/5 before requesting a maintainer review (Greptile reviews automatically once the PR is opened; only comment @greptileai to 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

#34474 resolved a managed (unified) batch input_file_id to its backend storage_url before dispatch so providers like Vertex AI receive a real gs:// path instead of the opaque token. Alongside that it added two fail-closed responses that were not in the original change and were not present before it: a 404 when the managed-file row is missing and a 503 when the lookup errors. Those two are backward-incompatible against the prior behavior, where the same requests fell through and dispatched the original id (the managed-files deployment hook maps it)

Verified live, base vs the merged #34474 behavior, real Vertex Gemini batchPredictionJobs, no mocks: the missing-row case returns 200 on the prior behavior when the deployment hook serves the mapping from cache, and 404 after #34474. This PR restores the prior response for that case

This change keeps the resolution and removes only the two added fail-closed responses, so the managed-file handling becomes strictly additive: when a row with a storage_url exists it is substituted, and every other path (no database, lookup error, missing row, row without a storage_url) falls back to dispatching the original id exactly as before #34474

Type

🧹 Refactoring

Changes

_resolve_managed_input_file_storage_url now returns None on a lookup error and on a missing row instead of raising 503 and 404, matching the no-database and no-storage_url paths. The result: resolution is applied when it can be, and no request that previously dispatched now fails. The two mapped tests assert the fall-back for the missing-row and lookup-error cases; the resolution, legacy-row, and load-balancing tests are unchanged

Behavior changes

QA runbook

  1. Upload a batch input file with target_model_names set (managed files, enterprise) against a vertex_ai model, create a batch with the owning key: the Vertex job still receives the resolved gs:// path
  2. Delete the LiteLLM_ManagedFileTable row and repeat: the request dispatches the original id (deployment-hook mapping) rather than returning 404, matching the behavior before fix(proxy/batches): resolve managed unified input_file_id to storage_url with ownership check before dispatch #34474

Open in Devin Review

Note

Medium Risk
Changes batch-create behavior for unified file ids when the DB lookup fails or the row is missing—requests proceed instead of failing fast, which restores compatibility but may send opaque ids to providers if the deployment hook does not map them.

Overview
Follow-up to managed-file storage_url resolution for batch create: substitution when a row has storage_url is unchanged, but 503 (DB lookup error) and 404 (missing row) are removed so those cases behave like before #34474.

_resolve_managed_input_file_storage_url now returns None on lookup exceptions and when no managed-file row exists, instead of raising. Batch create then dispatches the original unified input_file_id, relying on the managed-files deployment hook as before. Tests were updated to assert router dispatch with the raw id for DB errors and missing rows rather than expecting fail-closed HTTP errors.

Reviewed by Cursor Bugbot for commit cf71afd. Bugbot is set up for automated code reviews on this repo. Configure here.

…, fall back like base

Restore the original PR's behavior on every path that did not already
resolve: no database, a lookup error, a missing managed-file row, or a
row without a storage_url all fall back to dispatching the original id,
which the managed-files deployment hook still maps. This drops the 404
and 503 fail-closed responses I had added, which were the only behaviors
that diverged from litellm_internal_staging.

The change is now strictly additive: when a managed-file row with a
storage_url exists, the unified batch branch substitutes it so providers
like Vertex receive a real gs:// path instead of the opaque token; every
other path behaves exactly as before. Verified live that non-managed,
managed-owner, multi-model load-balanced, and missing-row requests are
byte-identical to base
@yucheng-berri

Copy link
Copy Markdown
Contributor Author

@greptileai

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

Open in Devin Review

@yucheng-berri yucheng-berri changed the title refactor(proxy/batches): make managed-file resolution additive, restore fall-back for missing-row and lookup errors fix(proxy/batches): make managed-file resolution additive, restore fall-back for missing-row and lookup errors Jul 25, 2026
@greptile-apps

greptile-apps Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

Restores best-effort managed-file resolution behavior.

  • Returns None for lookup errors and missing rows so batch creation retains the original input file ID.
  • Updates the corresponding tests to verify fallback dispatch instead of HTTP 404/503 responses.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains within the scope of the previous review thread.

Important Files Changed

Filename Overview
litellm/proxy/batches_endpoints/endpoints.py Changes managed-file lookup failures and missing rows from fail-closed HTTP responses to additive fallback behavior.
tests/test_litellm/proxy/batches_endpoints/test_endpoints.py Updates missing-row and lookup-error tests to assert dispatch with the original unified file ID.

Reviews (2): Last reviewed commit: "refactor(proxy/batches): make managed-fi..." | Re-trigger Greptile

Comment thread litellm/proxy/batches_endpoints/endpoints.py
@codecov

codecov Bot commented Jul 25, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@yucheng-berri

Copy link
Copy Markdown
Contributor Author

@greptileai

@yucheng-berri

Copy link
Copy Markdown
Contributor Author

bugbot run

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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 cf71afd. Configure here.

@yucheng-berri
yucheng-berri merged commit fc5ab31 into litellm_internal_staging Jul 25, 2026
85 of 86 checks passed
@yucheng-berri
yucheng-berri deleted the litellm_batches_managed_file_additive branch July 25, 2026 01:42
@codspeed-hq

codspeed-hq Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Merging this PR will not alter performance

✅ 31 untouched benchmarks


Comparing litellm_batches_managed_file_additive (cf71afd) with litellm_internal_staging (2d6b574)

Open in CodSpeed

Ericcwang23 pushed a commit to Ericcwang23/litellm that referenced this pull request Jul 27, 2026
…, fall back like base (BerriAI#34584)

Restore the original PR's behavior on every path that did not already
resolve: no database, a lookup error, a missing managed-file row, or a
row without a storage_url all fall back to dispatching the original id,
which the managed-files deployment hook still maps. This drops the 404
and 503 fail-closed responses I had added, which were the only behaviors
that diverged from litellm_internal_staging.

The change is now strictly additive: when a managed-file row with a
storage_url exists, the unified batch branch substitutes it so providers
like Vertex receive a real gs:// path instead of the opaque token; every
other path behaves exactly as before. Verified live that non-managed,
managed-owner, multi-model load-balanced, and missing-row requests are
byte-identical to base
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants