cleanup

netbox/circuits/models/circuits.py

@@ -413,7 +413,8 @@ class CircuitTermination(
             updates.setdefault(self._orig_circuit_id, {})[old_termination_name] = None
 
         # Write the termination row and the pointers which reference it together
-        with transaction.atomic(using=router.db_for_write(type(self))):
+        using = kwargs.get('using') or router.db_for_write(type(self))
+        with transaction.atomic(using=using):
             super().save(*args, **kwargs)
 
             # Update the new circuit's termination reference
@@ -422,13 +423,8 @@ class CircuitTermination(
 
             # 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
-            for circuit_id in sorted(updates, key=lambda pk: pk or 0):
-                circuit = self._set_circuit_terminations(circuit_id, updates[circuit_id])
-                # Adopt the fetched Circuit, so that to_objectchange() and the caller see the
-                # new pointers. Note this replaces the instance's cached parent, discarding any
-                # prefetch or annotation set up on it.
-                if circuit is not None and circuit.pk == self.circuit_id:
-                    self.circuit = circuit
+            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
@@ -436,7 +432,7 @@ class CircuitTermination(
             self._orig_term_side = self.term_side
 
     @staticmethod
-    def _set_circuit_terminations(circuit_id, fields):
+    def _set_circuit_terminations(circuit_id, fields, using=None):
         """
         Set or clear a Circuit's cached `termination_a`/`termination_z` fields. `fields` maps
         field name to CircuitTermination PK (or None).
@@ -447,12 +443,17 @@ class CircuitTermination(
         could record the first writer's pointer as null. no_key avoids blocking the foreign key
         inserts which reference this circuit.
 
-        Returns the Circuit, or None if no such row exists.
+        Does nothing if the Circuit no longer exists, or if every field already holds its
+        intended value.
         """
+        using = using or router.db_for_write(Circuit)
+
         # order_by() clears the default ordering, whose JOIN would leave the row unlockable
-        circuit = Circuit.objects.filter(pk=circuit_id).order_by().select_for_update(no_key=True).first()
+        circuit = Circuit.objects.using(using).filter(
+            pk=circuit_id
+        ).order_by().select_for_update(no_key=True).first()
         if circuit is None:
-            return None
+            return
 
         # Skip fields which already hold the intended value
         fields = {
@@ -461,14 +462,15 @@ class CircuitTermination(
             if getattr(circuit, f'{field_name}_id') != value
         }
         if not fields:
-            return circuit
+            return
 
         circuit.snapshot()
         for field_name, value in fields.items():
             setattr(circuit, f'{field_name}_id', value)
-        circuit.save(update_fields=[*fields, 'last_updated'])
 
-        return circuit
+        # update_fields excludes _abs_distance, which DistanceMixin.save() recomputes; safe only
+        # because the Circuit was just re-fetched
+        circuit.save(using=using, update_fields=[*fields, 'last_updated'])
 
     def cache_related_objects(self):
         self._provider_network = self._region = self._site_group = self._site = self._location = None

netbox/circuits/signals.py

@@ -1,9 +1,9 @@
-from django.db.models.signals import post_delete, post_save
+from django.db.models.signals import post_delete, post_save, pre_delete
 from django.dispatch import receiver
 
 from dcim.signals import rebuild_paths
 
-from .models import CircuitTermination
+from .models import Circuit, CircuitTermination
 
 
 @receiver((post_save, post_delete), sender=CircuitTermination)
@@ -15,3 +15,21 @@ def rebuild_cablepaths(instance, raw=False, **kwargs):
         peer_termination = instance.get_peer_termination()
         if peer_termination:
             rebuild_paths([peer_termination])
+
+
+@receiver(pre_delete, sender=CircuitTermination)
+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)
+    """
+    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
+
+    field_name = f'termination_{instance.term_side.lower()}'
+    CircuitTermination._set_circuit_terminations(instance.circuit_id, {field_name: None}, using=using)

netbox/circuits/tests/test_models.py

@@ -325,6 +325,14 @@ class CircuitTerminationChangeLoggingTestCase(TestCase):
         self.assertIsNone(changes[0].prechange_data['termination_a'])
         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,
+        )
+        self.assertLess(termination_create.pk, changes[0].pk)
+
     @tag('regression')  # Ref: #23134
     def test_second_termination_snapshots_current_state(self):
         # The A pointer is already committed when the Z termination is created
@@ -451,19 +459,67 @@ class CircuitTerminationChangeLoggingTestCase(TestCase):
         self.assertEqual(new_changes.count(), 1)
         self.assertEqual(new_changes[0].postchange_data['termination_a'], termination.pk)
 
-    def test_deletion_clears_pointer(self):
-        # on_delete=SET_NULL clears the pointer without a post_save, and related_name='+' keeps
-        # these relations out of _meta.related_objects, so handle_deleted_object() misses them
-        # too. Only the resulting database state is asserted.
+    @tag('regression')  # Ref: #23134
+    def test_deletion_records_circuit_update(self):
         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(termination.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.assertIsNone(changes[0].postchange_data['termination_a'])
+
+    @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
+        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)
+
+        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(
+            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)
+
+    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],
+        ))
+        ObjectChange.objects.all().delete()
+
+        self._tracked(self.circuits[0].delete)
+
+        self.assertFalse(self._circuit_changes(self.circuits[0]).exists())
+
     @tag('regression')  # Ref: #23134
     def test_failed_pointer_write_leaves_the_change_pending(self):
         # The cached originals must not advance until the pointer writes have succeeded