Repository navigation
fix(upload): retry chunked uploads after network errors - #6805
Conversation
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
|
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
📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthrough
Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to 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 ReviewSecurity architecture risk: 🔵 Low · up to 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
Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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. Comment |
|
Hi @ToteMeiSter thank you for your contributions. |
|
For reference: #6827 changes the same lines of Prepared with an AI assistant and reviewed by me. |
|
Thank you @ToteMeiSter 👍 |
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 everyExceptionand returnsfalse.UploadAndShareFilesWorker.doUpload()already mapsIOExceptiontoResult.retry()(up toMAX_UPLOAD_ATTEMPTS = 4,exponential backoff,
NetworkType.CONNECTEDconstraint), but for chunked uploads that branch was unreachable:falsewent straight tofailUpload(). 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()rethrowsIOException; every other exception still returnsfalse.uploadFile,uploadUsingConversationSubfoldersinsiderunBlocking)propagate the exception to the existing retry branch. No new retry mechanism.
Behaviour after the fix
MAX_UPLOAD_ATTEMPTS.HttpException(DavException : Exception, notIOException):e.g. 507 / 401 / 404 on PUT or MOVE still fail immediately, without retries.
IOException(existing code), so these get up to3 extra attempts, bounded by
MAX_UPLOAD_ATTEMPTS.isStopped/isCancelled()are checked first in the worker'sIOExceptionbranch →Result.failure(), no notification, no retry. Abort between chunks still returnsfalse.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.
VideoCompressor/ImageCompressorre-create the file on every attempt →new lastModified → new chunk folder. The retry uploads from scratch; the old chunk folder stays on the server.
(orphan copy on the server, one chat message).
ChunkedFileUploaderonly, 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 ktlintCheckpasses.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
/backport to stable-xx.x— maintainers to decide (stable-25.0.x?)🤖 AI (if applicable)
Analysis, code and tests were prepared with Claude Code (claude-opus-5-5) and reviewed by the author;
commits carry an
Assisted-bytrailer.🤖 Generated with Claude Code