diff --git a/netbox/extras/models/scripts.py b/netbox/extras/models/scripts.py index 02f3902d1..d9a2ca28c 100644 --- a/netbox/extras/models/scripts.py +++ b/netbox/extras/models/scripts.py @@ -3,7 +3,8 @@ import logging from functools import cached_property from django.contrib.contenttypes.fields import GenericRelation -from django.db import models +from django.contrib.contenttypes.models import ContentType +from django.db import models, router, transaction from django.db.models import Q from django.urls import reverse from django.utils.translation import gettext_lazy as _ @@ -11,7 +12,7 @@ from django.utils.translation import gettext_lazy as _ from core.choices import ManagedFileRootPathChoices from core.models import ManagedFile from extras.utils import is_script -from netbox.models.features import EventRulesMixin, JobsMixin +from netbox.models.features import EventRulesMixin, JobsMixin, batch_delete_jobs from utilities.querysets import RestrictedQuerySet from .mixins import PythonModuleMixin @@ -119,6 +120,28 @@ class ScriptModule(PythonModuleMixin, JobsMixin, ManagedFile): def __str__(self): return self.python_name + def delete(self, *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 + + # Deleting a ScriptModule cascades (via the Script.module FK) to its child Scripts, and + # 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) + with transaction.atomic(using=using): + script_type = ContentType.objects.get_for_model(Script, for_concrete_model=False) + child_jobs = Job.objects.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) + delete.alters_data = True + @property def ordered_scripts(self): script_objects = {s.name: s for s in self.scripts.all()} diff --git a/netbox/extras/tests/test_scripts_deletion.py b/netbox/extras/tests/test_scripts_deletion.py index 9aaf11e0c..5b928bd0d 100644 --- a/netbox/extras/tests/test_scripts_deletion.py +++ b/netbox/extras/tests/test_scripts_deletion.py @@ -90,6 +90,30 @@ class ScriptDeletionTestCase(TestCase): self.assertFalse(Script.objects.filter(pk=script.pk).exists()) self.assertEqual(Job.objects.filter(object_type=self.script_ct, object_id=script.pk).count(), 0) + def test_delete_scriptmodule_batches_child_script_jobs(self): + # The reporter's actual path: a script is only removable via the UI by deleting its + # ScriptModule. The module's delete must batch the child Script's jobs. + module, script = self._create_script() + self._add_jobs(script, 5) + + job_delete_calls = [] + original_delete = QuerySet.delete + + def counting_delete(qs, *args, **kwargs): + if qs.model is Job: + job_delete_calls.append(len(qs)) + return original_delete(qs, *args, **kwargs) + + with mock.patch('netbox.models.features.JOB_DELETE_BATCH_SIZE', 2): + with mock.patch.object(QuerySet, 'delete', counting_delete): + module.delete() + + # 5 child-script jobs at a batch size of 2 => three batched deletes (2, 2, 1). The module + # has no jobs of its own, so JobsMixin.delete adds no further Job deletes. + self.assertEqual(job_delete_calls, [2, 2, 1]) + self.assertFalse(Script.objects.filter(pk=script.pk).exists()) + self.assertEqual(Job.objects.filter(object_type=self.script_ct, object_id=script.pk).count(), 0) + def test_delete_datasource_deletes_jobs(self): datasource = DataSource.objects.create(name='DS', type='local', source_url='/tmp/test') self._add_jobs(datasource, 100) diff --git a/netbox/netbox/models/features.py b/netbox/netbox/models/features.py index 8755f7eaf..f608da50c 100644 --- a/netbox/netbox/models/features.py +++ b/netbox/netbox/models/features.py @@ -43,6 +43,7 @@ __all__ = ( 'NotificationsMixin', 'SyncedDataMixin', 'TagsMixin', + 'batch_delete_jobs', 'get_model_features', 'has_feature', 'model_is_public', @@ -449,6 +450,23 @@ class NotificationsMixin(models.Model): abstract = True +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. + """ + from core.models import Job + + 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]): + Job.objects.filter(pk__in=pks).delete() + + class JobsMixin(models.Model): """ Enables support for job results. @@ -464,20 +482,12 @@ class JobsMixin(models.Model): abstract = True def delete(self, *args, **kwargs): - from core.models import Job - - # Delete associated jobs in batches so the cascade never has to load thousands of - # Job rows (each carrying potentially large data/log_entries payloads) into memory - # at once. Wrapped in a transaction so that a failure in the parent delete rolls the - # job deletions back as well. 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 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) with transaction.atomic(using=using): - job_pks = self.jobs.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]): - Job.objects.filter(pk__in=pks).delete() + batch_delete_jobs(self.jobs) return super().delete(*args, **kwargs) delete.alters_data = True