From a970bc8c58acefac66b5efe649fd8665d1b4345a Mon Sep 17 00:00:00 2001 From: Kyle Felter Date: Tue, 1 Sep 2026 15:21:43 -0500 Subject: [PATCH] fix(rest-api): Retry HTTP/2 INTERNAL_ERROR streams over HTTP/1.1 Signed-off-by: Kyle Felter --- rest-api/cli/pkg/client.go | 45 +++++++++- rest-api/cli/pkg/client_test.go | 146 ++++++++++++++++++++++++++++++++ 2 files changed, 189 insertions(+), 2 deletions(-) diff --git a/rest-api/cli/pkg/client.go b/rest-api/cli/pkg/client.go index 6ee0b16876..e025ae72e2 100644 --- a/rest-api/cli/pkg/client.go +++ b/rest-api/cli/pkg/client.go @@ -33,6 +33,19 @@ type Client struct { AuthRetryNotify func(AuthRetryEvent) } +// http2StreamError matches net/http's private stream error through errors.As. +type http2StreamError struct { + StreamID uint32 + Code uint32 + Cause error +} + +const http2InternalErrorCode uint32 = 0x2 + +func (hse http2StreamError) Error() string { + return fmt.Sprintf("HTTP/2 stream %d failed with code %d", hse.StreamID, hse.Code) +} + type AuthRetryAction string const ( @@ -107,7 +120,34 @@ func (c *Client) rewriteAPIName(path string) string { // Do executes an HTTP request against the API. func (c *Client) Do(method, pathTemplate string, pathParams, queryParams map[string]string, body []byte) ([]byte, http.Header, error) { - respBody, respHeader, err := c.do(method, pathTemplate, pathParams, queryParams, body) + doClient := c + respBody, respHeader, err := doClient.do(method, pathTemplate, pathParams, queryParams, body) + streamErr, isHTTP2StreamError := errors.AsType[http2StreamError](err) + if method == http.MethodGet && isHTTP2StreamError && streamErr.Code == http2InternalErrorCode { + var transport *http.Transport + switch currentTransport := c.HTTPClient.Transport.(type) { + case nil: + transport = http.DefaultTransport.(*http.Transport).Clone() + case *http.Transport: + transport = currentTransport.Clone() + } + if transport != nil { + protocols := new(http.Protocols) + protocols.SetHTTP1(true) + transport.Protocols = protocols + transport.TLSNextProto = nil + if transport.TLSClientConfig != nil { + transport.TLSClientConfig.NextProtos = nil + } + + httpClient := *c.HTTPClient + httpClient.Transport = transport + retryClient := *c + retryClient.HTTPClient = &httpClient + doClient = &retryClient + respBody, respHeader, err = doClient.do(method, pathTemplate, pathParams, queryParams, body) + } + } if isUnauthorizedError(err) && c.TokenRefresh != nil && !canReplayAfterAuthRefresh(method) { apiErr := err.(*APIError) c.notifyAuthRetry(AuthRetryEvent{ @@ -140,6 +180,7 @@ func (c *Client) Do(method, pathTemplate string, pathParams, queryParams map[str return nil, nil, fmt.Errorf("refreshing auth token after unauthorized response: no token returned") } c.Token = token + doClient.Token = token c.notifyAuthRetry(AuthRetryEvent{ Action: AuthRetryActionRetry, Attempt: attempt, @@ -148,7 +189,7 @@ func (c *Client) Do(method, pathTemplate string, pathParams, queryParams map[str Status: apiErr.Status, Method: method, }) - respBody, respHeader, err = c.do(method, pathTemplate, pathParams, queryParams, body) + respBody, respHeader, err = doClient.do(method, pathTemplate, pathParams, queryParams, body) } return respBody, respHeader, err } diff --git a/rest-api/cli/pkg/client_test.go b/rest-api/cli/pkg/client_test.go index 79686aee11..e76c05ba96 100644 --- a/rest-api/cli/pkg/client_test.go +++ b/rest-api/cli/pkg/client_test.go @@ -9,11 +9,157 @@ import ( "net/http/httptest" "strings" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) +type roundTripFunc func(*http.Request) (*http.Response, error) + +func (rtf roundTripFunc) RoundTrip(req *http.Request) (*http.Response, error) { + return rtf(req) +} + +type errorReadCloser struct { + err error + closed bool +} + +func (erc *errorReadCloser) Read([]byte) (int, error) { + return 0, erc.err +} + +func (erc *errorReadCloser) Close() error { + erc.closed = true + return nil +} + +type observedRequest struct { + protocol int + method string + authorization string + pageSize string +} + +func newHTTP2ResetServer(t *testing.T) (*httptest.Server, *http.Transport, <-chan observedRequest) { + t.Helper() + + requests := make(chan observedRequest, 2) + server := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + requests <- observedRequest{ + protocol: req.ProtoMajor, + method: req.Method, + authorization: req.Header.Get("Authorization"), + pageSize: req.URL.Query().Get("pageSize"), + } + + if req.ProtoMajor == 2 { + w.WriteHeader(http.StatusOK) + assert.NoError(t, http.NewResponseController(w).Flush()) + panic(http.ErrAbortHandler) + } + + _, err := w.Write([]byte(`[{"id":"instance-1"}]`)) + assert.NoError(t, err) + })) + server.EnableHTTP2 = true + server.StartTLS() + t.Cleanup(server.Close) + + transport := server.Client().Transport.(*http.Transport).Clone() + transport.Protocols = new(http.Protocols) + transport.Protocols.SetHTTP1(true) + transport.Protocols.SetHTTP2(true) + + return server, transport, requests +} + +func TestClient_Do(t *testing.T) { + tests := []struct { + name string + run func(*testing.T) + }{ + { + name: "retries an HTTP/2 INTERNAL_ERROR once over HTTP/1.1", + run: func(t *testing.T) { + server, transport, requests := newHTTP2ResetServer(t) + + client := NewClient(server.URL, "test-org", "test-token", nil, false) + client.HTTPClient.Transport = transport + client.HTTPClient.Timeout = time.Second + + body, _, err := client.Do( + http.MethodGet, + "/v2/org/{org}/nico/instance", + nil, + map[string]string{"pageSize": "100"}, + nil, + ) + + require.NoError(t, err) + require.JSONEq(t, `[{"id":"instance-1"}]`, string(body)) + require.Equal(t, observedRequest{2, http.MethodGet, "Bearer test-token", "100"}, <-requests) + require.Equal(t, observedRequest{1, http.MethodGet, "Bearer test-token", "100"}, <-requests) + require.Empty(t, requests) + }, + }, + { + name: "does not retry a mutation", + run: func(t *testing.T) { + server, transport, requests := newHTTP2ResetServer(t) + + client := NewClient(server.URL, "test-org", "test-token", nil, false) + client.HTTPClient.Transport = transport + + _, _, err := client.Do( + http.MethodPost, + "/v2/org/{org}/nico/instance", + nil, + nil, + []byte(`{"name":"instance-1"}`), + ) + + require.ErrorContains(t, err, "INTERNAL_ERROR") + require.Equal(t, observedRequest{2, http.MethodPost, "Bearer test-token", ""}, <-requests) + require.Empty(t, requests) + }, + }, + { + name: "does not retry a non-HTTP/2 read error", + run: func(t *testing.T) { + body := &errorReadCloser{err: errors.New("non-HTTP/2 read failure")} + requests := 0 + client := NewClient("https://api.example.com", "test-org", "test-token", nil, false) + client.HTTPClient.Transport = roundTripFunc(func(*http.Request) (*http.Response, error) { + requests++ + return &http.Response{ + StatusCode: http.StatusOK, + Header: make(http.Header), + Body: body, + }, nil + }) + + _, _, err := client.Do( + http.MethodGet, + "/v2/org/{org}/nico/instance", + nil, + nil, + nil, + ) + + require.ErrorContains(t, err, "non-HTTP/2 read failure") + require.Equal(t, 1, requests) + require.True(t, body.closed) + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, tt.run) + } +} + func TestClientDoRefreshesTokenOnUnauthorizedAndRetries(t *testing.T) { requests := 0 server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {