From 03508043dde51678389860fcc7db115f83ed530c Mon Sep 17 00:00:00 2001
From: Tom Clegg <tom@curii.com>
Date: Thu, 30 Nov 2023 15:00:05 -0500
Subject: [PATCH] 21217: Don't retry after "invalid outgoing header" error.

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom@curii.com>
---
 sdk/go/arvados/client.go      | 18 ++++++++++++++++--
 sdk/go/arvados/client_test.go | 14 ++++++++++++++
 2 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/sdk/go/arvados/client.go b/sdk/go/arvados/client.go
index 735a44d24c..eab61075e6 100644
--- a/sdk/go/arvados/client.go
+++ b/sdk/go/arvados/client.go
@@ -238,6 +238,8 @@ var reqIDGen = httpserver.IDGenerator{Prefix: "req-"}
 
 var nopCancelFunc context.CancelFunc = func() {}
 
+var reqErrorRe = regexp.MustCompile(`net/http: invalid header `)
+
 // Do augments (*http.Client)Do(): adds Authorization and X-Request-Id
 // headers, delays in order to comply with rate-limiting restrictions,
 // and retries failed requests when appropriate.
@@ -274,6 +276,7 @@ func (c *Client) Do(req *http.Request) (*http.Response, error) {
 	var lastResp *http.Response
 	var lastRespBody io.ReadCloser
 	var lastErr error
+	var checkRetryCalled int
 
 	rclient := retryablehttp.NewClient()
 	rclient.HTTPClient = c.httpClient()
@@ -287,11 +290,15 @@ func (c *Client) Do(req *http.Request) (*http.Response, error) {
 		rclient.RetryMax = 0
 	}
 	rclient.CheckRetry = func(ctx context.Context, resp *http.Response, respErr error) (bool, error) {
+		checkRetryCalled++
 		if c.requestLimiter.Report(resp, respErr) {
 			c.last503.Store(time.Now())
 		}
 		if c.Timeout == 0 {
-			return false, err
+			return false, nil
+		}
+		if respErr != nil && reqErrorRe.MatchString(respErr.Error()) {
+			return false, nil
 		}
 		retrying, err := retryablehttp.DefaultRetryPolicy(ctx, resp, respErr)
 		if retrying {
@@ -322,7 +329,14 @@ func (c *Client) Do(req *http.Request) (*http.Response, error) {
 	}
 	resp, err := rclient.Do(rreq)
 	if (errors.Is(err, context.DeadlineExceeded) || errors.Is(err, context.Canceled)) && (lastResp != nil || lastErr != nil) {
-		resp, err = lastResp, lastErr
+		resp = lastResp
+		err = lastErr
+		if checkRetryCalled > 0 && err != nil {
+			// Mimic retryablehttp's "giving up after X
+			// attempts" message, even if we gave up
+			// because of time rather than maxretries.
+			err = fmt.Errorf("%s %s giving up after %d attempt(s): %w", req.Method, req.URL.String(), checkRetryCalled, err)
+		}
 		if resp != nil {
 			resp.Body = lastRespBody
 		}
diff --git a/sdk/go/arvados/client_test.go b/sdk/go/arvados/client_test.go
index a196003b8f..55e2f998c4 100644
--- a/sdk/go/arvados/client_test.go
+++ b/sdk/go/arvados/client_test.go
@@ -341,6 +341,20 @@ func (s *clientRetrySuite) TestNonRetryableError(c *check.C) {
 	c.Check(s.reqs, check.HasLen, 1)
 }
 
+// as of 0.7.2., retryablehttp does not recognize this as a
+// non-retryable error.
+func (s *clientRetrySuite) TestNonRetryableStdlibError(c *check.C) {
+	s.respStatus <- http.StatusOK
+	req, err := http.NewRequest(http.MethodGet, "https://"+s.client.APIHost+"/test", nil)
+	c.Assert(err, check.IsNil)
+	req.Header.Set("Good-Header", "T\033rrible header value")
+	err = s.client.DoAndDecode(&struct{}{}, req)
+	c.Check(err, check.ErrorMatches, `.*after 1 attempt.*net/http: invalid header .*`)
+	if !c.Check(s.reqs, check.HasLen, 0) {
+		c.Logf("%v", s.reqs[0])
+	}
+}
+
 func (s *clientRetrySuite) TestNonRetryableAfter503s(c *check.C) {
 	time.AfterFunc(time.Second, func() { s.respStatus <- http.StatusNotFound })
 	err := s.client.RequestAndDecode(&struct{}{}, http.MethodGet, "test", nil, nil)
-- 
2.30.2