From 556fe8c3223facc8340848cb6f7efa7079f71f4e Mon Sep 17 00:00:00 2001 From: queil <4584075+queil@users.noreply.github.com> Date: Wed, 15 Jul 2026 08:32:55 +0000 Subject: [PATCH] fix: various exec bugs --- .github/workflows/integration.yml | 1 + CLAUDE.md | 2 +- src/api/config.rs | 2 +- src/api/exec.rs | 84 +++++++-------- src/api/system_config.rs | 2 +- tests/interactive.rs | 167 ++++++++++++++++++++++++++++++ tests/volumes.rs | 25 ++++- 7 files changed, 232 insertions(+), 51 deletions(-) create mode 100644 tests/interactive.rs diff --git a/.github/workflows/integration.yml b/.github/workflows/integration.yml index 56f91ea..b5d0d41 100644 --- a/.github/workflows/integration.yml +++ b/.github/workflows/integration.yml @@ -66,6 +66,7 @@ jobs: --test volumes \ --test sidecars \ --test networks \ + --test interactive \ -- --test-threads=1 - name: Stop daemon diff --git a/CLAUDE.md b/CLAUDE.md index a72b1c8..6a77dce 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -14,7 +14,7 @@ This workspace runs in a restricted container (rooz with podman backend): Run locally, straight against `dkr` (no dind): ```sh -cargo test --test smoke --test lifecycle --test volumes --test sidecars --test networks -- --test-threads=1 +cargo test --test smoke --test lifecycle --test volumes --test sidecars --test networks --test interactive -- --test-threads=1 ``` This works because a gitignored `.cargo/config.toml` supplies the test env. Recreate it if missing: diff --git a/src/api/config.rs b/src/api/config.rs index d94abfe..3d7a261 100644 --- a/src/api/config.rs +++ b/src/api/config.rs @@ -82,7 +82,7 @@ impl<'a> ConfigApi<'a> { .one_shot_output( "read-workspace-config", format!( - "ls /etc/rooz/{} > /dev/null 2>&1 && cat /etc/rooz/{} || echo ''", + "ls /etc/rooz/{} > /dev/null 2>&1 && cat /etc/rooz/{} || true", config_path, config_path ) .into(), diff --git a/src/api/exec.rs b/src/api/exec.rs index fd16cc4..6d998ff 100644 --- a/src/api/exec.rs +++ b/src/api/exec.rs @@ -18,19 +18,25 @@ use tokio::{ }; async fn collect(stream: impl Stream>) -> Result { - let out = stream - .map(|x| match x { - Ok(r) => std::str::from_utf8(r.into_bytes().as_ref()) - .unwrap() - .to_string(), - Err(err) => panic!("{}", err), - }) - .collect::>() - .await - .join(""); - - let trimmed = out.trim(); - Ok(trimmed.to_string()) + let mut stream = std::pin::pin!(stream); + let mut out: Vec = Vec::new(); + + while let Some(item) = stream.next().await { + match item { + // IMPORTANT: stdout is returned byte-exact (no trimming, no tty + // CRLF translation) so generated file content round-trips + Ok(LogOutput::StdOut { message }) | Ok(LogOutput::Console { message }) => { + out.extend_from_slice(&message) + } + Ok(LogOutput::StdErr { message }) => { + log::debug!("stderr | {}", String::from_utf8_lossy(&message).trim_end()) + } + Ok(_) => {} + Err(err) => return Err(err.into()), + } + } + + Ok(String::from_utf8(out)?) } async fn log(stream: impl Stream>) -> Result<(), AnyError> { @@ -154,6 +160,7 @@ impl<'a> ExecApi<'a> { working_dir: Option<&str>, user: Option<&str>, cmd: Option>, + interactive: bool, ) -> Result { #[cfg(not(windows))] { @@ -171,8 +178,8 @@ impl<'a> ExecApi<'a> { CreateExecOptions { attach_stdout: Some(true), attach_stderr: Some(true), - attach_stdin: Some(true), - tty: Some(true), + attach_stdin: Some(interactive), + tty: Some(interactive), cmd, working_dir, user, @@ -193,7 +200,7 @@ impl<'a> ExecApi<'a> { cmd: Option>, ) -> Result<(), AnyError> { let exec_id = self - .create_exec(reason, container_id, working_dir, user, cmd) + .create_exec(reason, container_id, working_dir, user, cmd, true) .await?; self.start_tty(&exec_id).await @@ -207,7 +214,7 @@ impl<'a> ExecApi<'a> { cmd: Option>, ) -> Result { let exec_id = self - .create_exec(reason, container_id, None, user, cmd) + .create_exec(reason, container_id, None, user, cmd, false) .await?; if let StartExecResults::Attached { output, .. } = self.client.start_exec(&exec_id, None).await? @@ -226,16 +233,25 @@ impl<'a> ExecApi<'a> { cmd: Option>, ) -> Result<(), AnyError> { let exec_id = self - .create_exec(reason, container_id, None, user, cmd) + .create_exec(reason, container_id, None, user, cmd, false) .await?; if let StartExecResults::Attached { output, .. } = self.client.start_exec(&exec_id, None).await? { log(output).await?; - Ok(()) } else { panic!("Could not start exec"); } + + if let ExecInspectResponse { + exit_code: Some(exit_code), + .. + } = self.client.inspect_exec(&exec_id).await? + && exit_code != 0 + { + return Err(format!("{}: exec failed with exit code {}", reason, exit_code).into()); + } + Ok(()) } pub async fn install( @@ -265,31 +281,11 @@ echo '[install] {}: {}' ); let install_cmd = inject(cmd.as_str(), &format!("install-{}.sh", idx)); let v = install_cmd.iter().map(|x| x.as_str()).collect::>(); - let exec_id = self - .create_exec( - "install", - container_id, - None, - Some(constants::ROOT_UID), - Some(v), - ) - .await?; - self.start_tty(&exec_id).await.map_err(|e| -> AnyError { - format!("install step '{}' failed: {}", name, e).into() - })?; - if let ExecInspectResponse { - exit_code: Some(exit_code), - .. - } = self.client.inspect_exec(&exec_id).await? - { - if exit_code != 0 { - return Err(format!( - "install step '{}' failed with exit code {}", - name, exit_code - ) - .into()); - } - } + self.run("install", container_id, Some(constants::ROOT_UID), Some(v)) + .await + .map_err(|e| -> AnyError { + format!("install step '{}' failed: {}", name, e).into() + })?; } Ok(()) } diff --git a/src/api/system_config.rs b/src/api/system_config.rs index adcbe3f..8d1c49c 100644 --- a/src/api/system_config.rs +++ b/src/api/system_config.rs @@ -12,7 +12,7 @@ impl<'a> Api<'a> { .container .one_shot_output( "read-sys-config", - "ls /tmp/sys/rooz.config > /dev/null 2>&1 && cat /tmp/sys/rooz.config || echo ''" + "ls /tmp/sys/rooz.config > /dev/null 2>&1 && cat /tmp/sys/rooz.config || true" .into(), Some(vec![ RoozVolume::system_config_read("/tmp/sys").to_mount(None), diff --git a/tests/interactive.rs b/tests/interactive.rs new file mode 100644 index 0000000..ad184ee --- /dev/null +++ b/tests/interactive.rs @@ -0,0 +1,167 @@ +mod harness; + +use assert_cmd::cargo::cargo_bin; +use harness::{TestEnv, unique_key}; +use std::{ + fs, + io::Write, + process::{Command, Stdio}, +}; + +fn write_cfg(key: &str, yaml: &str) -> String { + let path = format!("/tmp/rooz-test-{}.yaml", key); + let mut f = fs::File::create(&path).expect("write config"); + f.write_all(yaml.as_bytes()).unwrap(); + path +} + +fn cleanup(env: &TestEnv, key: &str, cfg_path: &str) { + env.rooz().args(["rm", key, "--force"]).assert().success(); + let _ = fs::remove_file(cfg_path); +} + +fn has_script() -> bool { + Command::new("script") + .arg("--version") + .output() + .map(|o| o.status.success()) + .unwrap_or(false) +} + +/// Runs rooz under a real PTY (via util-linux `script`), feeding `input` to the +/// terminal. Returns (combined pty output, success). +fn pty_rooz(env: &TestEnv, args: &[&str], input: &str) -> (String, bool) { + let cmd_line = format!( + "env DOCKER_HOST={} HTTP_PROXY= http_proxy= {} {}", + env.docker_host, + cargo_bin("rooz").display(), + args.join(" ") + ); + let mut child = Command::new("timeout") + .args(["120", "script", "-qec", &cmd_line, "/dev/null"]) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .expect("spawn script"); + child + .stdin + .take() + .unwrap() + .write_all(input.as_bytes()) + .unwrap(); + let out = child.wait_with_output().expect("wait for script"); + ( + String::from_utf8_lossy(&out.stdout).to_string(), + out.status.success(), + ) +} + +// ── install steps (non-interactive exec during rooz new) ──────────────────── + +#[tokio::test] +async fn install_steps_run_in_order() { + let Some(env) = TestEnv::from_env() else { + return; + }; + let key = unique_key("int-inst"); + let cfg_path = write_cfg( + &key, + "image: alpine:latest\ninstall:\n 01-first: echo first > /work/install-marker\n 02-second: echo second >> /work/install-marker\n", + ); + + env.rooz() + .args(["system", "init", "--force"]) + .assert() + .success(); + // headless on purpose: install must not require a controlling terminal + env.rooz() + .args(["new", &key, "--config", &cfg_path]) + .assert() + .success(); + + let work_vol = format!("rooz-{}-work", key); + assert_eq!( + env.volume_file(&work_vol, "install-marker").await, + "first\nsecond\n", + "install steps did not run in order" + ); + + cleanup(&env, &key, &cfg_path); +} + +#[tokio::test] +async fn failing_install_step_fails_new() { + let Some(env) = TestEnv::from_env() else { + return; + }; + let key = unique_key("int-fail"); + let cfg_path = write_cfg( + &key, + "image: alpine:latest\ninstall:\n 01-boom: \"exit 7\"\n", + ); + + env.rooz() + .args(["system", "init", "--force"]) + .assert() + .success(); + + let output = env + .rooz() + .args(["new", &key, "--config", &cfg_path]) + .output() + .expect("run rooz new"); + + assert!( + !output.status.success(), + "rooz new must fail when an install step fails" + ); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("install step '01-boom' failed"), + "unexpected error output: {}", + stderr + ); + + cleanup(&env, &key, &cfg_path); +} + +// ── enter (interactive tty exec) ───────────────────────────────────────────── + +#[tokio::test] +async fn enter_runs_interactive_shell() { + let Some(env) = TestEnv::from_env() else { + return; + }; + if !has_script() { + eprintln!("skipping: util-linux 'script' not available for pty allocation"); + return; + } + let key = unique_key("int-enter"); + + env.rooz() + .args(["system", "init", "--force"]) + .assert() + .success(); + env.rooz() + .args(["new", &key, "--image", "alpine:latest"]) + .assert() + .success(); + + // the marker is computed in the shell so a match proves the command ran + // (the typed command line echoes back on the pty as well) + let (output, success) = pty_rooz( + &env, + &["enter", &key, "--shell", "sh"], + "echo pty-marker-$((6*7))\nexit\n", + ); + + assert!(success, "rooz enter failed, pty output:\n{}", output); + assert!( + output.contains("pty-marker-42"), + "shell did not evaluate the marker command, pty output:\n{}", + output + ); + + env.rooz().args(["rm", &key, "--force"]).assert().success(); +} diff --git a/tests/volumes.rs b/tests/volumes.rs index c78ec45..efb36bb 100644 --- a/tests/volumes.rs +++ b/tests/volumes.rs @@ -342,10 +342,27 @@ async fn generated_multiline_data_file() { let vol = format!("rooz-{}-genml", key); let content = env.volume_file(&vol, "genml.data").await; - // Pins current behavior: generator output is captured through a tty exec - // (LF becomes CRLF) and trimmed (trailing EOL lost). Inline content is - // preserved byte-exact; generated content is not. - assert_eq!(content, "l1\r\nl2"); + // generated content must round-trip byte-exact, same as inline content + assert_eq!(content, "l1\nl2\n"); + + cleanup(&env, &key, &cfg_path); +} + +#[tokio::test] +async fn generated_data_file_excludes_stderr() { + let Some(env) = TestEnv::from_env() else { + return; + }; + let key = unique_key("vol-generr"); + let cfg_path = write_cfg( + &key, + "data:\n gen:\n generate: echo warning >&2 && printf clean-output\nmounts:\n ~/gen: gen\n", + ); + + new_workspace(&env, &key, &cfg_path); + + let vol = format!("rooz-{}-gen", key); + assert_eq!(env.volume_file(&vol, "gen.data").await, "clean-output"); cleanup(&env, &key, &cfg_path); }