From 08b5d5bc57fbd4b528a18ac2771cc210e7ac44fc Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Thu, 9 Jul 2026 16:57:23 +0200 Subject: [PATCH 01/41] wip --- relay-server/src/endpoints/common.rs | 1 + relay-server/src/endpoints/upload.rs | 19 +++++++++++---- relay-server/src/services/objectstore.rs | 30 +++++++++++++++++++++--- relay-server/src/services/upload.rs | 21 +++++++++++++---- relay-server/src/utils/tus.rs | 25 +++++++++----------- 5 files changed, 71 insertions(+), 25 deletions(-) diff --git a/relay-server/src/endpoints/common.rs b/relay-server/src/endpoints/common.rs index 8961990115d..413eee360d6 100644 --- a/relay-server/src/endpoints/common.rs +++ b/relay-server/src/endpoints/common.rs @@ -612,6 +612,7 @@ where received: Utc::now(), project, location, + offset: 0, stream, }) .await diff --git a/relay-server/src/endpoints/upload.rs b/relay-server/src/endpoints/upload.rs index bcd6a5b68fe..e43723c7089 100644 --- a/relay-server/src/endpoints/upload.rs +++ b/relay-server/src/endpoints/upload.rs @@ -56,6 +56,9 @@ enum Error { #[error("TUS protocol error: {0}")] Tus(#[from] tus::Error), + #[error("Invalid Upload-Offset {0} for Upload-Length {1}")] + InvalidOffset(usize, usize), + #[error("request error: {0}")] Request(#[from] BadStoreRequest), @@ -80,6 +83,7 @@ impl IntoResponse for Error { let status = match self { Error::Tus(_) => StatusCode::BAD_REQUEST, + Error::InvalidOffset(_, _) => StatusCode::BAD_REQUEST, Error::Request(error) => return error.into_response(), Error::SendError(_) => StatusCode::INTERNAL_SERVER_ERROR, Error::Upload(error) => match error { @@ -218,7 +222,7 @@ async fn handle_patch( check_kill_switch(&state)?; relay_log::trace!("Validating headers"); - tus::validate_patch_headers(&headers).map_err(Error::from)?; + let offset = tus::validate_patch_headers(&headers).map_err(Error::from)?; let location = SignedLocation::from_parts( project_id, @@ -254,18 +258,23 @@ async fn handle_patch( let (lower_bound, upper_bound) = match upload_length.value() { None => (1, config.max_upload_size()), - Some(u) => (u, u), + Some(u) => { + let remaining_bytes = u + .checked_sub(offset) + .ok_or(Error::InvalidOffset(offset, u))?; + (1, remaining_bytes) + } }; let stream = BoundedStream::new(stream, lower_bound, upper_bound); let byte_counter = stream.byte_counter(); relay_log::trace!("Uploading"); - let result = upload(&state, project_context, location, stream).await; + let result = upload(&state, project_context, location, offset, stream).await; let location = result.inspect_err(|e| { relay_log::warn!(error = e as &dyn std::error::Error, "upload failed"); })?; - let upload_offset = byte_counter.get(); + let upload_offset = offset + byte_counter.get(); let mut response = NoContent.into_response(); @@ -329,6 +338,7 @@ async fn upload( state: &ServiceState, project: ProjectContext, location: SignedLocation, + offset: usize, stream: BoundedStream>, ) -> Result, Error> { let location = state @@ -337,6 +347,7 @@ async fn upload( received: Utc::now(), project, location, + offset, stream, }) .await??; diff --git a/relay-server/src/services/objectstore.rs b/relay-server/src/services/objectstore.rs index a950d7697b9..fcdae9ffa6f 100644 --- a/relay-server/src/services/objectstore.rs +++ b/relay-server/src/services/objectstore.rs @@ -334,16 +334,28 @@ pub struct UploadRef { /// The ID of the multipart upload session (chosen by objectstore). /// `None` if the upload is not multipart. pub upload_id: Option, + /// The offset from which to resume an upload. + /// + /// Zero for new uploads. + pub offset: usize, } impl UploadRef { /// Validates the upload ID and returns a new upload reference. - pub fn new(key: String, upload_id: Option) -> Result { + pub fn new( + key: String, + upload_id: Option, + offset: usize, + ) -> Result { let upload_id = match upload_id { Some(s) => Some(UploadId::new(s)?), None => None, }; - Ok(Self { key, upload_id }) + Ok(Self { + key, + upload_id, + offset, + }) } } @@ -788,6 +800,7 @@ impl ObjectstoreServiceInner { Ok(UploadRef { key, upload_id: Some(upload_id.clone()), + offset: 0, }) } @@ -918,9 +931,17 @@ impl ObjectstoreServiceInner { Ok(ObjectstoreKey(response.key)) } UploadAttempt::Stream { body, upload_ref } => { - let UploadRef { key, upload_id } = upload_ref; + let UploadRef { + key, + upload_id, + offset, + } = upload_ref; let Some(upload_id) = upload_id else { // No upload ID: simple upload in a single request. + if offset != 0 { + // Offset is only allowed for multipart. + todo!("raise error"); + } let request = session.put_stream(body.boxed()).key(key); let response = request.send().await?; return Ok(ObjectstoreKey(response.key)); @@ -929,6 +950,8 @@ impl ObjectstoreServiceInner { let multipart_upload = session.resume_multipart_upload(key, upload_id.to_string())?; + // TODO: map offset to compressed offset and vice versa. + let body = ReaderStream::new(ZstdEncoder::new(StreamReader::new(body))); // Unfortunately, MinIO has the limitation that the length of a multipart request @@ -1110,6 +1133,7 @@ mod tests { upload_ref: UploadRef { key: "my_file".to_owned(), upload_id: Some(UploadId::new("my_upload".to_owned()).unwrap()), + offset: 0, }, stream, }) diff --git a/relay-server/src/services/upload.rs b/relay-server/src/services/upload.rs index e713674bc27..4f12322a4d5 100644 --- a/relay-server/src/services/upload.rs +++ b/relay-server/src/services/upload.rs @@ -149,6 +149,8 @@ pub struct Stream { pub project: ProjectContext, /// The location to upload to. pub location: SignedLocation, + /// The offset from which to resume the upload. + pub offset: usize, /// The body to be uploaded to objectstore, with length validation. pub stream: BoundedStream>, } @@ -294,7 +296,11 @@ impl Service { let (key, upload_id) = match length { Some(0) => (key, None), // multipart does not allow empty uploads _ => { - let UploadRef { key, upload_id } = addr + let UploadRef { + key, + upload_id, + offset, + } = addr .send(objectstore::Create { organization_id, project_id, @@ -302,6 +308,7 @@ impl Service { }) .await .map_err(Error::ObjectstoreServiceUnavailable)??; + debug_assert_eq!(offset, 0); #[cfg(debug_assertions)] debug_assert_eq!(&key, &original_key); (key, upload_id) @@ -326,11 +333,13 @@ impl Service { received, project, location, + offset, stream, } = stream; match &self.backend { Backend::Upstream { addr } => { - let (request, rx) = UploadRequest::upload(project, location.try_to_uri()?, stream); + let (request, rx) = + UploadRequest::upload(project, location.try_to_uri()?, offset, stream); addr.send(SendRequest(request)); let response = rx.await??; SignedLocation::try_from_response(response) @@ -352,7 +361,7 @@ impl Service { debug_assert!(stream.length().is_none_or(|l| Some(l) == length.value())); let byte_counter = stream.byte_counter(); - let upload_ref = UploadRef::new(key, upload_id)?; + let upload_ref = UploadRef::new(key, upload_id, offset)?; let key = addr .send(objectstore::Stream { organization_id: scoping.organization_id, @@ -706,6 +715,7 @@ enum RequestKind { }, Upload { uri: String, + offset: usize, stream: TakeOnce>>, encoding: HttpEncoding, }, @@ -745,6 +755,7 @@ impl UploadRequest { fn upload( project: ProjectContext, uri: String, + offset: usize, stream: BoundedStream>, ) -> ( Self, @@ -756,6 +767,7 @@ impl UploadRequest { project, kind: RequestKind::Upload { uri, + offset, stream: TakeOnce::new(stream), encoding: HttpEncoding::Zstd, // just a default, will be overwritten by .configure() }, @@ -838,6 +850,7 @@ impl UpstreamRequest for UploadRequest { } RequestKind::Upload { uri: _, + offset, stream, encoding, } => { @@ -845,7 +858,7 @@ impl UpstreamRequest for UploadRequest { relay_log::error!("upload request stream was already consumed"); return Err(HttpError::Misconfigured); }; - tus::add_upload_headers(builder); + tus::add_upload_headers(builder, *offset); let body = encode_body(body, *encoding); builder.content_encoding(*encoding); diff --git a/relay-server/src/utils/tus.rs b/relay-server/src/utils/tus.rs index 1fc89bd83b6..d00c1c41ba7 100644 --- a/relay-server/src/utils/tus.rs +++ b/relay-server/src/utils/tus.rs @@ -30,8 +30,8 @@ pub enum Error { upload_defer_length: Option, }, /// The `Upload-Offset` header is missing or invalid - #[error("expected Upload-Offset: 0, got: {0:?}")] - UploadOffset(Option), + #[error("expected Upload-Offset >= 0")] + UploadOffset, /// The `Content-Type` header is not what TUS expects. #[error("expected Content-Type: {expected}, got: {received}")] ContentType { @@ -144,7 +144,9 @@ pub fn validate_post_headers(headers: &HeaderMap) -> Result { } /// Validates TUS protocol headers and returns the expected upload length. -pub fn validate_patch_headers(headers: &HeaderMap) -> Result<(), Error> { +/// +/// Returns the offset from which the upload is resumed. +pub fn validate_patch_headers(headers: &HeaderMap) -> Result { let tus_version = headers.get(TUS_RESUMABLE); if tus_version != Some(&TUS_VERSION) { return Err(Error::Version( @@ -163,14 +165,9 @@ pub fn validate_patch_headers(headers: &HeaderMap) -> Result<(), Error> { }); } - let upload_offset: usize = - parse_header(headers, UPLOAD_OFFSET).ok_or(Error::UploadOffset(None))?; - if upload_offset != 0 { - // Only allow full uploads for now. - return Err(Error::UploadOffset(Some(upload_offset))); - } + let upload_offset: usize = parse_header(headers, UPLOAD_OFFSET).ok_or(Error::UploadOffset)?; - Ok(()) + Ok(upload_offset) } /// Prepares the required TUS request headers for upstream requests. @@ -196,10 +193,10 @@ pub fn add_creation_headers( } /// Prepares the required TUS request headers for upstream requests. -pub fn add_upload_headers(builder: &mut RequestBuilder) { +pub fn add_upload_headers(builder: &mut RequestBuilder, offset: usize) { builder.header(TUS_RESUMABLE, TUS_VERSION); builder.header(http::header::CONTENT_TYPE, EXPECTED_CONTENT_TYPE); - builder.header(UPLOAD_OFFSET, "0"); // always zero until we implement retries / chunking + builder.header(UPLOAD_OFFSET, offset.to_string()); } /// Prepares the required TUS response headers. @@ -479,7 +476,7 @@ mod tests { headers.insert(TUS_RESUMABLE, HeaderValue::from_static("1.0.0")); headers.insert(http::header::CONTENT_TYPE, EXPECTED_CONTENT_TYPE); let result = validate_patch_headers(&headers); - assert!(matches!(result, Err(Error::UploadOffset(None)))); + assert!(matches!(result, Err(Error::UploadOffset))); } #[test] @@ -489,7 +486,7 @@ mod tests { headers.insert(http::header::CONTENT_TYPE, EXPECTED_CONTENT_TYPE); headers.insert(UPLOAD_OFFSET, HeaderValue::from_static("512")); let result = validate_patch_headers(&headers); - assert!(matches!(result, Err(Error::UploadOffset(Some(512))))); + assert!(matches!(result, Err(Error::UploadOffset))); } #[test] From ce6c7265283105a0dbe5f17a55c4b5daed23ced5 Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Fri, 10 Jul 2026 14:17:33 +0200 Subject: [PATCH 02/41] fix test --- tests/integration/test_upload.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/tests/integration/test_upload.py b/tests/integration/test_upload.py index 98867e8b9e3..3f62f37d044 100644 --- a/tests/integration/test_upload.py +++ b/tests/integration/test_upload.py @@ -424,9 +424,8 @@ def test_create_processing( assert response.headers["Upload-Offset"] == str(len(data)), response.headers -@pytest.mark.parametrize("length", [9, 11]) def test_processing_invalid_length( - mini_sentry, relay, relay_with_processing, project_config, length + mini_sentry, relay, relay_with_processing, project_config ): mini_sentry.fail_on_relay_error = False project_id = 42 @@ -447,8 +446,8 @@ def test_processing_invalid_length( assert response.headers["Tus-Resumable"] == "1.0.0" assert "Upload-Offset" not in response.headers - # Use the location to send a PATCH request that is too long // too short - data = length * b"X" + # Use the location to send a PATCH request that is too long + data = 11 * b"X" response = relay.patch( f"{response.headers['Location']}&sentry_key={project_key}", headers={ From d235437f6483b0cc613fb1be1bbed4aead00d975 Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Fri, 10 Jul 2026 16:36:06 +0200 Subject: [PATCH 03/41] test: show parts are compressed --- relay-server/src/endpoints/upload.rs | 2 +- relay-server/src/services/objectstore.rs | 111 ++++++++++++++++------- relay-server/src/services/upload.rs | 35 ++++--- tests/integration/test_upload.py | 56 +++++++++++- 4 files changed, 154 insertions(+), 50 deletions(-) diff --git a/relay-server/src/endpoints/upload.rs b/relay-server/src/endpoints/upload.rs index e43723c7089..45b0c6e891b 100644 --- a/relay-server/src/endpoints/upload.rs +++ b/relay-server/src/endpoints/upload.rs @@ -340,7 +340,7 @@ async fn upload( location: SignedLocation, offset: usize, stream: BoundedStream>, -) -> Result, Error> { +) -> Result, Error> { let location = state .upload() .send(upload::Stream { diff --git a/relay-server/src/services/objectstore.rs b/relay-server/src/services/objectstore.rs index fcdae9ffa6f..cd50a73efdf 100644 --- a/relay-server/src/services/objectstore.rs +++ b/relay-server/src/services/objectstore.rs @@ -36,7 +36,8 @@ use crate::services::store::{ use crate::services::upload::ByteStream; use crate::statsd::{RelayCounters, RelayTimers}; use crate::utils::{ - BoundedStream, MeteredStream, Rechunk, RetryableStream, TakeOnce, find_error_source, + BoundedStream, ByteCounter, MeteredStream, Rechunk, RetryableStream, TakeOnce, + find_error_source, }; use super::outcome::Outcome; @@ -51,7 +52,7 @@ pub enum Objectstore { EventAttachment(Managed), RawProfile(Managed), Create(Create, Sender>), - Stream(Stream, Sender>), + Stream(Stream, Sender>), } impl Objectstore { @@ -163,9 +164,9 @@ pub struct Stream { } impl FromMessage for Objectstore { - type Response = AsyncResponse>; + type Response = AsyncResponse>; - fn from_message(message: Stream, sender: Sender>) -> Self { + fn from_message(message: Stream, sender: Sender>) -> Self { Self::Stream(message, sender) } } @@ -334,10 +335,15 @@ pub struct UploadRef { /// The ID of the multipart upload session (chosen by objectstore). /// `None` if the upload is not multipart. pub upload_id: Option, - /// The offset from which to resume an upload. + /// The offset in bytes from which to resume an upload. /// /// Zero for new uploads. pub offset: usize, + + /// The total length of the upload in bytes. + /// + /// Must be `Some` in order to finalize an upload. + pub length: Option, } impl UploadRef { @@ -346,6 +352,7 @@ impl UploadRef { key: String, upload_id: Option, offset: usize, + length: Option, ) -> Result { let upload_id = match upload_id { Some(s) => Some(UploadId::new(s)?), @@ -355,6 +362,7 @@ impl UploadRef { key, upload_id, offset, + length, }) } } @@ -584,8 +592,8 @@ impl ObjectstoreServiceInner { .await; match result { - Ok(stored_key) => { - attachment.modify(|a, _| a.set_stored_key(stored_key.into_inner())); + Ok(UploadRef { key, .. }) => { + attachment.modify(|a, _| a.set_stored_key(key)); } Err(error) => { error.log(MessageKind::Event); @@ -638,11 +646,9 @@ impl ObjectstoreServiceInner { }; match upload_result { - Ok(stored_key) => { + Ok(UploadRef { key, .. }) => { attachment.modify(|attachment, _| { - attachment - .attachment - .set_stored_key(stored_key.into_inner()); + attachment.attachment.set_stored_key(key); }); self.store.send(attachment); } @@ -695,7 +701,7 @@ impl ObjectstoreServiceInner { #[cfg(debug_assertions)] let original_key = key.clone(); - let _stored_key = self + let _upload_ref = self .upload_bytes( MessageKind::TraceAttachment, &session, @@ -708,7 +714,7 @@ impl ObjectstoreServiceInner { .reject(&trace_item)?; #[cfg(debug_assertions)] - debug_assert_eq!(_stored_key.into_inner(), original_key); + debug_assert_eq!(_upload_ref.key, original_key); } // Only after successful upload forward the attachment to the store. @@ -730,12 +736,12 @@ impl ObjectstoreServiceInner { .try_upload_raw_profile(payload, content_type, store_message.retention_days, scoping) .await { - Ok(Some(stored_id)) => { + Ok(Some(UploadRef { key, .. })) => { store_message.modify(|message, _| { message.attachments.push(ProfileAttachment { name, content_type, - stored_id, + stored_id: ObjectstoreKey(key), }) }); } @@ -756,7 +762,7 @@ impl ObjectstoreServiceInner { content_type: ContentType, retention: u16, scoping: Scoping, - ) -> Result, Error> { + ) -> Result, Error> { if payload.is_empty() { return Ok(None); } @@ -766,7 +772,7 @@ impl ObjectstoreServiceInner { .for_project(scoping.organization_id.value(), scoping.project_id.value()) .session(&self.objectstore_client)?; - let stored_key = self + let upload_ref = self .upload_bytes( MessageKind::RawProfile, &session, @@ -777,7 +783,7 @@ impl ObjectstoreServiceInner { ) .await?; - Ok(Some(stored_key)) + Ok(Some(upload_ref)) } async fn handle_create(&self, create: Create) -> Result { @@ -801,10 +807,11 @@ impl ObjectstoreServiceInner { key, upload_id: Some(upload_id.clone()), offset: 0, + length: None, }) } - async fn handle_stream(&self, stream: Stream) -> Result { + async fn handle_stream(&self, stream: Stream) -> Result { let Stream { organization_id, project_id, @@ -813,11 +820,13 @@ impl ObjectstoreServiceInner { } = stream; let session = self.session(&self.event_attachments, organization_id, project_id)?; + let byte_counter = stream.byte_counter(); self.upload( MessageKind::Stream, &session, Upload::Stream { body: TakeOnce::new(stream), + byte_counter, upload_ref, }, ) @@ -832,7 +841,7 @@ impl ObjectstoreServiceInner { retention: u16, key: Option, content_type: Option, - ) -> Result { + ) -> Result { let retention_hours = retention.checked_mul(24); self.upload( kind, @@ -852,7 +861,7 @@ impl ObjectstoreServiceInner { kind: MessageKind, session: &Session, body: Upload, - ) -> Result { + ) -> Result { let mut attempts = 0; let timeout = match &body { Upload::Bytes { .. } => self.timeout, @@ -901,7 +910,7 @@ impl ObjectstoreServiceInner { kind: MessageKind, session: &Session, body: UploadAttempt, - ) -> Result { + ) -> Result { match body { UploadAttempt::Bytes { body, @@ -909,6 +918,7 @@ impl ObjectstoreServiceInner { retention_hours, content_type, } => { + let len = body.len(); let mut request = session.put(body); if let Some(content_type) = content_type { request = request.content_type(content_type.as_str()); @@ -928,14 +938,26 @@ impl ObjectstoreServiceInner { request.send().await? }); - Ok(ObjectstoreKey(response.key)) + Ok(UploadRef { + key: response.key, + upload_id: None, + offset: len, + length: Some(len), + }) } - UploadAttempt::Stream { body, upload_ref } => { + UploadAttempt::Stream { + body, + byte_counter, + upload_ref, + } => { let UploadRef { key, upload_id, offset, + length, } = upload_ref; + let original_key = key.clone(); + let Some(upload_id) = upload_id else { // No upload ID: simple upload in a single request. if offset != 0 { @@ -944,11 +966,17 @@ impl ObjectstoreServiceInner { } let request = session.put_stream(body.boxed()).key(key); let response = request.send().await?; - return Ok(ObjectstoreKey(response.key)); + return Ok(UploadRef { + key: response.key, + upload_id, + offset, + length, + }); }; let multipart_upload = session.resume_multipart_upload(key, upload_id.to_string())?; + let upload_id = multipart_upload.upload_id().clone(); // TODO: map offset to compressed offset and vice versa. @@ -960,7 +988,7 @@ impl ObjectstoreServiceInner { let chunks = Rechunk::new(body, CHUNK_SIZE); let mut body = chunks.enumerate(); - let result = relay_statsd::metric!( + relay_statsd::metric!( timer(RelayTimers::AttachmentUploadDuration), type = kind.as_str(), { @@ -974,10 +1002,19 @@ impl ObjectstoreServiceInner { let part = multipart_upload.put(chunk, part_number, None).await?; parts.push(part); } - multipart_upload.complete(parts).await? + + if length.is_some_and(|length| length == byte_counter.get()) { + let final_key = multipart_upload.complete(parts).await?; + debug_assert_eq!(&original_key, &final_key); + } }); - Ok(ObjectstoreKey(result)) + Ok(UploadRef { + key: original_key, + upload_id: Some(upload_id), + offset: byte_counter.get(), + length, + }) } } } @@ -1010,6 +1047,7 @@ enum Upload { }, Stream { body: TakeOnce>>, + byte_counter: ByteCounter, upload_ref: UploadRef, }, } @@ -1028,12 +1066,15 @@ impl Upload { retention_hours: *retention_hours, content_type: *content_type, }), - Self::Stream { body, upload_ref } => { - RetryableStream::new(body.clone()).map(|body| UploadAttempt::Stream { - body, - upload_ref: upload_ref.clone(), - }) - } + Self::Stream { + body, + byte_counter, + upload_ref, + } => RetryableStream::new(body.clone()).map(|body| UploadAttempt::Stream { + body, + byte_counter: byte_counter.clone(), + upload_ref: upload_ref.clone(), + }), } } } @@ -1050,6 +1091,7 @@ enum UploadAttempt { }, Stream { body: RetryableStream>>, + byte_counter: ByteCounter, upload_ref: UploadRef, }, } @@ -1134,6 +1176,7 @@ mod tests { key: "my_file".to_owned(), upload_id: Some(UploadId::new("my_upload".to_owned()).unwrap()), offset: 0, + length: None, }, stream, }) diff --git a/relay-server/src/services/upload.rs b/relay-server/src/services/upload.rs index 4f12322a4d5..8fdc3e3b6f4 100644 --- a/relay-server/src/services/upload.rs +++ b/relay-server/src/services/upload.rs @@ -112,7 +112,7 @@ pub enum Upload { /// /// The service also returns the signed location. This is redundant, but creates a simpler /// flow for the caller side. - Upload(Stream, InstrumentedSender), + Upload(Stream, InstrumentedSender), } impl Interface for Upload {} @@ -173,9 +173,12 @@ impl FromMessage for Upload { } impl FromMessage for Upload { - type Response = AsyncResponse, Error>>; + type Response = AsyncResponse, Error>>; - fn from_message(message: Stream, sender: Sender, Error>>) -> Self { + fn from_message( + message: Stream, + sender: Sender, Error>>, + ) -> Self { Self::Upload( message, InstrumentedSender { @@ -300,6 +303,7 @@ impl Service { key, upload_id, offset, + length: _, } = addr .send(objectstore::Create { organization_id, @@ -327,7 +331,7 @@ impl Service { } } - async fn upload(&self, stream: Stream) -> Result, Error> { + async fn upload(&self, stream: Stream) -> Result, Error> { let Stream { #[cfg_attr(not(feature = "processing"), expect(unused))] received, @@ -346,6 +350,8 @@ impl Service { } #[cfg(feature = "processing")] Backend::Objectstore { addr, config } => { + use objectstore_client::UploadId; + use crate::services::objectstore::UploadRef; let Location { @@ -361,8 +367,8 @@ impl Service { debug_assert!(stream.length().is_none_or(|l| Some(l) == length.value())); let byte_counter = stream.byte_counter(); - let upload_ref = UploadRef::new(key, upload_id, offset)?; - let key = addr + let upload_ref = UploadRef::new(key, upload_id, offset, length.value())?; + let upload_ref = addr .send(objectstore::Stream { organization_id: scoping.organization_id, project_id, @@ -370,15 +376,20 @@ impl Service { stream, }) .await - .map_err(Error::ObjectstoreServiceUnavailable)?? - .into_inner(); - let length = Final(byte_counter.get()); + .map_err(Error::ObjectstoreServiceUnavailable)??; + + let UploadRef { + key, + upload_id, + offset: _, + length, + } = upload_ref; Location { project_id, key, - length, - upload_id: None, + length: Provisional(length), + upload_id: upload_id.map(|id| id.to_string()), other, } .try_sign(config) @@ -499,7 +510,7 @@ impl Location { upload_id: upload_id.as_deref(), other, }; - let query = serde_urlencoded::to_string(params)?; + let query = dbg!(serde_urlencoded::to_string(params))?; match query.as_str() { "" => Ok(format!("/api/{project_id}/upload/{key}/")), _ => Ok(format!("/api/{project_id}/upload/{key}/?{query}")), diff --git a/tests/integration/test_upload.py b/tests/integration/test_upload.py index 3f62f37d044..e62772086a3 100644 --- a/tests/integration/test_upload.py +++ b/tests/integration/test_upload.py @@ -2,20 +2,25 @@ Tests for the TUS upload endpoint (/api/{project_id}/upload/). """ +import re import time import uuid from concurrent.futures import ThreadPoolExecutor, as_completed from flask import Response +from objectstore_client.multipart import MultipartUpload import pytest from sentry_relay.auth import SecretKey +from .asserts import matches_any from .consts import ( DUMMY_UPLOAD_PATH, DUMMY_UPLOAD_LOCATION, ) +LOCATION_REGEX = re.compile(r"/api/\d+/upload/(\w+)/?.*upload_id=([\w-]+)") + @pytest.fixture def project_config(mini_sentry): @@ -654,8 +659,14 @@ def multipart_upload(**opts): assert response.status_code == 500 # not 504 -def upload_something(relay, project_id, project_key): - data = b"hello world" +def test_objectstore_compression( + mini_sentry, relay_with_processing, project_config, objectstore +): + project_id = 42 + project_key = mini_sentry.get_dsn_public_key(project_id) + relay = relay_with_processing() + + data = 1_000_000 * b"X" response = relay.post( f"/api/{project_id}/upload/?sentry_key={project_key}", headers={ @@ -666,7 +677,9 @@ def upload_something(relay, project_id, project_key): ) assert response.status_code == 201, response.json() - return relay.patch( + # Upload only half the bytes + data = data[: len(data) // 2] + response = relay.patch( f"{response.headers['Location']}&sentry_key={project_key}", headers={ "Content-Length": str(len(data)), @@ -676,6 +689,19 @@ def upload_something(relay, project_id, project_key): }, data=data, ) + assert response.status_code == 204 + + objectstore = objectstore("attachments", project_id) + + key, upload_id = LOCATION_REGEX.match(response.headers["Location"]).groups() + (part,) = MultipartUpload(objectstore, key, upload_id).list_parts() + + assert vars(part) == { + "part_number": 1, + "etag": matches_any(), + "last_modified": matches_any(), + "size": len(data), + } @pytest.mark.parametrize( @@ -740,3 +766,27 @@ def test_upload_minidump_opt_in( ) else: assert mini_sentry.captured_outcomes.empty() + + +def upload_something(relay, project_id, project_key): + data = b"hello world" + response = relay.post( + f"/api/{project_id}/upload/?sentry_key={project_key}", + headers={ + "Content-Length": "0", + "Tus-Resumable": "1.0.0", + "Upload-Length": str(len(data)), + }, + ) + assert response.status_code == 201, response.json() + + return relay.patch( + f"{response.headers['Location']}&sentry_key={project_key}", + headers={ + "Content-Length": str(len(data)), + "Content-Type": "application/offset+octet-stream", + "Tus-Resumable": "1.0.0", + "Upload-Offset": "0", + }, + data=data, + ) From 5bf53ea4eb753ed31df094798edd45c1a87ec364 Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Fri, 10 Jul 2026 16:36:46 +0200 Subject: [PATCH 04/41] clean --- relay-server/src/endpoints/upload.rs | 2 +- relay-server/src/services/upload.rs | 4 ---- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/relay-server/src/endpoints/upload.rs b/relay-server/src/endpoints/upload.rs index 45b0c6e891b..f48f1664496 100644 --- a/relay-server/src/endpoints/upload.rs +++ b/relay-server/src/endpoints/upload.rs @@ -31,7 +31,7 @@ use crate::services::objectstore; use crate::services::projects::cache::Project; use crate::services::projects::project::ProjectState; use crate::services::upload::{ - self, ByteStream, Final, LocationQueryParams, ProjectContext, Provisional, SignedLocation, + self, ByteStream, LocationQueryParams, ProjectContext, Provisional, SignedLocation, UploadLength, }; use crate::services::upstream::UpstreamRequestError; diff --git a/relay-server/src/services/upload.rs b/relay-server/src/services/upload.rs index 8fdc3e3b6f4..12c9d156432 100644 --- a/relay-server/src/services/upload.rs +++ b/relay-server/src/services/upload.rs @@ -350,8 +350,6 @@ impl Service { } #[cfg(feature = "processing")] Backend::Objectstore { addr, config } => { - use objectstore_client::UploadId; - use crate::services::objectstore::UploadRef; let Location { @@ -365,8 +363,6 @@ impl Service { let scoping = project.scoping; debug_assert_eq!(scoping.project_id, project_id); debug_assert!(stream.length().is_none_or(|l| Some(l) == length.value())); - let byte_counter = stream.byte_counter(); - let upload_ref = UploadRef::new(key, upload_id, offset, length.value())?; let upload_ref = addr .send(objectstore::Stream { From e23db713d9248aadf877f1875d897775f486060a Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Fri, 10 Jul 2026 16:38:41 +0200 Subject: [PATCH 05/41] wip: disable compression --- relay-server/src/services/objectstore.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/relay-server/src/services/objectstore.rs b/relay-server/src/services/objectstore.rs index cd50a73efdf..822d35dbbf2 100644 --- a/relay-server/src/services/objectstore.rs +++ b/relay-server/src/services/objectstore.rs @@ -797,7 +797,7 @@ impl ObjectstoreServiceInner { let multipart_upload = session .initiate_multipart_upload() .key(&key) - .compression(Compression::Zstd) // make explicit because parts need to be manually compressed. + .compression(None) // needed to map offsets to parts .send() .await?; debug_assert_eq!(&key, multipart_upload.key()); @@ -978,9 +978,7 @@ impl ObjectstoreServiceInner { session.resume_multipart_upload(key, upload_id.to_string())?; let upload_id = multipart_upload.upload_id().clone(); - // TODO: map offset to compressed offset and vice versa. - - let body = ReaderStream::new(ZstdEncoder::new(StreamReader::new(body))); + // FIXME: how do we handle compression? // Unfortunately, MinIO has the limitation that the length of a multipart request // has to be known. Therefore, we need to materialize the stream into concrete From 5024e876b6991947372a35ae9eb00cf9bb55ea81 Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Fri, 10 Jul 2026 16:44:03 +0200 Subject: [PATCH 06/41] lint --- relay-server/src/services/objectstore.rs | 5 +---- relay-server/src/services/upload.rs | 8 ++++---- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/relay-server/src/services/objectstore.rs b/relay-server/src/services/objectstore.rs index 822d35dbbf2..76c67f0e723 100644 --- a/relay-server/src/services/objectstore.rs +++ b/relay-server/src/services/objectstore.rs @@ -5,13 +5,11 @@ use std::num::{NonZeroU16, NonZeroUsize}; use std::sync::Arc; use std::time::Duration; -use async_compression::tokio::bufread::ZstdEncoder; use bytes::Bytes; use futures::StreamExt; use http::StatusCode; use objectstore_client::{ - Client, Compression, ExpirationPolicy, SecretKey as SigningKey, Session, TokenGenerator, - UploadId, Usecase, + Client, ExpirationPolicy, SecretKey as SigningKey, Session, TokenGenerator, UploadId, Usecase, }; use objectstore_types::multipart::InvalidUploadId; @@ -23,7 +21,6 @@ use relay_system::{ Addr, AsyncResponse, FromMessage, Interface, LoadShed, NoResponse, Sender, SimpleService, }; use sentry_protos::snuba::v1::TraceItem; -use tokio_util::io::{ReaderStream, StreamReader}; use crate::constants::DEFAULT_ATTACHMENT_RETENTION; use crate::envelope::{ContentType, Item, ItemType}; diff --git a/relay-server/src/services/upload.rs b/relay-server/src/services/upload.rs index 12c9d156432..6c878910495 100644 --- a/relay-server/src/services/upload.rs +++ b/relay-server/src/services/upload.rs @@ -112,7 +112,7 @@ pub enum Upload { /// /// The service also returns the signed location. This is redundant, but creates a simpler /// flow for the caller side. - Upload(Stream, InstrumentedSender), + Upload(Box, InstrumentedSender), } impl Interface for Upload {} @@ -180,7 +180,7 @@ impl FromMessage for Upload { sender: Sender, Error>>, ) -> Self { Self::Upload( - message, + Box::new(message), InstrumentedSender { metric: RelayCounters::UploadUpload, inner: sender, @@ -411,7 +411,7 @@ impl SimpleService for Service { sender.send(self.timeout(self.create(create)).await); } Upload::Upload(stream, sender) => { - sender.send(self.timeout(self.upload(stream)).await); + sender.send(self.timeout(self.upload(*stream)).await); } } } @@ -506,7 +506,7 @@ impl Location { upload_id: upload_id.as_deref(), other, }; - let query = dbg!(serde_urlencoded::to_string(params))?; + let query = serde_urlencoded::to_string(params)?; match query.as_str() { "" => Ok(format!("/api/{project_id}/upload/{key}/")), _ => Ok(format!("/api/{project_id}/upload/{key}/?{query}")), From 67adbf0accc1565dbaa999af9813a16c333e7482 Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Sun, 12 Jul 2026 21:10:45 +0200 Subject: [PATCH 07/41] wip: compression --- relay-server/src/services/objectstore.rs | 89 +++++++++++++++++++----- 1 file changed, 71 insertions(+), 18 deletions(-) diff --git a/relay-server/src/services/objectstore.rs b/relay-server/src/services/objectstore.rs index 76c67f0e723..759c8e0bab0 100644 --- a/relay-server/src/services/objectstore.rs +++ b/relay-server/src/services/objectstore.rs @@ -1,15 +1,19 @@ //! Objectstore service for uploading attachments. use std::array::TryFromSliceError; use std::fmt; +use std::io::Write; use std::num::{NonZeroU16, NonZeroUsize}; use std::sync::Arc; use std::time::Duration; -use bytes::Bytes; +use async_compression::codecs::ZstdDecoder; +use async_compression::tokio::bufread::ZstdEncoder; +use bytes::{BufMut, Bytes, BytesMut}; use futures::StreamExt; use http::StatusCode; use objectstore_client::{ - Client, ExpirationPolicy, SecretKey as SigningKey, Session, TokenGenerator, UploadId, Usecase, + Client, Compression, ExpirationPolicy, SecretKey as SigningKey, Session, TokenGenerator, + UploadId, Usecase, }; use objectstore_types::multipart::InvalidUploadId; @@ -21,6 +25,7 @@ use relay_system::{ Addr, AsyncResponse, FromMessage, Interface, LoadShed, NoResponse, Sender, SimpleService, }; use sentry_protos::snuba::v1::TraceItem; +use tokio_util::io::{ReaderStream, StreamReader}; use crate::constants::DEFAULT_ATTACHMENT_RETENTION; use crate::envelope::{ContentType, Item, ItemType}; @@ -39,8 +44,15 @@ use crate::utils::{ use super::outcome::Outcome; -/// Size of an individual request to objectstore. -const CHUNK_SIZE: NonZeroUsize = NonZeroUsize::new(5 * 1024 * 1024).unwrap(); +/// 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 + +/// Every part except the last needs to be at least this size. +const MULTIPART_MIN_PART_SIZE: usize = 5 * 1024 * 1024; // 5 MiB + +// const CHUNK_SIZE: NonZeroUsize = NonZeroUsize::new(5 * 1024 * 1024).unwrap(); /// Messages that the objectstore service can handle. pub enum Objectstore { @@ -794,7 +806,7 @@ impl ObjectstoreServiceInner { let multipart_upload = session .initiate_multipart_upload() .key(&key) - .compression(None) // needed to map offsets to parts + .compression(Compression::Zstd) // make explicit because parts need to be manually compressed. .send() .await?; debug_assert_eq!(&key, multipart_upload.key()); @@ -975,29 +987,62 @@ impl ObjectstoreServiceInner { session.resume_multipart_upload(key, upload_id.to_string())?; let upload_id = multipart_upload.upload_id().clone(); - // FIXME: how do we handle compression? - - // Unfortunately, MinIO has the limitation that the length of a multipart request - // has to be known. Therefore, we need to materialize the stream into concrete - // chunks of bytes and send each chunk as an individual request. - let chunks = Rechunk::new(body, CHUNK_SIZE); - let mut body = chunks.enumerate(); - relay_statsd::metric!( timer(RelayTimers::AttachmentUploadDuration), type = kind.as_str(), { - let mut parts = vec![]; + // Ensure that the stream ends at the granularity boundary. // NOTE: Once every upload is a multipart upload, we can remove `RetryableStream` // because streams will never be effectively retried. - while let Some((i, chunk)) = body.next().await { - let chunk = chunk?; - let part_number = u32::try_from(i + 1) + let mut body = Rechunk::new(body, UPLOAD_GRANULARITY).enumerate(); + // let body = ReaderStream::new(ZstdEncoder::new(StreamReader::new(body))); + + // Unfortunately, MinIO has the limitation that the length of a multipart request + // has to be known. Therefore, we need to materialize the stream into concrete + // chunks of bytes and send each chunk as an individual request. + // + // Every chunk needs to be > 5 MB because that's what multipart requires. + let mut parts = vec![]; + let mut compressed_buffer = zstd::stream::write::Encoder::new(vec![], 0).unwrap(); // FIXME; + let mut error = None; + + let mut last_grain_index = 0; + while let Some((grain_index, bytes)) = body.next().await { + last_grain_index = grain_index; + match bytes { + Ok(bytes) => { + compressed_buffer.write_all(&bytes).unwrap(); // FIXME + + let done = length.is_some_and(|length| length == byte_counter.get()); + if done || compressed_buffer.get_ref().len() >= MULTIPART_MIN_PART_SIZE { + let part_number = u32::try_from(grain_index + 1) + .map_err(|_| objectstore_client::Error::InvalidPartNumber(u32::MAX))?; + + let mut new_buffer = zstd::stream::write::Encoder::new(vec![], 0).unwrap(); // FIXME; + std::mem::swap(&mut compressed_buffer, &mut new_buffer); + let part = multipart_upload.put(new_buffer.finish().expect("FIXME"),part_number, None).await?; + parts.push(part); + } + } + Err(e) => {error = Some(e); break} + } + } + + let done = length.is_some_and(|length| length == byte_counter.get()); + if done || compressed_buffer.get_ref().len() >= MULTIPART_MIN_PART_SIZE { + let part_number = u32::try_from(last_grain_index + 1) .map_err(|_| objectstore_client::Error::InvalidPartNumber(u32::MAX))?; - let part = multipart_upload.put(chunk, part_number, None).await?; + + let mut new_buffer = zstd::stream::write::Encoder::new(vec![], 0).unwrap(); // FIXME; + std::mem::swap(&mut compressed_buffer, &mut new_buffer); + let part = multipart_upload.put(new_buffer.finish().expect("FIXME"),part_number, None).await?; parts.push(part); } + if let Some(e) = error { + Err(e).map_err(objectstore_client::Error::from)?; + } + if length.is_some_and(|length| length == byte_counter.get()) { let final_key = multipart_upload.complete(parts).await?; debug_assert_eq!(&original_key, &final_key); @@ -1030,6 +1075,14 @@ impl ObjectstoreServiceInner { } } +#[derive(Debug, thiserror::Error)] +enum AttemptUploadError { + #[error(transparent)] + Objectstore(#[from] objectstore_client::Error), + #[error("zstd error: {0}")] + Zstd(#[source] std::io::Error), +} + /// Common interface for calls to [`ObjectstoreServiceInner::upload`]. /// /// This type is shared across retries. From a0a080d8a7831b2413ccbc8e9d12b2be972fb75d Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Mon, 13 Jul 2026 21:57:02 +0200 Subject: [PATCH 08/41] compile --- relay-server/src/endpoints/upload.rs | 3 + relay-server/src/services/objectstore.rs | 95 ++++++++++++++---------- 2 files changed, 58 insertions(+), 40 deletions(-) diff --git a/relay-server/src/endpoints/upload.rs b/relay-server/src/endpoints/upload.rs index f48f1664496..5fc95a7518c 100644 --- a/relay-server/src/endpoints/upload.rs +++ b/relay-server/src/endpoints/upload.rs @@ -117,6 +117,9 @@ impl IntoResponse for Error { objectstore::ErrorKind::InvalidScoping => StatusCode::INTERNAL_SERVER_ERROR, objectstore::ErrorKind::Timeout(_) => StatusCode::GATEWAY_TIMEOUT, objectstore::ErrorKind::LoadShed => StatusCode::SERVICE_UNAVAILABLE, + objectstore::ErrorKind::CompressionFailed(_) => { + StatusCode::INTERNAL_SERVER_ERROR + } objectstore::ErrorKind::UploadFailed(error) => match error { objectstore_client::Error::Io(error) if is_upload_length_error(&error) => { StatusCode::BAD_REQUEST diff --git a/relay-server/src/services/objectstore.rs b/relay-server/src/services/objectstore.rs index 759c8e0bab0..8efbf474c17 100644 --- a/relay-server/src/services/objectstore.rs +++ b/relay-server/src/services/objectstore.rs @@ -6,14 +6,12 @@ use std::num::{NonZeroU16, NonZeroUsize}; use std::sync::Arc; use std::time::Duration; -use async_compression::codecs::ZstdDecoder; -use async_compression::tokio::bufread::ZstdEncoder; -use bytes::{BufMut, Bytes, BytesMut}; +use bytes::Bytes; use futures::StreamExt; use http::StatusCode; use objectstore_client::{ - Client, Compression, ExpirationPolicy, SecretKey as SigningKey, Session, TokenGenerator, - UploadId, Usecase, + Client, CompletePart, Compression, ExpirationPolicy, MultipartUpload, SecretKey as SigningKey, + Session, TokenGenerator, UploadId, Usecase, }; use objectstore_types::multipart::InvalidUploadId; @@ -25,7 +23,6 @@ use relay_system::{ Addr, AsyncResponse, FromMessage, Interface, LoadShed, NoResponse, Sender, SimpleService, }; use sentry_protos::snuba::v1::TraceItem; -use tokio_util::io::{ReaderStream, StreamReader}; use crate::constants::DEFAULT_ATTACHMENT_RETENTION; use crate::envelope::{ContentType, Item, ItemType}; @@ -290,6 +287,8 @@ pub enum ErrorKind { Timeout(#[from] tokio::time::error::Elapsed), #[error("load shed")] LoadShed, + #[error("compression failed: {0}")] + CompressionFailed(#[source] std::io::Error), #[error("upload failed: {0}")] UploadFailed(#[from] objectstore_client::Error), #[error("UUID conversion failed: {0}")] @@ -302,6 +301,7 @@ impl ErrorKind { Self::InvalidScoping => "invalid_scoping", Self::Timeout(_) => "timeout", Self::LoadShed => "load_shed", + Self::CompressionFailed(_) => "compression_failed", Self::UploadFailed(_) => "upload_failed", Self::Uuid(_) => "uuid", } @@ -919,7 +919,7 @@ impl ObjectstoreServiceInner { kind: MessageKind, session: &Session, body: UploadAttempt, - ) -> Result { + ) -> Result { match body { UploadAttempt::Bytes { body, @@ -983,7 +983,7 @@ impl ObjectstoreServiceInner { }); }; - let multipart_upload = + let mut multipart_upload = session.resume_multipart_upload(key, upload_id.to_string())?; let upload_id = multipart_upload.upload_id().clone(); @@ -1004,49 +1004,29 @@ impl ObjectstoreServiceInner { // Every chunk needs to be > 5 MB because that's what multipart requires. let mut parts = vec![]; let mut compressed_buffer = zstd::stream::write::Encoder::new(vec![], 0).unwrap(); // FIXME; - let mut error = None; - let mut last_grain_index = 0; while let Some((grain_index, bytes)) = body.next().await { - last_grain_index = grain_index; match bytes { - Ok(bytes) => { - compressed_buffer.write_all(&bytes).unwrap(); // FIXME - - let done = length.is_some_and(|length| length == byte_counter.get()); - if done || compressed_buffer.get_ref().len() >= MULTIPART_MIN_PART_SIZE { - let part_number = u32::try_from(grain_index + 1) - .map_err(|_| objectstore_client::Error::InvalidPartNumber(u32::MAX))?; - - let mut new_buffer = zstd::stream::write::Encoder::new(vec![], 0).unwrap(); // FIXME; - std::mem::swap(&mut compressed_buffer, &mut new_buffer); - let part = multipart_upload.put(new_buffer.finish().expect("FIXME"),part_number, None).await?; - parts.push(part); + Err(error) => { + try_flush(&mut compressed_buffer, grain_index, &mut multipart_upload, &mut parts).await?; + return Err(objectstore_client::Error::Io(error).into()); + } Ok(bytes) => { + compressed_buffer.write_all(&bytes).map_err(AttemptUploadError::Zstd)?; + if compressed_buffer.get_ref().len() > MULTIPART_MIN_PART_SIZE { + try_flush(&mut compressed_buffer, grain_index, &mut multipart_upload, &mut parts).await?; } } - Err(e) => {error = Some(e); break} } + last_grain_index = grain_index; } - let done = length.is_some_and(|length| length == byte_counter.get()); - if done || compressed_buffer.get_ref().len() >= MULTIPART_MIN_PART_SIZE { - let part_number = u32::try_from(last_grain_index + 1) - .map_err(|_| objectstore_client::Error::InvalidPartNumber(u32::MAX))?; - - let mut new_buffer = zstd::stream::write::Encoder::new(vec![], 0).unwrap(); // FIXME; - std::mem::swap(&mut compressed_buffer, &mut new_buffer); - let part = multipart_upload.put(new_buffer.finish().expect("FIXME"),part_number, None).await?; - parts.push(part); - } - - if let Some(e) = error { - Err(e).map_err(objectstore_client::Error::from)?; - } + try_flush(&mut compressed_buffer, last_grain_index, &mut multipart_upload, &mut parts).await?; if length.is_some_and(|length| length == byte_counter.get()) { let final_key = multipart_upload.complete(parts).await?; debug_assert_eq!(&original_key, &final_key); } + }); Ok(UploadRef { @@ -1075,6 +1055,32 @@ impl ObjectstoreServiceInner { } } +async fn try_flush( + buffer: &mut zstd::stream::write::Encoder<'_, Vec>, + grain_index: usize, + multipart: &mut MultipartUpload, + parts: &mut Vec, +) -> Result<(), AttemptUploadError> { + let mut new_buffer = + zstd::stream::write::Encoder::new(vec![], 0).map_err(AttemptUploadError::Zstd)?; + std::mem::swap(buffer, &mut new_buffer); + + let part_number = u32::try_from(grain_index + 1) + .map_err(|_| objectstore_client::Error::InvalidPartNumber(u32::MAX))?; + + let part = multipart + .put( + new_buffer.finish().map_err(AttemptUploadError::Zstd)?, + part_number, + None, + ) + .await?; + + parts.push(part); + + Ok(()) +} + #[derive(Debug, thiserror::Error)] enum AttemptUploadError { #[error(transparent)] @@ -1083,6 +1089,15 @@ enum AttemptUploadError { Zstd(#[source] std::io::Error), } +impl From for ErrorKind { + fn from(value: AttemptUploadError) -> Self { + match value { + AttemptUploadError::Objectstore(error) => ErrorKind::UploadFailed(error), + AttemptUploadError::Zstd(error) => ErrorKind::CompressionFailed(error), + } + } +} + /// Common interface for calls to [`ObjectstoreServiceInner::upload`]. /// /// This type is shared across retries. @@ -1144,9 +1159,9 @@ enum UploadAttempt { }, } -fn is_retryable(error: &objectstore_client::Error) -> bool { +fn is_retryable(error: &AttemptUploadError) -> bool { match error { - objectstore_client::Error::Reqwest(error) => { + AttemptUploadError::Objectstore(objectstore_client::Error::Reqwest(error)) => { error.is_connect() || error.is_timeout() || matches!( From 16b9503bcc045fd69ba749989b26ef2a281f2b6d Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Mon, 13 Jul 2026 22:32:42 +0200 Subject: [PATCH 09/41] wip --- relay-server/src/services/objectstore.rs | 18 ++++---- tests/integration/test_upload.py | 58 ++++++++++++++++++------ 2 files changed, 53 insertions(+), 23 deletions(-) diff --git a/relay-server/src/services/objectstore.rs b/relay-server/src/services/objectstore.rs index 8efbf474c17..7908d38913d 100644 --- a/relay-server/src/services/objectstore.rs +++ b/relay-server/src/services/objectstore.rs @@ -994,7 +994,7 @@ impl ObjectstoreServiceInner { // Ensure that the stream ends at the granularity boundary. // NOTE: Once every upload is a multipart upload, we can remove `RetryableStream` // because streams will never be effectively retried. - let mut body = Rechunk::new(body, UPLOAD_GRANULARITY).enumerate(); + let mut body = Rechunk::new(body, UPLOAD_GRANULARITY); // let body = ReaderStream::new(ZstdEncoder::new(StreamReader::new(body))); // Unfortunately, MinIO has the limitation that the length of a multipart request @@ -1004,23 +1004,23 @@ impl ObjectstoreServiceInner { // Every chunk needs to be > 5 MB because that's what multipart requires. let mut parts = vec![]; let mut compressed_buffer = zstd::stream::write::Encoder::new(vec![], 0).unwrap(); // FIXME; - let mut last_grain_index = 0; - while let Some((grain_index, bytes)) = body.next().await { + let mut part_number = offset / UPLOAD_GRANULARITY; // FIXME: validate offset + while let Some(bytes) = body.next().await { + part_number += 1; match bytes { Err(error) => { - try_flush(&mut compressed_buffer, grain_index, &mut multipart_upload, &mut parts).await?; + try_flush(&mut compressed_buffer, part_number, &mut multipart_upload, &mut parts).await?; return Err(objectstore_client::Error::Io(error).into()); } Ok(bytes) => { compressed_buffer.write_all(&bytes).map_err(AttemptUploadError::Zstd)?; if compressed_buffer.get_ref().len() > MULTIPART_MIN_PART_SIZE { - try_flush(&mut compressed_buffer, grain_index, &mut multipart_upload, &mut parts).await?; + try_flush(&mut compressed_buffer, part_number, &mut multipart_upload, &mut parts).await?; } } } - last_grain_index = grain_index; } - try_flush(&mut compressed_buffer, last_grain_index, &mut multipart_upload, &mut parts).await?; + try_flush(&mut compressed_buffer, part_number, &mut multipart_upload, &mut parts).await?; if length.is_some_and(|length| length == byte_counter.get()) { let final_key = multipart_upload.complete(parts).await?; @@ -1057,7 +1057,7 @@ impl ObjectstoreServiceInner { async fn try_flush( buffer: &mut zstd::stream::write::Encoder<'_, Vec>, - grain_index: usize, + part_number: usize, multipart: &mut MultipartUpload, parts: &mut Vec, ) -> Result<(), AttemptUploadError> { @@ -1065,7 +1065,7 @@ async fn try_flush( zstd::stream::write::Encoder::new(vec![], 0).map_err(AttemptUploadError::Zstd)?; std::mem::swap(buffer, &mut new_buffer); - let part_number = u32::try_from(grain_index + 1) + let part_number = u32::try_from(part_number) .map_err(|_| objectstore_client::Error::InvalidPartNumber(u32::MAX))?; let part = multipart diff --git a/tests/integration/test_upload.py b/tests/integration/test_upload.py index e62772086a3..7ea842a91a9 100644 --- a/tests/integration/test_upload.py +++ b/tests/integration/test_upload.py @@ -13,7 +13,6 @@ from sentry_relay.auth import SecretKey -from .asserts import matches_any from .consts import ( DUMMY_UPLOAD_PATH, DUMMY_UPLOAD_LOCATION, @@ -665,8 +664,9 @@ def test_objectstore_compression( project_id = 42 project_key = mini_sentry.get_dsn_public_key(project_id) relay = relay_with_processing() + objectstore = objectstore("attachments", project_id) - data = 1_000_000 * b"X" + data = 3 * 1024 * 1024 * b"X" response = relay.post( f"/api/{project_id}/upload/?sentry_key={project_key}", headers={ @@ -677,31 +677,61 @@ def test_objectstore_compression( ) assert response.status_code == 201, response.json() - # Upload only half the bytes - data = data[: len(data) // 2] + # Upload only part of the bytes + split = len(data) // 2 + data1 = data[:split] + data2 = data[split:] + response = relay.patch( f"{response.headers['Location']}&sentry_key={project_key}", headers={ - "Content-Length": str(len(data)), + "Content-Length": str(len(data1)), "Content-Type": "application/offset+octet-stream", "Tus-Resumable": "1.0.0", "Upload-Offset": "0", }, - data=data, + data=data1, ) assert response.status_code == 204 - objectstore = objectstore("attachments", project_id) + key, upload_id = LOCATION_REGEX.match(response.headers["Location"]).groups() + (part1,) = MultipartUpload(objectstore, key, upload_id).list_parts() + + print(part1) + # assert vars(part1) == { + # "part_number": 1, + # "etag": matches_any(), + # "last_modified": matches_any(), + # "size": len(zstandard.compress(data1)), + # } + + response = relay.patch( + f"{response.headers['Location']}&sentry_key={project_key}", + headers={ + "Content-Length": str(len(data2)), + "Content-Type": "application/offset+octet-stream", + "Tus-Resumable": "1.0.0", + "Upload-Offset": str(len(data1)), + "Upload-Length": str(len(data)), # TODO: can we even make this required? + }, + data=data2, + ) + assert response.status_code == 204 key, upload_id = LOCATION_REGEX.match(response.headers["Location"]).groups() - (part,) = MultipartUpload(objectstore, key, upload_id).list_parts() + part1_again, part2 = MultipartUpload(objectstore, key, upload_id).list_parts() - assert vars(part) == { - "part_number": 1, - "etag": matches_any(), - "last_modified": matches_any(), - "size": len(data), - } + assert part1_again == part1 + + print(part1_again, part2) + # assert vars(part2) == { + # "part_number": 3, + # "etag": matches_any(), + # "last_modified": matches_any(), + # "size": len(zstandard.compress(data2)), + # } + + assert objectstore.get(key) == data @pytest.mark.parametrize( From 68439a95462e90c5124f7bf7e57a7018407b717d Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Tue, 14 Jul 2026 22:53:31 +0200 Subject: [PATCH 10/41] validate offset --- relay-server/src/endpoints/upload.rs | 1 + relay-server/src/services/objectstore.rs | 17 ++++++++++++++++- 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/relay-server/src/endpoints/upload.rs b/relay-server/src/endpoints/upload.rs index 5fc95a7518c..83b396964fb 100644 --- a/relay-server/src/endpoints/upload.rs +++ b/relay-server/src/endpoints/upload.rs @@ -115,6 +115,7 @@ impl IntoResponse for Error { #[cfg(feature = "processing")] upload::Error::Objectstore(service_error) => match service_error.kind { objectstore::ErrorKind::InvalidScoping => StatusCode::INTERNAL_SERVER_ERROR, + objectstore::ErrorKind::InvalidOffset { .. } => StatusCode::BAD_REQUEST, objectstore::ErrorKind::Timeout(_) => StatusCode::GATEWAY_TIMEOUT, objectstore::ErrorKind::LoadShed => StatusCode::SERVICE_UNAVAILABLE, objectstore::ErrorKind::CompressionFailed(_) => { diff --git a/relay-server/src/services/objectstore.rs b/relay-server/src/services/objectstore.rs index 7908d38913d..6c48597f69a 100644 --- a/relay-server/src/services/objectstore.rs +++ b/relay-server/src/services/objectstore.rs @@ -283,6 +283,10 @@ impl> From for Error { pub enum ErrorKind { #[error("invalid scoping")] InvalidScoping, + #[error( + "invalid upload offset {offset}: must be aligned to the upload granularity {granularity}" + )] + InvalidOffset { offset: usize, granularity: usize }, #[error("timeout: {0}")] Timeout(#[from] tokio::time::error::Elapsed), #[error("load shed")] @@ -299,6 +303,7 @@ impl ErrorKind { fn as_str(&self) -> &'static str { match self { Self::InvalidScoping => "invalid_scoping", + Self::InvalidOffset { .. } => "invalid_offset", Self::Timeout(_) => "timeout", Self::LoadShed => "load_shed", Self::CompressionFailed(_) => "compression_failed", @@ -309,6 +314,7 @@ impl ErrorKind { fn is_client_error(&self) -> bool { match self { + ErrorKind::InvalidOffset { .. } => true, ErrorKind::UploadFailed(objectstore_client::Error::Reqwest(error)) => { find_error_source(error, is_user_error).is_some() } @@ -827,6 +833,15 @@ impl ObjectstoreServiceInner { upload_ref, stream, } = stream; + + if upload_ref.offset % UPLOAD_GRANULARITY != 0 { + return Err(ErrorKind::InvalidOffset { + offset: upload_ref.offset, + granularity: UPLOAD_GRANULARITY.get(), + } + .into()); + } + let session = self.session(&self.event_attachments, organization_id, project_id)?; let byte_counter = stream.byte_counter(); @@ -1004,7 +1019,7 @@ impl ObjectstoreServiceInner { // Every chunk needs to be > 5 MB because that's what multipart requires. let mut parts = vec![]; let mut compressed_buffer = zstd::stream::write::Encoder::new(vec![], 0).unwrap(); // FIXME; - let mut part_number = offset / UPLOAD_GRANULARITY; // FIXME: validate offset + let mut part_number = offset / UPLOAD_GRANULARITY; while let Some(bytes) = body.next().await { part_number += 1; match bytes { From 4c079797ca0aff7ca26f694a90cf51e54507b782 Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Tue, 14 Jul 2026 23:20:57 +0200 Subject: [PATCH 11/41] fix --- relay-server/src/services/objectstore.rs | 28 +++++++++++------- tests/integration/test_upload.py | 36 +++++++++--------------- 2 files changed, 31 insertions(+), 33 deletions(-) diff --git a/relay-server/src/services/objectstore.rs b/relay-server/src/services/objectstore.rs index 6c48597f69a..f95b03ea17e 100644 --- a/relay-server/src/services/objectstore.rs +++ b/relay-server/src/services/objectstore.rs @@ -10,8 +10,8 @@ use bytes::Bytes; use futures::StreamExt; use http::StatusCode; use objectstore_client::{ - Client, CompletePart, Compression, ExpirationPolicy, MultipartUpload, SecretKey as SigningKey, - Session, TokenGenerator, UploadId, Usecase, + Client, CompletePart, Compression, ExpirationPolicy, MultipartUpload, PartInfo, + SecretKey as SigningKey, Session, TokenGenerator, UploadId, Usecase, }; use objectstore_types::multipart::InvalidUploadId; @@ -1037,18 +1037,26 @@ impl ObjectstoreServiceInner { try_flush(&mut compressed_buffer, part_number, &mut multipart_upload, &mut parts).await?; - if length.is_some_and(|length| length == byte_counter.get()) { + let new_offset = offset + byte_counter.get(); + if length.is_some_and(|length| length == new_offset) { + relay_log::trace!("Completing multipart upload"); + let parts = multipart_upload.list_parts().await?; + + let parts = parts.into_iter().map(|item| { + let PartInfo{ part_number, etag, .. } = item; + CompletePart { part_number, etag } + }); + let final_key = multipart_upload.complete(parts).await?; debug_assert_eq!(&original_key, &final_key); } - }); - - Ok(UploadRef { - key: original_key, - upload_id: Some(upload_id), - offset: byte_counter.get(), - length, + Ok(UploadRef { + key: original_key, + upload_id: Some(upload_id), + offset: offset + byte_counter.get(), + length, + }) }) } } diff --git a/tests/integration/test_upload.py b/tests/integration/test_upload.py index 7ea842a91a9..622af57bfc2 100644 --- a/tests/integration/test_upload.py +++ b/tests/integration/test_upload.py @@ -10,9 +10,11 @@ from flask import Response from objectstore_client.multipart import MultipartUpload import pytest +import zstandard from sentry_relay.auth import SecretKey +from .asserts import matches_any from .consts import ( DUMMY_UPLOAD_PATH, DUMMY_UPLOAD_LOCATION, @@ -658,9 +660,7 @@ def multipart_upload(**opts): assert response.status_code == 500 # not 504 -def test_objectstore_compression( - mini_sentry, relay_with_processing, project_config, objectstore -): +def test_upload_offset(mini_sentry, relay_with_processing, project_config, objectstore): project_id = 42 project_key = mini_sentry.get_dsn_public_key(project_id) relay = relay_with_processing() @@ -678,7 +678,7 @@ def test_objectstore_compression( assert response.status_code == 201, response.json() # Upload only part of the bytes - split = len(data) // 2 + split = len(data) // 3 data1 = data[:split] data2 = data[split:] @@ -697,13 +697,12 @@ def test_objectstore_compression( key, upload_id = LOCATION_REGEX.match(response.headers["Location"]).groups() (part1,) = MultipartUpload(objectstore, key, upload_id).list_parts() - print(part1) - # assert vars(part1) == { - # "part_number": 1, - # "etag": matches_any(), - # "last_modified": matches_any(), - # "size": len(zstandard.compress(data1)), - # } + assert vars(part1) == { + "part_number": 1, + "etag": matches_any(), + "last_modified": matches_any(), + "size": len(zstandard.compress(data1)), + } response = relay.patch( f"{response.headers['Location']}&sentry_key={project_key}", @@ -712,26 +711,17 @@ def test_objectstore_compression( "Content-Type": "application/offset+octet-stream", "Tus-Resumable": "1.0.0", "Upload-Offset": str(len(data1)), - "Upload-Length": str(len(data)), # TODO: can we even make this required? + # "Upload-Length": str(len(data)), # TODO: can we even make this required? }, data=data2, ) assert response.status_code == 204 key, upload_id = LOCATION_REGEX.match(response.headers["Location"]).groups() - part1_again, part2 = MultipartUpload(objectstore, key, upload_id).list_parts() - - assert part1_again == part1 - print(part1_again, part2) - # assert vars(part2) == { - # "part_number": 3, - # "etag": matches_any(), - # "last_modified": matches_any(), - # "size": len(zstandard.compress(data2)), - # } + # TODO: remove upload_id - assert objectstore.get(key) == data + assert objectstore.get(key).payload.read() == data @pytest.mark.parametrize( From a713d024a59934d1b95a43995691b1b4ac396872 Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Thu, 16 Jul 2026 14:36:30 +0200 Subject: [PATCH 12/41] fix --- relay-server/src/services/objectstore.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/relay-server/src/services/objectstore.rs b/relay-server/src/services/objectstore.rs index 52e2473f955..c6a0c6a1f85 100644 --- a/relay-server/src/services/objectstore.rs +++ b/relay-server/src/services/objectstore.rs @@ -1018,7 +1018,7 @@ impl ObjectstoreServiceInner { // // Every chunk needs to be > 5 MB because that's what multipart requires. let mut parts = vec![]; - let mut compressed_buffer = zstd::stream::write::Encoder::new(vec![], 0).unwrap(); // FIXME; + let mut compressed_buffer = zstd::stream::write::Encoder::new(vec![], 0).map_err(AttemptUploadError::Zstd)?; let mut part_number = offset / UPLOAD_GRANULARITY; while let Some(bytes) = body.next().await { part_number += 1; From c83bc3b0635c35d5254f04dd5789c349ce647c4c Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Thu, 16 Jul 2026 16:58:22 +0200 Subject: [PATCH 13/41] feat(upload): add finish message --- relay-server/src/endpoints/upload.rs | 2 + relay-server/src/services/objectstore.rs | 116 +++++++++++++++- relay-server/src/services/upload.rs | 164 ++++++++++++++++++++++- relay-server/src/statsd.rs | 8 +- 4 files changed, 286 insertions(+), 4 deletions(-) diff --git a/relay-server/src/endpoints/upload.rs b/relay-server/src/endpoints/upload.rs index 53349fd7daa..5bca991e080 100644 --- a/relay-server/src/endpoints/upload.rs +++ b/relay-server/src/endpoints/upload.rs @@ -116,6 +116,8 @@ impl IntoResponse for Error { upload::Error::Objectstore(service_error) => match service_error.kind { objectstore::ErrorKind::InvalidScoping => StatusCode::INTERNAL_SERVER_ERROR, objectstore::ErrorKind::InvalidOffset { .. } => StatusCode::BAD_REQUEST, + objectstore::ErrorKind::InvalidUploadRef + | objectstore::ErrorKind::InvalidLength { .. } => StatusCode::BAD_REQUEST, objectstore::ErrorKind::Timeout(_) => StatusCode::GATEWAY_TIMEOUT, objectstore::ErrorKind::LoadShed => StatusCode::SERVICE_UNAVAILABLE, objectstore::ErrorKind::CompressionFailed(_) => { diff --git a/relay-server/src/services/objectstore.rs b/relay-server/src/services/objectstore.rs index c6a0c6a1f85..c83241f5c29 100644 --- a/relay-server/src/services/objectstore.rs +++ b/relay-server/src/services/objectstore.rs @@ -59,6 +59,7 @@ pub enum Objectstore { RawProfile(Managed), Create(CreateMultipart, Sender>), Stream(Stream, Sender>), + Finish(Finish, Sender>), } impl Objectstore { @@ -70,6 +71,7 @@ impl Objectstore { Self::RawProfile(_) => MessageKind::RawProfile, Self::Stream { .. } => MessageKind::Stream, Self::Create { .. } => MessageKind::Create, + Self::Finish { .. } => MessageKind::Finish, } } @@ -81,6 +83,7 @@ impl Objectstore { Self::RawProfile(_) => 1, Self::Stream { .. } => 1, Self::Create { .. } => 0, + Self::Finish { .. } => 1, } } } @@ -128,6 +131,7 @@ enum MessageKind { RawProfile, Stream, Create, + Finish, } impl MessageKind { @@ -139,6 +143,7 @@ impl MessageKind { Self::RawProfile => "profile_raw", Self::Stream => "stream", Self::Create => "create", + Self::Finish => "finish", } } } @@ -177,6 +182,21 @@ impl FromMessage for Objectstore { } } +/// A request to finish an objectstore multipart upload. +pub struct Finish { + pub organization_id: OrganizationId, + pub project_id: ProjectId, + pub upload_ref: UploadRef, +} + +impl FromMessage for Objectstore { + type Response = AsyncResponse>; + + fn from_message(message: Finish, sender: Sender>) -> Self { + Self::Finish(message, sender) + } +} + /// An attachment that is ready for upload / EAP storage. pub struct StoreTraceAttachment { /// The body to be uploaded to objectstore. @@ -287,6 +307,10 @@ pub enum ErrorKind { "invalid upload offset {offset}: must be aligned to the upload granularity {granularity}" )] InvalidOffset { offset: usize, granularity: usize }, + #[error("cannot finish an upload without a multipart upload ID or final length")] + InvalidUploadRef, + #[error("invalid upload length {expected}: uploaded parts have a total length of {actual}")] + InvalidLength { expected: usize, actual: u64 }, #[error("timeout: {0}")] Timeout(#[from] tokio::time::error::Elapsed), #[error("load shed")] @@ -304,6 +328,8 @@ impl ErrorKind { match self { Self::InvalidScoping => "invalid_scoping", Self::InvalidOffset { .. } => "invalid_offset", + Self::InvalidUploadRef => "invalid_upload_ref", + Self::InvalidLength { .. } => "invalid_length", Self::Timeout(_) => "timeout", Self::LoadShed => "load_shed", Self::CompressionFailed(_) => "compression_failed", @@ -314,7 +340,9 @@ impl ErrorKind { fn is_client_error(&self) -> bool { match self { - ErrorKind::InvalidOffset { .. } => true, + ErrorKind::InvalidOffset { .. } + | ErrorKind::InvalidUploadRef + | ErrorKind::InvalidLength { .. } => true, ErrorKind::UploadFailed(objectstore_client::Error::Reqwest(error)) => { find_error_source(error, is_user_error).is_some() } @@ -507,6 +535,9 @@ impl LoadShed for ObjectstoreService { Objectstore::Create(_, sender) => { sender.send(Err(error)); } + Objectstore::Finish(_, sender) => { + sender.send(Err(error)); + } } } } @@ -560,6 +591,13 @@ impl ObjectstoreServiceInner { } sender.send(result); } + Objectstore::Finish(finish, sender) => { + let result = self.handle_finish(finish).await; + if let Err(error) = &result { + error.log(MessageKind::Finish); + } + sender.send(result); + } }; } @@ -858,6 +896,40 @@ impl ObjectstoreServiceInner { .await } + async fn handle_finish(&self, finish: Finish) -> Result { + let Finish { + organization_id, + project_id, + upload_ref, + } = finish; + let UploadRef { + key, + upload_id, + offset: _, + length, + } = upload_ref; + let (Some(upload_id), Some(length)) = (upload_id, length) else { + return Err(ErrorKind::InvalidUploadRef.into()); + }; + + let session = self.session(&self.event_attachments, organization_id, project_id)?; + let multipart = session.resume_multipart_upload(key.clone(), upload_id.to_string())?; + let parts = multipart.list_parts().await?; + validate_multipart_length(&parts, length)?; + + let final_key = multipart + .complete(parts.into_iter().map(Into::into)) + .await?; + debug_assert_eq!(&key, &final_key); + + Ok(UploadRef { + key: final_key, + upload_id: None, + offset: length, + length: Some(length), + }) + } + async fn upload_bytes( &self, kind: MessageKind, @@ -1119,6 +1191,18 @@ impl ObjectstoreServiceInner { } } +fn validate_multipart_length(parts: &[PartInfo], expected: usize) -> Result<(), ErrorKind> { + let actual = parts + .iter() + .fold(0u64, |sum, part| sum.saturating_add(part.size)); + + if actual != expected as u64 { + return Err(ErrorKind::InvalidLength { expected, actual }); + } + + Ok(()) +} + #[derive(Debug, thiserror::Error)] enum AttemptUploadError { #[error(transparent)] @@ -1248,6 +1332,9 @@ fn drop_attachments(event: &mut Managed>) { #[cfg(test)] mod tests { + use std::num::NonZeroU32; + use std::time::SystemTime; + use relay_event_schema::protocol::EventId; use relay_quotas::DataCategory; use relay_system::Service; @@ -1256,6 +1343,33 @@ mod tests { use super::*; + #[test] + fn validates_multipart_length() { + let parts = [ + PartInfo { + part_number: NonZeroU32::new(1).unwrap(), + etag: "first".to_owned(), + last_modified: SystemTime::UNIX_EPOCH, + size: 3, + }, + PartInfo { + part_number: NonZeroU32::new(2).unwrap(), + etag: "second".to_owned(), + last_modified: SystemTime::UNIX_EPOCH, + size: 5, + }, + ]; + + assert!(validate_multipart_length(&parts, 8).is_ok()); + assert!(matches!( + validate_multipart_length(&parts, 9), + Err(ErrorKind::InvalidLength { + expected: 9, + actual: 8 + }) + )); + } + #[tokio::test] async fn org_zero_rejected() { let service = ObjectstoreService::new(&test_config(), Some(Addr::dummy())) diff --git a/relay-server/src/services/upload.rs b/relay-server/src/services/upload.rs index fedb5ca6585..b7a950e40d6 100644 --- a/relay-server/src/services/upload.rs +++ b/relay-server/src/services/upload.rs @@ -113,6 +113,8 @@ pub enum Upload { /// The service also returns the signed location. This is redundant, but creates a simpler /// flow for the caller side. Upload(Box, InstrumentedSender), + /// Finishes an upload and declares its final length. + Finish(Finish, InstrumentedSender), } impl Interface for Upload {} @@ -157,6 +159,18 @@ pub struct Stream { pub stream: BoundedStream>, } +/// A request to finish an upload and declare its final length. +pub struct Finish { + /// Time of arrival of the request. + pub received: DateTime, + /// The project the upload belongs to. + pub project: ProjectContext, + /// The location to finish. + pub location: SignedLocation, + /// The final length of the upload in bytes. + pub length: usize, +} + impl FromMessage for Upload { type Response = AsyncResponse, Error>>; @@ -191,6 +205,20 @@ impl FromMessage for Upload { } } +impl FromMessage for Upload { + type Response = AsyncResponse, Error>>; + + fn from_message(message: Finish, sender: Sender, Error>>) -> Self { + Self::Finish( + message, + InstrumentedSender { + metric: RelayCounters::UploadFinish, + inner: sender, + }, + ) + } +} + /// Creates a new upload service. pub fn create_service( config: &Arc, @@ -399,6 +427,68 @@ impl Service { } } + async fn finish(&self, finish: Finish) -> Result, Error> { + let Finish { + #[cfg_attr(not(feature = "processing"), expect(unused))] + received, + project, + location, + length, + } = finish; + + match &self.backend { + Backend::Upstream { addr } => { + let (request, rx) = UploadRequest::finish(project, location.try_to_uri()?, length); + addr.send(SendRequest(request)); + let response = rx.await??; + SignedLocation::try_from_response(response) + } + #[cfg(feature = "processing")] + Backend::Objectstore { addr, config } => { + use crate::services::objectstore::UploadRef; + + let Location { + project_id, + key, + length: _, + upload_id, + other, + } = location.verify(received, config)?; + + let scoping = project.scoping; + debug_assert_eq!(scoping.project_id, project_id); + let upload_ref = UploadRef::new(key, upload_id, length, Some(length))?; + let upload_ref = addr + .send(objectstore::Finish { + organization_id: scoping.organization_id, + project_id, + upload_ref, + }) + .await + .map_err(Error::ObjectstoreServiceUnavailable)??; + + let UploadRef { + key, + upload_id, + offset, + length: final_length, + } = upload_ref; + debug_assert!(upload_id.is_none()); + debug_assert_eq!(offset, length); + debug_assert_eq!(final_length, Some(length)); + + Location { + project_id, + key, + length: Final(length), + upload_id: None, + other, + } + .try_sign(config) + } + } + } + async fn timeout(&self, future: F) -> Result, Error> where L: UploadLength, @@ -419,6 +509,9 @@ impl SimpleService for Service { Upload::Upload(stream, sender) => { sender.send(self.timeout(self.upload(*stream)).await); } + Upload::Finish(finish, sender) => { + sender.send(self.timeout(self.finish(finish)).await); + } } } } @@ -428,6 +521,7 @@ impl LoadShed for Service { match message { Upload::Create(_, tx) => tx.send(Err(Error::LoadShed)), Upload::Upload(_, tx) => tx.send(Err(Error::LoadShed)), + Upload::Finish(_, tx) => tx.send(Err(Error::LoadShed)), } } } @@ -732,6 +826,10 @@ enum RequestKind { stream: TakeOnce>>, encoding: HttpEncoding, }, + Finish { + uri: String, + length: usize, + }, } /// An upstream request made to the `/upload` endpoint. @@ -789,6 +887,25 @@ impl UploadRequest { rx, ) } + + fn finish( + project: ProjectContext, + uri: String, + length: usize, + ) -> ( + Self, + oneshot::Receiver>, + ) { + let (sender, rx) = oneshot::channel(); + ( + Self { + project, + kind: RequestKind::Finish { uri, length }, + sender, + }, + rx, + ) + } } impl fmt::Debug for UploadRequest { @@ -812,7 +929,7 @@ impl UpstreamRequest for UploadRequest { fn method(&self) -> Method { match self.kind { RequestKind::Create { .. } => Method::POST, - RequestKind::Upload { .. } => Method::PATCH, + RequestKind::Upload { .. } | RequestKind::Finish { .. } => Method::PATCH, } } @@ -820,7 +937,7 @@ impl UpstreamRequest for UploadRequest { let project_id = self.project.scoping.project_id; match &self.kind { RequestKind::Create { .. } => Cow::Owned(format!("/api/{project_id}/upload/")), - RequestKind::Upload { uri, .. } => Cow::Borrowed(uri), + RequestKind::Upload { uri, .. } | RequestKind::Finish { uri, .. } => Cow::Borrowed(uri), } } @@ -840,6 +957,7 @@ impl UpstreamRequest for UploadRequest { fn retry(&self) -> bool { match &self.kind { RequestKind::Create { .. } => true, + RequestKind::Finish { .. } => true, // Once the body has been polled, it cannot be replayed — give up instead. RequestKind::Upload { stream, .. } => !stream.is_taken(), } @@ -878,6 +996,12 @@ impl UpstreamRequest for UploadRequest { builder.body(reqwest::Body::wrap_stream(body)); } + RequestKind::Finish { uri: _, length } => { + tus::add_upload_headers(builder, *length); + builder.header(tus::UPLOAD_LENGTH, HeaderValue::from(*length)); + builder.header(http::header::CONTENT_LENGTH.as_str(), "0"); + builder.body(Bytes::new()); + } }; let project_key = self.project.scoping.project_key; @@ -912,6 +1036,42 @@ where mod tests { use super::*; + #[test] + fn finish_request_is_empty_and_declares_length() { + let project = ProjectContext { + scoping: Scoping { + organization_id: relay_base_schema::organization::OrganizationId::new(1), + project_id: ProjectId::new(42), + project_key: "a94ae32be2584e0bbd7a4cbb95971fee".parse().unwrap(), + key_id: None, + }, + upstream: None, + }; + let (mut upload, _receiver) = UploadRequest::finish( + project, + "/api/42/upload/key/?upload_signature=signature".to_owned(), + 123, + ); + let client = reqwest::Client::new(); + let mut builder = RequestBuilder::reqwest(client.request( + upload.method(), + format!("http://localhost{}", upload.path()), + )); + + upload.build(&mut builder).unwrap(); + let request = builder.finish().unwrap().0; + + assert_eq!(request.method(), Method::PATCH); + assert_eq!(request.headers()[tus::TUS_RESUMABLE], tus::TUS_VERSION); + assert_eq!(request.headers()[tus::UPLOAD_OFFSET], "123"); + assert_eq!(request.headers()[tus::UPLOAD_LENGTH], "123"); + assert_eq!(request.headers()[http::header::CONTENT_LENGTH], "0"); + assert_eq!( + request.body().and_then(reqwest::Body::as_bytes), + Some(&[][..]) + ); + } + #[cfg(feature = "processing")] mod with_processing { use chrono::Utc; diff --git a/relay-server/src/statsd.rs b/relay-server/src/statsd.rs index 5e5ad83e6d2..c8402d24074 100644 --- a/relay-server/src/statsd.rs +++ b/relay-server/src/statsd.rs @@ -981,11 +981,16 @@ pub enum RelayCounters { /// This metric is tagged with: /// - `result`: `success` or the failure reason. UploadCreate, - /// The number of times an upload location is created through the upload service. + /// The number of times bytes are streamed through the upload service. /// /// This metric is tagged with: /// - `result`: `success` or the failure reason. UploadUpload, + /// The number of times an upload is finished through the upload service. + /// + /// This metric is tagged with: + /// - `result`: `success` or the failure reason. + UploadFinish, /// The number of times an objectstore upload of an attachment occurs. /// /// This metric is tagged with: @@ -1076,6 +1081,7 @@ impl CounterMetric for RelayCounters { RelayCounters::UploadKillswitched => "upload.killswitched", RelayCounters::UploadCreate => "upload.create", RelayCounters::UploadUpload => "upload.upload", + RelayCounters::UploadFinish => "upload.finish", #[cfg(feature = "processing")] RelayCounters::AttachmentUpload => "attachment.upload", RelayCounters::EnvelopeWithLogs => "logs.envelope", From b75405f429b059dcca19c5e935835f4160862dab Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Fri, 17 Jul 2026 10:01:05 +0200 Subject: [PATCH 14/41] fix(upload): finalize multipart streams --- relay-server/src/endpoints/common.rs | 32 ++++++++++++++++++++-- relay-server/src/endpoints/upload.rs | 3 +- relay-server/src/services/objectstore.rs | 35 +++++++----------------- relay-server/src/services/upload.rs | 32 +++++++++++----------- 4 files changed, 56 insertions(+), 46 deletions(-) diff --git a/relay-server/src/endpoints/common.rs b/relay-server/src/endpoints/common.rs index 68cb3b810b4..e0db72385dd 100644 --- a/relay-server/src/endpoints/common.rs +++ b/relay-server/src/endpoints/common.rs @@ -24,7 +24,7 @@ use crate::service::ServiceState; use crate::services::buffer::{ProjectKeyPair, PushError}; use crate::services::outcome::{DiscardItemType, DiscardReason, Outcome}; use crate::services::processor::{BucketSource, MetricData, ProcessMetrics}; -use crate::services::upload::{Create, ProjectContext, Stream, Upload}; +use crate::services::upload::{Create, Finish, ProjectContext, Stream, Upload}; use crate::statsd::{RelayCounters, RelayDistributions}; use crate::utils::{ self, ApiErrorResponse, BoundedStream, FormDataIter, MeteredStream, find_error_source, @@ -611,7 +611,7 @@ where let result = upload .send(Stream { received: Utc::now(), - project, + project: project.clone(), location, offset: 0, stream, @@ -631,7 +631,33 @@ where ); }) .ok()?; - let location = location.into_header_value().ok()?; + + let location = if location.upload_id().is_some() { + upload + .send(Finish { + received: Utc::now(), + project, + location, + length: byte_counter.get(), + }) + .await + .ok()? + .inspect_err(|e| { + relay_log::warn!( + error = e as &dyn std::error::Error, + referrer = referrer, + organization_id = scoping.organization_id.value(), + project_id = scoping.project_id.value(), + bytes_uploaded = byte_counter.get(), + "multipart item upload finalization failed", + ); + }) + .ok()? + .into_header_value() + .ok()? + } else { + location.into_header_value().ok()? + }; let location = location.to_str().ok()?; let placeholder = serde_json::to_vec(&AttachmentPlaceholder { location, diff --git a/relay-server/src/endpoints/upload.rs b/relay-server/src/endpoints/upload.rs index 5bca991e080..069d3654ecc 100644 --- a/relay-server/src/endpoints/upload.rs +++ b/relay-server/src/endpoints/upload.rs @@ -116,8 +116,7 @@ impl IntoResponse for Error { upload::Error::Objectstore(service_error) => match service_error.kind { objectstore::ErrorKind::InvalidScoping => StatusCode::INTERNAL_SERVER_ERROR, objectstore::ErrorKind::InvalidOffset { .. } => StatusCode::BAD_REQUEST, - objectstore::ErrorKind::InvalidUploadRef - | objectstore::ErrorKind::InvalidLength { .. } => StatusCode::BAD_REQUEST, + objectstore::ErrorKind::InvalidLength { .. } => StatusCode::BAD_REQUEST, objectstore::ErrorKind::Timeout(_) => StatusCode::GATEWAY_TIMEOUT, objectstore::ErrorKind::LoadShed => StatusCode::SERVICE_UNAVAILABLE, objectstore::ErrorKind::CompressionFailed(_) => { diff --git a/relay-server/src/services/objectstore.rs b/relay-server/src/services/objectstore.rs index c83241f5c29..882fcd584f0 100644 --- a/relay-server/src/services/objectstore.rs +++ b/relay-server/src/services/objectstore.rs @@ -59,7 +59,7 @@ pub enum Objectstore { RawProfile(Managed), Create(CreateMultipart, Sender>), Stream(Stream, Sender>), - Finish(Finish, Sender>), + Finish(Finish, Sender>), } impl Objectstore { @@ -186,13 +186,15 @@ impl FromMessage for Objectstore { pub struct Finish { pub organization_id: OrganizationId, pub project_id: ProjectId, - pub upload_ref: UploadRef, + pub key: String, + pub upload_id: UploadId, + pub length: usize, } impl FromMessage for Objectstore { - type Response = AsyncResponse>; + type Response = AsyncResponse>; - fn from_message(message: Finish, sender: Sender>) -> Self { + fn from_message(message: Finish, sender: Sender>) -> Self { Self::Finish(message, sender) } } @@ -307,8 +309,6 @@ pub enum ErrorKind { "invalid upload offset {offset}: must be aligned to the upload granularity {granularity}" )] InvalidOffset { offset: usize, granularity: usize }, - #[error("cannot finish an upload without a multipart upload ID or final length")] - InvalidUploadRef, #[error("invalid upload length {expected}: uploaded parts have a total length of {actual}")] InvalidLength { expected: usize, actual: u64 }, #[error("timeout: {0}")] @@ -328,7 +328,6 @@ impl ErrorKind { match self { Self::InvalidScoping => "invalid_scoping", Self::InvalidOffset { .. } => "invalid_offset", - Self::InvalidUploadRef => "invalid_upload_ref", Self::InvalidLength { .. } => "invalid_length", Self::Timeout(_) => "timeout", Self::LoadShed => "load_shed", @@ -340,9 +339,7 @@ impl ErrorKind { fn is_client_error(&self) -> bool { match self { - ErrorKind::InvalidOffset { .. } - | ErrorKind::InvalidUploadRef - | ErrorKind::InvalidLength { .. } => true, + ErrorKind::InvalidOffset { .. } | ErrorKind::InvalidLength { .. } => true, ErrorKind::UploadFailed(objectstore_client::Error::Reqwest(error)) => { find_error_source(error, is_user_error).is_some() } @@ -896,21 +893,14 @@ impl ObjectstoreServiceInner { .await } - async fn handle_finish(&self, finish: Finish) -> Result { + async fn handle_finish(&self, finish: Finish) -> Result { let Finish { organization_id, project_id, - upload_ref, - } = finish; - let UploadRef { key, upload_id, - offset: _, length, - } = upload_ref; - let (Some(upload_id), Some(length)) = (upload_id, length) else { - return Err(ErrorKind::InvalidUploadRef.into()); - }; + } = finish; let session = self.session(&self.event_attachments, organization_id, project_id)?; let multipart = session.resume_multipart_upload(key.clone(), upload_id.to_string())?; @@ -922,12 +912,7 @@ impl ObjectstoreServiceInner { .await?; debug_assert_eq!(&key, &final_key); - Ok(UploadRef { - key: final_key, - upload_id: None, - offset: length, - length: Some(length), - }) + Ok(ObjectstoreKey(final_key)) } async fn upload_bytes( diff --git a/relay-server/src/services/upload.rs b/relay-server/src/services/upload.rs index b7a950e40d6..ed43d6af270 100644 --- a/relay-server/src/services/upload.rs +++ b/relay-server/src/services/upload.rs @@ -13,6 +13,8 @@ use chrono::Utc; use futures::StreamExt; use futures::stream::BoxStream; use http::{HeaderValue, Method}; +#[cfg(feature = "processing")] +use objectstore_types::multipart::UploadId; use relay_auth::Signature; use relay_auth::SignatureError; #[cfg(feature = "processing")] @@ -445,8 +447,6 @@ impl Service { } #[cfg(feature = "processing")] Backend::Objectstore { addr, config } => { - use crate::services::objectstore::UploadRef; - let Location { project_id, key, @@ -457,25 +457,20 @@ impl Service { let scoping = project.scoping; debug_assert_eq!(scoping.project_id, project_id); - let upload_ref = UploadRef::new(key, upload_id, length, Some(length))?; - let upload_ref = addr + let upload_id = UploadId::new( + upload_id.expect("Finish is only sent for multipart upload locations"), + )?; + let key = addr .send(objectstore::Finish { organization_id: scoping.organization_id, project_id, - upload_ref, + key, + upload_id, + length, }) .await - .map_err(Error::ObjectstoreServiceUnavailable)??; - - let UploadRef { - key, - upload_id, - offset, - length: final_length, - } = upload_ref; - debug_assert!(upload_id.is_none()); - debug_assert_eq!(offset, length); - debug_assert_eq!(final_length, Some(length)); + .map_err(Error::ObjectstoreServiceUnavailable)?? + .into_inner(); Location { project_id, @@ -727,6 +722,11 @@ impl SignedLocation { HeaderValue::from_str(&self.try_to_uri()?).map_err(Error::Internal) } + /// Returns the multipart upload ID, if this location refers to a multipart upload. + pub fn upload_id(&self) -> Option<&str> { + self.location.upload_id.as_deref() + } + fn try_to_uri(&self) -> Result { let Self { location, From 3b95811f6ac6655736cf17d666956dd27e208e0d Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Fri, 17 Jul 2026 10:47:14 +0200 Subject: [PATCH 15/41] clarify --- relay-server/src/endpoints/common.rs | 9 ++---- relay-server/src/services/upload.rs | 43 ++++++++++++++++------------ 2 files changed, 27 insertions(+), 25 deletions(-) diff --git a/relay-server/src/endpoints/common.rs b/relay-server/src/endpoints/common.rs index e0db72385dd..cc79067816f 100644 --- a/relay-server/src/endpoints/common.rs +++ b/relay-server/src/endpoints/common.rs @@ -632,13 +632,13 @@ where }) .ok()?; - let location = if location.upload_id().is_some() { + let location = { upload .send(Finish { received: Utc::now(), project, location, - length: byte_counter.get(), + trusted_length: byte_counter.get(), }) .await .ok()? @@ -653,11 +653,8 @@ where ); }) .ok()? - .into_header_value() - .ok()? - } else { - location.into_header_value().ok()? }; + let location = location.into_header_value().ok()?; let location = location.to_str().ok()?; let placeholder = serde_json::to_vec(&AttachmentPlaceholder { location, diff --git a/relay-server/src/services/upload.rs b/relay-server/src/services/upload.rs index ed43d6af270..617d359c650 100644 --- a/relay-server/src/services/upload.rs +++ b/relay-server/src/services/upload.rs @@ -170,7 +170,9 @@ pub struct Finish { /// The location to finish. pub location: SignedLocation, /// The final length of the upload in bytes. - pub length: usize, + /// + /// This value must never come from user input. + pub trusted_length: usize, } impl FromMessage for Upload { @@ -435,12 +437,13 @@ impl Service { received, project, location, - length, + trusted_length, } = finish; match &self.backend { Backend::Upstream { addr } => { - let (request, rx) = UploadRequest::finish(project, location.try_to_uri()?, length); + let (request, rx) = + UploadRequest::finish(project, location.try_to_uri()?, trusted_length); addr.send(SendRequest(request)); let response = rx.await??; SignedLocation::try_from_response(response) @@ -449,7 +452,7 @@ impl Service { Backend::Objectstore { addr, config } => { let Location { project_id, - key, + mut key, length: _, upload_id, other, @@ -457,25 +460,27 @@ impl Service { let scoping = project.scoping; debug_assert_eq!(scoping.project_id, project_id); - let upload_id = UploadId::new( - upload_id.expect("Finish is only sent for multipart upload locations"), - )?; - let key = addr - .send(objectstore::Finish { - organization_id: scoping.organization_id, - project_id, - key, - upload_id, - length, - }) - .await - .map_err(Error::ObjectstoreServiceUnavailable)?? - .into_inner(); + if let Some(upload_id) = upload_id { + key = addr + .send(objectstore::Finish { + organization_id: scoping.organization_id, + project_id, + key, + upload_id: UploadId::new(upload_id)?, + length: trusted_length, + }) + .await + .map_err(Error::ObjectstoreServiceUnavailable)?? + .into_inner(); + } + + // NOTE: `Finish` is never triggered by a client but always by the server, so + // we can trust the `length` here. Location { project_id, key, - length: Final(length), + length: Final(trusted_length), upload_id: None, other, } From cd5942608f0513145ea57284dd337aa8fbb272ba Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Fri, 17 Jul 2026 10:49:48 +0200 Subject: [PATCH 16/41] test --- tests/integration/test_minidump.py | 32 ++++++++++++++++++------------ 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/tests/integration/test_minidump.py b/tests/integration/test_minidump.py index fde87229890..3ae6a33cfeb 100644 --- a/tests/integration/test_minidump.py +++ b/tests/integration/test_minidump.py @@ -492,20 +492,21 @@ def test_minidump_invalid_nested_formdata(mini_sentry, relay): @pytest.mark.parametrize( - "rate_limit,minidump_filename,use_objectstore,stream_upload", + "rate_limit,minidump_filename,use_objectstore,stream_upload,multipart", [ - (None, "minidump.dmp", True, False), - (None, "minidump.dmp", False, False), - ("attachment", "minidump.dmp", True, False), - ("attachment", "minidump.dmp", False, False), - ("transaction", "minidump.dmp", False, False), - (None, "minidump.dmp.gz", False, False), - (None, "minidump.dmp.xz", False, False), - (None, "minidump.dmp.bz2", False, False), - (None, "minidump.dmp.bz2", True, False), - (None, "minidump.dmp.zst", False, False), - (None, "minidump.dmp.zst", True, False), - (None, "minidump.dmp.zst", True, True), + (None, "minidump.dmp", True, False, False), + (None, "minidump.dmp", False, False, False), + ("attachment", "minidump.dmp", True, False, False), + ("attachment", "minidump.dmp", False, False, False), + ("transaction", "minidump.dmp", False, False, False), + (None, "minidump.dmp.gz", False, False, False), + (None, "minidump.dmp.xz", False, False, False), + (None, "minidump.dmp.bz2", False, False, False), + (None, "minidump.dmp.bz2", True, False, False), + (None, "minidump.dmp.zst", False, False, False), + (None, "minidump.dmp.zst", True, False, False), + (None, "minidump.dmp.zst", True, True, False), + (None, "minidump.dmp.zst", True, True, True), ], ) def test_minidump_with_processing( @@ -518,6 +519,7 @@ def test_minidump_with_processing( use_objectstore, objectstore, stream_upload, + multipart, ): dmp_path = os.path.join(os.path.dirname(__file__), "fixtures/native/minidump.dmp") with open(dmp_path, "rb") as f: @@ -542,6 +544,10 @@ def test_minidump_with_processing( project_config["config"].setdefault("features", []).append( "projects:relay-minidump-uploads" ) + if multipart: + project_config["config"].setdefault("features", []).append( + "projects:relay-upload-multipart" + ) options = { "processing": { From 19a5bd44e8073c7432aa05fd2c2bb6cd68789691 Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Fri, 17 Jul 2026 10:50:53 +0200 Subject: [PATCH 17/41] lint --- relay-server/src/services/upload.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/relay-server/src/services/upload.rs b/relay-server/src/services/upload.rs index 617d359c650..5c30d35e2e3 100644 --- a/relay-server/src/services/upload.rs +++ b/relay-server/src/services/upload.rs @@ -114,7 +114,7 @@ pub enum Upload { /// /// The service also returns the signed location. This is redundant, but creates a simpler /// flow for the caller side. - Upload(Box, InstrumentedSender), + Stream(Box, InstrumentedSender), /// Finishes an upload and declares its final length. Finish(Finish, InstrumentedSender), } @@ -199,7 +199,7 @@ impl FromMessage for Upload { message: Stream, sender: Sender, Error>>, ) -> Self { - Self::Upload( + Self::Stream( Box::new(message), InstrumentedSender { metric: RelayCounters::UploadUpload, @@ -506,7 +506,7 @@ impl SimpleService for Service { Upload::Create(create, sender) => { sender.send(self.timeout(self.create(create)).await); } - Upload::Upload(stream, sender) => { + Upload::Stream(stream, sender) => { sender.send(self.timeout(self.upload(*stream)).await); } Upload::Finish(finish, sender) => { @@ -520,7 +520,7 @@ impl LoadShed for Service { fn handle_loadshed(&self, message: Upload) { match message { Upload::Create(_, tx) => tx.send(Err(Error::LoadShed)), - Upload::Upload(_, tx) => tx.send(Err(Error::LoadShed)), + Upload::Stream(_, tx) => tx.send(Err(Error::LoadShed)), Upload::Finish(_, tx) => tx.send(Err(Error::LoadShed)), } } From 46f1abe2639536d7c3c645460e75904cf03ea3ca Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Fri, 17 Jul 2026 11:03:45 +0200 Subject: [PATCH 18/41] resolve todo --- relay-server/src/endpoints/upload.rs | 1 + relay-server/src/services/objectstore.rs | 9 +++++++-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/relay-server/src/endpoints/upload.rs b/relay-server/src/endpoints/upload.rs index 069d3654ecc..35e7f27b2b0 100644 --- a/relay-server/src/endpoints/upload.rs +++ b/relay-server/src/endpoints/upload.rs @@ -116,6 +116,7 @@ impl IntoResponse for Error { upload::Error::Objectstore(service_error) => match service_error.kind { objectstore::ErrorKind::InvalidScoping => StatusCode::INTERNAL_SERVER_ERROR, objectstore::ErrorKind::InvalidOffset { .. } => StatusCode::BAD_REQUEST, + objectstore::ErrorKind::OffsetWithoutUploadId { .. } => StatusCode::BAD_REQUEST, objectstore::ErrorKind::InvalidLength { .. } => StatusCode::BAD_REQUEST, objectstore::ErrorKind::Timeout(_) => StatusCode::GATEWAY_TIMEOUT, objectstore::ErrorKind::LoadShed => StatusCode::SERVICE_UNAVAILABLE, diff --git a/relay-server/src/services/objectstore.rs b/relay-server/src/services/objectstore.rs index 882fcd584f0..4fb40eaf294 100644 --- a/relay-server/src/services/objectstore.rs +++ b/relay-server/src/services/objectstore.rs @@ -309,6 +309,8 @@ pub enum ErrorKind { "invalid upload offset {offset}: must be aligned to the upload granularity {granularity}" )] InvalidOffset { offset: usize, granularity: usize }, + #[error("offset without upload ID")] + OffsetWithoutUploadId, #[error("invalid upload length {expected}: uploaded parts have a total length of {actual}")] InvalidLength { expected: usize, actual: u64 }, #[error("timeout: {0}")] @@ -328,6 +330,7 @@ impl ErrorKind { match self { Self::InvalidScoping => "invalid_scoping", Self::InvalidOffset { .. } => "invalid_offset", + Self::OffsetWithoutUploadId { .. } => "offset_without_upload_id", Self::InvalidLength { .. } => "invalid_length", Self::Timeout(_) => "timeout", Self::LoadShed => "load_shed", @@ -1043,8 +1046,7 @@ impl ObjectstoreServiceInner { let Some(upload_id) = upload_id else { // No upload ID: simple upload in a single request. if offset != 0 { - // Offset is only allowed for multipart. - todo!("raise error"); + return Err(AttemptUploadError::OffsetWithoutUploadId); } let request = session.put_stream(body.boxed()).key(key); let response = request.send().await?; @@ -1194,6 +1196,8 @@ enum AttemptUploadError { Objectstore(#[from] objectstore_client::Error), #[error("zstd error: {0}")] Zstd(#[source] std::io::Error), + #[error("offset without upload ID")] + OffsetWithoutUploadId, } impl From for ErrorKind { @@ -1201,6 +1205,7 @@ impl From for ErrorKind { match value { AttemptUploadError::Objectstore(error) => ErrorKind::UploadFailed(error), AttemptUploadError::Zstd(error) => ErrorKind::CompressionFailed(error), + AttemptUploadError::OffsetWithoutUploadId => ErrorKind::OffsetWithoutUploadId, } } } From 1adf88db6bc940d5b6a8be95ee776082caee8dc3 Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Fri, 17 Jul 2026 11:16:24 +0200 Subject: [PATCH 19/41] test --- tests/integration/test_upload.py | 66 ++++++++++++++++++++++---------- 1 file changed, 45 insertions(+), 21 deletions(-) diff --git a/tests/integration/test_upload.py b/tests/integration/test_upload.py index 4dd7e44f3e1..e59d5f24746 100644 --- a/tests/integration/test_upload.py +++ b/tests/integration/test_upload.py @@ -20,7 +20,10 @@ DUMMY_UPLOAD_LOCATION, ) -LOCATION_REGEX = re.compile(r"/api/\d+/upload/(\w+)/?.*upload_id=([\w-]+)") +LOCATION_REGEX = re.compile(r"/api/\d+/upload/(\w+)/") +LOCATION_REGEX_WITH_UPLOAD_ID = re.compile( + r"/api/\d+/upload/(\w+)/?.*upload_id=([\w-]+)" +) @pytest.fixture @@ -658,9 +661,17 @@ def multipart_upload(**opts): assert response.status_code == 504 -def test_upload_offset(mini_sentry, relay_with_processing, project_config, objectstore): +@pytest.mark.parametrize("feature_flag", [False, True]) +def test_upload_offset( + mini_sentry, relay_with_processing, project_config, objectstore, feature_flag +): project_id = 42 + config = mini_sentry.add_full_project_config(project_id)["config"] + features = config.setdefault("features", []) + if feature_flag: + features.append("projects:relay-upload-multipart") project_key = mini_sentry.get_dsn_public_key(project_id) + relay = relay_with_processing() objectstore = objectstore("attachments", project_id) @@ -673,7 +684,7 @@ def test_upload_offset(mini_sentry, relay_with_processing, project_config, objec "Upload-Length": str(len(data)), }, ) - assert response.status_code == 201, response.json() + assert response.status_code == 201 # Upload only part of the bytes split = len(data) // 3 @@ -690,18 +701,25 @@ def test_upload_offset(mini_sentry, relay_with_processing, project_config, objec }, data=data1, ) - assert response.status_code == 204 - - key, upload_id = LOCATION_REGEX.match(response.headers["Location"]).groups() - (part1,) = MultipartUpload(objectstore, key, upload_id).list_parts() - - assert vars(part1) == { - "part_number": 1, - "etag": matches_any(), - "last_modified": matches_any(), - "size": len(zstandard.compress(data1)), - } + assert response.status_code == 204, response.text + + if feature_flag: + key, upload_id = LOCATION_REGEX_WITH_UPLOAD_ID.match( + response.headers["Location"] + ).groups() + (part1,) = MultipartUpload(objectstore, key, upload_id).list_parts() + assert vars(part1) == { + "part_number": 1, + "etag": matches_any(), + "last_modified": matches_any(), + "size": len(zstandard.compress(data1)), + } + else: + (key,) = LOCATION_REGEX.match(response.headers["Location"]).groups() + # The file is already there + assert objectstore.get(key).payload.read() == data1 + # Try to upload more: response = relay.patch( f"{response.headers['Location']}&sentry_key={project_key}", headers={ @@ -713,13 +731,19 @@ def test_upload_offset(mini_sentry, relay_with_processing, project_config, objec }, data=data2, ) - assert response.status_code == 204 - - key, upload_id = LOCATION_REGEX.match(response.headers["Location"]).groups() - - # TODO: remove upload_id - - assert objectstore.get(key).payload.read() == data + if feature_flag: + assert response.status_code == 204 + (key,) = LOCATION_REGEX.match(response.headers["Location"]).groups() + assert objectstore.get(key).payload.read() == data + else: + assert response.status_code == 400 + assert response.json() == { + "causes": [ + "objectstore upload failed", + "offset without upload ID", + ], + "detail": "upload error: objectstore upload failed", + } @pytest.mark.parametrize( From dd421991fd0d213f4e4fbf3cf8a606786dae7f29 Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Fri, 17 Jul 2026 11:20:29 +0200 Subject: [PATCH 20/41] test --- tests/integration/test_upload.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/tests/integration/test_upload.py b/tests/integration/test_upload.py index e59d5f24746..b625bb53808 100644 --- a/tests/integration/test_upload.py +++ b/tests/integration/test_upload.py @@ -231,8 +231,8 @@ def test_upload_missing_upload_length(mini_sentry, relay, dummy_upload, project_ [ pytest.param( 10, - 400, - "stream shorter than lower bound: received 10 < 11", + 204, + None, id="smaller_than_announced", ), pytest.param( @@ -280,9 +280,10 @@ def test_upload_body_size( ) assert response.status_code == expected_status_code - assert response.text == expected_error or any( - expected_error in source for source in response.json()["causes"] - ), response.json() + if expected_error: + assert response.text == expected_error or any( + expected_error in source for source in response.json()["causes"] + ), response.json() @pytest.mark.parametrize("data_category", ["attachment", "attachment_item"]) From 187f733e6f08ee9c4b34ccaa6ca4767b7e1baae4 Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Fri, 17 Jul 2026 14:25:50 +0200 Subject: [PATCH 21/41] wip: fix --- relay-server/src/endpoints/common.rs | 1 - relay-server/src/endpoints/upload.rs | 86 ++++++++++++++++++---------- relay-server/src/services/upload.rs | 4 ++ relay-server/src/utils/tus.rs | 9 +-- tests/integration/conftest.py | 5 +- 5 files changed, 69 insertions(+), 36 deletions(-) diff --git a/relay-server/src/endpoints/common.rs b/relay-server/src/endpoints/common.rs index cc79067816f..77f66c24b04 100644 --- a/relay-server/src/endpoints/common.rs +++ b/relay-server/src/endpoints/common.rs @@ -648,7 +648,6 @@ where referrer = referrer, organization_id = scoping.organization_id.value(), project_id = scoping.project_id.value(), - bytes_uploaded = byte_counter.get(), "multipart item upload finalization failed", ); }) diff --git a/relay-server/src/endpoints/upload.rs b/relay-server/src/endpoints/upload.rs index 35e7f27b2b0..dfcd9ab8433 100644 --- a/relay-server/src/endpoints/upload.rs +++ b/relay-server/src/endpoints/upload.rs @@ -31,7 +31,7 @@ use crate::services::objectstore; use crate::services::projects::cache::Project; use crate::services::projects::project::ProjectState; use crate::services::upload::{ - self, ByteStream, LocationQueryParams, ProjectContext, Provisional, SignedLocation, + self, ByteStream, Final, LocationQueryParams, ProjectContext, Provisional, SignedLocation, UploadLength, }; use crate::services::upstream::UpstreamRequestError; @@ -116,7 +116,7 @@ impl IntoResponse for Error { upload::Error::Objectstore(service_error) => match service_error.kind { objectstore::ErrorKind::InvalidScoping => StatusCode::INTERNAL_SERVER_ERROR, objectstore::ErrorKind::InvalidOffset { .. } => StatusCode::BAD_REQUEST, - objectstore::ErrorKind::OffsetWithoutUploadId { .. } => StatusCode::BAD_REQUEST, + objectstore::ErrorKind::OffsetWithoutUploadId => StatusCode::BAD_REQUEST, objectstore::ErrorKind::InvalidLength { .. } => StatusCode::BAD_REQUEST, objectstore::ErrorKind::Timeout(_) => StatusCode::GATEWAY_TIMEOUT, objectstore::ErrorKind::LoadShed => StatusCode::SERVICE_UNAVAILABLE, @@ -234,7 +234,7 @@ async fn handle_patch( check_kill_switch(&state)?; relay_log::trace!("Validating headers"); - let offset = tus::validate_patch_headers(&headers).map_err(Error::from)?; + let (offset, new_upload_length) = tus::validate_patch_headers(&headers).map_err(Error::from)?; let location = SignedLocation::from_parts( project_id, @@ -262,40 +262,50 @@ async fn handle_patch( relay_log::trace!("Checking request"); let project_context = validate(&state, meta, project).await?; - let stream = body - .into_data_stream() - .map(|result| result.map_err(io::Error::other)) - .boxed(); - let stream = MeteredStream::new(stream, "upload"); - - let (lower_bound, upper_bound) = match upload_length.value() { - None => (1, config.max_upload_size()), - Some(u) => { - let remaining_bytes = u - .checked_sub(offset) - .ok_or(Error::InvalidOffset(offset, u))?; - (1, remaining_bytes) - } - }; - let stream = BoundedStream::new(stream, lower_bound, upper_bound); - let byte_counter = stream.byte_counter(); + let (location_header, upload_offset) = if let Some(untrusted_length) = dbg!(new_upload_length) + && untrusted_length == dbg!(offset) + { + // Sentinel request. + let result = finish(&state, project_context, location, offset).await; + let location = result.inspect_err(|e| { + relay_log::warn!(error = e as &dyn std::error::Error, "upload failed"); + })?; + (location.into_header_value(), offset) + } else { + let stream = body + .into_data_stream() + .map(|result| result.map_err(io::Error::other)) + .boxed(); + let stream = MeteredStream::new(stream, "upload"); + + let (lower_bound, upper_bound) = match upload_length.value() { + None => (0, config.max_upload_size()), + Some(u) => { + let remaining_bytes = u + .checked_sub(offset) + .ok_or(Error::InvalidOffset(offset, u))?; + (0, remaining_bytes) + } + }; + let stream = BoundedStream::new(stream, lower_bound, upper_bound); + let byte_counter = stream.byte_counter(); - relay_log::trace!("Uploading"); - let result = upload(&state, project_context, location, offset, stream).await; - let location = result.inspect_err(|e| { - relay_log::warn!(error = e as &dyn std::error::Error, "upload failed"); - })?; + relay_log::trace!("Uploading"); + let result = upload(&state, project_context, location, offset, stream).await; + let location = result.inspect_err(|e| { + relay_log::warn!(error = e as &dyn std::error::Error, "upload failed"); + })?; - let upload_offset = offset + byte_counter.get(); + let new_offset = offset + byte_counter.get(); + (location.into_header_value(), new_offset) + }; let mut response = NoContent.into_response(); // Not required by TUS, but we respond with the location header: response.headers_mut().insert( header::LOCATION, - location - .into_header_value() - .map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?, + location_header.map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?, ); response .headers_mut() @@ -369,6 +379,24 @@ async fn upload( Ok(location) } +async fn finish( + state: &ServiceState, + project: ProjectContext, + location: SignedLocation, + offset: usize, +) -> Result, Error> { + let location = state + .upload() + .send(upload::Finish { + received: Utc::now(), + project, + location, + trusted_length: offset, // FIXME: truest + }) + .await??; + Ok(location) +} + /// Check request by converting it into a pseudo-envelope. /// /// This is currently the easiest way to guarantee that the upload gets checked the same way as diff --git a/relay-server/src/services/upload.rs b/relay-server/src/services/upload.rs index 5c30d35e2e3..35729d52c76 100644 --- a/relay-server/src/services/upload.rs +++ b/relay-server/src/services/upload.rs @@ -432,6 +432,7 @@ impl Service { } async fn finish(&self, finish: Finish) -> Result, Error> { + relay_log::trace!("Finishing request"); let Finish { #[cfg_attr(not(feature = "processing"), expect(unused))] received, @@ -442,10 +443,13 @@ impl Service { match &self.backend { Backend::Upstream { addr } => { + relay_log::trace!("Assembling request"); let (request, rx) = UploadRequest::finish(project, location.try_to_uri()?, trusted_length); addr.send(SendRequest(request)); + relay_log::trace!("Awaiting response"); let response = rx.await??; + relay_log::trace!("Returning signed location"); SignedLocation::try_from_response(response) } #[cfg(feature = "processing")] diff --git a/relay-server/src/utils/tus.rs b/relay-server/src/utils/tus.rs index d00c1c41ba7..ccb05c52439 100644 --- a/relay-server/src/utils/tus.rs +++ b/relay-server/src/utils/tus.rs @@ -146,7 +146,7 @@ pub fn validate_post_headers(headers: &HeaderMap) -> Result { /// Validates TUS protocol headers and returns the expected upload length. /// /// Returns the offset from which the upload is resumed. -pub fn validate_patch_headers(headers: &HeaderMap) -> Result { +pub fn validate_patch_headers(headers: &HeaderMap) -> Result<(usize, Option), Error> { let tus_version = headers.get(TUS_RESUMABLE); if tus_version != Some(&TUS_VERSION) { return Err(Error::Version( @@ -166,8 +166,9 @@ pub fn validate_patch_headers(headers: &HeaderMap) -> Result { } let upload_offset: usize = parse_header(headers, UPLOAD_OFFSET).ok_or(Error::UploadOffset)?; + let upload_length: Option = parse_header(headers, UPLOAD_LENGTH); - Ok(upload_offset) + Ok((upload_offset, upload_length)) } /// Prepares the required TUS request headers for upstream requests. @@ -480,11 +481,11 @@ mod tests { } #[test] - fn test_validate_patch_headers_nonzero_upload_offset() { + fn test_validate_patch_headers_noninteger_upload_offset() { let mut headers = HeaderMap::new(); headers.insert(TUS_RESUMABLE, HeaderValue::from_static("1.0.0")); headers.insert(http::header::CONTENT_TYPE, EXPECTED_CONTENT_TYPE); - headers.insert(UPLOAD_OFFSET, HeaderValue::from_static("512")); + headers.insert(UPLOAD_OFFSET, HeaderValue::from_static("adsf")); let result = validate_patch_headers(&headers); assert!(matches!(result, Err(Error::UploadOffset))); } diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index 247af52dac0..c78d83fe938 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -317,8 +317,9 @@ def create(**opts): @mini_sentry.app.route("/api//upload//", methods=["PATCH"]) def upload(**opts): - assert request.headers["Content-Encoding"] == "zstd" - assert request.data.startswith(ZSTD_MAGIC_HEADER) + if request.data: + assert request.headers["Content-Encoding"] == "zstd" + assert request.data.startswith(ZSTD_MAGIC_HEADER) return Response( "", status=204, From c100fd479704a12f5124f59978ab6f772d76e55d Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Fri, 17 Jul 2026 14:45:31 +0200 Subject: [PATCH 22/41] fix: finish --- relay-server/src/endpoints/common.rs | 2 +- relay-server/src/endpoints/upload.rs | 7 ++-- relay-server/src/services/objectstore.rs | 42 +++++++++++++++++------- relay-server/src/services/upload.rs | 41 +++++++++-------------- 4 files changed, 51 insertions(+), 41 deletions(-) diff --git a/relay-server/src/endpoints/common.rs b/relay-server/src/endpoints/common.rs index 77f66c24b04..0063bf9d777 100644 --- a/relay-server/src/endpoints/common.rs +++ b/relay-server/src/endpoints/common.rs @@ -638,7 +638,7 @@ where received: Utc::now(), project, location, - trusted_length: byte_counter.get(), + length: byte_counter.get(), }) .await .ok()? diff --git a/relay-server/src/endpoints/upload.rs b/relay-server/src/endpoints/upload.rs index dfcd9ab8433..30a1b60a6c1 100644 --- a/relay-server/src/endpoints/upload.rs +++ b/relay-server/src/endpoints/upload.rs @@ -118,6 +118,7 @@ impl IntoResponse for Error { objectstore::ErrorKind::InvalidOffset { .. } => StatusCode::BAD_REQUEST, objectstore::ErrorKind::OffsetWithoutUploadId => StatusCode::BAD_REQUEST, objectstore::ErrorKind::InvalidLength { .. } => StatusCode::BAD_REQUEST, + objectstore::ErrorKind::UnknownKey { .. } => StatusCode::NOT_FOUND, objectstore::ErrorKind::Timeout(_) => StatusCode::GATEWAY_TIMEOUT, objectstore::ErrorKind::LoadShed => StatusCode::SERVICE_UNAVAILABLE, objectstore::ErrorKind::CompressionFailed(_) => { @@ -262,8 +263,8 @@ async fn handle_patch( relay_log::trace!("Checking request"); let project_context = validate(&state, meta, project).await?; - let (location_header, upload_offset) = if let Some(untrusted_length) = dbg!(new_upload_length) - && untrusted_length == dbg!(offset) + let (location_header, upload_offset) = if let Some(length) = new_upload_length + && length == offset { // Sentinel request. let result = finish(&state, project_context, location, offset).await; @@ -391,7 +392,7 @@ async fn finish( received: Utc::now(), project, location, - trusted_length: offset, // FIXME: truest + length: offset, }) .await??; Ok(location) diff --git a/relay-server/src/services/objectstore.rs b/relay-server/src/services/objectstore.rs index 4fb40eaf294..c103b6d4473 100644 --- a/relay-server/src/services/objectstore.rs +++ b/relay-server/src/services/objectstore.rs @@ -187,7 +187,7 @@ pub struct Finish { pub organization_id: OrganizationId, pub project_id: ProjectId, pub key: String, - pub upload_id: UploadId, + pub upload_id: Option, pub length: usize, } @@ -311,8 +311,10 @@ pub enum ErrorKind { InvalidOffset { offset: usize, granularity: usize }, #[error("offset without upload ID")] OffsetWithoutUploadId, - #[error("invalid upload length {expected}: uploaded parts have a total length of {actual}")] + #[error("invalid upload length {expected}: upload has a total length of {actual}")] InvalidLength { expected: usize, actual: u64 }, + #[error("unknown key {0}")] + UnknownKey(String), #[error("timeout: {0}")] Timeout(#[from] tokio::time::error::Elapsed), #[error("load shed")] @@ -330,8 +332,9 @@ impl ErrorKind { match self { Self::InvalidScoping => "invalid_scoping", Self::InvalidOffset { .. } => "invalid_offset", - Self::OffsetWithoutUploadId { .. } => "offset_without_upload_id", + Self::OffsetWithoutUploadId => "offset_without_upload_id", Self::InvalidLength { .. } => "invalid_length", + Self::UnknownKey { .. } => "invalid_length", Self::Timeout(_) => "timeout", Self::LoadShed => "load_shed", Self::CompressionFailed(_) => "compression_failed", @@ -906,16 +909,31 @@ impl ObjectstoreServiceInner { } = finish; let session = self.session(&self.event_attachments, organization_id, project_id)?; - let multipart = session.resume_multipart_upload(key.clone(), upload_id.to_string())?; - let parts = multipart.list_parts().await?; - validate_multipart_length(&parts, length)?; - - let final_key = multipart - .complete(parts.into_iter().map(Into::into)) - .await?; - debug_assert_eq!(&key, &final_key); + if let Some(upload_id) = upload_id { + let multipart = session.resume_multipart_upload(key.clone(), upload_id.to_string())?; + let parts = multipart.list_parts().await?; + validate_multipart_length(&parts, length)?; + let _final_key = multipart + .complete(parts.into_iter().map(Into::into)) + .await?; + debug_assert_eq!(&key, &_final_key); + } else { + let Some(metadata) = session.head(&key).send().await? else { + return Err(ErrorKind::UnknownKey(key).into()); + }; + if let Some(actual) = metadata.size { + return Err(ErrorKind::InvalidLength { + expected: length, + actual: actual as u64, + } + .into()); + } else { + // NOTE: no good way to validate here, but this branch should disappear once + // we remove the `relay-upload-multipart` feature flag. + } + } - Ok(ObjectstoreKey(final_key)) + Ok(ObjectstoreKey(key)) } async fn upload_bytes( diff --git a/relay-server/src/services/upload.rs b/relay-server/src/services/upload.rs index 35729d52c76..95eca426c00 100644 --- a/relay-server/src/services/upload.rs +++ b/relay-server/src/services/upload.rs @@ -14,7 +14,6 @@ use futures::StreamExt; use futures::stream::BoxStream; use http::{HeaderValue, Method}; #[cfg(feature = "processing")] -use objectstore_types::multipart::UploadId; use relay_auth::Signature; use relay_auth::SignatureError; #[cfg(feature = "processing")] @@ -170,9 +169,7 @@ pub struct Finish { /// The location to finish. pub location: SignedLocation, /// The final length of the upload in bytes. - /// - /// This value must never come from user input. - pub trusted_length: usize, + pub length: usize, } impl FromMessage for Upload { @@ -438,14 +435,13 @@ impl Service { received, project, location, - trusted_length, + length, } = finish; match &self.backend { Backend::Upstream { addr } => { relay_log::trace!("Assembling request"); - let (request, rx) = - UploadRequest::finish(project, location.try_to_uri()?, trusted_length); + let (request, rx) = UploadRequest::finish(project, location.try_to_uri()?, length); addr.send(SendRequest(request)); relay_log::trace!("Awaiting response"); let response = rx.await??; @@ -456,7 +452,7 @@ impl Service { Backend::Objectstore { addr, config } => { let Location { project_id, - mut key, + key, length: _, upload_id, other, @@ -465,26 +461,21 @@ impl Service { let scoping = project.scoping; debug_assert_eq!(scoping.project_id, project_id); - if let Some(upload_id) = upload_id { - key = addr - .send(objectstore::Finish { - organization_id: scoping.organization_id, - project_id, - key, - upload_id: UploadId::new(upload_id)?, - length: trusted_length, - }) - .await - .map_err(Error::ObjectstoreServiceUnavailable)?? - .into_inner(); - } - - // NOTE: `Finish` is never triggered by a client but always by the server, so - // we can trust the `length` here. + let key = addr + .send(objectstore::Finish { + organization_id: scoping.organization_id, + project_id, + key, + upload_id, + length, + }) + .await + .map_err(Error::ObjectstoreServiceUnavailable)?? + .into_inner(); Location { project_id, key, - length: Final(trusted_length), + length: Final(length), upload_id: None, other, } From edea40696fed4d478aa412681d844be61a42499e Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Fri, 17 Jul 2026 14:57:31 +0200 Subject: [PATCH 23/41] lint --- relay-server/src/services/upload.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/relay-server/src/services/upload.rs b/relay-server/src/services/upload.rs index 95eca426c00..c5c9ca9ea13 100644 --- a/relay-server/src/services/upload.rs +++ b/relay-server/src/services/upload.rs @@ -13,7 +13,6 @@ use chrono::Utc; use futures::StreamExt; use futures::stream::BoxStream; use http::{HeaderValue, Method}; -#[cfg(feature = "processing")] use relay_auth::Signature; use relay_auth::SignatureError; #[cfg(feature = "processing")] From c449e5fe1a35e7d0b73e4251131704b14463806a Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Fri, 17 Jul 2026 21:18:25 +0200 Subject: [PATCH 24/41] fix: finish only multipart --- .../src/processing/utils/attachments.rs | 7 +++ relay-server/src/services/objectstore.rs | 47 +++++++------------ relay-server/src/services/upload.rs | 26 +++++----- 3 files changed, 38 insertions(+), 42 deletions(-) diff --git a/relay-server/src/processing/utils/attachments.rs b/relay-server/src/processing/utils/attachments.rs index 4b8bc0af450..cc3fac0f090 100644 --- a/relay-server/src/processing/utils/attachments.rs +++ b/relay-server/src/processing/utils/attachments.rs @@ -52,6 +52,13 @@ fn validate(item: &Item, config: &Config) -> Result<(), ProcessingError> { serde_json::from_slice(&payload).map_err(|_| ProcessingError::InvalidAttachmentRef)?; let signed_location: SignedLocation = SignedLocation::try_from_str(payload.location) .ok_or(ProcessingError::InvalidAttachmentRef)?; + + if signed_location.upload_id().is_some() { + // `SignedLocation` should never have an upload ID. + // NOTE: we could encode this into the `Final` type. + return Err(ProcessingError::InvalidAttachmentRef); + } + // NOTE: Using the received timestamp here breaks tests without a pop-relay. let location = signed_location .verify(chrono::Utc::now(), config) diff --git a/relay-server/src/services/objectstore.rs b/relay-server/src/services/objectstore.rs index c103b6d4473..8435bb3b3f6 100644 --- a/relay-server/src/services/objectstore.rs +++ b/relay-server/src/services/objectstore.rs @@ -59,7 +59,7 @@ pub enum Objectstore { RawProfile(Managed), Create(CreateMultipart, Sender>), Stream(Stream, Sender>), - Finish(Finish, Sender>), + Finish(FinishMultipart, Sender>), } impl Objectstore { @@ -183,18 +183,21 @@ impl FromMessage for Objectstore { } /// A request to finish an objectstore multipart upload. -pub struct Finish { +pub struct FinishMultipart { pub organization_id: OrganizationId, pub project_id: ProjectId, pub key: String, - pub upload_id: Option, + pub upload_id: String, pub length: usize, } -impl FromMessage for Objectstore { +impl FromMessage for Objectstore { type Response = AsyncResponse>; - fn from_message(message: Finish, sender: Sender>) -> Self { + fn from_message( + message: FinishMultipart, + sender: Sender>, + ) -> Self { Self::Finish(message, sender) } } @@ -899,8 +902,8 @@ impl ObjectstoreServiceInner { .await } - async fn handle_finish(&self, finish: Finish) -> Result { - let Finish { + async fn handle_finish(&self, finish: FinishMultipart) -> Result { + let FinishMultipart { organization_id, project_id, key, @@ -909,29 +912,13 @@ impl ObjectstoreServiceInner { } = finish; let session = self.session(&self.event_attachments, organization_id, project_id)?; - if let Some(upload_id) = upload_id { - let multipart = session.resume_multipart_upload(key.clone(), upload_id.to_string())?; - let parts = multipart.list_parts().await?; - validate_multipart_length(&parts, length)?; - let _final_key = multipart - .complete(parts.into_iter().map(Into::into)) - .await?; - debug_assert_eq!(&key, &_final_key); - } else { - let Some(metadata) = session.head(&key).send().await? else { - return Err(ErrorKind::UnknownKey(key).into()); - }; - if let Some(actual) = metadata.size { - return Err(ErrorKind::InvalidLength { - expected: length, - actual: actual as u64, - } - .into()); - } else { - // NOTE: no good way to validate here, but this branch should disappear once - // we remove the `relay-upload-multipart` feature flag. - } - } + let multipart = session.resume_multipart_upload(key.clone(), upload_id.to_string())?; + let parts = multipart.list_parts().await?; + validate_multipart_length(&parts, length)?; + let _final_key = multipart + .complete(parts.into_iter().map(Into::into)) + .await?; + debug_assert_eq!(&key, &_final_key); Ok(ObjectstoreKey(key)) } diff --git a/relay-server/src/services/upload.rs b/relay-server/src/services/upload.rs index c5c9ca9ea13..24b46e74f80 100644 --- a/relay-server/src/services/upload.rs +++ b/relay-server/src/services/upload.rs @@ -451,7 +451,7 @@ impl Service { Backend::Objectstore { addr, config } => { let Location { project_id, - key, + mut key, length: _, upload_id, other, @@ -460,17 +460,19 @@ impl Service { let scoping = project.scoping; debug_assert_eq!(scoping.project_id, project_id); - let key = addr - .send(objectstore::Finish { - organization_id: scoping.organization_id, - project_id, - key, - upload_id, - length, - }) - .await - .map_err(Error::ObjectstoreServiceUnavailable)?? - .into_inner(); + if let Some(upload_id) = upload_id { + key = addr + .send(objectstore::FinishMultipart { + organization_id: scoping.organization_id, + project_id, + key, + upload_id, + length, + }) + .await + .map_err(Error::ObjectstoreServiceUnavailable)?? + .into_inner(); + } Location { project_id, key, From 0bae8de6101c679cc5336e0d27ff759d27310da5 Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Fri, 17 Jul 2026 21:22:49 +0200 Subject: [PATCH 25/41] changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3e189794024..373c032a5d5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ - No longer write the deprecated `sentry.transaction` and `db.system` attributes. ([#6237](https://github.com/getsentry/relay/pull/6237), [#6238](https://github.com/getsentry/relay/pull/6238)) - Allow additional exceptions in minidump and apple crash report events. ([#6241](https://github.com/getsentry/relay/pull/6241)) +**Internal**: + +- Allow resumable uploads (feature-flagged). ([#6203](https://github.com/getsentry/relay/pull/6203)) + ## 26.7.0 **Features**: From 3fcff0ca06185f5dc3d7655c331bbd15c5c60261 Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Fri, 17 Jul 2026 22:12:56 +0200 Subject: [PATCH 26/41] unflake --- tests/integration/test_upload.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/integration/test_upload.py b/tests/integration/test_upload.py index b625bb53808..5443ae815c7 100644 --- a/tests/integration/test_upload.py +++ b/tests/integration/test_upload.py @@ -666,6 +666,7 @@ def multipart_upload(**opts): def test_upload_offset( mini_sentry, relay_with_processing, project_config, objectstore, feature_flag ): + mini_sentry.fail_on_relay_error = False project_id = 42 config = mini_sentry.add_full_project_config(project_id)["config"] features = config.setdefault("features", []) From 687d494480b43473802c31ee5dc9cf5e22b9fdcb Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Fri, 17 Jul 2026 22:28:23 +0200 Subject: [PATCH 27/41] review --- relay-server/src/services/objectstore.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/relay-server/src/services/objectstore.rs b/relay-server/src/services/objectstore.rs index 8435bb3b3f6..83549382364 100644 --- a/relay-server/src/services/objectstore.rs +++ b/relay-server/src/services/objectstore.rs @@ -1083,9 +1083,8 @@ impl ObjectstoreServiceInner { // Every chunk needs to be > 5 MB because that's what multipart requires. let mut parts = vec![]; let mut compressed_buffer = zstd::stream::write::Encoder::new(vec![], 0).map_err(AttemptUploadError::Zstd)?; - let mut part_number = offset / UPLOAD_GRANULARITY; + let mut part_number = offset / UPLOAD_GRANULARITY + 1; while let Some(bytes) = body.next().await { - part_number += 1; match bytes { Err(error) => { self.try_flush(&mut compressed_buffer, part_number, &mut multipart_upload, &mut parts).await?; @@ -1097,6 +1096,7 @@ impl ObjectstoreServiceInner { } } } + part_number += 1; } self.try_flush(&mut compressed_buffer, part_number, &mut multipart_upload, &mut parts).await?; @@ -1133,6 +1133,9 @@ impl ObjectstoreServiceInner { multipart: &mut MultipartUpload, parts: &mut Vec, ) -> Result<(), AttemptUploadError> { + if buffer.get_ref().is_empty() { + return Ok(()); + } let mut new_buffer = zstd::stream::write::Encoder::new(vec![], 0).map_err(AttemptUploadError::Zstd)?; std::mem::swap(buffer, &mut new_buffer); From e3b6dd3dda8012d4af167db8dc360d25f6a96e3e Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Fri, 17 Jul 2026 23:11:55 +0200 Subject: [PATCH 28/41] fixes --- relay-server/src/endpoints/common.rs | 1 - relay-server/src/endpoints/upload.rs | 14 +++-- relay-server/src/services/objectstore.rs | 45 ---------------- relay-server/src/services/upload.rs | 68 +++++++++++++++++------- 4 files changed, 57 insertions(+), 71 deletions(-) diff --git a/relay-server/src/endpoints/common.rs b/relay-server/src/endpoints/common.rs index 0063bf9d777..c11ca5ec492 100644 --- a/relay-server/src/endpoints/common.rs +++ b/relay-server/src/endpoints/common.rs @@ -638,7 +638,6 @@ where received: Utc::now(), project, location, - length: byte_counter.get(), }) .await .ok()? diff --git a/relay-server/src/endpoints/upload.rs b/relay-server/src/endpoints/upload.rs index 30a1b60a6c1..3f2286f15a2 100644 --- a/relay-server/src/endpoints/upload.rs +++ b/relay-server/src/endpoints/upload.rs @@ -263,11 +263,17 @@ async fn handle_patch( relay_log::trace!("Checking request"); let project_context = validate(&state, meta, project).await?; + // When a PATCH request comes in that has `Upload-Offset == Upload-Length == upload_length from location`, + // Treat it as a sentinel request that finalizes the upload. + // + // At this point the `upload_length` is still untrusted, but the upload service will verify + // it with the given signature. let (location_header, upload_offset) = if let Some(length) = new_upload_length && length == offset + && Some(length) == upload_length.value() { // Sentinel request. - let result = finish(&state, project_context, location, offset).await; + let result = finish(&state, project_context, location.into_final(length)).await; let location = result.inspect_err(|e| { relay_log::warn!(error = e as &dyn std::error::Error, "upload failed"); })?; @@ -365,7 +371,7 @@ async fn upload( location: SignedLocation, offset: usize, stream: BoundedStream>, -) -> Result, Error> { +) -> Result, Error> { let location = state .upload() .send(upload::Stream { @@ -383,8 +389,7 @@ async fn upload( async fn finish( state: &ServiceState, project: ProjectContext, - location: SignedLocation, - offset: usize, + location: SignedLocation, ) -> Result, Error> { let location = state .upload() @@ -392,7 +397,6 @@ async fn finish( received: Utc::now(), project, location, - length: offset, }) .await??; Ok(location) diff --git a/relay-server/src/services/objectstore.rs b/relay-server/src/services/objectstore.rs index 83549382364..756c4b01a37 100644 --- a/relay-server/src/services/objectstore.rs +++ b/relay-server/src/services/objectstore.rs @@ -188,7 +188,6 @@ pub struct FinishMultipart { pub project_id: ProjectId, pub key: String, pub upload_id: String, - pub length: usize, } impl FromMessage for Objectstore { @@ -908,13 +907,11 @@ impl ObjectstoreServiceInner { project_id, key, upload_id, - length, } = finish; let session = self.session(&self.event_attachments, organization_id, project_id)?; let multipart = session.resume_multipart_upload(key.clone(), upload_id.to_string())?; let parts = multipart.list_parts().await?; - validate_multipart_length(&parts, length)?; let _final_key = multipart .complete(parts.into_iter().map(Into::into)) .await?; @@ -1186,18 +1183,6 @@ impl ObjectstoreServiceInner { } } -fn validate_multipart_length(parts: &[PartInfo], expected: usize) -> Result<(), ErrorKind> { - let actual = parts - .iter() - .fold(0u64, |sum, part| sum.saturating_add(part.size)); - - if actual != expected as u64 { - return Err(ErrorKind::InvalidLength { expected, actual }); - } - - Ok(()) -} - #[derive(Debug, thiserror::Error)] enum AttemptUploadError { #[error(transparent)] @@ -1330,9 +1315,6 @@ fn drop_attachments(event: &mut Managed>) { #[cfg(test)] mod tests { - use std::num::NonZeroU32; - use std::time::SystemTime; - use relay_event_schema::protocol::EventId; use relay_quotas::DataCategory; use relay_system::Service; @@ -1341,33 +1323,6 @@ mod tests { use super::*; - #[test] - fn validates_multipart_length() { - let parts = [ - PartInfo { - part_number: NonZeroU32::new(1).unwrap(), - etag: "first".to_owned(), - last_modified: SystemTime::UNIX_EPOCH, - size: 3, - }, - PartInfo { - part_number: NonZeroU32::new(2).unwrap(), - etag: "second".to_owned(), - last_modified: SystemTime::UNIX_EPOCH, - size: 5, - }, - ]; - - assert!(validate_multipart_length(&parts, 8).is_ok()); - assert!(matches!( - validate_multipart_length(&parts, 9), - Err(ErrorKind::InvalidLength { - expected: 9, - actual: 8 - }) - )); - } - #[tokio::test] async fn org_zero_rejected() { let service = ObjectstoreService::new(&test_config(), Some(Addr::dummy())) diff --git a/relay-server/src/services/upload.rs b/relay-server/src/services/upload.rs index 24b46e74f80..39ccf724179 100644 --- a/relay-server/src/services/upload.rs +++ b/relay-server/src/services/upload.rs @@ -110,9 +110,8 @@ pub enum Upload { Create(Create, InstrumentedSender), /// Upload a stream of bytes for a given location. /// - /// The service also returns the signed location. This is redundant, but creates a simpler - /// flow for the caller side. - Stream(Box, InstrumentedSender), + /// The service returns the signed location with an updated `upload_length` parameter. + Stream(Box, InstrumentedSender), /// Finishes an upload and declares its final length. Finish(Finish, InstrumentedSender), } @@ -166,9 +165,7 @@ pub struct Finish { /// The project the upload belongs to. pub project: ProjectContext, /// The location to finish. - pub location: SignedLocation, - /// The final length of the upload in bytes. - pub length: usize, + pub location: SignedLocation, } impl FromMessage for Upload { @@ -189,12 +186,9 @@ impl FromMessage for Upload { } impl FromMessage for Upload { - type Response = AsyncResponse, Error>>; + type Response = AsyncResponse, Error>>; - fn from_message( - message: Stream, - sender: Sender, Error>>, - ) -> Self { + fn from_message(message: Stream, sender: Sender, Error>>) -> Self { Self::Stream( Box::new(message), InstrumentedSender { @@ -365,7 +359,7 @@ impl Service { } } - async fn upload(&self, stream: Stream) -> Result, Error> { + async fn upload(&self, stream: Stream) -> Result, Error> { let Stream { #[cfg_attr(not(feature = "processing"), expect(unused))] received, @@ -398,6 +392,7 @@ impl Service { debug_assert_eq!(scoping.project_id, project_id); debug_assert!(stream.length().is_none_or(|l| Some(l) == length.value())); let upload_ref = UploadRef::new(key, upload_id, offset, length.value())?; + let byte_counter = stream.byte_counter(); let upload_ref = addr .send(objectstore::Stream { organization_id: scoping.organization_id, @@ -411,14 +406,14 @@ impl Service { let UploadRef { key, upload_id, - offset: _, - length, + offset, + length: _, } = upload_ref; Location { project_id, key, - length: Provisional(length), + length: Final(offset + byte_counter.get()), upload_id: upload_id.map(|id| id.to_string()), other, } @@ -434,13 +429,16 @@ impl Service { received, project, location, - length, } = finish; match &self.backend { Backend::Upstream { addr } => { relay_log::trace!("Assembling request"); - let (request, rx) = UploadRequest::finish(project, location.try_to_uri()?, length); + let (request, rx) = UploadRequest::finish( + project, + location.try_to_uri()?, + location.location.length.into_inner(), + ); addr.send(SendRequest(request)); relay_log::trace!("Awaiting response"); let response = rx.await??; @@ -452,7 +450,7 @@ impl Service { let Location { project_id, mut key, - length: _, + length, upload_id, other, } = location.verify(received, config)?; @@ -467,7 +465,6 @@ impl Service { project_id, key, upload_id, - length, }) .await .map_err(Error::ObjectstoreServiceUnavailable)?? @@ -476,7 +473,7 @@ impl Service { Location { project_id, key, - length: Final(length), + length, upload_id: None, other, } @@ -549,6 +546,10 @@ impl UploadLength for Provisional { pub struct Final(usize); impl Final { + pub fn new(value: usize) -> Self { + Self(value) + } + /// Get the value. pub fn into_inner(self) -> usize { self.0 @@ -816,6 +817,33 @@ where } } +impl SignedLocation { + /// Transforms a provisional into a final length by overriding its `upload_length`. + pub fn into_final(self, length: usize) -> SignedLocation { + let Self { + location: + Location { + project_id, + key, + length: provisional_length, + upload_id, + other, + }, + signature, + } = self; + debug_assert!(provisional_length.value().is_none_or(|l| l == length)); + SignedLocation { + location: Location { + project_id, + key, + length: Final(length), + upload_id, + other, + }, + signature, + } + } +} enum RequestKind { Create { length: Option, From c665f6b1bae74e19f67f14363900e5eec34ce478 Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Fri, 17 Jul 2026 23:37:46 +0200 Subject: [PATCH 29/41] Revert "fixes" This reverts commit e3b6dd3dda8012d4af167db8dc360d25f6a96e3e. --- relay-server/src/endpoints/common.rs | 1 + relay-server/src/endpoints/upload.rs | 14 ++--- relay-server/src/services/objectstore.rs | 45 ++++++++++++++++ relay-server/src/services/upload.rs | 68 +++++++----------------- 4 files changed, 71 insertions(+), 57 deletions(-) diff --git a/relay-server/src/endpoints/common.rs b/relay-server/src/endpoints/common.rs index c11ca5ec492..0063bf9d777 100644 --- a/relay-server/src/endpoints/common.rs +++ b/relay-server/src/endpoints/common.rs @@ -638,6 +638,7 @@ where received: Utc::now(), project, location, + length: byte_counter.get(), }) .await .ok()? diff --git a/relay-server/src/endpoints/upload.rs b/relay-server/src/endpoints/upload.rs index 3f2286f15a2..30a1b60a6c1 100644 --- a/relay-server/src/endpoints/upload.rs +++ b/relay-server/src/endpoints/upload.rs @@ -263,17 +263,11 @@ async fn handle_patch( relay_log::trace!("Checking request"); let project_context = validate(&state, meta, project).await?; - // When a PATCH request comes in that has `Upload-Offset == Upload-Length == upload_length from location`, - // Treat it as a sentinel request that finalizes the upload. - // - // At this point the `upload_length` is still untrusted, but the upload service will verify - // it with the given signature. let (location_header, upload_offset) = if let Some(length) = new_upload_length && length == offset - && Some(length) == upload_length.value() { // Sentinel request. - let result = finish(&state, project_context, location.into_final(length)).await; + let result = finish(&state, project_context, location, offset).await; let location = result.inspect_err(|e| { relay_log::warn!(error = e as &dyn std::error::Error, "upload failed"); })?; @@ -371,7 +365,7 @@ async fn upload( location: SignedLocation, offset: usize, stream: BoundedStream>, -) -> Result, Error> { +) -> Result, Error> { let location = state .upload() .send(upload::Stream { @@ -389,7 +383,8 @@ async fn upload( async fn finish( state: &ServiceState, project: ProjectContext, - location: SignedLocation, + location: SignedLocation, + offset: usize, ) -> Result, Error> { let location = state .upload() @@ -397,6 +392,7 @@ async fn finish( received: Utc::now(), project, location, + length: offset, }) .await??; Ok(location) diff --git a/relay-server/src/services/objectstore.rs b/relay-server/src/services/objectstore.rs index 756c4b01a37..83549382364 100644 --- a/relay-server/src/services/objectstore.rs +++ b/relay-server/src/services/objectstore.rs @@ -188,6 +188,7 @@ pub struct FinishMultipart { pub project_id: ProjectId, pub key: String, pub upload_id: String, + pub length: usize, } impl FromMessage for Objectstore { @@ -907,11 +908,13 @@ impl ObjectstoreServiceInner { project_id, key, upload_id, + length, } = finish; let session = self.session(&self.event_attachments, organization_id, project_id)?; let multipart = session.resume_multipart_upload(key.clone(), upload_id.to_string())?; let parts = multipart.list_parts().await?; + validate_multipart_length(&parts, length)?; let _final_key = multipart .complete(parts.into_iter().map(Into::into)) .await?; @@ -1183,6 +1186,18 @@ impl ObjectstoreServiceInner { } } +fn validate_multipart_length(parts: &[PartInfo], expected: usize) -> Result<(), ErrorKind> { + let actual = parts + .iter() + .fold(0u64, |sum, part| sum.saturating_add(part.size)); + + if actual != expected as u64 { + return Err(ErrorKind::InvalidLength { expected, actual }); + } + + Ok(()) +} + #[derive(Debug, thiserror::Error)] enum AttemptUploadError { #[error(transparent)] @@ -1315,6 +1330,9 @@ fn drop_attachments(event: &mut Managed>) { #[cfg(test)] mod tests { + use std::num::NonZeroU32; + use std::time::SystemTime; + use relay_event_schema::protocol::EventId; use relay_quotas::DataCategory; use relay_system::Service; @@ -1323,6 +1341,33 @@ mod tests { use super::*; + #[test] + fn validates_multipart_length() { + let parts = [ + PartInfo { + part_number: NonZeroU32::new(1).unwrap(), + etag: "first".to_owned(), + last_modified: SystemTime::UNIX_EPOCH, + size: 3, + }, + PartInfo { + part_number: NonZeroU32::new(2).unwrap(), + etag: "second".to_owned(), + last_modified: SystemTime::UNIX_EPOCH, + size: 5, + }, + ]; + + assert!(validate_multipart_length(&parts, 8).is_ok()); + assert!(matches!( + validate_multipart_length(&parts, 9), + Err(ErrorKind::InvalidLength { + expected: 9, + actual: 8 + }) + )); + } + #[tokio::test] async fn org_zero_rejected() { let service = ObjectstoreService::new(&test_config(), Some(Addr::dummy())) diff --git a/relay-server/src/services/upload.rs b/relay-server/src/services/upload.rs index 39ccf724179..24b46e74f80 100644 --- a/relay-server/src/services/upload.rs +++ b/relay-server/src/services/upload.rs @@ -110,8 +110,9 @@ pub enum Upload { Create(Create, InstrumentedSender), /// Upload a stream of bytes for a given location. /// - /// The service returns the signed location with an updated `upload_length` parameter. - Stream(Box, InstrumentedSender), + /// The service also returns the signed location. This is redundant, but creates a simpler + /// flow for the caller side. + Stream(Box, InstrumentedSender), /// Finishes an upload and declares its final length. Finish(Finish, InstrumentedSender), } @@ -165,7 +166,9 @@ pub struct Finish { /// The project the upload belongs to. pub project: ProjectContext, /// The location to finish. - pub location: SignedLocation, + pub location: SignedLocation, + /// The final length of the upload in bytes. + pub length: usize, } impl FromMessage for Upload { @@ -186,9 +189,12 @@ impl FromMessage for Upload { } impl FromMessage for Upload { - type Response = AsyncResponse, Error>>; + type Response = AsyncResponse, Error>>; - fn from_message(message: Stream, sender: Sender, Error>>) -> Self { + fn from_message( + message: Stream, + sender: Sender, Error>>, + ) -> Self { Self::Stream( Box::new(message), InstrumentedSender { @@ -359,7 +365,7 @@ impl Service { } } - async fn upload(&self, stream: Stream) -> Result, Error> { + async fn upload(&self, stream: Stream) -> Result, Error> { let Stream { #[cfg_attr(not(feature = "processing"), expect(unused))] received, @@ -392,7 +398,6 @@ impl Service { debug_assert_eq!(scoping.project_id, project_id); debug_assert!(stream.length().is_none_or(|l| Some(l) == length.value())); let upload_ref = UploadRef::new(key, upload_id, offset, length.value())?; - let byte_counter = stream.byte_counter(); let upload_ref = addr .send(objectstore::Stream { organization_id: scoping.organization_id, @@ -406,14 +411,14 @@ impl Service { let UploadRef { key, upload_id, - offset, - length: _, + offset: _, + length, } = upload_ref; Location { project_id, key, - length: Final(offset + byte_counter.get()), + length: Provisional(length), upload_id: upload_id.map(|id| id.to_string()), other, } @@ -429,16 +434,13 @@ impl Service { received, project, location, + length, } = finish; match &self.backend { Backend::Upstream { addr } => { relay_log::trace!("Assembling request"); - let (request, rx) = UploadRequest::finish( - project, - location.try_to_uri()?, - location.location.length.into_inner(), - ); + let (request, rx) = UploadRequest::finish(project, location.try_to_uri()?, length); addr.send(SendRequest(request)); relay_log::trace!("Awaiting response"); let response = rx.await??; @@ -450,7 +452,7 @@ impl Service { let Location { project_id, mut key, - length, + length: _, upload_id, other, } = location.verify(received, config)?; @@ -465,6 +467,7 @@ impl Service { project_id, key, upload_id, + length, }) .await .map_err(Error::ObjectstoreServiceUnavailable)?? @@ -473,7 +476,7 @@ impl Service { Location { project_id, key, - length, + length: Final(length), upload_id: None, other, } @@ -546,10 +549,6 @@ impl UploadLength for Provisional { pub struct Final(usize); impl Final { - pub fn new(value: usize) -> Self { - Self(value) - } - /// Get the value. pub fn into_inner(self) -> usize { self.0 @@ -817,33 +816,6 @@ where } } -impl SignedLocation { - /// Transforms a provisional into a final length by overriding its `upload_length`. - pub fn into_final(self, length: usize) -> SignedLocation { - let Self { - location: - Location { - project_id, - key, - length: provisional_length, - upload_id, - other, - }, - signature, - } = self; - debug_assert!(provisional_length.value().is_none_or(|l| l == length)); - SignedLocation { - location: Location { - project_id, - key, - length: Final(length), - upload_id, - other, - }, - signature, - } - } -} enum RequestKind { Create { length: Option, From b97afdc364db49804825ebfc4a82d25c0dcc12c4 Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Sat, 18 Jul 2026 13:36:48 +0200 Subject: [PATCH 30/41] no finish --- relay-server/src/endpoints/common.rs | 23 +- relay-server/src/endpoints/upload.rs | 92 ++---- relay-server/src/services/objectstore.rs | 385 +++++++++-------------- relay-server/src/services/upload.rs | 198 ++---------- relay-server/src/statsd.rs | 6 - relay-server/src/utils/tus.rs | 5 +- 6 files changed, 203 insertions(+), 506 deletions(-) diff --git a/relay-server/src/endpoints/common.rs b/relay-server/src/endpoints/common.rs index 0063bf9d777..c01a07d8c52 100644 --- a/relay-server/src/endpoints/common.rs +++ b/relay-server/src/endpoints/common.rs @@ -24,7 +24,7 @@ use crate::service::ServiceState; use crate::services::buffer::{ProjectKeyPair, PushError}; use crate::services::outcome::{DiscardItemType, DiscardReason, Outcome}; use crate::services::processor::{BucketSource, MetricData, ProcessMetrics}; -use crate::services::upload::{Create, Finish, ProjectContext, Stream, Upload}; +use crate::services::upload::{Create, ProjectContext, Stream, Upload}; use crate::statsd::{RelayCounters, RelayDistributions}; use crate::utils::{ self, ApiErrorResponse, BoundedStream, FormDataIter, MeteredStream, find_error_source, @@ -632,27 +632,6 @@ where }) .ok()?; - let location = { - upload - .send(Finish { - received: Utc::now(), - project, - location, - length: byte_counter.get(), - }) - .await - .ok()? - .inspect_err(|e| { - relay_log::warn!( - error = e as &dyn std::error::Error, - referrer = referrer, - organization_id = scoping.organization_id.value(), - project_id = scoping.project_id.value(), - "multipart item upload finalization failed", - ); - }) - .ok()? - }; let location = location.into_header_value().ok()?; let location = location.to_str().ok()?; let placeholder = serde_json::to_vec(&AttachmentPlaceholder { diff --git a/relay-server/src/endpoints/upload.rs b/relay-server/src/endpoints/upload.rs index 30a1b60a6c1..42a0c5ea581 100644 --- a/relay-server/src/endpoints/upload.rs +++ b/relay-server/src/endpoints/upload.rs @@ -31,7 +31,7 @@ use crate::services::objectstore; use crate::services::projects::cache::Project; use crate::services::projects::project::ProjectState; use crate::services::upload::{ - self, ByteStream, Final, LocationQueryParams, ProjectContext, Provisional, SignedLocation, + self, ByteStream, LocationQueryParams, ProjectContext, Provisional, SignedLocation, UploadLength, }; use crate::services::upstream::UpstreamRequestError; @@ -117,7 +117,7 @@ impl IntoResponse for Error { objectstore::ErrorKind::InvalidScoping => StatusCode::INTERNAL_SERVER_ERROR, objectstore::ErrorKind::InvalidOffset { .. } => StatusCode::BAD_REQUEST, objectstore::ErrorKind::OffsetWithoutUploadId => StatusCode::BAD_REQUEST, - objectstore::ErrorKind::InvalidLength { .. } => StatusCode::BAD_REQUEST, + objectstore::ErrorKind::InvalidUploadLength { .. } => StatusCode::BAD_REQUEST, objectstore::ErrorKind::UnknownKey { .. } => StatusCode::NOT_FOUND, objectstore::ErrorKind::Timeout(_) => StatusCode::GATEWAY_TIMEOUT, objectstore::ErrorKind::LoadShed => StatusCode::SERVICE_UNAVAILABLE, @@ -235,7 +235,7 @@ async fn handle_patch( check_kill_switch(&state)?; relay_log::trace!("Validating headers"); - let (offset, new_upload_length) = tus::validate_patch_headers(&headers).map_err(Error::from)?; + let offset = tus::validate_patch_headers(&headers).map_err(Error::from)?; let location = SignedLocation::from_parts( project_id, @@ -263,57 +263,45 @@ async fn handle_patch( relay_log::trace!("Checking request"); let project_context = validate(&state, meta, project).await?; - let (location_header, upload_offset) = if let Some(length) = new_upload_length - && length == offset - { - // Sentinel request. - let result = finish(&state, project_context, location, offset).await; - let location = result.inspect_err(|e| { - relay_log::warn!(error = e as &dyn std::error::Error, "upload failed"); - })?; - (location.into_header_value(), offset) - } else { - let stream = body - .into_data_stream() - .map(|result| result.map_err(io::Error::other)) - .boxed(); - let stream = MeteredStream::new(stream, "upload"); - - let (lower_bound, upper_bound) = match upload_length.value() { - None => (0, config.max_upload_size()), - Some(u) => { - let remaining_bytes = u - .checked_sub(offset) - .ok_or(Error::InvalidOffset(offset, u))?; - (0, remaining_bytes) - } - }; - let stream = BoundedStream::new(stream, lower_bound, upper_bound); - let byte_counter = stream.byte_counter(); + let stream = body + .into_data_stream() + .map(|result| result.map_err(io::Error::other)) + .boxed(); + let stream = MeteredStream::new(stream, "upload"); + + let (lower_bound, upper_bound) = match upload_length.value() { + None => (0, config.max_upload_size()), + Some(u) => { + let remaining_bytes = u + .checked_sub(offset) + .ok_or(Error::InvalidOffset(offset, u))?; + (0, remaining_bytes) + } + }; + let stream = BoundedStream::new(stream, lower_bound, upper_bound); + let byte_counter = stream.byte_counter(); - relay_log::trace!("Uploading"); - let result = upload(&state, project_context, location, offset, stream).await; - let location = result.inspect_err(|e| { - relay_log::warn!(error = e as &dyn std::error::Error, "upload failed"); - })?; + relay_log::trace!("Uploading"); + let result = upload(&state, project_context, location, offset, stream).await; + let location = result.inspect_err(|e| { + relay_log::warn!(error = e as &dyn std::error::Error, "upload failed"); + })?; - let new_offset = offset + byte_counter.get(); - (location.into_header_value(), new_offset) - }; + let new_offset = offset + byte_counter.get(); let mut response = NoContent.into_response(); // Not required by TUS, but we respond with the location header: - response.headers_mut().insert( - header::LOCATION, - location_header.map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?, - ); + let location = location + .into_header_value() + .map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?; + response.headers_mut().insert(header::LOCATION, location); response .headers_mut() .insert(tus::TUS_RESUMABLE, tus::TUS_VERSION); response .headers_mut() - .insert(tus::UPLOAD_OFFSET, upload_offset.into()); + .insert(tus::UPLOAD_OFFSET, new_offset.into()); Ok(response) } @@ -380,24 +368,6 @@ async fn upload( Ok(location) } -async fn finish( - state: &ServiceState, - project: ProjectContext, - location: SignedLocation, - offset: usize, -) -> Result, Error> { - let location = state - .upload() - .send(upload::Finish { - received: Utc::now(), - project, - location, - length: offset, - }) - .await??; - Ok(location) -} - /// Check request by converting it into a pseudo-envelope. /// /// This is currently the easiest way to guarantee that the upload gets checked the same way as diff --git a/relay-server/src/services/objectstore.rs b/relay-server/src/services/objectstore.rs index 83549382364..6fcf4ba22c3 100644 --- a/relay-server/src/services/objectstore.rs +++ b/relay-server/src/services/objectstore.rs @@ -35,8 +35,7 @@ use crate::services::store::{ use crate::services::upload::ByteStream; use crate::statsd::{RelayCounters, RelayTimers}; use crate::utils::{ - BoundedStream, ByteCounter, MeteredStream, Rechunk, RetryableStream, TakeOnce, - find_error_source, + BoundedStream, MeteredStream, Rechunk, RetryableStream, TakeOnce, find_error_source, }; use super::outcome::Outcome; @@ -59,7 +58,6 @@ pub enum Objectstore { RawProfile(Managed), Create(CreateMultipart, Sender>), Stream(Stream, Sender>), - Finish(FinishMultipart, Sender>), } impl Objectstore { @@ -71,7 +69,6 @@ impl Objectstore { Self::RawProfile(_) => MessageKind::RawProfile, Self::Stream { .. } => MessageKind::Stream, Self::Create { .. } => MessageKind::Create, - Self::Finish { .. } => MessageKind::Finish, } } @@ -83,7 +80,6 @@ impl Objectstore { Self::RawProfile(_) => 1, Self::Stream { .. } => 1, Self::Create { .. } => 0, - Self::Finish { .. } => 1, } } } @@ -131,7 +127,6 @@ enum MessageKind { RawProfile, Stream, Create, - Finish, } impl MessageKind { @@ -143,7 +138,6 @@ impl MessageKind { Self::RawProfile => "profile_raw", Self::Stream => "stream", Self::Create => "create", - Self::Finish => "finish", } } } @@ -166,11 +160,22 @@ impl FromMessage for Objectstore { } } +#[derive(Clone)] +pub enum StreamingMode { + Oneshot, + Multipart { + upload_id: String, + offset: usize, + length: usize, + }, +} + /// A stream that can be uploaded to objectstore. pub struct Stream { pub organization_id: OrganizationId, pub project_id: ProjectId, - pub upload_ref: UploadRef, + pub key: String, + pub mode: StreamingMode, pub stream: BoundedStream>, } @@ -182,26 +187,6 @@ impl FromMessage for Objectstore { } } -/// A request to finish an objectstore multipart upload. -pub struct FinishMultipart { - pub organization_id: OrganizationId, - pub project_id: ProjectId, - pub key: String, - pub upload_id: String, - pub length: usize, -} - -impl FromMessage for Objectstore { - type Response = AsyncResponse>; - - fn from_message( - message: FinishMultipart, - sender: Sender>, - ) -> Self { - Self::Finish(message, sender) - } -} - /// An attachment that is ready for upload / EAP storage. pub struct StoreTraceAttachment { /// The body to be uploaded to objectstore. @@ -314,8 +299,8 @@ pub enum ErrorKind { InvalidOffset { offset: usize, granularity: usize }, #[error("offset without upload ID")] OffsetWithoutUploadId, - #[error("invalid upload length {expected}: upload has a total length of {actual}")] - InvalidLength { expected: usize, actual: u64 }, + #[error("invalid Upload-Length {length}: server has {server_offset} bytes")] + InvalidUploadLength { length: usize, server_offset: usize }, #[error("unknown key {0}")] UnknownKey(String), #[error("timeout: {0}")] @@ -336,7 +321,7 @@ impl ErrorKind { Self::InvalidScoping => "invalid_scoping", Self::InvalidOffset { .. } => "invalid_offset", Self::OffsetWithoutUploadId => "offset_without_upload_id", - Self::InvalidLength { .. } => "invalid_length", + Self::InvalidUploadLength { .. } => "invalid_upload_length", Self::UnknownKey { .. } => "invalid_length", Self::Timeout(_) => "timeout", Self::LoadShed => "load_shed", @@ -348,7 +333,7 @@ impl ErrorKind { fn is_client_error(&self) -> bool { match self { - ErrorKind::InvalidOffset { .. } | ErrorKind::InvalidLength { .. } => true, + ErrorKind::InvalidOffset { .. } | ErrorKind::InvalidUploadLength { .. } => true, ErrorKind::UploadFailed(objectstore_client::Error::Reqwest(error)) => { find_error_source(error, is_user_error).is_some() } @@ -384,35 +369,16 @@ pub struct UploadRef { /// The ID of the multipart upload session (chosen by objectstore). /// `None` if the upload is not multipart. pub upload_id: Option, - /// The offset in bytes from which to resume an upload. - /// - /// Zero for new uploads. - pub offset: usize, - - /// The total length of the upload in bytes. - /// - /// Must be `Some` in order to finalize an upload. - pub length: Option, } impl UploadRef { /// Validates the upload ID and returns a new upload reference. - pub fn new( - key: String, - upload_id: Option, - offset: usize, - length: Option, - ) -> Result { + pub fn new(key: String, upload_id: Option) -> Result { let upload_id = match upload_id { Some(s) => Some(UploadId::new(s)?), None => None, }; - Ok(Self { - key, - upload_id, - offset, - length, - }) + Ok(Self { key, upload_id }) } } @@ -541,9 +507,6 @@ impl LoadShed for ObjectstoreService { Objectstore::Create(_, sender) => { sender.send(Err(error)); } - Objectstore::Finish(_, sender) => { - sender.send(Err(error)); - } } } } @@ -597,13 +560,6 @@ impl ObjectstoreServiceInner { } sender.send(result); } - Objectstore::Finish(finish, sender) => { - let result = self.handle_finish(finish).await; - if let Err(error) = &result { - error.log(MessageKind::Finish); - } - sender.send(result); - } }; } @@ -866,8 +822,6 @@ impl ObjectstoreServiceInner { Ok(UploadRef { key, upload_id: Some(upload_id.clone()), - offset: 0, - length: None, }) } @@ -875,54 +829,25 @@ impl ObjectstoreServiceInner { let Stream { organization_id, project_id, - upload_ref, + key, + mode, stream, } = stream; - if upload_ref.offset % UPLOAD_GRANULARITY != 0 { - return Err(ErrorKind::InvalidOffset { - offset: upload_ref.offset, - granularity: UPLOAD_GRANULARITY.get(), - } - .into()); - } - let session = self.session(&self.event_attachments, organization_id, project_id)?; - let byte_counter = stream.byte_counter(); self.upload( MessageKind::Stream, &session, Upload::Stream { body: TakeOnce::new(stream), - byte_counter, - upload_ref, + key, + mode, }, ) .await } - async fn handle_finish(&self, finish: FinishMultipart) -> Result { - let FinishMultipart { - organization_id, - project_id, - key, - upload_id, - length, - } = finish; - - let session = self.session(&self.event_attachments, organization_id, project_id)?; - let multipart = session.resume_multipart_upload(key.clone(), upload_id.to_string())?; - let parts = multipart.list_parts().await?; - validate_multipart_length(&parts, length)?; - let _final_key = multipart - .complete(parts.into_iter().map(Into::into)) - .await?; - debug_assert_eq!(&key, &_final_key); - - Ok(ObjectstoreKey(key)) - } - async fn upload_bytes( &self, kind: MessageKind, @@ -1008,7 +933,6 @@ impl ObjectstoreServiceInner { retention_hours, content_type, } => { - let len = body.len(); let mut request = session.put(body); if let Some(content_type) = content_type { request = request.content_type(content_type.as_str()); @@ -1031,97 +955,114 @@ impl ObjectstoreServiceInner { Ok(UploadRef { key: response.key, upload_id: None, - offset: len, - length: Some(len), }) } - UploadAttempt::Stream { - body, - byte_counter, - upload_ref, - } => { - let UploadRef { - key, - upload_id, - offset, - length, - } = upload_ref; + UploadAttempt::Stream { body, key, mode } => { let original_key = key.clone(); - - let Some(upload_id) = upload_id else { - // No upload ID: simple upload in a single request. - if offset != 0 { - return Err(AttemptUploadError::OffsetWithoutUploadId); + match mode { + StreamingMode::Oneshot => { + let request = session.put_stream(body.boxed()).key(key); + let response = request.send().await?; + return Ok(UploadRef { + key: response.key, + upload_id: None, + }); } - let request = session.put_stream(body.boxed()).key(key); - let response = request.send().await?; - return Ok(UploadRef { - key: response.key, + StreamingMode::Multipart { upload_id, offset, length, - }); - }; - - let mut multipart_upload = - session.resume_multipart_upload(key, upload_id.to_string())?; - let upload_id = multipart_upload.upload_id().clone(); + } => { + let granularity = UPLOAD_GRANULARITY.get(); + + if (offset % granularity) != 0 { + return Err(AttemptUploadError::InvalidOffset { + offset, + granularity, + }); + } - relay_statsd::metric!( - timer(RelayTimers::AttachmentUploadDuration), - type = kind.as_str(), - { - // Ensure that the stream ends at the granularity boundary. - // NOTE: Once every upload is a multipart upload, we can remove `RetryableStream` - // because streams will never be effectively retried. - let mut body = Rechunk::new(body, UPLOAD_GRANULARITY); - - // Unfortunately, MinIO has the limitation that the length of a multipart request - // has to be known. Therefore, we need to materialize the stream into concrete - // chunks of bytes and send each chunk as an individual request. - // - // Every chunk needs to be > 5 MB because that's what multipart requires. - let mut parts = vec![]; - let mut compressed_buffer = zstd::stream::write::Encoder::new(vec![], 0).map_err(AttemptUploadError::Zstd)?; - let mut part_number = offset / UPLOAD_GRANULARITY + 1; - while let Some(bytes) = body.next().await { - match bytes { - Err(error) => { - self.try_flush(&mut compressed_buffer, part_number, &mut multipart_upload, &mut parts).await?; - return Err(objectstore_client::Error::Io(error).into()); - } Ok(bytes) => { - compressed_buffer.write_all(&bytes).map_err(AttemptUploadError::Zstd)?; - if compressed_buffer.get_ref().len() > MULTIPART_MIN_PART_SIZE { - self.try_flush(&mut compressed_buffer, part_number, &mut multipart_upload, &mut parts).await?; + let mut multipart_upload = + session.resume_multipart_upload(key, upload_id)?; + let mut final_upload_id = Some(multipart_upload.upload_id().clone()); + + relay_statsd::metric!( + timer(RelayTimers::AttachmentUploadDuration), + type = kind.as_str(), + { + // Ensure that the stream ends at the granularity boundary. + // NOTE: Once every upload is a multipart upload, we can remove `RetryableStream` + // because streams will never be effectively retried. + let mut body = Rechunk::new(body, UPLOAD_GRANULARITY); + + // Unfortunately, MinIO has the limitation that the length of a multipart request + // has to be known. Therefore, we need to materialize the stream into concrete + // chunks of bytes and send each chunk as an individual request. + // + // Every chunk needs to be > 5 MB because that's what multipart requires. + let mut parts = vec![]; + let mut compressed_buffer = zstd::stream::write::Encoder::new(vec![], 0).map_err(AttemptUploadError::Zstd)?; + let mut part_number = offset / granularity + 1; + let mut is_final = false; + while let Some(bytes) = body.next().await { + match bytes { + Err(error) => { + self.try_flush(&mut compressed_buffer, part_number, &mut multipart_upload, &mut parts).await?; + return Err(objectstore_client::Error::Io(error).into()); + } Ok(bytes) => { + debug_assert!(bytes.len() <= granularity); + let new_file_size = part_number * granularity + bytes.len(); + + // Only accept full grains, unless the upload is done: + is_final = new_file_size == length; + if bytes.len() == granularity || is_final { + compressed_buffer.write_all(&bytes).map_err(AttemptUploadError::Zstd)?; + if is_final || compressed_buffer.get_ref().len() > MULTIPART_MIN_PART_SIZE { + self.try_flush(&mut compressed_buffer, part_number, &mut multipart_upload, &mut parts).await?; + } + } + + if is_final { + // This should already be guaranteed by the BoundedStream. + #[cfg(debug_assertions)] + debug_assert!(body.next().await.is_none()); + break; + } + } } + part_number += 1; } - } - part_number += 1; - } - self.try_flush(&mut compressed_buffer, part_number, &mut multipart_upload, &mut parts).await?; + if is_final { + relay_log::trace!("Completing multipart upload"); + let parts = multipart_upload.list_parts().await?; + + // Verify that the last written part number is the highest part number in the list. + // Otherwise clients could fake a small `Upload-Length` for a large upload. + let max_part_number = parts.iter().map(|p|p.part_number.get()).max().unwrap_or(0) as usize; + if max_part_number != part_number { + return Err(AttemptUploadError::InvalidUploadLength { + length, server_offset: max_part_number * granularity + }) + } - let new_offset = offset + byte_counter.get(); - if length.is_some_and(|length| length == new_offset) { - relay_log::trace!("Completing multipart upload"); - let parts = multipart_upload.list_parts().await?; + let parts = parts.into_iter().map(|item| { + let PartInfo{ part_number, etag, .. } = item; + CompletePart { part_number, etag } + }); - let parts = parts.into_iter().map(|item| { - let PartInfo{ part_number, etag, .. } = item; - CompletePart { part_number, etag } - }); + let final_key = multipart_upload.complete(parts).await?; + final_upload_id = None; + debug_assert_eq!(&original_key, &final_key); + } - let final_key = multipart_upload.complete(parts).await?; - debug_assert_eq!(&original_key, &final_key); + Ok(UploadRef { + key: original_key, + upload_id: final_upload_id + }) + }) } - - Ok(UploadRef { - key: original_key, - upload_id: Some(upload_id), - offset: offset + byte_counter.get(), - length, - }) - }) + } } } } @@ -1186,26 +1127,16 @@ impl ObjectstoreServiceInner { } } -fn validate_multipart_length(parts: &[PartInfo], expected: usize) -> Result<(), ErrorKind> { - let actual = parts - .iter() - .fold(0u64, |sum, part| sum.saturating_add(part.size)); - - if actual != expected as u64 { - return Err(ErrorKind::InvalidLength { expected, actual }); - } - - Ok(()) -} - #[derive(Debug, thiserror::Error)] enum AttemptUploadError { #[error(transparent)] Objectstore(#[from] objectstore_client::Error), #[error("zstd error: {0}")] Zstd(#[source] std::io::Error), - #[error("offset without upload ID")] - OffsetWithoutUploadId, + #[error("invalid Upload-Offset {offset}: must be a multiple of {granularity}")] + InvalidOffset { offset: usize, granularity: usize }, + #[error("invalid Upload-Length {length}: server has {server_offset} bytes")] + InvalidUploadLength { length: usize, server_offset: usize }, } impl From for ErrorKind { @@ -1213,7 +1144,20 @@ impl From for ErrorKind { match value { AttemptUploadError::Objectstore(error) => ErrorKind::UploadFailed(error), AttemptUploadError::Zstd(error) => ErrorKind::CompressionFailed(error), - AttemptUploadError::OffsetWithoutUploadId => ErrorKind::OffsetWithoutUploadId, + AttemptUploadError::InvalidOffset { + offset, + granularity, + } => ErrorKind::InvalidOffset { + offset, + granularity, + }, + AttemptUploadError::InvalidUploadLength { + length, + server_offset, + } => ErrorKind::InvalidUploadLength { + length, + server_offset, + }, } } } @@ -1230,8 +1174,8 @@ enum Upload { }, Stream { body: TakeOnce>>, - byte_counter: ByteCounter, - upload_ref: UploadRef, + key: String, + mode: StreamingMode, }, } @@ -1249,15 +1193,13 @@ impl Upload { retention_hours: *retention_hours, content_type: *content_type, }), - Self::Stream { - body, - byte_counter, - upload_ref, - } => RetryableStream::new(body.clone()).map(|body| UploadAttempt::Stream { - body, - byte_counter: byte_counter.clone(), - upload_ref: upload_ref.clone(), - }), + Self::Stream { body, key, mode } => { + RetryableStream::new(body.clone()).map(|body| UploadAttempt::Stream { + body, + key: key.clone(), + mode: mode.clone(), + }) + } } } } @@ -1274,8 +1216,8 @@ enum UploadAttempt { }, Stream { body: RetryableStream>>, - byte_counter: ByteCounter, - upload_ref: UploadRef, + key: String, + mode: StreamingMode, }, } @@ -1330,8 +1272,6 @@ fn drop_attachments(event: &mut Managed>) { #[cfg(test)] mod tests { - use std::num::NonZeroU32; - use std::time::SystemTime; use relay_event_schema::protocol::EventId; use relay_quotas::DataCategory; @@ -1341,33 +1281,6 @@ mod tests { use super::*; - #[test] - fn validates_multipart_length() { - let parts = [ - PartInfo { - part_number: NonZeroU32::new(1).unwrap(), - etag: "first".to_owned(), - last_modified: SystemTime::UNIX_EPOCH, - size: 3, - }, - PartInfo { - part_number: NonZeroU32::new(2).unwrap(), - etag: "second".to_owned(), - last_modified: SystemTime::UNIX_EPOCH, - size: 5, - }, - ]; - - assert!(validate_multipart_length(&parts, 8).is_ok()); - assert!(matches!( - validate_multipart_length(&parts, 9), - Err(ErrorKind::InvalidLength { - expected: 9, - actual: 8 - }) - )); - } - #[tokio::test] async fn org_zero_rejected() { let service = ObjectstoreService::new(&test_config(), Some(Addr::dummy())) @@ -1385,13 +1298,9 @@ mod tests { .send(Stream { organization_id: OrganizationId::new(0), project_id: ProjectId::new(1), - upload_ref: UploadRef { - key: "my_file".to_owned(), - upload_id: Some(UploadId::new("my_upload".to_owned()).unwrap()), - offset: 0, - length: None, - }, stream, + key: "my_key".to_owned(), + mode: StreamingMode::Oneshot, }) .await .unwrap(); diff --git a/relay-server/src/services/upload.rs b/relay-server/src/services/upload.rs index 24b46e74f80..5e5604aebec 100644 --- a/relay-server/src/services/upload.rs +++ b/relay-server/src/services/upload.rs @@ -113,8 +113,6 @@ pub enum Upload { /// The service also returns the signed location. This is redundant, but creates a simpler /// flow for the caller side. Stream(Box, InstrumentedSender), - /// Finishes an upload and declares its final length. - Finish(Finish, InstrumentedSender), } impl Interface for Upload {} @@ -159,18 +157,6 @@ pub struct Stream { pub stream: BoundedStream>, } -/// A request to finish an upload and declare its final length. -pub struct Finish { - /// Time of arrival of the request. - pub received: DateTime, - /// The project the upload belongs to. - pub project: ProjectContext, - /// The location to finish. - pub location: SignedLocation, - /// The final length of the upload in bytes. - pub length: usize, -} - impl FromMessage for Upload { type Response = AsyncResponse, Error>>; @@ -205,20 +191,6 @@ impl FromMessage for Upload { } } -impl FromMessage for Upload { - type Response = AsyncResponse, Error>>; - - fn from_message(message: Finish, sender: Sender, Error>>) -> Self { - Self::Finish( - message, - InstrumentedSender { - metric: RelayCounters::UploadFinish, - inner: sender, - }, - ) - } -} - /// Creates a new upload service. pub fn create_service( config: &Arc, @@ -333,12 +305,7 @@ impl Service { // and if the upload actually has data (multipart does not allow empty parts). (false, _) | (_, Some(0)) => (key, None), _ => { - let UploadRef { - key, - upload_id, - offset, - length: _, - } = addr + let UploadRef { key, upload_id } = addr .send(objectstore::CreateMultipart { organization_id, project_id, @@ -346,7 +313,6 @@ impl Service { }) .await .map_err(Error::ObjectstoreServiceUnavailable)??; - debug_assert_eq!(offset, 0); #[cfg(debug_assertions)] debug_assert_eq!(&key, &original_key); (key, upload_id) @@ -384,7 +350,7 @@ impl Service { } #[cfg(feature = "processing")] Backend::Objectstore { addr, config } => { - use crate::services::objectstore::UploadRef; + use crate::services::objectstore::{StreamingMode, UploadRef}; let Location { project_id, @@ -396,29 +362,37 @@ impl Service { let scoping = project.scoping; debug_assert_eq!(scoping.project_id, project_id); - debug_assert!(stream.length().is_none_or(|l| Some(l) == length.value())); - let upload_ref = UploadRef::new(key, upload_id, offset, length.value())?; + + let mode = match (length.value(), upload_id) { + (Some(length), Some(upload_id)) => StreamingMode::Multipart { + upload_id, + offset, + length, + }, + (_, None) => StreamingMode::Oneshot, + (None, Some(_)) => { + // NOTE: this restriction could be encoded into the Location type. + return Err(Error::InvalidLocation(None)); + } + }; let upload_ref = addr .send(objectstore::Stream { organization_id: scoping.organization_id, project_id, - upload_ref, + stream, + key, + mode, }) .await .map_err(Error::ObjectstoreServiceUnavailable)??; - let UploadRef { - key, - upload_id, - offset: _, - length, - } = upload_ref; + let UploadRef { key, upload_id } = upload_ref; Location { project_id, key, - length: Provisional(length), + length, upload_id: upload_id.map(|id| id.to_string()), other, } @@ -427,64 +401,6 @@ impl Service { } } - async fn finish(&self, finish: Finish) -> Result, Error> { - relay_log::trace!("Finishing request"); - let Finish { - #[cfg_attr(not(feature = "processing"), expect(unused))] - received, - project, - location, - length, - } = finish; - - match &self.backend { - Backend::Upstream { addr } => { - relay_log::trace!("Assembling request"); - let (request, rx) = UploadRequest::finish(project, location.try_to_uri()?, length); - addr.send(SendRequest(request)); - relay_log::trace!("Awaiting response"); - let response = rx.await??; - relay_log::trace!("Returning signed location"); - SignedLocation::try_from_response(response) - } - #[cfg(feature = "processing")] - Backend::Objectstore { addr, config } => { - let Location { - project_id, - mut key, - length: _, - upload_id, - other, - } = location.verify(received, config)?; - - let scoping = project.scoping; - debug_assert_eq!(scoping.project_id, project_id); - - if let Some(upload_id) = upload_id { - key = addr - .send(objectstore::FinishMultipart { - organization_id: scoping.organization_id, - project_id, - key, - upload_id, - length, - }) - .await - .map_err(Error::ObjectstoreServiceUnavailable)?? - .into_inner(); - } - Location { - project_id, - key, - length: Final(length), - upload_id: None, - other, - } - .try_sign(config) - } - } - } - async fn timeout(&self, future: F) -> Result, Error> where L: UploadLength, @@ -505,9 +421,6 @@ impl SimpleService for Service { Upload::Stream(stream, sender) => { sender.send(self.timeout(self.upload(*stream)).await); } - Upload::Finish(finish, sender) => { - sender.send(self.timeout(self.finish(finish)).await); - } } } } @@ -517,7 +430,6 @@ impl LoadShed for Service { match message { Upload::Create(_, tx) => tx.send(Err(Error::LoadShed)), Upload::Stream(_, tx) => tx.send(Err(Error::LoadShed)), - Upload::Finish(_, tx) => tx.send(Err(Error::LoadShed)), } } } @@ -827,10 +739,6 @@ enum RequestKind { stream: TakeOnce>>, encoding: HttpEncoding, }, - Finish { - uri: String, - length: usize, - }, } /// An upstream request made to the `/upload` endpoint. @@ -888,25 +796,6 @@ impl UploadRequest { rx, ) } - - fn finish( - project: ProjectContext, - uri: String, - length: usize, - ) -> ( - Self, - oneshot::Receiver>, - ) { - let (sender, rx) = oneshot::channel(); - ( - Self { - project, - kind: RequestKind::Finish { uri, length }, - sender, - }, - rx, - ) - } } impl fmt::Debug for UploadRequest { @@ -930,7 +819,7 @@ impl UpstreamRequest for UploadRequest { fn method(&self) -> Method { match self.kind { RequestKind::Create { .. } => Method::POST, - RequestKind::Upload { .. } | RequestKind::Finish { .. } => Method::PATCH, + RequestKind::Upload { .. } => Method::PATCH, } } @@ -938,7 +827,7 @@ impl UpstreamRequest for UploadRequest { let project_id = self.project.scoping.project_id; match &self.kind { RequestKind::Create { .. } => Cow::Owned(format!("/api/{project_id}/upload/")), - RequestKind::Upload { uri, .. } | RequestKind::Finish { uri, .. } => Cow::Borrowed(uri), + RequestKind::Upload { uri, .. } => Cow::Borrowed(uri), } } @@ -958,7 +847,6 @@ impl UpstreamRequest for UploadRequest { fn retry(&self) -> bool { match &self.kind { RequestKind::Create { .. } => true, - RequestKind::Finish { .. } => true, // Once the body has been polled, it cannot be replayed — give up instead. RequestKind::Upload { stream, .. } => !stream.is_taken(), } @@ -997,12 +885,6 @@ impl UpstreamRequest for UploadRequest { builder.body(reqwest::Body::wrap_stream(body)); } - RequestKind::Finish { uri: _, length } => { - tus::add_upload_headers(builder, *length); - builder.header(tus::UPLOAD_LENGTH, HeaderValue::from(*length)); - builder.header(http::header::CONTENT_LENGTH.as_str(), "0"); - builder.body(Bytes::new()); - } }; let project_key = self.project.scoping.project_key; @@ -1037,42 +919,6 @@ where mod tests { use super::*; - #[test] - fn finish_request_is_empty_and_declares_length() { - let project = ProjectContext { - scoping: Scoping { - organization_id: relay_base_schema::organization::OrganizationId::new(1), - project_id: ProjectId::new(42), - project_key: "a94ae32be2584e0bbd7a4cbb95971fee".parse().unwrap(), - key_id: None, - }, - upstream: None, - }; - let (mut upload, _receiver) = UploadRequest::finish( - project, - "/api/42/upload/key/?upload_signature=signature".to_owned(), - 123, - ); - let client = reqwest::Client::new(); - let mut builder = RequestBuilder::reqwest(client.request( - upload.method(), - format!("http://localhost{}", upload.path()), - )); - - upload.build(&mut builder).unwrap(); - let request = builder.finish().unwrap().0; - - assert_eq!(request.method(), Method::PATCH); - assert_eq!(request.headers()[tus::TUS_RESUMABLE], tus::TUS_VERSION); - assert_eq!(request.headers()[tus::UPLOAD_OFFSET], "123"); - assert_eq!(request.headers()[tus::UPLOAD_LENGTH], "123"); - assert_eq!(request.headers()[http::header::CONTENT_LENGTH], "0"); - assert_eq!( - request.body().and_then(reqwest::Body::as_bytes), - Some(&[][..]) - ); - } - #[cfg(feature = "processing")] mod with_processing { use chrono::Utc; diff --git a/relay-server/src/statsd.rs b/relay-server/src/statsd.rs index c8402d24074..1e7273ed2a5 100644 --- a/relay-server/src/statsd.rs +++ b/relay-server/src/statsd.rs @@ -986,11 +986,6 @@ pub enum RelayCounters { /// This metric is tagged with: /// - `result`: `success` or the failure reason. UploadUpload, - /// The number of times an upload is finished through the upload service. - /// - /// This metric is tagged with: - /// - `result`: `success` or the failure reason. - UploadFinish, /// The number of times an objectstore upload of an attachment occurs. /// /// This metric is tagged with: @@ -1081,7 +1076,6 @@ impl CounterMetric for RelayCounters { RelayCounters::UploadKillswitched => "upload.killswitched", RelayCounters::UploadCreate => "upload.create", RelayCounters::UploadUpload => "upload.upload", - RelayCounters::UploadFinish => "upload.finish", #[cfg(feature = "processing")] RelayCounters::AttachmentUpload => "attachment.upload", RelayCounters::EnvelopeWithLogs => "logs.envelope", diff --git a/relay-server/src/utils/tus.rs b/relay-server/src/utils/tus.rs index ccb05c52439..97631e3668b 100644 --- a/relay-server/src/utils/tus.rs +++ b/relay-server/src/utils/tus.rs @@ -146,7 +146,7 @@ pub fn validate_post_headers(headers: &HeaderMap) -> Result { /// Validates TUS protocol headers and returns the expected upload length. /// /// Returns the offset from which the upload is resumed. -pub fn validate_patch_headers(headers: &HeaderMap) -> Result<(usize, Option), Error> { +pub fn validate_patch_headers(headers: &HeaderMap) -> Result { let tus_version = headers.get(TUS_RESUMABLE); if tus_version != Some(&TUS_VERSION) { return Err(Error::Version( @@ -166,9 +166,8 @@ pub fn validate_patch_headers(headers: &HeaderMap) -> Result<(usize, Option = parse_header(headers, UPLOAD_LENGTH); - Ok((upload_offset, upload_length)) + Ok(upload_offset) } /// Prepares the required TUS request headers for upstream requests. From 71a6eb24afce51d0fce0e60551283e6591ec6e81 Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Sat, 18 Jul 2026 13:49:52 +0200 Subject: [PATCH 31/41] no multipart for Defer-Length-1 --- relay-server/src/endpoints/upload.rs | 10 ++++++---- relay-server/src/services/upload.rs | 11 +++++++++-- 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/relay-server/src/endpoints/upload.rs b/relay-server/src/endpoints/upload.rs index 42a0c5ea581..344b8631769 100644 --- a/relay-server/src/endpoints/upload.rs +++ b/relay-server/src/endpoints/upload.rs @@ -195,10 +195,12 @@ async fn handle_post( StatusCode::SERVICE_UNAVAILABLE })?; - let multipart = match project.state() { - ProjectState::Enabled(p) => p.has_feature(Feature::UploadMultipart), - _ => false, - }; + // We don't enable multipart for `Upload-Defer-Length: 1`, or if the feature flag is disabled. + let multipart = headers.upload_length.is_none() + || match project.state() { + ProjectState::Enabled(p) => p.has_feature(Feature::UploadMultipart), + _ => false, + }; relay_log::trace!("Checking request"); let project_context = validate_and_limit(&state, meta, &headers, project).await?; diff --git a/relay-server/src/services/upload.rs b/relay-server/src/services/upload.rs index 5e5604aebec..348446356ad 100644 --- a/relay-server/src/services/upload.rs +++ b/relay-server/src/services/upload.rs @@ -375,6 +375,7 @@ impl Service { return Err(Error::InvalidLocation(None)); } }; + let byte_counter = stream.byte_counter(); let upload_ref = addr .send(objectstore::Stream { organization_id: scoping.organization_id, @@ -382,17 +383,23 @@ impl Service { stream, key, - mode, + mode: mode.clone(), }) .await .map_err(Error::ObjectstoreServiceUnavailable)??; let UploadRef { key, upload_id } = upload_ref; + // If it was a oneshot request, update the length: + let final_length = match mode { + StreamingMode::Oneshot => offset + byte_counter.get(), + StreamingMode::Multipart { length, .. } => length, + }; + Location { project_id, key, - length, + length: Provisional(Some(final_length)), // FIXME: could be Final upload_id: upload_id.map(|id| id.to_string()), other, } From f4d10897c02893f0591a5fe2669010b5c79b0168 Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Sat, 18 Jul 2026 14:05:05 +0200 Subject: [PATCH 32/41] reject offset > 0 --- relay-server/src/endpoints/upload.rs | 5 +++-- relay-server/src/services/upload.rs | 10 +++++++++- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/relay-server/src/endpoints/upload.rs b/relay-server/src/endpoints/upload.rs index 344b8631769..8c4992671d1 100644 --- a/relay-server/src/endpoints/upload.rs +++ b/relay-server/src/endpoints/upload.rs @@ -109,6 +109,7 @@ impl IntoResponse for Error { } #[cfg(feature = "processing")] upload::Error::InvalidUploadId(_) => StatusCode::BAD_REQUEST, + upload::Error::OffsetWithoutLength => StatusCode::BAD_REQUEST, upload::Error::SerializeFailed(_) => StatusCode::INTERNAL_SERVER_ERROR, upload::Error::InvalidSignature(_) => StatusCode::BAD_REQUEST, upload::Error::ObjectstoreServiceUnavailable(_) => StatusCode::SERVICE_UNAVAILABLE, @@ -196,8 +197,8 @@ async fn handle_post( })?; // We don't enable multipart for `Upload-Defer-Length: 1`, or if the feature flag is disabled. - let multipart = headers.upload_length.is_none() - || match project.state() { + let multipart = headers.upload_length.is_some() + && match project.state() { ProjectState::Enabled(p) => p.has_feature(Feature::UploadMultipart), _ => false, }; diff --git a/relay-server/src/services/upload.rs b/relay-server/src/services/upload.rs index 348446356ad..19c000fdefb 100644 --- a/relay-server/src/services/upload.rs +++ b/relay-server/src/services/upload.rs @@ -63,6 +63,8 @@ pub enum Error { #[cfg(feature = "processing")] #[error(transparent)] InvalidUploadId(#[from] objectstore_types::multipart::InvalidUploadId), + #[error("Upload-Offset is not currently supported with Defer-Length: 1")] + OffsetWithoutLength, #[error("serializing location failed: {0}")] SerializeFailed(#[from] serde_urlencoded::ser::Error), #[error("failed to sign location")] @@ -90,6 +92,7 @@ impl Error { Error::InvalidLocation(_) => "invalid_location", #[cfg(feature = "processing")] Error::InvalidUploadId(_) => "invalid_upload_id", + Error::OffsetWithoutLength => "offset_with_defer_length", Error::SigningFailed => "signing_failed", Error::SerializeFailed(_) => "serialize_failed", Error::InvalidSignature(_) => "invalid_signature", @@ -369,7 +372,12 @@ impl Service { offset, length, }, - (_, None) => StreamingMode::Oneshot, + (_, None) => { + if offset != 0 { + return Err(Error::OffsetWithoutLength); + } + StreamingMode::Oneshot + } (None, Some(_)) => { // NOTE: this restriction could be encoded into the Location type. return Err(Error::InvalidLocation(None)); From 9c60de66c21f0e54fac6adbc2290e4a46a336479 Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Sat, 18 Jul 2026 17:26:39 +0200 Subject: [PATCH 33/41] fix --- relay-server/src/services/objectstore.rs | 96 +++++++++++++----------- relay-server/src/services/upload.rs | 27 +++---- 2 files changed, 67 insertions(+), 56 deletions(-) diff --git a/relay-server/src/services/objectstore.rs b/relay-server/src/services/objectstore.rs index 6fcf4ba22c3..5f000fbdc5f 100644 --- a/relay-server/src/services/objectstore.rs +++ b/relay-server/src/services/objectstore.rs @@ -14,7 +14,6 @@ use objectstore_client::{ SecretKey as SigningKey, Session, TokenGenerator, UploadId, Usecase, }; -use objectstore_types::multipart::InvalidUploadId; use relay_base_schema::organization::OrganizationId; use relay_base_schema::project::ProjectId; use relay_config::ObjectstoreServiceConfig; @@ -35,7 +34,8 @@ use crate::services::store::{ use crate::services::upload::ByteStream; use crate::statsd::{RelayCounters, RelayTimers}; use crate::utils::{ - BoundedStream, MeteredStream, Rechunk, RetryableStream, TakeOnce, find_error_source, + BoundedStream, ByteCounter, MeteredStream, Rechunk, RetryableStream, TakeOnce, + find_error_source, }; use super::outcome::Outcome; @@ -162,7 +162,7 @@ impl FromMessage for Objectstore { #[derive(Clone)] pub enum StreamingMode { - Oneshot, + Oneshot(ByteCounter), Multipart { upload_id: String, offset: usize, @@ -369,18 +369,20 @@ pub struct UploadRef { /// The ID of the multipart upload session (chosen by objectstore). /// `None` if the upload is not multipart. pub upload_id: Option, + /// The byte offset from which to resume the upload, + pub offset: usize, } -impl UploadRef { - /// Validates the upload ID and returns a new upload reference. - pub fn new(key: String, upload_id: Option) -> Result { - let upload_id = match upload_id { - Some(s) => Some(UploadId::new(s)?), - None => None, - }; - Ok(Self { key, upload_id }) - } -} +// impl UploadRef { +// /// Validates the upload ID and returns a new upload reference. +// // pub fn new(key: String, upload_id: Option) -> Result { +// // let upload_id = match upload_id { +// // Some(s) => Some(UploadId::new(s)?), +// // None => None, +// // }; +// // Ok(Self { key, upload_id }) +// // } +// } impl fmt::Display for ObjectstoreKey { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { @@ -822,6 +824,7 @@ impl ObjectstoreServiceInner { Ok(UploadRef { key, upload_id: Some(upload_id.clone()), + offset: 0, }) } @@ -933,6 +936,7 @@ impl ObjectstoreServiceInner { retention_hours, content_type, } => { + let offset = body.len(); let mut request = session.put(body); if let Some(content_type) = content_type { request = request.content_type(content_type.as_str()); @@ -955,17 +959,19 @@ impl ObjectstoreServiceInner { Ok(UploadRef { key: response.key, upload_id: None, + offset, }) } UploadAttempt::Stream { body, key, mode } => { let original_key = key.clone(); match mode { - StreamingMode::Oneshot => { + StreamingMode::Oneshot(byte_counter) => { let request = session.put_stream(body.boxed()).key(key); let response = request.send().await?; return Ok(UploadRef { key: response.key, upload_id: None, + offset: byte_counter.get(), }); } StreamingMode::Multipart { @@ -1002,35 +1008,37 @@ impl ObjectstoreServiceInner { // Every chunk needs to be > 5 MB because that's what multipart requires. let mut parts = vec![]; let mut compressed_buffer = zstd::stream::write::Encoder::new(vec![], 0).map_err(AttemptUploadError::Zstd)?; - let mut part_number = offset / granularity + 1; + let mut buffered_offset = offset; + let mut flushed_offset = offset; let mut is_final = false; while let Some(bytes) = body.next().await { - match bytes { - Err(error) => { - self.try_flush(&mut compressed_buffer, part_number, &mut multipart_upload, &mut parts).await?; - return Err(objectstore_client::Error::Io(error).into()); - } Ok(bytes) => { - debug_assert!(bytes.len() <= granularity); - let new_file_size = part_number * granularity + bytes.len(); - - // Only accept full grains, unless the upload is done: - is_final = new_file_size == length; - if bytes.len() == granularity || is_final { - compressed_buffer.write_all(&bytes).map_err(AttemptUploadError::Zstd)?; - if is_final || compressed_buffer.get_ref().len() > MULTIPART_MIN_PART_SIZE { - self.try_flush(&mut compressed_buffer, part_number, &mut multipart_upload, &mut parts).await?; - } - } - - if is_final { - // This should already be guaranteed by the BoundedStream. - #[cfg(debug_assertions)] - debug_assert!(body.next().await.is_none()); - break; - } + let bytes = bytes.map_err(objectstore_client::Error::Io)?; + + debug_assert!(bytes.len() <= granularity); + buffered_offset += bytes.len(); + + // Only accept full grains, unless the upload is done: + is_final = buffered_offset == length; + if is_final || bytes.len() == granularity { + compressed_buffer.write_all(&bytes).map_err(AttemptUploadError::Zstd)?; + if is_final || compressed_buffer.get_ref().len() > MULTIPART_MIN_PART_SIZE { + self.try_flush(&mut compressed_buffer, buffered_offset, &mut multipart_upload, &mut parts).await?; + flushed_offset = buffered_offset; } } - part_number += 1; + + if is_final { + // This should already be guaranteed by the BoundedStream. + #[cfg(debug_assertions)] + debug_assert!(body.next().await.is_none()); + break; + } + } + + if !compressed_buffer.get_ref().is_empty() { + // This can happen when the body compresses so well that it never + // passes the minimum mark. + relay_log::warn!("Stream ended before reaching minimum part size"); } if is_final { @@ -1040,7 +1048,7 @@ impl ObjectstoreServiceInner { // Verify that the last written part number is the highest part number in the list. // Otherwise clients could fake a small `Upload-Length` for a large upload. let max_part_number = parts.iter().map(|p|p.part_number.get()).max().unwrap_or(0) as usize; - if max_part_number != part_number { + if max_part_number * granularity != flushed_offset { return Err(AttemptUploadError::InvalidUploadLength { length, server_offset: max_part_number * granularity }) @@ -1058,7 +1066,8 @@ impl ObjectstoreServiceInner { Ok(UploadRef { key: original_key, - upload_id: final_upload_id + upload_id: final_upload_id, + offset: flushed_offset, }) }) } @@ -1070,7 +1079,7 @@ impl ObjectstoreServiceInner { async fn try_flush( &self, buffer: &mut zstd::stream::write::Encoder<'_, Vec>, - part_number: usize, + byte_offset: usize, multipart: &mut MultipartUpload, parts: &mut Vec, ) -> Result<(), AttemptUploadError> { @@ -1085,6 +1094,7 @@ impl ObjectstoreServiceInner { .map_err(AttemptUploadError::Zstd)? .into(); + let part_number = byte_offset.div_ceil(UPLOAD_GRANULARITY.get()); let part_number = u32::try_from(part_number) .map_err(|_| objectstore_client::Error::InvalidPartNumber(u32::MAX))?; @@ -1300,7 +1310,7 @@ mod tests { project_id: ProjectId::new(1), stream, key: "my_key".to_owned(), - mode: StreamingMode::Oneshot, + mode: StreamingMode::Oneshot(stream.byte_counter()), }) .await .unwrap(); diff --git a/relay-server/src/services/upload.rs b/relay-server/src/services/upload.rs index 19c000fdefb..ed35e565549 100644 --- a/relay-server/src/services/upload.rs +++ b/relay-server/src/services/upload.rs @@ -63,7 +63,7 @@ pub enum Error { #[cfg(feature = "processing")] #[error(transparent)] InvalidUploadId(#[from] objectstore_types::multipart::InvalidUploadId), - #[error("Upload-Offset is not currently supported with Defer-Length: 1")] + #[error("Upload-Offset > 0 is currently unsupported with Defer-Length: 1")] OffsetWithoutLength, #[error("serializing location failed: {0}")] SerializeFailed(#[from] serde_urlencoded::ser::Error), @@ -308,7 +308,11 @@ impl Service { // and if the upload actually has data (multipart does not allow empty parts). (false, _) | (_, Some(0)) => (key, None), _ => { - let UploadRef { key, upload_id } = addr + let UploadRef { + key, + upload_id, + offset: _, + } = addr .send(objectstore::CreateMultipart { organization_id, project_id, @@ -366,7 +370,7 @@ impl Service { let scoping = project.scoping; debug_assert_eq!(scoping.project_id, project_id); - let mode = match (length.value(), upload_id) { + let mode = match dbg!((length.value(), upload_id)) { (Some(length), Some(upload_id)) => StreamingMode::Multipart { upload_id, offset, @@ -376,14 +380,13 @@ impl Service { if offset != 0 { return Err(Error::OffsetWithoutLength); } - StreamingMode::Oneshot + StreamingMode::Oneshot(stream.byte_counter()) } (None, Some(_)) => { // NOTE: this restriction could be encoded into the Location type. return Err(Error::InvalidLocation(None)); } }; - let byte_counter = stream.byte_counter(); let upload_ref = addr .send(objectstore::Stream { organization_id: scoping.organization_id, @@ -396,18 +399,16 @@ impl Service { .await .map_err(Error::ObjectstoreServiceUnavailable)??; - let UploadRef { key, upload_id } = upload_ref; - - // If it was a oneshot request, update the length: - let final_length = match mode { - StreamingMode::Oneshot => offset + byte_counter.get(), - StreamingMode::Multipart { length, .. } => length, - }; + let UploadRef { + key, + upload_id, + offset, + } = upload_ref; Location { project_id, key, - length: Provisional(Some(final_length)), // FIXME: could be Final + length: Provisional(Some(offset)), // FIXME: could be Final upload_id: upload_id.map(|id| id.to_string()), other, } From ace745acd355755d741969a80f8f7437767be734 Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Sat, 18 Jul 2026 18:17:50 +0200 Subject: [PATCH 34/41] more fixes --- relay-server/src/endpoints/common.rs | 7 +- relay-server/src/endpoints/upload.rs | 15 ++-- relay-server/src/services/objectstore.rs | 20 +++--- relay-server/src/services/upload.rs | 90 ++++++++++++++++-------- tests/integration/test_upload.py | 21 +++--- 5 files changed, 95 insertions(+), 58 deletions(-) diff --git a/relay-server/src/endpoints/common.rs b/relay-server/src/endpoints/common.rs index c01a07d8c52..d555d3ed779 100644 --- a/relay-server/src/endpoints/common.rs +++ b/relay-server/src/endpoints/common.rs @@ -24,7 +24,7 @@ use crate::service::ServiceState; use crate::services::buffer::{ProjectKeyPair, PushError}; use crate::services::outcome::{DiscardItemType, DiscardReason, Outcome}; use crate::services::processor::{BucketSource, MetricData, ProcessMetrics}; -use crate::services::upload::{Create, ProjectContext, Stream, Upload}; +use crate::services::upload::{Create, ProjectContext, Stream, StreamResult, Upload}; use crate::statsd::{RelayCounters, RelayDistributions}; use crate::utils::{ self, ApiErrorResponse, BoundedStream, FormDataIter, MeteredStream, find_error_source, @@ -619,7 +619,10 @@ where .await .ok()?; - let location = result + let StreamResult { + location, + offset: _, + } = result .inspect_err(|e| { relay_log::warn!( error = e as &dyn std::error::Error, diff --git a/relay-server/src/endpoints/upload.rs b/relay-server/src/endpoints/upload.rs index 8c4992671d1..95e262f93a0 100644 --- a/relay-server/src/endpoints/upload.rs +++ b/relay-server/src/endpoints/upload.rs @@ -32,7 +32,7 @@ use crate::services::projects::cache::Project; use crate::services::projects::project::ProjectState; use crate::services::upload::{ self, ByteStream, LocationQueryParams, ProjectContext, Provisional, SignedLocation, - UploadLength, + StreamResult, UploadLength, }; use crate::services::upstream::UpstreamRequestError; use crate::statsd::RelayCounters; @@ -282,16 +282,13 @@ async fn handle_patch( } }; let stream = BoundedStream::new(stream, lower_bound, upper_bound); - let byte_counter = stream.byte_counter(); relay_log::trace!("Uploading"); let result = upload(&state, project_context, location, offset, stream).await; - let location = result.inspect_err(|e| { + let StreamResult { location, offset } = result.inspect_err(|e| { relay_log::warn!(error = e as &dyn std::error::Error, "upload failed"); })?; - let new_offset = offset + byte_counter.get(); - let mut response = NoContent.into_response(); // Not required by TUS, but we respond with the location header: @@ -304,7 +301,7 @@ async fn handle_patch( .insert(tus::TUS_RESUMABLE, tus::TUS_VERSION); response .headers_mut() - .insert(tus::UPLOAD_OFFSET, new_offset.into()); + .insert(tus::UPLOAD_OFFSET, offset.into()); Ok(response) } @@ -356,8 +353,8 @@ async fn upload( location: SignedLocation, offset: usize, stream: BoundedStream>, -) -> Result, Error> { - let location = state +) -> Result { + let res = state .upload() .send(upload::Stream { received: Utc::now(), @@ -368,7 +365,7 @@ async fn upload( }) .await??; - Ok(location) + Ok(res) } /// Check request by converting it into a pseudo-envelope. diff --git a/relay-server/src/services/objectstore.rs b/relay-server/src/services/objectstore.rs index 5f000fbdc5f..10a96d8933c 100644 --- a/relay-server/src/services/objectstore.rs +++ b/relay-server/src/services/objectstore.rs @@ -161,7 +161,7 @@ impl FromMessage for Objectstore { } #[derive(Clone)] -pub enum StreamingMode { +pub enum StreamMode { Oneshot(ByteCounter), Multipart { upload_id: String, @@ -175,7 +175,7 @@ pub struct Stream { pub organization_id: OrganizationId, pub project_id: ProjectId, pub key: String, - pub mode: StreamingMode, + pub mode: StreamMode, pub stream: BoundedStream>, } @@ -965,7 +965,7 @@ impl ObjectstoreServiceInner { UploadAttempt::Stream { body, key, mode } => { let original_key = key.clone(); match mode { - StreamingMode::Oneshot(byte_counter) => { + StreamMode::Oneshot(byte_counter) => { let request = session.put_stream(body.boxed()).key(key); let response = request.send().await?; return Ok(UploadRef { @@ -974,7 +974,7 @@ impl ObjectstoreServiceInner { offset: byte_counter.get(), }); } - StreamingMode::Multipart { + StreamMode::Multipart { upload_id, offset, length, @@ -1035,10 +1035,11 @@ impl ObjectstoreServiceInner { } } - if !compressed_buffer.get_ref().is_empty() { + let remaining_compressed_bytes = compressed_buffer.get_ref().len(); + if remaining_compressed_bytes > 0 { // This can happen when the body compresses so well that it never // passes the minimum mark. - relay_log::warn!("Stream ended before reaching minimum part size"); + relay_log::warn!("Stream ended before reaching minimum part size ({remaining_compressed_bytes} < {MULTIPART_MIN_PART_SIZE})"); } if is_final { @@ -1185,7 +1186,7 @@ enum Upload { Stream { body: TakeOnce>>, key: String, - mode: StreamingMode, + mode: StreamMode, }, } @@ -1227,7 +1228,7 @@ enum UploadAttempt { Stream { body: RetryableStream>>, key: String, - mode: StreamingMode, + mode: StreamMode, }, } @@ -1304,13 +1305,14 @@ mod tests { usize::MAX, ); + let byte_counter = stream.byte_counter(); let result = addr .send(Stream { organization_id: OrganizationId::new(0), project_id: ProjectId::new(1), stream, key: "my_key".to_owned(), - mode: StreamingMode::Oneshot(stream.byte_counter()), + mode: StreamMode::Oneshot(byte_counter), }) .await .unwrap(); diff --git a/relay-server/src/services/upload.rs b/relay-server/src/services/upload.rs index ed35e565549..3d6d7c99080 100644 --- a/relay-server/src/services/upload.rs +++ b/relay-server/src/services/upload.rs @@ -110,12 +110,12 @@ pub enum Upload { /// Creates an upload resource. /// /// Returns the trusted identifier of the upload. - Create(Create, InstrumentedSender), + Create(Create, InstrumentedSender>), /// Upload a stream of bytes for a given location. /// - /// The service also returns the signed location. This is redundant, but creates a simpler - /// flow for the caller side. - Stream(Box, InstrumentedSender), + /// The service also returns the signed location and the current Upload-Offset stored on the + /// server. + Stream(Box, InstrumentedSender), } impl Interface for Upload {} @@ -160,6 +160,33 @@ pub struct Stream { pub stream: BoundedStream>, } +/// The result of a [`Stream`] operation. +pub struct StreamResult { + /// The signed location of the upload. + /// + /// This is "final" because we have either a pre-commited `Upload-Length` or + /// a oneshot upload. + pub location: SignedLocation, + /// The byte offset stored on the server after the operation. + pub offset: usize, +} + +impl StreamResult { + fn try_from_response(response: Response) -> Result { + let offset = response + .headers() + .get(tus::UPLOAD_OFFSET) + .ok_or(Error::InvalidLocation(None))? // FIXME: different error + .to_str() + .map_err(|_| Error::InvalidLocation(None))? + .parse() + .map_err(|_| Error::InvalidLocation(None))?; + let location = SignedLocation::try_from_response(response)?; + + Ok(Self { location, offset }) + } +} + impl FromMessage for Upload { type Response = AsyncResponse, Error>>; @@ -178,12 +205,9 @@ impl FromMessage for Upload { } impl FromMessage for Upload { - type Response = AsyncResponse, Error>>; + type Response = AsyncResponse>; - fn from_message( - message: Stream, - sender: Sender, Error>>, - ) -> Self { + fn from_message(message: Stream, sender: Sender>) -> Self { Self::Stream( Box::new(message), InstrumentedSender { @@ -242,13 +266,13 @@ pub struct Service { } /// A response channel that emits a metric for each response. -pub struct InstrumentedSender { +pub struct InstrumentedSender { metric: RelayCounters, - inner: Sender, Error>>, + inner: Sender>, } -impl InstrumentedSender { - fn send(self, result: Result, Error>) { +impl InstrumentedSender { + fn send(self, result: Result) { let result_msg = match &result { Ok(_) => "success", Err(e) => e.variant(), @@ -338,7 +362,7 @@ impl Service { } } - async fn upload(&self, stream: Stream) -> Result, Error> { + async fn upload(&self, stream: Stream) -> Result { let Stream { #[cfg_attr(not(feature = "processing"), expect(unused))] received, @@ -353,11 +377,11 @@ impl Service { UploadRequest::upload(project, location.try_to_uri()?, offset, stream); addr.send(SendRequest(request)); let response = rx.await??; - SignedLocation::try_from_response(response) + StreamResult::try_from_response(response) } #[cfg(feature = "processing")] Backend::Objectstore { addr, config } => { - use crate::services::objectstore::{StreamingMode, UploadRef}; + use crate::services::objectstore::{StreamMode, UploadRef}; let Location { project_id, @@ -371,7 +395,7 @@ impl Service { debug_assert_eq!(scoping.project_id, project_id); let mode = match dbg!((length.value(), upload_id)) { - (Some(length), Some(upload_id)) => StreamingMode::Multipart { + (Some(length), Some(upload_id)) => StreamMode::Multipart { upload_id, offset, length, @@ -380,7 +404,7 @@ impl Service { if offset != 0 { return Err(Error::OffsetWithoutLength); } - StreamingMode::Oneshot(stream.byte_counter()) + StreamMode::Oneshot(stream.byte_counter()) } (None, Some(_)) => { // NOTE: this restriction could be encoded into the Location type. @@ -405,22 +429,30 @@ impl Service { offset, } = upload_ref; - Location { - project_id, - key, - length: Provisional(Some(offset)), // FIXME: could be Final - upload_id: upload_id.map(|id| id.to_string()), - other, - } - .try_sign(config) + // For oneshot requests, update the length: + let length = match mode { + StreamMode::Oneshot(byte_counter) => byte_counter.get(), + StreamMode::Multipart { length, .. } => length, + }; + + Ok(StreamResult { + location: Location { + project_id, + key, + length: Final(length), + upload_id: upload_id.map(|id| id.to_string()), + other, + } + .try_sign(config)?, + offset, + }) } } } - async fn timeout(&self, future: F) -> Result, Error> + async fn timeout(&self, future: F) -> Result where - L: UploadLength, - F: IntoFuture, Error>>, + F: IntoFuture>, { tokio::time::timeout(self.timeout, future).await? } diff --git a/tests/integration/test_upload.py b/tests/integration/test_upload.py index 5443ae815c7..5745ae79d25 100644 --- a/tests/integration/test_upload.py +++ b/tests/integration/test_upload.py @@ -10,11 +10,11 @@ from flask import Response from objectstore_client.multipart import MultipartUpload import pytest -import zstandard +import random from sentry_relay.auth import SecretKey -from .asserts import matches_any +from .asserts import matches, matches_any from .consts import ( DUMMY_UPLOAD_PATH, DUMMY_UPLOAD_LOCATION, @@ -677,7 +677,12 @@ def test_upload_offset( relay = relay_with_processing() objectstore = objectstore("attachments", project_id) - data = 3 * 1024 * 1024 * b"X" + import time + + t = time.monotonic() + part = random.randbytes(7 * 1024 * 1024) + print(f"generated rand bytes in {time.monotonic() - t}") + data = 3 * part response = relay.post( f"/api/{project_id}/upload/?sentry_key={project_key}", headers={ @@ -711,10 +716,10 @@ def test_upload_offset( ).groups() (part1,) = MultipartUpload(objectstore, key, upload_id).list_parts() assert vars(part1) == { - "part_number": 1, + "part_number": 6, "etag": matches_any(), "last_modified": matches_any(), - "size": len(zstandard.compress(data1)), + "size": matches(lambda x: 6e6 <= x <= 6e7), } else: (key,) = LOCATION_REGEX.match(response.headers["Location"]).groups() @@ -729,7 +734,6 @@ def test_upload_offset( "Content-Type": "application/offset+octet-stream", "Tus-Resumable": "1.0.0", "Upload-Offset": str(len(data1)), - # "Upload-Length": str(len(data)), # TODO: can we even make this required? }, data=data2, ) @@ -741,10 +745,9 @@ def test_upload_offset( assert response.status_code == 400 assert response.json() == { "causes": [ - "objectstore upload failed", - "offset without upload ID", + "Upload-Offset > 0 is currently unsupported with Defer-Length: 1", ], - "detail": "upload error: objectstore upload failed", + "detail": "upload error: Upload-Offset > 0 is currently unsupported with Defer-Length: 1", } From 941913c6628b69658ffbe3c7dd2e97b61edde1bc Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Sat, 18 Jul 2026 18:29:24 +0200 Subject: [PATCH 35/41] error handling --- relay-server/src/endpoints/upload.rs | 3 ++- relay-server/src/services/upload.rs | 26 ++++++++++++++++---------- 2 files changed, 18 insertions(+), 11 deletions(-) diff --git a/relay-server/src/endpoints/upload.rs b/relay-server/src/endpoints/upload.rs index 95e262f93a0..2e198428ef2 100644 --- a/relay-server/src/endpoints/upload.rs +++ b/relay-server/src/endpoints/upload.rs @@ -104,9 +104,10 @@ impl IntoResponse for Error { Some(status) => status, None => StatusCode::INTERNAL_SERVER_ERROR, }, - upload::Error::InvalidLocation(_) | upload::Error::SigningFailed => { + upload::Error::InvalidFromUpstream { .. } | upload::Error::SigningFailed => { StatusCode::INTERNAL_SERVER_ERROR } + upload::Error::InvalidFromClient { .. } => StatusCode::BAD_REQUEST, #[cfg(feature = "processing")] upload::Error::InvalidUploadId(_) => StatusCode::BAD_REQUEST, upload::Error::OffsetWithoutLength => StatusCode::BAD_REQUEST, diff --git a/relay-server/src/services/upload.rs b/relay-server/src/services/upload.rs index 3d6d7c99080..1d590aabd5a 100644 --- a/relay-server/src/services/upload.rs +++ b/relay-server/src/services/upload.rs @@ -58,8 +58,10 @@ pub enum Error { Timeout(#[from] tokio::time::error::Elapsed), #[error("error response from upstream: {0}")] Upstream(#[source] reqwest::Error), - #[error("upstream provided invalid location: {0:?}")] - InvalidLocation(Option), + #[error("upstream provided invalid data for {0}: {1:?}")] + InvalidFromUpstream(&'static str, Option), + #[error("invalid data for {0}")] + InvalidFromClient(&'static str), #[cfg(feature = "processing")] #[error(transparent)] InvalidUploadId(#[from] objectstore_types::multipart::InvalidUploadId), @@ -89,7 +91,8 @@ impl Error { Error::UpstreamRequest(_) => "upstream_request", Error::Timeout(_) => "timeout", Error::Upstream(_) => "upstream_response", - Error::InvalidLocation(_) => "invalid_location", + Error::InvalidFromClient { .. } => "invalid_from_client", + Error::InvalidFromUpstream { .. } => "invalid_from_upstream", #[cfg(feature = "processing")] Error::InvalidUploadId(_) => "invalid_upload_id", Error::OffsetWithoutLength => "offset_with_defer_length", @@ -176,11 +179,11 @@ impl StreamResult { let offset = response .headers() .get(tus::UPLOAD_OFFSET) - .ok_or(Error::InvalidLocation(None))? // FIXME: different error + .ok_or(Error::InvalidFromUpstream("Upload-Offset", None))? .to_str() - .map_err(|_| Error::InvalidLocation(None))? + .map_err(|_| Error::InvalidFromUpstream("Upload-Offset", None))? .parse() - .map_err(|_| Error::InvalidLocation(None))?; + .map_err(|_| Error::InvalidFromUpstream("Upload-Offset", None))?; let location = SignedLocation::try_from_response(response)?; Ok(Self { location, offset }) @@ -408,7 +411,9 @@ impl Service { } (None, Some(_)) => { // NOTE: this restriction could be encoded into the Location type. - return Err(Error::InvalidLocation(None)); + return Err(Error::InvalidFromClient( + "upload_id without `Upload-Length`", + )); } }; let upload_ref = addr @@ -731,11 +736,12 @@ where let header = response .headers() .get(hyper::header::LOCATION) - .ok_or(Error::InvalidLocation(None))?; + .ok_or(Error::InvalidFromUpstream("location", None))?; let uri = header .to_str() - .map_err(|_| Error::InvalidLocation(Some(header.clone())))?; - Self::try_from_str(uri).ok_or(Error::InvalidLocation(Some(header.clone()))) + .map_err(|_| Error::InvalidFromUpstream("location", Some(header.clone())))?; + Self::try_from_str(uri) + .ok_or(Error::InvalidFromUpstream("location", Some(header.clone()))) } Err(e) => Err(Error::Upstream(e)), } From 84c16dbf3c956a7005acfa8e1ab3f4f68b53dac7 Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Sat, 18 Jul 2026 21:36:02 +0200 Subject: [PATCH 36/41] fix: save trailing data --- relay-server/src/services/objectstore.rs | 67 ++++++++++++------------ tests/integration/test_upload.py | 6 +-- 2 files changed, 36 insertions(+), 37 deletions(-) diff --git a/relay-server/src/services/objectstore.rs b/relay-server/src/services/objectstore.rs index 10a96d8933c..41cda95301c 100644 --- a/relay-server/src/services/objectstore.rs +++ b/relay-server/src/services/objectstore.rs @@ -988,9 +988,8 @@ impl ObjectstoreServiceInner { }); } - let mut multipart_upload = - session.resume_multipart_upload(key, upload_id)?; - let mut final_upload_id = Some(multipart_upload.upload_id().clone()); + let mut multipart = session.resume_multipart_upload(key, upload_id)?; + let mut final_upload_id = Some(multipart.upload_id().clone()); relay_statsd::metric!( timer(RelayTimers::AttachmentUploadDuration), @@ -1006,11 +1005,12 @@ impl ObjectstoreServiceInner { // chunks of bytes and send each chunk as an individual request. // // Every chunk needs to be > 5 MB because that's what multipart requires. - let mut parts = vec![]; - let mut compressed_buffer = zstd::stream::write::Encoder::new(vec![], 0).map_err(AttemptUploadError::Zstd)?; + let mut flushed_parts = vec![]; + let mut previous_buffer = (offset, vec![]); + let mut compressor = zstd::stream::write::Encoder::new(vec![], 0).map_err(AttemptUploadError::Zstd)?; let mut buffered_offset = offset; let mut flushed_offset = offset; - let mut is_final = false; + let mut is_closing = false; while let Some(bytes) = body.next().await { let bytes = bytes.map_err(objectstore_client::Error::Io)?; @@ -1018,16 +1018,21 @@ impl ObjectstoreServiceInner { buffered_offset += bytes.len(); // Only accept full grains, unless the upload is done: - is_final = buffered_offset == length; - if is_final || bytes.len() == granularity { - compressed_buffer.write_all(&bytes).map_err(AttemptUploadError::Zstd)?; - if is_final || compressed_buffer.get_ref().len() > MULTIPART_MIN_PART_SIZE { - self.try_flush(&mut compressed_buffer, buffered_offset, &mut multipart_upload, &mut parts).await?; + is_closing = buffered_offset == length; + if is_closing || bytes.len() == granularity { + compressor.write_all(&bytes).map_err(AttemptUploadError::Zstd)?; + if is_closing || compressor.get_ref().len() > MULTIPART_MIN_PART_SIZE { + let mut new_compressor = zstd::stream::write::Encoder::new(vec![], 0).map_err(AttemptUploadError::Zstd)?; + std::mem::swap(&mut compressor, &mut new_compressor); + let compressed = new_compressor.finish().map_err(AttemptUploadError::Zstd)?; + let mut current_buffer = (buffered_offset, compressed); + std::mem::swap(&mut current_buffer, &mut previous_buffer); + self.try_flush(current_buffer, &mut multipart, &mut flushed_parts).await?; flushed_offset = buffered_offset; } } - if is_final { + if is_closing { // This should already be guaranteed by the BoundedStream. #[cfg(debug_assertions)] debug_assert!(body.next().await.is_none()); @@ -1035,16 +1040,17 @@ impl ObjectstoreServiceInner { } } - let remaining_compressed_bytes = compressed_buffer.get_ref().len(); - if remaining_compressed_bytes > 0 { - // This can happen when the body compresses so well that it never - // passes the minimum mark. - relay_log::warn!("Stream ended before reaching minimum part size ({remaining_compressed_bytes} < {MULTIPART_MIN_PART_SIZE})"); - } + // The remaining `current_buffer` might not be large enough to flush on its own, + // which is why we kept the `previous_buffer` around to save it: + let (_, mut head) = previous_buffer; + let tail = compressor.finish().map_err(AttemptUploadError::Zstd)?; + relay_log::trace!("Combining {} previous bytes with {} remaining", head.len(), tail.len()); + head.extend(tail); + self.try_flush((buffered_offset, head), &mut multipart, &mut flushed_parts).await?; - if is_final { + if is_closing { relay_log::trace!("Completing multipart upload"); - let parts = multipart_upload.list_parts().await?; + let parts = multipart.list_parts().await?; // Verify that the last written part number is the highest part number in the list. // Otherwise clients could fake a small `Upload-Length` for a large upload. @@ -1060,7 +1066,7 @@ impl ObjectstoreServiceInner { CompletePart { part_number, etag } }); - let final_key = multipart_upload.complete(parts).await?; + let final_key = multipart.complete(parts).await?; final_upload_id = None; debug_assert_eq!(&original_key, &final_key); } @@ -1079,23 +1085,16 @@ impl ObjectstoreServiceInner { async fn try_flush( &self, - buffer: &mut zstd::stream::write::Encoder<'_, Vec>, - byte_offset: usize, + buffer: (usize, Vec), multipart: &mut MultipartUpload, parts: &mut Vec, ) -> Result<(), AttemptUploadError> { - if buffer.get_ref().is_empty() { + let (end_offset, bytes) = buffer; + if bytes.is_empty() { return Ok(()); } - let mut new_buffer = - zstd::stream::write::Encoder::new(vec![], 0).map_err(AttemptUploadError::Zstd)?; - std::mem::swap(buffer, &mut new_buffer); - let chunk: Bytes = new_buffer - .finish() - .map_err(AttemptUploadError::Zstd)? - .into(); - - let part_number = byte_offset.div_ceil(UPLOAD_GRANULARITY.get()); + let bytes = Bytes::from(bytes); + let part_number = end_offset.div_ceil(UPLOAD_GRANULARITY.get()); let part_number = u32::try_from(part_number) .map_err(|_| objectstore_client::Error::InvalidPartNumber(u32::MAX))?; @@ -1104,7 +1103,7 @@ impl ObjectstoreServiceInner { let mut attempts = 0; let part = loop { let result = multipart - .put(chunk.clone(), part_number, None) + .put(bytes.clone(), part_number, None) .await .map_err(AttemptUploadError::Objectstore); attempts += 1; diff --git a/tests/integration/test_upload.py b/tests/integration/test_upload.py index 5745ae79d25..e710acef74d 100644 --- a/tests/integration/test_upload.py +++ b/tests/integration/test_upload.py @@ -680,7 +680,7 @@ def test_upload_offset( import time t = time.monotonic() - part = random.randbytes(7 * 1024 * 1024) + part = random.randbytes(5 * 1024 * 1024) print(f"generated rand bytes in {time.monotonic() - t}") data = 3 * part response = relay.post( @@ -716,10 +716,10 @@ def test_upload_offset( ).groups() (part1,) = MultipartUpload(objectstore, key, upload_id).list_parts() assert vars(part1) == { - "part_number": 6, + "part_number": 5, "etag": matches_any(), "last_modified": matches_any(), - "size": matches(lambda x: 6e6 <= x <= 6e7), + "size": matches(lambda x: 5e6 < x < 6e6), } else: (key,) = LOCATION_REGEX.match(response.headers["Location"]).groups() From 1aad8f4bd55b771e26cf80c2a97ec2c0e6ea9a31 Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Sat, 18 Jul 2026 22:09:41 +0200 Subject: [PATCH 37/41] fix --- relay-server/src/endpoints/upload.rs | 1 + relay-server/src/services/objectstore.rs | 43 +++++++++++++++++------- relay-server/src/services/upload.rs | 4 +-- tests/integration/conftest.py | 1 - tests/integration/test_minidump.py | 2 +- tests/integration/test_upload.py | 18 +++++----- 6 files changed, 45 insertions(+), 24 deletions(-) diff --git a/relay-server/src/endpoints/upload.rs b/relay-server/src/endpoints/upload.rs index 2e198428ef2..701ab0965c5 100644 --- a/relay-server/src/endpoints/upload.rs +++ b/relay-server/src/endpoints/upload.rs @@ -120,6 +120,7 @@ impl IntoResponse for Error { objectstore::ErrorKind::InvalidOffset { .. } => StatusCode::BAD_REQUEST, objectstore::ErrorKind::OffsetWithoutUploadId => StatusCode::BAD_REQUEST, objectstore::ErrorKind::InvalidUploadLength { .. } => StatusCode::BAD_REQUEST, + objectstore::ErrorKind::RequestTooSmall { .. } => StatusCode::BAD_REQUEST, objectstore::ErrorKind::UnknownKey { .. } => StatusCode::NOT_FOUND, objectstore::ErrorKind::Timeout(_) => StatusCode::GATEWAY_TIMEOUT, objectstore::ErrorKind::LoadShed => StatusCode::SERVICE_UNAVAILABLE, diff --git a/relay-server/src/services/objectstore.rs b/relay-server/src/services/objectstore.rs index 41cda95301c..2d9dd564eb7 100644 --- a/relay-server/src/services/objectstore.rs +++ b/relay-server/src/services/objectstore.rs @@ -301,6 +301,8 @@ pub enum ErrorKind { OffsetWithoutUploadId, #[error("invalid Upload-Length {length}: server has {server_offset} bytes")] InvalidUploadLength { length: usize, server_offset: usize }, + #[error("request too small: compressed body is {size}, need at least {min_size}")] + RequestTooSmall { size: usize, min_size: usize }, #[error("unknown key {0}")] UnknownKey(String), #[error("timeout: {0}")] @@ -322,6 +324,7 @@ impl ErrorKind { Self::InvalidOffset { .. } => "invalid_offset", Self::OffsetWithoutUploadId => "offset_without_upload_id", Self::InvalidUploadLength { .. } => "invalid_upload_length", + Self::RequestTooSmall { .. } => "request_too_small", Self::UnknownKey { .. } => "invalid_length", Self::Timeout(_) => "timeout", Self::LoadShed => "load_shed", @@ -968,11 +971,11 @@ impl ObjectstoreServiceInner { StreamMode::Oneshot(byte_counter) => { let request = session.put_stream(body.boxed()).key(key); let response = request.send().await?; - return Ok(UploadRef { + Ok(UploadRef { key: response.key, upload_id: None, offset: byte_counter.get(), - }); + }) } StreamMode::Multipart { upload_id, @@ -1021,14 +1024,15 @@ impl ObjectstoreServiceInner { is_closing = buffered_offset == length; if is_closing || bytes.len() == granularity { compressor.write_all(&bytes).map_err(AttemptUploadError::Zstd)?; + relay_log::trace!("compressor now holds {} bytes", compressor.get_ref().len()); if is_closing || compressor.get_ref().len() > MULTIPART_MIN_PART_SIZE { + relay_log::trace!("swap and flush"); let mut new_compressor = zstd::stream::write::Encoder::new(vec![], 0).map_err(AttemptUploadError::Zstd)?; std::mem::swap(&mut compressor, &mut new_compressor); let compressed = new_compressor.finish().map_err(AttemptUploadError::Zstd)?; let mut current_buffer = (buffered_offset, compressed); std::mem::swap(&mut current_buffer, &mut previous_buffer); - self.try_flush(current_buffer, &mut multipart, &mut flushed_parts).await?; - flushed_offset = buffered_offset; + flushed_offset += self.try_flush(current_buffer, &mut multipart, &mut flushed_parts).await?; } } @@ -1042,11 +1046,20 @@ impl ObjectstoreServiceInner { // The remaining `current_buffer` might not be large enough to flush on its own, // which is why we kept the `previous_buffer` around to save it: - let (_, mut head) = previous_buffer; - let tail = compressor.finish().map_err(AttemptUploadError::Zstd)?; - relay_log::trace!("Combining {} previous bytes with {} remaining", head.len(), tail.len()); - head.extend(tail); - self.try_flush((buffered_offset, head), &mut multipart, &mut flushed_parts).await?; + let (_, mut remaining) = previous_buffer; + if !compressor.get_ref().is_empty() { + let remaining2 = compressor.finish().map_err(AttemptUploadError::Zstd)?; + relay_log::trace!("Combining {} previous bytes with {} remaining", remaining.len(), remaining2.len()); + remaining.extend(remaining2); + } + if is_closing || remaining.len() >= MULTIPART_MIN_PART_SIZE { + flushed_offset += self.try_flush((buffered_offset, remaining), &mut multipart, &mut flushed_parts).await?; + } else if !remaining.is_empty() { + // This can happen when a non-closing request body compresses so well that it + // fits under the S3 limit. In this case the client is forced + // to query the `Upload-Offset` again (see INGEST-1025) and retry with a larger chunk. + return Err(AttemptUploadError::RequestTooSmall {size: remaining.len(), min_size: MULTIPART_MIN_PART_SIZE}); + } if is_closing { relay_log::trace!("Completing multipart upload"); @@ -1088,10 +1101,11 @@ impl ObjectstoreServiceInner { buffer: (usize, Vec), multipart: &mut MultipartUpload, parts: &mut Vec, - ) -> Result<(), AttemptUploadError> { + ) -> Result { let (end_offset, bytes) = buffer; + relay_log::trace!("Trying to flush {} bytes", bytes.len()); if bytes.is_empty() { - return Ok(()); + return Ok(end_offset); } let bytes = Bytes::from(bytes); let part_number = end_offset.div_ceil(UPLOAD_GRANULARITY.get()); @@ -1118,7 +1132,7 @@ impl ObjectstoreServiceInner { parts.push(part); - Ok(()) + Ok(end_offset) } fn session( @@ -1147,6 +1161,8 @@ enum AttemptUploadError { InvalidOffset { offset: usize, granularity: usize }, #[error("invalid Upload-Length {length}: server has {server_offset} bytes")] InvalidUploadLength { length: usize, server_offset: usize }, + #[error("Request too small: compressed body is {size}, need at least {min_size}")] + RequestTooSmall { size: usize, min_size: usize }, } impl From for ErrorKind { @@ -1168,6 +1184,9 @@ impl From for ErrorKind { length, server_offset, }, + AttemptUploadError::RequestTooSmall { size, min_size } => { + ErrorKind::RequestTooSmall { size, min_size } + } } } } diff --git a/relay-server/src/services/upload.rs b/relay-server/src/services/upload.rs index 1d590aabd5a..2a60b390ade 100644 --- a/relay-server/src/services/upload.rs +++ b/relay-server/src/services/upload.rs @@ -60,7 +60,7 @@ pub enum Error { Upstream(#[source] reqwest::Error), #[error("upstream provided invalid data for {0}: {1:?}")] InvalidFromUpstream(&'static str, Option), - #[error("invalid data for {0}")] + #[error("invalid input: {0}")] InvalidFromClient(&'static str), #[cfg(feature = "processing")] #[error(transparent)] @@ -397,7 +397,7 @@ impl Service { let scoping = project.scoping; debug_assert_eq!(scoping.project_id, project_id); - let mode = match dbg!((length.value(), upload_id)) { + let mode = match (length.value(), upload_id) { (Some(length), Some(upload_id)) => StreamMode::Multipart { upload_id, offset, diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index a01bb33f973..ffb2ffa9839 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -307,7 +307,6 @@ def dummy_upload(mini_sentry): # noqa @mini_sentry.app.route("/api//upload/", methods=["POST"]) def create(**opts): - return Response( "", status=201, diff --git a/tests/integration/test_minidump.py b/tests/integration/test_minidump.py index 89ae3a71764..f7badb5f233 100644 --- a/tests/integration/test_minidump.py +++ b/tests/integration/test_minidump.py @@ -1062,7 +1062,7 @@ def test_size_limits(mini_sentry, relay, limit, expected_status_code): ], ids=["no-option", "no-minidumps", "stream-minidumps"], ) -def test_minidump_objectstore_uploads( +def test_minidump_streaming_uploads( mini_sentry, relay, dummy_upload, diff --git a/tests/integration/test_upload.py b/tests/integration/test_upload.py index e710acef74d..2a6788f9d5e 100644 --- a/tests/integration/test_upload.py +++ b/tests/integration/test_upload.py @@ -677,11 +677,7 @@ def test_upload_offset( relay = relay_with_processing() objectstore = objectstore("attachments", project_id) - import time - - t = time.monotonic() - part = random.randbytes(5 * 1024 * 1024) - print(f"generated rand bytes in {time.monotonic() - t}") + part = random.randbytes(13 * 1024 * 1024) data = 3 * part response = relay.post( f"/api/{project_id}/upload/?sentry_key={project_key}", @@ -714,12 +710,18 @@ def test_upload_offset( key, upload_id = LOCATION_REGEX_WITH_UPLOAD_ID.match( response.headers["Location"] ).groups() - (part1,) = MultipartUpload(objectstore, key, upload_id).list_parts() + part1, part2 = MultipartUpload(objectstore, key, upload_id).list_parts() assert vars(part1) == { - "part_number": 5, + "part_number": 6, + "etag": matches_any(), + "last_modified": matches_any(), + "size": matches(lambda x: 6e6 < x < 7e6), + } + assert vars(part2) == { + "part_number": 13, "etag": matches_any(), "last_modified": matches_any(), - "size": matches(lambda x: 5e6 < x < 6e6), + "size": matches(lambda x: 7e6 < x < 8e6), } else: (key,) = LOCATION_REGEX.match(response.headers["Location"]).groups() From 533f818141290aa21e3428ac9f83047a40b2ddfc Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Sat, 18 Jul 2026 22:38:03 +0200 Subject: [PATCH 38/41] fixes by claude --- relay-server/src/services/objectstore.rs | 36 ++++++++---------------- tests/integration/test_upload.py | 3 ++ 2 files changed, 15 insertions(+), 24 deletions(-) diff --git a/relay-server/src/services/objectstore.rs b/relay-server/src/services/objectstore.rs index 2d9dd564eb7..3d8509defe5 100644 --- a/relay-server/src/services/objectstore.rs +++ b/relay-server/src/services/objectstore.rs @@ -376,17 +376,6 @@ pub struct UploadRef { pub offset: usize, } -// impl UploadRef { -// /// Validates the upload ID and returns a new upload reference. -// // pub fn new(key: String, upload_id: Option) -> Result { -// // let upload_id = match upload_id { -// // Some(s) => Some(UploadId::new(s)?), -// // None => None, -// // }; -// // Ok(Self { key, upload_id }) -// // } -// } - impl fmt::Display for ObjectstoreKey { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { self.0.fmt(f) @@ -1011,28 +1000,27 @@ impl ObjectstoreServiceInner { let mut flushed_parts = vec![]; let mut previous_buffer = (offset, vec![]); let mut compressor = zstd::stream::write::Encoder::new(vec![], 0).map_err(AttemptUploadError::Zstd)?; - let mut buffered_offset = offset; - let mut flushed_offset = offset; + let mut new_offset = offset; let mut is_closing = false; while let Some(bytes) = body.next().await { let bytes = bytes.map_err(objectstore_client::Error::Io)?; debug_assert!(bytes.len() <= granularity); - buffered_offset += bytes.len(); - // Only accept full grains, unless the upload is done: - is_closing = buffered_offset == length; + // Only accept full grains, unless the upload is done. + is_closing = new_offset + bytes.len() == length; if is_closing || bytes.len() == granularity { compressor.write_all(&bytes).map_err(AttemptUploadError::Zstd)?; + new_offset += bytes.len(); relay_log::trace!("compressor now holds {} bytes", compressor.get_ref().len()); if is_closing || compressor.get_ref().len() > MULTIPART_MIN_PART_SIZE { relay_log::trace!("swap and flush"); let mut new_compressor = zstd::stream::write::Encoder::new(vec![], 0).map_err(AttemptUploadError::Zstd)?; std::mem::swap(&mut compressor, &mut new_compressor); let compressed = new_compressor.finish().map_err(AttemptUploadError::Zstd)?; - let mut current_buffer = (buffered_offset, compressed); + let mut current_buffer = (new_offset, compressed); std::mem::swap(&mut current_buffer, &mut previous_buffer); - flushed_offset += self.try_flush(current_buffer, &mut multipart, &mut flushed_parts).await?; + self.try_flush(current_buffer, &mut multipart, &mut flushed_parts).await?; } } @@ -1053,7 +1041,7 @@ impl ObjectstoreServiceInner { remaining.extend(remaining2); } if is_closing || remaining.len() >= MULTIPART_MIN_PART_SIZE { - flushed_offset += self.try_flush((buffered_offset, remaining), &mut multipart, &mut flushed_parts).await?; + self.try_flush((new_offset, remaining), &mut multipart, &mut flushed_parts).await?; } else if !remaining.is_empty() { // This can happen when a non-closing request body compresses so well that it // fits under the S3 limit. In this case the client is forced @@ -1068,7 +1056,7 @@ impl ObjectstoreServiceInner { // Verify that the last written part number is the highest part number in the list. // Otherwise clients could fake a small `Upload-Length` for a large upload. let max_part_number = parts.iter().map(|p|p.part_number.get()).max().unwrap_or(0) as usize; - if max_part_number * granularity != flushed_offset { + if max_part_number != new_offset.div_ceil(granularity) { return Err(AttemptUploadError::InvalidUploadLength { length, server_offset: max_part_number * granularity }) @@ -1087,7 +1075,7 @@ impl ObjectstoreServiceInner { Ok(UploadRef { key: original_key, upload_id: final_upload_id, - offset: flushed_offset, + offset: new_offset, }) }) } @@ -1101,11 +1089,11 @@ impl ObjectstoreServiceInner { buffer: (usize, Vec), multipart: &mut MultipartUpload, parts: &mut Vec, - ) -> Result { + ) -> Result<(), AttemptUploadError> { let (end_offset, bytes) = buffer; relay_log::trace!("Trying to flush {} bytes", bytes.len()); if bytes.is_empty() { - return Ok(end_offset); + return Ok(()); } let bytes = Bytes::from(bytes); let part_number = end_offset.div_ceil(UPLOAD_GRANULARITY.get()); @@ -1132,7 +1120,7 @@ impl ObjectstoreServiceInner { parts.push(part); - Ok(end_offset) + Ok(()) } fn session( diff --git a/tests/integration/test_upload.py b/tests/integration/test_upload.py index 2a6788f9d5e..63565365289 100644 --- a/tests/integration/test_upload.py +++ b/tests/integration/test_upload.py @@ -705,6 +705,8 @@ def test_upload_offset( data=data1, ) assert response.status_code == 204, response.text + # Relay acknowledges exactly the bytes it durably stored: + assert response.headers["Upload-Offset"] == str(len(data1)) if feature_flag: key, upload_id = LOCATION_REGEX_WITH_UPLOAD_ID.match( @@ -741,6 +743,7 @@ def test_upload_offset( ) if feature_flag: assert response.status_code == 204 + assert response.headers["Upload-Offset"] == str(len(data)) (key,) = LOCATION_REGEX.match(response.headers["Location"]).groups() assert objectstore.get(key).payload.read() == data else: From 99615fb970ea40fa28d9c6d02c5dc0e120af94bd Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Sat, 18 Jul 2026 22:49:43 +0200 Subject: [PATCH 39/41] test --- relay-server/src/services/upload.rs | 6 +++--- tests/integration/conftest.py | 5 ++++- tests/integration/test_upload.py | 19 ++++++++++++------- 3 files changed, 19 insertions(+), 11 deletions(-) diff --git a/relay-server/src/services/upload.rs b/relay-server/src/services/upload.rs index 2a60b390ade..0f049e085e4 100644 --- a/relay-server/src/services/upload.rs +++ b/relay-server/src/services/upload.rs @@ -179,11 +179,11 @@ impl StreamResult { let offset = response .headers() .get(tus::UPLOAD_OFFSET) - .ok_or(Error::InvalidFromUpstream("Upload-Offset", None))? + .ok_or(Error::InvalidFromUpstream(tus::UPLOAD_OFFSET, None))? .to_str() - .map_err(|_| Error::InvalidFromUpstream("Upload-Offset", None))? + .map_err(|_| Error::InvalidFromUpstream(tus::UPLOAD_OFFSET, None))? .parse() - .map_err(|_| Error::InvalidFromUpstream("Upload-Offset", None))?; + .map_err(|_| Error::InvalidFromUpstream(tus::UPLOAD_OFFSET, None))?; let location = SignedLocation::try_from_response(response)?; Ok(Self { location, offset }) diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index ffb2ffa9839..75288ff8918 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -321,5 +321,8 @@ def upload(**opts): return Response( "", status=204, - headers={"Location": DUMMY_UPLOAD_LOCATION}, + headers={ + "Location": DUMMY_UPLOAD_LOCATION, + "Upload-Offset": len(request.data), + }, ) diff --git a/tests/integration/test_upload.py b/tests/integration/test_upload.py index 63565365289..a437a3e24db 100644 --- a/tests/integration/test_upload.py +++ b/tests/integration/test_upload.py @@ -341,10 +341,16 @@ def test_timeout( """Ensure that the general HTTP timeout does not affect the upload endpoint""" mini_sentry.allow_chunked = True + data = b"hello world" + @mini_sentry.app.route(DUMMY_UPLOAD_PATH, methods=["PATCH"]) def slow_upload(**opts): time.sleep(2) - return Response("", status=204, headers={"Location": DUMMY_UPLOAD_LOCATION}) + return Response( + "", + status=204, + headers={"Location": DUMMY_UPLOAD_LOCATION, "Upload-Offset": len(data)}, + ) project_id = 42 relay = relay( @@ -355,7 +361,6 @@ def slow_upload(**opts): }, ) - data = b"hello world" response = relay.patch( "%s&sentry_key=%s" % ( @@ -594,14 +599,14 @@ def test_objectstore_retries( } ) - location = f"/api/{project_id}/upload/019cdc82ed6c7761ba21fd34b86481c2/" - sep = "?" + location = ( + f"/api/{project_id}/upload/019cdc82ed6c7761ba21fd34b86481c2/?upload_length=11" + ) if with_multipart: - location += "?upload_id=my_upload_id" - sep = "&" + location += "&upload_id=my_upload_id" signature = SecretKey.parse(relay.secret_key).sign(location.encode()) signed_location = ( - f"{location}{sep}sentry_key={project_key}&upload_signature={signature}" + f"{location}&sentry_key={project_key}&upload_signature={signature}" ) data = b"hello world" From 57ebe514476d2a5c57663a23c9574071b60ebf46 Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Sat, 18 Jul 2026 23:07:22 +0200 Subject: [PATCH 40/41] cleanup --- CHANGELOG.md | 2 +- relay-server/src/endpoints/common.rs | 3 +-- relay-server/src/services/objectstore.rs | 22 ++++++++++++++-------- 3 files changed, 16 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 373c032a5d5..96875e646b7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ **Internal**: -- Allow resumable uploads (feature-flagged). ([#6203](https://github.com/getsentry/relay/pull/6203)) +- Support resumable uploads (feature-flagged). ([#6203](https://github.com/getsentry/relay/pull/6203)) ## 26.7.0 diff --git a/relay-server/src/endpoints/common.rs b/relay-server/src/endpoints/common.rs index d555d3ed779..b358e0f5993 100644 --- a/relay-server/src/endpoints/common.rs +++ b/relay-server/src/endpoints/common.rs @@ -611,7 +611,7 @@ where let result = upload .send(Stream { received: Utc::now(), - project: project.clone(), + project, location, offset: 0, stream, @@ -634,7 +634,6 @@ where ); }) .ok()?; - let location = location.into_header_value().ok()?; let location = location.to_str().ok()?; let placeholder = serde_json::to_vec(&AttachmentPlaceholder { diff --git a/relay-server/src/services/objectstore.rs b/relay-server/src/services/objectstore.rs index 3d8509defe5..218882479b0 100644 --- a/relay-server/src/services/objectstore.rs +++ b/relay-server/src/services/objectstore.rs @@ -48,8 +48,6 @@ const UPLOAD_GRANULARITY: NonZeroUsize = NonZeroUsize::new(1024 * 1024).unwrap() /// Every part except the last needs to be at least this size. const MULTIPART_MIN_PART_SIZE: usize = 5 * 1024 * 1024; // 5 MiB -// const CHUNK_SIZE: NonZeroUsize = NonZeroUsize::new(5 * 1024 * 1024).unwrap(); - /// Messages that the objectstore service can handle. pub enum Objectstore { Event(Managed>), @@ -160,9 +158,12 @@ impl FromMessage for Objectstore { } } +/// Whether the current request contains the full upload. #[derive(Clone)] pub enum StreamMode { + /// The current request contains the full upload. Oneshot(ByteCounter), + /// The current request might be partial and requires multipart upload under the hood. Multipart { upload_id: String, offset: usize, @@ -372,7 +373,7 @@ pub struct UploadRef { /// The ID of the multipart upload session (chosen by objectstore). /// `None` if the upload is not multipart. pub upload_id: Option, - /// The byte offset from which to resume the upload, + /// The byte offset from which to resume the upload. pub offset: usize, } @@ -1004,7 +1005,6 @@ impl ObjectstoreServiceInner { let mut is_closing = false; while let Some(bytes) = body.next().await { let bytes = bytes.map_err(objectstore_client::Error::Io)?; - debug_assert!(bytes.len() <= granularity); // Only accept full grains, unless the upload is done. @@ -1012,14 +1012,20 @@ impl ObjectstoreServiceInner { if is_closing || bytes.len() == granularity { compressor.write_all(&bytes).map_err(AttemptUploadError::Zstd)?; new_offset += bytes.len(); - relay_log::trace!("compressor now holds {} bytes", compressor.get_ref().len()); + + // If the current compressed part is large enough, flush the previous one. + // We keep the current one around just in case we need to join it with the last part. + // (see concatenation after while loop). if is_closing || compressor.get_ref().len() > MULTIPART_MIN_PART_SIZE { - relay_log::trace!("swap and flush"); let mut new_compressor = zstd::stream::write::Encoder::new(vec![], 0).map_err(AttemptUploadError::Zstd)?; + + // Replace previous buffer with current compressor content. std::mem::swap(&mut compressor, &mut new_compressor); let compressed = new_compressor.finish().map_err(AttemptUploadError::Zstd)?; let mut current_buffer = (new_offset, compressed); std::mem::swap(&mut current_buffer, &mut previous_buffer); + + // Flush previous buffer. self.try_flush(current_buffer, &mut multipart, &mut flushed_parts).await?; } } @@ -1032,8 +1038,8 @@ impl ObjectstoreServiceInner { } } - // The remaining `current_buffer` might not be large enough to flush on its own, - // which is why we kept the `previous_buffer` around to save it: + // Remaining bytes are now < 5 MiB. Concatenate it with the previous part + // so we reach the limit. let (_, mut remaining) = previous_buffer; if !compressor.get_ref().is_empty() { let remaining2 = compressor.finish().map_err(AttemptUploadError::Zstd)?; From 9bf83902f21008c3e856c6d8c08d1be787a4eacc Mon Sep 17 00:00:00 2001 From: Joris Bayer Date: Sat, 18 Jul 2026 23:45:41 +0200 Subject: [PATCH 41/41] fix: return an error for unaligned bytes --- relay-server/src/endpoints/upload.rs | 1 + relay-server/src/services/objectstore.rs | 33 +++++++++- tests/integration/test_upload.py | 79 ++++++++++++++++++++++++ 3 files changed, 111 insertions(+), 2 deletions(-) diff --git a/relay-server/src/endpoints/upload.rs b/relay-server/src/endpoints/upload.rs index 701ab0965c5..909102c175e 100644 --- a/relay-server/src/endpoints/upload.rs +++ b/relay-server/src/endpoints/upload.rs @@ -121,6 +121,7 @@ impl IntoResponse for Error { objectstore::ErrorKind::OffsetWithoutUploadId => StatusCode::BAD_REQUEST, objectstore::ErrorKind::InvalidUploadLength { .. } => StatusCode::BAD_REQUEST, objectstore::ErrorKind::RequestTooSmall { .. } => StatusCode::BAD_REQUEST, + objectstore::ErrorKind::UnalignedBody { .. } => StatusCode::BAD_REQUEST, objectstore::ErrorKind::UnknownKey { .. } => StatusCode::NOT_FOUND, objectstore::ErrorKind::Timeout(_) => StatusCode::GATEWAY_TIMEOUT, objectstore::ErrorKind::LoadShed => StatusCode::SERVICE_UNAVAILABLE, diff --git a/relay-server/src/services/objectstore.rs b/relay-server/src/services/objectstore.rs index 218882479b0..451e08c4704 100644 --- a/relay-server/src/services/objectstore.rs +++ b/relay-server/src/services/objectstore.rs @@ -304,6 +304,10 @@ pub enum ErrorKind { InvalidUploadLength { length: usize, server_offset: usize }, #[error("request too small: compressed body is {size}, need at least {min_size}")] RequestTooSmall { size: usize, min_size: usize }, + #[error( + "the body of a non-final request must be a multiple of the upload granularity {granularity}, found {trailing} trailing bytes" + )] + UnalignedBody { trailing: usize, granularity: usize }, #[error("unknown key {0}")] UnknownKey(String), #[error("timeout: {0}")] @@ -326,6 +330,7 @@ impl ErrorKind { Self::OffsetWithoutUploadId => "offset_without_upload_id", Self::InvalidUploadLength { .. } => "invalid_upload_length", Self::RequestTooSmall { .. } => "request_too_small", + Self::UnalignedBody { .. } => "unaligned_body", Self::UnknownKey { .. } => "invalid_length", Self::Timeout(_) => "timeout", Self::LoadShed => "load_shed", @@ -337,7 +342,9 @@ impl ErrorKind { fn is_client_error(&self) -> bool { match self { - ErrorKind::InvalidOffset { .. } | ErrorKind::InvalidUploadLength { .. } => true, + ErrorKind::InvalidOffset { .. } + | ErrorKind::InvalidUploadLength { .. } + | ErrorKind::UnalignedBody { .. } => true, ErrorKind::UploadFailed(objectstore_client::Error::Reqwest(error)) => { find_error_source(error, is_user_error).is_some() } @@ -1003,6 +1010,7 @@ impl ObjectstoreServiceInner { let mut compressor = zstd::stream::write::Encoder::new(vec![], 0).map_err(AttemptUploadError::Zstd)?; let mut new_offset = offset; let mut is_closing = false; + let mut misalignment = 0; while let Some(bytes) = body.next().await { let bytes = bytes.map_err(objectstore_client::Error::Io)?; debug_assert!(bytes.len() <= granularity); @@ -1028,6 +1036,10 @@ impl ObjectstoreServiceInner { // Flush previous buffer. self.try_flush(current_buffer, &mut multipart, &mut flushed_parts).await?; } + } else { + // We're at the end of a non-final request, but it is not + // aligned with granularity: return an error. + misalignment = bytes.len(); } if is_closing { @@ -1048,13 +1060,19 @@ impl ObjectstoreServiceInner { } if is_closing || remaining.len() >= MULTIPART_MIN_PART_SIZE { self.try_flush((new_offset, remaining), &mut multipart, &mut flushed_parts).await?; - } else if !remaining.is_empty() { + } else if !remaining.is_empty() && misalignment == 0 { // This can happen when a non-closing request body compresses so well that it // fits under the S3 limit. In this case the client is forced // to query the `Upload-Offset` again (see INGEST-1025) and retry with a larger chunk. return Err(AttemptUploadError::RequestTooSmall {size: remaining.len(), min_size: MULTIPART_MIN_PART_SIZE}); } + // A non-final request did not end at the granularity boundary. + // All full grains were stored, but signal an error to the client. + if misalignment > 0 { + return Err(AttemptUploadError::UnalignedBody { trailing: misalignment, granularity }); + } + if is_closing { relay_log::trace!("Completing multipart upload"); let parts = multipart.list_parts().await?; @@ -1157,6 +1175,10 @@ enum AttemptUploadError { InvalidUploadLength { length: usize, server_offset: usize }, #[error("Request too small: compressed body is {size}, need at least {min_size}")] RequestTooSmall { size: usize, min_size: usize }, + #[error( + "the body of a non-final request must be a multiple of the upload granularity {granularity}, found {trailing} trailing bytes" + )] + UnalignedBody { trailing: usize, granularity: usize }, } impl From for ErrorKind { @@ -1181,6 +1203,13 @@ impl From for ErrorKind { AttemptUploadError::RequestTooSmall { size, min_size } => { ErrorKind::RequestTooSmall { size, min_size } } + AttemptUploadError::UnalignedBody { + trailing, + granularity, + } => ErrorKind::UnalignedBody { + trailing, + granularity, + }, } } } diff --git a/tests/integration/test_upload.py b/tests/integration/test_upload.py index a437a3e24db..70e5f0a28ad 100644 --- a/tests/integration/test_upload.py +++ b/tests/integration/test_upload.py @@ -761,6 +761,85 @@ def test_upload_offset( } +def test_upload_unaligned_body_rejected( + mini_sentry, relay_with_processing, objectstore +): + """A non-final PATCH body that is not a multiple of the upload granularity is rejected. + + Silently dropping the sub-grain tail would return a 204 whose ``Upload-Offset`` + excludes it; for bodies smaller than one grain that means acknowledging zero + progress, which sends offset-driven clients into an endless retry loop. + + Full grains received before the unaligned tail are still persisted where they + form a valid part, so the client can resume from the last durable offset. + """ + project_id = 42 + config = mini_sentry.add_full_project_config(project_id)["config"] + config.setdefault("features", []).append("projects:relay-upload-multipart") + project_key = mini_sentry.get_dsn_public_key(project_id) + + relay = relay_with_processing() + objectstore = objectstore("attachments", project_id) + + data = random.randbytes(10 * 1024 * 1024) + response = relay.post( + f"/api/{project_id}/upload/?sentry_key={project_key}", + headers={ + "Content-Length": "0", + "Tus-Resumable": "1.0.0", + "Upload-Length": str(len(data)), + }, + ) + assert response.status_code == 201 + location = f"{response.headers['Location']}&sentry_key={project_key}" + + # Sub-grain body (512 KiB) that does not complete the upload: + response = relay.patch( + location, + headers={ + "Content-Length": str(512 * 1024), + "Content-Type": "application/offset+octet-stream", + "Tus-Resumable": "1.0.0", + "Upload-Offset": "0", + }, + data=data[: 512 * 1024], + ) + assert response.status_code == 400, response.text + assert "upload granularity" in response.text + + # Grain-aligned body with a sub-grain tail (6.5 MiB) is rejected as well: + response = relay.patch( + location, + headers={ + "Content-Length": str(13 * 512 * 1024), + "Content-Type": "application/offset+octet-stream", + "Tus-Resumable": "1.0.0", + "Upload-Offset": "0", + }, + data=data[: 13 * 512 * 1024], + ) + assert response.status_code == 400, response.text + assert "upload granularity" in response.text + + # The first 6 MiB of the rejected request were still persisted (they formed a + # valid part), so the upload can be resumed and completed from that offset: + resume_offset = 6 * 1024 * 1024 + response = relay.patch( + location, + headers={ + "Content-Length": str(len(data) - resume_offset), + "Content-Type": "application/offset+octet-stream", + "Tus-Resumable": "1.0.0", + "Upload-Offset": str(resume_offset), + }, + data=data[resume_offset:], + ) + assert response.status_code == 204, response.text + assert response.headers["Upload-Offset"] == str(len(data)) + (key,) = LOCATION_REGEX.match(response.headers["Location"]).groups() + assert objectstore.get(key).payload.read() == data + + @pytest.mark.parametrize( "opted_in,metadata,expected_status_code", [