fix(cancel): stop a cancel from stranding the job that replaced it - #65
Merged
Merged
Conversation
Regression from the cancel fix. Cancelling a conversion and starting another one left the progress dialog spinning with no updates: the worker was healthy and the encode was progressing, but nothing was listening to it. cancel() now waits for the worker to genuinely exit, which can take seconds. Cancellation emits a completion, the queue acts on that by starting the next job, and the tail of the *cancelling* call then ran unconditionally -- so its _cleanup() nulled _process and cancelled the stdout/stderr subscriptions belonging to the new job. No progress events could ever arrive after that, and nothing errored, because from the worker's side everything was fine. The exit handler registered in startJob had the same problem on its own schedule, and additionally won the race to report: it is registered on exitCode before cancel() awaits it, so a deliberate cancellation surfaced as "Worker exited with code 143" rather than as a cancellation. Both are now scoped to the job they belong to via a generation counter, checked before any teardown or completion. The exit handler also consults _cancelRequested so a cancelled job reports itself as cancelled. The tests read the source rather than driving a real queue: the failure mode is an ordering bug between two asynchronous continuations, and a test that spawned real workers would reproduce it only intermittently. They assert the guards exist and, specifically, that they precede _cleanup() -- a guard after the teardown would pass a "contains" check while fixing nothing. All four fail against the previous version. One trap worth recording: cancel()'s own comments mention _cleanup(), so the first draft of the ordering assertion was comparing against prose and proved nothing. It matches the call now.
These tests read worker_manager.dart and match against "\n", but git checks that
file out CRLF on Windows -- so `contains('_cancelRequested\n')` and the scan for
'\n }\n' both failed there and nowhere else.
Exactly the trap that broke test_92 in filter_integration_test.rs two days ago,
walked into again. The comment now says so, next to the fix, so the next
assertion added here does not repeat it.
Verified by converting the source to CRLF locally and running both ways.
StuartCameronCode
added a commit
that referenced
this pull request
Aug 8, 2026
The artifacts on the draft carry the stuck-progress regression fixed in #65, so they are being replaced again. Version string stays at 0.9.12; build number 29 -> 30.
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.
Regression from #63. Cancelling a conversion and starting another left the
progress dialog spinning with no updates — the worker was healthy and the encode
was progressing, but nothing was listening to it.
Cause
cancel()now waits for the worker to genuinely exit, which can take seconds.Cancellation emits a completion, the queue acts on that by starting the next job,
and the tail of the cancelling call then ran unconditionally — so its
_cleanup()nulled_processand cancelled the stdout/stderr subscriptionsbelonging to the new job. No progress events could arrive after that, and
nothing errored, because from the worker's side everything was fine.
The exit handler registered in
startJobhad the same problem on its ownschedule, and additionally won the race to report: it is registered on
exitCodebefore
cancel()awaits it, so a deliberate cancellation surfaced asWorker exited with code 143rather than as a cancellation.Fix
Both are scoped to the job they belong to via a generation counter, checked
before any teardown or completion. The exit handler also consults
_cancelRequested, so a cancelled job reports itself as cancelled.Tests
They read the source rather than driving a real queue. The failure mode is an
ordering bug between two asynchronous continuations, and a test that spawned real
workers would reproduce it only intermittently — a flaky guard against a hang is
worse than none. They assert the guards exist and that they precede
_cleanup(), since a guard placed after the teardown would satisfy a plaincontainscheck while fixing nothing.All four fail against the previous version (
+0 -4) and pass against this one.