Repository navigation
fix(upload): resume chunked uploads from the parts the server already has - #6827
ToteMeiSter wants to merge 7 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ➖ Normal Merge Risk: 🔵 Low · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 23.20% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 125 functions across 11 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly and concisely describes the main change: resuming chunked uploads from parts already stored on the server. Description check The description explains the problem, solution, scope, tests, dependencies, follow-up work, and checklist status. The template's screenshots and TODO sections are absent, but the description is otherw… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
- 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.
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
2c57445d-534c-49a2-8d09-4fd90df47d34
📒 Files selected for processing (10)
app/src/main/java/com/nextcloud/talk/jobs/UploadAndShareFilesWorker.ktapp/src/main/java/com/nextcloud/talk/upload/UploadWorkspace.ktapp/src/main/java/com/nextcloud/talk/upload/chunked/ChunkedFileUploader.ktapp/src/main/java/com/nextcloud/talk/utils/FileUtils.ktapp/src/main/java/com/nextcloud/talk/utils/ImageCompressor.ktapp/src/main/java/com/nextcloud/talk/utils/VideoCompressor.ktapp/src/test/java/com/nextcloud/talk/upload/UploadWorkspaceTest.ktapp/src/test/java/com/nextcloud/talk/upload/chunked/ChunkedFileUploaderAbortTest.ktapp/src/test/java/com/nextcloud/talk/upload/chunked/ChunkedFileUploaderResumeTest.ktapp/src/test/java/com/nextcloud/talk/utils/FileUtilsContentResolverTest.kt
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Coordinate cancellation with chunked-uploader startup. · UploadAndShareFilesWorker.kt:502-510
app/src/main/java/com/nextcloud/talk/jobs/UploadAndShareFilesWorker.kt:502-510
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winCoordinate cancellation with chunked-uploader startup.
On the plain chunked-upload path, cancellation can arrive after
doUploadchecks it but beforeuploadFileassignschunkedFileUploader.onStopped()then seesnulland cannot abort the upload. The worker thread continues, so the new uploader can send chunks and assemble the remote file.doUploadchecks cancellation only afteruploadFilereturns, then skips sharing. Prevent uploader startup when cancellation wins this interval.
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
16e921b3-0e02-48c4-9162-5a3c6d098833
📒 Files selected for processing (4)
app/src/main/java/com/nextcloud/talk/jobs/UploadAndShareFilesWorker.ktapp/src/main/java/com/nextcloud/talk/upload/chunked/ChunkedFileUploader.ktapp/src/test/java/com/nextcloud/talk/upload/chunked/ChunkedFileUploaderAbortTest.ktapp/src/test/java/com/nextcloud/talk/upload/chunked/ChunkedFileUploaderResumeTest.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.
|
Thanks @coderabbitai. Fixed the outside-diff finding on |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Latch cancellation before doWork() clears the marker. · UploadAndShareFilesWorker.kt:517-520
app/src/main/java/com/nextcloud/talk/jobs/UploadAndShareFilesWorker.kt:517-520
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winLatch cancellation before
doWork()clears the marker.If a chunk request throws
IOExceptionafter cancellation is marked but before WorkManager callsonStopped(),doUpload()returnsstoppedResult()anddoWork()removes the marker infinally. WorkManager 2.11.2 can callonStopped()afterstartWork()completes if cancellation wins before the wrapper processes that completion. The callback then selectsstop(), which retains uploaded server parts; the failed request does not assemble or delete them. Latch cancellation inisCancelled()soonStopped()still selectsabortUpload().Suggested fix
@@ -119,2 +119,4 @@ private var referenceId: String? = null + @Volatile + private var cancellationObserved = false private var internalConversationId: String? = null @@ -133,1 +135,6 @@ - private fun isCancelled(): Boolean = referenceId?.let { cancelledReferenceIds.contains(it) } == true + private fun isCancelled(): Boolean { + if (referenceId?.let { cancelledReferenceIds.contains(it) } == true) { + cancellationObserved = true + } + return cancellationObserved + }
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
cb8b7c3e-4bf1-4c10-92ee-9a9f12549962
📒 Files selected for processing (1)
app/src/main/java/com/nextcloud/talk/jobs/UploadAndShareFilesWorker.kt
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 1 remain after this review.
|
@coderabbitai review |
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
fd -t f '^UploadAndShareFilesWorker\.kt$' | while IFS= read -r file; do
ast-grep outline "$file" --items all --match 'doUpload|onStopped|stoppedResult|upload' --view expanded
rg -n -C 18 'chunkedFileUploader|ChunkedFileUploader\(|fun onStopped\(|fun doUpload\(' "$file"
done
git show --format=short --stat dc2b18ec9Length of output: 8216
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. |
|
Thanks @coderabbitai. Fixed the outside-diff finding (latch the cancel before |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
fd -t f '^UploadAndShareFilesWorker\.kt$' | while IFS= read -r file; do
ast-grep outline "$file" --items all --match 'isCancelled|doWork|onStopped|stoppedResult' --view expanded
rg -n -C 16 'isCancelled|cancelled|cancelMarker|remove.*ancel|abortUpload|override.*doWork|override.*onStopped' "$file"
done
git show --format=short 8e2165951 -- app/src/main/java/com/nextcloud/talk/jobs/UploadAndShareFilesWorker.ktLength of output: 22232
Your explanation also distinguishes ✏️ Learnings added
You are interacting with an AI system. |
|
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 Suggested command for a maintainer after merge: This comment was drafted with AI assistance (Claude Code). |
8e21659 to
575c8df
Compare
|
Rebased onto current master; the conflicts with #6805 are resolved. |
There was a problem hiding this comment.
Actionable comments posted: 7
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
46ac8e7e-f6b9-43dc-acec-8c1935f7f6da
📒 Files selected for processing (4)
app/src/main/java/com/nextcloud/talk/jobs/UploadAndShareFilesWorker.ktapp/src/main/java/com/nextcloud/talk/upload/chunked/ChunkedFileUploader.ktapp/src/main/java/com/nextcloud/talk/utils/FileUtils.ktapp/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.
… has A chunked upload restarted from zero after every interruption, although the parts it had sent were still on the server: - The PROPFIND on the upload folder named no properties. isFile is set only from a resourcetype property, so it stayed false for every member and the list of uploaded parts was always empty. The request now asks for resourcetype, getcontentlength and displayname. - Parts are accepted only when their name is well formed and their size matches the name. A wrong or foreign entry is uploaded again or ignored. - Only the missing parts are sent. - The assembling MOVE sends OC-Total-Length, so the server can check that the parts add up to the file. - A MOVE the server rejects with 400 removes the parts once and sends the file again. The caller supplies the "already restarted" mark. - stop() interrupts the upload between two parts and keeps the parts on the server. Only abortUpload() removes them. - upload() no longer swallows exceptions. A failed request is thrown (an HttpException for an HTTP error, an IOException for a network error) and the parts stay on the server. A lost network while listing the parts is thrown too instead of being taken for "no parts", which would send the whole file again. Signed-off-by: Krainov Gleb <krajnov.g@kontentplus.ru> Assisted-by: Claude-Code:claude-sonnet-5-5
The folder of the parts on the server is keyed by the name, the modification time and the length of the file. UploadAndShareFilesWorker copied the content uri to the cache and compressed it again on every run. Each run produced a file with a new modification time, so a retry used another folder and sent the whole file again. The worker now prepares the file once into a directory of its own work (cacheDir/uploads/<work id>) and uploads the same file on every run: - UploadWorkspace keeps the prepared file and the "parts were removed once" mark, and holds a lock, because WorkManager does not interrupt the thread of a stopped worker and an old run can still be busy. - A workspace stays after a stop or before a retry. It is removed when the work ends, and the workspaces of finished or unknown works are removed before the next upload. - A run that is stopped while the file is prepared does not store the half-finished file. - A copy of a content uri that could not be read completely is not used as the file to upload. - onStopped() keeps the parts on the server when the system or the network stopped the work. The parts are removed only when the user cancelled it. Signed-off-by: Krainov Gleb <krajnov.g@kontentplus.ru> Assisted-by: Claude-Code:claude-sonnet-5-5
Remove the rejected parts before marking the upload as restarted: when the DELETE fails, the retry must still be allowed to remove them. Keep the upload workspace in noBackupFilesDir instead of cacheDir. The system may clear the cache under storage pressure while an upload waits for the network. A new copy of the file changes its resume key and leaves the parts on the server orphaned, and after a process restart the content URI may no longer be readable. Finished workspaces are still removed by the worker. Signed-off-by: Krainov Gleb <krajnov.g@kontentplus.ru> Assisted-by: Claude-Code:claude-opus-5-5
A stop that came after the last check of doUpload() and before the chunked uploader was assigned found no uploader in onStopped(), so the upload ran on to the end. Assign the uploader first and check for a stop after that: an earlier stop is seen by the check, a later one reaches the uploader. Signed-off-by: Krainov Gleb <krajnov.g@kontentplus.ru> Assisted-by: Claude-Code:claude-opus-5-5
doWork() removes the cancel marker of its reference id when it ends, but WorkManager may call onStopped() after that. onStopped() then saw no cancel and only stopped the uploader, so the parts stayed on the server. Latch the cancel once it is seen, and check it once more right before the marker is removed. Signed-off-by: Krainov Gleb <krajnov.g@kontentplus.ru> Assisted-by: Claude-Code:claude-opus-5-5
- Delete the workspace of a cancelled run even when it was marked to be kept. - Copy a file:// source into the workspace, so the prepared upload does not depend on the caller's cache file. - Fix the upload folder before the uploader is published, so a cancel that arrives early can still remove the parts; abort at once if it already came. - Restart after HTTP 400 only when the assembly was rejected, not a part. - Sort the listed parts before looking for gaps. - Treat 401, 403, 408 and 429 while listing the parts as errors, not as an empty folder. - Delete a partial copy when the stream breaks off. Signed-off-by: Krainov Gleb <krajnov.g@kontentplus.ru> Assisted-by: Claude-Code:claude-sonnet-5-5
copyFileToCache returned any existing target, including the leftover of a copy that died half way, and removed the target only on IOException. - Copy into a temporary file next to the target and rename it after a complete copy; remove the temporary file on any exception. - Treat an empty existing target as no cache hit and copy again. Assisted-by: Claude-Code:claude-sonnet-5-5 Signed-off-by: Krainov Gleb <krajnov.g@kontentplus.ru>
43d0b1d to
4107b52
Compare
2 participants
Depends on #6825. Part of #6826. Followed by #6828.
Problem
A chunked upload (files larger than 1 MiB) starts from zero after every interruption, although the parts it sent are still on the server. In
UploadAndShareFilesWorkerandChunkedFileUploaderon currentmasterI see four causes:isFileis set only from aresourcetypeproperty, so it staysfalsefor every member and the list of uploaded parts is always empty. Resume never finds a part.onStopped()callsabortUpload()on any stop (network lost, screen off, system), which deletes the parts on the server.upload()catches every exception and returnsfalse, so a network error never reaches the retry in the worker.To reproduce: send a video larger than a few MiB to a chat and switch on airplane mode during the upload for a minute. After the network returns, the upload starts from the first part.
Solution
ChunkedFileUploaderresourcetype,getcontentlengthanddisplayname.<start>-<end>(16 digits each) and its size matches the name. Other entries (.file, foreign files, wrong size) are ignored or uploaded again.OC-Total-Length.isRestarted/markRestarted), so this happens once per upload.stop()interrupts the upload between two parts and keeps the parts.abortUpload()still removes them.upload()does not swallow exceptions any more. An HTTP error is thrown asHttpException, a network error asIOException. The parts stay on the server. A network error while listing the parts is thrown too, instead of being taken for "no parts".UploadAndShareFilesWorkercacheDir/uploads/<work id>(newUploadWorkspace). Every run uploads the same file, so the folder key is stable. The workspace is kept after a stop or before a retry and removed when the work ends. Workspaces of finished or unknown works are removed before the next upload.onStopped()removes the parts only when the user cancelled the upload. Any other stop callsstop().IOException,MAX_UPLOAD_ATTEMPTS = 4) is unchanged: this PR makes the retry resume instead of restart. HTTP errors still end the upload as failed.Not in this PR (see #6828): foreground service, the attempt limit for network errors, interrupting a request that is already waiting on a dead network, removing the parts when the user cancels a work that waits for a retry.
Relation to #6805
#6805 rethrows
IOExceptionfromChunkedFileUploader.upload(). This PR changes the same lines:upload()rethrows every exception (IOExceptionandHttpException) and the worker handles both as before. The two PRs conflict inChunkedFileUploader.upload().throws IOException when the connection drops during the chunk uploadfrom fix(upload): retry chunked uploads after network errors #6805 passes on this branch.falsefor an HTTP error fail on this branch, becauseupload()throwsHttpExceptionnow. The worker ends such an upload as failed, as before.So this PR covers what #6805 does. If this PR is accepted, #6805 can be closed; if #6805 is merged first, this PR needs a rebase of
upload().Tests
Run:
ChunkedFileUploaderResumeTest(MockWebServer): only missing parts are sent, nothing is sent when all parts exist, a part of a wrong size or foreign names are handled,OC-Total-Lengthis sent, a failing request throws and keeps the parts, a lost network while listing or sending parts throwsIOException,stop()keeps the parts,abortUpload()removes them, a rejected assembly restarts once.UploadWorkspaceTest: the file is prepared once and the folder key stays stable, a vanished file is prepared again, leftovers of an interrupted preparation are removed, finished workspaces are removed and living or locked ones kept, one lock holder.FileUtilsContentResolverTest: a copy of an empty source or of a source whose read grant was revoked is not used (null).testGplayDebugUnitTestpasses (785 tests, 0 failures, 1 skipped), ktlint is clean, detekt reports no new findings againstmaster.Tested by the author on a Huawei DEL-LX9 with a fork build that contains #6825, this PR and the follow-up foreground PR: large file, screen off, airplane mode — the upload resumed and completed.
🏁 Checklist
/backport to stable-xx.x🤖 AI (if applicable)
The analysis, the code and the tests were prepared with an AI assistant (Claude Code). The commits carry an
Assisted-bytrailer. I reviewed the change and tested it on a device.🤖 Generated with Claude Code