From 00ed38771a002f4c45279a3b0cbe3c9d3dbabd4b Mon Sep 17 00:00:00 2001 From: Arthur Date: Tue, 8 Sep 2026 13:18:06 -0700 Subject: [PATCH] delete handling --- netbox/circuits/models/circuits.py | 25 +++++++++++------ netbox/circuits/signals.py | 6 ++++- netbox/circuits/tests/test_models.py | 40 +++++++++++++++++++++++++--- 3 files changed, 58 insertions(+), 13 deletions(-) diff --git a/netbox/circuits/models/circuits.py b/netbox/circuits/models/circuits.py index f4d845169..2cbb68543 100644 --- a/netbox/circuits/models/circuits.py +++ b/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 diff --git a/netbox/circuits/signals.py b/netbox/circuits/signals.py index 3060fc47f..bf772bd8f 100644 --- a/netbox/circuits/signals.py +++ b/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 + ) diff --git a/netbox/circuits/tests/test_models.py b/netbox/circuits/tests/test_models.py index 26ac070fe..2fbbd7e57 100644 --- a/netbox/circuits/tests/test_models.py +++ b/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],