fix: invalidate_cache() with no args now clears all entries#108
Merged
Conversation
When invalidate_cache() or ainvalidate_cache() was called with no arguments on a function that accepts parameters, it generated a cache key for the zero-argument call (which was never cached) and silently did nothing. This left all cached entries intact despite the user's intent to clear everything. The fix tracks all cache keys per decorated function in a set. When invalidation is called with no args on a parameterized function, it iterates and deletes each tracked key from both L1 and L2, then clears the tracking set. This also fixes cache_clear() which calls invalidate_cache() internally. Prefix-based matching was considered but rejected because _normalize_key() hashes long keys (>250 chars), destroying the prefix structure needed for pattern matching. Closes #59
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR implements namespace-level cache invalidation for parameterized functions and adds pre-commit hooks to prevent internal documentation commits. The wrapper now tracks concrete L1 cache keys per-decorator and enables bulk invalidation when ChangesNamespace-Level Cache Invalidation
Pre-commit Hook for Internal Documentation
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/unit/test_invalidate_no_args.py (1)
27-28: ⚡ Quick winAdd at least one backend-backed invalidation test.
Every case here uses
backend=None, so the new backend/L2 mass-invalidation path is still untested. Given the wrapper changes in both sync and async invalidation, a backend-backed case is the one most likely to catch regressions before release.Also applies to: 58-59, 81-82, 102-103, 132-133, 162-169
🤖 Prompt for 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. In `@tests/unit/test_invalidate_no_args.py` around lines 27 - 28, Add at least one test that uses a real backend instead of backend=None to exercise the L2 mass-invalidation path: replace the decorator on the existing expensive(query: str) function (the one decorated with cache(backend=None, ttl=300, namespace="test_sync_invalidate_no_args")) with a backend-backed cache (use the project’s in-memory or test backend fixture), call the cached expensive function to populate the backend, invoke the mass invalidation API on the wrapper (the invalidate/no-args invalidation path exposed by the cached wrapper), then call expensive again and assert it was recomputed (e.g., via a call counter or returned marker) to verify backend-backed invalidation occurred; replicate similar changes for the other test variants noted (the other expensive definitions at the same pattern).
🤖 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/cachekit/decorators/wrapper.py`:
- Around line 1313-1324: The no-arg invalidation branch (inside the wrapper
where _cached_keys, _l1_cache, _backend, invalidator and _l1_only_mode are used)
currently clears _cached_keys unconditionally after attempting L2 deletes and
swallows exceptions; change this so each key is removed from _cached_keys only
when its L2 delete succeeds — i.e., still call _l1_cache.invalidate(key)
immediately if _l1_cache is present, attempt backend.delete(key) under
invalidator.set_backend(_backend) and on success remove the key from
_cached_keys, but if backend.delete raises keep the key in _cached_keys (and
leave the exception logged via _logger.debug) so it can be retried later; apply
the same fix to the other similar no-arg invalidation branch around the
1352-1363 region.
- Around line 491-495: The _cached_keys set is accessed concurrently; create a
dedicated lock (e.g., _cached_keys_lock = threading.RLock()) next to the
_cached_keys declaration and use it to guard all mutations and snapshots: wrap
add/discard calls that update _cached_keys inside with _cached_keys_lock:, take
a snapshot of the set under the lock (e.g., keys = set(_cached_keys)) before
iterating or clearing, and perform clear() under the lock as well; apply the
same pattern for all places that touch _cached_keys and for invalidate_cache()
so iteration never races with concurrent mutations.
- Around line 491-495: The wrapper currently uses an in-memory set _cached_keys
to track keys, which can't clear shared L2 state across processes; replace this
process-local registry with a backend-scoped registry (or use backend-supported
function-level delete). Concretely: stop using the module-level _cached_keys;
when the wrapper writes a cache entry, also add the normalized key to a
backend-managed list/set namespaced by the function identifier (e.g., call
backend.add_function_key(function_id, key)); when
invalidate_cache()/cache_clear() is called with no args, fetch the stored keys
from the backend (backend.get_function_keys(function_id)), delete those keys via
the cache backend, and then remove/expire the registry
(backend.delete_function_keys(function_id)); ensure writes to the registry occur
on every cache miss/store and guard against race conditions/time-to-live for
stale entries. Use the existing wrapper function identifier (function name +
module or generated id used by the decorator) and the methods
invalidate_cache/cache_clear to locate where to call the backend registry APIs.
---
Nitpick comments:
In `@tests/unit/test_invalidate_no_args.py`:
- Around line 27-28: Add at least one test that uses a real backend instead of
backend=None to exercise the L2 mass-invalidation path: replace the decorator on
the existing expensive(query: str) function (the one decorated with
cache(backend=None, ttl=300, namespace="test_sync_invalidate_no_args")) with a
backend-backed cache (use the project’s in-memory or test backend fixture), call
the cached expensive function to populate the backend, invoke the mass
invalidation API on the wrapper (the invalidate/no-args invalidation path
exposed by the cached wrapper), then call expensive again and assert it was
recomputed (e.g., via a call counter or returned marker) to verify
backend-backed invalidation occurred; replicate similar changes for the other
test variants noted (the other expensive definitions at the same pattern).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ca069a20-cd09-4428-97a4-e323c80a494f
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockuv.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
src/cachekit/decorators/wrapper.pytests/unit/test_invalidate_no_args.py
Comment on lines
+491
to
+495
| # Track all cache keys written by this function (for no-args invalidation). | ||
| # When invalidate_cache() is called with no args on a parameterized function, | ||
| # we need to clear ALL entries — but key normalization (hashing of long keys) | ||
| # makes prefix matching unreliable. Tracking actual keys is simple and correct. | ||
| _cached_keys: set[str] = set() |
There was a problem hiding this comment.
Synchronize _cached_keys before mutating or iterating it.
The wrapper adds/discards keys on the request path and iterates the same set during invalidation without any lock. Concurrent calls can raise RuntimeError: Set changed size during iteration or miss keys that are added while the clear is in progress. Guard add/discard/snapshot/clear with a shared lock.
Also applies to: 603-603, 811-811, 945-945, 1048-1048, 1175-1175, 1255-1255, 1313-1324, 1331-1331, 1352-1363, 1370-1370
🤖 Prompt for 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.
In `@src/cachekit/decorators/wrapper.py` around lines 491 - 495, The _cached_keys
set is accessed concurrently; create a dedicated lock (e.g., _cached_keys_lock =
threading.RLock()) next to the _cached_keys declaration and use it to guard all
mutations and snapshots: wrap add/discard calls that update _cached_keys inside
with _cached_keys_lock:, take a snapshot of the set under the lock (e.g., keys =
set(_cached_keys)) before iterating or clearing, and perform clear() under the
lock as well; apply the same pattern for all places that touch _cached_keys and
for invalidate_cache() so iteration never races with concurrent mutations.
Process-local key tracking can't clear shared L2 state.
_cached_keys only records keys observed by this wrapper instance. After a restart, or when another worker populates the same backend entries, no-arg invalidation will leave those L2 keys behind, so invalidate_cache() / cache_clear() become partial for shared backends. This needs a backend-scoped registry or backend-supported function-level delete, not an in-memory set.
🤖 Prompt for 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.
In `@src/cachekit/decorators/wrapper.py` around lines 491 - 495, The wrapper
currently uses an in-memory set _cached_keys to track keys, which can't clear
shared L2 state across processes; replace this process-local registry with a
backend-scoped registry (or use backend-supported function-level delete).
Concretely: stop using the module-level _cached_keys; when the wrapper writes a
cache entry, also add the normalized key to a backend-managed list/set
namespaced by the function identifier (e.g., call
backend.add_function_key(function_id, key)); when
invalidate_cache()/cache_clear() is called with no args, fetch the stored keys
from the backend (backend.get_function_keys(function_id)), delete those keys via
the cache backend, and then remove/expire the registry
(backend.delete_function_keys(function_id)); ensure writes to the registry occur
on every cache miss/store and guard against race conditions/time-to-live for
stale entries. Use the existing wrapper function identifier (function name +
module or generated id used by the decorator) and the methods
invalidate_cache/cache_clear to locate where to call the backend registry APIs.
…rage 1. Partial L2 failure: keys are now removed from _cached_keys individually on successful L2 delete. If backend.delete() fails, the key stays tracked so the next invalidate_cache() call can retry. 2. Thread safety: snapshot _cached_keys before iterating in both invalidation paths to prevent RuntimeError from concurrent .add() by another thread. Full RLock wrapping was rejected — set.add() and .discard() are atomic under CPython's GIL; only iteration during mutation is the actual race. 3. L2 test coverage: added TestInvalidateNoArgsWithL2Backend using FileBackend to exercise backend.delete() in the mass-invalidation path, plus a partial-failure test verifying key retention on L2 delete errors. Skipped: backend-scoped key registry (distributed invalidation). This would require new BaseBackend protocol methods across all backends — a feature request, not a bug fix. The existing per-key invalidation has the same single-process scope.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Add 2 async FileBackend tests covering the ainvalidate_cache L2 delete path (lines 1358-1366) and the async single-key discard path (line 1375): partial-failure key retention and per-arg invalidation. Remaining 4 uncovered lines are _cached_keys.add() inside the L1+L2 async wrapper body (lines 1048/1113/1139/1175) which require a live Redis connection — integration test territory.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs/superpowers/specs/2026-05-14-distributed-key-registry-design.md`:
- Around line 30-36: The current delete_tracked(group: str) -> tuple[int,
list[str]] should stop materializing list_of_deleted_keys to avoid O(N)
memory/transfer; change it to return only an int (count_deleted) and modify the
implementation in delete_tracked to use SSCAN + batched pipelined DELs as before
but simply accumulate a numeric counter instead of appending keys, and
remove/avoid building the list_of_deleted_keys; keep error propagation as
BackendError so the tracking set remains for retries, and update any callers
(e.g., invalidate_cache) to expect a single integer return value rather than a
tuple.
- Around line 82-87: When set_backend() switches KeyTracker from local to
backend mode it currently sets self._use_backend and self._backend but never
migrates existing self._local_keys, causing those keys to be missed by backend
invalidation; update set_backend(self, backend) to iterate over self._local_keys
and call backend.track_key(...) (or the tracker interface method) for each key,
handle duplicates/deduplication as needed, clear or mark transferred entries in
self._local_keys after successful transfer, and preserve thread-safety (e.g.,
acquire the same lock used by other KeyTracker methods) so no keys are lost
during the transition.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e563645f-f82e-4b51-bcb0-cb39d3c8bc37
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
.github/workflows/security-fast.ymldocs/superpowers/specs/2026-05-14-distributed-key-registry-design.mdpyproject.toml
Comment on lines
+30
to
+36
| **`delete_tracked(group: str) -> tuple[int, list[str]]`** | ||
|
|
||
| - Uses SSCAN (cursor-based, non-blocking) to iterate tracking set members | ||
| - Pipelines DELETE in batches of 1000 keys | ||
| - DELs the tracking set after all keys are deleted | ||
| - Returns `(count_deleted, list_of_deleted_keys)` | ||
| - Errors propagate as `BackendError` — keys remain in tracking set for retry on next `invalidate_cache()` call |
There was a problem hiding this comment.
Avoid materializing list_of_deleted_keys for mass invalidation.
Returning all deleted keys scales poorly (memory and transfer) for large tracking sets, and this flow currently only needs aggregate deletion behavior. Prefer returning just a count (or streaming internally) to keep invalidation bounded.
Also applies to: 159-160
🤖 Prompt for 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.
In `@docs/superpowers/specs/2026-05-14-distributed-key-registry-design.md` around
lines 30 - 36, The current delete_tracked(group: str) -> tuple[int, list[str]]
should stop materializing list_of_deleted_keys to avoid O(N) memory/transfer;
change it to return only an int (count_deleted) and modify the implementation in
delete_tracked to use SSCAN + batched pipelined DELs as before but simply
accumulate a numeric counter instead of appending keys, and remove/avoid
building the list_of_deleted_keys; keep error propagation as BackendError so the
tracking set remains for retries, and update any callers (e.g.,
invalidate_cache) to expect a single integer return value rather than a tuple.
Comment on lines
+82
to
+87
| def set_backend(self, backend: Any) -> None: | ||
| """Late-bind backend (for lazy initialization in wrapper).""" | ||
| if hasattr(backend, 'track_key'): | ||
| self._use_backend = True | ||
| self._backend = backend | ||
| ``` |
There was a problem hiding this comment.
Preserve pre-backend tracked keys during set_backend() transition.
When KeyTracker switches from local to backend mode, existing self._local_keys are never flushed to backend tracking, so those entries can be missed by later mass invalidation.
Proposed fix
def set_backend(self, backend: Any) -> None:
"""Late-bind backend (for lazy initialization in wrapper)."""
if hasattr(backend, 'track_key'):
- self._use_backend = True
- self._backend = backend
+ # migrate already-tracked local keys before switching modes
+ for key in self._local_keys:
+ backend.track_key(group, key) # group should be stored on tracker or passed in
+ self._local_keys.clear()
+ self._use_backend = True
+ self._backend = backend🤖 Prompt for 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.
In `@docs/superpowers/specs/2026-05-14-distributed-key-registry-design.md` around
lines 82 - 87, When set_backend() switches KeyTracker from local to backend mode
it currently sets self._use_backend and self._backend but never migrates
existing self._local_keys, causing those keys to be missed by backend
invalidation; update set_backend(self, backend) to iterate over self._local_keys
and call backend.track_key(...) (or the tracker interface method) for each key,
handle duplicates/deduplication as needed, clear or mark transferred entries in
self._local_keys after successful transfer, and preserve thread-safety (e.g.,
acquire the same lock used by other KeyTracker methods) so no keys are lost
during the transition.
27Bslash6
force-pushed
the
fix/59-invalidate-cache-no-op
branch
from
c0e6d03 to
6b4f5ec
Compare
Adds check-no-internal-docs hook that rejects staged files matching internal development patterns: docs/superpowers/, .spec-workflow/specs/, strategy/, tooling/sessions/, CALIBER_LEARNINGS.md, .caliber/. These artifacts belong in the private tooling repo or MCP memory, not in the public OSS repo.
All 3 CVEs are in dev-only transitive dependencies with no runtime impact on cachekit users. Fixes require Python 3.10+ but cachekit supports 3.9. Ignoring is the correct approach per project convention.
27Bslash6
force-pushed
the
fix/59-invalidate-cache-no-op
branch
from
e4d166d to
3482a22
Compare
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
invalidate_cache()/ainvalidate_cache()called with no arguments on a parameterized function now clears ALL cached entries for that function, instead of silently no-oppingset[str]; on no-args invalidation, iterates and deletes each key from L1 and L2cache_clear()which callsinvalidate_cache()internallyfn_adoes not affectfn_beven when they share a namespaceWhy key tracking instead of prefix matching
Cache keys can exceed 250 chars (common with nested test classes), causing
_normalize_key()to hash-truncate them. This destroys the prefix structure, makingstartswith()matching unreliable. Tracking actual keys is simple, correct, and handles normalization transparently.Test plan
test_sync_invalidate_no_args_clears_all_entries— sync invalidation clears all entriestest_sync_invalidate_with_args_clears_single_entry— per-key invalidation still workstest_sync_no_param_function_invalidate_still_works— zero-param functions unaffectedtest_async_invalidate_no_args_clears_all_entries— async varianttest_cache_clear_clears_all_entries—cache_clear()works for parameterized functionstest_invalidate_does_not_affect_other_functions_same_namespace— cross-function isolationSummary by CodeRabbit
New Features
Tests
Chores