Revert changes, improve code comments

bsr-383

CHANGELOG.md

@@ -5,9 +5,6 @@
 - Change default for `--origin` flag of `buf beta studio-agent` to `https://studio.buf.build`
 - Change default for `--timeout` flag of `buf beta studio-agent` to `0` (no timeout). Before it was
   `2m` (the default for all the other `buf` commands).
-- Add `connect.CodeUnavailable` as an unhandled Connect error code for `buf beta studio-agent`,
-  which will trigger an `http.StatusBadGateway` response in case of a bad or unreachable upstream
-  server. Before the only unhandled Connect error code was `connect.CodeUnknown`.
 
 ## [v1.7.0] - 2022-06-27
 

private/bufpkg/bufstudioagent/bufstudioagent_test.go

@@ -21,6 +21,7 @@ import (
 	"encoding/base64"
 	"errors"
 	"io"
+	"net"
 	"net/http"
 	"net/http/httptest"
 	"strconv"
@@ -37,9 +38,8 @@ import (
 )
 
 const (
-	echoPath    = "/echo.Service/EchoEcho"
-	errorPath   = "/error.Service/Error"
-	unknownPath = "/unknown.Service/Unknown"
+	echoPath  = "/echo.Service/EchoEcho"
+	errorPath = "/error.Service/Error"
 )
 
 func TestPlainPostHandlerTLS(t *testing.T) {
@@ -185,9 +185,31 @@ func testPlainPostHandlerErrors(t *testing.T, upstreamServer *httptest.Server) {
 		assert.Equal(t, "something", upstreamResponseHeaders.Get("grpc-message"))
 	})
 
-	t.Run("unknown_response_bad_gateway", func(t *testing.T) {
+	t.Run("invalid_upstream", func(t *testing.T) {
+		// TODO: unskip this test. This is flaky because of two reasons:
+		//
+		// 1. When a connection is closed, the underlying HTTP client does not
+		// always knows it, since the http handler implementation in go has no way
+		// of changing the connection timeout. See:
+		// https://github.com/golang/go/issues/16100
+		//
+		// 2. The expected status code is `StatusBadGateway` since the issue
+		// happened client-side (a response never came back from the server). This
+		// is not deterministic in the business logic because we're based on the
+		// connect error code that's returned. See
+		// https://linear.app/bufbuild/issue/BSR-383/flaky-test-in-bufstudioagent-testgo
+		t.SkipNow()
+		listener, err := net.Listen("tcp", "127.0.0.1:")
+		require.NoError(t, err)
+		go func() {
+			conn, err := listener.Accept()
+			require.NoError(t, err)
+			require.NoError(t, conn.Close())
+		}()
+		defer listener.Close()
+
 		requestProto := &studiov1alpha1.InvokeRequest{
-			Target: upstreamServer.URL + unknownPath,
+			Target: "http://" + listener.Addr().String(),
 			Headers: goHeadersToProtoHeaders(http.Header{
 				"Content-Type": []string{"application/grpc"},
 			}),
@@ -227,15 +249,6 @@ func newTestConnectServer(t *testing.T, tls bool) *httptest.Server {
 		},
 		connect.WithCodec(&bufferCodec{name: "proto"}),
 	))
-	// unknownPath returns the body as error message with code unknown, to test
-	// studio agent behavior on potential server closed connections.
-	mux.Handle(unknownPath, connect.NewUnaryHandler(
-		unknownPath,
-		func(ctx context.Context, r *connect.Request[bytes.Buffer]) (*connect.Response[bytes.Buffer], error) {
-			return nil, connect.NewError(connect.CodeUnknown, errors.New(r.Msg.String()))
-		},
-		connect.WithCodec(&bufferCodec{name: "proto"}),
-	))
 	if tls {
 		upstreamServerTLS := httptest.NewUnstartedServer(mux)
 		upstreamServerTLS.EnableHTTP2 = true

private/bufpkg/bufstudioagent/plain_post_handler.go

@@ -169,20 +169,47 @@ func (i *plainPostHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	// TODO(rvanginkel) should this context be cloned to remove attached values (but keep timeout)?
 	response, err := client.CallUnary(r.Context(), request)
 	if err != nil {
+		// TODO deal with this error handling using `connect.IsFromServer(err)`
+		// instead of these heuristics. See
+		// https://github.com/bufbuild/connect-go/issues/222
+		//
+		// We need to differentiate client errors from server errors. In the former,
+		// trigger a `StatusBadGateway` result, and in the latter surface whatever
+		// error information came back from the server.
+		//
+		// Any error here is wrapped in a `connect.Error` struct. We need to check
+		// *first* if within it also wraps a low level network issue, so we can
+		// assume the request never left the client, or a response never arrived
+		// from the server. In those scenarios we trigger a `StatusBadGateway` to
+		// signal that the upstream server is unreachable or in a bad status...
+		if netErr := new(net.OpError); errors.As(err, &netErr) {
+			http.Error(w, err.Error(), http.StatusBadGateway)
+			return
+		}
+		if urlErr := new(url.Error); errors.As(err, &urlErr) {
+			http.Error(w, err.Error(), http.StatusBadGateway)
+			return
+		}
+		// ... but if a response was received from the server, we assume there's
+		// error information from the server we can surface to the user by including
+		// it in the headers response, unless it is a `CodeUnknown` error. Connect
+		// marks any issues connecting with the `CodeUnknown` error.
 		if connectErr := new(connect.Error); errors.As(err, &connectErr) {
-			// Any Connect error except these codes can be wrapped in the headers.
-			if connectErr.Code() != connect.CodeUnknown &&
-				connectErr.Code() != connect.CodeUnavailable {
-				i.writeProtoMessage(w, &studiov1alpha1.InvokeResponse{
-					// connectErr.Meta contains the trailers for the
-					// caller to find out the error details.
-					Headers: goHeadersToProtoHeaders(connectErr.Meta()),
-				})
+			if connectErr.Code() == connect.CodeUnknown {
+				http.Error(w, err.Error(), http.StatusBadGateway)
 				return
 			}
+			i.writeProtoMessage(w, &studiov1alpha1.InvokeResponse{
+				// connectErr.Meta contains the trailers for the
+				// caller to find out the error details.
+				Headers: goHeadersToProtoHeaders(connectErr.Meta()),
+			})
+			return
 		}
-		// Any issue connecting that is not a Connect error is assumed to be because
-		// the server is in some kind of bad and/or unreachable state.
+		i.Logger.Warn(
+			"non_connect_unary_error",
+			zap.Error(err),
+		)
 		http.Error(w, err.Error(), http.StatusBadGateway)
 		return
 	}