diff --git a/app/lib/services/worker_manager.dart b/app/lib/services/worker_manager.dart index dff8981..5508648 100644 --- a/app/lib/services/worker_manager.dart +++ b/app/lib/services/worker_manager.dart @@ -120,20 +120,27 @@ class WorkerManager { // 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, - cancelled: _cancelRequested, - errorMessage: _cancelRequested - ? 'Job cancelled by user' - : _lastErrorMessage ?? - (exitCode == 0 - ? 'Worker exited without reporting a result' - : 'Worker exited with code $exitCode'), - )); + // A cancelled job MUST report as cancelled, and that has to override + // `_pendingCompletion` rather than merely fill in for it. + // + // The worker sends `complete(false)` on its way out of a cancellation + // too (main.rs), and `_handleStdoutLine` builds that into a + // CompletionResult with no `cancelled` flag — so it defaults to false. + // Since `_pendingCompletion` is therefore always set, putting the flag + // in a `??` fallback left it permanently unreachable: cancelling looked + // exactly like a failure. + // + // That is not cosmetic. `_handleQueueItemCompletion` stops the queue on + // `cancelled`, but on a plain failure it marks the item failed and calls + // `_processNextItem()` — so a cancel silently started the next job and + // left `_isQueueProcessing` true, and the UI sat on "processing" with a + // spinner and no progress. + _emitCompletion(completionFor( + cancelRequested: _cancelRequested, + pending: _pendingCompletion, + lastError: _lastErrorMessage, + exitCode: exitCode, + )); _cleanup(); }); @@ -277,6 +284,51 @@ class WorkerManager { )); } + /// What to report when the worker exits. + /// + /// Pulled out as a pure function because getting it wrong is invisible from + /// inside the app: every value is plausible, nothing throws, and the only + /// symptom is the queue behaving as though a cancelled job had failed. + /// + /// A cancelled job MUST report `cancelled`, and that has to override [pending] + /// rather than merely fill in for it. The worker sends `complete(false)` on its + /// way out of a cancellation too, and `_handleStdoutLine` turns that into a + /// CompletionResult with no `cancelled` flag — so it defaults to false. Since + /// [pending] is therefore always set, an earlier attempt at this put the flag + /// in a `??` fallback, where it was permanently unreachable and cancelling + /// still looked exactly like a failure. + /// + /// The consequence is in `_handleQueueItemCompletion`: it stops the queue on + /// `cancelled`, but on a plain failure it marks the item failed and calls + /// `_processNextItem()`. So a cancel silently started the next job, left + /// `_isQueueProcessing` true, and the UI sat on "processing" with a spinner + /// and no progress. + // Public so it can be unit-tested directly; not part of the intended API. + static CompletionResult completionFor({ + required bool cancelRequested, + required CompletionResult? pending, + required String? lastError, + required int exitCode, + }) { + if (cancelRequested) { + return const CompletionResult( + success: false, + cancelled: true, + errorMessage: 'Job cancelled by user', + ); + } + // The worker reports `complete` on success and failure alike, so `pending` + // is normally set. If it isn't, the exit still has to be reported. + return pending ?? + CompletionResult( + success: false, + errorMessage: lastError ?? + (exitCode == 0 + ? 'Worker exited without reporting a result' + : 'Worker exited with code $exitCode'), + ); + } + /// Emit [result] unless this job has already reported one. void _emitCompletion(CompletionResult result) { if (_completionEmitted || _completionController.isClosed) return; diff --git a/app/test/worker_completion_result_test.dart b/app/test/worker_completion_result_test.dart new file mode 100644 index 0000000..18c5446 --- /dev/null +++ b/app/test/worker_completion_result_test.dart @@ -0,0 +1,100 @@ +// What the worker's exit reports, and why a cancellation must win. +// +// The bug these pin was invisible from inside the app: every value was +// plausible, nothing threw, and the only symptom was the queue behaving as +// though a cancelled job had failed — marking the item failed, starting the next +// job, and leaving the UI on "processing" with a spinner and no progress. +// +// Two earlier attempts missed it. The first reasoned about process teardown and +// never touched the reporting. The second put `cancelled: _cancelRequested` in a +// `??` fallback behind `_pendingCompletion` — which the worker always sets, +// because it sends `complete(false)` on its way out of a cancellation too. So +// the flag was unreachable and the behaviour was unchanged. +// +// Hence a direct test of the decision rather than a scan of the source: the +// previous tests read the file, passed, and the bug shipped anyway. + +import 'package:test/test.dart'; +import 'package:vapourbox/services/worker_manager.dart'; + +void main() { + group('completionFor', () { + test('a cancelled job reports cancelled even though the worker sent complete', + () { + // Exactly the shipped case: SIGTERM, the worker emits complete(false) on + // its way out, and _handleStdoutLine builds it with no cancelled flag. + const pending = CompletionResult(success: false, errorMessage: 'whatever'); + + final result = WorkerManager.completionFor( + cancelRequested: true, + pending: pending, + lastError: null, + exitCode: 130, + ); + + expect(result.cancelled, isTrue, + reason: 'the queue stops only on cancelled; reported as a plain ' + 'failure it marks the item failed and starts the next job'); + expect(result.success, isFalse); + }); + + test('a cancelled job reports cancelled when the worker sent nothing', () { + final result = WorkerManager.completionFor( + cancelRequested: true, + pending: null, + lastError: null, + exitCode: 143, + ); + expect(result.cancelled, isTrue); + }); + + test('a successful job is reported from the worker, untouched', () { + const pending = CompletionResult(success: true, outputPath: '/tmp/out.mkv'); + final result = WorkerManager.completionFor( + cancelRequested: false, + pending: pending, + lastError: null, + exitCode: 0, + ); + expect(result.success, isTrue); + expect(result.cancelled, isFalse); + expect(result.outputPath, '/tmp/out.mkv'); + }); + + test('a genuine failure stays a failure, not a cancellation', () { + const pending = CompletionResult(success: false, errorMessage: 'ffmpeg died'); + final result = WorkerManager.completionFor( + cancelRequested: false, + pending: pending, + lastError: 'ffmpeg died', + exitCode: 1, + ); + expect(result.cancelled, isFalse, + reason: 'a crash must not be swallowed as a cancellation — the queue ' + 'would stop silently instead of recording the failure'); + expect(result.errorMessage, 'ffmpeg died'); + }); + + test('an exit with no report at all is still reported', () { + // The hang from #50: without this the UI waits forever on a dead worker. + final result = WorkerManager.completionFor( + cancelRequested: false, + pending: null, + lastError: null, + exitCode: 3, + ); + expect(result.success, isFalse); + expect(result.errorMessage, contains('3')); + }); + + test('a clean exit with no report is distinguished from a crash', () { + final result = WorkerManager.completionFor( + cancelRequested: false, + pending: null, + lastError: null, + exitCode: 0, + ); + expect(result.errorMessage, contains('without reporting')); + }); + }); +} diff --git a/app/test/worker_manager_generation_test.dart b/app/test/worker_manager_generation_test.dart index 31079b1..7607831 100644 --- a/app/test/worker_manager_generation_test.dart +++ b/app/test/worker_manager_generation_test.dart @@ -72,17 +72,18 @@ void main() { 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. + test('the exit handler delegates the decision, passing the cancel flag', () { + // What gets reported is decided by WorkerManager.completionFor, which is + // unit-tested directly in worker_completion_result_test.dart. All that + // matters here is that the handler actually routes through it and hands + // over _cancelRequested — an earlier version computed the result inline + // and put the cancelled flag in an unreachable branch. 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'); + expect(body, contains('completionFor('), + reason: 'the exit handler must use the tested decision, not its own'); + expect(body, contains('cancelRequested: _cancelRequested'), + reason: 'a cancelled job is only distinguishable if the flag is passed'); }); test('startJob resets both flags so state cannot leak between jobs', () {