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