From bf0703f0c1c8d0c3b72a6dfadc0f15b6aacbf42e Mon Sep 17 00:00:00 2001 From: Brandon Thomas Date: Thu, 2 Apr 2026 00:20:23 -0400 Subject: [PATCH] refactor web errors with cause + notification details --- .../src/notification/notification_details.rs | 3 +- .../notification_details_builder.rs | 16 ++++++-- .../src/jobs/alert_on_error.rs | 2 +- .../jobs/process_job/process_failed_job.rs | 4 +- .../src/job/alert_on_error.rs | 2 +- .../advanced_common_web_error.rs | 39 ++++++++++++++----- ...ce_2p0_multi_function_video_gen_handler.rs | 4 +- .../endpoints/webhooks/fal_webhook_handler.rs | 2 +- .../check_advanced_common_web_error.rs | 28 ++++++------- .../db_health_checker_thread.rs | 2 +- 10 files changed, 61 insertions(+), 41 deletions(-) diff --git a/crates/lib/pager/src/notification/notification_details.rs b/crates/lib/pager/src/notification/notification_details.rs index 7dc5c9ce18..c9aa176d29 100644 --- a/crates/lib/pager/src/notification/notification_details.rs +++ b/crates/lib/pager/src/notification/notification_details.rs @@ -1,7 +1,6 @@ use chrono::{DateTime, Utc}; use crate::notification::generate_deduplication_key::generate_deduplication_key; -use crate::notification::notification_details_builder::NotificationDetailsBuilder; use crate::notification::notification_urgency::NotificationUrgency; /// Details for a pager notification. @@ -20,7 +19,7 @@ pub struct NotificationDetails { pub(crate) event_time: DateTime, /// The error that triggered this notification, if any. - pub(crate) maybe_error: Option>, + pub(crate) maybe_error: Option>, /// Whether this notification originated from an error. pub(crate) is_from_error: bool, diff --git a/crates/lib/pager/src/notification/notification_details_builder.rs b/crates/lib/pager/src/notification/notification_details_builder.rs index 42ad88e19a..544c3516d7 100644 --- a/crates/lib/pager/src/notification/notification_details_builder.rs +++ b/crates/lib/pager/src/notification/notification_details_builder.rs @@ -1,5 +1,7 @@ -use chrono::{DateTime, Utc}; use std::fmt::{Debug, Display}; +use std::sync::Arc; + +use chrono::{DateTime, Utc}; use crate::notification::notification_details::NotificationDetails; use crate::notification::notification_urgency::NotificationUrgency; @@ -10,7 +12,7 @@ pub struct NotificationDetailsBuilder { pub(crate) urgency: Option, pub(crate) event_time: DateTime, - pub(crate) maybe_error: Option>, + pub(crate) maybe_error: Option>, pub(crate) is_from_error: bool, pub(crate) http_method: Option, @@ -47,7 +49,13 @@ impl NotificationDetailsBuilder { /// /// Sets the title to a placeholder and attaches the error object. /// Callers should chain `.set_title()` to provide a meaningful title. - pub fn from_error(error: Box) -> Self { + /// Create a builder from a boxed error. Converts to `Arc` internally for shareability. + pub fn from_boxed_error(error: Box) -> Self { + Self::from_error(Arc::from(error)) + } + + /// Create a builder from an `Arc`'d error. + pub fn from_error(error: Arc) -> Self { Self { title: "Notification from Error".to_string(), description: None, @@ -80,7 +88,7 @@ impl NotificationDetailsBuilder { self } - pub fn set_error(mut self, error: Option>) -> Self { + pub fn set_error(mut self, error: Option>) -> Self { self.maybe_error = error; self } diff --git a/crates/service/job/seedance2_pro_job/src/jobs/alert_on_error.rs b/crates/service/job/seedance2_pro_job/src/jobs/alert_on_error.rs index 2e9972e882..d95039e765 100644 --- a/crates/service/job/seedance2_pro_job/src/jobs/alert_on_error.rs +++ b/crates/service/job/seedance2_pro_job/src/jobs/alert_on_error.rs @@ -13,7 +13,7 @@ pub fn alert_pager_and_return_err( ) -> anyhow::Result { let err_message = format!("{:#}", err); - let mut builder = NotificationDetailsBuilder::from_error(err.into()) + let mut builder = NotificationDetailsBuilder::from_boxed_error(err.into()) .set_title(title.to_string()) .set_urgency(Some(NotificationUrgency::Medium)); diff --git a/crates/service/job/seedance2_pro_job/src/jobs/process_job/process_failed_job.rs b/crates/service/job/seedance2_pro_job/src/jobs/process_job/process_failed_job.rs index 12a1ac5288..5c5550c447 100644 --- a/crates/service/job/seedance2_pro_job/src/jobs/process_job/process_failed_job.rs +++ b/crates/service/job/seedance2_pro_job/src/jobs/process_job/process_failed_job.rs @@ -85,7 +85,7 @@ pub async fn process_failed_job( err, ); - let notification = NotificationDetailsBuilder::from_error(err.into()) + let notification = NotificationDetailsBuilder::from_boxed_error(err.into()) .set_title("Seedance2Pro refund failed".to_string()) .set_inference_job_token(Some(job.job_token.to_string())) .set_third_party_id(Some(job.order_id.to_string())) @@ -140,7 +140,7 @@ pub async fn process_failed_job( err ); - let notification = NotificationDetailsBuilder::from_error(err.into()) + let notification = NotificationDetailsBuilder::from_boxed_error(err.into()) .set_title("Seedance2Pro mark job failed error".to_string()) .set_urgency(Some(NotificationUrgency::Medium)) .build(); diff --git a/crates/service/job/video_thumbnail_job/src/job/alert_on_error.rs b/crates/service/job/video_thumbnail_job/src/job/alert_on_error.rs index d94870e015..655ab1643f 100644 --- a/crates/service/job/video_thumbnail_job/src/job/alert_on_error.rs +++ b/crates/service/job/video_thumbnail_job/src/job/alert_on_error.rs @@ -11,7 +11,7 @@ pub fn alert_pager_and_return_err( ) -> anyhow::Result { let err_message = format!("{:#}", err); - let notification = NotificationDetailsBuilder::from_error(err.into()) + let notification = NotificationDetailsBuilder::from_boxed_error(err.into()) .set_title(title.to_string()) .set_urgency(Some(NotificationUrgency::Medium)) .build(); diff --git a/crates/service/web/storyteller_web/src/http_server/common_responses/advanced_common_web_error.rs b/crates/service/web/storyteller_web/src/http_server/common_responses/advanced_common_web_error.rs index c68b761ab4..c76be592ee 100644 --- a/crates/service/web/storyteller_web/src/http_server/common_responses/advanced_common_web_error.rs +++ b/crates/service/web/storyteller_web/src/http_server/common_responses/advanced_common_web_error.rs @@ -1,6 +1,8 @@ +use std::fmt::{Display, Formatter}; +use std::sync::Arc; + use actix_http::StatusCode; use actix_web::{HttpResponse, HttpResponseBuilder, ResponseError}; -use std::fmt::{Display, Formatter}; use crate::http_server::web_utils::user_session::require_user_session::RequireUserSessionError; @@ -39,12 +41,20 @@ pub enum AdvancedCommonWebError { /// Uncaught errors are always 500 Internal Server Error. /// The user will never see the error cause or message, but our /// middleware will handle alerting, logging, etc. - UncaughtServerError(Box), + /// + /// Stored in `Arc` so the error alerting middleware can clone it for paging + /// without consuming the original. + UncaughtServerError(Arc), } // =============== Public accessors =============== impl AdvancedCommonWebError { + /// Wrap any error as an `UncaughtServerError`. + pub fn from_error(error: impl std::error::Error + Send + Sync + 'static) -> Self { + Self::UncaughtServerError(Arc::new(error)) + } + /// Extract the wrapped causal error (only present for `UncaughtServerError`). pub fn cause(&self) -> Option<&(dyn std::error::Error + Send + Sync + 'static)> { match self { @@ -53,6 +63,15 @@ impl AdvancedCommonWebError { } } + /// Clone the wrapped causal error as an `Arc` (only present for `UncaughtServerError`). + /// Useful for passing the error to the pager system without consuming the original. + pub fn clone_cause_arc(&self) -> Option> { + match self { + Self::UncaughtServerError(err) => Some(Arc::clone(err)), + _ => None, + } + } + /// Whether this is a server error (500). pub fn is_server_error(&self) -> bool { matches!(self, Self::UncaughtServerError(_)) @@ -136,19 +155,21 @@ impl ResponseError for AdvancedCommonWebError { impl From for AdvancedCommonWebError { fn from(err: sqlx::Error) -> Self { - Self::UncaughtServerError(Box::new(err)) + Self::from_error(err) } } impl From for AdvancedCommonWebError { fn from(err: anyhow::Error) -> Self { - Self::UncaughtServerError(err.into()) + // anyhow::Error doesn't impl std::error::Error, so we go through Box -> Arc. + let boxed: Box = err.into(); + Self::UncaughtServerError(Arc::from(boxed)) } } impl From for AdvancedCommonWebError { fn from(err: serde_json::Error) -> Self { - Self::UncaughtServerError(Box::new(err)) + Self::from_error(err) } } @@ -156,9 +177,7 @@ impl From for AdvancedCommonWebError { fn from(value: RequireUserSessionError) -> Self { match value { RequireUserSessionError::NotAuthorized => Self::NotAuthorized, - RequireUserSessionError::ServerError => { - Self::UncaughtServerError(Box::new(value)) - } + RequireUserSessionError::ServerError => Self::from_error(value), } } } @@ -223,7 +242,7 @@ mod tests { #[test] fn uncaught_io_error_returns_500_and_hides_cause() { let io_err = std::io::Error::new(std::io::ErrorKind::Other, "disk exploded"); - let error = AdvancedCommonWebError::UncaughtServerError(Box::new(io_err)); + let error = AdvancedCommonWebError::from_error(io_err); assert_eq!(error.status_code(), StatusCode::INTERNAL_SERVER_ERROR); assert!(error.is_server_error()); @@ -297,7 +316,7 @@ mod tests { #[test] fn error_source_returns_wrapped_error() { let io_err = std::io::Error::new(std::io::ErrorKind::Other, "root cause"); - let error = AdvancedCommonWebError::UncaughtServerError(Box::new(io_err)); + let error = AdvancedCommonWebError::from_error(io_err); let source = std::error::Error::source(&error); assert!(source.is_some()); diff --git a/crates/service/web/storyteller_web/src/http_server/endpoints/generate/video/multi_function/seedance_2p0_multi_function_video_gen_handler.rs b/crates/service/web/storyteller_web/src/http_server/endpoints/generate/video/multi_function/seedance_2p0_multi_function_video_gen_handler.rs index a01f9dd88f..a808855e59 100644 --- a/crates/service/web/storyteller_web/src/http_server/endpoints/generate/video/multi_function/seedance_2p0_multi_function_video_gen_handler.rs +++ b/crates/service/web/storyteller_web/src/http_server/endpoints/generate/video/multi_function/seedance_2p0_multi_function_video_gen_handler.rs @@ -222,7 +222,7 @@ pub async fn seedance_2p0_multi_function_video_gen_handler( Err(err) => { warn!("Regular session fallback also failed for user {:?}: {:?}", user_token, err); if let Err(page_err) = server_state.pager.enqueue_page( - NotificationDetailsBuilder::from_error(err.into()) + NotificationDetailsBuilder::from_boxed_error(err.into()) .set_title("Seedance 2.0 generation failed (whitelist + fallback)".to_string()) .set_urgency(Some(NotificationUrgency::High)) .build() @@ -256,7 +256,7 @@ pub async fn seedance_2p0_multi_function_video_gen_handler( Err(err) => { warn!("Error calling seedance2pro generate_video: {:?}", err); if let Err(page_err) = server_state.pager.enqueue_page( - NotificationDetailsBuilder::from_error(err.into()) + NotificationDetailsBuilder::from_boxed_error(err.into()) .set_title("Seedance 2.0 generation failed".to_string()) .set_urgency(Some(NotificationUrgency::High)) .build() diff --git a/crates/service/web/storyteller_web/src/http_server/endpoints/webhooks/fal_webhook_handler.rs b/crates/service/web/storyteller_web/src/http_server/endpoints/webhooks/fal_webhook_handler.rs index 2866394c5e..bd0a8535c7 100644 --- a/crates/service/web/storyteller_web/src/http_server/endpoints/webhooks/fal_webhook_handler.rs +++ b/crates/service/web/storyteller_web/src/http_server/endpoints/webhooks/fal_webhook_handler.rs @@ -129,7 +129,7 @@ pub async fn fal_webhook_handler( request_id, err, raw_body, ); - let notification = NotificationDetailsBuilder::from_error(err.into()) + let notification = NotificationDetailsBuilder::from_boxed_error(err.into()) .set_title("FAL webhook processing failed".to_string()) .set_description(Some(description)) .set_third_party_id(Some(request_id.to_string())) diff --git a/crates/service/web/storyteller_web/src/http_server/middleware/error_alerting_middleware/check_advanced_common_web_error.rs b/crates/service/web/storyteller_web/src/http_server/middleware/error_alerting_middleware/check_advanced_common_web_error.rs index e848456440..211c6c7bee 100644 --- a/crates/service/web/storyteller_web/src/http_server/middleware/error_alerting_middleware/check_advanced_common_web_error.rs +++ b/crates/service/web/storyteller_web/src/http_server/middleware/error_alerting_middleware/check_advanced_common_web_error.rs @@ -21,29 +21,23 @@ pub(super) fn check_advanced_common_web_error( let title = format!("UncaughtServerError on {} {}", method, path); - let description = if let Some(cause) = error.cause() { - format!( - "An UncaughtServerError was returned with a wrapped cause.\n\n\ - Endpoint: {} {}\n\ - Cause: {}\n\ - Cause (debug): {:?}", - method, path, cause, cause, - ) + // Clone the Arc'd causal error so the notification owns a reference to it. + let cause_arc = error.clone_cause_arc(); + + let mut builder = if let Some(arc_err) = cause_arc { + NotificationDetailsBuilder::from_error(arc_err) + .set_title(title) } else { - format!( - "An UncaughtServerError was returned (no wrapped cause).\n\n\ - Endpoint: {} {}", - method, path, - ) + NotificationDetailsBuilder::from_title(title) }; - let notification = NotificationDetailsBuilder::from_title(title) - .set_description(Some(description)) + builder = builder .set_http_method(Some(method.to_string())) .set_http_path(Some(path.to_string())) .set_http_status_code(Some(500)) - .set_urgency(Some(NotificationUrgency::Medium)) - .build(); + .set_urgency(Some(NotificationUrgency::Medium)); + + let notification = builder.build(); if let Err(err) = pager.enqueue_page(notification) { warn!("Error alerting middleware: failed to enqueue page: {:?}", err); diff --git a/crates/service/web/storyteller_web/src/threads/db_health_checker_thread/db_health_checker_thread.rs b/crates/service/web/storyteller_web/src/threads/db_health_checker_thread/db_health_checker_thread.rs index 7e0a02b4bb..d4bd591bfd 100644 --- a/crates/service/web/storyteller_web/src/threads/db_health_checker_thread/db_health_checker_thread.rs +++ b/crates/service/web/storyteller_web/src/threads/db_health_checker_thread/db_health_checker_thread.rs @@ -35,7 +35,7 @@ pub async fn db_health_checker_thread( Ok(_) => {}, } - let notification = NotificationDetailsBuilder::from_error(database_error.into()) + let notification = NotificationDetailsBuilder::from_boxed_error(database_error.into()) .set_title("DB health check thread failed check".to_string()) .set_urgency(Some(NotificationUrgency::High)) .build();