Closes #658: Harden polymorphic multiobject through-model registration against concurrent creation/reads

[bctiemann]

Closes: #658

Related to: #640

Scope (updated per review)

This PR does not close #640. I investigated whether it does and could not reproduce #640's reported symptom (a deterministic FieldDoesNotExist on a fresh, single, unconcurrent process) on current main -- see "Investigation of #640" below. This PR instead fixes two independently real, verified concurrency bugs in the same function that I found while investigating (now filed separately as #658), and is scoped as registry-race hardening. #640 remains open pending further reproduction details.

Summary

• MultiObjectFieldType.create_polymorphic_m2m_table() (called once, when a polymorphic multiobject field is first created) built and registered a through-model class with Django's app registry, and only afterward repointed its source FK at the caller's model class -- 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, but table-equivalent) model instance instead -- leaving the through's FK and whatever get_model() subsequently caches pointing at two different Python classes for the same table. Fixed by holding _global_lock across the build+register+repoint sequence.
• That same lock, if held across the table-existence probe/DDL that follows, creates a different real deadlock: two threads racing to create the same field (e.g. a retried request) each build+register a through model for the same physical table before either knows which one will win the (name, custom_object_type) UniqueConstraint. Whichever thread's schema_editor.create_model() runs second blocks at the Postgres level waiting on the first thread's uncommitted CREATE TABLE (same table name) to resolve -- but the first thread's own save() needs _global_lock again in clear_model_cache() before it can commit and release that wait. Fixed by narrowing the lock to stop before the DDL.

Investigation of #640

Per review, I tried to reproduce #640's actual reported symptom directly: a fresh, single Python process, no concurrency, self-referencing polymorphic multiobject field with 2-3 related types, FieldDoesNotExist/model missing the field on the very first get_model() call. I could not reproduce it on current main:

• Created a COT with a self-referencing 3-type polymorphic multiobject field in one process, then in a separate, fresh process (no shared in-memory state) issued real GET requests against both the object detail page and the delete-confirmation page (the exact reported repro steps) -- both returned 200, no crash, both with and without this PR's lock fix.
• model._meta.get_fields() does not list depends_on for a polymorphic multiobject field regardless -- these fields are implemented as a plain descriptor (PolymorphicM2MDescriptor), not a real Django Field, so they were never expected to appear there. That's consistent with the snippet in Deleting a Custom Object with a multiobject field raises ValueError (recurrence of #477 in v0.6.0) #640's own report, but isn't itself evidence of breakage (obj.depends_on.set(...) and the actual views work fine via descriptor access).

Since main has moved substantially since v0.6.0 (the version #640 was filed against) via other fixes, it's plausible #640's actual root cause was already resolved by an unrelated change, or is specific to the reporter's serving environment (NGINX Unit, Python 3.14, multi-worker-process). I don't have a way to confirm either without reproduction access. Given I can't produce a regression that demonstrates #640's actual symptom, I'm leaving #640 open rather than closing it here, per the suggested alternative.

Test plan

• PolymorphicMultiObjectConcurrencyTestCase.test_forced_registration_interleaving_stays_consistent: deterministically forces the registration-before-repoint race via a mocked apps.register_model() hook. Fails against the unfixed code (source FK / get_model() class mismatch), passes with the fix. Asserts both threads actually completed (not just timed out) before checking results.
• PolymorphicMultiObjectConcurrencyTestCase.test_concurrent_double_submit_does_not_deadlock: two threads independently call CustomObjectTypeField.objects.create() for the identical (custom_object_type, name). Hangs against the wide-lock version (confirmed via pg_stat_activity: one thread idle-in-transaction waiting on the Python lock, the other actively blocked on Postgres waiting for the first's uncommitted CREATE TABLE); completes in ~1.5s with the narrowed lock.
• PolymorphicMultiObjectConcurrencyTestCase.test_field_creation_racing_concurrent_readers_yields_consistent_through_model / ..._with_two_type_setup_...: real-concurrency variants (12 looping get_model() readers against real field creation), including the reported two-type Custom Object setup, through the public CustomObjectTypeField.objects.create() path.
• Delete-confirmation GET coverage: the reported UI path (issue Deleting a Custom Object with a multiobject field raises ValueError (recurrence of #477 in v0.6.0) #640, step 4) doesn't call obj.delete(), which itself realigns the through's FK before Django's collector runs -- a plain GET exercises the unrepaired state directly.
• ruff check clean on both changed files.
• Full test_schema_operations.py passes (4 pre-existing, unrelated errors from a netbox_branching import artifact under the non-branching test configuration, present on unmodified main too).

[bctiemann]

@claude /review and limit response to potential problems of medium or higher severity.

[github-actions]

Claude finished @bctiemann's task in 3m 52s —— View job

---

PR Review: #648 — Fix polymorphic multiobject delete race

• Gathering context
• Reading changed files
• Analyzing for medium+ severity issues

---

Assessment

The production fix in field_types.py is correct and well-reasoned. The three findings below are all in the test code.

---

Medium: Thread completion not asserted after timed join() — spurious pass risk

test_forced_registration_interleaving_stays_consistent (test_schema_operations.py, near line ~440):

t_w.join(timeout=10)
t_r.join(timeout=10)

Neither call asserts not t_w.is_alive() / not t_r.is_alive(). If either thread deadlocks (e.g. a DB-level lock) or hits an unexpected exception before writing to writer_result/reader_result, the joins time out silently and the subsequent assertions all pass: writer_result = {} has no 'error' key, reader_result = {} has no 'error' key, and the final final_model/through_model assertions use a fresh get_model() call that's unaffected by the stuck threads.

The fix is to add after each join:

self.assertFalse(t_w.is_alive(), "writer thread did not complete within timeout")
self.assertFalse(t_r.is_alive(), "reader thread did not complete within timeout")

Fix this →

---

Medium: Deterministic test always burns a 2-second wall-clock delay per CI run

Inside ordered_register_model (the mock side-effect, called on thread W):

reader_done.wait(timeout=2)  # always times out with the fix applied

With the fix, W holds _global_lock for the entire create_polymorphic_m2m_table call. R's get_model(no_cache=True) blocks on _global_lock at _after_model_generation and never signals reader_done, so the 2-second timeout fires unconditionally on every run. The test comment acknowledges this is expected, but 2 seconds per run is a persistent CI tax.

Reducing the timeout to 0.5 or even 0.1 seconds preserves the intent ("give R a window; with the fix it won't get one") while cutting the CI cost by 4–20×.

Fix this →

---

Medium: _global_lock now held during table_names() DB query

Inside create_polymorphic_m2m_table (field_types.py ~line 1821):

with CustomObjectType._global_lock:
    ...
    with conn.cursor() as cursor:
        existing_tables = conn.introspection.table_names(cursor)  # full schema scan
        if table_name not in existing_tables:
            schema_editor.create_model(through)

table_names() fetches all table names from information_schema — a full round-trip that can take tens to hundreds of milliseconds on an instance with many tables. While this lock is held, any concurrent get_model() caller that needs _global_lock for _after_model_generation blocks completely.

Field creation is rare (only on the first save of a polymorphic multiobject field), so this is unlikely to be observable in practice. But it's a latency cliff if it ever coincides with high get_model() traffic. No code change strictly required, but worth tracking if get_model() latency regressions appear.

---

Production code fix — no issues found

The change to field_types.py correctly closes the race: get_polymorphic_through_model + FK repoint now share _global_lock with _after_model_generation's own reuse-or-create path. Since _global_lock is an RLock, re-entrant acquisition from the same thread is safe. No deadlock risk: the writer acquires the Python lock after opening the DB transaction/schema_editor, and the reader holds no DB locks when it reaches _global_lock. The idempotency guard (table_names check) is unchanged in behavior.

[bctiemann]

I fixed item 2 (reduced the timeout to avoid adding to the CI runtime), but left the other two alone—especially item 3 which basically would amount to neutralizing the very race-condition fix we're trying to implement.

[pheus]

Thanks for taking this on. The locking change itself looks reasonable, but I’d like the regression to cover the actual Custom Object lifecycle before approval. The current test uses a persisted field and calls the private schema helper directly, then verifies deletion through a path that repairs the FK first. I’ve left inline notes on matching the reported save and confirmation workflow, plus one on making the thread assertions deterministic.

[pheus]

Thanks for the additional coverage. I think this is now down to two blockers: the regression still does not demonstrate #640’s fresh-process FieldDoesNotExist, and the lock currently spans transactional DDL, which introduces a credible deadlock path.

[bctiemann]

Thanks -- both addressed.

On the deadlock (field_types.py:1808): confirmed, and reproduced it directly -- two threads double-submitting field creation for the same (custom_object_type, name) hung a real test run, and pg_stat_activity showed exactly the cycle you described: one thread idle-in-transaction waiting on _global_lock (needed again in clear_model_cache() to commit), the other actively blocked on Postgres waiting for the first thread's uncommitted CREATE TABLE on the same physical table. Narrowed the lock to just the build+register+repoint step; the table-existence probe/DDL now runs after it's released. Added test_concurrent_double_submit_does_not_deadlock, which hangs against the old wide-lock version and passes in ~1.5s with the fix.

On #640's actual symptom: I tried to reproduce it directly -- a fresh single process, no concurrency, self-referencing 2-3 type polymorphic multiobject field, then a real GET to both the object detail page and the delete-confirmation page from a separate fresh process. Both returned 200 on current main, with or without this PR's fix. I couldn't get FieldDoesNotExist to happen at all this way.

Rather than keep asserting this closes #640 without being able to demonstrate the reported failure, I've rescoped the PR: title/description updated, dropped the Closes: keyword, and left #640 open. What's left is two independently real, verified concurrency bugs in the same function (the original registration-before-repoint race, plus the deadlock above) -- registry-race hardening, not a fix for #640's specific report. Happy to revisit #640 itself separately if reproduction access or more detail becomes available; my best guess is it's either already fixed by something else merged since v0.6.0, or specific to the reporter's serving environment (NGINX Unit, Python 3.14, multi-worker-process), neither of which I can confirm from here.

[bctiemann]

Because this PR does not actually fix #640 directly, it's not completely clear to me whether we should merge this anyway because it's not in response to any specific raised bug; but it does at least eliminate a possible path for the deadlock or multi-worker race condition to manifest. We can keep #640 open and request further reproduction details.

[bctiemann]

Filed the two concurrency bugs this PR fixes as a separate issue, #658, and updated this PR to close it instead of referencing #640 directly. #640 stays open pending further reproduction details, per the discussion above.

[pheus]

Thanks for separating these findings into #658.

I’m happy to approve this. I’ve left two small documentation notes: please update the remaining #640 references to #658 and correct the test explanation that still describes the previous, wider lock scope. Please also ensure the PR and final merge message close #658 rather than #640.

[bctiemann]

Thanks! Both addressed in 7a47878:

• Updated the remaining Deleting a Custom Object with a multiobject field raises ValueError (recurrence of #477 in v0.6.0) #640 references (field_types.py, and the matching test docstrings/assertion messages in test_schema_operations.py) to Race and deadlock in polymorphic multiobject through-model registration #658 -- kept the one reference to Deleting a Custom Object with a multiobject field raises ValueError (recurrence of #477 in v0.6.0) #640 that specifically attributes the delete-confirmation-GET coverage to that issue's own numbered steps, since Race and deadlock in polymorphic multiobject through-model registration #658 doesn't have an equivalent.
• Shortened test_forced_registration_interleaving_stays_consistent's docstring and corrected the now-inaccurate "holds _global_lock for that whole call" claim (true before the lock was narrowed) to describe just the build+register+repoint step, with a pointer to Race and deadlock in polymorphic multiobject through-model registration #658 for the full analysis.

Left the double-submit deadlock test's potential-hang risk as-is per your note that it's not blocking -- agreed a safely-releasable interleaving or subprocess isolation would be the right way to harden it, but that's more surface area than this PR needs.