From 219fb253c1f40bd57dfdf07843c1098391477ac4 Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Sun, 2 Aug 2026 13:35:35 +0900 Subject: [PATCH 1/3] fix: report a real exit code from bssh ping `ping_nodes` counted per-host successes and failures only to print them and then returned `Ok(())` unconditionally, so `bssh -H unreachable-host ping; echo $?` printed 0 and any script branching on `bssh ping` succeeding passed unconditionally, while the help text promised "0 (all reachable), 1 (any unreachable)". Ping now follows a 0/1/255 contract: 0 when every targeted host connected and authenticated, 1 when at least one host was reachable and at least one failed, and 255 when no host succeeded or when bssh failed before it could attempt any connection. The 0/1 boundary is `ExitCodeStrategy::RequireAllSuccess`, since a health check is green only when every node is green; `MainRank` does not apply because ping runs no user command whose status could be forwarded. The 255 value follows OpenSSH's convention for "ssh itself encountered an error". `ping_nodes` returns a `PingOutcome` tally instead of `Result<()>`, which is a source break for library consumers, `dispatch_command` returns `Result`, and `main::dispatch_and_exit` is the single place that converts a nonzero command-level code into the process exit status. `main::map_hard_failure` maps a hard `Err` on the ping path to 255 so a pre-connection failure stays distinct from the 1 that means partial failure; every other subcommand keeps the generic exit code 1. The `long_about` also claimed ping reports "response times" while printing no timing at all. The claim is removed rather than implemented; per-host timing remains a separate proposal. Validated with `cargo test --lib commands::ping` (5 passed), `cargo test --test ping_exit_code_test` (8 passed), `cargo test --test exit_code_integration_test` (17 passed), `cargo test --bin bssh` (51 passed), `cargo test --test pdsh_compat_test` (35 passed), and `cargo clippy --lib --bins --tests -- -D warnings`. Manually verified that `bssh -H unreachable-host ping; echo $?` now prints 255. Refs #245 --- ARCHITECTURE.md | 3 + CHANGELOG.md | 2 + docs/architecture/exit-code-strategy.md | 40 ++++- docs/man/bssh.1 | 28 +++ src/app/dispatcher.rs | 42 +++-- src/cli/bssh.rs | 2 +- src/commands/ping.rs | 175 ++++++++++++++++++- src/main.rs | 57 ++++++- tests/ping_exit_code_test.rs | 217 ++++++++++++++++++++++++ 9 files changed, 539 insertions(+), 27 deletions(-) create mode 100644 tests/ping_exit_code_test.rs diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 9e5e9de3..4483905f 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -179,6 +179,8 @@ MPI-compatible exit code handling: - Automatic main rank detection (Backend.AI integration) - Preserves actual exit codes (SIGSEGV=139, OOM=137, etc.) +`ping` is the exception: it runs no user command, so no remote status exists to forward and `MainRank` does not apply. It reuses `RequireAllSuccess` for the 0/1 boundary and reports 255 when no host answered or when bssh failed before connecting, following OpenSSH's "ssh itself failed" convention. `ping_nodes` returns a `PingOutcome` tally rather than `Result<()>`, `dispatch_command` returns `Result`, and `main::dispatch_and_exit` is the single place that converts a nonzero command-level code into the process exit status. + ### Shared Module Common utilities for code reuse between bssh client and server implementations: @@ -906,6 +908,7 @@ See [docs/architecture/configuration.md](./docs/architecture/configuration.md) f - **0**: Success (all nodes, or main rank succeeded) - **1**: General failure - **130**: Terminated by SIGINT (Ctrl+C) +- **255**: `ping` only. No host was reachable, or bssh failed before attempting any connection - **Other**: Preserved from main rank (SIGSEGV=139, OOM=137, etc.) See [docs/architecture/exit-code-strategy.md](./docs/architecture/exit-code-strategy.md) for detailed strategy documentation. diff --git a/CHANGELOG.md b/CHANGELOG.md index ad91034f..10b0426f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **Honor known_hosts `@revoked`/`@cert-authority` marker lines, match hostnames case-insensitively, and close the window between creating `~/.ssh`/`known_hosts` and restricting their permissions** (#239). russh's known_hosts parser reads a marker line's own `@revoked`/`@cert-authority` token as the literal host field, so it never matched the real host and both accept-new and strict mode silently ignored these lines, meaning a key an operator had explicitly revoked was recorded and accepted like any ordinary first use. A dedicated scan now runs before the ordinary lookup in every known_hosts-based mode: a `@revoked` line whose host matches (the exact, comma-list, or `*`/`?` glob form, erring toward matching when in doubt since failing to honor a revocation is worse than an unnecessary rejection) and whose key equals the offered key is a hard rejection with a new `HostKeyRevoked` error, while a matching `@revoked` line naming a *different* key does not block the offered one; a `@cert-authority` match only prints a loud warning, since bssh has no CA signature validation to fall back to and failing closed there would break every working CA setup with no workaround, and falls through to ordinary TOFU. Hostname matching is also normalized to lowercase once at the point of verification, for both the lookup and the recorded entry, matching OpenSSH's case-insensitive known_hosts comparison (russh's own matcher is a plain byte compare), so `NODE1.example.com` and `node1.example.com` now hit the same pin instead of the differently-cased spelling looking unknown and re-recording. Finally, `~/.ssh` and `known_hosts` are created with mode 0700/0600 directly (`DirBuilder`/`OpenOptions` with an explicit mode) before `learn_known_hosts_path` runs, rather than letting it create them with the process umask and tightening the mode only afterward, closing the brief window in which an unusually permissive umask could leave either one group- or world-writable. ### Changed +- **Breaking, lib API: `bssh::commands::ping::ping_nodes` now returns `Result` instead of `Result<()>`** (#245). The per-host tally has to survive the return for the caller to compute an exit code from it. `PingOutcome` carries `total`, `succeeded`, and `failed`, and `PingOutcome::exit_code()` applies the 0/1/255 contract. Callers that ignored the old `Ok(())` keep compiling; callers that matched on it exhaustively update the pattern. - **Breaking, lib API: address family selection added a required `AddressFamily` parameter to several public library functions** (#246). Every call site needed the new value anyway, so the maintainer chose a source break over back-compat wrapper overloads; consumers of the `bssh` library crate update call sites when picking up this version. - `ForwardingSpec::parse_local`, `parse_dynamic`, and `parse` now take an `AddressFamily` argument. - The four public `SshClient::*_with_jump_hosts` helpers (`upload_file_with_jump_hosts`, `download_file_with_jump_hosts`, `upload_dir_with_jump_hosts`, `download_dir_with_jump_hosts`) likewise gained a required `AddressFamily` argument. @@ -21,6 +22,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `bssh::ssh::tokio_client::Error` gained a `NoAddressForFamily` variant. ### Fixed +- **Make `bssh ping` report an exit code, instead of always exiting 0 while its help text promised otherwise** (#245). `ping_nodes` counted successes and failures only to print them, returned `Ok(())` unconditionally, and nothing downstream translated the counts, so `bssh -H unreachable-host ping; echo $?` printed 0 and every script that branched on `bssh ping` succeeding passed unconditionally. Ping now follows a 0/1/255 contract: 0 when every targeted host connected and authenticated, 1 when at least one host was reachable and at least one failed, and 255 when no host succeeded or when bssh failed before it could attempt any connection (configuration file that could not be loaded, host list that resolved to nothing). The 0/1 boundary is `ExitCodeStrategy::RequireAllSuccess`, since a health check is green only when every node is green; `MainRank` does not apply because ping runs no user command whose status could be forwarded. The 255 value follows OpenSSH, which reserves it for "ssh itself encountered an error", and it keeps a partially degraded cluster distinct from one that could not be reached at all. This is a user-visible behavior change in both directions: the all-unreachable case previously exited 0 and now exits 255, which is also not the 1 the old help text implied. Command-line usage errors are still rejected by the argument parser with its own exit code 2, unchanged and identical across subcommands. The `ping` help text also claimed it "Reports connection status, authentication success, and response times" while printing no timing at all; the claim is removed rather than implemented, and per-host timing remains a separate proposal. - **Wire up the `-4` / `-6` address family flags and the ssh_config `AddressFamily` keyword, which were parsed and then discarded** (#246). Both were advertised in `--help` and the man page and had no effect: `bssh -6` against a dual-stack host could still connect over IPv4, and `AddressFamily any|inet|inet6` round-tripped through the config layer without ever being read. A single `AddressFamily` preference is now resolved once per dispatch path with OpenSSH precedence (command line flag over config keyword over the `any` default), carried on `SshConnectionConfig`, and applied when the connect path filters resolved addresses. It covers direct connections for `exec`, interactive sessions, `ping`, and SFTP `upload`/`download`, plus the first hop of a `-J` chain, which shares that path. Forcing a family that has no resolved address is a hard failure with no fallback to the other family, matching OpenSSH, reported as `no IPv6 address found for ` (or the IPv4 equivalent) instead of the generic `could not resolve to any addresses`. An unrecognized `AddressFamily` value warns and is treated as `any` rather than failing a configuration file OpenSSH would accept. With neither the flag nor the keyword set, every resolved address is still tried in resolver order, unchanged. - **Apply the address family preference to port forwarding, changing the default `-L`/`-D` listen address under `-6`** (#246). This is a user-visible default change: with `-6`, a `-L` or `-D` specification that does not name a bind address now listens on `::1` instead of `127.0.0.1`, and the `*:port` wildcard form listens on `::` instead of `0.0.0.0`. A specification that names a bind address explicitly is unaffected, as are `-4` and the no-flag default, which both keep the IPv4 loopback. Scripts that pass `-6` and assume an IPv4 loopback listener should name the bind address explicitly (for example `-L 127.0.0.1:8080:example.com:80`). Forwarding *targets* are also filtered by the forced family for `-L` and SOCKS5 `-D` requests, but the remote server performs the actual connect, so that filtering is a best-effort hint; SOCKS4 requests carry a literal IPv4 destination by protocol definition and are passed through unfiltered. The `-R` listener lives on the server and is not governed by the local flag. Jump hops past the first one still resolve their target locally through the same `direct-tcpip` mechanism, but this change does not yet apply the family filter there (scope limitation, tracked as issue #248); the family is still honored when selecting the address recorded for host key verification on those hops. The man page documents the full scope, including this limitation. - **Show the full error context chain when an interactive connection fails** (#238). Interactive mode printed only anyhow's outermost context, so a jump-host failure surfaced as `Failed to establish jump host connection to ` with no indication of which hop failed or why, leaving first-jump authentication failures, destination TCP refusals, and destination authentication failures indistinguishable. Both interactive execution paths (PTY and traditional) and the directory-download failure message now render the whole chain through a shared `format_connection_error` helper that uses anyhow's alternate `Display` form, matching the format the parallel exec path already used. Tracing verbosity and exec-mode output are unchanged. diff --git a/docs/architecture/exit-code-strategy.md b/docs/architecture/exit-code-strategy.md index a97d4015..06323287 100644 --- a/docs/architecture/exit-code-strategy.md +++ b/docs/architecture/exit-code-strategy.md @@ -8,6 +8,7 @@ - [Main Rank Detection](#main-rank-detection) - [Exit Code Strategies](#exit-code-strategies) - [Strategy Comparison](#strategy-comparison) +- [The ping Contract](#the-ping-contract) - [Implementation Details](#implementation-details) - [Migration Guide](#migration-guide) @@ -194,6 +195,39 @@ ExitCodeStrategy::MainRankWithFailureCheck => { **Bold** values show where strategies differ. +## The ping Contract + +`bssh ping` does not use `ExitCodeStrategy` directly, because it runs no user command whose status could be forwarded. `MainRank` is therefore meaningless for it. Instead it reuses the `RequireAllSuccess` semantics for the 0/1 boundary (a health check is green only when every host is green) and adds one value that no other path produces: 255. + +| Scenario | Exit code | +|----------|-----------| +| Every targeted host connected and authenticated | 0 | +| At least one host reachable, at least one failed | 1 | +| No host succeeded | 255 | +| bssh failed before attempting any connection (config load failure, no host resolved) | 255 | + +**Why 255**: OpenSSH reserves 255 for "ssh itself encountered an error" as opposed to a status forwarded from the remote command. Since ping has no remote command, every total failure is by definition an ssh-level failure. The split lets a caller distinguish a partially degraded cluster (1) from one it could not reach at all, or could not even start against (255). + +**Implementation**: `src/commands/ping.rs` returns a `PingOutcome { total, succeeded, failed }` instead of `Result<()>`, and `PingOutcome::exit_code()` applies the table above: + +```rust +pub fn exit_code(&self) -> i32 { + if self.succeeded == 0 { + PING_SSH_LEVEL_FAILURE // 255, including an empty host list + } else if self.failed > 0 { + 1 // RequireAllSuccess: any failure is a failure + } else { + 0 + } +} +``` + +A unit test asserts that `PingOutcome::exit_code()` equals `ExitCodeStrategy::RequireAllSuccess.calculate()` for every case where at least one host answered, so the two cannot drift apart. + +**Error propagation**: an `Err` returned from the ping path is a hard failure raised before any per-host tally exists, so it maps to 255 rather than to the 1 that means "some hosts answered and some did not". `main::map_hard_failure` applies that mapping on the ping path only; every other subcommand keeps the default, where returning `Err` from `main` prints the chain and exits 1. Command-line usage errors are rejected by clap before this contract applies and keep clap's own exit code 2, as they do for every subcommand. + +**Exit code plumbing**: `dispatch_command` returns `Result` and `main::dispatch_and_exit` is the single place that turns a nonzero command-level code into the process exit status. The `exec` path predates this and still calls `std::process::exit` itself from `src/commands/exec.rs` after applying its `ExitCodeStrategy`. + ## Implementation Details ### File Structure @@ -207,7 +241,11 @@ src/executor/ └── parallel.rs # Marks main rank in results src/commands/ -└── exec.rs # Applies exit strategy based on CLI flags +├── exec.rs # Applies exit strategy based on CLI flags +└── ping.rs # PingOutcome and the 0/1/255 ping contract + +src/app/ +└── dispatcher.rs # Returns the command-level exit code to main src/ └── cli.rs # CLI flags: --require-all-success, --check-all-nodes diff --git a/docs/man/bssh.1 b/docs/man/bssh.1 index 13e1ab02..f01e902d 100644 --- a/docs/man/bssh.1 +++ b/docs/man/bssh.1 @@ -1867,6 +1867,34 @@ Main rank succeeded but other nodes failed .B 2-255 Main rank failed with this exit code +.SS ping Subcommand +The +.B ping +subcommand runs no remote command, so there is no remote exit status to +forward and the main rank strategy does not apply. Its 0/1 boundary follows +.B --require-all-success +(a health check is green only when every host is green), and total failure is +reported as 255 following OpenSSH's convention that 255 means ssh itself +encountered an error: + +.TP +.B 0 +Every targeted host connected and authenticated + +.TP +.B 1 +At least one host was reachable and at least one failed + +.TP +.B 255 +No host was reachable, or bssh failed before it could attempt any connection +(configuration file could not be loaded, no host resolved, and similar) + +.PP +Command-line usage errors are rejected by the argument parser before this +contract applies and keep the parser's own exit code 2, as they do for every +subcommand. + .SH OUTPUT FILES When using the .B --output-dir diff --git a/src/app/dispatcher.rs b/src/app/dispatcher.rs index 05d12609..e950e255 100644 --- a/src/app/dispatcher.rs +++ b/src/app/dispatcher.rs @@ -40,6 +40,10 @@ use super::initialization::determine_use_keychain; use super::initialization::{AppContext, determine_ssh_key_path}; use super::utils::format_duration; +/// Exit code for a subcommand that completed without a per-host failure to +/// report. +const EXIT_SUCCESS: i32 = 0; + /// Build SSH connection config with keepalive, compression, and address /// family settings. /// Precedence: CLI > SSH config > YAML config > defaults. @@ -154,8 +158,14 @@ fn subcommand_name(command: &Option) -> &'static str { } } -/// Dispatch commands to their appropriate handlers -pub async fn dispatch_command(cli: &Cli, ctx: &AppContext) -> Result<()> { +/// Dispatch commands to their appropriate handlers. +/// +/// Returns the exit code the process should report. Most subcommands either +/// succeed (0) or return `Err`, but `ping` completes normally while still +/// having per-host failures to report, so the count has to survive the return. +/// The caller (`main`) is the single place that turns a nonzero value into the +/// process exit status. +pub async fn dispatch_command(cli: &Cli, ctx: &AppContext) -> Result { // Get command to execute let command = cli.get_command(); @@ -219,7 +229,7 @@ pub async fn dispatch_command(cli: &Cli, ctx: &AppContext) -> Result<()> { match &cli.command { Some(Commands::List) => { list_clusters(&ctx.config); - Ok(()) + Ok(EXIT_SUCCESS) } Some(Commands::Ping) => { let key_path = determine_ssh_key_path( @@ -247,7 +257,10 @@ pub async fn dispatch_command(cli: &Cli, ctx: &AppContext) -> Result<()> { ctx.cluster_name.as_deref().or(cli.cluster.as_deref()), ); - ping_nodes( + // `ping` is the one subcommand whose exit code depends on how many + // hosts answered, so the outcome is translated here rather than + // discarded. See `PingOutcome` for the 0/1/255 contract. + let outcome = ping_nodes( ctx.nodes.clone(), ctx.max_parallel, key_path.as_deref(), @@ -262,7 +275,9 @@ pub async fn dispatch_command(cli: &Cli, ctx: &AppContext) -> Result<()> { ssh_password.clone(), ssh_connection_config, ) - .await + .await?; + + Ok(outcome.exit_code()) } Some(Commands::Upload { source, @@ -300,7 +315,8 @@ pub async fn dispatch_command(cli: &Cli, ctx: &AppContext) -> Result<()> { hostname_for_ssh_config.as_deref(), ), }; - upload_file(params, source, destination).await + upload_file(params, source, destination).await?; + Ok(EXIT_SUCCESS) } Some(Commands::Download { source, @@ -338,7 +354,8 @@ pub async fn dispatch_command(cli: &Cli, ctx: &AppContext) -> Result<()> { hostname_for_ssh_config.as_deref(), ), }; - download_file(params, source, destination).await + download_file(params, source, destination).await?; + Ok(EXIT_SUCCESS) } Some(Commands::Interactive { single_node, @@ -357,15 +374,20 @@ pub async fn dispatch_command(cli: &Cli, ctx: &AppContext) -> Result<()> { work_dir.as_deref(), ssh_password.clone(), ) - .await + .await?; + Ok(EXIT_SUCCESS) } Some(Commands::CacheStats { .. }) => { // This is handled in main.rs before node resolution unreachable!("CacheStats should be handled before dispatch") } None => { - // Execute command (auto-exec or interactive shell) - handle_exec_command(cli, ctx, &command, ssh_password.clone()).await + // Execute command (auto-exec or interactive shell). This path owns + // its own exit code strategy (`ExitCodeStrategy`, selected by + // `--require-all-success` / `--check-all-nodes`) and exits the + // process itself when the strategy yields a nonzero code. + handle_exec_command(cli, ctx, &command, ssh_password.clone()).await?; + Ok(EXIT_SUCCESS) } } } diff --git a/src/cli/bssh.rs b/src/cli/bssh.rs index 3aa94f29..75407477 100644 --- a/src/cli/bssh.rs +++ b/src/cli/bssh.rs @@ -340,7 +340,7 @@ pub enum Commands { #[command( about = "Test connectivity to hosts", - long_about = "Verifies SSH connectivity and authentication to all target hosts.\nReports connection status, authentication success, and response times.\nUseful for validating cluster configuration and SSH key setup.\n\nExit codes: 0 (all reachable), 1 (any unreachable)" + long_about = "Verifies SSH connectivity and authentication to all target hosts.\nReports per-host connection status and the reason each failure occurred.\nUseful for validating cluster configuration and SSH key setup.\n\nExit codes:\n 0 every host connected and authenticated\n 1 at least one host reachable, at least one unreachable\n 255 no host reachable, or bssh failed before connecting" )] Ping, diff --git a/src/commands/ping.rs b/src/commands/ping.rs index 5ee3e5f1..61ee4b20 100644 --- a/src/commands/ping.rs +++ b/src/commands/ping.rs @@ -18,18 +18,90 @@ use std::path::Path; use std::sync::Arc; use crate::commands::error_format::format_connection_error; -use crate::executor::ParallelExecutor; +use crate::executor::{ExecutionResult, ParallelExecutor}; use crate::node::Node; use crate::security::Password; use crate::ssh::known_hosts::StrictHostKeyChecking; use crate::ssh::tokio_client::SshConnectionConfig; use crate::ui::OutputFormatter; +/// Exit code reported when bssh itself could not complete the connectivity +/// check: no targeted host answered, no host was resolved, or a hard error was +/// raised before the first connection attempt. +/// +/// OpenSSH reserves 255 for "ssh itself encountered an error" as opposed to a +/// status forwarded from a remote command. `ping` runs no remote command, so +/// every total failure is by definition an ssh-level failure, and a caller can +/// use this value to tell "I could not reach anything" apart from the exit code +/// 1 that means "the cluster is partially degraded". +pub const PING_SSH_LEVEL_FAILURE: i32 = 255; + +/// Aggregate result of a `bssh ping` run, returned so the caller can translate +/// it into the process exit status. +/// +/// # Exit code contract +/// +/// | Scenario | Exit code | +/// |---|---| +/// | Every targeted host connected and authenticated | 0 | +/// | At least one host reachable, at least one failed | 1 | +/// | No host succeeded (including an empty host list) | 255 | +/// +/// The 0/1 boundary is [`ExitCodeStrategy::RequireAllSuccess`]: ping is a health +/// check, so it is only green when every node is green. +/// [`ExitCodeStrategy::MainRank`] does not apply, because ping runs no user +/// command whose status could be forwarded. Total failure is escalated to +/// [`PING_SSH_LEVEL_FAILURE`] on top of that strategy. +/// +/// [`ExitCodeStrategy::RequireAllSuccess`]: crate::executor::ExitCodeStrategy::RequireAllSuccess +/// [`ExitCodeStrategy::MainRank`]: crate::executor::ExitCodeStrategy::MainRank +#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] +pub struct PingOutcome { + /// Number of hosts that were targeted. + pub total: usize, + /// Number of hosts that connected and authenticated. + pub succeeded: usize, + /// Number of hosts that could not be reached. + pub failed: usize, +} + +impl PingOutcome { + /// Count successes and failures from the executor results. + pub fn from_results(results: &[ExecutionResult]) -> Self { + let succeeded = results.iter().filter(|r| r.is_success()).count(); + Self { + total: results.len(), + succeeded, + failed: results.len() - succeeded, + } + } + + /// Process exit status for this outcome, per the contract on + /// [`PingOutcome`]. + pub fn exit_code(&self) -> i32 { + if self.succeeded == 0 { + // Nothing answered, so bssh never got a usable connection. This + // also covers an empty host list. + PING_SSH_LEVEL_FAILURE + } else if self.failed > 0 { + // RequireAllSuccess: any failure is a failure. + 1 + } else { + 0 + } + } +} + /// Test connectivity to every node. /// /// `ssh_connection_config` carries the resolved keepalive, compression, and /// address family settings so `bssh ping -6` tests the same address family the /// real connection would use. +/// +/// Returns the per-host tally as a [`PingOutcome`]; the caller turns it into the +/// process exit status. An `Err` means bssh failed before it could evaluate any +/// host, which the caller maps to [`PING_SSH_LEVEL_FAILURE`] rather than to the +/// exit code 1 that means "some hosts answered and some did not". #[allow(clippy::too_many_arguments)] pub async fn ping_nodes( nodes: Vec, @@ -44,7 +116,7 @@ pub async fn ping_nodes( jump_hosts: Option, ssh_password: Option>, ssh_connection_config: SshConnectionConfig, -) -> Result<()> { +) -> Result { println!( "{}", OutputFormatter::format_command_header("ping", nodes.len()) @@ -75,15 +147,12 @@ pub async fn ping_nodes( // Use normal execution (no TUI, no streaming) for ping let results = executor.execute("true").await?; - - let mut success_count = 0; - let mut failed_count = 0; + let outcome = PingOutcome::from_results(&results); println!("\n{} {}\n", "▶".cyan(), "Connection Test Results".bold()); for result in &results { if result.is_success() { - success_count += 1; println!( " {} {} - {}", "●".green(), @@ -91,7 +160,6 @@ pub async fn ping_nodes( "Connected".green() ); } else { - failed_count += 1; println!( " {} {} - {}", "●".red(), @@ -115,8 +183,97 @@ pub async fn ping_nodes( println!( "{}", - OutputFormatter::format_summary(nodes.len(), success_count, failed_count) + OutputFormatter::format_summary(nodes.len(), outcome.succeeded, outcome.failed) ); - Ok(()) + Ok(outcome) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::executor::ExitCodeStrategy; + use crate::ssh::client::CommandResult; + use anyhow::anyhow; + + fn reachable(host: &str) -> ExecutionResult { + ExecutionResult { + node: Node::new(host.to_string(), 22, "user".to_string()), + result: Ok(CommandResult { + host: host.to_string(), + output: Vec::new(), + stderr: Vec::new(), + exit_status: 0, + }), + is_main_rank: false, + } + } + + fn unreachable(host: &str) -> ExecutionResult { + ExecutionResult { + node: Node::new(host.to_string(), 22, "user".to_string()), + result: Err(anyhow!("Connection refused")), + is_main_rank: false, + } + } + + #[test] + fn all_hosts_reachable_exits_zero() { + let outcome = PingOutcome::from_results(&[reachable("host1"), reachable("host2")]); + + assert_eq!(outcome.succeeded, 2); + assert_eq!(outcome.failed, 0); + assert_eq!(outcome.exit_code(), 0); + } + + #[test] + fn partial_failure_exits_one() { + let outcome = PingOutcome::from_results(&[ + reachable("host1"), + unreachable("host2"), + reachable("host3"), + ]); + + assert_eq!(outcome.succeeded, 2); + assert_eq!(outcome.failed, 1); + assert_eq!(outcome.exit_code(), 1); + } + + #[test] + fn total_failure_exits_255() { + let outcome = PingOutcome::from_results(&[unreachable("host1"), unreachable("host2")]); + + assert_eq!(outcome.succeeded, 0); + assert_eq!(outcome.failed, 2); + assert_eq!(outcome.exit_code(), PING_SSH_LEVEL_FAILURE); + } + + #[test] + fn empty_host_list_exits_255() { + let outcome = PingOutcome::from_results(&[]); + + assert_eq!(outcome.total, 0); + assert_eq!(outcome.exit_code(), PING_SSH_LEVEL_FAILURE); + } + + #[test] + fn matches_require_all_success_whenever_a_host_answered() { + // The 0/1 boundary is ExitCodeStrategy::RequireAllSuccess. Ping only + // departs from it when nothing answered at all, where OpenSSH's 255 + // takes over. + let cases = vec![ + vec![reachable("host1"), reachable("host2")], + vec![reachable("host1"), unreachable("host2")], + vec![unreachable("host1"), reachable("host2")], + ]; + + for results in cases { + let outcome = PingOutcome::from_results(&results); + assert_eq!( + outcome.exit_code(), + ExitCodeStrategy::RequireAllSuccess.calculate(&results, None), + "ping must agree with RequireAllSuccess when at least one host answered" + ); + } + } } diff --git a/src/main.rs b/src/main.rs index 34e56fff..9c86e5c1 100644 --- a/src/main.rs +++ b/src/main.rs @@ -16,6 +16,7 @@ use anyhow::Result; use bssh::cli::{ Cli, Commands, PdshCli, has_pdsh_compat_flag, is_pdsh_compat_mode, remove_pdsh_compat_flag, }; +use bssh::commands::ping::PING_SSH_LEVEL_FAILURE; use bssh::hostlist; use clap::Parser; use glob::Pattern; @@ -23,8 +24,11 @@ use glob::Pattern; mod app; use app::{ - cache::handle_cache_stats, dispatcher::dispatch_command, initialization::initialize_app, - query::handle_query, utils::show_usage, + cache::handle_cache_stats, + dispatcher::dispatch_command, + initialization::{AppContext, initialize_app}, + query::handle_query, + utils::show_usage, }; /// Main entry point for bssh @@ -49,6 +53,42 @@ async fn main() -> Result<()> { run_bssh_mode(&args).await } +/// Run the dispatcher and translate its result into the process exit status. +/// +/// This is the single place where a command-level exit code becomes a process +/// exit code. `dispatch_command` returns the code instead of exiting itself, so +/// the mapping stays in one place instead of being scattered across command +/// implementations. +async fn dispatch_and_exit(cli: &Cli, ctx: &AppContext) -> Result<()> { + match dispatch_command(cli, ctx).await { + Ok(0) => Ok(()), + Ok(exit_code) => std::process::exit(exit_code), + Err(e) => Err(map_hard_failure(&cli.command, e)), + } +} + +/// Apply the `ping` exit code contract to a hard failure. +/// +/// `ping` reports 255 whenever bssh itself failed rather than a remote host: a +/// configuration file that could not be loaded, a host list that resolved to +/// nothing, or any error raised before the connectivity check could produce a +/// per-host tally. That is OpenSSH's convention for "ssh encountered an error", +/// and it keeps the pre-connection case distinct from the exit code 1 that means +/// "some hosts answered and some did not". +/// +/// Every other subcommand keeps the default behavior, where returning `Err` from +/// `main` prints the error chain and exits 1. +fn map_hard_failure(command: &Option, error: anyhow::Error) -> anyhow::Error { + if matches!(command, Some(Commands::Ping)) { + // Match the format anyhow's `Termination` impl uses, since this path + // replaces it. + eprintln!("Error: {error:?}"); + std::process::exit(PING_SSH_LEVEL_FAILURE); + } + + error +} + /// Run in pdsh compatibility mode /// /// Parses pdsh-style arguments and converts them to bssh CLI options. @@ -87,7 +127,7 @@ async fn run_pdsh_mode(args: &[String]) -> Result<()> { // Initialize and run let ctx = initialize_app(&mut cli, args).await?; - dispatch_command(&cli, &ctx).await + dispatch_and_exit(&cli, &ctx).await } /// Handle pdsh query mode (-q) @@ -230,9 +270,14 @@ async fn run_bssh_mode(args: &[String]) -> Result<()> { return Ok(()); } - // Initialize the application and load all configurations - let ctx = initialize_app(&mut cli, args).await?; + // Initialize the application and load all configurations. A failure here is + // a pre-connection failure, which `ping` reports as 255. + let init_result = initialize_app(&mut cli, args).await; + let ctx = match init_result { + Ok(ctx) => ctx, + Err(e) => return Err(map_hard_failure(&cli.command, e)), + }; // Dispatch to the appropriate command handler - dispatch_command(&cli, &ctx).await + dispatch_and_exit(&cli, &ctx).await } diff --git a/tests/ping_exit_code_test.rs b/tests/ping_exit_code_test.rs new file mode 100644 index 00000000..78316d41 --- /dev/null +++ b/tests/ping_exit_code_test.rs @@ -0,0 +1,217 @@ +// Copyright 2025 Lablup Inc. and Jeongkyu Shin +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! Integration tests for the `bssh ping` exit code contract. +//! +//! Two layers are covered: +//! +//! 1. Contract tests over `PingOutcome`, which is the value `main` hands to +//! `std::process::exit`. These follow the style of +//! `tests/exit_code_integration_test.rs` and pin all three cases: 0, 1, 255. +//! 2. Process-level tests that run the real binary and assert the observed exit +//! status. These cover every case reachable without a live SSH endpoint: all +//! hosts unreachable, and the pre-connection failures (configuration load +//! failure, no host resolved). The 0 and 1 cases need a host that actually +//! accepts an SSH connection, so they are pinned at the `PingOutcome` +//! boundary instead. + +use std::path::Path; +use std::process::Command; + +use anyhow::anyhow; +use bssh::commands::ping::{PING_SSH_LEVEL_FAILURE, PingOutcome}; +use bssh::executor::{ExecutionResult, ExitCodeStrategy}; +use bssh::node::Node; +use bssh::ssh::client::CommandResult; +use tempfile::NamedTempFile; + +/// A host that connected and authenticated. +fn reachable(host: &str) -> ExecutionResult { + ExecutionResult { + node: Node::new(host.to_string(), 22, "user".to_string()), + result: Ok(CommandResult { + host: host.to_string(), + output: Vec::new(), + stderr: Vec::new(), + exit_status: 0, + }), + is_main_rank: false, + } +} + +/// A host that could not be reached. +fn unreachable(host: &str) -> ExecutionResult { + ExecutionResult { + node: Node::new(host.to_string(), 22, "user".to_string()), + result: Err(anyhow!("Connection refused")), + is_main_rank: false, + } +} + +#[test] +fn all_hosts_reachable_yields_exit_code_0() { + let outcome = + PingOutcome::from_results(&[reachable("host1"), reachable("host2"), reachable("host3")]); + + assert_eq!(outcome.exit_code(), 0, "every host answered"); +} + +#[test] +fn partial_failure_yields_exit_code_1() { + let outcome = + PingOutcome::from_results(&[reachable("host1"), unreachable("host2"), reachable("host3")]); + + assert_eq!( + outcome.exit_code(), + 1, + "a partially degraded cluster is exit code 1" + ); + assert_eq!( + outcome.exit_code(), + ExitCodeStrategy::RequireAllSuccess.calculate( + &[reachable("host1"), unreachable("host2"), reachable("host3")], + None + ), + "the 0/1 boundary must stay aligned with RequireAllSuccess" + ); +} + +#[test] +fn total_failure_yields_exit_code_255() { + let outcome = PingOutcome::from_results(&[unreachable("host1"), unreachable("host2")]); + + assert_eq!( + outcome.exit_code(), + PING_SSH_LEVEL_FAILURE, + "no host answered, so bssh itself failed" + ); +} + +#[test] +fn empty_host_list_yields_exit_code_255() { + let outcome = PingOutcome::from_results(&[]); + + assert_eq!(outcome.exit_code(), PING_SSH_LEVEL_FAILURE); +} + +/// Build a `bssh` invocation isolated from the developer's environment: no SSH +/// agent, no `~/.ssh/config`, no Backend.AI cluster variables. +fn bssh() -> Command { + let mut cmd = Command::new(env!("CARGO_BIN_EXE_bssh")); + cmd.env_remove("SSH_AUTH_SOCK") + .env_remove("BACKENDAI_CLUSTER_HOSTS") + .env_remove("BACKENDAI_CLUSTER_HOST") + .env_remove("BACKENDAI_CLUSTER_ROLE") + .args(["--ssh-config", "/dev/null"]); + cmd +} + +/// A config file with no clusters, so the run depends only on `-H`. +fn empty_config() -> NamedTempFile { + let file = NamedTempFile::new().expect("failed to create temporary config file"); + std::fs::write(file.path(), "clusters: {}\n").expect("failed to write temporary config file"); + file +} + +/// Run `bssh` to completion and return its exit code. +fn run(args: &[&str], config: Option<&Path>) -> i32 { + let mut cmd = bssh(); + if let Some(path) = config { + cmd.arg("--config").arg(path); + } + let output = cmd.args(args).output().expect("failed to run bssh"); + + output + .status + .code() + .unwrap_or_else(|| panic!("bssh was terminated by a signal: {:?}", output.status)) +} + +#[test] +fn ping_with_no_reachable_host_exits_255() { + let config = empty_config(); + let code = run( + &[ + "--connect-timeout", + "2", + "-H", + // RFC 2606 reserves .invalid, so this never resolves. + "bssh-unreachable-host.invalid", + "ping", + ], + Some(config.path()), + ); + + assert_eq!( + code, PING_SSH_LEVEL_FAILURE, + "ping must report 255 when no host answered" + ); +} + +#[test] +fn ping_with_unloadable_config_exits_255() { + let code = run( + &[ + "--config", + "/nonexistent-directory-for-bssh-tests/config.yaml", + "-H", + "bssh-unreachable-host.invalid", + "ping", + ], + None, + ); + + assert_eq!( + code, PING_SSH_LEVEL_FAILURE, + "a pre-connection failure must report 255, not 1" + ); +} + +#[test] +fn ping_with_no_host_resolved_exits_255() { + let config = empty_config(); + let code = run( + &[ + "-H", + "bssh-unreachable-host.invalid", + "--filter", + "no-such-host", + "ping", + ], + Some(config.path()), + ); + + assert_eq!( + code, PING_SSH_LEVEL_FAILURE, + "resolving to zero hosts must report 255, not 1" + ); +} + +#[test] +fn non_ping_subcommands_keep_the_generic_failure_exit_code() { + // The 255 mapping is scoped to ping. Any other subcommand still exits 1 on + // the same pre-connection failure. + let code = run( + &[ + "--config", + "/nonexistent-directory-for-bssh-tests/config.yaml", + "-H", + "bssh-unreachable-host.invalid", + "true", + ], + None, + ); + + assert_eq!(code, 1, "non-ping paths keep the generic exit code 1"); +} From 89fd812366a7866f1a2a722e6d530ae3d6bc5624 Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Sun, 2 Aug 2026 13:49:48 +0900 Subject: [PATCH 2/3] docs: correct the claim that exit code 255 is unique to ping The default MainRank exit code strategy forwards a remote command's exit status verbatim, so `bssh -H host "exit 255"` already exits 255 today, which the man page's exit status table documents under `1-255 Main rank failed with this exit code`. ARCHITECTURE.md and exit-code-strategy.md both stated that 255 is produced only by ping, which contradicts that behavior and the very next bullet in ARCHITECTURE.md ("Other: Preserved from main rank"). Both statements are reworded to say that ping is the only path that generates 255 as a bssh-level signal, while the exec path can still report 255 by forwarding that status from a remote command under MainRank. Refs #245 --- ARCHITECTURE.md | 2 +- docs/architecture/exit-code-strategy.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 4483905f..f605d0d1 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -908,7 +908,7 @@ See [docs/architecture/configuration.md](./docs/architecture/configuration.md) f - **0**: Success (all nodes, or main rank succeeded) - **1**: General failure - **130**: Terminated by SIGINT (Ctrl+C) -- **255**: `ping` only. No host was reachable, or bssh failed before attempting any connection +- **255**: Generated by `ping` as a bssh-level signal when no host was reachable, or bssh failed before attempting any connection. The default `MainRank` strategy can also produce 255 by forwarding a remote command's exit status verbatim - **Other**: Preserved from main rank (SIGSEGV=139, OOM=137, etc.) See [docs/architecture/exit-code-strategy.md](./docs/architecture/exit-code-strategy.md) for detailed strategy documentation. diff --git a/docs/architecture/exit-code-strategy.md b/docs/architecture/exit-code-strategy.md index 06323287..65f17067 100644 --- a/docs/architecture/exit-code-strategy.md +++ b/docs/architecture/exit-code-strategy.md @@ -197,7 +197,7 @@ ExitCodeStrategy::MainRankWithFailureCheck => { ## The ping Contract -`bssh ping` does not use `ExitCodeStrategy` directly, because it runs no user command whose status could be forwarded. `MainRank` is therefore meaningless for it. Instead it reuses the `RequireAllSuccess` semantics for the 0/1 boundary (a health check is green only when every host is green) and adds one value that no other path produces: 255. +`bssh ping` does not use `ExitCodeStrategy` directly, because it runs no user command whose status could be forwarded. `MainRank` is therefore meaningless for it. Instead it reuses the `RequireAllSuccess` semantics for the 0/1 boundary (a health check is green only when every host is green) and adds one value it generates itself as a bssh-level signal: 255. The exec path can still report 255, but only when the default `MainRank` strategy forwards that exact status from a remote command. | Scenario | Exit code | |----------|-----------| From a92792f227a3b4aadca56c0fe58edaabb76cc9a2 Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Sun, 2 Aug 2026 13:56:35 +0900 Subject: [PATCH 3/3] refactor: feed the ping summary line from the outcome tally `format_summary` took `nodes.len()` for the total while the succeeded and failed columns came from `PingOutcome`, so `PingOutcome::total` was written but never read outside tests. The two agree today because ping never enables fail-fast, but sourcing all three columns from the same tally removes the chance of them drifting apart if that changes. --- src/commands/ping.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/commands/ping.rs b/src/commands/ping.rs index 61ee4b20..0406cd0d 100644 --- a/src/commands/ping.rs +++ b/src/commands/ping.rs @@ -183,7 +183,7 @@ pub async fn ping_nodes( println!( "{}", - OutputFormatter::format_summary(nodes.len(), outcome.succeeded, outcome.failed) + OutputFormatter::format_summary(outcome.total, outcome.succeeded, outcome.failed) ); Ok(outcome)