Fix crash when a job's tasks all end up canceled - #262
Fix crash when a job's tasks all end up canceled#262NumericalAdvantage wants to merge 2 commits into
Conversation
update_job_state() derived a finished job's status from success, warning and failure tasks only. A job whose tasks are all CANCELED matched no branch and fell through to the AssertionError, so cancelling every task of a job 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. Also stop reporting "All tasks ..." when some tasks were canceled: a job with one succeeded and one canceled task claimed "All tasks succeeded.", which is untrue. The existing "Some tasks ..." wording is now used whenever canceled tasks are present. Removes test_job_update_job_state_with_only_canceled_tasks, which asserted the crash was correct behaviour; the scenario is now covered with the expected outcome instead. This mirrors the same fix for ADIT (openradx/adit#387).
📝 WalkthroughWalkthroughThe job status flow now supports ChangesCanceled job state handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 71: Update the status flow documentation in AGENTS.md to show that
CANCELED may follow CANCELING and may result when a job has no remaining tasks
or only canceled tasks, rather than implying it only follows IN_PROGRESS.
Preserve the existing transitions and clearly separate these cancellation
outcomes while maintaining the document’s current structure.
🪄 Autofix
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: 9a1d32ac-3048-4e66-9b99-fa17cb822ee2
📒 Files selected for processing (3)
AGENTS.mdradis/core/models.pyradis/core/tests/test_models.py
|
|
||
| - An **AnalysisJob** contains multiple **AnalysisTasks** | ||
| - Status flow: `UNVERIFIED` -> `PREPARING` -> `PENDING` -> `IN_PROGRESS` -> `SUCCESS`/`WARNING`/`FAILURE` | ||
| - Status flow: `UNVERIFIED` -> `PREPARING` -> `PENDING` -> `IN_PROGRESS` -> `SUCCESS`/`WARNING`/`FAILURE`/`CANCELED` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the cancellation paths accurately.
CANCELED can follow CANCELING. It can also result when a job has no remaining tasks or only canceled tasks. The linear flow in Line 71 implies that CANCELED only follows IN_PROGRESS.
Proposed documentation update
- Status flow: `UNVERIFIED` -> `PREPARING` -> `PENDING` -> `IN_PROGRESS` -> `SUCCESS`/`WARNING`/`FAILURE`/`CANCELED`
+ Completion flow: `UNVERIFIED` -> `PREPARING` -> `PENDING` -> `IN_PROGRESS` -> `SUCCESS`/`WARNING`/`FAILURE`
+ Cancellation outcome: `CANCELING`, jobs with no tasks, and jobs with only canceled tasks become `CANCELED`.As per coding guidelines, “Maintain clear structure of agent descriptions, capabilities, and usage in AGENTS.md”.
📝 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.
| - Status flow: `UNVERIFIED` -> `PREPARING` -> `PENDING` -> `IN_PROGRESS` -> `SUCCESS`/`WARNING`/`FAILURE`/`CANCELED` | |
| - Completion flow: `UNVERIFIED` -> `PREPARING` -> `PENDING` -> `IN_PROGRESS` -> `SUCCESS`/`WARNING`/`FAILURE` | |
| - Cancellation outcome: `CANCELING`, jobs with no tasks, and jobs with only canceled tasks become `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 71, Update the status flow documentation in AGENTS.md to
show that CANCELED may follow CANCELING and may result when a job has no
remaining tasks or only canceled tasks, rather than implying it only follows
IN_PROGRESS. Preserve the existing transitions and clearly separate these
cancellation outcomes while maintaining the document’s current structure.
Source: Coding guidelines
Mirrors openradx/adit#387 — RADIS has the identical bug.
Bug:
update_job_state()derives a finished job's status from success/warning/failure tasks. A job whose tasks are all canceled matches no branch and hitsraise AssertionError. Reproduced on current main.Fix: a canceled branch, placed last so it only applies when no success, warning or failure task exists.
Also: a job with one succeeded + one canceled task reported "All tasks succeeded." — untrue, and for an analysis job misleading. The existing "Some tasks ..." wording is now used whenever canceled tasks are present. Statuses are unchanged; only messages.
Note: removes
test_job_update_job_state_with_only_canceled_tasks, added in #147, which asserted the crash was correct behaviour (pytest.raises(AssertionError)). The scenario is now covered with the expected outcome instead. Worth knowing the crash was observable in the suite since Nov 2025.Tests: 4 added (all-canceled; canceled must not mask a failure; and the two message cases). Full suite: 621 passed, ruff + pyright clean.
Summary by CodeRabbit
New Features
Bug Fixes
Tests