From 5c27801c77c781bdcfd62b1122a6a65bceee2266 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Sat, 22 Aug 2026 19:35:30 -0500 Subject: [PATCH] Retry work the system refused to start, instead of failing it terminally `ConversionWorker`'s KDoc says the queue survives process death, and that is the app's stated reason for choosing WorkManager at all. A Pixel 10 Pro XL says otherwise. An 18-minute transcode was killed mid-write with `kill -9`; 119 seconds later, on a natural dispatch with no `cmd jobscheduler run` anywhere in the session, WorkManager recovered it by itself and the system refused it: WM-ForceStopRunnable: Found unfinished work, scheduling it. ActivityManager: Background started FGS: Disallowed [callingPackage: org.libremediaconverter; uidState: CEM; BFGS denied: true; code:DENIED] WM-WorkerWrapper: android.app.ForegroundServiceStartNotAllowedException WM-WorkerWrapper: Worker result FAILURE WM-Processor: Processor 3d1c9862 executed; reschedule = false `Found unfinished work` is the clean process-death recovery path, not a force-stop. The job was ordinary -- `Priority: 300 [DEFAULT]`, not expedited -- so there was no allowance it could have carried. `HAS_FOREGROUND_EXEMPTION` was set on it and it was still denied; that flag governs the runtime guarantee once a service is running, not permission to start one. The cause is structural rather than subtle. `setForeground(...)` sat above `return try {`, so its throw escaped `doWork()` without reaching `handleTimeoutIfNeeded`, `Result.retry()`, the `workDataOf(KEY_ERROR ...)` or `staged.delete()`. Three separate costs, all measured on the device: `reschedule = false`, so nothing ran again despite `run_attempt_count=2`; output `Data` of `X'ABEF000100000000'`, a header with zero entries, so the screen said "Conversion failed." with nothing to add; and 2 MB of a partial `long_input2_converted.mp4` left in staging. `ConcatWorker` had the same shape. So `setForeground` moves inside the `try` in both workers, and the staging handle is named just above it -- `createStagingFile` only builds a path, nothing is written until an engine opens it, so naming it early costs nothing and makes the catch total. `MediaProbe.probe` was outside the `try` for the same reason and had the same problem; it is now inside too. On a retry the staged name is unchanged, so the delete on the way out collects the partial the killed attempt left behind rather than orphaning it. What to return was the real decision. `Result.retry()`, because the denial is about *when* the job ran and not about the job: the file is fine, the settings are fine, and the one thing that grants an app permission to start a foreground service is being in the foreground, which the user supplies by opening the app. But WorkManager never gives up on its own, so an unbounded retry means a job nobody comes back for waking the device forever while the screen says "paused" and never explains itself. `FailureOutcome` therefore bounds it at ten attempts, chosen against the default backoff rather than as a round number: exponential from `WorkRequest.DEFAULT_BACKOFF_DELAY_MILLIS` (30 s), doubling, clamped at `MAX_BACKOFF_MILLIS` (5 h), which spans about 8 h 30 m before the eleventh attempt fails with a message the user can act on. The counter is `runAttemptCount`, which counts every attempt and not only denied ones -- WorkManager exposes no other -- so a transcode already retried ten times by the six-hour budget will fail on its first denial rather than getting ten of its own. That is accepted rather than overlooked, and the timeout branch ignores the count entirely so the budget's own retries are untouched. The rule goes in `FailureOutcome` rather than beside it, which is what its KDoc asks for: isolate a decision whose triggering condition cannot be provoked in a test. It now reads the stop reason *and* the cause, with precedence stated rather than left to branch order -- once something has stopped the worker, the exception it was holding describes that stop and not a reason of its own. The match is on `ForegroundServiceStartNotAllowedException` exactly, never on its `IllegalStateException` supertype: a muxer that was never started throws one of those too, and matching the supertype would retry every ordinary failure for eight hours. Expedited work is still not used, and this is not an argument for it. It maps to JobScheduler expedited jobs with a short quota, which is the wrong shape for a multi-minute transcode; the exemption it buys is for starting, and the quota it costs would be paid by every job. Tested at both levels, because only one of them bites. `FailureOutcomeTest` gains six cases for the rule -- retry at the bound, give up past it, an unrelated `IllegalStateException` that must not be mistaken for a denial, a stop reason winning over the exception, and the budget timeout ignoring the count. `DeniedForegroundStartTest` covers the wiring, which is where the defect actually was: a `ForegroundUpdater` whose future completes exceptionally makes `setForeground` throw exactly what the platform throws, because `WorkForegroundUpdater`'s own comment says it propagates the exception to the caller and `ListenableFuture.await()` unwraps the `ExecutionException` on the way. Moving `setForeground` back above the `try` turns all four of those red with a bare `android.app.ForegroundServiceStartNotAllowedException`, which is the same line the device logged. Four comments claimed the old behaviour and are corrected with it. `ConversionState.Waiting` said the budget had run out; `observe()`'s ENQUEUED branch said the budget was the likely cause, where a denied restart is now the likelier of the two; and `Reattachment.choose` justified excluding FAILED partly on interrupted workers coming back FAILED "rather than retried", which is the sentence this commit falsifies. The exclusion stands on its own and says so now. The bound's own KDoc says what giving up does *not* buy, too. `FOREGROUND_DENIED` reports through a FAILED job and `Reattachment` excludes FAILED, so a user who was not watching at the eleventh attempt meets an empty screen rather than the message. What the bound reliably buys is the end of the retrying. Both `Waiting` screens change their wording. The state is `ENQUEUED` with a run attempt behind it, which cannot distinguish the two causes that now reach it, and the old copy named only the six-hour budget -- which is now the less likely of the two. "Keeping the app open helps it along" covers both, and for a denied start it is not filler but the actual remedy. Co-Authored-By: Claude Opus 5 (1M context) --- .../convert/ConversionViewModel.kt | 16 +- .../convert/ConverterScreen.kt | 10 +- .../libremediaconverter/join/JoinScreen.kt | 6 +- .../libremediaconverter/work/ConcatWorker.kt | 31 ++- .../work/ConversionWorker.kt | 99 +++++---- .../work/FailureOutcome.kt | 95 ++++++++- .../libremediaconverter/work/Reattachment.kt | 13 +- .../work/DeniedForegroundStartTest.kt | 200 ++++++++++++++++++ .../work/FailureOutcomeTest.kt | 107 +++++++++- 9 files changed, 499 insertions(+), 78 deletions(-) create mode 100644 app/src/test/java/org/libremediaconverter/work/DeniedForegroundStartTest.kt 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", + ) }