Reclaim mishandles a stalled run's files: deletes a live dump, and reads persisted fields instead of the derivable stem

[davmlaw]

🤖 Written by Claude

Split out of #1658 (found while implementing the reclaimed-run ownership fixes, not caused by them). Rewritten after review — the original framing led with the deletion race, which turns out to be the benign half. The real defect is below it.

_reset_run_for_redispatch (annotation/tasks/annotation_scheduler_task.py:432-470) mishandles a stalled run's files in two ways. Reclaim assumes the worker is dead; often it isn't — it's stalled (slow IO, paused VM, GC pause) and still running, which is the whole reason the lease can expire under a live process.

1. It deletes a dump the stalled worker's VEP may still be reading — benign, document it

elif annotation_run.status != AnnotationStatus.CREATED:
    # Worker got part-way before dying (pre-VEP) - scrub partial output (a CREATED run has none).
    annotation_run.delete_related_objects()
    for filename in (annotation_run.vcf_dump_filename, annotation_run.vcf_annotated_filename):
        if filename and os.path.exists(filename):
            os.remove(filename)

That branch is reached exactly when the run is mid-dump or mid-VEP, so the file removed is the live input of a still-running VEP. Per-task dump paths (#1658) don't help: reclaim deletes whatever the DB row names, and at reclaim time the row still names the zombie's per-task dump. The damage is confined to the zombie.

This is fine as-is, and should stay. In practice it acts as a crude abort, which is roughly what we want, and two things make it safe:

• The zombie fails loudly. annotate_variants.py:471-479 raises RuntimeError on any non-zero VEP return, and execute_cmd (library/utils/os_utils.py:20-47) is a plain Popen with returncode propagated verbatim — VEP is not run through the shell=True branch, so nothing masks the exit code. Since unlink doesn't truncate, there's no read-half-a-file-and-exit-zero path: VEP either already holds the fd (reads the inode to completion, unaffected) or opens by name afterwards and gets ENOENT.
• The failure is already classified correctly. annotate_variants.py:264-274 reports any exception on an attempt that no longer owns its run — the heartbeat's own kill, and equally a missing-input error from this unlink — at warning level as AnnotationRunReclaimedError, not as an error-level pipeline failure. The "wasted debugging on a confusing VEP error" this issue was originally filed about was already paid off by Handle workers reclaiming annotation runs #1658.

The two alternatives considered are both worse. Deferring the delete via rename-then-delete is close to a no-op on Linux — plain os.remove already gives a live reader its fd and a reopener an immediate ENOENT; deferring the unlink just reintroduces a leak with extra machinery. Leaving deletion to the losing attempt's _cleanup_reclaimed_run_files fails when a worker is genuinely dead and never runs it — reclaim is the only collector for that case, which is the point of §2.

Action: keep the unlink, and note in the docstring that it doubles as a crude abort of a zombie's VEP, safe for the reasons above.

Untested assumption worth confirming: that VEP itself exits non-zero on an unopenable -i. Surprising otherwise, but it's load-bearing.

2. It reads persisted filename fields instead of the derivable dump stem — the actual bug

Everything reclaim needs is derivable from the dump stem the row already carries. dump_variants persists vcf_dump_filename at annotate_variants.py:377, before VEP starts, and from that stem, deterministically:

• annotated VCF — _get_annotated_filename (annotate_variants.py:390)
• conservation sidecar — conservation_sidecar_filename(annotated)
• AnnotSV TSV — {splitext(basename(dump))[0]}.annotated.tsv inside annotsv_{run.pk} (annotsv_annotation.py:61-62)

vcf_annotated_filename and annotsv_tsv_filename by contrast are only written to the DB at the single annotation_run.save() on annotate_variants.py:529 — after VEP, AnnotSV and the conservation sidecar have all finished. Until then the row says vcf_annotated_filename = None however much work has completed.

Completed VEP work gets discarded. Reclaim picks its branch on that persisted field, so a run reclaimed before :529 looks pre-VEP — including one that finished VEP and is minutes deep in AnnotSV — and takes the scrub-everything branch instead of #1646's resume-upload branch. That's precisely the "minutes of VEP per stalled run" resume-upload exists to save. The heartbeat covers AnnotSV (the context manager at annotate_variants.py:252 wraps the whole of dump_and_annotate_variants), so a reclaim there is a genuine stall, not a missed renewal.

Derived files leak when the worker is genuinely dead. The branch removes only the two literal filename fields, then NULLs vcf_dump_filename — discarding the one key that could have named the annotated VCF, sidecar and AnnotSV TSV. A live losing attempt still recovers them, since _cleanup_reclaimed_run_files reads its own in-memory instance; a worker that actually died never runs that, and after reclaim NULLs the stem nothing can name the files again.

Action — derive rather than read, in both places:

1. Decide post-VEP vs pre-VEP with os.path.exists(_get_annotated_filename(run, run.vcf_dump_filename)) rather than the persisted field. No change to save ordering needed.
2. Before NULLing vcf_dump_filename, derive and remove the annotated VCF, conservation sidecar and AnnotSV TSV alongside the dump.

Worth factoring the derivation into one helper taking (run, dump_filename) and returning all four paths, shared with _cleanup_reclaimed_run_files so the two sides can't drift. Reuse run_annotsv's own naming rather than reimplementing the splitext step.

This is the same principle as the AnnotSV-directory note on #720 — keep paths derivable from the run, have each party delete by its own dump stem — applied to reclaim's side of it.

Related: #1658, #1646, #720 (the AnnotSV abortability gap noted there is a separate item, belonging with the runner refactor).

[davmlaw]

Annotation has been running fine