diff --git a/netbox/circuits/models/circuits.py b/netbox/circuits/models/circuits.py index e5e0847dc..8b2bae0c0 100644 --- a/netbox/circuits/models/circuits.py +++ b/netbox/circuits/models/circuits.py @@ -407,8 +407,11 @@ class CircuitTermination( # circuit clears the old side and sets the new one in a single write updates = {} - # Clear the old termination reference if circuit or term_side changed - if circuit_changed or term_side_changed: + # Clear the old termination reference if circuit or term_side changed. Skipped while + # inserting: nothing references the row yet, and the originals captured in __init__ + # describe whatever was passed to the constructor, which may be another termination's + # pointer. + if not is_new and (circuit_changed or term_side_changed): old_termination_name = f'termination_{self._orig_term_side.lower()}' updates.setdefault(self._orig_circuit_id, {})[old_termination_name] = None @@ -477,9 +480,13 @@ class CircuitTermination( for field_name, value in fields.items(): setattr(circuit, f'{field_name}_id', value) - # 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']) + # Saved in full rather than with update_fields, so that postchange_data describes the + # row as written. The mixin save() chain mutates fields beyond the pointers + # (custom_field_data, distance_unit, _abs_distance); excluding them from the write left + # them in the record but not the database, and a replaying consumer applies the + # difference. Writing every column is safe because the row was fetched under the lock + # held for this transaction, so no concurrent write can interleave. + circuit.save(using=using) def delete(self, *args, **kwargs): # Clear the parent Circuit's cached pointer before the deletion starts, so that its change @@ -493,8 +500,15 @@ class CircuitTermination( # # 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)) + # Model.delete() still accepts `using` positionally + using = kwargs.get('using') or (args[0] if args else None) or router.db_for_write(type(self)) with transaction.atomic(using=using): + # Lock this row before the circuit. super().save() locks it first too, so without + # this a concurrent save and delete of the same termination could deadlock. + CircuitTermination.objects.using(using).filter( + pk=self.pk + ).order_by().select_for_update().first() + if self.term_side: self._set_circuit_terminations( self.circuit_id, diff --git a/netbox/circuits/tests/test_models.py b/netbox/circuits/tests/test_models.py index d439207a4..9a669f342 100644 --- a/netbox/circuits/tests/test_models.py +++ b/netbox/circuits/tests/test_models.py @@ -431,6 +431,33 @@ class CircuitTerminationChangeLoggingTestCase(TestCase): self.assertEqual(new_changes.count(), 1) self.assertEqual(new_changes[0].postchange_data['termination_a'], termination.pk) + @tag('regression') # Ref: #23134 + def test_new_termination_does_not_clear_sibling_pointer(self): + # __init__ captures the originals from the constructor kwargs, so mutating term_side + # before the first save reaches the clear path with originals naming a live sibling + termination_a = self._tracked(lambda: CircuitTermination.objects.create( + circuit=self.circuits[0], term_side='A', termination=self.sites[0], + )) + ObjectChange.objects.all().delete() + + def _create(): + termination = CircuitTermination( + circuit=self.circuits[0], term_side='A', termination=self.sites[1], + ) + termination.term_side = 'Z' + termination.save() + return termination + + termination_z = self._tracked(_create) + + self.circuits[0].refresh_from_db() + self.assertEqual(self.circuits[0].termination_a_id, termination_a.pk) + self.assertEqual(self.circuits[0].termination_z_id, termination_z.pk) + + changes = self._circuit_changes(self.circuits[0]) + self.assertEqual(changes.count(), 1) + self.assertEqual(changes[0].postchange_data['termination_a'], termination_a.pk) + @tag('regression') # Ref: #23134 def test_noop_resave_records_no_circuit_update(self): termination = self._tracked(lambda: CircuitTermination.objects.create(