From e7caeeac435aa34e15b3dac6893782871a78562b Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Sat, 5 Sep 2026 19:15:11 -0500 Subject: [PATCH] Finish the startup sweep before onCreate returns, in the JVM suite Robolectric builds an Application per test class that asks for one, and each onCreate launched a staging sweep on Dispatchers.IO over the shared /conversions/. Nothing joined them, so a test asserting about a staged file was racing every sweep the classes before it had left in flight (#159). It was CI-only until wave 4 added ten Robolectric classes, at which point OutputPublisherStagingTest started failing locally too. LibreMediaConverterApp gains a protected open sweepScope and publishes the Job onCreate started; the JVM suite substitutes TestLibreMediaConverterApp, whose scope is Dispatchers.Unconfined so the sweep -- a plain function that never suspends -- runs to completion inline. The SupervisorJob is kept so this differs from production in the dispatcher alone. Suite-wide rather than per-test: 27 of the 58 Robolectric classes touch that directory, so opt-in was not a real option. It costs one assertion, knowingly. AppStartSweepTest opened by asserting that the manifest's android:name is what Robolectric instantiated. An application= override replaces the manifest rather than being checked against it, and applicationInfo.className reports the override too, so that claim is now unobservable from this source set and a rewritten version would assert the override against itself. The manifest link is device-only; the cast in setUp still catches the test app ceasing to extend the real one. AppStartSweepTest also joins the published Job instead of polling for ten seconds -- a poll cannot tell "swept" from "not started yet" -- and gains a test pinning that the sweep is complete when onCreate returns, which is the property the substitution exists for and the only place it is checked. Verified by mutation rather than by repetition. Putting the test app back on Dispatchers.IO reddens that test 5 times out of 5, while running the whole suite six times per arm caught nothing either way: at the rate #159 was observed at, a clean six-run arm is roughly a coin flip, so the comparison was underpowered and is not offered as evidence. Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 21 +++++ .../LibreMediaConverterApp.kt | 29 ++++++- .../libremediaconverter/AppStartSweepTest.kt | 76 +++++++++++-------- .../TestLibreMediaConverterApp.kt | 28 +++++++ .../convert/OutputPublisherStagingTest.kt | 33 ++++---- app/src/test/resources/robolectric.properties | 6 ++ 6 files changed, 144 insertions(+), 49 deletions(-) create mode 100644 app/src/test/java/org/libremediaconverter/TestLibreMediaConverterApp.kt diff --git a/CLAUDE.md b/CLAUDE.md index bb2f592..f8162d0 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -411,3 +411,24 @@ Because versions float, a build can change without a commit. `./gradlew :app:dep `app/build.gradle.kts`, neither of which moves a thread. `HangBoundTest` guards both numbers, and **a timed-out run writes no XML for the class that hung** — the dump is its only attribution, so do not delete the watchdog as stray config. +- **The JVM suite does not run `LibreMediaConverterApp`.** `app/src/test/resources/robolectric.properties` + names `TestLibreMediaConverterApp` for every test, and it differs from the real class in exactly + one thing: `sweepScope` is `Dispatchers.Unconfined`, so the startup staging sweep finishes before + `onCreate()` returns instead of running on `Dispatchers.IO`. + + **That line is load-bearing — do not delete it as stray config.** Robolectric builds an + `Application` per test class that asks for one, and each `onCreate` launched a sweep over the + shared `/conversions/` that nothing joined. So a test asserting about a staged file was + racing every sweep the classes before it had left in flight (#159). It was CI-only until wave 4 + added ten Robolectric classes, at which point `OutputPublisherStagingTest` failed on roughly one + local run in six. Per-test opt-in was measured and rejected: **27 of the 58 Robolectric classes + touch that directory**. The `SupervisorJob` is kept in the test scope so a throwing sweep is + swallowed there exactly as in production — the dispatcher is the only intended difference. + + **It cost one assertion, knowingly.** `AppStartSweepTest` used to open by asserting that the + manifest's `android:name` is what Robolectric instantiated, so the sweep is code that actually + runs. An `application=` override *replaces* the manifest rather than being checked against it, and + `applicationInfo.className` reports the override too — measured — so that claim is not merely + unasserted on the JVM now, it is unobservable, and a rewritten version would assert the override + against itself. **The manifest link is device-only.** What remains is the `as LibreMediaConverterApp` + cast in that class's `setUp`, which catches only the test app ceasing to extend the real one. diff --git a/app/src/main/java/org/libremediaconverter/LibreMediaConverterApp.kt b/app/src/main/java/org/libremediaconverter/LibreMediaConverterApp.kt index c25cd5f..312be98 100644 --- a/app/src/main/java/org/libremediaconverter/LibreMediaConverterApp.kt +++ b/app/src/main/java/org/libremediaconverter/LibreMediaConverterApp.kt @@ -3,6 +3,7 @@ package org.libremediaconverter import android.app.Application import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.Job import kotlinx.coroutines.SupervisorJob import kotlinx.coroutines.launch import org.libremediaconverter.convert.OutputPublisher @@ -17,14 +18,36 @@ import org.libremediaconverter.convert.OutputPublisher * ever becomes a `Converted` state, or a `reset()`'s delete is cancelled along with the * Activity. Process start is the one moment those leftovers are reliably observable. */ -class LibreMediaConverterApp : Application() { +open class LibreMediaConverterApp : Application() { /** * Deliberately process-lifetime and never cancelled: the work it carries is a single * short task that should outlive nothing in particular and be interrupted by nothing. * A `SupervisorJob` so a failure here could never take a sibling down with it. + * + * **`protected open` for #159.** Robolectric builds an `Application` for every test that asks + * for one, so on the JVM this is not one background sweep but one *per test* — all of them on + * `Dispatchers.IO`, all touching the same `cacheDir`, none of them joined by anything. That is + * a race against any test asserting about a file under `conversions/`, and it grew with the + * suite: wave 4 added ten Robolectric classes and took it from CI-only to roughly one local run + * in six. The JVM suite substitutes a scope that runs the sweep inline — see + * `app/src/test/resources/robolectric.properties` and `TestLibreMediaConverterApp`. + * + * A constructor parameter would be the ordinary way to inject this and is not available: the + * framework builds this class, so the seam has to be a member. */ - private val appScope = CoroutineScope(SupervisorJob() + Dispatchers.IO) + protected open val sweepScope: CoroutineScope = CoroutineScope(SupervisorJob() + Dispatchers.IO) + + /** + * The sweep [onCreate] last started, so a caller that needs it finished can wait for it. + * + * Nothing in production reads this — process start does not wait for its own housekeeping. It + * exists because the alternative for a test is a timed poll, and a poll cannot tell "the sweep + * has not run yet" from "the sweep ran and did nothing". + */ + @Volatile + var startupSweep: Job? = null + private set override fun onCreate() { super.onCreate() @@ -53,6 +76,6 @@ class LibreMediaConverterApp : Application() { // // sweepStaging() also re-reads each timestamp immediately before deleting, which // closes the window between listing the directory and acting on the listing. - appScope.launch { OutputPublisher(this@LibreMediaConverterApp).sweepStaging() } + startupSweep = sweepScope.launch { OutputPublisher(this@LibreMediaConverterApp).sweepStaging() } } } diff --git a/app/src/test/java/org/libremediaconverter/AppStartSweepTest.kt b/app/src/test/java/org/libremediaconverter/AppStartSweepTest.kt index e5a8646..47b0ad4 100644 --- a/app/src/test/java/org/libremediaconverter/AppStartSweepTest.kt +++ b/app/src/test/java/org/libremediaconverter/AppStartSweepTest.kt @@ -1,8 +1,8 @@ package org.libremediaconverter -import org.junit.Assert.assertEquals +import kotlinx.coroutines.runBlocking +import org.junit.Assert.assertNotNull import org.junit.Assert.assertTrue -import org.junit.Assert.fail import org.junit.Before import org.junit.Test import org.junit.runner.RunWith @@ -10,7 +10,6 @@ import org.libremediaconverter.convert.StagingSweep import org.robolectric.RobolectricTestRunner import org.robolectric.RuntimeEnvironment import java.io.File -import java.util.concurrent.TimeUnit /** * That process start actually sweeps. @@ -23,8 +22,19 @@ import java.util.concurrent.TimeUnit * output ever became a `Converted` state, a `reset()` whose delete was cancelled with the Activity. * * `onCreate()` is called again rather than a second Application being built: it is what the - * framework calls at process start, the scope it launches on is already there, and the first test - * below is what pins that the framework calls it on *this* class. + * framework calls at process start, and the scope it launches on is already there. + * + * **What this class stopped covering in #159, deliberately.** It used to open by asserting that + * `RuntimeEnvironment.getApplication()` is a [LibreMediaConverterApp] — that the manifest's + * `android:name` points here, so the sweep is code that actually runs. That assertion cannot exist + * on the JVM any more: `robolectric.properties` now names [TestLibreMediaConverterApp] for the + * whole suite, and an `application=` override replaces the manifest rather than being checked + * against it — `applicationInfo.className` reports the override too, measured. So the manifest is + * not merely unasserted here, it is unobservable from this source set, and a rewritten version of + * that test would have asserted the override against itself. **The manifest link is a device-only + * guarantee now**, and it was traded knowingly for the race that override fixes. The cast in + * [setUp] still fails if [TestLibreMediaConverterApp] stops extending the real class, which is a + * smaller claim than the one withdrawn. */ @RunWith(RobolectricTestRunner::class) class AppStartSweepTest { @@ -34,17 +44,35 @@ class AppStartSweepTest { @Before fun setUp() { - // The cast is an assertion in itself: Robolectric builds the Application named in the - // merged manifest, so this fails if `android:name` ever stops pointing here -- in which - // case the sweep below would be perfectly correct code that never runs. app = RuntimeEnvironment.getApplication() as LibreMediaConverterApp stagingDir = File(app.cacheDir, "conversions").apply { mkdirs() } stagingDir.listFiles()?.forEach { it.delete() } } + /** + * The property the whole substitution exists for, asserted directly rather than waited on. + * + * #159 is not "the sweep is slow", it is "the sweep is still running while some later test + * reads the directory". [TestLibreMediaConverterApp] answers that by finishing the sweep before + * `onCreate()` returns, and this is the only place that claim is checked -- every other test in + * the suite benefits from it silently and would go back to racing without saying why. + * + * Deterministic in the direction that matters: `Dispatchers.Unconfined` runs a `launch` whose + * body never suspends to completion inline, so this cannot flake green-to-red. Putting the test + * app back on `Dispatchers.IO` makes it a race that the assertion loses essentially every time, + * which is what a six-run suite comparison could not show -- at the rate #159 was observed at, + * a clean six-run arm is a coin flip. + */ @Test - fun `the application the manifest starts is the one that sweeps`() { - assertEquals(LibreMediaConverterApp::class.java, RuntimeEnvironment.getApplication().javaClass) + fun `the sweep is finished before onCreate returns`() { + app.onCreate() + + val sweep = app.startupSweep + assertNotNull("onCreate() started no sweep", sweep) + assertTrue( + "the JVM suite's sweep outlived onCreate(), so it is in flight during test bodies again", + sweep?.isCompleted == true, + ) } @Test @@ -64,35 +92,23 @@ class AppStartSweepTest { app.onCreate() - awaitGone(abandoned) + // Joined rather than polled. `onCreate` publishes the sweep it started, so this waits for + // that exact sweep -- where a timed poll could not tell "swept" from "not started yet", and + // answered the second case by failing after ten seconds. + val sweep = app.startupSweep + assertNotNull("onCreate() started no sweep to wait for", sweep) + runBlocking { sweep?.join() } + + assertTrue("process start left ${abandoned.name} in staging; nothing swept it", !abandoned.exists()) // The other half, and the one that says the sweep is a sweep rather than a // `clearStaging()`: the directory is shared by the convert tab, the join tab and // ConcatEngine's list file, so deleting everything could take a file from a running job. assertTrue("a file written moments ago belongs to a live job", live.exists()) } - /** - * Waits for [file] to be deleted. - * - * The sweep runs on `Dispatchers.IO`, deliberately: it lists a directory and stats every entry - * on the path that decides how long the launcher icon stays unresponsive. So there is nothing - * to join, and the wait is a bounded poll — long enough for a directory listing, short enough - * that a sweep which never happens fails rather than hangs. - */ - private fun awaitGone(file: File) { - val deadline = System.nanoTime() + TimeUnit.SECONDS.toNanos(AWAIT_TIMEOUT_SECONDS) - while (System.nanoTime() < deadline) { - if (!file.exists()) return - Thread.sleep(POLL_INTERVAL_MS) - } - fail("process start left ${file.name} in staging; nothing swept it") - } - private fun stagedFile(name: String): File = File(stagingDir, name).apply { writeBytes(ByteArray(4096)) } private companion object { const val ONE_MINUTE_MS = 60L * 1000 - const val AWAIT_TIMEOUT_SECONDS = 10L - const val POLL_INTERVAL_MS = 5L } } diff --git a/app/src/test/java/org/libremediaconverter/TestLibreMediaConverterApp.kt b/app/src/test/java/org/libremediaconverter/TestLibreMediaConverterApp.kt new file mode 100644 index 0000000..76bf6d0 --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/TestLibreMediaConverterApp.kt @@ -0,0 +1,28 @@ +package org.libremediaconverter + +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.SupervisorJob + +/** + * The [LibreMediaConverterApp] the JVM suite runs, differing from it in exactly one thing: the + * startup sweep runs inline on the thread that builds the Application instead of on + * `Dispatchers.Unconfined`. + * + * **This is #159.** Robolectric builds an `Application` per test class that asks for one, and each + * one launches a sweep over the shared `/conversions/`. Nothing joins them, so a test + * asserting about a staged file is racing however many sweeps the classes before it left in + * flight — `OutputPublisherStagingTest` being the one that lost, at roughly one local run in six + * once wave 4 added ten more Robolectric classes. Making the sweep finish before `onCreate()` + * returns removes the race for every test at once rather than asking each to opt in; 27 of the + * suite's 58 Robolectric classes touch that directory, so opting in was not a real option. + * + * `Dispatchers.Unconfined` is what makes it inline: `sweepStaging()` is a plain function, so an + * `Unconfined` `launch` runs it to completion before returning. The `SupervisorJob` is kept so this + * differs from production in the dispatcher alone — a sweep that throws is logged and swallowed + * here exactly as it is there, rather than taking Application construction down with it and failing + * every test in the class for an unrelated reason. + */ +class TestLibreMediaConverterApp : LibreMediaConverterApp() { + override val sweepScope: CoroutineScope = CoroutineScope(SupervisorJob() + Dispatchers.Unconfined) +} diff --git a/app/src/test/java/org/libremediaconverter/convert/OutputPublisherStagingTest.kt b/app/src/test/java/org/libremediaconverter/convert/OutputPublisherStagingTest.kt index c70b965..0c2472c 100644 --- a/app/src/test/java/org/libremediaconverter/convert/OutputPublisherStagingTest.kt +++ b/app/src/test/java/org/libremediaconverter/convert/OutputPublisherStagingTest.kt @@ -122,24 +122,25 @@ class OutputPublisherStagingTest { /** * Makes `cacheDir/conversions` a regular file, which is the whole precondition of the test - * above -- and does it in a loop, because a single delete-then-write loses a race that CI - * caught and this machine does not reproduce. + * above -- and does it in a loop, because a single delete-then-write once lost a race that CI + * caught and this machine did not reproduce. * - * `LibreMediaConverterApp.onCreate` ends with - * `appScope.launch { OutputPublisher(...).sweepStaging() }` on `Dispatchers.IO`, and - * `sweepStaging` reads `stagingDir`, whose getter calls `mkdirs()`. Robolectric instantiates - * the application for every test that asks for one, so that background `mkdirs()` is in flight - * across the whole suite, on a thread the paused main looper does not control. Between deleting - * this path and writing it there is a window where the path does not exist and that `mkdirs()` - * can win, which is `FileNotFoundException: ... (Is a directory)` out of `writeBytes` -- run - * 33069641674 on #149, once, against 468 tests that pass here. + * **That race is closed at the source as of #159, and the loop is kept anyway.** + * `LibreMediaConverterApp.onCreate` launched its staging sweep on `Dispatchers.IO`, and + * `sweepStaging` reads `stagingDir`, whose getter calls `mkdirs()`. Robolectric builds an + * application for every test class that asks for one, so that background `mkdirs()` was in + * flight across the whole suite, on a thread the paused main looper does not control. Between + * deleting this path and writing it there is a window where the path does not exist and that + * `mkdirs()` could win -- `FileNotFoundException: ... (Is a directory)` out of `writeBytes`, + * run 33069641674 on #149, once, against 468 tests that passed here. The JVM suite now runs + * `TestLibreMediaConverterApp`, whose sweep finishes before `onCreate()` returns, so nothing is + * sweeping while a test body runs. * - * Retrying closes it rather than narrowing it, because the race is not symmetric: `mkdirs()` - * fails on an existing regular file, so the invariant only has to survive being *established*. - * Once a write lands, nothing in the suite can turn this back into a directory. - * - * The wider problem -- application-scope IO work racing every Robolectric test that shares - * `cacheDir` -- is #159, and is deliberately not fixed here. + * The loop stays because it is what would catch that substitution being undone. Without it the + * regression returns as this one class failing rarely on CI -- the exact shape that took #159 + * from a single run on #149 to a wave-4 flake before anyone chased it. Retrying closes the + * window rather than narrowing it, because the race is not symmetric: `mkdirs()` fails on an + * existing regular file, so the invariant only has to survive being *established*. */ private fun stagingPathAsRegularFile(): File { val stagingPath = File(cacheDir, "conversions") diff --git a/app/src/test/resources/robolectric.properties b/app/src/test/resources/robolectric.properties index 131b785..6de0950 100644 --- a/app/src/test/resources/robolectric.properties +++ b/app/src/test/resources/robolectric.properties @@ -10,3 +10,9 @@ # Set here rather than in a @Config on each class so a later Robolectric test does not have # to rediscover it. Remove it once Robolectric ships an android-all jar for 37. sdk=36 + +# Every test gets TestLibreMediaConverterApp, whose only difference from the real one is that the +# startup sweep runs inline rather than on Dispatchers.IO. Set suite-wide because the race it fixes +# (#159) is suite-wide: any class that builds an Application leaves a sweep of the shared staging +# directory in flight for whatever runs next. TestLibreMediaConverterApp explains the choice. +application=org.libremediaconverter.TestLibreMediaConverterApp