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.

[jnovinger]

The guard is right and the registration-ordering argument holds, so super().get_models() really does cover the model under construction.

Two potential test improvements:

• The tests assert "didn't raise" plus existence, so they'd pass just as well against a get_models() that returned zero COT models — which is the degradation mode the guard introduces. test_schema_operations.py's #335 test has the shape I'd want here: assertIn(model, apps.get_models()).

• Since get_models() is a generator, the set/reset pair is scoped to iteration rather than a call frame, so an assertFalse(_generating_models.get()) after the call would pin that too. It can't fail against the list(...) the tests use today, but it's what would catch a later refactor that leaves the flag set and starts silently truncating everyone else's model list.

[bctiemann]

Both applied, thanks.

• assertIn(model, apps.get_models()) added to all three end-to-end/deterministic tests, plus assertFalse(_generating_models.get()) after every guarded call.

• Applying the assertIn check to the RecursionError when accessing COT models with polymorphic reverse descriptors #686 polymorphic-descriptor test surfaced a real bug, not a flaw in the test: cot.get_model() called directly (not via get_models()'s own loop) on a not-yet-cached COT can trigger generate_model() twice for the same COT -- once for the direct call, and once more when _wire_polymorphic_reverse_descriptors() re-enters get_models() before the direct call has cached anything (this reentrancy guard isn't set yet at that point, since the direct call never went through get_models()'s loop itself). The two generations register different classes in different places: get_model()'s return value and CustomObjectType._model_cache end up holding one class, while Django's app registry (apps.all_models, and therefore everything that resolves models through it) ends up holding the other.

  Confirmed via instrumentation -- generate_model() fires twice, producing two distinct class objects, with the divergence landing exactly where described. This guard correctly prevents it from recursing infinitely (bounded to 2 generations instead of a crash), but doesn't prevent the resulting cache/registry split. It's been latent in the caching design all along; before this fix, this exact interleaving always hit RecursionError before reaching a stable state, so it was never observable.

  Filed as get_model() can leave _model_cache and apps.all_models pointing at two different classes for the same COT #688 with the full repro and root-cause trace, out of scope for this PR. The test now looks up the actually-registered class via apps.get_model() rather than trusting get_model()'s return value, with an inline comment pointing at get_model() can leave _model_cache and apps.all_models pointing at two different classes for the same COT #688.

Verified: all 4 tests fail with a genuine RecursionError against the pre-fix code and pass with the fix. Full test_schema_api.py + test_polymorphic_fields.py run: 113 tests, 0 new failures (1 pre-existing, unrelated netbox-branching environment error, same as before).

[bctiemann]

The two tests (main, branching) CI failures here are unrelated to this PR's changes.

Root cause: NetBox's main branch was released as v4.7.0 today (2026-09-02). netboxlabs-netbox-branching's latest release (v1.1.3, 2026-08-26) still declares a maximum supported NetBox version of 4.6.99, so this workflow's "main, branching" matrix leg -- which floats both netbox-ref: main and netboxlabs-netbox-branching>=1.0.4,<2.0.0 -- now installs an incompatible combination. The plugin correctly refuses to load at that version mismatch, but test_branching.py imports netbox_branching.models directly regardless of whether the plugin loaded, and that import crashes before Django's app-registry check even runs.

Confirmed this is repo-wide, not branch-specific: manually dispatched this same workflow directly against main (https://github.com/netboxlabs/netbox-custom-objects/actions/runs/33687827160) with zero code changes, and tests (main, branching) fails there too, while tests (main) and tests (feature) both pass. The last green run of this leg was two days ago, before NetBox's 4.7.0 cut.

This needs a fix on the CI/dependency side (pin the branching leg's netbox-ref to a version netbox-branching actually supports, or wait for a netbox-branching release compatible with 4.7.0) rather than anything in this PR.

[bctiemann]

Update: netbox-branching 1.2.0 fixes the version-gate crash from my earlier comment (reran both failed jobs here after the release -- the plugin now loads correctly under NetBox 4.7.0), but surfaces two new, genuine test failures that are unrelated to this PR's diff: test_cot_deleted_in_branch_merge_and_revert (NOT NULL violation on cache_timestamp during branch.revert()) and test_single_field_delete_merge_and_revert (a dropped column isn't restored on revert), both in IterativeBranchingTestCase and SquashBranchingTestCase.

Confirmed these are pre-existing and not caused by anything here: dispatched lint-tests.yaml directly against main with zero code changes (https://github.com/netboxlabs/netbox-custom-objects/actions/runs/33695787237) and got the identical failure signature -- 61 tests, 2 failures, 2 errors, same test names.

Filed as #689 with full tracebacks and a working theory (likely related to 1.2.0's mptt->ltree/trigger migration per its release notes). This PR's own tests (test_schema_api.py, test_polymorphic_fields.py) are unaffected and still pass cleanly.