break: retry: simplify API (remove ClassifierFn, change WorkFn)

Taskfile.yml

@@ -48,9 +48,7 @@ tasks:
     desc: Run the unit tests.
     cmds:
       # One day I will understand how to use -coverpkg=./... :-(
-      # TODO: can we remove -short ?
-      - gotestsum -- -short -coverprofile=coverage.out -count=1 ./...
-    env: *test-env
+      - gotestsum -- -coverprofile=coverage.out -count=1 -skip=Integration ./...
 
   test:all:
     desc: Run all the tests (unit + integration). Use this target to get total coverage.

github/commitstatus.go

@@ -17,10 +17,11 @@ import (
 	"github.com/Pix4D/go-kit/retry"
 )
 
-// StatusError is one of the possible errors returned by the github package.
+// StatusError is one of the possible errors returned when interacting with the
+// "Commit Statuses" API https://docs.github.com/en/rest/commits/statuses
 type StatusError struct {
 	What       string
-	StatusCode int
+	StatusCode int // The HTTP status code.
 	Details    string
 }
 
@@ -57,6 +58,21 @@ type CommitStatus struct {
 	log *slog.Logger
 }
 
+// DefaultRetry returns a [retry.Retry] with the recommended values to be passed to
+// [NewCommitStatus] for production. If you have special requirements, or for testing,
+// you can override completely or partially.
+func DefaultRetry(log *slog.Logger) retry.Retry {
+	upTo := 15 * time.Minute
+	return retry.Retry{
+		UpTo:       upTo,
+		FirstDelay: 2 * time.Second,
+		// With an exponential backoff and a FirstDelay = 2s, the sequence will be:
+		// 2s 4s 8s 16s 32s 60s ... 60s, until reaching a cumulative delay of UpTo.
+		BackoffLimit: 1 * time.Minute,
+		Log:          log,
+	}
+}
+
 // NewCommitStatus returns a CommitStatus object associated to a specific GitHub owner
 // and repo.
 // Parameter token is the personal OAuth token of a user that has write access to the
@@ -129,11 +145,11 @@ func (cs CommitStatus) Add(ctx context.Context, sha, state, targetURL, descripti
 	req.Header.Set("Content-Type", "application/json")
 
 	// The retryable unit of work.
-	workFn := func() error {
+	workFn := func() (retry.Action, error) {
 		start := time.Now()
 		resp, err := cs.target.Client.Do(req)
 		if err != nil {
-			return fmt.Errorf("http client Do: %w", err)
+			return retry.HardFail, fmt.Errorf("http client Do: %w", err)
 		}
 		defer resp.Body.Close() //nolint:errcheck
 
@@ -154,25 +170,32 @@ func (cs CommitStatus) Add(ctx context.Context, sha, state, targetURL, descripti
 		)
 
 		if resp.StatusCode == http.StatusCreated {
-			return nil
+			return retry.Success, nil
 		}
 
 		body, _ := io.ReadAll(resp.Body)
 		buffer := body
 		if strings.Contains(strings.ToLower(contentType), "application/json") {
 			var foo map[string]any
 			if err := json.Unmarshal(body, &foo); err != nil {
-				return fmt.Errorf("normalizing JSON: unmarshal: %s", err)
+				return retry.HardFail, fmt.Errorf("normalizing JSON: unmarshal: %s", err)
 			}
 			buffer, err = json.Marshal(foo)
 			if err != nil {
-				return fmt.Errorf("normalizing JSON: marshal: %s", err)
+				return retry.HardFail, fmt.Errorf("normalizing JSON: marshal: %s", err)
 			}
 		}
-		return NewGitHubError(resp, errors.New(strings.TrimSpace(string(buffer))))
+		ghErr := NewGitHubError(resp, errors.New(strings.TrimSpace(string(buffer))))
+		if TransientError(resp.StatusCode) {
+			return retry.SoftFail, ghErr
+		}
+		if RateLimited(ghErr) {
+			return retry.SoftFail, ghErr
+		}
+		return retry.HardFail, ghErr
 	}
 
-	if err := cs.target.Retry.Do(Backoff, Classifier, workFn); err != nil {
+	if err := cs.target.Retry.Do(Backoff, workFn); err != nil {
 		return cs.explainError(err, state, sha, url)
 	}
 

github/commitstatus_test.go

@@ -13,7 +13,6 @@ import (
 
 	"github.com/Pix4D/go-kit/github"
 	"github.com/Pix4D/go-kit/internal/testutils"
-	"github.com/Pix4D/go-kit/retry"
 )
 
 type mockedResponse struct {
@@ -28,21 +27,6 @@ const (
 	fullRateRemaining  = "5000" // From the GitHub API.
 )
 
-// Copied from ghcommitsink.go
-// Note that sometimes in tests we override these values for practical reasons.
-const (
-	// retryUpTo is the total maximum duration of the retries.
-	retryUpTo = 15 * time.Minute
-
-	// retryFirstDelay is duration of the first backoff.
-	retryFirstDelay = 2 * time.Second
-
-	// retryBackoffLimit is the upper bound duration of a backoff.
-	// That is, with an exponential backoff and a retryFirstDelay = 2s, the sequence will be:
-	// 2s 4s 8s 16s 32s 60s ... 60s, until reaching a cumulative delay of retryUpTo.
-	retryBackoffLimit = 1 * time.Minute
-)
-
 func TestGitHubStatusSuccessMockAPI(t *testing.T) {
 	type testCase struct {
 		name     string
@@ -61,6 +45,7 @@ func TestGitHubStatusSuccessMockAPI(t *testing.T) {
 	targetURL := "https://go-kit.example/builds/job/42"
 	now := time.Now()
 	desc := now.Format("15:04:05")
+	retryFirstDelay := github.DefaultRetry(nil).FirstDelay
 
 	test := func(t *testing.T, tc testCase) {
 		attempt := 0
@@ -82,16 +67,12 @@ func TestGitHubStatusSuccessMockAPI(t *testing.T) {
 		defer ts.Close()
 		log := testutils.MakeTestLog()
 		sleepSpy := SleepSpy{}
+		rtr := github.DefaultRetry(log)
+		rtr.SleepFn = sleepSpy.Sleep
 		target := &github.Target{
 			Client: ts.Client(),
 			Server: ts.URL,
-			Retry: retry.Retry{
-				FirstDelay:   retryFirstDelay,
-				BackoffLimit: retryBackoffLimit,
-				UpTo:         retryUpTo,
-				SleepFn:      sleepSpy.Sleep,
-				Log:          log,
-			},
+			Retry:  rtr,
 		}
 		ghStatus := github.NewCommitStatus(target, cfg.Token, cfg.Owner, cfg.Repo, context, log)
 
@@ -208,6 +189,7 @@ func TestGitHubStatusFailureMockAPI(t *testing.T) {
 	now := time.Now()
 	desc := now.Format("15:04:05")
 	upTo := 5 * time.Minute
+	retryFirstDelay := github.DefaultRetry(nil).FirstDelay
 
 	run := func(t *testing.T, tc testCase) {
 		attempt := 0
@@ -233,16 +215,13 @@ func TestGitHubStatusFailureMockAPI(t *testing.T) {
 		wantErr := fmt.Sprintf(tc.wantErr, ts.URL)
 		log := testutils.MakeTestLog()
 		sleepSpy := SleepSpy{}
+		rtr := github.DefaultRetry(log)
+		rtr.UpTo = upTo
+		rtr.SleepFn = sleepSpy.Sleep
 		target := &github.Target{
 			Client: ts.Client(),
 			Server: ts.URL,
-			Retry: retry.Retry{
-				FirstDelay:   retryFirstDelay,
-				BackoffLimit: retryBackoffLimit,
-				UpTo:         upTo,
-				SleepFn:      sleepSpy.Sleep,
-				Log:          log,
-			},
+			Retry:  rtr,
 		}
 		ghStatus := github.NewCommitStatus(target, cfg.Token, cfg.Owner, cfg.Repo, context, log)
 
@@ -345,10 +324,6 @@ OAuth: X-Accepted-Oauth-Scopes: , X-Oauth-Scopes: `,
 }
 
 func TestGitHubStatusSuccessIntegration(t *testing.T) {
-	if testing.Short() {
-		t.Skip("Skipping integration test (reason: -short)")
-	}
-
 	ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
 	defer cancel()
 
@@ -361,12 +336,7 @@ func TestGitHubStatusSuccessIntegration(t *testing.T) {
 	target := &github.Target{
 		Client: &http.Client{},
 		Server: github.ApiRoot(github.GhDefaultHostname),
-		Retry: retry.Retry{
-			FirstDelay:   retryFirstDelay,
-			BackoffLimit: retryBackoffLimit,
-			UpTo:         retryUpTo,
-			Log:          log,
-		},
+		Retry:  github.DefaultRetry(log),
 	}
 	ghStatus := github.NewCommitStatus(target, cfg.Token, cfg.Owner, cfg.Repo, context, log)
 
@@ -377,10 +347,6 @@ func TestGitHubStatusSuccessIntegration(t *testing.T) {
 }
 
 func TestGitHubStatusFailureIntegration(t *testing.T) {
-	if testing.Short() {
-		t.Skip("Skipping integration test (reason: -short)")
-	}
-
 	type testCase struct {
 		name       string
 		token      string // default: cfg.Token
@@ -416,12 +382,7 @@ func TestGitHubStatusFailureIntegration(t *testing.T) {
 		target := &github.Target{
 			Client: &http.Client{},
 			Server: github.ApiRoot(github.GhDefaultHostname),
-			Retry: retry.Retry{
-				FirstDelay:   retryFirstDelay,
-				BackoffLimit: retryBackoffLimit,
-				UpTo:         retryUpTo,
-				Log:          log,
-			},
+			Retry:  github.DefaultRetry(log),
 		}
 		ghStatus := github.NewCommitStatus(target, tc.token, tc.owner, tc.repo, "dummy-context", log)
 		err := ghStatus.Add(ctx, tc.sha, state, "dummy-url", "dummy-desc")

github/githuberror.go

@@ -8,15 +8,17 @@ import (
 )
 
 type GitHubError struct {
-	StatusCode         int
-	OauthInfo          string
+	StatusCode int
+	OauthInfo  string
+	// Date comes from the HTTP "Date" header in the response that caused this error.
+	// When the header is missing, then it is set to the current time.
 	Date               time.Time
 	RateLimitRemaining int
 	RateLimitReset     time.Time
 	innerErr           error
 }
 
-func NewGitHubError(httpResp *http.Response, innerErr error) error {
+func NewGitHubError(httpResp *http.Response, innerErr error) GitHubError {
 	ghErr := GitHubError{
 		innerErr:   innerErr,
 		StatusCode: httpResp.StatusCode,
@@ -66,7 +68,8 @@ func NewGitHubError(httpResp *http.Response, innerErr error) error {
 	date, err := time.Parse(time.RFC1123, httpResp.Header.Get("Date"))
 	// FIXME this is not robust. Maybe log instead and put a best effort date instead?
 	if err != nil {
-		return fmt.Errorf("failed to parse the date header: %s", err)
+		ghErr.innerErr = fmt.Errorf("failed to parse the Date header: %s, %w", err, innerErr)
+		return ghErr
 	}
 	ghErr.Date = date
 

github/retry.go

@@ -10,25 +10,6 @@ import (
 	"github.com/Pix4D/go-kit/retry"
 )
 
-// Classifier implements [retry.ClassifierFunc] for GitHub.
-func Classifier(err error) retry.Action {
-	if err == nil {
-		return retry.Success
-	}
-
-	if ghErr, ok := errors.AsType[GitHubError](err); ok {
-		if TransientError(ghErr.StatusCode) {
-			return retry.SoftFail
-		}
-		if RateLimited(ghErr) {
-			return retry.SoftFail
-		}
-		return retry.HardFail
-	}
-
-	return retry.HardFail
-}
-
 // Backoff implements [retry.BackoffFunc] for GitHub.
 func Backoff(first bool, previous, limit time.Duration, err error) time.Duration {
 	// Optimization: Are we rate limited?

googlechat/googlechat.go

@@ -159,14 +159,14 @@ func retrySend(rtr retry.Retry, timeout time.Duration, webHook, threadKey string
 ) (*http.Response, error) {
 	var resp *http.Response
 	client := &http.Client{}
-	workFn := func() error {
+	workFn := func() (retry.Action, error) {
 		var err error
 		ctx, cancel := context.WithTimeout(context.Background(), timeout)
 		defer cancel()
 		req, err := http.NewRequestWithContext(ctx, http.MethodPost, webHook,
 			bytes.NewBuffer(body))
 		if err != nil {
-			return fmt.Errorf("TextMessage: new request: %w", RedactErrorURL(err))
+			return retry.HardFail, fmt.Errorf("TextMessage: new request: %w", RedactErrorURL(err))
 		}
 		req.Header.Set("Content-Type", "application/json; charset=UTF-8")
 		// Encode the thread Key as a URL parameter.
@@ -176,25 +176,24 @@ func retrySend(rtr retry.Retry, timeout time.Duration, webHook, threadKey string
 			req.URL.RawQuery = values.Encode()
 		}
 		resp, err = client.Do(req)
-		return err
-	}
-	classifierFn := func(err error) retry.Action {
-		if errors.Is(err, context.DeadlineExceeded) {
-			return retry.SoftFail
-		}
 		if err != nil {
-			return retry.HardFail
+			if errors.Is(err, context.DeadlineExceeded) {
+				return retry.SoftFail, err
+			}
+			return retry.HardFail, err
 		}
 		if resp.StatusCode >= 200 && resp.StatusCode <= 299 {
-			return retry.Success
+			return retry.Success, nil
 		}
 		if slices.Contains(retryables, resp.StatusCode) {
-			return retry.SoftFail
+			return retry.SoftFail, err
 		}
-		return retry.HardFail
+		// FIXME We never read and report the payload as error to the user!!!
+		return retry.HardFail, fmt.Errorf("unretriable status code: %d %s",
+			resp.StatusCode, http.StatusText(resp.StatusCode))
 	}
 
-	err := rtr.Do(retry.ExponentialBackoff, classifierFn, workFn)
+	err := rtr.Do(retry.ExponentialBackoff, workFn)
 	return resp, err
 }
 

retry/retry.go

@@ -10,7 +10,7 @@ import (
 	"time"
 )
 
-// Action is returned by a ClassifierFunc to indicate to Retry how to proceed.
+// Action is returned by a WorkFunc to indicate to Retry how to proceed.
 type Action int
 
 const (
@@ -43,20 +43,21 @@ type Retry struct {
 // [github.com/Pix4D/go-kit/github.Backoff].
 type BackoffFunc func(first bool, previous, limit time.Duration, err error) time.Duration
 
-// ClassifierFunc decides whether to proceed or not; called by [Retry.Do].
-// Parameter err allows to inspect the error; for an example see
-// [github.com/Pix4D/go-kit/github.Classifier]
-type ClassifierFunc func(err error) Action
-
-// WorkFunc does the unit of work that might fail and need to be retried; called
-// by [Retry.Do].
-type WorkFunc func() error
+// WorkFunc does the unit of work that might fail and need to be retried; it also
+// decides wether to proceed or not via the returned [Action]. Called by [Retry.Do].
+// When the returned [Action] is [Success] or [HardFail], then the returned error,
+// if any, will also be returned by [Retry.Do].
+//
+// For an example, see
+//
+//   - [github.CommitStatus.Add]
+//   - retry_example_test.go
+type WorkFunc func() (Action, error)
 
 // Do is the loop of [Retry].
 // See the examples in file retry_example_test.go.
 func (rtr Retry) Do(
 	backoffFn BackoffFunc,
-	classifierFn ClassifierFunc,
 	workFn WorkFunc,
 ) error {
 	if rtr.FirstDelay <= 0 {
@@ -74,8 +75,8 @@ func (rtr Retry) Do(
 	totalDelay := 0 * time.Second
 
 	for attempt := 1; ; attempt++ {
-		err := workFn()
-		switch classifierFn(err) {
+		action, err := workFn()
+		switch action {
 		case Success:
 			rtr.Log.Info("success", "attempt", attempt, "totalDelay", totalDelay)
 			return err