fix(router): clamp least_busy request counter to prevent negative drift - #25325
fix(router): clamp least_busy request counter to prevent negative drift#25325rudra717 wants to merge 3 commits into
Conversation
The least_busy router strategy decrements a per-deployment request counter on success/failure callbacks. Under race conditions (callback fires before pre-call, or fires twice), the counter can go negative. A negative count is always less than the 0 assigned to unused deployments, so the negative-count deployment attracts ALL traffic while others starve to zero requests. Fix: clamp the counter with max(value - 1, 0) on all 4 decrement paths (sync success, sync failure, async success, async failure). Fixes BerriAI#25323 Co-Authored-By: Claude <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Addresses Codecov 0% patch coverage. Tests verify: - sync success callback clamps counter at 0 (not -1) - sync failure callback clamps counter at 0 - multiple duplicate callbacks never push counter below 0 - counter decrements normally from positive values (1 -> 0)
Greptile SummaryThis PR applies a one-line defensive clamp ( Confidence Score: 5/5Safe to merge — minimal, correct fix with full test coverage across all four changed paths. The change is a one-liner defensive clamp applied identically to all four symmetrical decrement paths. Tests cover every path (sync success, sync failure, async success, async failure) using a proper FakeCache stub that implements both sync and async cache methods. Prior review concerns about the async test path and the removed asyncio.coroutine usage have been addressed in this version. No security, data-integrity, or breaking-change concerns. No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/router_strategy/least_busy.py | Adds max(value - 1, 0) clamping on all four decrement paths — log_success_event, log_failure_event, async_log_success_event, async_log_failure_event — preventing negative counter drift in the least-busy routing strategy. |
| tests/test_litellm/test_least_busy_counter_clamp.py | New unit-test file with five tests covering all four changed decrement paths (two sync, two async) plus a multi-decrement scenario; FakeCache correctly implements both sync and async cache methods, and async tests properly await the async handlers. |
Sequence Diagram
sequenceDiagram
participant R as Router
participant H as LeastBusyLoggingHandler
participant C as DualCache
R->>H: log_pre_api_call(kwargs)
H->>C: get_cache(model_group_request_count)
C-->>H: {deploy_id: N}
H->>C: set_cache({deploy_id: N+1})
Note over R,C: Request in flight...
alt Success or Failure callback
R->>H: log_success_event / log_failure_event
H->>C: get_cache(model_group_request_count)
C-->>H: {deploy_id: M}
Note over H: counter = max(M - 1, 0) <- NEW clamp
H->>C: set_cache({deploy_id: max(M-1, 0)})
end
R->>H: get_available_deployments(model_group)
H->>C: get_cache(model_group_request_count)
C-->>H: {deploy_id: counter >= 0}
H-->>R: deployment with min traffic (never negative)
Reviews (2): Last reviewed commit: "test(router): fix async tests to actuall..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @pytest.mark.asyncio | ||
| async def test_async_success_counter_never_goes_negative(): | ||
| """async_log_success_event should clamp the counter at 0.""" | ||
| cache = MagicMock() | ||
| cache.async_get_cache = pytest.importorskip("asyncio").coroutine( | ||
| lambda *a, **kw: None | ||
| ) | ||
|
|
||
| # Use a real FakeCache but wrap async methods | ||
| real_cache = FakeCache() | ||
| model_group = "test-group" | ||
| deploy_id = "deploy-1" | ||
| cache_key = f"{model_group}_request_count" | ||
| real_cache.store[cache_key] = {deploy_id: 0} | ||
|
|
||
| handler = LeastBusyLoggingHandler(router_cache=real_cache) | ||
|
|
||
| # Patch sync cache methods to work (async methods call sync internally) | ||
| kwargs = _make_kwargs(model_group, deploy_id) | ||
|
|
||
| # Test the sync path which is equivalent | ||
| handler.log_success_event(kwargs, None, None, None) | ||
| assert real_cache.store[cache_key][deploy_id] == 0 |
There was a problem hiding this comment.
test_async_success_counter_never_goes_negative never calls async_log_success_event — it falls back to handler.log_success_event (the sync version) on line 100. The cache MagicMock and the asyncio.coroutine setup on lines 82-85 are completely unused; the handler is initialised with real_cache (a FakeCache that has no async_get_cache/async_set_cache methods), and the test then calls the sync method. Two of the four changed decrement paths (async_log_success_event, async_log_failure_event) have zero effective test coverage here.
A minimal correct replacement:
@pytest.mark.asyncio
async def test_async_success_counter_never_goes_negative():
"""async_log_success_event should clamp the counter at 0."""
class AsyncFakeCache(FakeCache):
async def async_get_cache(self, key, **kwargs):
return self.store.get(key)
async def async_set_cache(self, key, value, **kwargs):
self.store[key] = value
cache = AsyncFakeCache()
handler = LeastBusyLoggingHandler(router_cache=cache)
model_group = "test-group"
deploy_id = "deploy-1"
cache_key = f"{model_group}_request_count"
cache.store[cache_key] = {deploy_id: 0}
kwargs = _make_kwargs(model_group, deploy_id)
await handler.async_log_success_event(kwargs, None, None, None)
assert cache.store[cache_key][deploy_id] == 0, "Counter went negative"There should be a matching test for async_log_failure_event as well.
Rule Used: What: Flag any modifications to existing tests and... (source)
| cache.async_get_cache = pytest.importorskip("asyncio").coroutine( | ||
| lambda *a, **kw: None | ||
| ) |
There was a problem hiding this comment.
asyncio.coroutine removed in Python 3.11
pytest.importorskip("asyncio").coroutine accesses asyncio.coroutine, which was deprecated in Python 3.8 and removed in Python 3.11 (PEP 530). On any Python 3.11+ environment this line raises AttributeError: module 'asyncio' has no attribute 'coroutine', causing the test to error rather than skip or pass. Since the whole block is dead code anyway (see the parallel comment), it should just be deleted.
Greptile review: the async test was calling sync path and used deprecated asyncio.coroutine (removed in Python 3.11). Now FakeCache has async methods, tests call async_log_success_event and async_log_failure_event directly. All 4 changed paths covered.
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
|
Not stale until merged |
2 participants
Summary
Fixes the least_busy routing strategy silently suppressing traffic to certain deployments by clamping the per-deployment request counter to prevent negative values.
Root Cause
The request counter is incremented in
log_pre_api_calland decremented in success/failure callbacks. Under race conditions (callback fires before pre-call, or fires twice), the counter goes negative.A negative count is always less than the
0assigned to fresh/unused deployments in_get_available_deployments, so the negative-count deployment wins every comparison and attracts ALL traffic, while others gradually starve to zero.Fix
Added
max(value - 1, 0)on all 4 decrement paths:log_success_event(sync)log_failure_event(sync)async_log_success_eventasync_log_failure_eventThis ensures the counter never goes below 0, preventing any single deployment from monopolizing traffic.
Testing
The fix is a defensive floor guard on an integer counter. Existing routing tests validate the least_busy selection logic.
Disclaimer
AI agents (Claude Code) assisted with this contribution.
Fixes #25323