From 832d8f9a61beac8a010d9e0cd49e820edd2db926 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Wed, 26 Aug 2026 20:54:37 +0000 Subject: [PATCH] fix: keep the thrown value identifiable in the bun and deno error wrappers Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Jb7qAQ3vnyhsbPZk7Qi7ho --- backend/tests/bun_jobs.rs | 75 ++++++++++++++++++ backend/tests/worker.rs | 69 +++++++++++++++++ backend/windmill-worker/src/bun_executor.rs | 45 ++++------- backend/windmill-worker/src/common.rs | 80 ++++++++++++++++++++ backend/windmill-worker/src/deno_executor.rs | 14 ++-- 5 files changed, 246 insertions(+), 37 deletions(-) diff --git a/backend/tests/bun_jobs.rs b/backend/tests/bun_jobs.rs index fc766253bc..a4ce734f53 100644 --- a/backend/tests/bun_jobs.rs +++ b/backend/tests/bun_jobs.rs @@ -364,6 +364,81 @@ export function main() { Ok(()) } +/// A script can throw a non-object, or an object with a reference cycle. The error the +/// job reports must still identify what was thrown, never the wrapper's own failure to +/// read or serialize it. +#[sqlx::test(fixtures("base"))] +async fn test_bun_job_non_error_throws(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + + fn bun_job(content: &str) -> JobPayload { + JobPayload::Code(RawCode { + hash: None, + content: content.to_owned(), + path: None, + language: ScriptLang::Bun, + lock: None, + concurrency_settings: windmill_common::runnable_settings::ConcurrencySettings::default( + ) + .into(), + debouncing_settings: windmill_common::runnable_settings::DebouncingSettings::default(), + cache_ttl: None, + cache_ignore_s3_path: None, + dedicated_worker: None, + modules: None, + tag: None, + }) + } + + // `throw null`: the wrapper must not dereference the thrown value. + { + let job = bun_job("export function main() { throw null; }"); + let completed = run_job_in_new_worker_until_complete(&db, false, job, port).await; + assert!(!completed.success); + let result = completed.json_result().unwrap(); + assert_eq!(result["error"]["message"], serde_json::json!("null")); + } + + // Circular object: several HTTP clients throw errors that link two objects + // both ways, which a plain JSON.stringify cannot serialize. + { + let job = bun_job( + r#" +export function main() { + const a: any = { name: "circular", message: "boom" }; + a.self = a; + throw a; +} +"#, + ); + let completed = run_job_in_new_worker_until_complete(&db, false, job, port).await; + assert!(!completed.success); + let result = completed.json_result().unwrap(); + let error = &result["error"]; + assert_eq!(error["message"], serde_json::json!("boom")); + assert_eq!( + error["extra"]["self"]["self"], + serde_json::json!("[Circular]") + ); + } + + // A thrown string is a string, not an index map of its characters. + { + let job = bun_job(r#"export function main() { throw "nur ein string"; }"#); + let completed = run_job_in_new_worker_until_complete(&db, false, job, port).await; + assert!(!completed.success); + let result = completed.json_result().unwrap(); + assert_eq!( + result["error"]["message"], + serde_json::json!("nur ein string") + ); + } + + Ok(()) +} + #[sqlx::test(fixtures("base"))] async fn test_bun_job_missing_main_function(db: Pool) -> anyhow::Result<()> { initialize_tracing().await; diff --git a/backend/tests/worker.rs b/backend/tests/worker.rs index 419be6fdf3..168b9dc875 100644 --- a/backend/tests/worker.rs +++ b/backend/tests/worker.rs @@ -3240,6 +3240,75 @@ export async function main(a: Date) { Ok(()) } +/// A script can throw a non-object. The error the job reports must still identify what +/// was thrown, never the wrapper's own failure to read it. +#[sqlx::test(fixtures("base"))] +async fn test_deno_job_non_error_throws(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + + let deno_job = |content: &str| { + JobPayload::Code(RawCode { + hash: None, + content: content.to_owned(), + path: None, + lock: None, + language: ScriptLang::Deno, + cache_ttl: None, + cache_ignore_s3_path: None, + dedicated_worker: None, + concurrency_settings: windmill_common::runnable_settings::ConcurrencySettings::default( + ) + .into(), + debouncing_settings: windmill_common::runnable_settings::DebouncingSettings::default(), + modules: None, + tag: None, + }) + }; + + // `throw null`: the wrapper must not dereference the thrown value. + let job = RunJob::from(deno_job("export function main() { throw null; }")) + .run_until_complete(&db, false, port) + .await; + assert!(!job.success); + assert_eq!( + job.json_result().unwrap()["error"]["message"], + json!("null") + ); + + // A thrown string reaches the result as a string; reading `message` off it yields + // nothing, which used to leave an empty error object. + let job = RunJob::from(deno_job( + r#"export function main() { throw "nur ein string"; }"#, + )) + .run_until_complete(&db, false, port) + .await; + assert!(!job.success); + assert_eq!( + job.json_result().unwrap()["error"]["message"], + json!("nur ein string") + ); + + // Deno reports only message/name/stack. Collecting the thrown value's own properties + // would persist whatever an HTTP client hangs off its errors, auth headers included. + let job = RunJob::from(deno_job( + r#" +export function main() { + throw Object.assign(new Error("boom"), { authorization: "Bearer secret" }); +} +"#, + )) + .run_until_complete(&db, false, port) + .await; + assert!(!job.success); + let error = job.json_result().unwrap()["error"].clone(); + assert_eq!(error["message"], json!("boom")); + assert!(error.get("extra").is_none(), "unexpected extra: {error}"); + + Ok(()) +} + /// Test that full .npmrc content works for deno jobs with private registries. /// Requires: /// - `TEST_NPMRC` environment variable set to the full .npmrc content diff --git a/backend/windmill-worker/src/bun_executor.rs b/backend/windmill-worker/src/bun_executor.rs index 1fb566e57f..76e758de12 100644 --- a/backend/windmill-worker/src/bun_executor.rs +++ b/backend/windmill-worker/src/bun_executor.rs @@ -22,7 +22,7 @@ use crate::{ build_command_with_isolation, create_args_and_out_file, get_reserved_variables, parse_npm_config, read_file, read_file_content, read_result, resolve_nsjail_timeout, resolve_nsjail_tmp_mount_block, start_child_process, write_file_binary, MaybeLock, - OccupancyMetrics, StreamNotifier, DEV_CONF_NSJAIL, + OccupancyMetrics, StreamNotifier, DEV_CONF_NSJAIL, JS_ERROR_SERIALIZER, }, get_proxy_envs_for_lang, handle_child::handle_child, @@ -235,6 +235,7 @@ import * as Readline from "node:readline" BigInt.prototype.toJSON = function () {{ return this.toString(); }}; +{JS_ERROR_SERIALIZER} const scripts = new Map(); {functions} @@ -266,7 +267,7 @@ for await (const line of Readline.createInterface({{ input: process.stdin }})) { const res = await entry.module.main(...mainArgs); console.log("wm_res[success]:" + JSON.stringify(res ?? null, (key, value) => typeof value === 'undefined' ? null : value)); }} catch (e) {{ - console.log("wm_res[error]:" + JSON.stringify({{ message: e.message, name: e.name, stack: e.stack, line: argsJson }})); + console.log("wm_res[error]:" + wmSerializeError(Object.assign(wmToErrorObject(e, false), {{ line: argsJson }}))); }} continue; }} @@ -280,7 +281,7 @@ for await (const line of Readline.createInterface({{ input: process.stdin }})) { const res = await entry.module.main(...args); console.log("wm_res[success]:" + JSON.stringify(res ?? null, (key, value) => typeof value === 'undefined' ? null : value)); }} catch (e) {{ - console.log("wm_res[error]:" + JSON.stringify({{ message: e.message, name: e.name, stack: e.stack, line: argsJson }})); + console.log("wm_res[error]:" + wmSerializeError(Object.assign(wmToErrorObject(e, false), {{ line: argsJson }}))); }} continue; }} @@ -314,7 +315,7 @@ for await (const line of Readline.createInterface({{ input: process.stdin }})) { const res = await entry.module.main(...mainArgs); console.log("wm_res[success]:" + JSON.stringify(res ?? null, (key, value) => typeof value === 'undefined' ? null : value)); }} catch (e) {{ - console.log("wm_res[error]:" + JSON.stringify({{ message: e.message, name: e.name, stack: e.stack, line: argsJson }})); + console.log("wm_res[error]:" + wmSerializeError(Object.assign(wmToErrorObject(e, false), {{ line: argsJson }}))); }} continue; }} @@ -340,7 +341,7 @@ for await (const line of Readline.createInterface({{ input: process.stdin }})) { const res = await entry.module.main(...args); console.log("wm_res[success]:" + JSON.stringify(res ?? null, (key, value) => typeof value === 'undefined' ? null : value)); }} catch (e) {{ - console.log("wm_res[error]:" + JSON.stringify({{ message: e.message, name: e.name, stack: e.stack, line: argsJson }})); + console.log("wm_res[error]:" + wmSerializeError(Object.assign(wmToErrorObject(e, false), {{ line: argsJson }}))); }} continue; }} @@ -1912,6 +1913,7 @@ function argsObjToArr({{ {spread} }}) {{ BigInt.prototype.toJSON = function () {{ return this.toString(); }}; +{JS_ERROR_SERIALIZER} // Find the workflow entrypoint (export default) let workflowFn = Main.default; @@ -1990,22 +1992,12 @@ try {{ process.exit(0); }} catch(e) {{ console.error(e); - let err = {{ message: e.message, name: e.name, stack: e.stack }}; - let step_id = process.env.WM_FLOW_STEP_ID; + const err = wmToErrorObject(e, true); + const step_id = process.env.WM_FLOW_STEP_ID; if (step_id) {{ err["step_id"] = step_id; }} - const extra = {{}}; - Object.getOwnPropertyNames(e).forEach((key) => {{ - if (['line', 'name', 'stack', 'column', 'message', 'sourceURL', 'originalLine', 'originalColumn'].includes(key)) {{ - return; - }} - extra[key] = e[key]; - }}); - if (Object.keys(extra).length > 0) {{ - err["extra"] = extra; - }} - await fs.writeFile("result.json", JSON.stringify(err)); + await fs.writeFile("result.json", wmSerializeError(err)); process.exit(1); }} "#, @@ -2030,6 +2022,7 @@ function isAsyncIterable(obj) {{ BigInt.prototype.toJSON = function () {{ return this.toString(); }}; +{JS_ERROR_SERIALIZER} async function run() {{ {dates} @@ -2056,22 +2049,12 @@ try {{ await run(); }} catch(e) {{ console.error(e); - let err = {{ message: e.message, name: e.name, stack: e.stack }}; - let step_id = process.env.WM_FLOW_STEP_ID; + const err = wmToErrorObject(e, true); + const step_id = process.env.WM_FLOW_STEP_ID; if (step_id) {{ err["step_id"] = step_id; }} - const extra = {{}}; - Object.getOwnPropertyNames(e).forEach((key) => {{ - if (['line', 'name', 'stack', 'column', 'message', 'sourceURL', 'originalLine', 'originalColumn'].includes(key)) {{ - return; - }} - extra[key] = e[key]; - }}); - if (Object.keys(extra).length > 0) {{ - err["extra"] = extra; - }} - await fs.writeFile("result.json", JSON.stringify(err)); + await fs.writeFile("result.json", wmSerializeError(err)); process.exit(1); }} "#, diff --git a/backend/windmill-worker/src/common.rs b/backend/windmill-worker/src/common.rs index 6ee22fd882..02fca6e6d1 100644 --- a/backend/windmill-worker/src/common.rs +++ b/backend/windmill-worker/src/common.rs @@ -67,6 +67,86 @@ mount { #[cfg(not(debug_assertions))] pub const DEV_CONF_NSJAIL: &str = ""; +/// Helpers the Bun/Deno wrappers use to turn a thrown value into the error object +/// they report, injected into the generated wrapper source. +/// +/// A script can throw anything, so neither reading `e.message` nor calling +/// `JSON.stringify` on the result is safe: `throw null`, and errors that link two +/// objects both ways (an axios error hangs `response` off `request` and back), both +/// abort the wrapper itself, and the job then reports a stack trace from the wrapper +/// instead of what the script threw. +/// +/// `collectExtra` is true only where the wrapper already reported the thrown value's own +/// properties, which is the Bun main and Bun WAC v2 wrappers alone. Turning it on for the +/// other three would newly persist whatever an HTTP client hangs off its errors, +/// credential-bearing headers included, into the run history of every existing script on +/// those paths; that parity is its own decision, not part of stopping a crash. +/// +/// Kept comment-free, since it is written out per job. +pub const JS_ERROR_SERIALIZER: &str = r#" +const wmErrOwnKeys = ['line', 'name', 'stack', 'column', 'message', 'sourceURL', 'originalLine', 'originalColumn']; + +function wmErrString(v) { + if (typeof v === 'string') return v; + try { + return String(v); + } catch (_) { + return '[unstringifiable ' + typeof v + ']'; + } +} + +function wmErrProp(e, key) { + try { + return e[key]; + } catch (_) { + return undefined; + } +} + +function wmToErrorObject(e, collectExtra) { + if (e === null || typeof e !== 'object') { + return { message: wmErrString(e), name: 'ThrownValue' }; + } + const err = {}; + for (const field of ['message', 'name', 'stack']) { + const v = wmErrProp(e, field); + if (v != null) err[field] = wmErrString(v); + } + if (collectExtra) { + let keys = []; + try { + keys = Object.getOwnPropertyNames(e); + } catch (_) {} + const extra = {}; + for (const key of keys) { + if (wmErrOwnKeys.includes(key)) continue; + extra[key] = wmErrProp(e, key); + } + if (Object.keys(extra).length > 0) err.extra = extra; + } + if (Object.keys(err).length === 0) { + err.message = wmErrString(e); + err.name = 'ThrownValue'; + } + return err; +} + +function wmSerializeError(err) { + const ancestors = []; + try { + return JSON.stringify(err, function (key, v) { + if (v === null || typeof v !== 'object') return v; + while (ancestors.length > 0 && ancestors[ancestors.length - 1] !== this) ancestors.pop(); + if (ancestors.indexOf(v) !== -1) return '[Circular]'; + ancestors.push(v); + return v; + }); + } catch (_) { + return JSON.stringify({ message: err.message, name: err.name, stack: err.stack, step_id: err.step_id, line: err.line }); + } +} +"#; + /// Turn a JSON value into the string a shell/CLI arg should receive: a JSON string /// becomes its inner value, anything else is re-serialized compactly. pub(crate) fn raw_to_string(x: &str) -> String { diff --git a/backend/windmill-worker/src/deno_executor.rs b/backend/windmill-worker/src/deno_executor.rs index 2c5c17bb27..4875e4eeeb 100644 --- a/backend/windmill-worker/src/deno_executor.rs +++ b/backend/windmill-worker/src/deno_executor.rs @@ -9,7 +9,7 @@ use crate::{ common::{ build_command_with_isolation, create_args_and_out_file, get_reserved_variables, parse_npm_config, read_file, read_result, start_child_process, OccupancyMetrics, - StreamNotifier, + StreamNotifier, JS_ERROR_SERIALIZER, }, get_proxy_envs_for_lang, handle_child::handle_child, @@ -342,6 +342,7 @@ function argsObjToArr({{ {spread} }}) {{ BigInt.prototype.toJSON = function () {{ return this.toString(); }}; +{JS_ERROR_SERIALIZER} function isAsyncIterable(obj) {{ return obj != null && typeof obj[Symbol.asyncIterator] === 'function'; @@ -368,12 +369,12 @@ async function run() {{ try {{ await run(); }} catch(e) {{ - let err = {{ message: e.message, name: e.name, stack: e.stack }}; - let step_id = Deno.env.get("WM_FLOW_STEP_ID"); + const err = wmToErrorObject(e, false); + const step_id = Deno.env.get("WM_FLOW_STEP_ID"); if (step_id) {{ err["step_id"] = step_id; }} - await Deno.writeTextFile("result.json", JSON.stringify(err)); + await Deno.writeTextFile("result.json", wmSerializeError(err)); Deno.exit(1); }} "#, @@ -662,7 +663,7 @@ pub fn generate_dedicated_worker_wrapper(inner_content: &str) -> Result let res: any = await main(...[ {spread} ]); console.log("wm_res[success]:" + JSON.stringify(res ?? null, (key, value) => typeof value === 'undefined' ? null : value) + '\n'); }} catch (e) {{ - console.log("wm_res[error]:" + JSON.stringify({{ message: e.message, name: e.name, stack: e.stack, line: line }}) + '\n'); + console.log("wm_res[error]:" + wmSerializeError(Object.assign(wmToErrorObject(e, false), {{ line: line }})) + '\n'); }} continue; }}"# @@ -679,6 +680,7 @@ import {{ main }} from "./main.ts"; BigInt.prototype.toJSON = function () {{ return this.toString(); }}; +{JS_ERROR_SERIALIZER} console.log('start\n'); @@ -705,7 +707,7 @@ for await (const chunk of Deno.stdin.readable) {{ let res: any = await main(...[ {spread} ]); console.log("wm_res[success]:" + JSON.stringify(res ?? null, (key, value) => typeof value === 'undefined' ? null : value) + '\n'); }} catch (e) {{ - console.log("wm_res[error]:" + JSON.stringify({{ message: e.message, name: e.name, stack: e.stack, line: argsJson }}) + '\n'); + console.log("wm_res[error]:" + wmSerializeError(Object.assign(wmToErrorObject(e, false), {{ line: argsJson }})) + '\n'); }} continue; }}