fix: drain _seen_logs in a while loop so drift can't inflate the cap

bdraco · bdraco · commit 0e25328e31d6 · 2026-05-17T20:00:32.000-07:00
Under free-threaded (3.14t) contention or sustained multi-instance
sync use, two callers can both pass the ``len(seen) &lt; _MAX_SEEN_LOGS``
check and both insert before either evicts — leaving the dict
transiently above the cap. The previous ``if``-based eviction
removed one entry on the next call, but the new insert added one
back, so the overshoot never drained and the dict silently grew
with thread count.

Switch the eviction to ``while len(seen) &gt;= _MAX_SEEN_LOGS:`` so a
single non-racing caller drains the accumulated drift back to the
cap. Bail on ``RuntimeError`` and ``StopIteration`` so a concurrent
mutation can't make the loop spin.

Add ``test_mark_seen_drains_drift_above_cap`` to pin the drain
behaviour — a regression to single-eviction would leave the dict
at ``cap + drift - 1`` and fail the assertion.
diff --git a/src/zeroconf/_logger.py b/src/zeroconf/_logger.py
@@ -48,7 +48,7 @@ def _mark_seen(seen: dict[str, None], key: str) -> bool:
 
     Bounds the dict so callers passing attacker-influenced keys (peer
     addresses, packet offsets) cannot grow it without bound. Evicts
-    the oldest entry per overflow (dict preserves insertion order on
+    the oldest entries on overflow (dict preserves insertion order on
     Python 3.7+), so ``_MAX_SEEN_LOGS`` is a recency window.
 
     The dict is shared across all ``Zeroconf`` instances in the
@@ -57,20 +57,20 @@ def _mark_seen(seen: dict[str, None], key: str) -> bool:
     (``in``, ``__setitem__``, ``pop``, ``len``) are atomic in CPython
     3.13+ FT and don't crash, but the compound ``iter`` → ``next``
     used to find the FIFO victim can raise ``RuntimeError`` if
-    another thread mutates the dict between the two ops. Catch and
-    skip — the other thread is already shrinking the set, so missing
-    one eviction here just lets the cap drift up by one entry per
-    racing thread until contention clears.
+    another thread mutates the dict between the two ops. The
+    eviction loop drains until ``len(seen) < _MAX_SEEN_LOGS`` so that
+    any drift accumulated by prior concurrent inserts is corrected by
+    the next caller, and bails on ``RuntimeError`` (another thread is
+    already shrinking the set) so we don't spin.
     """
     if key in seen:
         return False
-    if len(seen) >= _MAX_SEEN_LOGS:
+    while len(seen) >= _MAX_SEEN_LOGS:
         try:
-            oldest = next(iter(seen), None)
-        except RuntimeError:
-            oldest = None
-        if oldest is not None:
-            seen.pop(oldest, None)
+            oldest = next(iter(seen))
+        except (RuntimeError, StopIteration):
+            break
+        seen.pop(oldest, None)
     seen[key] = None
     return True
 
diff --git a/tests/test_logger.py b/tests/test_logger.py
@@ -105,6 +105,27 @@ def __iter__(self):  # type: ignore[override]
     assert "new-key" in seen
 
 
+def test_mark_seen_drains_drift_above_cap() -> None:
+    """``_mark_seen`` drains a drifted-over-cap dict back to the cap.
+
+    Concurrent inserts on the free-threaded build can leave the dict
+    transiently above ``_MAX_SEEN_LOGS`` (e.g. two threads both passed
+    the ``len < cap`` check and both inserted). The next non-racing
+    call must drain the accumulated overshoot, not just evict one
+    entry — otherwise the cap silently inflates with thread count.
+    """
+    seen: dict[str, None] = {}
+    drift = 10
+    for i in range(_MAX_SEEN_LOGS + drift):
+        seen[f"k-{i}"] = None
+    assert len(seen) == _MAX_SEEN_LOGS + drift
+    assert _mark_seen(seen, "new-key") is True
+    assert len(seen) == _MAX_SEEN_LOGS
+    assert "new-key" in seen
+    for i in range(drift + 1):
+        assert f"k-{i}" not in seen
+
+
 def test_seen_logs_is_bounded() -> None:
     """``_seen_logs`` stays at the cap and evicts oldest-first (FIFO)."""
     _logger._seen_logs.clear()
