delete handling

netbox/circuits/apps.py

@@ -1,20 +1,4 @@
 from django.apps import AppConfig
-from django.db.models.signals import pre_delete
-
-
-def _clear_circuit_termination_pointer(sender, **kwargs):
-    from .models import CircuitTermination
-    from .signals import clear_circuit_termination_pointer
-
-    if sender is CircuitTermination:
-        clear_circuit_termination_pointer(**kwargs)
-
-
-# This module is imported in populate() phase 1, ahead of the models phase which connects
-# core.signals.handle_deleted_object. Connecting here records the Circuit pointer clear before the
-# termination's own DELETE; branch revert replays newest-first and needs the termination restored
-# before the pointer referencing it. (#23134)
-pre_delete.connect(_clear_circuit_termination_pointer)
 
 
 class CircuitsConfig(AppConfig):

netbox/circuits/models/circuits.py

@@ -421,9 +421,9 @@ class CircuitTermination(
             termination_name = f'termination_{self.term_side.lower()}'
             updates.setdefault(self.circuit_id, {})[termination_name] = self.pk
 
-            # Ordered by PK so concurrent saves take the circuit locks in the same order. The
-            # delete path is unordered (see circuits.signals), so a bulk delete racing a save
-            # can still deadlock.
+            # Ordered by PK so concurrent saves take the circuit locks in the same order.
+            # delete() locks in queryset order, so a bulk delete under an enclosing transaction
+            # racing a save can still deadlock.
             for circuit_id in sorted(updates):
                 self._set_circuit_terminations(circuit_id, updates[circuit_id], using=using)
 
@@ -481,6 +481,31 @@ class CircuitTermination(
         # because the Circuit was just re-fetched
         circuit.save(using=using, update_fields=[*fields, 'last_updated'])
 
+    def delete(self, *args, **kwargs):
+        # Clear the parent Circuit's cached pointer before the deletion starts, so that its change
+        # record precedes this row's DELETE. on_delete=SET_NULL clears the column with a bulk
+        # UPDATE, and related_name='+' hides the relation from Circuit._meta.related_objects, so
+        # neither path records an ObjectChange. (#23134)
+        #
+        # Not a pre_delete receiver: core.signals.handle_deleted_object connects during the models
+        # import phase, ahead of any app's ready(), and Django dispatches in connection order, so a
+        # receiver here would run only after the DELETE had been recorded.
+        #
+        # Cascades (e.g. deleting the terminating Site, or the Circuit itself) reach the row through
+        # the collector rather than here, and remain unrecorded.
+        using = kwargs.get('using') or router.db_for_write(type(self))
+        with transaction.atomic(using=using):
+            if self.term_side:
+                self._set_circuit_terminations(
+                    self.circuit_id,
+                    {f'termination_{self.term_side.lower()}': None},
+                    using=using,
+                    only_if_references=self.pk,
+                )
+            return super().delete(*args, **kwargs)
+
+    delete.alters_data = True
+
     def cache_related_objects(self):
         self._provider_network = self._region = self._site_group = self._site = self._location = None
         if self.termination_type:

netbox/circuits/signals.py

@@ -3,7 +3,7 @@ from django.dispatch import receiver
 
 from dcim.signals import rebuild_paths
 
-from .models import Circuit, CircuitTermination
+from .models import CircuitTermination
 
 
 @receiver((post_save, post_delete), sender=CircuitTermination)
@@ -15,26 +15,3 @@ def rebuild_cablepaths(instance, raw=False, **kwargs):
         peer_termination = instance.get_peer_termination()
         if peer_termination:
             rebuild_paths([peer_termination])
-
-
-def clear_circuit_termination_pointer(instance, using=None, origin=None, **kwargs):
-    """
-    Clear the parent Circuit's cached `termination_a`/`termination_z` pointer with a change-logged
-    save. on_delete=SET_NULL clears it via a bulk UPDATE, and related_name='+' hides the relation
-    from Circuit._meta.related_objects, so neither path records an ObjectChange. (#23134)
-
-    Connected in CircuitsConfig, not here, so that it precedes handle_deleted_object.
-    """
-    if not instance.term_side:
-        return
-
-    # The pointer goes away with the circuit, so a change record for it would be spurious
-    if isinstance(origin, Circuit) or getattr(origin, 'model', None) is Circuit:
-        return
-
-    # only_if_references matches what on_delete=SET_NULL would have cleared: the in-memory
-    # term_side may not be what the pointer actually references
-    field_name = f'termination_{instance.term_side.lower()}'
-    CircuitTermination._set_circuit_terminations(
-        instance.circuit_id, {field_name: None}, using=using, only_if_references=instance.pk
-    )

netbox/circuits/tests/test_models.py

@@ -491,36 +491,37 @@ class CircuitTerminationChangeLoggingTestCase(TestCase):
 
     @tag('regression')  # Ref: #23134
     def test_bulk_deletion_records_circuit_update(self):
-        # A queryset delete() passes the queryset as the signal's origin rather than an instance
+        # NetBox's bulk delete views iterate obj.delete() rather than calling queryset.delete()
         termination = self._tracked(lambda: CircuitTermination.objects.create(
             circuit=self.circuits[0], term_side='A', termination=self.sites[0],
         ))
         ObjectChange.objects.all().delete()
         termination_pk = termination.pk
 
-        self._tracked(CircuitTermination.objects.filter(pk=termination_pk).delete)
+        def _bulk_delete():
+            for obj in CircuitTermination.objects.filter(pk=termination_pk):
+                obj.delete()
+
+        self._tracked(_bulk_delete)
 
         changes = self._circuit_changes(self.circuits[0])
         self.assertEqual(changes.count(), 1)
         self.assertEqual(changes[0].prechange_data['termination_a'], termination_pk)
         self.assertIsNone(changes[0].postchange_data['termination_a'])
 
-    def test_cascade_from_termination_parent_records_circuit_update(self):
-        # The circuit survives the cascade, so the pointer clear still has to be recorded
-        termination = self._tracked(lambda: CircuitTermination.objects.create(
+    def test_cascade_deletion_leaves_pointer_unrecorded(self):
+        # Deleting the terminating Site reaches the termination through the collector, which does
+        # not call delete(). on_delete=SET_NULL still clears the column, but nothing records it.
+        self._tracked(lambda: CircuitTermination.objects.create(
             circuit=self.circuits[0], term_side='A', termination=self.sites[0],
         ))
         ObjectChange.objects.all().delete()
-        termination_pk = termination.pk
 
         self._tracked(self.sites[0].delete)
 
         self.circuits[0].refresh_from_db()
         self.assertIsNone(self.circuits[0].termination_a_id)
-
-        changes = self._circuit_changes(self.circuits[0])
-        self.assertEqual(changes.count(), 1)
-        self.assertEqual(changes[0].prechange_data['termination_a'], termination_pk)
+        self.assertFalse(self._circuit_changes(self.circuits[0]).exists())
 
     def test_deletion_leaves_pointer_for_another_termination(self):
         # An in-memory term_side which diverges from the persisted one must not clear a pointer