From 65a3c04359c86edd137c773e2aaec3f688bb945a Mon Sep 17 00:00:00 2001 From: Stuart Cameron Date: Sun, 9 Aug 2026 02:33:18 +1000 Subject: [PATCH 1/2] fix(cancel): stop a cancel from stranding the job that replaced it 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. --- app/lib/services/worker_manager.dart | 52 +++++++++-- app/test/worker_manager_generation_test.dart | 92 ++++++++++++++++++++ 2 files changed, 139 insertions(+), 5 deletions(-) create mode 100644 app/test/worker_manager_generation_test.dart diff --git a/app/lib/services/worker_manager.dart b/app/lib/services/worker_manager.dart index af8e1ce..dff8981 100644 --- a/app/lib/services/worker_manager.dart +++ b/app/lib/services/worker_manager.dart @@ -29,6 +29,28 @@ class WorkerManager { /// indistinguishable from the hang reported in #50. bool _completionEmitted = false; + /// Which job the manager is currently running. + /// + /// Incremented by every [startJob]. Anything asynchronous that outlives a job + /// — the exit handler, the tail of [cancel] — captures this and does nothing + /// if it no longer matches, because by then it would be acting on someone + /// else's job. + /// + /// This matters because cancelling now waits for the worker to genuinely exit, + /// which can take seconds. Cancellation emits a completion, the queue advances + /// and starts the next job on that event, and the tail of the *cancelling* + /// call then ran `_cleanup()` — nulling `_process` and cancelling the new job's + /// stdout subscription. The job ran to completion with nobody listening, so the + /// progress dialog span forever with no updates. + int _generation = 0; + + /// Whether the job identified by [_generation] is being cancelled. + /// + /// The exit handler is registered on `exitCode` before [cancel] awaits it, so + /// it reports first. Without this it describes a deliberate cancellation as + /// "Worker exited with code 143". + bool _cancelRequested = false; + /// Stream of progress updates from the worker. final _progressController = StreamController.broadcast(); Stream get progressStream => _progressController.stream; @@ -53,6 +75,8 @@ class WorkerManager { } _completionEmitted = false; + _cancelRequested = false; + final generation = ++_generation; final toolLocator = ToolLocator.instance; final workerPath = toolLocator.workerPath; @@ -89,19 +113,26 @@ class WorkerManager { // Wait for process to exit _process!.exitCode.then((exitCode) { - // Clean up config file + // Clean up config file — safe regardless of whose job this is. configFile.delete().catchError((_) => configFile); + // A newer job has since started; reporting its completion or tearing + // down its plumbing here would strand it silently. + if (generation != _generation) return; + // The worker sends a `complete` message on both success and failure, so // _pendingCompletion is normally set. If it isn't, the exit still has to // be reported — see [_completionEmitted]. _emitCompletion(_pendingCompletion ?? CompletionResult( success: false, - errorMessage: _lastErrorMessage ?? - (exitCode == 0 - ? 'Worker exited without reporting a result' - : 'Worker exited with code $exitCode'), + cancelled: _cancelRequested, + errorMessage: _cancelRequested + ? 'Job cancelled by user' + : _lastErrorMessage ?? + (exitCode == 0 + ? 'Worker exited without reporting a result' + : 'Worker exited with code $exitCode'), )); _cleanup(); @@ -188,6 +219,10 @@ class WorkerManager { // running" check was never actually false. final process = _process; if (process == null) return; + // Which job this call is cancelling. Everything after the await below is + // conditional on it still being the current one. + final generation = _generation; + _cancelRequested = true; if (Platform.isWindows) { // No SIGTERM on Windows, and Process.kill maps to TerminateProcess, which @@ -223,6 +258,13 @@ class WorkerManager { } } + // Waiting above can take seconds, and cancelling emits a completion that + // the queue acts on by starting the next job. If that has happened, this + // call must not touch anything: `_cleanup()` would null `_process` and + // cancel the new job's stdout subscription, leaving it running with nobody + // listening and the progress dialog spinning forever. + if (generation != _generation) return; + _cleanup(); _emitCompletion(CompletionResult( diff --git a/app/test/worker_manager_generation_test.dart b/app/test/worker_manager_generation_test.dart new file mode 100644 index 0000000..26848c8 --- /dev/null +++ b/app/test/worker_manager_generation_test.dart @@ -0,0 +1,92 @@ +// Cancelling must not tear down the job that replaced it. +// +// `cancel()` waits for the worker to genuinely exit, which can take seconds. +// Cancelling emits a completion, the queue acts on that by starting the next +// job, and the tail of the *cancelling* call then ran `_cleanup()` — nulling +// `_process` and cancelling the new job's stdout subscription. The new job ran +// to completion with nobody listening, so the progress dialog span forever with +// no updates. The same applies to the previous job's `exitCode` handler, which +// fires on its own schedule. +// +// Both are now conditional on the generation counter still matching. These tests +// pin that, because the symptom is a UI hang with no error anywhere: the worker +// is healthy, the encode is progressing, and nothing is listening. + +import 'dart:io'; + +import 'package:test/test.dart'; + +void main() { + group('worker manager job scoping', () { + late String source; + + setUpAll(() { + source = File('lib/services/worker_manager.dart').readAsStringSync(); + }); + + test('the exit handler does nothing once a newer job has started', () { + final start = source.indexOf('_process!.exitCode.then('); + expect(start, greaterThan(-1), reason: 'exit handler not found'); + final body = source.substring(start, source.indexOf('} catch (e) {', start)); + + expect(body, contains('if (generation != _generation) return;'), + reason: 'the exit handler must bail out when superseded, or it emits ' + "a stale completion and calls _cleanup() on someone else's job"); + + // The guard has to come before the teardown, not after it. + final guardAt = body.indexOf('if (generation != _generation) return;'); + final cleanupAt = RegExp(r'^\s*_cleanup\(\);', multiLine: true) + .firstMatch(body)! + .start; + expect(guardAt, lessThan(cleanupAt), + reason: 'the guard must precede the _cleanup() call, or the damage is ' + 'done before it is checked'); + }); + + test('cancel does not clean up a job it did not start', () { + final start = source.indexOf('Future cancel() async {'); + expect(start, greaterThan(-1)); + final body = source.substring(start); + final end = body.indexOf('\n }\n'); + final cancelBody = body.substring(0, end); + + expect(cancelBody, contains('final generation = _generation;'), + reason: 'cancel() must capture which job it is cancelling'); + expect(cancelBody, contains('if (generation != _generation) return;'), + reason: 'cancel() waits for a real exit, so by the time it resumes the ' + 'queue may have started the next job — it must not clean that up'); + // Match the call, not the word: cancel()'s own comments mention + // _cleanup(), and comparing against prose proved nothing. + final guardAt = cancelBody.indexOf('if (generation != _generation) return;'); + final cleanupAt = RegExp(r'^\s*_cleanup\(\);', multiLine: true) + .firstMatch(cancelBody)! + .start; + expect(guardAt, lessThan(cleanupAt), + reason: 'the guard must precede the _cleanup() call'); + }); + + test('a cancelled job is reported as cancelled, not as an exit code', () { + // The exit handler is registered on exitCode before cancel() awaits it, so + // it reports first. Without _cancelRequested it describes a deliberate + // cancellation as "Worker exited with code 143", and the UI cannot tell a + // cancellation from a crash. + final start = source.indexOf('_process!.exitCode.then('); + final body = source.substring(start, source.indexOf('} catch (e) {', start)); + expect(body, contains('cancelled: _cancelRequested'), + reason: 'the exit handler must mark a cancelled job as cancelled'); + expect(body, contains("_cancelRequested\n"), + reason: 'and use it for the message too'); + }); + + test('startJob resets both flags so state cannot leak between jobs', () { + final start = source.indexOf('Future startJob('); + final body = source.substring(start, source.indexOf('_process = await Process.start', start)); + expect(body, contains('_completionEmitted = false;')); + expect(body, contains('_cancelRequested = false;'), + reason: 'a stale _cancelRequested would make the next job report ' + 'itself cancelled the moment it finished'); + expect(body, contains('++_generation'), + reason: 'each job needs its own generation, or the guards never fire'); + }); + }); +} From eabcd124a8cbe0cc7f905333c6395f3bb41ab11b Mon Sep 17 00:00:00 2001 From: Stuart Cameron Date: Sun, 9 Aug 2026 02:52:01 +1000 Subject: [PATCH 2/2] fix(test): normalise line endings before scanning the source 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. --- app/test/worker_manager_generation_test.dart | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/app/test/worker_manager_generation_test.dart b/app/test/worker_manager_generation_test.dart index 26848c8..31079b1 100644 --- a/app/test/worker_manager_generation_test.dart +++ b/app/test/worker_manager_generation_test.dart @@ -21,7 +21,14 @@ void main() { late String source; setUpAll(() { - source = File('lib/services/worker_manager.dart').readAsStringSync(); + // Normalise line endings. git checks this file out CRLF on Windows, and + // every scan below is written against "\n" — so without this the whole + // suite fails there and nowhere else. Same trap as test_92 in + // filter_integration_test.rs; if you add an assertion here, do not embed + // a bare "\n" without remembering this line exists. + source = File('lib/services/worker_manager.dart') + .readAsStringSync() + .replaceAll('\r\n', '\n'); }); test('the exit handler does nothing once a newer job has started', () {