Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jjbayer
force-pushed
the
feat/upload-create
branch
from
0329614 to
8f5c41c
Compare
Move PatchPath/PatchQueryParams from endpoints to services/upload as LocationPath/LocationQueryParams and reuse them in both the endpoint handler and SignedLocation::try_from_str. The matchit router is kept in a static LazyLock so it is only built once. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jjbayer
force-pushed
the
feat/upload-create
branch
from
ec8d5ae to
c47baa6
Compare
| if content_length.is_none_or(|v| v > 0) { | ||
| let stream = body | ||
| .into_data_stream() | ||
| .map(|result| result.map_err(io::Error::other)) | ||
| .boxed(); | ||
| let (lower_bound, upper_bound) = match upload_length { | ||
| None => (1, config.max_upload_size()), | ||
| Some(u) => (u, u), | ||
| }; | ||
| let stream = BoundedStream::new(stream, lower_bound, upper_bound); | ||
| let byte_counter = stream.byte_counter(); | ||
|
|
||
| relay_log::trace!("Uploading"); | ||
| let result = upload(&state, scoping, location, stream).await; | ||
| location = result.inspect_err(|e| { | ||
| relay_log::warn!(error = e as &dyn std::error::Error, "upload failed"); | ||
| })?; | ||
| upload_offset = Some(byte_counter.get()); | ||
| } |
There was a problem hiding this comment.
This block will be removed once we remove "Creation-With-Upload".
| public_key | ||
| .iter() | ||
| .any(|p| self.verify(p, start_time, max_age)) | ||
| .any(|p| self.verify(&[], p, start_time, max_age)) |
There was a problem hiding this comment.
I changed the signature of verify so we can verify the integrity of the data, not just the signature itself.
| Path(upload::LocationPath { project_id, key }): Path<upload::LocationPath>, | ||
| Query(upload::LocationQueryParams { length, signature }): Query<upload::LocationQueryParams>, |
There was a problem hiding this comment.
There's a potential forward-compatibility issue here: Outdated external relays may parse the wrong set of query parameters, i.e. upstream Relays cannot transparently put new query parameters in its signed location.
There's an argument to be made that we should forward all query parameters, but I didn't like how that interacted with for example sentry_key=.
| json-forensics = { workspace = true } | ||
| libc = { workspace = true } | ||
| liblzma = { workspace = true } | ||
| matchit = { workspace = true } |
There was a problem hiding this comment.
Not a new dependency, axum already uses it.
| serde_bytes = { workspace = true } | ||
| serde_json = { workspace = true } | ||
| serde_path_to_error = { workspace = true } | ||
| serde_urlencoded = { workspace = true } |
There was a problem hiding this comment.
Not a new dependency, axum already uses it.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| Poll::Pending => { | ||
| relay_log::trace!("still pending"); | ||
| Poll::Pending | ||
| } |
There was a problem hiding this comment.
Verbose trace logging in hot streaming path
Low Severity
Eight relay_log::trace!() calls were added to BoundedStream::poll_next, which fires on every single poll of the stream — including Pending wakeups. For a large upload with many chunks, this generates thousands of log entries per upload even at trace level. This looks like development debugging output that was left in. Contrast with the per-request traces in the endpoint handlers, which fire once; these per-poll traces are orders of magnitude more voluminous.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| builder.header("X-Sentry-Auth", format!("Sentry sentry_key={project_key}")); | ||
| let upload_length = (body.lower_bound == body.upper_bound).then_some(body.lower_bound); | ||
| for (key, value) in tus::request_headers(upload_length) { | ||
| for (key, value) in tus::request_headers(self.length()) { |
There was a problem hiding this comment.
Upload length lost because stream taken before length() call
Medium Severity
In build(), the stream is consumed via stream.take() before self.length() is called. The length() method for RequestKind::Upload reads from stream.as_ref().and_then(BoundedStream::length), which returns None since the stream was already taken. This causes request_headers(None) to emit Upload-Defer-Length: 1 instead of the correct Upload-Length header on forwarded PATCH requests.
Additional Locations (1)
Dav1dde
left a comment
Dav1dde
left a comment
There was a problem hiding this comment.
As we discussed: For the forward compat we can make the middleware remove sentry_key
2 participants
This PR adds a new route to upload, so a user can now
PATCHed.CreateorUploadmessages, so the upstream will always receive two requests.Once all experimental clients have switched to "create", we can remove the "create-with-upload" branch.
fixes: INGEST-802