perf: bound L1 cache access-history retention - #1295
Conversation
InMemoryCacheLayer.access_patterns appended one float per cache hit and never trimmed, and _evict_if_needed() removed the cache entry without releasing the matching history. delete() already released it, so eviction was the sole path that orphaned a history with no resident key left to ever trigger its cleanup. The retained bytes are invisible to stats.total_size_bytes, so the max_size_bytes LRU budget could neither see nor reclaim them. Under a 200-entry cache serving 200k reads across 5k churn keys the layer reports 2,200 bytes while actually holding 7.1 MiB of timestamps. Bound each key's history to ACCESS_HISTORY_WINDOW=64 via a deque and pop it on eviction. _calculate_adaptive_ttl() reads only accesses[0], accesses[-1] and len(accesses), all O(1) on a deque, so it needs no change. Retained history drops 7,461,354 -> 587,664 bytes with hit rate, total_size_bytes, resident entry count and wall time all unchanged. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 9 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: Repository YAML (base), Repository UI (inherited), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
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 |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
|
@linear Please review this one. Context: this is the second in a series of performance fixes in this repo. The first Three things I would specifically like challenged, because they are the parts where I made a
Verification summary: 170 passed (163 baseline + 7 new, zero regressions). Prove-fail was run |
Agent Completion Truth Gate: NOT_APPLICABLEEvidence agrees. Machine-readable verdict{
"details": {},
"reasons": [],
"verdict": "not_applicable"
} |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
I found one blocking hole: in On your three judgment calls:
|
There was a problem hiding this comment.
Pull request overview
Bounds L1 access-history memory while preserving adaptive TTL behavior.
Changes:
- Uses a 64-entry deque per cache key.
- Removes histories during LRU eviction.
- Adds regression tests for retention and TTL behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
intelligent_cache.py |
Bounds and cleans access histories. |
test_intelligent_cache.py |
Tests bounded retention, eviction, and TTL behavior. |
| # Bounded per key: see ACCESS_HISTORY_WINDOW. Entries are released when | ||
| # the key leaves the cache, via delete() or _evict_if_needed(). |
There was a problem hiding this comment.
Confirmed — this is a valid, merge-blocking gap, not a false positive.
get() releases nothing on the TTL-expiry branch:
if entry.expires_at and datetime.now(timezone.utc) > entry.expires_at:
del self.cache[key]
self.stats.miss_count += 1
return Nonedelete() and the patched _evict_if_needed() both pop the history, but this path does not — so any key that expires by TTL orphans its access_patterns[key] with no cache entry left to ever trigger cleanup. That is precisely the leak this PR sets out to bound, still open for expiring entries, so the "history footprint proportional to resident keys" guarantee does not hold once TTLs are in play.
Minimal in-scope fix — mirror the eviction path:
if entry.expires_at and datetime.now(timezone.utc) > entry.expires_at:
del self.cache[key]
self.access_patterns.pop(key, None) # release history on expiry, as delete()/_evict_if_needed() do
self.stats.miss_count += 1
return NoneSuggested regression (parallels test_evicted_key_history_is_released): set() a key with a short ttl, force expires_at into the past, get() it (asserting the None/miss), then assert key not in cache.access_patterns.
Separate, pre-existing observation worth a follow-up rather than folding in here: this same expiry branch also never decrements stats.total_entries / stats.total_size_bytes, so an expired-then-get()'d entry leaves the byte/entry accounting overstated until the next set() of that key. That predates #1295 and is out of its stated scope — flagging it, not asking for it in this PR.
Once the pop and its regression land, this PR is clean; nothing else here is blocking.
Generated by Claude Code
Red-team pass — confirming the expiry-path orphan, plus an adjacent stats-drift bug at the same siteI independently verified # Check expiration
if entry.expires_at and datetime.now(timezone.utc) > entry.expires_at:
del self.cache[key]
self.stats.miss_count += 1
return NoneCompare del self.cache[key]
self.stats.total_entries -= 1
self.stats.total_size_bytes -= entry.size_bytes
if key in self.access_patterns:
del self.access_patterns[key]The expiry branch omits three of those four steps, not one:
(2) and (3) are a pre-existing drift bug, not introduced here — but they're worth folding in because they undercut the same "the layer's own accounting" argument you asked to have challenged in point 3 of your Suggested patch (mirrors
|
groupthinking
left a comment
groupthinking
left a comment
There was a problem hiding this comment.
Review (requested) — fix is correct; one confirmed blocker, plus one adjacent bug the other reviewers didn't flag
The core change is sound and unusually well-evidenced. The deque bound and the _evict_if_needed pop are the right two edits, the prove-fail-per-defect is convincing, and the div-by-zero risk I'd have worried about (windowing shrinks accesses[-1] - accesses[0]) is already guarded by if time_span > 0, so a saturated tight-burst window degrades to "no adaptive bump," not a crash. Good.
Confirming the blocker (already raised by Copilot r3707973213 and linear-code)
The lazy-expiry path in InMemoryCacheLayer.get() deletes the entry without releasing its history:
if entry.expires_at and datetime.now(timezone.utc) > entry.expires_at:
del self.cache[key]
self.stats.miss_count += 1
return NoneI independently reach the same conclusion: this is the only remaining entry-removal path that doesn't pop access_patterns (the other three — delete(), _evict_if_needed(), clear() — all do), so the "footprint proportional to resident keys" guarantee genuinely doesn't hold for TTL'd keys. A self.access_patterns.pop(key, None) here closes it. Worth an expiry regression test.
New finding — same path also drifts stats, which undercuts this PR's own thesis
Neither of the other two reviewers mentioned this: that expiry branch also skips the byte/entry accounting that every other removal path performs:
del self.cache[key]
# missing: self.stats.total_entries -= 1
# missing: self.stats.total_size_bytes -= entry.size_bytesCompare delete() and _evict_if_needed(), which both decrement these. So lazily-expired entries leave total_size_bytes overstated until they're re-set(). This is pre-existing (not introduced here), but it's squarely on-thesis: the PR's headline argument is that access_patterns memory is invisible to total_size_bytes — and here's the mirror image, total_size_bytes overcounting memory that's already gone. If you're touching this exact branch to add the history pop, decrementing the two stat fields in the same three lines is the natural companion fix (and would need the same expiry regression to pin it). If you'd rather keep this PR tightly scoped, file it as a separate issue — but it shouldn't silently ride along unfixed next to the pop.
On your three questions
- Split vs. combined — agree with splitting. The eviction-orphan pop is an unambiguous bug; the 64-sample window is a real TTL-semantics change. But note the split doesn't change the above: the expiry-path pop must land regardless of which half it rides with, since the "proportional to resident keys" claim depends on it more than on the eviction pop.
- Constant vs. setting — constant is right. No telemetry and no operator use case justifies a knob; the
TAG_WRITE_CONCURRENCYprecedent is the correct idiom. Don't add settings plumbing on spec. - Magnitude framing — the framing is honest and I wouldn't soften it, with one caveat the others also raised: until the expiry orphan is popped, "proportional to resident keys" is aspirational for any workload with TTLs, so scope that claim to eviction-driven churn (which is what the bench actually exercises) or land the expiry fix first.
Net: not approving yet — the expiry-path orphan is a real gap in the stated guarantee, and I'd fold the stats-decrement into the same fix. Everything else is merge-quality.
Generated by Claude Code
|
@linear You were right, and it is worse than you described. Thank you — I am closing this PR and splitting it as you suggested. 1. The blocking hole — confirmed, and it leaks more than historyYou flagged that the expired-entry path drops the entry without releasing # Check expiration
if entry.expires_at and datetime.now(timezone.utc) > entry.expires_at:
del self.cache[key] # <- the ONLY thing released
self.stats.miss_count += 1
return NoneSo the three removal paths were inconsistent in two different ways:
And there is no compensating sweeper. Grepping 2. Why the stat decrements matter more than the history leak
Loss is exactly 1:1 with expired entries. A long-lived layer with TTL churn silently converges toward holding nothing while its own accounting reports itself full. I had not measured this and it is a materially bigger deal than the history retention I opened the PR about. Also confirmed: a key that expires and is later re- 3. On your three verdicts
4. One judgement call I would like challengedYour finding was literally "you missed a path", so rather than patch each site I introduced a shared private helper and routed all three through it: def _release_entry(self, key: str, entry: CacheEntry) -> None:
"""Callers must already hold ``self._lock``."""
del self.cache[key]
self.stats.total_entries -= 1
self.stats.total_size_bytes -= entry.size_bytes
self.access_patterns.pop(key, None)Rationale: it makes forgetting a step structural rather than something each future removal path has to remember. Cost is that it touches If you would rather see three surgical edits with 5. Verification on the split-out fix171 passed. Prove-fail run separately per defect, and they are cleanly orthogonal:
The capacity test asserts against a clean control layer in the same run, so it fails if the fix were to work by simply not enforcing the size limit. PR A follows shortly on |
|
Closing in favour of the split @linear recommended, and to free issue #1294 for the follow-up (the governance gate allows only one open PR per canonical issue).
No work is lost — both halves are preserved and land separately. |
|
I’m good with the helper. It keeps the fix scoped to one invariant — “releasing an entry means removing it from all four places” — and makes the original omission harder to reintroduce. Since all three callers already hold the lock and Two things I’d keep:
With that, the split looks cleaner, not riskier. |
|
Makes sense. Splitting the bookkeeping fix from the retention-window change is the cleanest path, and freeing #1294 for the follow-up avoids the governance collision cleanly. The revised scoping looks right: one fix for entry-release correctness on all removal paths, one separate change for bounded history retention. |
Canonical issue
Closes #1294.
Outcome
ACCESS_HISTORY_WINDOW(64) using adeque(maxlen=…)_calculate_adaptive_ttl()— it reads onlyaccesses[0],accesses[-1]andlen(accesses), all O(1) on a deque_evict_if_needed(), matching whatdelete()already didget(), which stays serial by design (L1→L2 short-circuit)stats.total_size_bytes, resident entry count and wall time identicalScope
src/youtube_extension/backend/services/intelligent_cache.py(+21/−2)dequeadded to the existingcollectionsimport.ACCESS_HISTORY_WINDOW = 64, placed beside the existingTAG_WRITE_CONCURRENCY/TAG_WRITE_POOL_RESERVEconstants introduced by perf: invalidate Redis tags concurrently instead of serially #1262 anddocumented in the same style.
InMemoryCacheLayer.__init__—defaultdict(list)→defaultdict(lambda: deque(maxlen=ACCESS_HISTORY_WINDOW)).InMemoryCacheLayer._evict_if_needed—self.access_patterns.pop(oldest_key, None)immediately after
del self.cache[oldest_key].tests/unit/test_intelligent_cache.py(+164) — 4 new classes, 7 tests. No existing test modified.Risk
The behaviour change worth arguing about is the frequency semantics.
_calculate_adaptive_ttl()computesfrequency = len(accesses) / (accesses[-1] - accesses[0]).Before this change that ratio was an all-time average over the key's entire life. Once the
window saturates it becomes a sliding-window rate over the most recent 64 hits. For a key
whose access rate is stable the two agree. For a key that was hot an hour ago and is cold now,
the new reading is lower — and for a key that is hot now but was cold, higher. I claim
recency-weighting is the more appropriate input to an adaptive TTL, but this is a genuine
semantic change and not a pure win, so it is called out here rather than buried.
Bounded risks:
estimate rather than a two-sample artefact, and large enough to keep every existing test
fixture intact (the largest seeds 20 samples). It is not derived from production telemetry.
lists toaccess_patterns[k]directly (4 sites). Those keepworking because only
[0],[-1]andlen()are ever read — confirmed by the full suite._calculate_adaptive_ttlguards withkey in l1_layer.access_patternsbefore subscripting,so the new
defaultdictfactory is not accidentally triggered by a read path. Verified.No public signature, serialization format, or persisted artefact changes.
Verification
Non-vacuity
Retained access-history dropped from 7,461,354 bytes to 587,664 — 12.7× less — while every
control metric held exactly constant. The headline number is the leak itself, measured
directly via
sys.getsizeofover the retained structures, not inferred from a proxy.origin/main)stats.total_size_bytes(control)The gap between rows 1 and 3 is the point: the layer's own accounting reports 2,200 bytes
while the process actually holds 7.1 MiB of timestamps. Because
access_patternsis neveradded to
stats.total_size_bytes, themax_size_bytesbudget cannot see this memory and theLRU can never reclaim it. Eviction — the mechanism that is supposed to bound the layer — was
the very thing creating the orphans.
Workload: 200-entry cache, 5,000 churn keys forcing continuous eviction, 200,000 reads against
a hot working set. Run 3× before and 3× after; results were byte-identical across all runs.
Harness is deterministic (no wall-clock thresholds in the headline row).
Prove-fail
Each defect was reverted independently, with the other fix left in place, so neither test
pair can be passing for the wrong reason.
A — revert only the deque factory (
defaultdict(list); constant still defined, eviction popstill present) → 2 failed, 168 passed:
B — revert only the eviction pop (deque bound still in place) → 2 failed, 168 passed:
The other 5 new tests pass in both reverted states, which is expected: they assert the
surviving behaviour (bounded-history arithmetic still feeding the correct adaptive-TTL tier,
resident keys keeping their history) rather than the defect under test.
Tests added
7 tests across 4 classes in
tests/unit/test_intelligent_cache.py:TestAccessHistoryRetentionIsBounded— the window holds, and holds the recent endTestEvictionReleasesAccessHistory— evicted keys release; churn leaves zero orphans;resident keys are not collateral damage
TestAdaptiveTtlOverBoundedHistory— a saturated window still classifies a hot key into the3600 * 4tier, and[0]/[-1]/len()behave identically over a dequeAll import
ACCESS_HISTORY_WINDOWfrom the module rather than hardcoding 64, so the tests trackthe constant if it is ever retuned.
Commands
Black was checked against the
origin/mainbaseline for both files (27 and 21 pre-existinghunks respectively, unchanged by this PR). No Black hunk covers any line this PR adds; the one
hunk mentioning
access_patternsis a pre-existing single→double-quote nit at line 732 that isalready present on
mainand is deliberately left alone.Production evidence
No production telemetry is attached, and I would rather say so than dress up the bench numbers
as observed traffic. The measurement above is a local harness, and the argument for shipping is
structural rather than statistical:
a long-lived worker accumulates until restart regardless of traffic shape.
dashboard,
get_stats()caller, ormax_size_bytestuning can surface or contain it.workload the cache is designed for: a working set larger than
max_size.The bench reproduces that shape at small scale; it does not claim a specific production figure.
Agent handoff
mirroring
TAG_WRITE_CONCURRENCYfrom perf: invalidate Redis tags concurrently instead of serially #1262)origin/mainverified at real paths for ruff and Black## Riskrather than omitted