proxyd: Handle unexpected JSON-RPC responses (ethereum-optimism#2628)

This fixes a bug where the infura backend would be labeled offline because it
returns an unexpected JSON-RPC response. Unexpected, but well-formed,
JSON-RPC response are handled specially. Such errors are surfaced up to
the backend proxier so failover still occurs.

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

.changeset/early-cougars-eat.md

@@ -0,0 +1,5 @@
+---
+'@eth-optimism/proxyd': patch
+---
+
+Improve robustness against unexpected JSON-RPC from upstream

proxyd/backend.go

@@ -73,6 +73,8 @@ var (
 		Message:       "gateway timeout",
 		HTTPErrorCode: 504,
 	}
+
+	ErrBackendUnexpectedJSONRPC = errors.New("backend returned an unexpected JSON-RPC response")
 )
 
 func ErrInvalidRequest(msg string) *RPCErr {
@@ -228,7 +230,20 @@ func (b *Backend) Forward(ctx context.Context, reqs []*RPCReq, isBatch bool) ([]
 		)
 
 		res, err := b.doForward(ctx, reqs, isBatch)
-		if err != nil {
+		switch err {
+		case nil: // do nothing
+		// ErrBackendUnexpectedJSONRPC occurs because infura responds with a single JSON-RPC object
+		// to a batch request whenever any Request Object in the batch would induce a partial error.
+		// We don't label the the backend offline in this case. But the error is still returned to
+		// callers so failover can occur if needed.
+		case ErrBackendUnexpectedJSONRPC:
+			log.Debug(
+				"Reecived unexpected JSON-RPC response",
+				"name", b.Name,
+				"req_id", GetReqID(ctx),
+				"err", err,
+			)
+		default:
 			lastError = err
 			log.Warn(
 				"backend request failed, trying again",
@@ -244,7 +259,7 @@ func (b *Backend) Forward(ctx context.Context, reqs []*RPCReq, isBatch bool) ([]
 		timer.ObserveDuration()
 
 		MaybeRecordErrorsInRPCRes(ctx, b.Name, reqs, res)
-		return res, nil
+		return res, err
 	}
 
 	b.setOffline()
@@ -387,12 +402,15 @@ func (b *Backend) doForward(ctx context.Context, rpcReqs []*RPCReq, isBatch bool
 
 	var res []*RPCRes
 	if err := json.Unmarshal(resB, &res); err != nil {
+		// Infura may return a single JSON-RPC response if, for example, the batch contains a request for an unsupported method
+		if responseIsNotBatched(resB) {
+			return nil, ErrBackendUnexpectedJSONRPC
+		}
 		return nil, ErrBackendBadResponse
 	}
 
-	// Alas! Certain node providers (Infura) always return a single JSON object for some types of errors
 	if len(rpcReqs) != len(res) {
-		return nil, ErrBackendBadResponse
+		return nil, ErrBackendUnexpectedJSONRPC
 	}
 
 	// capture the HTTP status code in the response. this will only
@@ -407,6 +425,11 @@ func (b *Backend) doForward(ctx context.Context, rpcReqs []*RPCReq, isBatch bool
 	return res, nil
 }
 
+func responseIsNotBatched(b []byte) bool {
+	var r RPCRes
+	return json.Unmarshal(b, &r) == nil
+}
+
 // sortBatchRPCResponse sorts the RPCRes slice according to the position of its corresponding ID in the RPCReq slice
 func sortBatchRPCResponse(req []*RPCReq, res []*RPCRes) {
 	pos := make(map[string]int, len(req))

proxyd/integration_tests/failover_test.go

@@ -8,13 +8,15 @@ import (
 	"testing"
 	"time"
 
+	"github.com/alicebob/miniredis"
 	"github.com/ethereum-optimism/optimism/proxyd"
 	"github.com/stretchr/testify/require"
 )
 
 const (
 	goodResponse       = `{"jsonrpc": "2.0", "result": "hello", "id": 999}`
 	noBackendsResponse = `{"error":{"code":-32011,"message":"no backends available for method"},"id":999,"jsonrpc":"2.0"}`
+	unexpectedResponse = `{"error":{"code":-32011,"message":"some error"},"id":999,"jsonrpc":"2.0"}`
 )
 
 func TestFailover(t *testing.T) {
@@ -240,3 +242,52 @@ func TestBatchWithPartialFailover(t *testing.T) {
 	require.Equal(t, 2, len(badBackend.Requests()))
 	require.Equal(t, 2, len(goodBackend.Requests()))
 }
+
+func TestInfuraFailoverOnUnexpectedResponse(t *testing.T) {
+	InitLogger()
+	// Scenario:
+	// 1. Send batch to BAD_BACKEND (Infura)
+	// 2. Infura fails completely due to a partially errorneous batch request (one of N+1 request object is invalid)
+	// 3. Assert that the request batch is re-routed to the failover provider
+	// 4. Assert that BAD_BACKEND is NOT labeled offline
+	// 5. Assert that BAD_BACKEND is NOT retried
+
+	redis, err := miniredis.Run()
+	require.NoError(t, err)
+	defer redis.Close()
+
+	config := ReadConfig("failover")
+	config.Server.MaxUpstreamBatchSize = 2
+	config.BackendOptions.MaxRetries = 2
+	// Setup redis to detect offline backends
+	config.Redis.URL = fmt.Sprintf("redis://127.0.0.1:%s", redis.Port())
+
+	goodBackend := NewMockBackend(BatchedResponseHandler(200, goodResponse, goodResponse))
+	defer goodBackend.Close()
+	badBackend := NewMockBackend(SingleResponseHandler(200, unexpectedResponse))
+	defer badBackend.Close()
+
+	require.NoError(t, os.Setenv("GOOD_BACKEND_RPC_URL", goodBackend.URL()))
+	require.NoError(t, os.Setenv("BAD_BACKEND_RPC_URL", badBackend.URL()))
+
+	client := NewProxydClient("http://127.0.0.1:8545")
+	shutdown, err := proxyd.Start(config)
+	require.NoError(t, err)
+	defer shutdown()
+
+	res, statusCode, err := client.SendBatchRPC(
+		NewRPCReq("1", "eth_chainId", nil),
+		NewRPCReq("2", "eth_chainId", nil),
+	)
+	require.NoError(t, err)
+	require.Equal(t, 200, statusCode)
+	RequireEqualJSON(t, []byte(asArray(goodResponse, goodResponse)), res)
+	require.Equal(t, 1, len(badBackend.Requests()))
+	require.Equal(t, 1, len(goodBackend.Requests()))
+
+	rr, err := proxyd.NewRedisRateLimiter(config.Redis.URL)
+	require.NoError(t, err)
+	online, err := rr.IsBackendOnline("bad")
+	require.NoError(t, err)
+	require.Equal(t, true, online)
+}