delete handling

netbox/circuits/models/circuits.py

@@ -421,21 +421,23 @@ class CircuitTermination(
             termination_name = f'termination_{self.term_side.lower()}'
             updates.setdefault(self.circuit_id, {})[termination_name] = self.pk
 
-            # Ordered by PK, so that two terminations moving between the same pair of circuits
-            # take the two locks in the same order and cannot deadlock
+            # 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.
             for circuit_id in sorted(updates):
                 self._set_circuit_terminations(circuit_id, updates[circuit_id], using=using)
 
-            # Update cached values for subsequent saves, only once the pointer writes have
-            # succeeded, so that a failed save is still pending on retry
-            self._orig_circuit_id = self.circuit_id
-            self._orig_term_side = self.term_side
+        # Advanced only once the writes have left the block, so that a rolled-back save is still
+        # pending on retry
+        self._orig_circuit_id = self.circuit_id
+        self._orig_term_side = self.term_side
 
     @staticmethod
-    def _set_circuit_terminations(circuit_id, fields, using=None):
+    def _set_circuit_terminations(circuit_id, fields, using=None, only_if_references=None):
         """
         Set or clear a Circuit's cached `termination_a`/`termination_z` fields. `fields` maps
-        field name to CircuitTermination PK (or None).
+        field name to CircuitTermination PK (or None). `only_if_references` restricts the write
+        to fields which currently hold that PK.
 
         Written via snapshot() + save() rather than a queryset update(), which emits no post_save
         and so records nothing in the changelog. The Circuit is re-fetched under a lock so that
@@ -455,6 +457,13 @@ class CircuitTermination(
         if circuit is None:
             return
 
+        if only_if_references is not None:
+            fields = {
+                field_name: value
+                for field_name, value in fields.items()
+                if getattr(circuit, f'{field_name}_id') == only_if_references
+            }
+
         # Skip fields which already hold the intended value
         fields = {
             field_name: value

netbox/circuits/signals.py

@@ -31,5 +31,9 @@ def clear_circuit_termination_pointer(instance, using=None, origin=None, **kwarg
     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)
+    CircuitTermination._set_circuit_terminations(
+        instance.circuit_id, {field_name: None}, using=using, only_if_references=instance.pk
+    )

netbox/circuits/tests/test_models.py

@@ -307,6 +307,13 @@ class CircuitTerminationChangeLoggingTestCase(TestCase):
         with event_tracking(request):
             return func()
 
+    def _termination_change(self, termination_pk, action):
+        return ObjectChange.objects.get(
+            changed_object_type=ContentType.objects.get_for_model(CircuitTermination),
+            changed_object_id=termination_pk,
+            action=action,
+        )
+
     def _circuit_changes(self, circuit):
         return ObjectChange.objects.filter(
             changed_object_type=ContentType.objects.get_for_model(Circuit),
@@ -326,10 +333,8 @@ class CircuitTerminationChangeLoggingTestCase(TestCase):
         self.assertEqual(changes[0].postchange_data['termination_a'], termination.pk)
 
         # The pointer references the termination's PK, so the create must be recorded first
-        termination_create = ObjectChange.objects.get(
-            changed_object_type=ContentType.objects.get_for_model(CircuitTermination),
-            changed_object_id=termination.pk,
-            action=ObjectChangeActionChoices.ACTION_CREATE,
+        termination_create = self._termination_change(
+            termination.pk, ObjectChangeActionChoices.ACTION_CREATE
         )
         self.assertLess(termination_create.pk, changes[0].pk)
 
@@ -477,6 +482,15 @@ class CircuitTerminationChangeLoggingTestCase(TestCase):
         self.assertEqual(changes[0].prechange_data['termination_a'], termination_pk)
         self.assertIsNone(changes[0].postchange_data['termination_a'])
 
+        # core.signals.handle_deleted_object is connected before this app's receiver, so the
+        # DELETE precedes the pointer clear. Replaying in this order relies on the consumer
+        # applying the DELETE through the ORM, where on_delete=SET_NULL clears the pointer, or
+        # on the FK being DEFERRABLE INITIALLY DEFERRED within one transaction.
+        termination_delete = self._termination_change(
+            termination_pk, ObjectChangeActionChoices.ACTION_DELETE
+        )
+        self.assertLess(termination_delete.pk, changes[0].pk)
+
     @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
@@ -510,6 +524,24 @@ class CircuitTerminationChangeLoggingTestCase(TestCase):
         self.assertEqual(changes.count(), 1)
         self.assertEqual(changes[0].prechange_data['termination_a'], termination_pk)
 
+    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
+        # belonging to a different termination
+        termination_a = self._tracked(lambda: CircuitTermination.objects.create(
+            circuit=self.circuits[0], term_side='A', termination=self.sites[0],
+        ))
+        termination_z = self._tracked(lambda: CircuitTermination.objects.create(
+            circuit=self.circuits[0], term_side='Z', termination=self.sites[1],
+        ))
+        ObjectChange.objects.all().delete()
+
+        termination_z.term_side = 'A'
+        self._tracked(termination_z.delete)
+
+        self.circuits[0].refresh_from_db()
+        self.assertEqual(self.circuits[0].termination_a_id, termination_a.pk)
+        self.assertFalse(self._circuit_changes(self.circuits[0]).exists())
+
     def test_circuit_deletion_records_no_pointer_update(self):
         self._tracked(lambda: CircuitTermination.objects.create(
             circuit=self.circuits[0], term_side='A', termination=self.sites[0],