collective
Browse / github.com/storytold/gridcraft / patch
CLI: finish workbook saves after stdout closes
CLI: finish workbook saves after stdout closes When a reader closes stdout during `gridcraft-cli run`, the CLI exits successfully before applying later commands or writing `--out`. Suppress further output after a broken pipe so the requested work finishes, and return failure if a later command or save fails. Other stdout errors still fail. The help text explains that closing stdout no longer cancels work. Handle a closed stderr pipe as well: when stdout and stderr share the closed reader, diagnostics no longer panic after a successful save or mask the exit status of failed work. Buffer each result line before writing it so short lines ret
- Status
- open
- Score
- 0.00
- Adopters
- 0 other operators
- Author
- DAnaeto (imported from pull request #2723, open)
- Base
45f5ab18e4e57bf50ee88d2526deb4b87c5b24cb- Size
- +153 −19 in 4 files
- Review
- pending
- Shared
- 2026-10-28
- Id
18b5f5b922dccba4b5b76d13
How it went
No other operator has reported using it yet.
Diff
raw · apps/gridcraft-cli/src/check.rs apps/gridcraft-cli/src/main.rs apps/gridcraft-cli/tests/it/cli.rs docs/cli.md
@@ -191,7 +191,7 @@ impl Diffs { /// every checked formula matches and every round trip is clean; 1 otherwise; 2 on a usage error. pub fn main(args: &[String]) -> ExitCode { let usage = |e: String| {- eprintln!("gridcraft-cli: {e}\n{USAGE}");+ errln!("gridcraft-cli: {e}\n{USAGE}"); ExitCode::from(2) }; let a = match Args::parse(args, &["tolerance", "max-report"], &["json"]) {@@ -17,18 +17,32 @@ #![forbid(unsafe_code)] use std::process::ExitCode;+use std::sync::atomic::{AtomicBool, Ordering}; -/// `println!` that ends the program quietly when stdout is closed (`gridcraft-cli commands |-/// head`) instead of panicking on a broken pipe.+static STDOUT_CLOSED: AtomicBool = AtomicBool::new(false);++/// `println!` that suppresses further output when a reader closes stdout, while allowing work to finish. macro_rules! outln { ($($arg:tt)*) => {{- use std::io::Write as _;- if let Err(e) = writeln!(std::io::stdout(), $($arg)*) {+ if !$crate::STDOUT_CLOSED.load(std::sync::atomic::Ordering::Relaxed)+ && let Err(e) = $crate::write_stdout(format_args!("{}\n", format_args!($($arg)*)))+ { $crate::stdout_failed(e); } }}; } +macro_rules! errln {+ ($($arg:tt)*) => {{+ use std::io::Write as _;+ if let Err(e) = writeln!(std::io::stderr().lock(), $($arg)*)+ && e.kind() != std::io::ErrorKind::BrokenPipe+ {+ std::process::exit(1);+ }+ }};+}+ mod check; use gridcraft_engine::Session;@@ -45,20 +59,38 @@ const USAGE: &str = "usage: [--out FILE] [--print RANGE] [--quiet] a script line is id={json} or {\"command\": \"id\", \"params\": {…}}; blank lines and lines starting with # or // are skipped+ closing stdout does not cancel work; cancel the process to stop it gridcraft-cli commands [--json] [--search X] gridcraft-cli functions [--json] [--search X] [--category C] gridcraft-cli mcp [--connect PORT] [--in FILE | --sample NAME] gridcraft-cli send <port> <method> [json] gridcraft-cli version"; -/// stdout went away. A reader that stopped early (a closed pipe) ends the program quietly; any-/// other write error is reported.-fn stdout_failed(e: std::io::Error) -> ! {+fn write_stdout(args: std::fmt::Arguments<'_>) -> std::io::Result<()> {+ use std::io::Write as _;+ #[cfg(unix)]+ {+ // Stdout suppresses EBADF, so write through a duplicate to report invalid destinations.+ use std::os::fd::AsFd as _;+ let stdout = std::io::stdout();+ std::fs::File::from(stdout.as_fd().try_clone_to_owned()?).write_all(args.to_string().as_bytes())+ }+ #[cfg(not(unix))]+ {+ let stdout = std::io::stdout();+ let mut out = stdout.lock();+ out.write_fmt(args).and_then(|()| out.flush())+ }+}++/// Stop writing after a reader closes the pipe; report any other write error.+fn stdout_failed(e: std::io::Error) { if e.kind() == std::io::ErrorKind::BrokenPipe {- std::process::exit(0);+ STDOUT_CLOSED.store(true, Ordering::Relaxed);+ } else {+ errln!("gridcraft-cli: can't write to stdout: {e}");+ std::process::exit(1); }- eprintln!("gridcraft-cli: can't write to stdout: {e}");- std::process::exit(1); } fn main() -> ExitCode {@@ -89,7 +121,7 @@ fn main() -> ExitCode { match r { Ok(()) => ExitCode::SUCCESS, Err(e) => {- eprintln!("gridcraft-cli: {e}");+ errln!("gridcraft-cli: {e}"); ExitCode::FAILURE } }@@ -159,7 +191,7 @@ fn exec(s: &mut Session, id: &str, params: Value) -> Result<Value, String> { && let Some(warnings) = value.get("warnings").and_then(Value::as_array) { for warning in warnings.iter().filter_map(Value::as_str) {- eprintln!("Warning: {warning}");+ errln!("Warning: {warning}"); } } r@@ -292,13 +324,13 @@ fn convert(args: &[String]) -> Result<(), String> { activate_sheet(&mut s, a.opt("sheet"))?; if pdf { let result = exec(&mut s, "file.exportPdf", json!({"path": out}))?;- eprintln!("wrote {out} ({} bytes, {} pages)", result["bytes"], result["pages"]);+ errln!("wrote {out} ({} bytes, {} pages)", result["bytes"], result["pages"]); return Ok(()); } let d = s.doc().map_err(|e| e.to_string())?; let bytes = gridcraft_engine::io::save_file_bytes(&d.wb, out).map_err(|e| e.to_string())?; gridcraft_engine::io::write_file(out, &bytes).map_err(|e| e.to_string())?;- eprintln!("wrote {out} ({} bytes)", bytes.len());+ errln!("wrote {out} ({} bytes)", bytes.len()); Ok(()) } @@ -451,7 +483,7 @@ fn run(args: &[String]) -> Result<(), String> { } if let Some(out) = a.opt("out") { let r = exec(&mut s, "file.save", json!({"path": out}))?;- eprintln!("wrote {out} ({} bytes)", r["bytes"]);+ errln!("wrote {out} ({} bytes)", r["bytes"]); } for (name, range) in &a.opts { if name == "print" {@@ -491,7 +523,7 @@ fn commands(args: &[String]) -> Result<(), String> { outln!("{:<w$} {}{menu}{sc}", c.id, c.label.trim_end_matches('…')); outln!("{:<w$} {}", "", c.params); }- eprintln!("{} commands", list.len());+ errln!("{} commands", list.len()); Ok(()) } @@ -518,7 +550,7 @@ fn functions(args: &[String]) -> Result<(), String> { let desc = f["description"].as_str().unwrap_or(""); outln!(" {cat}: {desc}"); }- eprintln!("{} functions", list.len());+ errln!("{} functions", list.len()); Ok(()) } @@ -549,7 +581,7 @@ fn mcp(args: &[String]) -> Result<(), String> { Box::new(Headless::with_session(session)) } };- eprintln!("gridcraft-cli: MCP server on stdio ({})", backend.describe());+ errln!("gridcraft-cli: MCP server on stdio ({})", backend.describe()); let stdin = std::io::stdin(); let stdout = std::io::stdout(); Server::new(backend).serve(stdin.lock(), stdout.lock()).map_err(|e| e.to_string())@@ -22,6 +22,105 @@ fn tmpdir(name: &str) -> PathBuf { d } +#[cfg(unix)]+fn run_with_closed_stdout(args: &[&str]) -> Output {+ // Hold the shell until the read end is closed, so the first result always sees EPIPE.+ let mut child = Command::new("sh")+ .args(["-c", "IFS= read -r ready; exec \"$@\"", "sh", env!("CARGO_BIN_EXE_gridcraft-cli")])+ .args(args)+ .stdin(Stdio::piped())+ .stdout(Stdio::piped())+ .stderr(Stdio::piped())+ .spawn()+ .unwrap();+ drop(child.stdout.take());+ writeln!(child.stdin.take().unwrap(), "go").unwrap();+ child.wait_with_output().unwrap()+}++#[cfg(unix)]+#[test]+fn broken_stdout_still_runs_later_commands_and_saves_workbook() {+ let dir = tmpdir("broken-stdout-save");+ let book = dir.join("out.xlsx");+ let out = run_with_closed_stdout(&[+ "run",+ "--cmd",+ r#"cell.set={"cell":"A1","input":"first"}"#,+ "--cmd",+ r#"cell.set={"cell":"A2","input":"saved"}"#,+ "--out",+ book.to_str().unwrap(),+ ]);+ assert!(out.status.success(), "{}", String::from_utf8_lossy(&out.stderr));+ assert!(book.exists(), "workbook was not saved");+ assert_eq!(ok(&["cat", book.to_str().unwrap(), "--range", "A1:A2"]).split_whitespace().collect::<Vec<_>>(), ["first", "saved"]);+ let _ = std::fs::remove_dir_all(dir);+}++#[cfg(unix)]+#[test]+fn broken_stdout_does_not_hide_later_work_or_save_errors() {+ let dir = tmpdir("broken-stdout-errors");+ let book = dir.join("missing").join("out.xlsx");+ let fail_save = run_with_closed_stdout(&["run", "--cmd", r#"cell.set={"cell":"A1","input":"first"}"#, "--out", book.to_str().unwrap()]);+ assert_eq!(fail_save.status.code(), Some(1), "save failure must return 1 without panicking");+ assert!(!fail_save.stderr.is_empty(), "save failure must include a diagnostic");+ let fail_command = run_with_closed_stdout(&["run", "--cmd", r#"cell.set={"cell":"A1","input":"first"}"#, "--cmd", "no.such.command"]);+ assert_eq!(fail_command.status.code(), Some(1), "later command failure must return 1 without panicking");+ assert!(String::from_utf8_lossy(&fail_command.stderr).contains("no.such.command"));+ let _ = std::fs::remove_dir_all(dir);+}++#[cfg(unix)]+#[test]+fn closed_stdout_and_stderr_do_not_hide_successful_save() {+ use std::os::fd::OwnedFd;+ use std::os::unix::net::UnixStream;++ let dir = tmpdir("closed-both-save");+ let book = dir.join("out.xlsx");+ let (writer, reader) = UnixStream::pair().unwrap();+ let error_writer = writer.try_clone().unwrap();+ drop(reader);+ let status = bin()+ .args(["run", "--cmd", r#"cell.set={"cell":"A1","input":"saved"}"#, "--out", book.to_str().unwrap()])+ .stdout(Stdio::from(OwnedFd::from(writer)))+ .stderr(Stdio::from(OwnedFd::from(error_writer)))+ .status()+ .unwrap();+ assert!(status.success(), "successful save must not fail after the reader closes: {status}");+ assert_eq!(ok(&["cat", book.to_str().unwrap(), "--range", "A1"]).trim(), "saved");+ let _ = std::fs::remove_dir_all(dir);+}++#[cfg(unix)]+#[test]+fn closed_stdout_and_stderr_still_report_work_failure() {+ use std::os::fd::OwnedFd;+ use std::os::unix::net::UnixStream;++ let (writer, reader) = UnixStream::pair().unwrap();+ let error_writer = writer.try_clone().unwrap();+ drop(reader);+ let status = bin()+ .args(["run", "--cmd", r#"cell.set={"cell":"A1","input":"first"}"#, "--cmd", "no.such.command"])+ .stdout(Stdio::from(OwnedFd::from(writer)))+ .stderr(Stdio::from(OwnedFd::from(error_writer)))+ .status()+ .unwrap();+ assert_eq!(status.code(), Some(1), "failed work should exit 1 without panicking");+}++#[cfg(unix)]+#[test]+fn stdout_errors_other_than_broken_pipe_fail() {+ let read_only = std::fs::File::open(env!("CARGO_BIN_EXE_gridcraft-cli")).unwrap();+ let out = bin().arg("version").stdout(Stdio::from(read_only)).output().unwrap();+ assert!(!out.status.success());+ assert!(String::from_utf8_lossy(&out.stderr).contains("can't write to stdout"));+}+ #[test] fn version_and_usage() { assert_eq!(ok(&["--version"]).trim(), format!("gridcraft-cli {}", env!("CARGO_PKG_VERSION")));@@ -45,6 +45,9 @@ gridcraft-cli run [--in FILE | --sample budget|sales|grades] \ Opens a file (or a sample, or a blank workbook), runs `--cmd` and `--script` steps in the order given, then saves `--out` and prints each `--print` range. Each non-null command result is printed as a JSON line unless `--quiet`. The first failing step stops the run with a non-zero exit.+If the reader of a stdout pipe closes early, printed output stops but the run continues, including later steps and a+requested `--out` save. The exit status is 0 if the work succeeds, or non-zero if it fails. Other stdout write errors+stop execution with a non-zero exit. Stop the process to cancel the run. A `--cmd` is `id`, `id={json}` or `id {json}`. A script has one command per line in the same forms, or as JSON objects `{"command": "cell.set", "params": {...}}`; blank lines and `#` comments are skipped.