From 1a31a9f54143a7e35b3840b6a41adb9aab5d5af9 Mon Sep 17 00:00:00 2001 From: Fintan Halpenny Date: Thu, 2 Apr 2026 16:57:28 +0000 Subject: [PATCH] cli: Handle broken pipe (SIGPIPE) gracefully Prevent `rad` from panicking when its stdout is a closed pipe, e.g. when running `rad config | head`. The broken pipes are handled at the output boundaries using three layers of defence: 1. Inner: Route all stdout output through `term::print()` and `term::print_inline()`, which use explicit `writeln!`/`write!` to a locked stdout and silently ignore write errors. All `println!` calls in `radicle-cli` and `radicle-term` are converted. 2. Middle: Catch `BrokenPipe` errors that propagate up via `anyhow` from subcommands in `main::run()`, and exit cleanly with code 0. 3. Outer: Install a panic hook that intercepts `println!` panics containing "Broken pipe" (from dependencies like clap) and exits cleanly, chained in front of human_panic's hook. To prevent regressions, `#![deny(clippy::print_stdout)]` is added to `radicle-cli`, requiring all future stdout output to go through the safe `term::` functions. Note: `eprintln!` calls (stderr) are not handled, as broken stderr pipes are extremely rare in practice. --- Cargo.lock | 1 + Cargo.toml | 1 + crates/radicle-cli/Cargo.toml | 3 +- crates/radicle-cli/src/commands/cob.rs | 6 +- crates/radicle-cli/src/commands/cob/args.rs | 1 - crates/radicle-cli/src/commands/debug.rs | 2 +- crates/radicle-cli/src/commands/id.rs | 4 +- crates/radicle-cli/src/commands/inspect.rs | 40 +++---- crates/radicle-cli/src/commands/node.rs | 2 +- .../radicle-cli/src/commands/node/control.rs | 6 +- .../radicle-cli/src/commands/node/events.rs | 4 +- .../radicle-cli/src/commands/node/routing.rs | 2 +- crates/radicle-cli/src/commands/path.rs | 2 +- crates/radicle-cli/src/lib.rs | 3 + crates/radicle-cli/src/main.rs | 102 ++++++++++++++++++ crates/radicle-node/Cargo.toml | 2 +- 16 files changed, 145 insertions(+), 36 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 007941db..acb72672 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3022,6 +3022,7 @@ name = "radicle-cli" version = "0.20.0" dependencies = [ "anyhow", + "backtrace", "chrono", "clap", "clap_complete", diff --git a/Cargo.toml b/Cargo.toml index bc6589c1..059569b7 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -20,6 +20,7 @@ rust-version = "1.85.0" [workspace.dependencies] amplify = { version = "4.0.0", default-features = false } +backtrace = "0.3.75" bstr = "1.3" bytes = "1.11.1" chrono = { version = "0.4.26", default-features = false } diff --git a/crates/radicle-cli/Cargo.toml b/crates/radicle-cli/Cargo.toml index ddc019e3..624a7588 100644 --- a/crates/radicle-cli/Cargo.toml +++ b/crates/radicle-cli/Cargo.toml @@ -14,7 +14,7 @@ name = "rad" path = "src/main.rs" [features] -default = ["i2p", "tor"] +default = ["backtrace", "i2p","tor"] i2p = ["radicle/i2p"] tor = ["radicle/tor"] @@ -57,6 +57,7 @@ tree-sitter-toml-ng = "0.6.0" tree-sitter-typescript = "0.23.2" [target.'cfg(unix)'.dependencies] +backtrace = { workspace = true, optional = true } shlex = { workspace = true } [target.'cfg(windows)'.dependencies] diff --git a/crates/radicle-cli/src/commands/cob.rs b/crates/radicle-cli/src/commands/cob.rs index 3a7a818a..ebf45a3e 100644 --- a/crates/radicle-cli/src/commands/cob.rs +++ b/crates/radicle-cli/src/commands/cob.rs @@ -80,7 +80,7 @@ pub fn run(args: Args, ctx: impl term::Context) -> anyhow::Result<()> { oid } }; - println!("{oid}"); + term::print(oid); } Migrate => { let mut db = profile.cobs_db_mut()?; @@ -100,7 +100,7 @@ pub fn run(args: Args, ctx: impl term::Context) -> anyhow::Result<()> { FilteredTypeName::from(type_name).as_ref(), )?; for cob in cobs { - println!("{}", cob.id); + term::print(cob.id); } } Log { @@ -215,7 +215,7 @@ pub fn run(args: Args, ctx: impl term::Context) -> anyhow::Result<()> { } }; - println!("{oid}"); + term::print(oid); } } Ok(()) diff --git a/crates/radicle-cli/src/commands/cob/args.rs b/crates/radicle-cli/src/commands/cob/args.rs index fcada46a..078ecedf 100644 --- a/crates/radicle-cli/src/commands/cob/args.rs +++ b/crates/radicle-cli/src/commands/cob/args.rs @@ -400,7 +400,6 @@ mod test { .chain(ARGS.iter()) .collect::>(), ); - println!("{args:?}"); assert!(args.is_ok()) } diff --git a/crates/radicle-cli/src/commands/debug.rs b/crates/radicle-cli/src/commands/debug.rs index 2564ddbc..c3750d76 100644 --- a/crates/radicle-cli/src/commands/debug.rs +++ b/crates/radicle-cli/src/commands/debug.rs @@ -60,7 +60,7 @@ fn debug(profile: Option<&Profile>) -> anyhow::Result<()> { warnings: collect_warnings(profile), }; - println!("{}", serde_json::to_string_pretty(&debug).unwrap()); + term::print(serde_json::to_string_pretty(&debug).unwrap()); Ok(()) } diff --git a/crates/radicle-cli/src/commands/id.rs b/crates/radicle-cli/src/commands/id.rs index 03f5e6f6..ab8683e8 100644 --- a/crates/radicle-cli/src/commands/id.rs +++ b/crates/radicle-cli/src/commands/id.rs @@ -373,7 +373,7 @@ fn print( profile: &Profile, ) -> anyhow::Result<()> { print_meta(revision, previous, profile)?; - println!(); + term::blank(); print_diff(revision.parent.as_ref(), &revision.id, repo)?; Ok(()) @@ -514,7 +514,7 @@ fn print_diff( if let Some(modified) = diff.modified().next() { let diff = modified.diff.to_unified_string()?; - print!("{diff}"); + term::print_inline(diff); } else { term::print(term::format::italic("No changes.")); } diff --git a/crates/radicle-cli/src/commands/inspect.rs b/crates/radicle-cli/src/commands/inspect.rs index c3bd8ba9..fcf442c2 100644 --- a/crates/radicle-cli/src/commands/inspect.rs +++ b/crates/radicle-cli/src/commands/inspect.rs @@ -68,7 +68,7 @@ pub fn run(args: Args, ctx: impl term::Context) -> anyhow::Result<()> { let refs = RefsAt::new(&repo, remote)?; let sigrefs = SignedRefs::load_at(refs.at, remote, &repo); - println!( + term::print(format_args!( "{:<48} {} {}", term::format::tertiary(remote.to_human()), term::format::secondary(refs.at), @@ -103,7 +103,7 @@ pub fn run(args: Args, ctx: impl term::Context) -> anyhow::Result<()> { term::format::negative("missing".to_string()) } } - ); + )); } } Target::Policy => { @@ -111,19 +111,19 @@ pub fn run(args: Args, ctx: impl term::Context) -> anyhow::Result<()> { let seed = policies.seed_policy(&rid)?; match seed.policy { SeedingPolicy::Allow { scope } => { - println!( + term::print(format_args!( "Repository {} is {} with scope {}", term::format::tertiary(&rid), term::format::positive("being seeded"), term::format::dim(format!("`{scope}`")) - ); + )); } SeedingPolicy::Block => { - println!( + term::print(format_args!( "Repository {} is {}", term::format::tertiary(&rid), term::format::negative("not being seeded"), - ); + )); } } } @@ -132,19 +132,19 @@ pub fn run(args: Args, ctx: impl term::Context) -> anyhow::Result<()> { let aliases = profile.aliases(); for did in doc.delegates().iter() { if let Some(alias) = aliases.alias(did) { - println!( + term::print(format_args!( "{} {}", term::format::tertiary(&did), term::format::parens(term::format::dim(alias)) - ); + )); } else { - println!("{}", term::format::tertiary(&did)); + term::print(term::format::tertiary(&did)); } } } Target::Visibility => { let (_, doc) = repo(rid, storage)?; - println!("{}", term::format::visibility(doc.visibility())); + term::print(term::format::visibility(doc.visibility())); } Target::History => { let (repo, _) = repo(rid, storage)?; @@ -177,22 +177,22 @@ pub fn run(args: Args, ctx: impl term::Context) -> anyhow::Result<()> { .with_timezone(&timezone) .to_rfc2822(); - println!( + term::print(format_args!( "{} {}", term::format::yellow("commit"), term::format::yellow(oid), - ); + )); if let Ok(parent) = tip.parent_id(0) { - println!("parent {parent}"); + term::print(format_args!("parent {parent}")); } - println!("blob {}", revision.blob); - println!("date {time}"); - println!(); + term::print(format_args!("blob {}", revision.blob)); + term::print(format_args!("date {time}")); + term::blank(); if let Some(msg) = tip.message() { for line in msg.lines() { if line.is_empty() { - println!(); + term::blank(); } else { term::indented(term::format::dim(line)); } @@ -200,10 +200,10 @@ pub fn run(args: Args, ctx: impl term::Context) -> anyhow::Result<()> { term::blank(); } for line in json::to_pretty(&doc, Path::new("radicle.json"))? { - println!(" {line}"); + term::print(format_args!(" {line}")); } - println!(); + term::blank(); } } Target::RepoId => { @@ -232,7 +232,7 @@ fn refs(repo: &radicle::storage::git::Repository) -> anyhow::Result<()> { } } - print!("{}", tree(refs)); + term::print_inline(tree(refs)); Ok(()) } diff --git a/crates/radicle-cli/src/commands/node.rs b/crates/radicle-cli/src/commands/node.rs index f8cec865..81783435 100644 --- a/crates/radicle-cli/src/commands/node.rs +++ b/crates/radicle-cli/src/commands/node.rs @@ -88,7 +88,7 @@ pub fn run(args: Args, ctx: impl term::Context) -> anyhow::Result<()> { Command::Inventory { nid } => { let nid = nid.as_ref().unwrap_or(profile.id()); for rid in profile.routing()?.get_inventory(nid)? { - println!("{}", term::format::tertiary(rid)); + term::print(term::format::tertiary(rid)); } } Command::Status { diff --git a/crates/radicle-cli/src/commands/node/control.rs b/crates/radicle-cli/src/commands/node/control.rs index c2805293..fa8351d4 100644 --- a/crates/radicle-cli/src/commands/node/control.rs +++ b/crates/radicle-cli/src/commands/node/control.rs @@ -170,7 +170,7 @@ pub fn logs(lines: usize, follow: Option, profile: &Profile) -> } tail.reverse(); - print!("{}", term::format::dim(String::from_utf8_lossy(&tail))); + term::print_inline(term::format::dim(String::from_utf8_lossy(&tail))); if let Some(timeout) = follow { file.seek(SeekFrom::End(0))?; @@ -184,7 +184,7 @@ pub fn logs(lines: usize, follow: Option, profile: &Profile) -> if len == 0 { thread::sleep(time::Duration::from_millis(250)); } else { - print!("{}", term::format::dim(line)); + term::print_inline(term::format::dim(line)); } } } @@ -414,7 +414,7 @@ pub fn config(node: &Node) -> anyhow::Result<()> { let cfg = node.config()?; let cfg = serde_json::to_string_pretty(&cfg)?; - println!("{cfg}"); + term::print(cfg); Ok(()) } diff --git a/crates/radicle-cli/src/commands/node/events.rs b/crates/radicle-cli/src/commands/node/events.rs index a90ef920..bd0f5fca 100644 --- a/crates/radicle-cli/src/commands/node/events.rs +++ b/crates/radicle-cli/src/commands/node/events.rs @@ -2,6 +2,8 @@ use std::time; use radicle::node::{Event, Handle}; +use crate::terminal as term; + pub fn run(node: H, count: usize, timeout: time::Duration) -> anyhow::Result<()> where H: Handle::Error>>, @@ -11,7 +13,7 @@ where let event = event?; let obj = serde_json::to_string(&event)?; - println!("{obj}"); + term::print(&obj); // Only output up to `count` events. if i + 1 >= count { diff --git a/crates/radicle-cli/src/commands/node/routing.rs b/crates/radicle-cli/src/commands/node/routing.rs index 77ff0165..dad1bb67 100644 --- a/crates/radicle-cli/src/commands/node/routing.rs +++ b/crates/radicle-cli/src/commands/node/routing.rs @@ -44,6 +44,6 @@ fn print_table(entries: impl IntoIterator) { fn print_json(entries: impl IntoIterator) { for (rid, nid) in entries { - println!("{}", serde_json::json!({ "rid": rid, "nid": nid })); + term::print(serde_json::json!({ "rid": rid, "nid": nid })); } } diff --git a/crates/radicle-cli/src/commands/path.rs b/crates/radicle-cli/src/commands/path.rs index d5ab589b..8177618b 100644 --- a/crates/radicle-cli/src/commands/path.rs +++ b/crates/radicle-cli/src/commands/path.rs @@ -9,7 +9,7 @@ pub use args::Args; pub fn run(_args: Args, _ctx: impl term::Context) -> anyhow::Result<()> { let home = profile::home()?; - println!("{}", home.path().display()); + term::print(home.path().display()); Ok(()) } diff --git a/crates/radicle-cli/src/lib.rs b/crates/radicle-cli/src/lib.rs index f397388e..78d6bc3a 100644 --- a/crates/radicle-cli/src/lib.rs +++ b/crates/radicle-cli/src/lib.rs @@ -1,4 +1,7 @@ #![allow(clippy::too_many_arguments)] +// Prevent use of `println!` and `print!` which panic on broken pipes. +// Use `terminal::io::println` or `terminal::io::print` instead. +#![deny(clippy::print_stdout)] pub mod commands; pub mod git; pub mod node; diff --git a/crates/radicle-cli/src/main.rs b/crates/radicle-cli/src/main.rs index 2d0f0a5e..3b356090 100644 --- a/crates/radicle-cli/src/main.rs +++ b/crates/radicle-cli/src/main.rs @@ -118,6 +118,25 @@ fn main() { .homepage(env!("CARGO_PKG_HOMEPAGE")) .support("Open a support request at https://radicle.zulipchat.com/ or file an issue via Radicle itself, or e-mail to team@radicle.dev")); + // Install a panic hook that intercepts panics caused by broken pipes and exits + // cleanly. This is a backstop for any uses of `println!` (in our code or + // dependencies like `clap`) that were not converted to `term::print`. + // + // `println!` panics with "failed printing to stdout: Broken pipe" when + // failing to write to a closed standard output. We chain our hook in front + // of `human_panic`'s hook so that panics not caused by broken pipes are + // still handled by `human_panic`. + // + // See also . + #[cfg(unix)] + { + let default_hook = std::panic::take_hook(); + std::panic::set_hook(Box::new(move |info| { + handle_broken_pipe(info); + default_hook(info); + })); + } + if let Some(lvl) = radicle::logger::env_level() { let logger = Box::new(radicle::logger::Logger::new(lvl)); log::set_boxed_logger(logger).expect("no other logger should have been set already"); @@ -146,12 +165,95 @@ fn run(command: Command, ctx: impl term::Context) -> ! { match run_command(command, ctx) { Ok(()) => process::exit(0), Err(err) => { + // If the error is a broken pipe, exit cleanly. This happens when + // output is piped to a command that exits before reading all our + // output, e.g. `rad config | head`. + // + // Rust ignores `SIGPIPE` by default (since 1.62), so broken pipes + // and instead returns `io::ErrorKind::BrokenPipe` errors on writes. + // We want to catch these and exit cleanly. + // + // See . + #[cfg(unix)] + if is_broken_pipe(&err) { + process::exit(0); + } term::fail(&err); process::exit(1); } } } +/// Handle an error of kind [`ErrorKind::BrokenPipe`] during a panic, and +/// exit the process with exit code 0. +/// +/// # Debug +/// +/// If compiled with `debug_assertions` enabled, then the panic is written to +/// [`std::io::stderr`]. +#[cfg(unix)] +fn handle_broken_pipe(info: &std::panic::PanicHookInfo<'_>) { + if !is_broken_pipe_panic(info) { + return; + } + + if cfg!(debug_assertions) { + let thread = std::thread::current(); + let thread = thread.name().unwrap_or(""); + + let mut stderr = std::io::stderr().lock(); + + match info.location() { + Some(location) => { + let _ = writeln!( + stderr, + "broken pipe in thread '{thread}' at: {}:{}", + location.file(), + location.line(), + ); + } + None => { + let _ = writeln!(stderr, "broken pipe in thread '{thread}'"); + } + } + + #[cfg(feature = "backtrace")] + let backtrace = format!("{:?}", backtrace::Backtrace::new()); + + #[cfg(not(feature = "backtrace"))] + let backtrace = "(no backtrace available)"; + + let _ = writeln!(stderr, "{backtrace}"); + } + process::exit(0); +} + +/// Check if any error in the [`anyhow::Error::chain`] of `err` is of kind +/// [`ErrorKind::BrokenPipe`]. +#[cfg(unix)] +fn is_broken_pipe(err: &anyhow::Error) -> bool { + err.chain() + .filter_map(|cause| cause.downcast_ref::()) + .any(|io_err| io_err.kind() == ErrorKind::BrokenPipe) +} + +/// Check whether a panic was caused by writing to a broken pipe. +/// +/// The standard library panics with a [`String`] payload containing +/// "Broken pipe" when [`println!`] or [`print!`] fail to write because standard +/// output is closed. This is stable behaviour across all Unix platforms, since +/// it is adopted from the description of `EPIPE` in [`errno.h` in POSIX.1-2024]. +/// +/// [`errno.h` in POSIX.1-2024]: https://pubs.opengroup.org/onlinepubs/9799919799.2024edition/basedefs/errno.h.html +#[cfg(unix)] +fn is_broken_pipe_panic(info: &std::panic::PanicHookInfo<'_>) -> bool { + info.payload() + .downcast_ref::<&'static str>() + .copied() + .or(info.payload().downcast_ref::().map(|s| s.as_str())) + .is_some_and(|message| message.contains("Broken pipe")) +} + fn run_command(command: Command, ctx: impl term::Context) -> Result<(), anyhow::Error> { match command { Command::Auth(args) => auth::run(args, ctx), diff --git a/crates/radicle-node/Cargo.toml b/crates/radicle-node/Cargo.toml index 17b0b164..ac287d7c 100644 --- a/crates/radicle-node/Cargo.toml +++ b/crates/radicle-node/Cargo.toml @@ -17,7 +17,7 @@ test = ["radicle/test", "radicle-crypto/test", "radicle-crypto/cyphernet", "radi tor = ["cyphernet/tor", "radicle/tor", "radicle-protocol/tor"] [dependencies] -backtrace = { version = "0.3.75", optional = true } +backtrace = { workspace = true, optional = true } bytes = { workspace = true } crossbeam-channel = { workspace = true } cyphernet = { workspace = true, features = ["dns", "ed25519", "p2p-ed25519", "noise-framework", "noise_sha2"] }