fix(internal/gensupport): update shouldRetry for GCS uploads (#2634)

* fix(storage): update shouldRetry for GCS uploads

A few fixes to shouldRetry, which is used only for storage
uploads.

* Remove attempt to unwrap syscall errors and use string matching
instead, as is used in the veneer client layer for storage.
* Use errors.Is and appropriate sentinel for net.ErrClosed.
* Add broken pipe error, which is causing flakes in veneer layer
tests (see googleapis/google-cloud-go#10374
* Add unit test for shouldRetry.

Closes googleapis/google-cloud-go#9178

* don't use xerrors

internal/gensupport/retry.go

@@ -8,6 +8,7 @@ import (
 	"errors"
 	"io"
 	"net"
+	"net/url"
 	"strings"
 	"time"
 
@@ -29,8 +30,6 @@ var (
 	backoff = func() Backoff {
 		return &gax.Backoff{Initial: 100 * time.Millisecond}
 	}
-	// syscallRetryable is a platform-specific hook, specified in retryable_linux.go
-	syscallRetryable func(error) bool = func(err error) bool { return false }
 )
 
 const (
@@ -56,30 +55,33 @@ func shouldRetry(status int, err error) bool {
 	if status == statusTooManyRequests || status == statusRequestTimeout {
 		return true
 	}
-	if err == io.ErrUnexpectedEOF {
+	if errors.Is(err, io.ErrUnexpectedEOF) {
 		return true
 	}
-	// Transient network errors should be retried.
-	if syscallRetryable(err) {
+	if errors.Is(err, net.ErrClosed) {
 		return true
 	}
-	if err, ok := err.(interface{ Temporary() bool }); ok {
-		if err.Temporary() {
-			return true
+	switch e := err.(type) {
+	case *net.OpError, *url.Error:
+		// Retry socket-level errors ECONNREFUSED and ECONNRESET (from syscall).
+		// Unfortunately the error type is unexported, so we resort to string
+		// matching.
+		retriable := []string{"connection refused", "connection reset", "broken pipe"}
+		for _, s := range retriable {
+			if strings.Contains(e.Error(), s) {
+				return true
+			}
 		}
-	}
-	var opErr *net.OpError
-	if errors.As(err, &opErr) {
-		if strings.Contains(opErr.Error(), "use of closed network connection") {
-			// TODO: check against net.ErrClosed (go 1.16+) instead of string
+	case interface{ Temporary() bool }:
+		if e.Temporary() {
 			return true
 		}
 	}
 
-	// If Go 1.13 error unwrapping is available, use this to examine wrapped
+	// If error unwrapping is available, use this to examine wrapped
 	// errors.
-	if err, ok := err.(interface{ Unwrap() error }); ok {
-		return shouldRetry(status, err.Unwrap())
+	if e, ok := err.(interface{ Unwrap() error }); ok {
+		return shouldRetry(status, e.Unwrap())
 	}
 	return false
 }

internal/gensupport/retry_test.go

@@ -0,0 +1,97 @@
+// Copyright 2024 Google LLC.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package gensupport
+
+import (
+	"errors"
+	"fmt"
+	"io"
+	"net"
+	"net/url"
+	"testing"
+)
+
+func TestShouldRetry(t *testing.T) {
+	t.Parallel()
+
+	for _, test := range []struct {
+		desc        string
+		code        int
+		inputErr    error
+		shouldRetry bool
+	}{
+		{
+			desc:        "nil error",
+			inputErr:    nil,
+			shouldRetry: false,
+		},
+		{
+			desc:        "429 error",
+			inputErr:    fmt.Errorf("too many requests"),
+			code:        429,
+			shouldRetry: true,
+		},
+		{
+			desc:        "408 error",
+			inputErr:    fmt.Errorf("request timed out"),
+			code:        408,
+			shouldRetry: true,
+		},
+		{
+			desc:        "unknown error",
+			inputErr:    errors.New("foo"),
+			shouldRetry: false,
+		},
+		{
+			desc:        "503 error",
+			inputErr:    fmt.Errorf("service unavailable"),
+			code:        503,
+			shouldRetry: true,
+		},
+		{
+			desc:        "403 error",
+			inputErr:    fmt.Errorf("forbidden"),
+			code:        403,
+			shouldRetry: false,
+		},
+		{
+			desc:        "connection refused",
+			inputErr:    &url.Error{Op: "blah", URL: "blah", Err: errors.New("connection refused")},
+			shouldRetry: true,
+		},
+		{
+			desc:        "connection reset",
+			inputErr:    &net.OpError{Op: "blah", Net: "tcp", Err: errors.New("connection reset by peer")},
+			shouldRetry: true,
+		},
+		{
+			desc:        "io.ErrUnexpectedEOF",
+			inputErr:    io.ErrUnexpectedEOF,
+			shouldRetry: true,
+		},
+		{
+			desc:        "wrapped retryable error",
+			inputErr:    fmt.Errorf("Test unwrapping of a temporary error: %w", io.ErrUnexpectedEOF),
+			shouldRetry: true,
+		},
+		{
+			desc:        "wrapped non-retryable error",
+			inputErr:    fmt.Errorf("Test unwrapping of a non-retriable error: %w", io.EOF),
+			shouldRetry: false,
+		},
+		{
+			desc:        "wrapped net.ErrClosed",
+			inputErr:    &net.OpError{Err: net.ErrClosed},
+			shouldRetry: true,
+		},
+	} {
+		t.Run(test.desc, func(s *testing.T) {
+			got := shouldRetry(test.code, test.inputErr)
+			if got != test.shouldRetry {
+				s.Errorf("got %v, want %v", got, test.shouldRetry)
+			}
+		})
+	}
+}