refactor: RetryAfter and CID cooldown

- human-readable retry-after errors
- adjusted CID cooldown/failure cache to minimize end user impact
  (1m delay is not the best, but way better than 5m which is plain user
  hostile – context: ipni/storetheindex#1344)
- easier to read and reason about durations, prepare for upstream
  support of RetryAfter erroris in go-libipfs/gateway

Author: lidel

caboose.go

@@ -83,8 +83,10 @@ const DefaultPoolRefreshInterval = 5 * time.Minute
 
 // we cool off sending requests to Saturn for a cid for a certain duration
 // if we've seen a certain number of failures for it already in a given duration.
-const DefaultMaxCidFailures = 3
-const DefaultCidCoolDownDuration = 5 * time.Minute
+// NOTE: before getting creative here, make sure you dont break end user flow
+// described in https://github.com/ipni/storetheindex/pull/1344
+const DefaultMaxCidFailures = 3 * DefaultMaxRetries // this has to fail more than DefaultMaxRetries done for a single gateway request
+const DefaultCidCoolDownDuration = 1 * time.Minute  // how long will a sane person wait and stare at blank screen with "retry later" error before hitting F5?
 
 // we cool off sending requests to a Saturn node if it returns transient errors rather than immediately downvoting it;
 // however, only upto a certain max number of cool-offs.
@@ -98,21 +100,27 @@ var ErrContentProviderNotFound error = errors.New("saturn failed to find content
 var ErrSaturnTimeout error = errors.New("saturn backend timed out")
 
 type ErrSaturnTooManyRequests struct {
-	Node         string
-	RetryAfterMs int64
+	Node       string
+	RetryAfter time.Duration // TODO: DRY refactor after https://github.com/ipfs/go-libipfs/issues/188
 }
 
 func (e ErrSaturnTooManyRequests) Error() string {
-	return fmt.Sprintf("saturn node %s returned `too many requests` error, please retry after %d milliseconds", e.Node, e.RetryAfterMs)
+	return fmt.Sprintf("saturn node %s returned Too Many Requests error, please retry after %s", e.Node, humanRetry(e.RetryAfter))
 }
 
 type ErrCidCoolDown struct {
-	Cid          cid.Cid
-	RetryAfterMs int64
+	Cid        cid.Cid
+	RetryAfter time.Duration // TODO: DRY refactor after https://github.com/ipfs/go-libipfs/issues/188
 }
 
 func (e *ErrCidCoolDown) Error() string {
-	return fmt.Sprintf("multiple saturn retrieval failures seen for CID %s, please retry after %d milliseconds", e.Cid, e.RetryAfterMs)
+	return fmt.Sprintf("multiple saturn retrieval failures seen for CID %s, please retry after %s", e.Cid, humanRetry(e.RetryAfter))
+}
+
+// TODO: move this to upstream error interface in https://github.com/ipfs/go-libipfs/issues/188
+// and refactor ErrCidCoolDown and ErrSaturnTooManyRequests to inherit from that instead
+func humanRetry(d time.Duration) string {
+	return d.Truncate(time.Second).String()
 }
 
 type Caboose struct {

failure_test.go

@@ -16,7 +16,7 @@ import (
 )
 
 var maxCabooseWeight = 20
-var retryAfterMs = 1000
+var expRetryAfter = 1 * time.Second
 
 func TestHttp429(t *testing.T) {
 	ctx := context.Background()
@@ -31,7 +31,7 @@ func TestHttp429(t *testing.T) {
 	require.Error(t, err)
 
 	ferr := err.(*caboose.ErrSaturnTooManyRequests)
-	require.EqualValues(t, retryAfterMs, ferr.RetryAfterMs)
+	require.EqualValues(t, expRetryAfter, ferr.RetryAfter)
 }
 
 func TestCabooseTransientFailures(t *testing.T) {
@@ -165,7 +165,7 @@ func (ch *CabooseHarness) fetchAndAssertCoolDownError(t *testing.T, ctx context.
 	coolDownErr, ok := err.(*caboose.ErrCidCoolDown)
 	require.True(t, ok)
 	require.EqualValues(t, cid, coolDownErr.Cid)
-	require.NotZero(t, coolDownErr.RetryAfterMs)
+	require.NotZero(t, coolDownErr.RetryAfter)
 }
 
 func (ch *CabooseHarness) fetchAndAssertFailure(t *testing.T, ctx context.Context, testCid cid.Cid, contains string) {

pool.go

@@ -285,8 +285,8 @@ func (p *pool) fetchWith(ctx context.Context, c cid.Cid, with string) (blk block
 
 		expireAt := at.(time.Time)
 		return nil, &ErrCidCoolDown{
-			Cid:          c,
-			RetryAfterMs: time.Until(expireAt).Milliseconds(),
+			Cid:        c,
+			RetryAfter: time.Until(expireAt),
 		}
 	}
 	p.cidLk.RUnlock()
@@ -438,8 +438,8 @@ func (p *pool) fetchAndUpdate(ctx context.Context, node string, c cid.Cid, attem
 	var errTooManyRequests ErrSaturnTooManyRequests
 	if errors.As(err, &errTooManyRequests) {
 		err = &ErrSaturnTooManyRequests{
-			Node:         errTooManyRequests.Node,
-			RetryAfterMs: errTooManyRequests.RetryAfterMs,
+			Node:       errTooManyRequests.Node,
+			RetryAfter: errTooManyRequests.RetryAfter,
 		}
 		if ok := p.isCoolOffLocked(node); ok {
 			return
@@ -642,20 +642,19 @@ func (p *pool) doFetch(ctx context.Context, from string, c cid.Cid, attempt int)
 
 	if resp.StatusCode != http.StatusOK {
 		if resp.StatusCode == http.StatusTooManyRequests {
-			rs := int64(0)
-			retryAfter := resp.Header.Get(saturnRetryAfterKey)
-			if len(retryAfter) != 0 {
-				seconds, err := strconv.ParseInt(retryAfter, 10, 64)
+			var retryAfter time.Duration
+			if strnRetryHint := resp.Header.Get(saturnRetryAfterKey); strnRetryHint != "" {
+				seconds, err := strconv.ParseInt(strnRetryHint, 10, 64)
 				if err == nil {
-					rs = seconds * 1000
+					retryAfter = time.Duration(seconds) * time.Second
 				}
 			}
 
-			if rs == 0 {
-				rs = p.config.SaturnNodeCoolOff.Milliseconds()
+			if retryAfter == 0 {
+				retryAfter = p.config.SaturnNodeCoolOff
 			}
 
-			return nil, fmt.Errorf("http error from strn: %d, err=%w", resp.StatusCode, ErrSaturnTooManyRequests{RetryAfterMs: rs, Node: from})
+			return nil, fmt.Errorf("http error from strn: %d, err=%w", resp.StatusCode, ErrSaturnTooManyRequests{RetryAfter: retryAfter, Node: from})
 		}
 
 		if resp.StatusCode == http.StatusGatewayTimeout {