Fixes #23160: Change-log the disconnect on terminating objects when a Cable is deleted (#23161)

Snapshot and save directly connected endpoints during cable deletion so
their change logs reflect the cleared cable attributes.

Track both instance and queryset deletions, respect the deletion database
alias, and preserve existing behavior when removing individual terminations.

Add regression coverage for disconnect logging, profiled cable attributes,
and non-path endpoints.

netbox/dcim/models/cables.py

@@ -74,13 +74,31 @@ class CableBundle(PrimaryModel):
 # Cables
 #
 
+class CableQuerySet(RestrictedQuerySet):
+
+    def delete(self):
+        # Track these Cables as being deleted for the duration, as Cable.delete() does for a single
+        # instance: a queryset delete never calls it. Between them the two cover every deletion, as a
+        # Cable is never itself cascade-deleted (nothing points at it with on_delete=CASCADE).
+        # Resolve the PKs on the DB the delete will use, so read routing can't miss a lagging replica.
+        using = self._db or router.db_for_write(self.model, **self._hints)
+        pks = list(self.using(using).values_list('pk', flat=True))
+        for pk in pks:
+            Cable._track_deletion(pk)
+        try:
+            return super().delete()
+        finally:
+            for pk in pks:
+                Cable._untrack_deletion(pk)
+
+
 class Cable(PrimaryModel):
     """
     A physical connection between two endpoints.
     """
-    # Per-thread tracking of Cable PKs currently in delete(); referenced by
-    # dcim.signals.nullify_connected_endpoints to skip per-CableTermination
-    # cable path retracing during cascade (retrace_cable_paths handles it once).
+    # Per-thread tracking of Cable PKs currently being deleted; referenced by
+    # dcim.signals.nullify_connected_endpoints to record the disconnect on each terminating object and
+    # to skip per-CableTermination path retracing during the cascade (retrace_cable_paths does it once).
     _deletion_tracking = threading.local()
 
     type = models.CharField(
@@ -150,6 +168,8 @@ class Cable(PrimaryModel):
 
     clone_fields = ('tenant', 'type', 'profile', 'bundle')
 
+    objects = CableQuerySet.as_manager()
+
     class Meta:
         ordering = ('pk',)
         verbose_name = _('cable')
@@ -385,20 +405,32 @@ class Cable(PrimaryModel):
         self._terminations_modified = False
 
     def delete(self, *args, **kwargs):
-        # Track this Cable as being deleted so the post_delete signal handler
-        # for cascaded CableTerminations can skip redundant path retracing;
-        # retrace_cable_paths() will retrace each affected path once after the
-        # Cable itself is deleted. Cache the PK locally because super().delete()
-        # clears self.pk before the finally block runs. The tracking set lives
-        # on a threading.local() to isolate concurrent deletions across threads.
-        if not hasattr(Cable._deletion_tracking, 'pks'):
-            Cable._deletion_tracking.pks = set()
+        # Cache the PK locally because super().delete() clears self.pk before the finally block runs; the
+        # finally also guarantees the PK is discarded if the delete raises. CableQuerySet.delete() tracks
+        # the same way for a queryset delete, which never calls this.
         pk = self.pk
-        Cable._deletion_tracking.pks.add(pk)
+        Cable._track_deletion(pk)
         try:
             return super().delete(*args, **kwargs)
         finally:
-            Cable._deletion_tracking.pks.discard(pk)
+            Cable._untrack_deletion(pk)
+
+    @classmethod
+    def _track_deletion(cls, pk):
+        """
+        Track a Cable as being deleted, so that the post_delete handler for its cascaded CableTerminations can
+        record the disconnect on each terminating object and skip redundant path retracing (retrace_cable_paths()
+        retraces each affected path once, after the Cable itself is deleted). The tracking set lives on a
+        threading.local() to isolate concurrent deletions across threads.
+        """
+        if not hasattr(cls._deletion_tracking, 'pks'):
+            cls._deletion_tracking.pks = set()
+        cls._deletion_tracking.pks.add(pk)
+
+    @classmethod
+    def _untrack_deletion(cls, pk):
+        if hasattr(cls._deletion_tracking, 'pks'):
+            cls._deletion_tracking.pks.discard(pk)
 
     @classmethod
     def _is_being_deleted(cls, pk):

netbox/dcim/signals.py

@@ -318,26 +318,67 @@ def update_passthrough_port_paths(instance, **kwargs):
 
 
 @receiver(post_delete, sender=CableTermination)
-def nullify_connected_endpoints(instance, **kwargs):
+def nullify_connected_endpoints(instance, using, **kwargs):
     """
     Disassociate the Cable from the termination object, and retrace any affected CablePaths.
     """
     model = instance.termination_type.model_class()
-    model.objects.filter(pk=instance.termination_id).update(
-        cable=None,
-        cable_end=None,
-        cable_connector=None,
-        cable_positions=None,
-    )
 
-    # If the removed termination was a channelized interface, also clear the cable attributes mirrored onto its channel
-    # subinterfaces. This must happen before the retrace below so that each channel's (now dead) path is torn down
-    # rather than rebuilt from a stale cable reference.
-    if model is Interface:
-        Interface.objects.filter(parent_id=instance.termination_id, channel_id__isnull=False).update(
-            cable=None, cable_end='', cable_connector=None, cable_positions=None
+    # Deleting a Cable deletes its terminations in bulk, bypassing CableTermination.delete() and the
+    # change-logged clear it performs on the terminating object; do the same here so the disconnect is
+    # recorded. `cable` is a SET_NULL FK which the deletion collector has already nulled by now, so the
+    # pre-delete values are restored before snapshotting, or the record would show no change. They are
+    # restored field by field rather than via set_cable_termination(), whose Interface override would
+    # propagate the cable back onto the channel subinterfaces we are about to clear.
+    termination = None
+    if Cable._is_being_deleted(instance.cable_id):
+        # The change record serializes the object twice (before and after), and ComponentModel.save()
+        # re-caches its denormalized references off the parent device: fetch both up front so neither
+        # costs a round trip per terminating object. Both the read and the save below use the alias the
+        # deletion ran on, so routing can't return a stale row or miss the object entirely.
+        queryset = model.objects.using(using).filter(pk=instance.termination_id).prefetch_related('tags')
+        if hasattr(model, 'device'):
+            queryset = queryset.select_related('device__site', 'device__location', 'device__rack')
+        termination = queryset.first()
+
+    if termination is not None:
+        termination.cable_id = instance.cable_id
+        termination.cable_end = instance.cable_end
+        termination.cable_connector = instance.connector
+        termination.cable_positions = instance.positions
+        termination.snapshot()
+        # clear_cable_termination() also clears the mirrored attributes on any channel subinterfaces
+        termination.clear_cable_termination(instance)
+        update_fields = ['cable', 'cable_end', 'cable_connector', 'cable_positions', 'last_updated']
+
+        # retrace_cable_paths() tears down the originating path once the Cable itself is deleted, clearing
+        # _path outside the changelog. Clear it here so the recorded state doesn't outlive the path.
+        if isinstance(termination, PathEndpoint):
+            termination._path = None
+            update_fields.append('_path')
+
+        # A narrow write: this row was read mid-cascade, and its full save() would pull in unrelated work
+        termination.save(using=using, update_fields=update_fields)
+    else:
+        # Already recorded by CableTermination.delete(), or the terminating object is going away too.
+        model.objects.filter(pk=instance.termination_id).update(
+            cable=None,
+            cable_end=None,
+            cable_connector=None,
+            cable_positions=None,
         )
 
+        # If the removed termination was a channelized interface, also clear the cable attributes mirrored onto
+        # its channel subinterfaces. This must happen before the retrace below so that each channel's (now dead)
+        # path is torn down rather than rebuilt from a stale cable reference. These writes are deliberately not
+        # change-logged, matching propagate_channel_cables(), which doesn't log them when it sets them either.
+        # A subinterface saved through a change-logged path while connected does record its mirrored cable
+        # fields, so its disconnect goes unlogged here; that gap predates this receiver and is untouched by it.
+        if model is Interface:
+            Interface.objects.filter(parent_id=instance.termination_id, channel_id__isnull=False).update(
+                cable=None, cable_end='', cable_connector=None, cable_positions=None
+            )
+
     # If the parent Cable is being deleted in this same operation, skip the
     # per-termination retrace; retrace_cable_paths() will retrace each affected
     # path once after the Cable is deleted.

netbox/dcim/tests/test_models.py

@@ -1,21 +1,25 @@
+import uuid
 from decimal import Decimal
 
 from django.core.exceptions import ValidationError
 from django.db import connection
 from django.db.models import ProtectedError
 from django.db.models.signals import post_save
-from django.test import TestCase, tag
+from django.test import RequestFactory, TestCase, tag
 from django.test.utils import CaptureQueriesContext
 
 from circuits.models import *
-from core.models import ObjectType
+from core.choices import ObjectChangeActionChoices
+from core.models import ObjectChange, ObjectType
 from dcim.choices import *
 from dcim.models import *
 from extras.events import serialize_for_event
 from extras.models import CustomField
 from ipam.models import Prefix
 from netbox.choices import DiameterUnitChoices, FlowRateUnitChoices, WeightUnitChoices
+from netbox.context_managers import event_tracking
 from tenancy.models import Tenant
+from users.models import User
 from utilities.data import drange
 from virtualization.models import Cluster, ClusterType
 
@@ -2922,6 +2926,160 @@ class CableTerminationTestCase(TestCase):
             cable_termination.cache_related_objects()
 
 
+class CableDisconnectChangeLoggingTestCase(TestCase):
+    """
+    Deleting a Cable must change-log the disconnect on each terminating object. Its CableTerminations are
+    deleted by the cascade, so CableTermination.delete() -- which records the disconnect when a termination
+    is removed from a Cable -- never runs.
+    """
+
+    @classmethod
+    def setUpTestData(cls):
+        site = Site.objects.create(name='Test Site 1', slug='test-site-1')
+        manufacturer = Manufacturer.objects.create(name='Test Manufacturer 1', slug='test-manufacturer-1')
+        device_type = DeviceType.objects.create(
+            manufacturer=manufacturer, model='Test Device Type 1', slug='test-device-type-1'
+        )
+        role = DeviceRole.objects.create(name='Test Device Role 1', slug='test-device-role-1')
+        cls.device1 = Device.objects.create(
+            name='Device 1', site=site, device_type=device_type, role=role
+        )
+        cls.device2 = Device.objects.create(
+            name='Device 2', site=site, device_type=device_type, role=role
+        )
+        cls.interface1 = Interface.objects.create(
+            device=cls.device1, name='eth0', type=InterfaceTypeChoices.TYPE_1GE_FIXED
+        )
+        cls.interface2 = Interface.objects.create(
+            device=cls.device2, name='eth0', type=InterfaceTypeChoices.TYPE_1GE_FIXED
+        )
+        cls.interface3 = Interface.objects.create(
+            device=cls.device2, name='eth1', type=InterfaceTypeChoices.TYPE_1GE_FIXED
+        )
+        cls.rear_port = RearPort.objects.create(
+            device=cls.device2, name='Rear Port 1', type=PortTypeChoices.TYPE_8P8C, positions=4
+        )
+        cls.front_port = FrontPort.objects.create(
+            device=cls.device2, name='Front Port 1', type=PortTypeChoices.TYPE_8P8C, positions=4
+        )
+        cls.user = User.objects.create_user(username='testuser')
+
+    def _request(self):
+        request = RequestFactory().get('/')
+        request.id = uuid.uuid4()
+        request.user = self.user
+        return request
+
+    def _connect(self, termination_a, termination_b, **kwargs):
+        with event_tracking(self._request()):
+            cable = Cable(a_terminations=[termination_a], b_terminations=[termination_b], **kwargs)
+            cable.save()
+        return cable
+
+    def _updates(self, obj):
+        return ObjectChange.objects.filter(
+            changed_object_type=ObjectType.objects.get_for_model(obj),
+            changed_object_id=obj.pk,
+            action=ObjectChangeActionChoices.ACTION_UPDATE,
+        ).order_by('time')
+
+    @tag('regression')  # Ref: netbox-branching#631
+    def test_cable_deletion_records_disconnect(self):
+        cable = self._connect(self.interface1, self.interface2)
+        cable_pk = cable.pk
+
+        with event_tracking(self._request()):
+            Cable.objects.get(pk=cable_pk).delete()
+
+        for interface in (self.interface1, self.interface2):
+            # One update for the connect, one for the disconnect
+            self.assertEqual(self._updates(interface).count(), 2, f'No disconnect recorded for {interface}')
+            change = self._updates(interface).last()
+
+            # The cable attributes are cleared in the database before this is recorded, so the pre-change
+            # data is only correct if they were restored before the snapshot was taken
+            self.assertEqual(change.prechange_data['cable'], cable_pk)
+            self.assertIn(change.prechange_data['cable_end'], (CableEndChoices.SIDE_A, CableEndChoices.SIDE_B))
+
+            self.assertIsNone(change.postchange_data['cable'])
+            self.assertIsNone(change.postchange_data['cable_end'])
+            # The originating path is deleted along with the Cable, outside the changelog
+            self.assertIsNone(change.postchange_data['_path'])
+
+            self.assertIsNone(Interface.objects.get(pk=interface.pk).cable_id)
+        self.assertFalse(CablePath.objects.exists())
+
+    def test_profiled_cable_deletion_records_connector_and_positions(self):
+        cable = self._connect(self.interface1, self.rear_port, profile=CableProfileChoices.SINGLE_1C4P)
+        cable_pk = cable.pk
+        connector = Interface.objects.get(pk=self.interface1.pk).cable_connector
+        positions = Interface.objects.get(pk=self.interface1.pk).cable_positions
+        self.assertIsNotNone(connector)
+        self.assertTrue(positions)
+
+        with event_tracking(self._request()):
+            Cable.objects.get(pk=cable_pk).delete()
+
+        change = self._updates(self.interface1).last()
+        self.assertEqual(change.prechange_data['cable_connector'], connector)
+        self.assertEqual(change.prechange_data['cable_positions'], positions)
+        self.assertIsNone(change.postchange_data['cable_connector'])
+        self.assertIsNone(change.postchange_data['cable_positions'])
+
+    def test_queryset_deletion_records_disconnect(self):
+        # A queryset delete never calls Cable.delete(), so the tracking is done by CableQuerySet.delete()
+        cable = self._connect(self.interface1, self.interface2)
+        cable_pk = cable.pk
+
+        with event_tracking(self._request()):
+            Cable.objects.filter(pk=cable_pk).delete()
+
+        change = self._updates(self.interface1).last()
+        self.assertEqual(change.prechange_data['cable'], cable_pk)
+        self.assertIsNone(change.postchange_data['cable'])
+
+    def test_non_path_endpoint_termination(self):
+        # A front port carries no _path of its own; the disconnect is recorded the same way
+        cable = self._connect(self.interface1, self.front_port)
+        cable_pk = cable.pk
+
+        with event_tracking(self._request()):
+            Cable.objects.get(pk=cable_pk).delete()
+
+        change = self._updates(self.front_port).last()
+        self.assertEqual(change.prechange_data['cable'], cable_pk)
+        self.assertIsNone(change.postchange_data['cable'])
+        self.assertIsNone(FrontPort.objects.get(pk=self.front_port.pk).cable_id)
+
+    def test_termination_removal_records_disconnect(self):
+        # Removing a termination from a Cable (rather than deleting the Cable) is recorded by
+        # CableTermination.delete(); the disconnect must not be logged twice or logged as a no-op.
+        cable = self._connect(self.interface1, self.interface2)
+        cable_pk = cable.pk
+
+        request = self._request()
+        with event_tracking(request):
+            cable = Cable.objects.get(pk=cable_pk)
+            cable.b_terminations = [self.interface3]
+            cable.save()
+
+        changes = self._updates(self.interface2).filter(request_id=request.id)
+        self.assertEqual(changes.count(), 1)
+        self.assertEqual(changes[0].prechange_data['cable'], cable_pk)
+        self.assertIsNone(changes[0].postchange_data['cable'])
+
+    def test_terminating_object_deletion_records_no_update(self):
+        # The cascade from deleting the terminating object itself reaches the same handler, but the object
+        # is on its way out: it must be left alone rather than resurrected as an update.
+        self._connect(self.interface1, self.interface2)
+
+        request = self._request()
+        with event_tracking(request):
+            Interface.objects.get(pk=self.interface1.pk).delete()
+
+        self.assertFalse(self._updates(self.interface1).filter(request_id=request.id).exists())
+
+
 class VirtualDeviceContextTestCase(TestCase):
 
     @classmethod