fix(separate): tear the worker down on any failure, and honour cancel before the CPU retry - #534
Merged
Merged
Conversation
… before the CPU retry The persistent worker teardown sat after the finally block, not inside it. An exception out of the read loop -- proc.stderr.read(1) raising OSError when the API thread's terminate() races the read, or _set() raising -- propagated without it, so _worker still held the process and the next job's _get_worker() saw a matching device and a live poll() and reused a worker whose CUDA state followed an exception. ml-pipeline.md is explicit that any non-success must tear it down. A second path missed it entirely: the pipe check raises before the try, so a worker that came back without stdin/stderr stayed cached and would be handed to every subsequent job. Found while writing the test for the first one. separate() also had no cancel check between its two attempts. The rmtree of a multi-GB partial result takes seconds and nothing is registered for cancel during it, so a cancel landing there was invisible and the full CPU pass ran to completion -- 10+ minutes -- before JobCancelled was finally raised. The UI showed "Cancelling" throughout. _kill_worker now reaps after kill(). Without communicate() a worker wedged in an uninterruptible CUDA call becomes a zombie whose pipes close only when the Popen refcount happens to drop; vocal_split.py already pairs the two. An entry-point cancel check was tried and removed. It broke test_cancel_kills_worker_next_job_spawns_fresh, which verifies that a cancelled job tears the worker down so the next one spawns fresh -- a check that never spawns has no worker to tear down, and that test encodes the rule this change is meant to protect. The queue worker already gates dispatch on cancellation, so the fallback check covers the gap that was actually reported. Verified: reverting each half fails its test. Refs #514
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #514. Independent; branches off
0.16.1.1. Worker teardown sat outside the
finallyAn exception out of the read loop --
proc.stderr.read(1)raisingOSErrorwhen the API thread'sterminate()races the read, or_set()raising -- propagated without it._workerstill held the process, so the next job's_get_worker()saw a matching device and a livepoll()and reused a worker whose CUDA state followed an exception..claude/rules/ml-pipeline.md: "torn down after any non-success (cancellation or failure) -- post-exception CUDA state can't be trusted. Both halves of this rule matter; don't relax either side."2. A second path missed teardown entirely
The pipe check raises before the
try:so a worker that came back without pipes stayed cached and would be handed to every subsequent job. Found while writing the test for #1 -- it failed for this reason rather than the one I was targeting.
3. Cancel was dropped before the CPU retry
separate()had no cancel check between attempts. Thermtreeof a multi-GB partial takes seconds with nothing registered for cancel, so a cancel landing there was invisible and the entire CPU pass ran to completion -- 10+ minutes -- beforeJobCancelledwas raised at the end. The UI showed "Cancelling" throughout.4.
_kill_workerkilled without reapingkill()only sends the signal. Withoutcommunicate()a worker wedged in an uninterruptible CUDA call becomes a zombie whose pipes close only when thePopenrefcount happens to drop.vocal_split.pyalready pairs the two.A check I tried and removed
I initially added a cancel check at the top of
_run_demucs. It broketest_cancel_kills_worker_next_job_spawns_fresh, which verifies that a cancelled job tears the worker down so the next spawns fresh -- a check that never spawns has no worker to tear down, so the test's premise collapses. That test encodes exactly the rule this PR is protecting, so the check went rather than the test.The queue worker already gates dispatch on cancellation, and the fallback check covers the window that was actually reported, so nothing is lost.
Verification
New
tests/test_separate_worker_lifecycle.py, 3 tests. Confirmed not vacuous -- reverting the teardown placement and the fallback check fails them.tests/test_separate_fallback.py(13 tests) still passes unchanged -- worth noting, since that is the suite that pushed back on the entry-point check.