From 90e663be5f83cd6045d494c3c74fec4380725941 Mon Sep 17 00:00:00 2001 From: "@permadeath.com" Date: Wed, 12 Aug 2026 20:33:13 -0400 Subject: [PATCH] fix(cli)!: end quietly when a reader stops reading, instead of panicking Rust sets SIGPIPE to SIG_IGN before main, so a write to a closed pipe returns EPIPE rather than killing the process -- and println! answers an EPIPE by panicking. `atgc completion zsh | head -c 10` exited 101 with a Rust panic raised inside clap_complete, and every one of atgc's ~240 println! sites had the same shape: `pr list | head`, `logs oauth | less` and quitting early. Restoring the default disposition is the only fix that reaches a panic thrown inside a dependency, which is why say.rs's rule that a failure is a Result reaching main could not cover this. Through the sigpipe crate because the call is signal(2) and this crate forbids unsafe; it is eight lines, adds no transitive dependency (libc was already here) and no-ops off Unix. Split main in two so the reset provably precedes tokio's first worker thread. Breaking, in one narrow place: quitting the pager early during `pr diff` now ends atgc with SIGPIPE where review.rs deliberately swallowed the write error. It takes an interactive pager, a patch past the 64 KiB pipe buffer and a quit before the end to reach, git ends the same way in the same place, and the patch has already been shown -- what changes is that $? is then 141. Co-Authored-By: Claude Opus 5 (1M context) --- Cargo.lock | 10 ++++++ Cargo.toml | 6 ++++ TESTING.md | 22 +++++++++++++ TODO.md | 14 ++++++++ src/cmd/pr/review.rs | 9 +++++ src/main.rs | 51 ++++++++++++++++++++++------- tests/broken_pipe.rs | 78 ++++++++++++++++++++++++++++++++++++++++++++ 7 files changed, 178 insertions(+), 12 deletions(-) create mode 100644 tests/broken_pipe.rs diff --git a/Cargo.lock b/Cargo.lock index bf790b2..84bd5a4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -146,6 +146,7 @@ dependencies = [ "serde", "serde_json", "sha2", + "sigpipe", "tangled-lexicon", "terminal_size", "tokio", @@ -3223,6 +3224,15 @@ dependencies = [ "rand_core 0.6.4", ] +[[package]] +name = "sigpipe" +version = "0.1.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5584bfb3e0d348139d8210285e39f6d2f8a1902ac06de343e06357d1d763d8e6" +dependencies = [ + "libc", +] + [[package]] name = "simd-adler32" version = "0.3.10" diff --git a/Cargo.toml b/Cargo.toml index 3beef61..6ae3e00 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -49,6 +49,12 @@ pulldown-cmark = { version = "0.13", default-features = false } reqwest = { version = "0.12", default-features = false, features = ["rustls-tls"] } serde = { version = "1.0.229", features = ["derive"] } serde_json = "1.0.151" +# Restores SIGPIPE to its default disposition, which Rust's runtime sets to +# SIG_IGN before main. A dependency rather than two lines of our own because +# the call is `signal(2)` and this crate forbids `unsafe`; the unsafe lives +# here instead, in eight lines that have not needed to change since 2022. +# See main.rs for what it buys. +sigpipe = "0.1.3" # Fingerprints for the OAuth log: it records the first 8 hex chars of a # token's SHA-256 so two sightings of one token can be matched, never the # token. Already in the tree via jacquard. diff --git a/TESTING.md b/TESTING.md index 3aef36f..71354ce 100644 --- a/TESTING.md +++ b/TESTING.md @@ -385,6 +385,28 @@ as the account you named or somebody else — and its refusal path, which lists the accounts you do hold rather than falling through to one of them, is the behaviour the code's own comment calls the worst possible failure. +## Testing the process itself + +Almost everything here is a unit test inside `src/`. The exception is a +property that is not a property of any function: how the *process* behaves — +its signal dispositions, its exit status, what it writes to which stream. +A `cargo test` binary has its own, so those cannot be observed from inside +one, and the test has to spawn the real thing. + +`tests/broken_pipe.rs` is the pattern. `env!("CARGO_BIN_EXE_atgc")` is the +binary cargo built for this test run, so what it drives is what a user runs; +the test pipes its stdout, reads a few bytes, drops the read end, and asserts +that atgc ended quietly rather than panicking. It is a real gate, not a +description: with `sigpipe::reset()` commented out of `main` it fails with +the panic it exists to prevent. + +Two rules for adding another. Pick a subcommand that cannot fail for an +unrelated reason — `completion` needs no session, no network and no checkout, +and is exempt from log initialization, so it touches nothing under `HOME`. +And assert on the *shape* of the outcome rather than one exact status: +`broken_pipe` accepts either a clean exit or death by SIGPIPE, because both +are correct and which one happens is a race with the pipe buffer. + ## What stays manual These have no hermetic test and are not going to get one. Verify them by diff --git a/TODO.md b/TODO.md index dd251e1..4f5ee28 100644 --- a/TODO.md +++ b/TODO.md @@ -1127,6 +1127,20 @@ warn where the stack command is better. match-on-the-colour in `class_for`. Left out of the anstyle change because it is a different fix, and because it lands on top of the `html/` split rather than beside it +- [x] SIGPIPE restored to its default disposition in `main`, so a reader that + stops reading ends atgc quietly instead of panicking. Rust sets it to + `SIG_IGN` before `main` and `println!` panics on the resulting `EPIPE`, + which made `atgc completion zsh | head` exit 101 with a Rust panic from + inside `clap_complete` — a failure no `Result` returned to `main` could + have caught, and one that applied to all ~240 `println!` sites. Through + the `sigpipe` crate because the call is `signal(2)` and this crate + forbids `unsafe`; it adds eight lines and no transitive dependency, + libc already being in the tree. `tests/broken_pipe.rs` drives the real + binary and fails without the fix. One consequence, stated rather than + discovered later: quitting the pager early during `pr diff` now ends + atgc with SIGPIPE where the write error used to be swallowed, which is + where git ends too, and needs a patch past the 64 KiB pipe buffer to + reach at all - [x] non-interactive mode — `--no-input` / `ATGC_NO_INPUT=1`, automatic when stdin is not a terminal or `CI` is set: `auth login` refuses with instructions instead of waiting out its five-minute browser timeout, diff --git a/src/cmd/pr/review.rs b/src/cmd/pr/review.rs index e042b71..eb7cb1f 100644 --- a/src/cmd/pr/review.rs +++ b/src/cmd/pr/review.rs @@ -532,6 +532,15 @@ fn print_patch(header: &str, patch: &str, use_pager: bool) -> Result<()> { if let Some(stdin) = child.stdin.as_mut() { // A pager the user quits early closes the pipe, and that is a normal // way to stop reading a patch rather than an error to report. + // + // Since `main` restored SIGPIPE, "not reported" now means atgc is + // killed by the signal here rather than reaching this `let _`, which + // only swallows the `EPIPE` a still-ignored SIGPIPE would have + // produced. It takes an interactive pager, a patch past the 64 KiB + // pipe buffer (below that the write lands in the buffer and returns + // before the pager can exit) and a quit before the end to get there, + // and git ends the same way in the same place. The patch has already + // been shown either way; what changes is that `$?` is then 141. let _ = stdin.write_all(body.as_bytes()); } drop(child.stdin.take()); diff --git a/src/main.rs b/src/main.rs index eda3be3..4716140 100644 --- a/src/main.rs +++ b/src/main.rs @@ -213,13 +213,41 @@ enum Command { }, } -/// Run the command, then turn whatever it returned into a status. +/// Restore SIGPIPE, run the command, then turn what it returned into a status. /// -/// Split from [`run`] because a `main` returning `anyhow::Result` gives up -/// both halves of this: every failure exits `1`, and the message is formatted -/// by Rust's `Termination` impl rather than by atgc. See [`crate::exit`] for -/// what the statuses mean and [`report`] for the formatting. +/// Two things `main` has to do that a `main` returning `anyhow::Result` cannot. +/// +/// **SIGPIPE.** Rust's runtime sets it to `SIG_IGN` before `main`, so a write +/// to a closed pipe comes back as `EPIPE` instead of ending the process — and +/// `println!` answers an `EPIPE` by panicking. Every one of atgc's ~240 +/// `println!` sites is therefore a panic waiting for a reader that stops +/// reading, and so is `clap_complete`'s own writer: +/// +/// ```text +/// $ atgc completion zsh | head -c 10 +/// thread 'main' panicked at clap_complete/src/aot/shells/shell.rs:86:14: +/// failed to write completion file: Os { code: 32, kind: BrokenPipe } +/// $ echo ${PIPESTATUS[0]} +/// 101 +/// ``` +/// +/// `atgc pr list | head` and `atgc logs oauth | less` are the same shape. +/// Restoring the default disposition makes all of them die quietly the way +/// `git`, `ls` and `grep` do, and it is the only fix that reaches a panic +/// thrown inside a dependency — [`crate::term::say`]'s rule that a failure is +/// a `Result` reaching `main` cannot cover a macro that panics on its way to +/// stdout. +/// +/// **The status.** See [`crate::exit`] for what each one means and [`report`] +/// for how the message is written. +/// +/// The reset is the first statement, and [`run`] is a separate function, so +/// that it provably lands before tokio builds a runtime and spawns a worker. +/// The disposition is process-wide, so this only matters against a thread that +/// could write before it — but *provably none* is cheaper to keep true than +/// *none that currently does*. fn main() -> std::process::ExitCode { + sigpipe::reset(); match run() { Ok(()) => std::process::ExitCode::SUCCESS, Err(err) => { @@ -232,13 +260,12 @@ fn main() -> std::process::ExitCode { /// A failure, on stderr, in the shape [`crate::term::say`]'s prefixes /// established: a lower-case label, then the sentence. /// -/// One line by default — anyhow's alternate form, the chain joined by `: `, -/// which is how a person -/// reads "what failed, doing what, while doing what". `--debug` prints -/// `anyhow`'s own multi-line form instead, which keeps the causes on separate -/// lines and carries a backtrace when `RUST_BACKTRACE` asks for one. That is -/// the flag's existing promise of "full error details", now true of errors -/// and not only of HTTP bodies. +/// One line by default — `anyhow`'s alternate form, the chain joined by `: `, +/// which is how a person reads "what failed, doing what, while doing what". +/// `--debug` prints `anyhow`'s own multi-line form instead, which keeps the +/// causes on separate lines and carries a backtrace when `RUST_BACKTRACE` asks +/// for one. That is the flag's existing promise of "full error details", now +/// true of errors and not only of HTTP bodies. fn report(err: &anyhow::Error) { if logging::debug::enabled() { eprintln!("error: {err:?}"); diff --git a/tests/broken_pipe.rs b/tests/broken_pipe.rs new file mode 100644 index 0000000..2733e66 --- /dev/null +++ b/tests/broken_pipe.rs @@ -0,0 +1,78 @@ +//! A reader that stops reading must not crash atgc. +//! +//! `atgc … | head` is the shape: the reader takes what it wants and closes +//! the pipe, and every well-behaved Unix tool ends there quietly. Rust's +//! runtime makes that the one thing a Rust program does *not* do by default, +//! because it sets SIGPIPE to `SIG_IGN` before `main` and `println!` panics +//! on the `EPIPE` that produces. +//! +//! This is an integration test rather than a unit test because the thing +//! under test is a property of the process — its signal disposition — and +//! there is no way to observe that from inside a `cargo test` binary that has +//! its own. `CARGO_BIN_EXE_atgc` is the real binary, built by cargo for this +//! test, so what runs here is what a user runs. +//! +//! `completion zsh` is the subject for three reasons: it needs no session, no +//! network and no checkout, so it can never fail for an unrelated reason; its +//! output is ~112 KiB, comfortably past the 64 KiB pipe buffer, so the child +//! is guaranteed to still be writing when the reader goes away; and the panic +//! it used to produce came from inside `clap_complete` rather than from atgc's +//! own code, which is the case a `Result` returned to `main` could never have +//! caught. + +#![cfg(unix)] + +use std::io::Read; +use std::os::unix::process::ExitStatusExt; +use std::process::{Command, Stdio}; + +/// `SIGPIPE`. Spelled here rather than pulled from `libc`, which is not a +/// dependency of this crate and would be a heavy one to add for a constant +/// that POSIX fixes at 13 on every platform atgc builds for. +const SIGPIPE: i32 = 13; + +#[test] +fn a_reader_that_stops_reading_does_not_panic_atgc() { + let mut child = Command::new(env!("CARGO_BIN_EXE_atgc")) + .args(["completion", "zsh"]) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .expect("spawn atgc"); + + // Take a little and then drop the read end, which is exactly what + // `| head -c 10` does to the process on the left. + let mut stdout = child.stdout.take().expect("atgc's stdout is piped"); + let mut first = [0u8; 10]; + stdout + .read_exact(&mut first) + .expect("atgc wrote nothing at all"); + drop(stdout); + + let finished = child.wait_with_output().expect("wait for atgc"); + let stderr = String::from_utf8_lossy(&finished.stderr); + + assert!( + !stderr.contains("panicked"), + "atgc panicked when its reader went away:\n{stderr}" + ); + // 101 is what a panicking Rust program exits with, and is what this + // command did before SIGPIPE was restored. Asserted separately from the + // message so a panic that somehow printed nothing still fails here. + assert_ne!( + finished.status.code(), + Some(101), + "atgc exited 101, the panic code:\n{stderr}" + ); + // Either ending is correct. Death by SIGPIPE is the usual one and is what + // git and grep do; a clean exit is what happens if the child managed to + // finish its last write before the reader was gone. What must not happen + // is any *other* code, which would mean the write failed and was reported + // as an ordinary error. + assert!( + finished.status.success() || finished.status.signal() == Some(SIGPIPE), + "atgc ended with {:?} (signal {:?}) rather than quietly:\n{stderr}", + finished.status.code(), + finished.status.signal() + ); +} -- 2.51.2