chore: Happy path on the left, early return

[breml]

No description provided.

[stgraber]

So we could actually flatten this further if we changed the beginning of the logic to:

err = f(ctx)
if err == nil {
    break
}

ERROR HANDLING LOGIC

That would let us get rid of that weird break at the end.

[stgraber]

I think I'd go with something like this these days:

// Retry wraps a function that interacts with the database, and retries it in
// case a transient error is hit.
//
// This should by typically used to wrap transactions.
func Retry(ctx context.Context, f func(ctx context.Context) error) error {
	var err error

	for i := 0; i < maxRetries; i++ {
		err = f(ctx)
		if err == nil {
			// The function succeeded, we're done here.
			break
		}

		if errors.Is(err, context.Canceled) {
			// The function was canceled, don't retry.
			break
		}

		if errors.Is(err, sql.ErrNoRows) || api.StatusErrorCheck(err, http.StatusNotFound) {
			// No point in re-trying or logging a no-row or not found error.
			break
		}

		// Process actual errors.
		if !IsRetriableError(err) {
			logger.Debug("Database error", logger.Ctx{"err": err})
			break
		}

		if i == maxRetries {
			logger.Warn("Database error, giving up", logger.Ctx{"attempt": i, "err": err})
			break
		}

		logger.Debug("Database error, retrying", logger.Ctx{"attempt": i, "err": err})
		time.Sleep(jitter.Deviation(nil, 0.8)(100 * time.Millisecond))

		continue
	}

	return err
}

(I removed the TODO given that we've not actually had a good reason to make this configurable in the past decade ;))

[breml]

I updated the PR as you proposed. In general, I am not a huge fan of if == nil, since I prefer to have the happy path on the left, but in this case I can see the benefit.

[breml]

With this change, even the final continue became obsolete, since at the end of the loop body this is what happens anyway.