diff --git a/app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt b/app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt index 805cb0f..6cc9978 100644 --- a/app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt +++ b/app/src/main/java/org/libremediaconverter/convert/ConversionViewModel.kt @@ -67,7 +67,14 @@ 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, @@ -274,8 +281,11 @@ class ConversionViewModel @JvmOverloads constructor( 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) diff --git a/app/src/main/java/org/libremediaconverter/convert/ConverterScreen.kt b/app/src/main/java/org/libremediaconverter/convert/ConverterScreen.kt index a68838d..93c9f44 100644 --- a/app/src/main/java/org/libremediaconverter/convert/ConverterScreen.kt +++ b/app/src/main/java/org/libremediaconverter/convert/ConverterScreen.kt @@ -163,9 +163,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( diff --git a/app/src/main/java/org/libremediaconverter/join/JoinScreen.kt b/app/src/main/java/org/libremediaconverter/join/JoinScreen.kt index e5b6707..8bf0eeb 100644 --- a/app/src/main/java/org/libremediaconverter/join/JoinScreen.kt +++ b/app/src/main/java/org/libremediaconverter/join/JoinScreen.kt @@ -111,9 +111,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( diff --git a/app/src/main/java/org/libremediaconverter/work/ConcatWorker.kt b/app/src/main/java/org/libremediaconverter/work/ConcatWorker.kt index 542c26c..5ecab5a 100644 --- a/app/src/main/java/org/libremediaconverter/work/ConcatWorker.kt +++ b/app/src/main/java/org/libremediaconverter/work/ConcatWorker.kt @@ -46,16 +46,23 @@ class ConcatWorker(context: Context, params: WorkerParameters) : CoroutineWorker 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. 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( @@ -65,11 +72,15 @@ class ConcatWorker(context: Context, params: WorkerParameters) : CoroutineWorker ) } 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."))) diff --git a/app/src/main/java/org/libremediaconverter/work/ConversionWorker.kt b/app/src/main/java/org/libremediaconverter/work/ConversionWorker.kt index 8775504..55bff81 100644 --- a/app/src/main/java/org/libremediaconverter/work/ConversionWorker.kt +++ b/app/src/main/java/org/libremediaconverter/work/ConversionWorker.kt @@ -37,6 +37,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) { @@ -63,33 +69,43 @@ class ConversionWorker(context: Context, params: WorkerParameters) : CoroutineWo 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})") - + // 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. val staged = publisher.createStagingFile(outputNameFor(displayName, spec)) 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)) + + 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})") + when (decision.engine) { Engine.MEDIA3 -> runMedia3OrFallBack(request, inputUri, staged, displayName) Engine.FFMPEG -> runFFmpeg(request, inputUri, staged, displayName) @@ -103,7 +119,7 @@ class ConversionWorker(context: Context, params: WorkerParameters) : CoroutineWo ) } catch (e: Throwable) { staged.delete() - handleTimeoutIfNeeded(e) + outcomeFor(e) } } @@ -172,24 +188,27 @@ class ConversionWorker(context: Context, params: WorkerParameters) : CoroutineWo private fun isCancellation(e: Throwable): Boolean = e is kotlinx.coroutines.CancellationException || isStopped /** - * Distinguishes a genuine failure from the foreground-service budget expiring. + * Turns whatever ended the attempt into a `Result`. [FailureOutcome] owns the rules. * - * `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. + * 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 handleTimeoutIfNeeded(cause: Throwable): Result = when (FailureOutcome.forStopReason(stopReason)) { - FailureOutcome.RETRY -> { - Log.w(TAG, "Foreground service budget exhausted; will retry.", cause) - Result.retry() + 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."))) + } } - 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. 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/Reattachment.kt b/app/src/main/java/org/libremediaconverter/work/Reattachment.kt index 4f943dd..8e249b3 100644 --- a/app/src/main/java/org/libremediaconverter/work/Reattachment.kt +++ b/app/src/main/java/org/libremediaconverter/work/Reattachment.kt @@ -80,11 +80,14 @@ sealed interface Reattachment { * * - **[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. That matters more than it looks: a worker - * interrupted by process death can come back FAILED rather than retried, because the - * restart's `setForeground` is refused as a background foreground-service start, so - * failures left behind by earlier sessions are ordinary rather than rare. + * - **[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. 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..7fd3646 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/work/DeniedForegroundStartTest.kt @@ -0,0 +1,200 @@ +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.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.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; reaching staged.delete() is what collects it. + val staged = stagedFile().apply { writeBytes(ByteArray(PARTIAL_BYTES)) } + + runBlocking { conversionWorker().doWork() } + + assertFalse("a denied restart must not orphan the previous attempt's partial", staged.exists()) + } + + @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(ConversionWorker.outputNameFor(DISPLAY_NAME, SPEC)) + + 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) +} + +/** + * A real publisher that never refuses on space. + * + * The space check reads the host's free disk, which has nothing to do with what these tests are + * about and would make them pass or fail on how full the machine is. Where staging lives, and the + * delete, are the production implementation. + */ +private class AlwaysRoomPublisher(context: Context) : OutputPublisher(context) { + override fun hasSpaceFor(bytes: Long): Boolean = true +} 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", + ) }