From c76bc4a9cc3ae5903e8137268528d697d76dfdbe Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Sun, 11 May 2025 11:35:31 +0200 Subject: [PATCH] fix: improve error display of nativets exceptions --- backend/windmill-common/src/error.rs | 2 + .../windmill-worker/src/graphql_executor.rs | 13 ++-- backend/windmill-worker/src/handle_child.rs | 2 +- backend/windmill-worker/src/js_eval.rs | 71 ++++++++++++++----- backend/windmill-worker/src/pg_executor.rs | 10 +-- .../windmill-worker/src/result_processor.rs | 1 + .../windmill-worker/src/snowflake_executor.rs | 5 +- 7 files changed, 73 insertions(+), 31 deletions(-) diff --git a/backend/windmill-common/src/error.rs b/backend/windmill-common/src/error.rs index feced2b6d5..a6d43a5fc4 100644 --- a/backend/windmill-common/src/error.rs +++ b/backend/windmill-common/src/error.rs @@ -66,6 +66,8 @@ pub enum Error { DatabaseMigration(#[from] MigrateError), #[error("Non-zero exit status for {0}: {1}")] ExitStatus(String, i32), + #[error("ExecutionRawError: {0}")] + ExecutionRawError(Box), #[error("Error: {error:#} @{location:#}")] Anyhow { error: anyhow::Error, location: String }, #[error("Error: {0:#?}")] diff --git a/backend/windmill-worker/src/graphql_executor.rs b/backend/windmill-worker/src/graphql_executor.rs index 5da8b2d193..1fbc547c30 100644 --- a/backend/windmill-worker/src/graphql_executor.rs +++ b/backend/windmill-worker/src/graphql_executor.rs @@ -1,6 +1,5 @@ use std::collections::HashMap; -use anyhow::anyhow; use futures::{stream, TryStreamExt}; use serde_json::{json, value::RawValue}; use sqlx::types::Json; @@ -134,11 +133,13 @@ pub async fn do_graphql( .map_err(|e| Error::ExecutionErr(e.to_string()))?; if let Some(errors) = result.errors { - return Err(anyhow!(errors - .into_iter() - .map(|x| x.message) - .collect::>() - .join("\n"),)); + return Err(Error::ExecutionErr( + errors + .into_iter() + .map(|x| x.message) + .collect::>() + .join("\n"), + )); } // And then check that we got back the same string we sent over. diff --git a/backend/windmill-worker/src/handle_child.rs b/backend/windmill-worker/src/handle_child.rs index 713046207f..6cf4272a58 100644 --- a/backend/windmill-worker/src/handle_child.rs +++ b/backend/windmill-worker/src/handle_child.rs @@ -526,7 +526,7 @@ pub async fn run_future_with_polling_update_job_poller( get_mem: S, ) -> error::Result where - Fut: Future>, + Fut: Future>, S: stream::Stream + Unpin, { let (tx, rx) = broadcast::channel::<()>(3); diff --git a/backend/windmill-worker/src/js_eval.rs b/backend/windmill-worker/src/js_eval.rs index 33a9b57690..5c9af6760b 100644 --- a/backend/windmill-worker/src/js_eval.rs +++ b/backend/windmill-worker/src/js_eval.rs @@ -789,7 +789,7 @@ pub async fn eval_fetch_timeout( w_id: &str, load_client: bool, occupation_metrics: &mut OccupancyMetrics, -) -> anyhow::Result> { +) -> windmill_common::error::Result> { use windmill_queue::append_logs; let (sender, mut receiver) = oneshot::channel::(); @@ -933,7 +933,7 @@ pub async fn eval_fetch_timeout( let r = runtime.block_on(future)?; // tracing::info!("total: {:?}", instant.elapsed()); - r + r as windmill_common::error::Result> }); let res = run_future_with_polling_update_job_poller( @@ -942,7 +942,7 @@ pub async fn eval_fetch_timeout( conn, mem_peak, canceled_by, - async { result_f.await? }, + async { result_f.await.map_err(windmill_common::error::to_anyhow)? }, worker_name, w_id, &mut Some(occupation_metrics), @@ -1004,22 +1004,26 @@ async fn eval_fetch( script_entrypoint_override: Option, load_client: bool, job_id: &Uuid, -) -> anyhow::Result> { +) -> windmill_common::error::Result> { if load_client { if let Some(env_code) = env_code.as_ref() { let _ = js_runtime .load_side_es_module_from_code( - &deno_core::resolve_url("file:///windmill.ts")?, + &deno_core::resolve_url("file:///windmill.ts").map_err(error::to_anyhow)?, format!("{env_code}\n{}", WINDMILL_CLIENT.to_string()), ) - .await?; + .await + .map_err(error::to_anyhow)?; } } use anyhow::Context; + use deno_core::error::CoreError; + use windmill_common::{error, worker::to_raw_value}; + let source = format!("{}\n{expr}", env_code.unwrap_or_default()); let _ = js_runtime .load_side_es_module_from_code( - &deno_core::resolve_url("file:///eval.ts")?, - format!("{}\n{expr}", env_code.unwrap_or_default()), + &deno_core::resolve_url("file:///eval.ts").map_err(error::to_anyhow)?, + source.to_string(), ) .await .map_err(|e| { @@ -1052,15 +1056,50 @@ import("file:///eval.ts").then((module) => module.{main_override}(...args)).then .map_err(|e| { write_error_expr(expr, &job_id); e - }) - .context("native script event loop")?; + }); - let scope = &mut js_runtime.handle_scope(); - let local = v8::Local::new(scope, global); - // Deserialize a `v8` object into a Rust type using `serde_v8`, - // in this case deserialize to a JSON `Value`. - let r = serde_v8::from_v8::>(scope, local)?; - Ok(unsafe_raw(r.unwrap_or_else(|| "null".to_string()))) + match global { + Ok(global) => { + let scope = &mut js_runtime.handle_scope(); + let local = v8::Local::new(scope, global); + // Deserialize a `v8` object into a Rust type using `serde_v8`, + // in this case deserialize to a JSON `Value`. + let r = serde_v8::from_v8::>(scope, local).map_err(error::to_anyhow)?; + Ok(unsafe_raw(r.unwrap_or_else(|| "null".to_string()))) + } + Err(CoreError::Js(e)) => { + let stack_head = e.frames.first().and_then(|f| { + if f.file_name.as_ref().is_some_and(|x| x == "file:///eval.ts") { + Some(format!( + "{}\n", + source + .lines() + .nth((f.line_number.unwrap_or(1)) as usize - 1) + .unwrap_or("") + .to_string() + )) + } else { + None + } + }); + let stack_s = format!( + "{}{}", + stack_head.unwrap_or("".to_string()), + e.stack.unwrap_or("".to_string()) + ); + let stack = if stack_s.is_empty() { + None + } else { + Some(stack_s) + }; + Err(Error::ExecutionRawError(to_raw_value(&serde_json::json!({ + "message": e.message, + "stack": stack, + "name": e.name, + })))) + } + Err(e) => Err(Error::ExecutionErr(e.print_with_cause())), + } } #[cfg(feature = "deno_core")] diff --git a/backend/windmill-worker/src/pg_executor.rs b/backend/windmill-worker/src/pg_executor.rs index 79002e4d1f..8cc97455c0 100644 --- a/backend/windmill-worker/src/pg_executor.rs +++ b/backend/windmill-worker/src/pg_executor.rs @@ -68,7 +68,7 @@ fn do_postgresql_inner<'a>( column_order: Option<&'a mut Option>>, siz: &'a AtomicUsize, skip_collect: bool, -) -> error::Result>>> { +) -> error::Result>>> { let mut query_params = vec![]; let arg_indices = parse_pg_statement_arg_indices(&query); @@ -136,17 +136,17 @@ fn do_postgresql_inner<'a>( if *CLOUD_HOSTED { let siz = siz.load(Ordering::Relaxed); if siz > MAX_RESULT_SIZE * 4 { - return Err(anyhow::anyhow!( + return Err(Error::ExecutionErr(format!( "Query result too large for cloud (size = {} > {})", siz, - MAX_RESULT_SIZE & 4 - )); + MAX_RESULT_SIZE & 4, + ))); } } if let Ok(v) = r { res.push(v); } else { - return Err(to_anyhow(r.err().unwrap())); + return Err(to_anyhow(r.err().unwrap()).into()); } } } diff --git a/backend/windmill-worker/src/result_processor.rs b/backend/windmill-worker/src/result_processor.rs index e1b2a763f6..0bde6fcdc6 100644 --- a/backend/windmill-worker/src/result_processor.rs +++ b/backend/windmill-worker/src/result_processor.rs @@ -373,6 +373,7 @@ pub async fn process_result( } } } + Error::ExecutionRawError(e) => to_raw_value(&e), err @ _ => to_raw_value(&SerializedError { message: format!("execution error:\n{err:#}",), name: "ExecutionErr".to_string(), diff --git a/backend/windmill-worker/src/snowflake_executor.rs b/backend/windmill-worker/src/snowflake_executor.rs index a4aba420c0..59ceccbc60 100644 --- a/backend/windmill-worker/src/snowflake_executor.rs +++ b/backend/windmill-worker/src/snowflake_executor.rs @@ -2,13 +2,12 @@ use base64::{engine, Engine as _}; use chrono::Datelike; use core::fmt::Write; use futures::future::BoxFuture; -use futures::{FutureExt, TryFutureExt}; +use futures::FutureExt; use jsonwebtoken::{encode, Algorithm, EncodingKey, Header}; use reqwest::{Client, Response}; use serde_json::{json, value::RawValue, Value}; use sha2::{Digest, Sha256}; use std::collections::HashMap; -use windmill_common::error::to_anyhow; use windmill_common::worker::Connection; use windmill_common::{error::Error, worker::to_raw_value}; @@ -428,7 +427,7 @@ pub async fn do_snowflake( conn, mem_peak, canceled_by, - result_f.map_err(to_anyhow), + result_f, worker_name, &job.workspace_id, &mut Some(occupancy_metrics),