fix(managed-files): skip null file_object rows in get_user_created_file_ids - #35453
fix(managed-files): skip null file_object rows in get_user_created_file_ids#35453Diwak4r wants to merge 16 commits into
Conversation
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
_await_model_servable used poll_timeout (120s), the spend/log read-back budget. A stuck model reload therefore stalled every suite that creates a deployment for two minutes before failing Give create_model a fixed harness middle ground: model_servable_timeout=40s, polled every 2s, with each /v1/models call capped at 5s and clamped to the remaining deadline so one slow GET cannot overrun the wait. Happy path still returns on the first listing. Not derived from proxy general_settings or env Transport.get accepts an optional per-call timeout for that clamp. Unit tests cover the deadline arithmetic and clamp without a live proxy (cherry picked from commit c082a0e)
create_model returned after the first /v1/models hit that listed the model, so chat could still land on a cold gateway worker (numWorkers>1 / peer pod) and 400 Invalid model name. Require continuous listing for the product default add_deployment interval (30s) after first sight so every worker has synced from the DB; first listing still bounded at 40s (cherry picked from commit 7d1ee2f)
Keep the create_model DB-sync wait in the harness; the pure-function unit file is not needed for this PR (cherry picked from commit 8920465)
When less than one full poll interval remained in the first-listing budget, the pre-sleep check returned NotServable without another /v1/models call. Sleep only min(interval, time left) so a model that becomes listable in the last seconds of the timeout still gets a clamped final poll (cherry picked from commit 8439195)
A poll may start with remaining budget and still return after started+timeout if the transport overruns its clamp. Recheck the first-listing deadline after the response so a late listing does not open the continuous DB-sync phase (cherry picked from commit 7ff2bcb)
…l_servable_timeout test(e2e): bound the post-/model/new servable wait at 40s
* fix(mcp): resolve call_tool by registry without requiring tool map Multi-worker reloads put MCP servers in the registry from the DB but do not re-run tools/list on every process. Gating call_tool on tool_name_to_mcp_server_name_mapping made cold workers 500 with Tool not found after another worker had already listed the tool. Treat a registry match on server id/name/alias as enough; upstream rejects unknown tools * test(e2e): poll MCP register, tools/list, and tools/call across multi-worker lag Stage multi-worker gateways only load MCP servers and tool maps on the process that handled the request. Poll until the server is listed, the tool appears on tools/list, and tools/call is not a cold-worker 500 so key-access and Datadog MCP e2e stop racing the LB * Revert "fix(mcp): resolve call_tool by registry without requiring tool map" This reverts commit 8b56e51. * test(e2e): tighten MCP multi-worker lag classifier Only retry tools/call on gateway shapes Tool <name> not found and server_not_found, not any 500 that mentions tool/server not found, so upstream failures are not retried until the poll deadline * test(e2e): drop unit file for MCP lag classifier The live await_call_tool polls already cover multi-worker lag; a separate string-match unit module is not worth keeping (cherry picked from commit c274cf3)
…p_e2e_poll test(e2e): poll MCP tools across multi-worker lag (BerriAI#35047)
chore(ci): promote internal staging to main
chore: promote staging to main
GET /v1/files returns 500 when any managed file row in LiteLLM_ManagedFileTable has a null file_object column. This happens because the batch cost poller registers output/error file IDs without storing file_object, so the column is validly null for those rows. The old list comprehension called OpenAIFileObject(**file_object.file_object) which raised TypeError when file_object was None, aborting the entire listing and returning nothing to the caller. Fix: iterate the rows explicitly, skip null ones with a warning, and use model_validate with json.loads for str-typed rows (consistent with the batch listing path nearby). A malformed but non-null row is also skipped rather than crashing the response. Fixes BerriAI#35361
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
|
Greptile SummaryThis PR prevents managed-file listings from failing when database rows contain null or malformed
Confidence Score: 5/5The PR appears safe to merge, with the null-row failure handled and directly covered by an isolated regression test. Valid managed-file metadata continues to produce the same model objects, while null metadata is skipped before unpacking and malformed metadata is contained without failing the entire listing.
|
| Filename | Overview |
|---|---|
| enterprise/litellm_enterprise/proxy/hooks/managed_files.py | Safely skips null or unparsable managed-file metadata while preserving owner-scoped querying and valid file results. |
| tests/test_litellm/enterprise/proxy/test_managed_files_hook.py | Adds a focused, fully mocked regression test proving that null metadata no longer causes the valid result to be lost. |
Reviews (1): Last reviewed commit: "fix: skip null file_object rows in get_u..." | Re-trigger Greptile
There was a problem hiding this comment.
🟢 Ready to approve
The fix is small, targeted to the reported 500, and includes a focused regression test covering the null-row scenario.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR fixes a crash in the managed-files proxy path where GET /v1/files could 500 if any litellm_managedfiletable row has a NULL file_object (or otherwise unparsable content), by skipping those rows and returning the parseable file objects instead
Changes:
- Update
get_user_created_file_ids()to skipNULLfile_objectrows and guard JSON parsing/model validation withtry/except - Add a regression test ensuring a mixed result set (valid row + null row) returns only the valid file without raising
File summaries
| File | Description |
|---|---|
| enterprise/litellm_enterprise/proxy/hooks/managed_files.py | Makes /v1/files resilient to NULL/unparsable file_object rows by skipping and logging rather than raising |
| tests/test_litellm/enterprise/proxy/test_managed_files_hook.py | Adds regression coverage for skipping null file_object rows in get_user_created_file_ids() |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| } | ||
| ) | ||
| return [OpenAIFileObject(**file_object.file_object) for file_object in file_ids] | ||
| result: list[OpenAIFileObject] = [] |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Closing since #35365 merged the same null file_object skip in get_user_created_file_ids, so this is superseded. Thanks for the fix |
Problem
GET
/v1/filesreturns 500 when any managed-file row hasfile_object = NULL:Fixes #35361.
Root cause
get_user_created_file_ids()inmanaged_files.pyblindly unpacks every row with**file_object.file_object. The Prisma schema marksfile_object Json?(nullable) — the batch cost poller creates null rows normally when registering output/error file IDs.Fix
Replace the one-liner with a loop that skips null rows (logged as warning) and wraps JSON deserialization in try/except. Matches the existing pattern already used in
list_user_batchesin the same class.Test
Added
test_get_user_created_file_ids_skips_null_file_object: one valid row + one null row → only the valid file is returned, no exception raised.