fix(remote): stop --exit-on-error reporting unattempted hosts as failures - #186
Open
tas50 wants to merge 1 commit into
Open
fix(remote): stop --exit-on-error reporting unattempted hosts as failures#186tas50 wants to merge 1 commit into
tas50 wants to merge 1 commit into
Conversation
tas50
force-pushed
the
fix/exit-on-error-failure-count
branch
3 times, most recently
from
September 8, 2026 17:38
52f7320 to
20e79be
Compare
…ures RunMany cancelled a shared context to stop the queue after the first failure. Every host still queued then took the cancelled-context branch and came back with ExitCode 255, so countRemoteFailures counted it. One failing command across twenty hosts reported: node ssh failed on 20 host(s) when nineteen of them were never contacted. Cancelling also contradicted the flag's own help text. "--exit-on-error: stop launching new SSH sessions after the first failure" promises to stop launching, but a cancelled context also aborts sessions already running. Use a plain atomic flag to stop launching, leaving in-flight commands to finish, and mark hosts that never ran as Skipped rather than failed. The error now distinguishes the two: node ssh failed on 1 host(s); 19 not attempted after --exit-on-error Signed-off-by: Tim Smith <tim@mondoo.com>
tas50
force-pushed
the
fix/exit-on-error-failure-count
branch
from
September 8, 2026 17:42
20e79be to
74b1578
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The defect
RunManycancelled a shared context to stop the queue after the first failure. Every host still queued then took the cancelled-context branch and came back withExitCode: 255, whichcountRemoteFailuresduly counted:So one failing command across twenty hosts told the operator
node ssh failed on 20 host(s), when nineteen were never contacted. That is the opposite of useful during an incident: it turns "one box is unhappy" into "the fleet is down".Cancelling also contradicted the flag's own help text:
It promises to stop launching, but a cancelled context also aborts sessions already in flight, and kills hosts mid-dial with a confusing
dial ssh: context canceled.The fix
atomic.Boolinstead of cancelling the context, so commands already running are left to finish. That is what the flag says it does.Skippedfield rather than a fabricated failure exit code. Nothing ran on them, so nothing is known about them.countRemoteFailuresignores skipped hosts, and the error message distinguishes the two:The caller's context is still honoured, so Ctrl-C still stops the run; those hosts are reported as skipped too, which is accurate.
Tests
TestRunManyExitOnErrorMarksRemainingSkipped— 20 hosts, one failure; asserts exactly one failure, the rest skipped with no bogus exit code, and that the runner was invoked once.TestRunManyExitOnErrorLetsInFlightWorkFinish— uses a barrier runner that blocks until all four commands are genuinely in flight, then releases them, proving none is aborted. (My first attempt at this test was flaky: the stub returned instantly, so the failure was observed before the other jobs were dispatched. The barrier makes it deterministic.)TestRunManyWithoutExitOnErrorRunsEveryHost— the flag is off by default and must change nothing.Verified with
-race -count=10.go test ./... -race,go vet ./..., andgofmt -l .are clean.node sshalready has acceptance coverage (TestNodeSSHSkipSearchAgainstSSHServer) and this adds no command, so the coverage manifest is unchanged.