fix(storage): wrap error when MaxAttempts is hit (#9767)

Wrap the error when retries are cut off by hitting the configured value for MaxAttempts. This makes it easier to verify that retries occurred. Also adds emulator tests verifying that wrapping occurs as expected for timeout errors and MaxAttempts. Updates #9720
googleapis · Apr 15, 2024 · 9cb262b · 9cb262b
1 parent e4eb5b4
commit 9cb262b
Show file tree

Hide file tree

Showing 3 changed files with 89 additions and 24 deletions.
diff --git a/storage/client_test.go b/storage/client_test.go
@@ -28,6 +28,7 @@ import (
 
 	"cloud.google.com/go/iam/apiv1/iampb"
 	"github.com/google/go-cmp/cmp"
+	"github.com/googleapis/gax-go/v2"
 	"github.com/googleapis/gax-go/v2/apierror"
 	"github.com/googleapis/gax-go/v2/callctx"
 	"google.golang.org/api/iterator"
@@ -1351,41 +1352,105 @@ func TestObjectConditionsEmulated(t *testing.T) {
 func TestRetryNeverEmulated(t *testing.T) {
 	transportClientTest(t, func(t *testing.T, project, bucket string, client storageClient) {
 		ctx := context.Background()
+		instructions := map[string][]string{"storage.buckets.get": {"return-503"}}
+		testID := createRetryTest(t, project, bucket, client, instructions)
+		ctx = callctx.SetHeaders(ctx, "x-retry-test-id", testID)
+		_, err := client.GetBucket(ctx, bucket, nil, withRetryConfig(&retryConfig{policy: RetryNever}))
 
-		attrs, err := client.CreateBucket(ctx, project, bucket, &BucketAttrs{}, nil)
-		if err != nil {
-			t.Fatalf("creating bucket: %v", err)
+		var ae *apierror.APIError
+		if errors.As(err, &ae) {
+			// We expect a 503/UNAVAILABLE error. For anything else including a nil
+			// error, the test should fail.
+			if ae.GRPCStatus().Code() != codes.Unavailable && ae.HTTPCode() != 503 {
+				t.Errorf("GetBucket: got unexpected error %v; want 503", err)
+			}
 		}
+	})
+}
 
-		// Need the HTTP hostname to set up a retry test, as well as knowledge of
-		// underlying transport to specify instructions.
-		host := os.Getenv("STORAGE_EMULATOR_HOST")
-		endpoint, err := url.Parse(host)
-		if err != nil {
-			t.Fatalf("parsing endpoint: %v", err)
+// Test that errors are wrapped correctly if retry happens until a timeout.
+func TestRetryTimeoutEmulated(t *testing.T) {
+	transportClientTest(t, func(t *testing.T, project, bucket string, client storageClient) {
+		ctx := context.Background()
+		instructions := map[string][]string{"storage.buckets.get": {"return-503", "return-503", "return-503", "return-503", "return-503"}}
+		testID := createRetryTest(t, project, bucket, client, instructions)
+		ctx = callctx.SetHeaders(ctx, "x-retry-test-id", testID)
+		ctx, cancel := context.WithTimeout(ctx, 100*time.Millisecond)
+		defer cancel()
+		_, err := client.GetBucket(ctx, bucket, nil, idempotent(true))
+
+		var ae *apierror.APIError
+		if errors.As(err, &ae) {
+			// We expect a 503/UNAVAILABLE error. For anything else including a nil
+			// error, the test should fail.
+			if ae.GRPCStatus().Code() != codes.Unavailable && ae.HTTPCode() != 503 {
+				t.Errorf("GetBucket: got unexpected error: %v; want 503", err)
+			}
 		}
-		var transport string
-		if _, ok := client.(*httpStorageClient); ok {
-			transport = "http"
-		} else {
-			transport = "grpc"
+		// Error should be wrapped so it's also equivalent to a context timeout.
+		if !errors.Is(err, context.DeadlineExceeded) {
+			t.Errorf("GetBucket: got unexpected error %v, want to match DeadlineExceeded.", err)
 		}
+	})
+}
 
-		et := emulatorTest{T: t, name: "testRetryNever", resources: resources{},
-			host: endpoint}
-		et.create(map[string][]string{"storage.buckets.get": {"return-503"}}, transport)
-		ctx = callctx.SetHeaders(ctx, "x-retry-test-id", et.id)
-		_, err = client.GetBucket(ctx, attrs.Name, nil, withRetryConfig(&retryConfig{policy: RetryNever}))
+// Test that errors are wrapped correctly if retry happens until max attempts.
+func TestRetryMaxAttemptsEmulated(t *testing.T) {
+	transportClientTest(t, func(t *testing.T, project, bucket string, client storageClient) {
+		ctx := context.Background()
+		instructions := map[string][]string{"storage.buckets.get": {"return-503", "return-503", "return-503", "return-503", "return-503"}}
+		testID := createRetryTest(t, project, bucket, client, instructions)
+		ctx = callctx.SetHeaders(ctx, "x-retry-test-id", testID)
+		config := &retryConfig{maxAttempts: expectedAttempts(3), backoff: &gax.Backoff{Initial: 10 * time.Millisecond}}
+		_, err := client.GetBucket(ctx, bucket, nil, idempotent(true), withRetryConfig(config))
 
 		var ae *apierror.APIError
 		if errors.As(err, &ae) {
-			// We espect a 503/UNAVAILABLE error. For anything else including a nil
+			// We expect a 503/UNAVAILABLE error. For anything else including a nil
 			// error, the test should fail.
 			if ae.GRPCStatus().Code() != codes.Unavailable && ae.HTTPCode() != 503 {
 				t.Errorf("GetBucket: got unexpected error %v; want 503", err)
 			}
 		}
+		// Error should be wrapped so it indicates that MaxAttempts has been reached.
+		if got, want := err.Error(), "retry failed after 3 attempts"; !strings.Contains(got, want) {
+			t.Errorf("got error: %q, want to contain: %q", got, want)
+		}
+	})
+}
+
+// createRetryTest creates a bucket in the emulator and sets up a test using the
+// Retry Test API for the given instructions. This is intended for emulator tests
+// of retry behavior that are not covered by conformance tests.
+func createRetryTest(t *testing.T, project, bucket string, client storageClient, instructions map[string][]string) string {
+	t.Helper()
+	ctx := context.Background()
+
+	_, err := client.CreateBucket(ctx, project, bucket, &BucketAttrs{}, nil)
+	if err != nil {
+		t.Fatalf("creating bucket: %v", err)
+	}
+
+	// Need the HTTP hostname to set up a retry test, as well as knowledge of
+	// underlying transport to specify instructions.
+	host := os.Getenv("STORAGE_EMULATOR_HOST")
+	endpoint, err := url.Parse(host)
+	if err != nil {
+		t.Fatalf("parsing endpoint: %v", err)
+	}
+	var transport string
+	if _, ok := client.(*httpStorageClient); ok {
+		transport = "http"
+	} else {
+		transport = "grpc"
+	}
+
+	et := emulatorTest{T: t, name: t.Name(), resources: resources{}, host: endpoint}
+	et.create(instructions, transport)
+	t.Cleanup(func() {
+		et.delete()
 	})
+	return et.id
 }
 
 // createObject creates an object in the emulator and returns its name, generation, and

diff --git a/storage/invoke.go b/storage/invoke.go
@@ -70,8 +70,8 @@ func run(ctx context.Context, call func(ctx context.Context) error, retry *retry
 	return internal.Retry(ctx, bo, func() (stop bool, err error) {
 		ctxWithHeaders := setInvocationHeaders(ctx, invocationID, attempts)
 		err = call(ctxWithHeaders)
-		if retry.maxAttempts != nil && attempts >= *retry.maxAttempts {
-			return true, err
+		if err != nil && retry.maxAttempts != nil && attempts >= *retry.maxAttempts {
+			return true, fmt.Errorf("storage: retry failed after %v attempts; last error: %w", *retry.maxAttempts, err)
 		}
 		attempts++
 		return !errorFunc(err), err

diff --git a/storage/invoke_test.go b/storage/invoke_test.go
@@ -251,9 +251,9 @@ func TestInvoke(t *testing.T) {
 				return test.finalErr
 			}
 			got := run(ctx, call, test.retry, test.isIdempotentValue)
-			if test.expectFinalErr && got != test.finalErr {
+			if test.expectFinalErr && !errors.Is(got, test.finalErr) {
 				s.Errorf("got %v, want %v", got, test.finalErr)
-			} else if !test.expectFinalErr && got != test.initialErr {
+			} else if !test.expectFinalErr && !errors.Is(got, test.initialErr) {
 				s.Errorf("got %v, want %v", got, test.initialErr)
 			}
 			wantAttempts := 1 + test.count