fix(upload): never throw from abortUpload when the worker is stopped by ToteMeiSter · Pull Request #6825 · nextcloud/talk-android

ToteMeiSter · 2026-10-05T19:30:50Z

Problem

UploadAndShareFilesWorker.onStopped() calls ChunkedFileUploader.abortUpload() synchronously. WorkManager runs onStopped() inside the cancellation handler of its coroutine (WorkerWrapper → ListenableWorker.stop()). An exception thrown there becomes a CompletionHandlerException on a WM.task-N thread and crashes the process.

Follow-up to #5605 / #5607, which handled NotFoundException only. abortUpload() can still throw:

IOException: the DELETE is sent while the network is gone, which is the usual reason the work is stopped (screen off, Doze, airplane mode);
HttpException / DavException: for example a 5xx response;
NullPointerException: from okHttpClientNoRedirects!! and uploadFolderUri.toHttpUrlOrNull()!! when the work is stopped before the chunked upload started.

To reproduce: send a file larger than 1 MiB to a chat and switch the phone to airplane mode during the upload. The process can crash in WM.task (this is how it showed up in a fork build after the phone woke from sleep).

Fix

abortUpload() never throws:

if the client or the folder URL is not set yet, it returns without a request;
NotFoundException still calls onSuccess();
other DavException and IOException are logged; onSuccess() is not called.

onStopped() and the worker logic are unchanged. Code change: ChunkedFileUploader.kt, +15/−4.

Tests

New ChunkedFileUploaderAbortTest (MockWebServer), 5 cases: called before the upload started, connection dropped, 500, 404, 204. On the old code 3 of them fail (NPE, IOException, HttpException 500). testGplayDebugUnitTest passes, ktlint is clean, detekt reports no new findings.

Tested by the author on a Huawei DEL-LX9.

Note: #6770 moves the abortUpload() call from onStopped() into a finally block in doWork(). The fix still applies there: an exception from that block would fail the work.

🏁 Checklist

⛑️ Tests (unit and/or integration) are included or not needed
🔖 Capability is checked or not needed
🔙 Backport requests are created or not needed: /backport to stable-xx.x
📅 Milestone is set
🌸 PR title is meaningful (if it should be in the changelog: is it meaningful to users?)

🤖 AI (if applicable)

The content of this PR was partly or fully generated using AI

The analysis, the fix and the test were prepared with an AI assistant (Claude Code). I reviewed the change and tested it on a device.

🤖 Generated with Claude Code

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 <krajnov.g@kontentplus.ru> Assisted-by: Claude-Code:claude-opus-5-5

coderabbitai · 2026-10-05T19:32:37Z

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 3fc9d564-ca73-464c-ba3f-30d1810335b6

📥 Commits

Reviewing files that changed from the base of the PR and between 850d84d and 2a0ae03.

📒 Files selected for processing (2)

app/src/main/java/com/nextcloud/talk/upload/chunked/ChunkedFileUploader.kt
app/src/test/java/com/nextcloud/talk/upload/chunked/ChunkedFileUploaderAbortTest.kt

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review.

📝 Walkthrough

Walkthrough

abortUpload sets the abort flag and checks that the HTTP client and upload-folder URL are available before sending a DELETE request. It catches and logs deletion exceptions. When the folder is missing, it catches and logs exceptions from onSuccess. Tests cover aborting before an upload starts, connection loss, HTTP 500, HTTP 404, HTTP 204, and a throwing success callback.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 2a0ae

Abort now skips deletion when prerequisites are unavailable and contains expected deletion and callback failures, allowing cancellation cleanup to continue. No supported merge-blocking risk remains.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 2a0ae

Cancellation now contains ordinary upload-abort failures so remaining cleanup can run. The account, deletion target, and credentials remain unchanged. Remote cleanup after a failed deletion remains best-effort, and server-side recovery guarantees have not been established.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

inferred — The inspected change does not expand the deletion scope: abort continues to target the same upload folder under the current user's configured server and account. No new caller or broader credential authority was introduced in the compared source.

Trust Boundaries and Controls

observed — Deletion retains the uploader's existing no-redirect HTTP client configured with the current user's credentials. The patch changes prerequisite validation and failure handling, not account selection, URL construction, or the authorization client.

Resilience and Maintainability Implications

inferred — A failed deletion can still leave remote chunks, and cancellation can still overlap an in-flight chunk request. These limitations predate the PR; exception containment does not add compensating remote cleanup or make deletion atomic with upload. Server-side retention and recovery behavior remain unestablished.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 15.38% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 13 functions across 2 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the fix: prevent abortUpload from throwing when the worker stops.
Description check	✅ Passed	The description explains the problem, fix, and tests, and includes the AI disclosure and most checklist details. The screenshot and TODO sections are omitted, and the backport and milestone checklist …
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

Fix all pre-merge checks with AI

Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai

🧹 Nitpick comments (1)

app/src/main/java/com/nextcloud/talk/upload/chunked/ChunkedFileUploader.kt (1)

351-351: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value

onSuccess() exceptions can still escape abortUpload.

The KDoc says the function must never throw. The onSuccess() lambda runs inside the delete callback, inside the try. Only DavException and IOException are caught. A RuntimeException from the callback propagates out of abortUpload. The same applies to any other runtime exception from DavResource or okhttp. The current caller passes {}, so this is not triggered today. The "never throws" contract is still not fully enforced.

Consider also catching RuntimeException, with a detekt suppression for TooGenericExceptionCaught, as upload() already does with Exception.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 08eb2246-d96f-49a7-b45c-5eae4e70c7ec

📥 Commits

Reviewing files that changed from the base of the PR and between f3c9097 and 2ec8c7b.

📒 Files selected for processing (2)

app/src/main/java/com/nextcloud/talk/upload/chunked/ChunkedFileUploader.kt
app/src/test/java/com/nextcloud/talk/upload/chunked/ChunkedFileUploaderAbortTest.kt

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.

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 <krajnov.g@kontentplus.ru> Assisted-by: Claude-Code:claude-opus-5-5

ToteMeiSter · 2026-10-05T20:07:23Z

Thanks @coderabbitai. Fixed the nitpick on abortUpload (onSuccess() exceptions) in 850d84d: it now catches Exception, like upload(), with a test where the success callback throws.

coderabbitai

Actionable comments posted: 1

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: d93d5cd4-944f-400f-8eff-77352a56a5da

📥 Commits

Reviewing files that changed from the base of the PR and between 2ec8c7b and 850d84d.

📒 Files selected for processing (2)

app/src/main/java/com/nextcloud/talk/upload/chunked/ChunkedFileUploader.kt
app/src/test/java/com/nextcloud/talk/upload/chunked/ChunkedFileUploaderAbortTest.kt

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review.

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 <krajnov.g@kontentplus.ru> Assisted-by: Claude-Code:claude-opus-5-5

coderabbitai · 2026-10-05T20:19:23Z

Tip

For best results, initiate chat on the files or code changes.

@ToteMeiSter Thanks for fixing this in 850d84d6f and adding the test for a throwing success callback. This addresses the onSuccess() exception concern.

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

You are interacting with an AI system.

ToteMeiSter · 2026-10-05T21:13:00Z

@coderabbitai review

coderabbitai · 2026-10-05T21:13:08Z

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

ToteMeiSter · 2026-10-06T12:53:52Z

This is a bug fix and all CodeRabbit review threads are resolved. Once it is approved and merged, would it be possible to backport it to stable-25.0.x (e.g. for 25.0.3)? The bug is present in v25.0.2.

Suggested command for a maintainer after merge: /backport to stable-25.0.x

This comment was drafted with AI assistance (Claude Code).

coderabbitai Bot reviewed Oct 5, 2026

View reviewed changes

Comment thread app/src/main/java/com/nextcloud/talk/upload/chunked/ChunkedFileUploader.kt

Navigation Menu

Repository navigation

fix(upload): never throw from abortUpload when the worker is stopped - #6825

fix(upload): never throw from abortUpload when the worker is stopped#6825
ToteMeiSter wants to merge 3 commits into
nextcloud:masterfrom
ToteMeiSter:fix/worker-cancellation-crash

ToteMeiSter commented Oct 5, 2026

Uh oh!

coderabbitai Bot commented Oct 5, 2026 •

edited

Loading

Walkthrough

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Uh oh!

ToteMeiSter commented Oct 5, 2026

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

coderabbitai Bot commented Oct 5, 2026

Uh oh!

ToteMeiSter commented Oct 5, 2026

Uh oh!

coderabbitai Bot commented Oct 5, 2026 •

edited

Loading

Uh oh!

ToteMeiSter commented Oct 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

ToteMeiSter commented Oct 5, 2026

Problem

Fix

Tests

🏁 Checklist

🤖 AI (if applicable)

Uh oh!

coderabbitai Bot commented Oct 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Security Architecture Review

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

ToteMeiSter commented Oct 5, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai Bot commented Oct 5, 2026

Uh oh!

ToteMeiSter commented Oct 5, 2026

Uh oh!

coderabbitai Bot commented Oct 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ToteMeiSter commented Oct 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

coderabbitai Bot commented Oct 5, 2026 •

edited

Loading

coderabbitai Bot commented Oct 5, 2026 •

edited

Loading