From 3f8e8423556fa4eb66f7452a9153f52a3a33e4ac Mon Sep 17 00:00:00 2001 From: Ray Walker Date: Mon, 21 Sep 2026 17:15:04 +1000 Subject: [PATCH 1/6] fix(decorators): honour set_default_backend() when called after decoration (LAB-4457) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The intent decorator resolved the module-level default at decoration (import) time only. In the normal app layout — business modules imported at the top, set_default_backend() in main() — the default was captured as None and the wrapper silently fell through to environment auto-detection. A cache pointed at the wrong store returns plausible wrong data, with no error. The wrapper now routes all five lazy get_backend() sites through one helper that consults get_default_backend() before DefaultBackendProvider, at first call. The decoration-time lookup in intent.py stays so configure-first apps keep their decoration-time interop guard unchanged. Docs: the resolution-order section listed _resolve_backend()'s dead three-tier list; replaced with DefaultBackendProvider's five selectors and made the set_default_backend() example executable. --- docs/backends/README.md | 54 +++++++++++++------ src/cachekit/decorators/intent.py | 6 ++- src/cachekit/decorators/wrapper.py | 24 +++++++-- ...test_file_backend_decorator_integration.py | 19 +++++++ 4 files changed, 81 insertions(+), 22 deletions(-) diff --git a/docs/backends/README.md b/docs/backends/README.md index 949994a4..49cdcd1b 100644 --- a/docs/backends/README.md +++ b/docs/backends/README.md @@ -170,43 +170,65 @@ def explicit_backend(): ### 2. Module-Level Default Backend (Middle Priority) -```python notest +```python +import tempfile + from cachekit import cache from cachekit.config.decorator import set_default_backend from cachekit.backends.file import FileBackend, FileBackendConfig # Set once at application startup -file_backend = FileBackend(FileBackendConfig(cache_dir="/var/cache/myapp")) +file_backend = FileBackend(FileBackendConfig(cache_dir=tempfile.mkdtemp())) set_default_backend(file_backend) # All decorators now use file backend — no backend= needed @cache.minimal(ttl=300) -def fast_lookup(): - return data() +def fast_lookup(x: int) -> int: + return x * 2 @cache.production(ttl=600) -def critical_function(): - return data() +def critical_function(x: int) -> int: + return x * 3 + +assert fast_lookup(2) == 4 and critical_function(2) == 6 + +set_default_backend(None) # clear the default ``` Call `set_default_backend(None)` to clear the default. Works with any backend (Redis, File, CachekitIO, custom). +**Call order does not matter for backend selection.** A decorator applied +without `backend=` pins the default when it is first seen — at decoration if +already set, otherwise at first call — so the usual layout (business modules +imported at the top of the file, `set_default_backend()` in `main()`) works. +Later `set_default_backend()` calls do not re-point already-pinned functions. +Exception: `stale_ttl` and `@cache.io`'s default stale window validate SWR +capability at decoration, so set a CachekitIO default *before* importing modules +that use them. + ### 3. Environment Variable Auto-Detection (Lowest Priority) -```bash -# Primary: CACHEKIT_REDIS_URL -CACHEKIT_REDIS_URL=redis://prod.example.com:6379 +If no explicit backend and no module-level default, `DefaultBackendProvider` +picks a backend from exactly one environment selector, in this order: -# Fallback: REDIS_URL -REDIS_URL=redis://localhost:6379 -``` +| Priority | Environment variable | Backend | +|----------|-----------------------------|--------------------| +| 1 | `CACHEKIT_API_KEY` | `CachekitIOBackend` (SaaS) | +| 2 | `CACHEKIT_REDIS_URL` | `RedisBackend` | +| 3 | `CACHEKIT_MEMCACHED_SERVERS`| `MemcachedBackend` | +| 4 | `CACHEKIT_FILE_CACHE_DIR` | `FileBackend` | +| 5 | `REDIS_URL`, or nothing set | `RedisBackend` (localhost fallback) | -If no explicit backend and no module-level default, cachekit creates a RedisBackend from environment variables. +Setting more than one of the four `CACHEKIT_*` selectors is ambiguous and raises +`ConfigurationError` at first call. The decorator catches it, logs a WARNING on +the `cachekit.decorators.orchestrator` logger, and runs the function uncached. +`REDIS_URL` is a 12-factor fallback and never counts as a conflict. **Resolution order**: -1. Check for explicit `backend` parameter in `@cache(backend=...)` -2. Check for module-level default via `set_default_backend()` -3. Create RedisBackend from environment variables (CACHEKIT_REDIS_URL > REDIS_URL) +1. Explicit `backend` parameter in `@cache(backend=...)` +2. Module-level default via `set_default_backend()` (checked at decoration, and + again at first call if still unset) +3. Environment auto-detection per the table above ## Performance Considerations diff --git a/src/cachekit/decorators/intent.py b/src/cachekit/decorators/intent.py index 4e4df964..43670ef2 100644 --- a/src/cachekit/decorators/intent.py +++ b/src/cachekit/decorators/intent.py @@ -137,7 +137,11 @@ def decorator(f: F) -> F: backend = manual_overrides.pop("backend", None) # Tier 2 resolution: if no explicit backend and not L1-only mode, - # check module-level default set via set_default_backend() + # check module-level default set via set_default_backend(). Kept here + # (not only lazily) because decoration-time validation — the interop + # backend guard and stale_ttl/SWR capability (LAB-557) — needs the + # backend when it is already known. If the default is set LATER, the + # wrapper re-consults it at first call (_resolve_lazy_backend, LAB-4457). if backend is None and not _explicit_l1_only: from ..config.decorator import get_default_backend diff --git a/src/cachekit/decorators/wrapper.py b/src/cachekit/decorators/wrapper.py index 0b3e476a..e94ba390 100644 --- a/src/cachekit/decorators/wrapper.py +++ b/src/cachekit/decorators/wrapper.py @@ -48,6 +48,20 @@ if TYPE_CHECKING: from ..serializers.base import SerializerProtocol + +def _resolve_lazy_backend() -> Any: + """Backend for a decorator that was applied without ``backend=``. + + Consulted at FIRST CALL, not at decoration, so ``set_default_backend()`` + takes effect regardless of whether it ran before or after the module holding + the decorated function was imported (LAB-4457). + """ + from ..config.decorator import get_default_backend + + default = get_default_backend() + return default if default is not None else get_backend_provider().get_backend() + + F = TypeVar("F", bound=Callable[..., Any]) _logger = logging.getLogger(__name__) @@ -1263,7 +1277,7 @@ def sync_wrapper(*args: Any, **kwargs: Any) -> Any: # noqa: PLR0912 nonlocal _backend if _backend is None: - _backend = get_backend_provider().get_backend() + _backend = _resolve_lazy_backend() # Setup cache handler strategy on first use handler = StandardCacheHandler( @@ -1672,7 +1686,7 @@ async def async_wrapper(*args: Any, **kwargs: Any) -> Any: if interop is not None: if _backend is None: try: - _backend = get_backend_provider().get_backend() + _backend = _resolve_lazy_backend() except Exception as e: # If Redis connection fails, execute function without caching - RETURN EARLY # This prevents the decorator from breaking the application @@ -1746,7 +1760,7 @@ async def async_wrapper(*args: Any, **kwargs: Any) -> Any: # Initialize backend only when needed (lazy init for performance) if _backend is None: try: - _backend = get_backend_provider().get_backend() + _backend = _resolve_lazy_backend() except Exception as e: # If Redis connection fails, execute function without caching - RETURN EARLY # This prevents the decorator from breaking the application @@ -2078,7 +2092,7 @@ def invalidate_cache(*args: Any, **kwargs: Any) -> None: # we should NOT try to get a backend from the provider if not _l1_only_mode and _backend is None: try: - _backend = get_backend_provider().get_backend() + _backend = _resolve_lazy_backend() except Exception as e: # If backend creation fails, can't invalidate L2 _logger.debug("Failed to get backend for invalidation: %s", redact_error_for_log(e)) @@ -2138,7 +2152,7 @@ async def ainvalidate_cache(*args: Any, **kwargs: Any) -> None: # we should NOT try to get a backend from the provider if not _l1_only_mode and _backend is None: try: - _backend = get_backend_provider().get_backend() + _backend = _resolve_lazy_backend() except Exception as e: # If backend creation fails, can't invalidate L2 _logger.debug("Failed to get backend for async invalidation: %s", redact_error_for_log(e)) diff --git a/tests/integration/test_file_backend_decorator_integration.py b/tests/integration/test_file_backend_decorator_integration.py index 067b8a7b..fcecbc91 100644 --- a/tests/integration/test_file_backend_decorator_integration.py +++ b/tests/integration/test_file_backend_decorator_integration.py @@ -181,6 +181,25 @@ def compute(x: int) -> int: finally: set_default_backend(original) + def test_set_default_backend_after_decoration(self, tmp_path: Path) -> None: + """set_default_backend() called AFTER decoration still takes effect at first call (LAB-4457).""" + original = get_default_backend() + try: + set_default_backend(None) + + @cache.minimal(ttl=300) # decorate FIRST: default is None here + def compute(x: int) -> int: + return x * 5 + + chosen = _make_file_backend(tmp_path, subdir="chosen") + set_default_backend(chosen) # configure SECOND + + assert compute(2) == 10 + # Proof the explicit choice was used, not the env-detected fallback. + assert any((tmp_path / "chosen").iterdir()) + finally: + set_default_backend(original) + def test_explicit_backend_overrides_default(self, tmp_path: Path) -> None: """Explicit backend= kwarg takes precedence over set_default_backend().""" default_backend = _make_file_backend(tmp_path, subdir="default_cache") From 3d950252bc2555f17dc2f37952c752062c7b0807 Mon Sep 17 00:00:00 2001 From: Ray Walker Date: Mon, 21 Sep 2026 19:25:27 +1000 Subject: [PATCH 2/6] =?UTF-8?q?fix:=20address=20coderabbit=20review=20?= =?UTF-8?q?=E2=80=94=20qualify=20backend=20call-order=20docs,=20tighten=20?= =?UTF-8?q?=5Fresolve=5Flazy=5Fbackend()=20return=20type?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review findings addressed, one rejected with evidence (LAB-4457): - docs/backends/README.md: reworded the bolded headline so it no longer overclaims "call order doesn't matter" — import order is what's free; set_default_backend() must still run before a decorated function's first call. Body paragraph and stale_ttl/@cache.io exception untouched. - src/cachekit/decorators/wrapper.py: annotated _resolve_lazy_backend() -> BaseBackend (the existing backends.base Protocol) instead of Any, imported under TYPE_CHECKING alongside SerializerProtocol. No runtime behavior change (from __future__ import annotations already stringifies it, and nothing calls get_type_hints() on this function). Tightening the annotation surfaced 3 basedpyright errors at the optional-capability call sites (get_ttl/refresh_ttl/acquire_lock, gated behind hasattr() since not every backend implements them) that were previously masked by Any: the TTL pair now narrows via cast("TTLInspectableBackend", _backend) and the lock site now uses getattr(_backend, "acquire_lock", None), mirroring the identical pattern already used for the same lookup in _l2_swr_revalidate_async a few hundred lines up. - tests/integration/test_file_backend_decorator_integration.py: rejected Kody's freezegun.freeze_time() request. freezegun is not a project dependency (absent from pyproject.toml and uv.lock), and test_set_default_backend_after_decoration calls compute(2) once, milliseconds after decoration, asserting only which backend directory was written to — it never checks expiry and never approaches the 300s TTL boundary, so there's no wall-clock sensitivity to freeze. CodeRabbit-Resolved: docs/backends/README.md:200:Qualify the call-order claim. CodeRabbit-Resolved: src/cachekit/decorators/wrapper.py:62:Imprecise return type in _res CodeRabbit-Resolved: tests/integration/test_file_backend_decorator_integration.py:197:Time-dependent test in test_f --- docs/backends/README.md | 3 ++- src/cachekit/decorators/wrapper.py | 15 +++++++++------ 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/docs/backends/README.md b/docs/backends/README.md index 49cdcd1b..ba126989 100644 --- a/docs/backends/README.md +++ b/docs/backends/README.md @@ -197,7 +197,8 @@ set_default_backend(None) # clear the default Call `set_default_backend(None)` to clear the default. Works with any backend (Redis, File, CachekitIO, custom). -**Call order does not matter for backend selection.** A decorator applied +**Import order does not matter, but configuration must happen before a decorated +function's first call.** A decorator applied without `backend=` pins the default when it is first seen — at decoration if already set, otherwise at first call — so the usual layout (business modules imported at the top of the file, `set_default_backend()` in `main()`) works. diff --git a/src/cachekit/decorators/wrapper.py b/src/cachekit/decorators/wrapper.py index e94ba390..ded5baac 100644 --- a/src/cachekit/decorators/wrapper.py +++ b/src/cachekit/decorators/wrapper.py @@ -10,7 +10,7 @@ import threading import time from collections.abc import Callable -from typing import TYPE_CHECKING, Any, NamedTuple, TypeVar, Union +from typing import TYPE_CHECKING, Any, NamedTuple, TypeVar, Union, cast from cachekit.hash_utils import redact_error_for_log @@ -46,10 +46,11 @@ from .tenant_context import TenantContextExtractor if TYPE_CHECKING: + from ..backends.base import BaseBackend, TTLInspectableBackend from ..serializers.base import SerializerProtocol -def _resolve_lazy_backend() -> Any: +def _resolve_lazy_backend() -> BaseBackend: """Backend for a decorator that was applied without ``backend=``. Consulted at FIRST CALL, not at decoration, so ``set_default_backend()`` @@ -1820,11 +1821,12 @@ async def async_wrapper(*args: Any, **kwargs: Any) -> Any: # Handle TTL refresh if configured and threshold met if refresh_ttl_on_get and ttl and hasattr(_backend, "get_ttl") and hasattr(_backend, "refresh_ttl"): + _ttl_backend = cast("TTLInspectableBackend", _backend) try: - remaining_ttl = await _backend.get_ttl(cache_key) + remaining_ttl = await _ttl_backend.get_ttl(cache_key) if remaining_ttl and remaining_ttl < (ttl * ttl_refresh_threshold): # Refresh TTL in background with error callback - task = asyncio.create_task(_backend.refresh_ttl(cache_key, ttl)) + task = asyncio.create_task(_ttl_backend.refresh_ttl(cache_key, ttl)) task.add_done_callback(lambda t: _ttl_refresh_done_callback(t, cache_key)) except Exception as e: # TTL refresh is optional, don't fail on error @@ -1873,10 +1875,11 @@ async def async_wrapper(*args: Any, **kwargs: Any) -> Any: blocking_timeout = 5.0 # Wait up to 5 seconds to acquire lock # Check if backend supports distributed locking - if hasattr(_backend, "acquire_lock"): + _acquire_lock = getattr(_backend, "acquire_lock", None) + if _acquire_lock is not None: try: # Use backend's async lock protocol - async with _backend.acquire_lock( + async with _acquire_lock( cache_key, timeout=lock_timeout, blocking_timeout=blocking_timeout, From 4c67b887f754aee6fa974c3dcb41faf1f4c6a3aa Mon Sep 17 00:00:00 2001 From: Ray Walker Date: Mon, 21 Sep 2026 19:57:55 +1000 Subject: [PATCH 3/6] fix(backends): type LockableBackend.acquire_lock as the async CM it actually is (LAB-4457) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Kody flagged the `getattr(_backend, "acquire_lock", None)` introduced in 3d95025 as type loss: the resulting callable is implicitly `Any`, so a mistyped argument to `acquire_lock` is invisible to the checker. Correct diagnosis. Its proposed remedy — define a `LockableBackend` protocol and `cast()` to it — does not compile, and the reason is the actual defect. `LockableBackend` already exists in backends/base. It declared `async def acquire_lock(...) -> AsyncIterator[bool]`, the shape of the *undecorated* generator, while every implementation (RedisBackendProvider's PerRequestRedisBackend, CachekitIOBackend) is `@asynccontextmanager`-wrapped and therefore returns `_AsyncGeneratorContextManager[bool]`. So no backend structurally satisfied the protocol, and narrowing to it made `async with` a type error: error: Object of type "CoroutineType[Any, Any, AsyncIterator[bool]]" cannot be used with "async with" because it does not correctly implement __aenter__ The protocol now declares the decorated shape, `def acquire_lock(...) -> AbstractAsyncContextManager[bool]`, which both implementations satisfy. The two wrapper call sites that invoke the lock — the thundering-herd guard on the async miss path and the SWR revalidation lease — narrow with `isinstance(_backend, LockableBackend)` instead of `getattr`. For a single-method runtime_checkable Protocol that is the same runtime test as `hasattr`, so the skip-when-unsupported behaviour on FileBackend / L1-only is unchanged; unlike `getattr` it also narrows for the checker. Red-verified: passing `timeout="oops"` at the lock site is now a basedpyright error; before this change it was silently accepted. The log-only `hasattr(_backend, "acquire_lock")` further down is left alone — it never invokes the callable, so it has nothing to lose. Docs: docs/features/distributed-locking.md carried the same undecorated signature in its protocol snippet; updated to match. basedpyright --level error: 0 errors. 3095 tests pass (non-slow, non-performance, excluding the credentialed SaaS e2e suite). --- docs/features/distributed-locking.md | 10 +++++++--- src/cachekit/backends/base.py | 14 +++++++++----- src/cachekit/decorators/wrapper.py | 11 +++++------ 3 files changed, 21 insertions(+), 14 deletions(-) diff --git a/docs/features/distributed-locking.md b/docs/features/distributed-locking.md index bec2efa3..70052a57 100644 --- a/docs/features/distributed-locking.md +++ b/docs/features/distributed-locking.md @@ -238,16 +238,20 @@ def cheap_lookup(x): The `LockableBackend` protocol defines how backends provide distributed locking: ```python notest -async def acquire_lock( +def acquire_lock( self, key: str, # Bare cache key (same key as get/set); backend derives lock namespace timeout: float, # How long to hold the lock (seconds) blocking_timeout: Optional[float] = None, # Max wait to acquire (None = non-blocking) -) -> AsyncIterator[bool]: - # Yields True if lock acquired, False if timeout waiting +) -> AbstractAsyncContextManager[bool]: + # Entering the context yields True if the lock was acquired, False if the wait timed out ... ``` +Implementations are `async` generators wrapped in `@asynccontextmanager`, so the +protocol declares the *decorated* shape. That is what lets a caller narrow with +`isinstance(backend, LockableBackend)` and still type-check the `async with`. + The decorator wrapper calls it with `timeout=30.0` (lock self-expiry) and `blocking_timeout=5.0` (max wait to acquire) — see [Lock Timing](#lock-timing-30-second-expiry-5-second-wait). diff --git a/src/cachekit/backends/base.py b/src/cachekit/backends/base.py index c1f3503b..a158db18 100644 --- a/src/cachekit/backends/base.py +++ b/src/cachekit/backends/base.py @@ -11,6 +11,7 @@ from __future__ import annotations from collections.abc import AsyncIterator, Callable +from contextlib import AbstractAsyncContextManager from typing import Any, BinaryIO, Optional, Protocol, runtime_checkable # Re-export BackendError for convenience (public API) @@ -293,12 +294,12 @@ class LockableBackend(Protocol): >>> # result = expensive_computation() """ - async def acquire_lock( + def acquire_lock( self, key: str, timeout: float, blocking_timeout: Optional[float] = None, - ) -> AsyncIterator[bool]: + ) -> AbstractAsyncContextManager[bool]: """Acquire a distributed lock on key. Args: @@ -309,9 +310,12 @@ async def acquire_lock( timeout: How long to hold the lock (seconds) before auto-release blocking_timeout: Max time to wait for lock acquisition (None = non-blocking) - Yields: - True if lock was acquired - False if timeout occurred waiting for lock + Returns: + An async context manager yielding True if the lock was acquired, + False if the wait timed out. Implementations are ``async`` + generators wrapped in ``@asynccontextmanager``; the protocol + declares the *decorated* shape so callers can narrow to + ``LockableBackend`` and still type-check ``async with``. Raises: BackendError: If backend operation fails diff --git a/src/cachekit/decorators/wrapper.py b/src/cachekit/decorators/wrapper.py index ded5baac..014c58f6 100644 --- a/src/cachekit/decorators/wrapper.py +++ b/src/cachekit/decorators/wrapper.py @@ -14,6 +14,7 @@ from cachekit.hash_utils import redact_error_for_log +from ..backends.base import LockableBackend from ..backends.errors import BackendError, BackendErrorType from ..cache_handler import ( CacheInvalidator, @@ -922,9 +923,8 @@ async def _l2_swr_revalidate_async(cache_key: str, call_args: tuple[Any, ...], c the caller already got the stale value; the entry hard-expires at evict_at and the next request takes the ordinary synchronous miss path (spec degradation).""" try: - _acquire_lock = getattr(_backend, "acquire_lock", None) - if _acquire_lock is not None: - async with _acquire_lock(cache_key, timeout=_l2_swr_lease_seconds, blocking_timeout=None) as got_lease: + if isinstance(_backend, LockableBackend): + async with _backend.acquire_lock(cache_key, timeout=_l2_swr_lease_seconds, blocking_timeout=None) as got_lease: if not got_lease: return # another client is revalidating — stale already served await _l2_swr_recompute_store_async(cache_key, call_args, call_kwargs) @@ -1875,11 +1875,10 @@ async def async_wrapper(*args: Any, **kwargs: Any) -> Any: blocking_timeout = 5.0 # Wait up to 5 seconds to acquire lock # Check if backend supports distributed locking - _acquire_lock = getattr(_backend, "acquire_lock", None) - if _acquire_lock is not None: + if isinstance(_backend, LockableBackend): try: # Use backend's async lock protocol - async with _acquire_lock( + async with _backend.acquire_lock( cache_key, timeout=lock_timeout, blocking_timeout=blocking_timeout, From fc52e3ce96a520d5f02388f30b3ddb6c62efc6d9 Mon Sep 17 00:00:00 2001 From: Ray Walker Date: Mon, 21 Sep 2026 20:14:40 +1000 Subject: [PATCH 4/6] fix(decorators): probe lock capability with hasattr, not protocol isinstance (LAB-4457) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 4c67b88 swapped the three `acquire_lock` capability probes for `isinstance(_backend, LockableBackend)` and claimed it was "the same runtime test as hasattr". It is not, and the expert panel caught it. Since CPython 3.12, `_ProtocolMeta.__instancecheck__` resolves protocol members with `inspect.getattr_static`, which deliberately does not consult `__getattr__` (so a protocol check cannot execute user code). Measured against this repo's own delegating-proxy fixture: py3.11.14 hasattr=True isinstance=True py3.12.12 hasattr=True isinstance=False py3.13.12 hasattr=True isinstance=False `requires-python = ">=3.10"` and CI runs 3.10-3.14, so a backend that delegates `acquire_lock` through `__getattr__` — an instrumentation, tenant- routing or retry wrapper; `tests/backends/fixtures.py` builds one — kept the thundering-herd lock on 3.10/3.11 and silently lost it on 3.12+. The SWR revalidation lease degraded the same way, and the only diagnostic (the `hasattr` log guard left behind at the bottom of the miss path) was the *old* predicate, so it stayed silent for exactly the objects the new one rejected: stampede protection off, no log line. `cache_handler` already owns this shape — `supports_ttl_inspection`, `supports_buffer_read`, `supports_swr` are all capability TypeGuards, and `supports_swr`'s docstring already reasons about dynamic-attribute objects (it is class-level *on purpose*, to keep proxies off the freshness path). Locking wants the opposite: a false positive raises inside the `async with` and fails open with a warning, a false negative silently drops protection. So `supports_locking()` joins them, instance-level `hasattr`, and all three probes now call it — skew is impossible by construction, including the diagnostic. The base.py protocol shape fix from 4c67b88 stays: it is what lets the TypeGuard narrow, which is what keeps Kody's finding fixed (`timeout="oops"` is still a basedpyright error). Net deletion elsewhere: the TTL refresh site was hand-rolling `hasattr(get_ttl) and hasattr(refresh_ttl)` + `cast("TTLInspectableBackend")` — the same guard `supports_ttl_inspection()` has provided all along. Using it removes the last `cast` in the module. Test: `test_proxied_backend_still_takes_the_lock` drives a `__getattr__`-only proxy through the real decorator on a cache miss. Red-verified — with the `isinstance` probe restored it fails on 3.12 and passes on 3.11, which is precisely why the first round looked green locally. Docs, verified against the classes rather than copied forward: `RedisBackend` has no `acquire_lock` (`issubclass(RedisBackend, LockableBackend)` is False). Locking comes from `PerRequestRedisBackend`, what `RedisBackendProvider` and the env-resolved Redis path hand out; a `RedisBackend()` you construct and pass as `backend=` gets none. Four places claimed otherwise. The FAQ's "check with isinstance" advice is replaced with the `hasattr` the SDK itself uses, and says why. ruff + basedpyright clean. 3095 pass on 3.11, 3096 on 3.12 (non-slow, non-performance, excluding the credentialed SaaS e2e suite). --- .secrets.baseline | 4 +- docs/features/distributed-locking.md | 14 +++--- src/cachekit/backends/base.py | 12 ++--- src/cachekit/cache_handler.py | 23 ++++++++++ src/cachekit/decorators/wrapper.py | 20 ++++----- tests/unit/test_wrapper_lock_bare_key.py | 56 ++++++++++++++++++++++++ 6 files changed, 106 insertions(+), 23 deletions(-) diff --git a/.secrets.baseline b/.secrets.baseline index 82424789..790d27c1 100644 --- a/.secrets.baseline +++ b/.secrets.baseline @@ -222,7 +222,7 @@ "filename": "src/cachekit/cache_handler.py", "hashed_secret": "5baa61e4c9b93f3f0682250b6cf8331b7ee68fd8", "is_verified": false, - "line_number": 448 + "line_number": 471 } ], "src/cachekit/config/decorator.py": [ @@ -871,5 +871,5 @@ } ] }, - "generated_at": "2026-09-17T05:12:11Z" + "generated_at": "2026-09-21T10:14:09Z" } diff --git a/docs/features/distributed-locking.md b/docs/features/distributed-locking.md index 70052a57..451a7f71 100644 --- a/docs/features/distributed-locking.md +++ b/docs/features/distributed-locking.md @@ -41,7 +41,7 @@ report = await get_report("2025-01-15") > [!NOTE] > Locking requires **both** of: > -> 1. A backend implementing the `LockableBackend` protocol. `RedisBackend` and `CachekitIOBackend` (the SaaS backend behind `api.cachekit.io`) both do. `FileBackend` and pure-L1 (zero-config) caching don't — they silently skip lock acquisition; the function still works, just without stampede protection. +> 1. A backend implementing the `LockableBackend` protocol. `CachekitIOBackend` (the SaaS backend behind `api.cachekit.io`) does, and so does the Redis backend you get from env auto-detection or `RedisBackendProvider` (`PerRequestRedisBackend`). A `RedisBackend` you construct yourself and pass as `backend=` does **not** — it has no `acquire_lock`. Neither do `FileBackend` or pure-L1 (zero-config) caching. All of them silently skip lock acquisition; the function still works, just without stampede protection. > 2. An **async** decorated function. Sync wrappers never take the lock path on any backend — see [Async-only](#async-only-sync-functions-are-never-lock-protected). --- @@ -192,9 +192,11 @@ leaderboard = await get_leaderboard() ### With Redis Backend (Explicit) ```python notest from cachekit import cache -from cachekit.backends.redis import RedisBackend +from cachekit.backends.redis.provider import RedisBackendProvider, tenant_context -backend = RedisBackend() # Implements LockableBackend +# PerRequestRedisBackend implements LockableBackend; a bare RedisBackend() does not. +tenant_context.set("default") +backend = RedisBackendProvider(redis_url="redis://localhost:6379").get_backend() @cache(ttl=300, backend=backend) async def generate_stats(date): @@ -249,8 +251,8 @@ def acquire_lock( ``` Implementations are `async` generators wrapped in `@asynccontextmanager`, so the -protocol declares the *decorated* shape. That is what lets a caller narrow with -`isinstance(backend, LockableBackend)` and still type-check the `async with`. +protocol declares the *decorated* shape — a backend author must apply the +decorator for `async with` to work. The decorator wrapper calls it with `timeout=30.0` (lock self-expiry) and `blocking_timeout=5.0` (max wait to acquire) — see @@ -336,7 +338,7 @@ A: Your function takes longer than the 5 s `blocking_timeout`, so waiters fall t **Q: Locking doesn't seem to be working** A: Two things to check: 1. The decorated function must be **async** — sync wrappers never lock ([Async-only](#async-only-sync-functions-are-never-lock-protected)). -2. The backend must implement `LockableBackend` (`RedisBackend`, `CachekitIOBackend`). Check with `from cachekit.backends.base import LockableBackend; isinstance(backend, LockableBackend)`. +2. The backend must implement `LockableBackend` (`CachekitIOBackend`, or `PerRequestRedisBackend` from `RedisBackendProvider` — a bare `RedisBackend()` does not). Check it the way the SDK does: `hasattr(backend, "acquire_lock")`. Avoid `isinstance(backend, LockableBackend)` — from CPython 3.12 a `runtime_checkable` protocol check resolves members with `inspect.getattr_static`, so it reports `False` for a backend that delegates `acquire_lock` through `__getattr__`. **Q: How do I know if stampedes are happening?** A: Check Prometheus: a spike in `rate(redis_cache_operations_total{status="miss"}[1m])` = stampede risk. See [Prometheus Metrics](prometheus-metrics.md). diff --git a/src/cachekit/backends/base.py b/src/cachekit/backends/base.py index a158db18..82a3dd03 100644 --- a/src/cachekit/backends/base.py +++ b/src/cachekit/backends/base.py @@ -270,8 +270,11 @@ class LockableBackend(Protocol): features like cache stampede prevention and critical sections. Not all backends support this capability: - - Supported: RedisBackend, CachekitIOBackend (SaaS ``POST /v1/cache/{key}/lock``) - - Not supported: FileBackend, L1-only (in-memory) + - Supported: ``PerRequestRedisBackend`` — what ``RedisBackendProvider`` and + therefore the env-resolved Redis path hand out — and ``CachekitIOBackend`` + (SaaS ``POST /v1/cache/{key}/lock``). + - Not supported: ``RedisBackend`` constructed directly and passed as + ``backend=``, ``FileBackend``, L1-only (in-memory). Contract — bare cache key: ``acquire_lock`` receives the **bare cache key**, identical to what @@ -313,9 +316,8 @@ def acquire_lock( Returns: An async context manager yielding True if the lock was acquired, False if the wait timed out. Implementations are ``async`` - generators wrapped in ``@asynccontextmanager``; the protocol - declares the *decorated* shape so callers can narrow to - ``LockableBackend`` and still type-check ``async with``. + generators wrapped in ``@asynccontextmanager``, so this protocol + declares the *decorated* shape. Raises: BackendError: If backend operation fails diff --git a/src/cachekit/cache_handler.py b/src/cachekit/cache_handler.py index c5d34e21..ef80ac31 100644 --- a/src/cachekit/cache_handler.py +++ b/src/cachekit/cache_handler.py @@ -18,6 +18,7 @@ BufferHandle, BufferReadableBackend, BufferWritableBackend, + LockableBackend, TTLInspectableBackend, ) from cachekit.backends.provider import ( @@ -192,6 +193,28 @@ def supports_ttl_inspection(backend: BaseBackend) -> TypeGuard[TTLInspectableBac return hasattr(backend, "get_ttl") and hasattr(backend, "refresh_ttl") +def supports_locking(backend: object) -> TypeGuard[LockableBackend]: + """Type guard: backend provides distributed locking (stampede prevention). + + Takes ``object``, not ``BaseBackend`` like its siblings, because the + decorator probes the lazily-resolved ``_backend`` cell, which is ``None`` + until first call and may already be narrowed by another capability guard. + + Checked on the INSTANCE, deliberately — unlike ``supports_swr`` below, which + is class-level to keep ``__getattr__`` proxies and mocks off the freshness + read path. The asymmetry is the failure direction: a false positive here + raises inside the ``async with`` and fails open with a warning, while a + false negative silently drops stampede protection on a hot key. + + Equally deliberate: not ``isinstance(backend, LockableBackend)``. Since + CPython 3.12 a ``runtime_checkable`` Protocol check resolves members with + ``inspect.getattr_static``, which does not consult ``__getattr__`` — so a + delegating backend proxy locks on 3.10/3.11 and silently stops locking on + 3.12+. ``hasattr`` is stable across every supported interpreter. + """ + return hasattr(backend, "acquire_lock") + + # Backend type names already warned about, so refresh_ttl_on_get degradation warns at most # once per backend type per process (avoids per-hit log spam). Tests clear this set. _TTL_REFRESH_UNSUPPORTED_WARNED: set[str] = set() diff --git a/src/cachekit/decorators/wrapper.py b/src/cachekit/decorators/wrapper.py index 014c58f6..94bba752 100644 --- a/src/cachekit/decorators/wrapper.py +++ b/src/cachekit/decorators/wrapper.py @@ -10,11 +10,10 @@ import threading import time from collections.abc import Callable -from typing import TYPE_CHECKING, Any, NamedTuple, TypeVar, Union, cast +from typing import TYPE_CHECKING, Any, NamedTuple, TypeVar, Union from cachekit.hash_utils import redact_error_for_log -from ..backends.base import LockableBackend from ..backends.errors import BackendError, BackendErrorType from ..cache_handler import ( CacheInvalidator, @@ -25,7 +24,9 @@ get_logger, handle_decrypt_failure, redact_cache_key, + supports_locking, supports_swr, + supports_ttl_inspection, warn_ttl_refresh_unsupported, ) from ..interop import ( @@ -47,7 +48,7 @@ from .tenant_context import TenantContextExtractor if TYPE_CHECKING: - from ..backends.base import BaseBackend, TTLInspectableBackend + from ..backends.base import BaseBackend from ..serializers.base import SerializerProtocol @@ -923,7 +924,7 @@ async def _l2_swr_revalidate_async(cache_key: str, call_args: tuple[Any, ...], c the caller already got the stale value; the entry hard-expires at evict_at and the next request takes the ordinary synchronous miss path (spec degradation).""" try: - if isinstance(_backend, LockableBackend): + if supports_locking(_backend): async with _backend.acquire_lock(cache_key, timeout=_l2_swr_lease_seconds, blocking_timeout=None) as got_lease: if not got_lease: return # another client is revalidating — stale already served @@ -1820,13 +1821,12 @@ async def async_wrapper(*args: Any, **kwargs: Any) -> Any: _l1_backfill_from_l2(cache_key, cached_data, _l2_is_stale, _l2_fresh_for) # Handle TTL refresh if configured and threshold met - if refresh_ttl_on_get and ttl and hasattr(_backend, "get_ttl") and hasattr(_backend, "refresh_ttl"): - _ttl_backend = cast("TTLInspectableBackend", _backend) + if refresh_ttl_on_get and ttl and supports_ttl_inspection(_backend): try: - remaining_ttl = await _ttl_backend.get_ttl(cache_key) + remaining_ttl = await _backend.get_ttl(cache_key) if remaining_ttl and remaining_ttl < (ttl * ttl_refresh_threshold): # Refresh TTL in background with error callback - task = asyncio.create_task(_ttl_backend.refresh_ttl(cache_key, ttl)) + task = asyncio.create_task(_backend.refresh_ttl(cache_key, ttl)) task.add_done_callback(lambda t: _ttl_refresh_done_callback(t, cache_key)) except Exception as e: # TTL refresh is optional, don't fail on error @@ -1875,7 +1875,7 @@ async def async_wrapper(*args: Any, **kwargs: Any) -> Any: blocking_timeout = 5.0 # Wait up to 5 seconds to acquire lock # Check if backend supports distributed locking - if isinstance(_backend, LockableBackend): + if supports_locking(_backend): try: # Use backend's async lock protocol async with _backend.acquire_lock( @@ -2019,7 +2019,7 @@ async def async_wrapper(*args: Any, **kwargs: Any) -> Any: # Fall through to execute without locking # Execute without locking (either backend doesn't support it or lock failed) - if not hasattr(_backend, "acquire_lock"): + if not supports_locking(_backend): logger().debug( f"Backend doesn't support locking for {redact_cache_key(cache_key)}, executing without thundering herd protection" ) diff --git a/tests/unit/test_wrapper_lock_bare_key.py b/tests/unit/test_wrapper_lock_bare_key.py index 86edb6a3..66bc84d4 100644 --- a/tests/unit/test_wrapper_lock_bare_key.py +++ b/tests/unit/test_wrapper_lock_bare_key.py @@ -26,6 +26,7 @@ from __future__ import annotations import logging +import sys from collections.abc import AsyncIterator, Iterator from contextlib import asynccontextmanager from typing import Any, Optional @@ -359,3 +360,58 @@ async def my_func(x: int) -> dict[str, int]: assert lock_warnings, "lock failure must be logged" assert not any(raw_key in m for m in lock_warnings), f"raw cache key leaked into lock warning: {lock_warnings!r}" assert any(redact_cache_key(raw_key) in m for m in lock_warnings), "digest must keep the failure correlatable" + + +class _DelegatingBackendProxy: + """Backend wrapper that exposes every capability through ``__getattr__``. + + The shape an instrumentation / tenant-routing / retry wrapper takes in user + code, and the shape ``tests/backends/fixtures.py`` builds internally. It has + no ``acquire_lock`` attribute of its own — only the delegated one. + """ + + def __init__(self, wrapped: Any) -> None: + """Store the wrapped backend; every attribute access delegates to it.""" + self._wrapped = wrapped + + def __getattr__(self, name: str) -> Any: + """Delegate any attribute the proxy does not define to the wrapped backend.""" + return getattr(self._wrapped, name) + + +@pytest.mark.unit +@pytest.mark.asyncio +class TestLockCapabilityProbeSeesDelegatingProxies: + """The lock-capability probe must stay ``hasattr``-shaped across interpreters. + + ``isinstance(backend, LockableBackend)`` looks equivalent and is not: since + CPython 3.12, ``runtime_checkable`` protocol checks resolve members with + ``inspect.getattr_static``, which does not consult ``__getattr__``. Swapping + the probe for ``isinstance`` therefore keeps locking on 3.10/3.11 and + silently drops it on 3.12+ for any delegating backend — a stampede on a hot + key with no log line saying protection was lost. This test fails on 3.12+ + the moment that swap is made again. + """ + + async def test_proxied_backend_still_takes_the_lock(self) -> None: + """A ``__getattr__``-delegated ``acquire_lock`` must still be called.""" + inner = _RecordingLockableBackend() + proxy = _DelegatingBackendProxy(inner) + + # Guard the premise: the proxy is exactly the case the two probes disagree on. + from cachekit.backends.base import LockableBackend as _Proto + + assert hasattr(proxy, "acquire_lock") + assert not isinstance(proxy, _Proto) or sys.version_info < (3, 12) + + @cache(backend=proxy, ttl=300, l1_enabled=False) + async def my_func(x: int) -> int: + """Trivial cached coroutine used to drive one cache miss through the lock path.""" + return x * 2 + + assert await my_func(7) == 14 + assert len(inner.lock_keys) == 1, ( + f"delegating proxy lost stampede protection — acquire_lock was not called " + f"(python {sys.version_info.major}.{sys.version_info.minor}); " + f"the capability probe must be hasattr-shaped, not isinstance-shaped" + ) From d8faac2661f15110e1f792141471dcb0c2ba7b2a Mon Sep 17 00:00:00 2001 From: Ray Walker Date: Mon, 21 Sep 2026 21:13:43 +1000 Subject: [PATCH 5/6] =?UTF-8?q?fix:=20address=20coderabbit=20review=20?= =?UTF-8?q?=E2=80=94=20require=20a=20callable=20acquire=5Flock?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit supports_locking() probed with hasattr, which is True for a backend carrying acquire_lock = None. The wrapper then called None(...) and the TypeError escaped: the lock handler degrades to lockless execution only for a BackendError and re-raises everything else, so a backend that merely declared the attribute broke every decorated call it was meant to protect. callable(getattr(...)) still consults __getattr__, so the delegating proxy case the hasattr probe existed to preserve keeps locking on 3.12+. This also matches supports_swr() in the same module, which already guards with callable(getattr(...)). The docstring claimed a false positive "fails open with a warning". It does not — corrected to describe the actual re-raise. CodeRabbit-Resolved: cache_handler.py:215:Require a callable lock --- .secrets.baseline | 4 +- src/cachekit/cache_handler.py | 16 +++++--- tests/unit/test_wrapper_lock_bare_key.py | 52 ++++++++++++++++++++++++ 3 files changed, 65 insertions(+), 7 deletions(-) diff --git a/.secrets.baseline b/.secrets.baseline index 790d27c1..7e05a17a 100644 --- a/.secrets.baseline +++ b/.secrets.baseline @@ -222,7 +222,7 @@ "filename": "src/cachekit/cache_handler.py", "hashed_secret": "5baa61e4c9b93f3f0682250b6cf8331b7ee68fd8", "is_verified": false, - "line_number": 471 + "line_number": 477 } ], "src/cachekit/config/decorator.py": [ @@ -871,5 +871,5 @@ } ] }, - "generated_at": "2026-09-21T10:14:09Z" + "generated_at": "2026-09-21T11:13:27Z" } diff --git a/src/cachekit/cache_handler.py b/src/cachekit/cache_handler.py index ef80ac31..6259d0d4 100644 --- a/src/cachekit/cache_handler.py +++ b/src/cachekit/cache_handler.py @@ -202,17 +202,23 @@ def supports_locking(backend: object) -> TypeGuard[LockableBackend]: Checked on the INSTANCE, deliberately — unlike ``supports_swr`` below, which is class-level to keep ``__getattr__`` proxies and mocks off the freshness - read path. The asymmetry is the failure direction: a false positive here - raises inside the ``async with`` and fails open with a warning, while a - false negative silently drops stampede protection on a hot key. + read path. The asymmetry is the failure direction: a false negative silently + drops stampede protection on a hot key, while a false positive raises out of + the ``async with`` — and does NOT fail open. The wrapper's handler degrades + to lockless execution only for a ``BackendError``; a ``TypeError`` from + calling a non-callable propagates to the caller and breaks the decorated + function. Hence ``callable``, not ``hasattr``: a backend carrying + ``acquire_lock = None`` is not lockable, and must take the lockless path + rather than crash the call it was meant to protect. Equally deliberate: not ``isinstance(backend, LockableBackend)``. Since CPython 3.12 a ``runtime_checkable`` Protocol check resolves members with ``inspect.getattr_static``, which does not consult ``__getattr__`` — so a delegating backend proxy locks on 3.10/3.11 and silently stops locking on - 3.12+. ``hasattr`` is stable across every supported interpreter. + 3.12+. Plain ``getattr`` consults ``__getattr__`` and is stable across + every supported interpreter. """ - return hasattr(backend, "acquire_lock") + return callable(getattr(backend, "acquire_lock", None)) # Backend type names already warned about, so refresh_ttl_on_get degradation warns at most diff --git a/tests/unit/test_wrapper_lock_bare_key.py b/tests/unit/test_wrapper_lock_bare_key.py index 66bc84d4..5d2b0abd 100644 --- a/tests/unit/test_wrapper_lock_bare_key.py +++ b/tests/unit/test_wrapper_lock_bare_key.py @@ -415,3 +415,55 @@ async def my_func(x: int) -> int: f"(python {sys.version_info.major}.{sys.version_info.minor}); " f"the capability probe must be hasattr-shaped, not isinstance-shaped" ) + + +class _NonCallableLockBackend(_RecordingLockableBackend): + """Backend that HAS an ``acquire_lock`` attribute which is not callable. + + The shape a feature-flagged or partially-initialised backend takes when it + declares the capability slot and leaves it unset. ``hasattr`` cannot tell it + apart from a genuinely lockable backend — ``callable`` can. + """ + + acquire_lock = None # type: ignore[assignment] + + +@pytest.mark.unit +@pytest.mark.asyncio +class TestNonCallableLockAttributeFallsBackToLockless: + """A non-callable ``acquire_lock`` must degrade to lockless, never crash the call. + + Under the previous ``hasattr`` probe this backend passed the capability + check, the wrapper then called ``None(...)``, and the resulting + ``TypeError`` escaped: the wrapper's handler degrades to lockless execution + only for a ``BackendError`` and re-raises everything else. So a backend that + merely declared the attribute broke every decorated call it was supposed to + protect (CodeRabbit PR #313). + """ + + async def test_non_callable_acquire_lock_does_not_break_the_call(self) -> None: + """The decorated function must still return, having taken the lockless path.""" + backend = _NonCallableLockBackend() + + # Guard the premise: hasattr cannot distinguish this from a lockable backend. + assert hasattr(backend, "acquire_lock") + assert not callable(getattr(backend, "acquire_lock", None)) + + @cache(backend=backend, ttl=300, l1_enabled=False) + async def my_func(x: int) -> int: + """Trivial cached coroutine used to drive one cache miss.""" + return x * 3 + + # Previously raised TypeError: 'NoneType' object is not callable. + assert await my_func(5) == 15 + + # And it genuinely took the lockless branch rather than a swallowed lock. + assert backend.lock_keys == [], "no lock should have been taken on a non-lockable backend" + + async def test_capability_probe_rejects_non_callable_attribute(self) -> None: + """``supports_locking`` is the single place the callable requirement lives.""" + from cachekit.cache_handler import supports_locking + + assert supports_locking(_RecordingLockableBackend()) is True + assert supports_locking(_NonCallableLockBackend()) is False + assert supports_locking(object()) is False From fa21774bf2f3ae4e174f6696ecf99b5a8750f94e Mon Sep 17 00:00:00 2001 From: Ray Walker Date: Mon, 21 Sep 2026 21:21:49 +1000 Subject: [PATCH 6/6] test(lock): type the delegating proxy's wrapped backend _DelegatingBackendProxy.__init__ took `Any`, so a drift in the LockableBackend protocol would go undetected at the one call site that exercises __getattr__ delegation. Annotating it also pins the premise the test relies on: the wrapped double really does satisfy the protocol. Kody-Resolved: test_wrapper_lock_bare_key.py:375:Type annotation defect --- tests/unit/test_wrapper_lock_bare_key.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/unit/test_wrapper_lock_bare_key.py b/tests/unit/test_wrapper_lock_bare_key.py index 5d2b0abd..fbf1edf2 100644 --- a/tests/unit/test_wrapper_lock_bare_key.py +++ b/tests/unit/test_wrapper_lock_bare_key.py @@ -35,6 +35,7 @@ import pytest from cachekit import cache +from cachekit.backends.base import LockableBackend from cachekit.backends.errors import BackendError, BackendErrorType from cachekit.hash_utils import redact_cache_key @@ -370,7 +371,7 @@ class _DelegatingBackendProxy: no ``acquire_lock`` attribute of its own — only the delegated one. """ - def __init__(self, wrapped: Any) -> None: + def __init__(self, wrapped: LockableBackend) -> None: """Store the wrapped backend; every attribute access delegates to it.""" self._wrapped = wrapped