Route child exits through telemetry tail (#321)

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);
+    }
 }

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(())
 }

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(())
     }

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

crates/clickhousectl/src/telemetry.rs

@@ -179,17 +179,19 @@ struct Payload {
     command: String,
     flags: Vec<String>,
     /// 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<String>,
 }
 
+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),

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;