On commit cleanup (#4388)

jmsmkn · web-flow · commit 322c2b257c58 · 2025-10-30T15:34:35.000+01:00
Ensures that: - only celery tasks are used in `on_commit` - the function passed to `on_commit` is a real method and not a lambda - `apply_async` is never called directly See DIAGNijmegen/rse-grand-challenge-admin#663
diff --git a/app/grandchallenge/algorithms/tasks.py b/app/grandchallenge/algorithms/tasks.py
@@ -67,7 +67,7 @@ def execute_algorithm_job_for_inputs(*, job_pk):
     job.status = job.PENDING
     job.save()
 
-    on_commit(job.execute)
+    job.execute()
 
 
 def create_algorithm_jobs(
@@ -175,7 +175,7 @@ def create_algorithm_jobs(
             job.utilization.challenge = job_utilization_challenge
             job.utilization.save()
 
-            on_commit(job.execute)
+            job.execute()
 
             jobs.append(job)
 
diff --git a/app/grandchallenge/algorithms/views.py b/app/grandchallenge/algorithms/views.py
@@ -15,6 +15,7 @@
 from django.core.exceptions import PermissionDenied, ValidationError
 from django.db.models import Q, Window
 from django.db.models.functions import Rank
+from django.db.transaction import on_commit
 from django.forms.utils import ErrorList
 from django.http import FileResponse, HttpResponse, HttpResponseRedirect
 from django.shortcuts import get_object_or_404
@@ -501,14 +502,16 @@ def form_valid(self, form):
             algorithm_image.import_status = ImportStatusChoices.QUEUED
             algorithm_image.save()
 
-            upload_to_registry_and_sagemaker.signature(
-                kwargs={
-                    "app_label": algorithm_image._meta.app_label,
-                    "model_name": algorithm_image._meta.model_name,
-                    "pk": algorithm_image.pk,
-                    "mark_as_desired": True,
-                }
-            ).apply_async()
+            on_commit(
+                upload_to_registry_and_sagemaker.signature(
+                    kwargs={
+                        "app_label": algorithm_image._meta.app_label,
+                        "model_name": algorithm_image._meta.model_name,
+                        "pk": algorithm_image.pk,
+                        "mark_as_desired": True,
+                    }
+                ).apply_async
+            )
 
         return response
 
diff --git a/app/grandchallenge/challenges/models.py b/app/grandchallenge/challenges/models.py
@@ -532,13 +532,13 @@ def save(self, *args, **kwargs):
 
         if adding or self.hidden != self._hidden_orig:
             on_commit(
-                lambda: assign_evaluation_permissions.apply_async(
+                assign_evaluation_permissions.signature(
                     kwargs={
                         "phase_pks": list(
                             self.phase_set.values_list("id", flat=True)
                         )
                     }
-                )
+                ).apply_async
             )
 
         if self.has_changed("compute_cost_euro_millicents"):
diff --git a/app/grandchallenge/codebuild/tasks.py b/app/grandchallenge/codebuild/tasks.py
@@ -53,9 +53,9 @@ def handle_completed_build_event(*, build_arn, build_status):
 
     if build.status == build.BuildStatusChoices.SUCCEEDED:
         on_commit(
-            lambda: add_image_to_algorithm.apply_async(
+            add_image_to_algorithm.signature(
                 kwargs={"build_pk": str(build.pk)}
-            )
+            ).apply_async
         )
 
 
diff --git a/app/grandchallenge/components/admin.py b/app/grandchallenge/components/admin.py
@@ -127,7 +127,7 @@ def requeue_jobs(modeladmin, request, queryset):
 
         jobs.append(job)
 
-        on_commit(job.execute)
+        job.execute()
 
     queryset.model.objects.bulk_update(
         jobs,
@@ -170,4 +170,6 @@ def cancel_jobs(modeladmin, request, queryset):
 )
 def deprovision_jobs(modeladmin, request, queryset):
     for job in queryset:
-        deprovision_job.signature(**job.signature_kwargs).apply_async()
+        on_commit(
+            deprovision_job.signature(**job.signature_kwargs).apply_async
+        )
diff --git a/app/grandchallenge/components/management/commands/interface_to_object_store.py b/app/grandchallenge/components/management/commands/interface_to_object_store.py
@@ -1,4 +1,5 @@
 from django.core.management import BaseCommand
+from django.db.transaction import on_commit
 
 from grandchallenge.components.models import (
     ComponentInterface,
@@ -31,6 +32,10 @@ def handle(self, *args, **options):
             interface.save()
 
             for pk in pks:
-                civ_value_to_file.apply_async(kwargs={"civ_pk": pk})
+                on_commit(
+                    civ_value_to_file.signature(
+                        kwargs={"civ_pk": pk}
+                    ).apply_async
+                )
 
         self.stdout.write("Conversion tasks scheduled")
diff --git a/app/grandchallenge/components/models.py b/app/grandchallenge/components/models.py
@@ -1791,9 +1791,9 @@ def update_status(  # noqa:C901
         self.save()
 
         if self.status == self.SUCCESS:
-            on_commit(self.execute_task_on_success)
+            self.execute_task_on_success()
         elif self.status in [self.FAILURE, self.CANCELLED]:
-            on_commit(self.execute_task_on_failure)
+            self.execute_task_on_failure()
 
     @property
     def executor_kwargs(self):
@@ -1842,17 +1842,21 @@ def signature_kwargs(self):
         }
 
     def execute(self):
-        return provision_job.signature(**self.signature_kwargs).apply_async()
+        on_commit(provision_job.signature(**self.signature_kwargs).apply_async)
 
     def execute_task_on_success(self):
-        deprovision_job.signature(**self.signature_kwargs).apply_async()
+        on_commit(
+            deprovision_job.signature(**self.signature_kwargs).apply_async
+        )
         if self.task_on_success:
-            signature(self.task_on_success).apply_async()
+            on_commit(signature(self.task_on_success).apply_async)
 
     def execute_task_on_failure(self):
-        deprovision_job.signature(**self.signature_kwargs).apply_async()
+        on_commit(
+            deprovision_job.signature(**self.signature_kwargs).apply_async
+        )
         if self.task_on_failure:
-            signature(self.task_on_failure).apply_async()
+            on_commit(signature(self.task_on_failure).apply_async)
 
     @property
     def animate(self):
diff --git a/app/grandchallenge/components/tasks.py b/app/grandchallenge/components/tasks.py
@@ -813,7 +813,9 @@ def execute_job(
     if job.status == job.PROVISIONED:
         job.update_status(status=job.EXECUTING)
     else:
-        deprovision_job.signature(**job.signature_kwargs).apply_async()
+        on_commit(
+            deprovision_job.signature(**job.signature_kwargs).apply_async
+        )
         raise PriorStepFailed("Job is not set to be executed")
 
     if not job.container.can_execute:
diff --git a/app/grandchallenge/evaluation/models.py b/app/grandchallenge/evaluation/models.py
@@ -770,9 +770,9 @@ def save(self, *args, skip_calculate_ranks=False, **kwargs):
 
         if not skip_calculate_ranks:
             on_commit(
-                lambda: calculate_ranks.apply_async(
+                calculate_ranks.signature(
                     kwargs={"phase_pk": self.pk}
-                )
+                ).apply_async
             )
 
     def clean(self):
@@ -1955,9 +1955,9 @@ def save(self, *args, **kwargs):
         self.assign_permissions()
 
         on_commit(
-            lambda: calculate_ranks.apply_async(
+            calculate_ranks.signature(
                 kwargs={"phase_pk": self.submission.phase.pk}
-            )
+            ).apply_async
         )
 
     @property
diff --git a/app/grandchallenge/evaluation/tasks.py b/app/grandchallenge/evaluation/tasks.py
@@ -149,12 +149,12 @@ def prepare_and_execute_evaluation(*, evaluation_pk):
         evaluation.status = Evaluation.PENDING
         evaluation.save()
         on_commit(
-            lambda: create_algorithm_jobs_for_evaluation.apply_async(
+            create_algorithm_jobs_for_evaluation.signature(
                 kwargs={
                     "evaluation_pk": evaluation_pk,
                     "first_run": True,
                 }
-            )
+            ).apply_async
         )
     elif evaluation.submission.predictions_file:
         mimetype = get_file_mimetype(evaluation.submission.predictions_file)
@@ -191,7 +191,8 @@ def prepare_and_execute_evaluation(*, evaluation_pk):
         evaluation.inputs.add(civ)
         evaluation.status = Evaluation.PENDING
         evaluation.save()
-        on_commit(evaluation.execute)
+
+        evaluation.execute()
     else:
         evaluation.update_status(
             status=Evaluation.FAILURE,
@@ -463,7 +464,7 @@ def set_evaluation_inputs(*, evaluation_pk):
         evaluation.status = evaluation.PENDING
         evaluation.save()
 
-        on_commit(evaluation.execute)
+        evaluation.execute()
 
 
 def filter_by_creators_most_recent(*, evaluations):
diff --git a/app/grandchallenge/evaluation/views/api.py b/app/grandchallenge/evaluation/views/api.py
@@ -1,4 +1,5 @@
 from django.conf import settings
+from django.db.transaction import on_commit
 from django.utils.timezone import now
 from django_filters.rest_framework import DjangoFilterBackend
 from drf_spectacular.types import OpenApiTypes
@@ -145,7 +146,7 @@ def update_external_evaluation(self, request, *args, **kwargs):
         if (
             evaluation.claimed_at - now()
         ).seconds > settings.EXTERNAL_EVALUATION_TIMEOUT_IN_SECONDS:
-            cancel_external_evaluations_past_timeout.apply_async()
+            on_commit(cancel_external_evaluations_past_timeout.apply_async)
             return Response(
                 {"status": "The evaluation was not updated in time."},
                 status=400,
diff --git a/app/grandchallenge/github/tasks.py b/app/grandchallenge/github/tasks.py
@@ -111,7 +111,7 @@ def save_zipfile(ghwm, tmpdirname):
 
 def build_repo(ghwm_pk):
     on_commit(
-        lambda: create_codebuild_build.apply_async(kwargs={"pk": ghwm_pk})
+        create_codebuild_build.signature(kwargs={"pk": ghwm_pk}).apply_async
     )
 
 
diff --git a/app/grandchallenge/profiles/admin.py b/app/grandchallenge/profiles/admin.py
@@ -2,6 +2,7 @@
 from django.contrib import admin
 from django.contrib.auth import get_user_model
 from django.contrib.auth.admin import UserAdmin
+from django.db.transaction import on_commit
 
 from grandchallenge.core.admin import (
     GroupObjectPermissionAdmin,
@@ -28,7 +29,9 @@ class UserProfileInline(admin.StackedInline):
 )
 def deactivate_users(modeladmin, request, queryset):
     for user in queryset.filter(is_active=True):
-        deactivate_user.signature(kwargs={"user_pk": user.pk}).apply_async()
+        on_commit(
+            deactivate_user.signature(kwargs={"user_pk": user.pk}).apply_async
+        )
 
 
 class UserProfileAdmin(UserAdmin):
diff --git a/app/grandchallenge/profiles/views.py b/app/grandchallenge/profiles/views.py
@@ -6,6 +6,7 @@
 from django.core.exceptions import ObjectDoesNotExist
 from django.core.signing import BadSignature, Signer
 from django.db.models import Q
+from django.db.transaction import on_commit
 from django.http import Http404
 from django.shortcuts import get_object_or_404, redirect
 from django.views.generic import DetailView, UpdateView
@@ -223,14 +224,16 @@ def test_func(self):
             return False
 
         if self.request.user.is_authenticated and user != self.request.user:
-            update_verification_user_set.signature(
-                kwargs={
-                    "usernames": [
-                        self.request.user.username,
-                        user.username,
-                    ]
-                }
-            ).apply_async()
+            on_commit(
+                update_verification_user_set.signature(
+                    kwargs={
+                        "usernames": [
+                            self.request.user.username,
+                            user.username,
+                        ]
+                    }
+                ).apply_async
+            )
 
         return True
 
diff --git a/app/grandchallenge/reader_studies/views.py b/app/grandchallenge/reader_studies/views.py
@@ -11,8 +11,8 @@
 )
 from django.contrib.messages.views import SuccessMessageMixin
 from django.core.exceptions import PermissionDenied, ValidationError
-from django.db import transaction
 from django.db.models import Count, Q
+from django.db.transaction import on_commit
 from django.forms import Form
 from django.forms.utils import ErrorList
 from django.http import (
@@ -653,13 +653,13 @@ def form_valid(self, form):  # noqa: C901
         rs.save()
         self.reader_study = rs
         if form.cleaned_data["copy_display_sets"]:
-            transaction.on_commit(
-                lambda: copy_reader_study_display_sets.apply_async(
+            on_commit(
+                copy_reader_study_display_sets.signature(
                     kwargs={
                         "orig_pk": str(reader_study.pk),
                         "new_pk": str(rs.pk),
                     }
-                )
+                ).apply_async
             )
             messages.add_message(
                 self.request,
diff --git a/app/grandchallenge/verifications/forms.py b/app/grandchallenge/verifications/forms.py
@@ -4,6 +4,7 @@
 from django.contrib.auth import get_user_model
 from django.core.exceptions import ObjectDoesNotExist, ValidationError
 from django.core.mail import mail_managers
+from django.db.transaction import on_commit
 from django.forms import CheckboxInput
 from django.utils.html import format_html
 
@@ -100,9 +101,11 @@ def clean_token(self):
         token = self.cleaned_data["token"]
 
         if not hasattr(self.user, "verification"):
-            deactivate_user.signature(
-                kwargs={"user_pk": self.user.pk}
-            ).apply_async()
+            on_commit(
+                deactivate_user.signature(
+                    kwargs={"user_pk": self.user.pk}
+                ).apply_async
+            )
 
             mail_managers(
                 subject="User automatically deactivated",
diff --git a/app/tests/algorithms_tests/test_tasks.py b/app/tests/algorithms_tests/test_tasks.py
@@ -559,7 +559,7 @@ def test_execute_algorithm_job_sets_on_failed_jobs(
 
     # Sanity: task should run till execution
     assert len(callbacks) == 1
-    assert "ComponentJob.execute" in str(callbacks[0])
+    assert "grandchallenge.components.tasks.provision_job" in str(callbacks[0])
 
     job.refresh_from_db()
     assert job.status == Job.PENDING
diff --git a/app/tests/algorithms_tests/test_views.py b/app/tests/algorithms_tests/test_views.py
diff --git a/app/tests/core_tests/test_celery.py b/app/tests/core_tests/test_celery.py
diff --git a/app/tests/evaluation_tests/test_api.py b/app/tests/evaluation_tests/test_api.py
diff --git a/app/tests/profiles_tests/test_views.py b/app/tests/profiles_tests/test_views.py
diff --git a/app/tests/verification_tests/test_forms.py b/app/tests/verification_tests/test_forms.py
diff --git a/scripts/codebuild_fixtures.py b/scripts/codebuild_fixtures.py
diff --git a/scripts/pentest_fixtures.py b/scripts/pentest_fixtures.py

Original file line number	Diff line number	Diff line change
`@@ -532,13 +532,13 @@ def save(self, args, *kwargs):`
`532`	`532`
`533`	`533`	`if adding or self.hidden != self._hidden_orig:`
`534`	`534`	`on_commit(`
`535`		`- lambda: assign_evaluation_permissions.apply_async(`
	`535`	`+ assign_evaluation_permissions.signature(`
`536`	`536`	`kwargs={`
`537`	`537`	`"phase_pks": list(`
`538`	`538`	`self.phase_set.values_list("id", flat=True)`
`539`	`539`	`)`
`540`	`540`	`}`
`541`		`- )`
	`541`	`+ ).apply_async`
`542`	`542`	`)`
`543`	`543`
`544`	`544`	`if self.has_changed("compute_cost_euro_millicents"):`
Original file line number	Diff line number	Diff line change
`@@ -53,9 +53,9 @@ def handle_completed_build_event(*, build_arn, build_status):`
`53`	`53`
`54`	`54`	`if build.status == build.BuildStatusChoices.SUCCEEDED:`
`55`	`55`	`on_commit(`
`56`		`- lambda: add_image_to_algorithm.apply_async(`
	`56`	`+ add_image_to_algorithm.signature(`
`57`	`57`	`kwargs={"build_pk": str(build.pk)}`
`58`		`- )`
	`58`	`+ ).apply_async`
`59`	`59`	`)`
`60`	`60`
`61`	`61`