From 30ffdbecc15270562ceed3030c50a1cf81b1195c Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Fri, 11 Sep 2026 15:09:45 +0200 Subject: [PATCH] fix: unpin only the specifiers in the bundle a bun modules run executes (#11083) * fix: keep version pins from imported scripts in bun lockfiles Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01WCq4nkUjiZBnPPMuGYCo6w * fix: strip version pins from the bundle a bun modules run executes Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01WCq4nkUjiZBnPPMuGYCo6w * fix: unpin only module specifiers, not matching text elsewhere in the script Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01WCq4nkUjiZBnPPMuGYCo6w * docs: name the raw endpoint lock generation fetches imports through Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01WCq4nkUjiZBnPPMuGYCo6w * fix: leave require calls alone and skip spans not on a quote pair when unpinning Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01WCq4nkUjiZBnPPMuGYCo6w * test: create the bun bundle cache dir a dependency job saves into Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01WCq4nkUjiZBnPPMuGYCo6w * test: drop the lock test #11082's module test already covers Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01WCq4nkUjiZBnPPMuGYCo6w * fix: narrow the change to a fail-open strip of the modules-run bundle Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01WCq4nkUjiZBnPPMuGYCo6w * fix: unpin only the specifiers in the modules-run bundle, and log a parse fallback Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01WCq4nkUjiZBnPPMuGYCo6w --------- Co-authored-by: Claude Opus 5 (1M context) --- backend/parsers/windmill-parser-ts/src/lib.rs | 101 ++++++++++++++++++ .../parsers/windmill-parser-ts/tests/tests.rs | 41 +++++++ backend/tests/bun_jobs.rs | 51 +++++++++ backend/windmill-worker/loader.bun.js | 2 + backend/windmill-worker/src/bun_executor.rs | 15 ++- .../sync_push_auto_metadata_repro.test.ts | 2 +- 6 files changed, 209 insertions(+), 3 deletions(-) diff --git a/backend/parsers/windmill-parser-ts/src/lib.rs b/backend/parsers/windmill-parser-ts/src/lib.rs index b0ed12fb1a..271ad4835b 100644 --- a/backend/parsers/windmill-parser-ts/src/lib.rs +++ b/backend/parsers/windmill-parser-ts/src/lib.rs @@ -694,6 +694,107 @@ pub fn remove_pinned_imports(code: &str) -> anyhow::Result { Ok(content) } +/// Spans of the string literals naming a loaded module: `import`/`export … from` sources and the +/// argument of a dynamic `import()`. A `require()` call is left out: `require` is an ordinary +/// binding a script can shadow, so its argument is not known to be a module. +struct ImportSpecifierSpans(Vec); + +impl Visit for ImportSpecifierSpans { + noop_visit_type!(); + + fn visit_import_decl(&mut self, n: &swc_ecma_ast::ImportDecl) { + self.0.push(n.src.span); + } + + fn visit_export_all(&mut self, n: &swc_ecma_ast::ExportAll) { + self.0.push(n.src.span); + } + + fn visit_named_export(&mut self, n: &swc_ecma_ast::NamedExport) { + if let Some(src) = &n.src { + self.0.push(src.span); + } + } + + fn visit_call_expr(&mut self, n: &swc_ecma_ast::CallExpr) { + if let (swc_ecma_ast::Callee::Import(_), Some(arg)) = (&n.callee, n.args.first()) { + if let (None, Expr::Lit(Lit::Str(s))) = (arg.spread, &*arg.expr) { + self.0.push(s.span); + } + } + n.visit_children_with(self); + } +} + +/// Drops the `@version` from each pinned module specifier (`pkg@1.2.3/sub` -> `pkg/sub`), +/// rewriting only the specifier literals. Unlike [`remove_pinned_imports`], the same text +/// elsewhere, such as a string the script returns, stays as written. +pub fn remove_pinned_import_specifiers(code: &str) -> anyhow::Result { + let cm: Lrc = Default::default(); + let fm = cm.new_source_file( + FileName::Custom("main.d.ts".into()).into(), + code.to_string(), + ); + let mut tss = TsSyntax::default(); + tss.tsx = true; + tss.no_early_errors = true; + let lexer = Lexer::new( + Syntax::Typescript(tss), + Default::default(), + StringInput::from(&*fm), + None, + ); + let module = Parser::new_from(lexer).parse_module().map_err(|e| { + anyhow::anyhow!("Error while parsing code, it is invalid TypeScript: {e:?}") + })?; + let mut specifiers = ImportSpecifierSpans(vec![]); + specifiers.visit_module(&module); + specifiers.0.sort_by_key(|s| s.lo); + + // Spans index the parsed source, which the source map stripped of any UTF-8 BOM. + let bom = if code.starts_with('\u{feff}') { + '\u{feff}'.len_utf8() + } else { + 0 + }; + let offset = + |pos: swc_common::BytePos| pos.0.checked_sub(fm.start_pos.0).map(|o| bom + o as usize); + let mut content = String::with_capacity(code.len()); + let mut copied = 0; + for span in specifiers.0 { + // A span covers the literal's quotes. One that does not land on a matching pair is left + // as written rather than risk rewriting the wrong bytes. + let (Some(open), Some(close)) = ( + offset(span.lo), + offset(span.hi).and_then(|e| e.checked_sub(1)), + ) else { + continue; + }; + let quote = code.as_bytes().get(open); + if open >= close + || open < copied + || !matches!(quote, Some(b'"' | b'\'')) + || code.as_bytes().get(close) != quote + { + continue; + } + let Some(specifier) = code.get(open + 1..close) else { + continue; + }; + let unpinned = IMPORTS_VERSION.captures(specifier).and_then(|x| { + x.get(1) + .map(|y| format!("{}{}", y.as_str(), x.get(2).map_or("", |z| z.as_str()))) + }); + if let Some(unpinned) = unpinned.filter(|u| u != specifier) { + content.push_str(&code[copied..open + 1]); + content.push_str(&unpinned); + copied = close; + } + } + content.push_str(&code[copied..]); + Ok(content) +} + fn resolve_type_ref(type_resolver: &HashMap, typ: &mut Typ) { let mut visited = std::collections::HashSet::new(); resolve_type_ref_with_visited(type_resolver, typ, &mut visited); diff --git a/backend/parsers/windmill-parser-ts/tests/tests.rs b/backend/parsers/windmill-parser-ts/tests/tests.rs index 4309018fb4..4206dd68f8 100644 --- a/backend/parsers/windmill-parser-ts/tests/tests.rs +++ b/backend/parsers/windmill-parser-ts/tests/tests.rs @@ -4,6 +4,7 @@ mod tests { use windmill_parser::{Arg, MainArgSignature, ObjectProperty, ObjectType, Typ}; use windmill_parser_ts::{ parse_deno_signature, parse_expr_for_imports, parse_relative_imports, + remove_pinned_import_specifiers, }; #[test] @@ -33,6 +34,46 @@ mod tests { ); } + #[test] + fn test_remove_pinned_import_specifiers_rewrites_only_specifiers() { + let code = r#"// héllo +import a from "pkg@1.2.3"; +import b from "@scope/pkg@^2/sub"; +export * from "other@3"; +import rel from "./helper"; +const c = await import("dyn@4"); +const require = (v: string) => v; +const d = require("req@5"); +// pkg@1.2.3 +export const label = "pkg@1.2.3"; +"#; + assert_eq!( + remove_pinned_import_specifiers(code).unwrap(), + r#"// héllo +import a from "pkg"; +import b from "@scope/pkg/sub"; +export * from "other"; +import rel from "./helper"; +const c = await import("dyn"); +const require = (v: string) => v; +const d = require("req@5"); +// pkg@1.2.3 +export const label = "pkg@1.2.3"; +"# + ); + assert_eq!( + remove_pinned_import_specifiers("\u{feff}import a from 'pkg@1';").unwrap(), + "\u{feff}import a from 'pkg';" + ); + assert_eq!( + remove_pinned_import_specifiers( + "// a\r\n// b\r\nimport a from \"pkg@1\";\r\nimport b from 'x@2';" + ) + .unwrap(), + "// a\r\n// b\r\nimport a from \"pkg\";\r\nimport b from 'x';" + ); + } + #[test] fn test_parse_empty_main_signature() { let code = r#" diff --git a/backend/tests/bun_jobs.rs b/backend/tests/bun_jobs.rs index fc766253bc..95b4a62141 100644 --- a/backend/tests/bun_jobs.rs +++ b/backend/tests/bun_jobs.rs @@ -939,6 +939,57 @@ export function main() { return midValue(); }"#, Ok(()) } +/// A run with local modules and no lock executes the bundle its lock generation built, which +/// kept the imported script's pin; the run must still load the one copy in node_modules, and +/// leave the script's own data alone even where it matches the pinned specifier. +#[sqlx::test(fixtures("base"))] +async fn test_bun_modules_run_loads_imported_pin_from_node_modules( + db: Pool, +) -> anyhow::Result<()> { + initialize_tracing().await; + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + + insert_deployed_bun_script( + &db, + "f/pinned_import_modules/module", + 41240002, + r#"import * as isNumber from "is-number@6.0.0"; +export const ns = isNumber; +export const label = "is-number@6.0.0";"#, + ) + .await; + + let job = JobPayload::Code(RawCode { + content: r#"import * as isNumber from "is-number"; +import { ns, label } from "/f/pinned_import_modules/module"; +import { local } from "./helper"; +export function main() { return [ns === isNumber, label, local()]; }"# + .into(), + path: Some("f/pinned_import_modules/main".into()), + language: ScriptLang::Bun, + modules: Some(std::collections::HashMap::from([( + "helper.ts".to_string(), + windmill_common::scripts::ScriptModule { + content: "export const local = () => 'local';".into(), + language: ScriptLang::Bun, + lock: None, + }, + )])), + ..RawCode::default() + }); + + let result = run_job_in_new_worker_until_complete(&db, false, job, port) + .await + .json_result() + .unwrap(); + assert_eq!( + result, + serde_json::json!([true, "is-number@6.0.0", "local"]) + ); + Ok(()) +} + #[sqlx::test(fixtures("base", "bun_edge_cases"))] async fn test_bun_shared_imports_both_styles(db: Pool) -> anyhow::Result<()> { initialize_tracing().await; diff --git a/backend/windmill-worker/loader.bun.js b/backend/windmill-worker/loader.bun.js index 9e0e071cca..7a38369dd7 100644 --- a/backend/windmill-worker/loader.bun.js +++ b/backend/windmill-worker/loader.bun.js @@ -105,6 +105,8 @@ const p = { const normalized = (isRelative ? join(dirname(file_path), pathNoExt) : pathNoExt.slice(1)).replace(/\\/g, "/"); const hash = TEMP_SCRIPT_REFS?.[normalized]; + // Lock generation substitutes `raw`: the dependency scan reads versions from the + // `pkg@version` specifiers in imported scripts, which `raw_unpinned` strips. const url = (isRelative ? `${base_internal_url}/api/w/${w_id}/scripts/RAW_GET_ENDPOINT/p/${file_path}/../${args.path}${endExt}` : `${base_internal_url}/api/w/${w_id}/scripts/RAW_GET_ENDPOINT/p/${args.path}${endExt}` diff --git a/backend/windmill-worker/src/bun_executor.rs b/backend/windmill-worker/src/bun_executor.rs index 4b62dac902..16f480815e 100644 --- a/backend/windmill-worker/src/bun_executor.rs +++ b/backend/windmill-worker/src/bun_executor.rs @@ -13,7 +13,7 @@ use itertools::Itertools; use serde_json::value::RawValue; use uuid::Uuid; -use windmill_parser_ts::remove_pinned_imports; +use windmill_parser_ts::{remove_pinned_import_specifiers, remove_pinned_imports}; use windmill_queue::{append_logs, CanceledBy, MiniPulledJob, PrecomputedAgentInfo}; @@ -1784,8 +1784,19 @@ pub async fn handle_bun_job( if modules.as_ref().is_some_and(|m| !m.is_empty()) { let bundle_path = std::path::Path::new(job_dir).join("out").join("main.js"); if bundle_path.exists() { + // The lock-generation build kept every `pkg@version` specifier, and bun resolves + // a pinned specifier outside node_modules, loading a second copy of the package. + // The bundle holds the user's code too, so only the specifiers are rewritten, and + // a bundle the parser rejects still runs as built, pins and all. let bundled = std::fs::read_to_string(&bundle_path)?; - write_file(job_dir, "main.ts", &bundled)?; + let unpinned = remove_pinned_import_specifiers(&bundled).unwrap_or_else(|e| { + tracing::warn!( + job_id = %job.id, + "could not unpin the modules bundle, running it as built: {e:#}" + ); + bundled + }); + write_file(job_dir, "main.ts", &unpinned)?; } } "\n\n--- BUN CODE EXECUTION ---\n".to_string() diff --git a/cli/test/sync_push_auto_metadata_repro.test.ts b/cli/test/sync_push_auto_metadata_repro.test.ts index e9ea167ffc..4854198d64 100644 --- a/cli/test/sync_push_auto_metadata_repro.test.ts +++ b/cli/test/sync_push_auto_metadata_repro.test.ts @@ -168,7 +168,7 @@ test( // Customer scenario: a barrel file (f/lib/errors/index.ts) re-exports from // siblings (./types.ts, ./WorkflowError.ts, ...). An importer in a different // folder imports from the barrel. On a fresh DB, the dep job for the importer -// fetches index.ts via raw_unpinned + temp_script_hash, but bun's resolver +// fetches index.ts via raw + temp_script_hash, but bun's resolver // then has to resolve the barrel's *sibling* imports — and those need to be // in TEMP_SCRIPT_REFS too. test(