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/git_handlers.rs b/src/commands/git_handlers.rs index 31c49e9..6b6c860 100644 --- a/src/commands/git_handlers.rs +++ b/src/commands/git_handlers.rs @@ -111,6 +111,13 @@ where } pub fn handle_git(args: &[String]) { + // 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); + } + let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { handle_git_inner(args); })); @@ -124,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/observability/mod.rs b/src/observability/mod.rs index 5344dd7..93ddf70 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; @@ -116,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. @@ -134,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())); @@ -144,6 +176,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..3bb2aad --- /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; the panic hook recognizes +//! that broken-pipe panic and exits quietly instead of crashing. + +#[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}" + ); + // 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(0), + "autter blame did not exit cleanly on a closed pipe (stderr:\n{stderr})" + ); +} 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..7207cd8 100644 --- a/tests/integration/repos/test_repo.rs +++ b/tests/integration/repos/test_repo.rs @@ -2748,13 +2748,10 @@ impl TestRepo { } } - pub fn autter_with_env(&self, args: &[&str], envs: &[(&str, &str)]) -> Result { - if autter_command_requires_daemon_sync(args) { - self.sync_daemon_force(); - } - - let is_checkpoint = autter_primary_command(args) == Some("checkpoint"); - + /// 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); @@ -2762,13 +2759,24 @@ impl TestRepo { 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); } + command + } + + pub fn autter_with_env(&self, args: &[&str], envs: &[(&str, &str)]) -> Result { + if autter_command_requires_daemon_sync(args) { + self.sync_daemon_force(); + } + + let is_checkpoint = autter_primary_command(args) == Some("checkpoint"); + + let mut command = self.autter_command(args); + // Add custom environment variables for (key, value) in envs { command.env(key, value); @@ -2824,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()