Repository navigation
Conversation
…et the memo own the kill A whole-function invalidation (cache_clear(), or invalidate_cache() with no arguments) deleted an L1-only cache's entries one by one, from the keys the process had recorded. A fill records its key only after its put, and a single-key invalidation untracks its key before its delete, so an entry stored before the call could outlive it and stay servable by the lock-free hit. The cache is now cleared whole. The memo now ends an entry's lock-free hits itself: forget, repoint and clear kill the entries they drop, so no removal route pairs a kill with a slot drop by hand. The at-fork reset clears the memo after it forgets every stored entry, which also ends an entry a refresh had in hand when a fork interrupted it, and a memo write skips a state the reset retired. A slot is read back with an exact type check, so an object planted in the memo's dict through the garbage collector is a miss or a TypeError, not undefined behaviour. L1-only TTLs run on the OS monotonic clock that the extension reads, so a patched time.monotonic does not move them. The docs say so and show how to test expiry instead. The memo's bounds are exported from the extension, the unused HitEntry getters and pyfunction shims are gone, and the SWR jitter's low end is one named constant.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 13 minutes. View limit details
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. |
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Providing Context (Files & MCPs)Add these hints in your PR description (or a comment) to unlock deeper checks:
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![kodus-27b[bot]](https://avatars.githubusercontent.com/u/223823690?s=60&v=4){kind=small}
Instruction budget: FAILHead
A path fails at +1% or more against the base (+2% for |
…out parameters too cache_clear() called invalidate_cache() with no arguments, which on a function without parameters deletes only the entry of the call without arguments. A key= that reads its context files that call under many keys, so the other contexts' entries outlived the clear. cache_clear() now takes the whole-function path directly, after the preamble every invalidation runs (settle a zero-config function, resolve a lazy backend, apply the interop guard), which invalidate_cache() and ainvalidate_cache() now share. An L1-only fill no longer records its key in _cached_keys. Nothing reads the record once the whole-function path clears the cache whole, and it grew by one entry for every distinct key ever filled. Docs: the monotonic-clock note now names a bare @cache that resolves no backend and the stale-while-revalidate timing, and the free-threading audit row and the at-fork comments match the code.
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Providing Context (Files & MCPs)Add these hints in your PR description (or a comment) to unlock deeper checks:
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
…s key and twin again The previous commit sent every cache_clear() down the whole-function path. On a backed function without parameters that path reaches only the keys this process recorded and, on Redis, the key registry, so the call's own key and its pre-0.20.0 twin could stay in L2 and keep being served. The whole-function path now applies to L1-only functions only; a backed function's cache_clear() goes through invalidate_cache() as it did before. Docs: an L1-only cache is any decorator that finds no backend configured, not only a bare @cache, and the cache_clear() and invalidate-all docstrings describe each mode.
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Providing Context (Files & MCPs)Add these hints in your PR description (or a comment) to unlock deeper checks:
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
|
@kody start-review |
|
@kody review --force |
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Providing Context (Files & MCPs)Add these hints in your PR description (or a comment) to unlock deeper checks:
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
The lock-free hit reached CPython through unsafe code: a hand-made METH_FASTCALL builtin, direct calls to the name descriptors' getters, unchecked casts and a raw pointer for the tests' clock. cachekit-rs holds no unsafe code (SECURITY.md), and the post-merge cargo-geiger check failed on main with six unsafe functions. The hit now goes through PyO3's checked API: a #[pymethods] get, attribute reads for the function's names, cast_exact for memo slots, exact-type checks against PyO3's type objects, and a mutex with an atomic flag for the tests' clock override. cargo-geiger counts no unsafe function, expression or impl in the crate. The crate root forbids unsafe code, so a pull request's build fails on any instead of the post-merge check. The checked calls cost the hit about 280 instructions, most of it the type lookup of the two name reads: l1_hit moves from 2,482 to 2,758 on CPython 3.14 and from 2,594 to 2,935 on 3.12, and the budgets are raised to match.
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Providing Context (Files & MCPs)Add these hints in your PR description (or a comment) to unlock deeper checks:
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
The Bench run of the previous commit on a GitHub-hosted AMD EPYC 7763 (CPython 3.14.7) timed the 100-user dict's L1-only hit at 0.341 ± 0.001μs and the DataFrame's at 0.34 ± 0.00μs, against 0.311μs and 0.31μs for the hit that reached CPython through unsafe code. The README, the performance guide, the comparison page and the backend pages quote the new figures; the hosted table cites the run for its two L1-only rows.
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Providing Context (Files & MCPs)Add these hints in your PR description (or a comment) to unlock deeper checks:
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
Conflicts: tests/performance/ir_baselines.json and its table in docs/performance.md. main re-recorded every budget after its workload edit (new heap layout), and this branch raised l1_hit and minimal_l1_hit for the safe-Rust hit, so neither side's figure for those two paths describes the merged code. Resolution: main's budgets for every path, then l1_hit and minimal_l1_hit re-recorded on the merged tree with `ir_budget.py --update --allow-increase --path l1_hit --path minimal_l1_hit` on CPython 3.12.12 and 3.14.3: path 3.12.12 3.14.3 l1_hit 2,936 2,765 minimal_l1_hit 2,937 2,765 The host reproduces main's own budgets for l1_hit, minimal_l1_hit and l1_hit_backed within 0.24%, and on the merged tree l1_hit_backed, secure_l1_hit and byte_storage_store_40b stay within 0.15% of main's budgets, so those keep main's figures. Against main the hit still costs about 280 more instructions on 3.14, as the docs say.
|
Resolved |
|
@kody start-review |
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Providing Context (Files & MCPs)Add these hints in your PR description (or a comment) to unlock deeper checks:
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
Brings in main 1ef1d05..43c2e48, six pull requests, among them the Rust StandardSerializer decode (msgpack_decode) and the backed-L1-hit stats change. Conflicts: the instruction budgets and the performance guide. Every budget is main's, except l1_hit and minimal_l1_hit, re-recorded on the merged tree with ir_budget.py --update --allow-increase (3.12.12: 2,935 and 2,936; 3.14.3: 2,765 and 2,764). The guide keeps main's decode wording and this branch's L1-only figures. msgpack_decode calls the CPython C API through unsafe code, which this branch's crate-wide #![forbid(unsafe_code)] rejects, so the merged crate would not build. The crate root now denies unsafe code and allows it in that one module only, and SECURITY.md and the module docs say so.
The hosted L1-only hit moved from 0.311 to 0.341us when it became safe Rust, but ratios worked out from the old figure stayed: a backed L1 hit of the 100-user dict is about 830 times the L1-only hit (was 910), a Redis L2 hit about 1,580 times (was 1,730), and a hit with one list argument about 19 times its instructions (53,153 against 2,765 on CPython 3.14; was 22). The guide states the safe hit's cost on both interpreters (about 280 more instructions on 3.14, 340 on 3.12) instead of saying the extension holds no unsafe code, and the README and the hosted-runner section say which run the two L1-only rows come from. An L1-only cache is any plaintext, non-interop decorator that finds no backend configured, not any such decorator, and the memo test's docstring says names are read by attribute access.
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Providing Context (Files & MCPs)Add these hints in your PR description (or a comment) to unlock deeper checks:
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
The crate-level doc names CPython without backticks, which the pedantic clippy run in Security Fast rejects. The Medium-tier row said "zero unsafe functions in cachekit-rs", which reads as a fact about the crate; with msgpack_decode exempt from the deny, the check fails on its unsafe functions, and the row now says so.
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Providing Context (Files & MCPs)Add these hints in your PR description (or a comment) to unlock deeper checks:
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
Security Fast's clippy builds without the python feature, so it never compiles l1_hit, python_bindings or msgpack_decode, and deny(unsafe_code) gives way to an allow(unsafe_code) added in the same change. The crate doc and SECURITY.md now say what does run: CI's Lint Rust clippy, on default features, rejects unsafe code outside msgpack_decode unless the change also adds an allow, and the post-merge cargo-geiger check counts it either way. The sentence leaves the Fast row, whose workflow does not run that check. The 830 and 1,580 ratios divide by an L1-only hit from a later run on another VM, so they compare figures on the same CPU model, not the same runner. Fix "than the other two" in the README.
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Providing Context (Files & MCPs)Add these hints in your PR description (or a comment) to unlock deeper checks:
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
Re-records l1_hit and minimal_l1_hit on the merged tree: main now runs them over 300,000 calls.
… about 2,600 With the safe-Rust hit, l1_hit and minimal_l1_hit cost 2,765 on CPython 3.14 and 2,938 on 3.12.
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Providing Context (Files & MCPs)Add these hints in your PR description (or a comment) to unlock deeper checks:
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
|
Resolved |
Takes main e39b506 (#700, #696, #702). Only the docs/performance.md budgets table conflicted: main added the l1_hit_backed_auto row beside this branch's re-recorded minimal_l1_hit row, and both rows are kept. The l1_hit and minimal_l1_hit budgets stand. Measured on the merged tree, they read 2,936 / 2,936 on 3.12.12 and 2,765 / 2,765 on 3.14.3, within 0.07% of the committed figures. #696 renames VerifiedMsgpack on the backed L1 path and adds a workload entry; it does not touch the memoised L1-only hit.
|
@kody review --force |
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Providing Context (Files & MCPs)Add these hints in your PR description (or a comment) to unlock deeper checks:
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
Follow-up to #673, which merged before its review fixes landed. The main fix: a whole-function invalidation could leave an entry it should have removed, and the lock-free hit from #673 kept serving that entry after the call returned.
It also takes #673's unsafe code out of cachekit-rs: that hit reached CPython through unsafe code, and it now runs in safe Rust.
main's Security Medium workflow stays red after this merges, becausemsgpack_decodefrom #674 holds unsafe functions too.Closes LAB-12024
BREAKING CHANGE: (from #673) an L1-only cache (
@cache(backend=None),@cache.local(), or any plaintext, non-interop decorator that finds no backend configured) times its TTLs and its stale-while-revalidate refresh on the OS monotonic clock that the Rust extension reads, not ontime.monotonic(). Patchingtime.monotonic(pytest'smonkeypatch, freezegun'sfreeze_timeortick) no longer expires its entries or brings a refresh due: a test that needs an entry gone callsfn.invalidate_cache(...)orfn.cache_clear(). Caches with a backend are unchanged.BEGIN_COMMIT_OVERRIDE
fix(decorators)!: clear an L1-only cache whole on invalidate-all and let the memo own the kill (LAB-12024) (#677)
BREAKING CHANGE: an L1-only cache (
@cache(backend=None),@cache.local(), or any plaintext, non-interop decorator that finds no backend configured) times its TTLs and its stale-while-revalidate refresh on the OS monotonic clock that the Rust extension reads, not ontime.monotonic(). Patchingtime.monotonic(pytest'smonkeypatch, freezegun'sfreeze_timeortick) no longer expires its entries or brings a refresh due: a test that needs an entry gone callsfn.invalidate_cache(...)orfn.cache_clear(). Caches with a backend are unchanged.END_COMMIT_OVERRIDE
What was wrong
cache_clear()andinvalidate_cache()with no arguments deleted an L1-only cache's entries one by one, from the keys the process had recorded. A fill records its key only after its put, and a single-key invalidation untracks its key before its delete, so an entry stored before the call could outlive it and stay servable without the lock.cache_clear()cleared one key on a function without parameters. It calledinvalidate_cache()with no arguments, which on such a function deletes only the entry of the call without arguments. Akey=that reads its context files that call under many keys, so the other contexts' entries outlived the clear. This predates #673.HitEntrywithout a type check. Python code that reached the memo's dict throughgc.get_referentscould plant another object there and crash the process.METH_FASTCALLbuiltin that called the name descriptors' getters directly, cast memo slots unchecked and read the tests' clock through a raw pointer. SECURITY.md keeps unsafe code out of cachekit-rs, and the check that counts it runs after merge, so it failed onmain:cachekit-rs has 6 unsafe functions — must be zero.The fix
cache_clear()andinvalidate_cache()with no arguments clear an L1-only function'sObjectCachewhole. L1 is not tenant-scoped, and an L1-only function has nothing in L2, so this removes what the key-by-key deletes removed, plus any entry stored but not yet recorded. An L1-only fill therefore no longer records its key in_cached_keys, which had grown by one entry for every distinct key ever filled.cache_clear()takes the whole-function path directly, on a function without parameters too. A backed function'scache_clear()still goes throughinvalidate_cache(): without parameters it deletes the calling context's key and its pre-0.20.0 twin in L2, which a whole-function drain reaches only when this process recorded them.invalidate_cache()andainvalidate_cache()now share their preamble (settle a zero-config function, resolve a lazy backend, apply the interop guard).HitMemo.forget,repointandclearkill the entries they drop, so no removal route pairs a kill with a slot drop by hand, andHitEntry.killis no longer exposed to Python. The at-fork reset forgets every stored entry, then clears the memo. Dropping the slots already leaves no hit a way to an entry a refresh had in hand when a fork interrupted it; the clear kills that entry too, as defence in depth. A memo write returns early on a retired state; the take-back after the write stays, for a fork during the write itself.HitMemo.getis a#[pymethods]method, the function's names are read as attributes, a memo slot is cast withcast_exact, so a planted object is a miss andsetover it raisesTypeError, arguments are type-checked against PyO3's type objects, and the tests' clock override is a mutex beside an atomic flag. Beforemain's #674 merged in, cargo-geiger counted no unsafe function, expression or impl in the crate (Security Medium on this branch:cachekit-rs unsafe functions: 0). The crate root declares#![deny(unsafe_code)].msgpack_decode, which #674 added and which calls the CPython C API directly, is the one module that allows it. CI'sLint Rustclippy, on default features, rejects unsafe code outsidemsgpack_decodeunless the same change adds anallow(unsafe_code); the post-merge cargo-geiger check counts it either way. Security Fast's clippy builds without thepythonfeature, so it does not catch it.l1_hitmoves from 2,487 to 2,765 on CPython 3.14 and from 2,595 to 2,938 on 3.12 (re-recorded on the merge withmain, which now runs these paths over 300,000 calls), still about a quarter of the 10,904 and 11,600 the hit cost under the lock. On the hosted EPYC 7763 the dict's L1-only hit takes 0.341 ± 0.001μs (Bench run of this branch), against 0.311μs for #673's. The budgets are raised to match, and the docs quote the new figures. TheInstruction budgetcheck reads FAIL forl1_hitandminimal_l1_hitagainstmainbecause of this trade, by design.docs/backends/none.mdgains "TTLs Follow the OS Monotonic Clock", with how to test expiry, thettlentry indocs/api-reference.mdpoints there, thedocs/free-threading.mdaudit row matches the retired-state handling, SECURITY.md states which check rejects unsafe code in cachekit-rs and its one exempt module, and the README, the performance guide, the comparison page and the backend pages quote the hosted figure.MEMO_MAX_LEN,MEMO_MAX_ARGS,MEMO_MAX_ENTRIES) instead of copied intowrapper.py; the unusedHitEntry.valueandfresh_untilgetters and the two pyfunction shims are gone; the SWR jitter's low end is one named constant, andget_with_swrcomputes the floor once, which makes a locked SWR read 2.8% cheaper (4,789 to 4,656 instructions on 3.14).Tests
TestRemovalRoutes::test_a_whole_function_invalidation_removes_an_entry_it_has_no_record_of(cache_clear,invalidate_cache)TestRemovalRoutes::test_cache_clear_removes_every_entry_of_a_function_without_parameterscache_clear()throughinvalidate_cache()leaves the other context's entrytest_invalidate_no_args.py::TestInvalidateNoArgsWithL2Backend::test_cache_clear_without_parameters_deletes_the_key_in_l2TestRemovalRoutes::test_replacementputthat skips removing the entry it replacesTestTheCacheKeepsTheMemoInStep::test_a_fork_inside_a_refresh_leaves_the_old_entry_deadTestTheCacheKeepsTheMemoInStep::test_no_slot_is_set_for_an_entry_of_a_state_the_fork_reset_retiredTestTheCacheKeepsTheMemoInStep::test_an_object_planted_in_a_slot_is_a_miss_not_a_crashtest_no_value_is_served_after_its_removal_returnednow stamps every entry stored before acache_clear()as removed once the call returns, whatever way the clear empties the cache. Against key-by-key invalidation it fails on 3.14t with the GIL off in three runs of three; with the fix it passes on 3.14 and on 3.14t. Mutants of the two kills fail 6 tests (forget) and 1 test (repoint). The safe hit needs no new test: the same suites pin its behaviour, and CI'sLint Rustclippy rejects unsafe code outsidemsgpack_decodethat comes without anallow(unsafe_code).Results
Every row ran on the safe extension at this branch's code. Doctests 121, markdown docs 155 and
tests/docs77 pass. Ruff, basedpyright,cargo fmtandcargo clippy -D warningsare clean. cargo-geiger was clean untilmain'smsgpack_decodemerged in, and it now counts that module's unsafe functions.Instruction gate on 3.14:
l1_hitandminimal_l1_hitat their new budgets, and every other path within 1% of its budget (miss+0.68%, which read 0.7 to 0.9% over its budget onmainbefore this change too).Not changed
getandget_with_swrkeep their shared read bookkeeping inline. A helper costs every@cache.local()hit 9.1% more instructions (3,954 to 4,315 on 3.14), and the two blocks now point at each other._set_clockstays a test hook until someone needs to move L1-only time in their own tests.os.fork()holds the GIL or, free-threaded, stops the world first.