Clean up the empty document a refused open leaves, and stop the space sums wrapping
publish() opened the destination stream outside its guarded region, justified by "nothing has been written at that point, so there is nothing of ours to remove". That reasoning is wrong about what exists: SAF's CreateDocument contract creates the document before publish() is ever called -- which is why every fixture in OutputPublisherPublishTest starts as an existing empty file. A provider that then hands out no stream, because it dropped between the picker and the write or simply returns null, left a zero-byte file at the name the user chose while the screen said the save had failed. The open moves inside the try, so the same two bounds that already govern a failed copy govern this: only a document URI, and only a destination positively known to be empty. The dead-provider case is untouched and now demonstrably by the guard rather than by the placement -- nothing answers for that authority, so no size can be read, and "I could not tell" still refuses to authorise a delete. Its test comment said the old thing and now says that one. The space arithmetic overflows in two places, both live on main and independent of the allocatable-versus-usable question that stays parked: - hasSpaceFor computed `free > required + headroom`. A request within 128 MiB of Long.MAX_VALUE wraps that sum negative, and every free-space measurement beats a negative number, so the check answers "plenty of room" to the largest request it can be handed. Rewritten as `free - headroom > required` with both operands clamped at zero, which is the form the parked branch's StagingSpace.hasRoomFor already argues for. - InputQuery.total folded a join's inputs with nothing stopping the sum from wrapping, and that is the reachable half: no single file overflows, three four-exabyte inputs do. It saturates at Long.MAX_VALUE now, which the check above then refuses. SpaceArithmeticTest ties the two together in the shape the defect had -- the total that came out negative is handed straight to the space check -- and keeps one allowed case so the refusals cannot pass by refusing everything. The negative-size clamp is deliberately left unasserted, with a comment saying why: it only changes the answer when free space is below the headroom, which a test reading the host's real cache volume cannot arrange. R6 / #15, R23 / #32 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -64,9 +64,18 @@ object InputQuery {
|
||||
* A join's total is only as good as its worst-known part. Adding up the ones that answered
|
||||
* would produce a lower bound that reads exactly like a real total, and the space check has
|
||||
* no way to tell the two apart — which is the same conflation this whole file exists to end.
|
||||
*
|
||||
* The sum saturates rather than wrapping. Sizes reach here non-negative — both of the ways one
|
||||
* is found reject a negative answer — but nothing bounds their *sum*, and a total that wrapped
|
||||
* negative would not be harmless nonsense: `OutputPublisher.hasSpaceFor` compares it against
|
||||
* free space, so the largest join representable would come back as the one with the most room.
|
||||
*/
|
||||
fun total(sizes: List<Long?>): Long? = sizes.fold(0L as Long?) { running, size ->
|
||||
if (running == null || size == null) null else running + size
|
||||
when {
|
||||
running == null || size == null -> null
|
||||
size > Long.MAX_VALUE - running -> Long.MAX_VALUE
|
||||
else -> running + size
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -67,8 +67,18 @@ open class OutputPublisher(private val context: Context) {
|
||||
* through its engine, with a message of its own.
|
||||
*
|
||||
* Open so a test can force a full disk; see `FakeFailures` in the instrumented source set.
|
||||
*
|
||||
* Written as `free - headroom > required` rather than the equivalent-looking
|
||||
* `free > required + headroom`. The second overflows: a request within 128 MiB of
|
||||
* [Long.MAX_VALUE] wraps the sum negative, every free-space measurement beats a negative
|
||||
* number, and the check answers "plenty of room" to the largest request it can be given. That
|
||||
* is reachable rather than theoretical — [InputQuery.total] sums a join's inputs, so the number
|
||||
* arriving here is not bounded by any single file. Both operands are clamped at zero first, so
|
||||
* the subtraction cannot underflow and a nonsense negative size decides exactly as zero does
|
||||
* instead of buying slack.
|
||||
*/
|
||||
open fun hasSpaceFor(bytes: Long): Boolean = stagingDir.usableSpace > bytes + SPACE_HEADROOM_BYTES
|
||||
open fun hasSpaceFor(bytes: Long): Boolean =
|
||||
stagingDir.usableSpace.coerceAtLeast(0L) - SPACE_HEADROOM_BYTES > bytes.coerceAtLeast(0L)
|
||||
|
||||
/**
|
||||
* The same check for a job whose input size nobody could determine — see [InputQuery].
|
||||
@@ -128,14 +138,22 @@ open class OutputPublisher(private val context: Context) {
|
||||
* 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.
|
||||
* `openOutputStream` is inside the guard as well, and the reasoning that used to keep it
|
||||
* out -- "nothing has been written at that point, so there is nothing of ours to remove" --
|
||||
* was wrong about what exists. SAF's `CreateDocument` contract creates the document *before*
|
||||
* this is called, which is why every fixture in `OutputPublisherPublishTest` starts as an
|
||||
* existing empty file. So a provider that hands out no stream at all -- gone between the
|
||||
* picker and the write, or simply returning null -- left a zero-byte file at the name the
|
||||
* user chose while the UI said "Could not save the file". The two bounds above are what make
|
||||
* removing it safe, and they apply to this case exactly as they do to a failed copy. A
|
||||
* provider that will not open its own empty document may well refuse to delete it too, which
|
||||
* is already [deletePartialOutput]'s documented no-op path.
|
||||
*/
|
||||
open fun publish(staged: File, destination: Uri) {
|
||||
val destinationWasEmpty = destinationIsKnownEmpty(destination)
|
||||
val out = context.contentResolver.openOutputStream(destination)
|
||||
?: error("Could not open destination for writing: $destination")
|
||||
try {
|
||||
val out = context.contentResolver.openOutputStream(destination)
|
||||
?: error("Could not open destination for writing: $destination")
|
||||
out.use { sink -> staged.inputStream().use { source -> source.copyTo(sink) } }
|
||||
} catch (failure: Throwable) {
|
||||
if (destinationWasEmpty) deletePartialOutput(destination, failure)
|
||||
|
||||
@@ -265,10 +265,34 @@ class OutputPublisherPublishTest {
|
||||
)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `a destination the provider will not open does not stay behind as an empty file`() {
|
||||
// No stream supplier is registered for this URI and the fake provider does not implement
|
||||
// openFile, which is a provider that has gone away between the picker and the write.
|
||||
//
|
||||
// The document exists all the same: SAF's CreateDocument contract created it before
|
||||
// publish() was ever called, so "nothing has been written yet" was never the same claim as
|
||||
// "there is nothing of ours here". Leaving it means a zero-byte file at the name the user
|
||||
// chose, while the screen says the save failed.
|
||||
val destination = FakeSafProvider.backingFile(documentUri)
|
||||
assertEquals("the fixture starts as the empty document SAF hands back", 0L, destination.length())
|
||||
|
||||
val failure = runCatching { publisher.publish(staged, documentUri) }.exceptionOrNull()
|
||||
|
||||
assertTrue("a destination that will not open must not appear to succeed, got $failure", failure != null)
|
||||
assertEquals(listOf(documentUri), FakeSafProvider.deleteRequests)
|
||||
assertFalse(
|
||||
"a zero-byte file must not be left at the name the user picked",
|
||||
destination.exists(),
|
||||
)
|
||||
}
|
||||
|
||||
@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.
|
||||
// The JVM twin of UnopenableUriTest's unwritable-destination case. The open sits inside
|
||||
// the guarded region now, so what keeps this one untouched is the guard rather than the
|
||||
// placement: nothing answers for that authority, so no size can be read, and "I could not
|
||||
// tell" must never authorise a delete.
|
||||
val failure = runCatching { publisher.publish(staged, deadUri) }.exceptionOrNull()
|
||||
|
||||
assertTrue("publishing to a dead provider must not appear to succeed, got $failure", failure != null)
|
||||
|
||||
@@ -0,0 +1,79 @@
|
||||
package org.libremediaconverter.convert
|
||||
|
||||
import android.content.Context
|
||||
import org.junit.Assert.assertEquals
|
||||
import org.junit.Assert.assertFalse
|
||||
import org.junit.Assert.assertTrue
|
||||
import org.junit.Before
|
||||
import org.junit.Test
|
||||
import org.junit.runner.RunWith
|
||||
import org.robolectric.RobolectricTestRunner
|
||||
import org.robolectric.RuntimeEnvironment
|
||||
import java.io.File
|
||||
|
||||
/**
|
||||
* The two sums the space check is made of, at the sizes where addition stops working.
|
||||
*
|
||||
* `SpaceCheckTest` pins which *question* each worker asks; this pins what the answer is once the
|
||||
* number is large. Both halves were live on main: `hasSpaceFor` added the headroom to the request
|
||||
* before comparing, and [InputQuery.total] folded a join's inputs with nothing stopping the sum
|
||||
* from wrapping. A wrapped total is not merely nonsense — it is negative, and every free-space
|
||||
* measurement beats a negative number, so the check that exists to refuse impossible jobs approved
|
||||
* the most impossible one it can be handed.
|
||||
*
|
||||
* Nothing here is about the *allocatable-versus-usable* question, which is a separate decision
|
||||
* still parked. This is the arithmetic on whichever number that decision ends up producing.
|
||||
*/
|
||||
@RunWith(RobolectricTestRunner::class)
|
||||
class SpaceArithmeticTest {
|
||||
|
||||
private lateinit var context: Context
|
||||
private lateinit var publisher: OutputPublisher
|
||||
|
||||
@Before
|
||||
fun setUp() {
|
||||
context = RuntimeEnvironment.getApplication()
|
||||
publisher = OutputPublisher(context)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `a request no disk could hold is refused rather than wrapping into plenty of room`() {
|
||||
assertFalse("eight exabytes do not fit anywhere", publisher.hasSpaceFor(Long.MAX_VALUE))
|
||||
// Just inside the headroom of the maximum, which is the arithmetic's actual edge: this is
|
||||
// the range where `bytes + headroom` goes negative while `bytes` alone still looks huge.
|
||||
assertFalse(publisher.hasSpaceFor(Long.MAX_VALUE - ONE_HUNDRED_MIB))
|
||||
}
|
||||
|
||||
// The clamp on a negative size is deliberately NOT asserted here. It only changes the answer
|
||||
// when free space is below the headroom, which this test cannot arrange -- the publisher reads
|
||||
// the host's real cache volume -- so any assertion available would pass against the unclamped
|
||||
// arithmetic too, and a test that cannot fail is worse than the gap it appears to close.
|
||||
|
||||
@Test
|
||||
fun `an ordinary request is still allowed, so the refusals above are not vacuous`() {
|
||||
val free = File(context.cacheDir, "conversions").usableSpace
|
||||
assertTrue(
|
||||
"a one-byte conversion must fit; the volume under the cache reports $free bytes free",
|
||||
publisher.hasSpaceFor(1L),
|
||||
)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `a join total too large to represent saturates instead of turning negative`() {
|
||||
val enormous = listOf(FOUR_EXABYTES, FOUR_EXABYTES, FOUR_EXABYTES)
|
||||
|
||||
val total = InputQuery.total(enormous)
|
||||
|
||||
assertEquals(Long.MAX_VALUE, total)
|
||||
// The whole point, in the shape the defect had: this total is handed straight to the space
|
||||
// check by ConcatWorker, and before the clamp it arrived negative and was approved.
|
||||
assertFalse("a join of three four-exabyte files does not fit", publisher.hasSpaceFor(total!!))
|
||||
}
|
||||
|
||||
private companion object {
|
||||
const val ONE_HUNDRED_MIB = 100L * 1024 * 1024
|
||||
|
||||
/** Big enough that three of them overflow, small enough to be a plausible `statSize`. */
|
||||
const val FOUR_EXABYTES = 4_000_000_000_000_000_000L
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user