test(e2e): bound the post-/model/new servable wait at 40s - #35012
test(e2e): bound the post-/model/new servable wait at 40s#35012mubashir1osmani wants to merge 6 commits into
Conversation
Greptile SummaryThe PR bounds and strengthens the E2E model-readiness wait.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| tests/e2e/proxy_client.py | Adds bounded model-readiness polling with continuous-listing validation and completes the previously requested deadline safeguards. |
| tests/e2e/transport.py | Plumbs an optional per-call GET timeout through the transport protocol, split router, and concrete HTTP transport. |
Reviews (8): Last reviewed commit: "fix(e2e): reject first listing that retu..." | Re-trigger Greptile
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
9e0acb5 to
8eca105
Compare
|
@greptile-apps review it |
|
@greptile-apps review it |
|
@greptile-apps review, give me a new score |
|
Timeout is a fixed harness constant (40s), not derived from proxy general_settings or PROXY_CONFIG_RELOAD_INTERVAL_SECONDS. That is intentional: the e2e suite owns how long create_model waits for /v1/models, as a middle ground between first-poll happy path and the 120s spend poll_timeout. |
|
Fair point — a test harness being opinionated about its own timing contract is a legitimate design choice, and "the suite owns how long My concern was specifically: the PR description justifies the 40s bound as "one full default reload interval plus headroom," which ties the value semantically to the proxy's reload interval. When that reasoning is embedded in the constant, a reader configuring the proxy above 30s might not realize the harness has its own fixed contract and will see unexplained failures. The concern is documentation, not correctness. A one-line comment on If you consider this out of scope or already obvious from the module context, I'm fine closing this thread. |
_await_model_servable used poll_timeout (120s), the spend/log read-back budget. A stuck model reload therefore stalled every suite that creates a deployment for two minutes before failing Give create_model a fixed harness middle ground: model_servable_timeout=40s, polled every 2s, with each /v1/models call capped at 5s and clamped to the remaining deadline so one slow GET cannot overrun the wait. Happy path still returns on the first listing. Not derived from proxy general_settings or env Transport.get accepts an optional per-call timeout for that clamp. Unit tests cover the deadline arithmetic and clamp without a live proxy
53cdbe9 to
c082a0e
Compare
|
Companion ops change: BerriAI/litellm-ops#110 drops stage's proxy_config_reload_interval_seconds: 5 override so stage keeps the product default (30s). This PR owns the post-/model/new wait as a fixed harness 40s budget and does not read that setting. |
create_model returned after the first /v1/models hit that listed the model, so chat could still land on a cold gateway worker (numWorkers>1 / peer pod) and 400 Invalid model name. Require continuous listing for the product default add_deployment interval (30s) after first sight so every worker has synced from the DB; first listing still bounded at 40s
Keep the create_model DB-sync wait in the harness; the pure-function unit file is not needed for this PR
When less than one full poll interval remained in the first-listing budget, the pre-sleep check returned NotServable without another /v1/models call. Sleep only min(interval, time left) so a model that becomes listable in the last seconds of the timeout still gets a clamped final poll
|
@greptileai re review |
A poll may start with remaining budget and still return after started+timeout if the transport overruns its clamp. Recheck the first-listing deadline after the response so a late listing does not open the continuous DB-sync phase
|
Please re-review. Latest tip rejects first listings whose response arrives after the first-listing deadline (started + 40s), so a late /v1/models return cannot open the 30s continuous DB-sync phase. |
Continuous 30s listing after first sight made the suite crawl: every create_model slept through repeated GET /v1/models, and multi-worker misses could reset the window. Keep the 40s first-listing budget; set model_servable_db_sync_seconds to 0 so we return as soon as the model appears
TLDR
Problem this solves:
How it solves it:
Companion ops PR
BerriAI/litellm-ops#110 removes stage's
proxy_config_reload_interval_seconds: 5override so stage uses the product default (30s). This harness PR owns the wait with a fixed 40s budget and does not read that settingRelevant issues
Linear ticket
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
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
Both runs against a live proxy (
python litellm/proxy/proxy_cli.py --config litellm/proxy/dev_config.yaml), driving the real harness call path. The wait is timed against a model name the data plane never serves, which is what a genuinely broken reload looks like from the harnessBefore, with the old budget (
poll_timeout=120s,poll_interval=5s):After, with this PR's bound (
model_servable_timeout=40s,model_servable_interval=2s,model_servable_request_timeout=5s):Happy path
create_modelreturned in 2.31s on the same proxy (model listed on the first poll). Unit coverage for the clamp and deadline arithmetic:Confirming the happy path still works on a live proxy:
Type
Test
Changes
_await_model_servablepolled/v1/modelstopoll_timeout, which exists for eventually-consistent read-backs like spend rows. Model readiness is a different wait: after/model/new, the data plane must list the model before callers can invoke it. Sharing the spend read-back budget meant a broken reload was absorbed as a two-minute stall per model rather than reportedFixed harness constants:
model_servable_timeout(40s) for first listing,model_servable_db_sync_seconds(30s, product defaultproxy_config_reload_interval_seconds) of continuous listing after first sight,model_servable_interval(2s),model_servable_request_timeout(5s, clamped to remaining). Not read from live proxy config. A single /v1/models success is not enough: stage gateway runs multiple workers, and only the writer reloads immediately; peers/workers sync on the add_deployment job every 30sThe poll loop is extracted as
await_servable, a pure function over an injected clock/sleep and alist_modelscallback that takes the per-poll request timeout, returning aServable | NotServableunion.tests/e2e/test_proxy_client_model_servable.pycovers the happy path, multi-poll appearance, deadline give-up, the request-timeout clamp regression, and failed-read reporting without a live proxyScope note: this changes only how long the harness waits before reporting. It does not make a model propagate faster, and it does not fix any test that fails after the wait succeeds; those have a separate root cause on the proxy side and are tracked outside this PR. Reviewers evaluating this should read it as latency-of-failure, not as a fix for model-propagation failures
QA runbook
tests/e2e/test_proxy_client_model_servable.py::test_clamps_request_timeout_to_remaining_deadline- when remaining budget is less than the 5s request cap, the poll receives the remaining budget so wall time never exceeds the overall timeoutcd tests/e2e && python3 -m pytest test_proxy_client_model_servable.py -qand expect 6 passedawait_servableto pass the full request cap (ignoring remaining) fails this testProxyClient._await_model_servable- a model the data plane never lists fails in ~40s instead of ~120s, with the diagnostic preservedpython litellm/proxy/proxy_cli.py --config litellm/proxy/dev_config.yaml(needsSTORE_MODEL_IN_DB=Trueand a reachableDATABASE_URL)ProxyClient._await_model_servable("model-that-will-never-be-listed")and time it; expect an AssertionError in ~40s naming the model and the 40.0s budgetFinal Attestation