fix(queue): guarantee a completion, and make the state machine testable - #67
Merged
Merged
Conversation
Fourth attempt at the cancellation hang, and the first with a test that drives the thing that was actually broken. The hang is structural. Two independent latches gate everything -- `_state` and `_isQueueProcessing` -- and both are retired only by a CompletionResult. If that event does not arrive, `canProcess` stays false, `startQueueProcessing` returns silently at its guard, and the UI spins with Start AND Cancel disabled, because `cancelling` is neither `idle` nor in `canCancel`. There is no way back short of restarting the app. The event genuinely does not always arrive. `cancel()` returned early when `_process` was null and reported nothing, while `cancelProcessing` had already latched `_state = cancelling`. `_process` is null more often than it looks -- notably during `preparingJob`, where the Cancel button is live before the process exists. Fixes, all now enforced rather than assumed: - `_jobInFlight` gives "at least one completion per started job"; only "at most one" was enforced before. cancel() reports even with nothing to signal. - Cancellation is observed, not merely inferred: exit code 130 counts alongside `_cancelRequested`. Depending on the flag alone is how the previous fix became unreachable. - An unattributable completion stands the queue down instead of being dropped. - `_stopProcessing()` is the single stand-down path, and it rescues items stranded in `processing` -- a status that is neither canProcess nor canReprocess, so such an item could never run again. - `cancelProcessing` reconciles to idle if nothing retired the latch, and logs it. A missed event should cost a mislabelled item, not a dead window. The reason three previous fixes shipped unverified is that MainViewModel constructed its own WorkerManager, so the queue state machine could not be driven from a test at all. It now takes a JobRunner (defaulting to WorkerManager), and main_viewmodel_lifecycle_test drives the real viewmodel against a fake: cancel then start again, cancel with no completion emitted, a stale completion, a stranded item, a failure mid-queue. Two of the eight fail against the previous code -- the two that describe the shipped bug. Also retires the positional source-scanning assertions added earlier. They anchored on "the first _cleanup()", which the new early-return branch displaced, and produced false failures with the behaviour perfectly correct. Twice now those tests have cost time without catching anything; behaviour covers them.
Adds a VAPOURBOX_WORKER override to ToolLocator._resolveWorker, mirroring the existing VAPOURBOX_DEPS_DIR one. Under `flutter test` the resolved executable is the test runner, so neither the production nor the dev lookup could find the worker binary -- which left WorkerManager, and with it the whole cancel/restart flow, with no way to be tested against a real worker at all. Same structural gap as the viewmodel had, one layer down. Two heavy tests with that seam, both currently passing: - progress still flows after a cancel and a restart - a restart that lands while the cancel is still in flight still reports, which models what the UI actually does (progress_panel.dart:440 does not await cancelProcessing, so a user can restart mid-cancel) Neither reproduces the reported symptom. Recording that as evidence rather than as a fix: the viewmodel relays progress correctly against a fake runner, and WorkerManager delivers it across both restart paths against the real worker, so the fault is somewhere neither layer models. They stand as regression cover for the parts now known good.
The actual cause of "the UI sits on processing and never updates". Four fixes
were made in the app for this symptom; the app was innocent throughout, because
it never received a progress event -- none was ever sent.
The worker polls ${TMPDIR}/vb_progress_${job.id}, and job.id is the queue item's
id, so it is identical every time that item is re-run. ffmpeg writes
`progress=end` as it terminates, so a cancelled run leaves one behind. The next
run's loop polls before its own ffmpeg has opened and truncated the file, reads
the previous run's tail, concludes the encode is finished, breaks out of the
progress loop on its first iteration, and then blocks in decoder.wait() forever
while the pipeline encodes at full speed behind it.
Confirmed by sampling the stuck worker: __wait4 under execute, 0% CPU, while
vspipe sat at 440% and both ffmpegs ran.
Fixed twice over: the file is deleted before the pipeline starts, and
progress_end_is_ours() refuses a `progress=end` seen before this run has reported
a frame.
On the tests: the heavy integration tests seed a stale file and assert the job
still completes and still reports progress, but they pass against the BROKEN
worker too -- locally ffmpeg truncates the file almost instantly, so the race
window barely exists. It is wide over a network share, which is where this
surfaced. They are smoke cover, not the regression guard.
The guard is test_94, on the extracted decision, which fails against the previous
behaviour. Same lesson as completionFor: when the bug is a decision, extract the
decision and test it, rather than hoping an end-to-end test happens to hit the
timing.
Also removes the temporary lifecycle tracing added to find this.
Third CI break from the same cause. git checks worker_manager.dart out CRLF on
Windows, so indexOf('\n }\n') returned -1 and substring threw a RangeError.
The sibling file was normalised two commits ago; this one was missed.
Adds a note at the top of the file, because the failure mode is opaque -- it has
presented once as a RangeError and twice as a nonsensical 'expected a value less
than N' -- and because the right answer is to prefer behavioural assertions over
reading source at all. These remain only because what they guard is a code shape
rather than an observable.
StuartCameronCode
added a commit
that referenced
this pull request
Aug 9, 2026
Artifacts on the draft predate the stale-progress-file fix (#67), which is the one that actually cured the hang. Build number 31 -> 32.
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.
Fourth attempt at the cancellation hang, and the first with a test that drives
the thing that was actually broken. Background:
docs/JOB_LIFECYCLE.md.The hang is structural
Two independent latches gate everything —
_stateand_isQueueProcessing— andboth are retired only by a
CompletionResult. If that event does not arrive:canProcessstays false andstartQueueProcessingreturns silently at itsguard, so Start does nothing;
cancellingis neitheridlenor incanCancel, so Cancel is disabled too;_state, so it turns forever.No error, no log, no way back short of restarting the app.
The event genuinely does not always arrive
cancel()returned early when_processwas null and reported nothing, whilecancelProcessinghad already latched_state = cancelling._processis nullmore often than it looks — notably during
preparingJob, where the Cancel buttonis live before the process exists.
Fixes — enforced, not assumed
_jobInFlightgives "at least one completion per started job"; only "atmost one" was enforced before.
cancel()now reports even with nothing to signal.130countsalongside
_cancelRequested. Depending on the flag alone is exactly how theprevious fix became unreachable.
_stopProcessing()is the single stand-down path, and it rescues itemsstranded in
processing— a status that is neithercanProcessnorcanReprocess, so such an item could never be run again.cancelProcessingreconciles toidleif nothing retired the latch, andlogs it. A missed event should cost a mislabelled item, not a dead window.
Why the previous three fixes shipped unverified
MainViewModelconstructed its ownWorkerManager, so the queue state machinecould not be driven from a test at all. It now takes a
JobRunner(defaulting to
WorkerManager), andmain_viewmodel_lifecycle_testdrives thereal viewmodel against a fake:
processingTwo of the eight fail against the previous code — the two that describe the
shipped bug. Verified by reverting the behaviour and re-running, not assumed.
Also retires the positional source-scanning assertions added earlier: they
anchored on "the first
_cleanup()", which the new early-return branchdisplaced, and produced false failures with the behaviour perfectly correct.
Twice now those have cost time without catching anything.
Still open, deliberately
Cancel stops the whole queue (your call, confirmed during planning). And the
manual check still matters — this fixes the app's state machine, but only you can
confirm the real flow on a real file.