Merge pull request #373 from bootjp/copilot/sub-pr-372

proxy: fix EVALSHA_RO fallback, guard script cache on primary error, bounded eviction

Author: bootjp

go.mod

@@ -19,6 +19,7 @@ require (
 	github.com/emirpasic/gods v1.18.1
 	github.com/getsentry/sentry-go v0.27.0
 	github.com/hashicorp/go-hclog v1.6.3
+	github.com/hashicorp/go-msgpack/v2 v2.1.2
 	github.com/hashicorp/raft v1.7.3
 	github.com/pkg/errors v0.9.1
 	github.com/prometheus/client_golang v1.23.2
@@ -66,7 +67,6 @@ require (
 	github.com/hashicorp/errwrap v1.0.0 // indirect
 	github.com/hashicorp/go-immutable-radix v1.3.1 // indirect
 	github.com/hashicorp/go-metrics v0.5.4 // indirect
-	github.com/hashicorp/go-msgpack/v2 v2.1.2 // indirect
 	github.com/hashicorp/go-multierror v1.1.1 // indirect
 	github.com/hashicorp/golang-lru v1.0.2 // indirect
 	github.com/klauspost/compress v1.18.0 // indirect

proxy/dualwrite.go

@@ -37,6 +37,8 @@ type DualWriter struct {
 	closed   bool
 	scriptMu sync.RWMutex
 	scripts  map[string]string
+	// scriptOrder tracks insertion order for FIFO eviction of the bounded script cache.
+	scriptOrder []string
 }
 
 // NewDualWriter creates a DualWriter with the given backends.
@@ -193,13 +195,13 @@ func (d *DualWriter) Script(ctx context.Context, cmd string, args [][]byte) (any
 	result := d.primary.Do(ctx, iArgs...)
 	resp, err := result.Result()
 	d.metrics.CommandDuration.WithLabelValues(cmd, d.primary.Name()).Observe(time.Since(start).Seconds())
-	d.rememberScript(cmd, args)
 
 	if err != nil && !errors.Is(err, redis.Nil) {
 		d.metrics.CommandTotal.WithLabelValues(cmd, d.primary.Name(), "error").Inc()
 		return nil, fmt.Errorf("primary script %s: %w", cmd, err)
 	}
 	d.metrics.CommandTotal.WithLabelValues(cmd, d.primary.Name(), "ok").Inc()
+	d.rememberScript(cmd, args)
 
 	if d.hasSecondaryWrite() {
 		d.goWrite(func() { d.writeSecondary(cmd, iArgs) })

proxy/proxy_test.go

@@ -3,6 +3,7 @@ package proxy
 import (
 	"context"
 	"errors"
+	"fmt"
 	"io"
 	"log/slog"
 	"sync"
@@ -829,6 +830,107 @@ func TestDualWriter_Script_CachesEvalForEvalSHAFallback(t *testing.T) {
 	assert.InDelta(t, 0, testutil.ToFloat64(metrics.SecondaryWriteErrors), 0.001)
 }
 
+func TestDualWriter_Script_EvalSHARO_FallsBackToEvalRO(t *testing.T) {
+	primary := newMockBackend("primary")
+	primary.doFunc = makeCmd("OK", nil)
+
+	secondary := newMockBackend("secondary")
+	script := "return KEYS[1]"
+	sha := scriptSHA(script)
+	var calls int
+	secondary.doFunc = func(ctx context.Context, args ...any) *redis.Cmd {
+		calls++
+		cmd := redis.NewCmd(ctx, args...)
+		switch calls {
+		case 1:
+			assert.Equal(t, []byte("EVALSHA_RO"), args[0])
+			cmd.SetErr(testRedisErr("NOSCRIPT No matching script. Please use EVAL."))
+		case 2:
+			// Must fall back to EVAL_RO, not EVAL, to preserve read-only semantics.
+			assert.Equal(t, []byte("EVAL_RO"), args[0])
+			assert.Equal(t, []byte(script), args[1])
+			cmd.SetVal("mykey")
+		default:
+			t.Fatalf("unexpected secondary call %d", calls)
+		}
+		return cmd
+	}
+
+	metrics := newTestMetrics()
+	d := NewDualWriter(primary, secondary, ProxyConfig{Mode: ModeRedisOnly, SecondaryTimeout: time.Second}, metrics, newTestSentry(), testLogger)
+
+	// Register the script body via EVAL_RO so the proxy can fall back.
+	_, err := d.Script(context.Background(), "EVAL_RO", [][]byte{[]byte("EVAL_RO"), []byte(script), []byte("1"), []byte("mykey")})
+	assert.NoError(t, err)
+
+	d.cfg.Mode = ModeDualWrite
+	d.writeSecondary("EVALSHA_RO", []any{[]byte("EVALSHA_RO"), []byte(sha), []byte("1"), []byte("mykey")})
+
+	assert.Equal(t, 2, calls)
+	assert.InDelta(t, 0, testutil.ToFloat64(metrics.SecondaryWriteErrors), 0.001)
+}
+
+func TestDualWriter_Script_NoRememberOnPrimaryError(t *testing.T) {
+	// Verify that a failed SCRIPT FLUSH on the primary does NOT clear the proxy
+	// script cache, so that subsequent EVALSHA → EVAL fallbacks still work.
+	primary := newMockBackend("primary")
+	primary.doFunc = makeCmd(nil, testRedisErr("ERR flush failed"))
+
+	secondary := newMockBackend("secondary")
+	secondary.doFunc = makeCmd("OK", nil)
+
+	metrics := newTestMetrics()
+	d := NewDualWriter(primary, secondary, ProxyConfig{Mode: ModeRedisOnly, SecondaryTimeout: time.Second}, metrics, newTestSentry(), testLogger)
+
+	// Seed the script cache directly.
+	script := "return 1"
+	d.storeScript(script)
+	sha := scriptSHA(script)
+	_, cached := d.lookupScript(sha)
+	assert.True(t, cached, "script should be cached before flush attempt")
+
+	// Attempt SCRIPT FLUSH — primary returns an error so the cache must be untouched.
+	_, err := d.Script(context.Background(), "SCRIPT", [][]byte{[]byte("SCRIPT"), []byte("FLUSH")})
+	assert.Error(t, err)
+
+	_, stillCached := d.lookupScript(sha)
+	assert.True(t, stillCached, "cache must not be cleared when primary SCRIPT FLUSH fails")
+}
+
+func TestDualWriter_ScriptCache_BoundedEviction(t *testing.T) {
+	// Fill the cache beyond maxScriptCacheSize and verify it stays bounded.
+	primary := newMockBackend("primary")
+	primary.doFunc = makeCmd("OK", nil)
+
+	metrics := newTestMetrics()
+	d := NewDualWriter(primary, nil, ProxyConfig{Mode: ModeRedisOnly, SecondaryTimeout: time.Second}, metrics, newTestSentry(), testLogger)
+
+	// Insert maxScriptCacheSize+10 unique scripts.
+	total := maxScriptCacheSize + 10
+	for i := range total {
+		d.storeScript(fmt.Sprintf("return %d", i))
+	}
+
+	d.scriptMu.RLock()
+	size := len(d.scripts)
+	d.scriptMu.RUnlock()
+
+	assert.Equal(t, maxScriptCacheSize, size, "cache must not exceed maxScriptCacheSize")
+
+	// The first 10 scripts (insertion order) must have been evicted.
+	for i := range 10 {
+		sha := scriptSHA(fmt.Sprintf("return %d", i))
+		_, ok := d.lookupScript(sha)
+		assert.False(t, ok, "script %d should have been evicted", i)
+	}
+	// The last maxScriptCacheSize scripts must still be present.
+	for i := 10; i < total; i++ {
+		sha := scriptSHA(fmt.Sprintf("return %d", i))
+		_, ok := d.lookupScript(sha)
+		assert.True(t, ok, "script %d should still be cached", i)
+	}
+}
+
 // ========== writeRedisValue tests ==========
 
 // testRedisErr satisfies the redis.Error interface for testing.

proxy/script_cache.go

@@ -10,20 +10,27 @@ import (
 )
 
 const (
-	cmdEval    = "EVAL"
-	cmdEvalSHA = "EVALSHA"
-	cmdScript  = "SCRIPT"
+	cmdEval      = "EVAL"
+	cmdEvalRO    = "EVAL_RO"
+	cmdEvalSHA   = "EVALSHA"
+	cmdEvalSHARO = "EVALSHA_RO"
+	cmdScript    = "SCRIPT"
 
 	minScriptSubcommandArgs = 2
 	scriptLoadArgIndex      = 2
 	minEvalSHAArgs          = 2
+
+	// maxScriptCacheSize is the maximum number of scripts retained in the
+	// proxy-side script cache. When the limit is reached, the oldest entry
+	// (by insertion order) is evicted to prevent unbounded memory growth.
+	maxScriptCacheSize = 512
 )
 
 func (d *DualWriter) rememberScript(cmd string, args [][]byte) {
 	upper := strings.ToUpper(cmd)
 
 	switch upper {
-	case cmdEval, "EVAL_RO":
+	case cmdEval, cmdEvalRO:
 		if len(args) > 1 {
 			d.storeScript(string(args[1]))
 		}
@@ -47,13 +54,30 @@ func (d *DualWriter) storeScript(script string) {
 
 	d.scriptMu.Lock()
 	defer d.scriptMu.Unlock()
+
+	if _, exists := d.scripts[sha]; !exists {
+		// Evict the oldest entry when at capacity.
+		// scriptOrder and scripts always have the same length, so the guard is
+		// sufficient; the additional len(scriptOrder) check is not needed.
+		if len(d.scripts) >= maxScriptCacheSize {
+			oldest := d.scriptOrder[0]
+			d.scriptOrder = d.scriptOrder[1:]
+			delete(d.scripts, oldest)
+		}
+		// Append to the end so that eviction follows strict FIFO insertion order.
+		// Re-storing an already-cached script does not move its position; the
+		// entry retains its original eviction priority, which is acceptable for
+		// this use-case because script bodies are immutable (same SHA ⇒ same body).
+		d.scriptOrder = append(d.scriptOrder, sha)
+	}
 	d.scripts[sha] = script
 }
 
 func (d *DualWriter) clearScripts() {
 	d.scriptMu.Lock()
 	defer d.scriptMu.Unlock()
 	clear(d.scripts)
+	d.scriptOrder = d.scriptOrder[:0]
 }
 
 func (d *DualWriter) lookupScript(sha string) (string, bool) {
@@ -65,7 +89,7 @@ func (d *DualWriter) lookupScript(sha string) (string, bool) {
 
 func (d *DualWriter) evalFallbackArgs(cmd string, iArgs []any) ([]any, bool) {
 	upper := strings.ToUpper(cmd)
-	if upper != cmdEvalSHA && upper != "EVALSHA_RO" {
+	if upper != cmdEvalSHA && upper != cmdEvalSHARO {
 		return nil, false
 	}
 	if len(iArgs) < minEvalSHAArgs {
@@ -78,8 +102,14 @@ func (d *DualWriter) evalFallbackArgs(cmd string, iArgs []any) ([]any, bool) {
 		return nil, false
 	}
 
+	// Preserve the read-only semantics: EVALSHA_RO falls back to EVAL_RO.
+	fallbackCmd := cmdEval
+	if upper == cmdEvalSHARO {
+		fallbackCmd = cmdEvalRO
+	}
+
 	fallback := make([]any, len(iArgs))
-	fallback[0] = []byte(cmdEval)
+	fallback[0] = []byte(fallbackCmd)
 	fallback[1] = []byte(script)
 	copy(fallback[2:], iArgs[2:])
 	return fallback, true