From eebaab9c87f975b70049e118a08665fc21653b13 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Wed, 6 May 2026 10:00:46 +0000 Subject: [PATCH] fix(bun): propagate non-zero exit from generate_bun_bundle on no-DB path (#9051) --- backend/src/main.rs | 2 - backend/tests/bun_jobs.rs | 215 ++++++++++++++++++ backend/windmill-worker/loader_builder.bun.js | 8 +- backend/windmill-worker/src/bun_executor.rs | 65 ++++-- backend/windmill-worker/src/lib.rs | 7 +- 5 files changed, 278 insertions(+), 19 deletions(-) diff --git a/backend/src/main.rs b/backend/src/main.rs index ce09c7f25f..3f3a9b0e29 100644 --- a/backend/src/main.rs +++ b/backend/src/main.rs @@ -305,8 +305,6 @@ async fn cache_hub_scripts(file_path: Option) -> anyhow::Result<()> { ) .await?; - let _ = windmill_common::worker::write_file(&job_dir, "main.js", &res.content)?; - if let Err(e) = windmill_worker::prebundle_bun_script( &res.content, &lock, diff --git a/backend/tests/bun_jobs.rs b/backend/tests/bun_jobs.rs index a791060a2c..fa15866bca 100644 --- a/backend/tests/bun_jobs.rs +++ b/backend/tests/bun_jobs.rs @@ -956,6 +956,221 @@ export function main() { Ok(()) } +// ============================================================================ +// Bundle Wrapper Safety Tests +// ============================================================================ + +/// Regression test for the "TS source ends up in the bun bundle cache" bug. +/// +/// The wrapper-side hardening: `node_builder.ts` discarded `Bun.build`'s +/// return value, so any silent-failure mode (`success: false` without +/// throwing — `throw: false`, or a future Bun where defaults change) made +/// the wrapper exit 0 even though no `main.js` was written. Pair that with +/// a pre-existing `main.js` containing raw TypeScript and `save_cache` +/// happily copied that TS into the bundle cache; the worker later choked +/// on `type GpgKey = {`. +/// +/// This test patches `node_builder.ts` to force the silent-failure shape +/// and asserts that our wrapper now refuses to silently succeed — bun must +/// exit non-zero so prebundling fails loudly instead of writing TypeScript +/// into the bundle cache. +#[test] +fn test_bun_bundle_wrapper_catches_silent_failure() { + use std::process::Command; + use windmill_worker::{build_loader, LoaderMode, BUN_PATH}; + + let temp_dir = tempfile::tempdir().unwrap(); + let dir = temp_dir.path(); + let dir_str = dir.to_str().unwrap(); + + // Script imports a package that won't exist in node_modules. + std::fs::write( + dir.join("main.ts"), + r#" +import x from "definitely-not-a-real-pkg-windmill-test"; +export function main() { return x; } +"#, + ) + .unwrap(); + + // Generate the real node_builder.ts via the production code path. + tokio::runtime::Runtime::new() + .unwrap() + .block_on(build_loader( + dir_str, + "http://localhost:8000", + "test_token", + "test-workspace", + "f/test/script", + LoaderMode::BunBundle, + &None, + )) + .expect("build_loader failed"); + + // Force the silent-failure shape by injecting `throw: false`. The + // wrapper's pre-fix `try/catch` would have swallowed this; the fixed + // wrapper inspects `result.success` and `result.outputs` and exits 1. + let path = dir.join("node_builder.ts"); + let original = std::fs::read_to_string(&path).unwrap(); + let patched = original.replace( + "external: [\"electron\"],", + "external: [\"electron\"], throw: false,", + ); + assert_ne!( + original, patched, + "expected to find Bun.build options block to patch; node_builder.ts template changed?" + ); + std::fs::write(&path, patched).unwrap(); + + // Pre-seed main.js with raw TypeScript (mimics the historical + // pre-write that originally seeded the bug). + std::fs::write( + dir.join("main.js"), + "type GpgKey = { email: string };\nexport const main = (): GpgKey => ({ email: \"\" });\n", + ) + .unwrap(); + + let output = Command::new(BUN_PATH.as_str()) + .args(["run", path.to_str().unwrap()]) + .current_dir(dir) + .output() + .expect("Failed to run bun"); + + let stdout = String::from_utf8_lossy(&output.stdout); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + !output.status.success(), + "node_builder.ts must exit non-zero when Bun.build silently fails to write a bundle.\nstdout:\n{stdout}\nstderr:\n{stderr}" + ); + assert!( + stdout.contains("Failed to build node bundle"), + "expected diagnostic in stdout, got:\n{stdout}" + ); +} + +/// Regression test for the actual root cause of the "TS source in bundle +/// cache" bug: `generate_bun_bundle` was awaiting `child_process.wait()` +/// without checking the exit code on the no-DB path (used by Docker-build +/// `windmill cache hubPaths.json`). bun would exit 1 after Bun.build threw, +/// `wait().await?` propagated only IO errors, and `generate_bun_bundle` +/// returned `Ok(())`. `save_cache` then copied a stale `main.js` (raw TS +/// source) straight into the bundle cache. +/// +/// This test runs `generate_bun_bundle` with `db: None` against a `node_builder.ts` +/// that calls `process.exit(1)`, and asserts the function now returns an error. +#[test] +fn test_generate_bun_bundle_propagates_exit_status() { + use windmill_worker::{generate_bun_bundle, get_common_bun_proc_envs}; + + let temp_dir = tempfile::tempdir().unwrap(); + let dir = temp_dir.path(); + let dir_str = dir.to_str().unwrap(); + + // node_builder.ts that exits 1, mimicking what bun does when Bun.build throws. + std::fs::write( + dir.join("node_builder.ts"), + "console.log('simulated bun build failure');\nprocess.exit(1);\n", + ) + .unwrap(); + + let runtime = tokio::runtime::Runtime::new().unwrap(); + let envs = runtime.block_on(get_common_bun_proc_envs(None)); + + let result = runtime.block_on(generate_bun_bundle( + dir_str, + "test-workspace", + &uuid::Uuid::new_v4(), + "test-worker", + None, // db: None — this is the cache_hub_scripts path that had the bug + None, + &mut 0, + &mut None, + &envs, + &mut None, + )); + + assert!( + result.is_err(), + "generate_bun_bundle must surface bun's non-zero exit on the no-DB path. \ + If it returns Ok(()) when bun exited 1, save_cache will silently cache stale main.js content." + ); + let err_msg = format!("{:?}", result.unwrap_err()); + assert!( + err_msg.contains("non-zero status"), + "expected exit-status error, got: {err_msg}" + ); +} + +/// Regression test for the install_bun_lockfile no-DB path: same code shape as +/// `generate_bun_bundle` (site 3 of the original bug) — `wait().await?` ignored +/// non-zero bun exits. A `bun install` failure (e.g. malformed package.json) +/// must now surface as an error so callers don't proceed with a half-installed +/// node_modules. +#[test] +fn test_install_bun_lockfile_propagates_exit_status() { + use windmill_worker::{get_common_bun_proc_envs, install_bun_lockfile}; + let temp_dir = tempfile::tempdir().unwrap(); + let dir = temp_dir.path(); + let dir_str = dir.to_str().unwrap(); + // Malformed package.json -> bun install fails with exit 1. + std::fs::write(dir.join("package.json"), "this is not valid json").unwrap(); + let runtime = tokio::runtime::Runtime::new().unwrap(); + let envs = runtime.block_on(get_common_bun_proc_envs(None)); + let result = runtime.block_on(install_bun_lockfile( + &mut 0, + &mut None, + &uuid::Uuid::new_v4(), + "test-workspace", + None, // db: None — no-DB path that had the bug + dir_str, + "test-worker", + envs, + false, // npm_mode + &mut None, + true, // quiet + )); + assert!( + result.is_err(), + "install_bun_lockfile must surface bun's non-zero exit on the no-DB path" + ); + let err_msg = format!("{:?}", result.unwrap_err()); + assert!( + err_msg.contains("non-zero status"), + "expected exit-status error, got: {err_msg}" + ); +} + +/// Regression test for the post-bundle existence check in `prebundle_bun_script` +/// and `handle_bun_job`. Both call sites guard against the case where +/// `generate_bun_bundle` returns `Ok(())` but `main.js` was never written — +/// the upstream wait-status fix is the primary defense, this is the catch-all +/// for any other silent-failure mode (Bun output-naming change, custom plugin +/// swallowing the build, etc.). Without this check, `save_cache` would +/// happily copy whatever's at the bundle path (often raw TypeScript that some +/// other code path left there). +#[test] +fn test_ensure_bundle_output_exists_rejects_missing_file() { + use windmill_worker::ensure_bundle_output_exists; + let temp_dir = tempfile::tempdir().unwrap(); + let dir = temp_dir.path(); + let missing = dir.join("main.js").to_str().unwrap().to_string(); + + let result = ensure_bundle_output_exists(&missing); + assert!( + result.is_err(), + "ensure_bundle_output_exists must reject when the bundle file is missing" + ); + let err_msg = format!("{:?}", result.unwrap_err()); + assert!( + err_msg.contains("bun bundle output missing"), + "expected 'bun bundle output missing' in error, got: {err_msg}" + ); + + // Sanity: when the file does exist, it returns Ok. + std::fs::write(&missing, "// @bun\n").unwrap(); + assert!(ensure_bundle_output_exists(&missing).is_ok()); +} + // ============================================================================ // Dedicated Worker Protocol Tests // ============================================================================ diff --git a/backend/windmill-worker/loader_builder.bun.js b/backend/windmill-worker/loader_builder.bun.js index 5ca1b22ab2..d2206b398b 100644 --- a/backend/windmill-worker/loader_builder.bun.js +++ b/backend/windmill-worker/loader_builder.bun.js @@ -1,5 +1,6 @@ +let buildResult; try { - await Bun.build({ + buildResult = await Bun.build({ entrypoints: ["./main.ts"], outdir: "./out", plugins: [p], @@ -17,6 +18,11 @@ try { console.log(err); process.exit(1); } +if (!buildResult?.success || !(buildResult.outputs?.length > 0)) { + for (const log of buildResult?.logs ?? []) console.log(log); + console.log("Failed to build bundle: success=" + buildResult?.success + ", outputs=" + (buildResult?.outputs?.length ?? 0)); + process.exit(1); +} const fs = require("fs/promises"); diff --git a/backend/windmill-worker/src/bun_executor.rs b/backend/windmill-worker/src/bun_executor.rs index be842e28b4..82d59d44b1 100644 --- a/backend/windmill-worker/src/bun_executor.rs +++ b/backend/windmill-worker/src/bun_executor.rs @@ -471,7 +471,12 @@ pub async fn gen_bun_lockfile( } result?; } else { - Box::into_pin(child_process.wait()).await?; + let status = Box::into_pin(child_process.wait()).await?; + if !status.success() { + return Err(error::Error::ExecutionErr(format!( + "bun build exited with non-zero status: {status:?}" + ))); + } } let new_package_json = read_file_content(&format!("{job_dir}/package.json")).await?; @@ -777,7 +782,12 @@ pub async fn install_bun_lockfile( } result?; } else { - Box::into_pin(child_process.wait()).await?; + let status = Box::into_pin(child_process.wait()).await?; + if !status.success() { + return Err(error::Error::ExecutionErr(format!( + "bun install exited with non-zero status: {status:?}" + ))); + } } if has_file { @@ -838,8 +848,9 @@ try {{ }} catch (e) {{ }} +let result; try {{ - await Bun.build({{ + result = await Bun.build({{ entrypoints: ["{job_dir_js}/wrapper.mjs"], outdir: "./", target: "node", @@ -852,6 +863,11 @@ try {{ console.log("Failed to build node bundle"); process.exit(1); }} +if (!result?.success || !(result.outputs?.length > 0)) {{ + for (const log of result?.logs ?? []) console.log(log); + console.log("Failed to build node bundle: success=" + result?.success + ", outputs=" + (result?.outputs?.length ?? 0)); + process.exit(1); +}} "# ), )?; @@ -880,8 +896,9 @@ plugin(p) r#" {loader} +let result; try {{ - await Bun.build({{ + result = await Bun.build({{ entrypoints: ["{job_dir_js}/main.ts"], outdir: "./", target: "{}", @@ -898,6 +915,11 @@ try {{ console.log("Failed to build node bundle"); process.exit(1); }} +if (!result?.success || !(result.outputs?.length > 0)) {{ + for (const log of result?.logs ?? []) console.log(log); + console.log("Failed to build node bundle: success=" + result?.success + ", outputs=" + (result?.outputs?.length ?? 0)); + process.exit(1); +}} "#, if mode == LoaderMode::BunBundle { "bun" @@ -1008,7 +1030,12 @@ pub async fn generate_bun_bundle( ) .await?; } else { - Box::into_pin(child_process.wait()).await?; + let status = Box::into_pin(child_process.wait()).await?; + if !status.success() { + return Err(error::Error::ExecutionErr(format!( + "bun build exited with non-zero status: {status:?}" + ))); + } } Ok(()) } @@ -1137,6 +1164,9 @@ pub async fn prebundle_bun_script( content = format!("export {{ WorkflowCtx, StepSuspend, setWorkflowCtx }} from \"windmill-client\";\n{content}"); } write_file(job_dir, "main.ts", &content)?; + // Remove any stale main.js so we never confuse a leftover (e.g. unbundled TS source + // a caller dropped at this path) with a fresh Bun bundle output. + let _ = std::fs::remove_file(&origin); build_loader( job_dir, base_internal_url, @@ -1170,11 +1200,25 @@ pub async fn prebundle_bun_script( ) .await?; + ensure_bundle_output_exists(&origin)?; + save_cache(&local_path, &remote_path, &origin, false).await?; Ok(()) } +/// Refuse to cache a bundle if `Bun.build` finished without producing the +/// expected output file. Belt-and-suspenders for any silent-failure mode the +/// upstream wait-status / `result.success` checks don't already trip on. +pub fn ensure_bundle_output_exists(bundle_path: &str) -> Result<()> { + if !std::path::Path::new(bundle_path).exists() { + return Err(error::Error::ExecutionErr(format!( + "bun bundle output missing at {bundle_path} after Bun.build — refusing to cache" + ))); + } + Ok(()) +} + pub const BUN_BUNDLE_OBJECT_STORE_PREFIX: &str = "bun_bundle/"; async fn get_script_import_updated_at(db: &DB, w_id: &str, script_path: &str) -> Result { @@ -1902,15 +1946,10 @@ try {{ &mut Some(occupancy_metrics), ) .await?; + let bundle_path = format!("{job_dir}/main.js"); + ensure_bundle_output_exists(&bundle_path)?; if !local_path.is_empty() { - match save_cache( - &local_path, - &remote_path, - &format!("{job_dir}/main.js"), - false, - ) - .await - { + match save_cache(&local_path, &remote_path, &bundle_path, false).await { Err(e) => { let em = format!("could not save {local_path} to bundle cache: {e:?}"); tracing::error!(em) diff --git a/backend/windmill-worker/src/lib.rs b/backend/windmill-worker/src/lib.rs index 2634824d5c..0d5df81a83 100644 --- a/backend/windmill-worker/src/lib.rs +++ b/backend/windmill-worker/src/lib.rs @@ -90,9 +90,10 @@ pub use otel_tracing_proxy_ee::{load_internal_otel_exporter, DENO_OTEL_INITIALIZ pub use worker::*; pub use bun_executor::{ - build_loader, compute_bundle_local_and_remote_path, get_common_bun_proc_envs, - install_bun_lockfile, prebundle_bun_script, prepare_job_dir, LoaderMode, - BUN_DEDICATED_WORKER_ARGS, RELATIVE_BUN_BUILDER, RELATIVE_BUN_LOADER, + build_loader, compute_bundle_local_and_remote_path, ensure_bundle_output_exists, + generate_bun_bundle, get_common_bun_proc_envs, install_bun_lockfile, prebundle_bun_script, + prepare_job_dir, LoaderMode, BUN_DEDICATED_WORKER_ARGS, RELATIVE_BUN_BUILDER, + RELATIVE_BUN_LOADER, }; #[cfg(any(feature = "private", test))] pub use bun_executor::{