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) <noreply@anthropic.com>
This commit is contained in:
committed by
Jason Ross
co-authored by
Claude Opus 5
parent
d59e9acce5
commit
c60d5d54c6
@@ -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
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user