Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions adapter/redis_lists.go
Original file line number Diff line number Diff line change
Expand Up @@ -713,6 +713,14 @@ func (r *RedisServer) rangeList(ctx context.Context, key []byte, startRaw, endRa
return r.proxyLRange(key, startRaw, endRaw)
}

// PR #749 follow-up: pass the per-call dispatch ctx so a stalled
// VerifyLeaderForKey honours the caller's deadline rather than the
// long-lived handlerContext + verifyLeaderEngineCtx fallback. Same
// shape as keys() / FLUSHDB.
if err := r.coordinator.VerifyLeaderForKey(ctx, key); err != nil {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Wait for the read index to apply before snapshotting

When the local leader has committed but not yet applied entries, VerifyLeaderForKey is not an applied-state fence: it reaches Engine.VerifyLeader, which calls submitRead(ctx, false), and handleReadStates completes such requests immediately without waiting for e.applied. Consequently this call can succeed while the MVCC store and its LastCommitTS remain stale, so the immediately selected LRANGE snapshot can still omit a write that completed before the read; the new EXEC pre-pass has the same problem. Use the linearizable/lease-read path that waits for the read index to be applied before selecting the snapshot.

Useful? React with 👍 / 👎.

return nil, errors.WithStack(err)
}

readTS := r.readTS()
typ, err := r.keyTypeAt(ctx, key, readTS)
if err != nil {
Expand All @@ -725,14 +733,6 @@ func (r *RedisServer) rangeList(ctx context.Context, key []byte, startRaw, endRa
return nil, wrongTypeError()
}

// PR #749 follow-up: pass the per-call dispatch ctx so a stalled
// VerifyLeaderForKey honours the caller's deadline rather than the
// long-lived handlerContext + verifyLeaderEngineCtx fallback. Same
// shape as keys() / FLUSHDB.
if err := r.coordinator.VerifyLeaderForKey(ctx, key); err != nil {
return nil, errors.WithStack(err)
}

meta, exists, err := r.resolveListMeta(ctx, key, readTS)
if err != nil {
return nil, err
Expand Down
35 changes: 35 additions & 0 deletions adapter/redis_txn.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,33 @@ var txnApplyHandlers = map[string]txnCommandHandler{
cmdPExpire: (*txnContext).applyExpireMilliseconds,
}

func (r *RedisServer) verifyQueuedCommandLeaders(ctx context.Context, queue []redcon.Command) error {
if r.coordinator == nil {
return nil
}
seen := make(map[string]struct{}, len(queue))
for _, cmd := range queue {
if len(cmd.Args) == 0 {
continue
}
meta, ok := redisCommandTable[strings.ToUpper(string(cmd.Args[0]))]
if !ok {
continue
}
for _, key := range redisCommandGetKeys(meta, cmd.Args) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Fence each Raft group once instead of every Redis key

When a valid transaction contains many distinct keys, such as one variadic DEL or EXISTS, this loop performs a sequential VerifyLeaderForKey for every key even when all keys resolve to the same Raft group. Each verification submits a quorum-backed Raft read request, so a large command creates hundreds or thousands of serialized ReadIndex rounds under the single 30-second EXEC context, causing otherwise valid transactions to time out and unnecessarily loading the Raft group. Resolve and deduplicate the affected groups, then fence each group once.

Useful? React with 👍 / 👎.

keyID := string(key)
if _, ok := seen[keyID]; ok {
continue
}
seen[keyID] = struct{}{}
if err := r.coordinator.VerifyLeaderForKey(ctx, key); err != nil {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Route fences through Redis-safe storage keys

For a legal Redis user key that resembles another adapter's namespace, such as !sqs|foo, passing the raw command argument to VerifyLeaderForKey changes its routing identity: routeKey treats the raw !sqs|... value as SQS-internal and routes it to the global SQS group, whereas the actual Redis rows (for example !redis|str|!sqs|foo or list-family rows) normalize back to the literal Redis user key and may belong to a different shard. The pre-pass can therefore fence an unrelated group, fail because this node does not lead that group, and leave the actual Redis data shard unfenced. Derive the fence footprint from canonical Redis storage keys, retaining a separate legacy-bare-key fence only where fallback reads require it.

Useful? React with 👍 / 👎.

return errors.WithStack(err)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Use leader-routed fences for queued shard keys

In a multi-group deployment with leadership balanced across nodes, exec proxies only to the default-group leader (IsLeader()), but this call verifies each key against that node's local shard engine. ShardedCoordinator.VerifyLeaderForKey does not forward to the key's leader, so even a single-key EXEC targeting a non-default shard led by another node returns a leader error (and cross-shard EXEC cannot succeed unless one node leads every involved group). Use a leader-routed per-shard fence or otherwise route execution without requiring the default leader to lead each key's group.

Useful? React with 👍 / 👎.

}
}
}
return nil
}

// MULTI/EXEC/DISCARD handling
func (r *RedisServer) multi(conn redcon.Conn, _ redcon.Command) {
state := getConnState(conn)
Expand Down Expand Up @@ -2379,6 +2406,10 @@ func (r *RedisServer) runTransactionDirect(queue []redcon.Command) ([]redisResul

var results []redisResult
err := r.retryRedisWrite(dispatchCtx, func() error {
if err := r.verifyQueuedCommandLeaders(dispatchCtx, queue); err != nil {
return err
}

startTS := r.txnStartTS()
readPin := r.pinReadTS(startTS)
defer readPin.Release()
Expand Down Expand Up @@ -2593,6 +2624,10 @@ func (r *RedisServer) runTransactionWithDedup(queue []redcon.Command) ([]redisRe
// from runTransactionWithDedup to keep that loop under the cyclop
// budget; the dedup rationale lives there.
func (r *RedisServer) firstExecAttempt(dispatchCtx context.Context, queue []redcon.Command) ([]redisResult, *reusableExecTxn, error) {
if err := r.verifyQueuedCommandLeaders(dispatchCtx, queue); err != nil {
return nil, nil, err
}

startTS := r.txnStartTS()
readPin := r.pinReadTS(startTS)
defer readPin.Release()
Expand Down
97 changes: 97 additions & 0 deletions adapter/redis_txn_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,34 @@ func newRedisTxnTestContext(server *RedisServer) *txnContext {
}
}

type verifyHookCoordinator struct {
*localAdapterCoordinator
verifyForKey func(context.Context, []byte) error
verifyCalls int
}

func newVerifyHookCoordinator(st store.MVCCStore) *verifyHookCoordinator {
return &verifyHookCoordinator{localAdapterCoordinator: newLocalAdapterCoordinator(st)}
}

func (c *verifyHookCoordinator) VerifyLeaderForKey(ctx context.Context, key []byte) error {
c.verifyCalls++
if c.verifyForKey != nil {
return c.verifyForKey(ctx, key)
}
return c.localAdapterCoordinator.VerifyLeaderForKey(ctx, key)
}

func seedRedisListAt(t *testing.T, st store.MVCCStore, key []byte, ts uint64, values ...string) {
t.Helper()
metaBytes, err := store.MarshalListMeta(store.ListMeta{Len: int64(len(values))})
require.NoError(t, err)
require.NoError(t, st.PutAt(context.Background(), store.ListMetaKey(key), metaBytes, ts, 0))
for i, value := range values {
require.NoError(t, st.PutAt(context.Background(), listItemKey(key, int64(i)), []byte(value), ts, 0))
}
}

func elemKeysContain(elems []*kv.Elem[kv.OP], want []byte) bool {
for _, elem := range elems {
if elem != nil && string(elem.Key) == string(want) {
Expand Down Expand Up @@ -117,6 +145,75 @@ func requireReadKeysMatch(t *testing.T, got [][]byte, want [][]byte) {
require.Equal(t, wantSet, gotSet)
}

func TestRedisRangeListVerifiesLeaderBeforeSnapshot(t *testing.T) {
t.Parallel()

st := store.NewMVCCStore()
key := []byte("list:leader-fence-lrange")
coord := newVerifyHookCoordinator(st)
server := NewRedisServer(nil, "", st, coord, nil, nil)

seeded := false
coord.verifyForKey = func(_ context.Context, got []byte) error {
require.Equal(t, key, got)
if !seeded {
seedRedisListAt(t, st, key, 10, "v1")
seeded = true
}
return nil
}

got, err := server.rangeList(context.Background(), key, []byte("0"), []byte("-1"))
require.NoError(t, err)
require.Equal(t, []string{"v1"}, got)
require.Equal(t, 1, coord.verifyCalls)
}

func TestRedisExecVerifiesLeaderBeforeSnapshot(t *testing.T) {
t.Parallel()

for _, tc := range []struct {
name string
dedup bool
}{
{name: "direct", dedup: false},
{name: "dedup", dedup: true},
} {
t.Run(tc.name, func(t *testing.T) {
t.Parallel()

st := store.NewMVCCStore()
key := []byte("list:leader-fence-exec:" + tc.name)
coord := newVerifyHookCoordinator(st)
server := &RedisServer{
store: st,
coordinator: coord,
scriptCache: map[string]string{},
onePhaseTxnDedup: tc.dedup,
}

seeded := false
coord.verifyForKey = func(_ context.Context, got []byte) error {
require.Equal(t, key, got)
if !seeded {
seedRedisListAt(t, st, key, 10, "v1")
seeded = true
}
return nil
}

results, err := server.runTransaction([]redcon.Command{{
Args: [][]byte{[]byte(cmdLRange), key, []byte("0"), []byte("-1")},
}})
require.NoError(t, err)
require.Len(t, results, 1)
require.Equal(t, resultArray, results[0].typ)
require.Equal(t, []string{"v1"}, results[0].arr)
require.Equal(t, 1, coord.verifyCalls)
})
}
}

// TestRedisTxnValidateReadSet_ConcurrentRPushTriggersConflict verifies that a
// concurrent RPUSH to a list triggers an OCC read-write conflict for a MULTI
// transaction that read the list via LRANGE. Without the boundary key tracking
Expand Down
Loading