diff --git a/app/src/main/java/org/libremediaconverter/convert/OutputPublisher.kt b/app/src/main/java/org/libremediaconverter/convert/OutputPublisher.kt index 59a847b..f97d2e3 100644 --- a/app/src/main/java/org/libremediaconverter/convert/OutputPublisher.kt +++ b/app/src/main/java/org/libremediaconverter/convert/OutputPublisher.kt @@ -2,6 +2,8 @@ package org.libremediaconverter.convert import android.content.Context import android.net.Uri +import android.provider.DocumentsContract +import android.provider.OpenableColumns import java.io.File /** @@ -34,11 +36,91 @@ open class OutputPublisher(private val context: Context) { */ open fun hasSpaceFor(bytes: Long): Boolean = stagingDir.usableSpace > bytes + SPACE_HEADROOM_BYTES - /** Copies a finished staging file into a user-chosen SAF destination. */ + /** + * Copies a finished staging file into a user-chosen SAF destination. + * + * A copy that fails partway -- the destination volume filling up is the obvious one, a + * provider giving out mid-write the other -- used to leave the bytes it had managed at + * the name the user picked, while the UI said "Could not save the file". The user was + * then holding a truncated file they had been told was never written, and nothing in the + * app would ever tidy it up: staging cleanup only reaches [stagingDir], never the + * destination. + * + * So a failed copy deletes the document. Three things bound that, because deleting a + * file the user already had would be a far worse defect than the one being fixed: + * + * - **Only a document URI.** `DocumentsContract.deleteDocument` is the only delete this + * has any right to attempt, and it is defined on document URIs. Anything else -- a + * `file://` path, a MediaStore item, a content URI from a provider that is not a + * documents provider -- is left exactly as it is. + * - **Only a destination that was empty when we started.** The size is read before the + * stream is opened, and the delete only runs if the answer was positively zero. Every + * destination reaching here comes from the SAF `CreateDocument` contract, so in + * practice it is a document this app just created; but `publish` cannot verify that + * from a `Uri`, and a provider that hands back an existing document for a name the + * user re-picked would otherwise have its file deleted rather than merely truncated. + * A provider that reports no size at all falls into the same "not known to be empty" + * bucket, so the fix is conservative rather than universal: it will not clean up + * behind such a provider, and it will not delete anything of theirs either. + * - **The original failure is what the caller sees.** Cleanup runs inside its own + * `runCatching`; if it throws, that goes on the original exception as a suppressed + * one. `save()` reports `e.message`, and "could not delete the half-written file" is + * not the thing to tell someone whose disk just filled up. + * + * The whole `use` is guarded, not just the copy: a `close()` that throws while flushing + * IS the disk-full case, and it arrives after `copyTo` has returned. The cost is that a + * file whose every byte reached the provider before a failing flush is deleted too -- + * which is the right way round, since a flush that failed means the bytes are not + * durably there to begin with. + * + * A failure from `openOutputStream` itself is deliberately outside the guard. Nothing + * has been written at that point, so there is nothing of ours to remove. + */ open fun publish(staged: File, destination: Uri) { - context.contentResolver.openOutputStream(destination)?.use { out -> - staged.inputStream().use { it.copyTo(out) } - } ?: error("Could not open destination for writing: $destination") + val destinationWasEmpty = destinationIsKnownEmpty(destination) + val out = context.contentResolver.openOutputStream(destination) + ?: error("Could not open destination for writing: $destination") + try { + out.use { sink -> staged.inputStream().use { source -> source.copyTo(sink) } } + } catch (failure: Throwable) { + if (destinationWasEmpty) deletePartialOutput(destination, failure) + throw failure + } + } + + /** + * True only when the destination is *positively known* to hold no bytes yet. + * + * Every other answer -- a provider that does not report `_size`, a query that returns no + * row, a resolver call that throws -- is false, because this decides whether a delete is + * allowed and "I could not tell" must never authorise one. + * + * The column is looked up by name rather than taken as index 0: a projection is a + * request, not a guarantee, and a provider is free to return its own column set. + */ + private fun destinationIsKnownEmpty(destination: Uri): Boolean = runCatching { + context.contentResolver + .query(destination, arrayOf(OpenableColumns.SIZE), null, null, null) + ?.use { row -> + val size = row.getColumnIndex(OpenableColumns.SIZE) + size >= 0 && row.moveToFirst() && !row.isNull(size) && row.getLong(size) == 0L + } + }.getOrNull() ?: false + + /** + * Removes the half-written document, never at the expense of [cause]. + * + * `deleteDocument` reports its own failure two different ways -- `false`, or a thrown + * `FileNotFoundException` -- and neither is worth failing the save over, because the + * save has already failed. Whatever it does, [cause] is what propagates; a thrown + * cleanup failure is attached to it so it is not simply lost. + */ + private fun deletePartialOutput(destination: Uri, cause: Throwable) { + runCatching { + if (DocumentsContract.isDocumentUri(context, destination)) { + DocumentsContract.deleteDocument(context.contentResolver, destination) + } + }.onFailure(cause::addSuppressed) } /** diff --git a/app/src/test/java/org/libremediaconverter/convert/OutputPublisherPublishTest.kt b/app/src/test/java/org/libremediaconverter/convert/OutputPublisherPublishTest.kt new file mode 100644 index 0000000..09f1adc --- /dev/null +++ b/app/src/test/java/org/libremediaconverter/convert/OutputPublisherPublishTest.kt @@ -0,0 +1,329 @@ +package org.libremediaconverter.convert + +import android.content.ComponentName +import android.content.ContentProvider +import android.content.ContentValues +import android.content.Context +import android.content.IntentFilter +import android.content.pm.ProviderInfo +import android.database.Cursor +import android.database.MatrixCursor +import android.net.Uri +import android.os.Bundle +import android.provider.DocumentsContract +import android.provider.OpenableColumns +import org.junit.Assert.assertArrayEquals +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertThrows +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.Robolectric +import org.robolectric.RobolectricTestRunner +import org.robolectric.RuntimeEnvironment +import org.robolectric.Shadows.shadowOf +import java.io.File +import java.io.IOException +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" + +private const val DOCUMENTS_AUTHORITY = "org.libremediaconverter.test.documents" +private const val PLAIN_AUTHORITY = "org.libremediaconverter.test.plain" + +/** + * A stand-in for the provider behind a SAF destination. + * + * It answers only what `publish()` asks of a destination -- how many bytes are already there, + * and delete it -- backed by a real file so the assertions are about the filesystem rather + * than about a mock's call log alone. The rest of the `ContentProvider` surface is stubbed. + * + * Writing is deliberately NOT routed through it. Robolectric's `ShadowContentResolver` + * consults its registered-stream map before it reaches any provider, which is what lets a + * test hand out a stream that writes some bytes and then fails -- the condition this whole + * file exists for, and one a real provider cannot be asked to produce on demand. + */ +internal open class FakeSafProvider : ContentProvider() { + + override fun onCreate() = true + + override fun query( + uri: Uri, + projection: Array?, + selection: String?, + selectionArgs: Array?, + sortOrder: String?, + ): Cursor? { + val file = backingFile(uri) + if (!file.exists()) return null + return MatrixCursor(arrayOf(OpenableColumns.DISPLAY_NAME, OpenableColumns.SIZE)).apply { + addRow(arrayOf(file.name, file.length())) + } + } + + override fun call(method: String, arg: String?, extras: Bundle?): Bundle? { + if (method != METHOD_DELETE_DOCUMENT) return null + val target = extras?.getParcelable(EXTRA_URI, Uri::class.java) ?: return null + deleteRequests += target + deleteFailure?.let { throw it } + backingFile(target).delete() + return Bundle() + } + + override fun getType(uri: Uri) = "video/mp4" + + override fun insert(uri: Uri, values: ContentValues?): Uri? = null + + override fun delete(uri: Uri, selection: String?, selectionArgs: Array?) = 0 + + override fun update(uri: Uri, values: ContentValues?, selection: String?, selectionArgs: Array?) = 0 + + companion object { + // DocumentsContract.METHOD_DELETE_DOCUMENT and EXTRA_URI are hidden from the public + // SDK, so they cannot be referenced. These are the wire names + // DocumentsContract.deleteDocument() actually sends, which is what a provider sees. + const val METHOD_DELETE_DOCUMENT = "android:deleteDocument" + const val EXTRA_URI = "uri" + + /** Where the "documents" really live. Set per test to a Robolectric temp path. */ + lateinit var root: File + + /** Every delete this provider was asked for, in order. Empty is an assertion too. */ + val deleteRequests = mutableListOf() + + /** Armed by the test that needs the cleanup itself to fail. */ + var deleteFailure: RuntimeException? = null + + fun backingFile(uri: Uri) = File(root, uri.lastPathSegment.orEmpty()) + + fun reset(directory: File) { + root = directory + deleteRequests.clear() + deleteFailure = null + } + } +} + +/** + * The same provider, registered WITHOUT the documents-provider intent filter. + * + * A separate class because the package manager keys providers by component name, so two + * authorities need two components. It exists to prove the guard is a guard: a content URI + * from something that is not a documents provider must not be handed to `deleteDocument`. + */ +internal class FakePlainProvider : FakeSafProvider() + +/** + * A sink that behaves like a volume filling up. + * + * Two failure shapes, because `publish()` has to survive both: a write that throws partway, + * and a `close()` that throws while flushing -- the second arriving after `copyTo` has + * already returned successfully. + */ +private class UnreliableOutputStream( + private val sink: OutputStream, + private val failAfterBytes: Int = Int.MAX_VALUE, + private val failOnClose: Boolean = false, +) : OutputStream() { + + private var written = 0 + + override fun write(b: Int) = write(byteArrayOf(b.toByte()), 0, 1) + + override fun write(b: ByteArray, off: Int, len: Int) { + val room = failAfterBytes - written + if (room <= 0) throw IOException(NO_SPACE) + val accepted = minOf(room, len) + sink.write(b, off, accepted) + written += accepted + if (accepted < len) throw IOException(NO_SPACE) + } + + override fun flush() = sink.flush() + + override fun close() { + sink.close() + if (failOnClose) throw IOException(NO_SPACE) + } +} + +/** + * `publish()` on the paths where something goes wrong. + * + * The defect: a copy that failed partway left the bytes it had managed at the name the user + * picked, while the UI said "Could not save the file". [OutputPublisherStagingTest] covers + * the staging side of the same class; this covers the destination side, and needs a provider + * rather than a bare file because the destination is a `content://` URI and the fix turns on + * what kind of URI it is. + * + * Every case here is a failure case except one, and that is the point -- these branches never + * run in a healthy test run and are exactly the ones a user meets on a bad day. + */ +@RunWith(RobolectricTestRunner::class) +class OutputPublisherPublishTest { + + private lateinit var context: Context + private lateinit var publisher: OutputPublisher + private lateinit var staged: File + + private val payload = ByteArray(8192) { (it % 251).toByte() } + + 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") + + @Before + fun setUp() { + context = RuntimeEnvironment.getApplication() + FakeSafProvider.reset(File(context.cacheDir, "destinations").apply { mkdirs() }) + register(FakeSafProvider::class.java, DOCUMENTS_AUTHORITY, asDocumentsProvider = true) + register(FakePlainProvider::class.java, PLAIN_AUTHORITY, asDocumentsProvider = false) + + // SAF's CreateDocument contract hands back a document that already exists and is + // empty, so that is the state every destination starts in here. + FakeSafProvider.backingFile(documentUri).writeBytes(ByteArray(0)) + FakeSafProvider.backingFile(plainUri).writeBytes(ByteArray(0)) + + publisher = OutputPublisher(context) + staged = publisher.createStagingFile("holiday.mp4").apply { writeBytes(payload) } + } + + @Test + fun `a copy that fails partway leaves nothing at the destination`() { + failMidCopy(documentUri, afterBytes = 512) + + val failure = assertThrows(IOException::class.java) { publisher.publish(staged, documentUri) } + + assertEquals(NO_SPACE, failure.message) + assertEquals(listOf(documentUri), FakeSafProvider.deleteRequests) + assertFalse( + "a truncated file must not be left at the name the user picked", + FakeSafProvider.backingFile(documentUri).exists(), + ) + } + + @Test + fun `a close that fails while flushing counts as a failed copy`() { + // copyTo() has already returned by the time this throws. Guarding only the copy and + // not the close would leave the file behind on exactly the disk-full case. + shadowOf(context.contentResolver).registerOutputStreamSupplier(documentUri) { + UnreliableOutputStream(FakeSafProvider.backingFile(documentUri).outputStream(), failOnClose = true) + } + + val failure = assertThrows(IOException::class.java) { publisher.publish(staged, documentUri) } + + assertEquals(NO_SPACE, failure.message) + assertFalse( + "bytes that were never flushed are not a saved file", + FakeSafProvider.backingFile(documentUri).exists(), + ) + } + + @Test + fun `a destination that already held bytes is not deleted`() { + // Not the CreateDocument case: a provider that handed back an existing document for + // a name the user re-picked. Truncating it is bad; removing it outright is worse, and + // publish() cannot tell from a Uri that the app created it. + val existing = FakeSafProvider.backingFile(documentUri).apply { writeBytes(ByteArray(4096)) } + failMidCopy(documentUri, afterBytes = 512) + + assertThrows(IOException::class.java) { publisher.publish(staged, documentUri) } + + assertTrue("a document this app did not create must survive", existing.exists()) + assertEquals(emptyList(), FakeSafProvider.deleteRequests) + } + + @Test + fun `a destination that is not a document is left alone`() { + failMidCopy(plainUri, afterBytes = 512) + + assertThrows(IOException::class.java) { publisher.publish(staged, plainUri) } + + assertEquals( + "deleteDocument has no business on a URI that is not a document", + emptyList(), + FakeSafProvider.deleteRequests, + ) + assertTrue(FakeSafProvider.backingFile(plainUri).exists()) + } + + @Test + fun `a cleanup that fails does not replace the failure the user needs to see`() { + FakeSafProvider.deleteFailure = SecurityException("provider refused the delete") + failMidCopy(documentUri, afterBytes = 512) + + val failure = assertThrows(IOException::class.java) { publisher.publish(staged, documentUri) } + + assertEquals("the disk-full failure is what save() reports", NO_SPACE, failure.message) + assertEquals( + "the cleanup failure is attached rather than lost", + listOf("provider refused the delete"), + failure.suppressedExceptions.map { it.message }, + ) + } + + @Test + fun `a destination that cannot be opened at all fails without any cleanup`() { + // The JVM twin of UnopenableUriTest's unwritable-destination case. Nothing was + // written, so there is nothing of ours to remove. + val failure = runCatching { publisher.publish(staged, deadUri) }.exceptionOrNull() + + assertTrue("publishing to a dead provider must not appear to succeed, got $failure", failure != null) + assertEquals(emptyList(), FakeSafProvider.deleteRequests) + } + + @Test + fun `a copy that succeeds delivers every byte and deletes nothing`() { + shadowOf(context.contentResolver).registerOutputStreamSupplier(documentUri) { + FakeSafProvider.backingFile(documentUri).outputStream() + } + + publisher.publish(staged, documentUri) + + assertArrayEquals(payload, FakeSafProvider.backingFile(documentUri).readBytes()) + assertEquals(emptyList(), FakeSafProvider.deleteRequests) + } + + /** + * Arms the destination to accept [afterBytes] and then fail. + * + * A supplier rather than a ready-made stream: opening the backing file truncates it, and + * doing that here would erase the very content the "already held bytes" case is about + * before `publish()` ever got to read its size. + */ + private fun failMidCopy(destination: Uri, afterBytes: Int) { + shadowOf(context.contentResolver).registerOutputStreamSupplier(destination) { + UnreliableOutputStream( + FakeSafProvider.backingFile(destination).outputStream(), + failAfterBytes = afterBytes, + ) + } + } + + private fun register(provider: Class, authority: String, asDocumentsProvider: Boolean) { + val info = ProviderInfo().apply { + this.authority = authority + packageName = context.packageName + name = provider.name + exported = true + grantUriPermissions = true + } + Robolectric.buildContentProvider(provider).create(info) + + // isDocumentUri() does not look at the URI alone: it asks the package manager whether + // anything answers ACTION_DOCUMENTS_PROVIDER for that authority. Registering the + // provider with the resolver is not enough, which is the whole reason the negative + // case above can exist. + val packageManager = shadowOf(context.packageManager) + packageManager.addOrUpdateProvider(info) + if (asDocumentsProvider) { + packageManager.addIntentFilterForProvider( + ComponentName(context.packageName, provider.name), + IntentFilter(DocumentsContract.PROVIDER_INTERFACE), + ) + } + } +}