Fix Windows preview process leak, and make the nightly heavy tests able to fail honestly - #70
Merged
Merged
Conversation
ProcessTree.killTree degrades to a leader-only kill on Windows, because there are no process groups there — the tree has to be walked with taskkill /T instead. Its doc comment said "callers on Windows use that directly", and worker_manager did, but preview_generator never had that branch. What it spawns is the worker in --preview mode, which spawns vspipe and ffmpeg itself, so on Windows every preview cancellation left both children running. Seeking makes that worse than it sounds: each scrub of the scrubber cancels an in-flight preview, so the strays accumulate, at full CPU, on work nobody is waiting for. That is precisely the failure this class was written to prevent. Move taskkill into killTree so no caller can forget it, and drop worker_manager's own copy. The signal argument is documented as advisory there: taskkill /F is unconditionally forceful, so a sigterm request kills as hard as a sigkill one. Nothing gentler reaches a child tree on Windows, and leaving the tree alive is worse. killTree becomes async because taskkill is a process spawn. Call sites await it; cancelPreviewGeneration still does not await the *reaping*, which is what kept seeking responsive. Also removes a duplicated _livePreviews.remove() left by an earlier edit. Harmless — Set.remove is idempotent — but it read like a bad merge. Unverified by me: I have no Windows machine, so this is a defect I could demonstrate by reading and not by running. The test changes that follow make the platform's teardown assertable at all.
Nightly has failed every night since 2026-08-08, on all four platforms, for two reasons that both amount to a test never reaching its assertion. integration_cancel_test counted child processes by matching the executable path against WorkerHarness.depsDir. On Linux the worker does not search upward from the executable at all (dependency_locator.rs restricts that to non-Linux debug builds), so it resolves deps through ~/.local/share/VapourBox/deps, which CI symlinks to the checkout. ps reported the symlinked path, the test held the workspace one, they shared no prefix, and the child count came back zero — failing with "the pipeline never started" when it had started fine. Match against every name the deps directory can legitimately be reached by. On Windows the helpers were ps, pgrep and ps -o pgid=, with no platform guard: unrunnable there, so the orphan checks passed vacuously on the one platform whose teardown had no other coverage — which is how the preview leak fixed in the previous commit survived. List processes with Get-CimInstance Win32_Process instead, and skip only the two tests whose subject genuinely does not exist on Windows (SIGTERM semantics, process group leadership), saying so in the skip reason. The preview test also claimed to do "exactly what cancelPreviewGeneration() does" while calling proc.kill() rather than killTree. On Unix a leader-only kill happens to work — the children take EPIPE — so it passed and the gap stayed invisible. It now calls killTree, which makes it the actual guard for the Windows behaviour. integration_worker_manager_restart_test needs VAPOURBOX_WORKER in the environment: ToolLocator resolves relative to the running executable, which under `flutter test` is the test runner. nightly.yml did not set it, so both tests died with "Worker executable not found". Its setUpAll comment claimed the setup pointed ToolLocator at the worker, which a Dart process cannot do — it cannot mutate its own environment. Export the variable in the heavy step and correct the comment. Verified locally on macOS arm64: both restart tests pass with the variable set (they failed before), and each cancel test passes when run alone. The file is timing-sensitive on a loaded machine and I could not get all four green in one local run — the failure moves between the SIGTERM and preview preconditions — so the four-platform CI run is the real check, and the Windows path I cannot exercise here at all.
The first attempt fixed the Linux path mismatch by matching more paths. The nightly run on this branch showed why that was the wrong shape: with detection working, Linux then failed the real assertion with three "orphans" that were never ours. `flutter test` runs test *files* concurrently and several of them drive the worker, so "any live process whose executable sits under deps/" counts another file's pipeline as this test's leftovers — a false failure that depends on how the runner happened to interleave that night. Ask the worker's own process group instead (pgrep -g, which is exactly what the setpgid in worker/src/main.rs is for), and on Windows walk ParentProcessId down from the worker. Ancestry cannot collide with another test, needs no before/after snapshot of the process table, and drops the guesswork about which name the deps directory was reached by — so the Linux symlink problem disappears rather than being worked around. Also polls at 100ms rather than 500ms while waiting for the pipeline to come up. A single-frame preview on a fast machine can start and finish inside one half-second sample, and missing it reported as "the pipeline never started" — which is what macos-arm64 failed on last night, and what made this file flaky locally. Local result on macOS arm64: all four tests pass, three runs in a row, in 11s rather than the 48s the snapshot approach took. Previously the file could not be got green in one run at all.
The SIGTERM orphan check fails on macos-arm64 in CI with three pids and passes locally every time, so the pid list is all there is to go on — and it cannot distinguish the bug under test (children still encoding at full CPU) from a zombie awaiting reaping, which is harmless and transient. Print state and command for each survivor, captured before they are killed so the message does not describe processes that no longer exist. Note this check now sees processes the old path matching could not: on macOS `vspipe` is a wrapper script, so its process is `/bin/sh …/vspipe`, which never matched a deps-path prefix but is in the worker's group. Some of what ancestry reports may therefore have been surviving unnoticed all along rather than being newly broken.
…pipeline ctrlc installs a handler for SIGINT only unless the `termination` feature is enabled. It was not, so `ctrlc::set_handler` — sitting under a comment reading "Handle SIGTERM/SIGINT for graceful cancellation" — never covered SIGTERM at all. A SIGTERM, which is exactly what the app sends to cancel a job, killed the worker by default disposition: the handler never ran, the cancellation flag was never set, PipelineExecutor::terminate() never ran, and the decoder, vspipe and encoder were left running mid-encode. Measured directly, spawning the worker and signalling it by hand: before: worker exit code 143 (128+15, killed by the default disposition) after: worker exit code 130 (cancelled, children reaped) Why it stayed hidden. The children usually die on their own moments later — their pipes close with the worker and they take EPIPE on the next write — so the leak only shows when a child is not writing: blocked on slow input, or busy inside a long computation. That is the NAS report this test file's header describes, and it is why the orphan assertion failed intermittently rather than every time (macos-arm64 in nightly, and 2 runs in 3 locally). It also does not affect a normal cancel from the app, which signals the whole process group, so vspipe and ffmpeg are hit directly whatever the worker does — that is what ProcessTree was built for and it masked this completely. The integration_cancel_test SIGTERM case is the regression guard: it signals the worker alone, which is the path that depends on the worker's own teardown. Five consecutive local runs of the file now pass; before this it failed roughly two runs in three.
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
1 participant
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.
Nightly has failed every night since 2026-08-08 on all four platforms. Investigating it turned up one real product bug behind two tests that were never reaching their assertions.
The product bug (Windows only)
ProcessTree.killTreedegrades to a leader-only kill on Windows — no process groups there, the tree has to be walked withtaskkill /T. Its comment said "callers on Windows use that directly", andworker_managerdid:preview_generatornever got that branch. All four of its teardown sites callkillTreealone, and what it spawns is the worker in--previewmode, which spawnsvspipeandffmpegitself. So on Windows every preview cancellation stranded both children — and since each scrub of the scrubber cancels an in-flight preview, they accumulate at full CPU. Exactly the failureProcessTree's header describes and exists to prevent.taskkillnow lives insidekillTree, so no caller can forget it. It became async accordingly.Why no test caught it
integration_cancel_test's process-inspection helpers areps,pgrepandps -o pgid=, with no platform guard. On Windows they cannot run, so the orphan assertions passed vacuously — on the one platform whose teardown had no other coverage. The preview test also claimed to do "exactly whatcancelPreviewGeneration()does" while callingproc.kill()instead ofkillTree; on Unix a leader-only kill happens to work (children takeEPIPE), so it passed and the gap stayed invisible.Now:
Get-CimInstance Win32_Processon Windows, and only the two genuinely POSIX-subject tests are skipped (SIGTERM semantics, process-group leadership).Why nightly was red
psoutput against$GITHUB_WORKSPACE/deps/..., but the worker resolves deps through~/.local/share/VapourBox/deps(dependency_locator.rsdisables the upward search on Linux), which CI symlinks. No shared prefix → 0 children → "the pipeline never started" on a pipeline that started fine.integration_worker_manager_restart_testneedsVAPOURBOX_WORKER, which nightly never set, so it failed with "Worker executable not found". Its setUpAll comment claimed the setup handled it — impossible, a Dart process cannot mutate its own environment.Verification
Locally on macOS arm64: both restart tests now pass (they failed before), and each cancel test passes run alone. The cancel file is timing-sensitive on a loaded machine and I could not get all four green in a single local run — the failure moves between the SIGTERM and preview preconditions, and each passes in isolation, so it is pre-existing ordering sensitivity rather than something here. The four-platform nightly run on this branch is the real check, and the Windows path cannot be exercised locally at all.
Does not affect the 0.9.13 release: Windows-only bug, predating it, in a code path unrelated to that release's changes.