Repository navigation
test(e2e): memory regression test for failing requests on the release gate - #40773
mateo-berri merged 11 commits into
Conversation
…mory regression test can read each gateway's RSS
…ate reads the serving workers On the release gate the e2e tests only see the nginx router, and the chart's ingress sent /debug/memory/summary to the backend catch-all, so the RSS check measured the backend pod instead of the gateway workers that serve the failing requests. Render it as an Exact gateway path next to /test, name the host in the summary response so workers behind one origin never collide on pid alone, and key the harness readings by (origin, hostname, pid)
… image reports memory The release image installs only the proxy extras, and psutil is a locust and mirakuru dev dependency, so /debug/memory/summary answered with an error and no ram_usage_mb on the e2e gate. Fall back to /proc/self/statm and /proc/meminfo on Linux when psutil cannot be imported
…itellm_e2e_memory_regression_failing_requests
Each checkpoint now samples until no new worker has answered for the settle window, and the growth assertion refuses a worker set that changed between the warm and after checkpoints instead of comparing only the intersection, so a leaking worker reached by one checkpoint alone cannot drop out of the gate
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Greptile SummaryThis PR adds a release-gate regression test for request-copy memory growth and stored request size during retries and fallbacks. It exposes authenticated per-worker memory summaries through the gateway, routes that exact endpoint through Helm ingress, and supports RSS reporting from Linux procfs when psutil is unavailable
Confidence Score: 5/5The PR appears safe to merge, with no outstanding correctness, security, or repository-rule failures The recent changes fully address the prior style findings without changing test behavior. The earlier access-control concern is not valid because non-admin keys without an explicit allowed route reach the admin-only rejection path, consistent with mateo-berri's live verification
|
| Filename | Overview |
|---|---|
| tests/e2e/router/test_reliability_memory_e2e.py | Adds the regression scenario and now places it in the required focused test class |
| litellm/proxy/common_utils/debug_utils.py | Adds hostname identification and a Linux procfs fallback for process RSS reporting |
| gateway/routes/allowlist.py | Exposes only the authenticated read-only memory summary route on gateway workers |
| helm/litellm/templates/ingress.yaml | Routes the exact memory summary path to gateway workers while preserving backend handling for other debug paths |
| tests/e2e/router/reliability_support.py | Adds refusing-deployment and transcript support used by the memory regression test |
Reviews (3): Last reviewed commit: "test(e2e): keep the memory regression ca..." | Re-trigger Greptile
| "/docs/oauth2-redirect", | ||
| "/redoc", | ||
| "/test", | ||
| "/debug/memory/summary", |
There was a problem hiding this comment.
Ordinary unscoped virtual keys can access this gateway route, exposing worker hostnames, PIDs, memory, GC state, and cache counts.
How this was verified: The route is absent from master-only lists, and unscoped virtual keys pass route authorization.
Rule Used: What: Fail any PR which may contains a security incident on litellm's authentication layer Why: Do not cause security incidents Bad: ```python # Check cache first cache_key = ( f"oidc_userinfo_{token[:20]}" # Use fi... (source)
There was a problem hiding this comment.
Verified live at a8ffc85: a plain virtual key gets 401 on the gateway summary route, user_api_key_auth admits proxy admins only
| def create_never_benched_refusing_deployment(proxy: ProxyClient, name: str) -> str: | ||
| """Register a deployment that refuses every call at the socket and opts out of the | ||
| stack's cooldown policy (cooldown_time 0), so the router keeps retrying it for the | ||
| whole run instead of benching it after allowed_fails and skipping the retry loop.""" |
There was a problem hiding this comment.
These docstrings restate helper behavior. Repository guidance permits comments only for necessary complex logic, so this requirement must be satisfied before merging.
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
…itellm_e2e_memory_regression_failing_requests # Conflicts: # tests/e2e/CLAUDE.md # tests/e2e/models.py
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 9ba7ec2. Configure here.
TLDR
Problem this solves:
How it solves it:
/debug/memory/summaryon the data plane, since the release stack's serving workers are the gateway replicas and their RSS is the reading that matters;/debug/memory/detailsand/debug/memory/gc/configurestay backend-only/debug/memory/summaryto the gateway too, since the release gate reaches the stack through the chart's ingress and the base chart sent that path to the backend, and the summary now names itshostnameso a worker is(replica, pod, pid)across replicas behind one address/proc/self/statmand/proc/meminfowhen psutil is missing instead of answering with an install hint and no RSSUser Flow
Before: a release cut with a v1.100.0-style leak passes the e2e gate and ships
litellm-e2estage runs its router suite against itproxy_server_requestof about 520 KB for an 80-message transcript, one copy of the request per failed attempt nested insidemetadata.previous_modelsAfter: the same release cut fails the e2e gate on its first failing request
litellm-e2estage runs its router suite against itx-litellm-call-idRelevant issues
The leak this guards was fixed by #39491 (per-request retry breadcrumbs); v1.100.1 carries it as #40455
Linear ticket
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
uv run pytest tests/test_litellm/<your_test_file>.py -v. Leave the suites (make test-unit-*,make test-unit) to CI: it finishes in ~15 minutes where a laptop takes an hour or more@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
Shared setup: two
python:3.12-slimcontainers, each withpip install litellm[proxy]==<version>and the release stage's config shape (store_prompts_in_spend_logs: true,json_logs: true, prometheus and otel callbacks,num_retries: 3,allowed_fails: 5,cooldown_time: 30,STORE_MODEL_IN_DB=True, its own Postgres database), plus two static model groupsmemory-primaryandmemory-fallbackwhose deployments point athttp://127.0.0.1:9/v1withcooldown_time: 0.body.jsonis a 40-turn chat transcript plus one user turn with"model": "memory-primary", "num_retries": 2, "fallbacks": ["memory-fallback"].LITELLM_MASTER_KEYis the container's master key. The leaky build is v1.100.0 (the release the customer hit), the fixed build is v1.101.0rc1 (the first tag carrying #39491). Both legs run single worker, the shape the release stage runs, with one process per containerBefore (ac66754)
the leaky release passes the gate
ls tests/e2e/router/at the merge base:reliability_support.py test_reliability_e2e.py, no test reads/debug/memory/summaryor a stored request size, sopython -m pytest tests/e2e/routeragainst v1.100.0 goes green on routing verdicts aloneAfter (93e2393)
the leaky release fails the gate
LITELLM_PROXY_URL=http://127.0.0.1:22377and the container's master key:the fixed release passes the gate
LITELLM_PROXY_URL=http://127.0.0.1:28082:Observations from the run:
the two-gateway release stack can read every serving worker's RSS (e4f958d)
Same stack shape as the stage-mirror e2e job, on this branch's proxy code:
backend.main:appon 45127 as the control plane, twogateway.main:app --workers 1replicas on 45125 and 45126 behind nginx on 45124, a fresh Postgres 16.6 migrated withmigrations/run.py,STORE_MODEL_IN_DB=True,DISABLE_SCHEMA_UPDATE=true, and the stage config minus its redis cache and vendor callbacks.LITELLM_PROXY_URL=http://127.0.0.1:45124 LITELLM_CONTROL_PLANE_URL=http://127.0.0.1:45127 LITELLM_PROXY_REPLICA_URLS=http://127.0.0.1:45125,http://127.0.0.1:45126/debugroute, so the test's RSS read 404s on both replicas after the probe and warmup had already passed (this is the failure the stage-mirror job showed on that tip):debug_utils.py, the test's worker key and unit tests, none of which those pip-installed release containers load. 77a0053 changes the test's checkpoint rule, 9375719 merges staging into the branch, and 9ba7ec2 moves the test into a class, so both legs ran again at 9ba7ec2 belowthe release gate's ingress sends the RSS read to the gateways (5fdb086)
The Buildkite gate reaches the stack through nginx rendered from the chart's ingress (
helm template -s templates/ingress.yaml, thenlitellm_release ingress-to-nginx). Same three processes as above, with nginx on 45124 rebuilt from the base chart's ingress and then from this branch's, six summary reads each, and the summary's newhostnamenext to the pid:The base ingress hands every summary read to the backend (pid 73366, the control plane, not a serving worker); the tip ingress alternates the two gateway workers, and a chat completion through the same router still lands on the gateways.
helm unittest helm/litellm: 142 passed, including the new assertion on the Exact path and the rejection of anextraPathsentry that would take it overthe release image reports RSS without psutil (eca7bb1)
Buildkite e2e-tests build #11761 at e4f958d failed on this test with
AssertionError: no /debug/memory/summary read carried ram_usage_mb, so the proxy cannot report its RSSafter the probe and the warmup had passed: the release image is built withuv sync --frozen --no-default-groups --extra proxy ...and psutil is only a locust and mirakuru dependency inuv.lock, so the summary answeredInstall psutil for memory monitoringand no RSS. The stage-mirror job never saw it because its uv sync installs the dev group. With the fallback, the/procreader inside a psutil-free Linux image against the kernel's ownVmRSS:The summary on this branch's stack (macOS, psutil present, so the psutil branch), direct on each gateway and through the tip router, plus the details route staying backend-only:
both release legs again at 9ba7ec2, the tree merged with staging and the test in a class
9375719 merges
litellm_internal_staginginto the branch (both sides kept intests/e2e/CLAUDE.mdandtests/e2e/models.py), a8ffc85 and 9ba7ec2 trim then drop the helper docstrings, and 9ba7ec2 moves the test intoTestReliabilityMemory, so both release legs ran again with the test at 9ba7ec2. Same twopython:3.12-slimcontainers as the release legs above (v1.100.0 on 22377, v1.101.0rc1 on 28082, each with its own Postgres and the release stage's config shape), pointed at one address each:The merged
debug_utils.py(staging's #40713 rewrote the cache counts next to the summary) on this branch's three-process stack booted from a8ffc85 (9ba7ec2 changes onlytests/e2e, so the proxy, gateway, and chart bytes are the same), the summary through the tip router (nginx on 45124) and direct, the other debug routes, and a plain virtual key from/key/generateon the new gateway route:helm unittest -f 'tests/*_tests.yaml' helm/litellmat a8ffc85: 150 passed; the harness's own tests (tests/e2e/test_*.py) at 9ba7ec2: 182 passedthe real release gate runs it and passes (9ba7ec2)
The Buildkite
e2e-testsgate that the RC and stable tags wait on: build #11761 (e4f958d) failed on the release image having no psutil, so every summary read came back withoutram_usage_mb, which is what eca7bb1 fixes; build #11828 at 77a0053 triggeredlitellm-e2e-prbuild 411, which deployed the chart at that commit and ran the full suite, this test included, in 57m36s; build #12285 at the final tip 9ba7ec2 triggeredlitellm-e2e-prbuild 449, which deployed the chart at this commit and ran the full suite again, the test now under its class node id, in 58m39s:The stage-mirror GitHub Actions job (
e2e-changed-tests, run 34668570996) ran the selected files three times at 9ba7ec2 (tests/e2e/router/test_reliability_memory_e2e.py: 1 collected, 0 skippedin each pass, then20 passedin 306.62s, 306.44s, and 313.37s); run 34556104459 did the same at 77a0053Type
✅ Test
Caveats (if any)
Low
E2E_MEMORY_STORED_REQUEST_BUDGET_KB,E2E_MEMORY_RSS_BUDGET_MB)store_prompts_in_spend_logs: trueand a spend-log writer, and/model/newneedsSTORE_MODEL_IN_DB=True; the stage-mirror config has both, and a stack without them fails on "no spend log row appeared" or "stored no request body" rather than passing vacuously/debug/memory/summary(worker pid, hostname, RSS, gc counters); leaving it backend-only is net-negative because the backend's RSS is not the serving worker's, and the mutating and heavier debug routes stay off the gateway400 Missing required parameter: 'messages'in an earlier rc1 curl loop were a curl body artifact, not a proxy behavior; the pasted run has none/debug/memory/summarypath is the guard for this one, and the real gate (build Add Elasticsearch Logging Tutorial #11761) is what caught the mismatch/procfallback;/debug/memory/detailsstill needs psutil for its file-handle and thread counts and keeps answering with the install hint on the release image, which nothing on the gate readserrorand noram_usage_mb, and the test fails on "no /debug/memory/summary read carried ram_usage_mb" rather than passing vacuouslyE2E_MEMORY_RSS_SETTLE_SAMPLESas the knob; the sampling is capped at four windows so a stack that keeps producing new pids cannot hang the gateQA runbook
STORE_MODEL_IN_DB=True,store_prompts_in_spend_logs: trueundergeneral_settings, and a database, then exportLITELLM_PROXY_URLandLITELLM_MASTER_KEYapi_base: http://127.0.0.1:9/v1, a placeholderapi_key, andcooldown_time: 0inlitellm_params, under two fresh model group names (primary and fallback)"num_retries": 2, and"fallbacks": ["<fallback group>"], and expect HTTP 500 with anx-litellm-call-idresponse headerproxy_server_requestserializes to under 64 KB (about 31 KB on a build with fix(router): keep retry breadcrumbs per request and out of the request snapshot #39491; about 300 KB on v1.100.0)memory.ram_usage_mbperworker_pidram_usage_mbper worker as the baselineFinal Attestation
Reopened from #40640 with the same commits so the approval can come from someone other than the author
Note
Medium Risk
Exposes one authenticated read-only debug route on the data-plane gateway and changes memory summary behavior (proc fallback, hostname); ingress/allowlist must stay in sync or RSS reads misroute.
Overview
Adds a release-gate e2e memory regression (
reliability.perf.memory.under_slo) that reproduces v1.100.0-style retry traffic: failing chat completions with retries, fallbacks, and a long transcript must keep the spend-log stored request under ~64 KB and per-worker RSS growth (warmup vs measured batch) under ~48 MB, sampling/debug/memory/summaryon every replica with settle logic keyed by(replica, hostname, pid).Routing and observability so the test works on gateway-heavy stacks:
/debug/memory/summaryis allowlisted on the gateway and routed Exact to gateway in Helm ingress (other/debug/memory/*stay backend-only). The summary endpoint gainshostname, refactored RSS helpers, and a/procfallback when psutil is absent (release images). Harness additions includememory_summary_everywhere, typedMemorySummaryResponse, env-tunableE2E_MEMORY_*knobs, and reliability helpers (cooldown_time: 0refusing deployments, optional chat history).Reviewed by Cursor Bugbot for commit 9ba7ec2. Bugbot is set up for automated code reviews on this repo. Configure here.