Fixes #9: Prevent deadlocks between activation and bulk edits

docs/development/netbox-internals.md

@@ -22,9 +22,11 @@ not a list of unsupported APIs alone.
 | `utilities.request.copy_safe_request` | `views/scripts.py`, `api/views.py` | Copies the request for a worker and strips sensitive headers. The copy must be picklable. |
 | `netbox.api.viewsets.mixins.discard_events_on_rollback` | `api/views.py` | Discards request events when a declaration-writing API action rolls back. Never used for disposable-write permission probes. |
 | `netbox.api.viewsets.NetBoxModelViewSet.perform_update` and `perform_destroy` | `api/views.py` | Lets the lock-order mixin acquire the Project lock before NetBox's conditional row lock. That row lock must remain inside these hooks. The canary detects a missing hook, not a change in where it takes the lock. A Script File update also gates and queues its Project's refresh here, which bulk updates reach because NetBox calls the hook once per object. |
+| `netbox.api.viewsets.mixins.BulkUpdateModelMixin.get_bulk_update_queryset` | `api/views.py` | Orders Script bulk updates by primary key to match synchronization. NetBox v4.7.4 and later also lock the whole batch in that order before saving. |
 | `utilities.exceptions.PermissionsViolation` | `permissions.py`, `api/views.py` | Rejects child writes outside the user's object-permission scope through NetBox's enclosing rollback path. |
 | `utilities.exceptions.AbortRequest` | `models/projects.py`, `forms/model_forms/projects.py` | Refuses a form or model save. REST returns 400, and the UI displays the message on the form. Documented in `docs/plugins/development/exceptions.md` and handled by `NetBoxModelViewSet.dispatch`, `ObjectEditView.post` and both bulk views. |
 | `netbox.views.generic.BulkEditView.pre_save_operations` | `views/projects.py` | Runs the source-field permission check before each save in NetBox's bulk-edit transaction. The check reads the undocumented `_nullify` list used by **Set null**. |
+| `netbox.views.generic.BulkEditView.get_queryset` | `views/scripts.py` | Uses primary-key order when applying a Script bulk edit. Checks `_apply`, the undocumented name of the Apply button, to preserve the initial confirmation page's default order. |
 | `utilities.data.normalize_update_fields` | `models/projects.py` | Converts `update_fields` to a frozenset in the forwarded save arguments. Membership checks then cannot exhaust a one-shot iterable before the save. NetBox uses the same pattern. |
 | `utilities.rqworker.get_queue_for_model` | `api/views.py`, `jobs.py` | Selects the configured queue for both worker checks and enqueue operations. |
 | `rq.utils.parse_timeout` | `runtime/introspection.py` | Parses the author's `job_timeout` using RQ's own grammar, including strings such as `"30m"`. |

netbox_scripts/activation.py

@@ -138,10 +138,11 @@ def synchronize_scripts(*, project, revision, records, using):
     """
     Bring one project's Script rows in line with a validated discovery snapshot.
 
-    Database work only, inside the transaction and the project lock the caller holds. A row the
-    snapshot no longer names is retired rather than deleted, keeping its primary key and the Job
-    history on it. A row whose recorded fields already match is left untouched. enabled is never
-    written.
+    Database work only, inside the transaction and the project lock the caller holds. Every
+    existing Script of the project is locked in primary-key order before any write, matching the
+    order used by bulk edits. A row the snapshot no longer names is retired rather than deleted,
+    keeping its primary key and the Job history on it. A row whose recorded fields already match
+    is left untouched. enabled is never written.
 
     Returns a ScriptSyncResult. Inside a request each written row is change logged and queues an
     event attributed to the user who asked for the activation, OBJECT_CREATED for a newly
@@ -150,7 +151,10 @@ def synchronize_scripts(*, project, revision, records, using):
     """
     written = 0
     retired = 0
-    rows = {(row.module_path, row.class_name): row for row in project.scripts.using(using)}
+    rows = {
+        (row.module_path, row.class_name): row
+        for row in project.scripts.using(using).order_by('pk').select_for_update()
+    }
     for record in records:
         identity = (record['module_path'], record['class_name'])
         # script_file_id and script_file_path stay in the revision's snapshot. Which

netbox_scripts/api/views.py

@@ -320,6 +320,10 @@ def initial(self, request, *args, **kwargs):
             # class already applied restricts to what the user may add, and that set is empty.
             self.queryset = type(self).queryset.restrict(request.user, 'run')
 
+    def get_bulk_update_queryset(self):
+        """Return Scripts in primary-key order to match synchronization."""
+        return super().get_bulk_update_queryset().order_by('pk')
+
     # http_method_names is checked on every dispatch, so the action declares its own as an
     # initkwarg, which applies to this route alone and leaves POST refused on the list route.
     @extend_schema(

netbox_scripts/models/scripts.py

@@ -253,6 +253,8 @@ def save(self, *args, **kwargs):
             original = (
                 type(self)
                 .objects.using(using)
+                # The default ordering joins the Project, and FOR UPDATE would lock that row too.
+                .order_by('pk')
                 .select_for_update()
                 .filter(pk=self.pk)
                 .values(*PUBLISHED_SCRIPT_FIELDS)

netbox_scripts/tests/api/test_scripts.py

@@ -1,5 +1,6 @@
 from unittest import mock
 
+from django.db.models.signals import pre_save
 from django.urls import reverse
 from rest_framework import status
 
@@ -17,6 +18,7 @@
     ObjectPermissionTestMixin,
     PluginAPIViewTestCase,
     PluginAPIViewTestCases,
+    create_scripts_out_of_default_order,
     refresh_count,
 )
 from utilities.api import get_serializer_for_model
@@ -268,6 +270,24 @@ def test_patching_requires_the_change_permission(self):
         )
         self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN)
 
+    def test_a_bulk_update_saves_in_primary_key_order(self):
+        pks = create_scripts_out_of_default_order()
+        saved = []
+
+        def record_save(sender, instance, **kwargs):
+            saved.append(instance.pk)
+
+        pre_save.connect(record_save, sender=NetBoxScript, dispatch_uid='bulk-update-save-order')
+        self.addCleanup(pre_save.disconnect, record_save, sender=NetBoxScript, dispatch_uid='bulk-update-save-order')
+        self.add_permissions('netbox_scripts.view_netboxscript', 'netbox_scripts.change_netboxscript')
+
+        response = self.client.patch(
+            self._get_list_url(), [{'id': pk, 'enabled': False} for pk in pks[::-1]], format='json', **self.header
+        )
+
+        self.assertEqual(response.status_code, status.HTTP_200_OK)
+        self.assertEqual(saved, pks)
+
 
 FILE_DIGEST = 'c' * 64
 

netbox_scripts/tests/plugin_testing.py

@@ -7,6 +7,7 @@
 
 from core.models import Job, ObjectType
 from netbox_scripts.jobs import ProjectScriptFileRefreshJob
+from netbox_scripts.models import NetBoxScript, ScriptProject
 from netbox_scripts.runtime.naming import PRIVATE_ROOT
 from netbox_scripts.storage.locks import advisory_key
 from users.models import ObjectPermission
@@ -44,6 +45,16 @@ def refresh_count(project):
     return Job.objects.filter(name=ProjectScriptFileRefreshJob.name, data__project_id=project.pk).count()
 
 
+def create_scripts_out_of_default_order():
+    """Create Scripts out of default order, interleaving two Projects, and return their primary keys."""
+    zeta = ScriptProject.objects.create(name='Zeta Project', key='zeta-project')
+    alpha = ScriptProject.objects.create(name='Alpha Project', key='alpha-project')
+    return [
+        NetBoxScript.objects.create(project=project, module_path='deploy', class_name=name, display_name=name).pk
+        for project, name in ((zeta, 'Zeta'), (alpha, 'Mid'), (zeta, 'Alpha'))
+    ]
+
+
 class SecondSession:
     """
     A separate database session, used to observe what this connection holds.

netbox_scripts/tests/storage/test_concurrency.py

@@ -21,6 +21,7 @@
 from django.test import TransactionTestCase, override_settings
 
 from core.models import Job
+from netbox_scripts.activation import activate_revision
 from netbox_scripts.choices import RevisionStatusChoices
 from netbox_scripts.jobs import ProjectStorageCleanupJob
 from netbox_scripts.models import NetBoxScript, ScriptFile, ScriptProject, ScriptProjectRevision
@@ -29,6 +30,7 @@
 from netbox_scripts.storage.manifest import compute_digest
 from netbox_scripts.storage.paths import revision_prefix
 from netbox_scripts.tests.plugin_testing import IN_MEMORY_STORAGES, SecondSession
+from netbox_scripts.tests.test_activation import record
 from utilities.testing.mixins import RQQueueTestMixin
 
 SOURCE = {'hello.py': b'print("hi")\n'}
@@ -70,7 +72,7 @@ def cleanup(self, digest, paths):
 
 
 class ScriptEditLockScopeTestCase(SerializationTestCase):
-    """An ordinary Script edit serializes on its own row, never on the project."""
+    """Script edits lock only their own row, and activation locks every Script of its Project before writing."""
 
     def observe_project_lock_during_save(self, instance):
         """Save one row and report whether another session could take the project lock meanwhile."""
@@ -102,6 +104,59 @@ def test_a_declaration_still_takes_the_project_lock(self):
 
         self.assertFalse(self.observe_project_lock_during_save(script_file), 'The declaration lost its lock.')
 
+    def test_an_edit_locks_only_its_script_row(self):
+        script = NetBoxScript.objects.create(
+            project=self.project, module_path='deploy', class_name='Deploy', display_name='Deploy'
+        )
+        observed = []
+
+        def observe(sender, instance, **kwargs):
+            with SecondSession() as other:
+                observed.append(
+                    {
+                        'script': other.row_locked(NetBoxScript, instance.pk),
+                        'project': other.row_locked(ScriptProject, self.project.pk),
+                    }
+                )
+
+        pre_save.connect(observe, sender=NetBoxScript, dispatch_uid='script-guard-row-lock')
+        self.addCleanup(pre_save.disconnect, observe, sender=NetBoxScript, dispatch_uid='script-guard-row-lock')
+        script.enabled = False
+        script.save()
+
+        self.assertEqual(observed, [{'script': True, 'project': False}])
+
+    def test_activation_locks_all_existing_scripts_before_writing(self):
+        revision = self.materialize()
+        # Primary keys run against publication order, and Retired is dropped from the snapshot.
+        zeta, alpha, retired = (
+            NetBoxScript.objects.create(project=self.project, module_path='deploy', class_name=name, display_name=name)
+            for name in ('Zeta', 'Alpha', 'Retired')
+        )
+        ScriptProjectRevision.objects.filter(pk=revision.pk).update(
+            status=RevisionStatusChoices.VALID,
+            discovered_scripts=[record(class_name='Alpha'), record(class_name='Zeta', position=1)],
+        )
+        revision.refresh_from_db()
+        other = ScriptProject.objects.create(name='Audit Devices', key='audit-devices')
+        bystander = NetBoxScript.objects.create(
+            project=other, module_path='audit', class_name='Audit', display_name='Audit'
+        )
+        observed = []
+
+        def observe(sender, instance, **kwargs):
+            if not observed:
+                with SecondSession() as session:
+                    observed.append(
+                        {row.pk: session.row_locked(NetBoxScript, row.pk) for row in (zeta, alpha, retired, bystander)}
+                    )
+
+        pre_save.connect(observe, sender=NetBoxScript, dispatch_uid='activation-row-locks')
+        self.addCleanup(pre_save.disconnect, observe, sender=NetBoxScript, dispatch_uid='activation-row-locks')
+        activate_revision(revision)
+
+        self.assertEqual(observed, [{zeta.pk: True, alpha.pk: True, retired.pk: True, bystander.pk: False}])
+
 
 class DeclarationGuardRowLockTestCase(SerializationTestCase):
     """The declaration guard holds the row it read until its own write lands."""

netbox_scripts/tests/test_activation.py

@@ -235,6 +235,13 @@ def test_job_history_survives_a_retirement_and_a_return(self):
         self.sync([record()])
         self.assertEqual(NetBoxScript.objects.get().jobs.count(), 1)
 
+    def test_the_scripts_are_locked_in_primary_key_order_before_any_write(self):
+        self.sync([record(), record(class_name='AuditDevices', position=1)])
+        with CaptureQueriesContext(connection) as captured:
+            self.sync([record(class_name='AuditDevices', display_name='Audit Devices')])
+        first = next(q['sql'] for q in captured.captured_queries if f'"{SCRIPT_TABLE}"' in q['sql'])
+        self.assertTrue(first.endswith(f'ORDER BY "{SCRIPT_TABLE}"."id" ASC FOR UPDATE'), first)
+
 
 class ActivationMixin(StorageServiceMixin):
     """A project with real stored content, plus a way to give a revision recorded scripts."""

netbox_scripts/tests/views/test_scripts.py

@@ -1,6 +1,6 @@
 from django.contrib.auth import get_user_model
 from django.db import connection
-from django.db.models.signals import post_save
+from django.db.models.signals import post_save, pre_save
 from django.test import Client, override_settings
 from django.test.utils import CaptureQueriesContext
 from django.urls import NoReverseMatch, reverse
@@ -9,7 +9,12 @@
 from core.tables import ObjectChangeTable
 from netbox_scripts.models import NetBoxScript, ScriptFile, ScriptProject
 from netbox_scripts.permissions import SELECTION_REFUSAL
-from netbox_scripts.tests.plugin_testing import ObjectPermissionTestMixin, PluginTestCases, refresh_count
+from netbox_scripts.tests.plugin_testing import (
+    ObjectPermissionTestMixin,
+    PluginTestCases,
+    create_scripts_out_of_default_order,
+    refresh_count,
+)
 from users.models import ObjectPermission
 from utilities.testing import TestCase, create_tags, post_data
 
@@ -80,6 +85,34 @@ def publish_between(sender, instance, **kwargs):
         self.assertEqual(clobbered.display_name, 'Renamed By Sync')
         self.assertFalse(clobbered.enabled)
 
+    def test_a_bulk_edit_saves_in_primary_key_order(self):
+        pks = create_scripts_out_of_default_order()
+        saved = []
+
+        def record_save(sender, instance, **kwargs):
+            saved.append(instance.pk)
+
+        pre_save.connect(record_save, sender=NetBoxScript, dispatch_uid='bulk-edit-save-order')
+        self.addCleanup(pre_save.disconnect, record_save, sender=NetBoxScript, dispatch_uid='bulk-edit-save-order')
+        self.add_permissions('netbox_scripts.view_netboxscript', 'netbox_scripts.change_netboxscript')
+
+        response = self.client.post(
+            self._get_url('bulk_edit'),
+            data={'pk': pks[::-1], '_apply': True, **post_data({'enabled': False})},
+        )
+
+        self.assertHttpStatus(response, 302)
+        self.assertEqual(saved, pks)
+
+    def test_the_bulk_edit_confirmation_preserves_default_order(self):
+        pks = create_scripts_out_of_default_order()
+        self.add_permissions('netbox_scripts.view_netboxscript', 'netbox_scripts.change_netboxscript')
+
+        response = self.client.post(self._get_url('bulk_edit'), data={'pk': pks})
+
+        self.assertHttpStatus(response, 200)
+        self.assertEqual([row.record.pk for row in response.context['table'].rows], [pks[1], pks[2], pks[0]])
+
 
 class NetBoxScriptViewTestCase(TestCase):
     """The detail view and the change-log rendering that reverses a Script's URL."""

netbox_scripts/views/scripts.py

@@ -95,6 +95,11 @@ class NetBoxScriptBulkEditView(generic.BulkEditView):
     table = NetBoxScriptTable
     form = NetBoxScriptBulkEditForm
 
+    def get_queryset(self, request):
+        """Return Scripts in primary-key order when a bulk edit is applied, to match synchronization."""
+        queryset = super().get_queryset(request)
+        return queryset.order_by('pk') if '_apply' in request.POST else queryset
+
 
 @register_model_view(NetBoxScript, 'run', path='run')
 class NetBoxScriptRunView(generic.ObjectView):

scripts/check_netbox_internals.py

@@ -45,12 +45,17 @@
         'netbox.api.viewsets:NetBoxModelViewSet.perform_update',
         'netbox.api.viewsets:NetBoxModelViewSet.perform_destroy',
     ),
+    (
+        'netbox.api.viewsets.mixins.BulkUpdateModelMixin.get_bulk_update_queryset',
+        'netbox.api.viewsets.mixins:BulkUpdateModelMixin.get_bulk_update_queryset',
+    ),
     ('utilities.exceptions.PermissionsViolation', 'utilities.exceptions:PermissionsViolation'),
     ('utilities.exceptions.AbortRequest', 'utilities.exceptions:AbortRequest'),
     (
         'netbox.views.generic.BulkEditView.pre_save_operations',
         'netbox.views.generic:BulkEditView.pre_save_operations',
     ),
+    ('netbox.views.generic.BulkEditView.get_queryset', 'netbox.views.generic:BulkEditView.get_queryset'),
     ('utilities.data.normalize_update_fields', 'utilities.data:normalize_update_fields'),
     ('utilities.rqworker.get_queue_for_model', 'utilities.rqworker:get_queue_for_model'),
     ('rq.utils.parse_timeout', 'rq.utils:parse_timeout'),