Fixes #23013: Extend the propagation guard to rack and device saves

handle_rack_site_change() and handle_device_site_change() rewrote every
component row beneath the saved object on every save, whether or not the
assignments they propagate had changed — the same defect just fixed for
handle_location_site_change().

Stash the pre-save values for Rack and Device as well, and route all four
handlers through a single _scope_fields_unchanged() helper so the guard
cannot drift between them. The fields each model propagates are declared
in PROPAGATED_SCOPE_FIELDS.

The pre-save read now clears the model's default ordering: Rack orders by
the nullable `location` foreign key, which resolves through the related
model's ordering and adds a LEFT OUTER JOIN that PostgreSQL refuses to
lock ("FOR NO KEY UPDATE cannot be applied to the nullable side of an
outer join"). A single row selected by primary key needs no ordering.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

netbox/dcim/signals.py

@@ -53,13 +53,29 @@ COMPONENT_MODELS = (
     RearPort,
 )
 
+# The scope-relevant fields stashed before each model's save by cache_presave_scope_fields(),
+# so that the post_save handlers can tell whether the save actually changed any of them and
+# skip their work when it did not.
+#
+# Two kinds of handler read the stash, and each entry must cover the trigger fields of every
+# handler that reads it: the propagation handlers below, which push a Location's, Rack's, or
+# Device's scope down to its related objects, and sync_cached_scope_fields(), which rebuilds
+# the CachedScopeMixin caches affected by a Site or Location change. Site is therefore listed
+# for the latter alone (nothing propagates a Site's own scope downwards), while Location's
+# entry serves both. Narrowing an entry silently tightens the skip condition of every handler
+# reading it, so weigh both consumers before editing one.
+STASHED_SCOPE_FIELDS = {
+    Site: ('region_id', 'group_id'),
+    Location: ('site_id',),
+    Rack: ('site_id', 'location_id'),
+    Device: ('site_id', 'location_id', 'rack_id'),
+}
+
 
 #
 # Location/rack/device assignment
 #
 
-@receiver(pre_save, sender=Location)
-@receiver(pre_save, sender=Site)
 def cache_presave_scope_fields(instance, raw=False, using=None, **kwargs):
     """
     Stash the scope-relevant field values currently in the database so that the post_save
@@ -67,21 +83,28 @@ def cache_presave_scope_fields(instance, raw=False, using=None, **kwargs):
     locks the row, so overlapping saves of the same object serialize here and the
     comparison always runs against the final committed state.
 
-    Outside of a transaction no stash is taken (and any stash left by a previous
-    transactional save of the same instance is cleared): in autocommit, this read and the
-    subsequent UPDATE would run in separate transactions, so the comparison could race a
-    concurrent save. The post_save handlers treat a missing stash as "the values may have
+    No stash is taken for a raw save, for a new instance, or outside a transaction: in
+    autocommit, this read and the subsequent UPDATE would run in separate transactions, so
+    the comparison could race a concurrent save. In each of those cases any stash left by a
+    previous save of the same instance is cleared, as it no longer reflects the current
+    database state. The post_save handlers treat a missing stash as "the values may have
     changed" and rebuild or repair unconditionally.
     """
-    if raw or instance.pk is None:
-        return
-    if not transaction.get_connection(using).in_atomic_block:
+    if raw or instance.pk is None or not transaction.get_connection(using).in_atomic_block:
+        # Clear any stash left by a previous save of this instance: a stale snapshot would
+        # be compared against instead of the current database state.
         instance._presave_scope_fields = None
         return
-    fields = ('region_id', 'group_id') if isinstance(instance, Site) else ('site_id',)
+    fields = STASHED_SCOPE_FIELDS[instance.__class__]
     instance._presave_scope_fields = (
         instance.__class__.objects.using(using)
         .filter(pk=instance.pk)
+        # A single row is selected by primary key, so the model's default ordering is
+        # meaningless here — and must be cleared: ordering by a nullable foreign key (as
+        # Rack does) resolves through the related model's own ordering and adds a LEFT
+        # OUTER JOIN, which PostgreSQL refuses to lock ("FOR NO KEY UPDATE cannot be
+        # applied to the nullable side of an outer join").
+        .order_by()
         # no_key: serializes overlapping saves of this object without blocking foreign
         # key inserts that reference it
         .select_for_update(no_key=True)
@@ -90,6 +113,30 @@ def cache_presave_scope_fields(instance, raw=False, using=None, **kwargs):
     )
 
 
+# Connected from the map rather than through a stack of @receiver decorators, so that the two
+# cannot drift apart: a receiver registered for a model with no entry here would raise a
+# KeyError on every save of it, and an entry with no receiver would leave the handlers that
+# read its stash finding none and doing their work unconditionally.
+for _model in STASHED_SCOPE_FIELDS:
+    pre_save.connect(cache_presave_scope_fields, sender=_model)
+
+
+def _scope_fields_unchanged(instance):
+    """
+    Return True when the values stashed immediately before this save show that it changed
+    none of the scope-relevant fields listed for the instance's model, meaning the caller's
+    propagation or rebuild can be skipped in its entirety.
+
+    A missing stash means the values may have changed — cache_presave_scope_fields()
+    deliberately takes none for a raw save, for a new instance, or outside a transaction —
+    so the caller must do its work unconditionally.
+    """
+    prev = getattr(instance, '_presave_scope_fields', None)
+    if prev is None:
+        return False
+    return all(value == getattr(instance, field) for field, value in prev.items())
+
+
 @receiver(post_save, sender=Location)
 def handle_location_site_change(instance, created, using=None, **kwargs):
     """
@@ -110,13 +157,9 @@ def handle_location_site_change(instance, created, using=None, **kwargs):
     if created:
         return
 
-    # Skip the propagation when this save left the Site assignment untouched. The pre-save
-    # value is read from the database by cache_presave_scope_fields() immediately before the
-    # write, with the row locked, so the comparison holds even when overlapping saves race on
-    # the same object. The stash exists only for saves made inside a transaction; when it's
-    # absent (autocommit saves), propagate unconditionally.
-    prev = getattr(instance, '_presave_scope_fields', None)
-    if prev is not None and prev['site_id'] == instance.site_id:
+    # Skip the propagation when this save left the Site assignment untouched: everything
+    # written below is derived from it.
+    if _scope_fields_unchanged(instance):
         return
 
     with transaction.atomic(using=using, savepoint=False):
@@ -188,18 +231,27 @@ def handle_rack_site_change(instance, created, using=None, **kwargs):
     Update child Devices if Site or Location assignment has changed. Queries are pinned to
     the connection the Rack was saved on, and the new values are assigned by ID so that no
     related object is fetched over a router-selected connection.
+
+    A save which changed neither assignment propagates nothing and is skipped.
     """
-    if not created:
-        Device.objects.using(using).filter(rack=instance).update(
-            site_id=instance.site_id,
-            location_id=instance.location_id,
+    if created:
+        return
+
+    # Skip the propagation when this save left the Site and Location assignments untouched:
+    # everything written below is derived from them.
+    if _scope_fields_unchanged(instance):
+        return
+
+    Device.objects.using(using).filter(rack=instance).update(
+        site_id=instance.site_id,
+        location_id=instance.location_id,
+    )
+    # Update component models for devices in this rack
+    for model in COMPONENT_MODELS:
+        model.objects.using(using).filter(device__rack=instance).update(
+            _site_id=instance.site_id,
+            _location_id=instance.location_id,
         )
-        # Update component models for devices in this rack
-        for model in COMPONENT_MODELS:
-            model.objects.using(using).filter(device__rack=instance).update(
-                _site_id=instance.site_id,
-                _location_id=instance.location_id,
-            )
 
 
 @receiver(post_save, sender=Device)
@@ -208,14 +260,23 @@ def handle_device_site_change(instance, created, using=None, **kwargs):
     Update child components to update the parent Site, Location, and Rack when a Device is saved.
     Queries are pinned to the connection the Device was saved on, and the new values are
     assigned by ID so that no related object is fetched over a router-selected connection.
+
+    A save which changed none of the three assignments propagates nothing and is skipped.
     """
-    if not created:
-        for model in COMPONENT_MODELS:
-            model.objects.using(using).filter(device=instance).update(
-                _site_id=instance.site_id,
-                _location_id=instance.location_id,
-                _rack_id=instance.rack_id,
-            )
+    if created:
+        return
+
+    # Skip the propagation when this save left the Site, Location, and Rack assignments
+    # untouched: everything written below is derived from them.
+    if _scope_fields_unchanged(instance):
+        return
+
+    for model in COMPONENT_MODELS:
+        model.objects.using(using).filter(device=instance).update(
+            _site_id=instance.site_id,
+            _location_id=instance.location_id,
+            _rack_id=instance.rack_id,
+        )
 
 
 #
@@ -397,19 +458,9 @@ def sync_cached_scope_fields(instance, created, using=None, **kwargs):
     else:
         return
 
-    # Skip the rebuild when this save changed no scope-relevant field. The pre-save values
-    # are read from the database by cache_presave_scope_fields() immediately before the
-    # write, with the row locked, so the comparison holds even when overlapping saves race
-    # on the same object. The stash exists only for saves made inside a transaction; when
-    # it's absent (autocommit saves), rebuild unconditionally.
-    prev = getattr(instance, '_presave_scope_fields', None)
-    if prev is not None:
-        if isinstance(instance, Site):
-            if prev['region_id'] == instance.region_id and prev['group_id'] == instance.group_id:
-                return
-        # The dispatch above ensures the instance can only be a Location here
-        elif prev['site_id'] == instance.site_id:
-            return
+    # Skip the rebuild when this save changed no scope-relevant field.
+    if _scope_fields_unchanged(instance):
+        return
 
     # These models are explicitly listed because they all subclass CachedScopeMixin
     # and therefore require their cached scope fields to be recomputed.

netbox/dcim/tests/test_signals.py

@@ -35,13 +35,39 @@ from utilities.testing import PinnedConnectionRouter
 from virtualization.models import Cluster, ClusterType
 from wireless.models import WirelessLAN
 
+COMPONENT_TABLES = frozenset(model._meta.db_table for model in signals.COMPONENT_MODELS)
 
-class LocationSiteChangeSignalTestCase(TestCase):
+
+class ScopePropagationCaptureMixin:
+    """
+    Helper for asserting whether a save propagated to the tables its post_save handler
+    rewrites.
+
+    dcim_cabletermination is never among them: the denormalized-field registry
+    (netbox.denormalized) rewrites it on Location, Rack, and Device saves alike, so it
+    cannot distinguish a propagation from a plain save. Neither is the saved object's own
+    table, which carries the save's own UPDATE.
+    """
+    propagation_tables = frozenset()
+
+    def capture_propagation_updates(self, obj):
+        with CaptureQueriesContext(connection) as ctx:
+            obj.save()
+
+        return {
+            table for table in self.propagation_tables
+            for q in ctx.captured_queries
+            if q['sql'].startswith(f'UPDATE "{table}"')
+        }
+
+
+class LocationSiteChangeSignalTestCase(ScopePropagationCaptureMixin, TestCase):
     """
     Verify dcim.signals.handle_location_site_change propagates a Location's new Site to
     every descendant Location, Rack, Device, PowerPanel, and component when the parent
     Location's site assignment changes.
     """
+    propagation_tables = COMPONENT_TABLES | {'dcim_rack', 'dcim_device', 'dcim_powerpanel'}
 
     @classmethod
     def setUpTestData(cls):
@@ -125,27 +151,6 @@ class LocationSiteChangeSignalTestCase(TestCase):
         # Should not raise — newly-created locations have no descendants.
         Location.objects.create(name='New', slug='new', site=self.site_a)
 
-    def _capture_propagation_updates(self, location):
-        """
-        Save the Location and return the UPDATE statements it issued against the tables this
-        handler propagates to. dcim_cabletermination and dcim_location are excluded: the
-        denormalized-field registry (netbox.denormalized) rewrites the former's _location on
-        every Location save, and the latter carries the saved row's own UPDATE, so neither
-        distinguishes a propagation from a plain save.
-        """
-        tables = ('dcim_rack', 'dcim_device', 'dcim_powerpanel', *(
-            model._meta.db_table for model in signals.COMPONENT_MODELS
-        ))
-
-        with CaptureQueriesContext(connection) as ctx:
-            location.save()
-
-        return {
-            table for table in tables
-            for q in ctx.captured_queries
-            if q['sql'].startswith(f'UPDATE "{table}"')
-        }
-
     def _seed_location_with_children(self):
         location = Location.objects.create(name='Parent', slug='parent', site=self.site_a)
         device = Device.objects.create(
@@ -169,7 +174,7 @@ class LocationSiteChangeSignalTestCase(TestCase):
         location = self._seed_location_with_children()
         location.description = 'updated'
 
-        self.assertEqual(self._capture_propagation_updates(location), set())
+        self.assertEqual(self.capture_propagation_updates(location), set())
 
     def test_changed_site_propagates(self):
         # Counterpart to the test above, which would pass vacuously if these UPDATEs stopped
@@ -177,11 +182,29 @@ class LocationSiteChangeSignalTestCase(TestCase):
         location = self._seed_location_with_children()
         location.site = self.site_b
 
-        updated_tables = self._capture_propagation_updates(location)
+        self.assertEqual(self.capture_propagation_updates(location), self.propagation_tables)
 
-        self.assertEqual(updated_tables, {'dcim_rack', 'dcim_device', 'dcim_powerpanel', *(
-            model._meta.db_table for model in signals.COMPONENT_MODELS
-        )})
+    def test_raw_save_does_not_reuse_a_previous_saves_stash(self):
+        # A raw save takes no stash of its own, so it must clear the one left by the previous
+        # save of the same instance: comparing against a snapshot of the database as it stood
+        # before an earlier write can report the propagated fields as unchanged when they are
+        # not, skipping a propagation the handler has no basis to rule out.
+        location = self._seed_location_with_children()
+        location.save()
+        self.assertIsNotNone(location._presave_scope_fields)
+
+        with CaptureQueriesContext(connection) as ctx:
+            location.save_base(raw=True)
+
+        self.assertIsNone(location._presave_scope_fields)
+        self.assertEqual(
+            {
+                table for table in self.propagation_tables
+                for q in ctx.captured_queries
+                if q['sql'].startswith(f'UPDATE "{table}"')
+            },
+            self.propagation_tables,
+        )
 
 
 class LocationSiteChangeAutocommitTestCase(TransactionTestCase):
@@ -223,11 +246,12 @@ class LocationSiteChangeAutocommitTestCase(TransactionTestCase):
         self.assertEqual(interface._site, site)
 
 
-class RackSiteChangeSignalTestCase(TestCase):
+class RackSiteChangeSignalTestCase(ScopePropagationCaptureMixin, TestCase):
     """
     Verify dcim.signals.handle_rack_site_change propagates a Rack's site/location to its
-    Devices and their components when the Rack is moved.
+    Devices and their components when the Rack is moved, and only then.
     """
+    propagation_tables = COMPONENT_TABLES | {'dcim_device'}
 
     @classmethod
     def setUpTestData(cls):
@@ -260,6 +284,86 @@ class RackSiteChangeSignalTestCase(TestCase):
         self.assertEqual(interface._site, self.site_b)
         self.assertEqual(interface._location, self.location_b)
 
+    def _seed_rack_with_devices(self):
+        rack = Rack.objects.create(name='Rack', site=self.site_a)
+        device = Device.objects.create(
+            name='Device',
+            site=self.site_a,
+            rack=rack,
+            device_type=self.device_type,
+            role=self.device_role,
+        )
+        Interface.objects.create(device=device, name='Interface 1')
+        return rack
+
+    def test_unchanged_scope_skips_propagation(self):
+        # Both values the handler writes are derived from the Rack's site and location
+        # assignments, so a save which leaves both alone must not rewrite a single device or
+        # component row.
+        rack = self._seed_rack_with_devices()
+        rack.description = 'updated'
+
+        self.assertEqual(self.capture_propagation_updates(rack), set())
+
+    def test_changed_site_propagates(self):
+        # Counterpart to the test above, which would pass vacuously if these UPDATEs stopped
+        # being issued (or their tables were renamed) rather than merely being skipped.
+        rack = self._seed_rack_with_devices()
+        rack.site = self.site_b
+
+        self.assertEqual(self.capture_propagation_updates(rack), self.propagation_tables)
+
+    def test_changed_location_propagates(self):
+        # Location moves within the same Site must propagate too: the guard covers both
+        # fields, not just the Site.
+        rack = self._seed_rack_with_devices()
+        rack.site = self.site_b
+        rack.save()
+        rack.location = self.location_b
+
+        self.assertEqual(self.capture_propagation_updates(rack), self.propagation_tables)
+
+
+class StashedScopeFieldsRegistrationTestCase(TestCase):
+    """
+    Verify cache_presave_scope_fields() is connected for every model in
+    signals.STASHED_SCOPE_FIELDS, and that each entry's fields resolve. An entry whose
+    receiver was never connected would leave the post_save handlers reading its stash
+    finding none, and doing their work unconditionally on every save.
+    """
+
+    @classmethod
+    def setUpTestData(cls):
+        cls.instances = {}
+        site = Site.objects.create(name='Site', slug='site')
+        location = Location.objects.create(name='Location', slug='location', site=site)
+        rack = Rack.objects.create(name='Rack', site=site, location=location)
+        manufacturer = Manufacturer.objects.create(name='Manufacturer', slug='manufacturer')
+        cls.instances = {
+            Site: site,
+            Location: location,
+            Rack: rack,
+            Device: Device.objects.create(
+                name='Device',
+                site=site,
+                location=location,
+                rack=rack,
+                device_type=DeviceType.objects.create(manufacturer=manufacturer, model='Device Type'),
+                role=DeviceRole.objects.create(name='Device Role', slug='device-role'),
+            ),
+        }
+
+    def test_every_mapped_model_stashes_its_fields_on_save(self):
+        # TestCase wraps each test in a transaction, so every save below takes a stash.
+        self.assertEqual(set(self.instances), set(signals.STASHED_SCOPE_FIELDS))
+
+        for model, fields in signals.STASHED_SCOPE_FIELDS.items():
+            with self.subTest(model=model.__name__):
+                instance = self.instances[model]
+                instance.save()
+
+                self.assertEqual(instance._presave_scope_fields.keys(), set(fields))
+
 
 class ScopeSignalConnectionTestCase(TestCase):
     """
@@ -382,11 +486,12 @@ class ScopeSignalConnectionTestCase(TestCase):
         self.assertEqual(cluster._region, region)
 
 
-class DeviceSiteChangeSignalTestCase(TestCase):
+class DeviceSiteChangeSignalTestCase(ScopePropagationCaptureMixin, TestCase):
     """
     Verify dcim.signals.handle_device_site_change propagates a Device's site/location/rack
-    to its components on save.
+    to its components on save, and only then.
     """
+    propagation_tables = COMPONENT_TABLES
 
     @classmethod
     def setUpTestData(cls):
@@ -412,6 +517,44 @@ class DeviceSiteChangeSignalTestCase(TestCase):
         interface.refresh_from_db()
         self.assertEqual(interface._site, self.site_b)
 
+    def _seed_device_with_components(self):
+        device = Device.objects.create(
+            name='Device',
+            site=self.site_a,
+            device_type=self.device_type,
+            role=self.device_role,
+        )
+        Interface.objects.create(device=device, name='Interface 1')
+        return device
+
+    def test_unchanged_scope_skips_propagation(self):
+        # Components repopulate _site/_location/_rack from their Device on their own save
+        # (see ComponentModel.save), so a Device save which moved the Device nowhere has
+        # nothing to push down and must not rewrite a single component row.
+        device = self._seed_device_with_components()
+        device.description = 'updated'
+
+        self.assertEqual(self.capture_propagation_updates(device), set())
+
+    def test_changed_site_propagates(self):
+        # Counterpart to the test above, which would pass vacuously if these UPDATEs stopped
+        # being issued (or their tables were renamed) rather than merely being skipped.
+        device = self._seed_device_with_components()
+        device.site = self.site_b
+
+        self.assertEqual(self.capture_propagation_updates(device), self.propagation_tables)
+
+    def test_changed_rack_propagates(self):
+        # A Rack assignment is the third guarded field, and the only one changed here: the
+        # Rack is deliberately left without a Location, so Device.save() does not inherit one
+        # and neither site nor location moves.
+        device = self._seed_device_with_components()
+        rack = Rack.objects.create(name='Rack', site=self.site_a)
+        self.assertIsNone(rack.location)
+        device.rack = rack
+
+        self.assertEqual(self.capture_propagation_updates(device), self.propagation_tables)
+
 
 class VirtualChassisMasterSignalTestCase(TestCase):
     """