From f9d44b5920c654b99f195dfca07a9cef13d19844 Mon Sep 17 00:00:00 2001 From: Krainov Gleb Date: Sat, 3 Oct 2026 09:54:16 +0000 Subject: [PATCH] fix(upload): retry chunked uploads after network errors ChunkedFileUploader.upload() caught every exception and returned false, so a dropped connection during a chunked upload never reached the IOException branch of UploadAndShareFilesWorker that returns Result.retry(). The upload failed immediately instead of being retried with backoff. Rethrow IOException from upload() so the worker's existing retry logic applies. A retry skips chunks that are already on the server as long as the local file is unchanged. HTTP errors (dav4jvm HttpException) and aborts still return false. Refs #4115 Signed-off-by: Krainov Gleb Assisted-by: Claude Code:claude-opus-5-5 --- .../upload/chunked/ChunkedFileUploader.kt | 11 ++ .../upload/chunked/ChunkedFileUploaderTest.kt | 155 ++++++++++++++++++ 2 files changed, 166 insertions(+) create mode 100644 app/src/test/java/com/nextcloud/talk/upload/chunked/ChunkedFileUploaderTest.kt diff --git a/app/src/main/java/com/nextcloud/talk/upload/chunked/ChunkedFileUploader.kt b/app/src/main/java/com/nextcloud/talk/upload/chunked/ChunkedFileUploader.kt index e0a5ba56dec..bc5fabd86d5 100644 --- a/app/src/main/java/com/nextcloud/talk/upload/chunked/ChunkedFileUploader.kt +++ b/app/src/main/java/com/nextcloud/talk/upload/chunked/ChunkedFileUploader.kt @@ -64,7 +64,16 @@ class ChunkedFileUploader( remoteChunkUrl = ApiUtils.getUrlForChunkedUpload(currentUser.baseUrl!!, currentUser.userId!!) } + /** + * Uploads [localFile] in chunks, skipping chunks that are already on the server, and assembles them at + * [targetPath]. + * + * @return false if the upload was aborted or the server answered with an HTTP error + * @throws IOException on I/O failures (e.g. connection loss or timeout) and chunk folder creation failures, + * so the caller can retry; chunks already on the server are skipped as long as [localFile] is unchanged + */ @Suppress("Detekt.TooGenericExceptionCaught") + @Throws(IOException::class) fun upload(localFile: File, mimeType: MediaType?, targetPath: String): Boolean { try { var isUploadSuccessful = true @@ -94,6 +103,8 @@ class ChunkedFileUploader( assembleChunks(uploadFolderUri, targetPath) } return isUploadSuccessful + } catch (e: IOException) { + throw e } catch (e: Exception) { Log.e(TAG, "Something went wrong in ChunkedFileUploader", e) return false diff --git a/app/src/test/java/com/nextcloud/talk/upload/chunked/ChunkedFileUploaderTest.kt b/app/src/test/java/com/nextcloud/talk/upload/chunked/ChunkedFileUploaderTest.kt new file mode 100644 index 00000000000..0e2e0613d50 --- /dev/null +++ b/app/src/test/java/com/nextcloud/talk/upload/chunked/ChunkedFileUploaderTest.kt @@ -0,0 +1,155 @@ +/* + * Nextcloud Talk - Android Client + * + * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: GPL-3.0-or-later + */ +package com.nextcloud.talk.upload.chunked + +import android.app.Application +import com.nextcloud.talk.api.NcApiCoroutines +import com.nextcloud.talk.data.user.model.User +import okhttp3.MediaType.Companion.toMediaType +import okhttp3.OkHttpClient +import okhttp3.mockwebserver.MockResponse +import okhttp3.mockwebserver.MockWebServer +import okhttp3.mockwebserver.SocketPolicy +import org.junit.After +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.Rule +import org.junit.Test +import org.junit.rules.TemporaryFolder +import org.junit.runner.RunWith +import org.mockito.kotlin.mock +import org.robolectric.RobolectricTestRunner +import org.robolectric.annotation.Config +import java.io.File +import java.io.IOException +import java.util.concurrent.TimeUnit + +/** + * A network failure must reach the caller as an [IOException] so the upload can be retried, while HTTP errors are + * reported as a plain `false`. PROPFIND is answered with 404, which the uploader treats as "no chunks + * on the server yet". + */ +@RunWith(RobolectricTestRunner::class) +@Config(application = Application::class, sdk = [33]) +class ChunkedFileUploaderTest { + + @get:Rule + val tempFolder = TemporaryFolder() + + private val server = MockWebServer() + private val ncApiCoroutines: NcApiCoroutines = mock() + private val progressListener: OnDataTransferProgressListener = mock() + private lateinit var client: OkHttpClient + private lateinit var user: User + + @Before + fun setUp() { + server.start() + client = OkHttpClient.Builder() + .retryOnConnectionFailure(false) + .connectTimeout(TIMEOUT_SECONDS, TimeUnit.SECONDS) + .readTimeout(TIMEOUT_SECONDS, TimeUnit.SECONDS) + .build() + user = User( + id = 1, + userId = USER_ID, + username = USER_ID, + token = "token", + baseUrl = server.url("/").toString().trimEnd('/') + ) + } + + @After + fun tearDown() { + server.shutdown() + } + + @Test + fun `uploads the chunk and assembles the file on the happy path`() { + val file = localFile(SMALL_FILE_SIZE) + enqueueFolderCreated() + enqueueNoChunksOnServer() + server.enqueue(MockResponse().setResponseCode(CREATED)) + server.enqueue(MockResponse().setResponseCode(CREATED)) + server.enqueue(MockResponse().setResponseCode(CREATED)) + + val result = uploader().upload(file, MIME_TYPE, TARGET_PATH) + + assertTrue(result) + assertEquals(listOf("MKCOL", "PROPFIND", "PUT", "MKCOL", "MOVE"), takeRequests().map { it.method }) + } + + @Test + fun `throws IOException when the connection drops during the chunk upload`() { + val file = localFile(SMALL_FILE_SIZE) + enqueueFolderCreated() + enqueueNoChunksOnServer() + server.enqueue(MockResponse().apply { socketPolicy = SocketPolicy.DISCONNECT_AFTER_REQUEST }) + + assertThrows(IOException::class.java) { + uploader().upload(file, MIME_TYPE, TARGET_PATH) + } + + assertEquals(listOf("MKCOL", "PROPFIND", "PUT"), takeRequests().map { it.method }) + } + + @Test + fun `returns false when the server rejects the chunk with an HTTP error`() { + val file = localFile(SMALL_FILE_SIZE) + enqueueFolderCreated() + enqueueNoChunksOnServer() + server.enqueue(MockResponse().setResponseCode(FORBIDDEN)) + + val result = uploader().upload(file, MIME_TYPE, TARGET_PATH) + + assertFalse(result) + assertEquals(listOf("MKCOL", "PROPFIND", "PUT"), takeRequests().map { it.method }) + } + + @Test + fun `returns false when the server rejects assembling the chunks with an HTTP error`() { + val file = localFile(SMALL_FILE_SIZE) + enqueueFolderCreated() + enqueueNoChunksOnServer() + server.enqueue(MockResponse().setResponseCode(CREATED)) + server.enqueue(MockResponse().setResponseCode(CREATED)) + server.enqueue(MockResponse().setResponseCode(FORBIDDEN)) + + val result = uploader().upload(file, MIME_TYPE, TARGET_PATH) + + assertFalse(result) + assertEquals(listOf("MKCOL", "PROPFIND", "PUT", "MKCOL", "MOVE"), takeRequests().map { it.method }) + } + + private fun uploader() = ChunkedFileUploader(client, user, progressListener, ncApiCoroutines) + + private fun localFile(size: Int): File = tempFolder.newFile("test.txt").apply { writeBytes(ByteArray(size)) } + + private fun enqueueFolderCreated() { + server.enqueue(MockResponse().setResponseCode(CREATED)) + } + + private fun enqueueNoChunksOnServer() { + server.enqueue(MockResponse().setResponseCode(NOT_FOUND)) + } + + private fun takeRequests() = (0 until server.requestCount).map { server.takeRequest(1, TimeUnit.SECONDS)!! } + + companion object { + private const val USER_ID = "alice" + private const val TARGET_PATH = "/Talk/test.txt" + private const val SMALL_FILE_SIZE = 5 + private const val TIMEOUT_SECONDS = 10L + private const val CREATED = 201 + private const val FORBIDDEN = 403 + private const val NOT_FOUND = 404 + private val MIME_TYPE = "text/plain".toMediaType() + } +}