-
-
Notifications
You must be signed in to change notification settings - Fork 11.7k
fix(pass-through): remove stale routes by key to prevent unbounded registry growth #24846
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,7 @@ | |
|
|
||
| from litellm.proxy.pass_through_endpoints.pass_through_endpoints import ( | ||
| HttpPassThroughEndpointHelpers, | ||
| _registered_pass_through_routes, | ||
| pass_through_request, | ||
| ) | ||
| from litellm.proxy.pass_through_endpoints.success_handler import ( | ||
|
|
@@ -2474,3 +2475,82 @@ async def mock_httpx_request(method, url, **kwargs): | |
| # Verify the response | ||
| assert response.status_code == 200 | ||
| async_client.request.assert_called_once() | ||
|
|
||
|
|
||
| class TestRemoveStaleEndpointRoute: | ||
| """Regression tests for https://github.com/BerriAI/litellm/issues/24833: | ||
| When pass-through endpoints stored in the DB have no ``id`` field, each | ||
| periodic reload generates a new UUID, causing ``_registered_pass_through_routes`` | ||
| to grow without bound because the old cleanup path | ||
| (``remove_endpoint_routes``) scanned by ``endpoint_id`` and never matched | ||
| the stale entries. The fix uses dict.pop() to remove by route key in O(1). | ||
| """ | ||
|
|
||
| def setup_method(self): | ||
| _registered_pass_through_routes.clear() | ||
|
|
||
| def teardown_method(self): | ||
| _registered_pass_through_routes.clear() | ||
|
|
||
| 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 | ||
|
Comment on lines
+2495
to
+2555
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
All three new tests directly manipulate The core regression test Consider mocking out the DB + FastAPI app interactions and calling There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Already fixed in 7b2a7e0 β |
||
| assert len(_registered_pass_through_routes) == 2 | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LiteLLMRoutes.openai_routescleanupThe PR description explicitly states the fix "also properly cleans up
LiteLLMRoutes.openai_routesentries (both exact paths and/*wildcard paths for subpath routes), matching the behavior of the existingremove_endpoint_routes." However, the actual implementation only pops from_registered_pass_through_routeswithout touchingLiteLLMRoutes.openai_routes.value.For auth-enabled endpoints registered with
include_subpath=True, each reload cycle appends the path and its/*wildcard toopenai_routes(lines 2180β2216), but the stale entries are never removed in this loop. This creates the same class of unbounded growth β just inopenai_routesinstead of_registered_pass_through_routes.route_checks.pyiteratesLiteLLMRoutes.openai_routes.valueon 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 poppedroute_infoand mirror that cleanup: