Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion objectstore-server/src/endpoints/batch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,10 @@ async fn got_to_part(
.meter_stream(stream, context)
.try_collect::<BytesMut>()
.await
.map_err(|e| ApiError::Service(e.into()))?
.map_err(|e| {
objectstore_log::error!(!!&e, "failed to collect payload stream");
ApiError::Service(e.into())

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we refactor ApiError similar to #547, we can directly propagate the IO error and no longer have to track it explicitly here.

})?
.freeze();

let mut metadata_headers = metadata.to_headers("").map_err(|err| {
Expand Down Expand Up @@ -317,6 +320,9 @@ fn create_success_part(
}

fn create_error_part(idx: usize, error: &ApiError) -> Part {
// Capture the error explicitly, as it is not converted via `IntoResponse` here.
error.capture();

let mut headers = HeaderMap::new();
insert_index_header(&mut headers, idx);
insert_status_header(&mut headers, error.status());
Expand Down
32 changes: 19 additions & 13 deletions objectstore-server/src/endpoints/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,6 @@ impl ApiError {
ApiError::Batch(BatchError::LimitExceeded(_)) => StatusCode::PAYLOAD_TOO_LARGE,
ApiError::Batch(BatchError::RateLimited) => StatusCode::TOO_MANY_REQUESTS,
ApiError::Batch(BatchError::ResponseSerialization { .. }) => {
objectstore_log::error!(!!self, "error serializing batch response");
StatusCode::INTERNAL_SERVER_ERROR
}

Expand All @@ -89,10 +88,7 @@ impl ApiError {
ApiError::Auth(AuthError::UnknownKey) => StatusCode::UNAUTHORIZED,
ApiError::Auth(AuthError::UnsupportedPresignedMethod) => StatusCode::FORBIDDEN,
ApiError::Auth(AuthError::NotPermitted) => StatusCode::FORBIDDEN,
ApiError::Auth(AuthError::InternalError(_)) => {
objectstore_log::error!(!!self, "auth system error");
StatusCode::INTERNAL_SERVER_ERROR
}
ApiError::Auth(AuthError::InternalError(_)) => StatusCode::INTERNAL_SERVER_ERROR,

ApiError::Service(ServiceError::Client(_)) => StatusCode::BAD_REQUEST,
ApiError::Service(ServiceError::Metadata(_)) => StatusCode::BAD_REQUEST,
Expand All @@ -102,21 +98,31 @@ impl ApiError {
ApiError::Service(ServiceError::InvalidUploadId(_)) => StatusCode::BAD_REQUEST,
ApiError::Service(ServiceError::AtCapacity) => StatusCode::TOO_MANY_REQUESTS,
ApiError::Service(ServiceError::NotImplemented) => StatusCode::NOT_IMPLEMENTED,
ApiError::Service(_) => {
objectstore_log::error!(!!self, "error handling request");
StatusCode::INTERNAL_SERVER_ERROR
}
ApiError::Service(_) => StatusCode::INTERNAL_SERVER_ERROR,
Comment thread
lcian marked this conversation as resolved.

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

/// Reports this error to error tracking if it indicates a server fault (5xx status).
///
/// Call this exactly once wherever an `ApiError` is serialized into a client-visible
/// response: standalone responses ([`IntoResponse`]) and batch response parts.
pub fn capture(&self) {
// Captured at the source in the service layer to prevent double-logging.
if matches!(self, ApiError::Service(_)) {
return;
}

if self.status().is_server_error() {
objectstore_log::error!(!!self, "error handling request");
}
}
}

impl IntoResponse for ApiError {
fn into_response(self) -> Response {
self.capture();
let body = ApiErrorResponse::from_error(&self);
(self.status(), Json(body)).into_response()
}
Expand Down
6 changes: 5 additions & 1 deletion objectstore-service/src/concurrency.rs
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,11 @@ where
}
.bind_hub(new_hub),
);
rx.await.map_err(|_| Error::Dropped)?

rx.await.map_err(|_| {
objectstore_log::error!(!!&Error::Dropped, operation, "Task failed");
Error::Dropped
})?
}

#[cfg(test)]
Expand Down
Loading