Pārlūkot izejas kodu

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.
Jason Novinger 2 nedēļas atpakaļ
vecāks
revīzija
0701a42a94

+ 236 - 0
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)

+ 5 - 0
netbox/netbox/constants.py

@@ -74,3 +74,8 @@ CENSOR_TOKEN_CHANGED = '***CHANGED***'
 
 
 # Placeholder text for empty tables
 # Placeholder text for empty tables
 EMPTY_TABLE_TEXT = 'No results found'
 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

+ 56 - 0
netbox/netbox/models/deletion.py

@@ -8,6 +8,62 @@ from django.utils.translation import gettext as _
 logger = logging.getLogger("netbox.models.deletion")
 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: <iterable>}` 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):
 class CustomCollector(Collector):
     """
     """
     Override Django's stock Collector to handle GenericRelations and ensure proper ordering of cascading deletions.
     Override Django's stock Collector to handle GenericRelations and ensure proper ordering of cascading deletions.

+ 20 - 2
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.fields import GenericForeignKey, GenericRelation
 from django.contrib.contenttypes.models import ContentType
 from django.contrib.contenttypes.models import ContentType
 from django.core.validators import ValidationError
 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.db.models import Q
 from django.utils import timezone
 from django.utils import timezone
 from django.utils.translation import gettext_lazy as _
 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.managers import NetBoxTaggableManager, NetBoxTaggableManagerField
 from extras.utils import is_taggable
 from extras.utils import is_taggable
 from netbox.config import get_config
 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.models.deletion import DeleteMixin
 from netbox.plugins import PluginConfig
 from netbox.plugins import PluginConfig
 from netbox.registry import registry
 from netbox.registry import registry
@@ -463,6 +463,24 @@ class JobsMixin(models.Model):
     class Meta:
     class Meta:
         abstract = True
         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):
     def get_latest_jobs(self):
         """
         """
         Return a list of the most recent jobs for this instance.
         Return a list of the most recent jobs for this instance.

+ 16 - 5
netbox/netbox/views/generic/object_views.py

@@ -4,7 +4,6 @@ from collections import defaultdict
 from django.contrib import messages
 from django.contrib import messages
 from django.db import router, transaction
 from django.db import router, transaction
 from django.db.models import ProtectedError, RestrictedError
 from django.db.models import ProtectedError, RestrictedError
-from django.db.models.deletion import Collector
 from django.http import HttpResponse
 from django.http import HttpResponse
 from django.shortcuts import redirect, render
 from django.shortcuts import redirect, render
 from django.urls import reverse
 from django.urls import reverse
@@ -13,6 +12,7 @@ from django.utils.safestring import mark_safe
 from django.utils.translation import gettext as _
 from django.utils.translation import gettext as _
 
 
 from core.signals import clear_events
 from core.signals import clear_events
+from netbox.models.deletion import ConfirmCollector, CountOnly
 from netbox.object_actions import BulkDelete, BulkEdit, CloneObject, DeleteObject, EditObject
 from netbox.object_actions import BulkDelete, BulkEdit, CloneObject, DeleteObject, EditObject
 from utilities.error_handlers import handle_protectederror
 from utilities.error_handlers import handle_protectederror
 from utilities.exceptions import AbortRequest, PermissionsViolation
 from utilities.exceptions import AbortRequest, PermissionsViolation
@@ -385,14 +385,19 @@ class ObjectDeleteView(GetReturnURLMixin, BaseObjectView):
 
 
     def _get_dependent_objects(self, obj):
     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:
         Args:
             obj: The object to return dependent objects for
             obj: The object to return dependent objects for
         """
         """
         using = router.db_for_write(obj._meta.model)
         using = router.db_for_write(obj._meta.model)
-        collector = Collector(using=using)
+        collector = ConfirmCollector(using=using)
         collector.collect([obj])
         collector.collect([obj])
 
 
         # Compile a mapping of models to instances
         # Compile a mapping of models to instances
@@ -406,7 +411,13 @@ class ObjectDeleteView(GetReturnURLMixin, BaseObjectView):
                 continue
                 continue
             dependent_objects[model].append(instances)
             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):
     def _handle_protected_objects(self, obj, protected_objects, request, exc):
         """
         """

+ 30 - 23
netbox/templates/htmx/delete_form.html

@@ -24,31 +24,38 @@
       </p>
       </p>
       <div class="accordion mb-3" id="deleteAccordion">
       <div class="accordion mb-3" id="deleteAccordion">
         {% for model, instances in dependent_objects.items %}
         {% for model, instances in dependent_objects.items %}
-          <div class="accordion-item">
-            <h2 class="accordion-header h4" id="deleteheading{{ forloop.counter }}">
-              <button class="accordion-button collapsed" type="button" data-bs-toggle="collapse" data-bs-target="#collapse{{ forloop.counter }}" aria-expanded="false" aria-controls="collapse{{ forloop.counter }}">
-                {% with object_count=instances|length %}
-                  {{ object_count }}
-                  {% if object_count == 1 %}
-                    {{ model|meta:"verbose_name" }}
-                  {% else %}
-                    {{ model|meta:"verbose_name_plural" }}
-                  {% endif %}
-                {% endwith %}
-              </button>
-            </h2>
-            <div id="collapse{{ forloop.counter }}" class="accordion-collapse collapse" aria-labelledby="deleteheading{{ forloop.counter }}" data-bs-parent="#deleteAccordion">
-              <div class="accordion-body p-0">
-                <div class="list-group list-group-flush">
-                  {% for instance in instances %}
-                    {% with url=instance.get_absolute_url %}
-                      <a {% if url %}href="{{ url }}" {% endif %}class="list-group-item list-group-item-action">{{ instance }}</a>
-                    {% endwith %}
-                  {% endfor %}
+          {% with object_count=instances|length %}
+            <div class="accordion-item">
+              <h2 class="accordion-header h4" id="deleteheading{{ forloop.counter }}">
+                {# 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 %}
+                  <div class="accordion-button collapsed no-toggle" style="cursor: default;">
+                    {{ object_count }}
+                    {% if object_count == 1 %}{{ model|meta:"verbose_name" }}{% else %}{{ model|meta:"verbose_name_plural" }}{% endif %}
+                  </div>
+                {% else %}
+                  <button class="accordion-button collapsed" type="button" data-bs-toggle="collapse" data-bs-target="#collapse{{ forloop.counter }}" aria-expanded="false" aria-controls="collapse{{ forloop.counter }}">
+                    {{ object_count }}
+                    {% if object_count == 1 %}{{ model|meta:"verbose_name" }}{% else %}{{ model|meta:"verbose_name_plural" }}{% endif %}
+                  </button>
+                {% endif %}
+              </h2>
+              {% if not instances.count_only %}
+                <div id="collapse{{ forloop.counter }}" class="accordion-collapse collapse" aria-labelledby="deleteheading{{ forloop.counter }}" data-bs-parent="#deleteAccordion">
+                  <div class="accordion-body p-0">
+                    <div class="list-group list-group-flush">
+                      {% for instance in instances %}
+                        {% with url=instance.get_absolute_url %}
+                          <a {% if url %}href="{{ url }}" {% endif %}class="list-group-item list-group-item-action">{{ instance }}</a>
+                        {% endwith %}
+                      {% endfor %}
+                    </div>
+                  </div>
                 </div>
                 </div>
-              </div>
+              {% endif %}
             </div>
             </div>
-          </div>
+          {% endwith %}
         {% endfor %}
         {% endfor %}
       </div>
       </div>
     {% endif %}
     {% endif %}