From d59e9acce5e0d12987f923d8e3f110f7870b2415 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Wed, 26 Aug 2026 22:42:46 -0500 Subject: [PATCH 1/3] C6 (#140): OutputPublisher's guarded branches, three of them guarding a delete destinationIsKnownEmpty's three short-circuits -- no SIZE column, no row, a null cell -- each had to answer false and none was tested. Its KDoc is unambiguous about why: "this decides whether a delete is allowed and 'I could not tell' must never authorise one." The existing tests only ever drove a provider that answers properly, where the answer is zero and the delete is correct. Getting the uncertain cases backwards costs the user a file they already had, on a save that failed. Also discardStaged's null parentFile, and sweepStaging's null listing -- which is not the case the existing `tolerates a staging directory that does not exist yet` covers, because stagingDir's own mkdirs() recreates a missing directory and it then lists as empty. Only a path that cannot be a directory makes listFiles() answer null. Five mutations, three bite: drop !row.isNull(size) short-circuit test red drop row.moveToFirst() five tests red parentFile!! instead of ?: return false parentless test red size >= 0 -> size >= -1 GREEN, does not bite parentless treated as staged GREEN -- bad mutation, see below The first green one is recorded in the test as a named exemption. Measured: getColumnIndex returns -1 for an absent column and isNull(-1) throws CursorIndexOutOfBoundsException, which the surrounding runCatching already turns into `?: false`. Same answer, reached by the exception path, so no behavioural test can pin that conjunct. It stays anyway -- control flow through an exception is worse than a comparison, and another Cursor implementation need not throw. The second was my mistake rather than a finding: substituting stagingDir for the null parent reaches `return false` by a different route, so it proves nothing. parentFile!! is the honest mutation and it goes red. :197, :235 and :258 are now covered. What is left in this file is exactly what the ticket scoped out: :173-174 (#142) and :267 (#143). Co-Authored-By: Claude Opus 5 (1M context) --- .../convert/OutputPublisherPublishTest.kt | 49 +++++++++++++++++++ .../convert/OutputPublisherStagingTest.kt | 26 ++++++++++ 2 files changed, 75 insertions(+) diff --git a/app/src/test/java/org/libremediaconverter/convert/OutputPublisherPublishTest.kt b/app/src/test/java/org/libremediaconverter/convert/OutputPublisherPublishTest.kt index c670a67..aa7fd24 100644 --- a/app/src/test/java/org/libremediaconverter/convert/OutputPublisherPublishTest.kt +++ b/app/src/test/java/org/libremediaconverter/convert/OutputPublisherPublishTest.kt @@ -20,6 +20,9 @@ import java.io.OutputStream /** What a destination volume says when it fills up mid-write. */ private const val NO_SPACE = "No space left on device" +/** How far a failing copy gets before the volume "fills up". Any value below the payload does. */ +private const val PARTIAL_BYTES = 512 + /** * A sink that behaves like a volume filling up. * @@ -75,6 +78,8 @@ class OutputPublisherPublishTest { private val payload = ByteArray(8192) { (it % 251).toByte() } + /** How far a failing copy gets before the volume "fills up". Any value below the payload does. */ + private val documentUri: Uri = Uri.parse("content://$DOCUMENTS_AUTHORITY/document/holiday.mp4") private val plainUri: Uri = Uri.parse("content://$PLAIN_AUTHORITY/document/holiday_plain.mp4") private val deadUri: Uri = Uri.parse("content://org.libremediaconverter.nonexistent/document/gone.mp4") @@ -203,6 +208,50 @@ class OutputPublisherPublishTest { assertEquals(emptyList(), FakeSafProvider.deleteRequests) } + @Test + fun `a destination whose size cannot be determined is never deleted`() { + // The three short-circuits in destinationIsKnownEmpty, and the reason its KDoc gives for + // each of them answering false: + // + // "this decides whether a delete is allowed and 'I could not tell' must never authorise + // one." + // + // The contrast is `a copy that fails partway leaves nothing at the destination` above: a + // provider that *does* say zero gets the delete. These say nothing, so they must not. + // Getting this backwards costs the user a file they already had, on a save that failed. + // + // Named exemption: of the three conjuncts, `size >= 0` cannot be falsified behaviourally. + // Measured -- getColumnIndex returns -1 for an absent column, and isNull(-1) throws + // CursorIndexOutOfBoundsException, which the surrounding runCatching already turns into + // `?: false`. So relaxing it to `size >= -1` leaves this test green: same answer, reached + // by the exception path instead. The guard should stay -- control flow through an exception + // is worse than a comparison, and another Cursor implementation need not throw -- but no + // assertion here pins it, and saying so beats implying the missing-column case covers it. + // `!row.isNull(size)` and `row.moveToFirst()` do both bite. + listOf( + RowShape.NO_SIZE_COLUMN to "a cursor with no SIZE column", + RowShape.NULL_SIZE to "a cursor whose SIZE cell is null", + RowShape.NO_ROWS to "a cursor holding no rows", + ).forEach { (shape, description) -> + FakeSafProvider.deleteRequests.clear() + FakeSafProvider.backingFile(documentUri).writeBytes(ByteArray(0)) + FakeSafProvider.rowShape = shape + failMidCopy(documentUri, afterBytes = PARTIAL_BYTES) + + assertThrows(IOException::class.java) { publisher.publish(staged, documentUri) } + + assertEquals( + "$description must not authorise a delete", + emptyList(), + FakeSafProvider.deleteRequests, + ) + assertTrue( + "$description must leave the destination where it was", + FakeSafProvider.backingFile(documentUri).exists(), + ) + } + } + @Test fun `a copy that succeeds delivers every byte and deletes nothing`() { shadowOf(context.contentResolver).registerOutputStreamSupplier(documentUri) { diff --git a/app/src/test/java/org/libremediaconverter/convert/OutputPublisherStagingTest.kt b/app/src/test/java/org/libremediaconverter/convert/OutputPublisherStagingTest.kt index f778f5a..a1d2365 100644 --- a/app/src/test/java/org/libremediaconverter/convert/OutputPublisherStagingTest.kt +++ b/app/src/test/java/org/libremediaconverter/convert/OutputPublisherStagingTest.kt @@ -1,6 +1,7 @@ package org.libremediaconverter.convert import org.junit.Assert.assertFalse +import org.junit.Assert.assertNull import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Test @@ -99,4 +100,29 @@ class OutputPublisherStagingTest { publisher.sweepStaging() } + + @Test + fun `the sweep tolerates a staging path that is not a directory`() { + // The other half of `listFiles() ?: return`, and not the same as the case above: a missing + // directory is created by `stagingDir`'s own mkdirs() and lists as empty. Only a path that + // 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)) + + publisher.sweepStaging() + } + + @Test + fun `discarding a file with no parent at all is refused`() { + // A relative name has no parent directory, so `staged.parentFile` is null. The handle + // reaches the ViewModel as a path string out of WorkInfo.outputData and is turned straight + // into a File, so this is not a shape the caller can rule out -- and the guard has to + // answer false rather than dereference it. + val parentless = File("holiday.mp4") + assertNull("the fixture is supposed to have no parent", parentless.parentFile) + + assertFalse("a file with no parent is not in staging", publisher.discardStaged(parentless)) + } } From c60d5d54c6fe5a22f9725bf36c88a1cbfd40c9c1 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Thu, 27 Aug 2026 07:12:32 -0500 Subject: [PATCH 2/3] 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 + } } From ad47ce6c9613091bae97a0b180fc84799865c99a Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Wed, 26 Aug 2026 22:58:08 -0500 Subject: [PATCH 3/3] S2 + S3 (#142, #143): the two OutputPublisher seams, and where the second one goes #142 -- openOutputStream refuses two ways and only one was reachable. A provider that has gone away throws from inside the call, which `a destination the provider will not open...` already drives. A provider that is present and declines returns null, and nothing could produce that on demand. openDestination is the seam; the test asserts the failure names the destination, which is what separates the `?: error(...)` from an NPE inside `use`. #143 -- the sweep's re-read. **The seam the ticket proposed does not reach it.** Overriding the listing fires before the entries are snapshotted, so StagingSweep.collectable is handed the new timestamp, the file is never proposed for deletion, and the guard is never exercised. Measured: with an entriesIn seam, deleting the guard outright left the test green. The race is a file that *was* collectable when the snapshot was taken and is not by the time the delete comes round, so the seam has to sit at the snapshot. `snapshot(listing)` does, and deleting the guard now reddens the test. Three mutations after the move, three red: null stream returns silently null-return test null stream via !! instead null-return test sweep deletes unconditionally race test OutputPublisher.kt now has no never-executed lines at all. Two partial branches are left and both are named exemptions rather than gaps: L216's `getOrNull() ?: false` and L304's `getOrDefault(absoluteFile)` are the failure arms of a runCatching whose body cannot be made to throw through any public entry point -- the same shape as the `size >= 0` exemption recorded in the previous commit. Co-Authored-By: Claude Opus 5 (1M context) --- .../convert/OutputPublisher.kt | 37 +++++++++++++++- .../convert/OutputPublisherPublishTest.kt | 20 +++++++++ .../convert/OutputPublisherStagingTest.kt | 43 +++++++++++++++++-- 3 files changed, 95 insertions(+), 5 deletions(-) diff --git a/app/src/main/java/org/libremediaconverter/convert/OutputPublisher.kt b/app/src/main/java/org/libremediaconverter/convert/OutputPublisher.kt index 4c8aceb..d532697 100644 --- a/app/src/main/java/org/libremediaconverter/convert/OutputPublisher.kt +++ b/app/src/main/java/org/libremediaconverter/convert/OutputPublisher.kt @@ -5,6 +5,7 @@ import android.net.Uri import android.provider.DocumentsContract import android.provider.OpenableColumns import java.io.File +import java.io.OutputStream /** * What a save has to say when the staged file is not there any more. @@ -170,7 +171,7 @@ open class OutputPublisher(private val context: Context) { open fun publish(staged: File, destination: Uri) { val destinationWasEmpty = destinationIsKnownEmpty(destination) try { - val out = context.contentResolver.openOutputStream(destination) + val out = openDestination(destination) ?: error("Could not open destination for writing: $destination") out.use { sink -> staged.inputStream().use { source -> source.copyTo(sink) } } } catch (failure: Throwable) { @@ -179,6 +180,22 @@ open class OutputPublisher(private val context: Context) { } } + /** + * Opens [destination] for writing, or null when the provider will not. + * + * A seam, and a narrow one: it exists because `openOutputStream` has **two** ways of refusing + * and only one of them is reachable from a test otherwise. A provider that has gone away throws + * `FileNotFoundException` from inside the call; a provider that is present and declines returns + * null. The two are not interchangeable here — the `?: error(...)` above is the only thing that + * turns the second into a failure rather than an NPE further down — and no fake provider can be + * asked to produce a null return on demand. + * + * `protected open` rather than injected, matching `hasSpaceFor` and `createStagingFile`: + * `WorkerStubs.kt`'s publishers already override one method to force one condition. + */ + protected open fun openDestination(destination: Uri): OutputStream? = + context.contentResolver.openOutputStream(destination) + /** * True only when the destination is *positively known* to hold no bytes yet. * @@ -256,7 +273,7 @@ open class OutputPublisher(private val context: Context) { open fun sweepStaging(nowMs: Long = System.currentTimeMillis()) { val dir = stagingDir val listing = dir.listFiles() ?: return - val entries = listing.map { StagingSweep.Entry(it.name, it.lastModified()) } + val entries = snapshot(listing) StagingSweep.collectable(entries, nowMs).forEach { name -> val file = File(dir, name) // Re-read the timestamp rather than trusting the snapshot above. Between the @@ -268,6 +285,22 @@ open class OutputPublisher(private val context: Context) { } } + /** + * The name and age of everything [sweepStaging] found, read once. + * + * A seam for the *race*, not for the clock — [sweepStaging] already takes `nowMs`, so the clock + * is the caller's. What has no seam otherwise is the window between this snapshot and the + * per-file re-read below it, and that window is the entire reason the re-read exists. + * + * **It has to be here and not around `listFiles()`.** A test that changes a file before the + * listing, or during it, changes what `StagingSweep.collectable` is given — so the file is + * never proposed for deletion and the re-read is never reached. The race being modelled is a + * file that *was* collectable when the snapshot was taken and is not by the time the delete + * comes round, which is exactly one worker resuming in this same process. + */ + protected open fun snapshot(listing: Array): List = + listing.map { StagingSweep.Entry(it.name, it.lastModified()) } + private fun File.canonicalOrAbsolute(): File = runCatching { canonicalFile }.getOrDefault(absoluteFile) private companion object { diff --git a/app/src/test/java/org/libremediaconverter/convert/OutputPublisherPublishTest.kt b/app/src/test/java/org/libremediaconverter/convert/OutputPublisherPublishTest.kt index aa7fd24..65ec5d6 100644 --- a/app/src/test/java/org/libremediaconverter/convert/OutputPublisherPublishTest.kt +++ b/app/src/test/java/org/libremediaconverter/convert/OutputPublisherPublishTest.kt @@ -252,6 +252,26 @@ class OutputPublisherPublishTest { } } + @Test + fun `a provider that declines by returning null fails with the destination named`() { + // openOutputStream has two ways of refusing, and only one of them is otherwise reachable. + // `a destination the provider will not open...` above drives the throwing one -- a provider + // that has gone away. This is the other: a provider that is present, answers, and hands + // back null. Without the `?: error(...)` that becomes an NPE inside `use`, which reaches + // the user as "Conversion failed." with a null message. + val nullOpening = object : OutputPublisher(context) { + override fun openDestination(destination: Uri): OutputStream? = null + } + + val failure = runCatching { nullOpening.publish(staged, documentUri) }.exceptionOrNull() + + assertTrue("a null stream must not appear to succeed, got $failure", failure != null) + assertTrue( + "the failure must name the destination rather than being a bare NPE; got ${failure?.message}", + failure?.message?.contains("Could not open destination for writing") == true, + ) + } + @Test fun `a copy that succeeds delivers every byte and deletes nothing`() { shadowOf(context.contentResolver).registerOutputStreamSupplier(documentUri) { diff --git a/app/src/test/java/org/libremediaconverter/convert/OutputPublisherStagingTest.kt b/app/src/test/java/org/libremediaconverter/convert/OutputPublisherStagingTest.kt index 8c39046..c70b965 100644 --- a/app/src/test/java/org/libremediaconverter/convert/OutputPublisherStagingTest.kt +++ b/app/src/test/java/org/libremediaconverter/convert/OutputPublisherStagingTest.kt @@ -1,5 +1,6 @@ package org.libremediaconverter.convert +import android.app.Application import org.junit.Assert.assertFalse import org.junit.Assert.assertNull import org.junit.Assert.assertTrue @@ -27,14 +28,18 @@ import java.util.UUID @RunWith(RobolectricTestRunner::class) class OutputPublisherStagingTest { + private lateinit var app: Application private lateinit var cacheDir: File private lateinit var publisher: OutputPublisher @Before fun setUp() { - val context = RuntimeEnvironment.getApplication() - cacheDir = context.cacheDir - publisher = OutputPublisher(context) + // Held as a field rather than a local: the race test below builds an anonymous + // OutputPublisher, and inside that `object` expression a bare `context` resolves to the + // superclass's own constructor property, which is not initialised at the super call. + app = RuntimeEnvironment.getApplication() + cacheDir = app.cacheDir + publisher = OutputPublisher(app) } @Test @@ -150,6 +155,38 @@ class OutputPublisherStagingTest { return stagingPath } + @Test + fun `a file that stops being collectable between the listing and the delete survives`() { + // The race the second timestamp read exists for, and the only branch of it that had never + // run. The comment in sweepStaging states the cost precisely: a worker resumed by + // WorkManager -- in this same process -- could have started writing this very file, and + // unlinking an inode a running job still holds open ends with the job reporting success for + // a path that no longer exists. + // + // So: a file old enough to collect at listing time, touched to now before the delete is + // reached. StagingSweep.collectable already said yes; isCollectable has to say no. + val orphan = publisher.createStagingFile( + StagingNames.forJob(UUID.randomUUID(), "mp4"), + ).apply { writeBytes(ByteArray(4096)) } + assertTrue(orphan.setLastModified(System.currentTimeMillis() - StagingSweep.GRACE_PERIOD_MS - 60_000)) + + // Touched *after* the snapshot is taken, which is the only window that reaches the + // re-read. Doing it around listFiles() instead changes what StagingSweep.collectable is + // given, so the file is never proposed for deletion and the guard is never exercised -- + // measured, and the reason the seam sits where it does. + val racing = object : OutputPublisher(app) { + override fun snapshot(listing: Array): List = + super.snapshot(listing).also { orphan.setLastModified(System.currentTimeMillis()) } + } + + racing.sweepStaging() + + assertTrue( + "a file a live job started writing after the listing must not be unlinked", + orphan.exists(), + ) + } + @Test fun `discarding a file with no parent at all is refused`() { // A relative name has no parent directory, so `staged.parentFile` is null. The handle