fix(managed_files): skip managed file rows with no file_object when listing files - #35368
Conversation
…isting files Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
|
Greptile SummaryFixes managed-file listing so one null or malformed stored file object does not fail the entire request.
Confidence Score: 5/5The PR appears safe to merge, with the reported listing failure handled defensively and covered by a targeted regression test. The parser supports the repository’s required Pydantic version and both persisted file-object shapes, while isolating invalid rows instead of allowing one row to fail the complete file listing.
|
| Filename | Overview |
|---|---|
| enterprise/litellm_enterprise/proxy/hooks/managed_files.py | Adds per-row parsing and omission of unusable managed-file objects, consistently handling the persisted JSON-string representation without introducing an actionable defect. |
| tests/test_litellm/enterprise/proxy/test_managed_files_hook.py | Adds focused mock-based regression coverage confirming valid rows remain listed while null and malformed rows are skipped. |
Reviews (1): Last reviewed commit: "fix(managed_files): skip managed file ro..." | Re-trigger Greptile
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 |
TLDR
Problem this solves:
GET /v1/files500s on one nullfile_objectrowHow it solves it:
Relevant issues
Fixes #35361
Linear ticket
Pre-Submission checklist
@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
I could not stand up the DB-backed proxy needed for the end to end repro in this environment: managed files require Postgres, and both apt and the Docker registry are unreachable from here, so there is no way to get a database up. Reviewer runbook for a live repro is below; the regression test stands in as the automated proof (fails before the fix with the reported
OpenAIFileObject() argument after ** must be a mapping, not NoneType, passes after)Before the fix (
71b825a7f0plus the test only):After the fix (
88aea5306f):The four other failures in that file (
test_should_pass_credentials_to_afile_retrieve,test_should_fallback_when_no_router,test_should_not_double_wrap_already_unified_output_file_id,test_afile_content_bedrock_unified_id_end_to_end) fail identically on the unmodified base commit in this environment; they are missing optional deps here, not regressions from this changeLive repro for a reviewer with a database, on a proxy with
enable_preview_features: true,database_urlset andCheckBatchCostenabled:curl -X POST http://localhost:4000/v1/files -H "Authorization: Bearer sk-1234" -F purpose=batch -F 'target_model_names=my-gpt' -F file=@batch.jsonlcurl -X POST http://localhost:4000/v1/batches -H "Authorization: Bearer sk-1234" -H 'Content-Type: application/json' -d '{"input_file_id": "<id from step 1>", "endpoint": "/v1/chat/completions", "completion_window": "24h"}'CheckBatchCostcycle, which registers the batch'soutput_file_idanderror_file_idas managed ids withfile_objectnullcurl "http://localhost:4000/v1/files?target_model_names=my-gpt" -H "Authorization: Bearer sk-1234"; before this change that returns a 500 namingNoneType, after it returns 200 with the parseable files and without the poller-registered rowsType
🐛 Bug Fix
Changes
store_unified_file_iddeliberately acceptsfile_object=None(LiteLLM_ManagedFileTable.file_objectisOptional, and the writer only sets the column when the value is non-null), and the batch cost poller uses exactly that path when it re-registers a completed batch'soutput_file_id/error_file_idas managed ids. The reader disagreed:get_user_created_file_idsdidOpenAIFileObject(**row.file_object)for every row, so one null column raisedTypeErrorand the wholeGET /v1/filesresponse became a 500 for that callerRows are now parsed through a small helper that returns
Nonefor a null or unparseable column and logs a warning, and the listing skips those, matching howalist_batchesalready tolerates rows it cannot parse. The helper also accepts the JSON-string form, which is what the writer actually persists (file_object.model_dump_json()into a Json column), so a string column no longer depends on Prisma decoding it back into a dictNo backfill is needed; a poller-registered output row carries no provider metadata worth showing
One nearby inconsistency I did not touch, since it is a separate bug:
delete_unified_file_idreturnsinitial_value.file_objectannotated asOpenAIFileObjectwhile the DB gives back the raw column, soDELETE /v1/files/{id}can hand a JSON string orNonestraight to the response modelFinal Attestation