fix(preview): stop leaking vspipe/ffmpeg when seeking or cancelling - #64
Merged
Merged
Conversation
Reported: seeking previews and cancelling jobs leave vspipe and ffmpeg running. Three separate causes, all of which rely on the same accident. Nothing ever killed those children deliberately. They tended to die because their pipes closed with the worker and they took EPIPE at the next write -- but a child blocked reading a slow source (a NAS share, in the report) writes nothing for minutes, never notices, and keeps burning CPU on work nobody wants. That also explains why this never reproduced locally: with fast local I/O the cascade wins every time. 1. Preview mode never installed a signal handler at all -- main() returns at the --preview branch before ctrlc is set up -- so SIGTERM killed the worker outright without unwinding. Drop never ran. And generate_preview holds vspipe and ffmpeg in locals, so PipelineExecutor::terminate() could not have reached them even if it had run. 2. PreviewGenerator tracked one _previewProcess, assigned *after* `await Process.start(...)` returned. A seek arriving inside that window cancelled whatever the field happened to hold, and the next assignment then overwrote the reference to the in-flight worker -- untracked, never killed. Scrubbing cancels a preview on every movement, so those accumulate. This is the "bunch of processes" in the report. 3. cancel() signalled the worker's pid alone, so even a clean shutdown depended on the worker getting far enough to kill its own children. The worker now makes itself a process-group leader (setpgid), so the app can tear down the whole tree with one signal to -pid, and ProcessTree does that with a fallback to pid-only signalling where groups are unavailable -- which is exactly today's behaviour, so this is never worse. PreviewGenerator tracks every live preview in a set, registered at spawn, so nothing can be lost in that window. Cancellation signals immediately and reaps in the background: a seek must not wait out a shutdown grace, or scrubbing feels broken. dispose() uses the waiting variant, since strays outliving the app are worse than a pause. Verified rather than assumed. The worker really does become a group leader (pgrp 3362 -> 3382), Dart really does forward a negative pid to kill(2) (killPid(-pid) returns true), and the group signal kills a child that was SIGSTOPped first -- so it cannot be credited to the EPIPE cascade. Tests: a process-group invariant test that stops the children before signalling, and a rapid-seek test that fires ten overlapping previews and asserts nothing survives. Both would pass vacuously without the SIGSTOP and the burst respectively, which is why they are written that way.
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.
Seeking previews and cancelling jobs leave
vspipeandffmpegrunning. Threeseparate causes, all resting on the same accident.
Nothing ever killed those children deliberately. They tended to die because
their pipes closed with the worker and they took EPIPE at the next write. A child
blocked reading a slow source — a NAS share, in the report — writes nothing for
minutes, never notices, and keeps burning CPU. That also explains why this never
reproduced locally: with fast local I/O the cascade wins every time.
1. Preview mode had no cleanup whatsoever
main()returns at the--previewbranch beforectrlcis installed, soSIGTERM killed the worker outright without unwinding —
Dropnever ran. Andgenerate_previewholdsvspipe/ffmpegin locals, soPipelineExecutor::terminate()could not have reached them even if it had run.2. A seek could lose a preview entirely
PreviewGeneratortracked one_previewProcess, assigned afterawait Process.start(...)returned. A seek arriving inside that window cancelledwhatever the field happened to hold, and the next assignment overwrote the
reference to the in-flight worker — untracked, never killed. Scrubbing cancels a
preview on every movement, so they accumulate. This is the reported symptom.
3. Cancel signalled the pid alone
Even a clean shutdown depended on the worker getting far enough to kill its own
children.
Fix
The worker makes itself a process-group leader (
setpgid), so the app cantear down the whole tree with one signal to
-pid.ProcessTreedoes that, witha fallback to pid-only signalling where groups are unavailable — which is exactly
today's behaviour, so this is never worse.
PreviewGeneratortracks every live preview in a set, registered at spawn, sonothing can be lost in that window. Cancellation signals immediately and reaps in
the background — a seek must not wait out a shutdown grace, or scrubbing feels
broken.
dispose()uses the waiting variant, since strays outliving the app areworse than a pause.
Verified, not assumed
pgrp 3362 → 3382)kill(2)(killPid(-pid)→true)cannot be credited to the EPIPE cascade
Two tests: a process-group invariant test that stops the children before
signalling, and a rapid-seek test firing ten overlapping previews. Both would
pass vacuously without the SIGSTOP and the burst respectively — which is why they
are written that way.