Merge branch 'fix_lua_scirpt_readonly_error_handling' into 'v9_master'

[DBS-000] fix lua script error handling

See merge request dbops/Redis!17

Author: Runze Cui

internal/proto/redis_errors.go

@@ -290,11 +290,23 @@ func IsReadOnlyError(err error) bool {
 	}
 	// Check if wrapped error is a RedisError with READONLY prefix
 	var redisErr RedisError
-	if errors.As(err, &redisErr) && strings.HasPrefix(redisErr.Error(), "READONLY ") {
-		return true
+	if errors.As(err, &redisErr) {
+		errMsg := redisErr.Error()
+		if strings.HasPrefix(errMsg, "READONLY ") {
+			return true
+		}
+		// For Lua scripts, the read-only error string contains "-READONLY" rather than beginning with "READONLY "
+		if strings.Contains(errMsg, "-READONLY ") {
+			return true
+		}
 	}
 	// Fallback to string checking for backward compatibility
-	return strings.HasPrefix(err.Error(), "READONLY ")
+	errMsg := err.Error()
+	if strings.HasPrefix(errMsg, "READONLY ") {
+		return true
+	}
+	// For Lua scripts, the read-only error string contains "-READONLY" rather than beginning with "READONLY "
+	return strings.Contains(errMsg, "-READONLY ")
 }
 
 // IsMovedError checks if an error is a MovedError, even if wrapped.

internal/proto/redis_errors_test.go

@@ -9,12 +9,12 @@ import (
 // TestTypedRedisErrors tests that typed Redis errors are created correctly
 func TestTypedRedisErrors(t *testing.T) {
 	tests := []struct {
-		name          string
-		errorMsg      string
-		expectedType  interface{}
-		expectedMsg   string
-		checkFunc     func(error) bool
-		extractAddr   func(error) string
+		name         string
+		errorMsg     string
+		expectedType interface{}
+		expectedMsg  string
+		checkFunc    func(error) bool
+		extractAddr  func(error) string
 	}{
 		{
 			name:         "LOADING error",
@@ -30,6 +30,13 @@ func TestTypedRedisErrors(t *testing.T) {
 			expectedMsg:  "READONLY You can't write against a read only replica",
 			checkFunc:    IsReadOnlyError,
 		},
+		{
+			name:         "READONLY error from Lua script",
+			errorMsg:     "ERR Error running script (call to f_xxx): @user_script:1: -READONLY You can't write against a read only replica",
+			expectedType: RedisError(""), // Lua script errors are parsed as generic RedisError (string type)
+			expectedMsg:  "ERR Error running script (call to f_xxx): @user_script:1: -READONLY You can't write against a read only replica",
+			checkFunc:    IsReadOnlyError, // But IsReadOnlyError should still detect it
+		},
 		{
 			name:         "MOVED error",
 			errorMsg:     "MOVED 3999 127.0.0.1:6381",
@@ -144,8 +151,17 @@ func TestTypedRedisErrors(t *testing.T) {
 			}
 
 			// Check error type using errors.As
-			if !errors.As(err, &tt.expectedType) {
-				t.Errorf("Error type mismatch: expected %T, got %T", tt.expectedType, err)
+			// Special handling for RedisError (string type) - use type assertion instead
+			if _, ok := tt.expectedType.(RedisError); ok {
+				// For RedisError, check using type assertion
+				if _, ok := err.(RedisError); !ok {
+					t.Errorf("Error type mismatch: expected RedisError, got %T", err)
+				}
+			} else {
+				// For other types, use errors.As
+				if !errors.As(err, &tt.expectedType) {
+					t.Errorf("Error type mismatch: expected %T, got %T", tt.expectedType, err)
+				}
 			}
 
 			// Check using the helper function
@@ -181,6 +197,11 @@ func TestWrappedTypedErrors(t *testing.T) {
 			errorMsg:  "READONLY You can't write against a read only replica",
 			checkFunc: IsReadOnlyError,
 		},
+		{
+			name:      "Wrapped READONLY error from Lua script",
+			errorMsg:  "ERR Error running script (call to f_xxx): @user_script:1: -READONLY You can't write against a read only replica",
+			checkFunc: IsReadOnlyError,
+		},
 		{
 			name:      "Wrapped CLUSTERDOWN error",
 			errorMsg:  "CLUSTERDOWN The cluster is down",
@@ -389,4 +410,3 @@ func TestBackwardCompatibility(t *testing.T) {
 		})
 	}
 }
-

redis.go

@@ -623,12 +623,7 @@ func (c *baseClient) initConn(ctx context.Context, cn *pool.Conn) error {
 	return nil
 }
 
-func (c *baseClient) releaseConn(ctx context.Context, cn *pool.Conn, err error) {
-	// Note: ReportResult is now called in withConn for circuit breaker limiters
-	// We only call it here for non-circuit-breaker limiters or when called from _withConn
-	// Since _withConn is only called from executeWithCircuitBreaker or when limiter is nil,
-	// we skip reporting here to avoid double-reporting
-
+func (c *baseClient) _releaseConn(ctx context.Context, cn *pool.Conn, err error) {
 	if isBadConn(err, false, c.opt.Addr) {
 		c.connPool.Remove(ctx, cn, err)
 	} else {
@@ -640,6 +635,13 @@ func (c *baseClient) releaseConn(ctx context.Context, cn *pool.Conn, err error)
 	}
 }
 
+func (c *baseClient) releaseConn(ctx context.Context, cn *pool.Conn, err error) {
+	if c.opt.Limiter != nil {
+		c.opt.Limiter.ReportResult(err)
+	}
+	c._releaseConn(ctx, cn, err)
+}
+
 func (c *baseClient) withConn(
 	ctx context.Context, fn func(context.Context, *pool.Conn) error,
 ) error {
@@ -661,14 +663,17 @@ func (c *baseClient) withConn(
 func (c *baseClient) _withConn(
 	ctx context.Context, fn func(context.Context, *pool.Conn) error,
 ) error {
-	cn, err := c.getConn(ctx)
+	// Use _getConn/_releaseConn to bypass limiter checks.
+	// Limiter is handled by the caller (withConn) to avoid double-checking
+	// when called from within circuit breaker Execute.
+	cn, err := c._getConn(ctx)
 	if err != nil {
 		return err
 	}
 
 	var fnErr error
 	defer func() {
-		c.releaseConn(ctx, cn, fnErr)
+		c._releaseConn(ctx, cn, fnErr)
 	}()
 
 	fnErr = fn(ctx, cn)
@@ -696,13 +701,19 @@ func (c *baseClient) executeWithCircuitBreaker(
 		}
 	}
 
+	// Execute with circuit breaker protection.
+	// _withConn uses _getConn/_releaseConn which bypass the limiter,
+	// avoiding double-checking since Allow() was already called in withConn().
 	return cbLimiter.Execute(func() error {
 		return c._withConn(ctx, fn)
 	})
 }
 
 func (c *baseClient) preConnect(ctx context.Context) error {
-	cn, err := c.getConn(ctx)
+	// Use _getConn to bypass limiter checks.
+	// This is called from executeWithCircuitBreaker after Allow() has already passed,
+	// to test connectivity before cb.Do() runs.
+	cn, err := c._getConn(ctx)
 	if err != nil {
 		return err
 	}