diff --git a/.gitignore b/.gitignore index b95fa12..43ab9aa 100644 --- a/.gitignore +++ b/.gitignore @@ -17,3 +17,10 @@ local.properties # The FFmpeg AAR is committed under bin/ so test runs do not depend on a rebuild. # Build outputs from tools/ffmpeg are not. tools/ffmpeg/out/ + +# Claude Code's per-machine state. Named file by file rather than ignoring .claude/, so that +# shared project config -- settings.json, agents/, skills/ -- stays committable if this project +# ever adopts it. worktrees/ holds complete working copies: during a parallel agent run there +# were six, each a full checkout with its own build output. +.claude/worktrees/ +.claude/scheduled_tasks.lock diff --git a/README.md b/README.md index fbd72fd..6f1ae20 100644 --- a/README.md +++ b/README.md @@ -108,7 +108,13 @@ restored after a restart. ## Building -Requires JDK 17+ (AGP 9 will not run on older) and the Android SDK with API 37. +Requires the Android SDK with API 37. **Do not pick a JDK** — the repo does. +`gradle/gradle-daemon-jvm.properties` pins the daemon to Java 25 and carries foojay +download URLs per platform, so Gradle finds an installed Java 25 or downloads one on the +first build, whatever `JAVA_HOME` points at. `JAVA_HOME` only chooses the *launcher*, which +Gradle 9.7.1 will run on Java 8 or newer. Everything the build actually compiles is Java 25, +the app's own bytecode included. `./gradlew --version` prints the launcher and the daemon +separately, and they routinely differ. FFmpeg is committed as a prebuilt archive under [`bin/`](bin/README.md), so a clone builds without a cross-compile. That is deliberate: rebuilding it per CI run made test diff --git a/app/build.gradle.kts b/app/build.gradle.kts index c5ab3ed..66bedbe 100644 --- a/app/build.gradle.kts +++ b/app/build.gradle.kts @@ -102,6 +102,23 @@ android { informational += "UsableSpace" } + testOptions { + unitTests { + // Robolectric needs the merged manifest and the compiled resource table to build + // an Android runtime on the JVM. Without this, AGP hands the unit tests a stub + // android.jar with no resources and Robolectric cannot start. + isIncludeAndroidResources = true + + all { + // Robolectric's native runtime calls System.load(), which Java 25 reports as + // a restricted method -- four lines of warning on every test run, and a hard + // failure in some later JDK. Granting it explicitly says the native access is + // known and wanted rather than leaving the JVM to guess. + it.jvmArgs("--enable-native-access=ALL-UNNAMED") + } + } + } + packaging { jniLibs { // Uncompressed .so, so the APK zip-aligns them on 16 KB boundaries. @@ -263,6 +280,32 @@ dependencies { debugImplementation(libs.compose.ui.tooling) testImplementation(libs.junit) + // An Android runtime on the JVM. Everything else in src/test is a pure function; this is + // here for the one thing a pure function cannot assert -- that a staged file is really + // gone from a real cacheDir. Instrumented tests do not run on the development host, so + // without it that assertion could only be written where nobody can execute it. + testImplementation(libs.robolectric) + // Already in the catalog for androidTest, and already inside the prerelease guard via its + // androidx. group. WorkManagerTestInitHelper + SynchronousExecutor are what let a JVM test + // drive a ViewModel through a real WorkManager to SUCCEEDED, which is where the cleanup + // handle is set -- the wiring the leak actually lived in. + testImplementation(libs.androidx.work.testing) + // Compose's own test rules, on the JVM source set as well as androidTest. Already in the + // catalog, already inside the prerelease guard via its androidx. group, and versioned by + // the BOM, so this adds no new pinning argument. + // + // Here rather than only in androidTest because ui-test-junit4 runs under Robolectric: + // createComposeRule() drives a real composition on the JVM. The defect it was added for + // -- the selected tab not surviving recreation -- is caught by StateRestorationTester, + // and putting that test where the instrumented suite lives would mean nobody on this + // host could ever watch it go red. + // + // ui-test-manifest is deliberately NOT repeated here. It supplies the ComponentActivity + // the rule launches, and the debugImplementation entry below already puts it in the + // merged manifest the unit tests build against -- checked by removing it and watching + // the tests stay green. + testImplementation(platform(libs.compose.bom)) + testImplementation(libs.compose.ui.test.junit4) androidTestImplementation(platform(libs.compose.bom)) androidTestImplementation(libs.androidx.junit) diff --git a/app/src/androidTest/java/org/libremediaconverter/convert/ReattachOnLaunchTest.kt b/app/src/androidTest/java/org/libremediaconverter/convert/ReattachOnLaunchTest.kt new file mode 100644 index 0000000..6545315 --- /dev/null +++ b/app/src/androidTest/java/org/libremediaconverter/convert/ReattachOnLaunchTest.kt @@ -0,0 +1,330 @@ +package org.libremediaconverter.convert + +import android.app.Application +import android.content.Context +import android.net.Uri +import androidx.media3.common.util.UnstableApi +import androidx.test.ext.junit.runners.AndroidJUnit4 +import androidx.test.platform.app.InstrumentationRegistry +import androidx.work.Data +import androidx.work.OneTimeWorkRequestBuilder +import androidx.work.WorkInfo +import androidx.work.WorkManager +import androidx.work.Worker +import androidx.work.WorkerParameters +import androidx.work.workDataOf +import kotlinx.coroutines.flow.first +import kotlinx.coroutines.runBlocking +import kotlinx.coroutines.withTimeout +import kotlinx.coroutines.withTimeoutOrNull +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +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.ConcatStrategy +import org.libremediaconverter.model.Engine +import org.libremediaconverter.work.ConcatWorker +import org.libremediaconverter.work.ConversionWorker +import org.libremediaconverter.work.JobTags +import java.io.File +import java.util.UUID +import java.util.concurrent.TimeUnit + +/** + * Stands in for a worker whose job is already over. + * + * Reattachment is defined entirely by what WorkManager can hand back — the worker class name + * as a tag, the tags the request carried, and the output `Data` — so a job with that shape is + * all the ViewModel needs to see. Producing one by running a real transcode would take minutes + * and would test the engines, which have their own suites. This echoes its input as its result, + * which lets a test state any finished job in a line. + */ +class EchoWorker(context: Context, params: WorkerParameters) : Worker(context, params) { + override fun doWork(): Result = Result.success(inputData) +} + +/** + * The defect: a conversion that outlives the process becomes unreachable. + * + * WorkManager's queue survives process death — that is why the app uses it — but the ViewModel + * held its job id in a plain field, so the next launch started at Idle while the finished output + * sat in `cacheDir` with no route to it from the UI. The realistic window is after the transcode + * finishes and before the user taps Save: the foreground service is gone and the process is an + * ordinary background one that may be reclaimed hours before the user comes back. + * + * A ViewModel constructed here *is* that next launch: it is a fresh instance with no memory of + * the work, exactly as after `am kill`. The real [WorkManager] is used rather than + * `WorkManagerTestInitHelper`, whose `setDelegate` replaces the singleton for the whole process + * and would silently turn `ConversionWorkerTest` — which exists to exercise the real WorkManager + * path, foreground service included — into a synchronous test double, depending on class order. + */ +@UnstableApi +@RunWith(AndroidJUnit4::class) +class ReattachOnLaunchTest { + + private val app = InstrumentationRegistry.getInstrumentation() + .targetContext.applicationContext as Application + private val workManager = WorkManager.getInstance(app) + + @Before + fun clearTheQueue() = emptyQueueAndStaging() + + @After + fun leaveNothingBehind() = emptyQueueAndStaging() + + /** + * The claim the whole fix rests on, checked against the production request builder rather + * than assumed: `WorkRequest.Builder` seeds every request's tags with its worker class name, + * so the app's own work is findable with nothing persisted anywhere. + */ + @Test + fun aConversionRequestIsFindableByItsWorkerClassName() { + val request = ConversionWorker.request( + // A file that does not exist, so the job fails within seconds instead of transcoding. + // What is under test is the request's tags, which are written when it is enqueued. + inputUri = Uri.fromFile(File(app.cacheDir, "no_such_input.mp4")), + displayName = "holiday.mp4", + sizeBytes = 4_096L, + ) + workManager.enqueue(request).result.get() + workManager.cancelWorkById(request.id).result.get() + val info = awaitFinished(request.id) + + assertTrue( + "no worker class name in ${info.tags}", + info.tags.contains(ConversionWorker::class.java.name), + ) + assertEquals("holiday.mp4", JobTags.displayNameOf(info.tags)) + assertEquals(4_096L, JobTags.sizeBytesOf(info.tags)) + } + + @Test + fun reattachesToAConversionThatFinishedWhileTheViewModelWasGone() { + val staged = stage("holiday_converted.mp4") + finishedJob( + tags = listOf( + ConversionWorker::class.java.name, + JobTags.displayName("holiday.mp4"), + JobTags.sizeBytes(4_096L), + ), + output = workDataOf( + ConversionWorker.KEY_OUTPUT_PATH to staged.absolutePath, + ConversionWorker.KEY_ENGINE_USED to Engine.FFMPEG.name, + ConversionWorker.KEY_ROUTE_REASON to "test route", + ), + ) + + val converted = awaitConversion() + + assertEquals(staged.absolutePath, converted.staged.absolutePath) + assertEquals("holiday.mp4", converted.input.displayName) + assertEquals(4_096L, converted.input.sizeBytes) + assertEquals(Engine.FFMPEG.name, converted.engineUsed) + } + + /** + * The Save button has to be reachable *and* mean something. A staged file the OS reclaimed + * out of the cache — or one a previous save already published and deleted — would otherwise + * be offered and fail on tap. + */ + @Test + fun ignoresAFinishedConversionWhoseStagedFileIsGone() { + val missing = File(File(app.cacheDir, "conversions"), "vanished_converted.mp4") + missing.delete() + finishedJob( + tags = listOf(ConversionWorker::class.java.name, JobTags.displayName("vanished.mp4")), + output = workDataOf(ConversionWorker.KEY_OUTPUT_PATH to missing.absolutePath), + ) + + assertStaysIdle(conversionViewModel()) + } + + /** + * The shape a device actually produced: two SUCCEEDED jobs reporting the same output path, + * with one file on disk, because the staging name is derived from the input's display name. + * The file has to stay reachable — losing it is the defect — while the card must not claim + * an input that may belong to the other job. + */ + @Test + fun offersAFileTwoJobsClaimWithoutAttributingItToEither() { + val staged = stage("input_converted.mp4") + val output = workDataOf(ConversionWorker.KEY_OUTPUT_PATH to staged.absolutePath) + finishedJob( + tags = listOf(ConversionWorker::class.java.name, JobTags.displayName("input.mp4")), + output = output, + ) + finishedJob( + tags = listOf(ConversionWorker::class.java.name, JobTags.displayName("input.mkv")), + output = output, + ) + + val converted = awaitConversion() + + assertEquals(staged.absolutePath, converted.staged.absolutePath) + assertTrue( + "attributed an aliased file to one of the jobs: ${converted.input.displayName}", + converted.input.displayName !in setOf("input.mp4", "input.mkv"), + ) + } + + @Test + fun reattachesToAConversionStillWaitingInTheQueue() { + queuedJob( + tags = listOf( + ConversionWorker::class.java.name, + JobTags.displayName("queued.mp4"), + JobTags.sizeBytes(2_048L), + ), + ) + + val converting = awaitConversion() + + assertEquals("queued.mp4", converting.input.displayName) + assertEquals(2_048L, converting.input.sizeBytes) + } + + /** + * The pair with the test above: same job, same tags, and the only difference is that the + * user cancelled it. Reattaching to it would undo their decision. + */ + @Test + fun doesNotResurrectAConversionTheUserCancelled() { + val id = queuedJob( + tags = listOf(ConversionWorker::class.java.name, JobTags.displayName("queued.mp4")), + ) + workManager.cancelWorkById(id).result.get() + assertEquals(WorkInfo.State.CANCELLED, awaitFinished(id).state) + + assertStaysIdle(conversionViewModel()) + } + + /** A pick the user has already made owns the screen; a job found afterwards must not take it. */ + @Test + fun doesNotOverwriteAPickTheUserHasAlreadyMade() { + val staged = stage("holiday_converted.mp4") + finishedJob( + tags = listOf(ConversionWorker::class.java.name, JobTags.displayName("holiday.mp4")), + output = workDataOf(ConversionWorker.KEY_OUTPUT_PATH to staged.absolutePath), + ) + + val picked = Uri.fromFile(stage("picked.mp4")) + val viewModel = conversionViewModel() + onMainThread { viewModel.onInputPicked(picked) } + + runBlocking { + val ready = withTimeout(TIMEOUT_MS) { + viewModel.state.first { it is ConversionState.Ready } + } as ConversionState.Ready + assertEquals(picked, ready.input.uri) + // And it stays the user's pick rather than being replaced a moment later. + val stolen = withTimeoutOrNull(SETTLE_MS) { + viewModel.state.first { it !is ConversionState.Ready } + } + assertNull("reattachment took the screen from the user: $stolen", stolen) + } + } + + @Test + fun reattachesToAJoinThatFinishedWhileTheViewModelWasGone() { + val staged = stage("joined.mp4") + finishedJob( + tags = listOf(ConcatWorker::class.java.name, JobTags.inputCount(3)), + output = workDataOf( + ConcatWorker.KEY_OUTPUT_PATH to staged.absolutePath, + ConcatWorker.KEY_STRATEGY to ConcatStrategy.STREAM_COPY.name, + ), + ) + + val viewModel = joinViewModel() + val joined = runBlocking { + withTimeout(TIMEOUT_MS) { viewModel.state.first { it is JoinState.Joined } } + } as JoinState.Joined + + assertEquals(staged.absolutePath, joined.staged.absolutePath) + assertEquals(ConcatStrategy.STREAM_COPY, joined.strategy) + } + + // --- staging the situation -------------------------------------------------------------- + + /** Enqueues a job that runs immediately and finishes with [output] as its result. */ + private fun finishedJob(tags: List, output: Data): UUID { + val builder = OneTimeWorkRequestBuilder().setInputData(output) + tags.forEach(builder::addTag) + val request = builder.build() + workManager.enqueue(request).result.get() + assertEquals(WorkInfo.State.SUCCEEDED, awaitFinished(request.id).state) + return request.id + } + + /** + * Enqueues a job that stays [WorkInfo.State.ENQUEUED]. The delay is what holds it there: it + * is long enough that nothing can run it during a test, and it is cancelled either way. + */ + private fun queuedJob(tags: List): UUID { + val builder = OneTimeWorkRequestBuilder() + .setInitialDelay(1, TimeUnit.HOURS) + tags.forEach(builder::addTag) + val request = builder.build() + workManager.enqueue(request).result.get() + return request.id + } + + private fun stage(name: String): File { + val dir = File(app.cacheDir, "conversions").apply { mkdirs() } + return File(dir, name).apply { writeBytes(ByteArray(1_024)) } + } + + private fun emptyQueueAndStaging() { + workManager.cancelAllWorkByTag(ConversionWorker::class.java.name).result.get() + workManager.cancelAllWorkByTag(ConcatWorker::class.java.name).result.get() + workManager.cancelAllWorkByTag(EchoWorker::class.java.name).result.get() + workManager.pruneWork().result.get() + File(app.cacheDir, "conversions").listFiles()?.forEach { it.delete() } + } + + // --- reading the result ----------------------------------------------------------------- + + /** A ViewModel built now is the next launch: no memory of the work, only what it can query. */ + private fun conversionViewModel(): ConversionViewModel = onMainThread { ConversionViewModel(app) } + + private fun joinViewModel(): JoinViewModel = onMainThread { JoinViewModel(app) } + + private inline fun awaitConversion(): T = runBlocking { + val viewModel = conversionViewModel() + withTimeout(TIMEOUT_MS) { viewModel.state.first { it is T } } as T + } + + private fun assertStaysIdle(viewModel: ConversionViewModel) = runBlocking { + val moved = withTimeoutOrNull(SETTLE_MS) { + viewModel.state.first { it !is ConversionState.Idle } + } + assertNull("reattached to work it should have left alone: $moved", moved) + } + + private fun awaitFinished(id: UUID): WorkInfo = runBlocking { + withTimeout(TIMEOUT_MS) { + workManager.getWorkInfoByIdFlow(id).first { it != null && it.state.isFinished } + }!! + } + + private fun onMainThread(block: () -> T): T { + lateinit var result: T + InstrumentationRegistry.getInstrumentation().runOnMainSync { result = block() } + return result + } + + private companion object { + const val TIMEOUT_MS = 30_000L + + /** + * How long "nothing happened" is given to happen. Reattachment is one indexed query + * against WorkManager's database, so this is generous rather than tuned. + */ + const val SETTLE_MS = 5_000L + } +} diff --git a/app/src/androidTest/java/org/libremediaconverter/join/ConcatEngineTest.kt b/app/src/androidTest/java/org/libremediaconverter/join/ConcatEngineTest.kt index 745b8aa..4a7d6cb 100644 --- a/app/src/androidTest/java/org/libremediaconverter/join/ConcatEngineTest.kt +++ b/app/src/androidTest/java/org/libremediaconverter/join/ConcatEngineTest.kt @@ -13,6 +13,7 @@ import org.junit.Before import org.junit.Test import org.junit.runner.RunWith import org.libremediaconverter.convert.MediaProbe +import org.libremediaconverter.convert.StagingNames import org.libremediaconverter.ffmpeg.ConcatEngine import org.libremediaconverter.model.ConcatStrategy import java.io.File @@ -152,9 +153,12 @@ class ConcatEngineTest { fun theListFileIsCleanedUpAfterJoining(): Unit = runBlocking { val out = output("joined_cleanup.mp4") engine.join(listOf(Uri.fromFile(clipA), Uri.fromFile(clipB)), out) + // Asked of StagingNames rather than spelled out: the list file used to be the constant + // concat_list.txt, and a literal here would have gone on passing vacuously once the name + // moved -- it would be asserting that a file nothing creates does not exist. assertTrue( "the concat list file was left behind", - !File(out.parentFile, "concat_list.txt").exists(), + !File(out.parentFile, StagingNames.concatListFor(out.name)).exists(), ) } diff --git a/app/src/main/AndroidManifest.xml b/app/src/main/AndroidManifest.xml index 983c19d..c77fe2f 100644 --- a/app/src/main/AndroidManifest.xml +++ b/app/src/main/AndroidManifest.xml @@ -20,10 +20,15 @@ off by default on new installs. --> + /conversions/` once per + * process. + * + * Every other cleanup path in the app depends on a ViewModel still being alive to run it. + * The cases that leak are exactly the ones where it is not — the process is reclaimed + * between a conversion finishing and the user saving it, a worker fails before its output + * ever becomes a `Converted` state, or a `reset()`'s delete is cancelled along with the + * Activity. Process start is the one moment those leftovers are reliably observable. + */ +class LibreMediaConverterApp : Application() { + + /** + * Deliberately process-lifetime and never cancelled: the work it carries is a single + * short task that should outlive nothing in particular and be interrupted by nothing. + * A `SupervisorJob` so a failure here could never take a sibling down with it. + */ + private val appScope = CoroutineScope(SupervisorJob() + Dispatchers.IO) + + override fun onCreate() { + super.onCreate() + + // Off the main thread: this lists a directory and stats each entry, and it runs on + // the path that decides how long the launcher icon stays unresponsive. + // + // Why this cannot race a live job -- and note the argument is NOT about ordering. + // WorkManager initialises through androidx.startup's InitializationProvider, which + // is a ContentProvider, so it is already up before onCreate() is called and can be + // resuming a worker on its own executor while this runs. Workers run in this same + // process, so "nothing has started yet" would simply be false. + // + // The grace period is what makes it safe. StagingSweep only collects a file nothing + // has written to for a full day: + // + // - Conversion and join outputs are written continuously, so a running job keeps + // its own mtime fresh and never looks abandoned. + // - A join's list file is the one written once and then only read, so it is the + // one that has to be reasoned about rather than observed. A WorkManager attempt + // is capped by the six-hour-per-day foreground-service budget and a retry starts + // doWork() again from the top, rewriting the list file -- so no single attempt + // can hold a file untouched for twenty-four hours. + // - A worker resuming right now writes its files at attempt start, which makes + // them zero seconds old, not a day. + // + // sweepStaging() also re-reads each timestamp immediately before deleting, which + // closes the window between listing the directory and acting on the listing. + appScope.launch { OutputPublisher(this@LibreMediaConverterApp).sweepStaging() } + } +} diff --git a/app/src/main/java/org/libremediaconverter/MainActivity.kt b/app/src/main/java/org/libremediaconverter/MainActivity.kt index 6fd219f..b03f7c1 100644 --- a/app/src/main/java/org/libremediaconverter/MainActivity.kt +++ b/app/src/main/java/org/libremediaconverter/MainActivity.kt @@ -20,7 +20,8 @@ import androidx.compose.material3.windowsizeclass.calculateWindowSizeClass import androidx.compose.runtime.Composable import androidx.compose.runtime.getValue import androidx.compose.runtime.mutableStateOf -import androidx.compose.runtime.remember +import androidx.compose.runtime.saveable.Saver +import androidx.compose.runtime.saveable.rememberSaveable import androidx.compose.runtime.setValue import androidx.compose.ui.Modifier import androidx.media3.common.util.UnstableApi @@ -28,11 +29,41 @@ import org.libremediaconverter.convert.ConverterScreen import org.libremediaconverter.join.JoinScreen import org.libremediaconverter.ui.theme.LibreMediaConverterTheme -private enum class Destination(val label: String) { +/** + * The tabs of the adaptive shell. + * + * `internal` rather than `private` so the unit tests can name a tab. The JVM test source + * set is a friend of `main`, so this stays invisible to anything outside the module. + */ +internal enum class Destination(val label: String) { CONVERT("Convert"), JOIN("Join"), } +/** + * Saves a [Destination] as its constant name. + * + * A saver is needed at all because `rememberSaveable`'s default only accepts what a + * `Bundle` can hold. An enum does qualify -- it is `Serializable`, so `autoSaver` would take + * it without complaint -- and that is the reason to be explicit rather than the reason not + * to be: nothing in the declaration says this type has to stay `Serializable`, so the + * implicit route would keep working until someone made it a value class or a sealed + * interface, and then quietly stop. + * + * The name and not the ordinal. An ordinal is a position, so inserting a tab between the + * existing two would silently redefine every value already written down; the name only + * changes when someone renames a constant, which is a visible edit. It also reads as itself + * in a `Bundle` dump. + * + * An unknown name restores to null, which `rememberSaveable` treats as "nothing saved" and + * falls back to the default tab. That is the state a downgrade or a renamed constant + * produces, and landing on Convert is the right answer for it. + */ +internal val DestinationSaver: Saver = Saver( + save = { it.name }, + restore = { name -> Destination.entries.firstOrNull { it.name == name } }, +) + @UnstableApi class MainActivity : ComponentActivity() { @@ -57,12 +88,28 @@ class MainActivity : ComponentActivity() { * `resizableActivity` and aspect-ratio limits on any display at least 600dp wide, and * the Android 16 opt-out no longer applies. The app will be resized and rotated * whether or not it is ready, so it has to lay out properly at every width. + * + * [content] is a parameter with a default rather than a direct call to [Content] so a test + * can drive the shell -- which tab is selected, and whether that survives recreation -- + * without standing up either screen. Both screens resolve a ViewModel, which builds a + * WorkManager and a media probe, none of which the tab selection depends on. The app + * itself never passes it. */ @UnstableApi @OptIn(ExperimentalMaterial3Api::class) @Composable -private fun AppRoot(widthSizeClass: WindowWidthSizeClass) { - var destination by remember { mutableStateOf(Destination.CONVERT) } +internal fun AppRoot( + widthSizeClass: WindowWidthSizeClass, + content: @Composable (Destination, Modifier) -> Unit = { destination, modifier -> + Content(destination, modifier) + }, +) { + // rememberSaveable, NOT remember. MainActivity declares no configChanges, so every + // rotation and every resize recreates it -- exactly the case the KDoc above says the + // shell exists for -- and remember does not survive that. + var destination by rememberSaveable(stateSaver = DestinationSaver) { + mutableStateOf(Destination.CONVERT) + } val useRail = widthSizeClass != WindowWidthSizeClass.Compact if (useRail) { @@ -78,7 +125,7 @@ private fun AppRoot(widthSizeClass: WindowWidthSizeClass) { } } Scaffold(modifier = Modifier.fillMaxSize()) { padding -> - Content(destination, Modifier.padding(padding)) + content(destination, Modifier.padding(padding)) } } } else { @@ -97,7 +144,7 @@ private fun AppRoot(widthSizeClass: WindowWidthSizeClass) { } }, ) { padding -> - Content(destination, Modifier.padding(padding)) + content(destination, Modifier.padding(padding)) } } } diff --git a/app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt b/app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt index 4e2ed51..3cc9352 100644 --- a/app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt +++ b/app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt @@ -2,12 +2,13 @@ package org.libremediaconverter.convert import android.app.Application import android.net.Uri -import android.provider.OpenableColumns +import android.util.Log import androidx.lifecycle.AndroidViewModel import androidx.lifecycle.viewModelScope import androidx.media3.common.util.UnstableApi import androidx.work.WorkInfo import androidx.work.WorkManager +import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.Job import kotlinx.coroutines.flow.MutableStateFlow @@ -19,6 +20,7 @@ import kotlinx.coroutines.flow.stateIn import kotlinx.coroutines.flow.update import kotlinx.coroutines.launch import kotlinx.coroutines.withContext +import org.libremediaconverter.ffmpeg.isNativeLoadFailure import org.libremediaconverter.model.AudioCodec import org.libremediaconverter.model.Container import org.libremediaconverter.model.ContainerCapabilities @@ -30,6 +32,9 @@ import org.libremediaconverter.model.QualityTier import org.libremediaconverter.model.Validation import org.libremediaconverter.model.VideoCodec import org.libremediaconverter.work.ConversionWorker +import org.libremediaconverter.work.JobTags +import org.libremediaconverter.work.Reattachment +import org.libremediaconverter.work.jobSnapshots import java.io.File import java.util.UUID @@ -47,7 +52,15 @@ data class ConversionSettings( data class InputFile( val uri: Uri, val displayName: String, - val sizeBytes: Long, + /** + * How big the file is, or null when nothing could say. + * + * Nullable rather than `0L`, and that is the point of it. The two were the same value before, + * so an unmeasurable file arrived at the space check claiming to be empty. [InputQuery] owns + * how the answer is found and what it means; every reader of this has to decide what an + * unknown size does, which is exactly the decision the old default made silently. + */ + val sizeBytes: Long?, /** * What probing found. Null only while the probe is still running. * @@ -63,23 +76,58 @@ sealed interface ConversionState { data class Ready(val input: InputFile) : ConversionState data class Converting(val input: InputFile, val percent: Int) : ConversionState - /** Budget for foreground work ran out; WorkManager will retry when it can. */ + /** + * Something stopped the job from running for now, and WorkManager will try again. + * + * Two causes reach here and the state cannot tell them apart, because `ENQUEUED` with an + * attempt behind it is all `WorkInfo` says: the six-hour-a-day foreground-service budget + * running out mid-job, and the system refusing to let a job restart while the app is in the + * background. See [org.libremediaconverter.work.FailureOutcome]. + */ data class Waiting(val input: InputFile) : ConversionState data class Converted( val input: InputFile, val staged: File, val engineUsed: String = "", val routeReason: String = "", + /** + * What to call the file, and what type to open the save dialog with. + * + * Carried on the state rather than derived when the Save button is tapped, because the + * only thing that knows them is the job — see `ConversionWorker.KEY_SUGGESTED_NAME`. The + * staged file's own name says nothing: it is the job's id. + */ + val suggestedName: String = "", + val mimeType: String = "", ) : ConversionState data class Saved(val displayName: String) : ConversionState data class Failed(val message: String) : ConversionState } @UnstableApi -class ConversionViewModel(app: Application) : AndroidViewModel(app) { +class ConversionViewModel @JvmOverloads constructor( + app: Application, + /** + * Where [reset] runs its delete. + * + * A parameter so a test can make the cleanup run inline and assert on the result. It + * also makes the ordering an explicit choice rather than an accident: the state flips + * to `Idle` synchronously while the delete is dispatched, and naming the dispatcher is + * what says that was decided rather than inherited. + * + * `@JvmOverloads` keeps the single-argument constructor that `viewModel()`'s default + * `AndroidViewModelFactory` looks up reflectively; without it the app would crash on + * the first screen. + */ + private val cleanupDispatcher: CoroutineDispatcher = Dispatchers.IO, +) : AndroidViewModel(app) { private val workManager = WorkManager.getInstance(app) - private val publisher = OutputPublisher(app) + + // Through ConversionDependencies, like the workers, rather than `OutputPublisher(app)` + // direct: the ViewModels were the only place bypassing the seam, which left the + // cleanup wiring impossible to substitute in a test. + private val publisher = ConversionDependencies.publisher(app) private val _state = MutableStateFlow(ConversionState.Idle) val state: StateFlow = _state.asStateFlow() @@ -87,6 +135,16 @@ class ConversionViewModel(app: Application) : AndroidViewModel(app) { private var observer: Job? = null private var activeWorkId: UUID? = null + /** + * The staged output this ViewModel is responsible for deleting. + * + * A field rather than something read back out of [_state], because the state machine + * cannot answer the question on the path that needs it most: a failed [save] lands on + * [ConversionState.Failed], which carries a message and no file at all. By then the + * only remaining reference would have been lost. + */ + private var pendingStaged: File? = null + /** Conversion settings, kept separate from the job state machine. */ private val _settings = MutableStateFlow(ConversionSettings()) val settings: StateFlow = _settings.asStateFlow() @@ -102,6 +160,71 @@ class ConversionViewModel(app: Application) : AndroidViewModel(app) { ContainerCapabilities.validate(settings.spec, state.probe() ?: InputProbe()) }.stateIn(viewModelScope, SharingStarted.Eagerly, Validation.Valid) + init { + reattach() + } + + /** + * Picks up a conversion this ViewModel did not start. + * + * The queue outliving the process is the entire reason [ConversionWorker] exists, but the + * ViewModel used to be where that stopped: its `activeWorkId` is a plain field, so a process + * reclaimed after a conversion finished came back to an empty screen while the output sat in + * `cacheDir` with nothing in the UI able to reach it. The realistic case is not a crash + * mid-transcode — it is the job finishing, the user not saving yet, and the process being + * reclaimed hours later as an ordinary background one. + * + * Nothing is persisted for this. The query is by worker class name, which WorkManager tags + * every request with on its own, so it finds work enqueued by an earlier run of the app — + * and by an earlier *version* of it — which an id saved in a `SavedStateHandle` would not. + * + * The save dialog's suggested name and MIME type used to be built from the current picker, + * which made a reattached job the worst case: its spec was never in these settings at all, so + * a job that converted to MP3 was offered `.mp4`. Both now travel in the job's own output + * `Data` — see [ConversionState.Converted]. + */ + private fun reattach() { + viewModelScope.launch { + val reattachment = Reattachment.choose( + workManager.jobSnapshots( + tag = ConversionWorker::class.java.name, + outputPathKey = ConversionWorker.KEY_OUTPUT_PATH, + ), + ) ?: return@launch + + // The query suspends, so by now the user may have picked a file or started a + // conversion of their own. Either owns the screen; reattaching over it would throw + // away what they just did. Both this check and the assignment below run on the main + // dispatcher with no suspension point between them, so nothing can interleave. + if (_state.value !is ConversionState.Idle || activeWorkId != null) return@launch + + // Only a job that is the sole explanation for its staged file gets to name the input. + // When several jobs report the same file — which staging on the job id has stopped for + // new work, but not for work already in the queue — the file is still the user's, but + // saying which of them produced it would be a guess, so the card falls back to a + // neutral label rather than borrowing the other job's. + val tags = (reattachment as? Reattachment.Certain)?.job?.tags.orEmpty() + val input = InputFile( + // The picked URI is not recoverable — WorkManager gives back a job's tags and + // its output, never the Data it was enqueued with — and nothing in the states + // reattachment produces reads it. The card shows the name and size, which the + // tags carry; a reattached job that is cancelled goes to Idle rather than Ready, + // so this can never reach the Convert button. Leaving the probe unset costs the + // card its source details, and re-probing is what there is no URI for. + uri = Uri.EMPTY, + displayName = JobTags.displayNameOf(tags) ?: UNKNOWN_INPUT_NAME, + // No `?: 0L`. A job tagged before sizes were tagged at all, or one enqueued + // for a file nothing could measure, has no size -- and answering that with + // zero is the same conflation this whole change is about. See [InputQuery]. + sizeBytes = JobTags.sizeBytesOf(tags), + ) + activeWorkId = reattachment.job.id + // No initial state of our own: the flow's first emission carries the job's real + // state, so observe() maps it exactly as it would for a conversion started here. + observe(reattachment.job.id, input, cancelled = ConversionState.Idle) + } + } + fun setPreset(format: OutputFormat) = _settings.update { it.copy(spec = format.spec) } fun setContainer(container: Container) = _settings.update { it.copy(spec = it.spec.copy(container = container)) } @@ -118,13 +241,13 @@ class ConversionViewModel(app: Application) : AndroidViewModel(app) { viewModelScope.launch { // Both the metadata query and the probe touch disk, and the probe spawns FFprobe. // Neither belongs on the main thread. - val file = withContext(Dispatchers.IO) { queryFile(uri) } + val file = withContext(Dispatchers.IO) { InputQuery.describe(getApplication(), uri) } // Show the file as soon as its name and size are known. Probing now runs FFprobe on // every pick, which is a native process spawn, and making the whole screen wait on it // would read as the app having ignored the tap. _state.value = ConversionState.Ready(file) - val probe = withContext(Dispatchers.IO) { MediaProbe.probe(getApplication(), uri) } + val probe = withContext(Dispatchers.IO) { probeOrUnreadable(uri) } // Only fill in the probe if the user has not moved on in the meantime. _state.update { current -> if (current is ConversionState.Ready && current.input.uri == uri) { @@ -136,6 +259,34 @@ class ConversionViewModel(app: Application) : AndroidViewModel(app) { } } + /** + * Probing, with the one failure the pick must survive rather than propagate. + * + * This runs inside `viewModelScope.launch`, which has no exception handler, so anything + * that escapes here abandons the launch — the file card never fills in — and reaches the + * thread's default handler, which on a device takes the process down. Picking a file is + * not a place to crash from. + * + * The one condition that reaches this is FFmpegKit's native library failing to load, + * which arrives as an `Error` rather than an `Exception`; [MediaProbe] handles its own + * FFprobe call now, and this covers the seam and the platform extractor beside it. The + * answer is [MediaProbe.UNREADABLE] — the same value [MediaProbe.probe] returns when + * neither of its probes could read the file, because that is what has happened. + * + * Anything else is rethrown deliberately. An [OutOfMemoryError] here is about this + * process, not about this file, and reporting it as an unreadable video would let the app + * carry on in a state it cannot honour. See + * [org.libremediaconverter.ffmpeg.isNativeLoadFailure] for which is which and why the + * distinction is drawn by a predicate rather than by the catch clause. + */ + private fun probeOrUnreadable(uri: Uri): InputProbe = try { + ConversionDependencies.probe(getApplication(), uri) + } catch (e: Error) { + if (!isNativeLoadFailure(e)) throw e + Log.w(TAG, "Could not probe $uri; reporting it as unreadable.", e) + MediaProbe.UNREADABLE + } + /** * Enqueues the conversion rather than running it inline. * @@ -161,7 +312,13 @@ class ConversionViewModel(app: Application) : AndroidViewModel(app) { observe(request.id, input) } - private fun observe(id: UUID, input: InputFile) { + /** + * @param cancelled where a cancellation lands. For a conversion started here that is the + * picked file, ready to convert again. For one picked up by [reattach] there is no picked + * file — the URI that job holds belongs to a process that no longer exists — so it lands + * on Idle instead, rather than offering a Convert button over a file nothing can open. + */ + private fun observe(id: UUID, input: InputFile, cancelled: ConversionState = ConversionState.Ready(input)) { observer?.cancel() observer = viewModelScope.launch { workManager.getWorkInfoByIdFlow(id).collect { info -> @@ -172,8 +329,11 @@ class ConversionViewModel(app: Application) : AndroidViewModel(app) { info.progress.getInt(ConversionWorker.KEY_PROGRESS, 0), ) - // ENQUEUED after a run means a retry is pending — most likely the - // six-hour foreground budget was exhausted mid-job. + // ENQUEUED after a run means a retry is pending. Either the six-hour + // foreground budget ran out mid-job, or the system refused to let the job + // start again while the app was in the background — the second being the + // likelier of the two, since it needs only a process restart. Nothing here + // can tell them apart, and nothing needs to: the answer is the same. WorkInfo.State.ENQUEUED -> if (info.runAttemptCount > 0) { ConversionState.Waiting(input) @@ -186,23 +346,50 @@ class ConversionViewModel(app: Application) : AndroidViewModel(app) { if (path == null) { ConversionState.Failed("Conversion 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 ConversionState.Converted( input = input, - staged = File(path), + staged = staged, engineUsed = info.outputData .getString(ConversionWorker.KEY_ENGINE_USED).orEmpty(), routeReason = info.outputData .getString(ConversionWorker.KEY_ROUTE_REASON).orEmpty(), + suggestedName = info.outputData + .getString(ConversionWorker.KEY_SUGGESTED_NAME) + ?.takeIf { it.isNotBlank() } + // Work enqueued before the worker reported this carries + // nothing, and WorkManager keeps finished work for about a + // week -- so this branch is ordinary for a few days rather + // than a corner. It is the old derivation, kept because it is + // the same guess the app already made and there is genuinely + // nothing better available for such a job. New work never + // reaches it. + ?: ConversionWorker.outputNameFor( + input.displayName, + _settings.value.spec, + ), + mimeType = info.outputData + .getString(ConversionWorker.KEY_MIME_TYPE) + ?.takeIf { it.isNotBlank() } + ?: _settings.value.spec.mimeType, ) } } + // A worker that dies before it can report anything leaves no output data at + // all — a foreground-service start refused after a process restart is one + // way — 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 -> ConversionState.Failed( info.outputData.getString(ConversionWorker.KEY_ERROR) + ?.takeIf { it.isNotBlank() } ?: "Conversion failed.", ) - WorkInfo.State.CANCELLED -> ConversionState.Ready(input) + WorkInfo.State.CANCELLED -> cancelled WorkInfo.State.BLOCKED -> ConversionState.Converting(input, 0) } } @@ -222,30 +409,42 @@ class ConversionViewModel(app: Application) : AndroidViewModel(app) { converted.staged.delete() } }.onSuccess { - _state.value = ConversionState.Saved( - ConversionWorker.outputNameFor( - converted.input.displayName, - _settings.value.spec, - ), - ) + // publish() already deleted it; nothing left to clean up. + pendingStaged = null + _state.value = ConversionState.Saved(converted.suggestedName) }.onFailure { e -> + // Deliberately NOT cleared. A failed save may mean the staged file is the + // only copy of an hour of transcoding, and the user's destination did not + // receive it -- deleting here would destroy the work to tidy up a cache + // directory. It stays collectable: by a later reset(), or by the sweep once + // it is old enough to be certain nobody is coming back for it. _state.value = ConversionState.Failed(e.message ?: "Could not save the file.") } } } + /** + * Returns to [ConversionState.Idle], deleting anything staged on the way out. + * + * "Start over" on a finished conversion is an ordinary path through the UI, and it used + * to drop the only reference to a full-size file in cache. The delete runs on + * [Dispatchers.IO] because it touches the filesystem, and is fire-and-forget: it is + * cancelled with [viewModelScope] if the Activity finishes first, so it is a best + * effort rather than a guarantee. `OutputPublisher.sweepStaging` is the backstop for + * the times it does not run. + */ fun reset() { observer?.cancel() observer = null activeWorkId = null + val staged = pendingStaged + pendingStaged = null + if (staged != null) { + viewModelScope.launch(cleanupDispatcher) { publisher.discardStaged(staged) } + } _state.value = ConversionState.Idle } - fun suggestedOutputName(): String = ConversionWorker.outputNameFor( - currentInput()?.displayName ?: "output", - _settings.value.spec, - ) - private fun ConversionState.probe(): InputProbe? = when (this) { is ConversionState.Ready -> input.probe is ConversionState.Converting -> input.probe @@ -262,21 +461,14 @@ class ConversionViewModel(app: Application) : AndroidViewModel(app) { else -> null } - private fun queryFile(uri: Uri): InputFile { - var name = "input" - var size = 0L - getApplication().contentResolver - .query(uri, null, null, null, null) - ?.use { cursor -> - if (cursor.moveToFirst()) { - cursor.getColumnIndex(OpenableColumns.DISPLAY_NAME) - .takeIf { it >= 0 } - ?.let { name = cursor.getString(it) ?: name } - cursor.getColumnIndex(OpenableColumns.SIZE) - .takeIf { it >= 0 } - ?.let { size = cursor.getLong(it) } - } - } - return InputFile(uri, name, size) + private companion object { + /** + * Shown for a reattached job whose tags predate them — work enqueued by an earlier + * version of the app. Neutral on purpose: it is a real file of the user's, and calling + * it "unknown" would read as an error rather than as a gap in what survived. + */ + const val UNKNOWN_INPUT_NAME = "Media file" + + const val TAG = "ConversionViewModel" } } diff --git a/app/src/main/java/org/libremediaconverter/convert/ConverterScreen.kt b/app/src/main/java/org/libremediaconverter/convert/ConverterScreen.kt index a68838d..921b348 100644 --- a/app/src/main/java/org/libremediaconverter/convert/ConverterScreen.kt +++ b/app/src/main/java/org/libremediaconverter/convert/ConverterScreen.kt @@ -28,6 +28,7 @@ import androidx.compose.material3.TextButton import androidx.compose.runtime.Composable import androidx.compose.runtime.getValue import androidx.compose.runtime.mutableStateOf +import androidx.compose.runtime.remember import androidx.compose.runtime.saveable.rememberSaveable import androidx.compose.runtime.setValue import androidx.compose.ui.Alignment @@ -66,8 +67,14 @@ fun ConverterScreen(modifier: Modifier = Modifier, viewModel: ConversionViewMode ActivityResultContracts.OpenDocument(), ) { uri -> uri?.let(viewModel::onInputPicked) } + // The contract's MIME type comes from the finished job rather than from the picker as it + // stands: some providers rewrite a document's extension to match it, so an MP3 offered as + // video/webm can arrive with the wrong one. Read straight off the collected state, so this + // recomposes because it depends on that rather than because an unrelated line happens to. + // Remembered against the type so the launcher re-registers only when it actually changes. + val destinationMime = (state as? ConversionState.Converted)?.mimeType ?: settings.spec.mimeType val chooseDestination = rememberLauncherForActivityResult( - ActivityResultContracts.CreateDocument(settings.spec.mimeType), + remember(destinationMime) { ActivityResultContracts.CreateDocument(destinationMime) }, ) { uri -> uri?.let(viewModel::save) } // Requested at the point of use rather than on first launch, so the ask carries its @@ -163,9 +170,15 @@ fun ConverterScreen(modifier: Modifier = Modifier, viewModel: ConversionViewMode is ConversionState.Waiting -> { FileCard(s.input) + // Two different causes land here and the state cannot tell them apart: + // the six-hour-a-day background media budget running out, and the system + // refusing to let a job restart while the app is in the background. The + // old wording named only the first, which is now the less likely of the + // two. "Keeping the app open helps" covers both -- it is literally what + // grants the second one permission to run. Text( - "Paused. The system limits background media processing to " + - "six hours a day, so this will resume automatically.", + "Paused. Android limits background media processing, so this will " + + "resume automatically — keeping the app open helps it along.", style = MaterialTheme.typography.bodyMedium, ) OutlinedButton( @@ -188,7 +201,7 @@ fun ConverterScreen(modifier: Modifier = Modifier, viewModel: ConversionViewMode AssistChip(onClick = {}, label = { Text(s.routeReason) }) } Button( - onClick = { chooseDestination.launch(viewModel.suggestedOutputName()) }, + onClick = { chooseDestination.launch(s.suggestedName) }, modifier = Modifier.fillMaxWidth().height(PrimaryButtonHeight), ) { Text("Save file") } OutlinedButton( @@ -409,7 +422,13 @@ private fun FileCard(input: InputFile) { Card(modifier = Modifier.fillMaxWidth()) { Column(modifier = Modifier.padding(16.dp)) { Text(input.displayName, style = MaterialTheme.typography.titleMedium) - Text(formatBytes(input.sizeBytes), style = MaterialTheme.typography.bodySmall) + // The null is handled here rather than inside formatBytes, because "no provider would + // say" is not a number and a formatter that invented one -- "0 B" -- is the defect + // this card would be showing. It degrades in words, like the codec rows below it. + Text( + input.sizeBytes?.let(::formatBytes) ?: "Size unknown", + style = MaterialTheme.typography.bodySmall, + ) val probe = input.probe if (probe == null) { diff --git a/app/src/main/java/org/libremediaconverter/convert/InputQuery.kt b/app/src/main/java/org/libremediaconverter/convert/InputQuery.kt new file mode 100644 index 0000000..0dd1730 --- /dev/null +++ b/app/src/main/java/org/libremediaconverter/convert/InputQuery.kt @@ -0,0 +1,102 @@ +package org.libremediaconverter.convert + +import android.content.Context +import android.database.Cursor +import android.net.Uri +import android.provider.OpenableColumns +import android.util.Log + +/** + * What the app can find out about a picked file before an engine opens it. + * + * One place rather than two: `queryFile` existed in `ConversionViewModel` and `JoinViewModel` + * byte for byte, so a fix to either was a fix to half the app. + * + * **An unknown size is null here, never zero.** That distinction is the whole point of this file. + * The old code started at `var size = 0L` and only moved off it when a provider answered the + * `OpenableColumns.SIZE` column, so "this file is empty" and "nobody told me how big it is" + * reached `OutputPublisher.hasSpaceFor` as the same number — and `hasSpaceFor(0)` is only "is + * there 128 MB free". Confirmed live on a Pixel 10 Pro XL, where `contentResolver.query` on a + * `file://` URI returns null outright and the default survived untouched: + * `queryFile gave displayName='input' sizeBytes=0`. + * + * So the size is asked for twice, in order: + * + * 1. **What the provider says.** `OpenableColumns.SIZE`, which documents providers *may* omit. + * 2. **What the file itself says.** `openFileDescriptor(uri, "r")` and `statSize`, which needs + * no cooperation from a provider beyond being openable — and the app is going to have to + * open the input anyway, so it is not asking for anything a conversion would not need. This + * is what answers the `file://` case above. + * + * Only when both decline is the answer null, and the callers each say what they do about that. + */ +object InputQuery { + + /** + * Shown when no provider names the file. + * + * Kept exactly as it was — it is what reaches the save dialog as `input_converted.mp4` for a + * job whose input nothing described, and changing it here would rename files for reasons + * unrelated to this fix. + */ + const val FALLBACK_DISPLAY_NAME = "input" + + /** Everything the picker knows about [uri] the moment it is chosen. */ + fun describe(context: Context, uri: Uri): InputFile = InputFile( + uri = uri, + displayName = firstRow(context, uri) { it.displayNameOrNull() } ?: FALLBACK_DISPLAY_NAME, + sizeBytes = sizeOf(context, uri), + ) + + /** + * How many bytes [uri] holds, or null when nothing can say. + * + * Public because the workers need it too, and for a reason worth stating: a worker's input + * `Data` carries the size the *picker* found, which is missing for work enqueued before this + * existed and for a request built by hand. The worker holds the URI, so when the number is + * absent it can ask the file rather than assume. + */ + fun sizeOf(context: Context, uri: Uri): Long? = firstRow(context, uri) { it.sizeOrNull() } ?: measure(context, uri) + + /** + * The sum of [sizes], or null if even one of them is unknown. + * + * A join's total is only as good as its worst-known part. Adding up the ones that answered + * would produce a lower bound that reads exactly like a real total, and the space check has + * no way to tell the two apart — which is the same conflation this whole file exists to end. + */ + fun total(sizes: List): Long? = sizes.fold(0L as Long?) { running, size -> + if (running == null || size == null) null else running + size + } + + /** + * Reads [read] out of the first row of a metadata query, or null if there is no row. + * + * Guarded because a resolver call is a call into another app: a provider that has been + * uninstalled, revoked its grant, or simply crashes takes the query with it, and a file + * picker is not a place to bring the process down from. + */ + private fun firstRow(context: Context, uri: Uri, read: (Cursor) -> T): T? = runCatching { + context.contentResolver.query(uri, null, null, null, null)?.use { cursor -> + if (cursor.moveToFirst()) read(cursor) else null + } + }.onFailure { Log.w(TAG, "Could not read metadata for $uri", it) }.getOrNull() + + /** + * The size according to the file descriptor, or null if it cannot be opened. + * + * `statSize` is `-1` for anything without a fixed length — a pipe, or a provider streaming its + * answer — which is a different way of saying "unknown" and is treated as one. + */ + private fun measure(context: Context, uri: Uri): Long? = runCatching { + context.contentResolver.openFileDescriptor(uri, "r")?.use { it.statSize } + }.getOrNull()?.takeIf { it >= 0 } + + private fun Cursor.displayNameOrNull(): String? = + getColumnIndex(OpenableColumns.DISPLAY_NAME).takeIf { it >= 0 && !isNull(it) }?.let(::getString) + + private fun Cursor.sizeOrNull(): Long? = + getColumnIndex(OpenableColumns.SIZE).takeIf { it >= 0 && !isNull(it) }?.let(::getLong)?.takeIf { it >= 0 } + + private const val TAG = "InputQuery" +} diff --git a/app/src/main/java/org/libremediaconverter/convert/MediaProbe.kt b/app/src/main/java/org/libremediaconverter/convert/MediaProbe.kt index 0fa80ae..f98cfec 100644 --- a/app/src/main/java/org/libremediaconverter/convert/MediaProbe.kt +++ b/app/src/main/java/org/libremediaconverter/convert/MediaProbe.kt @@ -8,6 +8,7 @@ import android.util.Log import com.arthenica.ffmpegkit.FFmpegKitConfig import com.arthenica.ffmpegkit.FFprobeKit import com.arthenica.ffmpegkit.MediaInformation +import org.libremediaconverter.ffmpeg.isNativeLoadFailure import org.libremediaconverter.model.ConcatInput import org.libremediaconverter.model.Container import org.libremediaconverter.model.InputKind @@ -33,6 +34,21 @@ import org.libremediaconverter.model.InputProbe */ object MediaProbe { + /** + * What [probe] reports when nothing could read the input. + * + * Named rather than inlined because a caller that has to handle [probe] itself failing + * needs to land on the same answer — see `ConversionViewModel.onInputPicked`. Two + * different spellings of "unreadable" would be two different behaviours downstream, since + * the router keys off [InputProbe.UNPARSEABLE] and the source-info card off the kind. + */ + val UNREADABLE = InputProbe( + videoCodec = InputProbe.UNPARSEABLE, + hasVideo = true, + durationMs = 0, + kind = InputKind.UNPARSEABLE, + ) + fun probe(context: Context, uri: Uri): InputProbe { val extracted = probeWithExtractor(context, uri) val info = probeWithFFprobe(context, uri) @@ -45,12 +61,7 @@ object MediaProbe { // Not a failure: an unparseable input is a strong signal that this job belongs on // FFmpeg. Reporting an unknown codec makes the router say so. Log.i(TAG, "Neither MediaExtractor nor FFprobe could read $uri; routing to FFmpeg.") - return InputProbe( - videoCodec = InputProbe.UNPARSEABLE, - hasVideo = true, - durationMs = 0, - kind = InputKind.UNPARSEABLE, - ) + return UNREADABLE } return InputProbe( @@ -145,6 +156,15 @@ object MediaProbe { } catch (e: Exception) { Log.i(TAG, "FFprobe could not read $uri.", e) null + } catch (e: Error) { + // Touching FFmpegKit at all loads its native library, and a failure there arrives as + // an Error, which the clause above cannot see -- so an unloadable library used to + // take the whole file pick down instead of reporting an unreadable file. Anything + // that is not that library failing to load is still this JVM's problem, not this + // file's, and is rethrown: see isNativeLoadFailure. + if (!isNativeLoadFailure(e)) throw e + Log.w(TAG, "FFmpegKit's native library could not be loaded; probing $uri without FFprobe.", e) + null } private fun readMediaInformation(path: String): FFprobeInfo? { diff --git a/app/src/main/java/org/libremediaconverter/convert/OutputPublisher.kt b/app/src/main/java/org/libremediaconverter/convert/OutputPublisher.kt index f0cbe30..ebb8355 100644 --- a/app/src/main/java/org/libremediaconverter/convert/OutputPublisher.kt +++ b/app/src/main/java/org/libremediaconverter/convert/OutputPublisher.kt @@ -2,6 +2,8 @@ package org.libremediaconverter.convert import android.content.Context import android.net.Uri +import android.provider.DocumentsContract +import android.provider.OpenableColumns import java.io.File /** @@ -27,24 +29,194 @@ open class OutputPublisher(private val context: Context) { open fun createStagingFile(name: String): File = File(stagingDir, name) /** - * True if there is room for a further [bytes], including headroom. + * True if staging can take a further [bytes], with [SPACE_HEADROOM_BYTES] left over. * - * Staging means peak usage is roughly input + output at once, so a job that would - * just barely fit is rejected rather than failing partway through. + * **The doc this replaces claimed peak usage was "roughly input + output at once" while the + * arithmetic reserved `input + 128 MB`.** The arithmetic is what stays, and this says why + * rather than the two continuing to disagree. + * + * [bytes] is the *input's* size standing in for the output's, because before an engine has + * run there is no other number. It is generous for the ordinary conversion, which is asked + * for precisely because it shrinks its input, and short for the ones that do not — a re-encode + * to a bulkier codec, or a stream copy into a container with more overhead. + * + * The 128 MB absorbs that error, and one more besides: [publish] copies the staged file to + * the user's destination, so while that runs the bytes exist twice on any destination sharing + * this volume. Reserving `input + output` outright would have refused jobs that fit, on a + * device where the destination is usually removable or remote. + * + * So this is a pre-flight check that stops a job which obviously cannot fit from spending + * minutes discovering it — not a guarantee. A conversion that runs out of space anyway fails + * through its engine, with a message of its own. + * + * Open so a test can force a full disk; see `FakeFailures` in the instrumented source set. */ open fun hasSpaceFor(bytes: Long): Boolean = stagingDir.usableSpace > bytes + SPACE_HEADROOM_BYTES - /** Copies a finished staging file into a user-chosen SAF destination. */ + /** + * The same check for a job whose input size nobody could determine — see [InputQuery]. + * + * **This deliberately produces the same number the defect produced by accident**, which is + * worth stating plainly: with no size to reserve for, all that is left to check is the + * headroom. What has changed is that it is now the answer to a question that was asked. The + * old code could not tell an unmeasurable file from an empty one, so it silently made this + * the answer for *both*; now [hasSpaceFor] means "there is room for this many bytes" and + * nothing else claims it. + * + * Refusing instead was considered and rejected. It would turn "no provider answered the + * `SIZE` column" into "this file cannot be converted" — a worse defect than the one being + * fixed, and one the user could do nothing about. + * + * The default answers *through* [hasSpaceFor], which is what keeps a publisher that refuses + * on space — `FakeFailures.FullDisk`, which overrides `hasSpaceFor` and nothing else — + * refusing this too. `SpaceCheckTest` pins that delegation, because an override here that + * stopped delegating would quietly stop honouring a full disk. + */ + open fun hasSpaceForUnknownSize(): Boolean = hasSpaceFor(0L) + + /** + * Copies a finished staging file into a user-chosen SAF destination. + * + * A copy that fails partway -- the destination volume filling up is the obvious one, a + * provider giving out mid-write the other -- used to leave the bytes it had managed at + * the name the user picked, while the UI said "Could not save the file". The user was + * then holding a truncated file they had been told was never written, and nothing in the + * app would ever tidy it up: staging cleanup only reaches [stagingDir], never the + * destination. + * + * So a failed copy deletes the document. Three things bound that, because deleting a + * file the user already had would be a far worse defect than the one being fixed: + * + * - **Only a document URI.** `DocumentsContract.deleteDocument` is the only delete this + * has any right to attempt, and it is defined on document URIs. Anything else -- a + * `file://` path, a MediaStore item, a content URI from a provider that is not a + * documents provider -- is left exactly as it is. + * - **Only a destination that was empty when we started.** The size is read before the + * stream is opened, and the delete only runs if the answer was positively zero. Every + * destination reaching here comes from the SAF `CreateDocument` contract, so in + * practice it is a document this app just created; but `publish` cannot verify that + * from a `Uri`, and a provider that hands back an existing document for a name the + * user re-picked would otherwise have its file deleted rather than merely truncated. + * A provider that reports no size at all falls into the same "not known to be empty" + * bucket, so the fix is conservative rather than universal: it will not clean up + * behind such a provider, and it will not delete anything of theirs either. + * - **The original failure is what the caller sees.** Cleanup runs inside its own + * `runCatching`; if it throws, that goes on the original exception as a suppressed + * one. `save()` reports `e.message`, and "could not delete the half-written file" is + * not the thing to tell someone whose disk just filled up. + * + * The whole `use` is guarded, not just the copy: a `close()` that throws while flushing + * IS the disk-full case, and it arrives after `copyTo` has returned. The cost is that a + * file whose every byte reached the provider before a failing flush is deleted too -- + * which is the right way round, since a flush that failed means the bytes are not + * durably there to begin with. + * + * A failure from `openOutputStream` itself is deliberately outside the guard. Nothing + * has been written at that point, so there is nothing of ours to remove. + */ open fun publish(staged: File, destination: Uri) { - context.contentResolver.openOutputStream(destination)?.use { out -> - staged.inputStream().use { it.copyTo(out) } - } ?: error("Could not open destination for writing: $destination") + val destinationWasEmpty = destinationIsKnownEmpty(destination) + val out = context.contentResolver.openOutputStream(destination) + ?: error("Could not open destination for writing: $destination") + try { + out.use { sink -> staged.inputStream().use { source -> source.copyTo(sink) } } + } catch (failure: Throwable) { + if (destinationWasEmpty) deletePartialOutput(destination, failure) + throw failure + } } - fun clearStaging() { - stagingDir.listFiles()?.forEach { it.delete() } + /** + * True only when the destination is *positively known* to hold no bytes yet. + * + * Every other answer -- a provider that does not report `_size`, a query that returns no + * row, a resolver call that throws -- is false, because this decides whether a delete is + * allowed and "I could not tell" must never authorise one. + * + * The column is looked up by name rather than taken as index 0: a projection is a + * request, not a guarantee, and a provider is free to return its own column set. + */ + private fun destinationIsKnownEmpty(destination: Uri): Boolean = runCatching { + context.contentResolver + .query(destination, arrayOf(OpenableColumns.SIZE), null, null, null) + ?.use { row -> + val size = row.getColumnIndex(OpenableColumns.SIZE) + size >= 0 && row.moveToFirst() && !row.isNull(size) && row.getLong(size) == 0L + } + }.getOrNull() ?: false + + /** + * Removes the half-written document, never at the expense of [cause]. + * + * `deleteDocument` reports its own failure two different ways -- `false`, or a thrown + * `FileNotFoundException` -- and neither is worth failing the save over, because the + * save has already failed. Whatever it does, [cause] is what propagates; a thrown + * cleanup failure is attached to it so it is not simply lost. + */ + private fun deletePartialOutput(destination: Uri, cause: Throwable) { + runCatching { + if (DocumentsContract.isDocumentUri(context, destination)) { + DocumentsContract.deleteDocument(context.contentResolver, destination) + } + }.onFailure(cause::addSuppressed) } + /** + * Deletes one staged file, if it really is one of ours. + * + * This is what a ViewModel's `reset()` calls when the user taps "Start over" on a + * finished-but-unsaved conversion, which is otherwise a full-size copy left in cache + * for the OS to reclaim whenever it feels like it. + * + * The guard is not decoration. The handle reaches the ViewModel as a path string in + * `WorkInfo.outputData` and is turned straight into a `File`, so this is the one place + * that checks where it points before deleting. Comparing the *canonical* parent rather + * than the path as written is what makes `conversions/../something` fail: the naive + * string comparison accepts it. + * + * @return true if a file was deleted. False covers both "not in staging" and "already + * gone", which the caller has no reason to tell apart — a `reset()` after a + * successful save is an ordinary second call. + */ + open fun discardStaged(staged: File): Boolean { + val parent = staged.parentFile?.canonicalOrAbsolute() ?: return false + if (parent != stagingDir.canonicalOrAbsolute()) return false + return staged.delete() + } + + /** + * Deletes staged files old enough to have been abandoned. + * + * The backstop for everything `discardStaged` cannot reach: a process killed between + * finishing a conversion and saving it, a worker that failed before its output ever + * became a `Converted` state, or a `reset()` whose delete was cancelled with the + * Activity. [StagingSweep] owns the rule and its reasoning. + * + * Deliberately not the `clearStaging()` this replaces. That deleted the directory's + * whole contents, and the convert tab, the join tab and `ConcatEngine`'s list file all + * share this directory — so a blanket delete could destroy a live job's file. Per-job + * staging names ([StagingNames]) stop two jobs from *sharing* a file; they say nothing + * about whether a file's job is still running, which is the question here. + * + * [nowMs] is a parameter so the clock is the caller's, not a hidden global. + */ + open fun sweepStaging(nowMs: Long = System.currentTimeMillis()) { + val dir = stagingDir + val listing = dir.listFiles() ?: return + val entries = listing.map { StagingSweep.Entry(it.name, it.lastModified()) } + StagingSweep.collectable(entries, nowMs).forEach { name -> + val file = File(dir, name) + // Re-read the timestamp rather than trusting the snapshot above. Between the + // listing and here, a worker resumed by WorkManager -- which runs in this same + // process -- could have started writing this very file, and unlinking an inode a + // running job still holds open would end with the job reporting success for a + // path that no longer exists. + if (StagingSweep.isCollectable(file.lastModified(), nowMs)) file.delete() + } + } + + private fun File.canonicalOrAbsolute(): File = runCatching { canonicalFile }.getOrDefault(absoluteFile) + private companion object { const val SPACE_HEADROOM_BYTES = 128L * 1024 * 1024 } diff --git a/app/src/main/java/org/libremediaconverter/convert/StagingNames.kt b/app/src/main/java/org/libremediaconverter/convert/StagingNames.kt new file mode 100644 index 0000000..c00f4a4 --- /dev/null +++ b/app/src/main/java/org/libremediaconverter/convert/StagingNames.kt @@ -0,0 +1,52 @@ +package org.libremediaconverter.convert + +import java.util.UUID + +/** + * Gives every job a staging path of its own. + * + * `/conversions/` is shared by the convert tab, the join tab and + * [org.libremediaconverter.ffmpeg.ConcatEngine]'s list file, and until this existed none of the + * three named a file that belonged to one job. A conversion derived its name from the input's + * display name, so two `holiday.mp4` from different folders collided; a join used the constant + * `joined.`, so any two joins of one format collided; the list file was the constant + * `concat_list.txt`, so any two joins at all collided. + * + * The collision was not theoretical. Two independent conversions on a Pixel each produced + * `cache/conversions/input_converted.mp4`, and a tag query in a fresh process returned two + * SUCCEEDED `WorkInfo`s naming that one file — which is what leaves reattachment unable to say + * which job the bytes on disk belong to. + * + * ## Why the job id, and why opaque + * + * The WorkManager request id is stable across retries: `WorkerWrapper` builds `WorkerParameters` + * from the `WorkSpec` id and only increments `runAttemptCount`. That matters more than uniqueness + * does — a retry runs `doWork()` from the top, and the delete on the way out of a failed attempt + * only collects the previous attempt's partial when the name has not moved. + * + * The staged name is never shown to anyone: `save()` recomputes a suggested name from the job's + * own spec, and the user picks the real one in the SAF dialog. So there is nothing to lose by + * making it opaque, and something to gain — the alternative was sanitising a provider-supplied + * display name, which can contain a separator, be empty, or be four kilobytes long. Naming the job + * retires that question instead of answering it. + * + * The extension is kept, and is not decoration. `FFmpegConcatCommand` names no output muxer, so + * FFmpeg infers it from the path; a name without the right extension would quietly produce the + * wrong container. + */ +object StagingNames { + + /** The staging filename for the job with this [jobId], producing a file of type [extension]. */ + fun forJob(jobId: UUID, extension: String): String = "$jobId.$extension" + + /** + * The concat list file that belongs to the output staged as [outputName]. + * + * Derived from the output rather than taken as another parameter, so the two cannot drift + * apart and so a directory listing shows which list belongs to which join. It ages with its + * output too, which is what [StagingSweep] needs. + */ + fun concatListFor(outputName: String): String = outputName.substringBeforeLast('.', outputName) + CONCAT_LIST_SUFFIX + + private const val CONCAT_LIST_SUFFIX = ".concat_list.txt" +} diff --git a/app/src/main/java/org/libremediaconverter/convert/StagingSweep.kt b/app/src/main/java/org/libremediaconverter/convert/StagingSweep.kt new file mode 100644 index 0000000..b54e39b --- /dev/null +++ b/app/src/main/java/org/libremediaconverter/convert/StagingSweep.kt @@ -0,0 +1,59 @@ +package org.libremediaconverter.convert + +/** + * Decides which staging entries are old enough to collect. + * + * A pure function rather than a loop inside [OutputPublisher], for the reason + * [org.libremediaconverter.work.FailureOutcome] documents: the decision is worth verifying + * and the situation that provokes it is not reproducible. Here the untestable part is the + * clock — an orphan is only interesting a day after it was written, and a filesystem's + * mtime granularity is not something a test should be measuring. Timestamps therefore + * arrive as values. + * + * The rule replaces an unconditional `clearStaging()` that deleted the directory's whole + * contents. That was hazardous: `/conversions/` is shared by the convert tab, the + * join tab and [org.libremediaconverter.ffmpeg.ConcatEngine]'s list file, and any two of + * them can be live at once, so a blanket delete could take a file out from under a running + * job. Age is the narrowing. + * + * Per-job staging names ([StagingNames]) do not change that. They stop two jobs from writing + * one file; they do nothing about a sweep deleting a file whose job is still running, which + * is what age is for. + */ +object StagingSweep { + + /** One directory entry, reduced to what the decision actually needs. */ + data class Entry(val name: String, val lastModifiedMs: Long) + + /** + * How stale a staging file has to be before it is assumed abandoned. + * + * Twenty-four hours, chosen against the longest a live file can plausibly go untouched + * rather than against how quickly cache should be reclaimed. Output files are written + * continuously, so a running job refreshes their mtime by itself. A join's list file is + * the exception — written once and then only read for the rest of the join — so the + * period has to exceed a whole join. WorkManager caps a single attempt at the + * six-hour-per-day foreground-service budget and then stops the worker, and a retry + * rewrites the list file, so no attempt can hold a file still for a day. + */ + const val GRACE_PERIOD_MS: Long = 24L * 60 * 60 * 1000 + + /** The names in [entries] that may be deleted, in the order they were given. */ + fun collectable(entries: List, nowMs: Long, gracePeriodMs: Long = GRACE_PERIOD_MS): List = entries + .filter { isCollectable(it.lastModifiedMs, nowMs, gracePeriodMs) } + .map { it.name } + + /** + * True if a file last written at [lastModifiedMs] is collectable at [nowMs]. + * + * Exposed separately so the caller can re-check a single entry immediately before + * deleting it, closing the window between listing a directory and acting on the list. + * + * A negative age — a file dated in the future, because the clock moved backwards — is + * deliberately not collectable. It carries no information about whether the file is in + * use, and keeping a file costs cache while deleting one can cost the user an hour of + * transcoding. + */ + fun isCollectable(lastModifiedMs: Long, nowMs: Long, gracePeriodMs: Long = GRACE_PERIOD_MS): Boolean = + nowMs - lastModifiedMs >= gracePeriodMs +} diff --git a/app/src/main/java/org/libremediaconverter/convert/Transcoders.kt b/app/src/main/java/org/libremediaconverter/convert/Transcoders.kt index b70b9ae..0a0b407 100644 --- a/app/src/main/java/org/libremediaconverter/convert/Transcoders.kt +++ b/app/src/main/java/org/libremediaconverter/convert/Transcoders.kt @@ -7,6 +7,7 @@ import org.libremediaconverter.codec.AndroidDeviceCodecs import org.libremediaconverter.ffmpeg.FFmpegEngine import org.libremediaconverter.model.ConversionRequest import org.libremediaconverter.model.DeviceCodecs +import org.libremediaconverter.model.InputProbe import org.libremediaconverter.model.OutputFormat import java.io.File @@ -74,10 +75,36 @@ object ConversionDependencies { @Volatile var deviceCodecs: () -> DeviceCodecs = { AndroidDeviceCodecs.get() } + /** + * Reading an input's codecs, container and duration. + * + * Here for a reason the others are not, and the reason is worth recording rather than + * just working around. [MediaProbe] spawns FFprobe, and when FFmpegKit's native library + * cannot load, the failure arrives as a `java.lang.Error` rather than an `Exception` — + * which is why `probeWithFFprobe`'s `catch (e: Exception)` did not see it, and why + * `ConversionViewModel.onInputPicked` used to abandon its `viewModelScope.launch` + * instead of reporting a file it could not read. + * + * **Both of those are guarded now**, by + * [org.libremediaconverter.ffmpeg.isNativeLoadFailure] — which also documents what the + * boundary actually throws, since all three of the obvious guesses turn out to be + * wrong. This seam is no longer what stands between a JVM test and an uncaught error. + * + * It still earns its place: injecting a probe is how a test reaches a *chosen* outcome + * for a file rather than the unreadable verdict the JVM has no libraries to improve on, + * and how the error path itself is forced — see `ConversionViewModelProbeFailureTest`, + * which drives an `OutOfMemoryError` through here to pin that the guard stays narrow. + * + * Instrumented tests and the app itself get the real probe, exactly as before. + */ + @Volatile + var probe: (Context, Uri) -> InputProbe = { context, uri -> MediaProbe.probe(context, uri) } + fun reset() { hardware = { Media3Engine(it) } software = { FFmpegEngine() } publisher = { OutputPublisher(it) } deviceCodecs = { AndroidDeviceCodecs.get() } + probe = { context, uri -> MediaProbe.probe(context, uri) } } } diff --git a/app/src/main/java/org/libremediaconverter/ffmpeg/ConcatEngine.kt b/app/src/main/java/org/libremediaconverter/ffmpeg/ConcatEngine.kt index 35ea0de..1afff26 100644 --- a/app/src/main/java/org/libremediaconverter/ffmpeg/ConcatEngine.kt +++ b/app/src/main/java/org/libremediaconverter/ffmpeg/ConcatEngine.kt @@ -8,6 +8,7 @@ import com.arthenica.ffmpegkit.FFmpegKitConfig import com.arthenica.ffmpegkit.ReturnCode import kotlinx.coroutines.suspendCancellableCoroutine import org.libremediaconverter.convert.MediaProbe +import org.libremediaconverter.convert.StagingNames import org.libremediaconverter.model.ConcatPlanner import org.libremediaconverter.model.ConcatStrategy import org.libremediaconverter.model.OutputFormat @@ -43,7 +44,12 @@ class ConcatEngine(private val context: Context) { // The demuxer reads its input list from a file, which must live somewhere // FFmpeg can read; app cache is a real path, so it just works. - val listFile = File(output.parentFile, "concat_list.txt").apply { + // + // Named after the output rather than by the constant "concat_list.txt" it used to use. + // The constant meant any two joins running at once shared one list file, so one of them + // read the other's inputs -- and it is why a blanket sweep of the staging directory was + // never safe. See StagingNames. + val listFile = File(output.parentFile, StagingNames.concatListFor(output.name)).apply { writeText(FFmpegConcatCommand.listFileContents(paths)) } diff --git a/app/src/main/java/org/libremediaconverter/ffmpeg/NativeLoadFailure.kt b/app/src/main/java/org/libremediaconverter/ffmpeg/NativeLoadFailure.kt new file mode 100644 index 0000000..0134867 --- /dev/null +++ b/app/src/main/java/org/libremediaconverter/ffmpeg/NativeLoadFailure.kt @@ -0,0 +1,59 @@ +package org.libremediaconverter.ffmpeg + +/** + * Whether [error] is FFmpegKit failing to load its native library, rather than this JVM + * being in trouble. + * + * ## Why a predicate rather than a catch clause + * + * `config/detekt/detekt.yml` turns `TooGenericExceptionCaught` off with a written argument: + * the engine boundaries sit in front of native code whose failure types are undocumented, + * "enumerating it would mean guessing, and a guess that is wrong crashes the app on a file + * it could have simply reported as unreadable." That argument is about *exceptions*, and it + * applies unchanged one level up — except that on the `Error` side the opposite mistake is + * available too. `catch (Throwable)` at a boundary that spawns a native process would + * swallow a genuine [OutOfMemoryError] and let the app carry on pretending it had merely + * met an unreadable file. + * + * So this names the failure instead of the catch clause. Everything it does not recognise is + * rethrown. + * + * ## What the boundary actually throws + * + * Read off the shipped AAR and confirmed by `MediaProbeNativeLoadTest`, because all three of + * the obvious guesses are wrong: + * + * - `NativeLoader.loadLibrary` catches the `UnsatisfiedLinkError` that `System.loadLibrary` + * raises and rethrows `java.lang.Error(message, cause)` — a **bare** `Error`, which is + * neither an `Exception` nor a [LinkageError]. `catch (e: UnsatisfiedLinkError)` sees + * nothing. Its `cause` is the original `UnsatisfiedLinkError`, which is what identifies it + * here; matching on the message would be matching on a format string. + * - That throw happens under `FFmpegKitConfig.`, so what a caller sees also depends + * on how the runtime treats an initializer that fails: observed as + * `ExceptionInInitializerError` on the JVM under Robolectric, and recorded as the bare + * `Error` in `docs/defect-audit.md`. Both shapes are handled rather than either being + * assumed. + * - Every touch **after** the first is a third type again — `NoClassDefFoundError: Could not + * initialize class …`, the JVM's own record that the class is poisoned. A guard written + * for the first shape alone would let the second pick onwards crash, which is the harder + * half to notice. + * + * All of the class-loading shapes are [LinkageError]s, and none of the errors that mean this + * JVM is failing — [OutOfMemoryError], `StackOverflowError`, the rest of + * `VirtualMachineError` — is one. That disjointness is what makes this narrow rather than a + * blanket `catch (Throwable)`. + * + * ## When it can fire + * + * Not on a healthy install: the `.so` files ship in the APK. A corrupted install or an ABI + * mismatch is the realistic device path, and the JVM unit tests are the other, where the + * libraries are absent by construction. + */ +internal fun isNativeLoadFailure(error: Error): Boolean = when { + // NoClassDefFoundError, ExceptionInInitializerError, UnsatisfiedLinkError: the JVM's + // whole vocabulary for "the code could not be loaded". + error is LinkageError -> true + // FFmpegKit's own bare java.lang.Error, identified by what it wraps. + error.cause is UnsatisfiedLinkError -> true + else -> false +} diff --git a/app/src/main/java/org/libremediaconverter/join/JoinScreen.kt b/app/src/main/java/org/libremediaconverter/join/JoinScreen.kt index e5b6707..c1a395e 100644 --- a/app/src/main/java/org/libremediaconverter/join/JoinScreen.kt +++ b/app/src/main/java/org/libremediaconverter/join/JoinScreen.kt @@ -18,6 +18,7 @@ import androidx.compose.material3.OutlinedButton import androidx.compose.material3.Text import androidx.compose.runtime.Composable import androidx.compose.runtime.getValue +import androidx.compose.runtime.remember import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier import androidx.compose.ui.text.style.TextAlign @@ -30,6 +31,7 @@ import org.libremediaconverter.model.ConcatStrategy import org.libremediaconverter.ui.PrimaryButtonHeight import org.libremediaconverter.ui.ScreenPaddingHorizontal import org.libremediaconverter.ui.ScreenPaddingVertical +import org.libremediaconverter.work.ConcatWorker @UnstableApi @Composable @@ -40,8 +42,13 @@ fun JoinScreen(modifier: Modifier = Modifier, viewModel: JoinViewModel = viewMod ActivityResultContracts.OpenMultipleDocuments(), ) { uris -> if (uris.isNotEmpty()) viewModel.onInputsPicked(uris) } + // The contract's MIME type comes from the finished job rather than from a literal: some + // providers rewrite a document's extension to match it, so naming MP4 for a join that is not + // one can hand the user a file the extension lies about. Remembered against that type so the + // launcher re-registers only when it actually changes. + val destinationMime = (state as? JoinState.Joined)?.mimeType ?: ConcatWorker.DEFAULT_FORMAT.mimeType val chooseDestination = rememberLauncherForActivityResult( - ActivityResultContracts.CreateDocument("video/mp4"), + remember(destinationMime) { ActivityResultContracts.CreateDocument(destinationMime) }, ) { uri -> uri?.let(viewModel::save) } Column( @@ -111,9 +118,11 @@ fun JoinScreen(modifier: Modifier = Modifier, viewModel: JoinViewModel = viewMod } is JoinState.Waiting -> { + // Same two causes as the converter screen's Waiting state, and the same + // wording for them -- see the comment there. Text( - "Paused. The system limits background media processing to " + - "six hours a day, so this will resume automatically.", + "Paused. Android limits background media processing, so this will " + + "resume automatically — keeping the app open helps it along.", style = MaterialTheme.typography.bodyMedium, ) OutlinedButton( @@ -136,7 +145,7 @@ fun JoinScreen(modifier: Modifier = Modifier, viewModel: JoinViewModel = viewMod style = MaterialTheme.typography.bodySmall, ) Button( - onClick = { chooseDestination.launch("joined.mp4") }, + onClick = { chooseDestination.launch(s.suggestedName) }, modifier = Modifier.fillMaxWidth().height(PrimaryButtonHeight), ) { Text("Save file") } OutlinedButton( diff --git a/app/src/main/java/org/libremediaconverter/join/JoinViewModel.kt b/app/src/main/java/org/libremediaconverter/join/JoinViewModel.kt index 2d09e3f..7b0dc7b 100644 --- a/app/src/main/java/org/libremediaconverter/join/JoinViewModel.kt +++ b/app/src/main/java/org/libremediaconverter/join/JoinViewModel.kt @@ -2,12 +2,12 @@ package org.libremediaconverter.join import android.app.Application import android.net.Uri -import android.provider.OpenableColumns import androidx.lifecycle.AndroidViewModel import androidx.lifecycle.viewModelScope import androidx.media3.common.util.UnstableApi import androidx.work.WorkInfo import androidx.work.WorkManager +import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.Job import kotlinx.coroutines.flow.MutableStateFlow @@ -15,10 +15,14 @@ import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.asStateFlow import kotlinx.coroutines.launch import kotlinx.coroutines.withContext +import org.libremediaconverter.convert.ConversionDependencies import org.libremediaconverter.convert.InputFile -import org.libremediaconverter.convert.OutputPublisher +import org.libremediaconverter.convert.InputQuery import org.libremediaconverter.model.ConcatStrategy import org.libremediaconverter.work.ConcatWorker +import org.libremediaconverter.work.JobTags +import org.libremediaconverter.work.Reattachment +import org.libremediaconverter.work.jobSnapshots import java.io.File import java.util.UUID @@ -27,16 +31,33 @@ sealed interface JoinState { data class Ready(val inputs: List) : JoinState data class Joining(val inputs: List) : JoinState data class Waiting(val inputs: List) : JoinState - data class Joined(val staged: File, val strategy: ConcatStrategy) : JoinState + data class Joined( + val staged: File, + val strategy: ConcatStrategy, + /** + * What to call the file, and what type to open the save dialog with. + * + * From the job, not from a literal. See `ConcatWorker.KEY_SUGGESTED_NAME`. + */ + val suggestedName: String, + val mimeType: String, + ) : JoinState data class Saved(val displayName: String) : JoinState data class Failed(val message: String) : JoinState } @UnstableApi -class JoinViewModel(app: Application) : AndroidViewModel(app) { +class JoinViewModel @JvmOverloads constructor( + app: Application, + /** Where [reset] runs its delete. See the same parameter on `ConversionViewModel`. */ + private val cleanupDispatcher: CoroutineDispatcher = Dispatchers.IO, +) : AndroidViewModel(app) { private val workManager = WorkManager.getInstance(app) - private val publisher = OutputPublisher(app) + + // Through ConversionDependencies, like the workers, rather than `OutputPublisher(app)` + // direct -- see the same line in ConversionViewModel. + private val publisher = ConversionDependencies.publisher(app) private val _state = MutableStateFlow(JoinState.Idle) val state: StateFlow = _state.asStateFlow() @@ -44,13 +65,74 @@ class JoinViewModel(app: Application) : AndroidViewModel(app) { private var observer: Job? = null private var activeWorkId: UUID? = null + /** + * The staged output this ViewModel is responsible for deleting. + * + * Held here rather than read back out of [_state] for the same reason as in + * `ConversionViewModel`: a failed [save] lands on [JoinState.Failed], which carries a + * message and no file, so the state machine cannot answer this on the one path that + * most needs it. + */ + private var pendingStaged: File? = null + + init { + reattach() + } + + /** + * Picks up a join this ViewModel did not start. + * + * The same defect as on the convert side, and the same shape of fix: a join outlives the + * process on purpose, so a process reclaimed after one finished came back to an empty screen + * with the joined file sitting unreachable in `cacheDir`. Found by querying for the worker's + * own class name, which WorkManager tags every request with, so nothing has to be persisted + * and work from an earlier version of the app is found too. [Reattachment.choose] carries + * the rules about which job and why. + */ + private fun reattach() { + viewModelScope.launch { + val reattachment = Reattachment.choose( + workManager.jobSnapshots( + tag = ConcatWorker::class.java.name, + outputPathKey = ConcatWorker.KEY_OUTPUT_PATH, + ), + ) ?: return@launch + + // The query suspends, so the user may have picked files or started a join in the + // meantime. Theirs wins. No suspension point between this check and the assignment + // below, and both run on the main dispatcher, so nothing can interleave. + if (_state.value !is JoinState.Idle || activeWorkId != null) return@launch + + // Joins used to stage under one constant name, so two finished joins always reported + // the same file and no tag of either could be trusted to describe it. That is what + // Ambiguous means here, and the count falls back rather than being borrowed — which + // costs nothing in practice, since the count is only rendered while a job is live and + // a live job names no file to be aliased on. Staging on the job id has closed that for + // new work, including the stream-copy-or-re-encode line on Joined, which comes from + // the picked job's output; joins already in the queue keep the old shape. + val tags = (reattachment as? Reattachment.Certain)?.job?.tags.orEmpty() + // Placeholders, and safe only because of where they can go. Joining reads nothing + // but the size of this list, Waiting and Joined read none of it, and a reattached + // job that is cancelled lands on Idle rather than Ready — the one state that would + // render these individually and offer to join them. Anything that starts drawing + // this list has to carry the names in the tags first. + val inputs = List(JobTags.inputCountOf(tags) ?: MIN_JOIN_INPUTS) { + InputFile(Uri.EMPTY, "", sizeBytes = null) + } + activeWorkId = reattachment.job.id + observe(reattachment.job.id, inputs, cancelled = JoinState.Idle) + } + } + fun onInputsPicked(uris: List) { if (uris.size < 2) { _state.value = JoinState.Failed("Pick at least two files to join.") return } viewModelScope.launch { - val files = withContext(Dispatchers.IO) { uris.map(::queryFile) } + val files = withContext(Dispatchers.IO) { + uris.map { InputQuery.describe(getApplication(), it) } + } _state.value = JoinState.Ready(files) } } @@ -59,15 +141,27 @@ class JoinViewModel(app: Application) : AndroidViewModel(app) { val inputs = (_state.value as? JoinState.Ready)?.inputs ?: return val request = ConcatWorker.request( inputs = inputs.map { it.uri }, - totalBytes = inputs.sumOf { it.sizeBytes }, + // Not `sumOf`, which cannot express what is being summed any more. A join's + // total is only as good as its least-known part, and adding up the inputs that + // did answer would hand the space check a lower bound it would read as a total. + totalBytes = InputQuery.total(inputs.map { it.sizeBytes }), ) activeWorkId = request.id workManager.enqueue(request) _state.value = JoinState.Joining(inputs) + observe(request.id, inputs) + } + /** + * @param cancelled where a cancellation lands. For a join started here that is the picked + * files, ready to join again. For one picked up by [reattach] there are no picked files — + * what that job holds are URIs granted to a process that no longer exists — so it lands on + * Idle rather than offering to re-join files nothing can open. + */ + private fun observe(id: UUID, inputs: List, cancelled: JoinState = JoinState.Ready(inputs)) { observer?.cancel() observer = viewModelScope.launch { - workManager.getWorkInfoByIdFlow(request.id).collect { info -> + workManager.getWorkInfoByIdFlow(id).collect { info -> if (info == null) return@collect _state.value = when (info.state) { WorkInfo.State.RUNNING, WorkInfo.State.BLOCKED -> JoinState.Joining(inputs) @@ -85,15 +179,39 @@ class JoinViewModel(app: Application) : AndroidViewModel(app) { if (path == null) { JoinState.Failed("Joining reported success but produced no file.") } else { - JoinState.Joined(File(path), strategy) + 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) ?: "Joining failed.", + info.outputData.getString(ConcatWorker.KEY_ERROR) + ?.takeIf { it.isNotBlank() } + ?: "Joining failed.", ) - WorkInfo.State.CANCELLED -> JoinState.Ready(inputs) + WorkInfo.State.CANCELLED -> cancelled } } } @@ -112,33 +230,43 @@ class JoinViewModel(app: Application) : AndroidViewModel(app) { joined.staged.delete() } }.onSuccess { - _state.value = JoinState.Saved("joined.mp4") + // publish() already deleted it; nothing left to clean up. + pendingStaged = null + _state.value = JoinState.Saved(joined.suggestedName) }.onFailure { e -> + // Deliberately NOT cleared -- see the same branch in ConversionViewModel. + // A failed save can leave the staged file as the only copy of the work, so + // it is left for a later reset() or for the sweep to collect once its age + // makes it certain nobody is coming back for it. _state.value = JoinState.Failed(e.message ?: "Could not save the file.") } } } + /** + * Returns to [JoinState.Idle], deleting anything staged on the way out. + * + * Best effort, not a guarantee: the delete is cancelled with [viewModelScope] if the + * Activity finishes first. `OutputPublisher.sweepStaging` is the backstop. + */ fun reset() { observer?.cancel() observer = null activeWorkId = null + val staged = pendingStaged + pendingStaged = null + if (staged != null) { + viewModelScope.launch(cleanupDispatcher) { publisher.discardStaged(staged) } + } _state.value = JoinState.Idle } - private fun queryFile(uri: Uri): InputFile { - var name = "input" - var size = 0L - getApplication().contentResolver - .query(uri, null, null, null, null) - ?.use { cursor -> - if (cursor.moveToFirst()) { - cursor.getColumnIndex(OpenableColumns.DISPLAY_NAME).takeIf { it >= 0 } - ?.let { name = cursor.getString(it) ?: name } - cursor.getColumnIndex(OpenableColumns.SIZE).takeIf { it >= 0 } - ?.let { size = cursor.getLong(it) } - } - } - return InputFile(uri, name, size) + private companion object { + /** + * Used when a reattached job carries no count tag — work enqueued by an earlier version + * of the app. Both the picker and the worker refuse fewer than two inputs, so this is a + * floor rather than a guess, and it keeps the screen from claiming a join of no files. + */ + const val MIN_JOIN_INPUTS = 2 } } diff --git a/app/src/main/java/org/libremediaconverter/work/ConcatWorker.kt b/app/src/main/java/org/libremediaconverter/work/ConcatWorker.kt index 8511d25..553310b 100644 --- a/app/src/main/java/org/libremediaconverter/work/ConcatWorker.kt +++ b/app/src/main/java/org/libremediaconverter/work/ConcatWorker.kt @@ -9,8 +9,12 @@ import androidx.work.Data import androidx.work.ForegroundInfo import androidx.work.OneTimeWorkRequestBuilder import androidx.work.WorkerParameters +import androidx.work.hasKeyWithValueOfType import androidx.work.workDataOf +import kotlinx.coroutines.CancellationException import org.libremediaconverter.convert.ConversionDependencies +import org.libremediaconverter.convert.InputQuery +import org.libremediaconverter.convert.StagingNames import org.libremediaconverter.ffmpeg.ConcatEngine import org.libremediaconverter.model.OutputFormat @@ -37,39 +41,67 @@ class ConcatWorker(context: Context, params: WorkerParameters) : CoroutineWorker if (uris.size < 2) { return Result.failure(workDataOf(KEY_ERROR to "Pick at least two files to join.")) } - val totalBytes = inputData.getLong(KEY_TOTAL_BYTES, 0L) + // Absent, not zero, when the picker could not size every input -- see the same read in + // ConversionWorker and InputQuery for why the two are no longer one number. + val declaredTotal = inputData + .takeIf { it.hasKeyWithValueOfType(KEY_TOTAL_BYTES) } + ?.getLong(KEY_TOTAL_BYTES, 0L) val format = OutputFormat.valueOf( - inputData.getString(KEY_FORMAT) ?: OutputFormat.MP4_H264.name, + inputData.getString(KEY_FORMAT) ?: DEFAULT_FORMAT.name, ) - if (!publisher.hasSpaceFor(totalBytes)) { + if (!hasRoomFor(declaredTotal, uris)) { return Result.failure(workDataOf(KEY_ERROR to "Not enough free space to join these files.")) } - setForeground( - ForegroundInfo( - NOTIFICATION_ID, - notifications.build(id, "Joining ${uris.size} files", 0, indeterminate = true), - ConversionForegroundType.current(), - ), - ) + // Named before anything below can throw, so every exit has the handle to clean up with. + // See the same line in ConversionWorker. + // + // Keyed on this job's id. The constant "joined." this replaces meant any two joins of + // the same format wrote one file, and ConcatEngine's list file collided harder still. + val staged = publisher.createStagingFile(StagingNames.forJob(id, format.extension)) - val staged = publisher.createStagingFile("joined.${format.extension}") return try { + // Inside the try: a foreground start refused because the app is in the background -- + // which is where a WorkManager restart after process death always begins -- used to + // throw straight past this catch, taking the retry, the error message and the delete + // with it. See ConversionWorker.doWork and FailureOutcome. + setForeground( + ForegroundInfo( + NOTIFICATION_ID, + notifications.build(id, "Joining ${uris.size} files", 0, indeterminate = true), + ConversionForegroundType.current(), + ), + ) + val result = ConcatEngine(applicationContext).join(uris, staged, format) Result.success( workDataOf( KEY_OUTPUT_PATH to staged.absolutePath, KEY_STRATEGY to result.strategy.name, + // See the same two in ConversionWorker. The join screen has no format picker + // today, so `joined.mp4` was right by accident everywhere it was written out; + // reporting them means the accident is not what holds it up. + KEY_SUGGESTED_NAME to outputNameFor(format), + KEY_MIME_TYPE to format.mimeType, ), ) + } catch (e: CancellationException) { + // Rethrown rather than answered with a Result -- see the same branch in + // ConversionWorker for why, and for why the delete stays. + staged.delete() + throw e } catch (e: Throwable) { staged.delete() - when (FailureOutcome.forStopReason(stopReason)) { + when (FailureOutcome.forFailure(stopReason, e, runAttemptCount)) { FailureOutcome.RETRY -> { - Log.w(TAG, "Foreground budget exhausted while joining; will retry.", e) + Log.w(TAG, "Joining interrupted; will retry.", e) Result.retry() } + FailureOutcome.FOREGROUND_DENIED -> { + Log.e(TAG, "Foreground start refused $runAttemptCount times; giving up.", e) + Result.failure(workDataOf(KEY_ERROR to FailureOutcome.FOREGROUND_DENIED_MESSAGE)) + } FailureOutcome.FAIL -> { Log.e(TAG, "Joining failed.", e) Result.failure(workDataOf(KEY_ERROR to (e.message ?: "Joining failed."))) @@ -78,6 +110,23 @@ class ConcatWorker(context: Context, params: WorkerParameters) : CoroutineWorker } } + /** + * Whether staging can take this join, measuring the inputs when nothing else has. + * + * The same shape as `ConversionWorker.hasRoomFor` and for the same reasons, with one + * difference worth naming: a join's total is [InputQuery.total], which is null the moment a + * *single* input cannot be sized. Summing the ones that answered would produce a lower bound + * indistinguishable from a real total, which is the conflation this change exists to end. + */ + private fun hasRoomFor(declared: Long?, uris: List): Boolean { + val bytes = declared ?: InputQuery.total(uris.map { InputQuery.sizeOf(applicationContext, it) }) + if (bytes == null) { + Log.i(TAG, "Nothing could size every input; checking headroom only.") + return publisher.hasSpaceForUnknownSize() + } + return publisher.hasSpaceFor(bytes) + } + override suspend fun getForegroundInfo(): ForegroundInfo = ForegroundInfo( NOTIFICATION_ID, notifications.build(id, "Joining files", 0, indeterminate = true), @@ -90,17 +139,49 @@ class ConcatWorker(context: Context, params: WorkerParameters) : CoroutineWorker const val KEY_FORMAT = "format" const val KEY_OUTPUT_PATH = "output_path" const val KEY_STRATEGY = "strategy" + + /** The name to offer in the save dialog, and the type to open it with. */ + const val KEY_SUGGESTED_NAME = "suggested_name" + const val KEY_MIME_TYPE = "mime_type" const val KEY_ERROR = "error" + /** + * What a join produces when nothing says otherwise. + * + * Named once rather than repeated at the three places that need it -- the input-Data + * default, [request]'s parameter default, and the fallback a ViewModel uses for a job + * enqueued before this worker reported its format. Those three disagreeing is the shape + * this whole entry is about. + */ + val DEFAULT_FORMAT: OutputFormat = OutputFormat.MP4_H264 + + /** + * The name to suggest in the save dialog for a join of this [format]. + * + * A function rather than the literal `joined.mp4` it replaces: that literal appeared in + * the ViewModel and twice in the screen, and all three were correct only because the join + * screen has no format picker yet. + */ + fun outputNameFor(format: OutputFormat): String = "joined.${format.extension}" + private const val NOTIFICATION_ID = 1002 private const val TAG = "ConcatWorker" - fun request(inputs: List, totalBytes: Long, format: OutputFormat = OutputFormat.MP4_H264) = + /** + * How many files are being joined is tagged as well as passed as input `Data`, because + * `WorkInfo` gives a job's tags back and its input `Data` never. It is the one thing the + * join screen says about a job in flight, and after a restart nothing else can supply + * it. See [JobTags]. + */ + fun request(inputs: List, totalBytes: Long?, format: OutputFormat = DEFAULT_FORMAT) = OneTimeWorkRequestBuilder() + .addTag(JobTags.inputCount(inputs.size)) .setInputData( Data.Builder() .putStringArray(KEY_INPUT_URIS, inputs.map(Uri::toString).toTypedArray()) - .putLong(KEY_TOTAL_BYTES, totalBytes) + // Omitted rather than zeroed when a total could not be worked out; a + // `Data` has no null, so the missing key is the unknown. + .apply { totalBytes?.let { putLong(KEY_TOTAL_BYTES, it) } } .putString(KEY_FORMAT, format.name) .build(), ) diff --git a/app/src/main/java/org/libremediaconverter/work/ConversionWorker.kt b/app/src/main/java/org/libremediaconverter/work/ConversionWorker.kt index 4b835fd..a31249f 100644 --- a/app/src/main/java/org/libremediaconverter/work/ConversionWorker.kt +++ b/app/src/main/java/org/libremediaconverter/work/ConversionWorker.kt @@ -9,10 +9,15 @@ import androidx.work.Data import androidx.work.ForegroundInfo import androidx.work.OneTimeWorkRequestBuilder import androidx.work.WorkerParameters +import androidx.work.hasKeyWithValueOfType import androidx.work.workDataOf import com.arthenica.ffmpegkit.FFmpegKitConfig +import com.google.common.util.concurrent.ListenableFuture +import kotlinx.coroutines.CancellationException import org.libremediaconverter.convert.ConversionDependencies -import org.libremediaconverter.convert.MediaProbe +import org.libremediaconverter.convert.InputQuery +import org.libremediaconverter.convert.OutputPublisher +import org.libremediaconverter.convert.StagingNames import org.libremediaconverter.model.AudioCodec import org.libremediaconverter.model.Container import org.libremediaconverter.model.ContainerCapabilities @@ -37,6 +42,12 @@ import java.io.File * * Expedited work is deliberately *not* used. It maps to JobScheduler expedited jobs * with a short quota, which is the wrong shape for a multi-minute transcode. + * + * That durability is not free, and the queue surviving is not the same as the job surviving. + * When WorkManager recovers a job after process death the app is by definition in the background, + * where the system refuses to start a foreground service — so the recovered attempt's + * `setForeground` throws. Handling that inside [doWork] rather than letting it escape is what + * turns the recovery into a retry instead of a terminal failure; see [FailureOutcome]. */ @UnstableApi class ConversionWorker(context: Context, params: WorkerParameters) : CoroutineWorker(context, params) { @@ -50,7 +61,11 @@ class ConversionWorker(context: Context, params: WorkerParameters) : CoroutineWo val inputUri = inputData.getString(KEY_INPUT_URI)?.let(Uri::parse) ?: return Result.failure(workDataOf(KEY_ERROR to "No input file.")) val displayName = inputData.getString(KEY_DISPLAY_NAME) ?: "input" - val sizeBytes = inputData.getLong(KEY_SIZE_BYTES, 0L) + // Absent, not zero, when nobody could say -- see InputQuery. `getLong(key, 0L)` is what + // made those two the same number, and `hasSpaceFor(0)` is only "is there 128 MB free". + val declaredSize = inputData + .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, @@ -59,37 +74,55 @@ class ConversionWorker(context: Context, params: WorkerParameters) : CoroutineWo inputData.getString(KEY_ENGINE_PREFERENCE) ?: EnginePreference.AUTO.name, ) - if (!publisher.hasSpaceFor(sizeBytes)) { + if (!hasRoomFor(declaredSize, inputUri)) { return Result.failure(workDataOf(KEY_ERROR to "Not enough free space to convert.")) } - setForeground(foregroundInfo(displayName, percent = 0, indeterminate = true)) - - val probe = MediaProbe.probe(applicationContext, inputUri) - val devices = ConversionDependencies.deviceCodecs() - val request = ConversionRequest( - spec = spec, - quality = quality, - enginePreference = preference, - probe = probe, - hardwareEncodeAvailable = devices.canEncode(spec.videoCodec), - ) - // The picker refuses an impossible combination before Convert is tappable, but a job can - // also arrive from a queued request made before the settings changed, or from a direct - // ConversionWorker.request(...) call. Checking here means an invalid spec fails with the - // reason rather than being silently coerced into something else. - val validation = ContainerCapabilities.validate(spec, probe) - if (validation is Validation.Invalid) { - Log.w(TAG, "Refusing $spec for $displayName: ${validation.message}") - return Result.failure(workDataOf(KEY_ERROR to validation.message)) - } - - val decision = ConversionRouter.route(request, devices) - Log.i(TAG, "Routing $displayName -> $spec via ${decision.engine} (${decision.reason})") - - val staged = publisher.createStagingFile(outputNameFor(displayName, spec)) + // Named before anything below can throw, so every exit has the handle to clean up with. + // This only builds a path -- nothing is written until an engine opens it -- so naming it + // early costs nothing, and it is what lets the catch collect a partial an earlier attempt + // left behind under the same name. + // + // Keyed on this job's id rather than on the input's display name: two conversions of files + // that happen to share a name are two jobs, and used to be one file. See StagingNames. + val staged = publisher.createStagingFile(StagingNames.forJob(id, spec.extension)) return try { + // Inside the try, and that placement is the whole point. setForeground() throws + // ForegroundServiceStartNotAllowedException when the system refuses a background + // foreground-service start -- which is exactly what a WorkManager restart after + // process death is. With it above the try that throw escaped doWork() entirely: no + // retry, no error in the output Data, and no staged.delete(). MediaProbe.probe below + // was outside for the same reason and had the same problem. + setForeground(foregroundInfo(displayName, percent = 0, indeterminate = true)) + + // Through the seam rather than MediaProbe directly. The seam already existed for the + // ViewModel and the worker was the last caller bypassing it, which is why nothing on + // the JVM could reach a line below this one: FFprobe's loader throws a bare + // java.lang.Error with no native library present. The app and the instrumented tests + // get the real probe, exactly as before. + val probe = ConversionDependencies.probe(applicationContext, inputUri) + val devices = ConversionDependencies.deviceCodecs() + val request = ConversionRequest( + spec = spec, + quality = quality, + enginePreference = preference, + probe = probe, + hardwareEncodeAvailable = devices.canEncode(spec.videoCodec), + ) + // The picker refuses an impossible combination before Convert is tappable, but a job + // can also arrive from a queued request made before the settings changed, or from a + // direct ConversionWorker.request(...) call. Checking here means an invalid spec fails + // with the reason rather than being silently coerced into something else. + val validation = ContainerCapabilities.validate(spec, probe) + if (validation is Validation.Invalid) { + Log.w(TAG, "Refusing $spec for $displayName: ${validation.message}") + return Result.failure(workDataOf(KEY_ERROR to validation.message)) + } + + val decision = ConversionRouter.route(request, devices) + Log.i(TAG, "Routing $displayName -> $spec via ${decision.engine} (${decision.reason})") + when (decision.engine) { Engine.MEDIA3 -> runMedia3OrFallBack(request, inputUri, staged, displayName) Engine.FFMPEG -> runFFmpeg(request, inputUri, staged, displayName) @@ -99,14 +132,56 @@ class ConversionWorker(context: Context, params: WorkerParameters) : CoroutineWo KEY_OUTPUT_PATH to staged.absolutePath, KEY_ENGINE_USED to decision.engine.name, KEY_ROUTE_REASON to decision.reason.explanation, + // Reported rather than left to be recomputed. The spec arrives here as input + // Data, and WorkInfo never hands input Data back -- so this is the only moment + // at which anything knows both the input's name and the spec that ran. A + // ViewModel deriving it later has only its own picker, which is not the same + // thing and is not the same thing in two different ways: a reattached job's + // spec was never in those settings, and a live picker can move mid-job. + KEY_SUGGESTED_NAME to outputNameFor(displayName, spec), + KEY_MIME_TYPE to spec.mimeType, ), ) + } catch (e: CancellationException) { + // Cancellation is not a result, and answering it with one breaks structured + // concurrency: this coroutine would report completion inside a scope that has already + // been cancelled. Invisible today only because WorkManager marks the work CANCELLED + // itself and ignores whatever the worker returned. + // + // The delete still has to happen, and has to happen here. A cancelled attempt leaves a + // partial in staging, the next attempt starts from the top rather than resuming it, + // and this is the only code holding the handle. + staged.delete() + throw e } catch (e: Throwable) { staged.delete() - handleTimeoutIfNeeded(e) + outcomeFor(e) } } + /** + * Whether staging can take this job, asking the input itself when nothing else has. + * + * [declared] is what the picker found, carried in this job's `Data`. It is absent for work + * enqueued before the size became optional, for a [request] built by hand, and for a file + * whose provider would not answer — so the fallback opens the input and asks the descriptor, + * which is one syscall on a file the conversion is about to open anyway. It runs only when + * [declared] is null, so an ordinary job pays nothing for it. + * + * When even that cannot answer, the *question* changes rather than a number being invented: + * [OutputPublisher.hasSpaceForUnknownSize] is the documented "all that is left to check is + * the headroom", and it is not a refusal. Failing every job whose provider is quiet would be + * a worse defect than the vacuous check it replaces. + */ + private fun hasRoomFor(declared: Long?, inputUri: Uri): Boolean { + val bytes = declared ?: InputQuery.sizeOf(applicationContext, inputUri) + if (bytes == null) { + Log.i(TAG, "Nothing could size $inputUri; checking headroom only.") + return publisher.hasSpaceForUnknownSize() + } + return publisher.hasSpaceFor(bytes) + } + /** * The dynamic half of the routing rules. * @@ -157,40 +232,85 @@ class ConversionWorker(context: Context, params: WorkerParameters) : CoroutineWo private var lastNotified = 0L + /** + * Reports how far along the conversion is, through WorkManager rather than around it. + * + * This used to call `NotificationManager.notify(NOTIFICATION_ID, …)` directly — on the very id + * WorkManager owns through [setForeground], with a notification built `setOngoing(true)`. Two + * owners of one id is a race, and on a Pixel 10 Pro XL it was lost on attempt 3 of 12 while + * cancelling a `BEST`-tier job: WorkManager tore the notification down at +300 ms and a tick + * still in flight put it back at +700 ms, where it stayed for ten minutes with no app process + * left to cancel it. The orphan's record carried `flags=ONGOING_EVENT|ONLY_ALERT_ONCE` and no + * `FOREGROUND_SERVICE` — which is what identifies the poster, since WorkManager's own goes out + * with that flag. + * + * Two things stop that, and they are independent on purpose: + * + * - **Nothing is published once the worker is stopped.** A tick arriving after the stop has + * nobody left to report to, and posting one is exactly the resurrection above. + * - **What is published goes through [setForegroundAsync].** The foreground notification is + * WorkManager's to post and to withdraw; sharing the id with it was the defect, not merely + * the mechanism of it. `setForegroundAsync` also refuses on its own for work that has + * already finished, which closes the window `isStopped` can only narrow. + * + * The `~1/sec` throttle is unchanged in effect and unchanged in reason: FFmpeg's statistics + * callback and Media3's progress polling both fire several times a second, and pushing every + * one of them janks the system UI. Routing them through WorkManager does not make them cheap. + * + * The future is deliberately not awaited — this is called from an engine callback, which is + * not a coroutine — and both of its failure modes are benign, so a refusal is logged rather + * than propagated. The initial [setForeground] in [doWork] is a different matter entirely and + * stays a suspending call inside the `try`: a denial *there* is what [FailureOutcome] turns + * into a retry rather than a terminal failure. + */ private fun publishProgress(displayName: String, percent: Int) { + if (isStopped) return setProgressAsync(workDataOf(KEY_PROGRESS to percent)) - // Throttle to ~1/sec: progress arrives several times a second and pushing every - // update janks the system UI. val now = System.currentTimeMillis() - if (now - lastNotified >= NOTIFICATION_INTERVAL_MS) { - lastNotified = now - applicationContext.getSystemService(android.app.NotificationManager::class.java) - .notify(NOTIFICATION_ID, notifications.build(id, displayName, percent)) - } + if (now - lastNotified < NOTIFICATION_INTERVAL_MS) return + lastNotified = now + val posted = setForegroundAsync(foregroundInfo(displayName, percent, indeterminate = false)) + posted.addListener({ logIfRefused(posted) }, Runnable::run) } - private fun isCancellation(e: Throwable): Boolean = e is kotlinx.coroutines.CancellationException || isStopped - /** - * Distinguishes a genuine failure from the foreground-service budget expiring. + * Notes a progress update WorkManager would not take, without making it the job's problem. * - * `mediaProcessing` allows six hours out of every twenty-four, shared across the - * app. When that runs out WorkManager reports - * `STOP_REASON_FOREGROUND_SERVICE_TIMEOUT`, and the right response is to retry - * later rather than tell the user the conversion failed — the work is still valid, - * there is simply no budget right now. + * The one that actually happens is a job finishing between [publishProgress]'s `isStopped` + * check and the update reaching the task thread: `WorkForegroundUpdater` refuses to post for + * work whose state is already terminal, which is the behaviour being relied on rather than + * worked around. Logged so it is greppable instead of vanishing into an unobserved future. */ - private fun handleTimeoutIfNeeded(cause: Throwable): Result = when (FailureOutcome.forStopReason(stopReason)) { - FailureOutcome.RETRY -> { - Log.w(TAG, "Foreground service budget exhausted; will retry.", cause) - Result.retry() - } - FailureOutcome.FAIL -> { - Log.e(TAG, "Conversion failed.", cause) - Result.failure(workDataOf(KEY_ERROR to (cause.message ?: "Conversion failed."))) - } + private fun logIfRefused(posted: ListenableFuture) { + runCatching { posted.get() } + .onFailure { Log.d(TAG, "Progress update refused; the job is already finishing.", it) } } + private fun isCancellation(e: Throwable): Boolean = e is CancellationException || isStopped + + /** + * Turns whatever ended the attempt into a `Result`. [FailureOutcome] owns the rules. + * + * Three answers, because there are three genuinely different situations: the work is still + * valid and should run later, the system will not let it run and the user has to be told how + * to unblock it, or the conversion itself failed and the reason belongs on screen. + */ + private fun outcomeFor(cause: Throwable): Result = + when (FailureOutcome.forFailure(stopReason, cause, runAttemptCount)) { + FailureOutcome.RETRY -> { + Log.w(TAG, "Conversion interrupted; will retry.", cause) + Result.retry() + } + FailureOutcome.FOREGROUND_DENIED -> { + Log.e(TAG, "Foreground start refused $runAttemptCount times; giving up.", cause) + Result.failure(workDataOf(KEY_ERROR to FailureOutcome.FOREGROUND_DENIED_MESSAGE)) + } + FailureOutcome.FAIL -> { + Log.e(TAG, "Conversion failed.", cause) + Result.failure(workDataOf(KEY_ERROR to (cause.message ?: "Conversion failed."))) + } + } + /** * Reads the output spec out of the worker's input Data. * @@ -238,6 +358,10 @@ class ConversionWorker(context: Context, params: WorkerParameters) : CoroutineWo const val KEY_OUTPUT_PATH = "output_path" const val KEY_ENGINE_USED = "engine_used" const val KEY_ROUTE_REASON = "route_reason" + + /** The name to offer in the save dialog, and the type to open it with. */ + const val KEY_SUGGESTED_NAME = "suggested_name" + const val KEY_MIME_TYPE = "mime_type" const val KEY_ERROR = "error" private const val NOTIFICATION_ID = 1001 @@ -245,28 +369,44 @@ class ConversionWorker(context: Context, params: WorkerParameters) : CoroutineWo private const val TAG = "ConversionWorker" /** - * The staged and suggested filename. + * The name to suggest in the save dialog. * * The extension comes from the container and whether a video track survives, so Matroska * yields `.mkv` or `.mka` and MP4 yields `.mp4` or `.m4a` without a preset having to * enumerate both. + * + * No longer the staged name as well. Staging is keyed on the job id -- see [StagingNames] + * -- so this is only ever the string offered to the user, which is also what makes it safe + * for it to carry a display name the app does not control. */ fun outputNameFor(inputName: String, spec: OutputSpec): String = inputName.substringBeforeLast('.', inputName) + "_converted.${spec.extension}" + /** + * The name and size are tagged as well as passed as input `Data`, and that is not + * redundant. `WorkInfo` hands back a job's tags and its output but never the `Data` it + * was enqueued with, so after a restart the tags are the only way for the UI to say + * *which file* a job it did not start is working on. See [JobTags]. + */ fun request( inputUri: Uri, displayName: String, - sizeBytes: Long, + sizeBytes: Long?, spec: OutputSpec = OutputFormat.MP4_H265.spec, quality: QualityTier = QualityTier.FAST, enginePreference: EnginePreference = EnginePreference.AUTO, ) = OneTimeWorkRequestBuilder() + .addTag(JobTags.displayName(displayName)) + // Neither the tag nor the Data entry is written for a size nobody knows. A `Data` has + // no null, so the absence of the key *is* the unknown — and a tag reading + // `size-bytes:0` would come back through Reattachment as a confident claim that the + // user's file is empty. + .apply { sizeBytes?.let { addTag(JobTags.sizeBytes(it)) } } .setInputData( Data.Builder() .putString(KEY_INPUT_URI, inputUri.toString()) .putString(KEY_DISPLAY_NAME, displayName) - .putLong(KEY_SIZE_BYTES, sizeBytes) + .apply { sizeBytes?.let { putLong(KEY_SIZE_BYTES, it) } } .putString(KEY_CONTAINER, spec.container.name) .putString(KEY_VIDEO_CODEC, spec.videoCodec.name) .putString(KEY_AUDIO_CODEC, spec.audioCodec.name) diff --git a/app/src/main/java/org/libremediaconverter/work/FailureOutcome.kt b/app/src/main/java/org/libremediaconverter/work/FailureOutcome.kt index ab84b96..84c491c 100644 --- a/app/src/main/java/org/libremediaconverter/work/FailureOutcome.kt +++ b/app/src/main/java/org/libremediaconverter/work/FailureOutcome.kt @@ -1,27 +1,104 @@ package org.libremediaconverter.work +import android.app.ForegroundServiceStartNotAllowedException import androidx.work.WorkInfo /** - * Decides whether a failed job should be retried or reported as failed. + * Decides what a worker should do about a failure: try again, give up, or report it. * - * A pure function rather than a branch inside the worker, because the case that matters - * cannot be provoked in a test: the foreground-service budget is six hours per - * twenty-four, and no test is going to exhaust it. Isolating the decision means the - * rule itself can still be verified on the JVM, even though the condition that triggers - * it in production cannot be reproduced. + * A pure function rather than a branch inside the worker, because none of the cases that matter + * can be provoked in a test. The foreground-service budget is six hours per twenty-four, and no + * test is going to exhaust it. A refused foreground-service start needs a process death, a + * WorkManager recovery and a real system to do the refusing. Isolating the decision means the + * rules themselves can still be verified on the JVM, even though the conditions that trigger them + * in production cannot be reproduced. */ enum class FailureOutcome { - /** Budget exhausted, not a real failure — the work is still valid, so try later. */ + /** Not a real failure — the work is still valid, so try later. */ RETRY, + /** + * The system would not let the job start, and has refused often enough that another retry + * would only postpone the same answer. + * + * Distinct from [FAIL] because nothing about the *job* is wrong: the file is fine, the settings + * are fine, and running the same job with the app open would work. What the user needs is that + * instruction, not "conversion failed", which is why the message comes from here rather than + * from the exception. + */ + FOREGROUND_DENIED, + /** A genuine failure; report it to the user. */ FAIL, ; companion object { - fun forStopReason(stopReason: Int): FailureOutcome = - if (stopReason == WorkInfo.STOP_REASON_FOREGROUND_SERVICE_TIMEOUT) RETRY else FAIL + + /** + * What to tell the user once a denied start has stopped being worth retrying. + * + * Deliberately actionable rather than descriptive. The single thing that grants an app + * permission to start a foreground service is being in the foreground, so "open the app" + * is not filler — it is the fix. + */ + const val FOREGROUND_DENIED_MESSAGE: String = + "Android would not let this run in the background. Open the app and start it again." + + /** + * How many attempts a denied foreground start gets before the job is failed. + * + * Retrying is right — the denial says *not now*, and the allowance arrives the moment the + * user next opens the app — but unbounded retrying is not. WorkManager never gives up on + * its own, so a job nobody comes back for would sit in the queue waking the device forever + * while the screen said "paused" and never explained itself. + * + * Ten, against the default backoff rather than against a round number. Backoff is + * exponential from `WorkRequest.DEFAULT_BACKOFF_DELAY_MILLIS` (30 s), doubling per attempt + * and clamped at `MAX_BACKOFF_MILLIS` (5 h), so ten attempts span + * 30 s + 1 m + 2 m + … + 4 h 16 m ≈ **8 h 30 m** — long enough to cover a normal day's + * gap between opening the app. + * + * What giving up buys is bounded, and worth stating rather than assuming. The message is + * carried on a FAILED job, and [Reattachment] excludes FAILED, so a user who was not + * watching when the eleventh attempt ran will find an empty screen rather than the + * explanation. What the bound reliably buys is the *end* of the retrying: no job waking + * the device every five hours for a device state that is not going to change on its own. + * + * The counter is [androidx.work.ListenableWorker.getRunAttemptCount], which counts *every* + * attempt, not only denied ones — WorkManager exposes no other. So a very long transcode + * that has already been retried ten times by the foreground-service budget will fail on its + * first denial rather than getting ten of its own. That is accepted rather than overlooked: + * separating the two would mean persisting a counter of our own, and a job that has already + * been attempted ten times has had its chances by any measure. The budget's own retries are + * unaffected — see the timeout branch in [forFailure], which ignores the count entirely. + */ + const val MAX_FOREGROUND_START_ATTEMPTS: Int = 10 + + /** + * @param stopReason [androidx.work.ListenableWorker.getStopReason], which reports what (if + * anything) asked the worker to stop. + * @param cause the exception that ended the attempt, when there was one. + * @param runAttemptCount how many times this job has already run. + */ + fun forFailure(stopReason: Int, cause: Throwable? = null, runAttemptCount: Int = 0): FailureOutcome = when { + // `mediaProcessing` allows six hours out of every twenty-four, shared across the app. + // When that runs out the right response is to retry later rather than tell the user + // the conversion failed -- the work is still valid, there is simply no budget now. + stopReason == WorkInfo.STOP_REASON_FOREGROUND_SERVICE_TIMEOUT -> RETRY + + // Something else asked the worker to stop, so whatever exception it was holding at the + // time describes that stop rather than a reason of its own. Retrying past a + // cancellation would ignore the user; retrying past a constraint would spin. + stopReason != WorkInfo.STOP_REASON_NOT_STOPPED -> FAIL + + // Matched on the exact class, never on its supertype. It extends IllegalStateException, + // and so do plenty of ordinary failures from the muxers and the platform extractor -- + // catching the supertype would retry every one of them for eight hours. + cause is ForegroundServiceStartNotAllowedException -> + if (runAttemptCount < MAX_FOREGROUND_START_ATTEMPTS) RETRY else FOREGROUND_DENIED + + else -> FAIL + } } } diff --git a/app/src/main/java/org/libremediaconverter/work/JobSnapshots.kt b/app/src/main/java/org/libremediaconverter/work/JobSnapshots.kt new file mode 100644 index 0000000..a7ed79c --- /dev/null +++ b/app/src/main/java/org/libremediaconverter/work/JobSnapshots.kt @@ -0,0 +1,44 @@ +package org.libremediaconverter.work + +import androidx.work.WorkManager +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.flow.first +import kotlinx.coroutines.withContext +import java.io.File + +/** + * Everything WorkManager still knows about one kind of this app's jobs. + * + * The framework half of reattachment, kept deliberately free of decisions: it queries, reads + * fields across, and stats one file. Which job to pick — and whether any of them is worth + * picking — is [Reattachment.choose], which is pure and tested on the JVM. + * + * `tag` is the worker's class name, which needs no cooperation from the enqueueing code: + * `WorkRequest.Builder` seeds every request's tag set with `workerClass.name`. That is what + * makes work enqueued by a previous run of the app — or a previous version of it — findable + * at all. R8 keeps those names (`-keepnames class * extends androidx.work.ListenableWorker`, + * from work-runtime's own consumer rules), so the key is stable in a minified build. + * + * Runs on [Dispatchers.IO] because it stats a file, and it does so here rather than at the + * call site so no caller can forget. + */ +suspend fun WorkManager.jobSnapshots(tag: String, outputPathKey: String): List = + withContext(Dispatchers.IO) { + getWorkInfosByTagFlow(tag).first().map { info -> + val path = info.outputData.getString(outputPathKey) + // An empty file is treated as no file: it would publish as a zero-byte "conversion" + // rather than fail, which is worse than not offering it at all. + val output = path?.let(::File)?.takeIf { it.isFile && it.length() > 0L } + JobSnapshot( + id = info.id, + state = info.state, + runAttemptCount = info.runAttemptCount, + outputPath = path, + outputExists = output != null, + // Read here because this is the only place a clock is available at all: it is + // the sole way to tell two of the app's results apart. See Reattachment.choose. + outputModifiedAt = output?.lastModified() ?: 0L, + tags = info.tags, + ) + } + } diff --git a/app/src/main/java/org/libremediaconverter/work/JobTags.kt b/app/src/main/java/org/libremediaconverter/work/JobTags.kt new file mode 100644 index 0000000..52b6320 --- /dev/null +++ b/app/src/main/java/org/libremediaconverter/work/JobTags.kt @@ -0,0 +1,46 @@ +package org.libremediaconverter.work + +/** + * The little that has to travel with a job so the UI can describe it after a restart. + * + * `WorkInfo` exposes a job's id, state, tags, progress and output — never the `Data` it was + * enqueued with. So a ViewModel that finds a job it did not start can see *that* there is a + * conversion, and where its output went, but not what file it was converting. Tags are the + * only channel WorkManager gives back, which is why the display name and size ride on them. + * + * Deliberately not carried: the input `Uri`. It is the one field nothing in a reattached state + * reads, and a `content://` grant taken by a picker in a process that no longer exists is not + * something to hand back to the user as if it still worked. + * + * Values are read back leniently — a missing or malformed tag is null, never an exception. + * Work enqueued by an older version of the app carries none of these, and it is exactly the + * work most likely to still be sitting in the queue when this code first runs. + */ +object JobTags { + + fun displayName(name: String): String = DISPLAY_NAME + name + + fun sizeBytes(bytes: Long): String = SIZE_BYTES + bytes + + fun inputCount(count: Int): String = INPUT_COUNT + count + + fun displayNameOf(tags: Set): String? = valueOf(tags, DISPLAY_NAME) + + fun sizeBytesOf(tags: Set): Long? = valueOf(tags, SIZE_BYTES)?.toLongOrNull() + + fun inputCountOf(tags: Set): Int? = valueOf(tags, INPUT_COUNT)?.toIntOrNull() + + /** + * Matching on the whole tag rather than searching within it is what makes a display name + * safe to carry verbatim: a file called `lmc.size-bytes:9` becomes the tag + * `lmc.display-name:lmc.size-bytes:9`, which no other prefix matches. + */ + private fun valueOf(tags: Set, prefix: String): String? = + tags.firstOrNull { it.startsWith(prefix) }?.removePrefix(prefix) + + // Namespaced so they cannot collide with the worker class name WorkManager tags every + // request with, which is what makes the job findable in the first place. + private const val DISPLAY_NAME = "lmc.display-name:" + private const val SIZE_BYTES = "lmc.size-bytes:" + private const val INPUT_COUNT = "lmc.input-count:" +} diff --git a/app/src/main/java/org/libremediaconverter/work/Reattachment.kt b/app/src/main/java/org/libremediaconverter/work/Reattachment.kt new file mode 100644 index 0000000..106e3d3 --- /dev/null +++ b/app/src/main/java/org/libremediaconverter/work/Reattachment.kt @@ -0,0 +1,181 @@ +package org.libremediaconverter.work + +import androidx.work.WorkInfo +import java.util.UUID + +/** + * One of the app's own jobs, as WorkManager last reported it. + * + * Only the fields the reattachment decision reads. [outputExists] is deliberately a + * `Boolean` rather than a `File`: whether the staged output is still on disk is the one + * input to that decision that cannot be answered without touching the filesystem, so the + * edge answers it and the rule stays testable on the JVM. + */ +data class JobSnapshot( + val id: UUID, + val state: WorkInfo.State, + val runAttemptCount: Int, + /** Where the worker said it left the output, for a job that got that far. */ + val outputPath: String?, + /** Whether [outputPath] still names a non-empty file. Answered from disk by the caller. */ + val outputExists: Boolean, + /** + * When that file was last written, or 0 when there is none. + * + * The only ordering available anywhere in this data: [WorkInfo] carries no timestamp, and + * the caller is already stat'ing the file. + */ + val outputModifiedAt: Long = 0L, + /** The job's tags, carrying what [JobTags] put there. Not read by the decision. */ + val tags: Set = emptySet(), +) + +/** + * Which of the app's own jobs a freshly created ViewModel should pick up, if any, and whether + * that job can be trusted to describe itself. + * + * A pure function rather than a branch inside the ViewModel, for the same reason as + * [FailureOutcome]: the condition that matters cannot be provoked in a test. It needs the + * process to be reclaimed while a job or its unsaved result is still around, which means a + * device, an `am kill` and a wait. Isolating the choice means the rule itself is verified on + * the JVM even though the situation that calls for it is not reproducible here. + * + * "Unfinished" here means unfinished *from the user's point of view*, not + * [WorkInfo.State.isFinished]. A conversion that succeeded and was never saved is finished + * work with a full-size file sitting in the cache and no route to it — that is the case this + * whole mechanism exists for, and it is why [WorkInfo.State.SUCCEEDED] is a candidate here. + */ +sealed interface Reattachment { + + /** The job the UI should pick up. */ + val job: JobSnapshot + + /** Exactly one job explains what is on screen, so its tags describe it. */ + data class Certain(override val job: JobSnapshot) : Reattachment + + /** + * Several finished jobs name the same staged file, so the file is reachable but nothing can + * say which job produced it. + * + * Observed on a device rather than imagined: a tag query in a fresh process returned two + * SUCCEEDED jobs whose output paths were both `…/conversions/input_converted.mp4`, with one + * file on disk. Nothing gave a job a staging path of its own — the name came from the input's + * display name — so a later conversion overwrote an earlier one's output while both jobs went + * on reporting that path as their result. + * + * The file is not the ambiguous part: whichever entry is picked, the user is offered the + * bytes actually on disk, which is the thing that would otherwise be lost. What cannot be + * recovered is which job wrote them, so the caller is told not to describe it. A card + * labelled with the other job's input would be a confident lie, where a neutral label is + * merely thin. + * + * [org.libremediaconverter.convert.StagingNames] has since keyed staging on the job id, so + * nothing enqueued from now on can alias. This stays because the queue outlives the change: + * WorkManager keeps finished work for about a week, and the jobs most likely to be sitting in + * it when this code first runs are exactly the ones named the old way. + */ + data class Ambiguous(override val job: JobSnapshot) : Reattachment + + companion object { + + /** + * Picks the one job to reattach to, or null when there is nothing worth showing. + * + * Excluded outright: + * + * - **[WorkInfo.State.CANCELLED]** — the user already said no. Reattaching would undo + * that. + * - **[WorkInfo.State.FAILED]** — nothing to act on, and nothing marks a failure as + * seen, so it would reappear on every launch. The reason this mattered has since been + * removed: a worker interrupted by process death used to come back FAILED rather than + * retried, because the restart's `setForeground` was refused as a background + * foreground-service start and the throw escaped `doWork()`. It now retries, so such + * failures are rare again rather than ordinary. The exclusion stands on its own — + * there is still nothing a FAILED job offers the user, and a job that exhausts its + * retries is a job whose message this cannot show either. + * - **[WorkInfo.State.SUCCEEDED] with no output file** — either it was saved, which + * deletes the staged copy, or the OS reclaimed the cache. Offering a Save button for a + * file that is gone turns a recoverable job into a failed save. + * + * Nothing filters by age, and nothing can. WorkManager keeps finished work for about a + * week and prunes on its own schedule, so a tag query in a fresh process routinely + * returns completed jobs from earlier sessions, and [WorkInfo] carries no timestamp to + * sort them by. Whether the staged file is still there is the only signal separating a + * result still worth offering from one already dealt with, which is why that check + * carries the weight here. + * + * It is also the seam for a neighbouring defect: a result the user dismissed with "Start + * over" currently keeps its staged file, so today it can be offered again on the next + * launch. Nothing here changes when that is fixed — the file stops existing and the job + * stops qualifying. + * + * Ranked, when more than one qualifies: + * + * 1. a job that is running now, + * 2. a job waiting to be retried, which has already done part of the work, + * 3. a job queued and not yet started, + * 4. a finished result still on disk. + * + * Live work outranks a finished result because a running job is holding a foreground + * notification: someone opening the app while that notification is in the shade expects + * to find that conversion, not a result from yesterday. It is also the right answer when + * the running job is overwriting the older one's staged file, which work enqueued before + * per-job staging names can still do. + * + * Within a rank the **newest staged file** wins, and that is not a detail. Losing a tie + * is not the same as waiting for the next launch: the query has no `ORDER BY`, so its + * order is unspecified but stable, and an arbitrary winner would keep winning every + * launch while the other result stayed unreachable for as long as its file existed. Two + * results at different paths is reachable — dismiss one with "Start over", which leaves + * its file behind, then convert something else and do not save it. + * + * The ordering is the file's own modification time because there is nothing else: + * [WorkInfo] carries no timestamp at all, and the file is already being stat'ed for + * [JobSnapshot.outputExists]. The job that wrote most recently is the one the user is + * likeliest to be waiting for. It is a heuristic to the extent that a clock can move + * backwards, which is a better failure than an order that is unspecified and + * systematically repeats itself. + * + * Two kinds of tie survive that and both are meant to. Live jobs have written no file, so + * they have no timestamp and keep the query's order — and two of them are not reachable + * from the UI today, since every state that can start a job is left the moment it does. + * Aliases share a file and therefore share its timestamp, so they stay tied, which is + * exactly right: the pick decides nothing about which bytes the user gets, and + * [Ambiguous] answers the part that is genuinely unknown. + */ + fun choose(jobs: List): Reattachment? { + // minWithOrNull keeps the first of equal elements, so the query's order is what + // breaks a tie the comparator leaves — deliberately, per the ordering notes above. + val chosen = jobs + .mapNotNull { job -> rank(job)?.let { rank -> rank to job } } + .minWithOrNull( + compareBy> { (rank, _) -> rank } + .thenByDescending { (_, job) -> job.outputModifiedAt }, + ) + ?.second + ?: return null + + // Only a job that finished names a file, so only one of those can be aliased. + val path = chosen.outputPath ?: return Certain(chosen) + val aliases = jobs.filter { it.id != chosen.id && it.outputPath == path && rank(it) != null } + // Aliases carrying identical tags describe the same input, so nothing turns on which + // of them wrote the file and the label is safe either way. That is the ordinary case: + // the same file converted twice. + return if (aliases.all { it.tags == chosen.tags }) Certain(chosen) else Ambiguous(chosen) + } + + private fun rank(job: JobSnapshot): Int? = when (job.state) { + WorkInfo.State.RUNNING -> RUNNING + // ENQUEUED after a run means a retry is pending — the reading observe() takes too. + WorkInfo.State.ENQUEUED -> if (job.runAttemptCount > 0) RETRYING else QUEUED + WorkInfo.State.BLOCKED -> QUEUED + WorkInfo.State.SUCCEEDED -> if (job.outputPath != null && job.outputExists) RESULT else null + WorkInfo.State.FAILED, WorkInfo.State.CANCELLED -> null + } + + private const val RUNNING = 0 + private const val RETRYING = 1 + private const val QUEUED = 2 + private const val RESULT = 3 + } +} diff --git a/app/src/main/res/xml/backup_rules.xml b/app/src/main/res/xml/backup_rules.xml deleted file mode 100644 index 4df9255..0000000 --- a/app/src/main/res/xml/backup_rules.xml +++ /dev/null @@ -1,13 +0,0 @@ - - - - \ No newline at end of file diff --git a/app/src/main/res/xml/data_extraction_rules.xml b/app/src/main/res/xml/data_extraction_rules.xml index 9ee9997..c890639 100644 --- a/app/src/main/res/xml/data_extraction_rules.xml +++ b/app/src/main/res/xml/data_extraction_rules.xml @@ -1,19 +1,45 @@ - + + + + - + + + + - --> - \ No newline at end of file + diff --git a/app/src/test/java/org/libremediaconverter/AppRootRestorationTest.kt b/app/src/test/java/org/libremediaconverter/AppRootRestorationTest.kt new file mode 100644 index 0000000..b0e1c78 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/AppRootRestorationTest.kt @@ -0,0 +1,113 @@ +package org.libremediaconverter + +import androidx.compose.foundation.layout.Box +import androidx.compose.material3.windowsizeclass.WindowWidthSizeClass +import androidx.compose.runtime.getValue +import androidx.compose.runtime.mutableStateOf +import androidx.compose.runtime.setValue +import androidx.compose.ui.platform.testTag +import androidx.compose.ui.test.assertIsSelected +import androidx.compose.ui.test.junit4.StateRestorationTester +import androidx.compose.ui.test.junit4.v2.createComposeRule +import androidx.compose.ui.test.onNodeWithTag +import androidx.compose.ui.test.onNodeWithText +import androidx.compose.ui.test.performClick +import androidx.media3.common.util.UnstableApi +import org.junit.Rule +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner + +/** + * The selected tab has to survive activity recreation, not just recomposition. + * + * `remember` covers recomposition only, and `MainActivity` declares no `configChanges`, so + * every rotation and every resize destroys and recreates the Activity. That is the exact + * case [AppRoot]'s own KDoc says the shell exists for: from targetSdk 37 the app is resized + * and rotated whether or not it is ready. + * + * [StateRestorationTester] is the tool for it -- `emulateSavedInstanceStateRestore()` + * disposes the composition and rebuilds it, so anything held only by `remember` is gone and + * only saved state comes back. It is Compose's own stand-in for the recreation rather than + * the real thing: it saves into an in-memory map instead of parcelling through a `Bundle`, + * so it proves `rememberSaveable` is being used -- not that a particular saved + * representation survives a `Bundle` round trip. A JVM round-trip test on the + * saver covers the representation. + * + * Robolectric rather than the instrumented suite, deliberately. The instrumented tests + * cannot run on the development host at all (see CLAUDE.md), and a red test nobody can + * execute is not a loop anyone can work in. + */ +@UnstableApi +@RunWith(RobolectricTestRunner::class) +class AppRootRestorationTest { + + @get:Rule + val composeRule = createComposeRule() + + private val restoration = StateRestorationTester(composeRule) + + /** + * The stub screen is matched by a test tag rather than by text: the label on the bar + * ("Join") and the enum constant ("JOIN") differ only in case, and a matcher that could + * pick up either is not an assertion. + */ + private fun tagFor(destination: Destination) = "content:${destination.name}" + + private fun assertShowing(destination: Destination) { + composeRule.onNodeWithTag(tagFor(destination)).assertExists() + composeRule.onNodeWithText(destination.label).assertIsSelected() + } + + private fun setShell(width: () -> WindowWidthSizeClass) { + restoration.setContent { + AppRoot(width()) { destination, modifier -> + Box(modifier.testTag(tagFor(destination))) + } + } + } + + @Test + fun `the selected tab survives recreation on a phone`() { + setShell { WindowWidthSizeClass.Compact } + assertShowing(Destination.CONVERT) + + composeRule.onNodeWithText(Destination.JOIN.label).performClick() + assertShowing(Destination.JOIN) + + restoration.emulateSavedInstanceStateRestore() + + assertShowing(Destination.JOIN) + } + + @Test + fun `the selected tab survives recreation on the rail layout`() { + setShell { WindowWidthSizeClass.Expanded } + + composeRule.onNodeWithText(Destination.JOIN.label).performClick() + assertShowing(Destination.JOIN) + + restoration.emulateSavedInstanceStateRestore() + + assertShowing(Destination.JOIN) + } + + /** + * The real rotation: the width class changes across the recreation, so the shell comes + * back as a rail where it went out as a bottom bar. The tab still has to be the one the + * user chose. + */ + @Test + fun `the selected tab survives a rotation that also changes the width class`() { + var width by mutableStateOf(WindowWidthSizeClass.Compact) + setShell { width } + + composeRule.onNodeWithText(Destination.JOIN.label).performClick() + assertShowing(Destination.JOIN) + + width = WindowWidthSizeClass.Expanded + restoration.emulateSavedInstanceStateRestore() + + assertShowing(Destination.JOIN) + } +} diff --git a/app/src/test/java/org/libremediaconverter/DestinationSaverTest.kt b/app/src/test/java/org/libremediaconverter/DestinationSaverTest.kt new file mode 100644 index 0000000..3f13363 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/DestinationSaverTest.kt @@ -0,0 +1,45 @@ +package org.libremediaconverter + +import androidx.compose.runtime.saveable.SaverScope +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull +import org.junit.Test + +/** + * What [AppRootRestorationTest] cannot see. + * + * `StateRestorationTester` saves into an in-memory map, so it proves the shell uses + * `rememberSaveable` and stops there -- it would be just as green if the saved value were an + * ordinal, or if the enum were left to `autoSaver`. The saved *representation* is a separate + * decision with separate consequences, and this is where it is pinned. + */ +class DestinationSaverTest { + + /** `canBeSaved` is the host registry's question; a String always can. */ + private val scope = SaverScope { true } + + private fun save(destination: Destination): Any? = with(DestinationSaver) { scope.save(destination) } + + @Test + fun `a destination is saved as its constant name, not its position`() { + // JOIN is ordinal 1. If this ever reads `1`, inserting a tab above it silently + // redefines every value already saved. + assertEquals("CONVERT", save(Destination.CONVERT)) + assertEquals("JOIN", save(Destination.JOIN)) + } + + @Test + fun `every destination survives the round trip`() { + Destination.entries.forEach { destination -> + assertEquals(destination, DestinationSaver.restore(save(destination) as String)) + } + } + + @Test + fun `a name no longer in the enum restores to nothing`() { + // A downgrade, or a renamed constant, leaves a name that no longer resolves. + // Returning null is what makes rememberSaveable fall back to the default tab + // instead of throwing on the way back from a rotation. + assertNull(DestinationSaver.restore("SETTINGS")) + } +} diff --git a/app/src/test/java/org/libremediaconverter/convert/ConversionViewModelCleanupTest.kt b/app/src/test/java/org/libremediaconverter/convert/ConversionViewModelCleanupTest.kt new file mode 100644 index 0000000..8b5a8a7 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/ConversionViewModelCleanupTest.kt @@ -0,0 +1,118 @@ +package org.libremediaconverter.convert + +import android.app.Application +import android.net.Uri +import androidx.media3.common.util.UnstableApi +import androidx.work.workDataOf +import kotlinx.coroutines.Dispatchers +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.libremediaconverter.model.InputProbe +import org.libremediaconverter.work.ConversionWorker +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment +import java.io.File + +/** + * That `reset()` actually deletes — the wiring, not the tool. + * + * D2 was never that `OutputPublisher` could not delete a file. It was that "Start over" + * dropped the reference without calling anything. So this drives the real ViewModel through + * a real `WorkManager` to `Converted` and then asserts on the filesystem. + */ +@UnstableApi +@RunWith(RobolectricTestRunner::class) +class ConversionViewModelCleanupTest { + + private lateinit var app: Application + private lateinit var publisher: RecordingPublisher + private lateinit var staged: File + + @Before + fun setUp() { + app = RuntimeEnvironment.getApplication() + publisher = RecordingPublisher(app) + ConversionDependencies.publisher = { publisher } + // MediaProbe spawns FFprobe, whose loader throws a bare java.lang.Error with no + // native library present. Without this the pick dies before the test starts. + ConversionDependencies.probe = { _, _ -> InputProbe() } + + staged = publisher.createStagingFile("holiday.mp4").apply { writeBytes(ByteArray(4096)) } + installTestWorkManager(app, workDataOf(ConversionWorker.KEY_OUTPUT_PATH to staged.absolutePath)) + } + + @After + fun tearDown() { + ConversionDependencies.reset() + } + + @Test + fun `start over on a finished conversion deletes the staged file`() { + val viewModel = convertedViewModel() + assertTrue("the conversion should have produced a staged file", staged.exists()) + + viewModel.reset() + + assertEquals(ConversionState.Idle, viewModel.state.value) + assertEquals("reset() should have discarded exactly the staged file", listOf(staged), publisher.discarded) + assertFalse("Start over must not leave a full-size copy in cache", staged.exists()) + } + + @Test + fun `reset after a successful save does not try to delete again`() { + val viewModel = convertedViewModel() + viewModel.save(DESTINATION) + awaitState(viewModel.state, "Saved") { it is ConversionState.Saved } + + // save() deletes the staged file itself, through File.delete() rather than through + // the publisher, so assert the disappearance as well as the absent second discard. + assertFalse("a successful save should have removed the staged file", staged.exists()) + + viewModel.reset() + + // Discarding again would be a delete aimed at a path this ViewModel no longer owns. + assertEquals(emptyList(), publisher.discarded) + assertFalse(staged.exists()) + } + + @Test + fun `a failed save keeps the staged file, and a later reset collects it`() { + val viewModel = convertedViewModel() + publisher.publishFailure = IllegalStateException("destination volume full") + + viewModel.save(DESTINATION) + awaitState(viewModel.state, "Failed") { it is ConversionState.Failed } + + // The deliberate decision, pinned: the staged file may be the only copy of an hour + // of transcoding, and the destination did not receive it. + assertTrue("a failed save must not destroy the only copy", staged.exists()) + assertEquals(emptyList(), publisher.discarded) + + // Failed carries no file reference at all, so this only works because the handle is + // a ViewModel field rather than something read back out of the state machine. + viewModel.reset() + + assertEquals(listOf(staged), publisher.discarded) + assertFalse(staged.exists()) + } + + /** A ViewModel driven all the way to [ConversionState.Converted]. */ + private fun convertedViewModel(): ConversionViewModel { + // Unconfined so reset()'s delete runs inline instead of on a real IO thread. + val viewModel = ConversionViewModel(app, Dispatchers.Unconfined) + viewModel.onInputPicked(Uri.parse("content://test/holiday.mp4")) + awaitState(viewModel.state, "Ready") { it is ConversionState.Ready } + viewModel.convert() + awaitState(viewModel.state, "Converted") { it is ConversionState.Converted } + return viewModel + } + + private companion object { + val DESTINATION: Uri = Uri.parse("content://test/destination.mp4") + } +} diff --git a/app/src/test/java/org/libremediaconverter/convert/ConversionViewModelNamingTest.kt b/app/src/test/java/org/libremediaconverter/convert/ConversionViewModelNamingTest.kt new file mode 100644 index 0000000..bebd281 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/ConversionViewModelNamingTest.kt @@ -0,0 +1,124 @@ +package org.libremediaconverter.convert + +import android.app.Application +import android.net.Uri +import androidx.media3.common.util.UnstableApi +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.model.InputProbe +import org.libremediaconverter.model.OutputFormat +import org.libremediaconverter.work.ConversionWorker +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment +import java.io.File + +/** + * That the save dialog offers the job's name, not the picker's. + * + * `save()` and `suggestedOutputName()` both built the name out of `_settings.value.spec` — the + * settings as they stand *now*, which is not necessarily the spec the job ran with. Today the + * pickers are only drawn in the `Ready` state, so they cannot move between enqueue and save, and + * the name comes out right by accident. Two things make the accident stop: a job picked up by + * `reattach()`, whose spec was never in this ViewModel's settings at all, and any future in which + * the pickers stay live while a conversion runs. + * + * `suggestedOutputName()` is gone rather than fixed: the answer belongs on the state, which the + * screen already collects, and an accessor that recomputed it would only be a second place for it + * to be wrong. + */ +@UnstableApi +@RunWith(RobolectricTestRunner::class) +class ConversionViewModelNamingTest { + + private lateinit var app: Application + private lateinit var publisher: RecordingPublisher + private lateinit var staged: File + + @Before + fun setUp() { + app = RuntimeEnvironment.getApplication() + publisher = RecordingPublisher(app) + ConversionDependencies.publisher = { publisher } + ConversionDependencies.probe = { _, _ -> InputProbe() } + + staged = publisher.createStagingFile("staged-under-a-job-id.mp3").apply { writeBytes(ByteArray(4096)) } + // What the worker reports for an MP3 job. The staged name is opaque and says nothing about + // either; these two strings are the only place the job's own output describes itself. + installTestWorkManager( + app, + workDataOf( + ConversionWorker.KEY_OUTPUT_PATH to staged.absolutePath, + ConversionWorker.KEY_SUGGESTED_NAME to "holiday_converted.mp3", + ConversionWorker.KEY_MIME_TYPE to "audio/mpeg", + ), + ) + } + + @After + fun tearDown() { + ConversionDependencies.reset() + } + + @Test + fun `a finished conversion carries the name and type its own job produced`() { + val converted = convertedViewModel().state.value as ConversionState.Converted + + // Not a name this ViewModel could have worked out. Its settings say MP4 + H.265, and the + // staged file is called after the job id, so both of these can only have come from the + // job's own output Data. + assertEquals("holiday_converted.mp3", converted.suggestedName) + // Wrong on its own, and worse in company: some providers rewrite a document's extension + // to match its MIME type, so an MP3 offered as video/webm can arrive with the wrong one. + assertEquals("audio/mpeg", converted.mimeType) + } + + @Test + fun `and the saved name is the job's, not the picker's as it stands now`() { + val viewModel = convertedViewModel() + + // The picker moves after the job has finished. Today the pickers are drawn only in the + // Ready state so this cannot happen through the UI -- but a reattached job is this exact + // situation arrived at differently, its spec having never been in these settings at all. + viewModel.setPreset(OutputFormat.WEBM_VP9) + viewModel.save(DESTINATION) + val saved = awaitState(viewModel.state, "Saved") { it is ConversionState.Saved } + + assertEquals("holiday_converted.mp3", (saved as ConversionState.Saved).displayName) + } + + @Test + fun `a result from before the worker reported its own name still gets one`() { + // Work enqueued by an earlier version carries neither string, and WorkManager keeps + // finished work for about a week -- so this is the ordinary case for a few days after the + // change ships, not a corner. The old derivation is kept for exactly that: it is a guess, + // but it is the same guess the app made before, and there is nothing better to hand. + installTestWorkManager(app, workDataOf(ConversionWorker.KEY_OUTPUT_PATH to staged.absolutePath)) + val viewModel = convertedViewModel() + + // "input" rather than "holiday.mp4" because no provider answers the metadata query here, + // so the ViewModel falls back to its own placeholder -- which is beside the point. What + // matters is the extension: `.mp4` is the default preset's, arrived at by the old + // derivation, and it is the only answer available for a job that reported nothing. + val converted = viewModel.state.value as ConversionState.Converted + assertEquals("input_converted.mp4", converted.suggestedName) + } + + /** A ViewModel driven all the way to [ConversionState.Converted]. */ + private fun convertedViewModel(): ConversionViewModel { + val viewModel = ConversionViewModel(app, Dispatchers.Unconfined) + viewModel.onInputPicked(Uri.parse("content://test/holiday.mp4")) + awaitState(viewModel.state, "Ready") { it is ConversionState.Ready } + viewModel.convert() + awaitState(viewModel.state, "Converted") { it is ConversionState.Converted } + return viewModel + } + + private companion object { + val DESTINATION: Uri = Uri.parse("content://test/destination.mp3") + } +} diff --git a/app/src/test/java/org/libremediaconverter/convert/ConversionViewModelProbeFailureTest.kt b/app/src/test/java/org/libremediaconverter/convert/ConversionViewModelProbeFailureTest.kt new file mode 100644 index 0000000..dfb5079 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/ConversionViewModelProbeFailureTest.kt @@ -0,0 +1,154 @@ +package org.libremediaconverter.convert + +import android.app.Application +import android.net.Uri +import android.os.Looper +import androidx.media3.common.util.UnstableApi +import androidx.work.workDataOf +import kotlinx.coroutines.Dispatchers +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNotNull +import org.junit.Assert.assertNull +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.libremediaconverter.model.InputKind +import org.libremediaconverter.model.InputProbe +import org.libremediaconverter.work.ConversionWorker +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment +import org.robolectric.Shadows.shadowOf +import java.util.concurrent.TimeUnit + +/** + * That a probe which throws leaves a screen the user can act on, not a dead coroutine. + * + * [MediaProbeNativeLoadTest] covers the boundary itself. This covers the other half of the + * same defect: `onInputPicked` runs inside `viewModelScope.launch`, so anything the probe + * throws and does not handle leaves the launch with no result at all — the file card never + * fills in, and on a device the default handler takes the process down. + * + * The seam is what makes that testable. Injecting a prober that throws reproduces the + * condition exactly, without depending on which types FFmpegKit happens to throw today. + */ +@UnstableApi +@RunWith(RobolectricTestRunner::class) +class ConversionViewModelProbeFailureTest { + + private lateinit var app: Application + + @Before + fun setUp() { + app = RuntimeEnvironment.getApplication() + installTestWorkManager(app, workDataOf(ConversionWorker.KEY_OUTPUT_PATH to "/dev/null")) + } + + @After + fun tearDown() { + ConversionDependencies.reset() + } + + /** + * The exact observed failure: FFmpegKit's loader rethrows a bare [Error] whose cause is + * the `UnsatisfiedLinkError` `System.loadLibrary` raised. + */ + @Test + fun `a native load failure during the probe reports an unreadable file`() { + ConversionDependencies.probe = { _, _ -> + throw Error( + "FFmpegKit failed to start on brand: robolectric.", + UnsatisfiedLinkError("dlopen failed: library \"libffmpegkit.so\" not found"), + ) + } + + val probe = pickedProbe() + + assertNotNull("the pick must finish; a thrown Error used to abandon the launch", probe) + assertEquals(InputKind.UNPARSEABLE, probe?.kind) + assertEquals(InputProbe.UNPARSEABLE, probe?.videoCodec) + } + + /** Every touch after the first throws this instead, so the guard has to cover it too. */ + @Test + fun `a NoClassDefFoundError from a poisoned class reports an unreadable file`() { + ConversionDependencies.probe = { _, _ -> + throw NoClassDefFoundError("Could not initialize class com.arthenica.ffmpegkit.FFmpegKitConfig") + } + + assertEquals(InputKind.UNPARSEABLE, pickedProbe()?.kind) + } + + /** + * The line the guard must not cross. + * + * `MediaProbe` spawns a native process, so an [OutOfMemoryError] raised in it is a real + * one about this JVM, not a report about the file. Swallowing it would turn "the device + * is out of memory" into "this video looks unreadable" and let the app carry on in a + * state it cannot honour — which is the regression a blanket `catch (Throwable)` would + * have introduced, and the reason this defect was left open rather than fixed carelessly. + */ + @Test + fun `an OutOfMemoryError is not swallowed`() { + ConversionDependencies.probe = { _, _ -> throw OutOfMemoryError("Failed to allocate 512 MB") } + + val viewModel = ConversionViewModel(app, Dispatchers.Unconfined) + viewModel.onInputPicked(INPUT) + + // The observable difference, and the reason this is asserted on state rather than on a + // caught throwable: the probe hop is on Dispatchers.IO, so an error that escapes lands + // on that thread's handler rather than at this call. What must not happen is the card + // filling in with an "unreadable" verdict the app would then act on. + val settled = settle(viewModel) + // `sizeBytes = null`, not `0L`: no provider is registered for this authority, so the + // metadata query returns nothing and the descriptor cannot be opened either. That is the + // unknown, and it stopped being spelled the same way as "empty" -- see [InputQuery]. + assertEquals(ConversionState.Ready(InputFile(INPUT, "input", sizeBytes = null)), settled) + assertNull("an OOM must not be reported as a probe result", (settled as ConversionState.Ready).input.probe) + } + + /** A working probe is untouched by any of this. */ + @Test + fun `a probe that succeeds still fills the card in`() { + ConversionDependencies.probe = { _, _ -> InputProbe(videoCodec = "h264", kind = InputKind.VIDEO) } + + assertEquals("h264", pickedProbe()?.videoCodec) + } + + /** + * Drives a real pick and returns the probe the card ended up with. + * + * Asserting on the probe rather than merely on `Ready` is deliberate: `onInputPicked` + * sets `Ready` *before* it probes, so a test that only checked the state would have + * passed against the unguarded code. + */ + private fun pickedProbe(): InputProbe? { + val viewModel = ConversionViewModel(app, Dispatchers.Unconfined) + viewModel.onInputPicked(INPUT) + val ready = awaitState(viewModel.state, "Ready with a probe") { + it is ConversionState.Ready && it.input.probe != null + } + assertNull("nothing here should reach a terminal failure", (ready as? ConversionState.Failed)) + return (ready as ConversionState.Ready).input.probe + } + + /** + * Pumps the looper the way [awaitState] does, but for a fixed span and without requiring + * anything to happen — here "the pick never came back" is the expected outcome, so there + * is no predicate to wait on. + */ + private fun settle(viewModel: ConversionViewModel): ConversionState { + val deadline = System.nanoTime() + TimeUnit.MILLISECONDS.toNanos(SETTLE_MS) + while (System.nanoTime() < deadline) { + shadowOf(Looper.getMainLooper()).idle() + Thread.sleep(POLL_MS) + } + return viewModel.state.value + } + + private companion object { + val INPUT: Uri = Uri.parse("content://test/holiday.mp4") + const val SETTLE_MS = 500L + const val POLL_MS = 5L + } +} diff --git a/app/src/test/java/org/libremediaconverter/convert/MediaProbeNativeLoadTest.kt b/app/src/test/java/org/libremediaconverter/convert/MediaProbeNativeLoadTest.kt new file mode 100644 index 0000000..7ca591f --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/MediaProbeNativeLoadTest.kt @@ -0,0 +1,93 @@ +package org.libremediaconverter.convert + +import android.net.Uri +import com.arthenica.ffmpegkit.FFmpegKitConfig +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Test +import org.junit.runner.RunWith +import org.libremediaconverter.model.InputKind +import org.libremediaconverter.model.InputProbe +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment + +/** + * That a failed native load is reported, not thrown. + * + * The JVM is the only place this is reachable: there are no `.so` files here by + * construction, which is exactly the shape a corrupted install or an ABI mismatch has on a + * device. So the condition that cannot be provoked on working hardware is free here, and + * these tests are the only ones that can exercise it at all. + */ +@RunWith(RobolectricTestRunner::class) +class MediaProbeNativeLoadTest { + + /** + * What the boundary actually throws, pinned against the library rather than assumed. + * + * This is the test that justifies the shape of the guard, and it contradicts the obvious + * guess. `NativeLoader.loadLibrary` catches `UnsatisfiedLinkError` from + * `System.loadLibrary` and rethrows `java.lang.Error(message, cause)` — so + * `UnsatisfiedLinkError` never escapes, and because a bare `Error` *is* an `Error`, JLS + * 12.4.2 propagates it out of the static initialiser unwrapped rather than boxing it in + * `ExceptionInInitializerError`. Catching either of those two named types would catch + * nothing at all. + * + * The second touch of the class is a different type again — `NoClassDefFoundError`, the + * JVM's own "this class already failed to initialise" — so a guard written for one shape + * lets the other through. Both are asserted, in whichever order this classloader reaches + * them. + */ + @Test + fun `loading FFmpegKit without its native library throws an Error, not an Exception`() { + val thrown: Throwable? = runCatching { FFmpegKitConfig.getLogLevel() }.exceptionOrNull() + + // The whole defect in one assertion: `catch (e: Exception)` could never have seen this. + assertTrue( + "expected the native load to fail with something no catch (e: Exception) can see, got $thrown", + thrown !is Exception, + ) + assertTrue("expected an Error, got $thrown", thrown is Error) + val error = thrown as Error + // Either the first touch (bare Error wrapping UnsatisfiedLinkError) or a later one + // (NoClassDefFoundError). Both are native-load failures; neither is a VirtualMachineError. + assertTrue( + "expected a bare Error caused by UnsatisfiedLinkError or a NoClassDefFoundError, got $error", + error is NoClassDefFoundError || error.cause is UnsatisfiedLinkError, + ) + } + + /** + * The defect itself: picking a file must not die because FFprobe could not start. + * + * `probeWithFFprobe` guarded its call with `catch (e: Exception)`, which an `Error` walks + * straight through. With neither probe able to read the file, the designed answer is the + * unparseable probe — "nothing could read it, route it to FFmpeg" — not a throw. + */ + @Test + fun `probe reports an unreadable input instead of throwing when FFprobe cannot start`() { + val probe = MediaProbe.probe(RuntimeEnvironment.getApplication(), CONTENT_URI) + + assertEquals(InputKind.UNPARSEABLE, probe.kind) + assertEquals(InputProbe.UNPARSEABLE, probe.videoCodec) + } + + /** + * The second call takes the other branch — `NoClassDefFoundError` rather than the bare + * `Error` — so a guard that covered only the first shape would still crash every pick + * after the first one. + */ + @Test + fun `a second probe is guarded too, though the JVM throws a different Error by then`() { + val first = MediaProbe.probe(RuntimeEnvironment.getApplication(), CONTENT_URI) + val second = MediaProbe.probe(RuntimeEnvironment.getApplication(), CONTENT_URI) + + assertEquals(InputKind.UNPARSEABLE, first.kind) + assertEquals(InputKind.UNPARSEABLE, second.kind) + } + + private companion object { + /** `content://` so the probe takes the SAF branch, which is what a real pick does. */ + val CONTENT_URI: Uri = Uri.parse("content://test/holiday.mp4") + } +} diff --git a/app/src/test/java/org/libremediaconverter/convert/OutputPublisherPublishTest.kt b/app/src/test/java/org/libremediaconverter/convert/OutputPublisherPublishTest.kt new file mode 100644 index 0000000..09f1adc --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/OutputPublisherPublishTest.kt @@ -0,0 +1,329 @@ +package org.libremediaconverter.convert + +import android.content.ComponentName +import android.content.ContentProvider +import android.content.ContentValues +import android.content.Context +import android.content.IntentFilter +import android.content.pm.ProviderInfo +import android.database.Cursor +import android.database.MatrixCursor +import android.net.Uri +import android.os.Bundle +import android.provider.DocumentsContract +import android.provider.OpenableColumns +import org.junit.Assert.assertArrayEquals +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertThrows +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.Robolectric +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment +import org.robolectric.Shadows.shadowOf +import java.io.File +import java.io.IOException +import java.io.OutputStream + +/** What a destination volume says when it fills up mid-write. */ +private const val NO_SPACE = "No space left on device" + +private const val DOCUMENTS_AUTHORITY = "org.libremediaconverter.test.documents" +private const val PLAIN_AUTHORITY = "org.libremediaconverter.test.plain" + +/** + * A stand-in for the provider behind a SAF destination. + * + * It answers only what `publish()` asks of a destination -- how many bytes are already there, + * and delete it -- backed by a real file so the assertions are about the filesystem rather + * than about a mock's call log alone. The rest of the `ContentProvider` surface is stubbed. + * + * Writing is deliberately NOT routed through it. Robolectric's `ShadowContentResolver` + * consults its registered-stream map before it reaches any provider, which is what lets a + * test hand out a stream that writes some bytes and then fails -- the condition this whole + * file exists for, and one a real provider cannot be asked to produce on demand. + */ +internal open class FakeSafProvider : ContentProvider() { + + override fun onCreate() = true + + override fun query( + uri: Uri, + projection: Array?, + selection: String?, + selectionArgs: Array?, + sortOrder: String?, + ): Cursor? { + val file = backingFile(uri) + if (!file.exists()) return null + return MatrixCursor(arrayOf(OpenableColumns.DISPLAY_NAME, OpenableColumns.SIZE)).apply { + addRow(arrayOf(file.name, file.length())) + } + } + + override fun call(method: String, arg: String?, extras: Bundle?): Bundle? { + if (method != METHOD_DELETE_DOCUMENT) return null + val target = extras?.getParcelable(EXTRA_URI, Uri::class.java) ?: return null + deleteRequests += target + deleteFailure?.let { throw it } + backingFile(target).delete() + return Bundle() + } + + override fun getType(uri: Uri) = "video/mp4" + + override fun insert(uri: Uri, values: ContentValues?): Uri? = null + + override fun delete(uri: Uri, selection: String?, selectionArgs: Array?) = 0 + + override fun update(uri: Uri, values: ContentValues?, selection: String?, selectionArgs: Array?) = 0 + + companion object { + // DocumentsContract.METHOD_DELETE_DOCUMENT and EXTRA_URI are hidden from the public + // SDK, so they cannot be referenced. These are the wire names + // DocumentsContract.deleteDocument() actually sends, which is what a provider sees. + const val METHOD_DELETE_DOCUMENT = "android:deleteDocument" + const val EXTRA_URI = "uri" + + /** Where the "documents" really live. Set per test to a Robolectric temp path. */ + lateinit var root: File + + /** Every delete this provider was asked for, in order. Empty is an assertion too. */ + val deleteRequests = mutableListOf() + + /** Armed by the test that needs the cleanup itself to fail. */ + var deleteFailure: RuntimeException? = null + + fun backingFile(uri: Uri) = File(root, uri.lastPathSegment.orEmpty()) + + fun reset(directory: File) { + root = directory + deleteRequests.clear() + deleteFailure = null + } + } +} + +/** + * The same provider, registered WITHOUT the documents-provider intent filter. + * + * A separate class because the package manager keys providers by component name, so two + * authorities need two components. It exists to prove the guard is a guard: a content URI + * from something that is not a documents provider must not be handed to `deleteDocument`. + */ +internal class FakePlainProvider : FakeSafProvider() + +/** + * A sink that behaves like a volume filling up. + * + * Two failure shapes, because `publish()` has to survive both: a write that throws partway, + * and a `close()` that throws while flushing -- the second arriving after `copyTo` has + * already returned successfully. + */ +private class UnreliableOutputStream( + private val sink: OutputStream, + private val failAfterBytes: Int = Int.MAX_VALUE, + private val failOnClose: Boolean = false, +) : OutputStream() { + + private var written = 0 + + override fun write(b: Int) = write(byteArrayOf(b.toByte()), 0, 1) + + override fun write(b: ByteArray, off: Int, len: Int) { + val room = failAfterBytes - written + if (room <= 0) throw IOException(NO_SPACE) + val accepted = minOf(room, len) + sink.write(b, off, accepted) + written += accepted + if (accepted < len) throw IOException(NO_SPACE) + } + + override fun flush() = sink.flush() + + override fun close() { + sink.close() + if (failOnClose) throw IOException(NO_SPACE) + } +} + +/** + * `publish()` on the paths where something goes wrong. + * + * The defect: a copy that failed partway left the bytes it had managed at the name the user + * picked, while the UI said "Could not save the file". [OutputPublisherStagingTest] covers + * the staging side of the same class; this covers the destination side, and needs a provider + * rather than a bare file because the destination is a `content://` URI and the fix turns on + * what kind of URI it is. + * + * Every case here is a failure case except one, and that is the point -- these branches never + * run in a healthy test run and are exactly the ones a user meets on a bad day. + */ +@RunWith(RobolectricTestRunner::class) +class OutputPublisherPublishTest { + + private lateinit var context: Context + private lateinit var publisher: OutputPublisher + private lateinit var staged: File + + private val payload = ByteArray(8192) { (it % 251).toByte() } + + private val documentUri: Uri = Uri.parse("content://$DOCUMENTS_AUTHORITY/document/holiday.mp4") + private val plainUri: Uri = Uri.parse("content://$PLAIN_AUTHORITY/document/holiday_plain.mp4") + private val deadUri: Uri = Uri.parse("content://org.libremediaconverter.nonexistent/document/gone.mp4") + + @Before + fun setUp() { + context = RuntimeEnvironment.getApplication() + FakeSafProvider.reset(File(context.cacheDir, "destinations").apply { mkdirs() }) + register(FakeSafProvider::class.java, DOCUMENTS_AUTHORITY, asDocumentsProvider = true) + register(FakePlainProvider::class.java, PLAIN_AUTHORITY, asDocumentsProvider = false) + + // SAF's CreateDocument contract hands back a document that already exists and is + // empty, so that is the state every destination starts in here. + FakeSafProvider.backingFile(documentUri).writeBytes(ByteArray(0)) + FakeSafProvider.backingFile(plainUri).writeBytes(ByteArray(0)) + + publisher = OutputPublisher(context) + staged = publisher.createStagingFile("holiday.mp4").apply { writeBytes(payload) } + } + + @Test + fun `a copy that fails partway leaves nothing at the destination`() { + failMidCopy(documentUri, afterBytes = 512) + + val failure = assertThrows(IOException::class.java) { publisher.publish(staged, documentUri) } + + assertEquals(NO_SPACE, failure.message) + assertEquals(listOf(documentUri), FakeSafProvider.deleteRequests) + assertFalse( + "a truncated file must not be left at the name the user picked", + FakeSafProvider.backingFile(documentUri).exists(), + ) + } + + @Test + fun `a close that fails while flushing counts as a failed copy`() { + // copyTo() has already returned by the time this throws. Guarding only the copy and + // not the close would leave the file behind on exactly the disk-full case. + shadowOf(context.contentResolver).registerOutputStreamSupplier(documentUri) { + UnreliableOutputStream(FakeSafProvider.backingFile(documentUri).outputStream(), failOnClose = true) + } + + val failure = assertThrows(IOException::class.java) { publisher.publish(staged, documentUri) } + + assertEquals(NO_SPACE, failure.message) + assertFalse( + "bytes that were never flushed are not a saved file", + FakeSafProvider.backingFile(documentUri).exists(), + ) + } + + @Test + fun `a destination that already held bytes is not deleted`() { + // Not the CreateDocument case: a provider that handed back an existing document for + // a name the user re-picked. Truncating it is bad; removing it outright is worse, and + // publish() cannot tell from a Uri that the app created it. + val existing = FakeSafProvider.backingFile(documentUri).apply { writeBytes(ByteArray(4096)) } + failMidCopy(documentUri, afterBytes = 512) + + assertThrows(IOException::class.java) { publisher.publish(staged, documentUri) } + + assertTrue("a document this app did not create must survive", existing.exists()) + assertEquals(emptyList(), FakeSafProvider.deleteRequests) + } + + @Test + fun `a destination that is not a document is left alone`() { + failMidCopy(plainUri, afterBytes = 512) + + assertThrows(IOException::class.java) { publisher.publish(staged, plainUri) } + + assertEquals( + "deleteDocument has no business on a URI that is not a document", + emptyList(), + FakeSafProvider.deleteRequests, + ) + assertTrue(FakeSafProvider.backingFile(plainUri).exists()) + } + + @Test + fun `a cleanup that fails does not replace the failure the user needs to see`() { + FakeSafProvider.deleteFailure = SecurityException("provider refused the delete") + failMidCopy(documentUri, afterBytes = 512) + + val failure = assertThrows(IOException::class.java) { publisher.publish(staged, documentUri) } + + assertEquals("the disk-full failure is what save() reports", NO_SPACE, failure.message) + assertEquals( + "the cleanup failure is attached rather than lost", + listOf("provider refused the delete"), + failure.suppressedExceptions.map { it.message }, + ) + } + + @Test + fun `a destination that cannot be opened at all fails without any cleanup`() { + // The JVM twin of UnopenableUriTest's unwritable-destination case. Nothing was + // written, so there is nothing of ours to remove. + val failure = runCatching { publisher.publish(staged, deadUri) }.exceptionOrNull() + + assertTrue("publishing to a dead provider must not appear to succeed, got $failure", failure != null) + assertEquals(emptyList(), FakeSafProvider.deleteRequests) + } + + @Test + fun `a copy that succeeds delivers every byte and deletes nothing`() { + shadowOf(context.contentResolver).registerOutputStreamSupplier(documentUri) { + FakeSafProvider.backingFile(documentUri).outputStream() + } + + publisher.publish(staged, documentUri) + + assertArrayEquals(payload, FakeSafProvider.backingFile(documentUri).readBytes()) + assertEquals(emptyList(), FakeSafProvider.deleteRequests) + } + + /** + * Arms the destination to accept [afterBytes] and then fail. + * + * A supplier rather than a ready-made stream: opening the backing file truncates it, and + * doing that here would erase the very content the "already held bytes" case is about + * before `publish()` ever got to read its size. + */ + private fun failMidCopy(destination: Uri, afterBytes: Int) { + shadowOf(context.contentResolver).registerOutputStreamSupplier(destination) { + UnreliableOutputStream( + FakeSafProvider.backingFile(destination).outputStream(), + failAfterBytes = afterBytes, + ) + } + } + + private fun register(provider: Class, authority: String, asDocumentsProvider: Boolean) { + val info = ProviderInfo().apply { + this.authority = authority + packageName = context.packageName + name = provider.name + exported = true + grantUriPermissions = true + } + Robolectric.buildContentProvider(provider).create(info) + + // isDocumentUri() does not look at the URI alone: it asks the package manager whether + // anything answers ACTION_DOCUMENTS_PROVIDER for that authority. Registering the + // provider with the resolver is not enough, which is the whole reason the negative + // case above can exist. + val packageManager = shadowOf(context.packageManager) + packageManager.addOrUpdateProvider(info) + if (asDocumentsProvider) { + packageManager.addIntentFilterForProvider( + ComponentName(context.packageName, provider.name), + IntentFilter(DocumentsContract.PROVIDER_INTERFACE), + ) + } + } +} diff --git a/app/src/test/java/org/libremediaconverter/convert/OutputPublisherStagingTest.kt b/app/src/test/java/org/libremediaconverter/convert/OutputPublisherStagingTest.kt new file mode 100644 index 0000000..4419919 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/OutputPublisherStagingTest.kt @@ -0,0 +1,100 @@ +package org.libremediaconverter.convert + +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment +import java.io.File +import java.util.UUID + +/** + * What a pure function cannot say: the file is really gone. + * + * [StagingSweepTest] pins the rule; this pins the effect. Robolectric gives each test a + * real, empty `cacheDir` on a temp path, so this drives the actual [OutputPublisher] over + * the actual filesystem — the same calls `reset()` makes, without needing a ViewModel (both + * of those construct a `WorkManager`, which is not initialised on the JVM classpath). + * + * The instrumented suite cannot run on the development host, so this is the only place the + * "Start over leaks a full-size copy" defect can be caught before CI. + */ +@RunWith(RobolectricTestRunner::class) +class OutputPublisherStagingTest { + + private lateinit var cacheDir: File + private lateinit var publisher: OutputPublisher + + @Before + fun setUp() { + val context = RuntimeEnvironment.getApplication() + cacheDir = context.cacheDir + publisher = OutputPublisher(context) + } + + @Test + fun `discarding a staged output actually removes it`() { + // This is the leak in D2: convert, decline to save, tap "Start over". + val staged = publisher.createStagingFile("holiday.mp4").apply { writeBytes(ByteArray(4096)) } + assertTrue("the staged file should exist to begin with", staged.exists()) + + assertTrue("discard should report that it deleted the file", publisher.discardStaged(staged)) + assertFalse("the staged file should be gone after the reset path runs", staged.exists()) + } + + @Test + fun `discarding a file that is already gone is not an error`() { + // reset() after a successful save, or two resets in a row. Neither should throw. + val staged = publisher.createStagingFile("already_published.mp4") + assertFalse(publisher.discardStaged(staged)) + } + + @Test + fun `a file outside the staging directory is refused`() { + // The handle can originate in WorkInfo.outputData, which is a string the ViewModel + // turns straight into a File. Nothing else checks where it points. + val outsider = File(cacheDir, "someone_elses.bin").apply { writeBytes(ByteArray(16)) } + + assertFalse(publisher.discardStaged(outsider)) + assertTrue("a file outside staging must survive", outsider.exists()) + } + + @Test + fun `a path that climbs out of the staging directory is refused`() { + // The naive parent check -- comparing path strings -- passes this one. + val outsider = File(cacheDir, "climbed_to.bin").apply { writeBytes(ByteArray(16)) } + val escaping = File(cacheDir, "conversions/../climbed_to.bin") + + assertFalse(publisher.discardStaged(escaping)) + assertTrue("a traversal must not delete outside staging", outsider.exists()) + } + + @Test + fun `the sweep collects an orphan and leaves a live job alone`() { + // Named the way the app names them, so the sweep is exercised against real shapes. + val liveJob = StagingNames.forJob(UUID.randomUUID(), "mp4") + val orphan = publisher.createStagingFile( + StagingNames.forJob(UUID.randomUUID(), "mp4"), + ).apply { writeBytes(ByteArray(4096)) } + val liveOutput = publisher.createStagingFile(liveJob).apply { writeBytes(ByteArray(4096)) } + val liveList = publisher.createStagingFile( + StagingNames.concatListFor(liveJob), + ).apply { writeText("file 'a.mp4'\n") } + assertTrue(orphan.setLastModified(System.currentTimeMillis() - StagingSweep.GRACE_PERIOD_MS - 60_000)) + + publisher.sweepStaging() + + assertFalse("an abandoned output should be collected", orphan.exists()) + assertTrue("a live job's output must survive", liveOutput.exists()) + assertTrue("a live join's list file must survive", liveList.exists()) + } + + @Test + fun `the sweep tolerates a staging directory that does not exist yet`() { + File(cacheDir, "conversions").deleteRecursively() + + publisher.sweepStaging() + } +} diff --git a/app/src/test/java/org/libremediaconverter/convert/ReattachedCleanupTest.kt b/app/src/test/java/org/libremediaconverter/convert/ReattachedCleanupTest.kt new file mode 100644 index 0000000..52efbad --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/ReattachedCleanupTest.kt @@ -0,0 +1,134 @@ +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.Assert.assertFalse +import org.junit.Assert.assertTrue +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 +import java.io.File + +/** + * Where the two staging fixes meet: a result nobody started is still a result somebody owns. + * + * Reattachment hands the user back a job this ViewModel did not start, which means the + * ViewModel inherits the staged file along with it — and "Start over" on that file has to + * delete it exactly as it would for a conversion run in this process. Neither fix implies + * the other: cleanup only reaches a file whose handle was recorded, and reattachment only + * records one because it goes through the same `observe()` the normal success path does. + * That is a structural claim about one function, which is precisely the kind that a merge + * resolving the two changes into different places would quietly break. Hence a test rather + * than a comment. + * + * These run a real `WorkManager` and a real `OutputPublisher` against a real `cacheDir`, so + * what is asserted at the end is the filesystem. + */ +@UnstableApi +@RunWith(RobolectricTestRunner::class) +class ReattachedCleanupTest { + + private lateinit var app: Application + private lateinit var publisher: RecordingPublisher + private lateinit var workManager: WorkManager + private lateinit var staged: File + + @Before + fun setUp() { + app = RuntimeEnvironment.getApplication() + publisher = RecordingPublisher(app) + ConversionDependencies.publisher = { publisher } + ConversionDependencies.probe = { _, _ -> InputProbe() } + + staged = publisher.createStagingFile("holiday_converted.mp4").apply { writeBytes(ByteArray(4096)) } + installTestWorkManager(app, workDataOf(ConversionWorker.KEY_OUTPUT_PATH to staged.absolutePath)) + workManager = WorkManager.getInstance(app) + } + + @After + fun tearDown() { + ConversionDependencies.reset() + } + + /** + * The defect end to end, on the JVM: work that finished with no ViewModel left to see it, + * and a ViewModel created afterwards that finds it anyway. + */ + @Test + fun `a conversion that finished before this ViewModel existed is picked up`() { + finishAConversionWithNobodyWatching() + + val viewModel = ConversionViewModel(app, Dispatchers.Unconfined) + val converted = awaitState(viewModel.state, "Converted") { it is ConversionState.Converted } + + converted as ConversionState.Converted + assertEquals(staged.absolutePath, converted.staged.absolutePath) + // The name came back through the job's tags — the only channel WorkManager returns, + // since WorkInfo never carries the Data a request was enqueued with. + assertEquals("holiday.mp4", converted.input.displayName) + assertEquals(4_096L, converted.input.sizeBytes) + } + + @Test + fun `start over on a reattached conversion deletes the staged file`() { + finishAConversionWithNobodyWatching() + val viewModel = ConversionViewModel(app, Dispatchers.Unconfined) + awaitState(viewModel.state, "Converted") { it is ConversionState.Converted } + assertTrue("the reattached result should still be on disk", staged.exists()) + + viewModel.reset() + + assertEquals(ConversionState.Idle, viewModel.state.value) + assertEquals( + "a reattached result is still this ViewModel's to discard", + listOf(staged), + publisher.discarded, + ) + assertFalse("Start over must not leave a full-size copy in cache", staged.exists()) + } + + @Test + fun `start over on a reattached join deletes the staged file`() { + workManager.enqueue( + ConcatWorker.request( + inputs = listOf(Uri.parse("content://test/one.mp4"), Uri.parse("content://test/two.mp4")), + totalBytes = 8_192L, + ), + ).result.get() + + val viewModel = JoinViewModel(app, Dispatchers.Unconfined) + awaitState(viewModel.state, "Joined") { it is JoinState.Joined } + + viewModel.reset() + + assertEquals(listOf(staged), publisher.discarded) + assertFalse(staged.exists()) + } + + /** + * A job from a process that is gone: enqueued and finished before any ViewModel exists, so + * nothing observed it and nothing recorded its output. + */ + private fun finishAConversionWithNobodyWatching() { + workManager.enqueue( + ConversionWorker.request( + inputUri = Uri.parse("content://test/holiday.mp4"), + displayName = "holiday.mp4", + sizeBytes = 4_096L, + ), + ).result.get() + } +} diff --git a/app/src/test/java/org/libremediaconverter/convert/StagingCleanupSupport.kt b/app/src/test/java/org/libremediaconverter/convert/StagingCleanupSupport.kt new file mode 100644 index 0000000..3ea2462 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/StagingCleanupSupport.kt @@ -0,0 +1,124 @@ +package org.libremediaconverter.convert + +import android.content.Context +import android.net.Uri +import android.os.Looper +import android.util.Log +import androidx.work.Configuration +import androidx.work.Data +import androidx.work.ListenableWorker +import androidx.work.Worker +import androidx.work.WorkerFactory +import androidx.work.WorkerParameters +import androidx.work.testing.SynchronousExecutor +import androidx.work.testing.WorkManagerTestInitHelper +import kotlinx.coroutines.flow.StateFlow +import org.robolectric.Shadows.shadowOf +import java.io.File +import java.util.concurrent.TimeUnit + +/** + * Shared scaffolding for the two ViewModel cleanup tests. + * + * These tests exist because [StagingSweepTest] and [OutputPublisherStagingTest] both prove + * the *tool* works while saying nothing about whether anything calls it — and the wiring is + * where D2 actually lived. Deleting the `discardStaged` line from either `reset()` left all + * of the earlier tests green. + */ + +/** + * A real [OutputPublisher] that records what it was asked to discard. + * + * It still really deletes, so the assertions are about the filesystem rather than about a + * mock's memory. [publish] is stubbed because the SAF destination is not what these tests + * are about, and because making it throw is the only way to reach the failed-save branch + * deterministically. + */ +open class RecordingPublisher(context: Context) : OutputPublisher(context) { + + val discarded = mutableListOf() + + /** When set, [publish] throws it — the failed-save path. */ + var publishFailure: Throwable? = null + + override fun publish(staged: File, destination: Uri) { + publishFailure?.let { throw it } + } + + override fun discardStaged(staged: File): Boolean { + discarded += staged + return super.discardStaged(staged) + } +} + +/** + * Stands in for whichever worker is enqueued and succeeds immediately with [outputData]. + * + * The real workers cannot run here: both drive FFmpeg or Media3 through native libraries + * that do not exist on the JVM. What the ViewModel actually needs from them is one + * `SUCCEEDED` `WorkInfo` carrying an output path, and that is exactly what this produces — + * through a real `WorkManager`, so the ViewModel's own observer, its `SUCCEEDED` branch and + * its cleanup handle are all the production ones. + * + * It also keeps every [Data] it was handed, which is the only way back to what a ViewModel + * actually enqueued: `WorkInfo` returns a job's tags and its output and never the input `Data` + * it was built with, so a test that wants to know what `convert()` or `join()` put in a request + * has to catch it here, on its way to the worker. + */ +class SucceedingWorkerFactory(private val outputData: Data) : WorkerFactory() { + + /** The input `Data` of each request that has reached a worker, in order. */ + val enqueued = mutableListOf() + + override fun createWorker( + appContext: Context, + workerClassName: String, + workerParameters: WorkerParameters, + ): ListenableWorker { + enqueued += workerParameters.inputData + return object : Worker(appContext, workerParameters) { + override fun doWork(): Result = Result.success(outputData) + } + } +} + +/** + * Installs a synchronous test WorkManager whose workers succeed with [outputData]. + * + * @return the factory, so a caller that cares can read back what was enqueued. + */ +fun installTestWorkManager(context: Context, outputData: Data): SucceedingWorkerFactory { + val factory = SucceedingWorkerFactory(outputData) + WorkManagerTestInitHelper.initializeTestWorkManager( + context, + Configuration.Builder() + .setMinimumLoggingLevel(Log.ASSERT) + .setExecutor(SynchronousExecutor()) + .setTaskExecutor(SynchronousExecutor()) + .setWorkerFactory(factory) + .build(), + ) + return factory +} + +/** + * Waits for [predicate] to hold, pumping the main looper as it goes. + * + * Both ViewModels hop to a real `Dispatchers.IO` for file metadata and resume on the main + * looper, which Robolectric leaves paused. So neither a bare read of `state.value` nor a + * single `idle()` is enough, and the timeout is generous because it only has to be longer + * than a few file stats — in practice this converges in milliseconds. + */ +fun awaitState(state: StateFlow, description: String, predicate: (T) -> Boolean): T { + val deadline = System.nanoTime() + TimeUnit.SECONDS.toNanos(AWAIT_TIMEOUT_SECONDS) + while (System.nanoTime() < deadline) { + shadowOf(Looper.getMainLooper()).idle() + val current = state.value + if (predicate(current)) return current + Thread.sleep(POLL_INTERVAL_MS) + } + throw AssertionError("Timed out waiting for $description; state was ${state.value}") +} + +private const val AWAIT_TIMEOUT_SECONDS = 10L +private const val POLL_INTERVAL_MS = 5L diff --git a/app/src/test/java/org/libremediaconverter/convert/StagingNamesTest.kt b/app/src/test/java/org/libremediaconverter/convert/StagingNamesTest.kt new file mode 100644 index 0000000..f495c08 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/StagingNamesTest.kt @@ -0,0 +1,89 @@ +package org.libremediaconverter.convert + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNotEquals +import org.junit.Assert.assertTrue +import org.junit.Test +import org.libremediaconverter.model.OutputFormat +import java.io.File +import java.util.UUID + +/** + * The rule that gives every job a staging path of its own. + * + * A pure function for the same reason as [StagingSweep] and + * [org.libremediaconverter.work.FailureOutcome]: what it prevents cannot be provoked here. The + * collision needs two jobs alive at once, one of them resumed by WorkManager after a process + * restart, which is a device and an `am kill`. What *is* checkable is the property that makes the + * collision impossible, and that is what these pin. + */ +class StagingNamesTest { + + @Test + fun `two jobs converting the same file stage under different names`() { + // The collision, seen on a device: two independent jobs both computed + // cache/conversions/input_converted.mp4, and a tag query in a fresh process returned two + // SUCCEEDED WorkInfos naming that one file. + assertNotEquals( + StagingNames.forJob(JOB_A, MP4.extension), + StagingNames.forJob(JOB_B, MP4.extension), + ) + } + + @Test + fun `the same job stages under the same name on every attempt`() { + // Load-bearing, not incidental. A retry runs doWork() from the top, and the catch on the + // way out deletes the staged file -- which only collects the previous attempt's partial if + // the name is the same. WorkManager builds WorkerParameters from the WorkSpec id and only + // increments runAttemptCount, so the id is what stays still across a retry. + assertEquals( + StagingNames.forJob(JOB_A, MP4.extension), + StagingNames.forJob(JOB_A, MP4.extension), + ) + } + + @Test + fun `the extension is the output's, because that is what infers the muxer`() { + // Not cosmetic. FFmpegConcatCommand names no output muxer, so FFmpeg infers it from the + // path -- an opaque name without the right extension would silently produce the wrong + // container. + assertTrue(StagingNames.forJob(JOB_A, MP4.extension).endsWith(".mp4")) + assertTrue(StagingNames.forJob(JOB_A, OutputFormat.MKV_H265.extension).endsWith(".mkv")) + assertTrue(StagingNames.forJob(JOB_A, OutputFormat.M4A_AAC.extension).endsWith(".m4a")) + } + + @Test + fun `a staging name is a bare filename and nothing else`() { + // The alternative to an opaque name was sanitising the provider-supplied display name, + // which can contain a separator, be empty, or be four kilobytes long. This is what makes + // that whole question moot. + val name = StagingNames.forJob(JOB_A, MP4.extension) + assertEquals("a staging name must not be a path", name, File(name).name) + assertTrue("a staging name must not be empty", name.isNotEmpty()) + } + + @Test + fun `each join gets a list file of its own`() { + // ConcatEngine used a constant, so any two joins at once shared one concat_list.txt and + // one of them read the other's input list. + assertNotEquals( + StagingNames.concatListFor(StagingNames.forJob(JOB_A, MP4.extension)), + StagingNames.concatListFor(StagingNames.forJob(JOB_B, MP4.extension)), + ) + } + + @Test + fun `a list file is named after the output it belongs to`() { + // So the pair is obvious in a directory listing, and so the sweep ages them together. + val output = StagingNames.forJob(JOB_A, MP4.extension) + val list = StagingNames.concatListFor(output) + assertEquals("$JOB_A.concat_list.txt", list) + assertTrue(list.startsWith(output.substringBeforeLast('.'))) + } + + private companion object { + val MP4 = OutputFormat.MP4_H264 + val JOB_A: UUID = UUID.fromString("00000000-0000-4000-8000-00000000000a") + val JOB_B: UUID = UUID.fromString("00000000-0000-4000-8000-00000000000b") + } +} diff --git a/app/src/test/java/org/libremediaconverter/convert/StagingSweepTest.kt b/app/src/test/java/org/libremediaconverter/convert/StagingSweepTest.kt new file mode 100644 index 0000000..f63f9dc --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/StagingSweepTest.kt @@ -0,0 +1,77 @@ +package org.libremediaconverter.convert + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test + +/** + * The orphan-collection rule. + * + * Timestamps are passed in as values rather than read off real files on purpose. The + * interesting part of this decision is clock arithmetic — the boundary, and a clock that + * has moved backwards — and a test that created real files would be measuring the + * filesystem's mtime granularity instead of the rule. + */ +class StagingSweepTest { + + private val now = 1_700_000_000_000L + private val grace = StagingSweep.GRACE_PERIOD_MS + + @Test + fun `an orphan older than the grace period is collectable`() { + // Left behind by a process that died, or by a "Start over" whose delete never ran. + val entries = listOf(StagingSweep.Entry("orphan.mp4", now - grace - 1)) + assertEquals(listOf("orphan.mp4"), StagingSweep.collectable(entries, now)) + } + + @Test + fun `a file written moments ago is left alone`() { + // The in-flight guard. A live job's output has its mtime refreshed by every write, + // so a running conversion always looks young; deleting it would destroy the job. + val entries = listOf(StagingSweep.Entry("in_progress.mp4", now - 1_000)) + assertEquals(emptyList(), StagingSweep.collectable(entries, now)) + } + + @Test + fun `the grace boundary itself collects`() { + // Pins the comparison: age >= grace collects, age one millisecond short does not. + assertTrue(StagingSweep.isCollectable(lastModifiedMs = now - grace, nowMs = now)) + assertFalse(StagingSweep.isCollectable(lastModifiedMs = now - grace + 1, nowMs = now)) + } + + @Test + fun `a file dated in the future is left alone`() { + // The clock moved backwards — an RTC correction, or the user setting the date. The + // age is negative, which says nothing about whether the file is still in use, so + // the safe answer is to keep it and let a later sweep decide. + val entries = listOf(StagingSweep.Entry("tomorrow.mp4", now + grace)) + assertEquals(emptyList(), StagingSweep.collectable(entries, now)) + } + + @Test + fun `an empty directory yields nothing`() { + assertEquals(emptyList(), StagingSweep.collectable(emptyList(), now)) + } + + @Test + fun `a mixed directory names only the orphans`() { + // The whole point of narrowing the old clearStaging(): a sweep that runs while a + // join is live must not take the list file out from under it. + val entries = listOf( + StagingSweep.Entry("orphan.mp4", now - grace - 1), + StagingSweep.Entry("concat_list.txt", now - 5_000), + StagingSweep.Entry("joined.mp4", now - 5_000), + StagingSweep.Entry("older_orphan.webm", now - grace * 7), + ) + assertEquals(listOf("orphan.mp4", "older_orphan.webm"), StagingSweep.collectable(entries, now)) + } + + @Test + fun `a shorter grace period can be asked for explicitly`() { + // The caller owns the period; the constant is only a default. + val entries = listOf(StagingSweep.Entry("recent.mp4", now - 60_000)) + assertEquals(emptyList(), StagingSweep.collectable(entries, now)) + assertEquals(listOf("recent.mp4"), StagingSweep.collectable(entries, now, gracePeriodMs = 30_000)) + } +} diff --git a/app/src/test/java/org/libremediaconverter/convert/UnknownInputSizeTest.kt b/app/src/test/java/org/libremediaconverter/convert/UnknownInputSizeTest.kt new file mode 100644 index 0000000..99e39c3 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/UnknownInputSizeTest.kt @@ -0,0 +1,165 @@ +package org.libremediaconverter.convert + +import android.app.Application +import android.net.Uri +import androidx.media3.common.util.UnstableApi +import androidx.work.Data +import androidx.work.hasKeyWithValueOfType +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNull +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.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment +import java.io.File + +/** + * That a size nobody reported is not the same thing as a size of zero. + * + * `queryFile` started at `var size = 0L` and only moved off it when a provider answered the + * `OpenableColumns.SIZE` column, so "the file is empty" and "nobody told me" arrived at the space + * check as the same number — and `hasSpaceFor(0)` is only "is there 128 MB free". + * + * The gap is not hypothetical. On a Pixel 10 Pro XL, `contentResolver.query` on a `file://` URI + * returns null outright, so the cursor block never runs and the default survives: + * `queryFile gave displayName='input' sizeBytes=0`. Robolectric reproduces that exactly, which is + * what makes the first test below a JVM test rather than a device one. + */ +@UnstableApi +@RunWith(RobolectricTestRunner::class) +class UnknownInputSizeTest { + + private lateinit var app: Application + private lateinit var workers: SucceedingWorkerFactory + + @Before + fun setUp() { + app = RuntimeEnvironment.getApplication() + // FFprobe's loader throws a bare java.lang.Error on the JVM, and nothing here is about + // what the probe found. + ConversionDependencies.probe = { _, _ -> InputProbe() } + // Both ViewModels reach WorkManager.getInstance() while constructing. + workers = installTestWorkManager(app, Data.EMPTY) + } + + @After + fun tearDown() { + ConversionDependencies.reset() + } + + @Test + fun `a picked file no provider describes is measured rather than reported as empty`() { + val input = fileOfSize(INPUT_BYTES, "holiday.mp4") + + val ready = pickedInto(Uri.fromFile(input)) + + // The resolver answers nothing at all for a file:// URI -- the exact device case -- so the + // only way to this number is opening the file and asking the descriptor. + assertEquals(INPUT_BYTES.toLong(), ready.input.sizeBytes) + } + + @Test + fun `a picked file nothing can measure has no size rather than a size of zero`() { + // No provider is registered for this authority, so the metadata query returns null and + // openFileDescriptor throws FileNotFoundException. Nothing can say how big it is, and + // saying "zero" would be a claim rather than an answer. + val ready = pickedInto(Uri.parse("content://test/holiday.mp4")) + + assertNull(ready.input.sizeBytes) + } + + @Test + fun `the same measurement is what the join picker gets`() { + // Not a copy of the convert test for its own sake: `queryFile` existed twice, once in each + // ViewModel, byte for byte. One of the two being fixed is the shape this would come back in. + val first = fileOfSize(FIRST_JOIN_BYTES, "one.mp4") + val second = fileOfSize(SECOND_JOIN_BYTES, "two.mp4") + val viewModel = JoinViewModel(app) + + viewModel.onInputsPicked(listOf(Uri.fromFile(first), Uri.fromFile(second))) + val ready = awaitState(viewModel.state, "Ready") { it is JoinState.Ready } as JoinState.Ready + + assertEquals( + listOf(FIRST_JOIN_BYTES.toLong(), SECOND_JOIN_BYTES.toLong()), + ready.inputs.map { it.sizeBytes }, + ) + } + + @Test + fun `a join enqueues the total it worked out, and no total at all when it could not`() { + // The wiring, which the two tests around it do not reach: `InputQuery.total` being right + // says nothing about `join()` calling it, and `ConcatWorker`'s unknown branch is reached + // by work built in that test rather than by this ViewModel. Restoring + // `inputs.sumOf { it.sizeBytes ?: 0L }` leaves both of those green. + // + // Read off the request on its way to a worker, because that is the only place it is + // legible: `WorkInfo` hands back a job's tags and its output, never the input `Data`. + val first = fileOfSize(FIRST_JOIN_BYTES, "one.mp4") + val second = fileOfSize(SECOND_JOIN_BYTES, "two.mp4") + + joined(Uri.fromFile(first), Uri.fromFile(second)) + val known = workers.enqueued.single() + assertEquals( + (FIRST_JOIN_BYTES + SECOND_JOIN_BYTES).toLong(), + known.getLong(ConcatWorker.KEY_TOTAL_BYTES, MISSING), + ) + + // And with one input nothing can size, the key is absent rather than carrying a short + // total -- a `Data` has no null, so absence is the only way to say "unknown" in one. + workers = installTestWorkManager(app, Data.EMPTY) + joined(Uri.fromFile(first), Uri.parse("content://test/two.mp4")) + val unknown = workers.enqueued.single() + assertFalse( + "a total that could not be worked out must not be enqueued as a number", + unknown.hasKeyWithValueOfType(ConcatWorker.KEY_TOTAL_BYTES), + ) + } + + @Test + fun `a total is only as good as its least-known part`() { + // The rule the join side needed that the convert side did not. `sumOf` over a list with an + // unknown in it produces a number, and a number that is short by one whole file is worse + // than no number: the space check cannot tell it from a real total, so it would reserve + // for half the job and pass. + assertEquals(3_333L, InputQuery.total(listOf(1_111L, 2_222L))) + assertNull(InputQuery.total(listOf(1_111L, null))) + assertNull(InputQuery.total(listOf(null, 2_222L))) + // A join of nothing has a known total of nothing. Both workers refuse fewer than two + // inputs long before this, so it is a statement about the fold rather than a real case. + assertEquals(0L, InputQuery.total(emptyList())) + } + + /** Drives a real [JoinViewModel] from a pick to an enqueued join. */ + private fun joined(vararg uris: Uri) { + val viewModel = JoinViewModel(app) + viewModel.onInputsPicked(uris.toList()) + awaitState(viewModel.state, "Ready") { it is JoinState.Ready } + viewModel.join() + awaitState(viewModel.state, "past Joining") { it !is JoinState.Ready } + } + + private fun pickedInto(uri: Uri): ConversionState.Ready { + val viewModel = ConversionViewModel(app) + viewModel.onInputPicked(uri) + return awaitState(viewModel.state, "Ready") { it is ConversionState.Ready } as ConversionState.Ready + } + + private fun fileOfSize(bytes: Int, name: String): File = + File(app.cacheDir, name).apply { writeBytes(ByteArray(bytes)) } + + private companion object { + const val INPUT_BYTES = 4_321 + const val FIRST_JOIN_BYTES = 1_111 + const val SECOND_JOIN_BYTES = 2_222 + + /** A `getLong` default no real total could be mistaken for. */ + const val MISSING = -1L + } +} diff --git a/app/src/test/java/org/libremediaconverter/ffmpeg/NativeLoadFailureTest.kt b/app/src/test/java/org/libremediaconverter/ffmpeg/NativeLoadFailureTest.kt new file mode 100644 index 0000000..6dced12 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/ffmpeg/NativeLoadFailureTest.kt @@ -0,0 +1,89 @@ +package org.libremediaconverter.ffmpeg + +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test + +/** + * Where the guard draws its line. + * + * The whole point of naming the predicate was that "catch what a failed native load throws" + * and "do not swallow an OutOfMemoryError in a method that spawns a native process" are two + * requirements a catch clause cannot express together. These are that pair, written down. + */ +class NativeLoadFailureTest { + + // --- Recognised: the installation is broken, not this JVM ------------------------------ + + /** + * FFmpegKit's own shape. `NativeLoader.loadLibrary` catches the `UnsatisfiedLinkError` + * that `System.loadLibrary` raises and rethrows a bare `java.lang.Error` wrapping it, so + * the type carries no information and the cause is what identifies it. + */ + @Test + fun `a bare Error wrapping an UnsatisfiedLinkError is a native load failure`() { + val error = Error("FFmpegKit failed to start on brand: robolectric.", UnsatisfiedLinkError("dlopen failed")) + + assertTrue(isNativeLoadFailure(error)) + } + + /** Every touch of the class after the first one, which is the easier half to miss. */ + @Test + fun `a NoClassDefFoundError is a native load failure`() { + val error = NoClassDefFoundError("Could not initialize class com.arthenica.ffmpegkit.FFmpegKitConfig") + + assertTrue(isNativeLoadFailure(error)) + } + + /** What the first touch looked like when the JVM wrapped the failing initializer. */ + @Test + fun `an ExceptionInInitializerError is a native load failure`() { + assertTrue(isNativeLoadFailure(ExceptionInInitializerError("Exception java.lang.Error: FFmpegKit failed"))) + } + + /** If a later FFmpegKit stops wrapping, the raw error is recognised on its own. */ + @Test + fun `a plain UnsatisfiedLinkError is a native load failure`() { + assertTrue(isNativeLoadFailure(UnsatisfiedLinkError("dlopen failed: libffmpegkit.so not found"))) + } + + // --- Not recognised: this JVM is in trouble and must be allowed to say so --------------- + + /** + * The regression the narrow guard exists to prevent. `catch (Throwable)` here would report + * "out of memory" to the user as "this file looks unreadable". + */ + @Test + fun `an OutOfMemoryError is not a native load failure`() { + assertFalse(isNativeLoadFailure(OutOfMemoryError("Failed to allocate a 512 MB allocation"))) + } + + @Test + fun `a StackOverflowError is not a native load failure`() { + assertFalse(isNativeLoadFailure(StackOverflowError())) + } + + @Test + fun `an AssertionError is not a native load failure`() { + assertFalse(isNativeLoadFailure(AssertionError("a broken invariant is not a broken install"))) + } + + /** + * A bare `Error` on its own says nothing. Only the `UnsatisfiedLinkError` underneath it + * makes it FFmpegKit's, so matching the type alone would be a blanket catch wearing a + * predicate's clothes. + */ + @Test + fun `a bare Error with no cause is not a native load failure`() { + assertFalse(isNativeLoadFailure(Error("something else went wrong"))) + } + + /** An OOM does not become catchable by acquiring a cause. */ + @Test + fun `an OutOfMemoryError caused by something else is still not a native load failure`() { + val error = OutOfMemoryError("Java heap space") + error.initCause(IllegalStateException("some unrelated cause")) + + assertFalse(isNativeLoadFailure(error)) + } +} diff --git a/app/src/test/java/org/libremediaconverter/join/JoinViewModelCleanupTest.kt b/app/src/test/java/org/libremediaconverter/join/JoinViewModelCleanupTest.kt new file mode 100644 index 0000000..91b7c05 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/join/JoinViewModelCleanupTest.kt @@ -0,0 +1,113 @@ +package org.libremediaconverter.join + +import android.app.Application +import android.net.Uri +import androidx.media3.common.util.UnstableApi +import androidx.work.workDataOf +import kotlinx.coroutines.Dispatchers +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.libremediaconverter.convert.ConversionDependencies +import org.libremediaconverter.convert.RecordingPublisher +import org.libremediaconverter.convert.awaitState +import org.libremediaconverter.convert.installTestWorkManager +import org.libremediaconverter.work.ConcatWorker +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment +import java.io.File + +/** + * The join tab's half of the same defect. + * + * Carried separately rather than parameterised with the convert tab, because the two are + * independent ViewModels that can each hold a staged file at the same time — the reason + * `clearStaging()` could not simply be wired up. + */ +@UnstableApi +@RunWith(RobolectricTestRunner::class) +class JoinViewModelCleanupTest { + + private lateinit var app: Application + private lateinit var publisher: RecordingPublisher + private lateinit var staged: File + + @Before + fun setUp() { + app = RuntimeEnvironment.getApplication() + publisher = RecordingPublisher(app) + ConversionDependencies.publisher = { publisher } + + staged = publisher.createStagingFile("joined.mp4").apply { writeBytes(ByteArray(4096)) } + installTestWorkManager(app, workDataOf(ConcatWorker.KEY_OUTPUT_PATH to staged.absolutePath)) + } + + @After + fun tearDown() { + ConversionDependencies.reset() + } + + @Test + fun `start over on a finished join deletes the staged file`() { + val viewModel = joinedViewModel() + assertTrue("the join should have produced a staged file", staged.exists()) + + viewModel.reset() + + assertEquals(JoinState.Idle, viewModel.state.value) + assertEquals(listOf(staged), publisher.discarded) + assertFalse("Start over must not leave a full-size copy in cache", staged.exists()) + } + + @Test + fun `reset after a successful save does not try to delete again`() { + val viewModel = joinedViewModel() + viewModel.save(DESTINATION) + awaitState(viewModel.state, "Saved") { it is JoinState.Saved } + + // save() deletes the staged file itself, through File.delete() rather than through + // the publisher, so assert the disappearance as well as the absent second discard. + assertFalse("a successful save should have removed the staged file", staged.exists()) + + viewModel.reset() + + assertEquals(emptyList(), publisher.discarded) + assertFalse(staged.exists()) + } + + @Test + fun `a failed save keeps the staged file, and a later reset collects it`() { + val viewModel = joinedViewModel() + publisher.publishFailure = IllegalStateException("destination volume full") + + viewModel.save(DESTINATION) + awaitState(viewModel.state, "Failed") { it is JoinState.Failed } + + assertTrue("a failed save must not destroy the only copy", staged.exists()) + assertEquals(emptyList(), publisher.discarded) + + viewModel.reset() + + assertEquals(listOf(staged), publisher.discarded) + assertFalse(staged.exists()) + } + + /** A ViewModel driven all the way to [JoinState.Joined]. */ + private fun joinedViewModel(): JoinViewModel { + // Unconfined so reset()'s delete runs inline instead of on a real IO thread. + val viewModel = JoinViewModel(app, Dispatchers.Unconfined) + viewModel.onInputsPicked(listOf(Uri.parse("content://test/a.mp4"), Uri.parse("content://test/b.mp4"))) + awaitState(viewModel.state, "Ready") { it is JoinState.Ready } + viewModel.join() + awaitState(viewModel.state, "Joined") { it is JoinState.Joined } + return viewModel + } + + private companion object { + val DESTINATION: Uri = Uri.parse("content://test/destination.mp4") + } +} diff --git a/app/src/test/java/org/libremediaconverter/join/JoinViewModelNamingTest.kt b/app/src/test/java/org/libremediaconverter/join/JoinViewModelNamingTest.kt new file mode 100644 index 0000000..459426c --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/join/JoinViewModelNamingTest.kt @@ -0,0 +1,113 @@ +package org.libremediaconverter.join + +import android.app.Application +import android.net.Uri +import androidx.media3.common.util.UnstableApi +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.convert.ConversionDependencies +import org.libremediaconverter.convert.RecordingPublisher +import org.libremediaconverter.convert.awaitState +import org.libremediaconverter.convert.installTestWorkManager +import org.libremediaconverter.model.ConcatStrategy +import org.libremediaconverter.work.ConcatWorker +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment +import java.io.File + +/** + * That a join is named after the format it produced. + * + * `JoinState.Saved("joined.mp4")` was a literal, and so were the screen's `CreateDocument` + * MIME type and the name it launched with. All three agree with reality only because the join + * screen has no format picker and `ConcatWorker.request` defaults to MP4 — three copies of one + * assumption, none of which would notice the day a picker arrives. + * + * MKV throughout below, because it is the format the old literals get wrong. + */ +@UnstableApi +@RunWith(RobolectricTestRunner::class) +class JoinViewModelNamingTest { + + private lateinit var app: Application + private lateinit var publisher: RecordingPublisher + private lateinit var staged: File + + @Before + fun setUp() { + app = RuntimeEnvironment.getApplication() + publisher = RecordingPublisher(app) + ConversionDependencies.publisher = { publisher } + + staged = publisher.createStagingFile("staged-under-a-job-id.mkv").apply { writeBytes(ByteArray(4096)) } + installTestWorkManager( + app, + workDataOf( + ConcatWorker.KEY_OUTPUT_PATH to staged.absolutePath, + ConcatWorker.KEY_STRATEGY to ConcatStrategy.STREAM_COPY.name, + ConcatWorker.KEY_SUGGESTED_NAME to "joined.mkv", + ConcatWorker.KEY_MIME_TYPE to "video/x-matroska", + ), + ) + } + + @After + fun tearDown() { + ConversionDependencies.reset() + } + + @Test + fun `a finished join carries the name and type its own format produced`() { + val joined = joinedViewModel().state.value as JoinState.Joined + + assertEquals("joined.mkv", joined.suggestedName) + assertEquals("video/x-matroska", joined.mimeType) + } + + @Test + fun `saving reports that name rather than a hardcoded one`() { + val viewModel = joinedViewModel() + + viewModel.save(DESTINATION) + val saved = awaitState(viewModel.state, "Saved") { it is JoinState.Saved } + + assertEquals("joined.mkv", (saved as JoinState.Saved).displayName) + } + + @Test + fun `a join from before the worker reported its own name still gets one`() { + // Work enqueued by an earlier version carries neither string. The fallback is the format + // ConcatWorker.request has always defaulted to, which is what such a job really used. + installTestWorkManager( + app, + workDataOf( + ConcatWorker.KEY_OUTPUT_PATH to staged.absolutePath, + ConcatWorker.KEY_STRATEGY to ConcatStrategy.STREAM_COPY.name, + ), + ) + + val joined = joinedViewModel().state.value as JoinState.Joined + + assertEquals("joined.mp4", joined.suggestedName) + assertEquals("video/mp4", joined.mimeType) + } + + /** A ViewModel driven all the way to [JoinState.Joined]. */ + private fun joinedViewModel(): JoinViewModel { + val viewModel = JoinViewModel(app, Dispatchers.Unconfined) + viewModel.onInputsPicked(listOf(Uri.parse("content://test/a.mkv"), Uri.parse("content://test/b.mkv"))) + awaitState(viewModel.state, "Ready") { it is JoinState.Ready } + viewModel.join() + awaitState(viewModel.state, "Joined") { it is JoinState.Joined } + return viewModel + } + + private companion object { + val DESTINATION: Uri = Uri.parse("content://test/destination.mkv") + } +} diff --git a/app/src/test/java/org/libremediaconverter/work/DeniedForegroundStartTest.kt b/app/src/test/java/org/libremediaconverter/work/DeniedForegroundStartTest.kt new file mode 100644 index 0000000..2308a44 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/work/DeniedForegroundStartTest.kt @@ -0,0 +1,199 @@ +package org.libremediaconverter.work + +import android.app.Application +import android.app.ForegroundServiceStartNotAllowedException +import android.content.Context +import android.net.Uri +import androidx.media3.common.util.UnstableApi +import androidx.work.Data +import androidx.work.ForegroundInfo +import androidx.work.ForegroundUpdater +import androidx.work.ListenableWorker +import androidx.work.testing.TestListenableWorkerBuilder +import androidx.work.workDataOf +import com.google.common.util.concurrent.ListenableFuture +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.StagingNames +import org.libremediaconverter.convert.installTestWorkManager +import org.libremediaconverter.model.OutputFormat +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment +import java.io.File +import java.util.UUID +import java.util.concurrent.ExecutionException +import java.util.concurrent.Executor +import java.util.concurrent.TimeUnit + +/** + * That a refused foreground-service start does not end the job. + * + * The wiring half of [FailureOutcomeTest], and the half the defect actually lived in. + * `setForeground()` used to sit *above* the `try` in both workers, so the exception the system + * throws when it refuses a background foreground-service start escaped `doWork()` altogether: + * WorkManager logged `Worker result FAILURE` and `reschedule = false`, the output `Data` reached + * the UI with zero entries, and the partial file the killed attempt had left in staging was never + * deleted. Confirmed on a Pixel 10 Pro XL — 119 seconds after a `kill -9`, WorkManager recovered + * the job unprompted and the system denied it. + * + * None of that is reproducible here, so what is reproduced is the single cause of it: the throw. + * A [ForegroundUpdater] whose future completes exceptionally makes `setForeground()` throw exactly + * what the platform throws — `WorkForegroundUpdater` deliberately propagates it rather than + * swallowing it, and `ListenableFuture.await()` unwraps the `ExecutionException`, so the worker + * meets it bare. + */ +@UnstableApi +@RunWith(RobolectricTestRunner::class) +class DeniedForegroundStartTest { + + private lateinit var app: Application + private lateinit var publisher: OutputPublisher + + @Before + fun setUp() { + app = RuntimeEnvironment.getApplication() + publisher = AlwaysRoomPublisher(app) + ConversionDependencies.publisher = { publisher } + // The progress notification builds its cancel action from WorkManager.getInstance(), which + // throws when nothing has initialised it. Without this the worker would fail for that + // reason rather than the one under test, and the assertions would still pass. + installTestWorkManager(app, Data.EMPTY) + } + + @After + fun tearDown() { + ConversionDependencies.reset() + } + + @Test + fun `a conversion whose foreground start is denied retries instead of failing terminally`() { + val result = runBlocking { conversionWorker().doWork() } + + // Retry, not failure: the denial is about when the job ran, not about the job. Terminal + // failure is what the device showed, and it is what "the queue survives process death" + // cannot survive. + assertEquals(ListenableWorker.Result.retry(), result) + } + + @Test + fun `a join whose foreground start is denied retries instead of failing terminally`() { + val result = runBlocking { concatWorker().doWork() } + + assertEquals(ListenableWorker.Result.retry(), result) + } + + @Test + fun `a denied foreground start collects the partial file the killed attempt left behind`() { + // Exactly the 2 MB orphan the device pass found. A process killed mid-transcode leaves a + // partial in staging, and the attempt WorkManager schedules to recover it stages under the + // same name -- the job id does not move across a retry -- so reaching staged.delete() is + // what collects it. + stagedFile().writeBytes(ByteArray(PARTIAL_BYTES)) + + runBlocking { conversionWorker().doWork() } + + // Asserted against the whole directory rather than one path. A path this test computes + // itself can stop matching the one the worker computes, and then the assertion passes by + // asking whether a file nobody wrote is absent. + assertEquals( + "a denied restart must not orphan the previous attempt's partial", + emptyList(), + stagedNames(), + ) + } + + @Test + fun `a start denied past the attempt bound fails with a message the user can act on`() { + val worker = conversionWorker(runAttemptCount = FailureOutcome.MAX_FOREGROUND_START_ATTEMPTS) + + val result = runBlocking { worker.doWork() } + + // Not merely "a failure". The defect's other half was output `Data` with zero entries, so + // the UI rendered its generic fallback with nothing to say. `Failure.equals` compares + // output data, which pins the message as well as the verdict. + assertEquals( + ListenableWorker.Result.failure( + workDataOf(ConversionWorker.KEY_ERROR to FailureOutcome.FOREGROUND_DENIED_MESSAGE), + ), + result, + ) + } + + private fun conversionWorker(runAttemptCount: Int = 0): 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, + ), + runAttemptCount = runAttemptCount, + ).setId(CONVERSION_ID) + .setForegroundUpdater(DenyingForegroundUpdater) + .build() + + private fun concatWorker(): ConcatWorker = TestListenableWorkerBuilder( + context = app, + inputData = workDataOf( + ConcatWorker.KEY_INPUT_URIS to arrayOf(INPUT.toString(), "content://test/second.mp4"), + ConcatWorker.KEY_TOTAL_BYTES to INPUT_BYTES, + ConcatWorker.KEY_FORMAT to OutputFormat.MP4_H264.name, + ), + runAttemptCount = 0, + ).setId(CONCAT_ID) + .setForegroundUpdater(DenyingForegroundUpdater) + .build() + + /** The staging path the worker will compute, asked for rather than spelled out here. */ + private fun stagedFile(): File = publisher.createStagingFile(StagingNames.forJob(CONVERSION_ID, SPEC.extension)) + + private fun stagedNames(): List = stagedFile().parentFile?.listFiles().orEmpty().map { it.name }.sorted() + + private companion object { + val INPUT: Uri = Uri.parse("content://test/holiday.mp4") + const val DISPLAY_NAME = "holiday.mp4" + const val INPUT_BYTES = 1024L + const val PARTIAL_BYTES = 2048 + val SPEC = OutputFormat.MP4_H265.spec + val CONVERSION_ID: UUID = UUID.fromString("00000000-0000-4000-8000-000000000001") + val CONCAT_ID: UUID = UUID.fromString("00000000-0000-4000-8000-000000000002") + } +} + +/** Stands in for the system refusing a background foreground-service start. */ +private object DenyingForegroundUpdater : ForegroundUpdater { + override fun setForegroundAsync( + context: Context, + id: UUID, + foregroundInfo: ForegroundInfo, + ): ListenableFuture = FailedFuture( + ForegroundServiceStartNotAllowedException( + "startForegroundService() not allowed: service " + + "org.libremediaconverter/androidx.work.impl.foreground.SystemForegroundService", + ), + ) +} + +/** + * An already-failed future, written out rather than pulled from a futures library. + * + * `await()` takes the `isDone` fast path and unwraps the `ExecutionException`, which is what puts + * the platform's own exception in front of the worker's catch rather than a wrapper. + */ +private class FailedFuture(private val failure: Throwable) : ListenableFuture { + override fun addListener(listener: Runnable, executor: Executor): Unit = executor.execute(listener) + override fun cancel(mayInterruptIfRunning: Boolean): Boolean = false + override fun isCancelled(): Boolean = false + override fun isDone(): Boolean = true + override fun get(): Void = throw ExecutionException(failure) + override fun get(timeout: Long, unit: TimeUnit): Void = throw ExecutionException(failure) +} diff --git a/app/src/test/java/org/libremediaconverter/work/FailureOutcomeTest.kt b/app/src/test/java/org/libremediaconverter/work/FailureOutcomeTest.kt index 6ac619e..d342137 100644 --- a/app/src/test/java/org/libremediaconverter/work/FailureOutcomeTest.kt +++ b/app/src/test/java/org/libremediaconverter/work/FailureOutcomeTest.kt @@ -1,17 +1,27 @@ package org.libremediaconverter.work +import android.app.ForegroundServiceStartNotAllowedException import androidx.work.WorkInfo import org.junit.Assert.assertEquals import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner /** * The retry-versus-fail rule. * - * Isolated from the worker precisely so it can be tested: the condition that triggers - * a retry in production is the foreground-service budget running out, six hours per - * twenty-four, which no test can reach. Extracting the decision means the rule is still - * verified even though its trigger cannot be reproduced. + * Isolated from the worker precisely so it can be tested: neither condition that triggers a retry + * in production is reproducible. One is the foreground-service budget running out, six hours per + * twenty-four, which no test can reach. The other is the system refusing a background + * foreground-service start, which needs a process death and a WorkManager recovery on a real + * device. Extracting the decision means the rule is still verified even though its triggers are + * not. + * + * Robolectric only for [ForegroundServiceStartNotAllowedException]: it is a platform class, and + * the stub `android.jar` the JVM tests compile against throws from every constructor. Nothing else + * here needs an Android runtime. */ +@RunWith(RobolectricTestRunner::class) class FailureOutcomeTest { @Test @@ -20,7 +30,7 @@ class FailureOutcomeTest { // user their conversion failed would be wrong. assertEquals( FailureOutcome.RETRY, - FailureOutcome.forStopReason(WorkInfo.STOP_REASON_FOREGROUND_SERVICE_TIMEOUT), + FailureOutcome.forFailure(WorkInfo.STOP_REASON_FOREGROUND_SERVICE_TIMEOUT), ) } @@ -28,7 +38,7 @@ class FailureOutcomeTest { fun `an ordinary failure is reported as a failure`() { assertEquals( FailureOutcome.FAIL, - FailureOutcome.forStopReason(WorkInfo.STOP_REASON_NOT_STOPPED), + FailureOutcome.forFailure(WorkInfo.STOP_REASON_NOT_STOPPED), ) } @@ -52,7 +62,90 @@ class FailureOutcomeTest { WorkInfo.STOP_REASON_UNKNOWN, ) others.forEach { - assertEquals("stop reason $it should fail", FailureOutcome.FAIL, FailureOutcome.forStopReason(it)) + assertEquals("stop reason $it should fail", FailureOutcome.FAIL, FailureOutcome.forFailure(it)) } } + + // --- a refused foreground-service start --------------------------------------------------- + + @Test + fun `a denied foreground start is a retry, not a terminal failure`() { + // The device pass caught this returning FAILURE with reschedule = false, which loses an + // hour of transcoding to a condition that clears the moment the user opens the app. + assertEquals( + FailureOutcome.RETRY, + FailureOutcome.forFailure(WorkInfo.STOP_REASON_NOT_STOPPED, denied(), runAttemptCount = 0), + ) + } + + @Test + fun `a denied foreground start keeps retrying up to the bound`() { + (0 until FailureOutcome.MAX_FOREGROUND_START_ATTEMPTS).forEach { attempt -> + assertEquals( + "attempt $attempt should still retry", + FailureOutcome.RETRY, + FailureOutcome.forFailure(WorkInfo.STOP_REASON_NOT_STOPPED, denied(), attempt), + ) + } + } + + @Test + fun `a denied foreground start gives up once the bound is reached`() { + // The alternative is a job that is never told to stop and never tells the user anything: + // WorkManager retries forever, and the screen says "paused" for as long as the app lives. + assertEquals( + FailureOutcome.FOREGROUND_DENIED, + FailureOutcome.forFailure( + WorkInfo.STOP_REASON_NOT_STOPPED, + denied(), + FailureOutcome.MAX_FOREGROUND_START_ATTEMPTS, + ), + ) + } + + @Test + fun `an unrelated IllegalStateException is not mistaken for a denied start`() { + // ForegroundServiceStartNotAllowedException extends IllegalStateException, and plenty of + // ordinary failures are IllegalStateExceptions -- a muxer that was never started, a + // provider that closed. Matching the supertype would retry all of them forever. + assertEquals( + FailureOutcome.FAIL, + FailureOutcome.forFailure( + WorkInfo.STOP_REASON_NOT_STOPPED, + IllegalStateException("muxer was not started"), + runAttemptCount = 0, + ), + ) + } + + @Test + fun `a stop the system asked for wins over the exception it caused`() { + // Precedence, stated rather than left to fall out of the branch order. Once something + // stopped the worker, the exception it was holding at the time describes the stop, not a + // reason of its own -- and retrying past a cancellation would ignore the user. + assertEquals( + FailureOutcome.FAIL, + FailureOutcome.forFailure(WorkInfo.STOP_REASON_CANCELLED_BY_APP, denied(), runAttemptCount = 0), + ) + } + + @Test + fun `the foreground budget still earns a retry however many attempts have been made`() { + // The bound belongs to the denial, not to the timeout: a six-hour transcode legitimately + // outlives more than ten daily budgets, and failing it for that would be the opposite of + // what the timeout branch exists for. + assertEquals( + FailureOutcome.RETRY, + FailureOutcome.forFailure( + WorkInfo.STOP_REASON_FOREGROUND_SERVICE_TIMEOUT, + denied(), + FailureOutcome.MAX_FOREGROUND_START_ATTEMPTS * 2, + ), + ) + } + + private fun denied() = ForegroundServiceStartNotAllowedException( + "startForegroundService() not allowed: service " + + "org.libremediaconverter/androidx.work.impl.foreground.SystemForegroundService", + ) } diff --git a/app/src/test/java/org/libremediaconverter/work/JobTagsTest.kt b/app/src/test/java/org/libremediaconverter/work/JobTagsTest.kt new file mode 100644 index 0000000..1595787 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/work/JobTagsTest.kt @@ -0,0 +1,73 @@ +package org.libremediaconverter.work + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull +import org.junit.Test + +/** + * What has to survive a restart, and what a malformed tag does. + * + * The values come from a picker and go into WorkManager's database, so the encoder and the + * decoder are the two halves of one round trip and are tested as one. The lenient reads + * matter as much as the round trip: work enqueued by an older version of the app carries none + * of these tags, and it is exactly the work most likely to still be queued the first time + * this code runs. + */ +class JobTagsTest { + + @Test + fun `a display name survives the round trip`() { + val tags = setOf("org.libremediaconverter.work.ConversionWorker", JobTags.displayName("holiday.mp4")) + assertEquals("holiday.mp4", JobTags.displayNameOf(tags)) + } + + @Test + fun `a display name that looks like another tag is still read back whole`() { + // Tags are matched by prefix over the whole string, so a file named after one of the + // other prefixes cannot be mistaken for it. + val name = "lmc.size-bytes:9" + val tags = setOf(JobTags.displayName(name), JobTags.sizeBytes(4096)) + assertEquals(name, JobTags.displayNameOf(tags)) + assertEquals(4096L, JobTags.sizeBytesOf(tags)) + } + + @Test + fun `a display name with spaces, colons and unicode is carried verbatim`() { + val name = "холидей: clip 2 — final.mkv" + assertEquals(name, JobTags.displayNameOf(setOf(JobTags.displayName(name)))) + } + + @Test + fun `a size survives the round trip`() { + assertEquals(9_000_000_000L, JobTags.sizeBytesOf(setOf(JobTags.sizeBytes(9_000_000_000L)))) + } + + @Test + fun `an input count survives the round trip`() { + assertEquals(7, JobTags.inputCountOf(setOf(JobTags.inputCount(7)))) + } + + @Test + fun `a job with no tags of ours reads back as nothing known`() { + // Work enqueued before this app version. The reattachment falls back rather than + // skipping the job, because the job is still the user's file. + val tags = setOf("org.libremediaconverter.work.ConversionWorker") + assertNull(JobTags.displayNameOf(tags)) + assertNull(JobTags.sizeBytesOf(tags)) + assertNull(JobTags.inputCountOf(tags)) + } + + @Test + fun `a size that is not a number reads as unknown rather than throwing`() { + assertNull(JobTags.sizeBytesOf(setOf("lmc.size-bytes:huge"))) + assertNull(JobTags.inputCountOf(setOf("lmc.input-count:"))) + } + + @Test + fun `the three tags do not read each other`() { + val tags = setOf(JobTags.displayName("clip.mp4"), JobTags.sizeBytes(12), JobTags.inputCount(3)) + assertEquals("clip.mp4", JobTags.displayNameOf(tags)) + assertEquals(12L, JobTags.sizeBytesOf(tags)) + assertEquals(3, JobTags.inputCountOf(tags)) + } +} diff --git a/app/src/test/java/org/libremediaconverter/work/PerJobStagingTest.kt b/app/src/test/java/org/libremediaconverter/work/PerJobStagingTest.kt new file mode 100644 index 0000000..0716e7b --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/work/PerJobStagingTest.kt @@ -0,0 +1,135 @@ +package org.libremediaconverter.work + +import android.app.Application +import android.net.Uri +import androidx.media3.common.util.UnstableApi +import androidx.work.Data +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.Assert.assertFalse +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.installTestWorkManager +import org.libremediaconverter.model.DeviceCodecs +import org.libremediaconverter.model.EnginePreference +import org.libremediaconverter.model.InputProbe +import org.libremediaconverter.model.OutputFormat +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment +import java.io.File +import java.util.UUID + +/** + * That two jobs cannot write the same staged file. + * + * Observed rather than imagined: two independent conversions on a Pixel each computed + * `cache/conversions/input_converted.mp4`, the second overwrote the first, and a tag query in a + * fresh process then returned two SUCCEEDED `WorkInfo`s naming that one file — which is what + * makes reattachment ambiguous about which job produced what is on disk. + * + * These drive the real worker rather than the naming function, because the naming function was + * never the part that was wrong. What was wrong is which name the worker asked for. + */ +@UnstableApi +@RunWith(RobolectricTestRunner::class) +class PerJobStagingTest { + + private lateinit var app: Application + private lateinit var publisher: OutputPublisher + private lateinit var stagingDir: File + + @Before + fun setUp() { + app = RuntimeEnvironment.getApplication() + publisher = AlwaysRoomPublisher(app) + ConversionDependencies.publisher = { publisher } + ConversionDependencies.probe = { _, _ -> InputProbe() } + ConversionDependencies.deviceCodecs = { DeviceCodecs.PERMISSIVE } + ConversionDependencies.software = { WritingTranscoder } + installTestWorkManager(app, Data.EMPTY) + + stagingDir = publisher.createStagingFile("anything").parentFile!! + stagingDir.listFiles()?.forEach { it.delete() } + } + + @After + fun tearDown() { + ConversionDependencies.reset() + } + + @Test + fun `two conversions of the same file stage under names of their own`() { + runBlocking { conversionWorker(JOB_A).doWork() } + runBlocking { conversionWorker(JOB_B).doWork() } + + // Two jobs, two files. One file here means the second job overwrote the first's output + // while both went on reporting that path as their result. + assertEquals( + "each job must have staged its own file, found ${stagedNames()}", + 2, + stagedNames().size, + ) + } + + @Test + fun `a second attempt at one job reuses the first attempt's staging path`() { + runBlocking { conversionWorker(JOB_A, runAttemptCount = 0).doWork() } + val first = stagedNames() + + runBlocking { conversionWorker(JOB_A, runAttemptCount = 1).doWork() } + + // A per-attempt name would leak one file per retry, and would stop the catch on the way + // out of a failed attempt from collecting the partial the previous one left. + assertEquals("a retry must not stage under a new name", first, stagedNames()) + } + + @Test + fun `a display name that tries to climb out of staging cannot`() { + // Display names come from a document provider and are not this app's to trust: one can + // contain a separator, be empty, or be four kilobytes long. Deriving the staged path from + // it put all of that on a filesystem path. Naming the job instead retires the question + // rather than answering it with a sanitiser. + runBlocking { conversionWorker(JOB_A, displayName = "../escape.mp4").doWork() } + + assertEquals("the output belongs in staging", 1, stagedNames().size) + assertFalse( + "nothing may be written outside the staging directory", + File(app.cacheDir, "escape_converted.mp4").exists(), + ) + } + + private fun stagedNames(): List = stagingDir.listFiles().orEmpty().map { it.name }.sorted() + + private fun conversionWorker( + id: UUID, + runAttemptCount: Int = 0, + displayName: String = DISPLAY_NAME, + ): ConversionWorker = TestListenableWorkerBuilder( + context = app, + inputData = workDataOf( + ConversionWorker.KEY_INPUT_URI to INPUT.toString(), + ConversionWorker.KEY_DISPLAY_NAME to displayName, + 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_ENGINE_PREFERENCE to EnginePreference.FORCE_SOFTWARE.name, + ), + runAttemptCount = runAttemptCount, + ).setId(id).build() + + private companion object { + val INPUT: Uri = Uri.parse("file:///tmp/input.mp4") + const val DISPLAY_NAME = "input.mp4" + const val INPUT_BYTES = 1024L + val SPEC = OutputFormat.MP4_H265.spec + val JOB_A: UUID = UUID.fromString("00000000-0000-4000-8000-00000000000a") + val JOB_B: UUID = UUID.fromString("00000000-0000-4000-8000-00000000000b") + } +} diff --git a/app/src/test/java/org/libremediaconverter/work/ProgressNotificationTest.kt b/app/src/test/java/org/libremediaconverter/work/ProgressNotificationTest.kt new file mode 100644 index 0000000..8dc51d9 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/work/ProgressNotificationTest.kt @@ -0,0 +1,213 @@ +package org.libremediaconverter.work + +import android.app.Application +import android.app.Notification +import android.app.NotificationManager +import android.content.Context +import android.net.Uri +import androidx.media3.common.util.UnstableApi +import androidx.work.Data +import androidx.work.ForegroundInfo +import androidx.work.WorkInfo +import androidx.work.testing.TestForegroundUpdater +import androidx.work.testing.TestListenableWorkerBuilder +import androidx.work.workDataOf +import com.google.common.util.concurrent.ListenableFuture +import kotlinx.coroutines.runBlocking +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.libremediaconverter.convert.ConversionDependencies +import org.libremediaconverter.convert.SoftwareTranscoder +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.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment +import org.robolectric.Shadows.shadowOf +import java.io.File +import java.util.UUID + +/** + * That progress goes through WorkManager rather than around it. + * + * `publishProgress` called `NotificationManager.notify(1001, …)` directly, on the very id + * WorkManager owns through `setForeground`, with a notification built `setOngoing(true)`. Two + * owners of one id is a race, and on a Pixel 10 Pro XL it was lost on attempt 3 of 12 while + * cancelling a `BEST`-tier job: + * + * ``` + * attempt 3: terminal state = CANCELLED + * attempt 3: +300ms active=0 id1001=false ongoing=null <- WorkManager tore it down + * attempt 3: +700ms active=1 id1001=true ongoing=true <- a progress tick put it back + * attempt 3: +5000ms active=1 id1001=true ongoing=true + * ``` + * + * Still there ten minutes later with no app process at all. The record carried + * `flags=ONGOING_EVENT|ONLY_ALERT_ONCE` and **no `FOREGROUND_SERVICE`**, which is what proves the + * direct `notify` posted it rather than `setForeground` — WorkManager's own post carries that flag. + * Whether the orphan could be swiped away was never established and is not what these tests are + * about; the resurrection is, and it is reproduced. + */ +@UnstableApi +@RunWith(RobolectricTestRunner::class) +class ProgressNotificationTest { + + private lateinit var app: Application + private lateinit var updater: RecordingForegroundUpdater + + @Before + fun setUp() { + app = RuntimeEnvironment.getApplication() + updater = RecordingForegroundUpdater() + ConversionDependencies.publisher = { AlwaysRoomPublisher(app) } + ConversionDependencies.probe = { _, _ -> InputProbe() } + ConversionDependencies.deviceCodecs = { DeviceCodecs.PERMISSIVE } + // The notification's cancel action is a WorkManager PendingIntent, so the worker would + // fail for that reason rather than the one under test without this. + installTestWorkManager(app, Data.EMPTY) + } + + @After + fun tearDown() { + ConversionDependencies.reset() + } + + @Test + fun `progress on a running worker updates WorkManager's own notification, not one of ours`() { + runBlocking { workerReporting { onProgress -> onProgress(PERCENT) }.doWork() } + + // The positive half: the update really happened, on the id WorkManager is holding, and it + // carries the percentage. Asserting only that nothing was posted directly would pass just + // as well against a `publishProgress` that had been deleted. + val progressUpdates = updater.infos.drop(1) + assertEquals("one throttled progress update expected", 1, progressUpdates.size) + assertEquals(updater.infos.first().notificationId, progressUpdates.single().notificationId) + assertEquals(PERCENT, progressUpdates.single().notification.extras.getInt(Notification.EXTRA_PROGRESS)) + + // And the negative half: nothing reached the notification manager under its own steam. + // Asserted over the whole manager rather than one id, so a renamed constant cannot make + // this pass by asking about a notification nobody posts. + assertEquals("the worker must post no notification of its own", 0, postedNotifications()) + } + + @Test + fun `a progress update that lands after the worker is stopped puts nothing back`() { + // The device sequence, in one worker: WorkManager has torn the notification down, and a + // tick that was already in flight arrives afterwards. `lastNotified` is still 0 here, so + // this tick is one the throttle would have let through -- which is what makes the test + // about `isStopped` rather than about timing. + val worker = workerReporting { onProgress -> + stopped(WorkInfo.STOP_REASON_CANCELLED_BY_APP) + onProgress(PERCENT) + } + + runBlocking { worker.doWork() } + + assertEquals("a stopped worker must publish nothing", 1, updater.infos.size) + assertEquals("and must resurrect nothing", 0, postedNotifications()) + } + + @Test + fun `progress arriving several times a second is still throttled to one update`() { + // The throttle is not decoration: FFmpeg's statistics callback and Media3's progress + // polling both fire several times a second, and pushing every one of them janks the + // system UI. Routing progress through `setForeground` does not make that cheaper. + runBlocking { + workerReporting { onProgress -> repeat(TICKS) { onProgress(it) } }.doWork() + } + + assertTrue( + "$TICKS ticks inside one throttle window must not be ${updater.infos.size - 1} updates", + updater.infos.size - 1 == 1, + ) + } + + /** + * A worker routed to the software engine, whose engine is [report] and a written output. + * + * `FORCE_SOFTWARE` because it is the one preference that decides without consulting the input, + * and a `file://` URI because a `content://` one would send the worker through FFmpegKit's SAF + * bridge, which is native. [report] is handed the worker's own progress callback, and runs with + * the worker as its receiver so a test can stop it mid-transcode. + */ + private fun workerReporting(report: ConversionWorker.((Int) -> Unit) -> Unit): ConversionWorker { + val worker = 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_ENGINE_PREFERENCE to EnginePreference.FORCE_SOFTWARE.name, + ), + runAttemptCount = 0, + ).setId(JOB_ID) + .setForegroundUpdater(updater) + .build() + // Resolved when the worker reaches the engine, so assigning after `build()` is in time. + ConversionDependencies.software = { ReportingTranscoder { onProgress -> worker.report(onProgress) } } + return worker + } + + /** Stops the worker the way WorkManager does, so `isStopped` becomes true. */ + private fun ConversionWorker.stopped(reason: Int) = stop(reason) + + private fun postedNotifications(): Int = shadowOf(app.getSystemService(NotificationManager::class.java)).size() + + private companion object { + val INPUT: Uri = Uri.parse("file:///tmp/holiday.mp4") + const val DISPLAY_NAME = "holiday.mp4" + const val INPUT_BYTES = 1024L + const val PERCENT = 42 + const val TICKS = 50 + val SPEC = OutputFormat.MP4_H265.spec + val JOB_ID: UUID = UUID.fromString("00000000-0000-4000-8000-000000000021") + } +} + +/** + * Records every [ForegroundInfo] the worker publishes, and otherwise behaves as the test default. + * + * Delegating to [TestForegroundUpdater] rather than hand-rolling a `ListenableFuture`: the + * worker awaits what this returns, so a future that never completes would hang the initial + * `setForeground` rather than test anything. + */ +private class RecordingForegroundUpdater : TestForegroundUpdater() { + val infos = mutableListOf() + + override fun setForegroundAsync( + context: Context, + id: UUID, + foregroundInfo: ForegroundInfo, + ): ListenableFuture { + infos += foregroundInfo + return super.setForegroundAsync(context, id, foregroundInfo) + } +} + +/** An engine that reports whatever [report] wants reported, then writes an output. */ +private class ReportingTranscoder(private val report: ((Int) -> Unit) -> Unit) : SoftwareTranscoder { + override suspend fun run( + request: ConversionRequest, + inputPath: String, + output: File, + durationMs: Long, + onProgress: (Int) -> Unit, + ) { + report(onProgress) + output.writeBytes(ByteArray(OUTPUT_BYTES)) + } + + private companion object { + const val OUTPUT_BYTES = 512 + } +} diff --git a/app/src/test/java/org/libremediaconverter/work/ReattachmentTest.kt b/app/src/test/java/org/libremediaconverter/work/ReattachmentTest.kt new file mode 100644 index 0000000..42725ff --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/work/ReattachmentTest.kt @@ -0,0 +1,220 @@ +package org.libremediaconverter.work + +import androidx.work.WorkInfo +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull +import org.junit.Test +import java.util.UUID + +/** + * The rule for picking up a job the ViewModel did not start. + * + * Isolated from the ViewModel precisely so it can be tested: reaching this code for real means + * the process being reclaimed while a job — or a result nobody saved — is still around, which + * needs a device and an `am kill`. Extracting the choice means the rule is verified even though + * the situation that calls for it cannot be reproduced on the JVM. + */ +class ReattachmentTest { + + @Test + fun `nothing to reattach to when there is no work at all`() { + assertNull(Reattachment.choose(emptyList())) + } + + @Test + fun `a cancelled job is never reattached to`() { + // The user already said no. Bringing it back would undo that. + val cancelled = job(state = WorkInfo.State.CANCELLED, outputPath = "/cache/out.mp4", outputExists = true) + assertNull(Reattachment.choose(listOf(cancelled))) + } + + @Test + fun `a failed job is not reattached to`() { + // Nothing marks a failure as seen, so it would reappear on every launch. Failures left + // by an interrupted worker are ordinary: a restart's setForeground can be refused. + assertNull(Reattachment.choose(listOf(job(state = WorkInfo.State.FAILED)))) + } + + @Test + fun `a finished result still on disk is offered`() { + val result = job(state = WorkInfo.State.SUCCEEDED, outputPath = "/cache/out.mp4", outputExists = true) + assertEquals(Reattachment.Certain(result), Reattachment.choose(listOf(result))) + } + + @Test + fun `a finished result whose staged file is gone is not offered`() { + // Saved already, or the OS reclaimed the cache. A Save button here would fail on tap. + val vanished = job(state = WorkInfo.State.SUCCEEDED, outputPath = "/cache/out.mp4", outputExists = false) + assertNull(Reattachment.choose(listOf(vanished))) + } + + @Test + fun `a job that reported success without a path is not offered`() { + val pathless = job(state = WorkInfo.State.SUCCEEDED, outputPath = null, outputExists = false) + assertNull(Reattachment.choose(listOf(pathless))) + } + + @Test + fun `a running job is preferred to a finished result`() { + // A running job holds a foreground notification. Someone opening the app while that + // notification is in the shade is looking for that conversion. + val result = job(state = WorkInfo.State.SUCCEEDED, outputPath = "/cache/out.mp4", outputExists = true) + val running = job(state = WorkInfo.State.RUNNING) + assertEquals(Reattachment.Certain(running), Reattachment.choose(listOf(result, running))) + } + + @Test + fun `a running job is preferred to a queued one`() { + val queued = job(state = WorkInfo.State.ENQUEUED) + val running = job(state = WorkInfo.State.RUNNING) + assertEquals(Reattachment.Certain(running), Reattachment.choose(listOf(queued, running))) + } + + @Test + fun `a job waiting to retry is preferred to one that has never run`() { + // It has already done part of the work — most likely it exhausted the foreground + // budget mid-conversion — so it is the one closer to producing a file. + val fresh = job(state = WorkInfo.State.ENQUEUED, runAttemptCount = 0) + val retrying = job(state = WorkInfo.State.ENQUEUED, runAttemptCount = 1) + assertEquals(Reattachment.Certain(retrying), Reattachment.choose(listOf(fresh, retrying))) + } + + @Test + fun `a queued job is preferred to a finished result`() { + val result = job(state = WorkInfo.State.SUCCEEDED, outputPath = "/cache/out.mp4", outputExists = true) + val queued = job(state = WorkInfo.State.ENQUEUED) + assertEquals(Reattachment.Certain(queued), Reattachment.choose(listOf(result, queued))) + } + + @Test + fun `blocked work counts as queued rather than being ignored`() { + val blocked = job(state = WorkInfo.State.BLOCKED) + assertEquals(Reattachment.Certain(blocked), Reattachment.choose(listOf(blocked))) + } + + @Test + fun `the newer of two results is the one offered`() { + // Losing this tie is not the same as waiting for the next launch: the query has no + // ORDER BY, so an arbitrary winner would win every launch and the other result would + // stay unreachable for as long as its file existed. + val older = finishedResult(STAGED, tags = emptySet(), modifiedAt = 1_000L) + val newer = finishedResult("/cache/beach_converted.mp4", tags = emptySet(), modifiedAt = 2_000L) + + assertEquals(Reattachment.Certain(newer), Reattachment.choose(listOf(older, newer))) + } + + @Test + fun `a newer result still does not outrank live work`() { + // Rank first, time second. A running job holds the notification the user is following. + val running = job(state = WorkInfo.State.RUNNING) + val newer = finishedResult(STAGED, tags = emptySet(), modifiedAt = Long.MAX_VALUE) + + assertEquals(Reattachment.Certain(running), Reattachment.choose(listOf(newer, running))) + } + + @Test + fun `two live jobs, which have written nothing to compare, resolve to the query's order`() { + val first = job(state = WorkInfo.State.RUNNING) + val second = job(state = WorkInfo.State.RUNNING) + assertEquals(Reattachment.Certain(first), Reattachment.choose(listOf(first, second))) + } + + @Test + fun `a result is still found when everything else is unusable`() { + val result = job(state = WorkInfo.State.SUCCEEDED, outputPath = "/cache/out.mp4", outputExists = true) + val jobs = listOf( + job(state = WorkInfo.State.CANCELLED), + job(state = WorkInfo.State.FAILED), + job(state = WorkInfo.State.SUCCEEDED, outputPath = "/cache/gone.mp4", outputExists = false), + result, + ) + assertEquals(Reattachment.Certain(result), Reattachment.choose(jobs)) + } + + // --- when two jobs claim the same staged file --------------------------------------------- + + @Test + fun `two results naming the same file are still offered, but not attributed`() { + // Straight off a device: two SUCCEEDED jobs whose output path was the same + // input_converted.mp4, with one file on disk. The file is the user's either way; which + // job wrote it is not knowable, so the caller is told not to describe it. + val first = finishedResult(STAGED, tags = setOf(JobTags.displayName("holiday.mp4"))) + val second = finishedResult(STAGED, tags = setOf(JobTags.displayName("holiday.mkv"))) + + assertEquals(Reattachment.Ambiguous(first), Reattachment.choose(listOf(first, second))) + } + + @Test + fun `aliases that describe the same input are attributed after all`() { + // The ordinary way to end up with two: convert the same file twice. Nothing turns on + // which of them wrote the file, so the label is safe. + val tags = setOf(JobTags.displayName("holiday.mp4"), JobTags.sizeBytes(4_096)) + val first = finishedResult(STAGED, tags = tags) + val second = finishedResult(STAGED, tags = tags) + + assertEquals(Reattachment.Certain(first), Reattachment.choose(listOf(first, second))) + } + + @Test + fun `results naming different files do not make each other ambiguous`() { + val first = finishedResult(STAGED, tags = setOf(JobTags.displayName("holiday.mp4"))) + val second = finishedResult("/cache/beach_converted.mp4", tags = setOf(JobTags.displayName("beach.mp4"))) + + assertEquals(Reattachment.Certain(first), Reattachment.choose(listOf(first, second))) + } + + @Test + fun `a job with no file yet is not aliased by every other job without one`() { + // Guards the obvious mistake: live jobs all carry a null output path, and grouping on + // that would make each of them ambiguous with all the others. + val running = job(state = WorkInfo.State.RUNNING, tags = setOf(JobTags.displayName("holiday.mp4"))) + val queued = job(state = WorkInfo.State.ENQUEUED, tags = setOf(JobTags.displayName("beach.mp4"))) + + assertEquals(Reattachment.Certain(running), Reattachment.choose(listOf(running, queued))) + } + + @Test + fun `an unusable alias does not make a result ambiguous`() { + // A cancelled or failed job deletes its staged file on the way out, so it never wrote + // what is on disk now and says nothing about who did. + val result = finishedResult(STAGED, tags = setOf(JobTags.displayName("holiday.mp4"))) + val abandoned = job( + state = WorkInfo.State.CANCELLED, + outputPath = STAGED, + outputExists = true, + tags = setOf(JobTags.displayName("something else.mp4")), + ) + + assertEquals(Reattachment.Certain(result), Reattachment.choose(listOf(result, abandoned))) + } + + private fun finishedResult(path: String, tags: Set, modifiedAt: Long = 0L) = job( + state = WorkInfo.State.SUCCEEDED, + outputPath = path, + outputExists = true, + tags = tags, + modifiedAt = modifiedAt, + ) + + private fun job( + state: WorkInfo.State, + runAttemptCount: Int = 0, + outputPath: String? = null, + outputExists: Boolean = false, + tags: Set = emptySet(), + modifiedAt: Long = 0L, + ) = JobSnapshot( + id = UUID.randomUUID(), + state = state, + runAttemptCount = runAttemptCount, + outputPath = outputPath, + outputExists = outputExists, + outputModifiedAt = modifiedAt, + tags = tags, + ) + + private companion object { + /** One staging path, because the interesting cases are the ones that share it. */ + const val STAGED = "/cache/holiday_converted.mp4" + } +} diff --git a/app/src/test/java/org/libremediaconverter/work/SpaceCheckTest.kt b/app/src/test/java/org/libremediaconverter/work/SpaceCheckTest.kt new file mode 100644 index 0000000..cf35cdd --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/work/SpaceCheckTest.kt @@ -0,0 +1,251 @@ +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.Assert.assertTrue +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.installTestWorkManager +import org.libremediaconverter.model.DeviceCodecs +import org.libremediaconverter.model.EnginePreference +import org.libremediaconverter.model.InputProbe +import org.libremediaconverter.model.OutputFormat +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment +import java.io.File +import java.util.UUID + +/** + * What the space check is actually asked, which is where D5 lived. + * + * The worker read its input size out of `Data` with `getLong(KEY_SIZE_BYTES, 0L)`, so a job whose + * size nobody could report asked "is there room for 0 bytes?" — which the headroom answers yes to + * on any device with 128 MB free, whatever the file turns out to weigh. + * + * These assert on the *question*, not on the verdict. A test that only checked whether the job ran + * would pass against the defect: the defect is that the guard is vacuous, not that it refuses. + */ +@UnstableApi +@RunWith(RobolectricTestRunner::class) +class SpaceCheckTest { + + private lateinit var app: Application + private lateinit var publisher: RecordingSpacePublisher + + @Before + fun setUp() { + app = RuntimeEnvironment.getApplication() + publisher = RecordingSpacePublisher(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 job with no declared size measures its input rather than asking for room for nothing`() { + val input = fileOfSize(INPUT_BYTES, "holiday.mp4") + + runBlocking { conversionWorker(Uri.fromFile(input), declaredSize = null).doWork() } + + assertEquals(listOf(INPUT_BYTES.toLong()), publisher.requested) + } + + @Test + fun `a declared size is trusted rather than re-measured`() { + // The ordinary path, pinned so the measurement stays a fallback. Opening a descriptor per + // job is cheap, but doing it when the picker already answered would be work for nothing -- + // and the declared number is the one the user was shown. + val input = fileOfSize(INPUT_BYTES, "holiday.mp4") + + runBlocking { conversionWorker(Uri.fromFile(input), declaredSize = DECLARED_BYTES).doWork() } + + assertEquals(listOf(DECLARED_BYTES), publisher.requested) + } + + @Test + fun `a join with no declared total measures its inputs rather than asking for room for nothing`() { + val first = fileOfSize(FIRST_JOIN_BYTES, "one.mp4") + val second = fileOfSize(SECOND_JOIN_BYTES, "two.mp4") + + runBlocking { concatWorker(listOf(Uri.fromFile(first), Uri.fromFile(second))).doWork() } + + assertEquals(listOf(FIRST_JOIN_BYTES.toLong() + SECOND_JOIN_BYTES), publisher.requested) + } + + @Test + fun `a job whose input nothing can size asks the unknown-size question instead of claiming zero`() { + // A file:// URI at a path that does not exist: the resolver answers no metadata, and + // opening a descriptor throws. Nothing left can say how big it is. + runBlocking { conversionWorker(MISSING_INPUT, declaredSize = null).doWork() } + + // The point of the whole change, in one line. `[0L]` -- what this recorded before -- is a + // claim that the file is empty; the unknown question is the absence of a claim. + assertEquals(emptyList(), publisher.requested) + assertEquals(1, publisher.unknownQuestions) + } + + @Test + fun `a join whose inputs are not all measurable has no total, rather than the ones that answered`() { + val known = fileOfSize(FIRST_JOIN_BYTES, "one.mp4") + + runBlocking { concatWorker(listOf(Uri.fromFile(known), MISSING_INPUT)).doWork() } + + // Emphatically not `[1111]`. A lower bound is indistinguishable from a total once it + // reaches the space check, and the check would then be reserving for half the job. + assertEquals(emptyList(), publisher.requested) + assertEquals(1, publisher.unknownQuestions) + } + + @Test + fun `a size nothing can determine is not by itself a reason to refuse the conversion`() { + // The decision this defect had to make, pinned so it cannot be quietly reversed. Refusing + // an unmeasurable input would turn "no provider answered the SIZE column" into "this file + // cannot be converted", which is a worse defect than the vacuous guard it replaces -- and + // one the user could do nothing at all about. + ConversionDependencies.publisher = { AlwaysRoomPublisher(app) } + ConversionDependencies.software = { WritingTranscoder } + + val result = runBlocking { + conversionWorker(MISSING_INPUT, declaredSize = null, engine = EnginePreference.FORCE_SOFTWARE).doWork() + } + + assertTrue("an unknown size must not end the job; got $result", result is ListenableWorker.Result.Success) + } + + @Test + fun `a full disk still refuses a job whose size is unknown`() { + // The other half of that decision, and what keeps `FakeFailures.FullDisk` -- which + // overrides `hasSpaceFor` and nothing else -- still meaning what it says. An independent + // implementation of the unknown-size question could stop honouring a full disk without a + // single caller changing. + ConversionDependencies.publisher = { NoRoomPublisher(app) } + + val result = runBlocking { conversionWorker(MISSING_INPUT, declaredSize = null).doWork() } + + assertEquals( + ListenableWorker.Result.failure( + workDataOf(ConversionWorker.KEY_ERROR to "Not enough free space to convert."), + ), + result, + ) + } + + /** + * A worker whose input `Data` carries a size only when [declaredSize] is given. + * + * Built entry by entry rather than through `ConversionWorker.request`, because "the key is + * simply not there" is the shape being tested and `request` is one of the two things that + * produces it. + */ + private fun conversionWorker( + input: Uri, + declaredSize: Long?, + engine: EnginePreference = EnginePreference.AUTO, + ): ConversionWorker { + val data = mutableMapOf( + ConversionWorker.KEY_INPUT_URI to input.toString(), + ConversionWorker.KEY_DISPLAY_NAME to "holiday.mp4", + 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_ENGINE_PREFERENCE to engine.name, + ) + declaredSize?.let { data[ConversionWorker.KEY_SIZE_BYTES] = it } + return TestListenableWorkerBuilder( + context = app, + inputData = workDataOf(*data.map { it.key to it.value }.toTypedArray()), + runAttemptCount = 0, + ).setId(CONVERSION_ID).build() + } + + private fun concatWorker(inputs: List): ConcatWorker = TestListenableWorkerBuilder( + context = app, + inputData = workDataOf( + ConcatWorker.KEY_INPUT_URIS to inputs.map(Uri::toString).toTypedArray(), + ConcatWorker.KEY_FORMAT to OutputFormat.MP4_H264.name, + ), + runAttemptCount = 0, + ).setId(CONCAT_ID).build() + + private fun fileOfSize(bytes: Int, name: String): File = + File(app.cacheDir, name).apply { writeBytes(ByteArray(bytes)) } + + private companion object { + const val INPUT_BYTES = 4_321 + const val DECLARED_BYTES = 9_999L + const val FIRST_JOIN_BYTES = 1_111 + const val SECOND_JOIN_BYTES = 2_222 + + /** + * An input nothing can size. + * + * A `file://` path that does not exist, which under Robolectric behaves exactly as the + * device pass recorded for a real one: `contentResolver.query` returns null, so no SIZE + * column is ever reached, and `openFileDescriptor` throws `FileNotFoundException`. It is + * also a scheme the worker handles without the FFmpegKit SAF bridge, which is native and + * therefore unavailable here. + */ + val MISSING_INPUT: Uri = Uri.parse("file:///nonexistent/holiday.mp4") + val SPEC = OutputFormat.MP4_H265.spec + val CONVERSION_ID: UUID = UUID.fromString("00000000-0000-4000-8000-000000000011") + val CONCAT_ID: UUID = UUID.fromString("00000000-0000-4000-8000-000000000012") + } +} + +/** + * Records *which* space question was asked, and refuses either way. + * + * Both overrides, and neither calls `super`. That is what makes the two questions tell apart at + * all: the production default answers `hasSpaceForUnknownSize()` by delegating to + * `hasSpaceFor(0L)`, so a recorder that delegated would log an unknown size as a request for zero + * bytes — the exact conflation being tested. The delegation itself is pinned separately, by + * [NoRoomPublisher] and the full-disk test. + * + * Refusing keeps the worker to the one line under test: the check runs before anything is staged + * or any engine is reached, so `false` ends `doWork` immediately and no native library is asked to + * load. + */ +private class RecordingSpacePublisher(context: Context) : OutputPublisher(context) { + val requested = mutableListOf() + var unknownQuestions = 0 + private set + + override fun hasSpaceFor(bytes: Long): Boolean { + requested += bytes + return false + } + + override fun hasSpaceForUnknownSize(): Boolean { + unknownQuestions++ + return false + } +} + +/** + * A full disk expressed the only way `FakeFailures.FullDisk` expresses it. + * + * `hasSpaceFor` and nothing else, so a job refused here is a job refused *through* the + * delegation rather than by an override of its own. + */ +private class NoRoomPublisher(context: Context) : OutputPublisher(context) { + override fun hasSpaceFor(bytes: Long): Boolean = false +} diff --git a/app/src/test/java/org/libremediaconverter/work/WorkerCancellationTest.kt b/app/src/test/java/org/libremediaconverter/work/WorkerCancellationTest.kt new file mode 100644 index 0000000..a04f345 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/work/WorkerCancellationTest.kt @@ -0,0 +1,169 @@ +package org.libremediaconverter.work + +import android.app.Application +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.CancellationException +import kotlinx.coroutines.runBlocking +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +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.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.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment +import java.io.File +import java.util.UUID + +/** + * That a cancelled conversion stays cancelled, and still cleans up after itself. + * + * The worker's outer catch is `catch (e: Throwable)`, which caught `CancellationException` along + * with everything else and answered it with a `Result`. That is a coroutine reporting completion + * inside a scope that has already been cancelled — structured concurrency's one rule, broken + * quietly. What made it invisible is that WorkManager marks the work `CANCELLED` itself and + * ignores the returned `Result`, so nothing on screen ever disagreed. + * + * The delete on that path is not incidental and has to survive the fix: an attempt that was + * cancelled leaves a partial file in staging, the worker starts from the top rather than resuming + * it, and this is the only code holding its handle. + */ +@UnstableApi +@RunWith(RobolectricTestRunner::class) +class WorkerCancellationTest { + + private lateinit var app: Application + private lateinit var publisher: OutputPublisher + + @Before + fun setUp() { + app = RuntimeEnvironment.getApplication() + publisher = AlwaysRoomPublisher(app) + ConversionDependencies.publisher = { publisher } + // FFprobe's loader throws a bare java.lang.Error on the JVM, and the device-codec query + // reads whatever MediaCodecList the runtime fabricates. Neither is what these tests are + // about; both would decide the routing for reasons no assertion mentions. + ConversionDependencies.probe = { _, _ -> InputProbe() } + ConversionDependencies.deviceCodecs = { DeviceCodecs.PERMISSIVE } + installTestWorkManager(app, Data.EMPTY) + } + + @After + fun tearDown() { + ConversionDependencies.reset() + } + + @Test + fun `a cancelled conversion propagates instead of being turned into a Result`() { + val worker = conversionWorker { throw CancellationException("stopped mid-transcode") } + + val thrown = runCatching { runBlocking { worker.doWork() } }.exceptionOrNull() + + assertTrue( + "cancellation must leave doWork as cancellation, not as a Result; got $thrown", + thrown is CancellationException, + ) + } + + @Test + fun `a cancelled conversion still deletes the partial it had already written`() { + val worker = conversionWorker { throw CancellationException("stopped mid-transcode") } + + runCatching { runBlocking { worker.doWork() } } + + // The engine stub writes before it throws, so a file really existed. Asserted against the + // whole directory rather than one path, so a name this test computes drifting from the + // worker's cannot turn it into a question about a file nobody wrote. + assertEquals("a cancelled attempt must not leave its partial behind", emptyList(), stagedNames()) + } + + @Test + fun `an ordinary engine failure is still answered with a Result`() { + val worker = conversionWorker { error("the muxer was never started") } + + val result = runBlocking { worker.doWork() } + + // The other half of the rule: only cancellation propagates. Widening the rethrow to every + // exception would take the user's error message away with it. + assertEquals( + ListenableWorker.Result.failure( + workDataOf(ConversionWorker.KEY_ERROR to "the muxer was never started"), + ), + result, + ) + assertEquals("a failed attempt must not leave its partial behind", emptyList(), stagedNames()) + } + + /** + * A worker routed to the software engine, which is [failure] and nothing else. + * + * `FORCE_SOFTWARE` rather than letting the router choose: it is the one preference that decides + * without consulting the input at all, so the test says which engine it is replacing instead of + * depending on a routing rule it is not about. The input is a `file://` URI for the same kind + * of reason — a `content://` one would send the worker through FFmpegKit's SAF bridge, which is + * native. + */ + private fun conversionWorker(failure: () -> Nothing): ConversionWorker { + ConversionDependencies.software = { PartialThenFailingTranscoder(failure) } + return 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_ENGINE_PREFERENCE to EnginePreference.FORCE_SOFTWARE.name, + ), + runAttemptCount = 0, + ).setId(JOB_ID).build() + } + + private fun stagedNames(): List = + publisher.createStagingFile("anything").parentFile?.listFiles().orEmpty().map { it.name }.sorted() + + 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 JOB_ID: UUID = UUID.fromString("00000000-0000-4000-8000-000000000003") + } +} + +/** + * An engine that writes something and then fails, which is what every real interruption looks like. + * + * Writing first is the point: a stub that only threw would let a missing `delete()` pass. + */ +private class PartialThenFailingTranscoder(private val failure: () -> Nothing) : SoftwareTranscoder { + override suspend fun run( + request: ConversionRequest, + inputPath: String, + output: File, + durationMs: Long, + onProgress: (Int) -> Unit, + ) { + output.writeBytes(ByteArray(PARTIAL_BYTES)) + failure() + } + + private companion object { + const val PARTIAL_BYTES = 2048 + } +} diff --git a/app/src/test/java/org/libremediaconverter/work/WorkerOutputNamingTest.kt b/app/src/test/java/org/libremediaconverter/work/WorkerOutputNamingTest.kt new file mode 100644 index 0000000..039fb70 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/work/WorkerOutputNamingTest.kt @@ -0,0 +1,118 @@ +package org.libremediaconverter.work + +import android.app.Application +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.StagingNames +import org.libremediaconverter.convert.installTestWorkManager +import org.libremediaconverter.model.ConversionRequest +import org.libremediaconverter.model.ConversionRouter +import org.libremediaconverter.model.DeviceCodecs +import org.libremediaconverter.model.EnginePreference +import org.libremediaconverter.model.InputProbe +import org.libremediaconverter.model.OutputFormat +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment +import java.util.UUID + +/** + * That a finished job reports the name and type it actually produced. + * + * Only the worker knows both. The spec travels to it as input `Data`, and `WorkInfo` hands input + * `Data` back to nobody — so a ViewModel picking a result up after a restart has no route to the + * spec at all, and one watching a job it started had only its own picker, which is free to move + * while the job runs. Two derived strings in the output `Data` close both. + */ +@UnstableApi +@RunWith(RobolectricTestRunner::class) +class WorkerOutputNamingTest { + + private lateinit var app: Application + private lateinit var publisher: OutputPublisher + + @Before + fun setUp() { + app = RuntimeEnvironment.getApplication() + publisher = AlwaysRoomPublisher(app) + ConversionDependencies.publisher = { publisher } + ConversionDependencies.probe = { _, _ -> InputProbe() } + ConversionDependencies.deviceCodecs = { DeviceCodecs.PERMISSIVE } + ConversionDependencies.software = { WritingTranscoder } + installTestWorkManager(app, Data.EMPTY) + } + + @After + fun tearDown() { + ConversionDependencies.reset() + } + + @Test + fun `a conversion reports the name and type its own spec produced`() { + // MP3, which is nothing like the default preset a fresh picker offers, so a suggestion + // built from the picker rather than from here is visibly wrong instead of accidentally + // right. + val staged = publisher.createStagingFile(StagingNames.forJob(JOB_ID, SPEC.extension)) + val decision = ConversionRouter.route( + ConversionRequest(SPEC, enginePreference = EnginePreference.FORCE_SOFTWARE), + DeviceCodecs.PERMISSIVE, + ) + + val result = runBlocking { conversionWorker().doWork() } + + assertEquals( + ListenableWorker.Result.success( + workDataOf( + ConversionWorker.KEY_OUTPUT_PATH to staged.absolutePath, + ConversionWorker.KEY_ENGINE_USED to decision.engine.name, + ConversionWorker.KEY_ROUTE_REASON to decision.reason.explanation, + ConversionWorker.KEY_SUGGESTED_NAME to "holiday_converted.mp3", + ConversionWorker.KEY_MIME_TYPE to "audio/mpeg", + ), + ), + result, + ) + } + + @Test + fun `a join names itself after the format it was asked for`() { + // The join screen has no format picker, so `joined.mp4` was right by accident. Ask for + // anything else and every hardcoded MP4 becomes wrong at once. + assertEquals("joined.mp4", ConcatWorker.outputNameFor(OutputFormat.MP4_H264)) + assertEquals("joined.mkv", ConcatWorker.outputNameFor(OutputFormat.MKV_H264)) + assertEquals("joined.webm", ConcatWorker.outputNameFor(OutputFormat.WEBM_VP9)) + } + + private fun 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_ENGINE_PREFERENCE to EnginePreference.FORCE_SOFTWARE.name, + ), + runAttemptCount = 0, + ).setId(JOB_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.MP3.spec + val JOB_ID: UUID = UUID.fromString("00000000-0000-4000-8000-00000000000d") + } +} diff --git a/app/src/test/java/org/libremediaconverter/work/WorkerStubs.kt b/app/src/test/java/org/libremediaconverter/work/WorkerStubs.kt new file mode 100644 index 0000000..6a16964 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/work/WorkerStubs.kt @@ -0,0 +1,45 @@ +package org.libremediaconverter.work + +import android.content.Context +import org.libremediaconverter.convert.OutputPublisher +import org.libremediaconverter.convert.SoftwareTranscoder +import org.libremediaconverter.model.ConversionRequest +import java.io.File + +/** + * Scaffolding more than one worker test needs. + * + * Only that. The stubs a test uses to force *its own* failure stay in that test, next to the + * assertion they serve. + */ + +/** + * A real [OutputPublisher] that never refuses on space. + * + * The space check reads the host's free disk, which has nothing to do with what any of these tests + * are about and would make them pass or fail on how full the machine is. Where staging lives, and + * the delete, stay the production implementation — the assertions are about the real filesystem. + */ +open class AlwaysRoomPublisher(context: Context) : OutputPublisher(context) { + override fun hasSpaceFor(bytes: Long): Boolean = true +} + +/** + * An engine that writes the output file and nothing else. + * + * Enough for the tests that ask *where* a conversion put its result and *what it called it*; what + * the bytes are is never the question there. + */ +object WritingTranscoder : SoftwareTranscoder { + override suspend fun run( + request: ConversionRequest, + inputPath: String, + output: File, + durationMs: Long, + onProgress: (Int) -> Unit, + ) { + output.writeBytes(ByteArray(OUTPUT_BYTES)) + } + + private const val OUTPUT_BYTES = 512 +} diff --git a/app/src/test/resources/robolectric.properties b/app/src/test/resources/robolectric.properties new file mode 100644 index 0000000..131b785 --- /dev/null +++ b/app/src/test/resources/robolectric.properties @@ -0,0 +1,12 @@ +# Robolectric defaults to the manifest's targetSdk, which is 37 here, and there is no +# android-all jar for 37 -- Robolectric 4.16.1 stops at 36 and fails the whole class with +# "Package targetSdkVersion=37 > maxSdkVersion=36" before any test body runs. +# +# 36 is where CI's emulator matrix already stops, for the unrelated reason in +# docs/api-37-emulator-crash.md, so this does not widen the gap between what is verified +# automatically and what is not: API 37 was already a manual check on the Pixel 10 Pro XL +# before each release, and still is. +# +# Set here rather than in a @Config on each class so a later Robolectric test does not have +# to rediscover it. Remove it once Robolectric ships an android-all jar for 37. +sdk=36 diff --git a/config/detekt/detekt.yml b/config/detekt/detekt.yml index c4d98f7..c08914f 100644 --- a/config/detekt/detekt.yml +++ b/config/detekt/detekt.yml @@ -102,3 +102,14 @@ exceptions: # and handles -- falls back to FFmpeg, or fails the job with a reason -- and the # SwallowedException rule stays active to keep it that way. active: false + TooGenericExceptionThrown: + # Still on for main source, which throws nothing generic and should not start. + # + # Relaxed for tests only, and for one reason: a test that reproduces a failed native + # load has to throw what the library actually throws, and FFmpegKit throws a *bare* + # `java.lang.Error` -- `NativeLoader` catches the UnsatisfiedLinkError from + # System.loadLibrary and rethrows `Error(message, cause)`. That is not incidental, it + # is the whole finding `ffmpeg/NativeLoadFailure.kt` exists to handle, and the reason + # the obvious narrower guards catch nothing. Substituting a tidier subclass here would + # leave the test passing against a defect it no longer reproduces. + excludes: ['**/test/**', '**/androidTest/**'] diff --git a/docs/defect-audit.md b/docs/defect-audit.md new file mode 100644 index 0000000..7ef28b2 --- /dev/null +++ b/docs/defect-audit.md @@ -0,0 +1,850 @@ +# Defect audit + +**Status:** ten fixed and merged, two in progress, one parked. Fix status is per entry in the +summary table; the entry bodies below describe each defect *as found* and are deliberately not +rewritten as fixes land — this is the record of what was wrong, not a changelog. +**Scope:** the Android-framework edge of the app, which has no JVM unit tests. +**Last verified:** 2026-08-22, against `main` at `903b43c`. +**Device pass:** 2026-08-22 on a physical Pixel 10 Pro XL, API 37. Four entries were driven on +hardware; **D1 did not reproduce and its premise is contradicted** — see its entry. Verdicts are +marked per entry. Everything unmarked is still inspection only. + +Instrumented baseline taken at the same time: `connectedDebugAndroidTest` on the Pixel gave +**49 tests, 0 failures, 0 errors, 2 skipped**, no regression against the 40/0/2 recorded in +`api-37-emulator-crash.md`. The 2 skips are the assumption-guarded `RealMediaBenchmark` tests. + +This is a survey, not a work order. Each entry records what is wrong, how confident we are that +it is wrong, how to provoke it, and what a fix would have to decide. Acting on any of them is a +separate decision, and each would be its own commit. + +## Why this document exists, and why it is not about detekt + +The obvious place to look for defects is the static-analysis output. There is nothing there: + +| Gate | Result on `903b43c` | +|---|---| +| `./gradlew :app:detekt` | **0 findings** across 41 files | +| `./gradlew :app:ktlintCheck` | clean | +| `./gradlew :app:lintDebug` | `0 errors, 0 warnings, 1 hint` | + +There is also no `detekt-baseline.xml`, no `lint-baseline.xml`, and not one `@Suppress`, +`//noinspection` or `tools:ignore` anywhere in the repository. The entire suppression surface is +`config/detekt/detekt.yml` and the `lint {}` block in `app/build.gradle.kts`, each entry carrying +its reason in prose. **A detekt baseline would be an empty file**, so none is proposed. + +Running detekt with `allRules` enabled produces 467 findings, which is misleading rather than +informative: + +| Count | Rule | Verdict | +|---|---|---| +| 177 | `UndocumentedPublicProperty` | KDoc on every public property | +| 126 | `FunctionNameMaxLength` | backtick test names in `src/test` | +| 45 | `UndocumentedPublicFunction` | as above | +| 41 | `UndocumentedPublicClass` | as above | +| 33 | `DocumentationOver*`, `LabeledExpression`, `ClassOrdering`, `UseIfInsteadOfWhen`, … | style opinions | +| 2 | `OutdatedDocumentation` | **incorrect** — see D12 | + +**Zero are in the `potential-bugs` ruleset.** Enabling `allRules` would mean writing KDoc for 177 +public properties, which is the opposite of what `config/detekt/detekt.yml` says its own purpose +is: *"Genuine smells … are fixed in the code, not silenced."* + +So the linters are clean and honest, and the defects are elsewhere — in the code they cannot see +into. `OutputPublisher`, both ViewModels, both Workers and `MainActivity` have **no JVM unit tests +at all**: roughly 1,200 of ~4,000 lines of main source, and the direct explanation for the ~31% +coverage figure recorded in `CLAUDE.md`. Every entry below is in that untested set. + +## How to read the confidence labels + +`api-37-emulator-crash.md` separates what was reproduced from what was ruled out. This does the +same, because an inventory that asserts a bug it cannot demonstrate is worse than a shorter one. + +- **Confirmed by inspection** — the control flow is fully readable and the defect follows from it. +- **Needs device confirmation** — the reasoning is sound, but the behaviour depends on framework + runtime semantics. Per `CLAUDE.md`, that means CI or the Pixel 10 Pro XL, never a local + emulator. Each such entry states its *forcing condition* so the check is a task, not a hunch. +- **Latent** — not reachable through today's UI, but wrong, and one change away from being live. + +Nothing below was observed on a device. The "confirmed" entries are confirmed as *code*; the +"needs device confirmation" entries are not confirmed at all yet. + +--- + +## D1 — `hasSpaceFor` measures the wrong quantity + +**Severity: medium · NOT REPRODUCED on the Pixel — the stated premise is contradicted** + +*Originally filed as "under-reports free space". The device pass falsified that direction; the +title and reasoning are corrected here rather than quietly dropped.* + +`app/src/main/java/org/libremediaconverter/convert/OutputPublisher.kt:35` + +```kotlin +open fun hasSpaceFor(bytes: Long): Boolean = stagingDir.usableSpace > bytes + SPACE_HEADROOM_BYTES +``` + +`File.usableSpace` gets two things wrong: it ignores cache the system would reclaim on request, +and it counts the low-storage reserve — space the framework will not let the app have — as +available. The platform documents the first direction in `StorageManager` itself: +`getAllocatableBytes` *"is typically larger than `File.getUsableSpace()`, since the system may be +willing to delete cached files to satisfy an allocation request."* + +**On the measured device the second effect dominates and the first is absent entirely**, which is +why this entry now reads "wrong quantity" rather than "under-reports". It is the one entry here +that a device disproved. + +This is the one defect that was already known. It is recorded in three places — the `lint {}` +block at `app/build.gradle.kts:94-102`, the body of commit `65a94b4`, and the PR #5 description — +all saying the fix "deserves its own commit and its own test". It is held visible rather than +hidden by `informational += "UsableSpace"`, which is the single hint in the lint report. + +**`OutputPublisher`'s own KDoc (lines 29-34) does not mention it.** That is the one place a +reader of the code would not learn about it. + +### Reproduction: **NOT REPRODUCED — the measured relationship is inverted** + +This entry predicted `getAllocatableBytes` would exceed `usableSpace`. On the Pixel 10 Pro XL at +66% free, it does not. Measured on the app's own staging volume: + +``` +usableSpace = 655141146624 (624791 MB) <- what hasSpaceFor() reads +allocatableBytes = 654616858624 (624291 MB) <- StorageManager +allocatable - usable = -524288000 (-500 MiB) +``` + +A control experiment settles why: writing 3 GB into the app's own cache dropped **both** numbers +by the identical 3,255,443,456 bytes and left the delta at exactly `-524288000`. **None of the +app cache counted as reclaimable** — `cacheClearable` is zero here. `dumpsys diskstats` reported +`Data-Free: 639775620K / 959840256K total = 66% free` with `App Cache Size: 14623028224`, so +13.6 GB of device-wide app cache yielded zero reclaimable bytes. The −500 MiB is the low-storage +reserve, and nothing offsets it. + +**What this does and does not settle.** It does not show the app is measuring the right thing — +counting the low-storage reserve as usable is still wrong, just wrong in the *other* direction. +What it kills is the stated justification: on this device, at this fill level, the app is not +refusing conversions it had room for, and switching to `getAllocatableBytes` would refuse +**more** jobs, by 500 MiB. + +The under-report direction requires reclaimable cache to be non-zero, which needs real storage +pressure — plausibly the regime where the guard actually fires, but **unverified**. Reaching it +would mean filling the device far past 13.6 GB of cache, which was not done. Until someone +measures near-full, this entry's original premise stands unproven, and any fix should be +justified as "measure the right quantity" rather than "stop refusing jobs we had room for". + +### What a fix has to decide + +- **It is not a pure loosening.** `getAllocatableBytes` also excludes the framework's low-storage + reserve, so on a nearly-full device it can return *less* than `usableSpace`. In that regime the + fix makes the app refuse **more** jobs. That is correct — staging lives in `cacheDir`, the first + thing the system reclaims, so writing into the reserve invites the staged output to be deleted + mid-job — but it will read as a regression unless the commit message says so. +- **Report, do not allocate.** `allocateBytes` can clear the app's *own* cache to satisfy a + reservation, so a pre-flight check for one job could destroy another job's unsaved staged + output. A check that deletes results is worse than the bug it fixes. +- **The `IOException` policy is a real choice, not a detail.** `getAllocatableBytes` throws when + the volume "isn't present, or doesn't support allocating space". Failing open (allow the job, + let it fail later with a real `ENOSPC`) and failing closed (refuse) are both defensible; the + decision belongs in a test, not in a `catch` block. +- **The code being replaced is executed by zero tests.** The only coverage, + `FakeFailures.FullDisk` (`app/src/androidTest/java/org/libremediaconverter/fallback/FakeFailures.kt:70-72`), + *overrides* `hasSpaceFor` rather than exercising it. Swapping a call that cannot throw for one + that can, with nothing testing the real body, is the main risk here. An instrumented test that + calls the real `hasSpaceFor` should land with the fix. + +### Test to write first + +A pure seam taking the measured space as a **nullable** `Long`, so "the platform could not measure +this volume" is a value a JUnit test can pass in: + +1. room when available exceeds required + headroom +2. refused when exactly equal — pins the boundary +3. refused when below +4. the unmeasurable case resolves to the chosen policy — *this is the test that documents the + decision* +5. an absurd reported input size cannot overflow into a wrong "yes" + +### Registry action on fix + +Delete `informational += "UsableSpace"` and its nine-line comment from `app/build.gradle.kts`. +`./gradlew :app:lintDebug` should then pass with no hint — locally verifiable, which is rare here. + +--- + +## D2 — The app never cleans up its own staging files + +**Severity: medium · Confirmed by inspection · REPRODUCED on the Pixel** + +Every conversion writes a full-size output into `/conversions/`. `save()` publishes it +to the user's destination and deletes it. But **`reset()` in both ViewModels drops the `File` +reference without deleting it**: + +- `app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt:237` +- `app/src/main/java/org/libremediaconverter/join/JoinViewModel.kt:122` + +and `reset()` is what the **"Start over" button** on the `Converted` and `Joined` states calls +(`ConverterScreen.kt:195`, `JoinScreen.kt:143`). Converting a file and deciding not to save it is +an ordinary, first-class path through the UI, and it leaves a full-size copy behind every time. + +`OutputPublisher.clearStaging()` (`OutputPublisher.kt:44`) exists to clean exactly this up and is +**called from nowhere**. `grep -rn --include='*.kt' 'clearStaging' app/src` returns one line: its +own declaration. Not even the instrumented tests reference it. It is dead code that documents the +author's own intent. + +### Severity, stated honestly + +This is **not** unbounded growth. `cacheDir` is OS-evictable under storage pressure, so the +platform reclaims it eventually. The defect is that the app relies on the OS to clean up after it: +until eviction the user sees inflated app storage for files that serve no purpose, and on a device +that is not under pressure they can sit there indefinitely. + +### Reproduction + +``` +convert a file → "Start over" +adb shell run-as org.libremediaconverter ls -l cache/conversions/ +``` + +The output file is still there. Repeat — one copy per conversion. + +**Done on the Pixel**, driven through the real `ConversionViewModel` (pick → `convert()` → +`Converted` → `reset()`, which is what "Start over" calls). Staging was empty beforehand: + +``` +terminal state = Converted(..., staged=.../cache/conversions/input_converted.mp4, engineUsed=MEDIA3) +state after reset() = Idle +AFTER_RESET staged = .../input_converted.mp4 exists=true length=456190 +``` + +and from the shell afterwards: +`-rw------- 1 u0_a540 u0_a540_cache 456190 input_converted.mp4` + +### Other paths to the same leak + +- **A failed save, and this one is worse than it looks.** `save()`'s `onFailure` sets + `Failed(message)` — a state that carries no `staged` reference at all. So even a `reset()` that + consulted the state could not find the file. Any fix needs the cleanup handle to be a ViewModel + field, not something read back out of the state machine. +- **Process death mid-job** — see D3. + +### `clearStaging()` cannot simply be wired up + +It deletes everything in the directory unconditionally. That would include a live +`concat_list.txt` mid-join, or the other tab's in-flight output (D8). Whatever closes this defect +has to be narrower than the dead method is. + +### Note the direction of the interaction with D1 + +These two do **not** compound, and it is worth being precise because the opposite is the intuitive +reading. `getAllocatableBytes` counts reclaimable cache, and the app's own `cacheDir` is +reclaimable — so once D1 is fixed, leaked staging files read as *available* space. **D1's fix +masks D2's effect on the space check rather than worsening it.** That is an argument for fixing +D2 first or alongside, not for treating them as one defect. + +### Tests to write first + +Two seams, because one cannot express both halves: + +- a pure "which of these entries is collectable" rule over `(name, lastModifiedMs)` pairs and a + clock — the interesting part is the boundary condition and a clock that moved backwards, and + passing timestamps in directly keeps that exact rather than filesystem-dependent; +- a Robolectric test driving a real `OutputPublisher` against a temp `cacheDir`, asserting the + file is actually **gone** after the reset path runs. No pure function can express that. + +--- + +## D3 — A conversion that outlives the process becomes unreachable + +**Severity: high · REPRODUCED on the Pixel** + +This is the strongest claim in this document. + +`ConversionViewModel.activeWorkId` and `observer` are plain fields +(`ConversionViewModel.kt:87-88`); nothing is persisted to a `SavedStateHandle`. WorkManager jobs +deliberately survive process death — that is the stated reason for choosing it +(`ConversionWorker.kt:33`: *"the queue survives process death"*). + +So whenever the process is reclaimed, the next launch starts at `ConversionState.Idle` while the +output sits in cache, and **the user has no way to reach or save it**. It then stays there per D2. +The app is architected specifically to protect long jobs from process death, and the ViewModel is +where that protection stops. + +Two windows, and the second is much wider than the first: + +- **During the transcode** — the worker holds a foreground service, so the process is high + priority and reclaim is unlikely, though not impossible under real memory pressure. +- **After it finishes, before the user saves.** The foreground service is gone, the process is an + ordinary background one, and the result is sitting in cache waiting for a tap that may come + hours later or never. This is the realistic case and the one to test. + +### Reproduction (forcing condition) + +The defect does not need the process to die *mid-job*. It needs the ViewModel to be gone while +the result exists — which is also the realistic case: the job finishes, the user never comes back, +and the process is reclaimed hours later. + +``` +convert a file and let it finish, so the screen reaches "Done" +do NOT tap Save; background the app +adb shell am kill org.libremediaconverter +``` + +Relaunch: the screen is Idle, and the finished output is still in `cache/conversions/`, with no +route to it from the UI. + +**Done on the Pixel.** Process `23087` killed after backgrounding, relaunched as `23252`; the +456190-byte output survived. In a fresh process: + +``` +FRESH_PROCESS input_converted.mp4 456190 bytes +WorkInfos by tag org.libremediaconverter.work.ConversionWorker: 2 + id=4b488279-... state=SUCCEEDED output=.../cache/conversions/input_converted.mp4 + id=767e021a-... state=SUCCEEDED output=.../cache/conversions/input_converted.mp4 +fresh ConversionViewModel state = Idle +``` + +`ConverterScreen` renders `viewModel.state.collectAsStateWithLifecycle()` directly, so a ViewModel +at Idle is the screen at Idle. + +**Two things this adds to the fix direction.** The tag query works as predicted — but it returned +**two SUCCEEDED infos carrying the same `output=` path** while only one file exists on disk. That +is D8's collision surfacing inside D3's own fix: "reattach to the unfinished work" is not +well-defined on real device state, and checking that the staged file exists does not +disambiguate. Finished work is also not pruned promptly, so any reattachment will routinely see +completed jobs from earlier sessions. + +**Correction to the `am kill` caveat above:** the refusal observed was **adj-dependent**, and the +foreground-service claim was not isolated. `am kill` no-opped against the top-activity process +(pid unchanged) and succeeded once the app was backgrounded to `oom: cur=700, state=LAST`. +Producing a plain app process holding a conversion foreground service needed the UI, and the +device was secure-locked. + +Two traps in getting this to fire: + +- **`am force-stop` will not show it** — it cancels the work outright. +- **`am kill` only kills processes the system considers safe**, and it refuses one holding a + foreground service. Both workers call `setForeground()`, so a kill attempted *while the + conversion runs* silently does nothing. Wait until the job is finished (the foreground service + is gone by then) or use `adb shell am crash org.libremediaconverter` instead. + +### Fix direction + +Reattach on init by querying WorkManager for the app's own unfinished work. **No production +change is needed to enable this** — WorkManager already tags every request with its worker class +name, so `getWorkInfosByTag(ConversionWorker::class.java.name)` works against the code as it +stands. + +--- + +## D4 — `publish()` can leave a truncated file at the user's destination + +**Severity: medium · Needs device confirmation** + +`OutputPublisher.publish()` (`OutputPublisher.kt:38-42`) streams the staged file into the SAF +destination with `copyTo`: + +```kotlin +open fun publish(staged: File, destination: Uri) { + context.contentResolver.openOutputStream(destination)?.use { out -> + staged.inputStream().use { it.copyTo(out) } + } ?: error("Could not open destination for writing: $destination") +} +``` + +If `copyTo` throws partway — destination volume full, provider error — the partial file remains at +the user's chosen location, under the name they picked, while the UI reports "Could not save the +file". The user is left holding a broken file they were told was not written. + +Whether the document survives depends on the provider, which is why this is not marked confirmed. + +### Reproduction (forcing condition) + +Fill the destination volume so it holds less than the staged output, then Save. Inspect the +destination: a same-named, short file is present. `UnopenableUriTest.kt:91-92` already covers the +*unopenable* destination case; this is the *fails-midway* case, which nothing covers. + +### Fix direction + +Delete the destination document on failure via `DocumentsContract.deleteDocument`, or +write-then-rename where the provider supports it. + +--- + +## D5 — The space check can be effectively vacuous + +**Severity: low-medium · CONFIRMED live on the Pixel** + +`hasSpaceFor` is given the **input** size as a proxy for the output size, and that size comes from +`OpenableColumns.SIZE` in `queryFile()` (`ConversionViewModel.kt:265-281`, +`JoinViewModel.kt:129-143`): + +```kotlin +var size = 0L // ConversionViewModel.kt:267 +… +cursor.getColumnIndex(OpenableColumns.SIZE) + .takeIf { it >= 0 } + ?.let { size = cursor.getLong(it) } +``` + +A provider that does not report `SIZE` leaves `size` at `0L`, and `hasSpaceFor(0)` degrades the +guard to "is there 128 MB free". The `0L` default is explicit and confirmed; **which real +providers omit the column is not confirmed**, and this document will not guess. + +Separately, `hasSpaceFor`'s KDoc claims peak usage is "roughly input + output at once" while the +check only reserves `input + 128 MB`. + +### Reproduction + +**Observed on the Pixel as a reachable value, not an inference.** `contentResolver.query` on a +`file://` URI returns null, so `queryFile` never reaches the `SIZE` column and leaves the default +in place: `queryFile gave displayName='input' sizeBytes=0`. `hasSpaceFor(0)` then degrades the +guard to "is there 128 MB free", exactly as predicted. + +Which *document-provider* URIs omit `SIZE` is still unconfirmed — the `file://` path is enough to +show the `0L` default is live, not enough to say how often a real pick hits it. + +### Note + +This shares a seam with D1. Both should be decided together, not in separate passes. + +--- + +## D6 — Rotating the device throws the user back to the Convert tab + +**Severity: medium · Confirmed by inspection** + +`MainActivity.kt:65`: + +```kotlin +var destination by remember { mutableStateOf(Destination.CONVERT) } +``` + +`remember` survives recomposition but not activity recreation, and `MainActivity` declares no +`configChanges`. Any rotation or resize resets the selected tab. + +The KDoc immediately above that line argues the adaptive shell "is not cosmetic", because from +targetSdk 37 *"the app will be resized and rotated whether or not it is ready"* — which is exactly +the case that loses the state. + +### Reproduction + +Open the app, switch to the Join tab, rotate the device. It returns to the Convert tab. The +ViewModel state survives (both ViewModels are Activity-scoped); only the tab selection is lost, +which is what makes it visibly wrong rather than merely stale. + +### Fix direction and test + +`rememberSaveable`. A Compose `StateRestorationTester` test covers it, and +`compose-ui-test-junit4` is **already on the androidTest classpath with zero current users** — no +new dependency needed. It is an instrumented test, so it runs on CI, not locally. + +--- + +## D7 — Direct `notify()` on WorkManager's foreground notification ID + +**Severity: low-medium · Resurrection REPRODUCED; undismissability NOT verified** + +`ConversionWorker.publishProgress()` (`ConversionWorker.kt:167-169`) calls the notification manager +directly, on the same ID WorkManager owns through `setForeground`: + +```kotlin +applicationContext.getSystemService(android.app.NotificationManager::class.java) + .notify(NOTIFICATION_ID, notifications.build(id, displayName, percent)) +``` + +A progress update landing after the worker is stopped can resurrect a notification built with +`setOngoing(true)` (`ConversionNotifications.kt:41`). + +**REPRODUCED on the Pixel**, on attempt 3 of 12, cancelling a `BEST`-tier job: + +``` +attempt 3: terminal state = CANCELLED +attempt 3: +300ms active=0 id1001=false ongoing=null <- WorkManager tore it down +attempt 3: +700ms active=1 id1001=true ongoing=true <- resurrected +attempt 3: +5000ms active=1 id1001=true ongoing=true +``` + +Still live ~10 minutes later with no app process at all. The record shows +`flags=ONGOING_EVENT|ONLY_ALERT_ONCE` and **no `FOREGROUND_SERVICE` flag** — which is what proves +`publishProgress()`'s direct `notify(1001, …)` posted it rather than `setForeground`. + +**An earlier draft of this entry claimed the user "cannot dismiss" it. That is unproven and may +be wrong.** `isClearable()` returned false, but only because `FLAG_ONGOING_EVENT` is set: the +record carries no `FOREGROUND_SERVICE` and no `NO_CLEAR`, and API 34+ lets users swipe away +ongoing notifications that are not foreground-service-backed. A SystemUI swipe test was +impossible behind the secure lock screen, so the severity of the orphan is still open. + +### Reproduction (forcing condition) + +Cancel a conversion at the moment a progress tick fires — the throttle is ~1/sec +(`NOTIFICATION_INTERVAL_MS`), so repeat cancels mid-conversion. Then check for an ongoing +notification with no running job: `adb shell dumpsys notification`. + +--- + +## D8 — Nothing gives a job a staging path of its own + +**Severity: medium, in a narrow window · Naming OBSERVED live; concurrency still unconfirmed** + +Three paths into `/conversions/` can be shared by two jobs at once: + +| Site | Name | Collides when | +|---|---|---| +| `ConcatWorker.kt:57` | `"joined.${format.extension}"` — a **constant** | any two joins of the same format | +| `ConversionWorker.kt:90` | derived from the input display name via `outputNameFor` | two inputs share a display name (two `holiday.mp4` from different folders) | +| `ConcatEngine.kt:46` | `"concat_list.txt"` — a **constant**, `finally`-deleted | any two joins at all | + +**As plain overwrite this is not data loss.** By the time a second job can start, the first result +is already unreachable: the only route from `Converted`/`Joined` back to a startable state is +"Start over", which clears it. + +**The live consequence is corruption.** Neither ViewModel uses a unique-work policy — both call +plain `workManager.enqueue(request)` (`ConversionViewModel.kt:159`, `JoinViewModel.kt:65`) — so +after a process restart WorkManager can resume an earlier job while the user starts a new one, and +two FFmpeg processes write the same path concurrently. For `concat_list.txt` that means one join +reading the other join's input list. + +### Reproduction + +**The naming half needs no device**, and was then seen live anyway. Two independent jobs on the +Pixel — the D2 run and the D3 run — each computed +`cache/conversions/input_converted.mp4`, and the second silently overwrote the first. The D3 probe +also found **two SUCCEEDED `WorkInfo`s carrying that same path** with only one file on disk, which +is the collision reaching the point where it makes a *fix* ambiguous, not just a file. + +**The concurrency half is harder to provoke than it looks**, and is the reason this entry is not +marked confirmed. It requires a join that is still non-terminal when the process dies, so that +WorkManager resumes it alongside a newly started one — and the same `am kill` caveat as D3 +applies, since a running worker holds a foreground service. A job that already returned +`Result.success` will not resume at all. Anyone checking this should say which of the two they +relied on; if it cannot be provoked, the entry should be reduced to the naming half alone, which +stands on inspection. + +### Note + +This is also why D2's `clearStaging()` is hazardous: a directory-wide delete would take out a live +list file mid-join. + +--- + +## D9 — Output names are derived from the wrong source + +**Severity: low · Latent, two instances** + +`JoinViewModel.kt:115` reports a hardcoded name on success: + +```kotlin +_state.value = JoinState.Saved("joined.mp4") +``` + +regardless of format. `ConcatWorker.request` already takes a `format` parameter defaulting to +`MP4_H264`, and `JoinViewModel.join()` never passes one, so the string is *accidentally* correct. +`JoinScreen.kt:44` (`CreateDocument("video/mp4")`) and `:139` (`launch("joined.mp4")`) hardcode the +same assumption. All three agree only because the Join screen has no format picker. + +The same shape is in the convert path: `ConversionViewModel.save()` (`:226`) and +`suggestedOutputName()` (`:244`) build the name from **`_settings.value.spec` — the current picker +state, not the spec the job actually ran with**. Today the pickers render only in the `Ready` +state, so settings cannot change between enqueue and save, and the name is right by accident too. + +Both become wrong the moment a format picker reaches the Join screen, or the pickers stay live +during a conversion. Recorded as one pattern rather than two footnotes, because it is one mistake +made twice. + +### Test to write first + +Pure name-derivation seams keyed on the job's own spec/format, mirroring the existing +`ConversionWorker.outputNameFor` and its assertions. + +--- + +## D10 — `CancellationException` is caught and converted to a `Result` + +**Severity: low · Needs device confirmation before being called a defect at all** + +`ConversionWorker.doWork()`'s outer `catch (e: Throwable)` (`ConversionWorker.kt:104`) catches +`CancellationException` — `runMedia3OrFallBack` deliberately rethrows it via `isCancellation` — +and routes it into `handleTimeoutIfNeeded`, returning `Result.failure`/`Result.retry` rather than +letting it propagate. Swallowing cancellation inside a coroutine breaks structured concurrency. + +**The user-visible impact today is probably nil.** WorkManager marks work `CANCELLED` itself and +ignores the returned `Result`. The `staged.delete()` on that path is desirable and any fix must +preserve it. + +### Reproduction (forcing condition) + +Cancel a running conversion and read the resulting `WorkInfo`. If its state is `CANCELLED` and no +error surfaces to the UI, this is confirmed harmless and should be **downgraded to a note** rather +than carried as a defect. + +--- + + +## D11 — Documentation and scaffold defects + +**Severity: low · Confirmed by inspection** + +| Item | Location | Note | +|---|---|---| +| Stale JDK claim | `README.md:111` | "Requires JDK 17+ (AGP 9 will not run on older) and the Android SDK with API 37." contradicts the Java 25 toolchain that `CLAUDE.md` documents. The same claim was already corrected once, in `CLAUDE.md`, by commit `f496291`. | +| Known bug not recorded at the code | `OutputPublisher.kt:29-34` | `hasSpaceFor`'s KDoc does not mention D1, though three other places record it. | +| Stale package directory | `app/src/main/java/com/example/androidmediaconverter/` | Empty; residue from the project template's old package name. | +| Template TODO | `app/src/main/res/xml/data_extraction_rules.xml:8` | Untouched Android Studio boilerplate, and the only literal `TODO` in the repository. | + +--- + +## D12 — Two detekt `allRules` findings warrant no code change + +**Recorded so that nobody "fixes" correct code** + +With `allRules` enabled, detekt 2.0.0-alpha.6 reports `OutdatedDocumentation` twice: + +| Finding | Claim | Reality | +|---|---|---| +| `ContainerCapabilities.kt:338` | documented parameter `suggestions` "is not present in the declaration" | `data class Invalid(val message: String, val suggestions: List)` — it is present | +| `OutputFormat.kt:18` | documented parameter `ffmpegFormat` "is not present in the declaration" | `enum class Container(val label: String, val ffmpegFormat: String, …)` — it is present | + +The KDoc is correct in both cases, so no code change is warranted. Why detekt emits them is not +investigated here and this document will not speculate. + +This matters beyond the two lines: it is the concrete evidence for leaving `allRules` off. A rule +that reports correct code as wrong would cost more in re-litigation than the 465 style findings +next to it. + +--- + +## D13 — Work interrupted by process death fails terminally instead of resuming + +**Severity: high · CONFIRMED ON NATURAL DISPATCH, Pixel 10 Pro XL, API 37** + +*This is the most serious entry in this document. It falsifies the premise the app's whole +background architecture rests on.* + +Found during the Pixel pass, not present in the original inventory. + +`ConversionWorker.setForeground(...)` is at line 66; the `return try {` is at line 92. **Any throw +from `setForeground` escapes `doWork()`** without reaching `handleTimeoutIfNeeded`, +`Result.retry()`, or `staged.delete()` — so no `KEY_ERROR` reaches the UI, no retry is attempted, +and a partial staged file is left behind. `ConcatWorker` has the same shape (`setForeground` at 49, +`try` at 58). That much is plain from the source. + +What makes it potentially serious is what the device did with it. When WorkManager tried to +restart a `ConversionWorker` with the app in the background: + +``` +WM-WorkerWrapper: Starting work for org.libremediaconverter.work.ConversionWorker +ActivityManager: Background started FGS: Disallowed [callingPackage: org.libremediaconverter; + targetSdkVersion:37; callerTargetSdkVersion:37] +WM-WorkerWrapper: android.app.ForegroundServiceStartNotAllowedException: startForegroundService() + not allowed: service org.libremediaconverter/androidx.work.impl.foreground.SystemForegroundService +WM-WorkerWrapper: Worker result FAILURE +``` + +**FAILURE, not retry.** If that holds, it contradicts `ConversionWorker.kt:33` — *"the queue +survives process death"* — which is the app's stated reason for choosing WorkManager at all, and +the premise D3's fix rests on. + +**The forcing caveat is retired.** A follow-up run reproduced this on a genuinely natural +dispatch, with no `cmd jobscheduler run` issued at any point (verified against the device's own +adbd command census). An 18-minute transcode was killed mid-write with `kill -9`; **119 seconds +later, unprompted**, WorkManager recovered it and the system denied it: + +``` +18:52:42.023 WM-ForceStopRunnable: Found unfinished work, scheduling it. +18:52:42.409 ActivityManager: Background started FGS: Disallowed [callingPackage: + org.libremediaconverter; uidState: CEM; BFGS denied: true; code:DENIED; + tempAllowListReason:; targetSdkVersion:37; callerTargetSdkVersion:37] +18:52:42.425 WM-WorkerWrapper: android.app.ForegroundServiceStartNotAllowedException +18:52:42.429 WM-WorkerWrapper: Worker result FAILURE [tags={ ...ConversionWorker }] +18:52:42.467 WM-Processor: Processor 3d1c9862 executed; reschedule = false +``` + +`Found unfinished work, scheduling it` is the clean process-death recovery path — **not** +"Application was force-stopped". The system logged its own `am_wtf` for the denial. A second +dispatch, via the force-stop recovery path and with the phone in active human use, was denied +identically — so the denial is not specific to how the work was re-enqueued, nor to the device +being idle. + +**The previous "roughly two minutes" wait stopped essentially at the moment it would have fired.** +Forcing was never necessary, and the forced result was correct. + +### What this costs the user + +- **`reschedule = false`.** Terminal. Nothing runs again, despite `run_attempt_count=2`. +- **No error message reaches the UI at all.** Both failed rows carry output `Data` of + `X'ABEF000100000000'` — the header with **zero entries**. The throw escapes at `setForeground` + (line 66), above `return try {` (line 92), so `handleTimeoutIfNeeded`, `Result.retry()`, the + `workDataOf(KEY_ERROR …)` and `staged.delete()` are all bypassed. `ConversionState.Failed` then + renders its generic fallback. +- **A partial file is orphaned.** 2 MB of `long_input2_converted.mp4` was left in staging because + `staged.delete()` is unreachable — feeding D2. +- **`HAS_FOREGROUND_EXEMPTION` was set on the job and it was still denied.** That flag governs + runtime guarantees once started, not permission to start. +- **The job is ordinary** — `Priority: 300 [DEFAULT]`, not expedited, not user-initiated. There is + no allowance a natural dispatch could carry that forcing withheld. + +**One observation with implications beyond this entry:** the *initial* foreground-service start +succeeded during testing only because instrumentation was active (`code:ACTIVITY_STARTER`, +`allowWiu:52`) — an allowance the app does not have in production either. What grants it in normal +use is the user launching the app; nothing grants it on a background restart. + +### Relationship to the other entries + +D3's fix makes this *visible* rather than silent — a reattached FAILED job with blank output data +falls back to "Conversion failed." instead of an empty screen — and D2's sweep collects the +orphaned partial. **Neither addresses the cause.** The user still loses a long conversion that the +architecture promised would survive. + +--- + +## D14 — A failed FFprobe load crashes the pick instead of reporting it + +**Severity: low (rare trigger) · Confirmed empirically on the JVM · Cannot fire on a device that ships the libraries** + +Found while building D2's ViewModel tests, not present in the original inventory. + +`MediaProbe.probeWithFFprobe` guards its FFprobe call with `catch (e: Exception)`. But when +FFmpegKit's native library cannot be loaded, the failure arrives as a bare **`java.lang.Error`**, +not an `Exception`: + +``` +java.lang.Error: FFmpegKit failed to start on brand: robolectric ... +``` + +`Error` is not a subclass of `Exception`, so that catch does not see it. `ConversionViewModel.onInputPicked` +does not catch it either, and it runs inside `viewModelScope.launch` — so the failure propagates as +an uncaught error rather than the "could not read this file" outcome the surrounding code is +written to produce. + +**Why the trigger is rare, and why it was still worth recording.** On a normally-installed app the +`.so` files are present and this cannot happen; it was observed on the JVM, where they are absent by +construction. A corrupted install or an ABI mismatch is the only realistic device path. That is also +why it was *not* fixed on discovery: widening the catch to `Throwable` changes the file-pick path on +a condition no ordinary user meets, and swallowing `Error` indiscriminately would hide genuine +`OutOfMemoryError`s in a method that spawns a native process. + +**Note the shape.** This is the same class of mistake as D1's original `catch (IOException)` being +too narrow for a platform call that throws `RuntimeException`. Both are "the guard does not cover +what the boundary actually throws". Worth checking the other native boundaries — `FFmpegEngine`, +`ConcatEngine`, `Media3Engine` — for the same gap before calling this one closed. + +--- + +## D15 — An oversized suggested name turns a finished conversion into a failure + +**Severity: low (very narrow trigger) · Found while fixing D9 · Not fixed** + +`Data.Builder.build()` throws above 10 KB, and both workers build their **success** `workDataOf(...)` +*inside* the `try`. So a `KEY_SUGGESTED_NAME` large enough to push the output `Data` over the cap +would be caught by the surrounding handler: the conversion is reported **failed**, and +`staged.delete()` removes the finished file. + +The trigger is genuinely narrow. Input `Data` already carries `KEY_DISPLAY_NAME` and is built at +`enqueue()` time, so it would have to survive that; the failure then needs a display name landing in +roughly a **74-byte window near 10 KB**. A provider would have to supply a filename of about that +length exactly. + +Left alone deliberately when D9 landed — capping the name is its own change with its own decision +(truncate where? preserve the extension?), and doing it inside a naming commit would have buried it. + +**Fix direction:** cap the suggested name before it reaches `Data`, or build the success `Data` +outside the `try`. The second is smaller but changes which failures delete the staged file, so it +needs its own test. + +--- + +## D16 — A job that exhausts its foreground-start retries reports to nobody + +**Severity: low-medium · Found while fixing D13 · Not fixed** + +D13's fix bounds foreground-start retries at 10 attempts and then reports `FOREGROUND_DENIED` — +a real message, replacing the empty output `Data` the device measured. But that message arrives on a +**FAILED** job, and `Reattachment.choose` (D3's fix) deliberately **excludes FAILED** work. + +So a user who is not watching at the eleventh attempt — roughly 8.5 hours after the job was +enqueued, given the measured backoff — opens the app to an empty screen. What the bound reliably +buys is the *end* of the retrying, not the telling; `MAX_FOREGROUND_START_ATTEMPTS`'s KDoc now says +exactly that rather than implying more. + +**Fix direction:** let reattachment surface a terminal failure that carries a message, distinct from +one that does not. That means changing `Reattachment.choose`'s exclusion rule, which was explicitly +out of scope for the commit that created the situation. + +--- + +## Summary + +| ID | Defect | Severity | Evidence | Fix | +|---|---|---|---|---| +| D13 | Interrupted work fails terminally instead of resuming | high | **confirmed on natural dispatch** | **merged** | +| D3 | A conversion outliving the process becomes unreachable | high | **reproduced on the Pixel** | **merged** | +| D2 | Staging files are never cleaned up | medium | **reproduced on the Pixel** | **merged** | +| D8 | No job gets a staging path of its own | medium | naming **observed live**; concurrency unconfirmed | **merged** | +| D4 | `publish()` can leave a truncated file at the destination | medium | not attempted | **merged** | +| D6 | Rotation resets the selected tab | medium | inspection only | **merged** | +| D1 | `hasSpaceFor` measures the wrong quantity | medium | **NOT reproduced — premise contradicted** | parked, see below | +| D5 | The space check can be vacuous | low-medium | **confirmed live** | open | +| D7 | Direct `notify()` on WorkManager's notification ID | low-medium | resurrection **reproduced**; undismissability unverified | open | +| D9 | Output names derived from the wrong source | low | latent | **merged** | +| D10 | `CancellationException` swallowed | low | not attempted | **merged** | +| D11 | Documentation and scaffold | low | inspection only | **merged** | +| D12 | Two detekt findings that are wrong | n/a | inspection only | no action — correct as written | +| D14 | A failed FFprobe load crashes the pick | low (rare trigger) | **confirmed on the JVM** | **merged** | +| D16 | Exhausted foreground-start retries report to nobody | low-medium | found while fixing D13 | open | +| D15 | An oversized suggested name fails a finished conversion | low (very narrow) | found while fixing D9 | open | + +**Where the fixes live.** All merged work is on `feat/defect-fixes-base`, which now carries D2, D3, +D4, D6, D8, D9, D10, D11, D13 and D14 and gates green (**242 JVM tests, detekt 0, lint clean**, up +from 180). D5 and D7 are in progress on `fix/space-proxy-and-notification`. **D1 is parked unmerged** on +`fix/allocatable-space`: the code is sound but the device pass contradicted its stated premise, so +landing it needs a near-full-disk measurement first — see its entry. D15 and D16 were both found *while fixing* other entries and are +recorded rather than folded in silently. + +Separately, `tools/local-emulator` carries the finding that **local emulators do work** — the +segfault was SwiftShader's JIT against Fedora's SELinux `execheap` denial, and API 33–36 now run +locally, 49 tests each, matching the Pixel. `docs/local-emulator.md` has the backtrace and the mode +matrix, and proposes a `CLAUDE.md` correction that has not been applied. + +### If these are fixed + +The order is not arbitrary: + +1. **D3** — highest severity, and independent of everything else. +2. **D8 before D2** — a sweep's notion of "orphan" means nothing until a staging name belongs to + exactly one job. **D9 rides along**, since it touches the same name derivations, and leaving + one corrected literal beside an uncorrected one is worse than fixing both. +3. **D2, then D1 last** — because D1's fix would otherwise mask D2 in the space check. +4. **D11** — independent cleanup, any time. + +Per `CLAUDE.md` and this repository's history, each concern is its own commit on its own branch +through a PR, never on `main`. + +### On testing these + +The JVM test source set has exactly one dependency, `testImplementation(libs.junit)`. That is why +every well-tested class in this project is pure (`model/`, `FFmpegCommandBuilder`, +`FailureOutcome`) and every untested one takes a `Context`. Instrumented tests cannot run on the +development host (`CLAUDE.md`, "Instrumented tests do not run locally"), so an androidTest-only +red test is not a TDD loop anyone can execute here. + +The approach chosen for the follow-up work is **pure seams plus Robolectric**: extract each +decision into a pure function on the existing JUnit 4 stack — the pattern `work/FailureOutcome.kt` +documents in its own KDoc — and add Robolectric for the file-lifecycle behaviour a pure function +cannot express. Adding it needs a **pinned** `testImplementation` entry (the `componentSelection` +prerelease guard in `app/build.gradle.kts` covers only `androidx.`, `junit` and `com.arthenica`, +so a new group would float unguarded) and a `testOptions { unitTests.isIncludeAndroidResources = true }` +block, which this module does not currently have at all. + +Worth knowing before adding anything: **`androidx.work:work-testing`, `compose-ui-test-junit4` and +`espresso-core` are already declared and have zero users.** `TestListenableWorkerBuilder` and +`createComposeRule` are available on the androidTest classpath today with no build change. + +## Not covered here + +- **The API 37 emulator crash** — fully documented in [`api-37-emulator-crash.md`](api-37-emulator-crash.md). + Its one open action is unchanged: the upstream issue is still *"Not yet filed."* +- **The 467 `allRules` findings** — see the opening section. 402 are documentation and + test-name-length opinions, 2 are wrong (D12), none are potential-bugs. +- **`CognitiveComplexMethod`** on `FileCard`, `ConversionViewModel.observe` and + `JoinViewModel.join`. The rule is off by default, and this project already forgives Composable + complexity deliberately. +- **The coverage gate.** Still reported, not gated, at ~31%. Fixing the entries above would move + the number, which is another reason not to set a floor before they are decided. diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index ffde3ac..7b96107 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -67,6 +67,19 @@ detekt = "2.0.0-alpha.6" # so the agent version that reads Kotlin 2.2.10 bytecode is stated, not implied. jacoco = "0.8.15" +# Robolectric. PINNED, and it belongs with ktlint/detekt/jacoco above rather than with +# the floating libraries, for two reasons that both point the same way. +# +# First, the prerelease guard in app/build.gradle.kts only covers the groups this project +# floats -- "androidx.", "junit", "com.arthenica" -- so org.robolectric is unguarded, and +# a "4.+" here would resolve straight to 4.17-beta-3, which is the newest thing published. +# Second, Robolectric is not a library the app ships: it is the JVM's Android runtime, and +# a version bump changes which android-all jar the tests execute against. That is the same +# "a tool moved under a diff that cannot explain it" failure the linters are pinned for. +# +# 4.16.1 is the newest RELEASED version; the 4.17 line is beta-only at the time of writing. +robolectric = "4.16.1" + [libraries] androidx-core-ktx = { group = "androidx.core", name = "core-ktx", version.ref = "coreKtx" } androidx-activity-compose = { group = "androidx.activity", name = "activity-compose", version.ref = "activityCompose" } @@ -115,6 +128,12 @@ junit = { group = "junit", name = "junit", version.ref = "junit" } androidx-junit = { group = "androidx.test.ext", name = "junit", version.ref = "androidxJunit" } androidx-espresso-core = { group = "androidx.test.espresso", name = "espresso-core", version.ref = "espressoCore" } +# Robolectric — an Android runtime for the JVM test source set, so file-lifecycle behaviour +# that needs a real Context can be verified without a device. The instrumented suite cannot +# run on the development host at all (see CLAUDE.md), so an androidTest-only red test is not +# a TDD loop anyone here can execute. +robolectric = { group = "org.robolectric", name = "robolectric", version.ref = "robolectric" } + [plugins] # com.android.application and org.jetbrains.kotlin.plugin.compose are deliberately absent. # They come from the root buildscript classpath (see build.gradle.kts) so that a newer KGP