fix(batches): bound the batch rate limiter's input-file read - #35408
fix(batches): bound the batch rate limiter's input-file read#35408mubashir1osmani wants to merge 3 commits into
Conversation
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
PR overviewThis pull request bounds how long the batch rate limiter spends reading an uploaded input file while calculating batch usage. It updates the batch-processing rate-limit path to handle file reads that exceed the deadline. One issue remains open: a caller with unrestricted model access can intentionally trigger the file-read timeout and execute a batch without incrementing configured RPM or TPM counters. This permits a targeted rate-limit bypass but does not directly grant broader access; one other issue has already been addressed. Open issues (1)
Fixed/addressed: 1 · PR risk: 5/10 |
Greptile SummaryAdds a manually dispatched workflow for building selected LiteLLM container variants and optionally publishing them to the fork owner's GHCR namespace. Confidence Score: 4/5The workflow should not be merged until requested variants are normalized or invalid input is rejected instead of silently omitting images. Exact variant matching drops names surrounded by common comma-list whitespace, while the unvalidated image tag can also produce malformed Docker references. Files Needing Attention: .github/workflows/publish-ghcr.yml
|
| Filename | Overview |
|---|---|
| .github/workflows/publish-ghcr.yml | Adds the GHCR build-and-publish matrix, but input handling can silently omit requested variants or fail on malformed image tags. |
Reviews (1): Last reviewed commit: "Merge branch 'BerriAI:litellm_internal_s..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| wanted="${{ github.event.inputs.variants }}" | ||
| name="${{ matrix.name }}" | ||
| if [[ ",${wanted}," == *",${name},"* ]] || [[ "${wanted}" == "${name}" ]]; then |
There was a problem hiding this comment.
When variants contains conventional spaces such as litellm, database, non_root, the exact substring match does not recognize the latter names, causing the workflow to complete without building or publishing all requested images.
| tag="${{ github.event.inputs.image_tag }}" | ||
| sha="$(git rev-parse --short HEAD)" | ||
| image="ghcr.io/${owner}/${{ matrix.image_suffix }}" | ||
| { | ||
| echo "image=${image}" | ||
| echo "tags=${image}:${tag},${image}:${sha}" |
There was a problem hiding this comment.
image_tag is copied directly into the image references without validation, so values containing spaces or commas produce malformed tags and make the Docker build step fail after the workflow has already performed setup and registry login.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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.
Passing `timeout` as its own keyword alongside `**fetch_kwargs` raised TypeError when the deployment's credentials already carried one. `timeout` is one of `_extract_file_access_credentials`' credential keys, so any deployment whose litellm_params set it hit "got multiple values for keyword argument 'timeout'", turning POST /v1/batches into a 500 rather than fixing its hang. Set it on the resolved kwargs instead, after credentials are merged, so the limiter's budget still wins: a deployment timeout is sized for serving traffic, not for a read that blocks the request path. The regression test drives the real resolver with a deployment that configures its own timeout, so it covers the merge rather than a stubbed return value.
405b228 to
971df2a
Compare
| e.file_id, | ||
| e.timeout_seconds, | ||
| ) | ||
| return data |
There was a problem hiding this comment.
Low: Rate-limit bypass on file-read timeout
A caller with unrestricted model access can upload a sufficiently large batch file to make the content read exceed this deadline, after which this return allows the batch to execute without incrementing its RPM or TPM counters. Fail closed on timeout whenever applicable rate-limit descriptors caused the file to be processed; unrestricted model access removes the allowlist requirement, not the configured usage limits.
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.
1 participant
TLDR
Problem this solves:
POST /v1/batchescould hang indefinitelyHow it solves it:
Relevant issues
Linear ticket
Resolves LIT-5027
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
Pending; I will attach the live-proxy runs against a proxy built from this branch. The reproduction needs a Files API that stalls on read, so the before/after is captured by pointing a deployment at a file host that holds the connection open, then re-running the same batch create with the fix in place
Type
🐛 Bug Fix
Changes
BatchRateLimiter.async_pre_call_hookruns synchronously in the request path. When the calling key has applicable rpm/tpm limits, it read the batch input file to count tokens and awaited that read with no deadline, so the OpenAI SDK default applied: 600s withmax_retries=2. A slow or stalled Files API therefore heldPOST /v1/batchesopen far past any reasonable client timeout. On stage a read took 63.6s against a client whose read timeout was 60s, so the client gave up first and the eventual 404 (the client's teardown had already deleted the file) read as the cause rather than the consequenceThe read is now bounded. Both fetch paths go through one
asyncio.wait_for, since the managed-files hook accepts no timeout argument of its ownThe failure policy splits, because the read does double duty. It counts tokens for rate limiting, and it validates every
body.modelin the JSONL against the caller's model allowlist. Those two have opposite safe defaults on timeout. A key restricted to a subset of models is rejected with a 504: admitting its batch without reading the file would grant exactly the bypass that_should_skip_batch_input_file_processingalready refuses to allow through operator config, letting a caller run models outside its allowlist under the proxy's shared credentials by making the read slow. A key with unrestricted model access has only rate-limit accuracy at stake, so its batch is admitted unmetered with a warning, matching the fail-open the generic handler already applied; a degraded Files API does not turn batch creation into an outageThe deadline is also passed to
afile_content, not just towait_for.afile_contentruns the sync client viarun_in_executor, and cancelling that await does not interrupt a thread already in the pool, so bounding only the await would free the request handler while leaking the worker until the SDK's own timeout fired. The managed-files path takes no timeout argument, so therewait_foris the only bound and an abandoned read may still hold its thread; that is called out in a comment rather than papered overTwo smaller things fall out. The hang was invisible in logs because the hook emitted nothing between entry and failure, and the eventual error was swallowed by the "don't block the request if rate limiting fails" handler, so neither the caller nor the operator learned the limiter had failed. Both timeout branches now log. The rejection raises
ProxyExceptionrather thanHTTPExceptionso it maps to the same OpenAI error shape as the proxy's other refusalsI checked the rest of the pre-call hooks for the same unbounded-await shape, as the ticket asked.
batch_rate_limiter.pyis the only hook that reads filesNew setting:
general_settings.batch_input_file_read_timeout, default 10s, also settable per-deployment viaBATCH_INPUT_FILE_READ_TIMEOUT_SECONDS. Non-positive and non-numeric values fall back to the default, since a 0s budget would expire instantly and reject every restricted key's batchQA runbook
curl -X POST http://localhost:4000/key/generate -H "Authorization: Bearer sk-1234" -H "Content-Type: application/json" -d '{"models": [], "tpm_limit": 1000000, "rpm_limit": 1000, "user_id": "e2e-batch-rl-manual"}'curl -s "http://localhost:4000/spend/logs/v2?start_date=$(date -u -v-1H +%Y-%m-%dT%H:%M:%SZ)&end_date=$(date -u -v+1H +%Y-%m-%dT%H:%M:%SZ)" -H "Authorization: Bearer sk-1234" | jq '[.data[] | select(.api_key == "" or .api_key == null)] | length'purpose=batchusing the key from step 1, thenPOST /v1/batcheswith the returnedinput_file_idand expect 200 within a few seconds rather than a client-side read timeoutFinal Attestation