From c60d5d54c6fe5a22f9725bf36c88a1cbfd40c9c1 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Thu, 27 Aug 2026 07:12:32 -0500 Subject: [PATCH] Stop the staging fixture losing a race with the app-start sweep (#159) `the sweep tolerates a staging path that is not a directory` failed once on run 33069641674, against 468 tests that pass on this machine including under `--rerun-tasks`: java.io.FileNotFoundException at OutputPublisherStagingTest.kt:112 468 tests completed, 1 failed Line 112 was `writeBytes` immediately after `deleteRecursively()`. `FileOutputStream` answers `FileNotFoundException` for an existing directory, so something had recreated the path inside that window. That something is `LibreMediaConverterApp.onCreate`, which ends with appScope.launch { OutputPublisher(...).sweepStaging() } on `Dispatchers.IO`, and `sweepStaging` reads `stagingDir`, whose getter calls `mkdirs()`. Robolectric builds 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 and no test awaits. 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*. Once a write lands, nothing in the suite can turn this path back into a directory -- which is also why the new assertion that the sweep left a file behind is worth making. The `check()` matters as much as the loop. The next failure here should say "something recreated conversions/ as a directory", not `FileNotFoundException at line 112` -- that is the difference between a flake someone reads and a flake someone re-runs. The wider problem is #159 and is deliberately not fixed here: `AppStartSweepTest`, `JobSnapshotsTest` and `SpaceArithmeticTest` all name the same path, and the real answer is an injectable scope rather than a retry loop in every staging test. #159's done-when is that this loop can be deleted. Mutation: `listFiles() ?: return` -> `listFiles()!!` reddens exactly this test. Gate green: assembleDebug, testDebugUnitTest, compileDebugAndroidTestKotlin, ktlintCheck, detekt, lintDebug. Co-Authored-By: Claude Opus 5 (1M context) --- .../convert/OutputPublisherStagingTest.kt | 46 ++++++++++++++++++- 1 file changed, 44 insertions(+), 2 deletions(-) diff --git a/app/src/test/java/org/libremediaconverter/convert/OutputPublisherStagingTest.kt b/app/src/test/java/org/libremediaconverter/convert/OutputPublisherStagingTest.kt index a1d2365..8c39046 100644 --- a/app/src/test/java/org/libremediaconverter/convert/OutputPublisherStagingTest.kt +++ b/app/src/test/java/org/libremediaconverter/convert/OutputPublisherStagingTest.kt @@ -108,10 +108,46 @@ class OutputPublisherStagingTest { // cannot be a directory makes listFiles() answer null, and a sweep that dereferenced that // would take the app down on a launch rather than on a conversion -- AppStartSweepTest is // where this runs from. - File(cacheDir, "conversions").deleteRecursively() - File(cacheDir, "conversions").writeBytes(ByteArray(8)) + val stagingPath = stagingPathAsRegularFile() publisher.sweepStaging() + + assertTrue("the sweep must not have replaced the fixture", stagingPath.isFile) + } + + /** + * 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. + * + * `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. + * + * 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. + */ + private fun stagingPathAsRegularFile(): File { + val stagingPath = File(cacheDir, "conversions") + repeat(FIXTURE_ATTEMPTS) { + if (stagingPath.isFile) return stagingPath + stagingPath.deleteRecursively() + runCatching { stagingPath.writeBytes(ByteArray(FIXTURE_BYTES)) } + } + check(stagingPath.isFile) { + "the fixture needs $stagingPath to be a regular file and it is a directory; " + + "something recreated it $FIXTURE_ATTEMPTS times -- see #159" + } + return stagingPath } @Test @@ -125,4 +161,10 @@ class OutputPublisherStagingTest { assertFalse("a file with no parent is not in staging", publisher.discardStaged(parentless)) } + + private companion object { + /** Enough to outlast a burst of application-scope sweeps; one attempt is what CI lost. */ + const val FIXTURE_ATTEMPTS = 50 + const val FIXTURE_BYTES = 8 + } }