* fix(proxy): serialize model reconciles so concurrent writes stop evicting each other
A model write is a read-modify-write of the shared `llm_router` global: read the
db into a snapshot, then make the router match that snapshot. Nothing serialized
it, so two of them interleaving was not a lost update but an eviction --
_delete_deployment removes every live deployment absent from the snapshot it was
handed, so the request holding the older snapshot reconciles the newer request's
model straight back out of the router. The row survives in the db, which is what
makes it easy to miss: the pod simply stops serving a model it was told to serve
until some later reload happens to put it back.
clear_cache compounds it. It deletes every db model from the router before
reloading them, so for the width of that reload the pod serves none of them --
and any concurrent write sampling the router in that window sees the hole.
Fix is one lock (MODEL_RECONCILE_LOCK) held across both, so each reconcile reads
the db and applies it atomically and no stale snapshot can evict a newer model.
clear_cache holds it across wipe+reload and calls the already-locked
_add_deployment_locked, since asyncio.Lock is not reentrant and routing back
through the public add_deployment would deadlock the pod's whole model-write
path.
The verdict needed the same treatment. raise_if_reload_degraded_serving compared
a desired-set read during the reload against a router snapshot taken after it,
so a neighbouring reconcile's in-flight wipe was reported to the caller as
collateral damage from its own reload -- a 500 on a create that had in fact
succeeded. Reconciles now return a ReconcileOutcome carrying both the desired set
and the post-reconcile serving state, captured before the lock is released, and
the verdict judges against that. Omitting live_after keeps the old live re-read,
which stays correct for the no-reconcile-ran case.
Found by running the e2e suite with pytest-xdist at 8 workers: three unrelated
tests failed together on "Previously served model id(s) [...] are also no longer
being served by this pod", which is this. Serial runs concurrent enough to hit it
are rare, which is why 78 minutes of sequential e2e never surfaced it -- but any
customer provisioning models in parallel (terraform, CI) is in exactly this race.
test_reconciles_serialize_so_no_stale_snapshot_can_evict fails with 5 == 1
without the lock.
* fix(tests): return a ReconcileOutcome from the PTU test's add_deployment mock
test_ptu_model_settings.py stubs proxy_config.add_deployment with
AsyncMock(return_value=None). Now that add_deployment returns a
ReconcileOutcome, add_new_model reads .still_desired off that None and
the two PTU gate tests fail with "'NoneType' object has no attribute
'still_desired'".
Return ReconcileOutcome(still_desired=None, live_after=None), matching
the other reconcile mocks. Both fields None means no reconcile state was
captured, so the serving verdict falls back to reading the router live,
which is what the test's mock_router already drives — the PTU assertions
are unchanged.
Two sibling test files were updated for this in the parent commit; this
one was missed because the local env cannot collect four modules under
tests/test_litellm/proxy (prisma generate artifacts), so the full shard
only ran in CI.
Also applies ruff format to proxy_server.py: the new add_deployment
wrapper's single call fits on one line under the project's line length.
* fix(proxy): lock the delete evictions and stop clear_cache wiping deployments
Two follow-ups to MODEL_RECONCILE_LOCK, both found by review.
1. delete_model and delete_team_models evict from llm_router directly,
outside the lock. The db row is gone by then, but a reconcile that
snapshotted the db BEFORE the delete still lists that id as desired and
upserts the deployment straight back, so the pod keeps serving a model
the database no longer has until some later reconcile notices. Taking
the lock orders the eviction after any in-flight reconcile's re-add.
Both new tests fail without the lock ("did not wait for
MODEL_RECONCILE_LOCK") and pass with it.
2. clear_cache no longer wipes deployments. It used to delete_deployment()
every db model before the reload restored them, which left the router
serving ZERO db models for the entire width of the reload -- every
inference request landing in that window fell into a real hole, and
serializing reconciles made the aggregate outage additive rather than
overlapping. The wipe was also redundant: _delete_deployment evicts
exactly the ids the db no longer lists, and upsert_deployment
pops-and-re-adds a deployment whose params changed while no-opping one
that did not, so the reconcile converges to the same state on its own.
Every mutation is visible to that comparison (blocked, and updated_at
for premium, are written into model_info).
The auto-router pops are NOT redundant and stay: they are keyed by
model_name, which no deployment-id reconcile touches.
The new tests patch their own lock rather than contending the module-level
one: asyncio.Lock binds to the event loop of its first contended acquire
and raises on every other loop after that, which would poison the next
asyncio test in the process. The proxy has a single event loop for its
lifetime so this is test-only, but it is a trap worth naming for whoever
writes the next concurrency test here.
* fix(proxy): scope the clear_cache wipe to auto-router deployments
Review caught a regression in the previous commit. Dropping the wipe
entirely stranded every db-backed auto-router on the pod.
The strategy registries (auto_routers, complexity_routers,
adaptive_routers, quality_routers) are keyed by model_name, which no
deployment-id reconcile touches, so clear_cache pops them and relies on
the reload to rebuild them. But the rebuild only happens on the ADD path:
Router.upsert_deployment returns early when a deployment is unchanged and
never reaches add_deployment -> _add_deployment ->
init_auto_router_deployment, which is what repopulates them. With the wipe
gone the deployment was always unchanged, so the pop was permanent: ANY
unrelated model write -- a team admin patching one team-owned model --
left every db-backed auto, complexity, adaptive and quality router
unroutable across tenants until a restart.
Restore the wipe for exactly the auto_router/* db deployments, whose
strategy entries are the ones being popped. Deleting them forces upsert
down the add path so both the deployment and its strategy entry come back.
Ordinary db models stay un-wiped, which is the point of the previous
commit: wiping them un-served every db model for the width of the reload,
and the reconcile converges without it.
test_clear_cache_wipes_auto_routers_but_leaves_ordinary_db_models pins
both halves against each other, since fixing either one naively breaks the
other. Both clear_cache tests fail with the pop-without-delete version.
* refactor(clear_cache): fold auto-router wipe into the classification pass
The auto-router scoping added in 5deddfd introduced two new mutable-collection
constructions, pushing LIT002 five over its budget ceiling.
Rather than suppress, do the work in the single pass that already walks
current_models: detect and delete the auto_router/* db deployments while
classifying, accumulating names into a set that replaces the old
db_router_deployments comprehension. Net-zero LIT002, same behaviour.
Comment updated to describe where the wipe actually happens now.
PostgreSQL rejects NUL in jsonb with 22P05, and the tags go into the managed
object's CREATE payload, so one poisoned tag aborts the whole row insert rather
than just that column. With no LiteLLM_ManagedObjectTable row, CheckBatchCost
never discovers the batch, so a batch that really ran and billed at the provider
produces no spend at all. The create-time write is fire and forget, so nothing
retries it.
This regressed in #36468, which started passing request_tags and
persist_attribution from the Anthropic passthrough; before that no
caller-supplied string reached the column.
Sanitize in the shared helper that builds the value, matching how
spend_tracking_utils already handles LiteLLM_SpendLogs.request_tags. Both the
Anthropic and the Vertex passthrough build tags through that one helper, so this
covers both. Rename it to _sanitized_str_tuple since it no longer merely
coerces.
* fix(proxy): expand config-defined model access groups when resolving team models for /v2/model/info
Teams whose only model grant is a config-defined access group (a model_info.access_groups
name listed in team.models) got an empty /v2/model/info?include_team_models=true result.
_add_team_models_to_all_models passed each team.models entry straight to
llm_router.get_model_list(model_name=...), which never matches an access-group name, so
the group's member deployments were dropped. Runtime auth and /v1/models were unaffected
because they expand team.models through get_team_models first.
Resolve team.models through the same get_team_models resolver before iterating, reusing the
exact path runtime auth and /v1/models trust so the two can't drift again. The get_model_names
and get_model_access_groups accessors are hoisted above the team loop so they run once.
* fix(proxy): keep a literal model whose name collides with an access-group name in listings
A grant string that names both a deployed model and a config access group grants
BOTH at runtime (_check_model_access_helper unions them), but the listing resolver
dropped the literal and substituted the group members, hiding a callable model from
/v1/models and /v2/model/info. Keep the literal when it is also a deployed model so
listings match runtime access exactly. Pure-group names (no collision) are still
replaced by their members. Also rewrites _get_models_from_access_groups to build
its result without mutating the input list.
Addresses the Greptile P1 on this PR.
* fix(proxy): type proxy_model_list param as Sequence to satisfy LIT001 budget
A request whose body never parses is rejected in auth, before the endpoint
runs, so nothing downstream fires the failure hook that writes the spend log
row Request Logs reads. The caller sees a 400 that leaves no trace.
Auth now records that rejection through the same post_call_failure_hook the
endpoints use, keyed to the caller it already authenticated. Logging is
best-effort: a logging failure is swallowed so the 400 the caller sees is
unchanged. The path where the key is also rejected is left alone, since the
auth failure handler already logs that request.
An upstream that ends its response stream without a JSON-RPC reply leaves the
request pending forever. Tool discovery then only ended when an outer cancel
scope killed it, which logged a cancelled list_tools, ignored the timeout the
operator configured, and reported no tools to the client. Prompts and resources
had no outer guard at all.
Give the client session a read timeout so every request it sends is bounded,
including initialize. The SDK reports its own elapsed timeout as an McpError
carrying an HTTP status code in the field that otherwise holds JSON-RPC error
codes, and it relays an upstream's JSON-RPC error through that same class and
field, so the code alone cannot separate the two: an upstream answering with
application code 408 would be blamed on the gateway as a 504. Translate the
SDK's timeout into a TimeoutError in the module that configures the timeout,
matching on the elapsed timeout in the exception's context chain rather than on
the number, so the listing taxonomy never has to read a JSON-RPC code as an HTTP
status and every caller gets the same signal.
The bare cancellation warning is replaced by a line naming the server and the
budget that elapsed, and quiet_on_error does not demote it.
A successful pass-through request left its pre-call budget reservation in
the shared Redis spend counter. `_init_kwargs_for_pass_through_endpoint`
built the request metadata from the sanitized key fields only, so
`_PROXY_track_cost_callback` resolved `budget_reservation = None` and
`increment_spend_counters` added the actual cost on top of a reservation
nobody released. The counter drifted above real spend on every request
until the key falsely tripped BudgetExceededError, while the Postgres
spend stayed far below the limit. The failure path was unaffected because
it releases `user_api_key_dict.budget_reservation` directly.
The reservation is now set alongside the other internal keys, after the
client-supplied metadata merge, so a request body cannot forge one that
names arbitrary counter keys.
Python keeps only the last binding for a name, so when a file defines the same
test twice the earlier one is unreachable. pytest cannot collect a function that
no longer exists, so nothing reports it and the file still looks like it covers
the scenario.
These ten are cases where the two definitions have different bodies, meaning a
real test was replaced rather than duplicated. Each is renamed to say what it
actually covers, which makes it reachable again:
- test_gemini_frequency_penalty: the dead copy checks the parameter is listed in
get_supported_openai_params for vertex_ai; the survivor checks get_optional_params
maps a value for gemini. Different function and different provider.
- test_async_log_success_event_adds_to_queue and the failure variant: the dead
copies run without mocking asyncio.create_task, so they exercise the real task
path the survivors mock out.
- test_async_send_batch_triggers_tasks: the dead copy asserts send is not awaited
directly; the survivor asserts create_task was called.
- test_model_id_in_required_metrics: the dead copy checks the model_id label on
twelve further metrics the survivor dropped.
- test_anthropic_messages_pt_file_block_preserves_cache_control: the dead copy
passes model and llm_provider explicitly and uses real base64 PDF content.
- test_translate_streaming_openai_chunk_to_anthropic_with_thinking: the dead copy
covers thinking_delta; the survivor covers signature_delta.
- test_client_initialization and test_client_without_api_key: the dead copies
assert the resource clients are wired with the right base URL and key; the
survivors only construct the object.
- test_client_initialization_strips_trailing_slash: the dead copy constructs
ModelsManagementClient directly rather than going through Client.
Verification: collecting the seven touched files gives 401 node IDs before and
411 after, the ten new names and nothing else, with nothing lost. All ten pass.
Running the touched files in full gives 299 passed, and test_optional_params.py
goes from 111 passed to 112.
Two further shadowed definitions were left alone rather than renamed: the dead
copies of test_prompt_caching and test_cost_calculator_with_base_model_with_router
have no assertions at all, one being a bare pass and the other a lone import, so
restoring them would add tests that cannot fail.
Three groups, all verified by running the suite rather than by inspection.
18 files whose every test function carries an unconditional @pytest.mark.skip,
39 test functions in total. They are collected on every CI run and always skip,
so they advertise coverage the suite does not have. Reasons on the marks include
"AWS Suspended Account", "lakera deprecated their v1 endpoint" and "moved to
using 'otel' for logging"; 26 of the marks predate 2025.
30 test functions with a byte-identical body and identical decorators to a
sibling in the same file and class, differing only in name. Deleting one of each
pair removes no coverage. Four further candidates were excluded because they
override an inherited test, where deleting the override un-shadows the base
class implementation instead of removing a duplicate.
9 test functions that a later definition of the same name shadows, so Python
never binds them and pytest cannot collect them.
One file that is a demo script rather than a test; its own docstring says to run
it with python.
Verification: collecting the 26 edited files gives 2,492 node IDs before and
2,462 after. The 30 duplicate deletions account for exactly 30 removals, the 9
shadowed deletions account for 0 (confirming at runtime that they were never
collectable), nothing unexplained disappeared, and nothing new appeared. No
other test or module imports any deleted symbol.
APScheduler anchors an interval job at now + interval, so every scheduled
background job registered in one proxy startup shares a single firing instant
for the life of the process, and every replica a rollout brought up together
shares that instant too. Each tick the spend flushes, budget reset sweep,
config-in-DB reload, credential reload and cost pollers all hit Postgres at the
same moment, on every pod, competing with request-path auth and budget queries.
Shift each eligible job by a deterministic offset derived from
sha256(job_id, identity), where identity covers the pod and the worker process.
The offset lives in the trigger rather than in a one-off next_run_time, because
a cron trigger recomputes each fire from the wall clock and would otherwise snap
straight back onto the shared instant. An interval job is never offset by more
than one of its own periods.
Only schedules LiteLLM chose are shifted: interval jobs always, cron jobs only
when the id is one of the product's own defaults, so an operator-supplied
crontab keeps the instant it asks for. general_settings.scheduled_job_stagger
turns it off, widens the window, replaces the identity, or pins a job. The
applied offsets are logged once at startup and each fire logs its scheduled
instant against its actual start.
Resolves LIT-5433
A capped pre-wait still burns the first daily pass when the router takes
longer than the cap to appear (a >10 minute boot), and reads the router
in two places. Folding the poll into the loop makes the first alert
unconditional on boot duration and keeps a single read per pass.
The comment restated what the gate does and carried incident detail that would drift,
including a claim about downstream callbacks that the evidence does not support. The
rationale belongs in the regression test, which fails if the copy is ever reintroduced
ahead of the gate, rather than in prose that can rot silently
Also corrects that test's docstring for the same overclaim: the raise aborts the handler
body at the redaction call, and what that costs a given deployment was not established
perform_redaction deepcopies the result before inspecting it, but every shape it does not
recognize falls through to the placeholder return at the end of that block, so the copy is
built and then discarded. Binary and HTTP response bodies land in exactly that case: batch
output, file content and audio responses hold an unpicklable `_thread.lock`, so
copy.deepcopy raises TypeError
The raise lands inside the try in Logging.success_handler that also wraps the callback
loop, so the handler body aborts at the redaction call and everything after it is skipped.
It surfaces only as "[Non-Blocking] Exception occurred while success logging cannot pickle
'_thread.lock' object", which is why it can run unnoticed. The async handler body reaches
perform_redaction the same way. Only deployments with message redaction enabled are
affected, since perform_redaction runs only when turn_off_message_logging resolves true
Deciding redactability before copying fixes the crash as a consequence rather than catching
it, and keeps the deepcopy off large batch bodies it was never going to help. Behaviour for
every recognized shape is unchanged: the copy still shields the caller's object from
in-place redaction
Observed on a live gateway with turn_off_message_logging enabled, where every managed-batch
output download logged that error; after this change the error no longer appears
/v1/messages and other litellm_metadata endpoints store proxy metadata,
including x-litellm-tags header tags, under litellm_metadata instead of
metadata. The pre-routing hook read request tags with a hardcoded
metadata bucket, so it never saw the tags that selected the marker and
cleared the consumed-tags stamp, and tag filtering then 401'd the routed
tier. Resolve the bucket from the request kwargs instead, matching how
the stamp write and the tag-filter read already resolve it.
The outer wrap_sse_stream_with_keepalive_pings layer duplicated the
keepalive engine that PR #34423 already runs inside async_data_generator
for chat completions and responses streams, and it kept pinging
deployments whose operator set keepalive_seconds: 0 as a hard disable.
sse_keepalive_ping_interval_seconds is now the global fallback inside
_resolve_keepalive_seconds, so deployment and request values keep
precedence, an explicit 0 still disables, the [1, 300]s clamp applies,
and router-less proxies arm the wrap when the global default is set.
The Rust messages bridge logs a parsed Anthropic response without an
httpx_response, so the fallback transform dropped the request speed and
billed fast-mode calls at the standard rate. Thread optional_params
speed into transform_parsed_response and add a regression test for the
parsed-response branch.
A stream can terminate with a response.failed event that still reports
consumed tokens; those were rebuilt as None and logged at zero spend.
Parse response.failed alongside completed and incomplete, matching the
buffered path, which prices any terminal response that reports usage.
