From f40bbba519a97cbb1ec142c335f038dbebcd4e7c Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Mon, 11 Sep 2023 18:55:19 +0200 Subject: [PATCH] feat: attempt to SIGTERM before SIGKILL for bash --- backend/Cargo.lock | 12 ++++++++++ backend/Cargo.toml | 3 ++- .../windmill-common/src/global_settings.rs | 3 ++- backend/windmill-worker/Cargo.toml | 1 + backend/windmill-worker/src/bash_executor.rs | 2 ++ backend/windmill-worker/src/bun_executor.rs | 3 +++ backend/windmill-worker/src/common.rs | 22 ++++++++++++++++++- backend/windmill-worker/src/deno_executor.rs | 2 ++ backend/windmill-worker/src/go_executor.rs | 4 ++++ .../windmill-worker/src/python_executor.rs | 3 +++ backend/windmill-worker/src/worker.rs | 5 +++++ 11 files changed, 57 insertions(+), 3 deletions(-) diff --git a/backend/Cargo.lock b/backend/Cargo.lock index e23f880fd9..e83c7f0959 100644 --- a/backend/Cargo.lock +++ b/backend/Cargo.lock @@ -3249,6 +3249,17 @@ version = "1.0.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e4a24736216ec316047a1fc4252e27dabb04218aa4a3f37c6e7ddbf1f9782b54" +[[package]] +name = "nix" +version = "0.27.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2eb04e9c688eff1c89d72b407f168cf79bb9e867a9d3323ed6c01519eb9cc053" +dependencies = [ + "bitflags 2.4.0", + "cfg-if", + "libc", +] + [[package]] name = "nom" version = "7.1.3" @@ -7388,6 +7399,7 @@ dependencies = [ "lazy_static", "mysql_async", "native-tls", + "nix", "once_cell", "pem 3.0.2", "postgres-native-tls", diff --git a/backend/Cargo.toml b/backend/Cargo.toml index e5acd6ef7c..9bb7167bd5 100644 --- a/backend/Cargo.toml +++ b/backend/Cargo.toml @@ -187,6 +187,7 @@ postgres-native-tls = "^0" native-tls = "^0" samael = { version = "0.0.12", features = ["xmlsec"] } gcp_auth = "0.9.0" -rust_decimal = {version = "1.31.0", features = ["db-postgres"]} +rust_decimal = { version = "1.31.0", features = ["db-postgres"]} jsonwebtoken = "8.3.0" pem = "3.0.1" +nix = { version = "0.27.1", features = ["process", "signal"] } \ No newline at end of file diff --git a/backend/windmill-common/src/global_settings.rs b/backend/windmill-common/src/global_settings.rs index 44a6567422..e01a8fd152 100644 --- a/backend/windmill-common/src/global_settings.rs +++ b/backend/windmill-common/src/global_settings.rs @@ -1,6 +1,6 @@ pub const WORKER_S3_BUCKET_SYNC: &str = "worker_s3_bucket_sync"; -pub const ENV_SETTINGS: [&str; 54] = [ +pub const ENV_SETTINGS: [&str; 55] = [ "DISABLE_NSJAIL", "DISABLE_SERVER", "NUM_WORKERS", @@ -55,4 +55,5 @@ pub const ENV_SETTINGS: [&str; 54] = [ "SMTP_TLS_IMPLICIT", "CREATE_WORKSPACE_REQUIRE_SUPERADMIN", "GLOBAL_ERROR_HANDLER_PATH_IN_ADMINS_WORKSPACE", + "MAX_WAIT_FOR_SIGTERM", ]; diff --git a/backend/windmill-worker/Cargo.toml b/backend/windmill-worker/Cargo.toml index 044297890b..a6dd3e47e5 100644 --- a/backend/windmill-worker/Cargo.toml +++ b/backend/windmill-worker/Cargo.toml @@ -71,6 +71,7 @@ jsonwebtoken = { workspace = true, optional = true } sha2 = { workspace = true, optional = true } pem = { workspace = true, optional = true } urlencoding.workspace = true +nix.workspace = true [build-dependencies] deno_fetch.workspace = true diff --git a/backend/windmill-worker/src/bash_executor.rs b/backend/windmill-worker/src/bash_executor.rs index 4d5b639630..173fc5ad72 100644 --- a/backend/windmill-worker/src/bash_executor.rs +++ b/backend/windmill-worker/src/bash_executor.rs @@ -123,6 +123,7 @@ pub async fn handle_bash_job( &job.workspace_id, "bash run", job.timeout, + true, ) .await?; @@ -260,6 +261,7 @@ pub async fn handle_powershell_job( &job.workspace_id, "bash/powershell run", job.timeout, + false, ) .await?; diff --git a/backend/windmill-worker/src/bun_executor.rs b/backend/windmill-worker/src/bun_executor.rs index 9d079a4214..b1e4e4b587 100644 --- a/backend/windmill-worker/src/bun_executor.rs +++ b/backend/windmill-worker/src/bun_executor.rs @@ -105,6 +105,7 @@ pub async fn gen_lockfile( w_id, "bun build", None, + false, ) .await?; @@ -171,6 +172,7 @@ pub async fn install_lockfile( w_id, "bun install", None, + false, ) .await?; Ok(()) @@ -410,6 +412,7 @@ plugin(p) &job.workspace_id, "bun run", job.timeout, + false, ) .await?; read_result(job_dir).await diff --git a/backend/windmill-worker/src/common.rs b/backend/windmill-worker/src/common.rs index 0b30a45c1e..e25ba71a4c 100644 --- a/backend/windmill-worker/src/common.rs +++ b/backend/windmill-worker/src/common.rs @@ -1,4 +1,6 @@ use async_recursion::async_recursion; +use nix::sys::signal::{self, Signal}; +use nix::unistd::Pid; use serde_json::{json, Value}; use sqlx::{Pool, Postgres}; use tokio::{fs::File, io::AsyncReadExt}; @@ -37,7 +39,9 @@ use futures::{ stream, StreamExt, }; -use crate::{AuthedClient, MAX_RESULT_SIZE, TIMEOUT_DURATION, WHITELIST_ENVS}; +use crate::{ + AuthedClient, MAX_RESULT_SIZE, MAX_WAIT_FOR_SIGTERM, TIMEOUT_DURATION, WHITELIST_ENVS, +}; #[tracing::instrument(level = "trace", skip_all)] pub async fn create_args_and_out_file( @@ -250,6 +254,7 @@ pub async fn handle_child( _w_id: &str, child_name: &str, custom_timeout: Option, + sigterm: bool, ) -> error::Result<()> { let start = Instant::now(); let update_job_interval = Duration::from_millis(500); @@ -388,6 +393,21 @@ pub async fn handle_child( } }; + if sigterm { + if let Some(id) = child.id() { + signal::kill(Pid::from_raw(id as i32), Signal::SIGTERM).unwrap(); + for _ in 0..*MAX_WAIT_FOR_SIGTERM { + if child.try_wait().is_ok_and(|x| x.is_some()) { + break; + } + sleep(Duration::from_secs(1)).await; + } + if child.try_wait().is_ok_and(|x| x.is_some()) { + set_reason.await; + return Ok(Err(kill_reason)); + } + } + } /* send SIGKILL and reap child process */ let (_, kill) = future::join(set_reason, child.kill()).await; kill.map(|()| Err(kill_reason)) diff --git a/backend/windmill-worker/src/deno_executor.rs b/backend/windmill-worker/src/deno_executor.rs index 4a2445e035..6718c80db2 100644 --- a/backend/windmill-worker/src/deno_executor.rs +++ b/backend/windmill-worker/src/deno_executor.rs @@ -112,6 +112,7 @@ pub async fn generate_deno_lock( w_id, "deno cache", None, + false, ) .await?; @@ -315,6 +316,7 @@ run().catch(async (e) => {{ &job.workspace_id, "deno run", job.timeout, + false, ) .await?; // logs.push_str(format!("execute: {:?}\n", start.elapsed().as_millis()).as_str()); diff --git a/backend/windmill-worker/src/go_executor.rs b/backend/windmill-worker/src/go_executor.rs index a0e0bc5920..2ec04e4c06 100644 --- a/backend/windmill-worker/src/go_executor.rs +++ b/backend/windmill-worker/src/go_executor.rs @@ -198,6 +198,7 @@ func Run(req Req) (interface{{}}, error){{ &job.workspace_id, "go build", None, + false, ) .await?; @@ -274,6 +275,7 @@ func Run(req Req) (interface{{}}, error){{ &job.workspace_id, "go run", job.timeout, + false, ) .await?; read_result(job_dir).await @@ -331,6 +333,7 @@ pub async fn install_go_dependencies( w_id, "go init", None, + false, ) .await?; @@ -391,6 +394,7 @@ pub async fn install_go_dependencies( &w_id, &format!("go {mod_command}"), None, + false, ) .await .map_err(|e| Error::ExecutionErr(format!("Lockfile generation failed: {e:?}")))?; diff --git a/backend/windmill-worker/src/python_executor.rs b/backend/windmill-worker/src/python_executor.rs index 4dc68a3ead..101487d550 100644 --- a/backend/windmill-worker/src/python_executor.rs +++ b/backend/windmill-worker/src/python_executor.rs @@ -143,6 +143,7 @@ pub async fn pip_compile( &w_id, "pip-compile", None, + false, ) .await .map_err(|e| Error::ExecutionErr(format!("Lock file generation failed: {e:?}")))?; @@ -478,6 +479,7 @@ mount {{ &job.workspace_id, "python run", job.timeout, + false, ) .await?; read_result(job_dir).await @@ -637,6 +639,7 @@ pub async fn handle_python_reqs( &w_id, &format!("pip install {req}"), None, + false, ) .await; tracing::info!( diff --git a/backend/windmill-worker/src/worker.rs b/backend/windmill-worker/src/worker.rs index a18c07f22f..8b30f749ba 100644 --- a/backend/windmill-worker/src/worker.rs +++ b/backend/windmill-worker/src/worker.rs @@ -253,6 +253,11 @@ lazy_static::lazy_static! { .and_then(|x| x.parse::().ok()) .unwrap_or_else(|| if *CLOUD_HOSTED { DEFAULT_CLOUD_TIMEOUT } else { DEFAULT_SELFHOSTED_TIMEOUT }); + pub static ref MAX_WAIT_FOR_SIGTERM: u64 = std::env::var("MAX_WAIT_FOR_SIGTERM") + .ok() + .and_then(|x| x.parse::().ok()) + .unwrap_or_else(|| 5); + pub static ref TIMEOUT_DURATION: Duration = Duration::from_secs(*TIMEOUT); pub static ref SCRIPT_TOKEN_EXPIRY: i32 = std::env::var("SCRIPT_TOKEN_EXPIRY")