From 2344e0a105e74236de00b8f863cdaa9227d12e29 Mon Sep 17 00:00:00 2001
From: Eric Lippmann <eric.lippmann@icinga.com>
Date: Mon, 8 Apr 2024 11:52:08 +0200
Subject: [PATCH] `retry`: Execute on error callbacks for retryable errors only

All of our error callbacks are used to log the error and indicate that
we are retrying. Previously, in the case of context errors or
non-retryable errors, we would have called these too, which would have
resulted in misleading log messages.
---
 pkg/config/redis.go    |  2 +-
 pkg/icingadb/db.go     |  2 +-
 pkg/icingadb/driver.go |  2 +-
 pkg/icingadb/ha.go     |  2 +-
 pkg/retry/retry.go     | 14 +++++++-------
 5 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/pkg/config/redis.go b/pkg/config/redis.go
index 73884aed2..201c22a9d 100644
--- a/pkg/config/redis.go
+++ b/pkg/config/redis.go
@@ -86,7 +86,7 @@ func dialWithLogging(dialer ctxDialerFunc, logger *logging.Logger) ctxDialerFunc
 			backoff.NewExponentialWithJitter(1*time.Millisecond, 1*time.Second),
 			retry.Settings{
 				Timeout: retry.DefaultTimeout,
-				OnError: func(_ time.Duration, _ uint64, err, lastErr error) {
+				OnRetryableError: func(_ time.Duration, _ uint64, err, lastErr error) {
 					if lastErr == nil || err.Error() != lastErr.Error() {
 						logger.Warnw("Can't connect to Redis. Retrying", zap.Error(err))
 					}
diff --git a/pkg/icingadb/db.go b/pkg/icingadb/db.go
index c7bc178ac..0e8731b42 100644
--- a/pkg/icingadb/db.go
+++ b/pkg/icingadb/db.go
@@ -674,7 +674,7 @@ func (db *DB) GetSemaphoreForTable(table string) *semaphore.Weighted {
 func (db *DB) getDefaultRetrySettings() retry.Settings {
 	return retry.Settings{
 		Timeout: retry.DefaultTimeout,
-		OnError: func(_ time.Duration, _ uint64, err, lastErr error) {
+		OnRetryableError: func(_ time.Duration, _ uint64, err, lastErr error) {
 			if lastErr == nil || err.Error() != lastErr.Error() {
 				db.logger.Warnw("Can't execute query. Retrying", zap.Error(err))
 			}
diff --git a/pkg/icingadb/driver.go b/pkg/icingadb/driver.go
index 2fde2f332..0f3453db0 100644
--- a/pkg/icingadb/driver.go
+++ b/pkg/icingadb/driver.go
@@ -56,7 +56,7 @@ func (c RetryConnector) Connect(ctx context.Context) (driver.Conn, error) {
 		backoff.NewExponentialWithJitter(time.Millisecond*128, time.Minute*1),
 		retry.Settings{
 			Timeout: retry.DefaultTimeout,
-			OnError: func(_ time.Duration, _ uint64, err, lastErr error) {
+			OnRetryableError: func(_ time.Duration, _ uint64, err, lastErr error) {
 				telemetry.UpdateCurrentDbConnErr(err)
 
 				if lastErr == nil || err.Error() != lastErr.Error() {
diff --git a/pkg/icingadb/ha.go b/pkg/icingadb/ha.go
index 36c600148..0d2c014eb 100644
--- a/pkg/icingadb/ha.go
+++ b/pkg/icingadb/ha.go
@@ -387,7 +387,7 @@ func (h *HA) realize(
 		backoff.NewExponentialWithJitter(time.Millisecond*256, time.Second*3),
 		retry.Settings{
 			// Intentionally no timeout is set, as we use a context with a deadline.
-			OnError: func(_ time.Duration, attempt uint64, err, lastErr error) {
+			OnRetryableError: func(_ time.Duration, attempt uint64, err, lastErr error) {
 				if lastErr == nil || err.Error() != lastErr.Error() {
 					log := h.logger.Debugw
 					if attempt > 2 {
diff --git a/pkg/retry/retry.go b/pkg/retry/retry.go
index 582f7c66b..e56d2bb0c 100644
--- a/pkg/retry/retry.go
+++ b/pkg/retry/retry.go
@@ -28,9 +28,9 @@ type Settings struct {
 	// This means that WithBackoff may not stop exactly after Timeout expires,
 	// or may not retry at all if the first execution of RetryableFunc already takes longer than Timeout.
 	Timeout time.Duration
-	// OnError is called if an error occurs.
-	OnError func(elapsed time.Duration, attempt uint64, err, lastErr error)
-	// OnSuccess is called once the operation succeeds.
+	// OnRetryableError is called if a retryable error occurs.
+	OnRetryableError func(elapsed time.Duration, attempt uint64, err, lastErr error)
+	// OnSuccess is called once the operation succeeds after a retryable error.
 	OnSuccess func(elapsed time.Duration, attempt uint64, lastErr error)
 }
 
@@ -61,10 +61,6 @@ func WithBackoff(
 			return
 		}
 
-		if settings.OnError != nil {
-			settings.OnError(time.Since(start), attempt, err, prevErr)
-		}
-
 		if errors.Is(err, context.DeadlineExceeded) || errors.Is(err, context.Canceled) {
 			if prevErr != nil {
 				err = errors.Wrap(err, prevErr.Error())
@@ -79,6 +75,10 @@ func WithBackoff(
 			return
 		}
 
+		if settings.OnRetryableError != nil {
+			settings.OnRetryableError(time.Since(start), attempt, err, prevErr)
+		}
+
 		select {
 		case <-time.After(b(attempt)):
 		case <-timeout: