From f3b7c21805fda5b664d838a46f6a7c5d3ee3d668 Mon Sep 17 00:00:00 2001 From: Tim 'mithro' Ansell Date: Tue, 9 Jun 2026 12:18:24 +0930 Subject: [PATCH 1/2] fix: schedule superseded-check cleanup (#261) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_cancel_superseded_checks()` cancels in-progress checks that have been superseded by a newer check for the same project file. It was only reachable via the combined `checks_cleanup()` task, which was never registered in `CELERY_BEAT_SCHEDULE` — so in dev/stage/prod the superseded-cancel logic never ran. Root cause: the commit that introduced `checks_cleanup()` / `_cancel_superseded_checks()` (de548dd) only touched the task module and its tests; it never added a beat-schedule entry. The combined task also duplicated two cleanups (`checks_cleanup_stale_files`, `checks_cleanup_stale_pending_tasks`) that were already scheduled independently, so scheduling it as-is would double-run them. The logic is genuinely needed: the manual DRC-update requeue view (`check_drc_update_requeue`) can create a newer check for a file whose latest check is still in progress, leaving the older one superseded with nothing to cancel it. (The scheduled `checks_drc_update_requeue` beat task only requeues FINISHED checks, so it never triggers this path.) Changes: - Extract the superseded-cancel logic into a single-responsibility `checks_cleanup_superseded()` task (matches the codebase's per-state task architecture) and schedule it at 60s in CELERY_BEAT_SCHEDULE. - Remove the never-scheduled, redundant combined `checks_cleanup()`. - Narrow the broad `except Exception` to `InvalidStateTransitionError`, matching the sibling `checks_cleanup_stale_files`. - Repoint the existing superseded-cancel tests at the new task. - Fix the misleading `create_check_drc_update` docstring and update the Celery task reference doc. Co-Authored-By: Claude Opus 4.8 (1M context) --- config/settings/base.py | 4 ++ docs/celery_tasks_reference.md | 2 +- wafer_space/projects/models.py | 5 ++- wafer_space/projects/tasks_checks.py | 50 +++++++----------------- wafer_space/projects/tests/test_tasks.py | 8 ++-- 5 files changed, 27 insertions(+), 42 deletions(-) diff --git a/config/settings/base.py b/config/settings/base.py index 4cb96e70..f7c1f035 100644 --- a/config/settings/base.py +++ b/config/settings/base.py @@ -551,6 +551,10 @@ def required_host_list(var: str) -> list[str]: "task": "wafer_space.projects.tasks_checks.checks_cleanup_stale_pending_tasks", "schedule": 60.0, }, + "checks-cleanup-superseded": { + "task": "wafer_space.projects.tasks_checks.checks_cleanup_superseded", + "schedule": 60.0, + }, "checks-drc-update-requeue": { "task": "wafer_space.projects.tasks_checks.checks_drc_update_requeue", "schedule": 60.0, diff --git a/docs/celery_tasks_reference.md b/docs/celery_tasks_reference.md index 9116b380..53f8b6c2 100644 --- a/docs/celery_tasks_reference.md +++ b/docs/celery_tasks_reference.md @@ -49,8 +49,8 @@ These tasks run periodically via Celery Beat to poll for checks needing action. | `checks_retry` | `none:ro:checks-orch` | 60s | Create retry checks for ERROR state | | `checks_cleanup_stale_files` | `none:ro:checks-orch` | 60s | Cancel checks on inactive files | | `checks_cleanup_stale_pending_tasks` | `none:ro:checks-orch` | 60s | Remove orphaned task tracking records | +| `checks_cleanup_superseded` | `none:ro:checks-orch` | 60s | Cancel checks superseded by newer ones | | `checks_drc_update_requeue` | `none:ro:checks-orch` | 60s | Create DRC_UPDATE checks for outdated versions | -| `checks_cleanup` | `none:ro:checks-orch` | - | Combined cleanup operations | ### Work Tasks diff --git a/wafer_space/projects/models.py b/wafer_space/projects/models.py index da0eb0f8..2356c75a 100644 --- a/wafer_space/projects/models.py +++ b/wafer_space/projects/models.py @@ -2370,8 +2370,9 @@ def root_check(self) -> "ManufacturabilityCheck": def create_check_drc_update(self) -> "ManufacturabilityCheck": """Create a new pending check to re-run with latest precheck version. - If this check is still in progress, it will be automatically cancelled - by the existing superseded check cleanup logic. + If this check is still in progress, the newly created check supersedes + it; the older in-progress check is then cancelled by the scheduled + ``checks_cleanup_superseded`` task. Returns: The newly created ManufacturabilityCheck. diff --git a/wafer_space/projects/tasks_checks.py b/wafer_space/projects/tasks_checks.py index bd296f9c..9f8cc2da 100644 --- a/wafer_space/projects/tasks_checks.py +++ b/wafer_space/projects/tasks_checks.py @@ -191,10 +191,10 @@ def wrapper(check_id: int, *args: Any, **kwargs: Any) -> T | dict[str, str]: __all__ = [ "checks_analyzing", "checks_cancelling", - "checks_cleanup", "checks_cleanup_orphaned_docker", "checks_cleanup_stale_files", "checks_cleanup_stale_pending_tasks", + "checks_cleanup_superseded", "checks_create", "checks_dispatching", "checks_drc_update_requeue", @@ -1948,11 +1948,21 @@ def checks_cleanup_stale_files() -> dict: return {"cancelled": cancelled} -def _cancel_superseded_checks() -> int: +@checks_task() +def checks_cleanup_superseded() -> dict: """Cancel in-progress checks that have been superseded by newer checks. + A check is "superseded" when a newer check exists for the same project + file while the older one is still in progress. This happens via the manual + DRC-update requeue view (``check_drc_update_requeue``), which can create a + fresh check for a file whose latest check is still running. The older, + now-redundant in-progress check is marked for cancellation here. + + The scheduled ``checks_drc_update_requeue`` beat task does not produce this + situation, because it only requeues FINISHED checks. + Returns: - Number of checks marked for cancellation. + Dict with 'cancelled' count of checks marked for cancellation. """ logger = logging.getLogger(__name__) @@ -1976,40 +1986,10 @@ def _cancel_superseded_checks() -> int: check.id, ) cancelled += 1 - except Exception: + except InvalidStateTransitionError: logger.exception("Failed to cancel superseded check %s", check.id) - return cancelled - - -@checks_task() -def checks_cleanup() -> dict: - """Cleanup task that performs all periodic cleanup operations. - - This task combines multiple cleanup operations: - - Cancel checks superseded by newer checks - - Cancel checks on inactive project files - - Remove orphaned pending task records - - Returns: - Dict with counts of cleanup operations performed. - """ - # Cancel superseded checks - superseded_cancelled = _cancel_superseded_checks() - - # Cancel checks on stale files - stale_files_result = checks_cleanup_stale_files() - stale_files_cancelled = stale_files_result.get("cancelled", 0) - - # Clean up orphaned pending tasks - pending_tasks_result = checks_cleanup_stale_pending_tasks() - pending_tasks_deleted = pending_tasks_result.get("deleted", 0) - - return { - "superseded_cancelled": superseded_cancelled, - "stale_files_cancelled": stale_files_cancelled, - "pending_tasks_deleted": pending_tasks_deleted, - } + return {"cancelled": cancelled} @checks_task() diff --git a/wafer_space/projects/tests/test_tasks.py b/wafer_space/projects/tests/test_tasks.py index 4507aca5..d239c09d 100644 --- a/wafer_space/projects/tests/test_tasks.py +++ b/wafer_space/projects/tests/test_tasks.py @@ -56,8 +56,8 @@ from wafer_space.projects.tasks import do_starting from wafer_space.projects.tasks import download_project_file from wafer_space.projects.tasks_checks import _save_output_gds -from wafer_space.projects.tasks_checks import checks_cleanup from wafer_space.projects.tasks_checks import checks_cleanup_stale_pending_tasks +from wafer_space.projects.tasks_checks import checks_cleanup_superseded from wafer_space.projects.tasks_checks import checks_drc_update_requeue from wafer_space.projects.tasks_download import _apply_post_download_processing from wafer_space.projects.tasks_download import _initialize_hash_calculators @@ -2625,7 +2625,7 @@ def test_cancels_older_in_progress_check_when_newer_exists(self) -> None: status=ManufacturabilityCheck.Status.PENDING, ) - checks_cleanup() + checks_cleanup_superseded() old_check.refresh_from_db() assert old_check.status == ManufacturabilityCheck.Status.CANCELLING @@ -2640,7 +2640,7 @@ def test_does_not_cancel_if_no_newer_check(self) -> None: status=ManufacturabilityCheck.Status.RUNNING, ) - checks_cleanup() + checks_cleanup_superseded() check.refresh_from_db() assert check.status == ManufacturabilityCheck.Status.RUNNING @@ -2660,7 +2660,7 @@ def test_does_not_cancel_finished_checks(self) -> None: status=ManufacturabilityCheck.Status.PENDING, ) - checks_cleanup() + checks_cleanup_superseded() old_check.refresh_from_db() assert old_check.status == ManufacturabilityCheck.Status.FINISHED From 9d91f6848c7c6110c149459ecba302539194fd61 Mon Sep 17 00:00:00 2001 From: Tim 'mithro' Ansell Date: Wed, 10 Jun 2026 17:32:00 +0930 Subject: [PATCH 2/2] fix: skip already-CANCELLING checks in superseded cleanup Status.in_progress() includes CANCELLING, but CANCELLING -> CANCELLING is not a valid transition. A superseded check that was already marked CANCELLING would be re-selected on every 60s run and produce a spurious ERROR-level traceback until checks_cancelling completed it. Exclude CANCELLING from the candidate query; the checks_cancelling task owns completing those. Adds a regression test asserting no ERROR logs are emitted and the check is left untouched. Also fixes the stale test class docstring. Co-Authored-By: Claude Fable 5 --- wafer_space/projects/tasks_checks.py | 14 ++++++++---- wafer_space/projects/tests/test_tasks.py | 27 +++++++++++++++++++++++- 2 files changed, 36 insertions(+), 5 deletions(-) diff --git a/wafer_space/projects/tasks_checks.py b/wafer_space/projects/tasks_checks.py index 9f8cc2da..23ba1b90 100644 --- a/wafer_space/projects/tasks_checks.py +++ b/wafer_space/projects/tasks_checks.py @@ -1972,10 +1972,16 @@ def checks_cleanup_superseded() -> dict: created_at__gt=OuterRef("created_at"), ) - # Find all superseded in-progress checks - superseded = ManufacturabilityCheck.objects.filter( - status__in=ManufacturabilityCheck.Status.in_progress(), - ).filter(Exists(newer_exists)) + # Find all superseded in-progress checks. Checks already in CANCELLING + # are excluded: CANCELLING -> CANCELLING is not a valid transition, and + # the checks_cancelling task completes them. + superseded = ( + ManufacturabilityCheck.objects.filter( + status__in=ManufacturabilityCheck.Status.in_progress(), + ) + .exclude(status=ManufacturabilityCheck.Status.CANCELLING) + .filter(Exists(newer_exists)) + ) cancelled = 0 for check in superseded: diff --git a/wafer_space/projects/tests/test_tasks.py b/wafer_space/projects/tests/test_tasks.py index d239c09d..c41f3149 100644 --- a/wafer_space/projects/tests/test_tasks.py +++ b/wafer_space/projects/tests/test_tasks.py @@ -2608,7 +2608,7 @@ def test_returns_zero_when_no_pending_tasks(self) -> None: class TestCancelSupersededChecks: - """Tests for cancel_superseded_checks functionality.""" + """Tests for the checks_cleanup_superseded task.""" @pytest.mark.django_db def test_cancels_older_in_progress_check_when_newer_exists(self) -> None: @@ -2665,6 +2665,31 @@ def test_does_not_cancel_finished_checks(self) -> None: old_check.refresh_from_db() assert old_check.status == ManufacturabilityCheck.Status.FINISHED + @pytest.mark.django_db + def test_skips_checks_already_cancelling_without_error_logs( + self, caplog: pytest.LogCaptureFixture + ) -> None: + """Checks already in CANCELLING are skipped, not re-cancelled with errors.""" + project_file = ProjectFileFactory() + old_check = ManufacturabilityCheckFactory( + project=project_file.project, + project_file=project_file, + status=ManufacturabilityCheck.Status.CANCELLING, + ) + ManufacturabilityCheckFactory( + project=project_file.project, + project_file=project_file, + status=ManufacturabilityCheck.Status.PENDING, + ) + + with caplog.at_level(logging.ERROR, logger="wafer_space.projects.tasks_checks"): + result = checks_cleanup_superseded() + + assert result == {"cancelled": 0} + assert not caplog.records + old_check.refresh_from_db() + assert old_check.status == ManufacturabilityCheck.Status.CANCELLING + @pytest.mark.django_db class TestChecksDrcUpdateRequeue: