fix(l1): delete dead backed-mode SWR machinery; docs stop claiming backed SWR (LAB-388) - #256
Conversation
WalkthroughThe change removes L1-managed SWR state and APIs. Documentation distinguishes L1-only SWR from backed-mode ChangesL1 SWR alignment
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
This comment has been minimized.
This comment has been minimized.
![kodus-27b[bot]](https://avatars.githubusercontent.com/u/223823690?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Auto-rebase attempted a merge of |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@kody start-review |
…SWR (LAB-388) The L1Cache SWR trio (get_with_swr / complete_refresh / cancel_refresh) had zero callers — the decorator wrapper only ever drove the live ObjectCache L1-only path. Its supporting state (_refreshing_keys, _entry_version, _swr_enabled, _swr_threshold_ratio, CacheEntry.cached_at) existed solely for the trio, so it goes with it: keeping version bookkeeping on every removal for a reader that no longer exists is the same trap the trio was. Docs now state the post-LAB-381 SWR reality: within-TTL refresh-ahead is L1-only mode (backend=None) only; production/secure have no backed SWR (Redis has no read-side freshness signal); io() keeps past-TTL stale_ttl SWR via headers (shipped in #228, untouched here). l1-invalidation.md's 'refresh from L2' story replaced with the live re-run-your-function semantics, incl. the corrected both-timers-restart TTL behaviour. Tests: unit+critical 2159 passed; docs markdown 121 passed; ruff + basedpyright clean. Live L1-only SWR path and test_l1_only_swr.py untouched.
…e; drop L2-era wording (LAB-388) Expert-panel findings applied: the canonical SWR config examples omitted ttl, configuring a mode where SWR never fires (ttl=None never goes stale); the 'Disable SWR' docstring still told the deleted L2 story; the second config table claimed a (0.1-1.0) threshold range vs the actual (0.0, 1.0] validation and lacked the L1-only qualifier. The panel also surfaced the inert namespace-index machinery (config never reaches the production L1Cache; no namespace= puts; no bulk invalidation callers) — filed as LAB-1433, out of this ticket's scope.
92e977f
c157261 to
92e977f
Compare
This comment has been minimized.
This comment has been minimized.
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 `@tests/performance/test_l1_invalidation_benchmarks.py`:
- Around line 468-475: Update the hot-path benchmark loop around L1Cache.get to
capture its found flag and assert each requested key is present, failing
immediately on any miss while retaining the existing latency measurement and
get_hit_p95 calculation.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 71210e48-09f3-455e-9634-cfe7adcf088f
📒 Files selected for processing (7)
README.mddocs/configuration.mddocs/features/l1-invalidation.mdsrc/cachekit/l1_cache.pytests/performance/test_l1_invalidation_benchmarks.pytests/unit/test_l1_invalidation.pytests/unit/test_l1_swr.py
💤 Files with no reviewable changes (2)
- tests/unit/test_l1_swr.py
- tests/unit/test_l1_invalidation.py
…s path (LAB-388) A miss returns early from L1Cache.get and is faster than a hit, so the loop discarding the return value could measure the miss path and still pass the <1500ns SLA if population or the key pattern ever drifted. Per CodeRabbit review on #256.
Kody Review CompleteGreat news! 🎉 Keep up the excellent work! 🚀 Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
1 participant
Summary
This PR removes dead backend-mode SWR (stale-while-revalidate) code from the L1 cache and corrects documentation that incorrectly claimed SWR works with backends configured.
Changes
Code Removal (
src/cachekit/l1_cache.py)Deleted the unused SWR machinery from
L1Cache:get_with_swr(),complete_refresh(), andcancel_refresh()methods_refreshing_keys,_entry_version) and config fields (_swr_enabled,_swr_threshold_ratio)cached_atfield fromCacheEntryand the associated version-bump/refresh-cancel logic in_remove_entry()andinvalidate_all()randomimportDocumentation Corrections
Clarified that within-TTL SWR only runs in L1-only mode (
backend=None) acrossREADME.md,docs/configuration.md, anddocs/features/l1-invalidation.md:swr_enabled/swr_threshold_ratiohave no effect when a backend (Redis, File, Memcached) is configured, since Redis exposes no read-side freshness signaldev(),production(),secure()now show SWR as "L1-only" rather than universally supported@cache.io's past-TTLstale_ttlmodettl(withttl=None, entries never go stale)Test Cleanup
get_with_swr,complete_refresh, version mismatch) and switched the SLA test to useget()Purpose
Eliminates misleading documentation and removes dead code paths that were never exercised in backend mode, reducing per-entry memory overhead and simplifying the L1 hot path.
Summary
This PR removes dead stale-while-revalidate (SWR) machinery from the L1 cache that only functioned in backed modes (with a configured backend like Redis), and updates documentation to accurately reflect that within-TTL SWR only works in L1-only mode.
Changes
Code cleanup (
src/cachekit/l1_cache.py)get_with_swr(),complete_refresh(), andcancel_refresh()methods that implemented the backed-mode SWR flow._refreshing_keys,_entry_version,_swr_enabled, and_swr_threshold_ratio.cached_atfield fromCacheEntryand drops the version-increment logic from_remove_entry()andinvalidate_all().randomimport.Documentation corrections
backend=None.swr_enabled/swr_threshold_ratioas L1-only-mode only, notes that SWR requires attl, and updates the presets table to indicate presets are "L1-only" for SWR.Test updates
get_with_swr,complete_refresh, version mismatch), and switches the SLA test to benchmark plainget()instead.Purpose
The removed SWR code was non-functional in backed modes since Redis exposes no read-side freshness signal, making it dead code. Documentation previously implied backed SWR worked, which was inaccurate. This change aligns the codebase and docs so that within-TTL SWR is correctly scoped to L1-only mode, while
@cache.io's past-TTLstale_ttlremains the only backed SWR mechanism.Based on the code changes provided, here's a description for this pull request:
Description
This PR removes dead code related to a "backed-mode" SWR (Stale-While-Revalidate) implementation from the L1 cache layer and updates documentation to no longer claim support for backed SWR functionality (LAB-388).
Changes
Benchmark correctness fix (
tests/performance/test_l1_invalidation_benchmarks.py):cache.get()(found, _).get()call actually results in an L1 hit. Previously, the benchmark discarded the return value, which meant an unnoticed cache miss could pass through undetected.Why
As noted in the added inline comment, a cache miss returns early and is faster than a cache hit. Without asserting that a hit occurred, the benchmark could inadvertently measure the miss path (the wrong code path) while still passing the SLA threshold. The new assertion ensures the benchmark measures the intended hit latency and fails loudly if the test setup produces unexpected misses.
Summary by CodeRabbit
Changed
stale_ttl.Documentation