From ed6313258b5bba56f8a7151b55bbba24ce41bc56 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Fri, 2 May 2025 00:37:28 +0200 Subject: [PATCH] fix: do not track relative deps for scripts with raw defined deps from CLI (#5696) --- ...f7f99067a05e9dfe270223164db8a1df2e4b.json} | 4 +- backend/src/monitor.rs | 4 +- backend/windmill-api/src/scripts.rs | 11 ++- backend/windmill-api/src/tracing_init.rs | 2 + backend/windmill-worker/src/bun_executor.rs | 19 ++++-- .../windmill-worker/src/python_executor.rs | 11 ++- .../windmill-worker/src/worker_lockfiles.rs | 67 ++++++++++++++----- 7 files changed, 92 insertions(+), 26 deletions(-) rename backend/.sqlx/{query-33c1793e55b1127d88d2509aadd0eb04e042463200f237b4c2cb176612fa16fe.json => query-92faee8714a45a403b623e04d789f7f99067a05e9dfe270223164db8a1df2e4b.json} (51%) diff --git a/backend/.sqlx/query-33c1793e55b1127d88d2509aadd0eb04e042463200f237b4c2cb176612fa16fe.json b/backend/.sqlx/query-92faee8714a45a403b623e04d789f7f99067a05e9dfe270223164db8a1df2e4b.json similarity index 51% rename from backend/.sqlx/query-33c1793e55b1127d88d2509aadd0eb04e042463200f237b4c2cb176612fa16fe.json rename to backend/.sqlx/query-92faee8714a45a403b623e04d789f7f99067a05e9dfe270223164db8a1df2e4b.json index 8728e35a0c..7df22ca7b7 100644 --- a/backend/.sqlx/query-33c1793e55b1127d88d2509aadd0eb04e042463200f237b4c2cb176612fa16fe.json +++ b/backend/.sqlx/query-92faee8714a45a403b623e04d789f7f99067a05e9dfe270223164db8a1df2e4b.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "INSERT INTO log_file (hostname, mode, worker_group, log_ts, file_path, ok_lines, err_lines, json_fmt) VALUES ($1, $2::text::LOG_MODE, $3, $4, $5, $6, $7, $8)", + "query": "INSERT INTO log_file (hostname, mode, worker_group, log_ts, file_path, ok_lines, err_lines, json_fmt)\n VALUES ($1, $2::text::LOG_MODE, $3, $4, $5, $6, $7, $8)\n ON CONFLICT (hostname, log_ts) DO UPDATE SET ok_lines = log_file.ok_lines + $6, err_lines = log_file.err_lines + $7", "describe": { "columns": [], "parameters": { @@ -17,5 +17,5 @@ }, "nullable": [] }, - "hash": "33c1793e55b1127d88d2509aadd0eb04e042463200f237b4c2cb176612fa16fe" + "hash": "92faee8714a45a403b623e04d789f7f99067a05e9dfe270223164db8a1df2e4b" } diff --git a/backend/src/monitor.rs b/backend/src/monitor.rs index 7366cb4f53..5d1532c216 100644 --- a/backend/src/monitor.rs +++ b/backend/src/monitor.rs @@ -638,7 +638,9 @@ async fn send_log_file_to_object_store( let (ok_lines, err_lines) = read_log_counters(ts_str); if let Some(db) = conn.as_sql() { - if let Err(e) = sqlx::query!("INSERT INTO log_file (hostname, mode, worker_group, log_ts, file_path, ok_lines, err_lines, json_fmt) VALUES ($1, $2::text::LOG_MODE, $3, $4, $5, $6, $7, $8)", + if let Err(e) = sqlx::query!("INSERT INTO log_file (hostname, mode, worker_group, log_ts, file_path, ok_lines, err_lines, json_fmt) + VALUES ($1, $2::text::LOG_MODE, $3, $4, $5, $6, $7, $8) + ON CONFLICT (hostname, log_ts) DO UPDATE SET ok_lines = log_file.ok_lines + $6, err_lines = log_file.err_lines + $7", hostname, mode.to_string(), worker_group.clone(), ts, highest_file, ok_lines as i64, err_lines as i64, *JSON_FMT) .execute(db) .await { diff --git a/backend/windmill-api/src/scripts.rs b/backend/windmill-api/src/scripts.rs index a1ca9c3322..aeb3686d0b 100644 --- a/backend/windmill-api/src/scripts.rs +++ b/backend/windmill-api/src/scripts.rs @@ -650,8 +650,13 @@ async fn create_script_internal<'c>( ) { Some(String::new()) } else { - ns.lock - .and_then(|e| if e.is_empty() { None } else { Some(e) }) + ns.lock.as_ref().and_then(|e| { + if e.is_empty() { + None + } else { + Some(e.to_string()) + } + }) }; let needs_lock_gen = lock.is_none() && codebase.is_none(); @@ -901,6 +906,7 @@ async fn create_script_internal<'c>( let permissioned_as2 = permissioned_as.clone(); let script_path2 = script_path.clone(); let parent_path = p_path_opt.clone(); + let lock = ns.lock.clone(); let deployment_message = ns.deployment_message.clone(); let content = ns.content.clone(); let language = ns.language.clone(); @@ -920,6 +926,7 @@ async fn create_script_internal<'c>( &authed2.email, &authed2.username, &permissioned_as2, + lock, ) .await { diff --git a/backend/windmill-api/src/tracing_init.rs b/backend/windmill-api/src/tracing_init.rs index 9f34e93592..c5c841b2dc 100644 --- a/backend/windmill-api/src/tracing_init.rs +++ b/backend/windmill-api/src/tracing_init.rs @@ -33,6 +33,8 @@ impl OnResponse for MyOnResponse { let status = response.status().as_u16(); if response.status().is_success() || response.status().is_redirection() { tracing::info!(latency = latency, status = status, "response") + } else if response.status().as_u16() == 404 { + tracing::warn!(latency = latency, status = status, "response") } else { tracing::error!(latency = latency, status = status, "response") } diff --git a/backend/windmill-worker/src/bun_executor.rs b/backend/windmill-worker/src/bun_executor.rs index 91284c9d41..6a6ad120e7 100644 --- a/backend/windmill-worker/src/bun_executor.rs +++ b/backend/windmill-worker/src/bun_executor.rs @@ -42,7 +42,7 @@ use windmill_common::{ error::{self, Result}, get_latest_hash_for_path, scripts::ScriptLang, - worker::{exists_in_cache, save_cache, write_file, Connection, DISABLE_BUNDLING}, + worker::{exists_in_cache, save_cache, to_raw_value, write_file, Connection, DISABLE_BUNDLING}, DB, }; @@ -111,7 +111,7 @@ pub async fn gen_bun_lockfile( let mut empty_deps = false; - if let Some(raw_deps) = raw_deps { + if let Some(raw_deps) = raw_deps.as_ref() { gen_bunfig(job_dir).await?; write_file(job_dir, "package.json", raw_deps.as_str())?; } else { @@ -201,10 +201,21 @@ pub async fn gen_bun_lockfile( } if export_pkg { - let mut content = "".to_string(); + let mut content; { let mut file = File::open(format!("{job_dir}/package.json")).await?; - file.read_to_string(&mut content).await?; + let mut buf = String::default(); + file.read_to_string(&mut buf).await?; + if raw_deps.is_some() { + let mut json_map: HashMap> = serde_json::from_str(&buf)?; + json_map.insert( + "generatedFromPackageJson".to_string(), + to_raw_value(&"true".to_string()), + ); + content = serde_json::to_string_pretty(&json_map)?; + } else { + content = buf; + } } if !npm_mode { #[cfg(any(target_os = "linux", target_os = "macos"))] diff --git a/backend/windmill-worker/src/python_executor.rs b/backend/windmill-worker/src/python_executor.rs index c642abae49..a776bbe7c7 100644 --- a/backend/windmill-worker/src/python_executor.rs +++ b/backend/windmill-worker/src/python_executor.rs @@ -77,6 +77,7 @@ use crate::{ start_child_process, OccupancyMetrics, }, handle_child::handle_child, + worker_lockfiles::LOCKFILE_GENERATED_FROM_REQUIREMENTS_TXT, worker_utils::ping_job_status, AuthedClient, DISABLE_NSJAIL, DISABLE_NUSER, HOME_ENV, INSTANCE_PYTHON_VERSION, NSJAIL_PATH, PATH_ENV, PIP_EXTRA_INDEX_URL, PIP_INDEX_URL, PROXY_ENVS, PY_INSTALL_DIR, TZ_ENV, UV_CACHE_DIR, @@ -2303,8 +2304,16 @@ fn split_requirements(requirements: &str) -> Vec<&str> { /// Check requirements/lockfile to figure out python version assigned to it. fn get_pyv_from_requirements_lines(requirements_lines: &[&str]) -> PyVersion { // If script is deployed we can try to parse first line to get assigned version + + let index = if requirements_lines.get(0).map_or(false, |line| { + line.starts_with(LOCKFILE_GENERATED_FROM_REQUIREMENTS_TXT) + }) { + 1 + } else { + 0 + }; if let Some(v) = requirements_lines - .get(0) + .get(index) .and_then(|line| PyVersion::parse_version(*line)) { // We have valid assigned version, we use it diff --git a/backend/windmill-worker/src/worker_lockfiles.rs b/backend/windmill-worker/src/worker_lockfiles.rs index 7db2640a92..82ad8cc77b 100644 --- a/backend/windmill-worker/src/worker_lockfiles.rs +++ b/backend/windmill-worker/src/worker_lockfiles.rs @@ -65,17 +65,17 @@ pub async fn update_script_dependency_map( relative_imports: Vec, ) -> error::Result<()> { let importer_kind = "script"; + + let mut tx = db.begin().await?; + tx = clear_dependency_parent_path(parent_path, script_path, w_id, importer_kind, tx).await?; + + tx = clear_dependency_map_for_item(script_path, w_id, importer_kind, tx, &None).await?; + if !relative_imports.is_empty() { let mut logs = "".to_string(); logs.push_str("\n--- RELATIVE IMPORTS ---\n\n"); logs.push_str(&relative_imports.join("\n")); - let mut tx = db.begin().await?; - tx = - clear_dependency_parent_path(parent_path, script_path, w_id, importer_kind, tx).await?; - - tx = clear_dependency_map_for_item(script_path, w_id, importer_kind, tx, &None).await?; - tx = add_relative_imports_to_dependency_map( script_path, w_id, @@ -86,9 +86,10 @@ pub async fn update_script_dependency_map( None, ) .await?; - tx.commit().await?; append_logs(job_id, w_id, logs, &db.into()).await; } + tx.commit().await?; + Ok(()) } @@ -381,6 +382,7 @@ pub async fn handle_dependency_job( &job.permissioned_as_email, &job.created_by, &job.permissioned_as, + None, ) .await?; @@ -433,18 +435,42 @@ pub async fn process_relative_imports( permissioned_as_email: &str, created_by: &str, permissioned_as: &str, + lock: Option, ) -> error::Result<()> { let relative_imports = extract_relative_imports(&code, script_path, script_lang); if let Some(relative_imports) = relative_imports { - update_script_dependency_map( - &job_id.unwrap_or_else(|| Uuid::nil()), - db, - w_id, - &parent_path, - script_path, - relative_imports, - ) - .await?; + if (script_lang.is_some_and(|v| v == ScriptLang::Bun) + && lock + .as_ref() + .is_some_and(|v| v.contains("generatedFromPackageJson"))) + || (script_lang.is_some_and(|v| v == ScriptLang::Python3) + && lock + .as_ref() + .is_some_and(|v| v.starts_with(LOCKFILE_GENERATED_FROM_REQUIREMENTS_TXT))) + { + // if the lock file is generated from a package.json/requirements.txt, we need to clear the dependency map + // because we do not want to have dependencies be recomputed automatically. Empty relative imports passed + // to update_script_dependency_map will clear the dependency map. + update_script_dependency_map( + &job_id.unwrap_or_else(|| Uuid::nil()), + db, + w_id, + &parent_path, + script_path, + vec![], + ) + .await?; + } else { + update_script_dependency_map( + &job_id.unwrap_or_else(|| Uuid::nil()), + db, + w_id, + &parent_path, + script_path, + relative_imports, + ) + .await?; + } let already_visited = args .map(|x| { x.get("already_visited") @@ -2050,6 +2076,8 @@ async fn ansible_dep( serde_json::to_string(&ansible_lockfile).map_err(|e| e.into()) } +pub const LOCKFILE_GENERATED_FROM_REQUIREMENTS_TXT: &str = "# from requirements.txt"; + async fn capture_dependency_job( job_id: &Uuid, job_language: &ScriptLang, @@ -2118,6 +2146,13 @@ async fn capture_dependency_job( anns, ) .await + .map(|res| { + if raw_deps { + format!("{}\n{}", LOCKFILE_GENERATED_FROM_REQUIREMENTS_TXT, res) + } else { + res + } + }) } } ScriptLang::Ansible => {