From 65d8bbb8ac4610c9722ecae03ffa16b352c47256 Mon Sep 17 00:00:00 2001 From: mateo-berri <277851410+mateo-berri@users.noreply.github.com> Date: Sat, 5 Sep 2026 16:10:40 -0700 Subject: [PATCH] fix(e2e): wait for every gateway before using a new model and keep the network rerun The changed-tests workflow overrode the suite's `--reruns 1` with `--reruns 0`, so a transport blip failed a pass that pytest.ini already scopes to network errors and 5xx responses. Pass 2 of run 33692484803 also went red 15s after a model write with "no healthy deployments": the barrier only polled /v1/models through nginx, which proves one gateway converged, and the next request rolled the other. The stack now exports LITELLM_PROXY_REPLICA_URLS, the barrier polls every replica with the full budget before settling, and up.sh refuses to boot without DD_API_KEY, since the gateway config enables the datadog callback on every run --- .github/e2e-stack/up.sh | 7 ++ .github/workflows/test-e2e-changed.yml | 4 +- tests/e2e/CONTRIBUTING.md | 6 +- tests/e2e/claude_code/_env.py | 1 + tests/e2e/claude_code/conftest.py | 1 + tests/e2e/e2e_config.py | 18 +++- tests/e2e/proxy_client.py | 117 +++++++++++++++++++------ tests/e2e/test_proxy_client.py | 87 ++++++++++++++++++ 8 files changed, 206 insertions(+), 35 deletions(-) create mode 100644 tests/e2e/test_proxy_client.py diff --git a/.github/e2e-stack/up.sh b/.github/e2e-stack/up.sh index e891add341d..2f2e6c6f9a8 100755 --- a/.github/e2e-stack/up.sh +++ b/.github/e2e-stack/up.sh @@ -54,6 +54,12 @@ if [[ -f "${REPO_ROOT}/tests/e2e/.env" ]]; then set +a fi +if [[ -z "${DD_API_KEY:-}" ]]; then + log "DD_API_KEY is empty; the gateway config enables the datadog callback, so put a Datadog API key in tests/e2e/.env" + exit 1 +fi +export DD_SITE="${DD_SITE:-datadoghq.com}" + if ! port_open "${DATABASE_PORT}"; then docker run -d --name e2e-postgres -p "${DATABASE_PORT}:5432" \ -e "POSTGRES_USER=${DATABASE_USER}" -e "POSTGRES_PASSWORD=${DATABASE_PASSWORD}" -e "POSTGRES_DB=${DATABASE_NAME}" \ @@ -189,6 +195,7 @@ wait_for "load balancer" "curl -fs http://127.0.0.1:${LB_PORT}/health/liveliness cat > "${STACK_DIR}/stack.env" <> "${GITHUB_ENV}" - - name: Run the selected tests three times with retries off + - name: Run the selected tests three times env: TESTS: ${{ needs.detect.outputs.tests }} E2E_FIXTURE_MODE: live @@ -172,7 +172,7 @@ jobs: report="${RUNNER_TEMP}/e2e-pass-${pass}.xml" echo "::group::pass ${pass} of 3" set +e - uv run --no-sync pytest "${test_files[@]}" --rootdir=. --reruns 0 -v -p no:cacheprovider \ + uv run --no-sync pytest "${test_files[@]}" --rootdir=. -v -p no:cacheprovider \ -o junit_family=xunit1 --junitxml="${report}" > "${RUNNER_TEMP}/e2e-pass-${pass}.log" 2>&1 status=$? set -e diff --git a/tests/e2e/CONTRIBUTING.md b/tests/e2e/CONTRIBUTING.md index d51e729fd1c..7bfd02023dd 100644 --- a/tests/e2e/CONTRIBUTING.md +++ b/tests/e2e/CONTRIBUTING.md @@ -54,13 +54,13 @@ Some suites need extra services the bare proxy does not start. The `logging/` OT ### The pull request check -Every same-repository PR that adds, modifies, or renames a `tests/e2e/**/test_*.py` file runs those changed files three times with pytest retries off. The stage-mirror stack has a control-plane backend, two gateways behind nginx, Postgres, Jaeger, and TLS cluster-mode Valkey. Documentation, harness, configuration, deleted-file, and workflow-only changes do not start the stack or request environment approval. The `ui/`, `claude_code/`, and `load/` directories and `batches/test_managed_files_enforcement_e2e.py` remain outside this check because they use separate tooling or need a differently configured stack +Every same-repository PR that adds, modifies, or renames a `tests/e2e/**/test_*.py` file runs those changed files three times. The suite's own single rerun for network errors and 5xx responses (see `pytest.ini`) applies on every pass, so a transport blip does not fail the check while a race inside a test still does. The stage-mirror stack has a control-plane backend, two gateways behind nginx, Postgres, Jaeger, and TLS cluster-mode Valkey. The stack exports every gateway address in `LITELLM_PROXY_REPLICA_URLS`, so model registration waits until each gateway lists the new model rather than whichever one the load balancer answered from. Documentation, harness, configuration, deleted-file, and workflow-only changes do not start the stack or request environment approval. The `ui/`, `claude_code/`, and `load/` directories and `batches/test_managed_files_enforcement_e2e.py` remain outside this check because they use separate tooling or need a differently configured stack -Every selected file must execute at least one passing test in each pass, and any test failure, collection error, or entirely skipped or deselected file fails the check. A failed pass stops the run without retrying. The final `e2e-changed-tests` job succeeds only when no supported test files changed or the approved run completed all three passes. Fork PRs with selected tests fail this gate until a maintainer brings the reviewed change onto a same-repository branch +Every selected file must execute at least one passing test in each pass, and any test failure, collection error, or entirely skipped or deselected file fails the check. A failed pass stops the run. The final `e2e-changed-tests` job succeeds only when no supported test files changed or the approved run completed all three passes. Fork PRs with selected tests fail this gate until a maintainer brings the reviewed change onto a same-repository branch Repository admins must require the `e2e-changed-tests` status check for merging and configure the `e2e-changed` environment with required reviewers, self-review disabled, and admin bypass disabled. Each push cancels the previous run; a new run that selects tests needs a fresh approval. Reviewers must inspect the entire executable PR diff, including application code, dependencies, tests, and workflow helpers, before approving the exact revision. Approved code executes with provider credentials, so environment approval is a trust decision about that code -Credentials come from the existing AWS Secrets Manager secrets in us-east-1, `litellm-e2e-changed-provider-keys` and `litellm-e2e-changed-license`. The OIDC role must trust only `repo:BerriAI/litellm:environment:e2e-changed` with audience `sts.amazonaws.com` and have read access only to these secrets. The short-lived reader credentials are scoped to the fetch step. Provider credentials must cover the selected suites, including Datadog credentials when logging or MCP tests need them; missing credentials fail the run. Keep provider credentials dedicated to this lane with only the permissions those tests need +Credentials come from the existing AWS Secrets Manager secrets in us-east-1, `litellm-e2e-changed-provider-keys` and `litellm-e2e-changed-license`. The OIDC role must trust only `repo:BerriAI/litellm:environment:e2e-changed` with audience `sts.amazonaws.com` and have read access only to these secrets. The short-lived reader credentials are scoped to the fetch step. Provider credentials must cover the selected suites, including Datadog credentials when logging or MCP tests need them; missing credentials fail the run. `up.sh` refuses to start without `DD_API_KEY`, because the stack's gateway config enables the Datadog callback for every run and a gateway booted without the key fails readiness. Keep provider credentials dedicated to this lane with only the permissions those tests need Fetched values are masked before use, and credential files and raw output are private to the runner. Public logs contain selected file names, counts, and pass status; raw pytest output, reports, and stack logs are not uploaded or printed. The workflow removes them and the credential files during cleanup. To diagnose a failed pass, reproduce the selected files locally with the appropriate credentials and inspect the local logs diff --git a/tests/e2e/claude_code/_env.py b/tests/e2e/claude_code/_env.py index ed2da1fe1e2..889d8f848dd 100644 --- a/tests/e2e/claude_code/_env.py +++ b/tests/e2e/claude_code/_env.py @@ -99,5 +99,6 @@ def require_proxy_client( base_url=cfg.base_url, master_key=cfg.api_key, control_plane_base_url=cfg.base_url, + replica_urls=(cfg.base_url,), ) return ProxyClientConfig(client=client, api_key=cfg.api_key) diff --git a/tests/e2e/claude_code/conftest.py b/tests/e2e/claude_code/conftest.py index aaad7667936..6e3dce0377e 100644 --- a/tests/e2e/claude_code/conftest.py +++ b/tests/e2e/claude_code/conftest.py @@ -594,6 +594,7 @@ def _build_control_plane_client(proxy_config: ProxyConfig): base_url=proxy_config.base_url, master_key=proxy_config.api_key, control_plane_base_url=proxy_config.base_url, + replica_urls=(proxy_config.base_url,), ) diff --git a/tests/e2e/e2e_config.py b/tests/e2e/e2e_config.py index 21c5a338dc3..70d1180cb38 100644 --- a/tests/e2e/e2e_config.py +++ b/tests/e2e/e2e_config.py @@ -10,6 +10,7 @@ import os import time import uuid from pathlib import Path +from typing import Final from dotenv import load_dotenv @@ -32,6 +33,14 @@ CONTROL_PLANE_BASE_URL = os.environ.get( "LITELLM_CONTROL_PLANE_URL", PROXY_BASE_URL ).rstrip("/") + +def parse_replica_urls(raw: str, fallback: str) -> tuple[str, ...]: + urls: Final = tuple(url.strip().rstrip("/") for url in raw.split(",") if url.strip()) + return urls or (fallback,) + + +PROXY_REPLICA_URLS: Final = parse_replica_urls(os.environ.get("LITELLM_PROXY_REPLICA_URLS", ""), PROXY_BASE_URL) + UI_USERNAME = os.environ.get("E2E_UI_USERNAME", "admin") UI_PASSWORD = os.environ.get("E2E_UI_PASSWORD", MASTER_KEY) @@ -86,10 +95,11 @@ SLOW_PROVIDER_TIMEOUT_SECONDS = float(os.environ.get("E2E_SLOW_PROVIDER_TIMEOUT" # (`proxy_config_reload_interval_seconds`, 30s by default and 7s on the e2e stack) # plus margin. # -# The barriers below wait this out instead of returning on first sight, because a -# single successful read only proves ONE replica converged: every request opens a -# fresh connection, so a load-balanced Service routes each one independently and -# the next call re-rolls. See ProxyClient._await_model_servable. +# The barriers below wait this out on top of polling /v1/models on every replica in +# PROXY_REPLICA_URLS: that poll proves each addressed gateway converged, but not the +# workers behind it, and behind a load balancer (PROXY_REPLICA_URLS unset) a +# successful read only proves ONE replica converged, because every request opens a +# fresh connection and the next call re-rolls. See ProxyClient._await_model_servable. PROPAGATION_TIMEOUT = float(os.environ.get("E2E_PROPAGATION_TIMEOUT", "15")) EXPECT_RUST = os.environ.get("E2E_EXPECT_RUST", "").strip().lower() in ("1", "true", "yes") diff --git a/tests/e2e/proxy_client.py b/tests/e2e/proxy_client.py index 2d382a610e1..c5b34a534b7 100644 --- a/tests/e2e/proxy_client.py +++ b/tests/e2e/proxy_client.py @@ -10,12 +10,15 @@ from __future__ import annotations import time import warnings -from collections.abc import Callable +from collections.abc import Callable, Mapping from dataclasses import dataclass from datetime import datetime +from types import MappingProxyType +from typing import Final from e2e_http import ( AnthropicHeaders, + AuthHeaders, NoBody, ProbeResult, Result, @@ -71,6 +74,7 @@ from e2e_config import ( POLL_INTERVAL, POLL_TIMEOUT, PROXY_BASE_URL, + PROXY_REPLICA_URLS, REQUEST_TIMEOUT, SLOW_PROVIDER_TIMEOUT_SECONDS, settle_propagation, @@ -79,10 +83,11 @@ from transport import HttpTransport, SplitTransport, Transport RowsPredicate = Callable[[list[SpendLogRow]], bool] -# After /model/new, poll data-plane /v1/models until the model is listed (or fail). -# Bound by MODEL_SERVABLE_TIMEOUT so a stuck reload does not burn the spend -# poll_timeout (120s). Return on first listing; settle_propagation owns the separate -# wait that lets every worker and replica reload before the caller uses the model. +# After /model/new, poll /v1/models on every replica in PROXY_REPLICA_URLS until each +# lists the model (or fail). Bound by MODEL_SERVABLE_TIMEOUT per replica so a stuck +# reload does not burn the spend poll_timeout (120s). Return on first listing; +# settle_propagation owns the separate wait that lets the workers behind each replica +# reload before the caller uses the model. MODEL_SERVABLE_TIMEOUT = 40.0 MODEL_SERVABLE_DB_SYNC_SECONDS = 0.0 MODEL_SERVABLE_INTERVAL = 2.0 @@ -108,6 +113,16 @@ class NotServable: ServableOutcome = Servable | NotServable +type ModelsPoller = Callable[[float], Result[ModelsListResponse]] + + +@dataclass(frozen=True, slots=True) +class NotServableOn: + """`NotServable` labeled with the replica whose /v1/models never listed the model.""" + + replica: str + last_result: Result[ModelsListResponse] | None + def await_servable( list_models: Callable[[float], Result[ModelsListResponse]], @@ -171,9 +186,41 @@ def await_servable( sleep(wait) +def await_servable_everywhere( + pollers: Mapping[str, ModelsPoller], + *, + model_name: str, + timeout: float, + interval: float, + request_timeout: float, + db_sync_seconds: float, + now: Callable[[], float], + sleep: Callable[[float], None], +) -> Servable | NotServableOn: + """`await_servable` against every replica in turn, each with the full budget, so + the model is only servable once every replica has listed it.""" + for replica, list_models in pollers.items(): + match await_servable( + list_models, + model_name=model_name, + timeout=timeout, + interval=interval, + request_timeout=request_timeout, + db_sync_seconds=db_sync_seconds, + now=now, + sleep=sleep, + ): + case NotServable(last_result=last_result): + return NotServableOn(replica=replica, last_result=last_result) + case Servable(): + continue + return Servable() + + def servable_timeout_message( *, model_name: str, + replica: str, timeout: float, db_sync_seconds: float, last_result: Result[ModelsListResponse] | None, @@ -184,8 +231,8 @@ def servable_timeout_message( else "" ) return ( - f"model {model_name!r} was created but never became servable on the data " - f"plane within {timeout}s of first listing (plus {db_sync_seconds}s continuous " + f"model {model_name!r} was created but never became servable on {replica} " + f"within {timeout}s of first listing (plus {db_sync_seconds}s continuous " f"DB sync) after /model/new (control/data-plane propagation or " f"STORE_MODEL_IN_DB reload issue){last_error}" ) @@ -194,6 +241,7 @@ def servable_timeout_message( @dataclass(frozen=True, slots=True) class ProxyClient: transport: Transport + replicas: Mapping[str, Transport] poll_timeout: float = 120.0 poll_interval: float = 5.0 model_servable_timeout: float = MODEL_SERVABLE_TIMEOUT @@ -310,12 +358,13 @@ class ProxyClient: model name passed". We poll the data-plane /v1/models until the model appears, then settle for the remainder of the propagation budget. - Both steps are needed, and the second is the one that matters at >1 replica. - The poll proves *a* replica is serving the model; it cannot prove they all - are, because every request opens a fresh connection and a load-balanced - Service routes each one independently -- so the caller's next request - re-rolls and can land on a replica that has not reloaded yet. Waiting out - PROPAGATION_TIMEOUT is what makes the model safe to use anywhere.""" + Both steps are needed. The poll asks every replica in PROXY_REPLICA_URLS + directly, so behind the stack's load balancer it proves each gateway serves + the model rather than whichever one the balancer routed the poll to. It still + cannot see the workers behind a gateway, nor any replica when only the + balancer address is configured (every request opens a fresh connection, so + the caller's next request re-rolls), so waiting out PROPAGATION_TIMEOUT is + what makes the model safe to use anywhere.""" model_id = unwrap( self.transport.post( "/model/new", @@ -330,16 +379,10 @@ class ProxyClient: return model_id def _await_model_servable(self, model_name: str, listed_for: str | None = None) -> None: - """Block until the data plane lists `model_name`, or fail at model_servable_timeout.""" - headers = self.transport.master if listed_for is None else self.transport.bearer(listed_for) - outcome = await_servable( - lambda poll_timeout: self.transport.get( - "/v1/models", - headers=headers, - params=NoBody(), - response_type=ModelsListResponse, - timeout=poll_timeout, - ), + """Block until every replica lists `model_name`, or fail at model_servable_timeout.""" + headers: Final = self.transport.master if listed_for is None else self.transport.bearer(listed_for) + outcome: Final = await_servable_everywhere( + {url: self._models_poller(transport, headers) for url, transport in self.replicas.items()}, model_name=model_name, timeout=self.model_servable_timeout, interval=self.model_servable_interval, @@ -351,16 +394,27 @@ class ProxyClient: match outcome: case Servable(): return - case NotServable(last_result=last_result): + case NotServableOn(replica=replica, last_result=last_result): raise AssertionError( servable_timeout_message( model_name=model_name, + replica=replica, timeout=self.model_servable_timeout, db_sync_seconds=self.model_servable_db_sync_seconds, last_result=last_result, ) ) + @staticmethod + def _models_poller(transport: Transport, headers: AuthHeaders) -> ModelsPoller: + return lambda poll_timeout: transport.get( + "/v1/models", + headers=headers, + params=NoBody(), + response_type=ModelsListResponse, + timeout=poll_timeout, + ) + def update_model(self, model_id: str, litellm_params: LiteLLMParamsBody) -> None: """Merge `litellm_params` over the deployment `model_id`'s stored params via POST /model/update. The proxy overlays only the non-null fields and clears @@ -547,16 +601,20 @@ def build_proxy_client( base_url: str = PROXY_BASE_URL, master_key: str = MASTER_KEY, control_plane_base_url: str = CONTROL_PLANE_BASE_URL, + replica_urls: tuple[str, ...] = PROXY_REPLICA_URLS, ) -> ProxyClient: """The ProxyClient every suite's client is built from: a SplitTransport that routes LLM calls to the data plane (PROXY_BASE_URL) and management/admin calls to the control plane (CONTROL_PLANE_BASE_URL), with the shared poll budget. The two base URLs are the same for a monolithic proxy, so routing is then a no-op. + ``replica_urls`` (PROXY_REPLICA_URLS) names every data-plane replica the model + barrier polls directly; it is the data-plane URL itself unless the stack + exports each gateway's own address. The endpoints are injectable for callers that resolve the proxy some other way than ``e2e_config``'s env names (see ``claude_code/_env.py``); they must - pass all three together, since a caller that overrides only the data plane - would leave management calls pointed at the env default. + pass all four together, since a caller that overrides only the data plane + would leave management calls and the replica poll pointed at the env defaults. Test-to-proxy traffic always goes over the wire, in every E2E_FIXTURE_MODE: record and replay scope to the proxy's provider-bound calls via the @@ -573,8 +631,15 @@ def build_proxy_client( request_timeout=REQUEST_TIMEOUT, ), ) + replicas: Final = MappingProxyType( + { + url: HttpTransport(base_url=url, master_key=master_key, request_timeout=REQUEST_TIMEOUT) + for url in replica_urls + } + ) return ProxyClient( transport=split, + replicas=replicas, poll_timeout=POLL_TIMEOUT, poll_interval=POLL_INTERVAL, ) diff --git a/tests/e2e/test_proxy_client.py b/tests/e2e/test_proxy_client.py new file mode 100644 index 00000000000..a508c97b9fb --- /dev/null +++ b/tests/e2e/test_proxy_client.py @@ -0,0 +1,87 @@ +"""Harness coverage for the model barrier that gates on every replica. + +No proxy needed and no ``e2e`` marker: this pins that a model registered through +the control plane only counts as servable once every configured replica lists it +on /v1/models, which is what keeps a two-gateway stack from handing a test a +model that one gateway has not reloaded yet. The fakes are plain pollers and an +injected clock, so nothing here monkeypatches anything. +""" + +from __future__ import annotations + +from collections.abc import Iterable, Mapping +from dataclasses import dataclass +from itertools import chain, repeat +from typing import Final + +import pytest + +from e2e_config import parse_replica_urls +from e2e_http import Success +from models import ModelListEntry, ModelsListResponse +from proxy_client import ModelsPoller, NotServableOn, Servable, await_servable_everywhere + +MODEL: Final = "gpt-under-test" +TIMEOUT: Final = 10.0 +INTERVAL: Final = 2.0 + + +@dataclass +class FakeClock: + elapsed: float = 0.0 + + def now(self) -> float: + return self.elapsed + + def sleep(self, seconds: float) -> None: + self.elapsed += seconds + + +def _listing(*model_ids: str) -> Success[ModelsListResponse]: + entries: Final = tuple(ModelListEntry(id=model_id) for model_id in model_ids) + return Success(status_code=200, data=ModelsListResponse(data=entries)) + + +def _poller(results: Iterable[Success[ModelsListResponse]]) -> ModelsPoller: + it: Final = iter(results) + return lambda _timeout: next(it) + + +def _await(pollers: Mapping[str, ModelsPoller]) -> Servable | NotServableOn: + clock: Final = FakeClock() + return await_servable_everywhere( + pollers, + model_name=MODEL, + timeout=TIMEOUT, + interval=INTERVAL, + request_timeout=5.0, + db_sync_seconds=0.0, + now=clock.now, + sleep=clock.sleep, + ) + + +class TestAwaitServableEverywhere: + @pytest.mark.parametrize("missing", ["gateway-1", "gateway-2"]) + def test_fails_on_the_replica_that_never_lists_the_model(self, missing: str) -> None: + pollers: Final = { + "gateway-1": _poller(repeat(_listing(MODEL))), + "gateway-2": _poller(repeat(_listing(MODEL))), + } | {missing: _poller(repeat(_listing()))} + assert _await(pollers) == NotServableOn(replica=missing, last_result=_listing()) + + def test_passes_once_every_replica_lists_the_model(self) -> None: + pollers: Final = { + "gateway-1": _poller(repeat(_listing(MODEL))), + "gateway-2": _poller(chain(repeat(_listing(), 2), repeat(_listing(MODEL)))), + } + assert _await(pollers) == Servable() + + +class TestParseReplicaUrls: + def test_splits_and_trims_the_gateway_addresses(self) -> None: + raw: Final = " http://127.0.0.1:4010/, http://127.0.0.1:4011 " + assert parse_replica_urls(raw, "http://lb") == ("http://127.0.0.1:4010", "http://127.0.0.1:4011") + + def test_falls_back_to_the_data_plane_address_when_unset(self) -> None: + assert parse_replica_urls("", "http://lb") == ("http://lb",)