fix(cancel): wait for the worker to exit instead of forcing it after 500ms - #63
Merged
Merged
Conversation
…500ms Cancelling a job left vspipe and ffmpeg running. Reported after a real cancel: three orphaned processes at ~670% CPU eleven minutes later, still writing to the output file of a job the user had cancelled, while the UI showed "Job cancelled by user". The worker side was already correct -- on cancel it calls PipelineExecutor::terminate() and has a Drop impl doing the same. The bug was that it never got the chance. WorkerManager.cancel() sent SIGTERM, slept a flat 500ms, then SIGKILLed. The worker's signal handler only sets an atomic flag; the teardown happens the next time the progress loop comes round, and that loop sleeps on progress_interval -- which is also 500ms. So the grace period was exactly the poll interval and the worker essentially never reached the check in time. SIGKILL cannot be caught, so terminate() and Drop never ran and the children were reparented to init. The "force kill if still running" guard did not help either: it tested `_process != null`, but `_process` is only nulled by _cleanup(), which runs afterwards -- so the SIGKILL was unconditional. Now cancel() awaits the process's actual exitCode with a 5s timeout and only escalates to SIGKILL if it is genuinely still alive, reporting that in the completion message so a forced kill is not silently indistinguishable from a clean one. Windows keeps taskkill /T, which walks the tree and so cannot orphan. Tests: - cancel_shutdown_grace_test (per-push) pins the coupling that caused this: the grace must be several times the worker's poll interval, and cancel() must wait on exitCode rather than a fixed delay. Both assertions fail against the old code, verified by reverting to it. - integration_cancel_test (heavy) pins the contract the fix depends on: SIGTERM a running job and assert the worker exits inside the grace with no children left. It builds a 60s source, because the committed fixtures are short enough that QTGMC can finish before the cancel lands and satisfy everything for the wrong reason. Worth recording what that second test does NOT do: it cannot reproduce the orphaning. SIGKILLing the worker in this harness still leaves no survivors, because the children's stderr pipes close with it and they die on EPIPE at the next write. The reported incident escaped that only because the job was reading a large file off a NAS and the children sat blocked on I/O for minutes without writing. The per-push guards are therefore the real regression protection.
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.
Cancelling a job left
vspipeandffmpegrunning. Observed after a realcancel: three orphaned processes at ~670% CPU eleven minutes later, still
writing to the output file of a job the user had cancelled — while the UI showed
"Job cancelled by user".
Cause
The worker side was already right: on cancel it calls
PipelineExecutor::terminate(), and has aDropimpl doing the same. It justnever got the chance.
WorkerManager.cancel()sent SIGTERM, slept a flat 500 ms, then SIGKILLed.But the worker's signal handler only sets an atomic flag — the teardown happens
the next time the progress loop comes round, and that loop sleeps on
progress_interval, which is also 500 ms. The grace period was exactly thepoll interval, so the worker essentially never reached the check in time.
SIGKILL can't be caught, so
terminate()andDropnever ran and the childrenwere reparented to init.
The "force kill if still running" guard didn't help either: it tested
_process != null, but_processis only nulled by_cleanup(), which runsafterwards — so the SIGKILL was unconditional.
Fix
Await the process's real
exitCodewith a 5 s timeout, and escalate to SIGKILLonly if it is genuinely still alive — reporting that in the completion message,
so a forced kill isn't silently indistinguishable from a clean one. Windows
keeps
taskkill /T, which walks the tree and so cannot orphan.Tests
cancel_shutdown_grace_test(per-push) pins the coupling that caused this:the grace must be several times the worker's poll interval, and
cancel()mustwait on
exitCoderather than a fixed delay. Both assertions fail againstthe old code — verified by reverting to it, not assumed.
integration_cancel_test(heavy) pins the contract the fix rests on:SIGTERM a running job, assert the worker exits inside the grace with no
children left. It builds a 60 s source, because the committed fixtures are
short enough that QTGMC can finish before the cancel lands and satisfy every
assertion for the wrong reason.
What the second test does not do, stated plainly because an earlier draft of
it claimed otherwise: it cannot reproduce the orphaning. SIGKILLing the worker in
this harness still leaves no survivors — the children's stderr pipes close with
it and they die on EPIPE at the next write. The reported incident escaped that
only because the job was reading a large file off a NAS, so the children sat
blocked on I/O for minutes without writing. The per-push guards are the real
regression protection here.
Related, not fixed here
dispose()callscancel()without awaiting it, so quitting the app mid-job canstill race. Worth a separate look.