diff --git a/netbox/extras/models/scripts.py b/netbox/extras/models/scripts.py index d9a2ca28c..042744fc6 100644 --- a/netbox/extras/models/scripts.py +++ b/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 diff --git a/netbox/extras/tests/test_scripts_deletion.py b/netbox/extras/tests/test_scripts_deletion.py index f6d9bad48..7419de359 100644 --- a/netbox/extras/tests/test_scripts_deletion.py +++ b/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): """ diff --git a/netbox/netbox/constants.py b/netbox/netbox/constants.py index fddce0ad9..096ae145b 100644 --- a/netbox/netbox/constants.py +++ b/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 diff --git a/netbox/netbox/models/features.py b/netbox/netbox/models/features.py index 4f1d5b684..1cf46d013 100644 --- a/netbox/netbox/models/features.py +++ b/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):