fix(log): Capture service errors only once (#558)

For every service error, we received two issues in Sentry:

1.  Captured within the service by the spawned task itself
2.  Captured by the web middleware when unwinding the error

Since tasks can outlive their requests, we need to keep capturing inside
the service. Consequently, we now skip error tracking for service error
kinds in the API layer. There are a few exceptions where a service error
is constructed outside of the spawned task, which is now tracked
explicitly.

Author: jan-auer

objectstore-server/src/endpoints/batch.rs

@@ -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())
+        })?
         .freeze();
 
     let mut metadata_headers = metadata.to_headers("").map_err(|err| {
@@ -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());

objectstore-server/src/endpoints/common.rs

@@ -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
             }
 
@@ -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,
@@ -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,
 
-            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()
     }

objectstore-service/src/concurrency.rs

@@ -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)]