mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-10 22:41:41 +00:00
fix(router): ignore a retry skip list the caller sent itself
The retry skip travels as a request kwarg, and the router forwards keys it does not recognize, so a client can put _retry_skipped_deployment_ids in its own request body. The value went straight into a pydantic TypeAdapter and then into a set(), so an int or an object raised TypeError and a string, a list, or a dict raised a ValidationError, each of them replacing the 400 the provider had actually returned. Every read now goes through one narrowing function that keeps a tuple of strings and skips nothing otherwise, so a forged value costs the caller nothing beyond the retry landing on the same deployment again.
This commit is contained in:
parent
ecf7e4e766
commit
6866eac96f
2 changed files with 46 additions and 4 deletions
|
|
@ -401,7 +401,10 @@ def _stream_chunks_have_generated_content(chunks: Sequence[ModelResponseStream])
|
|||
|
||||
_NO_SESSION_KWARGS: Final[Mapping[str, Mapping[str, object]]] = MappingProxyType({})
|
||||
_SESSION_ADAPTER: Final = TypeAdapter(Mapping[str, object])
|
||||
_SKIPPED_DEPLOYMENT_IDS_ADAPTER: Final = TypeAdapter(tuple[str, ...])
|
||||
|
||||
|
||||
def _as_retry_skipped_deployment_ids(value: object) -> tuple[str, ...]:
|
||||
return tuple(item for item in value if isinstance(item, str)) if isinstance(value, tuple) else ()
|
||||
|
||||
|
||||
def _with_router_resolved_session_model(session: object, model_name: str) -> Mapping[str, Mapping[str, object]]:
|
||||
|
|
@ -7467,7 +7470,7 @@ class Router:
|
|||
return ()
|
||||
if litellm._should_retry(status_code): # pyright: ignore[reportPrivateUsage] # as in should_retry_this_error
|
||||
return ()
|
||||
already_skipped_ids: Final = _SKIPPED_DEPLOYMENT_IDS_ADAPTER.validate_python(already_skipped or ())
|
||||
already_skipped_ids: Final = _as_retry_skipped_deployment_ids(already_skipped)
|
||||
skipped: Final = tuple(sorted(frozenset((*already_skipped_ids, failed_deployment_id))))
|
||||
verbose_router_logger.debug(
|
||||
"Retry skips deployments that already answered %s to this request: %s", status_code, skipped
|
||||
|
|
@ -12496,7 +12499,7 @@ class Router:
|
|||
## RETRY SKIP ## -> drop deployments that already refused this request with a
|
||||
## non-retryable status, unless that leaves nothing, so the caller still gets
|
||||
## the provider's own error instead of a no-deployments error.
|
||||
_retry_skipped_deployment_ids: Final = (
|
||||
_retry_skipped_deployment_ids: Final = _as_retry_skipped_deployment_ids(
|
||||
request_kwargs.pop("_retry_skipped_deployment_ids", None) if request_kwargs else None
|
||||
)
|
||||
healthy_deployments = (
|
||||
|
|
@ -13412,7 +13415,7 @@ class Router:
|
|||
)
|
||||
|
||||
## RETRY SKIP ## -> see async counterpart in async_get_healthy_deployments.
|
||||
_retry_skipped_deployment_ids: Final = (
|
||||
_retry_skipped_deployment_ids: Final = _as_retry_skipped_deployment_ids(
|
||||
request_kwargs.pop("_retry_skipped_deployment_ids", None) if request_kwargs else None
|
||||
)
|
||||
healthy_deployments = (
|
||||
|
|
|
|||
|
|
@ -13263,6 +13263,10 @@ def _retry_skip_deployment(deployment_id, host, litellm_params=None, model_info=
|
|||
(400, None, None, ()),
|
||||
(None, "rejecting", None, ()),
|
||||
("400", "rejecting", None, ()),
|
||||
(400, "second", 7, ("second",)),
|
||||
(400, "second", "first", ("second",)),
|
||||
(400, "second", ["first"], ("second",)),
|
||||
(400, "second", ("first", 7), ("first", "second")),
|
||||
],
|
||||
)
|
||||
def test_router_deployment_ids_to_skip_on_retry(status_code, failed_deployment_id, already_skipped, expected):
|
||||
|
|
@ -13282,6 +13286,11 @@ def test_router_deployment_ids_to_skip_on_retry(status_code, failed_deployment_i
|
|||
(["rejecting", "sibling"], (), ["rejecting", "sibling"]),
|
||||
(["rejecting", "sibling"], None, ["rejecting", "sibling"]),
|
||||
(["rejecting", "sibling"], ("absent",), ["rejecting", "sibling"]),
|
||||
(["rejecting", "sibling"], 7, ["rejecting", "sibling"]),
|
||||
(["rejecting", "sibling"], "rejecting", ["rejecting", "sibling"]),
|
||||
(["rejecting", "sibling"], ["rejecting"], ["rejecting", "sibling"]),
|
||||
(["rejecting", "sibling"], {"rejecting": True}, ["rejecting", "sibling"]),
|
||||
(["rejecting", "sibling"], ("rejecting", 7), ["sibling"]),
|
||||
],
|
||||
)
|
||||
@pytest.mark.asyncio
|
||||
|
|
@ -13298,6 +13307,36 @@ async def test_router_healthy_deployments_keep_the_last_candidate_a_retry_skippe
|
|||
assert "_retry_skipped_deployment_ids" not in request_kwargs
|
||||
|
||||
|
||||
@pytest.mark.parametrize("client_supplied", [7, "rejecting", ["rejecting"], {"rejecting": True}, object()])
|
||||
@pytest.mark.asyncio
|
||||
async def test_router_retry_policy_400_keeps_upstream_error_when_a_client_forges_the_skip_list(
|
||||
monkeypatch: pytest.MonkeyPatch, client_supplied
|
||||
):
|
||||
monkeypatch.setattr(litellm, "disable_aiohttp_transport", True)
|
||||
router = litellm.Router(
|
||||
model_list=[_retry_skip_deployment("rejecting", "rejecting"), _retry_skip_deployment("sibling", "sibling")],
|
||||
num_retries=2,
|
||||
retry_policy={"BadRequestErrorRetries": 2},
|
||||
disable_cooldowns=True,
|
||||
)
|
||||
|
||||
with respx.mock as respx_mock:
|
||||
respx_mock.post("https://rejecting.local/v1/chat/completions").mock(
|
||||
return_value=httpx.Response(400, json={"error": _UPSTREAM_400})
|
||||
)
|
||||
respx_mock.post("https://sibling.local/v1/chat/completions").mock(
|
||||
return_value=httpx.Response(400, json={"error": _UPSTREAM_400})
|
||||
)
|
||||
with pytest.raises(litellm.BadRequestError) as raised:
|
||||
await router.acompletion(
|
||||
model="gpt-5.6",
|
||||
messages=[{"role": "user", "content": "hi"}],
|
||||
_retry_skipped_deployment_ids=client_supplied,
|
||||
)
|
||||
|
||||
assert "upstream refused this request" in str(raised.value)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_router_retry_policy_400_keeps_upstream_error_on_order_fallback_hop(monkeypatch: pytest.MonkeyPatch):
|
||||
monkeypatch.setattr(litellm, "disable_aiohttp_transport", True)
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue