fix: schedule superseded-check cleanup (#261) - #262
Open
mithro wants to merge 2 commits into
Open
Conversation
WalkthroughThe change extracts superseded-check cancellation into a dedicated Celery task. Celery Beat runs it every 60 seconds. Documentation, model references, exports, and tests now use the dedicated task. ChangesScheduled Cleanup for Superseded Checks
Estimated code review effort: 2 (Simple) | ~12 minutes Sequence Diagram(s)sequenceDiagram
participant CeleryBeat
participant checks_cleanup_superseded
participant CheckDatabase
CeleryBeat->>checks_cleanup_superseded: Run every 60 seconds
checks_cleanup_superseded->>CheckDatabase: Find superseded in-progress checks
checks_cleanup_superseded->>CheckDatabase: Mark eligible checks as CANCELLING
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wafer_space/projects/tasks_checks.py (1)
1941-1944:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a deterministic tie-breaker for superseded detection.
Line 1943 only compares
created_at, so two checks created with identical timestamps are treated as neither newer nor older. That can skip cancellation for a truly superseded check.Suggested fix
+from django.db.models import Q from django.db.models import Subquery @@ newer_exists = ManufacturabilityCheck.objects.filter( project_file=OuterRef("project_file"), - created_at__gt=OuterRef("created_at"), -) +).filter( + Q(created_at__gt=OuterRef("created_at")) + | ( + Q(created_at=OuterRef("created_at")) + & Q(id__gt=OuterRef("id")) + ) +)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wafer_space/projects/tasks_checks.py` around lines 1941 - 1944, The current newer_exists query only compares created_at and can miss ties; update the filter in the ManufacturabilityCheck subquery to include a deterministic tie-breaker by checking either created_at__gt=OuterRef("created_at") OR (created_at=OuterRef("created_at") AND pk__gt=OuterRef("pk")). Replace the existing newer_exists definition to use Q(...) with the OR condition (using OuterRef("created_at") and OuterRef("pk")) so checks with identical timestamps are deterministically ordered by primary key.
🧹 Nitpick comments (1)
wafer_space/projects/tests/test_tasks.py (1)
2127-2179: ⚡ Quick winVerify return values to match test patterns.
The three test methods in
TestCancelSupersededCheckscallchecks_cleanup_superseded()but don't capture or verify the return value. Other cleanup task tests in this file consistently verify return values (e.g.,TestChecksRetryassertsresult["retried"] == 1,TestChecksCancellingassertsresult["cancelled"] == 1).♻️ Proposed enhancement to verify return values
For
test_cancels_older_in_progress_check_when_newer_exists:- checks_cleanup_superseded() + result = checks_cleanup_superseded() old_check.refresh_from_db() assert old_check.status == ManufacturabilityCheck.Status.CANCELLING + assert result["cancelled"] == 1For
test_does_not_cancel_if_no_newer_check:- checks_cleanup_superseded() + result = checks_cleanup_superseded() check.refresh_from_db() assert check.status == ManufacturabilityCheck.Status.RUNNING + assert result["cancelled"] == 0For
test_does_not_cancel_finished_checks:- checks_cleanup_superseded() + result = checks_cleanup_superseded() old_check.refresh_from_db() assert old_check.status == ManufacturabilityCheck.Status.FINISHED + assert result["cancelled"] == 0🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wafer_space/projects/tests/test_tasks.py` around lines 2127 - 2179, The tests in TestCancelSupersededChecks call checks_cleanup_superseded() but don't assert its return value; update each test (test_cancels_older_in_progress_check_when_newer_exists, test_does_not_cancel_if_no_newer_check, test_does_not_cancel_finished_checks) to capture the result = checks_cleanup_superseded() and add assertions matching the expected metrics (e.g., result["cancelled"] == 1 when an in-progress check is cancelled, and result["cancelled"] == 0 for the other two cases) so the tests follow the same return-value verification pattern as TestChecksRetry/TestChecksCancelling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/celery_tasks_reference.md`:
- Around line 52-53: The beat summary's "60 seconds" section is missing the
newly added task checks_cleanup_superseded; update the summary list in
docs/celery_tasks_reference.md to include checks_cleanup_superseded alongside
checks_drc_update_requeue under the 60 seconds/60s entry so the table and the
verbal summary match (ensure the summary references the exact task names
checks_cleanup_superseded and checks_drc_update_requeue).
---
Outside diff comments:
In `@wafer_space/projects/tasks_checks.py`:
- Around line 1941-1944: The current newer_exists query only compares created_at
and can miss ties; update the filter in the ManufacturabilityCheck subquery to
include a deterministic tie-breaker by checking either
created_at__gt=OuterRef("created_at") OR (created_at=OuterRef("created_at") AND
pk__gt=OuterRef("pk")). Replace the existing newer_exists definition to use
Q(...) with the OR condition (using OuterRef("created_at") and OuterRef("pk"))
so checks with identical timestamps are deterministically ordered by primary
key.
---
Nitpick comments:
In `@wafer_space/projects/tests/test_tasks.py`:
- Around line 2127-2179: The tests in TestCancelSupersededChecks call
checks_cleanup_superseded() but don't assert its return value; update each test
(test_cancels_older_in_progress_check_when_newer_exists,
test_does_not_cancel_if_no_newer_check, test_does_not_cancel_finished_checks) to
capture the result = checks_cleanup_superseded() and add assertions matching the
expected metrics (e.g., result["cancelled"] == 1 when an in-progress check is
cancelled, and result["cancelled"] == 0 for the other two cases) so the tests
follow the same return-value verification pattern as
TestChecksRetry/TestChecksCancelling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 95c9e35c-18ad-4b6d-bf2b-29b2e8e375d1
📒 Files selected for processing (5)
config/settings/base.pydocs/celery_tasks_reference.mdwafer_space/projects/models.pywafer_space/projects/tasks_checks.pywafer_space/projects/tests/test_tasks.py
mithro
force-pushed
the
issue/261-checks-cleanup
branch
from
June 10, 2026 18:00
d9ad686 to
330f279
Compare
4 tasks
`_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) <noreply@anthropic.com>
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 <noreply@anthropic.com>
mithro
force-pushed
the
issue/261-checks-cleanup
branch
from
August 2, 2026 08:12
330f279 to
9d91f68
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
wafer_space/projects/tests/test_tasks.py (1)
2668-2691: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit this test by assertion concept.
This test verifies the result contract, absence of error logs, and persisted status. Put each behavior in a separate test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wafer_space/projects/tests/test_tasks.py` around lines 2668 - 2691, Split test_skips_checks_already_cancelling_without_error_logs into separate tests covering the checks_cleanup_superseded result contract, absence of error logs, and persisted CANCELLING status. Keep the shared factory setup and scenario unchanged while giving each test a single assertion concept.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@wafer_space/projects/tests/test_tasks.py`:
- Around line 2668-2691: Split
test_skips_checks_already_cancelling_without_error_logs into separate tests
covering the checks_cleanup_superseded result contract, absence of error logs,
and persisted CANCELLING status. Keep the shared factory setup and scenario
unchanged while giving each test a single assertion concept.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d331173d-a948-4781-9d8a-6deea1d4ab18
📒 Files selected for processing (5)
config/settings/base.pydocs/celery_tasks_reference.mdwafer_space/projects/models.pywafer_space/projects/tasks_checks.pywafer_space/projects/tests/test_tasks.py
🚧 Files skipped from review as they are similar to previous changes (3)
- config/settings/base.py
- docs/celery_tasks_reference.md
- wafer_space/projects/models.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #261.
_cancel_superseded_checks()— which cancels in-progress checkssuperseded by a newer check on the same project file — was only reachable via
the combined
checks_cleanup()task, which was never registered inCELERY_BEAT_SCHEDULE. So in dev/stage/prod the superseded-cancel logic neverran.
Root cause
checks_cleanup()/_cancel_superseded_checks()(
de548dd) only touched the task module and its tests — it never added a beatentry. The bare
"checks-cleanup"schedule key has never existed on anybranch.
checks_cleanup()also duplicated two cleanups(
checks_cleanup_stale_files,checks_cleanup_stale_pending_tasks) that werealready scheduled independently, so scheduling it as-is would double-run
them. It is a leftover "combined task" anachronism in a codebase that had
already moved to single-responsibility polling tasks.
Why the logic is still needed (not dead code)
The issue suspected
_cancel_superseded_checks()might be dead. It is not.Besides the beat task
checks_drc_update_requeue(which only requeuesFINISHED checks),
create_check_drc_update()has a second caller: themanual view
check_drc_update_requeue(views.py:969). Its requeue button isgated on
check.is_using_latest_precheck is False— not terminal status —and in-progress checks carry a
docker_image_digest, so they satisfy all thecreate_check_drc_updateguards. A staff/owner can therefore requeue a checkwhose latest check is still running, leaving the older one superseded with
nothing to cancel it.
Changes
checks_cleanup_superseded()task and schedule it at 60s inCELERY_BEAT_SCHEDULE(matches the per-state task architecture).checks_cleanup().except ExceptiontoInvalidStateTransitionError, matchingthe sibling
checks_cleanup_stale_files.create_check_drc_updatedocstring and update the Celerytask reference doc.
Verification
make lint,make type-checkclean.make test: 1279 passed. The one failure (test_detect_zip_file, a libmagicMIME quirk) is pre-existing on untouched
mainand unrelated to this change.key are gone.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation