Skip to content

fix(upload): retry chunked uploads after network errors - #6805

Merged
mahibi merged 1 commit into
nextcloud:masterfrom
ToteMeiSter:fix/chunked-upload-retry
Oct 12, 2026
Merged

mahibi merged 1 commit into
nextcloud:masterfrom
ToteMeiSter:fix/chunked-upload-retry

Conversation

@ToteMeiSter

Copy link
Copy Markdown
Contributor

Chunked uploads (files > 1 MB) failed permanently on the first network error instead of going through
the worker's existing retry path. This PR makes the network error reach that path.
Refs #4115 — covers the part where a chunked upload fails on a dropped connection; other causes are not addressed.

Field case: Talk Android 25.0.1, video uploaded in 1 MB chunks, phone switching networks. The connection dropped
at chunk 14: no retry, immediate "upload failed" notification, 13 chunks left on the server.
Server, limits and quota were not involved.

Root cause

ChunkedFileUploader.upload() catches every Exception and returns false.
UploadAndShareFilesWorker.doUpload() already maps IOException to Result.retry() (up to MAX_UPLOAD_ATTEMPTS = 4,
exponential backoff, NetworkType.CONNECTED constraint), but for chunked uploads that branch was unreachable:
false went straight to failUpload(). Non-chunked uploads (FileUploader + blockingUpload) rethrow and do retry.

When Android reports the network as lost, WorkManager stops the worker on the constraint and reschedules it on its
own. When the socket dies while the network still counts as connected, the worker is not stopped, the exception
was swallowed and the upload failed for good.

Fix

  • upload() rethrows IOException; every other exception still returns false.
  • The worker is unchanged. Both call sites (uploadFile, uploadUsingConversationSubfolders inside runBlocking)
    propagate the exception to the existing retry branch. No new retry mechanism.

Behaviour after the fix

  • Connection loss / timeout during PUT, MKCOL or MOVE: retried with backoff, bounded by MAX_UPLOAD_ATTEMPTS.
  • HTTP errors from dav4jvm 2.1.3 are HttpException (DavException : Exception, not IOException):
    e.g. 507 / 401 / 404 on PUT or MOVE still fail immediately, without retries.
  • MKCOL of the chunk folder wraps non-405 HTTP errors into IOException (existing code), so these get up to
    3 extra attempts, bounded by MAX_UPLOAD_ATTEMPTS.
  • User cancel: isStopped / isCancelled() are checked first in the worker's IOException branch →
    Result.failure(), no notification, no retry. Abort between chunks still returns false.
  • A retry skips chunks already on the server: the chunk folder name is md5(file name + lastModified + length),
    stable across attempts for uncompressed files.

Known limitations (out of scope)

Before this PR each of these cases ended in an immediate failure, so none is a regression.

  • With media compression on (default), VideoCompressor / ImageCompressor re-create the file on every attempt →
    new lastModified → new chunk folder. The retry uploads from scratch; the old chunk folder stays on the server.
  • Every constraint-driven stop of the worker consumes one of the 4 attempts.
  • If the MOVE response is lost after the server finished assembling, the retry uploads the file again
    (orphan copy on the server, one chat message).
  • Overlap: Migrate workers to coroutine workers #6770 (draft, CoroutineWorker migration) — the fix is in ChunkedFileUploader only, worker untouched,
    so no conflict expected. Add a retry option for failed file uploads #6480 (manual retry button) is a separate feature and is not addressed.

How to test

New ChunkedFileUploaderTest (Robolectric + MockWebServer): happy path; connection dropped during chunk PUT →
IOException; 403 on PUT → false; 403 on MOVE → false.
./gradlew :app:testGenericDebugUnitTest --tests "com.nextcloud.talk.upload.chunked.ChunkedFileUploaderTest"
Run locally: 4/4 pass with the fix; the connection-drop test fails on master without it. ./gradlew detekt ktlintCheck passes.

Manual: turn media compression off (to observe resume), send a video > 1 MB, toggle Wi-Fi or kill the connection
mid-upload while mobile data stays on → upload resumes after backoff, no "upload failed" notification.
Cancel during upload → no retry, no failure notification.

🖼️ Screenshots

n/a, no UI change

🏁 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 — maintainers to decide (stable-25.0.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
    Analysis, code and tests were prepared with Claude Code (claude-opus-5-5) and reviewed by the author;
    commits carry an Assisted-by trailer.

🤖 Generated with Claude Code

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 nextcloud#4115

Signed-off-by: Krainov Gleb <krajnov.g@kontentplus.ru>
Assisted-by: Claude Code:claude-opus-5-5
@coderabbitai

coderabbitai Bot commented Oct 3, 2026

Copy link
Copy Markdown

Review in Change Stack →

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: a783cf78-d929-4085-900b-5f3eebdcc95c
📥 Commits

Reviewing files that changed from the base of the PR and between baaca43 and f9d44b5.

📒 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/ChunkedFileUploaderTest.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.


📝 Walkthrough

Walkthrough

ChunkedFileUploader.upload now propagates IOException instead of returning false for that exception. New Robolectric tests cover a successful upload, a connection drop during chunk upload, and HTTP 403 responses during chunk upload or assembly.

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to f9d44

Chunked uploads can now retry network failures while HTTP errors remain immediate failures. No merge-blocking issue is identified; normal checks still apply.

Security Architecture Review

Security architecture risk: 🔵 Low · up to f9d44

Retries remain bounded and use the upload’s assigned account. However, if assembly succeeds but its response is lost, a retry can select a different destination and leave an additional untracked file copy. No expanded access to another account was established.

Retained concerns

  • Low · reliability · inferred: If MOVE commits remotely but its response is lost, the newly enabled retry can assemble the same content at another destination. Ordinary uploads choose a collision-free filename; conversation-subfolder uploads generate another temporary UUID. The earlier copy is not reconciled by the inspected recovery path, weakening failure containment and ownership of retained uploaded content. Previously, this uploader failure returned false rather than entering that retry path.
Security review details

Security Blast Radius

  • inferred — The identified retry outcome affects uploaded content in the assigned account’s attachment or draft storage and the subsequent conversation attachment. The inspected path does not establish expanded cross-account authority or unauthorized readers; server-side draft visibility and retention remain unverified.

Trust Boundaries and Controls

  • observed — The inspected enqueue caller supplies the initiating account ID, which the worker persists and reloads. Missing accounts fail before upload. Retry decisions also reject stopped or cancelled work and enforce the configured attempt bound.

Resilience and Maintainability Implications

  • observed — Cancellation invokes deletion of the chunk folder, while terminal failure updates the placeholder and notification without remote-file reconciliation. These cleanup limitations predate the uploader exception change; the newly enabled retries make recovery after uncertain assembly important.

Hardening Proposals

  • proposed — Persist destination identity for each logical upload and reconcile an uncertain MOVE outcome before selecting another path. Define ownership and cleanup for any superseded assembled copy, including conversation draft files.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 8.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 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 summarizes the main change: retrying chunked uploads after network errors.
Description check ✅ Passed The description explains the problem, root cause, fix, behavior, limitations, and tests. It includes the screenshots, checklist, and AI sections; the TODO section is omitted, and the backport and mile…
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.

@mahibi

mahibi commented Oct 5, 2026

Copy link
Copy Markdown
Collaborator

Hi @ToteMeiSter thank you for your contributions.
We will review them soon

@ToteMeiSter

Copy link
Copy Markdown
Contributor Author

For reference: #6827 changes the same lines of ChunkedFileUploader.upload() and rethrows both network (IOException) and HTTP (HttpException) errors, so it covers this PR. The two PRs conflict in upload(). I suggest one of two paths: close this PR in favour of #6827 (part of #6826, together with #6825 and #6828), or merge this one first, then I will rebase #6827.

Prepared with an AI assistant and reviewed by me.

@mahibi
mahibi merged commit ff77931 into nextcloud:master Oct 12, 2026
19 of 22 checks passed
@mahibi

mahibi commented Oct 12, 2026

Copy link
Copy Markdown
Collaborator

Thank you @ToteMeiSter 👍
Could you rebase #6827 ?

@AndyScherzinger AndyScherzinger added this to the 25.1.0 milestone Oct 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Waiting for reviews AI assisted

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants