From a14fb7a82dc388a644f982b9e55c3904df8b6281 Mon Sep 17 00:00:00 2001 From: alperozturk96 Date: Tue, 1 Sep 2026 11:32:02 +0200 Subject: [PATCH 1/3] use more reasonable values for assemble Signed-off-by: alperozturk96 --- .../files/ChunkedFileUploadRemoteOperation.kt | 11 ++++-- .../ChunkedFileUploadRemoteOperationTest.kt | 39 +++++++++++++++---- 2 files changed, 39 insertions(+), 11 deletions(-) diff --git a/library/src/main/java/com/owncloud/android/lib/resources/files/ChunkedFileUploadRemoteOperation.kt b/library/src/main/java/com/owncloud/android/lib/resources/files/ChunkedFileUploadRemoteOperation.kt index 34744a744..9faf31add 100644 --- a/library/src/main/java/com/owncloud/android/lib/resources/files/ChunkedFileUploadRemoteOperation.kt +++ b/library/src/main/java/com/owncloud/android/lib/resources/files/ChunkedFileUploadRemoteOperation.kt @@ -61,7 +61,7 @@ class ChunkedFileUploadRemoteOperation // Assemble timeouts, in milliseconds. The literals are the definition itself, hence the MagicNumber opt-out. @Suppress("MagicNumber") @JvmField - val assembleTimeMin: Int = 30 * 1000 // 30s + val assembleTimeBase: Int = 3 * 60 * 1000 // 3min @Suppress("MagicNumber") @JvmField @@ -69,7 +69,7 @@ class ChunkedFileUploadRemoteOperation @Suppress("MagicNumber") @JvmField - val assembleTimePerGB: Int = 3 * 60 * 1000 // 3 min + val assembleTimePerGB: Int = 3 * 60 * 1000 // 3min private lateinit var uploadFolderUri: String private lateinit var destinationUri: String @@ -299,11 +299,16 @@ class ChunkedFileUploadRemoteOperation } } + /** + * The server answers the assembling MOVE only once every chunk has been merged, so the whole merge has to fit + * into the read timeout. Assembling scales with the total size, not with the number of chunks, and the base + * covers the fixed cost a small file still pays on slow (object) storage. + */ @VisibleForTesting fun calculateAssembleTimeout(file: File): Int { val fileSizeInGb = file.length() / BYTES_PER_GB - return max(assembleTimeMin, min((assembleTimePerGB * fileSizeInGb).toInt(), assembleTimeMax)) + return min(assembleTimeBase + (assembleTimePerGB * fileSizeInGb).toInt(), assembleTimeMax) } private data class UploadedChunks( diff --git a/library/src/test/java/com/owncloud/android/lib/resources/files/ChunkedFileUploadRemoteOperationTest.kt b/library/src/test/java/com/owncloud/android/lib/resources/files/ChunkedFileUploadRemoteOperationTest.kt index 4ce08c368..3304352d2 100644 --- a/library/src/test/java/com/owncloud/android/lib/resources/files/ChunkedFileUploadRemoteOperationTest.kt +++ b/library/src/test/java/com/owncloud/android/lib/resources/files/ChunkedFileUploadRemoteOperationTest.kt @@ -36,31 +36,31 @@ class ChunkedFileUploadRemoteOperationTest { // 0b Mockito.`when`(file.length()).thenReturn(0L) - assertEquals(sut.assembleTimeMin, sut.calculateAssembleTimeout(file)) + assertEquals(sut.assembleTimeBase, sut.calculateAssembleTimeout(file)) // 100b Mockito.`when`(file.length()).thenReturn(100L) - assertEquals(sut.assembleTimeMin, sut.calculateAssembleTimeout(file)) + assertEquals(sut.assembleTimeBase, sut.calculateAssembleTimeout(file)) // 1Mb Mockito.`when`(file.length()).thenReturn(1 * MB) - assertEquals(sut.assembleTimeMin, sut.calculateAssembleTimeout(file)) + assertEquals(sut.assembleTimeBase + sut.assembleTimePerGB / 1000, sut.calculateAssembleTimeout(file)) - // 100Mb + // 100Mb, the size that used to be capped to the flat minimum Mockito.`when`(file.length()).thenReturn(100 * MB) - assertEquals(sut.assembleTimeMin, sut.calculateAssembleTimeout(file)) + assertEquals(sut.assembleTimeBase + sut.assembleTimePerGB / 10, sut.calculateAssembleTimeout(file)) // 1Gb Mockito.`when`(file.length()).thenReturn(1 * GB) - assertEquals(sut.assembleTimePerGB, sut.calculateAssembleTimeout(file)) + assertEquals(sut.assembleTimeBase + sut.assembleTimePerGB, sut.calculateAssembleTimeout(file)) // 2Gb Mockito.`when`(file.length()).thenReturn(2 * GB) - assertEquals((2 * sut.assembleTimePerGB), sut.calculateAssembleTimeout(file)) + assertEquals(sut.assembleTimeBase + 2 * sut.assembleTimePerGB, sut.calculateAssembleTimeout(file)) // 5Gb Mockito.`when`(file.length()).thenReturn(5 * GB) - assertEquals((5 * sut.assembleTimePerGB), sut.calculateAssembleTimeout(file)) + assertEquals(sut.assembleTimeBase + 5 * sut.assembleTimePerGB, sut.calculateAssembleTimeout(file)) // 50Gb Mockito.`when`(file.length()).thenReturn(50 * GB) @@ -71,6 +71,29 @@ class ChunkedFileUploadRemoteOperationTest { assertEquals(sut.assembleTimeMax, sut.calculateAssembleTimeout(file)) } + @Test + fun assembleTimeoutGrowsWithFileSize() { + MockitoAnnotations.openMocks(this) + val sut = + ChunkedFileUploadRemoteOperation( + null, + null, + null, + null, + System.currentTimeMillis() / 1000, + false + ) + + val timeouts = + listOf(1 * MB, 100 * MB, 500 * MB, 1 * GB, 5 * GB).map { length -> + Mockito.`when`(file.length()).thenReturn(length) + sut.calculateAssembleTimeout(file) + } + + assertEquals(timeouts.sorted(), timeouts) + assertEquals(timeouts.distinct().size, timeouts.size) + } + @Test fun testChunkEquality() { val chunk1 = Chunk(0, 0, 5) From 2a8a43533f2973e58bb0670bc467d9c9ea011d26 Mon Sep 17 00:00:00 2001 From: alperozturk96 Date: Tue, 1 Sep 2026 11:37:40 +0200 Subject: [PATCH 2/3] use kotlin.time.Duration Signed-off-by: alperozturk96 --- .../files/ChunkedFileUploadRemoteOperation.kt | 45 ++++++--------- .../ChunkedFileUploadRemoteOperationTest.kt | 56 +++++++++---------- 2 files changed, 46 insertions(+), 55 deletions(-) diff --git a/library/src/main/java/com/owncloud/android/lib/resources/files/ChunkedFileUploadRemoteOperation.kt b/library/src/main/java/com/owncloud/android/lib/resources/files/ChunkedFileUploadRemoteOperation.kt index 9faf31add..8d7ae0468 100644 --- a/library/src/main/java/com/owncloud/android/lib/resources/files/ChunkedFileUploadRemoteOperation.kt +++ b/library/src/main/java/com/owncloud/android/lib/resources/files/ChunkedFileUploadRemoteOperation.kt @@ -32,7 +32,9 @@ import java.io.RandomAccessFile import java.nio.channels.FileChannel import java.util.Locale import kotlin.math.max -import kotlin.math.min +import kotlin.time.Duration +import kotlin.time.Duration.Companion.milliseconds +import kotlin.time.Duration.Companion.minutes @Suppress("LongParameterList") class ChunkedFileUploadRemoteOperation @@ -58,19 +60,6 @@ class ChunkedFileUploadRemoteOperation token, disableRetries ) { - // Assemble timeouts, in milliseconds. The literals are the definition itself, hence the MagicNumber opt-out. - @Suppress("MagicNumber") - @JvmField - val assembleTimeBase: Int = 3 * 60 * 1000 // 3min - - @Suppress("MagicNumber") - @JvmField - val assembleTimeMax: Int = 30 * 60 * 1000 // 30min - - @Suppress("MagicNumber") - @JvmField - val assembleTimePerGB: Int = 3 * 60 * 1000 // 3min - private lateinit var uploadFolderUri: String private lateinit var destinationUri: String private var moveMethod: MoveMethod? = null @@ -215,7 +204,8 @@ class ChunkedFileUploadRemoteOperation creationTimestamp?.takeIf { it > 0 }?.let { move.addRequestHeader(OC_X_OC_CTIME_HEADER, it.toString()) } token?.let { move.addRequestHeader(E2E_TOKEN, it) } - val status = client.executeMethod(move, calculateAssembleTimeout(file), DO_NOT_CHANGE_DEFAULT) + val readTimeout = calculateAssembleTimeout(file).inWholeMilliseconds.toInt() + val status = client.executeMethod(move, readTimeout, DO_NOT_CHANGE_DEFAULT) return RemoteOperationResult(isSuccess(status), move) } @@ -299,24 +289,25 @@ class ChunkedFileUploadRemoteOperation } } - /** - * The server answers the assembling MOVE only once every chunk has been merged, so the whole merge has to fit - * into the read timeout. Assembling scales with the total size, not with the number of chunks, and the base - * covers the fixed cost a small file still pays on slow (object) storage. - */ - @VisibleForTesting - fun calculateAssembleTimeout(file: File): Int { - val fileSizeInGb = file.length() / BYTES_PER_GB - - return min(assembleTimeBase + (assembleTimePerGB * fileSizeInGb).toInt(), assembleTimeMax) - } - private data class UploadedChunks( val nextByte: Long, val lastId: Int ) companion object { + val ASSEMBLE_TIME_BASE: Duration = 3.minutes + + val ASSEMBLE_TIME_PER_GB: Duration = 3.minutes + + val ASSEMBLE_TIME_MAX: Duration = 30.minutes + + @VisibleForTesting + fun calculateAssembleTimeout(file: File): Duration { + val fileSizeInGb = file.length() / BYTES_PER_GB + val timeout = minOf(ASSEMBLE_TIME_BASE + ASSEMBLE_TIME_PER_GB * fileSizeInGb, ASSEMBLE_TIME_MAX) + return timeout.inWholeMilliseconds.milliseconds + } + const val MIN_CHUNK_SIZE: Long = 10240000 const val DEFAULT_CHUNK_SIZE: Long = 40960000 const val SERVER_MAX_CHUNK_SIZE_UNKNOWN: Long = -1 diff --git a/library/src/test/java/com/owncloud/android/lib/resources/files/ChunkedFileUploadRemoteOperationTest.kt b/library/src/test/java/com/owncloud/android/lib/resources/files/ChunkedFileUploadRemoteOperationTest.kt index 3304352d2..b1a5c02ac 100644 --- a/library/src/test/java/com/owncloud/android/lib/resources/files/ChunkedFileUploadRemoteOperationTest.kt +++ b/library/src/test/java/com/owncloud/android/lib/resources/files/ChunkedFileUploadRemoteOperationTest.kt @@ -8,6 +8,9 @@ */ package com.owncloud.android.lib.resources.files +import com.owncloud.android.lib.resources.files.ChunkedFileUploadRemoteOperation.Companion.ASSEMBLE_TIME_BASE +import com.owncloud.android.lib.resources.files.ChunkedFileUploadRemoteOperation.Companion.ASSEMBLE_TIME_MAX +import com.owncloud.android.lib.resources.files.ChunkedFileUploadRemoteOperation.Companion.ASSEMBLE_TIME_PER_GB import org.junit.Assert.assertEquals import org.junit.Assert.assertNotEquals import org.junit.Test @@ -24,70 +27,67 @@ class ChunkedFileUploadRemoteOperationTest { @Test fun testAssembleTimeout() { MockitoAnnotations.openMocks(this) - val sut = - ChunkedFileUploadRemoteOperation( - null, - null, - null, - null, - System.currentTimeMillis() / 1000, - false - ) // 0b Mockito.`when`(file.length()).thenReturn(0L) - assertEquals(sut.assembleTimeBase, sut.calculateAssembleTimeout(file)) + assertEquals(ASSEMBLE_TIME_BASE, ChunkedFileUploadRemoteOperation.calculateAssembleTimeout(file)) // 100b Mockito.`when`(file.length()).thenReturn(100L) - assertEquals(sut.assembleTimeBase, sut.calculateAssembleTimeout(file)) + assertEquals(ASSEMBLE_TIME_BASE, ChunkedFileUploadRemoteOperation.calculateAssembleTimeout(file)) // 1Mb Mockito.`when`(file.length()).thenReturn(1 * MB) - assertEquals(sut.assembleTimeBase + sut.assembleTimePerGB / 1000, sut.calculateAssembleTimeout(file)) + assertEquals( + ASSEMBLE_TIME_BASE + ASSEMBLE_TIME_PER_GB / 1000, + ChunkedFileUploadRemoteOperation.calculateAssembleTimeout(file) + ) // 100Mb, the size that used to be capped to the flat minimum Mockito.`when`(file.length()).thenReturn(100 * MB) - assertEquals(sut.assembleTimeBase + sut.assembleTimePerGB / 10, sut.calculateAssembleTimeout(file)) + assertEquals( + ASSEMBLE_TIME_BASE + ASSEMBLE_TIME_PER_GB / 10, + ChunkedFileUploadRemoteOperation.calculateAssembleTimeout(file) + ) // 1Gb Mockito.`when`(file.length()).thenReturn(1 * GB) - assertEquals(sut.assembleTimeBase + sut.assembleTimePerGB, sut.calculateAssembleTimeout(file)) + assertEquals( + ASSEMBLE_TIME_BASE + ASSEMBLE_TIME_PER_GB, + ChunkedFileUploadRemoteOperation.calculateAssembleTimeout(file) + ) // 2Gb Mockito.`when`(file.length()).thenReturn(2 * GB) - assertEquals(sut.assembleTimeBase + 2 * sut.assembleTimePerGB, sut.calculateAssembleTimeout(file)) + assertEquals( + ASSEMBLE_TIME_BASE + ASSEMBLE_TIME_PER_GB * 2, + ChunkedFileUploadRemoteOperation.calculateAssembleTimeout(file) + ) // 5Gb Mockito.`when`(file.length()).thenReturn(5 * GB) - assertEquals(sut.assembleTimeBase + 5 * sut.assembleTimePerGB, sut.calculateAssembleTimeout(file)) + assertEquals( + ASSEMBLE_TIME_BASE + ASSEMBLE_TIME_PER_GB * 5, + ChunkedFileUploadRemoteOperation.calculateAssembleTimeout(file) + ) // 50Gb Mockito.`when`(file.length()).thenReturn(50 * GB) - assertEquals(sut.assembleTimeMax, sut.calculateAssembleTimeout(file)) + assertEquals(ASSEMBLE_TIME_MAX, ChunkedFileUploadRemoteOperation.calculateAssembleTimeout(file)) // 500Gb Mockito.`when`(file.length()).thenReturn(500 * GB) - assertEquals(sut.assembleTimeMax, sut.calculateAssembleTimeout(file)) + assertEquals(ASSEMBLE_TIME_MAX, ChunkedFileUploadRemoteOperation.calculateAssembleTimeout(file)) } @Test fun assembleTimeoutGrowsWithFileSize() { MockitoAnnotations.openMocks(this) - val sut = - ChunkedFileUploadRemoteOperation( - null, - null, - null, - null, - System.currentTimeMillis() / 1000, - false - ) val timeouts = listOf(1 * MB, 100 * MB, 500 * MB, 1 * GB, 5 * GB).map { length -> Mockito.`when`(file.length()).thenReturn(length) - sut.calculateAssembleTimeout(file) + ChunkedFileUploadRemoteOperation.calculateAssembleTimeout(file) } assertEquals(timeouts.sorted(), timeouts) From 05e37ba695ecee67b89cdf78f3616d7a6d366d05 Mon Sep 17 00:00:00 2001 From: alperozturk96 Date: Tue, 1 Sep 2026 13:13:05 +0200 Subject: [PATCH 3/3] adjust values Signed-off-by: alperozturk96 --- .../resources/files/ChunkedFileUploadRemoteOperation.kt | 7 ++++--- .../files/ChunkedFileUploadRemoteOperationTest.kt | 3 +++ 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/library/src/main/java/com/owncloud/android/lib/resources/files/ChunkedFileUploadRemoteOperation.kt b/library/src/main/java/com/owncloud/android/lib/resources/files/ChunkedFileUploadRemoteOperation.kt index 8d7ae0468..3c3eb382d 100644 --- a/library/src/main/java/com/owncloud/android/lib/resources/files/ChunkedFileUploadRemoteOperation.kt +++ b/library/src/main/java/com/owncloud/android/lib/resources/files/ChunkedFileUploadRemoteOperation.kt @@ -33,6 +33,7 @@ import java.nio.channels.FileChannel import java.util.Locale import kotlin.math.max import kotlin.time.Duration +import kotlin.time.Duration.Companion.hours import kotlin.time.Duration.Companion.milliseconds import kotlin.time.Duration.Companion.minutes @@ -295,11 +296,11 @@ class ChunkedFileUploadRemoteOperation ) companion object { - val ASSEMBLE_TIME_BASE: Duration = 3.minutes + val ASSEMBLE_TIME_BASE: Duration = 1.minutes - val ASSEMBLE_TIME_PER_GB: Duration = 3.minutes + val ASSEMBLE_TIME_PER_GB: Duration = 10.minutes - val ASSEMBLE_TIME_MAX: Duration = 30.minutes + val ASSEMBLE_TIME_MAX: Duration = 1.hours @VisibleForTesting fun calculateAssembleTimeout(file: File): Duration { diff --git a/library/src/test/java/com/owncloud/android/lib/resources/files/ChunkedFileUploadRemoteOperationTest.kt b/library/src/test/java/com/owncloud/android/lib/resources/files/ChunkedFileUploadRemoteOperationTest.kt index b1a5c02ac..e650f101b 100644 --- a/library/src/test/java/com/owncloud/android/lib/resources/files/ChunkedFileUploadRemoteOperationTest.kt +++ b/library/src/test/java/com/owncloud/android/lib/resources/files/ChunkedFileUploadRemoteOperationTest.kt @@ -71,6 +71,9 @@ class ChunkedFileUploadRemoteOperationTest { ChunkedFileUploadRemoteOperation.calculateAssembleTimeout(file) ) + Mockito.`when`(file.length()).thenReturn(6 * GB) + assertEquals(ASSEMBLE_TIME_MAX, ChunkedFileUploadRemoteOperation.calculateAssembleTimeout(file)) + // 50Gb Mockito.`when`(file.length()).thenReturn(50 * GB) assertEquals(ASSEMBLE_TIME_MAX, ChunkedFileUploadRemoteOperation.calculateAssembleTimeout(file))