Let a cancelled conversion stay cancelled
Both workers' outer `catch (e: Throwable)` caught `CancellationException` along with everything else and answered it with a `Result`. `runMedia3OrFallBack` goes out of its way to rethrow cancellation rather than fall back to software, and then the catch above it converted it anyway. A coroutine that reports completion inside a scope which has already been cancelled is structured concurrency's one rule broken, and it is the kind of break that stays quiet: nothing downstream complains, and the next thing to hold a resource across that boundary is the thing that finds out. Nothing on screen disagreed today, which is why this is a low-severity entry rather than a bug report. WorkManager cancels the worker's coroutine through `WorkerWrapper.interrupt`, which cancels `workerJob` with a `WorkerStoppedException`; the surrounding `withContext(workerJob)` then throws that whatever the worker returned, and `launch()` resolves it as `ResetWorkerStatus`. The returned `Result` is read only when nothing stopped the worker at all. So the change is about the shape of the code rather than about a symptom. One behaviour does move, and it is worth naming rather than discovering later. A cancellation that is *not* WorkManager stopping us -- FFmpegKit reporting `ReturnCode.isCancel`, which cancels the continuation -- now leaves `doWork` as a cancellation, and `WorkerWrapper` resolves a self-cancelled worker as `Resolution.Failed()` with no output data instead of the `Result.failure(KEY_ERROR ...)` it used to build. Both ViewModels already fall back on blank output data, deliberately and with a test, so the user sees "Conversion failed." either way. That is also the honest answer: the only route to `isCancel` is a cancellation someone asked for. The `staged.delete()` on that path is kept, and moved into the new branch rather than left to the one below it. A cancelled attempt leaves a partial in staging, the next attempt starts `doWork()` from the top rather than resuming it, and this catch holds the only handle to the file. Reaching any of this from a JVM test needed one more thing: `doWork` called `MediaProbe.probe` directly, and it was the last caller bypassing `ConversionDependencies.probe`. FFprobe's loader throws a bare `java.lang.Error` with no native library present, so no unit test could reach a single line below it. The seam's own KDoc says this is what it is for -- coverage of the branches that only run when something goes wrong -- and the default is the same real probe, so the app and the instrumented tests are unchanged. `WorkerCancellationTest` drives the real worker through a real `WorkManager` to the software engine, forced with `FORCE_SOFTWARE` because it is the one preference that decides without consulting the input, so the test does not depend on a routing rule it is not about. The engine stub writes bytes before it throws, which is what makes the delete assertion mean something: a stub that only threw would let a missing `delete()` pass. Three cases -- cancellation propagates, cancellation still deletes, and an ordinary failure is still answered with a `Result` carrying its message, which is the half that would break if the rethrow were widened past cancellation. Before the fix the first of those failed with "cancellation must leave doWork as cancellation, not as a Result; got null" -- `runCatching` had nothing to report, because `doWork` had returned. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -10,6 +10,7 @@ import androidx.work.ForegroundInfo
|
||||
import androidx.work.OneTimeWorkRequestBuilder
|
||||
import androidx.work.WorkerParameters
|
||||
import androidx.work.workDataOf
|
||||
import kotlinx.coroutines.CancellationException
|
||||
import org.libremediaconverter.convert.ConversionDependencies
|
||||
import org.libremediaconverter.ffmpeg.ConcatEngine
|
||||
import org.libremediaconverter.model.OutputFormat
|
||||
@@ -70,6 +71,11 @@ class ConcatWorker(context: Context, params: WorkerParameters) : CoroutineWorker
|
||||
KEY_STRATEGY to result.strategy.name,
|
||||
),
|
||||
)
|
||||
} catch (e: CancellationException) {
|
||||
// Rethrown rather than answered with a Result -- see the same branch in
|
||||
// ConversionWorker for why, and for why the delete stays.
|
||||
staged.delete()
|
||||
throw e
|
||||
} catch (e: Throwable) {
|
||||
staged.delete()
|
||||
when (FailureOutcome.forFailure(stopReason, e, runAttemptCount)) {
|
||||
|
||||
@@ -11,8 +11,8 @@ import androidx.work.OneTimeWorkRequestBuilder
|
||||
import androidx.work.WorkerParameters
|
||||
import androidx.work.workDataOf
|
||||
import com.arthenica.ffmpegkit.FFmpegKitConfig
|
||||
import kotlinx.coroutines.CancellationException
|
||||
import org.libremediaconverter.convert.ConversionDependencies
|
||||
import org.libremediaconverter.convert.MediaProbe
|
||||
import org.libremediaconverter.model.AudioCodec
|
||||
import org.libremediaconverter.model.Container
|
||||
import org.libremediaconverter.model.ContainerCapabilities
|
||||
@@ -84,7 +84,12 @@ class ConversionWorker(context: Context, params: WorkerParameters) : CoroutineWo
|
||||
// was outside for the same reason and had the same problem.
|
||||
setForeground(foregroundInfo(displayName, percent = 0, indeterminate = true))
|
||||
|
||||
val probe = MediaProbe.probe(applicationContext, inputUri)
|
||||
// Through the seam rather than MediaProbe directly. The seam already existed for the
|
||||
// ViewModel and the worker was the last caller bypassing it, which is why nothing on
|
||||
// the JVM could reach a line below this one: FFprobe's loader throws a bare
|
||||
// java.lang.Error with no native library present. The app and the instrumented tests
|
||||
// get the real probe, exactly as before.
|
||||
val probe = ConversionDependencies.probe(applicationContext, inputUri)
|
||||
val devices = ConversionDependencies.deviceCodecs()
|
||||
val request = ConversionRequest(
|
||||
spec = spec,
|
||||
@@ -117,6 +122,17 @@ class ConversionWorker(context: Context, params: WorkerParameters) : CoroutineWo
|
||||
KEY_ROUTE_REASON to decision.reason.explanation,
|
||||
),
|
||||
)
|
||||
} catch (e: CancellationException) {
|
||||
// Cancellation is not a result, and answering it with one breaks structured
|
||||
// concurrency: this coroutine would report completion inside a scope that has already
|
||||
// been cancelled. Invisible today only because WorkManager marks the work CANCELLED
|
||||
// itself and ignores whatever the worker returned.
|
||||
//
|
||||
// The delete still has to happen, and has to happen here. A cancelled attempt leaves a
|
||||
// partial in staging, the next attempt starts from the top rather than resuming it,
|
||||
// and this is the only code holding the handle.
|
||||
staged.delete()
|
||||
throw e
|
||||
} catch (e: Throwable) {
|
||||
staged.delete()
|
||||
outcomeFor(e)
|
||||
@@ -185,7 +201,7 @@ class ConversionWorker(context: Context, params: WorkerParameters) : CoroutineWo
|
||||
}
|
||||
}
|
||||
|
||||
private fun isCancellation(e: Throwable): Boolean = e is kotlinx.coroutines.CancellationException || isStopped
|
||||
private fun isCancellation(e: Throwable): Boolean = e is CancellationException || isStopped
|
||||
|
||||
/**
|
||||
* Turns whatever ended the attempt into a `Result`. [FailureOutcome] owns the rules.
|
||||
|
||||
Reference in New Issue
Block a user