From 4e88de30451c16cf0de6bdfd105b362e9bd4f2e4 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Wed, 2 Sep 2026 19:09:49 -0500 Subject: [PATCH] Stop a permission answer starting a second conversion (#202) currentInput() answered for Converting, Waiting and Converted as well as Ready. Those three arms were unreachable by tapping Convert -- the button renders only in the Ready branch -- but they were reachable through the POST_NOTIFICATIONS *result*, which ConverterScreen.kt:91 wires to convert() rather than to the button. Reaching one enqueued a SECOND job over a live one: activeWorkId was overwritten, and the first job kept running with its foreground notification orphaned and nothing left holding its id to cancel it. #202 decided to narrow rather than to test it as it stood, because a test written against the old shape would have frozen the double-enqueue as intended behaviour -- the F1/F5 failure mode docs/coverage-read-findings.md names. currentInput() is now (_state.value as? ConversionState.Ready)?.input, which is what JoinViewModel.join() has been all along; the two screens are the same shape and only one of them was over-general. Four cold refusal arms come with it, all reached the same way -- a system callback arriving after the screen has moved on, which is what a result redelivered after process death does: ConversionViewModel.kt:513 currentInput() ?: return ConversionViewModel.kt:600 pendingSave() ?: return JoinViewModel.kt:316 (as? Ready)?.inputs ?: return JoinViewModel.kt:390 pendingSave() ?: return One fixture note worth keeping: the second case needs a real staged file in the worker's output Data. A SUCCEEDED job with no output path maps to Failed rather than Converted, so Data.EMPTY never reaches the state the case is about -- which cost a timed-out awaitState before it was spotted. Mutations, all run and restored: restore the over-general four-arm when 1 red <- the defect this change fixes currentInput()!! at :513 2 red pendingSave()!! in save() 1 red drop both join guards 1 red Co-Authored-By: Claude Opus 5 (1M context) --- .../convert/ConversionViewModel.kt | 24 ++- .../convert/StaleLauncherResultTest.kt | 147 ++++++++++++++++++ 2 files changed, 164 insertions(+), 7 deletions(-) create mode 100644 app/src/test/java/org/libremediaconverter/convert/StaleLauncherResultTest.kt diff --git a/app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt b/app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt index 4a0204b..ac34ce4 100644 --- a/app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt +++ b/app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt @@ -673,13 +673,23 @@ class ConversionViewModel @JvmOverloads constructor( else -> null } - private fun currentInput(): InputFile? = when (val s = _state.value) { - is ConversionState.Ready -> s.input - is ConversionState.Converting -> s.input - is ConversionState.Waiting -> s.input - is ConversionState.Converted -> s.input - else -> null - } + /** + * The input `convert()` may act on, which is only ever the one on a `Ready` screen. + * + * This used to answer for `Converting`, `Waiting` and `Converted` as well. Those arms were not + * reachable by tapping Convert -- the button renders only in the `Ready` branch -- but they + * were reachable through the POST_NOTIFICATIONS **result**, which `ConverterScreen.kt:91` wires + * to `convert()` rather than to the button. Reaching one of them enqueued a *second* job over a + * live one: `activeWorkId` was overwritten, and the first job kept running with its foreground + * notification orphaned and nothing left holding its id to cancel it. + * + * Narrowed under #202 rather than tested as it stood, because a test written against the old + * shape would have frozen the double-enqueue as intended behaviour -- the F1/F5 failure mode. + * + * `JoinViewModel.join()` has been `(_state.value as? JoinState.Ready)?.inputs ?: return` all + * along. The two screens are the same shape and only one of them was over-general. + */ + private fun currentInput(): InputFile? = (_state.value as? ConversionState.Ready)?.input private companion object { /** diff --git a/app/src/test/java/org/libremediaconverter/convert/StaleLauncherResultTest.kt b/app/src/test/java/org/libremediaconverter/convert/StaleLauncherResultTest.kt new file mode 100644 index 0000000..6ff5eef --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/StaleLauncherResultTest.kt @@ -0,0 +1,147 @@ +package org.libremediaconverter.convert + +import android.app.Application +import android.net.Uri +import androidx.media3.common.util.UnstableApi +import androidx.work.WorkManager +import androidx.work.workDataOf +import kotlinx.coroutines.Dispatchers +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.join.JoinState +import org.libremediaconverter.join.JoinViewModel +import org.libremediaconverter.model.InputProbe +import org.libremediaconverter.work.ConcatWorker +import org.libremediaconverter.work.ConversionWorker +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment + +/** + * An answer that arrives after the screen has moved on does nothing. + * + * Four refusal arms, cold before this file: + * + * ``` + * convert/ConversionViewModel.kt:513 currentInput() ?: return + * convert/ConversionViewModel.kt:600 pendingSave() ?: return + * join/JoinViewModel.kt:316 (as? Ready)?.inputs ?: return + * join/JoinViewModel.kt:390 pendingSave() ?: return + * ``` + * + * They are not merely defensive. `ConverterScreen.kt:91` wires `convert()` to the + * **POST_NOTIFICATIONS result**, and `:83` wires `save()` to the CreateDocument result — so both + * are entered by a system callback rather than by a tap, and a result redelivered after process + * death arrives at a brand-new ViewModel sitting on `Idle`. + * + * ## The production change that came with this + * + * `currentInput()` used to answer for `Converting`, `Waiting` and `Converted` as well as `Ready`. + * Those arms were unreachable by tapping Convert but reachable through that permission callback, + * and reaching one enqueued a **second** job over a live one — `activeWorkId` overwritten, the + * first job still running with an orphaned notification and nothing holding its id. + * + * #202 decided to narrow rather than to test it as it stood, because a test written against the old + * shape would have frozen the double-enqueue as intended behaviour. `JoinViewModel.join()` has been + * `(_state.value as? JoinState.Ready)?.inputs ?: return` all along; the two screens are the same + * shape and only one was over-general. + */ +@UnstableApi +@RunWith(RobolectricTestRunner::class) +class StaleLauncherResultTest { + + private lateinit var app: Application + private lateinit var workManager: WorkManager + private lateinit var staged: java.io.File + + @Before + fun setUp() { + app = RuntimeEnvironment.getApplication() + val publisher = RecordingPublisher(app) + ConversionDependencies.publisher = { publisher } + ConversionDependencies.probe = { _, _ -> InputProbe() } + // A real staged file, because a SUCCEEDED job with no output path maps to Failed rather + // than Converted -- and Converted is the state this file's second case has to reach. + staged = publisher.createStagingFile("holiday.mp4").apply { writeBytes(ByteArray(4096)) } + installTestWorkManager( + app, + workDataOf( + ConversionWorker.KEY_OUTPUT_PATH to staged.absolutePath, + ConversionWorker.KEY_SUGGESTED_NAME to "holiday.mp4", + ConversionWorker.KEY_MIME_TYPE to "video/mp4", + ), + ) + workManager = WorkManager.getInstance(app) + } + + @After + fun tearDown() = ConversionDependencies.reset() + + @Test + fun `a permission answer arriving on an empty screen enqueues nothing`() { + val viewModel = ConversionViewModel(app, Dispatchers.Unconfined) + awaitState(viewModel.state, "Idle") { it is ConversionState.Idle } + + viewModel.convert() + + assertEquals(ConversionState.Idle, viewModel.state.value) + assertEquals("nothing may be enqueued for a file that is not there", 0, conversionJobs()) + } + + /** + * The narrowing itself: a permission answer that arrives while a conversion is already running + * must not start a second one. + * + * Reached by converting once — the synchronous test WorkManager finishes it inline, so the + * screen is `Converted`, which is one of the three arms `currentInput()` used to answer for. + * Calling `convert()` again from there is precisely what the permission callback can do. + */ + @Test + fun `a permission answer arriving after the job finished does not start a second one`() { + val viewModel = ConversionViewModel(app, Dispatchers.Unconfined) + viewModel.onInputPicked(Uri.parse("content://test/holiday.mkv")) + awaitState(viewModel.state, "Ready") { it is ConversionState.Ready } + viewModel.convert() + val converted = awaitState(viewModel.state, "Converted") { it is ConversionState.Converted } + assertEquals("the fixture needs exactly one job to start with", 1, conversionJobs()) + + viewModel.convert() + + assertEquals("a second job must not be enqueued over the first", 1, conversionJobs()) + assertEquals("and the screen must not move", converted, viewModel.state.value) + } + + @Test + fun `a save answer arriving on an empty screen does nothing`() { + val viewModel = ConversionViewModel(app, Dispatchers.Unconfined) + awaitState(viewModel.state, "Idle") { it is ConversionState.Idle } + + viewModel.save(DESTINATION) + + assertEquals(ConversionState.Idle, viewModel.state.value) + } + + @Test + fun `a join answer arriving on an empty screen enqueues nothing`() { + val viewModel = JoinViewModel(app, Dispatchers.Unconfined) + awaitState(viewModel.state, "Idle") { it is JoinState.Idle } + + viewModel.join() + viewModel.save(DESTINATION) + + assertEquals(JoinState.Idle, viewModel.state.value) + assertEquals(0, joinJobs()) + } + + private fun conversionJobs() = jobsTagged(ConversionWorker::class.java.name) + + private fun joinJobs() = jobsTagged(ConcatWorker::class.java.name) + + private fun jobsTagged(tag: String) = workManager.getWorkInfosByTag(tag).get().size + + private companion object { + val DESTINATION: Uri = Uri.parse("content://test/destination.mp4") + } +}