diff --git a/app/buck2_build_api/src/actions/calculation.rs b/app/buck2_build_api/src/actions/calculation.rs index 42e6e71eb5f7..a9789f02f86c 100644 --- a/app/buck2_build_api/src/actions/calculation.rs +++ b/app/buck2_build_api/src/actions/calculation.rs @@ -21,7 +21,7 @@ use buck2_build_signals::NodeDuration; use buck2_common::events::HasEvents; use buck2_core::base_deferred_key::BaseDeferredKey; use buck2_core::target::configured_target_label::ConfiguredTargetLabel; -use buck2_data::ActionErrorDiagnostics; +use buck2_data::{action_error_diagnostics, ActionErrorDiagnostics}; use buck2_data::ActionSubErrors; use buck2_data::ToProtoMessage; use buck2_events::dispatch::async_record_root_spans; @@ -57,6 +57,7 @@ use crate::actions::error_handler::ActionSubErrorResult; use crate::actions::error_handler::StarlarkActionErrorContext; use crate::actions::execute::action_executor::ActionOutputs; use crate::actions::execute::action_executor::HasActionExecutor; +use crate::actions::execute::error::ExecuteError; use crate::actions::RegisteredAction; use crate::artifact_groups::calculation::ensure_artifact_group_staged; use crate::deferred::calculation::lookup_deferred_holder; @@ -255,7 +256,10 @@ async fn build_action_no_redirect( let last_command = commands.last().cloned(); - let error_diagnostics = try_run_error_handler(action.dupe(), last_command.as_ref()); + let error_diagnostics = build_error_diagnostics( + &e, + try_run_error_handler(action.dupe(), last_command.as_ref()) + ); let e = ActionError::new( e, @@ -357,10 +361,50 @@ async fn build_action_no_redirect( }, spans, })?; - action_execution_data.action_result } +fn build_error_diagnostics(execute_error: &ExecuteError, error_diagnostics: Option) -> Option { + let ExecuteError::CommandExecutionError { error: Some(command_execute_error) } = execute_error else { + return error_diagnostics; + }; + let action_sub_error = || buck2_data::ActionSubError { + category: "ServerMessage".to_string(), + message: Some(format!("{}", command_execute_error)), + locations: None, + }; + if let Some(inner_error_diagnostics) = &error_diagnostics { + if let Some(error_diagnostics_data) = &inner_error_diagnostics.data { + match error_diagnostics_data { + action_error_diagnostics::Data::SubErrors(action_sub_errors) => { + let sub_errors = action_sub_errors + .sub_errors + .iter() + .chain([&action_sub_error()]) + .cloned() + .collect(); + Some(ActionErrorDiagnostics { + data: Some(action_error_diagnostics::Data::SubErrors(ActionSubErrors { sub_errors })), + }) + }, + action_error_diagnostics::Data::HandlerInvocationError(handler_error) => { + Some(ActionErrorDiagnostics { + data: Some(action_error_diagnostics::Data::HandlerInvocationError(format!("{}\n{}", handler_error, command_execute_error))), + }) + }, + } + } else { + Some(ActionErrorDiagnostics { + data: Some(action_error_diagnostics::Data::SubErrors(ActionSubErrors { sub_errors: vec![action_sub_error()] })), + }) + } + } else { + Some(ActionErrorDiagnostics { + data: Some(action_error_diagnostics::Data::SubErrors(ActionSubErrors { sub_errors: vec![action_sub_error()] })), + }) + } +} + // Attempt to run the error handler if one was specified. Returns either the error diagnostics, or // an actual error if the handler failed to run successfully. fn try_run_error_handler( diff --git a/app/buck2_build_api/src/actions/error.rs b/app/buck2_build_api/src/actions/error.rs index 12bed980a093..5a54c8d404f7 100644 --- a/app/buck2_build_api/src/actions/error.rs +++ b/app/buck2_build_api/src/actions/error.rs @@ -8,7 +8,6 @@ */ use std::fmt; - use buck2_event_observer::display::display_action_error; use buck2_event_observer::display::TargetDisplayOptions; diff --git a/app/buck2_build_api/src/actions/execute/action_executor.rs b/app/buck2_build_api/src/actions/execute/action_executor.rs index 4fbe7e7e9f82..1864ba2e6ed5 100644 --- a/app/buck2_build_api/src/actions/execute/action_executor.rs +++ b/app/buck2_build_api/src/actions/execute/action_executor.rs @@ -462,7 +462,15 @@ impl ActionExecutionCtx for BuckActionExecutionContext<'_> { error: Some(error.clone()), }) } - _ => Err(ExecuteError::CommandExecutionError { error: None }), + _ => { + #[derive(Display, Debug, thiserror::Error)] + struct ServerError { + message: String, + } + Err(ExecuteError::CommandExecutionError { error: result.server_message.map(|server_message| buck2_error::Error::new(ServerError{ + message: server_message, + })) }) + }, }; self.command_reports.extend(rejected_execution); self.command_reports.push(report); diff --git a/app/buck2_client_ctx/src/subscribers/superconsole.rs b/app/buck2_client_ctx/src/subscribers/superconsole.rs index 56a4b675b6fe..58be65db1b25 100644 --- a/app/buck2_client_ctx/src/subscribers/superconsole.rs +++ b/app/buck2_client_ctx/src/subscribers/superconsole.rs @@ -15,7 +15,7 @@ use std::time::Duration; use anyhow::Context as _; use async_trait::async_trait; -use buck2_data::CommandExecutionDetails; +use buck2_data::{action_error_diagnostics, ActionError, ActionErrorDiagnostics, CommandExecutionDetails}; use buck2_event_observer::display; use buck2_event_observer::display::display_file_watcher_end; use buck2_event_observer::display::TargetDisplayOptions; @@ -621,7 +621,7 @@ impl StatefulSuperConsoleImpl { async fn handle_action_error(&mut self, error: &buck2_data::ActionError) -> anyhow::Result<()> { let mut lines = vec![]; let display_platform = self.state.config.display_platform; - + Self::write_diagnostics(&error, &mut lines); let display::ActionErrorDisplay { action_id, reason, @@ -650,12 +650,39 @@ impl StatefulSuperConsoleImpl { if let Some(command) = command { lines_for_command_details(&command, self.verbosity, &mut lines); } - self.super_console.emit(Lines(lines)); - Ok(()) } + fn write_diagnostics(error: &ActionError, lines: &mut Vec) { + if let Some(ActionErrorDiagnostics {data: Some(data)} ) = &error.error_diagnostics { + match data { + action_error_diagnostics::Data::SubErrors(action_sub_errors) => { + for sub_error in &action_sub_errors.sub_errors { + if let Some(message) = &sub_error.message { + lines.push(Line::from_iter([Span::new_styled_lossy(StyledContent::new( + ContentStyle { + foreground_color: Some(Color::Yellow), + ..Default::default() + }, + format!("{}: {:?}", sub_error.category, message), + ))])); + } + } + }, + action_error_diagnostics::Data::HandlerInvocationError(handler_error) => { + lines.push(Line::from_iter([Span::new_styled_lossy(StyledContent::new( + ContentStyle { + foreground_color: Some(Color::Yellow), + ..Default::default() + }, + format!("{:?}", handler_error), + ))])); + } + } + } + } + async fn handle_test_result(&mut self, result: &buck2_data::TestResult) -> anyhow::Result<()> { if let Some(msg) = display::format_test_result(result)? { self.super_console.emit(msg); diff --git a/app/buck2_execute/src/execute/manager.rs b/app/buck2_execute/src/execute/manager.rs index 4381d1591f4c..6b8e5b626c63 100644 --- a/app/buck2_execute/src/execute/manager.rs +++ b/app/buck2_execute/src/execute/manager.rs @@ -143,6 +143,7 @@ impl CommandExecutionManagerLike for CommandExecutionManager { eligible_for_full_hybrid: false, dep_file_metadata: None, action_result: None, + server_message: None, } } @@ -223,6 +224,7 @@ impl CommandExecutionManagerLike for CommandExecutionManagerWithClaim { eligible_for_full_hybrid: false, dep_file_metadata: None, action_result: None, + server_message: None, } } diff --git a/app/buck2_execute/src/execute/result.rs b/app/buck2_execute/src/execute/result.rs index 9b4521e9ee6e..f8a191ce58dd 100644 --- a/app/buck2_execute/src/execute/result.rs +++ b/app/buck2_execute/src/execute/result.rs @@ -196,6 +196,7 @@ pub struct CommandExecutionResult { /// to be re-used when uploading the remote dep file. #[derivative(Debug = "ignore")] pub action_result: Option, + pub server_message: Option, } impl CommandExecutionResult { diff --git a/app/buck2_execute_impl/src/executors/re.rs b/app/buck2_execute_impl/src/executors/re.rs index 45ed83ef7f1b..0d0bcb5287d4 100644 --- a/app/buck2_execute_impl/src/executors/re.rs +++ b/app/buck2_execute_impl/src/executors/re.rs @@ -189,9 +189,9 @@ impl ReExecutor { platform: platform.clone(), remote_dep_file_key: None, }; - let execution_kind = response.execution_kind(remote_details); let manager = manager.with_execution_kind(execution_kind.clone()); + if response.status.code != TCode::OK { let res = if let Some(out) = as_missing_outputs_error(&response.status) { // TODO: Add a dedicated report variant for this. @@ -236,7 +236,6 @@ impl ReExecutor { error_type, ) }; - return ControlFlow::Break(res); } @@ -357,9 +356,9 @@ impl PreparedCommandExecutor for ReExecutor { ) .boxed() .await; - let DownloadResult::Result(mut res) = res; res.action_result = Some(response.action_result); + res.server_message = if response.message.is_empty() { None } else { Some(response.message) }; res } diff --git a/remote_execution/oss/re_grpc/src/client.rs b/remote_execution/oss/re_grpc/src/client.rs index 3c5b8a3b941c..b53dca66ee0d 100644 --- a/remote_execution/oss/re_grpc/src/client.rs +++ b/remote_execution/oss/re_grpc/src/client.rs @@ -664,6 +664,7 @@ impl REClient { }, cached_result: execute_response_grpc.cached_result, action_digest: Default::default(), // Filled in below. + message: execute_response_grpc.message, }; ExecuteWithProgressResponse { diff --git a/remote_execution/oss/re_grpc/src/response.rs b/remote_execution/oss/re_grpc/src/response.rs index 7c773c01da51..d7057ce10cbf 100644 --- a/remote_execution/oss/re_grpc/src/response.rs +++ b/remote_execution/oss/re_grpc/src/response.rs @@ -176,6 +176,7 @@ pub struct ExecuteResponse { pub action_digest: TDigest, pub action_result_digest: TDigest, pub action_result_ttl: i64, + pub message: String, } #[derive(Clone, Default)]