Skip to content

feat(mass_transfer): resumable automatic retries (durable volume progress) - #374

Draft
samuelvkwong wants to merge 11 commits into
mainfrom
feat/mass-transfer-resumable-retries
Draft

feat(mass_transfer): resumable automatic retries (durable volume progress)#374
samuelvkwong wants to merge 11 commits into
mainfrom
feat/mass-transfer-resumable-retries

Conversation

@samuelvkwong

Copy link
Copy Markdown
Collaborator

Summary

Makes automatic Procrastinate retries of mass transfer tasks resume from persisted per-series progress instead of wiping the destination partition and re-fetching everything. One dead series no longer costs the whole partition: it is retried alone, and the task ends WARNING with only the truly dead volumes marked ERROR.

Supersedes the final-attempt-continue stop-gap on the base branch: the _is_final_attempt() special case (and its cancel→resume edge case) is deleted; every attempt now behaves identically.

How it works

  • Wipe only on a fresh queue cycle (DicomTask.attempts <= 1): fresh jobs and UI Retry/Restart (which reset attempts to 0) keep clean-slate semantics. Automatic retries (attempts ≥ 2) skip the wipe and discovery and reload the persisted MassTransferVolume rows.
  • New MassTransferVolume.retriable flag: a per-volume RetriableDicomError marks the volume ERROR + retriable=True and the loop continues. On resume, only retriable/pending volumes are re-transferred; EXPORTED/CONVERTED/SKIPPED/permanent-ERROR volumes (rows and files) are untouched.
  • One aggregated raise at the end: if retriable failures remain and attempts remain, the task raises a single RetriableDicomError so Procrastinate retries with backoff. On the final attempt it returns the whole-partition summary instead.
  • is_final_attempt comes from the runner: _run_dicom_task computes it from Procrastinate's per-job counter (the arithmetic already used for the PENDING/FAILURE decision) and injects it into the processor subprocess — no more guessing from the cumulative DicomTask.attempts.
  • Per-series folder cleanup before re-export replaces the partition-wide rmtree as the defense against partially-written series.
  • Stable pseudonymization across retries: random-mode jobs get a per-cycle MassTransferTask.anonymizer_seed (generated on the fresh cycle, reused by retries) so a resumed attempt produces the same pseudonymized UIDs/date shifts as the volumes already transferred.
  • Atomic volume creation: discovery's bulk_create is wrapped in a transaction so a worker kill mid-create rolls back to zero rows and the next attempt re-discovers.
  • Runner save scoped: _run_dicom_task's final save writes only the fields it owns (status, message, log, end) so it cannot clobber fields the processor subprocess persisted mid-run.

Bonus: cancel → resume now actually resumes the partition instead of redoing it.

Migrations

Two backward-compatible AddField migrations (0006 MassTransferVolume.retriable, 0007 MassTransferTask.anonymizer_seed).

Deploy note (one-time transitional behavior)

A task that failed retriably under the old code and auto-retries after deploy resumes from rows written by the old code (retriable=False), so its interrupted volume is treated as permanent and the task ends WARNING without retrying that volume. Self-healing via the job Retry button.

Test plan

  • Full non-acceptance suite at HEAD: 861 passed, 3 xfailed, 0 failures; uv run cli lint (ruff + pyright + djlint) clean.
  • New DB-integration tests: resume without wipe/re-discovery (rows + files survive, only retriable volumes re-transferred, whole-partition summary), fresh-cycle wipe semantics, retriable-flag persistence, anonymizer-seed generation/reuse, runner save-clobber regression, discovery-error propagation.
  • Design docs: docs/superpowers/specs/2026-07-16-mass-transfer-resumable-retries-design.md (spec) and docs/superpowers/plans/2026-07-16-mass-transfer-resumable-retries.md (plan).

Non-blocking follow-ups (from review)

  • Consolidate the pebble-stub test helper in adit/core/tests/test_tasks.py (three inline copies now).
  • Consider a server-destination resume DB test.
  • Aggregated retry message says "N of {whole partition}" — could say "N of M pending".

🤖 Generated with Claude Code

samuelvkwong and others added 11 commits June 15, 2026 13:13
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…empt

A mass transfer task processes its partition's series sequentially. When a
single series exhausted the stamina network retries, the resulting
RetriableDicomError was re-raised, aborting the whole task. Each Procrastinate
retry re-fetched the entire partition and hit the same dead series again, so the
task ended FAILURE even when only 1-2 of its volumes were truly unrecoverable
(e.g. series on archived/offline PACS storage).

On the final attempt only, mark the dead volume ERROR and continue with the
remaining volumes, so the partition completes as WARNING instead of FAILURE.
Non-final attempts are unchanged: a transient PACS outage can still recover
during the task-level retry waits.

Implements the approved design in
docs/superpowers/specs/2026-06-12-mass-transfer-final-attempt-continue-design.md

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Supersedes the final-attempt-continue stop-gap design: automatic
Procrastinate retries resume from persisted MassTransferVolume rows
instead of wiping and re-fetching the whole partition.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…eries folders

- Delete obsolete _is_final_attempt() helper method
- Update _transfer_single_series to never raise RetriableDicomError: instead,
  mark the volume as ERROR with retriable=True, allowing process() to schedule
  a task retry after the whole partition was attempted
- Add per-series folder cleanup before export: if output_path exists from a
  previous partial attempt, delete it (folder exports are not atomic)
- Add "retriable" to volume.save() update_fields

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c69439b3-d8e2-4c9a-a8c8-5ed0751d752a

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/mass-transfer-resumable-retries

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@gemini-code-assist gemini-code-assist Bot left a comment

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.

Code Review

This pull request implements resumable automatic retries for mass transfer tasks, allowing them to resume from persisted volume progress instead of restarting the entire partition. It introduces a retriable field on MassTransferVolume and an anonymizer_seed on MassTransferTask, passes is_final_attempt from the runner to the processor, and updates the transfer loop to continue past retriable errors, raising a single aggregated error at the end. Feedback on the changes points out a potential network connection leak where the source DicomOperator is not closed if all volumes are already completed in a resumed run, suggesting it be closed in the finally block of process().

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

self.mass_task.save(update_fields=["anonymizer_seed"])
pseudonymizer = Pseudonymizer(seed=self.mass_task.anonymizer_seed)

operator = DicomOperator(source_node.dicomserver, persistent=True)

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.

medium

The operator (source DicomOperator) is initialized with persistent=True. In a resumed run where all volumes are already completed (i.e., volumes is not empty, but pending is empty), the else block is skipped, and the loop inside _transfer_grouped_series does not execute. As a result, operator.close() is never called, leading to a potential network connection/association leak on the PACS server.

To prevent this, consider initializing operator = None at the start of process(), and ensuring it is closed in the finally block of process():

        finally:
            if operator:
                operator.close()
            if dest_operator:
                dest_operator.close()

@samuelvkwong
samuelvkwong marked this pull request as draft July 21, 2026 11:33
Base automatically changed from fix/mass-transfer-final-attempt-continue to main July 31, 2026 10:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant