Only send a serialized Status in the gRPC protocol if it has details (#…

…713)

In the gRPC protocol, Connect would always serialize a
`Status` proto and return it in the `Grpc-Status-Details-Bin`
trailer. This differs from the official gRPC Go library's behavior,
which is to only emit that header if the status has non-empty
details. The remaining properties (`message` and `code`)
are still returned via dedicated headers, so returning the
encoded `Status` message is redundant.

This PR aligns Connect's gRPC protocol with the grpc-go
behavior, which also reduces the wire size of error
responses in the common case that no details are provided.

connect_ext_test.go

@@ -822,7 +822,11 @@ func TestGRPCMarshalStatusError(t *testing.T) {
 
 	mux := http.NewServeMux()
 	mux.Handle(pingv1connect.NewPingServiceHandler(
-		pingServer{},
+		pingServer{
+			// Include error details in the response, so that the Status protobuf will be marshaled.
+			includeErrorDetails: true,
+		},
+		// We're using a codec that will fail to marshal the Status protobuf, which means the returned error will be ignored
 		connect.WithCodec(failCodec{}),
 	))
 	server := memhttptest.NewServer(t, mux)
@@ -833,11 +837,12 @@ func TestGRPCMarshalStatusError(t *testing.T) {
 		request := connect.NewRequest(&pingv1.FailRequest{Code: int32(connect.CodeResourceExhausted)})
 		_, err := client.Fail(context.Background(), request)
 		tb.Log(err)
-		assert.NotNil(t, err)
+		assert.NotNil(t, err, assert.Sprintf("expected an error"))
 		var connectErr *connect.Error
 		ok := errors.As(err, &connectErr)
-		assert.True(t, ok)
-		assert.Equal(t, connectErr.Code(), connect.CodeInternal)
+		assert.True(t, ok, assert.Sprintf("expected the error to be a connect.Error"))
+		// This should be Internal, not ResourceExhausted, because we're testing when the Status object itself fails to marshal
+		assert.Equal(t, connectErr.Code(), connect.CodeInternal, assert.Sprintf("expected the error code to be Internal, was %s", connectErr.Code()))
 		assert.True(
 			t,
 			strings.HasSuffix(connectErr.Message(), ": boom"),
@@ -2742,7 +2747,8 @@ func expectMetadata(meta http.Header, metaType, key, value string) error {
 type pingServer struct {
 	pingv1connect.UnimplementedPingServiceHandler
 
-	checkMetadata bool
+	checkMetadata       bool
+	includeErrorDetails bool
 }
 
 func (p pingServer) Ping(ctx context.Context, request *connect.Request[pingv1.PingRequest]) (*connect.Response[pingv1.PingResponse], error) {
@@ -2779,6 +2785,13 @@ func (p pingServer) Fail(ctx context.Context, request *connect.Request[pingv1.Fa
 	err := connect.NewError(connect.Code(request.Msg.GetCode()), errors.New(errorMessage))
 	err.Meta().Set(handlerHeader, headerValue)
 	err.Meta().Set(handlerTrailer, trailerValue)
+	if p.includeErrorDetails {
+		detail, derr := connect.NewErrorDetail(&pingv1.FailRequest{Code: request.Msg.GetCode()})
+		if derr != nil {
+			return nil, derr
+		}
+		err.AddDetail(detail)
+	}
 	return nil, err
 }
 

protocol_grpc.go

@@ -838,33 +838,30 @@ func grpcContentTypeFromCodecName(web bool, name string) string {
 func grpcErrorToTrailer(trailer http.Header, protobuf Codec, err error) {
 	if err == nil {
 		setHeaderCanonical(trailer, grpcHeaderStatus, "0") // zero is the gRPC OK status
-		setHeaderCanonical(trailer, grpcHeaderMessage, "")
-		return
-	}
-	status := grpcStatusFromError(err)
-	code := strconv.Itoa(int(status.GetCode()))
-	bin, binErr := protobuf.Marshal(status)
-	if binErr != nil {
-		setHeaderCanonical(
-			trailer,
-			grpcHeaderStatus,
-			strconv.FormatInt(int64(CodeInternal), 10 /* base */),
-		)
-		setHeaderCanonical(
-			trailer,
-			grpcHeaderMessage,
-			grpcPercentEncode(
-				fmt.Sprintf("marshal protobuf status: %v", binErr),
-			),
-		)
 		return
 	}
 	if connectErr, ok := asError(err); ok && !connectErr.wireErr {
 		mergeMetadataHeaders(trailer, connectErr.meta)
 	}
-	setHeaderCanonical(trailer, grpcHeaderStatus, code)
-	setHeaderCanonical(trailer, grpcHeaderMessage, grpcPercentEncode(status.GetMessage()))
-	setHeaderCanonical(trailer, grpcHeaderDetails, EncodeBinaryHeader(bin))
+	var (
+		status  = grpcStatusFromError(err)
+		code    = status.GetCode()
+		message = status.GetMessage()
+		bin     []byte
+	)
+	if len(status.Details) > 0 {
+		var binErr error
+		bin, binErr = protobuf.Marshal(status)
+		if binErr != nil {
+			code = int32(CodeInternal)
+			message = fmt.Sprintf("marshal protobuf status: %v", binErr)
+		}
+	}
+	setHeaderCanonical(trailer, grpcHeaderStatus, strconv.Itoa(int(code)))
+	setHeaderCanonical(trailer, grpcHeaderMessage, grpcPercentEncode(message))
+	if len(bin) > 0 {
+		setHeaderCanonical(trailer, grpcHeaderDetails, EncodeBinaryHeader(bin))
+	}
 }
 
 func grpcStatusFromError(err error) *statusv1.Status {