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