Repository navigation
perf(router): fetch cooldown state and usage counters in one Redis round trip - #43320
Conversation
…und trip The cooldown filter (CooldownCache) and usage-based-routing-v2 selection (LowestTPMLoggingHandler_v2) each issued their own MGET on every request because they live in different objects. RoutingReadBatch fetches both key sets through DualCache.async_batch_get_cache_shared while the healthy deployments are resolved and hands the usage slice to the strategy, so selection does not read again. Each cache keeps its own memory tier, throttling, reservation rollback and circuit-breaker handling, and the strategy falls back to its own read when the prefetch does not cover its keys. simple-shuffle keeps reading only cooldowns. aresponses no longer issues a second, blocking response-cache read from the worker thread that runs the sync wrapper. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
|
|
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Wrap the memory-tier prepare and backfill steps of DualCache.async_batch_get_cache_shared so a failing tier degrades that cache's read to None the way async_batch_get_cache does, instead of escaping into routing. Drop the aresponses sync-cache guard: for native Responses models the worker-thread read is the one whose key matches the write, so skipping it broke cached /v1/responses replays. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…eck reads it as a key helper Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
@greptileai review 31f81c5 please: 8d55b19 wraps the per-cache tier steps of the shared read and drops the |
…ison Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
@greptileai review 5bb2c36 please: 5bb2c36 narrows the daily-report cache values in |
… results without mutation Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
@greptileai review 464b584 please: 464b584 types |
…_redis_round_trip
…_redis_round_trip
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
@greptileai please re-review at fedb248. Since the 5/5 on 464b584 the branch merged origin/main (ed3e4c4) and fixed one import-sort lint in tests/unit/caching/test_dual_cache.py (fedb248); no functional change to litellm/. |
…_redis_round_trip
…omprehension Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
bugbot run |
…_redis_round_trip
|
bugbot run |
1 similar comment
|
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 a71e6ff. Configure here.
TLDR
Problem this solves:
usage-based-routing-v2request paid two Redis round trips before the provider callHow it solves it:
RoutingReadBatchfetches cooldown keys and tpm/rpm counters in one MGETDualCache.async_batch_get_cache_sharedkeeps each cache's memory tier and failure handlingUser Flow
Before: a request on a
usage-based-routing-v2group spends two Redis round trips on routing before the provider is called/v1/messages,/v1/responses) for a model group with two deploymentsMGET deployment:*:cooldown, then a secondMGET <id>:<model>:tpm:<minute>, <id>:<model>:rpm:<minute>, then the response-cache GETAfter: the same request spends one Redis round trip on routing
MGETcarrying the cooldown keys and the tpm/rpm counter keys together, then the response-cache GETRelevant issues
Follow-up to #40841 (post-call I/O consolidation), whose body names the router cache reads as the remaining pre-call round trips. Fleet numbers for the perf track: #40539 and https://berriai.grafana.net/d/litellm-perf-1k-rps?from=1789028100000&to=1789033500000
Linear ticket
Resolves LIT-8693
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
uv run pytest tests/unit/<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)Design notes
Which path fires the usage MGET
Traced on a local proxy with
redis-cli monitorand a call-site tracer onRedisCache:simple-shufflewith norpm/tpmon the deployments: one cooldown MGET, zero usage reads. Unchanged by this PRusage-based-routing-v2(router-wide, per routing group, or per-keyrouting_strategyoverride through_get_routing_context): cooldown MGET fromCooldownCache.async_get_active_cooldownsplus the counter MGET fromLowestTPMLoggingHandler_v2.async_get_available_deployments. This is the two-read shape in the prod traces and the path this PR collapsesrpm/tpmset undersimple-shuffledo not read counters at routing time; they only write them (INCRBYFLOAT global_router:*after the call). The model-group limit read (get_model_group_usage) only fires whenmodel_group_rpm/tpmlimits are configuredRoot cause in one sentence: the cooldown filter and the usage-based selector each own their own MGET because they live in different objects (
CooldownCacheover its ownDualCache, the strategy over the router cache), and the response-cache GET is issued serially after routing although it does not depend on it.Option picked: (b) a per-request
RoutingReadBatchRouter.async_get_available_deploymentcreates aRoutingReadBatchright after_get_routing_contextwhen the resolved strategy isusage-based-routing-v2, and passes it down:Why not (a) inside
_async_get_cooldown_deployments: the cooldown helper has no view of the strategy, andasync_get_healthy_deploymentshas several callers (fallbacks, health probes,_select_deployment_asynccallers outside the main path) that must keep the old single read. The batch object is created in exactly one place and everything else keeps its existing signature with an optional parameter, so every other caller is byte-for-byte the old path.The two key sets live in two different
DualCacheinstances (cooldown store vs router cache) that share oneRedisCache.DualCache.async_batch_get_cache_sharedlets each cache consult its own in-memory tier andredis_batch_cache_expiryreservation independently, merges only the keys that still need Redis into oneRedisCache.async_batch_get_cache, then hands each cache its slice for the normal backfill into memory. If the two caches do not share a Redis client it falls back to two independent reads.Semantics preserved:
[tpm..., rpm...]values in its own key order and runs the unchanged_common_checks_available_deploymentactive_cooldowns_from_resultsis the old tail ofasync_get_active_cooldowns, factored out so both paths interpret results identically (expiry check, stale-key delete)async_get_healthy_deploymentsexactly like the old cooldown MGET did, ending in the sameRateLimitError: No deployments available; a circuit-breaker-open read serves memory-only for both caches, like beforePrefetchedUsage.covers()fails and the strategy issues its own read, so it can never use stale keyscovers()holds after filteringResponse-cache GET: left serial, not made concurrent
The cache lookup happens in
litellm.utils.client(_async_get_cache) beforeRouterrouting starts for the call itself, in a different layer: routing runs inside the routedacompletion, so the two are not siblings that could be gathered without restructuring theclientwrapper or moving cache lookup into the router. The key is independent of the chosen deployment (it hashes the request), so the reorder is possible in principle, but it touches the cache handler for every call type and is out of scope here; noted as a follow-up on LIT-8693./v1/responsessyncget_cache: left in placeThe prod trace's
get_cache <- _sync_get_cachecomes fromaresponsesrunning the syncresponses()wrapper in a worker thread (_worker <- _bootstrap_innerin the call-site trace below), so it is not a blocking call on the event loop. An earlier revision of this PR skipped it foraresponses, andtests/unit/test_utils.py::test_wrapper_async_replays_cached_converted_responses_stream_as_streamfailed: for native Responses API models the sync read is the one whose key matches what the write stores, so removing it turns every cached/v1/responsesstream into a provider call. The asyncaresponsesread and the write are keyed differently (the live trace below shows the async GET on one key and the SET on another). That key mismatch is the actual defect, it is outside routing, and it is recorded as a follow-up on LIT-8693. This PR does not touchlitellm/utils.py.Alerting file touched by the type gate
DualCache.async_batch_get_cacheused to return an untyped list (its element type was Unknown to basedpyright); the shared read gives it a typedlist[object | None], and the type-check gate then surfaced six latent diagnostics inSlackAlerting.send_daily_reports, which compared and sorted those elements as numbers. The values are narrowed there withisinstance(val, (int, float))before use; non-numeric or missing entries becomeNone, exactly what theall_noneand placeholder logic already handled. No behavior change on numeric values;tests/unit/integrations/SlackAlerting/test_slack_alerting.pyandtests/logging_callback_tests/test_alerting.py -k dailypass (11 tests, the Redis one against a local Redis)Out of scope, recorded on LIT-8693
redis async_get_cache <- _get_routed_model <- async_pre_call_hook(parallel request limiter) is a separate read on a separate path; not touched, per the ticket's instruction not to widen intoparallel_request_limiter_v3without a proven duplicate/v1/responses: the asyncaresponsescache read and the cache write use different keys, so the effective read for native Responses models is the sync one on the worker thread; aligning the keys would remove one GET per requestScreenshots / Proof of Fix
Admin UI, head fedb248
Logs page of the proxy started from this branch (
PYTHONPATH=<checkout>,usage-based-routing-v2, Redis 7, real Anthropicclaude-sonnet-4-6). The row is requestchatcmpl-da0eae5d-057d-4acf-bb79-0024bd021144; theredis-cli monitortrace for that request contains exactly one routingMGET, carrying the cooldown key and the tpm/rpm keys together:Local proxy, real Redis 7 on 127.0.0.1:6379, real Anthropic
claude-sonnet-4-6via a key from the environment, model groupclaude-groupwith two deployments (claude-dep-a,claude-dep-b), plusclaude-rpmwith two deployments carryingrpm/tpmlimits. Both arms run the sameconfig_v2.yaml(routing_strategy: usage-based-routing-v2,cache: truewithcache_params.type: redis,optional_pre_call_checks: [prompt_caching]) and the same fixture bytes, started withPYTHONPATH=<checkout>,redis-cli monitorcapturing to a file, and each request wrapped betweenSET __marker:<arm>:<case>:start|endso the per-request command list is exact. Each request sleeps 11 s first so the 10 sredis_batch_cache_expirythrottle cannot serve it from memory.Shared setup:
The lists below are the Redis commands between the two markers, in order, until the first post-call write (
INCRBYFLOAT/SET). Everything after that is the same on both arms: response-cacheSET, spendMGET/INCRBYFLOAT/EXPIRE, tpm counter increments, and the key/user/model tokenEVALSHA/INCRBYFLOAT.Before (14f4c34,
mainat the merge base)POST /v1/chat/completions
curl ... /v1/chat/completions -d @chat.json->200, body"model":"claude-group", content"pong""stream":true:200, SSE chunks with"model":"claude-group"; commands 1 and 2 are the two MGETs (4 counter keys), then the response-cache GETPOST /v1/messages
curl ... /v1/messages -d @messages.json->200, body"model":"claude-group",content[0].text="pong"(the second MGET carries fewer keys when the in-memory tier already holds some counters; the round trip still happens)
3.
"stream":true:200,event: message_startwith"model":"claude-group"; two MGETs (6 + 2 keys) then the GETPOST /v1/responses
curl ... /v1/responses -d @responses.json->200,"id":"resp_..."[REDIS get_cache SYNC] :: _sync_get_cache <- _worker <- _bootstrap_inner <- _bootstrapx3 (one per non-stream /v1/responses request, on a thread),async_get_available_deployments <- _select_deployment_asyncx18,async_get_active_cooldowns <- _async_get_cooldown_deploymentsx18rpm-limited group (claude-rpm), chat and /v1/messages
200on both; two MGETs (6 cooldown keys, then 4 or 2 counter keys forclaude-rpm-a/claude-rpm-b) before the response-cache GETAfter (branch tip, first measured at 078b26d)
POST /v1/chat/completions
curl ... /v1/chat/completions -d @chat.json->200, body"model":"claude-group", content"pong""stream":true:200, SSE"model":"claude-group"; one MGET (8 keys), then the GETPOST /v1/messages
curl ... /v1/messages -d @messages.json->200,"model":"claude-group",content[0].text="pong""stream":true:200,event: message_startwith"model":"claude-group"; one MGET (9 keys), then the GETPOST /v1/responses
Re-captured at 8d55b19 (the tip that keeps the worker-thread read; later commits only rename a helper, narrow the alerting values and type/de-mutate the two
DualCachehelpers, no routing or Redis-traffic change), same fixture,redis-cli flushdbbefore the run:curl ... /v1/responses -d @responses.json->200,"id":"resp_...","model":"claude-group"The
SETafter the call writes6bf0...(the bridge's chat-completion key), never69fa...; the second request in the same run served the response from cache with only the routing MGET reaching Redis3.
"stream":true:200,"model":"claude-sonnet-4-6"in the SSE events; one MGET, thenGET fa8c...(async),GET 5521...(sync, worker thread),GET 5814...(bridge chat key, the one that is written and hit on the second request)4. Call-site tracer over the original after run:
async_get_cooldown_deployments <- async_get_healthy_deploymentsx18,async_get_available_deployments <- _select_deployment_asyncx0rpm-limited group (claude-rpm), chat and /v1/messages
200on both; one MGET (10 keys: 6 cooldown +claude-rpm-a/claude-rpm-btpm and rpm) before the response-cache GETsimple-shuffle control (config_shuffle.yaml: same file without
routing_strategy), before and afterIdentical on both arms for every case:
200, thenMGET 6 keys: deployment:*:cooldown, then the response-cacheGET, and no counter read (theclaude-rpmgroup only writesINCRBYFLOAT global_router:*:tpmafter the call). This PR does not create aRoutingReadBatchfor simple-shuffle.Local latency A/B
Same box, same
config_v2.yaml, 50 concurrent closed-loop clients for 60 s against themock-groupmodel (mock_response, so no provider network), one run per arm, back to back:Regime: a single 8-vCPU box running the proxy, Redis and the load generator together, CPU-bound on the proxy at ~98 rps, with the fixture repeated so it is a response-cache hit after the first request (
cmdstat_getdid not move on either arm: the hit is served from memory). This run measures nothing about routing and is reported only because the ticket asked for the A/B; a second run with"cache": {"no-cache": true}so every request routes is below.Second run, same rig, fixture with
"cache": {"no-cache": true}so every request routes (and the counter MGET fires when the 10 sredis_batch_cache_expirythrottle allows a Redis read):No measurable difference at this regime: Redis is on loopback (
usec_per_callabout 3 us for MGET on both arms), the proxy is CPU-bound at ~83 rps on the shared box, and the in-memory tier plus the 10 s batch throttle mean only about one MGET per second reaches Redis on either arm. The saving this PR buys is one network round trip on the requests that do reach Redis (1.7 to 4.3 ms per MGET in the production traces that motivated the ticket), which a loopback box cannot show. Do not read these numbers as the production effect either way.Production traces (tip 275197b deployed)
Chart
0.0.0-branch-litellm-router-single-redis-round-275197bbuilt from this branch (https://github.com/BerriAI/litellm-ops/actions/runs/36264994770) and pinned on the production cluster via https://github.com/BerriAI/litellm-ops/pull/189: Argo Synced/Healthy at 19:56 UTC, migrations clean, gateway 2/2 with 0 restarts, real-model smoke 200 on chat,/v1/responses,/v1/messagesandkey/list. The DBrouter_settingson that cluster resolve tousage-based-routing-v2Fresh Tempo traces right after the old pods drained, one per surface, from the same tagged run (
otelverify1790452780, 19:59:40 UTC):Before this build the same three requests carried two routing MGETs each (
async_get_active_cooldownsat 4.1 ms plusasync_get_available_deploymentsat 1.6 ms, trace418e25ae328841906888e2fbee8959c1on the previous build). A TraceQL search forspan.litellm.service.caller=~"async_get_active_cooldowns.*|async_get_available_deployments.*"since the new pods took traffic returns zero spans,name=~".*<-.*"also returns zero, and every post-response Redis/Postgres write is still a linked root trace (5, 4 and 2 detached spans respectively), so the #43237 phase-based detach shipped with this branch is intactPerf-track context
This change is one of the pre-call items behind the 1k rps perf track (#40539, Grafana https://berriai.grafana.net/d/litellm-perf-1k-rps?from=1789028100000&to=1789033500000). Those fleet numbers were measured on the integration branch as a whole and do not isolate this PR; the numbers above are the only measurements of this change on its own.
Tests and mutation check
test_routing_read_batch.pydrives the realRouterwith aMagicMock(spec=RedisCache)that records everyasync_batch_get_cachecall and asserts exactly one Redis round trip perasync_get_available_deploymentonusage-based-routing-v2, still one on simple-shuffle, plus parity: same deployment chosen as the unbatched selector for fixed counter values, cooled deployments still excluded, Redis raising still ends inRateLimitError: No deployments available.Mutants (each reverted with
cpbackups, restore verified withcmp -s):RoutingReadBatch.for_strategyreturns None: round-trip test sees 2 reads, 2 failedPrefetchedUsage.coversforced False: 1 failedLive re-check at a71e6ff
This head adds the merge of main 7f95b5f into 8432eae. A read-count harness, run from this checkout with
PYTHONPATH=<checkout>, one warm request then one request per case,redis_reads_processedfromINFO statsminus the three marker commandsEvery response body was
pongfromclaude-groupwith usage populated, and the proxy log has no ERROR or Traceback linesType
🚄 Infrastructure
Caveats (if any)
Low
LowestTPMLoggingHandler_v2.async_pre_call_checkstill runs on the chosen deployment as before; TPM had no near-call check on either versionusage-based-routing-v2is batched; other counter-reading strategies keep their own readCI Status
Head a71e6ff = 8432eae + merge of origin/main 7f95b5f. The only failing check is
misc / Run tests, four tests in tests/unit/interactions/test_openapi_compliance.py that read a remote OpenAPI spec; the same job fails the same way on main at 7f95b5fLink to Devin session: https://app.devin.ai/sessions/f3b64ab771264ecf9b474add1ce663de
Open in Devin Desktop: https://app.devin.ai/desktop/session/f3b64ab771264ecf9b474add1ce663de?variant=devin
Requested by: @yassin-berriai
Note
Medium Risk
Changes pre-call routing and shared Redis read failure semantics for usage-based-routing-v2; behavior is heavily tested for parity but touches core deployment selection on every routed request.
Overview
Collapses two Redis MGETs into one for
usage-based-routing-v2by batching cooldown keys and tpm/rpm counter keys before deployment selection.Adds
DualCache.async_batch_get_cache_sharedso separateDualCacheinstances (cooldown store vs router cache) still use their own in-memory tier, throttling, and backfill while sharing a single RedisMGET.RoutingReadBatchruns that shared read during healthy-deployment resolution and passes counters toLowestTPMLoggingHandler_v2viaPrefetchedUsage, so selection skips a second cache read when keys are already covered.simple-shuffleand other strategies keep the existing single cooldown read.async_batch_get_cacheis refactored into prepare/apply helpers;CooldownCache.active_cooldowns_from_resultsis extracted so batched cooldown results parse the same way as before. Slack daily reports narrow batch cache values to numeric types for typing only.Reviewed by Cursor Bugbot for commit a71e6ff. Bugbot is set up for automated code reviews on this repo. Configure here.