Fix crash when a job's tasks all end up canceled - #387
Fix crash when a job's tasks all end up canceled#387NumericalAdvantage wants to merge 3 commits into
Conversation
post_process() evaluated the final job status from success, warning and failure tasks only. A finished job whose tasks are all CANCELED matched none of those branches and fell through to the AssertionError, so killing every task of a job from the admin crashed instead of leaving the job canceled. Add a canceled branch as the last case, so it only applies when no success, warning or failure task is present; mixed jobs keep resolving as before. Picks up the still-missing half of #311, whose branch no longer merges. The CancelledError handling from that PR is already on main.
|
Warning Review limit reached
Next review available in: 6 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesDICOM job status handling
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 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)
adit/core/models.py (1)
294-315: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTreat invalid DicomTask statuses as exceptions, not task values.
post_process()only ignores invalid task statuses in thePENDING/IN_PROGRESSbranches. For other task statuses, a stray invalid value makes theCANCELEDpath reachable instead of theelseassertion. Add a database/model-levelDicomTask.Statusconstraint, or require that no task exists outsideDicomTask.Status.CANCELEDbefore selectingCANCELED.🤖 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 `@adit/core/models.py` around lines 294 - 315, The post_process status selection must not classify invalid DicomTask statuses as canceled. Add a database/model-level constraint restricting task statuses to DicomTask.Status values, or update the canceled branch to require that no tasks exist outside DicomTask.Status.CANCELED before assigning DicomJob.Status.CANCELED, leaving invalid values to reach the existing assertion.
🧹 Nitpick comments (2)
adit/core/tests/test_models.py (1)
189-201: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover canceled tasks with success and warning.
The new regression test covers
CANCELED + FAILUREonly. Add cases forCANCELED + SUCCESSandCANCELED + WARNINGto verify that the new final branch remains after the existing success and warning precedence.🤖 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 `@adit/core/tests/test_models.py` around lines 189 - 201, Add regression cases alongside test_job_post_process_canceled_task_does_not_mask_failure for CANCELED + SUCCESS and CANCELED + WARNING task combinations, asserting post_process() preserves SUCCESS and WARNING respectively. Keep the canceled-only fallback after the existing success and warning precedence, and use the existing factories and status symbols.adit/core/models.py (1)
311-313: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueAdd a real test for canceled finished-mail delivery.
The canceled finalization calls
send_job_finished_mail(job), the template only uses{{ job }},{{ job.get_absolute_url }}, and{{ job.message }}, and the mail helper still callssend_mail_to_user(). Add a covered case forsend_finished_mail=Truewith all-cancelled tasks so this notification path cannot regress.🤖 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 `@adit/core/models.py` around lines 311 - 313, Add a test covering canceled job finalization when send_finished_mail=True and every task is canceled, exercising the send_job_finished_mail path and verifying the resulting notification is delivered through send_mail_to_user with the canceled job message and URL context. Use the existing job-finalization test fixtures and conventions, and ensure the test would fail if canceled finished-mail delivery regresses.
🤖 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 `@AGENTS.md`:
- Line 85: Update the job final-status documentation in the task-status
computation description to replace “but canceled tasks are” with “but all tasks
are canceled,” explicitly stating that CANCELED requires every task to be
canceled when no SUCCESS, WARNING, or FAILURE tasks exist.
---
Outside diff comments:
In `@adit/core/models.py`:
- Around line 294-315: The post_process status selection must not classify
invalid DicomTask statuses as canceled. Add a database/model-level constraint
restricting task statuses to DicomTask.Status values, or update the canceled
branch to require that no tasks exist outside DicomTask.Status.CANCELED before
assigning DicomJob.Status.CANCELED, leaving invalid values to reach the existing
assertion.
---
Nitpick comments:
In `@adit/core/models.py`:
- Around line 311-313: Add a test covering canceled job finalization when
send_finished_mail=True and every task is canceled, exercising the
send_job_finished_mail path and verifying the resulting notification is
delivered through send_mail_to_user with the canceled job message and URL
context. Use the existing job-finalization test fixtures and conventions, and
ensure the test would fail if canceled finished-mail delivery regresses.
In `@adit/core/tests/test_models.py`:
- Around line 189-201: Add regression cases alongside
test_job_post_process_canceled_task_does_not_mask_failure for CANCELED + SUCCESS
and CANCELED + WARNING task combinations, asserting post_process() preserves
SUCCESS and WARNING respectively. Keep the canceled-only fallback after the
existing success and warning precedence, and use the existing factories and
status symbols.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 61f2d3c0-93f6-403d-b8af-37d4bae91384
📒 Files selected for processing (3)
AGENTS.mdadit/core/models.pyadit/core/tests/test_models.py
| 2. Any IN_PROGRESS task → job becomes IN_PROGRESS (unless job is CANCELING) | ||
| 3. If job was `CANCELING` → job becomes `CANCELED` | ||
| 4. Otherwise the job is finished and its final status is computed from the combination of `SUCCESS`, `WARNING`, and `FAILURE` tasks | ||
| 4. Otherwise the job is finished and its final status is computed from the combination of `SUCCESS`, `WARNING`, and `FAILURE` tasks. If none of those are present but canceled tasks are, the job becomes `CANCELED` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the all-canceled condition explicitly.
The phrase “but canceled tasks are” can describe a job with one canceled task. The implementation intends CANCELED only when no SUCCESS, WARNING, or FAILURE task exists and all tasks are canceled. Replace the phrase with “but all tasks are canceled.”
Proposed wording
-4. Otherwise the job is finished and its final status is computed from the combination of `SUCCESS`, `WARNING`, and `FAILURE` tasks. If none of those are present but canceled tasks are, the job becomes `CANCELED`
+4. Otherwise the job is finished and its final status is computed from the combination of `SUCCESS`, `WARNING`, and `FAILURE` tasks. If none of those are present but all tasks are canceled, the job becomes `CANCELED`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 4. Otherwise the job is finished and its final status is computed from the combination of `SUCCESS`, `WARNING`, and `FAILURE` tasks. If none of those are present but canceled tasks are, the job becomes `CANCELED` | |
| 4. Otherwise the job is finished and its final status is computed from the combination of `SUCCESS`, `WARNING`, and `FAILURE` tasks. If none of those are present but all tasks are canceled, the job becomes `CANCELED` |
🤖 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 `@AGENTS.md` at line 85, Update the job final-status documentation in the
task-status computation description to replace “but canceled tasks are” with
“but all tasks are canceled,” explicitly stating that CANCELED requires every
task to be canceled when no SUCCESS, WARNING, or FAILURE tasks exist.
A job with one succeeded and one canceled task reported "All tasks succeeded.", which is untrue and, for a transfer job, misleading: it suggests data moved that never did. The same applies to "All tasks failed." and "All tasks have warnings." when canceled tasks are present. Fall back to the existing "Some tasks ..." wording whenever canceled tasks exist. Job statuses themselves are unchanged; only the messages are corrected.
1 participant
Redo of #311, which no longer merged (60+ commits of drift; its
CancelledErrorhandling has since landed on main separately).Bug:
DicomJob.post_process()derives a finished job's status from success/warning/failure tasks. A job whose tasks are all canceled matches no branch and hitsraise AssertionError. Killing every task of a job from the admin therefore crashes. Reproduced on current main.Fix: a canceled branch, placed last so it only applies when no success, warning or failure task exists. Mixed jobs are unaffected.
Tests: the all-canceled case (fails with
AssertionErrorwithout the fix) and a guard that a canceled task cannot mask a failure. Full suite: 856 passed, ruff + pyright clean.Also corrects the job-status note in
AGENTS.md.update_job_state()has the identical all-canceled gap.Original bug found by @Lucius1274.
Summary by CodeRabbit
Bug Fixes
CANCELEDstatus and clear completion messaging.Documentation