Fixes #22812: Address review — DB alias, batch size, MRO note

- batch_delete_jobs now writes through the same DB alias it reads from. In JobsMixin.delete
  the read queryset is bound to the instance's DB while Job.objects would use the router
  default; if those diverged on a multi-DB setup the deleted rows never left the read side
  and the batch loop never terminated.
- JobsMixin.delete and ScriptModule.delete honor a caller-supplied `using`, matching
  DeleteMixin.delete, instead of always recomputing it.
- Raise JOB_DELETE_BATCH_SIZE from 100 to 1000 and correct its rationale. With only('pk')
  the per-batch payload is gone, so the size now bounds per-cycle work rather than memory;
  1000 matches EXPORT_CHUNK_SIZE and was the fastest of 100/1000/5000 when benchmarked
  against a 200k-job deletion.
- Document that JobsMixin must precede DeleteMixin in the MRO or the batching is bypassed,
  and scope the ScriptModule.delete comment so it doesn't imply the on-disk file removal is
  transactional.
- Add a module-path rollback test alongside the existing script-path one.

netbox/extras/models/scripts.py

@@ -120,7 +120,7 @@ class ScriptModule(PythonModuleMixin, JobsMixin, ManagedFile):
     def __str__(self):
         return self.python_name
 
-    def delete(self, *args, **kwargs):
+    def delete(self, using=None, *args, **kwargs):
         # Job is imported here rather than at module level to avoid a circular import
         # (core.models.jobs -> core.signals -> extras.events -> extras.models -> this module).
         from core.models import Job
@@ -129,17 +129,18 @@ class ScriptModule(PythonModuleMixin, JobsMixin, ManagedFile):
         # Django's collector would materialize every one of those Scripts' Jobs to delete them.
         # A module's scripts can accumulate thousands of jobs, exhausting memory. Batch-delete
         # the child Scripts' jobs up front, in a single queryset (no per-script loop), before
-        # delegating to the cascade. Wrapped in a transaction so a failure in the parent delete
-        # rolls these deletions back as well. See #22812.
-        using = router.db_for_write(self.__class__, instance=self)
+        # delegating to the cascade. The transaction rolls the job deletions back if the parent
+        # delete fails; note it does not cover ManagedFile.delete removing the file from disk,
+        # which happens before the DB delete and is not transactional. See #22812.
+        using = using or router.db_for_write(self.__class__, instance=self)
         with transaction.atomic(using=using):
             script_type = ContentType.objects.get_for_model(Script, for_concrete_model=False)
-            child_jobs = Job.objects.filter(
+            child_jobs = Job.objects.using(using).filter(
                 object_type=script_type,
                 object_id__in=self.scripts.values_list('pk', flat=True),
             )
             batch_delete_jobs(child_jobs)
-            return super().delete(*args, **kwargs)
+            return super().delete(using, *args, **kwargs)
     delete.alters_data = True
 
     @property

netbox/extras/tests/test_scripts_deletion.py

@@ -151,6 +151,21 @@ class ScriptDeletionTestCase(TestCase):
         self.assertTrue(Script.objects.filter(pk=script.pk).exists())
         self.assertEqual(Job.objects.filter(object_type=self.script_ct, object_id=script.pk).count(), 10)
 
+    @override_settings(PROTECTION_RULES={'extras.script': [CustomValidator({'name': {'eq': ''}})]})
+    def test_delete_scriptmodule_rolls_back_child_jobs_on_failure(self):
+        # Same abort path via the module: the protection rule fires when the cascade pre_deletes
+        # the child Script, after ScriptModule.delete has already batch-deleted that script's jobs.
+        # The transaction must roll those job deletions back, leaving no orphaned partial state.
+        module, script = self._create_script()
+        self._add_jobs(script, 10)
+
+        with self.assertRaises(AbortRequest):
+            module.delete()
+
+        self.assertTrue(ScriptModule.objects.filter(pk=module.pk).exists())
+        self.assertTrue(Script.objects.filter(pk=script.pk).exists())
+        self.assertEqual(Job.objects.filter(object_type=self.script_ct, object_id=script.pk).count(), 10)
+
 
 class ConfirmCollectorTestCase(TestCase):
     """

netbox/netbox/constants.py

@@ -75,7 +75,9 @@ CENSOR_TOKEN_CHANGED = '***CHANGED***'
 # Placeholder text for empty tables
 EMPTY_TABLE_TEXT = 'No results found'
 
-# Batch size for deleting a JobsMixin object's associated jobs during cascade deletion.
-# Kept small because each Job carries potentially large data/log_entries payloads and
-# cannot be fast-deleted (a global pre_delete receiver forces per-instance signals). See #22812.
-JOB_DELETE_BATCH_SIZE = 100
+# Batch size for deleting a JobsMixin object's associated jobs during cascade deletion. Job
+# cannot be fast-deleted (a global pre_delete receiver forces per-instance signals), so deleting
+# in chunks bounds the work per delete cycle rather than building one huge collection and running
+# one long DELETE. 1000 matches EXPORT_CHUNK_SIZE and, in benchmarking a 200k-job deletion, was
+# the fastest of 100/1000/5000 while keeping peak memory flat. See #22812.
+JOB_DELETE_BATCH_SIZE = 1000

netbox/netbox/models/features.py

@@ -452,27 +452,37 @@ class NotificationsMixin(models.Model):
 
 def batch_delete_jobs(job_queryset):
     """
-    Delete the Jobs in `job_queryset` in JOB_DELETE_BATCH_SIZE chunks, so the caller never has
-    to load thousands of Job rows (each carrying potentially large data/log_entries payloads)
-    into memory at once. Callers are responsible for wrapping this in a transaction. As with the
-    prior cascade behavior, this bulk delete does not invoke Job.delete() and therefore does not
-    cancel the backing RQ job. See #22812.
+    Delete the Jobs in `job_queryset` in JOB_DELETE_BATCH_SIZE chunks. Job cannot be fast-deleted
+    (a global pre_delete receiver forces per-instance signals), so a single delete would build one
+    huge collection of Job instances and run one very long DELETE; batching bounds the per-cycle
+    work. Callers are responsible for wrapping this in a transaction. As with the prior cascade
+    behavior, this bulk delete does not invoke Job.delete() and therefore does not cancel the
+    backing RQ job. See #22812.
     """
     from core.models import Job
 
+    # Route writes to the same database the queryset reads from. In JobsMixin.delete the queryset
+    # is bound to the instance's DB while Job.objects would otherwise use the router default; if
+    # those diverge the deleted rows never leave the read side and the loop below never terminates.
+    jobs = Job.objects.using(job_queryset.db)
+
     job_pks = job_queryset.order_by('pk').values_list('pk', flat=True)
     # Re-slice the queryset each iteration: it re-queries after each batch delete, so the
     # remaining set shrinks and the loop terminates (do not hoist this into a cursor).
     while pks := list(job_pks[:JOB_DELETE_BATCH_SIZE]):
-        # only('pk'): the batch still can't fast-delete (a global pre_delete receiver forces
-        # per-instance signals), so each Job in the batch is instantiated. Loading just the PK
-        # avoids pulling the large data/log_entries payloads into those instances.
-        Job.objects.filter(pk__in=pks).only('pk').delete()
+        # only('pk'): the batch still can't fast-delete, so each Job in the batch is instantiated;
+        # loading just the PK avoids pulling the large data/log_entries payloads into memory.
+        jobs.filter(pk__in=pks).only('pk').delete()
 
 
 class JobsMixin(models.Model):
     """
     Enables support for job results.
+
+    Note: for the job-batching in delete() to run, JobsMixin must precede DeleteMixin in a
+    model's MRO. DeleteMixin.delete() drives its own collector and does not call super(), so a
+    model declared as e.g. `class Foo(NetBoxModel, JobsMixin)` would reach DeleteMixin first and
+    bypass the batching. Core models that combine both (e.g. DataSource) list JobsMixin first.
     """
     jobs = GenericRelation(
         to='core.Job',
@@ -484,14 +494,14 @@ class JobsMixin(models.Model):
     class Meta:
         abstract = True
 
-    def delete(self, *args, **kwargs):
+    def delete(self, using=None, *args, **kwargs):
         # Delete associated jobs in batches so the cascade never has to load thousands of Job
         # rows into memory at once. Wrapped in a transaction so that a failure in the parent
         # delete rolls the job deletions back as well. See #22812.
-        using = router.db_for_write(self.__class__, instance=self)
+        using = using or router.db_for_write(self.__class__, instance=self)
         with transaction.atomic(using=using):
-            batch_delete_jobs(self.jobs)
-            return super().delete(*args, **kwargs)
+            batch_delete_jobs(self.jobs.using(using))
+            return super().delete(using, *args, **kwargs)
     delete.alters_data = True
 
     def get_latest_jobs(self):