Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
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
18 changes: 18 additions & 0 deletions adit/core/migrations/0018_dicomfolder_suspended.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# Generated by Django 6.0.3 on 2026-04-20 14:32

from django.db import migrations, models


class Migration(migrations.Migration):

dependencies = [
('core', '0017_review_fixes'),
]

operations = [
migrations.AddField(
model_name='dicomfolder',
name='suspended',
field=models.BooleanField(default=False, help_text='Suspended destinations skip processing (e.g. disk full).'),
),
]
4 changes: 4 additions & 0 deletions adit/core/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,10 @@ class DicomFolder(DicomNode):
blank=True,
help_text="When to warn the admins by Email (used space in GB).",
)
suspended = models.BooleanField(
default=False,
help_text="Suspended destinations skip processing (e.g. disk full).",
)
Comment on lines +177 to +180

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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Find admin registrations and ModelForms bound to DicomFolder.
fd -e py | xargs rg -n -C4 'DicomFolder'

Repository: openradx/adit

Length of output: 45720


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== DicomFolder model and admin ==="
sed -n '163,182p' adit/core/models.py
sed -n '66,78p' adit/core/admin.py

echo "=== suspended/help_text references ==="
rg -n -C3 'suspended|unsuspend|Suspended' adit/core adit -g '*.py'

echo "=== BooleanField without blank in migrations ==="
python3 - <<'PY'
from pathlib import Path
import re
text = Path('adit/core/migrations/0001_initial.py').read_text()
checks = re.findall(r"models.BooleanField\([^)]*?\)", text, re.S)
for c in checks:
    print(c.replace('\n', ' ').replace('  ', ' ').strip())
PY

Repository: openradx/adit

Length of output: 23694


Add blank=True to the DicomFolder.suspended field.

DicomFolderAdmin exposes DicomFolder in Django admin. A BooleanField without blank=True is rendered as required, so an administrator cannot save the folder while suspended is unchecked. Keep this as a two-state boolean; do not add null=True, because processors.py reads it as a plain suspension gate.

🤖 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 177 - 180, Add blank=True to the
DicomFolder.suspended BooleanField while keeping default=False and excluding
null=True, so the Django admin accepts an unchecked value and the field remains
a two-state boolean.

Source: Coding guidelines


objects: DicomNodeManager["DicomFolder"] = DicomNodeManager["DicomFolder"]()

Expand Down
90 changes: 81 additions & 9 deletions adit/mass_transfer/processors.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import errno
import json
import logging
import secrets
Expand Down Expand Up @@ -74,6 +75,21 @@ def from_dict(cls, d: dict) -> "FilterSpec":

_MIN_SPLIT_WINDOW = timedelta(minutes=30)
_DELAY_BETWEEN_STUDIES = 0.5 # seconds between studies to avoid overwhelming the PACS
_SYSTEMIC_ERRNOS = (errno.ENOSPC, errno.ESTALE, errno.EROFS, errno.EIO)


def _is_systemic_error(err: Exception) -> bool:
"""Return True if the error indicates a systemic infrastructure problem.

Systemic errors (disk full, stale NFS handle, read-only filesystem, I/O error)
affect all remaining series equally, so continuing is pointless.
"""
if isinstance(err, OSError) and err.errno in _SYSTEMIC_ERRNOS:
return True
if isinstance(err, DicomError) and "Out of disk space" in str(err):
return True
return False


# Deterministic pseudonyms use 14 characters. Random pseudonyms use 15 so the
# two modes can be distinguished by length.
Expand Down Expand Up @@ -319,9 +335,19 @@ def process(self):
"log": "Task skipped because the mass transfer app is suspended.",
}

destination_node = self.mass_task.destination
if (
destination_node.node_type == DicomNode.NodeType.FOLDER
and destination_node.dicomfolder.suspended
):
return {
"status": MassTransferTask.Status.WARNING,
"message": f"Destination '{destination_node.name}' is suspended.",
"log": "Task skipped because the destination folder is suspended.",
}

job = self.mass_task.job
source_node = self.mass_task.source
destination_node = self.mass_task.destination

if source_node.node_type != DicomNode.NodeType.SERVER:
raise DicomError("Mass transfer source must be a DICOM server.")
Expand Down Expand Up @@ -374,18 +400,62 @@ def process(self):
grouped_volumes = self._group_volumes(volumes)

# Transfer: fetch series grouped by study
return self._transfer_grouped_series(
operator,
grouped_volumes,
job,
pseudonymizer,
output_base,
dest_operator,
)
try:
return self._transfer_grouped_series(
operator,
grouped_volumes,
job,
pseudonymizer,
output_base,
dest_operator,
)
except Exception as err:
if _is_systemic_error(err):
self._suspend_destination(destination_node, job, err)
return {
"status": MassTransferTask.Status.FAILURE,
"message": f"Destination suspended: {err}",
"log": (
"Systemic error detected. Destination folder suspended.\n"
f"{err}"
),
}
raise
finally:
if dest_operator:
dest_operator.close()
Comment on lines 424 to 426

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.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Close the source operator in the new finally block.

The new finally closes only dest_operator. The source operator is created at line 392 with persistent=True and is closed at line 396 on the success path, or inside _transfer_grouped_series per study. If _discover_series at line 395 or _create_pending_volumes at line 399 raises, neither close runs. The persistent DIMSE association to the source PACS stays open until the worker process exits.

_discover_series raises DicomError on its own paths, for example the window-too-small case at line 1064. So this path is reachable.

Bind the operator to a variable that the finally block can see, then close it there. DicomOperator.close() already swallows its own exceptions, so a double close is safe.

🔧 Proposed fix
         dest_operator: DicomOperator | None = None
         output_base: Path | None = None
+        operator: DicomOperator | None = None
         if destination_node.node_type == DicomNode.NodeType.SERVER:
-            if dest_operator:
+            if operator:
+                operator.close()
+            if dest_operator:
                 dest_operator.close()

Assign through the same name at line 392 so the finally block observes it:

operator = DicomOperator(source_node.dicomserver, persistent=True)
🤖 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/mass_transfer/processors.py` around lines 424 - 426, The source operator
created at line 392 with persistent=True is not being closed in the new finally
block, leaving the DIMSE association open if _discover_series or
_create_pending_volumes raises an exception. Assign the source operator at line
392 to a variable name that the finally block can access (the same variable name
used elsewhere in the function for clarity), then add a close call for this
source operator in the finally block alongside the existing
dest_operator.close() call. Since DicomOperator.close() safely handles being
called multiple times, the source operator can be safely closed in the finally
block even if it was already closed on the success path.


def _suspend_destination(
self,
destination_node: DicomNode,
job: MassTransferJob,
err: Exception,
) -> None:
"""Suspend a folder destination after a systemic error (e.g. disk full)."""
if destination_node.node_type != DicomNode.NodeType.FOLDER:
return

folder = destination_node.dicomfolder
folder.suspended = True
folder.save(update_fields=["suspended"])

logger.critical(
"Destination folder '%s' suspended due to systemic error (job %d): %s",
destination_node.name,
job.pk,
err,
)

from adit.core.utils.mail import send_mail_to_admins

send_mail_to_admins(
f"Mass transfer destination '{destination_node.name}' suspended",
f"Destination folder '{destination_node.name}' was automatically suspended "
f"due to a systemic error during mass transfer job {job.pk}.\n\n"
f"Error: {err}\n\n"
f"Please fix the underlying issue and unsuspend the folder in the admin panel.",
)
Comment on lines +439 to +457

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.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Contain mail failures and make the suspension idempotent.

Two problems affect this alerting path:

  1. send_mail_to_admins performs SMTP I/O with no error handling. If the mail backend is unavailable or raises, the exception propagates out of _suspend_destination, then out of the except Exception handler at line 412. The caller loses the intended FAILURE result dict and the task fails with an unhandled exception instead. The suspension is already persisted at that point, so the two outcomes diverge. Wrap the mail call so an alert failure cannot change the task result.

  2. Mass transfer runs many partition tasks in parallel against one destination. Every in-flight task that hits the same full disk calls _suspend_destination. Each call saves the row again and sends another administrator email. A single disk-full event then produces one email per in-flight partition. Use a conditional update and alert only when this call performed the transition.

🔧 Proposed fix
         folder = destination_node.dicomfolder
-        folder.suspended = True
-        folder.save(update_fields=["suspended"])
+        # Atomic transition so concurrent partition tasks alert only once.
+        transitioned = (
+            type(folder).objects.filter(pk=folder.pk, suspended=False).update(suspended=True) == 1
+        )
+        folder.suspended = True
+        if not transitioned:
+            return
 
         logger.critical(
             "Destination folder '%s' suspended due to systemic error (job %d): %s",
             destination_node.name,
             job.pk,
             err,
         )
 
         from adit.core.utils.mail import send_mail_to_admins
 
-        send_mail_to_admins(
-            f"Mass transfer destination '{destination_node.name}' suspended",
-            f"Destination folder '{destination_node.name}' was automatically suspended "
-            f"due to a systemic error during mass transfer job {job.pk}.\n\n"
-            f"Error: {err}\n\n"
-            f"Please fix the underlying issue and unsuspend the folder in the admin panel.",
-        )
+        try:
+            send_mail_to_admins(
+                f"Mass transfer destination '{destination_node.name}' suspended",
+                f"Destination folder '{destination_node.name}' was automatically suspended "
+                f"due to a systemic error during mass transfer job {job.pk}.\n\n"
+                f"Error: {err}\n\n"
+                f"Please fix the underlying issue and unsuspend the folder in the admin panel.",
+            )
+        except Exception:
+            logger.exception(
+                "Failed to send suspension alert for destination '%s'", destination_node.name
+            )
🤖 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/mass_transfer/processors.py` around lines 439 - 457, Wrap the
send_mail_to_admins call in error handling to prevent SMTP exceptions from
propagating out of _suspend_destination and altering the task result, since the
suspension is already persisted. Additionally, use a conditional update when
saving folder.suspended so the mail alert is sent only when this call
transitions the folder from suspended=False to suspended=True, preventing
duplicate emails when multiple concurrent tasks hit the same error.


def _create_pending_volumes(
self,
discovered: list[DiscoveredSeries],
Expand Down Expand Up @@ -626,6 +696,8 @@ def _transfer_single_series(
)
volume.status = MassTransferVolume.Status.ERROR
volume.log = str(err)
if _is_systemic_error(err):
raise
Comment on lines +699 to +700

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.

🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Reconcile the remaining PENDING volumes after a systemic abort, and update the stale contract.

The re-raise aborts the loop in _transfer_grouped_series. Volumes that were never attempted keep Status.PENDING. _build_task_summary is never reached, so nothing reconciles them.

A retry normally repairs this, because lines 381-384 delete and recreate the partition volumes. That repair no longer happens here. The destination is suspended by the same failure, so every later task for it returns early at line 339 with WARNING. The orphaned PENDING volumes then stay in the UI indefinitely and misreport the partition as still in progress.

Mark the unattempted volumes as ERROR or SKIPPED with an explanatory log when the systemic handler at line 413 runs.

The docstring at lines 621-622 states "Never raises except for RetriableDicomError." That is no longer accurate. Update it, because callers depend on the stated contract.

Do you want me to draft the reconciliation step in the systemic handler?

🤖 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/mass_transfer/processors.py` around lines 699 - 700, Update the
systemic-error handling in _transfer_grouped_series to reconcile every
unattempted volume still in PENDING, marking it ERROR or SKIPPED and emitting an
explanatory log before propagating the systemic failure. Ensure this
reconciliation runs when the systemic handler at line 413 is reached, including
volumes not yet processed. Revise the _transfer_grouped_series docstring to
state its actual exception contract rather than claiming it only raises
RetriableDicomError.

finally:
if volume.status == MassTransferVolume.Status.PENDING:
logger.error(
Expand Down
135 changes: 135 additions & 0 deletions adit/mass_transfer/tests/test_processor.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import errno
import json
from datetime import date, datetime, timedelta
from pathlib import Path
Expand Down Expand Up @@ -30,6 +31,7 @@
_birth_date_range,
_destination_base_dir,
_dicom_match,
_is_systemic_error,
_parse_int,
_series_folder_name,
_series_matches_filter,
Expand Down Expand Up @@ -713,6 +715,7 @@ def _make_process_env(
processor.mass_task.source.dicomserver = mocker.MagicMock()
processor.mass_task.destination.node_type = DicomNode.NodeType.FOLDER
processor.mass_task.destination.dicomfolder.path = str(tmp_path / "output")
processor.mass_task.destination.dicomfolder.suspended = False

processor.mass_task.pk = 42
processor.mass_task.partition_key = "20240101"
Expand Down Expand Up @@ -2462,3 +2465,135 @@ def test_process_final_attempt_all_dead_is_failure(mocker: MockerFixture, tmp_pa

assert result["status"] == MassTransferTask.Status.FAILURE
assert "Failed: 2" in result["log"]
# Systemic error detection tests
# ---------------------------------------------------------------------------


class TestIsSystemicError:
def test_enospc(self):
assert _is_systemic_error(OSError(errno.ENOSPC, "No space left on device"))

def test_estale(self):
assert _is_systemic_error(OSError(errno.ESTALE, "Stale file handle"))

def test_erofs(self):
assert _is_systemic_error(OSError(errno.EROFS, "Read-only file system"))

def test_eio(self):
assert _is_systemic_error(OSError(errno.EIO, "Input/output error"))

def test_disk_space_dicom_error(self):
assert _is_systemic_error(DicomError("Out of disk space while trying to save 'foo.dcm'."))

def test_regular_oserror(self):
assert not _is_systemic_error(OSError(errno.ENOENT, "No such file"))

def test_regular_exception(self):
assert not _is_systemic_error(ValueError("bad value"))

def test_regular_dicom_error(self):
assert not _is_systemic_error(DicomError("Something else went wrong"))


@pytest.mark.django_db
def test_process_stops_on_systemic_error(mocker: MockerFixture, mass_transfer_env):
"""When export raises ENOSPC, the task stops immediately and the destination is suspended."""
env = mass_transfer_env
series = [
_make_discovered(patient_id="PAT1", series_uid="series-1"),
_make_discovered(patient_id="PAT1", series_uid="series-2"),
]

processor = MassTransferTaskProcessor(env.task)
mocker.patch.object(processor, "_discover_series", return_value=series)
mocker.patch("adit.mass_transfer.processors.DicomOperator")
mocker.patch.object(
processor,
"_export_series",
side_effect=DicomError("Out of disk space while trying to save 'test.dcm'."),
)
mocker.patch("adit.core.utils.mail.send_mail_to_admins")

result = processor.process()

assert result["status"] == MassTransferTask.Status.FAILURE
assert "suspended" in result["message"].lower() or "suspended" in result["log"].lower()
Comment on lines +2499 to +2520

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

Correct the docstring and assert that the alert is sent.

Two gaps in this test:

  1. The docstring says "When export raises ENOSPC". The side effect raises DicomError("Out of disk space while trying to save 'test.dcm'."), which exercises the message branch of _is_systemic_error, not the errno branch. No integration test drives a real OSError(errno.ENOSPC, ...) through process(). Fix the docstring, and add a case that raises OSError(errno.ENOSPC, "No space left on device") so the errno branch is covered end to end.

  2. Line 2515 patches send_mail_to_admins but never asserts on the mock. The PR states that a systemic error sends an alert to administrators. That requirement is currently untested. Bind the patch and assert the call.

The patch target also depends on the function-local import at processors.py line 449. If that import moves to module scope, this patch stops intercepting and the test still passes. Asserting the call makes that regression visible.

🧪 Proposed fix
 `@pytest.mark.django_db`
 def test_process_stops_on_systemic_error(mocker: MockerFixture, mass_transfer_env):
-    """When export raises ENOSPC, the task stops immediately and the destination is suspended."""
+    """A disk-space DicomError stops the task immediately and suspends the destination."""
     env = mass_transfer_env
     series = [
         _make_discovered(patient_id="PAT1", series_uid="series-1"),
         _make_discovered(patient_id="PAT1", series_uid="series-2"),
     ]
 
     processor = MassTransferTaskProcessor(env.task)
     mocker.patch.object(processor, "_discover_series", return_value=series)
     mocker.patch("adit.mass_transfer.processors.DicomOperator")
     mocker.patch.object(
         processor,
         "_export_series",
         side_effect=DicomError("Out of disk space while trying to save 'test.dcm'."),
     )
-    mocker.patch("adit.core.utils.mail.send_mail_to_admins")
+    send_mail_mock = mocker.patch("adit.core.utils.mail.send_mail_to_admins")
 
     result = processor.process()
 
     assert result["status"] == MassTransferTask.Status.FAILURE
     assert "suspended" in result["message"].lower() or "suspended" in result["log"].lower()
+    send_mail_mock.assert_called_once()

Add an errno-branch integration test:

`@pytest.mark.django_db`
def test_process_stops_on_enospc_oserror(mocker: MockerFixture, mass_transfer_env):
    """A raw ENOSPC OSError also suspends the destination and stops the task."""
    env = mass_transfer_env
    processor = MassTransferTaskProcessor(env.task)
    mocker.patch.object(
        processor,
        "_discover_series",
        return_value=[_make_discovered(patient_id="PAT1", series_uid="series-1")],
    )
    mocker.patch("adit.mass_transfer.processors.DicomOperator")
    mocker.patch.object(
        processor,
        "_export_series",
        side_effect=OSError(errno.ENOSPC, "No space left on device"),
    )
    send_mail_mock = mocker.patch("adit.core.utils.mail.send_mail_to_admins")

    result = processor.process()

    assert result["status"] == MassTransferTask.Status.FAILURE
    env.destination.refresh_from_db()
    assert env.destination.suspended is True
    send_mail_mock.assert_called_once()
🤖 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/mass_transfer/tests/test_processor.py` around lines 2499 - 2520, Correct
test_process_stops_on_systemic_error’s docstring to describe the DicomError
message branch, bind the send_mail_to_admins patch and assert it is called once,
and add a separate end-to-end test for OSError(errno.ENOSPC, ...) that verifies
failure, destination suspension, and a single administrator alert.


env.destination.refresh_from_db()
assert env.destination.suspended is True

# Only the first series should have been attempted (second skipped due to early exit)
vols = MassTransferVolume.objects.filter(job=env.job)
error_vols = vols.filter(status=MassTransferVolume.Status.ERROR)
assert error_vols.count() == 1


@pytest.mark.django_db
def test_process_continues_on_regular_error(mocker: MockerFixture, mass_transfer_env):
"""Regular errors (non-systemic) mark the volume as ERROR but continue to the next series."""
env = mass_transfer_env
series = [
_make_discovered(patient_id="PAT1", series_uid="series-1"),
_make_discovered(patient_id="PAT1", series_uid="series-2"),
]

call_count = 0

def export_first_fails(*args, **kwargs):
nonlocal call_count
call_count += 1
if call_count == 1:
raise DicomError("Bad DICOM data")
return (1, "", "")

processor = MassTransferTaskProcessor(env.task)
mocker.patch.object(processor, "_discover_series", return_value=series)
mocker.patch("adit.mass_transfer.processors.DicomOperator")
mocker.patch.object(processor, "_export_series", side_effect=export_first_fails)

result = processor.process()

# Should be WARNING (one succeeded, one failed) — NOT FAILURE
assert result["status"] == MassTransferTask.Status.WARNING

env.destination.refresh_from_db()
assert env.destination.suspended is False

# Both series were attempted
assert call_count == 2


@pytest.mark.django_db
def test_process_skips_when_destination_suspended(mocker: MockerFixture, mass_transfer_env):
"""When the destination folder is suspended, process() returns WARNING immediately."""
env = mass_transfer_env
env.destination.suspended = True
env.destination.save()

processor = MassTransferTaskProcessor(env.task)
discover_mock = mocker.patch.object(processor, "_discover_series")

result = processor.process()

assert result["status"] == MassTransferTask.Status.WARNING
assert "suspended" in result["message"].lower()
discover_mock.assert_not_called()


@pytest.mark.django_db
def test_process_does_not_suspend_on_regular_error(mocker: MockerFixture, mass_transfer_env):
"""Non-systemic errors do NOT suspend the destination."""
env = mass_transfer_env
series = [_make_discovered(patient_id="PAT1", series_uid="series-1")]

processor = MassTransferTaskProcessor(env.task)
mocker.patch.object(processor, "_discover_series", return_value=series)
mocker.patch("adit.mass_transfer.processors.DicomOperator")
mocker.patch.object(processor, "_export_series", side_effect=DicomError("Export failed"))

result = processor.process()

assert result["status"] == MassTransferTask.Status.FAILURE

env.destination.refresh_from_db()
assert env.destination.suspended is False