If a worker gets lost / stalled etc, eventually the heartbeat stops and we reclaim the job.
Sometimes, this can cause problem - the old worker carries on, and the new worker re-uses the same annotation path and it gets corrupted.
So 2 things:
- Need to get old worker to die
- Need to ensure that path is unique each time
Old worker dies
The cooperative check is right, and we're most of the way there. The lease heartbeat already detects loss (_renew, annotate_variants.py:84):
updated = AnnotationRun.objects.filter(pk=..., task_id=self.task_id).update(lease_expires=...)
if not updated:
logging.warning("... no longer held by task %s - stopping", self.task_id)
self._stop.set() # <-- today this only stops the heartbeat thread
When the run gets reclaimed, reclaim sets task_id=None and the new attempt claims it, so this update matches 0 rows — the losing worker already knows it lost. It just stops heartbeating and lets VEP run to completion anyway. The fix is to escalate that same branch into aborting the run: terminate the VEP subprocess so communicate() returns non-zero, which trips the existing if return_code != 0: raise (annotate_variants.py:329) and fails the losing attempt cleanly without writing results.
The one implementation wrinkle: VEP runs via blocking execute_cmd → Popen.communicate() on the task's main thread, while the heartbeat is a separate daemon thread. So the heartbeat needs a handle to the Popen (or a shared abort flag +
process handle) to .kill() it from the "lost" branch. Localized, but it does mean threading the process handle through execute_cmd.
Unique path
Use task_id. It already exists on the run, it's already the execution/ownership lock, and the heartbeat already keys on it. Two concurrent executions always have distinct task_ids by construction — that's exactly the uniqueness invariant we need, and it means the output file is tagged with the very token that decides who owns the run. Fully traceable back to a task in the logs.
The correctness win is the real point: with per-task paths, each file is written by exactly one VEP, so both are individually valid. Whichever attempt's path the DB's vcf_annotated_filename ends up pointing at, import reads a complete
file - so even the DB-write race between two late finishers can't corrupt anything (worst case you import a valid-but-orphaned annotation for that range). That's why the unique path alone fully fixes the bug; the self-abort is the
efficiency follow-on (don't burn 25 min of VEP, don't double-write DB state).
One caveat to design around: _get_dump_path_stem (models.py:1259) is the shared stem for both the dump and the annotated file, and it's the "self-describing" name the #1568 external-dump flow copies to other machines — where there may be no live task_id. So don't bake task_id unconditionally into that stem. Add it as an optional token the local pipeline passes (from annotate_variants.request.id), leaving external dumps stable. That single change point makes both the dump and the annotated path per-task for local runs (the dump races too, just less visibly).
Suggested sequencing
- Unique per-task paths first — small, self-contained, and a complete correctness fix on its own. I'd audit every reader of get_dump_filename / vcf_annotated_filename first so nothing assumes the old fixed name (import reads the stored
DB field, so it's fine, but external-dump matching and any cleanup sweep need a look).
- Heartbeat self-abort second — stops wasted compute and duplicate DB writes; more invasive because of the Popen-handle plumbing.
If a worker gets lost / stalled etc, eventually the heartbeat stops and we reclaim the job.
Sometimes, this can cause problem - the old worker carries on, and the new worker re-uses the same annotation path and it gets corrupted.
So 2 things:
Old worker dies
The cooperative check is right, and we're most of the way there. The lease heartbeat already detects loss (_renew, annotate_variants.py:84):
When the run gets reclaimed, reclaim sets task_id=None and the new attempt claims it, so this update matches 0 rows — the losing worker already knows it lost. It just stops heartbeating and lets VEP run to completion anyway. The fix is to escalate that same branch into aborting the run: terminate the VEP subprocess so communicate() returns non-zero, which trips the existing if return_code != 0: raise (annotate_variants.py:329) and fails the losing attempt cleanly without writing results.
The one implementation wrinkle: VEP runs via blocking execute_cmd → Popen.communicate() on the task's main thread, while the heartbeat is a separate daemon thread. So the heartbeat needs a handle to the Popen (or a shared abort flag +
process handle) to .kill() it from the "lost" branch. Localized, but it does mean threading the process handle through execute_cmd.
Unique path
Use task_id. It already exists on the run, it's already the execution/ownership lock, and the heartbeat already keys on it. Two concurrent executions always have distinct task_ids by construction — that's exactly the uniqueness invariant we need, and it means the output file is tagged with the very token that decides who owns the run. Fully traceable back to a task in the logs.
The correctness win is the real point: with per-task paths, each file is written by exactly one VEP, so both are individually valid. Whichever attempt's path the DB's vcf_annotated_filename ends up pointing at, import reads a complete
file - so even the DB-write race between two late finishers can't corrupt anything (worst case you import a valid-but-orphaned annotation for that range). That's why the unique path alone fully fixes the bug; the self-abort is the
efficiency follow-on (don't burn 25 min of VEP, don't double-write DB state).
One caveat to design around: _get_dump_path_stem (models.py:1259) is the shared stem for both the dump and the annotated file, and it's the "self-describing" name the #1568 external-dump flow copies to other machines — where there may be no live task_id. So don't bake task_id unconditionally into that stem. Add it as an optional token the local pipeline passes (from annotate_variants.request.id), leaving external dumps stable. That single change point makes both the dump and the annotated path per-task for local runs (the dump races too, just less visibly).
Suggested sequencing
DB field, so it's fine, but external-dump matching and any cleanup sweep need a look).