fix(proxy): stop model writes 500ing on another pod's delete (#35400)

* fix(proxy): stop model writes 500ing on another pod's delete

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.

* test(proxy): fold reload-verdict test commentary into docstrings and 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.

* test: fix clear_cache mock return type in model block/unblock tests
This commit is contained in:
ryan-crabbe-berri 2026-07-31 18:10:48 -07:00 committed by GitHub
parent b5cfc2ca00
commit b4ff05be8e
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
7 changed files with 158 additions and 41 deletions

View file

@ -324,7 +324,7 @@ async def patch_model(
# Clear cache and reload models (uses config setting or defaults to preserving config models for DB updates)
live_before_reload = live_model_ids_snapshot()
await clear_cache()
still_desired_ids = await clear_cache()
## CREATE AUDIT LOG ##
asyncio.create_task(
@ -344,6 +344,7 @@ async def patch_model(
before=live_before_reload,
written_models=[(model_id, getattr(updated_model, "model_info", None))],
action="update",
still_desired=still_desired_ids,
)
return updated_model
@ -429,7 +430,7 @@ async def _set_model_blocked_status(
)
live_before_reload = live_model_ids_snapshot()
await clear_cache()
still_desired_ids = await clear_cache()
asyncio.create_task(
create_object_audit_log(
@ -450,6 +451,7 @@ async def _set_model_blocked_status(
before=live_before_reload,
written_models=[(data.model_id, getattr(updated_model, "model_info", None))],
action=action,
still_desired=still_desired_ids,
)
return updated_model
@ -1355,6 +1357,7 @@ async def add_new_model(
"""
live_before_reload = live_model_ids_snapshot()
still_desired_ids: frozenset[str] | None = None
try:
_original_litellm_model_name = model_params.model_name
if model_params.model_info.team_id is None:
@ -1369,7 +1372,9 @@ async def add_new_model(
user_api_key_dict=user_api_key_dict,
prisma_client=prisma_client,
)
await proxy_config.add_deployment(prisma_client=prisma_client, proxy_logging_obj=proxy_logging_obj)
still_desired_ids = await proxy_config.add_deployment(
prisma_client=prisma_client, proxy_logging_obj=proxy_logging_obj
)
# don't let failed slack alert block the /model/new response
_alerting = general_settings.get("alerting", []) or []
if "slack" in _alerting:
@ -1414,6 +1419,7 @@ async def add_new_model(
before=live_before_reload,
written_models=[(model_response.model_id, getattr(model_response, "model_info", None))],
action="create",
still_desired=still_desired_ids,
)
return model_response
@ -1542,7 +1548,7 @@ async def update_model(
# Clear cache and reload models (uses config setting or defaults to preserving config models for DB updates)
live_before_reload = live_model_ids_snapshot()
await clear_cache()
still_desired_ids = await clear_cache()
## CREATE AUDIT LOG ##
asyncio.create_task(
create_object_audit_log(
@ -1569,6 +1575,7 @@ async def update_model(
before=live_before_reload,
written_models=[(_model_id, getattr(model_response, "model_info", None))],
action="update",
still_desired=still_desired_ids,
)
return model_response
@ -1814,6 +1821,7 @@ def reload_serving_verdict(
before: frozenset[str],
written_models: Sequence[tuple[str, object]],
written_must_serve: bool,
still_desired: frozenset[str] | None = None,
) -> tuple[tuple[str, ...], tuple[str, ...]]:
"""Judge a write-triggered reload by diffing the router's serving state instead of
trusting any layer of the reload stack to report its own failure.
@ -1828,10 +1836,14 @@ def reload_serving_verdict(
this write and blaming it would block unrelated metadata fixes
- not written but live before and gone now: collateral degradation of this pod
caused by the reload this request triggered (a wholesale re-add failure, or a
newly introduced conflict), always reported
newly introduced conflict), reported only when the db still wants that id
``still_desired`` is the db + config id set the reload just reconciled against. An
id absent from it was deleted on purpose, most often by another pod this one had not
yet polled, so the reload dropping it is the reconcile working rather than damage.
Without it (no reconcile ran) every drop is reported, which is the safe direction.
Returns (written ids violating their obligation, collateral ids no longer served).
Best effort under concurrent admin writes: the snapshot spans only this request.
"""
now = live_model_ids_snapshot()
written_ids = frozenset(model_id for model_id, _ in written_models)
@ -1843,7 +1855,8 @@ def reload_serving_verdict(
)
else:
missing = tuple(model_id for model_id, _ in written_models if model_id in before and model_id not in now)
collateral = tuple(sorted(before - now - written_ids))
dropped = before - now - written_ids
collateral = tuple(sorted(dropped if still_desired is None else dropped & still_desired))
return (missing, collateral)
@ -1851,12 +1864,18 @@ def raise_if_reload_degraded_serving(
before: frozenset[str],
written_models: Sequence[tuple[str, object]],
action: str,
still_desired: frozenset[str] | None = None,
) -> None:
"""The caller-visible error this pod's model-write endpoints owe their caller when
the model they wrote is not being served after the reload they triggered. The DB
write is durable either way and every other pod reloads on its own interval; this
speaks only for the handling pod."""
missing, collateral = reload_serving_verdict(before=before, written_models=written_models, written_must_serve=True)
missing, collateral = reload_serving_verdict(
before=before,
written_models=written_models,
written_must_serve=True,
still_desired=still_desired,
)
if not missing and not collateral:
return
missing_clause = (
@ -1882,9 +1901,12 @@ def raise_if_reload_degraded_serving(
)
async def clear_cache():
async def clear_cache() -> frozenset[str] | None:
"""
Clear router caches and reload models.
Returns the db + config id set the reload reconciled against, or None when no
reload ran, so callers can pass it to raise_if_reload_degraded_serving.
"""
from litellm.proxy.proxy_server import (
llm_router,
@ -1896,7 +1918,7 @@ async def clear_cache():
if llm_router is None or prisma_client is None:
verbose_proxy_logger.debug("llm_router or prisma_client is None, skipping cache clear")
return
return None
try:
# Only clear DB models, preserve config models
@ -1943,10 +1965,14 @@ async def clear_cache():
llm_router.quality_routers.pop(model_name, None)
# Reload only DB models
await proxy_config.add_deployment(prisma_client=prisma_client, proxy_logging_obj=proxy_logging_obj)
still_desired_ids = await proxy_config.add_deployment(
prisma_client=prisma_client, proxy_logging_obj=proxy_logging_obj
)
verbose_proxy_logger.debug(
f"Cleared {len(db_model_ids)} DB models, preserved {len(config_models)} config models"
)
return still_desired_ids
except Exception as e:
verbose_proxy_logger.exception(f"Failed to clear cache and reload models. Due to error - {str(e)}")
return None

View file

@ -5300,7 +5300,7 @@ class ProxyConfig:
_model_info = RouterModelInfo(id=model.model_id, db_model=db_model)
return _model_info
async def _delete_deployment(self, db_models: list) -> int:
async def _delete_deployment(self, db_models: list) -> frozenset[str] | None:
"""
(Helper function of add deployment) -> combined to reduce prisma db calls
@ -5309,14 +5309,16 @@ class ProxyConfig:
- Remove any that are missing
Return:
- int - returns number of deleted deployments
- frozenset[str] - the ids the db + config say should be served after this
reconcile, so a caller can tell an id this evicted on purpose from one that
went missing. None when no reconcile ran and that set is therefore unknown.
"""
global user_config_file_path, llm_router
combined_id_list = []
## BASE CASES ##
if llm_router is None:
return 0
return None
# NOTE: db_models may be legitimately empty when all DB models have been deleted.
# Do NOT short-circuit on len(db_models) == 0 — we must still evict any
# DB-sourced deployments that are no longer in the DB. The caller
@ -5337,7 +5339,7 @@ class ProxyConfig:
"Skipping deployment cleanup to avoid removing valid models.",
str(e),
)
return 0
return None
model_list = config.get("model_list", None)
if model_list:
for model in model_list:
@ -5361,13 +5363,10 @@ class ProxyConfig:
router_model_ids = llm_router.get_model_ids()
# Check for model IDs in llm_router not present in combined_id_list and delete them
deleted_deployments = 0
for model_id in router_model_ids:
if model_id not in combined_id_list:
is_deleted = llm_router.delete_deployment(id=model_id)
if is_deleted is not None:
deleted_deployments += 1
return deleted_deployments
llm_router.delete_deployment(id=model_id)
return frozenset(combined_id_list)
def _resolve_db_litellm_param(self, key: str, value: object) -> object:
if not isinstance(value, str):
@ -5452,9 +5451,11 @@ class ProxyConfig:
self,
new_models: Optional[Json],
proxy_logging_obj: ProxyLogging,
):
) -> frozenset[str] | None:
global llm_router, llm_model_list, master_key, general_settings
still_desired_ids: frozenset[str] | None = None
# Load config separately so a timeout here doesn't block model loading
config_data: dict = {}
search_tools = None
@ -5501,7 +5502,7 @@ class ProxyConfig:
if search_tools is not None and llm_router is not None:
llm_router.search_tools = search_tools
## DELETE MODEL LOGIC
await self._delete_deployment(db_models=models_list)
still_desired_ids = await self._delete_deployment(db_models=models_list)
## ADD MODEL LOGIC
self._add_deployment(db_models=models_list)
@ -5527,6 +5528,8 @@ class ProxyConfig:
proxy_logging_obj=proxy_logging_obj,
)
return still_desired_ids
def _add_callback_from_db_to_in_memory_litellm_callbacks(
self,
callback: str,
@ -6159,14 +6162,20 @@ class ProxyConfig:
self,
prisma_client: PrismaClient,
proxy_logging_obj: ProxyLogging,
):
) -> frozenset[str] | None:
"""
- Check db for new models
- Check if model id's in router already
- If not, add to router
Returns the ids the db + config say should be served after the reconcile, or
None when no reconcile ran. Callers that judge their own reload need it to tell
a deliberate eviction from a deployment that went missing.
"""
global llm_router, llm_model_list, master_key, general_settings
still_desired_ids: frozenset[str] | None = None
try:
# warm the config cache so the per-param reads below all hit
await prefetch_config_params(
@ -6186,7 +6195,9 @@ class ProxyConfig:
new_models = await self._get_models_from_db(prisma_client=prisma_client)
# update llm router
await self._update_llm_router(new_models=new_models, proxy_logging_obj=proxy_logging_obj)
still_desired_ids = await self._update_llm_router(
new_models=new_models, proxy_logging_obj=proxy_logging_obj
)
db_general_settings = await get_config_param(prisma_client, "general_settings")
@ -6204,6 +6215,8 @@ class ProxyConfig:
"litellm.proxy.proxy_server.py::ProxyConfig:add_deployment - {}".format(str(e))
)
return still_desired_ids
async def _init_non_llm_objects_in_db(self, prisma_client: PrismaClient):
"""
Use this to read non-llm objects from the db and initialize them

View file

@ -839,7 +839,7 @@ class TestUpdateModel:
),
patch(
"litellm.proxy.management_endpoints.model_management_endpoints.clear_cache",
new=AsyncMock(return_value=True),
new=AsyncMock(return_value=None),
) as mock_clear_cache,
):
await update_model(
@ -3223,7 +3223,7 @@ class TestPatchModelBlockedAuthGate:
),
patch(
"litellm.proxy.management_endpoints.model_management_endpoints.clear_cache",
new=AsyncMock(return_value=True),
new=AsyncMock(return_value=None),
),
):
result = await patch_model(
@ -3314,6 +3314,72 @@ class TestWriteSurfacesReloadDrop:
before=frozenset({"m-live", "m-collateral"}), written_models=[("m-live", None)], action="update"
)
def test_a_model_the_db_no_longer_has_is_not_collateral(self, monkeypatch):
"""Another pod deleting a model is not this pod's reload breaking.
A pod that has not yet polled the delete still lists the id when the write
snapshots `before`; the reload it triggers then evicts the id because the db no
longer has it. That eviction is the reconcile working, so it must not fail the
write. `still_desired` is the db + config set the reload reconciled against, so
an id missing from it drops out of the collateral diff.
The cases below, in order: an id the db no longer wants is not collateral and the
write succeeds; an id the db still wants that stopped serving is real degradation
and still raises, so a genuinely broken reload is caught; and with no reconcile at
all the desired set is unknown, so every drop is reported.
"""
import litellm
from litellm.proxy._types import ProxyException
from litellm.proxy.management_endpoints.model_management_endpoints import (
raise_if_reload_degraded_serving,
reload_serving_verdict,
)
live_router = litellm.Router(
model_list=[
{
"model_name": "gpt-4o",
"litellm_params": {"model": "gpt-4o"},
"model_info": {"id": "m-live", "db_model": True},
}
]
)
monkeypatch.setattr("litellm.proxy.proxy_server.llm_router", live_router)
_, collateral = reload_serving_verdict(
before=frozenset({"m-live", "m-deleted-elsewhere"}),
written_models=[("m-live", None)],
written_must_serve=True,
still_desired=frozenset({"m-live"}),
)
assert collateral == ()
assert (
raise_if_reload_degraded_serving(
before=frozenset({"m-live", "m-deleted-elsewhere"}),
written_models=[("m-live", None)],
action="create",
still_desired=frozenset({"m-live"}),
)
is None
)
with pytest.raises(ProxyException, match="m-should-be-serving"):
raise_if_reload_degraded_serving(
before=frozenset({"m-live", "m-should-be-serving"}),
written_models=[("m-live", None)],
action="create",
still_desired=frozenset({"m-live", "m-should-be-serving"}),
)
with pytest.raises(ProxyException, match="m-deleted-elsewhere"):
raise_if_reload_degraded_serving(
before=frozenset({"m-live", "m-deleted-elsewhere"}),
written_models=[("m-live", None)],
action="create",
still_desired=None,
)
class TestModelInfoAsMapping:
"""The model_info column reaches consumers as a dict or as its JSON string; this is

View file

@ -1404,12 +1404,12 @@ def test_ProxyConfig_get_model_info_with_id_missing_model_id_raises(monkeypatch)
@pytest.mark.asyncio
async def test_ProxyConfig__delete_deployment_empty_returns_zero(monkeypatch):
async def test_ProxyConfig__delete_deployment_no_router_returns_none(monkeypatch):
monkeypatch.setattr("litellm.proxy.proxy_server.llm_router", None)
pc = ProxyConfig()
result = await pc._delete_deployment(db_models=[])
snapshot = {"deleted": result, "router_was": "none", "empty_db_models": True}
assert snapshot == {"deleted": 0, "router_was": "none", "empty_db_models": True}
snapshot = {"still_desired": result, "router_was": "none", "empty_db_models": True}
assert snapshot == {"still_desired": None, "router_was": "none", "empty_db_models": True}
@pytest.mark.asyncio

View file

@ -2459,13 +2459,11 @@ async def test_delete_deployment_type_mismatch():
patch("litellm.proxy.proxy_server.user_config_file_path", "test_config.yaml"),
):
# Call the function under test
deleted_count = await pc._delete_deployment(db_models=[])
still_desired = await pc._delete_deployment(db_models=[])
# The two SHA-hash models have no corresponding entry in combined_id_list
# and must be evicted.
assert (
deleted_count == 2
), f"Expected 2 deletions (SHA-hash models), got {deleted_count}"
assert len(deleted_ids) == 2, f"Expected 2 deletions (SHA-hash models), got {deleted_ids}"
assert (
"a96e12e76b36a57cfae57a41288eb41567629cac89b4828c6f7074afc3534695"
in deleted_ids
@ -2485,6 +2483,12 @@ async def test_delete_deployment_type_mismatch():
"12345679" not in deleted_ids
), f"Model 12345679 should NOT be deleted. Deleted IDs: {deleted_ids}"
assert still_desired is not None
assert {"12345678", "12345679"} <= still_desired, (
"the int-keyed config models must come back as strings in the desired set, so a "
f"caller judging its own reload reads them as wanted rather than evicted; got {still_desired}"
)
@pytest.mark.asyncio
async def test_get_config_from_file(tmp_path, monkeypatch):
@ -9119,10 +9123,13 @@ class TestDeleteDeploymentSync:
with patch.object(
proxy_config, "get_config", AsyncMock(return_value={"model_list": []})
):
count = await proxy_config._delete_deployment(db_models=[])
still_desired = await proxy_config._delete_deployment(db_models=[])
mock_router.delete_deployment.assert_called_once_with(id="model-id-to-evict")
assert count == 1
assert still_desired == frozenset(), (
"an empty db and an empty config want nothing, which must stay distinct from "
f"the None returned when no reconcile ran at all; got {still_desired}"
)
@pytest.mark.asyncio
async def test_update_llm_router_skips_update_on_db_fetch_failure(self):

View file

@ -115,8 +115,10 @@ class TestDeleteDeploymentResilience:
"""Test _delete_deployment handles get_config failures gracefully."""
@pytest.mark.asyncio
async def test_returns_zero_when_get_config_times_out(self):
"""Should return 0 (no deletions) when get_config fails, not raise."""
async def test_returns_none_when_get_config_times_out(self):
"""Should return None (no reconcile ran, desired set unknown) when get_config
fails, not raise. A caller judging its own reload must not read that as "the db
wants nothing" and blame the reload for every model it serves."""
proxy_config = ProxyConfig()
db_models = [_make_db_model("gpt-5.1", "db-id-1")]
@ -136,8 +138,8 @@ class TestDeleteDeploymentResilience:
):
result = await proxy_config._delete_deployment(db_models=db_models)
# Should safely return 0 instead of raising
assert result == 0
# Should safely return None instead of raising
assert result is None
# Should NOT have deleted any deployments
mock_router.delete_deployment.assert_not_called()
@ -175,5 +177,8 @@ class TestDeleteDeploymentResilience:
result = await proxy_config._delete_deployment(db_models=db_models)
# "stale-id" should have been deleted (not in db_models or config)
assert result == 1
mock_router.delete_deployment.assert_called_once_with(id="stale-id")
assert result == frozenset({"db-id-1", "config-id-1"}), (
"the returned set must be what the db + config still want, so a caller can "
f"tell that eviction apart from a deployment that went missing; got {result}"
)

View file

@ -36,7 +36,7 @@ def _setup_model_block_mocks(monkeypatch, *, updated_blocked: bool):
mock_router = MagicMock()
mock_router.get_model_ids.return_value = [model_id]
mock_clear_cache = AsyncMock(return_value=True)
mock_clear_cache = AsyncMock(return_value=None)
mock_audit_log = AsyncMock(return_value=None)
monkeypatch.setattr("litellm.proxy.proxy_server.prisma_client", mock_prisma_client)