From f2923f4ce4c2a741be823fd3d2f2f345b0ef1bb7 Mon Sep 17 00:00:00 2001 From: Jason Novinger Date: Wed, 5 Aug 2026 13:56:01 -0500 Subject: [PATCH] Fixes #22812: Batch child-script job deletion when deleting a ScriptModule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deleting a Script via the UI is only possible by deleting its parent ScriptModule (no Script delete view exists). That cascades to the child Script rows, and the collector materialized every one of those Scripts' jobs — the memory blowup, which scales with jobs-per-script. JobsMixin.delete() only batched the deleted object's own jobs, and a ScriptModule has none; the jobs live on its child Scripts. Extract the chunked job-deletion loop from JobsMixin.delete() into a shared batch_delete_jobs() helper, and add a ScriptModule.delete() override that batch-deletes its child Scripts' jobs (in a single queryset keyed on the script PKs, no per-script loop) before delegating to the cascade. This bounds peak memory to one batch regardless of how many jobs the module's scripts hold. --- netbox/extras/models/scripts.py | 27 ++++++++++++++-- netbox/extras/tests/test_scripts_deletion.py | 24 ++++++++++++++ netbox/netbox/models/features.py | 34 +++++++++++++------- 3 files changed, 71 insertions(+), 14 deletions(-) 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