Merge pull request #145 from JMR-dev/test/readspec-enum-fallbacks

C1: pin readSpec's three enum fallbacks

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<File>): List<StagingSweep.Entry> =
+        listing.map { StagingSweep.Entry(it.name, it.lastModified()) }
+
     private fun File.canonicalOrAbsolute(): File = runCatching { canonicalFile }.getOrDefault(absoluteFile)
 
     private companion object {

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,70 @@ class OutputPublisherPublishTest {
         assertEquals(emptyList<Uri>(), 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<Uri>(),
+                FakeSafProvider.deleteRequests,
+            )
+            assertTrue(
+                "$description must leave the destination where it was",
+                FakeSafProvider.backingFile(documentUri).exists(),
+            )
+        }
+    }
+
+    @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) {

app/src/test/java/org/libremediaconverter/convert/OutputPublisherStagingTest.kt

@@ -1,6 +1,8 @@
 package org.libremediaconverter.convert
 
+import android.app.Application
 import org.junit.Assert.assertFalse
+import org.junit.Assert.assertNull
 import org.junit.Assert.assertTrue
 import org.junit.Before
 import org.junit.Test
@@ -26,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
@@ -99,4 +105,103 @@ 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.
+        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
+    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<File>): List<StagingSweep.Entry> =
+                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
+        // 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))
+    }
+
+    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
+    }
 }

app/src/test/java/org/libremediaconverter/work/WorkerEnumFallbackTest.kt

@@ -22,6 +22,7 @@ import org.libremediaconverter.model.DeviceCodecs
 import org.libremediaconverter.model.EnginePreference
 import org.libremediaconverter.model.InputProbe
 import org.libremediaconverter.model.OutputFormat
+import org.libremediaconverter.model.OutputSpec
 import org.libremediaconverter.model.QualityTier
 import org.robolectric.RobolectricTestRunner
 import org.robolectric.RuntimeEnvironment
@@ -109,6 +110,50 @@ class WorkerEnumFallbackTest {
         )
     }
 
+    @Test
+    fun `a container this build does not define falls back to the default spec`() {
+        assertFallsBackToDefault(container = "HOLOTAPE")
+    }
+
+    @Test
+    fun `a video codec this build does not define falls back to the default spec`() {
+        assertFallsBackToDefault(video = "H267")
+    }
+
+    @Test
+    fun `an audio codec this build does not define falls back to the default spec`() {
+        assertFallsBackToDefault(audio = "SUPER_AAC")
+    }
+
+    /**
+     * Drives a job whose spec is [NOT_THE_FALLBACK] on every axis but the one named, and asserts the
+     * whole spec came back as [DEFAULT_SPEC].
+     *
+     * **The baseline is the point.** `readSpec` returns the *entire* fallback spec the moment any
+     * one axis fails to resolve, so a test starting from `MP4_H265` -- which is itself the fallback
+     * -- could not tell a worker that read the spec correctly from one that gave up on it. Starting
+     * from MKV/H.264 makes the difference visible on two axes at once.
+     *
+     * Asserting the spec that *ran*, rather than only that a `Result` came back, is the other half:
+     * the defect these three are written for threw out of `doWork` entirely, so "a Result at all"
+     * would pass against a fallback to something arbitrary.
+     */
+    private fun assertFallsBackToDefault(
+        container: String = NOT_THE_FALLBACK.container.name,
+        video: String = NOT_THE_FALLBACK.videoCodec.name,
+        audio: String = NOT_THE_FALLBACK.audioCodec.name,
+    ) {
+        val transcoder = RequestRecordingTranscoder()
+        ConversionDependencies.software = { transcoder }
+
+        val result = runBlocking {
+            conversionWorker(container = container, video = video, audio = audio).doWork()
+        }
+
+        assertEquals(ListenableWorker.Result.success(), stripOutput(result))
+        assertEquals(listOf(DEFAULT_SPEC), transcoder.specs)
+    }
+
     /** [ListenableWorker.Result.Success] compares its output data, which these tests do not pin. */
     private fun stripOutput(result: ListenableWorker.Result): ListenableWorker.Result =
         if (result is ListenableWorker.Result.Success) ListenableWorker.Result.success() else result
@@ -116,15 +161,18 @@ class WorkerEnumFallbackTest {
     private fun conversionWorker(
         quality: String = QualityTier.FAST.name,
         preference: String = EnginePreference.FORCE_SOFTWARE.name,
+        container: String = SPEC.container.name,
+        video: String = SPEC.videoCodec.name,
+        audio: String = SPEC.audioCodec.name,
     ): ConversionWorker = TestListenableWorkerBuilder<ConversionWorker>(
         context = app,
         inputData = workDataOf(
             ConversionWorker.KEY_INPUT_URI to INPUT.toString(),
             ConversionWorker.KEY_DISPLAY_NAME to DISPLAY_NAME,
             ConversionWorker.KEY_SIZE_BYTES to INPUT_BYTES,
-            ConversionWorker.KEY_CONTAINER to SPEC.container.name,
-            ConversionWorker.KEY_VIDEO_CODEC to SPEC.videoCodec.name,
-            ConversionWorker.KEY_AUDIO_CODEC to SPEC.audioCodec.name,
+            ConversionWorker.KEY_CONTAINER to container,
+            ConversionWorker.KEY_VIDEO_CODEC to video,
+            ConversionWorker.KEY_AUDIO_CODEC to audio,
             ConversionWorker.KEY_QUALITY to quality,
             ConversionWorker.KEY_ENGINE_PREFERENCE to preference,
         ),
@@ -146,6 +194,12 @@ class WorkerEnumFallbackTest {
         const val DISPLAY_NAME = "holiday.mp4"
         const val INPUT_BYTES = 1024L
         val SPEC = OutputFormat.MP4_H265.spec
+
+        /** What `readSpec` returns when any axis fails to resolve. */
+        val DEFAULT_SPEC = OutputFormat.MP4_H265.spec
+
+        /** A spec that differs from [DEFAULT_SPEC] on container *and* video codec. See the helper. */
+        val NOT_THE_FALLBACK = OutputFormat.MKV_H264.spec
         val CONVERSION_ID: UUID = UUID.fromString("00000000-0000-4000-8000-000000000021")
         val CONCAT_ID: UUID = UUID.fromString("00000000-0000-4000-8000-000000000022")
     }
@@ -156,6 +210,9 @@ private class RequestRecordingTranscoder : SoftwareTranscoder {
 
     val qualities = mutableListOf<QualityTier>()
 
+    /** The spec each run was asked for. Which one ran is what the three readSpec tests assert. */
+    val specs = mutableListOf<OutputSpec>()
+
     override suspend fun run(
         request: ConversionRequest,
         inputPath: String,
@@ -164,6 +221,7 @@ private class RequestRecordingTranscoder : SoftwareTranscoder {
         onProgress: (Int) -> Unit,
     ) {
         qualities += request.quality
+        specs += request.spec
         output.writeBytes(ByteArray(OUTPUT_BYTES))
     }