fix: make _mark_seen safe under free-threaded contention

bdraco · bdraco · commit ebe1ab21862e · 2026-05-17T19:50:41.000-07:00
Address review feedback on #1717. The previous eviction body — ``del seen[next(iter(seen))]`` — relied on the GIL to serialize the compound iter/next/del. Under the free-threaded build (3.14t) and under multi-instance sync use where multiple ``Zeroconf`` instances share the module-level ``_seen_logs``, callers can race: - Two threads pop the same victim — ``del`` raises ``KeyError`` on the loser - One thread mutates the dict between another's ``iter()`` and ``next()`` — the iterator's mutation-count check raises ``RuntimeError`` ("dictionary changed size during iteration") Switch to ``seen.pop(next(iter(seen), None), None)`` so the pop is idempotent (no ``KeyError``) and the iter start handles the empty- dict edge case (no ``StopIteration``), and wrap the iter/next in a ``try/except RuntimeError`` so concurrent mutation during eviction is absorbed. Worst case under contention is a transient overshoot of the cap by one entry per racing thread, which clears as soon as the contention does. Add ``test_mark_seen_absorbs_runtime_error_during_eviction`` which substitutes a dict subclass whose ``__iter__`` always raises, proving the new ``except`` branch lets the insert still complete. Also tighten ``tests/test_protocol.py::test_seen_logs_is_bounded`` per Kōan's review comment: previously asserted ``<= _MAX_SEEN_LOGS``, which would still pass if a future refactor collapsed the per-port keys to a single dedup string. Now asserts ``== _MAX_SEEN_LOGS`` and verifies port 0's exception text is gone while the highest port's is still present — pins both the bound and that the parser exception path actually enters the dict with per-source-unique keys.
diff --git a/src/zeroconf/_logger.py b/src/zeroconf/_logger.py
@@ -50,11 +50,27 @@ def _mark_seen(seen: dict[str, None], key: str) -> bool:
     addresses, packet offsets) cannot grow it without bound. Evicts
     the oldest entry per 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
+    process; on the free-threaded build (3.14t) and under multi-
+    instance sync use, callers can race. Individual dict operations
+    (``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.
     """
     if key in seen:
         return False
     if len(seen) >= _MAX_SEEN_LOGS:
-        del seen[next(iter(seen))]
+        try:
+            oldest = next(iter(seen), None)
+        except RuntimeError:
+            oldest = None
+        if oldest is not None:
+            seen.pop(oldest, None)
     seen[key] = None
     return True
 
diff --git a/tests/test_logger.py b/tests/test_logger.py
@@ -6,7 +6,7 @@
 from unittest.mock import call, patch
 
 from zeroconf import _logger
-from zeroconf._logger import _MAX_SEEN_LOGS, QuietLogger, set_logger_level_if_unset
+from zeroconf._logger import _MAX_SEEN_LOGS, QuietLogger, _mark_seen, set_logger_level_if_unset
 
 
 def test_loading_logger():
@@ -85,6 +85,26 @@ def test_llog_exception_debug():
     assert mock_log_debug.mock_calls == [call("the exception", exc_info=False)]
 
 
+def test_mark_seen_absorbs_runtime_error_during_eviction() -> None:
+    """Concurrent mutation can make ``iter(seen)`` raise ``RuntimeError``.
+
+    Free-threaded (3.14t) and multi-instance sync callers share
+    ``_seen_logs``; if another thread mutates it between ``iter()``
+    and ``next()`` the iterator raises ``RuntimeError``.
+    ``_mark_seen`` must absorb that and still insert the new key.
+    """
+
+    class RacyDict(dict[str, None]):
+        def __iter__(self):  # type: ignore[override]
+            raise RuntimeError("dictionary changed size during iteration")
+
+    seen: dict[str, None] = RacyDict()
+    for i in range(_MAX_SEEN_LOGS):
+        seen[f"k-{i}"] = None
+    assert _mark_seen(seen, "new-key") is True
+    assert "new-key" in seen
+
+
 def test_seen_logs_is_bounded() -> None:
     """``_seen_logs`` stays at the cap and evicts oldest-first (FIFO)."""
     _logger._seen_logs.clear()
diff --git a/tests/test_protocol.py b/tests/test_protocol.py
@@ -965,16 +965,25 @@ def test_dns_compression_generic_failure(caplog):
 
 
 def test_seen_logs_is_bounded():
-    """Corrupt packets from varying peers must not grow _seen_logs without bound."""
+    """Corrupt packets from varying peers fill ``_seen_logs`` exactly to the cap."""
     packet = (
         b"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x06domain\x05local\x00\x00\x01"
         b"\x80\x01\x00\x00\x00\x01\x00\x04\xc0\xa8\xd0\x05-\x0c\x00\x01\x80\x01\x00\x00"
         b"\x00\x01\x00\x04\xc0\xa8\xd0\x06"
     )
+    overflow = 5
     _incoming_module._seen_logs.clear()
-    for port in range(_MAX_SEEN_LOGS + 5):
+    for port in range(_MAX_SEEN_LOGS + overflow):
         r.DNSIncoming(packet, ("1.2.3.4", port))
-    assert len(_incoming_module._seen_logs) <= _MAX_SEEN_LOGS
+    # Bound is hit exactly — confirms the parser exception path actually
+    # entered the dict with a per-port-unique key; a future change that
+    # dropped self.source from the exception text would collapse to a
+    # single dedup key and fail this assertion.
+    assert len(_incoming_module._seen_logs) == _MAX_SEEN_LOGS
+    # FIFO eviction: the earliest port's exception string is gone, the
+    # latest port's is still present.
+    assert not any("'1.2.3.4', 0)" in k for k in _incoming_module._seen_logs)
+    assert any(f"'1.2.3.4', {_MAX_SEEN_LOGS + overflow - 1})" in k for k in _incoming_module._seen_logs)
 
 
 def test_label_length_attack():
