JMR-dev/LibreMediaConverter
1
0
Fork 0
Code Issues 16 Pull Requests Actions Packages Projects Releases Wiki Activity

Stop three enum reads escaping doWork, and pin what the attempt bound buys

Browse Source 
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) <noreply@anthropic.com>
This commit is contained in:
JMR-dev
2026-08-22 23:11:03 -05:00
co-authored by Claude Opus 5
parent c5c4c5323b
commit a6cf4f4ff4
4 changed files with 254 additions and 9 deletions
Download Patch File Download Diff File Expand all files Collapse all files
app/src/main/java/org/libremediaconverter/work/ConcatWorker.kt
+6 -3
View File
@@ -46,9 +46,12 @@ class ConcatWorker(context: Context, params: WorkerParameters) : CoroutineWorker
val declaredTotal = inputData
.takeIf { it.hasKeyWithValueOfType<Long>(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."))
app/src/main/java/org/libremediaconverter/work/ConversionWorker.kt
+12 -6
View File
@@ -67,12 +67,18 @@ class ConversionWorker(context: Context, params: WorkerParameters) : CoroutineWo
.takeIf { it.hasKeyWithValueOfType<Long>(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."))
app/src/test/java/org/libremediaconverter/work/FailureOutcomeTest.kt
+42
View File
@@ -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
}
}
app/src/test/java/org/libremediaconverter/work/WorkerEnumFallbackTest.kt
+194
View File
@@ -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<ConversionWorker>(
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<ConcatWorker>(
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<String>()
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<QualityTier>()
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
}
}