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..31079b1 --- /dev/null +++ b/app/test/worker_manager_generation_test.dart @@ -0,0 +1,99 @@ +// 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(() { + // 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', () { + 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'); + }); + }); +}