fix(batches,files): bound batch input-file read; keep untracked files on list - #35813
fix(batches,files): bound batch input-file read; keep untracked files on list#35813mubashir1osmani wants to merge 3 commits into
Conversation
POST /v1/batches could hang indefinitely. BatchRateLimiter.async_pre_call_hook runs inline in the request path and, for keys with applicable rpm/tpm limits, read the input file to count tokens with no deadline. With none set the OpenAI SDK default applied (600s, max_retries=2), so a slow or stalled Files API held the request open far past any client timeout; 63.6s was observed on stage against a 60s client read timeout. The read does double duty: it counts tokens for rate limiting, and it validates every body.model in the JSONL against the caller's allowlist. Those have opposite safe defaults, so the timeout policy splits on whether the key needs that check. A key restricted to a subset of models is rejected, because admitting it unchecked grants exactly the bypass _should_skip_batch_input_file_processing refuses to allow via operator config. A key with unrestricted access is admitted unmetered, matching the existing fail-open, so a degraded Files API does not become an outage. The deadline is passed to afile_content as well as to wait_for. afile_content runs the sync client via run_in_executor, and cancelling that await does not interrupt a thread already in the pool, so bounding only the await would leak the worker until the SDK's own timeout fired. Also unskips the e2e test that guards the LIT-3266 unattributed-spend-row regression, which was blocked on this hang. Defaults to 10s; override with general_settings.batch_input_file_read_timeout.
BATCH_INPUT_FILE_READ_TIMEOUT_SECONDS failed the documentation gate, which requires every env var to be documented in the environment-settings reference (that lives in the litellm-docs repo, not here). The env var was redundant anyway: general_settings.batch_input_file_read_timeout already makes the deadline configurable per deployment, which is what was asked for. Keeping only the general_settings key leaves one documented way to set it.
The managed-files list hook replaced the provider page with only rows owned by the caller in the managed table. Provider-scoped uploads that never enter that table (the normal /openai/v1/files path) were dropped, which is LIT-4820. Keep untracked provider files, hide managed files owned by other callers, and accept the hook's AsyncCursorPage return. Also remove the Linear OAuth MCP e2e suite (not needed) and unskip the files-list e2e now that the filter is fixed. Batch input-file read timeout (LIT-5027) was cherry-picked earlier on this branch.
Greptile SummaryThis PR bounds batch input-file reads, changes managed-file listing behavior to preserve provider files without managed rows, and removes an unused OAuth end-to-end suite
Confidence Score: 4/5The shared-provider file-list isolation failure must be fixed before merging; the new request-path database queries should also be moved behind an approved access layer Untracked provider files now bypass ownership filtering and can appear to other virtual keys using the same provider credentials, while the list hook also adds direct synchronous database work Files Needing Attention: enterprise/litellm_enterprise/proxy/hooks/managed_files.py
|
| Filename | Overview |
|---|---|
| enterprise/litellm_enterprise/proxy/hooks/managed_files.py | Reworks provider-file filtering but exposes untracked shared-provider files across callers and adds direct request-path database queries |
| litellm/proxy/hooks/batch_rate_limiter.py | Bounds input-file reads and distinguishes restricted-key rejection from the documented unrestricted-key fail-open policy |
| litellm/proxy/openai_files_endpoints/files_endpoints.py | Correctly accepts AsyncCursorPage replacements returned by the post-call hook |
| litellm/proxy/_types.py | Adds the typed general setting for the batch input-file read deadline |
| tests/test_litellm/enterprise/proxy/test_managed_files_hook.py | Covers preservation of untracked files and exclusion of other owners' managed rows but does not test isolation between callers sharing untracked provider files |
| tests/test_litellm/proxy/hooks/test_batch_file_validation.py | Adds focused timeout, policy, and configuration regression coverage |
| tests/e2e/batches/test_batches_e2e.py | Re-enables the affected batch and file-list end-to-end scenarios without weakening their assertions |
Reviews (1): Last reviewed commit: "fix(files): keep untracked provider uplo..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| for provider_file in provider_files: | ||
| if provider_file.id not in claimed_by_anyone: | ||
| kept.append(provider_file) |
There was a problem hiding this comment.
If multiple virtual keys share upstream provider credentials, an upload without a managed row is appended without applying build_owner_filter, causing its file metadata to appear in another caller's GET /v1/files results.
How this was verified: The untracked-ID branch returns the provider object directly, while ownership filtering is applied only when loading managed rows.
Rule Used: What: Fail any PR which may contains a security in... (source)
Knowledge Base Used: Enterprise package (enterprise/)
| rows = await self.prisma_client.db.litellm_managedfiletable.find_many( | ||
| where={"flat_model_file_ids": {"hasSome": model_object_ids}}, | ||
| ) |
There was a problem hiding this comment.
Every nonempty managed file-list response now performs this owner-agnostic query inline, followed by another direct query when claimed IDs exist, increasing database load and latency on GET /v1/files and violating the repository rule against direct request-path database access.
Rule Used: What: In critical path of request, there should be... (source)
Knowledge Base Used: Enterprise package (enterprise/)
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
| e.file_id, | ||
| e.timeout_seconds, | ||
| ) | ||
| return data |
There was a problem hiding this comment.
Medium: Batch rate-limit bypass on file-read timeout
An authenticated caller with unrestricted model access can reference a valid input file whose download exceeds this deadline. This return skips _check_and_increment_batch_counters, while batch creation continues using the file ID, allowing the caller to submit work without charging its configured RPM or TPM limits. Reaching this branch means applicable rate-limit descriptors were found, so the request should fail closed rather than be admitted unmetered.
| return data | |
| raise ProxyException( | |
| message=( | |
| f"Could not read the batch input file within {e.timeout_seconds}s " | |
| "to enforce batch rate limits. Retry when the files API is available." | |
| ), | |
| type=ProxyErrorTypes.internal_server_error, | |
| param="input_file_id", | |
| code=504, | |
| ) from e |
PR overviewThis PR adds a time bound when reading batch input files and keeps untracked files visible in file listings. One security issue remains open: when an input-file read times out, batch creation can continue without incrementing applicable RPM or TPM counters. An authenticated caller with unrestricted model access could use a slow valid file to bypass configured batch rate limits, creating a limited resource-abuse risk. Open issues (1)
Fixed/addressed: 0 · PR risk: 5/10 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
1 participant
TLDR
Problem this solves:
How it solves it:
Relevant issues
Linear ticket
Resolves LIT-5027
Resolves LIT-4820
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
Needs filling in before review. Live proxy curls for (1) rate-limited batch create completing within the read deadline with no unattributed spend row, and (2) provider-scoped file upload then GET /v1/files including the new id. Unit coverage is already on the branch
Type
🐛 Bug Fix
✅ Test
Changes
POST /v1/batches was hanging when BatchRateLimiter counted tokens: it awaited afile_content with no deadline, so a slow Files API held the request open past client timeouts (LIT-5027). The read is now bounded (default 10s via general_settings.batch_input_file_read_timeout). On timeout, keys with a model allowlist are rejected (504); unrestricted keys are admitted unmetered, matching the existing fail-open. The e2e that guards the unattributed-spend-row contract is unskipped
GET /v1/files was dropping provider-scoped uploads that never enter the managed-file table (LIT-4820). The managed-files list hook replaced the whole provider page with only caller-owned managed rows. It now keeps untracked provider files, keeps the caller's managed rows, and still hides managed files owned by other callers. list_files also accepts the hook's AsyncCursorPage return. The files-list e2e is unskipped
tests/e2e/mcp/test_mcp_chat_completion_oauth_e2e.py is removed (Linear OAuth path not needed)
The third batch skip (hosted_vllm file/batch create) is env-only: HOSTED_VLLM_API_BASE is not provisioned in e2e. Left skipped; not a product bug
QA runbook
tests/e2e/batches/test_batches_e2e.py::test_rate_limited_batch_create_leaves_no_unattributed_spend_row - rate-limited key batch create leaves no unattributed spend row
tests/e2e/batches/test_batches_e2e.py::TestOpenAIFiles::test_uploaded_file_appears_in_list - provider-scoped upload shows up on GET /v1/files
Final Attestation