Skip to content

Commit 0617c43

Browse files
committed
fix(redis): address review feedback for keyTypeAtExpect
Address findings from claude review on 79a6989: - [P0] Add hasHigherPriorityStringEncoding guard on the fast-path hit in keyTypeAtExpect, consistent with the wide-column fast-path callers (hashFieldFastLookup, zsetMemberFastScore, setMemberFastExists, hashFieldFastExists). Without this guard the rawKeyTypeAt "string wins" tiebreaker is bypassed when a redisStrKey row exists alongside a collection encoding for the same user key, returning the collection-specific answer instead of the WRONGTYPE / nil that the slow path produces. Cost: one extra ExistsAt seek per fast-path hit on non-string types; the 19->3/4 reduction is preserved. - [P1] Replace context.Background() with the in-scope ctx in eight keyTypeAtExpect call sites whose enclosing function (or retryRedisWrite closure) carries a real context with timeout / cancellation: applyHashFieldPairs (closure ctx from L2040 WithTimeout) hdelTxn (function param) hincrbyTxn (function param) zaddTxn (function param) zincrbyTxn (function param) zrem closure (closure ctx from L3540 WithTimeout) zremrangebyrank closure (closure ctx from L3588 WithTimeout) tryBZPopMin closure (closure ctx from L3625 WithTimeout) Pre-existing bug on the keyTypeAt callers, now fixed at the same lines this PR already touches. Callers without ctx in scope (hmget, hlen, hgetall, zrangeRead, xlen, rangeStream) are pre-existing and out of scope.
1 parent 79a6989 commit 0617c43

2 files changed

Lines changed: 37 additions & 17 deletions

File tree

adapter/redis_compat_commands.go

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2042,7 +2042,7 @@ func (r *RedisServer) applyHashFieldPairs(key []byte, args [][]byte) (int, error
20422042
var added int
20432043
err := r.retryRedisWrite(ctx, func() error {
20442044
readTS := r.readTS()
2045-
typ, err := r.keyTypeAtExpect(context.Background(), key, readTS, redisTypeHash)
2045+
typ, err := r.keyTypeAtExpect(ctx, key, readTS, redisTypeHash)
20462046
if err != nil {
20472047
return err
20482048
}
@@ -2669,7 +2669,7 @@ func (r *RedisServer) resolveHashFieldDelElems(ctx context.Context, key []byte,
26692669

26702670
func (r *RedisServer) hdelTxn(ctx context.Context, key []byte, fields [][]byte) (int, error) {
26712671
readTS := r.readTS()
2672-
typ, err := r.keyTypeAtExpect(context.Background(), key, readTS, redisTypeHash)
2672+
typ, err := r.keyTypeAtExpect(ctx, key, readTS, redisTypeHash)
26732673
if err != nil {
26742674
return 0, err
26752675
}
@@ -2941,7 +2941,7 @@ func (r *RedisServer) hincrbyWithMigration(ctx context.Context, key, fieldKey []
29412941

29422942
func (r *RedisServer) hincrbyTxn(ctx context.Context, key, field []byte, increment int64) (int64, error) {
29432943
readTS := r.readTS()
2944-
typ, err := r.keyTypeAtExpect(context.Background(), key, readTS, redisTypeHash)
2944+
typ, err := r.keyTypeAtExpect(ctx, key, readTS, redisTypeHash)
29452945
if err != nil {
29462946
return 0, err
29472947
}
@@ -3262,7 +3262,7 @@ func (r *RedisServer) applyZAddPair(ctx context.Context, key []byte, p zaddPair,
32623262

32633263
func (r *RedisServer) zaddTxn(ctx context.Context, key []byte, flags zaddFlags, pairs []zaddPair) (int, error) {
32643264
readTS := r.readTS()
3265-
typ, err := r.keyTypeAtExpect(context.Background(), key, readTS, redisTypeZSet)
3265+
typ, err := r.keyTypeAtExpect(ctx, key, readTS, redisTypeZSet)
32663266
if err != nil {
32673267
return 0, err
32683268
}
@@ -3332,7 +3332,7 @@ func (r *RedisServer) zaddTxn(ctx context.Context, key []byte, flags zaddFlags,
33323332
// Returns the new score after applying increment.
33333333
func (r *RedisServer) zincrbyTxn(ctx context.Context, key []byte, member string, increment float64) (float64, error) {
33343334
readTS := r.readTS()
3335-
typ, err := r.keyTypeAtExpect(context.Background(), key, readTS, redisTypeZSet)
3335+
typ, err := r.keyTypeAtExpect(ctx, key, readTS, redisTypeZSet)
33363336
if err != nil {
33373337
return 0, err
33383338
}
@@ -3542,7 +3542,7 @@ func (r *RedisServer) zrem(conn redcon.Conn, cmd redcon.Command) {
35423542
var removed int
35433543
if err := r.retryRedisWrite(ctx, func() error {
35443544
readTS := r.readTS()
3545-
typ, err := r.keyTypeAtExpect(context.Background(), cmd.Args[1], readTS, redisTypeZSet)
3545+
typ, err := r.keyTypeAtExpect(ctx, cmd.Args[1], readTS, redisTypeZSet)
35463546
if err != nil {
35473547
return err
35483548
}
@@ -3590,7 +3590,7 @@ func (r *RedisServer) zremrangebyrank(conn redcon.Conn, cmd redcon.Command) {
35903590
var removed int
35913591
if err := r.retryRedisWrite(ctx, func() error {
35923592
readTS := r.readTS()
3593-
typ, err := r.keyTypeAtExpect(context.Background(), cmd.Args[1], readTS, redisTypeZSet)
3593+
typ, err := r.keyTypeAtExpect(ctx, cmd.Args[1], readTS, redisTypeZSet)
35943594
if err != nil {
35953595
return err
35963596
}
@@ -3627,7 +3627,7 @@ func (r *RedisServer) tryBZPopMin(key []byte) (*bzpopminResult, error) {
36273627
var result *bzpopminResult
36283628
err := r.retryRedisWrite(ctx, func() error {
36293629
readTS := r.readTS()
3630-
typ, err := r.keyTypeAtExpect(context.Background(), key, readTS, redisTypeZSet)
3630+
typ, err := r.keyTypeAtExpect(ctx, key, readTS, redisTypeZSet)
36313631
if err != nil {
36323632
return err
36333633
}

adapter/redis_compat_helpers.go

Lines changed: 29 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -319,16 +319,27 @@ func (r *RedisServer) keyTypeAt(ctx context.Context, key []byte, readTS uint64)
319319
// seeks across every collection family before returning. The fast path:
320320
//
321321
// 1. Probe only the prefixes for `expected` (typically 2-3 seeks).
322-
// 2. On hit, apply the TTL filter and return `expected`.
322+
// 2. On hit, run the same string-priority guard the wide-column
323+
// fast-path callers use (hashFieldFastLookup, zsetMemberFastScore,
324+
// setMemberFastExists, hashFieldFastExists). When a redisStrKey
325+
// row also exists at the same user key, fall back to the slow
326+
// path so the rawKeyTypeAt "string wins" tiebreaker fires and
327+
// the caller gets WRONGTYPE / nil instead of the
328+
// collection-specific answer. The guard is the narrow form (see
329+
// hasHigherPriorityStringEncoding's doc comment): only redisStrKey
330+
// is checked, the rarer HLL / legacy-bare-key dual-encoding cases
331+
// remain a known residual risk shared with the other fast-path
332+
// callers.
323333
// 3. On miss, fall back to the full keyTypeAt slow path so that
324-
// wrongType collisions (the key exists under a different type) still
325-
// surface as the correct redisValueType.
334+
// wrongType collisions (the key exists under a different type)
335+
// still surface as the correct redisValueType.
326336
//
327337
// Steady-state production: most XADD/XREAD/HSET/etc. calls are on a key
328338
// of the expected type, so step 1 hits and the slow-path 19 seeks shrink
329-
// to 2-3. The slow path stays in place for first-write and wrongType
330-
// cases, which keep their existing semantics — wrongTypeError detection
331-
// is preserved by the fall-through.
339+
// to 2-3 (plus the priority-guard ExistsAt). The slow path stays in
340+
// place for first-write and wrongType cases, which keep their existing
341+
// semantics — wrongTypeError detection is preserved by the
342+
// fall-through.
332343
func (r *RedisServer) keyTypeAtExpect(ctx context.Context, key []byte, readTS uint64, expected redisValueType) (redisValueType, error) {
333344
if expected == redisTypeNone {
334345
return r.keyTypeAt(ctx, key, readTS)
@@ -337,10 +348,19 @@ func (r *RedisServer) keyTypeAtExpect(ctx context.Context, key []byte, readTS ui
337348
if err != nil {
338349
return redisTypeNone, err
339350
}
340-
if found {
341-
return r.applyTTLFilter(ctx, key, readTS, expected)
351+
if !found {
352+
return r.keyTypeAt(ctx, key, readTS)
353+
}
354+
if expected != redisTypeString {
355+
higher, hErr := r.hasHigherPriorityStringEncoding(ctx, key, readTS)
356+
if hErr != nil {
357+
return redisTypeNone, hErr
358+
}
359+
if higher {
360+
return r.keyTypeAt(ctx, key, readTS)
361+
}
342362
}
343-
return r.keyTypeAt(ctx, key, readTS)
363+
return r.applyTTLFilter(ctx, key, readTS, expected)
344364
}
345365

346366
// probeExpectedType issues only the prefix probes for the given type.

0 commit comments

Comments
 (0)