Opt-in allowlist for upload filename extensions, checked before the existing blocked_file_extensions blocklist and mapped through the same upload validation failure path. None keeps today's behaviour, [] rejects every upload, matching is case-insensitive on both sides, and a filename with no extension is rejected when the allowlist is set.
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Converge on staging's delete plumbing (_S3DeleteContext read from the logging call's additional_args, _sign_s3_request_without_body, the credential-stripping delete_data in the managed-files hook) and keep this PR's listing support, the 400 mapping for out-of-bucket file ids, the proxy-admin-only raw cloud id rule, and the OpenAI FileDeleted delete response.
Two staging tests move to this PR's contract: an out-of-bucket delete raises BedrockError 400 instead of ValueError, and deleting a stored provider output returns FileDeleted rather than the stored file object.
error_status_code only read status_code, so a ProxyException raised
before routing (which stores its status as the string code) answered
500 with its 4xx type through the rerank, images, realtime, files, and
pass-through tails. It now falls back to a decimal code. A 408 maps to
timeout_error instead of invalid_request_error.
Tail regressions for rerank, images, realtime calls, and the chat
pass-through fail at the merge base with ('None', 'None'); the new
files-test helpers are fully typed.
A key allowed to call a Bedrock model could delete any object under the
deployment's buckets through DELETE /bedrock/v1/files/{s3 id}?model=...
because the managed-file ownership check only runs for unified ids. Raw
cloud storage ids now answer 403 on every delete route unless the caller
is a proxy admin; managed ids and require_managed_files are unchanged
The proxy's exception tails defaulted `type` and `param` to the four-character
string "None", which is neither a known OpenAI error type nor the JSON null the
nullable `param` field is typed as, so a client's error handler matched nothing
and fell into its generic branch.
Lifts the helpers PR #39521 added for the unified LLM endpoints into
litellm/proxy/common_utils/openai_error_payload.py and calls them from the file,
rerank, image, realtime, anthropic, and pass-through route families, plus the
shared handle_exception_on_proxy handler that the management, batches,
fine-tuning, credential, SCIM, guardrail, and customer routes funnel through.
The remaining families (proxy_server, auth, health, spend tracking, and
management endpoints) follow in separate PRs so each slice stays QA'able on a
live proxy.
* fix(security): restrict and validate file uploads at /v1/files and /upload/logo
Extends fast-fail upload validation to every purpose at POST /v1/files,
not just purpose=batch: a configurable max_file_size_mb size cap and a
blocked_file_extensions denylist, plus rejection of filenames carrying a
directory-traversal component before anything is read, stored, or
forwarded to a provider.
Also fixes two concrete gaps found while auditing every upload surface:
the Azure Blob Storage backend derived a blob path's extension with
filename.split(".")[-1], which does not parse path structure and let a
crafted filename embed a directory traversal sequence into the stored
blob path; and POST /upload/logo (the admin UI logo upload) had no
role check at all, so any authenticated API key, not just a proxy
admin, could write a file to the server's disk.
* fix(lint): drop cast()/mutation from settings coercion, sync blocked_file_extensions on reload
Replaces the TypeAdapter+cast() reads of max_file_size_mb and
blocked_file_extensions with small isinstance-based validators, since the
codebase's cast() budget (LIT006) had no headroom left. Also adds the
blocked_file_extensions reload block that was missing from
_update_general_settings: it was registered as an editable setting but
never re-synced into runtime state, so a value set through the DB-backed
settings editor would silently never take effect (Greptile finding).
* fix(security): declare max_file_size_mb and blocked_file_extensions on ConfigGeneralSettings
The DB-backed general-settings update endpoints validate every field
through ConfigGeneralSettings.model_fields before persisting it, so
without these declarations an operator could never actually set either
setting through that path even though both were registered for the
Admin UI's settings editor and reloaded on config refresh (Greptile
finding). blocked_file_extensions is typed as a tuple, not a list, to
stay out of the immutable-collections lint budget; the stored JSON
value is unaffected since the raw request payload, not the validated
model, is what gets persisted.
* chore: regenerate schema.d.ts for the new ConfigGeneralSettings fields
* fix(security): normalize configured blocked_file_extensions casing
check_blocked_extension lowercased the uploaded filename's extension
before comparing but compared it against blocked_extensions verbatim,
so an admin-configured blocked_file_extensions: ['.EXE'] would never
match an uploaded payload.exe (Greptile finding). Normalizes the
configured values the same way at comparison time, and adds the
missing case (mismatched-case config, lowercase upload) as a
regression test, mutation-checked against the unfixed comparison.
* fix(security): restore caller-owned stream position after size inspection
_file_size_bytes unconditionally seeked back to 0 after measuring a
BinaryIO's length, discarding wherever the caller had actually
positioned it (Greptile finding). Saves and restores the original
position instead. Rewrites the existing test that had encoded the
old "always resets to 0" behavior as its expectation, and adds a
sibling case for the under-cap path; both are mutation-checked
against the unfixed always-reset-to-0 behavior.
* test: add regression coverage for twelve closed issues
Adds targeted regression tests for behavior that was fixed but left ungated,
so the fixes cannot silently regress:
- #33772 openai cache_write_tokens cost
- #34309 Responses API cache cost_breakdown
- #35363 /v1/responses batch spend
- #36619 auto-router api_base/api_key leak on a shared model name
- #35359 batch fallbacks within the owning model group
- #36523 passthrough streamed Responses spend log
- #36646 passthrough embeddings spend log
- #37147 non-object metadata on create_batch is a 400
- #35362 unscoped list files reads the managed-file store
- #33221 gpt-5.6 bridges to Responses on function tools alone
- #34487 LLM complexity classifier runs for every caller metadata shape
- #35124 streamed /v1/messages emits success logging on both bridges
Cost assertions read rates from litellm.model_cost rather than hardcoding
dollar amounts, so they do not drift on repricing.
* fix: stop the new regression tests polluting and tripping over shared global state
Two shard failures, both from global state the new tests share with their
neighbours rather than from the behaviour under test.
test_main.py's local_cost_map pinned litellm.model_cost but left the
get_model_info lru_cache warm, so completion_cost billed at whatever prices
were cached earlier in the process while the assertions read the pinned map.
Clear the cache on both sides of the fixture, matching the local_model_cost_map
fixture in tests/test_litellm/conftest.py.
The anthropic messages streaming tests called GLOBAL_LOGGING_WORKER.flush()
on whatever queue happened to be around. A queue left non-empty by an earlier
test is still bound to that test's loop, so join() either hangs or raises
"bound to a different event loop". Rebind to the running loop before the call
and wait for the captured payload instead of a fixed sleep.
The field is declared optional on OpenAIFileObject and its own docstring says it
is absent on every upload guardrails did not touch, but the /v1/files routes have
no response_model, so FastAPI falls through to jsonable_encoder with exclude_none
off and serialises the unset default as an explicit null. Every create and
retrieve response on a proxy with no guardrails configured at all picked up a
litellm_batch_guardrail: null it never had before, and so did every row of a file
list, since those rows are the same object.
A wrap serializer drops the key only when nothing set it, so the populated report
still reaches the wire intact, including a record whose guardrail is null. The
managed-files list route spreads a stored file_object blob rather than the model,
so rows persisted before this lands keep their null until it is dropped there too.
Eleven DummyManagedFiles stubs still declared afile_list(self, purpose,
litellm_parent_otel_span). The real hook grew user_api_key_dict, limit and
after, so the doubles no longer stand in for the interface they replace.
Their tests pass today only because every one of them takes a provider
branch that never reaches the hook, which means a stub going stale is
invisible until some later test does reach it and reads a TypeError as a
behavior change.
Signatures only; no test changes behavior.
* test: drop the cwd-relative sys.path.insert calls from the test suite
TQ003 stands at 1,077 across 1,058 files, and 1,015 of them are the same shape:
sys.path.insert(0, os.path.abspath("../..")) and its deeper siblings. The
argument resolves against the working directory rather than the file, so from
the repo root, where every job runs pytest, it inserts the directory two levels
above the checkout. It has never pointed at litellm. The package is installed
into the environment anyway, which is what actually makes the import work, and
what the rule's message has said all along.
Removing them leaves 1,634 imports of sys and os with no remaining reference,
and those go too, except where another test module imports the name back out of
the file. The rest of TQ003 is 62 call sites that resolve against __file__ or a
variable, which are a different question and are left alone.
Collection is identical either way: 45,871 tests and the same 51 pre-existing
collection errors before and after, and ruff reports no new undefined name.
* test: drop the duplicate imports the sys.path sweep exposed to F811
* test(pre-call-utils): restore the os import the new bedrock tests need
The managed hook returned the plain dict build_list_page builds, while
every other GET /v1/files path returns an SDK page object. A post-call
success hook or a logging callback that reads response.data off the
listing raised AttributeError as soon as a request took the managed path
FileListPage is a pydantic model over the same five fields, so hooks read
.data again and the response body does not move: jsonable_encoder gives
the same keys in the same order for the model and for the dict. It sits
in litellm.types.llms.openai because base_llm/files/transformation.py
already imports from there and cannot import proxy modules. It is
deliberately not subscriptable, since the provider-backed path returns a
page object that is not either, and dict access would be a third contract
to keep alive
Also reject a purpose the Files API never accepts. An unknown purpose
matches no row, so the listing answered an empty page for what is really
a bad request, while the upload route in this same file already refuses
those values against get_args(OpenAIFilesPurpose). The check runs before
the first query, and only in the managed hook, so providers that define
their own purposes keep them
Also put back the route's original except tail. Sending every error
through handle_exception_on_proxy changed error.type on a bad
target_model_names from "None" to the exception class name, which a
caller matching on the body would read as a break. create_file in this
file already pairs base's tail with a ProxyException passthrough, so
list_files does the same and the handle_exception_on_proxy import is gone
The managed file listing cut the page to `limit` first and applied the
purpose filter in Python afterwards, so a page whose rows all failed the
filter came back as `data: []` with `has_more: true`. openai-python stops
paging the moment `data` is empty, so `files.list(purpose="batch", limit=1)`
returned nothing at all instead of every batch file.
Read successive keyset chunks until the page holds `limit + 1` matches or
the caller's rows run out, then return at most `limit` of them. `data` is
now non-empty whenever matching files remain, its last id is always a
usable cursor, and `has_more: false` only ever means the caller has seen
everything. Rows whose stored blob will not parse drop out in the same
loop, so they cannot empty a page either.
That also makes the `next_cursor_id` escape hatch on `build_list_page`
dead, so it goes back to what it was for the batch and vector-store
listings that share it.
Also move `validate_file_list_limit` up into the list_files route, so the
target_model_names and provider branches reject an out-of-range limit the
same way the managed file store already did.
The unscoped GET /v1/files limit check accepted 0, which OpenAI's minimum
of 1 does not allow, and the route's except block rebuilt every error with
getattr(e, "status_code", 500). ProxyException has no status_code, so the
400 it raises went out as a 500 and the OpenAI SDK retried it three times.
Errors now go through handle_exception_on_proxy, the helper the sibling
batches route already uses, and the unknown-cursor error is a ProxyException
so it carries type invalid_request_error and param after instead of the
literal "None". The cursor still 400s whether the file belongs to someone
else or does not exist at all
The owner-scoped listing read every row the caller owns in one query, so an
admin key that owns every file on the proxy pulled the whole table into one
response. Page it with a keyset cursor on unified_file_id instead, and accept
limit and after on GET /v1/files so a client can walk the pages. limit follows
what OpenAI documents for that route: 1 to 10000, default 10000.
An after cursor is resolved inside the caller's own scope, so an id they do not
own gets a 400 rather than a page, and has_more now reflects whether another
row exists instead of always being false.
Refs #37714
* fix(proxy): scan batch records with the content hooks that are not guardrails
Guardrails were made to run on batch uploads by scanning each record through the pre-call hook
with the walk limited to guardrails. That limit exists because the same branch carries the rate
limiters and budget accounting, which must count an upload once rather than once per line. It
also excluded every enforcement hook written as a plain CustomLogger, so prompt-injection
detection, Azure content safety, banned keywords and the blocked-user check never saw a batch
record at all. Content that is a hard 400 online reached the provider verbatim through batch.
A CustomLogger now declares whether its pre-call hook judges the payload or merely counts the
request. The four that judge it opt in, the walk admits them, and both short-circuits learn
about them, including the one that decides whether the file is streamed off disk in the first
place: a proxy configured only with one of these hooks was skipping the scan entirely. Nothing
that counts a request is marked, so an upload still costs one slot and one budget check.
* refactor(proxy): drop the per-hook comment the attribute contract already states
* test(proxy): make the classification a ledger, and pin the wiring with a real hook
The classification test listed the two non-enterprise hooks by hand, so unmarking either
enterprise one changed nothing and the mutation matrix passed with both surviving. It now walks
the hook registries and fails on any pre-call CustomLogger that is on neither side, which also
gives the flag the forcing function it lacked: an enforcement hook added later would otherwise
default to off and silently skip batch records, which is the bug being fixed here.
Nothing exercised the path the bug actually lived on either, since every test raised its own
exception rather than a real hook's. One test now drives the shipped prompt-injection hook
through the scan, which pins the part no synthetic exception reaches: a chained exception reads
as a failure to judge, so refactoring any of these hooks to `raise ... from` would turn every
per-record drop into an aborted upload.
Also records why a hook that rewrites the payload for routing stays unmarked, and that only the
leaf class is consulted.
* test(proxy): set the callback list through monkeypatch rather than writing the global
* fix(proxy): read batch records the same way the upload validation does
The upload validation parses each JSONL line as bytes, where the json module sniffs the
encoding itself and accepts a leading byte order mark or a lone surrogate. The guardrail scan
that runs immediately after decoded each line to text first, which is stricter, so a file the
validation had just accepted could fail the scan. A `.jsonl` written by any of the editors that
emit a BOM, which includes PowerShell's Out-File and classic Notepad, uploaded fine until a
pre_call guardrail was configured and then returned 500 with a decode error and no indication
of which line or why. The scan now parses the same bytes the validation did, and an untouched
record is copied through as the bytes it arrived as rather than re-encoded.
A numeric custom_id was reported as null. The spec asks for a string, but callers do send
numbers, and null leaves the one field a caller reconciles on empty for exactly the records
that need it.
* fix(proxy): read the load-balancing record the same way, so a byte order mark keeps its routing
The first record is parsed to pick a deployment when batch load balancing is on, and it was
decoded to text before parsing, which rejects a leading byte order mark. The lookup returns None
on any parse failure, so such a file silently lost its routing and went to the default provider
rather than the configured one. That was already reachable for an upload no guardrail changed,
since the original bytes are passed straight through, and preserving the mark through a rewrite
widens it. Parsed as bytes now, like the validation and the scan.
* fix(proxy): find the routing record past a blank first line
The upload validation and the guardrail scan both skip blank lines, but deployment selection
read only the first physical line, so a file starting with a blank line lost its routing model
and went to the default provider rather than the configured one. It now skips blanks the way
the other two readers do, reading lazily so a large file is not read past its first record.
* fix(proxy): do not crash deployment selection on a record whose body is not an object
The upload validation checks that a record has a `body`, not that it is an object, so a record
can carry a string or a list there. Deployment selection called `.get` on it unconditionally and
raised, returning 500. That was already reachable for a plain file, and reading past a byte order
mark or a blank first line widened it to files that previously fell through to the default
provider instead. A record whose body names no readable model now resolves to no model, which is
the same answer the default-provider branch already handled.
* fix(proxy): keep a custom_id that cannot be encoded from failing the whole upload
A record identifier is echoed back in the create response. JSON parses a lone surrogate happily
but it cannot be encoded again, so a file the upload validation accepts returned 500 from the
response renderer rather than a report. Unencodable characters are replaced, which leaves every
ordinary identifier untouched and keeps a pathological one reconcilable.
This predates the reader change; reading past a byte order mark only altered which error the
same file produced first.
* fix(proxy): treat a url the parser rejects as one we do not recognize
Resolving a record's call type from its url runs the url through urlsplit, which raises on a few
malformed authorities such as an unclosed bracket. That happens before the try that wraps the
guardrail call, so it escaped the scan and returned 500 on a file the upload validation had just
accepted. An unreadable url is simply one we cannot recognize, which the body-shape fallback
already handles, so the record is still scanned rather than lost.
Reachable on staging today for a proxy running any guardrail. Enabling the scan for a proxy that
runs only a content-enforcing CustomLogger widens it to that configuration too, which is why it
is fixed here rather than left.
`pytest.raises(Exception)` with no `match=` passes on any error that broad. A
TypeError from a refactor, a botched fixture, an import that moved: all of them
read as the rejection the test claims to police, so the test goes green for the
wrong reason and stays green after the behaviour it guards is gone.
PT011 closes that gap for the 317 sites B017 could not reach, because B017 only
fires on a single-statement body with no `as e` binding. Each pattern here is the
message the code actually raised, recorded by running the sites under a plugin
that logged the concrete type and text per call site, so the assertions describe
observed behaviour rather than a guess. Where a site raises more than one message
across its parametrize cases, the pattern is an alternation of what was seen;
where the exception carries an empty `str()` and puts the text on `.message`, the
site keeps a narrow `noqa` with the reason.
PT014 removes four parametrize cases that were listed twice. The duplicate re-runs
an assertion that already passed, and it usually marks a case someone meant to
vary and forgot to edit.
* feat(proxy): redact or drop individual batch records instead of rejecting the file
A single record tripping a guardrail rejected the whole upload, which is unusable for a file
holding thousands of rows. A record a guardrail rewrites is now submitted in its rewritten
form, a record it blocks is left out, and the create response reports every changed record by
both custom_id and line so a caller can reconcile against the file it sent. The same outcome
is written to the proxy log and to request metadata, so it is not visible only to the caller.
A rewritten record goes straight to a spool and only its offset is carried, so a masking
guardrail touching most rows of a large upload does not build a second copy of the file on the
heap, and the rewrite runs off the event loop the way the sibling full-file validation does.
Both proxy-injected metadata keys are captured from the record and restored exactly, including
an explicit null, so a masked row keeps the tags that decide how it is attributed.
A record is dropped only when a guardrail judged its content. `GuardrailRaisedException` now
carries `blocked_content` for that, because half its raise sites in the repo signal an
unreachable or unparseable backend under a fail-closed policy, and treating those as blocks
would turn "refuse this request" into "drop this record and submit the rest". The default is
off, so a raise that does not say what it means aborts the upload instead of silently
shrinking the file.
* fix(proxy): only drop a batch record on a verdict the guardrail actually reached
A guardrail that reports a technical failure as an HTTPException carrying a block status was
read as a content block, so an unreachable backend under a fail-closed policy quietly shrank the
file instead of failing the upload. Two in-tree integrations do exactly that, and one of them
defaults to fail-closed, so the broken configuration was the default one. Such an exception is
raised `from` the underlying error, which is a deliberate statement that something else caused
it, and no content verdict in the repo is raised that way, so the chain now settles it. Implicit
context is left alone, since a block raised inside an unrelated `except` would read as a failure.
Two annotation errors in the same family: the one GuardrailRaisedException subclass in tree never
opted into blocked_content, so a real block took the whole upload down with it, and straiker's
block helper is reached both from its verdict and from its fail-closed handler, so it claimed a
verdict for an outage. The helper now takes the flag from its caller.
A record could also opt itself out of the chain. Guardrail selection reads a body-level
`guardrails` key ahead of the proxy-injected list, and online that key can only add to the key
and team selection, never replace it, so a batch record naming an empty list skipped every
guardrail that was not default_on and was still reported as scanned. Every injected key is now
stripped before dispatch and restored afterwards.
A guardrail that reroutes a record to another model is honoured on the online path by rewriting
the model, which the scan read as a rewrite and submitted in the same file, sending content to
the provider the reroute existed to avoid. Every record of a batch file goes to one provider, so
the upload is refused instead, naming the line.
The scan spool is closed on the paths that never read it back.
* fix(proxy): give the scan the metadata bag guardrails actually read, and close its spools
The narrowed request metadata was installed under `litellm_metadata` only, but a record is
scanned as the chat request it describes, and the guardrails that pick a policy from a request
header read `metadata` instead. Noma choosing an application and Aim choosing a user both look
there, so the header allowlist added for them did not reach either one and a batch record was
still evaluated under the fallback policy. The scan metadata now goes into both bags, which are
both stripped and restored, so neither survives into the record that ships.
The scan spool was closed on the paths that abort, which are exactly the paths where it is
empty, and left open on the one path where it holds the rewritten records. Nothing closed the
rewrite output either, where before this feature the uploaded handle belonged to Starlette. The
upload now owns both and closes them however it exits.
* fix(proxy): register the scan spool before the rewrite can fail
The scan spool was added to the request's cleanup list only after the rewrite returned, so a
rewrite that raised, which for a spilled file can be as ordinary as the disk filling up, jumped
to the handler with the list still empty and left the scan's own handle open. The rewrite also
left its half-written output behind on that path, since nothing owns that handle until it is
returned. Both now close.
* fix(proxy): run pre-call guardrails on batch input file uploads
POST /v1/files with purpose=batch was the only route in files_endpoints that
never reached pre_call_hook, so guardrails did not see batch content at all and
records reached the provider unscanned.
Stream the uploaded JSONL a record at a time and run each record's body through
the existing pre_call_hook dispatch under the call type its url maps to, so
guardrail resolution, key and team config, and the per-endpoint translations are
reused rather than reimplemented. The hook gains a guardrails_only mode for this,
since the same callback loop also drives rate limiters, budget hooks, prompt
templates and hanging-request alerting, none of which should fire once per record.
A guardrail that blocks raises its own exception, which propagates untouched so
its status code survives. A record a guardrail would rewrite, a record that
cannot be parsed, and a record whose url cannot be scanned all reject the upload,
since silently skipping any of them is the bypass this is meant to close.
Per-record redaction lands separately.
The scan only runs when a guardrail that actually runs pre_call, or a guardrail
pipeline, is configured, so deployments without one are byte for byte unchanged.
* fix(proxy): compare the dict a batch guardrail returns, not the one it was given
async_pre_call_hook may return a replacement dict instead of mutating its input, and
process_pre_call_hook_response then makes that replacement the request. The scan only
inspected the dict it passed in, so a guardrail that redacts by returning a copy was
treated as a no-op and its record uploaded unchanged.
* fix(proxy): treat a missing batch body key as different from a null one
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
* docs(proxy): document the guardrails_only mode on pre_call_hook
* fix(proxy): resolve a batch record's scan type from its body when the url is unfamiliar
The scanner only accepted five exact urls, but callers write that field by hand and the
provider transformers are far more permissive: bedrock treats any non-empty url as chat
and vertex strips query strings and trailing slashes. Uploads that work today would have
started failing the moment a pre-call guardrail was configured.
Normalize the url before lookup and fall back to the body shape when it is unfamiliar, so
a record we can still read is a record we still scan. Only a body with no messages, prompt
or input is now refused, and the error says so instead of listing urls that were never the
whole set.
Also pins the default side of the guardrails_only gate: the hanging-request alert and
prompt templating are asserted to still fire when the flag is absent.
* refactor(proxy): drop batch guardrail checks the upload validation already makes
check_batch_file_upload now runs first and rejects a line that does not parse, a line that
is not an object, and a line missing custom_id, method, url or body, so the guardrail scan
can rely on all four. Its own parse handling was unreachable through the endpoint and is
gone, along with the tests for it. What is left is the case that validation does not cover,
a body whose value is not an object, since it only checks that the key is present.
* fix(proxy): resolve a batch record's call type from the url path, not the whole url
A record naming its route in full, which is how callers actually write batch files, matched
no known route, so it fell through to the body shape. A Responses record carries `input`,
and that reads as an embedding, so the record was scanned as the wrong call type and any
guardrail scoped to chat or Responses skipped it while the upload was accepted. Chat records
survived only because their body shape happens to map back to the same call type. The url is
now reduced to its path before matching.
Guardrails that pick their policy from a request header, such as noma choosing an application
id, saw no headers at all during the scan and fell back to a default, so a batch record could
be evaluated under a different policy than the same content sent online. The sanitized headers
the proxy already stores in request metadata now travel with the scan.
Also drops the bare `dict` annotation, the unreachable non-dict branch on the guardrail chain's
own return, and the type alias that was missing its `TypeAlias`, which together were failing
the lint gate.
* fix(proxy): give each batch record its own copy of the scan metadata
The narrowed metadata was handed to every record as a shallow copy, so `headers` and `tags`
stayed shared with the upload request and with the other records in the same window. A guardrail
that writes into one of those in place, which several do to record their own bookkeeping, would
have its write show up in every record scanned after it and in the request itself. The narrowing
already removed the values that cannot be copied, so each record now gets a deep copy.
---------
Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Probe the column before the scheduler registers CheckBatchCost, closing the window where a retrieve that decided the poller was inactive billed a batch the first poll cycle then billed again. Also drop narration docstrings and section banners from the new tests.
The ownership question was asked twice for one retrieve: once before the provider
call to decide whether to suppress inline accounting, and again afterwards to
decide whether to mark the batch accounted. Between those two points the poller
can complete its first successful filtered query and become usable, so the two
answers disagree. The retrieve then accounts for the batch inline, having decided
the poller was unusable, while the later check sees a usable poller and leaves the
marker unset, so the poller accounts for the same batch again and its spend is
counted twice.
The retrieve now decides once and passes that decision to
update_batch_in_database, which prefers it over re-deriving one. Callers that
record no cost of their own leave it unset and keep deriving it as before, so the
cancel path is unchanged.
The handoff asked whether the poller was running, when what matters is whether it
will actually account for the batch. Those differ on a schema without the
batch_processed column: the poller cannot filter on it, so it falls back to a
query that excludes complete and completed rows, and it cannot set it either. A
caller retrieving a provider-completed batch before the poller saw it therefore
suppressed inline accounting, then marked the row complete, and the fallback query
could never find it again. Nobody accounted for that batch, so its cost escaped
the caller's budget entirely.
The poller now publishes batch_processed_support_confirmed, set only once a
filtered query has actually succeeded, and the handoff requires it. Defaulting to
unconfirmed keeps accounting on the retrieve path in exactly the cases the poller
would drop the batch, including the window before the poller's first cycle. All
four combinations account exactly once: unconfirmed leaves the retrieve
accounting and setting the marker, whether or not the column exists, and
confirmed is only reachable when the column is present, where the poller accounts
and sets it.
A scheduler that hands back something other than a bound method leaves no poller
to interrogate, which reads as unconfirmed rather than as working.
Two components computed a managed batch's cost and each assumed it was the only
one. Retrieving a batch computed it through the @client decorator's success
callback, and CheckBatchCost computed it on its own schedule. Whichever observed
completion first decided the outcome, so cost was either counted once per
retrieve or not at all.
The lockout is the worse half. Retrieving a batch that had reached completion set
batch_processed=True, which is what takes a batch out of CheckBatchCost's queue,
since it selects batch_processed=False. That write claimed the cost had been
accounted for on behalf of a callback that had not run yet and was not awaited.
When the callback then failed the cost was gone permanently, with the poller
already retired and no retry left. Observed on a live proxy: two completed
batches whose callbacks raised inside the logging worker, one on a provider
output path that did not resolve and one on a batch whose output file id was
still None, both left marked processed with no spend row and no way to recover
them. Nothing logged at error level for the batches themselves.
The over-count is the other half. Nothing suppressed recomputation, so each
retrieve of an already-completed batch recorded that batch's full cost again. A
caller polling its own batch to see whether it had finished inflated spend by
however many times it looked.
The flag now means what its name says, and only the component that actually
recorded the cost sets it. When the poller is running it owns accounting, so
retrieving a managed batch records no cost and leaves the flag alone; the poller
computes once and sets it. When the poller cannot be relied on, either because
polling is disabled by config or because the enterprise job never registered,
the retrieve path is the only accountant and behaves exactly as before. Batches
with no managed object row are untouched either way, since neither the flag nor
the poller queue applies to them.
The helper that carries the credential snapshot into litellm_params lived private
in files/main.py, and the batch retrieve needed it too. It now sits beside
get_litellm_params, which is what it augments, so neither caller reaches into the
other's private surface. Typed as Mapping/MutableMapping of object rather than
Any, which the strict import rules ban.
The file-content route builds the snapshot through the same helper as the batch
route instead of assembling a conditional mapping inline, which drops two mutable
constructions and leaves one way to attach it. Its name loses the batch suffix now
that both routes use it.
A third path reads a completed batch's output file, and it could not resolve the
bucket either. When cost is accounted from the retrieve itself rather than from
the poller, the batch success handler calls _handle_completed_batch, which fetches
the output file through _extract_file_access_credentials. That helper forwarded a
whitelist covering Azure and Vertex, gcs_bucket_name included, but nothing for
Bedrock, and retrieve_batch built its litellm_params through get_litellm_params,
whose fixed signature drops the trusted credential snapshot. So the snapshot never
reached the file read and it failed with "S3 bucket_name is required" for a bucket
the deployment had configured, leaving the batch's cost unrecorded.
Adding s3_bucket_name to that whitelist would not have worked. The Bedrock file
config deliberately resolves the bucket only from the immutable server-side
snapshot or the environment, never from a request param, because the bucket is
what managed file ids are validated against. The snapshot is therefore what has to
flow, exactly as it already does for the model-routed and cost-poller paths.
retrieve_batch now re-adds the snapshot after get_litellm_params, the same way the
file operations already do, the whitelist forwards it, and the proxy attaches it
for router-routed managed batches from the deployment behind the unified id.
Verified against a live proxy reading a real completed Bedrock batch: the cost row
appears within seconds of the retrieve carrying the batch's real spend and usage,
where before the read raised and no row was written.
Resolving those credentials is best effort. A batch whose deployment no longer
resolves, which happens when a model group is removed while batches are in
flight, still serves its status instead of failing the request on the lookup.
This matters for the OSS and polling-disabled configurations, where the retrieve
path is the only thing that accounts for a batch at all.
The mock merged every call into one shared dict, so a second routed retrieval would
overwrite the first and the assertions would still pass. Keep one frozen snapshot per
call and assert exactly one call, which also makes an unintended second retrieval a
failure rather than something the merge hides
get_configured_s3_bucket_name accepts the output bucket only from the immutable
_litellm_internal_model_credentials snapshot or AWS_S3_BUCKET_NAME. That refusal to read
litellm_params is deliberate: the bucket is what validate_managed_cloud_file_id checks a
file id against, so trusting a request-supplied value would let a caller redirect reads
to a bucket of their choosing
Two live entry points reach the Bedrock file-content transformation without ever building
that snapshot. The managed-files pre-call hook sets data["model"] for any id carrying
llm_output_file_id, which is every batch output, so get_file_content always takes the
model-routed branch; that branch called llm_router.afile_content directly, and
managed_files_obj.afile_content, the only caller that built the snapshot, is therefore
unreachable for batch output. CheckBatchCost spread the deployment credentials as plain
kwargs, and get_litellm_params does not carry s3_bucket_name across (gcs_bucket_name is
listed for exactly this reason, its S3 counterpart is not), so the poller lost the bucket
the same way
The result was that every completed Bedrock managed batch failed files.content with
"S3 bucket_name is required" and never had its cost tracked, leaving the row to be
re-polled every cycle. Both paths now resolve the deployment credentials and pass the
same MappingProxyType snapshot the managed-files hook already builds
* fix(proxy/batches): stop forwarding custom_llm_provider twice in list and cancel
The model-routing branches of list_batches and cancel_batch passed
custom_llm_provider as an explicit kwarg while also leaving it inside the dict
they splat, so every such call raised "got multiple values for keyword argument
'custom_llm_provider'" and returned a 500.
list_batches SCENARIO 2 called data.update(credentials) but never removed
custom_llm_provider before litellm.alist_batches(custom_llm_provider=..., **data);
it now uses prepare_data_with_credentials, the same helper the create and
retrieve branches already use, which pops it out.
cancel_batch SCENARIO 3 resolved the provider with
`provider or data.pop("custom_llm_provider", None) or ...`, so when the path
param provider was set the pop short-circuited and a body custom_llm_provider
stayed in data and collided with the explicit kwarg. The body value is now
popped unconditionally before the fallback chain, so the path param wins cleanly
and data no longer carries a duplicate.
Both paths already had strict-xfail regression tests documented "remove when
fixed"; those markers are dropped so the tests now guard the fix.
Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
* fix(proxy/files): avoid duplicate custom_llm_provider in list
Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
---------
Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
