From 72924075f129218495c8b9b910fe847bd2f013c6 Mon Sep 17 00:00:00 2001 From: Brian Tiemann Date: Fri, 7 Aug 2026 12:20:06 -0400 Subject: [PATCH 1/6] Fixes #640: Serialize polymorphic multiobject through-model creation against concurrent readers create_polymorphic_m2m_table() built and registered a fresh through-model class and only afterward repointed its "source" FK at the caller's model, all without holding CustomObjectType._global_lock. A concurrent get_model(no_cache=True) call -- lock-protected only on its own side -- could land in that window, find the through model already registered, and repoint "source" at its own (different) model instance instead, leaving the through's FK and whatever get_model() subsequently caches pointing at two different classes for the same table. That produced the intermittent ValueError ("Cannot query 'X': Must be 'TableYModel' instance.") and RecursionError reported here (recurrence of #477). Wrapping the build+register+repoint sequence in the same global lock closes the gap. Added a deterministic regression test that forces a writer thread (create_polymorphic_m2m_table) and a reader thread (get_model) into the exact interleaving via a mocked apps.register_model(), rather than relying on real thread-scheduling luck to land inside the race window. Co-Authored-By: Claude Sonnet 5 --- netbox_custom_objects/field_types.py | 46 +++- netbox_custom_objects/tests/test_deletion.py | 254 +++++++++++++++++++ 2 files changed, 286 insertions(+), 14 deletions(-) diff --git a/netbox_custom_objects/field_types.py b/netbox_custom_objects/field_types.py index c4065779..52bab95b 100644 --- a/netbox_custom_objects/field_types.py +++ b/netbox_custom_objects/field_types.py @@ -1787,23 +1787,41 @@ def create_polymorphic_m2m_table(self, field_instance, model, schema_editor): ``with connection.schema_editor()`` here would flush deferred SQL prematurely on PostgreSQL. """ + from netbox_custom_objects.models import CustomObjectType # noqa: PLC0415 + source_model_string = f"{APP_LABEL}.{model.__name__}" - through = self.get_polymorphic_through_model(field_instance, source_model_string) - source_field = through._meta.get_field("source") - source_field.remote_field.model = model - source_field.related_model = model + # Serialized against CustomObjectType.get_model()'s own through-model + # reuse-or-create check (_after_model_generation runs under the same + # lock, held by its caller for the whole call). Without this, a + # concurrent reader regenerating this COT's model can observe this + # through model mid-construction here -- registered by Django's + # ModelBase metaclass inside generate_model() below, but before its + # "source" FK is repointed at `model` on the next line -- and race to + # point the registered class's FK at its OWN (different) model + # instance. Whichever thread's mutation and whichever thread's + # get_model() cache-write happen last aren't guaranteed to be the + # same thread, leaving the through's "source" FK and the cached model + # class mismatched. Confirmed live under concurrent load: + # intermittent "ValueError: Cannot query 'X': Must be 'TableYModel' + # instance." and RecursionError (issue #640). + with CustomObjectType._global_lock: + through = self.get_polymorphic_through_model(field_instance, source_model_string) + + source_field = through._meta.get_field("source") + source_field.remote_field.model = model + source_field.related_model = model - # Probe the same schema the DDL will target. schema_editor is branch-aware - # (opened via _get_schema_connection() by the caller), whereas the module-level - # ``connection`` always points at the main schema — using it here would let the - # idempotency guard diverge from where create_model() actually writes. - conn = schema_editor.connection - table_name = through._meta.db_table - with conn.cursor() as cursor: - existing_tables = conn.introspection.table_names(cursor) - if table_name not in existing_tables: - schema_editor.create_model(through) + # Probe the same schema the DDL will target. schema_editor is branch-aware + # (opened via _get_schema_connection() by the caller), whereas the module-level + # ``connection`` always points at the main schema — using it here would let the + # idempotency guard diverge from where create_model() actually writes. + conn = schema_editor.connection + table_name = through._meta.db_table + with conn.cursor() as cursor: + existing_tables = conn.introspection.table_names(cursor) + if table_name not in existing_tables: + schema_editor.create_model(through) def drop_polymorphic_m2m_table(self, field_instance, model, schema_editor): """Drops the DB table for a polymorphic MultiObject through. diff --git a/netbox_custom_objects/tests/test_deletion.py b/netbox_custom_objects/tests/test_deletion.py index b6c8d7bd..e8d619ea 100644 --- a/netbox_custom_objects/tests/test_deletion.py +++ b/netbox_custom_objects/tests/test_deletion.py @@ -6,12 +6,16 @@ lets us verify table-level changes and FK SET NULL/CASCADE/PROTECT behaviour that cannot be observed inside a rolled-back savepoint. """ +import threading + from django.apps import apps as django_apps from django.db import connection from django.db.utils import IntegrityError from django.test import TransactionTestCase +from core.models import ObjectType from dcim.models import Device, DeviceRole, DeviceType, Manufacturer, Site +from extras.choices import CustomFieldTypeChoices from netbox_custom_objects.choices import ObjectFieldOnDeleteChoices from netbox_custom_objects.constants import APP_LABEL from netbox_custom_objects.models import CustomObjectType, CustomObjectTypeField @@ -798,3 +802,253 @@ class than the one cached under the current timestamp, and the create() call obj_target = target_model.objects.create(name='Target Object') obj_source = source_model.objects.create(name='Source Object', ref_target=obj_target) self.assertEqual(obj_source.ref_target, obj_target) + + +class PolymorphicMultiObjectConcurrencyTestCase(TransactionCleanupMixin, CustomObjectsTestCase, TransactionTestCase): + """ + Regression tests for issue #640: concurrent regeneration of a COT with a + polymorphic multiobject field could register two competing through-model + classes for the same name, leaving a stale "source" FK reference that later + surfaced as "ValueError: Cannot query 'X': Must be 'TableYModel' instance." + (the same symptom class as #477/#483) or a RecursionError, depending on + which thread's registration "won". Confirmed live under a real multi-threaded + gunicorn worker; reproducing the exact race deterministically in-process isn't + feasible, so this drives many genuinely concurrent get_model() calls through + the same code path and asserts the result is always self-consistent. + """ + + def setUp(self): + super().setUp() + self.site_ot = ObjectType.objects.get_for_model(Site) + + def test_field_creation_racing_concurrent_readers_yields_consistent_through_model(self): + """ + Racing bare get_model() calls against each other (no I/O between the + LookupError check and the register_model() write) rarely lands in the + actual race window -- the critical section is nearly pure Python with no + GIL-releasing I/O, so threads rarely get preempted inside it. What + reproduced this reliably live (issue #640) was racing the polymorphic + field's *creation* -- which does real, GIL-releasing DB I/O across several + statements (INSERT the field row, then several more for + related_object_types.set()) -- against other threads continuously calling + get_model(), which is exactly the shape of "one request creates a field + while other requests are rendering unrelated pages" in production. + """ + cot = self.create_simple_custom_object_type(name='polyrace', slug='poly-race') + + stop = threading.Event() + reader_errors = [] + reader_errors_lock = threading.Lock() + + def reader(): + while not stop.is_set(): + try: + CustomObjectType.objects.get(pk=cot.pk).get_model(no_cache=True) + except Exception as e: # noqa: BLE001 - captured for the assertion below + with reader_errors_lock: + reader_errors.append(e) + finally: + connection.close() + + n_readers = 12 + readers = [threading.Thread(target=reader) for _ in range(n_readers)] + for t in readers: + t.start() + + try: + field = self.create_custom_object_type_field( + cot, + name='depends_on', + label='Depends On', + type='multiobject', + is_polymorphic=True, + ) + field.related_object_types.set([self.site_ot]) + finally: + stop.set() + for t in readers: + t.join() + + self.assertEqual( + reader_errors, [], + "concurrent get_model() calls must not raise while a polymorphic " + "multiobject field is being created", + ) + + # Self-consistency: whatever get_model() now returns must be the same class + # the registered through model's "source" FK actually points at -- a + # mismatch here is exactly the #477/#483-class staleness this guards against. + final_model = CustomObjectType.objects.get(pk=cot.pk).get_model() + through_model = django_apps.get_model(APP_LABEL, field.through_model_name) + source_field = through_model._meta.get_field('source') + self.assertIs( + source_field.remote_field.model, final_model, + "the registered through model's source FK must point at the model class " + "get_model() currently returns, not an orphaned duplicate from a losing thread", + ) + + # And that consistency must actually be usable: creating an instance and + # relating it through the polymorphic field, then deleting it, must not raise + # the #477/#483-class ValueError. + obj = final_model.objects.create(name='Instance 1') + obj.depends_on.set([Site.objects.create(name='Race Site', slug='race-site')]) + obj.delete() # Must not raise ValueError: "Cannot query ...: Must be ... instance." + + def test_forced_registration_interleaving_stays_consistent(self): + """ + Deterministic version of the race, forced via mocking rather than relying + on real thread-scheduling luck (which the test above showed rarely lands + inside the narrow window this actually depends on). + + The real #640 race is NOT two get_model() readers colliding with each + other -- CustomObjectType.get_model() already wraps its whole call to + _after_model_generation() in CustomObjectType._global_lock, so two + concurrent readers regenerating the same COT are already fully + serialized there, with or without any change targeting that function. + + The actual gap is on the *writer* side: + CustomFieldType.create_polymorphic_m2m_table() (called exactly once, + from CustomObjectTypeField.save(), when a new polymorphic multiobject + field is first created) builds a fresh through-model class, lets + Django's ModelBase metaclass register it, and only *afterwards* points + its "source" FK at the caller's model -- all with no lock at all. A + concurrent reader's get_model(no_cache=True) -- lock-protected only on + its own side -- can run in that exact window: it finds the writer's + through model already registered (via the metaclass) and immediately + repoints "source" at ITS OWN freshly-regenerated model class. Whichever + of the two threads mutates "source" last, and whichever one's + get_model() call caches its own model last, aren't guaranteed to be + the same thread -- so the registered through's "source" FK and + whatever get_model() now returns can end up pointing at two different + (if table-equivalent) Python classes. Confirmed live under concurrent + load: intermittent "ValueError: Cannot query 'X': Must be + 'TableYModel' instance." and RecursionError. + + This test forces exactly that interleaving: thread "W" plays the + writer (calling create_polymorphic_m2m_table() directly, as + CustomObjectTypeField.save() would), thread "R" plays the reader + (get_model(no_cache=True)). A mocked apps.register_model() hook pauses + W immediately after its metaclass-driven registration -- but *before* + W repoints "source" at its own model -- and only resumes W once R has + had its chance to run. With the #640 fix, W's entire + create_polymorphic_m2m_table() body (including that registration) now + runs under CustomObjectType._global_lock, so R can't even start its + own lock-protected check until W's whole turn -- pause included -- + is over; the rendezvous below simply times out and W proceeds alone, + R correctly reuses W's finished result afterward. Without the fix, R + genuinely runs inside the pause and the two threads' "source" + FK/get_model() cache writes land in different orders, reliably + producing the mismatch this test asserts against. + """ + from unittest.mock import patch + + from netbox_custom_objects.field_types import FIELD_TYPE_CLASS + + cot = self.create_simple_custom_object_type(name='polyforce', slug='poly-force') + field = self.create_custom_object_type_field( + cot, + name='depends_on', + label='Depends On', + type='multiobject', + is_polymorphic=True, + ) + field.related_object_types.set([self.site_ot]) + + # The real table/through model already exist (created for real by + # create_custom_object_type_field() above via the normal save() path). + # Force the through model back to "unregistered" so a direct call to + # create_polymorphic_m2m_table() -- simulating field creation racing a + # concurrent reader, as CustomObjectTypeField.save() would trigger -- + # takes the same "build fresh, register, then repoint source" path a + # brand-new field's first save would. The physical table is left + # alone; create_polymorphic_m2m_table()'s own idempotency check will + # see it already exists and skip re-issuing the DDL. + writer_model = CustomObjectType.objects.get(pk=cot.pk).get_model() + CustomObjectType.clear_model_cache() + model_name_lower = field.through_model_name.lower() + del django_apps.all_models[APP_LABEL][model_name_lower] + django_apps.clear_cache() + + real_register_model = django_apps.register_model + reader_may_proceed = threading.Event() + reader_done = threading.Event() + gated = set() + + def ordered_register_model(app_label, model): + # Only intercept the through model under test; everything else + # (e.g. the reader's own source-model registration) is untouched. + if app_label != APP_LABEL or model.__name__ != field.through_model_name: + return real_register_model(app_label, model) + + # Only the FIRST call matters -- Django's ModelBase metaclass + # registers the model as soon as generate_model() builds it + # (inside get_polymorphic_through_model()); this is that call. + if 'seen' in gated: + return real_register_model(app_label, model) + gated.add('seen') + + result = real_register_model(app_label, model) + # The through model is now registered but W (the writer) hasn't + # yet repointed its "source" FK at writer_model -- give R (the + # reader) a chance to run right here. With the #640 fix, W is + # holding CustomObjectType._global_lock for this whole call, so R + # can't have even started its own check yet; this just times out + # and W proceeds immediately. + reader_may_proceed.set() + reader_done.wait(timeout=2) + return result + + writer_result = {} + + def run_writer(): + threading.current_thread().name = 'W' + field_type = FIELD_TYPE_CLASS[CustomFieldTypeChoices.TYPE_MULTIOBJECT]() + try: + with connection.schema_editor() as schema_editor: + field_type.create_polymorphic_m2m_table(field, writer_model, schema_editor) + except Exception as e: # noqa: BLE001 - surfaced via the assertion below + writer_result['error'] = e + finally: + connection.close() + + reader_result = {} + + def run_reader(): + threading.current_thread().name = 'R' + reader_may_proceed.wait(timeout=5) + try: + reader_result['model'] = CustomObjectType.objects.get(pk=cot.pk).get_model(no_cache=True) + except Exception as e: # noqa: BLE001 - surfaced via the assertion below + reader_result['error'] = e + finally: + reader_done.set() + connection.close() + + with patch.object(django_apps, 'register_model', side_effect=ordered_register_model): + t_w = threading.Thread(target=run_writer, name='W') + t_r = threading.Thread(target=run_reader, name='R') + t_w.start() + t_r.start() + t_w.join(timeout=10) + t_r.join(timeout=10) + + self.assertNotIn('error', writer_result, f"writer raised: {writer_result.get('error')!r}") + self.assertNotIn('error', reader_result, f"reader raised: {reader_result.get('error')!r}") + + # The invariant the fix establishes: whichever model get_model() now + # returns must be the one the registered through model's "source" FK + # actually points at. Without the #640 fix, this forced interleaving + # reliably produces a mismatch (reader's model cached, writer's model + # left on the through's "source" FK, or vice versa) every time. + final_model = CustomObjectType.objects.get(pk=cot.pk).get_model() + through_model = django_apps.get_model(APP_LABEL, field.through_model_name) + source_field = through_model._meta.get_field('source') + self.assertIs( + source_field.remote_field.model, final_model, + "the registered through model's source FK must point at the model class " + "get_model() currently returns -- a mismatch here is issue #640", + ) + + obj = final_model.objects.create(name='Instance 1') + obj.depends_on.set([Site.objects.create(name='Force Site', slug='force-site')]) + obj.delete() # Must not raise ValueError: "Cannot query ...: Must be ... instance." From 43c91341fc933db3731f6f0a46259d0d4fb889f6 Mon Sep 17 00:00:00 2001 From: Brian Tiemann Date: Fri, 7 Aug 2026 12:36:49 -0400 Subject: [PATCH 2/6] Move #640 concurrency tests to test_schema_operations.py and trim comments PolymorphicMultiObjectConcurrencyTestCase exercises through-model registration during polymorphic multiobject field creation (a schema operation), not deletion logic -- it only lived in test_deletion.py because the investigation started from the bug's delete-time symptom. Moved it next to the other schema-creation/registry tests it actually belongs with. Also trimmed the docstrings and inline comments, which had grown into multi-paragraph explanations restating the same points -- cut to the essential why (what's already locked, what isn't, and why the fixed case times out rather than deadlocking) without re-deriving the whole investigation inline. Co-Authored-By: Claude Sonnet 5 --- netbox_custom_objects/tests/test_deletion.py | 254 ------------------ .../tests/test_schema_operations.py | 210 ++++++++++++++- 2 files changed, 209 insertions(+), 255 deletions(-) diff --git a/netbox_custom_objects/tests/test_deletion.py b/netbox_custom_objects/tests/test_deletion.py index e8d619ea..b6c8d7bd 100644 --- a/netbox_custom_objects/tests/test_deletion.py +++ b/netbox_custom_objects/tests/test_deletion.py @@ -6,16 +6,12 @@ lets us verify table-level changes and FK SET NULL/CASCADE/PROTECT behaviour that cannot be observed inside a rolled-back savepoint. """ -import threading - from django.apps import apps as django_apps from django.db import connection from django.db.utils import IntegrityError from django.test import TransactionTestCase -from core.models import ObjectType from dcim.models import Device, DeviceRole, DeviceType, Manufacturer, Site -from extras.choices import CustomFieldTypeChoices from netbox_custom_objects.choices import ObjectFieldOnDeleteChoices from netbox_custom_objects.constants import APP_LABEL from netbox_custom_objects.models import CustomObjectType, CustomObjectTypeField @@ -802,253 +798,3 @@ class than the one cached under the current timestamp, and the create() call obj_target = target_model.objects.create(name='Target Object') obj_source = source_model.objects.create(name='Source Object', ref_target=obj_target) self.assertEqual(obj_source.ref_target, obj_target) - - -class PolymorphicMultiObjectConcurrencyTestCase(TransactionCleanupMixin, CustomObjectsTestCase, TransactionTestCase): - """ - Regression tests for issue #640: concurrent regeneration of a COT with a - polymorphic multiobject field could register two competing through-model - classes for the same name, leaving a stale "source" FK reference that later - surfaced as "ValueError: Cannot query 'X': Must be 'TableYModel' instance." - (the same symptom class as #477/#483) or a RecursionError, depending on - which thread's registration "won". Confirmed live under a real multi-threaded - gunicorn worker; reproducing the exact race deterministically in-process isn't - feasible, so this drives many genuinely concurrent get_model() calls through - the same code path and asserts the result is always self-consistent. - """ - - def setUp(self): - super().setUp() - self.site_ot = ObjectType.objects.get_for_model(Site) - - def test_field_creation_racing_concurrent_readers_yields_consistent_through_model(self): - """ - Racing bare get_model() calls against each other (no I/O between the - LookupError check and the register_model() write) rarely lands in the - actual race window -- the critical section is nearly pure Python with no - GIL-releasing I/O, so threads rarely get preempted inside it. What - reproduced this reliably live (issue #640) was racing the polymorphic - field's *creation* -- which does real, GIL-releasing DB I/O across several - statements (INSERT the field row, then several more for - related_object_types.set()) -- against other threads continuously calling - get_model(), which is exactly the shape of "one request creates a field - while other requests are rendering unrelated pages" in production. - """ - cot = self.create_simple_custom_object_type(name='polyrace', slug='poly-race') - - stop = threading.Event() - reader_errors = [] - reader_errors_lock = threading.Lock() - - def reader(): - while not stop.is_set(): - try: - CustomObjectType.objects.get(pk=cot.pk).get_model(no_cache=True) - except Exception as e: # noqa: BLE001 - captured for the assertion below - with reader_errors_lock: - reader_errors.append(e) - finally: - connection.close() - - n_readers = 12 - readers = [threading.Thread(target=reader) for _ in range(n_readers)] - for t in readers: - t.start() - - try: - field = self.create_custom_object_type_field( - cot, - name='depends_on', - label='Depends On', - type='multiobject', - is_polymorphic=True, - ) - field.related_object_types.set([self.site_ot]) - finally: - stop.set() - for t in readers: - t.join() - - self.assertEqual( - reader_errors, [], - "concurrent get_model() calls must not raise while a polymorphic " - "multiobject field is being created", - ) - - # Self-consistency: whatever get_model() now returns must be the same class - # the registered through model's "source" FK actually points at -- a - # mismatch here is exactly the #477/#483-class staleness this guards against. - final_model = CustomObjectType.objects.get(pk=cot.pk).get_model() - through_model = django_apps.get_model(APP_LABEL, field.through_model_name) - source_field = through_model._meta.get_field('source') - self.assertIs( - source_field.remote_field.model, final_model, - "the registered through model's source FK must point at the model class " - "get_model() currently returns, not an orphaned duplicate from a losing thread", - ) - - # And that consistency must actually be usable: creating an instance and - # relating it through the polymorphic field, then deleting it, must not raise - # the #477/#483-class ValueError. - obj = final_model.objects.create(name='Instance 1') - obj.depends_on.set([Site.objects.create(name='Race Site', slug='race-site')]) - obj.delete() # Must not raise ValueError: "Cannot query ...: Must be ... instance." - - def test_forced_registration_interleaving_stays_consistent(self): - """ - Deterministic version of the race, forced via mocking rather than relying - on real thread-scheduling luck (which the test above showed rarely lands - inside the narrow window this actually depends on). - - The real #640 race is NOT two get_model() readers colliding with each - other -- CustomObjectType.get_model() already wraps its whole call to - _after_model_generation() in CustomObjectType._global_lock, so two - concurrent readers regenerating the same COT are already fully - serialized there, with or without any change targeting that function. - - The actual gap is on the *writer* side: - CustomFieldType.create_polymorphic_m2m_table() (called exactly once, - from CustomObjectTypeField.save(), when a new polymorphic multiobject - field is first created) builds a fresh through-model class, lets - Django's ModelBase metaclass register it, and only *afterwards* points - its "source" FK at the caller's model -- all with no lock at all. A - concurrent reader's get_model(no_cache=True) -- lock-protected only on - its own side -- can run in that exact window: it finds the writer's - through model already registered (via the metaclass) and immediately - repoints "source" at ITS OWN freshly-regenerated model class. Whichever - of the two threads mutates "source" last, and whichever one's - get_model() call caches its own model last, aren't guaranteed to be - the same thread -- so the registered through's "source" FK and - whatever get_model() now returns can end up pointing at two different - (if table-equivalent) Python classes. Confirmed live under concurrent - load: intermittent "ValueError: Cannot query 'X': Must be - 'TableYModel' instance." and RecursionError. - - This test forces exactly that interleaving: thread "W" plays the - writer (calling create_polymorphic_m2m_table() directly, as - CustomObjectTypeField.save() would), thread "R" plays the reader - (get_model(no_cache=True)). A mocked apps.register_model() hook pauses - W immediately after its metaclass-driven registration -- but *before* - W repoints "source" at its own model -- and only resumes W once R has - had its chance to run. With the #640 fix, W's entire - create_polymorphic_m2m_table() body (including that registration) now - runs under CustomObjectType._global_lock, so R can't even start its - own lock-protected check until W's whole turn -- pause included -- - is over; the rendezvous below simply times out and W proceeds alone, - R correctly reuses W's finished result afterward. Without the fix, R - genuinely runs inside the pause and the two threads' "source" - FK/get_model() cache writes land in different orders, reliably - producing the mismatch this test asserts against. - """ - from unittest.mock import patch - - from netbox_custom_objects.field_types import FIELD_TYPE_CLASS - - cot = self.create_simple_custom_object_type(name='polyforce', slug='poly-force') - field = self.create_custom_object_type_field( - cot, - name='depends_on', - label='Depends On', - type='multiobject', - is_polymorphic=True, - ) - field.related_object_types.set([self.site_ot]) - - # The real table/through model already exist (created for real by - # create_custom_object_type_field() above via the normal save() path). - # Force the through model back to "unregistered" so a direct call to - # create_polymorphic_m2m_table() -- simulating field creation racing a - # concurrent reader, as CustomObjectTypeField.save() would trigger -- - # takes the same "build fresh, register, then repoint source" path a - # brand-new field's first save would. The physical table is left - # alone; create_polymorphic_m2m_table()'s own idempotency check will - # see it already exists and skip re-issuing the DDL. - writer_model = CustomObjectType.objects.get(pk=cot.pk).get_model() - CustomObjectType.clear_model_cache() - model_name_lower = field.through_model_name.lower() - del django_apps.all_models[APP_LABEL][model_name_lower] - django_apps.clear_cache() - - real_register_model = django_apps.register_model - reader_may_proceed = threading.Event() - reader_done = threading.Event() - gated = set() - - def ordered_register_model(app_label, model): - # Only intercept the through model under test; everything else - # (e.g. the reader's own source-model registration) is untouched. - if app_label != APP_LABEL or model.__name__ != field.through_model_name: - return real_register_model(app_label, model) - - # Only the FIRST call matters -- Django's ModelBase metaclass - # registers the model as soon as generate_model() builds it - # (inside get_polymorphic_through_model()); this is that call. - if 'seen' in gated: - return real_register_model(app_label, model) - gated.add('seen') - - result = real_register_model(app_label, model) - # The through model is now registered but W (the writer) hasn't - # yet repointed its "source" FK at writer_model -- give R (the - # reader) a chance to run right here. With the #640 fix, W is - # holding CustomObjectType._global_lock for this whole call, so R - # can't have even started its own check yet; this just times out - # and W proceeds immediately. - reader_may_proceed.set() - reader_done.wait(timeout=2) - return result - - writer_result = {} - - def run_writer(): - threading.current_thread().name = 'W' - field_type = FIELD_TYPE_CLASS[CustomFieldTypeChoices.TYPE_MULTIOBJECT]() - try: - with connection.schema_editor() as schema_editor: - field_type.create_polymorphic_m2m_table(field, writer_model, schema_editor) - except Exception as e: # noqa: BLE001 - surfaced via the assertion below - writer_result['error'] = e - finally: - connection.close() - - reader_result = {} - - def run_reader(): - threading.current_thread().name = 'R' - reader_may_proceed.wait(timeout=5) - try: - reader_result['model'] = CustomObjectType.objects.get(pk=cot.pk).get_model(no_cache=True) - except Exception as e: # noqa: BLE001 - surfaced via the assertion below - reader_result['error'] = e - finally: - reader_done.set() - connection.close() - - with patch.object(django_apps, 'register_model', side_effect=ordered_register_model): - t_w = threading.Thread(target=run_writer, name='W') - t_r = threading.Thread(target=run_reader, name='R') - t_w.start() - t_r.start() - t_w.join(timeout=10) - t_r.join(timeout=10) - - self.assertNotIn('error', writer_result, f"writer raised: {writer_result.get('error')!r}") - self.assertNotIn('error', reader_result, f"reader raised: {reader_result.get('error')!r}") - - # The invariant the fix establishes: whichever model get_model() now - # returns must be the one the registered through model's "source" FK - # actually points at. Without the #640 fix, this forced interleaving - # reliably produces a mismatch (reader's model cached, writer's model - # left on the through's "source" FK, or vice versa) every time. - final_model = CustomObjectType.objects.get(pk=cot.pk).get_model() - through_model = django_apps.get_model(APP_LABEL, field.through_model_name) - source_field = through_model._meta.get_field('source') - self.assertIs( - source_field.remote_field.model, final_model, - "the registered through model's source FK must point at the model class " - "get_model() currently returns -- a mismatch here is issue #640", - ) - - obj = final_model.objects.create(name='Instance 1') - obj.depends_on.set([Site.objects.create(name='Force Site', slug='force-site')]) - obj.delete() # Must not raise ValueError: "Cannot query ...: Must be ... instance." diff --git a/netbox_custom_objects/tests/test_schema_operations.py b/netbox_custom_objects/tests/test_schema_operations.py index 582cc9c6..fce5151d 100644 --- a/netbox_custom_objects/tests/test_schema_operations.py +++ b/netbox_custom_objects/tests/test_schema_operations.py @@ -4,15 +4,21 @@ Uses TransactionTestCase so DDL and on_commit callbacks behave exactly as they do in production (no wrapping savepoint prevents commits). """ +import threading from io import StringIO +from unittest.mock import patch from django.apps import apps from django.core.management import call_command from django.db import connection from django.test import TransactionTestCase +from core.models import ObjectType +from dcim.models import Site +from extras.choices import CustomFieldTypeChoices from netbox_custom_objects.constants import APP_LABEL -from netbox_custom_objects.models import CustomObjectTypeField +from netbox_custom_objects.field_types import FIELD_TYPE_CLASS +from netbox_custom_objects.models import CustomObjectType, CustomObjectTypeField from .base import CustomObjectsTestCase, TransactionCleanupMixin @@ -275,3 +281,205 @@ def test_coordinates_field_delete_drops_both_columns(self): columns = self._db_columns(cot.get_model()) self.assertNotIn('location_latitude', columns) self.assertNotIn('location_longitude', columns) + + +class PolymorphicMultiObjectConcurrencyTestCase(TransactionCleanupMixin, CustomObjectsTestCase, TransactionTestCase): + """ + Regression tests for issue #640: creating a polymorphic multiobject field + races registering its through-model class against a concurrent + get_model() call, producing a class-identity mismatch that later surfaces + as "ValueError: Cannot query 'X': Must be 'TableYModel' instance." or a + RecursionError (same symptom class as #477/#483). + """ + + def setUp(self): + super().setUp() + self.site_ot = ObjectType.objects.get_for_model(Site) + + def test_field_creation_racing_concurrent_readers_yields_consistent_through_model(self): + """ + Races field *creation* (real DB I/O) against 12 looping get_model() + readers -- the shape that reproduced #640 live. Rarely lands inside + the actual race window in-process (see the deterministic version + below), but exercises the same code path under real concurrency. + """ + cot = self.create_simple_custom_object_type(name='polyrace', slug='poly-race') + + stop = threading.Event() + reader_errors = [] + reader_errors_lock = threading.Lock() + + def reader(): + while not stop.is_set(): + try: + CustomObjectType.objects.get(pk=cot.pk).get_model(no_cache=True) + except Exception as e: # noqa: BLE001 - captured for the assertion below + with reader_errors_lock: + reader_errors.append(e) + finally: + connection.close() + + n_readers = 12 + readers = [threading.Thread(target=reader) for _ in range(n_readers)] + for t in readers: + t.start() + + try: + field = self.create_custom_object_type_field( + cot, + name='depends_on', + label='Depends On', + type='multiobject', + is_polymorphic=True, + ) + field.related_object_types.set([self.site_ot]) + finally: + stop.set() + for t in readers: + t.join() + + self.assertEqual( + reader_errors, [], + "concurrent get_model() calls must not raise while a polymorphic " + "multiobject field is being created", + ) + + # get_model() and the through model's "source" FK must agree on which + # class is canonical -- a mismatch is the #477/#483-class staleness. + final_model = CustomObjectType.objects.get(pk=cot.pk).get_model() + through_model = apps.get_model(APP_LABEL, field.through_model_name) + source_field = through_model._meta.get_field('source') + self.assertIs( + source_field.remote_field.model, final_model, + "the registered through model's source FK must point at the model class " + "get_model() currently returns, not an orphaned duplicate from a losing thread", + ) + + obj = final_model.objects.create(name='Instance 1') + obj.depends_on.set([Site.objects.create(name='Race Site', slug='race-site')]) + obj.delete() # Must not raise ValueError: "Cannot query ...: Must be ... instance." + + def test_forced_registration_interleaving_stays_consistent(self): + """ + Deterministic version of the same race, forced via mocking instead of + relying on thread-scheduling luck. + + get_model() already wraps _after_model_generation() in + CustomObjectType._global_lock, so two concurrent readers can't race + each other there. The actual gap is the *writer*: + create_polymorphic_m2m_table() (called once, from + CustomObjectTypeField.save(), when a polymorphic multiobject field is + first created) registers its through-model class via Django's + metaclass, then repoints its "source" FK -- all without that lock. A + concurrent reader can land in between: it finds the through model + already registered and repoints "source" at its own model instead, + so the through's FK and get_model()'s cache can end up pointing at + two different classes. + + Thread "W" plays the writer (create_polymorphic_m2m_table() + directly), thread "R" the reader (get_model()). A mocked + register_model() pauses W right after registration but before it + repoints "source", giving R a window to run. With the fix, W holds + _global_lock for that whole call, so R can't even start until W is + done -- the pause below just times out harmlessly. Without the fix, + R runs inside the pause and the two threads' writes land in + different orders, reliably producing the mismatch asserted below. + """ + cot = self.create_simple_custom_object_type(name='polyforce', slug='poly-force') + field = self.create_custom_object_type_field( + cot, + name='depends_on', + label='Depends On', + type='multiobject', + is_polymorphic=True, + ) + field.related_object_types.set([self.site_ot]) + + # The table/through model already exist (created for real above via + # the normal save() path). Force the through model back to + # "unregistered" so a direct create_polymorphic_m2m_table() call + # takes the same build-register-repoint path a brand-new field's + # first save would; create_polymorphic_m2m_table()'s own idempotency + # check will see the physical table already exists and skip the DDL. + writer_model = CustomObjectType.objects.get(pk=cot.pk).get_model() + CustomObjectType.clear_model_cache() + model_name_lower = field.through_model_name.lower() + del apps.all_models[APP_LABEL][model_name_lower] + apps.clear_cache() + + real_register_model = apps.register_model + reader_may_proceed = threading.Event() + reader_done = threading.Event() + gated = set() + + def ordered_register_model(app_label, model): + # Only intercept the through model under test. + if app_label != APP_LABEL or model.__name__ != field.through_model_name: + return real_register_model(app_label, model) + # Only the first call matters (Django's metaclass registers the + # model as soon as it's built; a harmless explicit re-registration + # follows immediately after in the real code). + if 'seen' in gated: + return real_register_model(app_label, model) + gated.add('seen') + + result = real_register_model(app_label, model) + # Registered, but "source" isn't repointed at writer_model yet -- + # give R a window here. With the fix, W holds _global_lock for + # this whole call, so R can't have started yet and this times out. + reader_may_proceed.set() + reader_done.wait(timeout=2) + return result + + writer_result = {} + + def run_writer(): + threading.current_thread().name = 'W' + field_type = FIELD_TYPE_CLASS[CustomFieldTypeChoices.TYPE_MULTIOBJECT]() + try: + with connection.schema_editor() as schema_editor: + field_type.create_polymorphic_m2m_table(field, writer_model, schema_editor) + except Exception as e: # noqa: BLE001 - surfaced via the assertion below + writer_result['error'] = e + finally: + connection.close() + + reader_result = {} + + def run_reader(): + threading.current_thread().name = 'R' + reader_may_proceed.wait(timeout=5) + try: + reader_result['model'] = CustomObjectType.objects.get(pk=cot.pk).get_model(no_cache=True) + except Exception as e: # noqa: BLE001 - surfaced via the assertion below + reader_result['error'] = e + finally: + reader_done.set() + connection.close() + + with patch.object(apps, 'register_model', side_effect=ordered_register_model): + t_w = threading.Thread(target=run_writer, name='W') + t_r = threading.Thread(target=run_reader, name='R') + t_w.start() + t_r.start() + t_w.join(timeout=10) + t_r.join(timeout=10) + + self.assertNotIn('error', writer_result, f"writer raised: {writer_result.get('error')!r}") + self.assertNotIn('error', reader_result, f"reader raised: {reader_result.get('error')!r}") + + # Without the fix, this reliably produces a mismatch: reader's model + # cached while writer's model is left on the through's "source" FK, + # or vice versa. + final_model = CustomObjectType.objects.get(pk=cot.pk).get_model() + through_model = apps.get_model(APP_LABEL, field.through_model_name) + source_field = through_model._meta.get_field('source') + self.assertIs( + source_field.remote_field.model, final_model, + "the registered through model's source FK must point at the model class " + "get_model() currently returns -- a mismatch here is issue #640", + ) + + obj = final_model.objects.create(name='Instance 1') + obj.depends_on.set([Site.objects.create(name='Force Site', slug='force-site')]) + obj.delete() # Must not raise ValueError: "Cannot query ...: Must be ... instance." From fc492cf45194163e6cdb6bba1f6a87537c7a04dd Mon Sep 17 00:00:00 2001 From: Brian Tiemann Date: Tue, 11 Aug 2026 12:18:05 -0400 Subject: [PATCH 3/6] Reduce forced-timeout duration in the #640 concurrency test reader_done.wait(timeout=2) inside ordered_register_model always times out with the fix applied (R is blocked on _global_lock and can never signal it), taxing every CI run by a flat 2 seconds. The duration only bounds an unavoidable wait; correctness doesn't depend on it, since R's ability to run concurrently is decided by lock state, not by wall-clock timing. Confirmed via 5 runs each way: cutting it to 0.5s still passes reliably with the fix and still fails reliably without it. Co-Authored-By: Claude Sonnet 5 --- netbox_custom_objects/tests/test_schema_operations.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/netbox_custom_objects/tests/test_schema_operations.py b/netbox_custom_objects/tests/test_schema_operations.py index fce5151d..bcbce260 100644 --- a/netbox_custom_objects/tests/test_schema_operations.py +++ b/netbox_custom_objects/tests/test_schema_operations.py @@ -426,9 +426,16 @@ def ordered_register_model(app_label, model): result = real_register_model(app_label, model) # Registered, but "source" isn't repointed at writer_model yet -- # give R a window here. With the fix, W holds _global_lock for - # this whole call, so R can't have started yet and this times out. + # this whole call, so R can't have started yet and this always + # times out rather than being signalled -- R can't reach + # reader_done.set() until W releases the lock, which doesn't + # happen until this wait returns. The duration only bounds how + # long that unavoidable wait lasts; it has no bearing on + # correctness (R's ability to run concurrently here is decided + # by lock state, not by wall-clock timing), so keep it short to + # avoid taxing every CI run by a fixed 2s. reader_may_proceed.set() - reader_done.wait(timeout=2) + reader_done.wait(timeout=0.5) return result writer_result = {} From 145feb390c60f2b0e823d401c10157699af5f0fd Mon Sep 17 00:00:00 2001 From: Brian Tiemann Date: Thu, 13 Aug 2026 20:39:30 -0400 Subject: [PATCH 4/6] Address round-2 review comments from Martin on PR #648 * test_forced_registration_interleaving_stays_consistent: assert both threads actually completed after join(), rather than letting a join() timeout silently leave the result dicts empty and the assertions below vacuously pass. * Cover the delete-confirmation GET (issue #640, step 4): obj.delete() realigns each through's "source" FK to type(self) before Django's collector runs, which would silently paper over a lingering registry mismatch that a plain GET -- the actual reported UI path -- does not repair. * Add a regression through the public field-save path (CustomObjectTypeField.objects.create()) with the reported two-type Custom Object setup, instead of only ever starting from an already-persisted field and calling create_polymorphic_m2m_table() directly. A deterministic (mocked apps.register_model()) version of this specific scenario was attempted and abandoned after it produced a genuine deadlock in testing: two threads targeting the identical through table can block each other at the Postgres DDL level while also contending for CustomObjectType._global_lock. Real thread-scheduling concurrency, exercised via 12 looping readers (mirroring the existing single-type test), reaches the same code path safely. --- .../tests/test_schema_operations.py | 114 ++++++++++++++++++ 1 file changed, 114 insertions(+) diff --git a/netbox_custom_objects/tests/test_schema_operations.py b/netbox_custom_objects/tests/test_schema_operations.py index bcbce260..af45a6c2 100644 --- a/netbox_custom_objects/tests/test_schema_operations.py +++ b/netbox_custom_objects/tests/test_schema_operations.py @@ -9,9 +9,11 @@ from unittest.mock import patch from django.apps import apps +from django.contrib.contenttypes.models import ContentType from django.core.management import call_command from django.db import connection from django.test import TransactionTestCase +from django.urls import reverse from core.models import ObjectType from dcim.models import Site @@ -19,6 +21,7 @@ from netbox_custom_objects.constants import APP_LABEL from netbox_custom_objects.field_types import FIELD_TYPE_CLASS from netbox_custom_objects.models import CustomObjectType, CustomObjectTypeField +from users.models import ObjectPermission from .base import CustomObjectsTestCase, TransactionCleanupMixin @@ -472,6 +475,12 @@ def run_reader(): t_w.join(timeout=10) t_r.join(timeout=10) + # A join() timeout leaves the result dicts empty rather than raising, so without these + # checks a hung thread could silently make the assertions below vacuously pass -- e.g. a + # writer that never finished never reaches the mismatch-inducing repoint at all. + self.assertFalse(t_w.is_alive(), "writer thread did not complete within the join timeout") + self.assertFalse(t_r.is_alive(), "reader thread did not complete within the join timeout") + self.assertNotIn('error', writer_result, f"writer raised: {writer_result.get('error')!r}") self.assertNotIn('error', reader_result, f"reader raised: {reader_result.get('error')!r}") @@ -489,4 +498,109 @@ def run_reader(): obj = final_model.objects.create(name='Instance 1') obj.depends_on.set([Site.objects.create(name='Force Site', slug='force-site')]) + + # The delete-confirmation GET is the reported UI path (issue #640, step 4): obj.delete() + # below realigns each through's "source" FK to type(self) before Django's collector runs + # (see CustomObject.delete()), which would silently paper over a lingering registry + # mismatch. A GET here never calls delete() at all, so it exercises the raw, unrepaired + # state directly -- exactly what crashed with "ValueError: Cannot query ...: Must be ... + # instance." in the original report, and what the class-identity assertion above cannot + # by itself confirm is actually reachable through the UI. + content_type = ContentType.objects.get_for_model(final_model) + obj_perm = ObjectPermission(name='poly-force-delete-view', actions=['view', 'delete']) + obj_perm.save() + obj_perm.users.add(self.user) + obj_perm.object_types.add(content_type) + delete_url = reverse( + 'plugins:netbox_custom_objects:customobject_delete', + kwargs={'custom_object_type': cot.slug, 'pk': obj.pk}, + ) + response = self.client.get(delete_url) + self.assertEqual( + response.status_code, 200, + f"delete-confirmation GET must render, not crash (got {response.status_code})", + ) + + obj.delete() # Must not raise ValueError: "Cannot query ...: Must be ... instance." + + def test_field_creation_via_public_save_path_with_two_type_setup_yields_consistent_through_model(self): + """ + Variant of test_field_creation_racing_concurrent_readers_yields_consistent_through_model + above, using the reported two-type Custom Object setup (a self-reference plus a second, + genuine Custom Object Type -- not just a single core dcim model) instead of one type, to + match the exact reported repro. Goes through the public field-save path + (CustomObjectTypeField.objects.create()) throughout, with no private-helper shortcut. + + A deterministic, forced-interleaving version of *this specific* scenario (two threads + both calling CustomObjectTypeField.objects.create() for the identical (name, + custom_object_type) at once, paused via the same mocked apps.register_model() technique + as test_forced_registration_interleaving_stays_consistent) was attempted and abandoned: + it can genuinely deadlock rather than just race. Both threads target the same physical + through table, so the second thread's CREATE TABLE blocks at the Postgres level on the + first thread's still-open transaction; CustomObjectType._global_lock is held by the first + thread across that same window (with the fix in place); and if anything downstream in the + first thread's own save() needs that lock again (e.g. a signal handler calling + get_model()), neither thread can make progress -- confirmed by hanging an actual test + run. Real thread-scheduling luck, exercised here instead via 12 looping readers (matching + the existing single-type test above), cannot deadlock this way: no reader ever holds + transaction.atomic() open across a paused lock acquisition. + """ + cot = self.create_simple_custom_object_type(name='polypublic', slug='poly-public') + other_cot = self.create_simple_custom_object_type(name='polypublicother', slug='poly-public-other') + self_ot = ObjectType.objects.get_for_model(cot.get_model()) + other_ot = ObjectType.objects.get_for_model(other_cot.get_model()) + + stop = threading.Event() + reader_errors = [] + reader_errors_lock = threading.Lock() + + def reader(): + while not stop.is_set(): + try: + CustomObjectType.objects.get(pk=cot.pk).get_model(no_cache=True) + except Exception as e: # noqa: BLE001 - captured for the assertion below + with reader_errors_lock: + reader_errors.append(e) + finally: + connection.close() + + n_readers = 12 + readers = [threading.Thread(target=reader) for _ in range(n_readers)] + for t in readers: + t.start() + + try: + field = CustomObjectTypeField.objects.create( + custom_object_type=cot, + name='depends_on', + label='Depends On', + type=CustomFieldTypeChoices.TYPE_MULTIOBJECT, + is_polymorphic=True, + ) + field.related_object_types.set([self_ot, other_ot]) + finally: + stop.set() + for t in readers: + t.join() + + self.assertEqual( + reader_errors, [], + "concurrent get_model() calls must not raise while a polymorphic multiobject field " + "with the reported two-type setup is being created", + ) + self.assertEqual(set(field.related_object_types.all()), {self_ot, other_ot}) + + # get_model() and the through model's "source" FK must agree on which class is canonical + # -- a mismatch is the #477/#483-class staleness that issue #640 reported. + final_model = CustomObjectType.objects.get(pk=cot.pk).get_model() + through_model = apps.get_model(APP_LABEL, field.through_model_name) + source_field = through_model._meta.get_field('source') + self.assertIs( + source_field.remote_field.model, final_model, + "the registered through model's source FK must point at the model class " + "get_model() currently returns, not an orphaned duplicate from a losing thread", + ) + + obj = final_model.objects.create(name='Instance 1') + obj.depends_on.set([Site.objects.create(name='Public Race Site', slug='public-race-site')]) obj.delete() # Must not raise ValueError: "Cannot query ...: Must be ... instance." From 77fa2eea89c907469795bd628fb7934ccd93d1d3 Mon Sep 17 00:00:00 2001 From: Brian Tiemann Date: Fri, 14 Aug 2026 08:19:06 -0400 Subject: [PATCH 5/6] Narrow CustomObjectType._global_lock to avoid a real deadlock (PR #648) create_polymorphic_m2m_table() held _global_lock across both the build+register+repoint step AND the table-existence probe/DDL. A concurrent CustomObjectTypeField.save() for the same field also calls CustomObjectType.clear_model_cache(), which acquires this same lock: if the lock stayed held across schema_editor.create_model() (an uncommitted CREATE TABLE inside this save()'s own transaction), a second thread blocked on the lock -- itself stuck at the Postgres level waiting on the first thread's uncommitted transaction for the same physical table -- would prevent the first thread from ever reaching clear_model_cache() to commit. Neither side could then make progress. Scope the lock to just the build+register+repoint step; release it before the table-existence probe/DDL runs. Confirmed via a new regression test (two threads double-submitting field creation for the identical (custom_object_type, name)): hangs against the previous, wider-scoped lock (reproduced the exact deadlock signature in pg_stat_activity -- one thread idle-in-transaction waiting on the lock, the other actively blocked on Postgres waiting for the first's uncommitted CREATE TABLE), completes in ~1.5s with the fix. --- netbox_custom_objects/field_types.py | 31 +++++++--- .../tests/test_schema_operations.py | 62 ++++++++++++++++++- 2 files changed, 82 insertions(+), 11 deletions(-) diff --git a/netbox_custom_objects/field_types.py b/netbox_custom_objects/field_types.py index 52bab95b..bac77e60 100644 --- a/netbox_custom_objects/field_types.py +++ b/netbox_custom_objects/field_types.py @@ -1805,6 +1805,17 @@ def create_polymorphic_m2m_table(self, field_instance, model, schema_editor): # class mismatched. Confirmed live under concurrent load: # intermittent "ValueError: Cannot query 'X': Must be 'TableYModel' # instance." and RecursionError (issue #640). + # + # Deliberately scoped to just the build+register+repoint above -- NOT the + # table-existence probe/DDL below. A concurrent CustomObjectTypeField.save() for the + # same field also calls CustomObjectType.clear_model_cache(), which acquires this same + # lock; if the lock stayed held across schema_editor.create_model() (an uncommitted + # CREATE TABLE inside this save()'s own transaction), a second thread blocked here + # waiting for the lock -- itself stuck at the Postgres level waiting on the first + # thread's uncommitted transaction for the same physical table -- would prevent the + # first thread from ever reaching clear_model_cache() to commit. Releasing the lock + # before the DDL avoids that deadlock; the DDL itself has no equivalent staleness + # window to guard (the "source" FK is already correctly repointed by the time it runs). with CustomObjectType._global_lock: through = self.get_polymorphic_through_model(field_instance, source_model_string) @@ -1812,16 +1823,16 @@ def create_polymorphic_m2m_table(self, field_instance, model, schema_editor): source_field.remote_field.model = model source_field.related_model = model - # Probe the same schema the DDL will target. schema_editor is branch-aware - # (opened via _get_schema_connection() by the caller), whereas the module-level - # ``connection`` always points at the main schema — using it here would let the - # idempotency guard diverge from where create_model() actually writes. - conn = schema_editor.connection - table_name = through._meta.db_table - with conn.cursor() as cursor: - existing_tables = conn.introspection.table_names(cursor) - if table_name not in existing_tables: - schema_editor.create_model(through) + # Probe the same schema the DDL will target. schema_editor is branch-aware + # (opened via _get_schema_connection() by the caller), whereas the module-level + # ``connection`` always points at the main schema — using it here would let the + # idempotency guard diverge from where create_model() actually writes. + conn = schema_editor.connection + table_name = through._meta.db_table + with conn.cursor() as cursor: + existing_tables = conn.introspection.table_names(cursor) + if table_name not in existing_tables: + schema_editor.create_model(through) def drop_polymorphic_m2m_table(self, field_instance, model, schema_editor): """Drops the DB table for a polymorphic MultiObject through. diff --git a/netbox_custom_objects/tests/test_schema_operations.py b/netbox_custom_objects/tests/test_schema_operations.py index 74dd52eb..de29d9f3 100644 --- a/netbox_custom_objects/tests/test_schema_operations.py +++ b/netbox_custom_objects/tests/test_schema_operations.py @@ -11,7 +11,7 @@ from django.apps import apps from django.contrib.contenttypes.models import ContentType from django.core.management import call_command -from django.db import connection +from django.db import IntegrityError, connection from django.test import TransactionTestCase from django.urls import reverse @@ -523,6 +523,66 @@ def run_reader(): obj.delete() # Must not raise ValueError: "Cannot query ...: Must be ... instance." + def test_concurrent_double_submit_does_not_deadlock(self): + """ + Two threads independently calling CustomObjectTypeField.objects.create() for the + identical (custom_object_type, name) at once -- a genuinely reachable scenario (e.g. a + retried request, or a doubly-clicked "save" button) -- must not deadlock. + + This is a real deadlock, not just a slow race, when CustomObjectType._global_lock spans + create_polymorphic_m2m_table()'s DDL: both threads build+register a through model for the + SAME physical table before either knows which one will win the (name, custom_object_type) + UniqueConstraint, so whichever thread's schema_editor.create_model() runs second blocks at + the Postgres level waiting for the first thread's uncommitted CREATE TABLE (same table + name) to resolve. If the first thread still needs the *same* Python lock afterward (its + own save() calls CustomObjectType.clear_model_cache(), which acquires it) before it can + commit and release that Postgres-level wait, neither thread can make progress. Confirmed + empirically: this exact scenario hung a live test run before the lock was narrowed to + cover only the build+register+repoint step, not the DDL. + """ + cot = self.create_simple_custom_object_type(name='doublesubmit', slug='double-submit') + self_ot = ObjectType.objects.get_for_model(cot.get_model()) + + results = {} + + def create_field(key): + threading.current_thread().name = key + try: + field = CustomObjectTypeField.objects.create( + custom_object_type=cot, + name='depends_on', + label='Depends On', + type=CustomFieldTypeChoices.TYPE_MULTIOBJECT, + is_polymorphic=True, + ) + field.related_object_types.set([self_ot, self.site_ot]) + results[key] = {'field': field} + except IntegrityError as e: + # Expected for exactly one of the two: the (name, custom_object_type) + # UniqueConstraint has only one winner. + results[key] = {'integrity_error': e} + except Exception as e: # noqa: BLE001 - surfaced via the assertion below + results[key] = {'error': e} + finally: + connection.close() + + t_a = threading.Thread(target=create_field, args=('A',), name='A') + t_b = threading.Thread(target=create_field, args=('B',), name='B') + t_a.start() + t_b.start() + t_a.join(timeout=15) + t_b.join(timeout=15) + + self.assertFalse(t_a.is_alive(), "thread A did not complete within the join timeout (deadlocked?)") + self.assertFalse(t_b.is_alive(), "thread B did not complete within the join timeout (deadlocked?)") + for key, result in results.items(): + self.assertNotIn('error', result, f"thread {key} raised an unexpected error: {result.get('error')!r}") + + succeeded = [key for key, result in results.items() if 'field' in result] + failed = [key for key, result in results.items() if 'integrity_error' in result] + self.assertEqual(len(succeeded), 1, f"expected exactly one winner: {results!r}") + self.assertEqual(len(failed), 1, f"expected exactly one IntegrityError: {results!r}") + def test_field_creation_via_public_save_path_with_two_type_setup_yields_consistent_through_model(self): """ Variant of test_field_creation_racing_concurrent_readers_yields_consistent_through_model From 7a4787899435880e3154c088d8ac38e7cbf87382 Mon Sep 17 00:00:00 2001 From: Brian Tiemann Date: Fri, 14 Aug 2026 13:33:49 -0400 Subject: [PATCH 6/6] Update remaining #640 references to #658; shorten a now-inaccurate test docstring The registration-before-repoint race and its regression test comments were still citing #640 (the unreproduced report this PR doesn't fix) instead of #658 (the actual bug this PR fixes and closes). Left the one reference to #640 that correctly attributes the delete-confirmation-GET test coverage to that issue's own numbered reproduction steps, which #658 doesn't have. Also corrected test_forced_registration_interleaving_stays_consistent's docstring: it described _global_lock as held "for that whole call," which was true before the lock was narrowed to stop before the table-existence probe/DDL. Shortened to describe only what the test itself asserts, with a pointer to #658 for the full analysis. --- netbox_custom_objects/field_types.py | 2 +- .../tests/test_schema_operations.py | 18 ++++++++++-------- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/netbox_custom_objects/field_types.py b/netbox_custom_objects/field_types.py index bac77e60..4be053d1 100644 --- a/netbox_custom_objects/field_types.py +++ b/netbox_custom_objects/field_types.py @@ -1804,7 +1804,7 @@ def create_polymorphic_m2m_table(self, field_instance, model, schema_editor): # same thread, leaving the through's "source" FK and the cached model # class mismatched. Confirmed live under concurrent load: # intermittent "ValueError: Cannot query 'X': Must be 'TableYModel' - # instance." and RecursionError (issue #640). + # instance." and RecursionError (issue #658). # # Deliberately scoped to just the build+register+repoint above -- NOT the # table-existence probe/DDL below. A concurrent CustomObjectTypeField.save() for the diff --git a/netbox_custom_objects/tests/test_schema_operations.py b/netbox_custom_objects/tests/test_schema_operations.py index de29d9f3..3099b1b8 100644 --- a/netbox_custom_objects/tests/test_schema_operations.py +++ b/netbox_custom_objects/tests/test_schema_operations.py @@ -288,7 +288,7 @@ def test_coordinates_field_delete_drops_both_columns(self): class PolymorphicMultiObjectConcurrencyTestCase(TransactionCleanupMixin, CustomObjectsTestCase, TransactionTestCase): """ - Regression tests for issue #640: creating a polymorphic multiobject field + Regression tests for issue #658: creating a polymorphic multiobject field races registering its through-model class against a concurrent get_model() call, producing a class-identity mismatch that later surfaces as "ValueError: Cannot query 'X': Must be 'TableYModel' instance." or a @@ -302,7 +302,7 @@ def setUp(self): def test_field_creation_racing_concurrent_readers_yields_consistent_through_model(self): """ Races field *creation* (real DB I/O) against 12 looping get_model() - readers -- the shape that reproduced #640 live. Rarely lands inside + readers -- the shape that reproduced #658 live. Rarely lands inside the actual race window in-process (see the deterministic version below), but exercises the same code path under real concurrency. """ @@ -383,10 +383,12 @@ def test_forced_registration_interleaving_stays_consistent(self): directly), thread "R" the reader (get_model()). A mocked register_model() pauses W right after registration but before it repoints "source", giving R a window to run. With the fix, W holds - _global_lock for that whole call, so R can't even start until W is - done -- the pause below just times out harmlessly. Without the fix, - R runs inside the pause and the two threads' writes land in - different orders, reliably producing the mismatch asserted below. + _global_lock across that build+register+repoint step, so R can't + start until W has repointed "source" -- the pause below just times + out harmlessly. Without the fix, R runs inside the pause and the two + threads' writes land in different orders, reliably producing the + mismatch asserted below. See #658 for the full analysis, including + why the lock can't simply span the rest of the call too. """ cot = self.create_simple_custom_object_type(name='polyforce', slug='poly-force') field = self.create_custom_object_type_field( @@ -493,7 +495,7 @@ def run_reader(): self.assertIs( source_field.remote_field.model, final_model, "the registered through model's source FK must point at the model class " - "get_model() currently returns -- a mismatch here is issue #640", + "get_model() currently returns -- a mismatch here is issue #658", ) obj = final_model.objects.create(name='Instance 1') @@ -651,7 +653,7 @@ def reader(): self.assertEqual(set(field.related_object_types.all()), {self_ot, other_ot}) # get_model() and the through model's "source" FK must agree on which class is canonical - # -- a mismatch is the #477/#483-class staleness that issue #640 reported. + # -- a mismatch is the #477/#483-class staleness that issue #658 reported. final_model = CustomObjectType.objects.get(pk=cot.pk).get_model() through_model = apps.get_model(APP_LABEL, field.through_model_name) source_field = through_model._meta.get_field('source')