diff --git a/netbox/circuits/models/circuits.py b/netbox/circuits/models/circuits.py index ceb73d04b..1ec25ab6b 100644 --- a/netbox/circuits/models/circuits.py +++ b/netbox/circuits/models/circuits.py @@ -399,6 +399,10 @@ class CircuitTermination( # Cache objects associated with the terminating object (for filtering) self.cache_related_objects() + if not pointer_moved: + super().save(*args, **kwargs) + return + # Collect the pointer writes per circuit, so that a term_side change within one # circuit clears the old side and sets the new one in a single write updates = {} @@ -412,23 +416,24 @@ class CircuitTermination( with transaction.atomic(using=router.db_for_write(type(self))): super().save(*args, **kwargs) - # Update the cache if this is a new termination or circuit/term_side changed - if pointer_moved: - # Update the new circuit's termination reference - termination_name = f'termination_{self.term_side.lower()}' - updates.setdefault(self.circuit_id, {})[termination_name] = self.pk + # Update the new circuit's termination reference + termination_name = f'termination_{self.term_side.lower()}' + updates.setdefault(self.circuit_id, {})[termination_name] = self.pk - for circuit_id, fields in updates.items(): - circuit = self._set_circuit_terminations(circuit_id, fields) - # Keep the instance's cached Circuit in step with the write + # 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 # Update cached values for subsequent saves, only once the pointer writes have # succeeded, so that a failed save is still pending on retry - if pointer_moved: - self._orig_circuit_id = self.circuit_id - self._orig_term_side = self.term_side + self._orig_circuit_id = self.circuit_id + self._orig_term_side = self.term_side @staticmethod def _set_circuit_terminations(circuit_id, fields): @@ -437,13 +442,15 @@ class CircuitTermination( field name to CircuitTermination PK (or None). 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 so that sequential A/Z - saves snapshot current state; concurrent writers under READ COMMITTED still cannot see - each other's uncommitted writes. + and so records nothing in the changelog. The Circuit is re-fetched under a lock so that + the snapshot reflects a sibling pointer written concurrently; without it, a second writer + 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. """ - circuit = Circuit.objects.filter(pk=circuit_id).first() + # 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() if circuit is None: return None