diff --git a/app/src/main/java/org/libremediaconverter/work/ConcatWorker.kt b/app/src/main/java/org/libremediaconverter/work/ConcatWorker.kt index 553310b..23d95d2 100644 --- a/app/src/main/java/org/libremediaconverter/work/ConcatWorker.kt +++ b/app/src/main/java/org/libremediaconverter/work/ConcatWorker.kt @@ -46,9 +46,12 @@ class ConcatWorker(context: Context, params: WorkerParameters) : CoroutineWorker val declaredTotal = inputData .takeIf { it.hasKeyWithValueOfType(KEY_TOTAL_BYTES) } ?.getLong(KEY_TOTAL_BYTES, 0L) - val format = OutputFormat.valueOf( - inputData.getString(KEY_FORMAT) ?: DEFAULT_FORMAT.name, - ) + // Looked up rather than `valueOf` -- see the same three reads in ConversionWorker. This one + // is above the try as well, so a format name this build does not define used to throw past + // the catch: FAILED with no error in the output Data, and no staged.delete(). + val format = inputData.getString(KEY_FORMAT) + ?.let { name -> OutputFormat.entries.firstOrNull { it.name == name } } + ?: DEFAULT_FORMAT if (!hasRoomFor(declaredTotal, uris)) { return Result.failure(workDataOf(KEY_ERROR to "Not enough free space to join these files.")) diff --git a/app/src/main/java/org/libremediaconverter/work/ConversionWorker.kt b/app/src/main/java/org/libremediaconverter/work/ConversionWorker.kt index a31249f..40e37cc 100644 --- a/app/src/main/java/org/libremediaconverter/work/ConversionWorker.kt +++ b/app/src/main/java/org/libremediaconverter/work/ConversionWorker.kt @@ -67,12 +67,18 @@ class ConversionWorker(context: Context, params: WorkerParameters) : CoroutineWo .takeIf { it.hasKeyWithValueOfType(KEY_SIZE_BYTES) } ?.getLong(KEY_SIZE_BYTES, 0L) val spec = readSpec() - val quality = QualityTier.valueOf( - inputData.getString(KEY_QUALITY) ?: QualityTier.FAST.name, - ) - val preference = EnginePreference.valueOf( - inputData.getString(KEY_ENGINE_PREFERENCE) ?: EnginePreference.AUTO.name, - ) + // Looked up rather than `valueOf`, for the reason [readSpec] gives twelve lines below and + // for one more: these three reads sit ABOVE the try. A name this build does not define -- + // which is what a downgrade with work still queued produces, the same previous-version case + // JobTags is written for -- threw IllegalArgumentException straight past the catch, taking + // the retry, the error message and the staged file's delete with it. That is the escape + // shape setForeground was moved inside the try to end. + val quality = inputData.getString(KEY_QUALITY) + ?.let { name -> QualityTier.entries.firstOrNull { it.name == name } } + ?: QualityTier.FAST + val preference = inputData.getString(KEY_ENGINE_PREFERENCE) + ?.let { name -> EnginePreference.entries.firstOrNull { it.name == name } } + ?: EnginePreference.AUTO if (!hasRoomFor(declaredSize, inputUri)) { return Result.failure(workDataOf(KEY_ERROR to "Not enough free space to convert.")) diff --git a/app/src/test/java/org/libremediaconverter/work/FailureOutcomeTest.kt b/app/src/test/java/org/libremediaconverter/work/FailureOutcomeTest.kt index d342137..f064ded 100644 --- a/app/src/test/java/org/libremediaconverter/work/FailureOutcomeTest.kt +++ b/app/src/test/java/org/libremediaconverter/work/FailureOutcomeTest.kt @@ -2,7 +2,9 @@ package org.libremediaconverter.work import android.app.ForegroundServiceStartNotAllowedException import androidx.work.WorkInfo +import androidx.work.WorkRequest import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue import org.junit.Test import org.junit.runner.RunWith import org.robolectric.RobolectricTestRunner @@ -144,8 +146,48 @@ class FailureOutcomeTest { ) } + @Test + fun `the attempt bound outlasts a night rather than being a round number`() { + // The KDoc argues the value rather than picking one: ten attempts against WorkManager's + // default backoff span about eight and a half hours, which is what turns "the user will + // have opened the app before this gives up" into a claim instead of a hope. + // + // Asserted as that span rather than as the literal 10, so a deliberate re-tune keeping the + // property passes while an accidental one fails. The accident is not hypothetical: at 2 the + // job gives up about ninety seconds after process death -- losing exactly the long + // conversion the retry exists to protect -- and every other test in this file still passes, + // because they are all written against the constant rather than against its value. + var delay = WorkRequest.DEFAULT_BACKOFF_DELAY_MILLIS + var span = 0L + repeat(FailureOutcome.MAX_FOREGROUND_START_ATTEMPTS) { + span += delay + // Doubling per attempt, clamped, exactly as WorkManager schedules it. Coerced each + // time round so the arithmetic cannot overflow whatever the bound is set to. + delay = (delay * 2).coerceAtMost(WorkRequest.MAX_BACKOFF_MILLIS) + } + + assertTrue( + "the retry budget must outlast a night; it spans ${span / MILLIS_PER_HOUR.toDouble()} hours", + span >= MINIMUM_RETRY_SPAN_MS, + ) + } + private fun denied() = ForegroundServiceStartNotAllowedException( "startForegroundService() not allowed: service " + "org.libremediaconverter/androidx.work.impl.foreground.SystemForegroundService", ) + + private companion object { + const val MILLIS_PER_HOUR = 60L * 60 * 1000 + + /** + * How long the denied-start retries have to keep going. + * + * Eight hours rather than the eight and a half the default backoff actually produces. The + * claim is about covering a night between one opening of the app and the next; pinning the + * exact arithmetic would instead fail on a WorkManager release that re-tuned its own + * constants without anything this app decides having changed. + */ + const val MINIMUM_RETRY_SPAN_MS = 8L * MILLIS_PER_HOUR + } } diff --git a/app/src/test/java/org/libremediaconverter/work/WorkerEnumFallbackTest.kt b/app/src/test/java/org/libremediaconverter/work/WorkerEnumFallbackTest.kt new file mode 100644 index 0000000..9b3ff3a --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/work/WorkerEnumFallbackTest.kt @@ -0,0 +1,194 @@ +package org.libremediaconverter.work + +import android.app.Application +import android.content.Context +import android.net.Uri +import androidx.media3.common.util.UnstableApi +import androidx.work.Data +import androidx.work.ListenableWorker +import androidx.work.testing.TestListenableWorkerBuilder +import androidx.work.workDataOf +import kotlinx.coroutines.runBlocking +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.libremediaconverter.convert.ConversionDependencies +import org.libremediaconverter.convert.OutputPublisher +import org.libremediaconverter.convert.SoftwareTranscoder +import org.libremediaconverter.convert.StagingNames +import org.libremediaconverter.convert.installTestWorkManager +import org.libremediaconverter.model.ConversionRequest +import org.libremediaconverter.model.DeviceCodecs +import org.libremediaconverter.model.EnginePreference +import org.libremediaconverter.model.InputProbe +import org.libremediaconverter.model.OutputFormat +import org.libremediaconverter.model.QualityTier +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment +import java.io.File +import java.util.UUID + +/** + * Input `Data` naming something this build does not define. + * + * Not a malformed-input hypothetical: WorkManager keeps queued and finished work for about a week, + * so a downgrade — or any rollback with work still in the queue — hands this build a job enqueued + * by another one. That is the same previous-version case [JobTags] is written for. + * + * What made it worth a test is *where* the reads are. All three sit above the workers' `try`, so an + * unknown name threw `IllegalArgumentException` out of `doWork()` entirely: WorkManager logged + * FAILURE with `reschedule = false`, the output `Data` reached the UI with zero entries so the + * screen said "Conversion failed." with nothing else, and the staged file was never deleted. That + * is the signature `setForeground` was moved inside the `try` to end, reached through a different + * door. + */ +@UnstableApi +@RunWith(RobolectricTestRunner::class) +class WorkerEnumFallbackTest { + + private lateinit var app: Application + private lateinit var publisher: NamingPublisher + + @Before + fun setUp() { + app = RuntimeEnvironment.getApplication() + publisher = NamingPublisher(app) + ConversionDependencies.publisher = { publisher } + ConversionDependencies.probe = { _, _ -> InputProbe() } + ConversionDependencies.deviceCodecs = { DeviceCodecs.PERMISSIVE } + // The progress notification builds its cancel action from WorkManager.getInstance(). + installTestWorkManager(app, Data.EMPTY) + } + + @After + fun tearDown() { + ConversionDependencies.reset() + } + + @Test + fun `a quality tier this build does not define falls back to the default`() { + val transcoder = RequestRecordingTranscoder() + ConversionDependencies.software = { transcoder } + + val result = runBlocking { conversionWorker(quality = "ULTRA_FIDELITY").doWork() } + + // A Result at all is half the assertion -- the read is above the try, so the defect was an + // exception rather than a wrong answer. The other half is which tier ran: falling back to + // something arbitrary would silently convert at a quality nobody asked for. + assertEquals(ListenableWorker.Result.success(), stripOutput(result)) + assertEquals(listOf(QualityTier.FAST), transcoder.qualities) + } + + @Test + fun `an engine preference this build does not define does not end the job`() { + // Refused on space, which is the first thing below the three reads: it proves the reads + // were reached and returned, without dragging in a routing decision this test is not about. + publisher.refuse = true + + val result = runBlocking { conversionWorker(preference = "FORCE_QUANTUM").doWork() } + + assertEquals( + ListenableWorker.Result.failure( + workDataOf(ConversionWorker.KEY_ERROR to "Not enough free space to convert."), + ), + result, + ) + } + + @Test + fun `an output format this build does not define falls back to the default`() { + runBlocking { concatWorker(format = "AVI_MPEG4").doWork() } + + // The join itself fails -- ConcatEngine is native and there is no seam for it here -- so + // what is asserted is the name it staged under, which is where the format actually lands. + // A format nobody could resolve must produce the default's extension, not no extension and + // not a throw on the way past. + assertEquals( + listOf(StagingNames.forJob(CONCAT_ID, ConcatWorker.DEFAULT_FORMAT.extension)), + publisher.requestedNames, + ) + } + + /** [ListenableWorker.Result.Success] compares its output data, which these tests do not pin. */ + private fun stripOutput(result: ListenableWorker.Result): ListenableWorker.Result = + if (result is ListenableWorker.Result.Success) ListenableWorker.Result.success() else result + + private fun conversionWorker( + quality: String = QualityTier.FAST.name, + preference: String = EnginePreference.FORCE_SOFTWARE.name, + ): ConversionWorker = 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_QUALITY to quality, + ConversionWorker.KEY_ENGINE_PREFERENCE to preference, + ), + runAttemptCount = 0, + ).setId(CONVERSION_ID).build() + + private fun concatWorker(format: String): ConcatWorker = TestListenableWorkerBuilder( + context = app, + inputData = workDataOf( + ConcatWorker.KEY_INPUT_URIS to arrayOf(INPUT.toString(), "file:///tmp/second.mp4"), + ConcatWorker.KEY_TOTAL_BYTES to INPUT_BYTES, + ConcatWorker.KEY_FORMAT to format, + ), + runAttemptCount = 0, + ).setId(CONCAT_ID).build() + + 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 CONVERSION_ID: UUID = UUID.fromString("00000000-0000-4000-8000-000000000021") + val CONCAT_ID: UUID = UUID.fromString("00000000-0000-4000-8000-000000000022") + } +} + +/** + * A real [OutputPublisher] that records the staging names it is asked for, and can refuse on space. + * + * The name is the only place a join's format is legible from outside: the engine that would use it + * is native, and the worker deletes the staged file on its way out of a failed attempt. + */ +private class NamingPublisher(context: Context) : OutputPublisher(context) { + + val requestedNames = mutableListOf() + var refuse = false + + override fun hasSpaceFor(bytes: Long): Boolean = !refuse + + override fun createStagingFile(name: String): File { + requestedNames += name + return super.createStagingFile(name) + } +} + +/** An engine that writes the output and remembers what it was asked to produce. */ +private class RequestRecordingTranscoder : SoftwareTranscoder { + + val qualities = mutableListOf() + + override suspend fun run( + request: ConversionRequest, + inputPath: String, + output: File, + durationMs: Long, + onProgress: (Int) -> Unit, + ) { + qualities += request.quality + output.writeBytes(ByteArray(OUTPUT_BYTES)) + } + + private companion object { + const val OUTPUT_BYTES = 512 + } +}