adapter: split redis.go / redis_compat_commands.go into cohesive files (no behavior change)#956
adapter: split redis.go / redis_compat_commands.go into cohesive files (no behavior change)#956bootjp wants to merge 4 commits into
Conversation
…s (no behavior change)
|
@claude review |
|
Claude finished @bootjp's task in 2m 12s —— View job Code Review:
|
| File | Domain |
|---|---|
redis.go |
Server struct, lifecycle, dispatch, tracing, pub/sub infra |
redis_txn.go |
MULTI/EXEC, txnContext, dedup machinery |
redis_lists.go |
List push/pop/range/trim (both directions, dedup path) |
redis_strings.go |
SET/GET/DEL/EXISTS options and execution |
redis_keys.go |
KEYS/SCAN pattern matching, visible-key encoding |
redis_proxy_leader.go |
Leader forwarding, go-redis writers |
redis_expire_cmds.go |
SETEX/GETDEL/SETNX, TTL/EXPIRE family |
redis_server_cmds.go |
INFO/CLIENT/COMMAND/HELLO/SELECT/PUBLISH/FLUSH/PUBSUB |
redis_set_cmds.go |
SADD/SREM/SISMEMBER/SMEMBERS/PFADD/PFCOUNT |
redis_hash_cmds.go |
HSET/HGET/HDEL/HEXISTS/HLEN/HINCRBY/HGETALL/INCR |
redis_zset_cmds.go |
ZADD/ZINCRBY/ZRANGE/ZREM/BZPOPMIN |
redis_stream_cmds.go |
XADD/XTRIM/XRANGE/XREVRANGE/XREAD/XLEN |
redis_compat_commands.go |
Shared constants (now misnamed — see §1) |
Verdict
Approve. The refactor is correct and the verification methodology (go doc parity + code-line multiset) is a solid proof of no behavioral change. Items 1–3 above are minor cleanup candidates for a follow-up PR; item 4 is pre-existing and tracked separately. Nothing here warrants blocking the merge.
📝 WalkthroughWalkthroughTen new files add a complete Redis protocol adapter on top of ElasticKV's Raft-backed KV store. The adapter covers strings, lists, hashes, sets, sorted sets, streams, expiry commands, MULTI/EXEC transactions, KEYS/SCAN key discovery, and Raft-leader-aware proxying, all using wide-column storage with legacy-blob migration paths. ChangesRedis Adapter — Full Command Implementation
Sequence Diagram(s)sequenceDiagram
participant Client
participant RedisServer
participant txnContext
participant Coordinator
participant Store
Client->>RedisServer: MULTI
Client->>RedisServer: SET k v
Client->>RedisServer: RPUSH list a
Client->>RedisServer: EXEC
RedisServer->>RedisServer: verify leader or proxyTransactionToLeader
RedisServer->>txnContext: create with txnStartTS (LastCommitTS+Observe)
loop per queued command
RedisServer->>txnContext: applyXxx (stage dirty values, track read keys)
end
txnContext->>txnContext: validateReadSet (OCC vs LatestCommitTS)
txnContext->>txnContext: prepareDispatch (OperationGroup + fenced commitTS)
txnContext->>Coordinator: commit dispatch
Coordinator-->>RedisServer: ok or retryable error
alt retryable and dedup enabled
RedisServer->>Coordinator: dispatchExecReuse (same write set, new commitTS)
end
RedisServer-->>Client: RESP2 array results
note over Client, Store: Follower read proxying
Client->>RedisServer: GET key (follower node)
RedisServer->>Store: tryLeaderGetAt (versioned RawGet via leader)
Store-->>RedisServer: value or ErrKeyNotFound
RedisServer-->>Client: bulk string reply
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements various Redis command adapters and transaction handlers for elastickv, introducing wide-column storage layouts, fast-path lookups, and transactional optimizations. The review feedback focuses heavily on ensuring proper context propagation by replacing instances of context.Background() with the active request or transaction context (ctx or t.ctx) across multiple command implementations to respect timeouts and cancellations. Additionally, it is recommended to log connection close errors in redis_server_cmds.go instead of silently ignoring them.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| var v []byte | ||
| err := r.retryRedisWrite(ctx, func() error { | ||
| readTS := r.readTS() | ||
| typ, err := r.keyTypeAt(context.Background(), key, readTS) |
There was a problem hiding this comment.
The request context ctx is available in this scope but context.Background() is passed to r.keyTypeAt. Using ctx ensures that request timeouts and cancellations are properly respected during the key type lookup.
| typ, err := r.keyTypeAt(context.Background(), key, readTS) | |
| typ, err := r.keyTypeAt(ctx, key, readTS) |
| // Wide-column path: check if any !hs|fld| keys exist for this key. | ||
| hashFieldPrefix := store.HashFieldScanPrefix(key) | ||
| hashFieldEnd := store.PrefixScanEnd(hashFieldPrefix) | ||
| wideKVs, err := r.store.ScanAt(context.Background(), hashFieldPrefix, hashFieldEnd, 1, readTS) |
There was a problem hiding this comment.
The request context ctx is available in hdelTxn but context.Background() is passed to r.store.ScanAt. Using ctx ensures that request timeouts and cancellations are properly respected during the scan.
| wideKVs, err := r.store.ScanAt(context.Background(), hashFieldPrefix, hashFieldEnd, 1, readTS) | |
| wideKVs, err := r.store.ScanAt(ctx, hashFieldPrefix, hashFieldEnd, 1, readTS) |
| } | ||
|
|
||
| // Legacy blob path. | ||
| value, err := r.loadHashAt(context.Background(), key, readTS) |
There was a problem hiding this comment.
The request context ctx is available in hdelTxn but context.Background() is passed to r.loadHashAt. Using ctx ensures that request timeouts and cancellations are properly respected during the hash load.
| value, err := r.loadHashAt(context.Background(), key, readTS) | |
| value, err := r.loadHashAt(ctx, key, readTS) |
| } | ||
|
|
||
| readTS := r.readTS() | ||
| typ, err := r.keyTypeAt(context.Background(), key, readTS) |
There was a problem hiding this comment.
The request context ctx is available in rangeList but context.Background() is passed to r.keyTypeAt. Using ctx ensures that request timeouts and cancellations are properly respected during the key type lookup.
| typ, err := r.keyTypeAt(context.Background(), key, readTS) | |
| typ, err := r.keyTypeAt(ctx, key, readTS) |
| return nil, errors.WithStack(err) | ||
| } | ||
|
|
||
| meta, exists, err := r.resolveListMeta(context.Background(), key, readTS) |
There was a problem hiding this comment.
The request context ctx is available in rangeList but context.Background() is passed to r.resolveListMeta. Using ctx ensures that request timeouts and cancellations are properly respected during the list metadata resolution.
| meta, exists, err := r.resolveListMeta(context.Background(), key, readTS) | |
| meta, exists, err := r.resolveListMeta(ctx, key, readTS) |
| return typ, nil | ||
| } | ||
| t.trackTypeReadKeys(key) | ||
| return t.server.keyTypeAt(context.Background(), key, t.startTS) |
There was a problem hiding this comment.
The transaction context t.ctx is available on txnContext but context.Background() is used directly in stagedKeyType. Using t.ctx (with a nil fallback) ensures that the transaction's timeout and cancellation are properly respected during the key type lookup.
| return t.server.keyTypeAt(context.Background(), key, t.startTS) | |
| ctx := t.ctx | |
| if ctx == nil { | |
| ctx = context.Background() | |
| } | |
| return t.server.keyTypeAt(ctx, key, t.startTS) |
| } | ||
|
|
||
| func (t *txnContext) applySet(cmd redcon.Command) (redisResult, error) { | ||
| if isList, err := t.server.isListKeyAt(context.Background(), cmd.Args[1], t.startTS); err != nil { |
There was a problem hiding this comment.
The transaction context t.ctx is available on txnContext but context.Background() is used directly in applySet. Using t.ctx (with a nil fallback) ensures that the transaction's timeout and cancellation are properly respected during the list check.
ctx := t.ctx
if ctx == nil {
ctx = context.Background()
}
if isList, err := t.server.isListKeyAt(ctx, cmd.Args[1], t.startTS); err != nil {| state.value = &expireAt | ||
| state.dirty = true | ||
| if typ == redisTypeString { | ||
| plain, err := t.server.isPlainRedisString(context.Background(), key, t.startTS) |
There was a problem hiding this comment.
The transaction context t.ctx is available on txnContext but context.Background() is used directly in applyPositiveExpire. Using t.ctx (with a nil fallback) ensures that the transaction's timeout and cancellation are properly respected during the plain string check.
ctx := t.ctx
if ctx == nil {
ctx = context.Background()
}
plain, err := t.server.isPlainRedisString(ctx, key, t.startTS)| switch { | ||
| case e < persistedLen: | ||
| return t.server.fetchListRange(context.Background(), key, st.meta, int64(s), int64(e), t.startTS) | ||
| case s >= persistedLen: | ||
| return appendValues(st.appends, s-persistedLen, e-persistedLen), nil | ||
| default: | ||
| head, err := t.server.fetchListRange(context.Background(), key, st.meta, int64(s), int64(persistedLen-1), t.startTS) |
There was a problem hiding this comment.
The transaction context t.ctx is available on txnContext but context.Background() is used directly in listRangeValues. Using t.ctx (with a nil fallback) ensures that the transaction's timeout and cancellation are properly respected during the list range fetch.
| switch { | |
| case e < persistedLen: | |
| return t.server.fetchListRange(context.Background(), key, st.meta, int64(s), int64(e), t.startTS) | |
| case s >= persistedLen: | |
| return appendValues(st.appends, s-persistedLen, e-persistedLen), nil | |
| default: | |
| head, err := t.server.fetchListRange(context.Background(), key, st.meta, int64(s), int64(persistedLen-1), t.startTS) | |
| ctx := t.ctx | |
| if ctx == nil { | |
| ctx = context.Background() | |
| } | |
| switch { | |
| case e < persistedLen: | |
| return t.server.fetchListRange(ctx, key, st.meta, int64(s), int64(e), t.startTS) | |
| case s >= persistedLen: | |
| return appendValues(st.appends, s-persistedLen, e-persistedLen), nil | |
| default: | |
| head, err := t.server.fetchListRange(ctx, key, st.meta, int64(s), int64(persistedLen-1), t.startTS) |
|
|
||
| func (r *RedisServer) quit(conn redcon.Conn, _ redcon.Command) { | ||
| conn.WriteString("OK") | ||
| _ = conn.Close() |
There was a problem hiding this comment.
The error returned by conn.Close() is silently ignored. Network connection close errors should be logged to ensure resource leaks or other cleanup problems are visible.
if err := conn.Close(); err != nil {
log.Printf("failed to close connection: %v", err)
}References
- Do not silently ignore errors from
Close()methods on resources like network connections. Log them to ensure resource leaks or other cleanup problems are visible.
|
@codex review |
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@claude review |
|
@copilot resolve the merge conflicts in this pull request |
Resolved the merge conflicts by merging |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (14)
adapter/redis_keys.go-205-237 (1)
205-237:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPattern matching allows prefix/suffix overlap, causing false positives.
The pattern
a*aincorrectly matches keya. After consuming the prefix"a"(pos=1), the suffix checkHasSuffix(key, "a")passes because the entire key satisfies it, but the prefix and suffix share the same character. Redis glob semantics require*to match zero-or-more characters between distinct prefix and suffix portions.Proposed fix
last := parts[len(parts)-1] - if len(last) > 0 && !bytes.HasSuffix(key, last) { + if len(last) > 0 { + if len(key) < pos+len(last) || !bytes.HasSuffix(key, last) { + return false + } + } - return false - } return true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapter/redis_keys.go` around lines 205 - 237, The matchesAsteriskPattern function has a bug where the suffix check using HasSuffix does not account for prefix/suffix overlap. When checking if the key ends with the suffix portion (the last part after splitting by asterisk), add a validation that the suffix does not overlap with the already-consumed prefix position. Specifically, ensure that before calling HasSuffix for the last part, verify that the position where the suffix would need to start (len(key) - len(last)) is greater than or equal to the current position (pos) to prevent patterns like "a*a" from incorrectly matching keys like "a" where the prefix and suffix overlap.adapter/redis_set_cmds.go-577-577 (1)
577-577:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPropagate
ctxtoloadSetAtinstead ofcontext.Background().The outer context has a timeout but this call ignores it.
Suggested fix
- value, err := r.loadSetAt(context.Background(), hllKind, cmd.Args[1], readTS) + value, err := r.loadSetAt(ctx, hllKind, cmd.Args[1], readTS)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapter/redis_set_cmds.go` at line 577, The loadSetAt function call is using context.Background() which ignores any timeout settings from the outer context. Replace context.Background() with ctx in the loadSetAt call to properly propagate the outer context's timeout and cancellation behavior through the function call.adapter/redis_set_cmds.go-461-484 (1)
461-484:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd timeout to context in
sismember.Other command handlers use
context.WithTimeout(e.g., line 451, 568). This handler usescontext.Background()without a timeout, which could cause unbounded blocking on store operations.Suggested fix
func (r *RedisServer) sismember(conn redcon.Conn, cmd redcon.Command) { if r.proxyToLeader(conn, cmd, cmd.Args[1]) { return } key := cmd.Args[1] member := cmd.Args[2] readTS := r.readTS() - ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), redisDispatchTimeout) + defer cancel() hit, alive, err := r.setMemberFastExists(ctx, key, member, readTS)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapter/redis_set_cmds.go` around lines 461 - 484, The sismember function uses context.Background() without a timeout, which could cause unbounded blocking on store operations, while other command handlers in the codebase use context.WithTimeout. Replace the ctx initialization in the sismember function to use context.WithTimeout instead of context.Background(), applying the same timeout pattern used by other command handlers (referenced at lines 451 and 568). This ensures the context passed to setMemberFastExists and sismemberSlow methods has appropriate timeout protection.adapter/redis_set_cmds.go-534-562 (1)
534-562:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd timeout context to
smembers.This handler makes multiple store calls using
context.Background()without a timeout. For consistency with write handlers and to prevent unbounded blocking, use a timeout-scoped context.Suggested fix
func (r *RedisServer) smembers(conn redcon.Conn, cmd redcon.Command) { if r.proxyToLeader(conn, cmd, cmd.Args[1]) { return } + ctx, cancel := context.WithTimeout(context.Background(), redisDispatchTimeout) + defer cancel() readTS := r.readTS() - typ, err := r.keyTypeAt(context.Background(), cmd.Args[1], readTS) + typ, err := r.keyTypeAt(ctx, cmd.Args[1], readTS) if err != nil { writeRedisError(conn, err) return } ... - value, err := r.loadSetAt(context.Background(), setKind, cmd.Args[1], readTS) + value, err := r.loadSetAt(ctx, setKind, cmd.Args[1], readTS)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapter/redis_set_cmds.go` around lines 534 - 562, The smembers function uses context.Background() without a timeout in both the r.keyTypeAt and r.loadSetAt calls, which can cause unbounded blocking. Create a timeout-scoped context at the beginning of the smembers function (following the pattern used in write handlers) and replace both context.Background() calls with this timeout context to ensure consistency and prevent unbounded blocking operations.adapter/redis_set_cmds.go-30-34 (1)
30-34:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPropagate context instead of using
context.Background().This function is called from within retry loops that have timeout-scoped contexts. Using
context.Background()ignores cancellation signals and timeouts.Suggested fix
-func (r *RedisServer) validateExactSetKind(kind string, key []byte, readTS uint64) error { - typ, err := r.keyTypeAt(context.Background(), key, readTS) +func (r *RedisServer) validateExactSetKind(ctx context.Context, kind string, key []byte, readTS uint64) error { + typ, err := r.keyTypeAt(ctx, key, readTS)Then update callers (lines 186, 211, 573) to pass
ctx.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapter/redis_set_cmds.go` around lines 30 - 34, The validateExactSetKind function is using context.Background() which ignores timeout and cancellation signals from parent contexts in retry loops. Add a context parameter to the validateExactSetKind function signature, replace the context.Background() call with this new parameter when calling r.keyTypeAt(), and then update all three callers of validateExactSetKind (referenced at lines 186, 211, and 573) to pass their own context instead of letting the function create a new one.adapter/redis_expire_cmds.go-222-223 (1)
222-223:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
r.handlerContext()for proper shutdown propagation.Same issue as other handlers in this file.
Suggested fix
- ctx, cancel := context.WithTimeout(context.Background(), redisDispatchTimeout) + ctx, cancel := context.WithTimeout(r.handlerContext(), redisDispatchTimeout)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapter/redis_expire_cmds.go` around lines 222 - 223, The context creation at lines 222-223 uses context.Background() which does not propagate shutdown signals properly. Replace the context.WithTimeout call that uses context.Background() with a call that uses r.handlerContext() instead, while maintaining the same timeout duration (redisDispatchTimeout). This ensures the context respects the handler's lifecycle and proper shutdown propagation similar to other handlers in this file.adapter/redis_expire_cmds.go-94-95 (1)
94-95:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
r.handlerContext()for proper shutdown propagation.Same issue as other handlers in this file.
Suggested fix
- ctx, cancel := context.WithTimeout(context.Background(), redisDispatchTimeout) + ctx, cancel := context.WithTimeout(r.handlerContext(), redisDispatchTimeout)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapter/redis_expire_cmds.go` around lines 94 - 95, The context is being created using context.WithTimeout(context.Background(), redisDispatchTimeout) which does not properly propagate shutdowns. Replace this context creation with r.handlerContext() which is the correct method used by other handlers in this file to ensure proper shutdown propagation and context lifecycle management.adapter/redis_expire_cmds.go-30-31 (1)
30-31:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
r.handlerContext()for proper shutdown propagation.
context.Background()is used here, but other handlers (e.g.,setLegacyin redis_strings.go:265) derive context fromr.handlerContext(). This ensures in-flight requests cancel when the server shuts down.Suggested fix
- ctx, cancel := context.WithTimeout(context.Background(), redisDispatchTimeout) + ctx, cancel := context.WithTimeout(r.handlerContext(), redisDispatchTimeout)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapter/redis_expire_cmds.go` around lines 30 - 31, Replace context.Background() with r.handlerContext() in the context creation at the beginning of the handler to ensure proper shutdown propagation. This aligns with the pattern used in other handlers like setLegacy and ensures in-flight requests are properly cancelled when the server shuts down. The ctx variable will then derive from the handler's context instead of being a detached background context.adapter/redis_expire_cmds.go-180-186 (1)
180-186:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
cockerrors.Wrapinstead offmt.Errorffor error wrapping.Per coding guidelines, errors at boundaries should use
github.com/cockroachdb/errorsfor proper stack traces.Suggested fix
func parseExpireTTL(raw []byte) (int64, error) { ttl, err := strconv.ParseInt(string(raw), 10, 64) if err != nil { - return 0, fmt.Errorf("parse expire ttl: %w", err) + return 0, cockerrors.Wrap(err, "parse expire ttl") } return ttl, nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapter/redis_expire_cmds.go` around lines 180 - 186, In the parseExpireTTL function, replace the fmt.Errorf call with cockerrors.Wrap to ensure proper stack trace handling according to coding guidelines. Import the cockerrors package from github.com/cockroachdb/errors if not already imported, and use cockerrors.Wrap to wrap the error instead of fmt.Errorf with the %w verb.Source: Coding guidelines
adapter/redis_expire_cmds.go-46-47 (1)
46-47:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
r.handlerContext()for proper shutdown propagation.Same issue as
setex- should derive context fromr.handlerContext()for consistency and proper shutdown behavior.Suggested fix
- ctx, cancel := context.WithTimeout(context.Background(), redisDispatchTimeout) + ctx, cancel := context.WithTimeout(r.handlerContext(), redisDispatchTimeout)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapter/redis_expire_cmds.go` around lines 46 - 47, The context initialization at lines 46-47 is creating a new context from context.Background() instead of using the handler's context for proper shutdown propagation. Replace the context.WithTimeout call to derive from r.handlerContext() instead of context.Background(), ensuring consistency with the setex implementation and allowing proper shutdown behavior throughout the handler.adapter/redis_expire_cmds.go-3-16 (1)
3-16:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMixed error package usage violates coding guidelines.
The file imports both standard
errors(line 5) andcockerrors "github.com/cockroachdb/errors"(line 14). Per coding guidelines, errors at boundaries should consistently usegithub.com/cockroachdb/errorsfor proper stack traces and error wrapping.Line 169 uses
errors.New("ERR syntax error")from the standard library.Suggested fix
import ( "context" - "errors" "fmt" "math" "strconv"And update line 169:
- return false, errors.New("ERR syntax error") + return false, cockerrors.New("ERR syntax error")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapter/redis_expire_cmds.go` around lines 3 - 16, The file has mixed error package imports (standard errors and cockerrors) which violates coding guidelines. Remove the standard `errors` import from the import block and replace the `errors.New("ERR syntax error")` call (referenced in the content at line 169) with `cockerrors.New("ERR syntax error")` to ensure consistent use of the cockroachdb errors package for proper stack traces and error wrapping throughout the file.Source: Coding guidelines
adapter/redis_zset_cmds.go-825-860 (1)
825-860:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
loadZSetAtusescontext.Background()instead of the availablectx.Line 845 calls
loadZSetAt(context.Background(), ...)inside the retry callback wherectx(with timeout) is in scope. This prevents proper cancellation if the parent context times out.- value, _, err := r.loadZSetAt(context.Background(), cmd.Args[1], readTS) + value, _, err := r.loadZSetAt(ctx, cmd.Args[1], readTS)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapter/redis_zset_cmds.go` around lines 825 - 860, The zrem method creates a context with timeout using ctx, cancel := context.WithTimeout(...) but then passes context.Background() instead of ctx to the loadZSetAt call on the line that loads the ZSet value. Replace the context.Background() argument in the loadZSetAt call with the ctx variable to ensure proper timeout propagation and cancellation behavior throughout the operation.adapter/redis_zset_cmds.go-862-911 (1)
862-911:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSame
context.Background()issue aszrem.Line 893 should use
ctxinstead ofcontext.Background()for consistent timeout propagation.- value, _, err := r.loadZSetAt(context.Background(), cmd.Args[1], readTS) + value, _, err := r.loadZSetAt(ctx, cmd.Args[1], readTS)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapter/redis_zset_cmds.go` around lines 862 - 911, In the zremrangebyrank function, replace the context.Background() argument in the r.loadZSetAt call with ctx to ensure the timeout created at the start of the function is properly propagated to all operations. The ctx variable with redisDispatchTimeout is already defined earlier in the function and should be used consistently throughout instead of creating a new background context.adapter/redis_zset_cmds.go-920-970 (1)
920-970:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSame
context.Background()issue inloadZSetAtcall.Line 943 should use
ctxfor timeout propagation.- value, _, err := r.loadZSetAt(context.Background(), key, readTS) + value, _, err := r.loadZSetAt(ctx, key, readTS)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapter/redis_zset_cmds.go` around lines 920 - 970, In the tryBZPopMinWithMode function, the call to r.loadZSetAt is using context.Background() instead of the ctx variable that was created with a timeout at the beginning of the function. Replace context.Background() with ctx in the r.loadZSetAt call to ensure proper timeout propagation throughout the operation.
🧹 Nitpick comments (6)
adapter/redis_server_cmds.go (2)
490-495: ⚡ Quick winUse
slogfor structured logging per coding guidelines.The
log.Printfcall should be replaced withslogfor consistency with the project's structured logging requirements.Suggested fix
func (r *RedisServer) quit(conn redcon.Conn, _ redcon.Command) { conn.WriteString("OK") if err := conn.Close(); err != nil { - log.Printf("redis quit close failed: %v", err) + slog.Error("redis quit close failed", "error", err) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapter/redis_server_cmds.go` around lines 490 - 495, In the quit method of the RedisServer type, replace the log.Printf call with slog for structured logging to maintain consistency with the project's logging standards. Use the appropriate slog function (such as slog.Error) to log the error when conn.Close() fails, ensuring the error information is preserved in the structured log output.Source: Coding guidelines
571-577: ⚡ Quick winUse
slogfor trace logging per coding guidelines.Similar to the
quitfunction, trace logging should useslogwith appropriate structured keys.Suggested fix
func (r *RedisServer) publish(conn redcon.Conn, cmd redcon.Command) { count := r.publishCluster(context.Background(), cmd.Args[1], cmd.Args[2]) if r.traceCommands { - log.Printf("redis trace publish remote=%s channel=%q subscribers=%d", conn.RemoteAddr(), string(cmd.Args[1]), count) + slog.Debug("redis trace publish", "remote", conn.RemoteAddr(), "channel", string(cmd.Args[1]), "subscribers", count) } conn.WriteInt64(count) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapter/redis_server_cmds.go` around lines 571 - 577, In the publish method of the RedisServer type, replace the log.Printf call used for trace logging with structured logging using slog. Extract the trace message components (remote address from conn.RemoteAddr(), channel from cmd.Args[1], and subscriber count) as structured key-value pairs to pass to slog, following the same pattern used in the quit function as referenced in the guidelines.Source: Coding guidelines
adapter/redis_proxy_leader.go (1)
226-245: 💤 Low valueCached Redis clients are never closed, risking connection leaks.
When leadership changes, stale clients for old leader addresses remain in the cache indefinitely. Over time, this can exhaust connection pools or file descriptors. Consider adding a cleanup mechanism (e.g., periodic eviction of unused clients, or replacing the cache entry when a new client is created for the same address after a leadership change).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapter/redis_proxy_leader.go` around lines 226 - 245, The getOrCreateLeaderClient method caches Redis clients indefinitely without ever closing them, leading to connection leaks when leadership changes. Add a cleanup mechanism to close and remove stale clients from the r.leaderClients cache when they are no longer needed, such as closing the old client before storing a new one for the same address, or implementing a separate method to explicitly close and evict clients from the cache when leadership transitions occur. Ensure the cleanup respects the existing mutex synchronization (r.leaderClientsMu) to prevent race conditions.adapter/redis_stream_cmds.go (1)
1143-1165: ⚡ Quick winRead operations use
context.Background()with no timeout.Multiple store calls (
keyTypeAtExpect,loadStreamMetaAt,loadStreamAt) usecontext.Background(), which means these reads have no timeout and won't abort during graceful shutdown. While less critical than write commands, this is inconsistent withxreadwhich properly derives context fromr.handlerContext().Consider wrapping these in a context with timeout for consistency and to prevent slow reads from hanging indefinitely.
Suggested approach
func (r *RedisServer) xlen(conn redcon.Conn, cmd redcon.Command) { if r.proxyToLeader(conn, cmd, cmd.Args[1]) { return } + ctx, cancel := context.WithTimeout(r.handlerContext(), redisDispatchTimeout) + defer cancel() readTS := r.readTS() - typ, err := r.keyTypeAtExpect(context.Background(), cmd.Args[1], readTS, redisTypeStream) + typ, err := r.keyTypeAtExpect(ctx, cmd.Args[1], readTS, redisTypeStream) // ... apply same pattern to other context.Background() calls🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapter/redis_stream_cmds.go` around lines 1143 - 1165, The read operations in this stream command handler are using context.Background() which provides no timeout for the keyTypeAtExpect, loadStreamMetaAt, and loadStreamAt calls, potentially allowing reads to hang indefinitely and making it inconsistent with how other commands like xread derive their context. Replace all instances of context.Background() in this code block with a context derived from r.handlerContext() to ensure these read operations have proper timeout handling and can be cancelled during graceful shutdown, maintaining consistency with the rest of the codebase.adapter/redis_set_cmds.go (1)
46-52: ⚡ Quick winUse
cockerrors.Wrapfor error wrapping consistency.The coding guidelines require using
github.com/cockroachdb/errorsfor error wrapping. This function usesfmt.Errorfwhile the rest of the file usescockerrors.Suggested fix
func (r *RedisServer) hllExistsAt(key []byte, readTS uint64) (bool, error) { exists, err := r.store.ExistsAt(context.Background(), redisHLLKey(key), readTS) if err != nil { - return false, fmt.Errorf("exists hll: %w", err) + return false, cockerrors.Wrap(err, "exists hll") } return exists, nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapter/redis_set_cmds.go` around lines 46 - 52, In the hllExistsAt function, replace the fmt.Errorf call with cockerrors.Wrap to maintain consistency with error wrapping practices in the rest of the file. The cockerrors.Wrap function should be called with the error returned from r.store.ExistsAt and a descriptive error message like "exists hll" to provide proper error context.Source: Coding guidelines
adapter/redis_zset_cmds.go (1)
792-823: 💤 Low valueConsider propagating a timeout context for read operations.
zrangeReadusescontext.Background()for bothkeyTypeAtExpect(line 794) andloadZSetAt(line 808), unlike write handlers that create timeout contexts. This could cause the operation to hang indefinitely on a slow or unresponsive store.Given that the PR objectives mention context propagation changes, this may warrant verification.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@adapter/redis_zset_cmds.go` around lines 792 - 823, The zrangeRead function uses context.Background() for both the keyTypeAtExpect and loadZSetAt operations, which lacks timeout protection unlike write handlers. Create a timeout context at the beginning of the zrangeRead function (similar to how write handlers handle context creation) and pass that timeout context to both the keyTypeAtExpect call and the loadZSetAt call instead of context.Background() to ensure operations cannot hang indefinitely.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@adapter/redis_hash_cmds.go`:
- Around line 848-856: The keyTypeAt function call on line 850 uses
context.Background() instead of the ctx parameter that has the
redisDispatchTimeout configured, which bypasses timeout propagation. Replace
context.Background() with ctx in the keyTypeAt function call to ensure the
timeout context is properly passed through to the store operation.
- Around line 75-150: The functions buildSetLegacyMigrationElems and
buildZSetLegacyMigrationElems are defined in redis_hash_cmds.go but are only
used in redis_set_cmds.go and redis_zset_cmds.go respectively, violating proper
code organization. Move buildSetLegacyMigrationElems to redis_set_cmds.go and
buildZSetLegacyMigrationElems to redis_zset_cmds.go. Additionally, on line 126
where the nolint:mnd directive appears in the capacity calculation for the elems
slice in buildZSetLegacyMigrationElems, replace the //nolint:mnd comment with a
named constant to clarify why 2 operations are needed per entry (one for member
key and one for score index key), and update all references to use this constant
instead of magic numbers.
In `@adapter/redis_keys.go`:
- Around line 14-16: The keys method in the RedisServer type accesses
cmd.Args[1] without validating that the cmd.Args slice contains at least 2
elements, which will cause a panic when a KEYS command is sent without the
required pattern argument. Add a length check before accessing cmd.Args[1] to
verify that len(cmd.Args) >= 2, and if the validation fails, use conn to send an
appropriate error response back to the client indicating that the pattern
argument is missing.
In `@adapter/redis_lists.go`:
- Around line 714-726: The listPushCmd function uses context.Background()
without timeouts in two places, which is inconsistent with other handlers like
lrange and risks indefinite hangs without parent context information. Replace
the context.Background() call in the r.keyTypeAt() method with
context.WithTimeout(r.handlerContext(), redisDispatchTimeout), and similarly
replace the ctx := context.Background() line with the same timeout context
pattern before passing it to pushFn(). This ensures consistency with other
handlers and provides proper timeout and context propagation.
- Around line 811-825: The lindex command handler uses context.Background()
without a timeout in the calls to r.keyTypeAt() and r.listValuesAt() methods,
which can lead to hung operations. Replace both context.Background() calls (at
lines 812 and 825) with a properly timeout-protected context by using
context.WithTimeout(), following the same pattern that was applied to fix
listPushCmd. This ensures that both the type check and value retrieval
operations have proper timeout protection.
In `@adapter/redis_set_cmds.go`:
- Around line 599-632: The pfcount function lacks a timeout-scoped context for
its store operations. After the proxyToLeader check, add a context with timeout
using `ctx, cancel := context.WithTimeout(context.Background(),
redisDispatchTimeout)` and defer the cancel call (following the pattern used in
pfadd), then replace all `context.Background()` calls within the loop for the
keyTypeAt, ExistsAt, and loadSetAt calls with this new ctx variable.
Additionally, verify whether the multi-key sharding assumption is correct since
proxyToLeader only examines cmd.Args[1] while pfcount iterates over all keys in
cmd.Args[1:]; confirm whether all keys in a single command are guaranteed to be
co-located on the same shard or if per-key routing is required.
In `@adapter/redis_stream_cmds.go`:
- Around line 215-216: Replace context.Background() with r.handlerContext() in
the context.WithTimeout calls within both the xadd handler (at lines 215-216)
and the xtrim handler (at line 546) to ensure these Redis write commands respect
server shutdown signals. This aligns with how the xread handler properly uses
r.handlerContext() at line 1052 and completes the "propagate Redis command
contexts" work. The change ensures operations will be properly cancelled during
graceful shutdown instead of potentially outliving the server's drain window.
In `@adapter/redis_zset_cmds.go`:
- Around line 849-854: The ZREM operation removes members from the zset but only
persists the remaining entries to the legacy blob key via persistZSetEntriesTxn,
leaving the wide-column ZSetMemberKey and ZSetScoreKey entries for removed
members untouched. This causes the removed members to be reloaded on the next
read. To fix this, after calling removeZSetMembers to identify which members
were removed, explicitly delete the ZSetMemberKey and ZSetScoreKey wide-column
entries for each removed member (following the same pattern used in
persistBZPopMinResult for lines 980-998) before or in conjunction with the
persistZSetEntriesTxn call.
---
Minor comments:
In `@adapter/redis_expire_cmds.go`:
- Around line 222-223: The context creation at lines 222-223 uses
context.Background() which does not propagate shutdown signals properly. Replace
the context.WithTimeout call that uses context.Background() with a call that
uses r.handlerContext() instead, while maintaining the same timeout duration
(redisDispatchTimeout). This ensures the context respects the handler's
lifecycle and proper shutdown propagation similar to other handlers in this
file.
- Around line 94-95: The context is being created using
context.WithTimeout(context.Background(), redisDispatchTimeout) which does not
properly propagate shutdowns. Replace this context creation with
r.handlerContext() which is the correct method used by other handlers in this
file to ensure proper shutdown propagation and context lifecycle management.
- Around line 30-31: Replace context.Background() with r.handlerContext() in the
context creation at the beginning of the handler to ensure proper shutdown
propagation. This aligns with the pattern used in other handlers like setLegacy
and ensures in-flight requests are properly cancelled when the server shuts
down. The ctx variable will then derive from the handler's context instead of
being a detached background context.
- Around line 180-186: In the parseExpireTTL function, replace the fmt.Errorf
call with cockerrors.Wrap to ensure proper stack trace handling according to
coding guidelines. Import the cockerrors package from
github.com/cockroachdb/errors if not already imported, and use cockerrors.Wrap
to wrap the error instead of fmt.Errorf with the %w verb.
- Around line 46-47: The context initialization at lines 46-47 is creating a new
context from context.Background() instead of using the handler's context for
proper shutdown propagation. Replace the context.WithTimeout call to derive from
r.handlerContext() instead of context.Background(), ensuring consistency with
the setex implementation and allowing proper shutdown behavior throughout the
handler.
- Around line 3-16: The file has mixed error package imports (standard errors
and cockerrors) which violates coding guidelines. Remove the standard `errors`
import from the import block and replace the `errors.New("ERR syntax error")`
call (referenced in the content at line 169) with `cockerrors.New("ERR syntax
error")` to ensure consistent use of the cockroachdb errors package for proper
stack traces and error wrapping throughout the file.
In `@adapter/redis_keys.go`:
- Around line 205-237: The matchesAsteriskPattern function has a bug where the
suffix check using HasSuffix does not account for prefix/suffix overlap. When
checking if the key ends with the suffix portion (the last part after splitting
by asterisk), add a validation that the suffix does not overlap with the
already-consumed prefix position. Specifically, ensure that before calling
HasSuffix for the last part, verify that the position where the suffix would
need to start (len(key) - len(last)) is greater than or equal to the current
position (pos) to prevent patterns like "a*a" from incorrectly matching keys
like "a" where the prefix and suffix overlap.
In `@adapter/redis_set_cmds.go`:
- Line 577: The loadSetAt function call is using context.Background() which
ignores any timeout settings from the outer context. Replace
context.Background() with ctx in the loadSetAt call to properly propagate the
outer context's timeout and cancellation behavior through the function call.
- Around line 461-484: The sismember function uses context.Background() without
a timeout, which could cause unbounded blocking on store operations, while other
command handlers in the codebase use context.WithTimeout. Replace the ctx
initialization in the sismember function to use context.WithTimeout instead of
context.Background(), applying the same timeout pattern used by other command
handlers (referenced at lines 451 and 568). This ensures the context passed to
setMemberFastExists and sismemberSlow methods has appropriate timeout
protection.
- Around line 534-562: The smembers function uses context.Background() without a
timeout in both the r.keyTypeAt and r.loadSetAt calls, which can cause unbounded
blocking. Create a timeout-scoped context at the beginning of the smembers
function (following the pattern used in write handlers) and replace both
context.Background() calls with this timeout context to ensure consistency and
prevent unbounded blocking operations.
- Around line 30-34: The validateExactSetKind function is using
context.Background() which ignores timeout and cancellation signals from parent
contexts in retry loops. Add a context parameter to the validateExactSetKind
function signature, replace the context.Background() call with this new
parameter when calling r.keyTypeAt(), and then update all three callers of
validateExactSetKind (referenced at lines 186, 211, and 573) to pass their own
context instead of letting the function create a new one.
In `@adapter/redis_zset_cmds.go`:
- Around line 825-860: The zrem method creates a context with timeout using ctx,
cancel := context.WithTimeout(...) but then passes context.Background() instead
of ctx to the loadZSetAt call on the line that loads the ZSet value. Replace the
context.Background() argument in the loadZSetAt call with the ctx variable to
ensure proper timeout propagation and cancellation behavior throughout the
operation.
- Around line 862-911: In the zremrangebyrank function, replace the
context.Background() argument in the r.loadZSetAt call with ctx to ensure the
timeout created at the start of the function is properly propagated to all
operations. The ctx variable with redisDispatchTimeout is already defined
earlier in the function and should be used consistently throughout instead of
creating a new background context.
- Around line 920-970: In the tryBZPopMinWithMode function, the call to
r.loadZSetAt is using context.Background() instead of the ctx variable that was
created with a timeout at the beginning of the function. Replace
context.Background() with ctx in the r.loadZSetAt call to ensure proper timeout
propagation throughout the operation.
---
Nitpick comments:
In `@adapter/redis_proxy_leader.go`:
- Around line 226-245: The getOrCreateLeaderClient method caches Redis clients
indefinitely without ever closing them, leading to connection leaks when
leadership changes. Add a cleanup mechanism to close and remove stale clients
from the r.leaderClients cache when they are no longer needed, such as closing
the old client before storing a new one for the same address, or implementing a
separate method to explicitly close and evict clients from the cache when
leadership transitions occur. Ensure the cleanup respects the existing mutex
synchronization (r.leaderClientsMu) to prevent race conditions.
In `@adapter/redis_server_cmds.go`:
- Around line 490-495: In the quit method of the RedisServer type, replace the
log.Printf call with slog for structured logging to maintain consistency with
the project's logging standards. Use the appropriate slog function (such as
slog.Error) to log the error when conn.Close() fails, ensuring the error
information is preserved in the structured log output.
- Around line 571-577: In the publish method of the RedisServer type, replace
the log.Printf call used for trace logging with structured logging using slog.
Extract the trace message components (remote address from conn.RemoteAddr(),
channel from cmd.Args[1], and subscriber count) as structured key-value pairs to
pass to slog, following the same pattern used in the quit function as referenced
in the guidelines.
In `@adapter/redis_set_cmds.go`:
- Around line 46-52: In the hllExistsAt function, replace the fmt.Errorf call
with cockerrors.Wrap to maintain consistency with error wrapping practices in
the rest of the file. The cockerrors.Wrap function should be called with the
error returned from r.store.ExistsAt and a descriptive error message like
"exists hll" to provide proper error context.
In `@adapter/redis_stream_cmds.go`:
- Around line 1143-1165: The read operations in this stream command handler are
using context.Background() which provides no timeout for the keyTypeAtExpect,
loadStreamMetaAt, and loadStreamAt calls, potentially allowing reads to hang
indefinitely and making it inconsistent with how other commands like xread
derive their context. Replace all instances of context.Background() in this code
block with a context derived from r.handlerContext() to ensure these read
operations have proper timeout handling and can be cancelled during graceful
shutdown, maintaining consistency with the rest of the codebase.
In `@adapter/redis_zset_cmds.go`:
- Around line 792-823: The zrangeRead function uses context.Background() for
both the keyTypeAtExpect and loadZSetAt operations, which lacks timeout
protection unlike write handlers. Create a timeout context at the beginning of
the zrangeRead function (similar to how write handlers handle context creation)
and pass that timeout context to both the keyTypeAtExpect call and the
loadZSetAt call instead of context.Background() to ensure operations cannot hang
indefinitely.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4205ad0d-e44c-488c-ac90-aaa093ddf6d2
📒 Files selected for processing (13)
adapter/redis.goadapter/redis_compat_commands.goadapter/redis_expire_cmds.goadapter/redis_hash_cmds.goadapter/redis_keys.goadapter/redis_lists.goadapter/redis_proxy_leader.goadapter/redis_server_cmds.goadapter/redis_set_cmds.goadapter/redis_stream_cmds.goadapter/redis_strings.goadapter/redis_txn.goadapter/redis_zset_cmds.go
| // buildSetLegacyMigrationElems returns ops that atomically migrate a legacy | ||
| // !redis|set| blob to wide-column !st|mem| keys. Returns nil if no legacy | ||
| // blob exists. | ||
| func (r *RedisServer) buildSetLegacyMigrationElems(ctx context.Context, key []byte, readTS uint64) ([]*kv.Elem[kv.OP], error) { | ||
| raw, err := r.store.GetAt(ctx, redisSetKey(key), readTS) | ||
| if cockerrors.Is(err, store.ErrKeyNotFound) { | ||
| return nil, nil | ||
| } | ||
| if err != nil { | ||
| return nil, cockerrors.WithStack(err) | ||
| } | ||
| value, err := unmarshalSetValue(raw) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| elems := make([]*kv.Elem[kv.OP], 0, len(value.Members)+setWideColOverhead) | ||
| for _, member := range value.Members { | ||
| elems = append(elems, &kv.Elem[kv.OP]{ | ||
| Op: kv.Put, | ||
| Key: store.SetMemberKey(key, []byte(member)), | ||
| Value: []byte{}, | ||
| }) | ||
| } | ||
| // Delete the legacy blob. | ||
| elems = append(elems, &kv.Elem[kv.OP]{Op: kv.Del, Key: redisSetKey(key)}) | ||
| // Write a base meta so that resolveSetMeta starts from an accurate count. | ||
| elems = append(elems, &kv.Elem[kv.OP]{ | ||
| Op: kv.Put, | ||
| Key: store.SetMetaKey(key), | ||
| Value: store.MarshalSetMeta(store.SetMeta{Len: int64(len(value.Members))}), | ||
| }) | ||
| return elems, nil | ||
| } | ||
|
|
||
| // buildZSetLegacyMigrationElems returns ops that atomically migrate a legacy | ||
| // !redis|zset| blob to wide-column !zs|mem| + !zs|scr| keys. Returns nil if no legacy | ||
| // blob exists. The base meta key is also written with the migrated count so | ||
| // that resolveZSetMeta works correctly after migration. | ||
| func (r *RedisServer) buildZSetLegacyMigrationElems(ctx context.Context, key []byte, readTS uint64) ([]*kv.Elem[kv.OP], error) { | ||
| raw, err := r.store.GetAt(ctx, redisZSetKey(key), readTS) | ||
| if cockerrors.Is(err, store.ErrKeyNotFound) { | ||
| return nil, nil | ||
| } | ||
| if err != nil { | ||
| return nil, cockerrors.WithStack(err) | ||
| } | ||
| value, err := unmarshalZSetValue(raw) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| // Each entry → member key + score index key; plus legacy blob deletion + base meta. | ||
| elems := make([]*kv.Elem[kv.OP], 0, len(value.Entries)*2+setWideColOverhead) //nolint:mnd // 2 ops per entry (member + score index) | ||
| for _, entry := range value.Entries { | ||
| elems = append(elems, | ||
| &kv.Elem[kv.OP]{ | ||
| Op: kv.Put, | ||
| Key: store.ZSetMemberKey(key, []byte(entry.Member)), | ||
| Value: store.MarshalZSetScore(entry.Score), | ||
| }, | ||
| &kv.Elem[kv.OP]{ | ||
| Op: kv.Put, | ||
| Key: store.ZSetScoreKey(key, entry.Score, []byte(entry.Member)), | ||
| Value: []byte{}, | ||
| }, | ||
| ) | ||
| } | ||
| // Delete the legacy blob. | ||
| elems = append(elems, &kv.Elem[kv.OP]{Op: kv.Del, Key: redisZSetKey(key)}) | ||
| // Write a base meta so that resolveZSetMeta starts from an accurate count. | ||
| elems = append(elems, &kv.Elem[kv.OP]{ | ||
| Op: kv.Put, | ||
| Key: store.ZSetMetaKey(key), | ||
| Value: store.MarshalZSetMeta(store.ZSetMeta{Len: int64(len(value.Entries))}), | ||
| }) | ||
| return elems, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify these functions are called from other files, not this one
echo "=== Checking callers of buildSetLegacyMigrationElems ==="
rg -n 'buildSetLegacyMigrationElems' --type=go
echo ""
echo "=== Checking callers of buildZSetLegacyMigrationElems ==="
rg -n 'buildZSetLegacyMigrationElems' --type=go
echo ""
echo "=== Checking callers of buildLegacySetMemberBase ==="
rg -n 'buildLegacySetMemberBase' --type=goRepository: bootjp/elastickv
Length of output: 1417
🏁 Script executed:
#!/bin/bash
# Check exact context around line 126 in redis_hash_cmds.go
echo "=== Context around line 126 in redis_hash_cmds.go ==="
sed -n '120,135n' adapter/redis_hash_cmds.go
echo ""
echo "=== File structure check: which functions are defined in redis_hash_cmds.go ==="
rg -n '^\s*func \(r \*RedisServer\)' adapter/redis_hash_cmds.go | head -20Repository: bootjp/elastickv
Length of output: 2374
🏁 Script executed:
#!/bin/bash
# Get actual file content to see the exact line numbers and nolint directive
echo "=== Full content of redis_hash_cmds.go around buildZSetLegacyMigrationElems ==="
wc -l adapter/redis_hash_cmds.go
echo ""
echo "=== Lines 110-140 of redis_hash_cmds.go ==="
sed -n '110,140p' adapter/redis_hash_cmds.go
echo ""
echo "=== Search for //nolint in the file ==="
rg -n 'nolint' adapter/redis_hash_cmds.goRepository: bootjp/elastickv
Length of output: 1587
Move SET and ZSET migration helpers to their respective domain files and remove //nolint directive.
buildSetLegacyMigrationElems, buildZSetLegacyMigrationElems, and buildLegacySetMemberBase are defined in redis_hash_cmds.go (lines 75–181) but are only called from redis_set_cmds.go and redis_zset_cmds.go. These functions should be moved to their respective domain files where they are used.
Additionally, line 126 uses //nolint:mnd which violates coding guidelines. Replace with a named constant:
Refactoring suggestion
+const zsetOpsPerEntry = 2 // member key + score index key
+
func (r *RedisServer) buildZSetLegacyMigrationElems(...) ([]*kv.Elem[kv.OP], error) {
...
- elems := make([]*kv.Elem[kv.OP], 0, len(value.Entries)*2+setWideColOverhead) //nolint:mnd // 2 ops per entry (member + score index)
+ elems := make([]*kv.Elem[kv.OP], 0, len(value.Entries)*zsetOpsPerEntry+setWideColOverhead)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@adapter/redis_hash_cmds.go` around lines 75 - 150, The functions
buildSetLegacyMigrationElems and buildZSetLegacyMigrationElems are defined in
redis_hash_cmds.go but are only used in redis_set_cmds.go and redis_zset_cmds.go
respectively, violating proper code organization. Move
buildSetLegacyMigrationElems to redis_set_cmds.go and
buildZSetLegacyMigrationElems to redis_zset_cmds.go. Additionally, on line 126
where the nolint:mnd directive appears in the capacity calculation for the elems
slice in buildZSetLegacyMigrationElems, replace the //nolint:mnd comment with a
named constant to clarify why 2 operations are needed per entry (one for member
key and one for score index key), and update all references to use this constant
instead of magic numbers.
Source: Coding guidelines
| if err := r.retryRedisWrite(ctx, func() error { | ||
| readTS := r.readTS() | ||
| typ, err := r.keyTypeAt(context.Background(), cmd.Args[1], readTS) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if typ != redisTypeNone && typ != redisTypeString { | ||
| return wrongTypeError() | ||
| } |
There was a problem hiding this comment.
context.Background() bypasses the timeout context in incr.
Line 850 uses context.Background() for keyTypeAt, but the outer function creates ctx with redisDispatchTimeout. This bypasses timeout propagation — if the store operation hangs, the timeout won't cancel it.
Proposed fix
if err := r.retryRedisWrite(ctx, func() error {
readTS := r.readTS()
- typ, err := r.keyTypeAt(context.Background(), cmd.Args[1], readTS)
+ typ, err := r.keyTypeAt(ctx, cmd.Args[1], readTS)
if err != nil {
return err
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if err := r.retryRedisWrite(ctx, func() error { | |
| readTS := r.readTS() | |
| typ, err := r.keyTypeAt(context.Background(), cmd.Args[1], readTS) | |
| if err != nil { | |
| return err | |
| } | |
| if typ != redisTypeNone && typ != redisTypeString { | |
| return wrongTypeError() | |
| } | |
| if err := r.retryRedisWrite(ctx, func() error { | |
| readTS := r.readTS() | |
| typ, err := r.keyTypeAt(ctx, cmd.Args[1], readTS) | |
| if err != nil { | |
| return err | |
| } | |
| if typ != redisTypeNone && typ != redisTypeString { | |
| return wrongTypeError() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@adapter/redis_hash_cmds.go` around lines 848 - 856, The keyTypeAt function
call on line 850 uses context.Background() instead of the ctx parameter that has
the redisDispatchTimeout configured, which bypasses timeout propagation. Replace
context.Background() with ctx in the keyTypeAt function call to ensure the
timeout context is properly passed through to the store operation.
| func (r *RedisServer) keys(conn redcon.Conn, cmd redcon.Command) { | ||
| pattern := cmd.Args[1] | ||
|
|
There was a problem hiding this comment.
Missing argument count validation causes panic on malformed command.
Accessing cmd.Args[1] without verifying len(cmd.Args) >= 2 will panic if a client sends KEYS without a pattern argument.
Proposed fix
func (r *RedisServer) keys(conn redcon.Conn, cmd redcon.Command) {
+ if len(cmd.Args) < 2 {
+ conn.WriteError("ERR wrong number of arguments for 'keys' command")
+ return
+ }
pattern := cmd.Args[1]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (r *RedisServer) keys(conn redcon.Conn, cmd redcon.Command) { | |
| pattern := cmd.Args[1] | |
| func (r *RedisServer) keys(conn redcon.Conn, cmd redcon.Command) { | |
| if len(cmd.Args) < 2 { | |
| conn.WriteError("ERR wrong number of arguments for 'keys' command") | |
| return | |
| } | |
| pattern := cmd.Args[1] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@adapter/redis_keys.go` around lines 14 - 16, The keys method in the
RedisServer type accesses cmd.Args[1] without validating that the cmd.Args slice
contains at least 2 elements, which will cause a panic when a KEYS command is
sent without the required pattern argument. Add a length check before accessing
cmd.Args[1] to verify that len(cmd.Args) >= 2, and if the validation fails, use
conn to send an appropriate error response back to the client indicating that
the pattern argument is missing.
| readTS := r.readTS() | ||
| typ, err := r.keyTypeAt(context.Background(), key, readTS) | ||
| if err != nil { | ||
| writeRedisError(conn, err) | ||
| return | ||
| } | ||
| if typ != redisTypeNone && typ != redisTypeList { | ||
| conn.WriteError(wrongTypeMessage) | ||
| return | ||
| } | ||
|
|
||
| ctx := context.Background() | ||
| length, err := pushFn(ctx, key, cmd.Args[2:]) |
There was a problem hiding this comment.
Missing timeout context in listPushCmd.
Lines 715 and 725 use context.Background() without a timeout. This is inconsistent with other handlers like lrange (line 740) that use context.WithTimeout(r.handlerContext(), redisDispatchTimeout).
Without a timeout, operations could hang indefinitely if the store or coordinator blocks. Additionally, using context.Background() loses any parent context information (tracing, cancellation signals) that r.handlerContext() would provide.
Proposed fix
func (r *RedisServer) listPushCmd(conn redcon.Conn, cmd redcon.Command, pushFn listPushFunc, proxyFn listProxyFunc) {
key := cmd.Args[1]
if !r.coordinator.IsLeaderForKey(key) {
length, err := proxyFn(key, cmd.Args[2:])
if err != nil {
writeRedisError(conn, err)
return
}
conn.WriteInt64(length)
return
}
+ ctx, cancel := context.WithTimeout(r.handlerContext(), redisDispatchTimeout)
+ defer cancel()
+
readTS := r.readTS()
- typ, err := r.keyTypeAt(context.Background(), key, readTS)
+ typ, err := r.keyTypeAt(ctx, key, readTS)
if err != nil {
writeRedisError(conn, err)
return
}
if typ != redisTypeNone && typ != redisTypeList {
conn.WriteError(wrongTypeMessage)
return
}
- ctx := context.Background()
length, err := pushFn(ctx, key, cmd.Args[2:])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| readTS := r.readTS() | |
| typ, err := r.keyTypeAt(context.Background(), key, readTS) | |
| if err != nil { | |
| writeRedisError(conn, err) | |
| return | |
| } | |
| if typ != redisTypeNone && typ != redisTypeList { | |
| conn.WriteError(wrongTypeMessage) | |
| return | |
| } | |
| ctx := context.Background() | |
| length, err := pushFn(ctx, key, cmd.Args[2:]) | |
| ctx, cancel := context.WithTimeout(r.handlerContext(), redisDispatchTimeout) | |
| defer cancel() | |
| readTS := r.readTS() | |
| typ, err := r.keyTypeAt(ctx, key, readTS) | |
| if err != nil { | |
| writeRedisError(conn, err) | |
| return | |
| } | |
| if typ != redisTypeNone && typ != redisTypeList { | |
| conn.WriteError(wrongTypeMessage) | |
| return | |
| } | |
| length, err := pushFn(ctx, key, cmd.Args[2:]) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@adapter/redis_lists.go` around lines 714 - 726, The listPushCmd function uses
context.Background() without timeouts in two places, which is inconsistent with
other handlers like lrange and risks indefinite hangs without parent context
information. Replace the context.Background() call in the r.keyTypeAt() method
with context.WithTimeout(r.handlerContext(), redisDispatchTimeout), and
similarly replace the ctx := context.Background() line with the same timeout
context pattern before passing it to pushFn(). This ensures consistency with
other handlers and provides proper timeout and context propagation.
| readTS := r.readTS() | ||
| typ, err := r.keyTypeAt(context.Background(), cmd.Args[1], readTS) | ||
| if err != nil { | ||
| writeRedisError(conn, err) | ||
| return | ||
| } | ||
| if typ == redisTypeNone { | ||
| conn.WriteNull() | ||
| return | ||
| } | ||
| if typ != redisTypeList { | ||
| conn.WriteError(wrongTypeMessage) | ||
| return | ||
| } | ||
| values, err := r.listValuesAt(context.Background(), cmd.Args[1], readTS) |
There was a problem hiding this comment.
Missing timeout context in lindex.
Lines 812 and 825 use context.Background() without a timeout. This is the same inconsistency as in listPushCmd and could lead to hung operations.
Proposed fix
func (r *RedisServer) lindex(conn redcon.Conn, cmd redcon.Command) {
if r.proxyToLeader(conn, cmd, cmd.Args[1]) {
return
}
index, err := parseInt(cmd.Args[2])
if err != nil {
writeRedisError(conn, err)
return
}
+ ctx, cancel := context.WithTimeout(r.handlerContext(), redisDispatchTimeout)
+ defer cancel()
+
readTS := r.readTS()
- typ, err := r.keyTypeAt(context.Background(), cmd.Args[1], readTS)
+ typ, err := r.keyTypeAt(ctx, cmd.Args[1], readTS)
if err != nil {
writeRedisError(conn, err)
return
}
if typ == redisTypeNone {
conn.WriteNull()
return
}
if typ != redisTypeList {
conn.WriteError(wrongTypeMessage)
return
}
- values, err := r.listValuesAt(context.Background(), cmd.Args[1], readTS)
+ values, err := r.listValuesAt(ctx, cmd.Args[1], readTS)
if err != nil {
writeRedisError(conn, err)
return🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@adapter/redis_lists.go` around lines 811 - 825, The lindex command handler
uses context.Background() without a timeout in the calls to r.keyTypeAt() and
r.listValuesAt() methods, which can lead to hung operations. Replace both
context.Background() calls (at lines 812 and 825) with a properly
timeout-protected context by using context.WithTimeout(), following the same
pattern that was applied to fix listPushCmd. This ensures that both the type
check and value retrieval operations have proper timeout protection.
| func (r *RedisServer) pfcount(conn redcon.Conn, cmd redcon.Command) { | ||
| if r.proxyToLeader(conn, cmd, cmd.Args[1]) { | ||
| return | ||
| } | ||
| readTS := r.readTS() | ||
| union := map[string]struct{}{} | ||
| for _, key := range cmd.Args[1:] { | ||
| typ, err := r.keyTypeAt(context.Background(), key, readTS) | ||
| if err != nil { | ||
| writeRedisError(conn, err) | ||
| return | ||
| } | ||
| if typ != redisTypeNone { | ||
| hllExists, err := r.store.ExistsAt(context.Background(), redisHLLKey(key), readTS) | ||
| if err != nil { | ||
| writeRedisError(conn, err) | ||
| return | ||
| } | ||
| if !hllExists { | ||
| conn.WriteError(wrongTypeMessage) | ||
| return | ||
| } | ||
| } | ||
| value, err := r.loadSetAt(context.Background(), hllKind, key, readTS) | ||
| if err != nil { | ||
| writeRedisError(conn, err) | ||
| return | ||
| } | ||
| for _, member := range value.Members { | ||
| union[member] = struct{}{} | ||
| } | ||
| } | ||
| conn.WriteInt(len(union)) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how other multi-key commands handle leader proxying
rg -n "proxyToLeader" --type=go -A2 -B2 | head -80Repository: bootjp/elastickv
Length of output: 4029
🏁 Script executed:
#!/bin/bash
# 1. Check if PFCOUNT actually accepts multiple keys and how it's used
echo "=== PFCOUNT command registration and tests ==="
rg -i "pfcount|pf" --type=go | head -30
echo ""
echo "=== Look for test cases or command definitions ==="
rg -n "pfcount" --type=go -B3 -A10Repository: bootjp/elastickv
Length of output: 6756
🏁 Script executed:
#!/bin/bash
# 2. Check for other multi-key commands and how they route
echo "=== Look for multi-key commands like mget, mset, sunion ==="
rg -n "func.*\(mget\|mset\|sunion\|pfcount\)" --type=go -A5
echo ""
echo "=== Check proxyToLeader implementation ==="
rg -n "func.*proxyToLeader" --type=go -A15Repository: bootjp/elastickv
Length of output: 3067
🏁 Script executed:
#!/bin/bash
# 3. Check for timeout context patterns
echo "=== Search for context.WithTimeout and redisDispatchTimeout ==="
rg -n "WithTimeout|redisDispatchTimeout" --type=go -B2 -A2 | head -60
echo ""
echo "=== Check context usage patterns in redis_set_cmds.go ==="
rg -n "context\." adapter/redis_set_cmds.go | head -20Repository: bootjp/elastickv
Length of output: 5929
🏁 Script executed:
#!/bin/bash
# Check if there are any tests for pfcount with multiple keys
echo "=== Search for pfcount tests ==="
rg -i "pfcount" --type=go test -A5 -B2
echo ""
echo "=== Check for MGET, MSET or other multi-key commands to understand the pattern ==="
rg -n "func.*\(mget\|mset\|smove\|sinter\|sunion\)" --type=go -A8
echo ""
echo "=== Look for documentation about key distribution ==="
find . -type f \( -name "*.md" -o -name "*.txt" \) -exec grep -l "shard\|partition\|key.*location" {} \; 2>/dev/null | head -10Repository: bootjp/elastickv
Length of output: 713
🏁 Script executed:
#!/bin/bash
# Check if redis commands that accept multiple keys are tested or documented
echo "=== Search for any comments about multi-key command routing ==="
rg -i "multi.*key|multiple.*key" --type=go -B2 -A2 | head -50
echo ""
echo "=== Look at how similar commands like PFMERGE handle this (if it exists) ==="
rg -n "pfmerge" --type=go -B3 -A15Repository: bootjp/elastickv
Length of output: 3929
🏁 Script executed:
#!/bin/bash
# Check the architecture docs for insights on multi-key commands and sharding
echo "=== Check architecture overview for multi-key/sharding info ==="
grep -i "multi.*key\|cross.*shard\|transaction" docs/architecture_overview.md | head -20
echo ""
echo "=== Check if there are comments about Redis command constraints ==="
rg -B5 -A5 "cross.*shard.*not.*support" --type=goRepository: bootjp/elastickv
Length of output: 1371
🏁 Script executed:
#!/bin/bash
# Look at the actual pfcount usage in context - check if there are any real tests
echo "=== Search all test files for pfcount ==="
find . -name "*_test.go" -o -name "*test*.go" | xargs grep -l "pfcount\|PFCOUNT" 2>/dev/null
echo ""
echo "=== Look at pfadd to see how it handles single key ==="
rg -n "func.*pfadd" --type=go -A15Repository: bootjp/elastickv
Length of output: 1159
🏁 Script executed:
#!/bin/bash
# Verify the timeout pattern is actually redisDispatchTimeout constant
echo "=== Check redisDispatchTimeout constant definition ==="
rg -n "redisDispatchTimeout" --type=go | head -10
echo ""
echo "=== Verify the suggested fix is consistent with pfadd pattern ==="
rg -n "pfadd\|sadd\|zadd" adapter/redis_*.go | grep "func.*pfadd\|func.*sadd\|func.*zadd" -A12 | head -50Repository: bootjp/elastickv
Length of output: 1209
Add timeout context to pfcount loop; verify multi-key sharding assumption.
The pfcount function loops over multiple keys without a timeout-scoped context. Compare with pfadd in the same file: it immediately sets ctx, cancel := context.WithTimeout(context.Background(), redisDispatchTimeout) after the proxyToLeader check. Apply the same pattern to pfcount to wrap the store calls (keyTypeAt, ExistsAt, loadSetAt) in a bounded context.
For the multi-key proxy check: proxyToLeader only examines cmd.Args[1] (the first key), yet the function processes all keys via cmd.Args[1:]. If keys can span multiple shards, this is incorrect. However, the codebase explicitly rejects cross-shard operations (ErrCrossShardTransactionNotSupported, ErrCrossShardMutationBatchNotSupported), and all similar multi-key commands (zadd, zrem, etc.) follow the same pattern. Confirm whether the system architecture guarantees all keys in a single command are co-located, or whether per-key routing is required.
Suggested timeout fix
func (r *RedisServer) pfcount(conn redcon.Conn, cmd redcon.Command) {
if r.proxyToLeader(conn, cmd, cmd.Args[1]) {
return
}
+ ctx, cancel := context.WithTimeout(context.Background(), redisDispatchTimeout)
+ defer cancel()
readTS := r.readTS()
union := map[string]struct{}{}
for _, key := range cmd.Args[1:] {
- typ, err := r.keyTypeAt(context.Background(), key, readTS)
+ typ, err := r.keyTypeAt(ctx, key, readTS)
if err != nil {
writeRedisError(conn, err)
return
}
if typ != redisTypeNone {
- hllExists, err := r.store.ExistsAt(context.Background(), redisHLLKey(key), readTS)
+ hllExists, err := r.store.ExistsAt(ctx, redisHLLKey(key), readTS)
if err != nil {
writeRedisError(conn, err)
return
}
if !hllExists {
conn.WriteError(wrongTypeMessage)
return
}
}
- value, err := r.loadSetAt(context.Background(), hllKind, key, readTS)
+ value, err := r.loadSetAt(ctx, hllKind, key, readTS)
if err != nil {
writeRedisError(conn, err)
return
}
for _, member := range value.Members {
union[member] = struct{}{}
}
}
conn.WriteInt(len(union))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (r *RedisServer) pfcount(conn redcon.Conn, cmd redcon.Command) { | |
| if r.proxyToLeader(conn, cmd, cmd.Args[1]) { | |
| return | |
| } | |
| readTS := r.readTS() | |
| union := map[string]struct{}{} | |
| for _, key := range cmd.Args[1:] { | |
| typ, err := r.keyTypeAt(context.Background(), key, readTS) | |
| if err != nil { | |
| writeRedisError(conn, err) | |
| return | |
| } | |
| if typ != redisTypeNone { | |
| hllExists, err := r.store.ExistsAt(context.Background(), redisHLLKey(key), readTS) | |
| if err != nil { | |
| writeRedisError(conn, err) | |
| return | |
| } | |
| if !hllExists { | |
| conn.WriteError(wrongTypeMessage) | |
| return | |
| } | |
| } | |
| value, err := r.loadSetAt(context.Background(), hllKind, key, readTS) | |
| if err != nil { | |
| writeRedisError(conn, err) | |
| return | |
| } | |
| for _, member := range value.Members { | |
| union[member] = struct{}{} | |
| } | |
| } | |
| conn.WriteInt(len(union)) | |
| } | |
| func (r *RedisServer) pfcount(conn redcon.Conn, cmd redcon.Command) { | |
| if r.proxyToLeader(conn, cmd, cmd.Args[1]) { | |
| return | |
| } | |
| ctx, cancel := context.WithTimeout(context.Background(), redisDispatchTimeout) | |
| defer cancel() | |
| readTS := r.readTS() | |
| union := map[string]struct{}{} | |
| for _, key := range cmd.Args[1:] { | |
| typ, err := r.keyTypeAt(ctx, key, readTS) | |
| if err != nil { | |
| writeRedisError(conn, err) | |
| return | |
| } | |
| if typ != redisTypeNone { | |
| hllExists, err := r.store.ExistsAt(ctx, redisHLLKey(key), readTS) | |
| if err != nil { | |
| writeRedisError(conn, err) | |
| return | |
| } | |
| if !hllExists { | |
| conn.WriteError(wrongTypeMessage) | |
| return | |
| } | |
| } | |
| value, err := r.loadSetAt(ctx, hllKind, key, readTS) | |
| if err != nil { | |
| writeRedisError(conn, err) | |
| return | |
| } | |
| for _, member := range value.Members { | |
| union[member] = struct{}{} | |
| } | |
| } | |
| conn.WriteInt(len(union)) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@adapter/redis_set_cmds.go` around lines 599 - 632, The pfcount function lacks
a timeout-scoped context for its store operations. After the proxyToLeader
check, add a context with timeout using `ctx, cancel :=
context.WithTimeout(context.Background(), redisDispatchTimeout)` and defer the
cancel call (following the pattern used in pfadd), then replace all
`context.Background()` calls within the loop for the keyTypeAt, ExistsAt, and
loadSetAt calls with this new ctx variable. Additionally, verify whether the
multi-key sharding assumption is correct since proxyToLeader only examines
cmd.Args[1] while pfcount iterates over all keys in cmd.Args[1:]; confirm
whether all keys in a single command are guaranteed to be co-located on the same
shard or if per-key routing is required.
| ctx, cancel := context.WithTimeout(context.Background(), redisDispatchTimeout) | ||
| defer cancel() |
There was a problem hiding this comment.
Context rooted in context.Background() won't respect server shutdown.
The xread handler at line 1052 uses r.handlerContext() to ensure operations abort during graceful shutdown, but xadd (and similarly xtrim at line 546) uses context.Background(). This inconsistency means write commands can outlive the server's drain window, potentially causing timeout-related failures during shutdown or keeping connections alive longer than intended.
Given the commit message "adapter: propagate Redis command contexts", this appears to be incomplete work.
Suggested fix
- ctx, cancel := context.WithTimeout(context.Background(), redisDispatchTimeout)
+ ctx, cancel := context.WithTimeout(r.handlerContext(), redisDispatchTimeout)Apply the same fix to xtrim at line 546.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ctx, cancel := context.WithTimeout(context.Background(), redisDispatchTimeout) | |
| defer cancel() | |
| ctx, cancel := context.WithTimeout(r.handlerContext(), redisDispatchTimeout) | |
| defer cancel() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@adapter/redis_stream_cmds.go` around lines 215 - 216, Replace
context.Background() with r.handlerContext() in the context.WithTimeout calls
within both the xadd handler (at lines 215-216) and the xtrim handler (at line
546) to ensure these Redis write commands respect server shutdown signals. This
aligns with how the xread handler properly uses r.handlerContext() at line 1052
and completes the "propagate Redis command contexts" work. The change ensures
operations will be properly cancelled during graceful shutdown instead of
potentially outliving the server's drain window.
| members := zsetEntriesToMap(value.Entries) | ||
| removed = removeZSetMembers(members, cmd.Args[2:]) | ||
| if removed == 0 { | ||
| return nil | ||
| } | ||
| return r.persistZSetEntriesTxn(ctx, cmd.Args[1], readTS, zsetMapToEntries(members)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if loadZSetAt merges wide-column and legacy blob data
ast-grep --pattern $'func ($r *RedisServer) loadZSetAt($$$) {
$$$
}'
# Check if there are other zset persist functions that handle wide-column cleanup
rg -n "ZSetMemberKey|ZSetScoreKey" --type go -A2 -B2Repository: bootjp/elastickv
Length of output: 14856
🏁 Script executed:
# Get the ZREM handler around lines 849-854
sed -n '840,860p' adapter/redis_zset_cmds.goRepository: bootjp/elastickv
Length of output: 555
🏁 Script executed:
# Get persistZSetEntriesTxn implementation
rg -n "func.*persistZSetEntriesTxn" adapter/redis_zset_cmds.go -A 30Repository: bootjp/elastickv
Length of output: 1027
🏁 Script executed:
# Get persistBZPopMinResult (lines 980-998) to compare the pattern
sed -n '980,1000p' adapter/redis_zset_cmds.goRepository: bootjp/elastickv
Length of output: 925
🏁 Script executed:
# Find and examine loadZSetAt to understand merge behavior
rg -n "func.*loadZSetAt" adapter/redis_zset_cmds.go -A 50 | head -100Repository: bootjp/elastickv
Length of output: 42
🏁 Script executed:
# Get loadZSetAt function with more context
rg -n "func.*loadZSetAt" adapter/redis_zset_cmds.go -A 100 | head -150Repository: bootjp/elastickv
Length of output: 42
🏁 Script executed:
# Check what deleteLogicalKeyElems does when entries are empty
rg -n "func.*deleteLogicalKeyElems" adapter/ -A 30Repository: bootjp/elastickv
Length of output: 1998
🏁 Script executed:
# Look at how zsetEntriesToMap and removeZSetMembers work
rg -n "func.*zsetEntriesToMap|func.*removeZSetMembers" adapter/ -A 20Repository: bootjp/elastickv
Length of output: 2773
🏁 Script executed:
# Check if there's migration logic for wide-column to blob
rg -n "isWide.*:=" adapter/redis_zset_cmds.go -B5 -A5Repository: bootjp/elastickv
Length of output: 497
🏁 Script executed:
# Get the full loadZSetAt function to see merge logic
sed -n '680,750p' adapter/redis_zset_cmds.goRepository: bootjp/elastickv
Length of output: 1764
🏁 Script executed:
# Search for where wide-column and blob data are merged
rg -n "isWide|wide.*column" adapter/redis_zset_cmds.go -B3 -A3Repository: bootjp/elastickv
Length of output: 3416
🏁 Script executed:
# Find and examine loadZSetAt with full implementation
rg -n "func.*loadZSetAt" adapter/redis_zset_cmds.go -A 70Repository: bootjp/elastickv
Length of output: 42
🏁 Script executed:
# Check what deleteLogicalKeyElems does - specifically look for wide-column cleanup
sed -n '1038,1100p' adapter/redis_compat_helpers.goRepository: bootjp/elastickv
Length of output: 1964
🏁 Script executed:
# Find deleteZSetWideColumnElems implementation
rg -n "func.*deleteZSetWideColumnElems" adapter/ -A 30Repository: bootjp/elastickv
Length of output: 2322
🏁 Script executed:
# Check if loadZSetAt merges wide-column entries with blob data
rg -n "func.*loadZSetAt" adapter/redis_zset_cmds.goRepository: bootjp/elastickv
Length of output: 42
🏁 Script executed:
# Look for any load function that merges wide-column and blob data
rg -n "mergeZSet|loadZSet|migrateZSet" adapter/redis_zset_cmds.go -B2 -A10Repository: bootjp/elastickv
Length of output: 2238
🏁 Script executed:
# Search for loadZSetAt in all files
find adapter -name "*.go" -type f -exec grep -l "loadZSetAt" {} \;Repository: bootjp/elastickv
Length of output: 197
🏁 Script executed:
# Look at the actual loadZSetAt function - try different search
rg -n "loadZSetAt\(" adapter/redis_zset_cmds.go -B2 -A60 | head -100Repository: bootjp/elastickv
Length of output: 2989
🏁 Script executed:
# Check the redisZSetValue type and how it's loaded/unmarshaled
rg -n "type redisZSetValue|unmarshalZSetValue" adapter/ -A10Repository: bootjp/elastickv
Length of output: 3958
🏁 Script executed:
# Find loadZSetAt function in redis_compat_helpers.go
rg -n "func.*loadZSetAt" adapter/redis_compat_helpers.go -A 50Repository: bootjp/elastickv
Length of output: 2298
🏁 Script executed:
# Also check redis_hash_cmds.go around line 121 for context
sed -n '115,160p' adapter/redis_hash_cmds.goRepository: bootjp/elastickv
Length of output: 1733
🏁 Script executed:
# Check loadZSetMembersAt to understand how wide-column is loaded
rg -n "func.*loadZSetMembersAt" adapter/redis_compat_helpers.go -A 40Repository: bootjp/elastickv
Length of output: 1761
🏁 Script executed:
# Verify if wide-column entries are merged with blob during write-back
rg -n "func.*persistZSetEntriesTxn" adapter/redis_zset_cmds.go -A 20Repository: bootjp/elastickv
Length of output: 798
Wide-column entries not deleted during ZREM when members remain.
When ZREM removes some (but not all) members, persistZSetEntriesTxn writes the remaining entries back to the legacy blob key only, leaving the wide-column ZSetMemberKey and ZSetScoreKey entries for removed members intact. On the next read, loadZSetAt detects the presence of any wide-column member key and loads exclusively from wide-column storage, effectively reloading the deleted members and un-deleting them.
This mirrors the correct pattern in persistBZPopMinResult (lines 980–998), which explicitly deletes both ZSetMemberKey and ZSetScoreKey for popped members. Apply the same cleanup to ZREM and other partial-removal operations that call persistZSetEntriesTxn.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@adapter/redis_zset_cmds.go` around lines 849 - 854, The ZREM operation
removes members from the zset but only persists the remaining entries to the
legacy blob key via persistZSetEntriesTxn, leaving the wide-column ZSetMemberKey
and ZSetScoreKey entries for removed members untouched. This causes the removed
members to be reloaded on the next read. To fix this, after calling
removeZSetMembers to identify which members were removed, explicitly delete the
ZSetMemberKey and ZSetScoreKey wide-column entries for each removed member
(following the same pattern used in persistBZPopMinResult for lines 980-998)
before or in conjunction with the persistZSetEntriesTxn call.
Summary
Behavior-preserving refactor that splits the two largest Redis adapter files into cohesive, same-package (
adapter) files. This is pure code movement: types/functions/methods/consts were relocated verbatim (comments moved with their code). No signatures, logic, names, or semantics changed.adapter/redis.go: 4,483 -> 935 lines (core entry kept: server struct, options,NewRedisServer, lifecycle, dispatch infra, conn state, metrics/error helpers, ping, validateCmd, pub/sub fan-out).adapter/redis_compat_commands.go: 5,434 -> 81 lines (now holds only the shared top-levelconstblock).File map (region -> new file, with line counts)
Moved out of
redis.go:redis_strings.goredis_keys.gotxnContext+ all txn methods, MULTI/DISCARD/EXEC,runTransaction,runTransactionWithDedup,dispatchExecReuse,firstExecAttempt,txnStartTS,txnApplyHandlersredis_txn.goredis_proxy_leader.golistPushCore,listPushCoreWithDedup,dispatchListPushReuse, pop/range/trim)redis_lists.goMoved out of
redis_compat_commands.go:redis_server_cmds.goredis_expire_cmds.goredis_set_cmds.goredis_hash_cmds.goredis_zset_cmds.goredis_stream_cmds.goredis_lists.go(shared dest)buildRouteMap(the dispatch table) was already inredis_command_specs.goand was left untouched.No behavior change
Pure move. Verified two independent ways:
go doc -all ./adapteridentical — captured the exported API surface against the worktree base (viagit stashof the source files) and after the split;diffreports no differences.package/importblocks from all 13 files and comparing the multiset of non-blank code lines against the two originals: 9,144 lines on both sides, 0 missing, 0 extra. The +147 net line delta in the raw diff is entirely the addedpackage adapter+import (...)boilerplate in the 11 new files.Imports were recomputed per file and the source-file alias conventions preserved exactly:
redis.go-derived files keep"github.com/cockroachdb/errors"unaliased;redis_compat_commands.go-derived files keep stdlib"errors"pluscockerrors "github.com/cockroachdb/errors", andredis_stream_cmds.gokeepsgoogle.golang.org/grpc/{codes,status}.Jepsen-guarded blocks moved intact
The recently-merged one-phase-dedup default-flip config options (#943) —
WithOnePhaseTxnDedup/WithStandaloneSetDedup— were left in place inredis.gocore. The txn/dedup machinery (txnContext+ methods,runTransaction,runTransactionWithDedup,dispatchExecReuse,firstExecAttempt,txnStartTS,txnApplyHandlers) was moved as one intact contiguous block intoredis_txn.go; the list-dedup path (listPushCoreWithDedup/dispatchListPushReuse/listPushCore) was moved intact intoredis_lists.go. No internals of these blocks were reorganized.Verification evidence
Five-lens self-review (pure move)
go docchecks prove no statement was dropped or altered. No new error-handling paths.go test -raceon the full Redis suite passes.Next()path touched.go docparity confirms the public contract is unchanged.*_test.gofiles were not split and continue to exercise the moved code (fullTestRedis|Redisrace suite green).Summary by CodeRabbit