Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion e2e/framework/command.go
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,9 @@ func (f *Framework) DevsySSH(
workspace string,
command string,
) (string, error) {
out, err := f.ExecCommandOutput(ctx, []string{"ssh", workspace, "--command", command})
out, err := execWithSSHRetry(ctx, workspace, func(ctx context.Context) (string, error) {
return f.ExecCommandOutput(ctx, []string{"ssh", workspace, "--command", command})
})
if err != nil {
return "", fmt.Errorf("devsy ssh failed: %s", err.Error())
}
Expand Down
60 changes: 60 additions & 0 deletions e2e/framework/retry.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"errors"
"fmt"
"os/exec"
"strings"
"time"

Expand All @@ -20,6 +21,16 @@ var dockerPullBackoff = wait.Backoff{
Jitter: 0.1,
}

// sshBackoff defines retry timing for transient SSH failures on Windows+WSL
// runners where the devsy agent binary injection can intermittently fail.
// 3 total attempts (1 initial + 2 retries) with waits of ~5s, ~10s.
var sshBackoff = wait.Backoff{
Steps: 3,
Duration: 5 * time.Second,
Factor: 2.0,
Jitter: 0.1,
}

// retryableDockerPatterns are stderr substrings indicating a transient Docker
// registry error that is worth retrying.
var retryableDockerPatterns = []string{
Expand All @@ -31,6 +42,17 @@ var retryableDockerPatterns = []string{
"503 Service Unavailable",
}

// isRetryableSSHError returns true when the error indicates a transient SSH
// failure (exit code 1) typically caused by the devsy agent not yet being
// ready inside the container.
func isRetryableSSHError(err error) bool {
var exitErr *exec.ExitError
if errors.As(err, &exitErr) {
return exitErr.ExitCode() == 1
}
return false
}
Comment on lines +45 to +54

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Retry predicate is too broad — may retry legitimate remote command failures.

isRetryableSSHError treats any error whose string contains "exit status 1" as transient. However, DevsySSH runs devsy ssh <workspace> --command <command>, and when the remote command itself exits non-zero the devsy CLI propagates that exit code. Shell tools like grep (no match), test/[ ] (false), diff (differences found), and any failing assertion in a user-provided command legitimately exit with status 1. With this predicate, every such failure will now be retried 3 times (≈15s of extra latency) before surfacing, and — worse — if the remote command is non-idempotent or has side effects, it will be re-executed.

Because ExecCommandOutput routes stderr to os.Stderr (see e2e/framework/exec.go:14-27) rather than capturing it, there is no stderr text to key the predicate on. Two options to tighten the scope:

  1. Capture stderr for the SSH path (e.g., use ExecCommandCapture inside DevsySSH) and only retry when stderr contains a known "agent not ready" / connection-setup signature (analogous to retryableDockerPatterns).
  2. Keep the exit-code heuristic but scope it to the injection/bootstrap phase only (e.g., a dedicated helper that the caller opts into) instead of applying it to every DevsySSH call.

Option 1 is safer and mirrors the Docker retry design already in this file.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@e2e/framework/retry.go` around lines 44 - 49, isRetryableSSHError is too
broad and treats any "exit status 1" as transient; update DevsySSH to capture
stderr (use ExecCommandCapture instead of ExecCommandOutput) and change
isRetryableSSHError to only return true when stderr contains a specific
agent/connection-setup signature (similar to retryableDockerPatterns), or
alternatively add a separate helper (e.g., isRetryableSSHAgentError) that
callers can opt into during bootstrap/injection; locate references to DevsySSH,
ExecCommandOutput/ExecCommandCapture, and isRetryableSSHError to implement the
stderr-capturing path and tighten the retry predicate.


// isRetryableDockerError returns true if stderr contains a transient Docker
// registry error (rate limits, timeouts, connection resets).
func isRetryableDockerError(stderr string) bool {
Expand Down Expand Up @@ -78,3 +100,41 @@ func execWithDockerRetry(
}
return lastStdout, lastStderr, err
}

// execWithSSHRetry runs fn and retries if the error indicates a transient SSH
// failure (exit status 1). This handles the case where the devsy agent binary
// injection into a WSL container has not completed yet.
func execWithSSHRetry(
ctx context.Context,
workspace string,
fn func(ctx context.Context) (string, error),
) (string, error) {
var lastOut string
var lastErr error
attempt := 0

err := wait.ExponentialBackoffWithContext(ctx, sshBackoff,
func(ctx context.Context) (bool, error) {
attempt++
lastOut, lastErr = fn(ctx)
if lastErr == nil {
return true, nil // success
}
if isRetryableSSHError(lastErr) {
ginkgo.GinkgoWriter.Printf(
"[retry] ssh %s: attempt %d failed with transient error, retrying: %s\n",
workspace, attempt, lastErr,
)
return false, nil // retry
}
return false, lastErr // non-retryable, stop immediately
},
)
if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) {
return lastOut, err
}
if err != nil && lastErr != nil {
return lastOut, fmt.Errorf("after %d attempts: %w", attempt, lastErr)
}
return lastOut, err
}
35 changes: 35 additions & 0 deletions e2e/framework/retry_test.go
Original file line number Diff line number Diff line change
@@ -1,11 +1,46 @@
package framework

import (
"fmt"
"os/exec"
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

// exitError runs a shell command that exits with the given code and returns
// the resulting *exec.ExitError.
func exitError(t *testing.T, code string) *exec.ExitError {
t.Helper()
// #nosec G204 -- test helper with controlled exit code argument
err := exec.Command("sh", "-c", "exit "+code).Run()
require.Error(t, err)
var exitErr *exec.ExitError
require.ErrorAs(t, err, &exitErr)
return exitErr
}

func TestIsRetryableSSHError_ExitStatus1(t *testing.T) {
assert.True(t, isRetryableSSHError(exitError(t, "1")))
}

func TestIsRetryableSSHError_ExitStatus10(t *testing.T) {
assert.False(t, isRetryableSSHError(exitError(t, "10")))
}

func TestIsRetryableSSHError_ExitStatus127(t *testing.T) {
assert.False(t, isRetryableSSHError(exitError(t, "127")))
}

func TestIsRetryableSSHError_Nil(t *testing.T) {
assert.False(t, isRetryableSSHError(nil))
}

func TestIsRetryableSSHError_NonExitError(t *testing.T) {
assert.False(t, isRetryableSSHError(fmt.Errorf("some other error")))
}

func TestIsRetryableDockerError_RateLimit(t *testing.T) {
stderr := `GET https://index.docker.io/v2/library/ubuntu/manifests/latest: ` +
`TOOMANYREQUESTS: You have reached your unauthenticated pull rate limit.`
Expand Down
Loading