feat(upload): Resumable Uploads#6203
Conversation
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| if signed_location.upload_id().is_some() { | ||
| // `SignedLocation<Final>` should never have an upload ID. | ||
| // NOTE: we could encode this into the `Final` type. | ||
| return Err(ProcessingError::InvalidAttachmentRef); | ||
| } |
There was a problem hiding this comment.
| .checked_sub(offset) | ||
| .ok_or(Error::InvalidOffset(offset, u))?; | ||
| (0, remaining_bytes) | ||
| } |
There was a problem hiding this comment.
Signed length below Upload-Length
Medium Severity
When Upload-Length is known, the PATCH handler sets the stream lower bound to 0 instead of requiring the remaining byte count. A client can send fewer bytes than declared, still receive 204, and the upload service signs a Final location whose length is the bytes actually streamed (oneshot) rather than the announced total. That can finalize an upload below the committed length the client advertised at creation.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 57ebe51. Configure here.
There was a problem hiding this comment.
This is by design: The client is allowed to upload a file in parts. For "oneshot", we deliberately correct the returned upload length down.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 9bf8390. Configure here.
| }); | ||
| } | ||
|
|
||
| let mut multipart = session.resume_multipart_upload(key, upload_id)?; |
There was a problem hiding this comment.
Multipart resume skips offset check
High Severity
Resumable multipart uploads accept the client Upload-Offset after alignment checks only, without comparing it to progress already stored via list_parts. A PATCH can start at the wrong offset, write overlapping or gap-ridden parts, and still pass the closing max_part_number check because that check uses the client-derived new_offset, not the server’s prior progress.
Reviewed by Cursor Bugbot for commit 9bf8390. Configure here.
There was a problem hiding this comment.
This is true, I decided to only do a list_parts only on the final request to reduce the number of requests to objectstore. We can still revisit this later, but the gist is that a client can only inflate or corrupt their own data, not fake an offset that is smaller than the actual bytes uploaded.
1 participant
Implement uploads that can be resumed from a given byte offset.
See core protocol of TUS: https://tus.io/protocols/resumable-upload#core-protocol
Design
We have to work around a few limitations:
Upload-Offsetto multipart part numbers and vice versa.This leads to the following design:
To prevent excessive buffering, we upload a part as soon as we hit the 5 MiB threshold. This creates a lot of requests and IMO contributes to the high failure rate.
Near-Future Work
Once the
content_lengthrequirement onmultipart.put_streamhas been lifted, we can simply put the stream through anasync_compressionzstd-encoder and forward it 1:1 to objectstore. We just need to make sure to upload full grains, such that the number of uploaded bytes divides the upload granularity cleanly. See INGEST-1030.Closes INGEST-974 INGEST-975