fix(cache): make the ttl parameter actually control expiry - #450
Conversation
CacheManager.set(key, data, ttl=...) stored the number and no read path
ever consulted it. Expiry came from a max_age inferred from substrings
in the key -- "live", "odds", "stock" -- so all 52 callers passing a ttl
were writing a value that did nothing. The docstring said so outright:
"stored for compatibility but expiration is still controlled via max_age
when reading". It is easier to read that as a note than as a defect,
which is presumably how it survived.
Both cache layers already hold the record when they decide, so each now
prefers an explicit ttl and falls back to max_age when there is none.
The caller that wrote the record knows what its data is; a substring
guess is a reasonable default for records that never said, and a poor
override for records that did.
Measured against a device's real cache of 8,875 entries carrying a ttl,
the inferred and intended values disagreed nearly everywhere:
stocks max_age 600 vs ttl 1800 4903 entries
news max_age 3600 vs ttl 600 1770 entries
odds max_age 1800 vs ttl 3600 1301 entries
images max_age 300 vs ttl 2592000 20 entries
In every case the ttl matches what the plugin plainly intended: stock
quotes cached for half an hour rather than ten minutes, headlines
refreshed every ten minutes rather than hourly, bird photographs that
never change kept for a month rather than five minutes.
Two things make this safe to land now. No sports_live entry carries a
ttl at all -- the live-score path does not use set(ttl=) -- so live
freshness is untouched, which matters with a season two weeks out. And
replaying the change against that real cache, 997 currently-expired
entries become live while not one live entry becomes expired, so there
is no invalidation spike on deploy.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
|
Warning Review limit reached
Next review available in: 38 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 8 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
|
@coderabbitai review |
|
CacheManager.set(key, data, ttl=...)stored the number and no read path ever consulted it. Expiry came from amax_ageinferred from substrings in the key —live,odds,stock— so all 52 callers passing a ttl (48 in the plugins, 4 in the core) were writing a value that did nothing.The docstring said so outright:
Which is easier to read as a note than as a defect, and is presumably how it survived. It surfaced while reviewing #449 — I'd tried to jitter odds TTLs and the review pointed out the write could not possibly matter.
The change
Both cache layers already hold the record when they decide, so each now prefers an explicit
ttland falls back tomax_agewhen there isn't one. The caller that wrote the record knows what its data is; a substring guess is a reasonable default for records that never said, and a poor override for records that did.What it actually changes
Measured against a device's real cache — 8,875 entries carrying a ttl — the inferred and intended values disagree nearly everywhere:
max_agetodayttlintendedIn every case the ttl matches what the plugin plainly intended. Nothing here is a plugin bug being exposed — they were all written correctly against a parameter that wasn't wired up.
Why it's safe to land two weeks before the season
Live scores are untouched. No
sports_liveentry carries a ttl at all — that path doesn't useset(ttl=), it goes through the strategy reader. So the 30-second freshness the heuristic provides is unchanged. This was my main worry, and the real cache settles it.No invalidation spike. Replaying the change against that same cache: 997 currently-expired entries become live, and not one live entry becomes expired. The deploy is strictly non-disruptive; the ongoing change (news refreshing more often) is gradual.
Notes
ttl=0means zero seconds, not "forever" —max_age=Noneis how a caller asks for no expiry. Pinned in a test.boolttls fall back tomax_age.Trueis anintin Python and would otherwise become a 1-second ttl; also pinned.Verification
16 tests in
test_cache_ttl_honoured.pycovering both layers: ttl longer than max_age keeps an entry, shorter expires it, absent falls back, both layers agree on the same record (so nothing is live in memory and expired on disk), and an end-to-endset(ttl=)→get_with_auto_strategy()round trip.Full suite: 2712 passed, 60 skipped, 1 pre-existing unrelated failure (
test_install_lowmem). The 221 existing cache tests pass unchanged.🤖 Generated with Claude Code
https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5