fix(proxy): stop model writes 500ing on another pod's delete - #35400
Conversation
A model write judges the reload it triggers by diffing this pod's router before and after, and reports anything that stopped serving as damage. On a pod that has not yet polled a delete another pod made, the snapshot still lists that model; the reload then evicts it because the db no longer has it, and the guard reads its own correct reconcile as degradation. The row is written and served, but the caller gets a 500. Since propagation between pods is a 30s db poll, any delete followed by a create inside that window can land on a pod that has not caught up, so a delete-then-create pair returns 500 whenever the two requests hit different pods. _delete_deployment already computes exactly the set that settles it: the ids the db and config still want. Thread it up through _update_llm_router, add_deployment and clear_cache to the verdict, and intersect the drop set with it so an id the db no longer has stops counting as collateral. Where no reconcile ran the set is None and every drop is still reported, so a genuinely broken reload is caught as before. _delete_deployment now returns that set instead of a delete count; the count had no callers in the proxy, and the tests asserting it already assert the eviction calls.
Greptile SummaryThe PR prevents model-write endpoints from treating deliberate cross-pod model eviction as reload degradation
Confidence Score: 5/5The PR appears safe to merge No blocking failure remains
|
| Filename | Overview |
|---|---|
| litellm/proxy/management_endpoints/model_management_endpoints.py | Propagates reconciled model IDs to reload verdicts and excludes intentionally deleted models from collateral degradation |
| litellm/proxy/proxy_server.py | Returns the desired deployment ID set from reconciliation while preserving None when reconciliation does not run |
| tests/test_litellm/proxy/management_endpoints/test_model_management_endpoints.py | Adds focused regression coverage and resolves the prior commentary concern by placing necessary context in the test docstring |
| tests/test_litellm/proxy/proxy_server/test_proxy_config.py | Updates reconciliation tests to verify the desired-ID return contract |
| tests/test_litellm/proxy/test_proxy_server.py | Preserves deployment-eviction assertions while validating desired IDs and empty reconciliation results |
| tests/test_litellm/proxy/test_update_llm_router_resilience.py | Verifies reconciliation failures return an unknown desired set and successful reconciliation returns DB and config IDs |
Reviews (3): Last reviewed commit: "test: fix clear_cache mock return type i..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…assertions Greptile flagged the inline comments against the repo's no-new-comments rule. The case-by-case context moves into the test docstring, and the two return-contract assertions carry their reasoning as failure messages instead.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
@greptileai re review |
|
QA'd this before and after in a two-pod setup, since the race needs a second pod Two proxy processes on 4001 and 4002 share one Postgres, which is what two replicas behind one Service look like to the db. Cross-pod delete, then The 500 names the id another pod deleted, while the model the caller actually wrote is in The update path goes through For the direction that must not change, I broke a reload for real by corrupting a row the db still wants ( So the guard still fails the write when a model the db still wants stops being served, and only the deliberate cross-pod eviction stops counting
|
… of a delete count _delete_deployment stopped returning a count of evictions in BerriAI#35400 and now returns the frozenset of ids the db and config still want, so a caller judging its own reload can tell a deliberate eviction from a deployment that went missing. These two tests in tests/local_testing were left comparing that frozenset against an int and have been failing since; the directory is only referenced by .circleci/config.yml, which no longer reports checks on PRs, so nothing caught them. The eviction behavior itself is unchanged, so the fix is on the assertions: compare against the expected id set, and pin the router's surviving ids so a mutation that evicts the wrong deployment is caught rather than passing a bare length check.
2 participants
TLDR
High level flow for the user:
High level flow on a technical level:
Relevant issues
Linear ticket
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
The failure is a race between two pods, so the honest proof is the stage occurrence plus a test that reproduces the exact id sets. A single-pod local proxy cannot hit it: the delete and the create land on the same process, so the router is already correct when the create snapshots it, which is also why the same test passes on some stage runs and fails on others
From the stage e2e run on 2026-07-30 12:42 UTC, ns
litellm, at stage image commit38f2e023f1. The model was created and deleted by the suite minutes apart, and the delete landed on a different backend pod than the create:0.68 seconds separate the delete from the 500. Pod
55jwchad not yet polled the delete, so itsbeforesnapshot still listeda50fa33f; the reload the create triggered read the db, correctly evicted the row, and the guard reported that eviction as collateral damage. The written model itself was live the whole time, which is why the message uses the generic "degraded this pod's serving state" clause rather than naming a model that failed to serveThe adjacency is the discriminator, and it holds across runs. From 2026-07-29 12:32Z, same signature, 200 when the pair shares a pod and 500 when it does not:
There is no reload failure behind any of these. Paging every non-OTel log line on the affected pods across the failure windows returns exactly one ERROR each, the guard's own exception; no
Error upserting deployment, noError creating deployment, no traceback from the reload stackThe new test reproduces the id sets directly and fails without the fix:
Full runs at commit
e8fc9a92f3:(One unrelated pre-existing failure,
test_mcp_token_opens_sealed_passthrough_code_and_exchanges_with_minted_client, reproduces with these source changes reverted; it readsPROXY_BASE_URLfrom a local.envand is not present in CI.)ruff checkandruff format --checkpass on both changed source filesType
🐛 Bug Fix
Changes
_delete_deploymentalready builds the set that settles this: the ids the db and config still want after reconciling (proxy_server.py). It used that set to evict and then discarded it, returning a delete count no proxy caller read. It now returns the set instead, and_update_llm_router,add_deploymentandclear_cachepass it up to the model-write endpoints, which hand it toraise_if_reload_degraded_servingasstill_desiredreload_serving_verdictintersects the drop set with it:dropped if still_desired is None else dropped & still_desired. An id the db no longer has was deleted deliberately, so the reload dropping it is the reconcile working rather than damage. Where no reconcile ran the set is None and every drop is reported exactly as before, so a genuinely broken reload still fails the write; there is a test for that caseAll four guard call sites are covered (create, the two update paths, and the patch path), because they all reach the reload through
add_deploymentand all share the same raceBackwards-incompatible detail, flagging explicitly:
ProxyConfig._delete_deploymentnow returnsfrozenset[str] | Noneinstead ofint. It is private, its single proxy caller ignored the return, and the tests that asserted the count already assert the eviction calls themselves, so the coverage they provided is preserved.add_deploymentandclear_cachepreviously returnedNoneimplicitly, so gaining a return value is additive and every existing caller is unaffectedNot addressed here, deliberately: the 30-second propagation delay itself. Other pods still learn about a write on their own poll interval, and a fixed sleep in the e2e harness was considered and rejected since it would have added roughly half an hour to the suite while leaving the customer-facing 500 in place