fix(remote): stop --exit-on-error reporting unattempted hosts as failures

apps/cinc/cmd/node.go

@@ -228,6 +228,9 @@ cinc node ssh 'web01 web02' uptime --ssh-user ubuntu --skip-search`,
 				printRemoteResults(cmd, results)
 			}
 			if failed := countRemoteFailures(results); failed > 0 {
+				if skipped := countRemoteSkipped(results); skipped > 0 {
+					return fmt.Errorf("node ssh failed on %d host(s); %d not attempted after --exit-on-error", failed, skipped)
+				}
 				return fmt.Errorf("node ssh failed on %d host(s)", failed)
 			}
 			return nil
@@ -677,16 +680,29 @@ func writePrefixedLines(out interface{ Write([]byte) (int, error) }, host, text
 	}
 }
 
+// countRemoteFailures counts hosts the command actually failed on. Hosts that
+// --exit-on-error skipped are not failures: nothing ran on them, so nothing is
+// known about them.
 func countRemoteFailures(results []remote.CommandResult) int {
 	var failed int
 	for _, result := range results {
-		if result.ExitCode != 0 {
+		if !result.Skipped && result.ExitCode != 0 {
 			failed++
 		}
 	}
 	return failed
 }
 
+func countRemoteSkipped(results []remote.CommandResult) int {
+	var skipped int
+	for _, result := range results {
+		if result.Skipped {
+			skipped++
+		}
+	}
+	return skipped
+}
+
 func validateBootstrapFlags(flags nodeBootstrapFlags, environmentChanged bool) error {
 	if (flags.policyName == "") != (flags.policyGroup == "") {
 		return fmt.Errorf("--policy-name and --policy-group must be provided together")

cli/remote/remote.go

@@ -13,6 +13,7 @@ import (
 	"strconv"
 	"strings"
 	"sync"
+	"sync/atomic"
 	"time"
 
 	sshconfig "github.com/kevinburke/ssh_config"
@@ -47,6 +48,10 @@ type CommandResult struct {
 	Stderr   string `json:"stderr,omitempty"`
 	ExitCode int    `json:"exit_code"`
 	Error    string `json:"error,omitempty"`
+	// Skipped marks a host the command was never attempted on, because
+	// --exit-on-error stopped the run first. It is not a failure: the host
+	// was not asked to do anything, so nothing is known about it.
+	Skipped bool `json:"skipped,omitempty"`
 }
 
 // Runner executes a command on a target.
@@ -141,28 +146,34 @@ func applyOpenSSHConfig(host string, opts SSHOptions) (SSHOptions, error) {
 
 // RunMany executes command across targets, limiting concurrency and preserving
 // the input order in the returned results.
+//
+// With exitOnError, the first non-zero exit stops further launches. Commands
+// already running are left to finish, which is what the flag promises, and
+// every host that never ran comes back marked Skipped rather than failed.
 func RunMany(ctx context.Context, runner Runner, targets []Target, command string, opts SSHOptions, concurrency int, exitOnError bool) []CommandResult {
 	if concurrency < 1 {
 		concurrency = 1
 	}
-	ctx, cancel := context.WithCancel(ctx)
-	defer cancel()
 	results := make([]CommandResult, len(targets))
 	jobs := make(chan int)
 	var wg sync.WaitGroup
+	// A plain flag rather than a cancelled context: cancelling would also
+	// abort sessions that had already started, which is not what
+	// "stop launching new SSH sessions" means.
+	var stopLaunching atomic.Bool
 	for i := 0; i < concurrency; i++ {
 		wg.Add(1)
 		go func() {
 			defer wg.Done()
 			for idx := range jobs {
-				if ctx.Err() != nil {
-					results[idx] = CommandResult{Host: targets[idx].Host, ExitCode: 255, Error: ctx.Err().Error()}
+				if stopLaunching.Load() || ctx.Err() != nil {
+					results[idx] = CommandResult{Host: targets[idx].Host, Skipped: true}
 					continue
 				}
 				result := runner.Run(ctx, targets[idx], command, opts)
 				results[idx] = result
 				if exitOnError && result.ExitCode != 0 {
-					cancel()
+					stopLaunching.Store(true)
 				}
 			}
 		}()

cli/remote/remote_test.go

@@ -1,10 +1,12 @@
 package remote
 
 import (
+	"context"
 	"fmt"
 	"net"
 	"os"
 	"path/filepath"
+	"sync"
 	"testing"
 	"time"
 )
@@ -108,6 +110,136 @@ func TestAgentSocketCandidatesPrefersSSHAuthSockOverGuesses(t *testing.T) {
 	}
 }
 
+// stubRunner fails the hosts named in fail and records what it was asked to run.
+type stubRunner struct {
+	mu   sync.Mutex
+	ran  []string
+	fail map[string]bool
+}
+
+func (r *stubRunner) Run(_ context.Context, target Target, _ string, _ SSHOptions) CommandResult {
+	r.mu.Lock()
+	r.ran = append(r.ran, target.Host)
+	r.mu.Unlock()
+	if r.fail[target.Host] {
+		return CommandResult{Host: target.Host, ExitCode: 1, Stderr: "boom\n"}
+	}
+	return CommandResult{Host: target.Host, Stdout: "ok\n"}
+}
+
+func testTargets(n int) []Target {
+	targets := make([]Target, n)
+	for i := range targets {
+		targets[i] = Target{Host: fmt.Sprintf("h%02d", i)}
+	}
+	return targets
+}
+
+// TestRunManyExitOnErrorMarksRemainingSkipped pins what --exit-on-error means.
+// The flag says it stops launching new sessions after a failure, so the hosts
+// that were never attempted are skipped, not failed: reporting them as
+// failures tells the operator that twenty machines broke when one command
+// returned non-zero.
+func TestRunManyExitOnErrorMarksRemainingSkipped(t *testing.T) {
+	targets := testTargets(20)
+	runner := &stubRunner{fail: map[string]bool{"h00": true}}
+
+	results := RunMany(context.Background(), runner, targets, "true", SSHOptions{}, 1, true)
+
+	if len(results) != len(targets) {
+		t.Fatalf("results = %d, want %d", len(results), len(targets))
+	}
+	if results[0].ExitCode != 1 {
+		t.Errorf("h00 should have failed, got %+v", results[0])
+	}
+	for i, r := range results[1:] {
+		if !r.Skipped {
+			t.Fatalf("result %d (%s) = %+v, want it marked skipped", i+1, r.Host, r)
+		}
+		if r.ExitCode != 0 {
+			t.Errorf("skipped host %s should not carry a failure exit code, got %d", r.Host, r.ExitCode)
+		}
+		if r.Host == "" {
+			t.Errorf("skipped result %d has no host name", i+1)
+		}
+	}
+	if len(runner.ran) != 1 {
+		t.Errorf("runner attempted %v, want only h00", runner.ran)
+	}
+}
+
+// barrierRunner blocks every command until `n` of them have started, so the
+// test can be sure all of them are genuinely in flight together.
+type barrierRunner struct {
+	mu      sync.Mutex
+	ran     []string
+	fail    map[string]bool
+	arrived chan struct{}
+	release chan struct{}
+}
+
+func (r *barrierRunner) Run(_ context.Context, target Target, _ string, _ SSHOptions) CommandResult {
+	r.mu.Lock()
+	r.ran = append(r.ran, target.Host)
+	r.mu.Unlock()
+	r.arrived <- struct{}{}
+	<-r.release
+	if r.fail[target.Host] {
+		return CommandResult{Host: target.Host, ExitCode: 1, Stderr: "boom\n"}
+	}
+	return CommandResult{Host: target.Host, Stdout: "ok\n"}
+}
+
+// TestRunManyExitOnErrorLetsInFlightWorkFinish covers the other half of the
+// flag's promise. Cancelling a shared context to stop the queue also killed
+// commands that had already started; only new launches should stop.
+func TestRunManyExitOnErrorLetsInFlightWorkFinish(t *testing.T) {
+	const n = 4
+	targets := testTargets(n)
+	runner := &barrierRunner{
+		fail:    map[string]bool{"h00": true},
+		arrived: make(chan struct{}, n),
+		release: make(chan struct{}),
+	}
+
+	done := make(chan []CommandResult, 1)
+	go func() {
+		done <- RunMany(context.Background(), runner, targets, "true", SSHOptions{}, n, true)
+	}()
+
+	// Wait until all four are inside Run, then let them all return at once.
+	for i := 0; i < n; i++ {
+		<-runner.arrived
+	}
+	close(runner.release)
+
+	for _, r := range <-done {
+		if r.Skipped {
+			t.Errorf("host %s was skipped even though it had already started", r.Host)
+		}
+	}
+	if len(runner.ran) != n {
+		t.Errorf("runner attempted %v, want all %d hosts", runner.ran, n)
+	}
+}
+
+// Without --exit-on-error every host is attempted, failures and all.
+func TestRunManyWithoutExitOnErrorRunsEveryHost(t *testing.T) {
+	targets := testTargets(5)
+	runner := &stubRunner{fail: map[string]bool{"h00": true, "h02": true}}
+
+	results := RunMany(context.Background(), runner, targets, "true", SSHOptions{}, 2, false)
+
+	if len(runner.ran) != 5 {
+		t.Errorf("runner attempted %v, want all five hosts", runner.ran)
+	}
+	for _, r := range results {
+		if r.Skipped {
+			t.Errorf("host %s was skipped without --exit-on-error", r.Host)
+		}
+	}
+}
+
 func TestApplyOpenSSHConfigUsesIdentityAgent(t *testing.T) {
 	old := sshConfigGet
 	sshConfigGet = func(host, key string) (string, error) {