From 0701a42a9434c47346e7f2300356c141d2f931cb Mon Sep 17 00:00:00 2001 From: Jason Novinger Date: Tue, 4 Aug 2026 16:39:35 -0500 Subject: [PATCH 01/15] Fixes #22812: Avoid loading all jobs into memory when deleting a JobsMixin object Deleting a Script (or any JobsMixin object) with thousands of associated jobs could consume several GB of memory and exhaust the host, because Django's deletion collector loads every related Job into memory. Jobs can never be fast-deleted (a global pre_delete receiver forces per-instance signal dispatch), and each Job carries potentially large data and log_entries payloads. Two paths loaded the full job set independently, so both are addressed: - The delete cascade: JobsMixin.delete() now deletes the object's jobs in batches before delegating to super().delete(), wrapped in a transaction so a failure in the parent delete rolls the job deletions back. After the loop the cascade collector finds no jobs to materialize. - The delete-confirmation page: _get_dependent_objects() uses a ConfirmCollector that counts the jobs relation rather than descending into it, so the page never instantiates the jobs. Counted relations render as a non-expandable row (via a CountOnly stand-in) alongside the itemized dependents. --- netbox/extras/tests/test_scripts_deletion.py | 236 +++++++++++++++++++ netbox/netbox/constants.py | 5 + netbox/netbox/models/deletion.py | 56 +++++ netbox/netbox/models/features.py | 22 +- netbox/netbox/views/generic/object_views.py | 21 +- netbox/templates/htmx/delete_form.html | 53 +++-- 6 files changed, 363 insertions(+), 30 deletions(-) create mode 100644 netbox/extras/tests/test_scripts_deletion.py diff --git a/netbox/extras/tests/test_scripts_deletion.py b/netbox/extras/tests/test_scripts_deletion.py new file mode 100644 index 000000000..9aaf11e0c --- /dev/null +++ b/netbox/extras/tests/test_scripts_deletion.py @@ -0,0 +1,236 @@ +import uuid +from unittest import mock + +from django.contrib.contenttypes.models import ContentType +from django.db import router +from django.db.models import QuerySet +from django.test import TestCase, override_settings +from django.urls import reverse + +from core.choices import ManagedFileRootPathChoices +from core.models import DataSource, Job +from extras.models import Script, ScriptModule +from extras.validators import CustomValidator +from netbox.models.deletion import ConfirmCollector, CountOnly +from utilities.exceptions import AbortRequest +from utilities.testing import TestCase as ViewTestCase + + +class ScriptDeletionTestCase(TestCase): + """ + Regression tests for #22812: deleting a JobsMixin object (Script, ScriptModule, DataSource) + with many associated Jobs must not load every Job into memory at once. + """ + @classmethod + def setUpTestData(cls): + cls.script_ct = ContentType.objects.get_for_model(Script, for_concrete_model=False) + + def _create_module(self): + return ScriptModule.objects.create( + file_root=ManagedFileRootPathChoices.SCRIPTS, + file_path=f'test_{uuid.uuid4().hex[:8]}.py', + ) + + def _create_script(self, module=None): + module = module or self._create_module() + script = Script.objects.create(module=module, name=f'S{uuid.uuid4().hex[:8]}') + return module, script + + def _add_jobs(self, obj, count, object_type=None): + object_type = object_type or ContentType.objects.get_for_model(type(obj), for_concrete_model=False) + Job.objects.bulk_create([ + Job( + object_type=object_type, + object_id=obj.pk, + name='testjob', + status='completed', + job_id=uuid.uuid4(), + data={'output': 'x' * 50}, + ) + for _ in range(count) + ]) + + def test_delete_script_deletes_all_jobs(self): + _, script = self._create_script() + self._add_jobs(script, 2500) + self.assertEqual(script.jobs.count(), 2500) + + script.delete() + + 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_script_batches_jobs(self): + _, 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): + script.delete() + + # 5 jobs at a batch size of 2 => three batched deletes (2, 2, 1) + self.assertEqual(job_delete_calls, [2, 2, 1]) + self.assertEqual(Job.objects.filter(object_type=self.script_ct, object_id=script.pk).count(), 0) + + def test_delete_scriptmodule_cascades_to_scripts_and_jobs(self): + module, script = self._create_script() + self._add_jobs(script, 100) + + module.delete() + + self.assertFalse(ScriptModule.objects.filter(pk=module.pk).exists()) + 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) + ds_ct = ContentType.objects.get_for_model(DataSource, for_concrete_model=False) + self.assertEqual(Job.objects.filter(object_type=ds_ct, object_id=datasource.pk).count(), 100) + + datasource.delete() + + self.assertFalse(DataSource.objects.filter(pk=datasource.pk).exists()) + self.assertEqual(Job.objects.filter(object_type=ds_ct, object_id=datasource.pk).count(), 0) + + def test_soft_delete_preserves_jobs(self): + _, script = self._create_script() + self._add_jobs(script, 10) + + script.delete(soft_delete=True) + + script.refresh_from_db() + self.assertFalse(script.is_executable) + 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_rolls_back_jobs_on_parent_failure(self): + # A protection rule that no real script can satisfy (name must be empty) makes the + # cascade's pre_delete handler raise AbortRequest *after* JobsMixin.delete has already + # batch-deleted the jobs. JobsMixin.delete wraps the batch loop and super().delete() in a + # transaction, so the job deletions must roll back, leaving no orphaned partial state. + # This exercises the real deletion-abort path rather than mocking Django internals. + _, script = self._create_script() + self._add_jobs(script, 10) + + with self.assertRaises(AbortRequest): + script.delete() + + 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): + """ + #22812: the delete-confirmation page must not materialize every dependent Job. + """ + def _create_script_with_jobs(self, count): + module = ScriptModule.objects.create( + file_root=ManagedFileRootPathChoices.SCRIPTS, + file_path=f'test_{uuid.uuid4().hex[:8]}.py', + ) + script = Script.objects.create(module=module, name=f'S{uuid.uuid4().hex[:8]}') + ct = ContentType.objects.get_for_model(Script, for_concrete_model=False) + Job.objects.bulk_create([ + Job(object_type=ct, object_id=script.pk, name='j', status='completed', + job_id=uuid.uuid4(), data={'output': 'x' * 50}) + for _ in range(count) + ]) + return script + + def test_confirm_collector_counts_jobs_without_instantiating(self): + script = self._create_script_with_jobs(500) + + init_calls = [] + original_init = Job.__init__ + + def counting_init(self, *args, **kwargs): + init_calls.append(1) + original_init(self, *args, **kwargs) + + with mock.patch.object(Job, '__init__', counting_init): + collector = ConfirmCollector(using=router.db_for_write(Script)) + collector.collect([script]) + + # No Job rows were instantiated; the relation was counted instead. + self.assertEqual(len(init_calls), 0) + self.assertNotIn(Job, collector.data) + self.assertEqual(collector.generic_relation_counts.get(Job), 500) + # The non-job cascade (the Script itself) is still collected. + self.assertIn(Script, collector.data) + + def test_count_only_wrapper(self): + # CountOnly reports its count via len() but iterates empty, so it slots into the + # dependent-objects mapping as a non-expandable, non-materializing row. + wrapper = CountOnly(3000) + self.assertEqual(len(wrapper), 3000) + self.assertEqual(list(wrapper), []) + self.assertTrue(wrapper.count_only) + + +class ObjectDeleteViewCountsTestCase(ViewTestCase): + """ + #22812: the delete-confirmation view must report a JobsMixin object's jobs as a count + (via CountOnly) without materializing them, and _get_dependent_objects must keep returning + a single dict. + """ + def test_get_dependent_objects_returns_count_only_for_jobs(self): + from netbox.views.generic.object_views import ObjectDeleteView + + module = ScriptModule.objects.create( + file_root=ManagedFileRootPathChoices.SCRIPTS, + file_path=f'test_{uuid.uuid4().hex[:8]}.py', + ) + script = Script.objects.create(module=module, name=f'S{uuid.uuid4().hex[:8]}') + ct = ContentType.objects.get_for_model(Script, for_concrete_model=False) + Job.objects.bulk_create([ + Job(object_type=ct, object_id=script.pk, name='j', status='completed', job_id=uuid.uuid4()) + for _ in range(50) + ]) + + view = ObjectDeleteView() + view.queryset = ScriptModule.objects.all() + dependent_objects = view._get_dependent_objects(module) + + # Single dict returned (not a tuple); jobs represented as a CountOnly. + self.assertIsInstance(dependent_objects, dict) + self.assertIn(Job, dependent_objects) + self.assertIsInstance(dependent_objects[Job], CountOnly) + self.assertEqual(len(dependent_objects[Job]), 50) + + @override_settings(EXEMPT_VIEW_PERMISSIONS=['*']) + def test_confirm_page_renders_job_count(self): + module = ScriptModule.objects.create( + file_root=ManagedFileRootPathChoices.SCRIPTS, + file_path=f'test_{uuid.uuid4().hex[:8]}.py', + ) + script = Script.objects.create(module=module, name=f'S{uuid.uuid4().hex[:8]}') + ct = ContentType.objects.get_for_model(Script, for_concrete_model=False) + Job.objects.bulk_create([ + Job(object_type=ct, object_id=script.pk, name='j', status='completed', job_id=uuid.uuid4()) + for _ in range(50) + ]) + + # ScriptModule is a proxy over core.ManagedFile, so the delete view requires the + # concrete model's permission (core.delete_managedfile), not extras.delete_scriptmodule. + self.add_permissions('core.delete_managedfile') + url = reverse('extras:scriptmodule_delete', kwargs={'pk': module.pk}) + response = self.client.get(url) + self.assertEqual(response.status_code, 200) + + # Assert on the rendered context, not brittle HTML substrings: Job is present in + # dependent_objects as a CountOnly reporting the true count, so the confirmation page + # renders it as a summarized (non-expandable) row without materializing 50 Job rows. + dependent_objects = response.context['dependent_objects'] + self.assertIn(Job, dependent_objects) + self.assertIsInstance(dependent_objects[Job], CountOnly) + self.assertEqual(len(dependent_objects[Job]), 50) + self.assertTrue(dependent_objects[Job].count_only) diff --git a/netbox/netbox/constants.py b/netbox/netbox/constants.py index 037059583..fddce0ad9 100644 --- a/netbox/netbox/constants.py +++ b/netbox/netbox/constants.py @@ -74,3 +74,8 @@ 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 diff --git a/netbox/netbox/models/deletion.py b/netbox/netbox/models/deletion.py index e87321ae3..0d9aa2c19 100644 --- a/netbox/netbox/models/deletion.py +++ b/netbox/netbox/models/deletion.py @@ -8,6 +8,62 @@ from django.utils.translation import gettext as _ logger = logging.getLogger("netbox.models.deletion") +class CountOnly: + """ + A stand-in for a list of dependent instances that reports a count without holding any + instances. Used on the delete-confirmation page for high-cardinality relations (e.g. a + JobsMixin object's jobs) which we deliberately do not materialize (see #22812). It is a + lenient, empty iterable: `len()` returns the true row count, but iterating yields nothing, + so it slots into the same `{model: }` mapping as real instance lists and renders + as a non-expandable row. + """ + # Template flag: distinguishes a count-only entry (no instances to list) from a real list, + # so the confirmation page can render it without an expand/collapse affordance. + count_only = True + + def __init__(self, count): + self.count = count + + def __len__(self): + return self.count + + def __iter__(self): + return iter(()) + + +class ConfirmCollector(Collector): + """ + A display-only Collector used to enumerate the objects that would be deleted along with a + given object, for rendering the delete confirmation page. It behaves like Django's stock + Collector (preserving the full FK cascade graph and its ProtectedError/RestrictedError + behavior) except that it does not descend into the `jobs` GenericRelation. A JobsMixin + object can accumulate thousands of Jobs, each carrying large data/log_entries payloads; + materializing them all just to render a confirmation page can exhaust memory (see #22812). + Instead, the related Jobs are counted and recorded in `generic_relation_counts`. + """ + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + self.generic_relation_counts = {} + + def collect(self, objs, source=None, *args, **kwargs): + """ + Override collect() to count the `jobs` GenericRelation rather than descend into it. + + Django's Collector offers no per-relation skip hook, so we intercept the one call it + makes when cascading into a GenericRelation: collect(sub_objs, source=model, ...), where + `sub_objs` is a queryset of the related model. When that model is Job, we count the rows + instead of collecting (and thus instantiating) them, and forward every other call to the + stock implementation untouched. A directly-deleted Job (top-level call, source=None) + still collects normally. + """ + from core.models import Job + + if source is not None and getattr(objs, 'model', None) is Job: + self.generic_relation_counts[Job] = self.generic_relation_counts.get(Job, 0) + objs.count() + return None + return super().collect(objs, source=source, *args, **kwargs) + + class CustomCollector(Collector): """ Override Django's stock Collector to handle GenericRelations and ensure proper ordering of cascading deletions. diff --git a/netbox/netbox/models/features.py b/netbox/netbox/models/features.py index 1556a22a1..8755f7eaf 100644 --- a/netbox/netbox/models/features.py +++ b/netbox/netbox/models/features.py @@ -5,7 +5,7 @@ from functools import cached_property from django.contrib.contenttypes.fields import GenericForeignKey, GenericRelation from django.contrib.contenttypes.models import ContentType from django.core.validators import ValidationError -from django.db import models +from django.db import models, router, transaction from django.db.models import Q from django.utils import timezone from django.utils.translation import gettext_lazy as _ @@ -17,7 +17,7 @@ from extras.constants import CUSTOMFIELD_EMPTY_VALUES from extras.managers import NetBoxTaggableManager, NetBoxTaggableManagerField from extras.utils import is_taggable from netbox.config import get_config -from netbox.constants import CORE_APPS +from netbox.constants import CORE_APPS, JOB_DELETE_BATCH_SIZE from netbox.models.deletion import DeleteMixin from netbox.plugins import PluginConfig from netbox.registry import registry @@ -463,6 +463,24 @@ class JobsMixin(models.Model): class Meta: 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. + 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() + return super().delete(*args, **kwargs) + delete.alters_data = True + def get_latest_jobs(self): """ Return a list of the most recent jobs for this instance. diff --git a/netbox/netbox/views/generic/object_views.py b/netbox/netbox/views/generic/object_views.py index 3e9690030..e815a45cb 100644 --- a/netbox/netbox/views/generic/object_views.py +++ b/netbox/netbox/views/generic/object_views.py @@ -4,7 +4,6 @@ from collections import defaultdict from django.contrib import messages from django.db import router, transaction from django.db.models import ProtectedError, RestrictedError -from django.db.models.deletion import Collector from django.http import HttpResponse from django.shortcuts import redirect, render from django.urls import reverse @@ -13,6 +12,7 @@ from django.utils.safestring import mark_safe from django.utils.translation import gettext as _ from core.signals import clear_events +from netbox.models.deletion import ConfirmCollector, CountOnly from netbox.object_actions import BulkDelete, BulkEdit, CloneObject, DeleteObject, EditObject from utilities.error_handlers import handle_protectederror from utilities.exceptions import AbortRequest, PermissionsViolation @@ -385,14 +385,19 @@ class ObjectDeleteView(GetReturnURLMixin, BaseObjectView): def _get_dependent_objects(self, obj): """ - Returns a dictionary mapping of dependent objects (organized by model) which will be deleted as a result of - deleting the requested object. + Returns a dictionary mapping each dependent model to the objects (of that model) which will + be deleted as a result of deleting the requested object. + + Values are normally a list of instances. For high-cardinality relations that we do not + materialize to avoid excessive memory use (currently a JobsMixin object's jobs, see + #22812), the value is a `CountOnly` — a lenient empty iterable whose `len()` is the true + row count, so it renders as a non-expandable row alongside the itemized relations. Args: obj: The object to return dependent objects for """ using = router.db_for_write(obj._meta.model) - collector = Collector(using=using) + collector = ConfirmCollector(using=using) collector.collect([obj]) # Compile a mapping of models to instances @@ -406,7 +411,13 @@ class ObjectDeleteView(GetReturnURLMixin, BaseObjectView): continue dependent_objects[model].append(instances) - return dict(dependent_objects) + # Add count-only entries for relations the collector enumerated by count rather than by + # instance (e.g. jobs), so they render as non-expandable rows in the same mapping. + dependent_objects = dict(dependent_objects) + for model, count in collector.generic_relation_counts.items(): + dependent_objects[model] = CountOnly(count) + + return dependent_objects def _handle_protected_objects(self, obj, protected_objects, request, exc): """ diff --git a/netbox/templates/htmx/delete_form.html b/netbox/templates/htmx/delete_form.html index eb267e44c..8f24816a4 100644 --- a/netbox/templates/htmx/delete_form.html +++ b/netbox/templates/htmx/delete_form.html @@ -24,31 +24,38 @@

{% for model, instances in dependent_objects.items %} -
-

- -

-
-
-
- {% for instance in instances %} - {% with url=instance.get_absolute_url %} - {{ instance }} - {% endwith %} - {% endfor %} + {% with object_count=instances|length %} +
+

+ {# High-cardinality relations (e.g. jobs) are summarized by count and are not #} + {# expandable, since their instances are intentionally not loaded (see #22812). #} + {% if instances.count_only %} + + {% else %} + + {% endif %} +

+ {% if not instances.count_only %} +
+
+
+ {% for instance in instances %} + {% with url=instance.get_absolute_url %} + {{ instance }} + {% endwith %} + {% endfor %} +
+
-
+ {% endif %}
-
+ {% endwith %} {% endfor %}
{% endif %} From f2923f4ce4c2a741be823fd3d2f2f345b0ef1bb7 Mon Sep 17 00:00:00 2001 From: Jason Novinger Date: Wed, 5 Aug 2026 13:56:01 -0500 Subject: [PATCH 02/15] 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 From be69c55a990ba99fbcad774e85b7fdb562b8737e Mon Sep 17 00:00:00 2001 From: Jason Novinger Date: Wed, 5 Aug 2026 13:59:02 -0500 Subject: [PATCH 03/15] Fixes #22812: Don't show a spurious "0 jobs" row for jobless objects Django's Collector calls into the jobs GenericRelation branch unconditionally, so ConfirmCollector recorded a zero count for objects with no jobs. _get_dependent_objects then added a CountOnly(0), and the delete-confirmation page rendered "The following objects will be deleted as a result of this action." plus a "0 jobs" row for every jobless JobsMixin object. Only record a count when there are actually jobs. --- netbox/extras/tests/test_scripts_deletion.py | 25 ++++++++++++++++++++ netbox/netbox/models/deletion.py | 7 +++++- 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/netbox/extras/tests/test_scripts_deletion.py b/netbox/extras/tests/test_scripts_deletion.py index 5b928bd0d..1d5c92581 100644 --- a/netbox/extras/tests/test_scripts_deletion.py +++ b/netbox/extras/tests/test_scripts_deletion.py @@ -199,6 +199,16 @@ class ConfirmCollectorTestCase(TestCase): self.assertEqual(list(wrapper), []) self.assertTrue(wrapper.count_only) + def test_confirm_collector_omits_jobs_when_none(self): + # A jobless object must not record a zero count, or the confirmation page would show a + # spurious "0 jobs" row (#22812 regression). + datasource = DataSource.objects.create(name='DS', type='local', source_url='/tmp/test') + + collector = ConfirmCollector(using=router.db_for_write(DataSource)) + collector.collect([datasource]) + + self.assertNotIn(Job, collector.generic_relation_counts) + class ObjectDeleteViewCountsTestCase(ViewTestCase): """ @@ -258,3 +268,18 @@ class ObjectDeleteViewCountsTestCase(ViewTestCase): self.assertIsInstance(dependent_objects[Job], CountOnly) self.assertEqual(len(dependent_objects[Job]), 50) self.assertTrue(dependent_objects[Job].count_only) + + def test_get_dependent_objects_omits_jobs_when_none(self): + from netbox.views.generic.object_views import ObjectDeleteView + + # A module with no jobs must not produce a CountOnly(0) entry (#22812 regression). + module = ScriptModule.objects.create( + file_root=ManagedFileRootPathChoices.SCRIPTS, + file_path=f'test_{uuid.uuid4().hex[:8]}.py', + ) + + view = ObjectDeleteView() + view.queryset = ScriptModule.objects.all() + dependent_objects = view._get_dependent_objects(module) + + self.assertNotIn(Job, dependent_objects) diff --git a/netbox/netbox/models/deletion.py b/netbox/netbox/models/deletion.py index 0d9aa2c19..a903b7dff 100644 --- a/netbox/netbox/models/deletion.py +++ b/netbox/netbox/models/deletion.py @@ -59,7 +59,12 @@ class ConfirmCollector(Collector): from core.models import Job if source is not None and getattr(objs, 'model', None) is Job: - self.generic_relation_counts[Job] = self.generic_relation_counts.get(Job, 0) + objs.count() + # Django calls this branch for the jobs relation even when there are none; only record + # a count when there are actually jobs, so jobless objects don't get a spurious + # "0 jobs" row on the delete-confirmation page. + count = objs.count() + if count: + self.generic_relation_counts[Job] = self.generic_relation_counts.get(Job, 0) + count return None return super().collect(objs, source=source, *args, **kwargs) From 7d46c995f79637f0361fb230a4d8c746d6deb5df Mon Sep 17 00:00:00 2001 From: Jason Novinger Date: Wed, 5 Aug 2026 14:04:19 -0500 Subject: [PATCH 04/15] Fixes #22812: Defer large Job payload fields during batched deletion The batched job delete can't fast-delete (a global pre_delete receiver forces per-instance signals), so each batch still instantiates its Job rows. Load only the PK via only('pk') so those instances don't pull the large data/log_entries payloads, cutting the resident set per batch. Also drop a dead `no-toggle` CSS class from the delete-confirmation template (it is defined nowhere and, under Tabler, has no effect) and use JobStatusChoices.STATUS_COMPLETED in the tests instead of a string literal. --- netbox/extras/tests/test_scripts_deletion.py | 16 +++++++++++----- netbox/netbox/models/features.py | 5 ++++- netbox/templates/htmx/delete_form.html | 2 +- 3 files changed, 16 insertions(+), 7 deletions(-) diff --git a/netbox/extras/tests/test_scripts_deletion.py b/netbox/extras/tests/test_scripts_deletion.py index 1d5c92581..f6d9bad48 100644 --- a/netbox/extras/tests/test_scripts_deletion.py +++ b/netbox/extras/tests/test_scripts_deletion.py @@ -7,7 +7,7 @@ from django.db.models import QuerySet from django.test import TestCase, override_settings from django.urls import reverse -from core.choices import ManagedFileRootPathChoices +from core.choices import JobStatusChoices, ManagedFileRootPathChoices from core.models import DataSource, Job from extras.models import Script, ScriptModule from extras.validators import CustomValidator @@ -43,7 +43,7 @@ class ScriptDeletionTestCase(TestCase): object_type=object_type, object_id=obj.pk, name='testjob', - status='completed', + status=JobStatusChoices.STATUS_COMPLETED, job_id=uuid.uuid4(), data={'output': 'x' * 50}, ) @@ -164,7 +164,7 @@ class ConfirmCollectorTestCase(TestCase): script = Script.objects.create(module=module, name=f'S{uuid.uuid4().hex[:8]}') ct = ContentType.objects.get_for_model(Script, for_concrete_model=False) Job.objects.bulk_create([ - Job(object_type=ct, object_id=script.pk, name='j', status='completed', + Job(object_type=ct, object_id=script.pk, name='j', status=JobStatusChoices.STATUS_COMPLETED, job_id=uuid.uuid4(), data={'output': 'x' * 50}) for _ in range(count) ]) @@ -226,7 +226,10 @@ class ObjectDeleteViewCountsTestCase(ViewTestCase): script = Script.objects.create(module=module, name=f'S{uuid.uuid4().hex[:8]}') ct = ContentType.objects.get_for_model(Script, for_concrete_model=False) Job.objects.bulk_create([ - Job(object_type=ct, object_id=script.pk, name='j', status='completed', job_id=uuid.uuid4()) + Job( + object_type=ct, object_id=script.pk, name='j', + status=JobStatusChoices.STATUS_COMPLETED, job_id=uuid.uuid4(), + ) for _ in range(50) ]) @@ -249,7 +252,10 @@ class ObjectDeleteViewCountsTestCase(ViewTestCase): script = Script.objects.create(module=module, name=f'S{uuid.uuid4().hex[:8]}') ct = ContentType.objects.get_for_model(Script, for_concrete_model=False) Job.objects.bulk_create([ - Job(object_type=ct, object_id=script.pk, name='j', status='completed', job_id=uuid.uuid4()) + Job( + object_type=ct, object_id=script.pk, name='j', + status=JobStatusChoices.STATUS_COMPLETED, job_id=uuid.uuid4(), + ) for _ in range(50) ]) diff --git a/netbox/netbox/models/features.py b/netbox/netbox/models/features.py index f608da50c..4f1d5b684 100644 --- a/netbox/netbox/models/features.py +++ b/netbox/netbox/models/features.py @@ -464,7 +464,10 @@ def batch_delete_jobs(job_queryset): # 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() + # 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() class JobsMixin(models.Model): diff --git a/netbox/templates/htmx/delete_form.html b/netbox/templates/htmx/delete_form.html index 8f24816a4..c07e83d0a 100644 --- a/netbox/templates/htmx/delete_form.html +++ b/netbox/templates/htmx/delete_form.html @@ -30,7 +30,7 @@ {# High-cardinality relations (e.g. jobs) are summarized by count and are not #} {# expandable, since their instances are intentionally not loaded (see #22812). #} {% if instances.count_only %} -