adapter: Fence Redis reads before snapshots - #1167
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRedis のリスト読み取りと MULTI/EXEC に read fence、シャードリーダールーティング、ObservedRouteVersion の伝搬・検証を導入します。ルート変更時の再試行、dedup/reuse、ワイヤエラー復元、リストキー正規化も更新します。 ChangesRedis read-fence routing
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant RedisClient
participant RedisServer
participant ReadFenceCoordinator
participant ShardLeader
RedisClient->>RedisServer: EXEC queued commands
RedisServer->>ReadFenceCoordinator: transactionProxyRoute
ReadFenceCoordinator->>ReadFenceCoordinator: redisReadFencedTimestamp
ReadFenceCoordinator->>ShardLeader: proxyTransactionToLeaderForKey
RedisServer->>RedisServer: applyExecQueueAtSnapshot
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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 |
|
@codex review |
![chatgpt-codex-connector[bot]](https://cdn.github.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cbc99dd14a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| if err := r.coordinator.VerifyLeaderForKey(ctx, key); err != nil { | ||
| return errors.WithStack(err) |
There was a problem hiding this comment.
In a multi-group deployment with leadership balanced across nodes, exec proxies only to the default-group leader (IsLeader()), but this call verifies each key against that node's local shard engine. ShardedCoordinator.VerifyLeaderForKey does not forward to the key's leader, so even a single-key EXEC targeting a non-default shard led by another node returns a leader error (and cross-shard EXEC cannot succeed unless one node leads every involved group). Use a leader-routed per-shard fence or otherwise route execution without requiring the default leader to lead each key's group.
Useful? React with 👍 / 👎.
| // VerifyLeaderForKey honours the caller's deadline rather than the | ||
| // long-lived handlerContext + verifyLeaderEngineCtx fallback. Same | ||
| // shape as keys() / FLUSHDB. | ||
| if err := r.coordinator.VerifyLeaderForKey(ctx, key); err != nil { |
There was a problem hiding this comment.
When the local leader has committed but not yet applied entries, VerifyLeaderForKey is not an applied-state fence: it reaches Engine.VerifyLeader, which calls submitRead(ctx, false), and handleReadStates completes such requests immediately without waiting for e.applied. Consequently this call can succeed while the MVCC store and its LastCommitTS remain stale, so the immediately selected LRANGE snapshot can still omit a write that completed before the read; the new EXEC pre-pass has the same problem. Use the linearizable/lease-read path that waits for the read index to be applied before selecting the snapshot.
Useful? React with 👍 / 👎.
| if !ok { | ||
| continue | ||
| } | ||
| for _, key := range redisCommandGetKeys(meta, cmd.Args) { |
There was a problem hiding this comment.
When a valid transaction contains many distinct keys, such as one variadic DEL or EXISTS, this loop performs a sequential VerifyLeaderForKey for every key even when all keys resolve to the same Raft group. Each verification submits a quorum-backed Raft read request, so a large command creates hundreds or thousands of serialized ReadIndex rounds under the single 30-second EXEC context, causing otherwise valid transactions to time out and unnecessarily loading the Raft group. Resolve and deduplicate the affected groups, then fence each group once.
Useful? React with 👍 / 👎.
| continue | ||
| } | ||
| seen[keyID] = struct{}{} | ||
| if err := r.coordinator.VerifyLeaderForKey(ctx, key); err != nil { |
There was a problem hiding this comment.
For a legal Redis user key that resembles another adapter's namespace, such as !sqs|foo, passing the raw command argument to VerifyLeaderForKey changes its routing identity: routeKey treats the raw !sqs|... value as SQS-internal and routes it to the global SQS group, whereas the actual Redis rows (for example !redis|str|!sqs|foo or list-family rows) normalize back to the literal Redis user key and may belong to a different shard. The pre-pass can therefore fence an unrelated group, fail because this node does not lead that group, and leave the actual Redis data shard unfenced. Derive the fence footprint from canonical Redis storage keys, retaining a separate legacy-bare-key fence only where fallback reads require it.
Useful? React with 👍 / 👎.
|
Updated the Redis read fence implementation:
Validation:
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ae2c34ac7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| for _, key := range kv.LeaseReadGroupKeys(r.coordinator, redisQueuedCommandReadFenceKeys(queue)) { | ||
| if _, err := kv.LeaseReadForKeyThrough(r.coordinator, ctx, key); err != nil { |
There was a problem hiding this comment.
When a sharded cluster elects the default group and a queued key's group on different nodes, exec remains on the default-group leader because it checks only coordinator.IsLeader(), but this loop then calls LeaseReadForKey against the local follower engine for the key's group and returns a leadership error. Previously the underlying ShardStore reads could proxy to each key's leader, so even a single-key EXEC targeting a non-default shard now fails unless its leader happens to be colocated with the default leader; the fence must be established through the owning leader or the transaction must be routed accordingly.
Useful? React with 👍 / 👎.
| return r.proxyLRange(key, routeKey, startRaw, endRaw) | ||
| } | ||
|
|
||
| if _, err := kv.LeaseReadForKeyThrough(r.coordinator, ctx, routeKey); err != nil { |
There was a problem hiding this comment.
In a sharded layout where a list's storage representations resolve to more than one Raft group, this fences only the listMetaKey group before choosing readTS. The subsequent keyTypeAt and resolveListMeta calls also read string/type keys and store.ListMetaDeltaScanPrefix(key); a completed write whose local apply is pending on one of those other groups can therefore be omitted from the snapshot, returning a stale type or range. Standalone LRANGE should group-deduplicate and fence the same storage footprint used by the EXEC path before selecting the timestamp.
Useful? React with 👍 / 👎.
|
Addressed the latest Redis read-fence review findings:
Validation:
|
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
![coderabbitai[bot]](https://cdn.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
adapter/redis_txn.go (2)
84-110: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winフェンスキー集合の再計算をリトライループ外へ巻き上げる余地があります。
redisQueuedCommandReadFenceKeysはキュー内コマンド数 N に対しredisTxnReadFenceKeysの 25 個の派生キーを毎回構築するため、O(25 × N × キー数)のアロケーションが発生します。これがrunTransactionDirectのretryRedisWriteループ内（Line 2566）から試行ごとに呼ばれます。lease 自体は試行ごとに必要ですが、
queuedCommandReadFenceGroupKeys(queue)の結果（グループキー集合）はキューが不変である限り試行間で再利用できます。ループ外で一度算出し、ループ内ではleaseRedisReadFenceGroupsにその結果を渡す形が素直です。♻️ 提案する変更（Line 2564 付近）
var results []redisResult + fenceGroupKeys := r.queuedCommandReadFenceGroupKeys(queue) err := r.retryRedisWrite(dispatchCtx, func() error { - if err := r.leaseQueuedCommandReadGroups(dispatchCtx, queue); err != nil { + if err := r.leaseRedisReadFenceGroups(dispatchCtx, fenceGroupKeys); err != nil { return 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_txn.go` around lines 84 - 110, In runTransactionDirect, compute the queued command fence group keys once before the retryRedisWrite loop, then pass that cached collection to leaseRedisReadFenceGroups on every attempt. Keep lease acquisition inside the loop, and reuse redisQueuedCommandReadFenceKeys only while the queue remains unchanged.
200-210: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winフェンスグループの lease を並列化してください。
leaseRedisReadFenceGroupsは各 Raft グループのLeaseReadForKey読みを順次実行します。DynamoDB の実装と同様に、groupKeys > 1の際はsync.WaitGroup+ cancel 子context で並列 fanning し、最初のエラーで残りの lease を取消す形にすると、マルチキー EXEC のレイテンシがグループ数に比例して増えるのを防げます。🤖 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_txn.go` around lines 200 - 210, Update RedisServer.leaseRedisReadFenceGroups to lease multiple groupKeys concurrently when more than one key is provided, using a sync.WaitGroup and cancellable child context. Have workers stop or cancel remaining leases after the first error, safely retain and return the first error, while preserving the existing nil-server/coordinator and single-key behavior.Source: Coding guidelines
🤖 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_txn.go`:
- Around line 128-161: Update readFenceLeader and the readFenceProxyKey flow so
groups led by the local node contribute a non-empty self-node address or
sentinel to targetLeader before recordReadFenceTargetLeader is called. Preserve
remote leader recording and ensure EXECs mixing local and remote leader groups
are recognized as split rather than proxied as a single remote target.
---
Nitpick comments:
In `@adapter/redis_txn.go`:
- Around line 84-110: In runTransactionDirect, compute the queued command fence
group keys once before the retryRedisWrite loop, then pass that cached
collection to leaseRedisReadFenceGroups on every attempt. Keep lease acquisition
inside the loop, and reuse redisQueuedCommandReadFenceKeys only while the queue
remains unchanged.
- Around line 200-210: Update RedisServer.leaseRedisReadFenceGroups to lease
multiple groupKeys concurrently when more than one key is provided, using a
sync.WaitGroup and cancellable child context. Have workers stop or cancel
remaining leases after the first error, safely retain and return the first
error, while preserving the existing nil-server/coordinator and single-key
behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4fe10946-57b0-4d00-a5b2-0e22f0a7f11d
📒 Files selected for processing (4)
adapter/redis_lists.goadapter/redis_proxy_leader.goadapter/redis_txn.goadapter/redis_txn_test.go
|
Addressed the latest read-fence routing feedback:
Validation:
|
|
CI is green on the latest head. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a9b26b9251
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| if err := r.leaseRedisReadFenceGroups(dispatchCtx, fenceGroupKeys); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| startTS := r.txnStartTS() |
There was a problem hiding this comment.
When an EXEC spans at least two shard groups led by the same node, this fence-before-snapshot ordering can still produce stale OCC reads: after group A's lease completes, an A write can obtain T1 and remain unapplied while group B applies T2 > T1; txnStartTS() then uses ShardStore.LastCommitTS()—the maximum across groups—as T2, so the transaction misses A's T1 value and later treats it as non-conflicting because T1 <= startTS. This preserves the lost-write/strict-serializability anomaly the fence is intended to close. Ensure every involved group is caught up through the selected startTS, such as with a timestamp-aware or second barrier, and cover this two-group interleaving.
AGENTS.md reference: AGENTS.md:L24-L25
Useful? React with 👍 / 👎.
| if *target != leader { | ||
| return errRedisExecSplitShardLeaders |
There was a problem hiding this comment.
When a request reaches a node that is not the leader of either involved group, two groups led by the same remote process still normally report different Raft endpoints because each group has its own listener, while buildLeaderRedis maps both endpoints to that process's single Redis address (main.go:1027-1029,1154-1167). Comparing the raw Raft endpoint strings here therefore returns ERR EXEC read fence spans multiple shard leaders even though one Redis target can execute and fence the transaction; the same transaction only succeeds when sent directly to the co-located leader node. Resolve and compare the Redis target or stable node identity before deciding the leaders are split, and cover this with real per-group addresses.
AGENTS.md reference: AGENTS.md:L25-L27
Useful? React with 👍 / 👎.
| redisTxnWideListFenceKey(userKey), | ||
| redisHashKey(userKey), | ||
| store.HashMetaKey(userKey), | ||
| store.HashFieldScanPrefix(userKey), |
There was a problem hiding this comment.
When a hash, set, zset, or stream wide-column range is split after its scan-prefix key, this representative fences only the group owning the prefix start, although ShardStore.ScanAt visits every intersecting route (kv/shard_store.go:302-308). Point reads are also missed: for example, loadHashFieldsIntoState reads HashFieldKey(key, field), which can fall in a different route than HashFieldScanPrefix(key). The proxy prepass can therefore conclude that all required groups are local and then build its transaction snapshot from an unfenced group, allowing stale reads during leadership churn. Enumerate all routes intersecting each scanned range and include command-dependent exact storage keys before selecting the proxy target or leasing groups.
AGENTS.md reference: AGENTS.md:L24-L25
Useful? React with 👍 / 👎.
|
Addressed the latest Redis read-fence P1 findings:
Validation:
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 86cd4f0933
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| if s == nil || s.engine == nil { | ||
| return nil | ||
| } | ||
| routes, clampToRoutes := s.routesForForwardScan(start, end) |
There was a problem hiding this comment.
When a shard cutover updates the distribution engine after this call but before the Redis read, these routes can differ from the routes that the later ScanAt independently obtains via routesForForwardScan. Both fence passes reuse the already-materialized groupKeys, so a newly selected group is never leased and the snapshot can read stale state from that group. Pin the route snapshot/version through the read, or re-resolve and retry until the route set remains unchanged across the second fence and scan.
Useful? React with 👍 / 👎.
|
Addressed the latest read-fence route-stability P1:
Validation:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 257d4b5835
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| if err := r.ensureRedisReadFenceRouteStable(routeVersion); err != nil { | ||
| return err |
There was a problem hiding this comment.
When the catalog changes after this check, txn.commit() can still spend time building deletion/stream elements and allocating a commit timestamp before dispatching. The resulting OperationGroup leaves ObservedRouteVersion at zero, and ShardedCoordinator deliberately does not auto-pin transactions with a supplied StartTS or read keys, so the FSM cannot reject a transaction that read from the old owner and is dispatched to the new owner. Carry the observed version through prepareDispatch into every OperationGroup, or otherwise make the final stability check atomic with dispatch; the dedup first-attempt path has the same gap between its check and prepareDispatch.
Useful? React with 👍 / 👎.
| func (r *RedisServer) firstExecAttempt(dispatchCtx context.Context, queue []redcon.Command) ([]redisResult, *reusableExecTxn, error) { | ||
| startTS := r.txnStartTS() | ||
| readPin := r.pinReadTS(startTS) | ||
| routeVersion := r.redisReadFenceRouteVersion() |
There was a problem hiding this comment.
With onePhaseTxnDedup enabled, this version is captured only while constructing the first attempt. If that dispatch returns a retryable error and the route catalog changes during the retry backoff, the pending != nil branch calls dispatchExecReuse directly without recomputing group keys, taking new read fences, or comparing the route version. It can therefore dispatch the old write/read set and return cached results against the new ownership; retain the version in reusableExecTxn and drop/rebuild the pending attempt when it changes.
Useful? React with 👍 / 👎.
| route, err := r.transactionProxyRoute(queue) | ||
| if err != nil { | ||
| writeRedisError(conn, err) | ||
| return | ||
| } | ||
| if route.defaultLeader { | ||
| r.proxyTransactionToLeader(conn, queue) | ||
| return | ||
| } | ||
| if len(route.key) > 0 { | ||
| r.proxyTransactionToLeaderForKey(conn, route.key, queue) |
There was a problem hiding this comment.
When transactionProxyRoute selects a remote target, the function can resolve its many range representatives across different catalog versions, but this proxy branch bypasses runTransaction, so none of the new route-version checks run. If a split or move is applied while those representatives are being collected, the mixed set can select the old Redis target and omit a newly intersecting group; that target may then execute using its still-stale catalog and return stale results or commit against the old ownership. Capture and verify the route version around proxy-route resolution before forwarding the EXEC.
Useful? React with 👍 / 👎.
|
Addressed the latest route-version P1 findings:
Validation:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 09a0e4959a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| if !v.tracked { | ||
| return 0 | ||
| } | ||
| return v.version |
There was a problem hiding this comment.
When the store is using the valid version-0 catalog (the state created by NewEngineWithDefaultRoute), this returns 0, which downstream code interprets as unpinned: verifyComposed1 skips its ownership checks and ensureReusableExecRouteStable skips rebuilding a pending attempt. If the first catalog transition occurs after the Redis read fence, EXEC can therefore dispatch or reuse writes derived from the version-0 routing without detecting that ownership changed. Preserve the tracked/untracked distinction through dispatch, or prevent Redis requests from being served while zero is a valid catalog version.
Useful? React with 👍 / 👎.
| return nil, nil, err | ||
| } | ||
|
|
||
| prepared, err := txn.prepareDispatch() |
There was a problem hiding this comment.
When the catalog changes during prepareDispatch, its delete/replacement builders can run range scans such as deleteLogicalKeyElems and deleteStreamWideColumnElems against groups that were not covered by the earlier fence. There is no stability check after these scans, and the FSM's composed-route check examines only emitted mutations, so a stale scan that omits moved rows can still commit a partial collection deletion. Recheck routeVersion after preparation and before dispatch; the direct txn.commit path has the same ordering and needs the same protection.
Useful? React with 👍 / 👎.
| if err := r.ensureReusableExecRouteStable(pending); err != nil { | ||
| return nil, true, err |
There was a problem hiding this comment.
When the first dispatch returns a retryable ambiguous error, pending exists specifically because that attempt may already have committed and must be probed through PrevCommitTS. If the route catalog changes before reuse, this early return sets drop=true, causing the retry loop to clear pending and rebuild the transaction without first determining whether the original commit landed. A committed non-idempotent command such as INCR can then execute a second time; if the old attempt cannot safely be probed after the route change, return the ambiguity instead of replaying it.
Useful? React with 👍 / 👎.
| if err := txn.commit(routeVersion.observedRouteVersion()); err != nil { | ||
| return err |
There was a problem hiding this comment.
If the catalog changes after the last adapter-side stability check but before the Raft apply, the newly supplied ObservedRouteVersion makes the FSM return ErrComposed1Violation or ErrComposed1VersionGCd. ShardedCoordinator deliberately surfaces those errors for read-write transactions so the adapter can rerun the reads, but retryRedisWrite only recognizes write conflicts and transaction locks, so this path returns the routing error to the Redis client instead of rebuilding EXEC. Classify the composed-route sentinels as retryable at the outer EXEC level while ensuring any pending ambiguous attempt retains its dedup protection.
Useful? React with 👍 / 👎.
|
Addressed the latest HEAD review findings in cf73c96. Changes:
Validation:
|
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (cf73c96):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (b36da7a):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
Addressed the latest read-fence review findings:
Caller audit after semantic changes:
Validation:
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
store/list_helpers.go (1)
178-180: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win長さ比較を
uint64へ広げ、//nolintを削除してください。
uint32(len(trimmed))は4GiB超で切り詰められ、厳密な長さ検証が成立しなくなります。修正案
- if uint32(len(trimmed)) != uint32(wideColKeyLenSize)+ukLen { //nolint:gosec // len is bounded by max slice size + if uint64(len(trimmed)) != uint64(wideColKeyLenSize)+uint64(ukLen) {As per coding guidelines, “Avoid using
//nolintdirectives; refactor instead in Go code.”🤖 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 `@store/list_helpers.go` around lines 178 - 180, In the length validation around ukLen, widen both sides of the comparison to uint64 so lengths above 4 GiB are checked without truncation, and remove the //nolint:gosec directive. Preserve the existing nil return for mismatched lengths.Sources: Coding guidelines, Linters/SAST tools
adapter/redis_txn_test.go (1)
694-733: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win選択されたfence/route keyを完全一致で検証してください。
現在の緩いassertionでは、不要なgroupの追加やroute変更後も古いrouteを返す退行を検出できません。
adapter/redis_txn_test.go#L694-L733: 各コマンドについて、必須の基底キーを含む完全な期待集合をrequire.ElementsMatchで比較してください。adapter/redis_txn_test.go#L1075-L1080:route.keyが非空だけでなく、更新後のrouteBと一致することを検証してください。🤖 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_txn_test.go` around lines 694 - 733, Strengthen the assertions in adapter/redis_txn_test.go:694-733 by replacing the per-route Contains/NotContains checks for each queued command with require.ElementsMatch comparisons against the complete expected fence/route-key sets, including all required base keys and excluding extras. In adapter/redis_txn_test.go:1075-1080, update the route.key assertion to verify equality with the updated routeB value rather than only checking that it is non-empty.kv/transcoder_test.go (1)
9-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winゲート有効時のpinned-zeroエンコードも検証してください。
現在は無効時とsentinelの直接decodeのみで、
EncodeObservedRouteVersion(0)がsentinelを返す分岐を通りません。グローバルゲートを復元する非parallelテストを追加してください。🤖 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 `@kv/transcoder_test.go` around lines 9 - 20, Extend TestEncodeObservedRouteVersionZeroGatedForRollingUpgrade to cover the enabled-gate path: temporarily enable the global gate used by EncodeObservedRouteVersion, restore its original value with cleanup, and mark this test non-parallel. Assert that EncodeObservedRouteVersion(0) returns ObservedRouteVersionZero, while preserving the existing disabled and direct-decode assertions.
🤖 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.
Nitpick comments:
In `@adapter/redis_txn_test.go`:
- Around line 694-733: Strengthen the assertions in
adapter/redis_txn_test.go:694-733 by replacing the per-route
Contains/NotContains checks for each queued command with require.ElementsMatch
comparisons against the complete expected fence/route-key sets, including all
required base keys and excluding extras. In adapter/redis_txn_test.go:1075-1080,
update the route.key assertion to verify equality with the updated routeB value
rather than only checking that it is non-empty.
In `@kv/transcoder_test.go`:
- Around line 9-20: Extend
TestEncodeObservedRouteVersionZeroGatedForRollingUpgrade to cover the
enabled-gate path: temporarily enable the global gate used by
EncodeObservedRouteVersion, restore its original value with cleanup, and mark
this test non-parallel. Assert that EncodeObservedRouteVersion(0) returns
ObservedRouteVersionZero, while preserving the existing disabled and
direct-decode assertions.
In `@store/list_helpers.go`:
- Around line 178-180: In the length validation around ukLen, widen both sides
of the comparison to uint64 so lengths above 4 GiB are checked without
truncation, and remove the //nolint:gosec directive. Preserve the existing nil
return for mismatched lengths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 094deaf2-12ac-4852-9384-29ade70a8e84
📒 Files selected for processing (14)
adapter/redis_error_prefix_test.goadapter/redis_lists.goadapter/redis_proxy_leader.goadapter/redis_retry.goadapter/redis_retry_test.goadapter/redis_txn.goadapter/redis_txn_test.gokv/shard_key.gokv/shard_key_test.gokv/shard_store.gokv/shard_store_test.gokv/transcoder.gokv/transcoder_test.gostore/list_helpers.go
🚧 Files skipped from review as they are similar to previous changes (5)
- kv/transcoder.go
- kv/shard_store_test.go
- kv/shard_store.go
- adapter/redis_lists.go
- adapter/redis_txn.go
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b36da7a679
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| case cmdHSet, cmdHMSet: | ||
| return redisHashReadFenceRanges(userKey) | ||
| case cmdRPush, cmdLRange: | ||
| return redisListReadFenceRanges(userKey) | ||
| case cmdZIncrBy: | ||
| return redisZSetReadFenceRanges(userKey) |
There was a problem hiding this comment.
HSET/HMSET, RPUSH/LRANGE, and ZINCRBY all call stagedKeyType, which falls through to the full keyTypeAt scan across the list/hash/set/zset families, but these cases fence only the command's expected collection ranges. When the key actually has a different collection type whose scan prefix intersects another shard, that shard is omitted from the proxy decision and snapshot fence, so the command can miss the existing type and execute instead of returning WRONGTYPE (or LRANGE can return an empty result). Union redisTypeDetectionReadFenceRanges into each of these command footprints before adding its command-specific ranges.
Useful? React with 👍 / 👎.
| // observedRouteVersionZeroWireEncodingEnabled stays disabled until every Raft | ||
| // member advertises support for ObservedRouteVersionZero. Mixed-version groups | ||
| // must keep literal zero on the wire so old followers do not treat the sentinel | ||
| // as an impossibly new catalog version and diverge from a new leader. | ||
| const observedRouteVersionZeroWireEncodingEnabled = false |
There was a problem hiding this comment.
On a fresh cluster whose valid catalog version is 0, this hard-coded false gate makes EncodeObservedRouteVersion(0) emit the unpinned wire value, so verifyComposed1 skips ownership validation. If the first catalog transition occurs after the adapter's final stability check but before dispatch/apply, a transaction derived from the version-0 owner can therefore commit on the new owner without rejection. Fresh evidence since the prior finding is that the new capability gate is permanently disabled and no member-advertisement path can enable it; retain a tracked/untracked distinction without emitting an old-node-unsafe sentinel, or implement the advertised capability negotiation.
Useful? React with 👍 / 👎.
| func isRedisExecTerminalProxyError(err error) bool { | ||
| return err != nil && | ||
| (errors.Is(err, errRedisExecRouteChangedAfterAmbiguousAttempt) || | ||
| err.Error() == errRedisExecRouteChangedAfterAmbiguousAttempt.Error()) |
There was a problem hiding this comment.
If leadership changes after one node chooses a common remote Redis target but before that target resolves its own read-fence groups, the target can return errRedisExecSplitShardLeaders as a top-level EXEC error. go-redis surfaces that decoded error on the pipeline and queued commands, but this predicate recognizes only the ambiguous-route-change text, so the proxy falls through to writeProxyCmdsResult and turns a transaction-level failure into an EXEC result array of command errors. Match the exact split-leader error here as another terminal proxy error.
Useful? React with 👍 / 👎.
| func (r *RedisServer) redisTxnReadFenceKeysForRanges(userKey []byte, ranges []redisReadFenceRange) [][]byte { | ||
| keys := append([][]byte{}, redisTxnReadFenceKeys(userKey)...) | ||
| for _, readRange := range ranges { | ||
| keys = append(keys, r.redisReadFenceRangeGroupKeys(readRange.start, readRange.end)...) |
There was a problem hiding this comment.
Fresh evidence after the command-specific range change is that every command still starts from the full redisTxnReadFenceKeys set here, which includes unused raw scan-prefix representatives such as ZSetScoreScanPrefix and StreamEntryScanPrefix. Those wide-column prefixes are routed by their raw bytes rather than normalized to the Redis user key, so in a split catalog they can resolve to unrelated groups even though GET/EXISTS never scan those ranges; MULTI; GET k; EXEC can therefore still fail with errRedisExecSplitShardLeaders solely because an unused collection prefix is led elsewhere. Build the point representatives per command as well as the range list, or omit scan-prefix point keys already covered by range expansion.
Useful? React with 👍 / 👎.
| if store.IsListMetaDeltaKey(key) { | ||
| if user := store.ExtractListUserKeyFromDelta(key); user != nil { |
There was a problem hiding this comment.
In a sharded deployment, an external Redis command can supply an arbitrary binary key beginning with !lst|meta|d|; routing that key now calls ExtractListUserKeyFromDelta, whose uint32 length calculation can wrap before slicing. For example, a short key with an encoded user length of 0xffffffff passes the wrapped bounds check and then evaluates a slice with start 4 and wrapped end 3, panicking during IsLeaderForKey/route resolution before any storage access. The claim decoder added below has the same arithmetic shape. Validate the encoded length without overflowing (for example against len(trimmed)-fixedSuffix) before invoking these extractors on untrusted routing keys.
Useful? React with 👍 / 👎.
|
Addressed the latest nitpick review on the new head:
Validation:
@codex review |
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (94fb695):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 94fb695e56
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
|
|
||
| func listRouteKey(key []byte) []byte { | ||
| if store.IsListMetaDeltaKey(key) { | ||
| if user := store.ExtractListUserKeyFromDelta(key); user != nil { |
There was a problem hiding this comment.
When a standalone list command supplies a binary key beginning with !lst|meta|d| or !lst|clm|, listPushCmd passes it directly to IsLeaderForKey, so this newly added routing path invokes the full-key extractor on untrusted bytes. Fresh evidence on this head is that only the new scan-prefix helper was widened to uint64; ExtractListUserKeyFromDelta and ExtractListUserKeyFromClaim still perform overflowing uint32 additions, so an encoded length of 0xffffffff can pass their bounds check and panic on a slice whose end wrapped below its start. Validate the full delta/claim extractors without overflow before calling them here.
Useful? React with 👍 / 👎.
|
Addressed the latest Redis read-fence review findings:
Validation:
|
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (d2951fb):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
store/list_helpers_test.go (1)
18-52: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winscan prefix の回帰テストを追加してください。
store/list_helpers.goで追加されたscan prefix抽出ヘルパは、kv/shard_key.goのルーティングfallbackで使用されますが、このテストではfull keyしか検証していません。正しいprefix、別prefix、切り詰めた長さ、余分な末尾バイトをtable-driven caseとして追加し、誤ったroute key抽出を防いでください。🤖 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 `@store/list_helpers_test.go` around lines 18 - 52, Add table-driven regression cases for the scan-prefix extraction helpers in store/list_helpers.go, covering the expected prefix, a different prefix, truncated input, and trailing extra bytes. Extend the existing TestExtractListUserKeyRejectsMalformedFullKeyLength or add a focused test using the relevant extraction symbols, asserting only the valid prefix is accepted and malformed route keys return nil.
🤖 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.
Nitpick comments:
In `@store/list_helpers_test.go`:
- Around line 18-52: Add table-driven regression cases for the scan-prefix
extraction helpers in store/list_helpers.go, covering the expected prefix, a
different prefix, truncated input, and trailing extra bytes. Extend the existing
TestExtractListUserKeyRejectsMalformedFullKeyLength or add a focused test using
the relevant extraction symbols, asserting only the valid prefix is accepted and
malformed route keys return nil.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ab219538-0047-4b68-90f4-b7a679df2b77
📒 Files selected for processing (8)
adapter/redis_error_prefix_test.goadapter/redis_proxy_leader.goadapter/redis_txn.goadapter/redis_txn_test.gokv/transcoder.gokv/transcoder_test.gostore/list_helpers.gostore/list_helpers_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- kv/transcoder_test.go
- kv/transcoder.go
- adapter/redis_proxy_leader.go
- adapter/redis_error_prefix_test.go
- adapter/redis_txn.go
- adapter/redis_txn_test.go
|
Addressed the latest test coverage nitpick:
Validation:
|
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (a5a5520):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a5a5520a18
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| if store.IsListMetaDeltaKey(key) { | ||
| if user := store.ExtractListUserKeyFromDelta(key); user != nil { | ||
| return user |
There was a problem hiding this comment.
When a legal Redis user key is itself a well-formed delta key, such as the bytes returned by store.ListMetaDeltaKey([]byte("other"), 1, 1), direct routing calls like IsLeaderForKey(key) now resolve it to other. However, the Redis/list storage rows created for that user key are wrapped in another internal prefix and route to the literal full user key, so the initial command can be proxied to an unrelated shard leader and perform its snapshot reads without fencing the shard that owns its data. The overflow fix does not address these valid encodings; claim-shaped user keys have the same mismatch, so list decoding must be limited to known storage keys or callers must route using canonical wrapped keys.
Useful? React with 👍 / 👎.
|
Addressed the latest P1 routing finding:
Validation:
|
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (4ed6cae):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ed6cae21b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| func redisUserRouteKey(userKey []byte) []byte { | ||
| return append([]byte(redisRoutePrefix), userKey...) |
There was a problem hiding this comment.
For a legal zero-length Redis key, this produces !redis|route|, but kv.redisRouteKey rejects keys with no bytes after the final separator, so the representative routes by the wrapper bytes rather than by the empty logical key. Empty-key storage representatives such as !redis|str| are likewise left as distinct raw prefixes; in a sharded catalog where those prefixes belong to different groups, even MULTI; GET ""; EXEC can appear to span multiple leaders and fail with ERR EXEC read fence spans multiple shard leaders, while standalone proxy checks can select a group different from the stored row. Encode empty keys unambiguously or make routing distinguish a successfully decoded empty key from decode failure.
Useful? React with 👍 / 👎.
|
Addressed the empty Redis key routing follow-up:
Validation:
|
TLA+ spec divergence review (auto-triggered)This PR touches files that the TLA+ safety spec has an anchor on (per Anchored files changed in this PR head (ae80151):
What to check, by subsystem:
If the change is correct but requires a spec update, edit @claude review please verify TLA+ spec divergence per the checklist above. @codex review please verify TLA+ spec divergence per the checklist above. |
|
To use Codex here, create a Codex account and connect to github. |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! 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". |
1 participant
Summary
LRANGE.MULTI/EXECin both direct and one-phase dedup paths.Root cause
The scheduled Redis Jepsen append workload exposed a strict-serializability anomaly during leader churn.
LRANGEandMULTI/EXECcould choose an MVCCreadTSbefore the quorum-backedVerifyLeaderForKeypath had caught the local replica up, so a successful read could observe an older local snapshot.Validation
GOCACHE=$(pwd)/.cache GOTMPDIR=$(pwd)/.cache/tmp go test ./adapter -run 'TestRedis(RangeListVerifiesLeaderBeforeSnapshot|ExecVerifiesLeaderBeforeSnapshot)' -count=1golangci-lint --config=.golangci.yaml run --fix(0 issues){:valid? true}/Everything looks good!Note:
go test ./adapteras a full package run timed out after 10 minutes. The timeout dump includedTestMilestone1SplitRange_RestartReloadsCatalog; that test passed when run in isolation with a 180s timeout.Summary by CodeRabbit