Fixes #685: guard get_models() against re-entrancy

[bctiemann]

Fixes: #685

Summary

POST /api/plugins/custom-objects/schema/apply/ raised RecursionError when a document created two or more new, cross-referencing Custom Object Types in a single request. Generating a brand-new COT's model calls _after_model_generation(), which can itself need Django's global relation graph (Options._relation_tree) -- e.g. via ObjectType.objects.get_for_model()'s .create(). Rebuilding that graph calls apps.get_models(), which re-enters this plugin's own get_models() while it's still mid-generation. Without a guard, that re-entrant call walks CustomObjectType.objects.all() and calls get_model() again for every COT -- including the one still under construction -- with no way to ever finish.

#686 (closed as a duplicate) hit the identical get_models() recursion through a completely different call site: _wire_polymorphic_reverse_descriptors() -> field_instance.related_object_types.all(), triggered during ordinary polymorphic-field save/modify, not the executor at all.

Evaluating the proposed fix

The issue's own investigation laid out three options, roughly by invasiveness: (1) serialize COT finalization in the executor, (2) pre-create ObjectType rows before any FK reference resolves, (3) a re-entrancy guard on get_models() itself. @jnovinger's review recommended option 3 specifically because of #686 -- an executor-only fix (1 or 2) would resolve #685 but leave #686, and any future path into the same hazard, live. This PR implements option 3.

A contextvars.ContextVar-based guard (matching the existing _is_migrating idiom already in this file) is safe here because generate_model()'s type() call already registers a COT's model with Django's app registry synchronously, before get_model() ever calls _after_model_generation() (the method that can trigger this re-entrancy). So a re-entrant call can simply fall back to super().get_models() (everything already registered) without needing to regenerate anything -- any COT the outer loop hasn't reached yet is just absent from that transient snapshot, and self-heals via get_model()'s own apps.clear_cache() once the remaining COTs finish generating.

Testing

Per jnovinger's explicit ask, adds a regression test for each re-entry path:

• SchemaApplyMultiCOTRecursionTestCase (test_schema_api.py) -- the executor path from schema/apply/ raises RecursionError when a document creates multiple new, cross-referencing Custom Object Types in one request #685's own repro (2-type and 3-type chained-reference documents posted through the real API view).
• PolymorphicReverseDescriptorRecursionTestCase (test_polymorphic_fields.py) -- the descriptor-wiring path from RecursionError when accessing COT models with polymorphic reverse descriptors #686.

Both needed one non-obvious adjustment to actually exercise the vulnerable code: get_models()'s CustomObjectType-enumeration loop is unconditionally disabled under manage.py test (should_skip_dynamic_model_creation() returns True whenever "test" in sys.argv), so without patching around that, these tests silently no-op regardless of the fix. Both tests now patch _app_ready / should_skip_dynamic_model_creation to replicate a non-test process, and each was verified to reproduce a genuine RecursionError against this commit's parent, then pass cleanly with the fix.

test_schema_api.py also gets a third, deterministic white-box test (test_get_models_guards_against_reentrant_cot_generation) that simulates the re-entrant trigger directly via a call-counting spy on get_model(), so the invariant is verified independent of whatever incidental factors (cache warmth, interpreter version, stack depth) determine whether a given environment happens to blow past Python's recursion limit.

Full netbox_custom_objects suite verified in the shared dev venv: 1179 tests, 0 new failures (15 pre-existing errors, all attributable to the sibling netbox-branching checkout's known incompatibility with this venv's Django/NetBox version -- confirmed to fail identically in isolation, unrelated to this change).

[bctiemann]

@claude /review and limit response to actionable issues.

[github-actions]

Claude finished @bctiemann's task in 2m 55s —— View job

---

Code Review

• Read changed files
• Review __init__.py changes (re-entrancy guard)
• Review test_schema_api.py changes
• Review test_polymorphic_fields.py changes

---

The core fix is correct and well-reasoned. ContextVar is the right tool here (matches the existing _is_migrating idiom, and is actually safer than a thread-local in any future async context). The try/finally correctly resets the token on every exit path (the early return at the guard check happens before token = _generating_models.set(True), so there's nothing to reset there). Two actionable issues below.

---

1. self.enterContext() is Python 3.11+, project requires 3.10

unittest.TestCase.enterContext() was added in Python 3.11. pyproject.toml declares requires-python = ">=3.10", so these tests will AttributeError on 3.10 installs. CI happens to run 3.12 so this doesn't surface there.

Affected lines: test_schema_api.py:430-432 and test_schema_api.py:475-477

Note that test_polymorphic_fields.py already uses the correct cross-version approach (the with (mock.patch.object(...), ...): context manager form). The schema API tests should match that pattern, or use the patcher.start() + self.addCleanup(patcher.stop) equivalent:

# Replace:
self.enterContext(mock.patch.object(nco_pkg, '_app_ready', True))
self.enterContext(
    mock.patch.object(app_config, 'should_skip_dynamic_model_creation', return_value=False)
)
# ...test body...

# With (wrapping the test body):
with (
    mock.patch.object(nco_pkg, '_app_ready', True),
    mock.patch.object(app_config, 'should_skip_dynamic_model_creation', return_value=False),
):
    # ...test body...

Fix this →

---

2. Repeated inline imports across test methods

from unittest import mock, from django.apps import apps as django_apps, and import netbox_custom_objects as nco_pkg are re-imported inside each of the three test methods in SchemaApplyMultiCOTRecursionTestCase. These can be module-level imports; there are no circular-import risks with these modules in a test file.

Fix this →

---

Everything else looks solid — the guard semantics are correct, the finally correctly covers all exit paths (including early returns inside the try), and the three-tier test approach (2-type API, 3-type API, deterministic spy) is thorough.

[bctiemann]

Re: finding #1 (Python 3.10 compatibility) -- looked into this. NetBox core itself requires Python 3.12+ as of v4.5 (requires-python = ">=3.12", and the install docs say outright "NetBox supports only Python 3.12 or later"). Since this plugin only ever runs loaded inside a NetBox instance, there's no real deployment path where its test suite executes under 3.10/3.11. The >=3.10 floor in this repo's pyproject.toml/AGENTS.md/ruff.toml looks like stale metadata that was never updated when NetBox moved its own floor -- CI only ever tests 3.12, and nothing else in the codebase avoids 3.11+ features.

Leaving enterContext() as-is here. We'll correct the stale requires-python/classifiers/ruff target in the v0.7.0 release PR instead of touching test code for a constraint that isn't real.

Finding #2 (repeated inline imports) is a fair, uncontroversial cleanup -- fixing that now.