fix(engine): close unpinned rule-cache torn-read + review follow-ups to #49 (#50)

* fix(engine): publish the rule cache via atomic rebind to close the unpinned torn-read

The unpinned path returned _RULE_CACHE["exact"], ["regex"], ["memo"] as three
separate dict reads while a reload mutated those keys in place, so a concurrent
reload on another worker thread could pair one rule-set version's rules with a
different version's memo and then memoize a stale-version result into the live
memo (persisting until the next rule edit).

_get_enabled_rules() now reads the module global once into a local, and a reload
publishes the new set by rebinding _RULE_CACHE to a fresh dict in a single atomic
assignment. A reader sees the whole old set or the whole new one — never exact
from V1 paired with memo from V2. The pin-prime path captures from the same
consistent local, so it can no longer snapshot a torn pair either.

* fix(config): raise min_version to 4.3.0 to match the documented floor

PR #49 raised the stated compatibility floor to NetBox >= 4.3.0 in README.md and
docs/installation.md but left the enforced PluginConfig.min_version at 4.2.0, so a
4.2.x install would still load despite the docs declaring it unsupported. Align
the gate with the docs and pin them together with a test.

* test(engine): harden rule-cache tests and guard _VERSION_COLUMNS exhaustiveness

- Add test_reload_publishes_a_fresh_cache_dict_atomically: asserts a reload swaps
  in a new cache dict instead of mutating the old one in place — the invariant
  behind the unpinned torn-read fix.
- Rewrite test_pinned_block_holds_snapshot_across_concurrent_cache_reload to use a
  real second thread, proving _pin is thread-local (a plain global now fails it),
  with try/finally cache restoration so the simulated reload can't leak.
- test_memo_is_bounded now checks the cap after every insert, catching a mid-loop
  leak toward 2*cap that the old end-of-loop snapshot missed.
- Add EnabledRuleFingerprintColumnsTest: fails if any concrete model column is
  neither fingerprinted nor explicitly excluded, so a future match-affecting field
  can't silently break fingerprint-based cache invalidation.

* fix(engine): make the rule-cache memo read atomic and isolate the pinned memo

CodeRabbit (Major, #50): find_matching_rule's `if sig in memo: return memo[sig]` is a
non-atomic compound read — another thread sharing the per-version memo can `memo.clear()`
at the cap between the membership test and the subscript, raising a sporadic KeyError under
threaded workers (the switch can land between the two bytecodes even under the GIL). The
pinned path made it worse by aliasing the shared memo into thread-local state.

- Read via a single atomic `memo.get(sig, _MEMO_MISS)` (sentinel distinct from a memoized
  None), so the separate membership test the race needs no longer exists — fixes both the
  unpinned and pinned paths.
- Copy the memo into the pin (`_pin.memo = dict(cache["memo"])`) and return it, so a pinned
  batch neither shares nor races the global memo and a concurrent clear can't evict its
  warmed entries mid-loop.

Both tests red-checked: test_find_matching_rule_survives_concurrent_memo_clear reproduces the
KeyError deterministically with a dict that clears on its membership test;
test_pinned_block_uses_a_private_memo_copy asserts the pin holds a private copy.

Author: marcinpsk

netbox_interface_name_rules/__init__.py

@@ -13,7 +13,7 @@ class InterfaceNameRulesConfig(PluginConfig):
     description = "Automatic interface renaming when modules are installed into bays."
     version = __version__
     base_url = "interface-name-rules"
-    min_version = "4.2.0"
+    min_version = "4.3.0"
     required_settings = []
     default_settings = {}
     author = "Marcin Zieba"

netbox_interface_name_rules/engine.py

@@ -27,6 +27,9 @@
 # per call and the set reloaded only when it changes, so it self-invalidates on any
 # create/delete/edit — including raw bulk ``.update()`` of any matching field and SET_NULL
 # cascades that bypass auto_now, and across test transactions — with no signal wiring.
+# A reload publishes the new set by rebinding this name to a fresh dict in one atomic assignment
+# (see _get_enabled_rules), so a concurrent reader on another worker thread sees either the whole
+# old set or the whole new one — never exact/regex/memo torn across two versions.
 _RULE_CACHE = {"version": None, "exact": (), "regex": (), "memo": {}}
 
 # Per-version cap on the find_matching_rule memo. A long-lived worker that sees many distinct
@@ -35,6 +38,13 @@
 # of contexts in any single module-sync render, so it never churns mid-render.
 _MEMO_MAX = 4096
 
+# Sentinel for the memo read. A single ``memo.get(sig, _MEMO_MISS)`` is one atomic dict lookup, so a
+# concurrent ``memo.clear()`` at the cap can't wedge between a membership test and the subscript the
+# way ``if sig in memo: return memo[sig]`` could — that compound read can raise a sporadic KeyError
+# under threaded workers sharing one per-version memo. A real "no rule matched" is memoized as None,
+# so the miss sentinel must be a distinct object, not None.
+_MEMO_MISS = object()
+
 # Per-thread pin depth (see pinned_rule_cache). While > 0 on the current thread, _get_enabled_rules
 # trusts the loaded set without re-reading the fingerprint, so an internal batch that wraps its loop
 # turns N per-call fingerprint queries into one. Thread-local so concurrent requests don't affect
@@ -198,6 +208,8 @@ def _get_enabled_rules():
     lookup in the block) the fingerprint query is skipped and the snapshot captured at prime time is
     returned — never the live ``_RULE_CACHE``, which another thread may reload mid-block.
     """
+    global _RULE_CACHE
+
     pinned = getattr(_pin, "depth", 0) > 0
     if pinned and getattr(_pin, "primed", False):
         # Serve the snapshot captured when this block primed, not the shared cache: a concurrent
@@ -207,27 +219,36 @@ def _get_enabled_rules():
 
     from .models import InterfaceNameRule
 
+    # Read the module global exactly once. A reload below publishes the new set by rebinding
+    # _RULE_CACHE to a brand-new dict (a single atomic name assignment) rather than mutating this
+    # one in place, so this local is a consistent snapshot: exact/regex/memo can never be torn
+    # across two rule-set versions even if another thread reloads between the reads at the end.
+    cache = _RULE_CACHE
     version = _enabled_rules_version()
-    if _RULE_CACHE["version"] != version:
+    if cache["version"] != version:
         rules = list(InterfaceNameRule.objects.filter(enabled=True).order_by("module_type__model", "pk"))
-        _RULE_CACHE["exact"] = tuple(r for r in rules if not r.module_type_is_regex)
+        exact = tuple(r for r in rules if not r.module_type_is_regex)
         regex_rules = sorted(
             (r for r in rules if r.module_type_is_regex),
             key=lambda r: (-len(r.module_type_pattern or ""), r.pk),
         )
-        _RULE_CACHE["regex"] = tuple((_compile_pattern(r.module_type_pattern), r) for r in regex_rules)
-        _RULE_CACHE["memo"] = {}
-        _RULE_CACHE["version"] = version
+        regex = tuple((_compile_pattern(r.module_type_pattern), r) for r in regex_rules)
+        # Publish the whole new version atomically: a reader either sees the old dict or this one,
+        # never a mix of the two. Last writer wins; a concurrent reload to the same version just
+        # rebuilds redundantly, never corrupts.
+        cache = {"version": version, "exact": exact, "regex": regex, "memo": {}}
+        _RULE_CACHE = cache
     if pinned:
         # Pin this thread to the freshly-resolved set for the rest of the block. The tuples are
-        # immutable and the memo dict is now this thread's own reference, so a later global reload
-        # leaves our snapshot — and the entries we memoize into it — untouched.
-        _pin.exact = _RULE_CACHE["exact"]
-        _pin.regex = _RULE_CACHE["regex"]
-        _pin.memo = _RULE_CACHE["memo"]
+        # immutable and the memo is COPIED into thread-local state — not aliased — so the pinned batch
+        # neither shares nor races the global memo: another thread clearing the shared memo at the cap
+        # can't evict our warmed entries (or wedge a KeyError) mid-loop, and entries we add stay private.
+        _pin.exact = cache["exact"]
+        _pin.regex = cache["regex"]
+        _pin.memo = dict(cache["memo"])
         _pin.primed = True
         return _pin.exact, _pin.regex, _pin.memo
-    return _RULE_CACHE["exact"], _RULE_CACHE["regex"], _RULE_CACHE["memo"]
+    return cache["exact"], cache["regex"], cache["memo"]
 
 
 def _get_parent_module_type(module_bay):
@@ -675,8 +696,11 @@ def find_matching_rule(module_type, parent_module_type, device_type, platform=No
     # The regex tier matches against module_type.model (a live string), so the memo must key on
     # it too — otherwise a ModuleType.model rename (same pk) would return a stale regex result.
     sig = (module_type.pk, module_type.model, *_scope_ids(parent_module_type, device_type, platform))
-    if sig in memo:
-        return memo[sig]
+    # One atomic lookup, not `if sig in memo: return memo[sig]`: another thread sharing this per-version
+    # memo can clear it at the cap between a membership test and the subscript, raising KeyError.
+    cached = memo.get(sig, _MEMO_MISS)
+    if cached is not _MEMO_MISS:
+        return cached
 
     candidates = _build_candidates(parent_module_type, device_type, platform)
     result = _find_exact_match(module_type, candidates, exact_rules) or _find_regex_match(

netbox_interface_name_rules/tests/test_misc.py

@@ -801,3 +801,22 @@ def test_to_csv_no_dots_in_headers(self):
         """csv_headers must not contain dots (regression guard for KeyError 'Ch' bug)."""
         for header in InterfaceNameRule.csv_headers:
             self.assertNotIn(".", header, f"csv_headers entry '{header}' contains a dot — would break import")
+
+
+# ---------------------------------------------------------------------------
+# __init__.py — PluginConfig version gate (must match the documented floor)
+# ---------------------------------------------------------------------------
+
+
+class PluginConfigVersionTest(TestCase):
+    """The enforced min_version gate must match the compatibility floor stated in the docs."""
+
+    def test_min_version_matches_documented_floor(self):
+        """min_version is the gate NetBox uses to refuse loading the plugin, so it must equal the
+        floor advertised in README.md / docs/installation.md (NetBox >= 4.3.0). A drift here would
+        let an unsupported NetBox load the plugin (or reject a supported one) with no other guard —
+        the inconsistency a reviewer flagged when the docs were raised to 4.3.0 but the gate stayed 4.2.0.
+        """
+        from netbox_interface_name_rules import InterfaceNameRulesConfig
+
+        self.assertEqual(InterfaceNameRulesConfig.min_version, "4.3.0")