From 718ef5d577fe3a027ff3800b6cc9c43844ffeaf1 Mon Sep 17 00:00:00 2001 From: Mark S Date: Thu, 17 Sep 2026 10:40:10 +1000 Subject: [PATCH 1/5] fix(decorators): async get hits record serializer/size/hit like the sync path (LAB-3765) The async wrapper's L1-hit and L2-hit record_cache_operation calls omitted serializer, size_bytes and hit, so FeatureOrchestrator's serializer="unknown" default swallowed every async get hit. The sync path labels the same tiers l1_memory / rust with the served size and hit=True. Pass the same kwargs at both async sites; size_bytes for the L2 hit is the raw envelope the async handler already returns, so L1 and L2 hits on one entry report the same size. Test drives both tiers through the real decorator stack and captures at the orchestrator, not the collector, because record_success() also forwards an unlabelled record at these sites. --- src/cachekit/decorators/wrapper.py | 8 ++ tests/unit/test_async_get_record_labels.py | 90 ++++++++++++++++++++++ 2 files changed, 98 insertions(+) create mode 100644 tests/unit/test_async_get_record_labels.py diff --git a/src/cachekit/decorators/wrapper.py b/src/cachekit/decorators/wrapper.py index 08b2209b..2b0332d9 100644 --- a/src/cachekit/decorators/wrapper.py +++ b/src/cachekit/decorators/wrapper.py @@ -1615,8 +1615,11 @@ async def async_wrapper(*args: Any, **kwargs: Any) -> Any: features.record_cache_operation( operation="get", namespace=namespace or "default", + serializer="l1_memory", success=True, duration_ms=0.001, # Sub-microsecond + size_bytes=len(l1_bytes), + hit=True, ) # Record L1 hit for cache_info() @@ -1710,11 +1713,16 @@ async def async_wrapper(*args: Any, **kwargs: Any) -> Any: features.record_success() if features.collect_stats: + # size_bytes is the raw L2 envelope actually served (and backfilled + # into L1 below), so L1 and L2 hits on one entry report the same size. features.record_cache_operation( operation="get", namespace=namespace or "default", + serializer="rust", success=True, duration_ms=get_duration_ms, + size_bytes=len(cached_data) if cached_data else 0, + hit=True, ) # Update L1 cache with the L2 value (serialized bytes) for subsequent diff --git a/tests/unit/test_async_get_record_labels.py b/tests/unit/test_async_get_record_labels.py new file mode 100644 index 00000000..9905ed3f --- /dev/null +++ b/tests/unit/test_async_get_record_labels.py @@ -0,0 +1,90 @@ +"""Async hit-record stats parity (LAB-3765). + +The sync wrapper records an L1 hit as ``operation="get", serializer="l1_memory", +hit=True`` and an L2 hit as ``serializer="rust", hit=True``, both with the served +payload size. The async wrapper's two hit sites recorded ``get`` with none of +those, so on a hit-heavy async workload most ``get`` traffic filed under +``serializer="unknown"``. Both async tiers are pinned here through the real +decorator stack: a miss primes L2 (and L1 via backfill); clearing L1 before the +second call forces it past L1 to the L2 site. +""" + +from __future__ import annotations + +from collections.abc import Iterator +from typing import Any + +import pytest + +from cachekit import cache +from cachekit.decorators.orchestrator import FeatureOrchestrator +from cachekit.l1_cache import get_l1_cache_manager + + +class _ByteStore: + """Plain in-memory byte store standing in for L2.""" + + def __init__(self) -> None: + self.store: dict[str, bytes] = {} + + def get(self, key: str) -> bytes | None: + return self.store.get(key) + + def set(self, key: str, value: bytes, ttl: int | None = None) -> None: + self.store[key] = value + + def delete(self, key: str) -> bool: + return self.store.pop(key, None) is not None + + def exists(self, key: str) -> bool: + return key in self.store + + def health_check(self) -> tuple[bool, dict[str, Any]]: + return True, {} + + +@pytest.fixture(autouse=True) +def setup_di_for_redis_isolation() -> Iterator[None]: + """Override the root conftest's Redis isolation: the backend is injected, no Redis needed. + + Keep L1 clear between cases — both share a cache key, and a leaked L1 entry would + turn the L2 case's priming miss into an L1 hit. + """ + get_l1_cache_manager().clear_all() + yield + get_l1_cache_manager().clear_all() + + +@pytest.fixture +def recorded(monkeypatch: pytest.MonkeyPatch) -> list[dict[str, Any]]: + """Capture the wrapper's explicit features.record_cache_operation(...) calls, keyword args as passed. + + Patched at the orchestrator, not the collector: record_success() also forwards an + operation-context record to the collector, which would shadow the labels under test. + """ + calls: list[dict[str, Any]] = [] + monkeypatch.setattr(FeatureOrchestrator, "record_cache_operation", lambda self, **kw: calls.append(kw)) + return calls + + +@pytest.mark.unit +@pytest.mark.parametrize("tier", ["l1_memory", "rust"], ids=["l1-hit", "l2-hit"]) +async def test_async_get_hit_records_sync_labels(recorded: list[dict[str, Any]], tier: str) -> None: + backend = _ByteStore() + + @cache(backend=backend, ttl=60, namespace="async-get-labels") + async def compute() -> dict[str, int]: + return {"answer": 42} + + assert await compute() == {"answer": 42} # miss: primes L2, and L1 via the miss-store + assert backend.store + if tier == "rust": + get_l1_cache_manager().clear_all() # force the hit past L1 to the L2 site + recorded.clear() + + assert await compute() == {"answer": 42} + + gets = [c for c in recorded if c["operation"] == "get"] + assert len(gets) == 1 + assert (gets[0].get("serializer"), gets[0].get("hit")) == (tier, True) # what the sync hit sites pass + assert gets[0].get("size_bytes", 0) > 0 From 407c88d4d5bcfc9eaefa0da6cf14b1ae1b190732 Mon Sep 17 00:00:00 2001 From: Mark S Date: Thu, 17 Sep 2026 10:53:58 +1000 Subject: [PATCH 2/5] fix(decorators): drop unreachable size guard, name the sync L2 divergence, tighten test docstrings (LAB-3765) Panel trims: the L2 hit tuple is only built after deserialize_data succeeded on cached_data, so the else-0 branch could not run; the comment now says why this site sizes the envelope where the sync L2 site sizes str(value); the test docstring no longer calls the miss-store a backfill and bounds its claim to the L1 and uncontended L2 sites; the fixture docstring says what the override actually buys (the L1 clear the unit conftest dropped). --- src/cachekit/decorators/wrapper.py | 6 +++--- tests/unit/test_async_get_record_labels.py | 21 ++++++++------------- 2 files changed, 11 insertions(+), 16 deletions(-) diff --git a/src/cachekit/decorators/wrapper.py b/src/cachekit/decorators/wrapper.py index 2b0332d9..8c3ee483 100644 --- a/src/cachekit/decorators/wrapper.py +++ b/src/cachekit/decorators/wrapper.py @@ -1713,15 +1713,15 @@ async def async_wrapper(*args: Any, **kwargs: Any) -> Any: features.record_success() if features.collect_stats: - # size_bytes is the raw L2 envelope actually served (and backfilled - # into L1 below), so L1 and L2 hits on one entry report the same size. + # size_bytes: raw envelope, matching the L1 site's len(l1_bytes); the sync L2 + # site has no envelope in hand and estimates from str(value) instead. features.record_cache_operation( operation="get", namespace=namespace or "default", serializer="rust", success=True, duration_ms=get_duration_ms, - size_bytes=len(cached_data) if cached_data else 0, + size_bytes=len(cached_data), hit=True, ) diff --git a/tests/unit/test_async_get_record_labels.py b/tests/unit/test_async_get_record_labels.py index 9905ed3f..cbf69e23 100644 --- a/tests/unit/test_async_get_record_labels.py +++ b/tests/unit/test_async_get_record_labels.py @@ -1,12 +1,10 @@ """Async hit-record stats parity (LAB-3765). -The sync wrapper records an L1 hit as ``operation="get", serializer="l1_memory", -hit=True`` and an L2 hit as ``serializer="rust", hit=True``, both with the served -payload size. The async wrapper's two hit sites recorded ``get`` with none of -those, so on a hit-heavy async workload most ``get`` traffic filed under -``serializer="unknown"``. Both async tiers are pinned here through the real -decorator stack: a miss primes L2 (and L1 via backfill); clearing L1 before the -second call forces it past L1 to the L2 site. +The sync wrapper records an L1 hit as ``serializer="l1_memory", hit=True`` and an L2 +hit as ``serializer="rust", hit=True``, both with the served size; the async L1 and +uncontended L2 hit sites recorded none of those, so async hits filed under +``serializer="unknown"``. A miss primes L2 (and L1 via the miss-store); clearing L1 +before the second call forces it past L1 to the L2 site. """ from __future__ import annotations @@ -45,12 +43,9 @@ def health_check(self) -> tuple[bool, dict[str, Any]]: @pytest.fixture(autouse=True) def setup_di_for_redis_isolation() -> Iterator[None]: - """Override the root conftest's Redis isolation: the backend is injected, no Redis needed. - - Keep L1 clear between cases — both share a cache key, and a leaked L1 entry would - turn the L2 case's priming miss into an L1 hit. - """ - get_l1_cache_manager().clear_all() + """L1 hygiene between the two cases: tests/unit/conftest.py's no-op override of the root + fixture dropped its clear_all(), and a leaked entry would turn the L2 case's priming + miss into an L1 hit.""" yield get_l1_cache_manager().clear_all() From c420bcc32434de0d13a5fd822d1f803491e60141 Mon Sep 17 00:00:00 2001 From: Mark S Date: Thu, 17 Sep 2026 11:58:38 +1000 Subject: [PATCH 3/5] docs(metrics): drop the sync-site aside from the async L2 hit comment (LAB-3765) The sync L2 site measures the served envelope as of #298; this comment described the state it was written against and would read as a lie once both land. The remaining sentence is about this site only. --- src/cachekit/decorators/wrapper.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/cachekit/decorators/wrapper.py b/src/cachekit/decorators/wrapper.py index 8c3ee483..75bc2708 100644 --- a/src/cachekit/decorators/wrapper.py +++ b/src/cachekit/decorators/wrapper.py @@ -1713,8 +1713,7 @@ async def async_wrapper(*args: Any, **kwargs: Any) -> Any: features.record_success() if features.collect_stats: - # size_bytes: raw envelope, matching the L1 site's len(l1_bytes); the sync L2 - # site has no envelope in hand and estimates from str(value) instead. + # size_bytes: raw envelope, matching the L1 site's len(l1_bytes). features.record_cache_operation( operation="get", namespace=namespace or "default", From f50f5093b1332e2e5c7b35a8c1939dbe83631f1a Mon Sep 17 00:00:00 2001 From: Mark S Date: Thu, 17 Sep 2026 12:12:19 +1000 Subject: [PATCH 4/5] test(decorators): assert exact served size in async hit-record test (LAB-3765) The positive-only size_bytes assertion would pass on a wrong constant (e.g. size_bytes=1). Pin it to the exact served envelope length: both hit sites record the raw serialized envelope, and the L1 backfill stores the same bytes L2 returned, so the single L2 store value is ground truth for either tier. Addresses the CodeRabbit review nitpick. --- tests/unit/test_async_get_record_labels.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/unit/test_async_get_record_labels.py b/tests/unit/test_async_get_record_labels.py index cbf69e23..e8128dea 100644 --- a/tests/unit/test_async_get_record_labels.py +++ b/tests/unit/test_async_get_record_labels.py @@ -82,4 +82,9 @@ async def compute() -> dict[str, int]: gets = [c for c in recorded if c["operation"] == "get"] assert len(gets) == 1 assert (gets[0].get("serializer"), gets[0].get("hit")) == (tier, True) # what the sync hit sites pass - assert gets[0].get("size_bytes", 0) > 0 + # Exact served size, not just positive: both hit sites record the raw serialized + # envelope's length — the L1 backfill stores the same bytes L2 returned, so the + # single L2 store value is the ground truth for either tier. A `> 0` assertion + # would pass on a wrong constant (e.g. size_bytes=1); this pins the real value. + expected_size = len(next(iter(backend.store.values()))) + assert gets[0].get("size_bytes") == expected_size From 386faa80d497e955833fd913f214d178b218697f Mon Sep 17 00:00:00 2001 From: Ray Walker Date: Fri, 18 Sep 2026 04:09:53 +1000 Subject: [PATCH 5/5] =?UTF-8?q?fix:=20address=20coderabbit=20review=20?= =?UTF-8?q?=E2=80=94=20measure=20async=20L2=20str=20envelopes=20in=20UTF-8?= =?UTF-8?q?=20bytes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The async L2 hit-record metric measured size_bytes with len(cached_data), which counts Unicode characters when the envelope is a str, under-reporting non-ASCII payloads. _l1_backfill_from_l2 already UTF-8-encodes str envelopes before storing, and the L1 site's len(l1_bytes) measures bytes; measure the same encoded envelope here so all three agree. CodeRabbit-Resolved: wrapper.py:1716:Measure string envelopes in byt --- src/cachekit/decorators/wrapper.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/cachekit/decorators/wrapper.py b/src/cachekit/decorators/wrapper.py index 75bc2708..1fa39f92 100644 --- a/src/cachekit/decorators/wrapper.py +++ b/src/cachekit/decorators/wrapper.py @@ -1713,14 +1713,17 @@ async def async_wrapper(*args: Any, **kwargs: Any) -> Any: features.record_success() if features.collect_stats: - # size_bytes: raw envelope, matching the L1 site's len(l1_bytes). + # size_bytes: the encoded envelope, matching the bytes _l1_backfill_from_l2 + # stores and the L1 site's len(l1_bytes). A str envelope is UTF-8 encoded + # first, so non-ASCII payloads report byte length, not character count. + _l2_envelope = cached_data.encode("utf-8") if isinstance(cached_data, str) else cached_data features.record_cache_operation( operation="get", namespace=namespace or "default", serializer="rust", success=True, duration_ms=get_duration_ms, - size_bytes=len(cached_data), + size_bytes=len(_l2_envelope), hit=True, )