feat(upload): Resumable Uploads - #6203
Conversation
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?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.
![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}
| /// The minimum distance between two `Upload-Offset`s in bytes. | ||
| /// | ||
| /// Every `Upload-Offset` must be 0 modulo `UPLOAD_GRANULARITY`. | ||
| const UPLOAD_GRANULARITY: NonZeroUsize = NonZeroUsize::new(1024 * 1024).unwrap(); // 1 MiB |
tobias-wilfert
left a comment
tobias-wilfert
left a comment
There was a problem hiding this comment.
Looks reasonable, before I deploy this, will make a DD notebook to monitor that this doesn't break something.
| Self::InvalidUploadLength { .. } => "invalid_upload_length", | ||
| Self::RequestTooSmall { .. } => "request_too_small", | ||
| Self::UnalignedBody { .. } => "unaligned_body", | ||
| Self::UnknownKey { .. } => "invalid_length", |
There was a problem hiding this comment.
Bug: The ErrorKind::UnknownKey variant is incorrectly mapped to the metric tag "invalid_length", which belongs to InvalidUploadLength, causing metric mislabeling.
Severity: LOW
Suggested Fix
Change the string returned for the ErrorKind::UnknownKey variant in the as_str() method from "invalid_length" to "unknown_key" to match the variant name and align with the pattern used for other error kinds.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: relay-server/src/services/objectstore.rs#L334
Potential issue: In the `as_str()` implementation for `ErrorKind`, the `UnknownKey`
variant is incorrectly mapped to the string `"invalid_length"`. This string is already
used by the `InvalidUploadLength` variant. As a result, when an `UnknownKey` error
occurs during an attachment upload, the `RelayCounters::AttachmentUpload` metric will be
incorrectly tagged with `result="invalid_length"`. This misattribution will skew
monitoring and alerting, conflating two distinct error types and hindering
observability.
| match self { | ||
| ErrorKind::InvalidOffset { .. } | ||
| | ErrorKind::InvalidUploadLength { .. } | ||
| | ErrorKind::UnalignedBody { .. } => true, |
There was a problem hiding this comment.
Client error misclassified as server
Medium Severity
RequestTooSmall is a client-retryable condition, but is_client_error does not include it. Failures then fall through to relay_log::error, so expected undersized compressed chunks are reported as server errors.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 3940d47. Configure here.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 3 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 6809aba. Configure here.
| headers={"Location": DUMMY_UPLOAD_LOCATION}, | ||
| headers={ | ||
| "Location": DUMMY_UPLOAD_LOCATION, | ||
| "Upload-Offset": len(request.data), |
There was a problem hiding this comment.
Dummy upload offset uses compressed size
Medium Severity
The dummy_upload fixture sets Upload-Offset from len(request.data), but that body is zstd-compressed (Content-Encoding: zstd). TUS Upload-Offset is the uncompressed byte offset, so upstream proxy tests can observe the wrong offset via StreamResult::try_from_response.
Reviewed by Cursor Bugbot for commit 6809aba. Configure here.
| (_, None) => { | ||
| if offset != 0 { | ||
| return Err(Error::OffsetWithoutLength); | ||
| } |
There was a problem hiding this comment.
Misleading resume error without multipart
Low Severity
OffsetWithoutLength is returned whenever upload_id is missing and offset != 0, including when the client did send Upload-Length but multipart was disabled. The error text blames Defer-Length: 1, which is incorrect in that case.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 6809aba. Configure here.
|
This is put on hold for now, we first need to figure out some stuff that came up. |
2 participants
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