From ea4a53fdf5af8b5d4d4ac7d2b0c8e050f3c942d4 Mon Sep 17 00:00:00 2001 From: Jan Michael Auer Date: Wed, 15 Jul 2026 15:22:38 +0200 Subject: [PATCH 1/4] fix(log): Capture service errors only once --- objectstore-server/src/endpoints/common.rs | 5 +---- objectstore-service/src/concurrency.rs | 6 +++++- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/objectstore-server/src/endpoints/common.rs b/objectstore-server/src/endpoints/common.rs index df33f90e..4ee62d18 100644 --- a/objectstore-server/src/endpoints/common.rs +++ b/objectstore-server/src/endpoints/common.rs @@ -102,10 +102,7 @@ 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"); diff --git a/objectstore-service/src/concurrency.rs b/objectstore-service/src/concurrency.rs index a1602f5b..93451085 100644 --- a/objectstore-service/src/concurrency.rs +++ b/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); + Error::Dropped + })? } #[cfg(test)] From 56a33d02680e7babacc2710f4cbe9a42254a35a4 Mon Sep 17 00:00:00 2001 From: Jan Michael Auer Date: Wed, 15 Jul 2026 16:29:56 +0200 Subject: [PATCH 2/4] ref: Another try --- objectstore-server/src/endpoints/batch.rs | 3 +++ objectstore-server/src/endpoints/common.rs | 23 +++++++++++++--------- objectstore-service/src/concurrency.rs | 18 ++++++++--------- 3 files changed, 25 insertions(+), 19 deletions(-) diff --git a/objectstore-server/src/endpoints/batch.rs b/objectstore-server/src/endpoints/batch.rs index b62219fa..6f8d1981 100644 --- a/objectstore-server/src/endpoints/batch.rs +++ b/objectstore-server/src/endpoints/batch.rs @@ -317,6 +317,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()); diff --git a/objectstore-server/src/endpoints/common.rs b/objectstore-server/src/endpoints/common.rs index 4ee62d18..5e33b4e2 100644 --- a/objectstore-server/src/endpoints/common.rs +++ b/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, @@ -104,16 +100,25 @@ impl ApiError { ApiError::Service(ServiceError::NotImplemented) => StatusCode::NOT_IMPLEMENTED, 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. Errors whose + /// results never reach a response are captured in the service's `spawn_metered` instead. + pub fn capture(&self) { + 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() } diff --git a/objectstore-service/src/concurrency.rs b/objectstore-service/src/concurrency.rs index 93451085..639838bb 100644 --- a/objectstore-service/src/concurrency.rs +++ b/objectstore-service/src/concurrency.rs @@ -175,18 +175,19 @@ where .await .unwrap_or_else(|payload| Err(Error::panic(payload))); - if let Err(ref e) = result { - let error = e as &dyn std::error::Error; - objectstore_log::event_dyn!(e.level(), error, operation, "Task failed"); - } - objectstore_metrics::record!( "service.task.duration" = start.elapsed(), operation = operation, outcome = if result.is_ok() { "success" } else { "error" }, ); - let _ = tx.send(result); + // Errors delivered to the caller are captured once at the response boundary. If the + // receiver is gone, nothing downstream can observe the error, so capture it here. + if let Err(Err(ref e)) = tx.send(result) { + let error = e as &dyn std::error::Error; + objectstore_log::event_dyn!(e.level(), error, operation, "Task failed"); + } + drop(guard); transaction.finish(); drop(scope_guard); @@ -194,10 +195,7 @@ where .bind_hub(new_hub), ); - rx.await.map_err(|_| { - objectstore_log::error!(!!&Error::Dropped); - Error::Dropped - })? + rx.await.map_err(|_| Error::Dropped)? } #[cfg(test)] From db71ca592dc37c39583b97d9f53a6cf77acf5f90 Mon Sep 17 00:00:00 2001 From: Jan Michael Auer Date: Wed, 15 Jul 2026 17:43:59 +0200 Subject: [PATCH 3/4] ref: Use drop handler --- objectstore-server/src/endpoints/common.rs | 8 ++++++-- objectstore-service/src/concurrency.rs | 7 +------ objectstore-service/src/error.rs | 15 +++++++++++++++ objectstore-service/src/service.rs | 2 +- 4 files changed, 23 insertions(+), 9 deletions(-) diff --git a/objectstore-server/src/endpoints/common.rs b/objectstore-server/src/endpoints/common.rs index 5e33b4e2..40c34f1a 100644 --- a/objectstore-server/src/endpoints/common.rs +++ b/objectstore-server/src/endpoints/common.rs @@ -107,9 +107,13 @@ impl ApiError { /// 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. Errors whose - /// results never reach a response are captured in the service's `spawn_metered` instead. + /// response: standalone responses ([`IntoResponse`]) and batch response parts. pub fn capture(&self) { + // Tracked inside ServiceError's Drop implementation. + if matches!(self, ApiError::Service(_)) { + return; + } + if self.status().is_server_error() { objectstore_log::error!(!!self, "error handling request"); } diff --git a/objectstore-service/src/concurrency.rs b/objectstore-service/src/concurrency.rs index 639838bb..1e90022e 100644 --- a/objectstore-service/src/concurrency.rs +++ b/objectstore-service/src/concurrency.rs @@ -181,12 +181,7 @@ where outcome = if result.is_ok() { "success" } else { "error" }, ); - // Errors delivered to the caller are captured once at the response boundary. If the - // receiver is gone, nothing downstream can observe the error, so capture it here. - if let Err(Err(ref e)) = tx.send(result) { - let error = e as &dyn std::error::Error; - objectstore_log::event_dyn!(e.level(), error, operation, "Task failed"); - } + let _ = tx.send(result); drop(guard); transaction.finish(); diff --git a/objectstore-service/src/error.rs b/objectstore-service/src/error.rs index 9643dd43..4eaf64fa 100644 --- a/objectstore-service/src/error.rs +++ b/objectstore-service/src/error.rs @@ -215,5 +215,20 @@ impl Error { } } +impl Drop for Error { + /// Captures the error when it goes out of scope. + /// + /// Every error is dropped exactly once, at whatever point its journey ends — after being + /// serialized into a response, when a buffered result is discarded because the client + /// disconnected, or inside a task whose receiver is gone. Capturing here guarantees that + /// every service error is reported exactly once, without requiring instrumentation at any + /// individual call site. The log level is derived from [`level`](Self::level), so errors + /// that are expected to be handled (e.g. client errors) stay below capture severity. + fn drop(&mut self) { + let error = &*self as &dyn std::error::Error; + objectstore_log::event_dyn!(self.level(), error, "service error"); + } +} + /// Result type for service operations. pub type Result = std::result::Result; diff --git a/objectstore-service/src/service.rs b/objectstore-service/src/service.rs index 74479aff..a9f9095b 100644 --- a/objectstore-service/src/service.rs +++ b/objectstore-service/src/service.rs @@ -571,7 +571,7 @@ mod tests { let id = ObjectId::new(make_context(), "panic-test".into()); let result = service.get_object(id, None).await; - let Err(Error::Panic(msg)) = result else { + let Err(Error::Panic(ref msg)) = result else { panic!("expected Panic error"); }; assert!(msg.contains("intentional panic in get_object"), "{msg}"); From 9e131f3a1021c5ae7842f5c0a200709379f0efcb Mon Sep 17 00:00:00 2001 From: Jan Michael Auer Date: Thu, 16 Jul 2026 10:07:26 +0200 Subject: [PATCH 4/4] ref: Back to original approach --- objectstore-server/src/endpoints/batch.rs | 5 ++++- objectstore-server/src/endpoints/common.rs | 2 +- objectstore-service/src/concurrency.rs | 11 +++++++++-- objectstore-service/src/error.rs | 15 --------------- objectstore-service/src/service.rs | 2 +- 5 files changed, 15 insertions(+), 20 deletions(-) diff --git a/objectstore-server/src/endpoints/batch.rs b/objectstore-server/src/endpoints/batch.rs index 6f8d1981..11ff1091 100644 --- a/objectstore-server/src/endpoints/batch.rs +++ b/objectstore-server/src/endpoints/batch.rs @@ -232,7 +232,10 @@ async fn got_to_part( .meter_stream(stream, context) .try_collect::() .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| { diff --git a/objectstore-server/src/endpoints/common.rs b/objectstore-server/src/endpoints/common.rs index 40c34f1a..97468764 100644 --- a/objectstore-server/src/endpoints/common.rs +++ b/objectstore-server/src/endpoints/common.rs @@ -109,7 +109,7 @@ impl ApiError { /// 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) { - // Tracked inside ServiceError's Drop implementation. + // Captured at the source in the service layer to prevent double-logging. if matches!(self, ApiError::Service(_)) { return; } diff --git a/objectstore-service/src/concurrency.rs b/objectstore-service/src/concurrency.rs index 1e90022e..ac9bb416 100644 --- a/objectstore-service/src/concurrency.rs +++ b/objectstore-service/src/concurrency.rs @@ -175,6 +175,11 @@ where .await .unwrap_or_else(|payload| Err(Error::panic(payload))); + if let Err(ref e) = result { + let error = e as &dyn std::error::Error; + objectstore_log::event_dyn!(e.level(), error, operation, "Task failed"); + } + objectstore_metrics::record!( "service.task.duration" = start.elapsed(), operation = operation, @@ -182,7 +187,6 @@ where ); let _ = tx.send(result); - drop(guard); transaction.finish(); drop(scope_guard); @@ -190,7 +194,10 @@ 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)] diff --git a/objectstore-service/src/error.rs b/objectstore-service/src/error.rs index 4eaf64fa..9643dd43 100644 --- a/objectstore-service/src/error.rs +++ b/objectstore-service/src/error.rs @@ -215,20 +215,5 @@ impl Error { } } -impl Drop for Error { - /// Captures the error when it goes out of scope. - /// - /// Every error is dropped exactly once, at whatever point its journey ends — after being - /// serialized into a response, when a buffered result is discarded because the client - /// disconnected, or inside a task whose receiver is gone. Capturing here guarantees that - /// every service error is reported exactly once, without requiring instrumentation at any - /// individual call site. The log level is derived from [`level`](Self::level), so errors - /// that are expected to be handled (e.g. client errors) stay below capture severity. - fn drop(&mut self) { - let error = &*self as &dyn std::error::Error; - objectstore_log::event_dyn!(self.level(), error, "service error"); - } -} - /// Result type for service operations. pub type Result = std::result::Result; diff --git a/objectstore-service/src/service.rs b/objectstore-service/src/service.rs index a9f9095b..74479aff 100644 --- a/objectstore-service/src/service.rs +++ b/objectstore-service/src/service.rs @@ -571,7 +571,7 @@ mod tests { let id = ObjectId::new(make_context(), "panic-test".into()); let result = service.get_object(id, None).await; - let Err(Error::Panic(ref msg)) = result else { + let Err(Error::Panic(msg)) = result else { panic!("expected Panic error"); }; assert!(msg.contains("intentional panic in get_object"), "{msg}");