Fixes #629: Delete stale model registration before regenerating, not after

[bctiemann]

Closes: #629

Summary

Reproduces and fixes #629 (Phantom Tagged Objects): deleting a tagged custom object left its TaggedItem row behind. The tag's detail page reported a nonzero item count, but filtering by that tag returned zero results, since the referenced object no longer existed.

Root cause

CustomObjectType.get_model() deleted the old apps.all_models registry entry for a COT's dynamic model after generate_model() had already built the replacement class, not before.

TagsMixin's tags field resolves its through model lazily: taggit's contribute_to_class() runs mid-construction, while Django's ModelBase.__new__() is still adding fields to the new class — well before the new class itself gets registered. It calls lazy_related_operation(), which treats the owning class as a dependency, resolved by looking it up in apps.all_models under the model's name (not the literal class object passed in). Since the old class was still registered under that name at that exact moment, the lookup resolved immediately against the stale, about-to-be-replaced class instead of deferring — so the new class's own tags field's post_through_setup() never ran against itself, and it never got a tagged_items GenericRelation. Without that GenericRelation, Django's deletion collector has no way to cascade-delete a custom object's TaggedItem rows on delete — regardless of whether the object is deleted via a single instance's .delete(), a bulk queryset.delete(), or a freshly-regenerated class.

Fix

Delete the stale registry entry before calling generate_model() instead of after, so generate_model()'s own type() call is what correctly registers (and satisfies the lazy dependency for) the new class. The now-redundant explicit apps.register_model() call afterward is guarded to skip re-registering when generate_model() already did so, avoiding a spurious "already registered" RuntimeWarning.

Testing

New PhantomTaggedObjectsTestCase in test_models.py:

• test_generated_model_has_tagged_items_generic_relation — the root cause: the generated model must actually have a tagged_items GenericRelation.
• test_direct_delete_removes_tagged_item / test_queryset_bulk_delete_removes_tagged_item / test_delete_after_model_regeneration_removes_tagged_item — all three deletion paths must clean up the TaggedItem row.
• test_filtering_by_tag_matches_actual_object_count — the user-visible symptom: filtering by tag after deleting a tagged object must return the correct count.

All 5 fail without the fix (reproducing the exact reported symptom) and pass with it.

Full suite verified in a clean venv (fresh install, no stray environment state): 1154 tests, 0 failures/errors, 8 skipped.

[bctiemann]

@claude /review and limit response to actionable issues.

[github-actions]

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

---

Code Review

The fix is correct and the root cause explanation is accurate. Actionable issues below.

---

1. Wider race window during concurrent model lookup (medium)

Moving the deletion to lines 1745–1749 (before generate_model()) widens the window where apps.all_models[APP_LABEL] has no entry for this model name. Previously the delete+register were both under _suppress_clear_cache() and happened atomically after generation — the window was a few microseconds. Now the window spans the entire generate_model() call (which calls type(), which runs field contribute_to_class() for all fields — potentially significant time).

During this window, any concurrent thread calling apps.get_model(APP_LABEL, model_name) — e.g., from a content-type resolution, a view lookup, or Django's own relation machinery — will raise LookupError. The existing comment at line 1673 already acknowledges the concurrent-generation trade-off, but this is a distinct failure mode: it can affect threads that aren't regenerating the model at all.

This is likely acceptable in practice (the window is still short and the existing design accepts some concurrency looseness), but it should be documented explicitly alongside the existing concurrency comment.

Document the wider window →

---

2. _suppress_clear_cache() is unnecessary around the direct dict deletion (minor)

# lines 1747-1749 — models.py
with _suppress_clear_cache():
    if model_key in apps.all_models[APP_LABEL]:
        del apps.all_models[APP_LABEL][model_key]

del apps.all_models[APP_LABEL][model_key] is a direct dict mutation; it does not call apps.clear_cache() (only apps.register_model() does that). The context manager is harmless but misleading — it implies apps.clear_cache() could fire here, which it can't. The _suppress_clear_cache() is only actually needed around apps.register_model() calls. Removing it from this block would make the code's intent clearer.

Remove unnecessary suppressor →

---

3. Redundant model_key computation (minor)

model_key = model_name.lower() is computed at line 1746 (inside if branch_id is None:) and again unconditionally at line 1788. Since both are inside the same branch_id is None conditional scope (the second block also only uses model_key when branch_id is None), the second assignment is always redundant when the first ran. Move the first assignment above the if branch_id is None: block to eliminate the duplication.

Fix redundant computation →

---

Tests — coverage is solid. The five cases in PhantomTaggedObjectsTestCase directly reproduce the reported symptom and all three deletion paths. No gaps.

[bctiemann]

1. Added a note to the existing concurrency comment at the top of get_model() explaining that a concurrent, unrelated lookup can now transiently see the model name as unregistered during regeneration
2. Removed redundant call
3. Fixed