Print server side messages, if sent by the RE

Vasko Mitanov · vaskomitanov · commit 2a0ee7d45a94 · 2024-09-12T20:12:37.000-07:00
diff --git 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,46 @@ async fn build_action_no_redirect(
         },
         spans,
     })?;
-
     action_execution_data.action_result
 }
 
+fn build_error_diagnostics(execute_error: &ExecuteError, error_diagnostics: Option<ActionErrorDiagnostics>) -> Option<ActionErrorDiagnostics> {
+    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 mut sub_errors = action_sub_errors.sub_errors.clone();
+                    sub_errors.push(action_sub_error());
+                    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
@@ -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
@@ -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
@@ -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<Line>) {
+        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
@@ -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
@@ -196,6 +196,7 @@ pub struct CommandExecutionResult {
     /// to be re-used when uploading the remote dep file.
     #[derivative(Debug = "ignore")]
     pub action_result: Option<TActionResult2>,
+    pub server_message: Option<String>,
 }
 
 impl CommandExecutionResult {
diff --git 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
@@ -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
@@ -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)]

Original file line number	Diff line number	Diff line change
`@@ -143,6 +143,7 @@ impl CommandExecutionManagerLike for CommandExecutionManager {`
`143`	`143`	`eligible_for_full_hybrid: false,`
`144`	`144`	`dep_file_metadata: None,`
`145`	`145`	`action_result: None,`
	`146`	`+ server_message: None,`
`146`	`147`	`}`
`147`	`148`	`}`
`148`	`149`
`@@ -223,6 +224,7 @@ impl CommandExecutionManagerLike for CommandExecutionManagerWithClaim {`
`223`	`224`	`eligible_for_full_hybrid: false,`
`224`	`225`	`dep_file_metadata: None,`
`225`	`226`	`action_result: None,`
	`227`	`+ server_message: None,`
`226`	`228`	`}`
`227`	`229`	`}`
`228`	`230`
Original file line number	Diff line number	Diff line change
`@@ -196,6 +196,7 @@ pub struct CommandExecutionResult {`
`196`	`196`	`/// to be re-used when uploading the remote dep file.`
`197`	`197`	`#[derivative(Debug = "ignore")]`
`198`	`198`	`pub action_result: Option<TActionResult2>,`
	`199`	`+ pub server_message: Option<String>,`
`199`	`200`	`}`
`200`	`201`
`201`	`202`	`impl CommandExecutionResult {`
Original file line number	Diff line number	Diff line change
`@@ -176,6 +176,7 @@ pub struct ExecuteResponse {`
`176`	`176`	`pub action_digest: TDigest,`
`177`	`177`	`pub action_result_digest: TDigest,`
`178`	`178`	`pub action_result_ttl: i64,`
	`179`	`+ pub message: String,`
`179`	`180`	`}`
`180`	`181`
`181`	`182`	`#[derive(Clone, Default)]`