From 69a7d5d6a61df9a90740a9632febc068f4fb852c Mon Sep 17 00:00:00 2001
From: "Grot (@grafanabot)" <43478413+grafanabot@users.noreply.github.com>
Date: Fri, 17 Mar 2023 12:33:37 +0100
Subject: [PATCH] [v9.4.x] SQLStore: Fix SQLite error propagation if query
 retries are disabled (#64948)

SQLStore: Fix SQLite error propagation if query retries are disabled (#64904)

* SQLStore: Add test when query retrying is disabled

* Fix condition

* Add test cases for sqlite3.ErrLocked

(cherry picked from commit 41843464d17df3800df447af4eec1ebfcc54d42b)

Co-authored-by: Sofia Papagiannaki <1632407+papagian@users.noreply.github.com>
---
 pkg/services/sqlstore/session.go      |  2 +-
 pkg/services/sqlstore/session_test.go | 80 +++++++++++++++++++++++----
 2 files changed, 69 insertions(+), 13 deletions(-)

diff --git a/pkg/services/sqlstore/session.go b/pkg/services/sqlstore/session.go
index 062d166963f..f475461e9ec 100644
--- a/pkg/services/sqlstore/session.go
+++ b/pkg/services/sqlstore/session.go
@@ -95,7 +95,7 @@ func (ss *SQLStore) retryOnLocks(ctx context.Context, callback DBTransactionFunc
 			ctxLogger.Info("Database locked, sleeping then retrying", "error", err, "retry", retry, "code", sqlError.Code)
 			// retryer immediately returns the error (if there is one) without checking the response
 			// therefore we only have to send it if we have reached the maximum retries
-			if retry == ss.dbCfg.QueryRetries {
+			if retry >= ss.dbCfg.QueryRetries {
 				return retryer.FuncError, ErrMaximumRetriesReached.Errorf("retry %d: %w", retry, err)
 			}
 			return retryer.FuncFailure, nil
diff --git a/pkg/services/sqlstore/session_test.go b/pkg/services/sqlstore/session_test.go
index 160bc280216..c833745f2bc 100644
--- a/pkg/services/sqlstore/session_test.go
+++ b/pkg/services/sqlstore/session_test.go
@@ -7,9 +7,61 @@ import (
 	"testing"
 
 	"github.com/mattn/go-sqlite3"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
+func TestRetryingDisabled(t *testing.T) {
+	store := InitTestDB(t)
+	require.Equal(t, 0, store.dbCfg.QueryRetries)
+
+	funcToTest := map[string]func(ctx context.Context, callback DBTransactionFunc) error{
+		"WithDbSession":    store.WithDbSession,
+		"WithNewDbSession": store.WithNewDbSession,
+	}
+
+	for name, f := range funcToTest {
+		t.Run(fmt.Sprintf("%s should return the error immediately", name), func(t *testing.T) {
+			i := 0
+			callback := func(sess *DBSession) error {
+				i++
+				return errors.New("some error")
+			}
+			err := f(context.Background(), callback)
+			require.Error(t, err)
+			require.Equal(t, 1, i)
+		})
+
+		errCodes := []sqlite3.ErrNo{sqlite3.ErrBusy, sqlite3.ErrLocked}
+		for _, c := range errCodes {
+			t.Run(fmt.Sprintf("%s should return the sqlite3.Error %v immediately", name, c.Error()), func(t *testing.T) {
+				i := 0
+				callback := func(sess *DBSession) error {
+					i++
+					return sqlite3.Error{Code: c}
+				}
+				err := f(context.Background(), callback)
+				require.Error(t, err)
+				var driverErr sqlite3.Error
+				require.ErrorAs(t, err, &driverErr)
+				require.Equal(t, 1, i)
+				assert.Equal(t, c, driverErr.Code)
+			})
+		}
+
+		t.Run(fmt.Sprintf("%s should not return error if the callback succeeds", name), func(t *testing.T) {
+			i := 0
+			callback := func(sess *DBSession) error {
+				i++
+				return nil
+			}
+			err := f(context.Background(), callback)
+			require.NoError(t, err)
+			require.Equal(t, 1, i)
+		})
+	}
+}
+
 func TestRetryingOnFailures(t *testing.T) {
 	store := InitTestDB(t)
 	store.dbCfg.QueryRetries = 5
@@ -31,18 +83,22 @@ func TestRetryingOnFailures(t *testing.T) {
 			require.Equal(t, 1, i)
 		})
 
-		t.Run(fmt.Sprintf("%s should return the sqlite3.Error if all retries have failed", name), func(t *testing.T) {
-			i := 0
-			callback := func(sess *DBSession) error {
-				i++
-				return sqlite3.Error{Code: sqlite3.ErrBusy}
-			}
-			err := f(context.Background(), callback)
-			require.Error(t, err)
-			var driverErr sqlite3.Error
-			require.ErrorAs(t, err, &driverErr)
-			require.Equal(t, store.dbCfg.QueryRetries, i)
-		})
+		errCodes := []sqlite3.ErrNo{sqlite3.ErrBusy, sqlite3.ErrLocked}
+		for _, c := range errCodes {
+			t.Run(fmt.Sprintf("%s should return the sqlite3.Error %v if all retries have failed", name, c.Error()), func(t *testing.T) {
+				i := 0
+				callback := func(sess *DBSession) error {
+					i++
+					return sqlite3.Error{Code: c}
+				}
+				err := f(context.Background(), callback)
+				require.Error(t, err)
+				var driverErr sqlite3.Error
+				require.ErrorAs(t, err, &driverErr)
+				require.Equal(t, store.dbCfg.QueryRetries, i)
+				assert.Equal(t, c, driverErr.Code)
+			})
+		}
 
 		t.Run(fmt.Sprintf("%s should not return the error if successive retries succeed", name), func(t *testing.T) {
 			i := 0