test: pin counter drift, restructure at-cap bench, trim docstring

bdraco · bdraco · commit a2a2d34c62ac · 2026-05-17T21:09:47.000-07:00
Three review follow-ups on #1718: **Bench actually measures eviction (Copilot inline at L43).** ``test_cache_add_at_cap_evicts`` previously created an empty cache and added ``_MAX_CACHE_RECORDS + 1000`` records, so ~91% of the measured time was the below-cap fill path and only the final 1000 inserts evicted — the CodSpeed number was not representative of the eviction performance the name implies. Restructure to pre-fill the cache to the cap *outside* the timed body and consume one fresh unique record per benchmark iteration. Each iteration measures exactly one ``_async_add`` + ``_async_evict_oldest`` cycle, with the cache permanently at the cap. ``_make_records`` gains a ``prefix=`` parameter so the fill records and the overflow pool don't collide on ``name``. **Defensive test pins counter state (Kōan suggestion ＃3).** ``test_cache_eviction_empty_heap_returns_without_evicting`` already asserts the new record lands; add the corresponding ``_total_records == _MAX_CACHE_RECORDS + 1`` assertion so a future change to the broken-state recovery (refuse the add, clamp the counter) fails this test explicitly rather than silently shifting behaviour. **One-line docstring (Kōan suggestion ＃2).** ``_maybe_rebuild_heap`` had an 11-line docstring restating the body and including amortized-cost analysis. Per CLAUDE.md the why lives at the call site comment in ``_async_add`` (already does); the function's contract is a single line. **No log on the empty-heap fall-through (Kōan suggestion ＃1).** Kōan suggested an assertion or debug log; the assertion would crash production on invariant drift, and even a guarded ``log.debug(...)`` adds Python attribute-lookup overhead in the function the cap overflow path runs through. Drift is already caught loudly by ``test_cache_total_records_invariant_under_mixed_ops``. Comment explains the deliberate silence.
diff --git a/src/zeroconf/_cache.py b/src/zeroconf/_cache.py
@@ -135,21 +135,12 @@ def _async_evict_oldest(self) -> None:
                 continue
             self._async_remove(record)
             return
+        # Heap-empty fall-through is unreachable by the cache invariant
+        # (every counted record has a heap entry); accounting drift would
+        # surface in tests via test_cache_total_records_invariant_under_mixed_ops.
 
     def _maybe_rebuild_heap(self) -> None:
-        """Rebuild ``_expire_heap`` if stale entries dominate.
-
-        Re-adds of an existing record with a new TTL append a fresh
-        ``(when, record)`` and leave the prior tuple behind as stale;
-        eviction's stale-skip loop and ``async_expire`` already absorb
-        these, but unchecked accumulation lets a peer that just replays
-        cached records grow the heap arbitrarily between cleanups.
-        Same threshold as the long-standing rebuild in ``async_expire``:
-        only fire when stale entries outweigh live ones (heap > 2 x
-        expirations), and only above a minimum floor so a small cache
-        isn't rebuilt for nothing. Amortized cost is O(1) per push;
-        the O(N) rebuild fires at most once per N stale pushes.
-        """
+        """Rebuild ``_expire_heap`` when stale entries dominate live ones."""
         expire_heap_len = len(self._expire_heap)
         if (
             expire_heap_len > _MIN_SCHEDULED_RECORD_EXPIRATION
diff --git a/tests/benchmarks/test_cache_bound.py b/tests/benchmarks/test_cache_bound.py
@@ -8,14 +8,14 @@
 from zeroconf.const import _CLASS_IN, _MAX_CACHE_RECORDS, _TYPE_A
 
 
-def _make_records(count: int, now: float) -> list[DNSAddress]:
+def _make_records(count: int, now: float, prefix: str = "bench") -> list[DNSAddress]:
     return [
         DNSAddress(
-            f"bench-{i}.local.",
+            f"{prefix}-{i}.local.",
             _TYPE_A,
             _CLASS_IN,
             120,
-            bytes((i & 0xFF, (i >> 8) & 0xFF, 0, 1)),
+            bytes(((i >> 24) & 0xFF, (i >> 16) & 0xFF, (i >> 8) & 0xFF, i & 0xFF)),
             created=now + i,
         )
         for i in range(count)
@@ -34,11 +34,20 @@ def _add() -> None:
 
 
 def test_cache_add_at_cap_evicts(benchmark: BenchmarkFixture) -> None:
-    """Steady-state add at the cap: every new record forces one eviction."""
+    """Steady-state add at the cap: every measured insert forces one eviction.
+
+    Pre-fills the cache to ``_MAX_CACHE_RECORDS`` outside the timed body so
+    only the eviction-path adds are measured. Each benchmark iteration
+    consumes one fresh unique record from a pre-built pool, keeping the
+    cache permanently at the cap and the work per iteration to a single
+    ``_async_add`` + ``_async_evict_oldest`` cycle.
+    """
     now = current_time_millis()
-    overflow_records = _make_records(_MAX_CACHE_RECORDS + 1000, now)
+    cache = DNSCache()
+    cache.async_add_records(_make_records(_MAX_CACHE_RECORDS, now, prefix="fill"))
+    # Large pool so the iterator outlives any reasonable codspeed run count.
+    pool = iter(_make_records(100_000, now + _MAX_CACHE_RECORDS, prefix="evict"))
 
     @benchmark
-    def _flood() -> None:
-        cache = DNSCache()
-        cache.async_add_records(overflow_records)
+    def _evict_one() -> None:
+        cache.async_add_records([next(pool)])
diff --git a/tests/test_cache.py b/tests/test_cache.py
@@ -525,11 +525,15 @@ def test_cache_eviction_empty_heap_returns_without_evicting() -> None:
     # By the cache invariant every record in ``_total_records`` has a heap
     # entry, so eviction should never see an empty heap. Force the broken
     # state directly to pin the defensive behaviour: ``_async_evict_oldest``
-    # returns without raising and the subsequent insert still lands.
+    # returns without raising and the subsequent insert still lands. Since
+    # eviction can't free space, the counter is allowed to drift past the
+    # cap by exactly one — pinned so a future change to the recovery
+    # semantics (e.g., refusing the add or clamping) fails this test.
     cache._total_records = const._MAX_CACHE_RECORDS
     cache._expire_heap = []
     cache.async_add_records([_addr("post-empty.local.", 0)])
     assert "post-empty.local." in cache.cache
+    assert cache._total_records == const._MAX_CACHE_RECORDS + 1
 
 
 def test_cache_eviction_skips_stale_heap_entries() -> None:
