JMR-dev/LibreMediaConverter
1
0
Fork 0
Code Issues 16 Pull Requests Actions Packages Projects Releases Wiki Activity

Retry work the system refused to start, instead of failing it terminally

Browse Source 
`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) <noreply@anthropic.com>
This commit is contained in:
JMR-dev
2026-08-22 20:11:39 -05:00
co-authored by Claude Opus 5
parent eb37f9ebea
commit 5c27801c77
9 changed files with 499 additions and 78 deletions
Download Patch File Download Diff File Expand all files Collapse all files
app/src/test/java/org/libremediaconverter/work/DeniedForegroundStartTest.kt
+200
View File
@@ -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<ConversionWorker>(
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<ConcatWorker>(
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<Void> = 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<Void> {
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
}
app/src/test/java/org/libremediaconverter/work/FailureOutcomeTest.kt
+100 -7
View File
@@ -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",
)
}