Skip to content

fix(proxy): unwrap nested managed batch output file IDs on retrieve - #34083

Closed
Solaris-star wants to merge 7 commits into
BerriAI:litellm_internal_stagingfrom
Solaris-star:fix/33988-managed-batch-output-double-wrap
Closed

fix(proxy): unwrap nested managed batch output file IDs on retrieve#34083
Solaris-star wants to merge 7 commits into
BerriAI:litellm_internal_stagingfrom
Solaris-star:fix/33988-managed-batch-output-double-wrap

Conversation

@Solaris-star

@Solaris-star Solaris-star commented Jul 21, 2026

Copy link
Copy Markdown

Summary

  • Repeated managed batch retrieve could nest a unified output_file_id / error_file_id inside another unified ID.
  • model_mappings then pointed at a managed ID instead of the raw provider file/gs URI, so content download failed.
  • Add _unwrap_managed_output_file_id to peel nested managed output IDs, keep the outermost response ID, and always persist the raw provider ID.
  • Unit coverage for single-layer no-double-wrap and nested unwrap.

Fixes #33988

Test plan

  • pytest managed_files_hook double_wrap / unwrap_nested / skip_non_file / pass_credentials / fallback_when_no_router -> 5 passed
  • CI full suite
  • Manual: complete managed batch, retrieve twice, confirm model_mappings stay raw provider IDs and file content still downloads

yuneng-berri and others added 7 commits July 11, 2026 17:42
chore(ci): promote internal staging to main
chore(ci): promote internal staging to main
chore(ci): promote internal staging to main
chore(ci): promote internal staging to main
chore(ci): promote internal staging to main
chore(ci): promote internal staging to main
Repeated managed batch retrieve could nest a unified output/error file ID
inside another unified ID, so model_mappings pointed at a managed ID
instead of the raw provider file-* / gs:// value and content download
broke.

Add _unwrap_managed_output_file_id to peel nested managed output IDs,
keep the outermost response ID, and always persist the raw provider ID.
Cover single-layer and nested cases with unit tests.

Fixes #33988
@CLAassistant

CLAassistant commented Jul 21, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@greptile-apps

greptile-apps Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR makes repeated managed batch retrieval preserve raw provider file IDs. The main changes are:

  • Adds recursive unwrapping for nested managed output and error file IDs
  • Keeps the outer unified ID in batch responses
  • Stores the raw provider ID in model mappings
  • Adds focused tests for nested and already-unified IDs

Confidence Score: 4/5

The production ID handling looks correct, but the process-wide test stub needs to be scoped before merging.

  • Nested IDs are unwrapped before storing model mappings.
  • The response keeps its outer unified ID as intended.
  • The module-level sys.modules mutation can break later tests based on worker order.

tests/test_litellm/enterprise/proxy/test_managed_files_hook.py

Important Files Changed

Filename Overview
enterprise/litellm_enterprise/proxy/hooks/managed_files.py Adds cycle-aware nested managed-ID unwrapping and persists the resulting raw provider ID.
tests/test_litellm/enterprise/proxy/test_managed_files_hook.py Adds meaningful retrieval tests but introduces a process-wide proxy-server stub that can leak into other tests.

Reviews (1): Last reviewed commit: "fix(proxy): unwrap nested managed batch ..." | Re-trigger Greptile

Comment on lines +16 to +19
if "litellm.proxy.proxy_server" not in sys.modules:
_proxy_server_stub = types.ModuleType("litellm.proxy.proxy_server")
_proxy_server_stub.llm_router = None
sys.modules["litellm.proxy.proxy_server"] = _proxy_server_stub

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.

P1 Stub Leaks Across Test Modules

This collection-time mutation permanently replaces litellm.proxy.proxy_server in the worker's import cache. Under xdist, later tests in the same worker can receive this stub, which only defines llm_router, and fail when managed-file paths import other real attributes such as scheduler, prisma_client, or user_api_key_cache.

Rule Used: What: Flag any modifications to existing tests and... (source)

@codecov

codecov Bot commented Jul 21, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@codspeed-hq

codspeed-hq Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Merging this PR will not alter performance

✅ 31 untouched benchmarks


Comparing Solaris-star:fix/33988-managed-batch-output-double-wrap (38644aa) with main (5d4c4d0)

Open in CodSpeed

@Solaris-star
Solaris-star changed the base branch from main to litellm_oss_daily_2026_07_20 July 21, 2026 07:42
@Solaris-star
Solaris-star changed the base branch from litellm_oss_daily_2026_07_20 to litellm_internal_staging July 24, 2026 08:17
@Solaris-star Solaris-star closed this by deleting the head repository Aug 12, 2026
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.

[Bug]: Managed batch output files can double-wrap on repeated retrieve

4 participants