From d00f148b76b8a3e3c0a0c688ef9c56c866a72c1a Mon Sep 17 00:00:00 2001 From: Ehab Younes Date: Tue, 7 Apr 2026 21:59:10 +0000 Subject: [PATCH] fix(cli): retry transient connection failures during SSH setup (#24010) When `coder ssh` connects to a workspace after laptop wake, DNS or the control plane may be briefly unavailable. Previously this caused an immediate failure, which VS Code Remote SSH classified as permanent ("Reload Window"). Wrap each network step (workspace resolution, template version fetch, agent connection info, Coder Connect dial, tailnet dial) with `retryWithInterval` so transient errors (DNS, connection refused, 5xx) are retried individually. Non-retryable errors (auth, 404) and context cancellation stop immediately. Data transfer is never retried. --- cli/ssh.go | 108 +++++++++++++++++++++++++++----- cli/ssh_internal_test.go | 130 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 221 insertions(+), 17 deletions(-) diff --git a/cli/ssh.go b/cli/ssh.go index 29b2967269..e6c7271503 100644 --- a/cli/ssh.go +++ b/cli/ssh.go @@ -52,6 +52,10 @@ import ( const ( disableUsageApp = "disable" + + // Retry transient errors during SSH connection establishment. + sshRetryInterval = 2 * time.Second + sshMaxAttempts = 10 // initial + retries per step ) var ( @@ -62,6 +66,51 @@ var ( workspaceNameRe = regexp.MustCompile(`[/.]+|--`) ) +// isRetryableError checks for transient connection errors worth +// retrying: DNS failures, connection refused, and server 5xx. +func isRetryableError(err error) bool { + if err == nil { + return false + } + if xerrors.Is(err, context.Canceled) || xerrors.Is(err, context.DeadlineExceeded) { + return false + } + if codersdk.IsConnectionError(err) { + return true + } + var sdkErr *codersdk.Error + if xerrors.As(err, &sdkErr) { + return sdkErr.StatusCode() >= 500 + } + return false +} + +// retryWithInterval calls fn up to maxAttempts times, waiting +// interval between attempts. Stops on success, non-retryable +// error, or context cancellation. +func retryWithInterval(ctx context.Context, logger slog.Logger, interval time.Duration, maxAttempts int, fn func() error) error { + var lastErr error + attempt := 0 + for r := retry.New(interval, interval); r.Wait(ctx); { + lastErr = fn() + if lastErr == nil || !isRetryableError(lastErr) { + return lastErr + } + attempt++ + if attempt >= maxAttempts { + break + } + logger.Warn(ctx, "transient error, retrying", + slog.Error(lastErr), + slog.F("attempt", attempt), + ) + } + if lastErr != nil { + return lastErr + } + return ctx.Err() +} + func (r *RootCmd) ssh() *serpent.Command { var ( stdio bool @@ -277,10 +326,17 @@ func (r *RootCmd) ssh() *serpent.Command { HostnameSuffix: hostnameSuffix, } - workspace, workspaceAgent, err := findWorkspaceAndAgentByHostname( - ctx, inv, client, - inv.Args[0], cliConfig, disableAutostart) - if err != nil { + // Populated by the closure below. + var workspace codersdk.Workspace + var workspaceAgent codersdk.WorkspaceAgent + resolveWorkspace := func() error { + var err error + workspace, workspaceAgent, err = findWorkspaceAndAgentByHostname( + ctx, inv, client, + inv.Args[0], cliConfig, disableAutostart) + return err + } + if err := retryWithInterval(ctx, logger, sshRetryInterval, sshMaxAttempts, resolveWorkspace); err != nil { return err } @@ -306,8 +362,13 @@ func (r *RootCmd) ssh() *serpent.Command { wait = false } - templateVersion, err := client.TemplateVersion(ctx, workspace.LatestBuild.TemplateVersionID) - if err != nil { + var templateVersion codersdk.TemplateVersion + fetchVersion := func() error { + var err error + templateVersion, err = client.TemplateVersion(ctx, workspace.LatestBuild.TemplateVersionID) + return err + } + if err := retryWithInterval(ctx, logger, sshRetryInterval, sshMaxAttempts, fetchVersion); err != nil { return err } @@ -347,8 +408,12 @@ func (r *RootCmd) ssh() *serpent.Command { // If we're in stdio mode, check to see if we can use Coder Connect. // We don't support Coder Connect over non-stdio coder ssh yet. if stdio && !forceNewTunnel { - connInfo, err := wsClient.AgentConnectionInfoGeneric(ctx) - if err != nil { + var connInfo workspacesdk.AgentConnectionInfo + if err := retryWithInterval(ctx, logger, sshRetryInterval, sshMaxAttempts, func() error { + var err error + connInfo, err = wsClient.AgentConnectionInfoGeneric(ctx) + return err + }); err != nil { return xerrors.Errorf("get agent connection info: %w", err) } coderConnectHost := fmt.Sprintf("%s.%s.%s.%s", @@ -384,23 +449,27 @@ func (r *RootCmd) ssh() *serpent.Command { }) defer closeUsage() } - return runCoderConnectStdio(ctx, fmt.Sprintf("%s:22", coderConnectHost), stdioReader, stdioWriter, stack) + return runCoderConnectStdio(ctx, fmt.Sprintf("%s:22", coderConnectHost), stdioReader, stdioWriter, stack, logger) } } if r.disableDirect { _, _ = fmt.Fprintln(inv.Stderr, "Direct connections disabled.") } - conn, err := wsClient. - DialAgent(ctx, workspaceAgent.ID, &workspacesdk.DialAgentOptions{ + var conn workspacesdk.AgentConn + if err := retryWithInterval(ctx, logger, sshRetryInterval, sshMaxAttempts, func() error { + var err error + conn, err = wsClient.DialAgent(ctx, workspaceAgent.ID, &workspacesdk.DialAgentOptions{ Logger: logger, BlockEndpoints: r.disableDirect, EnableTelemetry: !r.disableNetworkTelemetry, }) - if err != nil { + return err + }); err != nil { return xerrors.Errorf("dial agent: %w", err) } if err = stack.push("agent conn", conn); err != nil { + _ = conn.Close() return err } conn.AwaitReachable(ctx) @@ -1583,11 +1652,18 @@ func testOrDefaultDialer(ctx context.Context) coderConnectDialer { return dialer } -func runCoderConnectStdio(ctx context.Context, addr string, stdin io.Reader, stdout io.Writer, stack *closerStack) error { +func runCoderConnectStdio(ctx context.Context, addr string, stdin io.Reader, stdout io.Writer, stack *closerStack, logger slog.Logger) error { dialer := testOrDefaultDialer(ctx) - conn, err := dialer.DialContext(ctx, "tcp", addr) - if err != nil { - return xerrors.Errorf("dial coder connect host: %w", err) + var conn net.Conn + if err := retryWithInterval(ctx, logger, sshRetryInterval, sshMaxAttempts, func() error { + var err error + conn, err = dialer.DialContext(ctx, "tcp", addr) + if err != nil { + return xerrors.Errorf("dial coder connect host %q over tcp: %w", addr, err) + } + return nil + }); err != nil { + return err } if err := stack.push("tcp conn", conn); err != nil { return err diff --git a/cli/ssh_internal_test.go b/cli/ssh_internal_test.go index da6e36b96a..e0fe9bca65 100644 --- a/cli/ssh_internal_test.go +++ b/cli/ssh_internal_test.go @@ -5,7 +5,9 @@ import ( "fmt" "io" "net" + "net/http" "net/url" + "os" "sync" "testing" "time" @@ -226,6 +228,21 @@ func TestCloserStack_Timeout(t *testing.T) { testutil.TryReceive(ctx, t, closed) } +func TestCloserStack_PushAfterClose_ConnClosed(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitShort) + logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}).Leveled(slog.LevelDebug) + uut := newCloserStack(ctx, logger, quartz.NewMock(t)) + + uut.close(xerrors.New("canceled")) + + closes := new([]*fakeCloser) + fc := &fakeCloser{closes: closes} + err := uut.push("conn", fc) + require.Error(t, err) + require.Equal(t, []*fakeCloser{fc}, *closes, "should close conn on failed push") +} + func TestCoderConnectStdio(t *testing.T) { t.Parallel() @@ -254,7 +271,7 @@ func TestCoderConnectStdio(t *testing.T) { stdioDone := make(chan struct{}) go func() { - err = runCoderConnectStdio(ctx, ln.Addr().String(), clientOutput, serverInput, stack) + err = runCoderConnectStdio(ctx, ln.Addr().String(), clientOutput, serverInput, stack, logger) assert.NoError(t, err) close(stdioDone) }() @@ -448,3 +465,114 @@ func Test_getWorkspaceAgent(t *testing.T) { assert.Contains(t, err.Error(), "available agents: [clark krypton zod]") }) } + +func TestIsRetryableError(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + err error + retryable bool + }{ + {"Nil", nil, false}, + {"ContextCanceled", context.Canceled, false}, + {"ContextDeadlineExceeded", context.DeadlineExceeded, false}, + {"WrappedContextCanceled", xerrors.Errorf("wrapped: %w", context.Canceled), false}, + {"DNSError", &net.DNSError{Err: "no such host", Name: "example.com", IsNotFound: true}, true}, + {"OpError", &net.OpError{Op: "dial", Net: "tcp", Err: &os.SyscallError{}}, true}, + {"WrappedDNSError", xerrors.Errorf("connect: %w", &net.DNSError{Err: "no such host", Name: "example.com"}), true}, + {"SDKError_500", codersdk.NewTestError(http.StatusInternalServerError, "GET", "/api"), true}, + {"SDKError_502", codersdk.NewTestError(http.StatusBadGateway, "GET", "/api"), true}, + {"SDKError_503", codersdk.NewTestError(http.StatusServiceUnavailable, "GET", "/api"), true}, + {"SDKError_401", codersdk.NewTestError(http.StatusUnauthorized, "GET", "/api"), false}, + {"SDKError_403", codersdk.NewTestError(http.StatusForbidden, "GET", "/api"), false}, + {"SDKError_404", codersdk.NewTestError(http.StatusNotFound, "GET", "/api"), false}, + {"GenericError", xerrors.New("something went wrong"), false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tt.retryable, isRetryableError(tt.err)) + }) + } +} + +func TestRetryWithInterval(t *testing.T) { + t.Parallel() + + const interval = time.Millisecond + const maxAttempts = 3 + + dnsErr := &net.DNSError{Err: "no such host", Name: "example.com", IsNotFound: true} + logger := slogtest.Make(t, nil).Leveled(slog.LevelDebug) + + t.Run("Succeeds_FirstTry", func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitShort) + + attempts := 0 + err := retryWithInterval(ctx, logger, interval, maxAttempts, func() error { + attempts++ + return nil + }) + require.NoError(t, err) + assert.Equal(t, 1, attempts) + }) + + t.Run("Succeeds_AfterTransientFailures", func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitShort) + + attempts := 0 + err := retryWithInterval(ctx, logger, interval, maxAttempts, func() error { + attempts++ + if attempts < 3 { + return dnsErr + } + return nil + }) + require.NoError(t, err) + assert.Equal(t, 3, attempts) + }) + + t.Run("Stops_NonRetryableError", func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitShort) + + attempts := 0 + err := retryWithInterval(ctx, logger, interval, maxAttempts, func() error { + attempts++ + return xerrors.New("permanent failure") + }) + require.ErrorContains(t, err, "permanent failure") + assert.Equal(t, 1, attempts) + }) + + t.Run("Stops_MaxAttemptsExhausted", func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitShort) + + attempts := 0 + err := retryWithInterval(ctx, logger, interval, maxAttempts, func() error { + attempts++ + return dnsErr + }) + require.Error(t, err) + assert.Equal(t, maxAttempts, attempts) + }) + + t.Run("Stops_ContextCanceled", func(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithCancel(context.Background()) + + attempts := 0 + err := retryWithInterval(ctx, logger, interval, maxAttempts, func() error { + attempts++ + cancel() + return dnsErr + }) + require.Error(t, err) + assert.Equal(t, 1, attempts) + }) +}