Repository navigation
fix(cache): write DualCache's redis tier with default_redis_ttl - #43293
apex-mochen wants to merge 3 commits into
Conversation
|
| if "ttl" not in redis_kwargs and self.default_redis_ttl is not None: | ||
| redis_kwargs["ttl"] = self.default_redis_ttl | ||
| self.redis_cache.set_cache(key, value, **redis_kwargs) |
There was a problem hiding this comment.
This fix changes the Redis TTL passed by the sync, async, and pipeline writes, but the existing DualCache tests check only in-memory expiry. A test with different defaults for each tier should assert the Redis TTL in all three paths; without it, the original bug could return without a test failing.
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!
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
lets-order-some-fries
left a comment
There was a problem hiding this comment.
Both proxy-infra failures are the #43187 behaviour, pinned on purpose for the auth cache, so they need a decision rather than a test edit.
tests/test_litellm/proxy/common_utils/test_user_api_key_cache.py:142-160 and :179-190 (at 14f4c34c61) build UserApiKeyCache(redis_cache=fake, default_in_memory_ttl=60) (and 45) with no Redis TTL and assert the Redis write gets the in-memory value. At this head the fake gets no ttl at all: assert None == 60 and assert None == 45 in the job log, 2 failed / 12731 passed. Locally that file is 32 passed on 14f4c34c61 and 2 failed / 30 passed on 2a2e9af8c6.
Both tests came in with #26202, which put user_api_key_cache on Redis. The docstring says "Redis should receive the same TTL as memory (matches proxy defaults: enum 60s)", and the same PR wrote proxy_server.py:6400-6402: mirror the TTL on Redis, "otherwise DualCache misses in memory fall back to a key that outlasts user_api_key_cache_ttl". It also added the default_redis_ttl=litellm.default_redis_ttl at proxy_server.py:4745-4748, but DualCache never read that value, so what actually held is what the tests and that comment describe: Redis mirrors the in-memory TTL.
Loading each config with cache: true (type: redis) and enable_redis_auth_cache: true, and recording what a real RedisCache sends:
| config | Redis EX on main |
Redis EX at this head |
|---|---|---|
| defaults | 60 | 60 |
litellm_settings.default_redis_ttl: 3600 |
60 | 3600 |
general_settings.user_api_key_cache_ttl: 300 |
300 | 300 |
That is for auth-cache writes without an explicit ttl, e.g. the team-membership row at litellm/proxy/auth/auth_checks.py:2357-2361 and the SSO user row at litellm/proxy/management_endpoints/ui_sso.py:1930-1934. Key and team objects go through _cache_management_object, which passes ttl=get_management_object_ttl(...) (auth_checks.py:2779-2784), so they keep the in-memory TTL either way. The defaults row only matches because RedisCache has its own 60s fallback (litellm/caching/redis_cache.py:671-674, litellm/caching/base_cache.py:23). The 3600 row needs default_redis_ttl listed before cache: true, because Redis is attached to the auth cache while cache is processed; listed after it, both columns read 60.
One way to keep the auth-cache contract without special-casing DualCache: UserApiKeyCache falls back to its in-memory TTL for Redis, and the proxy stops passing the global on attach.
# litellm/proxy/common_utils/user_api_key_cache.py, UserApiKeyCache.__init__
redis_ttl: Final = default_redis_ttl if default_redis_ttl is not None else default_in_memory_ttl
# pass redis_ttl as default_redis_ttl to super().__init__() and to key_object_cache
# litellm/proxy/proxy_server.py:4745-4748
user_api_key_cache.attach_redis_cache(redis_cache)On top of this head that gives 60 / 60 / 300 for the three rows, both tests pass unchanged, and tests/unit/caching/test_dual_cache.py, test_user_api_key_cache.py and test_redis_auth_cache_flag.py pass (85, including the test below). If maintainers would rather the auth cache follow default_redis_ttl, updating the two tests is the other route, but that should be their call, stated in the PR.
The fix itself has no test (Greptile flagged this too). This fails on 14f4c34c61 (the three tier_defaults cases: Redis gets 60, not 3600) and passes on this head. It fits beside the other TTL tests in tests/unit/caching/test_dual_cache.py, which already has every import it needs:
async def _write_through(dual_cache: DualCache, write_path: str, **kwargs) -> None:
if write_path == "set_cache":
dual_cache.set_cache("ttl_key", "v", **kwargs)
elif write_path == "async_set_cache":
await dual_cache.async_set_cache("ttl_key", "v", **kwargs)
else:
await dual_cache.async_set_cache_pipeline([("ttl_key", "v")], **kwargs)
@pytest.mark.asyncio
@pytest.mark.parametrize("write_path", ["set_cache", "async_set_cache", "async_set_cache_pipeline"])
@pytest.mark.parametrize(
("kwargs", "memory_ttl", "redis_ttl"),
[({}, 60, 3600), ({"ttl": 99}, 99, 99)],
ids=["tier_defaults", "explicit_ttl"],
)
async def test_dual_cache_writes_each_tier_with_its_own_default_ttl(write_path, kwargs, memory_ttl, redis_ttl):
"""
Regression for #43187: the Redis tier was written with default_in_memory_ttl,
so a configured default_redis_ttl never took effect. An explicit ttl still
reaches both tiers unchanged.
"""
in_memory_cache = InMemoryCache(default_ttl=600)
mock_redis = MagicMock()
mock_redis.async_set_cache = AsyncMock()
mock_redis.async_set_cache_pipeline = AsyncMock()
dual_cache = DualCache(
in_memory_cache=in_memory_cache,
redis_cache=mock_redis,
default_in_memory_ttl=60,
default_redis_ttl=3600,
)
before = time.time()
await _write_through(dual_cache, write_path, **kwargs)
after = time.time()
assert getattr(mock_redis, write_path).call_args.kwargs["ttl"] == redis_ttl
expiry = in_memory_cache.ttl_dict["ttl_key"]
assert before + memory_ttl <= expiry <= after + memory_ttlValidate PR title fails on the subject: .github/workflows/conventional-commits.yml:45 requires a lowercase first character, e.g. fix(cache): write DualCache's redis tier with default_redis_ttl.
2a2e9af to
f01a77c
Compare
|
@lets-order-some-fries Thanks for the thorough review. Addressed both points in
Happy to adjust if you'd rather the auth cache follow |
|
@apex-mochen re-checked at What I ran:
- effective_redis_ttl: Final = (
- default_redis_ttl
- if default_redis_ttl is not None
- else default_in_memory_ttl
- )
+ effective_redis_ttl: Final = default_redis_ttl if default_redis_ttl is not None else default_in_memory_ttlThe later steps in that job haven't run yet. Locally at this head, On your question: I'd keep the mirror, but at this head it breaks when
So at this head the result also depends on key order in the YAML. A write with Why mirror:
A longer auth-cache TTL is still available through The change for the last column, the attach half of my earlier suggestion: # litellm/proxy/proxy_server.py:4744-4748
if enable_redis_auth_cache is True:
user_api_key_cache.attach_redis_cache(redis_cache)Below is a test that fails at this head ( def test_auth_cache_redis_tier_keeps_in_memory_ttl_when_default_redis_ttl_is_set(monkeypatch):
"""
#43187: user_api_key_cache mirrors its in-memory TTL on Redis. A global
default_redis_ttl, from litellm_settings or cache_params, must not override
that when Redis is attached.
"""
from types import SimpleNamespace
from litellm.proxy.common_utils.user_api_key_cache import UserApiKeyCache
monkeypatch.setattr(litellm, "default_redis_ttl", 3600)
auth_cache = UserApiKeyCache(default_in_memory_ttl=60)
with (
patch.object(ps, "user_api_key_cache", auth_cache),
patch.object(ps, "spend_counter_cache", DualCache()),
patch.object(ps, "cli_sso_session_cache", DualCache()),
patch.object(ps, "litellm_config_cache", SimpleNamespace(redis_cache=None)),
):
ps._attach_redis_usage_cache(_FakeRedisCache(), enable_redis_auth_cache=True)
assert auth_cache.redis_cache is not None
assert auth_cache.default_redis_ttl == 60
assert auth_cache.key_object_cache.default_redis_ttl == 60
if "default_redis_ttl" in cache_params:
litellm.default_redis_ttl = cache_params["default_redis_ttl"]
litellm.cache = Cache(**{k: v for k, v in cache_params.items() if k != "default_redis_ttl"})With the format fix, both changes and that test, |
|
Thanks for the detailed re-check. Addressed all three remaining points in f11995d:
I kept the diff tight: no reformatting of Local test run: I wasn't able to bring up the full litellm test env in this sandbox (editable install hit a metadata-generation error and pulling the whole transitive dep tree would not finish in the window), so I'd appreciate CI running the focused suites you enumerated — New commit: |
f11995d to
3e891a3
Compare
|
Follow-up: rebased onto latest main ( |
|
@apex-mochen CI did run them at
|
3e891a3 to
66cd3f9
Compare
|
Thanks for the re-check. Addressed both mechanical points in
Rebased onto latest main ( |
lets-order-some-fries
left a comment
There was a problem hiding this comment.
@apex-mochen re-checked at 66cd3f9a: both files are LF again, the diff is back to the five files you listed, and lint is green along with every test job. The attach change, the non-mutating filter and the regression test are all in. Thanks for turning this around so quickly. LGTM.
66cd3f9 to
38615ef
Compare
|
Rebased onto
The only conflict was append-only in The three production files auto-merged, and I checked the change survived rather than assuming it did: every added line from the approved revision is present verbatim in the rebased tree (24/24 in What I ran locally at
One thing worth flagging rather than leaving for CI: No further changes from me; the branch is yours to re-check. |
38615ef to
8fb80bd
Compare
|
Rebased and pushed
The cause was the semantics, not the test. That test asserts the redis tier is written with TTL 300 when only Fixed by making that fallback the general rule in Verification at
Negative control, because a local failure is easy to misattribute: the whole I'm happy to adjust if you'd rather |
|
Update on the post-fix CI run (
The one remaining red check,
The 5 failing tests are the same on both sides, and none of them are under the files this PR touches:
Local checks at @lets-order-some-fries could you confirm the remaining |
|
@lets-order-some-fries a nudge on this one, with the evidence collected in a single place. The remaining red check is
The 5 failing tests are the same set on both sides — Current state of this PR: 1 approval, 100 checks, 99 green, and Could you either confirm that One note so this doesn't need repeating: I have already posted the same finding on #43577, #43292 and #42892. If you confirm here, I'll point those three at this thread rather than re-argue it on each — the question and the evidence are identical on all four. |
|
@apex-mochen I'd keep the fallback. In #43187 I suggested falling through to the backend's One path isn't covered yet, new on if ttl is None and self.default_redis_ttl is not None:
return batch.set(key, value, self.default_redis_ttl)
return batch.set(key, value, effective_ttl)My approval is on |
8fb80bd to
d7462ad
Compare
|
@lets-order-some-fries Thanks for the detailed checklist. Both points are addressed: |
lets-order-some-fries
left a comment
There was a problem hiding this comment.
Approving at d7462ad2. 322b0952 is 8fb80bdc rebased with no change (git range-diff shows =), and d7462ad2 makes _set_on_batch resolve the Redis TTL in the same order as the direct paths: explicit ttl, then default_redis_ttl, then default_in_memory_ttl. Locally the three test files this PR touches pass (85 tests), and with only the _set_on_batch hunk reverted the new test fails with ('SET', 'deferred', 300) == ('SET', 'deferred', 3600), so it covers the gap. The required checks are green. misc fails the same four test_openapi_compliance.py tests as main at 9dda4d89 (#43764 fixed the fifth), and osv-scan flags urllib3 in uv.lock and next in the dashboard's package-lock.json, which this PR doesn't touch, and fails the same way on other open PRs.
On routing: I don't have write access, so this review is as far as I can take it. The litellm-engineering team is already a requested reviewer.
|
Thanks for the thorough review! Could someone from @BerriAI/litellm-engineering take a look when available? The latest revision has been approved, with 85 relevant tests passing locally. Happy to address any remaining feedback. |
d7462ad to
7001066
Compare
|
Hi @litellm-engineering — rebased #43293 onto latest main and resolved the test-file conflict; mergeable is back to true and the earlier approvals (lets-order-some-fries, 85 tests verified) still stand. Could this be merged when CI is green? Happy to make any further adjustments. |
|
CI triage for the four red checks on Inherited from main — not this PR
Mine — fixed in
Full affected files now pass (121 passed): I also re-ran the negative control: the same test files staged onto No Postgres job was red on the previous head — the four above were the only failures. |
|
Follow-up, because "red on The suite passes; the red is the The run on |
|
Heads-up on the approval SHA: the branch moved twice after the 09-30 LGTM, so the approved commit is no longer the head.
The branch is still six files, +159/-10 against |
|
@lets-order-some-fries thanks for the thorough check — glad the five failing tests are pre-existing on main and not from this PR. The DualCache redis-tier fix and tests are ready on our side. Is there anything else needed before this can merge? |
76bf500 to
9778fa9
Compare
Rebase onto latest main and address review feedback from @lets-order-some-fries: - DualCache resolves kwargs per tier: the in-memory tier gets default_in_memory_ttl, the redis tier gets default_redis_ttl and falls back to the in-memory default when no Redis-specific TTL is set, so the two tiers expire together unless an operator asks for otherwise. An explicit ttl= still reaches both tiers unchanged. The previous version mutated one shared kwargs dict, so the in-memory TTL leaked into the redis tier and the redis tier could not be configured independently. - Those per-tier dict copies carry `# mutable-ok:` reasons so the type-discipline budget in scripts/type-discipline-budget.json stays within its ceiling. - UserApiKeyCache computes effective_redis_ttl = default_redis_ttl if set else default_in_memory_ttl in __init__ and passes that to super().__init__ and key_object_cache, so the auth-cache contract (Redis mirrors in-memory TTL when no explicit Redis TTL is configured) is preserved without special-casing DualCache. - _attach_redis_usage_cache no longer passes the global litellm.default_redis_ttl to user_api_key_cache.attach_redis_cache; the auth cache already mirrors its own in-memory TTL, and overriding it here made the effective TTL depend on YAML key order. - _init_cache reads default_redis_ttl without popping the caller's dict and filters a copy out before constructing Cache, so redis-py Redis() does not receive an unexpected keyword argument and DB reloads do not force a cache rebuild on every poll. - Add regression tests covering per-tier TTL writes and the auth-cache attach path. Refs BerriAI#43187 Signed-off-by: apex-mochen <2756823972@qq.com>
…path _set_on_batch fed default_in_memory_ttl to Redis whenever no explicit ttl was passed, so a configured default_redis_ttl was silently ignored on the request/post-call pipeline path (behind async_set_cache_pre_call / async_set_cache_post_call). Chain default_redis_ttl -> default_in_memory_ttl the same way set_cache / async_set_cache / async_set_cache_pipeline do, and add a default_redis_ttl=3600 case beside the BerriAI#43779 post-call test.
CI feedback on this branch, and what it turned out to be: - basedpyright budget: the per-tier `dict(kwargs)` copies re-wrote each call site as `**per_tier_kwargs`, which is an unknown-typed argument (`reportUnknownArgumentType` went 44804 -> 44811 against a 44802 limit). Note whether the caller supplied a ttl with a `ttl_given` flag instead and keep the base's `**kwargs` call shape, so the rule adds nothing. - tests/unit/caching/test_dual_cache.py: build the regression helpers without `**kwargs` for the same reason, and rename the local ttl- recording redis mock to `_ttl_recording_redis` -- it shadowed the `_recording_redis(values)` helper already defined further down the file, which would have broken every `test_shared_batch_read_*` case. - tests/unit/caching/test_request_redis_batch_post_call.py: drop the two cases that called `async_set_cache_post_call`; DualCache has no such method (`AttributeError`, 2 failed / 566 passed in caching-local). The post-call batch path is covered by the existing BerriAI#43779 test, and the new regression for BerriAI#43187 moves to the pre-call file beside it. - proxy_server: revert the `dict(cache_params)` filter from the previous commit. `Cache.__init__` ends in `**kwargs`, which it forwards to the backends, so the base already tolerated `default_redis_ttl`; the filter was defensive scope creep that added the last unknown-typed argument. Refs BerriAI#43187 Signed-off-by: apex-mochen <2756823972@qq.com>
9778fa9 to
aab86f4
Compare
|
@lets-order-some-fries rebased once more onto the latest main — the test-file conflict from the last 80+ commits is resolved (head aab86f4). The change itself is unchanged from your last review; CI is re-running. Should be ready to merge once checks pass. |
|
Gentle bump for merge:
The branch moved after that approval, so a quick re-approve at |
|
Hi @litellm-engineering, gentle follow-up on this one. #43293 received two approvals from @lets-order-some-fries (Sep 29 & 30), and I've since rebased onto the latest main (head |
|
Hi team, #43293 has two approvals and green CI — could a maintainer merge it? |
|
Hi maintainers — just a friendly bump: both review approvals are in and CI is fully green (104 checks). This fixes a real TTL-mirroring bug for the auth cache behind |
|
Maintainer review request: this is ready from the implementation/testing side. The current version covers both the direct DualCache write path and the deferred batch path, preserves the existing in-memory fallback behavior, and includes regression coverage. I also validated the current patch against recent main and a real Redis 7.0.15 instance. The community review has been positive, but the reviewer correctly noted that a BerriAI maintainer approval is still required. @BerriAI/litellm-engineering could someone take a look when bandwidth allows? Happy to address any remaining concerns. |
|
Hi @lets-order-some-fries — thanks again for your earlier review and approval! The HEAD has moved from the previously approved d7462ad to aab86f4. The subsequent changes are limited to CI/lint and test adjustments; the core per-tier Redis TTL behavior remains unchanged. The current HEAD is CI-green, and the remaining historical review feedback has been addressed. Would you be willing to give the current revision a quick re-review to confirm that your earlier approval still holds? Once confirmed, I’d appreciate a final merge decision from the LiteLLM maintainers. Thanks again for your help! |
lets-order-some-fries
left a comment
There was a problem hiding this comment.
Re-checked at aab86f42 against d7462ad2. The ttl_given rewrite in dual_cache.py is safe: the in-memory tier is written first, from an unpacked copy, so the Redis TTL can't leak into it. I compared the four write paths from both heads over 152 combinations of the two defaults, caller ttl (absent, None, 99), local_only and an extra kwarg: identical. The #43187 script gets ttl=60 on all seven writes on main (63a4f3f2) and 3600 here; ttl=99 passes through unchanged. Across four proxy configs the auth-cache EX matches main. Locally, tests/unit/caching/ plus the two auth-cache test files: 614 passed, 1 skipped; merged into main, 696 passed.
This commit also drops the _init_cache filter, so cache_params: {type: redis, default_redis_ttl: 3600}, which loaded at d7462ad2, raises TypeError again, as on main. Contrary to the commit message, the base didn't tolerate it, and the body's "remove the cache-only setting before constructing the Redis client" no longer holds. That's outside #43187. My approval holds at this head.
|
Thanks @lets-order-some-fries for the thorough review and approval! Really appreciate the extensive regression testing. I’ll keep the approved head unchanged. The _init_cache issue appears separate from this PR’s scope and can be addressed independently. Would a maintainer be able to take a look at the final merge readiness? Happy to help with anything remaining. |
|
Hi @ishaan-berri @yuneng-berri, Could someone from the LiteLLM engineering team help with the final merge review for this PR? @lets-order-some-fries has thoroughly reviewed and approved the latest commit (aab86f4), including 152 TTL combinations and 696 passing tests against main. The remaining merge blockers, if any, would benefit from a maintainer decision. Happy to address anything needed. Thanks for your time! |
|
Thanks again for the approvals on #43293 — all 100 checks are green. Do you happen to know which BerriAI maintainer could give the merge a nudge, or is there anything else you'd want changed first? |
Fixes #43187.
Signed-off-by: ding 2756823972@qq.com
Problem and scope
DualCache inherited the in-memory default TTL for Redis writes even when a separate Redis default was configured (#43187). Resolve each tier's default independently while retaining an explicitly supplied TTL. Preserve auth-cache mirroring and remove the cache-only setting before constructing the Redis client. The current diff includes regression coverage for direct and batched writes and the auth-cache configuration path.
2026-10-05 compatibility evidence
PR head
aab86f42ee4e0ec91128b3975b9fc9e7bd154afawas combined with upstream main1238cfe90f1029bf33e31ab40a805e7ecabfa1ffin an isolated local integration worktree without conflicts. The published branch is unchanged. The auth-cache tests have moved undertests/unit/proxy/on current main./home/dcl/venvs/litellm/bin/python -m pytest tests/unit/caching/test_dual_cache.py tests/unit/caching/test_request_redis_batch_pre_call.py tests/unit/caching/test_request_redis_batch_post_call.py tests/unit/proxy/test_redis_auth_cache_flag.py tests/unit/proxy/common_utils/test_user_api_key_cache.py -q --tb=short # 165 passed, 2 warnings in 8.20sThe initial attempt with the old test path ran no tests; the result above is the successful run with the current paths. This is local compatibility evidence, not a new approval or a performance measurement.
The September 30 approvals refer to an earlier SHA and came from a community reviewer, who explicitly noted that a BerriAI maintainer approval is still needed. Current-head review remains outstanding; this PR is not being represented as merge-approved. AI assistance was used in this validation and description update.
Additional real Redis validation (2026-10-05)
A standalone integration probe drove the real DualCache/RedisCache paths against Redis 7.0.15 on loopback in Ubuntu 24.04/WSL. No Redis methods were mocked. Each run used a fresh isolated instance with persistence disabled and terminated it afterward.
At PR head
aab86f42ee4e0ec91128b3975b9fc9e7bd154afa, 12/12 checks passed: sync write, async write, async pipeline, and request-scoped deferred batch, each with Redis default TTL 3600 (memory TTL 300), explicit TTL 7, and memory-TTL fallback 300 when the Redis default is unset. Redis TTL was read back directly; cached values matched in every case.Negative control at main commit
1238cfe90f1029bf33e31ab40a805e7ecabfa1ffproduced 8/12 passes: all four Redis-default cases incorrectly read back 300 rather than 3600; explicit and fallback controls passed. The baseline tree and PR branch were not modified for this experiment.This verifies single-node Redis command/TTL behavior, not actual wall-clock expiry, cluster/sentinel behavior, fault injection, or performance. No throughput improvement is claimed. Previous community approvals apply to their reviewed SHAs; maintainer approval of the current head is still needed.