Fix sparse Lua list pop fallback#1094
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
![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 refactors the Redis Lua list sparse pop scan logic by introducing an exact scan fallback (scanListItemWindowExact) when physical limits are reached, and updates the corresponding tests to verify scanning past physical tombstones. The review feedback highlights a potential prefix collision vulnerability in scanListItemWindowExact where a single-item scan limit could cause valid items to be missed if a collider is encountered first, and suggests looping to advance the scan boundary. Additionally, it recommends an efficiency improvement to check for a valid item in the initial scan results before triggering the exact scan fallback.
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.
bootjp
force-pushed
the
feature/redis-lua-sparse-pop-fallback
branch
from
a02b568 to
d23ddc0
Compare
|
@codex review |
bootjp
force-pushed
the
feature/redis-lua-sparse-pop-fallback
branch
from
d23ddc0 to
5baaf5f
Compare
|
@codex review |
|
Addressed the sparse-list exact scan findings: the exact fallback now loops past prefix-collider keys, the initial physical-limit result is checked before falling back, and LPOP/RPOPLPUSH tombstone-heavy prefix-collider coverage was added. @codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.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: 5baaf5f1f3
ℹ️ 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".
bootjp
force-pushed
the
feature/redis-lua-sparse-pop-fallback
branch
from
5baaf5f to
fefd809
Compare
|
Addressed the remote physical-limit finding: exact sparse-list fallback is now gated by an optional store decider, so LeaderRoutedStore/ShardStore allow it only on local physical-limit scans and keep remote synthetic truncation fail-closed. Added a synthetic physical-limit regression test. @codex review |
|
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". |
|
Aligned ElasticKV proxy socket timeouts so backend read/write timeouts no longer fire before the ElasticKV Redis dispatch budget or the configured secondary write budget. Added unit coverage.\n\n@codex review |
|
Replaced best-effort blocking-command warmup for zset pops with deterministic secondary replay: BZPOPMIN/BZPOPMAX primary results are now mirrored as strict ZREM operations, and XREAD BLOCK no longer consumes secondary write capacity. Added proxy tests and audited DualWriter.Blocking callers.\n\n@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! 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". |
|
Implemented the remaining dual-write stability fixes observed during production testing:
Validation:
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e409795c6
ℹ️ 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".
| } | ||
| switch classifySecondaryWriteError(err) { | ||
| case "retry_limit", "write_conflict", "txn_locked": | ||
| case "retry_limit", "write_conflict", "txn_locked", "not_leader": |
There was a problem hiding this comment.
This retry path only covers single-command writeSecondary calls; MULTI/EXEC secondary replay goes through replaySecondaryPipeline and Backend.Pipeline, which never calls this predicate or refreshes the leader, and LeaderAwareRedisBackend.Pipeline returns nil for Redis reply errors so a NOTLEADER reply can be ignored. In dual-write mode during leader churn, a primary transaction can commit while the secondary transaction is rejected by the stale ElasticKV node without the new refresh/retry behavior, leaving the backends divergent.
Useful? React with 👍 / 👎.
| if !isNotLeaderError(cmd.Err()) { | ||
| return cmd | ||
| } | ||
| b.RefreshLeaderNow(ctx) | ||
| return b.doOnce(ctx, args...) |
There was a problem hiding this comment.
When ElasticKV is the primary backend, this retries any Redis error that isNotLeaderError classifies by substring, not just the adapter's real NOTLEADER replies. A Lua script can perform a write and then return redis.error_reply("not leader"); Redis does not roll back those script writes before returning the error, so this automatic retry can execute the script a second time and duplicate side effects even though no leader change happened.
Useful? React with 👍 / 👎.
| return item, true, nil | ||
| } | ||
| if left { | ||
| startKey = scanStartAfterKey(kvs[0].Key) |
There was a problem hiding this comment.
When the exact fallback skips a visible prefix-collision key during a left pop in a sharded deployment, advancing startKey to the collision key plus a NUL means the next ShardStore.ScanAt routes using that mutated internal key, not the original list key. Because shard routing extracts the user key from the scan start, the next page can be sent to the collider's shard and miss the buried real item, causing the Lua pop path to treat a non-empty sparse list as empty.
Useful? React with 👍 / 👎.
|
Addressed the latest dual-write production failure:
Validation:
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3df9239e81
ℹ️ 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".
| delta, err := c.newLuaStreamDelta(key) | ||
| if err != nil { | ||
| return nil, true, err | ||
| } | ||
| st.delta = delta |
There was a problem hiding this comment.
When a script has already changed this key's logical type without loading a stream state (for example SET k v; XADD k ..., or DEL k; XADD k ... where k was a string at startTS), this branch still builds a delta from storage at startTS instead of consulting cachedType/deleted. That lets XADD either succeed against a script-local string or recreate a stream without deleting the old physical rows because the delta commit uses preserveExisting, so the script can return a successful stream ID while the key remains the previous type. Please fall back to the materialized path or reject based on the script-local cached type before creating the delta.
Useful? React with 👍 / 👎.
| if diff > 0 { | ||
| delta.trimCount += int(diff) | ||
| } |
There was a problem hiding this comment.
When one Lua script issues multiple XADD ... MAXLEN calls on the same unmaterialized stream, trimCount can include entries appended earlier in the same script (e.g. empty stream, two XADD MAXLEN 1 calls). The later commit converts this count only into buildXTrimHeadElems at c.startTS, which cannot see those in-script appends, so the older appended entries survive and the committed meta length is too large. The delta path needs to delete/skip appended entries that fall off the MAXLEN window or materialize before trimming them.
Useful? React with 👍 / 👎.
| if typ != redisTypeNone && typ != redisTypeStream { | ||
| return nil, wrongTypeError() | ||
| } | ||
| legacyCleanup, meta, metaFound, err := c.server.streamWriteBase(c.scriptCtx(), key, c.startTS) |
There was a problem hiding this comment.
When keyTypeAtExpect returns redisTypeNone because the existing key is TTL-expired but not yet swept, this still calls streamWriteBase, which reads raw stream metadata and the later delta commit preserves existing physical rows/TTL. In that case a Lua XADD can return a successful ID while the old expired TTL continues to hide the newly written stream (and stale entries can be reused), whereas the previous materialized path rewrote the key and cleared the stale state. Force a full rewrite/TTL cleanup when the filtered type is none but physical data may remain.
Useful? React with 👍 / 👎.
| if delta.forceEmpty { | ||
| meta.Length = 0 |
There was a problem hiding this comment.
If an unmaterialized Lua script does XADD ... MAXLEN 0 and then appends to the same stream again without materializing it, forceEmpty remains set from the first call. This commit branch then writes metadata with length 0 regardless of the later appends, so the script can successfully return an ID for an entry that should remain but is hidden by the stream metadata. The delta state needs to distinguish entries trimmed by the MAXLEN 0 call from entries appended afterwards, or reset the force-empty marker before subsequent appends.
Useful? React with 👍 / 👎.
|
|
||
| func blockingReplayCommand(cmd string, _ [][]byte, resp any) (string, []any, bool) { | ||
| switch strings.ToUpper(cmd) { | ||
| case "BZPOPMIN", "BZPOPMAX": |
There was a problem hiding this comment.
When clients use mutating blocking list commands in dual-write modes (for example BLPOP/BRPOP with an item already available), Blocking now calls this replay helper and skips the secondary write whenever it returns false. Because the helper only handles BZPOPMIN/BZPOPMAX, the popped list element remains on the secondary instead of being removed, whereas the old warmup path at least issued the same blocking command to the secondary. Add deterministic replays for the list-pop variants or keep forwarding them so dual-write state does not diverge for those commands.
Useful? React with 👍 / 👎.
|
@codex review |
|
Addressed the remaining Lua XADD/list routing review items:
Validation:
@codex review |
|
Added follow-up regression coverage for the remaining MAXLEN 0 stream-delta case: a Lua script that performs XADD MAXLEN 0 and then appends again in the same script now verifies that the later entry remains visible with correct metadata. Validation:
@codex review |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: adf5cdb508
ℹ️ 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".
| err error | ||
| ) | ||
| if scanner, ok := c.server.store.(luaRoutePinnedScanStore); ok { | ||
| kvs, err = scanner.ScanAtWithReadFence(c.ctx, startKey, endKey, 1, c.startTS, !left, 0, 0, routeStart, routeEnd) |
There was a problem hiding this comment.
When this fallback runs against ShardStore/LeaderRoutedStore, ScanAtWithReadFence treats routeStart/routeEnd as route-key-normalized bounds, but this call passes raw !lst|itm|... storage keys. The route filter normalizes returned list item keys back to the logical Redis key and then compares that logical key against these raw bounds, so sparse LPOP/RPOP exact fallback can filter out every real item and return nil on sharded ElasticKV even though an item exists; use logical route bounds rather than the storage scan keys here.
Useful? React with 👍 / 👎.
1 participant
Summary
Tests