Merge branch 'pks-grpc-proxy-fix-flaky-serve' into 'master'

grpc-proxy: Fix flake caused by race between serving and stopping

Closes #4595

See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5366

Merged-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Approved-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Approved-by: Pavlo Strokov <pstrokov@gitlab.com>
Reviewed-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Co-authored-by: Patrick Steinhardt <psteinhardt@gitlab.com>

1 files changed, 33 insertions, 11 deletions

diff --git a/internal/praefect/grpc-proxy/proxy/handler_ext_test.go b/internal/praefect/grpc-proxy/proxy/handler_ext_test.go
index b9422a333..d6daa391c 100644
--- a/internal/praefect/grpc-proxy/proxy/handler_ext_test.go
+++ b/internal/praefect/grpc-proxy/proxy/handler_ext_test.go
@@ -15,6 +15,7 @@ import (
 	"net/http/httptest"
 	"net/url"
 	"path/filepath"
+	"sync"
 	"testing"
 
 	"github.com/getsentry/sentry-go"
@@ -336,24 +337,44 @@ func TestProxyErrorPropagation(t *testing.T) {
 		},
 	} {
 		t.Run(tc.desc, func(t *testing.T) {
+			ctx := testhelper.Context(t)
 			tmpDir := testhelper.TempDir(t)
 
 			backendListener, err := net.Listen("unix", filepath.Join(tmpDir, "backend"))
 			require.NoError(t, err)
 
+			// We use a wait group to synchronize shutdown of the gRPC servers. This is
+			// done so that any errors returned by the servers are propagated while the
+			// test is still execution.
+			var wg sync.WaitGroup
+			defer wg.Wait()
+
 			backendServer := grpc.NewServer(grpc.UnknownServiceHandler(func(interface{}, grpc.ServerStream) error {
 				return tc.backendError
 			}))
-			go testhelper.MustServe(t, backendServer, backendListener)
+			wg.Add(1)
+			go func() {
+				defer wg.Done()
+
+				err := backendServer.Serve(backendListener)
+				switch {
+				case errors.Is(err, grpc.ErrServerStopped):
+					// `grpc.Server.Serve()` may return `ErrServerStopped()` in
+					// case `Serve()` is called after `Stop()`. This can indeed
+					// happen in this test as the backend server will not get
+					// hit when the proxy server is already returning an error.
+				default:
+					require.NoError(t, err)
+				}
+			}()
 			defer backendServer.Stop()
-			ctx := testhelper.Context(t)
 
 			backendClientConn, err := grpc.DialContext(ctx, "unix://"+backendListener.Addr().String(),
-				grpc.WithTransportCredentials(insecure.NewCredentials()), grpc.WithDefaultCallOptions(grpc.ForceCodec(proxy.NewCodec())))
+				grpc.WithTransportCredentials(insecure.NewCredentials()),
+				grpc.WithDefaultCallOptions(grpc.ForceCodec(proxy.NewCodec())),
+			)
 			require.NoError(t, err)
-			defer func() {
-				require.NoError(t, backendClientConn.Close())
-			}()
+			defer testhelper.MustClose(t, backendClientConn)
 
 			proxyListener, err := net.Listen("unix", filepath.Join(tmpDir, "proxy"))
 			require.NoError(t, err)
@@ -373,15 +394,16 @@ func TestProxyErrorPropagation(t *testing.T) {
 					), tc.directorError
 				})),
 			)
-
-			go testhelper.MustServe(t, proxyServer, proxyListener)
+			wg.Add(1)
+			go func() {
+				defer wg.Done()
+				testhelper.MustServe(t, proxyServer, proxyListener)
+			}()
 			defer proxyServer.Stop()
 
 			proxyClientConn, err := grpc.DialContext(ctx, "unix://"+proxyListener.Addr().String(), grpc.WithTransportCredentials(insecure.NewCredentials()))
 			require.NoError(t, err)
-			defer func() {
-				require.NoError(t, proxyClientConn.Close())
-			}()
+			defer testhelper.MustClose(t, proxyClientConn)
 
 			resp, err := grpc_testing.NewTestServiceClient(proxyClientConn).UnaryCall(ctx, &grpc_testing.SimpleRequest{})
 			testhelper.RequireGrpcError(t, tc.returnedError, err)