perf: release full entry bookkeeping on all L1 cache removal paths - #1298
Conversation
InMemoryCacheLayer had three paths that remove an entry, and only delete() released everything the entry owned. Eviction left the key's access history behind; lazy expiry in get() left the access history and both size counters behind. total_size_bytes is budgeted against by _evict_if_needed, so bytes that expiry never released were charged against max_size_bytes forever. A 100 KB layer that has seen 50 entries expire retains 60 live entries instead of 110 -- a 45% capacity loss that never recovers, because the layer has no background sweeper and that branch is the only place expiry is ever handled. Route all three paths through a single private _release_entry() helper so that omitting a step is structurally impossible rather than something each future removal path has to remember. delete() is behaviour-identical and stays covered by its existing test. Closes #1297 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe cache now centralizes entry removal. Lazy expiration, explicit deletion, and LRU eviction update counters and clear access history through ChangesCache cleanup
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Warning Review ran into problems🔥 ProblemsThese MCP integrations need to be re-authenticated in the Integrations settings: Sentry Linked repositories: Public OSS repositories can only analyze public repositories installed in this organization. No linked repositories were analyzed; skipped 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 |
Agent Completion Truth Gate: NOT_APPLICABLEEvidence agrees. Machine-readable verdict{
"details": {},
"reasons": [],
"verdict": "not_applicable"
} |
|
@linear This is PR A of the split you recommended on #1295 — the pure bug fix, with the sliding window held back for #1294. It contains the expiry-path hole you found, plus the two stat decrements that same branch was also skipping, and the Three things I would like challenged, since they are judgement calls rather than mechanical transformations:
One thing I did not change: there is still no background sweeper, so expiry is only ever handled when a key is next read. This PR makes that path correct but does not make it proactive — a key that is never read again still occupies its slot until eviction reaches it. That felt out of scope here; flag it if you disagree. Verification: 171 passed, 0 regressions on the 163 pre-existing. Ruff parity confirmed against |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
No blocking issues from me.
Overall, the helper in src/youtube_extension/backend/services/intelligent_cache.py is carrying the right invariant, and the new coverage in tests/unit/test_intelligent_cache.py is strong enough that I wouldn’t split this further. |
There was a problem hiding this comment.
Pull request overview
Fixes #1297 by releasing L1 cache bookkeeping whenever entries expire, are deleted, or are evicted.
Changes:
- Centralizes entry cleanup in
_release_entry(). - Corrects lazy-expiry counters and access-history cleanup.
- Adds regression and capacity-churn tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/youtube_extension/backend/services/intelligent_cache.py |
Centralizes cache-entry cleanup across removal paths. |
tests/unit/test_intelligent_cache.py |
Tests expiry, eviction, history, and capacity accounting. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/youtube_extension/backend/services/intelligent_cache.py`:
- Around line 221-240: Update the existing-key replacement path in set() to
clear self.access_patterns[key] when the current entry is expired before
installing the new entry; preserve the access history when the existing entry
remains live. Add a regression test covering set() on an expired key without a
prior get() or delete(), verifying the replacement starts with clean access
history.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 286f6676-13a0-4f10-aa07-cc9d15746e1e
⛔ Files ignored due to path filters (1)
tests/unit/test_intelligent_cache.pyis excluded by!tests/**
📒 Files selected for processing (1)
src/youtube_extension/backend/services/intelligent_cache.py
📜 Review details
⏰ Context from checks skipped due to timeout. (6)
- GitHub Check: copilot-pull-request-reviewer
- GitHub Check: test
- GitHub Check: build
- GitHub Check: Security Scan - python
- GitHub Check: Generate and Upload Coverage
- GitHub Check: trivy
🧰 Additional context used
📓 Path-based instructions (10)
**/*.{py,js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{py,js,jsx,ts,tsx}: Use Python 3.9+ and Node 18+ for development
Never hardcode API keys, database URLs, or secrets in code
Make minimal, surgical changes and avoid deleting working code unless fixing security issues
Files:
src/youtube_extension/backend/services/intelligent_cache.py
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Always use type hints for Python functions
Use Black formatter with 88 character line length for Python code
Follow PEP 8 conventions for Python code
Use descriptive variable names and add docstrings to all public functions in Python
Group imports in Python: standard library, third-party, local
Use SQLAlchemy ORM and never write raw SQL queries
Use environment variables via os.getenv() or pydantic-settings to access configuration
Wrap database operations in try-except blocks and use context managers or FastAPI dependencies for connection cleanup
Implement comprehensive error handling with proper logging in all functions
Version APIs using /api/v1/ prefix for stability
Use JSON-RPC 2.0 protocol for all MCP communication
Follow the single-flow workflow: YouTube link → context extraction → agent dispatch → outputs
Use context managers for resource management in Python code
Implement comprehensive input validation and sanitize outputs for security
Use parameterized queries and SQLAlchemy ORM to prevent SQL injection
Define API request/response models using Pydantic for FastAPI endpoints
Store the single unified workflow as the only workflow; never introduce alternate flows or manual triggers
Use SQLAlchemy with connection pooling for database connections and manage sessions with context managers
Provide sensible defaults for non-sensitive configuration in Python settings
Maintain backward compatibility and do not break existing API endpoints
Include comprehensive logging for debugging in MCP implementations
Files:
src/youtube_extension/backend/services/intelligent_cache.py
⚙️ CodeRabbit configuration file
Python backend code. Check for type hints, proper exception handling, async context manager usage, and potential blocking calls in async functions. Flag any bare except clauses or missing timeout parameters on network calls. CRITICAL: Flag any file that contains placeholder/stub implementations — especially in unified_ai_sdk. Any class or function that says "TODO: Replace with production implementation" or returns mock/fake data must be flagged as a blocking issue. Flag any code generation output that reaches users without AST validation or syntax checking.
Files:
src/youtube_extension/backend/services/intelligent_cache.py
**/*.{py,js,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Maintain >80% code coverage for new features
Files:
src/youtube_extension/backend/services/intelligent_cache.py
**/*.{py,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{py,ts,tsx}: Keep frontend and backend data models synchronized using matching Pydantic (backend) and TypeScript (frontend) interfaces
Use type-safe interfaces for backend-frontend data exchange
**/*.{py,ts,tsx}: Production code must use real behavior only: no mock delays, fake data, or simulated responses.
Maintain strict type safety: mypy strict mode for Python and TypeScript strict mode for the frontend.
Name events using the<domain>.<entity>.<action>format.
Files:
src/youtube_extension/backend/services/intelligent_cache.py
**/*
📄 CodeRabbit inference engine (Custom checks)
**/*: Strictly verify that GitHub Copilot has explicitly reviewed and approved the pull request; human approvals alone must not satisfy this check.
Before allowing a merge, require thecopilot-rabbitlabel and AI-generated unit tests committed alongside the code changes; fail the check if either is missing.For Vercel-specific work, include
https://vercel.com/docs/llms-full.txtin the AI assistant context set.
Files:
src/youtube_extension/backend/services/intelligent_cache.py
**/*.{py,pyw}
📄 CodeRabbit inference engine (AGENTS.md)
Write Python code to remain compatible with Linux and Windows where possible, including correct handling of
asyncioevent loops.
Files:
src/youtube_extension/backend/services/intelligent_cache.py
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: Format Python code with Black using an 88-character line length.
Sort Python imports with isort using the Black profile.
Run Ruff with E, W, F, I, B, C4, and UP rules; E501 is ignored.
Use mypy strict mode; untyped function definitions are disallowed.
Target Python 3.9 or newer.
Validate backend inputs with Pydantic and sanitize subprocess arguments.
Use Anthropic SDK featuresthinking={"type": "adaptive"}andoutput_config={"effort": "..."}withanthropic>=0.105.0; do not addTypeErrorfallbacks for these parameters.Use the service container dependency-injection pattern in
backend/containers/.
Files:
src/youtube_extension/backend/services/intelligent_cache.py
**/*.{py,ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not commit secrets; store keys and credentials in gitignored
.envfiles.
Files:
src/youtube_extension/backend/services/intelligent_cache.py
**/*.{py,pyi}
📄 CodeRabbit inference engine (GEMINI.md)
**/*.{py,pyi}: Use Black formatting with an 88-character line length for Python code.
Use Ruff with rules E, W, F, I, B, C4, and UP for Python linting.
Use strict mypy type checking; do not define untyped functions.
Target Python 3.9 or newer.
Validate Python inputs with Pydantic.
Sanitize subprocess arguments before execution.
Do not add mock delays or fake data to production Python code; production runs in REAL_MODE_ONLY.
Keep secrets out of Python source code; load keys and credentials from environment variables instead.
Use absolute imports that resolve withPYTHONPATH=srcin the Python backend.
Files:
src/youtube_extension/backend/services/intelligent_cache.py
**/*.{py,pyi,ts,tsx}
📄 CodeRabbit inference engine (GEMINI.md)
**/*.{py,pyi,ts,tsx}: Preserve the single workflow: YouTube link → transcript → events → agents → outputs; do not introduce alternative flows or manual triggers that bypass it.
Use event names following<domain>.<entity>.<action>, such asyoutube.video.captured.
Make surgical, precise changes and do not delete working code without justification.
Files:
src/youtube_extension/backend/services/intelligent_cache.py
🔍 Remote MCP GitHub Copilot
Relevant review context
- The PR is
#1298, based onmainat0bfc783and headed by8ef9567; it changes only the cache service and its unit tests. _release_entry()is called for lazy expiry, explicit deletion, and LRU eviction.clear()remains a separate bulk-reset path that already clears entries, histories, and counters.- The helper assumes callers hold
_lock; all three call sites do so. Eviction-specificeviction_countaccounting remains outside the helper. set()still preserves access history when overwriting a live key; the new cleanup specifically handles expired or evicted entries.- The PR reports 171 focused tests passing, including independent prove-fail runs for expiry and eviction defects. However, at retrieval time several CI jobs—including tests, build, coverage, and security scans—were still in progress;
mergeable_statewasunstable. - CodeRabbit’s automated review excluded
tests/**, so its review covered only the production source file. - The linked issue remains open and identifies the same three removal paths and stale counter/history behavior.
🔇 Additional comments (1)
src/youtube_extension/backend/services/intelligent_cache.py (1)
153-156: LGTM!Also applies to: 242-247, 275-276
|
Review verdict (requested): one blocking correctness gap, otherwise green. CI is clean on One finding is real and should land before merge — the same one Copilot and CodeRabbit both raised (currently unresolved):
if key in self.cache:
old_entry = self.cache[key]
self.stats.total_size_bytes -= old_entry.size_bytes # access_patterns untouchedIf a key's entry has passed The one-line guard CodeRabbit proposed (pop Not approving or merging from here: protected- Generated by Claude Code |
|
I agree with that finding — the direct re- If an expired entry is replaced without a preceding read, src/youtube_extension/backend/services/intelligent_cache.py keeps |
InMemoryCacheLayer.set() replaced an existing entry without consulting its TTL. When the key had already expired, the successor inherited the dead entry's access timestamps, so a brand-new value presented to _calculate_adaptive_ttl as a hot key and was granted 14400s instead of the 3600s base TTL. This was the fourth path that stops an entry from existing. The other three (delete, eviction, lazy expiry) release bookkeeping via _release_entry; replacement reuses the slot, so total_entries must not move and only access_patterns is dropped. Adds a private _is_expired() predicate so the expiry check in get() and the new one in set() cannot drift apart. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
@coderabbitai you were right, and it was worse than the summary suggested. The claim in my Fixed in
Deliberately not reused Four regression tests added. The load-bearing one, Reverting just this hunk gives
@coderabbitai review |
|
@linear Follow-up on this PR after your clearance — the shape changed slightly, so I would rather re-check than assume the earlier "no blocking issues" still covers it. @coderabbitai found a fourth path I had missed, and your note about enumerating every way an entry stops existing applies to it directly: Two judgement calls in that fix I would like challenged:
One thing I did not change: there is still no proactive sweeper, so an expired key that is never read again holds its slot until eviction pressure arrives. That is a design change rather than a containment fix and I have kept it out. Verification: Good to merge, or do you want the predicate pulled back out? |
|
Looks good to me now.
No blocking issues from me. |
|
Good to merge. I would not pull the predicate back out.
I don’t see a reason to reopen the earlier approval over this shape change. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git show --stat --oneline 481969fd2
git show --format=fuller --find-renames --find-copies 481969fd2 -- \
src/youtube_extension/backend/services/intelligent_cache.py
ast-grep outline src/youtube_extension/backend/services/intelligent_cache.py \
--items all --type function --match '_is_expired|set|get|_release_entry|_calculate_adaptive_ttl'
rg -n -C 5 \
'test_reset_after_expiry_drops_dead_history|test_reset_after_expiry_keeps_entry_count_stable|test_reset_of_live_key_keeps_history|_is_expired|def set\(|def get\(|def _release_entry' \
src/youtube_extension/backend/services/intelligent_cache.py tests 2>/dev/null || trueLength of output: 19216 🏁 Script executed: #!/bin/bash
set -euo pipefail
git show --format= --find-renames 481969fd2 -- tests/unit/test_intelligent_cache.py
sed -n '179,323p' src/youtube_extension/backend/services/intelligent_cache.py
rg -n -C 12 \
'def test_(reset_after_expiry_drops_dead_history|reset_after_expiry_keeps_entry_count_stable|reset_of_live_key_keeps_history)|access_patterns|expires_at|_calculate_adaptive_ttl' \
tests/unit/test_intelligent_cache.pyLength of output: 12926 🏁 Script executed: #!/bin/bash
set -euo pipefail
sed -n '650,705p' src/youtube_extension/backend/services/intelligent_cache.py
sed -n '745,790p' src/youtube_extension/backend/services/intelligent_cache.py
fd -t f '^test_intelligent_cache\.py$' . -x sh -c '
echo "== $1 =="
rg -n -C 8 "_expire_now|class TestResetOfExpiredKeyStartsCleanHistory|test_adaptive_ttl_does_not_treat_reused_key_as_hot" "$1"
' sh {}
rg -n -C 5 \
'await .*\.set\(|_calculate_adaptive_ttl\(' \
src/youtube_extension/backend/services/intelligent_cache.pyLength of output: 6643
if self.adaptive_ttl_enabled and ttl is None:
ttl = self._calculate_adaptive_ttl(key)
for layer in self.layers:
result = await layer.set(key, value, ttl, tags)The new
Update the adaptive-TTL calculation path to ignore an expired L1 entry's history before it scores the key. Add an end-to-end regression test that:
This is a correctness issue. The stated You are interacting with an AI system. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== PR metadata and checks =='
gh pr view 1298 --repo groupthinking/EventRelay \
--json headRefOid,baseRefOid,mergeable,mergeStateStatus,statusCheckRollup \
--jq '{headRefOid, baseRefOid, mergeable, mergeStateStatus, checks: [.statusCheckRollup[] | {name, status, conclusion}]}'
echo
echo '== Changed files at the reported commit =='
git show --stat --oneline 481969fd2
git diff 481969fd2^ 481969fd2 -- src/youtube_extension/backend/services/intelligent_cache.py
echo
echo '== Source outline =='
ast-grep outline src/youtube_extension/backend/services/intelligent_cache.py --items all --type class,function --match 'InMemoryCacheLayer|_is_expired|_release_entry|get|set|delete|_evict_if_needed'
echo
echo '== Relevant source sections =='
rg -n -A45 -B12 'def (_is_expired|_release_entry|get|set|delete|_evict_if_needed)\b' \
src/youtube_extension/backend/services/intelligent_cache.py
echo
echo '== Changed tests =='
git diff 481969fd2^ 481969fd2 -- tests
echo
echo '== Test definitions and calls for replacement-after-expiry coverage =='
rg -n -A30 -B5 'def test_(reset_after_expiry|reset_of_live_key|reused_key).*|test_reset_after_expiry|test_reset_of_live_key|access_patterns|expires_at' tests \
-g '*.py'Length of output: 35177 🏁 Script executed: #!/bin/bash
set -euo pipefail
commit="481969fd2"
source_path="src/youtube_extension/backend/services/intelligent_cache.py"
test_path="tests/unit/test_intelligent_cache.py"
echo '== Adaptive TTL implementation and access-history consumers =='
git show "${commit}:${source_path}" |
nl -ba |
sed -n '/def _calculate_adaptive_ttl/,/^[[:space:]]*def /p'
echo
echo '== All access_patterns references in the committed source =='
git show "${commit}:${source_path}" |
rg -n -C 4 '\baccess_patterns\b|def _is_expired\b|def _release_entry\b'
echo
echo '== Test helper and complete new regression-test class =='
git show "${commit}:${test_path}" |
nl -ba |
sed -n '/def _expire_now/,/^class /p'
git show "${commit}:${test_path}" |
nl -ba |
sed -n '/class TestResetOfExpiredKeyStartsCleanHistory/,/\Z/p'
echo
echo '== Compare expiry predicate before and after this commit =='
git diff "${commit}^" "${commit}" --unified=20 -- "${source_path}" |
sed -n '/async def get/,/def _release_entry/p'Length of output: 1410 🏁 Script executed: #!/bin/bash
set -euo pipefail
commit="481969fd2"
source_path="src/youtube_extension/backend/services/intelligent_cache.py"
test_path="tests/unit/test_intelligent_cache.py"
echo '== Adaptive TTL implementation =='
git show "${commit}:${source_path}" |
sed -n '/def _calculate_adaptive_ttl/,/^[[:space:]]*def /p'
echo
echo '== All access_patterns references in committed source =='
git show "${commit}:${source_path}" |
rg -n -C 4 '\baccess_patterns\b|def _is_expired\b|def _release_entry\b'
echo
echo '== Expiry helper used by tests =='
git show "${commit}:${test_path}" |
sed -n '/def _expire_now/,/^class /p'
echo
echo '== Complete regression-test class =='
git show "${commit}:${test_path}" |
sed -n '/class TestResetOfExpiredKeyStartsCleanHistory/,$p'
echo
echo '== Direct comparison of the changed expiry and replacement logic =='
git diff "${commit}^" "${commit}" --unified=20 -- "${source_path}"Length of output: 17148
Do not use The magnitude statement is supported. The new end-to-end test verifies the relevant behavior: expired history would make the fresh entry qualify for the 14,400-second TTL, while the fixed path returns the 3,600-second base TTL. The live-key control also confirms that the fix does not discard valid history. I found no blocking issue in ✏️ Learnings added
You are interacting with an AI system. |
Canonical issue
Closes #1297.
Split out of #1295 on @linear's recommendation: that PR mixed a bug fix with a TTL-semantics change. This is the bug fix half. The sliding-window half follows separately under #1294.
Outcome
total_entriesandtotal_size_byteswhen an entry is lazily expireddelete()'s observable behaviourScope
src/youtube_extension/backend/services/intelligent_cache.py(+48/−12)get()lazy-expiry branch —del self.cache[key]replaced withself._release_entry(key, entry), plus a comment recording that this branch is the layer's only expiry handling and that no sweeper exists to reconcile drift. The inline expiry condition is replaced with the new predicate._is_expired(entry)— private@staticmethodholding the single definition of "this entry is dead". Added becauseget()andset()previously each carried their own copy of the condition, which is the exact drift that produced the fourth defect below._release_entry(key, entry)— private helper: drops the entry, decrements both counters, and pops the access history. Docstring enumerates the call sites and states that callers must already holdself._lock.set()replacement branch — when the key being overwritten is already expired, its access history is dropped before the new value is stored. Guarded by_is_expired, so overwriting a live key still preserves history.delete()— hand-rolled teardown replaced with a call to the helper. Behaviour-identical._evict_if_needed()— same substitution;stats.eviction_count += 1is retained at the call site because it is eviction-specific.tests/unit/test_intelligent_cache.py(+282)_expire_now(layer, key)module-level helper — backdatesexpires_atso the nextget()takes the expiry path deterministically, with nosleepand no added suite time.TestEvictionReleasesAccessHistory(3 tests),TestExpiryReleasesEntryBookkeeping(4 tests),TestExpiredBytesDoNotConsumeCapacity(1 test),TestResetOfExpiredKeyStartsCleanHistory(4 tests).Risk
The one behavioural change beyond releasing memory is that
total_entriesandtotal_size_bytesnow decrease when a key is lazily expired, where previously they did not. Anything reading those counters will see smaller, and correct, numbers. Within this file the only consumer is_evict_if_needed, which is the point — it budgets againsttotal_size_bytes, so the stale value was actively harmful. If an external dashboard has been calibrated against the inflated figures it will show a step change at deploy.delete()is rewritten to call the helper. It was already correct, so this is refactor risk rather than behaviour risk; it stays covered by the pre-existingtest_delete_cleans_access_patterns, and the prove-fail below exercises the other two call sites independently so the shared helper cannot mask a regression in either.The helper deliberately does not acquire
self._lock. All three callers already hold it, and it is athreading.RLock, so acquiring would also have been safe — this is documented in the docstring rather than left implicit.Correction to this PR's own earlier claim. As originally opened, the
Outcomerow about re-set()keys was only true when aget()happened to land between expiry and the rewrite — thatget()did the cleaning. @coderabbitai and @copilot both caught that the directset()-over-expired path was still inheriting history. That is fixed in481969fd2and is now covered by a test that specifically does not callget()after expiry. The gap is called out here rather than quietly folded in, because it is the second time on this branch that enumerating removal paths missed a replacement path.set()deliberately does not reuse_release_entry. The helper doesdel self.cache[key]and decrementstotal_entries, which is correct when a slot is freed and wrong when a slot is reused —set()immediately reinserts. A bareaccess_patterns.pop(key, None)is the right primitive there, andtest_reset_after_expiry_keeps_entry_count_stablepins that down.No public API, signature, return type or configuration changes.
Verification
Non-vacuity
A 100 KB L1 layer that has seen 50 entries expire permanently holds 60 live entries instead of 110 — a 45% capacity loss — because
_evict_if_neededbudgets against atotal_size_bytesthat lazy expiry never decremented.Probe: insert 200 entries into a 100 KB layer, with and without prior expiry churn.
access_patternskeys orphaned by expiryThe control row is the important one: capacity for a layer that never saw expiry is identical before and after, so the recovered capacity comes from releasing phantom bytes and not from relaxing enforcement.
Second, from @coderabbitai's finding: a value written over an expired key inherited its dead predecessor's entire access history, so a brand-new entry reported 20 accesses to
_calculate_adaptive_ttland was scored as a hot key — earning a 14400 s TTL instead of the 3600 s base.Probe:
set→ 5 reads while live → expire →setagain, with no interveningget()._calculate_adaptive_ttlfor a fresh entry144003600The second row is the control. If the guard had been unconditional it would have destroyed the frequency signal for genuinely hot keys, which is the signal
_calculate_adaptive_ttlexists to read.Prove-fail
Each defect was restored independently against the new tests. They are cleanly orthogonal — no test class can pass by accident from another's fix:
Restoring the eviction defect (
_evict_if_neededback to its original body) —2 failed, 169 passed:All expiry tests pass here, and
test_resident_keys_keep_their_historyalso passes — it is the fairness control, asserting the fix does not discard history for keys that are still resident, which holds under both versions.Restoring the expiry defect (
get()back to baredel self.cache[key]) —5 failed, 166 passed:All eviction tests pass here.
Restoring the replacement defect (the
_is_expired(old_entry)guard removed fromset(), leavingget()and the helper intact) —2 failed, 173 passed:The other two tests in that class pass under this revert, by design:
test_reset_of_live_key_keeps_historyis the control — replacing a live key must keep its history, and does, so the guard is genuinely conditional rather than a blanket wipe.test_reset_after_expiry_keeps_entry_count_stablepasses because the reverted code also lefttotal_entriesalone. It exists to pin down that the fix does not route through_release_entry, which would wrongly decrement a counter for a slot that is being reused rather than freed.A subtlety worth recording:
access_patternsonly accrues on a hit, and the append happens after the expiry check. A key that expires before it is ever successfully read has no history to orphan, so any test for this mustget()the key while it is still live. An earlier probe of mine missed the bug entirely for exactly this reason and produced a false negative. The mirror-image trap applies to the replacement path: the test must notget()the key after expiry, or the lazy-expiry fix pre-cleans the history and the test passes for the wrong reason.Tests added
test_evicted_key_history_is_releasedtest_eviction_churn_leaves_no_orphaned_historiesaccess_patternskeys never exceed resident keys under churntest_resident_keys_keep_their_historytest_expired_key_history_is_releasedtest_expired_key_releases_size_accountingtotal_size_bytesreturns to 0test_expired_key_releases_entry_counttotal_entriesreturns to 0test_reused_key_does_not_inherit_expired_historyget()→ re-set()starts with clean historytest_capacity_survives_expiry_churntest_reset_after_expiry_drops_dead_historyset()with no interveningget()starts with clean historytest_reset_of_live_key_keeps_historytest_reset_after_expiry_keeps_entry_count_stabletotal_entriestest_adaptive_ttl_does_not_treat_reused_key_as_hot3600base TTL, not3600 * 4Commands
Ruff was compared by swapping
origin/maincontent in at the real path, sincepyproject.toml's per-file ignores are path-scoped and a/tmpcopy would produce a false baseline.Production evidence
No production telemetry is attached. This layer is in-process and the leaked state is not exported to any metrics sink — the
total_size_bytesfigure a dashboard would read is precisely the value this PR corrects, so pre-fix telemetry would have understated the problem by construction.The evidence is therefore the reproductions above, run against the real
InMemoryCacheLayerwith no mocks or stubs on the code under test, using the layer's own publicset/getAPI and its own accounting to expose the drift. Each measurement carries a control row so it is falsifiable.Agent handoff
set()over an expired key) reproduced, fixed, and regression-testedorigin/mainat the real path