From 2ec8c7b7619efc1e3c1328efef670924da8c5279 Mon Sep 17 00:00:00 2001 From: Krainov Gleb Date: Mon, 5 Oct 2026 13:08:55 +0000 Subject: [PATCH 1/3] fix(upload): never throw from abortUpload when the worker is stopped WorkManager calls ListenableWorker.stop() -> onStopped() from the invokeOnCancellation handler of its coroutine. abortUpload() sent a DAV DELETE there and let network/DAV errors (and NPEs on an uninitialised upload folder) escape, which surfaces as CompletionHandlerException and kills the process. Return early when nothing was started and log DavException/IOException instead of throwing. Signed-off-by: Krainov Gleb Assisted-by: Claude-Code:claude-opus-5-5 --- .../upload/chunked/ChunkedFileUploader.kt | 19 ++- .../chunked/ChunkedFileUploaderAbortTest.kt | 140 ++++++++++++++++++ 2 files changed, 155 insertions(+), 4 deletions(-) create mode 100644 app/src/test/java/com/nextcloud/talk/upload/chunked/ChunkedFileUploaderAbortTest.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..2180617b9f1 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 @@ -335,13 +335,20 @@ class ChunkedFileUploader( } } + /** + * Called from the worker's onStopped, which WorkManager runs inside a coroutine cancellation handler: + * anything thrown here crashes the process, so this function must never throw. + */ fun abortUpload(onSuccess: () -> Unit) { isUploadAborted = true + val client = okHttpClientNoRedirects + val folderUrl = uploadFolderUri.toHttpUrlOrNull() + if (client == null || folderUrl == null) { + Log.i(TAG, "Nothing to abort, chunk upload was not started") + return + } try { - DavResource( - okHttpClientNoRedirects!!, - uploadFolderUri.toHttpUrlOrNull()!! - ).delete { response: Response -> + DavResource(client, folderUrl).delete { response: Response -> when { response.isSuccessful -> onSuccess() else -> isUploadAborted = false @@ -350,6 +357,10 @@ class ChunkedFileUploader( } catch (e: NotFoundException) { Log.i(TAG, "Chunk upload folder could not be found", e) onSuccess() + } catch (e: DavException) { + Log.w(TAG, "Failed to remove chunk upload folder", e) + } catch (e: IOException) { + Log.w(TAG, "Failed to remove chunk upload folder", e) } } diff --git a/app/src/test/java/com/nextcloud/talk/upload/chunked/ChunkedFileUploaderAbortTest.kt b/app/src/test/java/com/nextcloud/talk/upload/chunked/ChunkedFileUploaderAbortTest.kt new file mode 100644 index 00000000000..b8bd4730936 --- /dev/null +++ b/app/src/test/java/com/nextcloud/talk/upload/chunked/ChunkedFileUploaderAbortTest.kt @@ -0,0 +1,140 @@ +/* + * 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.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.util.concurrent.TimeUnit + +/** + * WorkManager calls the worker's onStopped from a coroutine cancellation handler. An exception thrown by + * [ChunkedFileUploader.abortUpload] there crashes the process, so it must never throw, whatever the network does. + */ +@RunWith(RobolectricTestRunner::class) +@Config(application = Application::class, sdk = [33]) +class ChunkedFileUploaderAbortTest { + + @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 + private var successCalls = 0 + + @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 = "alice", + username = "alice", + token = "token", + baseUrl = server.url("/").toString().trimEnd('/') + ) + } + + @After + fun tearDown() { + server.shutdown() + } + + @Test + fun `does not throw and sends no request when the upload was never started`() { + uploader().abortUpload { successCalls++ } + + assertEquals(0, server.requestCount) + assertEquals(0, successCalls) + } + + @Test + fun `does not throw and does not report success when the connection drops`() { + val uploader = startedUploader() + server.enqueue(MockResponse().apply { socketPolicy = SocketPolicy.DISCONNECT_AFTER_REQUEST }) + + uploader.abortUpload { successCalls++ } + + assertEquals(0, successCalls) + } + + @Test + fun `does not throw and does not report success on a server error`() { + val uploader = startedUploader() + server.enqueue(MockResponse().setResponseCode(SERVER_ERROR)) + + uploader.abortUpload { successCalls++ } + + assertEquals(0, successCalls) + } + + @Test + fun `reports success when the upload folder is already gone`() { + val uploader = startedUploader() + server.enqueue(MockResponse().setResponseCode(NOT_FOUND)) + + uploader.abortUpload { successCalls++ } + + assertEquals(1, successCalls) + } + + @Test + fun `reports success when the upload folder is deleted`() { + val uploader = startedUploader() + server.enqueue(MockResponse().setResponseCode(NO_CONTENT)) + + uploader.abortUpload { successCalls++ } + + assertEquals(1, successCalls) + } + + private fun uploader() = ChunkedFileUploader(client, user, progressListener, ncApiCoroutines) + + /** Runs a rejected upload so the uploader knows its upload folder, then drops the recorded requests. */ + private fun startedUploader(): ChunkedFileUploader { + val file = tempFolder.newFile("test.txt").apply { writeBytes(ByteArray(FILE_SIZE)) } + server.enqueue(MockResponse().setResponseCode(CREATED)) + server.enqueue(MockResponse().setResponseCode(NOT_FOUND)) + server.enqueue(MockResponse().setResponseCode(FORBIDDEN)) + val uploader = uploader() + uploader.upload(file, "text/plain".toMediaType(), "/Talk/test.txt") + repeat(server.requestCount) { server.takeRequest(1, TimeUnit.SECONDS) } + return uploader + } + + companion object { + private const val FILE_SIZE = 5 + private const val TIMEOUT_SECONDS = 10L + private const val CREATED = 201 + private const val NO_CONTENT = 204 + private const val FORBIDDEN = 403 + private const val NOT_FOUND = 404 + private const val SERVER_ERROR = 500 + } +} From 850d84d6f6fd00fc4f92772f71ed3c4f0fb894f2 Mon Sep 17 00:00:00 2001 From: Krainov Gleb Date: Mon, 5 Oct 2026 20:02:56 +0000 Subject: [PATCH 2/3] fix(upload): catch every exception in abortUpload abortUpload() runs from onStopped() inside a cancellation handler and must never throw. It caught only DavException and IOException, so a RuntimeException from the success callback, dav4jvm or OkHttp still escaped. Catch Exception, like upload() already does. Signed-off-by: Krainov Gleb Assisted-by: Claude-Code:claude-opus-5-5 --- .../nextcloud/talk/upload/chunked/ChunkedFileUploader.kt | 5 ++--- .../talk/upload/chunked/ChunkedFileUploaderAbortTest.kt | 8 ++++++++ 2 files changed, 10 insertions(+), 3 deletions(-) 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 2180617b9f1..e11c57376c8 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 @@ -339,6 +339,7 @@ class ChunkedFileUploader( * Called from the worker's onStopped, which WorkManager runs inside a coroutine cancellation handler: * anything thrown here crashes the process, so this function must never throw. */ + @Suppress("Detekt.TooGenericExceptionCaught") fun abortUpload(onSuccess: () -> Unit) { isUploadAborted = true val client = okHttpClientNoRedirects @@ -357,9 +358,7 @@ class ChunkedFileUploader( } catch (e: NotFoundException) { Log.i(TAG, "Chunk upload folder could not be found", e) onSuccess() - } catch (e: DavException) { - Log.w(TAG, "Failed to remove chunk upload folder", e) - } catch (e: IOException) { + } catch (e: Exception) { Log.w(TAG, "Failed to remove chunk upload folder", e) } } diff --git a/app/src/test/java/com/nextcloud/talk/upload/chunked/ChunkedFileUploaderAbortTest.kt b/app/src/test/java/com/nextcloud/talk/upload/chunked/ChunkedFileUploaderAbortTest.kt index b8bd4730936..d5dec8dc9fc 100644 --- a/app/src/test/java/com/nextcloud/talk/upload/chunked/ChunkedFileUploaderAbortTest.kt +++ b/app/src/test/java/com/nextcloud/talk/upload/chunked/ChunkedFileUploaderAbortTest.kt @@ -114,6 +114,14 @@ class ChunkedFileUploaderAbortTest { assertEquals(1, successCalls) } + @Test + fun `does not throw when the success callback throws`() { + val uploader = startedUploader() + server.enqueue(MockResponse().setResponseCode(NO_CONTENT)) + + uploader.abortUpload { throw IllegalStateException("callback failed") } + } + private fun uploader() = ChunkedFileUploader(client, user, progressListener, ncApiCoroutines) /** Runs a rejected upload so the uploader knows its upload folder, then drops the recorded requests. */ From 2a0ae03080661bab872f6a17d0cefb0919884462 Mon Sep 17 00:00:00 2001 From: Krainov Gleb Date: Mon, 5 Oct 2026 20:17:38 +0000 Subject: [PATCH 3/3] fix(upload): do not let the abort callback throw on a missing folder When the upload folder is already gone, abortUpload() calls onSuccess() from the NotFoundException handler, where the following catch does not apply. Guard that call too. Signed-off-by: Krainov Gleb Assisted-by: Claude-Code:claude-opus-5-5 --- .../nextcloud/talk/upload/chunked/ChunkedFileUploader.kt | 6 +++++- .../talk/upload/chunked/ChunkedFileUploaderAbortTest.kt | 8 ++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) 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 e11c57376c8..5975e9bca1e 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 @@ -357,7 +357,11 @@ class ChunkedFileUploader( } } catch (e: NotFoundException) { Log.i(TAG, "Chunk upload folder could not be found", e) - onSuccess() + try { + onSuccess() + } catch (callbackError: Exception) { + Log.w(TAG, "Abort callback failed", callbackError) + } } catch (e: Exception) { Log.w(TAG, "Failed to remove chunk upload folder", e) } diff --git a/app/src/test/java/com/nextcloud/talk/upload/chunked/ChunkedFileUploaderAbortTest.kt b/app/src/test/java/com/nextcloud/talk/upload/chunked/ChunkedFileUploaderAbortTest.kt index d5dec8dc9fc..6bfbbd747e9 100644 --- a/app/src/test/java/com/nextcloud/talk/upload/chunked/ChunkedFileUploaderAbortTest.kt +++ b/app/src/test/java/com/nextcloud/talk/upload/chunked/ChunkedFileUploaderAbortTest.kt @@ -122,6 +122,14 @@ class ChunkedFileUploaderAbortTest { uploader.abortUpload { throw IllegalStateException("callback failed") } } + @Test + fun `does not throw when the success callback throws for a folder that is already gone`() { + val uploader = startedUploader() + server.enqueue(MockResponse().setResponseCode(NOT_FOUND)) + + uploader.abortUpload { throw IllegalStateException("callback failed") } + } + private fun uploader() = ChunkedFileUploader(client, user, progressListener, ncApiCoroutines) /** Runs a rejected upload so the uploader knows its upload folder, then drops the recorded requests. */