Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ The job status is derived from its tasks via `post_process()`. The evaluation pr
1. Any PENDING task → job becomes PENDING (unless job is CANCELING)
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`

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 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.

Suggested change
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.


### Job Actions

Expand Down
6 changes: 5 additions & 1 deletion adit/core/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,7 @@ def post_process(self, suppress_email=False) -> bool:
has_success = self.tasks.filter(status=DicomTask.Status.SUCCESS).exists()
has_warning = self.tasks.filter(status=DicomTask.Status.WARNING).exists()
has_failure = self.tasks.filter(status=DicomTask.Status.FAILURE).exists()
has_canceled = self.tasks.filter(status=DicomTask.Status.CANCELED).exists()

if has_success and not has_warning and not has_failure:
self.status = DicomJob.Status.SUCCESS
Expand All @@ -307,8 +308,11 @@ def post_process(self, suppress_email=False) -> bool:
elif has_failure:
self.status = DicomJob.Status.FAILURE
self.message = "All tasks failed."
elif has_canceled:
self.status = DicomJob.Status.CANCELED
self.message = "All tasks were canceled."
else:
# at least one of success, warnings or failures must be > 0
# at least one of success, warnings, failures or cancellations must be > 0
raise AssertionError(f"Invalid task status list of {self}.")

self.end = timezone.now()
Expand Down
32 changes: 32 additions & 0 deletions adit/core/tests/test_models.py
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,38 @@ def test_job_post_process_canceling_status(self):
assert job.status == DicomJob.Status.CANCELED
assert job.end is None

@pytest.mark.django_db
def test_job_post_process_all_tasks_canceled(self):
# A job that is no longer CANCELING (so it reaches the final evaluation)
# but whose tasks all ended up canceled, e.g. after killing them from the
# admin. Without a canceled branch this raised an AssertionError.
job = ExampleTransferJobFactory.create(status=DicomJob.Status.PENDING)

ExampleTransferTaskFactory.create(job=job, status=DicomTask.Status.CANCELED)
ExampleTransferTaskFactory.create(job=job, status=DicomTask.Status.CANCELED)

result = job.post_process()
job.refresh_from_db()

assert result is True
assert job.status == DicomJob.Status.CANCELED
assert job.message == "All tasks were canceled."
assert job.end is not None

@pytest.mark.django_db
def test_job_post_process_canceled_task_does_not_mask_failure(self):
# A canceled task alongside a failed one must still resolve to FAILURE;
# the canceled branch only applies when nothing else is present.
job = ExampleTransferJobFactory.create(status=DicomJob.Status.PENDING)

ExampleTransferTaskFactory.create(job=job, status=DicomTask.Status.CANCELED)
ExampleTransferTaskFactory.create(job=job, status=DicomTask.Status.FAILURE)

job.post_process()
job.refresh_from_db()

assert job.status == DicomJob.Status.FAILURE

@pytest.mark.django_db
@time_machine.travel("2025-01-15 14:30:00+00:00")
def test_job_timezone_correctness(self):
Expand Down