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) + }) +}