From 25479542ceeca4b18a1abcaca9ddaad944fa1750 Mon Sep 17 00:00:00 2001 From: "posthog[bot]" <206114724+posthog[bot]@users.noreply.github.com> Date: Mon, 21 Sep 2026 18:41:16 +0000 Subject: [PATCH 1/2] fix: exit cleanly when stdout pipe closes early Rust ignores SIGPIPE before `main`, so a reader that closes a pipe early (e.g. `autter blame | head`) turned the closed pipe into an EPIPE that made `println!` panic with "failed printing to stdout". Restore the Unix default (SIG_DFL) at the top of `main` so the process exits quietly on a closed pipe, like every other CLI tool. This removes the panic path for every print-heavy subcommand at once. The long-lived daemon re-ignores SIGPIPE (SIG_IGN) at startup so its network and control-socket writes keep returning EPIPE instead of getting killed by a signal. Also record the current subcommand and add it to the panic report context, which previously carried only a source location. Generated-By: PostHog Desktop Task-Id: 30cd172e-097b-4b6e-bce6-c8f73634b434 --- src/commands/autter_handlers.rs | 3 ++ src/commands/daemon.rs | 10 ++++++ src/commands/git_handlers.rs | 5 +++ src/main.rs | 15 +++++++++ src/observability/mod.rs | 12 +++++++ tests/integration/broken_pipe.rs | 48 ++++++++++++++++++++++++++++ tests/integration/main.rs | 1 + tests/integration/repos/test_repo.rs | 20 ++++++++++++ 8 files changed, 114 insertions(+) create mode 100644 tests/integration/broken_pipe.rs diff --git a/src/commands/autter_handlers.rs b/src/commands/autter_handlers.rs index 173922e..f2e7490 100644 --- a/src/commands/autter_handlers.rs +++ b/src/commands/autter_handlers.rs @@ -70,6 +70,9 @@ pub fn handle_autter(args: &[String]) { std::process::exit(0); } + // Record the subcommand so any later panic report can name it. + crate::observability::set_current_command(args[0].as_str()); + // Initialize the global telemetry handle so that observability and CAS // events are routed over the control socket instead of being written to // per-PID log files. diff --git a/src/commands/daemon.rs b/src/commands/daemon.rs index 3420514..906bfe9 100644 --- a/src/commands/daemon.rs +++ b/src/commands/daemon.rs @@ -174,6 +174,16 @@ fn daemon_config_from_env_or_default_paths() -> Result { } fn handle_run(args: &[String]) -> Result<(), String> { + // The daemon is long-lived and writes to network and control sockets. + // `main` resets SIGPIPE to SIG_DFL for tidy CLI output on a closed pipe; + // re-ignore it here so a peer that closes a socket yields an EPIPE error we + // can handle, rather than a signal that would kill the whole daemon. + #[cfg(unix)] + // SAFETY: setting SIG_IGN for SIGPIPE is async-signal-safe. + unsafe { + libc::signal(libc::SIGPIPE, libc::SIG_IGN); + } + if has_flag(args, "--mode") { return Err("--mode is no longer supported; daemon always runs in write mode".to_string()); } diff --git a/src/commands/git_handlers.rs b/src/commands/git_handlers.rs index 31c49e9..0a136de 100644 --- a/src/commands/git_handlers.rs +++ b/src/commands/git_handlers.rs @@ -111,6 +111,11 @@ where } pub fn handle_git(args: &[String]) { + // Record the git subcommand so any later panic report can name it. + if let Some(subcommand) = best_effort_subcommand(args) { + crate::observability::set_current_command(subcommand); + } + let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { handle_git_inner(args); })); diff --git a/src/main.rs b/src/main.rs index 451ab72..e11c973 100644 --- a/src/main.rs +++ b/src/main.rs @@ -36,6 +36,21 @@ fn is_superuser_exempt_command(args: &[String]) -> bool { } fn main() { + // Rust's runtime sets SIGPIPE to SIG_IGN before `main`. That turns a reader + // closing the other end of a pipe (e.g. `autter blame | head`) into an + // EPIPE error, which makes the `println!`/`print!` macros panic with + // "failed printing to stdout". Restore the Unix default so the process + // instead exits quietly on a closed pipe, like every other CLI tool. The + // long-lived daemon re-ignores SIGPIPE at startup (see + // commands::daemon::handle_run) so its socket writes keep returning EPIPE + // instead of terminating the process. + #[cfg(unix)] + // SAFETY: restoring SIG_DFL for SIGPIPE is async-signal-safe and only sets + // the platform's default disposition for the signal. + unsafe { + libc::signal(libc::SIGPIPE, libc::SIG_DFL); + } + // Get the binary name that was called let binary_name = std::env::args_os() .next() diff --git a/src/observability/mod.rs b/src/observability/mod.rs index 5344dd7..a869b38 100644 --- a/src/observability/mod.rs +++ b/src/observability/mod.rs @@ -1,10 +1,21 @@ use std::collections::HashMap; +use std::sync::OnceLock; use std::time::Duration; use crate::metrics::MetricEvent; pub mod performance_targets; +/// The autter/git subcommand currently executing, recorded so the panic hook +/// can attach it to reported panics. Set once per process at dispatch time. +static CURRENT_COMMAND: OnceLock = OnceLock::new(); + +/// Record the subcommand being executed so a later panic report can name it. +/// Best-effort and idempotent: the first call wins, later calls are ignored. +pub fn set_current_command(command: impl Into) { + let _ = CURRENT_COMMAND.set(command.into()); +} + /// Maximum events per metrics envelope pub const MAX_METRICS_PER_ENVELOPE: usize = 1000; @@ -144,6 +155,7 @@ pub fn install_panic_hook() { context: Some(serde_json::json!({ "kind": "panic", "location": location, + "command": CURRENT_COMMAND.get(), })), }; submit_telemetry_envelope(vec![envelope]); diff --git a/tests/integration/broken_pipe.rs b/tests/integration/broken_pipe.rs new file mode 100644 index 0000000..8f9ea5e --- /dev/null +++ b/tests/integration/broken_pipe.rs @@ -0,0 +1,48 @@ +//! A reader that closes a pipe early (e.g. `autter blame | head`) must not +//! crash the CLI. Rust ignores SIGPIPE before `main`, which turns the closed +//! pipe into an EPIPE that makes `println!` panic; `main` restores the Unix +//! default so the process exits quietly instead. + +#[cfg(unix)] +#[test] +fn blame_into_closed_pipe_does_not_panic() { + use crate::repos::test_repo::TestRepo; + use std::io::Read; + use std::process::Stdio; + + let repo = TestRepo::new(); + + // A large file makes blame output overflow the pipe buffer, so autter keeps + // writing after the reader closes and would hit EPIPE. + let big: String = (0..40_000).map(|i| format!("line {i}\n")).collect(); + std::fs::write(repo.path().join("big.txt"), &big).unwrap(); + repo.stage_all_and_commit("add big file").unwrap(); + + let mut child = repo + .autter_command(&["blame", "big.txt"]) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .expect("spawn autter blame"); + + // Read a little, then close the read end to break the pipe. + let mut stdout = child.stdout.take().unwrap(); + let mut buf = [0u8; 64]; + let _ = stdout.read(&mut buf); + drop(stdout); + + let output = child.wait_with_output().expect("wait for autter blame"); + let stderr = String::from_utf8_lossy(&output.stderr); + + assert!( + !stderr.contains("panicked") && !stderr.contains("failed printing to stdout"), + "autter blame panicked on a closed pipe:\n{stderr}" + ); + // A panic exits with code 101; a clean SIGPIPE termination or graceful exit + // does not. + assert_ne!( + output.status.code(), + Some(101), + "autter blame exited via panic on a closed pipe" + ); +} diff --git a/tests/integration/main.rs b/tests/integration/main.rs index 275804f..73676d6 100644 --- a/tests/integration/main.rs +++ b/tests/integration/main.rs @@ -24,6 +24,7 @@ mod blame_comprehensive; mod blame_flags; mod blame_subdirectory; mod blame_why; +mod broken_pipe; mod checkout_switch; mod checkpoint_debug_log; mod checkpoint_explicit_paths; diff --git a/tests/integration/repos/test_repo.rs b/tests/integration/repos/test_repo.rs index 3b75fb0..3333491 100644 --- a/tests/integration/repos/test_repo.rs +++ b/tests/integration/repos/test_repo.rs @@ -2748,6 +2748,26 @@ impl TestRepo { } } + /// Build a configured `autter` [`Command`] without running it, so tests can + /// control stdio (e.g. to exercise closed-pipe behavior). Mirrors the env + /// setup used by [`Self::autter_with_env`]. + pub fn autter_command(&self, args: &[&str]) -> Command { + let binary_path = get_binary_path(); + let normalized_args = normalize_test_autter_checkpoint_args(args); + + let mut command = Command::new(binary_path); + command.args(&normalized_args).current_dir(&self.path); + self.configure_autter_env(&mut command); + + if let Some(patch) = &self.config_patch + && let Ok(patch_json) = serde_json::to_string(patch) + { + command.env("AUTTER_TEST_CONFIG_PATCH", patch_json); + } + + command + } + pub fn autter_with_env(&self, args: &[&str], envs: &[(&str, &str)]) -> Result { if autter_command_requires_daemon_sync(args) { self.sync_daemon_force(); From 3b722c8f18c49a709de025539d308a392736781e Mon Sep 17 00:00:00 2001 From: "posthog[bot]" <206114724+posthog[bot]@users.noreply.github.com> Date: Mon, 21 Sep 2026 18:54:22 +0000 Subject: [PATCH 2/2] refactor: intercept broken-pipe panic in the hook instead of resetting SIGPIPE The first approach reset SIGPIPE to SIG_DFL process-wide. Review found that the CLI (including the transparent git proxy) writes to the daemon control socket on nearly every invocation; under SIG_DFL a daemon that closed the socket mid-write would deliver SIGPIPE and silently kill the command, bypassing the existing reconnect/retry logic. A daemon-only re-ignore did not cover those CLI-side writers. Keep SIGPIPE ignored (Rust's default, network-safe everywhere) and instead recognize the broken-pipe panic from the print macros in the panic hook and exit quietly (exit 0), without printing a panic or reporting telemetry. This fixes the same symptom (`autter blame | head`) centrally, leaves every socket write's EPIPE handling intact, and keeps the existing BrokenPipe handling in log.rs working as written. Also dedup: route the test helpers autter_with_env/autter_with_stdin through the new autter_command, and compute the git subcommand once in handle_git. Generated-By: PostHog Desktop Task-Id: 30cd172e-097b-4b6e-bce6-c8f73634b434 --- src/commands/daemon.rs | 10 ---------- src/commands/git_handlers.rs | 7 ++++--- src/main.rs | 15 -------------- src/observability/mod.rs | 21 ++++++++++++++++++++ tests/integration/broken_pipe.rs | 14 +++++++------- tests/integration/repos/test_repo.rs | 29 ++-------------------------- 6 files changed, 34 insertions(+), 62 deletions(-) diff --git a/src/commands/daemon.rs b/src/commands/daemon.rs index 906bfe9..3420514 100644 --- a/src/commands/daemon.rs +++ b/src/commands/daemon.rs @@ -174,16 +174,6 @@ fn daemon_config_from_env_or_default_paths() -> Result { } fn handle_run(args: &[String]) -> Result<(), String> { - // The daemon is long-lived and writes to network and control sockets. - // `main` resets SIGPIPE to SIG_DFL for tidy CLI output on a closed pipe; - // re-ignore it here so a peer that closes a socket yields an EPIPE error we - // can handle, rather than a signal that would kill the whole daemon. - #[cfg(unix)] - // SAFETY: setting SIG_IGN for SIGPIPE is async-signal-safe. - unsafe { - libc::signal(libc::SIGPIPE, libc::SIG_IGN); - } - if has_flag(args, "--mode") { return Err("--mode is no longer supported; daemon always runs in write mode".to_string()); } diff --git a/src/commands/git_handlers.rs b/src/commands/git_handlers.rs index 0a136de..6b6c860 100644 --- a/src/commands/git_handlers.rs +++ b/src/commands/git_handlers.rs @@ -111,8 +111,10 @@ where } pub fn handle_git(args: &[String]) { - // Record the git subcommand so any later panic report can name it. - if let Some(subcommand) = best_effort_subcommand(args) { + // Record the git subcommand so any later panic report (and the recovery + // telemetry below) can name it. + let subcommand = best_effort_subcommand(args); + if let Some(subcommand) = subcommand.as_deref() { crate::observability::set_current_command(subcommand); } @@ -129,7 +131,6 @@ pub fn handle_git(args: &[String]) { // + the org database). The panic hook already emitted the raw `$exception`; // this complements it with the *recovery outcome* so we can see how often // the proxy degrades and on which subcommands. Best-effort and panic-safe. - let subcommand = best_effort_subcommand(args); crate::observability::report_cli_error( "git_proxy_panic_recovery", if git_already_ran { diff --git a/src/main.rs b/src/main.rs index e11c973..451ab72 100644 --- a/src/main.rs +++ b/src/main.rs @@ -36,21 +36,6 @@ fn is_superuser_exempt_command(args: &[String]) -> bool { } fn main() { - // Rust's runtime sets SIGPIPE to SIG_IGN before `main`. That turns a reader - // closing the other end of a pipe (e.g. `autter blame | head`) into an - // EPIPE error, which makes the `println!`/`print!` macros panic with - // "failed printing to stdout". Restore the Unix default so the process - // instead exits quietly on a closed pipe, like every other CLI tool. The - // long-lived daemon re-ignores SIGPIPE at startup (see - // commands::daemon::handle_run) so its socket writes keep returning EPIPE - // instead of terminating the process. - #[cfg(unix)] - // SAFETY: restoring SIG_DFL for SIGPIPE is async-signal-safe and only sets - // the platform's default disposition for the signal. - unsafe { - libc::signal(libc::SIGPIPE, libc::SIG_DFL); - } - // Get the binary name that was called let binary_name = std::env::args_os() .next() diff --git a/src/observability/mod.rs b/src/observability/mod.rs index a869b38..93ddf70 100644 --- a/src/observability/mod.rs +++ b/src/observability/mod.rs @@ -127,10 +127,27 @@ pub fn log_error(error: &dyn std::error::Error, context: Option bool { + (payload.starts_with("failed printing to stdout") + || payload.starts_with("failed printing to stderr")) + && (payload.contains("Broken pipe") || payload.contains("os error 32")) +} + /// Install a panic hook that reports unexpected panics as error events /// (surfacing in PostHog Error Tracking via the daemon) while preserving the /// default behavior of printing the panic to stderr. /// +/// One case is handled specially: a broken-pipe failure from the print macros +/// (a reader closing the pipe, as in `autter blame | head`) is not a real +/// crash. Exit quietly like any other CLI tool instead of printing a panic or +/// reporting telemetry noise. +/// /// Reporting is best-effort and routes through the same consent-gated path as /// every other event: the daemon only forwards to PostHog when the user has /// opted into telemetry. @@ -145,6 +162,10 @@ pub fn install_panic_hook() { "Box".to_string() }; + if is_broken_pipe_print_panic(&payload) { + std::process::exit(0); + } + let location = info .location() .map(|l| format!("{}:{}:{}", l.file(), l.line(), l.column())); diff --git a/tests/integration/broken_pipe.rs b/tests/integration/broken_pipe.rs index 8f9ea5e..3bb2aad 100644 --- a/tests/integration/broken_pipe.rs +++ b/tests/integration/broken_pipe.rs @@ -1,7 +1,7 @@ //! A reader that closes a pipe early (e.g. `autter blame | head`) must not //! crash the CLI. Rust ignores SIGPIPE before `main`, which turns the closed -//! pipe into an EPIPE that makes `println!` panic; `main` restores the Unix -//! default so the process exits quietly instead. +//! pipe into an EPIPE that makes `println!` panic; the panic hook recognizes +//! that broken-pipe panic and exits quietly instead of crashing. #[cfg(unix)] #[test] @@ -38,11 +38,11 @@ fn blame_into_closed_pipe_does_not_panic() { !stderr.contains("panicked") && !stderr.contains("failed printing to stdout"), "autter blame panicked on a closed pipe:\n{stderr}" ); - // A panic exits with code 101; a clean SIGPIPE termination or graceful exit - // does not. - assert_ne!( + // The panic hook turns the broken-pipe panic into a quiet exit 0, rather + // than the crash's exit code 101. + assert_eq!( output.status.code(), - Some(101), - "autter blame exited via panic on a closed pipe" + Some(0), + "autter blame did not exit cleanly on a closed pipe (stderr:\n{stderr})" ); } diff --git a/tests/integration/repos/test_repo.rs b/tests/integration/repos/test_repo.rs index 3333491..7207cd8 100644 --- a/tests/integration/repos/test_repo.rs +++ b/tests/integration/repos/test_repo.rs @@ -2775,19 +2775,7 @@ impl TestRepo { let is_checkpoint = autter_primary_command(args) == Some("checkpoint"); - let binary_path = get_binary_path(); - let normalized_args = normalize_test_autter_checkpoint_args(args); - - let mut command = Command::new(binary_path); - command.args(&normalized_args).current_dir(&self.path); - self.configure_autter_env(&mut command); - - // Add config patch as environment variable if present - if let Some(patch) = &self.config_patch - && let Ok(patch_json) = serde_json::to_string(patch) - { - command.env("AUTTER_TEST_CONFIG_PATCH", patch_json); - } + let mut command = self.autter_command(args); // Add custom environment variables for (key, value) in envs { @@ -2844,24 +2832,11 @@ impl TestRepo { let is_checkpoint = autter_primary_command(args) == Some("checkpoint"); - let binary_path = get_binary_path(); - let normalized_args = normalize_test_autter_checkpoint_args(args); - - let mut command = Command::new(binary_path); + let mut command = self.autter_command(args); command - .args(&normalized_args) - .current_dir(&self.path) .stdin(Stdio::piped()) .stdout(Stdio::piped()) .stderr(Stdio::piped()); - self.configure_autter_env(&mut command); - - // Add config patch as environment variable if present - if let Some(patch) = &self.config_patch - && let Ok(patch_json) = serde_json::to_string(patch) - { - command.env("AUTTER_TEST_CONFIG_PATCH", patch_json); - } let mut child = command .spawn()