perf(engine): load the enabled rule set once and memoize find_matching_rule#49
Merged
Conversation
…g_rule find_matching_rule() ran a DB query per scope-candidate per tier on every call, and it is called once per module row when an integration (netbox-librenms-plugin) renders a device's module-sync table — so a 40-module device fired hundreds of rule-lookup queries per render. Load the (small) enabled rule set once, match exact/regex in memory preserving the previous specificity ordering, and memoize the per-context result. The cache is keyed on a cheap (count, max last_updated) fingerprint of the enabled rules, so it self-invalidates on any create/delete/edit — including across test transactions — with no signal wiring. _find_exact_match/_find_regex_match keep their signatures (preloaded rules passed on the hot path, loaded on demand otherwise) so direct callers are unaffected. Tests: FindMatchingRuleCachingTest — a repeated call runs only the version fingerprint query (red against the per-candidate form), and a rule change invalidates the cache. Full engine + advanced + regex + rules + device_rules suites unchanged (semantics preserved). Claude-Session: https://claude.ai/code/session_01RKRVyWizgrukFSHmTF168J
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe rule engine now caches enabled interface-name rules, normalizes scope comparisons, memoizes lookup results, and pins cache state during deferred module reapplication. Tests cover cache behavior, validation handling, and supported NetBox version references were updated. ChangesInterface-name rule lookup cache
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Engine as find_matching_rule
participant Cache as _get_enabled_rules
participant RuleSet as exact and regex rules
Caller->>Engine: module_type and scope inputs
Engine->>Cache: get cached enabled rules
Cache->>RuleSet: load or reuse snapshot
RuleSet-->>Engine: matching candidates
Engine-->>Caller: rule or None
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Review of the memoization change surfaced cache-staleness gaps and weak test coverage. The in-process rule cache is keyed on a cheap fingerprint of the enabled rule set; the old (count, max last_updated) fingerprint plus the pk-only memo key missed several real invalidation inputs. - ModuleType.model rename: the regex tier matches against the live module_type.model string, but the memo keyed only on module_type.pk, so a rename (same pk) returned a stale regex result. Key the memo sig on module_type.model too. - auto_now-bypassing scope edits: a SET_NULL cascade nulling a scope FK, or a bulk .update() of a scope/channel field, changes neither the enabled count nor last_updated, so the cache never reloaded in a long-lived worker. Fold per-field Sum() of the scope FK ids and channel fields into the fingerprint so those edits change it. A raw bulk .update() of a *text* field still bypasses this (no count/sum/last_updated change) — documented; use .save(). Tests: - test_memoized_result_is_not_recomputed counts real _find_exact_match calls, so it fails if memoization is removed — the old query-count test passed with or without the memo because the matchers issue no query once preloaded. - test_scope_fk_update_invalidates_cache and test_module_type_model_rename_reevaluated are red against the pre-fix engine and green after; plus a same-count .save()-edit invalidation guard. The per-call fingerprint query is kept (now one slightly richer aggregate) rather than eliminated via a TTL throttle: a throttle broke the cache's self-invalidation across test transactions and opened a window where a rolled-back rule create is served — too much correctness risk for a perf nicety the per-candidate-storm removal already mostly delivers. Full plugin suite (318 tests) green.
Cleanup findings from the same review — all behavior-preserving except the two new guards, which are covered by tests. - Remove dead _scope_filter(): the in-memory match refactor left it with no callers anywhere in the repo. - De-triplicate the "scope object -> FK id" coalescing and the FK-equality check into _scope_ids() / _rule_scope_matches(), now used by both match tiers and the memo key instead of three hand-copied blocks. - Pre-sort and pre-compile the regex tier once when the cache is (re)built: _RULE_CACHE["regex"] is now a tuple of (compiled_pattern, rule) pairs ordered by (-pattern length, pk), so _find_regex_match no longer re-sorts per candidate nor recompiles each pattern on every match. An invalid pattern compiles to None and is skipped, same as the old per-match try/except re.error. - Bound the per-version memo with _MEMO_MAX so a long-lived worker that sees many distinct (module_type, scope) contexts can't grow it without limit; on overflow it is cleared wholesale (the cap is far above any single render's context count). - find_matching_rule(None, ...) now returns None instead of raising AttributeError. Both tiers dereference module_type (.pk / .model), so the old "None is valid" branches in the memo key were misleading dead defensiveness. Tests: the None-guard and memo-cap tests are red against the pre-change engine; the existing regex error-skip and pattern-length tiebreak tests pin the behavior-preserving refactor. Full plugin suite (320 tests) green.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@netbox_interface_name_rules/engine.py`:
- Line 15: The cache fingerprint used by _get_enabled_rules() is not
deterministic because it relies on aggregate Sum(...) and omits rule fields that
affect matching/output. Update the fingerprinting logic to derive a stable
version from ordered per-rule fields in the relevant rule model/query path,
including name_template, module_type_pattern, and module_type_is_regex, or add
explicit cache invalidation in the write path so stale memoized results cannot
persist after edits.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 302684b9-2947-49dc-bc16-7fba5660bdc8
📒 Files selected for processing (2)
netbox_interface_name_rules/engine.pynetbox_interface_name_rules/tests/test_rules.py
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
The perf change left total coverage at the ~96.5% rounding boundary, so the oldest matrix job (NetBox v4.3.7) reported 96% < fail_under=97 even though every test passed (newer versions rounded up to 97). Cover two real, previously untested paths so the gate clears on all versions: - _try_rename_device_interface logs and skips — rather than raising — a computed device-interface name that fails full_clean(), exercised with a name that exceeds Interface.name's max_length. This pins the "a bad computed name never aborts the VC re-rename batch" behaviour. - _find_exact_match loads the enabled rule set on demand when called without a preloaded set (the direct-caller branch the memoization change introduced), mirroring the existing _find_regex_match direct-call test. engine.py coverage 97% -> 98%.
…nt in batch callers find_matching_rule() re-reads a cheap rule-set fingerprint on every call so it self- invalidates. For a read-heavy batch caller that makes one call per row in a single request — e.g. netbox-librenms-plugin's module-sync table, which predicts names once per installed module — that is N fingerprint aggregates per render. pinned_rule_cache() is an opt-in context manager the caller wraps its loop in: the first lookup inside the block primes the set (one fingerprint read), and every later call reuses it, turning N fingerprint queries into one. Priming is lazy, so a block that makes no lookups (e.g. a render with nothing to predict) issues no query at all — important because callers may enter the block in code paths that must not touch the database. Unlike a global TTL throttle (considered and rejected for breaking cross-transaction self- invalidation), the pin is explicit and per-thread (threading.local depth + primed flag): outside the block, and in every other thread/request, the normal self-invalidating behaviour is unchanged, so there is no global staleness window and unmarked tests stay DB- free. Nesting is safe; rule edits made inside a block are intentionally not observed until it exits, so it is for read-only, edit-free batches. Tests: a pinned loop of N lookups issues one fingerprint query vs N unpinned; a pinned block matches the set primed by its first lookup and normal self-invalidation resumes after exit. Full suite (324) green.
pinned_rule_cache() was added for an external batch caller (librenms) to wrap its loop, but that requires a direct import of INR's engine API — cross-plugin coupling we don't want. Cross-plugin integration stays on the loose-coupled predict_module_interface_names signal (dispatched per row, so it can't pin). Re-scope the context manager as INR-internal: use it in _apply_rules_for_device_deferred's multi-module VC-reapply loop (one fingerprint read per device, not per module) and reframe the docstring.
…ash) The enabled-set fingerprint was an aggregate of count + max(last_updated) + FK/channel sums. That misses a raw bulk .update() of a text/bool matching field (name_template, module_type_pattern, module_type_is_regex), and Sum() can collide under compensating edits — both leave a stale rule cached. Replace it with a server-side md5(string_agg(... ORDER BY id)) over every enabled rule's matching/output columns: one query, a single 32-char hash, deterministic and collision-free (NUL-free control-char separators, pk-anchored rows). Raw SQL keeps it Django-version-independent. Adds tests for bulk text/pattern updates and a compensating-FK-swap collision (all red-checked). Addresses CodeRabbit finding on the engine.py fingerprint.
Add v4.6.3 and drop v4.4.10 to keep the matrix at three NetBox versions (4.3.7 / 4.5.3 / 4.6.3). The new content-hash fingerprint is raw Postgres SQL, so it is Django-version-independent across the range. The 3.14 × v4.3.7 exclude and the v4.5.3-gated coverage upload are unchanged (v4.5.3 stays in the matrix).
README said ≥ 4.2.0 and docs/installation.md said ≥ 4.4.0 — inconsistent with each other and with CI, which tests 4.3.7 as the oldest version. Set both to NetBox ≥ 4.3.0 to match the lowest tested release.
marcinpsk
force-pushed
the
perf/memoize-find-matching-rule
branch
from
dcb5bd3 to
d340e45
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
netbox_interface_name_rules/tests/test_rules.py (1)
286-294: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReset the engine cache between test methods.
These tests mutate
find_matching_rule()'s module-level_RULE_CACHE, butTestCaseonly rolls back the database. Because_enabled_rules_version()is content-based, a later method that recreates the same rule set can reuse stale memo entries or staleInterfaceNameRuleobjects from a previous method instead of reloading from the database, so this class is order-dependent.🧪 Suggested isolation reset
class FindMatchingRuleCachingTest(TestCase): """find_matching_rule loads rules once, memoizes per context, and reloads when rules change.""" + def setUp(self): + from netbox_interface_name_rules import engine + + engine._RULE_CACHE.update({"version": None, "exact": (), "regex": (), "memo": {}}) + engine._pin.depth = 0 + engine._pin.primed = False + `@classmethod` def setUpTestData(cls): mfr = Manufacturer.objects.create(name="CacheMfg", slug="cachemfg") cls.module_type = ModuleType.objects.create(manufacturer=mfr, model="C-SFP", part_number="C-SFP") cls.device_type = DeviceType.objects.create(manufacturer=mfr, model="C-DEV", slug="c-dev")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@netbox_interface_name_rules/tests/test_rules.py` around lines 286 - 294, The FindMatchingRuleCachingTest methods are sharing stale module-level cache state in find_matching_rule(), making the class order-dependent. Add test isolation by clearing the engine cache before each test method (for the _RULE_CACHE used by find_matching_rule and any memoized per-context entries), ideally in setUp or a helper on FindMatchingRuleCachingTest, so each method exercises a fresh reload of InterfaceNameRule data from the database.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@netbox_interface_name_rules/engine.py`:
- Around line 155-160: The pinned snapshot logic in pinned_rule_cache still
returns shared _RULE_CACHE objects, so a reload can change the rule set
mid-block; update the cache handling so a pinned thread keeps using one
immutable snapshot for the entire pinned section. Use pinned_rule_cache and the
_RULE_CACHE access path to capture and retain the exact/regex/memo tuple at
first priming, then return that same snapshot on subsequent lookups instead of
re-reading the mutable global cache.
In `@netbox_interface_name_rules/tests/test_device_rules.py`:
- Around line 148-156: The test in test_validation_failure_is_logged_and_skipped
does not actually verify the logged failure path it describes. Update it to
derive the overlong rule value from Interface._meta.get_field("name").max_length
instead of hard-coding 70, and wrap apply_device_interface_rules(self.device1)
in assertLogs so the warning branch is asserted. Keep the existing checks that
the result is 0 and that iface.refresh_from_db() restores the original name.
---
Outside diff comments:
In `@netbox_interface_name_rules/tests/test_rules.py`:
- Around line 286-294: The FindMatchingRuleCachingTest methods are sharing stale
module-level cache state in find_matching_rule(), making the class
order-dependent. Add test isolation by clearing the engine cache before each
test method (for the _RULE_CACHE used by find_matching_rule and any memoized
per-context entries), ideally in setUp or a helper on
FindMatchingRuleCachingTest, so each method exercises a fresh reload of
InterfaceNameRule data from the database.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 774ee3ca-d0e6-4f56-9251-557ae6d075f6
📒 Files selected for processing (8)
.github/workflows/test.yamlREADME.mddocs/installation.mdnetbox_interface_name_rules/engine.pynetbox_interface_name_rules/signals.pynetbox_interface_name_rules/tests/test_device_rules.pynetbox_interface_name_rules/tests/test_rules.pynetbox_interface_name_rules/tests/test_signals.py
SonarQube flagged the f-string-built SQL passed to cursor.execute() in _enabled_rules_version() as a dynamic-SQL hotspot. The query was safe (table name via quote_name, every other token a static literal), but rebuild it through the Django ORM so identifiers are quoted by the compiler and the separators are bound parameters — no string-interpolated SQL at all. Same semantics (verified: byte-identical hash for identical data; the existing fingerprint/invalidation tests stay green): md5(string_agg(... ORDER BY id)) over the matching/output columns, via a small custom Aggregate whose template we own so it does not depend on StringAgg's Python signature (which differs across the Django 5.x-6.x CI matrix).
pinned_rule_cache() pinned only depth/primed in thread-local state, but _get_enabled_rules() still returned the shared _RULE_CACHE objects on every pinned lookup. If another thread reloaded the cache mid-block, later iterations of the same pinned batch silently switched to the new rule set — so one device's modules could be renamed with mixed rule versions. Capture exact/regex/memo into the thread-local _pin at prime time and serve that snapshot for the rest of the block (releasing it on exit). The tuples are immutable and the memo becomes the thread's own reference, so a concurrent global reload can't change the batch's rule set. New test reloads _RULE_CACHE mid-block and asserts the pinned lookup still returns the primed set (red-checked). Also reset the module-level _RULE_CACHE/_pin in FindMatchingRuleCachingTest.setUp so the cache-mutating tests (incl. the new concurrent-reload one) stay order-independent — TestCase rolls back only the DB, not the in-process cache.
test_validation_failure_is_logged_and_skipped asserted only result==0 and a
preserved name — which "no rule matched" also satisfies, so it could pass without
exercising the full_clean() failure branch. Wrap the call in assertLogs(...) to
require the warning, and derive the over-length name from
Interface._meta.get_field("name").max_length instead of hard-coding 70 so it still
triggers if the field limit changes. Verified: with a fitting name the assertLogs
assertion fails.
marcinpsk
commented
Jun 25, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@netbox_interface_name_rules/engine.py`:
- Around line 121-156: The fingerprint encoding in _version_row_signature and
_Md5OrderedStringAgg is still vulnerable to collisions because _FIELD_SEP and
_ROW_SEP can appear in InterfaceNameRule text fields; update the row signature
construction to use an unambiguous scheme such as length-prefixed fields or
explicit escaping, and keep the change localized to the fingerprint-building
logic so distinct rule sets cannot serialize to the same signature.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: cea3806f-9493-4ae0-a9cf-a1622abd04ed
📒 Files selected for processing (3)
netbox_interface_name_rules/engine.pynetbox_interface_name_rules/tests/test_device_rules.pynetbox_interface_name_rules/tests/test_rules.py
marcinpsk
force-pushed
the
perf/memoize-find-matching-rule
branch
from
0f28009 to
7ab7427
Compare
The fingerprint joined columns/rows with chr(31)/chr(30) separators on the assumption they "cannot occur in the text columns" — but nothing rejects those bytes in name_template / module_type_pattern. A crafted name_template can embed the separators to forge a row boundary, so two distinct rule sets serialize to the same byte stream and hash identically, silently missing a cache invalidation. Encode each column length-prefixed as "<len>:<value>" instead. The row encoding is now self-delimiting and rows concatenate unambiguously (string_agg delimiter ''), so distinct rule sets cannot collide regardless of text-column content. New test forges a one-rule set reproducing a two-rule set's old byte stream and asserts the fingerprints differ (red-checked: identical hash under the old separator encoding).
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
marcinpsk
added a commit
that referenced
this pull request
Jun 29, 2026
#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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
find_matching_rule()previously ran a DB query per scope-candidate per tier on every call. Because it's called once per module row when an integration (netbox-librenms-plugin) renders a device's module-sync table, a 40-module device fired hundreds of rule-lookup queries per render.This change loads the (small) enabled rule set once, matches exact/regex in memory while preserving the previous specificity ordering, and memoizes the per-context result.
How it works
(count, max last_updated)fingerprint of the enabled rules, so it self-invalidates on any create/delete/edit — including across test transactions — with no signal wiring._find_exact_match/_find_regex_matchkeep their signatures (preloaded rules passed on the hot path, loaded on demand otherwise) so direct callers are unaffected.Tests
FindMatchingRuleCachingTest— a repeated call runs only the version-fingerprint query (red against the per-candidate form), and a rule change invalidates the cache.Summary by CodeRabbit
Performance
Bug Fixes
max_length.Tests
Documentation / Compatibility