From 67318a5f012d0bc8a2a8ccac41973fa15a55cccd Mon Sep 17 00:00:00 2001 From: Geert Hesselink <54070862+Ghesselink@users.noreply.github.com> Date: Mon, 29 Jun 2026 12:59:15 +0000 Subject: [PATCH 1/2] IVS-673: advance progress only after a task completes Prevents reporting 100% while instance completion still runs; atomic Least(F(progress)+inc, 100) update avoids the parallel-stage race. --- .../apps/ifc_validation/tasks/task_runner.py | 21 ++-- .../tests/tests_progress_update.py | 105 ++++++++++++++++++ 2 files changed, 119 insertions(+), 7 deletions(-) create mode 100644 backend/apps/ifc_validation/tests/tests_progress_update.py diff --git a/backend/apps/ifc_validation/tasks/task_runner.py b/backend/apps/ifc_validation/tasks/task_runner.py index 46f8f6a7..b461f55a 100644 --- a/backend/apps/ifc_validation/tasks/task_runner.py +++ b/backend/apps/ifc_validation/tasks/task_runner.py @@ -5,6 +5,9 @@ from celery import shared_task, chain, chord, group from celery.exceptions import SoftTimeLimitExceeded +from django.db.models import F, Value +from django.db.models.functions import Least + from core.redis_lock import acquire_user_lock, LockError from core.utils import log_execution @@ -192,11 +195,6 @@ def validation_subtask_runner(self, *args, **kwargs): else: # for testing, we're not instantiating a model invalid_blockers = [] - # update progress - increment = config.increment - request.progress = min(request.progress + increment, 100) - request.save() - # run or skip if not invalid_blockers: task.mark_as_initiated() @@ -224,12 +222,21 @@ def validation_subtask_runner(self, *args, **kwargs): logger.exception(f"Processing failed in task {task_type}: {err}") return - # Handle skipped tasks + # Handle skipped tasks else: reason = f"Skipped due to fail in blocking tasks: {', '.join(invalid_blockers)}" logger.debug(reason) task.mark_as_skipped(reason) - + + # Update progress only after the task's work has actually finished, so the + # request never reports 100% while a long-running task (e.g. instance + # completion) is still executing (IVS-673). Failed tasks return early above + # and intentionally don't advance progress. Use an atomic DB-side update + # because parallel-stage tasks increment progress concurrently. + ValidationRequest.objects.filter(pk=id).update( + progress=Least(F("progress") + config.increment, Value(100)) + ) + validation_subtask_runner.__doc__ = f"Validation task for {task_type} generated by the task_factory func." return validation_subtask_runner diff --git a/backend/apps/ifc_validation/tests/tests_progress_update.py b/backend/apps/ifc_validation/tests/tests_progress_update.py new file mode 100644 index 00000000..c75a24de --- /dev/null +++ b/backend/apps/ifc_validation/tests/tests_progress_update.py @@ -0,0 +1,105 @@ +from contextlib import contextmanager +from unittest import mock + +from django.test import TransactionTestCase +from django.contrib.auth.models import User + +from apps.ifc_validation_models.models import ( + ValidationRequest, + ValidationTask, + set_user_context, +) + +from ..tasks.configs import task_registry +from ..tasks import schema_validation_subtask +import apps.ifc_validation.tasks.task_runner as task_runner + + +@contextmanager +def _noop_lock(*args, **kwargs): + # Replaces the redis-backed user task lock so these tests stay hermetic. + yield None + + +@mock.patch.object(task_runner, "acquire_user_lock", _noop_lock) +class ProgressUpdateTaskTestCase(TransactionTestCase): + """ + Regression tests for IVS-673: the request must not report 100% (or advance + its progress at all) while a subtask is still executing. Progress should only + advance *after* a task's execution + processing layers have finished. + """ + + TASK_TYPE = ValidationTask.Type.SCHEMA # increment = 10 + + def setUp(self): + user, _ = User.objects.get_or_create( + id=1, defaults={"username": "SYSTEM", "is_active": True} + ) + set_user_context(user) + self.config = task_registry[self.TASK_TYPE] + self.increment = self.config.increment + + def _make_request(self, progress=0): + request = ValidationRequest.objects.create( + file_name="valid_file.ifc", file="valid_file.ifc", size=280 + ) + request.mark_as_initiated() # progress -> 0 + if progress: + ValidationRequest.objects.filter(pk=request.id).update(progress=progress) + return request + + def _run(self, request): + schema_validation_subtask( + prev_result={"is_valid": True, "reason": "test"}, + id=request.id, + file_name=request.file_name, + ) + + def test_progress_advances_only_after_task_work_completes(self): + request = self._make_request() + seen = {} + + def fake_check(context): + seen["during_check"] = ValidationRequest.objects.get(pk=request.id).progress + return context + + def fake_process(context): + seen["during_process"] = ValidationRequest.objects.get(pk=request.id).progress + return "ok" + + with mock.patch.object(self.config, "check_program", side_effect=fake_check), \ + mock.patch.object(self.config, "process_results", side_effect=fake_process): + self._run(request) + + # While the task body executed, progress had NOT advanced yet (the IVS-673 bug + # was that it had already been bumped before the work ran). + self.assertEqual(seen["during_check"], 0) + self.assertEqual(seen["during_process"], 0) + + # Only once the task finished does progress advance by the configured increment. + request.refresh_from_db() + self.assertEqual(request.progress, self.increment) + + def test_progress_is_clamped_to_100(self): + request = self._make_request(progress=95) # 95 + 10 would overflow to 105 + + with mock.patch.object(self.config, "check_program", return_value=None), \ + mock.patch.object(self.config, "process_results", return_value="ok"): + self._run(request) + + request.refresh_from_db() + self.assertEqual(request.progress, 100) + + def test_failed_task_does_not_advance_progress(self): + request = self._make_request() + + with mock.patch.object(self.config, "check_program", side_effect=RuntimeError("boom")): + self._run(request) + + request.refresh_from_db() + self.assertEqual(request.progress, 0) + + task = ValidationTask.objects.filter( + request_id=request.id, type=self.TASK_TYPE + ).first() + self.assertEqual(task.status, ValidationTask.Status.FAILED) From 87ad136f24c1c2456ffa43b3c3849cbdbeef4d45 Mon Sep 17 00:00:00 2001 From: bSI Validation Service CI/CD Date: Mon, 29 Jun 2026 13:19:01 +0000 Subject: [PATCH 2/2] adjust descr within code --- backend/apps/ifc_validation/tasks/task_runner.py | 8 +++----- .../ifc_validation/tests/tests_progress_update.py | 13 +++++-------- 2 files changed, 8 insertions(+), 13 deletions(-) diff --git a/backend/apps/ifc_validation/tasks/task_runner.py b/backend/apps/ifc_validation/tasks/task_runner.py index b461f55a..84fa8cd9 100644 --- a/backend/apps/ifc_validation/tasks/task_runner.py +++ b/backend/apps/ifc_validation/tasks/task_runner.py @@ -228,11 +228,9 @@ def validation_subtask_runner(self, *args, **kwargs): logger.debug(reason) task.mark_as_skipped(reason) - # Update progress only after the task's work has actually finished, so the - # request never reports 100% while a long-running task (e.g. instance - # completion) is still executing (IVS-673). Failed tasks return early above - # and intentionally don't advance progress. Use an atomic DB-side update - # because parallel-stage tasks increment progress concurrently. + # Advance progress only after the work is done, so a request never shows + # 100% while a long-running task (e.g. instance completion) is still running. + # Failed tasks returned early above. Atomic: parallel tasks increment together. ValidationRequest.objects.filter(pk=id).update( progress=Least(F("progress") + config.increment, Value(100)) ) diff --git a/backend/apps/ifc_validation/tests/tests_progress_update.py b/backend/apps/ifc_validation/tests/tests_progress_update.py index c75a24de..0aa91cf9 100644 --- a/backend/apps/ifc_validation/tests/tests_progress_update.py +++ b/backend/apps/ifc_validation/tests/tests_progress_update.py @@ -23,11 +23,8 @@ def _noop_lock(*args, **kwargs): @mock.patch.object(task_runner, "acquire_user_lock", _noop_lock) class ProgressUpdateTaskTestCase(TransactionTestCase): - """ - Regression tests for IVS-673: the request must not report 100% (or advance - its progress at all) while a subtask is still executing. Progress should only - advance *after* a task's execution + processing layers have finished. - """ + """Progress must advance only after a subtask's work finishes, so a request + never reports 100% while a task is still running.""" TASK_TYPE = ValidationTask.Type.SCHEMA # increment = 10 @@ -71,12 +68,12 @@ def fake_process(context): mock.patch.object(self.config, "process_results", side_effect=fake_process): self._run(request) - # While the task body executed, progress had NOT advanced yet (the IVS-673 bug - # was that it had already been bumped before the work ran). + # During the task body, progress must not have advanced (the bug: it was + # bumped before the work ran). self.assertEqual(seen["during_check"], 0) self.assertEqual(seen["during_process"], 0) - # Only once the task finished does progress advance by the configured increment. + # It advances by the increment only after the task finishes. request.refresh_from_db() self.assertEqual(request.progress, self.increment)