diff --git a/crates/clickhousectl/src/error.rs b/crates/clickhousectl/src/error.rs index ff6554f..ece52cb 100644 --- a/crates/clickhousectl/src/error.rs +++ b/crates/clickhousectl/src/error.rs @@ -48,6 +48,11 @@ pub enum Error { #[error("Failed to execute ClickHouse: {0}")] Exec(String), + /// A child process whose status must be returned unchanged. This is + /// intentionally not printed as a clickhousectl error by `run_parsed`. + #[error("child process exited with code {0}")] + ChildExit(i32), + #[error("Extraction failed: {0}")] Extract(String), @@ -100,6 +105,7 @@ impl Error { match self { Error::AuthRequired(_) => 4, Error::Cancelled => 3, + Error::ChildExit(code) => *code, _ => 1, } } @@ -133,4 +139,13 @@ mod tests { 1 ); } + + #[test] + fn child_exit_codes_pass_through_without_changing_normal_mappings() { + assert_eq!(Error::ChildExit(42).exit_code(), 42); + assert_eq!(Error::ChildExit(255).exit_code(), 255); + assert_eq!(Error::Cloud("boom".into()).exit_code(), 1); + assert_eq!(Error::Cancelled.exit_code(), 3); + assert_eq!(Error::AuthRequired("nope".into()).exit_code(), 4); + } } diff --git a/crates/clickhousectl/src/local/docker.rs b/crates/clickhousectl/src/local/docker.rs index 2562899..4265642 100644 --- a/crates/clickhousectl/src/local/docker.rs +++ b/crates/clickhousectl/src/local/docker.rs @@ -386,7 +386,7 @@ pub async fn exec_psql_one_shot( && let Some(code) = info.exit_code && code != 0 { - std::process::exit(code as i32); + return Err(Error::ChildExit(code as i32)); } Ok(()) } @@ -519,7 +519,7 @@ pub async fn exec_psql_in_container( && let Some(code) = info.exit_code && code != 0 { - std::process::exit(code as i32); + return Err(Error::ChildExit(code as i32)); } Ok(()) } diff --git a/crates/clickhousectl/src/local/mod.rs b/crates/clickhousectl/src/local/mod.rs index b7637c9..ebaceba 100644 --- a/crates/clickhousectl/src/local/mod.rs +++ b/crates/clickhousectl/src/local/mod.rs @@ -518,7 +518,7 @@ async fn start_server( if !status.success() && let Some(code) = status.code() { - std::process::exit(code); + return Err(Error::ChildExit(code)); } Ok(()) } diff --git a/crates/clickhousectl/src/main.rs b/crates/clickhousectl/src/main.rs index 07862f7..5f892d0 100644 --- a/crates/clickhousectl/src/main.rs +++ b/crates/clickhousectl/src/main.rs @@ -54,13 +54,13 @@ async fn main() { // The sole intended exemption is the hidden `telemetry send` child inside // `run_parsed`; the `exec()` handoffs (`local client`, host psql) record // their event via `telemetry::finalize_before_exec` just before the - // process image is replaced; three pre-existing child-exit-code - // passthroughs in the local handlers still bypass the tail (#321). Do not - // add exit paths. + // process image is replaced. Child-process exit codes are returned as + // `Error::ChildExit` so they also flow through this tail. Do not add exit + // paths. let (exit_code, telemetry_invocation) = match cmd.try_get_matches_from_mut(argv.iter()) { Ok(matches) => { #[cfg(feature = "telemetry")] - let invocation = telemetry::capture(&cmd, &matches); + let mut invocation = telemetry::capture(&cmd, &matches); // Stashed so the pre-exec hook can reach it from inside a // handler when `exec()` makes the tail below unreachable. #[cfg(feature = "telemetry")] @@ -71,7 +71,14 @@ async fn main() { // a clap derive bug, not a user error. let cli = Cli::from_arg_matches(&matches) .expect("Cli::from_arg_matches must accept matches from Cli::command()"); - (run_parsed(cli).await, invocation) + let (exit_code, is_child_exit) = run_parsed(cli).await; + #[cfg(feature = "telemetry")] + if is_child_exit { + invocation.mark_child_exit(); + } + #[cfg(not(feature = "telemetry"))] + let _ = is_child_exit; + (exit_code, invocation) } Err(e) => { // clap keeps its own formatting and colors; help/version print to @@ -116,11 +123,11 @@ async fn main() { } /// Run a successfully parsed invocation to completion and report the exit -/// code for `main`'s single exit. The hidden `telemetry send` child is the -/// one deliberate early exit in the binary: it does exactly one POST — no -/// update-cache refresh, no dispatch, and no telemetry hook of its own, so a -/// send can never trigger another send. -async fn run_parsed(cli: Cli) -> i32 { +/// code for `main`'s single exit plus whether it came from a child process. +/// The hidden `telemetry send` child is the one deliberate early exit in the +/// binary: it does exactly one POST — no update-cache refresh, no dispatch, +/// and no telemetry hook of its own, so a send can never trigger another send. +async fn run_parsed(cli: Cli) -> (i32, bool) { #[cfg(feature = "telemetry")] if matches!( cli.command, @@ -155,14 +162,17 @@ async fn run_parsed(cli: Cli) -> i32 { let _ = tokio::time::timeout(std::time::Duration::from_millis(500), handle).await; } - let exit_code = match result { - Ok(()) => 0, + let (exit_code, is_child_exit) = match result { + Ok(()) => (0, false), Err(e) => { - use std::io::Write; - // Not `eprintln!`, which panics on a closed stderr — see - // `telemetry::print_first_run_notice`. - let _ = writeln!(std::io::stderr(), "Error: {}", e); - e.exit_code() + let is_child_exit = matches!(&e, Error::ChildExit(_)); + if !is_child_exit { + use std::io::Write; + // Not `eprintln!`, which panics on a closed stderr — see + // `telemetry::print_first_run_notice`. + let _ = writeln!(std::io::stderr(), "Error: {}", e); + } + (e.exit_code(), is_child_exit) } }; @@ -172,7 +182,7 @@ async fn run_parsed(cli: Cli) -> i32 { update::print_cached_update_notice(); } - exit_code + (exit_code, is_child_exit) } /// The explicit `--json` flag for a command, or `None` for commands that never diff --git a/crates/clickhousectl/src/telemetry.rs b/crates/clickhousectl/src/telemetry.rs index 71beacc..63a9748 100644 --- a/crates/clickhousectl/src/telemetry.rs +++ b/crates/clickhousectl/src/telemetry.rs @@ -179,17 +179,19 @@ struct Payload { command: String, flags: Vec, /// Exit code: `Error::exit_code()` for dispatched commands — 0 success, - /// 1 error, 3 cancelled, 4 auth required — and clap's own code for parse - /// outcomes (0 help/version, 2 usage error). + /// 1 error, 3 cancelled, 4 auth required, or a child process's passthrough + /// code — and clap's own code for parse outcomes (0 help/version, 2 usage + /// error). exit_code: i32, /// How the invocation ended, from a closed vocabulary. Dispatched - /// invocations carry `"ok"`, `"error"`, `"cancelled"`, or - /// `"auth_required"` — derived from the dispatched exit code by - /// [`dispatched_outcome`] — or `"exec"` (parsed, dispatched, and the - /// process image was replaced by `exec()` — the handed-over program's - /// exit status is unknowable, so `exit_code` is a fixed 0 and not - /// meaningful). Failed parses carry a direct mapping of clap's - /// `ErrorKind` (`"help"`, `"version"`, `"invalid_subcommand"`, …). + /// invocations carry `"ok"`, `"error"` (including non-zero child exits), + /// `"cancelled"`, or `"auth_required"`. Child exits are explicitly marked + /// as `"error"`; the remaining dispatched outcomes are derived from the + /// exit code by [`dispatched_outcome`]. `"exec"` means the process image was + /// replaced by `exec()` — the handed-over program's exit status is + /// unknowable, so `exit_code` is a fixed 0 and not meaningful. Failed + /// parses carry a direct mapping of clap's `ErrorKind` (`"help"`, + /// `"version"`, `"invalid_subcommand"`, …). /// Literal strings only — this field can never carry user data. outcome: &'static str, /// Clap's "did you mean" for failed parses, anchored locally: recorded @@ -215,7 +217,8 @@ struct Payload { /// time the exit code says how dispatch actually ended, so only that /// placeholder is rewritten — the parse kinds from [`capture_lossy`] and /// `"exec"` from [`finalize_before_exec`] pass through untouched. The mapping -/// mirrors `Error::exit_code()`. +/// mirrors `Error::exit_code()`, with arbitrary child exit codes classified as +/// errors. fn dispatched_outcome(outcome: &'static str, exit_code: i32) -> &'static str { if outcome != "ok" { return outcome; @@ -268,6 +271,14 @@ pub struct Invocation { suggestion: Option, } +impl Invocation { + /// Keep a child's raw status while preventing reserved CLI exit codes from + /// changing its telemetry classification. + pub fn mark_child_exit(&mut self) { + self.outcome = "error"; + } +} + /// Map clap's parse-error kind to the closed outcome vocabulary. Every value /// is a literal owned by this match; the wildcard covers the remaining (and /// future — `ErrorKind` is non-exhaustive) kinds. @@ -902,6 +913,12 @@ mod tests { assert_eq!(dispatched_outcome("ok", 4), "auth_required"); // Any exit code outside the documented vocabulary is still a failure. assert_eq!(dispatched_outcome("ok", 5), "error"); + // The child-exit marker replaces the parse-time placeholder before + // this mapping, so colliding child statuses remain errors. + let mut child = invocation(); + child.mark_child_exit(); + assert_eq!(dispatched_outcome(child.outcome, 3), "error"); + assert_eq!(dispatched_outcome(child.outcome, 4), "error"); // Non-"ok" outcomes are never rewritten, whatever the exit code. assert_eq!( dispatched_outcome("unknown_argument", 2), diff --git a/crates/clickhousectl/tests/telemetry_test.rs b/crates/clickhousectl/tests/telemetry_test.rs index 32db206..14ac2c4 100644 --- a/crates/clickhousectl/tests/telemetry_test.rs +++ b/crates/clickhousectl/tests/telemetry_test.rs @@ -19,6 +19,9 @@ use std::path::PathBuf; use std::process::{Command, Output}; use std::time::{Duration, Instant}; +#[cfg(unix)] +use std::os::unix::fs::PermissionsExt; + use serde_json::Value; use wiremock::matchers::{method, path}; use wiremock::{Mock, MockServer, ResponseTemplate}; @@ -270,6 +273,68 @@ async fn failure_reported_and_positional_value_never_leaks() { ); } +#[cfg(unix)] +#[tokio::test] +async fn child_exit_code_reaches_the_telemetry_tail_unchanged() { + let sandbox = Sandbox::new().await; + sandbox.write_state(false); + let project = tempfile::tempdir().unwrap(); + + let binary = sandbox + .home + .path() + .join(".clickhouse/versions/25.12.9.61/clickhouse"); + std::fs::create_dir_all(binary.parent().unwrap()).unwrap(); + // 3 is clickhousectl's own cancellation code, so this also verifies that + // a child status cannot be mistaken for a CLI cancellation. + std::fs::write(&binary, "#!/bin/sh\nexit 3\n").unwrap(); + let mut permissions = std::fs::metadata(&binary).unwrap().permissions(); + permissions.set_mode(0o755); + std::fs::set_permissions(&binary, permissions).unwrap(); + + let cache = sandbox.home.path().join(".clickhouse/last_update_check"); + let now = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_secs(); + std::fs::write(cache, format!("{now}\n999.0.0")).unwrap(); + + let output = sandbox + .command(&[ + "local", + "server", + "start", + "--version", + "25.12.9.61", + "--foreground", + ]) + .env_clear() + .env("HOME", sandbox.home.path()) + .env( + "CHCTL_TELEMETRY_URL", + format!("{}/v1/telemetry", sandbox.mock.uri()), + ) + .current_dir(project.path()) + .output() + .unwrap(); + + assert_eq!(output.status.code(), Some(3)); + assert!( + !stderr_of(&output).contains("Error: child process exited"), + "child stderr should not gain a wrapper error: {}", + stderr_of(&output) + ); + assert!( + stderr_of(&output).contains("There is a new version of clickhousectl"), + "child failure should still reach the update-notice tail: {}", + stderr_of(&output) + ); + let payloads = sandbox.wait_for_requests(1).await; + assert_eq!(payloads[0]["command"], "local server start"); + assert_eq!(payloads[0]["exit_code"], 3); + assert_eq!(payloads[0]["outcome"], "error"); +} + #[tokio::test] async fn flag_names_sent_but_values_never_leak() { let sandbox = Sandbox::new().await;