Skip to content

[WIP] Fix CmdRunner to respect group timeout in wait method - #37

Draft
meling with Copilot wants to merge 2 commits into
masterfrom
copilot/cmdrunner-fix-ignore-group-timeout
Draft

[WIP] Fix CmdRunner to respect group timeout in wait method#37
meling with Copilot wants to merge 2 commits into
masterfrom
copilot/cmdrunner-fix-ignore-group-timeout

Conversation

Copilot AI commented Aug 4, 2026

Copy link
Copy Markdown

Thanks for asking me to work on this. I will get started on it and keep this PR's description up to date as I form a plan and make progress.


This section details on the original issue you should resolve

<issue_title>CmdRunner Start and Wait Ignore the Group Timeout</issue_title>
<issue_description>Group.Run builds a timeout context (context.WithTimeout(..., g.Timeout)) and passes it to each task function. But the CmdRunner "start now, wait later" pattern — Start(cmd) then Wait() — ignores that context entirely: sshCmd.Wait() calls ssh.Session.Wait(), which blocks until the remote process exits, regardless of the deadline. Only RunContext honors the context (it closes the session when the context fires).

So a task that starts a command and waits separately gets no timeout, even though it was handed a deadline-bearing context. One hung remote process blocks the Group task forever. The timeout looks enforced but is not.

How it was hit

The gorums cluster sweep tool (cmd/sweep) launched benchmark nodes with cmd.Start() + cmd.Wait() under a withTimeout(g, 5m+duration) group. A benchmark node hung during teardown; because Wait() ignored the group
timeout, the whole driven sweep wedged for ~14h instead of failing the run after ~5 minutes. It has been fixed on the caller side by switching to RunContext, but the iago footgun remains for the next caller.

Root cause

Start/Wait are the only blocking CmdRunner methods that take no context yet can be reached with one in scope. The interface offers three ways to run a command, and only one of the two blocking-with-a-deadline paths honors it:

  • Run(cmd) — blocks, no context. Honest: no deadline is in scope.
  • RunContext(ctx, cmd) — blocks, honors ctx. Correct.
  • Start(cmd) + Wait() — blocks, silently ignores any ctx. The footgun.

Proposed fix: remove Start and Wait from the CmdRunner interface

RunContext(ctx, cmd) already equals Start(ctx, cmd) + Wait(). Every use of the separate start/wait pair is expressible as RunContext with the pipes wired before the call (drained by goroutines), and run inside a goroutine when concurrency across several commands is needed — which is exactly what Shell.Apply does and what the sweep's launchAndWait now does. So the separate methods buy no capability that RunContext + a goroutine does not.

Proposed interface:

type CmdRunner interface {
        Run(cmd string) error
        RunContext(ctx context.Context, cmd string) error
        StdinPipe() (io.WriteCloser, error)
        StdoutPipe() (io.ReadCloser, error)
        StderrPipe() (io.ReadCloser, error)
}

Keep start/wait as unexported helpers on sshCmd behind RunContext, or inline them. This removes the footgun by construction: the only blocking runners are Run (deadline-free by design) and RunContext (ctx-honoring).

Consider also whether Run should stay or be folded into RunContext(context.Background(), cmd); it is a thin convenience over ssh.Session.Run. Minor, orthogonal.

Compatibility

Breaking change to the CmdRunner interface either way; no compatibility promised. Known in-tree consumer (gorums cmd/sweep) already uses RunContext, so it is unaffected. Audit other relab consumers before merging.

Test

Add a Docker-gated regression test (see iago_test.go / iagotest.CreateSSHGroup): start a long-running remote command (sleep 60) under a 500ms context and assert the wait returns well before the command would finish. Before the fix this hangs; after, it returns promptly. (fix/start-honors-context includes TestStartHonorsContextTimeout in this shape.)
</issue_description>

Comments on the Issue (you are @copilot in this section)

Co-authored-by: meling <810999+meling@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CmdRunner Start and Wait Ignore the Group Timeout

2 participants