From 959370a6e7463570e0b039c0a826cf62cc818f64 Mon Sep 17 00:00:00 2001 From: Miguel Miranda Date: Wed, 4 Nov 2026 16:31:54 +0000 Subject: [PATCH] fix: run packaged shell scripts and capture their output Since bundle v2 the packager puts every app file under app/, but the runtime still resolved .sh entry points from the package root, so every packaged shell app failed to load with MissingScript. Shell scripts now resolve from the app directory, falling back to the package root. The bash path also never read the script's stdout or stderr, losing its output and blocking any script that filled the pipe buffer. It now forwards both to the program channel, runs in its own process group so cancellation stops the script's children, and clears the environment in protected mode, as uv run does. Co-Authored-By: Claude Opus 5.5 --- crates/tower-package/src/native.rs | 68 ++++++--- crates/tower-runtime/src/local.rs | 123 ++++++++--------- .../example-apps/07-bash-script/Towerfile | 3 + .../tests/example-apps/07-bash-script/run.sh | 19 +++ crates/tower-runtime/tests/local_test.rs | 129 ++++++++++++++++++ crates/tower-uv/src/lib.rs | 4 +- 6 files changed, 255 insertions(+), 91 deletions(-) create mode 100644 crates/tower-runtime/tests/example-apps/07-bash-script/Towerfile create mode 100644 crates/tower-runtime/tests/example-apps/07-bash-script/run.sh 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();