diff --git a/app/src/main/java/org/libremediaconverter/work/ConcatWorker.kt b/app/src/main/java/org/libremediaconverter/work/ConcatWorker.kt index 5ecab5a..fed999b 100644 --- a/app/src/main/java/org/libremediaconverter/work/ConcatWorker.kt +++ b/app/src/main/java/org/libremediaconverter/work/ConcatWorker.kt @@ -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)) { diff --git a/app/src/main/java/org/libremediaconverter/work/ConversionWorker.kt b/app/src/main/java/org/libremediaconverter/work/ConversionWorker.kt index 55bff81..1691f2f 100644 --- a/app/src/main/java/org/libremediaconverter/work/ConversionWorker.kt +++ b/app/src/main/java/org/libremediaconverter/work/ConversionWorker.kt @@ -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. diff --git a/app/src/test/java/org/libremediaconverter/work/AlwaysRoomPublisher.kt b/app/src/test/java/org/libremediaconverter/work/AlwaysRoomPublisher.kt new file mode 100644 index 0000000..6c9373f --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/work/AlwaysRoomPublisher.kt @@ -0,0 +1,18 @@ +package org.libremediaconverter.work + +import android.content.Context +import org.libremediaconverter.convert.OutputPublisher + +/** + * A real [OutputPublisher] that never refuses on space. Shared by the worker unit tests. + * + * The space check reads the host's free disk, which has nothing to do with what any of those tests + * are about and would make them pass or fail on how full the machine is. Where staging lives, and + * the delete, stay the production implementation — the assertions are about the real filesystem. + * + * Only what more than one test needs lives here. The stubs each test uses to force *its own* + * failure stay in that test, next to the assertion they serve. + */ +open class AlwaysRoomPublisher(context: Context) : OutputPublisher(context) { + override fun hasSpaceFor(bytes: Long): Boolean = true +} diff --git a/app/src/test/java/org/libremediaconverter/work/DeniedForegroundStartTest.kt b/app/src/test/java/org/libremediaconverter/work/DeniedForegroundStartTest.kt index 7fd3646..c2495c4 100644 --- a/app/src/test/java/org/libremediaconverter/work/DeniedForegroundStartTest.kt +++ b/app/src/test/java/org/libremediaconverter/work/DeniedForegroundStartTest.kt @@ -187,14 +187,3 @@ private class FailedFuture(private val failure: Throwable) : ListenableFuture InputProbe() } + ConversionDependencies.deviceCodecs = { DeviceCodecs.PERMISSIVE } + installTestWorkManager(app, Data.EMPTY) + } + + @After + fun tearDown() { + ConversionDependencies.reset() + } + + @Test + fun `a cancelled conversion propagates instead of being turned into a Result`() { + val worker = conversionWorker { throw CancellationException("stopped mid-transcode") } + + val thrown = runCatching { runBlocking { worker.doWork() } }.exceptionOrNull() + + assertTrue( + "cancellation must leave doWork as cancellation, not as a Result; got $thrown", + thrown is CancellationException, + ) + } + + @Test + fun `a cancelled conversion still deletes the partial it had already written`() { + val worker = conversionWorker { throw CancellationException("stopped mid-transcode") } + + runCatching { runBlocking { worker.doWork() } } + + // The engine stub writes before it throws, so this file really existed. Rethrowing without + // deleting would trade one defect for another. + assertFalse("a cancelled attempt must not leave its partial behind", stagedFile().exists()) + } + + @Test + fun `an ordinary engine failure is still answered with a Result`() { + val worker = conversionWorker { error("the muxer was never started") } + + val result = runBlocking { worker.doWork() } + + // The other half of the rule: only cancellation propagates. Widening the rethrow to every + // exception would take the user's error message away with it. + assertEquals( + ListenableWorker.Result.failure( + workDataOf(ConversionWorker.KEY_ERROR to "the muxer was never started"), + ), + result, + ) + assertFalse("a failed attempt must not leave its partial behind", stagedFile().exists()) + } + + /** + * A worker routed to the software engine, which is [failure] and nothing else. + * + * `FORCE_SOFTWARE` rather than letting the router choose: it is the one preference that decides + * without consulting the input at all, so the test says which engine it is replacing instead of + * depending on a routing rule it is not about. The input is a `file://` URI for the same kind + * of reason — a `content://` one would send the worker through FFmpegKit's SAF bridge, which is + * native. + */ + private fun conversionWorker(failure: () -> Nothing): ConversionWorker { + ConversionDependencies.software = { PartialThenFailingTranscoder(failure) } + return TestListenableWorkerBuilder( + context = app, + inputData = workDataOf( + ConversionWorker.KEY_INPUT_URI to INPUT.toString(), + ConversionWorker.KEY_DISPLAY_NAME to DISPLAY_NAME, + ConversionWorker.KEY_SIZE_BYTES to INPUT_BYTES, + ConversionWorker.KEY_CONTAINER to SPEC.container.name, + ConversionWorker.KEY_VIDEO_CODEC to SPEC.videoCodec.name, + ConversionWorker.KEY_AUDIO_CODEC to SPEC.audioCodec.name, + ConversionWorker.KEY_ENGINE_PREFERENCE to EnginePreference.FORCE_SOFTWARE.name, + ), + runAttemptCount = 0, + ).setId(JOB_ID).build() + } + + /** The staging path the worker will compute, asked for rather than spelled out here. */ + private fun stagedFile(): File = publisher.createStagingFile(ConversionWorker.outputNameFor(DISPLAY_NAME, SPEC)) + + private companion object { + val INPUT: Uri = Uri.parse("file:///tmp/holiday.mp4") + const val DISPLAY_NAME = "holiday.mp4" + const val INPUT_BYTES = 1024L + val SPEC = OutputFormat.MP4_H265.spec + val JOB_ID: UUID = UUID.fromString("00000000-0000-4000-8000-000000000003") + } +} + +/** + * An engine that writes something and then fails, which is what every real interruption looks like. + * + * Writing first is the point: a stub that only threw would let a missing `delete()` pass. + */ +private class PartialThenFailingTranscoder(private val failure: () -> Nothing) : SoftwareTranscoder { + override suspend fun run( + request: ConversionRequest, + inputPath: String, + output: File, + durationMs: Long, + onProgress: (Int) -> Unit, + ) { + output.writeBytes(ByteArray(PARTIAL_BYTES)) + failure() + } + + private companion object { + const val PARTIAL_BYTES = 2048 + } +}