feat(service): Implement MultipartUploadBackend on InMemory and LocalFs#456
Conversation
lcian
changed the title
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| } | ||
|
|
||
| #[async_trait::async_trait] | ||
| impl MultipartUploadBackend for LocalFsBackend { |
There was a problem hiding this comment.
i think this impl will be pretty susceptible to 5XXs. operations don't acquire any sort of lock on the upload dir so a concurrent abort/complete request can remove the upload dir in the middle of other concurrent handlers. if/when we want this backend to be production-worthy we probably want to make sure races result in an appropriate 4XX
There was a problem hiding this comment.
Hmm yes I see what you mean. We need some sort of lock files. Filed as a follow-up we need to eventually do for self-hosted support: https://linear.app/getsentry/issue/PF-81/avoid-races-in-fs-backend
lcian
force-pushed
the
lcian/test/gcs-multipart
branch
from
36f1380 to
0e21505
Compare
…lidation InMemoryBackend::complete_multipart was calling .remove() on the multipart store before validating parts. If validation failed, the upload was permanently destroyed and the client could not retry. Changed to .get() for validation, only removing after successful assembly. Added retry-after-failure test coverage for both InMemory and LocalFs backends.
lcian
force-pushed
the
lcian/feat/multipart-upload-other-backends
branch
from
e8a193d to
803c658
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #456 +/- ##
==========================================
+ Coverage 86.45% 86.91% +0.46%
==========================================
Files 77 77
Lines 10806 11394 +588
==========================================
+ Hits 9342 9903 +561
- Misses 1464 1491 +27
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
upload_part and complete_multipart were buffering entire payloads in memory. Both now use StreamReader + BufWriter + tokio::io::copy to stream data to disk, matching the put_object pattern.
Add a TODO to validate bytes copied against content_length once we have a proper BadRequest error variant at the service layer.
S3 and GCS reject CompleteMultipartUpload requests where parts are not in ascending order. Add TODOs to LocalFs and InMemory backends to implement this validation once we have a proper client error variant.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 78f0d7f. Configure here.
2 participants
Implements
MultipartUploadBackendonInMemoryBackendandLocalFsBackend, used for tests.TieredStoragewill come in the next PR.InMemoryBackendnow stores some auxiliary data structures dedicated to multipart uploads, whileLocalFsuses a__multipart__/directory where to store metadata and parts for each ongoing upload.Close FS-349