feat(server): Add multipart upload endpoints by lcian · Pull Request #463 · getsentry/objectstore

lcian · 2026-05-07T12:30:14Z

⚠️ Stacked on #458

Final part of the Multipart uploads implementation server-side, this implements the endpoints on top of the Service.

Close FS-341

cursor

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit 5a3cfa2. Configure here.}

lcian · 2026-05-08T13:26:59Z

+    ///
+    /// The default returns `None`. Backends that implement
+    /// [`MultipartUploadBackend`] should override this to return `Some(self)`.
+    fn as_multipart_upload_backend(&self) -> Option<&dyn MultipartUploadBackend> {


Not sure if this is the best approach.
Backend should not know about MultipartUploadBackend, and this means that the server errors with HTTP status NOT IMPLEMENTED when the backend doesn't support this capability.

As an alternative, we could make StorageService store a Arc<dyn MultipartUploadBackend>.
That however would prevent someone from using Objectstore with a Bigtable only configuration, which I'm not sure if we consider a legitimate usecase or not.

Regardless, we can easily make that change at any point.

lcian · 2026-05-08T14:08:19Z

+    headers: HeaderMap,
+) -> ApiResult<Response> {
+    let mut metadata = Metadata::from_headers(&headers, "").map_err(ServiceError::from)?;
+    // TODO: Do this in `complete` instead, when we have a Service API to mutate metadata.


This is easy to do but we would need a new method on Backend, plus all the implementations and plumbing through Service, filed a follow-up: https://linear.app/getsentry/issue/FS-356/set-correct-metadatatime-created-for-multipart-uploads.

Or, we can just accept and document that time_created refers to the time the upload was initiated, but that sounds wrong, it should probably be the time the logical object is created, which is the time the multipart upload is completed.

lcian · 2026-05-08T17:43:12Z

+        max_parts: Option<u32>,
+        part_number_marker: Option<PartNumber>,
+    ) -> ApiResult<ListPartsResponse> {
+        self.assert_authorized(Permission::ObjectRead, id.context())?;


Maybe this should also require Permission::ObjectWrite instead, to match the rest of the methods.
But also, if a user has ObjectWrite they would usually also have ObjectRead in any realistic scenario.

not sure what to do here. GCS just gives listing its own permission

if a user has ObjectWrite they would usually also have ObjectRead in any realistic scenario

that might be how we assign permissions today, but i think probably most tokens only need to be read or write.

if the point is to allow an interrupted write to continue, maybe write permissions should allow it? so a write-only client doesn't need to request additional permissions. idk

codecov · 2026-05-15T16:58:57Z

Codecov Report

❌ Patch coverage is 93.67816% with 22 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.16%. Comparing base (c21fe73) to head (c33ba6f).

Files with missing lines	Patch %	Lines
objectstore-server/src/endpoints/common.rs	0.00%	3 Missing ⚠️
objectstore-server/src/endpoints/multipart.rs	98.13%	3 Missing ⚠️
objectstore-service/src/backend/common.rs	0.00%	3 Missing ⚠️
objectstore-service/src/backend/gcs.rs	0.00%	3 Missing ⚠️
objectstore-service/src/backend/in_memory.rs	0.00%	3 Missing ⚠️
objectstore-service/src/backend/testing.rs	0.00%	3 Missing ⚠️
objectstore-service/src/backend/tiered.rs	0.00%	3 Missing ⚠️
objectstore-service/src/error.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@                          Coverage Diff                           @@
##           lcian/feat/multipart-upload-tiered     #463      +/-   ##
======================================================================
+ Coverage                               86.95%   87.16%   +0.20%     
======================================================================
  Files                                      77       78       +1     
  Lines                                   11685    12032     +347     
======================================================================
+ Hits                                    10161    10488     +327     
- Misses                                   1524     1544      +20

Components	Coverage Δ
Rust Backend	`91.80% <93.67%> (+0.07%)`	⬆️
Rust Client	`80.10% <ø> (ø)`
Python Client	`86.36% <ø> (ø)`

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

matt-codecov

accept w comments

matt-codecov · 2026-05-21T23:08:30Z

+        max_parts: Option<u32>,
+        part_number_marker: Option<PartNumber>,
+    ) -> ApiResult<ListPartsResponse> {
+        self.assert_authorized(Permission::ObjectRead, id.context())?;


not sure what to do here. GCS just gives listing its own permission

if a user has ObjectWrite they would usually also have ObjectRead in any realistic scenario

that might be how we assign permissions today, but i think probably most tokens only need to be read or write.

if the point is to allow an interrupted write to continue, maybe write permissions should allow it? so a write-only client doesn't need to request additional permissions. idk

matt-codecov · 2026-05-21T23:09:48Z

                objectstore_log::error!(!!self, "error handling request");
                StatusCode::INTERNAL_SERVER_ERROR
            }
+
+            ApiError::Internal(_) => {
+                objectstore_log::error!(!!self, "internal error");


are we dropping important errors from these logs?

matt-codecov · 2026-05-21T23:12:49Z

            Self::Panic(_) => Level::ERROR,
            Self::Dropped => Level::ERROR,
            Self::UnexpectedTombstone => Level::ERROR,
+            Self::NotImplemented => Level::WARN,


this is probably an error? if this ever happens in a deployment it means something was misconfigured

matt-codecov · 2026-05-22T00:11:36Z

+    fn ensure_inner_multipart(&self) -> Result<()> {
+        self.inner
+            .as_multipart_upload_backend()
+            .ok_or(Error::NotImplemented)?;
+        Ok(())
+    }
+
+    /// Initiates a new multipart upload.
+    pub async fn initiate_multipart(
+        &self,
+        id: ObjectId,
+        metadata: Metadata,
+    ) -> Result<InitiateMultipartResponse> {
+        self.ensure_inner_multipart()?;
+        let inner = Arc::clone(&self.inner);
+        self.spawn("initiate_multipart", async move {
+            inner
+                .as_multipart_upload_backend()
+                .unwrap()
+                .initiate_multipart(&id, &metadata)
+                .await
+        })
+        .await
+    }


there's a way to streamline this a bit to get rid of the ensure_... helper and .unwrap() calls:

pub trait Backend { /// Make this an explicit `Arc<dyn T>` -> `Arc<dyn U>` cast. /// Return `Err(Error::NotImplemented)` by default. fn as_multipart_upload_backend(self: Arc<Self>) -> Result<Arc<dyn MultipartUploadBackend>> { Err(Error::NotImplemented) } } impl Backend for GcsBackend { /// Convert `Arc<dyn Backend>` into `Arc<dyn MultipartUploadBackend>` fn as_multipart_upload_backend(self: Arc<Self>) -> Result<Arc<dyn MultipartUploadBackend>> { Ok(self) } } impl StorageService { pub async fn initiate_multipart(...) -> Result<InitiateMultipartResponse> { // Clone `inner` so it can be `move`d into the async block, as before. // `.as_multipart_upload_backend()` transforms the clone into `Arc<dyn MultipartUploadBackend>`. // It still shares its underlying pointer/refcount with the original, though! // `?` to return `Err(Error::NotImplemented)` when not implemented. let inner_multipart = self.inner.clone().as_multipart_upload_backend()?; self.spawn("initiate_multipart", async move { inner_multipart.initiate_multipart(&id, &metadata).await }).await } }

the Arc refcounting all still tracks the same object. this is just a super verbose cast for trait objects inside Arcs. you can confirm the refcount stuff in this playground link https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=ea6b827dd6fb9c3194bac54d0f62b2ea

matt-codecov · 2026-05-22T00:47:53Z

+fn validate_part_number(part_number: u32) -> ApiResult<()> {
+    if part_number == 0 {
+        return Err(ApiError::Client("part_number must be >= 1".into()));
+    }
+    Ok(())
+}


would using a type PartNumber = std::num::NonZero<u32> alias instead of u32 for these part_number fields take care of this? we'd have to move the error handling somewhere else but we wouldn't have to remember to call validate()

matt-codecov · 2026-05-22T00:49:36Z

+            id,
+            params.upload_id,
+            params.max_parts,
+            params.part_number_marker,


technically you have to validate that this is >=1 as well

matt-codecov · 2026-05-22T00:54:13Z

+    Query(params): Query<UploadIdQuery>,
+    Json(body): Json<CompleteRequest>,
+) -> ApiResult<Response> {
+    service.check_permission(Permission::ObjectWrite, id.context())?;


is there something we need to check that isn't covered in AuthAwareService's wrapper around complete_multipart()?

lcian changed the base branch from main to lcian/feat/multipart-upload-tiered May 7, 2026 12:31

This comment was marked as outdated.

Sign in to view

sentry Bot reviewed May 7, 2026

View reviewed changes

Comment thread objectstore-server/src/endpoints/multipart.rs

cursor Bot reviewed May 7, 2026

View reviewed changes

Comment thread objectstore-server/src/endpoints/multipart.rs

lcian changed the title ~~lcian/feat/multipart upload endpoints~~ feat(server): Add multipart upload endpoints May 7, 2026

This comment was marked as off-topic.

Sign in to view

This comment was marked as outdated.

Sign in to view

cursor Bot reviewed May 7, 2026

View reviewed changes

Comment thread objectstore-server/src/endpoints/multipart.rs

This comment was marked as outdated.

Sign in to view

This comment was marked as resolved.

Sign in to view

cursor Bot reviewed May 8, 2026

View reviewed changes

lcian commented May 8, 2026

View reviewed changes

lcian marked this pull request as ready for review May 8, 2026 14:08

lcian requested a review from a team as a code owner May 8, 2026 14:08

lcian requested a review from matt-codecov May 8, 2026 17:19

lcian commented May 8, 2026

View reviewed changes

lcian mentioned this pull request May 11, 2026

feat(rust-client): Add Multipart Upload API #468

Open

lcian force-pushed the lcian/feat/multipart-upload-tiered branch from 409be20 to d6eb979 Compare May 12, 2026 11:32

lcian requested review from matt-codecov and removed request for matt-codecov May 15, 2026 16:37

lcian added 5 commits May 15, 2026 18:57

wip

c2ce1bc

wip

2076c40

fix: update doc link to renamed as_multipart_upload_backend method

6a90f1f

improve

23372bd

improve

3a87cec

lcian added 10 commits May 15, 2026 18:57

improve

45d220e

improve

3d9e9ea

fix

e4a5267

improve

d73efe0

improve, add tests

c6d5403

add tess

3fe73c0

fmt and clippy

c59246b

fix

30e500b

improve

e43f23c

improve

c33ba6f

lcian force-pushed the lcian/feat/multipart-upload-endpoints branch from 5babbfb to c33ba6f Compare May 15, 2026 16:57

sentry Bot reviewed May 15, 2026

View reviewed changes

Comment thread objectstore-server/src/endpoints/multipart.rs

matt-codecov approved these changes May 22, 2026

View reviewed changes

Uh oh!

Conversation

lcian commented May 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

Uh oh!

Uh oh!

This comment was marked as off-topic.

This comment was marked as outdated.

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

Uh oh!

This comment was marked as resolved.

This comment was marked as resolved.

cursor Bot left a comment

Choose a reason for hiding this comment

Uh oh!

lcian May 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lcian May 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lcian May 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented May 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

matt-codecov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lcian commented May 7, 2026 •

edited

Loading

lcian May 8, 2026 •

edited

Loading

lcian May 8, 2026 •

edited

Loading

lcian May 8, 2026 •

edited

Loading

codecov Bot commented May 15, 2026 •

edited

Loading