diff --git a/crates/tower-package/src/native.rs b/crates/tower-package/src/native.rs index e79c271e..fe601d0e 100644 --- a/crates/tower-package/src/native.rs +++ b/crates/tower-package/src/native.rs @@ -144,19 +144,7 @@ impl Package { }); } - // Shell scripts (.sh) are executed relative to package_path, while Python - // scripts are executed relative to the app working directory. Check the - // path that matches the runtime's actual execution behavior. - let resolved_path = if manifest.invoke.ends_with(".sh") { - package_path.join(&manifest.invoke) - } else { - let working_dir = if manifest.version == Some(1) { - package_path.to_path_buf() - } else { - package_path.join(&manifest.app_dir_name) - }; - working_dir.join(&manifest.invoke) - }; + let resolved_path = Self::resolve_invoke_path(manifest, package_path); if !resolved_path.exists() { return Err(Error::MissingScript { @@ -177,6 +165,30 @@ impl Package { Ok(()) } + /// The directory an app runs in: the package root for v1 packages, the app directory + /// otherwise. + pub fn working_dir(manifest: &Manifest, package_path: &Path) -> PathBuf { + if manifest.version == Some(1) { + package_path.to_path_buf() + } else { + package_path.join(&manifest.app_dir_name) + } + } + + /// Where the manifest's invoke script lives under `package_path`. Scripts resolve from the + /// working directory, where the packager puts them. Shell scripts fall back to the package + /// root, where the runtime used to look for them. + pub fn resolve_invoke_path(manifest: &Manifest, package_path: &Path) -> PathBuf { + let in_working_dir = Self::working_dir(manifest, package_path).join(&manifest.invoke); + if manifest.invoke.ends_with(".sh") && !in_working_dir.exists() { + let in_root = package_path.join(&manifest.invoke); + if in_root.exists() { + return in_root; + } + } + in_working_dir + } + // build creates a new package from a PackageSpec. PackageSpec is typically composed of fields // copied from the Towerfile. The most important thing to know is that the collection of file // globs to include in the package. @@ -653,26 +665,40 @@ mod test { } #[test] - fn test_validate_invoke_script_shell_resolves_from_package_root() { + fn test_validate_invoke_script_shell_resolves_from_app_dir() { let dir = TempDir::new().unwrap(); let app_dir = dir.path().join("app"); std::fs::create_dir_all(&app_dir).unwrap(); - // Shell script placed at package root level (not inside app/) - std::fs::write(dir.path().join("run.sh"), "#!/bin/bash").unwrap(); + // The packager puts every app file, shell scripts included, under app/. + std::fs::write(app_dir.join("run.sh"), "#!/bin/bash").unwrap(); let manifest = make_manifest("./run.sh", None); - // Should resolve from package_path for .sh files assert!(Package::validate_invoke_script(&manifest, dir.path()).is_ok()); + assert_eq!( + Package::resolve_invoke_path(&manifest, dir.path()), + app_dir.join("./run.sh") + ); } #[test] - fn test_validate_invoke_script_shell_not_in_app_dir() { + fn test_validate_invoke_script_shell_falls_back_to_package_root() { let dir = TempDir::new().unwrap(); let app_dir = dir.path().join("app"); std::fs::create_dir_all(&app_dir).unwrap(); - // Shell script placed inside app/ but runtime looks at package root - std::fs::write(app_dir.join("run.sh"), "#!/bin/bash").unwrap(); + std::fs::write(dir.path().join("run.sh"), "#!/bin/bash").unwrap(); let manifest = make_manifest("./run.sh", None); - // Should fail because runtime looks at package_path.join(invoke), not working_dir + assert!(Package::validate_invoke_script(&manifest, dir.path()).is_ok()); + assert_eq!( + Package::resolve_invoke_path(&manifest, dir.path()), + dir.path().join("./run.sh") + ); + } + + #[test] + fn test_validate_invoke_script_python_does_not_fall_back_to_package_root() { + let dir = TempDir::new().unwrap(); + std::fs::create_dir_all(dir.path().join("app")).unwrap(); + std::fs::write(dir.path().join("main.py"), "print('hi')").unwrap(); + let manifest = make_manifest("./main.py", None); let err = Package::validate_invoke_script(&manifest, dir.path()).unwrap_err(); assert!(matches!(err, Error::MissingScript { .. })); } diff --git a/crates/tower-runtime/src/local.rs b/crates/tower-runtime/src/local.rs index 51b0cafa..98e85a92 100644 --- a/crates/tower-runtime/src/local.rs +++ b/crates/tower-runtime/src/local.rs @@ -29,7 +29,7 @@ use nix::{ use tokio_util::sync::CancellationToken; -use tower_package::{Manifest, Package}; +use tower_package::Package; use tower_telemetry::debug; use tower_uv::Uv; @@ -149,11 +149,7 @@ async fn inner_execute_local_app( let package_path = package.unpacked_path.clone().unwrap().to_path_buf(); // set for later on. - let working_dir = if package.manifest.version == Some(1) { - package_path.to_path_buf() - } else { - package_path.join(&package.manifest.app_dir_name) - }; + let working_dir = Package::working_dir(&package.manifest, &package_path); debug!(ctx: &ctx, " - working directory: {:?}", &working_dir); @@ -182,12 +178,17 @@ async fn inner_execute_local_app( return Err(Error::Cancelled); } + // Without a cache, an app runs in protected mode: its process gets a cleared environment + // plus only what we pass in. + let protected_mode = opts.cache_dir.is_none(); + if is_bash_package(&package) { - let child = execute_bash_program( + let script = Package::resolve_invoke_path(manifest, &package_path); + let mut child = execute_bash_program( &ctx, working_dir, - package_path, - &manifest, + script, + protected_mode, secrets, params, other_env_vars, @@ -195,6 +196,8 @@ async fn inner_execute_local_app( ) .await?; + forward_output(&mut child, Channel::Program, &opts.output_sender); + let code = wait_for_process(ctx.clone(), &cancel_token, child).await; // `wait_for_process` returns -1 on both cancellation and IO error; // disambiguate so a cancellation surfaces as `Status::Cancelled` @@ -204,9 +207,6 @@ async fn inner_execute_local_app( } Ok(code) } else { - // we put Uv in to protected mode when there's no caching configured/enabled. - let protected_mode = opts.cache_dir.is_none(); - let uv = Uv::new(opts.cache_dir, protected_mode).await?; let env_vars = make_env_vars( &ctx, @@ -227,23 +227,7 @@ async fn inner_execute_local_app( } let mut child = uv.venv(&working_dir, &env_vars).await?; - - // Drain the logs to the output channel. - let stdout = child.stdout.take().expect("no stdout"); - tokio::spawn(drain_output( - FD::Stdout, - Channel::Setup, - opts.output_sender.clone(), - BufReader::new(stdout), - )); - - let stderr = child.stderr.take().expect("no stderr"); - tokio::spawn(drain_output( - FD::Stderr, - Channel::Setup, - opts.output_sender.clone(), - BufReader::new(stderr), - )); + forward_output(&mut child, Channel::Setup, &opts.output_sender); // Wait for venv to finish up. let res = wait_for_process(ctx.clone(), &cancel_token, child).await; @@ -333,23 +317,7 @@ async fn inner_execute_local_app( } let mut child = uv.run(&working_dir, &program_path, &env_vars).await?; - - // Drain the logs to the output channel. - let stdout = child.stdout.take().expect("no stdout"); - tokio::spawn(drain_output( - FD::Stdout, - Channel::Program, - opts.output_sender.clone(), - BufReader::new(stdout), - )); - - let stderr = child.stderr.take().expect("no stderr"); - tokio::spawn(drain_output( - FD::Stderr, - Channel::Program, - opts.output_sender.clone(), - BufReader::new(stderr), - )); + forward_output(&mut child, Channel::Program, &opts.output_sender); let code = wait_for_process(ctx.clone(), &cancel_token, child).await; if cancel_token.is_cancelled() { @@ -473,8 +441,8 @@ impl App for LocalApp { async fn execute_bash_program( ctx: &tower_telemetry::Context, cwd: PathBuf, - package_path: PathBuf, - manifest: &Manifest, + script: PathBuf, + protected_mode: bool, secrets: HashMap, params: HashMap, other_env_vars: HashMap, @@ -483,26 +451,39 @@ async fn execute_bash_program( let bash_path = find_bash().await?; debug!(ctx: &ctx, "using bash at {:?}", bash_path); - debug!(ctx: &ctx, " - bash script {}", manifest.invoke); + debug!(ctx: &ctx, " - bash script {:?}", script); - let child = Command::new(bash_path) - .current_dir(&cwd) - .arg(package_path.join(manifest.invoke.clone())) + let env_vars = tower_uv::normalize_env_vars(&make_env_vars( + &ctx, + &cwd, + &secrets, + ¶ms, + &other_env_vars, + import_paths, + )); + + let mut cmd = Command::new(bash_path); + cmd.current_dir(&cwd) + .arg(script) .stdin(Stdio::null()) .stdout(Stdio::piped()) .stderr(Stdio::piped()) - .envs(make_env_vars( - &ctx, - &cwd, - &secrets, - ¶ms, - &other_env_vars, - import_paths, - )) - .kill_on_drop(true) - .spawn()?; + .kill_on_drop(true); - Ok(child) + // Its own process group lets cancellation (`killpg`) reach everything the script starts. + #[cfg(unix)] + { + cmd.process_group(0); + } + + if protected_mode { + cmd.env_clear(); + } + + // After env_clear, so protected mode keeps these. + cmd.envs(env_vars); + + Ok(cmd.spawn()?) } fn make_env_var_key(src: &str) -> String { @@ -608,23 +589,27 @@ async fn run_setup_child( output_sender: &OutputSender, mut child: Child, ) -> i32 { + forward_output(&mut child, Channel::Setup, output_sender); + wait_for_process(ctx.clone(), cancel_token, child).await +} + +/// Sends the child's stdout and stderr lines to `output` on `channel` until the pipes close. +fn forward_output(child: &mut Child, channel: Channel, output: &OutputSender) { let stdout = child.stdout.take().expect("no stdout"); tokio::spawn(drain_output( FD::Stdout, - Channel::Setup, - output_sender.clone(), + channel, + output.clone(), BufReader::new(stdout), )); let stderr = child.stderr.take().expect("no stderr"); tokio::spawn(drain_output( FD::Stderr, - Channel::Setup, - output_sender.clone(), + channel, + output.clone(), BufReader::new(stderr), )); - - wait_for_process(ctx.clone(), cancel_token, child).await } async fn wait_for_process( diff --git a/crates/tower-runtime/tests/example-apps/07-bash-script/Towerfile b/crates/tower-runtime/tests/example-apps/07-bash-script/Towerfile new file mode 100644 index 00000000..9cf2d05e --- /dev/null +++ b/crates/tower-runtime/tests/example-apps/07-bash-script/Towerfile @@ -0,0 +1,3 @@ +[app] +name = "07-bash-script" +script = "./run.sh" diff --git a/crates/tower-runtime/tests/example-apps/07-bash-script/run.sh b/crates/tower-runtime/tests/example-apps/07-bash-script/run.sh new file mode 100644 index 00000000..9622107b --- /dev/null +++ b/crates/tower-runtime/tests/example-apps/07-bash-script/run.sh @@ -0,0 +1,19 @@ +#!/usr/bin/env bash +set -euo pipefail + +if [ "${MODE:-}" = "sleep" ]; then + # Start a child and wait on it, so tests can check that cancelling stops it too. + sleep 300 & + echo "child pid: $!" + wait + exit 0 +fi + +echo "The greeting is: ${GREETING:-unset}" +echo "The parent variable is: ${BASH_TEST_PARENT_VARIABLE:-unset}" + +# More than a pipe buffer (64 KiB), so the script blocks unless its output is read. +pad=$(printf '%0.sx' $(seq 1 90)) +for i in $(seq 1 2000); do + echo "filler $i $pad" +done diff --git a/crates/tower-runtime/tests/local_test.rs b/crates/tower-runtime/tests/local_test.rs index fdd634cf..9e19f919 100644 --- a/crates/tower-runtime/tests/local_test.rs +++ b/crates/tower-runtime/tests/local_test.rs @@ -431,3 +431,132 @@ async fn test_running_app_with_multiple_import_paths() { status ); } + +// Regression test: the runtime looked for shell scripts at the package root while the packager +// puts them under app/, so packaged .sh apps failed to load. Their output was also never read, +// which lost it and blocked any script that filled the pipe buffer. Unix-only: the runtime looks +// for bash on PATH as `bash`, never `bash.exe`. +#[cfg(unix)] +#[tokio::test] +async fn test_running_bash_script() { + debug!("Running 07-bash-script"); + + // Protected mode must keep this out of the script's environment. + std::env::set_var("BASH_TEST_PARENT_VARIABLE", "should not reach the script"); + + let dir = get_example_app_dir("07-bash-script"); + let package = build_package_from_dir(&dir).await; + let (sender, mut receiver) = unbounded_channel(); + + let mut parameters = HashMap::new(); + parameters.insert("GREETING".to_string(), "hello from a parameter".to_string()); + + let opts = StartOptions { + ctx: tower_telemetry::Context::new("runner-id".to_string()), + package, + output_sender: sender, + cwd: None, + environment: "local".to_string(), + secrets: HashMap::new(), + parameters, + env_vars: HashMap::new(), + // No cache dir means protected mode. + cache_dir: None, + }; + + let app = LocalApp::start(opts).await.expect("Failed to start app"); + + let mut outputs = Vec::new(); + while let Some(output) = receiver.recv().await { + assert!( + matches!(output.channel, tower_runtime::Channel::Program), + "bash output should be on the program channel: {:?}", + output.line + ); + outputs.push(output.line); + } + + assert!( + outputs.contains(&"The greeting is: hello from a parameter".to_string()), + "expected the parameter in the output; got {:?}", + &outputs[..outputs.len().min(5)] + ); + assert!( + outputs.contains(&"The parent variable is: unset".to_string()), + "protected mode should not pass the host environment through" + ); + assert_eq!( + outputs + .iter() + .filter(|line| line.starts_with("filler ")) + .count(), + 2000 + ); + + let status = app.status().await.expect("Failed to get app status"); + assert!( + status == Status::Exited, + "App should have exited cleanly, got {:?}", + status + ); +} + +// Cancelling a bash app must stop the processes it started, not only bash itself. +#[cfg(unix)] +#[tokio::test] +async fn test_terminating_bash_script_stops_its_children() { + use nix::{sys::signal::kill, unistd::Pid}; + + let dir = get_example_app_dir("07-bash-script"); + let package = build_package_from_dir(&dir).await; + let (sender, mut receiver) = unbounded_channel(); + + let mut parameters = HashMap::new(); + parameters.insert("MODE".to_string(), "sleep".to_string()); + + let opts = StartOptions { + ctx: tower_telemetry::Context::new("runner-id".to_string()), + package, + output_sender: sender, + cwd: None, + environment: "local".to_string(), + secrets: HashMap::new(), + parameters, + env_vars: HashMap::new(), + cache_dir: None, + }; + + let mut app = LocalApp::start(opts).await.expect("Failed to start app"); + + let child_pid = loop { + let output = receiver + .recv() + .await + .expect("script exited before reporting its child"); + if let Some(pid) = output.line.strip_prefix("child pid: ") { + break Pid::from_raw(pid.trim().parse().expect("child pid should be a number")); + } + }; + + app.terminate().await.expect("Failed to terminate app"); + let status = app.status().await.expect("Failed to get app status"); + assert!( + status == Status::Cancelled, + "App should be cancelled, got {:?}", + status + ); + + // The orphaned child is reaped by init shortly after it dies. + let mut alive = true; + for _ in 0..50 { + if kill(child_pid, None).is_err() { + alive = false; + break; + } + tokio::time::sleep(std::time::Duration::from_millis(100)).await; + } + if alive { + let _ = kill(child_pid, nix::sys::signal::Signal::SIGKILL); + } + assert!(!alive, "the script's child should have been stopped"); +} diff --git a/crates/tower-uv/src/lib.rs b/crates/tower-uv/src/lib.rs index 7af6006c..20ff084e 100644 --- a/crates/tower-uv/src/lib.rs +++ b/crates/tower-uv/src/lib.rs @@ -58,7 +58,9 @@ impl From for Error { } } -fn normalize_env_vars(env_vars: &HashMap) -> HashMap { +/// Adds the host variables a child process needs (PATH, HOME, temp dirs, TZ, LANG, ...) to +/// `env_vars`. Use it for any app process, since protected mode clears everything else. +pub fn normalize_env_vars(env_vars: &HashMap) -> HashMap { // we copy this locally so we can mutate the results. let mut env_vars = env_vars.clone();