fix: report the HTTP status when retries run out, and jitter the retry backoff

cmd/kosli/attestationSubmit_test.go

@@ -321,7 +321,7 @@ func TestAttestationSubmitterAttestsNothingWhenTheBucketRefusesTheFile(t *testin
 	err := submitter.submit(submission)
 
 	require.EqualError(t, err, "failed to upload attachment testdata/file1: "+
-		"upload refused with HTTP 403: BadDigest: The SHA256 you specified did not match what we received")
+		"upload refused with HTTP 403 Forbidden: BadDigest: The SHA256 you specified did not match what we received")
 	require.Empty(t, fake.recorded)
 }
 

cmd/kosli/evaluateHelpers.go

@@ -202,7 +202,7 @@ func fetchRemotePolicy(remoteURL string) ([]byte, error) {
 	}
 	defer func() { _ = resp.Body.Close() }()
 	if resp.StatusCode < 200 || resp.StatusCode >= 300 {
-		return nil, fmt.Errorf("failed to fetch policy from %s: HTTP %d", remoteURL, resp.StatusCode)
+		return nil, fmt.Errorf("failed to fetch policy from %s: %s", remoteURL, requests.HTTPStatus(resp.StatusCode))
 	}
 
 	body, err := io.ReadAll(io.LimitReader(resp.Body, policyMaxBytes+1))
@@ -421,9 +421,9 @@ func serverSideRequestError(err error) error {
 	// Quoting what arrived instead would dress a proxy's page, or the decoder's
 	// complaint about one, as the server's own reason.
 	if !apiError.HasServerMessage || apiError.Message == "" {
-		return fmt.Errorf("the Kosli server answered %d", apiError.StatusCode)
+		return fmt.Errorf("the Kosli server answered %s", requests.HTTPStatus(apiError.StatusCode))
 	}
-	return fmt.Errorf("the Kosli server answered %d: %s", apiError.StatusCode, apiError.Message)
+	return fmt.Errorf("the Kosli server answered %s: %s", requests.HTTPStatus(apiError.StatusCode), apiError.Message)
 }
 
 // serverSideReadError reports a failure to read a verdict back. By this point

cmd/kosli/evaluateHelpers_test.go

@@ -3,9 +3,12 @@ package main
 import (
 	"bytes"
 	"encoding/json"
+	"net/http"
+	"net/http/httptest"
 	"testing"
 
 	"github.com/kosli-dev/cli/internal/evaluate"
+	"github.com/kosli-dev/cli/internal/requests"
 	"github.com/stretchr/testify/require"
 )
 
@@ -21,3 +24,24 @@ func TestEvaluateResultKeysListsEveryKeyTheOutputCanHave(t *testing.T) {
 		require.Contains(t, evaluateResultKeys, key)
 	}
 }
+
+func TestFetchRemotePolicyNamesTheStatusItWasRefusedWith(t *testing.T) {
+	server := httptest.NewServer(http.NotFoundHandler())
+	defer server.Close()
+
+	_, err := fetchRemotePolicy(server.URL + "/policy.rego")
+
+	require.EqualError(t, err, "failed to fetch policy from "+server.URL+"/policy.rego: HTTP 404 Not Found")
+}
+
+func TestServerSideRequestErrorNamesTheStatus(t *testing.T) {
+	require.EqualError(t,
+		serverSideRequestError(&requests.APIError{StatusCode: http.StatusForbidden, Message: "not entitled", HasServerMessage: true}),
+		"the Kosli server answered HTTP 403 Forbidden: not entitled")
+	require.EqualError(t,
+		serverSideRequestError(&requests.APIError{StatusCode: http.StatusNotFound}),
+		"the Kosli server answered HTTP 404 Not Found")
+	require.EqualError(t,
+		serverSideRequestError(&requests.APIError{StatusCode: http.StatusBadRequest, HasServerMessage: true}),
+		"the Kosli server answered HTTP 400 Bad Request")
+}

internal/bitbucket/bitbucket.go

@@ -244,7 +244,7 @@ func (c *Config) getPullRequestDetailsFromBitbucket(prApiUrl, prHtmlLink, commit
 			}
 		}
 	} else {
-		return evidence, fmt.Errorf("failed to get PR details, got HTTP status %d. Please review repository permissions", response.Resp.StatusCode)
+		return evidence, fmt.Errorf("failed to get PR details, got %s instead of HTTP 200 OK", requests.HTTPStatus(response.Resp.StatusCode))
 	}
 	return evidence, nil
 }
@@ -332,7 +332,7 @@ func (c *Config) getPullRequestCommitsFromBitbucket(prID int) ([]types.Commit, e
 			return nil, err
 		}
 		if response.Resp.StatusCode != 200 {
-			return nil, fmt.Errorf("failed to get PR commits, got HTTP status %d", response.Resp.StatusCode)
+			return nil, fmt.Errorf("failed to get PR commits, got %s instead of HTTP 200 OK", requests.HTTPStatus(response.Resp.StatusCode))
 		}
 
 		var responseData map[string]any

internal/bitbucket/odd_replies_test.go

@@ -160,11 +160,21 @@ func (rt redirectTransport) RoundTrip(req *http.Request) (*http.Response, error)
 // runOddReplies serves replies and collects evidence for oddCommit through
 // the given entry point: 1 for PREvidenceForCommitV1, 2 for V2.
 func runOddReplies(t *testing.T, version int, replies map[string]string) ([]*types.PREvidence, error) {
+	t.Helper()
+	return runOddRepliesWithStatus(t, version, replies, nil)
+}
+
+// runOddRepliesWithStatus is runOddReplies with the status of each named reply
+// set by statuses; any other reply is a 200.
+func runOddRepliesWithStatus(t *testing.T, version int, replies map[string]string, statuses map[string]int) ([]*types.PREvidence, error) {
 	t.Helper()
 	var base string
 	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		for name, route := range oddRoutes {
 			if r.URL.RequestURI() == route {
+				if status, ok := statuses[name]; ok {
+					w.WriteHeader(status)
+				}
 				_, _ = w.Write([]byte(strings.ReplaceAll(replies[name], "{{base}}", base)))
 				return
 			}
@@ -423,3 +433,17 @@ func TestOddReplies_CommitWithNoHashFails(t *testing.T) {
 		})
 	}
 }
+
+// A 201 or 202 passes the client as a success but is not the reply the code
+// reads, so it fails, naming the status.
+func TestOddReplies_ASuccessOtherThanOKIsNamed(t *testing.T) {
+	for name, want := range map[string]string{
+		"pullrequest":           "failed to get PR details, got HTTP 202 Accepted instead of HTTP 200 OK",
+		"pullrequest_commits_1": "failed to get PR commits, got HTTP 202 Accepted instead of HTTP 200 OK",
+	} {
+		t.Run(name, func(t *testing.T) {
+			_, err := runOddRepliesWithStatus(t, 2, oddBaselineReplies(), map[string]int{name: http.StatusAccepted})
+			require.EqualError(t, err, want)
+		})
+	}
+}

internal/evaluations/client_test.go

@@ -221,10 +221,10 @@ func TestCreateSurfacesTheServerMessageAndStatus(t *testing.T) {
 }
 
 // A 5xx is retryable, so the shared HTTP client consumes it: it exhausts the
-// retries and reports giving up, discarding the response and the server's
-// message with it. Pinned because the enqueue failure the API documents at 503
-// therefore cannot reach a user as its own sentence, and any caller wanting to
-// name that failure has to say something of its own instead.
+// retries and reports giving up with only the status. Pinned because the
+// enqueue failure the API documents at 503 therefore cannot reach a user as its
+// own sentence, and any caller wanting to name that failure has to say
+// something of its own instead.
 func TestCreateLosesTheServerMessageOnAServerError(t *testing.T) {
 	server, _ := newFakeServer(t, http.StatusServiceUnavailable,
 		`{"message":"Evaluation '01JABCDEF' could not be queued"}`)
@@ -238,6 +238,7 @@ func TestCreateLosesTheServerMessageOnAServerError(t *testing.T) {
 	require.False(t, errors.As(err, &apiError), "a 5xx does not arrive as an API error")
 	require.NotContains(t, err.Error(), "could not be queued")
 	require.Contains(t, err.Error(), "giving up after")
+	require.Contains(t, err.Error(), "HTTP 503")
 }
 
 func TestCreateSendsNothingOnADryRun(t *testing.T) {

internal/requests/requests.go

@@ -9,6 +9,7 @@ import (
 	"fmt"
 	"io"
 	"log"
+	"math/rand/v2"
 	"mime/multipart"
 	"net/http"
 	"net/url"
@@ -17,6 +18,7 @@ import (
 	"reflect"
 	"regexp"
 	"strings"
+	"time"
 
 	retryablehttp "github.com/hashicorp/go-retryablehttp"
 	"github.com/kosli-dev/cli/internal/logger"
@@ -149,6 +151,8 @@ func NewKosliClient(httpProxyURL string, maxAPIRetries int, debug bool, logger *
 	retryClient := retryablehttp.NewClient()
 	retryClient.RetryMax = maxAPIRetries
 	retryClient.CheckRetry = customCheckRetry
+	retryClient.ErrorHandler = giveUpErrorHandler
+	retryClient.Backoff = jitteredBackoff
 	if debug {
 		retryClient.Logger = &CustomLogger{
 			Logger: log.New(os.Stderr, "[debug]", log.Lmsgprefix),
@@ -384,10 +388,7 @@ func (c *Client) Do(p *RequestParams) (*HTTPResponse, error) {
 			err := json.Unmarshal([]byte(body), &respBody)
 			if err != nil {
 				c.Logger.Debug("response body from %s (status %d):\n%s", req.URL, resp.StatusCode, string(body))
-				// Still carry the status code so callers can distinguish e.g. a 404
-				// with an empty or non-JSON body (a proxy/CDN page). Keep the
-				// (JSON parse) error text as the message.
-				return nil, &APIError{StatusCode: resp.StatusCode, Message: err.Error()}
+				return nil, &APIError{StatusCode: resp.StatusCode, Message: nonJSONErrorMessage(resp.StatusCode, body, c.Debug)}
 			}
 			cleanedErrorMessage := ""
 			hasServerMessage := false
@@ -497,9 +498,9 @@ func s3ErrorMessage(status int, body []byte) string {
 		Message string `xml:"Message"`
 	}
 	if err := xml.Unmarshal(body, &s3Error); err != nil || s3Error.Code == "" {
-		return fmt.Sprintf("upload refused with HTTP %d", status)
+		return "upload refused with " + HTTPStatus(status)
 	}
-	return fmt.Sprintf("upload refused with HTTP %d: %s: %s", status, s3Error.Code, s3Error.Message)
+	return fmt.Sprintf("upload refused with %s: %s: %s", HTTPStatus(status), s3Error.Code, s3Error.Message)
 }
 
 func (c *Client) PayloadOutput(req *http.Request, jsonFields map[string]any, message string) error {
@@ -546,6 +547,54 @@ func isSuccess(status int) bool {
 	return false
 }
 
+// nonJSONErrorMessage reports an error response whose body is not JSON, such as
+// a proxy's HTML page, by its status: the decoder's complaint about the body
+// tells the user nothing.
+func nonJSONErrorMessage(status int, body []byte, debug bool) string {
+	switch {
+	case len(bytes.TrimSpace(body)) == 0:
+		return HTTPStatus(status)
+	case debug:
+		return HTTPStatus(status) + ": the response is not JSON"
+	default:
+		return HTTPStatus(status) + ": the response is not JSON, rerun with --debug to see it"
+	}
+}
+
+// giveUpErrorHandler keeps the last response's status, which retryablehttp's
+// default handler drops. The *url.Error that http.Client wraps it in names the
+// method and URL.
+func giveUpErrorHandler(resp *http.Response, err error, numTries int) (*http.Response, error) {
+	msg := fmt.Sprintf("giving up after %d attempt(s)", numTries)
+	if resp != nil {
+		_, _ = io.Copy(io.Discard, io.LimitReader(resp.Body, 4096))
+		_ = resp.Body.Close()
+		msg += ": " + HTTPStatus(resp.StatusCode)
+	}
+	if err != nil {
+		return nil, fmt.Errorf("%s: %w", msg, err)
+	}
+	return nil, errors.New(msg)
+}
+
+// HTTPStatus renders a code as "HTTP 409 Conflict", or "HTTP 599" for a code
+// with no standard text.
+func HTTPStatus(code int) string {
+	return strings.TrimSpace(fmt.Sprintf("HTTP %d %s", code, http.StatusText(code)))
+}
+
+// jitteredBackoff stretches each wait by a random 0-50%, so clients that failed
+// together do not retry in lockstep, without shortening the time before giving up.
+// A wait that differs from the plain exponential one came from Retry-After and
+// is kept as is.
+func jitteredBackoff(minWait, maxWait time.Duration, attempt int, resp *http.Response) time.Duration {
+	exponential := retryablehttp.DefaultBackoff(minWait, maxWait, attempt, nil)
+	if wait := retryablehttp.DefaultBackoff(minWait, maxWait, attempt, resp); wait != exponential {
+		return wait
+	}
+	return min(exponential+rand.N(exponential/2+1), maxWait)
+}
+
 func customCheckRetry(ctx context.Context, resp *http.Response, err error) (bool, error) {
 	// Get the default retry policy for errors and certain status codes.
 	// It will retry on 5xx, 429 (rate limit) and we add 409 (lock conflict) via a custom check.