fix: ping always exits 0 despite documenting exit code 1 for unreachable hosts

[inureyes]

Problem / Background

The ping subcommand's help text promises exit codes that the implementation never produces. bssh ping always exits 0, even when every target host is unreachable. Any script or CI job that branches on bssh ping succeeding is silently broken: the check passes unconditionally.

Documented contract (src/cli/bssh.rs, lines 341 to 345):

#[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)"
)]
Ping,

Actual behavior, traced across three hops:

1. src/commands/ping.rs, ping_nodes(...) counts success_count and failed_count, prints them via OutputFormatter::format_summary(nodes.len(), success_count, failed_count), and then returns Ok(()) unconditionally. The counts are used for display only and are discarded.
2. src/app/dispatcher.rs, the Some(Commands::Ping) arm (around line 208) returns that Result straight through with no exit code translation.
3. src/main.rs, run_bssh_mode returns dispatch_command(&cli, &ctx).await directly. std::process::exit(1) appears in main.rs only on unrelated early error paths (pdsh query mode with no hosts, no command specified, and similar). Nothing converts a partially or fully failed ping into a nonzero status.

Reproduction:

bssh -H unreachable-host ping; echo $?   # prints 0

Current vs Expected Behavior

Scenario	Current	Expected (per current help text)
All hosts reachable	0	0
Some hosts unreachable	0	1
No host reachable	0	1 (or 255, see proposal below)
bssh failed before attempting any connection	varies by error path	255 (proposed)

Proposed Exit Code Mapping (needs sign off)

This is a proposal, not a settled decision. Two readings are on the table.

Existing in-repo precedent. No new scheme should be invented. src/executor/exit_strategy.rs already defines ExitCodeStrategy with three variants (MainRank, RequireAllSuccess, MainRankWithFailureCheck) and calculate(&self, results: &[ExecutionResult], main_idx: Option<usize>) -> i32. The rationale is documented in docs/architecture/exit-code-strategy.md: the default follows standard MPI tools (mpirun, srun, mpiexec) by returning the main rank's exit code, and --require-all-success returns 0 only if all nodes succeeded and 1 if any node failed. Ping is a health check, so RequireAllSuccess is the semantically correct alignment; MainRank makes no sense here because ping runs no user command whose status could be forwarded.

OpenSSH semantics. ssh exits with the remote command's exit status, and reserves 255 for "ssh itself encountered an error" (connection failure, authentication failure, bad usage). This matters for ping specifically: ping has no remote command whose status could be forwarded, so every ping failure is by definition an ssh-level failure.

Option A, strict reading of the current text (0/1 only):

• 0: every targeted host connected and authenticated successfully.
• 1: any host failed, including the case where none succeeded.

This matches RequireAllSuccess exactly and changes no documented behavior.

Option B, OpenSSH-aligned refinement (0/1/255), recommended:

• 0: every targeted host connected and authenticated successfully.
• 1: at least one host was reachable but at least one failed. This preserves the currently documented contract for the partial-failure case and matches RequireAllSuccess.
• 255: no host succeeded, or bssh failed before it could attempt any connection (config load failure, no hosts resolved, bad arguments). This matches OpenSSH's "ssh itself failed" convention and lets callers distinguish "the cluster is partially degraded" from "I could not reach anything / I never got off the ground".

The tension, stated explicitly so a reviewer can decide: the current help text says 1 (any unreachable), which under a strict reading makes the all-unreachable case exit 1, not 255. Adopting 255 for total failure is a deliberate, OpenSSH-compatible refinement that changes documented behavior. Recommendation is Option B, with the caveat that it is a user-visible behavior change and warrants a changelog note. If Option A is chosen instead, the fix reduces to propagating the failure count and the help text needs no change beyond the response-times correction below.

Whichever option is chosen, the implementation must state which ExitCodeStrategy it aligns with and stay consistent with docs/architecture/exit-code-strategy.md.

Secondary Issue: "response times" is also inaccurate

The same long_about claims ping "Reports connection status, authentication success, and response times." The implementation in src/commands/ping.rs prints only Connected or Failed per node, plus the formatted error chain on failure. It prints no timing at all.

Two ways to resolve, and the choice should be made explicitly:

1. Add per-host response time measurement to the ping output.
2. Correct the help text to drop the "response times" claim.

Recommendation is option 2 (correct the text) unless timing is actually wanted, since it is the smaller change and removes a false claim immediately. If option 1 is chosen, it should be split into a separate issue so it does not block the exit code fix.

Acceptance Criteria

• ping_nodes (or the Some(Commands::Ping) dispatcher arm) propagates the failure count to the process exit status. Real integration into the code flow is required: a helper that computes a code but is never wired to the process exit does not satisfy this.
• The chosen exit code mapping (Option A or Option B) is agreed on in this issue before implementation, and the implementation names the ExitCodeStrategy variant it aligns with.
• Tests assert the actual exit code for all three cases: all-success (0), partial-failure (1), and total-failure (1 or 255 per the chosen option). tests/exit_code_integration_test.rs is the existing precedent for this style of test.
• If Option B is chosen, a pre-connection failure path (no hosts resolved, config load failure) is covered by a test asserting 255.
• The long_about text in src/cli/bssh.rs (lines 341 to 345) is updated so both the exit code list and the "response times" claim match the implementation.
• docs/man/bssh.1 is checked and updated. Note: the .SH COMMANDS entry for ping (line 362) currently says only "Test connectivity to hosts" and does not repeat the exit code claim, and the .SH EXIT STATUS section documents only the general MainRank / --require-all-success strategies with no ping-specific case. Whichever option is chosen, .SH EXIT STATUS should gain the ping contract so the man page and --help agree.
• docs/architecture/exit-code-strategy.md is updated if the ping contract introduces 255, since that value is not currently described there.
• If Option B is adopted, a changelog entry records the user-visible behavior change.
• Manual verification: bssh -H unreachable-host ping; echo $? returns nonzero.

Technical Considerations

• The Result<()> return type of ping_nodes cannot express "ran fine, but N hosts failed". Either change the signature to return the counts (or a computed exit code) and have the dispatcher translate it, or have ping_nodes call the exit path itself. Returning the counts and translating in one place is preferable to scattering std::process::exit calls, and matches how the dispatcher already owns command-level outcomes.
• Care is needed so that anyhow error propagation (a genuine Err from executor.execute(...)) and the "connected but some hosts failed" case do not collapse into the same exit code if Option B is chosen. A hard Err before any connection attempt should map to 255, not 1.
• Exit codes are limited to 0 to 255 (POSIX), which docs/architecture/exit-code-strategy.md already notes.

Related

Found while refreshing #75 (Add network-ping command for fast ICMP connectivity testing). That proposed network-ping command would inherit whatever exit code contract is settled here, so this issue should be resolved first, or at minimum the contract agreed on, before #75 is implemented. Otherwise the two ping-family commands will ship with divergent exit semantics.

[inureyes]

Decision: Option B (0/1/255), with the help text corrected

Signed off before implementation, per the acceptance criteria.

Exit code mapping

Scenario	Exit code
All targeted hosts 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 hosts resolved, bad arguments)	255

This aligns with ExitCodeStrategy::RequireAllSuccess for the 0/1 boundary, and adds OpenSSH's 255 convention for "ssh itself failed". MainRank is not applicable: ping runs no user command whose status could be forwarded.

Consequences to carry in the PR:

• docs/architecture/exit-code-strategy.md gains the 255 value and the ping-specific contract.
• docs/man/bssh.1 .SH EXIT STATUS gains the ping contract so the man page and --help agree.
• Changelog entry records the user-visible behavior change (all-unreachable now exits 255, not the 1 the old help text implied, and not the 0 it actually returned).
• A hard Err before any connection attempt maps to 255, kept distinct from the "connected but some hosts failed" path so the two do not collapse.

"Response times"

Correcting the text rather than implementing timing. long_about drops the "response times" claim. Per-host timing, if wanted, belongs in its own issue and should not block this fix.

[inureyes]

Correction to the decision table: bad arguments exit 2, not 255

The signed-off table above listed "bad arguments" under the 255 case. The implementation does not do that, deliberately, and the table was wrong rather than the code.

clap rejects usage errors and exits 2 before any bssh code runs, and that behavior is global to every subcommand. Routing only ping to 255 would require intercepting try_parse and inspecting raw argv for the word ping, which misfires on a host literally named ping and on ping appearing as a command argument. The cost is a fragile special case; the benefit is parity with OpenSSH on a corner that no script branches on.

clap's exit code 2 stays, and it is now documented explicitly in docs/man/bssh.1 and docs/architecture/exit-code-strategy.md rather than left implied.

The rest of the table is implemented as agreed:

Scenario	Exit code	Verified
All hosts reachable	0	PingOutcome boundary test
Some reachable, some failed	1	PingOutcome boundary test
No host succeeded	255	end to end against the real binary
Config load failure	255	end to end against the real binary
No hosts resolved	255	end to end against the real binary
Usage error (clap)	2	pre-existing, now documented

The 0 and 1 rows are asserted at the PingOutcome boundary, the exact value main passes to std::process::exit, rather than by spawning the binary, because both require a host that actually accepts an SSH connection. The three offline-deterministic rows are asserted end to end.