cleanup

netbox/circuits/models/circuits.py

@@ -1,7 +1,7 @@
 from django.apps import apps
 from django.contrib.contenttypes.fields import GenericForeignKey, GenericRelation
 from django.core.exceptions import ValidationError
-from django.db import models
+from django.db import models, router, transaction
 from django.urls import reverse
 from django.utils.translation import gettext_lazy as _
 
@@ -394,15 +394,13 @@ class CircuitTermination(
 
         circuit_changed = tracking_relevant and self._orig_circuit_id and self._orig_circuit_id != self.circuit_id
         term_side_changed = tracking_relevant and self._orig_term_side and self._orig_term_side != self.term_side
+        pointer_moved = is_new or circuit_changed or term_side_changed
 
         # Cache objects associated with the terminating object (for filtering)
         self.cache_related_objects()
 
-        super().save(*args, **kwargs)
-
-        # Collect the pointer writes per circuit, so that a term_side change within a single
-        # circuit clears the old side and sets the new one in one write rather than passing
-        # through a state with neither side set
+        # 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 = {}
 
         # Clear the old termination reference if circuit or term_side changed
@@ -410,57 +408,61 @@ class CircuitTermination(
             old_termination_name = f'termination_{self._orig_term_side.lower()}'
             updates.setdefault(self._orig_circuit_id, {})[old_termination_name] = None
 
-        # Update the cache if this is a new termination or circuit/term_side changed
-        if is_new or circuit_changed or term_side_changed:
-            # Update the new circuit's termination reference
-            termination_name = f'termination_{self.term_side.lower()}'
-            updates.setdefault(self.circuit_id, {})[termination_name] = self.pk
+        # Write the termination row and the pointers which reference it together
+        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 cached values for subsequent saves
-            self._orig_circuit_id = self.circuit_id
-            self._orig_term_side = self.term_side
+            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
+                if circuit is not None and circuit.pk == self.circuit_id:
+                    self.circuit = circuit
 
-        for circuit_id, fields in updates.items():
-            self._set_circuit_terminations(circuit_id, fields)
+            # 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
 
     @staticmethod
     def _set_circuit_terminations(circuit_id, fields):
         """
-        Point a Circuit's cached `termination_a`/`termination_z` fields at the given
-        CircuitTermination PKs, or clear them. `fields` maps field name to PK (or None); both
-        sides are passed together where they change at once, to keep it to a single write.
-
-        This is written via snapshot() + save() rather than a queryset update() so that the write
-        passes through post_save and is recorded in the changelog. A raw update() emits no signal,
-        so consumers which replay ObjectChange records have no record of the write and silently
-        drop the association.
-
-        The Circuit is re-fetched rather than reusing a cached `self.circuit` so that saving the A
-        and Z terminations in sequence does not snapshot a Circuit loaded before the A pointer was
-        written. That only holds within a single sequential flow: under READ COMMITTED, concurrent
-        writers can each snapshot a Circuit which does not yet reflect the other's uncommitted
-        write. The row itself is safe, as update_fields limits the write to the pointer fields
-        this termination owns (plus last_updated, which is last-writer-wins).
+        Set or clear a Circuit's cached `termination_a`/`termination_z` fields. `fields` maps
+        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.
+
+        Returns the Circuit, or None if no such row exists.
         """
         circuit = Circuit.objects.filter(pk=circuit_id).first()
         if circuit is None:
-            return
+            return None
 
-        # Skip fields which already hold the intended value, so that a redundant write neither
-        # touches the row nor dispatches an event
+        # Skip fields which already hold the intended value
         fields = {
             field_name: value
             for field_name, value in fields.items()
             if getattr(circuit, f'{field_name}_id') != value
         }
         if not fields:
-            return
+            return circuit
 
         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
+
     def cache_related_objects(self):
         self._provider_network = self._region = self._site_group = self._site = self._location = None
         if self.termination_type:

netbox/circuits/tests/test_models.py

@@ -1,4 +1,5 @@
 import uuid
+from unittest.mock import patch
 
 from django.contrib.contenttypes.models import ContentType
 from django.core.exceptions import NON_FIELD_ERRORS, ValidationError
@@ -280,9 +281,8 @@ class CircuitTerminationDenormalizationTriggerTestCase(TestCase):
 
 class CircuitTerminationChangeLoggingTestCase(TestCase):
     """
-    The Circuit.termination_a/termination_z pointers are maintained by CircuitTermination.save().
-    They were previously written with a queryset update(), which emits no post_save and therefore
-    no ObjectChange, so consumers which replay the changelog never saw the association. (#23134)
+    Circuit.termination_a/termination_z are maintained by CircuitTermination.save(). Writing them
+    with a queryset update() emitted no post_save, and so no ObjectChange. (#23134)
     """
     @classmethod
     def setUpTestData(cls):
@@ -327,8 +327,7 @@ class CircuitTerminationChangeLoggingTestCase(TestCase):
 
     @tag('regression')  # Ref: #23134
     def test_second_termination_snapshots_current_state(self):
-        # The A pointer is already committed when the Z termination is created; its prechange
-        # snapshot must reflect that rather than a Circuit cached before the A write.
+        # The A pointer is already committed when the Z termination is created
         termination_a = self._tracked(lambda: CircuitTermination.objects.create(
             circuit=self.circuits[0], term_side='A', termination=self.sites[0],
         ))
@@ -358,13 +357,13 @@ class CircuitTerminationChangeLoggingTestCase(TestCase):
 
         self._tracked(_move)
 
-        # The old circuit's pointer is cleared...
+        # The old circuit's pointer is cleared
         old_changes = self._circuit_changes(self.circuits[0])
         self.assertEqual(old_changes.count(), 1)
         self.assertEqual(old_changes[0].prechange_data['termination_a'], termination.pk)
         self.assertIsNone(old_changes[0].postchange_data['termination_a'])
 
-        # ...and the new circuit's pointer is set.
+        # The new circuit's pointer is set
         new_changes = self._circuit_changes(self.circuits[1])
         self.assertEqual(new_changes.count(), 1)
         self.assertIsNone(new_changes[0].prechange_data['termination_a'])
@@ -383,8 +382,7 @@ class CircuitTerminationChangeLoggingTestCase(TestCase):
 
         self._tracked(_flip)
 
-        # Both pointers move within one circuit, so the clear and the set are coalesced into one
-        # write rather than passing through a state with neither side set.
+        # Both pointers move within one circuit, so the clear and the set are coalesced
         changes = self._circuit_changes(self.circuits[0])
         self.assertEqual(changes.count(), 1)
         self.assertEqual(changes[0].prechange_data['termination_a'], termination.pk)
@@ -394,8 +392,8 @@ class CircuitTerminationChangeLoggingTestCase(TestCase):
 
     @tag('regression')  # Ref: #23134
     def test_redundant_pointer_write_is_skipped(self):
-        # bulk_create() bypasses save(), so the circuit's pointer is never written. Moving the
-        # termination afterwards reaches the clear path with the pointer already null.
+        # bulk_create() bypasses save(), leaving the pointer unwritten; moving the termination
+        # afterwards reaches the clear path with it already null
         CircuitTermination.objects.bulk_create([
             CircuitTermination(circuit=self.circuits[0], term_side='A', termination=self.sites[0]),
         ])
@@ -409,13 +407,13 @@ class CircuitTerminationChangeLoggingTestCase(TestCase):
 
         self._tracked(_move)
 
-        # The old circuit's pointer was already null, so it is not written to at all...
+        # The old circuit's pointer was already null, so it is not written to
         self.assertFalse(self._circuit_changes(self.circuits[0]).exists())
         self.assertEqual(
             Circuit.objects.get(pk=self.circuits[0].pk).last_updated, old_circuit_last_updated
         )
 
-        # ...while the new circuit's pointer is set as usual.
+        # The new circuit's pointer is set as usual
         new_changes = self._circuit_changes(self.circuits[1])
         self.assertEqual(new_changes.count(), 1)
         self.assertEqual(new_changes[0].postchange_data['termination_a'], termination.pk)
@@ -433,8 +431,7 @@ class CircuitTerminationChangeLoggingTestCase(TestCase):
 
     @tag('regression')  # Ref: #23134
     def test_circuit_change_via_update_fields_records_circuit_update(self):
-        # save(update_fields=...) takes its own branch when deciding whether the circuit or
-        # term_side is being persisted; the pointer writes must be recorded there too.
+        # save(update_fields=...) takes its own branch when deciding what is being persisted
         termination = self._tracked(lambda: CircuitTermination.objects.create(
             circuit=self.circuits[0], term_side='A', termination=self.sites[0],
         ))
@@ -454,20 +451,45 @@ 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_without_recording_a_change(self):
-        # Deleting a termination clears the pointer via on_delete=SET_NULL, which emits no
-        # post_save and so is not change-logged. handle_deleted_object() does not cover it
-        # either: termination_a/termination_z declare related_name='+', so they are hidden
-        # relations and absent from CircuitTermination._meta.related_objects. The changelog is
-        # therefore still asymmetric here; documented rather than fixed, as replaying the
-        # termination's DELETE re-applies SET_NULL on the target side.
+    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.
         termination = self._tracked(lambda: CircuitTermination.objects.create(
             circuit=self.circuits[0], term_side='A', termination=self.sites[0],
         ))
-        ObjectChange.objects.all().delete()
 
         self._tracked(termination.delete)
 
         self.circuits[0].refresh_from_db()
         self.assertIsNone(self.circuits[0].termination_a_id)
-        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
+        termination = self._tracked(lambda: CircuitTermination.objects.create(
+            circuit=self.circuits[0], term_side='A', termination=self.sites[0],
+        ))
+
+        def _move():
+            termination.circuit = self.circuits[1]
+            termination.save()
+
+        with patch.object(
+            CircuitTermination, '_set_circuit_terminations', side_effect=OSError('boom')
+        ):
+            with self.assertRaises(OSError):
+                self._tracked(_move)
+
+        # The termination row was rolled back along with the pointer writes
+        termination.refresh_from_db()
+        self.assertEqual(termination.circuit, self.circuits[0])
+
+        # A retry still sees the move as pending, so both pointers end up correct
+        termination.circuit = self.circuits[1]
+        self._tracked(termination.save)
+
+        self.circuits[0].refresh_from_db()
+        self.circuits[1].refresh_from_db()
+        self.assertIsNone(self.circuits[0].termination_a_id)
+        self.assertEqual(self.circuits[1].termination_a_id, termination.pk)