From fea480b000bbe09d78d99a999ac067b04dd92f8a Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Sat, 29 Aug 2026 10:37:13 -0500 Subject: [PATCH] W2 (#155): the join state mapping, and a crash the seam exposed The join-side twin of W1, deliberately the same shape -- one refactor done twice, and letting the two diverge would cost more than the duplication. Five arms had never been chosen by any test, for the same reason: a real ConcatWorker only ever reaches a terminal state with well-formed output. WHAT THE SEAM TURNED UP. This line was in the SUCCEEDED arm: info.outputData.getString(ConcatWorker.KEY_STRATEGY) ?.let(ConcatStrategy::valueOf) ?: ConcatStrategy.REENCODE `valueOf` throws IllegalArgumentException on a name this build does not define, and this runs inside a `viewModelScope` collect with no handler -- so it is not a Failed state, it takes the process down. Not theoretical. WorkManager keeps finished work about a week, so a downgrade or rollback hands this build a job enqueued by another one -- the premise `WorkerEnumFallbackTest` and `JobTags` are both written on. `ConcatWorker` writes `result.strategy.name`, so a build that added a third strategy would leave this one crashing on its own completed joins. The codebase had already made this exact fix one file over, and said why: // 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 The matching read on the ViewModel side had not been changed with it. It is now `ConcatStrategy.entries.firstOrNull { it.name == name } ?: REENCODE`. PROVEN RATHER THAN ASSERTED. Restoring `valueOf` and running the new test: RED: an unknown strategy name is read as a re-encode rather than thrown java.lang.IllegalArgumentException: No enum constant org.libremediaconverter.model.ConcatStrategy.SMART_CONCAT_V2 REENCODE is the conservative default rather than an arbitrary one: it is the answer for inputs that do not match, so a job whose strategy cannot be read is described as the more cautious of the two rather than claimed as a lossless stream copy. The mutation to STREAM_COPY reddens two tests. Eight mutations, all red: unknown strategy -> STREAM_COPY | 2 tests runAttemptCount ignored, both directions | 2 tests success with no path -> empty Joined | 1 test BLOCKED unfolded from RUNNING | 1 test blank error no longer falls back | 1 test cancellation ignores the caller's state | 1 test 516 -> 530 tests, 87.7% -> 88.0% line, 70.4% -> 71.3% branch. Gate green: assembleDebug, testDebugUnitTest, compileDebugAndroidTestKotlin, ktlintCheck, detekt, lintDebug. Stacked on W1 (#162), which this mirrors and should not land before. Co-Authored-By: Claude Opus 5 (1M context) --- .../libremediaconverter/join/JoinViewModel.kt | 166 ++++++++++----- .../join/JoinStateMappingTest.kt | 200 ++++++++++++++++++ 2 files changed, 316 insertions(+), 50 deletions(-) create mode 100644 app/src/test/java/org/libremediaconverter/join/JoinStateMappingTest.kt diff --git a/app/src/main/java/org/libremediaconverter/join/JoinViewModel.kt b/app/src/main/java/org/libremediaconverter/join/JoinViewModel.kt index 665d7d7..1fcfa6d 100644 --- a/app/src/main/java/org/libremediaconverter/join/JoinViewModel.kt +++ b/app/src/main/java/org/libremediaconverter/join/JoinViewModel.kt @@ -5,6 +5,7 @@ import android.net.Uri import androidx.lifecycle.AndroidViewModel import androidx.lifecycle.viewModelScope import androidx.media3.common.util.UnstableApi +import androidx.work.Data import androidx.work.WorkInfo import androidx.work.WorkManager import kotlinx.coroutines.CoroutineDispatcher @@ -30,6 +31,108 @@ import org.libremediaconverter.work.jobSnapshots import java.io.File import java.util.UUID +/** + * One update about a running join, as WorkManager last reported it. + * + * The join-side twin of `ConversionUpdate`, and deliberately the same shape: this pair of seams is + * one refactor done twice, and letting them diverge would make the two flows harder to compare than + * the duplication costs. There is no `progressPercent` here because `ConcatWorker` publishes none — + * a join is indeterminate. + */ +internal data class JoinUpdate(val state: WorkInfo.State, val runAttemptCount: Int, val outputData: Data) + +/** + * What the join screen should show, given what WorkManager last said about the job. + * + * The join-side twin of `conversionStateFrom`, extracted for the same reason and with the same two + * exclusions: the ownership check stays at the call site, and this takes no responsibility for the + * staged file. See that function's KDoc for the argument in full. + * + * Five arms had never been chosen by any test before this was cut out, because a real `ConcatWorker` + * only ever produces a terminal state with well-formed output. + * + * @param cancelled where a cancellation lands, which differs for a reattached job — see [observe]. + */ +@UnstableApi +internal fun joinStateFrom(update: JoinUpdate, inputs: List, cancelled: JoinState): JoinState = + when (update.state) { + // BLOCKED is a job waiting on a prerequisite, which the user has nothing to do about and + // nothing useful to be told about. It reads as "starting", like a fresh ENQUEUED. + WorkInfo.State.RUNNING, WorkInfo.State.BLOCKED -> JoinState.Joining(inputs) + + // ENQUEUED after a run means a retry is pending -- the same rule, and the same reasoning, as + // the convert side. See `conversionStateFrom`. + WorkInfo.State.ENQUEUED -> + if (update.runAttemptCount > 0) { + JoinState.Waiting(inputs) + } else { + JoinState.Joining(inputs) + } + + WorkInfo.State.SUCCEEDED -> joinedFrom(update.outputData) + + // A worker that dies before it can report anything leaves no output data at all, and an + // exception's message can be an empty string. Both would read as a failure with nothing said, + // so blank falls back like missing does. + WorkInfo.State.FAILED -> JoinState.Failed( + update.outputData.getString(ConcatWorker.KEY_ERROR) + ?.takeIf { it.isNotBlank() } + ?: ConcatWorker.GENERIC_FAILURE_MESSAGE, + ) + + WorkInfo.State.CANCELLED -> cancelled + } + +/** + * The `SUCCEEDED` arm. A join that reported success and named no file has nothing to offer. + */ +@UnstableApi +private fun joinedFrom(outputData: Data): JoinState { + val path = outputData.getString(ConcatWorker.KEY_OUTPUT_PATH) + ?: return JoinState.Failed(JOINED_WITHOUT_A_FILE_MESSAGE) + return JoinState.Joined( + staged = File(path), + strategy = strategyFrom(outputData.getString(ConcatWorker.KEY_STRATEGY)), + // A join enqueued before the worker reported these carries neither, and the fallback is + // the format such a job really used -- ConcatWorker.request has always defaulted to it, + // and the join screen has never offered a choice. + suggestedName = outputData.getString(ConcatWorker.KEY_SUGGESTED_NAME) + ?.takeIf { it.isNotBlank() } + ?: ConcatWorker.outputNameFor(ConcatWorker.DEFAULT_FORMAT), + mimeType = outputData.getString(ConcatWorker.KEY_MIME_TYPE) + ?.takeIf { it.isNotBlank() } + ?: ConcatWorker.DEFAULT_FORMAT.mimeType, + ) +} + +/** + * The strategy a finished join reported, or [ConcatStrategy.REENCODE] when it named none. + * + * **Looked up rather than `valueOf`, and that is a fix rather than a style choice.** `valueOf` + * throws `IllegalArgumentException` on a name this build does not define, and this runs inside a + * `viewModelScope` collect with no handler -- so the throw does not become a `Failed` state, it + * takes the process down. + * + * Reachable for the reason `WorkerEnumFallbackTest` and `JobTags` are both written on: WorkManager + * keeps finished work for about a week, so a downgrade or a rollback hands this build a job + * enqueued by another one. `ConcatWorker` writes `result.strategy.name` into the output `Data`, so + * a build that added a third strategy would leave this one crashing on its own completed joins. + * + * `ConcatWorker.kt` already made exactly this change for `KEY_FORMAT`, and says why in as many + * words: *"Looked up rather than `valueOf` … a format name this build does not define used to throw + * past the catch."* The same read on this side had not been changed with it. + * + * REENCODE is the safe default rather than an arbitrary one: it is the answer for inputs that do + * not match, so a job whose strategy cannot be read is described as the more conservative of the + * two rather than being claimed as a lossless stream copy. + */ +private fun strategyFrom(name: String?): ConcatStrategy = + ConcatStrategy.entries.firstOrNull { it.name == name } ?: ConcatStrategy.REENCODE + +/** A join that reported success and named no file. There is nothing to offer the user to save. */ +internal const val JOINED_WITHOUT_A_FILE_MESSAGE: String = + "Joining reported success but produced no file." + sealed interface JoinState { data object Idle : JoinState data class Ready(val inputs: List) : JoinState @@ -251,56 +354,19 @@ class JoinViewModel @JvmOverloads constructor( // takes ownership of the staged file, and a superseded observation must not do // that either. if (!ownership.stillHeldBy(token)) return@collect - _state.value = when (info.state) { - WorkInfo.State.RUNNING, WorkInfo.State.BLOCKED -> JoinState.Joining(inputs) - WorkInfo.State.ENQUEUED -> - if (info.runAttemptCount > 0) { - JoinState.Waiting(inputs) - } else { - JoinState.Joining(inputs) - } - - WorkInfo.State.SUCCEEDED -> { - val path = info.outputData.getString(ConcatWorker.KEY_OUTPUT_PATH) - val strategy = info.outputData.getString(ConcatWorker.KEY_STRATEGY) - ?.let(ConcatStrategy::valueOf) ?: ConcatStrategy.REENCODE - if (path == null) { - JoinState.Failed("Joining reported success but produced no file.") - } else { - val staged = File(path) - // Take responsibility for the file at the same moment the state - // starts referring to it, so the two cannot disagree. - pendingStaged = staged - JoinState.Joined( - staged = staged, - strategy = strategy, - // A join enqueued before the worker reported these carries - // neither, and the fallback is the format such a job really - // used -- ConcatWorker.request has always defaulted to it, and - // the join screen has never offered a choice. - suggestedName = info.outputData - .getString(ConcatWorker.KEY_SUGGESTED_NAME) - ?.takeIf { it.isNotBlank() } - ?: ConcatWorker.outputNameFor(ConcatWorker.DEFAULT_FORMAT), - mimeType = info.outputData - .getString(ConcatWorker.KEY_MIME_TYPE) - ?.takeIf { it.isNotBlank() } - ?: ConcatWorker.DEFAULT_FORMAT.mimeType, - ) - } - } - - // A worker that dies before it can report anything leaves no output data at - // all, and an exception's message can be an empty string. Both would read as - // a failure with nothing said, so blank falls back like missing does. - WorkInfo.State.FAILED -> JoinState.Failed( - info.outputData.getString(ConcatWorker.KEY_ERROR) - ?.takeIf { it.isNotBlank() } - ?: ConcatWorker.GENERIC_FAILURE_MESSAGE, - ) - - WorkInfo.State.CANCELLED -> cancelled - } + val next = joinStateFrom( + JoinUpdate( + state = info.state, + runAttemptCount = info.runAttemptCount, + outputData = info.outputData, + ), + inputs = inputs, + cancelled = cancelled, + ) + // Read off the result rather than assigned inside the mapping -- see the same + // three lines in ConversionViewModel for why that is the better half of the swap. + if (next is JoinState.Joined) pendingStaged = next.staged + _state.value = next } } } diff --git a/app/src/test/java/org/libremediaconverter/join/JoinStateMappingTest.kt b/app/src/test/java/org/libremediaconverter/join/JoinStateMappingTest.kt new file mode 100644 index 0000000..3224278 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/join/JoinStateMappingTest.kt @@ -0,0 +1,200 @@ +package org.libremediaconverter.join + +import android.net.Uri +import androidx.media3.common.util.UnstableApi +import androidx.work.Data +import androidx.work.WorkInfo +import androidx.work.workDataOf +import org.junit.Assert.assertEquals +import org.junit.Test +import org.junit.runner.RunWith +import org.libremediaconverter.convert.InputFile +import org.libremediaconverter.model.ConcatStrategy +import org.libremediaconverter.work.ConcatWorker +import org.robolectric.RobolectricTestRunner + +/** + * Every answer [joinStateFrom] can give, chosen rather than stumbled into. + * + * The join-side twin of `ConversionStateMappingTest`, and the argument is the same one: the mapping + * ran on every test that drove a real `ConcatWorker`, but a real worker only ever reaches a terminal + * state with well-formed output, so five arms had never been *chosen* by anything. + * + * ## The one that is not just coverage + * + * `an unknown strategy name is read as a re-encode rather than thrown` covers a real defect this + * seam exposed. The line it replaces was: + * + * ```kotlin + * .getString(ConcatWorker.KEY_STRATEGY)?.let(ConcatStrategy::valueOf) ?: ConcatStrategy.REENCODE + * ``` + * + * `valueOf` throws on a name this build does not define, and this runs inside a `viewModelScope` + * collect with no handler — so it does not become a `Failed` state, it takes the process down. + * `ConcatWorker.kt` had already made this exact change for `KEY_FORMAT` and written down why; the + * matching read on this side had not been changed with it. + */ +@UnstableApi +@RunWith(RobolectricTestRunner::class) +class JoinStateMappingTest { + + @Test + fun `a running join is joining`() { + assertEquals(JoinState.Joining(INPUTS), map(WorkInfo.State.RUNNING)) + } + + @Test + fun `a blocked join looks like one that is starting`() { + // Folded into the RUNNING arm deliberately: a job waiting on a prerequisite is nothing the + // user can act on, and a separate word for it would be noise. + assertEquals(JoinState.Joining(INPUTS), map(WorkInfo.State.BLOCKED)) + } + + @Test + fun `an enqueued join that has already run is waiting to retry`() { + assertEquals(JoinState.Waiting(INPUTS), map(WorkInfo.State.ENQUEUED, runAttemptCount = 1)) + } + + @Test + fun `an enqueued join that has never run is simply starting`() { + // The other side. Without it, a mapping that ignored runAttemptCount passes the test above. + assertEquals(JoinState.Joining(INPUTS), map(WorkInfo.State.ENQUEUED, runAttemptCount = 0)) + } + + @Test + fun `a success that named no file is a failure, not an empty success`() { + assertEquals( + JoinState.Failed(JOINED_WITHOUT_A_FILE_MESSAGE), + map(WorkInfo.State.SUCCEEDED, data = Data.EMPTY), + ) + } + + @Test + fun `a success carries the strategy the worker actually used`() { + // Not cosmetic: the join screen tells the user whether their files were stream-copied or + // re-encoded, which is the difference between lossless and lossy. + val joined = map( + WorkInfo.State.SUCCEEDED, + data = workDataOf( + ConcatWorker.KEY_OUTPUT_PATH to "/cache/conversions/joined.mp4", + ConcatWorker.KEY_STRATEGY to ConcatStrategy.STREAM_COPY.name, + ), + ) as JoinState.Joined + + assertEquals(ConcatStrategy.STREAM_COPY, joined.strategy) + } + + @Test + fun `an unknown strategy name is read as a re-encode rather than thrown`() { + // The defect. A build that added a third strategy leaves finished joins in the queue naming + // it, and WorkManager keeps those about a week -- the premise WorkerEnumFallbackTest and + // JobTags are both written on. With `valueOf` this throws IllegalArgumentException inside a + // viewModelScope collect that has no handler, so it is not a Failed state, it is a crash. + // + // REENCODE rather than STREAM_COPY because it is the conservative answer: describing an + // unknown join as lossless would be a claim the app cannot support. + val joined = map( + WorkInfo.State.SUCCEEDED, + data = workDataOf( + ConcatWorker.KEY_OUTPUT_PATH to "/cache/conversions/joined.mp4", + ConcatWorker.KEY_STRATEGY to "SMART_CONCAT_V2", + ), + ) as JoinState.Joined + + assertEquals(ConcatStrategy.REENCODE, joined.strategy) + } + + @Test + fun `a success with no strategy at all falls back the same way`() { + val joined = map( + WorkInfo.State.SUCCEEDED, + data = workDataOf(ConcatWorker.KEY_OUTPUT_PATH to "/cache/conversions/joined.mp4"), + ) as JoinState.Joined + + assertEquals(ConcatStrategy.REENCODE, joined.strategy) + } + + @Test + fun `a success from older work falls back to the format such a job really used`() { + val joined = map( + WorkInfo.State.SUCCEEDED, + data = workDataOf(ConcatWorker.KEY_OUTPUT_PATH to "/cache/conversions/joined.mp4"), + ) as JoinState.Joined + + assertEquals(ConcatWorker.outputNameFor(ConcatWorker.DEFAULT_FORMAT), joined.suggestedName) + assertEquals(ConcatWorker.DEFAULT_FORMAT.mimeType, joined.mimeType) + } + + @Test + fun `a blank name or type falls back the same way a missing one does`() { + val joined = map( + WorkInfo.State.SUCCEEDED, + data = workDataOf( + ConcatWorker.KEY_OUTPUT_PATH to "/cache/conversions/joined.mp4", + ConcatWorker.KEY_SUGGESTED_NAME to "", + ConcatWorker.KEY_MIME_TYPE to " ", + ), + ) as JoinState.Joined + + assertEquals(ConcatWorker.outputNameFor(ConcatWorker.DEFAULT_FORMAT), joined.suggestedName) + assertEquals(ConcatWorker.DEFAULT_FORMAT.mimeType, joined.mimeType) + } + + @Test + fun `a failure carries the reason the worker gave`() { + assertEquals( + JoinState.Failed("Not enough free space to join these files."), + map( + WorkInfo.State.FAILED, + data = workDataOf( + ConcatWorker.KEY_ERROR to "Not enough free space to join these files.", + ), + ), + ) + } + + @Test + fun `a failure with nothing said still says something`() { + assertEquals( + JoinState.Failed(ConcatWorker.GENERIC_FAILURE_MESSAGE), + map(WorkInfo.State.FAILED, data = Data.EMPTY), + ) + } + + @Test + fun `a failure whose message is blank falls back like a missing one`() { + assertEquals( + JoinState.Failed(ConcatWorker.GENERIC_FAILURE_MESSAGE), + map(WorkInfo.State.FAILED, data = workDataOf(ConcatWorker.KEY_ERROR to " ")), + ) + } + + @Test + fun `a cancellation lands wherever the caller said it should`() { + // A join started here goes back to Ready with the picked files; one picked up by reattach + // goes to Idle, because those URIs belong to a process that no longer exists. + assertEquals( + JoinState.Ready(INPUTS), + map(WorkInfo.State.CANCELLED, cancelled = JoinState.Ready(INPUTS)), + ) + assertEquals(JoinState.Idle, map(WorkInfo.State.CANCELLED, cancelled = JoinState.Idle)) + } + + private fun map( + state: WorkInfo.State, + runAttemptCount: Int = 0, + data: Data = Data.EMPTY, + cancelled: JoinState = JoinState.Ready(INPUTS), + ): JoinState = joinStateFrom( + JoinUpdate(state = state, runAttemptCount = runAttemptCount, outputData = data), + inputs = INPUTS, + cancelled = cancelled, + ) + + private companion object { + val INPUTS = listOf( + InputFile(Uri.parse("content://test/a.mp4"), "a.mp4", 1024L), + InputFile(Uri.parse("content://test/b.mp4"), "b.mp4", 2048L), + ) + } +} -- 2.47.3