From a6cf4f4ff48cdff17f5295b6921b6b5c32bd3cd9 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Sat, 22 Aug 2026 23:11:03 -0500 Subject: [PATCH] Stop three enum reads escaping doWork, and pin what the attempt bound buys Both workers read enums out of their input Data with Enum.valueOf, and all three reads sit ABOVE the try. A name this build does not define -- which is what a downgrade or a rollback with work still in the queue produces, since WorkManager keeps work for about a week -- threw IllegalArgumentException straight out of doWork(). That is D13's signature verbatim: FAILURE with reschedule = false, output Data with zero entries so the screen says "Conversion failed." and nothing else, and no staged.delete(), so the partial stays in cache. readSpec() twelve lines below already handles exactly this case, and its KDoc says why. So all three take readSpec's shape: entries.firstOrNull { it.name == name } ?: default. Consistency argues for it as much as correctness does -- the file already contains the right answer to this question, three times. WorkerEnumFallbackTest reaches each read. Two of them pin the value that replaces the unknown name rather than only that nothing threw: a quality tier falling back to something arbitrary would convert at a setting nobody chose, and a join's format decides the extension its output is staged with, which is where FFmpeg infers the container from. The engine-preference case asserts through the space check instead, because predicting which engine AUTO picks would tie the test to a routing rule it is not about. Against the unfixed code all three fail with "No enum constant ...". MAX_FOREGROUND_START_ATTEMPTS had no test of its value. Both existing cases are written against the symbol, which pins the relationship and leaves the number free: changed to 2, the job gives up about ninety seconds after process death -- exactly the long conversion the retry exists to protect -- and all 257 tests stayed green. The new assertion is the property the KDoc argues, not the literal: summed against WorkRequest's own DEFAULT_BACKOFF_DELAY_MILLIS and MAX_BACKOFF_MILLIS, the attempts have to span at least eight hours, which is what makes "the user will have opened the app by then" a claim rather than a hope. A deliberate re-tune that keeps the property passes; the accident does not, and reports the span it got (0.025 hours at 2). R22 / #31, R10 / #19 Co-Authored-By: Claude Opus 5 (1M context) --- .../libremediaconverter/work/ConcatWorker.kt | 9 +- .../work/ConversionWorker.kt | 18 +- .../work/FailureOutcomeTest.kt | 42 ++++ .../work/WorkerEnumFallbackTest.kt | 194 ++++++++++++++++++ 4 files changed, 254 insertions(+), 9 deletions(-) create mode 100644 app/src/test/java/org/libremediaconverter/work/WorkerEnumFallbackTest.kt 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 + } +}