fix(pass-through): remove stale routes by key to prevent unbounded registry growth - #24846
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…registry growth When pass-through endpoints stored in DB have no `id` field, each 30s reload generates a new UUID. The old cleanup (`remove_endpoint_routes`) scanned by `endpoint_id` (UUID only) and never matched the full route key, so stale entries were never deleted — the dict grew without bound, O(n²). Fix: replace `remove_endpoint_routes(endpoint_key)` with `_registered_pass_through_routes.pop(endpoint_key, None)` to remove the stale route key directly in O(1). Fixes BerriAI#24833 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
6226760 to
11cecb3
Compare
Greptile SummaryThis PR fixes a real and severe CPU/memory regression (#24833) where the periodic pass-through endpoint reload caused Key findings:
Confidence Score: 4/5Safe to merge for the primary CPU/memory fix, but the incomplete openai_routes cleanup means the same class of unbounded growth persists for auth-enabled subpath endpoints. The core registry growth fix is correct and well-targeted. One P1 gap remains: the PR claims to clean up LiteLLMRoutes.openai_routes but does not, leaving a parallel leak for auth-enabled subpath endpoints that will cause the same O(n) per-request degradation in route_checks.py over time. The cleanup loop in initialize_pass_through_endpoints (lines 2292–2298 of pass_through_endpoints.py) needs the openai_routes removal logic added.
|
| Filename | Overview |
|---|---|
| litellm/proxy/pass_through_endpoints/pass_through_endpoints.py | Replaces the broken remove_endpoint_routes(endpoint_key) call with dict.pop(endpoint_key, None) — correctly fixing O(1) registry cleanup; however, the corresponding LiteLLMRoutes.openai_routes cleanup promised in the PR description is absent, leaving a parallel unbounded-growth issue for auth-enabled subpath endpoints. |
| tests/test_litellm/proxy/pass_through_endpoints/test_pass_through_endpoints.py | Adds three regression tests for the registry-growth bug, but all tests directly call dict.pop() on the module-level dict rather than going through initialize_pass_through_endpoints, so they verify Python dict semantics rather than the production code path. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[periodic reload every ~30s] --> B[initialize_pass_through_endpoints]
B --> C[snapshot current registry keys]
B --> D[register each DB and config endpoint]
D --> E{route already registered?}
E -- yes --> F[update metadata in registry]
E -- no --> G[add_exact_path_route or add_subpath_route]
G --> H[write to _registered_pass_through_routes]
G --> I{auth enabled?}
I -- yes --> J[append path and wildcard to openai_routes list]
B --> K[cleanup loop: stale keys not in visited set]
K --> L[_registered_pass_through_routes.pop key — FIXED O1]
K --> M[openai_routes cleanup — MISSING unbounded growth]
style M fill:#f96,stroke:#c33
style L fill:#6f6,stroke:#393
Reviews (1): Last reviewed commit: "fix(pass-through): pop stale route key d..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| for endpoint_key in registered_pass_through_endpoints: | ||
| if endpoint_key not in visited_endpoints: | ||
| InitPassThroughEndpointHelpers.remove_endpoint_routes(endpoint_key) | ||
| _registered_pass_through_routes.pop(endpoint_key, None) | ||
| verbose_proxy_logger.debug( | ||
| "Removed stale pass-through route from registry: %s", | ||
| endpoint_key, | ||
| ) |
There was a problem hiding this comment.
LiteLLMRoutes.openai_routes cleanup
The PR description explicitly states the fix "also properly cleans up LiteLLMRoutes.openai_routes entries (both exact paths and /* wildcard paths for subpath routes), matching the behavior of the existing remove_endpoint_routes." However, the actual implementation only pops from _registered_pass_through_routes without touching LiteLLMRoutes.openai_routes.value.
For auth-enabled endpoints registered with include_subpath=True, each reload cycle appends the path and its /* wildcard to openai_routes (lines 2180–2216), but the stale entries are never removed in this loop. This creates the same class of unbounded growth — just in openai_routes instead of _registered_pass_through_routes.
route_checks.py iterates LiteLLMRoutes.openai_routes.value on every request for fuzzy matching (line 317), so the accumulation will cause the same O(n) per-request degradation the PR aims to fix for _registered_pass_through_routes.
The existing remove_endpoint_routes (used for explicit delete/update flows) shows the intended cleanup pattern. The stale-route removal loop should also inspect the popped route_info and mirror that cleanup:
for endpoint_key in registered_pass_through_endpoints:
if endpoint_key not in visited_endpoints:
route_info = _registered_pass_through_routes.pop(endpoint_key, None)
if route_info is not None:
path = route_info.get("path")
if isinstance(path, str):
openai_routes_list = LiteLLMRoutes.openai_routes.value
if path in openai_routes_list:
openai_routes_list.remove(path)
if route_info.get("type") == "subpath":
wildcard_path = path.rstrip("/") + "/*"
if wildcard_path in openai_routes_list:
openai_routes_list.remove(wildcard_path)
verbose_proxy_logger.debug(
"Removed stale pass-through route from registry: %s",
endpoint_key,
)| def test_pop_removes_stale_route_by_key(self): | ||
| """Stale route is removed in O(1) by its route key via dict.pop().""" | ||
| route_key = "old-uuid:exact:/my-endpoint:GET,POST" | ||
| _registered_pass_through_routes[route_key] = { | ||
| "endpoint_id": "old-uuid", | ||
| "path": "/my-endpoint", | ||
| "type": "exact", | ||
| } | ||
|
|
||
| _registered_pass_through_routes.pop(route_key, None) | ||
|
|
||
| assert route_key not in _registered_pass_through_routes | ||
|
|
||
| def test_pop_noop_for_unknown_key(self): | ||
| """pop() with default None does not raise for nonexistent keys.""" | ||
| _registered_pass_through_routes["keep-me:exact:/a:GET"] = { | ||
| "endpoint_id": "keep-me", | ||
| "path": "/a", | ||
| "type": "exact", | ||
| } | ||
|
|
||
| _registered_pass_through_routes.pop("nonexistent:exact:/b:GET", None) | ||
|
|
||
| assert len(_registered_pass_through_routes) == 1 | ||
|
|
||
| def test_registry_does_not_grow_across_reload_cycles(self): | ||
| """Simulate multiple reload cycles with changing UUIDs. | ||
|
|
||
| Core regression test: without the fix the registry grows by N entries | ||
| every cycle; with the fix it stays constant. | ||
| """ | ||
| path = "/vertex-passthrough" | ||
| methods_str = "GET,POST" | ||
| num_cycles = 50 | ||
|
|
||
| for cycle in range(num_cycles): | ||
| old_keys = list(_registered_pass_through_routes.keys()) | ||
|
|
||
| new_uuid = f"uuid-{cycle}" | ||
| new_exact_key = f"{new_uuid}:exact:{path}:{methods_str}" | ||
| new_subpath_key = f"{new_uuid}:subpath:{path}:{methods_str}" | ||
|
|
||
| _registered_pass_through_routes[new_exact_key] = { | ||
| "endpoint_id": new_uuid, | ||
| "path": path, | ||
| "type": "exact", | ||
| } | ||
| _registered_pass_through_routes[new_subpath_key] = { | ||
| "endpoint_id": new_uuid, | ||
| "path": path, | ||
| "type": "subpath", | ||
| } | ||
|
|
||
| visited = {new_exact_key, new_subpath_key} | ||
|
|
||
| # Clean up stale routes — the fix: pop by key in O(1) | ||
| for key in old_keys: | ||
| if key not in visited: | ||
| _registered_pass_through_routes.pop(key, None) | ||
|
|
||
| # After 50 cycles, only the last cycle's 2 keys should remain |
There was a problem hiding this comment.
All three new tests directly manipulate _registered_pass_through_routes and call dict.pop() on it themselves. They do not call initialize_pass_through_endpoints or any production function. For example, test_pop_removes_stale_route_by_key invokes _registered_pass_through_routes.pop(route_key, None) directly — this only verifies that Python's dict works, not that the production code actually uses it.
The core regression test test_registry_does_not_grow_across_reload_cycles re-implements the cleanup loop inline rather than invoking the real initialize_pass_through_endpoints. If someone later refactors that function (e.g., accidentally re-introduces the old O(n) scan), the tests will continue to pass.
Consider mocking out the DB + FastAPI app interactions and calling initialize_pass_through_endpoints (or at minimum the cleanup slice of it) so the tests actually cover the production code path that was broken.
There was a problem hiding this comment.
Already fixed in 7b2a7e0 — result_rows = [] was moved to the top of Section 5 (before the message-building loop) and the duplicate initialization in Section 6 was removed.
2 participants
Relevant issues
Fixes #24833
Pre-Submission checklist
tests/test_litellm/directory, Adding at least 1 test is a hard requirement - see detailsmake test-unit@greptileaiand received a Confidence Score of at least 4/5 before requesting a maintainer reviewType
🐛 Bug Fix
Changes
One-line fix in
initialize_pass_through_endpoints(line ~2294):Root cause: When pass-through endpoints stored in DB have no
idfield, each 30s reload generates a new UUID. The old cleanup (remove_endpoint_routes) compared the full route key againstendpoint_id(UUID only) — they never matched, so stale entries were never deleted. The dict grew without bound, making cleanup O(n²) and eventually pinning CPU at 100%.Fix: Replace the
remove_endpoint_routes()call with a directdict.pop()on the route key, which removes the stale entry in O(1).Tests added
3 unit tests in
TestRemoveStaleEndpointRoute:test_pop_removes_stale_route_by_key— verifies O(1) removaltest_pop_noop_for_unknown_key— verifies no-op for nonexistent keystest_registry_does_not_grow_across_reload_cycles— core regression test: simulates 50 reload cycles with changing UUIDs and asserts the registry stays at constant size